Skip to content

docs: ADR 0071 — Auto-merge#2791

Open
ralphbean wants to merge 5 commits into
mainfrom
adr-0062-auto-merge
Open

docs: ADR 0071 — Auto-merge#2791
ralphbean wants to merge 5 commits into
mainfrom
adr-0062-auto-merge

Conversation

@ralphbean

Copy link
Copy Markdown
Member

Summary

  • Adds ADR 0062 proposing a separate fullsend-ai-merge GitHub App for auto-merge
  • Key finding: GitHub only counts approvals toward branch protection when the reviewer has contents: write — our review bot has contents: read, so its approvals are currently informational only
  • Proposes combining a new merge app (opt-in per repo) with blank-owner CODEOWNERS entries for dependency files

Looking for team feedback on the approach before implementation.

Test plan

  • Review the ADR for correctness and completeness
  • Discuss whether the two-app model is the right tradeoff vs. adding contents: write to the review app

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>
@ralphbean ralphbean requested a review from a team as a code owner June 30, 2026 17:39
@ralphbean ralphbean marked this pull request as draft June 30, 2026 17:39
@qodo-code-review

qodo-code-review Bot commented Jun 30, 2026

Copy link
Copy Markdown

PR Summary by Qodo

ADR 0062: Opt-in auto-merge via merge app identity + CODEOWNERS exemptions

📝 Documentation 🕐 10-20 Minutes

Grey Divider

AI Description

• Documents why bot approvals don’t satisfy branch protection without contents: write.
• Proposes opt-in auto-merge using a separate fullsend-ai-merge GitHub App identity.
• Recommends blank-owner CODEOWNERS entries for dependency files to remove owner gating.
Diagram

graph TD
  RA["Review agent"] --> EV[("REVIEW_AUTO_MERGE")]
  EV --> GH{{"GitHub"}} --> BP[("Branch protection")]
  GH --> MA["Merge app identity"]
  CO[("CODEOWNERS")]
  CO --> BP
  subgraph Legend
    direction LR
    _svc["Service/Agent"] ~~~ _cfg[("Repo setting") ] ~~~ _ext{{"Platform"}}
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Grant `contents: write` to `fullsend-ai-review`
  • ➕ Simplest operational model (one app, one agent).
  • ➕ No additional installation/identity management per repo.
  • ➖ Violates least-privilege boundary by giving all review installs write power.
  • ➖ Expands blast radius if the review app is compromised.
2. Dedicated merge agent + separate harness
  • ➕ Strong separation between review and merge responsibilities.
  • ➕ Clear operational and security boundary; fewer conditional behaviors.
  • ➖ Duplicates orchestration logic and increases maintenance surface area.
  • ➖ Introduces another agent lifecycle (deployments, config, on-call patterns).
3. Rely only on blank-owner CODEOWNERS exemptions
  • ➕ Avoids any new write-scoped GitHub App identity.
  • ➕ Solves the dependency-file codeowner gate directly.
  • ➖ Does not address branch-protection approval counting (approval gate remains).
  • ➖ Reduces human review guarantees for exempted files; shifts trust to CI.

Recommendation: The ADR’s chosen approach (new write-scoped app identity + env-var gate, plus blank-owner CODEOWNERS exemptions) is a good balance: it keeps the review app read-only by default, makes auto-merge explicitly opt-in per repo, and remains reversible if a dedicated merge agent becomes desirable later.

Files changed (1) +91 / -0

Documentation (1) +91 / -0
0062-auto-merge.mdAdd ADR 0062 defining opt-in auto-merge design and tradeoffs +91/-0

Add ADR 0062 defining opt-in auto-merge design and tradeoffs

• Introduces an accepted ADR documenting why bot approvals don’t count toward branch protection without 'contents: write'. Records the decision to use a separate 'fullsend-ai-merge' app identity plus an env-var gated auto-merge behavior in the existing review agent, and to use blank-owner CODEOWNERS entries for dependency files.

docs/ADRs/0062-auto-merge.md

@github-actions

Copy link
Copy Markdown

Site preview

Preview: https://70538298-site.fullsend-ai.workers.dev

Commit: 996296dfd975f8cd393b6675722f2b546193eaa3

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 30, 2026

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 5:42 PM UTC · Ended 5:49 PM UTC
Commit: 104508d · View workflow run →

@codecov

codecov Bot commented Jun 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@qodo-code-review

qodo-code-review Bot commented Jun 30, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 54 rules

Grey Divider


Action required

1. docs/architecture.md not updated ✓ Resolved 📜 Skill insight ⚙ Maintainability
Description
This PR adds a new ADR marked Accepted, but there is no corresponding update to
docs/architecture.md to reflect the newly decided behavior and annotate any resolved open
questions. This can leave the architecture overview inconsistent with the accepted decision and
makes it harder to discover the current state of the system.
Code

docs/ADRs/0062-auto-merge.md[R2-3]

+title: "62. Auto-merge"
+status: Accepted
Relevance

⭐⭐⭐ High

Accepted ADRs must update living docs/architecture.md; explicitly practiced in PR #1578 and policy
in PR #161.

PR-#1578
PR-#161

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The ADR is explicitly marked Accepted, which triggers the requirement to update
docs/architecture.md and to annotate resolved open questions. docs/architecture.md currently
contains relevant sections about GitHub Apps and an explicit open question about the auto-merge vs.
escalate policy, but there is no recorded 'Decided' update referencing ADR 0062 in this PR.

Rule 1062062: Update architecture overview and problem docs when ADR is accepted
docs/ADRs/0062-auto-merge.md[2-3]
docs/architecture.md[138-149]
docs/architecture.md[190-195]
Skill: writing-adrs

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A new ADR (`0062-auto-merge`) is introduced as `Accepted`, but the architecture overview is not updated to reflect/record the decision and to annotate any relevant resolved open questions.

## Issue Context
The compliance rules require that when an ADR is accepted, `docs/architecture.md` is updated in the same PR (and resolved open questions are annotated as resolved rather than left dangling).

## Fix Focus Areas
- docs/ADRs/0062-auto-merge.md[2-3]
- docs/architecture.md[138-200]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. ADR makes two decisions ✓ Resolved 📜 Skill insight ⚙ Maintainability
Description
The ## Decision section makes two distinct decisions (create a separate merge app, and adopt
blank-owner CODEOWNERS entries), which violates the requirement that each ADR records exactly one
decision. This reduces clarity and makes it harder to supersede or revisit each decision
independently.
Code

docs/ADRs/0062-auto-merge.md[R54-65]

+## Decision
+
+Use options B and C together:
+
+1. **Create a `fullsend-ai-merge` app** with `contents: write` and
+   `pull_requests: write`. Its approvals count toward branch protection. Repos
+   that want bot-driven auto-merge install this app; repos that only want
+   informational reviews keep `fullsend-ai-review`.
+
+2. **Use blank-owner CODEOWNERS entries** for files that bots should be able to
+   approve (starting with `go.mod` and `go.sum`). This removes the CODEOWNERS
+   gate for those files without affecting other paths.
Relevance

⭐⭐ Medium

