schemachange: type the shadow refusal causes and add the Proof view

[Kiran01bm]

pkg/schemachange's shadow operations now refuse with a typed *RefusalError carrying one RefusalCause from a closed set, and BuiltShadow gains an exported Proof view that a checkpoint can encode and a resume can decode and compare.

Follow-up to #121 (items C2-2 and C2-3 of the adoption/integration review, deferred there), which is on main.

Why

BuildShadow, DropShadow, and InspectShadow refused with a bare fmt.Errorf("%w: LK-1: …", ErrInvariantViolation), so every ST-5, ST-6, ST-7, and LK-1 refusal collapsed into one errors.Is target. An importer that needs to react differently — re-acquire the lock, leave a relation the engine did not build to an operator, drop and rebuild, or report a bug — had only the message text to branch on, and that text carries schema, role, sequence, and backend names it must not render verbatim. BuiltShadow had ten accessors and no encoding, so persisting the proof for a checkpoint was hand-rolled per importer, and the "compare with your checkpoint" step InspectShadow exists for had no defined shape to compare.

What

• refusal.go: RefusalCause with eleven constants (shadow-lock-unproven, -lock-lost, -lock-held-elsewhere, -lock-unconfirmed, -proof-empty, -source-shape, -statement-target, -owner-mismatch, -foreign-relation, -grants-differ, -identity-handoff), RefusalCauses(), (RefusalCause).Invariant(), RefusalError{Cause, Detail} whose Unwrap() []error exposes ErrInvariantViolation first and then the errors behind the refusal (the lock session's loss, the interrupted statement), and RefusalCauseOf(err). Every former sentinel site in shadow.go, lock.go, inspect.go, identity.go, grants.go, and fidelity.go goes through refuse(...); rg ErrInvariantViolation pkg/schemachange finds only the sentinel and refusal.go.
• proof.go: Proof struct with snake_case JSON tags mirroring the accessors; BuiltShadow.Proof(); BuiltShadow.MarshalJSON() encodes the Proof. No UnmarshalJSON on BuiltShadow: the constructor stays private, so the copier and cutover keep accepting only a value the builder or the inspection returned. IdentityColumn, SequenceOptions, FidelitySnapshot, Grant, ColumnGrant, Policy, and UnvalidatedConstraint gain snake_case JSON tags so the nested encoding is stable rather than Go-field-name-shaped.
• docs/refusal-classes.md: a "Shadow operation refusals, keyed on RefusalCause" table classifying each cause; docs/copy-and-swap-design.md package row names Proof and RefusalCause. docs_test.go (new, package schemachange_test) fails when a cause has no doc row and when a RefusalCause constant is not enumerated by RefusalCauses(), the same guard pkg/preflight has for CopySwapRefusalCause.
• Tests: every existing ErrInvariantViolation assertion in lock_, drop_, inspect_, shadow_, and shadow_owner_integration_test.go and shadow_test.go also asserts the cause (RefusalCauseOf); the caller-cancellation test asserts an empty cause; refusal_test.go pins Invariant(), RefusalCauseOf through wrapping, and the Error() rendering (the one renderer test the log-surface rule allows); proof_integration_test.go builds a shadow over a table with a GENERATED ALWAYS AS IDENTITY key, fillfactor, a table GRANT, a column GRANT, a POLICY, and a NOT VALID check, asserts Proof() equals the accessors field by field, round-trips json.Marshal(built) into a Proof equal to InspectShadow(...).Proof(), and pins the encoding's key paths at every depth (a walk of the decoded tree against a hand-written list, since encoder and decoder agree by Go field name when a nested tag is missing and proof equality alone would not notice). Run locally against PostgreSQL 16; CI covers 14 → 18.

Before / after

BEFORE ─────────────────────────────────────────────────────────────────────────────
  err := schemachange.BuildShadow(ctx, pool, lock, target, stmt, opts)
  errors.Is(err, ErrInvariantViolation)  ─▶ true, for all of:
      "invariant violation: LK-1: table lock is held by backend 4711, not …"
      "invariant violation: ST-5: relation app._pgsprite_a1b2_new is owned by …"
      "invariant violation: ST-7: statement targets app.other, proof is for app.orders"
  importer: strings.Contains(err.Error(), "LK-1") ?   (message carries names it must not render)

  checkpoint ◀── built.Schema(), built.SourceOID(), … ×10, hand-rolled per importer
  resume:  inspected, _ := InspectShadow(…); compare … how?

AFTER ──────────────────────────────────────────────────────────────────────────────
  err := schemachange.BuildShadow(ctx, pool, lock, target, stmt, opts)
  errors.Is(err, ErrInvariantViolation)  ─▶ still true
  switch schemachange.RefusalCauseOf(err) {
  case CauseLockLost, CauseLockUnconfirmed:   re-acquire the lock, repeat
  case CauseLockHeldElsewhere:                yield; another instance holds the table
  case CauseForeignRelation:                  operator removes the relation; engine never touches it
  case CauseShadowOwner, CauseSourceShape:    drop, re-run preflight
  case CauseIdentityHandoff:                  change the statement, or drop and rebuild
  case CauseLockUnproven, CauseProofEmpty,
       CauseStatementTarget, CauseGrantsDiffer: report a bug
  }
  err.Error() ─▶ "invariant violation: LK-1 (shadow-lock-held-elsewhere): table lock on app.orders is held by backend 4711, not the lock session's backend 4700"

  checkpoint ◀── json.Marshal(built)          {"schema":"app","source_table":"orders","shadow_table":"_pgsprite_a1b2_new",
                                                "source_oid":…,"shadow_oid":…,"source_fingerprint":"sha256:…",
                                                "target_fingerprint":"sha256:…","identity_columns":[…],"fidelity":{…},"copy_columns":[…]}
  resume:  var kept schemachange.Proof; json.Unmarshal(checkpoint, &kept)
           inspected, err := InspectShadow(…)
           kept == inspected.Proof()  ─▶ continue with inspected     (a BuiltShadow, minted by the inspection)
           kept != inspected.Proof()  ─▶ DropShadow, rebuild

Decisions a reviewer may want to veto

• The cause set covers build, drop, and inspect today. The copier's and cutover's refusals extend it when they land; the doc guard makes an unclassified addition fail, so extending per leaf is safe.
• Class assignments in docs/refusal-classes.md: invariant-violation only for shadow-lock-unproven, shadow-proof-empty, shadow-statement-target (caller-side incoherence) and shadow-grants-differ (the engine's own write did not take); the seven lock, owner, shape, foreign-relation, and identity-handoff causes are environmental. All still wrap ErrInvariantViolation; the sentinel is the stop mechanism, the class is the consumer's route.
• shadow-identity-handoff is one cause for both the build-time (statement replaced the column) and inspect-time (shadow altered since) detections; the detail text distinguishes them.
• Proof slices are not normalised: a shadow with no identity columns encodes identity_columns: null, and decodes back to nil, so the round-trip is faithful and assert.Equal on the two proofs holds without a normaliser.

References

• schemachange: require the table lock and add DropShadow/InspectShadow #121 review comment C2-2, C2-3; docs/refusal-classes.md § Shadow operation refusals; docs/invariants.md § LK-1, ST-5, ST-6, ST-7; pkg/preflight CopySwapRefusalCause for the house pattern.

🤖 Drafted with Amp (Claude Opus 4.6); reviewed and edited by the author.

[chatgpt-codex-connector]

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

[aparajon]

🤖 1/2 — adversarial correctness. Reviewed pkg/schemachange/refusal.go, proof.go, every converted refusal site in shadow.go, lock.go, inspect.go, identity.go, grants.go and fidelity.go, the two new guards in docs_test.go, the wire-shape walk in proof_integration_test.go, and the two doc tables, at bf7868c4. Mutation testing against pkg/schemachange and pkg/schemadiff; the full package suite run locally against PostgreSQL 16 (ok … 71.808s).

0 blocking, 4 non-blocking.

Every cause's Invariant() matches the // INV: comment at the site that mints it, at all eleven sites. lockLossCause still carries both the session's loss and the interrupted statement ([]error{lost, err}), so errors.Is reaches each of them and the sentinel — I checked that specifically, since it is the one conversion that dropped a %w chain of three. The refusal ordering (ErrInvariantViolation first in Unwrap) and RefusalCauseOf reading through a wrap both hold. No caller of BuildShadow/InspectShadow/DropShadow exists outside this package yet, so the message-text change breaks nothing in-repo.

Non-blocking

1. The completeness guard fails open for the one declaration shape it exists to catch.

declaredStringConstants (pkg/schemachange/docs_test.go:67) collects a constant only when vs.Type is an *ast.Ident naming the type:

ident, ok := vs.Type.(*ast.Ident)
if !ok || ident.Name != typeName {
    continue
}

A constant declared in the same block without repeating the type has vs.Type == nil, so the assertion fails, and the continue is silent. Such a constant is an untyped string constant, which still passes to refuse as a RefusalCause — so it is a usable cause that is invisible to RefusalCauses(), absent from the doc table, and green in CI. Both guards pass because both sides of assert.Equal(declared, enumerated) omit it.

That matters here beyond the usual coverage argument, because this PR extends the claim docs/refusal-classes.md:256-259 makes about itself to cover the new type:

the docs_test.go guards in pkg/verdict, pkg/executor, pkg/preflight, and pkg/schemachange fail when a Reason, CreateShapeCause, PartitionRefusalCause, CopySwapRefusalCause, or RefusalCause exists in the code without a row here, so a new discriminator value cannot land unclassified.

It can. There is a second-order effect too: an unclassified cause gets Invariant() == "" from the default at refusal.go:97, and Error() interpolates that at refusal.go:120, so the refusal renders invariant violation: (shadow-undocumented): … — two spaces, no invariant to look up.

This is inherited rather than introduced: pkg/preflight/docs_test.go:123 has the identical assertion, so the fix belongs in both copies. Requiring the type — fail the test on a nil vs.Type inside a const block that declares any constant of typeName, rather than skipping it — closes it without a style rule nobody will remember.

Mutation that survives on bf7868c4

Appended to the RefusalCause const block in refusal.go, and used at a real refusal site in grants.go (refuse(CauseUndocumented, nil, …)):

	// CauseUndocumented is a new cause declared without repeating the type.
	CauseUndocumented = "shadow-undocumented"
)

=== RUN   TestEveryRefusalCauseNamesItsInvariant
--- PASS: TestEveryRefusalCauseNamesItsInvariant (0.00s)
=== RUN   TestRefusalClassesDocListsEveryShadowCause
--- PASS: TestRefusalClassesDocListsEveryShadowCause (0.00s)
=== RUN   TestRefusalCausesEnumerateEveryDeclaredCause
--- PASS: TestRefusalCausesEnumerateEveryDeclaredCause (0.00s)
PASS
ok  	github.com/block/pg-sprite/pkg/schemachange	0.582s

The fix direction is not run here: making the guard fail on an untyped constant needs the same edit in pkg/preflight/docs_test.go, and I did not want to propose a shared helper's shape from the outside.

2. The Proof's wire shape is pinned; the wire shape its fingerprints are computed from is not.

proofKeyPaths (proof_integration_test.go:19-70) is a genuinely good guard — I confirmed it bites by removing json:"with_check" from Policy.WithCheck, and the walk reports fidelity.policies[].WithCheck against the expected with_check. But two of the fields it pins are source_fingerprint and target_fingerprint, and those values come from fingerprint (shadow.go:437-444):

encoded, err := json.Marshal(m)
...
sum := sha256.Sum256(encoded)

schemadiff.Model carries no JSON tags at all (pkg/schemadiff/schemadiff.go:100-102 onward), so the digest is taken over a Go-field-name encoding. Renaming a Model field, or adding a tag to one, is a pure refactor with no behavior change and no diff in the Proof's key paths — and it changes every fingerprint value, so every checkpoint written by an earlier build stops matching what InspectShadow re-derives. The resume refuses a shadow that is intact.

That is fail-closed, which is why it is not blocking, and it predates this PR. It is worth naming now because this PR is what turns the Proof into a durable format with a compatibility guard, and the guard stops one level above the values that actually carry the shape. Either tag schemadiff.Model and pin its key paths the same way, or compute the digest over an encoding this package owns rather than over another package's field names.

Mutation that survives on bf7868c4

// pkg/schemadiff/schemadiff.go:102
	Table string `json:"table"`

Every fingerprint the package computes changes value; the whole pkg/schemachange suite, wire-shape walk included, is green:

ok  	github.com/block/pg-sprite/pkg/schemachange	2.533s

3. Two of the eleven causes are asserted nowhere.

shadow-source-shape and shadow-grants-differ appear in no test — not in the integration tests, not in refusal_test.go. Grepping each constant across pkg/schemachange/*_test.go, the other nine each have at least one assertion.

shadow-grants-differ is the one I would want pinned most, because the doc classifies it invariant-violation on the grounds that "the build wrote the grants under the table lock and read back something else; that is the engine's write, not the environment" — an importer is told to treat it as a bug report rather than a retry. shadow-source-shape covers two distinct sites (identity.go:103, fidelity.go:385), so it is the cause most likely to be split later. Both are correct today; nothing pins them if a later edit changes the cause at either site, and a wrong cause is a silent mis-route rather than a failure.

4. Proof's doc states an iff that the struct's contents do not support.

proof.go:5-12:

the two are equal exactly when the shadow the inspection found is the one the build produced

The reverse direction holds. The forward one does not: Fidelity is read from the source relation on both paths (shadow.go:215 and inspect.go:76 both pass the source OID to readFidelity), so the Proof carries the source's owner, grants, column grants, policies, comment, reloptions and RLS switches. A GRANT on the source between the build and the resume makes the two proofs unequal with the shadow untouched.

Refusing there is right — the build synchronised the old grants onto the shadow, so the swap would drop the new one. But an importer implementing this sentence reads inequality as "the shadow is not the one I built" and reaches for the remedy the refusal-class table gives for a bad shadow, which is to drop and rebuild: a completed copy thrown away for a grant. Saying that inequality means either the shadow moved or the source's fidelity did, and that the two want different reactions, is the whole fix — the comparison is field-by-field, so an importer can already tell them apart once it knows to.

Checked, not findings

• Every Proof slice has a deterministic order, so equality cannot fail spuriously on ordering: fidelity.go:195/227/259/284 all carry ORDER BY, identity.go:89 orders by attnum, policy roles preserve unnest order, and copyColumns derives from the models in source order.
• Both paths build the proof through newBuiltShadow (shadow.go:250, inspect.go:109), so no field can be populated on one path and zero on the other.
• lockLossCause preserves the three-way chain. Verified directly: errors.Is reaches ErrInvariantViolation, the session's loss, and the statement error.
• refuse is inferred as a printf wrapper, so the format/args pairs are vet-checked; go vet ./pkg/schemachange/ is clean.
• No UnmarshalJSON on BuiltShadow is the right call, and the constructor stays private, so the copier and cutover still take only a minted value.
• ColumnGrant's embedded Grant flattens into the parent object, and the key-path list pins all five resulting keys.
• CI is green at bf7868c4 across PostgreSQL 14 → 18, Supabase compatibility, lint and the no-Docker docs-guard job.

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

[aparajon]

🤖 2/2 — the two lenses. OSS adoption, and integration ease for block/schemabot and other importers, at bf7868c4. Nothing here is blocking; the correctness pass is in 1/2.

Lens 1 — OSS adoption

This is the shape a library should expose, and it is the right half of the problem to have solved first. An external adopter gets a closed set they can enumerate (RefusalCauses()), a reader that works through arbitrary wrapping (RefusalCauseOf), a stable string value per cause that is safe to log and to store, and a published table saying what each one means and what to do about it. The Unwrap() []error returning the sentinel and the errors behind the refusal is the detail most libraries get wrong — an adopter keeps errors.Is(err, ErrInvariantViolation) working and can still reach context.Canceled or the session's own loss underneath.

Declining to add UnmarshalJSON on BuiltShadow is the strongest decision in the PR. A JSON-decodable proof is a forgeable proof, and the reasoning is written down where the next contributor will hit it (proof.go:53-55) rather than living in a reviewer's head.

Two things an external adopter would still trip on.

The library now has two different sentinels both spelled invariant violation. schemachange.ErrInvariantViolation (shadow.go:34) and dbconn.ErrInvariantViolation (dbconn/table_lock.go:33) are distinct errors.New values with identical text. A refusal that wraps a lock loss satisfies errors.Is for both, and renders the phrase — and the invariant ID — twice:

invariant violation: LK-1 (shadow-lock-lost): table lock was lost during the shadow operation: invariant violation: LK-1: table lock is no longer held by its session: context canceled

SAFETY.md:81-83 states the rule in the singular — "a distinct error class (ErrInvariantViolation) naming the invariant ID". An adopter reading that reasonably assumes one. Either pkg/schemachange should alias dbconn's the way pkg/executor already does (executor/optimistic.go:37-39), or the two should be spelled differently enough that a reader of a log line can tell which layer refused.

Proof is a durable format without a version. The doc positions it as what a checkpoint stores and a later run decodes. Adding a field later is safe: the old checkpoint decodes with a zero value, the comparison fails, and the resume refuses. Removing one is not: encoding/json ignores unknown keys, so an old checkpoint carrying a since-deleted field decodes cleanly and compares equal on the narrowed set — the proof silently gets weaker with no error anywhere. A version field the decoder checks, or a documented DisallowUnknownFields decode helper, turns that into a refusal instead of a quiet loosening. This is the same class as non-blocking 2 in 1/2, from the other direction.

Lens 2 — integration ease for importers

For an importer this is a clear improvement over the status quo, and it lands on the right side of the line: the fix is in the engine, not re-derived in the consumer. Before, an importer wanting to re-acquire a lock on shadow-lock-lost but escalate on shadow-grants-differ had to match on message text carrying schema, role, sequence and backend names — text that must not be rendered into a shared operator surface, and that changes whenever an error message is reworded. RefusalCauseOf retires that entirely. That is worth saying plainly because it is the kind of change that gets deferred forever in favour of the consumer's own string matching.

Three things that would finish the job.

The lock's own failures stayed untyped, and they are the ones an importer meets first. dbconn.AcquireTableLock precedes every shadow operation, and it refuses with bare fmt.Errorf("%w: LK-1: …") at seven sites (dbconn/table_lock.go:111, 147, 152, 216, 232, 294, 386). An importer that adopts RefusalCause for the shadow operations still has to string-match to distinguish "you asked for an empty schema/table" from "the lock was acquired but the session does not hold it" — and the second of those is the alarming one. Half the lock lifecycle is typed and half is not, which is an awkward place for an importer to build routing on.

The structured error carries no structured identifiers. The stated motivation is that the message text "carries schema, role, sequence, and backend names an importer must not render verbatim" (refusal.go:12-14) — but the replacement offers Cause and then Detail, and Detail is that same text (refusal.go:106-108 says as much). An importer that wants to log "another backend holds the lock on <schema>.<table>" with its own escaping has to parse Detail to get the names out, which is the thing the cause was introduced to avoid. A few typed fields on RefusalError — Schema, Table, and the relation or backend where the cause has one — would let an importer build its own message without ever touching Detail. Worth deciding now rather than after the field set is load-bearing, since adding fields to an exported struct is easy and changing Detail's meaning later is not.

Nothing pins the // INV: comment against Invariant(). SAFETY.md:84-85 makes the // INV: <id> comment at the enforcement point the locality mechanism, and Invariant() is now a second, independent copy of the same mapping. I checked all eleven by hand and they agree today. A test that walks each refuse( call, reads the nearest preceding // INV: comment, and asserts it equals cause.Invariant() would keep them agreeing — and it is the same AST machinery docs_test.go already carries, so it is cheap. Without it, a site that moves to a different invariant takes the comment with it and leaves the switch behind, and the refusal then reports an invariant ID the code does not enforce.

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

[aparajon]

🤖 Approving. The typed cause is correct at all eleven refusal sites — each cause's Invariant() matches the // INV: comment where it is minted — the sentinel and every wrapped error still answer errors.Is, and the Proof's wire shape is pinned by a guard I confirmed bites. All four findings in my review are non-blocking: a completeness-guard hole inherited from pkg/preflight, an unpinned encoding one level below the Proof, two causes without an assertion, and a doc sentence that overstates an iff.

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