Repository navigation
Conversation
|
🤖 Review · Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Site previewPreview: https://61c2aabc-site.fullsend-ai.workers.dev Commit: |
a774245 to
944ba16
Compare
|
🤖 Review · Commit: |
|
🤖 Finished Review · ✅ Success · Started 12:02 PM UTC · Completed 12:19 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.82 |
|
Risk Assessment: moderate (2/5) DetailsRe-review anchoring applies: Tier 1 signals are unchanged in category from the prior round (4 files, ~215 lines/medium blast radius, 0 protected/security/dependency/CI paths, non-bot non-first-time author), and the only actual delta since the prior review is a small 5/-2 line wording fix confined to the new ADR file itself (which has zero git history, being newly introduced); the high churn/fix-revert counts on architecture.md and glossary.md remain attributable to pre-existing structural hyper-coupling in those hub docs rather than to this incremental change, and Tier 3 remains minimal since no issue is closed and both referenced issues are explicit out-of-scope context -- so the prior score of 2 (moderate) is preserved. Previous runRisk Assessment: moderate (2/5) DetailsRe-review anchoring applies: Tier 1 signals are unchanged from the prior round (same 4 files, same order-of-magnitude line count, 0 protected/dependency/CI paths, non-bot non-first-time author), Tier 2 churn/coupling on architecture.md and glossary.md remains attributable to pre-existing structural hyper-coupling rather than this incremental wording change, and Tier 3 remains minimal since no issue is closed and all referenced issues/PRs are explicit out-of-scope context -- so the prior score of 2 (moderate) is preserved, consistent with 10+ prior review rounds on this same PR. Previous run (2)Risk Assessment: moderate (2/5) DetailsRe-review anchoring applies: Tier 1 signals are unchanged from the prior round (same 4 files, same order-of-magnitude line count, 0 protected/dependency/CI paths, non-bot non-first-time author), Tier 2's elevated churn/coupling/author-diversity signals remain attributable solely to architecture.md's pre-existing structural hyper-coupling rather than to this PR's incremental wording change (no new files, no new coupling introduced), and Tier 3 remains minimal since no issue is closed and all referenced issues/PRs are explicit out-of-scope context -- so the prior score of 2 (moderate) is preserved, consistent with 9+ prior review rounds on this same PR. Previous run (3)Risk Assessment: moderate (2/5) DetailsRe-review anchoring applies: Tier 1 signals are unchanged from the prior round (same 4 docs-only files, ~140 vs ~113 lines, 0 protected/dependency/CI paths, non-bot non-first-time author), Tier 2 elevation is attributable solely to architecture.md's pre-existing structural hyper-coupling rather than this change, and Tier 3 remains minimal since no linked issue is closed and referenced issues are explicit out-of-scope context — so the prior score of 2 (moderate) is preserved. Previous run (4)Risk Assessment: moderate (2/5) DetailsTier 1 signals unchanged from 7+ prior rounds on this PR (4 docs-only files, ~113 lines, 0 protected/dependency/CI paths, non-bot non-first-time author). Tier 2 is elevated only because architecture.md is a structurally hyper-coupled, near-universally-touched doc file, not because of anything specific to this change. Tier 3 is minimal: no issue is closed, referenced issues are explicitly out-of-scope context, and the change is additive/prose-only. Since the prior review only one file changed (a 1-line glossary cross-reference). Per re-review anchoring, the score holds at 2 (moderate), consistent with all prior rounds. Previous run (5)Risk Assessment: moderate (2/5) DetailsTier 1 signals are unchanged from the prior assessment (5 docs-only files, large blast radius by line count, 0 protected/dependency paths, non-bot/non-first-time author); Tier 2 shows normal doc churn concentrated in the pre-existing, high-traffic architecture.md; Tier 3 remains a topically sensitive but explicitly out-of-scope, default-off linked feature. Per re-review anchoring, the substantial content rewrite this round does not change any tier's underlying risk category, so the score holds at 2 (moderate), consistent across 6+ prior rounds. Previous run (6)Risk Assessment: moderate (2/5) DetailsTier 1 signals remain unchanged from the prior assessment category (docs-only, large blast radius, 0 protected/dependency paths, non-bot/non-first-time author); Tier 2 shows normal doc churn concentrated in the pre-existing architecture.md; Tier 3 reflects a topically sensitive but explicitly out-of-scope, default-off linked feature. Per re-review anchoring the score holds at 2 (moderate), consistent with 5+ consecutive prior rounds. Previous run (7)Risk Assessment: moderate (2/5) DetailsTier 1 signals remain in the same category as the prior assessment (docs-only, large blast radius, 0 protected paths, 0 dependency changes, non-bot/non-first-time author); Tier 2 shows normal multi-author doc churn with no revert/fix pattern specific to this content; Tier 3 reflects a topically sensitive but explicitly out-of-scope linked feature (dedicated auto-merge agent). Per re-review anchoring the score holds at 2 (moderate). Previous run (8)Risk Assessment: moderate (2/5) DetailsZero-diff re-review (identical head SHA to the prior assessment) with Tier 1 signals unchanged in category (4 files, docs-only, 0 protected paths, 0 dependency changes, non-bot/non-first-time author); per re-review anchoring the score holds at 2 (moderate), consistent with a large but purely documentation/ADR-defining change. Previous run (9)Risk Assessment: moderate (2/5) DetailsTier 1 signals are unchanged in category from the prior run (4 files, docs-only, 0 protected paths, 0 dependency changes, non-bot/non-first-time author) so per re-review anchoring the score holds at 2 (moderate); docs/architecture.md is a high-churn multi-author file but this PR's actual edit to it is trivial, and Tier 3 (partially degraded since the primary authorization issue is cross-repo) shows no scope mismatch, yielding a moderate composite consistent with a large but purely documentation/ADR-defining change. Previous run (10)Risk Assessment: moderate (2/5) DetailsDocumentation-only PR with low metadata risk; BLAST_RADIUS=large inflates change-size composite but all files are non-executable docs; docs/architecture.md is high-churn multi-author but actual change is minimal; Tier 1 signals unchanged from prior assessment, anchoring to prior score of 2 (moderate). Previous run (11)Risk Assessment: moderate (2/5) DetailsDocumentation-only PR (new ADR + minor architecture and adoption doc updates) with low metadata risk; the sole elevated signal is docs/architecture.md's high churn and multi-author history, but the actual change to that file is minimal (15 additions, 1 deletion), keeping overall risk moderate. |
ReviewFindingsLow
Next steps:
Previous runReviewFindingsLow
Next steps:
Previous run (2)ReviewFindingsMedium
Low
Next steps:
Previous run (3)ReviewFindingsMedium
Low
Next steps:
Previous run (4)ReviewFindingsMedium
Next steps:
Previous run (5)ReviewFindingsLow
Labels: PR modifies only documentation (a new ADR plus architecture/problem doc pointer updates); no existing area/component label reflects that. Next steps:
Previous run (6)ReviewFindingsMedium
Low
Next steps:
Previous run (7)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (8)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (9)ReviewFindingsMedium
Next steps:
Previous run (10)ReviewFindingsHigh
Medium
Next steps:
Previous run (11)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (12)ReviewFindingsHigh
Low
Next steps:
|
![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.
|
🤖 Finished Review · ✅ Success · Started 8:51 PM UTC · Completed 9:09 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.49 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Review · Commit: |
|
🤖 Finished Review · ❌ Failure (validation failed after 1 iteration(s)) · Started 9:14 PM UTC · Completed 9:19 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $0.60 |
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 9:43 PM UTC · Completed 10:02 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $4.29 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| would create two Fullsend-owned ways to authorize autonomous merging and make | ||
| policy, revocation, and auditing ambiguous. Related Fullsend policy and evidence | ||
| work is tracked in [fullsend#3016](https://github.com/fullsend-ai/fullsend/issues/3016) | ||
| and [fullsend#6892](https://github.com/fullsend-ai/fullsend/issues/6892). |
There was a problem hiding this comment.
Lets keep both and see what users end up preferring!
There was a problem hiding this comment.
I preserved the coexistence reality during migration, but the ADR selects one long-term Fullsend-owned authority boundary: the dedicated Auto-Merge stage. The legacy CODE_AUTO_MERGE* path remains temporarily and is planned for retirement rather than becoming a second permanent authorization path.
| required checks and reviews still pass, no human has blocked the change, and the | ||
| revision being merged is the one that was evaluated. This keeps merge | ||
| credentials and final enforcement outside the agent sandbox, consistent with | ||
| [ADR 0017](0017-credential-isolation-for-sandboxed-agents.md). |
There was a problem hiding this comment.
Lets get the scoping here right:
- Blocked by human (via open thread or REQUESTED_CHANGES) - enforced by SCM, NOT the agent!
- Required checks - enforced by SCM, NOT the agent!
- Passing reviews (e.g. open review agent threads or or REQUESTED_CHANGES) - enforced by SCM, NOT the agent!
What the agent needs to evaluate - does the issue and PR fall within the scope and size of issues and PRs that were decided by repo policiy/skills to be auto-mergable
There was a problem hiding this comment.
Updated the scope. The ADR now explicitly leaves required checks, formal reviews/REQUESTED_CHANGES, human blocking through SCM, conversation resolution, branch freshness, mergeability, and queue mechanics to SCM. Auto-Merge evaluates repository-defined unattended-merge scope, semantic intent/context, provider evidence, risk, and trusted human coordination signals.
| path: enter the merge queue when one is required, or merge directly only when | ||
| repository policy permits it. The implementation must never bypass a required | ||
| merge queue. GitHub's generic "merge when ready" feature is not a substitute for | ||
| Fullsend's authorization checks. |
There was a problem hiding this comment.
"merge when ready" is exactly the mechanism the agent should be using on GitHub.
There was a problem hiding this comment.
Clarified. GitHub’s native “merge when ready” behavior is the execution handoff Auto-Merge uses for the repository’s configured direct or merge-queue path. It does not replace the semantic authorization that Fullsend performs before making that request, and it cannot bypass repository rules.
| - The agent never receives merge credentials. A dedicated least-privilege | ||
| identity performs the final GitHub action, and administrator bypass is not | ||
| allowed. | ||
| - The system must keep a durable record of the authorization and its outcome. |
There was a problem hiding this comment.
Missing: Reading automerge policy from repo/skills.
There was a problem hiding this comment.
Addressed. The ADR now states that repository-defined policy and skills are inputs to the Auto-Merge evidence package and determine the allowed unattended-merge scope. Auto-Merge does not invent a universal branch-protection baseline; SCM remains authoritative for mechanical enforcement, and existing blocking rules cause Auto-Merge to wait or escalate.
Signed-off-by: Adam Scerra <ascerra@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 7:59 PM UTC · Completed 8:13 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $2.29 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: Adam Scerra <ascerra@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 8:27 PM UTC · Completed 8:39 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $1.91 |
| ## Decision: dedicated Auto-Merge agent and stage | ||
|
|
||
| We choose Option 3. Fullsend will provide a dedicated Auto-Merge agent as the | ||
| semantic authorization component of a new `auto-merge` stage. The agent |
There was a problem hiding this comment.
Should we just call it "merge agent" instead of "auto-merge agent"?
There was a problem hiding this comment.
/fs-merge vs /fs-auto-merge
/fs-merge is definitely easier to type, but this isn't the most important thing as humans triggering this won't be its main use case.
I personally don't love either of these right now. The way this is shaping up is to be a gate in front of the SCM auto-merge mechanism. I'm still thinking of a different name, but if nothing better surfaces I'd agree to change to "merge agent" since its simpler
| token to bypass required reviews, checks, conversation resolution, branch | ||
| policy, or queue admission. | ||
|
|
||
| ## Decision: dedicated Auto-Merge agent and stage |
There was a problem hiding this comment.
Does this require a new role too? (A new app.) I assume that it does. If so, we should make it explicit. If not, why not?
There was a problem hiding this comment.
It should require a new role... new app I will make that explicit
ggallen
left a comment
ggallen
left a comment
There was a problem hiding this comment.
The ADR does not yet establish why a new Auto-Merge agent is necessary. It conflates three distinct actions: evaluating a PR, requesting native auto-merge, and executing the merge.
The existing flow already separates these responsibilities:
- Review evaluates the PR and produces an approval or other verdict. It does not merge the PR and does not read
CODE_AUTO_MERGE. - When
CODE_AUTO_MERGE=true, the Code stage’s host post-script requests native GitHub auto-merge. - GitHub enforces the repository’s required reviews, checks, and merge queue, and performs the merge.
Consequently, rejecting the existing approach because “the same component would both evaluate and merge the change” does not accurately describe the current behavior. Requesting native auto-merge from a trusted post-script also does not inherently override repository policy. That depends on the credential’s permissions and bypass privileges, not on whether the caller is a post-script or a dedicated stage.
The “multiple ways” argument needs clarification too. The two paths would exist because this proposal introduces a second path alongside CODE_AUTO_MERGE. Retiring the old path is a reasonable migration requirement if the new stage is justified, but avoiding that duplication is not itself a justification for creating the stage.
There are potentially useful additions here: revision-bound attestations, explicit unattended-merge policy, durable authorization records, and a single trusted controller for merge requests. However, the ADR does not demonstrate why these require another model-powered agent. Review already applies protected-path and risk gates, and GitHub already enforces mechanical merge requirements. A trusted controller could consume Review’s result, apply additional deterministic policy, record the authorization, and request the native merge operation.
GitLab is also absent from the analysis. Fullsend supports multiple forges, and the existing Code post-script already has a GitLab auto-merge path. Yet the ADR defines its checks, credentials, and execution handoff entirely in GitHub terms while proposing to retire the existing path. Targeting GitHub for the initial rollout is reasonable, but the decision should distinguish a forge-independent authority boundary from GitHub-specific implementation details and explain what happens to existing GitLab functionality during migration.
Before accepting the new agent as the architectural choice, please provide:
- A concrete case where Review approves and the forge’s conditions pass, but unattended merging requires an additional judgment.
- The inputs and decision that distinguish that judgment from Review’s existing evaluation and policy gates.
- Why that decision requires another agent rather than extending Review or adding a deterministic trusted controller.
- An accurate comparison with the existing implementation, identifying which protections are already present and which would be introduced.
- A GitLab compatibility and migration plan, including how native merge conditions map to the proposed boundary and whether GitLab retains the existing path until a replacement is available.
As written, the ADR makes a plausible case for formalizing Fullsend’s merge authorization policy, but it does not yet justify a separate Auto-Merge agent or adequately address the consequences for other supported forges.
| 1. Keep auto-merge in Code or Review — **Rejected** because the same component | ||
| would both evaluate and merge the change, while Fullsend would still have | ||
| multiple ways to enable automatic merging. |
There was a problem hiding this comment.
This seems to be talking about two things.
First, it is not true that the agent merges the change here. It does evaluate it and give approval. And if CODE_AUTO_MERGE is set it will set the auto-merge flag for Github. But Github does the merging, and it only does so if all other conditions are met as well.
Second, if we kept this method only there would not be multiple ways to enable automatic merging. This seems to imply this method is rejected and should be removed because a different option has already been selected.
So it's not clear why this is really rejected.
ADR 0110 (fullsend-ai#7151) is being closed: no agent decides auto-merge. Say so in the context, keep the model-advisory rejection on its own grounds, and note that a model's verdict can be recorded for an accuracy eval. Signed-off-by: Guy Oron <goron@redhat.com>
|
Closing this PR as we have discovered different ideas behind how we should approach auto-merge, see experiments #8790 https://github.com/fullsend-ai/experiments/tree/main/0029-gemara-auto-merge |
Summary
Records the architectural decision for Fullsend-owned autonomous merging:
fullsendandagentsis an upfront design requirement.CODE_AUTO_MERGE*path will be retired rather than retained as a compatibility path.Scope
This PR intentionally contains only what is required to accept the architectural decision:
docs/architecture.md;docs/glossary.md; anddocs/problems/autonomy-spectrum.md.It adds no agent, credential, merge capability, policy schema, trigger, or runtime behavior.
Scope reduction
Earlier revisions included a detailed versioned implementation contract and user-adoption changes. Those documents were useful while exploring and testing the design, but they caused this ADR review to repeatedly evaluate field-level schemas, state machines, queue reconciliation, and rollout mechanics before the architectural decision itself could land.
They have therefore been intentionally removed from this PR. Follow-up design and implementation PRs will define the exact schemas, CEL/event triggers, two-phase merge-queue protocol, authenticated storage, reconciliation, conformance tests, and rollout gates in reviewable slices. Those follow-ups must preserve ADR 0110 authority boundary and support the merge queues required by the first dogfood repositories.
Related work
Validation
fullsend-ai/fullsendmainpre-commit run --from-ref origin/main --to-ref HEADSigned-off-bytrailer