[Kiran01bm]

migrate --desired and RunDesired now reject the block-writes acknowledgement, so desired-state execution never takes a table offline for writes.

Why

--block-writes SCHEMA.TABLE accepts an outage: every write to the table fails with 55006 until the swap. On migrate --alter the operator wrote the one statement that takes the copy, so they know what they are accepting. On migrate --desired the derived plan decides which statements take copy-and-swap, so a plan that retypes two columns runs two fenced copies, and the operator accepted an outage whose length they never saw. A library embedder that leaves BlockWrites set in its configuration would take the table offline for every later plan. This is the same reasoning that keeps --force and --allow-destructive off the declarative front door: make mistakes hard to commit.

What

• migrate --desired --block-writes … is a usage error naming migrate --alter --block-writes as the way to run offline.
• RunDesired returns the new sentinel ErrBlockWritesNotSupported before any database work.
• Desired-state copy-and-swap now always takes the live path, so its integration tests run on a server with logical decoding; the subtest for a mismatched block-writes acknowledgement on RunDesired is gone, since that path is no longer reachable.
• Docs: --block-writes help, copy-and-swap.md, limitations.md, and schemabot-integration.md.

Before / after

The same desired file retypes two columns of public.orders on a server without logical decoding. Before, the operator's one table name authorized both fenced copies the planner chose; after, nothing runs, and the operator runs each retype themselves on the imperative front door. migrate --alter --block-writes is unchanged.

Before
  migrate --desired orders.sql --block-writes public.orders
    planner derives: ALTER COLUMN id TYPE bigint, ALTER COLUMN note TYPE varchar(50)
    copy 1 runs behind the fence   ── writes to public.orders fail with 55006
    copy 2 runs behind the fence   ── writes fail again: a second outage nobody reviewed
    exit 0

After
  migrate --desired orders.sql --block-writes public.orders
    rejected before connecting: "--block-writes cannot be combined with --desired"
    nothing runs, writes keep flowing
    operator runs each statement with: migrate --alter '…' --block-writes public.orders

[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-30T00:11:39.389782Z We couldn’t complete this review. Comment @codex review to try again.	641a009	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]

🤖 Correctness review on Jash's behalf at head 641a009.

Verdict: no concrete blocker found in the full diff; surrounding-source and local-test verification are incomplete. The CLI rejects the combination and RunDesired returns the typed ErrBlockWritesNotSupported before diffplan.Plan. The nil-pool test meaningfully checks that the library refusal happens before database work. Moving desired-state copy tests to logical decoding preserves coverage for the newly required live path; imperative block-writes behavior is unchanged by the shown implementation.

1. (Optional) In internal/cli/migrate_test.go, supplement the direct validateInput test with a command-boundary test using --desired and --block-writes, an invalid connection target, and a nonexistent desired file. Assert the usage error wins before connection/file processing; this protects the early-refusal promise if command wiring changes.

Verification: read the description, all diff hunks, visible issue comments and current check results. All 16 checks succeeded, including PostgreSQL 14–18. I did not run a local build or tests. Exact-head surrounding-source API reads failed to connect, so unchanged command wiring, helpers, repository instructions outside the diff, and inline review threads were not independently verified. Generated by Codex.

[JashLal]

🤖 Product review at 641a009.

OSS adoption: The usage error explains why the combination is refused and gives an actionable imperative alternative. Help and copy-and-swap documentation describe the same restriction. This makes accepting each write outage explicit; automation using the old combination must change its invocation.

SchemaBot integration: ErrBlockWritesNotSupported gives embedders an errors.Is target without parsing prose, and the refusal precedes planning. An adapter should surface it as a configuration refusal requiring operator action, rather than repeatedly retrying. I have not verified the downstream adapter's current mapping. Generated by Codex.

[JashLal]

🤖 Approving on behalf of Jash after the reviews at 641a009. The optional coverage suggestion and verification limitations are recorded above. No local build or tests ran. Generated by Codex.

[aparajon]

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

Inspected the full diff at 641a009 against its merge base 7f77869, the callers of RunDesired, the plan renderers, and the docs the change touches. Ran the new tests, the two desired-state copy-and-swap integration tests (with logical decoding on), and the built CLI against a live PostgreSQL 16.

