[Kiran01bm]

migrate --alter now refuses any statement that drops a column, a constraint or a NOT NULL unless --allow-destructive SCHEMA.TABLE names the table, so data can no longer be discarded by accident.

Why

pg-sprite should make it very hard to make, by accident, a change that loses data or leaves a table unreadable or unwritable. The declarative path already refuses destructive plans, and lock risk already needs --force or --accept-blocking. The imperative path did not: migrate --alter "ALTER TABLE orders DROP COLUMN note" ran as written, with a destructive: true warning and exit 0. That protects SchemaBot users, whose --allow-unsafe gates the flag, but not anyone running the CLI on its own. A drop followed by a re-add of the same column was the sharpest case: most people who write it want to change the column's type, and instead lose every value in it.

What

• The gate. migrate.Run refuses a statement whose plan contains a destructive operation (DROP COLUMN, DROP CONSTRAINT, DROP NOT NULL). The refusal is destructive-change, class by-design, the same contract the declarative path uses.
  • It applies before every route (native, safer sequence, forced, copy-and-swap) and in --dry-run. Nothing executes.
  • A planner refusal such as replica-identity, dependent-view or cascading-drop keeps its own reason. DROP INDEX stays with the statement-kind gate and --accept-blocking.
• The acknowledgement. --allow-destructive SCHEMA.TABLE (Options.AllowDestructive) must name the resolved table, like --force; a mismatch is ErrAllowDestructiveMismatch and nothing executes.
  • It acknowledges only the data loss: every other gate still applies, so a forced rewrite that also drops a column needs both flags.
  • It combines with --force, --dry-run and --restart. It is refused with --accept-blocking and by RunDesired.
• Audit. An acknowledged run logs one warn-level decision line, and its verdict carries allowed_destructive: true.
• Drop and re-add. The refusal carries the new guidance code alter-column-type-in-place, which points to ALTER COLUMN ... TYPE ... USING ... as the way to keep the values. The new code bumps plan.FormatVersion from 13 to 14 and suggest.FormatVersion from 4 to 5.
• Invariant. The new RF-10 states the rule for both data loss and outage risk, and design-principles.md now covers the imperative path.
• Contract and docs.
  • The README, capabilities, limitations, refusal classes, CLI output examples and the plan and suggest report docs are updated.
  • schemabot-integration.md now says to map SchemaBot's unsafe acknowledgement to Options.AllowDestructive.
  • The demo tour checks both the refusal and the acknowledged run.
  • The buzz replay pins the four statements that contain drops as refused.

Before / after

The same statement, ALTER TABLE orders DROP COLUMN qty, ADD COLUMN qty bigint, run by a CLI user who meant to widen qty:

Before
  migrate --alter "...DROP COLUMN qty, ADD COLUMN qty bigint"
  planner   destructive: true (a warning only)
  route     native, metadata-only
  result    executed-natively, exit 0
            every row's qty is now NULL; the old values are gone

After
  migrate --alter "...DROP COLUMN qty, ADD COLUMN qty bigint"
  gate      the plan drops a column and no acknowledgement names public.orders
  result    refused: destructive-change (by-design), exit 2, nothing executed
            guidance alter-column-type-in-place:
            "to keep its values, ALTER TABLE ... ALTER COLUMN qty TYPE bigint USING ..."

  migrate --alter "...DROP COLUMN qty, ADD COLUMN qty bigint" --allow-destructive public.orders
  result    executed-natively, exit 0, "allowed_destructive": true, warn-level audit line

Upgrade note

This changes behaviour for existing callers. An imperative DROP COLUMN, DROP CONSTRAINT or DROP NOT NULL that used to execute is now refused, with exit code 2, until the acknowledgement is passed. An adapter should pass Options.AllowDestructive only for a change the operator approved.

References

• migrate: run ALTER INDEX ... RENAME TO natively for a plain index #248 merged first and took plan.FormatVersion 13 and suggest.FormatVersion 4 for coordinated-constraint-rename, so this PR now carries 14 and 5. diff: report an index left invalid by an unfinished build as an advisory #249 bumps the plan version too; whichever of the two merges second takes the next number.

🤖 Generated with Amp (Claude Opus 4.6)

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	⚠️ Failed 2026-10-24T19:09:27.447528Z We couldn’t complete this review. Comment @codex review to try again.	5403b46	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

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

[JashLal]

🤖 Adversarial correctness review performed by Jash's AI agent. Reviewed the PR delta at head 5403b46c04783d4567b5413f2ead81e2c68e70a0.

