[theCodeDrift]

Stack (root → tip):

• docs(openspec): propose unified rule fixtures #583
  • feat(rules): hand ast-grep a rules-only tree, and add the fixture converter #584
    • ➡️ feat(rules): every engine’s fixtures are case directories under tests/, judged through its own check #585 (you are here)
      • docs(agent): one fixture layout block for every authoring recipe #586

The rename and everything that only works with it, in one layer so no state in between ships.

Runner

• RULE_TESTS_DIRECTORY is tests; FixtureLayout is case-directories for every engine. Migrations 5, 9 and 10 read LEGACY_RULE_TESTS_DIRECTORY, so their behavior is unchanged.
• One case reader for every engine (rules/fixtures.ts): loose bucket files and stray tests/ entries are refused by path.
• ast-grep: rules/sg-fixtures.ts scans each case with runAstGrepScan under a one-rule config. Replaces sg test and the deleted stdin re-scan. A quiet fail/ case is re-run without scope, to say "scope excluded" or "did not match".
• Vale: each case is linted under the rule's own .vale.ini, not [*].
• Published verify envelopes now declare fixtures and findings. sg-fixture-id-matches-rule is removed.

Migration and delivery

• Migration 12 converts .tests/ for every engine, at paths each rule's scope admits, and names anything it leaves behind.
• Delivery converts an old-format served set before writing, so no Cloud change is needed. Reconcile and delivery verification treat tests/ and a leftover .tests/ as unsigned fixtures.
• The Vale 3.24 tests: rule migration writes the new layout. Corpus reference.json moves to version 4.
• This repo's .taskless/rules/ and example/ are migrated, with every case named for what it shows.

Bugs found and fixed:

• ast-grep's "note": null made verify --json fail its own schema. A schema test now runs real verify output, and I confirmed it fails without the fix.
• A malformed test YAML was converted from a partial parse.
• (review) Migration 12 deleted a test file whose case it had not written: it swallowed EEXIST and removed the source anyway. A destination now counts only if it already holds the same bytes; otherwise the source stays and is named. Case names continue across a rule's test files.

Second review round: a case the engine never read fails, whatever its bucket. ast-grep's own --inspect entity names every file a rule was applied to, and Vale gets a probe rule under the same matchers. Reproduced before the fix: a pass case under a gitignored path, holding an empty file, outside the scope, or behind ast-grep's *-crosses-/ glob dialect all passed without being read. Also: a signal-killed scan is a failure; the reader skips .DS_Store/.gitkeep and refuses an empty case and a leftover .tests/; migration 12 numbers past existing cases and finishes an interrupted run.

Typecheck, lint and the full suite pass on this layer alone (2,826 tests). 14/14 of this repo's rules pass pnpm cli verify, about 100 cases in 2.2 s.

Size: large, deliberately. The directory name is read by the runners, migration, delivery and reconcile, and splitting them would ship a layer where verify and delivery disagree. Everything separable is in #584.

Stack: #583 spec → #584 groundwork → #585 the tests/ rename → #586 recipes & archive. Every layer is green on its own; land it with gh stack merge. Design: openspec/changes/unified-rule-fixtures/design.md (archived on #586).

[theCodeDrift]

@claude /review

[github-actions]

Claude finished @theCodeDrift's task in 5m 48s —— View job

---

Review complete

Review mode: incremental — read 0 prior review thread(s) before reviewing. .prior-review.json contained no review summaries and no inline review threads, only the @claude /review trigger comment itself, so this is the first review of this PR. Everything below is new.

Todo list

• Read .prior-review.json (empty of prior findings — first review of this PR)
• Fetch PR diff and metadata
• Review runner/fixture-reader changes (rules/fixtures.ts, rules/sg-fixtures.ts)
• Review migration 12 and delivery/reconcile changes
• Review Vale rule-level .vale.ini linting change
• Review schema/bug fixes (note: null, malformed YAML)
• Spot-check migrated .taskless/ fixture content
• Post final review comment

Overall assessment

This is a well-executed, large-but-cohesive rename. rules/fixtures.ts genuinely unifies what used to be three divergent fixture readers, sg-fixtures.ts's scoped/unscoped rescan to distinguish "scope excluded" from "did not match" is a real improvement over the old sg test + stdin-rescan approach, and the schema (note: null) and malformed-YAML bugs are each backed by a regression test that fails without the fix. Test coverage is thorough — migrate-fixture-case-directories.test.ts, sg-fixtures.test.ts and the rewritten vale-verify.test.ts exercise the scope-exclusion messaging, isolation from broken sibling rules, idempotent reruns, and schema round-tripping of real CLI output.

