Skip to content

feat(github): re-plan sibling PR checks when an apply changes a target schema - #866

Open
aparajon wants to merge 3 commits into
armand/check-refresh-drive-tailfrom
armand/check-refresh-on-apply
Open

feat(github): re-plan sibling PR checks when an apply changes a target schema#866
aparajon wants to merge 3 commits into
armand/check-refresh-drive-tailfrom
armand/check-refresh-on-apply

Conversation

@aparajon

@aparajon aparajon commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Closes the merge gate guardrail. When an apply changes a target's schema, every other open PR holding stored plan check state against that (environment, database type, database) is holding a verdict computed against a schema that no longer exists. This PR drains the durable requests #868 records and re-plans those PRs — or blocks them. Builds on #868 (drive-tail recording) and #867 (storage).

The case this is acute for is a CLI-driven apply: nothing on GitHub even hints the target moved, so a sibling PR's green check simply stays green.

The drain loop. Each pass sweeps for completed applies with no request row (the outbox backstop), terminates requests wedged past the attempt cap, then claims pending requests under a rotating lease and fans out. The drive tail's notifier wakes the loop so a request drains immediately after its apply completes rather than waiting out the poll interval; the wake-up is an in-memory hint over the durable row, so losing it costs latency and nothing else. Same-target pending requests coalesce into one fan-out — a re-plan against the live schema already covers every schema change recorded before it started.

  sweep backfill ─┐
  poll tick ──────┤          ┌──────────────────────────┐
  drive-tail wake ┴─────────▶│ claim request (lease)    │
                             └────────────┬─────────────┘
                                          │ coalesce same-target siblings
                                          ▼
                             ┌──────────────────────────┐
                             │ fan out over checks on   │
                             │ (env, type, database)    │
                             └────────────┬─────────────┘
                                          │
        ┌─────────────────────────────────┼─────────────────────────────┐
        ▼                                 ▼                             ▼
  re-plan at the                    re-plan fails,                skip, logged
  PR's current head:                so the check is               and counted:
  stored check gains                flipped blocking              originator,
  an attribution note               (fail closed)                 in-flight apply,
                                                                  closed PR,
                                                                  superseded head

Fail closed. A failed re-plan durably flips the stored check to a blocking schema_changed_replan_failed conclusion with a fixed message — the raw re-plan error stays in server logs and is never rendered on the PR. The request still completes: the block is a durable outcome, not something to retry.

What a reviewer sees. A PR that nobody touched can acquire a new check result, so every re-planned check carries an attribution note saying which apply moved the schema under it.

Sibling PR re-planned after another PR's apply landed
Database Type Change Status
orders mysql 1 alter · re-planned: schema for orders in staging changed (apply apply_a1b2c3 by cli:operator@host) Pending
Sibling PR where the re-plan itself failed (fail closed)
Database Type Change Status
orders mysql schema for orders in staging changed (apply apply_a1b2c3); re-plan failed — see server logs Pending

The check's own detail carries the operator instruction:

The live schema for this database changed after this plan was computed, and SchemaBot could not re-plan the PR against it. Re-run schemabot plan (or push a new commit) before this check can pass; see server logs for the re-plan failure.

Invariants

  • Establishes MG-12 — a landed schema change invalidates the checks planned against it. The rule only becomes true here: feat(storage): durable merge gate requests for schema-mutating applies #867 and feat(api): record durable merge gate requests at apply drive tails #868 build the substrate, but nothing re-plans or blocks until this processor runs. The registry entry is owed before merge and lands with the port onto main, since docs/invariants.md postdates this branch; its Enforced: line names the request outbox, the drive-tail recording, and this fan-out.
  • Upholds MG-5. The fan-out never touches a row an apply owns; the fail-closed flip refuses apply-owned rows outright rather than releasing them.
  • Upholds MG-1. Every uncertain outcome resolves toward blocking: a re-plan that cannot run blocks, and a racing write that already superseded the head is skipped rather than overwritten.
  • Upholds MG-4. The re-plan targets the PR's current head, and a row a newer head already superseded is left alone.

Observability: per-PR and per-request outcome counters, a stuck-termination counter, and triage-complete logs on every skip and failure path.

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)