Verdict: no correctness defect found in the reviewed delta; full verification remains incomplete. The destructive gate precedes the extracted dispatch, so native, forced, and copy-and-swap routes share it. A matching acknowledgement lifts only this gate; planner refusals and the other route policies remain separate.

Findings

1. (Optional coverage) Exercise schema resolution through the real front doors. The unqualified-name case in pkg/migrate/allow_destructive_test.go:83 supplies the resolved schema directly. The new database tests use qualified ALTER statements, so they do not demonstrate the acknowledgement boundary when two schemas contain the same table name and the connection's search_path chooses between them. This is a coverage suggestion, not a demonstrated bypass.

Action items

1. (Optional, finding 1) Add an apply/dry-run regression with two same-name tables and a non-default search_path: the other schema's acknowledgement must fail without changing either table, while the resolved table's acknowledgement may proceed.

Verified

The changed code checks a supplied acknowledgement before routing, derives destructiveness from routed decisions, audits before dispatch, and attaches allowed_destructive to nonzero outcomes; desired-state and accepted-blocking combinations are rejected. New tests cover refusal with durable state intact, matched/mismatched acknowledgements, all three drop kinds, forced and copy-and-swap paths, dry-run parity, and drop/re-add guidance. Existing execution tests retain their assertions and now explicitly acknowledge the loss; no test functions or safety assertions were removed. All 16 reported checks pass, including PostgreSQL 14–18. Visible bot comments report usage-limit failures rather than code findings.

Exact-head surrounding-source and inline-comment API reads failed with connection errors; I obtained no checkout and ran no local build/tests. I therefore could not independently inspect the resolver, planner's existing destructive classification, complete dispatch, or inline discussions. These limits prevent an unconditional verification claim.

This review was generated by Codex (GPT-6.1 Sol).

[JashLal]

🤖 Product review at 5403b46:

OSS adoption ease: The default now protects standalone CLI users from accidental drops, and the refusal provides the exact table acknowledgement. Drop/re-add guidance offers a useful safer path. The README, exit-code examples, and demo distinguish the refused verdict from an acknowledged dry-run plan, which matters for scripts consuming JSON.

SchemaBot integration: This is a behavior change for existing migrate.Run adapters. Before upgrading, map an operator-approved destructive change to that statement's resolved Options.AllowDestructive table; verify that approval permits the intended drop, an unapproved drop stays blocked, and the audit field is retained. The new typed mismatch/unsupported errors and shared refusal reason provide usable seams without parsing prose. I could not inspect SchemaBot's current adapter here, so its compatibility remains unverified. Also preserve the PR body's release coordination note about the report-version changes shared with #248/#249.

This review was generated by Codex (GPT-6.1 Sol).

[JashLal]

🤖 Approving on Jash’s behalf after the reviews at 5403b46 above. The comments retain the optional schema-resolution regression, operator-approval adapter upgrade requirement, release coordination, and verification limits.

[Kiran01bm]

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

The optional coverage finding is fixed with a front-door regression test; the product review's release note is updated for the version clash with #248, and its SchemaBot adapter step is tracked as an internal follow-up.

#	Finding	Status	Explanation
C1-F1	Schema resolution is not exercised through the real front doors with two same-name tables and a non-default search_path	fixed	TestMigrateAcknowledgesTheTableTheSearchPathResolves (internal/cli) puts two orders tables on a search_path that resolves the second schema first. Naming the other schema's table is ErrAllowDestructiveMismatch from both the dry run and the apply, and neither table changes. Naming the resolved table drops the column from that table only. A mutant that trusts the acknowledgement's own schema for an unqualified name is caught only by this test.
C2	Preserve the release coordination with #248 / #249	fixed	#248 merged first with plan format 13 and suggest format 4. The follow-up merge from main keeps both guidance codes and moves this PR to plan 14 and suggest 5 everywhere they are pinned: code, tests, docs and the demo. The PR body's coordination note says so.
C2	Existing migrate.Run adapters must map operator approval to Options.AllowDestructive before upgrading	deferred	That is SchemaBot-side work at the upgrade. It is tracked as an internal follow-up together with #248's format bump: accept plan 14 and suggest 5, pass the acknowledgement only for an approved drop, and keep allowed_destructive in the audit trail. schemabot-integration.md in this PR already states the mapping.
C1	Verified: the gate precedes dispatch across native, forced and copy-and-swap; desired-state and accepted-blocking combinations rejected	no action	Agreed.

Source: block/pg-sprite#253, review comments 6391751905 and 6391753175 and review 5640880653 at head 5403b46c; fixes in the follow-up commits.