Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions .changeset/sg-fixture-findings.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
"@taskless/cli": patch
---

`test` now reports the findings an ast-grep rule's fixtures produced, with the
message as ast-grep rendered it. Previously only Vale and runtime rules carried
`findings`; an ast-grep rule reported an empty array, so a rule whose message
interpolated its metavariables in the wrong order fired in exactly the right
places and was reported green.

Each snippet is replayed through `ast-grep scan --stdin`, so the language comes
from the rule's own `language:` key and no temporary file is written. A finding
names the test YAML that declares the snippet, at the snippet's real line and
column in that file.

Additive, and `patch` under the pre-1.0 rule: the `findings` array was already
present on every rule result and documented as possibly empty, so nothing a
consumer reads changes shape. Vale and runtime behaviour is untouched, and the
verdict `ast-grep test` decides is unchanged — findings are gathered after it
and cannot alter it.
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
schema: spec-driven
created: 2026-09-23
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
## Why

`test` reports the findings a rule's fixtures produced for Vale and for
runtime, and reports an empty list for ast-grep. That was truthful and it was
the half of #386 left undone: an ast-grep rule whose `message` interpolates its
metavariables can have the slots in the wrong order, fire on every `invalid:`
snippet, stay quiet on every `valid:` one, and be reported green. The rendered
message is the only evidence otherwise, and `sg test` never renders it.

Two things made this harder than the other two engines, and neither is true any
more:

- **`sg test` cannot produce a finding.** Measured against the vendored 0.45.3
binary: `ast-grep test --help` offers `--filter`, `--skip-snapshot-tests`,
`--update-all`, `--interactive`, `--include-off` and `--color`, and no
`--json` or output-format flag of any kind. Its only machine-readable output
is the summary line `parseTestSummary` already reads.
- **ast-grep fixtures are not files**, so the `check <fixture path>` workaround
that exists for Vale does not exist here. Measured:
`pnpm cli check .taskless/rules/sg/ci-uses-workspace-cli/.tests --json`
returns `{"success":true,"results":[]}` — the snippets are inline YAML
scalars under `valid:`/`invalid:`, and there is no document to walk.

The route that was expected to be needed — materialise each snippet as a file
and scan it — requires a `language:` → file-extension mapping the CLI does not
own and cannot source reliably. `language:` takes ast-grep's own spelling
(`Yaml`, not `yaml`; the repo's own `ci-uses-workspace-cli` uses the
capitalised form), the set belongs to the binary, and a mapping that drifted
would silently scan a snippet as the wrong language.

`ast-grep scan --stdin` removes that question entirely. The language comes from
the rule's own `language:` key, parsed by ast-grep, so there is no mapping to
keep in step, no extension to guess, and no temp file to clean up.

## What Changes

- **`cli-rule-validation`** gains one requirement: the ast-grep engine reports
the findings its fixtures produced, with rendered messages and positions that
point back into the fixture file.
- `test --json` and the human render for an ast-grep rule now carry real
`findings`, in the shape Vale and runtime already produce. Nothing about
either of those engines changes, and the verdict `sg test` decides is
untouched — findings are gathered after it and cannot alter it.

## Delivery shape

**Single PR.** The spec delta, the collector, the tests and the archive are one
reviewable diff of roughly 500 lines, well inside the ~1200-line guidance, and
there is no intermediate state worth landing on its own.

## Why ADDED rather than MODIFIED

The standing scenario "The findings array is present and empty rather than
absent" lists four WHENs, one of which is "whose engine does not surface fixture
findings". After this change no engine is in that state, so the disjunct is
inert — but it is not false, and the other three (a rule that produced nothing,
verification that failed first, a refused run) are all still live and still
tested.

Restating a ten-scenario requirement to delete one clause of one WHEN is the
operation that silently drops scenarios on archive, and the clause costs
nothing standing. The new requirement states positively that ast-grep surfaces
its findings, which is what a reader needs; the inert disjunct is left for a
change that has reason to touch that requirement for its own sake.
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
## ADDED Requirements

### Requirement: Test reports the findings an ast-grep rule's fixtures produced

`test` SHALL report, for an ast-grep rule whose fixtures produced matches, the findings those fixtures produced, in the same shape the other engines report theirs. A consumer SHALL NOT be able to tell which engine produced a finding except by its `source`.

The rendered `message` is what this exists for. `ast-grep test` decides only whether each `invalid:` snippet fired and each `valid:` one stayed quiet, and a rule whose message interpolates its metavariables can have the slots in the wrong order while satisfying both — so the verdict cannot see the defect and the message is the only thing that can.

The findings SHALL be gathered independently of the verdict and SHALL NOT change it. `ast-grep test` remains what decides whether the rule passed; a failure to gather findings SHALL be reported as a rule with no findings rather than as a rule that failed.

A finding SHALL name the fixture FILE that declares the snippet, as a project-relative path, not the temporary or synthetic name any scanning mechanism used internally. Where the snippet is a literal block scalar, the position SHALL be the snippet's real position in that file, so the author can open it directly.

#### Scenario: The rendered message is reported, not just the verdict

