feat(cli): let `storage apply` converge to a release you name, before it rolls by aparajon · Pull Request #1413 · block/schemabot

aparajon · 2026-09-15T22:29:15Z

Why this matters

The storage a release needs has to exist before the first pod of that release starts, and the pod that would converge it cannot start until it is there. storage apply could not close that gap: a convergence ran the schema embedded in the binary answering the command. A storage index added in a new release otherwise copies the table on every pod in the roll, under a five-minute startup budget.

--release and --schema-dir now work on storage apply as they already do on storage plan, and the files they name are what converges. The boot path is untouched: it names no schema, and no schema is the embedded one.

An operator on CLI v1.2.3, fleet deployed on v1.2.3, converging v1.4.0's storage (which adds applies.driver_note) ahead of the roll:

Before                                   After

┌───────────────────────────────┐        ┌───────────────────────────────┐
│ storage apply                 │        │ storage apply                 │
│ --release v1.4.0              │        │ --release v1.4.0              │
└───────────────┬───────────────┘        └───────────────┬───────────────┘
                │                                        │ fetch v1.4.0's
                ▼                                        ▼ schema files
┌───────────────────────────────┐        ┌───────────────────────────────┐
│ refused: "--release cannot    │        │ plan, then the notice:        │
│ be used with a convergence"   │ ✗      │ "v1.2.3 converges the         │
│                               │        │  difference back"             │
└───────────────┬───────────────┘        └───────────────┬───────────────┘
                │ only way forward                       │ yes
                ▼                                        ▼
┌───────────────────────────────┐        ┌───────────────────────────────┐
│ storage apply, no selector    │        │ converged to v1.4.0           │
│ converges v1.2.3's schema     │        │ driver_note created, before   │
│ driver_note NOT created       │ ✗      │ the first v1.4.0 pod starts   │
└───────────────────────────────┘        └───────────────────────────────┘

The schema is resolved once, in Run, and threaded through the preview, the confirmation, and the convergence, so a tag that moves in between cannot have an operator approve one file set and run another. Two gates sit in front of it:

--auto-approve is refused with a named schema, so a cross-release convergence is never unattended. A pre-deploy job converging with -y and no selector is unchanged.
The confirmation states what a plan cannot show, for the deployment it is talking to — its standing policy and its dialect, never the flags on the command. On MySQL every pod booting the running release refuses to drop what it does not declare, so the pre-applied state survives and each boot logs a refused destructive change for it — a fleet warning about work done on purpose, which reads as an incident if nobody said it was coming. PostgreSQL keeps the state without remarking on it. A deployment that has configured destructive storage changes drops it instead, which is the one case where converging ahead of the roll is the wrong move. Where nobody could read that policy, the notice says so rather than picking one.

A release read from a plaintext $GITHUB_API_URL is now refused on a convergence where a plan only warns, since whatever DDL arrives is what runs. A mirror on loopback is exempt.

storage apply --release v1.4.0 (new)

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

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

📋 Plan: 1 table to alter

Converging schemabot on db-1.example (mysql) to the schema files of release v1.4.0 in block/schemabot.

  This is not the schema v1.2.3 converges on boot. Until that release is deployed,
  every pod that boots v1.2.3 refuses to drop what it does not declare, so the
  tables, columns and indexes applied here survive — and every one of those
  boots logs a refused destructive change for them. A release from before
  indexes were protected is the exception: its boots converge a surplus index
  away. Re-run `storage plan` just before the deploy to confirm what you
  applied is still there.

Do you want to apply these changes to schemabot on db-1.example (mysql)? Only 'yes' will be accepted: yes
✓ Ran 1 statement against schemabot on db-1.example. Nothing is outstanding.

storage apply --release v1.4.0 (previous)

$ schemabot storage apply --release v1.4.0
Error: --release cannot be used with a convergence: an apply runs the schema embedded in the binary running it, so that it converges exactly what that binary's next boot would. To converge a release's schema, run that release's binary — its container image is that release — or let the release's own first boot converge it. To see what it would do, use the same flag on `storage plan`

storage apply --release v1.4.0 --auto-approve (new refusal)

$ schemabot storage apply --release v1.4.0 --auto-approve
Error: --release cannot be combined with --auto-approve: converging storage to a schema this binary does not carry is confirmed at a terminal, because until that release is deployed the running one will not converge the same schema. Run it without --auto-approve and answer the prompt, or drop --release to converge what this binary's own next boot would

From review

--json answers in JSON on every exit — refused, declined, or converged. The gates printed a human plan on the surface a program parses, the destructive gate printed nothing, and a decline exited 0 with an empty stream. The attended path printed the plan and prompt to stdout too, which a named schema forces, since it cannot be paired with --auto-approve. The human half now goes to the terminal and stdout carries the response.
The storage target is resolved once per run. Resolving a storage.dsn_from config re-reads secret references, so three call sites meant three audited reads and a window where a changed value had the convergence run against a database its preview never saw. A completeness test pins the single call site.
A plaintext redirect no longer bypasses the fail-closed fetch. Guarding only the configured base URL left the choice of channel with the remote end, which can redirect an https base to http and supply the DDL that runs. Every hop is now held to the rule the base URL was.
A schema named with no files is refused where both transports reach it. The HTTP handler validated the pair, but the tern gRPC server reaches the adapter directly, where a name alone fell through to the embedded schema and converged it successfully for a caller that asked for another release's.
The notice reported the fleet's behavior from the operator's own flag. It branched on the effective destructive policy, which is the deployment's config widened by the caller's opt-in — so an operator who met the destructive gate and re-ran with the --allow-unsafe the gate itself handed them was told their deployment drops what they are applying and should wait for the deploy. Their fleet does no such thing. A report now carries the two facts apart: the effective policy for this call, and what a boot of this deployment does to surplus state, which no request moves and which is preservation on an additive-only dialect whatever it has configured. The two are different types, so transposing them at a call site is a compile error rather than a silent misreport — a gate reads whether either permits, so every gate would have stayed correct while only the notice went wrong.
The notice said a pre-applied index would be converged away, and spoke for every deployment at once. A boot refuses an index drop with the same verdict a table or column drop gets, so the pre-applied state survives; what an operator needs warning about is the refusal logged on every boot until the deploy. It now branches on the report's dialect and destructive policy: PostgreSQL computes no removal and logs nothing, and a deployment permitting destructive storage changes drops what was pre-applied — the one case where converging ahead of the roll is the wrong move. An integration test converges a named schema declaring a surplus index and boots the running release over it, so the prediction is pinned against behavior rather than against its own wording.

A boot policy nobody could report was rendered as the reassuring one. The field was a bool, so a data plane too old to set it and a deployment that preserves surplus state arrived identically, and a --dsn target — which has no config to read — was indistinguishable from one that had been read and forbids. Both told an operator to pre-apply storage their fleet may drop. It is now a three-valued enum whose zero is UNSPECIFIED, matching the convention field 10 already documents, and the notice has a branch that says the policy could not be established and names how to settle it. Unknown still permits nothing, so what a convergence may run is unchanged. PostgreSQL is read ahead of the policy, because an additive-only bootstrap preserves on every release including the ones too old to say so.

An operator converges v1.5.0's storage ahead of the roll with --dsn, against a MySQL deployment whose config sets allow_destructive_schema_changes: true — a config this path never reads:

Before                                   After

┌───────────────────────────────┐        ┌───────────────────────────────┐
│ no config behind --dsn        │        │ no config behind --dsn        │
│ deployment policy = false     │        │ deployment policy = unknown   │
└───────────────┬───────────────┘        └───────────────┬───────────────┘
                │                                        │
                ▼                                        ▼
┌───────────────────────────────┐        ┌───────────────────────────────┐
│ notice: "every pod that       │        │ notice: "what its pods do     │
│ boots v1.4.0 refuses to       │ ✗      │ with the difference could     │
│ drop what it does not         │        │ not be established ...        │
│ declare ... applied here      │        │ Confirm which before          │
│ survive"                      │        │ relying on this"              │
└───────────────┬───────────────┘        └───────────────┬───────────────┘
                │ operator pre-applies                   │ operator reads the policy
                ▼                                        ▼
┌───────────────────────────────┐        ┌───────────────────────────────┐
│ next pod boots and drops      │        │ converges as part of the      │
│ driver_note; the first        │ ✗      │ deploy instead; the first     │
│ v1.5.0 pod starts without it  │        │ v1.5.0 pod finds it           │
└───────────────────────────────┘        └───────────────────────────────┘

A target too old to accept a named schema is refused rather than answered. schema_files and schema_source are new request fields, and proto3 drops what it does not recognize: an older data plane converges its own embedded schema and answers success, which is indistinguishable from having honored the request. The report's own schema_source is the evidence, since the answering side sets it from the schema it diffed, so both routes now compare it against what was asked and fail with a 502 naming the schema that actually ran.

The version-skew guard caught the answer, not the convergence. A target too old to accept a named schema did not ignore the rest of the request along with it: it still honors allow_destructive, so it diffs its own older embedded schema, finds a newer release's state surplus, and drops it — removals no boot of that deployment would run, since a boot never sees an operator's opt-in. The 502 then discarded the reports, so the operator never saw what had executed. A named convergence now costs a diff first: the diff answers the same question and runs nothing, so the answer is bought for a catalog read rather than for executed DDL. The check after the convergence stays, because a deployment is many pods and a roll makes them different releases — reaching it means the pod that converged is not the pod that answered the diff, and by then DDL has run, so its refusal names what ran and which database to reconcile.