Copilot AI review requested due to automatic review settings July 28, 2026 21:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a durable “check refresh” guardrail so that when an apply successfully changes a target’s live schema, SchemaBot re-plans (or fail-closes) other open PRs whose stored plan checks were computed against the old schema—covering PR, CLI/gRPC, and rollback-driven applies.

Changes:

  • Introduces check_refresh_requests outbox + processor that leases, heartbeats, coalesces same-target requests, and fans out re-plans to sibling PRs (fail-closed on re-plan failure with a sanitized message).
  • Records refresh requests from the operator drive tail on terminal success, plus a backstop sweep for crash gaps.
  • Extends stored plan-check upsert to support refresh attribution notes, adds target-wide check lookup/indexing, and adds metrics + integration tests.

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
pkg/webhook/vschema_only_check_integration_test.go Updates test calls for new upsertPlanCheckRecord signature.
pkg/webhook/handler.go Adds check refresh processor lifecycle fields and defaults.
pkg/webhook/check_runs.go Adds a fixed fail-closed blocking reason/message for refresh re-plan failures.
pkg/webhook/check_refresh.go Implements the check refresh processor (sweep, lease/heartbeat, fan-out, fail-closed path).
pkg/webhook/check_refresh_integration_test.go End-to-end integration tests for drive-tail recording, sweep backfill, fan-out, fail-closed behavior, in-flight guard, and coalescing.
pkg/webhook/check_records.go Adds refresh attribution support and introduces errPlanCheckHeadStale for racing-head detection.
pkg/storage/types.go Adds CheckRefreshRequest type + constants and max-attempts budget.
pkg/storage/storage.go Extends storage interfaces with CheckRefreshRequestStore, CheckStore.GetByTarget, and MarkBlockedForFailedRefresh.
pkg/storage/mysqlstore/storage.go Wires MySQL storage to expose CheckRefreshRequests().
pkg/storage/mysqlstore/checks.go Implements target-wide check lookup and fail-closed update with head-SHA + in-flight guards.
pkg/storage/mysqlstore/check_refresh_requests.go Implements durable request recording/claiming/heartbeat/completion/failure/sweep queries.
pkg/storage/mysqlstore/check_refresh_requests_test.go Integration tests for request leasing semantics, retry/terminal behavior, sweep selection, coalescing, and check-store helpers.
pkg/storage/errors.go Adds durable check refresh request lease/not-found errors.
pkg/serve/serve.go Starts/stops the check refresh processor alongside other server background work.
pkg/schema/mysql/checks.sql Adds an index to support target-wide check lookups.
pkg/schema/mysql/check_refresh_requests.sql Adds the new durable check_refresh_requests table.
pkg/metrics/metrics.go Adds metrics for recording sources, record failures, PR fan-out outcomes, and request-level outcomes/terminations.
pkg/api/operator.go Records refresh requests from operator terminal-success transitions (before control-request cleanup).
pkg/api/handlers_test.go Updates mock storage to satisfy the extended storage interface.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/webhook/merge_gate.go
@aparajon
aparajon marked this pull request as ready for review July 28, 2026 22:28
@aparajon
aparajon force-pushed the armand/check-refresh-on-apply branch from 2ba0f41 to dd294aa Compare July 28, 2026 22:40
@aparajon aparajon changed the title feat(github): re-plan sibling PR checks when an apply changes a target's live schema feat(github): re-plan sibling PR checks when an apply changes a target schema Jul 28, 2026
@aparajon
aparajon changed the base branch from main to armand/check-refresh-drive-tail July 28, 2026 22:40
@aparajon
aparajon marked this pull request as draft July 29, 2026 01:51
@aparajon
aparajon force-pushed the armand/check-refresh-on-apply branch 2 times, most recently from 705ec9a to b054f62 Compare August 5, 2026 00:36
@aparajon
aparajon force-pushed the armand/check-refresh-on-apply branch from b054f62 to fd47dab Compare August 5, 2026 14:03
@aparajon
aparajon force-pushed the armand/check-refresh-drive-tail branch from 02dbe05 to 6ba60ea Compare August 5, 2026 14:03
aparajon and others added 3 commits August 5, 2026 11:13
…t schema

