Review agent: detect stale-branch merge artifacts and suggest rebasing

[fullsend-ai-retro]

What happened

On PR #59, a docs-only PR accumulated 8 CHANGES_REQUESTED reviews over 2 days before being approved on the 9th review. The branch docs/fullsend-agents-walkthrough had not been rebased onto main, so the diff included .github/workflows/fullsend.yaml, CODEOWNERS, and .fullsend/customized/* — files that were already merged to main via other PRs. The review agent treated these as intentional changes and repeatedly flagged: workflow pin regression @1bf016d9 vs @v0 (7 times), CONTRIBUTOR auth scope (3 times), protected-path violations (6 times), and permissions breadth (2 times). Each push triggered a new review cycle that re-discovered the same stale-branch artifacts. Once the author rebased (reducing the diff to just config.yaml and docs/fullsend-agents.md), the bot approved on the very next run.

What could go better

The review agent currently treats every file in the PR diff as an intentional change by the author. It cannot distinguish between 'author modified this file' and 'this file appears in the diff because the branch is behind base.' This led to 8 wasted review cycles (significant token cost) and noise that obscured the 2 genuinely useful findings (missing fix role in config, GCP project IDs in docs). Existing issues cover adjacent problems: #1446 addresses out-of-diff findings, #1013 addresses dedup across iterations, and #1355 addresses skipping re-review on rebases. However, none address the specific scenario where the review agent could detect that diff files are merge artifacts and short-circuit to a 'please rebase' suggestion. Confidence: high — the evidence is clear from the 8 review bodies and their inline comments, which overwhelmingly targeted files that were not authored in this PR.

Proposed change

Add a pre-review check in the review agent that compares the PR diff file list against the merge-base. Specifically: before running the full review, compute git diff --name-only <merge-base>...<base-branch-head> to identify files that changed on the base branch after the PR branch diverged. If any files in the PR diff are also in this set (i.e., they're merge artifacts, not author changes), the review agent should: (1) exclude those files from severity-bearing findings, (2) add a single top-level comment suggesting the author rebase, and (3) not issue CHANGES_REQUESTED solely based on merge-artifact findings. This check could live in the review agent's pre-processing step or in the harness before dispatching the review. The implementation should be in the review agent definition or harness config in fullsend-ai/fullsend.

Validation criteria

On the next 3 PRs where the branch is behind base and the diff includes merge-artifact files: (1) the review agent should not file CHANGES_REQUESTED solely based on findings in merge-artifact files, (2) it should suggest rebasing in a single comment rather than filing individual findings per artifact file, and (3) total review cycles on stale-branch PRs should drop from 5+ to 1-2. Token cost per review cycle on such PRs should decrease proportionally.

---

Generated by retro agent from https://github.com/redhat-developer/rhdh-agentic/pull/59

[fullsend-ai-triage]

Triage Summary

Severity: Medium · Category: Feature

Problem

The review agent cannot distinguish merge artifacts from intentional author changes in the PR diff. When a branch falls behind base, the diff includes already-merged files, leading to repeated false findings and wasted review cycles (8 in the observed case).

Related Issues

Several open issues cover adjacent scenarios but not this specific pre-review detection:

• Out-of-diff findings should not escalate review verdict #1446 — Out-of-diff findings escalating verdict (different: about findings outside the diff, not merge artifacts within it)
• Review agent should deduplicate findings across iterations on the same PR #1013 — Dedup findings across iterations (complementary but doesn't prevent the initial false findings)
• Skip re-review when bot dep-bump PR is rebased with no semantic change #1355 / Skip or diff-gate re-review when PR changes are rebase-only #1287 / Skip re-review on merge-only commits with no code changes #1282 / Skip re-review when PR diff is unchanged after rebase/reopen #1356 — Skip re-review on rebase-only changes (different: about skipping subsequent reviews, not filtering the initial diff)

This issue is not a duplicate — it proposes a pre-review merge-artifact detection step that none of the above address.

Recommended Approach

Add a pre-review check comparing the PR diff file list against git diff --name-only <merge-base>...<base-head>. Files in both sets are merge artifacts. The agent should exclude them from findings, suggest rebasing in a single comment, and not issue CHANGES_REQUESTED solely from artifact findings.

Proposed Test Case

Given a PR with files A, B, C where A and B are merge artifacts:
1. Findings on A and B should be excluded from the review verdict
2. A single rebase-suggestion comment should be emitted
3. CHANGES_REQUESTED should not be issued if only A and B had issues
4. Genuine findings on C should still be reported

Edge cases:
- All files are artifacts → rebase suggestion only, no review
- No artifacts → normal review
- File modified by both author and base → should still be reviewed

---

Labels: Review agent feature affecting the dispatch/harness pre-review step, sourced from retro agent analysis, medium priority consistent with related issues.

[fullsend-ai-prioritize]

RICE Priority Score: 1.6

Score breakdown

Dimension	Score	Reasoning
Reach	2	Stale branches are a common scenario for all users of the review agent across orgs. Any PR where the branch falls behind base will trigger this problem, making it a broadly experienced issue affecting most active users.
Impact	1.5	Significant pain point: 8 wasted review cycles with substantial token cost and noise that obscured genuine findings. However, the workaround (rebasing) is well-known and effective, preventing this from being a blocker.
Confidence	0.8	High confidence — the issue is exceptionally well-documented with clear evidence from a real PR (8 review bodies, specific false findings enumerated), a well-scoped proposed solution, and thorough triage confirming it's not a duplicate of adjacent issues (#1446, #1013, #1355).
Effort	1.5	Moderate complexity: requires implementing a merge-base comparison in the pre-review step, modifying the review agent's dispatch logic to exclude artifact files, handling edge cases (all artifacts, mixed authorship), and testing across multiple PR scenarios.

Formula: (2 x 1.5 x 0.8) / 1.5 = 1.6

[maruiz93]

Workaround in use: /fs-fix rebase on main

People are already working around stale-branch PRs by using /fs-fix with a human instruction to rebase. Examples:

• conforma/cli#3329 — the fix agent successfully rebased a 41-file octal notation PR that had been open for 33 days, resolving a merge conflict in opa_evaluator_test.go. The retro agent noted ~25 minutes of additional compute and human re-review time.

This works because git rebase is listed in the fix agent's disallowedTools frontmatter (agents/fix.md), but disallowedTools has no effect in --agent sessions — it's completely inert (see #303 and ADR 0027). So the agent ignores the restriction when a human explicitly asks for a rebase.

If we want rebase to be a supported flow (whether via /fs-fix or a dedicated mechanism), git rebase should be removed from disallowedTools in both agents/code.md and agents/fix.md — otherwise, if #303 is implemented and disallowedTools entries become hard-enforced via permissions.deny, the rebase workaround would break.