Skip to content

docs(storage): write the operator guide for SchemaBot's own storage schema - #1396

Merged
aparajon merged 17 commits into
mainfrom
armand/storage-schema-guide
Sep 18, 2026
Merged

aparajon merged 17 commits into
mainfrom
armand/storage-schema-guide

Conversation

@aparajon

@aparajon aparajon commented Sep 11, 2026 •

Copy link
Copy Markdown
Collaborator

What this adds

SchemaBot converges its own bookkeeping database at every startup. Almost nobody has had to learn how, until a deploy does not converge — and then the answer was spread across a configuration option, a release step, and the code.

docs/storage-schema.md collects it: the two convergence paths, startup bootstrap and storage apply, as the one code path they are; what each dialect converges automatically and what fails startup instead; both operator commands, with the renderings to read under pressure and an exit-status table a script can key on; playbooks for rolling a release that changes the storage schema and for a fleet crashlooping on a convergence that did not happen; and the indexes worth pre-creating on a database with a long history.

Two things it is deliberately explicit about:

  • Where the CLI and a booting pod disagree. storage apply stops on a destructive statement and exits non-zero. Startup skips that statement and converges the rest, rather than take a deployment down over surplus state a rollback left behind.
  • Where a rule is dialect-specific. MySQL refuses destructive statements; PostgreSQL converges additively and produces none. The pre-creatable indexes are named differently in each dialect, so both spellings are given.

The operator narrative moves out of configuration.md, which keeps the setting it documents and a pointer to the guide. README.md, docs/cli.md, and docs/release.md link to it. Every console block was generated by calling the renderers the commands use, so it is what an operator will actually see.

Invariants

AV-9, documented rather than changed. No behavior moves here. Rebased onto #1390, which extended AV-9 to removals that lose no data, so the guide now describes an index drop as refused on boot rather than applied, and keeps the part of that warning still true: the refusal lives in the booting binary, so a release from before #1390 still converges a pre-created index away.

Opened by Claude Code (Opus 5).

@aparajon
aparajon added this pull request to stack #1397 September 11, 2026 20:26
Copilot AI lite review requested due to automatic review settings September 11, 2026 20:29
@aparajon
aparajon force-pushed the armand/storage-schema-guide branch from f74ffb3 to efa616a Compare September 11, 2026 20:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The documentation has unresolved correctness and operator-guidance issues across dialect behavior, command semantics, examples, rollback, and timeout details.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This docs-only PR adds an operator and contributor guide for SchemaBot’s self-converging storage schema and links it from existing documentation.

Changes:

  • Documents convergence paths, dialect behavior, CLI commands, rollout playbooks, and index pre-creation.
  • Moves detailed storage guidance out of configuration documentation.
  • Registers and links the new guide across README, CLI, release documentation, and the TOC.
File summaries
File Change and final review notes
README.md Links to the storage-schema guide. No final findings.
docs/storage-schema.md Adds the operator and contributor guide. Final findings: [nit, 1 vote] qualify destructive safety as the default (L31); [nit, 3 votes] qualify the “no manual DDL” claim (L42); [nit, 1 vote] include stale Spirit tables in the fast-path condition (L61); [nit, 1 vote] avoid promising an exact match across dialects (L71); [nit, 1 vote] distinguish the five-minute per-attempt timeout from the eight-minute boot retry budget (L87); [nit, 2 votes] explain PostgreSQL partial commits after earlier DDL (L133); [nit, 1 vote] document PostgreSQL index-shape validation limits (L137); [nit, 1 vote] make the MySQL unsafe-operation list non-exhaustive (L161); [nit, 1 vote] qualify rollback index removal as MySQL-specific (L174); [nit, 2 votes] correct the default release-repository output at L204/L263/L356; [nit, 1 vote] make --schema-dir examples dialect-specific (L289); [nit, 1 vote] clarify that --release requires the named release binary to converge (L318); [nit, 2 votes] document storage apply exit-code semantics (L332); [nit, 1 vote] distinguish MySQL and PostgreSQL rollback behavior (L434); [nit, 1 vote] ensure offline examples use the target dialect (L401); [nit, 1 vote] clarify storage diff status 2 semantics (L395); [nit, 1 vote] explain that diff output may not reveal bootstrap failure causes (L450); [nit, 1 vote] limit confirmation guidance to runnable DDL (L450); [nit, 1 vote] state that apply clears only eligible additive changes (L456); [nit, 2 votes] state that storage apply shares the startup timeout (L464); [nit, 3 votes] distinguish PostgreSQL and MySQL index names at L491/L499/L507; [nit, 3 votes] label the PostgreSQL webhook index example (L516); [nit, 1 vote] limit contributor guidance to automatic changes and document manual remediation (L533).
docs/release.md Adds release guidance. Final findings: [nit, 1 vote] warn that MySQL index additions may require pre-creation because table copies can exceed startup limits (L267); [nit, 2 votes] require a selector for storage diff and distinguish it from storage apply (L281); [nit, 3 votes] limit index-removal guidance to MySQL (L285).
docs/configuration.md Replaces detailed guidance with a pointer. Final finding: [nit, 2 votes] qualify the claim that storage DDL is never applied manually (L902).
docs/cli.md Adds the storage-schema guide to CLI navigation. No final findings.
docs/.toc-manifest Registers the guide for TOC generation. No final findings.
Review details

