feat(test): report ast-grep fixture findings

[thecodedrift]

test reported the findings a rule's fixtures produced for Vale and runtime, and an empty array for ast-grep. That was truthful, and it left the half of #386 that matters most for ast-grep unfixed: a rule whose message interpolates its metavariables can name them in the wrong order, fire on every invalid: snippet, stay quiet on every valid: one, and be reported green. ast-grep test never renders the message, so nothing could see it.

The route, and what ruled out the alternatives

Each fixture snippet is replayed through ast-grep scan -r <rule> --stdin --json=stream. Three alternatives were measured against the vendored 0.45.3 binary first:

Route	Why not
Read findings out of sg test	ast-grep test --help offers --filter, --skip-snapshot-tests, --update-all, --interactive, --include-off, --color and nothing else. No --json, no output-format flag. Its only machine-readable output is the summary line parseTestSummary already reads.
check over the rule's .tests directory, as Vale does	pnpm cli check .taskless/rules/sg/ci-uses-workspace-cli/.tests --json returns {"success":true,"results":[]}. The fixtures are inline YAML scalars under valid:/invalid:, not documents, so there is nothing for a walk to find.
Materialise each snippet as a temp file and scan it	Needs a language: → extension mapping the CLI does not own. See below.

--stdin was the measurement that changed the design:

printf 'console.log("hi");' | ast-grep scan -r rule.yml --stdin --json=stream
{"text":"console.log(\"hi\")",…,"file":"STDIN","language":"TypeScript",
 "message":"avoid log on console in ",…}

The message comes back rendered, with metavariables interpolated — which is the entire point of the feature.

The language mapping: there isn't one, and that is the result

No language: → extension mapping was added, because --stdin does not need one. ast-grep parses the rule's own language: key and picks the grammar itself.

This was the main risk flagged going in, and it was a real one. There is no such list anywhere in this repo today: packages/cli/src/schemas/ast-grep-rule.ts requires language as a key and never validates its value. The set belongs to the binary, and language: takes ast-grep's own spelling — this repo's own ci-uses-workspace-cli uses Yaml, not yaml. A mapping we maintained would drift and silently scan a snippet as the wrong language. Not writing one is strictly better than writing one, so the module documents why it does not exist rather than leaving the next person to re-derive it.

Two further properties were measured rather than assumed:

• files: globs do not suppress a stdin scan. ci-uses-workspace-cli restricts itself to .github/workflows/*.yml, and the stdin document is named STDIN. It fires on the same snippet identically with and without the files: key — the glob filters the file walk, which stdin bypasses. Had this gone the other way, every path-scoped rule would have silently reported nothing.
• -r isolates a rule ast-grep cannot parse. An unrecognised language: exits 8 with Fail to parse yaml as RuleConfig and emits no JSON. Because -r loads exactly one rule file rather than the assembled config, that cannot take any other rule's report down with it — which is precisely the silent config-wide abort CLAUDE.md warns about.

What file reports

The test YAML that declares the snippet, cwd-relative and POSIX, matching what Vale reports. Not a temp path and not ast-grep's STDIN: both are unopenable, and the author's next move after reading a wrong message is to go and edit that snippet.

The position maps back to the snippet's real line and column in that file, better than the <test-file>.yml:invalid[0] the earlier exploration proposed. yaml's parseDocument gives each scalar's byte range; the block scalar's content starts on the following line and every content line carries the same stripped indentation, so file line is contentStart + snippetLine and file column is snippetColumn + indent. Verified against a real fixture: the reported …-test.yml:33:9 lands exactly on run: npx @taskless/cli check.

This is done only for literal block scalars (|), the spelling ast-grep's own test files use. A folded scalar (>) joins lines and a plain or quoted scalar can carry escapes, so in neither is snippet line N file line start+N. Those anchor to the snippet's first line instead — still openable, and honest about the precision available, rather than reporting a confidently wrong line.

Mutation-check results

Both are reported, because a test that passes either way is worse than none.

• Message order. Swapping the rule's message from replace $SECOND with $FIRST to replace $FIRST with $SECOND — which fires in exactly the same places and which sg test reports green either way — turned 4 ast-grep tests red, including the headline reports the rendered message, so swapped metavariables cannot pass. Vale's and runtime's tests stayed green, confirming the mutation was scoped to the rule under test.
• Temp-file cleanup. Making the collector write one temp file per snippet turned the cleanup test red with expected [ 'mutant-1790199804073.ts', …(1) ] to deeply equal [].

That second test redirects the CLI's whole TMPDIR to a private directory and asserts it is empty, rather than diffing the shared tmpdir() — which other suites write to concurrently, so a diff there would have been measuring the rest of the run.

Residual limitations

• One ast-grep process per snippet. Measured at ~7ms each (15 snippets in 0.112s), so it is not worth batching, but it is linear in fixture count.
• Folded and plain scalars get snippet-start positions rather than per-line ones, as above.
• A rule whose language: ast-grep rejects reports no findings. verify is what reports the malformed rule; this path stays silent by design rather than reporting the same defect twice.
• ast-grep emits "note": null, which the test payload schema rejects (note is an optional string there). Normalised to absent in the collector rather than in toCheckResult, which would have changed the shape check --json has been emitting.

OpenSpec

ADDED, not MODIFIED. The standing scenario "The findings array is present and empty rather than absent" lists four WHENs, one being "an engine that does not surface fixture findings". After this change no engine is in that state, so the disjunct is inert — but not false, and the other three are still live and still tested. Restating a ten-scenario requirement to delete one clause of one WHEN is the operation that silently drops scenarios, so the clause is left for a change with its own reason to touch that requirement.

Archive dry-run compared requirement and scenario title sets before and after: 9 requirements / 48 scenarios before, 10 / 54 after, with an empty before-only set both times. Archived for real on this branch — it is the tip, and there is no follow-up.

Gates

pnpm build, pnpm typecheck, pnpm lint all pass. pnpm test: 106 files / 1786 tests, up from main's 1778, with none broken and no existing assertion weakened. The one standing test that asserted ast-grep reports [] was replaced with a case that is still true — a rule whose fixtures matched nothing — rather than deleted.

Vale and runtime behaviour is untouched; the shared human renderer needed no change, since it was already engine-agnostic. No --include-fixtures on check and no --verbose, both explicitly out of scope.

Fixes #386

[thecodedrift]

@claude /review

[claude]

Claude finished @thecodedrift's task in 6m 49s —— View job

---

Claude finished reviewing this PR

Review mode: incremental — read 0 prior review thread(s) before reviewing. .prior-review.json contains no reviewThreads, reviews, or prior top-level comments other than the @claude /review trigger, so this is the first review of this PR — nothing to classify as already-addressed.

• Read .prior-review.json
• Fetched the diff (gh pr diff 401) and PR metadata
• Read the new collector (sg-fixture-findings.ts) in full, plus the files it touches or depends on (inspect.ts, fixtures.ts, engines.ts, scan.ts, types/check.ts) and the two output schemas (schemas/check.ts, schemas/verify-test.ts) and the command that parses against them (commands/verify.ts)
• Posted 3 inline findings
• Posted this summary

Findings

I posted 3 inline comments, ranked by severity:

1. fix isn't normalised for a possible null, unlike note (sg-fixture-findings.ts:310) — The collector explicitly normalises note: result.note ?? undefined because ast-grep emits "note": null for a rule without one, and the test/verify payload schema (fixtureFindingSchema) requires an optional string. But fix goes through unnormalised, and checkResultSchema in schemas/check.ts already declares both note and fix as .nullable() — which only makes sense if replacement (mapped straight to fix in toCheckResult) is known to come back null too. If it does, commands/verify.ts's verifyTestOutputSchema.parse(...) (uncaught) throws for any ast-grep fixture match with no fix: transform, crashing test --json/verify --json rather than reporting. This is the highest-confidence finding — the asymmetry between the two sibling fields, one fixed and one not, is very concrete even though I couldn't run the vendored ast-grep binary in this sandbox to directly confirm the null payload.
2. A purely-numeric rule id: silently excludes its own fixture file (sg-fixture-findings.ts:286) — document.get("id") !== ruleId compares a YAML-resolved value against a string; an unquoted all-digit id (valid per isValidRuleId) parses to a JS number, so the comparison is always unequal and the file is treated as belonging to a different rule. This mirrors an identical existing gap in verify.ts's fixtureCoverage, so it's copied rather than newly invented, but it is new code reproducing the same limitation.
3. Block-literal indentation is read from the literal first line, which can be blank (sg-fixture-findings.ts:140) — YAML detects a block scalar's indentation from the first non-empty line, not the literal first line. A snippet with a leading blank line before its content would compute indent: 0 instead of the real value, silently mis-reporting the column of every finding in that snippet. Not exercised by the added tests.

Everything else held up under an 8-angle pass (line-by-line diff scan, removed-behavior audit, cross-file call-site tracing, reuse/simplification/efficiency, altitude, and a CLAUDE.md conventions check):

• The core design decision (scanning fixtures via ast-grep scan --stdin instead of sg test or a materialised temp file) is sound and the tradeoffs are well-measured in the PR body.
• files: glob and -r rule-isolation behavior claims check out against how sgWalkArgv/runAstGrepScan use the assembled config vs. this collector's per-rule invocation.
• The literal-block-scalar-only position mapping, and falling back to snippet-start position for folded/plain/quoted scalars, is a reasonable and clearly documented scope limit.
• Import grouping/ordering and direct (non-barrel) imports in the new file follow .conventions/STYLEGUIDE-CODE.md.
• The OpenSpec delta is ADDED, not MODIFIED, correctly sidestepping the "a MODIFIED block replaces the whole requirement" pitfall called out in the root CLAUDE.md.
• The sequential one-process-per-snippet design is explicitly measured and justified (~7ms/snippet) rather than an oversight.

[thecodedrift]

Re: @claude[bot] — "Claude finished @thecodedrift's task in 6m 49s — Claude finished reviewing this PR…"
#401 (comment)

Both inline findings were real and are fixed in 794ac08: the fixture-file id: is now compared as a string (and the same gap in fixtureCoverage with it), and block-scalar indent is measured from the first non-empty line. The third, on fix normalisation, did not reproduce — the vendored binary omits replacement entirely rather than emitting null, so no ZodError is possible; the measurement is on that thread.

— AI Coding Agent