Some precedent to create new ADR for new decision (PR #2525), but no evidence of strict
one-decision-per-ADR rule.

PR-#2525

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The compliance rule requires exactly one decision per ADR. The Decision section explicitly
enumerates two separate decisions as items 1 and 2.

docs/ADRs/0062-auto-merge.md[54-65]
Skill: writing-adrs

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The ADR's Decision section currently records two distinct decisions.

## Issue Context
Compliance requires each ADR to record exactly one decision; multiple decisions should be split so each can be reviewed, accepted, and superseded independently.

## Fix Focus Areas
- docs/ADRs/0062-auto-merge.md[54-65]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

3. Auto-merge identity ambiguous ✓ Resolved 🐞 Bug ≡ Correctness ⭐ New
Description
The ADR says auto-merge is implemented in the existing review agent (same harness/post-script) while
also introducing a separate write-capable fullsend-ai-merge app, but it never specifies how the
review agent selects/receives the merge app token when REVIEW_AUTO_MERGE=true. This ambiguity can
lead to an implementation that either (a) still runs with the read-only review app token and fails
to enable auto-merge, or (b) silently runs the review agent under a write-capable identity,
undermining the stated least-privilege boundary.
Code

docs/ADRs/0062-auto-merge.md[R67-74]

+1. **Create a `fullsend-ai-merge` app** with `contents: write` and
+   `pull_requests: write`. Repos that want bot-driven auto-merge install this
+   app alongside or instead of `fullsend-ai-review`.
+
+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
+   agent harness — same `review.yaml`, same skills, same post-script.
Relevance

⭐⭐⭐ High

Team often accepts ADR clarifications removing implementation ambiguity (e.g., explicit
token/contract wiring in ADRs).

PR-#730
PR-#716
PR-#1580

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The decision text introduces a separate write-capable merge app while simultaneously asserting the
existing review agent/post-script will enable auto-merge, but it does not describe the
authentication/token-selection mechanism required to preserve the read-only review boundary
established in ADR 7.

docs/ADRs/0062-auto-merge.md[67-74]
docs/ADRs/0007-per-role-github-apps.md[23-27]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
ADR 0062 introduces a new write-capable GitHub App (`fullsend-ai-merge`) but also states auto-merge is implemented inside the existing review agent (same harness/post-script). The ADR does not state how the review agent authenticates as the merge app (or otherwise obtains a `pull_requests: write` token) when auto-merge is enabled.

## Issue Context
ADR 7 establishes per-role GitHub Apps to preserve least-privilege boundaries; the review role is expected to be read-only.

## Fix Focus Areas
- docs/ADRs/0062-auto-merge.md[67-74]

## What to change
- Add a short explicit subsection/bullet clarifying:
 - Which GitHub App token is used for normal review operations vs. enabling auto-merge.
 - How the post-script receives that token (e.g., separate token minting step using the merge app secrets when `REVIEW_AUTO_MERGE=true`).
 - What happens when only `fullsend-ai-review` is installed (auto-merge must be disabled / fail closed).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. Consequences has six bullets ✓ Resolved 📜 Skill insight ⚙ Maintainability ⭐ New
Description
The ADR ## Consequences section contains 6 bullet points, but the compliance standard requires 3–5
one-sentence bullets. This makes the ADR harder to scan and violates the required ADR consequence
format.
Code

docs/ADRs/0062-auto-merge.md[R87-91]

+- Auto-merge is opt-in per repo, visible in the GitHub App installation list.
+- No new agent to maintain — auto-merge is a small addition to `post-review.sh`.
+- If the env-var approach proves wrong, standing up a dedicated agent is not made harder by this decision.
+- Blank-owner CODEOWNERS entries remove code-owner review for listed files; CI status checks remain the primary safety gate.
+- The merge app's `contents: write` is a higher-value target if compromised — same mitigations from [ADR 7](0007-per-role-github-apps.md) apply.
Relevance

⭐⭐ Medium

Format/compliance constraints are inconsistently enforced; similar “bring ADR into compliance”
requests were rejected.

PR-#2582
PR-#2650

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 1062091 requires the ADR ## Consequences section to have 3–5 one-sentence bullet
points. The Consequences section in docs/ADRs/0062-auto-merge.md currently includes six bullets
(lines 86–91), exceeding the maximum.

docs/ADRs/0062-auto-merge.md[84-91]
Skill: writing-adrs

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`docs/ADRs/0062-auto-merge.md` has 6 bullets in `## Consequences`, but the standard requires 3–5 one-sentence bullet points.

## Issue Context
This ADR is marked `Accepted`, so it should follow the canonical ADR formatting rules, including a concise consequences list.

## Fix Focus Areas
- docs/ADRs/0062-auto-merge.md[84-91]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. Context lacks problem doc links ✓ Resolved 📜 Skill insight ⚙ Maintainability
Description
The ADR Context describes motivations but does not link to the related problem documents, despite
listing them in relates_to. This makes it harder to trace the decision back to the canonical
problem statements and risks duplicating/fragmenting context over time.
Code

docs/ADRs/0062-auto-merge.md[R21-32]

+## Context
+
+Our review bot (`fullsend-ai-review`) can approve PRs, but GitHub only counts
+an approval toward branch protection when the reviewer has write access
+(`contents: write`) to the repository. The review app intentionally has
+`contents: read` — giving it write access would violate the least-privilege
+boundary established in [ADR 7](0007-per-role-github-apps.md).
+
+We also need the bot's approval to satisfy CODEOWNERS for dependency files
+(`go.mod`, `go.sum`). GitHub App bots cannot be listed as CODEOWNERS entries —
+they are not "users" in GitHub's model.
+
Relevance

⭐⭐ Medium

No clear historical enforcement that Context must link problem docs; only general link tidy-ups like
PR #2663.

PR-#2663

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The rule requires the Context to link to problem docs instead of restating/isolating context. The
frontmatter indicates related problem docs, but the Context section does not link to those problem
doc files.

docs/ADRs/0062-auto-merge.md[4-6]
docs/ADRs/0062-auto-merge.md[21-32]
Skill: writing-adrs

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The ADR Context should link to the relevant problem docs rather than standing alone.

## Issue Context
The ADR declares `relates_to` entries for `agent-architecture` and `security-threat-model`, but the Context section does not include links to those problem documents.

## Fix Focus Areas
- docs/ADRs/0062-auto-merge.md[4-6]
- docs/ADRs/0062-auto-merge.md[21-32]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

6. Sentence fragment in rationale ✓ Resolved 🐞 Bug ⚙ Maintainability ⭐ New
Description
The reversibility rationale ends with a sentence fragment ("would be."), leaving the argument
incomplete and harder to interpret. This weakens the ADR’s documented justification for the chosen
approach.
Code

docs/ADRs/0062-auto-merge.md[R79-82]

+This is the most **reversible** path. If a dedicated merge agent turns out to
+be the better UX, having once had a more capable review agent does not make
+that harder — we stand up the new agent and deprecate the env var. Going the
+other direction (standalone agent first, then collapsing back) would be.
Relevance

⭐⭐⭐ High

Doc wording/grammar fixes are typically accepted in ADR reviews; many prior ADR text nits merged.

PR-#730
PR-#655
PR-#2465

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The cited lines contain an unfinished sentence, ending with “would be.” and no completion.

docs/ADRs/0062-auto-merge.md[79-82]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The reversibility paragraph ends with an incomplete sentence: "Going the other direction ... would be." This is a sentence fragment.

## Issue Context
This is part of the ADR’s justification for choosing options C+D and should clearly state what would be harder about the alternative direction.

## Fix Focus Areas
- docs/ADRs/0062-auto-merge.md[79-82]

## What to change
- Finish the sentence by stating the missing predicate (e.g., "... would be harder because ..."), so the reversibility argument is explicit and complete.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Previous review results

Review updated until commit 3a3507a

Results up to commit 996296d


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Action required
1. docs/architecture.md not updated ✓ Resolved 📜 Skill insight ⚙ Maintainability
Description
This PR adds a new ADR marked Accepted, but there is no corresponding update to
docs/architecture.md to reflect the newly decided behavior and annotate any resolved open
questions. This can leave the architecture overview inconsistent with the accepted decision and
makes it harder to discover the current state of the system.
Code

docs/ADRs/0062-auto-merge.md[R2-3]

+title: "62. Auto-merge"
+status: Accepted
Relevance

⭐⭐⭐ High

Accepted ADRs must update living docs/architecture.md; explicitly practiced in PR #1578 and policy
in PR #161.

PR-#1578
PR-#161

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The ADR is explicitly marked Accepted, which triggers the requirement to update
docs/architecture.md and to annotate resolved open questions. docs/architecture.md currently
contains relevant sections about GitHub Apps and an explicit open question about the auto-merge vs.
escalate policy, but there is no recorded 'Decided' update referencing ADR 0062 in this PR.

Rule 1062062: Update architecture overview and problem docs when ADR is accepted
docs/ADRs/0062-auto-merge.md[2-3]
docs/architecture.md[138-149]
docs/architecture.md[190-195]
Skill: writing-adrs

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A new ADR (`0062-auto-merge`) is introduced as `Accepted`, but the architecture overview is not updated to reflect/record the decision and to annotate any relevant resolved open questions.

## Issue Context
The compliance rules require that when an ADR is accepted, `docs/architecture.md` is updated in the same PR (and resolved open questions are annotated as resolved rather than left dangling).

## Fix Focus Areas
- docs/ADRs/0062-auto-merge.md[2-3]
- docs/architecture.md[138-200]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. ADR makes two decisions ✓ Resolved 📜 Skill insight ⚙ Maintainability
Description
The ## Decision section makes two distinct decisions (create a separate merge app, and adopt
blank-owner CODEOWNERS entries), which violates the requirement that each ADR records exactly one
decision. This reduces clarity and makes it harder to supersede or revisit each decision
independently.
Code

