Review agent should detect newly-unnecessary code paths after simplifying refactors

[fullsend-ai-retro]

What happened

In PR #1586, the author removed footnote formatting logic from FormatTaskName in status/format.go. After the removal, the function unconditionally returns name, nil — it no longer uses the test result at all. However, the function still calls GetTestResult() (which performs JSON unmarshaling and schema validation) even though its return value is no longer consumed.

The fullsend review agent (run 26946810360) approved the PR and correctly identified the behavioral change and the removal of FormatFootnotes, but did not flag the now-unnecessary GetTestResult() call. The gemini-code-assist bot caught this optimization in an inline review comment on status/format.go, suggesting the call could be removed for a performance improvement.

What could go better

The review agent should perform a "refactoring completeness" check: when a PR simplifies a function by removing code paths, verify that remaining calls, imports, and variable assignments within the changed function are still necessary. In this case, FormatTaskName was reduced to trivially returning its input, but still called an expensive helper whose result was discarded.

This is distinct from existing cross-file impact analysis issues (#1525) because the dead code is within the changed function itself — no cross-file reasoning is needed. The agent had full visibility of the simplified function and could have detected that GetTestResult() was called but its result unused in any remaining code path.

Confidence: Medium-high. The pattern is clear in this instance, but the frequency across other PRs is unknown. The impact per occurrence is low (minor performance improvement), but catching these improves review credibility and reduces follow-up cleanup PRs.

Proposed change

Add a review heuristic to the review agent's analysis prompt (in the review agent definition or review skill) that instructs it to check for refactoring completeness:

When a PR simplifies or removes logic from a function, verify that all remaining function calls, variable assignments, and imports within the changed scope are still consumed. Flag any calls whose return values are no longer used, or imports that are no longer referenced, as candidates for removal.

This should be scoped to changed functions only (not a full codebase scan) to keep token cost low. It could be framed as an optional [refactoring-completeness] finding category at info or low severity.

Validation criteria

On the next 5 PRs to konflux-ci/integration-service (or similar Go repos) that simplify functions by removing code paths, the review agent should flag at least one instance of a newly-unnecessary call or import that it would have previously missed. False positive rate should remain below 20% (i.e., at most 1 in 5 flagged items should be incorrect).

---

Generated by retro agent from konflux-ci/integration-service#1586

[fullsend-ai-triage]

🤖 Finished Triage · ✅ Success · Started 12:53 PM UTC · Completed 12:55 PM UTC
Commit: 4e21a60 · View workflow run →

[fullsend-ai-triage]

Triage Summary

This is a well-scoped feature request to add a refactoring-completeness heuristic to the review agent.

Problem: When a PR simplifies a function by removing logic branches, the review agent does not verify that remaining calls and assignments within the changed function are still necessary. In the cited example, FormatTaskName was reduced to trivially returning its input but still called GetTestResult() — an expensive operation whose return value was discarded.

Distinction from #1525: This is specifically about dead code within a changed function (no cross-file reasoning needed), whereas #1525 covers cross-file impact analysis gaps.

Proposed change: Add a prompt heuristic to the review agent that checks, when a PR simplifies a function, whether surviving calls/imports/assignments are still consumed. Scoped to changed functions only to keep token cost low, surfaced as info or low severity findings.

Validation criteria provided: On the next 5 PRs with simplifying refactors, the agent should catch at least one dead-code instance, with a false positive rate below 20%.

Test case (conceptual):
- Input: A PR that removes a code path from a function but leaves
  a function call whose return value is no longer consumed.
- Expected: The review agent produces a finding (info/low severity)
  identifying the now-unnecessary call as a candidate for removal.
- Edge cases:
  - Call has side effects (should NOT be flagged)
  - Multiple unused calls in the same function
  - Call result assigned to a variable that is still referenced
    in dead code (should be flagged)

Severity: Low — each missed instance is minor, but catching them improves review credibility.

---

Labels: Retro-generated feature request targeting the review agent's analysis capability, with low impact per occurrence.

[fullsend-ai-prioritize]

RICE Priority Score: 0.4

Score breakdown

Dimension	Score	Reasoning
Reach	1	The review agent operates across multiple repos and customers, so improved heuristics benefit all users. However, the specific pattern of simplifying refactors that leave dead code within the changed function is relatively infrequent, limiting the effective reach to a moderate number of users encountering this scenario.
Impact	0.5	The issue itself acknowledges 'impact per occurrence is low (minor performance improvement).' Each catch is a cleanup suggestion rather than a correctness fix. The main benefit is improving review credibility and reducing follow-up cleanup PRs, which is a nice-to-have rather than a significant pain point.
Confidence	0.8	High confidence — the issue is exceptionally well-described with a concrete example PR, clear proposed solution scoped to changed functions only, and explicit validation criteria with false-positive thresholds. The labeled priority/low aligns with the self-assessed severity.
Effort	1	Medium effort — requires modifying the review agent's analysis prompt or skill definition to add a refactoring-completeness heuristic. The proposed change is well-scoped (check within changed functions only), but needs careful handling of edge cases like side-effectful calls and testing against real PRs to validate the false-positive rate.

Formula: (1 x 0.5 x 0.8) / 1 = 0.4