fix(review): bound server memory for large tracked-file diffs

[rNoz]

Summary

Fixes #1120.

PR #1118 bounded server memory for large untracked files by rendering anything over MAX_REVIEW_FILE_CONTENT_BYTES (5 MiB) as a binary addition instead of loading its contents. The tracked working-tree diff path had no equivalent guard, so staging a large file (which moves it out of git ls-files --others and into git diff HEAD) re-buffered the full multi-megabyte patch. The reporter measured roughly 240 MB RSS after git add on a 51 MB text artifact. Any large tracked text file modified in the working tree hits the same unguarded path.

This change adds a size preflight in front of every content-producing tracked diff, so an oversized tracked blob is excluded from the real git diff invocation and rendered as a bounded stub instead. The oversized contents never enter git's diff machinery or the server's buffered stdout, matching how the untracked path is already bounded.

Approach

A raw preflight runs ahead of the diff: it enumerates the tracked paths in scope, batch-queries their object sizes with git cat-file --batch-check, and literally excludes (via pathspec) any path whose blob exceeds the 5 MiB threshold from the real git diff invocation. Each excluded path gets a bounded, diff-parseable stub spliced back in, in the same Binary files ... differ shape git itself emits for binary files (only for paths whose content actually changed, so a pure rename or a mode-only change never gets a spurious stub line), so downstream parsing is unaffected. --no-textconv is scoped to the size preflight itself, not the rendered diff, so a repository using a textconv driver still gets its normal converted diff for files under the size threshold; only a genuinely oversized file falls back to the raw stub. A gitlink (submodule) entry is never treated as an oversized blob, so submodule pointer changes keep rendering as the ordinary subproject-commit summary, and its diff still changes fingerprint on every pointer move.

For the freshness fingerprint, oversized worktree files are represented by size and mtime metadata rather than by hashing their content, so the every-few-seconds staleness poll stays cheap and never re-reads the bytes, while still changing when the file is genuinely edited (including an edit that keeps the same byte length). Reading that metadata goes through two required runtime-seam methods (file info and symlink resolution) on the existing per-runtime interface, rather than importing node:fs/node:path directly into the shared diff core. Making both methods required turns an incomplete host implementation into a compile error instead of silently disabling bounded reads or hunk expansion.

This covers the ordinary git provider's working-tree, staged, committed, and empty-tree diffs, the GitButler object diff, both PR-stack diff sites, and the file-content expansion endpoint, all against the same exact boundary (a blob at exactly the 5 MiB limit still renders as text; one byte over renders as a stub).

Scope

The preflight applies to every git-provider diff rather than only the working-tree ones named in the issue, so committed diffs of large tracked files and the empty-tree all view are bounded too. This covers the ordinary git provider and GitButler. The jj provider issues jj diff, which has no equivalent config knob, so it stays unbounded as it was before this change; a jj size guard would need its own stat-based approach and is out of scope here.

Validation

• Seven affected shared suites pass (157 tests), covering the size preflight, stub rendering, metadata-only fingerprint, gitlink handling, textconv preservation, the exact boundary, GitButler, PR stacks, VCS routing, commit history, and worktree pools.
• A broader validation pass across the affected packages plus a full typecheck and runtime check also passes.
• A compile-time regression assertion fails if either filesystem runtime method becomes optional again.
• Manual git confirmation: staging an oversized base64 text file renders as a short binary-style stub instead of a multi-megabyte patch, and a modified working-tree file still emits distinct fingerprints for distinct contents.
• Both the review-editor and hook production builds pass, confirming the shared diff core stays usable from the browser-bundled review UI.

Compatibility

Behavior-preserving for every file at or below 5 MiB (identical diff bytes), including files under a textconv driver. Oversized tracked files now render as a bounded stub, matching the established treatment of oversized untracked files, so the review UI is consistent across both. Submodule pointer changes are unaffected by the size guard. Host runtimes implementing the internal Git runtime interface must provide file metadata and symlink resolution; both production runtimes already did. No endpoint, prop, or payload changes.

Related work

Builds directly on #1118 (untracked-file bound). It may incidentally help #940 only when a single file in that repo exceeds the threshold; it does not address #940's primary trigger, which is a large file count rather than one oversized file.

[backnotprop]

Deep review done, with empirical verification against your branch's own code (git 2.50.1, driver script over real repos). The mechanism is sound and well-placed: the core.bigFileThreshold guard is prefixed on all 9 patch-producing diff sites plus the fingerprint, both runtimes flow through the shared function (verified neither has its own tracked-patch diff), vendor.sh and the pi tarball already carry both files, GitButler invariants untouched, and the stub keeps real blob hashes on both sides so staleness detection survives for bounded files. The #1120 repro (git add on a large file) is genuinely fixed: ~100 B stub across staged, uncommitted, and since-base. Renames, whitespace mode, both-sides size changes, committed views, and the exact 5 MiB boundary all verified bounded, and sub-threshold output is byte-identical with and without the guard.

