Post-review should apply ready-for-merge when human approval already satisfies protected-path requirement

[fullsend-ai-retro]

What happened

On PR #243, rh-hemartin submitted an APPROVED review at 11:36:12 UTC on Jul 20, then triggered /fs-review at 11:36:18 UTC. The review agent run completed at 11:49:33 UTC with zero code findings — correctness, style, and intent-coherence sub-agents all cleared the change. The only finding was a medium [protected-path] flag because all 3 changed files are under scripts/. The review agent logged "First automated review dispatch but PR already has 1 human review(s)" — it was aware of the existing approval. The verdict was comment (downgraded from approve), and the post-script applied requires-manual-review. The PR was merged at 14:01 UTC, ~2.5 hours after the human had already approved, requiring additional human action to enqueue for merge.

What could go better

The post-review script (scripts/post-review.sh, lines 363–414) determines outcome labels using only the agent's verdict (ACTION), the DOWNGRADED flag, and the PR's draft status. It contains zero logic to check whether a human has already approved the PR — confirmed by reading the script. When the agent approves but is downgraded due to protected paths, the label is unconditionally requires-manual-review, even though a human APPROVED review on the current HEAD already satisfies the protected-path requirement. This creates a workflow paradox: human approves → triggers agent review → agent says "needs human approval" → human must act again. The protected-path policy exists to ensure humans review sensitive changes — and a human did review. The agent itself detected this but the post-script ignored it.

Confidence: high. The root cause is clearly visible in the script logic, the timeline is unambiguous, and the review agent's own logs confirm it knew about the existing approval. The related issue fullsend#1583 addresses request-changes downgrades (a different code path), not the approve-to-comment protected-path downgrade seen here.

Proposed change

In scripts/post-review.sh, before the outcome label if/elif block (around line 363), add a check: when DOWNGRADED=true (meaning the agent approved but protected paths were hit), query the PR's reviews via gh api repos/{owner}/{repo}/pulls/{number}/reviews and check for at least one APPROVED review from a non-bot user on the current HEAD SHA. If found, set the outcome to ready-for-merge instead of falling through to requires-manual-review. The protected-path finding should still appear in the review comment for transparency, but the label should reflect that the human-approval requirement is already met. Bot detection can use the user's type field (User vs Bot) from the GitHub API response.

Validation criteria

On the next 3 PRs where (1) the review agent's verdict is approve downgraded to comment due to protected paths, and (2) a human APPROVED review already exists on the current HEAD SHA, the post-script should apply ready-for-merge instead of requires-manual-review. The PR should proceed to merge queue without additional human action after the review agent completes. Add a test case to scripts/post-review-test.sh covering this scenario: approve + downgraded + existing human approval → ready-for-merge.

---

Generated by retro agent from #243

[fullsend-ai-triage]

🤖 Finished Triage · ✅ Success · Started 2:20 PM UTC · Completed 2:24 PM UTC
Commit: 02a0757 · View workflow run →

[fullsend-ai-triage]

Triage Summary

Severity: Medium · Category: Bug

Problem

The post-review script unconditionally applies requires-manual-review when the agent verdict is downgraded from approve to comment due to protected-path findings. It never checks whether a human has already submitted an APPROVED review on the current HEAD SHA. This creates a workflow paradox where human approval is required despite already being present — observed on PR #243 with a ~2.5 hour unnecessary delay.

Root Cause

The outcome-label block in scripts/post-review.sh (lines 363–414) uses only three inputs: ACTION, DOWNGRADED, and draft status. When DOWNGRADED=true, it falls through to requires-manual-review with no review-query logic.

Recommended Fix

In the post-review source file (post-review.src.sh), before the outcome-label block, add a check when DOWNGRADED=true: query gh api repos/{owner}/{repo}/pulls/{number}/reviews for an APPROVED review from a non-bot user (type=User) on the current HEAD SHA. If found, set the outcome to ready-for-merge. The protected-path finding should still appear in the comment for transparency.

After editing, run make script-build to regenerate the bundled script and commit both files together.

Related Work

• Review agent outputs approve despite detecting protected-path constraint #297 / PR fix(#297): add schema constraint rejecting approve with protected-path findings #303 — schema enforcement preventing the agent from outputting approve with protected-path findings (different layer — agent output vs post-script logic)
• fullsend#1583 — review agent re-flagging human-resolved protected-path findings (different scope — agent behavior vs post-script labels)

Neither blocks this fix; they are complementary at different layers.

Proposed Test Case

# In scripts/post-review-test.sh
# Test: approve + downgraded + existing human approval → ready-for-merge
# Setup: ACTION=approve, DOWNGRADED=true
# Mock gh api to return APPROVED review from type=User on current HEAD
# Assert: outcome label is ready-for-merge

# Edge cases:
# 1. Human approval on current HEAD → ready-for-merge
# 2. Human approval on stale SHA (not HEAD) → requires-manual-review
# 3. Only bot approvals on HEAD → requires-manual-review
# 4. No approvals at all → requires-manual-review

---

Labels: This is a bug in the review-agent post-script. Applies bug and review-agent labels. Sufficient for implementation so ready-to-code replaces ready-for-triage.

[fullsend-ai-coder]

🤖 Finished Code · ✅ Success · Started 2:25 PM UTC · Completed 2:36 PM UTC
Commit: 02a0757 · View workflow run →