An apply reaching terminal success mutates its target's live schema, staling
the stored plan check state of every other open PR against that
(environment, database type, database) target — acute for CLI applies with no
PR surface, where a stale green check is a tier-0 safety gate failure. The
webhook processor now drains the durable check refresh requests the drive
tails record: each pass sweeps for completed applies missing a request,
terminates requests wedged past the attempt cap, then claims pending requests
under a lease and fans out. The fan-out re-plans each sibling PR's stored
check at its current head with an attribution note naming the apply and
caller, skipping the originating PR, in-flight apply rows (a started apply
stays authoritative), closed PRs, targets the PR no longer manages, and rows
a newer head already superseded. A failed re-plan flips the stored check to a
blocking schema_changed_replan_failed conclusion — fail closed — and the
request still completes because the block is durable. Same-target pending
requests coalesce into one fan-out.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… a request

Without a wake-up, a request recorded right after a poll tick waits the full
interval before sibling PR checks move. The handler now registers
KickCheckRefresh as the service's recorded-notifier: the drive tail's call
lands on a buffered channel the driver selects on alongside its ticker, so a
co-located processor drains the request immediately. The durable request row
stays the source of truth — a kick lost to a pod boundary or a stopped
driver only costs poll latency, never the refresh.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The processor consumes merge gate requests by their code-host neutral
change identity: the sweep records change_key via ChangeKeyForPullRequest
and the fan-out's originator skip is a named predicate that converts the
stored check's PR number at the comparison boundary. Empty change keys
(CLI/gRPC applies) exclude nothing.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@aparajon
aparajon force-pushed the armand/check-refresh-on-apply branch from fd47dab to 6bc0242 Compare August 5, 2026 15:14
@aparajon
aparajon force-pushed the armand/check-refresh-drive-tail branch from 6ba60ea to 56faf88 Compare August 5, 2026 15:14
@aparajon
aparajon marked this pull request as ready for review August 7, 2026 02:38
@Kiran01bm

Copy link
Copy Markdown
Collaborator

🤖 Review findings - created by Kiran's code review agent - for pull/866, 6bc0242.

Verdict: 9 findings — 3 blocking (fan-out panics escape the recover, coalescing drops the originator re-plan, dead stale-head guard), 4 non-blocking, 2 suggestions.

