From 3b3666f6dc6ab443c383c0e219be78174c985200 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Sun, 20 Sep 2026 14:34:34 -0700 Subject: [PATCH] fix(vale): drop the 128 KB file-size guard VALE_MAX_FILE_BYTES was written against Vale 3.20.0, whose lint time was superlinear in the size of one Markdown block. The CLI vendors 3.21.0, whose perf commits made that cost linear: the 3 MB single-block reproduction from taskless/cli#325 measures ~230 ms on the vendored binary where 3.20.0 took ~81 s, and 128 KB of one block ~26 ms. The cap no longer separates a cheap file from an expensive one, so the preemptive exclusion, its notice, the per-run override seam, and the section-glob threading that scoped its scan are removed with their tests. The per-file retry for an unparseable front matter is unchanged. The update ledger gains a 0.11.3 line, since a file 0.11.2 reported as skipped now produces findings, and both touched agent topics bump. --- .changeset/vale-size-guard.md | 29 ++ packages/cli/src/agent/create-vale-rule.md | 30 +- packages/cli/src/agent/update.md | 16 +- packages/cli/src/commands/check.ts | 1 - packages/cli/src/rules/assemble.ts | 9 +- packages/cli/src/rules/dispatch.ts | 11 - packages/cli/src/rules/git-ignored.ts | 9 +- packages/cli/src/rules/vale/formats.ts | 319 +----------------- packages/cli/src/rules/vale/run.ts | 159 ++------- packages/cli/test/assemble.test.ts | 8 +- packages/cli/test/vale-formats.test.ts | 108 +----- packages/cli/test/vale-run.test.ts | 228 +------------ .../cli/test/vale-vendor-contract.test.ts | 23 +- 13 files changed, 135 insertions(+), 815 deletions(-) create mode 100644 .changeset/vale-size-guard.md diff --git a/.changeset/vale-size-guard.md b/.changeset/vale-size-guard.md new file mode 100644 index 00000000..b81c82cc --- /dev/null +++ b/.changeset/vale-size-guard.md @@ -0,0 +1,29 @@ +--- +"@taskless/cli": patch +--- + +`check` no longer skips Vale target files over 128 KB. The guard +(`VALE_MAX_FILE_BYTES`, added in 0.11.2) was written against Vale 3.20.0, +whose lint time grew superlinearly with the size of a single Markdown block, +so one large file could consume the run's whole timeout and, because Vale +writes nothing until the run finishes, cost every other file its findings. +The same release moved the vendored Vale to 3.21.0, whose perf work makes that +cost linear regardless of block structure, so the cap no longer separates a +cheap file from an expensive one. Re-measured on the reproduction from +taskless/cli#325 against the vendored 3.21.0 binary (darwin/arm64, warm, median +of three): + +| fixture | Vale 3.20.0 | Vale 3.21.0 | +| ------------------------------- | ----------- | ----------- | +| 3.2 MB, one block (`huge.md`) | ~81,000 ms | ~230 ms | +| 3.2 MB, blank-line separated | ~4,400 ms | ~290 ms | +| 128 KB, one block (the old cap) | ~770 ms | ~26 ms | +| 25 MB, one block | — | ~2,200 ms | + +What a user sees: a file that 0.11.2 named in a `Vale did not check N file(s) +over 131072 bytes` notice is linted again and produces findings; the notice is +gone. The per-file retry for a target whose front matter Vale cannot parse +(taskless/cli#300) is unchanged, as is the 60 s run timeout. Vale still emits +nothing until the run completes, so a run killed by an external time limit +still loses every finding; with linear cost that takes a file in the hundreds +of megabytes rather than the hundreds of kilobytes. diff --git a/packages/cli/src/agent/create-vale-rule.md b/packages/cli/src/agent/create-vale-rule.md index 3f95e1eb..78e7c3ed 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 v8) +# Topic: create-vale-rule (CLI v%(CLI_VERSION)s / topic v9) ## You are here This is `create-vale-rule`. It helps you write a Vale rule: a check over @@ -598,27 +598,13 @@ 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 **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 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. + **A large file is linted, not skipped.** Vale's cost is linear in a + file's size as of 3.21.0 (a 3MB single-block document measures + ~230ms), so no file is excluded on size and a broad matcher such as + `[*.md]` at the project root reaches a generated changelog or an + exported note like any other document. If such a file should not be + checked, narrow the section rather than expecting `check` to skip + 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/agent/update.md b/packages/cli/src/agent/update.md index 09f6dfaf..e8ba3523 100644 --- a/packages/cli/src/agent/update.md +++ b/packages/cli/src/agent/update.md @@ -1,4 +1,4 @@ -# Topic: update (CLI v%(CLI_VERSION)s / topic v6) +# Topic: update (CLI v%(CLI_VERSION)s / topic v7) ## You are here This is `update`. It tells you what an upgrade changed for the rules @@ -293,6 +293,20 @@ one trap (a leaf element on its own, `doc(h2)`, is inert; chain it). No existing rule changes; this is a reason to revisit one that was narrowed by hand. +### Migrating to 0.11.3 + +**Files over 128KB are linted again.** 0.11.2 skipped any target file +over 128KB that a matcher's section reached, naming it in a `notices` +entry instead of checking it, because Vale 3.20.0's cost grew +superlinearly with the size of one Markdown block and a single large +file could consume the whole run's time budget. 3.21.0, the Vale that +0.11.2 itself shipped, made that cost linear (a 3MB single-block file +measures ~230ms where 3.20.0 took ~81s), so the skip is gone. A file +that was reported as skipped now produces findings, and the `notices` +entry that named it no longer appears. Nothing in a rule changes; if a +large generated file was being kept quiet by that skip, narrow the +matcher's section so it is not reached. + ## Errors With `--json`, `--rules` failures emit `{ ok: false, code, message }`: diff --git a/packages/cli/src/commands/check.ts b/packages/cli/src/commands/check.ts index d3a51402..da74339f 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -259,7 +259,6 @@ export const checkCommand = defineCommand({ paths: existingPaths, astGrepConfigPath: assembled.sg, 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 6188f05c..240e8eba 100644 --- a/packages/cli/src/rules/assemble.ts +++ b/packages/cli/src/rules/assemble.ts @@ -123,9 +123,12 @@ function sectionPatternsOf(body: string): string[] { * 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. + * lint can ask this module directly instead of re-parsing the config it just + * wrote. Its only consumer so far, the oversized-file guard's scoped scan, + * went with that guard (taskless/cli#351). The field stays: this module is + * the generator, so it is the one place this fact can be stated rather than + * re-derived, and the next reader of the sections should not have to parse + * the file to get it. */ export interface AssembledValeConfig { /** Config path relative to the project root, for `--config`. */ diff --git a/packages/cli/src/rules/dispatch.ts b/packages/cli/src/rules/dispatch.ts index 8d9c093c..c83ea824 100644 --- a/packages/cli/src/rules/dispatch.ts +++ b/packages/cli/src/rules/dispatch.ts @@ -91,16 +91,6 @@ 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; @@ -187,7 +177,6 @@ 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/git-ignored.ts b/packages/cli/src/rules/git-ignored.ts index 2909e5ab..cadb1df2 100644 --- a/packages/cli/src/rules/git-ignored.ts +++ b/packages/cli/src/rules/git-ignored.ts @@ -119,10 +119,11 @@ const ROOT_ENTRIES = new Set(["./", "."]); * An entry carrying any of them is left out of the exclusion rather than * 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. + * function makes the opposite call (escape, not drop) for the per-file retry + * exclusion in `vale/run.ts`, where dropping would mean the one file that + * aborts the whole invocation is the one file left in it. 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 diff --git a/packages/cli/src/rules/vale/formats.ts b/packages/cli/src/rules/vale/formats.ts index dd4f338f..623cd601 100644 --- a/packages/cli/src/rules/vale/formats.ts +++ b/packages/cli/src/rules/vale/formats.ts @@ -1,5 +1,5 @@ -import { glob, stat } from "node:fs/promises"; -import { basename, extname, resolve as resolvePath } from "node:path"; +import { glob } from "node:fs/promises"; +import { basename, extname } from "node:path"; import { VALE_CONVERTER_BY_EXTENSION, @@ -155,9 +155,10 @@ export function converterExclusionGlobs(): string[] { * **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 + * `**\/*.adoc`) and a literal discovered path (the name of a target file the + * per-file retry in `run.ts` excludes) 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. */ @@ -176,13 +177,14 @@ export function buildValeGlob(patterns: string[]): string | undefined { * 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. + * findings inside a vendored tree, nothing more), while dropping a target + * file the per-file retry in `run.ts` has to exclude (one whose front matter + * Vale cannot parse, taskless/cli#300) means the one file that aborts the + * whole invocation is the one file left in it, and the retry would spin on + * the same failure. A false-positive skip is the wrong failure mode for the + * same reason a false-positive notice was in the sibling case: the risk the + * retry exists to contain 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 @@ -202,14 +204,10 @@ 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. + * A notice naming hundreds of files is not more readable than one naming + * five and a count. Kept as its own helper so a second notice (there was one, + * for files over a size cap, until Vale 3.21.0 made it unnecessary) shares + * the limit and the truncation shape instead of restating them. */ function summarizeList(labels: string[]): string { const sample = labels.slice(0, NOTICE_SAMPLE_LIMIT); @@ -226,11 +224,6 @@ 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. * @@ -300,224 +293,6 @@ 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. - * - * `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 - * 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". - * - * **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. - * - * **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, - paths: string[], - maxBytes: number, - wholeProject: boolean, - sectionGlobs?: string[] -): Promise { - const roots = wholeProject ? ["."] : 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 { - for await (const match of glob(`${prefix}**/*`, { - cwd, - exclude: isUnwalkedEntry, - })) { - await checkCandidate(String(match)); - } - } 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. - } - } - } 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); - } - } 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)); -} - /** * The user-facing sentence for a set of skipped files, or `undefined` when * nothing was skipped. @@ -554,61 +329,3 @@ 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 listed = summarizeList(files.map((entry) => entry.file)); - - 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 1707afcc..b8bfa6e8 100644 --- a/packages/cli/src/rules/vale/run.ts +++ b/packages/cli/src/rules/vale/run.ts @@ -20,8 +20,6 @@ import { converterExclusionGlobs, escapeGlobLiteral, findConverterDependentFiles, - findOversizedFiles, - oversizedFilesNotice, skippedFilesNotice, TASKLESS_DIRECTORY, } from "./formats"; @@ -50,75 +48,6 @@ 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. (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 - * (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. * @@ -527,33 +456,6 @@ 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[]; - /** - * 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; } /** @@ -652,32 +554,24 @@ 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([ + // + // There is no file-size guard here any more. Until Vale 3.21.0 lint time + // was superlinear in the size of a single Markdown block (3.20.0: a 3 MB + // one-block file took ~81 s, 384 KB ~7 s), so one oversized file could + // consume the whole run's `VALE_TIMEOUT_MS` and, because Vale writes + // nothing until the run finishes, cost every other file its findings + // (taskless/cli#321, #325). 3.21.0 indexes the walker's context and rune + // positions per block, and cost is now linear at roughly 2.8 µs per + // sentence regardless of block structure: the same 3 MB block measures + // ~230 ms, 25 MB ~2.2 s. A file would have to reach several hundred + // megabytes before it threatened the budget, which is not a document, so + // the preemptive exclusion and its notice were removed (taskless/cli#351). + // `VALE_TIMEOUT_MS` remains the ceiling on damage for anything that large. + const [ignoredEntries, converterDependent] = 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, - maxFileBytes, - wholeProject, - options.sectionGlobs - ), ]); - // 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 ? [ @@ -686,28 +580,13 @@ export async function runVale( ] : []), ...converterExclusionGlobs(), - // 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 - // 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, maxFileBytes), - ].filter((notice) => notice !== undefined); - const skipped = notices.length === 0 ? undefined : notices.join("\n"); + // The notice describes files this run declined to check, and it has to + // reach the user or the decline is silent. + const skipped = skippedFilesNotice( + converterDependent.filter((file) => !isGitIgnoredPath(file, ignoredEntries)) + ); // 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/assemble.test.ts b/packages/cli/test/assemble.test.ts index 02807d6d..5da1cae3 100644 --- a/packages/cli/test/assemble.test.ts +++ b/packages/cli/test/assemble.test.ts @@ -135,11 +135,9 @@ describe("Vale config assembly", () => { 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. + // `sections` 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( diff --git a/packages/cli/test/vale-formats.test.ts b/packages/cli/test/vale-formats.test.ts index 3f2693ea..ad098f95 100644 --- a/packages/cli/test/vale-formats.test.ts +++ b/packages/cli/test/vale-formats.test.ts @@ -14,10 +14,9 @@ import { converterFor, escapeGlobLiteral, findConverterDependentFiles, - findOversizedFiles, skippedFilesNotice, } from "../src/rules/vale/formats"; -import { runVale, VALE_MAX_FILE_BYTES } from "../src/rules/vale/run"; +import { runVale } from "../src/rules/vale/run"; /** * The exclusion derived from the format tiers, and the run that uses it. @@ -168,9 +167,9 @@ 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. + // characters, escaped here instead of dropped, because dropping a target + // file from the per-file retry's exclusion leaves the one file that aborts + // the run inside it. 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`); @@ -290,105 +289,6 @@ 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, true, [ - "**/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, 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, 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, true) - ).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-run.test.ts b/packages/cli/test/vale-run.test.ts index 9234e518..a495db2f 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, VALE_MAX_FILE_BYTES } from "../src/rules/vale/run"; +import { runVale } from "../src/rules/vale/run"; /** * These run the real Vale binary. It ships as an `optionalDependency` for the @@ -316,10 +316,11 @@ withVale("runVale against the real binary", () => { // 17,000: this test asserts the message rather than the blocking flag, // which the sibling covers with the larger fixture. // - // `maxFileBytes` raises `VALE_MAX_FILE_BYTES` for THIS CALL ONLY — not a - // CLI flag, not a config surface, just a seam. Without it a document this - // size is excluded before Vale sees it (taskless/cli#321) and reports - // `status: "ok"` with a notice, never exercising the timeout at all. + // Nothing excludes a document this size before Vale sees it. The 128KB + // guard that once did (taskless/cli#321) was written against 3.20.0's + // superlinear cost and removed once 3.21.0 made it linear + // (taskless/cli#351), which is also why the fixture needs no per-call + // override to reach the binary. const cwd = makeProject( `${header}\n[*.md]\nno-simply.no-simply = YES\n`, { "no-simply": existenceRule("simply", "Avoid 'simply'") }, @@ -330,7 +331,6 @@ withVale("runVale against the real binary", () => { cwd, paths: ["doc.md"], timeoutMs: 100, - maxFileBytes: Number.POSITIVE_INFINITY, }); expect(outcome.status).toBe("timeout"); if (outcome.status !== "timeout") return; @@ -470,198 +470,6 @@ 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", - }) - ); - }); - - 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"); - }); - - 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"); - }); - }); }); describe("ValeRunOutcome.blocking", () => { @@ -706,9 +514,9 @@ withVale("ValeRunOutcome.blocking against the real binary", () => { // 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: + // this test that had shrunk its fixture to fit under the then 128KB + // file-size guard (taskless/cli#321) read as a regression, but the ratio + // was the wrong number: // // | version | duration | budget | headroom | // | -------------------------------- | -------- | ------ | -------- | @@ -721,15 +529,14 @@ withVale("ValeRunOutcome.blocking against the real binary", () => { // ~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. + // That size guard capped how large a fixture this test could use once it + // started sharing `runVale`'s production guard (taskless/cli#321): a + // document over the limit was excluded before Vale ever saw it, reporting + // `status: "ok"` with a notice instead of exercising the timeout this + // test is about, so a `maxFileBytes` seam raised the limit per call. The + // guard and the seam are both gone (taskless/cli#351): 3.21.0 made the + // cost linear, so no document is excluded on size any more and the + // fixture reaches the binary as written. // // VALE 3.21.0 RE-MEASURED THE FIXTURE, AGAIN. Its perf work made this // workload ~20x faster, so 17,000 repetitions (323KB) ran in ~67ms on the @@ -750,7 +557,6 @@ withVale("ValeRunOutcome.blocking against the real binary", () => { cwd, paths: ["doc.md"], timeoutMs: 100, - maxFileBytes: Number.POSITIVE_INFINITY, }) ).toMatchObject({ status: "timeout", blocking: true }); }); diff --git a/packages/cli/test/vale-vendor-contract.test.ts b/packages/cli/test/vale-vendor-contract.test.ts index c2a17bf1..8a1dffb5 100644 --- a/packages/cli/test/vale-vendor-contract.test.ts +++ b/packages/cli/test/vale-vendor-contract.test.ts @@ -1352,29 +1352,28 @@ withVale("check types", () => { * 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 `[...]` + * Raised in review of taskless/cli#323, when the (since removed) oversized- + * file guard globbed `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. + * matches only the one at the project root — a real dialect mismatch for any + * code that resolves section patterns outside Vale. * * 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. + * `glob()` already treats it. The guard that prompted the question is gone + * (taskless/cli#351), but the fact outlives it: anything that reads + * `AssembledValeConfig.sections` to predict what Vale will visit depends on + * this, so it stays pinned. * * 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. + * basename-recursive semantics — this test fails, and any 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", () => {