diff --git a/.claude/skills/release/SKILL.md b/.claude/skills/release/SKILL.md index 6310664..d864992 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. @@ -462,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 @@ -471,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 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*