From 3ac681c8770bf481adb24ae398a930cc5688535d Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Fri, 31 Jul 2026 17:37:58 -0500 Subject: [PATCH 1/2] release skill: own PGXS-target modifications, pin DEBUG exception, auto-sync preflight Three related improvements to the /release skill and its supporting definitions, found during 2.3.0 release prep: 1. Tighten the "User-Facing API Surface" known gap in CLAUDE.md: pgxntool's own modifications to PGXS-named targets (added/changed prerequisites, recipe bodies, EXTRA_CLEAN entries, etc.) are now explicitly in scope, rather than hiding behind "it's a PGXS target" because the old exclusion was by name alone. This has now caused two real misses (a stale README claim about test's prerequisites, and installcheck: install / issue #79 nearly being waved through during 2.3.0 prep) -- this settles the policy instead of leaving it as an open "revisit if it keeps happening" gap. 2. Make explicit that DEBUG's absence from README.asc/CLAUDE.md is an intentional, standing exception, not a finding -- API-documentation review agents must not re-flag it. This is the first entry in what's now a general pattern: intentional non-documentation decisions get recorded in the Terminology section so they aren't re-litigated release after release. SKILL.md's review-agent launch section now points at this list instead of duplicating it. 3. release-preflight.sh's sync check now self-heals instead of only warning: if local master is a clean fast-forward behind upstream, the script fast-forwards it (git merge --ff-only, never a plain merge) and pushes the result to origin (the fork). Master is only ever updated by fast-forward -- if --ff-only itself refuses, or local has commits upstream lacks, that's treated as a hard error requiring manual resolution, never auto-merged, rebased, or forced. Found during 2.3.0 prep, where both repos' local masters and forks had drifted behind upstream. SKILL.md's pre-flight section prose updated to match. Co-Authored-By: Claude --- .claude/skills/release/SKILL.md | 18 ++++- .../release/scripts/release-preflight.sh | 81 ++++++++++++++----- CLAUDE.md | 38 ++++++--- 3 files changed, 108 insertions(+), 29 deletions(-) diff --git a/.claude/skills/release/SKILL.md b/.claude/skills/release/SKILL.md index 6310664..1c63ce8 100644 --- a/.claude/skills/release/SKILL.md +++ b/.claude/skills/release/SKILL.md @@ -109,11 +109,21 @@ The script checks: 1. Upstream remotes exist (pointing to Postgres-Extensions) 2. Both working directories are clean 3. Both repos are on master -4. Local master is in sync with upstream +4. Local master is in sync with upstream — if local is simply behind (a + clean fast-forward), the script self-heals: it fast-forwards local + master to `upstream/master` (`git merge --ff-only`, never a plain merge + or rebase) and pushes the result to `origin` (the fork), no user + decision needed. Master is only ever updated by fast-forward — it must + never gain a merge commit. If a true fast-forward isn't possible for any + reason (including local having commits upstream lacks, or `--ff-only` + itself refusing), the script does not merge, rebase, or force anything; + it stops and reports a hard error requiring manual resolution. 5. Version format is valid and tag doesn't already exist 6. HISTORY.asc has a STABLE section **If the script exits with errors:** STOP and show the errors to the user. +This includes genuine master divergence from check 4 — that must be +resolved by hand, not auto-merged. **If there are warnings:** Show them and ask the user how to proceed. @@ -183,6 +193,12 @@ make any release-related change to git — until both sets of findings below have been retrieved and inspected.** See [Inspect API Documentation Review Findings](#inspect-api-documentation-review-findings). +Before launching, read (and fold into each agent's prompt) the "User-Facing +API Surface of pgxntool" section in `CLAUDE.md` — it records standing +exceptions (e.g. `DEBUG` being intentionally undocumented) that must not be +re-flagged as findings; don't duplicate that list here, just point agents at +it. + Launch two independent review efforts. Each may be one agent or a small set of agents if splitting the surface area (e.g. by file) makes sense; give every agent concrete file paths, not a vague "review the code" instruction. diff --git a/.claude/skills/release/scripts/release-preflight.sh b/.claude/skills/release/scripts/release-preflight.sh index a756327..bc134ba 100755 --- a/.claude/skills/release/scripts/release-preflight.sh +++ b/.claude/skills/release/scripts/release-preflight.sh @@ -94,33 +94,78 @@ echo "pgxntool-test: $pgxntool_test_branch" echo # 4. Fetch and check sync -echo "--- Sync Status ---" -if [ -n "$PGXNTOOL_UPSTREAM" ]; then - git -C "$PGXNTOOL_DIR" fetch "$PGXNTOOL_UPSTREAM" 2>/dev/null - local_head=$(git -C "$PGXNTOOL_DIR" rev-parse HEAD) - upstream_head=$(git -C "$PGXNTOOL_DIR" rev-parse "$PGXNTOOL_UPSTREAM/master" 2>/dev/null || echo "unknown") +# +# If local master is simply behind upstream (a clean fast-forward is +# possible - local has no commits upstream lacks), self-heal: fast-forward +# local master to upstream/master and push the result to the fork remote +# (origin), rather than just warning and leaving it to the user. Genuine +# divergence (local has commits upstream doesn't have) is NOT auto-merged +# or force-pushed - that's left as a hard error requiring manual resolution, +# since silently merging/rebasing here could be lossy. +sync_repo() { + local repo_path="$1" + local repo_label="$2" + local upstream_remote="$3" + local branch="$4" + + local local_head upstream_head + local_head=$(git -C "$repo_path" rev-parse HEAD) + upstream_head=$(git -C "$repo_path" rev-parse "$upstream_remote/master" 2>/dev/null || echo "unknown") + if [ "$local_head" = "$upstream_head" ]; then - echo "pgxntool: in sync with $PGXNTOOL_UPSTREAM/master ($local_head)" - else - echo "pgxntool: DIVERGED from $PGXNTOOL_UPSTREAM/master" + echo "$repo_label: in sync with $upstream_remote/master ($local_head)" + return + fi + + if [ "$branch" != "master" ]; then + echo "$repo_label: DIVERGED from $upstream_remote/master" echo " local: $local_head" echo " upstream: $upstream_head" - warnings+=("pgxntool: local master diverges from $PGXNTOOL_UPSTREAM/master") + echo " not on master ('$branch') - skipping auto-sync" + errors+=("$repo_label: local master diverges from $upstream_remote/master and current branch is not master") + return fi -fi -if [ -n "$PGXNTOOL_TEST_UPSTREAM" ]; then - git -C "$PGXNTOOL_TEST_DIR" fetch "$PGXNTOOL_TEST_UPSTREAM" 2>/dev/null - local_head=$(git -C "$PGXNTOOL_TEST_DIR" rev-parse HEAD) - upstream_head=$(git -C "$PGXNTOOL_TEST_DIR" rev-parse "$PGXNTOOL_TEST_UPSTREAM/master" 2>/dev/null || echo "unknown") - if [ "$local_head" = "$upstream_head" ]; then - echo "pgxntool-test: in sync with $PGXNTOOL_TEST_UPSTREAM/master ($local_head)" + # Is local a strict ancestor of upstream (i.e. purely behind, with + # nothing of its own ahead)? Guarded with if/else so a non-zero exit + # from --is-ancestor (the expected "not an ancestor" case) doesn't trip + # set -e. + if git -C "$repo_path" merge-base --is-ancestor "$local_head" "$upstream_head"; then + echo "$repo_label: BEHIND $upstream_remote/master - fast-forwarding" + echo " local: $local_head" + echo " upstream: $upstream_head" + # NEVER use a plain merge here -- master must only ever fast-forward, + # never gain a merge commit. --ff-only makes git refuse (non-zero + # exit) instead of creating a merge commit if a true fast-forward + # somehow isn't possible (e.g. a race with a concurrent push landing + # between the ancestor check above and this merge). Guarded with + # if/else, not `set -e`, so that refusal is caught and reported as + # the same divergence error below -- never retried with a different + # strategy (rebase, plain merge, force-push) and never forced. + if git -C "$repo_path" merge --ff-only "$upstream_remote/master"; then + git -C "$repo_path" push origin master + echo "$repo_label: fast-forwarded master $local_head -> $upstream_head and pushed to origin" + else + echo "$repo_label: fast-forward merge unexpectedly failed" + errors+=("$repo_label: git merge --ff-only failed even though local was expected to be a strict ancestor of $upstream_remote/master - resolve manually") + fi else - echo "pgxntool-test: DIVERGED from $PGXNTOOL_TEST_UPSTREAM/master" + echo "$repo_label: DIVERGED from $upstream_remote/master (local has commits upstream lacks)" echo " local: $local_head" echo " upstream: $upstream_head" - warnings+=("pgxntool-test: local master diverges from $PGXNTOOL_TEST_UPSTREAM/master") + errors+=("$repo_label: local master has diverged from $upstream_remote/master (not a clean fast-forward) - resolve manually") fi +} + +echo "--- Sync Status ---" +if [ -n "$PGXNTOOL_UPSTREAM" ]; then + git -C "$PGXNTOOL_DIR" fetch "$PGXNTOOL_UPSTREAM" 2>/dev/null + sync_repo "$PGXNTOOL_DIR" "pgxntool" "$PGXNTOOL_UPSTREAM" "$pgxntool_branch" +fi + +if [ -n "$PGXNTOOL_TEST_UPSTREAM" ]; then + git -C "$PGXNTOOL_TEST_DIR" fetch "$PGXNTOOL_TEST_UPSTREAM" 2>/dev/null + sync_repo "$PGXNTOOL_TEST_DIR" "pgxntool-test" "$PGXNTOOL_TEST_UPSTREAM" "$pgxntool_test_branch" fi echo diff --git a/CLAUDE.md b/CLAUDE.md index a8d7885..cc376fe 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -178,8 +178,21 @@ section once resolved. 1. **Make targets pgxntool defines**, including dev-helper targets like `list` and `print-%` (they exist specifically to help users introspect the Makefile). Excludes: - - Targets pgxntool inherits from PGXS unmodified (`install`, - `installcheck`, `submake-*`, etc.). + - Targets pgxntool inherits from PGXS **unmodified** (`install`, + `submake-*`, etc.) — but only to the extent they're genuinely + unmodified. The moment pgxntool adds or changes a prerequisite, + recipe body, or variable (e.g. `EXTRA_CLEAN`) on a PGXS-named target, + that modification is IN SCOPE and must be tracked like any other API + surface item — the underlying PGXS behavior pgxntool didn't touch + stays out of scope, but pgxntool's own authored changes to a + shared-name target don't get to hide behind "it's a PGXS target." + This is settled policy: it has already caused two real misses when + treated as excluded-by-name-alone — a stale README claim about + `test`'s prerequisites, and `installcheck: install` (issue #79, a + genuine ordering-bug fix to the PGXS-named `installcheck` target) + nearly being waved through as "out of scope" during 2.3.0 release + prep. Both should have been (and now are) tracked the same as any + other API surface change. - Pure generated-file targets (`META.json`, `meta.mk`, `control.mk`) — build plumbing, not something a user intentionally runs. - Conditionally-defined helper targets that exist purely to support @@ -189,15 +202,10 @@ section once resolved. primary target they support (e.g. `test-build` itself) remains in scope if it's meant to be invoked directly and is independently documented. - - **Known gap**: pgxntool's own modifications to shared-name PGXS - targets (e.g. `test`'s prerequisites, `clean`'s `EXTRA_CLEAN` - additions) are currently excluded along with the rest of that - target's PGXS lineage, since the exclusion is by name alone. This has - already hidden real drift once (a stale README claim about `test`'s - prerequisites) — revisit if it keeps happening. - Finding these requires reading the actual source, not just running - `make list` — conditionally-gated targets, and anything else a - discovery tool can't fully enumerate, only show up by inspecting + `make list` — conditionally-gated targets, pgxntool's own additions + to PGXS-named targets, and anything else a discovery tool can't + fully enumerate, only show up by inspecting `base.mk`/`control.mk.sh`/`meta.mk.sh` directly. 2. **Target prerequisites worth documenting by name** even when not invoked directly (e.g. `testdeps`), since extension authors may @@ -220,6 +228,16 @@ section once resolved. fine for users to know it exists), but the specific level numbers are an internal implementation detail, not a documented contract — changing them is not a behavior change that needs a `HISTORY.asc` entry. + `DEBUG`'s absence from `README.asc`/`CLAUDE.md` documentation is NOT + itself a finding — API-documentation review agents (used by the + `/release` skill, see `.claude/skills/release/SKILL.md`) must not flag + "`DEBUG` exists but isn't documented" as a gap, regardless of how many + scripts reference it. This is the first entry in what should be treated + as a general pattern: when a documentation gap is judged intentional + rather than an oversight, record that decision here as a standing + exception so later reviews don't re-flag and re-litigate the same + question release after release — add new entries to this list rather + than raising them fresh each time. 6. **`../pgxntool/CLAUDE.md` is in scope, not exempt as "doc-only."** Unlike ordinary dev-only documentation, this file ships into every consumer project via subtree and is written for AI agents working in *those* From 798cbf78b0e0885670dc2729ae8f77f241e6956c Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Fri, 31 Jul 2026 17:44:12 -0500 Subject: [PATCH 2/2] release skill: fix ambiguous PR-body wording that caused a real mix-up pgxntool's release PR body read "Companion (do-not-merge, CI trigger only): pgxntool-test release-VERSION" -- the do-not-merge parenthetical described the companion PR, but read in isolation it looked like it applied to the PR itself. During 2.3.0 this caused the reviewer to mistake the real release PR for a do-not-merge one. Both PR body templates now state plainly, as their own sentence, whether that PR should or shouldn't be merged, before ever mentioning the other one, and cross-reference each other's PR number. --- .claude/skills/release/SKILL.md | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/.claude/skills/release/SKILL.md b/.claude/skills/release/SKILL.md index 1c63ce8..d864992 100644 --- a/.claude/skills/release/SKILL.md +++ b/.claude/skills/release/SKILL.md @@ -478,7 +478,10 @@ gh pr create --repo Postgres-Extensions/pgxntool-test \ pgxntool release-VERSION branch -- pgxntool's own release PR is doc-only \ and skips the Postgres test matrix entirely. It changes nothing in \ pgxntool-test (empty commit) and must NOT be merged. Close it once its CI \ -is green." +is green. + +Companion PR (should be merged normally once CI is green): \ +Postgres-Extensions/pgxntool#" ``` ```bash @@ -487,9 +490,25 @@ git push PGXNTOOL_UPSTREAM release-VERSION gh pr create --repo Postgres-Extensions/pgxntool \ --base master --head release-VERSION \ --title "Release VERSION" \ - --body "Stamps HISTORY.asc for VERSION. Companion (do-not-merge, CI trigger only): pgxntool-test release-VERSION." + --body "Stamps HISTORY.asc for VERSION. This PR should be merged normally. + +Companion PR (must NOT be merged -- exists only to trigger a real CI test \ +run): Postgres-Extensions/pgxntool-test#" ``` +**Why the wording matters:** an earlier release's pgxntool PR body read +"Companion (do-not-merge, CI trigger only): pgxntool-test release-VERSION" +-- a single run-on sentence where the do-not-merge parenthetical, read out +of context, could be misread as applying to the PR you're looking at +instead of its companion. State plainly, as its own sentence, whether +*this* PR should or shouldn't be merged before ever mentioning the other +one. Backfill the actual PR numbers into `` / +`` once both PRs exist (each is opened after the +other in the commands above, so the second `gh pr create` can reference +the first PR's already-known number; if opening both concurrently via +parallel subagents, edit the missing cross-reference in with `gh pr edit +--body` immediately after both numbers are known). + Per this project's CI-monitoring rule, immediately start a background monitor for each push (exact SHA, not `--branch`, to avoid a race with any other concurrent push on this branch name -- one monitor per repo, not run