Skip to content

Default merge submission to direct - #407

Merged
justin808 merged 8 commits into
mainfrom
jg-codex/direct-merge-default
Aug 11, 2026
Merged

Default merge submission to direct#407
justin808 merged 8 commits into
mainfrom
jg-codex/direct-merge-default

Conversation

@justin808

@justin808 justin808 commented Aug 11, 2026

Copy link
Copy Markdown
Member

Why

Most consumer repositories do not use GitHub Merge Queue, but the portable merge-submission default currently assumes queue-only behavior. That makes an omitted seam reject normal direct merges and prevents consumers from explicitly declaring the intended non-queue path.

What changed

  • Add merge_submission.mode: direct as a closed, validated policy mode.
  • Make direct the portable default when the seam is omitted and the initializer default for new consumers.
  • Submit direct merges through GitHub's expected-head-bound mergePullRequest mutation only when live queue control is disabled.
  • Fail before mutation when direct mode encounters a queue-enabled repository, requiring an explicit queue-capable seam instead of silently changing routes.
  • Preserve merge_queue_only and merge_queue_or_guarded_direct behavior, including explicit queue fixtures for queue-specific tests.
  • Update the seam doctor, workflow contract, adoption docs, and changelog.

Impact

After this prerequisite is released, consumer repositories can opt out of Merge Queue in .agents/agent-workflow.yml with:

merge_submission:
  mode: direct

This PR does not change any GitHub repository setting and does not enable or disable Merge Queue.

Validation

  • bin/validate at e75dd6126fbc810fc93c893d6d90737e9dde7f85 — passed with exit code 0, including PASS agent-workflows validation
  • ruby skills/pr-batch/bin/pr-merge-submit-test.rb --verbose — 97 runs, 807 assertions, 0 failures/errors/skips
  • ruby -c skills/pr-batch/bin/pr-merge-submit — syntax OK
  • git diff --check origin/main...HEAD — clean
  • Independent full-diff checker at the exact head — no BLOCKING or DISCUSS findings
Agent details

Decision log

  • Keep GitHub repository settings out of scope; this PR changes only the agent-workflow seam and its implementation, tests, and documentation.
  • Use direct as the portable omitted-seam default while retaining explicit merge_queue_only and guarded-direct modes.
  • Fail closed when a repository is queue-enabled but its trusted seam requests direct submission.
  • Treat the review finding about the reassigned SOURCE_REPO_POLICY sentinel as Must-Fix. Commit 5e1af5ee restores the real-repository trusted-policy path and makes 24 queue-specific fixture call sites explicit.
  • Accept the direct-default behavior shift as the intended, user-approved policy decision rather than a review blocker.

Review outcomes

  • Must-Fix sentinel regression: fixed in 5e1af5ee; focused and full validation passed; review thread resolved.
  • Cosmetic extra blank line in the fake gh fixture: removed in 5e1af5ee.
  • CodeRabbit queue-capable-mode documentation finding: fixed in b73663c7; both affected docs now describe both canonical-enqueue modes; review thread resolved.
  • Claude direct GraphQL-error and invalid-response coverage findings: fixed with five tests and six fixture modes in e75dd612; both overlapping threads resolved without a production change.
  • Claude direct-default behavior-shift observation: accepted as the intended, documented policy decision; thread replied to and resolved.

QA Evidence

  • QA lane: /root/pr407_closeout_audit, read-only independent checker; candidate worktree /Users/justin/.codex/worktrees/agent-workflows-merge-default; completed on exact head
  • Scope checked: full 10-file origin/main...e75dd6126fbc810fc93c893d6d90737e9dde7f85 diff, direct-default and queue-only routing, trusted-policy sentinel repair, direct error-path reconciliation coverage, queue-capable-mode documentation, changelog, and validation coverage
  • Tested at: PR Default merge submission to direct #407 head e75dd6126fbc810fc93c893d6d90737e9dde7f85
  • Automated checks: full bin/validate; focused merge helper suite 97 runs/807 assertions; seam doctor suite 280 runs/2405 assertions; Ruby syntax; git diff --check
  • Manual checks: not applicable: deterministic CLI and policy change covered by exact-head unit, integration, schema, and drift tests
  • User-visible UI change: no
  • Visual evidence: not applicable: no user-visible UI change
  • Interaction change: no; not applicable: no UI interaction change
  • Interaction evidence: not applicable: no UI interaction change
  • Visual fix: no; not applicable: no visual fix
  • Negative control: not applicable: no visual fix
  • Performance evidence: not applicable: policy and CLI control-flow change with no rendered-page, asset, bundle, or runtime performance seam
  • Findings: none; prior review findings fixed in 5e1af5ee, b73663c7, and e75dd612
  • QA required: yes
  • QA required rationale: developer-tooling and broad merge-control behavior required independent full-diff review plus exact-head deterministic validation
  • QA lane status: satisfied
  • Release-blocking status: clear
  • Process-gap disposition: checklist+replay