A data plane still on v1.4.0, storage holding a driver_note column pre-applied earlier, and an operator converging v1.5.0 with the --allow-unsafe the destructive gate handed them:

Before                                   After

┌───────────────────────────────┐        ┌───────────────────────────────┐
│ apply --release v1.5.0        │        │ apply --release v1.5.0        │
│ --allow-unsafe                │        │ --allow-unsafe                │
└───────────────┬───────────────┘        └───────────────┬───────────────┘
                │                                        │
                ▼                                        ▼
                                         ┌───────────────────────────────┐
                                         │ diff: which schema would      │
                                         │ you converge?                 │
                                         └───────────────┬───────────────┘
                                                         │
                                                         ▼
┌───────────────────────────────┐        ┌───────────────────────────────┐
│ v1.4.0 pod drops the fields   │        │ v1.4.0 pod answers "my own    │
│ it does not know: it never    │ ✗      │ embedded v1.4.0 schema"       │
│ saw the schema, but it did    │        │                               │
│ see --allow-unsafe            │        │                               │
└───────────────┬───────────────┘        └───────────────┬───────────────┘
                │                                        │
                ▼                                        ▼
┌───────────────────────────────┐        ┌───────────────────────────────┐
│ DROP driver_note has run      │        │ 502 before any DDL runs       │
│ 502 'upgrade that target',    │ ✗      │ driver_note untouched         │
│ reports discarded             │        │                               │
└───────────────────────────────┘        └───────────────────────────────┘

The cross-release notice fell through to the reassuring text for a policy it did not recognize. The policy crosses the HTTP hop as an open string and the CLI is versioned apart from the server it dials, so the values arriving grow without it — and the producer's own comment says every case is named so a later policy falls to the unknown. The consumer did not hold to that, and the consumer is the half an operator reads. BootRemovalPreserves has its own branch now and the default is the unknown text.
Two docs claims were wrong, in the section an operator hand-pointing a selector reads. "A hand-assembled --schema-dir missing half its tables is refused rather than applied" is MySQL-only: PostgreSQL's convergence is additive-only and walks just the files it was given, so a set missing most of a release's tables converges cleanly and reports converged — this PR's own TestApplyStorageSchemaPostgres_ConvergesASuppliedSchema is that case executed. And the skew guard was attributed to the CLI, which compares nothing; it is the server that compares, on the gRPC hop, while the CLI-to-server hop fails closed separately as a strict-decode 400.
Single schema resolution is pinned by counting fetches. It was the one fault injection of fourteen that survived: deleting the memo left every package green while the tag was fetched twice, reopening the moved-tag window. Asserting on the resulting DDL cannot catch it — the two reads sit next to each other in one call, with no seam to change the files through — so the test counts the release fetches instead.

Invariants

AV-9, rewritten deliberately. The clause "converges the schema of the binary running it" is gone, because storage apply --release v1.5.0 converges v1.5.0's files from whatever binary you run. The entry keeps: a convergence never destroys SchemaBot's own storage, and a named schema supplies files and nothing else — every gate applies to it unchanged. The terminal confirmation is not stated as a rule, because it is the CLI's; the Enforced: line names what each surface enforces.
AV-9, extends the enforcement. A named schema is now confirmed honored by the target before anything converges, not only checked on the answer. The rule is unchanged; the Enforced: line gains the HTTP handler, which was always half of "every transport that carries one", and the pre-convergence check.
AV-9, upholds (the destructive gate). A named schema diffs like any other, and a statement dropping a storage table or column is refused on the same path a boot's is. That is what makes an incomplete file set safe: the storage's own tables report as surplus, the disposition that is refused. Pinned by an integration test.
AV-11 and AV-13, uphold. The apply request carries the schema alongside the budget, on new field numbers, so naming a release moves which files run and nothing about the bootstrap lock or what ends the run.

Note

Rebased onto main, which has since gained the convergence budget, per-table progress reporting, and the clause partition; the operator guide this branch originally carried landed separately. Neither command has shipped, so the ApplyStorageSchema / PlanStorageSchema signature change breaks no importer.

Opened by Claude Code (Opus 5).

… one answering storage apply took no schema selector: a convergence ran the schema embedded in the binary answering it, so the storage a release needs could not be in place until that release was already deployed. That is backwards for the workflow the command exists for, where an operator applies the next release's storage ahead of the rollout. --release and --schema-dir now work on apply the same way they work on plan, and the files they name are what converges. The schema is resolved once and threaded through the preview, the confirmation, and the convergence, so a moved tag cannot have an operator approve one file set and run another. Two gates sit in front of it. Naming a release is refused with --auto-approve, so a cross-release convergence is never reachable unattended, and the confirmation states the consequence a plan cannot show: until the named release is deployed, every pod that boots the running one converges the difference back, and it does so asymmetrically. A surplus table or column is refused as destructive and survives; a surplus index loses no data, so it is removed without asking. A release read over a plaintext $GITHUB_API_URL is now refused on a convergence rather than warned about. The rewrite that costs a plan its authority costs a convergence the storage database, since whatever DDL arrives is what runs, having parsed exactly as a real schema would. AV-9 is rewritten for this: which schema a convergence runs is the operator's to name. What it still refuses is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Four comments and a test assertion still described the retired rule, in the layers underneath rather than in the ones this branch rewrote. "apply is what a boot does" was doing two jobs: an apply runs under the same policy a boot runs under, which is still true and is the only thing the three comments citing it were about, and an apply runs the same schema a boot runs, which it no longer does. Each now names the policy property directly, so none of them rests on half a claim. The converged-plan test asserted the absence of the old hint's wording, which pinned a phrase the tree no longer has anywhere. It asserts the absence of a next step instead, which is what the case is about. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…ts target once Review feedback on the storage schema commands, in three parts. A refused `storage apply --json` printed a human plan on the surface a program parses — or, on the destructive gate, which prints nothing and returns silently, nothing at all. Both gates now stay quiet under `--json` and the refusal comes back as the same response shape a convergence does, both halves carrying the report: nothing ran, so everything the plan found is still outstanding. The storage target is resolved once per run and passed along. Every resolution of a config using `storage.dsn_from` is a fresh read of secret references, so up to three of them meant three audited reads of the storage credential, and a window where a value that changed in between had the convergence run against a database its own preview never looked at. The resolution now sits on the target flags themselves, so the copy an apply hands its preview carries the database it already resolved, and a completeness test over the package pins the single call site. The destructive gate's doc comment had run into the manual gate's, leaving one gate wearing the other's AV-9 rationale and the other with no doc.

Copilot

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate findings remain across the API, RPC, source-fetch, CLI, and documentation paths.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 4 · 2

Open (6)

Unresolved merge conflict markers remain in operator guide · New
Enforce approval for named-schema API convergence · New
Reject plaintext redirects during schema release fetches · New
Validate schema source and files pairing in adapter · New
Validate completeness of PostgreSQL supplied schema sources · New
Prevent interactive output from corrupting JSON mode · New

What changed in this PR

Adds named-release schema convergence to storage apply, allowing operators to prepare storage before deployment.

Changes:

Threads selected schema files through CLI, HTTP, gRPC, and bootstrap paths.
Adds confirmation gates, source resolution, JSON handling, and target resolution.
Updates tests, documentation, and AV-9 guidance.

File	Reviewed changes
`pkg/serve/storage_schema.go`	Applies supplied schemas through RPC; critical (2 votes): source-only requests can fall back to the embedded schema.
`pkg/serve/storage_schema_test.go`	Updates adapter tests.
`pkg/serve/serve_version_test.go`	Updates schema attribution tests.
`pkg/proto/ternv1/tern.pb.go`	Regenerates apply-request fields.
`pkg/proto/tern.proto`	Adds schema fields to the apply RPC.
`pkg/cmd/commands/storage_target.go`	Clarifies target policy behavior.
`pkg/cmd/commands/storage_target_resolution_test.go`	Tests single target resolution.
`pkg/cmd/commands/storage_schema.go`	Implements named convergence and gates; moderate (2 votes): JSON applies can emit human output; moderate (1 vote): direct named-schema application lacks integration coverage.
`pkg/cmd/commands/storage_schema_test.go`	Tests CLI convergence behavior and JSON refusals.
`pkg/cmd/commands/storage_schema_source.go`	Resolves schema sources; critical (2 votes): remote plaintext redirects can bypass the convergence protection.
`pkg/cmd/commands/storage_schema_source_test.go`	Tests source resolution and validation.
`pkg/cmd/commands/storage_schema_render.go`	Adds plan next-step hints.
`pkg/cmd/commands/storage_schema_render_test.go`	Tests plan rendering.
`pkg/apitypes/storage_schema_requests.go`	Adds schema fields to API requests.
`pkg/api/storage_schema.go`	Applies supplied schemas through the bootstrap.
`pkg/api/storage_schema_source.go`	Handles supplied schema files; moderate (2 votes): PostgreSQL completeness validation does not hold for partial sources.
`pkg/api/storage_schema_integration_test.go`	Tests supplied MySQL and PostgreSQL convergence.
`pkg/api/storage_schema_handlers.go`	Validates and forwards apply sources; critical (2 votes): named convergence is not enforced as human-confirmed at the API boundary.
`pkg/api/storage_schema_handlers_test.go`	Tests HTTP request validation.
`pkg/api/ensure_schema.go`	Supports supplied MySQL schema files.
`pkg/api/ensure_schema_postgres.go`	Supports supplied PostgreSQL schema files.
`docs/storage-schema.md`	Documents named convergence; critical (3 votes): unresolved merge conflict; nit (1 vote): inaccurate PostgreSQL safety statement; nit (1 vote): overly broad no-disagreement statement.
`docs/release.md`	Updates release workflow guidance.
`docs/invariants.md`	Rewrites AV-9 enforcement details.

