Review agent: do not auto-apply ready-for-merge label to fork PRs from new contributors

[fullsend-ai-retro]

What happened

On PR #1202, an external contributor (GHX5T-SOL) opened a fork-based PR for issue #1148. The account had zero prior contributions to the repo, the fork was created minutes before the PR, and the commit was authored under a pseudonym. The review bot approved the PR with "No findings" and applied the ready-for-merge label within 5 minutes of PR creation. Two days later, a second external contributor (resolvicomai) opened PR #1277 for the same issue, and the review bot again approved and labeled it ready-for-merge. Neither PR was merged. The maintainer closed both and triggered /fs-code to have the code agent redo the work.

The ready-for-merge label signals to maintainers that a PR is safe to merge. Applying it automatically to fork PRs from unknown contributors undermines this signal — the label implies trust that hasn't been established.

What could go better

The review bot should distinguish between PRs from trusted sources (org members, known contributors, the code agent itself) and PRs from unknown external contributors when deciding whether to apply the ready-for-merge label.

Confidence: High. This happened on two separate PRs in the same week. The maintainer's action (closing both and re-triggering the code agent) is a clear signal that the bot's approval alone was insufficient for these PRs. The repo's own threat model prioritizes external injection risks, which makes auto-approving unknown external code particularly concerning.

What I could be wrong about: The maintainer may have closed the PRs for reasons unrelated to contributor trust (e.g., preferring agent-generated code for consistency, or subtle code quality issues). However, the pattern of closing approved-and-labeled PRs without comment suggests the label itself was premature.

Proposed change

Modify the review agent's post-review logic (in the pr-review skill or the review workflow's post-processing) to check whether the PR is from a fork and whether the author has prior merged PRs in the repo before applying the ready-for-merge label.

Specifically, in the pr-review skill (skills/pr-review/SKILL.md in the scaffold or .fullsend repo):

1. After completing the code review, check pr.head.repo.fork (or equivalent) to determine if the PR is fork-based.
2. If fork-based, check contributor history: gh api repos/{owner}/{repo}/commits?author={pr_author}&per_page=1 to see if the author has any prior merged commits.
3. If the author is a first-time contributor from a fork, do NOT apply ready-for-merge. Instead, add an informational note in the review comment: "This PR is from a first-time contributor. A maintainer should verify before merging."
4. The review verdict (approve/request changes) should still be based on code quality — only the label application changes.

This keeps the review bot's code analysis useful while preventing it from signaling merge-readiness for untrusted sources.

Validation criteria

Over the next 10 fork-based PRs from first-time contributors:

• The ready-for-merge label should NOT be auto-applied to any of them.
• The review comment should include a note about first-time contributor status.
• PRs from org members or contributors with prior merged commits should still receive ready-for-merge when approved.
• No regression in review quality for internal PRs.

---

Generated by retro agent from #1202

[fullsend-ai-triage]

Triage Summary

Severity: High | Category: Security

This retro finding identifies a gap in the review agent's post-review logic: the ready-for-merge label is applied unconditionally when the review verdict is "approve" with no downgrade, without checking whether the PR author is a trusted contributor.

Root Cause

The label application logic in internal/scaffold/fullsend-repo/scripts/post-review.sh (around lines 145–150) applies ready-for-merge based solely on the review disposition. It does not check whether the PR is fork-based or whether the author has prior merged contributions.

Recommended Fix

1. Before applying ready-for-merge, check if the PR is fork-based (pr.head.repo.fork).
2. If fork-based, query the author's commit history (gh api repos/{owner}/{repo}/commits?author={author}&per_page=1).
3. If the author is a first-time fork contributor, skip ready-for-merge and add a note to the review comment indicating maintainer verification is needed.
4. Update post-review-test.sh with new test cases.

Proposed Test Cases

# In post-review-test.sh:

# 1. Fork PR from new contributor — should NOT get ready-for-merge
test_approve_fork_new_contributor:
  ACTION=approve, DOWNGRADED=false, IS_FORK=true, HAS_PRIOR_COMMITS=false
  Expected: label != ready-for-merge, review comment includes first-time contributor note

# 2. Fork PR from known contributor — should get ready-for-merge
test_approve_fork_known_contributor:
  ACTION=approve, DOWNGRADED=false, IS_FORK=true, HAS_PRIOR_COMMITS=true
  Expected: label == ready-for-merge

# 3. Non-fork PR — should get ready-for-merge (no regression)
test_approve_non_fork:
  ACTION=approve, DOWNGRADED=false, IS_FORK=false
  Expected: label == ready-for-merge

This change aligns with the repo's threat model, which prioritizes external injection risks above all other threat categories.

---

Labels: This is a security-related issue about the review agent's label behavior in the scaffold scripts, matching agent/review, security, area/scaffold, and priority/high labels.