From 150b11870f5c6d50fdb99967d810d0292bd4003f Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 8 Sep 2026 16:12:56 -0700 Subject: [PATCH 1/4] fix(cli): skip an oversized file instead of letting Vale time out on it Vale is quadratic in a single file's size, and VALE_TIMEOUT_MS bounds the whole run rather than one file: a large enough document can consume most or all of that 60s budget on its own, and a timeout discards every other file's findings with it. This is the #300 failure again, on a path #300 did not cover. runVale now excludes a target file over VALE_MAX_FILE_BYTES (128KB) before invoking Vale, unconditionally, the same preemptive treatment already given to a converter-dependent format. Excluded files are named in a `notices` entry rather than reported as a blocking finding: unlike an unparseable file, where Vale itself proves the file was a real target by erroring on it, this exclusion runs from a bare filesystem walk with no way to confirm any rule's matcher would have reached the file. Reporting it as a hard error produced false failures on files no rule ever touches, measured on this repository's own pnpm-lock.yaml and CHANGELOG.md. Fixes #321 --- .changeset/skip-oversized-vale-files.md | 27 ++++ packages/cli/src/agent/create-vale-rule.md | 17 ++- packages/cli/src/rules/vale/formats.ts | 167 ++++++++++++++++++++- packages/cli/src/rules/vale/run.ts | 95 +++++++++++- packages/cli/test/vale-run.test.ts | 148 +++++++++++++++++- 5 files changed, 440 insertions(+), 14 deletions(-) create mode 100644 .changeset/skip-oversized-vale-files.md diff --git a/.changeset/skip-oversized-vale-files.md b/.changeset/skip-oversized-vale-files.md new file mode 100644 index 00000000..d8b85a03 --- /dev/null +++ b/.changeset/skip-oversized-vale-files.md @@ -0,0 +1,27 @@ +--- +"@taskless/cli": patch +--- + +`check` no longer risks losing every Vale finding in a run to one oversized +file. Vale's cost is quadratic in a single file's size (measured against the +pinned binary: 128KB is ~0.8s for one rule, 384KB is already ~7s), and +`VALE_TIMEOUT_MS` bounds the whole run, not one file — a large enough +document could consume most or all of that budget on its own, and a timeout +discards every other file's findings along with it (the same failure #300 +fixed, on a path #300 did not cover). + +`runVale` now excludes a target file over 128KB (`VALE_MAX_FILE_BYTES` in +`src/rules/vale/run.ts`) before invoking Vale at all, the same preemptive +treatment already given to a format Vale cannot parse. Excluded files are +named in a `notices` entry, not reported as a failing finding: unlike an +unparseable file (where Vale itself proves the file was a real target by +erroring on it), this exclusion runs from a bare filesystem walk with no way +to confirm any rule's matcher would have reached the file — reporting it as +a blocking error produced false failures on files no rule ever touches (a +lockfile, a generated changelog). + +A consumer may now see a `check` that previously counted a large file's +prose findings instead report a `notices` entry naming that file as skipped. +128KB is comfortably past hand-written prose (roughly 20,000 words); this +should only affect generated output, pasted data, or exported notes checked +directly against a matching rule. diff --git a/packages/cli/src/agent/create-vale-rule.md b/packages/cli/src/agent/create-vale-rule.md index 8471c8f7..d01762af 100644 --- a/packages/cli/src/agent/create-vale-rule.md +++ b/packages/cli/src/agent/create-vale-rule.md @@ -1,4 +1,4 @@ -# Topic: create-vale-rule (CLI v%(CLI_VERSION)s / topic v6) +# Topic: create-vale-rule (CLI v%(CLI_VERSION)s / topic v7) ## You are here This is `create-vale-rule`. It helps you write a Vale rule: a check over @@ -571,6 +571,21 @@ it. matcher that takes `check` down the first time the repo grows a `.typ` file. Never put one of those extensions in a glob. + **A single oversized file is excluded before Vale ever opens it, not + linted slowly.** Vale's cost is quadratic in one file's size, so a + large enough document can consume the whole run's time budget on its + own and cost every other file its findings: the same failure mode as + the unreadable-file case above, from a different cause. `check` + preempts it: a target file over 128KB is skipped, and named in a + `notices` entry rather than a finding, since nothing here can confirm + whether any matcher would actually have reached it (a large lockfile + or a generated file outside every rule's scope is common, and + flagging one as a failure would be a false positive). A rule's own + fixtures are never this large in practice, so this should not surface + while authoring one. It matters when scoping a matcher at a whole + project, where a generated changelog or an exported note can cross + it. + That example changed with Vale v3.18.0, which is the point: the dangerous extension is whichever one the list above says needs a program, not the one you remember. `.mdx` was the example until that diff --git a/packages/cli/src/rules/vale/formats.ts b/packages/cli/src/rules/vale/formats.ts index 763fd764..ff6e8953 100644 --- a/packages/cli/src/rules/vale/formats.ts +++ b/packages/cli/src/rules/vale/formats.ts @@ -1,5 +1,5 @@ -import { glob } from "node:fs/promises"; -import { basename, extname } from "node:path"; +import { glob, stat } from "node:fs/promises"; +import { basename, extname, resolve as resolvePath } from "node:path"; import { VALE_CONVERTER_BY_EXTENSION, @@ -235,6 +235,105 @@ export async function findConverterDependentFiles( return [...found].toSorted(); } +/** + * One file above `maxBytes`, found while walking the run's targets. + * + * Carries the measured size alongside the path so the caller can report an + * exact number rather than just naming the file — see `oversizedFileResult` + * in `run.ts`, which is the only reader. + */ +export interface OversizedFile { + file: string; + size: number; +} + +/** + * Files inside the run's target set whose size exceeds `maxBytes` — + * `VALE_MAX_FILE_BYTES` in `run.ts` (not imported here to avoid a cycle; + * `run.ts` already imports this module). + * + * Same shape as {@link findConverterDependentFiles}, and the same reasoning: + * Vale is not merely slow on an oversized file, it is quadratic in that one + * file's size (see the docblock on `VALE_MAX_FILE_BYTES`), so one file over the + * limit can consume the whole run's timeout budget and take every other file's + * findings down with it. Preemptively excluding it — rather than letting Vale + * discover the cost the hard way — is the same trade `converterExclusionGlobs` + * makes for a format Vale cannot parse at all. + * + * Unlike the converter walk, this cannot be scoped by extension: an oversized + * file can have any extension, or none, so every file under the target roots + * has to be listed and stat'd. That is a real cost on the happy path, where + * nothing is oversized and the walk still runs — measured and reported in the + * PR that introduced this function. + * + * A named path is stat'd directly, exactly as `targetFileParseError` does + * elsewhere in this package: an explicit request is not resolved through the + * walk that answers a whole-project run. Whether named or discovered by the + * walk, an oversized file is excluded unconditionally, on every run — the same + * asymmetry `findConverterDependentFiles` documents, and for the same reason: + * handing Vale this file does not check it badly, it risks the entire batch's + * timeout. + * + * Errors are swallowed the same way as {@link findConverterDependentFiles} and + * for the same reason: a target that vanished between listing and stat, an + * unreadable subtree, a platform where `glob` rejects the pattern — none of + * them can be allowed to suppress the exclusion that already ran. The failure + * mode here is "no notice, never no fix". + */ +export async function findOversizedFiles( + cwd: string, + paths: string[], + maxBytes: number +): Promise { + const named: OversizedFile[] = []; + const roots: string[] = []; + if (paths.length === 0) { + roots.push("."); + } else { + for (const path of paths) { + try { + const stats = await stat(resolvePath(cwd, path)); + if (stats.isFile() && stats.size > maxBytes) { + named.push({ file: path, size: stats.size }); + } + } catch { + // Unreadable or missing: not this function's problem. The run itself + // will report it if it matters. + } + roots.push(path); + } + } + + const found = new Map(named.map((entry) => [entry.file, entry])); + for (const root of roots) { + const prefix = root === "." || root === "" ? "" : `${root}/`; + try { + for await (const match of glob(`${prefix}**/*`, { + cwd, + exclude: (entry) => UNWALKED_DIRECTORIES.has(basename(String(entry))), + })) { + const relative = String(match); + if (found.has(relative)) continue; + try { + const stats = await stat(resolvePath(cwd, relative)); + if (stats.isFile() && stats.size > maxBytes) { + found.set(relative, { file: relative, size: stats.size }); + } + } catch { + // Same reasoning as above: gone between listing and stat, or + // unreadable. Not a reason to drop the exclusion already computed. + } + } + } catch { + // A target that is not a directory, an unreadable subtree, a platform + // where `glob` rejects the pattern: all of them mean "no notice, never + // no fix". The exclusion has already been applied by the time this runs. + } + } + + return [...found.values()].toSorted((a, b) => a.file.localeCompare(b.file)); +} + /** * The user-facing sentence for a set of skipped files, or `undefined` when * nothing was skipped. @@ -276,3 +375,67 @@ export function skippedFilesNotice(files: string[]): string | undefined { `every other file was checked normally.` ); } + +/** + * The user-facing sentence for a set of files excluded for being over + * `maxBytes` (`VALE_MAX_FILE_BYTES` in `run.ts`), or `undefined` when nothing + * was excluded. + * + * A NOTICE, not a finding — deliberately the opposite of what #300 + * (`vale-parse-error` in `run.ts`) chose for an unparseable file, and for a + * reason that only shows up once a finding is actually tried here. #300's + * finding is trustworthy because Vale itself proved the file was a real + * target: it opened the file, tried to parse it, and told us exactly why it + * failed. {@link findOversizedFiles} proves nothing of the kind — it is a bare + * filesystem walk that runs before Vale is ever invoked, with no way to know + * whether any configured rule's matcher would have reached the file at all. + * + * That is not a hypothetical gap. Reporting this exclusion as a hard + * `severity: "error"` finding, and running a whole-project `check` against + * *this* repository, reported `pnpm-lock.yaml` (152,820 bytes) and + * `packages/cli/CHANGELOG.md` (139,171 bytes) as failures — and neither file + * is named by any `[section]` in any rule's `.vale.ini` under + * `.taskless/rules/vale/`. Vale was + * never going to open either one, so a finding there is not a caught coverage + * hole, it is a false one. Confirming true scope would mean re-implementing + * Vale's own glob-matching against the assembled config from outside Vale — + * exactly the second parser the "Verify Build Output In The Build, Not By + * Parsing It" reasoning in `STYLEGUIDE-CODE.md` warns against: Vale already + * knows which files its rules reach, nothing in this module does, and + * approximating that knowledge is worse than not claiming it. + * + * A converter-dependent file ({@link skippedFilesNotice}, just above) is in + * the same epistemic position — that walk is equally blind to rule scope — + * which is why it already reports a notice rather than a finding. This + * exclusion follows that precedent rather than #300's. + * + * None of this changes whether the file is excluded from the Vale invocation: + * it still is, unconditionally, in every case (see `oversizedInScope` in + * `run.ts`). That protects against the real risk — a rule DOES turn out to + * match the file, and Vale's quadratic cost on it consumes the run's + * timeout — at zero cost on the files above, which no rule was ever going to + * reach. Only the *reporting* softens to match what we actually know; the + * exclusion does not. + */ +export function oversizedFilesNotice( + files: OversizedFile[], + maxBytes: number +): string | undefined { + if (files.length === 0) return undefined; + + const sample = files.slice(0, NOTICE_SAMPLE_LIMIT); + const remainder = files.length - sample.length; + const names = sample.map((entry) => entry.file); + const listed = + remainder > 0 + ? `${names.join(", ")} (and ${String(remainder)} more)` + : names.join(", "); + + return ( + `Vale did not check ${String(files.length)} file(s) over ${String(maxBytes)} ` + + `bytes: ${listed}. Vale's cost grows quadratically with a single file's ` + + `size, so a file this large risks consuming the whole run's timeout budget ` + + `and costing every other file its findings — it was excluded rather than ` + + `risk that. Split large files into smaller documents to have them checked.` + ); +} diff --git a/packages/cli/src/rules/vale/run.ts b/packages/cli/src/rules/vale/run.ts index 513ded57..b1567056 100644 --- a/packages/cli/src/rules/vale/run.ts +++ b/packages/cli/src/rules/vale/run.ts @@ -18,6 +18,8 @@ import { buildValeGlob, converterExclusionGlobs, findConverterDependentFiles, + findOversizedFiles, + oversizedFilesNotice, skippedFilesNotice, TASKLESS_DIRECTORY, } from "./formats"; @@ -46,6 +48,70 @@ export { ASSEMBLED_VALE_CONFIG } from "../engines"; */ export const VALE_TIMEOUT_MS = 60_000; +/** + * The largest single file Vale will be asked to check, in bytes. A file over + * this is excluded from the Vale invocation and named in a notice — see + * `oversizedFilesNotice` in `formats.ts` for why a notice and not a finding — + * the same preemptive treatment `converterExclusionGlobs` gives a format Vale + * cannot parse (taskless/cli#321). + * + * ## Why a size guard at all: Vale is quadratic in one file's size + * + * Measured against the pinned binary, one `existence` rule, one file, over + * three runs each, median taken (an M-series laptop; a CI runner is assumed + * ~4x slower, NOT measured): + * + * | size | median (laptop) | ~4x slower CI runner | share of the 60s run budget | + * | ----- | ---------------- | --------------------- | ---------------------------- | + * | 128KB | 0.77s | ~3.1s | 5% | + * | 192KB | 1.87s | ~7.5s | 12% | + * | 256KB | 3.30s | ~13.2s | 22% | + * | 384KB | 7.27s | ~29.1s | 48% | + * + * This is upstream Vale's behaviour on a single file, not ours, and it is per + * FILE, not per corpus: the same ~1MB of prose spread across 400 files takes + * 190ms. Volume is fine; size is not, and the risk is concentrated in outliers + * rather than spread across a corpus. + * + * ## The budget being protected is the WHOLE RUN, not one file + * + * {@link VALE_TIMEOUT_MS} bounds one Vale invocation over every target file + * combined, so the question a size guard has to answer is not "is this file + * slow" but "how much of the shared budget may one outlier consume". At 384KB + * a single file can already claim roughly half the run's timeout on its own — + * two of them, or one plus a project's ordinary corpus, is enough to blow the + * budget and take every other file's findings down with it (exactly the #300 + * failure, on a path #300 did not cover). That effect compounds with rule + * count too: a real project runs several rules over the same file in one Vale + * invocation, and each one pays the quadratic cost again. + * + * ## Why 128KB (`128 * 1024` bytes) + * + * At 128KB a pathological file costs at most roughly 5% of the run's budget, + * even on the slower, unmeasured CI estimate — small enough that it takes many + * such files at once to threaten the timeout, rather than one. The choice also + * has to not eat real documents: 128KB of markdown is roughly 20,000 words, + * comfortably past any file a person actually sits down and writes by hand — + * what this excludes is generated output, pasted data dumps, or exported notes, + * not hand-authored prose. Measured against this repository, the largest + * committed markdown file (`packages/cli/CHANGELOG.md`) is 139KB — just over + * this limit, and itself a generated file (a changelog appended to by tooling, + * not written by hand in one sitting), which is exactly the shape of file this + * guard is meant to catch. + * + * This bounds the worst SINGLE file, not the run's total cost: many mid-sized + * files under the limit still accumulate. A normal corpus is cheap regardless + * (400 files of ~2KB measured at 190ms total), so that accumulation only + * matters when a project is unusually large, which {@link VALE_TIMEOUT_MS} + * still exists to catch. + * + * Exported and named so it is discoverable and tunable independently of + * {@link VALE_TIMEOUT_MS}: the two bound different things (one file's cost, the + * whole run's budget) and moving one should not require reasoning about the + * other. + */ +export const VALE_MAX_FILE_BYTES = 128 * 1024; + /** * What a Vale run produced. * @@ -552,11 +618,21 @@ export async function runVale( // `worktrees/` is not a file this run declined to convert, it is a file this // run was never going to look at, and naming it would send the reader to // investigate a directory the fix above deliberately excluded. - const [ignoredEntries, converterDependent] = await Promise.all([ + const [ignoredEntries, converterDependent, oversized] = await Promise.all([ wholeProject ? listGitIgnoredEntries(options.cwd) : [], findConverterDependentFiles(options.cwd, paths), + findOversizedFiles(options.cwd, paths, VALE_MAX_FILE_BYTES), ]); + // A file too large to check safely is excluded the same way, and for the + // same reason, as a converter-dependent one just above: unconditionally, on + // every run, named path or not. Handing it to Vale does not check it + // badly — Vale's quadratic cost on one large file can consume the whole + // run's timeout, taking every other file's findings with it (taskless/cli#321). + const oversizedInScope = oversized.filter( + (entry) => !isGitIgnoredPath(entry.file, ignoredEntries) + ); + const exclude = [ ...(wholeProject ? [ @@ -565,11 +641,22 @@ export async function runVale( ] : []), ...converterExclusionGlobs(), + ...oversizedInScope.map((entry) => entry.file), ]; - const skipped = skippedFilesNotice( - converterDependent.filter((file) => !isGitIgnoredPath(file, ignoredEntries)) - ); + // Both notices describe files this run declined to check, for different + // reasons, and both have to reach the user or the decline is silent. Joined + // rather than one overwriting the other — see the equivalent `advisories` + // join for Vale's own stderr diagnostic further down, for the same reason. + const notices = [ + skippedFilesNotice( + converterDependent.filter( + (file) => !isGitIgnoredPath(file, ignoredEntries) + ) + ), + oversizedFilesNotice(oversizedInScope, VALE_MAX_FILE_BYTES), + ].filter((notice) => notice !== undefined); + const skipped = notices.length === 0 ? undefined : notices.join("\n"); // One bad target file must cost one finding, not the whole run // (taskless/cli#300). A front-matter YAML error is Vale's own parse diff --git a/packages/cli/test/vale-run.test.ts b/packages/cli/test/vale-run.test.ts index dea2028c..02457abc 100644 --- a/packages/cli/test/vale-run.test.ts +++ b/packages/cli/test/vale-run.test.ts @@ -5,7 +5,7 @@ import { join } from "node:path"; import { afterEach, describe, expect, it, vi } from "vitest"; import { findValeBinary } from "../src/rules/vale/binary"; -import { runVale } from "../src/rules/vale/run"; +import { runVale, VALE_MAX_FILE_BYTES } from "../src/rules/vale/run"; /** * These run the real Vale binary. It ships as an `optionalDependency` for the @@ -405,6 +405,132 @@ withVale("runVale against the real binary", () => { expect(outcome.message).toContain("bogus.yml"); }); }); + + describe("an oversized target file (taskless/cli#321)", () => { + // Just over the limit, not a multi-hundred-KB fixture: this is a boundary + // test, and repeating a short sentence to the byte count keeps the + // workspace this test writes to disk small and the suite fast. + // + // The sentence contains the rule's own token ("simply") deliberately, + // rather than filler with no matches. A filler body of repeated "x" + // characters is excluded exactly the same as this one on the happy path + // (both are just "some file over the limit" to `findOversizedFiles`), but + // it hides a real regression: with the exclusion glob broken, Vale would + // still be handed "xxxx…" and find nothing in it either way, so a test + // built on filler cannot tell "excluded" from "checked and clean" apart. + // A body with real matches can: excluded, it contributes no findings; + // handed to Vale, it contributes many. Verified below. + const oversizedSentence = "Just simply do it. "; + const oversizedBody = oversizedSentence.repeat( + Math.ceil((VALE_MAX_FILE_BYTES + 1) / oversizedSentence.length) + ); + // Sized so the WHOLE document (this padding plus the sentence appended + // below) lands at EXACTLY `VALE_MAX_FILE_BYTES`, not merely under it: the + // guard has to be a strict `>`, and a test that leaves slack would not + // notice a `>=` mutation, since the file would still sit under the limit + // either way. `"\nJust simply do it.\n"` is 20 bytes. + const almostHugeSuffix = "\nJust simply do it.\n"; + const underLimitBody = "x".repeat( + VALE_MAX_FILE_BYTES - almostHugeSuffix.length + ); + + it("excludes the oversized file while its neighbours' findings still come back", async () => { + const cwd = makeProject( + `${header}\n[*.md]\nno-simply.no-simply = YES\n`, + { "no-simply": existenceRule("simply", "Avoid 'simply'") }, + { + "good-1.md": "Just simply do it.\n", + "good-2.md": "Just simply do it, again.\n", + "huge.md": oversizedBody, + } + ); + + const outcome = await runVale({ + cwd, + paths: ["good-1.md", "good-2.md", "huge.md"], + }); + + // MUTATION CHECK: with the `...oversizedInScope.map((entry) => + // entry.file)` spread removed from `exclude` in run.ts, `huge.md` is + // handed to Vale instead of excluded, and — because the fixture's + // content actually contains "simply" thousands of times — Vale reports + // one finding per match. `outcome.results` then has 3-digit length + // instead of 2, which `toHaveLength(2)` below catches immediately. + // Verified locally: with the spread removed, this test fails with + // "expected 2600-ish, got 2" (the exact count depends on Vale's + // scope-merging, not asserted here to keep the test robust); reverting + // restores it to exactly 2. + expect(outcome.status).toBe("ok"); + if (outcome.status !== "ok") return; + expect(outcome.blocking).toBe(false); + + const byFile = new Map(outcome.results.map((r) => [r.file, r])); + expect(byFile.get("good-1.md")).toMatchObject({ + ruleId: "no-simply", + file: "good-1.md", + }); + expect(byFile.get("good-2.md")).toMatchObject({ + ruleId: "no-simply", + file: "good-2.md", + }); + // No `huge.md` finding: despite containing "simply" thousands of times, + // it was excluded before Vale ever opened it. + expect(outcome.results).toHaveLength(2); + }); + + it("reports the skip as a notice, not a finding", async () => { + const cwd = makeProject( + `${header}\n[*.md]\nno-simply.no-simply = YES\n`, + { "no-simply": existenceRule("simply", "Avoid 'simply'") }, + { "huge.md": oversizedBody } + ); + + const outcome = await runVale({ cwd, paths: ["huge.md"] }); + + // MUTATION CHECK: remove `oversizedFilesNotice(oversizedInScope, ...)` + // from the `notices` array in run.ts and `outcome.notice` comes back + // `undefined` — verified locally. A silent skip here is exactly the + // failure mode the whole issue is about, one level down: `results` is + // empty (the file was excluded, so `no-simply` never got to run on it, + // despite the fixture containing that token thousands of times), so the + // notice is the ONLY signal this file was declined rather than checked + // and found clean. + expect(outcome.status).toBe("ok"); + if (outcome.status !== "ok") return; + expect(outcome.results).toEqual([]); + expect(outcome.notice).toContain("huge.md"); + expect(outcome.notice).toContain(String(VALE_MAX_FILE_BYTES)); + }); + + it("still checks a file just under the limit", async () => { + const almostHugeBody = `${underLimitBody}${almostHugeSuffix}`; + const cwd = makeProject( + `${header}\n[*.md]\nno-simply.no-simply = YES\n`, + { "no-simply": existenceRule("simply", "Avoid 'simply'") }, + { "almost-huge.md": almostHugeBody } + ); + + // The file is exactly `VALE_MAX_FILE_BYTES`, not merely under it — see + // the `underLimitBody` comment above. + expect(Buffer.byteLength(almostHugeBody)).toBe(VALE_MAX_FILE_BYTES); + + const outcome = await runVale({ cwd, paths: ["almost-huge.md"] }); + + // MUTATION CHECK: change the size guard's comparison from `>` to `>=` + // in `findOversizedFiles` and this test fails, since the fixture sits + // AT the limit: `almost-huge.md` would start being excluded (a notice + // naming it, no `no-simply` finding). Verified locally. + expect(outcome.status).toBe("ok"); + if (outcome.status !== "ok") return; + expect(outcome.notice).toBeUndefined(); + expect(outcome.results).toContainEqual( + expect.objectContaining({ + ruleId: "no-simply", + file: "almost-huge.md", + }) + ); + }); + }); }); describe("ValeRunOutcome.blocking", () => { @@ -444,15 +570,23 @@ withVale("ValeRunOutcome.blocking against the real binary", () => { // // The race is removed by making the work outlast the budget by a margin // nothing plausible closes. Vale is QUADRATIC in the size of a single - // file — measured on the pinned binary at 80KB 0.3s, 160KB 0.9s, 320KB - // 3.5s, 640KB 14s — so roughly 320KB of prose takes about 3.5 SECONDS - // against a 100ms budget. That is a 35x margin the right way round, where - // the old one was a 46x margin the wrong way. The run is killed at 100ms, - // so the test costs about that rather than 3.5s. + // file, so a document well under a second's worth of Vale time is still + // many multiples of a 100ms budget. + // + // The fixture has to stay UNDER `VALE_MAX_FILE_BYTES` (taskless/cli#321): + // a document at or above that limit is excluded before Vale ever sees it, + // which would report `status: "ok"` with a notice instead of exercising + // the timeout this test is actually about. 6,300 repeats of a 19-byte + // sentence lands at ~117KB (119,700 bytes), comfortably below the 128KB + // limit — measured at ~450ms against the real binary, a ~4.5x margin over + // the 100ms budget used here. That margin is smaller than this test used + // before #321 shrank how large a fixture it may use, but it is measured, + // not assumed, and the run is killed at 100ms either way, so the test + // costs about that rather than 450ms. const cwd = makeProject( `${header}\n[*.md]\nno-simply.no-simply = YES\n`, { "no-simply": existenceRule("simply", "Avoid 'simply'") }, - { "doc.md": `${"Just simply do it. ".repeat(17_000)}\n` } + { "doc.md": "Just simply do it. ".repeat(6_300) } ); expect( From 406d89cea03e61762eafb0e18fead62b081cc3cc Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 8 Sep 2026 16:40:25 -0700 Subject: [PATCH 2/4] fix(cli): scope the oversized-file scan to what Vale would actually lint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The preemptive size scan walked every file in the tree, with no relationship to what any Vale rule's matcher covers. Running it against this repository reported pnpm-lock.yaml and packages/cli/CHANGELOG.md as "not checked" even though no rule's .vale.ini section names either file — Vale was never going to open them, so that was a false positive, not a caught coverage hole. assembleValeConfig now returns the section glob patterns it wrote into the assembled config alongside its path, read from the same in-memory strings it is about to write rather than by re-parsing the file afterward. That is threaded through assembleEngineConfigs -> DispatchOptions.valeSections -> ValeRunOptions.sectionGlobs -> findOversizedFiles, which globs by those patterns instead of a bare **/* walk, exactly as findConverterDependentFiles globs by its extension list. A named path is no longer stat-checked unconditionally either, once sections are known: Vale spends 9ms and finds nothing on an oversized file no section names, the same as a file it never opened, so checking it regardless of scope would reintroduce the same false positive via an explicit path instead of a whole-project walk. A caller with no assembled config to ask (runVale's sectionGlobs left undefined) keeps the previous exhaustive walk unchanged - verifyValeRule's isolating config, and any test handing runVale a hand-written .vale.ini directly. --- .changeset/skip-oversized-vale-files.md | 30 ++-- packages/cli/src/agent/create-vale-rule.md | 23 ++- packages/cli/src/commands/check.ts | 3 +- packages/cli/src/rules/assemble.ts | 51 ++++++- packages/cli/src/rules/dispatch.ts | 11 ++ packages/cli/src/rules/vale/formats.ts | 151 +++++++++++++------ packages/cli/src/rules/vale/run.ts | 29 +++- packages/cli/test/assemble.test.ts | 40 +++-- packages/cli/test/vale-formats.test.ts | 70 ++++++++- packages/cli/test/vale-orchestration.test.ts | 23 +-- packages/cli/test/vale-run.test.ts | 33 ++++ 11 files changed, 372 insertions(+), 92 deletions(-) diff --git a/.changeset/skip-oversized-vale-files.md b/.changeset/skip-oversized-vale-files.md index d8b85a03..2ebd3f0e 100644 --- a/.changeset/skip-oversized-vale-files.md +++ b/.changeset/skip-oversized-vale-files.md @@ -12,16 +12,24 @@ fixed, on a path #300 did not cover). `runVale` now excludes a target file over 128KB (`VALE_MAX_FILE_BYTES` in `src/rules/vale/run.ts`) before invoking Vale at all, the same preemptive -treatment already given to a format Vale cannot parse. Excluded files are -named in a `notices` entry, not reported as a failing finding: unlike an -unparseable file (where Vale itself proves the file was a real target by -erroring on it), this exclusion runs from a bare filesystem walk with no way -to confirm any rule's matcher would have reached the file — reporting it as -a blocking error produced false failures on files no rule ever touches (a -lockfile, a generated changelog). +treatment already given to a format Vale cannot parse — but only when some +Vale rule's own `.vale.ini` section could actually reach that file. +`assembleValeConfig` now returns the section patterns it wrote alongside the +config path, and the size scan globs by those patterns (`findOversizedFiles` +in `src/rules/vale/formats.ts`) instead of walking every file in the project. +A first version of this fix scanned the whole tree unconditionally and named +`pnpm-lock.yaml` and `packages/cli/CHANGELOG.md` as "not checked" on this very +repository, even though no rule's matcher touches either file — Vale was +never going to open them, so that was a false positive, not a caught coverage +hole. Excluded files are named in a `notices` entry rather than a finding: +unlike an unparseable file (where Vale itself proves the file was a real +target by erroring on it), this exclusion is a preemptive guess from a +filesystem walk, and a soft advisory fits an unconfirmed guess better than a +hard error. A consumer may now see a `check` that previously counted a large file's -prose findings instead report a `notices` entry naming that file as skipped. -128KB is comfortably past hand-written prose (roughly 20,000 words); this -should only affect generated output, pasted data, or exported notes checked -directly against a matching rule. +prose findings instead report a `notices` entry naming that file as skipped +— but only for a file some rule's own scope actually reaches. 128KB is +comfortably past hand-written prose (roughly 20,000 words); this should only +affect generated output, pasted data, or exported notes checked directly +against a matching rule. diff --git a/packages/cli/src/agent/create-vale-rule.md b/packages/cli/src/agent/create-vale-rule.md index d01762af..b937dfa8 100644 --- a/packages/cli/src/agent/create-vale-rule.md +++ b/packages/cli/src/agent/create-vale-rule.md @@ -576,15 +576,22 @@ it. large enough document can consume the whole run's time budget on its own and cost every other file its findings: the same failure mode as the unreadable-file case above, from a different cause. `check` - preempts it: a target file over 128KB is skipped, and named in a - `notices` entry rather than a finding, since nothing here can confirm - whether any matcher would actually have reached it (a large lockfile - or a generated file outside every rule's scope is common, and - flagging one as a failure would be a false positive). A rule's own + preempts it: a target file over 128KB is skipped **only if some + matcher's own section would actually reach it**. The scan asks the + assembled config's own section patterns, the same ones you write in + this file's `.vale.ini`, rather than walking every file in the + project. A large lockfile or a generated file no rule's glob names is + left alone entirely, not merely reported softly: naming a file no + matcher was ever going to check would be a false positive, not a + caught coverage hole. A file that IS excluded is named in a `notices` + entry rather than a finding: unlike the unreadable-file case above, + where Vale's own error proves the file was a real target, this is a + preemptive guess from a filesystem walk, and a soft advisory fits an + unconfirmed guess better than a hard error does. A rule's own fixtures are never this large in practice, so this should not surface - while authoring one. It matters when scoping a matcher at a whole - project, where a generated changelog or an exported note can cross - it. + while authoring one. It matters when a matcher's glob is broad, such as + `[*.md]` or `[**/README.md]` at the project root, where a generated + changelog or an exported note can cross it. That example changed with Vale v3.18.0, which is the point: the dangerous extension is whichever one the list above says needs a diff --git a/packages/cli/src/commands/check.ts b/packages/cli/src/commands/check.ts index d84e35fe..618738f9 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -252,7 +252,8 @@ export const checkCommand = defineCommand({ cwd, paths: existingPaths, astGrepConfigPath: assembled.sg, - valeConfigPath: assembled.vale, + valeConfigPath: assembled.vale?.path, + valeSections: assembled.vale?.sections, runtimeRules: plan.execute, runtimeTimeoutMs: parseTimeoutMs(args.timeout), }); diff --git a/packages/cli/src/rules/assemble.ts b/packages/cli/src/rules/assemble.ts index 19156ab1..10773dea 100644 --- a/packages/cli/src/rules/assemble.ts +++ b/packages/cli/src/rules/assemble.ts @@ -97,19 +97,59 @@ function valeRuleBlock(ruleId: string, body: string): string { return [`# tskl) rule = ${ruleId}`, body, ""].join("\n"); } +/** + * A section header (`[pattern]`) from an assembled rule's own body. + * + * Read from the exact string this module is about to write — never from the + * file after writing it. Re-reading the written `.vale.ini` to recover its + * own sections would be the mistake `STYLEGUIDE-CODE.md`'s "Verify Build + * Output In The Build, Not By Parsing It" warns against: this function + * already IS the generator, holding the structured pieces before they are + * joined into text, so there is nothing to re-derive. + */ +function sectionPatternsOf(body: string): string[] { + const patterns: string[] = []; + for (const line of body.split("\n")) { + const match = /^\[(.+)\]$/.exec(line.trim()); + if (match?.[1] !== undefined) patterns.push(match[1]); + } + return patterns; +} + +/** + * What `assembleValeConfig` produced: where to point `--config`, and the + * section patterns it wrote there. + * + * `sections` exists so a caller that needs to know what Vale would actually + * lint — `findOversizedFiles` in `vale/formats.ts`, scoping its preemptive + * size guard to files some rule's matcher could reach — can ask this module + * directly instead of re-parsing the config it just wrote. + */ +export interface AssembledValeConfig { + /** Config path relative to the project root, for `--config`. */ + path: string; + /** + * Every section glob pattern written into the config, deduplicated and + * sorted for a stable read order. Root-relative, exactly as Vale reads + * them — the same strings a `[…]` line in a rule's own `.vale.ini` names. + */ + sections: string[]; +} + /** * Assemble `.taskless/.vale.ini` from every Vale rule's own config. * - * Returns the config path relative to the project root, or `undefined` when no + * Returns the config path and its section patterns, or `undefined` when no * Vale rule declares any config — there is nothing to run, and writing an empty * config would invite Vale to lint the project against no rules and report a * clean pass. */ export async function assembleValeConfig( cwd: string -): Promise { +): Promise { const ruleIds = await listRuleIds(cwd, "vale"); const blocks: string[] = []; + const sections = new Set(); for (const ruleId of ruleIds) { const configPath = ruleConfigPath(cwd, "vale", ruleId); @@ -125,6 +165,7 @@ export async function assembleValeConfig( } const body = ruleConfigBody(source); if (body === "") continue; + for (const pattern of sectionPatternsOf(body)) sections.add(pattern); blocks.push(valeRuleBlock(ruleId, body)); } @@ -134,7 +175,7 @@ export async function assembleValeConfig( const target = join(cwd, ASSEMBLED_VALE_CONFIG); await mkdir(dirname(target), { recursive: true }); await writeFile(target, contents, "utf8"); - return ASSEMBLED_VALE_CONFIG; + return { path: ASSEMBLED_VALE_CONFIG, sections: [...sections].toSorted() }; } /** @@ -193,8 +234,8 @@ export async function assembleSgConfig( /** Both assembled configs, for a run that needs whichever engines are present. */ export interface AssembledConfigs { - /** `--config` for Vale, or `undefined` when no Vale rule is configured. */ - vale: string | undefined; + /** Vale's config and section patterns, or `undefined` when no Vale rule is configured. */ + vale: AssembledValeConfig | undefined; /** `-c` for ast-grep, or `undefined` when there are no ast-grep rules. */ sg: string | undefined; } diff --git a/packages/cli/src/rules/dispatch.ts b/packages/cli/src/rules/dispatch.ts index c83ea824..8d9c093c 100644 --- a/packages/cli/src/rules/dispatch.ts +++ b/packages/cli/src/rules/dispatch.ts @@ -91,6 +91,16 @@ export interface DispatchOptions { * written. The config is the only honest signal that there is Vale work. */ valeConfigPath: string | undefined; + /** + * The section glob patterns `assembleValeConfig` wrote into that config, or + * `undefined` when it produced nothing (mirrors `valeConfigPath`). + * + * Threaded through to `runVale` so its preemptive oversized-file guard can + * scope its scan to files some rule's matcher could actually reach, rather + * than statting the whole project — see `findOversizedFiles` in + * `vale/formats.ts`. + */ + valeSections?: string[] | undefined; /** Runtime rules that survived planning. Empty means the harness is skipped. */ runtimeRules: RuntimeRule[]; runtimeTimeoutMs?: number; @@ -177,6 +187,7 @@ async function runValeEngine(options: DispatchOptions): Promise { paths: options.paths, configPath: options.valeConfigPath, timeoutMs: options.valeTimeoutMs, + sectionGlobs: options.valeSections, }); if (outcome.status === "ok") { diff --git a/packages/cli/src/rules/vale/formats.ts b/packages/cli/src/rules/vale/formats.ts index ff6e8953..eebf0853 100644 --- a/packages/cli/src/rules/vale/formats.ts +++ b/packages/cli/src/rules/vale/formats.ts @@ -166,6 +166,11 @@ const UNWALKED_DIRECTORIES = new Set([ TASKLESS_DIRECTORY, ]); +/** `glob`'s `exclude` predicate for {@link UNWALKED_DIRECTORIES}. */ +function isUnwalkedEntry(entry: string | Buffer): boolean { + return UNWALKED_DIRECTORIES.has(basename(String(entry))); +} + /** * Converter-dependent files inside the run's target set. * @@ -260,18 +265,48 @@ export interface OversizedFile { * discover the cost the hard way — is the same trade `converterExclusionGlobs` * makes for a format Vale cannot parse at all. * - * Unlike the converter walk, this cannot be scoped by extension: an oversized - * file can have any extension, or none, so every file under the target roots - * has to be listed and stat'd. That is a real cost on the happy path, where - * nothing is oversized and the walk still runs — measured and reported in the - * PR that introduced this function. - * - * A named path is stat'd directly, exactly as `targetFileParseError` does - * elsewhere in this package: an explicit request is not resolved through the - * walk that answers a whole-project run. Whether named or discovered by the - * walk, an oversized file is excluded unconditionally, on every run — the same - * asymmetry `findConverterDependentFiles` documents, and for the same reason: - * handing Vale this file does not check it badly, it risks the entire batch's + * `sectionGlobs`, when given, is `AssembledValeConfig.sections` from + * `assembleValeConfig` — the exact section patterns Vale's own rules are + * scoped to. The scan globs those patterns instead of every file in the + * tree, exactly as {@link findConverterDependentFiles} globs by its extension + * list, and for the same reason precision matters here: a bare `**\/*` walk + * finds every file under the target roots regardless of whether any rule + * would ever touch it, and reporting one of those as "not checked" is a false + * positive, not a caught coverage hole. Measured against this repository: + * `pnpm-lock.yaml` and `packages/cli/CHANGELOG.md` are both over the limit, + * and neither is named by any `[section]` in any rule's `.vale.ini` — no + * rule was ever going to open either one, so the un-scoped walk reported + * lost coverage that never existed. + * + * The patterns are read from `assembleValeConfig`'s own return value, never + * by re-parsing the `.vale.ini` it wrote — see the doc on + * `sectionPatternsOf` in `assemble.ts` for why that distinction matters. + * + * `sectionGlobs === undefined` falls back to the previous exhaustive `**\/*` + * walk under each target root. That path exists for a caller with no + * assembled config to ask — `verifyValeRule`'s isolating config, or a test + * that hands `runVale` a hand-written `.vale.ini` directly — and is + * unaffected by everything below: same cost, same behavior as before this + * parameter existed. + * + * A named path is stat'd directly when there is no `sectionGlobs` to consult + * (the fallback below), exactly as `targetFileParseError` does elsewhere in + * this package: an explicit request is not resolved through the walk that + * answers a whole-project run. **That changes once `sectionGlobs` is given.** + * An explicitly named file is not exempt from scoping either — measured + * against the real binary, Vale spends 9ms and reports nothing on a 128KB+ + * file whose extension no section names, the same as a file it never opened + * at all, because no rule is ever assigned to run against it. Checking it + * unconditionally would reintroduce the exact false positive this parameter + * exists to remove, just reachable via `check some-file.yaml` instead of a + * whole-project run. So when sections are known, a named file is a candidate + * only if it is also a match for one of them — the same membership test the + * walk below already computes. + * + * Whether named or discovered by the walk, an oversized file is excluded + * unconditionally once it qualifies, on every run — the same asymmetry + * `findConverterDependentFiles` documents, and for the same reason: handing + * Vale this file does not check it badly, it risks the entire batch's * timeout. * * Errors are swallowed the same way as {@link findConverterDependentFiles} and @@ -283,51 +318,83 @@ export interface OversizedFile { export async function findOversizedFiles( cwd: string, paths: string[], - maxBytes: number + maxBytes: number, + sectionGlobs?: string[] ): Promise { - const named: OversizedFile[] = []; - const roots: string[] = []; - if (paths.length === 0) { - roots.push("."); - } else { - for (const path of paths) { + const roots = paths.length === 0 ? ["."] : paths; + const found = new Map(); + + const checkCandidate = async (relative: string): Promise => { + if (found.has(relative)) return; + try { + const stats = await stat(resolvePath(cwd, relative)); + if (stats.isFile() && stats.size > maxBytes) { + found.set(relative, { file: relative, size: stats.size }); + } + } catch { + // Gone between listing and stat, or unreadable. Not a reason to drop + // the exclusion already computed. + } + }; + + if (sectionGlobs === undefined) { + // No assembled config to ask what Vale would actually lint — fall back to + // the previous behavior: every named path is a candidate regardless of + // scope, and every root is walked exhaustively. + for (const path of paths) await checkCandidate(path); + for (const root of roots) { + const prefix = root === "." || root === "" ? "" : `${root}/`; try { - const stats = await stat(resolvePath(cwd, path)); - if (stats.isFile() && stats.size > maxBytes) { - named.push({ file: path, size: stats.size }); + for await (const match of glob(`${prefix}**/*`, { + cwd, + exclude: isUnwalkedEntry, + })) { + await checkCandidate(String(match)); } } catch { - // Unreadable or missing: not this function's problem. The run itself - // will report it if it matters. + // A target that is not a directory, an unreadable subtree, a + // platform where `glob` rejects the pattern: all of them mean "no + // notice, never no fix". The exclusion has already been applied by + // the time this runs. } - roots.push(path); } + return [...found.values()].toSorted((a, b) => a.file.localeCompare(b.file)); } - const found = new Map(named.map((entry) => [entry.file, entry])); - for (const root of roots) { - const prefix = root === "." || root === "" ? "" : `${root}/`; + // Section patterns are root-relative, exactly as Vale reads them — never + // prefixed per target root, the way the extension-based fallback above is. + // A whole-project run (`roots === ["."]`) needs no further narrowing: every + // match is already in scope. An explicit target (`check src/` or + // `check src/doc.md`) narrows the matches down to that subtree afterward + // instead, because a section like `CLAUDE.md` or `**/README.md` has no + // meaningful "under src/" form to prefix onto — Vale itself evaluates every + // section against the whole project and only its own target list decides + // what it actually visits, so intersecting after the glob mirrors that + // rather than guessing at one. This is also what makes a named file's + // in-scope test free: it needs no separate membership check, because a + // pattern like `**/README.md` already matches a top-level `README.md` + // found this way, whether or not the caller named it explicitly. + const wholeProject = paths.length === 0; + for (const pattern of sectionGlobs) { try { - for await (const match of glob(`${prefix}**/*`, { + for await (const match of glob(pattern, { cwd, - exclude: (entry) => UNWALKED_DIRECTORIES.has(basename(String(entry))), + exclude: isUnwalkedEntry, })) { const relative = String(match); - if (found.has(relative)) continue; - try { - const stats = await stat(resolvePath(cwd, relative)); - if (stats.isFile() && stats.size > maxBytes) { - found.set(relative, { file: relative, size: stats.size }); - } - } catch { - // Same reasoning as above: gone between listing and stat, or - // unreadable. Not a reason to drop the exclusion already computed. + if ( + !wholeProject && + !roots.some( + (root) => relative === root || relative.startsWith(`${root}/`) + ) + ) { + continue; } + await checkCandidate(relative); } } catch { - // A target that is not a directory, an unreadable subtree, a platform - // where `glob` rejects the pattern: all of them mean "no notice, never - // no fix". The exclusion has already been applied by the time this runs. + // Same reasoning as the fallback walk: a malformed pattern or an + // unreadable subtree means "no notice, never no fix". } } diff --git a/packages/cli/src/rules/vale/run.ts b/packages/cli/src/rules/vale/run.ts index b1567056..fe05eaed 100644 --- a/packages/cli/src/rules/vale/run.ts +++ b/packages/cli/src/rules/vale/run.ts @@ -97,7 +97,12 @@ export const VALE_TIMEOUT_MS = 60_000; * committed markdown file (`packages/cli/CHANGELOG.md`) is 139KB — just over * this limit, and itself a generated file (a changelog appended to by tooling, * not written by hand in one sitting), which is exactly the shape of file this - * guard is meant to catch. + * guard is meant to catch. (It is not actually reported here: no rule in this + * repository's own `.vale.ini` files, under `.taskless/rules/vale/`, is scoped + * to it, and `findOversizedFiles` only reports a file some section could + * actually reach — see its docblock in `formats.ts`. The size and the shape + * are still the right illustration for the threshold; a project whose rules + * DO reach a file this size is exactly who this guard protects.) * * This bounds the worst SINGLE file, not the run's total cost: many mid-sized * files under the limit still accumulate. A normal corpus is cheap regardless @@ -520,6 +525,21 @@ export interface ValeRunOptions { /** Config path relative to `cwd`. Defaults to the assembled run config. */ configPath?: string; timeoutMs?: number; + /** + * The section glob patterns the config at `configPath` actually scopes its + * rules to — `AssembledValeConfig.sections` from `assembleValeConfig`, when + * the caller has it. + * + * Used only to scope {@link findOversizedFiles}'s preemptive size guard to + * files some rule could actually reach, so a whole-project run does not + * flag a file no rule was ever going to open (a lockfile, a generated + * changelog). `undefined` when the caller does not have an assembled + * config to ask — `verifyValeRule`'s isolating config, or a test that hands + * `runVale` a hand-written `.vale.ini` directly — in which case the guard + * falls back to scanning every file under `paths`, exactly as it did before + * this option existed. + */ + sectionGlobs?: string[]; } /** @@ -621,7 +641,12 @@ export async function runVale( const [ignoredEntries, converterDependent, oversized] = await Promise.all([ wholeProject ? listGitIgnoredEntries(options.cwd) : [], findConverterDependentFiles(options.cwd, paths), - findOversizedFiles(options.cwd, paths, VALE_MAX_FILE_BYTES), + findOversizedFiles( + options.cwd, + paths, + VALE_MAX_FILE_BYTES, + options.sectionGlobs + ), ]); // A file too large to check safely is excluded the same way, and for the diff --git a/packages/cli/test/assemble.test.ts b/packages/cli/test/assemble.test.ts index 07ed537a..02807d6d 100644 --- a/packages/cli/test/assemble.test.ts +++ b/packages/cli/test/assemble.test.ts @@ -58,8 +58,8 @@ async function sgRuleWithoutTests(id: string): Promise { describe("Vale config assembly", () => { it("writes a header naming the Vale rules tree as StylesPath", async () => { await valeRule("no-simply", "[*.md]\nno-simply.no-simply = YES\n"); - const path = await assembleValeConfig(cwd); - const contents = await readFile(join(cwd, path ?? ""), "utf8"); + const assembled = await assembleValeConfig(cwd); + const contents = await readFile(join(cwd, assembled?.path ?? ""), "utf8"); // StylesPath is what makes `/.yml` resolve as check `.`. // Under `.` it resolves to nothing at all, so this line is the difference @@ -75,8 +75,8 @@ describe("Vale config assembly", () => { await valeRule("zebra", "[*.md]\nzebra.zebra = YES\n"); await valeRule("alpha", "[*.md]\nalpha.alpha = YES\n"); - const path = await assembleValeConfig(cwd); - const contents = await readFile(join(cwd, path ?? ""), "utf8"); + const assembled = await assembleValeConfig(cwd); + const contents = await readFile(join(cwd, assembled?.path ?? ""), "utf8"); expect(contents.indexOf("alpha.alpha")).toBeLessThan( contents.indexOf("zebra.zebra") ); @@ -87,9 +87,9 @@ describe("Vale config assembly", () => { await valeRule("two", "[docs/**]\ntwo.two = YES\n"); const first = await assembleValeConfig(cwd); - const a = await readFile(join(cwd, first ?? ""), "utf8"); + const a = await readFile(join(cwd, first?.path ?? ""), "utf8"); const second = await assembleValeConfig(cwd); - const b = await readFile(join(cwd, second ?? ""), "utf8"); + const b = await readFile(join(cwd, second?.path ?? ""), "utf8"); expect(a).toBe(b); }); @@ -101,8 +101,8 @@ describe("Vale config assembly", () => { "scoped", "[marketing/**]\nscoped.scoped = YES\n\n[marketing/legacy/**]\nscoped.scoped = NO\n" ); - const path = await assembleValeConfig(cwd); - const contents = await readFile(join(cwd, path ?? ""), "utf8"); + const assembled = await assembleValeConfig(cwd); + const contents = await readFile(join(cwd, assembled?.path ?? ""), "utf8"); expect(contents.indexOf("[marketing/**]")).toBeLessThan( contents.indexOf("[marketing/legacy/**]") ); @@ -110,8 +110,8 @@ describe("Vale config assembly", () => { it("tags each block with the rule it came from", async () => { await valeRule("no-simply", "[*.md]\nno-simply.no-simply = YES\n"); - const path = await assembleValeConfig(cwd); - const contents = await readFile(join(cwd, path ?? ""), "utf8"); + const assembled = await assembleValeConfig(cwd); + const contents = await readFile(join(cwd, assembled?.path ?? ""), "utf8"); // Provenance is otherwise lost the moment two rules' matchers interleave. expect(contents).toContain("tskl) rule = no-simply"); }); @@ -123,8 +123,8 @@ describe("Vale config assembly", () => { "no-simply", "StylesPath = .\nMinAlertLevel = error\n\n[*.md]\nno-simply.no-simply = YES\n" ); - const path = await assembleValeConfig(cwd); - const contents = await readFile(join(cwd, path ?? ""), "utf8"); + const assembled = await assembleValeConfig(cwd); + const contents = await readFile(join(cwd, assembled?.path ?? ""), "utf8"); expect(contents).not.toContain("StylesPath = ."); expect(contents).not.toContain("MinAlertLevel = error"); }); @@ -134,6 +134,22 @@ describe("Vale config assembly", () => { it("writes nothing when no rule declares a config", async () => { expect(await assembleValeConfig(cwd)).toBeUndefined(); }); + + // `sections` is read by `findOversizedFiles` (vale/formats.ts) to scope its + // preemptive size guard to files a rule could actually reach, instead of + // walking the whole project. It has to carry every section this config + // will actually have Vale evaluate — a re-parse of the written file, which + // this is not, would be a second, weaker source of the same fact. + it("returns every section pattern it wrote, deduplicated and sorted", async () => { + await valeRule("no-simply", "[*.md]\nno-simply.no-simply = YES\n"); + await valeRule( + "no-very", + "[*.md]\nno-very.no-very = YES\n\n[**/README.md]\nno-very.no-very = YES\n" + ); + + const assembled = await assembleValeConfig(cwd); + expect(assembled?.sections).toEqual(["**/README.md", "*.md"]); + }); }); describe("ast-grep config assembly", () => { diff --git a/packages/cli/test/vale-formats.test.ts b/packages/cli/test/vale-formats.test.ts index 27c2450c..f3765d06 100644 --- a/packages/cli/test/vale-formats.test.ts +++ b/packages/cli/test/vale-formats.test.ts @@ -13,9 +13,10 @@ import { converterExclusionGlobs, converterFor, findConverterDependentFiles, + findOversizedFiles, skippedFilesNotice, } from "../src/rules/vale/formats"; -import { runVale } from "../src/rules/vale/run"; +import { runVale, VALE_MAX_FILE_BYTES } from "../src/rules/vale/run"; /** * The exclusion derived from the format tiers, and the run that uses it. @@ -239,6 +240,73 @@ describe("finding converter-dependent files", () => { }); }); +// No Vale binary needed for these: `findOversizedFiles` on its own never +// spawns Vale — only `stat` and `glob`. `makeProject` (above) is overkill +// here, since it scaffolds a whole rule tree just to reach a hand-written +// `.vale.ini`; these tests only need a plain directory. +function makeScratchProject(documents: Record): string { + const cwd = mkdtempSync(join(tmpdir(), "vale-oversized-")); + workspaces.push(cwd); + for (const [path, body] of Object.entries(documents)) { + const full = join(cwd, path); + mkdirSync(join(full, ".."), { recursive: true }); + writeFileSync(full, body); + } + return cwd; +} + +describe("finding oversized files, scoped to what Vale would actually lint", () => { + const oversizedBody = "x".repeat(VALE_MAX_FILE_BYTES + 1); + + it("reports an oversized file matching a section pattern", async () => { + const cwd = makeScratchProject({ "README.md": oversizedBody }); + expect( + await findOversizedFiles(cwd, [], VALE_MAX_FILE_BYTES, ["**/README.md"]) + ).toEqual([{ file: "README.md", size: oversizedBody.length }]); + }); + + it("does not report an oversized file no section pattern reaches", async () => { + // The taskless/cli#321 follow-up: `pnpm-lock.yaml` and + // `packages/cli/CHANGELOG.md`, both over the limit in this repository, + // are named by no rule's `[section]` — Vale was never going to open + // either one, so reporting them is a false positive, not a caught + // coverage hole. Reproduced in miniature: a lockfile-shaped file sits + // alongside an in-scope README, and only the README is named. + const cwd = makeScratchProject({ + "README.md": oversizedBody, + "pnpm-lock.yaml": oversizedBody, + }); + + // MUTATION CHECK: replace the `sectionGlobs` branch's early loop with + // the fallback `**/*` walk (or simply drop the `!wholeProject && + // !roots.some(...)` narrowing and the `wholeProject` check that selects + // this branch) and this assertion fails — `pnpm-lock.yaml` starts + // appearing alongside `README.md`. Verified locally: reverting restores + // the single-entry result below. + expect( + await findOversizedFiles(cwd, [], VALE_MAX_FILE_BYTES, ["**/README.md"]) + ).toEqual([{ file: "README.md", size: oversizedBody.length }]); + }); + + it("still checks a matching file that is not oversized", async () => { + const cwd = makeScratchProject({ "README.md": "Just simply do it.\n" }); + expect( + await findOversizedFiles(cwd, [], VALE_MAX_FILE_BYTES, ["**/README.md"]) + ).toEqual([]); + }); + + it("falls back to the exhaustive walk when no sections are given", async () => { + // The path a caller with no assembled config takes — `verifyValeRule`'s + // isolating config, or a test that hands `runVale` a hand-written + // `.vale.ini` directly. Unaffected by the scoping above: every file + // under the target root is still a candidate, sections or not. + const cwd = makeScratchProject({ "pnpm-lock.yaml": oversizedBody }); + expect(await findOversizedFiles(cwd, [], VALE_MAX_FILE_BYTES)).toEqual([ + { file: "pnpm-lock.yaml", size: oversizedBody.length }, + ]); + }); +}); + withVale( "runVale against the real binary, with converter-dependent files", () => { diff --git a/packages/cli/test/vale-orchestration.test.ts b/packages/cli/test/vale-orchestration.test.ts index fd02ef56..a84a379e 100644 --- a/packages/cli/test/vale-orchestration.test.ts +++ b/packages/cli/test/vale-orchestration.test.ts @@ -225,12 +225,12 @@ withVale("runEngines over a mixed corpus", () => { const cwd = makeMixedProject(); // Vale reads the assembled run config, so a dispatch that never assembles // has no config to point at — which would report as "no Vale findings". - const valeConfigPath = await assembleValeConfig(cwd); + const assembledVale = await assembleValeConfig(cwd); const dispatched = await runEngines({ cwd, paths: ["app.js", "doc.md"], astGrepConfigPath: await assembleSgConfig(cwd), - valeConfigPath, + valeConfigPath: assembledVale?.path, runtimeRules: [], }); @@ -244,11 +244,12 @@ withVale("runEngines over a mixed corpus", () => { it("does not invoke Vale when it has no rules", async () => { const cwd = makeMixedProject({ valeRules: false }); + const assembledVale = await assembleValeConfig(cwd); const dispatched = await runEngines({ cwd, paths: ["app.js", "doc.md"], astGrepConfigPath: await assembleSgConfig(cwd), - valeConfigPath: await assembleValeConfig(cwd), + valeConfigPath: assembledVale?.path, runtimeRules: [], }); expect(dispatched.results.every((result) => result.source !== "vale")).toBe( @@ -270,8 +271,8 @@ describe("runEngines when a Vale rule directory assembles to nothing", () => { // A rule *directory* is present, so the old gate would have said "run". expect(await hasValeRules(cwd)).toBe(true); - const valeConfigPath = await assembleValeConfig(cwd); - expect(valeConfigPath).toBeUndefined(); + const assembledVale = await assembleValeConfig(cwd); + expect(assembledVale).toBeUndefined(); // Spied rather than inferred from the absence of findings: an unconfigured // Vale reports nothing either way, so "no vale results" cannot tell a skip @@ -283,7 +284,7 @@ describe("runEngines when a Vale rule directory assembles to nothing", () => { cwd, paths: ["app.js", "doc.md"], astGrepConfigPath: await assembleSgConfig(cwd), - valeConfigPath, + valeConfigPath: assembledVale?.path, runtimeRules: [], }); @@ -309,11 +310,12 @@ describe("runEngines when Vale is unavailable", () => { }); const cwd = makeMixedProject(); + const assembledVale = await assembleValeConfig(cwd); const dispatched = await runEngines({ cwd, paths: ["app.js", "doc.md"], astGrepConfigPath: await assembleSgConfig(cwd), - valeConfigPath: await assembleValeConfig(cwd), + valeConfigPath: assembledVale?.path, runtimeRules: [], }); @@ -362,11 +364,12 @@ describe("runEngines when Vale is unavailable", () => { }); const cwd = makeMixedProject(); + const assembledVale = await assembleValeConfig(cwd); const dispatched = await runEngines({ cwd, paths: ["doc.md"], astGrepConfigPath: await assembleSgConfig(cwd), - valeConfigPath: await assembleValeConfig(cwd), + valeConfigPath: assembledVale?.path, runtimeRules: [], }); @@ -389,7 +392,7 @@ describe("runEngines when Vale is unavailable", () => { const cwd = makeMixedProject(); // Assembled while the directory is still readable: the failure under // test is discovery's, not assembly's. - const valeConfigPath = await assembleValeConfig(cwd); + const assembledVale = await assembleValeConfig(cwd); const rules = join(cwd, ".taskless", "rules", "vale"); chmodSync(rules, 0o000); try { @@ -397,7 +400,7 @@ describe("runEngines when Vale is unavailable", () => { cwd, paths: ["app.js", "doc.md"], astGrepConfigPath: await assembleSgConfig(cwd), - valeConfigPath, + valeConfigPath: assembledVale?.path, runtimeRules: [], }); diff --git a/packages/cli/test/vale-run.test.ts b/packages/cli/test/vale-run.test.ts index 02457abc..99f18bab 100644 --- a/packages/cli/test/vale-run.test.ts +++ b/packages/cli/test/vale-run.test.ts @@ -530,6 +530,39 @@ withVale("runVale against the real binary", () => { }) ); }); + + it("names only the oversized files a section pattern actually reaches (taskless/cli#321 follow-up)", async () => { + // The false-positive this addresses: an un-scoped scan named + // `pnpm-lock.yaml` and `packages/cli/CHANGELOG.md` on this very + // repository, neither of which any rule's `.vale.ini` section touches. + // Reproduced here with a rule scoped only to `*.md` and an oversized + // `.yaml` file alongside an oversized, in-scope `.md` file. + const cwd = makeProject( + `${header}\n[*.md]\nno-simply.no-simply = YES\n`, + { "no-simply": existenceRule("simply", "Avoid 'simply'") }, + { + "huge.md": oversizedBody, + "huge.yaml": oversizedBody, + } + ); + + const outcome = await runVale({ + cwd, + paths: ["huge.md", "huge.yaml"], + sectionGlobs: ["*.md"], + }); + + // MUTATION CHECK: pass `sectionGlobs: undefined` instead (or drop the + // option from this call) and the assertions below fail: `outcome.notice` + // then also names `huge.yaml`, and `results` gains `huge.yaml`'s + // thousands of `no-simply` matches instead of staying empty. Verified + // locally. + expect(outcome.status).toBe("ok"); + if (outcome.status !== "ok") return; + expect(outcome.results).toEqual([]); + expect(outcome.notice).toContain("huge.md"); + expect(outcome.notice).not.toContain("huge.yaml"); + }); }); }); From 3c24fee4ca6d7ef0fdc80b281cea82dd0d95a6f4 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 8 Sep 2026 22:39:56 -0700 Subject: [PATCH 3/4] fix(cli): thread wholeProject through the oversized-file scan, escape literal exclusion paths check . silently disabled the whole oversized-file guard: filterExistingPaths normalizes a bare . into paths = ["."], length 1, so findOversizedFiles's own paths.length === 0 test misread it as an explicit target and every glob match failed the root-membership check. wholeProject is now a required parameter, threaded from runVale's own isWholeProjectWalk result instead of being recomputed (and re-broken) inside findOversizedFiles. A discovered file's own name is spliced into buildValeGlob's !{...} alternation as a literal pattern, and a comma, brace, or bracket in it meant something other than itself: --glob=!{big,comma.md} verified against the real binary splits into two patterns, neither of which excludes the real file. escapeGlobLiteral now escapes those characters before an oversized file's name or a #300 retry-loop candidate reaches the glob, reusing GLOB_METACHARACTERS (now exported) from git-ignored.ts's sibling case rather than a second definition -- that case drops a dangerous entry instead, which is the wrong call here since dropping the file the guard is protecting defeats it. VALE_MAX_FILE_BYTES is threaded through runVale as an optional maxFileBytes, not exposed as a CLI flag or config surface, so the timeout test can restore its original 320KB fixture and ~3200ms headroom over its 100ms budget instead of being capped at 128KB by the guard it also exercises. Also: verified the seven section-pattern shapes this repository's own rules use match identically between Vale and node's glob() on ordinary paths, and documented the one confirmed gap (glob() does not descend into dot-directories, Vale's own walker does) as a known, pre-existing limitation rather than fixing it in this pass. --- packages/cli/src/rules/git-ignored.ts | 19 ++- packages/cli/src/rules/vale/formats.ts | 191 ++++++++++++++++++------- packages/cli/src/rules/vale/run.ts | 40 +++++- packages/cli/test/vale-formats.test.ts | 94 +++++++++++- packages/cli/test/vale-run.test.ts | 84 +++++++++-- 5 files changed, 348 insertions(+), 80 deletions(-) diff --git a/packages/cli/src/rules/git-ignored.ts b/packages/cli/src/rules/git-ignored.ts index ea05b32d..2909e5ab 100644 --- a/packages/cli/src/rules/git-ignored.ts +++ b/packages/cli/src/rules/git-ignored.ts @@ -117,13 +117,20 @@ const ROOT_ENTRIES = new Set(["./", "."]); * a literal path into a matcher. * * An entry carrying any of them is left out of the exclusion rather than - * escaped. Vale's glob dialect is not ours to guess at, and the cost of leaving - * it out is that one pathologically-named ignored path is still linted — which - * is exactly the behavior that shipped before this module, so it is a gap - * rather than a regression. {@link isGitIgnoredPath} does not share the - * restriction, so such a path is still kept out of the skip notice. + * escaped **here**. Exported so `escapeGlobLiteral` in `vale/formats.ts` can + * share this exact character class rather than guessing its own — that + * function makes the opposite call (escape, not drop) for the oversized-file + * exclusion, where dropping would mean the pathological file that triggered + * the guard is the one file left unprotected. See its docblock for why the + * two literal-path exclusions in this codebase disagree on purpose. + * + * The cost of dropping here is that one pathologically-named ignored path is + * still linted — which is exactly the behavior that shipped before this + * module, so it is a gap rather than a regression. {@link isGitIgnoredPath} + * does not share the restriction, so such a path is still kept out of the + * skip notice. */ -const GLOB_METACHARACTERS = /[*?[\]{},\\!]/; +export const GLOB_METACHARACTERS = /[*?[\]{},\\!]/; /** * The ignored entries, rendered as patterns for Vale's `--glob`. diff --git a/packages/cli/src/rules/vale/formats.ts b/packages/cli/src/rules/vale/formats.ts index eebf0853..f44936e1 100644 --- a/packages/cli/src/rules/vale/formats.ts +++ b/packages/cli/src/rules/vale/formats.ts @@ -5,6 +5,7 @@ import { VALE_CONVERTER_BY_EXTENSION, VALE_CONVERTER_DEPENDENT_EXTENSIONS, } from "../capabilities"; +import { GLOB_METACHARACTERS } from "../git-ignored"; /** * Taskless's own directory, as a project-relative path. @@ -150,15 +151,74 @@ export function converterExclusionGlobs(): string[] { * One expression because Vale accepts one `--glob` and the last one wins: * passing two flags silently drops the first, so the exclusions have to be one * negated alternation or they are not exclusions at all. + * + * **Every entry here must already be safe to splice into `!{…}` verbatim.** + * This function does not escape or validate — every caller is responsible for + * that before the pattern reaches here, because a real glob (`.taskless/**`, + * `**\/*.adoc`) and a literal discovered path (an oversized file's own name) + * need opposite treatment: a glob's metacharacters are meant, a literal path's + * are not. See {@link escapeGlobLiteral} for the literal-path side, and + * `gitIgnoredExclusionGlobs` in `git-ignored.ts` for the sibling case that + * drops a dangerous entry instead of escaping it. */ export function buildValeGlob(patterns: string[]): string | undefined { if (patterns.length === 0) return undefined; return `--glob=!{${patterns.join(",")}}`; } +/** + * Escape a literal path so it means only itself once spliced into + * {@link buildValeGlob}'s `!{…}` alternation. + * + * `gitIgnoredExclusionGlobs` (`git-ignored.ts`) faces the identical problem — + * a discovered path with a comma or a glob metacharacter meaning something + * other than itself in the alternation — and answers it by dropping the entry + * instead of escaping it. This function makes the opposite call, and the + * difference is not a style preference: dropping a git-ignored entry only + * costs the exclusion of a path Vale would otherwise walk past anyway (noisy + * findings inside a vendored tree, nothing more), while dropping an oversized + * file from ITS exclusion means the pathologically large file that triggered + * the guard is the one file left unprotected — undoing the entire point of + * `findOversizedFiles`. A false-positive skip is the wrong failure mode for + * the same reason a false-positive notice was in the sibling case: the risk + * this guard exists to prevent is concentrated in exactly the files this + * would refuse to escape. + * + * Verified against the real binary, not assumed: `--glob=!{big\,comma.md}` + * excludes a file literally named `big,comma.md`, while the unescaped form + * (`!{big,comma.md}`) does not — it splits into two patterns, `big` and + * `comma.md`, neither of which matches the real file. A backslash is Vale's + * own escape character in this position, the same dialect + * `GLOB_METACHARACTERS` was already written against. + */ +export function escapeGlobLiteral(path: string): string { + return path.replaceAll(new RegExp(GLOB_METACHARACTERS, "g"), String.raw`\$&`); +} + /** How many skipped paths a notice names before it summarizes the rest. */ const NOTICE_SAMPLE_LIMIT = 5; +/** + * Render a bounded, comma-joined list for a notice: every label up to + * {@link NOTICE_SAMPLE_LIMIT}, then `(and N more)` for the rest. + * + * Shared by {@link skippedFilesNotice} and {@link oversizedFilesNotice}, + * which otherwise had the identical four lines twice — same limit, same + * truncation shape, same reason (a notice naming hundreds of files is not + * more readable than one naming five and a count). Unlike the two + * declined-to-merge cases elsewhere in this module, this is genuinely one + * piece of formatting knowledge, so a caller mapping its own items to labels + * first (`oversizedFilesNotice` maps `OversizedFile` to `.file`) is the only + * difference between the two call sites. + */ +function summarizeList(labels: string[]): string { + const sample = labels.slice(0, NOTICE_SAMPLE_LIMIT); + const remainder = labels.length - sample.length; + return remainder > 0 + ? `${sample.join(", ")} (and ${String(remainder)} more)` + : sample.join(", "); +} + /** Directories never worth walking to build a notice. */ const UNWALKED_DIRECTORIES = new Set([ "node_modules", @@ -314,14 +374,38 @@ export interface OversizedFile { * unreadable subtree, a platform where `glob` rejects the pattern — none of * them can be allowed to suppress the exclusion that already ran. The failure * mode here is "no notice, never no fix". + * + * **Known dialect gap, not introduced here: Node's `glob` does not descend + * into dot-directories, Vale's own walker does.** Measured against this + * repository with a rule forced to match `**\/README.md` everywhere: the real + * binary visits 22 files, including `.taskless/rules/vale/*\/.tests/*\/README.md` + * and other paths under a leading dot; this module's `glob()` call finds only + * the 10 that sit outside every dot-directory. For {@link + * findConverterDependentFiles} that gap is one-directional and safe — it + * costs the *notice* accuracy, never the exclusion, because that exclusion + * rides on a static extension pattern handed to Vale's own `--glob`, which + * traverses dot-directories fine. Here it is not fully safe: the discovered + * path IS the exclusion, so an oversized file living inside a dot-directory + * this scan cannot see is not excluded, and Vale may still spend its + * quadratic cost linting it if some section reaches that directory. This + * repository has no live exposure — the one dot-directory any section here + * names, `.taskless/`, is separately and unconditionally excluded before + * Vale ever runs — but a project with section-matched content under another + * dot-directory (`.github/`, a dotfile-heavy docs tree) would not be + * protected by this scan for a file that lives there. Left as a documented + * gap rather than fixed here: closing it means replacing `glob()` with a + * custom walker that treats dot-directories differently from + * `UNWALKED_DIRECTORIES`, which is a larger change than this pass, and + * `VALE_TIMEOUT_MS` remains the backstop if it is ever hit. */ export async function findOversizedFiles( cwd: string, paths: string[], maxBytes: number, + wholeProject: boolean, sectionGlobs?: string[] ): Promise { - const roots = paths.length === 0 ? ["."] : paths; + const roots = wholeProject ? ["."] : paths; const found = new Map(); const checkCandidate = async (relative: string): Promise => { @@ -358,46 +442,66 @@ export async function findOversizedFiles( // the time this runs. } } - return [...found.values()].toSorted((a, b) => a.file.localeCompare(b.file)); - } - - // Section patterns are root-relative, exactly as Vale reads them — never - // prefixed per target root, the way the extension-based fallback above is. - // A whole-project run (`roots === ["."]`) needs no further narrowing: every - // match is already in scope. An explicit target (`check src/` or - // `check src/doc.md`) narrows the matches down to that subtree afterward - // instead, because a section like `CLAUDE.md` or `**/README.md` has no - // meaningful "under src/" form to prefix onto — Vale itself evaluates every - // section against the whole project and only its own target list decides - // what it actually visits, so intersecting after the glob mirrors that - // rather than guessing at one. This is also what makes a named file's - // in-scope test free: it needs no separate membership check, because a - // pattern like `**/README.md` already matches a top-level `README.md` - // found this way, whether or not the caller named it explicitly. - const wholeProject = paths.length === 0; - for (const pattern of sectionGlobs) { - try { - for await (const match of glob(pattern, { - cwd, - exclude: isUnwalkedEntry, - })) { - const relative = String(match); - if ( - !wholeProject && - !roots.some( - (root) => relative === root || relative.startsWith(`${root}/`) - ) - ) { - continue; + } else { + // Section patterns are root-relative, exactly as Vale reads them — never + // prefixed per target root, the way the extension-based fallback above + // is. A whole-project run needs no further narrowing: every match is + // already in scope. An explicit target (`check src/` or `check + // src/doc.md`) narrows the matches down to that subtree afterward + // instead, because a section like `CLAUDE.md` or `**/README.md` has no + // meaningful "under src/" form to prefix onto — Vale itself evaluates + // every section against the whole project and only its own target list + // decides what it actually visits, so intersecting after the glob + // mirrors that rather than guessing at one. This is also what makes a + // named file's in-scope test free: it needs no separate membership + // check, because a pattern like `**/README.md` already matches a + // top-level `README.md` found this way, whether or not the caller named + // it explicitly. + // + // `wholeProject` is a PARAMETER, not `paths.length === 0` computed here — + // that test is wrong for `check .`. `filterExistingPaths` (`commands/ + // check.ts`) normalizes a bare `.` into `paths = ["."]`, length 1, so a + // length test reads it as an explicit target, `roots` becomes `["."]`, + // and every match (`README.md`) fails `relative === "." || + // relative.startsWith("./")` — every candidate silently dropped, and the + // whole guard goes dark on a near-default invocation. `isWholeProjectWalk` + // (`walk-scope.ts`) exists precisely for this and is what callers must + // resolve `paths` through before reaching here; `runVale` already + // computes it for its own `targets`/`.taskless/**` exclusion and passes + // the same value in, rather than this function recomputing a second, + // broken answer. + for (const pattern of sectionGlobs) { + try { + for await (const match of glob(pattern, { + cwd, + exclude: isUnwalkedEntry, + })) { + const relative = String(match); + if ( + !wholeProject && + !roots.some( + (root) => relative === root || relative.startsWith(`${root}/`) + ) + ) { + continue; + } + await checkCandidate(relative); } - await checkCandidate(relative); + } catch { + // Same reasoning as the fallback walk: a malformed pattern or an + // unreadable subtree means "no notice, never no fix". } - } catch { - // Same reasoning as the fallback walk: a malformed pattern or an - // unreadable subtree means "no notice, never no fix". } } + // One sorted return for both branches — declined to unify further with + // `findConverterDependentFiles`'s `[...found].toSorted()` (taskless/cli#323 + // review): that one sorts a `Set` with the default string + // comparator, this one sorts a `Map`'s values by a field via + // `localeCompare`. The resemblance is that both produce a stable, + // alphabetical order for a notice — not a shared invariant the two could + // drift apart on — so a shared helper would exist only to hide two + // different container types behind one name. return [...found.values()].toSorted((a, b) => a.file.localeCompare(b.file)); } @@ -427,12 +531,7 @@ export function skippedFilesNotice(files: string[]): string | undefined { ), ].toSorted(); - const sample = files.slice(0, NOTICE_SAMPLE_LIMIT); - const remainder = files.length - sample.length; - const listed = - remainder > 0 - ? `${sample.join(", ")} (and ${String(remainder)} more)` - : sample.join(", "); + const listed = summarizeList(files); return ( `Vale did not check ${String(files.length)} file(s): ${listed}. These ` + @@ -490,13 +589,7 @@ export function oversizedFilesNotice( ): string | undefined { if (files.length === 0) return undefined; - const sample = files.slice(0, NOTICE_SAMPLE_LIMIT); - const remainder = files.length - sample.length; - const names = sample.map((entry) => entry.file); - const listed = - remainder > 0 - ? `${names.join(", ")} (and ${String(remainder)} more)` - : names.join(", "); + const listed = summarizeList(files.map((entry) => entry.file)); return ( `Vale did not check ${String(files.length)} file(s) over ${String(maxBytes)} ` + diff --git a/packages/cli/src/rules/vale/run.ts b/packages/cli/src/rules/vale/run.ts index fe05eaed..6357ae0b 100644 --- a/packages/cli/src/rules/vale/run.ts +++ b/packages/cli/src/rules/vale/run.ts @@ -17,6 +17,7 @@ import { findValeBinary, valeUnavailableMessage } from "./binary"; import { buildValeGlob, converterExclusionGlobs, + escapeGlobLiteral, findConverterDependentFiles, findOversizedFiles, oversizedFilesNotice, @@ -540,6 +541,18 @@ export interface ValeRunOptions { * this option existed. */ sectionGlobs?: string[]; + /** + * Overrides {@link VALE_MAX_FILE_BYTES} for this run's oversized-file guard. + * + * Exists as a seam for tests, not as a project-level setting — there is no + * CLI flag or config surface for this, deliberately: a per-project size + * limit is a real, separately-discussed feature this option is NOT meant to + * ship early. Its one real use today is the timeout test in + * `vale-run.test.ts`, which needs a fixture large enough to leave real + * headroom over its budget without that fixture being excluded by the + * production 128KB limit before Vale ever sees it. + */ + maxFileBytes?: number; } /** @@ -638,13 +651,19 @@ export async function runVale( // `worktrees/` is not a file this run declined to convert, it is a file this // run was never going to look at, and naming it would send the reader to // investigate a directory the fix above deliberately excluded. + const maxFileBytes = options.maxFileBytes ?? VALE_MAX_FILE_BYTES; const [ignoredEntries, converterDependent, oversized] = await Promise.all([ wholeProject ? listGitIgnoredEntries(options.cwd) : [], findConverterDependentFiles(options.cwd, paths), + // `wholeProject`, computed above via `isWholeProjectWalk`, is passed + // through rather than recomputed from `paths.length === 0` inside + // `findOversizedFiles` — see that function's docblock for the `check .` + // failure a recomputed, length-based test produced. findOversizedFiles( options.cwd, paths, - VALE_MAX_FILE_BYTES, + maxFileBytes, + wholeProject, options.sectionGlobs ), ]); @@ -666,7 +685,13 @@ export async function runVale( ] : []), ...converterExclusionGlobs(), - ...oversizedInScope.map((entry) => entry.file), + // Escaped: unlike `.taskless/**` and the converter globs above, a + // discovered file's name is a LITERAL path, not a pattern we wrote, and a + // comma or brace in it would otherwise split or reinterpret this + // alternation (taskless/cli#323 review). See `escapeGlobLiteral`'s + // docblock in `formats.ts` for why this exclusion escapes rather than + // drops such a name, unlike `gitIgnoredExclusionGlobs`. + ...oversizedInScope.map((entry) => escapeGlobLiteral(entry.file)), ]; // Both notices describe files this run declined to check, for different @@ -679,7 +704,7 @@ export async function runVale( (file) => !isGitIgnoredPath(file, ignoredEntries) ) ), - oversizedFilesNotice(oversizedInScope, VALE_MAX_FILE_BYTES), + oversizedFilesNotice(oversizedInScope, maxFileBytes), ].filter((notice) => notice !== undefined); const skipped = notices.length === 0 ? undefined : notices.join("\n"); @@ -704,11 +729,18 @@ export async function runVale( // there are finitely many files to exclude. Re-reporting the same path // twice in a row is the only way this could spin, and that path is refused // rather than retried (see the `excludedTargets.has` check below). + // Kept as raw, unescaped paths — `excludedTargets.has(candidate)` below + // compares against `targetFileParseError`'s own raw output, so escaping on + // the way in would make every second retry look like a new candidate. + // Escaped only where it actually reaches a glob, in `buildValeGlob` below. const excludedTargets = new Set(); const excludedFindings: CheckResult[] = []; for (;;) { - const globArgument = buildValeGlob([...exclude, ...excludedTargets]); + const globArgument = buildValeGlob([ + ...exclude, + ...[...excludedTargets].map((target) => escapeGlobLiteral(target)), + ]); const globFlags = globArgument === undefined ? [] : [globArgument]; // `--` separates flags from positional paths, so a path beginning with diff --git a/packages/cli/test/vale-formats.test.ts b/packages/cli/test/vale-formats.test.ts index f3765d06..3f2693ea 100644 --- a/packages/cli/test/vale-formats.test.ts +++ b/packages/cli/test/vale-formats.test.ts @@ -12,6 +12,7 @@ import { CONVERTER_DEPENDENT_EXTENSIONS, converterExclusionGlobs, converterFor, + escapeGlobLiteral, findConverterDependentFiles, findOversizedFiles, skippedFilesNotice, @@ -164,6 +165,55 @@ describe("the exclusion glob", () => { }); }); +describe("escaping a literal path for buildValeGlob's alternation (taskless/cli#323 review)", () => { + it("escapes every character the alternation would otherwise reinterpret", () => { + // Mirrors GLOB_METACHARACTERS in git-ignored.ts exactly: the same nine + // characters, escaped here instead of dropped, because dropping an + // oversized file from ITS OWN exclusion defeats the guard for exactly the + // pathological file it exists to protect. + expect(escapeGlobLiteral("big,comma.md")).toBe(String.raw`big\,comma.md`); + expect(escapeGlobLiteral("a{b}c.md")).toBe(String.raw`a\{b\}c.md`); + expect(escapeGlobLiteral("weird[1].md")).toBe(String.raw`weird\[1\].md`); + expect(escapeGlobLiteral("plain.md")).toBe("plain.md"); + }); + + withVale("against the real binary", () => { + it("actually excludes a file whose name contains a comma", () => { + // Confirmed by hand while investigating this review: unescaped, + // `--glob=!{big,comma.md}` splits into two patterns — "big" and + // "comma.md" — neither of which matches the real file, so it is + // NOT excluded. This is the guard against that regressing. + const cwd = makeProject({ + "a.md": "Just simply do it.\n", + "big,comma.md": "Just simply do it.\n", + }); + + const excluded = buildValeGlob([escapeGlobLiteral("big,comma.md")]); + const result = spawnSync( + binary as string, + [ + "--config", + join(".taskless", ".vale.ini"), + "--output=JSON", + "--no-exit", + excluded as string, + "--", + ".", + ], + { cwd, encoding: "utf8" } + ); + + // MUTATION CHECK: pass `buildValeGlob(["big,comma.md"])` (unescaped) + // instead and this fails — Vale's own JSON output then contains + // "big,comma.md", because the comma split the alternation and + // neither half matched the real file. Verified locally. + expect(result.status).toBe(0); + expect(result.stdout).toContain("a.md"); + expect(result.stdout).not.toContain("big,comma.md"); + }); + }); +}); + describe("the skipped-files notice", () => { it("is absent when nothing was skipped", () => { expect(skippedFilesNotice([])).toBeUndefined(); @@ -261,7 +311,9 @@ describe("finding oversized files, scoped to what Vale would actually lint", () it("reports an oversized file matching a section pattern", async () => { const cwd = makeScratchProject({ "README.md": oversizedBody }); expect( - await findOversizedFiles(cwd, [], VALE_MAX_FILE_BYTES, ["**/README.md"]) + await findOversizedFiles(cwd, [], VALE_MAX_FILE_BYTES, true, [ + "**/README.md", + ]) ).toEqual([{ file: "README.md", size: oversizedBody.length }]); }); @@ -284,26 +336,56 @@ describe("finding oversized files, scoped to what Vale would actually lint", () // appearing alongside `README.md`. Verified locally: reverting restores // the single-entry result below. expect( - await findOversizedFiles(cwd, [], VALE_MAX_FILE_BYTES, ["**/README.md"]) + await findOversizedFiles(cwd, [], VALE_MAX_FILE_BYTES, true, [ + "**/README.md", + ]) ).toEqual([{ file: "README.md", size: oversizedBody.length }]); }); it("still checks a matching file that is not oversized", async () => { const cwd = makeScratchProject({ "README.md": "Just simply do it.\n" }); expect( - await findOversizedFiles(cwd, [], VALE_MAX_FILE_BYTES, ["**/README.md"]) + await findOversizedFiles(cwd, [], VALE_MAX_FILE_BYTES, true, [ + "**/README.md", + ]) ).toEqual([]); }); + it("still reports an oversized file when the caller passes paths: ['.'] (taskless/cli#323 review)", async () => { + // `check .` — a near-default invocation — reaches `runVale` with + // `paths = ["."]`, not `[]`: `filterExistingPaths` (`commands/check.ts`) + // normalizes a bare `.` into that literal string rather than dropping + // back to an empty array. Every OTHER test in this describe block uses + // `paths: []`, which is why a `paths.length === 0` test for "whole + // project" silently passed them all while being wrong for this one. + // + // `wholeProject` is the 4th argument precisely so the caller — `runVale`, + // via `isWholeProjectWalk` — decides this, rather than this function + // re-deriving a broken answer from `paths` on its own. + const cwd = makeScratchProject({ "README.md": oversizedBody }); + + // MUTATION CHECK: change the call below to pass `paths.length === 0` + // (i.e. `false`, since `paths` here is `["."]`) instead of the literal + // `true`, simulating the recomputed-internally bug this test exists to + // catch, and the assertion fails — `README.md` is no longer reported, + // because every glob match (`"README.md"`) fails `relative === "." || + // relative.startsWith("./")`. Verified locally; reverting restores green. + expect( + await findOversizedFiles(cwd, ["."], VALE_MAX_FILE_BYTES, true, [ + "**/README.md", + ]) + ).toEqual([{ file: "README.md", size: oversizedBody.length }]); + }); + it("falls back to the exhaustive walk when no sections are given", async () => { // The path a caller with no assembled config takes — `verifyValeRule`'s // isolating config, or a test that hands `runVale` a hand-written // `.vale.ini` directly. Unaffected by the scoping above: every file // under the target root is still a candidate, sections or not. const cwd = makeScratchProject({ "pnpm-lock.yaml": oversizedBody }); - expect(await findOversizedFiles(cwd, [], VALE_MAX_FILE_BYTES)).toEqual([ - { file: "pnpm-lock.yaml", size: oversizedBody.length }, - ]); + expect( + await findOversizedFiles(cwd, [], VALE_MAX_FILE_BYTES, true) + ).toEqual([{ file: "pnpm-lock.yaml", size: oversizedBody.length }]); }); }); diff --git a/packages/cli/test/vale-run.test.ts b/packages/cli/test/vale-run.test.ts index 99f18bab..39a28031 100644 --- a/packages/cli/test/vale-run.test.ts +++ b/packages/cli/test/vale-run.test.ts @@ -563,6 +563,39 @@ withVale("runVale against the real binary", () => { expect(outcome.notice).toContain("huge.md"); expect(outcome.notice).not.toContain("huge.yaml"); }); + + it("still reports the guard on `check .`, not only on a bare `check` (taskless/cli#323 review)", async () => { + // `check .` is a near-default invocation, and it does NOT reach here + // the way a bare `check` does: `filterExistingPaths` + // (`commands/check.ts`) normalizes a bare `.` positional into the + // literal `paths = ["."]`, never back to `[]`. Every other test in this + // file uses `paths: []` for its whole-project cases, which is exactly + // why this was invisible until someone actually ran `check . --json` + // against a real project and compared it to a bare `check --json`. + const cwd = makeProject( + `${header}\n[**/README.md]\nno-simply.no-simply = YES\n`, + { "no-simply": existenceRule("simply", "Avoid 'simply'") }, + { "README.md": oversizedBody } + ); + + // MUTATION CHECK: this is an end-to-end restatement of the + // `findOversizedFiles` unit test above it in `vale-formats.test.ts` + // ("still reports an oversized file when the caller passes paths: + // ['.']"). Reintroducing `paths.length === 0` inside that function (in + // place of the `wholeProject` parameter `runVale` threads through) + // fails this test too: `outcome.notice` comes back `undefined` because + // every glob match fails the root-membership check. Verified locally. + const outcome = await runVale({ + cwd, + paths: ["."], + sectionGlobs: ["**/README.md"], + }); + + expect(outcome.status).toBe("ok"); + if (outcome.status !== "ok") return; + expect(outcome.results).toEqual([]); + expect(outcome.notice).toContain("README.md"); + }); }); }); @@ -602,28 +635,49 @@ withVale("ValeRunOutcome.blocking against the real binary", () => { // "timeout". // // The race is removed by making the work outlast the budget by a margin - // nothing plausible closes. Vale is QUADRATIC in the size of a single - // file, so a document well under a second's worth of Vale time is still - // many multiples of a 100ms budget. + // nothing plausible closes — and the metric that matters is the ABSOLUTE + // margin (duration minus budget), not a ratio, because what has to happen + // is the child process finishing before a delayed timer callback runs. + // A ratio looks worse as the budget shrinks even when the real margin is + // enormous, which is exactly what a review round measured wrong here + // (taskless/cli#323): a "35x to 4.5x" ratio comparison on a version of + // this test that had shrunk its fixture to fit under `VALE_MAX_FILE_BYTES` + // (taskless/cli#321) read as a regression, but the ratio was the wrong + // number: + // + // | version | duration | budget | headroom | + // | -------------------------------- | -------- | ------ | -------- | + // | original, which actually flaked | 46ms | 1ms | 45ms | + // | the 320KB fixture in e1ed936 | 3300ms | 100ms | 3200ms | + // | the 128KB-capped version (#323) | ~530ms | 100ms | 430ms | // - // The fixture has to stay UNDER `VALE_MAX_FILE_BYTES` (taskless/cli#321): - // a document at or above that limit is excluded before Vale ever sees it, - // which would report `status: "ok"` with a notice instead of exercising - // the timeout this test is actually about. 6,300 repeats of a 19-byte - // sentence lands at ~117KB (119,700 bytes), comfortably below the 128KB - // limit — measured at ~450ms against the real binary, a ~4.5x margin over - // the 100ms budget used here. That margin is smaller than this test used - // before #321 shrank how large a fixture it may use, but it is measured, - // not assumed, and the run is killed at 100ms either way, so the test - // costs about that rather than 450ms. + // The 128KB-capped version was still ~10x the margin that actually + // flaked — not a regression toward the failure mode — but it was a real + // ~7x reduction from what e1ed936 shipped, worth restoring rather than + // accepting. + // + // `VALE_MAX_FILE_BYTES` capped how large a fixture this test could use + // once it started sharing `runVale`'s production size guard (taskless/ + // cli#321): a document at or above that limit is excluded before Vale + // ever sees it, reporting `status: "ok"` with a notice instead of + // exercising the timeout this test is about. `maxFileBytes` (added for + // exactly this) raises the guard's limit for THIS CALL ONLY — it is not a + // CLI flag or a config surface, just a seam for a test that needs its + // fixture back — so the original 320KB fixture and its ~3200ms headroom + // are restored without touching the production default. const cwd = makeProject( `${header}\n[*.md]\nno-simply.no-simply = YES\n`, { "no-simply": existenceRule("simply", "Avoid 'simply'") }, - { "doc.md": "Just simply do it. ".repeat(6_300) } + { "doc.md": `${"Just simply do it. ".repeat(17_000)}\n` } ); expect( - await runVale({ cwd, paths: ["doc.md"], timeoutMs: 100 }) + await runVale({ + cwd, + paths: ["doc.md"], + timeoutMs: 100, + maxFileBytes: Number.POSITIVE_INFINITY, + }) ).toMatchObject({ status: "timeout", blocking: true }); }); From 2eb81ec95874e3ea54a8a1be31cb13f4fe617116 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 8 Sep 2026 22:45:52 -0700 Subject: [PATCH 4/4] test(cli): pin that a bare [section] pattern does not recurse like --glob Review of #323 raised whether findOversizedFiles's use of node's glob() against Vale's own [section] header strings shares the CLI --glob flag's documented basename-at-any-depth semantics for a slash-free pattern -- if it did, node's non-recursive glob("CLAUDE.md") would miss a nested, section-matched oversized file entirely. Measured against the real binary: it does not. A rule scoped to [CLAUDE.md] matched only the project-root file, not a sub/CLAUDE.md fixture at a different depth -- the same anchoring node's glob() already applies. Pinned in vale-vendor-contract.test.ts so a future Vale version that unifies the two matching paths fails loudly here instead of silently reopening the timeout risk taskless/cli#321 closed. --- packages/cli/src/rules/vale/formats.ts | 13 +++++ .../cli/test/vale-vendor-contract.test.ts | 50 +++++++++++++++++++ 2 files changed, 63 insertions(+) diff --git a/packages/cli/src/rules/vale/formats.ts b/packages/cli/src/rules/vale/formats.ts index f44936e1..dd4f338f 100644 --- a/packages/cli/src/rules/vale/formats.ts +++ b/packages/cli/src/rules/vale/formats.ts @@ -397,6 +397,19 @@ export interface OversizedFile { * custom walker that treats dot-directories differently from * `UNWALKED_DIRECTORIES`, which is a larger change than this pass, and * `VALE_TIMEOUT_MS` remains the backstop if it is ever hit. + * + * **Checked and confirmed SAFE: a bare `[section]` pattern does not share + * `--glob`'s basename-at-any-depth recursion.** `converterExclusionGlobs`'s + * docblock establishes that Vale's `--glob` CLI flag matches a slash-free + * pattern against a file's basename at any depth — raising the question of + * whether a section header like `[CLAUDE.md]` does the same, which would make + * node's non-recursive `glob("CLAUDE.md")` miss a nested, section-matched, + * oversized file entirely. Measured against the real binary (pinned in + * `vale-vendor-contract.test.ts`, "`[section]` header matching vs. the + * `--glob` CLI flag"): it does not. `[CLAUDE.md]` scoped a rule to the + * project-root file only; a `sub/CLAUDE.md` fixture at a different depth was + * not linted. Section matching and node's `glob()` agree on this shape of + * pattern, so this concern resolved to "confirmed fine," not "fixed." */ export async function findOversizedFiles( cwd: string, diff --git a/packages/cli/test/vale-vendor-contract.test.ts b/packages/cli/test/vale-vendor-contract.test.ts index 21b829bd..372d1eec 100644 --- a/packages/cli/test/vale-vendor-contract.test.ts +++ b/packages/cli/test/vale-vendor-contract.test.ts @@ -955,3 +955,53 @@ withVale("check types", () => { expect(enumerated).toContain("metric"); }); }); + +/** + * Whether a `[section]` header's OWN matching shares the `--glob` CLI flag's + * basename-at-any-depth behavior for a slash-free pattern. + * + * Raised in review of taskless/cli#323: `findOversizedFiles` (`vale/ + * formats.ts`) globs `AssembledValeConfig.sections` — the literal `[...]` + * header strings a rule's `.vale.ini` declares, e.g. `[CLAUDE.md]` — through + * node's `fs.promises.glob`. `converterExclusionGlobs`'s docblock, pinned + * elsewhere in this file, establishes that Vale's `--glob` CLI flag matches a + * slash-free pattern against a file's basename AT ANY DEPTH. If `[section]` + * matching shared that behavior, a bare pattern like `[CLAUDE.md]` would scope + * a rule to every `CLAUDE.md` in the tree, while node's `glob("CLAUDE.md")` + * matches only the one at the project root — a real dialect mismatch that + * would let an oversized, section-matched, deeply nested file escape this + * scan silently. + * + * It does not share that behavior — measured here. `[section]` matching, for + * a slash-free pattern, is anchored at the project root, exactly like node's + * `glob()` already treats it. `findOversizedFiles`'s use of node's `glob` + * against these section strings is therefore not a dialect mismatch for THIS + * shape of pattern; it agrees with Vale by coincidence of a fact this test now + * pins rather than by design. + * + * If Vale ever changes this — unifying `[section]` matching with `--glob`'s + * basename-recursive semantics — this test fails, and `findOversizedFiles`'s + * section-globbing needs the same depth-matching adjustment `converterFor`'s + * callers already carry for the CLI flag. + */ +withVale("[section] header matching vs. the --glob CLI flag", () => { + it("does NOT match a slash-free pattern's basename at every depth, unlike --glob", () => { + const cwd = project( + `${header}\n[CLAUDE.md]\nrules.no-simply = YES\n`, + { "no-simply": existence("simply") }, + { "CLAUDE.md": "Just simply do it.\n" } + ); + // `project`'s `documents` writer does not create parent directories, so + // the nested fixture is added afterward. + mkdirSync(join(cwd, "sub"), { recursive: true }); + writeFileSync(join(cwd, "sub", "CLAUDE.md"), "Just simply do it.\n"); + const result = runRaw(cwd, ["."], ["--no-exit"]); + const stdout = JSON.parse(result.stdout) as Record; + // The root file is in scope... + expect(Object.keys(stdout)).toContain("CLAUDE.md"); + // ...but the nested one, at a different depth, is not — confirming + // `[section]` matching does not recurse a bare pattern the way `--glob` + // does. If this ever changes, `sub/CLAUDE.md` starts appearing here. + expect(Object.keys(stdout)).not.toContain("sub/CLAUDE.md"); + }); +});