test(cli): pin the two metavariable-binding fixes, and warn about inert rules - #199
Conversation
…watch for The 0.11.x ledger's hardest section said "re-run its fixtures and read the findings rather than the exit code", which tells an author to look without saying what for. Measuring both shapes against the two binaries (#199) turned each into an instruction. A rule using `nthChild` with a binding `ofRule` was INERT at 0.41.0 and now reports. The rule is not broken, it never ran, so its findings on this version have never been seen and may be numerous on a codebase that was never actually checked against it. Fixtures cannot surface this: a rule that matched nothing passed the `pass/` side of its own tests. A non-binding `ofRule`, `kind:` or `regex:`, is unaffected, which is the check that tells an author whether their rule is in scope. A metavariable bound inside a negated `not` and referenced in `message`, `fix` or a constraint now renders empty, because a `not` contributes no bindings and the old value was leaked from the candidate the negation REJECTED. This one changes no counts at all: same file, same range, same rule, different rendered output. So the instruction is to grep for a metavariable appearing both inside a `not` and in a `message` or `fix`, and the consequence is worth stating plainly, since a `fix` in that shape has been writing leaked text into files. Root metavariables and comments is named as unreproduced rather than dressed up as guidance, because no shape we tried showed a difference. Refs #180 Refs #184
|
@claude /review |
|
Claude finished @thecodedrift's task in 1m 44s —— View job Review:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…watch for The 0.11.x ledger's hardest section said "re-run its fixtures and read the findings rather than the exit code", which tells an author to look without saying what for. Measuring both shapes against the two binaries (#199) turned each into an instruction. A rule using `nthChild` with a binding `ofRule` was INERT at 0.41.0 and now reports. The rule is not broken, it never ran, so its findings on this version have never been seen and may be numerous on a codebase that was never actually checked against it. Fixtures cannot surface this: a rule that matched nothing passed the `pass/` side of its own tests. A non-binding `ofRule`, `kind:` or `regex:`, is unaffected, which is the check that tells an author whether their rule is in scope. A metavariable bound inside a negated `not` and referenced in `message`, `fix` or a constraint now renders empty, because a `not` contributes no bindings and the old value was leaked from the candidate the negation REJECTED. This one changes no counts at all: same file, same range, same rule, different rendered output. So the instruction is to grep for a metavariable appearing both inside a `not` and in a `message` or `fix`, and the consequence is worth stating plainly, since a `fix` in that shape has been writing leaked text into files. Root metavariables and comments is named as unreproduced rather than dressed up as guidance, because no shape we tried showed a difference. Refs #180 Refs #184
…rt rules The 0.41.0 to 0.45.2 upgrade changed what some valid rules match, with no error and no warning, and nothing verified it. #182 checked the interface; this checks the semantics. Both cases are taken from the upstream pull requests rather than written from the changelog, and that distinction is the whole result. Hand-written rules over the same features, nthChild and negated not and root metavariables, showed NO difference between the two binaries: neither bug fires unless a metavariable is bound in the exact position the fix changed. A corpus built by guessing would have compared non-empty results, agreed, and been reported as evidence that nothing moved. Measured against both binaries: ast-grep/ast-grep#2677. `nthChild`'s `ofRule` reused one environment across siblings, so the first match committed `$S` and every later sibling failed the consistency check and went uncounted. At 0.41.0 the rule matched NOTHING; at 0.45.2 it matches `b();`. ast-grep/ast-grep#2676. A `not` passed the live environment to its inner matcher, so a candidate that matched, and therefore failed the negation, left its binding behind. At 0.41.0 `$A` rendered as `foo`, leaked from a `return foo;` that the negation had rejected; at 0.45.2 it is unbound. The second one is asserted on the MESSAGE, not the match, and that is worth keeping: the finding is identical across the upgrade. Same file, same range, same rule. Only the binding differs. #184 specifies comparing findings as sets of (file, range, ruleId), and that comparison would have reported no change here. ast-grep/ast-grep#2868, root metavariables against comments, is NOT pinned. I could not reproduce a difference in any shape I tried, including the TSX case the PR names, so there is nothing honest to assert yet. `create-sg-rule` also gains the two ways a relational rule verifies clean and reports nothing forever, both of which cost real time here and were written down nowhere: a sibling relation needs `stopBy: end` because the separators between siblings are nodes too, and a `not` searching the subtree that bound the metavariable always finds it, so it rejects everything. Refs #184 Refs #162
From review of #199, which asked whether `position: 2` really selects the second match. It was right to ask: the vendored schema describes the plain-number form of `nthChild` as "A number indicating the precise element index" with `minimum: 0`, which reads as 0-indexed and would make the binding test off by one, selecting `c();` while claiming `b();`. Measured against the binary. Position 0 matches nothing at all, and 1, 2, 3 select the first, second and third sibling. The behaviour is CSS-like and the schema's own description is misleading. Pinned rather than answered in a comment, because the schema will keep suggesting otherwise to the next reader, and because the ordering the metavariable-binding test depends on should be asserted rather than assumed. A future bump that changes the indexing now turns this red instead of quietly inverting what that test proves.
2ab5738 to
8155a56
Compare
All three addressed. Threads replied to and resolved. The Measured against the binary:
The assertion holds and the schema's own description is the misleading part. Pinned as its own case in The duplication finding is declined, deliberately.
— AI Coding Agent |
1 participant
Verifies that the 0.41.0 to 0.45.2 upgrade did not change what rules match, and pins the two cases where it did.
#182 verified the interface did not move. Nothing had ever checked the semantics: a valid, unchanged rule matching a different set of nodes, with no error and no warning.
Going to the upstream PRs was the whole result
My first corpus was hand-written from the changelog: rules over
nthChild, negatednot, and root metavariables. Run against both binaries, every construct produced non-empty and identical results. That looks like evidence the upgrade was safe. It was not evidence of anything.Neither bug fires unless a metavariable is bound in the exact position the fix changed. Reading the actual pull requests produced reproducing cases immediately, and both show a real difference:
#2677 —
nthChild'sofRulereused one environment across siblings, so the first match committed$Sand every later sibling failed the consistency check and went uncounted.b();#2676 — a
notpassed the live environment to its inner matcher, so a candidate that matched (and therefore failed the negation) left its binding behind.message: $Afoo— leaked from areturn foo;the negation had rejectedThe second case breaks this issue's own comparison
That finding is identical across the upgrade: same file, same range, same rule. Only the binding differs.
#184 specifies comparing findings as sets of
(file, range, ruleId). That comparison would have reported no change here. The test therefore asserts on the rendered message, and any future differential needs to compare bindings, not just locations.Pinned in the vendor contract, not as a differential harness
The tests assert what the vendored binary does, so they need no second binary, no network, and no skip condition. A future bump that regresses either behaviour turns them red. That is the durable half of what #184 asked for; a two-binary harness remains useful for the next upgrade and is not built here.
Not pinned
#2868, root metavariables against comments. I could not reproduce a difference in any shape I tried, including the TSX case the PR names. Rather than assert something I have not observed, it is left out and recorded here.
Two gotchas for
create-sg-ruleBoth cost real time while building this, and both were written down nowhere. Each parses, verifies, runs, exits 0, and reports nothing forever, which reads exactly like a clean codebase:
followsandprecedesneedstopBy: endto cross punctuation. Between two array elements sits a,, which is itself a node, so the previous element is not the previous sibling. Measured:[10, 10, 30]with a barefollowsmatches nothing; withstopBy: endit matches.notcontaining a bare metavariable excludes everything.$Abound to a name node is its own descendant, so anotsearching the same subtree finds it in every candidate and rejects them all. The negation has to be scoped to the part you meant.Verification
pnpm typecheck,pnpm lint, 966 tests,pnpm cli checkexit 0.Refs #184
Refs #162