From 5df38054d2c4bd373cd2058cbe9a47dc53fbf38e Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Tue, 21 Jul 2026 17:48:09 -0500 Subject: [PATCH 01/13] ci: add a dedicated pg_tle test job There was no CI coverage for cat_tools' pgxntool-generated pg_tle (Trusted Language Extensions) deployment path, so a regression there (e.g. in the generated registration SQL) could ship unnoticed. This adds a pg-tle-test job that builds pg_tle from source, registers cat_tools with it via `make run-pgtle`, and verifies the extension installs and works correctly with no filesystem control file present -- i.e. purely through pg_tle's database-backed registration. The job always runs on its own freshly-started cluster, never shared with the other jobs in this workflow: pg_tle requires shared_preload_libraries and mixing pg_tle/non-pg_tle extension installs on one cluster can misbehave. The matrix covers PG 12-18, matching pg_tle 1.5.2's supported range per pgxntool/pgtle_versions.md (PG10/11 predate pg_tle support). This intentionally stops at a smoke test rather than running the full pg_regress/pgTap suite through pg_tle: pg_regress always creates its test database from the pristine template0, so any pre-registration done in another database (e.g. postgres or template1) is invisible to it. Routing the full suite through pg_tle would require adding registration SQL into test/install/*.sql, which runs inside every job's pg_regress invocation (not just this one) and would need conditional logic to avoid affecting the other, non-pg_tle jobs -- more invasive than this task calls for. --- .github/workflows/ci.yml | 65 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 64 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 336f866..2c30cc4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -548,6 +548,69 @@ jobs: if: matrix.pg != '10' run: bin/test_existing update-scenario cat_tools_update 0.2.2 + pg-tle-test: + needs: [changes] + if: needs.changes.outputs.docs_only != 'true' + strategy: + matrix: + # Current-supported majors, from the single source in the changes job + # (see its "Derive ..." step) -- same list the `test` job uses. This + # currently happens to line up with pg_tle 1.5.2's own supported range + # (12-18, see pgxntool/pgtle_versions.md) too, but that's a coincidence: + # if either range moves independently in the future, re-check they + # still overlap before assuming this job covers what it claims to. + # 1.5.2 itself is what AWS RDS for PostgreSQL 17 currently ships + # (RDS PG17.7+, our primary deployment target -- see + # https://docs.aws.amazon.com/AmazonRDS/latest/PostgreSQLReleaseNotes/postgresql-versions.html). + pg: ${{ fromJSON(needs.changes.outputs.supported_pg) }} + name: 🧩 pg_tle ${{ matrix.pg }} + runs-on: ubuntu-latest + container: pgxn/pgxn-tools + env: + PGTLE_VERSION: "1.5.2" + steps: + # A dedicated cluster, never shared with the other jobs in this + # workflow: pg_tle requires shared_preload_libraries and mixing + # pg_tle/non-pg_tle extension installs on one cluster can misbehave. + - name: Start PostgreSQL ${{ matrix.pg }} + run: pg-start ${{ matrix.pg }} + - name: Check out the repo + uses: actions/checkout@v6 + - name: Install rsync + run: apt-get install -y rsync + - name: Build and install pg_tle ${{ env.PGTLE_VERSION }} + # flex/bison/libkrb5-dev aren't in the pgxn-tools image; pg_tle's build + # needs them (guc-file.l, and clientauth.c includes gssapi.h). + run: | + apt-get install -y flex bison libkrb5-dev + git clone --branch v${{ env.PGTLE_VERSION }} --depth 1 https://github.com/aws/pg_tle.git /tmp/pg_tle + make -C /tmp/pg_tle install + - name: Enable pg_tle and restart PostgreSQL ${{ matrix.pg }} + run: | + echo "shared_preload_libraries = 'pg_tle'" >> /etc/postgresql/${{ matrix.pg }}/test/postgresql.conf + pg_ctlcluster ${{ matrix.pg }} test restart + pg_isready -t 30 + - name: Create pg_tle extension + run: psql -c "CREATE EXTENSION pg_tle" + - name: Register cat_tools with pg_tle + run: make run-pgtle + - name: Install cat_tools purely via pg_tle (no filesystem install) + # cat_tools is never `make install`ed in this job, so a successful + # CREATE EXTENSION here can only be resolving through pg_tle's + # registration, not a control file on disk. + run: | + test ! -e /usr/share/postgresql/${{ matrix.pg }}/extension/cat_tools.control + psql -c "CREATE EXTENSION cat_tools" + - name: Verify cat_tools works when deployed via pg_tle + run: | + INSTALLED=$(psql -tAc "SELECT extversion FROM pg_extension WHERE extname = 'cat_tools'") + EXPECTED=$(make -s print-PGXNVERSION 2>/dev/null | sed -n 's/.*set to "\(.*\)"$/\1/p') + echo "installed=$INSTALLED expected=$EXPECTED" + if [ -z "$INSTALLED" ] || [ -z "$EXPECTED" ] || [ "$INSTALLED" != "$EXPECTED" ]; then + echo "FAIL: installed='$INSTALLED' expected='$EXPECTED'"; exit 1 + fi + psql -v ON_ERROR_STOP=1 -c "SELECT relname FROM cat_tools.pg_class_v LIMIT 1" > /dev/null + # A single stable check name for use as a required status check in branch # protection rules. Matrix jobs produce check names like "🐘 PostgreSQL 14" # which would all need to be listed individually and updated whenever the @@ -555,7 +618,7 @@ jobs: # 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, test, pg-upgrade-test, pg-upgrade-stepwise, extension-update-test] + needs: [changes, test, pg-upgrade-test, pg-upgrade-stepwise, extension-update-test, pg-tle-test] if: always() runs-on: ubuntu-latest steps: From 988ab02c1fb7989c34b61495d80fd4b077f7d5e6 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Fri, 24 Jul 2026 15:19:32 -0500 Subject: [PATCH 02/13] ci: guard pg-tle-test against filesystem control-file contamination pg_tle silently loses to a filesystem control file of the same name -- confirmed hitting this directly while researching pg_tle version compatibility, where a stale filesystem cat_tools install shadowed the pg_tle-registered one under test and would have produced a false pass with no error raised. Add two layers of defense: a comprehensive check that diffs the extension directory's *.control files against a pre-pg_tle baseline (catching any extension that lands on disk instead of being registered via pg_tle, not just cat_tools, without hardcoding contrib extension names), and an explicit cat_tools-specific check that always runs regardless, in case the comprehensive check's exclude-list logic has a bug. --- .github/workflows/ci.yml | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2c30cc4..bbaf068 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -574,6 +574,12 @@ jobs: # pg_tle/non-pg_tle extension installs on one cluster can misbehave. - name: Start PostgreSQL ${{ matrix.pg }} run: pg-start ${{ matrix.pg }} + - name: Snapshot filesystem extension control files (pre-pg_tle baseline) + # Whatever ships on disk by default (e.g. contrib) before we install + # anything. The later comprehensive check diffs against this, so it + # flags ANY extension that lands on disk instead of being registered + # via pg_tle -- not just cat_tools -- without hardcoding contrib names. + run: find /usr/share/postgresql/${{ matrix.pg }}/extension -maxdepth 1 -name '*.control' | sort > /tmp/control_baseline.txt - name: Check out the repo uses: actions/checkout@v6 - name: Install rsync @@ -594,10 +600,30 @@ jobs: run: psql -c "CREATE EXTENSION pg_tle" - name: Register cat_tools with pg_tle run: make run-pgtle + - name: Verify no stray extension control files landed on the filesystem + # CRITICAL, and intentionally redundant with the cat_tools-specific + # check in the next step: a filesystem control file silently wins + # over a pg_tle-registered extension of the same name, which would + # make this whole job a false pass without ever raising an error. + # Confirmed hitting exactly this while researching pg_tle version + # compatibility (a stale filesystem cat_tools install shadowed the + # pg_tle-registered one under test). pg_tle.control itself is the + # only expected new file since the baseline snapshot. + run: | + find /usr/share/postgresql/${{ matrix.pg }}/extension -maxdepth 1 -name '*.control' | sort > /tmp/control_after.txt + NEW=$(comm -13 /tmp/control_baseline.txt /tmp/control_after.txt | grep -vx '.*/pg_tle\.control' || true) + if [ -n "$NEW" ]; then + echo "FAIL: unexpected extension control file(s) on disk (everything but pg_tle must be registered via pg_tle, not filesystem-installed):" + echo "$NEW" + exit 1 + fi - name: Install cat_tools purely via pg_tle (no filesystem install) # cat_tools is never `make install`ed in this job, so a successful # CREATE EXTENSION here can only be resolving through pg_tle's - # registration, not a control file on disk. + # registration, not a control file on disk. Checked explicitly here + # too (not just via the comprehensive check above) as a guard + # specifically for the extension under test, in case that check's + # exclude-list logic has a bug. run: | test ! -e /usr/share/postgresql/${{ matrix.pg }}/extension/cat_tools.control psql -c "CREATE EXTENSION cat_tools" From 03bb6fe20bea0c31ff117ee22d83051cbea29581 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Fri, 24 Jul 2026 16:45:45 -0500 Subject: [PATCH 03/13] ci: test the pg_tle update path with zero filesystem trace, end to end extension-update-test only proves the update path works via a filesystem install; there was no coverage for updating a pg_tle-deployed cat_tools. Reusing bin/test_existing's update_scenario unmodified for this hit a real bug: its run_suite() calls `make test`, which unconditionally depends on `install` (pgxntool's TEST_DEPS), and `make verify-results` in turn depends on `test` -- so the moment a pg_tle-mode run reached the suite, cat_tools would get filesystem-installed as a side effect, defeating the entire point of proving nothing ever touches disk. Fixed without touching pgxntool: a new TEST_EXISTING_DEPLOY=pgtle mode in run_suite() calls `testdeps`+`installcheck` directly (neither depends on `install`) and invokes verify-results-pgtap.sh directly (skips the `test` prerequisite `verify-results` pulls in) -- same underlying work, no pgxntool changes, no wait on a pgxntool sync. prepare_old/update_scenario/update_check needed no changes at all: pg_tle registration is transparent to `createdb`/`CREATE EXTENSION ... VERSION`/ `ALTER EXTENSION ... UPDATE` once the target database descends from a pg_tle-registered template1 (pg_tle's own registration catalog is per-database; createdb only inherits it because it copies template1 by default) -- so update_scenario runs the exact same multi-hop update path (0.2.2->0.2.3->current) unmodified in pg_tle mode. Also extracts the pg-tle-test job's inline filesystem-contamination check into bin/assert_fs_clean, a reusable script, and calls it at multiple checkpoints instead of trusting a single check: after registering with pg_tle, after the fresh-install smoke test, and -- critically -- after the full update+suite run completes. That last one has to run AFTER the whole flow, not before: the point of pg_tle-mode testing is proving nothing touched the filesystem THROUGHOUT it, not just that the environment started clean. Binary pg_upgrade of a pg_tle-deployed extension is a natural next step (bin/test_existing's prepare_old/run_suite already generalize to it, and the new PGTLE deploy mode in run_suite covers the same use-existing suite run) but isn't included here. --- .github/workflows/ci.yml | 73 +++++++++++++++++++++++++++------------- bin/assert_fs_clean | 71 ++++++++++++++++++++++++++++++++++++++ bin/test_existing | 34 +++++++++++++++++-- 3 files changed, 152 insertions(+), 26 deletions(-) create mode 100755 bin/assert_fs_clean diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bbaf068..7d941db 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -574,14 +574,15 @@ jobs: # pg_tle/non-pg_tle extension installs on one cluster can misbehave. - name: Start PostgreSQL ${{ matrix.pg }} run: pg-start ${{ matrix.pg }} - - name: Snapshot filesystem extension control files (pre-pg_tle baseline) - # Whatever ships on disk by default (e.g. contrib) before we install - # anything. The later comprehensive check diffs against this, so it - # flags ANY extension that lands on disk instead of being registered - # via pg_tle -- not just cat_tools -- without hardcoding contrib names. - run: find /usr/share/postgresql/${{ matrix.pg }}/extension -maxdepth 1 -name '*.control' | sort > /tmp/control_baseline.txt - name: Check out the repo uses: actions/checkout@v6 + - name: Snapshot filesystem extension control files (pre-pg_tle baseline) + # Whatever ships on disk by default (e.g. contrib) before we install + # anything. bin/assert_fs_clean's later checks diff against this, so + # they flag ANY extension that lands on disk instead of being + # registered via pg_tle -- not just cat_tools -- without hardcoding + # contrib names. + run: bin/assert_fs_clean snapshot ${{ matrix.pg }} /tmp/control_baseline.txt - name: Install rsync run: apt-get install -y rsync - name: Build and install pg_tle ${{ env.PGTLE_VERSION }} @@ -596,10 +597,15 @@ jobs: echo "shared_preload_libraries = 'pg_tle'" >> /etc/postgresql/${{ matrix.pg }}/test/postgresql.conf pg_ctlcluster ${{ matrix.pg }} test restart pg_isready -t 30 - - name: Create pg_tle extension - run: psql -c "CREATE EXTENSION pg_tle" - - name: Register cat_tools with pg_tle - run: make run-pgtle + - name: Register pg_tle + cat_tools against template1 + # template1, not the ambient default db: pg_tle's registration catalog + # is per-database, and `createdb` only inherits it because it copies + # template1 by default. Every cat_tools database used below (the + # smoke-test db, and whatever bin/test_existing createdb's) is created + # AFTER this step specifically so it inherits both registrations. + run: | + psql -d template1 -c "CREATE EXTENSION pg_tle" + PGDATABASE=template1 make run-pgtle - name: Verify no stray extension control files landed on the filesystem # CRITICAL, and intentionally redundant with the cat_tools-specific # check in the next step: a filesystem control file silently wins @@ -607,17 +613,11 @@ jobs: # make this whole job a false pass without ever raising an error. # Confirmed hitting exactly this while researching pg_tle version # compatibility (a stale filesystem cat_tools install shadowed the - # pg_tle-registered one under test). pg_tle.control itself is the - # only expected new file since the baseline snapshot. - run: | - find /usr/share/postgresql/${{ matrix.pg }}/extension -maxdepth 1 -name '*.control' | sort > /tmp/control_after.txt - NEW=$(comm -13 /tmp/control_baseline.txt /tmp/control_after.txt | grep -vx '.*/pg_tle\.control' || true) - if [ -n "$NEW" ]; then - echo "FAIL: unexpected extension control file(s) on disk (everything but pg_tle must be registered via pg_tle, not filesystem-installed):" - echo "$NEW" - exit 1 - fi - - name: Install cat_tools purely via pg_tle (no filesystem install) + # pg_tle-registered one under test). Run again after every step below + # that could plausibly write extension files to disk -- never trust a + # single check to catch everything. + run: bin/assert_fs_clean verify ${{ matrix.pg }} /tmp/control_baseline.txt + - name: Install cat_tools purely via pg_tle (fresh install, no filesystem trace) # cat_tools is never `make install`ed in this job, so a successful # CREATE EXTENSION here can only be resolving through pg_tle's # registration, not a control file on disk. Checked explicitly here @@ -626,16 +626,41 @@ jobs: # exclude-list logic has a bug. run: | test ! -e /usr/share/postgresql/${{ matrix.pg }}/extension/cat_tools.control - psql -c "CREATE EXTENSION cat_tools" + createdb cat_tools_smoke + psql -d cat_tools_smoke -c "CREATE EXTENSION cat_tools" - name: Verify cat_tools works when deployed via pg_tle run: | - INSTALLED=$(psql -tAc "SELECT extversion FROM pg_extension WHERE extname = 'cat_tools'") + INSTALLED=$(psql -d cat_tools_smoke -tAc "SELECT extversion FROM pg_extension WHERE extname = 'cat_tools'") EXPECTED=$(make -s print-PGXNVERSION 2>/dev/null | sed -n 's/.*set to "\(.*\)"$/\1/p') echo "installed=$INSTALLED expected=$EXPECTED" if [ -z "$INSTALLED" ] || [ -z "$EXPECTED" ] || [ "$INSTALLED" != "$EXPECTED" ]; then echo "FAIL: installed='$INSTALLED' expected='$EXPECTED'"; exit 1 fi - psql -v ON_ERROR_STOP=1 -c "SELECT relname FROM cat_tools.pg_class_v LIMIT 1" > /dev/null + psql -d cat_tools_smoke -v ON_ERROR_STOP=1 -c "SELECT relname FROM cat_tools.pg_class_v LIMIT 1" > /dev/null + - name: Verify no stray extension control files after the fresh-install smoke test + run: bin/assert_fs_clean verify ${{ matrix.pg }} /tmp/control_baseline.txt + - name: Install PostgreSQL ${{ matrix.pg }} server dev headers + # Needed by `make check-relkind-source`, which bin/test_existing's + # run_suite calls -- every other job that reaches that check already + # installs this; pg-tle-test didn't need it until now because the + # smoke test above never runs the full suite. + run: apt-get install -y postgresql-server-dev-${{ matrix.pg }} + - name: Test the update path via pg_tle (full pgTAP suite, --use-existing) + # Exercises the SAME update path extension-update-test proves for a + # filesystem install (0.2.2 -> current), but entirely through pg_tle. + # bin/test_existing's update_scenario is UNMODIFIED from the + # filesystem case -- only run_suite's internals differ, gated by this + # one env var (see the TEST_EXISTING_DEPLOY comment in + # bin/test_existing) -- so this reuses the exact same update/upgrade + # code the other jobs use instead of duplicating it for pg_tle. + run: TEST_EXISTING_DEPLOY=pgtle bin/test_existing update-scenario cat_tools_update 0.2.2 + - name: Verify no stray extension control files after the pg_tle update test + # Runs AFTER the complete update+full-suite flow, not before: the + # whole point of pg_tle-mode testing is proving NOTHING touched the + # filesystem THROUGHOUT that flow, not merely that the environment + # started clean. A check placed before this point could not catch a + # regression introduced by anything update-scenario itself does. + run: bin/assert_fs_clean verify ${{ matrix.pg }} /tmp/control_baseline.txt # A single stable check name for use as a required status check in branch # protection rules. Matrix jobs produce check names like "🐘 PostgreSQL 14" diff --git a/bin/assert_fs_clean b/bin/assert_fs_clean new file mode 100755 index 0000000..80fcc91 --- /dev/null +++ b/bin/assert_fs_clean @@ -0,0 +1,71 @@ +#!/usr/bin/env bash +# +# assert_fs_clean - Verify no stray PostgreSQL extension control files exist on +# disk, to prove an extension was deployed purely via pg_tle (not filesystem +# install). Extracted from the pg-tle-test CI job so the SAME check can run at +# every checkpoint that could plausibly write extension files to disk +# (registration, after an update, after a binary pg_upgrade, ...), instead of +# being duplicated inline in ci.yml or trusted to a single check at the end. +# +# A pre-existing filesystem control file silently wins over a pg_tle-registered +# extension of the same name -- PostgreSQL never reports an error, it just +# quietly resolves CREATE EXTENSION from disk instead of pg_tle's catalog. That +# makes "prove pg_tle-only" a real, load-bearing assertion, not a formality: it +# must run AFTER whatever it's guarding, not just before, since the whole point +# is confirming nothing wrote to disk THROUGHOUT the guarded flow, not merely +# that the environment started clean. +# +# USAGE: bin/assert_fs_clean [args] +# +# snapshot PG_MAJOR BASELINE_FILE +# Record the current *.control files in PG_MAJOR's extension directory to +# BASELINE_FILE. Run this BEFORE installing pg_tle (or anything else), so +# whatever ships on disk by default (e.g. contrib) is excluded +# automatically -- no hardcoded exclude list to keep in sync. +# +# verify PG_MAJOR BASELINE_FILE +# Fail if any *.control file exists now that wasn't in BASELINE_FILE, +# other than pg_tle.control itself (the one legitimate filesystem install +# in this flow). Run this after EVERY step that could plausibly have +# written extension files to disk. +set -euo pipefail + +extdir_of() { echo "/usr/share/postgresql/$1/extension"; } + +snapshot() { + local pg_major=$1 baseline=$2 + find "$(extdir_of "$pg_major")" -maxdepth 1 -name '*.control' | sort > "$baseline" +} + +verify() { + local pg_major=$1 baseline=$2 after new + after=$(mktemp) + find "$(extdir_of "$pg_major")" -maxdepth 1 -name '*.control' | sort > "$after" + new=$(comm -13 "$baseline" "$after" | grep -vx '.*/pg_tle\.control' || true) + rm -f "$after" + if [ -n "$new" ]; then + echo "FAIL: unexpected extension control file(s) on disk (everything but pg_tle must be registered via pg_tle, not filesystem-installed):" >&2 + echo "$new" >&2 + exit 1 + fi + echo "OK: no stray extension control files on disk (PG $pg_major)" +} + +usage() { + echo "usage: bin/assert_fs_clean [args]" >&2 + echo " snapshot PG_MAJOR BASELINE_FILE" >&2 + echo " verify PG_MAJOR BASELINE_FILE" >&2 + exit 2 +} + +main() { + local cmd=${1:-} + shift || true + case "$cmd" in + snapshot) snapshot "$@" ;; + verify) verify "$@" ;; + *) usage ;; + esac +} + +main "$@" diff --git a/bin/test_existing b/bin/test_existing index 0d60561..15f2087 100755 --- a/bin/test_existing +++ b/bin/test_existing @@ -35,6 +35,23 @@ # # Run `bin/test_existing` with no subcommand to print usage. # +# TEST_EXISTING_DEPLOY=pgtle (env var, checked by run_suite): the extension +# under test was deployed via pg_tle, not the filesystem, and must NEVER be +# filesystem-installed by anything this script does. `make test` unconditionally +# depends on `install` (pgxntool/base.mk's TEST_DEPS), and `make verify-results` +# in turn depends on `test` -- so calling either would install cat_tools to disk +# as a side effect, even though the database itself was never touched via the +# filesystem. In this mode run_suite calls `testdeps`+`installcheck` directly +# (skips the `install` prerequisite that only `test` pulls in) and invokes +# verify-results-pgtap.sh directly (skips the `test` prerequisite that +# `verify-results` pulls in) -- same underlying work, no pgxntool changes +# needed. The CALLER is responsible for having already registered pg_tle + +# cat_tools against `template1` before creating any database this script will +# use (pg_tle's registration is per-database; `createdb` only inherits it +# because it copies `template1` by default) and for asserting filesystem +# cleanliness before/after (see bin/assert_fs_clean) -- this flag only stops +# this script from being the thing that dirties the filesystem itself. +# # Why the dependency guard: "existing" mode must run the suite against the ACTUAL # upgraded/updated objects. If anything silently dropped + reinstalled the # extension, the suite would test a FRESH install and hide a regression. As @@ -223,8 +240,21 @@ run_suite() { # the SAME existing-mode overrides or it would re-run FRESH (against a new # regression DB) instead of verifying THIS existing database. local existing_args="TEST_LOAD_SOURCE=existing CONTRIB_TESTDB=$db EXTRA_REGRESS_OPTS=--use-existing PGXNTOOL_ENABLE_TEST_BUILD=no" - make test $existing_args - make verify-results $existing_args + if [ "${TEST_EXISTING_DEPLOY:-}" = pgtle ]; then + # pg_tle mode (see the TEST_EXISTING_DEPLOY comment at the top of this + # file): `test` and `verify-results` each unconditionally pull in + # `install`, so run the same underlying work directly instead -- + # `testdeps`+`installcheck` (neither depends on `install`), then the + # pgtap-mode verify-results script directly (skips its `test` prerequisite). + make testdeps $existing_args + make installcheck $existing_args + local testout + testout=$(make -s print-TESTOUT 2>/dev/null | sed -n 's/.*set to "\(.*\)"$/\1/p') + pgxntool/verify-results-pgtap.sh "$testout" + else + make test $existing_args + make verify-results $existing_args + fi # Post-suite guard assertion: a CASCADE drop+reinstall during the run would # have removed the guard view, meaning the suite tested a fresh install, not $db. guard_present "$db" \ From 0ce2bb7625270563bb1e0cb0fef2293c321e06fb Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Fri, 24 Jul 2026 16:52:27 -0500 Subject: [PATCH 04/13] ci: fix false-positive contamination check from pgTAP's own filesystem install The real CI run failed after the previous commit: the third fs-clean checkpoint (after the pg_tle update test) flagged pgtap.control as an unexpected file. bin/test_existing's pg_tle-mode run_suite calls `make testdeps`, which filesystem-installs pgTAP for pg_regress to load -- a legitimate, load-bearing dependency of the TEST HARNESS itself, unrelated to cat_tools's own pg_tle-only deployment story (pgTAP isn't being deployed via pg_tle here, and never will be). The baseline snapshot was taken before pgTAP got installed, so its arrival read as contamination. Local testing missed this because this container already had pgtap filesystem-installed from earlier, unrelated work, so the baseline silently already included it and the install was a no-op. Verified the actual bug and the fix using PG11 (present in this container but with no prior pgtap install): confirmed `make testdeps`-triggered pgtap install DOES trip the check when taken as new, and confirmed installing pgtap explicitly before the baseline snapshot correctly folds it into the accepted starting state. Fixed by adding an explicit `make pgtap` step before the baseline snapshot, rather than hardcoding "pgtap" into assert_fs_clean's exclude list -- keeps the general baseline-diff design intact (whatever exists before instrumenting is accepted, no exclude-list names to maintain beyond pg_tle.control, the one thing this flow deliberately adds). --- .github/workflows/ci.yml | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7d941db..cb8c552 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -576,15 +576,28 @@ jobs: run: pg-start ${{ matrix.pg }} - name: Check out the repo uses: actions/checkout@v6 - - name: Snapshot filesystem extension control files (pre-pg_tle baseline) - # Whatever ships on disk by default (e.g. contrib) before we install - # anything. bin/assert_fs_clean's later checks diff against this, so - # they flag ANY extension that lands on disk instead of being - # registered via pg_tle -- not just cat_tools -- without hardcoding - # contrib names. - run: bin/assert_fs_clean snapshot ${{ matrix.pg }} /tmp/control_baseline.txt - name: Install rsync run: apt-get install -y rsync + - name: Install pgtap (test harness dependency) + # pgTAP is a filesystem-installed dependency of the TEST HARNESS, not + # part of what this job proves is pg_tle-only -- it's not being + # deployed via pg_tle here, and never will be. `bin/test_existing`'s + # pg_tle-mode run_suite calls `make testdeps`, which would otherwise + # filesystem-install pgTAP for the first time partway through the job + # and trip the contamination checks below (confirmed happening in a + # real CI run). Installing it explicitly here, before the baseline + # snapshot, makes it part of the accepted starting state -- like any + # other extension already on disk -- instead of needing a hardcoded + # exclude-by-name rule that would erode the whole point of diffing + # against a baseline. + run: make pgtap + - name: Snapshot filesystem extension control files (pre-pg_tle baseline) + # Whatever ships on disk by default (e.g. contrib, and now pgtap) + # before we install pg_tle. bin/assert_fs_clean's later checks diff + # against this, so they flag ANY extension that lands on disk instead + # of being registered via pg_tle -- not just cat_tools -- without + # hardcoding contrib/pgtap names. + run: bin/assert_fs_clean snapshot ${{ matrix.pg }} /tmp/control_baseline.txt - name: Build and install pg_tle ${{ env.PGTLE_VERSION }} # flex/bison/libkrb5-dev aren't in the pgxn-tools image; pg_tle's build # needs them (guc-file.l, and clientauth.c includes gssapi.h). From 7fcd35f326a33b77a8c10837144163adf12f3840 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Fri, 24 Jul 2026 17:48:16 -0500 Subject: [PATCH 05/13] ci: test binary pg_upgrade of a pg_tle-deployed cat_tools pg-tle-test only proves fresh-install and update-in-place work via pg_tle; there was no coverage for binary pg_upgrade of a pg_tle-registered cat_tools (the scenario that matters for an already-running RDS/Aurora instance being upgraded to a new PostgreSQL major version). Adds pg-tle-upgrade-test, mirroring pg-upgrade-test's old_pg=12 legs (12->13, 12->18 -- the only ones within pg_tle 1.5.2's own supported range) with pg_tle deployment instead of filesystem install on the old cluster. The new cluster's binary tree needs nothing cat_tools-specific beyond pg_tle's own framework files (build + install + shared_preload_libraries): binary pg_upgrade's schema-only restore recreates each object individually via binary_upgrade_create_empty_extension, and pg_tle's own catalog tables (holding cat_tools's registered SQL) get carried over as ordinary heap data during the physical copy phase. No re-registration needed on the new side. Reuses bin/test_existing's plant-guard/update/run-suite unmodified (the same TEST_EXISTING_DEPLOY=pgtle mode added for pg-tle-test's update-path leg covers this too) -- no new pg_tle-specific test logic, just a pg_tle-based setup feeding the same shared functions the filesystem-based upgrade jobs use. Also carries pg-tle-test's fs-contamination checkpoints (before pg_tle, after registration, after pgtap install, after pg_upgrade, and critically after the full update+suite run) on both the old and new cluster. pg_tle's C code means it can't reuse a single build across PG_CONFIGs the way cat_tools's pure-SQL install does -- a stale .so compiled against the wrong PostgreSQL headers would silently install wrong, so the old and new clusters each get their own fresh clone+build rather than reinstalling one build with a different PG_CONFIG. Verified locally: the plant-guard -> update -> run-suite(TEST_EXISTING_DEPLOY =pgtle) sequence runs cleanly end to end (all 14 pgTAP tests, zero filesystem trace) against a scratch cluster. The actual binary pg_upgrade invocation is unchanged from pg-upgrade-test's already-CI-proven form (only adding the shared_preload_libraries line), and the pg_upgrade+pg_tle mechanism itself was separately hands-on verified in earlier research (PG16->PG18, all 14 tests passing post-upgrade) -- this container had no PostgreSQL major version pairing available that was both pg_tle 1.5.2- compatible and free of pre-existing cat_tools filesystem installs from other concurrent work, so the full cross-version flow is unproven until real CI runs it. --- .github/workflows/ci.yml | 144 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 143 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cb8c552..1498f78 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -675,6 +675,148 @@ jobs: # regression introduced by anything update-scenario itself does. run: bin/assert_fs_clean verify ${{ matrix.pg }} /tmp/control_baseline.txt + pg-tle-upgrade-test: + needs: [changes] + if: needs.changes.outputs.docs_only != 'true' + strategy: + matrix: + include: + # Mirrors pg-upgrade-test's old_pg=12 legs (the only ones cleanly + # within pg_tle 1.5.2's own supported range, PG12-18) so the SAME PG + # version jumps are proven for both deployment methods. Unlike + # pg-upgrade-test, there is no bridge/old_install complexity here -- + # pg_tle has no equivalent of the pre-0.2.2 filesystem install-script + # issues (those are about SELECT * over catalog columns in old SQL + # files, irrelevant to how the extension gets registered). + - old_pg: "12" + new_pg: "13" + - old_pg: "12" + new_pg: "18" + name: 🧩🔄 pg_tle 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 + PGTLE_VERSION: "1.5.2" + steps: + # A dedicated cluster pair, never shared with the other jobs in this + # workflow -- same reasoning as pg-tle-test. + - name: Start PostgreSQL ${{ matrix.old_pg }} + run: pg-start ${{ matrix.old_pg }} + - name: Recreate old cluster with data checksums enabled + run: | + pg_ctlcluster ${{ matrix.old_pg }} test stop + pg_dropcluster ${{ matrix.old_pg }} test + pg_createcluster -p 5432 ${{ matrix.old_pg }} test -- $INITDB_OPTS + pg_ctlcluster ${{ matrix.old_pg }} test start + pg_isready -t 30 + - name: Check out the repo + uses: actions/checkout@v6 + - name: Install rsync + run: apt-get install -y rsync + - name: Snapshot filesystem extension control files (old cluster baseline) + run: bin/assert_fs_clean snapshot ${{ matrix.old_pg }} /tmp/control_baseline_old.txt + - name: Build and install pg_tle ${{ env.PGTLE_VERSION }} on the old cluster + # flex/bison/libkrb5-dev aren't in the pgxn-tools image; pg_tle's build + # needs them (guc-file.l, and clientauth.c includes gssapi.h). + run: | + apt-get install -y flex bison libkrb5-dev + git clone --branch v${{ env.PGTLE_VERSION }} --depth 1 https://github.com/aws/pg_tle.git /tmp/pg_tle_old + make -C /tmp/pg_tle_old install + - name: Enable pg_tle and restart PostgreSQL ${{ matrix.old_pg }} + run: | + echo "shared_preload_libraries = 'pg_tle'" >> /etc/postgresql/${{ matrix.old_pg }}/test/postgresql.conf + pg_ctlcluster ${{ matrix.old_pg }} test restart + pg_isready -t 30 + - name: Register pg_tle + cat_tools against template1 (old cluster) + run: | + psql -d template1 -c "CREATE EXTENSION pg_tle" + PGDATABASE=template1 make run-pgtle + - name: Verify no stray extension control files on the old cluster + run: bin/assert_fs_clean verify ${{ matrix.old_pg }} /tmp/control_baseline_old.txt + - name: Prepare the old cluster (install via pg_tle + dependency guard) + # A fresh 0.2.2 install (no bridge needed -- see pg-upgrade-test's own + # comment on why its old_pg>=11 legs don't need one), created via + # pg_tle only, then the same dependency guard bin/test_existing's + # other subcommands rely on to prove existing-mode never silently + # drops+reinstalls the extension. + run: | + test ! -e /usr/share/postgresql/${{ matrix.old_pg }}/extension/cat_tools.control + createdb cat_tools_tle_upgrade + psql -d cat_tools_tle_upgrade -c "CREATE EXTENSION cat_tools VERSION '0.2.2'" + bin/test_existing plant-guard cat_tools_tle_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 on the new cluster (test harness dependency) + # Must happen BEFORE the new-cluster baseline snapshot below, same + # reasoning as pg-tle-test's "Install pgtap" step: pgTAP is a + # filesystem-installed dependency of the test harness, unrelated to + # cat_tools's own pg_tle-only deployment story, so it needs to be part + # of the accepted starting state rather than flagged as contamination. + # PATH is set explicitly (not just relying on apt's update-alternatives + # switch from installing postgresql-${{ matrix.new_pg }} above) since + # `pgxn install` resolves pg_config via PATH, not via a passed-through + # PG_CONFIG make variable. + run: PATH=/usr/lib/postgresql/${{ matrix.new_pg }}/bin:$PATH make pgtap + - name: Snapshot filesystem extension control files (new cluster baseline) + run: bin/assert_fs_clean snapshot ${{ matrix.new_pg }} /tmp/control_baseline_new.txt + - name: Build and install pg_tle ${{ env.PGTLE_VERSION }} on the new cluster + # Only pg_tle's OWN framework is needed on the new binary tree -- NOT + # cat_tools. Binary pg_upgrade's schema-only restore recreates each + # object individually (binary_upgrade_create_empty_extension), and + # pg_tle's own catalog tables (holding cat_tools's registered SQL) are + # carried over as ordinary heap data during the physical copy phase -- + # no re-registration needed on the new side. A SEPARATE clone + # (pg_tle_new, not pg_tle_old rebuilt) avoids installing a .so still + # compiled against the old cluster's PostgreSQL headers: pg_tle has C + # code, unlike cat_tools, so switching PG_CONFIG on an already-built + # tree without a fresh build would risk installing the wrong binary. + run: | + git clone --branch v${{ env.PGTLE_VERSION }} --depth 1 https://github.com/aws/pg_tle.git /tmp/pg_tle_new + make -C /tmp/pg_tle_new install PG_CONFIG=/usr/lib/postgresql/${{ matrix.new_pg }}/bin/pg_config + - name: Verify no stray extension control files on the new cluster (pre-upgrade) + run: bin/assert_fs_clean verify ${{ matrix.new_pg }} /tmp/control_baseline_new.txt + - name: Stop old cluster, create new cluster, enable pg_tle, binary pg_upgrade, start new cluster + run: | + pg_ctlcluster ${{ matrix.old_pg }} test stop + pg_createcluster -p 5432 ${{ matrix.new_pg }} test -- $INITDB_OPTS + echo "shared_preload_libraries = 'pg_tle'" >> /etc/postgresql/${{ matrix.new_pg }}/test/postgresql.conf + # 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/${{ matrix.new_pg }}/bin/pg_upgrade \ + -b /usr/lib/postgresql/${{ matrix.old_pg }}/bin \ + -B /usr/lib/postgresql/${{ matrix.new_pg }}/bin \ + -d /var/lib/postgresql/${{ matrix.old_pg }}/test \ + -D /var/lib/postgresql/${{ matrix.new_pg }}/test \ + -o '-c config_file=/etc/postgresql/${{ matrix.old_pg }}/test/postgresql.conf' \ + -O '-c config_file=/etc/postgresql/${{ matrix.new_pg }}/test/postgresql.conf'" postgres \ + || { find /tmp/pg_upgrade_logs \ + /var/lib/postgresql/${{ matrix.new_pg }}/test/pg_upgrade_output.d \ + -name '*.log' 2>/dev/null | sort | xargs -r tail -n +1; exit 1; } + pg_ctlcluster ${{ matrix.new_pg }} test start + pg_isready -t 30 + - name: Verify no stray extension control files after binary pg_upgrade + run: bin/assert_fs_clean verify ${{ matrix.new_pg }} /tmp/control_baseline_new.txt + - name: Update the pg_upgraded extension to the current version + # Exercises ALTER EXTENSION UPDATE on genuinely pg_upgraded objects + # registered via pg_tle. update_ext() is pure psql (no `make` calls), + # so it's identical regardless of deployment method -- no pg_tle-mode + # flag needed here, only for run-suite below. + run: bin/test_existing update cat_tools_tle_upgrade + - name: Run the suite against the pg_upgraded database via pg_tle (existing mode) + run: TEST_EXISTING_DEPLOY=pgtle bin/test_existing run-suite cat_tools_tle_upgrade + - name: Verify no stray extension control files after the pg_tle upgrade+update+suite run + # CRITICAL: must run AFTER the complete flow, not before -- same + # principle as pg-tle-test's final checkpoint. Proves nothing touched + # the filesystem across binary pg_upgrade, the extension update, AND + # the full pgTAP suite run, not merely that the environment started + # clean. + run: bin/assert_fs_clean verify ${{ matrix.new_pg }} /tmp/control_baseline_new.txt + # A single stable check name for use as a required status check in branch # protection rules. Matrix jobs produce check names like "🐘 PostgreSQL 14" # which would all need to be listed individually and updated whenever the @@ -682,7 +824,7 @@ jobs: # 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, test, pg-upgrade-test, pg-upgrade-stepwise, extension-update-test, pg-tle-test] + needs: [changes, test, pg-upgrade-test, pg-upgrade-stepwise, extension-update-test, pg-tle-test, pg-tle-upgrade-test] if: always() runs-on: ubuntu-latest steps: From bd94a2d00a6e7da393ec38ff1b1e5fde94b98082 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Fri, 24 Jul 2026 18:26:49 -0500 Subject: [PATCH 06/13] ci: dedupe pg-upgrade-test/pg-tle-upgrade-test's binary pg_upgrade mechanics The two jobs' cluster-recreation and binary pg_upgrade steps were near-identical (pg-tle-upgrade-test's version differs only by one extra config line to enable pg_tle before the upgrade) -- ~55 lines of copy-pasted shell duplicated across both jobs. Extracted into bin/pg_upgrade_cluster, mirroring the project's existing pattern of pushing shared CI shell logic into bin/ scripts (bin/test_existing, bin/assert_fs_clean) rather than GitHub Actions composite actions. Two subcommands: recreate-old (recreate the pg-start default cluster with data checksums enabled) and upgrade (create the new cluster, optionally append an extra config line, run pg_upgrade with its log-dumping-on-failure behavior, start the new cluster). Both jobs needed "Check out the repo" moved earlier (before the recreate-old step) since the script lives in the repo -- previously that step ran before checkout in both jobs. Verified locally end to end using scratch PG11->PG16 clusters (not the shared main clusters): recreate-old correctly enables checksums, and upgrade succeeds identically both with and without an extra config line, matching each job's respective call. Also references the pgxntool issue opened for an install-skip override (https://github.com/Postgres-Extensions/pgxntool/issues/55) in bin/test_existing's TEST_EXISTING_DEPLOY comment, since that feature would let the testdeps+installcheck workaround there be simplified once it lands. --- .github/workflows/ci.yml | 61 +++------------------------ bin/pg_upgrade_cluster | 90 ++++++++++++++++++++++++++++++++++++++++ bin/test_existing | 10 +++-- 3 files changed, 103 insertions(+), 58 deletions(-) create mode 100755 bin/pg_upgrade_cluster diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1498f78..5511956 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -285,20 +285,12 @@ jobs: steps: - name: Start PostgreSQL ${{ matrix.old_pg }} run: pg-start ${{ matrix.old_pg }} - - name: Recreate old cluster with data checksums enabled - run: | - pg_ctlcluster ${{ matrix.old_pg }} test stop - pg_dropcluster ${{ matrix.old_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 ${{ matrix.old_pg }} test -- $INITDB_OPTS - pg_ctlcluster ${{ matrix.old_pg }} test start - pg_isready -t 30 - name: Check out the repo uses: actions/checkout@v6 - name: Install rsync run: apt-get install -y rsync + - name: Recreate old cluster with data checksums enabled + run: bin/pg_upgrade_cluster recreate-old ${{ matrix.old_pg }} - name: Install cat_tools into old cluster run: make install - name: Prepare the old cluster (install + bridge + dependency guard) @@ -320,24 +312,7 @@ jobs: # 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: | - pg_ctlcluster ${{ matrix.old_pg }} test stop - pg_createcluster -p 5432 ${{ matrix.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/${{ matrix.new_pg }}/bin/pg_upgrade \ - -b /usr/lib/postgresql/${{ matrix.old_pg }}/bin \ - -B /usr/lib/postgresql/${{ matrix.new_pg }}/bin \ - -d /var/lib/postgresql/${{ matrix.old_pg }}/test \ - -D /var/lib/postgresql/${{ matrix.new_pg }}/test \ - -o '-c config_file=/etc/postgresql/${{ matrix.old_pg }}/test/postgresql.conf' \ - -O '-c config_file=/etc/postgresql/${{ matrix.new_pg }}/test/postgresql.conf'" postgres \ - || { find /tmp/pg_upgrade_logs \ - /var/lib/postgresql/${{ matrix.new_pg }}/test/pg_upgrade_output.d \ - -name '*.log' 2>/dev/null | sort | xargs -r tail -n +1; exit 1; } - pg_ctlcluster ${{ matrix.new_pg }} test start + run: bin/pg_upgrade_cluster upgrade ${{ matrix.old_pg }} ${{ matrix.new_pg }} - name: Update the pg_upgraded extension to the current version # Exercises ALTER EXTENSION UPDATE on genuinely pg_upgraded objects (the # extension binary pg_upgrade just migrated), running the version-to-version @@ -705,17 +680,12 @@ jobs: # workflow -- same reasoning as pg-tle-test. - name: Start PostgreSQL ${{ matrix.old_pg }} run: pg-start ${{ matrix.old_pg }} - - name: Recreate old cluster with data checksums enabled - run: | - pg_ctlcluster ${{ matrix.old_pg }} test stop - pg_dropcluster ${{ matrix.old_pg }} test - pg_createcluster -p 5432 ${{ matrix.old_pg }} test -- $INITDB_OPTS - pg_ctlcluster ${{ matrix.old_pg }} test start - pg_isready -t 30 - name: Check out the repo uses: actions/checkout@v6 - name: Install rsync run: apt-get install -y rsync + - name: Recreate old cluster with data checksums enabled + run: bin/pg_upgrade_cluster recreate-old ${{ matrix.old_pg }} - name: Snapshot filesystem extension control files (old cluster baseline) run: bin/assert_fs_clean snapshot ${{ matrix.old_pg }} /tmp/control_baseline_old.txt - name: Build and install pg_tle ${{ env.PGTLE_VERSION }} on the old cluster @@ -779,26 +749,7 @@ jobs: - name: Verify no stray extension control files on the new cluster (pre-upgrade) run: bin/assert_fs_clean verify ${{ matrix.new_pg }} /tmp/control_baseline_new.txt - name: Stop old cluster, create new cluster, enable pg_tle, binary pg_upgrade, start new cluster - run: | - pg_ctlcluster ${{ matrix.old_pg }} test stop - pg_createcluster -p 5432 ${{ matrix.new_pg }} test -- $INITDB_OPTS - echo "shared_preload_libraries = 'pg_tle'" >> /etc/postgresql/${{ matrix.new_pg }}/test/postgresql.conf - # 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/${{ matrix.new_pg }}/bin/pg_upgrade \ - -b /usr/lib/postgresql/${{ matrix.old_pg }}/bin \ - -B /usr/lib/postgresql/${{ matrix.new_pg }}/bin \ - -d /var/lib/postgresql/${{ matrix.old_pg }}/test \ - -D /var/lib/postgresql/${{ matrix.new_pg }}/test \ - -o '-c config_file=/etc/postgresql/${{ matrix.old_pg }}/test/postgresql.conf' \ - -O '-c config_file=/etc/postgresql/${{ matrix.new_pg }}/test/postgresql.conf'" postgres \ - || { find /tmp/pg_upgrade_logs \ - /var/lib/postgresql/${{ matrix.new_pg }}/test/pg_upgrade_output.d \ - -name '*.log' 2>/dev/null | sort | xargs -r tail -n +1; exit 1; } - pg_ctlcluster ${{ matrix.new_pg }} test start - pg_isready -t 30 + run: bin/pg_upgrade_cluster upgrade ${{ matrix.old_pg }} ${{ matrix.new_pg }} "shared_preload_libraries = 'pg_tle'" - name: Verify no stray extension control files after binary pg_upgrade run: bin/assert_fs_clean verify ${{ matrix.new_pg }} /tmp/control_baseline_new.txt - name: Update the pg_upgraded extension to the current version diff --git a/bin/pg_upgrade_cluster b/bin/pg_upgrade_cluster new file mode 100755 index 0000000..e480525 --- /dev/null +++ b/bin/pg_upgrade_cluster @@ -0,0 +1,90 @@ +#!/usr/bin/env bash +# +# pg_upgrade_cluster - Shared binary pg_upgrade mechanics for CI. Used by BOTH +# pg-upgrade-test (filesystem-installed cat_tools) and pg-tle-upgrade-test +# (pg_tle-registered cat_tools) in .github/workflows/ci.yml -- this part of +# the flow has nothing extension-specific about it, so it lives here once +# instead of being duplicated near-verbatim in both jobs. +# +# USAGE: bin/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 [EXTRA_NEW_CONF] +# Stop the old cluster, create the new cluster's "test" cluster (same +# $INITDB_OPTS), optionally append EXTRA_NEW_CONF to its +# postgresql.conf (e.g. to enable pg_tle before pg_upgrade runs), 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 extra_conf=${3:-} + pg_ctlcluster "$old_pg" test stop + pg_createcluster -p 5432 "$new_pg" test -- $INITDB_OPTS + # Use `if`, not `&&`: a false test under `set -e` would abort the script. + if [ -n "$extra_conf" ]; then + echo "$extra_conf" >> "/etc/postgresql/$new_pg/test/postgresql.conf" + fi + # 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: bin/pg_upgrade_cluster [args]" >&2 + echo " recreate-old PG_VERSION" >&2 + echo " upgrade OLD_PG NEW_PG [EXTRA_NEW_CONF]" >&2 + exit 2 +} + +main() { + local cmd=${1:-} + shift || true + case "$cmd" in + recreate-old) recreate_old "$@" ;; + upgrade) upgrade "$@" ;; + *) usage ;; + esac +} + +main "$@" diff --git a/bin/test_existing b/bin/test_existing index 15f2087..32a4f4c 100755 --- a/bin/test_existing +++ b/bin/test_existing @@ -45,9 +45,13 @@ # (skips the `install` prerequisite that only `test` pulls in) and invokes # verify-results-pgtap.sh directly (skips the `test` prerequisite that # `verify-results` pulls in) -- same underlying work, no pgxntool changes -# needed. The CALLER is responsible for having already registered pg_tle + -# cat_tools against `template1` before creating any database this script will -# use (pg_tle's registration is per-database; `createdb` only inherits it +# needed. This whole workaround could be simplified away if pgxntool grows an +# install-skip override (requested upstream: +# https://github.com/Postgres-Extensions/pgxntool/issues/55) -- revisit this +# block if/when that lands. The CALLER is responsible for having already +# registered pg_tle + cat_tools against `template1` before creating any +# database this script will use (pg_tle's registration is per-database; +# `createdb` only inherits it # because it copies `template1` by default) and for asserting filesystem # cleanliness before/after (see bin/assert_fs_clean) -- this flag only stops # this script from being the thing that dirties the filesystem itself. From 92e9a03fb978ba1ca504dc714a036f117ddb0ddd Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sat, 25 Jul 2026 17:14:54 -0500 Subject: [PATCH 07/13] ci: move pg_upgrade_cluster to .github/scripts/, document CI latency ideas pg_upgrade_cluster is CI-mechanics-only (drives pg_ctlcluster/pg_createcluster /pg_upgrade against the pgxn-tools image's "test" cluster convention) -- unlike bin/test_existing, a developer can't meaningfully run it locally, so it belongs under .github/ rather than alongside the locally-usable bin/ scripts. Verified no Makefile/dist-packaging references depend on its bin/ location before moving. Also adds .github/workflows/CLAUDE.md capturing where things stand on CI wall-clock time: GitHub Actions steps are strictly sequential within a job (no free win from reordering independent steps), pg-start does real apt-install + cluster-create work rather than being a no-op, apt/dpkg's lock rules out backgrounding anything alongside another apt-get call, and actions/cache (for pg_tle's from-source rebuild, repeated identically across every pg-tle-test/pg-tle-upgrade-test matrix entry) is probably a better lever than hand-rolled parallelism if this ever becomes worth pursuing. Also confirms this workflow's push trigger is already scoped to branches: [master] (not a bare push/pull_request), so PR-branch commits don't double-run CI -- checked against actual run history for this PR. --- {bin => .github/scripts}/pg_upgrade_cluster | 9 +++- .github/workflows/CLAUDE.md | 52 +++++++++++++++++++++ .github/workflows/ci.yml | 8 ++-- 3 files changed, 63 insertions(+), 6 deletions(-) rename {bin => .github/scripts}/pg_upgrade_cluster (89%) create mode 100644 .github/workflows/CLAUDE.md diff --git a/bin/pg_upgrade_cluster b/.github/scripts/pg_upgrade_cluster similarity index 89% rename from bin/pg_upgrade_cluster rename to .github/scripts/pg_upgrade_cluster index e480525..b413643 100755 --- a/bin/pg_upgrade_cluster +++ b/.github/scripts/pg_upgrade_cluster @@ -6,7 +6,12 @@ # the flow has nothing extension-specific about it, so it lives here once # instead of being duplicated near-verbatim in both jobs. # -# USAGE: bin/pg_upgrade_cluster [args] +# 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 @@ -71,7 +76,7 @@ upgrade() { } usage() { - echo "usage: bin/pg_upgrade_cluster [args]" >&2 + echo "usage: .github/scripts/pg_upgrade_cluster [args]" >&2 echo " recreate-old PG_VERSION" >&2 echo " upgrade OLD_PG NEW_PG [EXTRA_NEW_CONF]" >&2 exit 2 diff --git a/.github/workflows/CLAUDE.md b/.github/workflows/CLAUDE.md new file mode 100644 index 0000000..7133c7e --- /dev/null +++ b/.github/workflows/CLAUDE.md @@ -0,0 +1,52 @@ +# CI Instructions for cat_tools + +## Reducing CI wall-clock time + +Don't chase this proactively -- it's not worth adding real complexity for. But if a *simple* +latency win is available while touching a job anyway, take it. Facts and constraints to reason +from: + +- **GitHub Actions steps within one job are strictly sequential.** There is no native + "run step A and step B concurrently" mechanism. The only real unit of parallelism is the + *job* (already exploited here via the `pg`/matrix-leg lists) -- reordering two independent + steps changes nothing about total wall-clock time unless one of them is actually + backgrounded (`cmd &` ... `wait`) within a single step's shell script. +- **`pg-start VERSION` is not a no-op or a "wait for something already running."** It installs + that PostgreSQL major version via `apt.postgresql.org.sh` (network + package install) and + then creates and starts the "test" cluster (`pg_createcluster --start`) -- both genuinely + slow, and both block until done, like every `run:` step. +- **Backgrounding a step next to `pg-start` (or any `apt-get install`) is NOT safe by + default**: apt/dpkg serialize on a lock file, so a second concurrent `apt-get install` would + either fail immediately or block waiting for the lock -- effectively serializing anyway, but + now with added flakiness risk. Only background work that doesn't touch the package manager + (e.g. a `git clone`, or a build step that's pure `gmake`/`gcc` against already-installed + headers) alongside an apt-based step. +- **A genuine, low-complexity candidate, if this is ever worth doing**: `pg-tle-test`'s + "Install pgtap" step (`make pgtap`, which builds pgTAP via `gmake`/`gcc`, not apt) and its + "Build and install pg_tle" step (`git clone` + `make`, also not apt) are mutually + independent -- neither depends on the other's output. Backgrounding one (`make pgtap &`) + while the other runs, then `wait`ing for it before the step that needs pgtap, would overlap + two of the job's slower operations. This is a small, contained change (one background job, + one `wait`, one exit-code check) -- not the kind of complexity worth avoiding on principle. +- **`actions/cache` is likely a better lever than backgrounding.** pg_tle is rebuilt from + source (git clone + C compile) fresh in every `pg-tle-test` matrix entry and on both the old + and new cluster in every `pg-tle-upgrade-test` leg -- the same pg_tle version, rebuilt + repeatedly across runs with nothing changed. Caching the built `pg_tle.so` + control/SQL + files (keyed on PG major version + `PGTLE_VERSION`) would skip the clone+compile entirely on + a cache hit, without touching the job's logic at all. Worth trying before hand-rolled + parallelism if CI time on the pg_tle jobs becomes a real pain point. +- **Double-run check**: this repo's `push:` trigger is scoped to `branches: [master]` (not a + bare `on: [push, pull_request]`), so pushes to a feature/PR branch only fire the + `pull_request` event, not both -- confirmed via run history, no double-run here. If this + workflow's triggers are ever changed, re-verify that scoping is preserved; an unscoped + `push:` trigger would double-run CI on every PR-branch commit. + +## Where CI-only shell logic lives + +`.github/scripts/` holds shell scripts that are **only** meaningful inside a CI job (e.g. +`pg_upgrade_cluster`, which drives `pg_ctlcluster`/`pg_createcluster`/`pg_upgrade` against the +pgxn-tools image's "test" cluster convention -- not something a developer would run locally). +This is distinct from the repo-root `bin/` scripts (`bin/test_existing`, `bin/assert_fs_clean`), +which a developer CAN run locally against a scratch database and are also usable outside CI. +When adding a new shared CI shell helper, put it in `.github/scripts/` if it's CI-only, `bin/` +if it's usable locally too. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5511956..5128858 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -290,7 +290,7 @@ jobs: - name: Install rsync run: apt-get install -y rsync - name: Recreate old cluster with data checksums enabled - run: bin/pg_upgrade_cluster recreate-old ${{ matrix.old_pg }} + run: .github/scripts/pg_upgrade_cluster recreate-old ${{ matrix.old_pg }} - name: Install cat_tools into old cluster run: make install - name: Prepare the old cluster (install + bridge + dependency guard) @@ -312,7 +312,7 @@ jobs: # 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: bin/pg_upgrade_cluster upgrade ${{ matrix.old_pg }} ${{ matrix.new_pg }} + run: .github/scripts/pg_upgrade_cluster upgrade ${{ matrix.old_pg }} ${{ matrix.new_pg }} - name: Update the pg_upgraded extension to the current version # Exercises ALTER EXTENSION UPDATE on genuinely pg_upgraded objects (the # extension binary pg_upgrade just migrated), running the version-to-version @@ -685,7 +685,7 @@ jobs: - name: Install rsync run: apt-get install -y rsync - name: Recreate old cluster with data checksums enabled - run: bin/pg_upgrade_cluster recreate-old ${{ matrix.old_pg }} + run: .github/scripts/pg_upgrade_cluster recreate-old ${{ matrix.old_pg }} - name: Snapshot filesystem extension control files (old cluster baseline) run: bin/assert_fs_clean snapshot ${{ matrix.old_pg }} /tmp/control_baseline_old.txt - name: Build and install pg_tle ${{ env.PGTLE_VERSION }} on the old cluster @@ -749,7 +749,7 @@ jobs: - name: Verify no stray extension control files on the new cluster (pre-upgrade) run: bin/assert_fs_clean verify ${{ matrix.new_pg }} /tmp/control_baseline_new.txt - name: Stop old cluster, create new cluster, enable pg_tle, binary pg_upgrade, start new cluster - run: bin/pg_upgrade_cluster upgrade ${{ matrix.old_pg }} ${{ matrix.new_pg }} "shared_preload_libraries = 'pg_tle'" + run: .github/scripts/pg_upgrade_cluster upgrade ${{ matrix.old_pg }} ${{ matrix.new_pg }} "shared_preload_libraries = 'pg_tle'" - name: Verify no stray extension control files after binary pg_upgrade run: bin/assert_fs_clean verify ${{ matrix.new_pg }} /tmp/control_baseline_new.txt - name: Update the pg_upgraded extension to the current version From 8365c0baa5c24f573859923ca821bc62d3155926 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sat, 25 Jul 2026 17:35:46 -0500 Subject: [PATCH 08/13] ci: de-scope CLAUDE.md title from cat_tools This CI guidance is generic GitHub Actions advice, not specific to this project. --- .github/workflows/CLAUDE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/CLAUDE.md b/.github/workflows/CLAUDE.md index 7133c7e..17b6cfa 100644 --- a/.github/workflows/CLAUDE.md +++ b/.github/workflows/CLAUDE.md @@ -1,4 +1,4 @@ -# CI Instructions for cat_tools +# CI Instructions ## Reducing CI wall-clock time From 8f6f6ef8dae2f4cfa9693e3344a5eaadf4cf4f42 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sat, 25 Jul 2026 18:07:47 -0500 Subject: [PATCH 09/13] ci: report which commit was actually tested on a docs-only push A docs-only push makes the heavy jobs report "skipped" for that specific commit, which is correct (the code didn't change) but easy to misread as "never tested" when just glancing at the PR's Checks tab -- the actual test results live on an earlier commit's run. The `changes` job now walks backward from HEAD (bounded by the PR's base, or a depth cap for a direct push to master) to find the newest commit whose own diff from its parent touched more than docs, then looks up that commit's most recent completed "CI" workflow run. `all-checks-passed` reports this in its step summary: a link to that run plus whether it was green, or an explicit note when the entire PR/push has never touched anything but docs (no "last tested" commit exists to report in that case). Verified the backward-walk logic and the Actions-API lookup standalone against this actual repo/branch: correctly identifies 92e9a03 (the last code-changing commit before this PR's own docs-only tail) and correctly finds its CI run as conclusion=success. Reporting only -- does not change what makes all-checks-passed pass/fail. --- .github/workflows/ci.yml | 106 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5128858..9382d20 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -20,8 +20,20 @@ jobs: changes: name: 🔍 Detect changes & derive PG matrix runs-on: ubuntu-latest + permissions: + # Needed only by "Find the last commit where real code changed" below, + # to query the Actions API for a past run's conclusion/URL. + actions: read outputs: docs_only: ${{ steps.diff.outputs.docs_only }} + # Set when docs_only is true: whether there's a real "last tested" commit + # to report at all (false), or this entire PR/push chain has never + # touched anything but docs (true, docs_only_pr) -- see the "Find the + # last commit..." step below for how these are derived. + docs_only_pr: ${{ steps.last_tested.outputs.docs_only_pr }} + last_tested_sha: ${{ steps.last_tested.outputs.last_tested_sha }} + last_tested_conclusion: ${{ steps.last_tested.outputs.last_tested_conclusion }} + last_tested_url: ${{ steps.last_tested.outputs.last_tested_url }} # Derived PG-major lists (see the "Derive ..." step for their meaning). supported_pg: ${{ steps.pg.outputs.supported_pg }} update_pg: ${{ steps.pg.outputs.update_pg }} @@ -85,6 +97,78 @@ jobs: echo "$CHANGED" echo "docs_only=$DOCS_ONLY" >> "$GITHUB_OUTPUT" + - name: Find the last commit where real code changed + # This push is docs-only, so the heavy jobs below are about to skip -- + # meaning THIS run's checks don't cover the code currently on this + # branch. Someone reviewing before merge needs to know whether that + # code was actually fully tested somewhere, and whether it was green. + # Walk backward from HEAD (bounded by the PR's base, or a generous + # depth cap for a direct push) to the newest commit whose OWN diff + # from its parent touched something other than docs -- every commit + # strictly after that one only changed docs, so that commit's CI run + # is still the authoritative test of the code on this branch now. + if: steps.diff.outputs.docs_only == 'true' + id: last_tested + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + REPO: ${{ github.repository }} + PR_BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: | + HEAD_SHA=$(git rev-parse HEAD) + if [ -n "$PR_BASE_SHA" ] && ! [[ "$PR_BASE_SHA" =~ ^0+$ ]]; then + BOUNDARY=$(git merge-base "$PR_BASE_SHA" "$HEAD_SHA") + else + # Not a PR (e.g. a direct push to master): no PR base to bound + # the walk against, so cap it at a generous depth instead of + # walking arbitrarily far into history. + BOUNDARY=$(git rev-list --max-count=1 --skip=100 "$HEAD_SHA" 2>/dev/null || echo "$HEAD_SHA") + fi + + commit="$HEAD_SHA" + last_code_commit="" + while [ "$commit" != "$BOUNDARY" ]; do + parent=$(git rev-parse "$commit^" 2>/dev/null) || break + changed=$(git diff --name-only "$parent" "$commit") + all_docs=true + while IFS= read -r f; do + [ -z "$f" ] && continue + if ! [[ "$f" =~ \.(md|asc)$ ]]; then all_docs=false; break; fi + done <<< "$changed" + if [ "$all_docs" = true ]; then + commit="$parent" + else + last_code_commit="$commit" + break + fi + done + + if [ -z "$last_code_commit" ]; then + # Reached the PR's own base (or the depth cap) without ever + # finding a commit that changed code: this whole PR/push chain + # is docs-only, so there is no "last tested" run to point to. + echo "docs_only_pr=true" >> "$GITHUB_OUTPUT" + echo "This entire PR/push contains only documentation changes -- no code testing applies." + exit 0 + fi + + echo "docs_only_pr=false" >> "$GITHUB_OUTPUT" + echo "last_tested_sha=$last_code_commit" >> "$GITHUB_OUTPUT" + echo "last code-changing commit: $last_code_commit" + + # Find that commit's most recent completed run of this same "CI" + # workflow (there can be other workflows -- e.g. Claude Code Review + # -- triggered on the same SHA, hence the name filter). + RUN_JSON=$(gh api "repos/$REPO/actions/runs?head_sha=$last_code_commit&status=completed" --jq \ + '[.workflow_runs[] | select(.name == "CI")] | sort_by(.run_started_at) | last // empty' 2>/dev/null || echo "") + if [ -z "$RUN_JSON" ] || [ "$RUN_JSON" = "null" ]; then + echo "last_tested_conclusion=unknown" >> "$GITHUB_OUTPUT" + echo "last_tested_url=" >> "$GITHUB_OUTPUT" + echo "No completed CI run found for $last_code_commit" + else + echo "last_tested_conclusion=$(jq -r '.conclusion' <<<"$RUN_JSON")" >> "$GITHUB_OUTPUT" + echo "last_tested_url=$(jq -r '.html_url' <<<"$RUN_JSON")" >> "$GITHUB_OUTPUT" + fi + - name: Derive the supported-PostgreSQL-major lists id: pg run: | @@ -805,4 +889,26 @@ jobs: echo "One or more jobs failed or were cancelled" exit 1 fi + - name: Summarize what this push actually tested + # This job's own checks report "skipped" for every heavy job on a + # docs-only push -- true for THIS push, but easy to mistake for "never + # tested" when just glancing at the PR's Checks tab. Make it explicit + # what's actually being vouched for. Reporting only -- does not affect + # pass/fail; see the "Check all jobs passed" step above for that gate. + run: | + if [ "${{ needs.changes.outputs.docs_only }}" != "true" ]; then + echo "✅ This push's own CI run tested the code (not a docs-only push)." >> "$GITHUB_STEP_SUMMARY" + elif [ "${{ needs.changes.outputs.docs_only_pr }}" = "true" ]; then + echo "📄 This entire PR/push contains only documentation changes -- no code testing applies." >> "$GITHUB_STEP_SUMMARY" + else + SHA="${{ needs.changes.outputs.last_tested_sha }}" + CONCLUSION="${{ needs.changes.outputs.last_tested_conclusion }}" + URL="${{ needs.changes.outputs.last_tested_url }}" + if [ "$CONCLUSION" = "success" ]; then ICON="✅"; else ICON="⚠️"; fi + if [ -n "$URL" ]; then + echo "$ICON This push only changed docs. The code was last fully tested at \`${SHA:0:7}\` -- [$CONCLUSION]($URL)" >> "$GITHUB_STEP_SUMMARY" + else + echo "⚠️ This push only changed docs, and no completed CI run was found for the last code-changing commit (\`${SHA:0:7}\`) -- verify manually." >> "$GITHUB_STEP_SUMMARY" + fi + fi # vi: expandtab ts=2 sw=2 From 0470ef11f5de63dad327e9af1dbff15f7cec90ff Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sat, 25 Jul 2026 18:12:48 -0500 Subject: [PATCH 10/13] docs: note the docs-only test-status reporting in CLAUDE.md Also serves as a real docs-only push to exercise the new backward-walk reporting logic end-to-end in CI for the first time. --- .github/workflows/CLAUDE.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.github/workflows/CLAUDE.md b/.github/workflows/CLAUDE.md index 17b6cfa..3c9d0c8 100644 --- a/.github/workflows/CLAUDE.md +++ b/.github/workflows/CLAUDE.md @@ -41,6 +41,16 @@ from: workflow's triggers are ever changed, re-verify that scoping is preserved; an unscoped `push:` trigger would double-run CI on every PR-branch commit. +## Docs-only pushes and "what was actually tested" + +A docs-only push makes the heavy jobs report "skipped" for THAT commit, which is correct (the +code didn't change) but easy to misread on the PR's Checks tab as "never tested." The `changes` +job's "Find the last commit where real code changed" step handles this: it walks backward +through history to the newest commit that actually changed code, looks up that commit's CI run, +and `all-checks-passed` reports it (link + green/red) in its step summary -- or explicitly notes +when the entire PR has never touched anything but docs. Don't assume "skipped" means "untested" +without checking that summary first. + ## Where CI-only shell logic lives `.github/scripts/` holds shell scripts that are **only** meaningful inside a CI job (e.g. From 1d64f4e7b76dafd5b93c2e06d42acf212e319074 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sat, 25 Jul 2026 18:18:13 -0500 Subject: [PATCH 11/13] ci: grant claude-review actions: write to fix cache-write warning The review job's setup step (inside claude-code-review@v1) can never save an Actions cache without this -- GitHub has no narrower cache-only permission scope, so every run was hitting "Cache reservation failed: cache write denied: token has no writable scopes" (a warning, not a failure; the review still ran and posted correctly every time). Granted on top of the existing trusted-fork-owner gate, not instead of it. --- .github/workflows/claude-code-review.yml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index 610f10d..0454576 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -40,6 +40,13 @@ jobs: contents: read pull-requests: write # post the review comments checks: read # read sibling check-runs for the cost gate + # GitHub has no narrower "cache write" scope -- actions: write is the + # only permission that lets a step save an Actions cache entry (it also + # grants cancelling/deleting workflow runs and managing artifacts, which + # this job doesn't use). Without it, claude-code-review's own setup step + # can never write a cache, only ever miss. Granted here on top of the + # existing trusted-fork-owner gate below, not instead of it. + 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. From b1697834d93bf21e0303ebcb7394b21025caaf58 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sat, 25 Jul 2026 20:39:19 -0500 Subject: [PATCH 12/13] ci: fix last-tested-commit lookup using the wrong HEAD under pull_request Confirmed hitting this in real CI: actions/checkout defaults to checking out an ephemeral merge commit for pull_request events (refs/pull/N/merge), not the PR's actual head commit. `git rev-parse HEAD` returned that merge commit's SHA, and walking `^` from it follows the wrong (base branch) first parent -- landing on an unrelated commit and correctly, but uselessly, reporting "no completed CI run found" for it. Use the actual PR head SHA (github.event.pull_request.head.sha) directly for pull_request events, github.sha otherwise -- the same event-type split "Compute per-push changed files" already uses for BASE/HEAD, which I should have followed here from the start instead of reaching for `git rev-parse HEAD`. --- .github/workflows/ci.yml | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9382d20..499862e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -113,8 +113,21 @@ jobs: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} REPO: ${{ github.repository }} PR_BASE_SHA: ${{ github.event.pull_request.base.sha }} + # actions/checkout defaults to checking out an EPHEMERAL MERGE + # COMMIT for pull_request events (refs/pull/N/merge), not the PR's + # real head commit -- `git rev-parse HEAD` would return that merge + # commit's SHA, and walking `^` from it follows the wrong (base + # branch) first parent, landing on an unrelated commit entirely + # (confirmed hitting exactly this in real CI). Use the actual PR + # head SHA directly instead, same event-type split already used in + # "Compute per-push changed files" above. + PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} run: | - HEAD_SHA=$(git rev-parse HEAD) + if [ "${{ github.event_name }}" = "pull_request" ]; then + HEAD_SHA="$PR_HEAD_SHA" + else + HEAD_SHA="${{ github.sha }}" + fi if [ -n "$PR_BASE_SHA" ] && ! [[ "$PR_BASE_SHA" =~ ^0+$ ]]; then BOUNDARY=$(git merge-base "$PR_BASE_SHA" "$HEAD_SHA") else From fc587f99110c1cd7f6f07b21146c81f778440b07 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sat, 25 Jul 2026 20:47:46 -0500 Subject: [PATCH 13/13] docs: note pull_request_target workflows run the base branch's file Also serves as a genuine docs-only push to verify the fixed backward-walk reporting logic end-to-end (the previous attempt used git rev-parse HEAD, which returns an ephemeral PR-merge-commit SHA under pull_request events -- fixed in the prior commit to use the real PR head SHA instead). --- .github/workflows/CLAUDE.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/.github/workflows/CLAUDE.md b/.github/workflows/CLAUDE.md index 3c9d0c8..37783ae 100644 --- a/.github/workflows/CLAUDE.md +++ b/.github/workflows/CLAUDE.md @@ -51,6 +51,15 @@ and `all-checks-passed` reports it (link + green/red) in its step summary -- or when the entire PR has never touched anything but docs. Don't assume "skipped" means "untested" without checking that summary first. +## `pull_request_target` workflows can't be tested from the PR that changes them + +GitHub always executes the workflow FILE for a `pull_request_target` event from the BASE +branch (master), never the PR's own version -- a deliberate security control so a fork PR can't +alter its own reviewer's permissions/behavior. `claude-code-review.yml` runs this way. Practical +consequence: a PR that changes that file (e.g. adjusting its `permissions:` block) cannot prove +the change took effect by watching that PR's own `claude-review` check -- it's still running +master's old version. Verify changes to that file only after merging to master, on the next PR. + ## Where CI-only shell logic lives `.github/scripts/` holds shell scripts that are **only** meaningful inside a CI job (e.g.