alibaba / alibaba/open-code-review

file_read_diff output is unbounded and can exhaust the LLM context window

Open
#981 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
24.4k
Forks
1.8k
Avg merge
2d 6h
Merged PRs (30d)
105

Description

## OpenCodeReview version

Built from source at `ec5f6851d2fb363ec36b9f283910145ef87cf5df` (`upstream/main` on 2026-08-17). The installed CLI used to inspect the environment is `v1.9.3 (c35ddd72)`.

## Operating system

macOS (Apple Silicon), Darwin 25.5.0 arm64.

## Installation method

Built from source.

## LLM provider

Provider-independent. This report covers the size of the tool result before it is sent to an LLM.

## Bug description

`file_read_diff` concatenates the complete diff for every requested path without any output budget, truncation marker, or continuation mechanism. A sufficiently large result can consume or exceed the context available to the next LLM request and interrupt the review loop.

The current implementation appends each complete diff to one `strings.Builder` and returns it directly:

- [`internal/tool/file_read_diff.go`](https://github.com/alibaba/open-code-review/blob/ec5f6851d2fb363ec36b9f283910145ef87cf5df/internal/tool/file_read_diff.go#L54-L73)
- [`file_read_diff` tool schema](https://github.com/alibaba/open-code-review/blob/ec5f6851d2fb363ec36b9f283910145ef87cf5df/internal/config/toolsconfig/tools.json#L160-L182), which exposes only `path_array` and no pagination parameter

This differs from other read-oriented tools such as `file_read`, which limits output and reports `IS_TRUNCATED`.

## Steps to reproduce

1. Save the following temporary test as `internal/tool/file_read_diff_repro_test.go` at commit `ec5f6851d2fb363ec36b9f283910145ef87cf5df`:

```go
// SPDX-License-Identifier: Apache-2.0
// Copyright 2026 alibaba/open-code-review Contributors

package tool

import (
"context"
"fmt"
"strings"
"testing"
)

func TestReproFileReadDiffUnboundedOutput(t *testing.T) {
const (
files = 10
changedPairs = 10000
)

diffs := make(map[string]string, files)
paths := make([]any, 0, files)
for fileIndex := 1; fileIndex <= files; fileIndex++ {
path := fmt.Sprintf("file-%02d.go", fileIndex)
var diff strings.Builder
fmt.Fprintf(&diff, "--- a/%s\n+++ b/%s\n@@ -1,%d +1,%d @@\n", path, path, changedPairs, changedPairs)
for line := 1; line <= changedPairs; line++ {
fmt.Fprintf(&diff, "-old line %06d from file %02d\n+new line %06d from file %02d\n", line, fileIndex, line, fileIndex)
}
diffs[path] = diff.String()
paths = append(paths, path)
}

provider := NewFileReadDiff(NewDiffMap(diffs))
result, err := provider.Execute(context.Background(), map[string]any{"path_array": paths})
if err != nil {
t.Fatal(err)
}

t.Logf("output_bytes=%d output_lines=%d", len(result), 1+strings.Count(result, "\n"))
if strings.Contains(result, "IS_TRUNCATED") || strings.Contains(result, "NEXT_OFFSET") {
t.Fatal("unexpected truncation or continuation metadata")
}
}
```

2. Run the repository-required unit-test entry point:

```shell
make test
```

3. Observe that `TestReproFileReadDiffUnboundedOutput` reports `output_bytes=6000860 output_lines=200051`, with neither truncation nor continuation metadata.
4. Remove the temporary test file after reproducing the issue.

Each synthetic changed-line pair has one removed line and one added line. The following table reports raw bytes and lines; it does not estimate tokens.

| Files | Changed line pairs per file | Input bytes | Output bytes | Output lines | Truncation marker | Paging metadata |
|---:|---:|---:|---:|---:|:---:|:---:|
| 1 | 10,000 | 600,058 | 600,086 | 20,006 | No | No |
| 3 | 10,000 | 1,800,174 | 1,800,258 | 60,016 | No | No |
| 10 | 10,000 | 6,000,580 | 6,000,860 | 200,051 | No | No |
| 10 | 50,000 | 30,000,580 | 30,000,860 | 1,000,051 | No | No |

All requested file markers were present in every result. The output grew linearly with the complete requested diff size; the only overhead was the per-file header and trailing newline.

This is a deterministic source-level reproduction. It does not claim that a specific LLM provider was observed returning a context-overflow error, and byte size is not treated as token count.

## Logs / Error output

N/A. This is a deterministic source-level reproduction and does not call an LLM provider.

## Expected behavior

`file_read_diff` should have a deterministic response budget. When the complete result does not fit:

1. the response should explicitly state that it is partial;
2. omitted content should remain retrievable through a follow-up call; and
3. tests should verify the output bound and continuation behavior for both a single large diff and multiple requested files.

## Proposed direction

Keep the existing `path_array` behavior compatible, add a bounded response with explicit partial-result metadata, and expose a cursor or offset that lets the model retrieve the remainder. The exact budget unit, limit, and continuation schema can be agreed before implementation rather than hard-coded in this report.

## Additional context

The existing `file_read_diff` tests cover small single-file and multi-file results but do not assert any response-size bound. I am happy to implement the agreed behavior and add focused unit tests.

Contributor guide

Open the contributing guide

Research direction

Start with internal/tool/file_read_diff.go and the existing file_read_diff tests; run make test and the reproduction described in the issue to confirm current unbounded output. Review internal/config/toolsconfig/tools.json alongside the implementation before agreeing the response budget and continuation schema. Done means bounded output with explicit partial metadata, retrievable remainder, and focused tests for one large diff and multiple paths.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
tooling
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.