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

[aparajon]

Why this matters: The moment a drive settles an apply to terminal success, every other open PR planning against that target holds check state computed against a schema that no longer exists. If that moment isn't captured durably, the merge gate guardrail has nothing to act on — and a pod crash right after the terminal write would lose the signal forever. Stack 2/7, on top of #867.

What it does:

• recordMergeGateIfApplyResolved runs at all three operator drive tails (recovery, multi-operation drive, pending-stop recovery), placed before pending control requests are completed — the same ordering the terminal summary uses. Only terminal success records; rollbacks are ordinary apply rows settling to Completed, so they're covered with zero special-casing.
• Gated on a merge gate consumer existing. A gRPC/CLI-only server (no GitHub 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: drive tails record only when it is set — a nil check, so the drive tail pays nothing for it.
• OnMergeGateRecorded also serves as the wake-up: the drive tail invokes it after a successful record so a co-located merge gate processor drains immediately instead of waiting out its poll interval. The durable row stays the source of truth: a lost wake-up only costs poll latency, never the re-plan.
• Recording never fails the drive: a storage error is logged with the apply's triage attributes, counted (RecordMergeGateRecordFailure), and left for the processor's backstop sweep to re-record. A sustained source="sweep" rate on RecordMergeGateRecorded is the operator signal that drive tails are failing to record.
• Tests: the integration test drives a real apply through the webhook command path to terminal success and asserts the pending merge gate request is durably visible with full target + attribution fields and that the notifier fires; a unit test pins both sides of the consumer gate.

drive tail (apply → Completed)
    ├─ 0. consumer registered?           (nil ⇒ no GitHub runtime ⇒ skip)
    ├─ 1. record merge_gate_request      (durable, idempotent per apply)
    ├─ 2. kick the merge gate processor  (in-memory hint, loss-tolerant)
    └─ 3. complete pending control requests / terminal summary

How it moves us toward the northstar: Every schema mutation on a GitHub-connected server — PR-driven or CLI-driven — leaves a durable, at-least-once breadcrumb the moment it lands, and servers with no PR surface stay entirely out of the flow. The processor (stack 3/7, #866) turns those breadcrumbs into re-planned sibling PR checks.

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.

🤖 Generated with Claude Code

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