Skip to content

feat(postgres): execute greenfield CREATE TABLE through the native-safe path - #1209

Merged
Kiran01bm merged 2 commits into
mainfrom
kiran01bm/ct4-create-pin-bump
Aug 30, 2026
Merged

feat(postgres): execute greenfield CREATE TABLE through the native-safe path#1209
Kiran01bm merged 2 commits into
mainfrom
kiran01bm/ct4-create-pin-bump

Conversation

@Kiran01bm

Copy link
Copy Markdown
Collaborator

Greenfield CREATE TABLE now executes through the PostgreSQL native-safe path instead of planning as a blocked shape.

Why

The native-safe envelope stopped at ALTERs on existing tables, so a first table in a schema file could be reviewed but never applied. pg-sprite now provides a fail-closed create path (block/pg-sprite#63): absence and schema-CREATE proofs minted in the executing session, typed collision refusals, and declared-index sequencing. This PR adopts it so the envelope widens deliberately.

What

  • Bump the pg-sprite pin and route the off-ladder create tier through executor.ExecuteCreate: the existing-table preflight ladder never runs for a greenfield target (it states facts about a table that has none).
  • Plan-time privilege posture: the create tier is checked against the schema (CheckCreatePrivileges), not the table; steps that depend on a not-yet-created table are blocked with that dependency as the reason instead of an unanswerable "table not found" probe.
  • New refusal classifications: create collision / occupied name, schema not found, duplicate create name, and unsupported create shapes — all permanent refusals directing a re-plan, never retried.
  • The native-safe table size ceiling does not apply to creates: it bounds rewrites of existing data, and a table that does not exist yet has none.

Before / after

Before                                       After
┌──────────────────────────────────┐         ┌──────────────────────────────────┐
│ plan: CREATE TABLE → blocked     │         │ plan: CREATE TABLE → executable  │
│ "shape … does not execute yet"   │         │ (schema CREATE access verified)  │
│                                  │         │                                  │
│ apply: refused at acceptance     │         │ apply: absence + privilege       │
│                                  │         │ proofs in executing session      │
│                                  │         │  ├─ ok → create + indexes        │
│                                  │         │  ├─ name taken → create-collision│
│                                  │         │  │   (permanent, re-plan)        │
│                                  │         │  └─ no grant → provisioning SQL  │
└──────────────────────────────────┘         └──────────────────────────────────┘
Plan verdict rendering: greenfield CREATE TABLE

Before — always blocked:

statement for table "users" is a shape SchemaBot's PostgreSQL support does not execute yet; rewriting the change cannot make it eligible

After — executable when the role can create in the schema; a step that depends on the table's creation reads:

table "users" does not exist on the target; this statement depends on the statement that creates it — apply the creating change first, then re-plan

Copilot AI lite review requested due to automatic review settings August 30, 2026 05:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR expands SchemaBot’s PostgreSQL “native-safe” execution envelope to include greenfield CREATE TABLE, routing creates through pg-sprite’s dedicated create executor path so they can be planned as executable (when schema CREATE access is present) and applied with fail-closed absence/privilege proofs minted in the executing session.

Changes:

  • Route TierCreateTable privilege checks to schema-scoped CheckCreatePrivileges, and block non-create dependents when the target table is provably absent.
  • Execute greenfield CREATE TABLE via executor.ExecuteCreate, adding typed permanent refusal classifications for create collisions and unsupported create shapes.
  • Add/adjust integration tests for executable planning and end-to-end create apply behavior; bump the pg-sprite dependency and update docs.

Reviewed changes

Copilot reviewed 8 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
pkg/engine/postgres/postgres.go Updates plan-time privilege checking to support schema-scoped CREATE checks and dependency-based blocking when the table is absent.
pkg/engine/postgres/postgres_test.go Adjusts unit test expectations for the absent-table dependency reason and ensures no privilege probe occurs.
pkg/engine/postgres/postgres_integration_test.go Updates plan test to expect executable greenfield CREATE and adds integration coverage for create apply + collision refusal.
pkg/engine/postgres/apply.go Adds the greenfield create execution path using pg-sprite’s create executor and new refusal classifications.
pkg/engine/postgres/apply_test.go Updates acceptance-time validation test to use a non-native-supported shape now that CREATE is supported.
go.mod Bumps pg-sprite to a newer pseudo-version containing the native-safe create support.
go.sum Records checksum entries for the updated pg-sprite version.
e2e/consumermodule/go.mod Updates the consumer module’s indirect pg-sprite pin to match the root module.
e2e/consumermodule/go.sum Updates checksums for the consumer module’s updated pg-sprite version.
docs/configuration.md Documents that the native-safe table size ceiling does not apply to greenfield CREATE TABLE.

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

Comment thread pkg/engine/postgres/postgres.go Outdated
@aparajon

Copy link
Copy Markdown
Collaborator

🤖 Adversarial correctness review, requested by @aparajon and performed by their agent. Reviewed at head 85a67c9d.

Verdict: the routing is right and the create path is proved end to end — but two operator-facing messages on the greenfield plan are wrong in a way I could reproduce, and the pg-sprite pin points at a commit that is not on pg-sprite main. Nothing here is a safety break: every new outcome is a permanent refusal, and the executor cross-verifies its own proofs. The findings are about what the plan comment tells an operator to do, and about what the pin will mean at the next bump.

CI ran only three checks on this draft (Semgrep, zizmor, DCO) — the test suite has not run — so I ran the package's unit and integration tests locally at head instead; all pass. The branch is 10 commits behind main and currently CONFLICTING.

Findings

1. When the role cannot create in the schema, the plan tells the operator to do something the same plan says they cannot do. Reproduced against a real target with a role holding USAGE but not CREATE on public, planning CREATE TABLE users (...) plus a declared index:

step 0 — CREATE TABLE public.users (…) → blocked: the engine role lacks access for create a new table …; provision with: GRANT CREATE ON SCHEMA "public" TO "zz_limited"
step 1 — CREATE INDEX CONCURRENTLY idx_users_email … → blocked: table "users" does not exist on the target; this statement depends on the statement that creates it — apply the creating change first, then re-plan

There is no creating change to apply. The cause is ordering: blockAbsentTableDependents writes the dependent reason at the top of blockMissingPrivileges, before the privilege loop below decides the create step's own fate, so the message asserts an action whose availability is still unknown. The old wording ("the statement that would create it is blocked, so this statement cannot run") was accurate for exactly this case. Either move the dependent-blocking below the privilege loop and word it on the create step's actual verdict, or make it neutral — "this statement depends on the statement that creates it; see that statement's verdict above".

2. The create tier's privilege refusal names a table that does not exist, and the wrong scope. Same reproduction, step 0: "lacks access for create a new table on table "users"". classifyRefusal's privilege detail is templated as "the engine role lacks access for %s on table %q", which reads correctly for the table-scoped tiers ("for in-place ALTER TABLE on table "users"") but not for the create tier, whose name is a verb phrase and whose access is schema-scoped — as the GRANT CREATE ON SCHEMA "public" in the very same sentence shows. The create tier wants its own phrasing, e.g. "lacks access to create table "users" in schema "public"".

3. The pin is a commit that is not on pg-sprite main. github.com/block/pg-sprite v0.1.1-0.20260830051709-d2504e81cd64 resolves to d2504e81, the head of a closed PR branch; git merge-base --is-ancestor d2504e81 origin/main is false. The content is right — that tree is byte-identical to main at c1d33b6 — but pinning off the main line means the pin is not reachable from pg-sprite's released history and cannot be reasoned about by version ordering against future main commits. c1d33b6 is 14 minutes newer, so go get github.com/block/pg-sprite@c1d33b6 is a clean forward move to v0.1.1-0.20260830053109-c1d33b6b85c8, not a downgrade. Related: the root go.sum was not tidied — go mod tidy drops four stale lines, v0.1.0 and v0.1.1-0.20260828104643-ab8ca3a32dce (an intermediate commit from that same closed branch). e2e/consumermodule/go.sum was tidied correctly, so the two modules currently disagree.

4. executor.ErrInvariantViolation is not classified, so it surfaces as retryable. The create path adds four new invariant checks inside ExecuteCreate (absence proof empty, creation-role proof empty, role/absence schema mismatch, desired/absence table mismatch). None of the six new classifyRefusal arms matches ErrInvariantViolation, so it falls to the final branch and is published with Retryable = true and "see server logs" — telling an operator to retry a violated invariant. Pre-existing on the alter path via its own ST-7 check, but this PR widens the surface fourfold.

Interlock: docs/postgresql.md in #1144 still states "CREATE TABLE is not supported by the current apply path. Adding a new table therefore produces a blocked plan." This PR falsifies that page, and both are open at once.

Action items

  1. Reorder or reword the absent-table dependent reason so it never instructs the operator to apply a change the same plan has blocked.
  2. Give the create tier its own privilege-refusal phrasing — schema-scoped, and without "on table" for a table that does not exist.
  3. Re-pin to c1d33b6 (pg-sprite main) rather than the closed branch's head, and run go mod tidy on the root module.
  4. Classify executor.ErrInvariantViolation as a permanent refusal so an invariant violation is never offered as retryable.
  5. Rebase onto main — the branch is 10 commits behind and conflicts in pkg/engine/postgres/postgres.go and docs/configuration.md; the resolution must keep the plan-time size ceiling's own absent-table early return.
  6. (optional) Land after docs: document the PostgreSQL support envelope #1144 with its CREATE TABLE paragraph updated, or update that paragraph here.

Verified (tried to break, couldn't)

go build ./..., go test ./pkg/engine/postgres/..., and go test -tags=integration ./pkg/engine/postgres/... all pass locally at head, including the two new integration tests. The six new refusal classifications are genuinely reachable rather than dead: SequenceStepError implements Unwrap, so errors.Is traverses the fmt.ErrorfSequenceStepError → cause chain, and the collision test confirms it end to end with Retryable == false. The two CREATE TABLEDROP TABLE swaps in the shape-refusal tests are the correct kind of test change — the shape genuinely became native, and DROP TABLE still exercises RequiredTier's default arm, so the invariant keeps a guard rather than losing one. Skipping the size ceiling for creates is consistent on both surfaces: the apply path returns before CheckTable, and the plan-time ceiling already early-returns on TableExists == false, so no probe runs against a table that does not exist. An empty Namespace is not a silent public bind — both CheckCreatePrivileges and CheckTableAbsent resolve '' to current_schema() — and a disagreement between the two proofs cannot slip through, because ExecuteCreate refuses when cr.Schema() != at.Schema() (that refusal is finding 4's subject, not a correctness hole). blockAbsentTableDependents leaves steps that already carry a verdict untouched, and when it blocks everything required is empty and the function returns before any probe, so the nil-pool unit tests still prove no probe runs. One ordering nit not worth its own finding: the helper mutates changes in place before the report.Table == "" invariant check, so that error path returns after having written a table "" reason into the caller's slice.

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

@aparajon aparajon left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Approving on @aparajon's behalf after the adversarial correctness review above. The findings are follow-ups, not blockers.

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

…fe path

Bump the pg-sprite pin and route the off-ladder create tier through
pg-sprite's create path: absence and schema-CREATE proofs are minted in
the executing session, collisions and unsupported create shapes surface
as permanent refusals directing a re-plan, and steps that depend on a
not-yet-created table are blocked with that dependency as the reason
instead of an unanswerable privilege probe. The table size ceiling does
not apply to creates: it bounds rewrites of existing data.
@Kiran01bm
Kiran01bm force-pushed the kiran01bm/ct4-create-pin-bump branch from 85a67c9 to 598ad7d Compare August 30, 2026 06:56
Review follow-up: the refusal classifier hand-mirrored a subset of
pg-sprite sentinels, so unmapped outcomes (notably if-not-exists)
stayed retryable forever. Classification is now total over
executor.Codes(), invariant violations fail closed, create-tier
privilege wording is schema-scoped, the unnamed-target guard runs
before dependent-step blocking, and the absent-table dependent
reason stays neutral about the create step's undecided verdict.
Pin moves to the tagged v0.2.0 release, clearing the orphan
pseudo-version from go.sum.
@Kiran01bm

Copy link
Copy Markdown
Collaborator Author

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

# Concern Status
1 Dependent-step reason instructs "apply the creating change first" even when the same plan blocks the create (no applicable creating change exists) fixed — adopted the review's neutral-wording option: "…depends on the statement that creates it — see that statement's verdict". The reason is written before the privilege loop decides the create's fate, so it no longer asserts an availability it cannot know
4 executor.ErrInvariantViolation unclassified → surfaced as retryable "see server logs" fixed — refusal classification is now total over executor.Codes(); CodeInvariantViolation maps to a permanent fail-closed refusal, exhaustiveness pinned by TestRefusalForOutcomeTotalOverExecutorCodes
2 Create-tier privilege refusal names a nonexistent table and the wrong scope fixed — tier-branched wording "in the schema that would hold table %q"; asserted with a wantNotDetail on the table-grant phrasing
3 Pin resolves to a commit off pg-sprite main; root go.sum untidied, modules disagree fixed — pinned past the suggested c1d33b6 to the tagged v0.2.0 release (same tree, on main, orderable); both modules agree and the stale/orphan pseudo-version lines are gone
5 Branch 10 commits behind main and conflicting; resolution must keep the plan-time size ceiling's absent-table early return fixed — rebased to a single commit on main; TestEnginePlanCreateTableIgnoresSizeCeiling proves a greenfield create survives a 1-byte ceiling (the #1199 interaction this PR makes reachable)
6 (optional) #1144 still documents CREATE TABLE as unsupported while both PRs are open deferred — #1144's CREATE TABLE section is reworked in that PR against the widened envelope; tracked as an internal follow-up
nit Helper mutates changes before the report.Table == "" invariant check fixed — guard hoisted ahead of blockAbsentTableDependents in blockMissingPrivileges

@Kiran01bm
Kiran01bm marked this pull request as ready for review August 30, 2026 07:09
@chatgpt-codex-connector

Copy link
Copy Markdown

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

@Kiran01bm
Kiran01bm enabled auto-merge (squash) August 30, 2026 07:10
@Kiran01bm
Kiran01bm merged commit 7be3f33 into main Aug 30, 2026
37 checks passed
@Kiran01bm
Kiran01bm deleted the kiran01bm/ct4-create-pin-bump branch August 30, 2026 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants