From b458774acc8c086af3802fdfc72dc368ceaf4195 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sat, 25 Jul 2026 17:38:19 -0500 Subject: [PATCH 1/2] Fix claude-review's trust gate: allow same-repo PRs, not just the old fork The gate checked github.event.pull_request.head.repo.owner.login == 'jnasbyupgrade', written back when PRs came from that personal fork. Now that the convention is to push branches directly to this repo instead (never the fork), every PR's head repo owner is Postgres-Extensions, not jnasbyupgrade - so the condition never matched and claude-review silently skipped on every single PR this session (#12, #13, #14 all show it as skipped). Replaced with head.repo.full_name == github.repository, which is actually a tighter trust boundary than the old check: it only allows PRs whose head branch lives in this exact repo, requiring push access here, rather than naming one specific external fork as trusted. --- .github/workflows/claude-code-review.yml | 25 +++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index fc7e26c..090b42d 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -1,16 +1,19 @@ name: Claude Code Review # Runs on PRs INTO this repo. We use pull_request_target (not pull_request) so -# that PRs from a fork can access CLAUDE_CODE_OAUTH_TOKEN — GitHub withholds -# secrets from `pull_request` runs triggered by forks, which is why the plain -# `pull_request` version never worked for fork PRs. +# that the workflow definition always comes from the base branch, never from +# the PR's own head - a plain `pull_request` run uses whatever workflow file +# is on the PR branch itself, which a same-repo feature branch could modify to +# steal secrets before this ever runs. # # 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 whose head branch lives in THIS +# repo (not an external fork) — that requires push access to this repo in the +# first place, so 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. on: pull_request_target: types: [opened, synchronize, reopened, ready_for_review] @@ -21,11 +24,11 @@ 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. + # Same-repo PRs only (never an external fork), and skip drafts (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.head.repo.full_name == github.repository runs-on: ubuntu-latest timeout-minutes: 60 permissions: From d89b46162b4e5159d417a60e526140634a3fe845 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sat, 25 Jul 2026 17:47:03 -0500 Subject: [PATCH 2/2] Add pg-tle-test CI job: smoke-test count_nulls deployed purely through pg_tle Ports the smoke-test portion of cat_tools' pg-tle-test job (Postgres-Extensions/cat_tools PR #47) so count_nulls gets the same coverage: build pg_tle 1.5.2 from source, register the extension against template1 via pg_tle instead of a filesystem .control file, and verify with bin/assert_fs_clean (copied over unchanged - it's already extension-agnostic) that nothing ever lands on disk while a fresh CREATE EXTENSION count_nulls in a smoke-test db resolves and null_count() actually works. Matrix is [17, 16, 15, 14, 13, 12] for this job specifically: count_nulls' own supported range is 10-17, but pg_tle 1.5.2 only supports 12+ (pgxntool/pgtle_versions.md), so 10 and 11 are dropped here. Deliberately NOT porting cat_tools' update-path portion (TEST_EXISTING_DEPLOY=pgtle bin/test_existing update-scenario ...): count_nulls' master has no bin/test_existing at all - that infrastructure only exists on the still-unmerged extension-update-test/pgxntool-update branches. Once that lands, the update-path half of this job can be added the same way cat_tools did it. --- .github/workflows/ci.yml | 95 ++++++++++++++++++++++++++++++++++++++++ bin/assert_fs_clean | 71 ++++++++++++++++++++++++++++++ 2 files changed, 166 insertions(+) create mode 100755 bin/assert_fs_clean diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 23bacab..1b98f33 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -19,3 +19,98 @@ jobs: uses: actions/checkout@v4 - name: Test on PostgreSQL ${{ matrix.pg }} run: pg-build-test + + pg-tle-test: + strategy: + matrix: + # Intersection of count_nulls' own supported range (10-17, see the + # `test` job above) with pg_tle 1.5.2's supported range (12-18, see + # pgxntool/pgtle_versions.md): drop 10 and 11 since pg_tle doesn't + # support them. + pg: [17, 16, 15, 14, 13, 12] + 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@v4 + - 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. 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 count_nulls -- 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). + 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: Register pg_tle + count_nulls 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 count_nulls database used below (the + # smoke-test db) 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 count_nulls-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. 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 count_nulls purely via pg_tle (fresh install, no filesystem trace) + # count_nulls 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 + # 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/count_nulls.control + createdb count_nulls_smoke + psql -d count_nulls_smoke -c "CREATE EXTENSION count_nulls" + - name: Verify count_nulls works when deployed via pg_tle + run: | + INSTALLED=$(psql -d count_nulls_smoke -tAc "SELECT extversion FROM pg_extension WHERE extname = 'count_nulls'") + 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 + RESULT=$(psql -d count_nulls_smoke -v ON_ERROR_STOP=1 -tAc "SELECT null_count(1, NULL, 2)") + echo "null_count(1, NULL, 2)=$RESULT" + if [ "$RESULT" != "1" ]; then + echo "FAIL: expected null_count(1, NULL, 2) = 1, got '$RESULT'"; exit 1 + fi + - 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 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 "$@"