Blocking

  1. safeFanOutMergeGate's recover only covers the calling goroutine; the per-PR wg.Go fan-out goroutines have no recover, so a panic in refreshPRPlanForTarget crashes the process — the advertised crash-loop protection is inert for exactly the work it exists to contain. The only recover() in the file is deferred on the parent goroutine (merge_gate.go#L392-L401) before return h.fanOutMergeGate(ctx, req); the per-PR re-plans run on child goroutines via wg.Go(func() { … h.refreshPRPlanForTarget(ctx, req, check) … }) (merge_gate.go#L448-L456) with no recover of their own — Go recover only intercepts panics on the goroutine where the deferred call runs. Failure scenario: one poison sibling PR whose re-plan deterministically panics crashes the entire server, taking down all webhook processing; the request row stays processing with an expired lease, another replica reclaims it and panics too — a fleet-wide crash-loop bounded only by the 5-attempt cap, the exact scenario the comment at merge_gate.go#L386-L390 ("a driver panic would otherwise crash-loop every replica on the same poison request") claims to prevent. Contrast durable_dispatch.go#L308, whose safeProcessDurableWebhookEvent processes on the same goroutine so its recover works; the existing goroutine-local helpers handler.go#L958 (recoverPanic) / handler.go#L979 (goSafe) show the shape of the fix.

  2. Coalescing completes sibling requests from a different originating PR even though the driven fan-out skipped its own originator — permanently dropping the re-plan of the driven request's originating PR against the sibling's schema change, i.e. the stale-green check this guardrail exists to close. Siblings are captured by target only (merge_gate.go#L278; the query at merge_gate_requests.go#L181 never compares Repository/ChangeKey), the driven fan-out skips exactly its own originator (merge_gate.go#L420 if isOriginatingChange(check, req) {), and every captured sibling is then completed unconditionally (merge_gate.go#L361). Failure scenario: PR#2's apply A completes (kick lost to a pod restart, a documented loss mode), PR#3's apply B completes on the same target before the next 30s tick; the drain claims A (ClaimNext orders ORDER BY created_at, id, merge_gate_requests.go#L123, so the unsafe order is the guaranteed order), skips PR#2 as originator, and coalesces B — whose own fan-out would NOT have skipped PR#2. PR#2's stored check, computed before B mutated the schema, keeps passing forever: requests are one-per-apply and B is completed, so nothing recovers it. The only coalescing test uses CLI-style requests with empty ChangeKey (matches nothing), so this case is untested. Fix direction: only coalesce siblings with the same (Repository, ChangeKey) as the driven request, or include the driven request's originator in the fan-out when a pending sibling has a different originator.

  3. The merge-gate re-plan's skipped_superseded head-stale guard is structurally dead (cache-hit tautology), so a stale head-A re-plan can overwrite a racing synchronize's head-B check row via the head-unconditional UpsertPlanResult, wedging the PR's aggregate check. commandBootstrap attaches a request-scoped PR-info cache (bootstrap.go#L47); discovery seeds it (schema.go#L68 prInfo, err := ic.FetchPullRequest(...), schema.go#L100 HeadSHA: prInfo.HeadSHA); the guard's re-fetch at check_records.go#L165 is a cache hit, so if prInfo.HeadSHA != headSHA (check_records.go#L177) compares the discovery snapshot against itself and the errors.Is(err, errPlanCheckHeadStale) branch at merge_gate.go#L566 is unreachable from this path. UpsertPlanResult's WHERE has no head condition (checks.go#L163 guards only apply-owned in-progress rows). Failure scenario: developer pushes head B while the merge gate is mid-re-plan of head A; the synchronize auto-plan stores the head-B row, then the merge gate's upsert overwrites it with head-A data; the aggregate folds the stale row as a blocking placeholder ("Awaiting results for the latest commit", check_aggregate.go#L204) and the PR wedges until a manual re-plan or push. Fails closed, but the advertised TOCTOU protection does not exist from this path, and a stale driver overwrites newer state — against the repo's conditional-update review bar. The dead comparison pre-existed in base, but the merge-gate path, the sentinel, and the skipped_superseded reliance on it are this PR's delta. The repo already documents the exact fix: plan.go#L75-L77 "Use FetchPullRequestNoCache: the cached FetchPullRequest used by discovery would return the discovery-time HeadSHA, masking the race." — do a NoCache re-check before the upsert, or add a head-conditional UpsertPlanResult variant.

Non-blocking

  1. driveClaimedMergeGate treats driver-context cancellation (pod shutdown) as fan-out failure, burning retry budget on every deploy that catches a claimed request. merge_gate.go#L298 if fanErr != nil { has no ctx.Err() check, and the WithoutCancel finishCtx (merge_gate.go#L295) guarantees the MarkFailed write lands despite cancellation — consuming the attempt ClaimNext already charged. The durable-webhook sibling in the same package handles exactly this via a Release refund (durable_dispatch.go#L216-L227), but MergeGateRequestStore has no Release method (only WebhookEventStore does, storage.go#L288). Under deploy churn, five interruptions terminally fail a request that never got one real attempt; the sweep cannot backfill it because FindCompletedAppliesMissingRequest requires r.id IS NULL. Also: the lease-lost branches of MarkFailed/MarkCompleted (merge_gate.go#L310, #L344) emit no events_total outcome, unlike every other terminal path. Non-blocking because the fix needs a new storage-layer Release method (stack follow-up), mitigated by the 5-attempt budget and 1-minute retry delay.

  2. Run-context cancellation is inert for per-PR fan-out work, so the heartbeat contract comment is false past bootstrap and shutdown can block for many minutes. refreshPRPlanForTarget switches to prCtx from commandBootstrap (merge_gate.go#L491), rooted in context.Background() with a 2-minute timeout (bootstrap.go#L46); neither the wg.Go bodies nor refreshPRPlanForTarget re-check runCtx (the only runCtx-using step, resolveRepoWebhookInstallation, short-circuits on installation-cache hit), so the comment "On lease loss it cancels the run context so in-flight work stops" (merge_gate.go#L637) does not hold. Consequences: after lease loss the reclaiming driver re-plans the same PRs while the old driver's detached re-plans keep running (doubled GitHub/plan load; a late transient failure can flip a freshly-green check blocked — fail-closed but confusing); on shutdown StopMergeGateProcessormergeGateWg.Wait (merge_gate.go#L116) blocks up to ~2min × ceil(N/3) at mergeGatePRConcurrency = 3 (merge_gate.go#L47) — a 30-PR target ≈ 20 minutes, past pod termination grace, so the pod is SIGKILLed mid-write. Fix: derive the per-PR context from runCtx (preserving the timeout), or at minimum correct the comment.

  3. No test covers refreshPRPlanForTarget's documented skip paths or the driver's failed-fanout retry branch. Zero test hits for errPlanCheckHeadStale/superseded, closed-PR skip (merge_gate.go#L505), or not-managed skip (merge_gate.go#L516); the seven integration tests cover record/sweep/replan/fail-closed/in-flight/coalesce/kick only, and the fail-closed test asserts completion, never the MarkFailed-with-retryAfter branch (merge_gate.go#L298-L309). The superseded path is the fragile one: it depends on the stale-head error keeping its %w wrap of the sentinel (check_records.go#L187, a plain non-wrapping error until this PR) — a refactor dropping the %w silently converts every benign head race into blockCheckForFailedRefresh, durably flipping healthy sibling PRs to action_required, and no test fails. The PR body documents these behaviors, and AGENTS.md ("Tests must prove documented behavior") requires focused tests or an explicit why-not callout. A test that stubs a slow plan and lands a newer-head write mid-re-plan would also expose Blocking Fix MySQL health checks to use TCP and add failure log capture #3.

  4. The docs Blocking Reasons table is not updated with the new stable schema_changed_replan_failed blocking reason. This PR adds the value at check_runs.go#L118 but touches no docs; the table at docs/check-runs.md#L541 — the reference the doc says exists "for logs, metrics, and operator triage" — omits it, so an operator triaging the new fail-closed flip finds nothing. The table already omitted two pre-existing reasons (managed_dir_missing_config, review_time_deployment_drift); consider adding all three.

General suggestions

  • Sustained GitHub unavailability during sibling schema discovery fails the check closed at merge_gate.go#L526 and completes the request with 4 of 5 claim attempts unused, so the PR author must manually re-plan. This is deliberate, precedented fail-closed design (client-level retryGitHubUnavailableRead already absorbs single 503s, and the code's own contract sanctions fail-closed for discovery uncertainty), but consider routing ghclient.IsUnavailableError discovery errors to the retryable path (return err) so a one-minute retry clears them instead of a durable action_required flip.
  • Lease-owner strings keep the pre-rename feature name: merge_gate.go#L751 return fmt.Sprintf("%s/%d/check-refresh", hostname, os.Getpid()) and merge_gate_integration_test.go#L34, while the table, metrics, and logs all say merge gate. Cosmetic, but the stale token lands in the persisted lease_owner column operators grep during triage.

The one thing that could have broken, verified

The fan-out's concurrent stored-check writes racing other writers on the same row — in-flight applies, synchronize auto-plans, other merge-gate drivers. The in-flight-apply race is proven safe at the storage layer, not the snapshot layer: both UpsertPlanResult UPDATE branches carry AND NOT (status = ? AND apply_id IS NOT NULL) (checks.go#L136, #L163) and MarkBlockedForFailedRefresh adds AND head_sha = ? (checks.go#L546-L547), so a stale GetByTarget snapshot cannot stomp a started apply and the fail-closed flip always loses to a newer head or a claiming apply. But the success-path write is NOT protected as advertised: tracing schemaResult.HeadSHA (cached FetchPullRequest at schema.go#L68) into upsertPlanCheckRecord's guard (check_records.go#L165, same request-scoped cache from bootstrap.go#L47; the NoCache fetch at merge_gate.go:500 does not seed it) shows the head re-check compares the discovery snapshot against itself — the superseded branch is unreachable and the head-unconditional UpsertPlanResult can overwrite a mid-re-plan synchronize's newer-head row (Blocking #3). The remaining safety is the aggregate's fail-closed stale-head placeholder, which wedges the PR instead of passing it stale. Definitive proof either way: a test that stubs a slow plan, lands a synchronize write for a new head during it, and asserts the stored row still carries the new head — today it would fail.

Verified correct

  • CI effectively green: latest run passes all 32 checks including all matrix shards; the 3 stale "fail" rows (unexpanded matrix names, 0s duration) come from a superseded duplicate run.
  • Attempt-cap alignment: maxMergeGateAttempts aliases storage.MaxMergeGateAttempts (merge_gate.go#L52); ClaimNext increments attempts transactionally and the claimable predicate requires attempts < cap, so the driver's terminal decision goes terminal on exactly the attempt after which the store stops handing the row out.
  • Lease lifecycle: MarkCompleted/MarkFailed/Heartbeat are lease-token-conditional and idempotent; every finish path treats ErrMergeGateLeaseLost/NotFound as yield-to-other-driver; finishCtx = WithTimeout(WithoutCancel(ctx), 5s) so terminal bookkeeping survives driver cancellation.
  • Heartbeat correctness: transient store errors keep the lease until expiry and retry next tick; intentional stop is distinguished via hbCtx.Err(); heartbeatErr is written strictly before the goroutine's deferred close(done) and read only after <-done — proper happens-before, no data race.
  • Fan-out success + heartbeat failure is fail-safe: the driver skips MarkCompleted and leaves the row processing for lease-expiry reclaim rather than completing under uncertain ownership; re-planning the same PRs is idempotent-safe.
  • Sweep parity and idempotency: sweepMergeGateRequests builds requests from the same apply fields as the drive tail, both scoped to state=completed only; Record is idempotent per apply via the unique apply_id key; the sweep query is index-covered and bounded by the 6h lookback.
  • Coalescing ordering is sound for same-originator coverage: siblings are captured before the fan-out starts, a pending request only exists after its apply's terminal write, and CompletePendingCoalesced is pending-conditional so a concurrently-claimed sibling is finished by its own lifecycle. (The differing-originator gap is Blocking Initial port #2.)
  • The drain loop cannot tight-spin: it stops on ctx error, empty claim, or claim error; MarkFailed sets retry_after one minute out; a row wedged by a failed MarkFailed stays leased until expiry.
  • Kick wiring: mergeGateKick is a buffered-1 channel allocated in NewHandlerWithDispatch so KickMergeGate is safe before Start and a mid-pass kick is retained; OnMergeGateRecorded registration matches hasMergeGateConsumer's gate so GitHub-less servers record nothing and no pending rows leak.
  • Lifecycle wiring: both single- and multi-app webhook runtimes wire start/stop, Start/Close nil-guard the hooks, the processor stops before storage closes, and StartMergeGateProcessor is idempotent and registers on the WaitGroup under the mutex so Start cannot race Stop's Wait.
  • The fail-closed flip is head-SHA-conditional and sanitized: MarkBlockedForFailedRefresh refusals map to skipped_superseded; the stored block carries only the fixed message with the raw cause in server logs; caller-influenced text is routed through clampDriftSummary per the no-untrusted-markdown rule.
  • isMergeGateNotManagedError matches only determinate typed not-managed answers (merge_gate.go#L697) — none can wrap a transient GitHub failure; the PR-state fetch before discovery uses FetchPullRequestNoCache and its failure stays retryable, not flipped.
  • isOriginatingChange handles CLI/gRPC applies correctly: empty ChangeKey matches nothing, so CLI-originated requests exclude no PR and every sibling is re-planned.
  • Aggregate and participant sentinel rows can never enter the fan-out: GetByTarget filters on a real (environment, database_type, database_name) while aggregate rows carry the sentinel in both type and name; requests reject empty type/name at Record.
  • Reuse/altitude on the re-plan path is right: the fan-out reuses commandBootstrap, createManagedSchemaRequestFromPR, attachServerEnvironments, executePlanProtoWithTransientRetry, reviewTimeDrift, upsertPlanCheckRecord, and updateAggregateCheck rather than forking the plan pipeline; the only signature change (refreshNote) is threaded through all four call sites and is a no-op for pre-existing callers passing "".
  • Deleted-line audit clean: the only deleted/rewritten non-test lines are serve.go doc comments (re-established), the upsertPlanCheckRecord signature + stale-head error (%w sentinel added, behavior-preserving for base callers), and a test header comment; the base drive-tail test is retained unmodified, so no coverage was lost.
  • Metrics helpers follow repo conventions (addCounter/addCounterN, EnvironmentAttribute, actionable outcome comments); go 1.26 so sync.WaitGroup.Go is available.
  • Conventions honored: driver/drive vocabulary throughout, no "migration"/"worker" in new code, sweep logs use apply.LogAttrs(), no internal numeric row IDs logged, commit scopes match AGENTS.md.
  • The six new integration tests assert on specific values (exact blocking reason, message, attribution substring, attempts counts, untouched originator fields), use t.Context(), require/assert, and EventuallyWithT with the shared poll deadline — no raw sleeps.
  • safeFanOutMergeGate's recover does convert same-goroutine fan-out panics (GetByTarget, target filtering, goroutine launch) into retryable failures — the containment gap is specifically the wg.Go child goroutines (Blocking Dependency Dashboard #1).

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

@morgo morgo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Correction I owe this PR first: it is green, and an earlier automated sweep of mine had it recorded as failing. The three FAILURE entries are aggregate gate jobs from a run that was cancelled wholesale — their matrix dependencies were cancelled, not failed — and the live run is green on those same names, with mergeStateStatus: CLEAN. My sweep counted non-SUCCESS as failure, which is why this sat from 2026-08-08 without a verdict from me. That was my error, not a CI problem.

+1300/-14 reads worse than it is: 360 lines are tests, and the bulk is merge_gate.go arriving as a new 700-line file. I'd already audited that file's machinery at the stack tip, so rather than re-reading it I diffed this revision against the audited one: the function set here is a strict subset, and the shared bodies are textually identical apart from doc comments, one import, a metric constant, and the later sweeps. So the audit transfers, and it covers the parts that matter — exactly two writes of MergeGateCompleted (lease-guarded MarkCompleted after a successful fan-out, and pending-guarded CompletePendingCoalesced on same-kind siblings captured before the fan-out began), TerminateStuckProcessing terminalizing as Failed rather than completed, a heartbeat failure leaving the row for reclaim instead of completing work it can't prove it owns, panic recovery routed through the attempt cap so a poison request can't crash-loop every replica, and a failed coalesce degrading to a redundant-but-safe re-plan.

The lifecycle wiring is the part I'd expect to go wrong, and it doesn't. startMergeGateProcessor/stopMergeGateProcessor are nil-guarded, registered on both the single-app and multi-app runtimes (a missed path there would silently disable the gate for one deployment shape), and stopped in Close. The processor also starts before StartOperator. That ordering isn't load-bearing in this PR, but it becomes so once #941 keys its apply-start gate on hasMergeGateConsumer() — an operator started first could drive an apply that sees no consumer and skips the gate entirely. Worth a comment pinning the order deliberately, since right now it reads as incidental.

I verified the sanitization claim rather than taking it. mergeGateNote's comment says RequestedBy is caller-influenced so the note is "sanitized for markdown-table rendering," and the only call is clampDriftSummary — a name that sounds like pure truncation. It isn't: it collapses \n and \r, rewrites | to /, and truncates on a rune boundary. So a crafted username can't break the aggregate's Change column. The finding is the name. The function's contract is "sanitize and clamp" while it advertises "clamp," and it's now called from three places that depend on the sanitizing half. Someone simplifying it to a truncation, or writing a new summary path that reaches for a plain clamp instead, reintroduces the injection with nothing failing. Renaming it to say what it guarantees is cheap.

One thing I expected to be a behavior change and isn't, worth recording so the next reviewer doesn't re-flag it: the plan-head staleness check in upsertPlanCheckRecord already existed. This PR only wraps the existing error with %w and the new errPlanCheckHeadStale sentinel so the fan-out can treat a racing synchronize as a benign skip. Ordinary plan writes behave exactly as before. That %w is load-bearing though — switching it to %v during a later cleanup would turn a benign race into a hard fan-out failure that retries the request for nothing.

Nit: when drift blocks, changeSummary = drift.summary overwrites the appended refresh attribution, so a drift-blocked PR loses the "why was I re-planned" line. The comment says this is deliberate and the reasoning is sound; it just means the two explanations are mutually exclusive rather than combined.

Scope, so this isn't read as broader than it is: I verified merge_gate.go by structural diff against the audited tip, plus serve.go, check_records.go, check_runs.go and clampDriftSummary directly. I did not read the 360 test lines or the metrics additions closely.

Not blocking.

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.

4 participants