From 0051c850059ae2dd428ded1f871cf5cb6230a5de Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Tue, 4 Aug 2026 16:14:32 +1000 Subject: [PATCH 1/5] test/ci: PG 14-18 dev database, TLS tests, docs-only gating, hooks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Compose-based local database mirrors the CI matrix (PG_DSN path in the harness); docs-only PRs skip the heavy jobs behind a fail-open filter with a single ci-ok required status; pre-commit/pre-push hooks give fast local feedback and skip cleanly on Markdown-only changes. A TLS-only server harness proves the verify-full CACertPath path live (trusted CA encrypts, plaintext and untrusted CA fail closed) — the one peer-CI pattern (pg-delta) applicable to Phase 0. docs/testing.md records Phase 0 coverage, Phases 1-7 test obligations including the topology patterns surveyed from peer-tool CIs, and the vanilla PostgreSQL vs real-Aurora validation boundary. The lint config adopts SchemaBot's proven linter set plus gofmt/goimports formatters, with a staged-files lint-fix pre-commit flow ported from the same repo. --- .githooks/pre-commit | 48 ++++++++ .githooks/pre-push | 115 +++++++++++++++++++ .github/workflows/ci.yml | 74 +++++++++++- .golangci.yml | 33 +++++- AGENTS.md | 10 +- Makefile | 39 ++++++- README.md | 15 ++- compose/compose.yml | 21 ++++ docs/README.md | 1 + docs/testing.md | 84 ++++++++++++++ internal/testutil/postgres.go | 25 +++-- internal/testutil/postgres_test.go | 59 ++++++++++ internal/testutil/postgres_tls.go | 174 +++++++++++++++++++++++++++++ pkg/dbconn/tls_integration_test.go | 64 +++++++++++ scripts/lint-fix.sh | 86 ++++++++++++++ 15 files changed, 829 insertions(+), 19 deletions(-) create mode 100755 .githooks/pre-commit create mode 100755 .githooks/pre-push create mode 100644 compose/compose.yml create mode 100644 docs/testing.md create mode 100644 internal/testutil/postgres_test.go create mode 100644 internal/testutil/postgres_tls.go create mode 100644 pkg/dbconn/tls_integration_test.go create mode 100755 scripts/lint-fix.sh diff --git a/.githooks/pre-commit b/.githooks/pre-commit new file mode 100755 index 0000000..22b1492 --- /dev/null +++ b/.githooks/pre-commit @@ -0,0 +1,48 @@ +#!/bin/sh +set -e + +# Pre-commit: fast local feedback on staged Go files. Docs-only commits +# (no staged .go files) skip the Go checks entirely so Markdown edits stay +# instant. CI remains the authoritative gate. +# +# Enable with: make setup (sets core.hooksPath .githooks) + +# Ensure we're at the worktree root (not the git common dir). Unset GIT_DIR +# first — git sets it during hooks and it makes subprocess git commands +# resolve the wrong repo root. +unset GIT_DIR +cd "$(git rev-parse --show-toplevel)" + +staged_go=$(git diff --cached --name-only --diff-filter=d -- '*.go') +if [ -z "$staged_go" ]; then + echo "pre-commit: no staged Go files; skipping Go checks." + exit 0 +fi + +# Ensure GOROOT matches the Go version in go.mod. External tools (hermit, +# asdf) may set GOROOT to a different version, causing "compile: version +# does not match go tool version" errors in golangci-lint and go build. +GO_VERSION=$(sed -n 's/^go //p' go.mod | head -1) +if [ -n "$GO_VERSION" ]; then + for candidate in \ + "/opt/homebrew/opt/go/libexec" \ + "/usr/local/go" \ + "$HOME/sdk/go${GO_VERSION}" \ + "$HOME/Library/Caches/hermit/pkg/go-${GO_VERSION}"; do + if [ -x "$candidate/bin/go" ]; then + CANDIDATE_VERSION=$("$candidate/bin/go" version 2>/dev/null | grep -o "go${GO_VERSION}" || true) + if [ -n "$CANDIDATE_VERSION" ]; then + export GOROOT="$candidate" + export PATH="$GOROOT/bin:$PATH" + break + fi + fi + done +fi +export PATH="$PATH:$HOME/go/bin:/usr/local/go/bin:/opt/homebrew/bin" + +# Everything staged must still build. +go build ./... + +# gofmt + golangci-lint on staged files: auto-fix, re-stage, verify. +scripts/lint-fix.sh diff --git a/.githooks/pre-push b/.githooks/pre-push new file mode 100755 index 0000000..3e7c8bd --- /dev/null +++ b/.githooks/pre-push @@ -0,0 +1,115 @@ +#!/bin/sh +set -e + +# Pre-push: run unit tests (go test -race, integration skipped) for the +# packages touched by the commits being pushed. Docs-only pushes skip +# entirely. CI remains the authoritative gate and runs the full suite +# including the PostgreSQL matrix — this hook does not replace it. +# +# Reads the standard pre-push stdin records: +# + +unset GIT_DIR +cd "$(git rev-parse --show-toplevel)" + +remote_name="$1" + +# Ensure GOROOT matches the Go version in go.mod (see pre-commit). +GO_VERSION=$(sed -n 's/^go //p' go.mod | head -1) +if [ -n "$GO_VERSION" ]; then + for candidate in \ + "/opt/homebrew/opt/go/libexec" \ + "/usr/local/go" \ + "$HOME/sdk/go${GO_VERSION}" \ + "$HOME/Library/Caches/hermit/pkg/go-${GO_VERSION}"; do + if [ -x "$candidate/bin/go" ]; then + CANDIDATE_VERSION=$("$candidate/bin/go" version 2>/dev/null | grep -o "go${GO_VERSION}" || true) + if [ -n "$CANDIDATE_VERSION" ]; then + export GOROOT="$candidate" + export PATH="$GOROOT/bin:$PATH" + break + fi + fi + done +fi +export PATH="$PATH:$HOME/go/bin:/usr/local/go/bin:/opt/homebrew/bin" + +# Resolve a base ref for new-branch pushes (no remote sha yet): prefer the +# push remote's default branch, then origin/main, then main. +base_ref="" +for cand in "refs/remotes/$remote_name/HEAD" "refs/remotes/$remote_name/main" "refs/remotes/origin/main" "main"; do + if git rev-parse --verify --quiet "$cand" >/dev/null 2>&1; then + base_ref="$cand" + break + fi +done + +# is_zero_sha returns success when the sha is all zeros (branch create/delete +# sentinel), independent of SHA-1 vs SHA-256 length. +is_zero_sha() { + case "$1" in + *[!0]*) return 1 ;; + *) return 0 ;; + esac +} + +changed_go_files="" +while read -r local_ref local_sha remote_ref remote_sha; do + # Skip branch deletions — nothing to test. + if is_zero_sha "$local_sha"; then + continue + fi + + if is_zero_sha "$remote_sha"; then + # New branch on the remote: test what this branch introduces relative + # to the base. Fall back to the single tip commit if no base exists. + if [ -n "$base_ref" ]; then + range_base=$(git merge-base "$base_ref" "$local_sha" 2>/dev/null || true) + else + range_base="" + fi + if [ -z "$range_base" ]; then + range_base="${local_sha}~1" + fi + else + range_base="$remote_sha" + fi + + files=$(git diff --name-only --diff-filter=d "$range_base" "$local_sha" -- '*.go' 2>/dev/null || true) + if [ -n "$files" ]; then + if [ -z "$changed_go_files" ]; then + changed_go_files="$files" + else + changed_go_files="$changed_go_files +$files" + fi + fi +done + +changed_go_files=$(printf '%s\n' "$changed_go_files" | sed '/^$/d' | sort -u) +if [ -z "$changed_go_files" ]; then + echo "pre-push: no Go changes in pushed commits; skipping unit tests." + exit 0 +fi + +# Map changed files to package directories that still exist and contain Go +# source for the default build. +pkgs="" +for dir in $(printf '%s\n' "$changed_go_files" | xargs -n1 dirname | sort -u); do + testable_pkg=$(go list -e -f '{{if or (or .GoFiles .TestGoFiles) .XTestGoFiles}}{{.ImportPath}}{{end}}' "./$dir" 2>/dev/null || true) + if [ -d "$dir" ] && [ -n "$testable_pkg" ]; then + pkgs="$pkgs ./$dir" + fi +done + +pkgs=$(printf '%s' "$pkgs" | sed 's/^ *//') +if [ -z "$pkgs" ]; then + echo "pre-push: changed Go files map to no testable packages; skipping unit tests." + exit 0 +fi + +echo "pre-push: running unit tests (race, integration skipped) for changed packages:" +for p in $pkgs; do echo " $p"; done + +# shellcheck disable=SC2086 +SKIP_INTEGRATION=1 go test -race -count=1 $pkgs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7546aec..4fc3def 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -5,8 +5,55 @@ on: branches: [main] pull_request: +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + jobs: + # Detect whether the change touches anything other than *.md / docs/**. + # PRs that touch only docs skip the heavy jobs below; pushes to main + # always run. Fails open: if detection fails, treat as a code change. + changes: + runs-on: ubuntu-latest + permissions: + pull-requests: read + outputs: + code: ${{ steps.out.outputs.code }} + steps: + - name: Detect non-docs changes + if: github.event_name == 'pull_request' + id: filter + continue-on-error: true + uses: dorny/paths-filter@d1c1ffe0248fe513906c8e24db8ea791d46f8590 # v3.0.3 + with: + filters: | + code: + - '**' + - '!**/*.md' + - '!docs/**' + - name: Resolve code-change flag + id: out + shell: bash + env: + EVENT_NAME: ${{ github.event_name }} + FILTER_OUTCOME: ${{ steps.filter.outcome }} + FILTER_CODE: ${{ steps.filter.outputs.code }} + run: | + set -euo pipefail + if [ "$EVENT_NAME" != "pull_request" ]; then + code=true + elif [ "$FILTER_OUTCOME" != "success" ]; then + echo "::warning::paths-filter failed; running CI instead of skipping" + code=true + else + code="$FILTER_CODE" + fi + echo "code=$code" >> "$GITHUB_OUTPUT" + echo "event=$EVENT_NAME code=$code" + lint: + needs: changes + if: needs.changes.outputs.code == 'true' runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 @@ -16,6 +63,8 @@ jobs: - uses: golangci/golangci-lint-action@v6 build: + needs: changes + if: needs.changes.outputs.code == 'true' runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 @@ -25,9 +74,13 @@ jobs: - run: make build # The integration suite runs against every Aurora-supported PostgreSQL - # major (see the version-support research doc): the version floor is a - # promise CI enforces, not documentation. + # major (see docs/postgresql-version-support.md): the version floor is a + # promise CI enforces, not documentation. These are vanilla PostgreSQL + # images — real Aurora engine-version validation is a separate gate that + # cannot run in public CI. test: + needs: changes + if: needs.changes.outputs.code == 'true' runs-on: ubuntu-latest strategy: fail-fast: false @@ -41,3 +94,20 @@ jobs: with: go-version-file: go.mod - run: make test + + # Single required status for branch protection. Succeeds when nothing + # failed — including docs-only PRs where the heavy jobs were skipped. + ci-ok: + if: always() + needs: [changes, lint, build, test] + runs-on: ubuntu-latest + steps: + - name: Check job results + env: + RESULTS: ${{ toJSON(needs) }} + run: | + echo "$RESULTS" + if echo "$RESULTS" | grep -Eq '"result": *"(failure|cancelled)"'; then + echo "a required job failed or was cancelled" + exit 1 + fi diff --git a/.golangci.yml b/.golangci.yml index 31c51ae..c4d6ddc 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -2,6 +2,33 @@ version: "2" linters: enable: - - bodyclose - - misspell - - nolintlint + - errcheck # unchecked errors + - govet # suspicious constructs + - staticcheck # comprehensive static analysis + - ineffassign # useless assignments + - unused # unused code + - gocritic # opinionated but catches real bugs + - errorlint # proper error wrapping/comparison + - modernize # suggest modern Go idioms (wg.Go, etc.) + - noctx # requests without context + - bodyclose # unclosed HTTP response bodies + - usetesting # enforce t.Context() over context.Background() in tests + - unparam # unused/constant function parameters and results + - misspell # commonly misspelled words + - nolintlint # ill-formed or unexplained nolint directives + settings: + usetesting: + context-background: true + context-todo: true + exclusions: + rules: + # Test helpers intentionally keep uniform signatures and fixed arguments + # for readability, so unused/constant params there are not worth churn. + - path: _test\.go + linters: + - unparam + +formatters: + enable: + - gofmt + - goimports diff --git a/AGENTS.md b/AGENTS.md index efcd234..2f730d0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -14,9 +14,12 @@ SAFETY.md — the review bar and the AI-assistance posture differ by side. ## Build and test ```sh +make setup # one-time: configure git hooks (core.hooksPath .githooks) make build # build ./... and bin/pg-sprite make test # full suite; integration tests need Docker make test-unit # SKIP_INTEGRATION=1, no Docker +make test-db # suite against the compose DB (make db-up first); PG_DSN +make test-supported-postgres # full suite on every major 14 -> 18 make lint # golangci-lint ``` @@ -33,9 +36,10 @@ make lint # golangci-lint Every session runs under bounded `lock_timeout` / `statement_timeout`. - All SQL parsing goes through `pg_query_go` (once `pkg/statement` exists). No `strings.Split(";")`, no hand-parsing; a parse failure is an error surfaced to the caller. -- Tests use testify (`require` for setup, `assert` for verification), `t.Context()` (except in - cleanups, which run after the context is cancelled), and named polling deadlines — no bare - `time.Sleep` readiness waits. +- Tests use testify (`require` for setup, `assert` for verification), `t.Context()` (in + cleanups, which run after the context is cancelled, use + `context.WithoutCancel(t.Context())`), and named polling deadlines — no bare `time.Sleep` + readiness waits. - Errors: wrap with context and identifiers (`fmt.Errorf("create slot %s: %w", name, err)`); never log-and-continue; no silent branch cases; no `nolint`; no `--no-verify`. diff --git a/Makefile b/Makefile index 6fe553f..f8a23fc 100644 --- a/Makefile +++ b/Makefile @@ -1,6 +1,17 @@ GO ?= go +COMPOSE ?= docker compose -.PHONY: build test test-unit lint clean +# PostgreSQL major under test (see docs/postgresql-version-support.md). +PG_VERSION ?= 16 +PG_PORT ?= 5432 +# Local dev database credentials (compose/compose.yml); test-only defaults. +PG_USER ?= pgsprite +PG_PASSWORD ?= pgsprite +PG_DATABASE ?= pgsprite +# Localhost-only test credentials, parameterized above — not a real secret. +PG_DSN_LOCAL = postgres://$(PG_USER):$(PG_PASSWORD)@localhost:$(PG_PORT)/$(PG_DATABASE)?sslmode=disable# sadscan:disable np.postgres.1 + +.PHONY: build test test-unit test-db test-supported-postgres lint setup db-up db-down clean build: $(GO) build -o bin/pg-sprite ./cmd/pg-sprite @@ -13,8 +24,34 @@ test: test-unit: SKIP_INTEGRATION=1 $(GO) test -race ./... +# Integration suite against the long-lived compose database (no per-test +# containers). Start it first: make db-up [PG_VERSION=14] +test-db: + PG_DSN="$(PG_DSN_LOCAL)" $(GO) test -race -count=1 ./... + +# Full suite against every supported PostgreSQL major (14 -> 18) via +# testcontainers — the local mirror of the CI matrix. +test-supported-postgres: + @for v in 14 15 16 17 18; do \ + echo "=== PostgreSQL $$v ==="; \ + PG_VERSION=$$v $(GO) test -race -count=1 ./... || exit 1; \ + done + lint: golangci-lint run +# Configure git hooks (relative path so worktrees work too). +setup: + git config core.hooksPath .githooks + +# Start / stop the local development database (compose/compose.yml). +COMPOSE_ENV = PG_VERSION=$(PG_VERSION) PG_PORT=$(PG_PORT) PG_USER=$(PG_USER) PG_PASSWORD=$(PG_PASSWORD) PG_DATABASE=$(PG_DATABASE) + +db-up: + $(COMPOSE_ENV) $(COMPOSE) -f compose/compose.yml up --wait -d + +db-down: + $(COMPOSE_ENV) $(COMPOSE) -f compose/compose.yml down -v + clean: rm -rf bin diff --git a/README.md b/README.md index 8dc2a51..e1c9562 100644 --- a/README.md +++ b/README.md @@ -29,6 +29,7 @@ rules that apply inside the core. Read it before changing anything under ## Development ```sh +make setup # one-time: configure git hooks (.githooks) make build # build ./... and the bin/pg-sprite binary make test # full suite; integration tests need Docker make test-unit # unit tests only (SKIP_INTEGRATION=1) @@ -36,7 +37,19 @@ make lint # golangci-lint ``` Integration tests run against a real PostgreSQL via testcontainers. `PG_VERSION` -selects the major (default 16); CI runs the matrix 14 → 18. +selects the major (default 16); CI runs the matrix 14 → 18. To iterate against a +long-lived local database instead of per-test containers: + +```sh +make db-up PG_VERSION=14 # start PostgreSQL 14 on localhost via compose +make test-db # run the suite against it (PG_DSN) +make db-down # stop and discard it +``` + +`make test-supported-postgres` runs the full suite against every supported +major (14 → 18) — the local mirror of the CI matrix. See +[docs/testing.md](docs/testing.md) for the test-suite layout, what each build +phase owes, and the vanilla-PostgreSQL-vs-real-Aurora validation boundary. ## Contributing diff --git a/compose/compose.yml b/compose/compose.yml new file mode 100644 index 0000000..3a9046f --- /dev/null +++ b/compose/compose.yml @@ -0,0 +1,21 @@ +# Local development database — the long-lived analogue of the testcontainers +# harness. `make db-up PG_VERSION=14` starts one PostgreSQL major on +# localhost; `make test-db` points the integration suite at it via PG_DSN so +# no per-test containers are started. This is a test database: durability is +# deliberately relaxed and all state is discarded on `make db-down`. +services: + postgres: + image: postgres:${PG_VERSION:-16} + environment: + POSTGRES_USER: ${PG_USER:-pgsprite} + POSTGRES_PASSWORD: ${PG_PASSWORD:-pgsprite} + POSTGRES_DB: ${PG_DATABASE:-pgsprite} + ports: + - "${PG_PORT:-5432}:5432" + # Test-only speed settings; never carry these to a real deployment. + command: ["postgres", "-c", "fsync=off", "-c", "full_page_writes=off"] + healthcheck: + test: ["CMD-SHELL", "pg_isready -U $${POSTGRES_USER} -d $${POSTGRES_DB}"] + interval: 1s + timeout: 3s + retries: 60 diff --git a/docs/README.md b/docs/README.md index 5a44e96..fa46b97 100644 --- a/docs/README.md +++ b/docs/README.md @@ -44,6 +44,7 @@ checkpoint/resume, tuned for Aurora. That is the gap this engine targets. | [change-capture-tradeoff.md](change-capture-tradeoff.md) | The canonical **triggers vs logical-decoding** trade-off for copy-and-swap — overhead, failover survival, WAL risk, and whether either lets us drop the checksum/checkpoint (answer: keep the checksum; triggers simplify but don't remove the checkpoint). Any doc proposing logical decoding as the default points here. | | [invariants.md](invariants.md) | The canonical **invariant registry** — testable runtime MUST-statements (correctness, locking, state/resume, refusals, orchestration), each with its enforcement point and source. Mined from this doc set plus [Spirit](https://github.com/block/spirit)'s stated safety invariants and [SchemaBot](https://github.com/block/schemabot)'s control-plane discipline; the build plan's phases carry per-invariant test obligations. | | [tcb-model.md](tcb-model.md) | The **TCB model** — the trusted-computing-base partition of the engine: which components are the small trusted core that enforces the invariant registry vs the untrusted periphery, the never-trust-callers rule, domain types that make illegal states unrepresentable, the in-TCB engineering rules (from TigerBeetle TIGER_STYLE, s2n-tls, qmail, bitcoin-core), the verification ladder, and the per-side AI-assisted development policy. | +| [testing.md](testing.md) | The **test-suite guide** — how to run the suite (unit, per-major, all supported majors, compose database), what Phase 0 covers today, the per-phase deferred test obligations, and the vanilla-PostgreSQL-matrix vs real-Aurora validation boundary. | | [schemabot-integration.md](schemabot-integration.md) | The **single home for orchestrator integration** — how SchemaBot (the reference orchestrator) drives the engine: the pluggable-engine overview, the verb mappings, the concrete adapter contract, and the design constraints (OC-* invariants) the integration imposes on the core. | ## TL;DR recommendation diff --git a/docs/testing.md b/docs/testing.md new file mode 100644 index 0000000..8fdb56b --- /dev/null +++ b/docs/testing.md @@ -0,0 +1,84 @@ +# Testing + +How the test suite is organized, what it covers today, and what each build +phase is obligated to add. Core logic is validated against a **real +PostgreSQL** — no mocked-DB tests for core logic (see +[design-principles.md](design-principles.md)). + +## How to run + +| Command | What it does | +| --- | --- | +| `make test-unit` | Race-enabled unit tests, no Docker (`SKIP_INTEGRATION=1`). | +| `make test` | Full suite; integration tests start disposable PostgreSQL containers (testcontainers). `PG_VERSION` selects the major (default 16). | +| `make test-supported-postgres` | Full suite against every supported major, 14 → 18 — the local mirror of the CI matrix. | +| `make db-up` / `make test-db` / `make db-down` | Long-lived compose database on localhost; the suite connects to it via `PG_DSN` instead of starting per-test containers. Fastest loop for repeated integration runs. | + +The harness is [internal/testutil](../internal/testutil/postgres.go): +`StartPostgres` returns a connection URL (container, or `PG_DSN` when set) +and `NewSchema` gives each test a throwaway schema so parallel tests never +collide — which also means every integration test runs against a +**non-`public` schema**, an axis some peer tools (pgroll) treat as a separate +matrix dimension. `StartPostgresTLS` starts a TLS-only server with a +generated CA for verify-full tests. The harness has its own tests proving +the version selected by `PG_VERSION` is the version actually running, and +that throwaway schemas are isolated. + +## Version matrix vs real Aurora + +CI runs the matrix against **vanilla PostgreSQL 14 → 18 images** — the floor +promised in [postgresql-version-support.md](postgresql-version-support.md) is +enforced by CI, not just documented. Vanilla PostgreSQL is *not* Aurora: +storage internals, replication, and failover behavior differ, and some +Aurora-specific behavior (e.g. `rds.logical_replication`, failover slot +loss) cannot be exercised in public CI. Validation against real Aurora +engine versions is a separate, environment-specific gate that lives outside +this repository's CI. + +## Current coverage (Phase 0) + +| Area | Tests | +| --- | --- | +| CLI grammar / config | [internal/cli](../internal/cli/cli_test.go) | +| Pool config, bounded session timeouts | [pkg/dbconn](../pkg/dbconn/pool_config_test.go), [integration](../pkg/dbconn/dbconn_integration_test.go) | +| Retry classification and behavior | [pkg/dbconn/retry_test.go](../pkg/dbconn/retry_test.go) | +| RDS/Aurora TLS (unit) | [pkg/dbconn/rds_test.go](../pkg/dbconn/rds_test.go) | +| Verify-full TLS against a live TLS-only server | [pkg/dbconn/tls_integration_test.go](../pkg/dbconn/tls_integration_test.go) | +| Targeted blocker termination | [pkg/dbconn/dbconn_integration_test.go](../pkg/dbconn/dbconn_integration_test.go) | +| Test harness self-checks | [internal/testutil](../internal/testutil/postgres_test.go) | + +## Deferred test obligations (Phases 1–3) + +These are owed when the corresponding implementation lands — they are not +written speculatively against unimplemented behavior. The authoritative +per-phase test lists live in the build plan; the invariant registry +([invariants.md](invariants.md)) carries the per-invariant enforcement +points. + +| Phase | Test obligations (summary) | +| --- | --- | +| 1 — statement/classifier | Parse-based classification per DDL form; refusal (`not-native-safe`) contract; every classification decision tested against the reference table in [postgres-online-ddl-reference.md](postgres-online-ddl-reference.md). | +| 2 — native executor | Each native idiom (`CONCURRENTLY`, `NOT VALID` + `VALIDATE`, fast default, `USING INDEX`) exercised against all supported majors; bounded lock behavior under contention; invalid-index cleanup. | +| 3 — declarative diff | Desired-state → `ALTER` derivation correctness; diff idempotency (no-op on converged schema); refusal propagation through the diff path. | + +Copy-and-swap (shadow table, CDC, checksum gate, cutover, resume) is +Phases 4–7 and carries its own obligations, including checksum-gate and +checkpoint/resume fault-injection tests. + +## Topology obligations from peer-tool CIs + +A survey of the CI setups of pgroll, Reshape, pg-osc, pg_repack, +pg-schema-diff, pg-delta, migra, Atlas, SchemaHero, and Bytebase found no +peer testing physical replicas, poolers as live intermediaries, failover, or +cloud-managed PostgreSQL — those remain environment-gate territory (see +above). The patterns worth carrying, tied to the phase whose implementation +makes them meaningful: + +| Pattern (peer precedent) | Where it lands here | +| --- | --- | +| TLS-required server, verify-full + untrusted-CA rejection (pg-delta) | **Done** — `StartPostgresTLS` + [tls_integration_test.go](../pkg/dbconn/tls_integration_test.go). | +| Non-`public` schema placement (pgroll matrix dimension) | Structural — every test already runs in a throwaway non-`public` schema; Phase 1 classifier tests must keep qualifying objects. | +| Partitioned tables (pg_repack regression, pg-schema-diff acceptance) | Phases 1–2 — classifier and native-executor cases for partitioned parents/partitions (`DETACH PARTITION CONCURRENTLY` is PG 14+). | +| Tablespaces, including quoted names (pg_repack) | Phases 4–7 — shadow-table placement must preserve tablespace. | +| `wal_level=logical` server + publication interaction (pg_repack, pg-delta) | Phases 4–7 — CDC tests run against logical-decoding-enabled servers; add `wal_level=logical` to the harness/compose when Phase 4 starts. | +| Pinned minor versions in the matrix (pgroll, SchemaHero) vs floating major tags | Deliberate choice: we track floating major tags (`postgres:14` … `postgres:18`) so CI follows each major's latest minor automatically. Revisit if a minor-specific regression ever matters. | diff --git a/internal/testutil/postgres.go b/internal/testutil/postgres.go index 8742f39..1535f61 100644 --- a/internal/testutil/postgres.go +++ b/internal/testutil/postgres.go @@ -29,17 +29,24 @@ func PGVersion() string { return DefaultPGVersion } -// StartPostgres starts a disposable PostgreSQL container for the test and -// returns its connection URL. The container is terminated when the test ends. -// Set SKIP_INTEGRATION=1 to skip tests that need Docker. +// StartPostgres returns a PostgreSQL connection URL for the test. +// +// By default it starts a disposable container (terminated when the test +// ends). When PG_DSN is set, that external server is used instead and no +// container is started — the compose/ workflow and CI variants that run a +// long-lived server use this. Set SKIP_INTEGRATION=1 to skip tests that need +// a database entirely. func StartPostgres(t *testing.T) string { t.Helper() if os.Getenv("SKIP_INTEGRATION") != "" { - t.Skip("SKIP_INTEGRATION set; skipping test that needs Docker") + t.Skip("SKIP_INTEGRATION set; skipping test that needs a database") } - // The container must outlive t.Context (which is cancelled before - // cleanups run), so use Background and terminate via t.Cleanup. - ctx := context.Background() + if dsn := os.Getenv("PG_DSN"); dsn != "" { + return dsn + } + // t.Context only governs the start request; the running container is + // not tied to it and is terminated via t.Cleanup below. + ctx := t.Context() ctr, err := tcpostgres.Run(ctx, "postgres:"+PGVersion(), tcpostgres.BasicWaitStrategies()) require.NoError(t, err, "start postgres container") t.Cleanup(func() { @@ -63,8 +70,8 @@ func NewSchema(t *testing.T, pool *pgxpool.Pool) string { _, err := pool.Exec(t.Context(), fmt.Sprintf("CREATE SCHEMA %s", name)) require.NoError(t, err, "create throwaway schema") t.Cleanup(func() { - // t.Context is done by cleanup time; use a fresh context. - _, err := pool.Exec(context.Background(), fmt.Sprintf("DROP SCHEMA IF EXISTS %s CASCADE", name)) + // t.Context is cancelled by cleanup time; strip the cancellation. + _, err := pool.Exec(context.WithoutCancel(t.Context()), fmt.Sprintf("DROP SCHEMA IF EXISTS %s CASCADE", name)) if err != nil { t.Logf("drop throwaway schema %s: %v", name, err) } diff --git a/internal/testutil/postgres_test.go b/internal/testutil/postgres_test.go new file mode 100644 index 0000000..7cf29d9 --- /dev/null +++ b/internal/testutil/postgres_test.go @@ -0,0 +1,59 @@ +package testutil_test + +import ( + "os" + "strings" + "testing" + + "github.com/jackc/pgx/v5/pgxpool" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/block/pg-sprite/internal/testutil" +) + +// TestServerMajorMatchesRequestedVersion proves the harness tests what it +// claims: the server the suite connects to actually runs the PG_VERSION +// major the CI matrix selected. Without this, a matrix entry that silently +// fell back to a default image would still pass every test. +func TestServerMajorMatchesRequestedVersion(t *testing.T) { + if os.Getenv("PG_DSN") != "" { + t.Skip("PG_DSN points at an external server; version is not harness-selected") + } + url := testutil.StartPostgres(t) + + pool, err := pgxpool.New(t.Context(), url) + require.NoError(t, err) + t.Cleanup(pool.Close) + + var major string + require.NoError(t, pool.QueryRow(t.Context(), + "SELECT (current_setting('server_version_num')::int / 10000)::text").Scan(&major)) + assert.Equal(t, testutil.PGVersion(), major, + "server major must match the requested PG_VERSION") +} + +// TestNewSchemaIsolation proves the per-test schema isolation the whole +// suite relies on: two schemas from NewSchema never collide, and objects +// created in one are invisible to the other. +func TestNewSchemaIsolation(t *testing.T) { + url := testutil.StartPostgres(t) + + pool, err := pgxpool.New(t.Context(), url) + require.NoError(t, err) + t.Cleanup(pool.Close) + + s1 := testutil.NewSchema(t, pool) + s2 := testutil.NewSchema(t, pool) + require.NotEqual(t, s1, s2) + require.True(t, strings.HasPrefix(s1, "t_")) + + _, err = pool.Exec(t.Context(), "CREATE TABLE "+s1+".only_here (id int)") + require.NoError(t, err) + + var exists bool + require.NoError(t, pool.QueryRow(t.Context(), + "SELECT EXISTS (SELECT 1 FROM information_schema.tables WHERE table_schema = $1 AND table_name = 'only_here')", + s2).Scan(&exists)) + assert.False(t, exists, "object in one throwaway schema must not appear in another") +} diff --git a/internal/testutil/postgres_tls.go b/internal/testutil/postgres_tls.go new file mode 100644 index 0000000..3890f76 --- /dev/null +++ b/internal/testutil/postgres_tls.go @@ -0,0 +1,174 @@ +package testutil + +import ( + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "math/big" + "net" + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/require" + "github.com/testcontainers/testcontainers-go" + tcpostgres "github.com/testcontainers/testcontainers-go/modules/postgres" +) + +// TLSPostgres describes a TLS-only PostgreSQL started by StartPostgresTLS. +type TLSPostgres struct { + // URL is the connection URL without an sslmode parameter, so the + // caller's TLS configuration decides the handshake. + URL string + // CACertPath is the PEM CA certificate that signed the server + // certificate — the trust anchor for verify-full connections. + CACertPath string + // UntrustedCACertPath is a valid CA certificate that did NOT sign the + // server certificate, for negative verification tests. + UntrustedCACertPath string +} + +// tlsInitScript runs as the postgres user during initdb: it installs the +// server certificate and restricts pg_hba to TLS-only TCP connections, so +// every network connection in the test must complete a TLS handshake. +const tlsInitScript = `#!/bin/sh +set -e +cp /tls/server.crt /tls/server.key "$PGDATA"/ +chmod 0600 "$PGDATA"/server.key +cat >> "$PGDATA"/postgresql.conf < "$PGDATA"/pg_hba.conf </dev/null; then + echo "Auto-fixed: $file" + git add "$file" + fi + done +} + +# Use local golangci-lint if available, otherwise Docker. +# Check common Go binary paths since git hooks may not inherit the full user PATH. +LINT_CMD="" +for candidate in golangci-lint "$HOME/go/bin/golangci-lint" "$GOPATH/bin/golangci-lint" "$GOBIN/golangci-lint"; do + if command -v "$candidate" >/dev/null 2>&1; then + LINT_CMD="$candidate" + break + fi +done +if [ -z "$LINT_CMD" ]; then + LINT_CMD="docker run --rm -v $(pwd):/app -w /app golangci/golangci-lint:latest golangci-lint" +fi + +# Detect the merge-base so we only flag issues introduced by this branch. +# If merge-base equals HEAD (e.g., after git reset --soft for squashing), +# skip --new-from-rev to avoid treating every changed line as "new". +NEW_FROM_REV="" +for base_branch in origin/main origin/master; do + if git rev-parse --verify "$base_branch" >/dev/null 2>&1; then + MERGE_BASE=$(git merge-base HEAD "$base_branch" 2>/dev/null || true) + if [ -n "$MERGE_BASE" ] && [ "$MERGE_BASE" != "$(git rev-parse HEAD)" ]; then + NEW_FROM_REV="$MERGE_BASE" + fi + break + fi +done + +new_flag="" +if [ -n "$NEW_FROM_REV" ]; then + new_flag="--new-from-rev=$NEW_FROM_REV" +fi + +# Lint the packages containing staged files: auto-fix, re-stage, then verify. +PACKAGES=$(echo "$STAGED_GO_FILES" | xargs -n1 dirname | sort -u | sed 's|^|./|' | sed 's|$|/...|') + +echo "Running golangci-lint --fix..." +# shellcheck disable=SC2086 +$LINT_CMD run --fix --timeout=5m $PACKAGES || true + +restage_fixed_files + +# shellcheck disable=SC2086 +if ! $LINT_CMD run --timeout=5m $new_flag $PACKAGES; then + echo "" + echo "golangci-lint found issues that cannot be auto-fixed." + echo "Please fix them manually before committing." + exit 1 +fi + +echo "All lint checks passed!" From e55146c456f08e29f1521279e7e7367130207a05 Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Tue, 4 Aug 2026 16:22:16 +1000 Subject: [PATCH 2/5] test/ci: PG 14-18 dev database, TLS tests, docs-only gating, hooks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Compose-based local database mirrors the CI matrix (PG_DSN path in the harness); docs-only PRs skip the heavy jobs behind a fail-open filter with a single ci-ok required status; pre-commit/pre-push hooks give fast local feedback and skip cleanly on Markdown-only changes. A TLS-only server harness proves the verify-full CACertPath path live (trusted CA encrypts, plaintext and untrusted CA fail closed) — the one peer-CI pattern (pg-delta) applicable to Phase 0. docs/testing.md records Phase 0 coverage, Phases 1-7 test obligations including the topology patterns surveyed from peer-tool CIs, and the vanilla PostgreSQL vs real-Aurora validation boundary. The lint config adopts SchemaBot's proven linter set plus gofmt/goimports formatters, with a staged-files lint-fix pre-commit flow ported from the same repo. AGENTS.md stays canonical for agent guidance: CLAUDE.md, GEMINI.md, .cursorrules, .goosehints, and copilot-instructions.md are symlinks to it, and review-agent checks live in .agents/checks/review.md. --- .agents/checks/review.md | 33 +++++++++++++++++++++++++++++++++ .cursorrules | 1 + .github/copilot-instructions.md | 1 + .goosehints | 1 + AGENTS.md | 4 ++++ CLAUDE.md | 1 + GEMINI.md | 1 + 7 files changed, 42 insertions(+) create mode 100644 .agents/checks/review.md create mode 120000 .cursorrules create mode 120000 .github/copilot-instructions.md create mode 120000 .goosehints create mode 120000 CLAUDE.md create mode 120000 GEMINI.md diff --git a/.agents/checks/review.md b/.agents/checks/review.md new file mode 100644 index 0000000..8a59c3e --- /dev/null +++ b/.agents/checks/review.md @@ -0,0 +1,33 @@ +# Review Checks + +Checks for AI review agents. The authoritative rules live in [SAFETY.md](../../SAFETY.md), +[AGENTS.md](../../AGENTS.md), and [docs/tcb-model.md](../../docs/tcb-model.md) — this file is +the reviewer's distillation. + +- Look up every touched `pkg/` package in the SAFETY.md partition table first — the review bar + differs between the safety-critical core and the periphery. Flag core changes with 🌶️ and + state the blast radius (data corruption, lost writes, wrong-table swap, stranded slot). +- Core packages: every loop, queue, retry, and wait must be bounded. An unbounded anything in + a core package is a review-blocking defect. +- Dangerous APIs accept proof types (`statement.Classified`, `PreflightedTable`, + `VerifiedShadow`, `CleanWatermark`, `TableLock`) with package-private constructors — never a + raw string or bool that a caller could fabricate. Core code re-verifies its own + preconditions; it never trusts that the planner or CLI checked. +- Invariant enforcement points carry a `// INV: ` comment matching + [docs/invariants.md](../../docs/invariants.md); violations use `ErrInvariantViolation` + naming the ID and abort fail-closed — never a warning, never retried. +- New dependencies inside a core package require a recorded decision (core dependency list: + `pgx/v5`, `pglogrepl`, stdlib). `github.com/block/spirit` must never be imported as a + module — ideas are ported with citations, not code. +- Connections go through `pkg/dbconn` (bounded `lock_timeout` / `statement_timeout`) — flag + raw `pgx` pools in production code. +- SQL parsing goes through `pg_query_go`; flag `strings.Split(";")` or any hand-parsing. A + parse failure is an error surfaced to the caller. +- Errors: wrapped with context and identifiers; no log-and-continue, no silent branch cases, + no discarded `Close()` errors, no `nolint`, no `--no-verify`. +- Tests: real PostgreSQL for core logic (no mocked-DB tests), testify, `t.Context()` + (cleanups use `context.WithoutCancel(t.Context())`), named polling deadlines — flag bare + `time.Sleep` readiness waits and any timeout increase that masks a flake instead of fixing + the root cause. +- CI coverage: behavior that varies by PostgreSQL major must be exercised across the + supported matrix (14–18), not just the default version. diff --git a/.cursorrules b/.cursorrules new file mode 120000 index 0000000..47dc3e3 --- /dev/null +++ b/.cursorrules @@ -0,0 +1 @@ +AGENTS.md \ No newline at end of file diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 120000 index 0000000..be77ac8 --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1 @@ +../AGENTS.md \ No newline at end of file diff --git a/.goosehints b/.goosehints new file mode 120000 index 0000000..47dc3e3 --- /dev/null +++ b/.goosehints @@ -0,0 +1 @@ +AGENTS.md \ No newline at end of file diff --git a/AGENTS.md b/AGENTS.md index 2f730d0..0fceb75 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -3,6 +3,10 @@ Guidance for AI coding agents working on pg-sprite — an online schema-change engine for Aurora PostgreSQL. Deliberately short: don't restate what you can infer from the code. +This file is canonical. `CLAUDE.md`, `GEMINI.md`, `.cursorrules`, `.goosehints`, and +`.github/copilot-instructions.md` are symlinks to it — edit only this file. Review-agent +checks live in [.agents/checks/review.md](.agents/checks/review.md). + ## Read SAFETY.md first This codebase is partitioned into a **safety-critical core** and a periphery. diff --git a/CLAUDE.md b/CLAUDE.md new file mode 120000 index 0000000..47dc3e3 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1 @@ +AGENTS.md \ No newline at end of file diff --git a/GEMINI.md b/GEMINI.md new file mode 120000 index 0000000..47dc3e3 --- /dev/null +++ b/GEMINI.md @@ -0,0 +1 @@ +AGENTS.md \ No newline at end of file From d89c87247edb12254afe4a989f10d7d30ef1079e Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Tue, 4 Aug 2026 16:41:59 +1000 Subject: [PATCH 3/5] docs: coverage invariant + TM-* test-methodology registry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make the testing bar explicit and binding: same-PR tests, regression-first bug fixes, real-DB core validation, full-matrix CI as a merge gate, and a coverage ratchet (numeric gate wired when Phase 1 lands core code). Add eight phase-tagged test-methodology invariants (TM-1..TM-8) mined from the pgroll / pg_repack / pg-delta suites: lifecycle fixtures, two-oracle SQL tests, real fault injection asserting durable state, an adversarial schema corpus, the convergence oracle for declarative diff, mutation-direction coverage, correctness-asserting benchmarks, and a compiled-binary e2e CI path — plus seeded generative DDL as the beyond-peers goal. --- AGENTS.md | 5 +++ docs/testing.md | 109 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 0fceb75..fcef23b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -27,6 +27,11 @@ make test-supported-postgres # full suite on every major 14 -> 18 make lint # golangci-lint ``` +- **Coverage invariant:** no behavior lands without a test that would fail without it; bug + fixes land with a regression test; the full suite (unit, integration, TLS, version matrix) + is a merge gate. Full statement: [docs/testing.md](docs/testing.md#the-coverage-invariant); + test-methodology rules (lifecycle fixtures, two-oracle SQL tests, real fault injection, + convergence oracle) are the `TM-*` registry in the same doc. - Always run the full `make test` when the scope of a change is unclear. - Never assume a test failure is unrelated to your change; investigate it. - Never increase timeouts to fix flakes; find the root cause. diff --git a/docs/testing.md b/docs/testing.md index 8fdb56b..184ed42 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -5,6 +5,115 @@ phase is obligated to add. Core logic is validated against a **real PostgreSQL** — no mocked-DB tests for core logic (see [design-principles.md](design-principles.md)). +## The coverage invariant + +The suite is part of the safety argument, not a formality. The standing rule, +binding on every merge: + +> **No behavior lands without a test that would fail without it. Core logic +> is proven against real PostgreSQL on every supported major. CI runs the +> whole suite — unit, integration, TLS, and the 14 → 18 version matrix — as +> a merge gate on every PR. Coverage only ratchets up.** + +Concretely: + +- **Same-PR tests.** New behavior and the tests proving it land in the same + PR — code never merges ahead of its tests, and an invariant from + [invariants.md](invariants.md) lands with the test named for it (see the + build-phase mapping there). +- **Regression-first bug fixes.** A bug fix lands with a test that reproduces + the bug and fails on the pre-fix code. +- **Real database, race-enabled.** Unit tests run with `-race`; core (`pkg/`) + logic is never validated against mocks — integration tests run against + real PostgreSQL, across every supported major in CI. +- **The matrix is a gate, not advisory.** The `ci-ok` sentinel requires the + full version matrix; docs-only changes are the only path that skips it. +- **Coverage never regresses.** Deleting or skipping a test to get green is + forbidden (same rule as the hooks: no `--no-verify`, no `nolint`). A + numeric coverage ratchet on `pkg/` packages is wired into CI once Phase 1 + lands the first core package — until then this clause is enforced in + review. + +## Test-methodology invariants (TM) + +How tests are *built*, mined from the peer suites (pgroll, pg_repack, +pg-delta — the topology survey below covers *what environments* they test; +this covers *how*). Each rule binds from the phase noted. + +### TM-1 — Lifecycle fixture, not happy-path tests + +Every executor/migration integration test drives the **full lifecycle +through one shared fixture** — start → assert → abort → assert → restart → +complete → assert — so interrupted-and-retried is the default tested path, +not a special case. Once checkpointing exists, kill → resume joins the +lifecycle. *Binds:* Phase 2 (native executor) onward. *Source:* pgroll +`ExecuteTests` (`pkg/migrations/op_common_test.go`). + +### TM-2 — Two oracles for safety-encoding SQL + +Generated SQL whose exact shape carries a safety property (chunk +continuation predicates, `ON CONFLICT` arbiters, timeout preludes, +fallback-mode trigger bodies) is **frozen by exact-string test AND proven +behaviorally against a real database** — never just one of the two. +*Binds:* Phase 2 onward. *Source:* pgroll trigger/backfill template tests; +pg-delta's snapshot + roundtrip pairing. + +### TM-3 — Fault injection is real, and asserts durable state + +Contention tests hold a real `ACCESS EXCLUSIVE` lock from a second +connection; cancellation tests use context deadlines. After any injected +failure the test asserts the **durable state** (no wedged migration record, +no leaked shadow objects/slots/triggers) and the ability to proceed — not +merely the returned error type. *Binds:* Phase 2 onward; full +phase-boundary kill/resume matrix at Phases 4–8. *Source:* pgroll's +lock-holder pattern — and pg_repack's absence of it, the gap peers left +that our copy-and-swap phases must fill. + +### TM-4 — The adversarial schema corpus only grows + +Integration fixtures include the shapes that break naive engines: quoted +and whitespace identifiers, dropped-column tuple layouts, TOASTed values, +expression/partial indexes, non-default reloptions, generated and identity +columns, partitioned parents, tablespaces (including quoted names). The +corpus is shared across phases and **never shrinks to make a phase land**. +*Binds:* Phase 1 onward. *Source:* pg_repack `regress/sql/repack-setup.sql`. + +### TM-5 — Convergence is the diff oracle + +Every declarative-diff test proves, against two real databases: the derived +plan applies cleanly; re-introspect + re-diff yields **empty**; a second +derivation emits nothing (idempotency). Comparison is **semantic catalog +state** (normalized), with SQL snapshots as the secondary oracle; failures +print the residual diff and the original plan. *Binds:* Phase 3. +*Source:* pg-delta `tests/integration/roundtrip.ts`. + +### TM-6 — Every mutation direction per property + +For each object property the diff handles: absent → present, present → +changed, present → absent — plus replacement where PostgreSQL cannot ALTER +in place. *Binds:* Phase 3. *Source:* pg-delta operation suites. + +### TM-7 — Benchmarks carry correctness assertions + +Performance tests (copy throughput at multiple row scales; fallback-mode +trigger write amplification) verify post-benchmark data correctness and tag +results with commit SHA + PG version. A fast wrong answer is a failure. +*Binds:* Phase 4 onward. *Source:* pgroll `internal/benchmarks`. + +### TM-8 — A compiled-binary e2e path exists in CI + +Separate from Go package tests, CI runs the **built `pg-sprite` binary** +against a real database with checked-in example inputs as the acceptance +corpus — exit codes, output, and resulting database state asserted. +*Binds:* Phase 2 (first executing command). *Source:* pgroll `make +examples` CI job; pg_repack driving its CLI through `pg_regress`. + +**Beyond the peers:** none of the three does generative testing. From +Phase 3 we add **seeded schema/DDL generation** (generate desired state → +plan → apply → re-diff must be empty), printing the seed on failure and +promoting failing seeds to fixed regression cases. This is deliberately a +capability no peer suite has. + ## How to run | Command | What it does | From 764f02e66e5d411ffe19565cf9e49801764c0329 Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Tue, 4 Aug 2026 16:51:44 +1000 Subject: [PATCH 4/5] Enforce mechanical style rules via lint; add judgment maxims to AGENTS.md Doc comments on exported symbols, no init(), no package-level mutable state, and no ctx-in-struct move to .golangci.yml (revive, gochecknoinits, gochecknoglobals, containedctx) so prose stays short. New maxims (panic policy, SQLSTATE error matching, goroutine lifetimes, injected clock, comment hygiene, OSS hygiene) ported from schemabot's AGENTS.md and Go community style canon, mirrored into the review checks. --- .agents/checks/review.md | 11 ++++++++++- .golangci.yml | 15 +++++++++++++++ AGENTS.md | 21 +++++++++++++++++++++ cmd/pg-sprite/main.go | 1 + 4 files changed, 47 insertions(+), 1 deletion(-) diff --git a/.agents/checks/review.md b/.agents/checks/review.md index 8a59c3e..cda08af 100644 --- a/.agents/checks/review.md +++ b/.agents/checks/review.md @@ -24,7 +24,16 @@ the reviewer's distillation. - SQL parsing goes through `pg_query_go`; flag `strings.Split(";")` or any hand-parsing. A parse failure is an error surfaced to the caller. - Errors: wrapped with context and identifiers; no log-and-continue, no silent branch cases, - no discarded `Close()` errors, no `nolint`, no `--no-verify`. + no discarded `Close()` errors, no `nolint`, no `--no-verify`. No panics in library code — + invariant violations return `ErrInvariantViolation` fail-closed. Postgres errors are matched + by SQLSTATE (`errors.As` → `*pgconn.PgError`, `.Code`), never by message text. +- Goroutines: every goroutine has an owner, a bounded lifetime, and a stop path — flag + fire-and-forget `go func()`. Core logic takes time from an injected clock — flag inline + `time.Now()`/`time.Sleep` in core packages. +- Comments describe *what* and *why*, never history — flag bug/PR references, + "previously X" notes, and counts or thresholds that will go stale. Log messages state what + *will* happen, not what *might*. No internal company details (cluster names, hostnames, + org names) in code, comments, commits, or PRs. - Tests: real PostgreSQL for core logic (no mocked-DB tests), testify, `t.Context()` (cleanups use `context.WithoutCancel(t.Context())`), named polling deadlines — flag bare `time.Sleep` readiness waits and any timeout increase that masks a flake instead of fixing diff --git a/.golangci.yml b/.golangci.yml index c4d6ddc..dc76b27 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -16,10 +16,20 @@ linters: - unparam # unused/constant function parameters and results - misspell # commonly misspelled words - nolintlint # ill-formed or unexplained nolint directives + - revive # doc comments on exported symbols (see settings) + - gochecknoinits # no init() functions + - gochecknoglobals # no package-level mutable state (error sentinels exempt) + - containedctx # no context.Context stored in struct fields settings: usetesting: context-background: true context-todo: true + revive: + # Only the doc-comment rules; everything else revive offers is either + # covered by other linters or a judgment call that lives in AGENTS.md. + rules: + - name: exported + - name: package-comments exclusions: rules: # Test helpers intentionally keep uniform signatures and fixed arguments @@ -27,6 +37,11 @@ linters: - path: _test\.go linters: - unparam + # Test files and test-only support code may use fixtures and shared + # state; the no-globals bar applies to production code. + - path: (_test\.go|^internal/testutil/) + linters: + - gochecknoglobals formatters: enable: diff --git a/AGENTS.md b/AGENTS.md index fcef23b..9a1c55e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -77,6 +77,27 @@ make lint # golangci-lint redundant safety closer on a handle someone else owns discards its guaranteed already-closed error). - State comparisons use typed constants and helpers, never raw string matching. +- **Never panic in library code.** Invariant violations return `ErrInvariantViolation` + fail-closed (see [SAFETY.md](SAFETY.md)); panics are reserved for provable programmer error + at startup. +- **Match Postgres errors by SQLSTATE** (`errors.As` → `*pgconn.PgError`, branch on `.Code`), + never by message text — error text varies by server version and locale. Sentinel/typed + errors are compared with `errors.Is`/`As` at boundaries. +- **Every goroutine has an owner, a bounded lifetime, and a stop path** — no fire-and-forget + `go func()`. +- **Core logic takes time from an injected clock**, not inline `time.Now()`/`time.Sleep` — + deterministic tests depend on it. +- Comments describe *what* and *why*, never history — no bug/PR references, no + "previously X" notes, no counts or thresholds that go stale; move comments with the code + they explain. +- Tests assert specific values, not just existence; no negative regression tests for removed + behavior. Log messages state what *will* happen, not what *might* ("will block", not + "may be blocked"). +- Never reference internal company details (cluster names, hostnames, org names) in code, + comments, commits, or PRs — this is a public repo. + +Mechanical style rules (doc comments on exported symbols, no `init()`, no package-level +mutable state, no `context.Context` in structs) are enforced by `.golangci.yml`, not prose. Design docs live in [docs/](docs/) — start at [docs/README.md](docs/README.md); the invariant registry is [docs/invariants.md](docs/invariants.md). diff --git a/cmd/pg-sprite/main.go b/cmd/pg-sprite/main.go index 462f3ba..0f1ad85 100644 --- a/cmd/pg-sprite/main.go +++ b/cmd/pg-sprite/main.go @@ -1,3 +1,4 @@ +// Command pg-sprite is an online schema-change engine for Aurora PostgreSQL. package main import ( From d4e45d439131ea51dffa7c0986ae1c9e6f435a5e Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Wed, 5 Aug 2026 19:20:41 +1000 Subject: [PATCH 5/5] ci: pin golangci-lint-action and lint binary version The action's default binary is built with an older Go than the module targets and cannot load the v2 config; SHA-pinning also satisfies the semgrep and zizmor unpinned-action checks. --- .github/workflows/ci.yml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4fc3def..0bbc1c7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -60,7 +60,11 @@ jobs: - uses: actions/setup-go@v5 with: go-version-file: go.mod - - uses: golangci/golangci-lint-action@v6 + # Pin the same golangci-lint major used locally (v2 config format); + # the action's default binary lags and cannot load a v2 config. + - uses: golangci/golangci-lint-action@ba0d7d2ec06a0ea1cb5fa41b2e4a3ab91d21278a # v9.3.0 + with: + version: v2.12.2 build: needs: changes