[New] One finding, posted inline on packages/cli/src/filesystem/migrations/0012-fixture-case-directories.ts:

migrateSg converts each legacy .tests/*.yml file independently, and convertSgTestFile names cases purely by their position within that one file (pass-01, fail-01, …). A rule with more than one legacy test file sharing the same id: — a state this codebase already handles elsewhere (migration 9's .tests/<id>-*-test.yml glob, and the agent instructions that write dated files like .tests/no-eval-YYYYMMDD-test.yml) — produces colliding case names across files. writeNew's wx-flag EEXIST swallow silently drops the second file's content, and apply still reports success, so the second source file is deleted with no notice. Net effect: a real fixture can be silently lost during migration, with the run reporting a clean applied: [12]. See the inline comment for the exact mechanics and a concrete repro, and the contrast with the taken-set collision handling the Vale 3.24 migration already uses for the same category of problem.

No other correctness issues found in the runner, migration, delivery, reconcile, schema, or test changes.

[theCodeDrift]

@claude /review

This is a follow-up review after the first review-fix round (6a89cc0e, case names claimed across a rule's test files). Most of the ~195 files are renamed fixtures under .taskless/. Please skip those and spend the review on packages/cli/src. Confidence is lowest in the four areas below. I reproduced findings 1 and 2 against this branch.

1. Project .gitignore hides fixture files (reproduced). sg-fixtures.ts scans each case with cwd = the case directory, and scan.ts passes only --no-ignore hidden, so the project's .gitignore still applies. Repro: a git repo with .gitignore containing dist/, and a rule with files: ['**/dist/**'] and fail case tests/fail/b/dist/x.js holding eval(1). The verdict is quiet: "no-match", meaning "the rule did not match", because the unscoped re-run is also gitignored. Remove the ignore line and the case fires. A pass case goes vacuously green the same way. Migration 12 makes this worse, because deriveFixturePath builds paths from the scope (build/**, dist/**), so it can write fixtures that git will not commit. Decide whether this needs --no-ignore for fixture scans, or at least a distinct message.

2. readRuleFixtures strictness (reproduced). A .DS_Store in tests/ fails the rule with "is not a fixture bucket", and in tests/pass/ it fails as a "loose file". .gitkeep is treated the same way. Check whether dotfiles should be ignored here. Also check a case directory with zero files: git drops it on clone, and locally a zero-file pass case passes vacuously and still counts toward coverage: "both".

3. convertLegacyFileSet during delivery (reproduced). claimed starts empty and is not seeded from tests/ paths already in kept. In a set that mixes both layouts, the converted tests/fail/fail-01/fail-01.js merges into an existing tests/fail/fail-01/ case, and nothing deduplicates paths. An unconvertible legacy file, such as .tests/__snapshots__/*.yml written by sg test, is moved to tests/__snapshots__/…, which verify then refuses as a stray. The warning says it was "written as served", but its path changed. Also check that verify-delivery.ts exempting .tests/ from signatures cannot be used to slip an unsigned rule-shaped file in. Delivery also orders cases by served order, while the migration orders them by name.

4. Migration 12 after an interrupted run. Say file A is converted and removed, and the run then dies before file B. On the rerun, claimed is rebuilt only from the remaining files, so B claims fail-01 again, collides with A's output, and is kept with a notice on every run. That is not data loss, but the migration can never finish. Also exercise .tests/ containing only .gitkeep, a non-yml file, a __snapshots__/ directory, a sg rule directory with no rule file, an existing runtime destination, and whether a thrown error leaves the migration unrecorded.

Lower priority: the parseDocument edit removing files/ignores when they come from utils or constraints, Vale's buildIsolatingConfig re-run, and scratch cleanup when writeOneRuleConfig throws.

[github-actions]

Claude finished @theCodeDrift's task in 8m 48s —— View job

---

Review complete

Review mode: incremental — read 1 prior review thread(s) before reviewing. The only prior thread (on packages/cli/src/filesystem/migrations/0012-fixture-case-directories.ts, the cross-file case-name collision / silent-deletion finding) is isResolved: true and carries theCodeDrift's reply pointing at the fix in 6a89cc0e. Treated as ADDRESSED and not repeated — I re-read the current migrateSg/apply/writeNew and confirmed the fix is in place (shared claimed across a rule's legacy files, a destination only counts as written when it already holds the same bytes, a mismatched destination keeps the source and names it).

Todo list

• Read .prior-review.json (1 resolved thread, addressed by 6a89cc0e)
• Fetch PR diff and metadata (via gh pr diff/gh pr view; gh pr view --json files truncates at 100 entries and is dominated by .taskless/ alphabetically — used gh pr diff for the real file list, which is wider: fixtures.ts, sg-fixtures.ts, scan.ts, fixture-convert.ts, deliver.ts, verify-delivery.ts, vale/verify.ts, migration 12, etc. are all genuinely part of this PR's diff against 2-verify)
• Investigate finding 1: .gitignore hiding fixture files — confirmed, and found it also applies to Vale, not just ast-grep
• Investigate finding 2: readRuleFixtures strictness on dotfiles / zero-file cases — confirmed
• Investigate finding 3: convertLegacyFileSet seeding/dedup — confirmed, with a sharper failure mode than described
• Investigate finding 4: migration 12 interrupted-run behavior — confirmed, a distinct gap from the already-fixed one
• Review lower-priority items — checked, no issues found (see below)
• Post final review comment

Per the trigger comment, .taskless/ fixture renames were skipped; everything below is about packages/cli/src.

The four confidence areas — all four reproduce from reading the code

1. .gitignore hides fixture files — confirmed, and it's not ast-grep-only. sg-fixtures.ts's scanCase runs with cwd = fixture.root and paths = []; scan.ts's sgWalkArgv never passes --no-ignore vcs, so ast-grep's walker keeps honoring .gitignore even rooted at a case directory (the ignore crate climbs to ancestor .gitignores by default). The unscoped re-run that's supposed to distinguish "scope excluded" from "did not match" runs the exact same way, so it's equally blind — a gitignored fixture reports quiet: "no-match" instead of naming the real cause. Vale has the identical gap through a different mechanism: vale/verify.ts's runCase also calls runVale with no paths, which makes vale/run.ts treat it as a whole-project walk and apply listGitIgnoredEntries/gitIgnoredExclusionGlobs against the case directory. Both inline comments have the full mechanism and a concrete repro.

2. readRuleFixtures strictness — confirmed. Neither bucketCases nor the stray check in readRuleFixtures (fixtures.ts) exempts dotfiles, so a .DS_Store or .gitkeep under tests/ or tests/pass/ throws and discards every real case for that rule, not just the stray. Separately, nothing requires a case directory to hold at least one file, so an empty case (which git doesn't track, so it reappears/vanishes depending on checkout) passes vacuously and still counts toward coverage: "both".

3. convertLegacyFileSet — confirmed, and worse than described. Its fallback for anything unconvertible (moveAsServed) unconditionally relocates the file from .tests/<rest> to tests/<rest>, preserving nested paths like a stale sg test __snapshots__/ directory. readRuleFixtures requires tests/ to hold only pass//fail/, so this doesn't just leave a misnamed leftover — it hard-fails fixture reading for the whole rule, discarding every legitimate case alongside the one bad leftover. Migration 12 avoids this entirely by leaving unconvertible files under the legacy .tests/ directory, which readRuleFixtures never looks at — the two "what to do with something we couldn't convert" fallbacks disagree about which tree is safe to drop a leftover into, despite the module doc describing the converters as shared. Separately, claimed is seeded only from files processed in the same loop, never from kept (paths already in the new layout), so a served set mixing both layouts can merge an extra file into an existing case directory without assessDelivery's exact-duplicate-path check catching it. I couldn't anchor this one inline — fixture-convert.ts isn't touched by this PR's diff (it's from an earlier stacked layer), so GitHub won't resolve a comment on it; full writeup is above and would apply equally to a future PR that touches that file.

4. Migration 12 after an interrupted run — confirmed, a new gap distinct from the fixed one. claimed in migrateSg/migrateVale is in-memory per run, never seeded from case directories that already exist under ruleDirectory/tests/ from a prior run. A rule with two legacy files, interrupted between files (first fully converted and deleted, second untouched) gets stuck: on rerun, the second file's snippet claims fail-01 again (not fail-02), collides with the first file's already-written destination, and is permanently kept with a notice on every subsequent run — no progress is possible without manual intervention. This is a narrower gap than the one 6a89cc0e fixed (that one was within-run; this one is across runs) and isn't covered by the existing "finishes a conversion whose files were written but whose source stayed" test, which only exercises a single file's own content matching on retry.

Lower-priority items — checked, nothing found

• parseDocument edit removing files/ignores (sg-fixtures.ts, building the unscoped config): files:/ignores: are top-level ast-grep keys unrelated to utils:/constraints: (those are pattern-composition constructs, per constraints.ts's sg-regex-needs-kind entry), so deleting the two top-level keys does correctly disable the rule's scope. No issue found.
• buildIsolatingConfig re-run (Vale): mechanically sound — it's the same quiet-case rerun pattern as ast-grep's, and inherits the gitignore gap noted in finding 1, but nothing additional beyond that.
• scratch cleanup when writeOneRuleConfig throws (sg-fixtures.ts): the try/finally around the whole scoped/unscoped-write-and-scan block means rm(scratch, { recursive: true, force: true }) runs regardless of where inside the try an error originates. No issue found.
• Delivery ordering cases by served order vs. migration ordering by name: confirmed as a real inconsistency, but cosmetic (affects which snippet gets fail-01 vs fail-02 between the two code paths for otherwise-identical content) rather than a correctness bug.
• verify-delivery.ts exempting fixtures from signatures: describeUnsafePath's path-containment checks in assessDelivery apply uniformly to every delivered path regardless of fixture status, so the signature exemption only waives content-integrity signing (documented as deliberate — "fixtures are never signed"), not path containment. No way found to use it to smuggle a file outside the rule directory.
  (branch: openspec/unified-rule-fixtures-3-migrate)

[github-actions]

[New] Fixture scans inherit the real project .gitignore, so a scope targeting build/ignored output silently never fires

scanCase runs runAstGrepScan(fixture.root, [], ...) — cwd is the case directory, paths is empty. sgWalkArgv([]) treats that as a whole-project walk (isWholeProjectWalk([]) === true in walk-scope.ts), which adds the .taskless//.git/ --globs exclusions, but never passes --no-ignore vcs (by design, per the long comment in scan.ts's sgWalkArgv). That means ast-grep's own ignore-crate walker keeps honoring .gitignore even when rooted at tests/fail/<case>/: the crate climbs to ancestor directories looking for .gitignore files (this is the crate's default parents behavior), so a project-root .gitignore entry like dist/ still excludes tests/fail/<case>/dist/x.js from the scan.

Confirmed repro (matches the PR author's own repro): a rule with files: ['**/dist/**'] and a fail/ case whose only file sits under a case-relative dist/ that the project's .gitignore also excludes. The scoped scan finds nothing (expected, scope matched). The unscoped re-run at line ~178 (scanCase(cwd, ruleId, fixture, unscoped)) is run the exact same way — same cwd, same empty paths — so it is also gitignore-filtered and also finds nothing. outcome.quiet is therefore computed as "no-match" (unscopedFindings.length > 0 ? "scope-excluded" : "no-match"), telling the author "the rule did not match" when the true cause is that the case's own files are invisible to the walker. A pass/ case under the same path passes vacuously for the identical reason.

Worth noting this is not ast-grep-specific: vale/verify.ts's runCase has the same shape (runVale({ cwd: fixture.root, ... }), no paths), and Vale's runVale computes listGitIgnoredEntries(options.cwd) and builds --glob exclusions from it whenever isWholeProjectWalk is true — which it is here too, for the identical reason. So both engines' fixture verification are blind to files that live under a gitignored case-relative path, and both engines' "quiet fail case" re-run inherits the same blindness, misreporting a gitignore exclusion as "the rule did not match."

Compounding factor: deriveFixturePath (fixture-convert.ts) derives case paths straight from the rule's own files:/ignores: globs, so migration 12 and delivery's convertLegacyFileSet can mechanically produce exactly this shape for any rule scoped at build/**, dist/**, coverage/**, etc. — i.e. the rules most likely to hit this are the ones whose whole purpose is to say something about build output.

This looks like a real scoping gap in fixture verification, not a hypothetical: it would benefit from either excluding .gitignore effects for fixture scans (mirroring what an explicit user-named path already gets — isWholeProjectWalk already carves out that case for a real whole-project check, but [] as "no explicit path" collapses the "run this one case" intent into "whole project" here), or at minimum a third quiet value distinguishing "gitignored" from "no-match" so the error message points at the right thing.

[github-actions]

[New] readRuleFixtures/bucketCases have no dotfile exemption, and no empty-case guard

Two related strictness gaps in this newly-unified reader:

1. Dotfiles are treated as strays/loose files, not ignored. bucketCases (line 128) flags any non-directory entry in pass//fail/ as an error (A fixture case is a directory: …), and readRuleFixtures's stray check here (line 171) flags any tests/ entry that isn't pass/fail. Neither skips .DS_Store, .gitkeep, or any other dotfile. A macOS-authored tests/.DS_Store, or a tests/.gitkeep someone adds to keep an otherwise-empty tests/ tracked by git, breaks verify for the entire rule with "is not a fixture bucket" / "is not a directory" — not a warning, a hard throw that runSgFixtures/verifyValeRule/the runtime verifier all catch and turn into coverage: "none" with an error, discarding every real case the rule actually has. Given this reader is reached from ordinary developer machines (not just CI), .DS_Store showing up here is a realistic, not hypothetical, occurrence.

2. A zero-file case directory passes vacuously and still counts toward coverage: "both". bucketCases only requires each pass//fail/ entry to be a directory — nothing checks it contains at least one file. Git does not track empty directories, so a case accidentally left with no files (e.g. a bad migration, a .gitignore excluding everything inside it — see the sibling finding on sg-fixtures.ts) silently clones as an absent directory for one contributor and as an empty, vacuously-passing one for another, and classifyCoverage cannot tell "this case exercises nothing" from "this case exercises something and found nothing", because a pass/ case passing on zero files and a pass/ case passing on real files that stayed clean produce the identical outcome.

Neither is the collision/data-loss bug already fixed in 6a89cc0e; both are new to this unification (the old three per-engine readers had their own, different rejection rules, per this file's own doc comment above).

[github-actions]

[New] A rule interrupted mid-migration, with more than one legacy test file, can get permanently stuck

claimed is a fresh, in-memory Set per call to migrateSg (and likewise migrateVale) — it is never seeded from case directories that already exist on disk under ruleDirectory/tests/, only built up across the legacy files processed within this one run (the fix from 6a89cc0e addresses exactly that: collisions within a single run).

That leaves a gap one level up. Suppose a rule has two legacy files, no-eval-20260101-test.yml (invalid: [eval(a)]) and no-eval-20260215-test.yml (invalid: [eval(b)]), and the process is killed after the first file is fully converted and its source removed, but before the second file is even opened — e.g. a crash or OOM between iterations of the for (const entry of await entriesOf(legacy)) loop, not a partial write within one iteration (that case is the one 6a89cc0e's tests cover: "finishes a conversion whose files were written but whose source stayed").

On the next run: entriesOf(legacy) now returns only the second file (the first's source is already gone). claimed starts empty again, so the second file's invalid: snippet claims fail-01 (the first available ordinal) instead of fail-02. writeNew finds tests/fail/fail-01/fail-01.ts already holds eval(a)\n from the first file, which is not byte-identical to eval(b)\n, so it returns false; apply records "no-eval: tests/fail/fail-01/fail-01.ts already exists with other content" and leaves the second source in place. Every subsequent run repeats exactly this — the in-memory claim never advances past fail-01 because nothing seeds claimed from what migration already wrote to tests/ in a prior, interrupted run. The rule is left permanently straddling .tests/ and tests/, reporting the same note on every init/upgrade, with no path to resolution short of a human manually renumbering or deleting the stuck file.

This isn't data loss (the source file survives, per the 6a89cc0e fix), but it is a correctness gap in the "a rerun after a partial failure finishes the job" claim in this function's own doc comment (lines 58-61) — it only holds for a crash within processing one legacy file, not a crash between files. Worth seeding claimed by scanning the existing tests/pass/+tests/fail/ case names already under ruleDirectory before the loop starts, the same way the loop already knows to skip a destination that holds the right bytes.

[github-actions]

[New] Same .gitignore-hides-fixture gap as ast-grep's, through Vale's own gitignore handling

runCase calls runVale({ cwd: fixture.root, configPath, timeoutMs }) with no paths. In vale/run.ts, options.paths ?? [] then makes isWholeProjectWalk([]) true, which triggers wholeProjectTargets(options.cwd) and listGitIgnoredEntries(options.cwd) → gitIgnoredExclusionGlobs(...) fed into Vale's --glob. That machinery exists specifically to keep a real whole-project check out of dist/, vendored trees, etc. (per the comment at run.ts:618-626), but here options.cwd is the fixture case directory, not the project root — so a project .gitignore entry like dist/ excludes tests/fail/<case>/dist/x.js from the Vale run exactly the way it excludes it from the ast-grep run in sg-fixtures.ts (see that inline comment for the full repro). The same applies to the unscoped re-run a few lines below (built from buildIsolatingConfig): it goes through runCase the same way, so it inherits the same blind spot and would also misreport a gitignore exclusion as "the rule did not match" rather than naming the real cause.

Flagging this mainly so it's considered together with the ast-grep instance rather than fixed once and assumed solved for both engines — the mechanism differs (native ignore-crate behavior vs. this file's explicit git-status-derived glob) but the user-visible effect (a fail/ case under a case-relative gitignored path goes quiet, a pass/ case passes vacuously) is identical.