Suppressed comments (21)

docs/release.md:270

  • This new release guidance only calls out PostgreSQL pre-creation, while the preceding MySQL sentence says additive changes need no action. MySQL index additions are automatic but run as a Spirit table copy and can exceed the startup budget on long-lived tables, so this paragraph should also tell release authors to include a pre-creation statement for such indexes rather than implying every additive change is safe to leave to boot.
needs manual remediation — `NOT NULL` without a `DEFAULT`, generated or
identity, `UNIQUE`, `REFERENCES` with a `DEFAULT` — fails startup until an
operator creates it by hand, so it always belongs in the release notes with its
statement (see [storage-schema.md](./storage-schema.md)). A destructive

docs/storage-schema.md:73

  • This final node promises an exact match, but that is not what either default path guarantees: MySQL refuses surplus tables/columns under AV-9, while PostgreSQL deliberately leaves extra objects in place. The diagram therefore contradicts the later dialect sections; describe additive changes being applied while unhandled surplus state can remain.
                  the storage database, converged to the
                  schema of the binary that ran it, and to
                  nothing else

docs/storage-schema.md:499

  • This is the PostgreSQL index name, but MySQL's embedded schema declares idx_created_id for apply_operations (pkg/schema/mysql/apply_operations.sql:30). A MySQL operator following this example will not pre-create the index the embedded schema expects; provide dialect-specific statements.
CREATE INDEX idx_apply_operations_created_id ON apply_operations (created_at, id);

docs/storage-schema.md:507

  • This is the PostgreSQL index name, but MySQL's embedded schema declares idx_external_id for apply_operations (pkg/schema/mysql/apply_operations.sql:31). A MySQL operator following this example will not pre-create the index the embedded schema expects; provide dialect-specific statements.
CREATE INDEX idx_apply_operations_external_id ON apply_operations (external_id);

docs/storage-schema.md:457

  • This says storage apply always clears the statements printed by the recovery diff, but MySQL destructive statements are intentionally left behind and PostgreSQL manual-remediation entries abort before DDL. Qualify this as clearing only eligible additive changes and explain which remaining items still need operator action.
`storage apply` clears them under the same advisory lock the pods are contending
for, and the pod starts on its next backoff.

docs/storage-schema.md:177

  • This rollback warning is true only for the MySQL Spirit differ. PostgreSQL's bootstrap is explicitly additive-only and preserves extra indexes (as stated above), so an older PostgreSQL pod does not remove a pre-created index. Qualify this asymmetry as MySQL-specific to avoid contradicting the PostgreSQL guidance.
**One asymmetry bites operators, and it is worth memorizing:** dropping an index
destroys no data, so it is *not* destructive and *not* refused. A table or
column you create ahead of a roll survives a boot of the still-running older
release. **An index does not.** Any boot of the earlier release converges a

docs/storage-schema.md:438

  • Rollback behavior differs by dialect. MySQL reports newer tables and columns as refused destructive changes and exits 2, but PostgreSQL ignores extra objects by design and reports converged against the older desired schema. This paragraph needs a MySQL qualifier and a dialect-aware gate recommendation.
During a rollback window the diff reports the newer release's tables and columns
as refused destructive statements and exits 2. That is the expected steady state
rather than drift: the surplus state is deliberate, and it is what lets the
release be rolled forward again. A pre-deploy gate keyed on exit status 0 will
flag it, which is the correct signal to pause on.

docs/storage-schema.md:289

  • This parenthetical gives only the MySQL path even though --schema-dir supports both storage dialects. A PostgreSQL operator following this section could choose the wrong files; name both examples, such as pkg/schema/mysql and pkg/schema/postgres, and say to match the live dialect.
  dialect directory (`pkg/schema/mysql`), not at its parent. A directory with no

docs/storage-schema.md:319

  • The --release path makes this statement false: the data plane reports a diff against the supplied release files, but its next boot uses its own embedded schema and would not run those reported statements. The data plane is authoritative for the live database only; say that the named release's binary must perform convergence of the desired schema.
