feat(github): say how much a rollout's targets agree this round

[aparajon]

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 exactly that and nothing else:

each target holds its own schema, so their plans are not expected to match

That is true of the contract and silent about the round in front of the reviewer. Targets that are free to differ usually do not, and the shape a fleet rolling out over several PRs actually has — some targets changed, the rest already there — was invisible.

This groups the members by the plan each would run and says what came out.

What changes for the reader

A production fleet of three targets, where two still need the reviewed change and one already has it:

Before                                   After

✅ Planned separately for all 3 targets   ✅ Planned separately for all 3 targets
   (t1, t2, t3) — each target holds         (t1, t2, t3) — 2 need this change,
   its own schema, so their plans           1 is already at this schema.
   are not expected to match.

   ▲                                        ▲
   │ says what the config permits           │ says what this round does
   │ identical on every PR, forever         │ changes as the fleet converges
   ✗ a reviewer cannot tell whether
     this apply touches 3 targets or 2

The wording, by case:

The round	What the comment says
Every target needs the same change	every target needs the same change.
Some are already there	2 need this change, 1 is already at this schema.
Targets need different changes	2 distinct plans. Each target applies its own.
Different changes, some already there	2 distinct plans across the 3 targets that change; 2 are already at this schema.
The whole fleet is already there	every target is already at this schema.
Members were not grouped	the prior contract sentence, unchanged

Divergence renders on the success glyph. Under a targets list, targets holding different schemas is the contract rather than a problem, so calling it out with the attention glyph would send an operator to reconcile a fleet that is behaving correctly.

Where grouping applies

Members group on the plan fingerprint, so they share a group exactly when their plans are the same work. Two cases deliberately stay ungrouped:

• A blocked rollup. Grouping describes the targets that were planned, so it is only meaningful once every one of them was. A blocked rollup still lists each member on its own: the operator's next step is the target that could not be planned, not the plans of an apply that cannot run. This upholds MG-1.
• Mirrored members. A clean mirrored rollup has already proved they are one group. Saying so again, in the vocabulary of a fleet free to diverge, would read as an outcome rather than the requirement that let the check pass.

A group with no statements and no vschema rewrite is "already at this schema" — a plan in its own right, not a missing one. A plan that only rewrites the vschema runs no DDL and is still work, so it is not folded in with the targets that have nothing to do.

Each group carries its plan

A group holds the changes its own members would run, converted into the same shape the comment already renders the reviewed plan in, rather than a summary of it. A sharded namespace keeps both views of its changes and a satisfied shard stays visible, so the next change can render a group's plan through the code that renders the plan a reviewer has already read, instead of a second renderer that agrees until it does not.

Not in this change

The per-group DDL blocks. The comment states how many distinct plans there are but still renders the reviewed plan's DDL once, unattributed to a group. That change carries the preview fixtures for these renderings, so TEMPLATES.md shows the finished output rather than a half-rendered intermediate.

Opened by Claude (Claude Opus 5).

🤖 Generated with Claude Code

[Kiran01bm]

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

Verdict: 3 findings — 1 blocking (drift line contradicts "No changes detected"), 2 non-blocking.

Blocking

A converged primary makes the drift line claim N targets need a change the same comment says does not exist. templates/plan.go:1253 assumes the non-empty plan group is the reviewed primary's, but with 3 independent targets where the primary (rollout index 0) is already at the desired schema and testapp_2/testapp_3 still need ALTER TABLE users ADD COLUMN email, the clean rollup yields an empty primary group plus one group of two, so the line renders "2 need this change, 1 is already at this schema." writeDeploymentDrift runs before the unconditional if totalChanges == 0 short-circuit at line 356, so the next line is "✅ No schema changes detected" and the DDL those two targets will run appears nowhere — "this change" refers to nothing the reviewer can see. This is the converging-fleet case the PR exists to describe, and no test covers it: TestRenderPlanComment_PlanGroupsDescribeThisRound always seeds a non-empty top-level Changes, so the contradiction cannot surface.

Non-blocking

Same contradiction in the 2-target shape: "1 needs this change" immediately followed by "No schema changes detected". With primary/testapp_1 (primary, already converged, empty plan) and primary/testapp_2 (needs the ADD COLUMN), reviewTimeDrift runs unconditionally after a successful plan, the rollup is clean, and describePlanGroups takes case plans == 1. Executing RenderPlanComment at this head reproduces verbatim "✅ Planned separately for all 2 targets (primary/testapp_1, primary/testapp_2) — 1 needs this change, 1 is already at this schema." followed by "✅ No schema changes detected"; the pre-PR sentence ("each target holds its own schema, so their plans are not expected to match.") claimed no change existed.

