fix(github): route rollback through direct-execution disclosure and consent gates - #897
Draft
aparajon wants to merge 2 commits into
Draft
fix(github): route rollback through direct-execution disclosure and consent gates#897aparajon wants to merge 2 commits into
aparajon wants to merge 2 commits into
Conversation
…onsent gates The rollback flow now follows the same consent model as the apply flow when a reverse plan carries direct-execution changes: - The rollback plan comment renders the ⚙️ direct-execution disclosure, with the consent sentence naming rollback confirmation — the comment rollback-confirm consents against spells out the consequences. - A reverse plan containing engine-blocked changes is rejected before a plan is pinned for confirmation (a blocked change guarantees the rollback apply fails), with a rollback-titled ⛔ rejection comment and the lock left free. - rollback-confirm gains matching gates on the lock-pinned stored plan: a blocked pinned plan is rejected and the lock released (defense in depth), and --defer-cutover on an all-direct rollback plan is rejected while preserving the pending rollback. Stored plans already persist the per-table execution-mode verdict, so storage.TableChange gains EngineBlocked/DirectExecution helpers mirroring the apitypes ones, and the webhook package gains stored-plan equivalents of HasBlockedChanges/AllChangesDirect. The blocked-rejection renderer and the plan-comment blocked/direct population are extracted into shared helpers used by both flows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR aligns the rollback workflow with the existing apply “disclosure → consent” contract for direct-execution DDL, and adds safety gates so rollbacks that are guaranteed to fail (engine-blocked reverse plans) are rejected before pinning/confirmation.
Changes:
- Render the same direct-execution disclosure section on rollback plan comments, with consent wording tied to
rollback-confirm. - Reject reverse rollback plans that contain engine-blocked changes before pinning a plan; add confirm-time defense-in-depth gates for blocked pinned plans and
--defer-cutoveron all-direct rollbacks. - Refactor/extend shared template and plan-comment data plumbing; add new previews, docs, and integration tests for rollback direct-execution behavior.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| TEMPLATES.md | Adds rendered examples for rollback direct-execution disclosure and rollback rejection on engine-blocked changes. |
| pkg/webhook/templates/rollback.go | Renders direct-execution disclosure on rollback plan comments. |
| pkg/webhook/templates/rollback_test.go | Adds unit tests validating rollback direct disclosure and blocked-change rollback rejection rendering. |
| pkg/webhook/templates/preview.go | Adds preview renderers for rollback direct plan and rollback blocked rejection comments. |
| pkg/webhook/templates/plan.go | Parameterizes direct-execution consent sentence and appends it to engine-specific semantics copy. |
| pkg/webhook/templates/direct_test.go | Updates direct-consent copy test to reflect split semantics vs consent sentence. |
| pkg/webhook/templates/apply_commands.go | Shares blocked-changes rejection rendering between apply and rollback variants. |
| pkg/webhook/rollback.go | Implements rollback-time and confirm-time gates for engine-blocked plans and all-direct defer-cutover; wires comment data for direct/blocked changes. |
| pkg/webhook/rollback_direct_integration_test.go | Adds integration tests covering rollback direct disclosure, defer-cutover rejection (lock preserved), blocked reverse rejection, and blocked pinned-plan confirm rejection. |
| pkg/webhook/plan.go | Extracts helpers to build blocked/direct change template data and reuses them across flows. |
| pkg/storage/types.go | Adds EngineBlocked() / DirectExecution() helpers to storage.TableChange based on persisted execution-mode verdict. |
| pkg/cmd/internal/templates/preview.go | Adds new preview type constants for rollback plan direct and rollback blocked rejection. |
| pkg/cmd/internal/templates/preview_dispatch.go | Wires new preview types to template preview renderers. |
| pkg/cmd/internal/templates/preview_comment.go | Includes new rollback preview sections in the “all comments” preview output. |
| pkg/cmd/commands/preview.go | Allows CLI preview command to select the new rollback preview types. |
| docs/direct-execution.md | Documents that rollback follows the same disclosure/consent model and rejects blocked reverse plans up front. |
Suppressed comments (1)
pkg/webhook/rollback.go:432
- The
--defer-cutoverrejection coaches re-runningschemabot rollback-confirm -e …using only the environment placeholder. In tenant-scoped deployments, the re-run also needs--tenant <tenant>or the command will be ignored by routing.
Consider formatting the coached command with environment + " --tenant <tenant>" when h.deploymentTenant() is set (similar to the tenant-aware hints in the plan/rollback templates).
h.postCommandError(repo, pr, installationID, action.RollbackConfirm, environment, requestedBy,
fmt.Sprintf(msgDeferCutoverAllDirectRollbackConfirm, environment))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…hints In tenant mode, commands without an explicit tenant target are ignored, so every pasteable command hint must carry the deployment's tenant. The confirm-time rejection messages (blocked rollback plan, --defer-cutover on an all-direct plan) coached bare commands; route them through the templates package's tenant-aware command builders. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why this matters
Some statements are routed to direct execution: SchemaBot runs them as native MySQL DDL instead of as an online schema change. That changes what an operator is agreeing to — the statement blocks writes to the table while it runs, cannot be reverted afterward, and ignores
--defer-cutover(there is no cutover to defer). The forward apply flow already handles this honestly: the plan comment discloses the direct changes, and confirming the apply is the consent.The rollback flow did not.
schemabot rollbackproduced reverse plans through the same policy without disclosing anything, andschemabot rollback-confirmexecuted the pinned plan without the direct or blocked gates. Two bad outcomes were possible: an operator could confirm a rollback that runs blocking, non-revertible DDL with no warning, or pin a rollback whose reverse plan the engine refuses to run — guaranteeing the confirm fails after the database lock is already taken.What it does
rollback-confirmis the consent, so the disclosure sits on the comment the confirmation acts on. Apply-comment copy is byte-identical; only the final consent sentence is parameterized.schemabot rollbackrejects it up front with a rollback-specific comment (reconcile with a follow-up schema change PR instead), releases the lock, and never offersrollback-confirm.rollback-confirmgates on the stored pinned plan. A blocked pinned plan (defense-in-depth for pins that predate the gate) is rejected and the lock released — no confirm retry can succeed.--defer-cutoveron an all-direct rollback plan is rejected without releasing the lock: the flag has no effect on direct DDL, and the pending rollback is preserved so a bare re-run confirms it.storage.TableChangegainsEngineBlocked()/DirectExecution()helpers so webhook gates can read the persisted execution-mode verdict, and the blocked-rejection template is shared between the apply and rollback variants.Northstar: every operator command that can execute DDL carries the same disclosure-then-consent contract, regardless of which direction the schema is moving.
🤖 Generated with Claude Code