also what makes the answer trustworthy, since the binary that reports the diff
is the binary whose next boot would run it.

docs/storage-schema.md:32

  • This is too absolute: MySQL can destroy storage state when allow_destructive_schema_changes or --allow-destructive is enabled, as documented below. Qualify the safety claim as the default behavior so the introduction does not contradict the supported opt-in.
conservative: it is additive, it decides before it writes, and it never
destroys state a peer on another release might still be reading.

docs/storage-schema.md:401

  • This offline example hard-codes the MySQL schema directory even though this guide covers PostgreSQL too. With PostgreSQL storage, passing pkg/schema/mysql reads the wrong DDL and cannot answer the release's schema; tell operators to use the directory matching the target dialect, including pkg/schema/postgres for PostgreSQL.
   schemabot storage diff --schema-dir ./pkg/schema/mysql --dsn "$STORAGE_DSN"

docs/storage-schema.md:263

  • This API example has the same renderer mismatch: with the default repository, the report source is the schema files of release v1.4.0, not ... in block/schemabot. Remove the repository suffix or make the command explicitly use a non-default --release-repo.
schemabot on db-1.example (mysql), deployment west in production needs 1 statement: 1 outstanding, against the schema files of release v1.4.0 in block/schemabot.

docs/storage-schema.md:356

  • This destructive-diff output is likewise not generated by the shown default-repository command. The actual source description omits in block/schemabot; keep the example aligned with the renderer or pass a non-default --release-repo.
schemabot on db-1.example (mysql) needs 1 statement: 1 destructive, against the schema files of release v1.4.0 in block/schemabot.

docs/storage-schema.md:143

  • The PostgreSQL drift check matches indexes by name and validates only indisvalid plus required uniqueness; it does not compare the live index's columns. A valid same-named index over the wrong columns is therefore treated as converged, so this section should state that limitation rather than implying the embedded index shape is verified.
A live index only counts as present when PostgreSQL reports it valid.
PostgreSQL marks an index invalid both while a `CREATE INDEX CONCURRENTLY` is
still building it and after one fails part-way — a unique build that hits
duplicate keys, a cancelled session — and in either case the planner never uses
it. Startup fails closed naming that index rather than reading it as converged
or colliding with it on a fresh `CREATE INDEX`, and reads
`pg_stat_progress_create_index` to say which situation it is:

docs/storage-schema.md:68

  • On MySQL, a no-change plan is followed by a staleSpiritTableNames check; leftover Spirit shadow tables trigger the advisory lock and cleanup even when no schema DDL is outstanding. The diagram's unconditional “nothing outstanding? done, without taking a lock” is therefore inaccurate; qualify the fast path as requiring no stale Spirit tables too.
                   │  1. diff the live catalog        │
                   │     against the embedded files   │
                   │  2. nothing outstanding? done,   │
                   │     without taking a lock        │
                   │  3. take the advisory lock       │
                   │  4. re-diff, holding it          │
                   │  5. refuse what it must not run  │
                   │  6. run what is left             │

docs/storage-schema.md:90

  • The five-minute value is the EnsureSchema timeout for one convergence attempt, not the whole server boot: bootStorage retries failed attempts for up to eight minutes. Describing the startup path as having a single hard five-minute budget omits that retry behavior; state the per-attempt limit and the separate boot retry budget so operators do not misread incident timing.
- **Startup convergence is bounded.** The startup path runs inside a hard
  five-minute budget before the pod serves traffic. A change that cannot finish
  in it keeps the pod out of service, which is why large index builds belong
  ahead of the roll rather than inside it.

docs/storage-schema.md:398

  • storage diff returns status 2 when the report contains refused destructive statements or manual-remediation entries, even though the release's normal boot will not run a refused destructive statement. Thus status 0 is not merely “the boot has nothing to do”; this step should say that status 2 requires inspecting the report, including the expected rollback-window case documented below.
   Exit status 0 means that release's boot has nothing to do and the rest of
   this does not apply. Where the tag cannot be fetched — no network to the
   repository, or a commit that was never tagged — a checkout of it answers the
   same question with no network at all:

docs/storage-schema.md:454

  • A direct storage diff is only a read-only catalog comparison; it is not guaranteed to print the cause of a failed bootstrap. Lock acquisition, stale Spirit-table cleanup, DDL execution, parser, and re-verification failures can leave the diff empty or fail before it produces statements. Reword this as the DDL still reported as outstanding and direct operators to the pod logs for the failure cause.
The statements it prints are the ones the pod is failing on. With no network to
the repository, `--schema-dir` reads the same files from a checkout of that
release; with neither, `storage apply --dsn "$STORAGE_DSN"` prints the
statements the binary in your hand would run and waits for a `yes` before
running any of them.

