fix(agent): budget file groups by review body size, not just diff size (#1171) - #1194
fix(agent): budget file groups by review body size, not just diff size (#1171)#1194beillahi wants to merge 1 commit into
Conversation
|
✅ OpenCodeReview: Review complete: 0 finding(s) across 1 selected item(s). |
Qiyuanqiii
left a comment
There was a problem hiding this comment.
This addresses the gap described in #1171 at the right layer. Accounting for the file bodies that may be pulled into the review context prevents small-diff/large-body groups from being dispatched as a single item only to fail later during compression.
The fallback to per-file review is consistent with the existing behavior, and the regression tests cover both the overflowing and healthy cases as well as the single-file boundary.
I don't see any blocking issues with the implementation. Looks good to me.
| // reviewGroupBudgetFraction is the share of the per-item token budget a group's | ||
| // file *content* may occupy before the group is split to per-file review. The | ||
| // remainder is reserved for the review's own working set — reasoning, tool-call | ||
| // framing, and the compression summaries that share the context window with the | ||
| // file bodies the agent reads. Reviewing a patch means reading the surrounding | ||
| // file body via file_read, so a group's real cost is dominated by member bodies, | ||
| // not diffs; a group whose bodies alone approach the budget cannot also hold that | ||
| // working set and overflows ("context compression exceeded its threshold"). | ||
| // | ||
| // Calibrated against observed runs: a 6-file group totaling ~97k body tokens | ||
| // overflowed a 160k per-item ceiling, while ~77–80k groups reviewed cleanly. 0.55 | ||
| // (≈88k of a 160k budget) splits the former with margin and keeps the latter. |
There was a problem hiding this comment.
Can you clean these up? They seem long and pointless.
There was a problem hiding this comment.
Done — trimmed all three comments in grouping.go to the essentials and dropped the calibration narrative (that context lives in the PR description and #1171). Force-pushed.
enforceGroupTokenBudget measured only CountTokens(d.Diff), but reviewing a
patch reads the surrounding file body via file_read, so a grouped item's
context is dominated by member bodies. A group of small-diff/large-body files
(generated config, dashboards, lockfiles, fixtures) therefore passed the
diff-only budget, bundled into one review item, and overflowed at review time
("context compression exceeded its threshold") — failing every member at once,
even though those same files review cleanly on their own PRs where they land
in smaller groups.
Count each member's diff + NewFileContent (the body actually read) and split
an over-budget group to per-file review (the existing fallback shape).
reviewGroupBudgetFraction (0.55) reserves the rest of the per-item budget for
the review's own working set — reasoning, tool-call framing, and compression
summaries that share the context window.
Calibrated against an observed run: a 6-file group totaling ~97k body tokens
overflowed a 160k per-item ceiling, while ~77-80k groups reviewed cleanly.
Adds a regression test mirroring both cases.
Refs alibaba#1171
759dd67 to
62ff3d6
Compare
Fixes #1171.
Problem
enforceGroupTokenBudgetsplits a group only whensum(CountTokens(d.Diff))exceeds the limit — diff text only. But reviewing a patch reads the surrounding file body viafile_read, so a grouped item's context is dominated by member bodies, not diffs. A group of small-diff / large-body files (generated config, dashboards, lockfiles, fixtures) therefore sails past the diff-only budget, is bundled into one review item, and then overflows at review time — returningStopCompression("context compression exceeded its threshold") and failing every member at once. The same files review cleanly on their own PRs, where they land in smaller groups. (filterLargeDiffshas the same diff-only blind spot.)See #1171 for the full write-up and the reproduction.
Fix
groupReviewTokensnow counts each member'sDiffplusNewFileContent(the body the review actually reads).NewFileContentis already populated at parse time, so this adds no I/O.enforceGroupTokenBudgetsplits an over-budget group to per-file review — the existing fallback shape — and never tries to split a single-file group (a lone oversized body is handled by read-chunking + compression, not here).reviewGroupBudgetFraction = 0.55reserves the rest of the per-item budget for the review's own working set (reasoning, tool-call framing, compression summaries that share the context window with the bodies being read).Calibration
Measured with the project's own
CountTokens: a 6-file group totaling ~97k body tokens overflowed a 160k per-item ceiling, while groups of ~77–80k reviewed cleanly.0.55(≈88k of a 160k budget) splits the former with margin and keeps the latter — so it fixes the overflow without over-splitting healthy groups.Tests
Adds
grouping_body_budget_test.gomirroring both real cases:go build ./...,go vet ./internal/agent/,gofmt, and the fullinternal/agentsuite pass; existingenforceGroupTokenBudgettests are unchanged and still green.Notes
An alternative/complementary fix is to degrade a group to per-file after a
StopCompressionat review time; this PR takes the pre-dispatch path because it also avoids the wasted churn (and inflated request count) of reviewing a bundle that cannot fit. Happy to adjust the fraction or approach per maintainer preference.