Files not reviewed (1)

pkg/proto/ternv1/tern.pb.go: Generated file

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

Kiran01bm · 2026-09-21T21:40:03Z

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1413, eadd348.
Verdict: 3 findings — 2 blocking (unresolved conflict markers in the runbook, a false claim in the cross-release notice), 1 suggestion.

Blocking

The operator runbook ships with unresolved git merge conflict markers. docs/storage-schema.md:361 adds a literal <<<<<<< HEAD, with ======= at L492 and >>>>>>> 00d0a351 … at L496, trapping ~130 lines of exit-status and in-flight-notice documentation inside the conflict and printing the "Under --json a refusal comes back in the shape a convergence does" paragraph twice. docs.yaml's only checks are gen-doc-toc.py --check and the terminology hyphenation grep, neither of which scans for conflict markers, so CI is green.

The cross-release notice tells the operator a surplus index is dropped without asking, which the boot path refuses to do. pkg/cmd/commands/storage_schema.go:638 rests its whole rationale on "an index loses no data, so it is removed without asking", but buildSpiritConfig hardcodes "invisible_index_before_drop": {"raiseError": "true"}, so the DROP INDEX comes back IsUnsafe and partitionDestructiveChanges refuses it — as the untouched TestEnsureSchema_RefusesIndexDropByDefault asserts, and as PostgreSQL's additive-only convergence never does either. An operator who pre-applies release v2's schema therefore schedules the window to protect an index that was never at risk, is never told the real consequence (every v1 pod boot logs a refused destructive change and converges nothing until v2 ships), and reads a notice that contradicts the pre-existing AV-9 sentence that removing a schema object without losing data is still a refusal.

General suggestions

The notice's only test compares strings against the notice's own source text. pkg/cmd/commands/storage_schema_test.go:701 asserts Contains(named, "an index loses no"), a substring of the literal it is checking, so the false half of the asymmetry and its doc-comment rationale both pass CI unchallenged. A test that converged a surplus index and asserted the notice's prediction would have caught finding 2 before review.

The one thing that could have broken, verified

converge := append(append([]EnsureSchemaOption(nil), opts...), WithStorageSchema(desired)) at pkg/api/storage_schema.go:184 is the riskiest mechanism: option-slice reuse across a plan/converge/re-read sequence is where a caller's backing array gets silently mutated or a stale WithStorageSchema wins. It starts from a nil slice, so no aliasing is possible, and WithStorageSchema(desired) is appended last so it overrides any option already in opts. All three reads in ApplyStorageSchema take the same desired argument, so plan, convergence and remaining report cannot describe three different schemas.

Verified correct

desired.Describe() at pkg/serve/storage_schema.go:213 and storage_schema.go:758 is nil-safe — explicit if s == nil guard at storage_schema_source.go:194.
preview shares &preview.storageSchemaTargetFlags with both resolveDesired and converge, so the resolvedTarget memo holds and resolveStorageTarget runs at most once per invocation.
dialectOfTarget's probe := &StoragePlanCmd{…} copy loses the memo, but that branch is only reached when !flags.direct(), where target() is never called — dialectThroughAPI goes over the wire.
storageSchemaPlanHints is passed unconditionally at storage_schema.go:179, but writeStorageSchemaBody short-circuits on report.Converged at storage_schema_render.go:81, so the converged-output test still holds.
The gRPC/HTTP supplied-schema path cannot reach the duplicate-table append in postgresSchemaFiles: validateStorageSchemaFileName requires a flat .sql base name, and two distinct keys cannot trim to the same table.
validateStorageSchemaSource runs after authorizeStorageSchemaOperation on the apply handler, so an unauthorized caller learns nothing about request-shape validation.

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

…ose three gaps around it The confirmation's notice predicted that a pre-applied index would be converged away by the deployed release, which is not what a boot does: an index drop is refused with the same verdict a table or a column drop gets, so everything applied ahead of a roll survives. What the operator needs telling instead is that every one of those boots logs a refused destructive change for it, and that only a release from before indexes were protected undoes the work. An integration test now converges a named schema declaring a surplus index and boots the running release over it, so the prediction is pinned against the behavior rather than against the notice's own wording. Alongside it: - A plaintext redirect no longer bypasses the convergence's fail-closed policy. Guarding only the configured base URL left the choice of channel with the remote end, which can redirect an https base to http and supply the DDL that runs. - A schema named with no files to go with it is refused in the adapter both transports reach, rather than defaulting to the embedded schema and converging it successfully for a caller that asked for another release's. - `--json` keeps stdout to itself on the attended path. A named schema cannot be paired with --auto-approve, so the plan and the prompt would otherwise make the only machine-readable form of a cross-release convergence unparseable. AV-9 no longer claims the terminal confirmation as a rule every surface holds to, since it is the CLI's; its Enforced line names what each surface does enforce. Resolves the conflict markers left in the operator guide, whose two sides were the same paragraph. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Kiran01bm · 2026-09-21T22:29:30Z

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1413, 08eca53.
Re-review of the delta only: eadd3486..08eca535, since the last delivered review at eadd3486.

Verdict: 4 findings — 1 blocking (unattended --json apply prompts for a plan it never prints), 3 non-blocking.

Blocking

An interactive --json apply now asks the operator to confirm DDL it prints nowhere. The new guard at storage_schema.go:379 is if !cmd.AutoApprove && !cmd.JSON, so the only DDL-printing call is skipped on the attended path while storageSchemaConfirmation still prompts; --schema-dir ./pkg/schema/mysql --json against a database missing a table gets the prompt on stderr and the plan nowhere. The operator types yes to a CREATE TABLE on SchemaBot's own storage without ever seeing a statement — before this PR the same invocation printed the plan to stdout. TestStorageApplyCmd_NamedSchemaKeepsJSONParseable only passes because no plan is printed.

Non-blocking

Declining the prompt under --json exits 0 with empty stdout, so the new machine-readable shape cannot report a decline. storage_schema.go:416 returns nil with nothing written; a wrapper's json.Unmarshal fails and it cannot tell "declined, nothing ran" from a crash that exited 0. This is the hole reportRefusal was added to close for gate refusals, left open on the decline path that only exists because a named schema forces the prompt.

The rewritten notice promises a refusal log line that PostgreSQL never emits. storage_schema.go:647 unconditionally says each boot "logs a refused destructive change" for surplus objects, but ensurePostgresSchema never produces a destructive set ("The flow never destroys or alters existing objects…"), so refusalTelemetry emits nothing and the boot logs storage schema up-to-date instead. An operator grepping for the promised line reads its absence as the object having been dropped; the string has no dialect branch and every notice test pins Dialect: "mysql".

The same notice denies destructive convergence outright, which is false under allow_destructive_schema_changes. storage_schema.go:646 tells the operator the deployed release "refuses to drop what it does not declare", yet a deployment that set that config converges destructively and drops the pre-applied index — the exact silent pre-deploy loss the notice exists to warn about, now actively denied. The signal is already at hand: apitypes.StorageSchemaReport.DestructiveAllowed is populated from the resolved policy and blockDestructiveStorageApply reads it. Largely pre-existing — the replaced text made a similar unconditional claim.

General suggestions

Gate the notice's two behavioural promises on report.DestructiveAllowed and the report's dialect rather than emitting one string for every deployment shape; both inputs are already on the report the function receives.

The one thing that could have broken, verified

refuseInsecureRedirect's new closure, since a wrong branch leaks a release token over plaintext. net/http copies Authorization only on a same-host redirect and drops it cross-host before calling CheckRedirect, so the empty-Authorization branch genuinely is the no-token-exposed case across all four hop shapes, and GuardInsecureToken's https-or-loopback carve-out matches what the new loopback test asserts.

Verified correct

guardReleaseHostPlaintext(converging) returning early on a present token is not a hole — that case is refused earlier in getReleaseContents.
The new desiredSchema refusal in pkg/serve/storage_schema.go mirrors validateStorageSchemaSource's second case, aligning the gRPC path with both HTTP routes.
No non-test caller sets SchemaSource without SchemaFiles, so the new refusal breaks nothing: only report/proto plumbing and the two already-validating handlers.
"Files with no name are refused by StorageSchemaFromFiles" holds — empty name, path, non-.sql and bare .sql all rejected, with the description refusals above it.
checkSupplied and StorageSchemaFromFiles both refuse an empty file set, so the embedded default is reachable only when no source names it.
The docs/storage-schema.md conflict-marker resolution loses no content, including the --json refusal-shape paragraph and the exit-code table.
AV-9's amended Enforced line names two real sites that do what it says: transport refusal and CLI confirmation.

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

aparajon · 2026-09-21T22:58:48Z

🤖 Both blocking findings confirmed and fixed in 08eca53. Thanks — the second one is the kind of defect this PR was least able to catch on its own.

Conflict markers. Mine, from the rebase. Both sides turned out to be the same paragraph — the incoming side was a verbatim duplicate of the --json sentence already in the HEAD side — so the resolution keeps the HEAD side whole and drops the duplicate. Your note about docs.yaml is right and is the more durable half of the finding: the TOC check and the terminology grep both pass on a file full of markers.