One real hole and a few fixes before merge:

1. Equal-byte-size edits bypass the guard entirely. git's skip-stat-unmatch pass compares content when the two sizes match, populating both filespecs before bigFileThreshold is consulted. Measured: a committed 5 MiB file edited in place to the same length returns a 10.00 MiB patch on uncommitted, since-base, and unstaged, and the freshness fingerprint re-buffers it on every poll. Same-length edits are common (sed-style substitutions, fixed-width ID swaps, regenerated fixtures). At the reporter's 51 MB scale that is the original ~240 MB RSS again. Compounding it: the new test named "freshness fingerprint stays bounded..." writes a-repeat then b-repeat at identical lengths, which is exactly this case; it passes because the path is unbounded and the full patch gets hashed, while its comment claims the guard collapsed it. A test asserting a property that holds for the opposite reason is the thing most likely to mislead the next maintainer. Fix options: (a) a Bound server memory usage when reviewing large untracked files #1118-style size pre-check (enumerate over-limit tracked paths, exclude via pathspec, synthesize stubs) which is git-version- and gitattributes-independent, or (b) keep the config guard and document the hole explicitly in the constant's comment, rewriting that test around a size-changing edit. Either is acceptable; the false test comment is not.

2. A >5 MiB tracked text file is now silently unviewable with a dishonest label. It renders as "Binary files differ" with 0/0 counts and file-content expansion nulls (good: expansion cannot re-trigger the blowup; bad: no way to see the file at all). Realistic hit: a large monorepo lockfile that previously rendered as a normal diff. Bound server memory usage when reviewing large untracked files #1118 accepted this for untracked files; extending it to every tracked file is a bigger behavioral change than the description implies. Suggest at minimum a note in the docs, ideally a server-side marker so the UI can say "too large to diff (>5 MiB)" instead of "binary", and consider an env override.

3. Small ones: verified that a repo .gitattributes with an explicit text diff attribute or a textconv driver defeats the guard even in the blob-vs-blob path (worth a sentence in the comment; the size pre-check option is immune); the guard now leaks into user-facing error strings via the formatted git command in assertGitSuccess; pr-stack.ts's two PR diffs are one-line additions where the guard works reliably and a 51 MB blob in a PR still buffers today; the relaxed commandArgs.includes("diff") test assertions would match a pathspec literally named diff, prefer index-based; boundary at exactly MAX is untested (verified: MAX = text, MAX+1 = stub, worth pinning).

Nothing here is a regression against main: every case is bounded or unchanged. The gap is between what the PR claims and what it delivers, and the fix list is small. Happy to re-review on push.

[rNoz]

Pushed the review fixes. Equal-size edits, attributes, PR stack diffs, and the exact boundary are covered. Checks are passing.

[backnotprop]

Re-verified 7775aa0 empirically with the same driver-script approach. The redesign to a #1118-style preflight was the right call and every original finding is genuinely fixed: the equal-size edit now returns ~100-byte stubs on all three working-tree views AND the fingerprint (which uses size+mtime metadata with zero hash-object calls while polling, still flipping on same-size re-edits); gitattributes/textconv bypass is immune by construction and covered by a rewritten test that intercepts every rendered patch and pins the byte bound for real this time; both pr-stack sites are guarded with error messages preserved; boundary pinned at exactly MAX vs MAX+1; assertions are index-based; and the batch design keeps preflight cost O(1) in file count (one cat-file --batch-check, measured +15ms on the fingerprint poll). Adversarial probing of the new mechanism held up against glob-hostile paths including newlines, merge-conflict states, blobless partial clones, and rename/delete/mode stubs, with byte-identical output to main everywhere except genuinely oversized files. Full suite 2797 pass, typecheck clean, pi pack correct.

The new mechanism introduces one blocker and two decisions, then this is ready:

1. Blocking: every submodule pointer change is destroyed. getGitObjectSizes maps objects that cat-file reports missing to +Infinity, and gitlink (mode 160000) commit ids are always missing from the superproject's object store. Verified on a real submodule bump: main renders the Subproject commit old/new SHAs (the only information a submodule diff carries); this PR renders "Binary files a/sub and b/sub differ". Worse, the stub hardcodes the new id to 000000000000, so moving the pointer again produces an identical fingerprint and the "Diff out of date" banner never fires for submodule changes. Fix: exempt mode 160000 from oversize classification (or treat missing as unknown-not-oversized for gitlinks), plus a regression test.

