feat(cli): render a storage plan the way every other plan is rendered

[aparajon]

What this adds

SchemaBot runs schema changes against your databases, and it has a database of its own: the bookkeeping storage that holds plans, applies, checks, leases and locks. That storage has a schema too, which every release converges when it starts up, and storage plan is how an operator asks what a release will converge before rolling it out.

This renders that plan through the same templates as plan: the header box, the environment heading, the per-table sections with their change symbols, the disclosure of changes that will not run, and the summary line. The question being asked is the same one either way — here is a live database, here is a desired schema, here is the DDL between them — and only the target differs. So anyone who can read plan can read storage plan, which matters because plan is read every day and storage plan is read during an incident.

Two shared templates grow an optional field, and keep their existing behavior when it is empty: one to name the database family in the title (SchemaBot's own storage is MySQL or PostgreSQL, which the existing MySQL/Vitess flag cannot express), and one to rename the row carrying the desired schema, which here is a release or a path rather than a directory.

A destructive statement renders through the CLI's existing unsafe-change templates rather than a parallel set. The three dispositions it can be in are the three plan and apply already have: disclosed by a plan, refused by an apply, or running under consent already in effect. Two of those had a detail hard-coded that only a schema change apply could supply — the re-run command to copy, and the phrase naming what granted consent — so both became parameters. For storage the consent is not necessarily a flag at all, since a deployment's own policy can permit destructive changes with nothing on the command line.

Three rules the rendering follows:

• A plan shows the whole difference between the two schemas, whatever each statement's disposition — the refused DROP to weigh, the manual remediation to run by hand — and the sections underneath say which of it will run. That is what plan does with an unsafe change it is about to refuse. Each disposition gets its own SQL section, because the MySQL formatter combines a table's alters into a single statement: an ALTER split so its safe half could run would otherwise be recombined into a statement nothing is going to run.
• A manual remediation outranks the destructive refusal. It blocks the whole set, so --allow-unsafe would permit the DROP and converge nothing. The statement is still disclosed as destructive, but the blocked-apply heading and its re-run command are not printed: a remedy that changes nothing is worse than none.
• The summary counts tables; a report lists statements. Those are the same thing on MySQL and not on PostgreSQL, where a plan emits one statement per missing column and per missing index — so a table short two columns read as "2 tables to alter" against a plan naming one table. The summary collapses to one entry per table and kind; every statement is still printed.

storage plan — outstanding changes, with one refused

╭─────────────────────────────────────────────────────────────────╮
│  MySQL Schema Change Plan                                       │
│                                                                 │
│  Database: schemabot on db-1.example                            │
│  Schema: the schema files of release v1.4.0 in block/schemabot  │
╰─────────────────────────────────────────────────────────────────╯

Production
     + apply_control_requests
       CREATE TABLE `apply_control_requests` (
           `id` bigint unsigned AUTO_INCREMENT,
           PRIMARY KEY(`id`)
       );

     ~ applies
       ALTER TABLE `applies` ADD COLUMN `caller` varchar(255) NOT NULL DEFAULT '';

     - legacy_checks
       DROP TABLE `legacy_checks`;

⚠️ Unsafe Changes Detected:
  1. legacy_checks: DROP TABLE destroys data

📋 Plan: 1 table to create, 1 table to alter, 1 table to drop

storage apply — the same statement, refused

╭─────────────────────────────────────────────╮
│  MySQL Schema Change Apply                  │
│                                             │
│  Database: schemabot on db-1.example        │
│  Schema: the schema embedded in v1.4.0      │
╰─────────────────────────────────────────────╯

     ~ applies
       ALTER TABLE `applies` ADD COLUMN `caller` varchar(255) NOT NULL DEFAULT '';

     - legacy_checks
       DROP TABLE `legacy_checks`;

📋 Plan: 1 table to alter, 1 table to drop

⛔ Apply blocked: 1 unsafe change(s) detected
  1. legacy_checks: DROP TABLE destroys data

🚨 To proceed with these destructive changes, re-run with --allow-unsafe:

  schemabot storage apply --allow-unsafe

The refusal is written after the summary, which is where apply writes its own: it carries the command an operator copies, so it belongs last on screen. A plan's disclosure stays above the summary, again matching plan.

storage plan — a change that needs manual remediation

╭─────────────────────────────────────────────────────╮
│  PostgreSQL Schema Change Plan                      │
│                                                     │
│  Database: schemabot on pg-1.example                │
│  Schema: the schema files in ./pkg/schema/postgres  │
╰─────────────────────────────────────────────────────╯

     ~ applies
       ALTER TABLE applies ADD COLUMN caller varchar(255) NOT NULL;

⚠️ Needs manual remediation; nothing converges until these are resolved:
  1. applies: a NOT NULL column with no DEFAULT cannot be added to a table that already has rows

📋 Plan: 1 table to alter

Invariants

• AV-9, upholds. Rendering only. Every statement is printed with its disposition stated beneath it, so what the convergence will run is described by the report rather than widened by how it is displayed.
• UX-4, upholds. Every disposition names its next action: a blocked apply prints the command that grants the consent it refused, and where a manual entry blocks the set instead, the remediation is named rather than a flag that would permit the DROP and converge nothing.

Opened by Claude Code (Opus 5).

[Copilot]

🟡 Changes recommended

Critical unresolved issues prevent reliable CLI behavior and accurate reporting of gated changes.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR adapts storage-schema plan and apply output to the shared CLI templates for MySQL and PostgreSQL.

Changes:

• Extends shared templates with engine/schema labels and change notices.
• Adds storage schema rendering and convergence guidance.
• Adds rendering scenario tests.

File summaries

File	Review summary
pkg/cmd/internal/templates/plan.go	Adds optional labels and configurable change notices.
pkg/cmd/commands/storage_schema_render.go	Implements storage rendering; unresolved critical issues include missing CLI wiring and incorrect display of manually gated changes. Additional summary, hint, environment, and error-context issues remain.
pkg/cmd/commands/storage_schema_render_test.go	Adds coverage for storage rendering scenarios and summaries.

Review details

Suppressed comments (7)

pkg/cmd/commands/storage_schema_render.go:90

• This bare return drops the context that the failure occurred while rendering the outstanding section; the same helper is used for multiple report lists. Wrap the error so a caller can distinguish which storage-schema section failed.

		return err

pkg/cmd/commands/storage_schema_render.go:96

• This bare return drops the context that the failure occurred while rendering the destructive section. Wrap the error so the command reports which storage-schema section could not be rendered.

		return err

pkg/cmd/commands/storage_schema_render.go:171

• The planned-report rendering error is returned without context, so callers cannot tell whether the preview or the remaining-report rendering failed. Wrap this branch with its operation.

			return err

pkg/cmd/commands/storage_schema_render.go:185

• The remaining-report rendering error is returned without context, so a convergence failure is not identified as the remaining-state report. Wrap this branch with its operation.

	})

