From aea735fa6d9a23c8aa45c372faa72c55d8cbeb49 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 10 Aug 2026 17:31:30 -0700 Subject: [PATCH 1/4] feat(cli): verify Vale rules from per-rule fixture directories MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unit 2, tasks 1.4 and the verify half of 1.5. For each .taskless/vale/rule-tests//, generate an isolating config, run Vale over its pass/ and fail/ fixtures, and assert every fail/ fires and every pass/ is clean. The generated config is isolating, not merely minimal, and three details in it are load-bearing. StylesPath is absolute, because the config is written to a temp directory and Vale resolves StylesPath relative to the config file. BasedOnStyles is empty, so none of Vale's bundled styles load — otherwise a fixture could trip Vale.Spelling and be counted as the rule under test firing. And the rule is enabled by exactly one assignment in one matcher: precedence is positional, so a config that assigned the key twice would be leaning on the very semantics that the scoping spec got wrong. The expected set comes from disk, not from Vale's output. A fixture that produces no finding is absent from the payload entirely, so a missing fail/ finding is unobservable in the output alone — which is precisely the case the verifier exists to catch. Two states that could each have shipped as false confidence: - A rule with no fixtures reports `empty`, not `passed`. An empty directory proves nothing, and calling that a pass is how an unverified rule ships looking verified. - An unavailable or failed Vale stops the pass rather than being recorded per rule. With no binary every rule reports no findings, indistinguishable from every rule being broken, and a wall of verification failures would send someone to debug rules over a missing install. Writing the fixtures turned up that Vale's `existence` is case-sensitive by default — `Simply` does not match the token `simply`. It first read as a bug in the verifier rather than as fixtures that never matched, so it is now pinned in the vendor contract alongside the rest of Vale's observable behaviour. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3 --- .../changes/add-vale-rule-engine/tasks.md | 4 +- packages/cli/src/rules/vale/verify.ts | 210 +++++++++++++++ .../cli/test/vale-vendor-contract.test.ts | 18 ++ packages/cli/test/vale-verify.test.ts | 254 ++++++++++++++++++ 4 files changed, 484 insertions(+), 2 deletions(-) create mode 100644 packages/cli/src/rules/vale/verify.ts create mode 100644 packages/cli/test/vale-verify.test.ts diff --git a/openspec/changes/add-vale-rule-engine/tasks.md b/openspec/changes/add-vale-rule-engine/tasks.md index 3f7fb13..61004a1 100644 --- a/openspec/changes/add-vale-rule-engine/tasks.md +++ b/openspec/changes/add-vale-rule-engine/tasks.md @@ -8,8 +8,8 @@ - [x] 1.1 Extract the platform-binary resolution in `findSgBinary()` (`rules/scan.ts:38-61`) into a shared helper — resolve `/package.json` via `createRequire(import.meta.url)`, exec the binary beside it, fall back to `PATH` — and use it for both `sg` and `vale`. Parameterize the package name rather than reusing ast-grep's: `add-vale-binary-packages` ships `@taskless/vale--` with no libc suffix, so the `-gnu` mapping `findSgBinary()` applies to every Linux must not be applied to the Vale lookup. When nothing resolves, report the Vale engine unavailable without aborting other engines (D6b) - [x] 1.2 Add `rules/vale/run.ts`: run `vale --config .taskless/vale/.vale.ini --output=JSON --no-exit `, bounded by a subprocess timeout that terminates and reports on expiry - [x] 1.3 Map Vale JSON findings → `CheckResult`: `source: "vale"`, `ruleId` = check name with `rules.` stripped, severity `error/warning/suggestion → error/warning/hint`, and `message`/`note`/`range`/`matchedText`/`fix` per the mapping -- [ ] 1.4 Add `rules/vale/verify.ts`: for each `vale/rule-tests//`, generate an ephemeral `.vale.ini` enabling only that rule, run Vale over `pass/`/`fail/` fixtures, assert every `fail/` yields a finding and every `pass/` none -- [ ] 1.5 Tests: `rules.` stripping + severity mapping; committed-config scoping respected (include union, exclude disable, duplicate matchers merge); verify pass/fail; missing-binary and timeout paths. **Partially complete — the verify half belongs with 1.4 and lands in unit 2.** Done in unit 1: `rules.` stripping and severity mapping (`test/vale-map.test.ts`), and against the real binary (`test/vale-run.test.ts`) all three scoping cases, the missing-binary path, and the timeout path +- [x] 1.4 Add `rules/vale/verify.ts`: for each `vale/rule-tests//`, generate an ephemeral `.vale.ini` enabling only that rule, run Vale over `pass/`/`fail/` fixtures, assert every `fail/` yields a finding and every `pass/` none +- [x] 1.5 Tests: `rules.` stripping + severity mapping; committed-config scoping respected (include union, exclude disable, duplicate matchers merge); verify pass/fail; missing-binary and timeout paths. Unit 1 covered `rules.` stripping and severity mapping (`test/vale-map.test.ts`), and against the real binary (`test/vale-run.test.ts`) the scoping cases, missing-binary, and timeout. Unit 2 adds verify pass/fail (`test/vale-verify.test.ts`), including rule isolation and the empty-fixture case. Vale's own behaviour is pinned separately in `test/vale-vendor-contract.test.ts`, since it is a vendored binary on its own upgrade cadence ## 2. Check orchestration diff --git a/packages/cli/src/rules/vale/verify.ts b/packages/cli/src/rules/vale/verify.ts new file mode 100644 index 0000000..6f218ef --- /dev/null +++ b/packages/cli/src/rules/vale/verify.ts @@ -0,0 +1,210 @@ +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { readdir } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join, posix, resolve, sep } from "node:path"; + +import { ENGINE_LAYOUTS } from "../engines"; +import { runVale, type ValeRunOutcome } from "./run"; + +/** Where a rule's fixtures live, relative to the project root. */ +export function valeRuleTestsDirectory(cwd: string, ruleId?: string): string { + const base = join(cwd, ".taskless", ENGINE_LAYOUTS.vale.ruleTestsDirectory); + return ruleId === undefined ? base : join(base, ruleId); +} + +/** The styles root Vale resolves `rules.` against. */ +function stylesPath(cwd: string): string { + return resolve(cwd, ".taskless", "vale"); +} + +/** + * An ephemeral config that enables exactly one rule and nothing else. + * + * Generated rather than committed, per the spec: verification is one-time, and + * a `rule-tests//` subdirectory holds fixtures only. It also has to be + * *isolating* — a `pass/` fixture proves the rule under test does not fire, and + * that claim is worthless if some other rule's finding is what got counted. + * + * Three details are load-bearing: + * + * - `StylesPath` is absolute. The config is written to a temp directory, and + * Vale resolves StylesPath relative to the config file, so a relative path + * would look for styles beside the temp file. + * - `BasedOnStyles =` is empty, so none of Vale's bundled styles load. Without + * it a fixture could fail on `Vale.Spelling` and be read as the rule firing. + * - Exactly one assignment of the key, in one matcher. Precedence here is + * positional (a later matcher wins; a repeat inside one matcher is + * discarded), so a config that assigned it twice would be relying on the + * rule that bit the scoping spec. + */ +export function buildIsolatingConfig(cwd: string, ruleId: string): string { + return [ + `StylesPath = ${stylesPath(cwd)}`, + "MinAlertLevel = suggestion", + "", + "[*]", + "BasedOnStyles =", + `rules.${ruleId} = YES`, + "", + ].join("\n"); +} + +/** Fixture documents directly under `///`. */ +async function fixtureFiles( + cwd: string, + ruleId: string, + bucket: "pass" | "fail" +): Promise { + const directory = join(valeRuleTestsDirectory(cwd, ruleId), bucket); + try { + const entries = await readdir(directory, { withFileTypes: true }); + return entries + .filter((entry) => entry.isFile()) + .map((entry) => join(directory, entry.name)); + } catch { + return []; + } +} + +/** Vale keys findings by the path it was given, so compare in that shape. */ +function toRelativePosix(cwd: string, absolute: string): string { + return absolute + .slice(cwd.length + 1) + .split(sep) + .join(posix.sep); +} + +export interface ValeRuleVerification { + ruleId: string; + passed: boolean; + /** Fixtures that should have fired and did not. */ + missingFailures: string[]; + /** Fixtures that should have been clean and were not. */ + unexpectedFindings: string[]; + /** No `pass/` and no `fail/` documents — nothing was actually proven. */ + empty: boolean; +} + +export type ValeVerifyOutcome = + | { status: "ok"; rules: ValeRuleVerification[] } + | { status: "unavailable"; message: string } + | { status: "failed"; message: string }; + +/** Rule ids that have a `rule-tests//` directory. */ +export async function discoverValeRuleTests(cwd: string): Promise { + try { + const entries = await readdir(valeRuleTestsDirectory(cwd), { + withFileTypes: true, + }); + return entries + .filter((entry) => entry.isDirectory()) + .map((entry) => entry.name) + .toSorted(); + } catch { + return []; + } +} + +/** + * Verify one rule against its fixtures. + * + * Both buckets run in a single Vale invocation over the rule's directory — + * `pass/` and `fail/` are siblings, so one run covers them and halves the + * process spawns. + * + * A fixture that produces no finding does not appear in Vale's output at all, + * so the expected set comes from disk rather than from the output: absence is + * the signal for `pass/` and the failure for `fail/`, and neither can be read + * off a payload that simply omits them. + */ +export async function verifyValeRule( + cwd: string, + ruleId: string, + options: { timeoutMs?: number } = {} +): Promise { + const [passFixtures, failFixtures] = await Promise.all([ + fixtureFiles(cwd, ruleId, "pass"), + fixtureFiles(cwd, ruleId, "fail"), + ]); + + if (passFixtures.length === 0 && failFixtures.length === 0) { + return { + ruleId, + passed: false, + missingFailures: [], + unexpectedFindings: [], + empty: true, + }; + } + + const configDirectory = mkdtempSync(join(tmpdir(), `vale-verify-${ruleId}-`)); + const configPath = join(configDirectory, ".vale.ini"); + writeFileSync(configPath, buildIsolatingConfig(cwd, ruleId)); + + try { + const outcome = await runVale({ + cwd, + configPath, + paths: [toRelativePosix(cwd, valeRuleTestsDirectory(cwd, ruleId))], + timeoutMs: options.timeoutMs, + }); + if (outcome.status !== "ok") return { unavailable: outcome }; + + // Only findings for the rule under test count. The config isolates it, so + // this should be every finding — filtering anyway means a leak shows up as + // a verification that still measures the right thing. + const firedIn = new Set( + outcome.results + .filter((result) => result.ruleId === ruleId) + .map((result) => result.file) + ); + + const missingFailures = failFixtures + .map((file) => toRelativePosix(cwd, file)) + .filter((file) => !firedIn.has(file)); + const unexpectedFindings = passFixtures + .map((file) => toRelativePosix(cwd, file)) + .filter((file) => firedIn.has(file)); + + return { + ruleId, + passed: missingFailures.length === 0 && unexpectedFindings.length === 0, + missingFailures, + unexpectedFindings, + empty: false, + }; + } finally { + rmSync(configDirectory, { recursive: true, force: true }); + } +} + +/** + * Verify every rule that has fixtures. + * + * An unavailable or failed Vale stops the whole pass rather than being recorded + * per rule: with no working binary every rule would report "no findings", which + * is indistinguishable from every rule being broken. Reporting that as a wall + * of verification failures would send someone to debug their rules over a + * missing install. + */ +export async function verifyValeRules( + cwd: string, + options: { timeoutMs?: number } = {} +): Promise { + const ruleIds = await discoverValeRuleTests(cwd); + const rules: ValeRuleVerification[] = []; + + for (const ruleId of ruleIds) { + const result = await verifyValeRule(cwd, ruleId, options); + if ("unavailable" in result) { + const outcome = result.unavailable; + return { + status: outcome.status === "unavailable" ? "unavailable" : "failed", + message: "message" in outcome ? outcome.message : "Vale failed", + }; + } + rules.push(result); + } + + return { status: "ok", rules }; +} diff --git a/packages/cli/test/vale-vendor-contract.test.ts b/packages/cli/test/vale-vendor-contract.test.ts index a90f7d2..0266d4c 100644 --- a/packages/cli/test/vale-vendor-contract.test.ts +++ b/packages/cli/test/vale-vendor-contract.test.ts @@ -317,6 +317,24 @@ withVale("Vale vendor contract", () => { }); }); + it("matches existence tokens case-sensitively by default", () => { + // Depended on by: every fixture we author, and by anyone writing a rule. + // `Simply` does not match the token `simply`. This cost real time once — + // a verify fixture that read as a bug in the verifier rather than as a + // fixture that never matched. If Vale ever changes this default, rules + // that relied on case sensitivity start firing on prose they ignored. + const cwd = project( + `${header}\n[*.md]\nrules.no-simply = YES\n`, + { "no-simply": existence("simply") }, + { "doc.md": "Simply put, simply.\n" } + ); + const parsed = JSON.parse( + runRaw(cwd, ["doc.md"], ["--no-exit"]).stdout + ) as Record>; + // One finding: the lowercase occurrence only. + expect(parsed["doc.md"]).toHaveLength(1); + }); + it("ignores a `tskl)` breadcrumb key in the config", () => { // Depended on by: the spec's breadcrumb requirement — Taskless writes // `tskl) rule = ` keys into .vale.ini and relies on Vale's ini parser diff --git a/packages/cli/test/vale-verify.test.ts b/packages/cli/test/vale-verify.test.ts new file mode 100644 index 0000000..92fe59e --- /dev/null +++ b/packages/cli/test/vale-verify.test.ts @@ -0,0 +1,254 @@ +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { findValeBinary } from "../src/rules/vale/binary"; +import { + buildIsolatingConfig, + discoverValeRuleTests, + verifyValeRule, + verifyValeRules, +} from "../src/rules/vale/verify"; + +const withVale = findValeBinary().path === undefined ? describe.skip : describe; + +const workspaces: string[] = []; +afterEach(() => { + vi.restoreAllMocks(); + while (workspaces.length > 0) { + rmSync(workspaces.pop() as string, { recursive: true, force: true }); + } +}); + +interface RuleFixtures { + pass?: Record; + fail?: Record; +} + +/** A project with rules and per-rule fixture buckets, and no committed config. */ +function makeProject( + rules: Record, + fixtures: Record +): string { + const cwd = mkdtempSync(join(tmpdir(), "vale-verify-")); + workspaces.push(cwd); + mkdirSync(join(cwd, ".taskless", "vale", "rules"), { recursive: true }); + for (const [name, body] of Object.entries(rules)) { + writeFileSync(join(cwd, ".taskless", "vale", "rules", `${name}.yml`), body); + } + for (const [ruleId, buckets] of Object.entries(fixtures)) { + for (const bucket of ["pass", "fail"] as const) { + const documents = buckets[bucket]; + if (documents === undefined) continue; + const directory = join( + cwd, + ".taskless", + "vale", + "rule-tests", + ruleId, + bucket + ); + mkdirSync(directory, { recursive: true }); + for (const [name, body] of Object.entries(documents)) { + writeFileSync(join(directory, name), body); + } + } + } + return cwd; +} + +const existence = (token: string) => + `extends: existence\nmessage: "Avoid '${token}'"\nlevel: warning\ntokens:\n - ${token}\n`; + +describe("buildIsolatingConfig", () => { + it("enables exactly one rule, once", () => { + const config = buildIsolatingConfig("/proj", "no-simply"); + expect(config).toContain("rules.no-simply = YES"); + // Precedence is positional, so a repeated assignment would be relying on + // the very rule that bit the scoping spec. + expect(config.match(/rules\.no-simply/g)).toHaveLength(1); + expect(config.match(/^\[.*]$/gm)).toHaveLength(1); + }); + + it("uses an absolute StylesPath, since the config lives in a temp dir", () => { + const config = buildIsolatingConfig("/proj", "no-simply"); + expect(config).toContain("StylesPath = /proj/.taskless/vale"); + }); + + it("loads no bundled styles", () => { + // Without this a fixture could trip Vale.Spelling and be counted as the + // rule under test firing. + expect(buildIsolatingConfig("/proj", "r")).toContain("BasedOnStyles ="); + }); +}); + +describe("discoverValeRuleTests", () => { + it("lists rule ids that have a fixture directory", async () => { + const cwd = makeProject( + { "no-simply": existence("simply"), "no-very": existence("very") }, + { + "no-simply": { fail: { "a.md": "Just simply do it.\n" } }, + "no-very": { fail: { "a.md": "It is very good.\n" } }, + } + ); + expect(await discoverValeRuleTests(cwd)).toEqual(["no-simply", "no-very"]); + }); + + it("returns nothing when the directory does not exist", async () => { + const cwd = makeProject({ r: existence("simply") }, {}); + expect(await discoverValeRuleTests(cwd)).toEqual([]); + }); +}); + +withVale("verifyValeRule", () => { + it("passes when every fail fixture fires and every pass fixture is clean", async () => { + const cwd = makeProject( + { "no-simply": existence("simply") }, + { + "no-simply": { + fail: { + "a.md": "Just simply do it.\n", + "b.md": "Keep it simply short.\n", + }, + pass: { "c.md": "Nothing objectionable.\n" }, + }, + } + ); + const result = await verifyValeRule(cwd, "no-simply"); + expect(result).toEqual({ + ruleId: "no-simply", + passed: true, + missingFailures: [], + unexpectedFindings: [], + empty: false, + }); + }); + + it("reports a fail fixture that does not fire", async () => { + const cwd = makeProject( + { "no-simply": existence("simply") }, + { + "no-simply": { + fail: { + "a.md": "Just simply do it.\n", + "quiet.md": "Nothing here.\n", + }, + }, + } + ); + const result = await verifyValeRule(cwd, "no-simply"); + if ("unavailable" in result) throw new Error("expected a verification"); + expect(result.passed).toBe(false); + // A fixture that produces nothing is absent from Vale's output entirely, + // so this can only be caught by comparing against the files on disk. + expect(result.missingFailures).toEqual([ + ".taskless/vale/rule-tests/no-simply/fail/quiet.md", + ]); + }); + + it("reports a pass fixture that fires", async () => { + const cwd = makeProject( + { "no-simply": existence("simply") }, + { + "no-simply": { + fail: { "a.md": "Just simply do it.\n" }, + pass: { "oops.md": "This simply fires.\n" }, + }, + } + ); + const result = await verifyValeRule(cwd, "no-simply"); + if ("unavailable" in result) throw new Error("expected a verification"); + expect(result.passed).toBe(false); + expect(result.unexpectedFindings).toEqual([ + ".taskless/vale/rule-tests/no-simply/pass/oops.md", + ]); + }); + + it("isolates the rule under test from every other rule", async () => { + // The pass fixture trips a DIFFERENT rule. Without isolation that finding + // would be counted and the fixture reported as wrongly firing — a + // verification failure for a rule that behaved correctly. + const cwd = makeProject( + { "no-simply": existence("simply"), "no-very": existence("very") }, + { + "no-simply": { + fail: { "a.md": "Just simply do it.\n" }, + pass: { "b.md": "It is very fine.\n" }, + }, + } + ); + const result = await verifyValeRule(cwd, "no-simply"); + if ("unavailable" in result) throw new Error("expected a verification"); + expect(result.passed).toBe(true); + expect(result.unexpectedFindings).toEqual([]); + }); + + it("does not report success for a rule with no fixtures", async () => { + // An empty fixture directory proves nothing. Reporting it as passing is + // how an unverified rule ships looking verified. + const cwd = makeProject( + { "no-simply": existence("simply") }, + { + "no-simply": {}, + } + ); + const result = await verifyValeRule(cwd, "no-simply"); + if ("unavailable" in result) throw new Error("expected a verification"); + expect(result.empty).toBe(true); + expect(result.passed).toBe(false); + }); + + it("needs no committed .vale.ini", async () => { + // The spec is explicit that rule-tests hold fixtures only; the config is + // generated. makeProject never writes one. + const cwd = makeProject( + { "no-simply": existence("simply") }, + { "no-simply": { fail: { "a.md": "Just simply do it.\n" } } } + ); + const result = await verifyValeRule(cwd, "no-simply"); + if ("unavailable" in result) throw new Error("expected a verification"); + expect(result.passed).toBe(true); + }); +}); + +withVale("verifyValeRules", () => { + it("verifies every rule that has fixtures", async () => { + const cwd = makeProject( + { "no-simply": existence("simply"), "no-very": existence("very") }, + { + "no-simply": { fail: { "a.md": "Just simply do it.\n" } }, + "no-very": { fail: { "a.md": "It is very good.\n" } }, + } + ); + const outcome = await verifyValeRules(cwd); + expect(outcome.status).toBe("ok"); + if (outcome.status !== "ok") return; + expect(outcome.rules.map((rule) => [rule.ruleId, rule.passed])).toEqual([ + ["no-simply", true], + ["no-very", true], + ]); + }); +}); + +describe("verifyValeRules without a binary", () => { + it("reports unavailable rather than failing every rule", async () => { + // With no binary every rule produces no findings, which is + // indistinguishable from every rule being broken. Reporting a wall of + // verification failures would send someone to debug rules over a missing + // install. + const binary = await import("../src/rules/vale/binary"); + vi.spyOn(binary, "findValeBinary").mockReturnValue({ + path: undefined, + tried: ["@taskless/vale-darwin-arm64", "PATH"], + }); + + const cwd = makeProject( + { "no-simply": existence("simply") }, + { "no-simply": { fail: { "a.md": "Just simply do it.\n" } } } + ); + const outcome = await verifyValeRules(cwd); + expect(outcome.status).toBe("unavailable"); + }); +}); From fef22c2d236825578e5f62d217618a65965369e9 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 11 Aug 2026 17:07:34 -0700 Subject: [PATCH 2/4] fix(cli): require both fixture buckets, and stop swallowing IO errors in Vale verify Three review findings on the Vale verification path, none of which has a production caller yet -- only tests import `verifyValeRule`, so the shape changes are free to make now. A rule with fixtures in only one bucket is not `empty`, and could report `passed: true` without proving anything: `pass/`-only passes on a trivially empty `missingFailures` and never shows the rule fires at all. `empty: boolean` becomes `fixtures: ValeFixtureCoverage`, which keeps "no fixtures" apart from "one bucket missing", and only `"both"` is verifiable. `fixtureFiles` and `discoverValeRuleTests` read any `readdir` failure as "nothing here". The buckets are read independently, so an unreadable `pass/` silently became `[]` beside a populated `fail/`. Only ENOENT and ENOTDIR are swallowed now; everything else rethrows. `{ unavailable: ValeRunOutcome }` also carries `timeout` and `failed`, and the name invites a caller to treat a real Vale failure as a skip. Renamed to `{ outcome }`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3 --- packages/cli/src/rules/vale/verify.ts | 98 +++++++++++++++++++---- packages/cli/test/vale-verify.test.ts | 111 +++++++++++++++++++++++--- 2 files changed, 183 insertions(+), 26 deletions(-) diff --git a/packages/cli/src/rules/vale/verify.ts b/packages/cli/src/rules/vale/verify.ts index 6f218ef..32c27e1 100644 --- a/packages/cli/src/rules/vale/verify.ts +++ b/packages/cli/src/rules/vale/verify.ts @@ -49,7 +49,32 @@ export function buildIsolatingConfig(cwd: string, ruleId: string): string { ].join("\n"); } -/** Fixture documents directly under `///`. */ +/** + * Whether a `readdir` failure genuinely means "that directory is not there". + * + * `ENOENT` is the path not existing; `ENOTDIR` is a path that exists but is a + * file, or that has a file for an ancestor. Every other code — `EACCES` above + * all — is a real IO problem, and reading it as "nothing here" is what makes an + * unreadable bucket indistinguishable from an unwritten one. + */ +function isMissingDirectory(error: unknown): boolean { + if (error === null || typeof error !== "object" || !("code" in error)) { + return false; + } + const { code } = error as NodeJS.ErrnoException; + return code === "ENOENT" || code === "ENOTDIR"; +} + +/** + * Fixture documents directly under `///`. + * + * A missing directory is an empty bucket; anything else rethrows. The buckets + * are read independently, so a swallowed `EACCES` on `pass/` would silently + * yield `[]` while `fail/` still had fixtures — the rule would not look + * one-sided, and could report `passed: true` having never checked the pass side + * at all. A permissions problem must not read as "no pass fixtures were + * written". + */ async function fixtureFiles( cwd: string, ruleId: string, @@ -61,8 +86,9 @@ async function fixtureFiles( return entries .filter((entry) => entry.isFile()) .map((entry) => join(directory, entry.name)); - } catch { - return []; + } catch (error) { + if (isMissingDirectory(error)) return []; + throw error; } } @@ -74,6 +100,26 @@ function toRelativePosix(cwd: string, absolute: string): string { .join(posix.sep); } +/** + * Which fixture buckets a rule actually populated. + * + * This replaces the earlier `empty: boolean`, which only distinguished "no + * fixtures at all" from everything else and so let a one-sided rule report + * `passed: true`. The four cases are kept apart because a caller wants to say + * different things about them: `"none"` is an unwritten rule, while + * `"fail-only"`/`"pass-only"` is a half-written one, which is the more + * misleading state of the two. + */ +export type ValeFixtureCoverage = "both" | "pass-only" | "fail-only" | "none"; + +/** Classify a rule's buckets by how many documents each held. */ +function coverageOf(passCount: number, failCount: number): ValeFixtureCoverage { + if (passCount > 0 && failCount > 0) return "both"; + if (passCount > 0) return "pass-only"; + if (failCount > 0) return "fail-only"; + return "none"; +} + export interface ValeRuleVerification { ruleId: string; passed: boolean; @@ -81,8 +127,14 @@ export interface ValeRuleVerification { missingFailures: string[]; /** Fixtures that should have been clean and were not. */ unexpectedFindings: string[]; - /** No `pass/` and no `fail/` documents — nothing was actually proven. */ - empty: boolean; + /** + * Which buckets held documents. Only `"both"` can be `passed: true`: a + * `fail/` fixture proves the rule fires, a `pass/` fixture proves it does not + * over-fire, and either alone is half a claim. A rule with only `pass/` + * fixtures passes with `missingFailures: []` without ever demonstrating the + * rule can fire at all. + */ + fixtures: ValeFixtureCoverage; } export type ValeVerifyOutcome = @@ -100,8 +152,9 @@ export async function discoverValeRuleTests(cwd: string): Promise { .filter((entry) => entry.isDirectory()) .map((entry) => entry.name) .toSorted(); - } catch { - return []; + } catch (error) { + if (isMissingDirectory(error)) return []; + throw error; } } @@ -116,24 +169,41 @@ export async function discoverValeRuleTests(cwd: string): Promise { * so the expected set comes from disk rather than from the output: absence is * the signal for `pass/` and the failure for `fail/`, and neither can be read * off a payload that simply omits them. + * + * Both buckets have to hold at least one document before the rule can report + * `passed: true`. An empty fixture directory proves nothing, and reporting it + * as passing is how an unverified rule ships looking verified — but so is a + * rule with only `fail/` fixtures (never shown not to over-fire) or only + * `pass/` fixtures (never shown to fire at all, passing on an empty + * `missingFailures`). The one-sided cases are the same failure mode, one level + * less obvious, so `fixtures` records which buckets were populated and only + * `"both"` is verifiable. + * + * Non-`ok` outcomes come back as `{ outcome }` rather than a verification. + * The discriminant is deliberately not named for the unavailable case: it + * carries `timeout` and `failed` too, and a name like `unavailable` invites a + * caller to treat a genuine Vale failure as a skip. */ export async function verifyValeRule( cwd: string, ruleId: string, options: { timeoutMs?: number } = {} -): Promise { +): Promise { const [passFixtures, failFixtures] = await Promise.all([ fixtureFiles(cwd, ruleId, "pass"), fixtureFiles(cwd, ruleId, "fail"), ]); - if (passFixtures.length === 0 && failFixtures.length === 0) { + // Short-circuited before Vale runs: the rule has to be edited either way, so + // there is nothing a subprocess could add that `fixtures` does not say. + const fixtures = coverageOf(passFixtures.length, failFixtures.length); + if (fixtures !== "both") { return { ruleId, passed: false, missingFailures: [], unexpectedFindings: [], - empty: true, + fixtures, }; } @@ -148,7 +218,7 @@ export async function verifyValeRule( paths: [toRelativePosix(cwd, valeRuleTestsDirectory(cwd, ruleId))], timeoutMs: options.timeoutMs, }); - if (outcome.status !== "ok") return { unavailable: outcome }; + if (outcome.status !== "ok") return { outcome }; // Only findings for the rule under test count. The config isolates it, so // this should be every finding — filtering anyway means a leak shows up as @@ -171,7 +241,7 @@ export async function verifyValeRule( passed: missingFailures.length === 0 && unexpectedFindings.length === 0, missingFailures, unexpectedFindings, - empty: false, + fixtures, }; } finally { rmSync(configDirectory, { recursive: true, force: true }); @@ -196,8 +266,8 @@ export async function verifyValeRules( for (const ruleId of ruleIds) { const result = await verifyValeRule(cwd, ruleId, options); - if ("unavailable" in result) { - const outcome = result.unavailable; + if ("outcome" in result) { + const { outcome } = result; return { status: outcome.status === "unavailable" ? "unavailable" : "failed", message: "message" in outcome ? outcome.message : "Vale failed", diff --git a/packages/cli/test/vale-verify.test.ts b/packages/cli/test/vale-verify.test.ts index 92fe59e..c8ad466 100644 --- a/packages/cli/test/vale-verify.test.ts +++ b/packages/cli/test/vale-verify.test.ts @@ -1,4 +1,10 @@ -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { + chmodSync, + mkdirSync, + mkdtempSync, + rmSync, + writeFileSync, +} from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -102,6 +108,41 @@ describe("discoverValeRuleTests", () => { }); }); +// Root ignores the mode bits, so there is no unreadable directory to make. +const asUser = process.getuid?.() === 0 ? describe.skip : describe; + +asUser("verifyValeRule with an unreadable bucket", () => { + it("surfaces the IO error instead of reading it as an empty bucket", async () => { + // The buckets are read independently: swallowing this would leave `pass/` + // silently `[]` beside a populated `fail/`, and the rule could report + // passing having never checked the pass side. + const cwd = makeProject( + { "no-simply": existence("simply") }, + { + "no-simply": { + fail: { "a.md": "Just simply do it.\n" }, + pass: { "c.md": "Nothing objectionable.\n" }, + }, + } + ); + const passDirectory = join( + cwd, + ".taskless", + "vale", + "rule-tests", + "no-simply", + "pass" + ); + chmodSync(passDirectory, 0o000); + try { + await expect(verifyValeRule(cwd, "no-simply")).rejects.toThrow(/EACCES/); + } finally { + // Restore, or the afterEach cleanup cannot recurse into it either. + chmodSync(passDirectory, 0o700); + } + }); +}); + withVale("verifyValeRule", () => { it("passes when every fail fixture fires and every pass fixture is clean", async () => { const cwd = makeProject( @@ -122,7 +163,7 @@ withVale("verifyValeRule", () => { passed: true, missingFailures: [], unexpectedFindings: [], - empty: false, + fixtures: "both", }); }); @@ -135,11 +176,12 @@ withVale("verifyValeRule", () => { "a.md": "Just simply do it.\n", "quiet.md": "Nothing here.\n", }, + pass: { "c.md": "Nothing objectionable.\n" }, }, } ); const result = await verifyValeRule(cwd, "no-simply"); - if ("unavailable" in result) throw new Error("expected a verification"); + if ("outcome" in result) throw new Error("expected a verification"); expect(result.passed).toBe(false); // A fixture that produces nothing is absent from Vale's output entirely, // so this can only be caught by comparing against the files on disk. @@ -159,7 +201,7 @@ withVale("verifyValeRule", () => { } ); const result = await verifyValeRule(cwd, "no-simply"); - if ("unavailable" in result) throw new Error("expected a verification"); + if ("outcome" in result) throw new Error("expected a verification"); expect(result.passed).toBe(false); expect(result.unexpectedFindings).toEqual([ ".taskless/vale/rule-tests/no-simply/pass/oops.md", @@ -180,7 +222,7 @@ withVale("verifyValeRule", () => { } ); const result = await verifyValeRule(cwd, "no-simply"); - if ("unavailable" in result) throw new Error("expected a verification"); + if ("outcome" in result) throw new Error("expected a verification"); expect(result.passed).toBe(true); expect(result.unexpectedFindings).toEqual([]); }); @@ -195,8 +237,35 @@ withVale("verifyValeRule", () => { } ); const result = await verifyValeRule(cwd, "no-simply"); - if ("unavailable" in result) throw new Error("expected a verification"); - expect(result.empty).toBe(true); + if ("outcome" in result) throw new Error("expected a verification"); + expect(result.fixtures).toBe("none"); + expect(result.passed).toBe(false); + }); + + it("does not report success for a rule with only fail fixtures", async () => { + // The rule is shown to fire and never shown not to over-fire. Half a + // claim, and it is not "empty" — without this it would report passing on + // an unexpectedFindings that had nothing to check. + const cwd = makeProject( + { "no-simply": existence("simply") }, + { "no-simply": { fail: { "a.md": "Just simply do it.\n" } } } + ); + const result = await verifyValeRule(cwd, "no-simply"); + if ("outcome" in result) throw new Error("expected a verification"); + expect(result.fixtures).toBe("fail-only"); + expect(result.passed).toBe(false); + }); + + it("does not report success for a rule with only pass fixtures", async () => { + // The more misleading half: `missingFailures` is trivially empty, so this + // used to pass without ever demonstrating the rule can fire at all. + const cwd = makeProject( + { "no-simply": existence("simply") }, + { "no-simply": { pass: { "c.md": "Nothing objectionable.\n" } } } + ); + const result = await verifyValeRule(cwd, "no-simply"); + if ("outcome" in result) throw new Error("expected a verification"); + expect(result.fixtures).toBe("pass-only"); expect(result.passed).toBe(false); }); @@ -205,10 +274,15 @@ withVale("verifyValeRule", () => { // generated. makeProject never writes one. const cwd = makeProject( { "no-simply": existence("simply") }, - { "no-simply": { fail: { "a.md": "Just simply do it.\n" } } } + { + "no-simply": { + fail: { "a.md": "Just simply do it.\n" }, + pass: { "c.md": "Nothing objectionable.\n" }, + }, + } ); const result = await verifyValeRule(cwd, "no-simply"); - if ("unavailable" in result) throw new Error("expected a verification"); + if ("outcome" in result) throw new Error("expected a verification"); expect(result.passed).toBe(true); }); }); @@ -218,8 +292,14 @@ withVale("verifyValeRules", () => { const cwd = makeProject( { "no-simply": existence("simply"), "no-very": existence("very") }, { - "no-simply": { fail: { "a.md": "Just simply do it.\n" } }, - "no-very": { fail: { "a.md": "It is very good.\n" } }, + "no-simply": { + fail: { "a.md": "Just simply do it.\n" }, + pass: { "c.md": "Nothing objectionable.\n" }, + }, + "no-very": { + fail: { "a.md": "It is very good.\n" }, + pass: { "c.md": "Nothing objectionable.\n" }, + }, } ); const outcome = await verifyValeRules(cwd); @@ -244,9 +324,16 @@ describe("verifyValeRules without a binary", () => { tried: ["@taskless/vale-darwin-arm64", "PATH"], }); + // Both buckets, or the rule short-circuits on incomplete fixtures and Vale + // is never invoked — the binary's absence would go unnoticed. const cwd = makeProject( { "no-simply": existence("simply") }, - { "no-simply": { fail: { "a.md": "Just simply do it.\n" } } } + { + "no-simply": { + fail: { "a.md": "Just simply do it.\n" }, + pass: { "c.md": "Nothing objectionable.\n" }, + }, + } ); const outcome = await verifyValeRules(cwd); expect(outcome.status).toBe("unavailable"); From 0606a94de88129b782fe15f0e6f3f7e51d1fd10c Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 12 Aug 2026 12:38:02 -0700 Subject: [PATCH 3/4] ref(cli): tidy the Vale fixture verifier Four clarity changes, no behavior change: - Extract `directoryEntries()` so the "missing directory is empty, every other errno throws" policy lives in one place instead of being spelled out identically in `fixtureFiles` and `discoverValeRuleTests`. - `toRelativePosix` uses `relative()` instead of slicing by `cwd.length + 1`. - `verifyValeRule` returns `{ outcome: ValeRunFailure }`, the non-`ok` half of `ValeRunOutcome`. Every member carries a `message`, so the unreachable `"message" in outcome ? ... : "Vale failed"` fallback goes away. - Tests: `workspaces.splice(0)` drops an `as string` cast, and a `verification()` helper replaces seven copies of the narrowing guard while naming the Vale status that actually arrived. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3 --- packages/cli/src/rules/vale/verify.ts | 68 +++++++++++++++------------ packages/cli/test/vale-verify.test.ts | 44 ++++++++++------- 2 files changed, 67 insertions(+), 45 deletions(-) diff --git a/packages/cli/src/rules/vale/verify.ts b/packages/cli/src/rules/vale/verify.ts index 32c27e1..94d1e02 100644 --- a/packages/cli/src/rules/vale/verify.ts +++ b/packages/cli/src/rules/vale/verify.ts @@ -1,7 +1,7 @@ -import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { type Dirent, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { readdir } from "node:fs/promises"; import { tmpdir } from "node:os"; -import { join, posix, resolve, sep } from "node:path"; +import { join, posix, relative, resolve, sep } from "node:path"; import { ENGINE_LAYOUTS } from "../engines"; import { runVale, type ValeRunOutcome } from "./run"; @@ -65,6 +65,21 @@ function isMissingDirectory(error: unknown): boolean { return code === "ENOENT" || code === "ENOTDIR"; } +/** + * Directory entries, with a directory that is not there reading as an empty one. + * + * The single place that decides which `readdir` failures are absence and which + * are problems, so no caller can accidentally answer that question differently. + */ +async function directoryEntries(directory: string): Promise { + try { + return await readdir(directory, { withFileTypes: true }); + } catch (error) { + if (isMissingDirectory(error)) return []; + throw error; + } +} + /** * Fixture documents directly under `///`. * @@ -81,23 +96,15 @@ async function fixtureFiles( bucket: "pass" | "fail" ): Promise { const directory = join(valeRuleTestsDirectory(cwd, ruleId), bucket); - try { - const entries = await readdir(directory, { withFileTypes: true }); - return entries - .filter((entry) => entry.isFile()) - .map((entry) => join(directory, entry.name)); - } catch (error) { - if (isMissingDirectory(error)) return []; - throw error; - } + const entries = await directoryEntries(directory); + return entries + .filter((entry) => entry.isFile()) + .map((entry) => join(directory, entry.name)); } /** Vale keys findings by the path it was given, so compare in that shape. */ function toRelativePosix(cwd: string, absolute: string): string { - return absolute - .slice(cwd.length + 1) - .split(sep) - .join(posix.sep); + return relative(cwd, absolute).split(sep).join(posix.sep); } /** @@ -137,6 +144,16 @@ export interface ValeRuleVerification { fixtures: ValeFixtureCoverage; } +/** + * The Vale outcomes that end a verification instead of producing one. + * + * Every member carries a `message`, which is why `verifyValeRule` hands back + * this rather than the full {@link ValeRunOutcome}: an `ok` run is never what a + * caller is handed there, so it should not have to write a fallback for the + * message an `ok` run would not have had. + */ +export type ValeRunFailure = Exclude; + export type ValeVerifyOutcome = | { status: "ok"; rules: ValeRuleVerification[] } | { status: "unavailable"; message: string } @@ -144,18 +161,11 @@ export type ValeVerifyOutcome = /** Rule ids that have a `rule-tests//` directory. */ export async function discoverValeRuleTests(cwd: string): Promise { - try { - const entries = await readdir(valeRuleTestsDirectory(cwd), { - withFileTypes: true, - }); - return entries - .filter((entry) => entry.isDirectory()) - .map((entry) => entry.name) - .toSorted(); - } catch (error) { - if (isMissingDirectory(error)) return []; - throw error; - } + const entries = await directoryEntries(valeRuleTestsDirectory(cwd)); + return entries + .filter((entry) => entry.isDirectory()) + .map((entry) => entry.name) + .toSorted(); } /** @@ -188,7 +198,7 @@ export async function verifyValeRule( cwd: string, ruleId: string, options: { timeoutMs?: number } = {} -): Promise { +): Promise { const [passFixtures, failFixtures] = await Promise.all([ fixtureFiles(cwd, ruleId, "pass"), fixtureFiles(cwd, ruleId, "fail"), @@ -270,7 +280,7 @@ export async function verifyValeRules( const { outcome } = result; return { status: outcome.status === "unavailable" ? "unavailable" : "failed", - message: "message" in outcome ? outcome.message : "Vale failed", + message: outcome.message, }; } rules.push(result); diff --git a/packages/cli/test/vale-verify.test.ts b/packages/cli/test/vale-verify.test.ts index c8ad466..b8f2168 100644 --- a/packages/cli/test/vale-verify.test.ts +++ b/packages/cli/test/vale-verify.test.ts @@ -14,6 +14,8 @@ import { findValeBinary } from "../src/rules/vale/binary"; import { buildIsolatingConfig, discoverValeRuleTests, + type ValeRunFailure, + type ValeRuleVerification, verifyValeRule, verifyValeRules, } from "../src/rules/vale/verify"; @@ -23,11 +25,28 @@ const withVale = findValeBinary().path === undefined ? describe.skip : describe; const workspaces: string[] = []; afterEach(() => { vi.restoreAllMocks(); - while (workspaces.length > 0) { - rmSync(workspaces.pop() as string, { recursive: true, force: true }); + for (const workspace of workspaces.splice(0)) { + rmSync(workspace, { recursive: true, force: true }); } }); +/** + * Narrow away the non-`ok` Vale outcomes, which are a broken test environment + * rather than anything under test — and name the one that arrived, so a Vale + * that times out here does not look like a verifier that returned the wrong + * shape. + */ +function verification( + result: ValeRuleVerification | { outcome: ValeRunFailure } +): ValeRuleVerification { + if ("outcome" in result) { + throw new Error( + `expected a verification, got Vale ${result.outcome.status}` + ); + } + return result; +} + interface RuleFixtures { pass?: Record; fail?: Record; @@ -180,8 +199,7 @@ withVale("verifyValeRule", () => { }, } ); - const result = await verifyValeRule(cwd, "no-simply"); - if ("outcome" in result) throw new Error("expected a verification"); + const result = verification(await verifyValeRule(cwd, "no-simply")); expect(result.passed).toBe(false); // A fixture that produces nothing is absent from Vale's output entirely, // so this can only be caught by comparing against the files on disk. @@ -200,8 +218,7 @@ withVale("verifyValeRule", () => { }, } ); - const result = await verifyValeRule(cwd, "no-simply"); - if ("outcome" in result) throw new Error("expected a verification"); + const result = verification(await verifyValeRule(cwd, "no-simply")); expect(result.passed).toBe(false); expect(result.unexpectedFindings).toEqual([ ".taskless/vale/rule-tests/no-simply/pass/oops.md", @@ -221,8 +238,7 @@ withVale("verifyValeRule", () => { }, } ); - const result = await verifyValeRule(cwd, "no-simply"); - if ("outcome" in result) throw new Error("expected a verification"); + const result = verification(await verifyValeRule(cwd, "no-simply")); expect(result.passed).toBe(true); expect(result.unexpectedFindings).toEqual([]); }); @@ -236,8 +252,7 @@ withVale("verifyValeRule", () => { "no-simply": {}, } ); - const result = await verifyValeRule(cwd, "no-simply"); - if ("outcome" in result) throw new Error("expected a verification"); + const result = verification(await verifyValeRule(cwd, "no-simply")); expect(result.fixtures).toBe("none"); expect(result.passed).toBe(false); }); @@ -250,8 +265,7 @@ withVale("verifyValeRule", () => { { "no-simply": existence("simply") }, { "no-simply": { fail: { "a.md": "Just simply do it.\n" } } } ); - const result = await verifyValeRule(cwd, "no-simply"); - if ("outcome" in result) throw new Error("expected a verification"); + const result = verification(await verifyValeRule(cwd, "no-simply")); expect(result.fixtures).toBe("fail-only"); expect(result.passed).toBe(false); }); @@ -263,8 +277,7 @@ withVale("verifyValeRule", () => { { "no-simply": existence("simply") }, { "no-simply": { pass: { "c.md": "Nothing objectionable.\n" } } } ); - const result = await verifyValeRule(cwd, "no-simply"); - if ("outcome" in result) throw new Error("expected a verification"); + const result = verification(await verifyValeRule(cwd, "no-simply")); expect(result.fixtures).toBe("pass-only"); expect(result.passed).toBe(false); }); @@ -281,8 +294,7 @@ withVale("verifyValeRule", () => { }, } ); - const result = await verifyValeRule(cwd, "no-simply"); - if ("outcome" in result) throw new Error("expected a verification"); + const result = verification(await verifyValeRule(cwd, "no-simply")); expect(result.passed).toBe(true); }); }); From 3a70ffb2d96865176d91673d6b25587c3733cff9 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 12 Aug 2026 12:50:20 -0700 Subject: [PATCH 4/4] fix(cli): reject nested Vale fixture directories instead of skipping them The two halves of verification disagreed about recursion. `fixtureFiles` reads one directory deep, but Vale is invoked over the whole `rule-tests/` tree and lints recursively, so a nested fixture was linted while never being collected. That fails in the dangerous direction. A nested `pass/` fixture that wrongly fires produces a finding the expected-set never knew about, so `unexpectedFindings` discards it; a nested `fail/` fixture is never required to fire. Either way the rule reports `passed: true` with half its fixtures unchecked -- the same "ships looking verified" failure `ValeFixtureCoverage` was added to prevent, one directory deeper. Flat and loud rather than recursive: one legal layout instead of two, and the error names the offending path the moment someone creates it. Nothing nests today, so this closes the gap before it can be hit rather than fixing a live break. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3 --- packages/cli/src/rules/vale/verify.ts | 25 +++++++++++++++++++++++++ packages/cli/test/vale-verify.test.ts | 26 ++++++++++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/packages/cli/src/rules/vale/verify.ts b/packages/cli/src/rules/vale/verify.ts index 94d1e02..a00c844 100644 --- a/packages/cli/src/rules/vale/verify.ts +++ b/packages/cli/src/rules/vale/verify.ts @@ -89,6 +89,20 @@ async function directoryEntries(directory: string): Promise { * one-sided, and could report `passed: true` having never checked the pass side * at all. A permissions problem must not read as "no pass fixtures were * written". + * + * A bucket is one directory deep, and a nested directory is rejected rather + * than ignored. The two halves of verification disagree about recursion: this + * read is flat, but Vale is invoked over the whole `rule-tests/` tree and + * lints recursively. Silently skipping a nested entry therefore fails in the + * dangerous direction — a nested `pass/` fixture that wrongly fires produces a + * finding this function never collected, so `unexpectedFindings` discards it, + * and a nested `fail/` fixture is never required to fire. Either way the rule + * reports `passed: true` while half its fixtures went unchecked, which is the + * exact failure `ValeFixtureCoverage` exists to prevent. + * + * Flat-and-loud is chosen over recursing because it keeps one layout legal + * instead of two, and because the error names the offending path at the moment + * someone creates it. */ async function fixtureFiles( cwd: string, @@ -97,6 +111,17 @@ async function fixtureFiles( ): Promise { const directory = join(valeRuleTestsDirectory(cwd, ruleId), bucket); const entries = await directoryEntries(directory); + + const nested = entries.find((entry) => entry.isDirectory()); + if (nested !== undefined) { + throw new Error( + `Vale fixture buckets are flat: ${join(directory, nested.name)} is a ` + + `directory. Move its documents directly into ${bucket}/ — Vale lints ` + + `the rule's whole directory, so a nested fixture is linted but never ` + + `checked.` + ); + } + return entries .filter((entry) => entry.isFile()) .map((entry) => join(directory, entry.name)); diff --git a/packages/cli/test/vale-verify.test.ts b/packages/cli/test/vale-verify.test.ts index b8f2168..0fee836 100644 --- a/packages/cli/test/vale-verify.test.ts +++ b/packages/cli/test/vale-verify.test.ts @@ -162,6 +162,32 @@ asUser("verifyValeRule with an unreadable bucket", () => { }); }); +describe("fixture buckets are flat", () => { + it("rejects a nested directory instead of silently skipping it", async () => { + // The dangerous case: Vale lints `rule-tests/` recursively, so a + // nested fixture IS linted, but a flat read never collects it. Skipping it + // quietly would let a nested `pass/` fixture fire with its finding + // discarded, and a nested `fail/` fixture never be required to fire — + // `passed: true` over fixtures that were never checked. + const cwd = makeProject( + { "no-simply": existence("simply") }, + { + "no-simply": { + pass: { "clean.md": "Nothing objectionable.\n" }, + fail: { "a.md": "Just simply do it.\n" }, + }, + } + ); + mkdirSync(join(cwd, ".taskless", "vale", "rule-tests", "no-simply", "pass", "nested"), { + recursive: true, + }); + + await expect(verifyValeRule(cwd, "no-simply")).rejects.toThrow( + /fixture buckets are flat/i + ); + }); +}); + withVale("verifyValeRule", () => { it("passes when every fail fixture fires and every pass fixture is clean", async () => { const cwd = makeProject(