docs/storage-schema.md:536

  • This contributor guidance is too absolute for PostgreSQL: a new column with a manually remediated shape does require an operator-run DDL statement and release-note/runbook instructions, as this guide explains above and release.md requires. Limit the “next deploy picks it up”/“no DDL to write” claim to changes classified as automatic, and point unsafe column shapes to the manual-remediation procedure.
For contributors: adding a table or a column to SchemaBot's storage means adding
or editing a file under `pkg/schema/mysql/` and its counterpart under
`pkg/schema/postgres/`. The next deploy picks it up — there is no schema
directory to ship and no DDL to write into a runbook. Schema parity tests pin

docs/storage-schema.md:165

  • The description of MySQL refusals names only DROP TABLE and DROP COLUMN, but the actual Spirit unsafe classification also refuses storage diffs containing DROP PRIMARY KEY, partition drops/truncation, and other data-loss operations (see pkg/api/ensure_schema.go:407-414 and its tests). Since this section is the operator's list of what is never automatic, make the examples explicitly non-exhaustive or cover the other supported unsafe forms.
On MySQL, destructive statements in the diff — `DROP TABLE`, or an `ALTER TABLE`
containing `DROP COLUMN` — are refused and skipped by default. A mixed `ALTER
TABLE` is split: its additive clauses still execute and only the destructive
clauses are refused, except that a clause which cannot run without a refused
clause (the `ADD PRIMARY KEY` half of a primary-key change) is refused with it.

docs/storage-schema.md:454

  • storage apply does not always wait for confirmation: its preview returns immediately with an error when PostgreSQL reports manual-remediation entries, before confirmAction is called. Qualify this fallback as applying only to runnable outstanding DDL; for manual entries, resolve the named condition first.
The statements it prints are the ones the pod is failing on. With no network to
the repository, `--schema-dir` reads the same files from a checkout of that
release; with neither, `storage apply --dsn "$STORAGE_DSN"` prints the
statements the binary in your hand would run and waits for a `yes` before
running any of them.
  • Files reviewed: 6/6 changed files
  • Comments generated: 11
  • Review effort level: Lite

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

Comment thread docs/configuration.md Outdated
Comment thread docs/release.md Outdated
Comment thread docs/release.md Outdated
Comment thread docs/storage-schema.md Outdated
Comment thread docs/storage-schema.md Outdated
Comment thread docs/storage-schema.md Outdated
Comment thread docs/storage-schema.md Outdated
Comment thread docs/storage-schema.md Outdated
Comment thread docs/storage-schema.md Outdated
Comment thread docs/storage-schema.md Outdated
@aparajon
aparajon force-pushed the armand/storage-schema-guide branch from efa616a to 0391cb2 Compare September 12, 2026 06:56
@aparajon
aparajon force-pushed the armand/storage-schema-guide branch from 0391cb2 to 2a1a4e3 Compare September 12, 2026 07:00
@aparajon
aparajon marked this pull request as ready for review September 12, 2026 07:15
@aparajon
aparajon force-pushed the armand/storage-schema-guide branch from 2a1a4e3 to 92614d7 Compare September 12, 2026 17:36
@aparajon
aparajon removed this pull request from stack #1397 September 12, 2026 21:00
@aparajon
aparajon added this pull request to stack #1408 September 12, 2026 21:00
@aparajon
aparajon force-pushed the armand/storage-schema-guide branch from 92614d7 to 0e9063e Compare September 12, 2026 21:45
@Kiran01bm

Copy link
Copy Markdown
Collaborator

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1396, 0e9063e.
Verdict: 1 finding — 1 non-blocking (miscounted list lead-in).

Non-blocking

The "Three consequences worth holding onto" lead-in is followed by four bullets. docs/storage-schema.md#L80 promises three, but bullets land at L82, L86, L91 and L99. A reader who counts to three stops at "Convergence is bounded…" and misses "A converged storage costs one diff" — the bullet that explains why the lock-free fast path is safe and why a converged pod never contends. Either say "Four" or fold the last bullet in.

The one thing that could have broken, verified

The guide's exit-code contract is the part an operator will script against, so a wrong number would silently break automation. Checked against the code rather than the prose: ExitStorageSchemaOutstanding = 2, ExitCodeFor falls through to 1, and StoragePlanCmd.Run returns nil when report.Converged — giving exactly the documented 0/2/1. The storage apply table holds too: storageSchemaConvergenceOutcome errors only when len(remaining.Manual) > 0, and both a declined confirmAction and refused destructive statements return nil, so exit 0 as documented.