The notice's claim about a surplus index. Confirmed exactly as you describe. The drop comes back IsUnsafe under invisible_index_before_drop, partitionDestructiveChanges withholds it, and TestEnsureSchema_RefusesIndexDropByDefault has been asserting the index survives the whole time — so the notice was contradicting a test in the same repository, and contradicting AV-9's own sentence that removing a schema object without losing data is still a refusal.

What went wrong is worth naming, because it is not a reasoning error: this branch predates the clause partition landing on main, the notice was written when the asymmetry was real, and the rebase produced no conflict on it because main never edited those lines. The claim arrived stale rather than wrong.

The notice now states the two things an operator cannot see in the plan: everything pre-applied survives the deployed release's boots, and every one of those boots logs a refused destructive change for it until the release ships — a fleet warning about work done on purpose, which is worth predicting before it reads as an incident. The exception you would still want to know about is kept: a release from before indexes were protected does converge a surplus index away.

Your suggestion, which is the fix for the other two. You are right that the only test compared the notice against a substring of its own literal, and that a test converging a surplus index would have caught it. That test now exists: TestApplyStorageSchemaMySQL_PreAppliedIndexSurvivesTheRunningRelease converges a named schema declaring an extra index, boots the running release over it, and asserts the index survives and the refusal names it. The string assertions in the unit test are kept for the wording but now point at it, so the prediction is pinned against behavior rather than against itself.

Also in the same commit, from the Copilot pass: a plaintext redirect no longer bypasses the convergence's fail-closed fetch, a schema named with no files is refused in the adapter both transports reach rather than falling through to the embedded schema, --json keeps stdout to itself on the attended path, and AV-9 no longer states the terminal confirmation as a rule every surface holds to.

Thanks also for the verified-correct list — the option-slice aliasing question in ApplyStorageSchema is the one I would have wanted a second reader on.

This reply was generated by Claude Code (Opus 5).

…pprove Suppressing the plan under --json kept stdout parseable by taking away the only thing the prompt is for. A named schema cannot be paired with --auto-approve, so that path always ends at a confirmation: an operator typing yes to DDL that was printed nowhere is a worse answer than an unparseable stream. The person and the program are two audiences, so the plan and the prompt go to the terminal and stdout carries the response. A decline answers in that shape too. It exited 0 with an empty stream, which a wrapper cannot tell from a crash, and nothing ran — so both halves of the report are what the plan found, as they are for a refusal. The notice also spoke for every deployment at once, and was wrong for two of them. A deployment permitting destructive storage changes converges them, so what is pre-applied is dropped by the next pod rather than kept, which inverts the advice to converge ahead of the roll. PostgreSQL's bootstrap computes no removal at all, so it keeps the state for a different reason, logs no refusal to grep for, and has no release with an index gap to warn about. Both inputs were already on the report the notice receives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

aparajon · 2026-09-21T23:05:01Z

🤖 Re-review findings all confirmed and fixed in a117afa. The blocking one is mine and it is the worse half of the trade I made — thanks for catching it in the delta.

An interactive --json apply asked for approval of DDL it printed nowhere. Correct, and the reasoning error is worth stating: I treated a clean stdout and a visible plan as a choice between two audiences, and picked the wrong one. They are not a choice. The plan and the prompt both belong to the person at the terminal; stdout belongs to the program. Both now go to stderr under --json — the renderers print through fmt.Print, so a scoped writeToTerminal points os.Stdout at stderr for the duration of the render and restores it before anything writes the response. The test asserted the wrong property, as you say; it now asserts CREATE TABLE \applies`` reached stderr, so suppressing the plan again fails it.

A decline under --json exits 0 with empty stdout. Fixed. It returns encodeStorageSchemaConvergence(report, report) — the same shape and the same reasoning as reportRefusal, because nothing ran and everything the plan found is still outstanding. You are right that this hole only exists because a named schema forces the prompt, which makes it this PR's to close rather than pre-existing.

The notice promised a log line PostgreSQL never emits, and denied a destructive convergence that allow_destructive_schema_changes permits. Both fixed, and your suggestion is what I implemented: the notice now branches on report.DestructiveAllowed and the report's dialect, which were already on the struct it receives.

Destructive permitted: the deployed release drops what it does not declare, so the pre-applied state is gone at the next pod restart and the advice inverts — converge as part of the deploy, not ahead of it. This is the case where the silent pre-deploy loss the notice exists to warn about is real, so it was the worst one to be denying.
PostgreSQL: the boots converge only what their own schema adds and never consider a removal, so the state survives untouched and unremarked. No refusal to log, and no release with an index gap.
MySQL without the permission: unchanged from the previous commit.

docs/storage-schema.md now states the same three cases, so the runbook and the notice do not diverge again. Tests cover all three branches, including the assertion that the PostgreSQL notice does not contain the refusal-log sentence or the index-gap caveat.

One note on sequencing: I replied to your first review before seeing this one, so that reply claims the --json change was a clean fix. It was not, and this supersedes it.

This reply was generated by Claude Code (Opus 5).

Kiran01bm · 2026-09-21T23:26:10Z

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1413, a117afa.
Re-review of the delta only: 08eca535..a117afa3, since the last delivered review at 08eca535.

