Phase 0: scaffold, dbconn layer, PG 14-18 harness, governance

[Kiran01bm]

Summary

Phase 0 foundations for pg-sprite: the repo scaffold, the connection layer, the design-doc baseline, and OSS governance. First PR of the foundation stack (see the stack overview in the top PR).

What

• CLI scaffold (Kong): migrate / diff / fmt / lint / status command tree with shared DB flags.
• pkg/dbconn: pgx/v5 pool behind a Config seam — bounded lock_timeout / statement_timeout on every session, retries, RDS/Aurora auto-TLS via an embedded global CA bundle.
• PG 14–18 containerized test harness (internal/testutil).
• Design docs: vision-level docs, safety partition (SAFETY.md), TCB model, orchestrator-integration seam.
• OSS governance: Apache-2.0 LICENSE, CONTRIBUTING, CODE_OF_CONDUCT, WIP banner, OSPO-seeded templates.

Why

Everything later (planner, executors) builds on a connection layer with bounded sessions and a safety-partitioned repo layout; landing it separately keeps each subsequent phase PR reviewable on its own terms.

[chatgpt-codex-connector]

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

[aparajon]

🤖 Review requested by Armand and performed by his agent — same two lenses used across this stack (#4, #5, #6): pg-sprite as an OSS-first, best-in-class Postgres DDL tool, and pg-sprite as a clean integration target for an orchestrator. Reviewed at head 8a87ba4. This PR is already merged, so everything here is follow-up material rather than merge-blocking — but an adversarial pass on the connection layer turned up one item worth fixing soon (posted separately). Reviewing the base of the stack last means the later PRs' behavior informed what to look for here.

pkg/dbconn is the right foundation: bounded lock_timeout/statement_timeout on every session (not per-call opt-in), a Config seam that keeps pgxpool's defaults unless the caller has a reason, and BeforeConnect for short-lived credentials — that last one is exactly the hook an orchestrator needs for IAM auth tokens, and it's rare to see it anticipated this early.

OSS lens

1. The embedded RDS bundle needs a documented refresh contract. rdsGlobalBundle.pem is go:embed-ed so connections verify out of the box, which is a genuinely good adopter experience. But AWS rotates those roots, and an embedded bundle that goes stale doesn't degrade — it fails every connection at once, at a moment nobody has associated with pg-sprite. Nothing in the repo records where the bundle came from beyond a comment, when it was fetched, or who refreshes it. Suggest a short section in the docs (or a make refresh-rds-bundle target) covering the source URL, the fetch date, and the check — and ideally a CI job that fails when the embedded roots are within N months of expiry, so the warning arrives before the outage.

2. Auto-TLS only covers one AWS partition. IsRDSHost anchors on a host ending exactly in .rds.amazonaws.com, so endpoints in partitions with a different DNS suffix (notably the China partition, .amazonaws.com.cn) never take the auto-verify path and silently fall back to whatever the connection string asked for. For a tool aiming at broad adoption, "RDS just works" should not be region-dependent — and see the adversarial comment for a sharper form of the same root cause.

3. CONTRIBUTING closes the door at the moment enthusiasm peaks. "PRs opened at this stage will likely be closed without review" is honest and I understand the intent, but it's the first thing a curious Postgres person reads. The safety-issue carve-out is the right instinct; consider extending it to a "here's what we'd take today" line (repro cases, PostgreSQL-version findings, docs corrections) so early interest converts into signal rather than bouncing.

Integration lens

4. application_name is hard-coded, and PR Phase 1: optimistic front door with typed not-native-safe verdicts #5's status keys on it. buildPoolConfig sets application_name = "pg-sprite" with no Config override, while StatusCmd.querySessions (landed in Phase 1: optimistic front door with typed not-native-safe verdicts #5) filters WHERE application_name = 'pg-sprite'. In the decided embedding — pg-sprite inside a control plane driving many databases — every connection from every tenant and every concurrent change carries the same identity: status lists them all with nothing to attribute a session to a caller, an apply, or a tenant, and an operator triaging one stuck change sees an undifferentiated pile. Adding Config.ApplicationName (defaulting to pg-sprite, and ideally letting a caller append a suffix like pg-sprite/<apply-id>) is a small change now and a schema-of-a-contract change later, since status's filter has to stay compatible with whatever callers set.

5. TerminateBlockers is a privileged escape hatch with no audit seam. Terminating other people's backends is the most operationally sensitive thing in this package, and today it's a plain function that returns pids. For control-plane embedding, the orchestrator needs to record who invoked it, why, and what it killed — worth deciding now whether that's the caller's job (in which case say so in the doc comment) or whether the package should take a logger and emit a structured event. See also the adversarial comment: today the failure mode reports fewer pids than it actually terminated.

6. Retryable's classification is a de facto public contract. It's exported, it's the right set, and an orchestrator will end up branching on it — worth stating in the doc comment that lock_not_available is retryable by design (bounded lock waits are the mitigation, not a failure) so a consumer doesn't "fix" it by treating a lock timeout as fatal. The adversarial comment raises the idempotency question that goes with it.

Verified solid

The version matrix as a gate rather than documentation (PG 14–18, fail-fast: false, harness asserting the server major actually matches PG_VERSION) is the correct instinct and something most projects bolt on far too late — a matrix entry that silently fell back to a default image would still pass every test without that assertion. buildPoolConfig being pure and separately testable means TLS/pool wiring is verified without a server. The IsRDSHost regex's leading \. deliberately prevents fake-rds.amazonaws.com from spoofing into the auto-TLS path, with a comment explaining why — the right kind of comment. Timeouts are session-level defaults rather than per-call decoration, so a forgotten call site can't run unbounded. The safety partition (SAFETY.md), invariant registry, and TCB model existing before the code they govern is unusual discipline; starting invariants.md at Phase 0 is what makes the later INV: comments in the executor meaningful rather than decorative.

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

[aparajon]

🤖 Adversarial correctness review requested by Armand and performed by his agent — separate from the two-lens pass. Method: attack the code, then verify every candidate finding against a real PostgreSQL / the real config path at head 8a87ba4 (checkout + testcontainers). This PR is merged, so these are follow-ups — but finding 1 is security-relevant and worth a fix soon rather than at leisure.

Findings, most severe first

1. An RDS hostname with any uppercase letter silently loses TLS verification and gains a plaintext fallback. IsRDSHost matches with a case-sensitive regex, but DNS is case-insensitive — hostnames arrive however the user, secrets store, or connection-string template spelled them. When the match fails, configureTLS takes no branch at all and the connection keeps pgx's default (sslmode=prefer). Reproduced side by side:

url=postgres://u:p@db.abc.us-east-1.rds.amazonaws.com:5432/d
  ServerName="db.abc.us-east-1.rds.amazonaws.com" RootCAs!=nil=true  InsecureSkipVerify=false   (fallbacks: 0)

url=postgres://u:p@db.abc.us-east-1.RDS.amazonaws.com:5432/d
  ServerName="db.abc.us-east-1.RDS.amazonaws.com" RootCAs!=nil=false InsecureSkipVerify=true    (fallbacks: 1, tls=false)

One capitalized letter turns verify-full-with-the-embedded-RDS-roots into encrypt-without-verifying plus a cleartext fallback — a MITM-able connection to a production database, with no warning anywhere. The same miss happens for the fully-qualified trailing-dot form (…rds.amazonaws.com.), which is legal DNS and appears in some resolver configurations. Fix: case-fold (and strip a trailing dot) before matching — (?i) plus a strings.TrimSuffix(host, "."). Worth also considering whether a failed RDS match on a host that merely looks AWS-shaped deserves a warning log, since the failure is silent by construction.

Related, and deliberate but worth a second look: sslmode=disable on an RDS host is honored as written, so the engine will happily talk plaintext to Aurora if a connection string says so. That's defensible ("the caller chose"), but given this package's premise — that the engine supplies safe defaults the caller can't forget — a loud warning would fit the house style better than silence.

2. TerminateBlockers can kill backends and report failure. The query filters rows with WHERE pg_terminate_backend(b.pid), so termination happens as a side effect of row evaluation. If a later target raises (permission denied is the realistic case — 42501, confirmed below), the whole query errors and TerminateBlockers returns (nil, err) — while the earlier targets are already dead. Reproduced with the same query shape:

terminate [own=78, superuser=77] as limited role: err=ERROR: permission denied to terminate process (SQLSTATE 42501)
own session (pid 78) still alive after the failed call: false

The first backend was terminated; the caller was told the operation failed and learned nothing about it. For a cutover escape hatch, "I killed some of your connections but I'm reporting an error" is the worst possible report — an operator retries, or assumes nothing happened. Fix: collect the pid list first, terminate one at a time (or use a LEFT JOIN LATERAL that yields a per-pid success flag), and return both the terminated set and the failures, so a partial outcome is representable in the return type rather than lost in an error.

3. Retry re-invokes non-idempotent work on ambiguous errors. Retryable includes SQLSTATE class 08 (connection exception), which is exactly the class where the statement's fate is unknown — the server may well have committed before the connection dropped. Retry then calls fn again. Confirmed:

connection-class error: fn invoked 3 times, final err=retries exhausted after 3 attempts: connection failure (SQLSTATE 08006)
Retryable(08006)=true  Retryable(55P03 lock_timeout)=true  Retryable(57014 statement_timeout)=false

The classification set is right; the missing piece is the contract. Nothing in Retry's doc says fn must be idempotent, and DDL is the least idempotent work there is: a committed-then-disconnected ALTER TABLE … ADD COLUMN retries into "column already exists", surfacing as a failure for a change that actually succeeded (or worse, for statements where the retry is destructive rather than merely noisy). Since this package is the foundation every later executor builds on, the doc comment should state the requirement explicitly, and ideally the API should make it hard to get wrong — e.g. a separate RetryIdempotent name, or splitting the connection class out of the default retryable set so a caller opts into ambiguity deliberately.

Probed and held

I expected TerminateBlockers to silently under-report when pg_terminate_backend returns false, and it mostly doesn't: an unprivileged termination errors (42501) rather than filtering the row away, so the caller does learn something went wrong (finding 2 is about the other half — what it doesn't learn). The only false-without-error case is a pid that no longer exists, which is benign: the blocker is already gone. Retryable correctly excludes 57014 (statement_timeout), so a cancelled long-running statement is never retried into a second attempt — the right call, and the distinction from 55P03 is deliberate. The IsRDSHost anti-spoof guard holds: fake-rds.amazonaws.com does not match. A URL whose password contains the literal text sslmode= does not derail the auto-TLS branch (the check is on the raw URL, but percent-encoding keeps the substring out of the common cases I could construct). buildPoolConfig's zero-value handling is consistent — every option is "zero means keep the default", with no field where zero means "disable".