2. --no-textconv silently changes normal diffs. It is injected into the rendered patch, not just the preflight, so repos using textconv drivers (pdf, exif, docx, ipynb) lose their readable diffs relative to main (verified with a custom driver). There is a defensible motive, since a driver can expand a small file into an unbounded conversion the size preflight cannot see, but it is undocumented and untested. Either scope it to the raw preflight or keep it and document the deliberate tradeoff with a test.

3. Small: stubs always emit "Binary files differ" even when nothing did. A pure rename shows similarity 100% with identical object ids plus a spurious differ line, and a mode-only chmod gets one too. Gate the line on oldObjectId !== newObjectId.

Nits, take or leave: hash-object slurps the full oversized worktree file per patch (a core.bigFileThreshold on that one call would make git stream it); stubs append at the end with a different abbreviation width than the rest of the diff; the big-lockfile UX from the first review remains an accepted tradeoff worth a docs line eventually.

The rewritten tests deserve a call-out: intercepting rendered patches and asserting the byte bound, running under a textconv attribute, and pinning the exact exclude pathspec argv is exactly how this property should be pinned. Happy to do a final pass on push.

[rNoz]

Pushed the final fixes. Gitlinks remain normal pointer diffs, small textconv output is preserved, pure rename and mode-only stubs stay honest, and the browser build boundary is restored. Local review and hook builds pass.

[backnotprop]

Final verification of 141eb4b, all with the same empirical driver. Everything from the previous round is fixed with directed regression tests:

• Gitlinks: mode-160000 entries are excluded from the batch id list and the oversize loop, so a submodule pointer can never be misread as an infinite-size missing object. Verified on a real submodule bump: uncommitted output is byte-identical to main, Subproject lines intact on all views, and the fingerprint moves again when the pointer moves (it was frozen before).
• Textconv: --no-textconv now scoped to the raw preflight only; the small-file custom-driver repro is byte-identical to main again, and the oversized-source case stays bounded on every view including the worst-case same-length edit. The sub-threshold-file-with-exploding-driver residual is unchanged from main, so no new exposure.
• Stub honesty: the differ line is gated on object-id inequality. Pure rename shows similarity 100 with no differ line, mode-only chmod shows old/new mode only, rename-with-edit still says differ. Correct in all three.
• The browser build boundary claim is a misnomer but the change is good: nothing was broken (main's review-core already had node builtins and no browser bundle imports it, confirmed against the built HTML), and what the commit actually does is remove the node builtins entirely by routing filesystem touches through two new runtime seam methods, implemented in both production runtimes with a genuine virtual-runtime seam test. Both single-file builds succeed.
• Suite 2803 pass, typecheck clean, pi pack correct, and output parity: every view byte-identical to main on this repo except the one genuinely oversized file, which differs by id-abbreviation width and patch position only.

One ask, fine as a follow-up if you prefer to land now: make getFileInfo/readLink required rather than optional on ReviewGitRuntime. A runtime that omits them compiles today and silently loses the memory bounds (measured: the untracked guard from #1118 disables entirely) plus hunk expansion. Every implementer already has them, so requiring them costs nothing and turns a silent degradation into a compile error, which matters before this seam gets copied by other hosts.

Merge-ready from my side. For the record across all three of your PRs today: this is some of the best iterative contribution work this repo has seen, and the redesigns (preflight here, occurrence-binding in #1169, the prefix-sum index in #1168) each ended up structurally better than the review's own suggestions.

[rNoz]

Merge-ready from my side. For the record across all three of your PRs today: this is some of the best iterative contribution work this repo has seen, and the redesigns (preflight here, occurrence-binding in #1169, the prefix-sum index in #1168) each ended up structurally better than the review's own suggestions.

Thank you for your words. Don't forget checking this fix #1154 (I am using in my integration branch on a daily basis, as I run my own plannotator built from that branch).
Happy to contribute!

P.S. Working on the optional follow-up.

[rNoz]

Implemented the optional runtime-contract follow-up: getFileInfo and readLink are required, the optional fallback branches are removed, and a compile-time assertion prevents either method from becoming optional again. :)

All typed Git and GitButler test runtimes satisfy the contract. Local validation: 157 affected tests, every TypeScript project check, and both review/hook builds pass. CI is running on the new head.

[backnotprop]

Verified cb06aff: getFileInfo and readLink are required members, the optional-fallback branches are gone (net -13 lines in review-core), and the IsRequired compile-time assertion pins both so a future edit cannot quietly re-optionalize them. Typecheck clean, 1194 shared+server tests pass. Nothing outstanding; fully merge-ready including the follow-up. Thanks for closing the loop on the same day.