- **WHEN** `test --json` runs against an ast-grep rule whose `invalid:` snippet matched
- **THEN** the rule result SHALL carry a finding for that snippet
- **AND** the finding's `message` SHALL be the message as ast-grep rendered it, with the rule's metavariables already interpolated
- **AND** a rule whose message names the same metavariables in the other order SHALL produce a different `message`, so the two cannot both pass

#### Scenario: A finding points back into the fixture file

- **WHEN** an ast-grep fixture snippet produces a finding
- **THEN** the finding's `file` SHALL be the project-relative path of the test YAML declaring the snippet
- **AND** SHALL NOT be a temporary path or ast-grep's name for a stream
- **AND** where the snippet is a literal block scalar, the finding's line and column SHALL be its position in that file rather than its position within the snippet

#### Scenario: Both buckets are reported, and named in ast-grep's vocabulary

- **WHEN** `test --json` reports an ast-grep rule's findings
- **THEN** a finding from an `invalid:` snippet SHALL carry the `fail` bucket
- **AND** a finding from a `valid:` snippet SHALL carry the `pass` bucket
- **AND** the `fail` bucket SHALL be reported even when the rule passed

#### Scenario: A valid snippet that wrongly fired is printed without --json

- **WHEN** `test` runs without `--json` and an ast-grep rule fails because a `valid:` snippet fired
- **THEN** the offending findings SHALL be printed under that rule, labelled by bucket
- **AND** they SHALL be rendered the way `check` renders a finding
- **AND** a rule that passed SHALL still print one line and no findings

#### Scenario: A language ast-grep cannot parse degrades to no findings

- **WHEN** an ast-grep rule declares a `language:` the vendored binary does not recognise
- **THEN** `test` SHALL report that rule as carrying an empty `findings` array
- **AND** SHALL NOT fail to produce a report
- **AND** the failure SHALL NOT suppress any other rule's findings

#### Scenario: Collecting findings writes nothing to disk

- **WHEN** `test` collects an ast-grep rule's fixture findings, whether the rule passes or fails
- **THEN** no file SHALL be left behind in the project tree or the temporary directory
- **AND** nothing SHALL be written that a later `check` could report against
61 changes: 61 additions & 0 deletions openspec/changes/archive/2026-09-23-sg-fixture-findings/tasks.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
## 1. Measure the routes before choosing one

- [x] 1.1 Confirm `ast-grep test --help` on the vendored 0.45.3 binary offers no
`--json` and no output-format flag, so the verdict path cannot carry
findings.
- [x] 1.2 Confirm `check` over an sg rule's `.tests` directory returns no
results, so the Vale workaround does not transfer.
- [x] 1.3 Measure `scan --stdin --json=stream`: it renders the message with
metavariables interpolated and names the document `STDIN`.
- [x] 1.4 Measure that `files:` globs do NOT suppress a stdin scan, since a
path-scoped rule would otherwise silently report nothing.
- [x] 1.5 Measure that `-r` isolates an unparseable rule (exit 8, no JSON),
rather than aborting a whole config the way the assembled path would.

## 2. Spec

- [x] 2.1 Confirm the standing "present and empty" scenario's carve-out becomes
inert rather than false, so ADDED is available and MODIFIED is not forced.
- [x] 2.2 Write the requirement as an ADDED block, restating nothing standing.
- [x] 2.3 Dry-run `openspec archive` and compare requirement and scenario title
sets in `cli-rule-validation` before and after.

## 3. Collector

- [x] 3.1 Read each `.tests/*.yml`, applying the same `id:` exclusion
`countFixtures` applies.
- [x] 3.2 Stream each `valid:`/`invalid:` snippet to
`ast-grep scan -r <rule> --stdin --json=stream`.
- [x] 3.3 Map positions from snippet coordinates back into the fixture file for
literal block scalars; anchor to the snippet's first line otherwise,
rather than reporting a confidently wrong line.
- [x] 3.4 Report `file` as the cwd-relative POSIX path of the test YAML.
- [x] 3.5 Normalise ast-grep's `"note": null` to absent, which the `test`
payload schema requires and `check` never validated.
- [x] 3.6 Return an empty list for every shortfall — no rule file, no tests, a
rule ast-grep refuses — so gathering findings can never change a verdict.

## 4. Wire in

- [x] 4.1 Populate `findings` on the sg branch of `testOneRule`, after the
verdict is decided.
- [x] 4.2 Leave Vale, runtime and the shared human renderer untouched.

## 5. Tests

- [x] 5.1 Message-order regression: a two-metavariable message, asserted on the
rendered string.
- [x] 5.2 Mutation-check it by swapping the message's slots; confirm the
assertions go red.
- [x] 5.3 Both buckets: an `invalid:` snippet that fires, and a `valid:` one
that wrongly fires and makes the run fail.
- [x] 5.4 Exact line and column, proving the mapping back into the fixture file.
- [x] 5.5 A `language:` the binary does not know degrades to empty findings.
- [x] 5.6 No temp files left behind, on a passing run and a failing one, with
the CLI's temp directory redirected so the assertion is not measuring
other suites.
- [x] 5.7 Mutation-check 5.6 by making the collector write a temp file; confirm
it goes red.
- [x] 5.8 Replace the standing "empty for an ast-grep rule" case with one that
is still true — a rule whose fixtures matched nothing — without weakening
it.
51 changes: 51 additions & 0 deletions openspec/specs/cli-rule-validation/spec.md
Original file line number Diff line number Diff line change
Expand Up @@ -419,3 +419,54 @@ A notice SHALL be reported on a rule that passed as well as on one that failed,

