refactor(generate): make the step graph directly testable and harden it#2105
Merged
Merged
Conversation
Extract the generate() step graph's static shape (id, writesSharedFile, dependsOn) into an exported GENERATION_STEP_GRAPH, separate from the run closures that need a live config/logger. This lets the ordering guarantee be tested directly against the real graph instead of a hand-copied one, restores rationale comments explaining why each dependsOn edge exists, and adds a test asserting every dependsOn edge corresponds to an actual writesSharedFile overlap (or the documented rules->skills value dependency) plus a test pinning the real execution order, so a future edit that silently drops a shared-file overlap or reorders steps in a way that changes runtime behavior fails loudly.
… dedupe test stub Address low-severity review feedback: mark GENERATION_STEP_GRAPH and its writesSharedFile/dependsOn arrays readonly so the module-level singleton can't be mutated by a consumer, correct the hooks/permissions rationale comments to name the actual overlapping shared-file tokens, and share one stub run() between the two test helpers instead of duplicating the literal result.
Owner
Author
|
@dyoshikawa Thank you! |
Merged
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.
Summary
generate()'s shared-file write ordering into a declarative step graph (dependsOn+writesSharedFile), topologically sorted and validated byresolveExecutionOrder/assertSharedFilesOrdered.id,writesSharedFile,dependsOn) into an exportedGENERATION_STEP_GRAPH, separate from therunclosures that need a liveconfig/logger. This lets tests exerciseresolveExecutionOrderagainst the real graph instead of a hand-copied stand-in.dependsOnedge, explaining which shared file forces the ordering (the original PR's prose comments were dropped when the graph became declarative).dependsOnedge and asserts it corresponds to either an actual overlappingwritesSharedFiletoken or the one documented value-dependency (rulesreading theskillsstep's output) — this catches a stale or unjustified edge, and combined with the existingassertSharedFilesOrderedruntime check (which catches a missing edge for a real overlap), the two together keep the declared edges honest in both directions.mcpstep's position (2nd → 5th after the refactor) — or any future reordering that changes runtime behavior — is caught rather than silently regressing.Not addressed (left for a follow-up if needed)
writesSharedFileenumerates every shared file across all tool targets (proving there's no un-declared overlap) would require cross-referencing each feature'sgetSettablePaths()per tool target, which is a materially larger undertaking than this hardening pass. The new edge-justification test narrows the risk (no stale/incorrect edges) but doesn't fully close this gap.if (!skillsResult) throw,get()'s missing-result throw) were left as-is — they're a cheap safety net against future graph regressions and only ratedlow/nit.Test plan
pnpm cicheckpassesGENERATION_STEP_GRAPHgenerate.test.tsandresolveExecutionOrdertests still pass unmodifiedCloses #2082