diff --git a/.changeset/notice-list-contract.md b/.changeset/notice-list-contract.md new file mode 100644 index 00000000..e34c64d1 --- /dev/null +++ b/.changeset/notice-list-contract.md @@ -0,0 +1,11 @@ +--- +"@taskless/cli": patch +--- + +`taskless check` now marks every notice it prints. A run with more than one advisory used to print the first behind a `Notice: ` marker and the rest as bare, unindented lines with nothing identifying them as notices — so a Vale config advisory sitting beside Vale's own diagnostic read as stray output. `verify` had the same defect and it was fixed earlier; `check` did not get the fix until now. + +A second, related gap in the same output: the notices from runtime rule planning — what was repaired, what could not be, and why — were printed by their own loop with no `Notice: ` marker at all, while engine notices were marked. `check --json` merges both into one `notices` array, so the same message looked like two different kinds of thing depending on which list it arrived on. Every notice `check` prints is now marked, and every line of one is. + +The cause was that notices were joined into one string before they reached the renderer, so `check --json` also published array elements that were several notices glued together, with no separator a consumer could rely on to split them back apart. Notices are now carried as a list from producer to output: in `check --json` the `notices` array keeps its name and type, and only its element boundaries change — one element is now exactly one notice. + +**What a consumer crosses:** the optional `notice` string on `verify --json` and `test --json` per-rule results is now a `notices` array of strings, present and empty rather than absent when there is nothing to say. The same replacement applies to the exported `verifyOutputSchema` (its `schema` layer) and `valeVerifyOutputSchema`. Read `notices` where you read `notice`, and render one marker per element instead of splitting on a separator. It was replaced rather than mirrored because a joined `notice` kept alongside would preserve the convention this change removes, and nothing ever published the separator that would have made splitting it safe. `check --json` consumers need change nothing. diff --git a/openspec/changes/archive/2026-09-23-notice-list-contract/.openspec.yaml b/openspec/changes/archive/2026-09-23-notice-list-contract/.openspec.yaml new file mode 100644 index 00000000..265da3d9 --- /dev/null +++ b/openspec/changes/archive/2026-09-23-notice-list-contract/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-09-23 diff --git a/openspec/changes/archive/2026-09-23-notice-list-contract/proposal.md b/openspec/changes/archive/2026-09-23-notice-list-contract/proposal.md new file mode 100644 index 00000000..13e77b28 --- /dev/null +++ b/openspec/changes/archive/2026-09-23-notice-list-contract/proposal.md @@ -0,0 +1,79 @@ +## Why + +taskless/cli#390 asked for four duplicated notice joiners to become one helper. +The duplication was never the problem. The problem is that "one notice per +line" was an agreement between four producers and two renderers that existed +only as four string literals nobody was obliged to keep identical — and one of +them already did not, joining with a space. + +Nine spec files mention "notice", every one of them about **whether** a notice +surfaces. Nothing standing says how several notices are separated, how they are +rendered, or what a machine consumer receives. So a producer could switch +separator, or a renderer stop prefixing, and no requirement would be violated. + +That is not hypothetical. `check` shipped the defect: it printed `Notice: ` +once per element while producers glued several advisories into one element, so +a run with two advisories printed the first behind a marker and the second as +an unlabelled stray line. `verify` had the same bug and it was fixed in +`241e1c4`; nothing recorded the fix as a requirement, so `check` kept it. + +The behaviour is now a list — one notice per element, all the way from the +producers to the published envelope — and these deltas say so, in the three +capabilities that own the producers and the renderers. + +## What Changes + +- **`cli-check`** — a new requirement: `check` renders one marker per notice + and prefixes every line of one, and `--json` publishes `notices` as a flat + list in which one element is one notice. +- **`cli-rule-validation`** — a new requirement: `verify` and `test` carry + notices as a list, render one `notice:` marker per element, and publish + `notices` in `--json`, replacing the joined `notice` string. +- **`cli-vale-rule-engine`** — a new requirement: when the Vale engine has + several independent things to say about one run, each is a distinct notice + rather than being folded into one. + +All three are ADDED. Nothing standing describes this, so there is no +requirement to restate, and a MODIFIED block would risk dropping scenarios from +requirements that are about a different question entirely. + +## Capabilities + +### New Capabilities + +None. Three existing capabilities gain a requirement each. + +### Modified Capabilities + +- `cli-check`: gains "Notices render one marker per notice and publish as a + flat list". +- `cli-rule-validation`: gains "Verify and test carry notices as a list". +- `cli-vale-rule-engine`: gains "Independent Vale advisories stay separate + notices". + +## Impact + +The published `notice?: string` field becomes `notices: string[]` in +`verifyOutputSchema.schema`, `valeVerifyOutputSchema`, and the `verify`/`test` +envelope. It was replaced rather than mirrored: a joined `notice` kept beside +the list would preserve the separator convention this change exists to remove, +and a consumer could not safely split it apart in the first place, since +nothing published the separator. `check --json`'s `notices` keeps its name and +its `string[]` type; only its element boundaries change. + +The bump is `patch`. The package is `0.11.2`, pre-1.0, where semver puts the +public API outside the stability guarantee — the changeset body says what a +consumer crosses. + +## Delivery shape + +**Single PR.** The helper, the structural change, the renderer fix, the tests +and these deltas are one reviewable diff, and splitting them would land a spec +describing behaviour that is not yet there, or a renderer fix without the +requirement that keeps it fixed. + +This PR is the tip — no open PR is based on this branch — so the change is +archived here, on this PR, rather than on landing. `openspec-label.yml` reports +an unarchived change directory, and `main` takes pull requests only, so a change +that reaches `main` unarchived needs a second PR to do what the tip should have +done. diff --git a/openspec/changes/archive/2026-09-23-notice-list-contract/specs/cli-check/spec.md b/openspec/changes/archive/2026-09-23-notice-list-contract/specs/cli-check/spec.md new file mode 100644 index 00000000..8e9693ea --- /dev/null +++ b/openspec/changes/archive/2026-09-23-notice-list-contract/specs/cli-check/spec.md @@ -0,0 +1,39 @@ +## ADDED Requirements + +### Requirement: Notices render one marker per notice and publish as a flat list + +`check` SHALL carry advisory messages as an ordered list in which one element is one notice, and SHALL NOT join several notices into one element. + +In human output the CLI SHALL prefix EVERY line it prints of a notice with the `Notice: ` marker, including the second and later lines of a notice that spans lines. A notice may legitimately span lines — an engine's own diagnostic output is passed through as written — so a marker on the first line alone leaves the rest reading as unlabelled stray output, which is the failure this requires against. + +Every notice the run reports SHALL be marked alike, whichever stage produced it — runtime planning or engine dispatch. `--json` merges both into one `notices` array, so text output that marked one source and not the other made the same message look like two different kinds of thing depending on which list it arrived on. + +Under `--json` the `notices` array SHALL be flat: each element SHALL be exactly one notice. A consumer SHALL NOT be required to split an element on a separator, because no separator between notices is published and none is part of the contract. + +A notice SHALL NOT affect the exit code. + +#### Scenario: Two independent advisories each get their own marker + +- **WHEN** a `check` run produces two independent notices and human output is rendered +- **THEN** each notice SHALL be printed on its own line behind its own `Notice: ` marker + +#### Scenario: Every line of a multi-line notice is marked + +- **WHEN** a single notice spans several lines and human output is rendered +- **THEN** the CLI SHALL print each of its lines behind a `Notice: ` marker + +#### Scenario: A runtime plan notice is marked like an engine notice + +- **WHEN** runtime planning reports a notice and human output is rendered +- **THEN** the CLI SHALL print it behind the same `Notice: ` marker it gives an engine's notice + +#### Scenario: A machine consumer receives one notice per element + +- **WHEN** a `check --json` run produces two independent notices +- **THEN** the `notices` array SHALL hold two elements, one notice each + +#### Scenario: Nothing to say publishes nothing + +- **WHEN** a `check` run produces no notices +- **THEN** human output SHALL print no `Notice: ` line +- **AND** `--json` SHALL omit the `notices` field diff --git a/openspec/changes/archive/2026-09-23-notice-list-contract/specs/cli-rule-validation/spec.md b/openspec/changes/archive/2026-09-23-notice-list-contract/specs/cli-rule-validation/spec.md new file mode 100644 index 00000000..bb808b22 --- /dev/null +++ b/openspec/changes/archive/2026-09-23-notice-list-contract/specs/cli-rule-validation/spec.md @@ -0,0 +1,25 @@ +## ADDED Requirements + +### Requirement: Verify and test carry notices as a list + +`verify` and `test` SHALL carry what they have to say about a rule without failing it as an ordered list in which one element is one notice, and SHALL NOT join several notices into one element with any separator. + +In human output the CLI SHALL print one ` notice:` marker per notice, and SHALL prefix every line of a notice that spans lines. Under `--json` each per-rule result SHALL carry a `notices` array of strings, present and empty when there is nothing to say rather than absent, so a consumer can tell "nothing to report" from "this CLI does not report notices" — the same distinction `violations` beside it already draws. + +A notice SHALL be reported on a rule that passed as well as on one that failed, and SHALL NOT affect the exit code. + +#### Scenario: Two advisories about one rule are two notices + +- **WHEN** one rule draws both a style-layer advisory and a config-layer advisory +- **THEN** `verify --json` SHALL report them as two elements of that rule's `notices` +- **AND** human output SHALL print each behind its own `notice:` marker + +#### Scenario: Two language advisories on one sg rule stay separate + +- **WHEN** an sg rule declares an accepted-but-off-list `language:` spelling AND a `files:` glob that language cannot parse +- **THEN** `verify` SHALL report the two as separate notices rather than as one joined line + +#### Scenario: A rule with nothing to report carries an empty list + +- **WHEN** `verify --json` reports a rule that drew no advisory +- **THEN** that rule's `notices` SHALL be present and empty diff --git a/openspec/changes/archive/2026-09-23-notice-list-contract/specs/cli-vale-rule-engine/spec.md b/openspec/changes/archive/2026-09-23-notice-list-contract/specs/cli-vale-rule-engine/spec.md new file mode 100644 index 00000000..ac55d8e9 --- /dev/null +++ b/openspec/changes/archive/2026-09-23-notice-list-contract/specs/cli-vale-rule-engine/spec.md @@ -0,0 +1,24 @@ +## ADDED Requirements + +### Requirement: Independent Vale advisories stay separate notices + +When a single Vale run has more than one thing to say without failing, the engine SHALL carry each as its own notice rather than folding them into one. + +The two arise independently and a project can perfectly well draw both at once: the converter-skip notice naming files this build cannot parse, and Vale's own stderr from a run that still exited zero. The config schema's advisories about the assembled rules are a third, and they ride beside whatever Vale itself reported. Folding them into one message made the second and later ones render without a marker of their own, which reads as stray output rather than as something the run is telling the author. + +The engine SHALL NOT choose a separator between notices: presentation belongs to the renderer, which marks each notice and each of its lines. + +#### Scenario: A skip notice and a zero-exit diagnostic are two notices + +- **WHEN** one Vale run both declines a converter-dependent file and writes a diagnostic to stderr while exiting zero +- **THEN** the engine SHALL report two notices, one for each + +#### Scenario: A config advisory rides beside Vale's own report + +- **WHEN** the config schema advises on an assembled rule and Vale then reports a diagnostic of its own +- **THEN** the two SHALL reach the check result as separate notices + +#### Scenario: A config advisory survives a Vale that could not run + +- **WHEN** the config schema advises on an assembled rule and the Vale binary is unavailable +- **THEN** the advisory and the unavailability message SHALL both reach the result, as separate notices diff --git a/openspec/changes/archive/2026-09-23-notice-list-contract/tasks.md b/openspec/changes/archive/2026-09-23-notice-list-contract/tasks.md new file mode 100644 index 00000000..ba5fee1c --- /dev/null +++ b/openspec/changes/archive/2026-09-23-notice-list-contract/tasks.md @@ -0,0 +1,49 @@ +## 1. Implementation + +- [x] 1.1 Add `packages/cli/src/util/notices.ts` as a leaf module with no + imports of its own, exporting `collectNotices`, which drops `undefined` + and `""` and preserves order. +- [x] 1.2 Cut the unreachable `?? outcome.message` fallback in the Vale + dispatch `unavailable` branch. +- [x] 1.3 Carry notices as `string[]` through `EngineOutcome`, + `ValeAttempt`/`ValeRunOutcome`, `ValeVerifyResult`, `SchemaLayerResult`, + `RuleVerification` and `RuleTestResult`, so `DispatchResult.notices` is + genuinely flat. +- [x] 1.4 Remove the space join in `rules/verify.ts`, so `language.notices` + contributes elements like every other producer and no second separator + convention remains. +- [x] 1.5 Fix `check`'s renderer to prefix every line of every notice, the + defect `241e1c4` fixed in `verify` and left live in `check`. +- [x] 1.6 Replace the published `notice?: string` with `notices: string[]` in + `verifyOutputSchema.schema`, `valeVerifyOutputSchema` and the + `verify`/`test` envelope. +- [x] 1.7 Mark the runtime plan's notices too. They were printed by their own + loop with no marker while the dispatched ones were marked, though + `--json` merges both into one array. +- [x] 1.8 Share one `markNotice` helper between `check` and `verify`, so the + two renderers differ only in the marker. + +## 2. Tests + +- [x] 2.1 Unit-test `collectNotices`: order preserved, `undefined` dropped, + `""` dropped, empty in empty out, multi-line notice left as one element. +- [x] 2.2 Add the missing end-to-end test of `check`'s TEXT output: two + advisories, two `Notice: ` lines. +- [x] 2.3 Assert `check --json` publishes them as separate elements, none + spanning lines. +- [x] 2.4 Unit-test `markNotice` on a multi-line notice, and assert + end-to-end that the runtime plan's warning is marked. +- [x] 2.5 Strengthen the three separator-blind tests to assert on elements + rather than `toContain` over the whole field. + +## 3. Spec + +- [x] 3.1 Add the rendering and list contract to `cli-check`. +- [x] 3.2 Add the list contract to `cli-rule-validation`. +- [x] 3.3 Add the separate-advisories contract to `cli-vale-rule-engine`. +- [x] 3.4 Dry-run `openspec archive` and diff requirement and scenario counts + per capability to prove nothing standing is dropped. +- [x] 3.5 Archive for real on this PR, which is the tip, and check the result + against the dry run's counts. `openspec-label.yml` reports an unarchived + change directory, and `main` takes pull requests only, so a change that + lands unarchived needs a second PR to correct it. diff --git a/openspec/specs/cli-check/spec.md b/openspec/specs/cli-check/spec.md index 38b713f9..f305bc5a 100644 --- a/openspec/specs/cli-check/spec.md +++ b/openspec/specs/cli-check/spec.md @@ -455,3 +455,41 @@ A rule id is the name of a rule's directory under `.taskless/rules//`. T - **WHEN** a user runs `taskless check --rule src/foo.ts` - **THEN** the CLI SHALL treat `src/foo.ts` as the path to scan and `` as the rule filter - **AND** SHALL NOT treat `` as a path + +### Requirement: Notices render one marker per notice and publish as a flat list + +`check` SHALL carry advisory messages as an ordered list in which one element is one notice, and SHALL NOT join several notices into one element. + +In human output the CLI SHALL prefix EVERY line it prints of a notice with the `Notice: ` marker, including the second and later lines of a notice that spans lines. A notice may legitimately span lines — an engine's own diagnostic output is passed through as written — so a marker on the first line alone leaves the rest reading as unlabelled stray output, which is the failure this requires against. + +Every notice the run reports SHALL be marked alike, whichever stage produced it — runtime planning or engine dispatch. `--json` merges both into one `notices` array, so text output that marked one source and not the other made the same message look like two different kinds of thing depending on which list it arrived on. + +Under `--json` the `notices` array SHALL be flat: each element SHALL be exactly one notice. A consumer SHALL NOT be required to split an element on a separator, because no separator between notices is published and none is part of the contract. + +A notice SHALL NOT affect the exit code. + +#### Scenario: Two independent advisories each get their own marker + +- **WHEN** a `check` run produces two independent notices and human output is rendered +- **THEN** each notice SHALL be printed on its own line behind its own `Notice: ` marker + +#### Scenario: Every line of a multi-line notice is marked + +- **WHEN** a single notice spans several lines and human output is rendered +- **THEN** the CLI SHALL print each of its lines behind a `Notice: ` marker + +#### Scenario: A runtime plan notice is marked like an engine notice + +- **WHEN** runtime planning reports a notice and human output is rendered +- **THEN** the CLI SHALL print it behind the same `Notice: ` marker it gives an engine's notice + +#### Scenario: A machine consumer receives one notice per element + +- **WHEN** a `check --json` run produces two independent notices +- **THEN** the `notices` array SHALL hold two elements, one notice each + +#### Scenario: Nothing to say publishes nothing + +- **WHEN** a `check` run produces no notices +- **THEN** human output SHALL print no `Notice: ` line +- **AND** `--json` SHALL omit the `notices` field diff --git a/openspec/specs/cli-rule-validation/spec.md b/openspec/specs/cli-rule-validation/spec.md index f1a7cfad..d6ed8c7a 100644 --- a/openspec/specs/cli-rule-validation/spec.md +++ b/openspec/specs/cli-rule-validation/spec.md @@ -364,3 +364,27 @@ The defect belongs to how `substitution` compiles its keys, not to the regex eng - **WHEN** a Vale upgrade makes a rejected pattern fire - **THEN** the vendor contract test asserting it silent SHALL fail - **AND** the schema's rejection SHALL be removed rather than the test relaxed + +### Requirement: Verify and test carry notices as a list + +`verify` and `test` SHALL carry what they have to say about a rule without failing it as an ordered list in which one element is one notice, and SHALL NOT join several notices into one element with any separator. + +In human output the CLI SHALL print one ` notice:` marker per notice, and SHALL prefix every line of a notice that spans lines. Under `--json` each per-rule result SHALL carry a `notices` array of strings, present and empty when there is nothing to say rather than absent, so a consumer can tell "nothing to report" from "this CLI does not report notices" — the same distinction `violations` beside it already draws. + +A notice SHALL be reported on a rule that passed as well as on one that failed, and SHALL NOT affect the exit code. + +#### Scenario: Two advisories about one rule are two notices + +- **WHEN** one rule draws both a style-layer advisory and a config-layer advisory +- **THEN** `verify --json` SHALL report them as two elements of that rule's `notices` +- **AND** human output SHALL print each behind its own `notice:` marker + +#### Scenario: Two language advisories on one sg rule stay separate + +- **WHEN** an sg rule declares an accepted-but-off-list `language:` spelling AND a `files:` glob that language cannot parse +- **THEN** `verify` SHALL report the two as separate notices rather than as one joined line + +#### Scenario: A rule with nothing to report carries an empty list + +- **WHEN** `verify --json` reports a rule that drew no advisory +- **THEN** that rule's `notices` SHALL be present and empty diff --git a/openspec/specs/cli-vale-rule-engine/spec.md b/openspec/specs/cli-vale-rule-engine/spec.md index 2ad8653a..8a7383a7 100644 --- a/openspec/specs/cli-vale-rule-engine/spec.md +++ b/openspec/specs/cli-vale-rule-engine/spec.md @@ -296,3 +296,26 @@ Assembly SHALL write each accepted config's source verbatim. The parsed structur - **WHEN** a config passes the schema - **THEN** the assembled run config SHALL contain that file's bytes unchanged under the rule's breadcrumb comment - **AND** the matcher patterns reported for the run SHALL equal the section names in the parsed structure + +### Requirement: Independent Vale advisories stay separate notices + +When a single Vale run has more than one thing to say without failing, the engine SHALL carry each as its own notice rather than folding them into one. + +The two arise independently and a project can perfectly well draw both at once: the converter-skip notice naming files this build cannot parse, and Vale's own stderr from a run that still exited zero. The config schema's advisories about the assembled rules are a third, and they ride beside whatever Vale itself reported. Folding them into one message made the second and later ones render without a marker of their own, which reads as stray output rather than as something the run is telling the author. + +The engine SHALL NOT choose a separator between notices: presentation belongs to the renderer, which marks each notice and each of its lines. + +#### Scenario: A skip notice and a zero-exit diagnostic are two notices + +- **WHEN** one Vale run both declines a converter-dependent file and writes a diagnostic to stderr while exiting zero +- **THEN** the engine SHALL report two notices, one for each + +#### Scenario: A config advisory rides beside Vale's own report + +- **WHEN** the config schema advises on an assembled rule and Vale then reports a diagnostic of its own +- **THEN** the two SHALL reach the check result as separate notices + +#### Scenario: A config advisory survives a Vale that could not run + +- **WHEN** the config schema advises on an assembled rule and the Vale binary is unavailable +- **THEN** the advisory and the unavailability message SHALL both reach the result, as separate notices diff --git a/packages/cli/src/commands/check.ts b/packages/cli/src/commands/check.ts index fab1a094..52383c46 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -19,6 +19,7 @@ import { resolveRuleSelection, type RuleSelection } from "../rules/rule-filter"; // because `test` runs a rule's fixtures under exactly this policy. Sharing the // implementation is what makes that a fact rather than an intention. import { planRuntime } from "../rules/runtime/plan"; +import { markNotice } from "../util/notices"; async function pathExists(absolutePath: string): Promise { try { @@ -162,6 +163,28 @@ export const checkCommand = defineCommand({ if (!args.json) console.error(message); }; + /** + * Print one notice, marking EVERY line of it. + * + * One marker per notice, and one per line within a notice that spans + * lines. Both halves matter and both were missing somewhere. A notice can + * be multi-line prose — Vale's stderr is passed through as written, and a + * runtime repair notice embeds an `Error.message` it did not author — so + * marking the first line alone leaves the rest reading as stray output + * rather than as something the run is telling its author. That is the + * defect `241e1c4` fixed for `verify`; `check` had it on the dispatched + * notices, and printed the runtime plan's notices with no marker at all. + * + * Every notice `check` prints in text goes through here, so the three + * sources cannot drift apart again: the `--json` `notices` array mixes + * them, and text output that marked some and not others made the same + * message look like two different kinds of thing depending on which list + * it arrived on. + */ + const warnNotice = (notice: string) => { + for (const line of markNotice(notice, "Notice: ")) warn(line); + }; + // Set when a scan actually runs; drives cli_check_completed with counts // only (never matched code). let scanCounts: @@ -319,10 +342,10 @@ export const checkCommand = defineCommand({ anonymous: args.anonymous, dangerouslyRunScripts: Boolean(args["dangerously-run-scripts"]), }); - for (const notice of plan.notices) warn(notice); + for (const notice of plan.notices) warnNotice(notice); for (const skipped of plan.skipped) { - warn( - `Notice: runtime rule ${skipped.rule} was not run — ${skipped.reason}.` + warnNotice( + `runtime rule ${skipped.rule} was not run — ${skipped.reason}.` ); } @@ -353,7 +376,7 @@ export const checkCommand = defineCommand({ }); const results = dispatched.results; - for (const notice of dispatched.notices) warn(`Notice: ${notice}`); + for (const notice of dispatched.notices) warnNotice(notice); const runNotices = [...plan.notices, ...dispatched.notices]; for (const failure of dispatched.failures) warn(`Error: ${failure}`); diff --git a/packages/cli/src/commands/verify.ts b/packages/cli/src/commands/verify.ts index acced10f..5e6b074c 100644 --- a/packages/cli/src/commands/verify.ts +++ b/packages/cli/src/commands/verify.ts @@ -19,6 +19,7 @@ import { import { outputSchema as verifyTestOutputSchema } from "../schemas/verify-test"; import { makeErrorEnvelope, writeJsonError } from "../types/errors"; import { CLIError } from "../util/cli-error"; +import { markNotice } from "../util/notices"; /** * The shared body of `verify` and `test`. @@ -160,12 +161,14 @@ async function runOverPath(options: { // makes Vale exit zero having enabled nothing, so the clean line above // is exactly the moment the author needs to hear this. // - // A notice is one advisory per line, joined with `\n` (a style-layer - // advisory beside a config-layer one, say). Every line gets the prefix, - // so the second reads as a notice rather than as an unlabelled stray. - if ("notice" in result && result.notice !== undefined) { - for (const line of result.notice.split("\n")) { - console.log(` notice: ${line}`); + // One marker per notice (a style-layer advisory beside a config-layer + // one, say), and one per line within a notice that spans lines — Vale's + // stderr is passed through as written. Without the inner split the + // second line reads as an unlabelled stray rather than as a notice, + // which is the defect `241e1c4` fixed. + for (const notice of result.notices) { + for (const line of markNotice(notice, " notice: ")) { + console.log(line); } } } diff --git a/packages/cli/src/rules/dispatch.ts b/packages/cli/src/rules/dispatch.ts index 2b9f8071..7c0d8383 100644 --- a/packages/cli/src/rules/dispatch.ts +++ b/packages/cli/src/rules/dispatch.ts @@ -8,6 +8,7 @@ import { executeRuntimeRules } from "./runtime/harness"; import type { RuntimeRule } from "./runtime/discover"; import { runAstGrepScan } from "./scan"; import { runVale } from "./vale/run"; +import { collectNotices } from "../util/notices"; /** * Whether `.taskless/rules/vale/` holds anything to run. @@ -47,10 +48,17 @@ export interface EngineOutcome { engine: EngineName; results: CheckResult[]; /** - * Something the user should see that is not a finding — an engine that could - * not run. Advisory: it does not affect the exit code. + * Things the user should see that are not findings — an engine that could not + * run, a config advisory. Advisory: they do not affect the exit code. + * + * A list rather than one joined string, so `runEngines` can concatenate every + * engine's notices into a genuinely flat `DispatchResult.notices`. When this + * was one `"\n"`-joined string, two advisories from one engine arrived as a + * single array element, which `check --json` published with an embedded + * newline and `check`'s text output rendered with only its first line + * prefixed. */ - notice?: string; + notices: string[]; /** * The engine was present and failed. Unlike a notice this must reach the exit * code, or a broken engine reads as a clean run. @@ -153,7 +161,7 @@ async function runAstGrepEngine( options: DispatchOptions ): Promise { if (options.astGrepConfigPath === undefined) { - return { engine: "sg", results: [] }; + return { engine: "sg", results: [], notices: [] }; } const scan = await runAstGrepScan(options.cwd, options.paths, { configPath: options.astGrepConfigPath, @@ -161,7 +169,7 @@ async function runAstGrepEngine( ? {} : { ruleIds: options.astGrepRuleIds }), }); - return { engine: "sg", results: scan.results }; + return { engine: "sg", results: scan.results, notices: [] }; } /** @@ -186,7 +194,7 @@ async function runValeEngine(options: DispatchOptions): Promise { // gate because it is the stronger claim — a rules directory can be present // while assembly yields nothing. if (options.vale === undefined) { - return { engine: "vale", results: [] }; + return { engine: "vale", results: [], notices: [] }; } // A refused assembly is a failure, not a notice, and not a quiet omission of // the offending rule. The config schema turned away a file Vale would have @@ -200,11 +208,12 @@ async function runValeEngine(options: DispatchOptions): Promise { return { engine: "vale", results: [], + notices: [], failure: describeValeRefusal(options.vale), }; } if (!(await hasValeRules(options.cwd))) { - return { engine: "vale", results: [] }; + return { engine: "vale", results: [], notices: [] }; } const outcome = await runVale({ @@ -224,11 +233,10 @@ async function runValeEngine(options: DispatchOptions): Promise { // format, or a key a future Vale adds. It rides through as a notice and // never as a failure: the run succeeded, and letting it touch the exit // code would fail checks over a warning. - const notice = joinNotices([...advisories, outcome.notice]); return { engine: "vale", results: outcome.results, - ...(notice === undefined ? {} : { notice }), + notices: collectNotices([...advisories, ...outcome.notices]), }; } if (outcome.blocking) { @@ -237,18 +245,17 @@ async function runValeEngine(options: DispatchOptions): Promise { // than being folded into it (an advisory is not what failed the run) or // dropped (the config's authors would hear about a `[*]` matcher only on // a run where Vale happened not to crash). - const notice = joinNotices(advisories); return { engine: "vale", results: [], failure: outcome.message, - ...(notice === undefined ? {} : { notice }), + notices: collectNotices(advisories), }; } return { engine: "vale", results: [], - notice: joinNotices([...advisories, outcome.message]) ?? outcome.message, + notices: collectNotices([...advisories, outcome.message]), }; } @@ -271,24 +278,18 @@ function describeValeRefusal(refused: RefusedValeConfig): string { ].join("\n"); } -/** Every present notice on its own line, or `undefined` when there are none. */ -function joinNotices(notices: Array): string | undefined { - const present = notices.filter((notice) => notice !== undefined); - return present.length === 0 ? undefined : present.join("\n"); -} - /** The runtime harness, over rules that planning already cleared to run. */ async function runRuntimeEngine( options: DispatchOptions ): Promise { if (options.runtimeRules.length === 0) { - return { engine: "runtime", results: [] }; + return { engine: "runtime", results: [], notices: [] }; } const results = await executeRuntimeRules(options.cwd, options.runtimeRules, { paths: options.paths, timeoutMs: options.runtimeTimeoutMs, }); - return { engine: "runtime", results }; + return { engine: "runtime", results, notices: [] }; } /** @@ -327,6 +328,7 @@ export async function runEngines( return { engine, results: [], + notices: [], failure: `${engine} engine failed: ${ reason instanceof Error ? reason.message : String(reason) }`, @@ -338,7 +340,7 @@ export async function runEngines( return { results, - notices: outcomes.flatMap((outcome) => outcome.notice ?? []), + notices: outcomes.flatMap((outcome) => outcome.notices), failures, outcomes, exitCode: diff --git a/packages/cli/src/rules/inspect.ts b/packages/cli/src/rules/inspect.ts index 823ecced..372d719a 100644 --- a/packages/cli/src/rules/inspect.ts +++ b/packages/cli/src/rules/inspect.ts @@ -23,6 +23,7 @@ import { } from "./runtime/run-fixtures"; import { validateValeRuleConfig } from "../schemas/vale-config"; import { validateValeRule } from "../schemas/vale-rule"; +import { collectNotices } from "../util/notices"; import { verifyRule, type VerifyResult } from "./verify"; import { violate, type RuleViolation } from "./constraints"; import { describeRuleIdCollision, findRuleIdCollision } from "./id-uniqueness"; @@ -51,13 +52,18 @@ export interface RuleVerification { */ violations: RuleViolation[]; /** - * Something true about the rule that does not make it invalid. An sg rule - * spelled `language: typescript` reaches the right parser and fails nothing, - * but the canonical spelling is `TypeScript` — worth saying, not worth - * failing. Surfaced even on a pass, for the same reason - * {@link RuleTestResult.notice} is. + * Things true about the rule that do not make it invalid, one per element. An + * sg rule spelled `language: typescript` reaches the right parser and fails + * nothing, but the canonical spelling is `TypeScript` — worth saying, not + * worth failing. Surfaced even on a pass, for the same reason + * {@link RuleTestResult.notices} is. + * + * A list rather than one joined string so the renderer owns presentation: it + * prints one ` notice: ` marker per element. A producer that picked its own + * separator mis-rendered instead — which is exactly what `language:` did, + * joining two independent advisories with a space into one run-on line. */ - notice?: string; + notices: string[]; } /** What `test` concluded about one rule. */ @@ -102,14 +108,14 @@ export interface RuleTestResult { */ refused?: string; /** - * Something the engine said about its own configuration, as opposed to about - * the rule. Vale reports a misplaced `.vale.ini` assignment this way: it - * exits zero and finds nothing, so the run looks clean precisely when the - * rule was never enabled. Carried separately from `errors` because it does - * not make the result a failure — and surfaced even on a pass, since a pass - * is the case it exists for. + * What the engine said about its own configuration, as opposed to about the + * rule, one notice per element. Vale reports a misplaced `.vale.ini` + * assignment this way: it exits zero and finds nothing, so the run looks + * clean precisely when the rule was never enabled. Carried separately from + * `errors` because it does not make the result a failure — and surfaced even + * on a pass, since a pass is the case it exists for. */ - notice?: string; + notices: string[]; } /** What `test` needs beyond a rule, all of it about the runtime engine. */ @@ -175,9 +181,7 @@ async function verifySgRule( ok: errors.length === 0, errors, violations, - ...(result.schema.notice === undefined - ? {} - : { notice: result.schema.notice }), + notices: result.schema.notices, }, result, }; @@ -336,7 +340,7 @@ async function verifyRuleComponents( ok: errors.length === 0, errors, violations, - ...(advisories.length === 0 ? {} : { notice: advisories.join("\n") }), + notices: collectNotices(advisories), }; } @@ -385,7 +389,14 @@ async function verifyRuleComponents( } } } - return { engine, ruleId, ok: errors.length === 0, errors, violations: [] }; + return { + engine, + ruleId, + ok: errors.length === 0, + errors, + violations: [], + notices: [], + }; } /** @@ -471,9 +482,7 @@ export async function testOneRule( errors, violations, ran: true, - ...(verification.notice === undefined - ? {} - : { notice: verification.notice }), + notices: verification.notices, }; } @@ -492,6 +501,7 @@ export async function testOneRule( errors: [result.outcome.message], violations: [], ran: false, + notices: [], }; } const errors: string[] = []; @@ -512,7 +522,7 @@ export async function testOneRule( errors, violations: [], ran: true, - ...(result.notice === undefined ? {} : { notice: result.notice }), + notices: result.notices, }; } @@ -552,6 +562,7 @@ export async function testOneRule( violations: [], ran: false, refused: reason, + notices: [], }; } @@ -575,6 +586,7 @@ export async function testOneRule( violations: [], ran: false, refused: reason, + notices: [], }; } @@ -593,6 +605,7 @@ export async function testOneRule( errors: [error instanceof Error ? error.message : String(error)], violations: [], ran: false, + notices: [], }; } @@ -608,5 +621,6 @@ export async function testOneRule( errors: describeFixtureReport(ruleId, report), violations: [], ran: true, + notices: [], }; } diff --git a/packages/cli/src/rules/vale/run.ts b/packages/cli/src/rules/vale/run.ts index b8bfa6e8..a2131846 100644 --- a/packages/cli/src/rules/vale/run.ts +++ b/packages/cli/src/rules/vale/run.ts @@ -29,6 +29,7 @@ import { type ValeConfigError, type ValeOutput, } from "./map"; +import { collectNotices } from "../../util/notices"; /** * The Vale config a run reads, relative to the project root. @@ -81,7 +82,7 @@ export const VALE_TIMEOUT_MS = 60_000; * `ok` is non-blocking even when it carries findings: severity decides the exit * code there, the same as for every other engine. * - * `ok` also carries an optional `notice`: whatever Vale wrote to stderr while + * `ok` also carries `notices`: whatever Vale wrote to stderr while * still exiting zero. That combination is not noise. Vale reports a rule * assignment placed outside any section as `W101 … is ignoring it` — on stderr, * with exit 0 and a well-formed empty result on stdout — so discarding it @@ -94,8 +95,16 @@ export type ValeRunOutcome = status: "ok"; blocking: false; results: CheckResult[]; - /** Vale's stderr on a zero-exit run, when it wrote any. */ - notice?: string; + /** + * What this run has to say without failing: Vale's stderr on a zero-exit + * run, and the converter-skip notice, one element each. + * + * A list rather than one joined string, because the two are independent — + * a project can perfectly well have a section-less rule assignment *and* + * an AsciiDoc file — and every consumer renders one notice per marker. + * Empty, never absent, so a caller can concatenate without a fallback. + */ + notices: string[]; } | { status: "unavailable"; blocking: false; message: string } | { status: "timeout"; blocking: true; message: string } @@ -286,7 +295,7 @@ async function targetFileParseError( * message it also carries. */ type ValeAttempt = - | { status: "ok"; results: CheckResult[]; notice?: string } + | { status: "ok"; results: CheckResult[]; notices: string[] } | { status: "timeout"; message: string } | { status: "failed"; message: string; configError?: ValeConfigError }; @@ -388,18 +397,16 @@ async function spawnVale( // Attached to every `ok` path so a diagnostic cannot be dropped by which // branch happened to produce the (empty) results. const diagnostic = stderrChunks.join("").trim(); - // Both advisories share one field, so they are joined rather than one - // overwriting the other: a project can perfectly well have a section-less - // rule assignment *and* an AsciiDoc file, and dropping either message - // would be a silent skip wearing the other's clothes. - const advisories = [ - ...(skipped === undefined ? [] : [skipped]), - ...(diagnostic === "" - ? [] - : [`Vale reported while running: ${diagnostic}`]), - ]; - const notice = - advisories.length === 0 ? {} : { notice: advisories.join("\n") }; + // Both advisories ride the same field as separate elements, rather than + // one overwriting the other: a project can perfectly well have a + // section-less rule assignment *and* an AsciiDoc file, and dropping + // either message would be a silent skip wearing the other's clothes. + const notices = collectNotices([ + skipped, + diagnostic === "" + ? undefined + : `Vale reported while running: ${diagnostic}`, + ]); const stdout = stdoutChunks.join("").trim(); if (stdout === "") { @@ -407,7 +414,7 @@ async function spawnVale( // maps to [] below. This branch is for a Vale that says nothing at all // — cheap insurance against JSON.parse("") reporting a clean run as a // failure. - settle({ status: "ok", results: [], ...notice }); + settle({ status: "ok", results: [], notices }); return; } @@ -434,7 +441,7 @@ async function spawnVale( settle({ status: "ok", results: toValeCheckResults(parsed as ValeOutput), - ...notice, + notices, }); } catch (error) { settle({ @@ -648,7 +655,7 @@ export async function runVale( status: "ok", blocking: false, results: [...excludedFindings, ...attempt.results], - ...(attempt.notice === undefined ? {} : { notice: attempt.notice }), + notices: attempt.notices, }; } diff --git a/packages/cli/src/rules/vale/verify.ts b/packages/cli/src/rules/vale/verify.ts index 5594d126..2abb74cd 100644 --- a/packages/cli/src/rules/vale/verify.ts +++ b/packages/cli/src/rules/vale/verify.ts @@ -155,15 +155,17 @@ export interface ValeRuleVerification { */ fixtures: ValeFixtureCoverage; /** - * Vale's stderr from a run that still exited zero, when it wrote any. + * What Vale said about a run that still exited zero, one notice per element. * * Carried on the verification rather than dropped at this seam because the * `W101 … isn't a core option` warning — what Vale says about an assignment * placed above the first `[…]` section — arrives on exactly this path: exit * zero, empty findings, a rule that verifies clean while Vale ignores it. - * Absent when Vale was never run (a one-sided fixture set short-circuits). + * Empty when Vale was never run (a one-sided fixture set short-circuits) and + * when it ran with nothing to say; the two are not worth distinguishing, and + * empty-never-absent lets a caller concatenate without a fallback. */ - notice?: string; + notices: string[]; } /** @@ -246,6 +248,7 @@ export async function verifyValeRule( missingFailures: [], unexpectedFindings: [], fixtures, + notices: [], }; } @@ -285,7 +288,7 @@ export async function verifyValeRule( missingFailures, unexpectedFindings, fixtures, - ...(outcome.notice === undefined ? {} : { notice: outcome.notice }), + notices: outcome.notices, }; } finally { rmSync(configDirectory, { recursive: true, force: true }); diff --git a/packages/cli/src/rules/verify.ts b/packages/cli/src/rules/verify.ts index 6522f81a..8caeaef4 100644 --- a/packages/cli/src/rules/verify.ts +++ b/packages/cli/src/rules/verify.ts @@ -54,13 +54,18 @@ export interface LayerResult { * Layer 1's verdict, plus anything true about the rule that is worth saying * without failing it. * - * The `notice` carries the non-fatal half of the `language:` check — an + * `notices` carries the non-fatal half of the `language:` check — an * accepted-but-off-list spelling, or a `files:` glob a valid language cannot - * reach. Separate from `errors` for the same reason `RuleTestResult.notice` is: - * it must be sayable on a rule that passed, and it must not turn CI red. + * reach. Separate from `errors` for the same reason `RuleTestResult.notices` + * is: it must be sayable on a rule that passed, and it must not turn CI red. + * + * One notice per element, and the two above can both be true of one rule. They + * used to be joined with a space into a single string, which the renderer then + * printed behind one ` notice: ` marker as a run-on sentence. Handing back + * elements is what lets the renderer mark each one. */ export interface SchemaLayerResult extends LayerResult { - notice?: string; + notices: string[]; } export interface RequirementsResult extends LayerResult { @@ -738,7 +743,7 @@ export async function verifyRule( return { success: false, ruleId, - schema: { valid: false, errors: [errorMessage] }, + schema: { valid: false, errors: [errorMessage], notices: [] }, requirements: { valid: false, errors: [errorMessage] }, tests: { valid: false, @@ -771,6 +776,7 @@ export async function verifyRule( errors: [ `Rule file not found: .taskless/${RULES_DIRECTORY}/sg/${ruleId}/${ruleId}.yml`, ], + notices: [], }, requirements: { valid: false, @@ -794,7 +800,7 @@ export async function verifyRule( return { success: false, ruleId, - schema: { valid: false, errors: [message] }, + schema: { valid: false, errors: [message], notices: [] }, requirements: { valid: false, errors: ["Cannot check requirements: invalid YAML"], @@ -822,9 +828,7 @@ export async function verifyRule( valid: parsed.valid && language.errors.length === 0, errors: [...parsed.errors, ...language.errors], violations: language.violations, - ...(language.notices.length === 0 - ? {} - : { notice: language.notices.join(" ") }), + notices: language.notices, }; // Layer 2 diff --git a/packages/cli/src/schemas/rules-verify.ts b/packages/cli/src/schemas/rules-verify.ts index b560df41..4a64fdc4 100644 --- a/packages/cli/src/schemas/rules-verify.ts +++ b/packages/cli/src/schemas/rules-verify.ts @@ -49,18 +49,22 @@ const layerResultSchema = z.object({ }); /** - * Layer 1, which alone can also report something true that is not a failure. + * Layer 1, which alone can also report things that are true but not failures. * * An sg rule spelled `language: typescript` reaches the right parser and * verifies clean, but `TypeScript` is how ast-grep spells it. That is worth * saying on a rule that passed, so it cannot ride in `errors`. + * + * A list, one notice per element, because a rule can be true of more than one + * of these at once — an off-list spelling and a `files:` glob its language + * cannot parse. It was a single string joined with a space, which a consumer + * could not split back apart and which rendered as one run-on line. */ const schemaLayerResultSchema = layerResultSchema.extend({ - notice: z - .string() - .optional() + notices: z + .array(z.string()) .describe( - "Something true about the rule that does not make it invalid — an accepted-but-off-list `language:` spelling above all. Present only when there is something to say" + "Things true about the rule that do not make it invalid — an accepted-but-off-list `language:` spelling above all. One notice per element; empty when there is nothing to say, never absent" ), }); @@ -117,11 +121,10 @@ export const valeVerifyOutputSchema = z.object({ unexpectedFindings: z .array(z.string()) .describe("pass/ fixtures the rule flagged and should not have"), - notice: z - .string() - .optional() + notices: z + .array(z.string()) .describe( - "What Vale wrote to stderr while still exiting zero — a W101 ignored-assignment warning above all. Present only when Vale said something" + "What Vale wrote to stderr while still exiting zero — a W101 ignored-assignment warning above all — plus the converter-skip notice, one per element. Empty when Vale said nothing, never absent" ), }); diff --git a/packages/cli/src/schemas/verify-test.ts b/packages/cli/src/schemas/verify-test.ts index 1fab3d65..41dacdab 100644 --- a/packages/cli/src/schemas/verify-test.ts +++ b/packages/cli/src/schemas/verify-test.ts @@ -42,11 +42,10 @@ const ruleResultSchema = z.object({ .describe( "`test` only: the execution policy declined to run the rule's fixtures, and why. Neither a pass nor a failure, excluded from the rules tested, and never on its own a reason for a non-zero exit" ), - notice: z - .string() - .optional() + notices: z + .array(z.string()) .describe( - "Something true about the rule that does not make it a failure, reported even on a pass" + "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" ), }); diff --git a/packages/cli/src/util/notices.ts b/packages/cli/src/util/notices.ts new file mode 100644 index 00000000..b5f14e78 --- /dev/null +++ b/packages/cli/src/util/notices.ts @@ -0,0 +1,78 @@ +/** + * Building the `notices` a run carries — advisory text a command should say + * without failing over it. + * + * A leaf module with no imports of its own, deliberately. The producers live in + * `rules/dispatch.ts`, `rules/inspect.ts`, `rules/verify.ts` and + * `rules/vale/run.ts`; having any of them import this from a sibling would add + * an edge inside `src/rules/` between modules that already sit close to a cycle + * (see the `filesystem/migrate.ts` cycle fixed in taskless/cli#388, which cost + * real time and was found by luck). `import-x/no-cycle` runs over every + * TypeScript file and would catch it eventually; not creating the edge is + * cheaper than relying on that. + */ + +/** + * Gather advisories into the flat, ordered list a `notices` field carries. + * + * **One element per notice.** This is the contract the renderers depend on: + * `commands/check.ts` prints `Notice: ` and `commands/verify.ts` prints + * ` notice: ` once per notice, so two advisories glued into one element read + * as one notice with a stray tail. That is the defect `241e1c4` fixed in + * `verify`, and it is why producers hand back elements rather than a + * pre-joined string: presentation belongs to the renderer, and a producer that + * picked its own separator would mis-render in the direction hardest to notice + * — the notice still appears, just wrongly attributed. + * + * A single element may still contain `"\n"`, because one notice can be + * multi-line prose: Vale's stderr on a zero-exit run is passed through as + * written and can span lines. That is formatting *within* one message, not a + * separator *between* messages, and the renderers prefix every line of it. + * + * `undefined` entries are dropped, because callers assemble notices from + * optional sources — a Vale advisory that may not exist beside a schema one + * that may not either — and the alternative is a filter at every call site. + * + * **Empty strings are dropped too.** A `""` would otherwise render as a bare + * `Notice: ` marker saying nothing. Four producers independently emitting that + * was never a contract anyone designed; it was what + * `filter((x) => x !== undefined)` happened to do. + * + * @param notices Advisories in the order they should be read. Order is + * preserved: callers put the more specific advisory first. + * @returns Every present, non-empty notice, in order. Empty when none survive, + * which is what lets a call site spread + * `...(notices.length === 0 ? {} : { notices })` and leave the field off. + */ +export function collectNotices( + notices: ReadonlyArray +): string[] { + return notices.filter( + (notice): notice is string => notice !== undefined && notice !== "" + ); +} + +/** + * Render one notice as the lines a command prints, each behind `marker`. + * + * **Every line, not just the first.** A notice can be multi-line prose that + * the CLI did not author: Vale's stderr is passed through as written, and a + * runtime repair notice embeds an `Error.message` from whatever failed. Mark + * only the first line and the rest read as stray output rather than as + * something the run is telling its author — the defect `241e1c4` fixed for + * `verify`, which `check` then carried on its dispatched notices and, on the + * runtime plan's notices, printed with no marker at all. + * + * Shared by both renderers so the two cannot drift apart again. They differ + * only in the marker — `commands/check.ts` prints `Notice: ` at the left + * margin, `commands/verify.ts` prints ` notice: ` indented under the rule + * it belongs to — which is a presentation choice, not a second contract. + * + * @param notice One notice, as {@link collectNotices} yields it. + * @param marker The prefix to put on each of its lines. + * @returns One string per line of `notice`, each already prefixed. Never + * empty: a notice with no newline renders as exactly one line. + */ +export function markNotice(notice: string, marker: string): string[] { + return notice.split("\n").map((line) => `${marker}${line}`); +} diff --git a/packages/cli/test/notices.test.ts b/packages/cli/test/notices.test.ts new file mode 100644 index 00000000..42edaf2d --- /dev/null +++ b/packages/cli/test/notices.test.ts @@ -0,0 +1,101 @@ +import { describe, expect, it } from "vitest"; + +import { collectNotices, markNotice } from "../src/util/notices"; + +/** + * The one place the `notices` contract is decided, so it is the one place the + * contract is pinned. + * + * Four producers used to hold this shape as four copies of an expression, and + * nothing obliged them to agree — one of them did not, joining with a space + * where the others joined with a newline. These are the claims the producers + * are now entitled to make. + */ +describe("collectNotices", () => { + it("keeps every present notice as its own element, in order", () => { + // Order is the caller's, not the helper's: a call site puts the more + // specific advisory first and the renderer prints them in that order. + expect(collectNotices(["first", "second", "third"])).toEqual([ + "first", + "second", + "third", + ]); + }); + + it("drops undefined, which is how an absent source is spelled", () => { + // Callers assemble notices from optional sources — a Vale advisory that + // may not exist beside a schema one that may not either. Filtering here is + // what keeps a `.filter` out of every call site. + expect(collectNotices([undefined, "present", undefined])).toEqual([ + "present", + ]); + }); + + it("drops the empty string, which renders as a marker saying nothing", () => { + // Not what the previous `filter((x) => x !== undefined)` did, and + // deliberately so. A `""` survived it and reached the renderer, which + // printed a bare `Notice: ` with nothing after it. Four producers + // independently emitting that was never a contract anyone designed. + expect(collectNotices(["", "present", ""])).toEqual(["present"]); + }); + + it("is empty when nothing survives, so a call site can omit the field", () => { + expect(collectNotices([])).toEqual([]); + expect(collectNotices([undefined, ""])).toEqual([]); + }); + + it("leaves a multi-line notice as one element", () => { + // A single notice may legitimately span lines — Vale's stderr is passed + // through as written. That is formatting WITHIN one message, not a + // separator BETWEEN messages, so the helper does not split it; the + // renderers prefix each of its lines instead. + expect(collectNotices(["line one\nline two"])).toEqual([ + "line one\nline two", + ]); + }); +}); + +/** + * The renderers' half of the same contract. + * + * `check` and `verify` differ only in the marker, so they share this. The + * multi-line case is the one worth pinning directly: no notice the CLI + * produces today spans lines, but several embed text the CLI did not author — + * Vale's stderr, and an `Error.message` inside a runtime repair notice — so it + * is a latent case that will arrive without anyone choosing it. + */ +describe("markNotice", () => { + it("marks every line of a multi-line notice, not just the first", () => { + // The whole defect, in one assertion. An unmarked second line reads as + // stray output rather than as part of the notice above it. + expect(markNotice("first line\nsecond line\nthird", "Notice: ")).toEqual([ + "Notice: first line", + "Notice: second line", + "Notice: third", + ]); + }); + + it("renders a single-line notice as exactly one line", () => { + expect(markNotice("just the one", "Notice: ")).toEqual([ + "Notice: just the one", + ]); + }); + + it("carries the caller's marker, which is the only difference between the two renderers", () => { + // `verify` indents under the rule the notice belongs to; `check` marks at + // the left margin. Presentation, not a second contract. + expect(markNotice("a\nb", " notice: ")).toEqual([ + " notice: a", + " notice: b", + ]); + }); + + it("keeps an empty trailing line marked rather than dropping it", () => { + // Splitting is not filtering. A notice that ends in a newline still had + // that line, and silently dropping output is how a notice loses its tail. + expect(markNotice("text\n", "Notice: ")).toEqual([ + "Notice: text", + "Notice: ", + ]); + }); +}); diff --git a/packages/cli/test/runtime-check.test.ts b/packages/cli/test/runtime-check.test.ts index c370cee5..f06f0294 100644 --- a/packages/cli/test/runtime-check.test.ts +++ b/packages/cli/test/runtime-check.test.ts @@ -285,7 +285,18 @@ describe("check: static vs runtime dispatch", () => { directory, "--dangerously-run-scripts", ]); - expect(stderr).toContain("dangerously-run-scripts"); + // The warning is a runtime PLAN notice, and plan notices used to be + // printed by their own loop with no marker at all while the dispatched + // ones were marked — so the same message looked like two different kinds + // of thing depending on which list it arrived on, and `--json` mixed both + // into one `notices` array. Every notice `check` prints is marked now. + const warningLines = stderr + .split("\n") + .filter((line) => line.includes("dangerously-run-scripts")); + expect(warningLines.length).toBeGreaterThan(0); + for (const line of warningLines) { + expect(line.startsWith("Notice: ")).toBe(true); + } expect(stdout).toContain("demo"); // runtime finding surfaced }); diff --git a/packages/cli/test/schemas-export.test.ts b/packages/cli/test/schemas-export.test.ts index e7ef6b65..9f5bd5ed 100644 --- a/packages/cli/test/schemas-export.test.ts +++ b/packages/cli/test/schemas-export.test.ts @@ -226,7 +226,7 @@ describe("verifyOutputSchema and valeVerifyOutputSchema", () => { fixtures: result.fixtures, missingFailures: result.missingFailures, unexpectedFindings: result.unexpectedFindings, - ...(result.notice === undefined ? {} : { notice: result.notice }), + notices: result.notices, }) as { success: boolean; ruleId: string }; expect(parsed.success).toBe(true); diff --git a/packages/cli/test/vale-formats.test.ts b/packages/cli/test/vale-formats.test.ts index e9b5805e..c6772a0c 100644 --- a/packages/cli/test/vale-formats.test.ts +++ b/packages/cli/test/vale-formats.test.ts @@ -334,8 +334,11 @@ withVale( expect(outcome.status).toBe("ok"); if (outcome.status !== "ok") return; - expect(outcome.notice).toContain("docs/nested.adoc"); - expect(outcome.notice).toContain("asciidoctor"); + // One skip notice, as one element: the converter-skip advisory is a + // single notice, not two glued together by a separator. + expect(outcome.notices).toHaveLength(1); + expect(outcome.notices[0]).toContain("docs/nested.adoc"); + expect(outcome.notices[0]).toContain("asciidoctor"); }); it("declines a converter-dependent file even when named explicitly", async () => { @@ -359,7 +362,8 @@ withVale( true ); expect(outcome.results.length).toBeGreaterThan(0); - expect(outcome.notice).toContain("d.adoc"); + expect(outcome.notices).toHaveLength(1); + expect(outcome.notices[0]).toContain("d.adoc"); }); it("keeps out of .taskless/ while excluding converter formats", () => { diff --git a/packages/cli/test/vale-orchestration.test.ts b/packages/cli/test/vale-orchestration.test.ts index 84aadfd0..aa416c59 100644 --- a/packages/cli/test/vale-orchestration.test.ts +++ b/packages/cli/test/vale-orchestration.test.ts @@ -111,13 +111,17 @@ const binPath = resolve(import.meta.dirname, "../dist/index.js"); /** Run the built CLI, tolerating a non-zero exit. */ async function runCli( args: string[] -): Promise<{ stdout: string; exitCode: number }> { +): Promise<{ stdout: string; stderr: string; exitCode: number }> { try { - const { stdout } = await execFileAsync("node", [binPath, ...args]); - return { stdout, exitCode: 0 }; + const { stdout, stderr } = await execFileAsync("node", [binPath, ...args]); + return { stdout, stderr, exitCode: 0 }; } catch (error) { - const execError = error as { stdout: string; code: number }; - return { stdout: execError.stdout ?? "", exitCode: execError.code }; + const execError = error as { stdout: string; stderr: string; code: number }; + return { + stdout: execError.stdout ?? "", + stderr: execError.stderr ?? "", + exitCode: execError.code, + }; } } @@ -361,6 +365,7 @@ describe("runEngines when Vale is unavailable", () => { matchedText: "simply", }, ], + notices: [], }); const cwd = makeMixedProject(); @@ -445,16 +450,22 @@ describe("config advisories ride on every Vale outcome", () => { }); } - it("joins Vale's own zero-exit diagnostic on a run that succeeded", async () => { + it("carries Vale's own zero-exit diagnostic beside the schema advisory", async () => { const dispatched = await dispatchWithAdvisory({ status: "ok", blocking: false, results: [], - notice: "W101 something Vale said", + notices: ["W101 something Vale said"], }); - expect(dispatched.notices).toHaveLength(1); + // TWO elements, not one string carrying both. The schema advisory and + // Vale's own diagnostic are independent notices, and a producer that + // joined them — with "\n", "; " or anything else — would fail here. + expect(dispatched.notices).toHaveLength(2); expect(dispatched.notices[0]).toContain("[.taskless/**]"); - expect(dispatched.notices[0]).toContain("W101 something Vale said"); + expect(dispatched.notices[1]).toBe("W101 something Vale said"); + expect(dispatched.notices.some((notice) => notice.includes("\n"))).toBe( + false + ); expect(dispatched.failures).toEqual([]); }); @@ -474,15 +485,18 @@ describe("config advisories ride on every Vale outcome", () => { expect(dispatched.exitCode).toBe(1); }); - it("joins the skip notice when the binary is unavailable", async () => { + it("carries the skip notice beside the unavailable message", async () => { const dispatched = await dispatchWithAdvisory({ status: "unavailable", blocking: false, message: "Vale binary not found", }); - expect(dispatched.notices).toHaveLength(1); + // Two independent notices, two elements. The advisory is about the config + // and the message is about the binary; gluing them into one string made + // `check` render the second without its `Notice: ` marker. + expect(dispatched.notices).toHaveLength(2); expect(dispatched.notices[0]).toContain("[.taskless/**]"); - expect(dispatched.notices[0]).toContain("Vale binary not found"); + expect(dispatched.notices[1]).toContain("Vale binary not found"); expect(dispatched.failures).toEqual([]); expect(dispatched.exitCode).toBe(0); }); @@ -564,3 +578,89 @@ withVale("a repository containing a converter-dependent file", () => { expect(notices).toContain("rst2html"); }); }); + +/** A project whose two Vale rules each draw a repeated-key advisory. */ +function makeTwoAdvisoryProject(): string { + const cwd = mkdtempSync(join(tmpdir(), "vale-notices-")); + workspaces.push(cwd); + mkdirSync(join(cwd, ".taskless", "rules", "vale"), { recursive: true }); + for (const ruleId of ["no-simply", "no-twist"] as const) { + const token = ruleId === "no-simply" ? "simply" : "twist"; + mkdirSync(join(cwd, ".taskless", "rules", "vale", ruleId), { + recursive: true, + }); + writeFileSync( + join(cwd, ".taskless", "rules", "vale", ruleId, `${ruleId}.yml`), + `extends: existence\nmessage: "Avoid '${token}'"\nlevel: warning\ntokens:\n - ${token}\n` + ); + // The repeat has to be within ONE matcher, which is what the config + // schema says something about. `[docs/**]` keeps the rule enabled + // somewhere, so the repeat under `[*.md]` stays an advisory rather than + // becoming a rejection for a rule that is off everywhere. + writeFileSync( + join(cwd, ".taskless", "rules", "vale", ruleId, ".vale.ini"), + `[docs/**]\ntskl) rule = ${ruleId}\n${ruleId}.${ruleId} = YES\n\n` + + `[*.md]\ntskl) rule = ${ruleId}\n${ruleId}.${ruleId} = YES\n` + + `${ruleId}.${ruleId} = NO\n` + ); + } + writeFileSync( + join(cwd, ".taskless", "taskless.json"), + JSON.stringify({ version: LATEST_SCHEMA_VERSION, install: {} }) + ); + writeFileSync(join(cwd, "doc.md"), "Just simply do it.\n"); + return cwd; +} + +/** + * `check` renders one marker per notice, on stderr. + * + * The defect these cover was user-visible and lived in `check` alone. Every + * producer used to glue its advisories into ONE string with `"\n"`, and + * `check` printed `Notice: ` once per element — so a run with two advisories + * printed the first behind a marker and the second as a bare, unindented line + * with nothing marking it as a notice. `verify` had the same bug and it was + * fixed in `241e1c4`; `check` kept it. + * + * Driven through the built CLI over a real project rather than through + * `runEngines`, because the renderer is what regressed and it lives in the + * command. Two Vale rules each carrying a config advisory is the smallest + * project that produces two independent notices, and it needs no Vale binary: + * the advisories come from the config schema at assembly time, so they are on + * the result whether Vale then runs or reports itself unavailable. + */ +describe("check renders one marker per notice", () => { + it("gives each advisory its own Notice: line in text output", async () => { + const cwd = makeTwoAdvisoryProject(); + const { stderr } = await runCli(["check", "-d", cwd]); + + const advisoryLines = stderr + .split("\n") + .filter((line) => line.includes("assigns")); + + // Two advisories, two lines, each marked. Before the fix these arrived as + // one `"\n"`-joined element and printed as one marked line plus one stray. + expect(advisoryLines).toHaveLength(2); + for (const line of advisoryLines) { + expect(line.startsWith("Notice: ")).toBe(true); + } + expect(advisoryLines.some((line) => line.includes("no-simply"))).toBe(true); + expect(advisoryLines.some((line) => line.includes("no-twist"))).toBe(true); + }); + + it("publishes them as separate --json elements, none spanning lines", async () => { + const cwd = makeTwoAdvisoryProject(); + const { stdout } = await runCli(["check", "-d", cwd, "--json"]); + const notices = parseJson(stdout).notices ?? []; + + expect(notices.filter((notice) => notice.includes("assigns"))).toHaveLength( + 2 + ); + // The half a consumer sees. A `"\n"` inside an element means several + // notices were shipped as one, and nothing published the separator that + // would let the consumer split them back apart. + for (const notice of notices) { + expect(notice).not.toContain("\n"); + } + }); +}); diff --git a/packages/cli/test/vale-run.test.ts b/packages/cli/test/vale-run.test.ts index 1bfe82dd..f313d085 100644 --- a/packages/cli/test/vale-run.test.ts +++ b/packages/cli/test/vale-run.test.ts @@ -119,7 +119,12 @@ withVale("runVale against the real binary", () => { const outcome = await runVale({ cwd, paths: ["doc.md"] }); // Vale prints nothing at all when it finds nothing; that must read as an // empty result rather than as unparseable output. - expect(outcome).toEqual({ status: "ok", blocking: false, results: [] }); + expect(outcome).toEqual({ + status: "ok", + blocking: false, + results: [], + notices: [], + }); }); it("normalizes suggestion to hint", async () => { @@ -246,11 +251,15 @@ withVale("runVale against the real binary", () => { expect(outcome.status).toBe("ok"); if (outcome.status !== "ok") return; expect(outcome.results).toEqual([]); - expect(outcome.notice).toContain("W101"); - expect(outcome.notice).toContain("rules.no-simply"); + // One notice per element. Asserting on the element rather than on the + // whole field is what makes a producer that glued two advisories together + // fail here instead of passing a substring match. + expect(outcome.notices).toHaveLength(1); + expect(outcome.notices[0]).toContain("W101"); + expect(outcome.notices[0]).toContain("rules.no-simply"); }); - it("reports no notice when Vale writes nothing to stderr", async () => { + it("reports no notices when Vale writes nothing to stderr", async () => { const cwd = makeProject( `${header}\n[*.md]\nrules.no-simply = YES\n`, { "no-simply": existenceRule("simply", "Avoid 'simply'") }, @@ -260,7 +269,7 @@ withVale("runVale against the real binary", () => { const outcome = await runVale({ cwd, paths: ["doc.md"] }); expect(outcome.status).toBe("ok"); if (outcome.status !== "ok") return; - expect(outcome.notice).toBeUndefined(); + expect(outcome.notices).toEqual([]); }); it("terminates and reports a timeout rather than hanging", async () => { diff --git a/packages/cli/test/vale-verify.test.ts b/packages/cli/test/vale-verify.test.ts index 744ba17f..ec3c2f95 100644 --- a/packages/cli/test/vale-verify.test.ts +++ b/packages/cli/test/vale-verify.test.ts @@ -253,6 +253,7 @@ withVale("verifyValeRule", () => { missingFailures: [], unexpectedFindings: [], fixtures: "both", + notices: [], }); }); @@ -495,24 +496,25 @@ describe("verifyValeRule and Vale's notice", () => { matchedText: "simply", }, ], - notice: NOTICE, + notices: [NOTICE], }); const result = verification(await verifyValeRule(cwd, "no-simply")); expect(result.passed).toBe(true); - expect(result.notice).toBe(NOTICE); + expect(result.notices).toEqual([NOTICE]); }); - it("leaves the notice absent when Vale said nothing", async () => { + it("leaves the notices empty when Vale said nothing", async () => { const cwd = bothBuckets(); const run = await import("../src/rules/vale/run"); vi.spyOn(run, "runVale").mockResolvedValue({ status: "ok", blocking: false, results: [], + notices: [], }); const result = verification(await verifyValeRule(cwd, "no-simply")); - expect(result.notice).toBeUndefined(); + expect(result.notices).toEqual([]); }); }); diff --git a/packages/cli/test/verify-test-commands.test.ts b/packages/cli/test/verify-test-commands.test.ts index 048cd4a1..3a9b1833 100644 --- a/packages/cli/test/verify-test-commands.test.ts +++ b/packages/cli/test/verify-test-commands.test.ts @@ -35,7 +35,7 @@ interface Report { ok: boolean; errors: string[]; violations: { constraintId: string; message: string }[]; - notice?: string; + notices: string[]; }[]; } @@ -202,7 +202,7 @@ describe("verify checks components without requiring tests", () => { expect(rule?.errors).toContain(rule?.violations[0]?.message); }); - it("accepts a Vale config with a repeated key, and says so on notice", async () => { + it("accepts a Vale config with a repeated key, and says so on notices", async () => { // The repeat is in a matcher that ends NO, but [docs/**] before it keeps // the rule enabled somewhere, so the config is accepted with a notice. await valeRule("no-simply", { @@ -215,14 +215,16 @@ describe("verify checks components without requiring tests", () => { const rule = (JSON.parse(result.stdout) as Report).rules[0]; expect(rule?.ok).toBe(true); expect(rule?.violations).toEqual([]); - expect(rule?.notice).toMatch(/assigns no-simply\.no-simply again/); + expect(rule?.notices).toEqual([ + expect.stringMatching(/assigns no-simply\.no-simply again/), + ]); }); // The style schema's own advisory, through the real CLI. // `vale-schema-contract.test.ts` holds the advisory to its rule; this says - // it reaches `notice` on `verify` output and `--json`, joined with whatever - // the config layer had to say, and that a one-entry list draws nothing. - it("accepts a Vale rule whose raw has two entries, and says so on notice", async () => { + // it reaches `notices` on `verify` output and `--json`, beside whatever the + // config layer had to say, and that a one-entry list draws nothing. + it("accepts a Vale rule whose raw has two entries, and says so on notices", async () => { await valeRule("no-twist", { config: "[*.md]\ntskl) rule = no-twist\nno-twist.no-twist = YES\n", style: @@ -234,9 +236,9 @@ describe("verify checks components without requiring tests", () => { const rule = (JSON.parse(json.stdout) as Report).rules[0]; expect(rule?.ok).toBe(true); expect(rule?.errors).toEqual([]); - expect(rule?.notice).toBe( - "no-twist: raw has 2 entries; Vale joins them into one pattern with no separator, so the second never matches on its own. Write one entry with (a|b) unless the join is intended." - ); + expect(rule?.notices).toEqual([ + "no-twist: raw has 2 entries; Vale joins them into one pattern with no separator, so the second never matches on its own. Write one entry with (a|b) unless the join is intended.", + ]); const text = await runCli(["verify", "-d", cwd]); expect(text.exitCode).toBe(0); @@ -254,13 +256,13 @@ describe("verify checks components without requiring tests", () => { expect(json.exitCode).toBe(0); const rule = (JSON.parse(json.stdout) as Report).rules[0]; expect(rule?.ok).toBe(true); - expect(rule?.notice).toBeUndefined(); + expect(rule?.notices).toEqual([]); const text = await runCli(["verify", "-d", cwd]); expect(text.stdout).not.toContain("notice:"); }); - it("joins a raw advisory and a config advisory on one notice", async () => { + it("carries a raw advisory and a config advisory as two notices", async () => { await valeRule("no-twist", { config: "[docs/**]\ntskl) rule = no-twist\nno-twist.no-twist = YES\n\n" + @@ -272,10 +274,16 @@ describe("verify checks components without requiring tests", () => { const result = await runCli(["verify", "-d", cwd, "--json"]); expect(result.exitCode).toBe(0); const rule = (JSON.parse(result.stdout) as Report).rules[0]; - expect(rule?.notice?.split("\n")).toEqual([ + // Two elements, not one string a consumer has to split on a separator + // nothing published. `--json` is the shape an agent reads, so the split + // has to have happened before it gets there. + expect(rule?.notices).toEqual([ expect.stringContaining("raw has 2 entries"), expect.stringContaining("assigns no-twist.no-twist again"), ]); + for (const notice of rule?.notices ?? []) { + expect(notice).not.toContain("\n"); + } // Text mode prefixes every line, so the second advisory is labelled too // rather than trailing the first as an unindented stray. @@ -300,7 +308,9 @@ describe("verify checks components without requiring tests", () => { expect(rule?.violations.map((violation) => violation.constraintId)).toEqual( ["vale-config-enabled-somewhere"] ); - expect(rule?.notice).toMatch(/assigns no-simply\.no-simply again/); + expect(rule?.notices).toEqual([ + expect.stringMatching(/assigns no-simply\.no-simply again/), + ]); }); // Found by running the recipe's own worked `consistency` rule under the diff --git a/packages/cli/test/verify.test.ts b/packages/cli/test/verify.test.ts index 95e76d89..f58b73a3 100644 --- a/packages/cli/test/verify.test.ts +++ b/packages/cli/test/verify.test.ts @@ -476,7 +476,10 @@ describe("verifyRule", () => { runTests: false, }); expect(result.schema.valid).toBe(true); - expect(result.schema.notice).toContain( + // One element, asserted as the whole list: a producer that appended a + // second advisory into the same string would fail here. + expect(result.schema.notices).toHaveLength(1); + expect(result.schema.notices[0]).toContain( "TypeScript is how ast-grep spells it" ); }); @@ -490,7 +493,8 @@ describe("verifyRule", () => { runTests: false, }); expect(result.schema.valid).toBe(true); - expect(result.schema.notice).toContain('"ts" works'); + expect(result.schema.notices).toHaveLength(1); + expect(result.schema.notices[0]).toContain('"ts" works'); }); it("says nothing at all about the canonical spelling", async () => { @@ -499,7 +503,7 @@ describe("verifyRule", () => { runTests: false, }); expect(result.schema.valid).toBe(true); - expect(result.schema.notice).toBeUndefined(); + expect(result.schema.notices).toEqual([]); }); it("fails a TypeScript rule scoped only to .tsx files", async () => { @@ -529,7 +533,8 @@ describe("verifyRule", () => { runTests: false, }); expect(result.schema.valid).toBe(true); - expect(result.schema.notice).toContain("some globs name .tsx"); + expect(result.schema.notices).toHaveLength(1); + expect(result.schema.notices[0]).toContain("some globs name .tsx"); }); it("says nothing about globs that name no extension", async () => { @@ -542,7 +547,7 @@ describe("verifyRule", () => { runTests: false, }); expect(result.schema.valid).toBe(true); - expect(result.schema.notice).toBeUndefined(); + expect(result.schema.notices).toEqual([]); }); it("catches the mirror image: Tsx scoped only to .ts", async () => { @@ -587,7 +592,8 @@ describe("verifyRule", () => { runTests: false, }); expect(result.schema.valid).toBe(true); - expect(result.schema.notice).toContain("some globs name .tsx"); + expect(result.schema.notices).toHaveLength(1); + expect(result.schema.notices[0]).toContain("some globs name .tsx"); }); it("leaves a missing language to the required-fields layer", async () => {