fix(cli): require both fixture buckets for an ast-grep rule - #156
fix(cli): require both fixture buckets for an ast-grep rule#156thecodedrift wants to merge 3 commits into
Conversation
|
@claude /review |
|
Claude finished @thecodedrift's task in 3m 57s —— View job Review in progress
|
5308ccf to
bd9d73f
Compare
`verify` Layer 2 checked that a rule's `-test.yml` existed and never read what was in it, and `ast-grep test` reports an empty `invalid:` bucket as `1 passed; 0 failed` and exits zero. A rule whose fixtures were all `valid:` therefore reported `ok: true, ran: true` while `check` found nothing anywhere. `test` now counts the `valid:`/`invalid:` entries across every test file a rule owns and requires both, mirroring `ValeFixtureCoverage` state for state with ast-grep's own vocabulary. This rejects sg rules that passed before; the changeset says so. Also pins the `$$$` separator behaviour behind #152 as a vendor contract: a comma-adjacent `$$$` is not "zero or more", because the comma is itself a node that must match under the default `smart` strictness. Upstream calls this intended and 0.45.2 is identical, so `verify --schema` gains a curated `strictness: ast` example rather than a version bump. Fixes #152 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cyga14bww8rmazH2XrF8ms
…e is written `create-sg-rule` sent an author to upstream for the rule format and said nothing about the two fields that fail without local feedback. The `$$$` separator gets its own step, with the arity table measured against the pinned binary: a comma-adjacent `$$$` is not "zero or more", because the comma is itself a node that must match under the default `smart` strictness. Both remedies are stated as the different things they are — `strictness: ast` inside a pattern object for a trailing `$$$` (moving the boundary from two arguments to one, not to zero), `any` per arity for a leading one — and the fixture step now asks for a case on each side of an arity boundary, since a one-sided rule reports `1 passed; 0 failed` and exits zero. `language:` gets the same treatment in the authoring step, rendered from `AST_GREP_LANGUAGES` rather than transcribed. Nothing local validates that field, and it fails two ways: a spelling ast-grep does not recognize takes the whole scan down, and `TypeScript` over a `.tsx` tree reports nothing and looks clean. Both are now pinned in the vendor contract, so the prose is quoting the binary rather than describing it. `improve-rule` carries the two notes that only matter on a rewrite: read the pattern for a comma-adjacent `$$$` before reporting a rule as too narrow, and re-check both fixture buckets after the service returns a narrowed rule. Refs #152 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cyga14bww8rmazH2XrF8ms
The suite and its `atLanguage` helper moved down to fix/engine-capabilities, so that the recipe claim in route.txt and the binary behaviour pinning it land in the same change. Rebasing brought them here, leaving a duplicate declaration. The $$$ arity suite stays — that one pins claims made by this branch's own recipe prose in create-sg-rule.txt. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cyga14bww8rmazH2XrF8ms
bd9d73f to
ee1a034
Compare
|
@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 finished @thecodedrift's task in 2m 45s —— View job Review completeReview mode: incremental — read 0 prior review thread(s) before reviewing.
Findings posted inline (all
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /** | ||
| * How a wrong `language:` fails — the two shapes `create-sg-rule.txt` warns | ||
| * about where the field is written. | ||
| * | ||
| * Nothing of ours catches either one first: the vendored | ||
| * `src/generated/ast-grep-rule-schema.json` types `$defs.Language` as a bare | ||
| * string with no enum, and `verify` never reads the field. So the binary's | ||
| * response IS the contract, and a recipe telling an author what to expect is | ||
| * quoting it. | ||
| */ | ||
| describe("the language field", () => { | ||
| it("fails the whole scan on a spelling it does not recognize", () => { | ||
| // `C#` is the plausible wrong spelling of `CSharp`, and getting it wrong | ||
| // is not a rule that quietly matches nothing: ast-grep cannot parse the | ||
| // config, so every OTHER rule in the project goes unreported too. The | ||
| // error names the enum, which is what an author sees. | ||
| const result = scan( | ||
| project({ rules: { "no-eval": atLanguage("C#") }, sources: evalSource }) | ||
| ); | ||
| expect(result.status).toBeGreaterThan(1); | ||
| expect(result.stderr).toContain("SgLang"); | ||
| }); | ||
|
|
||
| it("treats Tsx and TypeScript as different parsers, not aliases", () => { | ||
| // The quiet half of the same field, and the reason the recipe names this | ||
| // pair specifically. `TypeScript` over a `.tsx` tree exits clean with no | ||
| // findings, which is indistinguishable from a codebase with nothing to | ||
| // flag — the rule looks written and proves nothing. | ||
| const sources = { "src/a.tsx": "const el = <div>{eval(x)}</div>;\n" }; | ||
| const asTypeScript = scan( | ||
| project({ rules: { "no-eval": atLanguage("TypeScript") }, sources }) | ||
| ); | ||
| expect(asTypeScript.status).toBe(0); | ||
| expect(asTypeScript.stdout.trim()).toBe(""); | ||
| expect( | ||
| scan(project({ rules: { "no-eval": atLanguage("Tsx") }, sources })) | ||
| .stdout | ||
| ).toContain("eval(x)"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[New] This describe("the language field", ...) block is a leftover duplicate of the one at line 526, not new coverage.
origin/fix/engine-capabilities (the base) already carries this exact suite (verified via git show origin/fix/engine-capabilities:packages/cli/test/ast-grep-vendor-contract.test.ts) — including the "accepts off-list aliases" case that's missing here:
- Base (
fix/engine-capabilities): one"the language field"describe with 3 tests (spelling rejection, off-list aliases, Tsx/TypeScript) — this is what's now at line 526 in the current file. - This PR's diff adds a second
"the language field"describe (this one, 658-697) with 2 of those 3 tests, verbatim except the docstring now sayscreate-sg-rule.txtinstead ofroute.txt.
Commit ee1a034 ("drop the language-field suite now carried by the branch below") says its purpose was exactly to remove this kind of rebase leftover, but it only deleted the duplicate atLanguage const declaration (1 insertion / 4 deletions) — it missed this describe block. Both copies currently pass, so nothing catches them diverging on a future edit, and the "one canonical pin per binary guarantee" premise this file's own docstring states (lines ~17-24) is violated for this claim specifically.
Suggest deleting this block (lines 658-697) and keeping the one at 526-573, which is the fuller version (3 tests vs. 2).
| for (const entry of entries) { | ||
| if (!entry.startsWith(`${ruleId}-`) || !entry.endsWith("-test.yml")) { | ||
| continue; | ||
| } | ||
| let parsed: unknown; | ||
| try { | ||
| parsed = parse(await readFile(join(directory, entry), "utf8")); | ||
| } catch { | ||
| continue; | ||
| } | ||
| if ( | ||
| parsed === null || | ||
| typeof parsed !== "object" || | ||
| Array.isArray(parsed) | ||
| ) { | ||
| continue; | ||
| } | ||
| const buckets = parsed as Record<string, unknown>; | ||
| if (Array.isArray(buckets.valid)) validCount += buckets.valid.length; | ||
| if (Array.isArray(buckets.invalid)) invalidCount += buckets.invalid.length; | ||
| } |
There was a problem hiding this comment.
[New] fixtureCoverage counts a test file by filename glob only (${ruleId}-*-test.yml); it never checks the file's own id: field. sg test --filter ^${ruleId}$ (line 322), by contrast, resolves against the YAML's internal id:, as every test fixture built by this repo's own helpers does (see testFile/rule builders in ast-grep-vendor-contract.test.ts).
Concretely: a file named no-eval-scratch-test.yml sitting in no-eval's .tests/ directory (e.g. copy-pasted from a draft of a different rule, with id: left as no-alert-scratch and its own valid:/invalid: fixtures) would be counted by fixtureCoverage toward no-eval's coverage — flipping it to "both" — while sg test --filter ^no-eval$ never executes that file's cases at all, since ast-grep resolves test cases against a rule id and no-alert-scratch isn't no-eval. The gate reports ok: true for a rule whose invalid: bucket was never actually exercised — the exact class of gap this PR exists to close, reopened through the file-naming path rather than the empty-bucket path.
This is a narrow scenario (requires an author to misname/miscopy a test file's internal id: while keeping the correct filename prefix), but worth at least a code comment noting the assumption, or a cheap fix: also check buckets.id === ruleId before counting a file's buckets, mirroring what --filter actually keys on.
| const directory = ruleTestsDirectory(cwd, "sg", ruleId); | ||
| let entries: string[]; | ||
| try { | ||
| entries = await readdir(directory); | ||
| } catch { | ||
| return "none"; | ||
| } | ||
|
|
||
| let validCount = 0; | ||
| let invalidCount = 0; | ||
| for (const entry of entries) { | ||
| if (!entry.startsWith(`${ruleId}-`) || !entry.endsWith("-test.yml")) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
[New] This re-readdirs ruleTestsDirectory(cwd, "sg", ruleId) and re-applies the identical entry.startsWith(${ruleId}-) && entry.endsWith("-test.yml") filter that validateRequirements already computed a few lines up (lines 172-175), inside the same verifyRule() call — runTestLayer only reaches fixtureCoverage when hasTestFile is already true (line 414). The predicate is written twice in the same file; a future change to the naming convention (new suffix, different prefix rule, etc.) has two call sites to update and nothing ties them together.
Not a hot-path concern (one extra syscall + filter per rule per run), but worth extracting a shared discoverRuleTestFiles(cwd, ruleId): Promise<string[]> used by both validateRequirements (for hasTestFile) and fixtureCoverage (for the bucket count), so the "what counts as this rule's test file" rule is stated once.
| /** | ||
| * How a wrong `language:` fails — the two shapes `create-sg-rule.txt` warns | ||
| * about where the field is written. | ||
| * | ||
| * Nothing of ours catches either one first: the vendored | ||
| * `src/generated/ast-grep-rule-schema.json` types `$defs.Language` as a bare | ||
| * string with no enum, and `verify` never reads the field. So the binary's | ||
| * response IS the contract, and a recipe telling an author what to expect is | ||
| * quoting it. | ||
| */ | ||
| describe("the language field", () => { | ||
| it("fails the whole scan on a spelling it does not recognize", () => { | ||
| // `C#` is the plausible wrong spelling of `CSharp`, and getting it wrong | ||
| // is not a rule that quietly matches nothing: ast-grep cannot parse the | ||
| // config, so every OTHER rule in the project goes unreported too. The | ||
| // error names the enum, which is what an author sees. | ||
| const result = scan( | ||
| project({ rules: { "no-eval": atLanguage("C#") }, sources: evalSource }) | ||
| ); | ||
| expect(result.status).toBeGreaterThan(1); | ||
| expect(result.stderr).toContain("SgLang"); | ||
| }); | ||
|
|
||
| it("treats Tsx and TypeScript as different parsers, not aliases", () => { | ||
| // The quiet half of the same field, and the reason the recipe names this | ||
| // pair specifically. `TypeScript` over a `.tsx` tree exits clean with no | ||
| // findings, which is indistinguishable from a codebase with nothing to | ||
| // flag — the rule looks written and proves nothing. | ||
| const sources = { "src/a.tsx": "const el = <div>{eval(x)}</div>;\n" }; | ||
| const asTypeScript = scan( | ||
| project({ rules: { "no-eval": atLanguage("TypeScript") }, sources }) | ||
| ); | ||
| expect(asTypeScript.status).toBe(0); | ||
| expect(asTypeScript.stdout.trim()).toBe(""); | ||
| expect( | ||
| scan(project({ rules: { "no-eval": atLanguage("Tsx") }, sources })) | ||
| .stdout | ||
| ).toContain("eval(x)"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[New] This describe("the language field", ...) block is a leftover duplicate of the one at line 526, not new coverage.
origin/fix/engine-capabilities (the base) already carries this exact suite at line 501 of that ref's copy of the file — including the "accepts off-list aliases" case that's missing here. Comparing the three ref's-eye views:
- Base (
fix/engine-capabilities): one"the language field"describe with 3 tests (spelling rejection, off-list aliases, Tsx/TypeScript). - This PR's diff: adds a second
"the language field"describe with 2 of those 3 tests, verbatim except the docstring now sayscreate-sg-rule.txtinstead ofroute.txt.
Commit ee1a034 ("drop the language-field suite now carried by the branch below") says its purpose was exactly to remove this kind of rebase leftover, but it only deleted the duplicate atLanguage const declaration (1 insertion / 4 deletions) — it missed this describe block. Both copies currently pass, so nothing catches them diverging on a future edit, and the "one canonical pin per binary guarantee" premise this file's own docstring states (lines ~17-24) is violated for this claim specifically.
Suggest deleting lines 658-697 (or lines 526-573, whichever is meant to be the one true copy — the surrounding doc comment at 658-667 reads as the copy that should go, since it duplicates prose from 516-524 too).
Stack (root → tip):
verifyLayer 2 checked that a-test.ymlexists, never what was in it — andast-grep testpasses an emptyinvalid:bucket (1 passed; 0 failed, exit 0). So a rule that flags nothing reported success:We already close this on the Vale side.
rules/vale/verify.tshasValeFixtureCoverageandinspect.tsfails any rule whosefixtures !== "both"— "half a claim". The sg branch of that same function had no equivalent, andcli-rule-validationspecified 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.ymlfiles has never been shown to fire and does not pass. It reads the author's own YAML with the parser already imported inverify.ts— derives nothing, guesses at nothing — and generalizes past$$$to any rule that matches nothing.minoris defensible;patchwas 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:foo(),foo(1),foo(1,2),foo(1,2,3)foo($$$)foo()foo($A, $$$)foo(1,2),foo(1,2,3)— notfoo(1)foo($$$, $A)foo(1)foo($A, $$$, $B)foo(1,2)The pattern's
,is itself an AST node, and under defaultsmartstrictness 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$$$ARGSbehave 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: astinside a pattern object — and note it moves the boundary from ≥2 to ≥1, not to zero, since$Astill has to bind something.strictnessat 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: astdoes not help andany: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, andstrictness: astfixes 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, andcreate-sg-rule.txtpointed agents at the upstream docs, which say only "zero or more AST nodes" — the omission that makes this a trap.Recipes
create-sg-rule.txtgains 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.txtgains 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 leaveinvalid: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.tsgains astrictness: astentry, so the safe form reaches agents throughverify --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 inast-grep-vendor-contract.test.tsincluding rule-levelstrictnesshard-failing, and two cases pinning the language-field claims added to the recipe: an unrecognized spelling takes the whole scan down, andTsx/TypeScriptare distinct parsers rather than aliases.OpenSpec delta on
cli-rule-validationadding the sg bucket scenario. Not archived; not the tip.