feat(api): record durable merge gate requests at apply drive tails

[aparajon]

Records a durable merge gate request at the moment an apply settles to terminal success, so the schema change that just landed on a target is captured before anything else can lose it. Builds on #867, which added the table and store; #866 adds the processor that drains what this records.

The recording runs at all three operator drive tails — recovery, multi-operation drive, and pending-stop recovery — placed ahead of pending control request completion, the same ordering the terminal summary already uses. Only terminal success records. A rollback is an ordinary apply row settling to completed, so it is covered without a special case.

  drive tail: apply settles to completed
        │
        │  no merge gate consumer registered ──▶ skip
        │  (no PR surface to refresh, nothing to drain the row)
        ▼
  record merge_gate_request              durable, idempotent per apply
        │
        ▼
  wake the merge gate processor          in-memory hint, loss-tolerant
        │
        ▼
  complete pending control requests / terminal summary

Gated on a consumer existing. A server with no code-host runtime has no PR check state to refresh and no processor to drain requests, so recording there would leave rows pending forever. The webhook handler's registration of OnMergeGateRecorded doubles as the consumer signal — a nil check, so a drive tail with no consumer pays nothing for the gate.

The wake-up is a hint, never the record. After a successful record the drive tail invokes the same callback so a co-located processor drains immediately instead of waiting out its poll interval. The durable row stays the source of truth: a lost wake-up costs poll latency, never the re-plan.

Recording never fails the drive. A storage error is logged with the apply's triage attributes and counted, and the request is left for the processor's backstop sweep over completed applies with no request row. A sustained sweep-sourced record rate is the operator signal that drive tails are failing to record.

Invariants

• Touches no registry entry. This PR writes no check state and changes no apply state transition; it appends to an outbox. The merge gate rule these rows serve is enforced end-to-end only once the processor consumes them, so it is established with feat(github): re-plan sibling PR checks when an apply changes a target schema #866 rather than here.
• The drive tail's own ordering constraint — record before completing pending control requests — is a property of this flow, not a new invariant: an operator's control request must not be acknowledged as finished before the schema change it produced is durably recorded.

The chain: #867 (storage) → #868 (drive-tail recording) → #866 (settle re-plan processor) → #939 (request kinds + hold storage) → #940 (preflight hold fan-out) → #941 (apply-start gate) → #942 (plan-time holds). Merges bottom-up; each PR retargets to main as its base merges.

🤖 Drafted by Armand's AI agent (Claude Fable 5)

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for pull/868, 56faf88.

Verdict: 6 findings — 3 non-blocking (legacy claim-path gap, untested gate branches, future-fact docs), 3 suggestions.

Non-blocking