Reproduction tests

Finding 1 — pkg/dbconn/adv_test.go: RDS host case-sensitivity and the resulting TLS config (no database needed)

package dbconn

import (
	"testing"

	"github.com/stretchr/testify/require"
)

// Is the RDS host match case-sensitive? DNS is not.
func TestAdvRDSHostCase(t *testing.T) {
	for _, h := range []string{
		"db.abc123.us-east-1.rds.amazonaws.com",
		"db.abc123.us-east-1.RDS.amazonaws.com",
		"DB.ABC123.US-EAST-1.RDS.AMAZONAWS.COM",
		"db.abc123.us-east-1.rds.amazonaws.com.",
		"db.abc123.rds.cn-north-1.amazonaws.com.cn",
	} {
		t.Logf("IsRDSHost(%-45q) = %v", h, IsRDSHost(h))
	}
}

// What TLS config results for an uppercase RDS host with no sslmode?
func TestAdvUppercaseRDSTLS(t *testing.T) {
	for _, url := range []string{
		"postgres://u:p@db.abc.us-east-1.rds.amazonaws.com:5432/d",
		"postgres://u:p@db.abc.us-east-1.RDS.amazonaws.com:5432/d",
	} {
		pc, err := buildPoolConfig(Config{URL: url})
		require.NoError(t, err)
		tls := pc.ConnConfig.TLSConfig
		t.Logf("url=%s\n  TLSConfig=%v fallbacks=%d", url, tls != nil, len(pc.ConnConfig.Fallbacks))
		if tls != nil {
			t.Logf("  ServerName=%q RootCAs!=nil=%v InsecureSkipVerify=%v",
				tls.ServerName, tls.RootCAs != nil, tls.InsecureSkipVerify)
		}
		for i, f := range pc.ConnConfig.Fallbacks {
			t.Logf("  fallback[%d] tls=%v", i, f.TLSConfig != nil)
		}
	}
}

