CI: binary pg_upgrade testing, docs-only gate, single-source PG matrix

[jnasbyupgrade]

Rebase note + follow-up fixes

Rebased onto the updated advanced-testing/foundation (this stack is sync-pgxntool-2.3.0 -> add-test-build (#26) -> advanced-testing/foundation (#22) -> this PR; coordinator is handling gh stack link separately). advanced-testing/ci had a merge commit bringing in the OLD advanced-testing/foundation history directly, so a plain git rebase tried to replay that entire flattened history against the new base -- used git rebase --onto <new-foundation> <old-foundation-tip> instead, to replay only this branch's own unique commits.

Real conflicts requiring judgment (not mechanical): .github/workflows/ci.yml (this PR's changes/pg-upgrade-test/all-checks-passed structure vs. the now-simplified lint+test structure inherited from #26/the pgxntool 2.3.0 sync -- merged both), test/expected/pgtap.out (this PR's ORDER BY fix for test/helpers/create.sql's security-definer-function check reorders that output; regenerated the fresh-mode expected file from a real run once the conflict was resolved -- pgtap_1.out, the existing-mode alternate, already had the correct sorted order and needed no change), and the same test/sql/install.sql/pgtap.sql mode-gating reconciliation #22 already needed against #26's trim (nothing new here, just replayed).

Fixes applied per this round of review, all verified locally before pushing

(a) pg-upgrade-test now gates on test, plus added concurrency. Previously needs: [changes] only, so a trivially-broken PR still burned the full binary pg_upgrade matrix. Now needs: [changes, test]. Added a top-level concurrency: {group: "${{ github.workflow }}-${{ github.ref }}", cancel-in-progress: true} so a superseded push cancels an in-flight run. all-checks-passed's own needs/self-check invariant is unaffected (job set didn't change shape).

(b) bin/test_existing's run_suite() was running the full suite twice. make test followed immediately by make verify-results -- verify-results already depends on test (pgxntool's base.mk), so every pg-upgrade-test leg paid for two full pg_regress --use-existing runs against the real migrated database. Dropped the redundant make test call. Verified this is still correct post-pgxntool-2.3.0 by reproducing a REAL pg_upgrade leg end to end locally (16 -> 17 at the time, using throwaway pg_createcluster data directories on custom ports -- never touching this container's shared clusters): recreated the old cluster with data checksums, installed pgtap + test_factory, prepared it via bin/test_existing prepare-old, created the new cluster, installed pgtap + test_factory there too, ran the actual pg_upgrade binary, started the new cluster, then ran bin/test_existing run-suite against it -- all 3 tests passed with the deduplicated run_suite().

(c) actions/checkout@v4 -> @v7 (current latest major -- v5/v6 have both released since v4), all 5 occurrences (lint, changes, test, pg-upgrade-test, all-checks-passed -- one more than the original "4" since the lint job wasn't part of this branch when that count was written; it arrived via the pgxntool 2.3.0 sync / #26 stacked below). Coordination note: PR #29 (ci/bump-actions-versions, someone else's, out of scope for me to touch) also bumps this same pin elsewhere in this file for the original, not-yet-restructured job set -- whichever of these two efforts merges second will need a final grep -rn 'actions/checkout@v4' sweep to make sure nothing was missed.

(d) NEWEST bumped 17 -> 18. The advanced-extension-testing doc's reference implementation (cat_tools) is already on NEWEST=18. Rather than assume PG18 installs cleanly in the pgxn/pgxn-tools image, confirmed it via an actual CI run first (pushed the NEWEST=18 bump alone, watched the new "PostgreSQL 18" job) -- it installed postgresql-18 18.4-1.pgdg13+1 via pg-start and the fresh-install suite passed. With 18 now the newest major, also shifted pg-upgrade-test's two legs to match its own stated rationale (widest catalog distance; most likely to hit a new major's catalog change first): oldest-to-newest is now 10 -> 18 (was 10 -> 17), and the newest-boundary leg is now 17 -> 18 (was 16 -> 17).

Verification after all fixes

• make lint and make verify-results (fresh mode) pass locally.
• bash -n on both shell scripts, YAML parse check on ci.yml.
• A full local binary pg_upgrade leg (see (b) above) passes with the deduplicated script.
• Pushed to this PR and watched: changes, all test matrix legs (10-18), pg-upgrade-test's two legs (10->18, 17->18), and all-checks-passed.

---

Summary

PR 2 of the advanced update+upgrade (U&U) testing stack, on top of #22
(advanced-testing/foundation, not yet merged -- this PR targets that
branch, not master). Implements the remaining CI-structure items from the
advanced-extension-testing pattern, modeled on Postgres-Extensions/cat_tools's
actual ci.yml/bin/test_existing (read directly off its master, not just
prose about it) and scoped down deliberately per the plan agreed before
starting.

Note: advanced-testing/foundation was pushed to this repo (unchanged, same
commit as the fork's branch backing #22) purely so this PR's base could
reference it directly -- it is not new work, just a ref needed for a clean
stacked diff. Once #22 merges, this PR should be retargeted to master as a
follow-up (not done here).

What shipped

• Fixed the double-triggered-CI bug: push scoped to branches: [master]; pull_request stays unrestricted. test_factory was explicitly
  named as one of the repos still needing this.
• Job-level changes/docs-only gate instead of workflow-level
  paths-ignore -- computes the real per-push diff, fail-safe (docs_only=false)
  written as the literal first line so any early exit/error leaves the safe
  default in place. Avoids the stuck-Pending-required-check trap a
  workflow-level paths-ignore would cause.
• Single source of truth for the supported-PostgreSQL-major list
  (NEWEST=18, FLOOR=10), derived
  once in the changes job and consumed via fromJSON by both the test
  and new pg-upgrade-test matrices.
• all-checks-passed gate, self-verifying its own needs: list matches
  the actual job set. This is the check to wire up as required in branch
  protection -- I don't have permission to change that setting myself.
• New pg-upgrade-test job: binary pg_upgrade legs (10 -> 18,
  17 -> 18, updated from the original 10 -> 17/16 -> 17 once NEWEST was bumped -- see the rebase note above) -- install the current version on an old cluster, pg_upgrade
  to a newer major, run the suite against the migrated objects in existing
  mode. No bridge step: test_factory has shipped only one version (0.5.0),
  so every leg installs current directly on the old cluster -- there's no
  older, pg_upgrade-unsafe install to carry forward.
• bin/test_existing (new, test_factory-specific) and
  .github/scripts/pg_upgrade_cluster (new, generic pg_upgrade CI
  mechanics) -- the install -> pg_upgrade -> assert -> run-suite flow
  factored into committed scripts instead of inline YAML per job, per the
  doc's own guidance. bin/test_existing is much smaller than cat_tools's
  own: no bridge/multi-origin subcommands, just prepare-old + run-suite.
• test job now gates on make verify-results, not pgxn-tools'
  pg-build-test. Found while rewriting this job: pg-build-test's own
  make installcheck || status=$? never actually catches a failure, because
  pgxntool marks installcheck .IGNORE -- make itself exits 0 there
  regardless of regression.diffs. The old CI was silently green on a real
  regression. PGXNTOOL_ENABLE_VERIFY_RESULTS was already pgxntool's own
  default, but I pinned it explicitly in the Makefile (matching
  ENABLE_TEST_INSTALL's existing explicit-over-implicit convention) so a
  future pgxntool default change can't silently disable the gate.
• Dynamic version assertion: bin/test_existing's assert_version
  derives the expected version from make -s print-PGXNVERSION, never
  hardcoded, with empty-value guards on both sides ("" != "" is false, so a
  broken extraction can't silently pass).

A real bug the local pg_upgrade dry run actually found

Per the brief, I ran the full old-cluster-install -> pg_upgrade ->
new-cluster-suite-run cycle locally before trusting any of this in CI (PG12
-> PG17 and PG12 -> PG16, using throwaway initdb data directories, never
touching the container's shared clusters) -- and it surfaced a real, if
low-stakes, bug: test/helpers/create.sql's security-definer-function check
had no ORDER BY, so its row order depended on pg_proc's physical layout.
That happens to match creation order on a fresh CREATE EXTENSION but is
not preserved by pg_upgrade's dump/restore (which reconstructs it in a
different, apparently name-sorted, order) -- producing a real but harmless
text diff (every individual pgTAP assertion still said ok, just reordered)
against the fresh-install expected output. Fixed with an explicit
ORDER BY p.oid::regproc::text.

Bonus: this makes ONE set of expected-output files valid for fresh,
existing, and pg_upgraded modes alike -- no third numbered alternate file
needed (unlike the doc's more general guidance for genuinely different
axes like TEST_SCHEMA), since the divergence here was pure incidental
non-determinism, not a legitimate different-but-correct scenario. Simpler
than it first looked once actually run.

Convergence / divergence from cat_tools PR #16 (and its master since)

Took near-verbatim (generic CI mechanics, no cat_tools-specific
content): the shape of .github/scripts/pg_upgrade_cluster
(recreate-old/upgrade subcommands, INITDB_OPTS convention, pg_upgrade
log capture for both PG17+'s pg_upgrade_output.d/ and older's CWD), the
changes job's docs-only fail-safe-first-line pattern, the
all-checks-passed self-verifying needs check, and the dynamic
version-assertion empty-guard pattern.

Adapted: bin/test_existing keeps only two subcommands
(prepare-old/run-suite) instead of cat_tools's six -- no
plant-guard/update/update-scenario/update-check, because
test_factory's dependency guard is planted and proved entirely inside
test/install/load.sql's existing-mode branch (from PR #22), not by this
script, and there's no update path to exercise yet. The changes job also
drops cat_tools's "find the last commit where real code changed" reporting
machinery -- useful polish, but not part of the checklist items this PR
scoped to; noting it here as a real simplification, not an oversight, in
case a human wants it added later.

Skipped, and why (all decided before starting, confirmed still correct
after doing the work):

• extension-update-test job -- test_factory has shipped only one version
  (0.5.0), so TEST_LOAD_SOURCE=update has no real historical update script
  to exercise yet. The mechanism exists (PR Add test/install foundation for update+upgrade testing (fresh/update/existing) #22); no CI job drives it.
• Bridge/multi-origin update machinery -- cat_tools-specific technical debt
  (recovering from old pg_upgrade-unsafe releases). test_factory has no
  catalog-touching views and only one shipped version, so there's nothing to
  bridge from.
• pg-upgrade-stepwise (every-major climb) -- test_factory has no
  catalog-internals-touching views/functions (no SELECT * over a system
  catalog), so the per-major-boundary risk this protects against is low. A
  human may disagree and ask for it later; flagging explicitly rather than
  silently omitting.
• pg_tle testing -- no evidence test_factory targets pg_tle/RDS/Aurora
  deployment.
• The stable pseudo-version -- real feature work, not part of a
  testing-infrastructure PR, not requested.

Verification

• make verify-results passes cleanly (fresh mode) on both PG12 and PG17.
• A full old-cluster-install -> pg_upgrade -> new-cluster-suite-run cycle,
  using the actual committed bin/test_existing and
  .github/scripts/pg_upgrade_cluster (not just ad hoc commands), passes
  with zero raw not ok TAP lines -- run twice, PG12->PG17 and PG12->PG16,
  in scratch data directories that never touched the container's shared
  clusters.
• .github/workflows/ci.yml parses cleanly under PyYAML; both new shell
  scripts pass bash -n.

Test plan

• CI runs green on this PR (test + pg-upgrade-test + all-checks-passed)
• Once Add test/install foundation for update+upgrade testing (fresh/update/existing) #22 merges, retarget this PR to master (follow-up, not done here)
• A repo admin adds all-checks-passed as a required status check in
  branch protection

🤖 Generated with Claude Code

---

Recreated from #23 (fork-headed) as an upstream-branch PR so it can be part of a formal GitHub stack (gh stack link), which refuses fork PRs. Same content/commits, same CI results.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 0a31cb3f-b0ad-4504-bc2f-0c3e823e59d0

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[jnasbyupgrade]

Added a commit fixing claude-review's fork-PR checkout refusal (allow-unsafe-pr-checkout: true on claude-code-review.yml, safe here since the trust gate + read-only handling are both already in place), strengthened the trust-gate's own warning comment, and bumped actions/checkout@v4→v7 on both claude-code-review.yml and claude.yml.

Note on verification: this PR's own claude-review check can't demonstrate the fix -- pull_request_target always runs the workflow from the PR's base branch, and separately, this whole stack is upstream-headed (not fork-headed), so the trust gate itself evaluates false and the job shows "skipping" regardless. Real verification happens on the next fork-headed PR (the normal pattern for this repo) based on a branch that includes this commit.

[jnasbyupgrade]

Correction to my earlier fix: the allow-unsafe-pr-checkout: true approach above was solving the wrong problem. Found via Postgres-Extensions/extension_tools#28, which hit and fixed the identical mistake: claude-code-action fetches and reads a PR's content itself internally (src/github/operations/branch.ts: for a fork PR it fetches origin's refs/pull/<n>/head, a ref GitHub maintains on the BASE repo for any PR). Redirecting the checkout step's origin to the fork (via repository:/ref:) breaks that internal fetch, since refs/pull/<n>/head doesn't exist on the fork's own remote -- would have failed with fatal: couldn't find remote ref pull/<n>/head on the next real fork-headed PR, trading one broken check for another.

Fixed: removed the repository:/ref:/allow-unsafe-pr-checkout overrides entirely. Just uses: actions/checkout@v7 with no inputs -- checks out this repo's own base branch, matching claude-code-action's own documented preferred pattern (its docs/security.md names the fork-ref-into-workspace pattern as the anti-pattern to avoid). The trust-gate if: is unchanged and still real defense-in-depth, even though the checkout itself is now safe by construction regardless.

Same self-verification limitation applies -- can't demonstrate this on this PR's own check.

[jnasbyupgrade]

Two more gotchas from the updated ~/security-notice.md handoff doc (Postgres-Extensions/cat_tools PR #62 and #47), both silent failure modes that don't affect a job's pass/fail status:

1. Inline comments silently dropped: claude-code-review.yml drives claude-code-action via a bare prompt: (no @claude mention) = "agent mode", which decides which MCP servers to start from --allowedTools inside claude_args -- NOT from the invoked plugin's own allowed-tools frontmatter. Without claude_args: '--allowedTools mcp__github_inline_comment__create_inline_comment', the inline-comment tool never starts at all, so every review before this fix silently fell back to one consolidated PR comment instead of real per-line inline comments.
2. Silent cache-write failures: neither claude-code-review.yml nor claude.yml's permissions: block had actions: write (both had read or nothing). 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...), a warning not a hard failure. Fixed both files (claude.yml's existing actions: read upgraded to write, which still covers its original "read CI results" purpose).

Same self-verification limitation as before -- neither is visible from this PR's own claude-review check.