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

.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.

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

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

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

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,23 +66,99 @@ whole rule.
    `.taskless/rules/sg/<id>/<id>.yml`, where `<id>` 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
 
    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/<id>/.tests/<id>-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
    `id` so ast-grep test filtering pairs them. These paths and this
    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/<id> --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/<id>/` 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