Audit receipts

Completed-batch audit

Status: Clean — no outstanding findings or follow-ups. Durable receipt.

…-default

* origin/main:
  Fix locale-dependent test, surface policy-only CLAUDE.md follow-ups, add read-only seam-drift audit (#337)
…-default

* origin/main:
  Enforce requested-versus-observed model route provenance (#348)
@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The PR makes direct the default merge submission mode. It adds expected-head-bound GraphQL merging for queue-disabled bases, preserves explicit queue modes, updates seam validation and documentation, and expands direct and queue reconciliation tests.

Changes

Merge submission policy

Layer / File(s) Summary
Policy defaults and validation
.agents/agent-workflow.yml, bin/agent-workflow-seam-doctor, bin/agent-workflow-seam-doctor-test.rb, docs/adoption.md, docs/seam-design.md, skills/pr-batch/SKILL.md, CHANGELOG.md
direct is now an accepted and default submission mode. Queue-only and guarded-direct modes remain explicit alternatives.
Direct merge routing and reconciliation
skills/pr-batch/bin/pr-merge-submit, workflows/pr-processing.md
Queue state and policy now control routing. Queue-disabled bases use expected-head-bound direct merging. Queue-enabled bases reject direct mode or use explicit queue submission. Direct responses and errors are reconciled against live state.
Direct submission test coverage
skills/pr-batch/bin/pr-merge-submit-test.rb
Tests cover direct defaults, queue-state races, transport failures, successful mutations, and exact merge reconciliation.
Queue policy fixtures and regression coverage
skills/pr-batch/bin/pr-merge-submit-test.rb
Queue tests use explicit policies and dynamic repository metadata. Enqueue idempotency, base races, replacement entries, timeouts, and interrupt handling remain covered.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related issues

  • shakacode/agent-coordination#114 — The PR implements the direct merge_submission policy and direct-merge path described by the issue.

Possibly related PRs

Suggested reviewers: ihabadham

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 2.22% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the primary change: making direct merge submission the default.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch jg-codex/direct-merge-default

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Comment thread skills/pr-batch/bin/pr-merge-submit-test.rb Outdated
@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown

Review summary

What this PR does: Adds merge_submission.mode: direct as a new, validated policy mode and makes it the portable default when the seam is omitted (replacing the previous merge_queue_only default). Direct mode submits merges via GitHub's expected-head-bound mergePullRequest GraphQL mutation, re-fetching and re-validating PR state immediately before mutation, and fails closed (pre-mutation) if the live repository turns out to be queue-controlled. merge_queue_only and merge_queue_or_guarded_direct behavior is preserved. Docs, changelog, seam doctor, and both test suites are updated in step.

Correctness / Test coverage — bug found

Left an inline comment on skills/pr-batch/bin/pr-merge-submit-test.rb (around lines 1406–1417). The new reassignment of the merge_submission local variable before the existing merge_submission.equal?(SOURCE_REPO_POLICY) check makes that check permanently false afterward, so the "run against the real repository" branch in run_cli is now dead code. I confirmed this by diffing against origin/main's prior version of the file, where the identity check was the only occurrence and the real-repo branch was live.

Since 75 of the 77 run_cli(...) call sites in this file rely on the default (merge_submission: SOURCE_REPO_POLICY), this means nearly the entire suite silently stopped dogfooding this repo's actual committed .agents/agent-workflow.yml (and the git-blob trust-reading path that resolves it) and now runs against a synthetic prepare_consumer_repo fixture instead. No assertions fail because the synthetic fixture is self-consistent — which is presumably why the PR's validation section still shows 92 runs, 765 assertions, 0 failures — but the specific integration guarantee this suite existed to provide (that the real, shipped policy resolves correctly through the real trust path) is now silently gone, in the same PR that flips this repo's own default from merge_queue_only to direct. Worth fixing before merge, e.g. by capturing the sentinel check once into a local flag before any reassignment.

Design/security observation (not a bug, but worth surfacing)

Making direct the zero-config default is a real behavior shift: previously, submitting a merge outside a merge queue required an explicit, audited opt-in (merge_queue_or_guarded_direct, with a required non-atomic-base acknowledgement + rationale). Now a consumer that does nothing gets a live mergePullRequest mutation by default. The PR compensates well for this — expected-head-bound mutation, an immediate re-fetch/re-validate directly before the mutation, fail-closed pre-mutation errors if the queue state changed, and no auto-merge/queue-enable anywhere — so this reads as a deliberate, reasonably safe tradeoff given the stated goal ("most consumer repos don't use Merge Queue"). Flagging it explicitly so it's a conscious call by maintainers, not something that slips in under the "just a default value" framing.

Other notes

  • direct_merge_result, validate_merged_pr!, reconcile_direct_graphql_errors!, and the route: threading through reconcile_mutation_outcome! all look correct on close reading — mirrors the existing enqueue_result/guarded-direct reconciliation patterns, correctly distinguishes pre-mutation config errors (exit 1) from post-mutation ambiguous outcomes (exit 2/UnknownOutcome), and the GraphQL enum casing (options[:method].upcase) matches GitHub's PullRequestMergeMethod values.
  • bin/agent-workflow-seam-doctor and its test file were updated consistently with the new mode set; the "unknown mode" test fixtures were correctly changed from "direct" to "unknown" now that direct is valid.
  • Docs (docs/adoption.md, docs/seam-design.md, workflows/pr-processing.md, skills/pr-batch/SKILL.md) accurately reflect the new default and mode semantics.
  • Minor style nit: an extra blank line was introduced before the mergePullRequest stub handling in fake_gh (pr-merge-submit-test.rb) — cosmetic only.

Overall: the core production logic in pr-merge-submit looks solid and well-guarded. The main actionable item is the dead-branch test bug above.

…-default

* origin/main:
  Fix <PROJECT> digit ambiguity, anchor section extraction, ship coordination gate (#339)
@justin808

Copy link
Copy Markdown
Member Author

Review follow-up summary

Review scope: full available PR history (no previous address-review cutoff was present).

Fixed

  • The SOURCE_REPO_POLICY sentinel was reassigned before the repository-selection identity check, silently bypassing the real-repository trusted-policy path. Fixed in 5e1af5ee; the sentinel identity is captured once, queue-specific fixtures are explicit, and the exact combined head passed 92 runs / 765 assertions plus full bin/validate. The inline thread was replied to and resolved.
  • Removed the adjacent cosmetic blank line in the fake gh fixture.

Decision retained

  • The default changes from queue-only to direct while explicit queue and guarded modes remain available. This is the intended policy change for repositories that do not use Merge Queue; direct mode still fails closed if live queue control is enabled.

No action required

  • CodeRabbit's draft-status comment contained no code finding. Current-head automated review will be allowed to run after the PR leaves draft.

Current head: ac92ac1a959d87b9aebe6d5a20029ddf9619f9a0.

@justin808
justin808 marked this pull request as ready for review August 11, 2026 04:22

@coderabbitai coderabbitai 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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/adoption.md`:
- Around line 95-109: Update the queue policy documentation to state that
queue-enabled repositories must select either merge_queue_only or
merge_queue_or_guarded_direct, since both use canonical enqueue on queue-enabled
bases. In docs/adoption.md lines 95-109, revise the current
merge_queue_only-only wording; make the corresponding requirement update in
docs/seam-design.md lines 175-179. Preserve the existing descriptions of guarded
direct exceptions and queue-disabled behavior.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c405e351-7ea8-4f4e-842e-bf6e94e1eadd

📥 Commits

Reviewing files that changed from the base of the PR and between 40bd1af and ac92ac1.

📒 Files selected for processing (10)
  • .agents/agent-workflow.yml
  • CHANGELOG.md
  • bin/agent-workflow-seam-doctor
  • bin/agent-workflow-seam-doctor-test.rb
  • docs/adoption.md
  • docs/seam-design.md
  • skills/pr-batch/SKILL.md
  • skills/pr-batch/bin/pr-merge-submit
  • skills/pr-batch/bin/pr-merge-submit-test.rb
  • workflows/pr-processing.md

Comment thread docs/adoption.md
Comment thread skills/pr-batch/bin/pr-merge-submit
Comment thread skills/pr-batch/bin/pr-merge-submit
@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown

Review summary

Reviewed the merge_submission.mode: direct default change (skills/pr-batch/bin/pr-merge-submit, bin/agent-workflow-seam-doctor, docs/tests).

Overall: the core logic is sound and internally consistent — submit, direct_merge_result, validate_merged_pr!, and the pre-mutation queue rechecks correctly mirror the existing guarded_direct narrowing pattern (re-fetch → validate receipt bindings → validate PR → recheck queue state → mutate), and the injection-safety properties (reject_leading_at!, host/URL validation, whitelisted GraphQL enum values) carry over correctly to the new mergePullRequest mutation. Docs (docs/adoption.md, docs/seam-design.md, workflows/pr-processing.md, skills/pr-batch/SKILL.md) and the seam-doctor validator were updated consistently and I didn't find stale references to the old merge_queue_only-as-default behavior. Test fixtures that previously relied on the source repo's own (now-changed) default were correctly patched to pass explicit merge_queue_policy where needed.

Two things worth a look before merge, posted as inline comments:

  1. Untested error pathreconcile_direct_graphql_errors! (the handler for a GraphQL-level error from the new mergePullRequest mutation) has zero test coverage. No fixture returns a GraphQL errors body for the merge mutation (only a raw transport failure is simulated), unlike the analogous enqueue_graphql_error* fixtures for the queue path. This also hides an asymmetry: the same post-failure "PR is now queue-enabled" race is classified as a deterministic Error (exit 1) here but as UnknownOutcome (exit 2) in the sibling transport-failure handler.

  2. Default behavior change for existing consumers — the new default (mode: direct) performs the same non-atomic-expected-base-OID merge that previously required an explicit guarded_direct.non_atomic_base.acknowledged/rationale opt-in, but now with zero configuration. This is clearly the intent of the PR and is disclosed in the docs/changelog, but it does mean any existing queue-disabled consumer with no merge_submission key goes from "fails closed with a config error" to "performs a real merge" on upgrade, with no config change on their end — worth a deliberate double-check that this is well-flagged for downstream consumers.

No security vulnerabilities (injection, auth bypass, etc.) or correctness bugs found in the core submission logic itself.

Comment thread skills/pr-batch/bin/pr-merge-submit-test.rb
@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown

Review summary

Reviewed the diff introducing merge_submission.mode: direct as the new portable default and pr-merge-submit's expected-head-bound mergePullRequest submission path.

Overall: the implementation is careful and consistent with the existing security posture of this file:

  • Fail-closed ordering is preserved — every entry point re-checks live isMergeQueueEnabled/isInMergeQueue and raises a deterministic pre-mutation Error (not UnknownOutcome) when a queue-enabled repo is misconfigured for direct mode, both at the initial submit() dispatch and again immediately before the mutation in direct_merge_result (narrowing the TOCTOU window the same way guarded_direct_result already does).
  • validate_merged_pr! re-verifies host, URL, MERGED state, merged: true, a well-formed merge commit oid, and exact base/head match before trusting a "successful" mutation response — it doesn't just trust a 0 exit status.
  • The existing @" / leading-@ guard on --subject/--body still applies to the new mutation's commitHeadline/commitBody inputs.
  • The self-referential fixture bug (tests using the SOURCE_REPO_POLICY sentinel, which now resolves to direct for this repo instead of the old merge_queue_only default) was caught and fixed across ~24 call sites, and the doctor/init defaults, docs, and CHANGELOG were all updated in lockstep with the code.

One finding posted inline: the new mergePullRequest GraphQL-errors-in-200-response path (reconcile_direct_graphql_errors!) and the validate_merged_pr!-failure reconciliation branch have no dedicated test fixtures, unlike their enqueuePullRequest counterparts (enqueue_graphql_error* modes) which are thoroughly covered. Given the rest of this PR's branch coverage is exhaustive, this looks like a gap worth closing rather than an intentional omission.

No security or correctness issues found in the core merge-submission logic itself.

@justin808

Copy link
Copy Markdown
Member Author

Review follow-up summary

Review scope: feedback since the previous address-review summary at 2026-08-11T04:21:01Z.

Fixed

  • CodeRabbit correctly noted that both merge_queue_only and merge_queue_or_guarded_direct use canonical enqueue on queue-enabled bases. Commit b73663c7 updates docs/adoption.md and docs/seam-design.md to describe both queue-capable modes while preserving direct-mode failure and guarded-direct semantics. Full bin/validate passed on the exact head; the inline thread was replied to and resolved.

No additional findings

  • Claude's current-head review found the implementation sound and internally consistent.

Current head: b73663c73da8444f6367aa15a3ca61c7a4e1f3ae.

Comment thread skills/pr-batch/bin/pr-merge-submit
@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown

Review summary

Reviewed the change that adds a portable merge_submission.mode: direct and makes it the default when the seam is omitted, backed by GitHub's expected-head-bound mergePullRequest mutation. The implementation is careful in the places I'd worry most about:

  • Race safety: direct_merge_result re-fetches the PR and re-validates receipt bindings, base, head, and live queue state at the narrowest practical point immediately before mutation (mirroring the existing guarded_direct_result pattern), and there's an explicit test (test_direct_mode_rechecks_queue_control_immediately_before_mutation) proving a queue-enabled race is caught before any mutation is attempted.
  • Injection safety: the new -f expectedHeadOid=... / mergeMethod=... / commitHeadline=... / commitBody=... args reuse the same Process.spawn array-argv (no shell) and the existing reject_leading_at! guard against gh's "@ means read a file" behavior.
  • Post-mutation verification: validate_merged_pr! checks URL/host, state == MERGED, merged == true, a valid merge-commit OID, and that both base and head match expectations before trusting the result — it doesn't just trust an HTTP 200.

One correctness finding left as an inline comment: reconcile_direct_graphql_errors! (the handler for a GraphQL-level error response from the mergePullRequest mutation) treats "merge queue is merely enabled" and "this PR is already an exact member of the queue" identically, both producing a deterministic exit-1 config error. Elsewhere in this same file (reconcile_mutation_outcome!(route: :direct), reconcile_enqueue_graphql_errors!), actual queue membership after an ambiguous mutation is deliberately treated as UnknownOutcome (exit 2, "do not retry blindly") instead. This new method also has no test coverage — the added direct_transport_* fixtures only exercise the transport-failure path, not a GraphQL errors body for the merge mutation.

Worth a maintainer gut-check (not a bug, called out explicitly in the PR description as an intended decision): this PR flips the portable default from fail-closed (merge_queue_only — an omitted seam previously made queue-disabled submission a hard error before any mutation) to fail-open (direct — an omitted seam now performs a real direct merge). Any consumer repo that never configured merge_submission will, after upgrading, start having PRs merged automatically where they previously got a deterministic stop. That's the intended point of the PR, but it's a real behavior change in production merge automation for every such repo, so it's worth making sure downstream consumers are aware before this ships.

Docs (docs/adoption.md, docs/seam-design.md, workflows/pr-processing.md, skills/pr-batch/SKILL.md), the seam doctor (bin/agent-workflow-seam-doctor), and CHANGELOG were all updated consistently with the implementation — I didn't find any place still describing the old queue-only default.

@justin808

Copy link
Copy Markdown
Member Author

Review follow-up summary

Review scope: final consolidated sweep after the previous summary at 2026-08-11T04:45:24Z, including unresolved threads created earlier but not yet triaged.

Fixed

  • Two overlapping Claude threads identified missing direct-merge GraphQL-error and invalid-success-response coverage. Test-only commit e75dd612 adds five tests and six fake-gh modes covering exact-merge reconciliation, deterministic queue-configuration errors after a proved GraphQL rejection, unresolved unknown outcomes, and invalid-response reconciliation. Both threads were replied to and resolved. Focused suite: 97 runs / 807 assertions; full bin/validate passed.

Decision retained

  • The omitted-seam direct-default behavior shift is intentional and documented. Existing pinned consumers do not change until they update the workflow pack. The reviewer-awareness thread was replied to and resolved.

Final review state

  • All known inline review threads are resolved.
  • Independent exact-head checker: no BLOCKING or DISCUSS findings.

Current head: e75dd6126fbc810fc93c893d6d90737e9dde7f85.

Comment thread skills/pr-batch/bin/pr-merge-submit-test.rb
Comment thread skills/pr-batch/bin/pr-merge-submit
@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown

Review: Default merge submission to direct

Overview
Adds a third merge_submission.mode (direct) to the portable merge seam, alongside the existing merge_queue_only and merge_queue_or_guarded_direct. direct becomes the portable default when the seam is omitted and the --init default for new consumers. Direct submission goes through GitHub's expected-head-bound mergePullRequest GraphQL mutation, only when the live PR proves isMergeQueueEnabled == false and isInMergeQueue == false at re-fetch time immediately before mutation; a queue-enabled repository under mode: direct fails deterministically (exit 1) before any mutation, telling the caller to opt into merge_queue_only/merge_queue_or_guarded_direct. merge_queue_only and merge_queue_or_guarded_direct behavior is preserved (test fixtures were updated to pass merge_queue_policy explicitly now that the omitted-seam default changed). Seam doctor validation, --init, docs (docs/adoption.md, docs/seam-design.md, workflows/pr-processing.md, skills/pr-batch/SKILL.md), and the changelog were all updated in lockstep.

Code quality / correctness

  • The new direct_merge_result mirrors the existing guarded_direct_result pattern well: re-fetch PR state immediately before mutation, re-validate receipt bindings and queue-disabled state at the narrowest point, then mutate and reconcile ambiguous outcomes (transport failure, in-band GraphQL errors, and malformed mutation responses each have dedicated, tested reconciliation paths that only ever confirm success on an exact head/base/merge-commit match).
  • expectedHeadOid is bound to a value already validated as a full commit SHA at option-parsing time, and commitHeadline/commitBody go through the existing reject_leading_at! guard against gh's @file field-value convention — consistent with how the pre-existing enqueue/guarded-direct paths handle the same inputs.
  • Post-merge validation (validate_merged_pr!) checks exact PR URL (host + owner/repo + number), state == MERGED, merged == true, a well-formed 40-hex merge-commit OID, base branch name, and case-insensitive head OID match — matching the rigor of exact_merge? used elsewhere.
  • The TOCTOU window between the submit dispatcher's initial queue-state read and the actual mutation is closed by re-fetching and re-checking queue state inside direct_merge_result right before calling gh; this is exercised by test_direct_mode_rechecks_queue_control_immediately_before_mutation, which asserts exactly two PR queries.
  • The PR body notes a self-caught Must-Fix (a reassigned SOURCE_REPO_POLICY sentinel) was fixed in a follow-up commit, and queue-specific test call sites were made explicit — confirmed those call sites in the diff now pass merge_queue_policy explicitly, which looks correct.
  • One thing worth a second look (also left as an inline comment): reconcile_direct_graphql_errors! treats bare isMergeQueueEnabled/isInMergeQueue truthiness as a deterministic Error (exit 1), while the sibling reconcile_mutation_outcome!(route: :direct) requires an exact queue-entry match before doing anything (and then raises UnknownOutcome, exit 2) for what is conceptually the same 'queue appeared where direct mode didn't expect it' situation. Not clearly wrong, but the differing exactness bar and exit-code semantics between the two reconciliation paths could use either alignment or a comment explaining the intentional difference.

Security
No repository settings are touched (as the PR description states) — this is purely client-side routing logic in the trusted helper script. direct_merge_args only interpolates already-validated inputs (SHA-shaped head, allow-listed merge method, @-guarded subject/body) into gh api graphql -f field values, so no injection surface beyond what already existed for the enqueue/guarded-direct paths. Fail-closed behavior is preserved and, if anything, strengthened: a queue-enabled repo under direct mode cannot silently fall through to a real merge or a real enqueue — it hard-fails before any mutation is attempted.

Test coverage
Extensive new coverage for the direct-mode dispatch, queue-configuration rejection (including the race-recheck case), transport-failure reconciliation, in-band GraphQL error reconciliation, and malformed-response reconciliation. Gap (also left as an inline comment): the SIGINT-during-mutation interrupt tests (run_cli_with_interrupt) still only exercise the enqueuePullRequest mutation; there's no equivalent case with wait_for: "mergePullRequest" for the new direct-merge mutation, even though it shares the same cancellation/@mutation_attempted machinery and is now the default path for most consumers.

Performance
No meaningful performance implications — same GraphQL-based read-then-mutate-then-reconcile shape as the existing queue path, at most one extra PR re-fetch which is already how the guarded-direct path behaves.

Summary
This is a well-scoped, carefully fail-closed change with strong test coverage and docs kept in sync with behavior. The two items above (asymmetric queue-detection reconciliation semantics, and missing interrupt-test coverage for the new mutation) are non-blocking suggestions rather than confirmed bugs.

@justin808

Copy link
Copy Markdown
Member Author

Final review follow-up summary

Review scope: final-head feedback after the previous summary at 2026-08-11T05:06:13Z.

Decision retained

  • Structured GraphQL errors and transport failures intentionally have different outcomes. A structured error proves the direct mutation was rejected, so newly observed queue control is a deterministic configuration error; a transport failure is ambiguous and exact queue membership remains UnknownOutcome. Commit e75dd612 covers and pins both paths.
  • Additional direct-mutation SIGINT tests were not added. The signal and cancellation behavior is shared mutation-agnostic run_process machinery already covered by three subprocess-heavy interrupt tests; direct mutation and reconciliation branches have dedicated current-head coverage.

Final review state

  • All inline review threads are resolved.
  • Independent exact-head checker found no BLOCKING or DISCUSS findings.
  • Current head: e75dd6126fbc810fc93c893d6d90737e9dde7f85.

@justin808

Copy link
Copy Markdown
Member Author

Completed-batch audit: replay evidence follows.

@justin808
justin808 added this pull request to the merge queue Aug 11, 2026
Merged via the queue into main with commit 845ab02 Aug 11, 2026
18 checks passed
@justin808
justin808 deleted the jg-codex/direct-merge-default branch August 11, 2026 06:58
@justin808

Copy link
Copy Markdown
Member Author

Completed-batch audit: replay evidence follows.

justin808 added a commit that referenced this pull request Aug 11, 2026
…8-batch-docs-status-skill

* origin/main:
  Default merge submission to direct (#407)
  Fix <PROJECT> digit ambiguity, anchor section extraction, ship coordination gate (#339)
  Enforce requested-versus-observed model route provenance (#348)
  Fix locale-dependent test, surface policy-only CLAUDE.md follow-ups, add read-only seam-drift audit (#337)
  Add PR #377 changelog entry (#382)
  Make PR descriptions human-first (#377)
  Remove unsupported signed-launch enforcement (#374)

# Conflicts:
#	bin/validate
justin808 added a commit that referenced this pull request Aug 11, 2026
…gn-implementation

* origin/main:
  Harden coordination parsing after PR #339 audit (#410)
  Change merge submission mode to merge_queue_only (#412)
  Scope archive-readiness, add retry heartbeat and launch modes, ship the batch-status skill (#362)
  Default merge submission to direct (#407)
  Fix <PROJECT> digit ambiguity, anchor section extraction, ship coordination gate (#339)
justin808 added a commit that referenced this pull request Aug 12, 2026
…itable-controls

* origin/main:
  Suppress unchanged Goal monitor wakes (#395)
  Document single-target plan-pr-batch routing (#405)
  Require exact-head hosted QA before merge (#291)
  Change merge mode from queue to direct (#415)
  Harden coordination parsing after PR #339 audit (#410)
  Change merge submission mode to merge_queue_only (#412)
  Scope archive-readiness, add retry heartbeat and launch modes, ship the batch-status skill (#362)
  Default merge submission to direct (#407)
  Fix <PROJECT> digit ambiguity, anchor section extraction, ship coordination gate (#339)
  Enforce requested-versus-observed model route provenance (#348)

# Conflicts:
#	CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant