alibaba / alibaba/open-code-review

file_read tool accepts paths not in the current commit, driving review to status: partial

Open
#989 0 comments 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

- Reproduced on `v1.9.4 (f7344e79) windows/amd64` (production)
- Also affected on `v1.9.5 (ea1de41a windows/amd64)` (local)
- Installed via `npm install -g @alibaba-group/open-code-review`

## Operating System

Windows (Jenkins agent on Windows Server; reproducible from any platform)

## Installation Method

npm (global)

## LLM Provider / Model

`deepseek` / `deepseek-v4-flash` (also seen to reproduce on other providers per #287)

## Bug description

During `ocr review --commit ` on a focused change, the LLM independently calls its `file_read` tool with paths that **do not exist in the commit at all**. OCR executes the read via `git show :`, git returns "fatal: path ... does not exist in ", and the failed tool call is reported back to the LLM. When the hallucination is severe enough (or when the LLM is persistent), the review run goes from `status: complete` to `status: partial`, and the Jenkins pipeline fails OCR Review.

Three consecutive failed builds on the same Gerrit Change 4700 (different patchsets rebased on top of each other) all failed with the same root cause, in slightly different ways:

| Patchset | Commit SHA | Hallucinated path (file_read) | File actually exists? |
|---|---|---|---|
| 1 | `b087039` | `DOC/云配置项/project.yct` | **Never existed** (path entirely invented by LLM) |
| 4 | `d86b0f1` | `App/MID/RF/CendricApp/src/phscaCendricCadsApp.c` | **Deleted in this commit** (file lived in history but this PR removed the entire Cendric tree) |
| 4 | `d86b0f1` | `App/MID/RF/CendricSwTimer/src/phscaCendricCadsSwTimer.c` | **Deleted in this commit** (same) |

The hallucination has two distinct mechanisms:

1. **Imagined paths** that were never in the repo (e.g. `DOC/云配置项/project.yct` — possibly extrapolated from the commit subject's mention of "切换 RF 芯片").
2. **Deleted-in-this-commit paths** that the LLM remembers from the project's history / training data (e.g. Cendric legacy files — this commit removes the Cendric tree, but the LLM still calls file_read on its members).

OCR executes the read either way, and each call that fails contributes to driving the run into `partial`.

## Steps to reproduce

The exact conditions are LLM-specific, but a high-rate reproduction recipe:

1. Pick a focused commit that touches a small number of files in one area, AND that **removes or renames** other files elsewhere in the repo.
2. Run `ocr review --commit --provider deepseek --model deepseek-v4-flash --format json --timeout 30` (or any comparable review).
3. After ~5–10 minutes, observe:
- `status: partial` instead of `status: complete`
- In the logs: `✘ file_read failed: file "" not found: git show :: ... fatal: path '' does not exist in ` — one or more such entries
- No `code_comment` tool call on the actually-changed files; the LLM has burned the LLM budget retrying the missing file or pivoted away

A simpler deterministic repro: hand-craft a prompt that says "review this commit" while seeding the LLM with a "looks like a project config file at `DOC/云配置项/project.yct>"-style misdirection. The model will call `file_read("DOC/云配置项/project.yct")` and OCR will fail it.

## Expected behavior

The `file_read` tool is supposed to be a fallback for the LLM to inspect a file it has been told about (typically the changed files in the diff, or files referenced by changed code). It is **not** supposed to allow free-form exploration of unrelated paths in the commit's history.

Desired properties:

- `file_read` accepts only paths that **actually exist in the current commit** (the diff's `Ref`); a non-existent path is rejected up-front, not after a wasted `git show` call.
- Optionally, `file_read` is constrained to the changed files in the diff (or to a small allowlist provided by the user via `--include`). A focused `ocr review` of a 10-file commit should never let the model fetch a 200-file tree.
- The `--exclude` / `--path` family of flags already exists for `ocr scan`; review mode should expose the same controls (`--include` for review, or a path allowlist knob).
- When a `file_read` call is rejected, OCR should report it as a deterministic structural error, not a transient failure that the retry loop can re-attempt. (Today, a hallucinated path makes `status: partial` because the retry treats it as flaky.)
- `existing_code` matching and the diff/filter pipeline should not be perturbed; the constraint is purely on the input side.

## Proposed direction

Add a path-validity check on `file_read` (and the related read-oriented tools) before the git call, against the same `Ref` the read is intended to resolve into. Two reasonable shapes:

1. **Hard allowlist**: read paths must be a subset of the changed files in the diff (or a user-supplied `--include` set). Anything else returns a clean "path is not in this review scope" and is not retried.
2. **Soft guard**: read paths must be a path that exists at the ref. If it does not, return a one-line "no such file" error and do not contribute to the partial status (or trigger retry).

Either shape kills the hallucination-driven partial-status path. The hard allowlist is closer to what `ocr scan --path` already supports; the soft guard is a smaller change.

A related improvement, separately tracked in #981, is bounding `file_read_diff` output size; the two are complementary and should not be conflated.

## Acceptance criteria

- [ ] A `file_read` request for a path that does not exist in the current commit's tree returns a deterministic structural error and is **not** retried by the partial-status retry loop.
- [ ] A `file_read` request for a path that is not in the diff's changed files (and not in a user-supplied allowlist) is rejected with a clear "out of review scope" message.
- [ ] The 3 reproduction patterns above (imagined path, deleted-in-commit path) no longer drive `ocr review` to `status: partial`.
- [ ] Normal review flow on focused commits is unchanged (existing legitimate reads still work).
- [ ] A focused test exercises each reproduction pattern and asserts the review run ends in `status: complete` with the expected comment set.

## Affected Area

Built-in Tools (file_read, code_search, etc.); review pipeline (partial-status retry policy).

## See also

- **#287** — `file_read` call failures in a monorepo (closed as completed, no functional fix shipped). Different root cause (relative path resolution in subdirectory) but the same symptom: `file_read` produces a "fatal: path ... does not exist" message, and the run goes partial.
- **#981** — `file_read_diff` output is unbounded and can exhaust the LLM context window. Same tool family, different overflow mode.
- **#167** — Near-duplicate comments with wrong filenames. Different category but tangentially related to model/filename mismatches.

## Logs

Sample (Patchset 4, v1.9.4, `deepseek-v4-flash`):

```
11:14:40 [OCR] attempt 1/2 status=partial elapsed=541s, retrying…
11:15:18 [ocr] ✘ file_read failed: file "App/MID/RF/CendricApp/src/phscaCendricCadsApp.c" not found: git show d86b0f1:App/MID/RF/CendricApp/src/phscaCendricCadsApp.c: exit status 128: fatal: path 'App/MID/RF/CendricApp/src/phscaCendricCadsApp.c' does not exist in 'd86b0f1'
11:15:18 [ocr] ✘ file_read failed: file "App/MID/RF/CendricSwTimer/src/phscaCendricCadsSwTimer.c" not found: git show d86b0f1:App/MID/RF/CendricSwTimer/src/phscaCendricCadsSwTimer.c: exit status 128: fatal: path 'App/MID/RF/CendricSwTimer/src/phscaCendricCadsSwTimer.c' does not exist in 'd86b0f1'
11:24:25 [ERROR] OCR 未完整结束,status=partial
```

Contributor guide

Open the contributing guide

Research direction

Trace the file_read and related read-oriented tools from the ocr review entry point, then follow the review pipeline's partial-status retry policy. Compare review's path handling with the existing --exclude/--path controls in ocr scan and inspect the focused tests around these flows. Done means invalid or out-of-scope paths produce deterministic errors without retries, while legitimate reads and expected comments remain unchanged.

Written by the indexing model from the issue text.

Assessment

Tech stack
git, go
Domain
cli, devtools
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.