[Kiran01bm]

One registry in pkg/migrate lists every operation pg-sprite can run that cannot be undone, with the acknowledgement that lifts its refusal, and a unit test fails CI when a new acknowledgement or destructive shape arrives without an entry.

Why

"Make mistakes hard to commit" is enforced gate by gate today: --allow-destructive, --accept-blocking, --force, --restart, --block-writes, each with its own tests. Nothing checks that the set of gates is complete, so a new irreversible path is caught only if its author remembers to add one. Writing the registry found a gap straight away: RunDesired rejected Force, AllowDestructive and BlockWrites, but passed AcceptBlocking and CopySwap.Restart through to every statement the plan derived. The CLI already rejects both with --desired, so only library callers were exposed.

What

• pkg/migrate/irreversible_registry.go: the five acknowledgements (ackKind) and the registry entries. The destructive-shape entries are derived from the new planner.DestructiveOpKinds(), so the planner's classification stays the only list of destructive shapes. The other five are listed: blocking DROP INDEX, blocking REINDEX, the submitted blocking form, copy-and-swap restart, and blocked writes.
• RunDesired rejects every acknowledgement by walking that list, adding ErrAcceptBlockingNotSupported and ErrRestartNotSupported.
• TestIrreversibleOperationRegistryIsComplete checks that:
  • each acknowledgement reads its own Options field, has its own sentinel, and is rejected by RunDesired;
  • each destructive shape has exactly one entry, and admission refuses a destructive plan whole;
  • each entry names an acknowledgement and a real refusal reason, and each acknowledgement gates an entry;
  • the walk is not vacuous.
• docs/schemabot-integration.md: RunDesired takes no acknowledgement.

No CLI behaviour changes; the behaviour of each gate under a real database is the follow-up.

Before / after

A library caller reuses one Options value, with CopySwap.Restart: "public.orders" left set from an imperative run, for a desired-state run whose plan retypes public.orders.id.

Before
  RunDesired(opts with Restart = "public.orders")
    plan: ALTER TABLE public.orders ALTER COLUMN id TYPE bigint   (copy-and-swap)
    the planned statement runs with Restart set
      a stopped copy-and-swap on public.orders is discarded       ── progress lost, nobody asked
    the test suite has no list to notice the gap

After
  RunDesired(opts with Restart = "public.orders")
    returns ErrRestartNotSupported before any database work
    nothing is planned or discarded; the stopped run resumes on the next run without Restart
  TestIrreversibleOperationRegistryIsComplete fails if a sixth acknowledgement
    or a fifth destructive shape arrives without an entry

Stacked on #281; retarget to main once it merges.

