From a24d7a56dade97adb5f4a2f05c573f0991e4c8b1 Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Thu, 17 Sep 2026 13:33:44 -0400 Subject: [PATCH 1/6] feat(github): say how much a rollout's targets agree this round MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An environment whose members are distinct targets plans each one against its own live schema, so the members are free to run different work. The plan comment said only that: "each target holds its own schema, so their plans are not expected to match". True of the contract, and silent about the round in front of the reviewer. Targets free to differ usually do not, and a fleet converging over several PRs — some targets changed, the rest already there — was invisible. The members are now grouped by the plan each would run, and the comment states the result: every target needs the same change, or how many of them are already at this schema, or how many distinct plans the apply would run. Members group on the plan fingerprint, so they share a group exactly when their plans are the same work. Each group carries the plan its own members would run, in the shape the comment already renders the reviewed plan in, so a later change can show it. Grouping is confined to a clean rollup of independent members. A blocked rollup still lists every member on its own, because the operator's next step is the target that could not be planned. Mirrored members stay ungrouped: a clean mirrored rollup has already proved they are one group, and re-reporting that in the vocabulary of a fleet free to diverge would read as an outcome rather than the requirement that let the check pass. Co-Authored-By: Claude Opus 5 --- pkg/webhook/plan_drift.go | 129 +++++++++- pkg/webhook/plan_drift_test.go | 306 +++++++++++++++++++++++ pkg/webhook/templates/plan.go | 94 ++++++- pkg/webhook/templates/plan_drift_test.go | 147 ++++++++++- 4 files changed, 670 insertions(+), 6 deletions(-) diff --git a/pkg/webhook/plan_drift.go b/pkg/webhook/plan_drift.go index eff75f97b..4ba028e68 100644 --- a/pkg/webhook/plan_drift.go +++ b/pkg/webhook/plan_drift.go @@ -3,9 +3,11 @@ package webhook import ( "context" "fmt" + "slices" "strings" "github.com/block/schemabot/pkg/api" + "github.com/block/schemabot/pkg/apitypes" ternv1 "github.com/block/schemabot/pkg/proto/ternv1" "github.com/block/schemabot/pkg/routing" "github.com/block/schemabot/pkg/tern" @@ -106,12 +108,135 @@ func deploymentDriftPreview(rollup api.PlanRollup) *templates.DeploymentDriftDat } entries[i] = entry } - return &templates.DeploymentDriftData{ + independent := rollup.Planning == api.PlanIndependent + data := &templates.DeploymentDriftData{ Deployments: entries, Clean: rollup.Clean, Computed: true, - Independent: rollup.Planning == api.PlanIndependent, + Independent: independent, + } + // Grouping describes the targets that were planned, so it is only meaningful + // once every one of them was. A blocked rollup lists each member on its own + // instead: the operator's next step is the member that could not be planned, + // not the plans of an apply that cannot run. + // + // Mirrored members are left ungrouped because a clean mirrored rollup has + // already proved they are one group. Saying so a second time, in the + // vocabulary of a fleet that may diverge, would suggest the agreement was an + // outcome rather than the requirement that let the check pass. + if rollup.Clean && independent { + data.Plans = deploymentPlanGroups(rollup) + } + return data +} + +// deploymentPlanGroups groups the rollout's members by the plan each would run, +// one entry per distinct plan. +// +// Members are grouped on the plan fingerprint, which two members share exactly +// when their plans are the same work — so a group can be described once and +// attributed to all of its members without comparing every pair. Groups come out +// in the rollout order of their first member, with the primary's group first: +// the reviewed plan is the one an operator has already seen, and a fixed order +// keeps a comment that is re-rendered on a later push from reshuffling under a +// reader who is looking for what changed. +func deploymentPlanGroups(rollup api.PlanRollup) []templates.DeploymentPlanGroup { + names := rollupMemberNames(rollup) + var groups []templates.DeploymentPlanGroup + byPlan := make(map[string]int, len(rollup.Entries)) + for i, e := range rollup.Entries { + at, ok := byPlan[e.PlanFingerprint] + if !ok { + groups = append(groups, templates.DeploymentPlanGroup{ + Primary: i == 0, + Changes: memberPlanChanges(e.ChangeSet), + }) + at = len(groups) - 1 + byPlan[e.PlanFingerprint] = at + } + groups[at].Members = append(groups[at].Members, names[i]) + } + // The primary is the first member, so its group is already first. Ordering is + // stated as a property of the result rather than left to that coincidence, + // which a later change to rollout order would silently break. + slices.SortStableFunc(groups, func(a, b templates.DeploymentPlanGroup) int { + switch { + case a.Primary == b.Primary: + return 0 + case a.Primary: + return -1 + default: + return 1 + } + }) + return groups +} + +// memberPlanChanges renders one member's plan in the shape the comment renders +// the reviewed plan in, so a group's changes are described by the same code that +// describes the plan a reviewer has already read. +// +// A sharded namespace carries its changes twice: once per shard, and once in a +// collapsed namespace view that dedupes tables across shards. Both are kept, the +// same way the reviewed plan keeps them, so the rendering can show what applies +// where rather than a namespace-level view that hides a shard. +// +// A namespace that appears only on shard rows still gets an entry. Dropping it +// would silently remove work from a plan the comment claims to describe in full. +func memberPlanChanges(cs tern.ChangeSet) []templates.KeyspaceChangeData { + shardsByNamespace := make(map[string][]templates.KeyspaceShardChange, len(cs.Shards)) + var shardedNamespaces []string + for _, sp := range cs.Shards { + if sp == nil { + continue + } + shard := templates.KeyspaceShardChange{Shard: sp.GetShard()} + for _, tc := range sp.GetChanges() { + if tc.GetDdl() == "" { + continue + } + shard.Statements = append(shard.Statements, tc.GetDdl()) + } + // A shard with nothing to run already matches the desired schema while + // its siblings change. It is carried as a satisfied group rather than + // dropped, so a partially-applied namespace shows its divergent state. + shard.Satisfied = len(shard.Statements) == 0 + if _, seen := shardsByNamespace[sp.GetNamespace()]; !seen { + shardedNamespaces = append(shardedNamespaces, sp.GetNamespace()) + } + shardsByNamespace[sp.GetNamespace()] = append(shardsByNamespace[sp.GetNamespace()], shard) + } + + changes := make([]templates.KeyspaceChangeData, 0, len(cs.Changes)) + named := make(map[string]bool, len(cs.Changes)) + for _, sc := range cs.Changes { + if sc == nil { + continue + } + named[sc.GetNamespace()] = true + ks := templates.KeyspaceChangeData{ + Keyspace: sc.GetNamespace(), + Shards: shardsByNamespace[sc.GetNamespace()], + } + for _, tc := range sc.GetTableChanges() { + if tc.GetDdl() == "" { + continue + } + ks.Statements = append(ks.Statements, tc.GetDdl()) + } + if sc.GetMetadata()[apitypes.VSchemaChangedMetadataKey] == "true" { + ks.VSchemaChanged = true + ks.VSchemaDiff = sc.GetMetadata()[apitypes.VSchemaDiffMetadataKey] + } + changes = append(changes, ks) + } + for _, ns := range shardedNamespaces { + if named[ns] { + continue + } + changes = append(changes, templates.KeyspaceChangeData{Keyspace: ns, Shards: shardsByNamespace[ns]}) } + return changes } // describeDriftDiff renders a short, count-based summary of how a diverged diff --git a/pkg/webhook/plan_drift_test.go b/pkg/webhook/plan_drift_test.go index d12f082a4..7fbf398be 100644 --- a/pkg/webhook/plan_drift_test.go +++ b/pkg/webhook/plan_drift_test.go @@ -5,11 +5,15 @@ import ( "unicode/utf8" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/block/schemabot/pkg/api" "github.com/block/schemabot/pkg/apitypes" + ternv1 "github.com/block/schemabot/pkg/proto/ternv1" + "github.com/block/schemabot/pkg/schema" "github.com/block/schemabot/pkg/storage" "github.com/block/schemabot/pkg/tern" + "github.com/block/schemabot/pkg/webhook/templates" ) // The drift summary names diverged deployments so the check's Change column @@ -194,3 +198,305 @@ func TestSummarizeReviewDrift_NamesMultiTargetMembers(t *testing.T) { assert.Contains(t, summary, "could not plan: primary/testapp-002, eu-west") assert.NotContains(t, summary, "drift blocks apply") } + +// plannedMember builds a clean, independently-planned rollup member running the +// given DDL. A member with no DDL is already at the desired schema. +func plannedMember(deployment, target string, ddl ...string) api.DeploymentRollupEntry { + cs := tern.ChangeSet{} + if len(ddl) > 0 { + change := &ternv1.SchemaChange{Namespace: "testapp"} + for _, stmt := range ddl { + change.TableChanges = append(change.TableChanges, &ternv1.TableChange{ + TableName: "users", + Ddl: stmt, + ChangeType: ternv1.ChangeType_CHANGE_TYPE_ALTER, + Namespace: "testapp", + }) + } + cs.Changes = []*ternv1.SchemaChange{change} + } + fp, err := tern.ChangeSetFingerprint(schema.DialectMySQL, cs) + if err != nil { + panic(err) + } + return api.DeploymentRollupEntry{ + DatabaseType: "vitess", + Deployment: deployment, + Target: target, + Class: api.DeploymentPlanned, + ChangeSet: cs, + PlanFingerprint: fp, + } +} + +// groupMembers flattens the grouped members for assertions. +func groupMembers(groups []templates.DeploymentPlanGroup) [][]string { + out := make([][]string, len(groups)) + for i, g := range groups { + out[i] = g.Members + } + return out +} + +// Targets running the same work are described once and attributed to all of +// them, so a converged fleet does not repeat one plan per target. +func TestDeploymentPlanGroups_SameWorkGroupsTogether(t *testing.T) { + email := "ALTER TABLE users ADD COLUMN email VARCHAR(255)" + rollup := api.PlanRollup{ + Clean: true, + Planning: api.PlanIndependent, + Entries: []api.DeploymentRollupEntry{ + plannedMember("primary", "testapp_1", email), + plannedMember("primary", "testapp_2", email), + plannedMember("primary", "testapp_3", email), + }, + } + + groups := deploymentPlanGroups(rollup) + assert.Equal(t, [][]string{{"primary/testapp_1", "primary/testapp_2", "primary/testapp_3"}}, groupMembers(groups)) + assert.True(t, groups[0].Primary) + assert.Equal(t, []string{email}, groups[0].Changes[0].Statements) + assert.False(t, groups[0].Empty()) +} + +// Targets that hold their own schemas can need different work. Each distinct +// plan is its own group, so the comment describes every plan the apply would +// run rather than the reviewed one alone. +func TestDeploymentPlanGroups_DifferentWorkSplits(t *testing.T) { + email := "ALTER TABLE users ADD COLUMN email VARCHAR(255)" + phone := "ALTER TABLE users ADD COLUMN phone VARCHAR(32)" + rollup := api.PlanRollup{ + Clean: true, + Planning: api.PlanIndependent, + Entries: []api.DeploymentRollupEntry{ + plannedMember("primary", "testapp_1", email), + plannedMember("primary", "testapp_2", phone, email), + plannedMember("primary", "testapp_3", email), + }, + } + + groups := deploymentPlanGroups(rollup) + assert.Equal(t, [][]string{ + {"primary/testapp_1", "primary/testapp_3"}, + {"primary/testapp_2"}, + }, groupMembers(groups)) + assert.Equal(t, []string{email}, groups[0].Changes[0].Statements) + assert.Equal(t, []string{phone, email}, groups[1].Changes[0].Statements, + "a group carries the plan its own members would run, not the reviewed one") +} + +// Targets already at the desired schema form a group of their own, which the +// comment can name. Folding them into the changing targets would tell an +// operator the apply runs DDL on targets it will not touch. +func TestDeploymentPlanGroups_ConvergedTargetsAreTheirOwnGroup(t *testing.T) { + email := "ALTER TABLE users ADD COLUMN email VARCHAR(255)" + rollup := api.PlanRollup{ + Clean: true, + Planning: api.PlanIndependent, + Entries: []api.DeploymentRollupEntry{ + plannedMember("primary", "testapp_1", email), + plannedMember("primary", "testapp_2"), + plannedMember("primary", "testapp_3", email), + plannedMember("primary", "testapp_4"), + }, + } + + groups := deploymentPlanGroups(rollup) + assert.Equal(t, [][]string{ + {"primary/testapp_1", "primary/testapp_3"}, + {"primary/testapp_2", "primary/testapp_4"}, + }, groupMembers(groups)) + assert.False(t, groups[0].Empty()) + assert.True(t, groups[1].Empty(), "targets with nothing to apply are named, not dropped") +} + +// The primary's group comes first whatever the primary's own plan, because the +// reviewed plan is the one the operator has already read. +func TestDeploymentPlanGroups_PrimaryGroupComesFirst(t *testing.T) { + email := "ALTER TABLE users ADD COLUMN email VARCHAR(255)" + rollup := api.PlanRollup{ + Clean: true, + Planning: api.PlanIndependent, + Entries: []api.DeploymentRollupEntry{ + plannedMember("primary", "testapp_1"), + plannedMember("primary", "testapp_2", email), + plannedMember("primary", "testapp_3", email), + }, + } + + groups := deploymentPlanGroups(rollup) + assert.True(t, groups[0].Primary) + assert.Equal(t, []string{"primary/testapp_1"}, groups[0].Members) + assert.True(t, groups[0].Empty(), "the primary having nothing to apply does not move its group") +} + +// Grouping describes targets that were planned, so a rollup that blocked +// carries none: the operator's next step is the target that could not be +// planned, not the plans of an apply that cannot run. +func TestDeploymentDriftPreview_BlockedRollupIsNotGrouped(t *testing.T) { + email := "ALTER TABLE users ADD COLUMN email VARCHAR(255)" + blocked := plannedMember("primary", "testapp_2") + blocked.Class = api.DeploymentErrored + blocked.PlanFingerprint = "" + blocked.ChangeSet = tern.ChangeSet{} + rollup := api.PlanRollup{ + Clean: false, + Planning: api.PlanIndependent, + Entries: []api.DeploymentRollupEntry{ + plannedMember("primary", "testapp_1", email), + blocked, + }, + } + + preview := deploymentDriftPreview(rollup) + assert.Empty(t, preview.Plans) + assert.Len(t, preview.Deployments, 2) +} + +// Members required to match each other are not grouped: a clean mirrored rollup +// has already proved they are one group, and re-reporting that in the vocabulary +// of a fleet free to diverge would read as an outcome rather than the +// requirement that let the check pass. +func TestDeploymentDriftPreview_MirroredMembersAreNotGrouped(t *testing.T) { + email := "ALTER TABLE users ADD COLUMN email VARCHAR(255)" + eu := plannedMember("eu", "eu", email) + eu.Class = api.DeploymentMatch + au := plannedMember("au", "au", email) + au.Class = api.DeploymentMatch + rollup := api.PlanRollup{ + Clean: true, + Planning: api.PlanMirrored, + Entries: []api.DeploymentRollupEntry{eu, au}, + } + + preview := deploymentDriftPreview(rollup) + assert.Empty(t, preview.Plans) +} + +// A clean independent rollup reaches the comment already grouped, so the +// rendering never has to fall back to describing the contract instead of this +// round's plans. +func TestDeploymentDriftPreview_CleanIndependentRollupCarriesGroups(t *testing.T) { + email := "ALTER TABLE users ADD COLUMN email VARCHAR(255)" + rollup := api.PlanRollup{ + Clean: true, + Planning: api.PlanIndependent, + Entries: []api.DeploymentRollupEntry{ + plannedMember("primary", "testapp_1", email), + plannedMember("primary", "testapp_2"), + }, + } + + preview := deploymentDriftPreview(rollup) + assert.Len(t, preview.Plans, 2) +} + +// A member's plan reaches the comment in the same shape the reviewed plan does, +// so a group's changes render through the code that renders the plan a reviewer +// has already read. +func TestMemberPlanChanges_CarriesNamespaceStatements(t *testing.T) { + cs := tern.ChangeSet{Changes: []*ternv1.SchemaChange{{ + Namespace: "testapp", + TableChanges: []*ternv1.TableChange{ + {TableName: "users", Ddl: "ALTER TABLE users ADD COLUMN email VARCHAR(255)"}, + {TableName: "orders", Ddl: "ALTER TABLE orders ADD COLUMN total BIGINT"}, + }, + }}} + + changes := memberPlanChanges(cs) + assert.Equal(t, []templates.KeyspaceChangeData{{ + Keyspace: "testapp", + Statements: []string{ + "ALTER TABLE users ADD COLUMN email VARCHAR(255)", + "ALTER TABLE orders ADD COLUMN total BIGINT", + }, + }}, changes) +} + +// A sharded namespace keeps both views of its changes, so the comment can show +// what applies to which shard rather than a namespace-level view that hides a +// shard. +func TestMemberPlanChanges_KeepsPerShardChanges(t *testing.T) { + email := "ALTER TABLE users ADD COLUMN email VARCHAR(255)" + cs := tern.ChangeSet{ + Changes: []*ternv1.SchemaChange{{ + Namespace: "testapp", + TableChanges: []*ternv1.TableChange{{TableName: "users", Ddl: email}}, + }}, + Shards: []*ternv1.ShardPlan{ + {Namespace: "testapp", Shard: "-80", Changes: []*ternv1.TableChange{{TableName: "users", Ddl: email}}}, + {Namespace: "testapp", Shard: "80-", Changes: []*ternv1.TableChange{{TableName: "users", Ddl: email}}}, + }, + } + + changes := memberPlanChanges(cs) + require.Len(t, changes, 1) + assert.Equal(t, []string{email}, changes[0].Statements) + assert.Equal(t, []templates.KeyspaceShardChange{ + {Shard: "-80", Statements: []string{email}}, + {Shard: "80-", Statements: []string{email}}, + }, changes[0].Shards) +} + +// A shard already at the desired schema while its siblings change is carried as +// satisfied rather than dropped, so a partially-applied namespace shows its +// divergent state instead of looking uniform. +func TestMemberPlanChanges_MarksSatisfiedShards(t *testing.T) { + email := "ALTER TABLE users ADD COLUMN email VARCHAR(255)" + cs := tern.ChangeSet{ + Changes: []*ternv1.SchemaChange{{ + Namespace: "testapp", + TableChanges: []*ternv1.TableChange{{TableName: "users", Ddl: email}}, + }}, + Shards: []*ternv1.ShardPlan{ + {Namespace: "testapp", Shard: "-80", Changes: []*ternv1.TableChange{{TableName: "users", Ddl: email}}}, + {Namespace: "testapp", Shard: "80-"}, + }, + } + + changes := memberPlanChanges(cs) + require.Len(t, changes[0].Shards, 2) + assert.False(t, changes[0].Shards[0].Satisfied) + assert.True(t, changes[0].Shards[1].Satisfied) +} + +// A namespace carried only by shard rows still reaches the comment. Dropping it +// would remove work from a plan the comment claims to describe in full. +func TestMemberPlanChanges_KeepsShardOnlyNamespace(t *testing.T) { + email := "ALTER TABLE users ADD COLUMN email VARCHAR(255)" + cs := tern.ChangeSet{Shards: []*ternv1.ShardPlan{ + {Namespace: "testapp", Shard: "-80", Changes: []*ternv1.TableChange{{TableName: "users", Ddl: email}}}, + }} + + changes := memberPlanChanges(cs) + require.Len(t, changes, 1) + assert.Equal(t, "testapp", changes[0].Keyspace) + assert.Equal(t, []string{email}, changes[0].Shards[0].Statements) +} + +// A vschema rewrite carries no table DDL. It reaches the comment as a change the +// namespace needs, so a plan that only rewrites the vschema is not mistaken for +// a namespace with nothing to apply. +func TestMemberPlanChanges_CarriesVSchemaChange(t *testing.T) { + cs := tern.ChangeSet{Changes: []*ternv1.SchemaChange{{ + Namespace: "testapp", + Metadata: map[string]string{ + apitypes.VSchemaChangedMetadataKey: "true", + apitypes.VSchemaDiffMetadataKey: "+ table users", + }, + }}} + + changes := memberPlanChanges(cs) + require.Len(t, changes, 1) + assert.True(t, changes[0].VSchemaChanged) + assert.Equal(t, "+ table users", changes[0].VSchemaDiff) + assert.Empty(t, changes[0].Statements) + assert.False(t, templates.DeploymentPlanGroup{Changes: changes}.Empty()) +} + +// A member already at the desired schema produces no changes at all, which is +// the group the comment names as having nothing to apply. +func TestMemberPlanChanges_EmptyPlanHasNoChanges(t *testing.T) { + assert.Empty(t, memberPlanChanges(tern.ChangeSet{})) + assert.True(t, templates.DeploymentPlanGroup{}.Empty()) +} diff --git a/pkg/webhook/templates/plan.go b/pkg/webhook/templates/plan.go index e8a76ee0f..1789e1ed3 100644 --- a/pkg/webhook/templates/plan.go +++ b/pkg/webhook/templates/plan.go @@ -225,6 +225,42 @@ type DeploymentDriftData struct { // rollup means every target was planned rather than that they agree — which // is the opposite of what the mirrored wording says. Independent bool + // Plans is the members grouped by the plan they would run, one entry per + // distinct plan, the primary's first. It says how much the members actually + // agree this round, which the contract alone cannot: members that are free + // to differ usually do not. Set only for a clean rollup of independent + // members — members expected to match each other say nothing by matching, + // and a blocked rollup describes each member on its own instead. + Plans []DeploymentPlanGroup +} + +// DeploymentPlanGroup is the members of a rollout that would run the same plan. +// Members share a group exactly when their plans are identical work, so a group +// is what the comment can describe once and attribute to all of them. +type DeploymentPlanGroup struct { + // Members names the group's members the way an operator addresses them, in + // rollout order. + Members []string + // Primary marks the group the reviewed primary member belongs to. Exactly + // one group carries it, and it is the group operators read first: the + // reviewed plan is the one they have already seen. + Primary bool + // Changes is the plan every member of the group would run, in the same shape + // the comment renders the reviewed plan itself. Empty for a group whose + // members are already at the desired schema. + Changes []KeyspaceChangeData +} + +// Empty reports that the group's members are already at the desired schema and +// would apply nothing. That is a plan in its own right, not a missing one, and +// naming it is the difference between a fleet that is converging and one the +// comment has quietly left out. +// A vschema rewrite carries no DDL and is still work, so a group is counted the +// same way the comment counts the reviewed plan: statements and vschema +// rewrites together. +func (g DeploymentPlanGroup) Empty() bool { + statements, vschema := countChanges(g.Changes) + return statements+vschema == 0 } // DeploymentDriftEntry is one rollout member's classification against the @@ -1208,9 +1244,10 @@ func writeDeploymentDrift(sb *strings.Builder, drift *DeploymentDriftData) { case drift.Clean && drift.Independent: // Independent members were deliberately never compared to each other, so // the mirrored headline would assert agreement the rollup did not check. - // It says what was actually established: every target has a plan. - fmt.Fprintf(sb, "✅ **Planned separately for all %d targets** (%s) — each target holds its own schema, so their plans are not expected to match.\n\n", - len(drift.Deployments), strings.Join(names, ", ")) + // It says what was actually established: every target has a plan, and how + // far those plans agree this round. + fmt.Fprintf(sb, "✅ **Planned separately for all %d targets** (%s) — %s\n\n", + len(drift.Deployments), strings.Join(names, ", "), describePlanGroups(drift.Plans)) case drift.Clean: fmt.Fprintf(sb, "✅ **Same plan on all %d deployments** (%s).\n\n", len(drift.Deployments), strings.Join(names, ", ")) @@ -1273,6 +1310,57 @@ func blockedSuffix(blocked int) string { return fmt.Sprintf(" · blocked: %d", blocked) } +// describePlanGroups states how much the members' plans actually agree this +// round: how many distinct plans there are, and how many members already hold +// the desired schema and would apply nothing. +// +// The contract alone cannot say this. Targets that are free to differ usually do +// not, and a fleet converging over time — some targets changed, the rest already +// there — is otherwise invisible in a comment that only reports what members are +// permitted to do. +// +// With no groups it falls back to the contract, which is all that is known: a +// caller that did not group the members has established nothing about this round +// beyond what the configuration already said. +func describePlanGroups(groups []DeploymentPlanGroup) string { + if len(groups) == 0 { + return "each target holds its own schema, so their plans are not expected to match." + } + var plans, changing, converged int + for _, g := range groups { + if g.Empty() { + converged += len(g.Members) + continue + } + plans++ + changing += len(g.Members) + } + + switch { + case plans == 0: + return "every target is already at this schema." + case plans == 1 && converged == 0: + return "every target needs the same change." + case plans == 1: + return fmt.Sprintf("%s this change, %s already at this schema.", + countedVerb(changing, "needs", "need"), countedVerb(converged, "is", "are")) + case converged == 0: + return fmt.Sprintf("%d distinct plans. Each target applies its own.", plans) + default: + return fmt.Sprintf("%d distinct plans across the %d targets that change; %s already at this schema.", + plans, changing, countedVerb(converged, "is", "are")) + } +} + +// countedVerb renders a count and the verb that agrees with it, e.g. "3 need" or +// "1 needs". +func countedVerb(n int, singular, plural string) string { + if n == 1 { + return "1 " + singular + } + return fmt.Sprintf("%d %s", n, plural) +} + // driftDetailSuffix renders a deployment's drift detail as a trailing clause, or // an empty string when there is no detail. func driftDetailSuffix(detail string) string { diff --git a/pkg/webhook/templates/plan_drift_test.go b/pkg/webhook/templates/plan_drift_test.go index 27debb25c..6b591b140 100644 --- a/pkg/webhook/templates/plan_drift_test.go +++ b/pkg/webhook/templates/plan_drift_test.go @@ -1,6 +1,7 @@ package templates import ( + "fmt" "strings" "testing" @@ -346,13 +347,144 @@ func TestRenderPlanComment_DriftCleanNamesMultiTargetMembers(t *testing.T) { {Deployment: "primary", Target: "testapp-002", Class: "planned"}, {Deployment: "eu-west", Target: "orders-eu", Class: "planned"}, }, + Plans: []DeploymentPlanGroup{{ + Members: []string{"primary/testapp-001", "primary/testapp-002", "eu-west"}, + Primary: true, + Changes: planGroupChanges(1), + }}, }, } out := RenderPlanComment(data) assert.Contains(t, out, "Planned separately for all 3 targets") assert.Contains(t, out, "`primary/testapp-001`, `primary/testapp-002`, `eu-west`") - assert.True(t, strings.Contains(out, "each target holds its own schema")) + assert.True(t, strings.Contains(out, "every target needs the same change")) +} + +// Targets are free to hold different schemas, so what an operator needs to know +// is how much they agree this round. The comment says how many distinct plans +// the apply would run and how many targets are already there, which the contract +// alone cannot tell them. +func TestRenderPlanComment_PlanGroupsDescribeThisRound(t *testing.T) { + render := func(plans []DeploymentPlanGroup) string { + members := make([]DeploymentDriftEntry, 0, 5) + for _, g := range plans { + for range g.Members { + members = append(members, DeploymentDriftEntry{Deployment: "primary", Class: "planned"}) + } + } + members[0].Primary = true + return RenderPlanComment(PlanCommentData{ + Database: "testapp", Environment: "production", IsMySQL: true, + Changes: []KeyspaceChangeData{{ + Keyspace: "testapp", + Statements: []string{"ALTER TABLE `users` ADD COLUMN `email` varchar(255)"}, + }}, + DeploymentDrift: &DeploymentDriftData{ + Computed: true, Clean: true, Independent: true, + Deployments: members, + Plans: plans, + }, + }) + } + group := func(statements int, members ...string) DeploymentPlanGroup { + return DeploymentPlanGroup{Members: members, Changes: planGroupChanges(statements)} + } + + cases := []struct { + name string + plans []DeploymentPlanGroup + expect string + }{ + { + name: "every target needs the same change", + plans: []DeploymentPlanGroup{group(1, "a", "b", "c")}, + expect: "every target needs the same change.", + }, + { + name: "some targets are already there", + plans: []DeploymentPlanGroup{group(1, "a", "c", "d"), group(0, "b", "e")}, + expect: "3 need this change, 2 are already at this schema.", + }, + { + name: "a single target still needs it", + plans: []DeploymentPlanGroup{group(1, "a"), group(0, "b")}, + expect: "1 needs this change, 1 is already at this schema.", + }, + { + name: "targets need different changes", + plans: []DeploymentPlanGroup{group(1, "a", "b", "c"), group(2, "d", "e")}, + expect: "2 distinct plans. Each target applies its own.", + }, + { + name: "different changes with some already there", + plans: []DeploymentPlanGroup{group(1, "a", "b"), group(2, "c"), group(0, "d", "e")}, + expect: "2 distinct plans across the 3 targets that change; 2 are already at this schema.", + }, + { + name: "the whole fleet is already there", + plans: []DeploymentPlanGroup{group(0, "a", "b", "c")}, + expect: "every target is already at this schema.", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert.Contains(t, render(tc.plans), tc.expect) + }) + } +} + +// A plan that only rewrites the vschema runs no DDL, and is still work. It is +// described as a change the targets need rather than as a schema they already +// hold, which would tell an operator the apply does nothing. +func TestRenderPlanComment_VSchemaOnlyPlanIsNotAlreadyApplied(t *testing.T) { + data := PlanCommentData{ + Database: "testapp", Environment: "production", + Changes: []KeyspaceChangeData{{Keyspace: "testapp", VSchemaChanged: true}}, + DeploymentDrift: &DeploymentDriftData{ + Computed: true, Clean: true, Independent: true, + Deployments: []DeploymentDriftEntry{ + {Deployment: "primary", Target: "testapp_1", Primary: true, Class: "planned"}, + {Deployment: "primary", Target: "testapp_2", Class: "planned"}, + }, + Plans: []DeploymentPlanGroup{ + { + Members: []string{"primary/testapp_1"}, + Primary: true, + Changes: []KeyspaceChangeData{{Keyspace: "testapp", VSchemaChanged: true}}, + }, + {Members: []string{"primary/testapp_2"}}, + }, + }, + } + + out := RenderPlanComment(data) + assert.Contains(t, out, "1 needs this change, 1 is already at this schema.") +} + +// A rollup that reaches the comment ungrouped states the contract and nothing +// more. Claiming the targets agree — or that they do not — would be a claim +// about plans nobody compared. +func TestRenderPlanComment_UngroupedIndependentRollupStatesTheContract(t *testing.T) { + data := PlanCommentData{ + Database: "testapp", Environment: "production", IsMySQL: true, + Changes: []KeyspaceChangeData{{ + Keyspace: "testapp", + Statements: []string{"ALTER TABLE `users` ADD COLUMN `email` varchar(255)"}, + }}, + DeploymentDrift: &DeploymentDriftData{ + Computed: true, Clean: true, Independent: true, + Deployments: []DeploymentDriftEntry{ + {Deployment: "primary", Target: "testapp_1", Primary: true, Class: "planned"}, + {Deployment: "primary", Target: "testapp_2", Class: "planned"}, + }, + }, + } + + out := RenderPlanComment(data) + assert.Contains(t, out, "each target holds its own schema, so their plans are not expected to match.") + assert.NotContains(t, out, "distinct plans") } // A member name reaches the comment from server config, so the rollup renders @@ -380,3 +512,16 @@ func TestRenderPlanComment_DriftContainsHostileMemberNames(t *testing.T) { assert.NotContains(t, out, "\n## Injected", "a name must not start a heading of its own") assert.Contains(t, out, "`` us` ## Injected ``") } + +// planGroupChanges builds a group plan running the given number of statements. +// A group running none is already at the desired schema. +func planGroupChanges(statements int) []KeyspaceChangeData { + if statements == 0 { + return nil + } + ks := KeyspaceChangeData{Keyspace: "testapp"} + for i := range statements { + ks.Statements = append(ks.Statements, fmt.Sprintf("ALTER TABLE `t%d` ADD COLUMN `c` int", i)) + } + return []KeyspaceChangeData{ks} +} From 1e139d5a3114c153450095700c60328c5d68d459 Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Wed, 23 Sep 2026 22:24:47 -0400 Subject: [PATCH 2/6] fix(github): stop a converged reviewed target from headlining as a no-op An independent rollout plans each target against its own live schema, so the reviewed primary being at the desired schema establishes nothing about the rest. The comment headlined that empty reviewed plan with "No schema changes detected" while the line above it said two other targets still needed the change, and the DDL those targets would run appeared nowhere. A reviewer reading the green line merges believing an apply does nothing. Where the rollup shows a target whose own plan runs work, the headline now says so instead. A rollout whose every target is already at the schema still reads as the no-op it is. Also addresses review precision notes: a group's Changes is one member's rendering rather than a spelling every member produces; the empty fingerprint's "do not group" meaning is safe only behind the caller's clean gate; memberPlanChanges is a second builder of the reviewed plan's shape rather than the same one, and a shard that reported changes without DDL cannot reach it as satisfied. Co-Authored-By: Claude Opus 5 --- pkg/webhook/plan_drift.go | 29 ++++++++++-- pkg/webhook/templates/plan.go | 53 +++++++++++++++++++-- pkg/webhook/templates/plan_drift_test.go | 59 ++++++++++++++++++++++++ 3 files changed, 131 insertions(+), 10 deletions(-) diff --git a/pkg/webhook/plan_drift.go b/pkg/webhook/plan_drift.go index 4ba028e68..64f7cb7bf 100644 --- a/pkg/webhook/plan_drift.go +++ b/pkg/webhook/plan_drift.go @@ -143,6 +143,12 @@ func deploymentDriftPreview(rollup api.PlanRollup) *templates.DeploymentDriftDat func deploymentPlanGroups(rollup api.PlanRollup) []templates.DeploymentPlanGroup { names := rollupMemberNames(rollup) var groups []templates.DeploymentPlanGroup + // A member that could not be keyed carries an empty fingerprint, which means + // "do not group" and would collapse every such member into one plan here. Only + // a clean rollup reaches this function, and a clean rollup has no unkeyed + // member, so the empty key is never a key. The caller's gate is what + // establishes that, in a package of its own — a later caller that groups a + // rollup that did not pass has to key the members itself. byPlan := make(map[string]int, len(rollup.Entries)) for i, e := range rollup.Entries { at, ok := byPlan[e.PlanFingerprint] @@ -172,17 +178,22 @@ func deploymentPlanGroups(rollup api.PlanRollup) []templates.DeploymentPlanGroup return groups } -// memberPlanChanges renders one member's plan in the shape the comment renders -// the reviewed plan in, so a group's changes are described by the same code that -// describes the plan a reviewer has already read. +// memberPlanChanges renders one member's plan into the shape the comment renders +// the reviewed plan in, so a group's changes can be shown the way a reviewer has +// already read the primary's. It is a second builder of that shape, not the same +// one: the reviewed plan is built from the plan response in buildPlanCommentData, +// and the two have to be kept in step by hand. // // A sharded namespace carries its changes twice: once per shard, and once in a // collapsed namespace view that dedupes tables across shards. Both are kept, the // same way the reviewed plan keeps them, so the rendering can show what applies // where rather than a namespace-level view that hides a shard. // -// A namespace that appears only on shard rows still gets an entry. Dropping it -// would silently remove work from a plan the comment claims to describe in full. +// A namespace that appears only on shard rows still gets an entry. The planner +// opens a namespace's collapsed entry and its shard rows in the same step, so it +// does not produce one — the entry exists because dropping a namespace would +// silently remove work from a plan the comment claims to describe in full, and a +// renderer should not be the thing that decides a shape is impossible. func memberPlanChanges(cs tern.ChangeSet) []templates.KeyspaceChangeData { shardsByNamespace := make(map[string][]templates.KeyspaceShardChange, len(cs.Shards)) var shardedNamespaces []string @@ -200,6 +211,14 @@ func memberPlanChanges(cs tern.ChangeSet) []templates.KeyspaceChangeData { // A shard with nothing to run already matches the desired schema while // its siblings change. It is carried as a satisfied group rather than // dropped, so a partially-applied namespace shows its divergent state. + // + // A shard that reported changes and produced no DDL is a different thing: + // an incomplete plan, which the reviewed plan refuses to render rather + // than call satisfied. Calling it satisfied here would say the inverse — + // already at this schema — so it is worth naming why it cannot arrive. + // canonicalDDLForDrift rejects a blank statement, so a member carrying + // one fails its own comparison, classifies errored, and is excluded from + // the clean rollup this grouping runs on. shard.Satisfied = len(shard.Statements) == 0 if _, seen := shardsByNamespace[sp.GetNamespace()]; !seen { shardedNamespaces = append(shardedNamespaces, sp.GetNamespace()) diff --git a/pkg/webhook/templates/plan.go b/pkg/webhook/templates/plan.go index 1789e1ed3..96d7cb58b 100644 --- a/pkg/webhook/templates/plan.go +++ b/pkg/webhook/templates/plan.go @@ -245,9 +245,15 @@ type DeploymentPlanGroup struct { // one group carries it, and it is the group operators read first: the // reviewed plan is the one they have already seen. Primary bool - // Changes is the plan every member of the group would run, in the same shape - // the comment renders the reviewed plan itself. Empty for a group whose - // members are already at the desired schema. + // Changes is one member's plan, in the same shape the comment renders the + // reviewed plan itself. Empty for a group whose members are already at the + // desired schema. + // + // The group's members run the same work, so any member's plan describes all + // of them — but they are grouped on canonicalized DDL, so two members can + // legitimately share a group while spelling the same statement differently. + // What renders is whichever member came first in rollout order, not a + // spelling every member would produce. Changes []KeyspaceChangeData } @@ -881,8 +887,45 @@ func writeMultiEnvIgnoredNamespaces(sb *strings.Builder, data MultiEnvPlanCommen } } +// noChangesHeadline states what an empty reviewed plan means for the rollout an +// apply would run on. +// +// On its own an empty plan reads as a no-op, and that is only established for +// the target that was reviewed. An independent rollout plans each target +// against its own live schema, so a primary already at the desired schema says +// nothing about the rest: the apply can still run DDL on every other target. A +// reviewer who takes the green line at face value merges believing nothing will +// happen, so where some target still needs the change the headline says so. +func noChangesHeadline(drift *DeploymentDriftData) string { + changing := changingTargetCount(drift) + if changing == 0 { + return "✅ **No schema changes detected**" + } + return fmt.Sprintf("%s **No schema changes for the reviewed target** — %s this change, so an apply would not be a no-op.", + glyph.Attention, countedVerb(changing, "target still needs", "targets still need")) +} + +// changingTargetCount counts the rollout's members whose own plan runs work. +// +// It answers only for a grouped rollup, which is a clean independent one: a +// mirrored rollup that passed has already established that every member matches +// the reviewed plan, so an empty reviewed plan is empty everywhere, and a rollup +// that did not pass carries no per-member plans to count. +func changingTargetCount(drift *DeploymentDriftData) int { + if drift == nil { + return 0 + } + var changing int + for _, g := range drift.Plans { + if !g.Empty() { + changing += len(g.Members) + } + } + return changing +} + func writeNoChangesDetected(sb *strings.Builder, data PlanCommentData) { - sb.WriteString("✅ **No schema changes detected**\n") + sb.WriteString(noChangesHeadline(data.DeploymentDrift) + "\n") if data.RecoveredApplyOwnedCheckState { sb.WriteString("\n" + glyph.Info + " SchemaBot found stored PR check state for this database/environment that was still marked as an apply in progress. Because this fresh plan shows the target schema already matches this PR, SchemaBot updated the PR check to passing.\n") } @@ -1981,7 +2024,7 @@ func writeEnvironmentPlanSection(sb *strings.Builder, plan *PlanCommentData, bud // summary (writePlanSummary) or no-changes message, because entries can // resolve differently per environment. if totalChanges == 0 { - sb.WriteString("✅ **No schema changes detected**\n\n") + sb.WriteString(noChangesHeadline(plan.DeploymentDrift) + "\n\n") writeIgnoredNamespaces(sb, plan.IgnoredNamespaces) writeExemptTables(sb, plan.ExemptTables) return diff --git a/pkg/webhook/templates/plan_drift_test.go b/pkg/webhook/templates/plan_drift_test.go index 6b591b140..719f0e14a 100644 --- a/pkg/webhook/templates/plan_drift_test.go +++ b/pkg/webhook/templates/plan_drift_test.go @@ -513,6 +513,65 @@ func TestRenderPlanComment_DriftContainsHostileMemberNames(t *testing.T) { assert.Contains(t, out, "`` us` ## Injected ``") } +// A rollout whose reviewed target is already at the desired schema, while other +// targets are not, must not headline as a no-op. The reviewed plan is empty, so +// the comment shows no DDL; a reviewer who reads "no schema changes detected" +// merges believing an apply does nothing, when it would run the change on every +// target that has not had it yet. +func TestRenderPlanComment_ConvergedPrimaryDoesNotHeadlineAsNoOp(t *testing.T) { + alter := []KeyspaceChangeData{{ + Keyspace: "testapp", + Statements: []string{"ALTER TABLE `users` ADD COLUMN `email` varchar(255)"}, + }} + data := PlanCommentData{ + Database: "testapp", Environment: "production", IsMySQL: true, + DeploymentDrift: &DeploymentDriftData{ + Computed: true, Clean: true, Independent: true, + Deployments: []DeploymentDriftEntry{ + {Deployment: "primary", Target: "testapp_1", Primary: true, Class: "planned"}, + {Deployment: "primary", Target: "testapp_2", Class: "planned"}, + {Deployment: "primary", Target: "testapp_3", Class: "planned"}, + }, + Plans: []DeploymentPlanGroup{ + {Members: []string{"primary/testapp_1"}, Primary: true}, + {Members: []string{"primary/testapp_2", "primary/testapp_3"}, Changes: alter}, + }, + }, + } + + out := RenderPlanComment(data) + assert.NotContains(t, out, "✅ **No schema changes detected**") + assert.Contains(t, out, "⚠️ **No schema changes for the reviewed target** — 2 targets still need this change, so an apply would not be a no-op.") + + // The same shape with a single other target agrees with itself on number. + data.DeploymentDrift.Deployments = data.DeploymentDrift.Deployments[:2] + data.DeploymentDrift.Plans[1].Members = []string{"primary/testapp_2"} + assert.Contains(t, RenderPlanComment(data), "⚠️ **No schema changes for the reviewed target** — 1 target still needs this change, so an apply would not be a no-op.") +} + +// A rollout where every target is already at the desired schema is a no-op, and +// still says so: the headline above is reserved for the targets that would run +// work, not for every empty plan that has a rollup beside it. +func TestRenderPlanComment_FullyConvergedRolloutIsStillANoOp(t *testing.T) { + data := PlanCommentData{ + Database: "testapp", Environment: "production", IsMySQL: true, + DeploymentDrift: &DeploymentDriftData{ + Computed: true, Clean: true, Independent: true, + Deployments: []DeploymentDriftEntry{ + {Deployment: "primary", Target: "testapp_1", Primary: true, Class: "planned"}, + {Deployment: "primary", Target: "testapp_2", Class: "planned"}, + }, + Plans: []DeploymentPlanGroup{ + {Members: []string{"primary/testapp_1", "primary/testapp_2"}, Primary: true}, + }, + }, + } + + out := RenderPlanComment(data) + assert.Contains(t, out, "✅ **No schema changes detected**") + assert.Contains(t, out, "every target is already at this schema.") +} + // planGroupChanges builds a group plan running the given number of statements. // A group running none is already at the desired schema. func planGroupChanges(statements int) []KeyspaceChangeData { From 5d782c8b2993d37f47afcb45240e84b62058fe0e Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Wed, 23 Sep 2026 23:59:16 -0400 Subject: [PATCH 3/6] fix(github): read a namespace's VSchema work through one predicate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A plan change annotates VSchema work two ways: a rendered diff, or a flag when the work is known without one. Three places had to decide whether a namespace changes its VSchema, and they did not agree — the rollout's member renderer tested only the flag, so a member whose plan carried a rendered diff alone rendered as already at this schema. Fixing the renderer alone would have desynchronized it from the grouping key, which reads the other annotation too: the member would render non-empty while still being grouped with members that have nothing to run, making the group's rendering depend on rollout order. So the decision moves to one predicate, apitypes.HasVSchemaWork, that the renderer, the drift comparison, and the response accessor all read. Also adds the two rollout plan previews to TEMPLATES.md, so the grouped rollup and the converged-primary headline are visible in generated docs without checking out the branch. Co-Authored-By: Claude Opus 5 --- TEMPLATES.md | 65 +++++++++++++++++++ pkg/apitypes/vschema.go | 14 +++- pkg/cmd/internal/templates/preview_comment.go | 4 ++ pkg/tern/change_set_compare.go | 3 +- pkg/webhook/plan_drift.go | 2 +- pkg/webhook/plan_drift_test.go | 25 +++++++ pkg/webhook/templates/preview.go | 61 +++++++++++++++++ 7 files changed, 171 insertions(+), 3 deletions(-) diff --git a/TEMPLATES.md b/TEMPLATES.md index 6b5f612a7..5354b2823 100644 --- a/TEMPLATES.md +++ b/TEMPLATES.md @@ -1527,6 +1527,71 @@ schemabot apply -e production +
+Rollout Plans (Converging) + + +## Schema Change Plan — Production + +**Database**: `testapp` | **Type**: `MySQL` | **Schema Name**: `testapp` + +*Requested by @jackjackbits at 2026-01-01 00:00:00 UTC · planned from [`abcdef1`](https://github.com/block/schemabot/commit/abcdef1234567890abcdef1234567890abcdef12)* + +✅ **Planned separately for all 3 targets** (`primary/testapp_1`, `primary/testapp_2`, `primary/testapp_3`) — 2 need this change, 1 is already at this schema. + +```sql +CREATE TABLE `users` ( + `id` bigint unsigned NOT NULL AUTO_INCREMENT, + `email` varchar(255) NOT NULL, + `created_at` timestamp DEFAULT current_timestamp(), + PRIMARY KEY(`id`), + INDEX `idx_email`(`email`) +) ENGINE InnoDB, + CHARSET utf8mb4, + COLLATE utf8mb4_0900_ai_ci; + +CREATE TABLE `orders` ( + `id` bigint unsigned NOT NULL AUTO_INCREMENT, + `user_id` bigint NOT NULL, + `total_cents` bigint NOT NULL, + `status` varchar(50) NOT NULL DEFAULT 'pending', + PRIMARY KEY(`id`), + INDEX `idx_user_id`(`user_id`) +) ENGINE InnoDB, + CHARSET utf8mb4, + COLLATE utf8mb4_0900_ai_ci; + +ALTER TABLE `products` ADD INDEX `idx_category_price`(`category`, `price`); +``` + +📋 **Plan**: **2** tables to create, **1** table to alter + + +--- + +▶️ **To apply** all schema changes from this PR, comment: +``` +schemabot apply -e production +``` + +
+ +
+Rollout Plans (Reviewed Target Already There) + + +## Schema Change Plan — Production + +**Database**: `testapp` | **Type**: `MySQL` | **Schema Name**: `testapp` + +*Requested by @jackjackbits at 2026-01-01 00:00:00 UTC · planned from [`abcdef1`](https://github.com/block/schemabot/commit/abcdef1234567890abcdef1234567890abcdef12)* + +✅ **Planned separately for all 3 targets** (`primary/testapp_1`, `primary/testapp_2`, `primary/testapp_3`) — 2 need this change, 1 is already at this schema. + +⚠️ **No schema changes for the reviewed target** — 2 targets still need this change, so an apply would not be a no-op. + +
+
Drop Column Blocked diff --git a/pkg/apitypes/vschema.go b/pkg/apitypes/vschema.go index 33c4f57b7..6fb0b5d8d 100644 --- a/pkg/apitypes/vschema.go +++ b/pkg/apitypes/vschema.go @@ -154,9 +154,21 @@ func (sc *SchemaChangeResponse) VSchemaUnsafeChanges() []UnsafeChange { return result } +// HasVSchemaWork reports whether a plan change's metadata records VSchema work. +// +// Either key alone means work, because they are two annotations of the same +// thing: an engine records a rendered diff when it has one and the flag when the +// work is known without one. Every caller that has to decide whether a namespace +// changes its VSchema reads it here rather than testing one key, so a surface +// that renders the work, a comparison that judges it, and a grouping key derived +// from that comparison cannot come to different answers about the same change. +func HasVSchemaWork(metadata map[string]string) bool { + return metadata[VSchemaDiffMetadataKey] != "" || metadata[VSchemaChangedMetadataKey] == "true" +} + // HasVSchemaChange reports whether this namespace's change carries VSchema work. func (sc *SchemaChangeResponse) HasVSchemaChange() bool { - return sc.Metadata[VSchemaDiffMetadataKey] != "" || sc.Metadata[VSchemaChangedMetadataKey] == "true" + return HasVSchemaWork(sc.Metadata) } // VSchemaChange is one keyspace's VSchema application state for display. Each diff --git a/pkg/cmd/internal/templates/preview_comment.go b/pkg/cmd/internal/templates/preview_comment.go index f94d8f8a1..42ac2dfe4 100644 --- a/pkg/cmd/internal/templates/preview_comment.go +++ b/pkg/cmd/internal/templates/preview_comment.go @@ -88,6 +88,8 @@ func previewCommentAllOutput() { {"DEPLOYMENT DRIFT (CLEAN, BLOCKED)", func() { fmt.Print(webhooktemplates.PreviewCommentPlanDriftCleanBlocked()) }}, {"DEPLOYMENT DRIFT (DETECTED)", func() { fmt.Print(webhooktemplates.PreviewCommentPlanDriftDetected()) }}, {"DEPLOYMENT DRIFT (COULD NOT VERIFY)", func() { fmt.Print(webhooktemplates.PreviewCommentPlanDriftUnverified()) }}, + {"ROLLOUT PLANS (CONVERGING)", func() { fmt.Print(webhooktemplates.PreviewCommentPlanRolloutConverging()) }}, + {"ROLLOUT PLANS (REVIEWED TARGET ALREADY THERE)", func() { fmt.Print(webhooktemplates.PreviewCommentPlanRolloutConvergedPrimary()) }}, {"HELP COMMENT", func() { fmt.Print(webhooktemplates.PreviewCommentHelp()) }}, {"SUPPORT CHANNEL FOOTER", func() { fmt.Print(webhooktemplates.PreviewCommentSupportChannel()) }}, {"OVERSIZED COMMENT", func() { fmt.Print(webhooktemplates.PreviewCommentOversized()) }}, @@ -201,6 +203,8 @@ func previewCommentPlanAllOutput() { {"DEPLOYMENT DRIFT (CLEAN, BLOCKED)", func() { fmt.Print(webhooktemplates.PreviewCommentPlanDriftCleanBlocked()) }}, {"DEPLOYMENT DRIFT (DETECTED)", func() { fmt.Print(webhooktemplates.PreviewCommentPlanDriftDetected()) }}, {"DEPLOYMENT DRIFT (COULD NOT VERIFY)", func() { fmt.Print(webhooktemplates.PreviewCommentPlanDriftUnverified()) }}, + {"ROLLOUT PLANS (CONVERGING)", func() { fmt.Print(webhooktemplates.PreviewCommentPlanRolloutConverging()) }}, + {"ROLLOUT PLANS (REVIEWED TARGET ALREADY THERE)", func() { fmt.Print(webhooktemplates.PreviewCommentPlanRolloutConvergedPrimary()) }}, {"DROP COLUMN BLOCKED", func() { fmt.Print(webhooktemplates.PreviewCommentDropColumnBlocked()) }}, {"DROP INDEX BLOCKED", func() { fmt.Print(webhooktemplates.PreviewCommentDropIndexBlocked()) }}, {"SCHEMA LINT ERRORS BLOCKED", func() { fmt.Print(webhooktemplates.PreviewCommentLintErrorsBlocked()) }}, diff --git a/pkg/tern/change_set_compare.go b/pkg/tern/change_set_compare.go index bd65f1a7c..bc50c0722 100644 --- a/pkg/tern/change_set_compare.go +++ b/pkg/tern/change_set_compare.go @@ -5,6 +5,7 @@ import ( "sort" "strings" + "github.com/block/schemabot/pkg/apitypes" "github.com/block/schemabot/pkg/ddl" ternv1 "github.com/block/schemabot/pkg/proto/ternv1" "github.com/block/schemabot/pkg/schema" @@ -153,7 +154,7 @@ func changeSetMultiset(parser ddl.StatementParser, cs ChangeSet) (driftChangeMul return nil, nil, fmt.Errorf("nil schema change") } ns := sc.Namespace - if sc.Metadata["vschema_changed"] == "true" { + if apitypes.HasVSchemaWork(sc.Metadata) { vschema[ns] = true } hasTableChanges := false diff --git a/pkg/webhook/plan_drift.go b/pkg/webhook/plan_drift.go index 64f7cb7bf..e2e96202b 100644 --- a/pkg/webhook/plan_drift.go +++ b/pkg/webhook/plan_drift.go @@ -243,7 +243,7 @@ func memberPlanChanges(cs tern.ChangeSet) []templates.KeyspaceChangeData { } ks.Statements = append(ks.Statements, tc.GetDdl()) } - if sc.GetMetadata()[apitypes.VSchemaChangedMetadataKey] == "true" { + if apitypes.HasVSchemaWork(sc.GetMetadata()) { ks.VSchemaChanged = true ks.VSchemaDiff = sc.GetMetadata()[apitypes.VSchemaDiffMetadataKey] } diff --git a/pkg/webhook/plan_drift_test.go b/pkg/webhook/plan_drift_test.go index 7fbf398be..b753b3178 100644 --- a/pkg/webhook/plan_drift_test.go +++ b/pkg/webhook/plan_drift_test.go @@ -494,6 +494,31 @@ func TestMemberPlanChanges_CarriesVSchemaChange(t *testing.T) { assert.False(t, templates.DeploymentPlanGroup{Changes: changes}.Empty()) } +// A rendered VSchema diff marks the namespace as carrying work on its own. The +// two metadata keys annotate the same thing, so a renderer that recognized only +// the flag would call a member with work "already at this schema" — and, since +// the grouping key reads the other annotation too, group it with members that +// genuinely have nothing to run. +func TestMemberPlanChanges_DiffAloneIsVSchemaWork(t *testing.T) { + cs := tern.ChangeSet{Changes: []*ternv1.SchemaChange{{ + Namespace: "testapp", + Metadata: map[string]string{apitypes.VSchemaDiffMetadataKey: "+ table users"}, + }}} + + changes := memberPlanChanges(cs) + require.Len(t, changes, 1) + assert.True(t, changes[0].VSchemaChanged) + assert.False(t, templates.DeploymentPlanGroup{Changes: changes}.Empty()) + + // The grouping key agrees, so this member is not folded in with one that has + // nothing to run. + withDiff, err := tern.ChangeSetFingerprint(schema.DialectMySQL, cs) + require.NoError(t, err) + empty, err := tern.ChangeSetFingerprint(schema.DialectMySQL, tern.ChangeSet{}) + require.NoError(t, err) + assert.NotEqual(t, empty, withDiff) +} + // A member already at the desired schema produces no changes at all, which is // the group the comment names as having nothing to apply. func TestMemberPlanChanges_EmptyPlanHasNoChanges(t *testing.T) { diff --git a/pkg/webhook/templates/preview.go b/pkg/webhook/templates/preview.go index 866de1332..bc81944e9 100644 --- a/pkg/webhook/templates/preview.go +++ b/pkg/webhook/templates/preview.go @@ -602,6 +602,67 @@ func PreviewCommentPlanDriftDetected() string { }) } +// previewRolloutMembers is the three independent targets the rollout previews +// below are rendered for, in rollout order with the reviewed primary first. +func previewRolloutMembers() []DeploymentDriftEntry { + return []DeploymentDriftEntry{ + {Deployment: "primary", Target: "testapp_1", Primary: true, Class: "planned"}, + {Deployment: "primary", Target: "testapp_2", Class: "planned"}, + {Deployment: "primary", Target: "testapp_3", Class: "planned"}, + } +} + +// PreviewCommentPlanRolloutConverging renders a plan comment for a rollout of +// independent targets partway through converging: the reviewed target and one +// other still need the change, and the third already holds it. +func PreviewCommentPlanRolloutConverging() string { + return RenderPlanComment(PlanCommentData{ + Database: "testapp", + SchemaName: "testapp", + Environment: "production", + HeadSHA: previewHeadSHA, + Repository: previewRepository, + RequestedBy: previewRequestedBy, + IsMySQL: true, + DatabaseType: "mysql", + Changes: samplePlanChanges(), + DeploymentDrift: &DeploymentDriftData{ + Computed: true, Clean: true, Independent: true, + Deployments: previewRolloutMembers(), + Plans: []DeploymentPlanGroup{ + {Members: []string{"primary/testapp_1", "primary/testapp_2"}, Primary: true, Changes: samplePlanChanges()}, + {Members: []string{"primary/testapp_3"}}, + }, + }, + }) +} + +// PreviewCommentPlanRolloutConvergedPrimary renders a plan comment for a rollout +// whose reviewed target already holds the desired schema while other targets do +// not. The reviewed plan is empty, so the comment renders no DDL — and says that +// an apply is still not a no-op rather than reading as one. +func PreviewCommentPlanRolloutConvergedPrimary() string { + return RenderPlanComment(PlanCommentData{ + Database: "testapp", + SchemaName: "testapp", + Environment: "production", + HeadSHA: previewHeadSHA, + Repository: previewRepository, + RequestedBy: previewRequestedBy, + IsMySQL: true, + DatabaseType: "mysql", + Changes: nil, + DeploymentDrift: &DeploymentDriftData{ + Computed: true, Clean: true, Independent: true, + Deployments: previewRolloutMembers(), + Plans: []DeploymentPlanGroup{ + {Members: []string{"primary/testapp_1"}, Primary: true}, + {Members: []string{"primary/testapp_2", "primary/testapp_3"}, Changes: samplePlanChanges()}, + }, + }, + }) +} + // PreviewCommentPlanDriftUnverified renders a plan comment whose review-time // drift rollup could not be computed, so the plan check fails closed. func PreviewCommentPlanDriftUnverified() string { From ba9785abb897cd816dfc9f685f0a18e9c7eecef6 Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Thu, 24 Sep 2026 00:48:54 -0400 Subject: [PATCH 4/6] fix(github): surface a converging rollout on the paths a reviewer reads The headline for an empty reviewed plan said an apply "would not be a no-op". An apply is gated on the reviewed target's own re-plan, so an empty reviewed plan ends the command before any member runs: the unconverged targets the line names are not reconciled by applying this plan, and a reviewer acting on that sentence would wait for a convergence that never comes. It now states what is true of the targets and says the apply will not run the change for them. The line also could not reach the reviewer on the two paths that post most comments. A converging rollout passes its contract, so it is clean, and AnyEnvHasDriftToShow answered only on class: auto-plan took a clean rollup with a converged primary as nothing to say, retired the prior comments and posted none, and the multi-environment comment collapsed to the single green line for every environment. A clean rollup with a member that still needs the change is now drift to show, which is the one case where no environment planning changes does not mean the fleet holds this schema. Co-Authored-By: Claude Opus 5 --- TEMPLATES.md | 2 +- pkg/apitypes/vschema.go | 14 ++-- pkg/tern/change_set_compare.go | 5 +- pkg/webhook/templates/plan.go | 44 +++++++++--- pkg/webhook/templates/plan_drift_test.go | 92 ++++++++++++++++++++++-- 5 files changed, 135 insertions(+), 22 deletions(-) diff --git a/TEMPLATES.md b/TEMPLATES.md index 5354b2823..cb8e24ef6 100644 --- a/TEMPLATES.md +++ b/TEMPLATES.md @@ -1588,7 +1588,7 @@ schemabot apply -e production ✅ **Planned separately for all 3 targets** (`primary/testapp_1`, `primary/testapp_2`, `primary/testapp_3`) — 2 need this change, 1 is already at this schema. -⚠️ **No schema changes for the reviewed target** — 2 targets still need this change, so an apply would not be a no-op. +⚠️ **No schema changes for the reviewed target** — 2 targets still need this change, and applying this plan will not run it for them.
diff --git a/pkg/apitypes/vschema.go b/pkg/apitypes/vschema.go index 6fb0b5d8d..5dc9f54fa 100644 --- a/pkg/apitypes/vschema.go +++ b/pkg/apitypes/vschema.go @@ -158,10 +158,16 @@ func (sc *SchemaChangeResponse) VSchemaUnsafeChanges() []UnsafeChange { // // Either key alone means work, because they are two annotations of the same // thing: an engine records a rendered diff when it has one and the flag when the -// work is known without one. Every caller that has to decide whether a namespace -// changes its VSchema reads it here rather than testing one key, so a surface -// that renders the work, a comparison that judges it, and a grouping key derived -// from that comparison cannot come to different answers about the same change. +// work is known without one. +// +// Review reads the annotations here: the surface that renders a namespace's +// work, the comparison that judges it, and the grouping key derived from that +// comparison cannot come to different answers about the same change. Storage and +// apply still test the flag directly, which agrees with this for every plan an +// engine produces today, since an engine that records a diff records the flag +// with it. An engine that recorded only the diff would be described as changing +// its VSchema and persisted as not changing it, so widening those callers is +// what keeps the two halves from splitting. func HasVSchemaWork(metadata map[string]string) bool { return metadata[VSchemaDiffMetadataKey] != "" || metadata[VSchemaChangedMetadataKey] == "true" } diff --git a/pkg/tern/change_set_compare.go b/pkg/tern/change_set_compare.go index bc50c0722..42aac6d7b 100644 --- a/pkg/tern/change_set_compare.go +++ b/pkg/tern/change_set_compare.go @@ -162,8 +162,9 @@ func changeSetMultiset(parser ddl.StatementParser, cs ChangeSet) (driftChangeMul if tc == nil { return nil, nil, fmt.Errorf("nil table change in namespace %q", ns) } - // In the plan/proto representation a vschema change is signalled via - // Metadata["vschema_changed"] and carries no table DDL. A vschema table + // In the plan/proto representation a vschema change is signalled by the + // metadata keys HasVSchemaWork reads — a rendered diff, the changed + // flag, or both — and carries no table DDL. A vschema table // change indicates malformed input (e.g. a change set built from an // apply request's DdlChanges), so fail closed rather than skip it and // risk a false match. Checked before the shard skip so a sharded diff --git a/pkg/webhook/templates/plan.go b/pkg/webhook/templates/plan.go index 96d7cb58b..bf06ab3ee 100644 --- a/pkg/webhook/templates/plan.go +++ b/pkg/webhook/templates/plan.go @@ -237,6 +237,13 @@ type DeploymentDriftData struct { // DeploymentPlanGroup is the members of a rollout that would run the same plan. // Members share a group exactly when their plans are identical work, so a group // is what the comment can describe once and attribute to all of them. +// +// Identical work is what an apply would do to each member, not what each +// member's plan looks like written down. Members are keyed on canonicalized +// table DDL and on which namespaces change their VSchema, because a VSchema is +// applied as the file the PR holds rather than as a computed delta: two members +// given the same file are doing the same work even where their recorded diffs +// differ, since a diff differs by where the member started. type DeploymentPlanGroup struct { // Members names the group's members the way an operator addresses them, in // rollout order. @@ -887,21 +894,27 @@ func writeMultiEnvIgnoredNamespaces(sb *strings.Builder, data MultiEnvPlanCommen } } -// noChangesHeadline states what an empty reviewed plan means for the rollout an -// apply would run on. +// noChangesHeadline states what an empty reviewed plan means for the rest of the +// rollout. // // On its own an empty plan reads as a no-op, and that is only established for -// the target that was reviewed. An independent rollout plans each target -// against its own live schema, so a primary already at the desired schema says -// nothing about the rest: the apply can still run DDL on every other target. A -// reviewer who takes the green line at face value merges believing nothing will -// happen, so where some target still needs the change the headline says so. +// the target that was reviewed. An independent rollout plans each target against +// its own live schema, so a primary already at the desired schema says nothing +// about the rest: other targets can still be missing the change. A reviewer who +// takes the green line at face value merges believing the fleet holds this +// schema, so where some target does not the headline says so. +// +// It says what is true of the targets, not what an apply would do about them. +// An apply is gated on the reviewed target's own re-plan, so an empty reviewed +// plan ends the command before any member runs — the unconverged targets named +// here are not reconciled by applying this plan, and the headline must not read +// as though they would be. func noChangesHeadline(drift *DeploymentDriftData) string { changing := changingTargetCount(drift) if changing == 0 { return "✅ **No schema changes detected**" } - return fmt.Sprintf("%s **No schema changes for the reviewed target** — %s this change, so an apply would not be a no-op.", + return fmt.Sprintf("%s **No schema changes for the reviewed target** — %s this change, and applying this plan will not run it for them.", glyph.Attention, countedVerb(changing, "target still needs", "targets still need")) } @@ -2206,8 +2219,16 @@ func allPlansIdentical(data MultiEnvPlanCommentData) bool { // AnyEnvHasDriftToShow reports whether any environment has drift that must be // surfaced even when no environment plans changes: a deployment that diverged or -// could not be verified. A clean uniform rollup is not "drift to show" — with no -// changes anywhere the simple no-changes message is clearer. +// could not be verified, or a rollout still converging. A clean uniform rollup +// is not "drift to show" — with no changes anywhere the simple no-changes +// message is clearer. +// +// A converging rollout passes its contract, so it is clean and says nothing here +// on that count. It is still the one case where no environment planning changes +// does not mean the fleet holds this schema: the reviewed target is at the +// desired schema and another target is not. Callers consult this only when +// nothing else would post a comment, so leaving it out is what decides whether +// the reviewer is told at all. func AnyEnvHasDriftToShow(data MultiEnvPlanCommentData) bool { for _, env := range data.Environments { plan, ok := data.Plans[env] @@ -2218,6 +2239,9 @@ func AnyEnvHasDriftToShow(data MultiEnvPlanCommentData) bool { if !d.Computed || !d.Clean { return true } + if changingTargetCount(d) > 0 { + return true + } } return false } diff --git a/pkg/webhook/templates/plan_drift_test.go b/pkg/webhook/templates/plan_drift_test.go index 719f0e14a..76d2a2677 100644 --- a/pkg/webhook/templates/plan_drift_test.go +++ b/pkg/webhook/templates/plan_drift_test.go @@ -269,13 +269,31 @@ func TestRenderPlanComment_DriftBeforeChangeList(t *testing.T) { } // AnyEnvHasDriftToShow drives the auto-plan comment-skip decision: it is true -// only when an environment has drift that must be explained (diverged or -// unverifiable), so a red check from drift is never left without a comment. A -// clean or nil rollup is not "drift to show". +// when an environment has drift that must be explained (diverged or +// unverifiable) or is a rollout still converging, so a PR is never left with no +// comment where the fleet does not hold the reviewed schema. A clean rollup +// whose targets are all there, and a nil rollup, are not "drift to show". func TestAnyEnvHasDriftToShow(t *testing.T) { drift := func(computed, clean bool) *DeploymentDriftData { return &DeploymentDriftData{Computed: computed, Clean: clean} } + converging := func() *DeploymentDriftData { + d := drift(true, true) + d.Independent = true + d.Plans = []DeploymentPlanGroup{ + {Members: []string{"primary/testapp_1"}, Primary: true}, + {Members: []string{"primary/testapp_2"}, Changes: convergingAlter()}, + } + return d + } + converged := func() *DeploymentDriftData { + d := drift(true, true) + d.Independent = true + d.Plans = []DeploymentPlanGroup{ + {Members: []string{"primary/testapp_1", "primary/testapp_2"}, Primary: true}, + } + return d + } cases := []struct { name string plans map[string]*PlanCommentData @@ -286,6 +304,8 @@ func TestAnyEnvHasDriftToShow(t *testing.T) { {"diverged rollup", map[string]*PlanCommentData{"prod": {DeploymentDrift: drift(true, false)}}, true}, {"uncomputed rollup", map[string]*PlanCommentData{"prod": {DeploymentDrift: drift(false, false)}}, true}, {"nil plan", map[string]*PlanCommentData{"prod": nil}, false}, + {"converging rollout", map[string]*PlanCommentData{"prod": {DeploymentDrift: converging()}}, true}, + {"fully converged rollout", map[string]*PlanCommentData{"prod": {DeploymentDrift: converged()}}, false}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -295,6 +315,68 @@ func TestAnyEnvHasDriftToShow(t *testing.T) { } } +func convergingAlter() []KeyspaceChangeData { + return []KeyspaceChangeData{{ + Keyspace: "testapp", + Statements: []string{"ALTER TABLE `users` ADD COLUMN `email` varchar(255)"}, + }} +} + +// A rollout partway through converging reaches the reviewer on the multi +// environment comment too. Both environments' reviewed targets are already at +// the desired schema while a target in production is not, so the comment cannot +// collapse to the one green line that says no environment changes: that line is +// what a reviewer merges on, and here it would be read as the whole fleet +// holding this schema. +func TestRenderMultiEnvPlanComment_ConvergingRolloutIsNotAllClear(t *testing.T) { + convergingDrift := func(others ...string) *DeploymentDriftData { + entries := []DeploymentDriftEntry{ + {Deployment: "primary", Target: "testapp_1", Primary: true, Class: "planned"}, + } + for _, o := range others { + entries = append(entries, DeploymentDriftEntry{Deployment: "primary", Target: o, Class: "planned"}) + } + return &DeploymentDriftData{ + Computed: true, Clean: true, Independent: true, + Deployments: entries, + Plans: []DeploymentPlanGroup{ + {Members: []string{"primary/testapp_1"}, Primary: true}, + {Members: []string{"primary/" + others[0]}, Changes: convergingAlter()}, + }, + } + } + convergedDrift := &DeploymentDriftData{ + Computed: true, Clean: true, Independent: true, + Deployments: []DeploymentDriftEntry{ + {Deployment: "primary", Target: "testapp_1", Primary: true, Class: "planned"}, + {Deployment: "primary", Target: "testapp_2", Class: "planned"}, + }, + Plans: []DeploymentPlanGroup{ + {Members: []string{"primary/testapp_1", "primary/testapp_2"}, Primary: true}, + }, + } + + data := MultiEnvPlanCommentData{ + Database: "testapp", DatabaseType: "mysql", IsMySQL: true, + Environments: []string{"staging", "production"}, + Plans: map[string]*PlanCommentData{ + "staging": {Database: "testapp", Environment: "staging", IsMySQL: true, DeploymentDrift: convergedDrift}, + "production": {Database: "testapp", Environment: "production", IsMySQL: true, DeploymentDrift: convergingDrift("testapp_2")}, + }, + } + + out := RenderMultiEnvPlanComment(data) + assert.NotContains(t, out, "**No schema changes detected** for any environment.") + assert.Contains(t, out, "⚠️ **No schema changes for the reviewed target** — 1 target still needs this change, and applying this plan will not run it for them.") + // Staging is genuinely converged, so its own section keeps the green line. + assert.Contains(t, out, "✅ **No schema changes detected**") + + // With every environment's rollout converged, the all-clear is correct and + // still renders. + data.Plans["production"] = &PlanCommentData{Database: "testapp", Environment: "production", IsMySQL: true, DeploymentDrift: convergedDrift} + assert.Contains(t, RenderMultiEnvPlanComment(data), "**No schema changes detected** for any environment.") +} + // When one deployment addresses several targets, the deployment name alone // labels two different members identically. The plan comment names every member // of that deployment by its routing pair, while a sibling deployment that @@ -541,12 +623,12 @@ func TestRenderPlanComment_ConvergedPrimaryDoesNotHeadlineAsNoOp(t *testing.T) { out := RenderPlanComment(data) assert.NotContains(t, out, "✅ **No schema changes detected**") - assert.Contains(t, out, "⚠️ **No schema changes for the reviewed target** — 2 targets still need this change, so an apply would not be a no-op.") + assert.Contains(t, out, "⚠️ **No schema changes for the reviewed target** — 2 targets still need this change, and applying this plan will not run it for them.") // The same shape with a single other target agrees with itself on number. data.DeploymentDrift.Deployments = data.DeploymentDrift.Deployments[:2] data.DeploymentDrift.Plans[1].Members = []string{"primary/testapp_2"} - assert.Contains(t, RenderPlanComment(data), "⚠️ **No schema changes for the reviewed target** — 1 target still needs this change, so an apply would not be a no-op.") + assert.Contains(t, RenderPlanComment(data), "⚠️ **No schema changes for the reviewed target** — 1 target still needs this change, and applying this plan will not run it for them.") } // A rollout where every target is already at the desired schema is a no-op, and From 81396f4347d7eeaf09c12f137cd4b8e1effe006d Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Thu, 24 Sep 2026 01:02:53 -0400 Subject: [PATCH 5/6] docs: say that a converging rollout keeps the auto-plan comment The skip at the auto-plan gate is narrower than "no environment has changes", and three places still described the old rule: the operator docs for check runs, the gate's own comment, and handleMultiEnvPlan. A maintainer reading any of them could narrow the call site and bring back the case where a PR is left with no comment while a target is missing the change. Also corrects two doc comments that outlived the headline they described, on the converged-primary test and the preview that generates its TEMPLATES.md rendering. Co-Authored-By: Claude Opus 5 --- docs/check-runs.md | 8 ++++++++ pkg/webhook/plan.go | 15 ++++++++++----- pkg/webhook/templates/plan_drift_test.go | 4 ++-- pkg/webhook/templates/preview.go | 4 ++-- 4 files changed, 22 insertions(+), 9 deletions(-) diff --git a/docs/check-runs.md b/docs/check-runs.md index c8cd0f584..5e9a5c0dd 100644 --- a/docs/check-runs.md +++ b/docs/check-runs.md @@ -750,6 +750,14 @@ the aggregate blocks. If auto-plan finds no changes, the records become `success` and the aggregate passes. Auto-plan skips the PR comment when every environment has no changes and no errors, but it still writes the check state. +Two cases keep the comment even though no environment plans changes, because +the reviewer needs to be told something the check state alone does not say. A +deployment that diverged or could not be verified fails the check closed, and +the comment is what explains the red. A rollout of independent targets whose +reviewed target is already at the desired schema still posts when another +target is not: the reviewed plan is empty, but the fleet does not hold this +schema. + ### PR touches no managed schema files If the PR has no managed schema files, SchemaBot publishes passing aggregate diff --git a/pkg/webhook/plan.go b/pkg/webhook/plan.go index ceb6ee03a..17f918972 100644 --- a/pkg/webhook/plan.go +++ b/pkg/webhook/plan.go @@ -227,7 +227,9 @@ func (h *Handler) planForResolvedDatabaseBlocked(ctx context.Context, repo strin } // handleMultiEnvPlan runs plan for all configured environments and posts a single combined comment. -// When isAutoPlan is true and no environments have changes or errors, the comment is skipped to reduce PR noise. +// When isAutoPlan is true and there is genuinely nothing to show, the comment is skipped to reduce +// PR noise — which is narrower than "no environment has changes": a rollout still converging plans +// no changes for the target that was reviewed and is not a no-op for the fleet. // commentID is the command comment to acknowledge once discovery commits this // deployment to acting; auto-plans pass zero (no comment to acknowledge). func (h *Handler) handleMultiEnvPlan(repo string, pr int, databaseName, tenant string, installationID int64, requestedBy string, isAutoPlan bool, postPlanComment bool, commentID int64) { @@ -514,10 +516,13 @@ func (h *Handler) handleMultiEnvPlan(repo string, pr int, databaseName, tenant s } // Auto-plan: skip the comment only when there is genuinely nothing to show — - // no changes, no errors, and no deployment drift. A drifted or unverifiable - // deployment fails the check closed even when every primary plan is a clean - // no-op, so the comment must still post to explain why the check is red; - // skipping it would leave a red check with no visible reason on the PR. + // no changes, no errors, and no deployment drift. Two kinds of drift keep the + // comment. A drifted or unverifiable deployment fails the check closed even + // when every primary plan is a clean no-op, so the comment must still post to + // explain why the check is red; skipping it would leave a red check with no + // visible reason on the PR. A rollout still converging keeps it for the + // opposite reason: the check is green and the reviewed plan is empty, and the + // comment is the only place that says another target is missing the change. if isAutoPlan { hasErrors := len(multiEnvData.Errors) > 0 anyChanges := false diff --git a/pkg/webhook/templates/plan_drift_test.go b/pkg/webhook/templates/plan_drift_test.go index 76d2a2677..0af20b929 100644 --- a/pkg/webhook/templates/plan_drift_test.go +++ b/pkg/webhook/templates/plan_drift_test.go @@ -598,8 +598,8 @@ func TestRenderPlanComment_DriftContainsHostileMemberNames(t *testing.T) { // A rollout whose reviewed target is already at the desired schema, while other // targets are not, must not headline as a no-op. The reviewed plan is empty, so // the comment shows no DDL; a reviewer who reads "no schema changes detected" -// merges believing an apply does nothing, when it would run the change on every -// target that has not had it yet. +// merges believing the fleet holds this schema, when targets are still missing +// it and applying this plan does not give it to them. func TestRenderPlanComment_ConvergedPrimaryDoesNotHeadlineAsNoOp(t *testing.T) { alter := []KeyspaceChangeData{{ Keyspace: "testapp", diff --git a/pkg/webhook/templates/preview.go b/pkg/webhook/templates/preview.go index bc81944e9..a15074953 100644 --- a/pkg/webhook/templates/preview.go +++ b/pkg/webhook/templates/preview.go @@ -639,8 +639,8 @@ func PreviewCommentPlanRolloutConverging() string { // PreviewCommentPlanRolloutConvergedPrimary renders a plan comment for a rollout // whose reviewed target already holds the desired schema while other targets do -// not. The reviewed plan is empty, so the comment renders no DDL — and says that -// an apply is still not a no-op rather than reading as one. +// not. The reviewed plan is empty, so the comment renders no DDL — and names the +// targets that are still missing the change rather than reading as a no-op. func PreviewCommentPlanRolloutConvergedPrimary() string { return RenderPlanComment(PlanCommentData{ Database: "testapp", From 412790170f2554879ff7e4a6ae3ef3961873d771 Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Thu, 24 Sep 2026 19:05:51 -0400 Subject: [PATCH 6/6] docs: say that the converged-primary preview counts targets, not names them The doc comment claimed the preview names the targets still missing the change. It does not: the rollup line lists every member, and nothing attributes a member to the group that still needs the change. Co-Authored-By: Claude Opus 5 --- pkg/webhook/templates/preview.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/webhook/templates/preview.go b/pkg/webhook/templates/preview.go index a15074953..7d58a9536 100644 --- a/pkg/webhook/templates/preview.go +++ b/pkg/webhook/templates/preview.go @@ -639,8 +639,10 @@ func PreviewCommentPlanRolloutConverging() string { // PreviewCommentPlanRolloutConvergedPrimary renders a plan comment for a rollout // whose reviewed target already holds the desired schema while other targets do -// not. The reviewed plan is empty, so the comment renders no DDL — and names the -// targets that are still missing the change rather than reading as a no-op. +// not. The reviewed plan is empty, so the comment renders no DDL — and counts the +// targets that are still missing the change rather than reading as a no-op. It +// counts them rather than naming them: the rollup line lists every member, but +// nothing here attributes a member to the group that still needs the change. func PreviewCommentPlanRolloutConvergedPrimary() string { return RenderPlanComment(PlanCommentData{ Database: "testapp",