build(deps): pin pg-sprite to released v0.3.1

[Kiran01bm]

Pins pg-sprite to the released v0.3.1 and adopts its invalid-index outcome vocabulary in the PostgreSQL engine.

Why

The PostgreSQL engine was pinned to v0.2.0. Two releases have landed since. v0.3.0 splits the single "pre-existing invalid index" verdict into five, each carrying what the executor actually proved about the entry occupying the requested name, and marks two of them permanent via Code.Permanent(): the name is held on a different table, or by an index the server will not drop concurrently (a partitioned table's index, an index partition, a constraint's index). Treating those as operational, as the engine did for every invalid-index verdict, would tell an operator to clear something that retrying cannot clear. v0.3.1 adds only library API — a typed CreateShapeCause on create-path refusals and preflight.LookupOwnedRelationNames — with no vocabulary or report-format change, so one pin move picks up both and the engine adopts the new API in small follow-on changes rather than here.

What

go.mod and the consumer module pin v0.3.1. In pkg/engine/postgres/apply.go the invalid-index arm of the apply path now handles only non-permanent verdicts as operational; permanent ones fall through to classifyRefusal, which maps them to a new invalid-index-occupied refusal reason. The operator advice ladder gains a per-code step: own leftover and abandoned name a drop; build-in-flight names the builder PID and says wait; builder-unobservable points at pg_stat_progress_create_index with a stats-reading role; other-table and not-droppable say rename in the schema file and re-plan. The unproven default still fails safe with investigation steps. CodeCancelledByCaller joins the operational set so the outcome vocabulary stays total; the totality test over executor.Code pins that. Integration coverage renames the pre-existing fixture to the abandoned verdict it now produces and adds an other-table fixture asserting the refusal.

Upholds UX-4: every invalid-index verdict still names the operator's next step, and the two permanent ones now say the true one (rename and re-plan) instead of a drop that would not help. No verdict is converted into a success outcome; the change only moves two from "retry after operator action" to "refused until the schema change changes".

Before / after

Before                                      After

InvalidIndexError ──▶ operational           InvalidIndexError
  (any code)          retry after operator    │
                      clears the index        ├─ !Permanent() ──▶ operational
                                              │   own-leftover      drop, retry
                                              │   abandoned         drop, retry
                                              │   build-in-flight   wait for PID, retry
                                              │   builder-unobs.    inspect w/ stats role
                                              │   unproven          investigate
                                              │
                                              └─ Permanent() ────▶ refused
                                                  other-table       "invalid-index-occupied"
                                                  not-droppable     rename + re-plan

References

• https://github.com/block/pg-sprite/releases/tag/v0.3.0
• https://github.com/block/pg-sprite/releases/tag/v0.3.1

[Copilot]

🟢 Approval recommended

The dependency pin is paired with correct apply-path classification updates and comprehensive unit/integration coverage for the new invalid-index outcome vocabulary.

Pull request overview

Pins SchemaBot’s PostgreSQL engine dependency pg-sprite to released v0.3.1 and updates apply-time classification + operator-facing guidance to match pg-sprite’s expanded invalid-index outcome vocabulary, especially distinguishing operational (retryable) vs permanent (refusal) cases.

Changes:

• Bump github.com/block/pg-sprite from v0.2.0 to v0.3.1 in the main module and the consumer module.
• Update PostgreSQL apply error handling to treat only non-permanent invalid-index verdicts as operational, and map permanent verdicts to a refusal reason (invalid-index-occupied) with code-specific operator guidance.
• Expand/rename tests to cover the new invalid-index verdicts, including a new integration test asserting the “index name occupied on other table” permanent refusal.

File summaries

File	Description
pkg/engine/postgres/apply.go	Updates invalid-index handling to separate operational vs permanent verdicts; adds code-specific operator guidance and new refusal mapping.
pkg/engine/postgres/apply_test.go	Extends unit coverage for the new invalid-index codes and pins executor code disposition totality.
pkg/engine/postgres/postgres_integration_test.go	Renames the prior invalid-index integration scenario and adds an “other table” permanent-refusal fixture.
go.mod	Pins github.com/block/pg-sprite to v0.3.1.
go.sum	Updates sums for github.com/block/pg-sprite v0.3.1.
e2e/consumermodule/go.mod	Pins consumer module’s indirect pg-sprite to v0.3.1.
e2e/consumermodule/go.sum	Updates consumer module sums for github.com/block/pg-sprite v0.3.1.

Review details

• Files reviewed: 5/7 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chatgpt-codex-connector]

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

[aparajon]

🤖 Adversarial correctness review — d7b7ee86 (7 files, +205/−56)