1. The legacy apply-level claim path settles applies with no merge gate recording tail. When ShouldClaimOperations() is false (explicit operator_claim_operations: false, or the data-plane default: serve.go#L874 flips claiming to the apply level whenever GRPC_PORT is set and the key is unset, per config.go#L127), the whole-apply drive tail is operator.go#L280 _, _ = s.resumeClaimedApply(ctx, driverID, apply, 0, "") — and no recordMergeGateIfApplyResolved follows it. All three instrumented tails (:562, :744, :921) sit inside the operation-claim branch. Failure scenario: a GitHub-connected server in apply-level claim mode (post-feat(github): re-plan sibling PR checks when an apply changes a target schema #866, once a consumer is registered) drives an apply to Completed and records nothing at the tail; sibling PR checks stay stale until feat(github): re-plan sibling PR checks when an apply changes a target schema #866's backstop sweep, contradicting the PR body's "the moment it lands" claim and skewing the source="sweep" signal that metrics.go#L1526 documents as the drive-tail-failure indicator. Non-blocking because the path is dormant at this head (no production OnMergeGateRecorded registration) and the sweep gives eventual coverage — but either instrument the apply-level tail here or make feat(github): re-plan sibling PR checks when an apply changes a target schema #866's sweep-covers-this explicit.

2. The "only terminal success records" gate and its neighbors are untested. No test drives a non-Completed apply through recordMergeGateIfApplyResolved: the state gate at operator.go#L1064 if !state.IsState(apply.State, state.Apply.Completed) {, the dedup branch at :1089 if !recorded {, and the Record-error metric at :1086 are all unexercised — the unit test's only fixture is a Completed apply (merge_gate_record_test.go#L66) and its stub Record always returns (true, nil) (:32). Failure scenario: a future refactor swaps the gate to state.IsTerminalApplyState (the exact form of the adjacent predicate at operator.go#L984) and every failed/stopped/cancelled apply starts recording merge gates and triggering sibling re-plans — nothing in the suite fails, since Completed is terminal. The existing harness makes these one-line subtests.

3. New comments, ERROR logs, and metric docs assert stack-future machinery as present fact. operator.go#L1023-L1024 says "The handler registers OnMergeGateRecorded at construction" — but at this head webhook.NewHandler registers only OnApplyTerminalSummary (handler.go#L312); the only assignments are in tests. The ERROR logs at operator.go#L1055 ("the backstop sweep will record it") and :1084, plus metrics.go#L1539 and service.go#L181, reference a sweep and a processor with no implementation at this head. And merge_gate_integration_test.go#L31-L32's "apply drive tails in earlier tests record requests" is false today — earlier tests leave the callback nil, so their tails skip recording. Failure scenario: this merges bottom-up and feat(github): re-plan sibling PR checks when an apply changes a target schema #866 stalls or reverts; an operator debugging missing merge_gate_requests rows (or seeing the "backstop sweep will record it" ERROR) hunts for a registration and a sweep that don't exist. Cheap fix: soften to contract wording ("a consumer registers this to signal…"), or consciously accept as a fast-follow stack artifact.

General suggestions

• Each drive tail issues two back-to-back identical Applies().Get point reads of the same row on every pass. recordMergeGateIfApplyResolved reloads at operator.go#L1053 and completePendingControlRequestsIfApplyResolved reloads again at :977 — call pairs at :562/:567, :744/:746, :921/:923 — including non-terminal multi-op passes. Not a correctness bug (Record is idempotent, attribution fields immutable), but one shared reload or threading the projection's DerivedState would halve the reads and give both predicates one snapshot.
• clearMergeGateRequests closes a solely-owned *sql.DB with the discard form. merge_gate_integration_test.go#L38 defer func() { _ = db.Close() }() — AGENTS.md's Resource Cleanup rule mandates defer utils.CloseAndLog(db) for sole closers (the discard form is sanctioned only for redundant closers). Sibling tests in the package already follow the sanctioned pattern (e.g. apply_comment_integration_test.go:167), and this file will be the template for the rest of the stack's merge-gate tests.
• Rename residue in a new doc comment: operator.go#L1035-L1036 reads "…that target. The check / merge gate processor consumes…" — a stranded "check" from the "check refresh" → "merge gate" rename (the pre-rename text was "The check refresh processor"). No "check merge gate" component exists; drop "check".

The one thing that could have broken, verified

The riskiest mechanism is drive-tail coverage completeness combined with exactly-once recording: every path that newly settles an apply to terminal success must pass one of the three inserted recordMergeGateIfApplyResolved calls, and each apply must record and wake exactly once even under racing drivers. Verified both halves in the worktree:

• Exactly-once: proven safe. UNIQUE KEY idx_merge_gate_apply (apply_id) (merge_gate_requests.sql#L23) plus isDuplicateKeyError → (false, nil) (mysqlstore/merge_gate_requests.go#L58) and the recorded == false early return before s.OnMergeGateRecorded() (operator.go#L1089-L1098) make racing drivers single-record and single-wake; the unit test pins exactly-once wake (merge_gate_record_test.go#L99). Production registration ordering is race-free: serve.Build constructs the webhook handler (serve.go#L764) before Server.Start launches the operator (serve.go#L551).
• Coverage: proven complete for operation-level claiming, NOT fully closed overall. Enumerating all updateApplyStateFromOperations call sites plus the single-op engine resume shows the three uncovered sites (:633 pre-drive projection, :1156 already-terminal parent reconcile, :1182 post-MarkFailed derive) cannot produce a first-time Completed settle, and all three tails that can are instrumented. The residual gaps are (a) the legacy apply-level claim path (finding 1) and (b) a crash between the terminal write and the record call — both recoverable-by-design via the durable row plus feat(github): re-plan sibling PR checks when an apply changes a target schema #866's sweep, and both harmless at this head because with no production OnMergeGateRecorded registration the entire recording path ships dark. feat(github): re-plan sibling PR checks when an apply changes a target schema #866's review must verify the sweep actually covers apply-level-claimed applies and that the handler registers the callback at construction time, as this PR's comments promise.

Verified correct

• CI green: 32/32 checks pass at 56faf88.
• The diff is purely additive (zero deleted lines): no removed behavior, no invariant dropped, no tests deleted or rewritten.
• Drive-tail coverage is complete for operation-level claiming: all three tails that can newly settle an apply to Completed call recordMergeGateIfApplyResolved (:562 recoverSingleApplyOperation, :744 driveClaimedMultiOperation, :921 recoverApplyPendingStop), each placed after the terminal projection and before completePendingControlRequestsIfApplyResolved so a cleanup error cannot suppress the record — mirroring the publishTerminalSummaryIfWon ordering.
• Recording never fails the drive: every error path in operator.go#L1053-L1088 logs with apply LogAttrs + driver, counts RecordMergeGateRecordFailure only on storage errors, and returns without propagating; no silent branch (every early return logs Debug or Error), matching the established never-fail-tail pattern.
• The Completed gate uses state.IsState, which normalizes proto/uppercase/lowercase forms (state/apply.go#L331), so no settle is missed to case drift; the Reverted exclusion is correct (tern revert-window restores the pre-apply schema), and user-facing rollbacks ARE recorded (they settle as ordinary Completed apply rows).
• Record's storage validations are satisfied by the freshly reloaded row; Provider is correctly left to the store's github default (mysqlstore/merge_gate_requests.go#L42-L44); the full field mapping matches the feat(storage): durable merge gate requests for schema-mutating applies #867 schema and the integration test asserts every field plus MergeGatePending state; requested_by varchar(255) matches applies.caller varchar(255), so no truncation path.
• ChangeKeyForPullRequest (types.go#L1425) honors the base feat(storage): durable merge gate requests for schema-mutating applies #867 contract exactly — strconv.Itoa(pr), "" for pr <= 0 meaning "exclude nothing" for CLI/gRPC applies — single caller, no duplicated rendering.
• The nil consumer gate (hasMergeGateConsumer, operator.go#L1027) is the right altitude: gRPC/CLI-only servers correctly record nothing (rows would sit pending forever), and the unit test pins both sides of the gate.
• New metrics follow pkg/metrics conventions exactly (naming, annotated-unit counter strings, EnvironmentAttribute); MergeGateSourceSweep being defined-but-unused at this head is legal forward-compat consumed by feat(github): re-plan sibling PR checks when an apply changes a target schema #866, not a forward dependency. No forward symbol dependencies on later stack PRs: every referenced symbol resolves at 56faf88.
• Integration test structure is sound: separate EventuallyWithT loops tolerate the window between the apply's terminal write and the tail's record; clearMergeGateRequests's DELETE over the shared table is safe at this head (no other test asserts on merge_gate_requests). The claimed post-StartOperator registration data race was refuted: the only apply is inserted after the field write through the same *sql.DB pool (happens-before via the pool mutex), matching the established post-start OnApplyTerminalSummary idiom.
• Repo conventions otherwise followed: state comparisons via pkg/state constants, LogAttrs() on all apply-scoped logs once the row loads, testify + t.Context() + named poll deadline in tests, required scenario comment on the integration test, no "migration" terminology, feat(api)/refactor(api) commit scoping, and no TEMPLATES.md regeneration needed.

This review was generated by Claude Code (claude-fable-5).

[morgo]

🤖 Approving on Morgan's behalf (automated review).

Additive (+397/-0), CI green across the full suite, and the design decisions I probed hold up:

• The consumer gate is the right shape. Using the webhook handler's OnMergeGateRecorded registration as the "is there a GitHub runtime" signal means a gRPC/CLI-only server never records rows that nothing would ever drain, and it costs a nil check on the drive tail. Reusing an existing callback as the capability probe rather than adding a parallel config flag is the less error-prone option — the two can't drift out of sync.
• Placement before control-request cleanup at all three tails, with the comment explaining that a cleanup error must not suppress the record. That's the same ordering publishTerminalSummaryIfWon already uses, so it's consistent rather than ad hoc.
• Recording never fails the drive — reload error, nil apply, and Record error all log and return, with RecordMergeGateRecordFailure counted. A guardrail that can fail the thing it guards is worse than the gap it closes; this gets that right.
• The wake-up is explicitly loss-tolerant and the durable row stays the source of truth. Worth noting the !recorded branch returns before OnMergeGateRecorded(), so a pod that records and dies before waking leaves the request to poll latency on any later re-drive — which is exactly what the doc says it's willing to pay, so it's disclosed rather than accidental.
• Reloading the apply because "the derived-state write operates on a copy and does not mutate the caller's row" is a real trap avoided, not boilerplate.

One finding: a partially-succeeded apply mutates the target schema and records nothing.

if !state.IsState(apply.State, state.Apply.Completed) {
	// Only terminal success mutates the target schema; every other outcome
	// (still running, stopped, cancelled, failed, reverted) leaves sibling
	// plan checks accurate.
	return
}

The gate is right; the stated justification is what doesn't hold. Checking the derivation in pkg/state/apply.go:

if counts[Apply.Failed] > 0 {
	return Apply.Failed
}
...
if counts[Apply.Completed] == total {
	return Apply.Completed
}

Failed needs one failed task. Completed needs all tasks complete. So a ten-table apply where nine tables cut over and the tenth fails settles to Failed — and nine tables' live schemas have changed on the target. Same structure for Cancelled and Stopped (single-task-wins, checked before the all-complete rule), and an operator stopping a multi-table apply midway is not an exotic scenario.

The consequence lands squarely on this stack's premise: every sibling PR planning against those nine tables holds check state computed against a schema that no longer exists, and no durable breadcrumb is left. It's precisely the failure the PR opens by describing, reached through the partial path instead of the terminal-success one.

I'd be cautious about simply widening the state test — recording on Failed would fan out re-plans for applies that genuinely changed nothing, which has its own cost. The honest fix is to key on whether any task completed rather than on the apply's rolled-up state, since task completion is what actually mutates the target. If the intended scope really is terminal success only, the comment is the thing to fix: it currently asserts something about the other states that the derivation contradicts, and that's the kind of comment a later reader builds on.

Related, and I couldn't settle it from this diff alone: the backstop sweep is described as re-recording "anything missed here." Whether it re-records partial applies depends on whether it selects on completed applies or on completed tasks — worth confirming, because if it selects on apply state it inherits this same gap rather than covering it.

Nit, non-blocking: ChangeKeyForPullRequest renders a bare PR number with no repository qualifier. Repository is a sibling field on the row so the composite identity is fine — just flagging that the key is only meaningful paired with it, in case it ever travels alone.

Not blocking.