fix(cli): anchor sg test count parsing, and pin ast-grep's contract

[thecodedrift]

Two coupled issues in one PR. runTests recovers pass/fail counts by regex-scanning ast-grep test's human-readable output, and nothing pins that output's shape — so the fix and the guard belong together. Pin without fix leaves the bug; fix without pin leaves the parser unguarded against a version bump.

The bug (#112)

Two unanchored regexes ran against stdout and stderr concatenated, taking the first match anywhere. A failing run echoes the offending fixture's source, so fixture text could be read as the summary:

invalid:
  - "const msg = '7 passed; 0 failed';"

reported passed = 7, failed = 0 where the truth was 0 passed; 1 failed.

It corrupted counts; it did not cause a false pass. Validity comes from the exit code, and a passing run never echoes fixture source, so the only match on a clean run is the genuine summary. A rule could not be made to verify green this way. What it produced was a failure reported with nonsense numbers — output the improve-rule loop feeds back to an agent iterating on a rule.

Fixed by anchoring to the summary line after stripping ANSI, last match wins, plus a StringDecoder per stream (the same UTF-8 chunk-boundary defect already fixed for Vale in vale/run.ts). The regression test fails against unpatched source with expected 7 to be +0.

The contract (#108)

New packages/cli/test/ast-grep-vendor-contract.test.ts — 18 cases, binary invoked directly via findSgBinary() rather than through runAstGrepScan, mirroring vale-vendor-contract.test.ts. @ast-grep/cli is exact-pinned at 0.41.0, so a deliberate bump is the only moment these assumptions can change; this is what makes that moment say something.

Two behaviors turned out different from what the issue claimed:

• Severity is five values, not four — hint, info, warning, error, and off. off is accepted in a rule but disables it, so it can never reach the stream, which is asserted. AstGrepMatch's four-value union is therefore still correct and was left unchanged, with the reasoning recorded.
• sg test splits across streams — the passing summary is on stdout, the failing one on stderr. The issue did not say which, and it matters: the parser joins the streams with a newline so the anchor holds across the concatenation.

Also newly pinned rather than assumed: exact engine-failure exit codes (6 missing config, 8 unparseable rule), that --skip-snapshot-tests is load-bearing (without it a valid invalid-case fails "No baseline found"), and that --filter is an unanchored regex — no-eval runs two rules where ^no-eval$ runs one, which is why the caller anchors it.

engine-layout.test.ts is deleted: it held only the .tests/ discovery cases, which move into the contract file where they belong alongside the rest. The stale reference in src/rules/engines.ts now points at the new location.

Verification

pnpm lint clean · pnpm typecheck clean · pnpm test 619 passing across 50 files · openspec validate --all --strict 23/0

The contract suite ran rather than skipping, and it is cheap: 18 tests in ~120 ms (0.7 s wall), despite spawning the real binary.

Found, not fixed

• --include-off is a test-only flag; scan rejects it, so off-severity rules are unreachable from runAstGrepScan. Nothing depends on it.
• runAstGrepScan still uses chunk.toString() for stderr — the same UTF-8 boundary defect, but there it only feeds an error message, and it sits outside both issues.

Fixes #108
Fixes #112

[Copilot]

Pull request overview

Fixes incorrect pass/fail count reporting for ast-grep test runs by anchoring parsing to the actual summary line (after ANSI stripping) and improving UTF-8 decoding robustness, while adding a vendor-contract test suite to pin ast-grep’s observed CLI behaviors so future version bumps fail loudly at the boundary.

Changes:

• Anchor sg test pass/fail parsing to the final summary line (stdout/stderr), stripping ANSI, and decode streams with StringDecoder to avoid UTF-8 chunk-boundary corruption.
• Add ast-grep vendor-contract tests covering --json=stream shape, severities, exit codes, sg test output/streams, filter semantics, and layout discovery behaviors.
• Remove the now-redundant engine layout test file and update the reference to the new contract test location; add a changeset for the patch.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
packages/cli/src/rules/verify.ts	Fixes sg test count extraction by parsing an anchored summary line and decoding output robustly.
packages/cli/test/verify.test.ts	Adds a regression test ensuring echoed fixture text can’t poison parsed pass/fail counts.
packages/cli/test/ast-grep-vendor-contract.test.ts	Introduces a vendor-contract suite to pin ast-grep CLI/output/exit-code semantics and layout assumptions.
packages/cli/test/engine-layout.test.ts	Deletes the old layout-only test now covered by the vendor-contract suite.
packages/cli/src/rules/engines.ts	Updates documentation reference to the new contract test file.
.changeset/anchor-sg-test-summary.md	Adds release notes for the verify count parsing fix and decoding improvement.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[thecodedrift]

@claude /review

[claude]

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

---

Review complete

• Gather context (diff, changed files, PR description)
• Understand the request (code review of PR fix(cli): anchor sg test count parsing, and pin ast-grep's contract #117)
• Review packages/cli/src/rules/verify.ts changes (sg test parsing fix)
• Review packages/cli/test/ast-grep-vendor-contract.test.ts (new contract suite)
• Review packages/cli/test/verify.test.ts regression test
• Review packages/cli/src/rules/engines.ts doc reference update
• Review deletion of packages/cli/test/engine-layout.test.ts
• Post inline comments for concrete issues
• Post final top-level assessment

Overall assessment

This is a solid, well-reasoned fix. I traced the actual bug — the old code ran two unanchored, case-insensitive regexes (/(\d+)\s+passed/i, /(\d+)\s+failed/i) over stdout + stderr concatenated with no separator, so the first digit-followed-by-"passed" anywhere, including inside an echoed failing fixture, won. The new parseTestSummary anchors on the two exact summary-line forms ast-grep emits (test result: ok. / Error: test failed.), strips ANSI first (the escape sits inside the phrase — test result: [32mok[0m.), and takes the last match rather than the first. I confirmed this genuinely fixes the described bug: the poisoned fixture text ('7 passed; 0 failed') no longer matches because it lacks the required line-start prefix.

The StringDecoder-per-stream change mirrors the identical, already-shipped pattern in packages/cli/src/rules/vale/run.ts (same comment structure, same per-stream write/end sequencing) — good consistency, and I verified the close handler flushes both decoders (.end()) before joining, so no truncated tail byte is dropped.

The vendor-contract suite is thorough and does real work: I checked several of its 18 assertions against the actual code paths they claim to guard (severity vocabulary vs. AstGrepMatch's 4-value union, exit-code boundary at >1, --filter being unanchored vs. the caller's ^...$ wrapping, sg test's split across stdout/stderr) and they line up. The two relocated .tests/-discovery cases from the deleted engine-layout.test.ts are faithfully preserved (same assertions, just migrated to the shared project() helper).

Findings posted inline (both minor, not blocking):

1. packages/cli/src/rules/verify.ts:170 — stripAnsi hand-rolls what node:util's stripVTControlCharacters already does; the repo requires node >=22.22.0 so it's available. Removes a lint suppression and a bit of bespoke regex.
2. packages/cli/src/rules/verify.ts:197 — The anchor closes the specific runTests scrapes ast-grep test's human output, and fixture text can corrupt the counts #112 poisoning vector but not the general class: a fixture that echoes the literal string Error: test failed. N passed; M failed; (not just "N passed") at the start of a line would still be read as the real summary. Much narrower window than the original bug and self-inflicted at worst (a rule author poisoning their own fixture), so low severity — flagging in case it's worth a one-line caveat in the doc comment.

Also worth noting (already caught by Copilot's review, and I agree with it): project() in ast-grep-vendor-contract.test.ts only mkdirSyncs cwd/src for sources entries, regardless of the actual path key — every current usage happens to be under src/, so it doesn't bite today, but a future case adding a sources path outside src/ will hit ENOENT. Worth a follow-up (mkdirSync(dirname(join(cwd, path)), { recursive: true })) whenever that helper grows a non-src/ case, not blocking now.

No correctness issues found in the actual bug fix — the regex anchoring, ANSI stripping, and last-match-wins logic all check out against the described failure mode and the contract test's pinned output shapes. The engines.ts doc-comment update and the file deletion are clean; nothing was lost in the .tests/-discovery migration.

[thecodedrift]

Just building on the statement from Claude, we should use builtins when possible within the CLI, as they're a lot more consistent and we maintain less code

[thecodedrift]

Re: @claude[bot] — "Claude finished @thecodedrift's task in 4m 6s — Review complete…"
#117 (comment)

Both inline findings are addressed in 33b0806, along with the project() helper point you seconded from Copilot: stripVTControlCharacters replaces the hand-rolled stripper, project() mkdirs the parent of the path it is actually given, and the residual poisoning window turned out to be already closed — sg test indents every echoed line two spaces, so it is now pinned as a contract case rather than noted as an accepted residual.

— AI Coding Agent

[thecodedrift]

Re: @thecodedrift — "Just building on the statement from Claude, we should use builtins when possible within the CLI…"

Done in 33b0806 — the hand-rolled stripAnsi and its no-control-regex suppression are replaced by node:util's stripVTControlCharacters. Checked the rest of the diff for the same class and this was the only bespoke reimplementation of a builtin; StringDecoder was already the builtin route.

— AI Coding Agent

[thecodedrift]

@claude /review

[claude]

Claude finished @thecodedrift's task in 2m 40s —— View job

---

Review complete

• Gather context (diff 5596e39...HEAD, matches PR's stated 6-file / +682/-121 shape)
• Re-review packages/cli/src/rules/verify.ts (parseTestSummary, StringDecoder streams)
• Re-review packages/cli/test/ast-grep-vendor-contract.test.ts (all 19 cases, including the new "indents every echoed line" case from 33b0806)
• Confirm the three findings from the prior review round are actually fixed in 33b0806
• Check packages/cli/src/rules/engines.ts doc update and callers of runTests/verifyRule for breakage
• Check packages/cli/test/verify.test.ts regression test and the deleted engine-layout.test.ts migration
• Post final assessment

Overall assessment

This is a re-review after 33b0806, which addresses everything raised in my previous pass. I re-verified all three fixes against the actual diff rather than taking the commit message at its word:

1. Builtin ANSI stripper — the hand-rolled stripAnsi + eslint-disable-next-line no-control-regex is gone; verify.ts now imports stripVTControlCharacters from node:util directly. Confirmed no other bespoke reimplementation of a builtin was introduced elsewhere in the diff.
2. project() mkdir fix — ast-grep-vendor-contract.test.ts:105-111 now does mkdirSync(dirname(join(cwd, path)), { recursive: true }) for each sources entry instead of assuming src/, matching what the Project.sources doc comment promises.
3. Poisoning-class closure, not just the one vector — rather than adding a caveat to the doc comment, 33b0806 pins the actual vendor property as a contract case (ast-grep-vendor-contract.test.ts:403-427, "indents every echoed line, so fixture text never reaches column 0"), and folds the reasoning into parseTestSummary's doc comment in verify.ts:160-176. I checked the test's escaping (poison.replaceAll(/[.*+?^${}()|[\]\\]/g, ...)) — it correctly builds an anchored regex from the poison string without leaving metacharacters live, so the negative assertion is meaningful and not vacuously true.

I also re-traced the core fix end-to-end against the diff (not just the description):

• parseTestSummary in verify.ts:191-200 anchors on ^(?:test result: ok\.|Error: test failed\.) (\d+) passed; (\d+) failed; with gm, over ANSI-stripped, decoder-flushed, newline-joined stdout+stderr. Walked through why the \n join matters: without it, a stdout chunk not ending in a newline immediately followed by stderr's Error: test failed... would glue onto the previous line and never match ^ — the join is load-bearing, not cosmetic, and the doc comment says so.
• runTests (verify.ts:203-291) is otherwise untouched in its control flow — TestLayerResult's shape, the code === 0 success branch, and the error-message fallback logic are unchanged, so I checked callers (inspect.ts) for any shape assumption that could break: none, passed/failed were already numbers before this change.
• Cross-checked StringDecoder usage against vale/run.ts:174-226 line by line — identical write-per-chunk, end()-flush-before-join pattern. No divergence.

No new correctness, security, or test-adequacy issues found in this round. The regex, anchoring, ANSI stripping, and stream-join logic all hold up against the described failure mode and the now-pinned vendor properties. The engines.ts doc-comment update and engine-layout.test.ts deletion remain clean — the two relocated .tests/-discovery cases in the new contract file preserve the same assertions as the deleted file (now via the shared --json=stream + scan() helper rather than a one-off --json + execFile call, which is consistent with the rest of the new file).

Nothing further to flag; no inline comments needed this round.
· branch fix/ast-grep-contract-and-test-parsing