feat(cli): say which storage DDL is still outstanding, and converge it

[aparajon]

What this adds

SchemaBot runs schema changes against your databases, and it has a database of its own: the bookkeeping storage holding plans, applies, checks, leases and locks. Its schema is converged by every release at startup. These two commands let an operator ask what a release will converge, and converge it ahead of time:

schemabot storage plan  --release v1.4.0 --deployment west -e production
schemabot storage apply --deployment west -e production

storage plan is read-only — no DDL, no advisory lock — so it is safe against production with an apply in flight, which is when it is needed. storage apply converges by calling the same startup bootstrap a booting pod calls, and without -y it previews what it would run and stops, so the preview and the convergence are the same code reading the same database.

It inherits the bootstrap's five-minute budget along with the code, since that budget is fixed inside the bootstrap rather than passed in by the caller. So converging ahead of a roll takes the work out of the roll but not out of the budget: DDL too slow to finish during a boot is still too slow here, and still has to be run by hand. Giving the deliberate path a budget of its own is follow-up work.

They render as a plan and an apply because that is what they are: the same header box, the same +/~/- change symbols and the same summary line as schemabot plan. Only the target differs.

The desired side of a plan is always a release you name. --release <tag> fetches that tag's schema files, --schema-dir <path> reads a checkout, there is no default, and naming both is refused rather than resolved by precedence. One live database gives different answers against different releases, so a plan whose desired side you did not choose is unusable, not merely weaker. (storage apply here converges the answering binary's own schema; #1413 gives it the same selectors, behind a terminal confirmation.)

A destructive statement blocks an apply, exactly as it blocks a schema change apply. --allow-unsafe permits it; without it nothing converges and the command exits non-zero rather than running the additive remainder and reporting success. The gate sits in front of the confirmation prompt, so -y cannot skip it: consenting to run an apply is not consenting to destroy state. A manual-remediation entry outranks it, since that blocks the whole set and makes a destructive statement unreachable rather than merely refused.

A booting pod deliberately does the opposite — it skips the refused statement and converges the safe remainder, because refusing to boot over surplus state left by a rollback would take the deployment down. At a terminal there is someone to decide, so the command stops and lets them.

  storage plan / storage apply
     │
     ├── --dsn <dsn>                ─▶ that database, dialect read from the DSN
     ├── (server config file)       ─▶ this server's own storage, connected
     │                                 directly — for when it will not boot
     └── --deployment <d> -e <env>  ─▶ that data plane's storage, over gRPC

  storage plan    0  converged      2  outstanding      1  read failed
  storage apply   0  converged, or the prompt was declined
              non-0  refused (destructive, or manual remediation),
                     unreadable, or the DDL failed

Before and after

An operator runs storage apply against a storage database that is short a caller column on applies and carries a surplus legacy_checks table left behind by a rolled-back release.

Before                                   After

┌──────────────────────────────┐         ┌──────────────────────────────┐
│ plan                         │         │ plan                         │
│ 1 table to alter             │         │ 1 table to alter             │
│ 1 destructive, refused       │         │ 1 destructive, refused       │
└──────────────┬───────────────┘         └──────────────┬───────────────┘
               │                                        │
               ▼                                        ▼
┌──────────────────────────────┐         ┌──────────────────────────────┐
│ prompt answered yes          │         │ blocked before the prompt    │
│ ALTER ran, DROP left behind  │         │ nothing ran                  │
└──────────────┬───────────────┘         └──────────────┬───────────────┘
               │                                        │
               ▼                                        ▼
┌──────────────────────────────┐         ┌──────────────────────────────┐
│ exit 0                       │         │ exit 1                       │
│ the refusal scrolled past,   │         │ the refusal is last on       │
│ under a successful exit      │         │ screen, naming the flag      │
└──────────────────────────────┘         └──────────────────────────────┘
     ✗ a half-converged storage              ✓ nothing ran, and the
       reads as success                        operator decides

The command was also called storage diff, and printed a rendering written only for it.

Before — storage diff --release v1.4.0

$ schemabot storage diff --release v1.4.0
schemabot on db-1.example (mysql) needs 3 statements: 3 outstanding, against the schema files of release v1.4.0 in block/schemabot.

Outstanding, and run automatically on the next boot or apply (3):

