Stop the review when a file read fails

[sergical]

The lead reviewer's prompt told it to continue with the diff hunks when read_file failed, so a run that read none of the files still approved. Now it stops and says so.

Created with Claude Code

[github-actions]

Verdict: Request changes — the new "stop and say so" instruction is likely to break the review pipeline on common, benign diffs (deleted/renamed files) and should be softened before merge.

Correctness

• apps/pr-reviewer/src/agents/review.ts, step 2 instruction: changing "If a read fails, continue with the diff hunks for that file" to "If a read fails, stop and say so" is a behavioral regression given how read_file actually works. read_file resolves paths against the current working tree (repoRoot), not against the diff's blob content, so it will routinely fail to read:

  • Deleted files (diff header targets /dev/null) — no file exists on disk to read.
  • Renamed files — the old path in diff --git a/old b/new won't exist at repoRoot once renamed.
  • Any path mismatch between the diff's ref and the checked-out state.

  These are completely normal occurrences in real PRs, not exceptional error conditions. With the new instruction, a single such file now halts step 2 for the entire diff review, preventing run_tests and post_review from ever running — even though the diff hunks (already loaded in step 1) are sufficient to review that one file. This is a disproportionate failure mode: one missing file blocks review of an otherwise-reviewable PR. The previous "continue with the diff hunks" fallback handled this gracefully; the new wording removes that fallback without providing an alternative path for deletions/renames.

  • Suggested fix: keep (or restore) a per-file fallback — e.g., "If a read fails, note it and continue with the diff hunks for that file," possibly special-casing deleted files (no reviewable file) vs. unexpected read errors (genuinely worth surfacing/stopping on).
  • Secondary note: read_file truncates large files to 20000 chars silently (no error), so the new strict "stop on failure" policy only guards hard read errors, not silent truncation — an inconsistent safety story if the intent is "always have full context before proceeding."

• run_tests ran with no related tests found/failing for this diff (prompt-text-only change, no related test files): {"passed":0,"failed":0,"ran":0,"note":"no related tests"}. No regressions surfaced, but this is expected since the change only touches instruction strings.

Style

• The phrase "stop and say so" is vague and informal compared to the precise, imperative tone of the surrounding numbered steps. It doesn't specify how the agent should "say so" — abort immediately, or flag it and still proceed to later steps like run_tests/post_review? This also seems to conflict with step 6's "Finish by replying with the verdict line only," since it's unclear whether a stop-and-say-so exit still goes through post_review.
  • Suggested rewording: something explicit and procedural, e.g., "If a read fails, record the file and error, and continue with the remaining files," or if a hard stop truly is intended, "If a read fails, skip delegation and publish a review via post_review explaining which file could not be read."

Summary

The diff is a single-line prompt wording change with no code/runtime logic touched, so blast radius is limited, but the new instruction actively steers the agent toward a brittle failure mode for routine diffs containing deletions or renames. Recommend revising the instruction text before merging.

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 8937d86. Configure here.}

[cursor]

Deleted files abort the review

Medium Severity

A failed read_file now aborts the whole review. Deleted and renamed paths still appear in the diff but are gone from the tree, so those reads fail and the review never finishes — including large.diff, which deletes a file.

 

^{Reviewed by Cursor Bugbot for commit 8937d86. Configure here.}

[linear-code]

MAR-1676