Skip to content

perf(review): skip duplicate full-file grounding fetch for modified files#4046

Merged
JSONbored merged 2 commits into
mainfrom
perf/grounding-dedup-modified-files-v2-iso
Jul 7, 2026
Merged

perf(review): skip duplicate full-file grounding fetch for modified files#4046
JSONbored merged 2 commits into
mainfrom
perf/grounding-dedup-modified-files-v2-iso

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

Closes #4009.

  • fetchFullFileContents unconditionally re-fetched the full post-change body of every changed file, even a MODIFIED file whose diff hunk already carries every line of the file (e.g. a small file rewritten in one hunk) — a byte-for-byte duplicate of what the diff itself already sent to the review prompt.
  • Adds diffFullyCoversFile: true only when a single hunk starts at line 1 on both sides AND its unchanged-line count is strictly below git's default 3-line context window. Fewer than 3 trailing unchanged lines proves the file truly ends there (git would show exactly 3 lines of context if more remained); exactly 3 is treated as ambiguous and falls back to fetching.
  • Deliberately scoped to status === "modified" only, so an added file's fetch (restored by fix(review): ground newly added files #3976, after perf(review): skip duplicate full-file grounding fetch for added files #3918's blanket status === "added" skip was found unsafe — GitHub can omit/truncate an added file's patch on large files) can never be re-skipped by this check.

Test plan

  • npm run typecheck — clean
  • npx vitest run test/unit/review-grounding.test.ts test/unit/grounding-wiring.test.ts — 55/55 passed, including 3 new tests: a fully-covering single-hunk rewrite (skips the fetch), a partial hunk at the exact 3-line context boundary (still fetches — this is the case an earlier draft of this change got wrong, caught by this exact test), and an added file that would otherwise look fully-covering (status gate holds, still fetches)

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@gittensory-orb gittensory-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.5x multiplier. label Jul 7, 2026
@gittensory-orb

gittensory-orb Bot commented Jul 7, 2026

Copy link
Copy Markdown

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-07 13:52:19 UTC

4 files · 1 AI reviewer · no blockers · readiness 100/100 · CI pending · blocked

⏸️ Suggested Action - Manual Review

Review summary
This adds `diffFullyCoversFile` to skip the redundant full-file GitHub fetch for a MODIFIED file whose single diff hunk already provably contains the entire post-change body (hunk starts at line 1 on both sides, and old/new context-line count is strictly below git's default 3-line context window). I traced the math: since trailing context is always ≤ total context counted by `oldCount - deletions`/`newCount - additions`, a total below 3 guarantees the file truly ends at the hunk — the algorithm can never falsely skip a fetch when unseen trailing content exists, only conservatively over-fetch (e.g., when interior context from merged nearby edits pushes the total to ≥3 even though trailing context alone is short), which is safe. The scoping to `status === 'modified'` (added files always fall through to `false`) correctly avoids reintroducing the #3976 large-added-file gap, and missing `patch`/`additions`/`deletions` also falls through to the safe fetch path.

Nits — 5 non-blocking
  • codecov/patch reports only 75% hit vs the 99% target; the tests shown cover multi-hunk, bare-shorthand, and combined-leading-context cases but I don't see a dedicated test for the malformed-hunk-header (`!match`) branch or the missing-patch/additions/deletions early-return in `diffFullyCoversFile` (src/review/review-grounding.ts:~180-192) — worth closing before merge given this repo's coverage bar.
  • src/review/grounding-wire.ts:106-113 — `toGroundingFiles` unconditionally assigns `additions`/`deletions` from `file.additions`/`file.deletions` while `patch`/`status` use the optional-spread pattern; since typecheck passed this compiles fine, but worth confirming `PullRequestFileRecord.additions`/`deletions` can never be `undefined` at runtime so the `!== undefined` guard in `diffFullyCoversFile` behaves as intended rather than silently always falling back to fetch.
  • The JSDoc above `diffFullyCoversFile` (review-grounding.ts) is quite long and repeats reasoning already stated in the `DIFF_CONTEXT_LINES` comment directly above it — could be trimmed for readability.
  • Add a unit test hitting the `!match` regex-mismatch path and the `!file.patch`/`additions === undefined`/`deletions === undefined` early-returns in `diffFullyCoversFile` to close the codecov/patch gap flagged by CI.
  • Consider a comment or test note confirming GitHub's Compare/Files API omits (not silently truncates) `patch` for oversized modified-file diffs, since the whole optimization's safety hinges on 'missing patch → always fetch' holding — same class of assumption fix(review): ground newly added files #3976 found broken for added files.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #4009
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 51 registered-repo PR(s), 43 merged, 560 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 51 PR(s), 560 issue(s).
Gate result ✅ Passing No configured blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 51 PR(s), 560 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.57%. Comparing base (39cc958) to head (eb5e72f).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4046   +/-   ##
=======================================
  Coverage   93.56%   93.57%           
=======================================
  Files         378      378           
  Lines       35448    35466   +18     
  Branches    13000    13011   +11     
=======================================
+ Hits        33167    33186   +19     
  Misses       1618     1618           
+ Partials      663      662    -1     
Files with missing lines Coverage Δ
src/review/grounding-wire.ts 96.38% <100.00%> (+1.44%) ⬆️
src/review/review-grounding.ts 94.28% <100.00%> (+1.42%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@gittensory-orb gittensory-orb Bot added the manual-review Gittensor contributor context label Jul 7, 2026
JSONbored added 2 commits July 7, 2026 06:47
…iles

fetchFullFileContents unconditionally re-fetched the full post-change body of
every changed file, even a MODIFIED file whose diff hunk already carries every
line of the file (a small file rewritten in one hunk) -- a byte-for-byte
duplicate of what the diff itself already sent to the review prompt.

Adds diffFullyCoversFile: true only when a single hunk starts at line 1 on both
sides AND its unchanged-line count is strictly below git's default 3-line
context window -- fewer than that proves the file truly ends there, since git
would show exactly 3 lines of context if more remained. Deliberately scoped to
status: modified only, so an added file's fetch (restored by #3976) can never
be re-skipped by this check.
…GroundingFiles

Codecov flagged 5 uncovered branches on the prior commit: diffFullyCoversFile's
multi-hunk guard, its bare single-line hunk-header shorthand (no comma count),
and its leading-context guard, plus toGroundingFiles' now-optional status field.
@JSONbored JSONbored force-pushed the perf/grounding-dedup-modified-files-v2-iso branch from bc99547 to eb5e72f Compare July 7, 2026 13:47
@JSONbored JSONbored merged commit 1dc6e0a into main Jul 7, 2026
9 checks passed
@JSONbored JSONbored deleted the perf/grounding-dedup-modified-files-v2-iso branch July 7, 2026 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.5x multiplier. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

perf(review): dedupe full-file grounding fetch for modified files

1 participant