feat(github): stop an apply that would discard work in progress

[aparajon]

Why this matters

A schema change that throws away an unfinished copy destroys work already done on the target, often hours of it, and until now it happened in one step. The plan comment disclosed the discard and the apply started in the same breath: by the time anyone read the warning, the copy was gone.

Disclosing something nobody can act on is not a safety property. This makes the disclosure a decision.

What it does

An automatic apply that would discard an unfinished copy stops for confirmation instead. The operator reads what is being destroyed on a comment where it still exists, then confirms, and the remedy on that comment is still reachable: applying the schema change the copy was made for resumes it rather than restarting it.

apply
  └─ plan discloses a discarded copy?
       ├─ no  → apply proceeds
       └─ yes → downgrade to apply-confirm, lock held, copy untouched
                  ├─ apply-confirm  → proceeds, copy destroyed
                  └─ unlock         → plan discarded, copy untouched

There is no flag that skips the stop. -y was recognized on the apply comment command and then never read, so every gate it looked like it would skip ran anyway; it is now rejected there, with a reply pointing at the surface it belongs to. The CLI keeps its own -y (--auto-approve), which skips an interactive prompt that genuinely exists. A comment has no prompt to skip, and consent to destroying hours of copying is not something a flag can express in advance. The flag is read off the parsed command rather than the comment body, so an environment ending in -y still applies and a comment that merely mentions --yes in prose is not rejected for it. The -d and --defer-cutover probes now read the same directive line, so prose or a fenced example mentioning either no longer rejects an unrelated command.

