test(generate): derive shared-file writers from the registry and enforce declarations - #2109
Merged
Conversation
…rce declarations GENERATION_STEP_GRAPH declared each step's shared-file writers by hand; a missing writer passed CI silently and was caught only in review (e.g. codexcli/vibe config.toml in ab61e38). Derive the multi-writer set from PROCESSOR_REGISTRY x getSettablePaths across both project and global scope, plus a getExtraSharedWritePaths contract for writes not visible as settable paths, and assert it matches the step graph so a missing declaration fails CI instead of review. Deriving surfaced three drifted declarations, now fixed: .takt/config.yaml was unregistered; permissions was wrongly listed as a kilo.json writer (it writes kilo.jsonc); and the global-only .config/devin/config.json shared write was covered only by chance. Refs dyoshikawa#2081
Owner
|
@saitota Thank you! |
Merged
This was referenced Jul 3, 2026
Merged
rudironsoni
pushed a commit
to rudironsoni/rulesync
that referenced
this pull request
Jul 10, 2026
Address the two `mid` follow-ups from PR dyoshikawa#2109 review (dyoshikawa#2110): mid#1: SHARED_WRITE_FEATURE_ORDER is hand-maintained, so a feature that later starts writing a shared config file could be silently forgotten. Add an explicit NON_SHARED_WRITE_FEATURES exclusion list (commands, skills — each writes its own dedicated per-item artifact) plus a test asserting the two lists partition ALL_FEATURES exactly. Adding a new Feature now fails the test until it is consciously classified as a shared writer or an excluded one. mid#2: settablePathsForScope early-returned from the getSettablePaths catch, which skipped the subsequent getExtraSharedWritePaths collection. A tool whose global getSettablePaths throws while it also has a global-only extra shared path would silently drop that path. Collect extra paths independently of getSettablePaths success (still fail-open per hook), and cover it with a test. Closes dyoshikawa#2110 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
2 participants
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
Which config file each feature read-modify-writes is known in two places. The tool classes know their real write targets through
getSettablePaths()and their implementations;GENERATION_STEP_GRAPHre-declares the same knowledge as opaque tokens ("reasonix-config", ...). Nothing — no type, no test — connects the two:generate.test.tsonly checks the graph's internal consistency (aredependsOnedges justified for duplicated tokens), never whether the token set matches reality.So
assertSharedFilesOrderedcan only order writers that are declared. An undeclared second writer passes silently and looks safe only while the execution order happens to be correct. And a tool adder has no reason to opengenerate.ts: the code compiles, tests pass, and output is correct under the current order — the drift surfaces only in review (e.g. ab61e38).Changes
PROCESSOR_REGISTRY×getSettablePaths(both scopes) plus agetExtraSharedWritePathscontract for writes not visible as settable paths, and assert it matches the graph — a missing declaration now fails CI, not review.dir/filekeys so declarations and derivation share one namespace..takt/config.yaml; drop permissions fromkilo.json(it writeskilo.jsonc); cover the global-only.config/devin/config.jsonwriter.No change to generated output.
Test Plan
pnpm cicheckgreen (6740 tests).Refs #2081