Verified correct

  • Index-name table (L578-L581) matches pkg/schema/{postgres,mysql}/*.sql exactly for all eight indexes.
  • Every cross-doc anchor resolves: auth.md#what-read-and-write-access-include, configuration.md#allow_destructive_schema_changes, AV-9 in invariants.md, plus all four intra-file anchors.
  • configuration.md's auto-generated TOC is H2-only, so the new ### allow_destructive_schema_changes heading correctly needs no entry — consistent with the green Docs TOC check.
  • The new TOC lists exactly the 11 H2 headings in file order, and docs/.toc-manifest inserts the file in correct sort position.
  • "A binary converges its own schema, never another's" holds: SchemaDir/Release are hidden flags refused by storageSchemaSourceRefusal, and neither the apitypes nor the proto request carries schema files.
  • The five-minute budget claim holds on every path: EnsureSchemaTimeout = 5 * time.Minute is applied inside EnsureSchema via its own context, and operatorHTTPClient's 15-minute allowance does not cut it short.

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

@Kiran01bm Kiran01bm left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@aparajon
aparajon force-pushed the armand/storage-schema-guide branch from 0e9063e to 794d123 Compare September 14, 2026 18:16
@aparajon
aparajon force-pushed the armand/storage-schema-guide branch from 794d123 to adf6352 Compare September 14, 2026 19:05
@aparajon

Copy link
Copy Markdown
Collaborator Author

🤖 Addressed in adf63523, and the branch is now rebased onto 7cb9fccc.

The "Three consequences worth holding onto" lead-in is followed by four bullets. Fixed by saying "Four" rather than folding the last bullet in — your reasoning for which way to resolve it decided that: a reader who counts to three stops at "Convergence is bounded…" and misses the bullet explaining why the lock-free fast path is safe, which is the one worth keeping as its own item.

The same commit fills a related gap the review pass on #1395 turned up, in the storage apply walkthrough rather than in the list: a preview that finds nothing outstanding still offers the run, and taking it is worth a moment, because the bootstrap converges more than the catalog. It also clears the schema change engine's leftover tables, which outlive an interrupted convergence and which a catalog diff cannot see. The guide now says so, so an operator reading "nothing is outstanding" does not read it as "nothing to do".

Thanks for checking the exit-code contract against the code rather than the prose. That turned out to be load-bearing in the other direction too: ExitCodeFor had no caller in main at the time, so the numbers the guide documented were right about intent and wrong about behaviour until #1406's fix landed. The guide's 0/2/1 now matches what the binary actually does.


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

@aparajon
aparajon force-pushed the armand/storage-schema-guide branch from 6c54562 to cc69f40 Compare September 16, 2026 22:54

@morgo morgo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Approving on Morgan's behalf (agent review).

Fresh review of the whole PR at 590ae1f7 — @Kiran01bm's approval SHA a3ea9ed4 was force-pushed away, so there was no drift baseline to review against and I had not reviewed this one before. Docs-only: 6 files, +764/-138, against base armand/storage-schema-cli (#1395, which I re-approved at 3bd164fe).

The configuration.md rework is a relocation, not a deletion. That was the thing worth checking — -160 lines out of a settings doc. Every operational instruction landed in docs/storage-schema.md: all four CREATE INDEX statements and the webhook_events ALTER, the invalid-index recovery (pg_stat_progress_create_index, the pg_read_all_stats caveat, REINDEX INDEX CONCURRENTLY), the mixed-ALTER split rule including the ADD PRIMARY KEY half, and the five-minute startup budget. What stays behind in configuration.md is just the allow_destructive_schema_changes setting plus a pointer, which is the right split.

Claims checked against code rather than taken from the prose:

  • The exit-status contract. storage plan returning 0/2/1 is real: pkg/cmd/commands/storage_schema.go:152 returns nil on report.Converged and otherwise exitStorageSchemaOutstanding(), which is &ExitCodeError{Code: 2, Err: ErrSilent} (pkg/cmd/commands/exit_code.go:20,57), and ExitCodeFor defaults everything else to 1. The apply table's easiest-to-get-wrong row is also right: declining at the prompt is if !confirmed { return nil } at storage_schema.go:327, so it really is exit 0.
  • Every flag the doc invokes exists with the semantics stated, including the one that would be most embarrassing to get wrong — storage apply takes no --release or --schema-dir (they are hidden:"" and refused by storageSchemaSourceRefusal), and the doc correspondingly only ever runs it from the new release's binary. --allow-unsafe's "widens this policy and never narrows it" is verbatim from the flag's own help string.
  • The drop-index asymmetry is correct, and it is the most valuable paragraph here. docs/storage-schema.md:190 and the new paragraph in docs/release.md both claim a pre-created index on MySQL is converged away by any boot of the still-running earlier release. Checked at the exact pinned spirit commit (v0.17.1-0.20260908172838-1ab2595e45a1): 1ab2595e:pkg/lint/lint_unsafe.go classifies both *ast.DropIndexStmt and ast.AlterTableDropIndex as safe — "since none lose data we consider them safe" — so UnsafeStatement returns false and partitionDestructiveChanges never refuses it. The warning is real, and it is exactly the kind of thing operators only learn the hard way.

Links: all 11 internal anchors in storage-schema.md resolve against its own headings, both cross-file anchors into it resolve (#what-is-never-automatic, #deploying-a-release-that-changes-the-storage-schema), and configuration.md#allow_destructive_schema_changes resolves against the heading this PR adds.

CI at 590ae1f7: 18 SUCCESS, 15 SKIPPED, no failures — the skips are the code jobs the paths filter drops on a docs-only change, which is what should happen here.

One precision nit, not blocking. The apply exit table lists "a change needs manual remediation, so nothing ran | non-zero", and the sentence below generalizes to "Both non-zero refusals mean the same thing: nothing converged". That holds for the pre-gate refusal (blockManualStorageApply), but storageSchemaConvergenceOutcome at storage_schema.go:465 also returns non-zero when manual changes are found at convergence time, after other statements have already run — and on PostgreSQL convergence is per-table transactional, so that is a partial apply, not "nothing converged". It needs concurrent DDL between the preview read and the convergence, so it is narrow, but the doc is explicitly written for scripts keyed on the status and that is the one sentence a script author could act on wrongly.

One stack note, no action: docs/.toc-manifest here still lists docs/spirit_progress.md, because this branch predates #1376's deletion of that file. Whichever of the two lands second will need the manifest reconciled.

No github.com/squareup/ references in any added line — public-repo separation clean.

Base automatically changed from armand/storage-schema-cli to main September 17, 2026 16:36
@aparajon
aparajon force-pushed the armand/storage-schema-guide branch from 590ae1f to fe55852 Compare September 17, 2026 16:36
@aparajon

Copy link
Copy Markdown
Collaborator Author

🤖 "What converges by itself" now leads with what a boot does, not with how it does it

Pushed after both approvals, so flagging what changed: bc15524d rewrites that one section. It explained the two dialects in the order the code runs, which left the reader to assemble the answer they came for, namely whether this release's drift needs anything from them. It now opens with a grid and keeps only the prose that says what to do:

  the boot finds                  MySQL             PostgreSQL
  ────────────────────────────────────────────────────────────────────────────
  a table it is missing           creates it        creates it
  a column it is missing          adds it           adds it, or stops the boot
  an index it is missing          builds it         builds it
  a column defined differently    alters it         does not see it
  a surplus table or column       refuses it        leaves it
  a surplus index                 drops it          leaves it
  an index it cannot use          does not arise    stops the boot

64 lines to 51, and the implementation detail is gone: transaction boundaries, when the advisory lock is taken relative to the diff, and which catalog view reports a build in progress. What an operator has to act on stays, including the MySQL index copy, the PostgreSQL column shapes that stop a boot, and what to do about an index a failed build left behind.

Each MySQL cell is checked against Spirit's UnsafeLinter rather than inferred: it classifies DROP INDEX, RENAME, and MODIFY/CHANGE COLUMN as safe, so a surplus index is dropped and a redefined column is altered on the next boot today.

Two follow-ups, so neither is a surprise:


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

aparajon and others added 12 commits September 18, 2026 14:58
…ve storage DDL

The guide and the configuration reference both named a flag spelled only in
this feature. The convergence takes --allow-unsafe, the same flag `schemabot
apply` takes for destructive changes.

The config key keeps its name: allow_destructive_schema_changes is shipped
configuration, and the surrounding prose already distinguishes the standing
policy from the flag that widens it for one invocation.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
storage apply now refuses to run anything until a destructive statement
is permitted, so the guide's exit-status table no longer has an outcome
where one is left refused and the run still succeeded. The startup
bootstrap still skips and continues, and the guide says why the two
differ.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three console transcripts carried reason strings no code path emits. The
destructive refusals showed "DROP TABLE destroys data", a phrase that exists
only in a unit-test fixture; the real line is the engine linter's own message,
carried through untouched. The PostgreSQL manual-remediation example dropped
the column name and the remedy, which are the two facts an operator needs in
order to write the DDL by hand — so the guide made the CLI look less actionable
than it is.

An operator grepping the documented phrase out of the output, or out of a
--json report, matched nothing.

Also: PostgreSQL index names are unique per schema rather than per database,
the interactive transcript was missing one of the two blank lines the summary
and the prompt each print, and the release checklist said MySQL additive
changes need no action — true of a table or a column, but an index add runs as
a table copy inside the startup budget, which is exactly why the guide says to
pre-create it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ocal hosting

Two claims in the guide were stated for every dialect and are not.

The leftover engine tables a converged run clears are a MySQL thing: the
PostgreSQL bootstrap has none to clear, and a PostgreSQL operator taking the run
for the stated reason got a no-op. What it does do there is re-check the shape
of every storage table — each expected index present, valid, and unique where
the schema requires it — which a column diff does not cover, so that is what the
guide now says for PostgreSQL.

And --allow-unsafe does not widen the policy everywhere: a locally hosted server
never runs destructive storage statements and refuses the request that opts in
(AZ-6), so the flag's description carries the carve-out and the reason for it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The three `storage plan` renderings carried the hint the CLI no longer
prints, which told an operator to converge by letting a release's boot do it.
Converging ahead of the roll is what this guide's own pre-deploy sequence
covers, and it names the command rather than the boot.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…w does

The destructive and manual examples showed a notice with no statement
above it. A storage plan renders the whole difference between the two
schemas and counts it, so the examples show that, and the prose says
the summary is the difference rather than a promise of what will run.
The section explained the two dialects' convergence in the order the code
does it, which left an operator to work out the one thing they are reading
for: whether this release's drift needs anything from them. Lead with a
grid of what a boot does with each kind of drift on each dialect, and keep
only the prose that tells an operator what to do about it.
The guide had grown to 693 lines, and most of the growth was rationale: why
each rule is the way it is, restated where the rule itself already said it.
Nobody reads 693 lines during a failed roll.

This is the same guide at 399 lines. Every rule, refusal, exit status, and
pre-creation statement survives; what went is the second telling of each one,
the contributor section that AGENTS.md already carries, and two console
transcripts whose renderings are one sentence of prose apart from the two that
remain. The plan and the destructive-apply refusal keep their transcripts,
because those are the two screens an operator has to read under pressure.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@morgo morgo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Reviewed on Morgan's behalf by his AI agent.

Docs-only, six files, no code. I read docs/storage-schema.md end to end and checked its
falsifiable claims against main. Most of it verifies exactly:

  • All eight pre-creation index statements match the embedded schema, names and column
    lists, in both dialect directories (pkg/schema/{mysql,postgres}/{plans,apply_operations,webhook_events}.sql).
  • The five-minute budget is EnsureSchemaTimeout = 5 * time.Minute (pkg/api/ensure_schema.go:41).
  • The storage plan exit-status contract (0 / 2 / 1) matches StoragePlanCmd.Run.
  • Every row of the storage apply exit table matches StorageApplyCmd.Run: the declined
    prompt really is return nil → 0, and both blockManualStorageApply and
    blockDestructiveStorageApply run on the --auto-approve path too, so "skipping the prompt
    is not permitting a DROP" is accurate.
  • The rendered refusal block matches WriteUnsafeChangesBlocked (pkg/cmd/internal/templates/plan.go:561).

One blocking issue: the guide's most emphasized warning is now inverted.

#1390 merged today (2026-09-18T18:44Z, 89532247) and moved the bootstrap's unsafe verdict from
pkg/ddl's text vocabulary to the engine plan's linter verdict. Before it, "unsafe" meant loses
data
— UnsafeStatement's doc comment named "DROP TABLE or an ALTER TABLE clause like DROP COLUMN
or DROP PARTITION", and index removal was not in that set, so a surplus index was dropped. After it,
partitionDestructiveChanges says the opposite in so many words:

An index the fleet's live queries plan around is as load-bearing as a column: dropping it destroys
no rows and completes in milliseconds because it is metadata-only, and it can still take the
storage database down. The registry's error set spans both, so the same gate covers both.

and AV-9 — which this guide cites as its authority — was rewritten in the same commit to

a statement that loses data and one that removes a schema object without losing any are both
refusals … a table, a column, or an index the fleet's live queries plan around.

So a surplus index is now refused, not dropped. Four places in the guide say otherwise:

  1. docs/storage-schema.md:91 — the dialect table row a surplus index │ drops it │ leaves it.
    Should read refuses it, which also makes the MySQL column uniform for table/column/index.
  2. :132-136 — "One asymmetry bites operators, and it is worth memorizing: dropping an index
    destroys no data, so it is not destructive and not refused. … An index does not. Any boot
    of the earlier release converges a newly created index away without comment". This is the
    paragraph an operator is most likely to retain, and it is now backwards: a pre-created index
    survives a boot of the older release exactly as a table or column does. There is no asymmetry
    left to memorize.
  3. :315-319 — deploy step 3 ("On MySQL, if you converged ahead of the roll, re-check right before
    it … an index does not") is a step that no longer needs doing on either dialect.
  4. :330-331 — "the plan reports the newer release's tables and columns as refused destructive
    statements": now tables, columns, and indexes. Same root cause, and worth fixing because the
    rollback-window paragraph is the one that tells an operator the exit-2 is expected rather than drift.

This was correct when written — main moved under the PR six hours ago. Nothing here is a mistake in
the writing; it just has to be re-checked against 89532247 before it lands. The PR is DIRTY
regardless (#1390 also edited docs/configuration.md and docs/release.md, both of which this PR
rewrites), so the rebase is happening anyway and is the natural place to do it.

Not blocking, one nit: :174-180 says the storage plan exit status is 2 when statements are
outstanding. After #1390 a refused statement is outstanding-and-will-never-run, whereas before,
part of that set would converge on the next boot. The status is still right, but the guide might be
clearer for a pre-deploy gate if it distinguished "outstanding, converges on next boot" from
"outstanding, refused until an operator acts" — the section already has the vocabulary for it.

Holding on the four items above. Everything else I checked holds up, and the guide is a genuine
improvement over what it replaces in configuration.md.

The guide, the setting, and the release checklist each described a
bootstrap that refused only statements losing data, and told operators
in as many words that dropping an index is not destructive and not
refused. A boot refuses it now, so the guidance built on that asymmetry
pointed the wrong way: an operator pre-creating an index was told to
expect it gone.

Each surface now names both kinds of destructive statement and what a
statement carrying a removal beside an addition does. The caveat the
asymmetry earned survives in the form that is still true: the refusal
lives in the booting binary, so a release from before indexes were
protected still converges a pre-created index away, and the gap is worth
re-checking while any such release is running.
@aparajon
aparajon force-pushed the armand/storage-schema-guide branch from afe5a8c to 28a2622 Compare September 18, 2026 19:02
… means

Three places still described the bootstrap dropping a surplus index. The
dialect table said so in one word, which is the version a reader scanning
for the answer takes away, and the rollback-window paragraph listed only
tables and columns among the refusals an operator should expect to see
standing.

Exit 2 also covers two situations a pre-deploy gate must not treat
alike, and the split matters more now that refusals cover index removal
as well. The report already separates them: statements under
`outstanding` converge on the next boot, statements under `destructive`
never will. A gate reading only the exit status waits on a convergence
that is not coming.
@aparajon

Copy link
Copy Markdown
Collaborator Author

🤖 Fixed in 28051b54, on top of 28a2622a which rebased onto 89532247. You caught this at the same time I did, and you caught two I didn't.

Items 2 and 3 were already fixed in the rebase commit. Items 1 and 4 were not: I swept the prose for the inverted claim and missed the one-word cell in the dialect table and the "tables and columns" list in the rollback-window paragraph. The table cell was the worse miss — it's the version a reader scanning for the answer actually takes away.

I took the nit too. #1390 makes it bite harder, since the refused set now includes index removal, and the report already separates the two: outstanding converges on the next boot, destructive never does. A gate keyed only on the exit status waits on a convergence that is not coming, which is now spelled out.

The asymmetry paragraph is not simply deleted, because a true version survives: the refusal lives in the booting binary, so a release from before #1390 still converges a pre-created index away. That's the part that matters while a mixed fleet is rolling.

Replied by Claude Code (claude-opus-5).

aparajon and others added 3 commits September 18, 2026 15:40
The dialect table read `a surplus table or column │ refuses it`, under
four rows that all name an additive action. The parallel made it land as
an addition being refused, and "refuses it" takes a statement as its
object anyway, not a table.

Both rows now state what becomes of the object, which is what an
operator is scanning for and can only be read one way: MySQL keeps it
and warns, PostgreSQL keeps it silently. The sentence below the table
carried the refusal vocabulary alone, so it now says where the warning
comes from and why the other dialect has none.
"Surplus" named the rollback case in the dialect table and nowhere defined it,
so a reader met the word cold in the one place they were scanning for an
answer. Name the rows for what they are — a table, column, or index the
booting binary does not declare — and follow the table with the definition and
a rollback walked through end to end.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three validation errors described the danger as reporting every storage table
"as surplus" — the same undefined word the guide just stopped using, and here
it reaches an operator at a terminal with no definition in reach. Say the
consequence instead: the diff would propose dropping every existing storage
table. The refusals are unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@aparajon
aparajon merged commit 7735f20 into main Sep 18, 2026
43 checks passed
@aparajon
aparajon deleted the armand/storage-schema-guide branch September 18, 2026 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants