From f8d22acf12dd610d6617144713afd381ffcc8217 Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Fri, 31 Jul 2026 22:43:32 -0400 Subject: [PATCH 1/2] fix(github): route rollback through direct-execution disclosure and consent gates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- TEMPLATES.md | 66 ++++ docs/direct-execution.md | 9 + pkg/cmd/commands/preview.go | 2 + pkg/cmd/internal/templates/preview.go | 2 + pkg/cmd/internal/templates/preview_comment.go | 4 + .../internal/templates/preview_dispatch.go | 4 + pkg/storage/types.go | 21 ++ pkg/webhook/plan.go | 102 +++--- pkg/webhook/rollback.go | 141 +++++++- .../rollback_direct_integration_test.go | 329 ++++++++++++++++++ pkg/webhook/templates/apply_commands.go | 26 +- pkg/webhook/templates/direct_test.go | 1 - pkg/webhook/templates/plan.go | 35 +- pkg/webhook/templates/preview.go | 49 +++ pkg/webhook/templates/rollback.go | 8 + pkg/webhook/templates/rollback_test.go | 48 +++ 16 files changed, 769 insertions(+), 78 deletions(-) create mode 100644 pkg/webhook/rollback_direct_integration_test.go diff --git a/TEMPLATES.md b/TEMPLATES.md index 0d005c5ac..a44e7711f 100644 --- a/TEMPLATES.md +++ b/TEMPLATES.md @@ -4415,6 +4415,72 @@ ALTER TABLE `users` DROP INDEX `idx_email`; ``` + + + +
+Rollback Plan (Direct-execution Change) + + +## Schema Rollback Plan — Staging + +**Database**: `testapp` | **Type**: `MySQL` + +*Requested by @jackjackbits at 2026-01-01 00:00:00 UTC* + +```sql +ALTER TABLE `users` + DROP PRIMARY KEY, + ADD PRIMARY KEY(`id`); +``` + +⚙️ **Direct execution**: **1** change will run as native MySQL DDL +- `users`: dropping primary key is not supported; runs as native MySQL DDL on a table with ~1,240 rows + +These statements run synchronously outside the schema-change engine: writes to each table are blocked while its statement runs, the change is **not revertible**, and `--defer-cutover` does not apply to it. Confirming the rollback consents to this. + +> **Warning**: Rollback may include destructive changes (e.g., DROP INDEX, DROP COLUMN). These will be applied automatically. + +📋 **Plan**: **1** table to alter + +--- + +To confirm this rollback, comment: +``` +schemabot rollback-confirm -e staging +``` + +To cancel, comment: +``` +schemabot unlock +``` + +
+ +
+Rollback Rejected (Engine-blocked Changes) + + +## Schema Rollback Plan — Staging + +**Database**: `testapp` | **Type**: `MySQL` + +*Requested by @jackjackbits at 2026-01-01 00:00:00 UTC* + +```sql +ALTER TABLE `users` + DROP PRIMARY KEY, + ADD PRIMARY KEY(`id`); +``` + +📋 **Plan**: **1** table to alter + +--- + +**⛔ Rollback rejected**: **1** planned change not supported by the schema-change engine +- `users`: dropping primary key is not supported; direct execution is enabled but the table has ~2,400,000 rows, above the configured limit of 1,000,000 + +Reconcile the target schema with a follow-up schema change PR instead, or contact your SchemaBot operators for help.
### CLI Output diff --git a/docs/direct-execution.md b/docs/direct-execution.md index 1a4499851..d1af528cb 100644 --- a/docs/direct-execution.md +++ b/docs/direct-execution.md @@ -151,6 +151,15 @@ containing direct-execution changes never does: statements only, and the disclosure says so. A rejection at confirm time preserves the pending confirmation, so re-running `apply-confirm` without the flag executes the confirmed plan. +- `schemabot rollback` follows the same consent model. A rollback plan whose + reverse DDL the policy routes to direct execution carries the ⚙️ disclosure + on the rollback plan comment, and `schemabot rollback-confirm` is the + consent against it — including the all-direct `--defer-cutover` rejection, + which preserves the pending rollback. A reverse plan that resolves to + blocked (e.g. the table grew past the size bound since the apply) is + rejected before anything is pinned for confirmation: a blocked change + guarantees the rollback would fail, so the target must be reconciled with a + follow-up schema change instead. ## Observability diff --git a/pkg/cmd/commands/preview.go b/pkg/cmd/commands/preview.go index 656c910aa..6f069850d 100644 --- a/pkg/cmd/commands/preview.go +++ b/pkg/cmd/commands/preview.go @@ -73,6 +73,8 @@ func (cmd *PreviewCmd) Run(g *Globals) error { case templates.PreviewCommentPlan, templates.PreviewCommentPlanBlocked, templates.PreviewCommentPlanDirect, templates.PreviewCommentApplyBlockedRejected, + templates.PreviewCommentRollbackPlanDirect, + templates.PreviewCommentRollbackBlockedRejected, templates.PreviewCommentPlanTenant, templates.PreviewCommentPlanEmpty, templates.PreviewCommentNoManagedSchema, diff --git a/pkg/cmd/internal/templates/preview.go b/pkg/cmd/internal/templates/preview.go index 688b2b708..1f05a1f78 100644 --- a/pkg/cmd/internal/templates/preview.go +++ b/pkg/cmd/internal/templates/preview.go @@ -114,6 +114,8 @@ const ( PreviewCommentPlanBlocked PreviewType = "comment_plan_blocked" // Plan with a statement the engine refuses (blocked verdict) PreviewCommentPlanDirect PreviewType = "comment_plan_direct" // Locked plan with a statement routed to direct execution (direct verdict) PreviewCommentApplyBlockedRejected PreviewType = "comment_apply_blocked_rejected" // Apply rejected: plan contains engine-blocked statements + PreviewCommentRollbackPlanDirect PreviewType = "comment_rollback_plan_direct" // Rollback plan with a reverse statement routed to direct execution + PreviewCommentRollbackBlockedRejected PreviewType = "comment_rollback_blocked_rejected" // Rollback rejected: reverse plan contains engine-blocked statements PreviewCommentPlanTenant PreviewType = "comment_plan_tenant" // Tenant-targeted plan comment PreviewCommentPlanEmpty PreviewType = "comment_plan_empty" // Plan comment with no changes PreviewCommentNoManagedSchema PreviewType = "comment_no_managed_schema" // No managed schema changes in current PR diff --git a/pkg/cmd/internal/templates/preview_comment.go b/pkg/cmd/internal/templates/preview_comment.go index 98d37ec1f..a74363cb3 100644 --- a/pkg/cmd/internal/templates/preview_comment.go +++ b/pkg/cmd/internal/templates/preview_comment.go @@ -106,6 +106,8 @@ func previewCommentAllOutput() { {"SUMMARY: MULTI-NAMESPACE COMPLETED", func() { fmt.Print(webhooktemplates.PreviewCommentSummaryMultiNamespaceCompleted()) }}, {"ROLLBACK STATUS: RUNNING", func() { fmt.Print(webhooktemplates.PreviewCommentRollbackStatus()) }}, {"SUMMARY: ROLLBACK COMPLETE", func() { fmt.Print(webhooktemplates.PreviewCommentRollbackSummaryCompleted()) }}, + {"ROLLBACK PLAN (DIRECT-EXECUTION CHANGE)", func() { fmt.Print(webhooktemplates.PreviewCommentRollbackPlanDirect()) }}, + {"ROLLBACK REJECTED (ENGINE-BLOCKED CHANGES)", func() { fmt.Print(webhooktemplates.PreviewCommentRollbackBlockedRejected()) }}, } for i, s := range sections { @@ -282,6 +284,8 @@ func previewCommentApplyFlowAllOutput() { {"SUMMARY: MULTI-NAMESPACE COMPLETED", func() { fmt.Print(webhooktemplates.PreviewCommentSummaryMultiNamespaceCompleted()) }}, {"ROLLBACK STATUS: RUNNING", func() { fmt.Print(webhooktemplates.PreviewCommentRollbackStatus()) }}, {"SUMMARY: ROLLBACK COMPLETE", func() { fmt.Print(webhooktemplates.PreviewCommentRollbackSummaryCompleted()) }}, + {"ROLLBACK PLAN (DIRECT-EXECUTION CHANGE)", func() { fmt.Print(webhooktemplates.PreviewCommentRollbackPlanDirect()) }}, + {"ROLLBACK REJECTED (ENGINE-BLOCKED CHANGES)", func() { fmt.Print(webhooktemplates.PreviewCommentRollbackBlockedRejected()) }}, } printSections(sections) } diff --git a/pkg/cmd/internal/templates/preview_dispatch.go b/pkg/cmd/internal/templates/preview_dispatch.go index d1e6dd7fa..0d27100d5 100644 --- a/pkg/cmd/internal/templates/preview_dispatch.go +++ b/pkg/cmd/internal/templates/preview_dispatch.go @@ -122,6 +122,10 @@ func PreviewCLIOutput(previewType PreviewType) { fmt.Print(webhooktemplates.PreviewCommentPlanDirect()) case PreviewCommentApplyBlockedRejected: fmt.Print(webhooktemplates.PreviewCommentApplyBlockedRejected()) + case PreviewCommentRollbackPlanDirect: + fmt.Print(webhooktemplates.PreviewCommentRollbackPlanDirect()) + case PreviewCommentRollbackBlockedRejected: + fmt.Print(webhooktemplates.PreviewCommentRollbackBlockedRejected()) case PreviewCommentPlanTenant: fmt.Print(webhooktemplates.PreviewCommentPlanTenant()) case PreviewCommentPlanEmpty: diff --git a/pkg/storage/types.go b/pkg/storage/types.go index e13d6ddec..abe30576b 100644 --- a/pkg/storage/types.go +++ b/pkg/storage/types.go @@ -360,6 +360,27 @@ func (tc TableChange) UnsafeOptInReason() string { return "unsafe schema change requires explicit opt-in" } +// Execution-mode verdict values a stored plan carries per table change. The +// canonical vocabulary lives with the engines; storage keeps its own copies so +// this package stays dependency-free. +const ( + executionModeBlocked = "blocked" + executionModeDirect = "direct" +) + +// EngineBlocked reports whether the planner recorded that the engine +// deterministically refuses this change. A blocked change guarantees the +// apply fails, so gates on stored plans reject them instead of executing. +func (tc TableChange) EngineBlocked() bool { + return strings.EqualFold(tc.ExecutionMode, executionModeBlocked) +} + +// DirectExecution reports whether the planner routed this change to direct +// execution as native DDL on the target. +func (tc TableChange) DirectExecution() bool { + return strings.EqualFold(tc.ExecutionMode, executionModeDirect) +} + // NamespacePlanData contains plan data for a single namespace. OriginalFiles is // captured once for the namespace and applies to every table/artifact change in // Tables and Artifacts. diff --git a/pkg/webhook/plan.go b/pkg/webhook/plan.go index 546c9e9a4..66631d1c7 100644 --- a/pkg/webhook/plan.go +++ b/pkg/webhook/plan.go @@ -665,6 +665,13 @@ const msgDeferCutoverAllDirect = "`--defer-cutover` has no effect on this plan: // The format verb takes the environment for the coached command. const msgDeferCutoverAllDirectConfirm = "`--defer-cutover` has no effect on this plan: every change runs directly as native DDL, which has no cutover to defer. The pending confirmation is preserved — re-run `schemabot apply-confirm -e %s` without the flag." +// msgDeferCutoverAllDirectRollbackConfirm rejects --defer-cutover at rollback +// confirmation on an all-direct rollback plan. The rejection preserves the +// pending rollback — the lock still pins the plan the operator confirmed +// against — so the recovery is re-running rollback-confirm without the flag. +// The format verb takes the environment for the coached command. +const msgDeferCutoverAllDirectRollbackConfirm = "`--defer-cutover` has no effect on this rollback: every change runs directly as native DDL, which has no cutover to defer. The pending rollback is preserved — re-run `schemabot rollback-confirm -e %s` without the flag." + // shardedDirectChanges collects direct-execution per-shard changes, grouped by // (table, reason) so a change present on several shards lists them together // rather than repeating. Returns nil when the plan carries no per-shard @@ -737,6 +744,57 @@ func shardedBlockedChanges(shards []*apitypes.ShardPlanResponse) []templates.Blo return out } +// blockedChangesData collects a plan's engine-blocked changes for rendering — +// the apply commands will reject these. Like the unsafe view, a sharded plan +// derives them per shard so a blocked change confined to one shard names the +// shard it applies to; otherwise the namespace-level view is used. +func blockedChangesData(planResp *apitypes.PlanResponse) []templates.BlockedChangeData { + if blocked := shardedBlockedChanges(planResp.Shards); len(blocked) > 0 { + return blocked + } + var out []templates.BlockedChangeData + for _, sc := range planResp.Changes { + if sc == nil { + continue + } + for _, t := range sc.TableChanges { + if !t.EngineBlocked() { + continue + } + out = append(out, templates.BlockedChangeData{ + Table: t.TableName, + Reason: t.ModeReason, + }) + } + } + return out +} + +// directChangesData collects a plan's direct-execution changes for rendering — +// the policy routes these to native MySQL DDL. Derived the same way as the +// blocked view. +func directChangesData(planResp *apitypes.PlanResponse) []templates.DirectChangeData { + if direct := shardedDirectChanges(planResp.Shards); len(direct) > 0 { + return direct + } + var out []templates.DirectChangeData + for _, sc := range planResp.Changes { + if sc == nil { + continue + } + for _, t := range sc.TableChanges { + if !t.DirectExecution() { + continue + } + out = append(out, templates.DirectChangeData{ + Table: t.TableName, + Reason: t.ModeReason, + }) + } + } + return out +} + // buildPlanCommentData converts plan results into template data. func buildPlanCommentData(schema *ghclient.SchemaRequestResult, planResp *apitypes.PlanResponse, environment, tenant, requestedBy string) templates.PlanCommentData { data := templates.PlanCommentData{ @@ -820,48 +878,8 @@ func buildPlanCommentData(schema *ghclient.SchemaRequestResult, planResp *apityp } } - // Blocked changes — the apply commands will reject these. Like the - // unsafe view, a sharded plan derives them per shard so a blocked change - // confined to one shard names the shard it applies to. - if blocked := shardedBlockedChanges(planResp.Shards); len(blocked) > 0 { - data.BlockedChanges = blocked - } else { - for _, sc := range planResp.Changes { - if sc == nil { - continue - } - for _, t := range sc.TableChanges { - if !t.EngineBlocked() { - continue - } - data.BlockedChanges = append(data.BlockedChanges, templates.BlockedChangeData{ - Table: t.TableName, - Reason: t.ModeReason, - }) - } - } - } - - // Direct-execution changes — the policy routes these to native MySQL DDL, - // derived the same way as the blocked view. - if direct := shardedDirectChanges(planResp.Shards); len(direct) > 0 { - data.DirectChanges = direct - } else { - for _, sc := range planResp.Changes { - if sc == nil { - continue - } - for _, t := range sc.TableChanges { - if !t.DirectExecution() { - continue - } - data.DirectChanges = append(data.DirectChanges, templates.DirectChangeData{ - Table: t.TableName, - Reason: t.ModeReason, - }) - } - } - } + data.BlockedChanges = blockedChangesData(planResp) + data.DirectChanges = directChangesData(planResp) // Add lint violations (error-severity results are shown via UnsafeChanges instead) for _, w := range planResp.LintNonErrors() { diff --git a/pkg/webhook/rollback.go b/pkg/webhook/rollback.go index 778dfe42e..de555aa86 100644 --- a/pkg/webhook/rollback.go +++ b/pkg/webhook/rollback.go @@ -19,6 +19,17 @@ const ( vSchemaArtifactName = "vschema.json" ) +// msgRollbackConfirmBlockedPlan rejects rollback-confirm when the pinned +// rollback plan carries engine-blocked changes: the rollback apply is +// guaranteed to fail, so there is nothing that can be confirmed. The format +// verb takes the environment for the coached command. +const msgRollbackConfirmBlockedPlan = "The pinned rollback plan contains changes the schema-change engine does not support, so this rollback cannot execute. The lock has been released — re-run `schemabot rollback -e %s` to generate a fresh plan, or contact your SchemaBot operators for help." + +// msgRollbackConfirmBlockedPlanLockHeld is the variant posted when the lock +// release itself failed: the operator must unlock before anything else can +// run on the database. +const msgRollbackConfirmBlockedPlanLockHeld = "The pinned rollback plan contains changes the schema-change engine does not support, so this rollback cannot execute. SchemaBot failed to release the database lock — release it with `schemabot unlock`, then re-run `schemabot rollback -e %s` to generate a fresh plan, or contact your SchemaBot operators for help." + // handleRollbackCommand handles the "schemabot rollback -e " PR comment command. // It looks up the specified apply, generates a rollback plan from its original schema files, // acquires a lock, and posts the plan for confirmation. @@ -199,23 +210,6 @@ func (h *Handler) handleRollbackCommand(repo string, pr int, installationID int6 return } - lock := &storage.Lock{ - DatabaseName: database, - DatabaseType: dbType, - Owner: lockOwner, - Repository: repo, - PullRequest: pr, - PendingPlanID: rollbackPendingPlanID(planResp.PlanID), - } - if err := lockStore.Acquire(ctx, lock); err != nil { - h.releaseRollbackLockAfterRejectedPlan(ctx, database, dbType, lockOwner, lockAcquiredByCommand) - h.logger.Error("failed to pin rollback plan on lock", "repo", repo, "pr", pr, - "database", database, "database_type", dbType, "environment", environment, - "plan_id", planResp.PlanID, "error", err) - h.postCommandError(repo, pr, installationID, action.Rollback, environment, requestedBy, "Failed to pin rollback plan on lock: "+err.Error()) - return - } - // Build comment data. The source apply ID stays in the comment metadata for // auditability, but rollback-confirm loads the lock-pinned rollback plan so // the user does not need to repeat the apply ID. @@ -250,6 +244,40 @@ func (h *Handler) handleRollbackCommand(repo string, pr int, installationID int6 }) } commentData.Errors = planResp.Errors + commentData.BlockedChanges = blockedChangesData(planResp) + commentData.DirectChanges = directChangesData(planResp) + + // A rollback plan can carry statements the engine refuses that the direct + // execution policy does not route (e.g. re-reshaping a primary key back on + // a table that grew past the policy's size bound). A blocked change + // guarantees the rollback apply fails, so reject it before pinning a plan + // that could never be confirmed. + if planResp.HasBlockedChanges() { + h.releaseRollbackLockAfterRejectedPlan(ctx, database, dbType, lockOwner, lockAcquiredByCommand) + h.logger.Warn("rollback rejected: plan contains engine-blocked changes", + "repo", repo, "pr", pr, "apply_id", applyID, + "database", database, "database_type", dbType, + "environment", environment, "plan_id", planResp.PlanID) + h.postComment(repo, pr, installationID, templates.RenderBlockedChangesRollbackRejected(commentData)) + return + } + + lock := &storage.Lock{ + DatabaseName: database, + DatabaseType: dbType, + Owner: lockOwner, + Repository: repo, + PullRequest: pr, + PendingPlanID: rollbackPendingPlanID(planResp.PlanID), + } + if err := lockStore.Acquire(ctx, lock); err != nil { + h.releaseRollbackLockAfterRejectedPlan(ctx, database, dbType, lockOwner, lockAcquiredByCommand) + h.logger.Error("failed to pin rollback plan on lock", "repo", repo, "pr", pr, + "database", database, "database_type", dbType, "environment", environment, + "plan_id", planResp.PlanID, "error", err) + h.postCommandError(repo, pr, installationID, action.Rollback, environment, requestedBy, "Failed to pin rollback plan on lock: "+err.Error()) + return + } h.postComment(repo, pr, installationID, templates.RenderRollbackPlanComment(commentData)) } @@ -368,6 +396,43 @@ func (h *Handler) handleRollbackConfirmCommand(repo string, pr int, environment return } + // A pinned rollback plan with engine-blocked changes can never execute — + // the rollback command rejects such plans before pinning, so reaching one + // here means the pin predates the gate or storage changed out of band. No + // retry of rollback-confirm can succeed, so release the lock: re-running + // the rollback command re-plans through the up-front gate. + if storedPlanHasBlockedChanges(rollbackPlan) { + h.logger.Warn("rollback-confirm rejected: pinned rollback plan contains engine-blocked changes", + "repo", repo, "pr", pr, "database", database, + "database_type", dbType, "environment", environment, + "plan_id", rollbackPlan.PlanIdentifier) + msg := fmt.Sprintf(msgRollbackConfirmBlockedPlan, environment) + if err := h.service.Storage().Locks().Release(ctx, database, dbType, lockOwner); err != nil { + h.logger.Error("rollback-confirm rejected a blocked rollback plan but failed to release the database lock; applies on this database will be blocked until the lock is released manually", + "repo", repo, "pr", pr, "database", database, + "database_type", dbType, "environment", environment, + "lock_owner", lockOwner, "plan_id", rollbackPlan.PlanIdentifier, "error", err) + msg = fmt.Sprintf(msgRollbackConfirmBlockedPlanLockHeld, environment) + } + h.postCommandError(repo, pr, installationID, action.RollbackConfirm, environment, requestedBy, msg) + return + } + + // --defer-cutover only affects engine-driven statements; an all-direct + // rollback plan has no cutover to defer, so reject the flag instead of + // silently ignoring it. Keep the lock: it still pins the rollback plan the + // operator confirmed against, and re-running rollback-confirm without the + // flag executes it. + if result.DeferCutover && storedPlanAllChangesDirect(rollbackPlan) { + h.logger.Info("rollback-confirm rejected: --defer-cutover on an all-direct rollback plan; the pending rollback is preserved", + "repo", repo, "pr", pr, "database", database, + "database_type", dbType, "environment", environment, + "plan_id", rollbackPlan.PlanIdentifier) + h.postCommandError(repo, pr, installationID, action.RollbackConfirm, environment, requestedBy, + fmt.Sprintf(msgDeferCutoverAllDirectRollbackConfirm, environment)) + return + } + // Build apply options — rollback always allows unsafe changes, and is marked // as a rollback so the terminal check update lands action_required (the PR's // change is reverted) even when an operator driver, not this command's @@ -595,3 +660,45 @@ func planHasChanges(plan *storage.Plan) bool { } return false } + +// storedPlanHasBlockedChanges reports whether any stored change carries the +// blocked execution-mode verdict. A blocked change guarantees the apply +// fails, so confirm-time gates reject the plan instead of executing it. +func storedPlanHasBlockedChanges(plan *storage.Plan) bool { + if plan == nil { + return false + } + for _, tc := range plan.FlatDDLChanges() { + if tc.EngineBlocked() { + return true + } + } + return false +} + +// storedPlanAllChangesDirect reports whether every stored change is a +// direct-execution change (and at least one exists). Options that only affect +// engine-driven statements — like a deferred cutover — have nothing to act on +// in such a plan, so their commands are rejected rather than silently +// ignored. A VSchema artifact is an engine-driven change, so its presence +// makes the plan mixed. +func storedPlanAllChangesDirect(plan *storage.Plan) bool { + if plan == nil { + return false + } + changes := plan.FlatDDLChanges() + if len(changes) == 0 { + return false + } + for _, tc := range changes { + if !tc.DirectExecution() { + return false + } + } + for _, nsData := range plan.Namespaces { + if nsData != nil && nsData.Artifacts[vSchemaArtifactName] != "" { + return false + } + } + return true +} diff --git a/pkg/webhook/rollback_direct_integration_test.go b/pkg/webhook/rollback_direct_integration_test.go new file mode 100644 index 000000000..13df013a2 --- /dev/null +++ b/pkg/webhook/rollback_direct_integration_test.go @@ -0,0 +1,329 @@ +//go:build integration + +// Rollback direct-execution consent and gating webhook integration tests. + +package webhook + +import ( + "database/sql" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + "time" + + gh "github.com/google/go-github/v86/github" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/block/schemabot/pkg/api" + ternv1 "github.com/block/schemabot/pkg/proto/ternv1" + "github.com/block/schemabot/pkg/state" + "github.com/block/schemabot/pkg/storage" +) + +// completeDirectPKSwapApply plans and applies the composite-primary-key schema +// against a target holding the single-column key, waits for it to complete, +// and returns the stored apply. With the direct execution policy enabled the +// reshape runs as native DDL, leaving the target with PK (id, tenant_id) and +// original files captured for rollback. +func completeDirectPKSwapApply(t *testing.T, svc *api.Service, dbName string) *storage.Apply { + t.Helper() + ctx := t.Context() + + prNumber := int32(1) + planResp, err := svc.ExecutePlan(ctx, api.PlanRequest{ + Database: dbName, + Environment: "staging", + Type: "mysql", + Repository: "octocat/hello-world", + PullRequest: &prNumber, + SchemaFiles: map[string]*ternv1.SchemaFiles{ + dbName: {Files: map[string]string{"users.sql": pkSwapSchema}}, + }, + }) + require.NoError(t, err) + require.NotEmpty(t, planResp.Changes, "expected the primary-key reshape in the plan") + + applyResp, applyID, err := svc.ExecuteApply(ctx, api.ApplyRequest{ + PlanID: planResp.PlanID, + Environment: "staging", + Options: map[string]string{"allow_unsafe": "true"}, + }) + require.NoError(t, err) + require.True(t, applyResp.Accepted) + + require.Eventually(t, func() bool { + a, err := svc.Storage().Applies().Get(ctx, applyID) + return err == nil && a != nil && state.IsState(a.State, state.Apply.Completed) + }, webhookIntegrationPollDeadline, 500*time.Millisecond, "direct apply should complete") + + require.Equal(t, []string{"id", "tenant_id"}, appPrimaryKeyColumns(t, dbName, "users"), + "the direct apply reshaped the primary key on the target") + + storedApply, err := svc.Storage().Applies().Get(ctx, applyID) + require.NoError(t, err) + require.NotNil(t, storedApply) + return storedApply +} + +// A rollback whose reverse DDL the engine refuses follows the same consent +// model as a direct apply: the rollback plan comment discloses the ⚙️ +// direct-execution routing, and rollback-confirm is the operator's consent +// against that disclosure. A confirm carrying --defer-cutover is rejected (an +// all-direct rollback plan has no cutover to defer) but preserves the pending +// rollback, so a re-run without the flag still executes and restores the +// original primary key on the target. +func TestE2ERollbackDirectPlanDisclosesThenConfirmExecutes(t *testing.T) { + dbName := "webhook_rb_direct" + svc := setupE2EServiceOpts(t, dbName, e2eServiceOpts{ + engineMetadata: map[string]string{ + "direct_execution": "true", + "direct_execution_max_table_rows": "1000000", + }, + }) + seedPKSwapTargetTable(t, dbName) + + storedApply := completeDirectPKSwapApply(t, svc, dbName) + + mux := http.NewServeMux() + server := httptest.NewServer(mux) + t.Cleanup(server.Close) + + client := gh.NewClient(nil) + client.BaseURL, _ = url.Parse(server.URL + "/") + + schemabotConfig := fmt.Sprintf("database: %s\ntype: mysql\n", dbName) + result := setupFakeGitHubForPlan(t, mux, map[string]string{"users.sql": pkSwapSchema}, schemabotConfig, dbName) + + h := newE2EHandler(t, svc, client) + rollbackReq := buildWebhookRequest(t, webhookPayloadOpts{ + comment: fmt.Sprintf("schemabot rollback %s -e staging", storedApply.ApplyIdentifier), + isPR: true, + }, nil) + + rr := httptest.NewRecorder() + h.ServeHTTP(rr, rollbackReq) + require.Equal(t, http.StatusOK, rr.Code) + + select { + case body := <-result.comments: + assert.Contains(t, body, "## Schema Rollback Plan") + assert.Contains(t, body, "⚙️ **Direct execution**", "the rollback plan comment discloses the direct change") + assert.Contains(t, body, "runs as native MySQL DDL") + assert.Contains(t, body, "`users`") + assert.Contains(t, body, "schemabot rollback-confirm -e staging") + case <-time.After(webhookIntegrationPollDeadline): + t.Fatal("timed out waiting for the rollback plan comment") + } + + lock, err := svc.Storage().Locks().Get(t.Context(), dbName, "mysql") + require.NoError(t, err) + require.NotNil(t, lock, "the rollback command pins the plan on the lock for the confirm step") + assert.Equal(t, "octocat/hello-world#1", lock.Owner) + require.True(t, strings.HasPrefix(lock.PendingPlanID, rollbackPendingPlanPrefix)) + + // --defer-cutover has nothing to defer on this all-direct rollback plan, so + // the confirm is rejected — but the rejection must not discard the pending + // rollback: the lock keeps pinning the disclosed plan so a bare re-run of + // rollback-confirm still executes it. + flaggedConfirmReq := buildWebhookRequest(t, webhookPayloadOpts{ + comment: "schemabot rollback-confirm -e staging --defer-cutover", + isPR: true, + }, nil) + + rr = httptest.NewRecorder() + h.ServeHTTP(rr, flaggedConfirmReq) + require.Equal(t, http.StatusOK, rr.Code) + + select { + case body := <-result.comments: + assert.Contains(t, body, "`--defer-cutover` has no effect on this rollback") + assert.Contains(t, body, "The pending rollback is preserved") + assert.Contains(t, body, "schemabot rollback-confirm -e staging") + case <-time.After(webhookIntegrationPollDeadline): + t.Fatal("timed out waiting for the defer-cutover rejection comment") + } + + preserved, err := svc.Storage().Locks().Get(t.Context(), dbName, "mysql") + require.NoError(t, err) + require.NotNil(t, preserved, "the rejected confirm must keep the pending rollback locked") + assert.Equal(t, lock.Owner, preserved.Owner) + assert.Equal(t, lock.PendingPlanID, preserved.PendingPlanID, + "the lock must still pin the rollback plan the operator confirmed against") + assert.Equal(t, []string{"id", "tenant_id"}, appPrimaryKeyColumns(t, dbName, "users"), + "the rejected confirm must not have executed the rollback") + + confirmReq := buildWebhookRequest(t, webhookPayloadOpts{ + comment: "schemabot rollback-confirm -e staging", + isPR: true, + }, nil) + + rr = httptest.NewRecorder() + h.ServeHTTP(rr, confirmReq) + require.Equal(t, http.StatusOK, rr.Code) + + // The direct statement is synchronous and the table is empty, so the + // rollback may terminalize before the first progress poll: scan comments + // until the terminal rollback summary arrives. + gotSummary := false + deadline := time.After(webhookIntegrationPollDeadline) + for !gotSummary { + select { + case body := <-result.comments: + if strings.Contains(body, "Rollback Complete") { + gotSummary = true + assert.Contains(t, body, "Rolled back successfully") + } + case <-deadline: + t.Fatal("timed out waiting for the rollback summary comment") + } + } + + assert.Equal(t, []string{"id"}, appPrimaryKeyColumns(t, dbName, "users"), + "the confirmed direct rollback restored the original primary key on the target") +} + +// A rollback whose reverse DDL the engine refuses and the direct execution +// policy does not route is rejected before a plan is pinned: after a direct +// apply reshaped the primary key of a small table, the table grows past the +// policy's size bound, so the reverse reshape resolves to blocked. The PR gets +// a ⛔ rollback rejection naming the table and the refusal, nothing is pinned +// for confirmation, and the database lock stays free. +func TestE2ERollbackRejectedOnBlockedReversePlan(t *testing.T) { + dbName := "webhook_rb_blocked" + svc := setupE2EServiceOpts(t, dbName, e2eServiceOpts{ + engineMetadata: map[string]string{ + "direct_execution": "true", + "direct_execution_max_table_rows": "10", + }, + }) + seedPKSwapTargetTable(t, dbName) + + storedApply := completeDirectPKSwapApply(t, svc, dbName) + + // Grow the table past the policy bound so the reverse reshape can no + // longer be routed to direct execution. + db, err := sql.Open("mysql", driftDSN(t, dbName)) + require.NoError(t, err) + defer func() { _ = db.Close() }() + values := strings.TrimSuffix(strings.Repeat("(1),", 50), ",") + _, err = db.ExecContext(t.Context(), "INSERT INTO `users` (`tenant_id`) VALUES "+values) + require.NoError(t, err, "seed rows past the direct-execution size bound") + _, err = db.ExecContext(t.Context(), "ANALYZE TABLE `users`") + require.NoError(t, err) + + mux := http.NewServeMux() + server := httptest.NewServer(mux) + t.Cleanup(server.Close) + + client := gh.NewClient(nil) + client.BaseURL, _ = url.Parse(server.URL + "/") + + schemabotConfig := fmt.Sprintf("database: %s\ntype: mysql\n", dbName) + result := setupFakeGitHubForPlan(t, mux, map[string]string{"users.sql": pkSwapSchema}, schemabotConfig, dbName) + + h := newE2EHandler(t, svc, client) + rollbackReq := buildWebhookRequest(t, webhookPayloadOpts{ + comment: fmt.Sprintf("schemabot rollback %s -e staging", storedApply.ApplyIdentifier), + isPR: true, + }, nil) + + rr := httptest.NewRecorder() + h.ServeHTTP(rr, rollbackReq) + require.Equal(t, http.StatusOK, rr.Code) + + select { + case body := <-result.comments: + assert.Contains(t, body, "## Schema Rollback Plan") + assert.Contains(t, body, "⛔ Rollback rejected") + assert.Contains(t, body, "not supported by the schema-change engine") + assert.Contains(t, body, "`users`") + assert.NotContains(t, body, "schemabot rollback-confirm", + "a rejected rollback must not coach a confirmation that can never succeed") + case <-time.After(webhookIntegrationPollDeadline): + t.Fatal("timed out waiting for the rollback rejection comment") + } + + lock, err := svc.Storage().Locks().Get(t.Context(), dbName, "mysql") + require.NoError(t, err) + assert.Nil(t, lock, "a rejected rollback must not leave the database locked") +} + +// A pinned rollback plan carrying engine-blocked changes can never execute, so +// rollback-confirm rejects it instead of starting an apply guaranteed to fail: +// the PR gets a rejection explaining the plan cannot run, and the lock is +// released so re-running the rollback command re-plans through the up-front +// gate. +func TestE2ERollbackConfirmRejectsBlockedPinnedPlan(t *testing.T) { + dbName := "webhook_rb_blocked_pin" + svc := setupE2EService(t, dbName) + ctx := t.Context() + + planID := "plan_rbblockedpin1" + _, err := svc.Storage().Plans().Create(ctx, &storage.Plan{ + PlanIdentifier: planID, + Database: dbName, + DatabaseType: "mysql", + Repository: "octocat/hello-world", + PullRequest: 1, + Environment: "staging", + CreatedAt: time.Now(), + Namespaces: map[string]*storage.NamespacePlanData{ + dbName: { + Tables: []storage.TableChange{{ + Table: "users", + DDL: "ALTER TABLE `users` DROP PRIMARY KEY, ADD PRIMARY KEY (`id`)", + Operation: "alter", + ExecutionMode: "blocked", + ModeReason: "the schema-change engine cannot change a table's PRIMARY KEY", + }}, + }, + }, + }) + require.NoError(t, err) + + require.NoError(t, svc.Storage().Locks().Acquire(ctx, &storage.Lock{ + DatabaseName: dbName, + DatabaseType: "mysql", + Owner: "octocat/hello-world#1", + Repository: "octocat/hello-world", + PullRequest: 1, + PendingPlanID: rollbackPendingPlanID(planID), + })) + + mux := http.NewServeMux() + server := httptest.NewServer(mux) + t.Cleanup(server.Close) + + client := gh.NewClient(nil) + client.BaseURL, _ = url.Parse(server.URL + "/") + + schemabotConfig := fmt.Sprintf("database: %s\ntype: mysql\n", dbName) + result := setupFakeGitHubForPlan(t, mux, map[string]string{"users.sql": pkSwapSchema}, schemabotConfig, dbName) + + h := newE2EHandler(t, svc, client) + confirmReq := buildWebhookRequest(t, webhookPayloadOpts{ + comment: "schemabot rollback-confirm -e staging", + isPR: true, + }, nil) + + rr := httptest.NewRecorder() + h.ServeHTTP(rr, confirmReq) + require.Equal(t, http.StatusOK, rr.Code) + + select { + case body := <-result.comments: + assert.Contains(t, body, "The pinned rollback plan contains changes the schema-change engine does not support") + assert.Contains(t, body, "The lock has been released") + assert.Contains(t, body, "schemabot rollback -e staging") + case <-time.After(webhookIntegrationPollDeadline): + t.Fatal("timed out waiting for the blocked-plan rejection comment") + } + + lock, err := svc.Storage().Locks().Get(ctx, dbName, "mysql") + require.NoError(t, err) + assert.Nil(t, lock, "the rejected confirm must release the lock so a fresh rollback can re-plan") +} diff --git a/pkg/webhook/templates/apply_commands.go b/pkg/webhook/templates/apply_commands.go index 04a352a05..54a9cc141 100644 --- a/pkg/webhook/templates/apply_commands.go +++ b/pkg/webhook/templates/apply_commands.go @@ -157,11 +157,27 @@ func RenderUnsafeChangesBlocked(data PlanCommentData) string { // through, so the comment carries no retry instructions — the guidance is to // rewrite the change or contact the operators. func RenderBlockedChangesApplyRejected(data PlanCommentData) string { + return renderBlockedChangesRejected(data, "Schema Change Plan", "Apply rejected", + "Rewrite these statements as a supported schema change, or contact your SchemaBot operators for help.") +} + +// RenderBlockedChangesRollbackRejected renders the rejection comment for a +// rollback whose reverse plan contains statements the schema-change engine +// refuses. A blocked change guarantees the rollback apply would fail, so the +// rollback is rejected before a plan is pinned for confirmation — there is +// nothing to confirm, and the target must be reconciled another way. +func RenderBlockedChangesRollbackRejected(data PlanCommentData) string { + return renderBlockedChangesRejected(data, "Schema Rollback Plan", "Rollback rejected", + "Reconcile the target schema with a follow-up schema change PR instead, or contact your SchemaBot operators for help.") +} + +// renderBlockedChangesRejected renders the full plan (DDL, summary) so the +// user can see what was planned — but without a lock or confirm footer — then +// the engine-blocked changes that reject the command outright. +func renderBlockedChangesRejected(data PlanCommentData, title, rejection, guidance string) string { var sb strings.Builder - // Render the full plan first (DDL, summary) so the user can see what was - // planned — but without a lock or confirm footer. - writeEnvironmentTitle(&sb, "Schema Change Plan", data.Environment) + writeEnvironmentTitle(&sb, title, data.Environment) writePlanMetadata(&sb, data) writePlanAttribution(&sb, data) @@ -176,7 +192,7 @@ func RenderBlockedChangesApplyRejected(data PlanCommentData) string { sb.WriteString("---\n\n") n := len(data.BlockedChanges) - fmt.Fprintf(&sb, "**⛔ Apply rejected**: **%d** planned %s not supported by the schema-change engine\n", n, pluralize("change", n)) + fmt.Fprintf(&sb, "**⛔ %s**: **%d** planned %s not supported by the schema-change engine\n", rejection, n, pluralize("change", n)) for _, c := range data.BlockedChanges { table := "`" + c.Table + "`" if len(c.Shards) > 0 { @@ -188,7 +204,7 @@ func RenderBlockedChangesApplyRejected(data PlanCommentData) string { fmt.Fprintf(&sb, "- %s\n", table) } } - sb.WriteString("\nRewrite these statements as a supported schema change, or contact your SchemaBot operators for help.\n") + fmt.Fprintf(&sb, "\n%s\n", guidance) return sb.String() } diff --git a/pkg/webhook/templates/direct_test.go b/pkg/webhook/templates/direct_test.go index d2a02a5b4..bd0e84291 100644 --- a/pkg/webhook/templates/direct_test.go +++ b/pkg/webhook/templates/direct_test.go @@ -69,7 +69,6 @@ func TestDirectConsentCopy_KeyedByDatabaseType(t *testing.T) { assert.Equal(t, "native DDL", otherHeader) assert.Contains(t, otherFooter, "each table is unavailable while its statement runs") assert.Contains(t, otherFooter, "**not revertible**") - assert.Contains(t, otherFooter, "Confirming the apply consents to this.") } // A multi-environment plan renders each environment's own direct section, diff --git a/pkg/webhook/templates/plan.go b/pkg/webhook/templates/plan.go index d24d9c318..9b271a8bc 100644 --- a/pkg/webhook/templates/plan.go +++ b/pkg/webhook/templates/plan.go @@ -169,7 +169,7 @@ func RenderPlanComment(data PlanCommentData) string { // operator's consent to their blocking, non-revertible semantics, so the // disclosure must sit on the comment the confirmation acts on. if len(data.DirectChanges) > 0 { - writeDirectChanges(&sb, data.DirectChanges, data.DatabaseType, data.IsMySQL) + writeDirectChanges(&sb, data.DirectChanges, data.DatabaseType, data.IsMySQL, directApplyConsent) } // Unsafe changes warning — shown on the plan comment for review, omitted on @@ -588,32 +588,41 @@ func writeBlockedChanges(sb *strings.Builder, changes []BlockedChangeData) { sb.WriteString("\nAn apply will fail on these statements. Rewrite them as a supported schema change, or contact your SchemaBot operators for help.\n\n") } -// directConsentCopy returns the header noun and consent footer for the -// direct-execution disclosure, keyed by database type. The footer is the -// sentence the operator consents to by confirming the apply, and what a -// direct statement does to the table while it runs is engine-specific — an -// engine that adopts direct execution adds its own copy here rather than -// inheriting another engine's semantics. +// Consent sentences appended to the direct-execution disclosure: the +// engine-specific footer discloses the semantics, and this sentence names the +// command whose confirmation consents to them. +const ( + directApplyConsent = "Confirming the apply consents to this." + directRollbackConsent = "Confirming the rollback consents to this." +) + +// directConsentCopy returns the header noun and semantics footer for the +// direct-execution disclosure, keyed by database type. The footer is what +// the operator consents to by confirming, and what a direct statement does to +// the table while it runs is engine-specific — an engine that adopts direct +// execution adds its own copy here rather than inheriting another engine's +// semantics. func directConsentCopy(databaseType string, isMySQL bool) (headerNoun, footer string) { // Strata is sharded MySQL: a direct statement there is the same native // MySQL DDL, executed per shard. databaseType = strings.TrimSpace(databaseType) if databaseType == storage.DatabaseTypeMySQL || databaseType == storage.DatabaseTypeStrata || isMySQL { return "native MySQL DDL", - "These statements run synchronously outside the schema-change engine: writes to each table are blocked while its statement runs, the change is **not revertible**, and `--defer-cutover` does not apply to it. Confirming the apply consents to this." + "These statements run synchronously outside the schema-change engine: writes to each table are blocked while its statement runs, the change is **not revertible**, and `--defer-cutover` does not apply to it." } // Deliberately conservative fallback for an engine that emits direct // verdicts without registering its own copy above: disclose the broadest // impact rather than understate what the operator is consenting to. return "native DDL", - "These statements run synchronously outside the schema-change engine: each table is unavailable while its statement runs, the change is **not revertible**, and `--defer-cutover` does not apply to it. Confirming the apply consents to this." + "These statements run synchronously outside the schema-change engine: each table is unavailable while its statement runs, the change is **not revertible**, and `--defer-cutover` does not apply to it." } // writeDirectChanges writes the section for statements the direct execution // policy routes to native DDL, naming each table and the planner's reason // (which carries the row estimate). The fixed footer discloses the semantics -// the operator consents to by confirming the apply. -func writeDirectChanges(sb *strings.Builder, changes []DirectChangeData, databaseType string, isMySQL bool) { +// the operator consents to, closed by the consent sentence naming the +// confirming command (apply-confirm or rollback-confirm). +func writeDirectChanges(sb *strings.Builder, changes []DirectChangeData, databaseType string, isMySQL bool, consent string) { headerNoun, footer := directConsentCopy(databaseType, isMySQL) n := len(changes) fmt.Fprintf(sb, "⚙️ **Direct execution**: **%d** %s will run as %s\n", n, pluralize("change", n), headerNoun) @@ -628,7 +637,7 @@ func writeDirectChanges(sb *strings.Builder, changes []DirectChangeData, databas fmt.Fprintf(sb, "- %s\n", table) } } - sb.WriteString("\n" + footer + "\n\n") + sb.WriteString("\n" + footer + " " + consent + "\n\n") } func writeUnsafeWarning(sb *strings.Builder, changes []UnsafeChangeData, isMySQL bool) { @@ -940,7 +949,7 @@ func writeEnvironmentPlanSection(sb *strings.Builder, plan *PlanCommentData) { // Direct-execution changes — each environment's section discloses its own, // since the policy is configured per environment. if len(plan.DirectChanges) > 0 { - writeDirectChanges(sb, plan.DirectChanges, plan.DatabaseType, plan.IsMySQL) + writeDirectChanges(sb, plan.DirectChanges, plan.DatabaseType, plan.IsMySQL, directApplyConsent) } // Unsafe changes warning diff --git a/pkg/webhook/templates/preview.go b/pkg/webhook/templates/preview.go index af38b4267..271157f44 100644 --- a/pkg/webhook/templates/preview.go +++ b/pkg/webhook/templates/preview.go @@ -139,6 +139,55 @@ func PreviewCommentApplyBlockedRejected() string { }) } +// PreviewCommentRollbackPlanDirect renders a sample rollback plan whose +// reverse statement the direct execution policy routes to native MySQL DDL, +// showing the disclosure the operator consents to with rollback-confirm. +func PreviewCommentRollbackPlanDirect() string { + return RenderRollbackPlanComment(PlanCommentData{ + Database: "testapp", + Environment: "staging", + RequestedBy: previewRequestedBy, + DatabaseType: "mysql", + IsMySQL: true, + ApplyID: "apply_a1b2c3d4e5f6", + Changes: []KeyspaceChangeData{ + { + Keyspace: "testapp", + Statements: []string{ + "ALTER TABLE `users` DROP PRIMARY KEY, ADD PRIMARY KEY (`id`)", + }, + }, + }, + DirectChanges: []DirectChangeData{ + {Table: "users", Reason: "dropping primary key is not supported; runs as native MySQL DDL on a table with ~1,240 rows"}, + }, + }) +} + +// PreviewCommentRollbackBlockedRejected renders a sample rollback rejection +// for a reverse plan containing statements the engine refuses. +func PreviewCommentRollbackBlockedRejected() string { + return RenderBlockedChangesRollbackRejected(PlanCommentData{ + Database: "testapp", + Environment: "staging", + RequestedBy: previewRequestedBy, + DatabaseType: "mysql", + IsMySQL: true, + ApplyID: "apply_a1b2c3d4e5f6", + Changes: []KeyspaceChangeData{ + { + Keyspace: "testapp", + Statements: []string{ + "ALTER TABLE `users` DROP PRIMARY KEY, ADD PRIMARY KEY (`id`)", + }, + }, + }, + BlockedChanges: []BlockedChangeData{ + {Table: "users", Reason: "dropping primary key is not supported; direct execution is enabled but the table has ~2,400,000 rows, above the configured limit of 1,000,000"}, + }, + }) +} + // PreviewCommentPlanTenant renders a tenant-targeted plan comment. func PreviewCommentPlanTenant() string { return RenderPlanComment(PlanCommentData{ diff --git a/pkg/webhook/templates/rollback.go b/pkg/webhook/templates/rollback.go index 72e00b3d4..9966a812c 100644 --- a/pkg/webhook/templates/rollback.go +++ b/pkg/webhook/templates/rollback.go @@ -30,6 +30,14 @@ func RenderRollbackPlanComment(data PlanCommentData) string { // Detailed changes writeKeyspaceChanges(&sb, data) + // Direct-execution changes — statements the policy routes to native DDL. + // rollback-confirm is the operator's consent to their blocking, + // non-revertible semantics, so the disclosure must sit on the comment the + // confirmation acts on. + if len(data.DirectChanges) > 0 { + writeDirectChanges(&sb, data.DirectChanges, data.DatabaseType, data.IsMySQL, directRollbackConsent) + } + // Unsafe warning — rollback typically produces DROP operations sb.WriteString("> **Warning**: Rollback may include destructive changes (e.g., DROP INDEX, DROP COLUMN). These will be applied automatically.\n\n") diff --git a/pkg/webhook/templates/rollback_test.go b/pkg/webhook/templates/rollback_test.go index 06810d74e..324db4628 100644 --- a/pkg/webhook/templates/rollback_test.go +++ b/pkg/webhook/templates/rollback_test.go @@ -343,3 +343,51 @@ func TestRollbackTemplates_NoStrayWhitespace(t *testing.T) { "%s body should not start with whitespace", name) } } + +// A rollback plan whose reverse statement routes to direct execution renders +// the same ⚙️ disclosure the apply flow uses, with the consent sentence naming +// rollback confirmation — rollback-confirm consents against this comment. +func TestRenderRollbackPlanComment_DirectDisclosure(t *testing.T) { + rendered := RenderRollbackPlanComment(PlanCommentData{ + Database: "testapp", Environment: "staging", IsMySQL: true, + ApplyID: "apply_a1b2c3d4e5f6", + Changes: []KeyspaceChangeData{{ + Keyspace: "testapp", + Statements: []string{"ALTER TABLE `users` DROP PRIMARY KEY, ADD PRIMARY KEY (`id`)"}, + }}, + DirectChanges: []DirectChangeData{ + {Table: "users", Reason: "dropping primary key is not supported; runs as native MySQL DDL on a table with ~1,240 rows"}, + }, + }) + + assert.Contains(t, rendered, "## Schema Rollback Plan") + assert.Contains(t, rendered, "⚙️ **Direct execution**: **1** change will run as native MySQL DDL") + assert.Contains(t, rendered, "`users`: dropping primary key is not supported; runs as native MySQL DDL on a table with ~1,240 rows") + assert.Contains(t, rendered, "the change is **not revertible**") + assert.Contains(t, rendered, "Confirming the rollback consents to this.") + assert.Contains(t, rendered, "schemabot rollback-confirm -e staging") +} + +// A rollback whose reverse plan contains engine-blocked statements is rejected +// with a comment that shows the reverse plan, names each blocked table and +// reason, and coaches reconciliation instead of a confirmation that can never +// succeed. +func TestRenderBlockedChangesRollbackRejected(t *testing.T) { + rendered := RenderBlockedChangesRollbackRejected(PlanCommentData{ + Database: "testapp", Environment: "staging", IsMySQL: true, + Changes: []KeyspaceChangeData{{ + Keyspace: "testapp", + Statements: []string{"ALTER TABLE `users` DROP PRIMARY KEY, ADD PRIMARY KEY (`id`)"}, + }}, + BlockedChanges: []BlockedChangeData{ + {Table: "users", Reason: "dropping primary key is not supported; direct execution is enabled but the table has ~2,400,000 rows, above the configured limit of 1,000,000"}, + }, + }) + + assert.Contains(t, rendered, "## Schema Rollback Plan") + assert.Contains(t, rendered, "**⛔ Rollback rejected**: **1** planned change not supported by the schema-change engine") + assert.Contains(t, rendered, "`users`: dropping primary key is not supported") + assert.Contains(t, rendered, "Reconcile the target schema with a follow-up schema change PR instead") + assert.NotContains(t, rendered, "schemabot rollback-confirm", + "a rejected rollback must not coach a confirmation that can never succeed") +} From 05a44a14f81060563848988a2fad0f4e5762d32e Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Sat, 1 Aug 2026 09:13:02 -0400 Subject: [PATCH 2/2] fix(github): carry the tenant flag in confirm-time rejection command 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 --- pkg/webhook/apply_execute.go | 2 +- pkg/webhook/plan.go | 14 ----- pkg/webhook/rollback.go | 17 +----- pkg/webhook/templates/apply_commands.go | 13 +++++ pkg/webhook/templates/rollback.go | 33 +++++++++++ pkg/webhook/templates/rollback_tenant_test.go | 56 +++++++++++++++++++ 6 files changed, 106 insertions(+), 29 deletions(-) create mode 100644 pkg/webhook/templates/rollback_tenant_test.go diff --git a/pkg/webhook/apply_execute.go b/pkg/webhook/apply_execute.go index 4c1db7721..403d5b4c7 100644 --- a/pkg/webhook/apply_execute.go +++ b/pkg/webhook/apply_execute.go @@ -155,7 +155,7 @@ func (h *Handler) executeApply( h.logger.Info("apply rejected: --defer-cutover on an all-direct plan; the pending confirmation is preserved", "repo", repo, "pr", pr, "database", database, "environment", environment, "action", actionName) h.postCommandError(repo, pr, installationID, actionName, environment, requestedBy, - fmt.Sprintf(msgDeferCutoverAllDirectConfirm, environment)) + templates.RenderDeferCutoverAllDirectConfirm(environment, h.deploymentTenant())) return } diff --git a/pkg/webhook/plan.go b/pkg/webhook/plan.go index 66631d1c7..1231bbfad 100644 --- a/pkg/webhook/plan.go +++ b/pkg/webhook/plan.go @@ -658,20 +658,6 @@ func shardedUnsafeChanges(shards []*apitypes.ShardPlanResponse) []templates.Unsa // cutover to defer, so the flag is refused instead of silently ignored. const msgDeferCutoverAllDirect = "`--defer-cutover` has no effect on this plan: every change runs directly as native DDL, which has no cutover to defer. Re-run without the flag." -// msgDeferCutoverAllDirectConfirm rejects --defer-cutover at confirm time on -// an all-direct plan. The rejection preserves the pending confirmation — the -// lock still pins the plan the operator confirmed against — so the recovery -// is re-running apply-confirm without the flag, not restarting from apply. -// The format verb takes the environment for the coached command. -const msgDeferCutoverAllDirectConfirm = "`--defer-cutover` has no effect on this plan: every change runs directly as native DDL, which has no cutover to defer. The pending confirmation is preserved — re-run `schemabot apply-confirm -e %s` without the flag." - -// msgDeferCutoverAllDirectRollbackConfirm rejects --defer-cutover at rollback -// confirmation on an all-direct rollback plan. The rejection preserves the -// pending rollback — the lock still pins the plan the operator confirmed -// against — so the recovery is re-running rollback-confirm without the flag. -// The format verb takes the environment for the coached command. -const msgDeferCutoverAllDirectRollbackConfirm = "`--defer-cutover` has no effect on this rollback: every change runs directly as native DDL, which has no cutover to defer. The pending rollback is preserved — re-run `schemabot rollback-confirm -e %s` without the flag." - // shardedDirectChanges collects direct-execution per-shard changes, grouped by // (table, reason) so a change present on several shards lists them together // rather than repeating. Returns nil when the plan carries no per-shard diff --git a/pkg/webhook/rollback.go b/pkg/webhook/rollback.go index de555aa86..1e12ffb40 100644 --- a/pkg/webhook/rollback.go +++ b/pkg/webhook/rollback.go @@ -19,17 +19,6 @@ const ( vSchemaArtifactName = "vschema.json" ) -// msgRollbackConfirmBlockedPlan rejects rollback-confirm when the pinned -// rollback plan carries engine-blocked changes: the rollback apply is -// guaranteed to fail, so there is nothing that can be confirmed. The format -// verb takes the environment for the coached command. -const msgRollbackConfirmBlockedPlan = "The pinned rollback plan contains changes the schema-change engine does not support, so this rollback cannot execute. The lock has been released — re-run `schemabot rollback -e %s` to generate a fresh plan, or contact your SchemaBot operators for help." - -// msgRollbackConfirmBlockedPlanLockHeld is the variant posted when the lock -// release itself failed: the operator must unlock before anything else can -// run on the database. -const msgRollbackConfirmBlockedPlanLockHeld = "The pinned rollback plan contains changes the schema-change engine does not support, so this rollback cannot execute. SchemaBot failed to release the database lock — release it with `schemabot unlock`, then re-run `schemabot rollback -e %s` to generate a fresh plan, or contact your SchemaBot operators for help." - // handleRollbackCommand handles the "schemabot rollback -e " PR comment command. // It looks up the specified apply, generates a rollback plan from its original schema files, // acquires a lock, and posts the plan for confirmation. @@ -406,13 +395,13 @@ func (h *Handler) handleRollbackConfirmCommand(repo string, pr int, environment "repo", repo, "pr", pr, "database", database, "database_type", dbType, "environment", environment, "plan_id", rollbackPlan.PlanIdentifier) - msg := fmt.Sprintf(msgRollbackConfirmBlockedPlan, environment) + msg := templates.RenderRollbackConfirmBlockedPlan(environment, h.deploymentTenant()) if err := h.service.Storage().Locks().Release(ctx, database, dbType, lockOwner); err != nil { h.logger.Error("rollback-confirm rejected a blocked rollback plan but failed to release the database lock; applies on this database will be blocked until the lock is released manually", "repo", repo, "pr", pr, "database", database, "database_type", dbType, "environment", environment, "lock_owner", lockOwner, "plan_id", rollbackPlan.PlanIdentifier, "error", err) - msg = fmt.Sprintf(msgRollbackConfirmBlockedPlanLockHeld, environment) + msg = templates.RenderRollbackConfirmBlockedPlanLockHeld(environment, h.deploymentTenant()) } h.postCommandError(repo, pr, installationID, action.RollbackConfirm, environment, requestedBy, msg) return @@ -429,7 +418,7 @@ func (h *Handler) handleRollbackConfirmCommand(repo string, pr int, environment "database_type", dbType, "environment", environment, "plan_id", rollbackPlan.PlanIdentifier) h.postCommandError(repo, pr, installationID, action.RollbackConfirm, environment, requestedBy, - fmt.Sprintf(msgDeferCutoverAllDirectRollbackConfirm, environment)) + templates.RenderDeferCutoverAllDirectRollbackConfirm(environment, h.deploymentTenant())) return } diff --git a/pkg/webhook/templates/apply_commands.go b/pkg/webhook/templates/apply_commands.go index 54a9cc141..57b05bfa6 100644 --- a/pkg/webhook/templates/apply_commands.go +++ b/pkg/webhook/templates/apply_commands.go @@ -355,6 +355,19 @@ func RenderCannotUnlock(database, environment, applyID, applyState string) strin return sb.String() } +// RenderDeferCutoverAllDirectConfirm rejects --defer-cutover at confirm time +// on a plan whose every change the policy routes to direct execution: a +// direct statement has no cutover to defer, so the flag is refused instead of +// silently ignored. The rejection preserves the pending confirmation — the +// lock still pins the plan the operator confirmed against — so the recovery +// is re-running apply-confirm without the flag, not restarting from apply. +// Tenant is the deployment's own tenant; when set, the coached command +// carries it so pasting the hint addresses this deployment. +func RenderDeferCutoverAllDirectConfirm(environment, tenant string) string { + return fmt.Sprintf("`--defer-cutover` has no effect on this plan: every change runs directly as native DDL, which has no cutover to defer. The pending confirmation is preserved — re-run `%s` without the flag.", + tenantCommand("schemabot apply-confirm", environment, tenant)) +} + // RenderApplyConfirmNoChanges renders a comment when apply-confirm finds no changes. func RenderApplyConfirmNoChanges(database, environment string) string { var sb strings.Builder diff --git a/pkg/webhook/templates/rollback.go b/pkg/webhook/templates/rollback.go index 9966a812c..6a93288bc 100644 --- a/pkg/webhook/templates/rollback.go +++ b/pkg/webhook/templates/rollback.go @@ -103,6 +103,39 @@ func RenderRollbackMissingApplyID(tenant string) string { fmt.Sprintf("or by running `%s`.", appendTenantFlag("schemabot status", tenant))) } +// RenderRollbackConfirmBlockedPlan rejects rollback-confirm when the pinned +// rollback plan carries engine-blocked changes: the rollback apply is +// guaranteed to fail, so there is nothing that can be confirmed. Tenant is +// the deployment's own tenant; when set, the coached command carries it so +// pasting the hint addresses this deployment. +func RenderRollbackConfirmBlockedPlan(environment, tenant string) string { + return fmt.Sprintf("The pinned rollback plan contains changes the schema-change engine does not support, so this rollback cannot execute. The lock has been released — re-run `%s` to generate a fresh plan, or contact your SchemaBot operators for help.", + tenantCommand("schemabot rollback ", environment, tenant)) +} + +// RenderRollbackConfirmBlockedPlanLockHeld is the variant posted when the +// lock release itself failed: the operator must unlock before anything else +// can run on the database. Tenant is the deployment's own tenant; when set, +// the coached commands carry it so pasting a hint addresses this deployment. +func RenderRollbackConfirmBlockedPlanLockHeld(environment, tenant string) string { + return fmt.Sprintf("The pinned rollback plan contains changes the schema-change engine does not support, so this rollback cannot execute. SchemaBot failed to release the database lock — release it with `%s`, then re-run `%s` to generate a fresh plan, or contact your SchemaBot operators for help.", + appendTenantFlag("schemabot unlock", tenant), + tenantCommand("schemabot rollback ", environment, tenant)) +} + +// RenderDeferCutoverAllDirectRollbackConfirm rejects --defer-cutover at +// rollback confirmation on an all-direct rollback plan: a direct statement +// has no cutover to defer, so the flag is refused instead of silently +// ignored. The rejection preserves the pending rollback — the lock still pins +// the plan the operator confirmed against — so the recovery is re-running +// rollback-confirm without the flag. Tenant is the deployment's own tenant; +// when set, the coached command carries it so pasting the hint addresses +// this deployment. +func RenderDeferCutoverAllDirectRollbackConfirm(environment, tenant string) string { + return fmt.Sprintf("`--defer-cutover` has no effect on this rollback: every change runs directly as native DDL, which has no cutover to defer. The pending rollback is preserved — re-run `%s` without the flag.", + tenantCommand("schemabot rollback-confirm", environment, tenant)) +} + // RenderRollbackApplyNotFound renders the message posted when the supplied apply ID // does not match any stored apply. func RenderRollbackApplyNotFound(applyID string) string { diff --git a/pkg/webhook/templates/rollback_tenant_test.go b/pkg/webhook/templates/rollback_tenant_test.go new file mode 100644 index 000000000..345ad8544 --- /dev/null +++ b/pkg/webhook/templates/rollback_tenant_test.go @@ -0,0 +1,56 @@ +package templates + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// Every pasteable command hint in a confirm-time rejection addresses a +// specific deployment, so on a tenant deployment each carries the --tenant +// flag; single-tenant deployments render the same command unchanged. +func TestConfirmRejectionHintsCarryTenant(t *testing.T) { + cases := []struct { + name string + render func(environment, tenant string) string + commands []string + }{ + { + name: "rollback-confirm blocked plan", + render: RenderRollbackConfirmBlockedPlan, + commands: []string{"schemabot rollback -e production"}, + }, + { + name: "rollback-confirm blocked plan, lock held", + render: RenderRollbackConfirmBlockedPlanLockHeld, + commands: []string{ + "schemabot unlock", + "schemabot rollback -e production", + }, + }, + { + name: "defer-cutover on all-direct apply-confirm", + render: RenderDeferCutoverAllDirectConfirm, + commands: []string{"schemabot apply-confirm -e production"}, + }, + { + name: "defer-cutover on all-direct rollback-confirm", + render: RenderDeferCutoverAllDirectRollbackConfirm, + commands: []string{"schemabot rollback-confirm -e production"}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + untenanted := tc.render("production", "") + assert.NotContains(t, untenanted, "--tenant") + for _, cmd := range tc.commands { + assert.Contains(t, untenanted, cmd+"`", "single-tenant hint must be the bare command") + } + + tenanted := tc.render("production", "acme") + for _, cmd := range tc.commands { + assert.Contains(t, tenanted, cmd+" --tenant acme`", "tenant hint must carry the deployment's tenant") + } + }) + } +}