fix(github): drop eager PR diff fetch — judges don't read it anyway#234
Conversation
|
Warning Review limit reached
Your plan includes 5 reviews of capacity. Refill in 23 minutes and 41 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThe PR makes PR diffs lazy and local-git-first: ChangesLocal Git Diff Preference with API Fallback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/cli/test_pulls_local_diff.py (2)
186-191: ⚡ Quick winTighten the “skip local path” test to also block
git fetch.Line 186 currently blocks only
git diff. If local logic still runsgit fetchwith missing refs, this test would not catch it.Proposed guard update
+ if cmd[:2] == ["git", "fetch"]: + raise AssertionError("local git path must NOT run when head/base are missing") if cmd[:3] == ["git", "diff", "--no-color"]: raise AssertionError("local git path must NOT run when head/base are missing")🤖 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 `@tests/cli/test_pulls_local_diff.py` around lines 186 - 191, The test currently only blocks the local-path call for git diff (the condition checking cmd[:3] == ["git", "diff", "--no-color"]) but misses git fetch; update that conditional to also detect git fetch (e.g., if cmd[:3] == ["git","diff","--no-color"] or cmd[:2] == ["git","fetch"]) and raise the same AssertionError so any attempt to run git fetch when head/base are missing fails the test; leave the ["gh","pr","diff"] branch (api_called["v"] = True) unchanged.
65-67: ⚡ Quick winAssert the exact three-dot diff refspec in the happy-path test.
At the moment, Line 65 and Line 78 only verify
git diff --no-colorwas called, not that it usedorigin/<base>...origin/<head>. A two-dot or wrong ref regression could still pass.Proposed test hardening
- # `git diff --no-color origin/main...origin/feat/big-branch` - if cmd[:3] == ["git", "diff", "--no-color"]: + # `git diff --no-color origin/main...origin/feat/big-branch` + if cmd[:4] == ["git", "diff", "--no-color", "origin/main...origin/feat/big-branch"]: return subprocess.CompletedProcess(cmd, 0, stdout="diff --git a/x b/x\n+huge content\n", stderr="") + if cmd[:3] == ["git", "diff", "--no-color"]: + raise AssertionError(f"unexpected git diff args: {cmd}") @@ - assert any(cmd[:3] == ["git", "diff", "--no-color"] for cmd in calls), "local diff command must have been called" + assert any( + cmd[:4] == ["git", "diff", "--no-color", "origin/main...origin/feat/big-branch"] + for cmd in calls + ), "local three-dot diff command must have been called"Also applies to: 78-79
🤖 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 `@tests/cli/test_pulls_local_diff.py` around lines 65 - 67, The test currently matches only the prefix cmd[:3] == ["git","diff","--no-color"], so it doesn’t assert the exact three-dot refspec; update the mock in tests/cli/test_pulls_local_diff.py (the block that returns subprocess.CompletedProcess) to verify the full git diff command includes the exact refspec "origin/<base>...origin/<head>" (or the concrete "origin/main...origin/feat/big-branch" used in the test) instead of only the first three args—e.g. check cmd == ["git","diff","--no-color","origin/main...origin/feat/big-branch"] or assert cmd[3] == "origin/main...origin/feat/big-branch"; apply the same stricter check for the second occurrence around the later mock (the one at lines 78-79) so two-dot/refspec regressions are caught.python/cube/github/pulls.py (1)
148-149: 💤 Low valueUse iterable unpacking for list construction.
Per RUF005, prefer unpacking over concatenation for cleaner syntax.
♻️ Suggested fix
diff_result = subprocess.run( - ["gh", "pr", "diff", str(pr_number)] + repo_flags, + ["gh", "pr", "diff", str(pr_number), *repo_flags], capture_output=True,🤖 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 `@python/cube/github/pulls.py` around lines 148 - 149, The subprocess.run call building the command for diff_result currently concatenates two lists; change it to use iterable unpacking instead of + (update the call where diff_result is assigned and the subprocess.run invocation that builds ["gh", "pr", "diff", str(pr_number)] + repo_flags) so the command list is constructed with unpacking of repo_flags; also make sure repo_flags is an iterable (list/tuple) before unpacking to avoid runtime errors.
🤖 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.
Nitpick comments:
In `@python/cube/github/pulls.py`:
- Around line 148-149: The subprocess.run call building the command for
diff_result currently concatenates two lists; change it to use iterable
unpacking instead of + (update the call where diff_result is assigned and the
subprocess.run invocation that builds ["gh", "pr", "diff", str(pr_number)] +
repo_flags) so the command list is constructed with unpacking of repo_flags;
also make sure repo_flags is an iterable (list/tuple) before unpacking to avoid
runtime errors.
In `@tests/cli/test_pulls_local_diff.py`:
- Around line 186-191: The test currently only blocks the local-path call for
git diff (the condition checking cmd[:3] == ["git", "diff", "--no-color"]) but
misses git fetch; update that conditional to also detect git fetch (e.g., if
cmd[:3] == ["git","diff","--no-color"] or cmd[:2] == ["git","fetch"]) and raise
the same AssertionError so any attempt to run git fetch when head/base are
missing fails the test; leave the ["gh","pr","diff"] branch (api_called["v"] =
True) unchanged.
- Around line 65-67: The test currently matches only the prefix cmd[:3] ==
["git","diff","--no-color"], so it doesn’t assert the exact three-dot refspec;
update the mock in tests/cli/test_pulls_local_diff.py (the block that returns
subprocess.CompletedProcess) to verify the full git diff command includes the
exact refspec "origin/<base>...origin/<head>" (or the concrete
"origin/main...origin/feat/big-branch" used in the test) instead of only the
first three args—e.g. check cmd ==
["git","diff","--no-color","origin/main...origin/feat/big-branch"] or assert
cmd[3] == "origin/main...origin/feat/big-branch"; apply the same stricter check
for the second occurrence around the later mock (the one at lines 78-79) so
two-dot/refspec regressions are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f14e490d-43bc-486d-8f20-1d1ffd76965c
📒 Files selected for processing (2)
python/cube/github/pulls.pytests/cli/test_pulls_local_diff.py
📜 Review details
🧰 Additional context used
🪛 Ruff (0.15.13)
python/cube/github/pulls.py
[error] 148-148: subprocess call: check for execution of untrusted input
(S603)
[warning] 149-149: Consider ["gh", "pr", "diff", str(pr_number), *repo_flags] instead of concatenation
Replace with ["gh", "pr", "diff", str(pr_number), *repo_flags]
(RUF005)
[error] 193-193: subprocess call: check for execution of untrusted input
(S603)
[error] 194-194: Starting a process with a partial executable path
(S607)
[error] 206-206: subprocess call: check for execution of untrusted input
(S603)
[error] 207-212: Starting a process with a partial executable path
(S607)
🔇 Additional comments (4)
tests/cli/test_pulls_local_diff.py (1)
1-64: LGTM!Also applies to: 68-77, 80-185, 192-199
python/cube/github/pulls.py (3)
85-98: LGTM!
100-111: LGTM!
168-223: LGTM!
Original problem: ``cube prv <N>`` aborted on large PRs with
``HTTP 406: PullRequest.diff too_large`` (GitHub's REST diff API
caps at 20,000 lines).
Jacob's question: "judges can use Read, no?" — exactly right. The
judges literally don't use the diff. judge_panel.py:353 tells them
"Don't pull the whole PR diff. Read files individually." The only
consumers are:
- the empty-PR safety check (boolean, doesn't need content)
- the dedupe agent (truncates to ~8k chars anyway)
The fix is two-pronged:
1. ``fetch_pr`` no longer eagerly fetches the diff. It returns
metadata + cwd/repo and is done. Removes the failure mode AND
cuts a network round-trip from every panel launch.
2. ``PRData`` gains two lazy methods:
- ``has_changes()`` — cheap boolean via `git diff --quiet` exit
status. No content fetch, no API cap. Defaults to True on git
failure so we never skip a real PR because the network blipped.
- ``get_diff(max_lines=8000)`` — local-git-first (no cap),
fallback to ``gh pr diff`` when refs aren't resolvable.
Truncates at ``max_lines`` with a marker line. Memoised.
Callers updated:
- peer_review.py: `pr.diff.strip()` → `pr.has_changes()`,
`pr_diff=pr.diff` → `pr_diff=pr.get_diff()` (lazy).
- ui_review.py: `pr.diff` → `pr.get_diff(max_lines=20000)`
(generous ceiling for big component bumps).
- pr_fix.py: didn't read .diff, unaffected.
Back-compat: ``PRData.diff`` still exists as a (str) attribute,
starts empty, gets populated on the first ``get_diff()`` call. Any
external caller reading ``.diff`` directly sees "" rather than
AttributeError.
Error UX: when the API IS hit AND fails with the 20k-line cap, the
error surfaces a copy-pasteable workaround command pointing at the
local-git path. Beats raw HTTP 406.
Tests (9 new):
- fetch_pr does NOT call any diff command eagerly
- get_diff prefers local git, falls back to API on git failure
- get_diff is memoised across repeated calls
- get_diff truncates at max_lines with a clear marker
- 20k cap error includes the workaround hint
- has_changes returns True/False correctly on git exit-status,
defaults to True on git failure
Full suite: 501/501 pass. mypy clean. ruff clean. Scope: 4 files
(pulls.py + 2 callers + the new tests). No scope creep.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
jacsamell
force-pushed
the
fix/large-pr-diff-via-local-git
branch
from
4d6d541 to
ea1f448
Compare
jacsamell
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@python/cube/github/pulls.py`:
- Around line 235-240: The subprocess.run call that assigns diff_result should
include a timeout (e.g., timeout=30) to avoid hanging; update the invocation of
subprocess.run([...], capture_output=True, text=True, cwd=cwd) to pass a
sensible timeout and wrap it in a try/except for subprocess.TimeoutExpired,
handling the timeout by logging/raising a clear error or returning an
empty/failure result so the caller can proceed (refer to the diff_result
variable and the subprocess.run call in this module).
- Around line 104-118: The current memoization stores a possibly truncated value
in self.diff so subsequent calls with a larger max_lines cannot recover missing
lines; change the caching so the full upstream text from _fetch_pr_diff is
stored (e.g. self._full_diff or similar) and only apply the max_lines truncation
when returning (keeping the splitlines/truncate logic), and update any
early-return that checks self.diff to instead check the full-cache
(self._full_diff) or compute truncation on every call; ensure you still use
_fetch_pr_diff(self.number, head_branch=self.head_branch,
base_branch=self.base_branch, cwd=self._cwd, repo_flags=(["--repo", self._repo]
if self._repo else [])) to populate the full cache.
🪄 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
Run ID: 504d2a6c-a1b5-4c65-85c4-308fb7197b78
📒 Files selected for processing (4)
python/cube/commands/peer_review.pypython/cube/commands/ui_review.pypython/cube/github/pulls.pytests/cli/test_pulls_local_diff.py
📜 Review details
🧰 Additional context used
🪛 Ruff (0.15.13)
python/cube/github/pulls.py
[error] 64-64: subprocess call: check for execution of untrusted input
(S603)
[error] 65-65: Starting a process with a partial executable path
(S607)
[error] 72-72: subprocess call: check for execution of untrusted input
(S603)
[error] 73-78: Starting a process with a partial executable path
(S607)
[error] 235-235: subprocess call: check for execution of untrusted input
(S603)
[warning] 236-236: Consider ["gh", "pr", "diff", str(pr_number), *repo_flags] instead of concatenation
Replace with ["gh", "pr", "diff", str(pr_number), *repo_flags]
(RUF005)
[error] 280-280: subprocess call: check for execution of untrusted input
(S603)
[error] 281-281: Starting a process with a partial executable path
(S607)
[error] 293-293: subprocess call: check for execution of untrusted input
(S603)
[error] 294-299: Starting a process with a partial executable path
(S607)
🔇 Additional comments (3)
python/cube/commands/peer_review.py (1)
565-569: LGTM!Also applies to: 858-867, 945-946, 1173-1174, 1246-1247
python/cube/commands/ui_review.py (1)
74-77: LGTM!Also applies to: 464-465
tests/cli/test_pulls_local_diff.py (1)
52-72: LGTM!Also applies to: 79-128, 130-176, 183-213, 220-277
![the-agent-cube[bot]](https://avatars.githubusercontent.com/u/156566864?s=60&v=4){kind=small}
| capture_output=True, | ||
| timeout=30, | ||
| ) | ||
| return diff_check.returncode == 1 |
There was a problem hiding this comment.
[WARNING] git diff --quiet returns 0 for no changes, 1 for changes, and values above 1 for errors. Treating every non-1 result as False can make an unresolvable ref after a successful fetch look like an empty PR, causing _run_pr_review() to exit instead of using the documented conservative fallback.
— Agent Cube / Judge Backend 🤖
| on the API path. The local checkout has the full diff with no cap. | ||
| """ | ||
| if head_branch and base_branch: | ||
| local_diff = _try_local_git_diff( |
There was a problem hiding this comment.
[WARNING] fetch_pr(..., repo=...) stores repo flags for the API fallback, but _fetch_pr_diff() still tries the local origin/<base>...origin/<head> diff first from the current checkout. For ui-review --pr N --repo other/repo, matching local branch names could produce a diff from this repository instead of the requested one.
— Agent Cube / Judge Backend, Judge Frontend & UX 🤖
| # UI review filters the diff by extension. Fetch lazily (local git | ||
| # first, no API cap). Cap is generous — ui_review's filter shrinks | ||
| # this further; we want headroom for big component bumps. | ||
| ui_diff = _filter_ui_diff(pr.get_diff(max_lines=20000)) |
There was a problem hiding this comment.
[WARNING] UI review caps the full PR diff at 20,000 lines before _filter_ui_diff() runs. In a large mixed PR with many non-UI changes before the first UI file, this can drop UI hunks and make the review incomplete or report no UI diff even though local git can provide the full diff.
— Agent Cube / Judge Backend 🤖
…emoise uncapped, timeout Round-2 cube findings (5 findings, all addressed): 1. has_changes treated >1 exit as empty (judge_1,2,3,4): git diff --quiet returns 0/1/>1 (no-diff/diff/error). Now: 0→False, 1→True, anything else falls through to conservative True. Matches docstring. 2. cross-repo regression (judge_1,2,3,4,5): local-git path ignored the --repo flag. cube ui-review --pr N --repo other/owner could diff THIS repo's branches if names happened to match. Both has_changes and _fetch_pr_diff now skip the local path when _repo is set; gh --repo is used instead so the diff comes from the target. 3. ui_review filter-then-truncate (judge_1,2,3,4): get_diff was capping at 20k BEFORE the UI filter ran. Large mixed PRs with non-UI hunks first lost their UI sections. Now: fetch with 1M-line ceiling, filter to UI only, THEN cap the FILTERED result at 20k. 4. memoisation contract (judge_2,3,4): self.diff cached the truncated value, so a later call with a larger cap got the smaller cap's result. Now memoise the UNCAPPED diff (_uncapped_diff), apply max_lines per-call. Each caller's cap is independent; second call doesn't re-shell out. 5. missing timeout on gh pr diff (judge_1,2,3,4): the API fallback had no timeout while every other subprocess in this file did. A stalled gh would hang the panel forever. Added timeout=120 to match the local-git path; TimeoutExpired raises a clear RuntimeError pointing operators at the local-git workaround. Tests: 2 new (14 total): - max_lines respected per-call after memoisation (larger cap recovers lines a smaller cap dropped, no second shell-out) - gh pr diff subprocess passes timeout= kwarg; TimeoutExpired surfaces as RuntimeError Full suite: 506/506 pass. mypy clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
🤖 Agent Cube Review
✅ All 5/5 judges approved this PR.
Issues:
- Resolve the stale existing thread at python/cube/github/pulls.py:255; the current cross-repo guard at python/cube/github/pulls.py:253-261 addresses it.
✅ Auto-approved by cube — 5/5 judges APPROVED, zero review blockers.
Per-judge:
- judge_1: APPROVED
- judge_2: APPROVED
- judge_3: APPROVED
- judge_4: APPROVED
- judge_5: APPROVED
CI runs in parallel and continues to gate merge; cube does not race it for approval.
🤖 Agent Cube Peer Review
Real-world hit: collins-platform PR #12 (123,773 additions, merged). After #234 landed, cube prv ran the local-git path but git diff --quiet origin/main...origin/feat/... returned exit 0 (no diff), so the empty-PR bail fired. False-empty. Root cause: three-dot origin/<base>...origin/<head> diffs from the merge-base. For OPEN PRs, merge-base = where head branched off base. For MERGED PRs, base has absorbed head — so merge-base IS head itself, and merge-base..head is empty. GitHub's diff endpoint dodges this by diffing the merge commit's two parents (^1 = base-before-merge, ^2 = head). cube does the same locally now: - fetch_pr requests mergeCommit alongside other PR fields - PRData.merge_commit_oid carries the SHA - has_changes() / get_diff() prefer git diff <merge>^1 <merge>^2 when the SHA is set. Fetches by SHA so this works even when the head branch was deleted post-merge. - Falls back to three-dot, then to API, then to conservative True. Smoke test on the real PR: has_changes True (was: False, silently bailed) get_diff 130k+ lines (was: empty) Tests: 2 new (16 total). Full suite: 508/508. mypy clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Real-world hit: collins-platform PR #12 (123,773 additions, merged). After #234 landed, cube prv ran the local-git path but git diff --quiet origin/main...origin/feat/... returned exit 0 (no diff), so the empty-PR bail fired. False-empty. Root cause: three-dot origin/<base>...origin/<head> diffs from the merge-base. For OPEN PRs, merge-base = where head branched off base. For MERGED PRs, base has absorbed head — so merge-base IS head itself, and merge-base..head is empty. GitHub's diff endpoint dodges this by diffing the merge commit's two parents (^1 = base-before-merge, ^2 = head). cube does the same locally now: - fetch_pr requests mergeCommit alongside other PR fields - PRData.merge_commit_oid carries the SHA - has_changes() / get_diff() prefer git diff <merge>^1 <merge>^2 when the SHA is set. Fetches by SHA so this works even when the head branch was deleted post-merge. - Falls back to three-dot, then to API, then to conservative True. Smoke test on the real PR: has_changes True (was: False, silently bailed) get_diff 130k+ lines (was: empty) Tests: 2 new (16 total). Full suite: 508/508. mypy clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…Rs (#236) Real-world hit: collins-platform PR #12 (123,773 additions, merged). After #234 landed, cube prv ran the local-git path but git diff --quiet origin/main...origin/feat/... returned exit 0 (no diff), so the empty-PR bail fired. False-empty. Root cause: three-dot origin/<base>...origin/<head> diffs from the merge-base. For OPEN PRs, merge-base = where head branched off base. For MERGED PRs, base has absorbed head — so merge-base IS head itself, and merge-base..head is empty. GitHub's diff endpoint dodges this by diffing the merge commit's two parents (^1 = base-before-merge, ^2 = head). cube does the same locally now: - fetch_pr requests mergeCommit alongside other PR fields - PRData.merge_commit_oid carries the SHA - has_changes() / get_diff() prefer git diff <merge>^1 <merge>^2 when the SHA is set. Fetches by SHA so this works even when the head branch was deleted post-merge. - Falls back to three-dot, then to API, then to conservative True. Smoke test on the real PR: has_changes True (was: False, silently bailed) get_diff 130k+ lines (was: empty) Tests: 2 new (16 total). Full suite: 508/508. mypy clean. Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
1 participant
Summary
Unblocks
cube prvon large PRs by removing the unnecessary diff fetch entirely.Jacob's question: "why are we diffing tho, judges can use READ no?" — exactly right. Judges literally don't read the PR diff.
judge_panel.py:353: "Don't pull the whole PR diff. Read files individually."So we were eagerly fetching the diff via
gh pr diff(capped at 20k lines, 406 on big PRs) for two consumers:What changes
fetch_prno longer fetches the diff at all. Returns metadata + cwd/repo handle and is done. Cuts a network round-trip from every panel launch.PRDatagains:has_changes()— cheap boolean viagit diff --quietexit status. No content fetch, no API cap. Defaults to True on git failure so we never silently skip a real PR.get_diff(max_lines=8000)— lazy. Local-git first (no cap), API fallback only when refs aren't resolvable. Truncates atmax_lineswith a marker. Memoised.Callers updated:
peer_review.py:pr.diff.strip()→pr.has_changes(),pr_diff=pr.diff→pr_diff=pr.get_diff()ui_review.py:pr.diff→pr.get_diff(max_lines=20000)pr_fix.py: didn't read.diff, unaffectedBack-compat
PRData.diffstill exists as astrattribute (default""), populated on firstget_diff()call. External callers reading it directly see""rather thanAttributeError.Test plan
🤖 Generated with Claude Code
Summary
Prevent "HTTP 406: PullRequest.diff too_large" failures by preferring local git for PR diffs and falling back to the GitHub API only when local refs are unavailable; provide a clear local-git workaround when the API hits the 20,000-line cap.
Changes
python/cube/github/pulls.py
tests/cli/test_pulls_local_diff.py
python/cube/commands/peer_review.py
python/cube/commands/ui_review.py
Tests & quality
Scope & impact