fix(api): apply safe clauses of a mixed destructive storage-schema ALTER

[Kiran01bm]

Summary

EnsureSchema now splits a refused mixed ALTER by clause so its non-destructive clauses still apply, instead of silently skipping them along with the destructive ones.

Fixes #1094.

Why

Spirit's schema diff emits one combined ALTER per table. When that statement mixed additive clauses (e.g. ADD COLUMN) with destructive ones (e.g. DROP COLUMN of surplus state from a newer binary), the whole-statement refusal skipped the additive clauses too — the starting binary then ran without a column it requires, and the only signal was a refusal warning naming the full statement.

What

• New ddl.SplitUnsafeAlter: parses a single ALTER, classifies each clause through ddl.UnsafeStatement (Spirit's UnsafeLinter stays the single authority on the unsafe vocabulary), and restores separate safe/unsafe statements. The safe partition is re-classified before use; any parse/partition uncertainty is an error.
• partitionDestructiveChanges splits an unsafe ALTER: the safe statement executes, only the destructive remainder is refused (logged and counted as before). Purely destructive statements keep the existing refuse-whole, log-and-continue behavior; a failed split fails startup rather than executing unclassified DDL.
• Unit tests pin the split boundary; a new integration test covers same-table drift in both directions (missing required column + surplus column).

Before                                     After
┌───────────────────────────────┐          ┌───────────────────────────────┐
│ ALTER `tasks`                 │          │ ALTER `tasks`                 │
│   ADD COLUMN `required`,      │          │   ADD COLUMN `required`  ──▶ applied
│   DROP COLUMN `surplus`       │          │   DROP COLUMN `surplus`  ──▶ refused,
│         │                     │          │                              logged +
│         ▼                     │          │                              counted
│ refused whole:                │          └───────────────────────────────┘
│ ADD silently skipped,         │
│ binary runs without           │
│ a required column             │
└───────────────────────────────┘

[Copilot]

Pull request overview

Updates MySQL schema reconciliation to apply safe clauses from mixed destructive ALTER TABLE statements while refusing destructive clauses.

Changes:

• Added AST-based ALTER clause partitioning.
• Integrated safe/unsafe handling into EnsureSchema.
• Added unit and integration coverage.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Review summary
pkg/ddl/unsafe.go	Implements safe/unsafe ALTER splitting.
pkg/ddl/unsafe_test.go	Tests clause partitioning.
pkg/api/ensure_schema.go	Critical (2 votes): dependency-coupled clauses may be split into non-executable statements; stale documentation also needs updating.
pkg/api/ensure_schema_test.go	Tests schema partitioning behavior.
pkg/api/ensure_schema_integration_test.go	Moderate (2 votes): cleanup uses a canceled context, potentially leaving containers running.

---

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

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 Adversarial correctness review, requested by @aparajon and performed by their agent. Reviewed at head c14ddb14, in a worktree, with the unit and both new integration tests run locally.

Verdict: clean — nothing blocks. The split is well-constructed and the part I expected to break didn't: re-rendering the safe partition from the AST rather than slicing the input text is the right choice, and I couldn't find a column attribute it loses. Delegating the vocabulary entirely to Spirit's linter, then re-classifying the safe partition before returning it, means the split can only ever narrow what executes. One thing worth acting on, and it is about the failure path rather than the happy one: when a split can't be performed this crash-loops startup, which is the one outcome the surrounding design exists to prevent.

#	Finding	Severity
1	A failed split crash-loops startup, defeating the reason this path doesn't fail closed	correctness / operability
2	The refusal metric can't distinguish a split refusal from a whole one	nit

---

1. The one failure this path is designed never to have

partitionDestructiveChanges turns a SplitUnsafeAlter error into a hard error, so EnsureSchema fails and the pod does not start. The comment justifies it as "classification or partitioning uncertainty must never widen what the bootstrap will execute" — but the function's own doc, twenty lines up, gives the reason this whole path is a deliberate exception to fail-closed:

failing here would crash-loop every pod running an older binary during a rolling deploy or rollback where a storage table or column was legitimately removed

Refusing the statement whole satisfies both. It executes strictly less than the split would, so it cannot widen anything, and it is exactly the behavior that shipped before this PR — a known-good fallback rather than a new one.

The trigger isn't hypothetical, and your own comment names it. The re-classification guard exists because "the linter is an external authority whose rules may grow cross-clause reasoning." If a future Spirit bump adds a cross-clause unsafe rule that fires on some safe partition, that guard returns an error, and every pod with that mixed ALTER pending crash-loops at startup — during exactly the rolling deploy the gate was built to survive. A Spirit version bump is a routine change; a startup crash-loop across the fleet is not a routine consequence for one.

It also isn't pinned. I replaced the error return with the whole-refusal fallback and the suite stayed green:

return nil, nil, fmt.Errorf("split unsafe storage schema change ...")
  → safeDDL, unsafeDDL = "", ""      // falls through to refuse-whole
go test ./pkg/ddl/... ./pkg/api/...  → green

So whichever way you decide, the decision deserves a test that states it.

2. A split refusal and a whole refusal look identical to the metric

RecordStorageSchemaDestructiveRefusal(ctx, r.change.Table, ddl.StatementTypeToOp(r.change.Operation)) gets alter_table in both branches. The new log line distinguishes them nicely — split_from_ddl is a genuinely good field, and phrasing the message so it states what will happen reads well during a page. The counter behind it can't tell an operator whether the safe clauses ran. Same operator action either way, so this is a nit rather than a gap; if you do add it, a reason-style attribute on the existing counter is the cheaper shape than a second instrument.

---

Action items

1. (Finding 1) Fall back to refusing the statement whole when SplitUnsafeAlter fails, rather than failing startup — and pin whichever behavior you choose with a test.
2. (optional, Finding 2) Distinguish split from whole refusals on the existing refusal counter.

---

Verified — tried to break, couldn't

The coupled-clause question, which is where I expected to find something. Copilot raised DROP PRIMARY KEY, ADD PRIMARY KEY; that one is handled, and mutating the coupling off kills three tests across both layers. So I went looking for the other pairs — a clause that is safe alone but invalid while the refused clause's object still exists. The answer is that the unsafe vocabulary is narrow enough that they can't arise:

Clause	Unsafe?
DROP PRIMARY KEY	yes
DROP COLUMN	yes
DROP INDEX / DROP KEY	no
DROP FOREIGN KEY	no
ADD PRIMARY KEY, ADD COLUMN, ADD INDEX, RENAME COLUMN, MODIFY COLUMN	no

Because DROP INDEX and DROP FOREIGN KEY are safe, a same-name index or constraint redefinition keeps both halves in the safe partition together — I confirmed DROP INDEX idx_a, ADD INDEX idx_a (...), the UNIQUE variant and the FK variant all split to a safe partition containing both clauses and an empty unsafe partition. ADD COLUMN b AFTER a, DROP COLUMN a also behaves: the add runs while a is still there. That leaves DROP COLUMN a, ADD COLUMN a as the only orphaning shape, and Spirit's differ can't emit it — a column present on both sides produces MODIFY, not a drop plus an add.

The AST round trip preserves everything I could throw at it. The safe partition is restored from the parser, not sliced from the text, so I checked the attributes SchemaBot's storage schema actually uses: CHARACTER SET/COLLATE, NOT NULL, DEFAULT (string, numeric, CURRENT_TIMESTAMP(6) with ON UPDATE), COMMENT, AUTO_INCREMENT, ENUM members, DECIMAL(18,6) precision, generated columns with STORED, functional index expressions, UNIQUE KEY, and a trailing ALGORITHM=. All survive semantically. The differences are cosmetic normalizations — _UTF8MB4'x' introducers, CHARACTER SET UTF8MB4 uppercased, ->> expanded to JSON_UNQUOTE(JSON_EXTRACT(...)) — and MySQL accepts each.

The remaining guards are real:

Mutation	Result
ADD PRIMARY KEY no longer follows a refused DROP PRIMARY KEY	🔴 3 tests, unit + partition layer
split applied to every operation, not just ALTER TABLE	🔴 …/refuses_DROP_TABLE
the safe partition is computed but not executed	🔴 …/a_mixed_ALTER_splits_so_only_its_destructive_clauses_are_refused
the destructive remainder is silently dropped instead of refused	🔴 same

The re-classification guard survives mutation, but the comment already says it is unreachable today and explains precisely why it stays — an honestly-labelled unreachable guard, not an untested claim.

Refusal bookkeeping stays consistent under the split. reason comes from the statement-level classification, which returns the first violation; violations only come from unsafe clauses, and every unsafe clause lands in the unsafe partition, so the reason can never name a clause that was executed. Clause order is preserved within each partition.

All three doc sites move together — WithAllowDestructiveSchemaChanges, EnsureSchema, StorageConfig.AllowDestructiveSchemaChanges and docs/configuration.md — so Copilot's stale-comment point is genuinely closed rather than resolved. Its cleanup-context point is a pre-existing convention in this file (t.Context() in a defer, four sites on main), and the new tests match it; deferred calls run before the framework cancels that context, so the containers do terminate.

Ran locally at head: go build ./..., pkg/ddl and pkg/api unit suites green, and both new integration tests against a real MySQL container — MixedAlterAppliesSafeClauses (7.33s) and RefusesPrimaryKeyChangeWhole (6.94s). CI 34/34. Leak check on the body and diff clean, terminology clean.

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

[Kiran01bm]

🤖 Review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — pull/1126, follow-up commit

Finding 1 — fixed. partitionDestructiveChanges no longer turns a SplitUnsafeAlter error into a startup failure. A split that cannot be performed now falls back to refusing the statement whole: nothing in it executes (strictly less than any split would run, so the failure cannot widen what the bootstrap executes), the fallback is logged at warn with the split error and the exact DDL, and startup proceeds. The behavior is pinned by a new unit subtest (an unsafe ALTER whose clauses cannot be partitioned is refused whole) that feeds the splitter an input it rejects and asserts the whole-statement refusal with no partition error surfaced to EnsureSchema.

Finding 2 — fixed. Taken as suggested: a scope attribute (whole | split) on the existing schemabot.storage_schema.destructive_refusals_total counter, no second instrument. split means the safe clauses of a mixed ALTER executed and only the destructive remainder was refused; whole means nothing in the statement ran (including the split-failure fallback, which the log line distinguishes via split_error).

[Kiran01bm]

🤖 Review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — pull/1126, follow-up PR #1160

Both findings are fixed in follow-up PR #1160 (this PR merged before the fix commit was pushed).

Finding 1 — fixed. partitionDestructiveChanges no longer turns a SplitUnsafeAlter error into a startup failure. A split that cannot be performed now falls back to refusing the statement whole: nothing in it executes (strictly less than any split would run, so the failure cannot widen what the bootstrap executes), the fallback is logged at warn with the split error and the exact DDL, and startup proceeds. The behavior is pinned by a new unit subtest (an unsafe ALTER whose clauses cannot be partitioned is refused whole) that feeds the splitter an input it rejects and asserts the whole-statement refusal with no partition error surfaced to EnsureSchema.

Finding 2 — fixed. Taken as suggested: a scope attribute (whole | split) on the existing schemabot.storage_schema.destructive_refusals_total counter, no second instrument. split means the safe clauses of a mixed ALTER executed and only the destructive remainder was refused; whole means nothing in the statement ran (including the split-failure fallback, which the log line distinguishes via split_error).