From 3d154040071630283e3620a2f0aa9cef6494de7b Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 20 Oct 2026 16:48:16 -0700 Subject: [PATCH 1/2] fix(vale): make the Vale 3.24.0 upgrade land green, and refuse a rule's tests: key Vale 3.24.0 warns W101 about every key it does not recognise inside a matcher, including the `tskl) rule` breadcrumb. That printed one notice per matcher on every check, and the warning ahead of a fatal error's JSON broke the per-file narrowing from taskless/cli#300. Assembly now comments the breadcrumb out of the config Vale reads, and runVale peels Vale's warning lines off before parsing its error, one notice each. The converter E100 dropped its `[lintAdoc]` checker tag, so the contract asserts the format Vale now names instead. New 3.24.0 pins cover the W101, warning-then-error order, C# tool directives, a non-breaking space after a link, and files no rule runs on. `validate` and `verify` now reject a Vale rule's `tests:` key: Taskless verifies Vale rules by their .tests/pass and .tests/fail documents and never runs `vale test`. --- .changeset/vale-3-24-0.md | 6 + packages/cli/src/agent/update.md | 43 +++++- packages/cli/src/rules/assemble.ts | 30 +++- packages/cli/src/rules/capabilities.ts | 40 +++--- packages/cli/src/rules/vale/formats.ts | 10 +- packages/cli/src/rules/vale/run.ts | 64 +++++++-- packages/cli/src/schemas/vale-config.ts | 19 +++ packages/cli/src/schemas/vale-rule.ts | 31 +++++ packages/cli/test/assemble.test.ts | 11 +- packages/cli/test/vale-corpus.ts | 26 ++-- packages/cli/test/vale-formats.test.ts | 2 +- packages/cli/test/vale-run.test.ts | 45 ++++++ .../cli/test/vale-schema-contract.test.ts | 15 ++ .../cli/test/vale-vendor-contract.test.ts | 131 +++++++++++++++++- 14 files changed, 411 insertions(+), 62 deletions(-) diff --git a/.changeset/vale-3-24-0.md b/.changeset/vale-3-24-0.md index cf77ea57..9fd0aa15 100644 --- a/.changeset/vale-3-24-0.md +++ b/.changeset/vale-3-24-0.md @@ -3,3 +3,9 @@ --- Update the bundled Vale to 3.24.0. + +Vale 3.24.0 warns about any key it does not recognise inside a matcher, including the `tskl) rule` breadcrumb every Vale rule config carries. The CLI now comments the breadcrumb out of the config it assembles for Vale, so `check` no longer prints one notice per matcher, and a file Vale cannot parse is again set aside with its own finding instead of failing the whole Vale run. Vale's warnings now arrive as one notice each. + +Two findings change for existing rules: C# `// ReSharper disable` and `// NOSONAR` comments are no longer linted, and a word after a link and a non-breaking space is now matched. `update` (topic v18) walks both. + +`validate` and `verify` now reject a Vale rule's `tests:` key. Vale 3.24.0 runs those cases with `vale test`, which Taskless never calls; a Vale rule is verified by its `.tests/pass` and `.tests/fail` documents. diff --git a/packages/cli/src/agent/update.md b/packages/cli/src/agent/update.md index facbe62c..ef079eb8 100644 --- a/packages/cli/src/agent/update.md +++ b/packages/cli/src/agent/update.md @@ -497,8 +497,9 @@ loaded it without complaint. ### Migrating to 0.13.3 -The vendored ast-grep moves from 0.45.3 to 0.51.0. Nothing here needs -work by hand. +The vendored ast-grep moves from 0.45.3 to 0.51.0, and Vale from 3.23.0 +to 3.24.0. Nothing here needs work by hand except a Vale rule carrying +a `tests:` key, described at the end. **A Haskell type synonym is `type_synonym` now, not `type_synomym`, and `update --rules` renames it for you.** ast-grep's Haskell grammar moved @@ -526,6 +527,44 @@ files that was routed to the runtime tier, or not written because ast-grep could not parse Zig, can now be an sg rule. No existing rule changes. +Two things in Vale 3.24.0 change which findings an existing rule +reports, each measured against both binaries on the same fixture. + +**A C# tool directive is no longer read.** A `// ReSharper disable`, +`// ReSharper restore` or `// NOSONAR` line in a `.cs` file was linted +as an ordinary comment, so a rule over comments fired on a suppression +list. It is now skipped, as 3.23.0 already did for `//nolint`, `# noqa` +and `eslint-disable`. Findings disappear; none appear. + +**A word after a link and a non-breaking space is matched.** Through +3.23.0 a U+00A0 or U+202F after inline markup was dropped, fusing the +link text and the next word, so a token there went unreported. It is +now a word break, and a rule over that word starts reporting. + +What the CLI absorbed, so you do not have to: Vale 3.24.0 warns `W101` +about every key it does not recognise inside a matcher, which includes +the `tskl) rule` breadcrumb every rule config carries. Keep writing the +breadcrumb; the CLI comments it out of the file it hands Vale, so no +notice appears. A file no rule runs on is no longer parsed, which +`check` already approximated by setting converter-dependent files +aside, so nothing changes there. + +Additions a rule config can use: a matcher may carry a condition, +`[*.md if .Meta.draft]`, which applies it only to the matched files +whose front matter satisfies it. `validate` accepts it. + +**A rule's `tests:` key is refused.** Vale 3.24.0 ships `vale test`, +which runs cases written in a rule's `tests:` list. Taskless never runs +it: `verify` judges a Vale rule by the documents under `.tests/pass` +and `.tests/fail`. So `validate` and `verify` now reject the key, which +0.12.0 had started accepting, rather than let a rule look tested by +cases nothing runs. `check` still runs such a rule. Search +`.taskless/rules/vale/*/*.yml` for a top-level `tests:`; for each case, +write its `input` as a document under `.tests/fail` if the case expects +the rule to fire, or `.tests/pass` if it expects no alert, named with +the case's `format` as its extension (`.md` when it has none). Then +delete the key and run `verify` on the rule. + ## Errors With `--json`, `--rules` failures emit `{ ok: false, code, message }`: diff --git a/packages/cli/src/rules/assemble.ts b/packages/cli/src/rules/assemble.ts index 561b759f..5e5a455c 100644 --- a/packages/cli/src/rules/assemble.ts +++ b/packages/cli/src/rules/assemble.ts @@ -83,16 +83,27 @@ function valeHeader(): string { } /** - * A rule's assembled block: its breadcrumb comment, then its config verbatim. + * A rule's assembled block: its breadcrumb comment, then its config verbatim + * except that each breadcrumb is commented out. * * 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 + * would be a semantic edit. The one addition is a newline after a source that * lacks one, so the next rule's breadcrumb starts its own line. * + * The one edit is `# ` in front of each `tskl) rule` line, at the lines the + * schema located (`breadcrumbLines`), so nothing here searches the text for + * it. That is not a semantic edit: Vale read the key as a rule named + * `tskl) rule` switched off, which is nothing. What changed in 3.24.0 is that + * it says so, `W101 'tskl) rule' isn't a section option`, once per matcher on + * every run, and that line on stderr in front of a real error's JSON is what + * kept a file Vale cannot parse from being narrowed out of the run + * (taskless/cli#300). Commenting rather than deleting keeps every line where + * the author put it, so a line number Vale reports is still the author's. + * * 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 @@ -100,8 +111,17 @@ function valeHeader(): string { * key above the first matcher is now a rejection, so there is nothing left to * strip. */ -function valeRuleBlock(ruleId: string, source: string): string { - const terminated = source.endsWith("\n") ? source : `${source}\n`; +function valeRuleBlock( + ruleId: string, + source: string, + breadcrumbLines: readonly number[] +): string { + const lines = source.split("\n"); + for (const line of breadcrumbLines) { + lines[line - 1] = `# ${lines[line - 1] ?? ""}`; + } + const commented = lines.join("\n"); + const terminated = commented.endsWith("\n") ? commented : `${commented}\n`; return `# tskl) rule = ${ruleId}\n${terminated}`; } @@ -243,7 +263,7 @@ export async function assembleValeConfig( if (selected !== undefined && !selected.has(ruleId)) continue; for (const pattern of verdict.sections) sections.add(pattern); advisories.push(...verdict.advisories); - blocks.push(valeRuleBlock(ruleId, source)); + blocks.push(valeRuleBlock(ruleId, source, verdict.breadcrumbLines)); } if (refusals.length > 0) return { status: "refused", refusals }; diff --git a/packages/cli/src/rules/capabilities.ts b/packages/cli/src/rules/capabilities.ts index 5be3222f..f32df291 100644 --- a/packages/cli/src/rules/capabilities.ts +++ b/packages/cli/src/rules/capabilities.ts @@ -569,30 +569,34 @@ export const VALE_CONVERTER_DEPENDENT: readonly ValeConverterFormat[] = groupByConverter(); /** - * Vale's checker tag per converter-dependent extension, from the `E100` text. - * - * This is the host-independent half of the failure. The prose after the tag is - * not: `.xml` reports `xsltproc not found` where the program is absent and - * `no XSLT transform provided` where it is present, and the two are split by - * platform — macOS ships `/usr/bin/xsltproc`, the Linux CI image does not. A - * contract test that matched on the program name therefore passed locally and - * failed in CI, which is how this list came to exist. + * The format Vale names per converter-dependent extension, from the `E100` text. + * + * Since 3.24.0 the error reads `asciidoctor not found; it's needed to read + * AsciiDoc`, and the format at the end is the host-independent half. Through + * 3.23.0 that half was a checker tag, `E100 [lintAdoc] Runtime error`, which + * 3.24.0's typed errors dropped. The program name is not host-independent: + * `.xml` reports `xsltproc not found` where the program is absent and `no XSLT + * transform provided` where it is present, and the two are split by platform — + * macOS ships `/usr/bin/xsltproc`, the Linux CI image does not. A contract + * test that matched on the program name therefore passed locally and failed in + * CI, which is how this list came to exist. * * `.xml` is also the one entry whose converter is not sufficient on its own. An * XSLT transform is document-specific, so there is no default to ship and * installing `xsltproc` does not make `.xml` lintable — unlike `asciidoctor`, * which genuinely fixes `.adoc`. That is why its `converter` names the - * stylesheet as well as the program. + * stylesheet as well as the program, and why the stylesheet message, which + * names no format, is the one case the contract test accepts in its place. */ -export const VALE_CONVERTER_CHECKERS: Readonly> = { - ".rst": "lintRST", - ".rest": "lintRST", - ".adoc": "lintAdoc", - ".asciidoc": "lintAdoc", - ".asc": "lintAdoc", - ".xml": "lintXML", - ".dita": "lintDITA", - ".typ": "lintTypst", +export const VALE_CONVERTER_READERS: Readonly> = { + ".rst": "reStructuredText", + ".rest": "reStructuredText", + ".adoc": "AsciiDoc", + ".asciidoc": "AsciiDoc", + ".asc": "AsciiDoc", + ".xml": "XML", + ".dita": "DITA", + ".typ": "Typst", }; /** Every converter-dependent extension, flattened. */ diff --git a/packages/cli/src/rules/vale/formats.ts b/packages/cli/src/rules/vale/formats.ts index 623cd601..5d43a304 100644 --- a/packages/cli/src/rules/vale/formats.ts +++ b/packages/cli/src/rules/vale/formats.ts @@ -24,9 +24,9 @@ export const TASKLESS_DIRECTORY = ".taskless"; * program to convert the source into something it can lint, and the * `@taskless/vale-*` platform packages ship the Vale binary as pure payload * with none of those programs alongside it. On a host without the converter the - * conversion fails, and Vale does not degrade: it prints one - * `E100 [lintAdoc] Runtime error` object on stderr, writes **nothing** to - * stdout, and exits 2. The abort is Vale's, not ours, and it is not scoped to + * conversion fails, and Vale does not degrade: it prints one `E100` object + * (`asciidoctor not found; it's needed to read AsciiDoc`) on stderr, writes + * **nothing** to stdout, and exits 2. The abort is Vale's, not ours, and it is not scoped to * the offending file — every finding from every other file in the same run is * lost inside Vale before it is ever serialized. * @@ -108,8 +108,8 @@ export const CONVERTER_DEPENDENT_EXTENSIONS: readonly string[] = [ * The converter Vale would need for `path`, or `undefined` if it needs none. * * Extension comparison is **case-sensitive, because Vale's own routing is**. - * Measured against the pinned binary: `docs/guide.adoc` exits 2 with - * `E100 [lintAdoc]`, while `docs/guide.ADOC` and `docs/guide.AdOc` are read as + * Measured against the pinned binary: `docs/guide.adoc` exits 2 with an + * `E100`, while `docs/guide.ADOC` and `docs/guide.AdOc` are read as * plain text and exit 0 with findings — including when the uppercase spelling * names a lowercase file on a case-insensitive filesystem, since Vale routes on * the path string it was handed, not on the name the disk holds. diff --git a/packages/cli/src/rules/vale/run.ts b/packages/cli/src/rules/vale/run.ts index b94878f0..4b4c6798 100644 --- a/packages/cli/src/rules/vale/run.ts +++ b/packages/cli/src/rules/vale/run.ts @@ -129,6 +129,39 @@ function parseValeConfigError(stderr: string): ValeConfigError | undefined { } } +/** A line Vale's `core.Warn` wrote: its code, a space, then the message. */ +const VALE_WARNING_LINE = /^W\d{3} /; + +/** + * Vale's stderr, split into the warnings it opens with and what follows. + * + * Vale writes each warning as its own `W101 ` line as it meets it, + * and a fatal error as one JSON object after them, so a run can produce both: + * since 3.24.0 a misspelled level (`. = maybe`) warns and the run then + * dies on a document it cannot parse. Parsing the whole stream as JSON failed + * on the warning, which threw away the error's `Path` and with it the + * narrowing that keeps one unparseable file from taking every other file's + * findings down (taskless/cli#300). + * + * Only the leading run of warning lines is peeled off. Anything after the + * first line that is not one is returned whole as `rest`, so a JSON object + * whose `Text` happens to contain such a line is never split. + */ +function splitValeWarnings(stderr: string): { + warnings: string[]; + rest: string; +} { + const lines = stderr.split("\n"); + const firstOther = lines.findIndex( + (line) => !VALE_WARNING_LINE.test(line.trim()) + ); + const count = firstOther === -1 ? lines.length : firstOther; + return { + warnings: lines.slice(0, count).map((line) => line.trim()), + rest: lines.slice(count).join("\n").trim(), + }; +} + /** * Vale's stderr, rendered as a sentence instead of a JSON blob. * @@ -390,13 +423,20 @@ async function spawnVale( // With --no-exit, a non-zero code is Vale failing, not Vale finding. if (code !== null && code !== 0) { - const stderr = stderrChunks.join("").trim(); - const configError = parseValeConfigError(stderr); + const { warnings, rest } = splitValeWarnings( + stderrChunks.join("").trim() + ); + const configError = parseValeConfigError(rest); + // The error first, because it is why the run stopped; the warnings + // after it, because one may be the reason (a rule switched off by a + // misspelled level). + const said = [ + ...(rest === "" ? [] : [describeValeStderr(rest, configError)]), + ...warnings, + ].join(" "); settle({ status: "failed", - message: `Vale exited ${String(code)}${ - stderr === "" ? "" : `: ${describeValeStderr(stderr, configError)}` - }`, + message: `Vale exited ${String(code)}${said === "" ? "" : `: ${said}`}`, ...(configError === undefined ? {} : { configError }), }); return; @@ -406,16 +446,20 @@ async function spawnVale( // otherwise succeeded — the `W101` ignored-assignment warning above all. // 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 ride the same field as separate elements, rather than + const { warnings, rest } = splitValeWarnings( + stderrChunks.join("").trim() + ); + // Every advisory rides the same field as its own element, 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. + // One element per warning for the same reason: each `W101` line is its + // own message, and Vale since 3.24.0 writes one per misread key. const notices = collectNotices([ skipped, - diagnostic === "" - ? undefined - : `Vale reported while running: ${diagnostic}`, + ...[...warnings, rest] + .filter((said) => said !== "") + .map((said) => `Vale reported while running: ${said}`), ]); const stdout = stdoutChunks.join("").trim(); diff --git a/packages/cli/src/schemas/vale-config.ts b/packages/cli/src/schemas/vale-config.ts index 9fc2cbc9..7fdc981f 100644 --- a/packages/cli/src/schemas/vale-config.ts +++ b/packages/cli/src/schemas/vale-config.ts @@ -503,6 +503,18 @@ export interface ValeRuleConfigVerdict { advisories: string[]; /** Every matcher's pattern, in source order, exactly as the header spells it. */ sections: string[]; + /** + * The 1-based line of every breadcrumb that could be located, in source + * order, so assembly can hide them from Vale without re-reading the text. + * + * Since Vale 3.24.0 a key Vale does not recognise inside a matcher is said + * aloud, `W101 'tskl) rule' isn't a section option`, once per matcher on + * every run. The breadcrumb is ours and Vale never needed it, so the file + * Vale reads carries it as a comment instead. A breadcrumb whose line could + * not be located is left as written: Vale then warns about it, which is + * noise, and the alternative of guessing a line is a wrong edit. + */ + breadcrumbLines: number[]; } /** @@ -545,5 +557,12 @@ export function validateValeRuleConfig( sections: ast.sections .map((section) => section.name) .filter((name): name is string => name !== undefined), + breadcrumbLines: ast.sections.flatMap((section) => + properties(section).flatMap((property) => + property.key === VALE_BREADCRUMB_KEY && property.line !== undefined + ? [property.line] + : [] + ) + ), }; } diff --git a/packages/cli/src/schemas/vale-rule.ts b/packages/cli/src/schemas/vale-rule.ts index 9ba22a54..a2aebc1d 100644 --- a/packages/cli/src/schemas/vale-rule.ts +++ b/packages/cli/src/schemas/vale-rule.ts @@ -819,6 +819,36 @@ function actionMessages( ]; } +/** + * A rule's own `tests:` cases, refused as policy rather than as vocabulary. + * + * Vale reads the key: it is a common field of every check, measured accepted + * since 3.22.0 and announced with `vale test` in 3.24.0, and the generated + * vocabulary records it as one. Taskless refuses it anyway. `verify` judges a + * Vale rule by the documents under `.tests/pass` and `.tests/fail`, which are + * whole files in any format the rule reaches, and it never runs `vale test`. A + * rule carrying cases there would read as tested while nothing ran them, and + * a second place to write a rule's cases is a second place for them to drift. + * + * `test/vale-corpus.ts` carries this as a counted divergence, so the + * disagreement with the binary stays visible. + */ +function testsMessages( + rule: Record +): { path: PropertyKey[]; message: string }[] { + if (!("tests" in rule)) return []; + return [ + { + path: ["tests"], + message: + "'tests' holds cases for `vale test`, which Taskless never runs. " + + "A Vale rule is verified by its fixtures: write each case as a " + + "document under .tests/pass (the rule must stay quiet) or " + + ".tests/fail (the rule must fire), then delete the key.", + }, + ]; +} + /** * The union's members, spelled out, and the guard that keeps them honest. * @@ -924,6 +954,7 @@ const valeBodySchema = z ...fatalShapeMessages(rule), ...swapBackreferenceMessages(rule), ...actionMessages(rule), + ...testsMessages(rule), ]) { context.issues.push({ code: "custom", input: rule, path, message }); } diff --git a/packages/cli/test/assemble.test.ts b/packages/cli/test/assemble.test.ts index 48fc30fd..105c6e2c 100644 --- a/packages/cli/test/assemble.test.ts +++ b/packages/cli/test/assemble.test.ts @@ -201,15 +201,20 @@ describe("Vale config assembly", () => { // 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 () => { + // Verbatim except the breadcrumbs, which are commented out in place: Vale + // 3.24.0 warns W101 about an unknown key in a matcher, once per matcher on + // every run. In place, so every other line keeps its number. + it("writes an accepted config verbatim under its breadcrumb, breadcrumbs commented", async () => { const source = - "# scope\n[docs/**]\ntskl) rule = no-simply\n\nno-simply.no-simply = YES \n"; + "# scope\n[docs/**]\ntskl) rule = no-simply\n\nno-simply.no-simply = YES \n\n[*.md]\n tskl) rule = no-simply\r\nno-simply.no-simply = NO\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}`); + expect(contents).toContain( + "# tskl) rule = no-simply\n# scope\n[docs/**]\n# tskl) rule = no-simply\n\nno-simply.no-simply = YES \n\n[*.md]\n# tskl) rule = no-simply\r\nno-simply.no-simply = NO\n" + ); }); // What the schema notes without refusing rides on the result, so `check` can diff --git a/packages/cli/test/vale-corpus.ts b/packages/cli/test/vale-corpus.ts index 1629a143..29d03ec9 100644 --- a/packages/cli/test/vale-corpus.ts +++ b/packages/cli/test/vale-corpus.ts @@ -492,7 +492,7 @@ const REJECTED_DOC_SCOPES: ValeCorpusEntry[] = [ ]; /** - * The one place the schema deliberately disagrees with the binary. + * Where the schema deliberately disagrees with the binary, each row saying why. * * `~fenced` does not fail — it fires on everything, because there is no * `fenced` to subtract. The binary "accepts" it in the only sense an exit code @@ -565,6 +565,20 @@ const DIVERGENCES: ValeCorpusEntry[] = [ "as a field of `conditional`; whether a View exists is a property of " + "the run's config, not of the rule, and this CLI's layout defines none.", }, + { + // Read by `vale test`, announced with it in 3.24.0 and already accepted + // by 3.22.0. The vocabulary records it, because the binary reads it. + name: "field/existence+tests", + construct: "tests", + rule: existence("tests:\n - fixture.md\n"), + control: PROSE, + expected: "accepted", + divergence: + "Vale loads the key and runs the rule. The schema refuses it as " + + "policy: `verify` judges a Vale rule by its .tests/pass and " + + ".tests/fail documents and never runs `vale test`, so cases written " + + "here would read as coverage while nothing ran them.", + }, ]; /** @@ -680,16 +694,6 @@ const FIELDS: ValeCorpusEntry[] = [ control: PROSE, expected: "accepted", }, - { - // Read by `vale test`, announced in 3.23.0 and already accepted by - // 3.22.0. Absent from the generator's candidates until 3.23.0, so - // `validate` rejected it as an E201 Vale never raised. - name: "field/existence+tests", - construct: "tests", - rule: existence("tests:\n - fixture.md\n"), - control: PROSE, - expected: "accepted", - }, { name: "field/existence+vocab", construct: "vocab", diff --git a/packages/cli/test/vale-formats.test.ts b/packages/cli/test/vale-formats.test.ts index c6772a0c..908bcf9b 100644 --- a/packages/cli/test/vale-formats.test.ts +++ b/packages/cli/test/vale-formats.test.ts @@ -100,7 +100,7 @@ describe("the format tier table", () => { }); it("reads the extension case-sensitively, exactly as Vale routes it", () => { - // Measured, not assumed: `doc.adoc` exits 2 with `E100 [lintAdoc]` while + // Measured, not assumed: `doc.adoc` exits 2 with an `E100` while // `doc.ADOC` is read as plain text and exits 0 with findings — even when // the uppercase spelling names a lowercase file on a case-insensitive // filesystem, because Vale routes on the path string it was handed. This diff --git a/packages/cli/test/vale-run.test.ts b/packages/cli/test/vale-run.test.ts index 76336eca..d4616383 100644 --- a/packages/cli/test/vale-run.test.ts +++ b/packages/cli/test/vale-run.test.ts @@ -259,6 +259,27 @@ withVale("runVale against the real binary", () => { expect(outcome.notices[0]).toContain("rules.no-simply"); }); + it("carries each warning as its own notice", async () => { + // Vale writes one `W101` line per thing it ignores, and 3.24.0 ignores + // more of them aloud: an unknown key in a matcher, a level that is not + // one. Two lines are two notices; glued, a consumer renders one marker + // and a stray line. + const cwd = makeProject( + `${header}\n[*.md]\nno-simply.no-simply = YES\nfirst stray = x\nsecond stray = y\n`, + { "no-simply": existenceRule("simply", "Avoid 'simply'") }, + { "doc.md": "Just simply do it.\n" } + ); + + const outcome = await runVale({ cwd, paths: ["doc.md"] }); + expect(outcome.status).toBe("ok"); + if (outcome.status !== "ok") return; + expect(outcome.results).toHaveLength(1); + expect(outcome.notices).toHaveLength(2); + expect(outcome.notices[0]).toContain("'first stray'"); + expect(outcome.notices[1]).toContain("'second stray'"); + for (const notice of outcome.notices) expect(notice).not.toContain("\n"); + }); + it("reports no notices when Vale writes nothing to stderr", async () => { const cwd = makeProject( `${header}\n[*.md]\nrules.no-simply = YES\n`, @@ -404,6 +425,30 @@ withVale("runVale against the real binary", () => { expect(outcome.results).toHaveLength(3); }); + it("still narrows when Vale warns before the error", async () => { + // Since 3.24.0 Vale says aloud what it ignores in a matcher, as `W101` + // lines on stderr ahead of the error's JSON. Parsing the stream whole + // lost the error's `Path`, and with it this narrowing: the run failed + // with every finding gone, the #300 failure by another route. + const cwd = makeProject( + `${header}\n[*.md]\nno-simply.no-simply = YES\nstray key = here\n`, + { "no-simply": existenceRule("simply", "Avoid 'simply'") }, + { "good.md": "Just simply do it.\n", "bad.md": badFrontMatter } + ); + + const outcome = await runVale({ cwd, paths: ["good.md", "bad.md"] }); + + expect(outcome.status).toBe("ok"); + if (outcome.status !== "ok") return; + const byFile = new Map(outcome.results.map((r) => [r.file, r])); + expect(byFile.get("good.md")).toMatchObject({ ruleId: "no-simply" }); + expect(byFile.get("bad.md")).toMatchObject({ + ruleId: "vale-parse-error", + }); + expect(outcome.notices).toHaveLength(1); + expect(outcome.notices[0]).toContain("'stray key'"); + }); + it("distinguishes a run that found nothing from a run that could not read anything", async () => { // The exact confusion from the issue: an empty `results` used to mean // both "clean corpus" and "the run never got to look at anything". diff --git a/packages/cli/test/vale-schema-contract.test.ts b/packages/cli/test/vale-schema-contract.test.ts index 5ce428f4..0546bfd7 100644 --- a/packages/cli/test/vale-schema-contract.test.ts +++ b/packages/cli/test/vale-schema-contract.test.ts @@ -272,6 +272,21 @@ withVale("Vale schema contract", () => { }); describe("errors an author can act on", () => { + it("points a rule's tests: cases at the .tests fixtures", () => { + // Every check takes the key, so the refusal cannot live in one field + // table: a permissive check would let it through. + for (const check of ["existence", "spelling"]) { + const { valid, errors } = validateValeRule( + "demo", + parseYaml( + `extends: ${check}\nmessage: "x"\nlevel: warning\ntests:\n - name: a\n input: x\n want: ""\n` + ) + ); + expect(valid, check).toBe(false); + expect(errors.join("\n")).toContain(".tests/fail"); + } + }); + it("names the field and the accepted values for an unknown extends", () => { const { errors } = validateValeRule( "demo", diff --git a/packages/cli/test/vale-vendor-contract.test.ts b/packages/cli/test/vale-vendor-contract.test.ts index 7c37ccd8..4760804f 100644 --- a/packages/cli/test/vale-vendor-contract.test.ts +++ b/packages/cli/test/vale-vendor-contract.test.ts @@ -9,7 +9,7 @@ import { findValeBinary } from "../src/rules/vale/binary"; import { asValeConfigError } from "../src/rules/vale/map"; import { VALE_COMMENT_EXTENSIONS, - VALE_CONVERTER_CHECKERS, + VALE_CONVERTER_READERS, VALE_CONVERTER_DEPENDENT, VALE_CONVERTER_DEPENDENT_EXTENSIONS, VALE_FORMAT_TIERS, @@ -1816,6 +1816,113 @@ withVale("Vale vendor contract", () => { expect(lines(existence("simply"), document, "a.py").lines).toEqual([2]); }); }); + + describe("Vale 3.24.0", () => { + // Each case below was run against 3.22.0 and 3.24.0 on the same fixture. + // The first three are what assembly and `runVale` rest on, the next two + // change which findings a rule reports, and the last is a narrowing that + // `formats.ts` does not take advantage of yet. + + it("warns W101 about a key it does not recognise inside a matcher", () => { + // Upstream's `validateLevel` now says what it ignores. Through 3.23.0 + // an unknown key in a matcher was read as a rule switched off, in + // silence, which is what made `tskl) rule = ` a safe breadcrumb. + // Now it costs one W101 per matcher on every run, and that line ahead + // of a fatal error's JSON broke the #300 narrowing. Assembly comments + // the breadcrumb out for that reason (`valeRuleBlock` in assemble.ts). + // If this ever becomes an error, the breadcrumb must leave the file + // Vale reads before the next bump. + const cwd = project( + `${header}\n[*.md]\ntskl) rule = no-simply\nrules.no-simply = YES\n`, + { "no-simply": existence("simply") }, + { "doc.md": "Just simply do it.\n" } + ); + const result = runRaw(cwd, ["doc.md"], ["--no-exit"]); + expect(result.status).toBe(0); + expect(result.stderr.trim()).toBe( + "W101 'tskl) rule' isn't a section option; Vale is ignoring it." + ); + const parsed = JSON.parse(result.stdout) as Record; + expect(parsed["doc.md"]).toHaveLength(1); + }); + + it("says nothing about a commented breadcrumb", () => { + // The other half of the assembly fix: the commented form is silent. + const cwd = project( + `${header}\n[*.md]\n# tskl) rule = no-simply\nrules.no-simply = YES\n`, + { "no-simply": existence("simply") }, + { "doc.md": "Just simply do it.\n" } + ); + const result = runRaw(cwd, ["doc.md"], ["--no-exit"]); + expect(result.status).toBe(0); + expect(result.stderr).toBe(""); + }); + + it("writes its warnings first, one per line, then a fatal error's JSON", () => { + // The order `splitValeWarnings` in run.ts rests on. A misspelled level + // is new in 3.24.0 too: read as off, as before, but now said. + const cwd = project( + `${header}\n[*.md]\nstray key = x\nrules.no-simply = YES\nrules.other = maybe\n`, + { "no-simply": existence("simply") }, + { + "doc.md": "Just simply do it.\n", + "bad.md": "---\nd: a: b\n---\n\nsimply\n", + } + ); + const result = runRaw(cwd, ["doc.md", "bad.md"], ["--no-exit"]); + expect(result.status).toBe(2); + const [first, second, ...rest] = result.stderr.split("\n"); + expect(first).toBe( + "W101 'stray key' isn't a section option; Vale is ignoring it." + ); + expect(second).toBe( + "W101 'rules.other = maybe' isn't YES, NO, UNSET, or a level; Vale is switching the rule off." + ); + const error = JSON.parse(rest.join("\n")) as Record; + expect(error).toMatchObject({ Code: "E201", Path: "bad.md" }); + }); + + it("drops a C# ReSharper or NOSONAR comment", () => { + // Upstream 8cbda873 gave C# its own grammar; 3.23.0 read `.cs` with the + // C one, which is why the tier did not move. What moved is the 3.23.0 + // directive masking reaching C#: both lines below were linted as prose. + // The plain comment is the control. + expect( + lines( + existence("simply"), + "// ReSharper disable simply\n// NOSONAR simply\n// simply here\nint x = 1;\n", + "a.cs" + ).lines + ).toEqual([3]); + }); + + it("keeps a non-breaking space after a link as a word break", () => { + // Upstream 84deb6c3. Through 3.23.0 the U+00A0 after inline markup was + // dropped, fusing `there` and `simply` into one word, so a token after + // a link went unmatched. It now fires. + expect( + lines(existence("simply"), "Go [there](x)\u00A0simply now.\n").lines + ).toEqual([1]); + }); + + it("does not parse a file no rule runs on, converter or not", () => { + // Upstream bc8b91d8. Through 3.23.0 a file was parsed whenever any + // matcher named a rule for it, even to switch it off, so an `.adoc` + // under `[*.adoc] rule = NO` still aborted the run. Now it is skipped + // unparsed. `formats.ts` still excludes every converter-dependent file + // by extension, which stays correct: a rule that IS on still crashes. + const cwd = project( + `${header}\n[*]\nrules.no-simply = YES\n\n[*.adoc]\nrules.no-simply = NO\n`, + { "no-simply": existence("simply") }, + { "doc.md": "Just simply do it.\n", "doc.adoc": "We simply do it.\n" } + ); + const result = runRaw(cwd, ["doc.md", "doc.adoc"], ["--no-exit"]); + expect(result.status, result.stderr).toBe(0); + const parsed = JSON.parse(result.stdout) as Record; + expect(parsed["doc.md"]).toHaveLength(1); + expect(parsed["doc.adoc"]).toBeUndefined(); + }); + }); }); /** @@ -2138,8 +2245,8 @@ withVale("Vale engine capabilities", () => { expect(result.status, `${extension} no longer fails`).not.toBe(0); const output = `${result.stdout}${result.stderr}`; expect(output).toContain("E100"); - // Assert the checker tag, not the prose after it. The tag is the same - // everywhere; the prose is not — `.xml` says `xsltproc not found` where + // Assert the format Vale names, not the program. The format is the same + // everywhere; the program is not — `.xml` says `xsltproc not found` where // the program is absent and `no XSLT transform provided` where it is // present, and macOS ships `/usr/bin/xsltproc` while the Linux CI image // does not. Deriving the expectation from our own `converter` string @@ -2147,7 +2254,17 @@ withVale("Vale engine capabilities", () => { // in CI on exactly that split — a host-dependent assertion dressed up as a // vendor contract. `converter` is still asserted, one level up, against // our own data where no binary is involved. - expect(output).toContain(`[${VALE_CONVERTER_CHECKERS[extension]!}]`); + // + // Through 3.23.0 this asserted the checker tag, `[lintAdoc]`. 3.24.0's + // typed errors dropped it and name the format in the sentence instead; + // the run still dies the same way, which is the half that matters. + const stylesheetMissing = + extension === ".xml" && output.includes("no XSLT transform provided"); + if (!stylesheetMissing) { + expect(output).toContain( + `it's needed to read ${VALE_CONVERTER_READERS[extension]!}` + ); + } }); it("routes to a converter by EXACT-case extension", () => { @@ -2172,10 +2289,10 @@ withVale("Vale engine capabilities", () => { } }); - it("names a checker for every converter-dependent extension", () => { + it("names a format for every converter-dependent extension", () => { // Set-equality, so an extension added to one and not the other fails here - // rather than throwing on an undefined tag inside the probe above. - expect(Object.keys(VALE_CONVERTER_CHECKERS).toSorted()).toEqual( + // rather than asserting on `undefined` inside the probe above. + expect(Object.keys(VALE_CONVERTER_READERS).toSorted()).toEqual( [...VALE_CONVERTER_DEPENDENT_EXTENSIONS].toSorted() ); }); From 243c24f6d7922fd8e13f7415213e551c0eed849f Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 20 Oct 2026 17:06:36 -0700 Subject: [PATCH 2/2] feat(update): move a Vale rule's tests: cases into fixtures on 3.24.0 `validate` refuses a Vale rule's `tests:` key from 3.24.0 on, and 0.12.0 had accepted it. A rule migration keyed on Vale 3.24.0 now writes each case's input as a fixture under .tests/fail when it expects an alert and .tests/pass when it expects none, named for the case with its format as the extension, then deletes the block by its byte range. It never overwrites a fixture, reruns as a no-op, and leaves a rule whose cases read through a view or name another rule file untouched. No issued rule carries the key. --- .changeset/vale-3-24-0.md | 2 +- packages/cli/src/agent/update.md | 30 ++- packages/cli/src/rules/rule-migrate.ts | 6 +- .../vale-3-24-0-tests-to-fixtures.ts | 253 ++++++++++++++++++ packages/cli/test/reconcile-marker.test.ts | 50 ++++ packages/cli/test/rule-migrate.test.ts | 233 +++++++++++++++- 6 files changed, 559 insertions(+), 15 deletions(-) create mode 100644 packages/cli/src/rules/rule-migrations/vale-3-24-0-tests-to-fixtures.ts diff --git a/.changeset/vale-3-24-0.md b/.changeset/vale-3-24-0.md index 9fd0aa15..f365aff8 100644 --- a/.changeset/vale-3-24-0.md +++ b/.changeset/vale-3-24-0.md @@ -8,4 +8,4 @@ Vale 3.24.0 warns about any key it does not recognise inside a matcher, includin Two findings change for existing rules: C# `// ReSharper disable` and `// NOSONAR` comments are no longer linted, and a word after a link and a non-breaking space is now matched. `update` (topic v18) walks both. -`validate` and `verify` now reject a Vale rule's `tests:` key. Vale 3.24.0 runs those cases with `vale test`, which Taskless never calls; a Vale rule is verified by its `.tests/pass` and `.tests/fail` documents. +`validate` and `verify` now reject a Vale rule's `tests:` key. Vale 3.24.0 runs those cases with `vale test`, which Taskless never calls; a Vale rule is verified by its `.tests/pass` and `.tests/fail` documents. `update --rules` moves each case into one of those fixtures for you. diff --git a/packages/cli/src/agent/update.md b/packages/cli/src/agent/update.md index ef079eb8..7b1f20fd 100644 --- a/packages/cli/src/agent/update.md +++ b/packages/cli/src/agent/update.md @@ -498,8 +498,7 @@ loaded it without complaint. ### Migrating to 0.13.3 The vendored ast-grep moves from 0.45.3 to 0.51.0, and Vale from 3.23.0 -to 3.24.0. Nothing here needs work by hand except a Vale rule carrying -a `tests:` key, described at the end. +to 3.24.0. Nothing here needs work by hand. **A Haskell type synonym is `type_synonym` now, not `type_synomym`, and `update --rules` renames it for you.** ast-grep's Haskell grammar moved @@ -553,17 +552,24 @@ Additions a rule config can use: a matcher may carry a condition, `[*.md if .Meta.draft]`, which applies it only to the matched files whose front matter satisfies it. `validate` accepts it. -**A rule's `tests:` key is refused.** Vale 3.24.0 ships `vale test`, -which runs cases written in a rule's `tests:` list. Taskless never runs -it: `verify` judges a Vale rule by the documents under `.tests/pass` -and `.tests/fail`. So `validate` and `verify` now reject the key, which +**A Vale rule's `tests:` cases move into its fixtures, and +`update --rules` moves them for you.** Vale 3.24.0 ships `vale test`, which runs +cases written in a rule's `tests:` list. Taskless never runs it: +`verify` judges a Vale rule by the documents under `.tests/pass` and +`.tests/fail`. So `validate` and `verify` now reject the key, which 0.12.0 had started accepting, rather than let a rule look tested by -cases nothing runs. `check` still runs such a rule. Search -`.taskless/rules/vale/*/*.yml` for a top-level `tests:`; for each case, -write its `input` as a document under `.tests/fail` if the case expects -the rule to fire, or `.tests/pass` if it expects no alert, named with -the case's `format` as its extension (`.md` when it has none). Then -delete the key and run `verify` on the rule. +cases nothing runs. `check` still runs such a rule. + +Do not move them by hand. Step 5 writes each case's `input` as a +document named for the case, under `.tests/fail` when the case expects +an alert and `.tests/pass` when it expects none, with the case's +`format` as its extension. It then deletes the `tests:` block, leaves +everything else in the rule as it was, never overwrites a fixture, and +names each file it wrote. No rule Taskless issued carries `tests:`, so +it never touches a signed rule. A rule with a case that reads through a +`view`, or names another rule file in `rule:`, is left as it is, and +`validate` keeps naming it; move that rule's cases by hand the same +way. ## Errors diff --git a/packages/cli/src/rules/rule-migrate.ts b/packages/cli/src/rules/rule-migrate.ts index 7d585a96..c5c70452 100644 --- a/packages/cli/src/rules/rule-migrate.ts +++ b/packages/cli/src/rules/rule-migrate.ts @@ -2,6 +2,7 @@ import { join } from "node:path"; import { AST_GREP_VERSION, VALE_VERSION } from "./capabilities"; import haskellTypeSynonym from "./rule-migrations/sg-0-50-0-haskell-type-synonym"; +import valeTestsToFixtures from "./rule-migrations/vale-3-24-0-tests-to-fixtures"; import { compareVersions } from "../util/version-compare"; /** @@ -60,7 +61,10 @@ export interface AppliedRuleMigration { files: string[]; } -const RULE_MIGRATIONS: readonly RuleMigration[] = [haskellTypeSynonym]; +const RULE_MIGRATIONS: readonly RuleMigration[] = [ + haskellTypeSynonym, + valeTestsToFixtures, +]; const BUNDLED: Record = { sg: AST_GREP_VERSION, diff --git a/packages/cli/src/rules/rule-migrations/vale-3-24-0-tests-to-fixtures.ts b/packages/cli/src/rules/rule-migrations/vale-3-24-0-tests-to-fixtures.ts new file mode 100644 index 00000000..e87a23ea --- /dev/null +++ b/packages/cli/src/rules/rule-migrations/vale-3-24-0-tests-to-fixtures.ts @@ -0,0 +1,253 @@ +import { mkdir, readdir, readFile, writeFile } from "node:fs/promises"; +import { join } from "node:path"; + +import { isMap, isScalar, isSeq, parseAllDocuments } from "yaml"; + +import { RULE_TESTS_DIRECTORY } from "../layout"; +import type { RuleMigration } from "../rule-migrate"; + +/** + * Move a Vale rule's `tests:` cases into `.tests/pass` and `.tests/fail`. + * + * Vale 3.24.0 runs the cases a rule carries under `tests:` with `vale test`. + * Taskless never runs it: `verify` judges a Vale rule by the documents under + * `.tests/`, and `validate` refuses the key from 3.24.0 on, so a rule written + * against 0.12.0, which accepted it, would otherwise stop validating. + * + * A case is one document and whether the rule fires on it, which is exactly + * what a fixture is. An in-rule case loads only the rule it lives in, so any + * alert it expects is that rule's: + * + * | the case | becomes | + * | ------------------------------ | ---------------- | + * | `want` with any text | `.tests/fail/` | + * | `want` that is empty | `.tests/pass/` | + * | no `want`, but `contains` | `.tests/fail/` | + * | only `absent` | `.tests/pass/` | + * + * What `want` pins beyond that, the exact line, column and message, is not + * something a rule's verification needs, and it is not carried. + * + * The fixture holds the case's `input` byte for byte, is named for the case, + * and takes its extension from `format` (`md` when there is none, as Vale + * reads it). A rule with a case this cannot place is left exactly as it is: + * one that reads its input through a `view`, which this layout never defines, + * or that names another rule file in `rule:`. `validate` keeps refusing that + * rule, and its message says what to do. + * + * The `tests:` block is removed by the parser's byte range, not by + * re-serializing the file, so everything else in the rule stays as written. + * An existing fixture with the same bytes counts as written, which is what + * makes a rerun after a partial failure a no-op; one with other bytes is + * never overwritten, and the case takes the next free name. + * + * The Taskless service has issued no Vale rule carrying `tests:` (confirmed + * against its corpus, 2026-10-20), so no issued rule's signature is affected. + */ + +/** One case, reduced to the two facts a fixture records. */ +interface PlacedCase { + bucket: "pass" | "fail"; + name: string; + extension: string; + input: string; +} + +/** A rule this migration rewrites: its cases, and the block to remove. */ +interface Plan { + cases: PlacedCase[]; + blockStart: number; + blockEnd: number; +} + +function text(value: unknown): string | undefined { + return typeof value === "string" ? value : undefined; +} + +/** Whether `contains` or `absent` says anything: one excerpt, or a list. */ +function listed(value: unknown): boolean { + return ( + typeof value === "string" || (Array.isArray(value) && value.length > 0) + ); +} + +/** A case's bucket, or `undefined` when it asserts nothing Vale would run. */ +function bucketOf( + item: Record +): PlacedCase["bucket"] | undefined { + const want = text(item.want); + if (want !== undefined) return want.trim() === "" ? "pass" : "fail"; + if (listed(item.contains)) return "fail"; + if (listed(item.absent)) return "pass"; + return undefined; +} + +/** `flags a hedge` as `flags-a-hedge`; `case-` when nothing survives. */ +function slug(name: string | undefined, index: number): string { + const cleaned = (name ?? "") + .toLowerCase() + .replaceAll(/[^a-z0-9]+/g, "-") + .replaceAll(/^-+|-+$/g, ""); + return cleaned === "" ? `case-${String(index + 1)}` : cleaned; +} + +/** + * What to do with one rule file, or `undefined` to leave it alone. + * + * Alone means: not a single-document rule with a top-level `tests` list, or + * one whose cases cannot all be placed. Partly migrating a rule would leave + * it refused by `validate` with half its cases moved, which is harder to + * finish by hand than none. + */ +export function planTestsMigration( + source: string, + ruleFile: string +): Plan | undefined { + let documents; + try { + documents = parseAllDocuments(source); + } catch { + return undefined; + } + if (!Array.isArray(documents) || documents.length !== 1) return undefined; + const [document] = documents; + if (document === undefined || document.errors.length > 0) return undefined; + const root = document.contents; + if (!isMap(root) || !root.has("extends")) return undefined; + const pair = root.items.find( + (item) => isScalar(item.key) && item.key.value === "tests" + ); + if ( + pair === undefined || + !isScalar(pair.key) || + !isSeq(pair.value) || + pair.key.range === undefined || + pair.key.range === null || + pair.value.range === undefined || + pair.value.range === null + ) { + return undefined; + } + + const items: unknown = pair.value.toJSON(); + if (!Array.isArray(items)) return undefined; + const cases: PlacedCase[] = []; + for (const [index, raw] of items.entries()) { + if (typeof raw !== "object" || raw === null || Array.isArray(raw)) { + return undefined; + } + const item = raw as Record; + if (item.view !== undefined && item.view !== null) return undefined; + const rule = text(item.rule); + if (rule !== undefined && rule !== "" && rule !== ruleFile) { + return undefined; + } + const bucket = bucketOf(item); + if (bucket === undefined) return undefined; + const format = text(item.format) ?? ""; + cases.push({ + bucket, + name: slug(text(item.name), index), + extension: format.replace(/^\./, "") || "md", + input: text(item.input) ?? "", + }); + } + + // From the start of the key's line through the end of the value's line. + const keyStart = pair.key.range[0]; + const blockStart = source.lastIndexOf("\n", keyStart - 1) + 1; + let blockEnd = pair.value.range[1]; + if (blockEnd > 0 && source[blockEnd - 1] !== "\n") { + const newline = source.indexOf("\n", blockEnd); + blockEnd = newline === -1 ? source.length : newline + 1; + } + return { cases, blockStart, blockEnd }; +} + +async function readExisting(path: string): Promise { + try { + return await readFile(path, "utf8"); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") return undefined; + throw error; + } +} + +/** + * Write one case's fixture, returning its path relative to the rule + * directory, or `undefined` when identical bytes were already there. + */ +async function writeFixture( + ruleDirectory: string, + placed: PlacedCase, + taken: Set +): Promise { + const bucket = join(RULE_TESTS_DIRECTORY, placed.bucket); + await mkdir(join(ruleDirectory, bucket), { recursive: true }); + for (let attempt = 1; ; attempt += 1) { + const stem = + attempt === 1 ? placed.name : `${placed.name}-${String(attempt)}`; + const relative = join(bucket, `${stem}.${placed.extension}`); + if (taken.has(relative)) continue; + const existing = await readExisting(join(ruleDirectory, relative)); + if (existing === placed.input) { + taken.add(relative); + return undefined; + } + if (existing !== undefined) continue; + taken.add(relative); + await writeFile(join(ruleDirectory, relative), placed.input, "utf8"); + return relative; + } +} + +const migration: RuleMigration = { + engine: "vale", + engineVersion: "3.24.0", + summary: + "Vale rule `tests:` cases moved into `.tests/pass` and `.tests/fail`", + async apply(engineDirectory) { + let ruleDirectories; + try { + ruleDirectories = await readdir(engineDirectory, { withFileTypes: true }); + } catch (error) { + // No Vale rules tree: nothing to move. Anything else has to fail, not + // read as "nothing to do": the stamp that follows would record this + // migration as applied, and no later run would retry it. + if ((error as NodeJS.ErrnoException).code === "ENOENT") return []; + throw error; + } + const written: string[] = []; + for (const ruleDirectory of ruleDirectories) { + if (!ruleDirectory.isDirectory()) continue; + const directory = join(engineDirectory, ruleDirectory.name); + const files = await readdir(directory, { withFileTypes: true }); + for (const file of files) { + if (!file.isFile() || !/\.ya?ml$/.test(file.name)) continue; + const path = join(directory, file.name); + const source = await readFile(path, "utf8"); + const plan = planTestsMigration(source, file.name); + if (plan === undefined) continue; + // Fixtures first, rule last: a failure in between leaves the cases + // in the rule, so the rerun finds them, sees its own fixtures, and + // only finishes the removal. + const taken = new Set(); + for (const placed of plan.cases) { + const fixture = await writeFixture(directory, placed, taken); + if (fixture !== undefined) { + written.push(join(ruleDirectory.name, fixture)); + } + } + await writeFile( + path, + source.slice(0, plan.blockStart) + source.slice(plan.blockEnd), + "utf8" + ); + written.push(join(ruleDirectory.name, file.name)); + } + } + return written; + }, +}; + +export default migration; diff --git a/packages/cli/test/reconcile-marker.test.ts b/packages/cli/test/reconcile-marker.test.ts index c0a6f1ab..690bca51 100644 --- a/packages/cli/test/reconcile-marker.test.ts +++ b/packages/cli/test/reconcile-marker.test.ts @@ -201,6 +201,56 @@ describe("recording a rules reconciliation", () => { vale: VALE_VERSION, }); }); + + it("moves a Vale rule's tests: cases into fixtures when it crosses 3.24.0", async () => { + const ruleDirectory = join(cwd, ".taskless", "rules", "vale", "hedge"); + await mkdir(ruleDirectory, { recursive: true }); + await writeFile( + join(ruleDirectory, "hedge.yml"), + 'extends: existence\nmessage: "x"\nlevel: warning\ntokens:\n - perhaps\n' + + "tests:\n - name: flags it\n input: Perhaps.\n contains: x\n" + ); + const manifestPath = join(cwd, ".taskless", "taskless.json"); + const manifest = JSON.parse( + await readFile(manifestPath, "utf8") + ) as Record; + manifest["rules"] = { + reconciledTo: "0.11.0", + engines: { sg: AST_GREP_VERSION, vale: "3.23.0" }, + }; + await writeFile(manifestPath, JSON.stringify(manifest, undefined, 2)); + + const result = await runCli(["update", "--rules", "--json", "-d", cwd]); + expect(result.exitCode).toBe(0); + const payload = JSON.parse(result.stdout) as { + migrated: { engineVersion: string; files: string[] }[]; + }; + expect(payload.migrated).toContainEqual( + expect.objectContaining({ + engineVersion: "3.24.0", + files: [ + join( + ".taskless", + "rules", + "vale", + "hedge", + ".tests", + "fail", + "flags-it.md" + ), + join(".taskless", "rules", "vale", "hedge", "hedge.yml"), + ], + }) + ); + expect( + await readFile(join(ruleDirectory, "hedge.yml"), "utf8") + ).not.toContain("tests:"); + const rules = await readRules(); + expect(rules?.engines).toEqual({ + sg: AST_GREP_VERSION, + vale: VALE_VERSION, + }); + }); }); it("refuses when there is no .taskless/ to reconcile", async () => { diff --git a/packages/cli/test/rule-migrate.test.ts b/packages/cli/test/rule-migrate.test.ts index 46894129..1dc187e9 100644 --- a/packages/cli/test/rule-migrate.test.ts +++ b/packages/cli/test/rule-migrate.test.ts @@ -2,8 +2,10 @@ import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { parse as parseYaml } from "yaml"; import { AST_GREP_VERSION, VALE_VERSION } from "../src/rules/capabilities"; +import { validateValeRule } from "../src/schemas/vale-rule"; import { applyRuleMigrations, pendingRuleMigrations, @@ -12,6 +14,9 @@ import { import haskellTypeSynonym, { renameTypeSynonymKind, } from "../src/rules/rule-migrations/sg-0-50-0-haskell-type-synonym"; +import valeTestsToFixtures, { + planTestsMigration, +} from "../src/rules/rule-migrations/vale-3-24-0-tests-to-fixtures"; const noop = ( engine: RuleMigration["engine"], @@ -125,6 +130,7 @@ describe("applying the Haskell rename to a project", () => { await rule("syn", haskell("Haskell")); const applied = await applyRuleMigrations(tasklessDirectory, { sg: "0.45.3", + vale: VALE_VERSION, }); expect(applied).toEqual([ { @@ -184,8 +190,233 @@ describe("applying the Haskell rename to a project", () => { it("does not run once the recorded engine has crossed it", async () => { await rule("syn", haskell("Haskell")); expect( - await applyRuleMigrations(tasklessDirectory, { sg: AST_GREP_VERSION }) + await applyRuleMigrations(tasklessDirectory, { + sg: AST_GREP_VERSION, + vale: VALE_VERSION, + }) ).toEqual([]); expect(await read("syn")).toBe(haskell("Haskell")); }); }); + +/** A Vale rule with `tests` spliced in where the caller puts it. */ +const valeRule = (tests: string, after = "level: warning\n") => + `extends: existence\nmessage: "Avoid '%s'"\n# kept\n${tests}${after}tokens:\n - simply\n`; + +describe("planning the Vale tests: move", () => { + const without = valeRule(""); + + it.each([ + ["an indented list", "tests:\n - name: a\n input: x\n want: ''\n"], + [ + "a list at the key's own column", + "tests:\n- name: a\n input: x\n want: ''\n", + ], + [ + "a list with a blank line inside", + "tests:\n - name: a\n\n input: x\n want: ''\n", + ], + ])("removes %s and keeps every other byte", (_shape, tests) => { + const source = valeRule(tests); + const plan = planTestsMigration(source, "r.yml"); + expect(plan).toBeDefined(); + if (plan === undefined) return; + expect(source.slice(0, plan.blockStart) + source.slice(plan.blockEnd)).toBe( + without + ); + }); + + it("removes a block that ends the file with no newline", () => { + const source = + 'extends: existence\nmessage: "x"\ntests:\n - name: a\n want: ""'; + const plan = planTestsMigration(source, "r.yml"); + expect(plan).toBeDefined(); + if (plan === undefined) return; + expect(source.slice(0, plan.blockStart)).toBe( + 'extends: existence\nmessage: "x"\n' + ); + expect(plan.blockEnd).toBe(source.length); + }); + + it.each([ + ["want with text", "want: '1:1:r.r:Avoid'", "fail"], + ["an empty want", "want: ''", "pass"], + ["an empty block want", "want: |\n", "pass"], + ["contains", "contains: r.r", "fail"], + ["a contains list", "contains:\n - Avoid", "fail"], + ["only absent", "absent: r.r", "pass"], + ["want beside contains", "want: ''\n contains: x", "pass"], + ])("files a case with %s under %s", (_shape, assertion, bucket) => { + const plan = planTestsMigration( + valeRule(`tests:\n - name: a\n input: x\n ${assertion}\n`), + "r.yml" + ); + expect(plan?.cases[0]?.bucket).toBe(bucket); + }); + + it("names the fixture for the case and reads its format as Vale does", () => { + const plan = planTestsMigration( + valeRule( + "tests:\n - name: Flags a hedge!\n input: x\n format: .rst\n want: ''\n" + + " - name: b\n input: y\n format: txt\n want: ''\n" + + " - input: z\n want: ''\n" + ), + "r.yml" + ); + expect(plan?.cases.map((c) => `${c.name}.${c.extension}`)).toEqual([ + "flags-a-hedge.rst", + "b.txt", + "case-3.md", + ]); + }); + + it.each([ + ["reads through a view", "view: Docs\n want: ''"], + ["names another rule file", "rule: ../other/other.yml\n want: ''"], + ["asserts nothing", "about: no assertion"], + ])("leaves a rule alone when a case %s", (_shape, extra) => { + expect( + planTestsMigration( + valeRule(`tests:\n - name: a\n input: x\n ${extra}\n`), + "r.yml" + ) + ).toBeUndefined(); + }); + + it("accepts a case that names its own rule file", () => { + expect( + planTestsMigration( + valeRule("tests:\n - name: a\n rule: r.yml\n want: ''\n"), + "r.yml" + ) + ).toBeDefined(); + }); + + it.each([ + ["no tests key", valeRule("")], + ["no extends", "message: x\ntests:\n - name: a\n want: ''\n"], + [ + "two documents", + `${valeRule("tests:\n - name: a\n want: ''\n")}---\nextends: x\n`, + ], + ["a tests key that is not a list", valeRule("tests: fixture.md\n")], + ])("is not a rule to move with %s", (_shape, source) => { + expect(planTestsMigration(source, "r.yml")).toBeUndefined(); + }); +}); + +describe("applying the Vale tests: move to a project", () => { + let tasklessDirectory: string; + const vale = () => join(tasklessDirectory, "rules", "vale"); + const path = (...segments: string[]) => join(vale(), ...segments); + + const source = valeRule( + "tests:\n - name: flags it\n input: Just simply do it.\n contains: Avoid\n" + + " - name: quiet\n input: |\n Just do it.\n want: ''\n" + ); + + async function rule(id: string, body: string) { + await mkdir(path(id), { recursive: true }); + await writeFile(path(id, `${id}.yml`), body); + } + + beforeEach(async () => { + tasklessDirectory = join( + await mkdtemp(join(tmpdir(), "rule-migrate-vale-")), + ".taskless" + ); + }); + + afterEach(async () => { + await rm(join(tasklessDirectory, ".."), { recursive: true, force: true }); + }); + + it("writes each case as a fixture, removes the key, and reports both", async () => { + await rule("no-simply", source); + const written = await valeTestsToFixtures.apply(vale()); + expect(written).toEqual([ + join("no-simply", ".tests", "fail", "flags-it.md"), + join("no-simply", ".tests", "pass", "quiet.md"), + join("no-simply", "no-simply.yml"), + ]); + expect( + await readFile(path("no-simply", ".tests", "fail", "flags-it.md"), "utf8") + ).toBe("Just simply do it."); + expect( + await readFile(path("no-simply", ".tests", "pass", "quiet.md"), "utf8") + ).toBe("Just do it.\n"); + const migrated = await readFile(path("no-simply", "no-simply.yml"), "utf8"); + expect(migrated).toBe(valeRule("")); + // The point of the move: the rule validates again. + expect(validateValeRule("no-simply", parseYaml(migrated)).valid).toBe(true); + }); + + it("writes nothing on a second run", async () => { + await rule("no-simply", source); + await valeTestsToFixtures.apply(vale()); + expect(await valeTestsToFixtures.apply(vale())).toEqual([]); + }); + + it("finishes a run interrupted after the fixtures, without duplicating them", async () => { + await rule("no-simply", source); + await mkdir(path("no-simply", ".tests", "fail"), { recursive: true }); + await writeFile( + path("no-simply", ".tests", "fail", "flags-it.md"), + "Just simply do it." + ); + expect(await valeTestsToFixtures.apply(vale())).toEqual([ + join("no-simply", ".tests", "pass", "quiet.md"), + join("no-simply", "no-simply.yml"), + ]); + }); + + it("never overwrites a different fixture of the same name", async () => { + await rule("no-simply", source); + await mkdir(path("no-simply", ".tests", "fail"), { recursive: true }); + await writeFile(path("no-simply", ".tests", "fail", "flags-it.md"), "mine"); + const written = await valeTestsToFixtures.apply(vale()); + expect(written).toContain( + join("no-simply", ".tests", "fail", "flags-it-2.md") + ); + expect( + await readFile(path("no-simply", ".tests", "fail", "flags-it.md"), "utf8") + ).toBe("mine"); + }); + + it("leaves a rule it cannot place untouched", async () => { + const body = valeRule( + "tests:\n - name: a\n view: Docs\n want: ''\n" + ); + await rule("viewed", body); + expect(await valeTestsToFixtures.apply(vale())).toEqual([]); + expect(await readFile(path("viewed", "viewed.yml"), "utf8")).toBe(body); + }); + + it("is a no-op on a project with no Vale rules", async () => { + expect(await valeTestsToFixtures.apply(vale())).toEqual([]); + }); + + it("fails, rather than reporting nothing to do, when the tree is unreadable", async () => { + await mkdir(join(tasklessDirectory, "rules"), { recursive: true }); + await writeFile(vale(), "not a directory"); + await expect(valeTestsToFixtures.apply(vale())).rejects.toThrow(); + }); + + it("runs for rules recorded below 3.24.0 and not after", async () => { + await rule("no-simply", source); + expect( + await applyRuleMigrations(tasklessDirectory, { + sg: AST_GREP_VERSION, + vale: VALE_VERSION, + }) + ).toEqual([]); + const applied = await applyRuleMigrations(tasklessDirectory, { + sg: AST_GREP_VERSION, + vale: "3.23.0", + }); + expect(applied.map((m) => m.engineVersion)).toEqual(["3.24.0"]); + expect(applied[0]?.files).toContain( + join(".taskless", "rules", "vale", "no-simply", "no-simply.yml") + ); + }); +});