The gate also runs on the re-plan inside the automatic path. A copy can appear between review and confirmation (another apply starts one, or an adopted copy's checkpoint ages out), so a confirmation collected against a comment that showed no discard does not authorize one. The confirm path itself is exempt through the stored plan that marks the automatic path. That exemption is only as strong as the comment the operator confirmed: the disposition can flip after the disclosure, and this path does not know what that comment showed. Carrying the confirmed disclosure durably on the lock is the stacked follow-up's job.

Even where the gate fires, it is a prediction rather than enforcement: the disposition is read at the re-plan and the apply is queued, so an unbounded queue delay or apply-time statement routing can still change what the engine compares at dispatch. Closing that window needs a copy fingerprint carried on the apply request — a separate design change.

PlanResponse.DiscardedCopies() treats an unrecognized disposition as discarded. Deciding whether an operator must confirm before work is destroyed is not a place to fail open on a value this build does not know.

The pause is acknowledged only once the stored check state blocks the merge gate on the pending changes: the gate stores the check record before posting the paused comment, and a storage failure releases the lock and leaves the command retryable rather than pausing over unknown check state.

Automatic apply paused because it would discard a copy

Schema Change Apply — Staging

Database: testapp | Type: MySQL | Schema Name: testapp

Requested by @jackjackbits at 2026-01-01 00:00:00 UTC · planned from abcdef1

🔒 Lock acquired by block/schemabot#42 at 2026-01-15 14:30:00 UTC

ALTER TABLE `orders` ADD INDEX `idx_user_id`(`user_id`);

⚠️ Applying destroys work in progress: 1 unfinished copy on the target

• orders in testapp (last progress 3h 12m ago): the schema change differs from the one that started it, which was ALTER TABLE orders ADD INDEX idx_user_created (user_id, created_at)

Applying restarts the copy from zero rows. To keep the work already done, apply the schema change that started it.

📋 Plan: 1 table to alter

---

⚠️ Automatic apply paused: Applying destroys work in progress on the target

Review the plan above, then confirm manually:

schemabot apply-confirm -e staging

🔓 To discard this plan and unlock, comment:

schemabot unlock

The same copy on a comment announcing an apply already under way

Schema Change Apply — Staging

Database: testapp | Type: MySQL | Schema Name: testapp

Requested by @jackjackbits at 2026-01-01 00:00:00 UTC · planned from abcdef1

🔒 Lock acquired by block/schemabot#42 at 2026-01-15 14:30:00 UTC

ALTER TABLE `orders` ADD INDEX `idx_user_id`(`user_id`);

ℹ️ This apply destroys work in progress: 1 unfinished copy on the target

• orders in testapp (last progress 3h 12m ago): the schema change differs from the one that started it, which was ALTER TABLE orders ADD INDEX idx_user_created (user_id, created_at)

📋 Plan: 1 table to alter

---

Applying automatically

-y on a comment command

The -y flag is not supported for apply.

-y belongs to the CLI, where it skips an interactive confirmation prompt. A PR comment has no prompt to skip: when a command stops for confirmation, it is asking you to read what it discloses and reply with the confirm command it posts.

How it moves us toward the northstar

The engine has been able to predict this for a while, and the two PRs before this one made the prediction visible. Visibility alone still let the destructive step happen unattended. This is the link that turns a prediction into something an operator gets to decide, which is the difference between SchemaBot telling you what it did and SchemaBot asking first.

Opened by Claude (Opus 5).

[Copilot]

Pull request overview

This PR adds a confirmation gate for automatic schema applies that would discard unfinished copies, while updating comment-command flag handling and related UX.

Changes:

• Pause destructive automatic applies until apply-confirm or unlock.
• Handle discarded and unknown copy dispositions.
• Update templates, previews, documentation, and integration tests.
• Reject comment-level -y/--yes flags while retaining CLI auto-approval.

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Summary / review status
TEMPLATES.md	Adds paused-copy preview. Nit: correct the duration wording to “last progress 3h 12m ago.”
pkg/webhook/templates/preview.go	Adds the paused-plan preview fixture.
pkg/webhook/templates/issue_comment.go	Updates unsupported-flag guidance.
pkg/webhook/templates/issue_comment_test.go	Tests the updated guidance.
pkg/webhook/plan.go	Uses shared copy dispositions and downgrade messaging.
pkg/webhook/issue_comment.go	Moderate: restrict -y detection to parsed directives and valid token boundaries.
pkg/webhook/existing_copy_test.go	Aligns copy-disposition tests with API constants.
pkg/webhook/durable_issue_comment_test.go	Tests durable gate parity.
pkg/webhook/copy_discard_gate_integration_test.go	Moderate: verify the lock and copy state remain unchanged; nit: validate and close the database pool correctly.
pkg/webhook/commands.go	Removes comment-level auto-confirm support.
pkg/webhook/commands_test.go	Updates command parsing tests.
pkg/webhook/apply_integration_test.go	Updates automatic-apply integration coverage.
pkg/webhook/apply_handlers.go	Critical: fail closed on copy-inspection errors and ensure persistence failures cannot leave a passing check.
pkg/webhook/apply_execute.go	Critical: only bypass the re-plan gate when discard was disclosed; moderate: add coverage for copies appearing or expiring during re-planning.
pkg/cmd/internal/templates/preview.go	Registers the paused preview type.
pkg/cmd/internal/templates/preview_dispatch.go	Dispatches the paused preview.
pkg/cmd/internal/templates/preview_comment.go	Includes the preview in grouped output.
pkg/cmd/commands/preview.go	Exposes the new preview command.
pkg/apitypes/apitypes.go	Adds disposition constants and discard filtering.
pkg/apitypes/apitypes_test.go	Tests discarded-copy handling.
docs/check-runs.md	Documents automatic-apply gating behavior.

Suppressed comments (7)

pkg/webhook/apply_execute.go:157

• The re-plan gate has the same fail-open behavior: if the final findExistingCopy inspection fails, planResp.DiscardedCopies() is empty and this automatic apply continues. That can destroy a copy precisely when the last safety check cannot determine its disposition; propagate an unknown/error state and pause or reject rather than treating the read failure as no copy.

		if discarded := planResp.DiscardedCopies(); len(discarded) > 0 {

pkg/webhook/apply_execute.go:160

• This re-plan is only a snapshot: after it reports no discard, the handler still performs GitHub/App setup and queues the apply, while Spirit evaluates checkpoint age later. A copy just below the age limit can expire in that window and be discarded by an automatic apply without this gate running. The destructive-copy disposition needs admission-time or atomic revalidation (or the automatic path must fail closed on a changed target), not only this earlier prediction.

		if discarded := planResp.DiscardedCopies(); len(discarded) > 0 {
			h.logger.Info("automatic apply downgraded: re-plan discards an existing copy",
				"repo", repo, "pr", pr, "database", database, "environment", environment,
				"discarded_copies", len(discarded))

pkg/webhook/apply_execute.go:160

• The new re-plan downgrade log also omits database_type even though dbType is available. This makes a production pause harder to distinguish and triage across database engines; include the database type in the structured attributes.

			h.logger.Info("automatic apply downgraded: re-plan discards an existing copy",
				"repo", repo, "pr", pr, "database", database, "environment", environment,
				"discarded_copies", len(discarded))

pkg/webhook/apply_handlers.go:436

• postComment logs GitHub failures but returns no status, so this branch can store the plan and return success with the lock held even when the safety disclosure was never posted. The durable command will not retry, leaving the operator without the copy warning or the copy-pasteable apply-confirm/unlock actions. Make failure to publish this gate retryable, or persist a notification/reconciliation path before treating the downgrade as handled.

		h.postComment(repo, pr, installationID, templates.RenderPlanComment(commentData))

pkg/webhook/apply_handlers.go:434

• This new safety-transition log omits database_type, even though the handler has dbType in scope. When multiple engine types are routed through the same webhook logs, an operator cannot fully identify which target was paused from this event; include the canonical database type with the existing repo, PR, database, and environment fields.

		h.logger.Info("automatic apply downgraded: applying discards an existing copy",
			"repo", repo, "pr", pr, "database", database, "environment", environment,
			"discarded_copies", len(discarded))

pkg/webhook/apply_handlers.go:439

• storeApplyPlanCheckRecord writes SchemaBot's stored per-database check state and can fail while fetching the PR or writing storage; it does not create a per-database GitHub Check Run. This message misclassifies those failures as Check Run creation failures, which makes a safety-gate incident harder to triage. Log the stored-state operation instead and include the database, database type, and environment.

			h.logger.Error("failed to create apply plan check run", "repo", repo, "pr", pr, "error", checkRunErr)

pkg/webhook/templates/preview.go:226

• Age is checkpoint age, and the generated rendering labels it as “last progress ... ago”; it is not time spent copying or the amount of work that will be lost. The PR description's sample presents 3h 12m as “3h 12m of copying” and “already spent,” which overstates what this field measures. Align the sample with the generated text or expose a real copied-duration metric.

				Age:       "3h 12m",

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1102, 5853636.

Verdict: 6 findings — no blockers; 4 non-blocking (confirm-path gap, fail-open signal, flag-probe asymmetry, test coverage), 2 suggestions.

Non-blocking

1. The copy-discard gate is skipped on the apply-confirm path, and the comment justifying that skip states an invariant the code does not hold. pkg/webhook/apply_execute.go:156 wraps the whole gate in if storedPlan != nil {, and the confirm path passes nil: apply_handlers.go:745 — h.executeApply(..., result, nil, existingLock.PendingPlanID). The in-code justification at apply_execute.go:153-155 says the confirm path carries "the operator's own acknowledgement of the copy the comment they confirmed disclosed" — true only when the comment disclosed a discard.

   Concrete counterexample: a plan with one direct-execution change plus one engine ALTER downgrades at apply_handlers.go:410 — before the copy gate at :431 — so the locked comment renders the copy as an adopt ("♻️ Resuming work in progress"), since apitypes.go:626 filters adopts out of DiscardedCopies(). Hours later the disposition flips — c.Age >= maxAge with DefaultCheckpointMaxAge = 3 * 24 * time.Hour (settings.go:18), or c.Statement != statement because apply-time routing changed the joined batch. Confirm dispatches; the copy is destroyed with only a server-side logger.Warn, no PR comment. Confirm-time staleness is SHA-only (plan_freshness.go:78), so there is no age bound on how long a pending confirmation may sit.

   Not a regression — main had no gate on either path — and the stacked child PR 1128 removes exactly this storedPlan != nil wrapper in favour of a !disclosedCopyDiscard flag persisted on the lock. So the actionable ask is: land 1102 and 1128 together, or at minimum fix the comment here so it doesn't assert an invariant that only holds for discard-disclosing comments.

2. The signal the gate depends on fails open, in the opposite direction from every consumer of it. pkg/engine/spirit/existing_copy.go:161-163 logs "cannot tell whether applying this plan continues or discards an existing copy; the plan discloses nothing" and then return nil. Nothing on the wire distinguishes "clean target" from "could not tell", so DiscardedCopies() is empty and both gates (apply_handlers.go:431, apply_execute.go:157) silently no-op while Spirit discards anyway. Meanwhile apitypes.go:624 deliberately fails toward discard ("an unrecognized verdict is not a reason to skip that confirmation"). The producer's bias defeats the consumer's.

   Two corrections to how this is easy to overstate: an unreachable target does not trigger it — fetchCurrentSchema fails the whole plan first — and the file is not in this PR's diff. The live triggers are partial failures after a successful schema read: the copy probe exceeding copyDetectionTimeout = 10 * time.Second (existing_copy.go:47) on a loaded target, and — the sharper, non-transient case — a failure in the separately opened checkpoint pool (:310) or checkpoint read (:316) for a copy whose shadow tables were already found. A missing SELECT grant on _spirit_checkpoint makes the gate no-op on every plan for that database, indefinitely. readCheckpoint's own doc promises "an unreadable target can never be mistaken for a target with no checkpoint" — line 163 collapses precisely that distinction.

   The finding is against this PR because this PR is what promotes the disclosure from informational to consent-bearing; the in-file justification ("which leaves the plan exactly as it is without this check") goes stale the moment that happens. Follow-up shape: surface a detection-failed signal on PlanResponse and downgrade on it, the same way apply_handlers.go downgrades when the stored plan cannot be loaded.

3. HasAutoConfirmFlag was narrowed to the directive line; its two siblings were not, so the gate ladder now applies two definitions of "carries a flag". commands.go:378 is still return p.databaseRegex.MatchString(body) and :384 return p.deferCutoverRegex.MatchString(body), both whole-body, while :367 now goes through firstDirectiveLine(markdownDirectiveText(body)). The raw body is what reaches all three (issue_comment.go:98, :101), so schemabot stop apply_abc123 -e production followed by prose mentioning -d accounts is still rejected as "database flag unsupported", and a schemabot rollback whose prose mentions --defer-cutover is still rejected. Empirically confirmed against the actual regexes.

   The false rejections are pre-existing — on main all three probes matched the whole body — so this PR does not break these paths; it fixes one of three and leaves the inconsistency. Follow-up is mechanical (route both siblings through the same directive-line helper), and should add prose/fence cases to TestHasDatabaseFlag, which today only exercises single-line bodies and would pass either way.

4. No test reaches the executeApply re-plan gate — the branch that exists specifically for the check-to-use window is unexercised on both sides. The only new behavioural test, copy_discard_gate_integration_test.go:125, posts one comment — "schemabot apply -e staging" at :130 — which stops at the applyCommandCore gate and returns before apply_handlers.go:490, so executeApply is never entered. msgCopyDiscardDowngrade appears in zero _test.go files, and no test invokes executeApply directly.

   Green CI is therefore not evidence for the executeApply gate or for the confirm-path exemption. Deleting apply_execute.go:156-165 still compiles (storedPlan remains used at :128 and :140) and leaves the whole suite green; so would flipping the condition to storedPlan == nil, which would break apply-confirm for every PR whose target holds a stale copy. Green CI is evidence for the applyCommandCore gate — deleting apply_handlers.go:431 fails this test's paused-comment and requireCopyIntact assertions.

General suggestions

5. A durable schemabot apply -e <env> -y enqueued by the previous build posts nothing back to the PR under the new build. The gate changed from result.Action != action.Apply && parser.HasAutoConfirmFlag(...) to plain issue_comment.go:95 if parser.HasAutoConfirmFlag(commentBody) {. During rollout the durable driver picks up such a row, issueCommentGateAutoConfirm is not in the narrow reason list, and control falls to :993 — warn, durable_command_routing_blocked, return false, nil. Not truly silent (that path is deliberately operator-visible per its own comment) and not universally preceded by a reaction, but the PR author gets no "unsupported flag" reply. Exposure is bounded to pre-PR rows carrying -y on apply plus retry-delayed rows, and it self-corrects on re-comment. Worth a line in the deploy note rather than code.

6. Even on the automatic path the gate is a prediction, not enforcement — worth saying so in the PR title's promise. The disposition is read at the re-plan and never re-read; apply_execute.go:249 applyResp, applyID, err := h.service.ExecuteApply(ctx, applyReq) sends only a PlanID, and ExecuteApply queues the apply. Note the window is not "several network round-trips between check and dispatch" — FetchPullRequestNoCache and assertBaseSchemaStillCurrent run before the gate, annotateAttributedChanges is on a branch that returns, and factoryForRepo is an in-memory lookup. The real residual is (a) unbounded queue delay, so a checkpoint can cross checkpointMaxAge with no concurrency at all, and (b) apply-time routeAlterStatements recomputing direct-vs-engine against the live table, changing the batch string and count Spirit compares. Closing this needs a copy fingerprint threaded through ApplyRequest — a separate design change, not this PR. The file already documents the property ("This only predicts it"); the PR description could too.

The one thing that could have broken, verified

The riskiest mechanism is the time-of-check-to-time-of-use window the PR exists to close: the copy disposition is read at plan time and consumed at dispatch time, with an operator in between. I traced all three consumption points and the single producer.

• The automatic one-step path is genuinely closed for the common case: apply_handlers.go:431 stops before the stored-plan load and before executeApply, and apply_execute.go:157 re-checks against the re-plan for copies that appear or age out after review. Proven safe end-to-end by TestE2EDiscardingCopyDowngradesToConfirm, which asserts the paused comment and requireCopyIntact.
• The two-step confirm path is not closed — proven unsafe, not merely unproven. grep over the repo finds exactly two production consumers of DiscardedCopies(), both on the automatic path; applyConfirmCommandCore reads copies nowhere; and the storedPlan != nil wrapper is what excludes it. The disposition flip needs no second actor (checkpoint age alone suffices, and confirm-time freshness is SHA-only with no age bound), and the outcome is PR-silent. Decisive corroboration: the stacked child PR 1128 deletes this exact wrapper and replaces it with len(discarded) > 0 && !disclosedCopyDiscard, persisting the disclosure on the lock.
• The producer defeats both on partial read failure (existing_copy.go:163 return nil), and there is no wire state for "could not tell". TestPlanDisclosesNothingWhenTheTargetCannotBeRead pins this as intended, so it corroborates rather than refutes.

Net: the gate is a real improvement over main (which had no gate at all), fails safe where it fires, and the residual holes are incomplete coverage rather than new destruction — none of it is a regression.

Verified correct

• applyCommandCore's gate returns false, nil before the stored-plan load and before executeApply, so a discard-disclosing plan cannot fall through to dispatch on the one-step path.
• DiscardedCopies() fails toward discard for an unrecognized disposition, and plan.go renders an unrecognized disposition as a discard — both deliberate, both documented, both correct for a consent gate.
• splitExistingCopies and the adopt/discard rendering split are unit-tested, and the disposition constants are asserted for engine/apitypes parity.
• Adopted copies are correctly excluded from DiscardedCopies(), so an adopt renders as "♻️ Resuming work in progress" rather than a false alarm.
• The auto-confirm flag narrowing is right and well-tested: a -y inside a fenced example or in prose no longer counts as passing the flag, with explicit fence/prose test cases.
• Removing SupportsAutoConfirm from the action.Apply spec and result.AutoConfirm from CommandResult is a complete sweep — no orphaned readers remain.
• plannedSpiritBatch correctly skips ExecutionModeDirect entries, so a mixed direct+engine plan still detects and discloses the copy for the engine half.
• Deleting the executeApply discard block would still compile (storedPlan remains used at :128 and :140) — confirming the block is additive and not load-bearing for the surrounding control flow.
• Downgrade paths (DDL drift, direct changes, plan-load failure, copy discard) all route through postAutoConfirmDowngrade and set AutoConfirmDowngradeReason, so applyingWithoutConfirmation() is false and the comment correctly asks for confirmation.

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

[aparajon]

🤖 Thanks — addressed at 03f7eb7, finding by finding:

1. Confirm-path comment — fixed in 03f7eb7. The comment no longer claims the confirm path carries an acknowledgement of this copy; it now states the exemption's real boundary: the disposition can flip after the disclosure, and this path does not know what the confirmed comment showed. The durable fix (persisting the disclosure on the lock) remains the stacked follow-up, which will be restacked onto this head.

2. Producer fails open (existing_copy.go returning nil on a partial read) — agreed, and accepted as a follow-up in the shape you suggest: a detection-failed signal on PlanResponse that the gates downgrade on, mirroring the plan-load-failure downgrade. It needs a wire change, so it is not folded into this PR.

3. Flag-probe asymmetry — fixed in 4e3159c. HasDatabaseFlag and HasDeferCutoverFlag now read the same directive line as HasAutoConfirmFlag, with prose and fenced-example cases pinned for each.

4. The re-plan gate was unexercised — fixed in 03f7eb7. TestE2EReplanDiscardingCopyDowngradesToConfirm drives the dispatch core directly with a plan stored against a clean target and a copy seeded inside the window: the apply downgrades, the copy survives, and no apply starts. Mutation-verified — disabling the gate condition fails the test.

5. Durable pre-rollout -y rows — agreed this is a deploy-note line rather than code; it will be called out in the release notes for the release that ships this.

6. Prediction, not enforcement — the PR description now says so, alongside the confirm-exemption caveat from finding 1.

This reply was generated by Claude Code (Claude Fable 5).