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.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
🤖 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 —
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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>
|
🤖 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 |
|
I do think that having a variable on the review agent, or somehwere else, and have the review agent set "merge when ready" when it approves the PR makes sense. The other options look more difficult to setup and not that intuitive. |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-squad pass (3 independent reviewers: claude-coder, claude-researcher, grok-review-agent) focused on ADR internal consistency, contradictions with other ADRs/living docs, and premature/unverified claims rather than prose style. All findings below were independently verified against the current file content, the repo's own skills/writing-adrs/SKILL.md / AGENTS.md conventions, referenced ADRs, and live repo state (including the GitHub ruleset API) before posting. Several items already raised or fixed in prior review rounds (sentence fragment, ADR 0007 formatting, token handoff, security considerations section, implementation deferral, docs/architecture.md gap, the two-decisions-in-one-ADR structure, and the existing 'never holds both tokens simultaneously' claim) were confirmed already addressed or already tracked and are intentionally not repeated here.
|
|
||
| ### Token handoff | ||
|
|
||
| When `REVIEW_AUTO_MERGE` is set, the harness mints a token from the |
There was a problem hiding this comment.
[HIGH] contradicts-adr-0020
This Token Handoff mechanism has the same review agent process operate under a different, more privileged GitHub App identity (fullsend-ai-merge, with contents: write + pull_requests: write) depending on the REVIEW_AUTO_MERGE runtime setting. ADR 0020 (Accepted, not superseded) states as an explicit, currently-in-force consequence: "Adding a new capability to a stage means adding a new agent, skill, and sandbox policy — not modifying an existing agent's permissions." This ADR's own Option B ("create a separate merge agent") is the ADR-0020-consistent path and is considered, but the rejection of it (in favor of Option C, on reversibility grounds) never engages with ADR 0020 at all. That's worth calling out because the review agent is the one agent in the pipeline that processes attacker-influenced input (PR diffs/descriptions) — exactly the actor ADR 0020's isolation principle was written to constrain.
Suggestion: Add a line acknowledging the departure from ADR 0020's "new capability → new agent" pattern and why an exception is warranted here (reversibility vs. blast-radius tradeoff), or reconsider whether Option B is actually the more architecturally consistent choice.
(Independently verified against the current text of ADR 0020's Consequences section.)
| arbitrary PRs — branch protection (required reviewers, status checks) still | ||
| gates the actual merge. The blast radius is one PR, same as today. | ||
| - **CODEOWNERS bypass scope.** Blank-owner entries only remove the code-owner | ||
| review requirement for the listed paths (`go.mod`, `go.sum`). CI status |
There was a problem hiding this comment.
[HIGH] premature-decision — codeowners-bypass-contradicts-threat-model
This bullet states as settled fact that "CI status checks remain the primary safety gate" once code-owner review is bypassed for go.mod/go.sum. But this ADR's own relates_to lists security-threat-model, and that doc's "Threat 4: Supply chain attacks" recommends the opposite of what's happening here: dependency-update PRs "should be treated with the same scrutiny as external PRs," and "major version bumps or new dependencies should require higher scrutiny" — it also explicitly notes a compromised dependency PR can pass all tests/CI cleanly. Removing code-owner review from exactly these files and relying on CI as the primary gate runs counter to the ADR's own cited threat model, and the ADR doesn't explain why that's acceptable here. (The blank-owner CODEOWNERS mechanics were validated in the linked experiment — this finding is about the scrutiny-level tradeoff, not whether the mechanism technically works.)
Suggestion: Either engage directly with Threat 4's recommendation (explain why CI-only is sufficient despite it), or narrow the exemption to no-behavior-change dependency bumps, matching the threat model's own suggested distinction.
(Flagged independently by 2 of 3 reviewers.)
|
|
||
| 2. **Add auto-merge behavior to the review agent**, gated behind | ||
| `REVIEW_AUTO_MERGE`. When the var is set and the review verdict is | ||
| `approve`, the post-script enables GitHub auto-merge on the PR. No new |
There was a problem hiding this comment.
[MEDIUM] premature-decision — merge-queue-incompatible
"the post-script enables GitHub auto-merge on the PR" reads as GitHub's native auto-merge toggle (gh pr merge --auto / enablePullRequestAutoMerge). This repo's own main branch ruleset has an active merge_queue rule, and this repo's own skills/merge-queue/SKILL.md documents exactly this incompatibility: "GitHub's auto-merge API (gh pr merge --auto) does not work with merge queues" — await-and-enqueue.sh (the enqueuePullRequest GraphQL mutation) is the documented substitute. As written, the described mechanism would silently no-op on any merge-queue-enabled repo, which includes this org's own fullsend repo.
Suggestion: Note that merge-queue-enabled repos require enqueueing via the GraphQL mutation rather than the plain auto-merge toggle, and add this as an explicit branch in the deferred implementation scope.
|
|
||
| ### Implementation deferral | ||
|
|
||
| Implementation of `REVIEW_AUTO_MERGE` in `review.yaml` and `post-review.sh` |
There was a problem hiding this comment.
[MEDIUM] stale-living-docs
Two other living docs will be inaccurate once this ADR merges as Accepted, and neither is in this PR's diff or mentioned in this deferral section:
docs/glossary.md's "Automerge" entry currently states: "The team has explicitly decided not to implement automerge in the MVP; agents will comment that they approve, but a human must merge." This ADR decides the opposite.docs/roadmap.md:177still references "ADR 0062" for this exact PR ("...ADR 0062 (#2791)"). This ADR was renumbered 0062→0071 in commit 50f5e87 afterdocs/roadmap.mdwas last touched, so that link now reads as pointing to the unrelated, already-merged0062-dispatch-version-skew.md.
Suggestion: Update both in this PR (small, mechanical edits) or add them to this Implementation deferral section as explicit follow-up scope so they aren't forgotten.
(Both points independently raised by all 3 reviewers.)
| title: "71. Auto-merge" | ||
| status: Accepted | ||
| relates_to: | ||
| - agent-architecture |
There was a problem hiding this comment.
[MEDIUM] relates-to-incomplete
relates_to lists only agent-architecture and security-threat-model. It omits docs/problems/autonomy-spectrum.md — which opens with essentially this ADR's question ("when should agents auto-merge vs. escalate") and which docs/architecture.md's Policy Store open question already points to — and docs/problems/code-review.md, which defines the verdict model this ADR's REVIEW_AUTO_MERGE trigger condition depends on. Notably, docs/roadmap.md's own entry for this exact work links to autonomy-spectrum and code-review, not agent-architecture.
Suggestion: Add autonomy-spectrum and code-review to relates_to, and consider whether all three remain warranted given the writing-adrs skill's guidance that more than 3 problem docs may signal too-broad a decision.
(Flagged independently by 2 of 3 reviewers.)
| - **Prompt injection blast radius.** A compromised review agent with the merge | ||
| token can enable auto-merge on the PR it was invoked for. It cannot merge | ||
| arbitrary PRs — branch protection (required reviewers, status checks) still | ||
| gates the actual merge. The blast radius is one PR, same as today. |
There was a problem hiding this comment.
[MEDIUM] blast-radius-framing
"The blast radius is one PR, same as today" conflates horizontal scope (true: still limited to one PR) with severity (not true: today, a compromised review agent's fraudulent approval is just a comment — a human must still click merge for it to take effect. With REVIEW_AUTO_MERGE on, that same fraudulent approval can autonomously execute the merge with no human action). The scope is unchanged; the consequence within that scope is materially higher.
Suggestion: Reword to acknowledge that the human merge gate is intentionally removed for that one PR — that's the point of the feature — rather than implying no change in outcome severity.
(Flagged independently by all 3 reviewers.)
| ### C. New app identity, same review agent, env-var toggle | ||
|
|
||
| Create a `fullsend-ai-merge` app with `contents: write` and | ||
| `pull_requests: write`, but teach the existing review agent to auto-merge when |
There was a problem hiding this comment.
[MEDIUM] redundant-auto-merge-mechanism
Building on the existing open thread about internal/config/config.go's auto_merge field: this isn't just an unmentioned struct field. It's a documented config surface — ADR 0045's config table lists defaults.auto_merge as "Org-wide auto-merge policy" — with test coverage (config_test.go) and a UI-exposed type (web/admin/src/lib/layers/orgConfigParse.ts: auto_merge?: boolean). There's no Go code that actually reads/branches on it today (internal/layers/enrollment.go never references it), so it's currently schema without behavior — but it's the existing, already-designed answer to the same question REVIEW_AUTO_MERGE now answers a different way (a raw env var set directly in a repo's workflow file, outside the org-config/enrollment layering entirely).
Suggestion: State whether REVIEW_AUTO_MERGE supersedes auto_merge, or how the two are meant to relate (org-level default vs. per-repo override) — right now they're two disconnected mechanisms for the same concept.
Avoid collision with open ADR 0071 (auto-merge fullsend-ai#2791). Drop closed fullsend-ai#5449 as active tracker, align forge wording with ADR 0005, and point the join contract at ABEvalFlow fullsend-ai#57 until it lands on main. Co-authored-by: Cursor <cursoragent@cursor.com>
Avoid collision with open ADR 0071 (auto-merge fullsend-ai#2791). Drop closed fullsend-ai#5449 as active tracker; mark vcs.*/cicd.* illustrative pending the shared join contract; scope forge coverage to GitHub/GitLab today; link the contract via ABEvalFlow fullsend-ai#57 only. Signed-off-by: Hofni Gartner <hgartner@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
- Renumber to 0079 to avoid collision with PR fullsend-ai#2791 (ADR 0071 auto-merge) and PR fullsend-ai#5016 (ADR 0072 pre-script output protocol) - Replace deprecated runner_env with env.runner/env.sandbox per ADR 0055 - Remove incorrect ADR 0045 forge: citation for Jira-aware behavior - Narrow Consequences claim to Jira-token portion of ADR 0063's open question - Soften idempotent assertion to intended contract - Add missing ADR 0063 cross-reference in architecture.md Signed-off-by: Manish Kumar <30774250+manish-jangra@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Avoid collision with open ADR 0071 (auto-merge fullsend-ai#2791). Drop closed shared join contract; scope forge coverage to GitHub/GitLab today; link the contract via ABEvalFlow fullsend-ai#57 only. Signed-off-by: Hofni Gartner <hgartner@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Avoid collision with open ADR 0071 (auto-merge fullsend-ai#2791). Drop closed shared join contract; scope forge coverage to GitHub/GitLab today; link the contract via ABEvalFlow fullsend-ai#57 only. Signed-off-by: Hofni Gartner <hgartner@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Prerequisite: review qualityLinking #6322: multi-model collaborative review should be treated as a prerequisite before advancing this auto-merge design. The This is complementary to the implementation prerequisites here (the separate merge identity, credential isolation, scoped CODEOWNERS policy, and merge-queue behavior); #6322 does not replace those controls. |
|
dropping this - I think we're going to approach this a different way based on team planning today |
|
🤖 Finished Retro · ✅ Success · Started 1:35 AM UTC · Completed 1:49 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.42 |
Retro: PR #2791 — ADR 0071 Auto-mergeHuman-authored ADR proposing auto-merge infrastructure via a separate Timeline
Review quality assessmentThe review agent performed well on procedural and mechanical checks (ADR number collision, formatting, missing sections, sentence completeness) with zero false positives. All its findings drove useful author action. However, it missed the two most architecturally significant findings on the PR, both of which required cross-document reasoning the agent did not perform. Existing issue evidence
Infrastructure noteThe two June 30 review failures were OpenShell 0.0.63 sandbox Proposals skipped (target repo not allowed)File manually or update
|
4 participants
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