From 24027db320763873a82d7a0fe07fd199ecb4257e Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 21 Sep 2026 11:44:23 -0700 Subject: [PATCH 1/5] feat(vale): refuse the Vale run on a rejected .vale.ini assembleValeConfig validates every rule's config against the config schema and, on any rejection, returns a refusal naming each rule and line without writing .taskless/.vale.ini. ruleConfigBody and sectionPatternsOf are gone: an accepted config is written verbatim under its breadcrumb, and sections come from the parsed structure. dispatch reports a refusal as the Vale engine's failure, so it reaches the exit code while ast-grep still runs; the schema's advisories join the notices. The mixed-engine e2e test that expected check to surface W101 at run time now asserts the refusal, and the run-time notice stays pinned in vale-run.test.ts through a hand-written config that bypasses the schema. enforcedBy stays "verify" for the vale-config-* constraints: every one is decidable from the files alone, which is the ordering question the field answers; check refuses and points back at verify. --- openspec/changes/vale-config-schema/tasks.md | 4 +- packages/cli/src/commands/check.ts | 2 +- packages/cli/src/rules/assemble.ts | 145 ++++++++++------- packages/cli/src/rules/constraints.ts | 7 + packages/cli/src/rules/dispatch.ts | 86 ++++++++-- packages/cli/test/assemble.test.ts | 149 +++++++++++++++--- packages/cli/test/mixed-engine-check.test.ts | 157 +++++++++++++------ packages/cli/test/vale-orchestration.test.ts | 18 +-- packages/cli/test/vale-run.test.ts | 6 + 9 files changed, 419 insertions(+), 155 deletions(-) diff --git a/openspec/changes/vale-config-schema/tasks.md b/openspec/changes/vale-config-schema/tasks.md index 765b5b62..44974543 100644 --- a/openspec/changes/vale-config-schema/tasks.md +++ b/openspec/changes/vale-config-schema/tasks.md @@ -10,8 +10,8 @@ ## 2. Slice 2: assembly refusal, dispatch, recipe, ledger (tip) -- [ ] 2.1 In `rules/assemble.ts`, delete `ruleConfigBody` and `sectionPatternsOf`; `assembleValeConfig` validates each config, and on any rejection returns a refusal naming the rule and line without writing the file. Accepted configs are written verbatim under their breadcrumb; `sections` comes from the AST. Verify the "Assembly order is stable" test still asserts byte-identical output, and a new test asserts a foreign key leaves `.taskless/.vale.ini` unwritten. -- [ ] 2.2 In `rules/dispatch.ts`, a refusal becomes the Vale engine's `failure` (reaches the exit code); advisories join `notices`. Verify a mixed-engine test shows ast-grep results alongside the Vale failure and a non-zero exit. +- [x] 2.1 In `rules/assemble.ts`, delete `ruleConfigBody` and `sectionPatternsOf`; `assembleValeConfig` validates each config, and on any rejection returns a refusal naming the rule and line without writing the file. Accepted configs are written verbatim under their breadcrumb; `sections` comes from the AST. Verify the "Assembly order is stable" test still asserts byte-identical output, and a new test asserts a foreign key leaves `.taskless/.vale.ini` unwritten. +- [x] 2.2 In `rules/dispatch.ts`, a refusal becomes the Vale engine's `failure` (reaches the exit code); advisories join `notices`. Verify a mixed-engine test shows ast-grep results alongside the Vale failure and a non-zero exit. - [ ] 2.3 Recipe `packages/cli/src/agent/create-vale-rule.md`: state that the config is schema-checked, list what is rejected and what is advised, drop any `.taskless/**` matcher from its examples, and bump the topic version. Verify `pnpm build && pnpm cli agent create-vale-rule` renders and `pnpm cli check` is clean over the prose. - [ ] 2.4 `packages/cli/src/agent/update.md`: 0.11.3 ledger entry — a config `check` used to tolerate now refuses the Vale run, with the `verify` command that names the line; bump the topic version. - [ ] 2.5 Extend the changeset with the refusal. Run `pnpm typecheck`, `pnpm lint`, `pnpm --filter @taskless/cli test`; open PR 2 against PR 1's branch. diff --git a/packages/cli/src/commands/check.ts b/packages/cli/src/commands/check.ts index da74339f..14cb984b 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -258,7 +258,7 @@ export const checkCommand = defineCommand({ cwd, paths: existingPaths, astGrepConfigPath: assembled.sg, - valeConfigPath: assembled.vale?.path, + vale: assembled.vale, runtimeRules: plan.execute, runtimeTimeoutMs: parseTimeoutMs(args.timeout), }); diff --git a/packages/cli/src/rules/assemble.ts b/packages/cli/src/rules/assemble.ts index 240e8eba..1ed94238 100644 --- a/packages/cli/src/rules/assemble.ts +++ b/packages/cli/src/rules/assemble.ts @@ -8,6 +8,8 @@ import { ruleConfigPath, ruleTestsDirectory, } from "./engines"; +import { validateValeRuleConfig } from "../schemas/vale-config"; +import type { RuleViolation } from "./constraints"; import { RULE_TESTS_DIRECTORY, RULES_DIRECTORY } from "./layout"; /** @@ -71,56 +73,39 @@ function valeHeader(): string { } /** - * Read a rule's own config, dropping a leading `StylesPath`/`MinAlertLevel` if - * an author copied one in. + * A rule's assembled block: its breadcrumb comment, then its config verbatim. * - * Those two are properties of the run, not of a rule, and a per-rule copy would - * either be redundant or silently fight the header. Everything else — matchers, - * assignments, `tskl)` breadcrumbs, comments — is carried through **verbatim**, - * because matcher order inside a rule is the author's expression of precedence. + * The config is the generator's structured input and is never re-serialized. + * The schema has already read it as an AST, for validation and for the matcher + * list; what Vale reads is the file's own bytes, so matcher order, comments and + * spacing survive exactly as the author wrote them. Matcher order inside a rule + * is the author's expression of precedence, so anything short of verbatim + * would be a semantic edit. The only addition is a newline after a source that + * lacks one, so the next rule's breadcrumb starts its own line. + * + * This is also why the schema, not this module, turns away a copied-in + * `StylesPath` or `MinAlertLevel`. Assembly used to strip those two by + * splitting each line on `=`; that was the one string edit it performed, and + * it meant the file Vale read was not the file the author wrote. A run-level + * key above the first matcher is now a rejection, so there is nothing left to + * strip. */ -function ruleConfigBody(source: string): string { - const lines = source.split("\n"); - const kept: string[] = []; - let seenSection = false; - for (const line of lines) { - if (line.trimStart().startsWith("[")) seenSection = true; - if (!seenSection) { - const key = line.split("=")[0]?.trim().toLowerCase(); - if (key === "stylespath" || key === "minalertlevel") continue; - } - kept.push(line); - } - return kept.join("\n").trim(); +function valeRuleBlock(ruleId: string, source: string): string { + const terminated = source.endsWith("\n") ? source : `${source}\n`; + return `# tskl) rule = ${ruleId}\n${terminated}`; } -/** A rule's assembled block, tagged so its provenance survives interleaving. */ -function valeRuleBlock(ruleId: string, body: string): string { - return [`# tskl) rule = ${ruleId}`, body, ""].join("\n"); +/** One rule whose config the schema refused. */ +export interface ValeConfigRefusal { + ruleId: string; + /** Every rejection, each naming the line and the constraint it broke. */ + rejections: RuleViolation[]; } /** - * A section header (`[pattern]`) from an assembled rule's own body. - * - * Read from the exact string this module is about to write — never from the - * file after writing it. Re-reading the written `.vale.ini` to recover its - * own sections would be the mistake `STYLEGUIDE-CODE.md`'s "Verify Build - * Output In The Build, Not By Parsing It" warns against: this function - * already IS the generator, holding the structured pieces before they are - * joined into text, so there is nothing to re-derive. - */ -function sectionPatternsOf(body: string): string[] { - const patterns: string[] = []; - for (const line of body.split("\n")) { - const match = /^\[(.+)\]$/.exec(line.trim()); - if (match?.[1] !== undefined) patterns.push(match[1]); - } - return patterns; -} - -/** - * What `assembleValeConfig` produced: where to point `--config`, and the - * section patterns it wrote there. + * What `assembleValeConfig` produced when every config passed the schema: + * where to point `--config`, the section patterns it wrote there, and what the + * schema noted about the configs without rejecting them. * * `sections` exists so a caller that needs to know what Vale would actually * lint can ask this module directly instead of re-parsing the config it just @@ -131,30 +116,64 @@ function sectionPatternsOf(body: string): string[] { * the file to get it. */ export interface AssembledValeConfig { + status: "ok"; /** Config path relative to the project root, for `--config`. */ path: string; /** * Every section glob pattern written into the config, deduplicated and * sorted for a stable read order. Root-relative, exactly as Vale reads - * them — the same strings a `[…]` line in a rule's own `.vale.ini` names. + * them — the same strings a `[…]` line in a rule's own `.vale.ini` names, + * read from the parsed structure rather than from the text. */ sections: string[]; + /** + * What the schema said about the configs without refusing them: a repeated + * key, a `[*]` matcher, a `.taskless/**` matcher. Advisory, so the caller + * surfaces them as notices and they never touch the exit code. + */ + advisories: string[]; } +/** + * What `assembleValeConfig` produced when a config was refused: nothing on + * disk, and every rule it refused with the rejections the schema raised. + * + * Refusing rather than omitting the rule is the point. A rule left out of the + * assembled config would verify, run, and report nothing, which is the silent + * disable this engine's design exists to prevent. Refusing rather than + * stripping the offending line is the same decision from the other side: a + * config the schema rejects is one Vale would have read as something other + * than what its author wrote. + */ +export interface RefusedValeConfig { + status: "refused"; + /** Every rule refused, in id order. Never empty. */ + refusals: ValeConfigRefusal[]; +} + +export type ValeAssembly = AssembledValeConfig | RefusedValeConfig; + /** * Assemble `.taskless/.vale.ini` from every Vale rule's own config. * - * Returns the config path and its section patterns, or `undefined` when no - * Vale rule declares any config — there is nothing to run, and writing an empty - * config would invite Vale to lint the project against no rules and report a - * clean pass. + * Every config is validated against the config schema first. On any rejection + * the file is not written and the refusal names each rule and line, so the + * caller can report it as the Vale engine's failure. Otherwise the header and + * each rule's verbatim config are written, and the result carries the config + * path, its section patterns, and the schema's advisories. + * + * Returns `undefined` when no Vale rule declares any config — there is nothing + * to run, and writing an empty config would invite Vale to lint the project + * against no rules and report a clean pass. */ export async function assembleValeConfig( cwd: string -): Promise { +): Promise { const ruleIds = await listRuleIds(cwd, "vale"); const blocks: string[] = []; const sections = new Set(); + const advisories: string[] = []; + const refusals: ValeConfigRefusal[] = []; for (const ruleId of ruleIds) { const configPath = ruleConfigPath(cwd, "vale", ruleId); @@ -168,19 +187,29 @@ export async function assembleValeConfig( // `verify` reports it. continue; } - const body = ruleConfigBody(source); - if (body === "") continue; - for (const pattern of sectionPatternsOf(body)) sections.add(pattern); - blocks.push(valeRuleBlock(ruleId, body)); + const verdict = validateValeRuleConfig(ruleId, source); + if (verdict.rejections.length > 0) { + refusals.push({ ruleId, rejections: verdict.rejections }); + continue; + } + for (const pattern of verdict.sections) sections.add(pattern); + advisories.push(...verdict.advisories); + blocks.push(valeRuleBlock(ruleId, source)); } + if (refusals.length > 0) return { status: "refused", refusals }; if (blocks.length === 0) return undefined; const contents = [valeHeader(), ...blocks].join("\n"); const target = join(cwd, ASSEMBLED_VALE_CONFIG); await mkdir(dirname(target), { recursive: true }); await writeFile(target, contents, "utf8"); - return { path: ASSEMBLED_VALE_CONFIG, sections: [...sections].toSorted() }; + return { + status: "ok", + path: ASSEMBLED_VALE_CONFIG, + sections: [...sections].toSorted(), + advisories, + }; } /** @@ -239,8 +268,11 @@ export async function assembleSgConfig( /** Both assembled configs, for a run that needs whichever engines are present. */ export interface AssembledConfigs { - /** Vale's config and section patterns, or `undefined` when no Vale rule is configured. */ - vale: AssembledValeConfig | undefined; + /** + * Vale's config and section patterns, the refusal that kept it unwritten, + * or `undefined` when no Vale rule is configured. + */ + vale: ValeAssembly | undefined; /** `-c` for ast-grep, or `undefined` when there are no ast-grep rules. */ sg: string | undefined; } @@ -257,7 +289,6 @@ export async function assembleEngineConfigs( /** Exported for the assembly tests, which assert on the artifact directly. */ export const ASSEMBLY_INTERNALS = { - ruleConfigBody, valeHeader, RULE_TESTS_DIRECTORY, }; diff --git a/packages/cli/src/rules/constraints.ts b/packages/cli/src/rules/constraints.ts index 93b62016..b386863e 100644 --- a/packages/cli/src/rules/constraints.ts +++ b/packages/cli/src/rules/constraints.ts @@ -134,6 +134,13 @@ export const RULE_CONSTRAINTS = [ // never over the file's text. The common thread is that Vale accepts each of // these configs and does something other than what the author wrote, with a // zero exit and an empty report: the rule verifies, runs, and finds nothing. + // + // `enforcedBy` reads `verify` for all of them even though `check` runs the + // same schema and refuses the Vale run on a rejection. The field answers a + // consumer's ordering question — can this be decided from the files alone, + // before anything executes? — and for every entry here it can. `verify` is + // where the author is told which line and which constraint; `check` only + // refuses, and points back at `verify`. { id: "vale-config-no-root-keys", engine: "vale", diff --git a/packages/cli/src/rules/dispatch.ts b/packages/cli/src/rules/dispatch.ts index c83ea824..633b65e2 100644 --- a/packages/cli/src/rules/dispatch.ts +++ b/packages/cli/src/rules/dispatch.ts @@ -1,5 +1,6 @@ import { readdir } from "node:fs/promises"; import type { CheckResult } from "../types/check"; +import type { RefusedValeConfig, ValeAssembly } from "./assemble"; import { engineRulesDirectory } from "./engines"; import { type EngineName } from "./layout"; import { isMissingDirectory } from "./errno"; @@ -81,16 +82,21 @@ export interface DispatchOptions { */ astGrepConfigPath: string | undefined; /** - * The assembled Vale `--config` path, or `undefined` when assembly produced - * nothing to run. + * What Vale assembly produced: the `--config` path with its advisories, a + * refusal because a rule's config broke the schema, or `undefined` when + * assembly produced nothing to run. * * Distinct from "the project has a Vale rules directory". A rule directory can * exist while every rule in it declares no config, and assembly then writes no * file and deletes none — so gating on the directory alone ran Vale against a * config left behind by a previous run, or against a path that was never * written. The config is the only honest signal that there is Vale work. + * + * A refusal arrives here rather than being resolved by the caller because it + * is the Vale engine's failure: the engine was present, had rules to run, + * and could not, which has to reach the exit code the same way a crash does. */ - valeConfigPath: string | undefined; + vale: ValeAssembly | undefined; /** Runtime rules that survived planning. Empty means the harness is skipped. */ runtimeRules: RuntimeRule[]; runtimeTimeoutMs?: number; @@ -153,7 +159,8 @@ async function runAstGrepEngine( * and failing there would make `check` unrunnable on a machine where the other * engines work; a timeout or a crash is a failure, because Vale was present and * asked to work, and reporting that as a skip lets a broken rule file read as - * "no Vale findings". + * "no Vale findings". A refused assembly sits on the failure side for the same + * reason, and is decided before the binary is even looked for. * * Reading the severity off the outcome rather than asking a helper is the point * of that field: an engine reports how bad its own trouble is, and a caller @@ -165,9 +172,24 @@ async function runValeEngine(options: DispatchOptions): Promise { // work, and there is nothing to point Vale at. Checked before the directory // gate because it is the stronger claim — a rules directory can be present // while assembly yields nothing. - if (options.valeConfigPath === undefined) { + if (options.vale === undefined) { return { engine: "vale", results: [] }; } + // 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 + // read as something other than what its author wrote, so there is no config + // to run. Leaving that rule out and running the rest would have it verify, + // run, and report nothing, which is the silent disable this engine exists + // to prevent; downgrading it to a notice would let a broken engine exit + // zero. The rejection messages already name the rule and the line, so they + // are carried as written rather than paraphrased. + if (options.vale.status === "refused") { + return { + engine: "vale", + results: [], + failure: describeValeRefusal(options.vale), + }; + } if (!(await hasValeRules(options.cwd))) { return { engine: "vale", results: [] }; } @@ -175,24 +197,60 @@ async function runValeEngine(options: DispatchOptions): Promise { const outcome = await runVale({ cwd: options.cwd, paths: options.paths, - configPath: options.valeConfigPath, + configPath: options.vale.path, timeoutMs: options.valeTimeoutMs, }); + // What the schema said about the configs without refusing them travels with + // whatever Vale itself has to say, both advisory. + const advisories = options.vale.advisories; + if (outcome.status === "ok") { - // A zero-exit run that still wrote to stderr carries a diagnostic — most - // often a rule assignment Vale ignored for sitting outside a section. 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. + // A zero-exit run that still wrote to stderr carries a diagnostic the + // schema could not foresee: Vale's own warning about a style file, a + // 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, - ...(outcome.notice === undefined ? {} : { notice: outcome.notice }), + ...(notice === undefined ? {} : { notice }), }; } - return outcome.blocking - ? { engine: "vale", results: [], failure: outcome.message } - : { engine: "vale", results: [], notice: outcome.message }; + if (outcome.blocking) { + return { engine: "vale", results: [], failure: outcome.message }; + } + return { + engine: "vale", + results: [], + notice: joinNotices([...advisories, outcome.message]) ?? outcome.message, + }; +} + +/** + * The Vale engine's failure message for a refused assembly. + * + * One message, because an outcome carries one `failure`, but every refused + * rule and every rejection is in it: an author with two broken configs should + * not fix one, re-run, and discover the other. + */ +function describeValeRefusal(refused: RefusedValeConfig): string { + const rules = refused.refusals.map((refusal) => refusal.ruleId).join(", "); + const lines = refused.refusals.flatMap((refusal) => + refusal.rejections.map((rejection) => rejection.message) + ); + return [ + `Vale did not run: the config of ${rules} was rejected by the config schema. ` + + `Run \`taskless verify\` for the constraint behind each line.`, + ...lines, + ].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. */ diff --git a/packages/cli/test/assemble.test.ts b/packages/cli/test/assemble.test.ts index 5da1cae3..b1dbf529 100644 --- a/packages/cli/test/assemble.test.ts +++ b/packages/cli/test/assemble.test.ts @@ -10,11 +10,25 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; -import { assembleSgConfig, assembleValeConfig } from "../src/rules/assemble"; +import { + assembleSgConfig, + assembleValeConfig, + type ValeAssembly, +} from "../src/rules/assemble"; import { ruleDirectory, ruleTestsDirectory } from "../src/rules/engines"; let cwd: string; +/** The written config's path, failing loudly when assembly did not write one. */ +function okPath(assembled: ValeAssembly | undefined): string { + if (assembled?.status !== "ok") { + throw new Error( + `expected an assembled config, got ${JSON.stringify(assembled)}` + ); + } + return assembled.path; +} + beforeEach(async () => { cwd = await mkdtemp(join(tmpdir(), "tskl-assemble-")); }); @@ -57,9 +71,12 @@ async function sgRuleWithoutTests(id: string): Promise { describe("Vale config assembly", () => { it("writes a header naming the Vale rules tree as StylesPath", async () => { - await valeRule("no-simply", "[*.md]\nno-simply.no-simply = YES\n"); + await valeRule( + "no-simply", + "[*.md]\ntskl) rule = no-simply\nno-simply.no-simply = YES\n" + ); const assembled = await assembleValeConfig(cwd); - const contents = await readFile(join(cwd, assembled?.path ?? ""), "utf8"); + const contents = await readFile(join(cwd, okPath(assembled)), "utf8"); // StylesPath is what makes `/.yml` resolve as check `.`. // Under `.` it resolves to nothing at all, so this line is the difference @@ -72,24 +89,24 @@ describe("Vale config assembly", () => { // directory iteration would give a rule a different effective scope // depending on the machine it ran on. it("emits rules in sorted id order", async () => { - await valeRule("zebra", "[*.md]\nzebra.zebra = YES\n"); - await valeRule("alpha", "[*.md]\nalpha.alpha = YES\n"); + await valeRule("zebra", "[*.md]\ntskl) rule = zebra\nzebra.zebra = YES\n"); + await valeRule("alpha", "[*.md]\ntskl) rule = alpha\nalpha.alpha = YES\n"); const assembled = await assembleValeConfig(cwd); - const contents = await readFile(join(cwd, assembled?.path ?? ""), "utf8"); + const contents = await readFile(join(cwd, okPath(assembled)), "utf8"); expect(contents.indexOf("alpha.alpha")).toBeLessThan( contents.indexOf("zebra.zebra") ); }); it("is byte-identical across runs", async () => { - await valeRule("one", "[*.md]\none.one = YES\n"); - await valeRule("two", "[docs/**]\ntwo.two = YES\n"); + await valeRule("one", "[*.md]\ntskl) rule = one\none.one = YES\n"); + await valeRule("two", "[docs/**]\ntskl) rule = two\ntwo.two = YES\n"); const first = await assembleValeConfig(cwd); - const a = await readFile(join(cwd, first?.path ?? ""), "utf8"); + const a = await readFile(join(cwd, okPath(first)), "utf8"); const second = await assembleValeConfig(cwd); - const b = await readFile(join(cwd, second?.path ?? ""), "utf8"); + const b = await readFile(join(cwd, okPath(second)), "utf8"); expect(a).toBe(b); }); @@ -99,34 +116,115 @@ describe("Vale config assembly", () => { it("preserves each rule's own matcher order", async () => { await valeRule( "scoped", - "[marketing/**]\nscoped.scoped = YES\n\n[marketing/legacy/**]\nscoped.scoped = NO\n" + "[marketing/**]\ntskl) rule = scoped\nscoped.scoped = YES\n\n[marketing/legacy/**]\ntskl) rule = scoped\nscoped.scoped = NO\n" ); const assembled = await assembleValeConfig(cwd); - const contents = await readFile(join(cwd, assembled?.path ?? ""), "utf8"); + const contents = await readFile(join(cwd, okPath(assembled)), "utf8"); expect(contents.indexOf("[marketing/**]")).toBeLessThan( contents.indexOf("[marketing/legacy/**]") ); }); it("tags each block with the rule it came from", async () => { - await valeRule("no-simply", "[*.md]\nno-simply.no-simply = YES\n"); + await valeRule( + "no-simply", + "[*.md]\ntskl) rule = no-simply\nno-simply.no-simply = YES\n" + ); const assembled = await assembleValeConfig(cwd); - const contents = await readFile(join(cwd, assembled?.path ?? ""), "utf8"); + const contents = await readFile(join(cwd, okPath(assembled)), "utf8"); // Provenance is otherwise lost the moment two rules' matchers interleave. expect(contents).toContain("tskl) rule = no-simply"); }); // A per-rule StylesPath would either duplicate the header or silently fight - // it, and it is a property of the run rather than of a rule. - it("drops a StylesPath an author copied into a rule config", async () => { + // it, and it is a property of the run rather than of a rule. Assembly used + // to strip it; now the schema rejects it, and assembly refuses rather than + // edits, because the file Vale reads has to be the file the author wrote. + it("refuses a StylesPath an author copied into a rule config", async () => { await valeRule( "no-simply", - "StylesPath = .\nMinAlertLevel = error\n\n[*.md]\nno-simply.no-simply = YES\n" + "StylesPath = .\nMinAlertLevel = error\n\n[*.md]\ntskl) rule = no-simply\nno-simply.no-simply = YES\n" ); const assembled = await assembleValeConfig(cwd); - const contents = await readFile(join(cwd, assembled?.path ?? ""), "utf8"); - expect(contents).not.toContain("StylesPath = ."); - expect(contents).not.toContain("MinAlertLevel = error"); + expect(assembled?.status).toBe("refused"); + if (assembled?.status !== "refused") return; + expect(assembled.refusals.map((refusal) => refusal.ruleId)).toEqual([ + "no-simply", + ]); + expect( + assembled.refusals[0]?.rejections.map((r) => r.constraintId) + ).toContain("vale-config-no-root-keys"); + expect(assembled.refusals[0]?.rejections[0]?.message).toContain( + "no-simply/.vale.ini line 1:" + ); + }); + + // Refused means refused: nothing on disk. A stale config left behind from an + // earlier run would let Vale lint against yesterday's rules and report them + // as today's. + it("leaves the assembled config unwritten when a rule assigns a foreign key", async () => { + await valeRule( + "no-hedging", + "[*.md]\ntskl) rule = no-hedging\nno-hedging.no-hedging = YES\n" + ); + await valeRule( + "no-simply", + "[*.md]\ntskl) rule = no-simply\nno-simply.no-simply = YES\nno-hedging.no-hedging = NO\n" + ); + const assembled = await assembleValeConfig(cwd); + expect(assembled?.status).toBe("refused"); + if (assembled?.status !== "refused") return; + // Only the rule that broke the schema is named; its neighbour is fine. + expect(assembled.refusals.map((refusal) => refusal.ruleId)).toEqual([ + "no-simply", + ]); + expect(assembled.refusals[0]?.rejections[0]?.constraintId).toBe( + "vale-config-own-key-only" + ); + expect(assembled.refusals[0]?.rejections[0]?.message).toContain("line 4"); + await expect(stat(join(cwd, ".taskless", ".vale.ini"))).rejects.toThrow(); + }); + + // Every refused rule is reported at once. An author with two broken configs + // should not fix one, re-run, and only then hear about the other. + it("names every refused rule, not just the first", async () => { + await valeRule("alpha", "alpha.alpha = YES\n"); + await valeRule("beta", "[*.md]\ntskl) rule = beta\nbeta.beta = NO\n"); + const assembled = await assembleValeConfig(cwd); + expect(assembled?.status).toBe("refused"); + if (assembled?.status !== "refused") return; + expect(assembled.refusals.map((refusal) => refusal.ruleId)).toEqual([ + "alpha", + "beta", + ]); + }); + + // The config is the generator's structured input and is never re-serialized: + // the schema reads it as an AST, Vale reads the author's own bytes. + it("writes an accepted config verbatim under its breadcrumb", async () => { + const source = + "# scope\n[docs/**]\ntskl) rule = no-simply\nBasedOnStyles =\n\nno-simply.no-simply = YES \n"; + await valeRule("no-simply", source); + const assembled = await assembleValeConfig(cwd); + expect(assembled?.status).toBe("ok"); + if (assembled?.status !== "ok") return; + const contents = await readFile(join(cwd, assembled.path), "utf8"); + expect(contents).toContain(`# tskl) rule = no-simply\n${source}`); + }); + + // What the schema notes without refusing rides on the result, so `check` can + // say it without the exit code hearing it. + it("carries the schema's advisories", async () => { + await valeRule( + "no-simply", + "[*.md]\ntskl) rule = no-simply\nno-simply.no-simply = YES\n\n[.taskless/**]\ntskl) rule = no-simply\nno-simply.no-simply = NO\n" + ); + const assembled = await assembleValeConfig(cwd); + expect(assembled?.status).toBe("ok"); + if (assembled?.status !== "ok") return; + expect(assembled.advisories).toHaveLength(1); + expect(assembled.advisories[0]).toContain("[.taskless/**]"); + expect(assembled.advisories[0]).toContain("unnecessary"); }); // Writing an empty config would have Vale lint the project against no rules @@ -139,14 +237,19 @@ describe("Vale config assembly", () => { // Vale evaluate — a re-parse of the written file, which this is not, would // be a second, weaker source of the same fact. it("returns every section pattern it wrote, deduplicated and sorted", async () => { - await valeRule("no-simply", "[*.md]\nno-simply.no-simply = YES\n"); + await valeRule( + "no-simply", + "[*.md]\ntskl) rule = no-simply\nno-simply.no-simply = YES\n" + ); await valeRule( "no-very", - "[*.md]\nno-very.no-very = YES\n\n[**/README.md]\nno-very.no-very = YES\n" + "[*.md]\ntskl) rule = no-very\nno-very.no-very = YES\n\n[**/README.md]\ntskl) rule = no-very\nno-very.no-very = YES\n" ); const assembled = await assembleValeConfig(cwd); - expect(assembled?.sections).toEqual(["**/README.md", "*.md"]); + expect(assembled?.status).toBe("ok"); + if (assembled?.status !== "ok") return; + expect(assembled.sections).toEqual(["**/README.md", "*.md"]); }); }); diff --git a/packages/cli/test/mixed-engine-check.test.ts b/packages/cli/test/mixed-engine-check.test.ts index 3ced1096..6df841dc 100644 --- a/packages/cli/test/mixed-engine-check.test.ts +++ b/packages/cli/test/mixed-engine-check.test.ts @@ -289,60 +289,119 @@ describe("check over a project with both engines", () => { // The pairing that keeps a per-rule config honest. Scope is the author's // decision, which means the assignment can land above every matcher in - // their own `.vale.ini`. Vale does not error on that: it warns on stderr, - // exits zero, and returns a well-formed empty result, which is - // indistinguishable from a clean run. Surfacing the warning is the only - // thing standing between that mistake and a silently disabled rule. + // their own `.vale.ini`. Vale does not error on that: it warns `W101` on + // stderr, exits zero, and returns a well-formed empty result, which is + // indistinguishable from a clean run. // - // `verify` rejects this config before Vale ever sees it: the config schema - // refuses any assignment above the first matcher, under - // `vale-config-no-root-keys`. `check` does not yet consult the schema (that - // is the next slice of taskless/cli#359), so its run-time notice remains - // the last line of defence and is asserted here alongside the rejection. - it("surfaces Vale's W101 when an assignment sits outside every matcher", async () => { - const scaffold = await mkdtemp(join(tmpdir(), "taskless-w101-")); - try { - const init = await runCli(["init", "-d", scaffold]); - expect(init.exitCode).toBe(0); + // The config schema now turns that file away before Vale ever sees it, at + // `verify` (a rejection under `vale-config-no-root-keys`) and at `check` + // (the Vale engine's failure, which reaches the exit code). The run-time + // W101 notice is still pinned, in `vale-run.test.ts`, for the diagnostics + // the schema cannot foresee; this is the case it can. + it("refuses the Vale run when an assignment sits outside every matcher", async () => { + const rule = join(project, ".taskless", "rules", "vale", "no-simply"); + // The mistake: enabled, but above the `[…]` line, so it belongs to no + // matcher. A matcher exists and the check is named, so the substring + // checks `verify` used to run passed this; the schema does not. + await writeFile( + join(rule, ".vale.ini"), + "no-simply.no-simply = YES\n[*.md]\ntskl) rule = no-simply\nBasedOnStyles =\n" + ); - const rule = join(scaffold, ".taskless", "rules", "vale", "no-simply"); - await mkdir(rule, { recursive: true }); - await writeFile( - join(rule, "no-simply.yml"), - "extends: existence\nmessage: \"Avoid 'simply'\"\nlevel: warning\ntokens:\n - simply\n" - ); - // The mistake: enabled, but above the `[…]` line, so it belongs to no - // matcher. A matcher exists and the check is named, so the substring - // checks `verify` used to run passed this; the schema does not. - await writeFile( - join(rule, ".vale.ini"), - "no-simply.no-simply = YES\n[*.md]\ntskl) rule = no-simply\nBasedOnStyles =\n" - ); - await writeFile(join(scaffold, "doc.md"), "Just simply do it.\n"); + const verified = await runCli(["verify", "-d", project, "--json"]); + expect(verified.exitCode).not.toBe(0); + const report = JSON.parse(verified.stdout) as { + rules: { ruleId: string; violations: { constraintId: string }[] }[]; + }; + const rejected = report.rules.find( + (entry) => entry.ruleId === "no-simply" + ); + expect( + rejected?.violations.map((violation) => violation.constraintId) + ).toContain("vale-config-no-root-keys"); - const verified = await runCli(["verify", "-d", scaffold, "--json"]); - expect(verified.exitCode).not.toBe(0); - const report = JSON.parse(verified.stdout) as { - rules: { violations: { constraintId: string }[] }[]; - }; - expect( - report.rules[0]?.violations.map((violation) => violation.constraintId) - ).toContain("vale-config-no-root-keys"); + const { stdout, exitCode } = await runCli([ + "check", + "-d", + project, + "--json", + ]); + const output = JSON.parse(stdout.trim()) as CheckOutput & { + failures?: string[]; + notices?: string[]; + }; + + // Refused rather than stripped: the failure names the rule and the line, + // and the exit code hears it. Vale is never run, so the run-time W101 + // notice that used to be the last line of defence has nothing to say. + expect(exitCode).toBe(1); + expect(output.success).toBe(false); + expect(output.failures).toHaveLength(1); + expect(output.failures?.[0]).toContain("Vale did not run"); + expect(output.failures?.[0]).toContain("no-simply/.vale.ini line 1:"); + expect(output.failures?.[0]).toContain("no-simply.no-simply"); + expect(output.notices).toBeUndefined(); + + // The other engine is unaffected: ast-grep still reports, and Vale, which + // was refused, reports nothing rather than something partial. + const sources = new Set(output.results.map((finding) => finding.source)); + expect(sources).toContain("ast-grep"); + expect(sources).not.toContain("vale"); + }); - const { stdout, stderr, exitCode } = await runCli([ - "check", - "-d", - scaffold, - ]); + // The cross-rule prohibition the spec has always stated and nothing + // enforced: a rule cannot override another rule's matchers. The offending + // config here is `no-simply`'s, so its neighbour `no-obviously`, which is + // fine, is not the rule named. + it("refuses the Vale run when a rule assigns another rule's key", async () => { + const rule = join(project, ".taskless", "rules", "vale", "no-simply"); + await writeFile( + join(rule, ".vale.ini"), + "[*.md]\ntskl) rule = no-simply\nBasedOnStyles =\nno-simply.no-simply = YES\nno-obviously.no-obviously = NO\n" + ); - // Advisory, so the run still succeeds: nothing is broken, something is - // misplaced. Failing here would make an ignorable warning block a check. - expect(exitCode).toBe(0); - expect(`${stdout}${stderr}`).toContain("W101"); - expect(`${stdout}${stderr}`).toContain("no-simply"); - } finally { - await rm(scaffold, { recursive: true, force: true }); - } + const { stdout, exitCode } = await runCli([ + "check", + "-d", + project, + "--json", + ]); + const output = JSON.parse(stdout.trim()) as CheckOutput & { + failures?: string[]; + }; + + expect(exitCode).toBe(1); + expect(output.failures).toHaveLength(1); + expect(output.failures?.[0]).toContain("the config of no-simply was"); + expect(output.failures?.[0]).toContain("no-simply/.vale.ini line 5:"); + expect(output.failures?.[0]).toContain("no-obviously.no-obviously"); + expect(output.failures?.[0]).not.toContain("config of no-obviously"); + + const sources = new Set(output.results.map((finding) => finding.source)); + expect(sources).toContain("ast-grep"); + expect(sources).not.toContain("vale"); + }); + + // An advisory is said, not refused: the run proceeds and the exit code + // hears only the findings. + it("carries a config advisory as a notice on a run that still happens", async () => { + const rule = join(project, ".taskless", "rules", "vale", "no-simply"); + await writeFile( + join(rule, ".vale.ini"), + "[*.md]\ntskl) rule = no-simply\nBasedOnStyles =\nno-simply.no-simply = YES\n\n[.taskless/**]\ntskl) rule = no-simply\nno-simply.no-simply = NO\n" + ); + + const { stdout } = await runCli(["check", "-d", project, "--json"]); + const output = JSON.parse(stdout.trim()) as CheckOutput & { + failures?: string[]; + notices?: string[]; + }; + + expect(output.failures).toBeUndefined(); + expect(output.notices?.join("\n")).toContain("[.taskless/**]"); + expect(output.notices?.join("\n")).toContain("unnecessary"); + const sources = new Set(output.results.map((finding) => finding.source)); + expect(sources).toContain("vale"); }); }); diff --git a/packages/cli/test/vale-orchestration.test.ts b/packages/cli/test/vale-orchestration.test.ts index a84a379e..acc96090 100644 --- a/packages/cli/test/vale-orchestration.test.ts +++ b/packages/cli/test/vale-orchestration.test.ts @@ -183,7 +183,7 @@ describe("exit code carried on the dispatch result", () => { cwd, paths: ["app.js"], astGrepConfigPath: await assembleSgConfig(cwd), - valeConfigPath: undefined, + vale: undefined, runtimeRules: [], }); expect(dispatched.results.length).toBeGreaterThan(0); @@ -196,7 +196,7 @@ describe("exit code carried on the dispatch result", () => { cwd, paths: ["app.js"], astGrepConfigPath: await assembleSgConfig(cwd), - valeConfigPath: undefined, + vale: undefined, runtimeRules: [], }); expect( @@ -211,7 +211,7 @@ describe("exit code carried on the dispatch result", () => { cwd, paths: ["doc.md"], // the sg rule is javascript-only, so nothing matches astGrepConfigPath: await assembleSgConfig(cwd), - valeConfigPath: undefined, + vale: undefined, runtimeRules: [], }); expect(dispatched.results).toEqual([]); @@ -230,7 +230,7 @@ withVale("runEngines over a mixed corpus", () => { cwd, paths: ["app.js", "doc.md"], astGrepConfigPath: await assembleSgConfig(cwd), - valeConfigPath: assembledVale?.path, + vale: assembledVale, runtimeRules: [], }); @@ -249,7 +249,7 @@ withVale("runEngines over a mixed corpus", () => { cwd, paths: ["app.js", "doc.md"], astGrepConfigPath: await assembleSgConfig(cwd), - valeConfigPath: assembledVale?.path, + vale: assembledVale, runtimeRules: [], }); expect(dispatched.results.every((result) => result.source !== "vale")).toBe( @@ -284,7 +284,7 @@ describe("runEngines when a Vale rule directory assembles to nothing", () => { cwd, paths: ["app.js", "doc.md"], astGrepConfigPath: await assembleSgConfig(cwd), - valeConfigPath: assembledVale?.path, + vale: assembledVale, runtimeRules: [], }); @@ -315,7 +315,7 @@ describe("runEngines when Vale is unavailable", () => { cwd, paths: ["app.js", "doc.md"], astGrepConfigPath: await assembleSgConfig(cwd), - valeConfigPath: assembledVale?.path, + vale: assembledVale, runtimeRules: [], }); @@ -369,7 +369,7 @@ describe("runEngines when Vale is unavailable", () => { cwd, paths: ["doc.md"], astGrepConfigPath: await assembleSgConfig(cwd), - valeConfigPath: assembledVale?.path, + vale: assembledVale, runtimeRules: [], }); @@ -400,7 +400,7 @@ describe("runEngines when Vale is unavailable", () => { cwd, paths: ["app.js", "doc.md"], astGrepConfigPath: await assembleSgConfig(cwd), - valeConfigPath: assembledVale?.path, + vale: assembledVale, runtimeRules: [], }); diff --git a/packages/cli/test/vale-run.test.ts b/packages/cli/test/vale-run.test.ts index a495db2f..b510aba1 100644 --- a/packages/cli/test/vale-run.test.ts +++ b/packages/cli/test/vale-run.test.ts @@ -230,6 +230,12 @@ withVale("runVale against the real binary", () => { // and ignores it. Measured: `W101 … isn't a core option` on stderr, exit // zero, an empty result on stdout. Without the notice this is a clean run // with no findings — the rule is disabled and nothing says so. + // + // `check` no longer reaches Vale with this file: the config schema rejects + // a root-level assignment at assembly. The config is hand-written here and + // handed to `runVale` directly, deliberately around the schema, because + // the notice path has to stay pinned for the diagnostics the schema cannot + // foresee, and W101 is the one Vale is known to emit on a zero exit. const cwd = makeProject( `${header}rules.no-simply = YES\n\n[*.md]\nBasedOnStyles =\n`, { "no-simply": existenceRule("simply", "Avoid 'simply'") }, From 79475e12d4709e20fb3bc7f188e4a429625fcd07 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 21 Sep 2026 11:46:37 -0700 Subject: [PATCH 2/5] docs(vale): recipe, ledger and changeset for the config refusal create-vale-rule (topic v10) says the config is schema-checked at verify and refused at check, lists what is rejected and what is advised, and notes that a NO matcher preceding every YES is rejected. update (topic v8) gains a 0.11.3 ledger entry: a config check used to tolerate now refuses the Vale run, with the four shapes most likely to trip an existing rule and the verify command that names the line. The changeset grows by the refusal and the reason the bump stays patch. --- .changeset/vale-config-schema.md | 6 ++- openspec/changes/vale-config-schema/tasks.md | 6 +-- packages/cli/src/agent/create-vale-rule.md | 53 +++++++++++++++++--- packages/cli/src/agent/update.md | 24 ++++++++- 4 files changed, 76 insertions(+), 13 deletions(-) diff --git a/.changeset/vale-config-schema.md b/.changeset/vale-config-schema.md index ecd422cb..1f3725d6 100644 --- a/.changeset/vale-config-schema.md +++ b/.changeset/vale-config-schema.md @@ -2,4 +2,8 @@ "@taskless/cli": patch --- -`verify` validates a Vale rule's `.vale.ini` against a schema and names the constraint each rejection violates. The config is parsed into an ordered structure and checked there: an assignment above the first matcher, a matcher without its `tskl) rule` breadcrumb, a key naming another rule, a value other than `YES`/`NO`, a non-empty `BasedOnStyles`, a config with no matcher or no `YES`, and a `NO` matcher that precedes every `YES` are each rejected under a `vale-config-*` constraint that `verify --json` reports in `violations[]` and `reference.json` publishes. A repeated key, a `[*]` matcher, and a `.taskless/**` matcher are reported as a notice without failing the rule. `check` is unchanged. +`verify` validates a Vale rule's `.vale.ini` against a schema and names the constraint each rejection violates. The config is parsed into an ordered structure and checked there: an assignment above the first matcher, a matcher without its `tskl) rule` breadcrumb, a key naming another rule, a value other than `YES`/`NO`, a non-empty `BasedOnStyles`, a config with no matcher or no `YES`, and a `NO` matcher that precedes every `YES` are each rejected under a `vale-config-*` constraint that `verify --json` reports in `violations[]` and `reference.json` publishes. A repeated key, a `[*]` matcher, and a `.taskless/**` matcher are reported as a notice without failing the rule. + +`check` runs the same schema before assembling the Vale run config, and a rejected config refuses the Vale engine for that run: the failure names the rule and the line, reaches the exit code, and ast-grep still runs. A rule is never silently left out of the assembled config. Accepted configs are written verbatim under their breadcrumb, so the one string edit assembly used to make (dropping a copied-in `StylesPath`) is gone; that line is now a rejection. Advisories reach `check`'s notices. To find every rejected line at once, run `taskless verify`. + +This is still `patch`. The package is `0.y.z`, and every config the schema refuses was already being misread by Vale: a rule enabled nowhere with a `W101` on stderr, a rule silently overriding a neighbour, a disable the following enable cancelled. The release surfaces a defect the consumer already had rather than introducing one, the same call as linting files over 128 KB again in 0.11.3. diff --git a/openspec/changes/vale-config-schema/tasks.md b/openspec/changes/vale-config-schema/tasks.md index 44974543..7a882902 100644 --- a/openspec/changes/vale-config-schema/tasks.md +++ b/openspec/changes/vale-config-schema/tasks.md @@ -12,7 +12,7 @@ - [x] 2.1 In `rules/assemble.ts`, delete `ruleConfigBody` and `sectionPatternsOf`; `assembleValeConfig` validates each config, and on any rejection returns a refusal naming the rule and line without writing the file. Accepted configs are written verbatim under their breadcrumb; `sections` comes from the AST. Verify the "Assembly order is stable" test still asserts byte-identical output, and a new test asserts a foreign key leaves `.taskless/.vale.ini` unwritten. - [x] 2.2 In `rules/dispatch.ts`, a refusal becomes the Vale engine's `failure` (reaches the exit code); advisories join `notices`. Verify a mixed-engine test shows ast-grep results alongside the Vale failure and a non-zero exit. -- [ ] 2.3 Recipe `packages/cli/src/agent/create-vale-rule.md`: state that the config is schema-checked, list what is rejected and what is advised, drop any `.taskless/**` matcher from its examples, and bump the topic version. Verify `pnpm build && pnpm cli agent create-vale-rule` renders and `pnpm cli check` is clean over the prose. -- [ ] 2.4 `packages/cli/src/agent/update.md`: 0.11.3 ledger entry — a config `check` used to tolerate now refuses the Vale run, with the `verify` command that names the line; bump the topic version. -- [ ] 2.5 Extend the changeset with the refusal. Run `pnpm typecheck`, `pnpm lint`, `pnpm --filter @taskless/cli test`; open PR 2 against PR 1's branch. +- [x] 2.3 Recipe `packages/cli/src/agent/create-vale-rule.md`: state that the config is schema-checked, list what is rejected and what is advised, drop any `.taskless/**` matcher from its examples, and bump the topic version. Verify `pnpm build && pnpm cli agent create-vale-rule` renders and `pnpm cli check` is clean over the prose. +- [x] 2.4 `packages/cli/src/agent/update.md`: 0.11.3 ledger entry — a config `check` used to tolerate now refuses the Vale run, with the `verify` command that names the line; bump the topic version. +- [x] 2.5 Extend the changeset with the refusal. Run `pnpm typecheck`, `pnpm lint`, `pnpm --filter @taskless/cli test`; open PR 2 against PR 1's branch. - [ ] 2.6 Archive on this tip: wip commit, `pnpm openspec archive vale-config-schema -y`, confirm every prior scenario in `cli-vale-rule-engine` (23 before, 32 after) and `cli-rule-validation` (26 before, 30 after) survives, reset to the wip SHA, then archive for real. diff --git a/packages/cli/src/agent/create-vale-rule.md b/packages/cli/src/agent/create-vale-rule.md index 78e7c3ed..e39d4ba7 100644 --- a/packages/cli/src/agent/create-vale-rule.md +++ b/packages/cli/src/agent/create-vale-rule.md @@ -1,4 +1,4 @@ -# Topic: create-vale-rule (CLI v%(CLI_VERSION)s / topic v9) +# Topic: create-vale-rule (CLI v%(CLI_VERSION)s / topic v10) ## You are here This is `create-vale-rule`. It helps you write a Vale rule: a check over @@ -547,17 +547,54 @@ it. no-simply.no-simply = NO ``` - Write the breadcrumb in the second matcher too, or assembly attributes - it to nobody. Reversing the two blocks silently re-enables the rule on - the file you meant to exempt, and nothing reports that. + Write the breadcrumb in the second matcher too, or `verify` rejects + the matcher as unattributed. Reversing the two blocks would silently + re-enable the rule on the file you meant to exempt, so the schema + rejects a `NO` matcher that precedes every `YES`: with no style + loaded the rule is off until something turns it on, which makes such + a `NO` either dead or overridden. Put the `NO` after the `YES` it + narrows. **Do NOT write `StylesPath` or `MinAlertLevel` here.** Those describe - the run rather than a rule, the assembler supplies them, and a copy - in a rule's config is dropped on the way in. + the run rather than a rule, and the assembler supplies them. A copy in + a rule's config is rejected, not dropped: the file you write is the + file Vale reads, byte for byte, so nothing is edited on the way in. Keep assignments underneath a matcher. An assignment above the first - `[…]` line belongs to no matcher, and Vale ignores it after warning - on stderr. + `[…]` line belongs to no matcher; Vale would ignore it after a + warning on stderr and leave the rule enabled nowhere, so the schema + rejects it instead. + + **The config is schema-checked.** `verify` parses `.vale.ini` into a + structure and validates it against a schema keyed by the rule's + directory name, naming the line and the `vale-config-*` constraint + behind each rejection. `check` runs the same schema before assembling + the run config, and a rejected config refuses the Vale engine for + that run: the failure names the rule and the line, the exit code is + non-zero, and the other engines still run. A rule is never quietly + left out. What is **rejected**: + + - anything assigned above the first matcher (`StylesPath`, + `MinAlertLevel`, or a rule assignment) + - a matcher without a `tskl) rule = ` breadcrumb naming this rule + - an assignment key other than `.` (a key naming another rule + is a cross-rule override, which a rule cannot do) + - a value other than `YES` or `NO` + - a `BasedOnStyles` that is not empty + - a config with no matcher, or one that never assigns `YES` + - a `NO` matcher that precedes every `YES` matcher + + What is **advised**, on the rule's notice, without rejecting: + + - the same key assigned twice inside one matcher (Vale keeps the last + assignment as of 3.21.0; 3.20.0 kept the first) + - a `[*]` matcher, which reaches every file Vale can read + - a matcher under `.taskless/**`, which `check` already excludes + before Vale runs, so it acts only under a bare `vale` invocation + + Each advisory has a legitimate reading, which is what separates the + two lists. Fix a rejection before moving on; read an advisory and + decide. **What a matcher's glob is allowed to catch.** Vale (v%(VALE_VERSION)s) treats a file one of four ways, decided by extension. The lists are diff --git a/packages/cli/src/agent/update.md b/packages/cli/src/agent/update.md index e8ba3523..4463a5f8 100644 --- a/packages/cli/src/agent/update.md +++ b/packages/cli/src/agent/update.md @@ -1,4 +1,4 @@ -# Topic: update (CLI v%(CLI_VERSION)s / topic v7) +# Topic: update (CLI v%(CLI_VERSION)s / topic v8) ## You are here This is `update`. It tells you what an upgrade changed for the rules @@ -307,6 +307,28 @@ entry that named it no longer appears. Nothing in a rule changes; if a large generated file was being kept quiet by that skip, narrow the matcher's section so it is not reached. +**A Vale rule's `.vale.ini` is validated against a schema, and a +config `check` used to tolerate now refuses the Vale run.** Every +config was carried into the run verbatim; nothing checked that a +matcher carried its breadcrumb, that an assignment named this rule and +not another, or that the run-level keys stayed out. Each of those is a +config Vale accepts and reads as something other than what its author +wrote, with a zero exit and an empty report. Now `verify` rejects such +a config, naming the line and the `vale-config-*` constraint, and +`check` refuses the Vale engine over it: the failure names the rule and +the line, the exit code is non-zero, and ast-grep still runs. Run +`%(TASKLESS_CLI)s verify` to see every rejected line at once. The four +most likely to trip an existing rule: an assignment above the first +matcher (`StylesPath`, `MinAlertLevel`, or the rule's own key, which +Vale was ignoring with a `W101`); a matcher with no `tskl) rule = ` +breadcrumb; a key naming another rule (`no-hedging.no-hedging = NO` +inside `no-simply`'s config, which was a cross-rule override); and a +`NO` matcher declared before every `YES`, which the `YES` was +overriding. A `.taskless/**` matcher is reported as unnecessary rather +than rejected, since `check` excludes that tree before Vale runs; +delete it. `%(TASKLESS_CLI)s agent create-vale-rule` lists every +rejection and advisory. + ## Errors With `--json`, `--rules` failures emit `{ ok: false, code, message }`: From 0e3c4f7f557dc5c461d572677c0fd6e98573dcfb Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 21 Sep 2026 11:56:52 -0700 Subject: [PATCH 3/5] fix(vale): carry config advisories through a blocking Vale failure runValeEngine threaded the schema's advisories into the notice on the ok and unavailable branches but returned only the failure when Vale timed out or crashed, so an advisory was heard only on runs where Vale did not break. The blocking branch now carries them on notice beside the failure; they are not folded into the failure because an advisory is not what failed the run. Pinned for all three outcome shapes. --- packages/cli/src/rules/dispatch.ts | 13 +++- packages/cli/test/vale-orchestration.test.ts | 74 ++++++++++++++++++++ 2 files changed, 86 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/rules/dispatch.ts b/packages/cli/src/rules/dispatch.ts index 633b65e2..f175fe8e 100644 --- a/packages/cli/src/rules/dispatch.ts +++ b/packages/cli/src/rules/dispatch.ts @@ -219,7 +219,18 @@ async function runValeEngine(options: DispatchOptions): Promise { }; } if (outcome.blocking) { - return { engine: "vale", results: [], failure: outcome.message }; + // The failure is Vale's; the advisories are still the schema's, and they + // are still advisory. They ride on `notice` beside the failure rather + // 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 }), + }; } return { engine: "vale", diff --git a/packages/cli/test/vale-orchestration.test.ts b/packages/cli/test/vale-orchestration.test.ts index acc96090..8efca0a4 100644 --- a/packages/cli/test/vale-orchestration.test.ts +++ b/packages/cli/test/vale-orchestration.test.ts @@ -414,6 +414,80 @@ describe("runEngines when Vale is unavailable", () => { ); }); +describe("config advisories ride on every Vale outcome", () => { + // The schema's advisories are decided at assembly, before Vale runs, so + // whether the author hears them cannot depend on how Vale's own run went. + // Vale is mocked because the shape under test is dispatch's merge, not the + // binary; each case drives one branch of `runValeEngine`. + const ADVISED_CONFIG = + "[*.md]\ntskl) rule = no-simply\nBasedOnStyles =\nno-simply.no-simply = YES\n\n" + + "[.taskless/**]\ntskl) rule = no-simply\nno-simply.no-simply = NO\n"; + + async function dispatchWithAdvisory( + outcome: Awaited< + ReturnType<(typeof import("../src/rules/vale/run"))["runVale"]> + > + ) { + const run = await import("../src/rules/vale/run"); + vi.spyOn(run, "runVale").mockResolvedValue(outcome); + const cwd = makeMixedProject(); + writeFileSync( + join(cwd, ".taskless", "rules", "vale", "no-simply", ".vale.ini"), + ADVISED_CONFIG + ); + const assembledVale = await assembleValeConfig(cwd); + return runEngines({ + cwd, + paths: ["doc.md"], + astGrepConfigPath: await assembleSgConfig(cwd), + vale: assembledVale, + runtimeRules: [], + }); + } + + it("joins Vale's own zero-exit diagnostic on a run that succeeded", async () => { + const dispatched = await dispatchWithAdvisory({ + status: "ok", + blocking: false, + results: [], + notice: "W101 something Vale said", + }); + expect(dispatched.notices).toHaveLength(1); + expect(dispatched.notices[0]).toContain("[.taskless/**]"); + expect(dispatched.notices[0]).toContain("W101 something Vale said"); + expect(dispatched.failures).toEqual([]); + }); + + it("survives a Vale timeout as a notice beside the failure", async () => { + // The case a reviewer caught: the blocking branch used to return only the + // failure, so an advisory was heard only on runs where Vale did not crash. + const dispatched = await dispatchWithAdvisory({ + status: "timeout", + blocking: true, + message: "Vale timed out after 1ms", + }); + expect(dispatched.failures).toEqual(["Vale timed out after 1ms"]); + expect(dispatched.notices).toHaveLength(1); + expect(dispatched.notices[0]).toContain("[.taskless/**]"); + // An advisory is not what failed the run, so it is not in the failure. + expect(dispatched.failures[0]).not.toContain("[.taskless/**]"); + expect(dispatched.exitCode).toBe(1); + }); + + it("joins the skip notice when the binary is unavailable", async () => { + const dispatched = await dispatchWithAdvisory({ + status: "unavailable", + blocking: false, + message: "Vale binary not found", + }); + expect(dispatched.notices).toHaveLength(1); + expect(dispatched.notices[0]).toContain("[.taskless/**]"); + expect(dispatched.notices[0]).toContain("Vale binary not found"); + expect(dispatched.failures).toEqual([]); + expect(dispatched.exitCode).toBe(0); + }); +}); + describe("an engine failure under --json", () => { it("reaches the machine envelope, not only the suppressed warning", async () => { // `warn()` is a no-op under `--json`, so without a field for it the From e0c9cc9bfb64b0f137180b2310ecabdbb7af6698 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 21 Sep 2026 12:04:50 -0700 Subject: [PATCH 4/5] spec(vale): archive vale-config-schema Dry-run first per CLAUDE.md, against the parent's revised delta (final per-matcher verdicts): cli-vale-rule-engine 23 -> 33 scenarios, cli-rule-validation 26 -> 31, no prior scenario lost by name. Then archived for real on this tip. --- .../.openspec.yaml | 0 .../proposal.md | 0 .../specs/cli-rule-validation/spec.md | 0 .../specs/cli-vale-rule-engine/spec.md | 0 .../2026-09-21-vale-config-schema}/tasks.md | 2 +- openspec/specs/cli-rule-validation/spec.md | 42 +++++++-- openspec/specs/cli-vale-rule-engine/spec.md | 91 ++++++++++++++++++- 7 files changed, 124 insertions(+), 11 deletions(-) rename openspec/changes/{vale-config-schema => archive/2026-09-21-vale-config-schema}/.openspec.yaml (100%) rename openspec/changes/{vale-config-schema => archive/2026-09-21-vale-config-schema}/proposal.md (100%) rename openspec/changes/{vale-config-schema => archive/2026-09-21-vale-config-schema}/specs/cli-rule-validation/spec.md (100%) rename openspec/changes/{vale-config-schema => archive/2026-09-21-vale-config-schema}/specs/cli-vale-rule-engine/spec.md (100%) rename openspec/changes/{vale-config-schema => archive/2026-09-21-vale-config-schema}/tasks.md (98%) diff --git a/openspec/changes/vale-config-schema/.openspec.yaml b/openspec/changes/archive/2026-09-21-vale-config-schema/.openspec.yaml similarity index 100% rename from openspec/changes/vale-config-schema/.openspec.yaml rename to openspec/changes/archive/2026-09-21-vale-config-schema/.openspec.yaml diff --git a/openspec/changes/vale-config-schema/proposal.md b/openspec/changes/archive/2026-09-21-vale-config-schema/proposal.md similarity index 100% rename from openspec/changes/vale-config-schema/proposal.md rename to openspec/changes/archive/2026-09-21-vale-config-schema/proposal.md diff --git a/openspec/changes/vale-config-schema/specs/cli-rule-validation/spec.md b/openspec/changes/archive/2026-09-21-vale-config-schema/specs/cli-rule-validation/spec.md similarity index 100% rename from openspec/changes/vale-config-schema/specs/cli-rule-validation/spec.md rename to openspec/changes/archive/2026-09-21-vale-config-schema/specs/cli-rule-validation/spec.md diff --git a/openspec/changes/vale-config-schema/specs/cli-vale-rule-engine/spec.md b/openspec/changes/archive/2026-09-21-vale-config-schema/specs/cli-vale-rule-engine/spec.md similarity index 100% rename from openspec/changes/vale-config-schema/specs/cli-vale-rule-engine/spec.md rename to openspec/changes/archive/2026-09-21-vale-config-schema/specs/cli-vale-rule-engine/spec.md diff --git a/openspec/changes/vale-config-schema/tasks.md b/openspec/changes/archive/2026-09-21-vale-config-schema/tasks.md similarity index 98% rename from openspec/changes/vale-config-schema/tasks.md rename to openspec/changes/archive/2026-09-21-vale-config-schema/tasks.md index 7a882902..3547fe97 100644 --- a/openspec/changes/vale-config-schema/tasks.md +++ b/openspec/changes/archive/2026-09-21-vale-config-schema/tasks.md @@ -15,4 +15,4 @@ - [x] 2.3 Recipe `packages/cli/src/agent/create-vale-rule.md`: state that the config is schema-checked, list what is rejected and what is advised, drop any `.taskless/**` matcher from its examples, and bump the topic version. Verify `pnpm build && pnpm cli agent create-vale-rule` renders and `pnpm cli check` is clean over the prose. - [x] 2.4 `packages/cli/src/agent/update.md`: 0.11.3 ledger entry — a config `check` used to tolerate now refuses the Vale run, with the `verify` command that names the line; bump the topic version. - [x] 2.5 Extend the changeset with the refusal. Run `pnpm typecheck`, `pnpm lint`, `pnpm --filter @taskless/cli test`; open PR 2 against PR 1's branch. -- [ ] 2.6 Archive on this tip: wip commit, `pnpm openspec archive vale-config-schema -y`, confirm every prior scenario in `cli-vale-rule-engine` (23 before, 32 after) and `cli-rule-validation` (26 before, 30 after) survives, reset to the wip SHA, then archive for real. +- [x] 2.6 Archive on this tip: wip commit, `pnpm openspec archive vale-config-schema -y`, confirm every prior scenario in `cli-vale-rule-engine` (23 before, 32 after) and `cli-rule-validation` (26 before, 30 after) survives, reset to the wip SHA, then archive for real. diff --git a/openspec/specs/cli-rule-validation/spec.md b/openspec/specs/cli-rule-validation/spec.md index 174b1ab4..1fd51284 100644 --- a/openspec/specs/cli-rule-validation/spec.md +++ b/openspec/specs/cli-rule-validation/spec.md @@ -42,11 +42,11 @@ The two commands split because they have different preconditions. An agent part- Per engine, `verify` SHALL check: -| Engine | Components | -| --------- | ---------------------------------------------------------------------------------------------------- | -| `sg` | `.yml` against the ast-grep schema and the Taskless required fields | -| `vale` | `.yml` against the Vale rule schema and the Taskless required fields, and the rule's `.vale.ini` | -| `runtime` | `check.ts` present, and at least one capture rule under `captures/` | +| Engine | Components | +| --------- | ------------------------------------------------------------------------------------------------------------------------------ | +| `sg` | `.yml` against the ast-grep schema and the Taskless required fields | +| `vale` | `.yml` against the Vale rule schema and the Taskless required fields, and the rule's `.vale.ini` against the config schema | +| `runtime` | `check.ts` present, and at least one capture rule under `captures/` | The `vale` row previously read "against Vale's own validation." Measured against the pinned 3.18.0 binary, that covers less than it claims: `level: bananas` is reported, while `extends: nonsense` and `scope: fenced` both verify clean and produce a rule that matches nothing. Vale validates a rule when it _runs_ one, and it runs one field at a time — so a name it does not recognize is not an error, it is a check that never fires. Schema validation is therefore its own layer for `vale`, as it already is for `sg`. @@ -80,6 +80,29 @@ The `vale` row previously read "against Vale's own validation." Measured against The failure it prevents is not a local one: Vale reports this as `E201: has invalid keys` and reads one assembled config per run, so a single rule with a stray field suppresses every other Vale rule's findings. +#### Scenario: A rule config that never enables the rule is rejected + +- **WHEN** a Vale rule's `.vale.ini` declares matchers but no `. = YES` +- **THEN** `verify` SHALL report that the rule is present but off, naming the file + +#### Scenario: A rule config that assigns a foreign key is rejected + +- **WHEN** a Vale rule's `.vale.ini` assigns a `