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
17 changes: 17 additions & 0 deletions .changeset/test-show-fixture-findings.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
---
"@taskless/cli": patch
---

`test --json` now reports the findings a rule's fixtures produced.

Each rule result carries a `findings` array: the same finding shape `check --json` prints — `source`, `ruleId`, `severity`, `message`, `file`, `range`, `matchedText`, and the optional `note` and `fix` — plus a `bucket` of `"pass"` or `"fail"` naming the fixture that produced it. The rendered `message` is the point. A rule whose message interpolates its captures can have the slots in the wrong order, fire on every `fail/` fixture, stay quiet on every `pass/` one, and be reported as a rule that passed; the rendered message is the only evidence otherwise, and until now the only way to see it was a second `check` run against a fixture path you had to construct yourself.

The array is **always present and empty rather than absent** — for a rule that produced nothing, one whose verification failed before its fixtures ran, a refused runtime rule, `verify` (which runs no fixtures at all), and ast-grep rules, whose findings are not surfaced yet. Fail-bucket findings are reported on a passing run too, since that is the only run that produces them.

On the human path a passing rule still prints one line. A **failing** rule now prints the findings that bear on the failure beneath it, labelled by bucket and rendered the way `check` renders a finding. That includes pass-bucket findings: `pass fixture wrongly fired: <file>` said _that_ it happened and never what matched.

Vale and runtime rules only. ast-grep follows separately: the vendored binary's `sg test` has no `--json` and no output-format flag, and its fixtures are inline YAML scalars rather than files.

No flag was added, and nothing new is executed: the findings were already in hand and were being discarded.

**If you consume `@taskless/cli/schemas`:** `zod`'s `.parse()` strips keys the schema does not declare, so a consumer still on the previously published `verifyTestOutputSchema` will silently drop `findings` from the payload it returns until the dependency is upgraded. Nothing breaks — but the field simply not being there, on a CLI that is emitting it, is the kind of thing that generates a bug report.
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
## Why

`test --json` reports a boolean per rule and nothing else:

```json
{
"ok": true,
"rules": [
{
"engine": "vale",
"ruleId": "no-hedging",
"ok": true,
"errors": [],
"violations": [],
"ran": true
}
]
}
```

`check <fixture path> --json` over the same rule's `fail/` fixture returns three findings carrying a rendered `message`, `file`, `range`, `matchedText` and `severity`. The rendered message is what a verdict cannot express: a `substitution` rule whose message interpolates its captures can have the `%s` slots in the wrong order, fire on every `fail/` fixture, stay quiet on every `pass/` one, and be reported as a rule that passed. An agent authoring a rule has no way to see the message it wrote except by running a second command against a path it has to construct.

The findings already exist in-process and are discarded at a seam:

- **Vale** — `packages/cli/src/rules/vale/verify.ts` reduces `outcome.results`, a `CheckResult[]`, to `firedIn`, a `Set<string>` of file paths, and derives `missingFailures`/`unexpectedFindings` from the set. Message, range, matched text and severity die at that line.
- **Runtime** — `packages/cli/src/rules/runtime/run-fixtures.ts` reads only `execution.findings.length`. `findings` is a `CheckResult[]`.

So this is plumbing rather than a new engine invocation: no extra subprocess, no second scan, nothing re-run.

## What Changes

- **`test --json` carries a `findings` array on every rule result**, each entry the `CheckResult` shape `check --json` already prints plus a `bucket` of `"pass"` or `"fail"`. `CheckResult` is reused verbatim rather than narrowed: it is already the published shape for a finding, and a second shape is a second thing to keep in sync.
- **The array is always present, never omitted.** Empty for an ast-grep rule, for a rule whose `verify` failed before fixtures ran, for a refused runtime rule, and for a rule that simply produced nothing. A key that is sometimes absent is one a consumer learns to treat as optional, and the reading that follows is that absence means zero — which is exactly the inference this change exists to make unnecessary.
- **Fail-bucket findings are reported on a green run.** They are the `%s`-order evidence, and a payload that carries the evidence only once the rule is failing carries it at the one moment it is no longer needed.
- **The human path keeps one line per passing rule.** Under a failing rule the offending findings print underneath, labelled by bucket, through `util/format.ts`'s existing `formatText` — the same renderer `check` uses, over the same `CheckResult[]` it takes as its parameter. A pass-bucket finding prints there: today `unexpectedFindings` names the file that wrongly fired and stops, which says _that_ it happened and not _what_ matched. Fail-bucket findings are not dumped on a green human run; that is what `--json` is for.
- **No new flag.** No `--verbose`: it has no precedent anywhere in `packages/cli/src` or `openspec/specs`, and the house pattern is a rich `--json` beside a terse human render. No `--include-fixtures` on `check` either: an explicit fixture path already works, because the `.taskless/**` exclusion in `rules/vale/run.ts` is gated on `wholeProject`. A flag an agent has to know to pass is the ergonomics problem being filed, not its fix.
- **Vale's `unexpectedFindings` and `missingFailures` are left alone.** They are internal to `ValeRuleVerification`, and whether the findings array subsumes them is a separate decision from whether the findings are reported at all.
- **ast-grep reports an empty array in this PR.** `runTests` spawns `sg test` and regex-parses `test result: ok. N passed; N failed;`. The vendored 0.45.3 binary has no `--json` and no output-format flag, and its fixtures are inline YAML scalars rather than files, so there is no directory to attribute a finding to. Surfacing them needs a different mechanism, which is PR 2.

## Delivery shape

**Single PR, targeting `main`.** The spec, the implementation and the archive land together. It is not a stack: nothing is stacked above this branch and nothing needs to be, because the two engines that already hold a `CheckResult[]` are the whole of what this change plumbs.

ast-grep is deliberately **out of scope rather than a later slice of this change**, and gets its own proposal when it is written. Surfacing its findings is not more of the same work: `sg test` has no `--json` and no output-format flag, and its fixtures are inline YAML scalars rather than files, so it needs a way to get structured output out of a binary that offers none. Nothing in this change's spec delta requires it — the requirement states that the array is empty for an engine that does not yet surface its fixture findings, which is exactly what an ast-grep rule reports here and is true rather than misleading.

## Capabilities

### New Capabilities

None.

### Modified Capabilities

- `cli-rule-validation`: "Test runs a rule's fixtures and runs verify first" restated to require the per-rule `findings` array, its always-present-and-possibly-empty contract, fail-bucket reporting on a green `--json` run, and the human rendering under a failing rule.

Deliberately untouched: "A rejection names the constraint it violated", which governs `violations[]`. Those are CONSTRAINT violations — what a rule broke about its own shape — and a fixture finding is what a rule reported about a document. Conflating the two is the confusion this change is correcting.

## Impact

- `packages/cli/src/schemas/verify-test.ts`: `findings` on `ruleResultSchema`, the `CheckResult` shape plus `bucket`, defaulted to `[]` so the field is present in output for `verify` as well as `test`.
- `packages/cli/src/rules/vale/verify.ts`: keep the `CheckResult[]` beside `firedIn` and attribute each finding to a bucket by its file path.
- `packages/cli/src/rules/runtime/run-fixtures.ts`: keep `execution.findings`, tagged with the case's bucket.
- `packages/cli/src/rules/inspect.ts`: `findings` on `RuleTestResult`, populated per engine and empty on every early return.
- `packages/cli/src/commands/verify.ts`: the human render under a failing rule.
- Tests: `packages/cli/test/test-fixture-findings.test.ts`.
- `.changeset/test-show-fixture-findings.md`.
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
## 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.

A runtime rule's fixtures execute code, so they SHALL run only when `--dangerously-run-scripts` is passed, and under no other mechanism. `test` SHALL NOT consult the rule service to decide this, and SHALL make no network request in the course of running fixtures.

That is deliberately stricter than the policy `check` applies, not softer. `check` executes rules as a side effect of scanning a repository, so a blessed signature admits code the user never asked to run and a reconcile stands between the request and the execution. `test` runs fixtures because the user asked for them, so the verb is the consent and the flag is the confirmation; a rule that `check` would run unflagged on a blessed signature still requires the flag here. Nothing executes under `test` that would not have executed under the shared policy.

A run refused for want of the flag SHALL be reported as not run, and SHALL be reported as neither a pass nor a failure: the rule is not defective, and no action available to its holder would make a failure green. The refusal SHALL name the flag, and SHALL NOT direct the reader to authenticate — authenticating cannot bless a rule that never left the working tree, so naming it would offer a fix that is not one.

`test` SHALL report, per rule, the findings its fixtures produced, as a `findings` array carrying the same finding shape `check --json` prints — `source`, `ruleId`, `severity`, `message`, `file`, `range`, `matchedText` and the optional `note` and `fix` — plus the `bucket` the fixture that produced it belongs to.

The RENDERED message is the point, and it is what a verdict cannot carry. A rule whose message interpolates its captures can have its slots in the wrong order and still fire on every `fail/` fixture and stay quiet on every `pass/` one, so a boolean verdict reports it as a rule that passed. The only evidence that the message says what its author meant is the message as the engine rendered it, against material the author wrote, which `test` already has in hand and discards.

The array SHALL be present on every rule result and SHALL be empty rather than absent when there is nothing to report — including for an engine that does not yet surface its fixture findings, a rule whose verification failed before fixtures ran, and a run the execution policy refused. A consumer SHALL NOT have to distinguish "this rule produced no findings" from "this command does not report findings", because a key that is sometimes absent is one a reader learns to treat as optional, and the reading that follows is that its absence means zero.

Fail-bucket findings SHALL be reported under `--json` on a passing run. They are the evidence, and a payload that carries the evidence only once the rule is already failing carries it at the one moment it is no longer needed.

The human rendering SHALL stay a single line per passing rule. Under a FAILING rule it SHALL print the findings that bear on the failure, labelled by bucket, using the same renderer `check` prints findings with, so one finding does not read two ways depending on which command surfaced it. A pass-bucket finding SHALL be printed there: it is a fixture that wrongly fired, and naming the file says only that it happened, while the finding says what matched.

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

