fix(cli): require both fixture buckets for an ast-grep rule

[thecodedrift]

Stack (root → tip):

• feat(cli): state each local engine's reach in the routing recipe #155
  • ➡️ fix(cli): require both fixture buckets for an ast-grep rule #156 (you are here)
  • fix(cli): stop one AsciiDoc file from disabling every Vale rule #157
    • feat(cli): move the Vale engine to 3.18.0 and re-measure its format reach #161

Stacked on #155. Review only the last two commits; the base is that PR.

verify Layer 2 checked that a -test.yml exists, never what was in it — and ast-grep test passes an empty invalid: bucket (1 passed; 0 failed, exit 0). So a rule that flags nothing reported success:

$ taskless test .taskless/rules/sg/no-bare-fetch --json
{"ok":true,"rules":[{"engine":"sg","ruleId":"no-bare-fetch","ok":true,"errors":[],"ran":true}]}
$ taskless check                # source contains `fetch(url);`
No issues found.

We already close this on the Vale side. rules/vale/verify.ts has ValeFixtureCoverage and inspect.ts fails any rule whose fixtures !== "both" — "half a claim". The sg branch of that same function had no equivalent, and cli-rule-validation specified the Vale rule with no sg counterpart, so the spec actively endorsed the gap.

Now symmetric: a rule whose invalid: bucket is empty across all its .tests/*-test.yml files has never been shown to fire and does not pass. It reads the author's own YAML with the parser already imported in verify.ts — derives nothing, guesses at nothing — and generalizes past $$$ to any rule that matches nothing.

⚠️ This rejects rules that passed before. minor is defensible; patch was chosen (pre-1.0) with the warning carried in the changeset body.

Fixes #152

What #152 reported vs. what is true

The report was that a trailing $$$ does not match a zero-argument call. That is false as worded — and a worse variant is real. Measured against the bundled 0.41.0 and now pinned:

pattern	matches, given foo(), foo(1), foo(1,2), foo(1,2,3)
foo($$$)	all four, including foo()
foo($A, $$$)	only foo(1,2), foo(1,2,3) — not foo(1)
foo($$$, $A)	only foo(1)
foo($A, $$$, $B)	only foo(1,2)

The pattern's , is itself an AST node, and under default smart strictness every pattern node must match, so a call with no comma cannot match a pattern that has one. $$$ matching zero nodes is fine — the separator is what fails. $$$ and $$$ARGS behave identically; statement blocks are unaffected because ; lives inside the statement node.

Upstream closed this as working-as-intended (ast-grep/ast-grep#1365) and 0.45.2 behaves identically, so there is no version to wait for.

The remedy is strictness: ast inside a pattern object — and note it moves the boundary from ≥2 to ≥1, not to zero, since $A still has to bind something. strictness at rule level is rejected as an unknown field and fails the whole scan, so placement is load-bearing; a test pins that too. For a leading $$$, strictness: ast does not help and any: with one branch per arity is the fix.

The coverage gate is what surfaces this. It forces an invalid: case, that case fails against the original pattern, and strictness: ast fixes it — the trap becomes visible at authoring time instead of in production.

Zero shipped rules were affected

Every $$$ in this repo is the sole element of its argument list. The exposure was entirely prospective: no recipe mentioned $$$ at all, and create-sg-rule.txt pointed agents at the upstream docs, which say only "zero or more AST nodes" — the omission that makes this a trap.

Recipes

create-sg-rule.txt gains a step for the separator trap with the measured table and both remedies, and the fixture step now states that both buckets must be non-empty and that a rule with an arity boundary needs a case on each side of it. improve-rule.txt gains a note to check for a comma-adjacent $$$ before assuming a pattern is merely too narrow, and to re-confirm both buckets after the service rewrites a rule — narrowing a pattern to kill a false positive can leave invalid: with nothing the rule still matches.

Inserting a step shifted four numbers; three in-file cross-references were corrected, and every recipe was grepped for step [0-9] to catch others. (A previous change here shipped an off-by-one exactly this way.)

verify-examples.ts gains a strictness: ast entry, so the safe form reaches agents through verify --schema — it previously showed only the standalone $$$.

Tests

One case per coverage state in verify.test.ts (nothing covered an empty bucket before), the $$$ behavior pinned in ast-grep-vendor-contract.test.ts including rule-level strictness hard-failing, and two cases pinning the language-field claims added to the recipe: an unrecognized spelling takes the whole scan down, and Tsx/TypeScript are distinct parsers rather than aliases.

OpenSpec delta on cli-rule-validation adding the sg bucket scenario. Not archived; not the tip.

Built on top of #155

An agent reading route.txt could not tell whether a language was buildable locally or had to escalate to runtime, which needs a login. The reporter's case: two GitHub Actions YAML rules were routed to runtime because nothing said whether ast-grep handles YAML.

It does. The pinned @ast-grep/cli@0.41.0 lists 26 languages including Yaml, and a real pr-workflow-no-branches-filter rule (language: Yaml) passes verify and test. Both Actions rules belong in create-sg-rule with no login.

Refs #151

Derived, not transcribed

There was no source of truth for either engine's reach. $defs.Language in the vendored schema is a bare string with no enum, verify never validates language, and detect --json reports the repository's languages in a different vocabulary. The binaries were the only authority, and route.txt said nothing.

Transcribing two lists into a .txt would drift on a version bump with nothing to catch it, and stale prose is worse than the current silence — silence at least makes the agent ask. So: pinned constants in src/rules/capabilities.ts, substituted into recipes as %(KEY)s, with vendor-contract tests asserting the constants still match the binaries. That is the pattern ast-grep-vendor-contract.test.ts already exists for — exact-pinned binary plus a test that fires on the bump.

capabilities.ts is deliberately import-free: prompts/recipes.ts imports it, and the assert-prompts-graph vite plugin fails the build if that chunk reaches a host capability.

Rejected alternative: extending detect --json with an engines block. cli-detect's spec frames detect as an offline repo scan, engine reach is knowable at build time, and it would cost a third subprocess turn in a recipe already spending two.

Vale's reach was measured, not read

Vale self-reports no format list, so every tier here comes from probing the shipped 3.17.1 binary — and the measurement corrected the starting assumptions in five places:

• .css is comment-tier, not plaintext. The first probe said otherwise because it fed CSS a // comment, which is not one.
• .sass is comment-tier; .scss is not.
• Extension matching is case-sensitive and not closed over aliases — .R is comment-aware, .PY/.JS/.TS are not; .cc/.cxx/.pyw/.pm are, .hh/.hxx are not.
• Markup tier is wider than assumed (.htm, .xhtml).
• --no-exit does not suppress the converter tier's E100 exit 2.

Each tier is pinned by its discriminating property, because on ordinary prose all three readable tiers look identical: markup by a construct only a parser skips, comment-tier by the negative (a bare non-comment line must yield zero), converter-tier by non-zero exit plus E100 plus the converter's name. A test also asserts the fixture map covers the markup constant exactly, so an unprobed entry cannot be added to a constant.

create-vale-rule.txt was teaching the crash

Its worked example globbed [*.{md,mdx}]. .mdx needs mdx2vast, which this CLI does not ship, and one such file fails the entire Vale pass — so any rule following that example silently disabled every other Vale rule. Now [*.{md,markdown}], with the point stated plainly: a matcher like that is not a wider [*.md], it is a broken one. (The guard that makes this non-fatal is in the PR above this one.)

A correction, included deliberately

The last commit walks back a claim this branch originally shipped. Both the recipe and the constants said a plausible C++ or yaml "passes verify and fails inside the binary." Measured at 0.41.0, C++ and cpp are accepted aliases that resolve to Cpp, so an off-list spelling is not reliably an error.

The two failures that are real, neither caught locally: a name ast-grep does not recognize (C# for CSharp) aborts config parsing, taking every other rule's report down with it; and a valid name for the wrong parser (TypeScript over a .tsx tree) reports nothing and reads as a clean codebase. The advice to copy from the list is unchanged; the reason for it is now the accurate one. The vendor-contract assertions were already right — they pin the canonical list, not what the binary rejects — but their comment said otherwise.

Also

route.txt now states that a language on neither list does not route to runtime by default: create-legacy-rule comes first, since the repo's own linter may already speak it. And a new note separates reach (a property of the pinned version) from availability (a property of the host), which the existing "availability you can assert" bullet was conflating while giving the agent no way to assert it.

OpenSpec delta on cli-rule-routing — engine reach becomes a fourth thing the recipe is specified to state, with the normative core being that it is derived from the pinned versions rather than transcribed. Not archived; this is not the tip of the stack.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[thecodedrift]

@claude /review

[claude]

Claude finished @thecodedrift's task in 3m 57s —— View job

---

Review in progress

Review mode: incremental — read 0 prior review thread(s) before reviewing. (No prior inline review threads exist on this PR; the only prior review activity was Copilot's quota-limit failure, so there is nothing previously-raised to classify as addressed/unchanged. This is effectively the first substantive review.)

• Read .prior-review.json
• Gather diff (git diff origin/fix/engine-capabilities...HEAD)
• Manual line-by-line pass (correctness, cross-file tracing, removed-behavior)
• Finder agents: reuse/simplification/efficiency/altitude, cross-file tracer, conventions/removed-behavior
• Verify candidate findings
• Post inline comments for concrete issues
• Post final top-level summary

[thecodedrift]

@claude /review

The earlier review on this PR stopped before posting findings (the checklist above is unfinished), and the branch has changed since — it was rebased onto a corrected base and carries new commits. Please review the current head.

[claude]

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

---

Review complete

Review mode: incremental — read 0 prior review thread(s) before reviewing. .prior-review.json contains no prior inline threads, reviews, or comments beyond Copilot's quota-limit failure notice, so there is nothing previously-raised to classify as addressed/unchanged. This is effectively the first substantive review of this PR — resuming after an earlier run that stopped mid-checklist, now against the current rebased head (git diff origin/fix/engine-capabilities...HEAD).

• Read .prior-review.json
• Gather diff (git diff origin/fix/engine-capabilities...HEAD)
• Manual line-by-line pass (correctness, cross-file tracing, removed-behavior)
• Finder agents (8): reuse/duplication, cross-file tracer, removed-behavior auditor, simplification, CLAUDE.md conventions, efficiency, altitude, line-by-line correctness scan
• Verify candidate findings against the actual code and git history
• Post inline comments for concrete issues
• Post final top-level summary

Findings posted inline (all [New])

1. packages/cli/test/ast-grep-vendor-contract.test.ts:658-697 — a leftover duplicate describe("the language field", ...) block. Confirmed against origin/fix/engine-capabilities: the base already carries this exact 3-test suite (now at line 526 in the current file); this second copy (2 of the 3 tests, verbatim) is a rebase artifact. Commit ee1a034 explicitly says its purpose was removing this duplication but only deleted the duplicate atLanguage helper, missing the describe block itself.
2. packages/cli/src/rules/verify.ts:227-247 (fixtureCoverage) — counts a test file toward a rule's coverage by filename glob only, never checking the file's own id: against ruleId, while sg test --filter (line 322) resolves against that id:. A misnamed/copy-pasted test file's id: inside the correct rule directory would be counted here but silently skipped by sg test, reopening the exact "never actually shown to fire" gap this PR closes, through a different door. Narrow scenario, but real.
3. packages/cli/src/rules/verify.ts:217-230 — fixtureCoverage re-lists the same directory and re-applies the identical filename filter that validateRequirements (lines 172-175) already computed earlier in the same verifyRule() call. Same-file duplication of the "what counts as this rule's test file" predicate, worth extracting into one shared helper.

Noted but not filed as separate blocking comments

• coverageOf (verify.ts:190-198) duplicates vale/verify.ts's function of the same name/shape, and the inspect.ts message-construction for "none"/"-only" states is duplicated near-verbatim between the sg and Vale branches of testOneRule (lines 218-229 vs 255-262). Multiple finder agents flagged this, but it's a documented, deliberate mirroring (see the doc comments at verify.ts:46-56 and inspect.ts:219) rather than an accidental copy — a reasonable simplification target (a shared classifyCoverage/describeFixtureCoverage helper), not a defect.
• Swallowed readdir/readFile/parse failures inside fixtureCoverage (lines 219-236) produce no diagnostic — but this is explicitly the documented intent (verify.ts:209-211: a file that can't be read/parsed "contributes nothing," since sg test itself will report the malformation), so treating this as a gap would go against a stated, defensible design choice.
• The fixture-coverage gate is a non-emptiness check; it can't by itself verify a populated invalid: bucket actually straddles a $$$-adjacent-comma arity boundary (the [sg] Trailing $$$ breaks sg rules #152 mechanism). That's out of scope for what this PR sets out to do (bucket coverage, not pattern-shape linting) and is already covered by recipe prose per the PR description — flagging only as a forward-looking design note, not a finding against this diff.
• Repeated { valid: false, errors: [...], passed: 0, failed: 0, fixtures: "none" } skip-result literals (5 call sites in verify.ts) and the step-renumbering fragility in create-sg-rule.txt are minor, defensible simplification opportunities, not correctness issues.

No security, correctness-blocking, or test-adequacy issues beyond what's posted inline above. The core design (reading the author's own YAML for bucket counts rather than ast-grep test's unstructured output) is well-justified and consistent with this codebase's documented history with parseTestSummary/#112.

[thecodedrift]

Re: @claude[bot] — "Claude finished @thecodedrift's task in 3m 57s —— View job … ### Review in progress"
#156 (comment)

This is the in-progress checklist from a run that stopped mid-review; it was superseded by the completed run in #issuecomment-5391681373, whose three inline findings are all addressed in 5c19a47. Acknowledging so a re-run does not resurface it.

— AI Coding Agent

[thecodedrift]

Re: @claude[bot] — "Claude finished @thecodedrift's task in 2m 45s … ### Review complete — Findings posted inline (all [New])"
#156 (comment)

All three inline findings verified against the code and fixed in 5c19a47: the duplicate describe("the language field") block is deleted (the base branch keeps the fuller three-test copy); fixtureCoverage now requires buckets.id === ruleId before counting, matching what sg test --filter resolves against, pinned by a new verify.test.ts case; and the filename predicate is extracted into one discoverRuleTestFiles shared by validateRequirements and fixtureCoverage. The "noted but not filed" items — the deliberate coverageOf/inspect.ts mirroring of the Vale engine, the intentionally silent parse failures, and the skip-result literals — are left as-is, since each is a documented choice rather than a defect, and a shared abstraction across the two engines is a change that should be made deliberately rather than as review cleanup. Suite is green at 788 (789 minus the two duplicated language-field cases, plus the new id-mismatch case).

— AI Coding Agent