docs/ADRs/0062-auto-merge.md[R54-65]

+## Decision
+
+Use options B and C together:
+
+1. **Create a `fullsend-ai-merge` app** with `contents: write` and
+   `pull_requests: write`. Its approvals count toward branch protection. Repos
+   that want bot-driven auto-merge install this app; repos that only want
+   informational reviews keep `fullsend-ai-review`.
+
+2. **Use blank-owner CODEOWNERS entries** for files that bots should be able to
+   approve (starting with `go.mod` and `go.sum`). This removes the CODEOWNERS
+   gate for those files without affecting other paths.
Relevance

⭐⭐ Medium

Some precedent to create new ADR for new decision (PR #2525), but no evidence of strict
one-decision-per-ADR rule.

PR-#2525

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The compliance rule requires exactly one decision per ADR. The Decision section explicitly
enumerates two separate decisions as items 1 and 2.

docs/ADRs/0062-auto-merge.md[54-65]
Skill: writing-adrs

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The ADR's Decision section currently records two distinct decisions.

## Issue Context
Compliance requires each ADR to record exactly one decision; multiple decisions should be split so each can be reviewed, accepted, and superseded independently.

## Fix Focus Areas
- docs/ADRs/0062-auto-merge.md[54-65]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended
3. Context lacks problem doc links ✓ Resolved 📜 Skill insight ⚙ Maintainability
Description
The ADR Context describes motivations but does not link to the related problem documents, despite
listing them in relates_to. This makes it harder to trace the decision back to the canonical
problem statements and risks duplicating/fragmenting context over time.
Code

docs/ADRs/0062-auto-merge.md[R21-32]

+## Context
+
+Our review bot (`fullsend-ai-review`) can approve PRs, but GitHub only counts
+an approval toward branch protection when the reviewer has write access
+(`contents: write`) to the repository. The review app intentionally has
+`contents: read` — giving it write access would violate the least-privilege
+boundary established in [ADR 7](0007-per-role-github-apps.md).
+
+We also need the bot's approval to satisfy CODEOWNERS for dependency files
+(`go.mod`, `go.sum`). GitHub App bots cannot be listed as CODEOWNERS entries —
+they are not "users" in GitHub's model.
+
Relevance

⭐⭐ Medium

No clear historical enforcement that Context must link problem docs; only general link tidy-ups like
PR #2663.

PR-#2663

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The rule requires the Context to link to problem docs instead of restating/isolating context. The
frontmatter indicates related problem docs, but the Context section does not link to those problem
doc files.

docs/ADRs/0062-auto-merge.md[4-6]
docs/ADRs/0062-auto-merge.md[21-32]
Skill: writing-adrs

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The ADR Context should link to the relevant problem docs rather than standing alone.

## Issue Context
The ADR declares `relates_to` entries for `agent-architecture` and `security-threat-model`, but the Context section does not include links to those problem documents.

## Fix Focus Areas
- docs/ADRs/0062-auto-merge.md[4-6]
- docs/ADRs/0062-auto-merge.md[21-32]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Qodo Logo

Comment thread docs/ADRs/0062-auto-merge.md Outdated
Comment thread docs/ADRs/0062-auto-merge.md Outdated
Comment thread docs/ADRs/0071-auto-merge.md
@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ❌ Failure · Started 5:42 PM UTC · Completed 5:49 PM UTC
Commit: 996296d · View workflow run →

@ralphbean

Copy link
Copy Markdown
Member Author

/fs-review

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 30, 2026

Copy link
Copy Markdown

🤖 Finished Review · ❌ Failure · Started 6:30 PM UTC · Completed 6:46 PM UTC
Commit: b95b879 · View workflow run →

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>
@ralphbean ralphbean marked this pull request as ready for review July 13, 2026 18:06
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 13, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 6:07 PM UTC · Completed 6:18 PM UTC
Commit: 5921a16 · View workflow run →

Comment thread docs/ADRs/0071-auto-merge.md Outdated
Comment thread docs/ADRs/0071-auto-merge.md
Comment thread docs/ADRs/0071-auto-merge.md Outdated
@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 5921a16

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 13, 2026

Copy link
Copy Markdown

Review

Findings

High

  • [missing-architecture-update] docs/architecture.md — ADR 0071 has status Accepted but docs/architecture.md is not updated in this PR. Per AGENTS.md: "When status is Accepted, update docs/architecture.md and related problem docs in the same PR per the writing-adrs skill." Line 213 has an open question — "How does policy interact with the autonomy spectrum — is the auto-merge vs. escalate decision a policy setting?" — that this ADR directly addresses. The ADR establishes that auto-merge is opt-in per repo via app installation and an env-var toggle, answering (at least partially) whether it is a policy setting.
    Remediation: Add a Decided: entry under the Policy Store section referencing ADR 0071. Annotate the open question at line 213 with a reference to ADR 0071 (use the parenthetical format from the writing-adrs skill if partial resolution).

Medium

  • [incorrect-security-claim] docs/ADRs/0071-auto-merge.md:97 — The claim "The review agent never holds both the merge token and a provider credential simultaneously" is misleading. In the current review.yaml, the sandbox receives GH_TOKEN as an env var and GCP credentials via host_files — the agent holds both simultaneously inside the sandbox. The Token Handoff section says the harness mints a token from fullsend-ai-merge "instead of" fullsend-ai-review, implying the merge token would replace GH_TOKEN in the sandbox, coexisting with provider credentials. If the intended design is that only the post-script uses the merge token while the sandbox keeps a read-only token, the ADR should state that explicitly.
    Remediation: Clarify which token flows where: state that the merge token is consumed only by the post-script outside the sandbox (if that is the intent), or remove the "never holds both simultaneously" claim and describe the actual security posture.

  • [missing-authorization] — Non-trivial ADR (131 lines, proposes a new GitHub App and env-var-gated behavior) with status Accepted, but the PR has no linked issue providing explicit authorization for this work.

Low

  • [internal-consistency] docs/ADRs/0071-auto-merge.md:66 — 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 or explain how the env var relates to or replaces it.

  • [metadata-consistency] — The PR body still says "Adds ADR 0062" but the file is docs/ADRs/0071-auto-merge.md. Update the PR description to match.

  • [missing-problem-doc-cross-reference] docs/problems/agent-architecture.md — The ADR's relates_to lists agent-architecture. Adding a cross-reference annotation where ADR 0007 is cited would improve discoverability of the auto-merge extension to the per-role GitHub Apps model.

  • [missing-problem-doc-cross-reference] docs/problems/security-threat-model.md — The ADR's relates_to lists security-threat-model. A cross-reference annotation would help readers understand how auto-merge interacts with the prompt injection blast radius and credential isolation discussions.

Previous run

Review

Findings

High

  • [missing-architecture-update] docs/architecture.md — ADR 0071 has status Accepted but docs/architecture.md is not updated in this PR. Per AGENTS.md: "When status is Accepted, update docs/architecture.md and related problem docs in the same PR per the writing-adrs skill." Line 213 has an open question — "How does policy interact with the autonomy spectrum — is the auto-merge vs. escalate decision a policy setting?" — that this ADR directly resolves.
    Remediation: Add a Decided: entry under the relevant component section in docs/architecture.md documenting the fullsend-ai-merge app and auto-merge behavior. Annotate the open question at line 213 with a reference to ADR 0071.

Medium

  • [missing-authorization] — Non-trivial ADR (91 lines, proposes a new GitHub App and env-var-gated behavior) with status Accepted, but the PR has no linked issue providing explicit authorization for this work.

  • [incomplete-sentence] docs/ADRs/0071-auto-merge.md:80 — The sentence "Going the other direction (standalone agent first, then collapsing back) would be." is grammatically incomplete — it ends without a predicate complement. The Decision section's reversibility argument is ambiguous without knowing what the author intended (e.g., "would be harder," "would be more disruptive").
    Remediation: Complete the sentence with the intended predicate.

  • [undocumented-config-var] docs/ADRs/0071-auto-merge.md:52 — The ADR introduces REVIEW_AUTO_MERGE as a gating env var but doesn't clarify that implementation is deferred to a follow-up PR, nor note how the naming relates to the agent env-var convention (ADR 0049). Readers may be uncertain whether this PR is complete.
    Remediation: Add a sentence noting that REVIEW_AUTO_MERGE implementation in review.yaml and post-review.sh is deferred to a follow-up PR.

Low

  • [cross-reference-formatting] docs/ADRs/0071-auto-merge.md:27 — Uses "ADR 7" but the established convention across the ADR corpus is zero-padded "ADR 0007." Also appears at line 91.
    Remediation: Change to [ADR 0007](0007-per-role-github-apps.md).

  • [missing-problem-doc-annotations] docs/problems/agent-architecture.md — The ADR's relates_to lists agent-architecture and security-threat-model. Adding cross-reference annotations to these problem docs would improve discoverability, though neither has an open question directly resolved by this ADR.

  • [metadata-consistency] — The PR body still says "Adds ADR 0062" but the file is docs/ADRs/0071-auto-merge.md. Update the PR description to match.

  • [security-design-gaps] docs/ADRs/0071-auto-merge.md:62 — The ADR references ADR 0007 mitigations but does not address credential isolation for the merge app's token, prompt injection blast radius when auto-merge removes the human gate, CODEOWNERS bypass scope, or REVIEW_AUTO_MERGE delivery protection. These are valid considerations for the implementation PR.

  • [undocumented-github-app] docs/guides/getting-started/configuring-github.md — The fullsend-ai-merge app proposed by this ADR will need to be added to the getting-started guide's installation table when implemented.

  • [missing-implementation-details] docs/ADRs/0071-auto-merge.md:72 — The ADR references post-review.sh integration but doesn't note that the implementation is deferred. See also: [undocumented-config-var] finding.

Previous run

Review

Findings

High

  • [missing-architecture-update] docs/architecture.md — ADR 0071 has status Accepted but docs/architecture.md is not updated in this PR. Per AGENTS.md: "When status is Accepted, update docs/architecture.md and related problem docs in the same PR per the writing-adrs skill." Line 213 has an open question — "How does policy interact with the autonomy spectrum — is the auto-merge vs. escalate decision a policy setting?" — that this ADR directly resolves.
    Remediation: Add a Decided: entry under the relevant component section in docs/architecture.md documenting the fullsend-ai-merge app and auto-merge behavior. Annotate the open question at line 213 with a reference to ADR 0071.

Medium

  • [missing-authorization] — Non-trivial ADR (91 lines, proposes a new GitHub App and env-var-gated behavior) with status Accepted, but the PR has no linked issue providing explicit authorization for this work.

  • [incomplete-sentence] docs/ADRs/0071-auto-merge.md:80 — The sentence "Going the other direction (standalone agent first, then collapsing back) would be." is grammatically incomplete — it ends without a predicate complement. The Decision section's reversibility argument is ambiguous without knowing what the author intended (e.g., "would be harder," "would be more disruptive").
    Remediation: Complete the sentence with the intended predicate.

  • [undocumented-config-var] docs/ADRs/0071-auto-merge.md:52 — The ADR introduces REVIEW_AUTO_MERGE as a gating env var but doesn't clarify that implementation is deferred to a follow-up PR, nor note how the naming relates to the agent env-var convention (ADR 0049). Readers may be uncertain whether this PR is complete.
    Remediation: Add a sentence noting that REVIEW_AUTO_MERGE implementation in review.yaml and post-review.sh is deferred to a follow-up PR.

Low

  • [cross-reference-formatting] docs/ADRs/0071-auto-merge.md:27 — Uses "ADR 7" but the established convention across the ADR corpus is zero-padded "ADR 0007." Also appears at line 91.
    Remediation: Change to [ADR 0007](0007-per-role-github-apps.md).

  • [missing-problem-doc-annotations] docs/problems/agent-architecture.md — The ADR's relates_to lists agent-architecture and security-threat-model. Adding cross-reference annotations to these problem docs would improve discoverability, though neither has an open question directly resolved by this ADR.

  • [metadata-consistency] — The PR body still says "Adds ADR 0062" but the file is docs/ADRs/0071-auto-merge.md. Update the PR description to match.

  • [security-design-gaps] docs/ADRs/0071-auto-merge.md:62 — The ADR references ADR 0007 mitigations but does not address credential isolation for the merge app's token, prompt injection blast radius when auto-merge removes the human gate, CODEOWNERS bypass scope, or REVIEW_AUTO_MERGE delivery protection. These are valid considerations for the implementation PR.

  • [undocumented-github-app] docs/guides/getting-started/configuring-github.md — The fullsend-ai-merge app proposed by this ADR will need to be added to the getting-started guide's installation table when implemented.

  • [missing-implementation-details] docs/ADRs/0071-auto-merge.md:72 — The ADR references post-review.sh integration but doesn't note that the implementation is deferred. See also: [undocumented-config-var] finding.

Previous run (2)

Review — docs: ADR 0062 — Auto-merge

Verdict: Request Changes

Summary

This PR adds a new ADR proposing a fullsend-ai-merge GitHub App for auto-merge, combining a new app identity with blank-owner CODEOWNERS entries. The architectural approach is sound — separating the merge capability into its own app preserves the least-privilege boundary from ADR 7. However, the PR has two blocking issues and several items requiring attention before it can merge.

Findings

🔴 High

1. ADR number collision — 0062 is already taken
docs/ADRs/0062-auto-merge.md

ADR 0062 already exists on main as 0062-dispatch-version-skew.md (status: Accepted, date: 2026-06-25). The highest ADR on main is 0067-gitlab-cron-polling-event-dispatch.md, so the next available number is 0068. The writing-adrs skill checklist step 1 requires: "Find the next number. List docs/ADRs/ on current main, then scan open pull requests."

Remediation: Renumber to 0068 (or the next available at merge time). Update the filename, frontmatter title, and # heading.


2. Missing docs/architecture.md update — required for accepted ADRs
docs/architecture.md

The ADR has status Accepted, but docs/architecture.md is not updated in this PR. Per AGENTS.md: "When status is Accepted, update docs/architecture.md and related problem docs in the same PR per the writing-adrs skill."

Specifically, docs/architecture.md line 213 has an open question that this ADR at least partially resolves:

How does policy interact with the autonomy spectrum — is the auto-merge vs. escalate decision a policy setting?

This question should be annotated with a reference to this ADR. Additionally, a "Decided:" entry should be added under the relevant component section (likely Agent Identity Provider or Reference workflow components § 12. Coordinator merge algorithm) documenting the auto-merge decision.

Remediation: Add a Decided: entry to docs/architecture.md under the appropriate section, annotate the open question at line 213, and annotate any resolved questions in docs/problems/agent-architecture.md and docs/problems/security-threat-model.md per the writing-adrs skill.


🟡 Medium

3. Incomplete sentence in Decision section
docs/ADRs/0062-auto-merge.md

The last sentence in the Decision section is truncated:

"Going the other direction (standalone agent first, then collapsing back) would be."

This sentence has no predicate — it cuts off without completing the thought. The Decision section is the most important part of an ADR; an incomplete sentence leaves the reversibility rationale ambiguous. The reader cannot tell whether the author meant "would be harder," "would be more disruptive," or something else.

Remediation: Complete the sentence (e.g., "...would be more disruptive" or "...would require additional migration work").


4. Missing problem doc annotations
docs/problems/agent-architecture.md, docs/problems/security-threat-model.md

The ADR's relates_to lists agent-architecture and security-threat-model. Per the writing-adrs skill, if those problem docs have open questions this ADR resolves, they should be annotated with ~~question~~ Decided in [ADR NNNN](...) links in this PR.

Remediation: Review both problem docs for open questions about auto-merge behavior and annotate any that this ADR resolves or partially resolves.


🔵 Low

5. Security design considerations not addressed
docs/ADRs/0062-auto-merge.md

The ADR's Consequences section acknowledges the merge app is "a higher-value target if compromised" and references ADR 7's mitigations, but does not address several security-relevant design questions:

  • Credential isolation: Will the merge app's contents: write token enter the sandbox, or stay in the post-script (per ADR 0017)?
  • Prompt injection blast radius: Auto-merge increases the impact of a successful prompt injection against the review agent (ranked threat docs: Add agent-compatible code problem document #1 in the threat model). What guardrails prevent a prompt-injected approve verdict from triggering auto-merge?
  • CODEOWNERS bypass scope: Blank-owner entries remove all code-owner review requirements for listed paths, not just substitute a different owner. The ADR should note this and scope which files are appropriate candidates.
  • REVIEW_AUTO_MERGE protection: How is the env var delivered and protected from being set by untrusted actors?

These are design-level observations for the ADR's completeness. They could be addressed by expanding the Consequences section or by noting them as deferred to implementation PRs.


Labels: PR adds a new ADR document under docs/ADRs/

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added the component/docs User-facing documentation label Jul 13, 2026
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>
@ralphbean ralphbean changed the title docs: ADR 0062 — Auto-merge docs: ADR 0071 — Auto-merge Jul 13, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 13, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 6:32 PM UTC · Completed 6:48 PM UTC
Commit: 50f5e87 · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

- 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>
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 13, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 7:09 PM UTC · Ended 7:10 PM UTC
Commit: bd28f41 · View workflow run →

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>
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 13, 2026

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 7:11 PM UTC · Ended 7:27 PM UTC
Commit: bd28f41 · View workflow run →

@ralphbean

Copy link
Copy Markdown
Member Author

Ran an experiment to validate the assumptions here: fullsend-ai/experiments#40

Set up two throwaway GitHub Apps against appdumpster/codeowners-lab with CODEOWNERS + branch protection and tested 6 scenarios. All hypotheses confirmed — blank-owner entries + contents: write app works exactly as proposed. Details in the experiment writeup.

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the review comment for full details.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 7:11 PM UTC · Completed 7:27 PM UTC
Commit: 3a3507a · View workflow run →

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/docs User-facing documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant