diff --git a/.changeset/verify-status-field.md b/.changeset/verify-status-field.md new file mode 100644 index 00000000..0b471396 --- /dev/null +++ b/.changeset/verify-status-field.md @@ -0,0 +1,5 @@ +--- +"@taskless/cli": patch +--- + +`verify --json` now reports a `status` on each rule: `passed`, `failed`, `not-run` (the rule failed before its tests ran) or `refused` (the execution policy declined to run them). Branch on it instead of combining `ok`, `ran` and `refused`, where reading `ok` alone could report a rule that never ran as passing. The change is additive: `ok`, `ran` and `refused` are still reported and are now derived from `status`, so they always agree with it. `validate` runs no tests and does not report `status`. diff --git a/openspec/changes/archive/2026-11-07-verify-status-field/proposal.md b/openspec/changes/archive/2026-11-07-verify-status-field/proposal.md new file mode 100644 index 00000000..6eeb819b --- /dev/null +++ b/openspec/changes/archive/2026-11-07-verify-status-field/proposal.md @@ -0,0 +1,27 @@ +## Why + +A rule's `verify` result has four outcomes (passed, failed after its tests ran, failed before they ran, refused by the execution policy) spread across three fields: `ok`, `ran` and `refused`. Every consumer combines them with its own precedence, and a consumer that reads `ok` alone can report a rule that never ran as passing. The encoding also admits combinations no producer should emit, such as `ok: true` with `ran: false` (#627). + +## What Changes + +- Each `verify --json` rule result carries `status`, one of `passed`, `failed`, `not-run` or `refused`, computed once by the producer. +- `ok`, `ran` and `refused` stay, derived from `status` by one constructor, so they can no longer disagree with it. The change is additive for existing `--json` consumers. +- `verify`'s marks, summary counts and exit code, and the report draft's test status, read `status` instead of combining the old fields. +- `validate` runs no tests and does not report `status`. +- The `validate-rule`, `create-sg-rule` and `create-vale-rule` recipes name `status` as the field to branch on. + +## Capabilities + +### New Capabilities + +None. + +### Modified Capabilities + +- `cli-rule-validation`: `verify` reports one outcome field per rule. + +## Impact + +- `packages/cli/src/rules/inspect.ts`, `src/schemas/rule-check.ts`, `src/commands/verify.ts`, `src/report/draft.ts`, and three recipes. +- Exit codes, top-level `ok` and every existing field are unchanged. +- Delivery shape: **single PR**. The field, its consumers and the spec only make sense together, and the diff fits one review. diff --git a/openspec/changes/archive/2026-11-07-verify-status-field/specs/cli-rule-validation/spec.md b/openspec/changes/archive/2026-11-07-verify-status-field/specs/cli-rule-validation/spec.md new file mode 100644 index 00000000..936d93f4 --- /dev/null +++ b/openspec/changes/archive/2026-11-07-verify-status-field/specs/cli-rule-validation/spec.md @@ -0,0 +1,46 @@ +## ADDED Requirements + +### Requirement: Verify reports one outcome per rule + +Under `--json`, each rule result from `verify` SHALL carry a `status` naming its outcome, one of `passed`, `failed`, `not-run` or `refused`: + +- `passed`: the rule's tests ran and passed. +- `failed`: the rule's tests ran and something failed. +- `not-run`: the rule failed before its tests ran, because `validate` rejected it or its fixtures could not be read, and the execution policy did not refuse it. +- `refused`: the execution policy declined to run the rule's tests. + +`ok`, `ran` and `refused` SHALL remain on the result and SHALL be derived from `status`: `ok` is true exactly when `status` is `passed`, `ran` is true exactly when `status` is `passed` or `failed`, and `refused` is present exactly when `status` is `refused`. The CLI's own readers of a verification (the human marks, the summary counts, the exit code and the report draft) SHALL read `status` rather than combining the other three. + +`validate` runs no tests and SHALL NOT report `status`. + +**Rationale.** Three fields for four outcomes let every consumer invent its own precedence, and let a consumer that reads `ok` alone report a rule that never ran as passing. One field computed by the producer removes the choice. Deriving the old fields from it, rather than setting them alongside it, keeps them consistent for the consumers that already read them. + +#### Scenario: A passing rule reports passed + +- **WHEN** `verify --json` runs a rule whose tests run and pass +- **THEN** the rule's `status` SHALL be `passed` +- **AND** its `ok` SHALL be `true` and its `ran` SHALL be `true` + +#### Scenario: A failing rule reports failed + +- **WHEN** `verify --json` runs a rule whose tests run and a case fails +- **THEN** the rule's `status` SHALL be `failed` +- **AND** its `ok` SHALL be `false` and its `ran` SHALL be `true` + +#### Scenario: A rule validate rejected reports not-run + +- **WHEN** `verify --json` runs a rule that `validate` rejects +- **THEN** the rule's `status` SHALL be `not-run` +- **AND** its `ok` SHALL be `false`, its `ran` SHALL be `false`, and it SHALL carry no `refused` +- **AND** `verify` SHALL exit 1 + +#### Scenario: A refused rule reports refused + +- **WHEN** `verify --json` runs a runtime rule without `--dangerously-run-scripts` +- **THEN** the rule's `status` SHALL be `refused` +- **AND** its `ok` SHALL be `false`, its `ran` SHALL be `false`, and `refused` SHALL carry the reason + +#### Scenario: Validate reports no status + +- **WHEN** `validate --json` reports a rule +- **THEN** the rule's result SHALL NOT carry `status` diff --git a/openspec/changes/archive/2026-11-07-verify-status-field/tasks.md b/openspec/changes/archive/2026-11-07-verify-status-field/tasks.md new file mode 100644 index 00000000..e1435f34 --- /dev/null +++ b/openspec/changes/archive/2026-11-07-verify-status-field/tasks.md @@ -0,0 +1,14 @@ +## 1. Status field + +- [x] 1.1 Add `status` to `RuleVerification`, built by one constructor that derives `ok`, `ran` and `refused` from it +- [x] 1.2 Describe `status` in the `rule-check` schema as the field to branch on, verify-only +- [x] 1.3 Read `status` in `commands/verify.ts` and `report/draft.ts` + +## 2. Recipes + +- [x] 2.1 Name `status` in `validate-rule`, `create-sg-rule` and `create-vale-rule` + +## 3. Tests + +- [x] 3.1 Assert `ok`, `ran` and `refused` agree with `status` for all four outcomes +- [x] 3.2 Assert `status` end to end for a pass, a failure, a rule validate rejected, a refused runtime rule, and its absence from `validate` diff --git a/openspec/specs/cli-rule-validation/spec.md b/openspec/specs/cli-rule-validation/spec.md index 4604d663..444d6941 100644 --- a/openspec/specs/cli-rule-validation/spec.md +++ b/openspec/specs/cli-rule-validation/spec.md @@ -520,3 +520,48 @@ A trap that ast-grep itself refuses (a keyword or Markdown inline construct name - **WHEN** `verify` runs against a rule whose only finding from `validate` is a trap notice, and whose fixtures pass - **THEN** the rule SHALL be reported as passing - **AND** the notice SHALL be reported on it + +### Requirement: Verify reports one outcome per rule + +Under `--json`, each rule result from `verify` SHALL carry a `status` naming its outcome, one of `passed`, `failed`, `not-run` or `refused`: + +- `passed`: the rule's tests ran and passed. +- `failed`: the rule's tests ran and something failed. +- `not-run`: the rule failed before its tests ran, because `validate` rejected it or its fixtures could not be read, and the execution policy did not refuse it. +- `refused`: the execution policy declined to run the rule's tests. + +`ok`, `ran` and `refused` SHALL remain on the result and SHALL be derived from `status`: `ok` is true exactly when `status` is `passed`, `ran` is true exactly when `status` is `passed` or `failed`, and `refused` is present exactly when `status` is `refused`. The CLI's own readers of a verification (the human marks, the summary counts, the exit code and the report draft) SHALL read `status` rather than combining the other three. + +`validate` runs no tests and SHALL NOT report `status`. + +**Rationale.** Three fields for four outcomes let every consumer invent its own precedence, and let a consumer that reads `ok` alone report a rule that never ran as passing. One field computed by the producer removes the choice. Deriving the old fields from it, rather than setting them alongside it, keeps them consistent for the consumers that already read them. + +#### Scenario: A passing rule reports passed + +- **WHEN** `verify --json` runs a rule whose tests run and pass +- **THEN** the rule's `status` SHALL be `passed` +- **AND** its `ok` SHALL be `true` and its `ran` SHALL be `true` + +#### Scenario: A failing rule reports failed + +- **WHEN** `verify --json` runs a rule whose tests run and a case fails +- **THEN** the rule's `status` SHALL be `failed` +- **AND** its `ok` SHALL be `false` and its `ran` SHALL be `true` + +#### Scenario: A rule validate rejected reports not-run + +- **WHEN** `verify --json` runs a rule that `validate` rejects +- **THEN** the rule's `status` SHALL be `not-run` +- **AND** its `ok` SHALL be `false`, its `ran` SHALL be `false`, and it SHALL carry no `refused` +- **AND** `verify` SHALL exit 1 + +#### Scenario: A refused rule reports refused + +- **WHEN** `verify --json` runs a runtime rule without `--dangerously-run-scripts` +- **THEN** the rule's `status` SHALL be `refused` +- **AND** its `ok` SHALL be `false`, its `ran` SHALL be `false`, and `refused` SHALL carry the reason + +#### Scenario: Validate reports no status + +- **WHEN** `validate --json` reports a rule +- **THEN** the rule's result SHALL NOT carry `status` diff --git a/packages/cli/src/agent/create-sg-rule.md b/packages/cli/src/agent/create-sg-rule.md index 1c6a84d2..2ac23592 100644 --- a/packages/cli/src/agent/create-sg-rule.md +++ b/packages/cli/src/agent/create-sg-rule.md @@ -1,4 +1,4 @@ -# Topic: create-sg-rule (CLI v%(CLI_VERSION)s / topic v12) +# Topic: create-sg-rule (CLI v%(CLI_VERSION)s / topic v13) %(ONE_COMMAND_PER_CALL)s @@ -151,7 +151,7 @@ whole rule. Both answer in the same shape: ```json {"ok":true,"rules":[{"engine":"sg","ruleId":"no-eval", - "ok":true,"errors":[],"ran":true}]} + "status":"passed","ok":true,"errors":[],"ran":true}]} ``` - `ok: true` → go to step 8. - `ok: false` → read `errors` and fix. Repeat up to 3 times. diff --git a/packages/cli/src/agent/create-vale-rule.md b/packages/cli/src/agent/create-vale-rule.md index f86f05c0..0dccfbdc 100644 --- a/packages/cli/src/agent/create-vale-rule.md +++ b/packages/cli/src/agent/create-vale-rule.md @@ -1,4 +1,4 @@ -# Topic: create-vale-rule (CLI v%(CLI_VERSION)s / topic v19) +# Topic: create-vale-rule (CLI v%(CLI_VERSION)s / topic v20) %(ONE_COMMAND_PER_CALL)s @@ -980,7 +980,7 @@ it. ```json {"ok":true,"rules":[{"engine":"vale","ruleId":"no-simply", - "ok":true,"errors":[],"ran":true}]} + "status":"passed","ok":true,"errors":[],"ran":true}]} ``` `ok` is the answer. `errors` names what failed, one string per diff --git a/packages/cli/src/agent/validate-rule.md b/packages/cli/src/agent/validate-rule.md index 58d62956..fe4b8bfa 100644 --- a/packages/cli/src/agent/validate-rule.md +++ b/packages/cli/src/agent/validate-rule.md @@ -1,4 +1,4 @@ -# Topic: validate-rule (CLI v%(CLI_VERSION)s / topic v9) +# Topic: validate-rule (CLI v%(CLI_VERSION)s / topic v10) %(ONE_COMMAND_PER_CALL)s @@ -53,8 +53,8 @@ A runtime rule's fixtures execute its `check.ts`, so they run only when you pass `--dangerously-run-scripts`. Nothing else runs them: `verify` makes no network request and does not reconcile, so a signature the service blessed does not count here. Without the flag, that rule prints `○` rather than a tick, names the reason -and the flag, and is reported as `"ran": false` with a `refused` string -saying why. It is counted under "did not run" rather than among the +and the flag, and is reported as `"status": "refused"` with a `refused` +string saying why. It is counted under "did not run" rather than among the rules verified. `verify` exits 0 only when every rule you asked for was verified: @@ -65,8 +65,8 @@ rules verified. | `1` | at least one rule failed | `false` | | `2` | nothing failed, but at least one rule did not run | `false` | -Exit `2` is not a broken rule. Read `ran` on each rule to see which ones -did not run, and rerun them with the flag if you mean to execute them. +Exit `2` is not a broken rule. Read `status` on each rule to see which +ones did not run, and rerun them with the flag if you mean to execute them. A runtime rule that did not run is not a rule that passed, and treating the two alike is the defect this reporting exists to prevent. @@ -102,10 +102,13 @@ Both commands answer in the same shape, one entry per rule: - `ok` at the top is true only when every rule passed. - `errors` is one string per problem, written to be read by a person. -- `verify` adds `ran`, which is false when `validate` failed and the tests - never executed, and false for a runtime rule the execution policy - refused. The second case also carries `refused` with the reason, and - is the one where `ok` alone would mislead you. +- `verify` adds `status`, the field to branch on. It is one of + `passed` (the tests ran and passed), `failed` (they ran and something + failed), `not-run` (`validate` failed, so the tests never executed), + or `refused` (the execution policy declined to run them, with the + reason in `refused`). `validate` runs no tests and reports no + `status`. The older `ran` and `refused` fields are still there and + agree with it, but `ok` alone would mislead you on a refused rule. - `notice` is something true about the rule that is not a failure, and it is reported on a pass as well. For a Vale rule it carries what Vale wrote to stderr on a run that still exited zero, such as the @@ -113,7 +116,7 @@ Both commands answer in the same shape, one entry per rule: validates, Vale ignores it, and only this field says so. For an `sg` rule it names the silent traps from `create-sg-rule`'s Quirks section that the rule's shape matches, such as `$$$` beside a comma. Read it - before `ok`, the same as `ran`. + before `ok`, the same as `status`. A green Vale `verify` also assumes Vale read every fixture. A fixture whose front matter Vale cannot parse is excluded from the run and diff --git a/packages/cli/src/commands/verify.ts b/packages/cli/src/commands/verify.ts index 4bfe846e..9f906c0e 100644 --- a/packages/cli/src/commands/verify.ts +++ b/packages/cli/src/commands/verify.ts @@ -74,6 +74,22 @@ function printFindings(findings: FixtureFinding[]): void { } } +/** + * How a result counts toward the marks, the summary and the exit code. + * + * `verify` results carry `status`, which is read as given apart from one fold: + * a rule that failed before its tests ran (`not-run`) counts as a failure, + * because something about it is broken and the errors say what. Only a refusal + * is held apart. `validate` runs no tests and has no `status`, so `ok` is its + * whole answer. + */ +function verdictOf( + result: RuleValidation | RuleVerification +): "passed" | "failed" | "refused" { + if (!("status" in result)) return result.ok ? "passed" : "failed"; + return result.status === "not-run" ? "failed" : result.status; +} + /** * The shared body of `validate` and `verify`. * @@ -189,10 +205,8 @@ async function runOverPath(options: { // its own marker below, and has its own exit code, because an exit code is all // a CI step or an agent reads, and a rule nothing exercised must not read the // same as a rule that passed. - const isRefused = (result: RuleValidation | RuleVerification): boolean => - "refused" in result && result.refused !== undefined; - const refused = results.filter((result) => isRefused(result)); - const failed = results.filter((result) => !result.ok && !isRefused(result)); + const refused = results.filter((result) => verdictOf(result) === "refused"); + const failed = results.filter((result) => verdictOf(result) === "failed"); const judged = results.length - refused.length; if (json) { @@ -211,7 +225,9 @@ async function runOverPath(options: { // `○` is neither tick nor cross on purpose: the rule was not verified, and // a reader scanning the column has to be able to see that at a glance. // The reason prints below it, from `errors`, and names what would run it. - const mark = isRefused(result) ? "○" : result.ok ? "✓" : "✗"; + const verdict = verdictOf(result); + const mark = + verdict === "refused" ? "○" : verdict === "passed" ? "✓" : "✗"; console.log(`${mark} ${result.engine}/${result.ruleId}`); for (const error of result.errors) { console.log(` ${error}`); @@ -233,7 +249,7 @@ async function runOverPath(options: { // 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)) { + if (verdict === "failed") { printFindings(findingsOf(result)); } } diff --git a/packages/cli/src/report/draft.ts b/packages/cli/src/report/draft.ts index b2abd6ef..8e746d23 100644 --- a/packages/cli/src/report/draft.ts +++ b/packages/cli/src/report/draft.ts @@ -138,9 +138,22 @@ async function testStatus( engine: "sg" | "vale", id: string ): Promise { - const verification = await verifyOneRule(cwd, { engine, ruleId: id }); - if (verification.refused !== undefined) return "refused"; - return verification.ok && verification.ran ? "pass" : "fail"; + const { status } = await verifyOneRule(cwd, { engine, ruleId: id }); + switch (status) { + case "passed": { + return "pass"; + } + case "refused": { + return "refused"; + } + // A rule that failed before its tests ran is reported as failing, not as + // `not-run`: the report's `not-run` means nothing tried to test it, and + // here something did and found it broken. + case "failed": + case "not-run": { + return "fail"; + } + } } /** Every rule under `.taskless/rules/`, with its tests run. */ diff --git a/packages/cli/src/rules/inspect.ts b/packages/cli/src/rules/inspect.ts index 18e35ca7..083489c9 100644 --- a/packages/cli/src/rules/inspect.ts +++ b/packages/cli/src/rules/inspect.ts @@ -71,10 +71,32 @@ export interface RuleValidation { notices: string[]; } +/** + * The four outcomes of verifying one rule, as one value. + * + * - `passed`: the tests ran and passed. + * - `failed`: the tests ran and something failed. + * - `not-run`: the rule failed before its tests ran (`validate` rejected it, + * or its fixtures could not be read), and nothing refused it. + * - `refused`: the execution policy declined to run its tests. + */ +export type VerifyStatus = "passed" | "failed" | "not-run" | "refused"; + /** What `verify` concluded about one rule. */ export interface RuleVerification { engine: EngineName; ruleId: string; + /** + * The outcome, and the field to branch on. + * + * `ok`, `ran` and `refused` are all derived from it by + * {@link verification}, so no producer can set them inconsistently. They + * spread the four outcomes over three fields, which let a consumer read `ok` + * alone and report a rule that never ran as passing; each consumer had also + * grown its own precedence for combining them. + */ + status: VerifyStatus; + /** Derived from `status`: true exactly when it is `passed`. */ ok: boolean; errors: string[]; /** @@ -93,7 +115,8 @@ export interface RuleVerification { */ violations: RuleViolation[]; /** - * Whether the rule's tests actually ran. + * Whether the rule's tests actually ran. Derived from `status`: true exactly + * when it is `passed` or `failed`. * * Load-bearing rather than advisory. `verify` used to report a runtime rule as * `ok: true, ran: false` and print a tick, so the one field that knew the @@ -102,14 +125,15 @@ export interface RuleVerification { ran: boolean; /** * Set when the execution policy refused the run, carrying the reason. + * Present exactly when `status` is `refused`. * - * The third outcome, and the one `ok` cannot express. A rule that cannot run - * because nothing blessed it is not a pass, and it is not a failure either: - * failing it would turn `verify` red for every project holding a runtime rule, - * with no action available to its holder that makes it green. So it is - * neither, and the renderer, the summary count and the exit code all read - * this field rather than inferring the state from `ok` and `ran` together - * (which cannot distinguish it from a rule whose `validate` failed). + * The outcome `ok` cannot express. A rule that cannot run because nothing + * blessed it is not a pass, and it is not a failure either: failing it would + * turn `verify` red for every project holding a runtime rule, with no action + * available to its holder that makes it green. So it is neither, and the + * renderer, the summary count and the exit code all read `status` rather + * than inferring the state from `ok` and `ran` together (which cannot + * distinguish it from a rule whose `validate` failed). */ refused?: string; /** @@ -174,6 +198,52 @@ export interface VerifyOptions { onRuntimeWarning?: (message: string) => void; } +/** The fields of a {@link RuleVerification} that `status` does not decide. */ +type VerificationBody = Omit< + RuleVerification, + "status" | "ok" | "ran" | "refused" +>; + +/** An outcome, with the reason when it is a refusal. */ +type VerificationOutcome = + | { status: "passed" | "failed" | "not-run" } + | { status: "refused"; reason: string }; + +/** + * Build a {@link RuleVerification}, deriving `ok`, `ran` and `refused` from its + * outcome. + * + * The only way this module builds one, so the legacy fields cannot disagree + * with `status`: `ok: true` with `ran: false`, the false pass that once + * shipped, has no spelling here. + */ +export function verification( + outcome: VerificationOutcome, + body: VerificationBody +): RuleVerification { + const { status } = outcome; + return { + engine: body.engine, + ruleId: body.ruleId, + status, + ok: status === "passed", + errors: body.errors, + violations: body.violations, + ran: status === "passed" || status === "failed", + ...(outcome.status === "refused" ? { refused: outcome.reason } : {}), + notices: body.notices, + findings: body.findings, + cases: body.cases, + }; +} + +/** Whether the tests ran and passed, as a `passed` or `failed` outcome. */ +function judged(passed: boolean): VerificationOutcome { + return { status: passed ? "passed" : "failed" }; +} + +const NOT_RUN: VerificationOutcome = { status: "not-run" }; + async function readYaml(path: string): Promise { return parse(await readFile(path, "utf8")) as unknown; } @@ -453,22 +523,20 @@ export async function verifyOneRule( // call, which would run every case twice for one answer. const validation = await withIdCollision(cwd, ruleId, verdict); if (!validation.ok) { - return { ...validation, ran: false, findings: [], cases: [] }; + return verification(NOT_RUN, { ...validation, findings: [], cases: [] }); } // A rule that populated only one bucket has proved only half of what a // rule claims, and the CLI says so: coverage is counted over case // directories, never read from an engine's summary. const shortfall = describeCoverageShortfall(ruleId, result.tests.fixtures); - return { + return verification(judged(result.tests.valid), { engine, ruleId, - ok: result.tests.valid, errors: [ ...(shortfall === undefined ? [] : [shortfall]), ...result.tests.errors, ], violations: [], - ran: true, // The cases now carry paths, so a `fail/` case shows the rule's globs // reach the fixture. Whether they reach any file in the PROJECT is a // separate question, asked here on a rule that validated, and only ever @@ -481,44 +549,40 @@ export async function verifyOneRule( // rendered messages, and `file` naming the fixture in its case. findings: result.tests.findings, cases: result.tests.cases, - }; + }); } const validation = await validateOneRule(cwd, rule); if (!validation.ok) { - return { ...validation, ran: false, findings: [], cases: [] }; + return verification(NOT_RUN, { ...validation, findings: [], cases: [] }); } if (engine === "vale") { const result = await verifyValeRule(cwd, ruleId); if ("outcome" in result) { - return { + return verification(NOT_RUN, { engine, ruleId, - ok: false, errors: [result.outcome.message], violations: [], - ran: false, notices: [], findings: [], cases: [], - }; + }); } const errors: string[] = []; const shortfall = describeCoverageShortfall(ruleId, result.fixtures); if (shortfall !== undefined) errors.push(shortfall); errors.push(...result.errors); - return { + return verification(judged(result.passed), { engine, ruleId, - ok: result.passed, errors, violations: [], - ran: true, notices: result.notices, findings: result.findings, cases: result.cases, - }; + }); } // Runtime rules execute code, so `verify` runs their fixtures only behind @@ -549,18 +613,18 @@ export async function verifyOneRule( const reason = `fixtures did not run: running them executes the rule's check.ts. ` + `Pass --dangerously-run-scripts to run them.`; - return { - engine, - ruleId, - ok: false, - errors: [reason], - violations: [], - ran: false, - findings: [], - cases: [], - refused: reason, - notices: [], - }; + return verification( + { status: "refused", reason }, + { + engine, + ruleId, + errors: [reason], + violations: [], + findings: [], + cases: [], + notices: [], + } + ); } // Discovery, not a plan: the bytes are the working tree's, which is the point @@ -575,18 +639,18 @@ export async function verifyOneRule( const reason = `fixtures did not run: it was not discovered as a runnable runtime ` + `rule (no capture rules under captures/).`; - return { - engine, - ruleId, - ok: false, - errors: [reason], - violations: [], - ran: false, - findings: [], - cases: [], - refused: reason, - notices: [], - }; + return verification( + { status: "refused", reason }, + { + engine, + ruleId, + errors: [reason], + violations: [], + findings: [], + cases: [], + notices: [], + } + ); } let fixtures; @@ -597,17 +661,15 @@ export async function verifyOneRule( // Both are failures of the fixtures rather than refusals of the run, so // they fail: the alternative is a bucket reading as empty, which makes a // two-sided rule look one-sided and a one-sided rule look complete. - return { + return verification(NOT_RUN, { engine, ruleId, - ok: false, errors: [error instanceof Error ? error.message : String(error)], violations: [], - ran: false, notices: [], findings: [], cases: [], - }; + }); } // Warned at the last possible moment: everything above can still decline to @@ -615,15 +677,13 @@ export async function verifyOneRule( options.onRuntimeWarning?.(RUN_SCRIPTS_WARNING); const report = await runRuntimeFixtures(cwd, runtimeRule, fixtures); - return { + return verification(judged(report.passed), { engine, ruleId, - ok: report.passed, errors: describeFixtureReport(ruleId, report), violations: [], - ran: true, notices: [], findings: report.findings, cases: report.cases, - }; + }); } diff --git a/packages/cli/src/schemas/rule-check.ts b/packages/cli/src/schemas/rule-check.ts index e63dc34a..c7b1b1bf 100644 --- a/packages/cli/src/schemas/rule-check.ts +++ b/packages/cli/src/schemas/rule-check.ts @@ -94,7 +94,17 @@ export const fixtureCaseSchema = z.object({ const ruleResultSchema = z.object({ engine: z.enum(["sg", "vale", "runtime"]), ruleId: z.string(), - ok: z.boolean(), + status: z + .enum(["passed", "failed", "not-run", "refused"]) + .optional() + .describe( + "`verify` only, absent from `validate`, which runs no tests. The rule's outcome, and the field to branch on: `passed` (its tests ran and passed), `failed` (its tests ran and something failed), `not-run` (it failed before its tests ran, for example because `validate` rejected it), or `refused` (the execution policy declined to run its tests). `ok`, `ran` and `refused` are derived from it and kept for existing consumers" + ), + ok: z + .boolean() + .describe( + "Whether the rule passed. Under `verify`, derived from `status`: true exactly when it is `passed`" + ), errors: z.array(z.string()).describe("Human-readable failure messages"), violations: z .array( @@ -118,13 +128,13 @@ const ruleResultSchema = z.object({ .boolean() .optional() .describe( - "`verify` only: whether the rule's tests actually ran. Branch on this, not on `ok` alone: `ok: true` with `ran: false` is not a rule that passed" + "`verify` only: whether the rule's tests actually ran. Derived from `status`: true exactly when it is `passed` or `failed`. Prefer `status`; if you read this instead, never read `ok` alone: `ok: true` with `ran: false` is not a rule that passed" ), refused: z .string() .optional() .describe( - "`verify` only: the execution policy declined to run the rule's fixtures, and why. Neither a pass nor a failure, and excluded from the rules verified. Sets top-level `ok` to false and, when no rule failed, makes the command exit 2" + "`verify` only: the execution policy declined to run the rule's fixtures, and why. Present exactly when `status` is `refused`. Neither a pass nor a failure, and excluded from the rules verified. Sets top-level `ok` to false and, when no rule failed, makes the command exit 2" ), notices: z .array(z.string()) diff --git a/packages/cli/test/runtime-fixture-runner.test.ts b/packages/cli/test/runtime-fixture-runner.test.ts index e318305b..ede24d6e 100644 --- a/packages/cli/test/runtime-fixture-runner.test.ts +++ b/packages/cli/test/runtime-fixture-runner.test.ts @@ -46,6 +46,7 @@ interface Report { ruleId: string; ok: boolean; errors: string[]; + status?: "passed" | "failed" | "not-run" | "refused"; ran?: boolean; refused?: string; cases: { @@ -248,6 +249,7 @@ describe("a runtime rule whose fixtures did not run", () => { const rule = report.rules[0]; expect(rule?.engine).toBe("runtime"); + expect(rule?.status).toBe("refused"); expect(rule?.ran).toBe(false); expect(rule?.ok).not.toBe(true); // The envelope agrees with the exit code. #612: this read `ok: true` over @@ -317,6 +319,7 @@ describe("fixture directions", () => { const report = await testRuleJson("--dangerously-run-scripts"); const rule = report.rules[0]; + expect(rule?.status).toBe("failed"); expect(rule?.ran).toBe(true); expect(rule?.ok).toBe(false); expect(rule?.errors.join("\n")).toContain("fail/uses-eval"); diff --git a/packages/cli/test/validate-verify-commands.test.ts b/packages/cli/test/validate-verify-commands.test.ts index e3c350e8..bcfa8420 100644 --- a/packages/cli/test/validate-verify-commands.test.ts +++ b/packages/cli/test/validate-verify-commands.test.ts @@ -33,6 +33,7 @@ interface Report { rules: { engine: string; ruleId: string; + status?: "passed" | "failed" | "not-run" | "refused"; ok: boolean; errors: string[]; violations: { constraintId: string; message: string }[]; @@ -426,6 +427,7 @@ describe("verify reports every sg case", () => { const result = await runCli(["verify", "-d", cwd, "--json"]); expect(result.exitCode).not.toBe(0); const rule = (JSON.parse(result.stdout) as Report).rules[0]; + expect(rule?.status).toBe("failed"); const path = ".taskless/rules/sg/no-eval/tests/fail/eval-call"; expect(rule?.cases).toEqual([ { @@ -444,6 +446,9 @@ describe("verify reports every sg case", () => { await sgRule("no-eval", "no-eval"); const result = await runCli(["verify", "-d", cwd, "--json"]); expect(result.exitCode).toBe(0); + expect((JSON.parse(result.stdout) as Report).rules[0]?.status).toBe( + "passed" + ); expect((JSON.parse(result.stdout) as Report).rules[0]?.cases).toEqual([ { name: "fail/eval-call", expected: "fail", ok: true }, { name: "pass/plain", expected: "pass", ok: true }, @@ -455,6 +460,30 @@ describe("verify reports every sg case", () => { const result = await runCli(["validate", "-d", cwd, "--json"]); expect((JSON.parse(result.stdout) as Report).rules[0]?.cases).toEqual([]); }); + + it("reports no status from validate, which runs no tests", async () => { + await sgRule("no-eval", "no-eval"); + const result = await runCli(["validate", "-d", cwd, "--json"]); + const rule = (JSON.parse(result.stdout) as Report).rules[0]; + expect(rule?.ok).toBe(true); + expect(rule).not.toHaveProperty("status"); + }); + + it("reports not-run for a rule validate rejected", async () => { + // Directory and `id:` disagree, which `validate` refuses, so `verify` + // stops before its cases run. + await sgRule("no-eval", "something-else"); + const result = await runCli(["verify", "-d", cwd, "--json"]); + expect(result.exitCode).toBe(1); + const rule = ( + JSON.parse(result.stdout) as { + rules: { status?: string; ok: boolean; ran?: boolean }[]; + } + ).rules[0]; + expect(rule?.status).toBe("not-run"); + expect(rule?.ok).toBe(false); + expect(rule?.ran).toBe(false); + }); }); withVale("verify reports every Vale case", () => { @@ -635,11 +664,17 @@ withVale("a rule Vale would choke on never reaches Vale", () => { expect(result.exitCode).not.toBe(0); const rule = ( JSON.parse(result.stdout) as { - rules: { ok: boolean; ran: boolean; errors: string[] }[]; + rules: { + status?: string; + ok: boolean; + ran: boolean; + errors: string[]; + }[]; } ).rules[0]; // `ran: false` is the assertion that matters: the fixtures were never // linted, so Vale was never asked to load this config. + expect(rule?.status).toBe("not-run"); expect(rule?.ran).toBe(false); expect(rule?.errors.join(" ")).toContain("occurrence"); // And the fixture complaint does not crowd out the real one. diff --git a/packages/cli/test/verify-status.test.ts b/packages/cli/test/verify-status.test.ts new file mode 100644 index 00000000..05122d48 --- /dev/null +++ b/packages/cli/test/verify-status.test.ts @@ -0,0 +1,60 @@ +import { describe, expect, it } from "vitest"; + +import { verification, type VerifyStatus } from "../src/rules/inspect"; +import { outputSchema } from "../src/schemas/rule-check"; + +const BODY = { + engine: "sg" as const, + ruleId: "no-eval", + errors: [], + violations: [], + notices: [], + findings: [], + cases: [], +}; + +/** + * `status` is the field to branch on, and `ok`, `ran` and `refused` are kept + * for consumers that read them. They are derived from `status`, so for every + * outcome they must say the same thing it does. The combination this exists to + * rule out is `ok: true` with `ran: false`, a rule that never ran reading as a + * pass to anything that checks `ok` alone. + */ +describe("a verification's legacy fields agree with its status", () => { + const expected: Record< + VerifyStatus, + { ok: boolean; ran: boolean; refused: string | undefined } + > = { + passed: { ok: true, ran: true, refused: undefined }, + failed: { ok: false, ran: true, refused: undefined }, + "not-run": { ok: false, ran: false, refused: undefined }, + refused: { ok: false, ran: false, refused: "not blessed" }, + }; + + for (const [status, fields] of Object.entries(expected) as [ + VerifyStatus, + (typeof expected)[VerifyStatus], + ][]) { + it(`derives ok, ran and refused for ${status}`, () => { + const result = verification( + status === "refused" ? { status, reason: "not blessed" } : { status }, + BODY + ); + expect(result.status).toBe(status); + expect(result.ok).toBe(fields.ok); + expect(result.ran).toBe(fields.ran); + expect(result.refused).toBe(fields.refused); + // Absent rather than `undefined`, so `--json` carries no stray key. + expect("refused" in result).toBe(status === "refused"); + }); + + it(`keeps status through the --json schema for ${status}`, () => { + const result = verification( + status === "refused" ? { status, reason: "not blessed" } : { status }, + BODY + ); + const parsed = outputSchema.parse({ ok: false, rules: [result] }); + expect(parsed.rules[0]?.status).toBe(status); + }); + } +});