Repository navigation
Conversation
The swap carries only the source's sequences, so a sequence the statement minted on the shadow was found only after the full copy, at cutover-swap-mismatch. Refuse it in the build transaction instead (ST-6). Library-path guard: the migrate CLI already routes these shapes away.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
🤖 Adversarial correctness review by Jash's AI agent at Verdict: the inspected changes support the intended early refusal; no confirmed correctness blocker found, with verification incomplete. The new catalog query checks identity attributes and serial-style sequence ownership separately, qualifies catalog operators, and propagates read failures. The guard follows the re-add check, preserving its refusal priority, and precedes the handoff proof. The added Runner test asserts the typed cause, no shadow, no fence, no checkpoint row, and a writable source. The renamed re-added-identity test deliberately replaces successful-build assertions with the new refusal and no-shadow assertions; that matches the behavior change, and the separate re-add unit coverage remains. No unjustified test deletion found. All 16 reported CI checks succeeded, including PostgreSQL 14–18, build and unit tests. Exact-head repository instructions, unchanged surrounding source and full inline threads were unavailable because API reads failed connecting to api.github.com. I did not independently verify the unchanged transaction cleanup or sequence handoff implementation, or run a local build/tests. Generated by Codex. |
|
🤖 OSS adoption ease: refusing before copying rows gives callers earlier, structured feedback. Optional documentation improvement in SchemaBot integration: the public refusal cause, capability-boundary classification and invariant mapping allow structured handling without parsing the message. The consuming adapter was not verified in this review. Generated by Codex. |
JashLal
left a comment
There was a problem hiding this comment.
🤖 Approving on Jash's behalf after the review at c2930d2. No confirmed correctness blocker found in the full diff; the optional staged-identity remedy clarification is recorded in the product review. Exact-head surrounding source and inline threads were unavailable, and no local build or tests ran. Approval does not certify those unverified paths. Generated by Codex.
The shadow build now refuses a gated statement that gives the shadow an identity or
serialcolumn the source does not have, with a new typed causeshadow-statement-new-sequence, instead of copying every row and stopping at the swap.Why
The swap carries only the source's sequences to the live table: each is re-owned by the shadow column that kept its source column's name, and the swap's catalog re-read proves the live table has exactly the source's identity columns and sequences. A sequence the statement minted on the shadow —
ADD COLUMN … GENERATED … AS IDENTITY,ADD COLUMN … serial,ALTER COLUMN … ADD GENERATED, or a dropped column re-added as one — has no source counterpart in that handoff, so a run carrying one did the whole copy and then failedcutover-swap-mismatch, leaving the shadow and the hold on the source for--restart. That is knowable the moment the statement has run on the empty shadow, so ST-6 says refuse it there.This is an engine-layer guard, not a CLI-visible change.
migratealready routes every one of these shapes away before the build: a new identity orserialcolumn is a volatile-defaultADD COLUMNthe copy-and-swap backend does not carry (backend-unavailable), andADD GENERATEDon an existing column isunsupported-operation. The copy-then-swap-failure was reachable only through the library (schemachange.Runner,BuildShadow). The docs claimed the re-add case "builds, copies, and then stops at the swap" as if that were the design; this PR corrects that too.What
pkg/schemachange/new_sequence_column.go:readSequenceOwnedColumnsreads, inside the build's transaction, every shadow column withattidentity <> ''or apg_depend'a'/'i'edge to a sequence;refuseNewSequenceColumnsrefuses the first withCauseStatementNewSequence(classcapability-boundary, invariant ST-6). The shadow is builtLIKE … EXCLUDING IDENTITY, a kept source identity becomesDEFAULT nextval(<source seq>)without ownership, and a source serial's default is copied without itsOWNED BYedge, so before the statement no shadow column owns a sequence — every hit was minted by the statement, and the fixture'sid bigserialmakes every existing build test a no-false-positive check.buildShadowruns the check afterrefuseUnfillableReadds(so a re-added source identity keepsshadow-statement-readd) and beforeverifyIdentityDefaults.serial,ADD GENERATEDon a kept column, a retype beside the source's own serial key (admitted), theRunnerpath (no shadow, fence, or checkpoint row), and the formerTestBuildShadowDoesNotRefuseAPlainColumnReaddedAsAnIdentityinverted to assert the new cause.refusal-classes.mdnew row and correctedshadow-statement-readdrow;limitations.mdnew row and corrected re-add row;invariants.mdST-6 Enforced line and ST-11 cross-reference;capabilities.yaml→capabilities.md; README pointer.Before / after
One statement through the library path against the
ordersfixture (id bigserial PRIMARY KEY, qty integer NOT NULL, note text, sku text, 2500 rows):ALTER TABLE orders ADD COLUMN seqno bigint GENERATED BY DEFAULT AS IDENTITY, ALTER COLUMN qty TYPE bigint. Before, the build committed and the run copied every row before the swap's catalog re-read found a sequence it had not carried; after, the build refuses inside its own transaction and nothing exists to clean up. ThemigrateCLI refuses this statement asbackend-unavailablebefore the build on both sides, unchanged.🤖 Generated with Amp (Claude Opus 4.6)