Extract PrethinkContextRecipe base class for cycle-2-deferred recipes#17
Draft
kmccarp wants to merge 2 commits into
Draft
Extract PrethinkContextRecipe base class for cycle-2-deferred recipes#17kmccarp wants to merge 2 commits into
kmccarp wants to merge 2 commits into
Conversation
This module has a small family of ScanningRecipes that share the same shape: they declare causesAnotherCycle()=true, scan for existing .moderne/context/* paths, and defer their real work to cycle 2 because the data tables they read are populated by other recipes' visitors during cycle 1's edit phase. Each such recipe also has a structural problem: it contributes zero changes in cycle 1 (generate returns empty, visit returns the tree unchanged), so RecipeScheduler.runRecipeCycles can break the loop after cycle 1 if no other recipe in the pipeline made a change. When that happens, the recipe never reaches cycle 2 and never produces output. The existing recipes have ad-hoc workarounds (a placeholder file written from generate(), incidental content updates from another recipe) that are fragile and corpus-dependent. PrethinkContextRecipe absorbs the shared structure: a fixed Set<Path> accumulator, a scanner that populates it, causesAnotherCycle, and a cycle-1 visitor that signals the scheduler to run another cycle by calling ctx.putMessage(Prethink.CYCLE_TRIGGER, true). RecipeRunCycle treats any ExecutionContext mutation as the recipe contributing a change for cycle-2 enrollment. CursorValidatingExecutionContextView's allow-list requires the io.moderne. prefix. Subclasses override only cycle2Visit(Tree, Accumulator, ExecutionContext), which the base class invokes only on cycle 2 (cycle 3+ returns the tree unchanged so cycle2Visit implementations don't have to be idempotent). The accumulator shape is intentionally fixed to a single Set<Path>. Recipes needing additional per-run state should keep that state outside the accumulator (or stay outside this base class) until a need to generalize emerges.
…Recipe
Both recipes match the contract the new base class encodes: they read
data tables in cycle 2 to export context files, they need cycle 2 to
actually run, and they each had their own copy of the scanner +
accumulator + causesAnotherCycle + cycle-trigger boilerplate. Migrating
removes ~30-50 lines of duplication per recipe and centralizes the
cycle-2 contract in one place.
ExportContext: extends PrethinkContextRecipe; deletes its own Accumulator,
causesAnotherCycle, getInitialValue, and getScanner; moves its old
getVisitor body into cycle2Visit. Its generate() keeps the cycle == 1
early return; that's a no-op now (the base class doesn't call generate
differently per cycle) but is harmless and self-documenting.
GenerateCalmArchitecture: same migration. Additionally drops the
cycle-1 placeholder hack from generate() that wrote a "{}" file at
.moderne/context/calm-architecture.json purely to trick the scheduler
into running cycle 2 -- the base class triggers cycle 2 directly so the
placeholder is dead code. The "cycle != 2" guard in its old visitor is
now redundant with the base class and removed.
Two existing GenerateCalmArchitectureTest tests asserted the placeholder
behavior and are updated for the new (cleaner) cycle counts:
generatesNothingWithNoDataTables now expects 0 cycles with changes
(previously 2: placeholder created, placeholder deleted), and the
data-table-populated tests expect 1 cycle with changes (the file gets
created in cycle 2 only).
Two new tests, one in ExportContextTest and one in
GenerateCalmArchitectureTest, exercise the regression that motivated
this whole change: when calm-architecture.json (or any existing context
file) is already present in the LST and no other recipe makes a
tree-modifying change in cycle 1, RecipeScheduler.runRecipeCycles
terminates the loop after cycle 1, and the existing context file would
never be updated. Each test invokes Recipe.run directly with
maxCycles=3, minCycles=1 to mirror the production CLI scheduler call --
RewriteTest.rewriteRun cannot represent this scenario because its
expectedCyclesThatMakeChanges assertion contradicts the success criterion.
Both tests fail without the base class's cycle-1 trigger.
Closed-source recipes that share this shape (e.g. GenerateCalmMermaidDiagram
in moderneinc/rewrite-prethink) can migrate in a follow-up after this lands
and a release is cut. FindCalmRelationships uses a different accumulator
shape (Set<String> of in-repo class names) and stays outside this hierarchy.
b681088 to
27fe4b6
Compare
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background / problem
When the build-time LST manifest includes existing
.moderne/context/*files (the post-moderne-cli#3661 reality), the recipes in this module that defer real work to cycle 2 stop firing for repos that already have those files committed. Customer-facing symptom: runningUpdatePrethinkContextNoAiStarteracross a multi-repo corpus generates context for some repos and silently skips others.The mechanism:
ExportContext.getVisitorreturns the tree unchanged on cycle 1 because data tables are populated by other recipes' visitors during cycle 1's edit phase, after this recipe'sgenerate()has already run.ExportContext.generatealso defers to cycle 2 for the same reason. The recipe contributes zero changes in cycle 1, and the only thing historically keeping the cycle alive wasGenerateCalmArchitecture's placeholder hack (writing a{}file at.moderne/context/calm-architecture.jsonpurely to trick the scheduler into running cycle 2). Once.moderne/context/calm-architecture.jsonis in the LST, the placeholder check fails, the placeholder isn't written, andRecipeScheduler.runRecipeCyclesterminates the loop after cycle 1.GenerateCalmArchitecturehas the same fragility, just expressed differently.The minimal fix for that specific bug is in #16: each affected recipe sets a marker on the
ExecutionContextfrom cycle 1 to keep the scheduler loop alive. This PR is an alternative that fixes the same bug and extracts the shared shape into a base class so future recipes don't have to remember to wire it up.Solution
Introduce
PrethinkContextRecipe, a public abstractScanningRecipethat absorbs the shared structure of recipes which write to data tables in cycle 1's edit phase and need to read those data tables in cycle 2 to export context files. The base class:Set<Path> existingContextPathsaccumulator and a scanner that populates it.causesAnotherCycle(),getInitialValue(),getScanner(), andgetVisitor()final.ctx.putMessage(Prethink.CYCLE_TRIGGER, true).RecipeRunCycletreats anyExecutionContextmutation as the recipe contributing a change for cycle-2 enrollment. Theio.moderne.prefix is required byCursorValidatingExecutionContextView's allow-list.cycle2Visit(Tree, Accumulator, ExecutionContext)template method. On cycle 3+ it returns the tree unchanged so implementations don't have to be idempotent across cycles.ExportContextandGenerateCalmArchitecturemigrate to extendPrethinkContextRecipe. Their oldgetVisitorbodies move intocycle2Visit.GenerateCalmArchitecture's placeholder hack and its visitor'scycle != 2guard become dead code and are removed.Relationship to #16
This PR and #16 fix the same bug. #16 does it with the smallest possible change (per-recipe
ctx.putMessagecalls); this PR does it by introducing the base class and migrating the recipes to extend it. Either can ship on its own. If reviewers prefer the minimal fix, merge #16 and close this. If reviewers prefer the framework refactor, merge this and close #16.Test plan
requestsAnotherCycleWhenNoOtherRecipeMakesChangesInCycle1regression test inExportContextTest, andupdatesExistingCalmFileEvenWhenNoOtherRecipeMakesChangesInCycle1inGenerateCalmArchitectureTest. Both invokeRecipe.rundirectly withmaxCycles=3, minCycles=1-- matching the production CLI scheduler call -- becauseRewriteTest.rewriteRuncannot represent this scenario throughexpectedCyclesThatMakeChangeswithout contradicting the success criterion. Both tests fail without the base class's cycle-1 trigger.GenerateCalmArchitectureTestcycle counts updated to reflect the placeholder removal:generatesNothingWithNoDataTablesnow expects 0 cycles with changes (previously 2: placeholder created, placeholder deleted), and the data-table-populated tests expect 1 cycle (the file gets created in cycle 2 only)..moderne/context/*entries infix.patchgo from21 + 0 + 0(broken) to21 + 16 + 18(each repo now produces output), byte-identical end state to the pre-#3661 baseline.What's NOT in scope
GenerateCalmMermaidDiagraminmoderneinc/rewrite-prethink) will migrate in a separate PR there once this lands and a release is cut.FindCalmRelationships(also closed-source) has the samecausesAnotherCycle()=true-without-a-trigger latent bug, but uses a different accumulator shape (Set<String>of in-repo classes, not paths). Stays outside this hierarchy.PrethinkContextRecipeto support custom accumulator shapes. Deferred until a recipe actually needs it.