[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-30T04:49:14.025823Z We couldn’t complete this review. Comment @codex review to try again.	c135be8	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.

[aparajon]

🤖 Review 1/2 of c135be8: 0 blocking, 4 non-blocking.

I read the whole diff at c135be8 (base is #281's branch, so the PR's own change is the registry, the two new sentinels, RunDesired's walk, planner.DestructiveOpKinds, and one doc bullet), the callers it touches (Options.validate, the CLI's validateInput, the planner's destructive classification), and ran TestIrreversibleOperationRegistryIsComplete, the planner's classification tests, and the RunDesired tests against the branch. The fix itself is correct and fails closed: before this PR RunDesired rejected Force, AllowDestructive and BlockWrites but handed AcceptBlocking and CopySwap.Restart to every statement it derived, so a reused Options with Restart set would discard a stopped copy-and-swap nobody asked to discard. The walk now rejects all five before any database work, in the same relative order as the old checks, after validate() as before. Nothing here is blocking. The weak spot is the test's name: it proves the registry has the right members, not that each entry says the right thing, and a new acknowledgement is only noticed if someone also adds it to a hand-written list.

Invariants: extends RF-10 (no statement that discards data or leaves the table unavailable runs without a typed acknowledgement; the declarative path now takes none of the five, not only AllowDestructive), and keeps to the fail-closed rule: rejection happens before planning, so nothing executes.

Blocking

None.

Non-blocking

1. The completeness test checks that entries exist, not that they are right. Of the registry's four columns, loss, ack and refusal are only checked for membership in the closed sets (irreversible_registry_test.go#L106-L123), and irreversibleOps() has no reader outside the test (irreversible_registry.go#L136). So the registry is documentation that can drift from the gates it describes. Two mutations survive the whole package suite: pointing the blocking DROP INDEX entry at ReasonDestructiveChange instead of ReasonIndexStatement (L149-L154), and giving blocking REINDEX the force acknowledgement instead of accept-blocking (the "every acknowledgement gates an entry" check is still satisfied, because blocking DROP INDEX also uses it). An operator or adapter reading the registry to learn which flag lifts which refusal would be told the wrong one. Suggested fix: pin the five hand-listed entries' ack and refusal against a table in the test, or better, drive each real gate and compare what it refuses with.

Test that passes on c135be8 and fails when either entry is mis-stated

package migrate

import (
	"testing"

	"github.com/stretchr/testify/assert"

	"github.com/block/pg-sprite/pkg/verdict"
)

// Each non-shape entry names the acknowledgement and refusal the real gate
// uses, not just any member of the closed sets.
func TestIrreversibleRegistryEntriesNameTheirGate(t *testing.T) {
	want := map[string]struct {
		ack     ackKind
		refusal verdict.Reason
	}{
		"blocking DROP INDEX":                           {ackAcceptBlocking, verdict.ReasonIndexStatement},
		"blocking REINDEX":                              {ackAcceptBlocking, verdict.ReasonIndexStatement},
		"submitted form that blocks the table":          {ackForce, verdict.ReasonRewriteRequired},
		"copy-and-swap restart":                         {ackRestart, ""},
		"copy-and-swap with the table's writes blocked": {ackBlockWrites, verdict.ReasonCopyAndSwapRefused},
	}
	seen := 0
	for _, op := range irreversibleOps() {
		w, ok := want[op.key]
		if !ok {
			continue // destructive shapes are derived and checked elsewhere
		}
		seen++
		assert.Equal(t, w.ack, op.ack, "entry %q", op.key)
		assert.Equal(t, w.refusal, op.refusal, "entry %q", op.key)
	}
	assert.Equal(t, len(want), seen)
}

Passes on c135be8. With the blocking DROP INDEX refusal changed to ReasonDestructiveChange it fails with entry "blocking DROP INDEX"; with the blocking REINDEX acknowledgement changed to ackForce it fails with entry "blocking REINDEX". The PR's own test passes on both mutants.

2. A new acknowledgement is only caught if it is also added to ackKinds(). The PR title and the test's comment promise that the test "fails when a new acknowledgement arrives without an entry". What it enumerates is ackKinds(), a hand-written list (L35-L37), and assert.Len(t, acks, 5) (test L50) repeats the same count. Add a sixth typed acknowledgement field to CopySwapOptions and forget the registry: the test stays green and RunDesired hands the new field to every derived statement, which is the exact gap this PR closes for Restart. This is latent today: every string field on Options and CopySwapOptions is accounted for, and the check below passes at c135be8. A reflection check over the two option structs makes the claim true.

Test that passes on c135be8 and fails when a new string field is added to CopySwapOptions

func TestOptionsStringFieldsAreAccountedFor(t *testing.T) {
	known := map[string]bool{"Force": true, "AcceptBlocking": true, "AllowDestructive": true, "CreateOwner": true}
	copyKnown := map[string]bool{"BlockWrites": true, "Restart": true}
	for _, tc := range []struct {
		v     any
		known map[string]bool
	}{{Options{}, known}, {CopySwapOptions{}, copyKnown}} {
		ty := reflect.TypeOf(tc.v)
		for i := 0; i < ty.NumField(); i++ {
			f := ty.Field(i)
			if f.Type.Kind() == reflect.String {
				assert.True(t, tc.known[f.Name], "%s.%s", ty.Name(), f.Name)
			}
		}
	}
}

Passes on c135be8. With Abandon string added to CopySwapOptions it fails with CopySwapOptions.Abandon, while TestIrreversibleOperationRegistryIsComplete stays green.

3. "Setting any of them returns its own sentinel" is not true for combinations. RunDesired runs opts.validate() first (desired.go#L198-L203), and validate already rejects Force with AcceptBlocking, and Restart with either (migrate.go#L178-L189). A caller that left Force and Restart set gets the generic validation error, not ErrForceNotSupported, so an adapter keying on the sentinels (as the new doc bullet invites) misses it. This is not a regression, the order is the old one, but the new sentence and the test (single acknowledgements only) overstate it. Either run rejectDesiredAcks before validate, or say "one of them".

Test case: two acknowledgements together

func TestRunDesiredNamesTheSentinelForTwoAcknowledgements(t *testing.T) {
	opts := DefaultOptions()
	opts.Force = "public.t"
	opts.CopySwap.Restart = "public.t"
	_, err := RunDesired(t.Context(), nil, DesiredRequest{Schema: "public"}, opts)
	require.ErrorIs(t, err, ErrForceNotSupported)
}

--- FAIL on c135be8: the chain is migrate: Options.CopySwap.Restart acknowledges a copy-and-swap run starting over, which Options.Force and Options.AcceptBlocking both route away from; name one acknowledgement.

4. "Every operation that cannot be undone" is wider than the list. The file's header says the registry lists every operation pg-sprite can run that cannot be undone (L9-L18), but it lists the acknowledged ones. A copy-and-swap with KeepOld false drops the retained source before it reports done (copy_swap.go#L63-L66), which ends the rollback window with no acknowledgement and no entry. That may be by design (the swap is the change the operator asked for), but then the header should say "every acknowledged operation", or the entry should exist with an explicit "no acknowledgement, by design" shape so the next reader does not take the list as exhaustive.

Verified

• go build ./... and the registry, planner-classification, and RunDesired tests pass at c135be8; the diff is unchanged in the CLI, so validateInput still rejects all five with --desired.
• Ordering of the walk (Force, AcceptBlocking, AllowDestructive, Restart, BlockWrites) matches the previous three checks' relative order, so TestRunDesiredRejectsForce, ...AllowDestructive and ...BlockWrites keep their meaning.
• DestructiveOpKinds returns statement.OpKinds() order filtered by the same destructiveOp the gates use; the new planner assertion pins the equivalence.

Mutant	Caught by
drop ackRestart from ackKinds	registry test ("not vacuous", "every acknowledgement gates an entry")
AcceptBlocking value reads the Force field	registry test ("reads its own field", "rejects every acknowledgement")
DestructiveOpKinds returns every kind	TestEveryOpKindIsClassifiedForDestructiveness
blocking DROP INDEX entry names the wrong refusal	survives (finding 1)
blocking REINDEX entry names the wrong acknowledgement	survives (finding 1)
copy-and-swap restart entry's loss class changed	registry test ("every entry is gated")
submitted-form entry marked refuses-plan	registry test ("exactly one entry", admission)
RunDesired reverted to the three old checks	registry test ("rejects every acknowledgement")
block-writes mapped to the restart sentinel	TestRunDesiredRejectsBlockWrites, registry test ("own sentinel")
shape entries dropped from the registry	registry test ("not vacuous", "exactly one entry")
rejectDesiredAcks skips restart	registry test ("rejects every acknowledgement")
shape entries claim rejects-ack	registry test ("exactly one entry")
a sixth acknowledgement field added, registry untouched	survives (finding 2)

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

[aparajon]

🤖 Review 2/2 of c135be8: OSS adoption and SchemaBot integration lenses, 0 blocking, 3 non-blocking.

This is the second of two comments; the correctness pass is the one above. The change is a deliberate, fail-closed tightening of an exported surface: migrate.RunDesired now returns ErrAcceptBlockingNotSupported or ErrRestartNotSupported (both new exported sentinels) where it previously ran. For a library importer that is a behavior change, but a safe one and the right direction: the only callers it can break are ones that reused one Options for both front doors with AcceptBlocking or CopySwap.Restart left set, and what they lose is a silent discard of a stopped copy-and-swap. The new integration-doc bullet tells them to clear the fields before calling, which is the right instruction. I found no caller of RunDesired in the schemabot repository, so nothing there moves. A standalone CLI user sees no change: migrate --desired already rejected both flags.

Blocking

None.

Non-blocking

1. RF-10's Enforced entry does not name what now enforces it. RF-10 is the invariant this PR extends ("The declarative path takes no acknowledgement"), and its Enforced line names only RunDesired's admission through ErrAllowDestructiveNotSupported (invariants.md#L1401-L1420); the RF-10 row of the test table (L1552) does not list the registry test either. Per the repo's own rule that the registry stays true, the line should add rejectDesiredAcks (covering all five sentinels) and TestIrreversibleOperationRegistryIsComplete. An external reader looking for where "no acknowledgement reaches a derived statement" is enforced is otherwise pointed at one of five.

2. --restart, --accept-blocking and --force help text does not say they cannot be combined with --desired. The CLI rejects all five (migrate_desired.go#L22-L39), and the help for --allow-destructive and --block-writes says so, but the help for the other three (cli.go#L114-L115, L123) lists other exclusions and omits --desired. A user finds out from the error, not from --help. The PR says "No CLI behaviour changes", which holds, but this is the natural place to make the five read the same.

3. The rule is now stated in three places in the integration doc. The new bullet (schemabot-integration.md#L642-L648) covers all five, while the bullet just above it still restates the AllowDestructive half (L636-L641) and an earlier paragraph restates the BlockWrites half (L466). They agree today, and each is still true, but three statements drift independently. Letting the new bullet own the rule and pointing the other two at it keeps an adapter author reading one list. The sentence "Setting any of them returns its own sentinel" also needs the "one of" caveat from the first comment's finding 3, since a combination returns the validation error instead.

Verified

• Exported surface: two sentinels added (ErrAcceptBlockingNotSupported, ErrRestartNotSupported); DestructiveOpKinds added to pkg/planner; no existing export changed, and the rest of the registry is unexported.
• The sentinel messages follow the existing ErrForceNotSupported shape ("applies to the imperative front door only") and are safe to surface to an operator: they name no table, DSN, or host.
• The PR description's before/after example (a reused Options with Restart set) matches the code path: Restart != "" reaches schemachange.RunOptions.Restart for each derived statement before this change.
• No standalone-user path is affected; the CLI rejection already existed.

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

[aparajon]

🤖 Stamping with comments: 0 blocking, 7 non-blocking (see the two review comments above).

This stamp was left by Claude Code (claude-sonnet-5-5).