diff --git a/.github/scripts/pg_upgrade_cluster b/.github/scripts/pg_upgrade_cluster new file mode 100755 index 0000000..6ee59c0 --- /dev/null +++ b/.github/scripts/pg_upgrade_cluster @@ -0,0 +1,90 @@ +#!/usr/bin/env bash +# +# pg_upgrade_cluster - Shared binary pg_upgrade mechanics for CI. Modeled on +# Postgres-Extensions/cat_tools's script of the same name/purpose -- this +# part of the flow (recreating the pgxn-tools image's "test" cluster with +# matching initdb options, then driving pg_upgrade between two majors) has +# nothing extension-specific about it, so it lives here once instead of +# being inlined in the workflow. +# +# Lives under .github/, not bin/: unlike bin/test_existing (which a developer +# can run locally against a scratch database), this is CI-mechanics-only -- +# pg_ctlcluster/pg_createcluster/pg_upgrade against the pgxn-tools image's +# "test" cluster convention isn't a locally-runnable workflow. +# +# USAGE: .github/scripts/pg_upgrade_cluster [args] +# +# recreate-old PG_VERSION +# pg-start's default "test" cluster doesn't have data checksums +# enabled, but binary pg_upgrade requires the old and new clusters to +# have MATCHING checksum/auth settings (see $INITDB_OPTS below) -- +# stop, drop, and recreate the "test" cluster for PG_VERSION with them, +# then start it and wait for readiness. +# +# upgrade OLD_PG NEW_PG +# Stop the old cluster, create the new cluster's "test" cluster (same +# $INITDB_OPTS), binary pg_upgrade OLD_PG -> NEW_PG, then start the new +# cluster and wait for readiness. On pg_upgrade failure, dumps its logs +# (PG17+ writes them to $new_datadir/pg_upgrade_output.d/; older +# versions write to CWD -- both are searched) before failing. +# +# $INITDB_OPTS (env var, required): initdb options both clusters must share so +# pg_upgrade sees consistent settings on old and new (e.g. +# "--data-checksums --auth trust"). Deliberately left unquoted at each use +# site so its (space-separated) options word-split into separate arguments. +set -euo pipefail + +: "${INITDB_OPTS:?INITDB_OPTS must be set}" + +recreate_old() { + local pg=$1 + pg_ctlcluster "$pg" test stop + pg_dropcluster "$pg" test + # -p 5432: pg_createcluster assigns the next available port, which may not + # be 5432 after pg-start has claimed and released it. Force 5432 so + # subsequent psql/createdb calls connect without -p. + pg_createcluster -p 5432 "$pg" test -- $INITDB_OPTS + pg_ctlcluster "$pg" test start + pg_isready -t 30 +} + +upgrade() { + local old_pg=$1 new_pg=$2 + pg_ctlcluster "$old_pg" test stop + pg_createcluster -p 5432 "$new_pg" test -- $INITDB_OPTS + # PG17+ writes logs to $new_datadir/pg_upgrade_output.d/; older versions + # write to CWD. Search both on failure. + mkdir -p /tmp/pg_upgrade_logs + chown postgres:postgres /tmp/pg_upgrade_logs + su -c "cd /tmp/pg_upgrade_logs && /usr/lib/postgresql/$new_pg/bin/pg_upgrade \ + -b /usr/lib/postgresql/$old_pg/bin \ + -B /usr/lib/postgresql/$new_pg/bin \ + -d /var/lib/postgresql/$old_pg/test \ + -D /var/lib/postgresql/$new_pg/test \ + -o '-c config_file=/etc/postgresql/$old_pg/test/postgresql.conf' \ + -O '-c config_file=/etc/postgresql/$new_pg/test/postgresql.conf'" postgres \ + || { find /tmp/pg_upgrade_logs \ + "/var/lib/postgresql/$new_pg/test/pg_upgrade_output.d" \ + -name '*.log' 2>/dev/null | sort | xargs -r tail -n +1; exit 1; } + pg_ctlcluster "$new_pg" test start + pg_isready -t 30 +} + +usage() { + echo "usage: .github/scripts/pg_upgrade_cluster [args]" >&2 + echo " recreate-old PG_VERSION" >&2 + echo " upgrade OLD_PG NEW_PG" >&2 + exit 2 +} + +main() { + local cmd=${1:-} + shift || true + case "$cmd" in + recreate-old) recreate_old "$@" ;; + upgrade) upgrade "$@" ;; + *) usage ;; + esac +} + +main "$@" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a55553f..507bf1a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,5 +1,46 @@ +# Test strategy +# +# A test_factory install can be arrived at two ways, each of which can break +# differently, so each gets its own job below: +# +# test -- FRESH install: CREATE EXTENSION at the current +# version, on every supported PostgreSQL major. The +# baseline a brand-new user gets. +# pg-upgrade-test -- BINARY pg_upgrade: install the current version on an +# OLD major, binary-upgrade the cluster to a NEWER +# major, then run the suite against the migrated +# objects in "existing" mode (test/install/load.sql). +# Proves objects created on an old server still work +# read back on a new one -- fresh-install testing +# never exercises this at all. +# +# test_factory has shipped only one version (0.5.0), so there is no +# in-place `ALTER EXTENSION UPDATE` path to test yet (no extension-update-test +# job) and the pg_upgrade job needs no bridge step (it always installs the +# CURRENT version on the old cluster -- there's no older, pg_upgrade-unsafe +# version in the wild to carry forward). Both jobs derive their PostgreSQL +# major list from the single source of truth computed in the `changes` job +# below, so adding/dropping a supported major is a one-line edit there. name: CI -on: [push, pull_request] +on: + # Post-merge CI only; pull_request already covers every PR commit. Without + # this, a commit on a branch with an open PR fires the workflow TWICE for + # the identical SHA (once for push, once for pull_request/synchronize) -- + # double the compute, and a flake on one duplicate run shows a confusing + # red check right next to an identical green one for the same commit. + push: + branches: + - master + pull_request: +# A superseded push (e.g. a quick follow-up commit on the same PR) cancels +# any in-flight run for the same branch instead of letting the full, +# expensive matrix -- including pg-upgrade-test's binary pg_upgrade legs -- +# run to completion for a commit nobody cares about anymore. +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true +env: + PGUSER: postgres jobs: # Style linter (https://github.com/Postgres-Extensions/linter, vendored at # .vendor/linter). Deliberately checked out WITHOUT submodules -- `make @@ -15,14 +56,105 @@ jobs: runs-on: ubuntu-latest steps: - name: Check out the repo - uses: actions/checkout@v4 + uses: actions/checkout@v7 - name: Lint SQL run: make lint + # Cheap gate that lets the heavy jobs below skip themselves on commits that + # touch only docs, without a workflow-level `paths-ignore` (which would skip + # the *whole* workflow, including all-checks-passed, on a docs-only push -- + # leaving a required check stuck Pending forever). Also derives, from a + # single set of constants, the supported-PostgreSQL-major list every heavy + # job below consumes, so adding a major is a one-line edit here instead of + # N separate matrix edits. + changes: + name: 🔍 Detect changes & derive PG matrix + runs-on: ubuntu-latest + outputs: + docs_only: ${{ steps.diff.outputs.docs_only }} + supported_pg: ${{ steps.pg.outputs.supported_pg }} + steps: + - name: Check out the repo + uses: actions/checkout@v7 + with: + # Full history needed so BASE and HEAD below are both reachable + # for `git diff`. + fetch-depth: 0 + - name: Compute per-push changed files + id: diff + run: | + # Fail-safe is the literal first line: any early exit or error + # further down (a bad BASE/HEAD, a failed git diff) leaves this in + # place, so a broken check never silently skips real testing. + echo "docs_only=false" >> "$GITHUB_OUTPUT" + + if [ "${{ github.event_name }}" = "pull_request" ] && \ + [ "${{ github.event.action }}" = "synchronize" ] && \ + [ -n "${{ github.event.before }}" ]; then + # A push to an already-open PR: before/after give the true + # per-push diff, same as for a branch push. + BASE="${{ github.event.before }}" + HEAD="${{ github.event.after }}" + elif [ "${{ github.event_name }}" = "pull_request" ]; then + # First run for this PR (opened/reopened/etc, or synchronize + # without a usable before): fall back to the whole base...head + # diff. + BASE="${{ github.event.pull_request.base.sha }}" + HEAD="${{ github.event.pull_request.head.sha }}" + else + BASE="${{ github.event.before }}" + HEAD="${{ github.event.after }}" + fi + + echo "base=$BASE" + echo "head=$HEAD" + + # A missing HEAD, or an all-zeros BASE (e.g. a new branch's first + # push, where GitHub reports no prior commit), means we can't + # compute a real diff -- the fail-safe default above stands. + if [ -z "$HEAD" ] || [ -z "$BASE" ] || [[ "$BASE" =~ ^0+$ ]]; then + exit 0 + fi + + CHANGED=$(git diff --name-only "$BASE" "$HEAD" || echo __DIFF_FAILED__) + echo "changed files:" + echo "$CHANGED" + + if [ "$CHANGED" = "__DIFF_FAILED__" ] || [ -z "$CHANGED" ]; then + exit 0 + fi + + DOCS_ONLY=true + while IFS= read -r f; do + if ! [[ "$f" =~ \.(md|asc)$ ]]; then + DOCS_ONLY=false + break + fi + done <<< "$CHANGED" + + echo "docs_only=$DOCS_ONLY" >> "$GITHUB_OUTPUT" + - name: Derive the supported-PostgreSQL-major list + id: pg + run: | + # Single source of truth for the supported PostgreSQL majors: the + # `test` and `pg-upgrade-test` matrices below both derive their + # version lists from here, so they cannot silently drift onto + # different lists. To add or drop a major, edit only NEWEST/FLOOR. + NEWEST=18 + FLOOR=10 + supported=$(seq "$NEWEST" -1 "$FLOOR") + # Emit a JSON array from a list of ints, for matrix: to consume + # with fromJSON. + json() { printf '%s\n' "$@" | paste -sd, - | sed 's/^/[/; s/$/]/'; } + echo "supported_pg=$(json $supported)" >> "$GITHUB_OUTPUT" + test: + needs: [changes] + if: needs.changes.outputs.docs_only != 'true' strategy: matrix: - pg: [17, 16, 15, 14, 13, 12, 11, 10] + # Current-supported majors, from the single source in the changes job. + pg: ${{ fromJSON(needs.changes.outputs.supported_pg) }} name: 🐘 PostgreSQL ${{ matrix.pg }} runs-on: ubuntu-latest container: pgxn/pgxn-tools @@ -30,7 +162,7 @@ jobs: - name: Start PostgreSQL ${{ matrix.pg }} run: pg-start ${{ matrix.pg }} - name: Check out the repo - uses: actions/checkout@v4 + uses: actions/checkout@v7 # rsync first: pgxntool/run-test-build.sh needs it to sync # test/build/*.sql into test/build/sql/, and the CONTAINER IMAGE # doesn't ship rsync. @@ -46,3 +178,136 @@ jobs: # extension itself, so no separate `make install` step is needed here. - name: Test on PostgreSQL ${{ matrix.pg }} run: make verify-results + + # Proves test_factory survives a BINARY pg_upgrade (in-place catalog + # migration to a newer PostgreSQL major), not just a fresh CREATE EXTENSION. + # Each leg: install the CURRENT version on an old cluster, binary-pg_upgrade + # to a newer major, then run the suite against the REAL migrated objects in + # "existing" mode (test/install/load.sql asserts the version, plants, and + # proves the dependency guard -- see bin/test_existing and + # test/install/load.sql). No bridge step: unlike an extension with multiple + # shipped versions, test_factory has only ever shipped 0.5.0, so there is no + # older, pg_upgrade-unsafe install to carry forward -- every leg installs + # the current version directly on the old cluster. + # + # Two legs, not an every-major stepwise climb (deliberately out of scope -- + # see the PR description): the oldest-to-newest jump (widest catalog + # distance) and the newest-boundary jump (most likely to hit a *new* + # PostgreSQL major's catalog change first). test_factory has no views or + # functions touching catalog internals (no SELECT * over a system catalog), + # so the per-major-boundary risk a full stepwise climb protects against is + # low here. + pg-upgrade-test: + # Also needs `test`, not just `changes`: without this, a trivially-broken + # PR (fails the cheap fresh-install matrix) still burns the full, + # expensive binary pg_upgrade matrix for nothing. + needs: [changes, test] + if: needs.changes.outputs.docs_only != 'true' + strategy: + matrix: + include: + - old_pg: "10" + new_pg: "18" + - old_pg: "17" + new_pg: "18" + name: 🔄 Binary pg_upgrade ${{ matrix.old_pg }} → ${{ matrix.new_pg }} + runs-on: ubuntu-latest + container: pgxn/pgxn-tools + env: + # Both clusters must use the same initdb options so pg_upgrade sees + # consistent settings (checksums, auth) on old and new clusters. + INITDB_OPTS: --data-checksums --auth trust + steps: + - name: Start PostgreSQL ${{ matrix.old_pg }} + run: pg-start ${{ matrix.old_pg }} + - name: Check out the repo + uses: actions/checkout@v7 + - name: Recreate old cluster with data checksums enabled + run: .github/scripts/pg_upgrade_cluster recreate-old ${{ matrix.old_pg }} + - name: Install pgtap into the old cluster + # test_factory_pgtap requires pgtap (see prepare-old below). Unlike + # the fresh-install `test` job, which gets this for free because + # pg-start's default cluster already has it, this job recreates the + # cluster from scratch (previous step) with none of the fresh-install + # job's setup -- pgtap must be installed explicitly here too, for + # every PostgreSQL major this job touches, old and new alike. + # --pg_config is explicit (not relying on pg-start having already + # pointed the bare `pg_config` on PATH at this major) so this step's + # correctness doesn't depend on that PATH-switching behavior. + run: pgxn install pgtap --sudo --pg_config /usr/lib/postgresql/${{ matrix.old_pg }}/bin/pg_config + - name: Install test_factory into old cluster + run: make install + - name: Prepare the old cluster (install current version + tap schema) + # test_factory_pgtap needs pgtap installed into a dedicated "tap" + # schema BEFORE it installs (see bin/test_existing's prepare-old + # comment) -- a bare CREATE EXTENSION test_factory_pgtap CASCADE with + # no schema prep leaves pgtap wherever the ambient search_path + # resolves, and the suite's SET search_path = tap then can't find + # pgtap's functions. + run: bin/test_existing prepare-old test_factory_upgrade + - name: Install PostgreSQL ${{ matrix.new_pg }} + run: apt-get install -y postgresql-${{ matrix.new_pg }} postgresql-server-dev-${{ matrix.new_pg }} + - name: Install pgtap into the new cluster + # pg_upgrade validates that every extension installed in a database + # being upgraded is also available (control file + library) on the + # NEW cluster's PostgreSQL install -- without this, the upgrade + # itself fails, not just the post-upgrade suite run. --pg_config is + # required here (unlike the previous pgtap install step): this step + # runs before pg-start ever points at the new major, so the plain + # `pg_config` on PATH would still resolve to the OLD version's. + run: pgxn install pgtap --sudo --pg_config /usr/lib/postgresql/${{ matrix.new_pg }}/bin/pg_config + - name: Install test_factory into new cluster + # PG_CONFIG must be specified explicitly: at this point both old and + # new PostgreSQL are installed, and the default pg_config on PATH may + # not be the new version's. + run: make install PG_CONFIG=/usr/lib/postgresql/${{ matrix.new_pg }}/bin/pg_config + - name: Stop old cluster, binary pg_upgrade to PostgreSQL ${{ matrix.new_pg }}, start new cluster + run: .github/scripts/pg_upgrade_cluster upgrade ${{ matrix.old_pg }} ${{ matrix.new_pg }} + - name: Run the suite against the pg_upgraded database (existing mode) + # run-suite dynamically asserts the installed version (never + # hardcoded), then runs the suite via --use-existing (so pg_regress + # does not drop/recreate the database) and gates on verify-results -- + # test/install/load.sql's existing-mode branch plants and proves the + # dependency guard as part of this same invocation. + run: bin/test_existing run-suite test_factory_upgrade + + # A single stable check name for use as a required status check in branch + # protection rules (not configured by this PR -- that needs a repo admin). + # Matrix jobs produce check names like "🐘 PostgreSQL 14", which would all + # need to be listed individually and updated whenever the matrix changes. + # This job passes if every other job passed or was skipped (e.g. the heavy + # jobs gated off by the `changes` job on a docs-only push), and fails if any + # failed or were cancelled. + all-checks-passed: + needs: [changes, lint, test, pg-upgrade-test] + if: always() + runs-on: ubuntu-latest + steps: + - name: Check out the repo + uses: actions/checkout@v7 + - name: Verify all jobs are listed in needs + # Ensures this job won't silently ignore a newly-added job that was + # omitted from the needs list above. + run: | + DEFINED=$(python3 -c " + import yaml + with open('.github/workflows/ci.yml') as f: + w = yaml.safe_load(f) + print('\n'.join(sorted(j for j in w['jobs'] if j != 'all-checks-passed'))) + ") + NEEDED=$(echo '${{ toJson(needs) }}' | python3 -c " + import json, sys + print('\n'.join(sorted(json.load(sys.stdin)))) + ") + if [ "$DEFINED" != "$NEEDED" ]; then + echo "Some jobs are missing from all-checks-passed needs:" + diff <(echo "$DEFINED") <(echo "$NEEDED") + exit 1 + fi + - name: Check all jobs passed or were skipped + run: | + if [[ "${{ contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled') }}" == "true" ]]; then + echo "One or more jobs failed or were cancelled" + exit 1 + fi +# vi: expandtab ts=2 sw=2 diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index 6585d24..f86281f 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -6,11 +6,13 @@ name: Claude Code Review # `pull_request` version never worked for fork PRs. # # SECURITY: pull_request_target runs in the BASE repo with secrets and a -# write-capable token. The job is gated to PRs from the trusted `jnasbyupgrade` -# fork only — an arbitrary external fork can never trigger this secret-bearing -# job. The workflow file always comes from the base branch (master), so a PR -# cannot modify the reviewer that runs on it. We check out the PR head only for -# read context (persist-credentials: false) and never build or execute PR code. +# write-capable token. The job is gated to PRs authored by the trusted +# `jnasbyupgrade` account only — an arbitrary external actor can never +# trigger this secret-bearing job. The workflow file always comes from the +# base branch (master), so a PR cannot modify the reviewer that runs on it. +# This workflow never checks out the PR's own ref into the workspace (see the +# checkout step below) -- claude-code-action fetches and reads the PR's +# content itself, safely, and never builds or executes it. on: pull_request_target: types: [opened, synchronize, reopened, ready_for_review] @@ -21,17 +23,46 @@ concurrency: jobs: claude-review: - # Trusted fork only, and skip drafts (don't spend API/CI on unfinished PRs). - # To add more trusted owners, extend the head-owner check. + # !!! SECURITY-CRITICAL -- DO NOT REMOVE OR WEAKEN THE user.login CHECK + # BELOW !!! It is the ONLY thing standing between an arbitrary external + # actor's PR and this job's write-capable GITHUB_TOKEN and + # CLAUDE_CODE_OAUTH_TOKEN. Drop or loosen this check and anyone can trigger + # a job that runs with this repo's secrets. To trust an additional + # account, EXTEND this condition explicitly (e.g. `|| ... == + # 'other-trusted-account'`) -- never replace it with something broader + # (a wildcard, etc.). + # + # Checks PR AUTHOR (github.event.pull_request.user.login), not head repo + # owner: an earlier version of this check used head.repo.owner.login, + # which only works for fork-headed PRs -- for an upstream-branch-headed + # PR (e.g. one opened for `gh stack`, base and head both in this repo), + # head.repo.owner.login is always this repo's OWN org, never the actual + # author, so that check silently skipped review on every such PR + # regardless of who opened it (caught when review kept skipping on a + # whole PR stack that was legitimately jnasbyupgrade's own work). + # user.login is the PR's original author and can't be spoofed by PR + # content (unlike, say, a string embedded in the PR body or a commit + # message), so this check holds regardless of whether the PR head lives + # in this repo or an external fork -- it's the right question here + # anyway: we're trusting the PERSON asking for a review to run, not the + # repository their branch happens to live in. + # Skips drafts too (don't spend API/CI on unfinished PRs). if: >- github.event.pull_request.draft == false && - github.event.pull_request.head.repo.owner.login == 'jnasbyupgrade' + github.event.pull_request.user.login == 'jnasbyupgrade' runs-on: ubuntu-latest timeout-minutes: 60 permissions: contents: read pull-requests: write # post the review comments checks: read # read sibling check-runs for the cost gate + # No narrower scope exists for cache-write alone; without this, + # claude-code-action's own internal setup silently fails to save its + # Actions cache ("Cache reservation failed: cache write denied: token + # has no writable scopes") -- a warning, not a hard failure, so the job + # still completes and looks fine, just slower/uncached every run. Don't + # try to "tighten" this down to something narrower; it doesn't exist. + actions: write steps: # COST GATE: the paid Claude review is the last thing to run. Wait for the # PR head's OTHER check-runs to finish and only proceed if they are clean. @@ -74,15 +105,33 @@ jobs: echo "decision=$decision" >> "$GITHUB_OUTPUT" echo "gate decision: $decision" - - name: Check out PR head (read-only context) + - name: Check out base branch if: steps.gate.outputs.decision == 'run' + # Deliberately NO ref:/repository: override -- this checks out this + # repo's own base branch (master), not the PR's fork/ref. Checking + # out an untrusted PR ref into the workspace root before this action + # is exactly the anti-pattern anthropics/claude-code-action's own + # docs/security.md warns against; its "preferred" pattern is a plain + # checkout of the base ref, nothing more. claude-code-action fetches + # and reviews the PR's actual content itself, from its own internal + # logic (src/github/operations/branch.ts): for a fork PR it fetches + # origin's refs/pull//head -- a ref GitHub maintains on THIS repo + # for any PR, fork or not, so it never needs direct access to the + # fork's own remote at all. That's why this step must leave `origin` + # pointing at this repo (the default) rather than being redirected to + # the fork: an earlier version of this step did that, which broke the + # action's own internal fetch ("couldn't find remote ref + # pull//head") since that ref doesn't exist on the fork. # Intentionally tracks the major-version tag (not a pinned SHA) so # upstream fixes are picked up automatically. - uses: actions/checkout@v4 + uses: actions/checkout@v7 with: - repository: ${{ github.event.pull_request.head.repo.full_name }} - ref: ${{ github.event.pull_request.head.sha }} - fetch-depth: 1 + # This job's permissions include pull-requests: write, a real + # write-capable credential -- nothing here legitimately runs `git + # push` (review comments post via the API/claude-code-action, not + # git), so there's no reason to leave that credential sitting in + # .git/config for the rest of the job to misuse if anything later + # goes wrong. persist-credentials: false - name: Run Claude Code Review @@ -99,6 +148,16 @@ jobs: # marketplace repo's default branch (upstream anthropics/claude-code). plugin_marketplaces: 'https://github.com/anthropics/claude-code.git' plugins: 'code-review@claude-code-plugins' + # A bare prompt: (no @claude mention) runs claude-code-action in + # "agent mode", which decides which MCP servers to start by + # scanning --allowedTools inside claude_args -- it does NOT consult + # the invoked plugin's own allowed-tools frontmatter. Without this, + # mcp__github_inline_comment__create_inline_comment never starts + # (not "exists but blocked" -- genuinely absent), so the + # code-review plugin silently falls back to one consolidated PR + # comment instead of real per-line inline comments. No error, no + # warning -- every review just quietly uses the wrong output shape. + claude_args: '--allowedTools mcp__github_inline_comment__create_inline_comment' # --comment is required: without it, the code-review plugin only # prints its findings to the job log and never posts anything to # the PR (confirmed by capturing the hidden SDK transcript on a diff --git a/.github/workflows/claude.yml b/.github/workflows/claude.yml index 7d1656c..4fa242b 100644 --- a/.github/workflows/claude.yml +++ b/.github/workflows/claude.yml @@ -31,12 +31,19 @@ jobs: pull-requests: read issues: read id-token: write - actions: read # Required for Claude to read CI results on PRs + # write (not just read) despite the name: `write` includes read access + # to CI results, AND there's no narrower scope for cache-write alone -- + # without it, claude-code-action's own internal setup silently fails to + # save its Actions cache ("Cache reservation failed: cache write + # denied: token has no writable scopes"), a warning not a hard + # failure, so the job still completes and looks fine, just + # slower/uncached every run. Don't "tighten" this back down to `read`. + actions: write steps: - name: Checkout repository # Intentionally tracks the major-version tag (not a pinned SHA) so # upstream fixes are picked up automatically. - uses: actions/checkout@v4 + uses: actions/checkout@v7 with: fetch-depth: 1 persist-credentials: false diff --git a/CLAUDE.md b/CLAUDE.md index 9d060fc..5f2ec66 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -6,6 +6,10 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co After pushing to a branch with an open PR, monitor CI using `gh pr checks --watch` in a background subagent until all jobs pass or a failure is confirmed. Investigate and fix failures immediately rather than leaving them for the user to notice. +### PR title convention for CI-only PRs + +A PR whose diff is confined entirely to CI configuration should have a title starting with `CI: ` (capital, colon, space). "CI-only" means literally that -- every changed file lives under `.github/workflows/`, nothing else. A PR that's CI-*motivated* but also adds/changes a real file elsewhere (a new `bin/` script a workflow calls, a linter's Makefile wiring, a test fixture) is NOT CI-only under this reading, even though CI is the reason it exists -- don't stretch the prefix to cover those. Check with `gh pr view --json files --jq '.files[].path'` before applying it, don't guess from the title/description alone. + ## psql Script Conventions Any `\if`/`\elsif`/`\else`/`\endif` block spanning more than ~a dozen lines needs a short comment naming the block on each of its control statements (including `\endif`), so a reader scrolling past `\else`/`\endif` on their own can tell at a glance which `\if` they belong to without scrolling back up. Put the comment on its own line immediately above the control statement, NOT trailing on the same line -- unlike SQL statements, psql's `\if`/`\else`/`\endif` don't treat a trailing `--` as a comment to strip: `\if` parses the entire rest of the line as its boolean expression (so a trailing comment breaks parsing outright), and `\else`/`\endif` parse it as an "extra argument" that gets ignored but still prints a warning into the actual output. Example: diff --git a/Makefile b/Makefile index c8787b5..bb7b306 100644 --- a/Makefile +++ b/Makefile @@ -7,6 +7,14 @@ PGXNTOOL_ENABLE_TEST_BUILD = yes PGXNTOOL_ENABLE_TEST_INSTALL = yes +# Already pgxntool's own default, but pinned explicitly (same reasoning as +# ENABLE_TEST_INSTALL above): CI's `test`/`pg-upgrade-test` jobs gate on +# `make verify-results`, not a bare `make test` -- pgxntool marks +# installcheck .IGNORE, so a plain test run exits 0 even when +# regression.diffs is nonempty. Relying on a default silently protects that +# gate only until pgxntool's own default changes. +PGXNTOOL_ENABLE_VERIFY_RESULTS = yes + # ------------------------------------------------------------------------------ # TEST_LOAD_SOURCE: how test/install/load.sql gets the extension to its # target state (fresh/update/existing). See test/install/load.sql for what diff --git a/bin/test_existing b/bin/test_existing new file mode 100755 index 0000000..a4f8a74 --- /dev/null +++ b/bin/test_existing @@ -0,0 +1,116 @@ +#!/usr/bin/env bash +# +# Exercise the test_factory test suite against a REAL database whose +# extension was installed -- and, for the pg_upgrade CI job, binary +# pg_upgraded -- OUTSIDE the suite ("existing" mode; see +# test/install/load.sql and test/CLAUDE.md). Modeled on +# Postgres-Extensions/cat_tools's bin/test_existing (same purpose), scoped +# down: test_factory has shipped only one version, so there is no +# bridge/multi-origin update path to carry, and every subcommand below +# always targets the CURRENT build. +# +# USAGE: bin/test_existing [args] +# +# prepare-old DB +# Old-cluster prep for the pg_upgrade CI job: create DB, then install +# pgtap into a dedicated "tap" schema BEFORE test_factory_pgtap +# installs, then CREATE EXTENSION test_factory_pgtap CASCADE (which +# pulls in test_factory) at the current version -- there is no older +# version to bridge from. The tap-schema-first ordering matters: if +# pgtap lands wherever the ambient search_path resolves instead (e.g. +# a bare CREATE EXTENSION test_factory_pgtap CASCADE with no schema +# prep), test_factory_pgtap's own suite (SET search_path = tap) can't +# find pgtap's functions and every test errors with "function +# no_plan() does not exist" -- hit for real while testing the +# foundation this script builds on (PR #22). +# +# run-suite DB +# Assert the installed version matches the current build (never +# hardcoded -- derived from `make -s print-PGXNVERSION`, so a broken +# extraction can't silently compare "" to ""), then run the suite in +# existing mode via --use-existing (pg_regress must not drop/recreate +# DB) and gate on verify-results, not a bare `make test` (pgxntool +# marks installcheck .IGNORE, so a plain test run exits 0 even when +# regression.diffs is nonempty). +# +# No separate guard-planting subcommand: unlike cat_tools, test_factory's +# dependency guard (a view in schema test_factory_drop_guard depending on +# tf.tap(text,text)) is planted AND proved entirely inside +# test/install/load.sql's existing-mode branch, which run-suite's `make +# verify-results` invocation triggers as its own first regression step. This +# script doesn't need to plant or re-prove it separately -- it exists to +# protect the SUITE RUN itself (a stray non-CASCADE drop from a bug +# elsewhere in the suite) rather than to prove the guard survived +# pg_upgrade, since binary pg_upgrade either carries every user object over +# intact or fails outright; there's no partial-survival case here for a +# pre-upgrade guard to detect that a post-upgrade one wouldn't already +# catch. +set -euo pipefail + +# Run from the repository root regardless of the caller's cwd, same +# resolution cat_tools's bin/test_existing uses. +cd "$(dirname "$(readlink -f "$0")")/.." + +psql_do() { + local db=$1 + shift + psql -d "$db" -v ON_ERROR_STOP=1 "$@" +} + +current_version() { + make -s print-PGXNVERSION 2>/dev/null | sed -n 's/.*set to "\(.*\)"$/\1/p' +} + +installed_version() { + psql -d "$1" -tAc "SELECT extversion FROM pg_extension WHERE extname = 'test_factory'" +} + +# The empty-value guards matter -- "" != "" is false, so a broken extraction +# on either side must not silently pass. +assert_version() { + local db=$1 expected installed + expected=$(current_version) + installed=$(installed_version "$db") + echo "version check '$db': installed='$installed' expected='$expected'" + if [ -z "$installed" ] || [ -z "$expected" ] || [ "$installed" != "$expected" ]; then + echo "FAIL: test_factory in '$db' is '$installed', expected '$expected'" >&2 + exit 1 + fi +} + +prepare_old() { + local db=$1 + createdb "$db" + psql_do "$db" -c "CREATE SCHEMA tap; CREATE EXTENSION pgtap SCHEMA tap; CREATE EXTENSION test_factory_pgtap CASCADE;" +} + +run_suite() { + local db=$1 + assert_version "$db" + local existing_args="TEST_LOAD_SOURCE=existing CONTRIB_TESTDB=$db EXTRA_REGRESS_OPTS=--use-existing PGXNTOOL_ENABLE_TEST_BUILD=no" + # verify-results already depends on `test` (pgxntool's base.mk), so a + # separate `make test` call first would just re-run the entire regression + # suite a second time against the real migrated database for nothing -- + # every pg-upgrade-test CI leg was paying for two full + # `pg_regress --use-existing` runs instead of one. + make verify-results $existing_args +} + +usage() { + echo "usage: bin/test_existing [args]" >&2 + echo " prepare-old DB" >&2 + echo " run-suite DB" >&2 + exit 2 +} + +main() { + local cmd=${1:-} + shift || true + case "$cmd" in + prepare-old) prepare_old "$@" ;; + run-suite) run_suite "$@" ;; + *) usage ;; + esac +} + +main "$@" diff --git a/test/expected/base.out b/test/expected/base.out index 75a9c3a..7af44d8 100644 --- a/test/expected/base.out +++ b/test/expected/base.out @@ -3,11 +3,11 @@ ok 1 - Register test customers ok 2 - Create function customer__add ok 3 - Register test invoices ok 4 - Ensure original_role temp table was dropped -ok 5 - Security definer function _tf.schema__getsert has search_path=pg_catalog -ok 6 - Security definer function _tf.test_factory__get has search_path=pg_catalog -ok 7 - Security definer function _tf.test_factory__set has search_path=pg_catalog -ok 8 - Security definer function _tf.table_create has search_path=pg_catalog -ok 9 - Security definer function _tf.get has search_path=pg_catalog +ok 5 - Security definer function _tf.get has search_path=pg_catalog +ok 6 - Security definer function _tf.schema__getsert has search_path=pg_catalog +ok 7 - Security definer function _tf.table_create has search_path=pg_catalog +ok 8 - Security definer function _tf.test_factory__get has search_path=pg_catalog +ok 9 - Security definer function _tf.test_factory__set has search_path=pg_catalog ok 10 - customer table is empty ok 11 - invoice table is empty ok 12 - invoice factory output diff --git a/test/expected/pgtap.out b/test/expected/pgtap.out index 2e529f2..1793e6e 100644 --- a/test/expected/pgtap.out +++ b/test/expected/pgtap.out @@ -4,11 +4,11 @@ ok 2 - Register test customers ok 3 - Create function customer__add ok 4 - Register test invoices ok 5 - Ensure original_role temp table was dropped -ok 6 - Security definer function _tf.schema__getsert has search_path=pg_catalog -ok 7 - Security definer function _tf.test_factory__get has search_path=pg_catalog -ok 8 - Security definer function _tf.test_factory__set has search_path=pg_catalog -ok 9 - Security definer function _tf.table_create has search_path=pg_catalog -ok 10 - Security definer function _tf.get has search_path=pg_catalog +ok 6 - Security definer function _tf.get has search_path=pg_catalog +ok 7 - Security definer function _tf.schema__getsert has search_path=pg_catalog +ok 8 - Security definer function _tf.table_create has search_path=pg_catalog +ok 9 - Security definer function _tf.test_factory__get has search_path=pg_catalog +ok 10 - Security definer function _tf.test_factory__set has search_path=pg_catalog ok 11 - Get test data set "base" for table invoice ok 12 - Get test data set "base" for table invoice ok 13 - Ensure we get sane error for a non-existent table diff --git a/test/helpers/create.sql b/test/helpers/create.sql index 8ff7b7f..775a299 100644 --- a/test/helpers/create.sql +++ b/test/helpers/create.sql @@ -90,6 +90,15 @@ SELECT hasnt_table( * anymore now that this file no longer runs it itself. */ +/* + * ORDER BY is load-bearing, not cosmetic: with no ordering this scans + * pg_proc in physical order, which happens to match creation order on a + * fresh CREATE EXTENSION but is NOT preserved by pg_upgrade (its + * dump/restore reconstructs pg_proc in a different, e.g. name-sorted, + * order) -- discovered because the pg_upgrade CI leg's "existing" run + * produced a row-reordered (but otherwise identical) diff against this + * same query's fresh-install expected output. + */ SELECT cmp_ok( proconfig , '@>' @@ -100,6 +109,7 @@ SELECT cmp_ok( JOIN pg_namespace n ON n.oid = pronamespace WHERE n.nspname IN ( 'tf', '_tf' ) AND p.prosecdef + ORDER BY p.oid::regproc::text ;