Observed:

IsRDSHost("db.abc123.us-east-1.rds.amazonaws.com"      ) = true
IsRDSHost("db.abc123.us-east-1.RDS.amazonaws.com"      ) = false
IsRDSHost("DB.ABC123.US-EAST-1.RDS.AMAZONAWS.COM"      ) = false
IsRDSHost("db.abc123.us-east-1.rds.amazonaws.com."     ) = false
IsRDSHost("db.abc123.rds.cn-north-1.amazonaws.com.cn"  ) = false

url=postgres://u:p@db.abc.us-east-1.rds.amazonaws.com:5432/d
  ServerName="db.abc.us-east-1.rds.amazonaws.com" RootCAs!=nil=true InsecureSkipVerify=false
url=postgres://u:p@db.abc.us-east-1.RDS.amazonaws.com:5432/d
  ServerName="db.abc.us-east-1.RDS.amazonaws.com" RootCAs!=nil=false InsecureSkipVerify=true
  fallback[0] tls=false

Findings 2 and 3 — pkg/dbconn/adv_integration_test.go: partial termination reporting, and Retry re-invocation

package dbconn_test

import (
	"context"
	"net/url"
	"testing"
	"time"

	"github.com/jackc/pgx/v5/pgconn"
	"github.com/stretchr/testify/require"

	"github.com/block/pg-sprite/internal/testutil"
	"github.com/block/pg-sprite/pkg/dbconn"
)