ALTER TABLE `applies` ADD COLUMN `driver_note` varchar(255) NOT NULL DEFAULT '' AFTER `lease_owner`;
ALTER TABLE `checks` ADD COLUMN `blocked_reason` varchar(64) NOT NULL DEFAULT '' AFTER `state`;
CREATE TABLE `check_gate_audit` (
  `id` BIGINT UNSIGNED AUTO_INCREMENT,
  `check_id` BIGINT UNSIGNED NOT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci;

After — storage plan --release v1.4.0 (exit 2)

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

     + check_gate_audit
       CREATE TABLE `check_gate_audit` (
           `id` bigint unsigned AUTO_INCREMENT,
           `check_id` bigint unsigned NOT NULL,
           PRIMARY KEY(`id`)
       ) ENGINE InnoDB,
         CHARSET utf8mb4,
         COLLATE utf8mb4_0900_ai_ci;

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

     ~ checks
       ALTER TABLE `checks` ADD COLUMN `blocked_reason` varchar(64) NOT NULL DEFAULT '' AFTER `state`;

📋 Plan: 1 table to create, 2 tables to alter

After — storage apply blocked by a destructive statement (exit 1)

$ schemabot storage apply --deployment west -e production
╭─────────────────────────────────────────────────────────╮
│  MySQL Schema Change Apply                              │
│                                                         │
│  Database: schemabot on db-1.example (deployment west)  │
│  Schema: the schema embedded in v1.4.0                  │
╰─────────────────────────────────────────────────────────╯

Production
     ~ 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 --deployment west -e production --allow-unsafe

The offered command addresses the same target the blocked run did, so it is safe to copy. A --dsn run is the exception: the flag is named rather than repeated, because a storage DSN carries the credentials to SchemaBot's own state and does not belong echoed to a terminal.

Invariants

• AV-9, upholds. storage apply accepts no schema source, so a convergence always runs the schema of the binary running it. The destructive gate runs strictly less than the convergence it stops, and grants no permission the deployment's own storage policy had not already granted.
• AZ-5, upholds. Routing is decided by raw flag presence, never by trimmed content or an environment variable, so a malformed direct connection fails closed instead of quietly reporting a different database.
• UX-4, upholds. Every refusal names the next action: the blocked apply prints the command that grants consent, addressed at the target it was refused against.

Opened by Claude Code (Opus 5).

[Copilot]

🟡 Changes recommended

Unresolved target-routing, security, timeout, cancellation, and convergence-reporting issues remain.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds storage diff and storage apply commands for inspecting and converging storage schemas through direct databases or API routes.

Changes:

• Adds schema-source selection, rendering, and exit status handling.
• Adds direct/API target resolution and storage schema client methods.
• Adds tests for command behavior and schema fetching.

File summaries

File	Description
pkg/cmd/main.go	Registers storage commands and custom exit-code handling.
pkg/cmd/commands/storage.go	Resolves direct storage targets and dialects.
pkg/cmd/commands/storage_schema.go	Implements diff/apply behavior and output.
pkg/cmd/commands/storage_schema_test.go	Tests command behavior and rendering.
pkg/cmd/commands/storage_schema_source.go	Loads release or checkout schema sources.
pkg/cmd/commands/storage_schema_source_test.go	Tests schema-source resolution and fetching.
pkg/cmd/commands/exit_code.go	Defines outstanding-schema exit statuses.
pkg/cmd/client/request.go	Supports long-running storage operations.
pkg/cmd/client/client.go	Adds storage schema API methods.

Review details

Suppressed comments (10)

pkg/cmd/client/request.go:35

• The API-routed apply is synchronous and calls the startup bootstrap, whose EnsureSchemaTimeout is five minutes, but the server still uses the global 30-second http.Server.WriteTimeout (pkg/serve/serve.go:218-224). Any valid convergence taking more than 30 seconds will have its response closed before this 15-minute client timeout, so the CLI reports failure even though the bootstrap may continue or complete. Give this route a server-side budget compatible with the bootstrap (or make it asynchronous/pollable); increasing only the client timeout is insufficient.

var operatorHTTPClient = &http.Client{Timeout: 15 * time.Minute, Transport: authTransport}

pkg/cmd/commands/storage_schema.go:352

• The direct convergence path only assigns Version, while ApplyStorageSchema leaves SchemaSource as the schema embedded in this binary. Consequently storage apply --config/--dsn does not identify the running release in its headline or JSON, unlike the API path and the documented output. Attribute both reports with g.Version here, as readDirect already does.

		plannedReport.Version = g.Version
		remainingReport.Version = g.Version

pkg/cmd/commands/storage_schema.go:268

• When --json is used without -y, this preview is rendered as human-readable text to stdout, then the final result is encoded as JSON to the same stream after confirmation. The command therefore emits invalid JSON (and can include the prompt) for exactly the interactive mode a caller would otherwise use. Reject --json without --auto-approve, or move all preview/prompt text off stdout and emit one defined JSON document.

	if !cmd.AutoApprove {
		// No selector on the preview asks the target about its own embedded
		// schema, which is the schema this convergence is about to run.
		preview := &StorageDiffCmd{
			storageSchemaTargetFlags: cmd.storageSchemaTargetFlags,

pkg/cmd/commands/storage_schema.go:347

• Although this call accepts ctx, the convergence path currently ignores it: api.ApplyStorageSchema invokes EnsureSchema, whose MySQL bootstrap creates a new context.Background() deadline and does not receive the caller context. Ctrl+C therefore cannot stop a direct storage apply while DDL is running; it waits for the bootstrap and then the canceled confirmation read reports an error even though the database may already have changed. Thread cancellation through the bootstrap or explicitly document/handle this non-cancelable operation before exposing it here.

		plannedReport, remainingReport, err := api.ApplyStorageSchema(ctx, target.dsn, logger,
			target.ensureSchemaOptions(cmd.AllowDestructive)...)

pkg/cmd/commands/storage_schema.go:327

• storageSchemaConvergenceOutcome returns success whenever Manual is empty, including when the only remaining changes are refused destructive statements. That makes storage apply exit 0 while remaining.Converged is false and the database still differs, contradicting the documented exit-2 outcome for outstanding statements and weakening AV-9's fail-closed operator signal. Return the existing outstanding status for any non-manual, non-converged result; reserve exit 1 for manual remediation or a convergence error.

func storageSchemaConvergenceOutcome(remaining *apitypes.StorageSchemaReport) error {
	if len(remaining.Manual) == 0 {
		return nil

pkg/cmd/commands/storage_schema.go:443

• A manual entry aborts the entire convergence before any statement runs, but this section is always titled as if Outstanding statements will run automatically. A PostgreSQL report can contain both a safe drift and a manual drift, so the preview would tell the operator that the safe statement will run even though ApplyStorageSchema returns before executing anything. That contradicts AV-9's whole-drift gate; use a waiting-on-manual-remediation heading whenever report.Manual is non-empty, including in the post-convergence rendering.

		{storageSchemaOutstandingTitle, report.Outstanding},
		{storageSchemaDestructiveTitle(report), report.Destructive},
		{"Needs manual remediation before anything converges", report.Manual},

pkg/cmd/commands/storage_schema.go:527

• When report.Destructive is present but not allowed, the hint never names the opt-in needed to run it; it only says to run the release binary, whose default convergence still refuses destructive DDL. The operator is therefore not told to add --allow-destructive (or enable the equivalent policy), contrary to UX-4 and the described action. Include the explicit opt-in in the hint for refused destructive statements.

func storageSchemaDiffHints(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.", storageSchemaDatabaseLabel(report), report.SchemaSource)}

pkg/cmd/commands/storage_schema.go:527

• This hint hardcodes a published release and its container image, but --schema-dir is also a supported desired source and can point at an unreleased checkout. In that mode the output tells the operator to run a release that may not exist, which fails UX-4's actionable-remediation requirement. Branch the remediation text on the source type and tell directory users to run/build the binary from that checkout.

func storageSchemaDiffHints(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.", storageSchemaDatabaseLabel(report), report.SchemaSource)}

pkg/cmd/commands/storage_schema.go:162

• The API route wraps read-only diffs in api.StorageSchemaDiffTimeout, but this direct path passes the CLI's signal-only context straight to DiffStorageSchema. A blocked catalog query or server can therefore make storage diff --dsn hang indefinitely even though the shared API contract says a read-only diff is bounded. Use the same timeout around the direct diff while still honoring earlier caller cancellation.

	report, err := api.DiffStorageSchema(ctx, target.dsn, desired, logger,
		target.ensureSchemaOptions(cmd.AllowDestructive)...)

pkg/cmd/commands/storage_schema.go:289

• If the preview contains only destructive statements and --allow-destructive is not effective, this still asks Run these statements? even though the bootstrap will refuse every one and run zero statements. Saying yes therefore confirms a no-op and the preview does not name the flag needed to make the statements runnable; short-circuit this case with an actionable --allow-destructive message/status instead of prompting.

		confirmed, err := confirmAction(
			fmt.Sprintf("\nRun these statements against %s? Only 'yes' will be accepted: ", storageSchemaDatabaseLabel(report)),
			"\nConvergence aborted.",
		)

• Files reviewed: 9/9 changed files
• Comments generated: 5
• 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/1395, 5fb0087.

Verdict: 5 findings — 1 blocking (interactive apply skips the bootstrap), 3 non-blocking, 1 suggestion.

Blocking

Interactive storage apply skips the bootstrap when the catalog is converged, so -y and non--y do different work. storage_schema.go:288 returns nil on report.Converged, never reaching cmd.converge, so ApplyStorageSchema/EnsureSchema never run. Stale engine tables (_checks_new, _spirit_checkpoint) left by a killed pod are invisible to a catalog diff — the live schema loads WithoutUnderscoreTables — so only ensureMySQLSchema drops them (ensure_schema.go:193). This is the exact skip storage_schema.go:121 forbids: "returning here would report the storage clean while leaving them on it".

Non-blocking

The non-PostgreSQL refusal now runs after DSN resolution, so the error names the wrong cause. storage.go:201 resolves the target before checking the dialect, so a dialect: mysql config with no DSN fails with "storage DSN not configured" instead of "the identity sequence resync only applies to postgres". It also fetches a dsn_from secret for a command that will be refused. Every mysql fixture in the tests also sets a resolvable dsn, so nothing catches it.

storage apply mislabels its own schema version. storage_schema.go:361 assigns plannedReport.Version by hand instead of calling AttributeTo, which sets both fields. So apply -y --dsn prints Schema: the schema embedded in this binary while plan and the serve path both print the schema embedded in v1.4.0 — within one run the preview and the post-convergence header disagree.

Neither StorageApplyCmd.Run nor the exit-2 contract of StoragePlanCmd.Run has a test. The new tests cover validate(), direct(), and the leaf helpers only; the sole Run call returns at validateSource. Negating storage_schema.go:140 or dropping the manual-remediation guard passes the whole suite, and rollback_test.go:67 shows the httptest seam already exists.

General suggestions

Mirror --release-repo alongside the other hidden flags on StorageApplyCmd. storage_schema.go:257 hides --schema-dir and --release so the diff's selectors get a reasoned refusal, but not --release-repo. Copy-pasting a plan --release … --release-repo … line and editing plan to apply dies on Kong's bare unknown flag --release-repo before Run executes — exactly the error the hidden fields exist to avoid.

The one thing that could have broken, verified

The resolveStorageDSN → resolveStorageTarget refactor could have dropped one of the old refusals. Each was traced to its new home: --dsn + --config still hits mutual exclusion (storage_target.go:84), whitespace-only --dsn still returns "--dsn contains only whitespace", and a non-PostgreSQL direct DSN still returns the purpose-naming refusal at storage.go:196. Only the ordering of the config-dialect refusal changed — finding 2 above.

Verified correct

• desired.Describe() is nil-safe, so the apply preview's nil source cannot panic.
• report.AttributeTo(g.Version) rewrites SchemaSource only when empty or the embedded description, so an operator-supplied release is not relabelled.
• No nil deref on report.Converged or remaining.Manual; both planners return a non-nil report when err is nil, and the API path nil-checks its fields.
• exitStorageSchemaOutstanding keeps errors.Is(err, ErrSilent) true while ExitCodeFor returns 2; every other error still exits 1.
• usesLocalRuntime reads the right subcommand's flags, and an unknown subcommand defaults to false so maintenance commands never start a runtime.
• The apply preview and the convergence it previews share one target and options policy, so the preview cannot describe a different schema.
• The preview's API path costs no extra round trip: with no source selector, resolve returns (nil, nil) without invoking the callback.

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

[morgo]

🤖 Approving on Morgan's behalf — his AI agent. This is the PR in the stack that refactors code the existing PostgreSQL maintenance commands depend on, so I checked what the refactor dropped before looking at what it adds.

The refactor drops no guard

resolveStorageDSN lost its own --dsn/--config mutual-exclusion check and its own whitespace refusal. Both survive, by falling through rather than by being restated: the new guard is directDSN != "" && configFlag == "", so --dsn with --config and a whitespace-only --dsn both miss it and reach resolveStorageTarget, whose first two checks are exactly those refusals with the same wording. A MySQL DSN passed directly still gets the PostgreSQL-specific refusal naming the operation. Same behaviour, one implementation.

Routing

usesLocalRuntime growing a *CLI parameter to answer storage is the part that could have gone wrong, and the default is the safe one: UsesAPI returns false for any subcommand it does not know, so an unrecognised storage subcommand resolves no endpoint rather than starting a runtime the operator explicitly asked to avoid. TestUsesLocalRuntime_StorageSubcommands covers it.

direct() keying on flag presence rather than content is the right choice and the comment gives the right reason: reading a whitespace --dsn as "no DSN" would silently route through the API and report a different database than the one the operator addressed. That is a worse failure than any error message, and validate() refusing every mixed combination instead of honouring one flag and dropping the other follows the same principle.

Exit status

report.Converged → 0, otherwise exitStorageSchemaOutstanding() → ErrSilent wrapped in &ExitCodeError{Code: 2}. main suppresses the Error: line via errors.Is (which unwraps through ExitCodeError.Unwrap) and exits 2 via errors.As. The three-way split is the right contract for a pre-deploy gate — "converged" and "unreachable" do call for opposite decisions, and collapsing outstanding-work into 1 would make them indistinguishable.

The JSON path takes the same exit status, which is what a script consuming --json needs.

Apply

Confirming against a fresh read rather than a description of the command is the right call for DDL against SchemaBot's own storage, and returning success when the preview comes back converged — rather than prompting for an empty change — is honest. The preview deliberately passing no source selector, so the target reports its own embedded schema, is consistent with the apply actually running that binary's schema.

One gap in the refusal affordance

StorageApplyCmd declares hidden --schema-dir and --release purely so storageSchemaSourceRefusal can explain why they do not apply, instead of Kong emitting a bare "unknown flag". --release-repo is not declared, so:

schemabot storage apply --release v1.4.0 --release-repo block/schemabot-fork

fails at parse time with an unknown-flag error before Run is reached, and the operator never sees the explanation — which is the exact case the hidden flags exist for, since --release-repo is only ever typed alongside --release. Adding it as a third hidden field and ignoring it in the refusal (the --release message is already the right one) would close it. Minor, and not worth holding the stack for.

41/41 green. Note the stack is based on 342f9def, two commits behind current main (7cb9fccc, 5f7e4657) — both tern-side and nowhere near pkg/cmd, so no conflict risk I can see, but worth a rebase before merge so CI has run against what will actually land.

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1395, 6c35a24.
Verdict: 5 findings — 3 non-blocking (destructive gate aborts the whole convergence, --json emits human text, direct path untested), 2 suggestions.

Non-blocking

The destructive gate aborts the whole convergence, so storage apply converges none of the additive DDL that the same binary's boot would have run. storage_schema.go:417 — after a v1.4→v1.3 rollback the plan carries one refused DROP TABLE, and the pre-deploy storage apply --auto-approve exits 1 having done nothing, while a v1.3 pod's own boot refuses only the DROP and converges the safe remainder (ensure_schema.go:398). The only escape, --allow-unsafe, means "run the DROP", not "skip it". Note the asymmetry: the identical leftover destructive state produced during a run is exit 0 per storageSchemaConvergenceOutcome, so the same database gets opposite verdicts based only on when the surplus appeared.

storage apply --json prints the human plan and both gate refusals to stdout and never reaches the JSON encoder. storage_schema.go:301 — blockDestructiveStorageApply calls outputStorageSchemaPlan (ANSI/box-drawing to stdout via fmt.Print*) then returns ErrSilent, exiting before the if cmd.JSON encoder at line 334; the manual gate at line 408 does the same. A scripted consumer's jq fails to parse stdout and cannot tell a refusal from a crashed CLI. StoragePlanCmd.Run honours --json on every exit path, so the two halves of the feature disagree on what the flag promises.

The direct-connection path — the path the PR exists for — has no test at any layer. storage_schema.go:166 — nothing reaches readDirect or the cmd.direct() branch of converge; every new test drives the API path through storageSchemaTestServer, and the one direct-flag test bails in validateSource() before read(). A regression in ensureSchemaOptions wiring, AttributeTo, statement-timeout propagation, or the allowDestructive carry-over would keep CI green. storage_integration_test.go already boots a PostgreSQL storage container, so the harness exists.

General suggestions

storage apply resolves the storage target twice per direct-path run, so storage.dsn_from reads the secret twice. storage_schema.go:474 — the preview calls resolveStorageTarget at line 167 and converge calls it again, with nothing cached in between (no memo on StorageDSN, and secrets.Resolve re-fetches). That is exactly the cost the storageConfig/target() split was introduced to avoid ("reads a secret, which is a call to someone else's system with its own audit trail", storage_target.go:120); if the secret rotates between the two, the convergence runs against a target the operator never previewed. Resolving once in Run and passing the *storageTarget down removes it.

The doc comment for blockDestructiveStorageApply runs into the next one, so it documents the wrong function. storage_schema.go:392 — no blank line after line 391, so lines 372-402 form one comment group attached to blockManualStorageApply at line 403, and blockDestructiveStorageApply at line 416 has no doc at all. go doc -u confirms: the manual gate renders with the destructive gate's AV-9 rationale, and the function that implements AV-9 documents nothing. A blank line at 392 fixes it.

The one thing that could have broken, verified

The destructive gate could have narrowed standing policy — refusing a DROP the config already permits. It does not: the preview and the convergence pass the same AllowDestructive/ensureSchemaOptions, so DestructiveAllowed reflects config || --allow-unsafe identically on the API and direct paths, and a permitted DROP converges untouched.

Verified correct

• usesLocalRuntime's storage branch matches only kong's derived plan/apply; every other subcommand hits default: return false — all six cases plus a control covered in main_test.go.
• storageSchemaSourceRefusal's new repo parameter: swept the tree, the only non-test caller is StorageApplyCmd.Run:277 passing cmd.Repo; no caller left on the 2-arg form.
• resolveStorageDSN preserves the original order and error strings; the only delta is --dsn ' ' --config x now reporting mutual exclusion instead of whitespace — both refusals.
• The storageConfig/target() split keeps AllowDestructiveSchemaChanges and StatementTimeoutOrDefault on the resolveStorageTarget path, and the --dialect-vs-config check moved with the same message and configured.source text.
• No nil dereference on any report path: readThroughAPI rejects a nil Report, converge rejects a half-nil Planned/Remaining pair, and Describe/AttributeTo/APIType are nil-receiver safe.
• Flag matrix in storageSchemaTargetFlags.validate: -e without --deployment, --deployment without -e, --dialect through the API, and --deployment/-e with a direct connection are each refused.
• Whitespace --dsn stays on the direct path because direct() tests presence, not content, so it cannot silently fall back to the API and report a different database than addressed (AZ-5) — pinned by TestStorageSchemaTargetFlags_Direct.

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 6c35a24. See the review comment above; non-blocking findings and suggestions, if any, are not merge gates.

[aparajon]

🤖 The blocking finding is fixed where you said it belonged, the direct path now has tests, and the plan's trailing hint is gone. The branch has been rebased onto main as #1404, #1405 and #1406 merged, and #1407's latest commit is merged in, so the SHAs you reviewed are orphaned.

The one-command path to a destructive bootstrap on a local runtime. Fixed in serve, on #1404, not at the routing line here. A locally hosted server now refuses a request that opts in to destructive statements instead of honoring it, so AZ-6 is upheld rather than amended; the refusal names the way forward, and AZ-6's *Enforced:* line names the adapter. Details are in the #1404 reply, including the five mutations.

The direct path had no test at any layer. Fixed in c9c1e08d. pkg/cmd/commands/storage_schema_integration_test.go drives it against a real, empty PostgreSQL storage database: a convergence creates the whole schema from nothing, the plan that follows agrees it did, a plan against an unconverged database reports the whole schema outstanding with its own exit status, and an attended apply's preview names this build as the schema it is about to converge to. Three mutations, all red: dropping ensureSchemaOptions from the direct convergence, dropping AttributeTo in readDirect, and dropping attributeStorageSchemaConvergence. The second of those initially survived — the attribution is only observable through the preview a named source does not overwrite, which is why the attended case is in there.

direct()'s doc claimed a $SCHEMABOT_CONFIG_FILE fallback that cannot fire. Fixed in the same commit. It is the fallback --config source for the direct-only storage maintenance commands; on plan and apply a direct connection is already chosen by the time the config is read, so it is never consulted. The doc says that now, and keeps the routing rule it was there to state.

The plan no longer ends with a hint, as of 803b1877. Not a review finding — it came out of reading the rendered output. The paragraph under every non-converged plan restated the header box above it, and then named the release's boot as the way to converge what it had just listed. Converging ahead of the roll, from the new release's binary and under supervision, is what this command exists for; boot-time convergence is the server's concern rather than a next step to hand the operator holding the earlier lever. It was also wrong under --schema-dir, where it told an operator to run "that release's binary" for a directory that may be a commit no release was ever cut from. The builder and its test are removed in #1407 (205acb64); the call site here now passes no hint, and the plan ends on its summary line. The convergence path's own hint — what was left behind by a run that already happened — is unchanged.

--json on the refusal paths, the misattached doc comment, and the target resolved twice — fixed downstream, in #1413. All three live in the same function that #1413 rewrites, so fixing them here would land the same edits twice and conflict. At that head: gatePrintsPlan := cmd.AutoApprove && !cmd.JSON, a reportRefusal that encodes the planned and remaining halves as the same report (a refusal ran nothing, so everything the plan found is still outstanding), the two gate doc comments separated, and resolveStorageTarget called once on the flags value and passed down. I would rather not restack those into this PR while it carries approvals at this head — say the word if you would prefer them here.

The destructive gate aborting the whole convergence — left as is, and I think it should change in its own PR. Your argument is the right one and I do not want to wave it off: a boot of the same binary refuses the destructive statement and converges the safe remainder, so a pre-deploy storage apply that aborts runs less than the pod it is preparing for, and the asymmetry with surplus state that appears mid-run gives one database opposite verdicts. The reason not to do it in this PR is that "converge the remainder and report the refusal" changes the exit-status contract, the renderer's mixed-run path and the guide's account of the gate all at once, across three PRs the stack has already approved at their heads. It is a behavior decision worth its own diff and its own review rather than a late commit here, so I am flagging it as a decision rather than making it in this PR.

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

[morgo]

🤖 Re-reviewed at 3bd164f. The branch was rebased onto main (the stack below it landed), so I reviewed the delta against 093c57b9, where I approved on 2026-09-15.

The finding I raised is fixed, correctly. I said then that storage apply -y against a report carrying manual entries hit neither refusal: blockDestructiveStorageApply deferred to a manual refusal that sat inside if !cmd.AutoApprove and so could not run, and with manual entries alone the command went on to converge — a different contract from the attended path.

f1265cce extracts blockManualStorageApply and runs it on both paths, ahead of the destructive gate. Three things I checked rather than assumed:

• The ordering is right, not just different. Manual runs first because it gates the whole drift set, which makes a destructive statement behind it unreachable rather than merely refused — so naming --allow-unsafe there would offer a remedy that does not apply.
• Deleting the Manual branch from blockDestructiveStorageApply is safe, not a dropped guard. Control cannot reach the destructive gate with manual entries present, because the manual gate returned an error first. The removed branch is genuinely dead.
• withPlan keeps the error honest. The message says the entries are listed above, and on the unattended path nothing had listed them; the gate now prints the plan there.

The test is the right test. TestStorageApplyCmd_ManualRemediationOutranksTheDestructiveRefusal is table-driven over attended and unattended, with a report carrying both a destructive statement and a manual entry — the exact combination in the finding. It asserts the entries are printed on both paths, that Apply blocked is not printed, and — the assertion that actually proves the behaviour rather than the wording — that the only route touched is POST /api/storage/schema/plan, so nothing converged either way.

Rest of the delta:

• 796e6af7 adds an integration test driving plan and apply over the direct connection, which was the path with the least coverage.
• 6f134b25 drops the plan's trailing hint. It sits in the else if branch, so --json output is untouched; human rendering only.
• 3bd164fe touches storage_schema_render.go — I confirmed it changes no non-comment line.

The safety properties I verified last time still hold: the engine gates destructive statements independently at pkg/api/ensure_schema.go, and --allow-unsafe only ever widens the target's standing policy.

CI 41/41 SUCCESS.