diff --git a/.changeset/sg-fixture-coverage.md b/.changeset/sg-fixture-coverage.md new file mode 100644 index 00000000..8fbd0a33 --- /dev/null +++ b/.changeset/sg-fixture-coverage.md @@ -0,0 +1,42 @@ +--- +"@taskless/cli": patch +--- + +Fail `test` for an ast-grep rule that never demonstrates it can fire. + +`verify` 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 — verified +looking verified, having proved nothing. `test` now counts the `valid:` and +`invalid:` entries across every test file a rule owns and requires both, which +is the rule Vale fixtures have always been held to. + +**This rejects rules that passed before.** Any sg rule with an empty or absent +`invalid:` bucket now fails `test` until a fixture is added that the rule +actually matches. That is the intended effect: adding one is how the underlying +mistake surfaces. + +The mistake that prompted this is worth knowing about, because the pattern +looks correct. A trailing `$$$` next to a comma does not mean "zero or more" — +the comma is itself an AST node, and under ast-grep's default `smart` +strictness every node in the pattern must match, so `fetch($URL, $$$REST)` +never matches `fetch(url)` and silently starts at two arguments. A leading +`$$$` is worse: `foo($$$, $A)` collapses to exactly one argument. Upstream +considers this intended and 0.45.2 behaves identically, so there is no version +to upgrade to; write the pattern as an object with `strictness: ast` to ignore +the separator, or use `any:` with one branch per arity. `verify --schema` now +carries a worked example, and the behaviour is pinned against the vendored +binary so a bump that changes it fails loudly. + +`create-sg-rule` states all of this where a pattern is written: the arity table +measured against the pinned binary, both remedies and the fact that +`strictness: ast` moves a trailing `$$$` from two arguments to one rather than +to zero, and the fixture requirement with a case on each side of an arity +boundary. It also names ast-grep's `language:` vocabulary from the same pinned +constants — nothing local validates that field, an unrecognized spelling takes +the whole scan down, and `Tsx` is a different parser from `TypeScript` rather +than an alias. `improve-rule` gains the two notes that matter when a rule is +rewritten rather than written: read the pattern for a comma-adjacent `$$$` +before reporting it as too narrow, and re-check both fixture buckets after the +service returns a narrowed rule. diff --git a/openspec/changes/sg-fixture-coverage/proposal.md b/openspec/changes/sg-fixture-coverage/proposal.md new file mode 100644 index 00000000..c1ae015d --- /dev/null +++ b/openspec/changes/sg-fixture-coverage/proposal.md @@ -0,0 +1,94 @@ +## Why + +`test` reports an ast-grep rule that has never been shown to fire as +passing. Layer 2 checks that a `-test.yml` **exists** and never reads +what is in it, and `ast-grep test` is content with an empty `invalid:` +bucket — `1 passed; 0 failed`, exit 0. A rule whose fixtures are all +`valid:` therefore reports `ok: true, ran: true` while `check` finds +nothing anywhere. + +The spec already closes this for Vale — _"a rule populating only one +bucket SHALL be reported as unverified rather than passing"_ — with no +ast-grep counterpart, so it currently endorses the gap rather than +merely omitting it. + +What made the gap visible is taskless/cli#152. A pattern like +`fetch($URL, $$$REST)` reads as "fetch with any trailing arguments" and +is not: the pattern's `,` is itself a node, and under ast-grep's default +`smart` strictness every pattern node must match, so a one-argument +`fetch(url)` has no comma to match against and the rule silently starts +at arity two. Upstream considers this working as intended +(ast-grep/ast-grep#1365) and 0.45.2 behaves identically, so there is no +version to upgrade to. The author-side remedy is `strictness: ast` +inside the pattern object. + +The arity trap is the symptom; the reason it shipped undetected is that +nothing ever required the rule to demonstrate a match. A fixture in the +`invalid:` bucket would have caught it on the first run. + +## What Changes + +- **An sg rule's fixture coverage is classified and gates the test + layer.** `verify.ts` reads the author's own test YAML and counts the + `valid:` and `invalid:` entries across every `-test.yml` the rule + owns, yielding `"both" | "valid-only" | "invalid-only" | "none"`. + Only `"both"` can pass, mirroring `ValeFixtureCoverage` in + `rules/vale/verify.ts` state for state. +- **`testOneRule` emits the coverage message**, in the wording the Vale + branch beside it already uses — _"half a claim"_ for a one-sided + bucket, _"nothing shows it fires or stays quiet"_ for none. +- **The `$$$` separator behaviour is pinned as a vendor contract.** The + binary is exact-pinned, upstream calls this intended, and the failure + mode is a rule that quietly matches a narrower set than its author + wrote — so it belongs where a version bump that changes it fails + loudly. +- **`verify --schema` gains a curated `strictness: ast` example.** The + examples currently only ever show a standalone `$$$`, which is the one + form that has no trap. + +**Deliberately not done:** a static lint over pattern strings hunting +for a comma-adjacent `$$$`. `.conventions/STYLEGUIDE-CODE.md` warns +against reconstructing facts by parsing text, and the `>= 1` semantics +is sometimes exactly what the author meant — a rule about `fetch` +called _with_ options is a legitimate rule. It could only ever be an +often-wrong warning. + +**Delivery is a single PR.** The coverage check, its tests, and the spec +delta are one reviewable diff, and the check is not correct in halves. + +## Capabilities + +### Modified Capabilities + +- `cli-rule-validation`: `test` requires an ast-grep rule to populate + both fixture buckets, on the same terms it already requires of Vale. + +## Impact + +- **Behaviour change.** Rules that passed `test` before this change now + fail it — specifically any sg rule with an empty or absent `invalid:` + bucket. That is the point of the change, but it is a rejection of + previously accepted rules and the changeset says so. +- **Modified**: `packages/cli/src/rules/verify.ts` — `SgFixtureCoverage`, + `fixtureCoverage()`, and a `fixtures` field on `TestLayerResult`. + Exported because `TestLayerResult` is reachable from `verifyRule`'s + return type under `declaration: true`. +- **Modified**: `packages/cli/src/rules/inspect.ts` — the sg branch of + `testOneRule` builds its error list rather than forwarding + `tests.errors` unchanged. +- **Modified**: `packages/cli/src/rules/verify-examples.ts` — a fourth + curated example. +- **Modified**: `packages/cli/test/verify.test.ts` (one case per + coverage state, plus one for summing across several test files) and + `packages/cli/test/ast-grep-vendor-contract.test.ts` (six cases + pinning `$$$` against the separator). +- **Unchanged**: `packages/cli/src/agent/*.txt`. The recipes should warn + about the trap and about the arity-boundary fixture, but those files + are being edited on a parallel branch and the prose lands at + integration. +- **Out of scope**: the leading-`$$$` case (`foo($$$, $A)`), which + `strictness: ast` does not rescue. Its remedy is an `any:` with one + branch per arity, which is authoring guidance rather than a CLI + change; it is pinned as a contract here and belongs in the recipes. + +**Tracking:** taskless/cli#152 diff --git a/openspec/changes/sg-fixture-coverage/specs/cli-rule-validation/spec.md b/openspec/changes/sg-fixture-coverage/specs/cli-rule-validation/spec.md new file mode 100644 index 00000000..a57d3d3f --- /dev/null +++ b/openspec/changes/sg-fixture-coverage/specs/cli-rule-validation/spec.md @@ -0,0 +1,30 @@ +## MODIFIED Requirements + +### Requirement: Test runs a rule's fixtures and runs verify first + +`test` SHALL execute a rule against its test material — ast-grep test cases, Vale `pass`/`fail` fixture buckets, or the runtime harness — and SHALL run `verify` first, stopping on a verify failure without running the fixtures. + +Ordering is the point. When a rule is both malformed and under-fixtured, the fixture complaint is the less useful of the two errors and is the one that surfaces first if the checks run in the other order — so the author is told their fixtures are incomplete while the reason the rule could never have run goes unmentioned. + +A rule that populates only one bucket has proved only half of what a rule claims, whatever its engine. An engine SHALL NOT be trusted to report this itself: `ast-grep test` reports an empty `invalid:` bucket as `1 passed; 0 failed` and exits zero, so a rule that has never matched anything is indistinguishable from one that passed. + +#### Scenario: A malformed rule reports the malformation, not the fixtures + +- **WHEN** `test` runs against a rule that is both invalid and missing a fixture bucket +- **THEN** it SHALL report the validation error +- **AND** it SHALL NOT report the fixture coverage as the failure + +#### Scenario: Vale fixtures are tested per bucket + +- **WHEN** `test` runs against a Vale rule +- **THEN** every `fail/` document SHALL produce at least one finding for that rule +- **AND** every `pass/` document SHALL produce none +- **AND** a rule populating only one bucket SHALL be reported as unverified rather than passing + +#### Scenario: ast-grep fixtures are counted per bucket + +- **WHEN** `test` runs against an ast-grep rule +- **THEN** the `valid:` and `invalid:` entries SHALL be counted across every `-test.yml` file the rule owns +- **AND** a rule populating only one bucket SHALL be reported as unverified rather than passing +- **AND** a rule whose buckets are all empty or absent SHALL be reported as unverified rather than passing +- **AND** a green `ast-grep test` run SHALL NOT on its own be sufficient to report the rule as passing diff --git a/openspec/changes/sg-fixture-coverage/tasks.md b/openspec/changes/sg-fixture-coverage/tasks.md new file mode 100644 index 00000000..64975d7b --- /dev/null +++ b/openspec/changes/sg-fixture-coverage/tasks.md @@ -0,0 +1,32 @@ +Delivery shape: **single PR**. The coverage check, the tests that pin it, and the spec delta land together — the check is not correct in halves, and landing it without the spec would leave the spec endorsing the gap. + +## 1. Coverage classification + +- [x] 1.1 Add `SgFixtureCoverage` and `coverageOf()` to `rules/verify.ts`, mirroring `ValeFixtureCoverage` state for state with ast-grep's `valid`/`invalid` vocabulary +- [x] 1.2 Add `fixtureCoverage()`, parsing each `-test.yml` with the `yaml` parser already imported at the top of the file rather than deriving counts from `ast-grep test` output — the run says nothing useful, since an empty `invalid:` bucket is still `1 passed; 0 failed` +- [x] 1.3 Sum across every test file the rule owns; coverage is a property of the rule, not of one dated file +- [x] 1.4 Carry `fixtures` on `TestLayerResult` and gate `valid` on it in `runTestLayer`; keep the field exported, since `TestLayerResult` is reachable from `verifyRule`'s signature under `declaration: true` +- [x] 1.5 Treat an unreadable or unparseable test file as contributing nothing — `sg test` reports malformed test YAML itself, and guessing a bucket count from a file we could not parse is a worse error than the one already being raised + +## 2. Reporting + +- [x] 2.1 Build the sg error list in `testOneRule` instead of forwarding `tests.errors` unchanged +- [x] 2.2 Match the Vale branch's wording — "half a claim" for one-sided, "nothing shows it fires or stays quiet" for none + +## 3. Vendor contract + +- [x] 3.1 Pin that a standalone `$$$` matches a zero-argument call — the reported bug, which is not real in that shape +- [x] 3.2 Pin that `foo($A, $$$)` does NOT match a one-argument call, and that `foo($$$, $A)` matches only the one-argument call +- [x] 3.3 Pin that `strictness: ast` inside the pattern object moves the trailing-`$$$` boundary from `>= 2` to `>= 1` — not to zero, since `$A` still has to bind +- [x] 3.4 Pin that `strictness` at rule level fails the scan rather than being silently ignored, since the remedy depends on the placement + +## 4. Schema examples + +- [x] 4.1 Add a `RULE_EXAMPLES` entry showing the object-pattern `strictness: ast` form, naming the separator mechanism and the leading-`$$$` exception + +## 5. Verification + +- [x] 5.1 One `verify.test.ts` case per coverage state; nothing covered an empty bucket before this change +- [x] 5.2 `pnpm typecheck`, `pnpm lint`, `pnpm test` +- [x] 5.3 Reproduce the gap against a scratch project before and after, confirming `test` flips from `ok: true` to a coverage failure and that the `strictness: ast` remedy makes `check` fire +- [x] 5.4 Add the changeset, and say in it that previously accepted rules now fail diff --git a/packages/cli/src/agent/create-sg-rule.txt b/packages/cli/src/agent/create-sg-rule.txt index bb91f094..03cfadb3 100644 --- a/packages/cli/src/agent/create-sg-rule.txt +++ b/packages/cli/src/agent/create-sg-rule.txt @@ -1,4 +1,4 @@ -# Topic: create-sg-rule (CLI v%(CLI_VERSION)s / topic v2) +# Topic: create-sg-rule (CLI v%(CLI_VERSION)s / topic v3) ## You are here This is `create-sg-rule`. It helps you write an ast-grep rule: a check @@ -66,7 +66,7 @@ whole rule. `.taskless/rules/sg//.yml`, where `` is kebab-case and names both the directory and the file. At minimum: - `id` — kebab-case, matching the filename (e.g. `no-eval`) - - `language` — the target language + - `language` — the target language, in ast-grep's spelling (below) - `severity` — `error`, `warning`, `info`, or `hint` - `message` — a concise single-line explanation - `rule` — the ast-grep rule object @@ -74,7 +74,71 @@ whole rule. Optional but useful: `note` (multi-line guidance, supports markdown), `fix` (auto-fix pattern), `ignores` (file patterns to skip). -5. **Write the tests.** Write + **`language` is ast-grep's vocabulary, and nothing here validates + it.** The vendored rule schema types the field as a bare string with + no enum and `verify` never reads it, so the first thing that has an + opinion is the binary. ast-grep (v%(AST_GREP_VERSION)s) parses: + + %(AST_GREP_LANGUAGES)s + + Copy a spelling from that list rather than typing one that looks + right. Off-list spellings fail two different ways and neither is + caught locally: one ast-grep does not recognize at all takes the + whole scan down (`did not match any variant of untagged enum + SgLang`, so every other rule goes unreported too), and one it + recognizes but that names the wrong parser reports nothing and looks + like a clean codebase. + + Two specific traps: + - **Do not copy from `detect --json`.** It reports the + *repository's* languages in a different vocabulary — it says + `C++` where the list above says `Cpp`. + - **`Tsx` and `TypeScript` are two parsers, not aliases.** A rule + over `.tsx` files that declares `TypeScript` does not match JSX + syntax; it does not read those files at all. + +5. **Check any variadic pattern against the separator trap.** A `$$$` + next to a comma does not mean "zero or more". The `,` in the pattern + is itself an AST node, and under ast-grep's default `smart` + strictness every node in the pattern must match — so a call with no + comma cannot match a pattern that has one. Measured against the + ast-grep this CLI ships (v%(AST_GREP_VERSION)s), given the four calls + `foo()`, `foo(1)`, `foo(1,2)`, and `foo(1,2,3)`: + +| pattern | what it matches | +|--------------------|----------------------------------------------| +| `foo($$$)` | all four, `foo()` included | +| `foo($A, $$$)` | `foo(1,2)` and `foo(1,2,3)` — never `foo(1)` | +| `foo($$$, $A)` | `foo(1)` alone | +| `foo($A, $$$, $B)` | `foo(1,2)` alone | + + A standalone `$$$` needs none of this — it is the comma beside it + that narrows the pattern. The two remedies are not the same: + + - **Trailing `$$$`** — write the pattern as an object with + `strictness: ast`, which compares named AST nodes and ignores the + separator. An object pattern also requires `context` and + `selector`: + ```yaml + rule: + pattern: + context: foo($A, $$$) + selector: call_expression + strictness: ast + ``` + This moves the boundary from two arguments to one, **not to + zero** — `$A` still has to bind something, so `foo()` is still + unmatched. And `strictness` is valid only inside the pattern + object: at rule level ast-grep rejects it as an unknown field and + fails the whole scan. + - **Leading `$$$`** — `strictness: ast` does not rescue it. Use + `any` with one branch per arity you mean to cover. + + This is upstream's intended behaviour (ast-grep/ast-grep#1365, + closed as working-as-intended), not a bug waiting on a release: + 0.45.2 behaves identically, so there is no version to wait for. + +6. **Write the tests.** Write `.taskless/rules/sg//.tests/-YYYYMMDD-test.yml` with the matching `id` field plus `valid` and `invalid` arrays — at least two of each, drawn from real code where you can. The `id` must match the rule's @@ -82,7 +146,19 @@ whole rule. shape are the same ones the service writes; do not invent a different layout. -6. **Run the verify feedback loop.** Both commands take the rule's + **Both arrays must be non-empty, and `test` fails the rule if either + one is.** An empty `invalid:` is not a neutral starting point to fill + in later: ast-grep reports `1 passed; 0 failed` and exits zero over + no cases at all, so a rule that matches nothing anywhere looks + exactly like a rule that works. The `invalid:` bucket is the only + thing that demonstrates the rule can fire. + + Where the rule has an arity boundary — anything from step 5 — put a + case on each side of it. A pattern that starts at two arguments when + it was meant to start at one passes a test suite whose fixtures all + have two. + +7. **Run the verify feedback loop.** Both commands take the rule's directory as their argument: ``` %(TASKLESS_CLI)s verify .taskless/rules/sg/ --json @@ -99,7 +175,7 @@ whole rule. {"ok":true,"rules":[{"engine":"sg","ruleId":"no-eval", "ok":true,"errors":[],"ran":true}]} ``` - - `ok: true` → go to step 7. + - `ok: true` → go to step 8. - `ok: false` → read `errors` and fix. Repeat up to 3 times. | what `errors` says | fix | @@ -115,11 +191,11 @@ whole rule. one entry per rule in the report. `.taskless/rules/sg` covers every ast-grep rule; no argument at all covers the project. -7. **On success, report.** Show the rule directory and what is in it, +8. **On success, report.** Show the rule directory and what is in it, plus a one-line summary of what the rule detects. Suggest `%(TASKLESS_CLI)s agent check` to validate against the broader codebase. -8. **On failure, escalate — with confirmation.** If after the feedback +9. **On failure, escalate — with confirmation.** If after the feedback loop the rule still cannot capture the user's cases: - Delete the candidate `.taskless/rules/sg//` directory so the repo is not left with a broken rule. One `rm -rf` removes the rule @@ -136,12 +212,12 @@ whole rule. - Do NOT write to `.taskless/rule-metadata/` — locally authored rules have no metadata sidecar; they iterate via file edits. - The verify loop is the quality gate. A clean failure is a legitimate - reason to escalate, but only with the user's confirmation (step 8). + reason to escalate, but only with the user's confirmation (step 9). ## See Also - `%(TASKLESS_CLI)s agent route` — re-decide the destination -- `%(TASKLESS_CLI)s agent verify-rule` — the `verify` and `test` commands step 6 calls +- `%(TASKLESS_CLI)s agent verify-rule` — the `verify` and `test` commands step 7 calls - `%(TASKLESS_CLI)s agent improve-rule` — iterate on a rule that already exists - `%(TASKLESS_CLI)s agent create-remote-rule` — generate via the service (login) - `%(TASKLESS_CLI)s agent check` — validate the new rule against the codebase diff --git a/packages/cli/src/agent/improve-rule.txt b/packages/cli/src/agent/improve-rule.txt index dba5d1d4..54ea9cb6 100644 --- a/packages/cli/src/agent/improve-rule.txt +++ b/packages/cli/src/agent/improve-rule.txt @@ -1,4 +1,4 @@ -# Topic: improve-rule (CLI v%(CLI_VERSION)s / topic v2) +# Topic: improve-rule (CLI v%(CLI_VERSION)s / topic v3) ## Goal Iterate on an existing Taskless rule. The CLI submits the user's @@ -42,6 +42,16 @@ If the user wants the local-only flow (no API call), fetch - Is the message confusing or misleading? - Should the severity change? + **"It doesn't fire on X" is often the `$$$` separator, not a pattern + that is merely too narrow.** Read the rule's pattern for a `$$$` + sitting next to a comma before you write that guidance. The comma is + itself an AST node that has to match, so `f($A, $$$)` never sees a + one-argument call and `f($$$, $A)` sees only one-argument calls — the + pattern reads as variadic and is not. `%(TASKLESS_CLI)s agent create-sg-rule` + carries the arity table and both remedies. Guidance that asks to + widen the rule when the fix is `strictness: ast` sends the iterate + endpoint after the wrong change, and it will oblige. + 5. **Collect supporting references.** Ask the user for any code examples that should be: - **Not flagged** (currently flagged but shouldn't be) — false @@ -72,6 +82,16 @@ If the user wants the local-only flow (no API call), fetch summary of what changed. Suggest fetching `%(TASKLESS_CLI)s agent check` to validate. + **Confirm both fixture buckets are still non-empty.** `test` fails + an ast-grep rule whose `valid:` or `invalid:` array is empty, and + narrowing a pattern to kill a false positive is exactly how + `invalid:` ends up with nothing the rule still matches. Run + ``` + %(TASKLESS_CLI)s test .taskless/rules/sg/ --json + ``` + on the returned files rather than trusting them, and add a fixture + the new pattern does match if that bucket came back empty. + ## Input schema The `--from` JSON file conforms to: diff --git a/packages/cli/src/rules/inspect.ts b/packages/cli/src/rules/inspect.ts index 89cc6a52..28b042fe 100644 --- a/packages/cli/src/rules/inspect.ts +++ b/packages/cli/src/rules/inspect.ts @@ -215,11 +215,23 @@ export async function testOneRule( if (!verification.ok) { return { ...verification, ran: false }; } + const errors = [...result.tests.errors]; + // Mirrors the Vale branch below, and for the same reason: a rule that + // populated only one bucket has proved only half of what a rule claims. + // `ast-grep test` will not say so — an empty `invalid:` bucket is + // `1 passed; 0 failed`, exit zero — so the message has to come from here. + if (result.tests.fixtures !== "both") { + errors.push( + result.tests.fixtures === "none" + ? `${ruleId} has no fixtures, so nothing shows it fires or stays quiet.` + : `${ruleId} has only ${result.tests.fixtures.replace("-only", "")}: fixtures — half a claim.` + ); + } return { engine, ruleId, ok: result.tests.valid, - errors: result.tests.errors, + errors, ran: true, }; } diff --git a/packages/cli/src/rules/verify-examples.ts b/packages/cli/src/rules/verify-examples.ts index 045423e8..01b34e54 100644 --- a/packages/cli/src/rules/verify-examples.ts +++ b/packages/cli/src/rules/verify-examples.ts @@ -60,4 +60,22 @@ export const RULE_EXAMPLES = [ }, }, }, + { + description: + 'Object pattern with `strictness: ast` — detect fetch() with any trailing arguments. A `$$$` next to a comma does NOT mean "zero or more": the comma is itself a node, and under the default `smart` strictness every node in the pattern must match, so `fetch($URL, $$$REST)` skips `fetch(url)` entirely and matches only calls with two or more arguments. Write the pattern as an object with `strictness: ast` to compare named AST nodes and ignore the separator. `strictness` is only valid inside a pattern object — at rule level ast-grep rejects it as an unknown field — and an object pattern also needs `context` plus `selector`. A leading `$$$` (`fetch($$$REST, $LAST)`) is the mirror case and `strictness: ast` does not rescue it; use `any` with one branch per arity instead. A standalone `$$$` with no comma beside it already matches zero arguments and needs none of this.', + rule: { + id: "no-bare-fetch", + language: "typescript", + severity: "warning", + message: "Pass explicit options to fetch().", + note: "Set a timeout and headers rather than relying on the defaults.", + rule: { + pattern: { + context: "fetch($URL, $$$REST)", + selector: "call_expression", + strictness: "ast", + }, + }, + }, + }, ]; diff --git a/packages/cli/src/rules/verify.ts b/packages/cli/src/rules/verify.ts index 9c041dbd..0145595f 100644 --- a/packages/cli/src/rules/verify.ts +++ b/packages/cli/src/rules/verify.ts @@ -1,4 +1,5 @@ import { readFile, readdir } from "node:fs/promises"; +import { join } from "node:path"; import { spawn } from "node:child_process"; import { StringDecoder } from "node:string_decoder"; import { stripVTControlCharacters } from "node:util"; @@ -41,9 +42,36 @@ export interface RequirementsResult extends LayerResult { hasTestFile: boolean; } +/** + * Which fixture buckets a rule actually populated. + * + * The sg counterpart of `ValeFixtureCoverage`, and kept in the same four + * states for the same reason: a caller wants to say different things about + * them. `"none"` is an unwritten rule, while `"valid-only"`/`"invalid-only"` + * is a half-written one, which is the more misleading state of the two. + * + * The buckets here are the `valid:`/`invalid:` keys of ast-grep's own test + * YAML rather than Vale's `pass/`/`fail/` directories, so the names follow + * ast-grep's vocabulary. + */ +export type SgFixtureCoverage = "both" | "valid-only" | "invalid-only" | "none"; + export interface TestLayerResult extends LayerResult { passed: number; failed: number; + /** + * Which buckets held sources. Only `"both"` can be `valid: true`: an + * `invalid:` fixture proves the rule fires, a `valid:` fixture proves it + * does not over-fire, and either alone is half a claim. + * + * Without this, `ast-grep test` reports an empty `invalid:` bucket as + * `1 passed; 0 failed` and exits zero, so a rule that has never been shown + * to match anything reports `ok: true, ran: true` — the exact state a + * `foo($A, $$$)` pattern lands in, since the pattern's comma is a node that + * a one-argument call has no counterpart for and the rule silently matches + * nothing an author expected it to. + */ + fixtures: SgFixtureCoverage; } export interface VerifyResult { @@ -158,6 +186,69 @@ async function validateRequirements( return { valid: errors.length === 0, errors, hasTestFile }; } +/** Classify a rule's buckets by how many sources each held. */ +function coverageOf( + validCount: number, + invalidCount: number +): SgFixtureCoverage { + if (validCount > 0 && invalidCount > 0) return "both"; + if (validCount > 0) return "valid-only"; + if (invalidCount > 0) return "invalid-only"; + return "none"; +} + +/** + * Count what the author actually put in each bucket, across every test file + * the rule owns. + * + * Read from the author's own YAML rather than derived from `ast-grep test`'s + * output: the counts are already structured in the file, and the run says + * nothing useful about them — an empty `invalid:` bucket still reports + * `1 passed; 0 failed` and exits zero. + * + * A file that cannot be read or parsed contributes nothing. `sg test` reports + * malformed test YAML itself, and guessing at a bucket count from a file we + * could not parse would be a worse error than the one already being raised. + */ +async function fixtureCoverage( + cwd: string, + ruleId: string +): Promise { + 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; + } + 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; + if (Array.isArray(buckets.valid)) validCount += buckets.valid.length; + if (Array.isArray(buckets.invalid)) invalidCount += buckets.invalid.length; + } + + return coverageOf(validCount, invalidCount); +} + // --- Layer 3: Test execution --- /** @@ -200,7 +291,10 @@ function parseTestSummary( return found; } -async function runTests(cwd: string, ruleId: string): Promise { +async function runTests( + cwd: string, + ruleId: string +): Promise> { // Assembly names every rule's `.tests/` as its own `testConfigs` entry, so // the filter below selects a rule whose tests ast-grep already knows how to // find. @@ -314,14 +408,26 @@ async function runTestLayer( errors: ["Skipped: tests were not requested"], passed: 0, failed: 0, + fixtures: "none", }; } - if (hasTestFile) return runTests(cwd, ruleId); + if (hasTestFile) { + // Both halves, because a green `sg test` run is not on its own evidence + // the rule fires: ast-grep is content to report an empty `invalid:` + // bucket as a pass. Coverage is read from the fixtures rather than from + // the run, and a one-sided set fails the layer however the run went. + const [fixtures, result] = await Promise.all([ + fixtureCoverage(cwd, ruleId), + runTests(cwd, ruleId), + ]); + return { ...result, valid: result.valid && fixtures === "both", fixtures }; + } return { valid: false, errors: ["Skipped: no test file found"], passed: 0, failed: 0, + fixtures: "none", }; } @@ -353,7 +459,13 @@ export async function verifyRule( ruleId, schema: { valid: false, errors: [errorMessage] }, requirements: { valid: false, errors: [errorMessage] }, - tests: { valid: false, errors: [errorMessage], passed: 0, failed: 0 }, + tests: { + valid: false, + errors: [errorMessage], + passed: 0, + failed: 0, + fixtures: "none", + }, }; } @@ -388,6 +500,7 @@ export async function verifyRule( errors: ["Cannot run tests: rule file not found"], passed: 0, failed: 0, + fixtures: "none", }, }; } @@ -410,6 +523,7 @@ export async function verifyRule( errors: ["Cannot run tests: invalid YAML"], passed: 0, failed: 0, + fixtures: "none", }, }; } diff --git a/packages/cli/test/ast-grep-vendor-contract.test.ts b/packages/cli/test/ast-grep-vendor-contract.test.ts index c72d9e50..60bec275 100644 --- a/packages/cli/test/ast-grep-vendor-contract.test.ts +++ b/packages/cli/test/ast-grep-vendor-contract.test.ts @@ -157,6 +157,24 @@ const evalSource = { "src/a.ts": 'const x = eval("1");\n' }; const atLanguage = (language: string) => rule("no-eval").replace("language: TypeScript", `language: ${language}`); +/** Four calls at increasing arity, one per line, for the `$$$` cases below. */ +const aritySource = { + "src/a.ts": "foo();\nfoo(1);\nfoo(1,2);\nfoo(1,2,3);\n", +}; + +/** A rule whose `rule:` body is given verbatim, already indented. */ +const arityRule = (body: string) => + [ + "id: arity", + "language: TypeScript", + "severity: error", + "message: arity", + "note: n", + "rule:", + body, + "", + ].join("\n"); + /** Exit status of scanning one finding declared at `severity`. */ const statusAt = (severity: string) => scan( @@ -166,6 +184,13 @@ const statusAt = (severity: string) => }) ).status; +/** The call each finding matched, in file order — i.e. the arities accepted. */ +const arityMatches = (body: string): string[] => + scan(project({ rules: { arity: arityRule(body) }, sources: aritySource })) + .stdout.split("\n") + .filter((line) => line !== "") + .map((line) => (JSON.parse(line) as { text: string }).text); + /** A rule whose fixtures all pass. */ const passingProject = () => project({ @@ -547,6 +572,130 @@ withSg("ast-grep vendor contract", () => { }); }); + /** + * What `$$$` does next to a comma — the mechanism behind #152. + * + * Depended on by: every recipe and curated example that tells an author how + * to write a variadic pattern, and by the `strictness: ast` example in + * `verify-examples.ts`. The failure mode is the quiet one this whole file + * exists for: a pattern that reads as "any number of arguments" silently + * matches a narrower set, the rule finds nothing, and `check` reports a + * clean codebase. + * + * Upstream considers this working as intended (ast-grep/ast-grep#1365) and + * 0.45.2 behaves identically, so a version bump is not a fix. What a bump + * could do is change it — which is what these cases are here to catch. + */ + describe("$$$ next to a comma", () => { + it("matches a zero-argument call when $$$ stands alone", () => { + // The reported bug, and it is not real in this shape: a lone `$$$` does + // mean "zero or more". `$$$` binding nothing was never the problem. + expect(arityMatches(" pattern: foo($$$)")).toEqual([ + "foo()", + "foo(1)", + "foo(1,2)", + "foo(1,2,3)", + ]); + }); + + it("does NOT match a one-argument call for a trailing $A, $$$", () => { + // The real bug. The pattern's `,` is itself a node, and under the + // default `smart` strictness every node in the pattern must match — so + // `foo(1)`, which has no comma, fails. The pattern reads as ">= 1 + // argument" and behaves as ">= 2". + expect(arityMatches(" pattern: foo($A, $$$)")).toEqual([ + "foo(1,2)", + "foo(1,2,3)", + ]); + }); + + it("matches only the one-argument call for a leading $$$, $A", () => { + // The mirror case, and the more surprising one: the separator forces a + // comma, `$A` claims the last argument, and `$$$` is left unable to + // spread — so this collapses to exactly one arity rather than widening. + expect(arityMatches(" pattern: foo($$$, $A)")).toEqual(["foo(1)"]); + }); + + it("matches only the two-argument call when $$$ sits between two metavars", () => { + // Both separators bind, so the "any number in the middle" reading is + // wrong in both directions at once. + expect(arityMatches(" pattern: foo($A, $$$, $B)")).toEqual(["foo(1,2)"]); + }); + + it("widens a trailing $A, $$$ to one argument under strictness: ast", () => { + // The author-side remedy, and the reason `strictness` has to sit INSIDE + // the pattern object: at rule level ast-grep rejects it as an unknown + // field. `ast` compares named AST nodes and ignores the comma, so the + // boundary moves from ">= 2" to ">= 1" — it does NOT reach `foo()`, + // because `$A` still has to bind something. + expect( + arityMatches( + [ + " pattern:", + " context: foo($A, $$$)", + " selector: call_expression", + " strictness: ast", + ].join("\n") + ) + ).toEqual(["foo(1)", "foo(1,2)", "foo(1,2,3)"]); + }); + + it("accepts strictness only inside the pattern object, not at rule level", () => { + // Pins the placement the remedy depends on. At rule level this is not a + // no-op that quietly leaves `smart` in force — ast-grep fails the scan. + const cwd = project({ + rules: { + arity: arityRule(" pattern: foo($A, $$$)\n strictness: ast"), + }, + sources: aritySource, + }); + const result = scan(cwd); + expect(result.status).toBeGreaterThan(1); + expect(result.stderr).toContain("strictness"); + }); + }); + + /** + * 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 =
{eval(x)}
;\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)"); + }); + }); + /** * Relocated from `engine-layout.test.ts`, which existed only for these two. * diff --git a/packages/cli/test/recipe-cross-references.test.ts b/packages/cli/test/recipe-cross-references.test.ts index 1fb28816..0a4b4618 100644 --- a/packages/cli/test/recipe-cross-references.test.ts +++ b/packages/cli/test/recipe-cross-references.test.ts @@ -380,6 +380,24 @@ describe("recipes state engine reach from the pinned versions", () => { expect(route).toContain("E100"); }); + it("names ast-grep's languages in rendered create-sg-rule.txt", async () => { + // create-sg-rule.txt is where a `language:` field is actually written, and + // it is the one field nothing local validates — the vendored schema types + // it as a bare string and `verify` never reads it. So the spellings have to + // reach the recipe, or the first thing with an opinion is the binary. + const recipe = await rendered("create-sg-rule.txt"); + expect(recipe).toContain(astGrepLanguageList()); + // `Tsx` by name: a reader who assumes it is an alias of `TypeScript` + // writes a rule that reads `.tsx` files not at all, and this pins the + // warning rather than letting it fall out of the list above. + expect(recipe).toContain("Tsx"); + // Asserted per-file as well as in the sweep above, because this recipe is + // the one that renders BOTH the version and the language list: a marker + // that survives here reaches an agent as a variable it thinks it should + // fill in, in the middle of the field it is being warned about. + expect(recipe).not.toMatch(/%\([A-Z_]+\)s/); + }); + it("repeats the reach where a Vale matcher is written", async () => { // create-vale-rule.txt is where a glob is authored, which is the only // place the converter-dependent extensions can actually do damage. diff --git a/packages/cli/test/verify.test.ts b/packages/cli/test/verify.test.ts index 06ce9569..7c572cb1 100644 --- a/packages/cli/test/verify.test.ts +++ b/packages/cli/test/verify.test.ts @@ -6,6 +6,37 @@ import { stringify } from "yaml"; import { verifyRule, getSchemaPayload } from "../src/rules/verify"; +/** + * A rule that fires on `eval(...)`, plus one test file holding exactly the + * buckets given. Mirrors the Vale coverage cases in `vale-verify.test.ts`, + * which build the same four shapes out of `pass/` and `fail/` directories. + */ +async function coverageProject( + cwd: string, + buckets: { valid?: string[]; invalid?: string[] } +): Promise { + const rulesDirectory = join(cwd, ".taskless", "sg", "rules"); + const testsDirectory = join(cwd, ".taskless", "sg", "rule-tests"); + await mkdir(rulesDirectory, { recursive: true }); + await mkdir(testsDirectory, { recursive: true }); + await writeFile( + join(rulesDirectory, "no-eval.yml"), + stringify({ + id: "no-eval", + language: "typescript", + severity: "error", + message: "Do not use eval()", + rule: { pattern: "eval($$$)" }, + }), + "utf8" + ); + await writeFile( + join(testsDirectory, "no-eval-20260330-test.yml"), + stringify({ id: "no-eval", ...buckets }), + "utf8" + ); +} + describe("verifyRule", () => { let temporaryDirectory: string; @@ -366,6 +397,71 @@ describe("verifyRule", () => { expect(result.tests.passed).toBe(0); expect(result.tests.failed).toBe(1); }); + + describe("fixture coverage", () => { + it("does not report success for a rule with no fixtures", async () => { + // Both buckets present and both empty. `ast-grep test` calls this + // `1 passed; 0 failed` and exits zero, so without the coverage check the + // rule reports a clean pass having demonstrated nothing at all. + await coverageProject(temporaryDirectory, { valid: [], invalid: [] }); + const result = await verifyRule(temporaryDirectory, "no-eval"); + expect(result.tests.fixtures).toBe("none"); + expect(result.tests.valid).toBe(false); + expect(result.success).toBe(false); + }); + + it("does not report success for a rule with only valid fixtures", async () => { + // The misleading half, and the shape #152 arrived as: every fixture is + // a source the rule should stay quiet on, so the run is trivially green + // and the rule has never been shown to match anything. + await coverageProject(temporaryDirectory, { valid: ["const x = 1;"] }); + const result = await verifyRule(temporaryDirectory, "no-eval"); + expect(result.tests.fixtures).toBe("valid-only"); + expect(result.tests.valid).toBe(false); + }); + + it("does not report success for a rule with only invalid fixtures", async () => { + // The rule is shown to fire and never shown not to over-fire. + await coverageProject(temporaryDirectory, { + invalid: ["eval('alert(1)')"], + }); + const result = await verifyRule(temporaryDirectory, "no-eval"); + expect(result.tests.fixtures).toBe("invalid-only"); + expect(result.tests.valid).toBe(false); + }); + + it("reports both buckets for a rule that populated each", async () => { + await coverageProject(temporaryDirectory, { + valid: ["const x = 1;"], + invalid: ["eval('alert(1)')"], + }); + const result = await verifyRule(temporaryDirectory, "no-eval"); + expect(result.tests.fixtures).toBe("both"); + expect(result.tests.valid).toBe(true); + expect(result.success).toBe(true); + }); + + it("sums buckets across every test file the rule owns", async () => { + // Coverage is a property of the rule, not of one file: an author who + // splits `valid:` and `invalid:` across two dated test files has still + // made the whole claim. + await coverageProject(temporaryDirectory, { valid: ["const x = 1;"] }); + await writeFile( + join( + temporaryDirectory, + ".taskless", + "sg", + "rule-tests", + "no-eval-20260331-test.yml" + ), + stringify({ id: "no-eval", invalid: ["eval('alert(1)')"] }), + "utf8" + ); + const result = await verifyRule(temporaryDirectory, "no-eval"); + expect(result.tests.fixtures).toBe("both"); + expect(result.tests.valid).toBe(true); + }); + }); }); describe("getSchemaPayload", () => {