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

[morgo]

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

First, a correction that is mine to make: this PR is green and I had it recorded as failing. The rollup shows three FAILURE entries — E2E Tests, LocalScale Tests, K8s E2E Tests — but all three belong to run 31223145894, which was cancelled wholesale (13 of its jobs are CANCELLED, several still carrying unexpanded ${{ matrix.shard }} names). They are aggregate gate jobs reporting FAILURE because their matrix dependencies were cancelled, not because anything failed. The live run 31223147191 is green on all three of those same names. mergeStateStatus is CLEAN, which agrees. My earlier sweep counted non-SUCCESS as failure and held this PR on that basis; that was wrong, and it's why it sat.

On the change itself — small (+144/-0), additive, and it closes a real hole rather than papering one.

It's the missing half of a race the fan-out could previously only log. holdStoredPRChecks writes each hold through MarkBlockedForApplyInFlight, which is head-SHA-conditional, and its !flipped branch already reasons about "a racing synchronize re-planned a newer head against the pre-apply schema." Before this PR that racing plan won and the gate reopened mid-apply; now the plan checks the target first and is born held. The two pieces fit, and the fit is what makes me confident the mechanism is right rather than merely plausible.

Checked the failure directions:

• Storage error fails the write, not open. The comment claims "never by assuming the target is quiet" and the code delivers — it returns the error rather than falling through to a passing check. That's the one place where a fail-open would be serious, and it isn't one.
• Only a would-pass verdict is flipped. Verdicts that already block keep their more specific reason, and they don't need the hold since they block on their own merits.
• ErrorMessage is newly set in the struct literal but was already the zero value on the unheld path, so the non-held case is unchanged rather than newly clearing a field.

Two residual windows, both narrow and both failing safe — worth recording because neither is closed, not because either should block:

1. Hold lands between the query and the write. HasActivePreflightedApplyOnTarget returns false, a preflight then holds the row, and this write overwrites it passing. The window is two adjacent storage calls, so it's small — and it self-corrects, because fanOutCheckSettle re-plans every sibling check for the target, not only held ones. Worth knowing that rearmPreflightRendersForActiveApplies is not a backstop here: it re-arms renders, not stored holds.
2. Born held after the settle fan-out already ran. The check is written held with nothing left to release it until the next push or plan. Fail-closed (a stuck block, not a stuck open), and RecordMergeGatePlanTimeHold's doc anticipates exactly this — "a sustained rate with no matching settle re-plans means holds are piling up on the target." Instrumenting the failure mode you can't fully close is the right call.

One note on landing order: this is stacked on #941 (armand/check-preflight-gate), which I'm holding separately as a core change to the apply-start path. This approval is on #942's own diff and doesn't carry to its base.

Nit: clampDriftSummary is doing duty as a generic length clamp for a hold summary that has nothing to do with drift; the name will read oddly at the next call site someone adds.

Not blocking.