- **WHEN** `verify --json` reports a rule that drew no advisory
- **THEN** that rule's `notices` SHALL be present and empty

### Requirement: Test reports the findings an ast-grep rule's fixtures produced

`test` SHALL report, for an ast-grep rule whose fixtures produced matches, the findings those fixtures produced, in the same shape the other engines report theirs. A consumer SHALL NOT be able to tell which engine produced a finding except by its `source`.

The rendered `message` is what this exists for. `ast-grep test` decides only whether each `invalid:` snippet fired and each `valid:` one stayed quiet, and a rule whose message interpolates its metavariables can have the slots in the wrong order while satisfying both — so the verdict cannot see the defect and the message is the only thing that can.

The findings SHALL be gathered independently of the verdict and SHALL NOT change it. `ast-grep test` remains what decides whether the rule passed; a failure to gather findings SHALL be reported as a rule with no findings rather than as a rule that failed.

A finding SHALL name the fixture FILE that declares the snippet, as a project-relative path, not the temporary or synthetic name any scanning mechanism used internally. Where the snippet is a literal block scalar, the position SHALL be the snippet's real position in that file, so the author can open it directly.

#### Scenario: The rendered message is reported, not just the verdict

- **WHEN** `test --json` runs against an ast-grep rule whose `invalid:` snippet matched
- **THEN** the rule result SHALL carry a finding for that snippet
- **AND** the finding's `message` SHALL be the message as ast-grep rendered it, with the rule's metavariables already interpolated
- **AND** a rule whose message names the same metavariables in the other order SHALL produce a different `message`, so the two cannot both pass

#### Scenario: A finding points back into the fixture file

- **WHEN** an ast-grep fixture snippet produces a finding
- **THEN** the finding's `file` SHALL be the project-relative path of the test YAML declaring the snippet
- **AND** SHALL NOT be a temporary path or ast-grep's name for a stream
- **AND** where the snippet is a literal block scalar, the finding's line and column SHALL be its position in that file rather than its position within the snippet

#### Scenario: Both buckets are reported, and named in ast-grep's vocabulary

- **WHEN** `test --json` reports an ast-grep rule's findings
- **THEN** a finding from an `invalid:` snippet SHALL carry the `fail` bucket
- **AND** a finding from a `valid:` snippet SHALL carry the `pass` bucket
- **AND** the `fail` bucket SHALL be reported even when the rule passed

#### Scenario: A valid snippet that wrongly fired is printed without --json

- **WHEN** `test` runs without `--json` and an ast-grep rule fails because a `valid:` snippet fired
- **THEN** the offending findings SHALL be printed under that rule, labelled by bucket
- **AND** they SHALL be rendered the way `check` renders a finding
- **AND** a rule that passed SHALL still print one line and no findings

#### Scenario: A language ast-grep cannot parse degrades to no findings

- **WHEN** an ast-grep rule declares a `language:` the vendored binary does not recognise
- **THEN** `test` SHALL report that rule as carrying an empty `findings` array
- **AND** SHALL NOT fail to produce a report
- **AND** the failure SHALL NOT suppress any other rule's findings

#### Scenario: Collecting findings writes nothing to disk

- **WHEN** `test` collects an ast-grep rule's fixture findings, whether the rule passes or fails
- **THEN** no file SHALL be left behind in the project tree or the temporary directory
- **AND** nothing SHALL be written that a later `check` could report against
16 changes: 8 additions & 8 deletions packages/cli/src/rules/inspect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
ruleFilePath,
} from "./engines";
import { describeCoverageShortfall, type FixtureFinding } from "./fixtures";
import { collectSgFixtureFindings } from "./sg-fixture-findings";
import { type EngineName } from "./layout";
import {
assessCaptureDirectory,
Expand Down Expand Up @@ -498,14 +499,13 @@ export async function testOneRule(
violations,
ran: true,
notices: verification.notices,
// Empty, and true. `sg test` reports `test result: ok. N passed; N
// failed;` and nothing else — the vendored binary has no `--json` and no
// output-format flag — and its fixtures are inline YAML scalars rather
// than files, so there is no document to attribute a finding to.
// Surfacing them needs a different mechanism than reading what the
// engine already handed us, which is the whole of what this does for the
// other two engines.
findings: [],
// `sg test` still decides the verdict above and still cannot produce a
// finding — the vendored binary has no `--json` and no output-format
// flag. So unlike the other two engines, these are not read back out of
// what the run already handed us: each fixture snippet is replayed
// through `scan --stdin`, which renders the message the author needs to
// see. Gathered AFTER the verdict, and never able to change it.
findings: await collectSgFixtureFindings(cwd, ruleId),
};
}

Expand Down
Loading
Loading