// The TerminateBlockers query shape: SELECT pid FROM unnest(...) WHERE
// pg_terminate_backend(pid). If one target succeeds and a later one raises,
// what does the caller learn about the ones already terminated?
func TestAdvPartialTerminationReporting(t *testing.T) {
	superURL := testutil.StartPostgres(t)
	super, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: superURL})
	require.NoError(t, err)
	t.Cleanup(super.Close)
	_, err = super.Exec(t.Context(), "CREATE ROLE limited LOGIN PASSWORD 'pw'")
	require.NoError(t, err)

	u, err := url.Parse(superURL)
	require.NoError(t, err)
	u.User = url.UserPassword("limited", "pw")
	limited, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: u.String()})
	require.NoError(t, err)
	t.Cleanup(limited.Close)

	// Target A: another session owned by the limited role (terminable).
	ownConn, err := limited.Acquire(t.Context())
	require.NoError(t, err)
	var ownPID int
	require.NoError(t, ownConn.QueryRow(t.Context(), "SELECT pg_backend_pid()").Scan(&ownPID))
	// Target B: a superuser session (not terminable by the limited role).
	superConn, err := super.Acquire(t.Context())
	require.NoError(t, err)
	t.Cleanup(superConn.Release)
	var superPID int
	require.NoError(t, superConn.QueryRow(t.Context(), "SELECT pg_backend_pid()").Scan(&superPID))

	// Same query shape as TerminateBlockers, with an explicit pid list.
	rows, err := limited.Query(t.Context(),
		`SELECT b.pid FROM unnest(ARRAY[$1::int, $2::int]) AS b(pid) WHERE pg_terminate_backend(b.pid)`,
		ownPID, superPID)
	if err == nil {
		rows.Close()
		err = rows.Err()
	}
	t.Logf("terminate [own=%d, superuser=%d] as limited role: err=%v", ownPID, superPID, err)

	time.Sleep(200 * time.Millisecond)
	var alive bool
	require.NoError(t, super.QueryRow(t.Context(),
		"SELECT EXISTS (SELECT 1 FROM pg_stat_activity WHERE pid = $1)", ownPID).Scan(&alive))
	t.Logf("own session (pid %d) still alive after the failed call: %v", ownPID, alive)
	ownConn.Release()
}

// Retry re-invokes fn on connection-class errors. Is the contract safe for
// non-idempotent work such as DDL?
func TestAdvRetryReinvokesOnConnectionError(t *testing.T) {
	calls := 0
	connErr := &pgconn.PgError{Code: "08006", Message: "connection failure"}
	err := dbconn.Retry(t.Context(), 3, time.Millisecond, func(context.Context) error {
		calls++
		return connErr
	})
	t.Logf("connection-class error: fn invoked %d times, final err=%v", calls, err)
	t.Logf("Retryable(08006)=%v Retryable(55P03 lock_timeout)=%v Retryable(57014 statement_timeout)=%v",
		dbconn.Retryable(connErr),
		dbconn.Retryable(&pgconn.PgError{Code: "55P03"}),
		dbconn.Retryable(&pgconn.PgError{Code: "57014"}))
}

Observed:

terminate [own=78, superuser=77] as limited role: err=ERROR: permission denied to terminate process (SQLSTATE 42501)
own session (pid 78) still alive after the failed call: false

connection-class error: fn invoked 3 times, final err=retries exhausted after 3 attempts: connection failure (SQLSTATE 08006)
Retryable(08006)=true Retryable(55P03 lock_timeout)=true Retryable(57014 statement_timeout)=false

This review was generated by Claude Code (claude-fable-5). All three findings were reproduced at head 8a87ba4 using the tests above (finding 1 offline, findings 2–3 against a live PostgreSQL 16).