pkg/cmd/commands/storage_schema_render.go:303

• This hint is not valid for --schema-dir: that path can point at an unreleased checkout, and its report source is the schema files in <path>, not a published release with a matching binary. The current wording nevertheless tells the operator to run “that release's binary”, which can name a nonexistent or mismatched next step; carry the source kind into the renderer and emit directory-specific guidance to preserve UX-4.

func storageSchemaPlanHints(report *apitypes.StorageSchemaReport) []string {
	return []string{fmt.Sprintf("These are what %s needs in order to match %s. To converge them, run that release's binary against this database — its container image is that release — or let the release's first boot converge them.", storageSchemaHeaderDatabase(report), report.SchemaSource)}

pkg/cmd/commands/storage_schema_render.go:185

• A manual-only apply returns the planned report unchanged before any destructive statement exists, but this unconditional hint still says that a destructive statement was refused and recommends --allow-destructive. That is false for PostgreSQL manual remediation and can send the operator toward an irrelevant, potentially unsafe retry; choose the hint from the remaining report's actual Manual and Destructive sets.

	return writeStorageSchemaBody(remaining, true, []string{
		"These were not run. A destructive statement is refused unless --allow-destructive is passed; a manual entry has to be resolved by hand before anything else converges.",
	})

pkg/cmd/commands/storage_schema_render.go:182

• On the withPlan == false apply path, this is the only line identifying the target, but storageSchemaHeaderDatabase omits report.Environment. A staging and production report for the same deployment/database can therefore produce indistinguishable Ran ... against ... output. Include the environment in the target label here, or render the environment heading whenever no plan header was printed.

	fmt.Printf("✓ Ran %d %s against %s.\n\n",
		applied, pluralStatements(applied), storageSchemaHeaderDatabase(planned))

• Files reviewed: 3/3 changed files
• Comments generated: 3
• Review effort level: Lite

---

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

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1407, 05e29e2.
Verdict: 4 findings — 3 non-blocking (gated-list DDL blob, refusal rendered as success), 1 suggestion.

Both finder lenses completed. Adversarial verification was capped at 8 candidates, so 1 lower-ranked candidate was never verified either way and is not reported here.

Non-blocking

The gated list feeds reason-less Outstanding statements into storageSchemaNotices, so a PostgreSQL missing-table entry prints a whole schema file as one list item. At storage_schema_render.go:111 the gated branch hands the Outstanding set to the notice builder, but a create_table entry carries DDL: files[table] — the entire .sql file — and no Reason (storage_schema.go:288). Reproduced with an overlay probe: 1. checks: is followed by ~40 unindented lines of CREATE TABLE + CREATE INDEX, collapsing the numbered list and burying the manual-remediation section under it. The non-gated path avoids this by routing DDL through formatProgressDDLForDialect/IndentSQL; the gated path has no equivalent.

The DDL fallback itself is the mechanism, and no test covers it. storage_schema_render.go:291 sets reason = statement.DDL on the assumption (stated in its own comment) that every destructive/manual statement carries a reason, which create_table does not, and plan.go:590 prints it with a single fmt.Printf(" %d. %s: %s\n", …). ui.LintReasons only splits on "; ", so newlines pass through untouched and any DDL containing "; " additionally renders as several bogus numbered entries. Every test in the diff uses one-line DDL, so none of them catch it.

An apply that was refused outright still headlines with a success check mark, and with withPlan=true the gated/manual body prints twice. api.ApplyStorageSchema returns planned, planned, nil when len(planned.Manual) > 0 (storage_schema.go:119), so storage_schema_render.go:223 prints ✓ Ran 0 statements against … for a convergence that ran nothing — the repo's own vocabulary gives a refusal ⛔, not ✓ (glyph.go:36-41) — and the identical report is rendered on both sides of that line. Verdict is PLAUSIBLE rather than CONFIRMED only because outputStorageSchemaConvergence has no non-test caller at this SHA, so the operator-visible outcome waits on a wiring commit.

General suggestions

pluralStatements re-implements ui.Pluralize. storage_schema_render.go:303 duplicates format.go:337 exactly, and this same package already calls it (ui.Pluralize("PR", n) at checks_backfill.go:330), so there is no import-cycle or layering reason for a private copy. AGENTS.md:301/324 discourage exactly this.

The one thing that could have broken, verified

templates.WritePlanHeader now branches on showSchemaName := data.SchemaName != "" && (data.IsMySQL || data.EngineLabel != "") and honours new EngineLabel/SchemaLabel overrides — a change inside the header every existing plan rendering goes through. Grepping pkg/ for both fields shows no caller outside the new storage path sets either one, so EngineLabel == "" holds everywhere else and the new condition reduces to the old IsMySQL behaviour: a no-op for every pre-existing caller.

Verified correct

• storageSchemaChangeType covers the full server operation vocabulary (create_table/add_column/create_index, alter_table/drop_table) and errors instead of defaulting on anything else.
• PostgreSQL create_index statements carry the table name, not the index name (storage_schema.go:286), so the per-table summary cannot double-count an index as a second table.
• storageSchemaRunnable pre-sizes with make([]DDLChange, 0, len(outstanding)+len(destructive)), so appending destructive cannot alias or clobber the caller's slice; summaryTables only reads.
• The gated notice's append(append([]apitypes.StorageSchemaStatement{}, …)) starts from a fresh slice, so it cannot write into report.Outstanding's backing array.
• MySQL summary and SQL sections agree: combineAlterStatements groups by table and storageSchemaSummaryTables collapses by (table, kind), so a split mixed ALTER is one section and one table.
• schema.Dialect(report.Dialect) round-trips the wire strings "mysql"/"postgres" correctly, as TestStorageSchemaEngineLabel pins.
• Nil-report deref is unreachable from the real server: handleStorageSchemaApply 500s on a response missing either half before the renderer sees it.

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

[Kiran01bm]

🤖 Approved on Kiran's (@kmuddukrishna) behalf by the scheduled review agent — no blocking findings at 05e29e2. See the review comment above; non-blocking findings and suggestions, if any, are not merge gates.

[aparajon]

🤖 All four addressed in cff35028, and the branch is now rebased onto 7cb9fccc.

The gated list feeds reason-less Outstanding statements into storageSchemaNotices, so a PostgreSQL missing-table entry prints a whole schema file as one list item. Fixed, and chasing it turned up something worse than the DDL blob. The reason = statement.DDL fallback is gone entirely — a statement with no reason is now named by what it would do (create table, add column), which keeps an entry to one line — and WriteChangeNotice prints the reason as written instead of routing it through ui.LintReasons.

That last part is the real defect underneath your finding. ui.LintReasons splits on "; ", which is correct for a lint reason because that is a concatenation of findings, but a reason written about one change is a sentence. The live PostgreSQL manual reason is:

definition is NOT NULL without a DEFAULT; add it manually or ship the column with a DEFAULT

...which rendered as two numbered findings, the second being the remedy for the first. So the operator was told to fix two things, one of which was the fix for the other. There is now a test pinning that reason unsplit, and one pinning one line per statement with no CREATE TABLE in the gated list.

The DDL fallback itself is the mechanism, and no test covers it. The mechanism is deleted rather than tested, which I think is the right disposition — your note that newlines pass through LintReasons untouched while "; " inside DDL renders as bogus entries describes two independent ways for the same fallback to produce garbage.

An apply that was refused outright still headlines with a success check mark, and with withPlan=true the body prints twice. Both fixed. A refusal gets ⛔ per the repo's own vocabulary and says no statements ran; the body is printed once. Your PLAUSIBLE-not-CONFIRMED verdict was the correct call at that SHA — the wiring commit that makes it operator-visible is #1395 — and there is now a test asserting strings.Count(out, "Needs manual remediation") == 1 in both withPlan modes.

pluralStatements re-implements ui.Pluralize. Deleted; it calls ui.Pluralize.

One note on the shape of the fix: adding a sibling WriteReasonNotice beside WriteChangeNotice would have left a dead export, since WriteChangeNotice was introduced by this same stack and had no other caller (git log -S confirms). Replacing its body was the right move rather than shipping a twin.

---

Replied by Claude Code (claude-opus-5) on Armand's behalf.

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1407, e296dca.
Verdict: 2 findings — 2 non-blocking (duplicate refusal output on mixed convergence runs).

Non-blocking

The duplicate-refusal guard is conditioned on applied == 0, so a mixed run prints "Apply blocked" twice. At storage_schema_render.go:237 the dedup branch only fires when nothing ran, but AppliedStatements() returns Outstanding whenever !DestructiveAllowed, so a convergence that applies an ALTER and refuses a DROP has applied == 1 and falls through to writeStorageSchemaBody(remaining, …). Running that case against head printed Apply blocked and schemabot storage apply --allow-unsafe twice. The existing test only covers applied == 0, the one path the guard does cover.

The if withPlan { return nil } dedupe is nested inside the same applied == 0 branch, so unattended mixed runs report one refusal twice. At storage_schema_render.go:242 the planned body already emits the blocked block and re-run hint; with applied = 1 the return is skipped, so "✓ Ran 1 statement" prints and then line 251 re-renders the same destructive statement. Executing the scenario yielded BLOCKED_COUNT=2 RERUN_COUNT=2 RAN_COUNT=1 — an operator sees "Apply blocked" above a line saying a statement ran. Neither withPlan=true test hits this combination.

Both are non-blocking only because the function has no production caller at this head, so the duplication is latent; wire-up should fix the guard first.

The one thing that could have broken, verified

The refactor of WriteUnsafeChangesBlocked into a caller-supplied rerun string was the riskiest mechanism: the caller at apply.go:399 builds apply -s %s -e %s --allow-unsafe, which reproduces the old printf byte for byte, and the TEMPLATES drift check agrees.

Verified correct

• preview_lint.go:30,41 — both preview callers pass the values the old signatures formatted; TEMPLATES.md previews unchanged.
• plan.go:593 — WriteUnsafeWarningAllowed renders the old literal "🚨 Unsafe Changes (--allow-unsafe enabled)" exactly.
• plan.go:63 — showSchemaName only widens when EngineLabel is set, and no existing caller sets it, so MySQL and Vitess headers render as before.
• plan.go:611,632 — list and count both route through unsafeChangeFindings, so the heading's N matches the lines below it in both forms.
• plan.go:626 — the PR comment path uses its own UnsafeChangeData and no producer sets Reasons, so engine plans count identically on both surfaces.
• apitypes.go:854 — UnsafeChange is a derived view from PlanResponse.UnsafeChanges(), not a documented payload field, so adding Reasons does not trip the docs sync rule.
• storage_schema_render.go:191 — storageSchemaSummaryTables dedupes by (table, kind), counting the PostgreSQL multi-statement-per-table case as one table, as its test asserts.

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

[Kiran01bm]

🤖 Approved on Kiran's (@kmuddukrishna) behalf by the scheduled review agent — no blocking findings at e296dca. See the review comment above; non-blocking findings and suggestions, if any, are not merge gates.

[Kiran01bm]

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

Verdict: 2 findings — 1 non-blocking (refusal printed twice on a mixed run), 1 suggestion (renderer has no caller).

Non-blocking

The "one refusal reported twice" guard only covers applied == 0, so a mixed run prints the whole Apply-blocked block twice. The dedup lives under if applied == 0 { at storage_schema_render.go#L237, but AppliedStatements() returns Outstanding when !DestructiveAllowed, so an apply that ran one additive statement and refused one DROP falls through to a second writeStorageSchemaBody(remaining, …) at #L242. Executing the renderer with planned = {1 outstanding, 1 refused DROP} and remaining = {same DROP}, withPlan=true, printed "⛔ Apply blocked: 1 unsafe change(s) detected", the numbered finding and the copyable --allow-unsafe command 2× each — exactly what the comment at L243-245 says must not happen. TestOutputStorageSchemaConvergence_RefusedRunsSayNothingRan asserts this property only for applied == 0, so nothing catches it. (Reported independently by two finders.)

General suggestions

Nothing in the CLI reaches this renderer — it ships as test-only code. outputStorageSchemaPlan, outputStorageSchemaConvergence and storageSchemaDatabaseLabel have no non-test callers, and storage.go declares only resync-identity-sequences and canonicalize-identity-keys under the single registered StorageCmd — there is no storage plan / storage apply. Until the wiring PR lands, section ordering, the rerun string and the hint text are validated only against hand-built reports, never a real StorageSchemaApplyResponse. Worth confirming the follow-up is imminent, or the ordering assumptions may drift.

The one thing that could have broken, verified

Changing the WriteUnsafeChangesBlocked signature could have silently altered the long-standing apply output that the CLI GIF and TEMPLATES drift checks pin. All three non-test callers were updated (apply.go#L399, preview_lint.go#L30, storage_schema_render.go#L137), and the string apply.go now builds (apply -s %s -e %s --allow-unsafe) reproduces the old hard-coded line byte for byte — Templates drift CI is green.

Verified correct

• WriteUnsafeWarningAllowed: UnsafeConsentAllowFlag renders the identical 🚨 Unsafe Changes (--allow-unsafe enabled); both call sites updated.
• The dropped database parameter was already unused; blockUnsafeApply still passes its own database to OutputPlanResult, so no caller lost information.
• unsafeChangeFindings keeps writeUnsafeChangesList and countUnsafeFindings reading a change identically, so the heading count always equals the numbered lines — declared-Reasons and split-Reason paths alike.
• The "must agree with pkg/webhook/templates.countUnsafeFindings" comment still holds: the webhook side uses UnsafeChangeData (no Reasons field) and no producer feeds both surfaces.
• ui.LintReasons("") returns nil, so a reasonless notice falls through to the ChangeType line instead of printing an empty finding.
• WritePlanHeader's widened showSchemaName cannot change Vitess/MySQL output: every pre-existing caller leaves EngineLabel empty, and width and print share the same schemaLabel.
• storageSchemaEngineLabel never returns empty (defaults to "Storage"), so the storage header always shows its Schema row and overrides the Vitess/MySQL title.

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

[Kiran01bm]

🤖 Approved on Kiran's (@kmuddukrishna) behalf by the scheduled review agent — no blocking findings at e279115. See the review comment above; non-blocking findings and suggestions, if any, are not merge gates.

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1407, 98ac2c6.
Verdict: 4 findings — 1 non-blocking (one refusal reported twice), 3 general suggestions.

Non-blocking

The duplicate-refusal guard in outputStorageSchemaConvergence only covers the nothing-ran case, so a partly-applied convergence prints the same refusal twice. The suppression is nested under if applied == 0 { … if withPlan { return nil } } (storage_schema_render.go#L237, #L242), so with Outstanding=[ALTER … caller], Destructive=[DROP TABLE stale_state], DestructiveAllowed=false and withPlan=true, applied=1 skips it and line 251 re-enters writeStorageSchemaBody(remaining, true, …). A probe run emitted "⛔ Apply blocked: 1 unsafe change(s) detected" and the --allow-unsafe re-run command twice in one run — exactly what the comment at line 244 says it avoids. Cosmetic today (no production caller passes withPlan=true at this head), and the existing tests miss it: the exactly-once assertion only covers applied==0 and the left-behind test uses withPlan=false.

General suggestions

Dead code shipped with its own test: storageSchemaDatabaseLabel has no production caller. git grep at this head returns only the definition (#L356), the prose mention at line 46, and four _test.go assertions — storageSchemaHeaderDatabase is the one actually wired into writeStorageSchemaHeader. Its deployment clause also appends a dangling "in " when Environment is empty ("schemabot (postgres), deployment west in "), latent only because resolveStorageSchemaTarget rejects that pairing server-side. Either wire it or drop it with its test.

The gated-changes heading is hard-coded to glyph.Attention even on an apply that already refused those statements. On an apply with a manual entry, #L113 renders "⚠️ Gated behind the manual remediation below…" directly above "⛔ Needs manual remediation…", because the manual heading at line 148 takes the isApply branch and the gated one does not. glyph.go says Attention means "nothing has been refused yet", so one run mixes both severities for the same refusal. Judgment call — the same file's "Refused attaches to the refusal, never to what was refused" rule also supports leaving it.

The copyable re-run command is not last on screen on the convergence path. The blocked-apply refusal is deliberately held until after the summary because "it belongs last on screen" (#L132-L135), but outputStorageSchemaConvergence passes a non-nil hint slice at line 251, so writeStorageSchemaHints prints a paragraph after schemabot storage apply --allow-unsafe (#L161). The only ordering assertions come from the plan path with nil hints, so nothing pins it.

The one thing that could have broken, verified

Re-signaturing WriteUnsafeChangesBlocked to take a rerun string and drop database is the riskiest edit here — it changes the operator-copyable remediation line on every unsafe apply. The new rerun reproduces the old output byte-for-byte for apply ("apply -s %s -e %s --allow-unsafe" formatted into " %s %s"), the dropped database parameter was unused in the old body, and a repo-wide grep found every caller already swept: apply.go:185/399, preview_lint.go:30/41, plus tests — no e2e or integration caller left on the old signature, with those build-tag lint jobs green.

Verified correct

• storageSchemaChangeType covers the full producer vocabulary (create_table, alter_table, drop_table, add_column, create_index); an unmapped op errors rather than defaulting to ALTER.
• WriteUnsafeWarningAllowed is unchanged for apply: UnsafeConsentAllowFlag = "--allow-unsafe enabled" reproduces the previous literal heading exactly.
• WritePlanHeader's showSchemaName gate and the schemaLabel/dbType defaults only widen behaviour for callers setting the new fields; MySQL/Vitess headers untouched.
• unsafeChangeFindings backs both writeUnsafeChangesList and countUnsafeFindings, so the heading count always matches the lines beneath it, including declared Reasons.
• apitypes.UnsafeChange is a derived view (PlanResponse.UnsafeChanges()), not a wire or proto type, so adding Reasons cannot break API or proto round-trips.
• The webhook comment path uses its own templates.UnsafeChangeData and is unaffected by the Reasons addition.
• storageSchemaSummaryTables dedupes on (TableName, ChangeType) and the PostgreSQL drift map is keyed by table, so a create_index cannot inflate the "tables to alter" count.

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

[Kiran01bm]

🤖 Approved on Kiran's (@kmuddukrishna) behalf by the scheduled review agent — no blocking findings at 98ac2c6. See the review comment above; non-blocking findings and suggestions, if any, are not merge gates.

[aparajon]

🤖 Four commits since the last round: dd055cef, b7de1ede, 682ae14d and 205acb64. The branch has been rebased onto main as #1404, #1405 and #1406 merged, so the SHAs you reviewed are orphaned; the last commit is new work and is not from review.

A mixed run printed one refusal twice. Fixed in dd055cef. The dedupe was nested under applied == 0; it is now hoisted out, so the withPlan path returns after the count line whatever ran — the count of what ran is the only new fact on that path, and the refusal below it is not. The new test drives exactly your scenario (planned = one outstanding ALTER plus one refused DROP, remaining = the DROP) and asserts one Apply blocked and one copyable re-run command in both withPlan modes.

storageSchemaDatabaseLabel — kept, with the dangling clause fixed. I deleted it as dead code, then restored it in b7de1ede: it has no production caller at this head, but #1395 wires it at three call sites, so the deletion would have broken the child branch. The dangling "in " you spotted is a real defect and is fixed — deployment without environment now renders , deployment west rather than , deployment west in — and the test covers that pairing.

The copyable command was not last on screen on the convergence path. Fixed in 682ae14d. The refusal is held until after the summary precisely because it ends with the command an operator copies, but the attended convergence path passes a hint paragraph and the hint printed after it. Hints now print before the refusal, so the command is last on every path that prints one, and the plan paths — which pass no refusal — render byte for byte as before. Pinned by an assertion on the last non-empty line; restoring the old order fails it.

The plan's own hint is gone, in 205acb64. Not a review finding — it came out of reading the rendered output, and the reasoning is worth stating because it is about what this command is for. storageSchemaPlanHints printed a paragraph under every non-converged plan, and both of its sentences were wrong for the command printing them:

• The first restated the header. It named the database and the schema source that the box directly above already carries, under a summary line that already counts the tables.
• The second named the release's boot as the way to converge what the plan listed. Converging the storage ahead of the roll — from the new release's binary, before any pod boots on it — is what this CLI exists to do. Boot-time convergence is the server's concern, not a next step to hand an operator who is holding the command that does it earlier and under supervision.

It was also wrong on half of its own input. The hint was one format string over report.SchemaSource, so a plan run with --schema-dir — the selector that exists for a commit that was never tagged — told the operator to run "that release's binary" when they had named a directory and there was no release. Nothing caught it: TestStorageSchemaPlanHints only ever fed it a release-shaped source.

The hints parameter stays. outputStorageSchemaConvergence still passes one, and that one is about what just happened rather than a next step to take. TestOutputStorageSchemaPlan_Converged now passes an explicit hint and asserts it is suppressed, so the converged-plan rule is still covered by something that would fail if it broke.

The gated-changes heading's glyph — left as Attention. Both readings are defensible and you named the rule that decides it: "Refused attaches to the refusal, never to what was refused". The gated statements are not themselves refused, they are unreachable until the manual entry below them is resolved, and that entry carries Refused on an apply. Changing the gated heading would put the refusal glyph on statements the run has no verdict on yet.

Replied by Claude Code (claude-opus-5) on Armand's behalf.

[aparajon]

🤖 The plan was still doing its own thing with what it would not run — it no longer does

Three pieces of bespoke rendering are gone. A destructive statement reached the SQL section only when consent was already in effect; a manual remediation pulled every other statement out of it into a numbered "gated" list that existed nowhere else in the CLI; and neither counted in the summary, so a refused plan printed no 📋 Plan: line at all. plan does none of that. It prints the whole difference between the two schemas, warns underneath about the part it will refuse, and counts all of it.

A plan against storage carrying a surplus table:

Before                                   After

╭─────────────────────────────╮          ╭─────────────────────────────╮
│  MySQL Schema Change Plan   │          │  MySQL Schema Change Plan   │
╰─────────────────────────────╯          ╰─────────────────────────────╯
                                              - check_gate_audit
  ✗ no SQL section at all                       DROP TABLE `check_gate_audit`;

⚠️ Unsafe Changes Detected:               ⚠️ Unsafe Changes Detected:
  1. check_gate_audit: Unsafe …            1. check_gate_audit: Unsafe …

  ✗ no summary line                      📋 Plan: 1 table to drop

storageSchemaRunnable is deleted with it: the summary now counts the difference, which is what every other summary in this CLI counts.

Two things kept deliberately, both in e7b12af and 0873816:

• Each disposition renders its own WriteSQLChanges call. One combined call would be wrong on MySQL, where the formatter merges a table's alters into a single statement: an ALTER split so its safe half could run would be recombined into a statement nothing is going to run, hiding the split the refusal exists to make. TestOutputStorageSchemaPlan_SplitAlterStaysSplit pins it.
• A manual entry still outranks the destructive refusal. It blocks the whole set, so --allow-unsafe would permit the DROP and converge nothing. The destructive statement is now disclosed with the standard ⚠️ heading instead of being silent, but the blocked-apply block and its re-run command stay suppressed while a manual entry is present.

The one local helper left on the plan path is storageSchemaSummaryTables, which collapses the summary to one entry per table and kind. PostgreSQL emits an add_column per missing column, so without it a table short two columns reads as "2 tables to alter". It cannot move into WritePlanSummary: Vitess legitimately has the same table name in several keyspaces and has to keep counting them separately.

The PR summary's previews are regenerated from the renderer, not hand-edited. CI is green.

Replied by Claude Code (claude-opus-5) on Armand's behalf.