Correct change, and the interesting half is the direction it moves: two verdicts go from operational (retried against the same wall until the budget is spent, with advice telling an operator to drop something a drop cannot clear) to refused. That is the fail-closed direction, and the advice ladder now says the true next step for each. The pin itself is consistent — v0.3.1 in both go.mod files with matching go.sum hashes in both modules.

The claim I went after is the one the whole change rests on: can a verdict reach a disposition nobody chose? Two independent things close that, and it took checking both to be sure:

• InvalidIndexError.Code() is total by construction — its default arm returns CodeInvalidIndexUnproven, so it can never yield the empty code or anything outside the closed set. That is what makes the discarded known flag at apply.go:387 (r, _ :=) safe rather than lucky.
• TestRefusalForOutcomeTotalOverExecutorCodes (apply_test.go:267, pre-existing) walks executor.Codes() and asserts every code has an explicit arm, so an upstream addition fails CI at bump time instead of draining into the retryable tail.

Together those are airtight for coverage. What they don't pin is agreement, which is the one thing I'd change.

#	Sev	Where	What
1	low	apply.go:180 / :492	The bump introduces a second authority on permanence (Code.Permanent(), new in v0.3.0) for one code family while refusalForOutcome keeps hand-maintaining all 26. The totality test pins coverage, not agreement — and the two already disagree on two codes, with nothing recording that either is deliberate

---

1 — two authorities on permanence, and the seam between them is untested (low)

Before this bump, refusalForOutcome was the only answer to "is this permanent" — Code.Permanent() does not exist in v0.2.0 at all. This PR adopts it as the gate at apply.go:180 for the invalid-index family, which is the right call for that family. But the other twenty codes keep their disposition from the hand-maintained switch, so there are now two sources of truth, and no test compares them.

I ran the comparison across all 26 codes. Two disagree:

CODE                        Permanent()   refusalForOutcome        verdict
budget-statement-exceeded   false         refusal                  SchemaBot stricter  — sanctioned
pool-too-small              true          operational              SchemaBot looser    — undocumented

budget-statement-exceeded is fine, and upstream says so explicitly. Permanent()'s own doc comment (code.go:158) calls itself "the floor an adapter's retry policy stands on, not its ceiling: a code that is not permanent may still be one a particular adapter declines to retry (a statement budget it sized as a lease, for one)" — which names this exact case. Being stricter than Permanent() is blessed.

pool-too-small is the other direction, the one the same comment warns about ("a permanent code retried unchanged loops for ever"). It is reachable, not theoretical: pool_max_conns survives SchemaBot's DSN normalization, so a target DSN carrying pool_max_conns=1 yields MaxConns=1 against buildMinConns=2 (native.go:184) and admission returns ErrPoolTooSmall before anything executes. Classified operational, that burns the retry budget on a config problem no retry can change, and the operator gets the generic tail rather than "your pool is too small."

I am not asking you to reclassify it, because I could not establish that operational is wrong. SchemaBot's vocabulary reasons about this differently from pg-sprite's: UX-5 says "an error in the operator's own input carries Attention rather than Refused: fixing the input and retrying works, so it is not a refusal of the request" — and a pool parameter is arguably exactly that. pg-sprite means permanence relative to the library call; SchemaBot means it relative to the request. Under SchemaBot's reading, operational may well be right. (Though insufficient-privileges is a refusal and is also "fix the deployment config and retry", so the internal line isn't crisp either — that predates this PR.)

The finding is the seam, not the classification: after this change, a reviewer looking at refusalForOutcome can't tell whether a divergence from Permanent() is a considered decision or an oversight, and CI can't tell either. Concretely, extend TestRefusalForOutcomeTotalOverExecutorCodes to also assert code.Permanent() == (r != nil) with an explicit allowlist of sanctioned divergences, each carrying its reason:

// Divergences from pg-sprite's Permanent(): stricter is sanctioned by its
// own contract; looser needs a reason recorded here or it is a bug.
var sanctionedDivergence = map[executor.Code]string{
	executor.CodeBudgetStatementExceeded: "sized as a lease; a budget SchemaBot declines to retry",
	executor.CodePoolTooSmall:            "deployment config, not the request (UX-5)",
}

That turns the seam into one where a new upstream code, or a change to Permanent(), forces the decision in review instead of landing silently. Given the totality test already exists in exactly this shape, it's a few lines — and it is the structural-enforcement tier docs/invariants.md asks new guards to aspire to ("prefer a completeness test over the relevant registry to a hand-maintained list").

---

Verified — 6/6 mutations killed, the advice ladder against AV-8, and why refusing here is the safe direction

