fix(api): refuse unsplittable storage ALTERs whole, not fail startup

[Kiran01bm]

Makes a failed SplitUnsafeAlter fall back to refusing the mixed destructive ALTER whole instead of hard-failing EnsureSchema and crash-looping startup.

Why

Follow-up to a post-approval review finding on #1126: partitionDestructiveChanges turned a split error into a hard EnsureSchema error, so an unparseable or unpartitionable mixed destructive ALTER would crash-loop every boot — the exact outcome the destructive-refusal path's deliberate fail-open exists to prevent. The trigger is routine, not exotic: a future Spirit linter bump adding cross-clause unsafe reasoning could fire on an already-safe partition and take down startup fleet-wide.

What

• partitionDestructiveChanges: on split failure, refuse the statement whole (executes strictly less than any successful split — the exact pre-fix(api): apply safe clauses of a mixed destructive storage-schema ALTER #1126 shipped behavior) instead of returning an error; the refusal log carries the split_error.
• RecordStorageSchemaDestructiveRefusal gains a scope attribute (whole|split) on schemabot.storage_schema.destructive_refusals_total so operators can tell whether the safe clauses ran.
• Pinned by a unit subtest driving an unsplittable unsafe ALTER through the partition.

Before / after

Before (#1126 as merged)                      After
┌─────────────────────────────────┐           ┌─────────────────────────────────┐
│ mixed destructive ALTER         │           │ mixed destructive ALTER         │
│   └─ SplitUnsafeAlter           │           │   └─ SplitUnsafeAlter           │
│        ├─ ok → apply safe,      │           │        ├─ ok → apply safe,      │
│        │       refuse unsafe    │           │        │       refuse unsafe    │
│        └─ error → EnsureSchema  │           │        │       (scope=split)    │
│                   fails →       │           │        └─ error → refuse WHOLE  │
│                   startup       │           │            statement, log with  │
│                   crash-loop    │           │            split_error           │
└─────────────────────────────────┘           │            (scope=whole),       │
                                              │            startup proceeds     │
                                              └─────────────────────────────────┘

[chatgpt-codex-connector]

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

[Copilot]

Pull request overview

Adjusts SchemaBot’s storage-schema bootstrap safety behavior so that an unpartitionable mixed destructive ALTER TABLE no longer turns into a fatal EnsureSchema error (and thus a startup crash-loop), instead falling back to “refuse the whole statement” while preserving operator visibility via logs and metrics.

Changes:

• Update partitionDestructiveChanges to fall back to refusing the entire unsafe ALTER TABLE when ddl.SplitUnsafeAlter fails, rather than failing startup.
• Add a scope attribute (whole vs split) to schemabot.storage_schema.destructive_refusals_total to distinguish “nothing ran” from “safe clauses ran”.
• Add a unit subtest pinning the new fallback behavior for an unsplittable unsafe ALTER.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
pkg/metrics/metrics.go	Adds refusal scope constants and extends the destructive-refusal metric with a scope attribute.
pkg/api/ensure_schema.go	Implements the split-failure fallback to “refuse whole” and enriches logging/metrics emission accordingly.
pkg/api/ensure_schema_test.go	Adds a subtest ensuring unsplittable unsafe ALTER statements are refused whole without failing the bootstrap.

---

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

[aparajon]

🤖 Adversarial correctness review, requested by @aparajon and performed by their agent. Reviewed at head 25dde1d6, in a worktree, with the affected packages built and run locally.

Verdict: clean — nothing blocks. This is the fix I asked for on #1126 and it lands the right way: the fallback refuses the statement whole, which is the pre-#1126 shipped behavior, so the failed split provably cannot widen what the bootstrap executes while startup survives it. The scope attribute closes the second item too. Two things worth acting on, neither of them in the executing path.

#	Finding	Severity
1	The sibling error path still hard-fails, and the doc now justifies opposite dispositions with the same sentence	doc / correctness-of-reasoning
2	The scope attribute is unpinned, and the counter isn't in the metrics README	coverage

---

1. Two adjacent error paths, opposite answers, one justification

partitionDestructiveChanges still returns a hard error when ddl.UnsafeStatement can't classify a statement, and the doc justifies it with "classification uncertainty must never widen what the bootstrap will execute" — then two sentences later justifies the opposite disposition for a split failure with "partitioning uncertainty never widens what the bootstrap will execute." Refusing whole doesn't widen in either case, so as written the stated reason doesn't distinguish them, and the next reader has an open invitation to "fix the inconsistency" in whichever direction they happen to prefer.

I think the split is right and the classification path should keep failing — but for a reason the comment doesn't give. A split failure only ever happens on a statement Spirit has already classified unsafe, so refusing it whole is the established answer and the starting binary demonstrably doesn't need the drop. A classification failure can land on a statement the binary does need — an ADD COLUMN in a syntax a bumped parser trips on — and silently skipping that trades a loud startup failure for an "unknown column" at query time, which is strictly harder to triage. That asymmetry is the actual load-bearing distinction and it deserves a sentence.

2. The operator-facing half isn't pinned

scope is the part an operator reads to tell whether the safe clauses ran, and nothing holds it:

scope always "whole"  → go test ./pkg/api/... ./pkg/ddl/... ./pkg/metrics/...  green
scope always "split"  → green
the split-error log branch removed entirely      → green

The fallback itself is well pinned — reverting it to the hard error, or dropping splitErr from the record, each kills the new subtest immediately. It's only the reporting that floats. Asserting scope needs the refusal loop to be reachable from a test, so the cheap version is to have the loop build (scope, attrs) in a small helper and assert that directly.

Separately: schemabot.storage_schema.destructive_refusals_total has never appeared in pkg/metrics/README.md — not in the metric table, not in Attribute Values. Pre-existing, not yours, but this is the natural moment to add the row with all three attributes, since scope is exactly the kind of two-value vocabulary that section exists to document.

Also

Copilot's thread on pkg/metrics/metrics.go:460 (normalize scope inside the helper) is still unresolved. For what it's worth I'd decline it — two package constants at a single call site don't need runtime narrowing, and the compiler already covers the typo case — but per the repo convention the thread wants a reply either way before this goes in.

---

Action items

1. (Finding 1) Say why the classification path still fails while the split path falls back: the split failure is on an already-unsafe statement, a classification failure can be on one the binary needs.
2. (Finding 2) Pin scope, and add the counter to pkg/metrics/README.md with its attributes.
3. Reply to and resolve the open Copilot thread.

---

Verified — tried to break, couldn't

The fallback is real and pinned. Reverting it to the pre-PR hard error kills TestPartitionDestructiveChangesPinsUnsafeVocabulary/an_unsafe_ALTER_whose_clauses_cannot_be_partitioned_is_refused_whole; dropping splitErr from the appended record kills the same test on its require.Error. Both partitions stay mutually exclusive by construction — a record carries splitFrom or splitErr, never both — so the switch and the scope derivation can't disagree.

The new test's construction is legitimately clever, and I checked it isn't accidental. It builds a split failure today by handing the partitioner two statements. That works because UnsafeStatement doesn't enforce a single statement (it lints everything statement.New returns) while SplitUnsafeAlter does (len(parsed) != 1). So the DDL classifies unsafe, reaches the splitter, and fails there — a real trip through the new branch rather than a mocked one, without waiting for a future linter rule. Spirit's differ emits one ALTER per table, so the shape can't occur in production; as a stand-in for the reachable trigger it's honest.

The fallback is genuinely the pre-#1126 posture, not a new risk. Before #1126 every mixed destructive ALTER was refused whole, so "the binary needed a clause that didn't run" was already the accepted trade of the refuse-and-continue design; #1126 improved the common case and this restores the old answer only for the rare split failure. Nothing regresses relative to what shipped.

Base is clean. #1126 (2ec9b28b) and #1146 (5ff3b8c8) are both merged, and this sits directly on top — no stack interlock, no merge-order condition.

The metric change follows the repo's own guidance — a new attribute on an existing hot-path counter rather than a new instrument, which is what pkg/metrics/README.md's Adding New Metrics asks for. Cardinality is two values.

Ran locally at head: go build ./..., plus pkg/api, pkg/ddl and pkg/metrics green. CI 34/34. No test deletions or weakened assertions in the diff. 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/1160, follow-up commit

#	Concern	Status
2	The scope attribute — the operator-facing half of the fix — is unpinned: forcing it to always-whole, always-split, or removing the split-error log branch all stay green; the counter has also never appeared in pkg/metrics/README.md	fixed — the refusal loop's (scope, message, attrs) derivation is extracted into refusedStorageChange.refusalTelemetry() and asserted in every refusal subtest (whole for plain/PK/unsplittable refusals, split for the mixed ALTER, plus the split_error attr and message), so all three listed mutations now die — empirically re-verified: forcing the split limb to return whole fails the mixed-ALTER subtest; README gains the metric-table row and Attribute Values entries for operation and scope
1	The sibling classification-failure path still hard-fails while the split-failure path falls back, and the doc justifies the opposite dispositions with the same "never widens" sentence — an open invitation to "fix the inconsistency" in either direction	fixed — the partitionDestructiveChanges doc now states the load-bearing asymmetry: a classification failure can land on a statement the starting binary needs (a loud startup failure beats a silent missing column at query time), while a split failure only ever happens on a statement already classified unsafe, so refusing it whole is the established answer
3	Copilot thread on pkg/metrics/metrics.go:460 (normalize scope at runtime) is unresolved and needs a reply before merge	reply — declined per the review's own recommendation: scope only ever receives the two package constants at the single call site, the compiler already covers the typo case, and an "unknown" fallback would document a series no current call can produce; the value set is now test-pinned and README-documented. Draft reply below, to post + resolve on Kiran's approval