Two different vschema rewrites of the same namespace collide into one group and are described as "the same change". Grouping keys on the plan fingerprint at plan_drift.go:147, but the fingerprint's vschema contribution is namespace-only — lines = append(lines, "v"+fingerprintRecord(ns)) — while the differing per-target diff lives in a separate metadata key. Two independent Vitess targets needing a vindex on users vs on orders therefore share a fingerprint and the comment prints "every target needs the same change.", which is false; the CLI's own plan fingerprint asserts the opposite contract ("plans with differing VSchema diffs must not dedupe") and change_set_fingerprint_test.go has no differing-diff case.

General suggestions

• Consider moving writeDeploymentDrift after the totalChanges == 0 short-circuit, or suppressing the change-count clause when the primary's group is empty — both blocking/non-blocking contradictions above are the same ordering choice at plan.go:346 vs 356.

The one thing that could have broken, verified

An errored member silently sharing the empty-fingerprint bucket would have merged unplanned targets into the "already at this schema" group. It cannot happen: grouping is gated on rollup.Clean && independent, and rollupIndependentMembers fails a member closed whenever recordMemberPlan cannot key it, so every entry in a clean independent rollup carries a non-empty PlanFingerprint.

Verified correct

• names[i] indexing in deploymentPlanGroups is safe: routing.DisplayNames returns exactly len(members) entries.
• Exactly one group carries Primary (index 0 always allocates), and the SortStableFunc comparator is a consistent ordering, so the primary's group is first.
• Empty() and the fingerprint agree — canonicalDDLForDrift rejects empty DDL — so a group can never be Empty while carrying work, and at most one group is ever Empty.
• Members sharing a fingerprint share the same multiset with counts, so the first member's statement count / Empty() is valid for the whole group.
• Shard-only namespaces are kept once, in first-appearance order (covered by TestMemberPlanChanges_KeepsShardOnlyNamespace).
• nil elements in cs.Shards and cs.Changes are skipped before dereference — no nil-deref path in the new code.
• DeploymentDriftData is never persisted or serialized, so it cannot round-trip, lose Plans, and silently fall back to the contract sentence.

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

[morgo]

🤖 Reviewed on Morgan's behalf, at bf36a875. Approving — every rule this PR adds is pinned by a test that fails when I break it, and all six sentences in the description's table are what the code actually produces. One thing worth deciding before the stack lands, plus two precision notes.

1. memberPlanChanges is a second implementation of the reviewed-plan renderer, and the two already disagree

The doc comment says it renders a 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." The second half is not so: the reviewed plan is built at pkg/webhook/plan.go:925-973, and this is a parallel build of the same []KeyspaceChangeData. (The counting side genuinely is shared — Empty() goes through countChanges/keyspaceStatementCount, which is why a group whose only DDL is per-shard is correctly not "already at this schema".)

Where they differ, on the same input:

• A namespace that appears only on shard rows. memberPlanChanges keeps it, via the shardedNamespaces fallback, with the comment "dropping it would silently remove work from a plan the comment claims to describe in full." plan.go builds ksData only by looping planResp.Changes, so it drops exactly that namespace. One of two things is true and it matters which: either the shape is producible, and the reviewed-plan block has been quietly omitting work all along — in which case the fix belongs in plan.go, not only in the new renderer, or the two surfaces will keep disagreeing; or it is not producible, and the fallback plus TestMemberPlanChanges_KeepsShardOnlyNamespace cover a shape that cannot occur. I could not settle it from the planner side, and you are better placed to say.
• A shard that reports changes but whose DDL is all empty. plan.go treats this as "shard %q in keyspace %q reported %d change(s) with no DDL — plan is incomplete for this shard" and refuses to render the shard. memberPlanChanges takes len(shard.Statements) == 0 and sets Satisfied = true, which renders it as already at this schema — the inverse. This one is unreachable through the grouped path today, and I checked rather than assumed: canonicalDDLForDrift returns empty DDL for a blank statement (pkg/tern/local_plan_drift.go:204), so such a member fails its self-comparison, classifies DeploymentErrored, clears Clean, and never reaches deploymentPlanGroups. It is safe because of a gate two packages away, not because of anything local, so it is worth a word in the comment.

