Skip to content

K1: Add merge queue observation contract - #38

Merged
mchwang merged 6 commits into
mainfrom
codex/merge-queue-k1
Sep 24, 2026
Merged

mchwang merged 6 commits into
mainfrom
codex/merge-queue-k1

Conversation

@mchwang

@mchwang mchwang commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Problem

The guarded merge gate can detect that a branch uses a merge queue, but it cannot distinguish an actively queued PR from removal, queue failure, or a confirmed merge. A successful enqueue command therefore cannot safely serve as merge completion.

Result

Adds the K1 read-only GitHub adapter contract for merge-queue lifecycle observations:

  • maps QUEUED, AWAITING_CHECKS, LOCKED, and MERGEABLE entries to an active queued observation;
  • maps UNMERGEABLE to failed, preserves RemovedFromMergeQueueEvent.reason, and reports merged only from GitHub's confirmed PR state;
  • binds active entries and removal events to the exact reviewed pull request and head;
  • rejects replaced heads, stale removals, partial or malformed GraphQL responses, missing terminal reasons, and ambiguous absence;
  • bounds the read with one deadline and preserves caller cancellation even when the runner returns or rejects after cancellation;
  • uses literal gh api graphql argv and recorded response fixtures for every state.

This leaves merge queues blocked in the existing runner. K2 owns persisted lifecycle/retry guards and K3 owns enqueue/polling/UI integration and the remaining #24 race acceptance.

Closes no issue; contributes K1 to #24.

Validation

Exact pushed head: e1001abeaa89a59f056922b3198792423a24fecb

  • Failing before: the initial 11 focused tests failed because inspectQueue did not exist; later review regressions reproduced partial records, malformed merged timelines, late cancellation, generic abort errors, mismatched entry heads, and stale removal events.
  • npx vitest run test/merge.test.ts — 73 passed.
  • npm test — 12 files, 264 tests passed.
  • npm run typecheck — passed.
  • git diff --check — passed.
  • Two required CI runs passed on the exact head; CI includes typecheck, unit/integration, and 43 browser tests.
  • The exact GraphQL query succeeded against GitHub's live schema. Live enum introspection confirmed the five supported queue-entry states.

Ownership and dependencies

  • Baseline: b181d152ed522cf0ea6179d0ab7f0b45b00a6a4f
  • Owned implementation/test files: github/merge.ts, test/merge.test.ts
  • AGENTS.md contains the required reusable review lesson.
  • No overlap with active Lane D or E PRs.
  • K2/K3 remain blocked on their documented runner/web ownership handoffs.

Review record

  • Round 1: required all requested GraphQL fields before terminal returns; added omission regressions. Finding fixed, replied to, and resolved.
  • Round 2: validated timeline nodes before MERGED and discarded responses that arrive after cancellation. Both concrete concerns fixed; thread replied to and resolved.
  • Round 3: preserved the caller's cancellation reason when the runner rejects generically. Summary finding fixed with regression.
  • Round 4: added an exact literal-argv assertion for the GraphQL query and typed variables. No inline findings.
  • Round 5: bound queue entries and removal events to the reviewed PR/head and rejected stale removal history. Finding fixed, replied to, and resolved.
  • Round 6: Findings: None. Declined the summary-only MERGING suggestion because GitHub's live MergeQueueEntryState schema does not contain that value; unknown future states continue to fail closed.

No findings were otherwise declined. All review threads are resolved. The broadly reusable lesson is now explicit in AGENTS.md: validate every requested safety-critical field before any early return and distinguish omission from API-defined null. Existing stable-identity and async-cancellation rules cover the remaining fixes without duplicate rules.

Copilot AI lite review requested due to automatic review settings September 24, 2026 09:09

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

Unresolved findings affect partial-response validation and cancellation/removal correctness.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
What changed in this PR

Adds a read-only GitHub merge-queue observation contract for queued, failed, removed, and confirmed-merged states.

Changes:

  • Adds GraphQL queue inspection and lifecycle mapping.
  • Validates heads, responses, cancellation, and deadlines.
  • Adds focused fixtures and tests.
File Description
test/​merge.test.ts Covers queue lifecycle mappings, validation, timeout, and cancellation.
github/​merge.ts Implements merge-queue inspection and observation types.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread github/merge.ts Outdated
Copilot AI review requested due to automatic review settings September 24, 2026 09:14

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

Address the unresolved response-validation and cancellation-handling findings in github/merge.ts.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Check abort signal after GraphQL read

github/​merge.ts:260

The combined abort signal is only checked before the GraphQL read and in the rejection path. If the underlying run resolves after the timeout or caller cancellation (for example, a runner that does not honor AbortSignal), this code continues through the response mapping and can report a successful observation after cancellation. Check signal.aborted immediately after the awaited read and rethrow its reason before validating or returning the response.

Comment thread github/merge.ts
Copilot AI review requested due to automatic review settings September 24, 2026 09:19

Copilot AI 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.

Copilot review overview

🔵 Needs a closer look

Moderate issues remain in malformed-state validation and cancellation error propagation.

Review effort: Lite
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Preserve caller cancellation reason when runner rejects

github/​merge.ts:304

When the supplied runner rejects in response to the composite signal—as the default execFile-based runner does—the catch rethrows that runner error instead of options.signal.reason; the current test only passes because its fake runner explicitly rejects with the reason. A shutdown/caller cancellation can therefore surface a generic AbortError rather than the actionable cancellation reason. Handle caller cancellation before the timeout check and rethrow options.signal.reason when it is aborted.

Copilot AI review requested due to automatic review settings September 24, 2026 09:23

Copilot AI 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.

Copilot review overview

🔵 Needs a closer look

Queue tests should assert the exact gh api graphql argv and variable flags.

Review effort: Lite
Findings: None

Copilot AI review requested due to automatic review settings September 24, 2026 09:27

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

Unresolved head-identity validation and stale removal-event handling findings remain in github/merge.ts.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Avoid attributing stale removal events to the current PR head

github/​merge.ts:301

When mergeQueueEntry is absent, this treats the last removal event in the 20-item history as proof that the reviewed head was removed. A PR can have an old removal for a previous head and then be updated to the current reviewed head without ever being queued, which would be incorrectly reported as removed. Query and validate a stable entry/head identity on the queue events (or fail closed when it is unavailable) before returning this terminal state.

Comment thread github/merge.ts
Copilot AI review requested due to automatic review settings September 24, 2026 09:33

Copilot AI 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.

Copilot review overview

🔵 Needs a closer look

Handle GitHub’s MERGING queue state as a nonterminal observation.

Review effort: Lite
Findings: None

Resolved since last review (1)

@mchwang
mchwang merged commit c4c4618 into main Sep 24, 2026
3 checks passed
@mchwang
mchwang deleted the codex/merge-queue-k1 branch September 24, 2026 15:14
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.

2 participants