Mutation testing — 6 run, 6 killed. The one that matters is M1: deleting && !invalidErr.Code().Permanent() from apply.go:180 — the PR's central new guard — survives the entire unit suite and is killed only by TestEngineApplyConcurrentIndexOnOtherTableRefused (postgres_integration_test.go:989), which catches it at :1007-1008 (phase refused→failed, Retryable false→true). That is the integration test this PR adds, landing exactly where AGENTS.md puts the workhorse coverage, so the guard is pinned — just not in the fast layer. Worth knowing if that test is ever moved or trimmed. The rest died in the unit suite: reverting refusalForCause to return nil for every verdict (3 failures), moving OtherTable back to operational (4), dropping the typed-detail override so the generic refusal detail leaks (3), dropping the empty-Table guard in invalidIndexTableSuffix (2, caught by the "no inspected table name" case asserting ("") never renders), and removing CodeCancelledByCaller from the operational arm (2, one being the totality test).

The advice ladder holds AV-8 and UX-4 across all seven arms. Only typed identifiers are interpolated, every arm ends in sanitizeReasonText, and the tests assert the raw-server-text fixture (db-internal-1.example.com) never appears in any rendering — including the arms that wrap a BudgetError, where the inner cause is deliberately not the outcome. BuilderPID is set at every ErrInvalidIndexBuildInFlight construction site upstream (recover.go:311, :389, from an observed backend pid), so backend %d can't render a placeholder zero. The one arm that names a drop is the abandoned verdict, and it still says "confirm it is still invalid with no builder" first.

Refusing is the fail-closed direction for both new permanent codes. OtherTable — the name is held on a different table, so this change genuinely cannot claim it, and the advice correctly points at that table's own change. NotDroppable — a partitioned table's index, an index partition, or a constraint's index, which DROP INDEX CONCURRENTLY will not remove. Neither is transient, and both previously retried into an unchanging wall. A refused apply is still StateFailed, so nothing here converts a verdict into a passing check; the change only moves two verdicts from "retry after operator action" to "blocked until the plan or target changes."

invalid-index-occupied needs no registration elsewhere. I checked whether refusal reasons are enumerated in a docs table, metrics allowlist, or completeness test that would now be stale — they are not; every reason lives only in apply.go and its tests, so the new one is consistent with all six existing ones.

Invariants. The PR cites UX-4 ("A refusal says what to do next") and upholds it — each of the seven arms names an action, and the unproven default's "some situations genuinely need a decision only an operator can make" is precisely what UX-4 permits. The entry it also moves and doesn't cite is UX-5, whose stated distinction is the one this change implements: "what separates Refused from Attention is whether retrying unchanged could ever succeed, which is the first thing an operator needs to know." Moving two verdicts across that line changes which glyph an operator sees, so it's worth naming in the summary — and it's the entry finding 1 lands on. AV-8 is upheld (guidance written by SchemaBot, never assembled from an untrusted error string). No *Enforced:* line moves.

Local run at d7b7ee86: go test ./pkg/engine/postgres/ pass, go test -tags=integration ./pkg/engine/postgres/ -run 'TestEngineApplyConcurrentIndex(OnOtherTableRefused|AbandonedInvalidRetryable)' both pass against a real Postgres container, gofmt -l clean, go vet clean. All cited line numbers verified against the head commit.

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

[aparajon]

🤖 Approving. The pin is consistent across both modules and the vocabulary adoption moves in the fail-closed direction: two invalid-index verdicts stop being retried against a wall no retry can move, and each of the seven advice arms now names the true next step. Verified the load-bearing property — InvalidIndexError.Code() is total by construction and the pre-existing totality test over executor.Codes() pins every code to an explicit arm, so the discarded known flag at apply.go:387 is safe rather than lucky. 6/6 mutations killed; deleting the new Permanent() gate survives the unit suite and is caught only by the integration test this PR adds.