The change is correct. RunDesired rejects Options.CopySwap.BlockWrites before any database work (desired.go#L206-L208) with the new typed sentinel, and migrate --desired --block-writes is a usage error that names the imperative alternative (migrate_desired.go#L36-L38). The check runs ahead of the planner and in validateInput, so --dry-run and the real run both hit it; I confirmed the CLI output: pg-sprite: error: migrate: --block-writes cannot be combined with --desired: .... migrate --alter --block-writes is untouched, and rename_index.go and migrate.go still handle their own acknowledgement checks. The test rewiring is sound: desiredCopySwapOptions clears the acknowledgement and the desired copy-and-swap tests now run on a wal_level=logical server, so the converge test exercises the live path it could not reach before. No other RunDesired caller passes a block-writes value (the supabase suites use DefaultOptions(), the rest runOptions(), neither of which sets it).

Nothing here weakens a safety property: a desired-state run can no longer take a table offline, which is the safer direction. No entry in docs/invariants.md names block-writes or the desired door's acknowledgement set, so this upholds the existing destructive-ack stance (the entry at invariants.md#L1410) without touching its Enforced: line.

Non-blocking

1. Desired-state plan output still advertises --block-writes, the flag this PR now rejects for it. writeExecutionNotes prints the same note for every copy-and-swap statement (dryrun_text.go#L180), and it is reached by diff --desired and migrate --desired --dry-run (migrate_desired.go#L64-L73), which are desired-state surfaces by construction. Built at 641a009, a desired file that retypes two columns prints:

   note:
     pg-sprite migrate will run it as a copy-and-swap: build a shadow table
     with the change applied, copy the rows into it while streaming live
     writes, verify, then swap the two under a brief lock (needs --free-disk
     and logical decoding on the server; --block-writes instead blocks the
     table's writes until the swap (offline mode))
   

   An operator who follows that advice with migrate --desired ... --block-writes public.orders gets the new usage error. The note is correct for migrate --alter --dry-run, so the fix is to make it desired-aware: for a desired plan say that offline mode runs per statement through migrate --alter --block-writes SCHEMA.TABLE. The runner argument already distinguishes the surfaces, or the note could take a flag.

2. Nothing pins the new user-visible consequence: a desired plan with a copy-and-swap statement is now refused whole on a server without logical decoding. This is the "After" half of the PR's own before/after example, and it is the behavior an operator will actually meet. The PR deleted the only desired-state subtest that ran on a server without logical decoding and moved the rest to wal_level=logical, so no test covers the refusal. Mutant: LogicalDecoding: o.BlockWrites == "" to LogicalDecoding: false in copy_swap.go#L178 survives TestRunDesiredCopyAndSwap and the PR's other desired tests. The repo's own coverage invariant (AGENTS.md) asks for a test that fails without the behavior.

   Test case: passes at 641a009, fails on the mutant

   func TestRunDesiredRefusesACopyOnAServerWithoutLogicalDecoding(t *testing.T) {
   	serverURL := testutil.StartPostgres(t) // default wal_level = replica
   	pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: serverURL})
   	require.NoError(t, err)
   	defer pool.Close()
   	schema := testutil.NewSchema(t, pool)
   	_, err = pool.Exec(t.Context(), fmt.Sprintf(`CREATE TABLE %s.t (id int PRIMARY KEY, owner_id int NOT NULL)`, schema))
   	require.NoError(t, err)
   
   	res, err := migrate.RunDesired(t.Context(), pool, migrate.DesiredRequest{
   		Schema:  schema,
   		Desired: parseDesired(t, `CREATE TABLE t (id int PRIMARY KEY, owner_id bigint NOT NULL, note text)`),
   	}, desiredCopySwapOptions(serverURL))
   	require.NoError(t, err)
   	assert.Equal(t, verdict.OutcomeRefused, res.Outcome)
   	assert.Equal(t, verdict.ReasonCopyAndSwapRefused, res.Reason)
   	assert.Contains(t, res.Detail, string(preflight.CopySwapCauseLogicalDecodingUnavailable))
   	assert.Contains(t, res.Detail, "nothing was executed")
   	assert.Empty(t, res.Verdicts)
   }

   --- PASS: TestRunDesiredRefusesACopyOnAServerWithoutLogicalDecoding (2.92s) at 641a009; --- FAIL with the mutant applied.

3. Three pieces of prose now read against the new behavior.

   • limitations.md#L166 still says desired-state execution "runs it with the same preflight gates and flags as migrate --alter", in the same cell that now says it rejects --block-writes. It no longer takes the same flags.
   • The comment on admitCopySwap lists "logical decoding or a block-writes acknowledgement" as the preflight's alternatives (desired_copy_swap.go#L20); on this path only logical decoding is possible.
   • The field documentation on CopySwapOptions.BlockWrites (copy_swap.go#L68-L76) is where an embedder looks first, and it does not say RunDesired rejects the value. The sentinel's own comment does, but a reader of the options struct would not find it.

4. Adjacent asymmetry: the library accepts CopySwap.Restart for RunDesired, the CLI does not. migrate --desired --restart is a usage error (migrate_desired.go#L30), but neither RunDesired nor Options.validate (migrate.go#L178-L189) refuses it, so a value left on an embedder's configuration is honored for every later plan, the same argument this PR makes for BlockWrites. Restart is not an outage and is pre-existing, so this is only worth a follow-up if the intent is that the declarative door accepts no per-run acknowledgements at all.

Verified

• go test ./internal/cli ./pkg/migrate -run 'TestCopySwapFlagsWireIntoEnginePolicy|TestRunDesiredRejects' passes. TestRunDesiredCopyAndSwap and TestRunDesiredRefusesAConnectionOnAnotherDatabase pass against a real PostgreSQL 16 (--- PASS, 2.86s and 2.10s).
• CLI against a live server: migrate --desired orders.sql --block-writes public.orders --free-disk 1GiB fails with the new usage error; the same command without the flag reaches admission and is refused with copy-and-swap-logical-decoding-unavailable and "nothing was executed".
• The "Before" half of the PR's example is accurate: the base RunDesired ran each planned statement through Run, which only checks that the acknowledgement names the statement's table, so one acknowledgement covered every copy on that table.
• CI is green at 641a009.

Mutant	Caught by
Delete the RunDesired rejection	TestRunDesiredRejectsBlockWrites
Delete the CLI validateInput rejection	TestCopySwapFlagsWireIntoEnginePolicy/rejects_--block-writes_with_--desired
LogicalDecoding: false in the copy-swap environment	survived (finding 2; killed by the test above)

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

[aparajon]

🤖 Review 2/2 of 641a009: 0 blocking, 2 non-blocking (the OSS adoption and integration lenses; the correctness review is the comment above).

OSS adoption: what a first-time operator meets

The usage error is the right shape: it says what is wrong, why (the plan decides how many copies run), and the command to run instead. The --block-writes help gains "Cannot be combined with --desired.", matching the sentence --allow-destructive already carries (cli.go#L116), so --help and the error agree. The new paragraph in copy-and-swap.md#L40-L46 states the rule and the reason where a reader choosing between live and offline mode will be looking.

1. The operator who is stranded by this is the one on a server that cannot enable logical decoding, and nothing on the desired path tells them where to go. Before the PR, --block-writes was the escape hatch from a copy-and-swap-logical-decoding-unavailable refusal on migrate --desired. Now the same refusal's detail names only the server setting (set wal_level = logical (a restart-only setting), copy_swap_environment.go#L215); I confirmed the live refusal text and it carries no pointer to the per-statement route. The path exists (migrate --desired --dry-run lists each statement, migrate --alter ... --block-writes runs one, a rerun of --desired converges), but it lives only in copy-and-swap.md and limitations.md. Pairing this with finding 1 of the correctness review: the plan note advertises --block-writes, the refusal does not mention the offline route, and the usage error does. One consistent sentence in the desired-state refusal for this cause, "offline mode: run each statement with migrate --alter --block-writes SCHEMA.TABLE", would make the three agree. The long added sentence in copy-and-swap.md#L40-L46 also runs a paragraph onto one line; a rewrap would keep the diff readable.

Integration ease: schemabot and other importers

Source-compatible and runtime-breaking, in the same way the Force and AllowDestructive rejections are. The sentinel ErrBlockWritesNotSupported is errors.Is-able, returns before any database work with a zero DesiredResult, and the check order (Force, AllowDestructive, BlockWrites) is deterministic, so an adapter can branch on it. block/schemabot pins pg-sprite v0.3.3 and references neither CopySwap.BlockWrites nor RunDesired, so a bump there is unaffected.

2. An embedder that builds one Options and passes it to both Run and RunDesired starts failing at runtime, and the repo has no release note to say so. The PR's own test shows the idiom an adapter needs: copy the options and clear the field (desiredOpts := opts; desiredOpts.CopySwap.BlockWrites = "", copy_swap_connection_integration_test.go#L264-L267). schemabot-integration.md#L466-L468 documents the rejection but not that idiom. A sentence there ("pass RunDesired a copy of the options with BlockWrites cleared") plus a line in the release notes for the next tag would save an adopter the first failed call. There is no changelog file in the tree, so the PR description or release notes are the only place to put it.

Verified

• Read the PR's docs and help text end to end for contradictions with the new rule; the stale spots are listed in the correctness review (finding 3).
• Checked block/schemabot's use of pg-sprite (module pin and a search for BlockWrites and RunDesired): no use.
• The sentinel follows the ErrForceNotSupported and ErrAllowDestructiveNotSupported convention: typed, documented on the sentinel, rejected at the top of RunDesired.

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

[aparajon]

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

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