From 34ab31ae30e7eac11179a6e1443dc391b0b06238 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 23 Sep 2026 11:44:58 -0700 Subject: [PATCH 1/3] spec(cli-rule-validation): test reports its fixtures' findings Restates "Test runs a rule's fixtures and runs verify first" in full: a MODIFIED block replaces the standing requirement, so all 8 existing scenarios and the full normative prose are carried verbatim or archive deletes them silently. Five of the eight are unrelated to this change and are present only to survive. Adds the findings requirement: a per-rule array carrying check's own finding shape plus the fixture bucket, always present and empty rather than absent, with the fail bucket reported on a green --json run, and the human render printing the bearing findings under a failing rule. --- .../test-show-fixture-findings/proposal.md | 64 +++++++++++ .../specs/cli-rule-validation/spec.md | 103 ++++++++++++++++++ .../test-show-fixture-findings/tasks.md | 29 +++++ 3 files changed, 196 insertions(+) create mode 100644 openspec/changes/test-show-fixture-findings/proposal.md create mode 100644 openspec/changes/test-show-fixture-findings/specs/cli-rule-validation/spec.md create mode 100644 openspec/changes/test-show-fixture-findings/tasks.md diff --git a/openspec/changes/test-show-fixture-findings/proposal.md b/openspec/changes/test-show-fixture-findings/proposal.md new file mode 100644 index 00000000..87b981fe --- /dev/null +++ b/openspec/changes/test-show-fixture-findings/proposal.md @@ -0,0 +1,64 @@ +## 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 --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` 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 + +**Stacked, merging forward — this is the bottom PR of two.** Each unit reaches production on its own: this one plumbs the two engines that already hold `CheckResult[]`, and an ast-grep rule reporting `findings: []` is true rather than misleading, since the array's contract is "present and empty when there is nothing to report". PR 2 adds ast-grep, which needs a way to get structured output out of a binary that offers none, and would otherwise hold a working feature behind an unrelated investigation. The changeset lands here, at the bottom, and PR 2 extends it. + +## 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`. diff --git a/openspec/changes/test-show-fixture-findings/specs/cli-rule-validation/spec.md b/openspec/changes/test-show-fixture-findings/specs/cli-rule-validation/spec.md new file mode 100644 index 00000000..de9789d3 --- /dev/null +++ b/openspec/changes/test-show-fixture-findings/specs/cli-rule-validation/spec.md @@ -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 diff --git a/openspec/changes/test-show-fixture-findings/tasks.md b/openspec/changes/test-show-fixture-findings/tasks.md new file mode 100644 index 00000000..ffa2c8b0 --- /dev/null +++ b/openspec/changes/test-show-fixture-findings/tasks.md @@ -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`. From ab6a9c7e79b01f8db47502b9bd927a0455f20eab Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 23 Sep 2026 11:45:13 -0700 Subject: [PATCH 2/3] feat(test): report the findings a rule's fixtures produced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit test --json now carries a findings array on every rule result: check's own CheckResult shape plus the bucket of the fixture that produced it. The rendered message is the point — a rule whose message interpolates its captures can have the slots reversed, fire on every fail/ fixture, stay quiet on every pass/ one, and be reported as a rule that passed. The findings were already in hand and were being discarded: Vale reduced its CheckResult[] to a Set of file paths, and the runtime runner read only findings.length. Nothing new is executed and no flag was added. The array is always present and empty rather than absent — a key that is sometimes absent is one a reader learns to treat as optional, and then reads absence as zero. Fail-bucket findings are reported on a passing run, since that is the only run that produces them. On the human path a passing rule still prints one line; a failing rule prints the bearing findings beneath it, labelled by bucket and rendered through util/format.ts's formatText, the same renderer check uses. Pass-bucket findings print there: "pass fixture wrongly fired: " said that it happened and never what matched. ast-grep reports an empty array for now: sg test has no --json and no output-format flag, and its fixtures are inline YAML scalars rather than files. --- .changeset/test-show-fixture-findings.md | 17 + packages/cli/src/commands/verify.ts | 50 ++ packages/cli/src/rules/fixtures.ts | 28 ++ packages/cli/src/rules/inspect.ts | 35 +- packages/cli/src/rules/runtime/fixtures.ts | 4 +- .../cli/src/rules/runtime/run-fixtures.ts | 26 +- packages/cli/src/rules/vale/verify.ts | 42 +- packages/cli/src/schemas/verify-test.ts | 50 ++ .../cli/test/test-fixture-findings.test.ts | 460 ++++++++++++++++++ packages/cli/test/vale-verify.test.ts | 33 ++ 10 files changed, 734 insertions(+), 11 deletions(-) create mode 100644 .changeset/test-show-fixture-findings.md create mode 100644 packages/cli/test/test-fixture-findings.test.ts diff --git a/.changeset/test-show-fixture-findings.md b/.changeset/test-show-fixture-findings.md new file mode 100644 index 00000000..7e94cbe3 --- /dev/null +++ b/.changeset/test-show-fixture-findings.md @@ -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: ` 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. diff --git a/packages/cli/src/commands/verify.ts b/packages/cli/src/commands/verify.ts index 5e6b074c..1b2d7b96 100644 --- a/packages/cli/src/commands/verify.ts +++ b/packages/cli/src/commands/verify.ts @@ -5,6 +5,7 @@ import { defineCommand } from "citty"; import { ensureTasklessDirectory } from "../filesystem/directory"; import { requireCurrentSchema } from "../filesystem/migrate"; +import type { FixtureFinding } from "../rules/fixtures"; import { testOneRule, verifyOneRule, @@ -19,8 +20,51 @@ import { import { outputSchema as verifyTestOutputSchema } from "../schemas/verify-test"; import { makeErrorEnvelope, writeJsonError } from "../types/errors"; import { CLIError } from "../util/cli-error"; +import { formatText } from "../util/format"; import { markNotice } from "../util/notices"; +/** + * The findings a result carries, or none. + * + * `verify` never runs fixtures, so its results have no `findings` at all, while + * `test`'s always do — a narrowing rather than an optional field, because the + * two commands share this renderer and only one of them has anything to render. + */ +function findingsOf( + result: RuleVerification | RuleTestResult +): FixtureFinding[] { + return "findings" in result ? result.findings : []; +} + +/** + * The findings under a FAILING rule, labelled by bucket. + * + * Printed only on a failure, and only on the human path. A green run says one + * line per rule, which is what makes a wall of ticks readable; the evidence a + * passing run produced goes to `--json`, where something is reading it on + * purpose. + * + * `pass` first, because a `pass/` fixture that fired is usually the reason the + * rule failed, and because it is the case the existing output serves worst: + * `pass fixture wrongly fired: ` says THAT it happened and never what + * matched, which is the one thing the author has to know to fix it. + * + * Rendered through `check`'s own `formatText` rather than a second renderer. + * These are `CheckResult`s, which is its parameter type, and a finding that + * read two ways depending on which command surfaced it would be a worse + * outcome than any formatting this costs. + */ +function printFindings(findings: FixtureFinding[]): void { + for (const bucket of ["pass", "fail"] as const) { + const inBucket = findings.filter((finding) => finding.bucket === bucket); + if (inBucket.length === 0) continue; + console.log(` ${bucket} fixture findings:`); + for (const line of formatText(inBucket).split("\n")) { + console.log(line === "" ? "" : ` ${line}`); + } + } +} + /** * The shared body of `verify` and `test`. * @@ -171,6 +215,12 @@ async function runOverPath(options: { console.log(line); } } + // Under the errors, because the errors say which fixture is wrong and + // these say what the rule reported about it. A refused run has nothing + // to show: nothing ran. + if (!result.ok && !isRefused(result)) { + printFindings(findingsOf(result)); + } } // A rule that did not run is not among the rules tested. Counting it there // is the summary half of the same defect as the tick: "1 rule(s) tested" diff --git a/packages/cli/src/rules/fixtures.ts b/packages/cli/src/rules/fixtures.ts index cd569de1..b2adc22e 100644 --- a/packages/cli/src/rules/fixtures.ts +++ b/packages/cli/src/rules/fixtures.ts @@ -2,6 +2,7 @@ import type { Dirent } from "node:fs"; import { readdir } from "node:fs/promises"; import { isMissingDirectory } from "./errno"; +import type { CheckResult } from "../types/check"; /** * The two questions every engine's fixture reader asks, answered once. @@ -98,3 +99,30 @@ export function describeCoverageShortfall( ? `${ruleId} has no fixtures, so nothing shows it fires or stays quiet.` : `${ruleId} has only ${coverage.replace("-only", "")}${suffix} fixtures — half a claim.`; } + +/** + * The two buckets a fixture lives in, over the two engines whose buckets are + * directories. + * + * Declared here rather than beside either engine because a bucket is now part + * of what `test` REPORTS, not only of how each engine reads its own tests: a + * consumer reading `findings[].bucket` is reading one vocabulary, and it should + * not be one engine's copy of it that they happen to be reading. ast-grep's + * buckets are the `valid:`/`invalid:` keys of its own test YAML and keep their + * own spelling, for {@link FixtureCoverage}'s reason. + */ +export type FixtureBucket = "pass" | "fail"; + +/** + * One finding a fixture produced, with the bucket that produced it. + * + * {@link CheckResult} verbatim, which is deliberate: it is already the shape + * `check --json` prints, so a finding means the same thing whichever command + * surfaced it, and there is one shape to keep in step with the engines rather + * than two. `bucket` is the only addition, and it is the half a `CheckResult` + * cannot carry — a finding does not know whether the document it came from was + * supposed to be clean. + */ +export interface FixtureFinding extends CheckResult { + bucket: FixtureBucket; +} diff --git a/packages/cli/src/rules/inspect.ts b/packages/cli/src/rules/inspect.ts index 372d719a..cdd9e4cd 100644 --- a/packages/cli/src/rules/inspect.ts +++ b/packages/cli/src/rules/inspect.ts @@ -8,7 +8,7 @@ import { ruleDirectory, ruleFilePath, } from "./engines"; -import { describeCoverageShortfall } from "./fixtures"; +import { describeCoverageShortfall, type FixtureFinding } from "./fixtures"; import { type EngineName } from "./layout"; import { assessCaptureDirectory, @@ -116,6 +116,21 @@ export interface RuleTestResult { * on a pass, since a pass is the case it exists for. */ notices: string[]; + /** + * Every finding this rule's fixtures produced, tagged with its bucket. + * + * ALWAYS PRESENT, empty rather than absent. A rule that produced nothing, a + * rule whose `verify` failed before its fixtures ran, a refused runtime rule + * and an ast-grep rule all report `[]`, because a consumer must never have to + * tell "this rule produced no findings" from "this command does not report + * findings" — a key that is sometimes absent is one a reader learns to treat + * as optional, and then reads absence as zero. + * + * Both buckets, including on a rule that passed. The `fail` bucket is the + * evidence that the rendered messages say what their author meant, and it is + * only ever produced by the runs that passed. + */ + findings: FixtureFinding[]; } /** What `test` needs beyond a rule, all of it about the runtime engine. */ @@ -425,7 +440,7 @@ export async function testOneRule( // call, which would spawn `sg test` twice for one answer. const verification = await withIdCollision(cwd, ruleId, verdict); if (!verification.ok) { - return { ...verification, ran: false }; + return { ...verification, ran: false, findings: [] }; } const errors = [...result.tests.errors]; const violations: RuleViolation[] = []; @@ -483,12 +498,20 @@ 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: [], }; } const verification = await verifyOneRule(cwd, rule); if (!verification.ok) { - return { ...verification, ran: false }; + return { ...verification, ran: false, findings: [] }; } if (engine === "vale") { @@ -502,6 +525,7 @@ export async function testOneRule( violations: [], ran: false, notices: [], + findings: [], }; } const errors: string[] = []; @@ -523,6 +547,7 @@ export async function testOneRule( violations: [], ran: true, notices: result.notices, + findings: result.findings, }; } @@ -561,6 +586,7 @@ export async function testOneRule( errors: [reason], violations: [], ran: false, + findings: [], refused: reason, notices: [], }; @@ -585,6 +611,7 @@ export async function testOneRule( errors: [reason], violations: [], ran: false, + findings: [], refused: reason, notices: [], }; @@ -606,6 +633,7 @@ export async function testOneRule( violations: [], ran: false, notices: [], + findings: [], }; } @@ -622,5 +650,6 @@ export async function testOneRule( violations: [], ran: true, notices: [], + findings: report.findings, }; } diff --git a/packages/cli/src/rules/runtime/fixtures.ts b/packages/cli/src/rules/runtime/fixtures.ts index 099750d8..38c81def 100644 --- a/packages/cli/src/rules/runtime/fixtures.ts +++ b/packages/cli/src/rules/runtime/fixtures.ts @@ -3,13 +3,11 @@ import { join } from "node:path"; import { bucketEntries, classifyCoverage, + type FixtureBucket, type FixtureCoverage, } from "../fixtures"; import { ruleTestsDirectory } from "../engines"; -/** The two buckets a fixture case can live in. */ -export type FixtureBucket = "pass" | "fail"; - /** * One fixture case: a DIRECTORY, whose path is the `root` the harness hands * the check. diff --git a/packages/cli/src/rules/runtime/run-fixtures.ts b/packages/cli/src/rules/runtime/run-fixtures.ts index 88edb97f..39c0ec56 100644 --- a/packages/cli/src/rules/runtime/run-fixtures.ts +++ b/packages/cli/src/rules/runtime/run-fixtures.ts @@ -1,4 +1,4 @@ -import { describeCoverageShortfall } from "../fixtures"; +import { describeCoverageShortfall, type FixtureFinding } from "../fixtures"; import type { RuntimeRule } from "./discover"; import { executeRuntimeRuleDetailed } from "./harness"; import type { RuntimeRunOptions } from "./harness"; @@ -43,6 +43,20 @@ export interface RuntimeFixtureReport { neverInvoked: string[]; /** Cases where the narrow or the check itself failed, rather than found nothing. */ checkFailures: FixtureCheckFailure[]; + /** + * Every finding the check produced, each tagged with its case's bucket. + * + * The lists above are counts of cases that broke an expectation; this is what + * the check actually SAID, which is the half a verdict cannot carry. A rule + * whose message interpolates its captures can have the slots reversed and + * still fire in every `fail/` case and stay quiet in every `pass/` one. + * + * A case counted in `checkFailures` contributes nothing here. A harness + * failure arrives as a single synthesized error-severity finding, and + * reporting that as something the rule found would attribute the harness's + * crash to the rule — the exact confusion `checkFailures` exists to prevent. + */ + findings: FixtureFinding[]; } /** `fail/case-1` — the bucket is half the identity of a case. */ @@ -71,6 +85,7 @@ export async function runRuntimeFixtures( const unexpectedFindings: string[] = []; const neverInvoked: string[] = []; const checkFailures: FixtureCheckFailure[] = []; + const findings: FixtureFinding[] = []; for (const fixtureCase of fixtures.cases) { const execution = await executeRuntimeRuleDetailed( @@ -95,6 +110,14 @@ export async function runRuntimeFixtures( continue; } + // Collected before the bucket expectations are judged, so a case that met + // its expectation still reports what it found. That is the whole point on + // the `fail/` side: those findings are the evidence the messages render + // correctly, and they exist only on the runs that passed. + for (const finding of execution.findings) { + findings.push({ ...finding, bucket: fixtureCase.bucket }); + } + if (fixtureCase.bucket === "fail") { if (execution.findings.length === 0) missingFailures.push(label(fixtureCase)); @@ -118,6 +141,7 @@ export async function runRuntimeFixtures( unexpectedFindings, neverInvoked, checkFailures, + findings, }; } diff --git a/packages/cli/src/rules/vale/verify.ts b/packages/cli/src/rules/vale/verify.ts index 2abb74cd..40049024 100644 --- a/packages/cli/src/rules/vale/verify.ts +++ b/packages/cli/src/rules/vale/verify.ts @@ -8,6 +8,7 @@ import { bucketEntries, classifyCoverage, type FixtureCoverage, + type FixtureFinding, } from "../fixtures"; import { runVale, type ValeRunOutcome } from "./run"; @@ -146,6 +147,21 @@ export interface ValeRuleVerification { missingFailures: string[]; /** Fixtures that should have been clean and were not. */ unexpectedFindings: string[]; + /** + * Every finding the fixtures produced, each tagged with its bucket. + * + * Carried ALONGSIDE the two lists above rather than instead of them. They + * answer "which fixture is wrong", which is what decides `passed`; this + * answers "what did the rule actually say", which is what an author needs to + * see and what no verdict can express — a rule whose message interpolates + * its captures can have the slots reversed, fire on every `fail/` fixture, + * stay quiet on every `pass/` one, and pass. + * + * Empty when Vale was never run, which a one-sided fixture set short-circuits + * into, for `findings`' reason on the reported envelope: absent and empty must + * not be two different answers to the same question. + */ + findings: FixtureFinding[]; /** * Which buckets held documents. Only `"both"` can be `passed: true`: a * `fail/` fixture proves the rule fires, a `pass/` fixture proves it does not @@ -247,6 +263,7 @@ export async function verifyValeRule( passed: false, missingFailures: [], unexpectedFindings: [], + findings: [], fixtures, notices: [], }; @@ -269,11 +286,27 @@ export async function verifyValeRule( // Only findings for the rule under test count. The config isolates it, so // this should be every finding — filtering anyway means a leak shows up as // a verification that still measures the right thing. - const firedIn = new Set( - outcome.results - .filter((result) => result.ruleId === ruleId) - .map((result) => result.file) + const ruleResults = outcome.results.filter( + (result) => result.ruleId === ruleId + ); + const firedIn = new Set(ruleResults.map((result) => result.file)); + + // The findings themselves, kept rather than reduced away. `firedIn` above + // is the same data with everything but the file path thrown out, and what + // it throws out — the rendered message, the range, the matched text — is + // the only evidence an author has that the rule says what they wrote. + // + // Bucketed by membership in `pass/`, with `fail/` as the remainder rather + // than as a second lookup. Vale ran over the rule's tests directory and + // nothing else, and `fixtureFiles` refuses a nested directory inside a + // bucket, so every file here is a fixture in one bucket or the other. + const passPaths = new Set( + passFixtures.map((file) => toRelativePosix(cwd, file)) ); + const findings: FixtureFinding[] = ruleResults.map((result) => ({ + ...result, + bucket: passPaths.has(result.file) ? "pass" : "fail", + })); const missingFailures = failFixtures .map((file) => toRelativePosix(cwd, file)) @@ -287,6 +320,7 @@ export async function verifyValeRule( passed: missingFailures.length === 0 && unexpectedFindings.length === 0, missingFailures, unexpectedFindings, + findings, fixtures, notices: outcome.notices, }; diff --git a/packages/cli/src/schemas/verify-test.ts b/packages/cli/src/schemas/verify-test.ts index 41dacdab..eee38134 100644 --- a/packages/cli/src/schemas/verify-test.ts +++ b/packages/cli/src/schemas/verify-test.ts @@ -7,6 +7,50 @@ import { z } from "zod"; * so they share one envelope. `ran` is `test`-only: `verify` never reaches a * test run, and `test` reports `false` when verification failed first. */ +/** + * A position, zero-based in both axes, exactly as `check --json` reports one. + */ +const positionSchema = z.object({ + line: z.number().int(), + column: z.number().int(), +}); + +/** + * One finding a rule's fixtures produced. + * + * The `CheckResult` shape `check --json` already prints, plus the `bucket` the + * fixture that produced it lives in. Reused verbatim rather than narrowed to + * what `test` "needs": a finding means the same thing whichever command + * surfaced it, and a second, smaller shape would be a second thing to keep in + * step with the engines. + * + * `message` is the field this exists for. It is the RENDERED message, with the + * rule's captures already interpolated, and it is the only evidence that a + * rule whose message interpolates its captures put the slots in the right + * order — such a rule fires on every `fail/` fixture, stays quiet on every + * `pass/` one, and a boolean verdict reports it as a rule that passed. + */ +const fixtureFindingSchema = z.object({ + source: z.string().describe("The engine that produced it"), + ruleId: z.string(), + severity: z.enum(["error", "warning", "info", "hint"]), + message: z + .string() + .describe( + "The message as the engine rendered it, with the rule's captures already interpolated" + ), + note: z.string().optional(), + file: z.string().describe("The fixture the finding was reported against"), + range: z.object({ start: positionSchema, end: positionSchema }), + matchedText: z.string(), + fix: z.string().optional(), + bucket: z + .enum(["pass", "fail"]) + .describe( + "The fixture bucket that produced it. A `pass` finding is a rule that fired where it should not have; a `fail` finding is the rule doing its job" + ), +}); + const ruleResultSchema = z.object({ engine: z.enum(["sg", "vale", "runtime"]), ruleId: z.string(), @@ -47,6 +91,12 @@ const ruleResultSchema = z.object({ .describe( "Things true about the rule that do not make it a failure, reported even on a pass. One notice per element, so a consumer can render each on its own; empty when there is nothing to say, never absent" ), + findings: z + .array(fixtureFindingSchema) + .default([]) + .describe( + "The findings this rule's fixtures produced, each tagged with its bucket. ALWAYS PRESENT, empty rather than absent — for a rule that produced nothing, for one whose verification failed before its fixtures ran, for a refused run, for `verify`, which runs no fixtures at all, and for an engine whose fixture findings are not surfaced yet. A key that is sometimes absent is one a reader learns to treat as optional, and reads absence as zero. The `fail` bucket is reported on a passing run too: it is the evidence that the rendered messages say what their author meant, and a payload carrying that only once the rule is already failing carries it at the one moment it is no longer needed" + ), }); /** Output schema for `taskless verify --json` and `taskless test --json`. */ diff --git a/packages/cli/test/test-fixture-findings.test.ts b/packages/cli/test/test-fixture-findings.test.ts new file mode 100644 index 00000000..c1f8d384 --- /dev/null +++ b/packages/cli/test/test-fixture-findings.test.ts @@ -0,0 +1,460 @@ +import { execFile } from "node:child_process"; +import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { promisify } from "node:util"; + +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { findValeBinary } from "../src/rules/vale/binary"; +import { cliRejectionToResult } from "./support/spawn-cli"; + +/** + * The findings `test` reports, per rule and per fixture bucket. + * + * The defect this file exists for: `test --json` reported a boolean per rule + * and nothing else, while the findings that produced the boolean were in hand + * and thrown away — Vale's reduced to a `Set` of file paths, runtime's to + * `findings.length`. 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, so the assertions on it below are the + * point of the whole file and must not be relaxed to "some message". + * + * These spawn the built CLI rather than calling `testOneRule`, because the + * always-present `findings` key and the human rendering are both properties of + * what the command prints. + */ + +const execFileAsync = promisify(execFile); +const binPath = resolve(import.meta.dirname, "../dist/index.js"); + +const withVale = findValeBinary().path === undefined ? describe.skip : describe; + +let cwd: string; + +async function runCli(args: string[]) { + try { + const { stdout, stderr } = await execFileAsync("node", [binPath, ...args]); + return { stdout, stderr, exitCode: 0 }; + } catch (error) { + return cliRejectionToResult(error, [binPath, ...args]); + } +} + +interface Finding { + source: string; + ruleId: string; + severity: string; + message: string; + file: string; + range: { + start: { line: number; column: number }; + end: { line: number; column: number }; + }; + matchedText: string; + bucket: "pass" | "fail"; +} + +interface Report { + ok: boolean; + rules: { + engine: string; + ruleId: string; + ok: boolean; + errors: string[]; + ran?: boolean; + refused?: string; + findings: Finding[]; + }[]; +} + +function findingsOf(report: Report, bucket?: "pass" | "fail"): Finding[] { + const findings = report.rules.flatMap((rule) => rule.findings); + return bucket === undefined + ? findings + : findings.filter((finding) => finding.bucket === bucket); +} + +beforeEach(async () => { + cwd = await mkdtemp(join(tmpdir(), "tskl-findings-")); + await runCli(["init", "-d", cwd]); +}); + +afterEach(async () => { + await rm(cwd, { recursive: true, force: true }); +}); + +/* -------------------------------------------------------------------------- */ +/* Vale */ +/* -------------------------------------------------------------------------- */ + +const VALE_RULE = "use-email"; + +/** + * A `substitution` rule, chosen deliberately over `existence`. + * + * `substitution` is the check whose message takes TWO `%s` slots — the + * replacement and the match — so it is the one that can be wrong in the way a + * verdict cannot see. Vale fills them in that order, which is the opposite of + * the order they appear in the swap, and an author who assumes otherwise ships + * a rule that tells every reader to replace `email` with `e-mail`. + */ +const SUBSTITUTION_STYLE = [ + "extends: substitution", + `message: "Use '%s' instead of '%s'"`, + "level: warning", + "ignorecase: true", + "swap:", + " e-mail: email", + "", +].join("\n"); + +const VALE_CONFIG = [ + "[*.md]", + `tskl) rule = ${VALE_RULE}`, + `${VALE_RULE}.${VALE_RULE} = YES`, + "", +].join("\n"); + +/** The message Vale renders, with the slots in the order Vale fills them. */ +const RENDERED = "Use 'email' instead of 'e-mail'"; + +async function writeValeRule(options: { passFires?: boolean } = {}) { + const directory = join(cwd, ".taskless", "rules", "vale", VALE_RULE); + await mkdir(join(directory, ".tests", "pass"), { recursive: true }); + await mkdir(join(directory, ".tests", "fail"), { recursive: true }); + await writeFile(join(directory, `${VALE_RULE}.yml`), SUBSTITUTION_STYLE); + await writeFile(join(directory, ".vale.ini"), VALE_CONFIG); + await writeFile( + join(directory, ".tests", "fail", "bad.md"), + "Send an e-mail today.\n" + ); + await writeFile( + join(directory, ".tests", "pass", "ok.md"), + (options.passFires ?? false) + ? "Send an e-mail now.\n" + : "Send an email today.\n" + ); +} + +async function testVale(...extra: string[]) { + return runCli([ + "test", + `.taskless/rules/vale/${VALE_RULE}`, + "-d", + cwd, + ...extra, + ]); +} + +withVale("a Vale rule's fixture findings", () => { + it("reports the rendered message, so a swapped %s pair cannot pass", async () => { + // THE test. `substitution` renders replacement-then-match; a rule written + // the other way round renders "Use 'e-mail' instead of 'email'" and fires + // and stays quiet in exactly the same places, so nothing but this string + // distinguishes the two. + await writeValeRule(); + + const { stdout, exitCode } = await testVale("--json"); + const report = JSON.parse(stdout) as Report; + + expect(exitCode).toBe(0); + expect(report.ok).toBe(true); + const messages = findingsOf(report).map((finding) => finding.message); + expect(messages).toEqual([RENDERED]); + expect(messages).not.toContain("Use 'e-mail' instead of 'email'"); + }); + + it("carries the whole check finding, not a summary of one", async () => { + await writeValeRule(); + + const { stdout } = await testVale("--json"); + const report = JSON.parse(stdout) as Report; + const finding = findingsOf(report)[0]; + + expect(finding).toBeDefined(); + expect(finding?.source).toBe("vale"); + expect(finding?.ruleId).toBe(VALE_RULE); + expect(finding?.severity).toBe("warning"); + expect(finding?.matchedText).toBe("e-mail"); + expect(finding?.file).toContain("fail/bad.md"); + expect(finding?.range.start.line).toBe(0); + expect(finding?.range.start.column).toBe(8); + }); + + it("reports the fail bucket on a run that passed", async () => { + // The evidence is only ever produced by a green run. A payload that + // carried it once the rule was already failing would carry it at the one + // moment nobody needs it. + await writeValeRule(); + + const { stdout } = await testVale("--json"); + const report = JSON.parse(stdout) as Report; + + expect(report.rules[0]?.ok).toBe(true); + expect(findingsOf(report, "fail")).toHaveLength(1); + expect(findingsOf(report, "pass")).toHaveLength(0); + }); + + it("tags a wrongly-fired pass fixture as the pass bucket", async () => { + await writeValeRule({ passFires: true }); + + const { stdout, exitCode } = await testVale("--json"); + const report = JSON.parse(stdout) as Report; + + expect(exitCode).toBe(1); + expect(report.ok).toBe(false); + const passFindings = findingsOf(report, "pass"); + expect(passFindings).toHaveLength(1); + expect(passFindings[0]?.file).toContain("pass/ok.md"); + expect(passFindings[0]?.message).toBe(RENDERED); + }); + + it("prints one line and no findings when the rule passed", async () => { + // The human path stays scannable. A wall of ticks is the format's whole + // value, and the evidence goes to `--json`, where something reads it on + // purpose. + await writeValeRule(); + + const { stdout } = await testVale(); + + expect(stdout).toContain(`✓ vale/${VALE_RULE}`); + expect(stdout).not.toContain(RENDERED); + expect(stdout).not.toContain("fixture findings"); + }); + + it("prints what matched under a failing rule, not only which file", async () => { + // `pass fixture wrongly fired: ` says THAT it happened. The author + // still has to open the file to learn what matched, which is the one thing + // they need to fix it. + await writeValeRule({ passFires: true }); + + const { stdout } = await testVale(); + + expect(stdout).toContain(`✗ vale/${VALE_RULE}`); + expect(stdout).toContain("pass fixture wrongly fired:"); + expect(stdout).toContain("pass fixture findings:"); + expect(stdout).toContain(RENDERED); + // `check`'s own renderer, so a finding does not read two ways depending on + // which command surfaced it. + expect(stdout).toContain(`warning[${VALE_RULE}] ${RENDERED}`); + expect(stdout).toContain("> e-mail"); + }); +}); + +/* -------------------------------------------------------------------------- */ +/* Runtime */ +/* -------------------------------------------------------------------------- */ + +const RUNTIME_RULE = "no-eval"; + +const EVAL_CAPTURE = [ + "id: no-eval-abc12345", + "language: typescript", + "rule:", + " pattern: eval($ARG)", + "metadata:", + " taskless:", + " version: 1", + " kind: runtime", + " name: no-eval", + " check: check.ts", + " match: anchor", + "", +].join("\n"); + +const RUNTIME_MESSAGE = "eval on a non-literal is not allowed"; + +/** Reports only where the argument is not a literal, so a pass case can run. */ +const FLAGS_DYNAMIC_EVAL = String.raw`import { readFileSync } from "node:fs"; +import { join } from "node:path"; + +export default async function (root, matches) { + return matches + .filter((m) => !readFileSync(join(root, m.file), "utf8").includes('eval("')) + .map((m) => ({ + file: m.file, + line: m.line, + column: m.column, + message: "${RUNTIME_MESSAGE}", + severity: "warning", + })); +} +`; + +/** Reports every match, so the `pass/` case wrongly fires. */ +const FLAGS_EVERY_MATCH = `export default async function (root, matches) { + return matches.map((m) => ({ + file: m.file, + line: m.line, + column: m.column, + message: "${RUNTIME_MESSAGE}", + severity: "warning", + })); +} +`; + +async function writeRuntimeRule(check: string = FLAGS_DYNAMIC_EVAL) { + const directory = join(cwd, ".taskless", "rules", "runtime", RUNTIME_RULE); + await mkdir(join(directory, "captures"), { recursive: true }); + await writeFile(join(directory, "captures", "eval.yml"), EVAL_CAPTURE); + await writeFile(join(directory, "check.ts"), check); + + for (const [bucket, source] of [ + ["fail", "const input = globalThis.userInput;\neval(input);\n"], + ["pass", 'eval("1 + 1");\n'], + ] as const) { + const caseDirectory = join(directory, ".tests", bucket, `${bucket}-case`); + await mkdir(caseDirectory, { recursive: true }); + await writeFile(join(caseDirectory, "sample.ts"), source); + } +} + +async function testRuntime(...extra: string[]) { + return runCli([ + "test", + `.taskless/rules/runtime/${RUNTIME_RULE}`, + "-d", + cwd, + "--dangerously-run-scripts", + ...extra, + ]); +} + +describe("a runtime rule's fixture findings", () => { + it("reports what the check said, bucketed by case, on a passing run", async () => { + await writeRuntimeRule(); + + const { stdout, exitCode } = await testRuntime("--json"); + const report = JSON.parse(stdout) as Report; + + expect(exitCode).toBe(0); + expect(report.rules[0]?.ok).toBe(true); + const failFindings = findingsOf(report, "fail"); + expect(failFindings).toHaveLength(1); + expect(failFindings[0]?.message).toBe(RUNTIME_MESSAGE); + expect(failFindings[0]?.severity).toBe("warning"); + expect(failFindings[0]?.file).toContain("sample.ts"); + expect(findingsOf(report, "pass")).toHaveLength(0); + }); + + it("tags a wrongly-fired pass case as the pass bucket", async () => { + await writeRuntimeRule(FLAGS_EVERY_MATCH); + + const { stdout, exitCode } = await testRuntime("--json"); + const report = JSON.parse(stdout) as Report; + + expect(exitCode).toBe(1); + expect(findingsOf(report, "pass")).toHaveLength(1); + expect(findingsOf(report, "fail")).toHaveLength(1); + }); + + it("prints the pass-bucket finding under a failing rule", async () => { + await writeRuntimeRule(FLAGS_EVERY_MATCH); + + const { stdout } = await testRuntime(); + + expect(stdout).toContain(`✗ runtime/${RUNTIME_RULE}`); + expect(stdout).toContain("pass fixture findings:"); + expect(stdout).toContain(RUNTIME_MESSAGE); + }); + + it("prints one line and no findings when the rule passed", async () => { + await writeRuntimeRule(); + + const { stdout } = await testRuntime(); + + expect(stdout).toContain(`✓ runtime/${RUNTIME_RULE}`); + expect(stdout).not.toContain("fixture findings"); + }); + + it("carries an empty array, not an absent key, on a refused run", async () => { + // Without the flag nothing executes, so there is nothing to report — and + // that is the case a consumer must not have to tell apart from "this + // command does not report findings". + await writeRuntimeRule(); + + const { stdout } = await runCli([ + "test", + `.taskless/rules/runtime/${RUNTIME_RULE}`, + "-d", + cwd, + "--json", + ]); + const report = JSON.parse(stdout) as Report; + + expect(report.rules[0]?.refused).toBeDefined(); + expect(report.rules[0]?.findings).toEqual([]); + }); +}); + +/* -------------------------------------------------------------------------- */ +/* Always present */ +/* -------------------------------------------------------------------------- */ + +async function writeSgRule() { + const directory = join(cwd, ".taskless", "rules", "sg", "no-eval-sg"); + await mkdir(join(directory, ".tests"), { recursive: true }); + await writeFile( + join(directory, "no-eval-sg.yml"), + "id: no-eval-sg\nlanguage: TypeScript\nseverity: error\n" + + "message: no eval\nrule:\n pattern: eval($ARG)\n" + ); + await writeFile( + join(directory, ".tests", "no-eval-sg-test.yml"), + "id: no-eval-sg\nvalid:\n - const a = 1;\ninvalid:\n - eval(x);\n" + ); +} + +describe("the findings array is present on every rule result", () => { + it("is empty rather than absent for an ast-grep rule", async () => { + // `sg test` reports a count and nothing else, and its fixtures are inline + // YAML scalars rather than files. Empty is the true answer here, and it has + // to be stated rather than left to an absent key. + await writeSgRule(); + + const { stdout, exitCode } = await runCli([ + "test", + ".taskless/rules/sg/no-eval-sg", + "-d", + cwd, + "--json", + ]); + const report = JSON.parse(stdout) as Report; + + expect(exitCode).toBe(0); + expect(report.rules[0]?.ok).toBe(true); + expect(report.rules[0]?.findings).toEqual([]); + }); + + it("is empty rather than absent for verify, which runs no fixtures", async () => { + await writeSgRule(); + + const { stdout } = await runCli([ + "verify", + ".taskless/rules/sg/no-eval-sg", + "-d", + cwd, + "--json", + ]); + const report = JSON.parse(stdout) as Report; + + expect(report.rules[0]?.findings).toEqual([]); + }); + + it("is empty rather than absent when verification failed before fixtures ran", async () => { + // No `.vale.ini`, so `verify` fails and `test` never reaches the fixtures. + const directory = join(cwd, ".taskless", "rules", "vale", VALE_RULE); + await mkdir(directory, { recursive: true }); + await writeFile(join(directory, `${VALE_RULE}.yml`), SUBSTITUTION_STYLE); + + const { stdout, exitCode } = await testVale("--json"); + const report = JSON.parse(stdout) as Report; + + expect(exitCode).toBe(1); + expect(report.rules[0]?.ok).toBe(false); + expect(report.rules[0]?.ran).toBe(false); + expect(report.rules[0]?.findings).toEqual([]); + }); +}); diff --git a/packages/cli/test/vale-verify.test.ts b/packages/cli/test/vale-verify.test.ts index ec3c2f95..48cdff2f 100644 --- a/packages/cli/test/vale-verify.test.ts +++ b/packages/cli/test/vale-verify.test.ts @@ -247,6 +247,11 @@ withVale("verifyValeRule", () => { } ); const result = await verifyValeRule(cwd, "no-simply"); + // Exhaustive on purpose: a field added to the verification shows up here + // rather than sliding in unasserted. `findings` is spelled out per finding + // for the same reason — it is carried rather than reduced to the file paths + // in `missingFailures`/`unexpectedFindings`, and the rendered `message` is + // the field the array exists to carry. expect(result).toEqual({ ruleId: "no-simply", passed: true, @@ -254,6 +259,34 @@ withVale("verifyValeRule", () => { unexpectedFindings: [], fixtures: "both", notices: [], + findings: [ + { + source: "vale", + ruleId: "no-simply", + severity: "warning", + message: "Avoid 'simply'", + file: ".taskless/rules/vale/no-simply/.tests/fail/a.md", + range: { + start: { line: 0, column: 5 }, + end: { line: 0, column: 10 }, + }, + matchedText: "simply", + bucket: "fail", + }, + { + source: "vale", + ruleId: "no-simply", + severity: "warning", + message: "Avoid 'simply'", + file: ".taskless/rules/vale/no-simply/.tests/fail/b.md", + range: { + start: { line: 0, column: 8 }, + end: { line: 0, column: 13 }, + }, + matchedText: "simply", + bucket: "fail", + }, + ], }); }); From c87633dfbf19cc3877b2fff5ff17813615d69a1c Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 23 Sep 2026 13:34:20 -0700 Subject: [PATCH 3/3] chore(openspec): archive test-show-fixture-findings This PR targets main, so it is the tip and the tip archives. The delivery shape in the proposal said "stacked, merging forward, bottom of two", which was never true of a branch based on main; corrected to a single PR with ast-grep as a separate future change rather than a later slice of this one. Checked before archiving that no part of the delta describes behaviour only the ast-grep half would deliver. Every ast-grep mention in the MODIFIED block is standing text already on main, and the new prose makes an empty array explicitly conformant for an engine that does not yet surface its fixture findings. Verified by title set rather than by count: cli-rule-validation 37 -> 40 scenarios, 7 -> 7 requirements, zero lost, and the result is identical to the pre-flight dry run. --- .../proposal.md | 4 ++- .../specs/cli-rule-validation/spec.md | 0 .../tasks.md | 0 openspec/specs/cli-rule-validation/spec.md | 31 +++++++++++++++++++ 4 files changed, 34 insertions(+), 1 deletion(-) rename openspec/changes/{test-show-fixture-findings => archive/2026-09-23-test-show-fixture-findings}/proposal.md (85%) rename openspec/changes/{test-show-fixture-findings => archive/2026-09-23-test-show-fixture-findings}/specs/cli-rule-validation/spec.md (100%) rename openspec/changes/{test-show-fixture-findings => archive/2026-09-23-test-show-fixture-findings}/tasks.md (100%) diff --git a/openspec/changes/test-show-fixture-findings/proposal.md b/openspec/changes/archive/2026-09-23-test-show-fixture-findings/proposal.md similarity index 85% rename from openspec/changes/test-show-fixture-findings/proposal.md rename to openspec/changes/archive/2026-09-23-test-show-fixture-findings/proposal.md index 87b981fe..5318f1e7 100644 --- a/openspec/changes/test-show-fixture-findings/proposal.md +++ b/openspec/changes/archive/2026-09-23-test-show-fixture-findings/proposal.md @@ -39,7 +39,9 @@ So this is plumbing rather than a new engine invocation: no extra subprocess, no ## Delivery shape -**Stacked, merging forward — this is the bottom PR of two.** Each unit reaches production on its own: this one plumbs the two engines that already hold `CheckResult[]`, and an ast-grep rule reporting `findings: []` is true rather than misleading, since the array's contract is "present and empty when there is nothing to report". PR 2 adds ast-grep, which needs a way to get structured output out of a binary that offers none, and would otherwise hold a working feature behind an unrelated investigation. The changeset lands here, at the bottom, and PR 2 extends it. +**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 diff --git a/openspec/changes/test-show-fixture-findings/specs/cli-rule-validation/spec.md b/openspec/changes/archive/2026-09-23-test-show-fixture-findings/specs/cli-rule-validation/spec.md similarity index 100% rename from openspec/changes/test-show-fixture-findings/specs/cli-rule-validation/spec.md rename to openspec/changes/archive/2026-09-23-test-show-fixture-findings/specs/cli-rule-validation/spec.md diff --git a/openspec/changes/test-show-fixture-findings/tasks.md b/openspec/changes/archive/2026-09-23-test-show-fixture-findings/tasks.md similarity index 100% rename from openspec/changes/test-show-fixture-findings/tasks.md rename to openspec/changes/archive/2026-09-23-test-show-fixture-findings/tasks.md diff --git a/openspec/specs/cli-rule-validation/spec.md b/openspec/specs/cli-rule-validation/spec.md index d6ed8c7a..c19b07ee 100644 --- a/openspec/specs/cli-rule-validation/spec.md +++ b/openspec/specs/cli-rule-validation/spec.md @@ -126,6 +126,16 @@ That is deliberately stricter than the policy `check` applies, not softer. `chec 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 @@ -183,6 +193,27 @@ A run refused for want of the flag SHALL be reported as not run, and SHALL be re - **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 + ### Requirement: The generation loop runs verify and test The rule generation loop SHALL run `verify` and then `test` against a newly authored or newly delivered rule, and SHALL treat a failure of either as a rule that is not ready to report as complete.