One low finding: the bump makes Code.Permanent() a second authority on permanence for one code family while refusalForOutcome keeps hand-maintaining all 26, and the totality test pins coverage but not agreement — the two already disagree on budget-statement-exceeded (stricter, and explicitly sanctioned by upstream's own contract) and pool-too-small (looser, and undocumented). I'm not asking for a reclassification; UX-5's reading may well make operational correct there. The ask is to assert the agreement with an allowlist of sanctioned divergences so the seam can't drift silently.

Upholds UX-4 and AV-8; UX-5 is the entry it also moves and is worth citing in the summary.

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

[aparajon]

🤖 Follow-up on the merge — 2106b409 (my earlier stamp was at d7b7ee86)

The head move is a merge of origin/main, not new authored work, and the resolution is a semantic merge rather than a mechanical one: #1319 replaced the flat detail with typed cause/remedy fields composed at classifyRefusal's single exit, and this branch's arms were adapted to match — invalidIndexDetail became invalidIndexAdvice returning (cause, remedy), refusalForCause sets r.cause, r.remedy, and the invalid-index-occupied arm carries both instead of a flat detail. That is the right adaptation, and it keeps #1319's assert.NotEmpty(t, r.cause) satisfied. Suite green locally.

But the merge creates one interaction neither PR could have seen on its own, and it's the defect #1319 just finished fixing. #1319 established that the composed detail must survive the CLI status clamp (240 bytes, truncated from the tail) with the remedy's lead intact, and pinned it for create-collision. The invalid-index advice on this branch was written before that discipline existed, and now flows through the same composition. Measured at 2106b409:

verdict         identifiers   raw    clamped   what the operator is left with
other-table     realistic     233B   no        full remedy
other-table     63-char       396B   yes       ...t table ("aaaa...      ← NO remedy at all
not-droppable   realistic     273B   yes       ...must resolve it on the target, or rename the ...
not-droppable   63-char       379B   yes       ...an index partition, or a constrain...  ← NO remedy

Two of the four shapes lose the remedy entirely — the operator reads a description of the problem and no instruction. The not-droppable cause is the expensive one: it enumerates all three shapes ("a partitioned table's index, an index partition, or a constraint's index") in the cause, which is exactly the position #1319 learned to keep short so the tail survives.

This is low severity, not a behavior bug — the full advice is intact in the published PR comment and the server log, and only the CLI status listing clamps. But it is the same operator-facing gap, and the fix is the one this repo just adopted: shorten the causes so the remedy's lead lands inside the clamp for identifiers of any legal length, and extend TestCreateCollisionRefusalLeadSurvivesStatusReasonClamp (or add a sibling over the invalid-index arms) so the property is pinned rather than incidental. Moving the three-shape enumeration out of the cause and into the remedy's tail would likely be enough on its own.

Worth saying plainly: my original review could not have caught this. At d7b7ee86 the invalid-index advice went into a flat detail, and #1319 had not merged, so neither the shared composition path nor the clamp discipline existed on this branch. The merge is what put them in contact — which is the argument for re-reading a merge commit against the invariants of both sides rather than treating it as a no-op.

My finding from the first review is unaffected: the operational arm still lists CodePoolTooSmall, so the Permanent() agreement seam stands as written.

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

[aparajon]

🤖 Re-approving at 2106b409 — my earlier stamp was at d7b7ee86. The head move is a merge of origin/main, and the resolution is genuinely semantic: this branch's arms were adapted to #1319's typed cause/remedy fields rather than left on the flat detail, which is what keeps #1319's new cause assertion satisfied.

One low finding in the follow-up comment, created by the merge rather than by either PR: the invalid-index advice now flows through the composition #1319 taught to survive the 240-byte CLI status clamp, and two of its four shapes lose the remedy entirely (other-table and not-droppable at max-length identifiers, 396 B and 379 B). Same operator-facing gap #1319 just closed for create-collision, and the same fix applies. The Permanent() seam finding from my first review is unchanged.

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

[Kiran01bm]

Review response from Kiran's (@Kiran01bm) code review assessment agent (Amp / Claude Opus 4.5)

All six findings on #1333 are resolved in the follow-up commit or already on main; one (dual permanence encoding) is kept as designed.

#	Finding	Status	Explanation
3	CodePoolTooSmall treated as retryable though pg-sprite marks it permanent	fixed	Own pool-too-small refusal in refusalForOutcome; removed from the operational arm. Pool size is fixed by the target DSN, so a retry is refused at admission the same way.
4	Totality test checks codes are known, not that dispositions match Code.Permanent()	fixed	TestRefusalForOutcomeTotalOverExecutorCodes now asserts Permanent() == refused per code, with the single deliberate exception (BudgetStatementExceeded) in a named map and a stale-exception check.
2	Constraint-backed (not-droppable) invalid-index branch untested	fixed	Added TestEngineApplyConcurrentIndexBackingConstraintRefused: UNIQUE-constraint index left invalid under the requested name → refused, non-retryable, detail points at the constraint's index rather than a drop.
1	docs/postgresql.md says every invalid-index outcome is retryable	fixed	Bullet rewritten: operator-clearable invalid indexes retry; other-table / constraint / partitioned-table indexes and pool-too-small refuse permanently.
6	Refusal log and doc wording lag the vocabulary	fixed	Already on main via #1319 and merged into this branch; head emits the refusal Warn log with cause/remedy halves.
5	Permanence encoded twice (Permanent() gate + advice table); known=false looks reachable	rejected	Intentional split: the upstream Permanent() gate decides whether to refuse, the table decides what to tell the operator, which only this engine can phrase. InvalidIndexError.Code() always returns one of the seven codes, so known cannot be false at runtime; the extended totality test (#4) keeps both encodings in lockstep.

"Verified correct" section (26-code enumeration, nested budget cause cannot shadow the verdict, reclassification is a bug fix): no action.

Source: adversarial review of #1333 at d7b7ee8, generated by Kiran's code review agent (Claude Code / claude-opus-5); not posted on the PR.