Neither is a defect in what ships. I am raising it because the comment invites a reader to believe the two renderers cannot drift, and they already have.

Notes

• DeploymentPlanGroup.Changes is the first member's rendering, not "the plan every member of the group would run". Members share a group on canonicalized work, and TestChangeSetFingerprint_CanonicallyIdenticalSetsShareAKey pins that backticked and bare spellings of the same ALTER share a key — so two members can legitimately group while their raw DDL text differs, and the group renders whichever one came first in rollout order. Same work, so the grouping is right; but the rendered statement is a representative's spelling rather than every member's, and the field comment currently promises the stronger thing.
• deploymentPlanGroups keys a map[string] on PlanFingerprint, where "" means "do not group". It is safe here only because the rollup.Clean gate excludes every errored member, which is the one thing that produces an empty fingerprint — and TestDeploymentDriftPreview_BlockedRollupIsNotGrouped pins that gate, so it cannot be removed silently. Flagging it because the gate and the sentinel are in different packages: the next caller of PlanFingerprint that is not behind Clean will group every blocked member into one plan.
• case converged == 0: "%d distinct plans. Each target applies its own." is the only arm that does not name a member count, while the arm below it says "across the %d targets that change". Matches the description's table, so intentional — noting it only in case the asymmetry was not.

What I checked rather than took on trust

• Fault injection — eight for eight. Dropping the rollup.Clean gate → BlockedRollupIsNotGrouped; dropping the independent gate → MirroredMembersAreNotGrouped; inverting the primary-first comparator → PrimaryGroupComesFirst + DifferentWorkSplits + ConvergedTargetsAreTheirOwnGroup; forcing Primary false → SameWorkGroupsTogether + PrimaryGroupComesFirst; forcing Satisfied false → MarksSatisfiedShards; removing the shard-only-namespace fallback → KeepsShardOnlyNamespace; disabling the plans == 1 && converged == 0 arm → PlanGroupsDescribeThisRound + DriftCleanNamesMultiTargetMembers; forcing countedVerb plural → PlanGroupsDescribeThisRound + VSchemaOnlyPlanIsNotAlreadyApplied; disabling the ungrouped fallback → UngroupedIndependentRollupStatesTheContract. Nothing I could break stayed green.
• All six rows of the wording table are literally what the code emits. Traced each arm by hand: plans==0 → "every target is already at this schema."; plans==1 && converged==0 → "every target needs the same change."; plans==1 → countedVerb(2,"needs","need") + countedVerb(1,"is","are") giving "2 need this change, 1 is already at this schema."; converged==0 → "2 distinct plans. Each target applies its own."; default → "2 distinct plans across the 3 targets that change; 2 are already at this schema."; len(groups)==0 → the prior contract sentence verbatim. Subject-verb agreement holds at 1 on both halves.
• Empty() cannot call a shard-only plan "already at this schema". keyspaceStatementCount falls back to the distinct statements across ks.Shards when the collapsed Statements is empty, so a group whose DDL lives only on shard rows counts above zero. This was the failure mode I most expected and it is closed by reuse rather than by a new count.
• A vschema-only plan is not folded in with the converged targets. countChanges returns keyspacesWithVSchema separately and Empty() sums both, so a namespace whose only work is a vschema rewrite keeps its own group. Pinned by VSchemaOnlyPlanIsNotAlreadyApplied.
• Group order is deterministic and does not depend on map iteration. byPlan is only an index; groups are appended in rollout order of first appearance, and shardedNamespaces is a slice built alongside the map for the same reason. A re-rendered comment on a later push cannot reshuffle.
• Exactly one group can carry Primary. It is set from i == 0 at group creation, so only the group the first member opens can have it, and SortStableFunc leaves the rest in insertion order.
• memberPlanChanges is nil-safe where the reviewed-plan builder is not — it goes through GetDdl()/GetChanges() getters, while plan.go:965 dereferences t.DDL without a nil check on the namespace-level loop. Not introduced here, and not reachable through this path, but the new code is the better-behaved of the two.
• Merge-base against pr1423 is an ordinary commit; +668/−5, and the five deletions are the one fmt.Fprintf call this replaces. No test deletions. CI: 41 checks, no genuine failures.