Verdict: 1 finding — 1 blocking (cross-release notice reads the per-request policy, not the deployment's).

Blocking

The cross-release notice tells the operator their deployment permits destructive storage changes whenever they pass --allow-unsafe, which is exactly when it doesn't. storage_schema.go:650 branches on report.DestructiveAllowed, but that field is the effective per-request policy: destructivePolicy returns configAllowsDestructive || requestAllowsDestructive, and the preview sends AllowDestructive: cmd.AllowUnsafe. Deployed boots converge under config alone (serve.go:649).

Concretely: a deployment leaves storage.allow_destructive_schema_changes unset, the destructive gate prints the rerun hint, and the operator re-runs storage apply --release v1.5.0 --allow-unsafe — the very command rerunWithAllowUnsafe() handed them. The confirmation now claims "this deployment permits destructive storage changes" and "Converge as part of the deploy rather than ahead of it", so the operator abandons a safe pre-deploy convergence on the strength of a false statement; in reality those boots refuse and the pre-applied state survives. Neither report type carries a config-only field, so the branch cannot be made correct without adding one.

Verified correct

writeToTerminal's mechanism claim holds: every renderer on the diverted path prints through bare fmt.Print*, which resolves os.Stdout per call — no captured writer in templates/plan.go.
Restoration is deferred, so a panic inside fn still restores os.Stdout.
Nothing writes the JSON response inside the divert window — the encode at lines 420/431 is outside the closure, as the doc comment claims.
prompts := io.Writer(os.Stdout) at line 403 is evaluated after the window closes, so it captures the real stdout before the --json switch to stderr; the divert cannot leak into it.
No colour defect from the stream swap: ui.Colors snapshots os.Stdout at package init but is consulted only in templates/pull.go, and neither plan.go nor pkg/glyph emits ANSI.
No data race on the os.Stdout global: the attended path is single-goroutine, and no t.Parallel test in pkg/cmd/commands overlaps a writeToTerminal window.
gatePrintsPlan := cmd.AutoApprove && !cmd.JSON is unchanged, so the manual and destructive gates still print no second plan and still report refusals as JSON via reportRefusal.
The non-JSON attended path is byte-for-byte unchanged: writeToTerminal(false, fn) just calls fn.
The decline cannot skip a needed non-zero exit: blockManualStorageApply has already errored when report.Manual is non-empty, so the bypassed storageSchemaConvergenceOutcome check would have returned nil anyway.

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

… does The cross-release notice predicts what the deployed release does to state converged ahead of it, and read the effective destructive policy to decide. That policy is the deployment's config widened by the caller's own opt-in, so an operator who met the destructive gate and re-ran with --allow-unsafe (the command the gate's rerun hint hands them) was told their deployment permits destructive storage changes and should converge during the deploy instead. Their deployment permits nothing of the kind: those boots still refuse the drops and the pre-applied state survives, so the notice talked them out of the safe pre-deploy convergence the command exists for. A report now carries both facts. DestructiveAllowed stays the effective policy for the call. BootConvergesDestructively is what the next pod to start does, which is the deployment's standing policy and its dialect, and no request moves it — false on PostgreSQL whatever that deployment configured, since an additive convergence computes no removal to permit. The two travel separately from the one place that knows both: WithDestructiveSchemaChangePolicy takes the deployment's policy and the request's opt-in and derives the effective one, replacing an OR that three call sites each did for themselves before handing over the single flag a report then could not take apart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

aparajon · 2026-09-22T00:20:20Z

🤖 Confirmed and fixed in b0eb9cf. The finding is exactly right, including the part about there being no field to branch on.

The notice read the effective policy where only the standing one is meaningful. destructivePolicy returned configAllowsDestructive || requestAllowsDestructive and the preview sends AllowDestructive: cmd.AllowUnsafe, so --allow-unsafe alone set the field the notice branched on. Your scenario is the one that makes it bite: the destructive gate hands the operator rerunWithAllowUnsafe(), they run it, and the notice then tells them their deployment drops what they are about to apply and they should wait for the deploy. Nothing about their fleet changed — those boots still refuse — so the notice argues them out of the pre-deploy convergence this command exists for, on the one run its own hint asked for.

The fix adds the field rather than hedging the sentence. A report now carries two:

DestructiveAllowed — unchanged, the effective policy for this call.
BootConvergesDestructively — whether the next pod to start drops what its own schema does not declare. The deployment's standing policy and its dialect; no request moves it.

crossReleaseConsequence reads the second and never the first, since its whole subject is what happens after the command exits, when the only thing still converging is a pod starting.

Two things fell out of doing it this way rather than with a config-only mirror of the same bool.

The first is that the field is dialect-aware at the point that knows the dialect. planPostgresStorageSchema sets neither field, so BootConvergesDestructively is false on PostgreSQL whatever that deployment configured — correct, because an additive convergence computes no removal for a policy to permit. The old test asserted the opposite: report("postgres", true) was expected to produce "this deployment permits", which would have told a PostgreSQL operator their boots drop pre-applied state. Unreachable in practice, since the PostgreSQL planner never sets DestructiveAllowed — but it was a hand-built report pinning a renderer against a state the system cannot produce, so it was pinning the wrong thing in the wrong direction. It is gone.

The second is that a plain second bool would have been the same defect waiting: default false reads as "boots refuse", which is the reassuring answer, so any path that forgot to set it would go quietly wrong in the dangerous direction. So the two facts are now set together or not at all. WithDestructiveSchemaChangePolicy(deployment, request) takes both and derives the effective one, which also retires the OR that destructivePolicy, storageTarget.allowsDestructive and the boot path each performed separately before handing over a single flag the report could no longer decompose. checkDestructiveOptIn keeps the local-hosting refusal (AZ-6) and resolves nothing.

Tests: TestDiffStorageSchemaMySQL_BootPolicyIsNotMovedByTheRequestOptIn runs all three policy combinations against a real surplus index and asserts the two fields diverge on exactly the (config=false, request=true) case; TestStorageSchemaConfirmation_CrossReleaseNoticeIgnoresTheRequestOptIn asserts the notice for that report says "refuses to drop what it does not declare" and does not say "this deployment permits"; TestWithDestructiveSchemaChangePolicy_RequestWidensOnlyThisRun pins the widening rule where it now lives.

Invariants: AV-9 upholds. Nothing about what may run moved — the effective policy is computed from the same two inputs and the destructive gate is unchanged. What changed is that a report can now state what a different process will do, which it previously could only guess at from this one's permissions.

Thanks for the verified-correct list on writeToTerminal too — the prompts := io.Writer(os.Stdout) ordering and the ui.Colors init-time snapshot were both things I had reasoned about but not written down anywhere a reader could check.

This reply was generated by Claude Code (Opus 5).

Kiran01bm · 2026-09-22T00:38:24Z

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1413, b0eb9cf.
Re-review of the delta only: a117afa3..b0eb9cf2, since the last delivered review at a117afa3.

Verdict: 6 findings — 2 blocking (the cross-release notice asserts survival where the field is merely absent), 4 non-blocking.

Blocking

The cross-release notice branches on a brand-new proto field, so an older data plane's absent field decodes as false and the CLI positively asserts the pre-applied state survives. storage_schema.go:663 reads BootConvergesDestructively, introduced as tag 11 by this commit and consumed raw with no backfill at storage_schema_proto.go:57. A gRPC deployment with allow_destructive_schema_changes: true whose embedded data plane predates field 11 answers --release with the field absent and destructive_allowed true, so the operator is told "the tables, columns and indexes applied here survive", pre-applies, and the next pod drops it. Pre-PR the notice keyed off DestructiveAllowed (field 6), which an old data plane does populate; this also contradicts the convention stated on field 10 that "false is the absence of evidence, not a claim".

The same false value conflates "the deployment forbids destructive changes" with "this code path cannot know", so the survival promise prints on exactly the deployments whose boots will drop the state. On the documented --dsn pre-deploy flow (docs/storage-schema.md:557) resolveStorageTarget builds a target with no config, so deploymentAllowsDestructive is false and the notice reassures an operator whose MySQL deployment set allow_destructive_schema_changes. The wrong outcome needs a deployment config not confirmable from the tree, and the identical text already printed pre-commit without --allow-unsafe, so only the --allow-unsafe sub-case regresses — but it shares the root cause above and wants the same fix: distinguish absent from false.

Non-blocking

On the --dsn path the deployment policy is always false, so the notice asserts an outcome for a deployment it cannot read. storage_target.go:182 is allowDestructive's only assignment and lives in the mutually-exclusive --config branch. The docs paragraph added here says "What the notice reports is the deployment's own standing policy, never the flags on your command" — on this path it reports neither, only the absence of a config file.

The adapter's type doc still points at destructivePolicy, the symbol this PR renamed to checkDestructiveOptIn. storage_schema.go:33 is the sole remaining mention in the tree, and the PR's hunks start at @@ -289 so the method's own doc was rewritten but not the type block. The sibling pointers (see desiredSchema) and (see attributableVersion) both resolve, proving the form is a symbol pointer that now dangles and makes the AZ-6 refusal look undocumented.

The proto round-trip test was not extended with BootConvergesDestructively, so the new field passes vacuously in both directions. Remove the assignment from either conversion and storage_schema_proto_test.go:15 still passes: the fixture leaves the field zero, so assert.Equal(t, report, round) compares false to false. TestStorageSchemaReport_APIType has the same gap, and no parity or exhaustruct guard covers the report type.

Nothing pins the order of WithDestructiveSchemaChangePolicy's two same-typed bool arguments at either production call site. Swap them at storage_target.go:65 or storage_schema.go:297 and it compiles, every gate keyed on DestructiveAllowed is unchanged because the effective policy is the OR, and BootConvergesDestructively silently becomes the operator's --allow-unsafe flag — the exact misreport this PR exists to remove. Surviving assertions only count options, newEnsureSchemaOptions is unexported, and the two tests that did pin the wiring were deleted here.

The one thing that could have broken, verified

Collapsing allowsDestructive() and destructivePolicy() into one option could have widened or narrowed a safety gate. It did not: ensure_schema.go:128 sets allowDestructive = deployment || request, byte-identical in effect to both deleted helpers, and grep found exactly three production call sites — all passing (deployment, request) in the right order. The new deploymentAllowsDestructive field is read in one place only, so nothing else can observe the split.

Verified correct

checkDestructiveOptIn preserves the AZ-6 locally-hosted refusal byte for byte: same condition, sentinel, message and warn log; its only caller was updated.
The MySQL planner stamps the new field on the single report literal, so the NoChanges early return carries it rather than returning a second unstamped report.
The PostgreSQL planner leaves both destructive fields false soundly — that bootstrap never computes a removal, so the destructive branch is unreachable for its reports.
blockDestructiveStorageApply and the renderer's case report.DestructiveAllowed: still read the effective policy, which is what must honour --allow-unsafe.
All three conversions carry the new field (StorageSchemaReportProto, StorageSchemaReportFromProto, APIType()); no other struct copies the report field-by-field.
tern.proto uses a fresh tag 11 with no renumbering or type change, satisfying the additive-only wire rule, and the regenerated pb.go matches.

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

Three of the answers this command gives an operator deciding whether to pre-apply storage were reassurance a caller had not earned. boot_converges_destructively was a bool, so a data plane too old to set it and one reporting a deployment that preserves surplus state arrived identically, and the notice told both operators their pre-applied state survives to the deploy. It is now a three-valued enum whose zero is UNSPECIFIED, and the CLI has a branch that says the policy could not be established and names how to settle it. PostgreSQL is read before the policy, because an additive-only bootstrap preserves on every release including the ones too old to say so. A --dsn target has no config to read, so its deployment policy is unknown rather than forbidding. The two permit the same thing — nothing — and claim different things, so the execution gate and the report now take different types: transposing WithDestructiveSchemaChangePolicy's two arguments is a compile error rather than a report describing a caller's own flag as the fleet's policy. --release and --schema-dir travel as request fields a release predating them ignores, converging its own embedded schema and reporting success. The report's schema_source is the evidence, so both routes now refuse an answer that names a schema the caller did not ask about. Upholds AV-9: none of this widens what a convergence may run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The symbol pointer corrected in the previous commit left one line past the width the rest of the block keeps to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Kiran01bm · 2026-09-22T01:32:49Z

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1413, 8d3a98f.
Re-review of the delta only: b0eb9cf2..8d3a98f6, since the last delivered review at b0eb9cf2.

Verdict: 3 findings — all non-blocking (one over-claiming comment, two dangling doc pointers, one unguarded CLI switch).

Non-blocking

The apply-path guard's justifying comment over-claims: allow_destructive travels on the same request, so an unhonored convergence can run removals no boot of that target would have run. storage_schema_handlers.go:289 says the convergence "ran that target's own schema, which is what a boot of it would have run anyway", but an older target that ignores schema_files/schema_source still honors the allow_destructive forwarded at :255 — so it diffs against its embedded schema, finds a pre-applied table surplus, and drops it where a boot under a forbidding policy would have refused. The guard then 502s "Upgrade that target" and returns without writing planned/remaining (:292), so the operator never sees the drop that just executed. The drop path predates this PR; what is new is the comment claiming it cannot happen and the 502 discarding the evidence.

Two doc comments still point at BootConvergesDestructively / boot_converges_destructively, which this PR deleted. storage_schema.go:72 sends a reader of DestructiveAllowed to a name nothing declares, with no pointer to the replacement BootRemovalPolicy three fields below. The same dead name ships in the published tern.proto:1048 and its generated tern.pb.go:4199, where external consumers read it.

crossReleaseConsequence has no default, so an unrecognized BootRemovalPolicy falls through to the reassuring "your pre-applied state survives" text. storage_schema.go:703 names only BootRemovalRemoves and BootRemovalUnknown; the policy is an open Go string decoded verbatim from JSON at request.go:272, and the CLI is versioned independently of the control plane it dials. Both proto converters are hardened and unit-tested against exactly this (storage_schema_proto_test.go:110), so only the one consumer that renders the claim to an operator lacks the guard — adding case apitypes.BootRemovalPreserves: and routing default: to the unknown text closes it.

The one thing that could have broken, verified

WithDestructiveSchemaChangePolicy changing its zero value from an effective Forbids to Unknown could have silently widened or narrowed an existing destructive gate. It did not: deployment == DestructivePolicyPermits || request (ensure_schema.go:181) is behaviour-equivalent to the old deployment || request for both boolean-reachable policies and grants nothing on Unknown, and every production call site passes the option explicitly (serve.go:649, storage_schema.go:297, storage_target.go:71), leaving the new zero value reachable only from the deliberate --dsn-only target that two tests pin.

Verified correct

refuseUnhonoredSchema trim symmetry is sound — both sides trim (handlers:407, storage_schema_source.go:109), so whitespace raises no spurious 502.
AttributeTo cannot overwrite a caller-supplied SchemaSource: it rewrites only the empty or reserved-phrase case, so a honored named schema echoes back intact.
mysqlBootRemovalPolicy is exhaustive over the three policies and its unreachable fallthrough returns BootRemovalUnknown, never Preserves; pinned by TestMySQLBootRemovalPolicy.
bootRemovalPolicyProto and bootRemovalPolicyFromProto both map unrecognized input to UNSPECIFIED/Unknown rather than Preserves, covered by the value-99 and later-release-string assertions.
omitempty on a "" Unknown means an older JSON client sees the field absent and resolves it to Unknown — the honest reading.
The crossReleaseConsequence reorder putting the PostgreSQL check ahead of the policy switch changes no reachable output, since planPostgresStorageSchema always sets BootRemovalPreserves (storage_schema.go:372).

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

morgo

🤖 Reviewed on Morgan's behalf, at 8d3a98f6. Approving. The code is right where it matters: the boot path resolves to the identical old value, the three production call sites of the renamed destructive option are equivalent for what actually runs, the schema is resolved once, and thirteen of fourteen fault injections were caught. The DeploymentDestructivePolicy/bool pair is the right shape and the reason given for it holds — I transposed them by hand and pkg/api failed, which two bools would not have.

The findings are all in the prose, and two of them are load-bearing enough to fix before merge because an operator acts on them.

1. The `--schema-dir` safety claim is false on PostgreSQL, and this PR's own test is the counterexample

docs/storage-schema.md:546-550:

The convergence gets no more permission than a boot: a statement that would drop a storage table or column is refused as it is at startup, and a PostgreSQL column shape the convergence will not run still stops the whole set. So a hand-assembled --schema-dir missing half its tables is refused rather than applied.

True on MySQL. On PostgreSQL it is the opposite, and the PR says so itself sixty lines away in pkg/api/storage_schema_source.go:96-107:

On PostgreSQL the convergence is additive-only and walks only the tables the set supplies, so an omitted table is not reported at all. Nothing is dropped, and the cost is the other way round: a set missing most of a release's files can plan as converged

TestApplyStorageSchemaPostgres_ConvergesASuppliedSchema, added in this PR, is that case executed: it supplies exactly one file, converges an empty storage database, and asserts remaining.Converged() is true while every other storage table is absent. A set missing all but one table converged cleanly and reported converged.

The sentence is a problem rather than a wording nit because of the sentence in front of it. That one names PostgreSQL explicitly, so the unqualified "So a hand-assembled --schema-dir…" reads as covering both dialects — and the paragraph sits inside the new Converging a release before it rolls section, whose whole subject is an operator hand-pointing a selector at a checkout. On PostgreSQL a partial --schema-dir exits 0 with "Nothing is outstanding" for a release whose storage is not ready, which is the exact belief this command exists to make reliable. The source comment already has the right words; the docs want the same split.

2. The CLI does not do what this paragraph says it does

docs/storage-schema.md:539-544:

A target too old to accept a named schema is refused rather than answered. --release and --schema-dir travel as request fields a release that predates them ignores, converging its own embedded schema instead and reporting success — so the CLI checks the schema the report says it used against the one you asked for, and fails the command when they differ.

The CLI checks nothing. StorageApplyCmd.converge sends request.SchemaSource and returns response.Planned without ever comparing them (pkg/cmd/commands/storage_schema.go:860-875), and readThroughAPI does the same.

The real guard is refuseUnhonoredSchema (pkg/api/storage_schema_handlers.go:406), which runs in the server, on both routes, against the answer from the gRPC hop to the data plane. That is the correct place for it: proto3 drops unknown fields, so an old data plane silently answers about its own schema, which is the failure being described.

The CLI→server hop it does not cover does not need covering, for a different reason worth stating in the doc since it produces a different error: decodeOptionalStorageSchemaBody sets DisallowUnknownFields() (pkg/api/storage_schema_handlers.go:523, present in main too), and schema_files/schema_source are new on StorageSchemaApplyRequest. A SchemaBot server predating this PR therefore 400s the request rather than ignoring the fields. So the hop fails closed, but it fails closed as a body-decode error, not as "the target answered about X, not the schema that was asked for". An operator handed this paragraph and then a 400 will go looking for the comparison it promises.

Both routes really are compared, and the PR body's own wording ("both routes now compare it") is accurate — it is only the doc that attributes the check to the CLI.

3. `crossReleaseConsequence` falls through to the reassuring message for a policy it does not recognise

pkg/cmd/commands/storage_schema.go:680 switches on report.BootRemovalPolicy with arms for BootRemovalRemoves and BootRemovalUnknown. BootRemovalPreserves and every unrecognised value share the final unguarded return. Measured, with running = "v1.2.3":

policy=""                     → "…could not be established… Confirm which before relying on this"   ✅ correct
policy="preserves"            → "…every pod that boots v1.2.3 refuses to drop what it does not declare, so the
                                 tables, columns and indexes applied here survive…"
policy="removes"              → "…this deployment permits destructive storage changes… Converge as part of the
                                 deploy rather than ahead of it."
policy="removes_after_grace"  → byte-identical to policy="preserves"

The zero value is handled correctly — I got that wrong on a first pass and checked the rendered text rather than a classifier.

What makes this worth a line is that the producer forty lines up is deliberately exhaustive and says why:

mysqlBootRemovalPolicy … Every case is named, so a policy added later falls to the unknown rather than inheriting whichever answer a default happened to be.

The consumer does not hold to that, and it is the consumer that decides what an operator reads. Reachability is one-directional and forward-looking: bootRemovalPolicyFromProto maps unknown proto enums to Unknown, so gRPC is safe; the JSON transport passes the string through verbatim, so a value only arrives from a newer server answering an older CLI — the reverse of this feature's usual skew, but the direction that accumulates as the field grows values. One line fixes it: give BootRemovalPreserves its own case and let the default be the Unknown text, which is the shape the whole design argues for ("Survival is the claim an operator acts on by pre-applying, so it is the claim that has to be earned").

Notes

A renamed field left its old name in two doc comments. boot_converges_destructively / BootConvergesDestructively appears in pkg/proto/tern.proto:1048 ("Use boot_converges_destructively to reason about what some other process does"), pkg/apitypes/storage_schema.go:72, and the generated tern.pb.go. git grep -i finds no such identifier anywhere. The field is boot_removal_policy / BootRemovalPolicy. The proto one matters more than the Go one: tern.proto is the contract another implementation reads, and it points at a field that does not exist.
The one invariant in the PR body that nothing pins is the headline one. "The schema is resolved once, in Run, and threaded through the preview, the confirmation, and the convergence." Deleting preview.resolved = desired leaves pkg/cmd/commands, pkg/api, pkg/serve and pkg/apitypes all green — desiredSchema then falls through to resolve, which memoises nothing, so the tag is fetched twice and the moved-tag window the comment describes is reopened. It is the only injection of fourteen that survived. The asymmetry is what stands out: the target-resolution twin got TestStorageTargetIsResolvedInOnePlace, an AST walk that will catch a second call site someone adds next year, and the schema resolution — the one whose second read can change the DDL that runs — has nothing. An assertion that a source resolved once is reused would be enough; the AST version would be better and is already written for the sibling.
AV-9's Enforced: line under-cites. "a named schema is refused apart from the files it names on every transport that carries one (pkg/serve/storage_schema.go)" — validateStorageSchemaSource in pkg/api/storage_schema_handlers.go:545 is the other half of "every transport", and it is the half that was already there. Worth naming both, since the invariant's claim is specifically about the pair.
writeToTerminal swaps the process-global os.Stdout. It is the right call for this codebase — the renderers print through fmt.Print, so threading an io.Writer through all of them is a much larger change — and the doc comment names the mechanism honestly, including the constraint that makes it work ("nothing writes the response until after it is restored"). Noting only that it is not concurrency-safe and only diverts writers that resolve os.Stdout per call: a renderer added later that captures the writer up front escapes it silently. Not reachable today; -race on the package is clean, and Go's parallel-test scheduling keeps the serial tests that use it away from the parallel ones in the same package.
The convergence-budget check is correctly hoisted above the plan (Run, before preview.read), so a bad --timeout does not cost a production catalog read first. Small thing, deliberate, and the comment says why.

What I checked rather than took on trust

The boot path is untouched, verified rather than asserted. serve.go:649 goes from WithAllowDestructiveSchemaChanges(cfg.Storage.AllowDestructiveSchemaChanges) to WithDestructiveSchemaChangePolicy(ConfiguredDestructivePolicy(cfg.Storage.AllowDestructiveSchemaChanges), false); allowDestructive resolves to config || false, the old value. No WithStorageSchema, and (*StorageSchemaSource)(nil).isEmbedded() is true, so o.schemaSource.mysqlSchemaFiles() returns readEmbeddedSchemaFiles() exactly as before.
All three production call sites of the renamed option, not just the boot. pkg/serve/storage_schema.go:297 is config || request, the old allowsDestructive. pkg/cmd/commands/storage_target.go:71 passes t.destructive, whose zero value on a --dsn target is Unknown — Unknown == Permits || request is request, identical to the old false || request. Execution is unchanged on every path; only the report moved. git grep WithAllowDestructiveSchemaChanges returns nothing, so nothing was left behind.
The "too old to accept a named schema" story on both hops. Read above; the gRPC hop is guarded by refuseUnhonoredSchema on both routes, and the HTTP hop fails closed on strict decoding. Finding 2 is what I found by checking it.
The proto is additive. boot_removal_policy = 11 on a message whose previous max was 10; schema_files = 4 and schema_source = 5 on a request whose previous max was 3. No reuse, no renumbering, and the enum's zero is UNSPECIFIED.
MySQL really does report an omitted table as surplus, which is what the "incomplete file set is safe" argument rests on and what makes finding 1 a PostgreSQL-only problem. TestEnsureSchema_RemovesObsoleteVitessTasks (pre-existing) is the proof: a table on the database and absent from the files is computed as a removal, run when destructive is permitted and refused when it is not.
Fault injection — thirteen of fourteen. blockUnattendedNamedSchema never refusing → pkg/cmd/commands; resolveDesired always nil → pkg/cmd/commands; ApplyStorageSchema dropping WithStorageSchema → caught by pkg/api under -tags integration (green without it, so this one needs the integration job); refuseUnhonoredSchema disabled → pkg/api; the same on the apply route alone → pkg/api; the adapter letting a name with no files fall through to embedded → pkg/serve; guardReleaseHostPlaintext downgrading a convergence to a warning → pkg/cmd/commands; refuseInsecureRedirect doing the same per hop → pkg/cmd/commands; the deployment policy taken from the request flag → pkg/api; mysqlBootRemovalPolicy reporting Unknown as preserves → pkg/api; writeToTerminal never diverting → pkg/cmd/commands; the prompt left on stdout under --json → pkg/cmd/commands; attributeStorageSchemaConvergence attributing only the planned half → pkg/cmd/commands; a --dsn target claiming Forbids instead of Unknown → pkg/cmd/commands. Only the single-resolution note above survived.
The two findings in the prose are measured, not read. Finding 1 is the PR's own new PostgreSQL test run against the sentence; finding 3 is crossReleaseConsequence called directly with four policy values and the rendered strings compared, because the difference between the arms is a difference between two paragraphs and that is invisible from the switch.
Base is current and the delta is what GitHub shows. merge-base origin/main pr1413 is 2afa3ffe; origin/main...pr1413 is 35 files, +2679/−651, matching the PR. go build, and the pkg/cmd/commands, pkg/api, pkg/serve and pkg/apitypes suites, are green at this head, plus -race. CI: 43 checks, 40 SUCCESS, no genuine failures, nothing pending, mergeStateStatus: CLEAN.

…chema The version-skew guard caught the answer, not the convergence. A target too old to accept a named schema still honors allow_destructive on the same request, so it diffs its own older embedded schema, finds a newer release's state surplus, and drops it — removals no boot of that deployment would run, since a boot never sees an operator's opt-in. The guard then 502'd and discarded the reports, so the operator never saw what had executed. A named convergence now costs a diff first. The diff answers the same question and runs nothing, so "would this target work from the schema I sent" is bought for a catalog read instead of for executed DDL. The post-hoc check stays, because a deployment is many pods and a roll makes them different releases: reaching it means the pod that converged is not the pod that answered the diff, and by then DDL has run, so its refusal names what ran and which database to reconcile. crossReleaseConsequence gains a default. The policy crosses the HTTP hop as an open string and the CLI is versioned apart from the server it dials, so a value from a later release fell through to "your pre-applied state survives" — the one claim that has to be earned. Docs: the "--schema-dir missing half its tables is refused" claim was MySQL-only; PostgreSQL's additive-only convergence reports such a set as converged. The guard is also attributed to the server rather than to the CLI, which compares nothing. Pins the single resolution of a named release by counting fetches, the one fault injection of morgo's fourteen that survived. Upholds AV-9; its Enforced line now names both transports and the pre-convergence check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

aparajon · 2026-09-22T18:27:59Z

🤖 All six fixed in 2761f6e. The two blocking findings are one defect, and the fix generalises past the case you named.

A bool cannot report "I could not say", and every path that could not say was rendered as the reassuring half. You are right that field 11 contradicts the convention field 10 already documents — and the same conflation exists one layer up, before the wire is involved: storageTarget.allowDestructive was false on the --dsn path because no config was read, which is a different fact from a config that was read and forbids. The two produce identical reports and identical notices, and the notice is the one that tells an operator to pre-apply.

So the fix is a three-valued policy at both altitudes:

On the wire, boot_removal_policy is an enum whose zero is BOOT_REMOVAL_POLICY_UNSPECIFIED. An older data plane leaves it there; so does a newer one sending a policy this binary has no name for. Both map to unknown, never to preservation.
In the CLI, DeploymentDestructivePolicy is Unknown | Forbids | Permits, and --dsn resolves to Unknown. It permits nothing, so the execution gate is byte-identical to the old false — what changed is only what may be claimed.

crossReleaseConsequence has a third branch for it, which says the policy could not be established, names both outcomes, and gives the two ways to settle it. PostgreSQL is now read before the policy, because an additive-only bootstrap preserves surplus state on every release, including the ones too old to report a policy at all — reading the field first would have answered "could not be established" for a case the dialect settles on its own.

On your --allow-unsafe sub-case: correct that only that one regresses, and correct that the root cause is shared. The --dsn half is the wider one — the flag is not needed for it, just a deployment that set allow_destructive_schema_changes.

The dangling destructivePolicy pointer is fixed; it now names checkDestructiveOptIn. The observation about the sibling pointers resolving is the part that made it worth fixing rather than deleting — the form is load-bearing, so a broken one is worse than none.

The vacuous round-trip is fixed by naming the field, not by setting it. You are right that assert.Equal(t, report, round) compares false to false, and right that the same applies to TestStorageSchemaReport_APIType — but a fixture value alone only fixes it until the next field. The round trip is blind to any symmetric error by construction, so the proto test now asserts the wire value by name (ternv1.BootRemovalPolicy_BOOT_REMOVAL_POLICY_REMOVES) alongside the round trip, next to the existing named assertions. Two more cover the skew directly: an older report with the field unset and everything else populated stays unknown through both hops, and an unrecognised value does too.

The argument order is now pinned by the compiler rather than by a test. Your analysis of why a test would not have caught it is the reason: the effective policy is the disjunction, so a swap leaves every gate correct and only the report wrong, which is the one thing that has no gate. WithDestructiveSchemaChangePolicy(DeploymentDestructivePolicy, bool) takes two different types, so the transposition does not compile. ConfiguredDestructivePolicy(bool) is the one place a config's bool becomes a policy, and it is what the two config call sites pass.

One thing your review did not reach, which the same skew produces more severely. schema_files and schema_source are also new request fields on this PR, and proto3 drops unknown fields: an older data plane converges its own embedded schema and answers success. Not a wrong report — a wrong convergence, reported as the named release's storage being ready, surfacing when that release's pods boot into storage missing what they declare. It needs no new field to detect, because the report already echoes the schema the answering side diffed: honored, it matches what was asked; ignored, it names the answering binary's own files. Both routes now compare the two and answer 502 naming the schema that actually ran. TestStorageSchemaRoutes_RefuseAnAnswerAboutAnotherSchema covers plan, apply, the honored case, and a request that named no schema.

Invariants: AV-9 upholds throughout. Unknown permits nothing, so nothing widened; the destructive gate, blockDestructiveStorageApply, and the renderer all still read the effective policy.

TestDiffStorageSchemaMySQL_BootPolicyIsNotMovedByTheRequestOptIn gained a fourth case for the unread policy against a real surplus index, and the docs paragraph you quoted now says what the --dsn path actually reports.

This reply was generated by Claude Code (Opus 5).

aparajon · 2026-09-22T18:28:04Z

🤖 Both reviews confirmed and fixed in 1829a10 — @Kiran01bm's three and @morgo's three, plus two of the notes. Thanks to you both; the overlap on the cross-release default and the dangling proto pointer is what made those two unarguable, and the two you each found alone are the two that changed the shape of the PR.

The guard was in the wrong place, and the comment defending it was the tell. Kiran's first finding is right, and right about the part I would have defended: allow_destructive rides the same request an older target ignores the schema fields on, so that target diffs its own embedded schema, finds a newer release's pre-applied state surplus, and drops it — a removal no boot of that deployment would ever compute, because a boot never sees an operator's opt-in. My comment claimed the convergence "is what a boot of it would have run anyway"; it is not, and the 502 then discarded both reports so the operator never learned what had executed.

The narrow fix was to correct the comment and write the reports out. That leaves executed DDL on the table and calls it disclosure. What the guard actually needs is to ask the question before it costs anything, and there is already a call that asks exactly it and runs nothing: a named convergence now plans first, and the plan's schema_source answers "which schema would you converge?" for a catalog read instead of for a dropped column.

A data plane still on v1.4.0, storage holding a driver_note column pre-applied earlier, and an operator converging v1.5.0 with the --allow-unsafe the destructive gate handed them:

Before                                   After

┌───────────────────────────────┐        ┌───────────────────────────────┐
│ apply --release v1.5.0        │        │ apply --release v1.5.0        │
│ --allow-unsafe                │        │ --allow-unsafe                │
└───────────────┬───────────────┘        └───────────────┬───────────────┘
                │                                        │
                ▼                                        ▼
                                         ┌───────────────────────────────┐
                                         │ diff: which schema would      │
                                         │ you converge?                 │
                                         └───────────────┬───────────────┘
                                                         │
                                                         ▼
┌───────────────────────────────┐        ┌───────────────────────────────┐
│ v1.4.0 pod drops the fields   │        │ v1.4.0 pod answers "my own    │
│ it does not know: it never    │ ✗      │ embedded v1.4.0 schema"       │
│ saw the schema, but it did    │        │                               │
│ see --allow-unsafe            │        │                               │
└───────────────┬───────────────┘        └───────────────┬───────────────┘
                │                                        │
                ▼                                        ▼
┌───────────────────────────────┐        ┌───────────────────────────────┐
│ DROP driver_note has run      │        │ 502 before any DDL runs       │
│ 502 'upgrade that target',    │ ✗      │ driver_note untouched         │
│ reports discarded             │        │                               │
└───────────────────────────────┘        └───────────────────────────────┘

The check after the convergence stays rather than being replaced, because a deployment is many pods and a roll makes them different releases: reaching it now means the pod that converged is not the pod that answered the plan. By then DDL has run, so that refusal no longer says "upgrade the target" and stops — it names how many statements ran, which database on which host they ran against, and that the database has to be reconciled against the release the target is actually running before anything is retried. The plan's extra round trip is paid for in the deadline — a convergence's write budget already covers the two diffs bracketing it, and a third diff's worth is added only when a schema is named, and a request naming no schema still makes exactly one call — TestStorageSchemaRoutes_RefuseAnAnswerAboutAnotherSchema asserts the plan request is nil in that case, and that applyReq is nil in the refusing one.

The cross-release notice. Both of you, and morgo measured it: policy="removes_after_grace" rendered byte-identical to preserves. The argument that settles it is the one about direction — the JSON hop passes the string verbatim and the CLI is versioned apart from the server, so an unrecognised value arrives from a newer server, which is the skew that accumulates as the field grows values rather than the one this feature was built for. BootRemovalPreserves has its own case now and default routes to the unknown text. Pinned with a policy string from a hypothetical later release.

The --schema-dir safety claim was false on PostgreSQL — morgo's first, and the counterexample being this PR's own TestApplyStorageSchemaPostgres_ConvergesASuppliedSchema is what makes it indefensible as a wording nit. The two dialects fail an incomplete set in opposite directions: MySQL reads the omissions as surplus and refuses, PostgreSQL walks only what it was given and reports converged for storage that is not ready. The paragraph now splits by dialect and says so, and recommends pointing the flag at a whole pkg/schema/<dialect> directory rather than a hand-assembled set.

The skew paragraph credited the CLI with a comparison it does not make. Also right — converge sends SchemaSource and returns Planned without looking at either. The docs now say the server compares, on the hop to the data plane, before converging and again on the answer; and the CLI→server hop is described separately as what it is, a strict-decode 400 that fails closed with a different error than the one the paragraph used to promise.

The dangling boot_converges_destructively / BootConvergesDestructively. Fixed in both, proto regenerated. Agreed the proto one is the one that matters: another implementation reads that file and it pointed at a field no message declares.

On the notes:

Single resolution was the one injection of fourteen that survived. Fixed, and the diagnosis of why the obvious test does not work is the useful part: resolveDesired and the preview's desiredSchema sit next to each other in one call, so there is no seam to change the files through between them — a DDL assertion cannot see the second read. TestStorageApplyCmd_ResolvesTheNamedReleaseOnce counts fetches against a fake release host instead: one listing, one file read, and a second resolution doubles both. I injected preview.resolved = desired away again afterwards and confirmed it fails. The AST version for the sibling is the better form and is worth having here too, but counting the fetches pins the property the moved-tag window actually turns on, which is the number of times the tag is read.
AV-9's Enforced: line under-cited. Right, and the HTTP handler was the half that was already there. The line now names both transports, and gains the pre-convergence check as its own clause.
writeToTerminal swapping os.Stdout. No change, and the reasoning is yours: a renderer added later that captures the writer up front escapes it silently. That is real but not reachable today, and the alternative is threading an io.Writer through every renderer that prints via fmt.Print. Recording it here rather than in a comment that would go stale.
The budget hoist. Noted — and the plan added above sits under the same hoisted check, so a bad --timeout still costs no catalog read.

Invariants. AV-9 extends the enforcement: the rule is unchanged, and a named schema is now confirmed honored before anything converges rather than only checked on the answer. Nothing weakens. The destructive gate, blockDestructiveStorageApply, and the renderer all still read the effective policy, and the new refusal path permits nothing it did not already permit — it only refuses earlier.

This reply was generated by Claude Code (Opus 5).

aparajon force-pushed the armand/storage-schema-guide branch from 95191f4 to 10aa468 Compare September 15, 2026 22:50

aparajon force-pushed the armand/storage-apply-named-release branch from 28171d0 to cf405fa Compare September 15, 2026 22:53

aparajon force-pushed the armand/storage-schema-guide branch 7 times, most recently from 590ae1f to fe55852 Compare September 17, 2026 16:36

aparajon force-pushed the armand/storage-apply-named-release branch from 28ff40f to 00d0a35 Compare September 17, 2026 18:37

aparajon force-pushed the armand/storage-schema-guide branch from afe5a8c to 28a2622 Compare September 18, 2026 19:02

Base automatically changed from armand/storage-schema-guide to main September 18, 2026 20:23

aparajon and others added 3 commits September 21, 2026 17:04

Copilot AI lite review requested due to automatic review settings September 21, 2026 21:10

aparajon force-pushed the armand/storage-apply-named-release branch from 00d0a35 to eadd348 Compare September 21, 2026 21:10

Copilot started reviewing on behalf of aparajon September 21, 2026 21:11 View session

aparajon marked this pull request as ready for review September 21, 2026 21:17

aparajon requested review from JashLal, Kiran01bm, eeSeeGee, jayjanssen and morgo as code owners September 21, 2026 21:17

aparajon requested a review from jemiahw as a code owner September 21, 2026 21:17

Copilot AI reviewed Sep 21, 2026

View reviewed changes

aparajon changed the title ~~feat(cli): converge storage to the release an operator names, not the one answering~~ feat(cli): let storage apply converge to a release you name, before it rolls Sep 21, 2026

aparajon and others added 2 commits September 21, 2026 20:57

style(serve): rewrap the storage schema adapter's doc comment

8d3a98f

The symbol pointer corrected in the previous commit left one line past the width the rest of the block keeps to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Kiran01bm approved these changes Sep 22, 2026

View reviewed changes

morgo approved these changes Sep 22, 2026

View reviewed changes

aparajon merged commit 873ddb5 into main Sep 22, 2026
43 checks passed

aparajon deleted the armand/storage-apply-named-release branch September 22, 2026 18:41

Conversation

aparajon commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why this matters

From review

Invariants

Note

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kiran01bm commented Sep 21, 2026

Blocking

General suggestions

The one thing that could have broken, verified

Verified correct

Uh oh!

Kiran01bm commented Sep 21, 2026

Blocking

Non-blocking

General suggestions

The one thing that could have broken, verified

Verified correct

Uh oh!

aparajon commented Sep 21, 2026

Uh oh!

aparajon commented Sep 21, 2026

Uh oh!

Kiran01bm commented Sep 21, 2026

Blocking

Verified correct

Uh oh!

aparajon commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kiran01bm commented Sep 22, 2026

Blocking

Non-blocking

The one thing that could have broken, verified

Verified correct

Uh oh!

Kiran01bm commented Sep 22, 2026

Non-blocking

The one thing that could have broken, verified

Verified correct

Uh oh!

Kiran01bm left a comment

Choose a reason for hiding this comment

Uh oh!

morgo left a comment

Choose a reason for hiding this comment

1. The --schema-dir safety claim is false on PostgreSQL, and this PR's own test is the counterexample

2. The CLI does not do what this paragraph says it does

3. crossReleaseConsequence falls through to the reassuring message for a policy it does not recognise

Notes

What I checked rather than took on trust

Uh oh!

aparajon commented Sep 22, 2026

Uh oh!

aparajon commented Sep 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aparajon commented Sep 15, 2026 •

edited

Loading

aparajon commented Sep 22, 2026 •

edited

Loading

1. The `--schema-dir` safety claim is false on PostgreSQL, and this PR's own test is the counterexample

3. `crossReleaseConsequence` falls through to the reassuring message for a policy it does not recognise