#### Scenario: Runtime fixtures are run per case

- **WHEN** `test` runs against a runtime rule with `--dangerously-run-scripts`
- **THEN** each directory under `.tests/fail/` SHALL be passed to the check as its `root` and SHALL produce at least one finding
- **AND** each directory under `.tests/pass/` SHALL be passed as its `root` and SHALL produce none
- **AND** a rule populating only one bucket SHALL be reported as unverified rather than passing
- **AND** a rule holding no fixture cases at all SHALL be reported as unverified rather than passing

#### Scenario: A runtime rule without the flag is reported as not run

- **WHEN** `test` runs against a runtime rule without `--dangerously-run-scripts`
- **THEN** the rule SHALL NOT be reported as passing
- **AND** the output SHALL say the fixtures did not run and why
- **AND** the output SHALL name `--dangerously-run-scripts` as what would run them
- **AND** the output SHALL NOT direct the reader to authenticate
- **AND** the rule SHALL NOT be counted among the rules tested
- **AND** the refusal alone SHALL NOT fail the command

#### Scenario: Testing a runtime rule reaches no network

- **WHEN** `test` runs against a runtime rule, with or without `--dangerously-run-scripts`
- **THEN** the CLI SHALL NOT request a token, resolve an organization, or reconcile
- **AND** the outcome SHALL NOT depend on authentication state, a git remote, or the availability of the rule service

#### Scenario: A case that never reaches the check is reported as a fixture defect

- **WHEN** a fixture case produces no narrow matches, so the check is never invoked
- **THEN** the CLI SHALL report that case as a fixture defect naming the case
- **AND** SHALL say the check did not run
- **AND** SHALL do so whether the case is in `pass/` or `fail/`, since a case that never invokes the check is evidence about the fixture rather than about the rule

#### Scenario: A check that throws is distinguished from one that finds nothing

- **WHEN** a runtime rule's check raises while running a fixture case
- **THEN** that SHALL be reported as the check failing, not as the case producing no findings

#### Scenario: Test reports the findings its fixtures produced

- **WHEN** `test --json` runs against a rule whose fixtures produced findings
- **THEN** each rule result SHALL carry a `findings` array
- **AND** each entry SHALL carry the rendered `message`, `file`, `range`, `matchedText` and `severity` that `check --json` reports for the same finding
- **AND** each entry SHALL name the bucket of the fixture that produced it
- **AND** the fail-bucket findings SHALL be reported even when the rule passed

#### Scenario: The findings array is present and empty rather than absent

- **WHEN** `test --json` reports a rule that produced no findings, whose verification failed before its fixtures ran, whose run was refused, or whose engine does not surface fixture findings
- **THEN** the rule result SHALL still carry a `findings` array
- **AND** that array SHALL be empty

#### Scenario: A failing rule prints the findings that bear on the failure

- **WHEN** `test` runs without `--json` and a rule fails because a `pass/` fixture 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
## 1. Spec

- [x] 1.1 Restate "Test runs a rule's fixtures and runs verify first" in full — all 8 standing scenarios, byte-identical titles, and the full normative prose — with the findings requirement added. Verify by archiving into a throwaway commit and counting scenarios before and after.

## 2. Schema

- [x] 2.1 `schemas/verify-test.ts`: a `findings` array on `ruleResultSchema`, carrying the `CheckResult` fields plus `bucket: "pass" | "fail"`, defaulted to `[]` so every rule result in both commands' output carries it.

## 3. Engines

- [x] 3.1 `rules/vale/verify.ts`: carry the filtered `CheckResult[]` on `ValeRuleVerification` alongside `firedIn`, each finding bucketed by whether its file is a `pass/` or `fail/` fixture. Leave `missingFailures`/`unexpectedFindings` as they are.
- [x] 3.2 `rules/runtime/run-fixtures.ts`: collect `execution.findings` per case onto the report, tagged with the case's bucket.
- [x] 3.3 `rules/inspect.ts`: `findings` on `RuleTestResult`, populated from both engines, `[]` for ast-grep and on every path that returns before fixtures run.

## 4. Human rendering

- [x] 4.1 `commands/verify.ts`: under a failing rule print the bearing findings, labelled by bucket, through `util/format.ts`'s `formatText`. One line per passing rule, unchanged.

## 5. Tests

- [x] 5.1 Vale and runtime, both buckets, `--json` and human paths.
- [x] 5.2 A message-order regression: a `substitution` rule whose rendered message interpolates a token, asserting the rendered text, failing if the slots are swapped.
- [x] 5.3 A pass-bucket finding appears on the human path when the rule failed.
- [x] 5.4 `findings` present and empty for an ast-grep rule.

## 6. Release

- [x] 6.1 `.changeset/test-show-fixture-findings.md`, `patch`, noting that `zod`'s `parse()` strips unknown keys so a consumer on the old published schema drops the new field until they upgrade.
- [x] 6.2 `pnpm build`, `pnpm typecheck`, `pnpm lint`, `pnpm test`.
Loading
Loading