Skip to content

Commit 62ff3d6

Browse files
committed
fix(agent): budget file groups by review body size, not just diff size
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 #1171
1 parent 04284b5 commit 62ff3d6

2 files changed

Lines changed: 134 additions & 13 deletions

File tree

internal/agent/grouping.go

Lines changed: 31 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,11 @@ import (
2121

2222
const maxFilesPerGroup = 10
2323

24+
// reviewGroupBudgetFraction is the share of the per-item token budget a group's
25+
// file bodies may occupy before it is split to per-file review; the remainder
26+
// covers the review's working set (reasoning, tool framing, compression).
27+
const reviewGroupBudgetFraction = 0.55
28+
2429
// smallChangeSetLabel labels the single group a below-threshold change set is
2530
// bundled into. Unlike an LLM-produced label it carries no semantics, because
2631
// no partition was computed: every file simply went in together.
@@ -332,26 +337,39 @@ func enforceMaxFilesPerGroup(groups []FileGroup) []FileGroup {
332337
return result
333338
}
334339

335-
// enforceGroupTokenBudget splits groups whose combined diffs exceed the token limit.
340+
// groupReviewTokens estimates the context a group's review must hold: each
341+
// member's diff plus its file body, which is read via file_read and is the term
342+
// the old diff-only budget missed.
343+
func groupReviewTokens(g FileGroup) int64 {
344+
var total int64
345+
for _, d := range g.Diffs {
346+
if d.IsDeleted {
347+
continue
348+
}
349+
total += int64(llm.CountTokens(d.Diff)) + int64(llm.CountTokens(d.NewFileContent))
350+
}
351+
return total
352+
}
353+
354+
// enforceGroupTokenBudget splits a group to per-file review when its estimated
355+
// review context (diffs + the bodies read to review them) exceeds the budget.
336356
func enforceGroupTokenBudget(groups []FileGroup, tokenLimit int) []FileGroup {
337357
if tokenLimit <= 0 {
338358
return groups
339359
}
360+
budget := int64(float64(tokenLimit) * reviewGroupBudgetFraction)
340361
var result []FileGroup
341362
for _, g := range groups {
342-
total := int64(0)
343-
for _, d := range g.Diffs {
344-
total += int64(llm.CountTokens(d.Diff))
345-
}
346-
if total <= int64(tokenLimit) {
363+
// A single-file group cannot be split further.
364+
if len(g.Diffs) <= 1 || groupReviewTokens(g) <= budget {
347365
result = append(result, g)
348-
} else {
349-
for _, d := range g.Diffs {
350-
result = append(result, FileGroup{
351-
Label: g.Label + " (split: " + d.NewPath + ")",
352-
Diffs: []model.Diff{d},
353-
})
354-
}
366+
continue
367+
}
368+
for _, d := range g.Diffs {
369+
result = append(result, FileGroup{
370+
Label: g.Label + " (split: " + d.NewPath + ")",
371+
Diffs: []model.Diff{d},
372+
})
355373
}
356374
}
357375
return result
Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
// SPDX-License-Identifier: Apache-2.0
2+
// Copyright 2026 alibaba/open-code-review Contributors
3+
4+
package agent
5+
6+
import (
7+
"strings"
8+
"testing"
9+
10+
"github.com/alibaba/open-code-review/internal/llm"
11+
"github.com/alibaba/open-code-review/internal/model"
12+
)
13+
14+
// bodyWithTokens returns text whose CountTokens is at least target. It measures
15+
// the repeat unit once and multiplies, avoiding an O(n^2) grow-and-recount loop.
16+
func bodyWithTokens(target int) string {
17+
const unit = "alert rule threshold reduce noDataState execErrState summary description\n"
18+
per := llm.CountTokens(unit)
19+
if per < 1 {
20+
per = 1
21+
}
22+
return strings.Repeat(unit, target/per+1)
23+
}
24+
25+
// smallDiffLargeBody is the pathological shape: a tiny patch against a large
26+
// file. The old diff-only budget saw only the diff and never split.
27+
func smallDiffLargeBody(path string, bodyTokens int) model.Diff {
28+
return model.Diff{
29+
NewPath: path,
30+
Diff: "@@ -1,1 +1,2 @@\n context\n+one changed line\n", // tiny
31+
NewFileContent: bodyWithTokens(bodyTokens),
32+
}
33+
}
34+
35+
func TestEnforceGroupTokenBudget_SplitsLargeBodySmallDiffGroup(t *testing.T) {
36+
const tokenLimit = 160000 // == PromptTokenLimit(200000)
37+
38+
// The real observability bundle that overflowed a 160k per-item ceiling:
39+
// six small-diff files whose measured body tokens total ~97k. Combined diff
40+
// is trivial, so the OLD diff-only budget kept them as one item.
41+
bundle := []struct {
42+
path string
43+
bodyTokens int
44+
}{
45+
{"grafana-alerts-dev.yaml", 35308},
46+
{"grafana-alerts-production.yaml", 35992},
47+
{"grafana-dashboard-journey-health.json", 11091},
48+
{"grafana-dashboard-router-pay.json", 6214},
49+
{"grafana-alert-contract.test.ts", 6056},
50+
{"grafana-router-pay-assets.test.ts", 2533},
51+
}
52+
var diffs []model.Diff
53+
for _, f := range bundle {
54+
diffs = append(diffs, smallDiffLargeBody(f.path, f.bodyTokens))
55+
}
56+
group := FileGroup{Label: "Grafana observability bundle", Diffs: diffs}
57+
58+
// Guard: the old diff-only measure would NOT have split this group.
59+
var diffOnly int64
60+
for _, d := range group.Diffs {
61+
diffOnly += int64(llm.CountTokens(d.Diff))
62+
}
63+
if diffOnly > int64(tokenLimit) {
64+
t.Fatalf("test setup: diffs alone (%d) must be under the limit to prove the body drives the split", diffOnly)
65+
}
66+
67+
got := enforceGroupTokenBudget([]FileGroup{group}, tokenLimit)
68+
if len(got) != len(diffs) {
69+
t.Fatalf("expected the over-budget bundle to split to %d per-file groups, got %d groups", len(diffs), len(got))
70+
}
71+
for _, g := range got {
72+
if len(g.Diffs) != 1 {
73+
t.Fatalf("expected each split group to hold exactly one file, got %d", len(g.Diffs))
74+
}
75+
}
76+
}
77+
78+
func TestEnforceGroupTokenBudget_KeepsWithinBudgetGroup(t *testing.T) {
79+
const tokenLimit = 160000
80+
// The real 4-file subset that reviewed cleanly as one group on its own PR:
81+
// the two big alert YAMLs plus their two tests, ~80k body tokens total.
82+
group := FileGroup{Label: "keep", Diffs: []model.Diff{
83+
smallDiffLargeBody("grafana-alerts-dev.yaml", 35308),
84+
smallDiffLargeBody("grafana-alerts-production.yaml", 35992),
85+
smallDiffLargeBody("grafana-alert-contract.test.ts", 6056),
86+
smallDiffLargeBody("grafana-router-pay-assets.test.ts", 2533),
87+
}}
88+
got := enforceGroupTokenBudget([]FileGroup{group}, tokenLimit)
89+
if len(got) != 1 || len(got[0].Diffs) != 4 {
90+
t.Fatalf("within-budget group must stay intact, got %d groups", len(got))
91+
}
92+
}
93+
94+
func TestEnforceGroupTokenBudget_NeverSplitsSingleFile(t *testing.T) {
95+
const tokenLimit = 160000
96+
// A lone oversized file cannot be split further; leave it for the
97+
// read-chunking + compression path, not the grouping split.
98+
group := FileGroup{Label: "solo", Diffs: []model.Diff{smallDiffLargeBody("huge.json", 200000)}}
99+
got := enforceGroupTokenBudget([]FileGroup{group}, tokenLimit)
100+
if len(got) != 1 || len(got[0].Diffs) != 1 {
101+
t.Fatalf("single-file group must not be split, got %d groups", len(got))
102+
}
103+
}

0 commit comments

Comments
 (0)