feat(github): store born-held checks while a preflighted apply changes the target

[aparajon]

Why this matters

The preflight fan-out holds every sibling check that exists when an apply starts — but a sibling that pushes a commit (or runs a manual plan) mid-apply gets a fresh plan against the mid-change schema, and a passing verdict from that plan would mint a fresh green check that sidesteps the hold entirely. This closes the last first-party gap in the merge gate: plan-time writes now know when the target is mid-apply. Stack 7/7, on top of #941.

What it does

• upsertPlanCheckRecord — the single choke point every plan-time stored-check write funnels through (auto plans, manual plans, and the settle fan-out's re-plans) — consults HasActivePreflightedApplyOnTarget before storing a verdict that would pass. If a preflighted apply is active on the (environment, database type, database) target, the check is stored born held: action_required with the same apply_in_flight_on_target blocking reason the preflight fan-out writes, so the aggregate check and its release path are identical to any other hold.
• Released like every other hold: the apply's settle fan-out re-plans held sibling checks against the settled schema. Nothing new to sweep or release.
• Verdicts that already block (plan changes, plan errors, review-time drift) keep their more specific reason — the hold only replaces a would-be-green conclusion.
• Fails closed on uncertainty: a storage error while checking for an active apply fails the check write; it never assumes the target is quiet.
• The plan comment is untouched — the hold changes the stored verdict, not the plan UX.
• Metric: schemabot.merge_gate.plan_time_holds_total — a sustained rate with no matching settle re-plans means holds are piling up on a target; check the merge gate processor's logs.

 plan (auto / manual / re-plan) on target T
        │
        ▼
 verdict would pass? ──no──► store the real (blocking) verdict
        │yes
 preflighted apply active on T?
        ├─ storage error ──► fail the write (fail closed)
        ├─ yes ──► store born held (apply_in_flight_on_target)
        │            └─ released by the apply's settle re-plan
        └─ no ───► store success

How it moves us toward the northstar

With this, every path that can produce a passing check during an apply window is closed at the source: existing checks are held by the preflight fan-out, and new checks are born held. The merge gate on a busy target is now airtight against first-party surfaces — what remains (admin overrides, unprotected branches) is outside the code host's checks and is handled by drift detection and, eventually, merge-time revalidation.

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) → this PR (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/942, 4d42d5f.

Verdict: 8 findings — none blocking; 4 non-blocking (a narrow residual TOCTOU window plus observability and test-convention gaps), 4 suggestions.

Non-blocking

1. Residual TOCTOU: the HasActivePreflightedApplyOnTarget read and the UpsertPlanResult write are unserialized against the preflight → fan-out → gate-confirm sequence. The read at check_records.go#L222 and the write at check_records.go#L265 are separate autocommit statements with no shared transaction or lock. Failure scenario: sibling PR B's plan reads activeHold=false before apply A's preflight row lands; the read-to-write gap stretches to seconds under row-lock contention because withLockRetry re-runs only the exec closure, never the read. Meanwhile A's preflight records, the fan-out sweeps (merge_gate.go#L490) missing B's uncommitted row (or holding B's existing row), and the feat(api): gate apply start on confirmed sibling PR check holds #941 gate confirms — it only polls request state (operator.go#L1228 if req.State == storage.MergeGateCompleted), never re-scanning sibling checks. B's delayed write then lands conclusion=success, clobbering a just-placed hold: hold rows are completed/apply_id-NULL (merge_gate.go#L681) so the guard AND NOT (status = ? AND apply_id IS NOT NULL) at checks.go#L163 does not protect it, and the CASE arms preserve only the review-drift reason. Result: a green, mergeable check mid-DDL. The window is narrow and the PR still closes the previously wide-open gap — follow-up is the right disposition: make the success-path upsert conditional on no-active-preflight in the same SQL statement, or re-verify after the write.

2. The born-held metric and log fire before the store, so they miscount and mispromise. RecordMergeGatePlanTimeHold at check_records.go#L247 and the Info log at check_records.go#L240 precede the guarded UpsertPlanResult at :265, so the metric — whose contract is "counts plan-time check writes stored born held" (metrics.go#L1622) — counts holds never stored when the upsert errors, or when the apply-owned in_progress guards (checks.go#L136, #L163) silently no-op the UPDATE. Concrete path: the originating PR — which HasActivePreflightedApplyOnTarget does not exclude — pushes a clean-diffing commit mid-apply: the flip fires, the metric counts, the log promises "the apply's settle fan-out will re-plan this check when it settles" — but the fan-out skips the originating change via isOriginatingChange at merge_gate.go#L497, so the log names a releaser that will never run, misleading triage during exactly the mid-apply window the metric exists for.

3. New test uses an inline time.After(10 * time.Second) instead of the package's named polling deadline. merge_gate_integration_test.go#L963 reads case <-time.After(10 * time.Second): while webhookIntegrationPollDeadline = 30 * time.Second exists at webhook_integration_test.go#L103 and this same file uses it everywhere else (lines 151, 169, 188, 200, 561, 582). AGENTS.md:198 mandates the named constant, and AGENTS.md:28 forbids fixing the resulting flake by bumping the inline timeout. On a contended CI runner the plan flow (real MySQL plan + fake-GitHub round trips) exceeds 10s and this test flakes with "timed out waiting for plan comment" while every sibling wait survives at 30s.

4. The documented fail-closed branch has no test and no untestability call-out. The storage-error path at check_records.go#L223–:234 is documented in the PR body ("Fails closed on uncertainty"), and AGENTS.md:113 requires focused tests for documented edge cases. The sole new test covers only the happy born-held path; the layer is demonstrably testable via the package's existing failing-store pattern (check_runs_test.go#L79). A later refactor that swallows the error into log-and-continue would mint a green check exactly when the merge-gate DB is unhealthy, and no test would fail.

General suggestions

• The flip silently disables the manual-plan no-op rescue (fix(github): recover stuck checks from no-op plans #217) while a wedged apply is non-terminal. With a preflight recorded and the apply non-terminal, a rescue schemabot plan sees activeHold=true and the flip at check_records.go#L236 makes successfulNoOpPlanResult (checks.go#L210, requires Conclusion == checkConclusionSuccess) return false, so RecoverApplyOwnedCheckWithNoOpPlan bails at checks.go#L174 and the rescue is a complete no-op until the apply is made terminal — undocumented in the PR body, untested, and nothing logs "recovery skipped because born held". Defensible fail-closed hardening, but worth a doc line and a log.
• No test proves the settle fan-out actually releases a born-held-shaped row. The new test ends at storage assertions (merge_gate_integration_test.go#L975) without driving a settle, so the PR-body release claim ("the apply's settle fan-out re-plans held sibling checks... Nothing new to sweep or release") is untested for the born-held shape specifically (HasChanges=false, apply-id-less summary — check_records.go#L239 vs the fan-out's attributed format at merge_gate.go#L685). Today's settle path reads none of those fields, so release works — this is a contract-pinning test to protect against future enumeration/skip-condition changes, not a bug.
• Born-held checks reuse the fan-out hold's stored shape but not its operator-facing surfaces. No hold comment is posted (the fan-out treats the comment as part of the preflight contract, merge_gate.go#L739), and the summary/log cannot name the holding apply or requester because HasActivePreflightedApplyOnTarget returns only a bool (storage.go#L399). On a busy target, identical holds triage differently — the fan-out row names its apply, the born-held one does not. The PR body declares the comment omission deliberate; consider returning the holding request (not a bool) so the summary can attribute.
• The new test hand-rolls handler construction instead of reusing newE2EHandlerWithoutMergeGateProcessor. merge_gate_integration_test.go#L945–:948 re-implement the shared helper (webhook_integration_test.go#L294), adding log/slog, os, and ghclient imports and silently building a slug-less NewInstallationClient where the helper uses NewInstallationClientWithSlug(client, logger, "schemabot") — future helper-routed wiring changes (trusted check app slugs, logger policy) would bypass this one test.

The one thing that could have broken, verified

The born-held conversion of a would-pass verdict at the plan-write choke point (check_records.go#L221–:249). I verified its supporting premises directly in the worktree: (1) upsertPlanCheckRecord is genuinely the only plan-time passing-verdict writer — auto plan, manual plan, apply-time plan (apply_check_records.go:19), and the settle re-plan (merge_gate.go#L873) all route through it, and the remaining success writers in mysqlstore/checks.go are safe (CompleteForApply serves only the apply's own terminal outcome; MarkStalePlanSuccessful fires only when the database left the PR; RecoverApplyOwnedCheckWithNoOpPlan is defeated by the flip itself, since :236 makes successfulNoOpPlanResult false). (2) The stored hold triple at :236-:238 is byte-identical to the fan-out's (merge_gate.go#L681–:685), so aggregation and settle release are shared for free. (3) The settle re-plan cannot re-hold its own release: the settled apply is terminal, and the non-terminal-apply predicate in merge_gate_requests.go#L375's query excludes it, so the fresh in-upsert read returns false and real verdicts store. What I could NOT prove safe is premise (4): atomicity of the :222 read against the :265 write — they are two separate storage operations, the gate never re-scans sibling checks after confirming, and withLockRetry widens the gap by retrying only the exec. That residual interleaving is Non-blocking finding 1; a deterministic proof would hold an InnoDB row lock on the sibling's checks row while driving a preflight to completion, then release it and observe the green clobber.

Verified correct

• CI effectively green: the latest run passes all 32 checks; the 3 failing rows are stale entries from a superseded duplicate run.
• Guard placement: the flip sits after drift/refresh-note handling and gates on conclusion == checkConclusionSuccess (:221); planCheckConclusion returns only failure/action_required/success, so drift blocks, plan errors, and has-changes verdicts keep their more specific reasons, exactly as the PR body claims.
• Argument order of HasActivePreflightedApplyOnTarget(ctx, environment, schema.Type, schema.Database) at :222 matches the interface declaration (storage.go#L399) and the existing call in merge_gate.go; the SQL filters kind=preflight joined to a non-terminal apply on exactly that target.
• ErrorMessage is a real column on every UpsertPlanResult write path (INSERT, drift-preserving UPDATE, plain UPDATE), and setting it to "" for non-held writes matches the pre-PR zero-value behavior — no existing write path changed shape.
• The born-held verdict reaches the visible check run: both plan flows and the merge-gate re-plan recompute the aggregate after the store with the returned headSHA, and the aggregate fold fails allChecksAreUpToDate on action_required.
• Born-held writes cannot clobber an in-flight apply-owned row: both UPDATE branches of UpsertPlanResult carry AND NOT (status = ? AND apply_id IS NOT NULL) (checks.go#L136, #L163), preserving "started applies remain authoritative", and the CASE arms preserve an existing review-drift block.
• Release lifecycle closes: the settle fan-out defers to a still-active later apply, otherwise re-plans every GetByTarget row through the same choke point, which re-evaluates the hold — a re-plan while yet another apply is active goes born-held again instead of leaking a pass, and the missing-settle sweep backstops applies that die without settling. This PR incidentally closes the settle fan-out's own mid-fan-out race (a new preflighted apply starting between the settle's check and a per-PR re-plan write now yields a born-held write instead of a green one).
• Fail-closed on storage uncertainty: the error path (:223-:234) returns before any write with the established plan_check_recorded/error metric shape and a fully-identified wrapped error; no caller converts the failure into passing state, and the settle re-plan path durably fails closed via blockCheckForFailedRefresh.
• Metric follows conventions: schemabot.merge_gate.plan_time_holds_total (metrics.go#L1628) uses addCounter with the same database + environment label shape as sibling merge-gate metrics, a {hold} unit, and a doc comment stating the operator action.
• The new integration test exercises the real webhook plan path end-to-end: it seeds a Running apply plus a preflight on the exact (staging, mysql, dbName) target via existing helpers, uses ChangeKey "2" vs webhook PR 1 so isOriginatingChange does not exempt the row, pre-creates the target table so the verdict would pass, and asserts specific stored values. The post-comment storage read is race-free (the row is stored before the plan comment posts).
• The born-held summary goes through clampDriftSummary (:239) per the AGENTS.md summary-sanitization rule, and the Info log carries the full triage identifier set (repo, pr, head_sha, environment, database_type, database).
• Efficiency: the extra storage round trip is a single indexed EXISTS query executed only when the verdict would pass — no per-row loop, no N+1.
• The delta is purely additive (144 insertions, 0 deletions across metrics.go, check_records.go, and the test file): no behavior, coverage, or invariant removed; no merge-gate rename drift; all consumed symbols exist at the base ref.
• Repo conventions: feat(github) scope is correct for GitHub PR UX changes in pkg/webhook/; the new test has the required scenario comment, uses t.Context() and testify, and asserts specific values; new verbiage avoids banned terms and internal details.

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