perf(lint-cli): verify the buildinfo instead of rebuilding the program - #658
Merged
Conversation
`computeAffectedFiles` constructed a full TypeScript program on every warm run purely to learn nothing had changed — 1.3s on this repo, 98% of it in `createEmitAndSemanticDiagnosticsBuilderProgram`. `runBuilder` now reads the existing `.tsbuildinfo` first and verifies it directly: TypeScript's own `getSourceFileVersionAsHashFromText` over text read through `ts.sys`, plus a root-membership comparison against the config's resolved file names. Nothing changed means an empty affected set with no program, no emit and no write — 80ms instead of 1.3s. The check only sees source text and root membership. `impliedFormat` drift, `referencedMap` drift and referenced-file deletion are resolution effects it is structurally blind to, so a gate written beside each buildinfo covers them: the effective compiler options, the resolution fields and `type` of every manifest in the workspace, and a lockfile content hash. It is written by the builder path only, after a successful persist, and only compared by the fast path — a compare-and-swap would consume the change at read time and leave a gate vouching for a buildinfo the builder never advanced. `emitDeclarationOnly` halves what the persist emits and discards (428 outputs and 1.8MB down to 217 and 0.6MB). It buys no wall-clock time: the emit's cost is the declaration work, which the shape hash needs. `persistBuilderState` now writes through a temp file and a rename, since the buildinfo has a second reader. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
✅ Deploy Preview for beamish-daffodil-b0f61d ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis change adds deterministic resolution gates, workspace-member discovery, and defensive ChangesBuildinfo fast path
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AffectedFiles
participant ResolutionGate
participant BuildInfoCheck
participant Builder
participant GateState
AffectedFiles->>ResolutionGate: compute resolution digest
AffectedFiles->>BuildInfoCheck: validate buildinfo and gate
BuildInfoCheck-->>AffectedFiles: unchanged or invalid
AffectedFiles->>Builder: run incremental builder when invalid
Builder->>GateState: persist gate after successful buildinfo write
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/lint-cli/lib/files/workspace.ts (1)
22-29: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider widening
SKIPPED_DIRECTORIES.Only
.git/node_modulesare skipped; large build-output directories (dist,.turbo,.next,coverage, etc.) inside a monorepo still get walked, consumingWALK_BUDGETunnecessarily. The budget-exceeded fallback (undefined) keeps this safe, but it costs an extra builder pass on large trees.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lint-cli/lib/files/workspace.ts` around lines 22 - 29, Widen SKIPPED_DIRECTORIES to include common generated and build-output directories such as dist, .turbo, .next, and coverage, while retaining .git and node_modules. Keep the workspace member discovery and WALK_BUDGET fallback behavior unchanged.src/lint-cli/lib/typescript/typescript-internal.d.ts (1)
15-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid relying on this internal TypeScript export.
getSourceFileVersionAsHashFromTextis not a public TypeScript API and can be renamed or removed without notice. The currenttypeof === "function"guard makes the absence safe, but a future TypeScript release can silently disable this fast path without any visible signal; use a public/unsupported-but-documented API or accept that this is a fragile performance optimization.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lint-cli/lib/typescript/typescript-internal.d.ts` around lines 15 - 18, Update the TypeScript integration around getSourceFileVersionAsHashFromText to stop depending on this unstable internal export. Prefer a public or explicitly documented unsupported API for source-file version hashing; otherwise remove the optional fast path and use the existing stable fallback rather than silently relying on a potentially renamed or removed symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lint-cli/lib/files/workspace.ts`:
- Around line 309-318: Update patternDepth to add the globstar search depth to
the number of literal segments preceding the first "**", rather than returning
GLOBSTAR_DEPTH alone. Preserve the minimum depth behavior for non-globstar
patterns and ensure patterns with multiple globstars use the relevant
literal-prefix depth so findWorkspaceMembers does not stop before valid members.
In `@src/lint-cli/lib/typescript/buildinfo.ts`:
- Around line 235-262: Bound range expansion in expandRootIds with a sane
maximum such as a MAX_ROOT_RANGE constant, rejecting or otherwise safely
handling any [start, end] range that exceeds it before entering the id loop.
Preserve existing validation for numeric bounds and normal expansion for
realistic ranges.
In `@src/lint-cli/lib/typescript/gate.ts`:
- Around line 69-81: Update the lockfile hashing flow around readFileIfPresent
so bun.lockb is read and hashed as raw bytes rather than a UTF-8 string. Add or
reuse an fs-based byte-reading helper, while preserving the existing handling
for missing files and text lockfiles; ensure crypto.createHash(...).update
receives the unmodified file bytes.
---
Nitpick comments:
In `@src/lint-cli/lib/files/workspace.ts`:
- Around line 22-29: Widen SKIPPED_DIRECTORIES to include common generated and
build-output directories such as dist, .turbo, .next, and coverage, while
retaining .git and node_modules. Keep the workspace member discovery and
WALK_BUDGET fallback behavior unchanged.
In `@src/lint-cli/lib/typescript/typescript-internal.d.ts`:
- Around line 15-18: Update the TypeScript integration around
getSourceFileVersionAsHashFromText to stop depending on this unstable internal
export. Prefer a public or explicitly documented unsupported API for source-file
version hashing; otherwise remove the optional fast path and use the existing
stable fallback rather than silently relying on a potentially renamed or removed
symbol.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5aef4909-877a-47ad-8b1e-0bf150120572
📒 Files selected for processing (9)
src/lint-cli/lib/cache/package-hash.tssrc/lint-cli/lib/files/workspace.tssrc/lint-cli/lib/stable-json.tssrc/lint-cli/lib/typescript/affected.tssrc/lint-cli/lib/typescript/buildinfo.tssrc/lint-cli/lib/typescript/gate.tssrc/lint-cli/lib/typescript/typescript-internal.d.tstest/buildinfo-child.tstest/lint-cli-buildinfo.spec.ts
Comment on lines
+309
to
+318
| /** | ||
| * How many directory levels below the root a pattern can match at. | ||
| * | ||
| * @param pattern - One workspace package glob. | ||
| * @returns The deepest level the walk must reach for it. | ||
| */ | ||
| function patternDepth(pattern: string): number { | ||
| const segments = pattern.split("/").filter((segment) => segment !== "" && segment !== "."); | ||
| return segments.includes("**") ? GLOBSTAR_DEPTH : Math.max(segments.length, 1); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
patternDepth under-scans globstar patterns with a literal prefix.
GLOBSTAR_DEPTH (4) is applied flatly whenever ** appears, regardless of how many literal segments precede it. For a pattern like "a/b/c/packages/**", real members can sit deeper than the computed maxDepth, so the walk stops before reaching them and they are silently omitted from findWorkspaceMembers's result — the opposite of the function's stated design goal that "under-reporting would hide a real resolution change."
🐛 Proposed fix: account for the literal prefix before `**`
function patternDepth(pattern: string): number {
const segments = pattern.split("/").filter((segment) => segment !== "" && segment !== ".");
- return segments.includes("**") ? GLOBSTAR_DEPTH : Math.max(segments.length, 1);
+ const globstarIndex = segments.indexOf("**");
+ if (globstarIndex === -1) {
+ return Math.max(segments.length, 1);
+ }
+
+ return globstarIndex + GLOBSTAR_DEPTH;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * How many directory levels below the root a pattern can match at. | |
| * | |
| * @param pattern - One workspace package glob. | |
| * @returns The deepest level the walk must reach for it. | |
| */ | |
| function patternDepth(pattern: string): number { | |
| const segments = pattern.split("/").filter((segment) => segment !== "" && segment !== "."); | |
| return segments.includes("**") ? GLOBSTAR_DEPTH : Math.max(segments.length, 1); | |
| } | |
| function patternDepth(pattern: string): number { | |
| const segments = pattern.split("/").filter((segment) => segment !== "" && segment !== "."); | |
| const globstarIndex = segments.indexOf("**"); | |
| if (globstarIndex === -1) { | |
| return Math.max(segments.length, 1); | |
| } | |
| return globstarIndex + GLOBSTAR_DEPTH; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lint-cli/lib/files/workspace.ts` around lines 309 - 318, Update
patternDepth to add the globstar search depth to the number of literal segments
preceding the first "**", rather than returning GLOBSTAR_DEPTH alone. Preserve
the minimum depth behavior for non-globstar patterns and ensure patterns with
multiple globstars use the relevant literal-prefix depth so findWorkspaceMembers
does not stop before valid members.
Comment on lines
+235
to
+262
| function expandRootIds(root: unknown): Array<number> | undefined { | ||
| if (!Array.isArray(root)) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const ids: Array<number> = []; | ||
| for (const entry of root) { | ||
| if (typeof entry === "number") { | ||
| ids.push(entry); | ||
| continue; | ||
| } | ||
|
|
||
| if (!Array.isArray(entry)) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const [start, end] = entry as Array<unknown>; | ||
| if (typeof start !== "number" || typeof end !== "number") { | ||
| return undefined; | ||
| } | ||
|
|
||
| for (let id = start; id <= end; id += 1) { | ||
| ids.push(id); | ||
| } | ||
| } | ||
|
|
||
| return ids; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unbounded expansion of [start, end] root-id ranges.
expandRootIds iterates start..end with no cap. A corrupted or crafted .tsbuildinfo's root array (explicitly documented as untrusted input) could contain an absurd range, causing this loop to run for an extremely long time or exhaust memory before the later per-id name lookup ever gets a chance to reject it.
🛡️ Proposed fix: bound the range against a sane limit
function expandRootIds(root: unknown): Array<number> | undefined {
if (!Array.isArray(root)) {
return undefined;
}
const ids: Array<number> = [];
for (const entry of root) {
if (typeof entry === "number") {
ids.push(entry);
continue;
}
if (!Array.isArray(entry)) {
return undefined;
}
const [start, end] = entry as Array<unknown>;
if (typeof start !== "number" || typeof end !== "number") {
return undefined;
}
+ if (end < start || end - start > MAX_ROOT_RANGE) {
+ return undefined;
+ }
+
for (let id = start; id <= end; id += 1) {
ids.push(id);
}
}
return ids;
}(MAX_ROOT_RANGE could be a small constant, e.g. a few thousand — realistic root-file counts never approach that)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lint-cli/lib/typescript/buildinfo.ts` around lines 235 - 262, Bound range
expansion in expandRootIds with a sane maximum such as a MAX_ROOT_RANGE
constant, rejecting or otherwise safely handling any [start, end] range that
exceeds it before entering the id loop. Preserve existing validation for numeric
bounds and normal expansion for realistic ranges.
Comment on lines
+69
to
+81
| const lockfiles: Record<string, unknown> = {}; | ||
| const lockfileDirectories = new Set([cwd, root]); | ||
| for (const directory of lockfileDirectories) { | ||
| for (const name of LOCKFILES) { | ||
| const raw = readFileIfPresent(path.join(directory, name)); | ||
| if (raw !== undefined) { | ||
| lockfiles[`${path.relative(root, directory)}/${name}`] = crypto | ||
| .createHash("sha256") | ||
| .update(raw) | ||
| .digest("hex"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Binary bun.lockb is hashed through a lossy UTF-8 decode.
readFileIfPresent reads every lockfile as a UTF-8 string (via fs.readFileSync(path, "utf8")), but bun.lockb is a binary format. Invalid byte sequences get replaced with U+FFFD before hashing, so two different binary lockfiles that decode to the same replacement-character string would produce the same digest — a real dependency change could then fail to invalidate the gate.
🐛 Proposed fix: hash lockfiles by raw bytes
- const raw = readFileIfPresent(path.join(directory, name));
- if (raw !== undefined) {
- lockfiles[`${path.relative(root, directory)}/${name}`] = crypto
- .createHash("sha256")
- .update(raw)
- .digest("hex");
- }
+ let raw: Buffer | undefined;
+ try {
+ raw = fs.readFileSync(path.join(directory, name));
+ } catch {
+ raw = undefined;
+ }
+ if (raw !== undefined) {
+ lockfiles[`${path.relative(root, directory)}/${name}`] = crypto
+ .createHash("sha256")
+ .update(raw)
+ .digest("hex");
+ }(would need import fs from "node:fs"; or an equivalent raw-bytes helper alongside readFileIfPresent)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lint-cli/lib/typescript/gate.ts` around lines 69 - 81, Update the
lockfile hashing flow around readFileIfPresent so bun.lockb is read and hashed
as raw bytes rather than a UTF-8 string. Add or reuse an fs-based byte-reading
helper, while preserving the existing handling for missing files and text
lockfiles; ensure crypto.createHash(...).update receives the unmodified file
bytes.
Slicing the real file at a byte offset assumed a fixed layout. How far 400 characters reaches into a buildinfo depends on the path lengths baked into it, and those are platform-specific: a fixture whose `typescript` resolves onto another drive cannot store relative file names, so it stores absolute ones and the same offset lands somewhere else entirely. The cut corrupted fatally on Windows and harmlessly on Linux, where the builder simply rebuilt and the pass was never skipped. Write a fixed unparseable prefix instead, and assert that both sides were actually corrupted — a mutation that silently matched no state file would have made the surviving assertions pass for the wrong reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
TypeScript compares the emitted buildinfo name against its own normalised form, so a native Windows path with backslashes tripped an internal assertion — `Debug Failure. Expected <forward-slash> === <backslash>` — whenever the builder had to rebuild from no usable prior state. The throw was swallowed by `computeAffectedFiles`, which returned undefined, so the run silently skipped type-aware invalidation and left the buildinfo unrepaired: a corrupt state file poisoned every later run on Windows. TypeScript recovers from an unparseable buildinfo by design — `getBuildInfo` reads it as "no prior state" — so with the path normalised the rebuild completes and rewrites the file. The A/B scenario now pins that recovery rather than the failure it used to reproduce, on both platforms. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
computeAffectedFilesconstructed a full TypeScript program on every run to decide which files the type-aware ESLint pass must re-lint. On this repo that is 1349 files / 14.8 MB, and 98% of the cost is a singlecreateEmitAndSemanticDiagnosticsBuilderProgramcall — paid in full even when the answer is "nothing changed".The
.tsbuildinfoalready stores what that question needs: per-file source-text hashes, the root set, and pending-work markers. So verify it directly and only build a program when verification declines.Warm run, nothing changed: ~1.3–2.3s → ~0.2s. Runs that do have a change pay ~74ms for the failed verification, ~1% of their cost.
Correctness
The equivalence target is
createBuilderProgramState(typescript.js:131324), which marks a file changed iff it is new, itsversionhash differs, itsimpliedFormatdiffers, itsreferencedMapkeys differ, or a referenced file was deleted. Verification covers the hash test directly and root membership via therootfield; it structurally cannot see format or resolution drift, so those are gated.getSourceFileVersionAsHashFromText, over text read withts.sys.readFile. Not a reimplementation: TS strips a trailingsourceMappingURLcomment before hashing, andfs.readFileSync(f, "utf8")would mishandle BOMs and UTF-16, silently disabling the fast path forever with nothing failing. The UTF-8-BOM/UTF-16 A/B pair exists to catch exactly that.rootfileId ranges, notfileNames.fileNamesis every program file including libs andnode_modules, a strict superset of the roots; comparing against it would never match and the fast path would never fire.typeofpackage.jsonfor cwd, workspace root and every workspace member, plus lockfile content hashes. This covers the two drift classes nothing else caught: a workspace sibling remapping itsexports(doesn'"'"'t touch the lockfile, isn'"'"'t in the bust patterns) and atypeflip changingimpliedFormatprogram-wide.The fast path never writes. Gate state is written only by the builder path, after a successful persist. A compare-and-swap would consume the change at read time: options change → CAS stores the new value → falls through → builder throws → nothing persists → next run reads "unchanged" and verifies against a buildinfo the builder never advanced. For the same reason the existing
package-json-hashstate is not reused —plan.tshas already swapped it earlier in the same process.Bails to the builder on: pending
changeFileSet/affectedFilesPendingEmit/pendingEmit/checkPending,resolvedRoot,options.outFile, ats.versionmismatch, an unexpected shape, or a missing hash function. One try/catch around the whole thing; an unreadable file is a mismatch, never a throw.persistBuilderStatenow writes temp+rename, since the buildinfo has a second consumer.Documented as out of contract: non-root in-repo files that shadow resolution, hand-edited
node_moduleswith no lockfile change.Testing
23 new tests. Each A/B scenario builds two identical fixtures, warms them identically, then forces one side to decline and runs both in separate processes against private buildinfos. Whether the fast path actually fired is observed by backdating gate mtimes, since only the builder path writes them.
All 21 scenarios agree with the real builder. Untouched; mtime-bumped-but-identical; real change; post-persist; content restored so the buildinfo describes edited text; first run; lockfile-only bump;
typeflip; siblingexportsremap; sibling unrelated field (control — verification correctly stays engaged); tsconfig gaining and losing a root; deleted root and deletednode_modulesdeclaration; truncated buildinfo; TS patch bump;outFileproject; a solution with one cold and one warm project; BOM/UTF-16 clean and edited.Plus a regression assertion that a normally-persisted buildinfo leaves all four pending markers absent — without it, a future change could leave
affectedFilesPendingEmitpopulated and permanently disable this feature with no test failing.Full suite: 526 passing (503 on
main).Also:
emitDeclarationOnly, kept but not a winForcing
emitDeclarationOnly: truein the builder options halves the emit'"'"'s output — 428 writes/1.8 MB → 217/0.6 MB — and a controlled a←b←c fixture confirms it does not degrade shape hashing: a body-only edit still reports one file, not its importers.But wall clock does not move. The emit'"'"'s cost is the declaration work itself, which
declaration: truerequires. The commit says so plainly so it isn'"'"'t later mistaken for a perf lever.Review notes
findWorkspaceMembersparsespnpm-workspace.yaml'"'"'spackages:with a hand-rolled line scanner — no YAML or glob parser is a production dependency here. It handles block and JSON-flow forms; anything else yields no gate, which falls through to the builder. This is the piece most worth a second look.🤖 Generated with Claude Code
Summary by CodeRabbit