fix(cli): skip an oversized file instead of letting Vale time out on it - #323
fix(cli): skip an oversized file instead of letting Vale time out on it#323thecodedrift wants to merge 4 commits into
Conversation
|
@claude /review |
|
Claude finished @thecodedrift's task in 3m 41s —— View job Review mode: incremental — read 0 prior review thread(s) before reviewing.
Todo
SummaryThe core idea — scope the oversized-file guard to what Vale would actually lint, using Confirmed, high severity: Plausible, needs verification: the new Also flagged, lower severity:
Conventions: no violations found. Barrel-import rules, declaration-emit export correctness ( Everything else in the diff — the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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
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.
683353f to
406d89c
Compare
… 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.
…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 #321 closed.
Addressed all four medium/low items and the two duplication nits, each with its own reply on the relevant thread:
Full verification: — AI Coding Agent |
1 participant
Stack (root → tip):
Summary
checkabandons every file's findings when a single file is large enough to exceed the Vale timeout, because Vale's cost is quadratic in one file's size andVALE_TIMEOUT_MSbounds the whole run, not one file. This is the same shape #300 fixed for unparseable files, on a path #300 did not cover.runValenow excludes a target file overVALE_MAX_FILE_BYTESbefore invoking Vale at all, and reports it in anoticesentry, the same preemptive treatmentconverterExclusionGlobsalready gives a format Vale cannot parse.Update: a review round found that the first version of this fix scanned the whole tree for oversized files with no relationship to what any Vale rule actually covers, producing false-positive notices on this very repository. That is fixed in a follow-up commit — see "Follow-up: scoping the scan" below.
Decisions, with evidence
1. Threshold: 128KB (
VALE_MAX_FILE_BYTESinsrc/rules/vale/run.ts). The repository owner supplied fresh measurements against the pinned binary (one rule, one file, median of three runs):VALE_TIMEOUT_MScovers the whole run, not one file, so the question is how much of the shared budget one outlier may consume. At 128KB a pathological file costs at most ~5%, even on the slower unmeasured CI estimate. 128KB of markdown is ~20,000 words, comfortably past hand-written prose — this excludes generated output, pasted data, or exported notes, not documents someone wrote. The full reasoning and table are inVALE_MAX_FILE_BYTES's docblock.2. Finding vs. notice: notice, not a finding. I initially matched #300's precedent (a hard
severity: "error"finding), on the issue's own suggested direction. Running the required before/after repro against this repository's own whole-projectchecksurfaced the problem: it reportedpnpm-lock.yamlandpackages/cli/CHANGELOG.mdas failures, and neither file is named by any[section]in any rule's.vale.ini— no rule was ever going to open either one. Unlike #300, where Vale itself proves a 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 ever have reached the file. So this followsskippedFilesNotice's existing precedent instead: anoticesentry, not a finding. The file is still excluded from the Vale invocation unconditionally either way — only the reporting softened.3. Happy-path cost of the stat walk: ~20-25ms measured against this repository's own whole-project target set. Negligible against the 60s run budget. (Numbers updated in the follow-up below — the scoped scan is comparable, not faster, in this repo specifically; see that section for why.)
Follow-up: scoping the scan to what Vale would actually lint
A second review round against this repository's own
checkcaught a real problem in the first version: the size scan walked**/*under the target root with no relationship to any rule's actual matcher, and reportedpnpm-lock.yaml(152,820 bytes) andpackages/cli/CHANGELOG.md(139,171 bytes) as "not checked" — even though no rule's[section]names either file. Vale was never going to open them, so that was a false positive, not a caught coverage hole.Fixed by asking the generator, not by re-parsing its output:
assembleValeConfig(src/rules/assemble.ts) now returns the section glob patterns it wrote into the assembled config, alongside the config path — read from the exact in-memory strings it is about to write, never by re-reading the.vale.iniafterward (STYLEGUIDE-CODE.md's "Verify Build Output In The Build, Not By Parsing It" is directly on point here).assembleEngineConfigs->DispatchOptions.valeSections->ValeRunOptions.sectionGlobs->findOversizedFiles, which now globs by those section patterns instead of a bare**/*walk — exactly asfindConverterDependentFilesglobs by its extension list.check some-file.yamlinstead of a whole-project run.sectionGlobsleftundefined) keeps the exact previous exhaustive-walk behavior —verifyValeRule's isolating config, or a test that handsrunValea hand-written.vale.inidirectly.Verified after the fix:
taskless check --jsonon this repository:{"success":true,"results":[]}— no oversize notice at all, because neitherpnpm-lock.yamlnorCHANGELOG.mdis in any rule's scope.[**/README.md], a 200KB in-scopeREADME.md, and a 200KB out-of-scopepnpm-lock.yaml: the notice names onlyREADME.md.findOversizedFilesdirectly) and the integration test (runValewithsectionGlobs) fail withpnpm-lock.yaml/huge.yamlreappearing; reverting restores green.packages/cli/src/**/*.ts) is itself a broad recursive glob over the whole TypeScript tree, and running N pattern-scoped globs instead of one**/*walk adds a bit of fixed overhead. Both are negligible against the 60s run budget; the primary win is eliminating the false positive, not speed.Repro (dedicated fixture: two normal
.mdfiles + one oversized file)Before (stashed the fix, rebuilt,
.mdfile at 1.6MB, matching the issue's own repro size):After (this fix):
Tests
test/vale-run.test.ts,describe("an oversized target file (taskless/cli#321)"):simplymatches, not filler, so an unexcluded file would visibly add thousands of findings)VALE_MAX_FILE_BYTES - 1(boundary test)*.mdalongside an oversized, out-of-scope.yamlfiletest/vale-formats.test.ts,describe("finding oversized files, scoped to what Vale would actually lint"):pnpm-lock.yaml/README.mdrepro in miniature)test/assemble.test.ts:assembleValeConfigreturns every section pattern it wrote, deduplicated and sorted.All mutation-checked by hand (applied, confirmed red, reverted, confirmed green): the exclusion-glob entry, the notice-building call, the
>/>=size comparison, and (in the follow-up) forcing the un-scoped fallback branch to always run.What the issue got wrong / worth flagging
Nothing factually wrong in the issue itself, but the issue's suggested direction ("report an oversized file as its own finding") turned out to be the wrong choice once tested against a real project — see decision 2. Separately, my own first attempt at the notice/exclusion fix was itself too broad until a second review round caught it against this repository's own
checkoutput — see the follow-up section.Verification
pnpm typecheck— passpnpm test(full@taskless/clisuite) — 1383/1383 passpnpm lint(build + eslint + house-stylecheck) — pass, "No issues found." (no oversize notice at all now, since neitherpnpm-lock.yamlnorCHANGELOG.mdis in scope)packages/cli/src/agent/create-vale-rule.mddocuments the new limit and its scoping next to the existing "single unreadable file" callout; topic bumped v6 -> v7.Fixes #321