Review agent first-pass missed dead-code finding that second-pass caught

[fullsend-ai-retro]

What happened

On PR #1348, the review agent's first run (commit 5ae39f2, 22:41 UTC) returned "No findings." The second run (commit 6de3ac6, 22:51 UTC) identified that ListPullRequestFiles at internal/forge/forge.go:216 became dead code. Commit 1 replaced the call site in submitFormalReview with the new ListPullRequestFileDiffs, so the dead code should have been detectable on the first pass. The second commit only added empty-patch handling and a HasPrefix fast-path — it did not change the call site.

What could go better

The review agent's analysis quality was inconsistent between runs on essentially the same change. The dead-code observation (the only finding across both runs) should have been caught on the first pass, since commit 1 is where ListPullRequestFiles lost its only caller. This inconsistency is likely due to LLM non-determinism rather than a systematic bug, but it reduces trust in "No findings" verdicts. If the review agent had caught this on the first pass, it could have informed the author before they pushed the second commit. Confidence: medium — I cannot confirm the exact diff of commit 1 vs commit 2, so the dead code may have been ambiguous on the first commit if the old method was still referenced in tests.

Proposed change

Consider adding a deterministic dead-code detection step to the review agent's analysis pipeline — for example, running go vet or a lightweight unused-code linter (like deadcode or staticcheck) as a pre-analysis step whose output is fed into the LLM review context. This would make dead-code detection reliable rather than dependent on LLM attention. This could be added as a review agent skill or a pre-review hook in the harness configuration. Relevant files: the review agent definition and the pr-review / code-review skills.

Validation criteria

On the next 5 PRs that introduce dead code in Go files, the review agent consistently identifies it on the first pass. Alternatively, if a linter step is added, verify it runs successfully and its output appears in the review agent's context.

---

Generated by retro agent from #1348

[fullsend-ai-triage]

Triage Summary

This is a well-defined feature request to improve the review agent's reliability for detecting dead code in Go files.

Problem: The review agent's dead-code detection depends entirely on LLM attention, which is non-deterministic. On PR #1348, the first pass returned "No findings" but the second pass correctly identified dead code that was already present after the first commit.

Proposed solution: Add a deterministic linter step (e.g., deadcode, staticcheck) to the review agent's Go analysis pipeline, feeding its output into the LLM review context. This could be a skill or a pre-review hook.

Related issues:

• Review agent should attempt Go compilation as a basic validation step #1345 — Go compilation as a validation step (complementary, not duplicate)
• Review bot should align approval status with finding severity on first pass #1319 — Approval status alignment on first pass (different aspect of first-pass quality)

Proposed test case:

Scenario: Dead-code detection on first review pass

Given: A PR that removes the last call site of a Go function
  And: The function definition remains in the codebase
When: The review agent runs its first pass
Then: The linter step identifies the unused function
  And: The finding appears in the review agent's output

Edge cases to cover:
- Function used only in tests (should not flag)
- Function used via reflection or interface satisfaction
- Multiple dead functions in the same PR
- Function in a different package from the removed call site

Severity: Medium — findings are eventually caught but delayed feedback reduces trust in first-pass verdicts.

---

Labels: Review agent feature request about adding linter-based analysis to the review pipeline, matching agent/review, agent/retro (filed by retro agent), area/skills, and priority/medium conventions.