docs: ADR 0071 — Auto-merge#2791
Conversation
Proposes a separate fullsend-ai-merge app with contents:write so bot approvals count toward branch protection, combined with blank-owner CODEOWNERS entries for dependency files. Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
PR Summary by QodoADR 0062: Opt-in auto-merge via merge app identity + CODEOWNERS exemptions
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Site previewPreview: https://70538298-site.fullsend-ai.workers.dev Commit: |
|
🤖 Review · ❌ Terminated · Started 5:42 PM UTC · Ended 5:49 PM UTC |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1.
|
|
🤖 Finished Review · ❌ Failure · Started 5:42 PM UTC · Completed 5:49 PM UTC |
|
/fs-review |
|
🤖 Finished Review · ❌ Failure · Started 6:30 PM UTC · Completed 6:46 PM UTC |
Reframe the auto-merge decision: instead of a standalone merge agent, add a new app identity (fullsend-ai-merge) and teach the existing review agent to auto-merge behind REVIEW_AUTO_MERGE. This is the more reversible path — standing up a dedicated agent later is not made harder by this decision. Also links Context to problem docs per ADR conventions. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 6:07 PM UTC · Completed 6:18 PM UTC |
|
Code review by qodo was updated up to the latest commit 5921a16 |
ReviewFindingsHigh
Medium
Low
Previous runReviewFindingsHigh
Medium
Low
Previous runReviewFindingsHigh
Medium
Low
Previous run (2)Review —
|
0062 is taken on main by dispatch-version-skew.md. Renumber to 0071, the next available number considering main and open PRs. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 6:32 PM UTC · Completed 6:48 PM UTC |
- Complete the sentence fragment in the reversibility rationale - Zero-pad ADR cross-references to match established convention - Add token handoff section: merge app token minted when REVIEW_AUTO_MERGE is set, fail-hard on misconfiguration - Add security considerations: credential isolation, prompt injection blast radius, CODEOWNERS bypass scope, env var delivery protection - Note that implementation in review.yaml and post-review.sh is deferred to a follow-up PR Signed-off-by: Ralph Bean <rbean@redhat.com> Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Review · |
The review agent moved to fullsend-ai/agents, but auto-merge touches core architectural concerns (app identity boundaries, credential isolation, CODEOWNERS policy) that belong in the central ADR corpus. Signed-off-by: Ralph Bean <rbean@redhat.com> Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Review · ❌ Terminated · Started 7:11 PM UTC · Ended 7:27 PM UTC |
|
Ran an experiment to validate the assumptions here: fullsend-ai/experiments#40 Set up two throwaway GitHub Apps against |
| When `REVIEW_AUTO_MERGE` is set, the harness mints a token from the | ||
| `fullsend-ai-merge` app instead of `fullsend-ai-review`. If the merge app is | ||
| not installed on the repo, the agent fails hard — this is a misconfiguration. | ||
| Normal review operations (reading code, posting comments) do not change; only |
There was a problem hiding this comment.
[medium] incorrect-security-claim
The claim 'The review agent never holds both the merge token and a provider credential simultaneously' is misleading given the current review.yaml architecture where GH_TOKEN and GCP credentials coexist in the sandbox. The Token Handoff section implies full token replacement, which would put the merge token in the sandbox alongside provider credentials.
Suggested fix: Clarify which token flows where: state that the merge token is consumed only by the post-script outside the sandbox, or remove the 'never holds both simultaneously' claim.
| ### D. Exempt dependency files via blank-owner CODEOWNERS entries | ||
|
|
||
| Use [blank-owner entries](https://github.com/orgs/community/discussions/23064) | ||
| to remove the CODEOWNERS requirement for specific files. Orthogonal to the app |
There was a problem hiding this comment.
[low] internal-consistency
The ADR introduces REVIEW_AUTO_MERGE as a new env var, but the codebase already has an auto_merge boolean field in internal/config/config.go (RepoDefaults struct). The ADR does not mention this existing config mechanism.
|
🤖 Finished Review · ✅ Success · Started 7:11 PM UTC · Completed 7:27 PM UTC |
Summary
fullsend-ai-mergeGitHub App for auto-mergecontents: write— our review bot hascontents: read, so its approvals are currently informational onlyLooking for team feedback on the approach before implementation.
Test plan
contents: writeto the review app