From fce5eee4ae521b36ad149e93edd8af5f5c89a510 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 23 Sep 2026 11:36:50 -0700 Subject: [PATCH 1/8] refactor(cli): drop the unreachable notice fallback in vale dispatch The `unavailable` branch built its notice as `joinNotices([...advisories, outcome.message]) ?? outcome.message`. On that branch `outcome.message` is a `string`, so the list handed to the joiner is never empty and the joiner never returns `undefined`. The `??` arm could not run. It read as a safety net, which is worse than no net: the next person to touch the joiner would have reasoned about a fallback path that does not exist. --- packages/cli/src/rules/dispatch.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/rules/dispatch.ts b/packages/cli/src/rules/dispatch.ts index 2b9f8071..feb5829c 100644 --- a/packages/cli/src/rules/dispatch.ts +++ b/packages/cli/src/rules/dispatch.ts @@ -248,7 +248,10 @@ async function runValeEngine(options: DispatchOptions): Promise { return { engine: "vale", results: [], - notice: joinNotices([...advisories, outcome.message]) ?? outcome.message, + // No `?? outcome.message` fallback: this branch is `unavailable`, whose + // `message` is a `string`, so the list is never empty and the joiner always + // returns a value. The fallback read as a safety net and was dead code. + notice: joinNotices([...advisories, outcome.message]), }; } From 9cb76875c0176fb1212538c9a867b0376a57e3c6 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 23 Sep 2026 11:43:27 -0700 Subject: [PATCH 2/8] fix(cli): render every notice with its own marker, and carry notices as a list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check`'s text output printed `Notice: ` once per element of `DispatchResult.notices`, while the producers glued several advisories into one element with `"\n"`. A run with a schema advisory and a Vale diagnostic therefore printed the first behind a marker and the second as an unlabelled stray line. That is the same defect `241e1c4` fixed in `verify`, still live in `check`. Fixed twice over, because one fix alone leaves the trap set: - Both renderers now split a notice on `"\n"` and prefix every line. A single notice can legitimately span lines — Vale's stderr is passed through as written — so this is needed even once the list is flat. - The underlying field is a list. `EngineOutcome.notices`, `ValeRunOutcome`, `ValeVerifyResult`, `SchemaLayerResult`, `RuleVerification` and `RuleTestResult` all carry `string[]`, one notice per element, so `runEngines` concatenates into a genuinely flat `DispatchResult.notices` and `check --json` stops publishing array elements that are several notices in a trench coat. Presentation belongs to the renderer; a producer that picked its own separator could only mis-render. `rules/verify.ts` was the producer that proved the point: it joined `language.notices` with a SPACE, so an off-list `language:` spelling and a `files:` glob its language cannot parse arrived as one run-on sentence behind one marker. There is now no separator convention left for a producer to get wrong. `packages/cli/src/util/notices.ts` holds the one helper, `collectNotices`, as a leaf module with no imports of its own: having `dispatch.ts` import it from `vale/run.ts` would add an edge inside `src/rules/` between modules that already sit close to the cycle #388 fixed. It drops `undefined` entries, and also `""`, which no longer renders as a bare marker saying nothing. BREAKING: the published `notice?: string` field becomes `notices: string[]` in `verifyOutputSchema.schema`, `valeVerifyOutputSchema` and the `verify`/`test` envelope — so `verify --json` and `test --json` emit `notices`. Replaced rather than mirrored: keeping a joined `notice` alongside would preserve the separator convention this change exists to remove, and a consumer could not safely split it in the first place. Empty, never absent, as `violations` beside it already is. Refs #390 --- packages/cli/src/commands/check.ts | 9 ++- packages/cli/src/commands/verify.ts | 12 ++-- packages/cli/src/rules/dispatch.ts | 47 ++++++++------- packages/cli/src/rules/inspect.ts | 58 ++++++++++++------- packages/cli/src/rules/vale/run.ts | 45 ++++++++------ packages/cli/src/rules/vale/verify.ts | 11 ++-- packages/cli/src/rules/verify.ts | 22 ++++--- packages/cli/src/schemas/rules-verify.ts | 21 ++++--- packages/cli/src/schemas/verify-test.ts | 7 +-- packages/cli/src/util/notices.ts | 53 +++++++++++++++++ packages/cli/test/schemas-export.test.ts | 2 +- packages/cli/test/vale-formats.test.ts | 10 +++- packages/cli/test/vale-orchestration.test.ts | 24 +++++--- packages/cli/test/vale-run.test.ts | 19 ++++-- packages/cli/test/vale-verify.test.ts | 10 ++-- .../cli/test/verify-test-commands.test.ts | 36 +++++++----- packages/cli/test/verify.test.ts | 18 ++++-- 17 files changed, 268 insertions(+), 136 deletions(-) create mode 100644 packages/cli/src/util/notices.ts diff --git a/packages/cli/src/commands/check.ts b/packages/cli/src/commands/check.ts index fab1a094..f5077bf8 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -353,7 +353,14 @@ export const checkCommand = defineCommand({ }); const results = dispatched.results; - for (const notice of dispatched.notices) warn(`Notice: ${notice}`); + // One marker per notice, and one per line within a notice that spans + // lines. `dispatched.notices` is already flat — one element per notice + // — but a single notice can still be multi-line prose, Vale's stderr + // above all, and without this split only its first line was marked. + // The same defect `241e1c4` fixed in `verify`. + for (const notice of dispatched.notices) { + for (const line of notice.split("\n")) warn(`Notice: ${line}`); + } 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..9210ca74 100644 --- a/packages/cli/src/commands/verify.ts +++ b/packages/cli/src/commands/verify.ts @@ -160,11 +160,13 @@ 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")) { + // 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 notice.split("\n")) { console.log(` notice: ${line}`); } } diff --git a/packages/cli/src/rules/dispatch.ts b/packages/cli/src/rules/dispatch.ts index feb5829c..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,21 +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: [], - // No `?? outcome.message` fallback: this branch is `unavailable`, whose - // `message` is a `string`, so the list is never empty and the joiner always - // returns a value. The fallback read as a safety net and was dead code. - notice: joinNotices([...advisories, outcome.message]), + notices: collectNotices([...advisories, outcome.message]), }; } @@ -274,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: [] }; } /** @@ -330,6 +328,7 @@ export async function runEngines( return { engine, results: [], + notices: [], failure: `${engine} engine failed: ${ reason instanceof Error ? reason.message : String(reason) }`, @@ -341,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..24ea29b8 --- /dev/null +++ b/packages/cli/src/util/notices.ts @@ -0,0 +1,53 @@ +/** + * 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 !== "" + ); +} 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..7d50ee2c 100644 --- a/packages/cli/test/vale-orchestration.test.ts +++ b/packages/cli/test/vale-orchestration.test.ts @@ -361,6 +361,7 @@ describe("runEngines when Vale is unavailable", () => { matchedText: "simply", }, ], + notices: [], }); const cwd = makeMixedProject(); @@ -445,16 +446,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 +481,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); }); 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 () => { From 401d14e8d053236e5bc6ad31d72a5933dea430bf Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 23 Sep 2026 11:48:07 -0700 Subject: [PATCH 3/8] test(cli): pin the notices contract the four producers used to hold as copies Three gaps, all of them the reason the defect survived: - `collectNotices` gets its own tests: order preserved, `undefined` dropped, `""` dropped, empty input empty out, and a multi-line notice left as one element. - `check`'s TEXT output had no test at all for how it renders notices. The new one drives the built CLI over a project whose two Vale rules each draw a config advisory, and asserts each reaches its own `Notice: ` line. On the old code the two arrived joined and the second printed as an unmarked stray, so this fails there. - `check --json` now asserts the two are separate elements and that no element spans lines. The three separator-blind tests named in #390 asserted with `toContain` over the whole field, so a producer switching separator would have kept passing. They now assert on the elements. --- packages/cli/test/notices.test.ts | 56 +++++++++++ packages/cli/test/vale-orchestration.test.ts | 100 ++++++++++++++++++- 2 files changed, 151 insertions(+), 5 deletions(-) create mode 100644 packages/cli/test/notices.test.ts diff --git a/packages/cli/test/notices.test.ts b/packages/cli/test/notices.test.ts new file mode 100644 index 00000000..eac5d100 --- /dev/null +++ b/packages/cli/test/notices.test.ts @@ -0,0 +1,56 @@ +import { describe, expect, it } from "vitest"; + +import { collectNotices } 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", + ]); + }); +}); diff --git a/packages/cli/test/vale-orchestration.test.ts b/packages/cli/test/vale-orchestration.test.ts index 7d50ee2c..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, + }; } } @@ -574,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"); + } + }); +}); From af2e7183477a3c9a2ec56d44f10aeadff038fd75 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 23 Sep 2026 11:49:36 -0700 Subject: [PATCH 4/8] docs(openspec): write the notices contract into the standing specs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nine spec files mention "notice" and every one is about WHETHER one surfaces. Nothing said how several are separated, how they render, or what a machine consumer receives — so the four producers and two renderers agreed only by coincidence, and `check`'s renderer could ship the `241e1c4` defect without violating a requirement. Three ADDED requirements, one per capability that owns a producer or a renderer: - `cli-check` — one marker per notice, every line of a multi-line notice marked, and `--json` `notices` flat with one notice per element. - `cli-rule-validation` — `verify`/`test` carry notices as a list, one `notice:` marker each, and publish `notices` present-and-empty rather than absent. - `cli-vale-rule-engine` — independent advisories from one run stay separate notices; the engine picks no separator. ADDED rather than MODIFIED throughout: nothing standing describes this, so there is no requirement to restate, and a MODIFIED block against a requirement about a different question is how scenarios get dropped silently. Verified by dry-run archive and reset. Requirements 20/7/11 -> 21/8/12 and scenarios 55/37/37 -> 59/40/40, with no prior scenario missing from any of the three. --- .../.openspec.yaml | 2 + .../proposal.md | 74 +++++++++++++++++++ .../specs/cli-check/spec.md | 32 ++++++++ .../specs/cli-rule-validation/spec.md | 25 +++++++ .../specs/cli-vale-rule-engine/spec.md | 24 ++++++ .../2026-09-23-notice-list-contract/tasks.md | 38 ++++++++++ 6 files changed, 195 insertions(+) create mode 100644 openspec/changes/2026-09-23-notice-list-contract/.openspec.yaml create mode 100644 openspec/changes/2026-09-23-notice-list-contract/proposal.md create mode 100644 openspec/changes/2026-09-23-notice-list-contract/specs/cli-check/spec.md create mode 100644 openspec/changes/2026-09-23-notice-list-contract/specs/cli-rule-validation/spec.md create mode 100644 openspec/changes/2026-09-23-notice-list-contract/specs/cli-vale-rule-engine/spec.md create mode 100644 openspec/changes/2026-09-23-notice-list-contract/tasks.md diff --git a/openspec/changes/2026-09-23-notice-list-contract/.openspec.yaml b/openspec/changes/2026-09-23-notice-list-contract/.openspec.yaml new file mode 100644 index 00000000..265da3d9 --- /dev/null +++ b/openspec/changes/2026-09-23-notice-list-contract/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-09-23 diff --git a/openspec/changes/2026-09-23-notice-list-contract/proposal.md b/openspec/changes/2026-09-23-notice-list-contract/proposal.md new file mode 100644 index 00000000..c18d46a0 --- /dev/null +++ b/openspec/changes/2026-09-23-notice-list-contract/proposal.md @@ -0,0 +1,74 @@ +## 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, so the change is archived +here. diff --git a/openspec/changes/2026-09-23-notice-list-contract/specs/cli-check/spec.md b/openspec/changes/2026-09-23-notice-list-contract/specs/cli-check/spec.md new file mode 100644 index 00000000..5a78319c --- /dev/null +++ b/openspec/changes/2026-09-23-notice-list-contract/specs/cli-check/spec.md @@ -0,0 +1,32 @@ +## 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. + +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 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/2026-09-23-notice-list-contract/specs/cli-rule-validation/spec.md b/openspec/changes/2026-09-23-notice-list-contract/specs/cli-rule-validation/spec.md new file mode 100644 index 00000000..bb808b22 --- /dev/null +++ b/openspec/changes/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/2026-09-23-notice-list-contract/specs/cli-vale-rule-engine/spec.md b/openspec/changes/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/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/2026-09-23-notice-list-contract/tasks.md b/openspec/changes/2026-09-23-notice-list-contract/tasks.md new file mode 100644 index 00000000..64646654 --- /dev/null +++ b/openspec/changes/2026-09-23-notice-list-contract/tasks.md @@ -0,0 +1,38 @@ +## 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. + +## 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 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. From 939c7dc62ce353161c85dd2107e56430a4801082 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 23 Sep 2026 11:50:18 -0700 Subject: [PATCH 5/8] chore(changeset): note the check notice-rendering fix `patch`, and pre-1.0 settles it: the package is 0.11.2, where semver puts the public API outside the stability guarantee, so replacing a published field's shape does not earn more. The body says what a consumer crosses rather than leaning on the bump to say it. --- .changeset/notice-list-contract.md | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 .changeset/notice-list-contract.md diff --git a/.changeset/notice-list-contract.md b/.changeset/notice-list-contract.md new file mode 100644 index 00000000..2d1d87ca --- /dev/null +++ b/.changeset/notice-list-contract.md @@ -0,0 +1,9 @@ +--- +"@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. + +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. From a3e57acd758184f755f4eab70c0e31e10893d032 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 23 Sep 2026 12:14:42 -0700 Subject: [PATCH 6/8] fix(check): mark the runtime plan's notices too, through one shared renderer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check` printed the runtime plan's notices — what a repair restored, what it could not, and why — in their own loop with NO marker, while the dispatched notices got `Notice: `. Both end up in the same `--json` `notices` array, so the same message looked like two different kinds of thing depending on which list it arrived on, and the plan's notices are exactly the ones whose whole purpose is explaining a rule that did not run. All three sources (plan notices, skipped runtime rules, dispatched notices) now go through one `warnNotice`, so they cannot drift apart again. `markNotice` joins `collectNotices` in the leaf module and is shared by both commands, which differ only in the marker: `Notice: ` at the left margin for `check`, ` notice: ` indented under the rule for `verify`. That is a presentation choice, not a second contract, and having one helper is what stops `241e1c4` being fixed in one renderer and left live in the other — which is the exact history of this bug. Per-line marking matters here for a reason that is latent rather than live: no notice the CLI produces today spans lines, but a runtime repair notice embeds an `Error.message` it did not author, and Vale's stderr is passed through as written. `markNotice` is unit-tested on the multi-line case directly, since nothing reachable end to end exercises it yet. The `cli-check` delta gains a sentence and a scenario for the marked-alike rule. Re-verified by dry-run archive and reset: requirements 20/7/11 -> 21/8/12 and scenarios 55/37/37 -> 60/40/40, no prior scenario missing. --- .../specs/cli-check/spec.md | 7 +++ .../2026-09-23-notice-list-contract/tasks.md | 9 +++- packages/cli/src/commands/check.ts | 38 ++++++++++----- packages/cli/src/commands/verify.ts | 5 +- packages/cli/src/util/notices.ts | 25 ++++++++++ packages/cli/test/notices.test.ts | 47 ++++++++++++++++++- packages/cli/test/runtime-check.test.ts | 13 ++++- 7 files changed, 128 insertions(+), 16 deletions(-) diff --git a/openspec/changes/2026-09-23-notice-list-contract/specs/cli-check/spec.md b/openspec/changes/2026-09-23-notice-list-contract/specs/cli-check/spec.md index 5a78319c..8e9693ea 100644 --- a/openspec/changes/2026-09-23-notice-list-contract/specs/cli-check/spec.md +++ b/openspec/changes/2026-09-23-notice-list-contract/specs/cli-check/spec.md @@ -6,6 +6,8 @@ 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. @@ -20,6 +22,11 @@ A notice SHALL NOT affect the exit code. - **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 diff --git a/openspec/changes/2026-09-23-notice-list-contract/tasks.md b/openspec/changes/2026-09-23-notice-list-contract/tasks.md index 64646654..78e93cd8 100644 --- a/openspec/changes/2026-09-23-notice-list-contract/tasks.md +++ b/openspec/changes/2026-09-23-notice-list-contract/tasks.md @@ -17,6 +17,11 @@ - [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 @@ -26,7 +31,9 @@ advisories, two `Notice: ` lines. - [x] 2.3 Assert `check --json` publishes them as separate elements, none spanning lines. -- [x] 2.4 Strengthen the three separator-blind tests to assert on elements +- [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 diff --git a/packages/cli/src/commands/check.ts b/packages/cli/src/commands/check.ts index f5077bf8..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,14 +376,7 @@ export const checkCommand = defineCommand({ }); const results = dispatched.results; - // One marker per notice, and one per line within a notice that spans - // lines. `dispatched.notices` is already flat — one element per notice - // — but a single notice can still be multi-line prose, Vale's stderr - // above all, and without this split only its first line was marked. - // The same defect `241e1c4` fixed in `verify`. - for (const notice of dispatched.notices) { - for (const line of notice.split("\n")) warn(`Notice: ${line}`); - } + 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 9210ca74..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`. @@ -166,8 +167,8 @@ async function runOverPath(options: { // 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 notice.split("\n")) { - console.log(` notice: ${line}`); + for (const line of markNotice(notice, " notice: ")) { + console.log(line); } } } diff --git a/packages/cli/src/util/notices.ts b/packages/cli/src/util/notices.ts index 24ea29b8..b5f14e78 100644 --- a/packages/cli/src/util/notices.ts +++ b/packages/cli/src/util/notices.ts @@ -51,3 +51,28 @@ export function collectNotices( (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 index eac5d100..42edaf2d 100644 --- a/packages/cli/test/notices.test.ts +++ b/packages/cli/test/notices.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from "vitest"; -import { collectNotices } from "../src/util/notices"; +import { collectNotices, markNotice } from "../src/util/notices"; /** * The one place the `notices` contract is decided, so it is the one place the @@ -54,3 +54,48 @@ describe("collectNotices", () => { ]); }); }); + +/** + * 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 }); From ad8b1ce43f0311953d95998f399bcaf86be0682f Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 23 Sep 2026 12:51:31 -0700 Subject: [PATCH 7/8] chore(changeset): record the runtime-plan notice fix in the release note The paragraph was written but never staged: the amend it was meant to ride on reported no staged files and went through with the earlier tree, so the shipped note described only the dispatched-notice half of a fix that has two. --- .changeset/notice-list-contract.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.changeset/notice-list-contract.md b/.changeset/notice-list-contract.md index 2d1d87ca..e34c64d1 100644 --- a/.changeset/notice-list-contract.md +++ b/.changeset/notice-list-contract.md @@ -4,6 +4,8 @@ `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. From 48afe1ae8aaa95a874e0b9e752bebf77f3299b87 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 23 Sep 2026 12:58:39 -0700 Subject: [PATCH 8/8] docs(openspec): archive the notices contract onto the standing specs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR is the tip — no open PR is based on this branch — so the change archives here 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. The real archive matches the dry run exactly, which is the check that matters: an archive REPLACES each requirement it names, so a delta that quietly drops a scenario leaves no trace and `validate --strict` still passes. cli-check 20 -> 21 requirements, 55 -> 60 scenarios cli-rule-validation 7 -> 8 requirements, 37 -> 40 scenarios cli-vale-rule-engine 11 -> 12 requirements, 37 -> 40 scenarios Zero prior scenarios missing in any of the three, compared set-wise rather than by count so a drop masked by an addition could not hide. The proposal's delivery-shape note said the change "is archived here" while tasks.md recorded only the dry run, which read as a contradiction. Both now say the same thing, and say why the tip is where it happens. --- .../.openspec.yaml | 0 .../proposal.md | 9 ++++- .../specs/cli-check/spec.md | 0 .../specs/cli-rule-validation/spec.md | 0 .../specs/cli-vale-rule-engine/spec.md | 0 .../2026-09-23-notice-list-contract/tasks.md | 4 ++ openspec/specs/cli-check/spec.md | 38 +++++++++++++++++++ openspec/specs/cli-rule-validation/spec.md | 24 ++++++++++++ openspec/specs/cli-vale-rule-engine/spec.md | 23 +++++++++++ 9 files changed, 96 insertions(+), 2 deletions(-) rename openspec/changes/{ => archive}/2026-09-23-notice-list-contract/.openspec.yaml (100%) rename openspec/changes/{ => archive}/2026-09-23-notice-list-contract/proposal.md (90%) rename openspec/changes/{ => archive}/2026-09-23-notice-list-contract/specs/cli-check/spec.md (100%) rename openspec/changes/{ => archive}/2026-09-23-notice-list-contract/specs/cli-rule-validation/spec.md (100%) rename openspec/changes/{ => archive}/2026-09-23-notice-list-contract/specs/cli-vale-rule-engine/spec.md (100%) rename openspec/changes/{ => archive}/2026-09-23-notice-list-contract/tasks.md (88%) diff --git a/openspec/changes/2026-09-23-notice-list-contract/.openspec.yaml b/openspec/changes/archive/2026-09-23-notice-list-contract/.openspec.yaml similarity index 100% rename from openspec/changes/2026-09-23-notice-list-contract/.openspec.yaml rename to openspec/changes/archive/2026-09-23-notice-list-contract/.openspec.yaml diff --git a/openspec/changes/2026-09-23-notice-list-contract/proposal.md b/openspec/changes/archive/2026-09-23-notice-list-contract/proposal.md similarity index 90% rename from openspec/changes/2026-09-23-notice-list-contract/proposal.md rename to openspec/changes/archive/2026-09-23-notice-list-contract/proposal.md index c18d46a0..13e77b28 100644 --- a/openspec/changes/2026-09-23-notice-list-contract/proposal.md +++ b/openspec/changes/archive/2026-09-23-notice-list-contract/proposal.md @@ -70,5 +70,10 @@ consumer crosses. **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, so the change is archived -here. +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/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 similarity index 100% rename from openspec/changes/2026-09-23-notice-list-contract/specs/cli-check/spec.md rename to openspec/changes/archive/2026-09-23-notice-list-contract/specs/cli-check/spec.md diff --git a/openspec/changes/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 similarity index 100% rename from openspec/changes/2026-09-23-notice-list-contract/specs/cli-rule-validation/spec.md rename to openspec/changes/archive/2026-09-23-notice-list-contract/specs/cli-rule-validation/spec.md diff --git a/openspec/changes/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 similarity index 100% rename from openspec/changes/2026-09-23-notice-list-contract/specs/cli-vale-rule-engine/spec.md rename to openspec/changes/archive/2026-09-23-notice-list-contract/specs/cli-vale-rule-engine/spec.md diff --git a/openspec/changes/2026-09-23-notice-list-contract/tasks.md b/openspec/changes/archive/2026-09-23-notice-list-contract/tasks.md similarity index 88% rename from openspec/changes/2026-09-23-notice-list-contract/tasks.md rename to openspec/changes/archive/2026-09-23-notice-list-contract/tasks.md index 78e93cd8..ba5fee1c 100644 --- a/openspec/changes/2026-09-23-notice-list-contract/tasks.md +++ b/openspec/changes/archive/2026-09-23-notice-list-contract/tasks.md @@ -43,3 +43,7 @@ - [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