From b1502e9d4bab95538308a7a05a1541d544324c70 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 17 Sep 2026 15:33:37 -0500 Subject: [PATCH 1/3] Migrate to shared claude-code-review.yml from Postgres-Extensions/ai MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the hand-maintained review workflow with a thin caller into Postgres-Extensions/ai's reusable `workflow_call` version, so a future fix lands here without a manual copy. pgxntool-test is the permanent canary for that shared workflow, pinned to `@main` rather than `@v1`, so a change runs here for real before the `v1` tag protecting the other consuming repos is ever moved to include it. This also picks up the `--comment` fix for the silent-review-failure bug that the old per-repo copy still carried. **This PR cannot be fully validated by its own CI.** A `pull_request_target` workflow change only takes effect after merging to the base branch, and even then it's only exercised by a *subsequent* PR event against this repo — this PR's own CI run still uses the old workflow file. What's verified here is structural correctness only: the YAML parses, `Postgres-Extensions/ai/.github/workflows/claude-code-review.yml@main` resolves (`gh api repos/Postgres-Extensions/ai/contents/.github/workflows/claude-code-review.yml?ref=main` returns the file), the repo's default workflow permissions (`read`) and secret availability match what every other caller relies on via `secrets: inherit`, and `git merge-tree` against `upstream/master` is clean. Once merged, the real proof is the next PR opened against this repo. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/claude-code-review.yml | 152 +++++------------------ 1 file changed, 31 insertions(+), 121 deletions(-) diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index b37f868..4f3f6fd 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -1,137 +1,47 @@ 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. +# Thin caller. All logic lives in Postgres-Extensions/ai; read that file for +# the SECURITY rationale behind pull_request_target + the trusted-author gate. +# Everything below is the minimum GitHub requires to live in THIS repo: +# - the trigger (a called workflow cannot declare its own) +# - run-level concurrency (only a workflow-level `concurrency:` can cancel +# the whole caller run outright; `jobs..concurrency` on the caller job +# itself can't, and the per-label cancellation logic below needs exactly +# that) +# - the GITHUB_TOKEN ceiling (a called workflow can only narrow it, never widen) +# Do not add logic here. If this repo needs different behavior, change ai/ so +# every repo gets it. # -# 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 never check out the fork's PR -# head: GitHub Actions refuses that combination by default (the "pwn request" -# guard — see actions/checkout's allow-unsafe-pr-checkout), and -# anthropics/claude-code-action's own docs (docs/security.md) recommend -# checking out the base ref and letting the action read PR content via the -# GitHub API instead. The code-review prompt passes the PR number; the action -# has a GitHub token and pull-requests read/write, so it fetches the diff -# itself (e.g. `gh pr diff`) without ever writing fork code to disk. +# Pinned to @main, not @v1: this repo is the permanent canary for +# claude-code-review.yml. A change to that file runs here for real before the +# v1 tag protecting every other consuming repo is ever moved to include it. on: pull_request_target: - types: [opened, synchronize, reopened, ready_for_review] + # `labeled` lets adding the claude-debug label start a run on its own, with + # no push needed. Scoped in ai/'s job `if:` so only that label proceeds. + types: [opened, synchronize, reopened, ready_for_review, labeled] concurrency: - group: claude-review-${{ github.event.pull_request.number }} + # A non-debug `labeled` event gets its own per-label group so it can never + # cancel an in-progress real review: cancellation resolves when a run is + # admitted, before any `if:` is evaluated, so an `if:` can only no-op itself, + # not un-cancel what it displaced. labeled+claude-debug deliberately keeps the + # plain group -- it is meant to supersede a running review. + # 'claude-debug' is spelled out because `inputs` is not readable here; it must + # match ai/'s debug_label default. + group: claude-review-${{ github.event.pull_request.number }}${{ (github.event.action == 'labeled' && github.event.label.name != 'claude-debug') && format('-{0}', github.event.label.name) || '' }} cancel-in-progress: true jobs: claude-review: - # Trusted author only, and skip drafts (don't spend API/CI on unfinished PRs). - # To add more trusted authors, extend the author check. - if: >- - github.event.pull_request.draft == false && - github.event.pull_request.user.login == 'jnasbyupgrade' - runs-on: ubuntu-latest - timeout-minutes: 60 + uses: Postgres-Extensions/ai/.github/workflows/claude-code-review.yml@main permissions: contents: read pull-requests: write # post the review comments checks: read # read sibling check-runs for the cost gate - # write (not just read) needed so claude-code-action's internal - # bun-setup step can save its cache; read-only causes a harmless but - # noisy "Cache reservation failed: cache write denied: token has no - # writable scopes" warning. + # actions: write is the only scope that permits an Actions cache write + # (no narrower one exists). Don't "tighten" this to read. 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. - # If any sibling check failed we skip the review to avoid spending money - # reviewing a PR that is already known-broken. Uniform across all repos: - # it discovers sibling checks dynamically (no per-repo workflow names). - # - decision=run : all sibling checks completed with a good conclusion, - # OR no sibling checks exist after a short grace window - # (nothing to gate on), OR the poll timed out is treated - # as skip (see below). - # - decision=skip : at least one sibling check failed/cancelled/etc, or - # we timed out waiting for still-pending checks. - # We exclude this workflow's own check-run (job name `claude-review`) so the - # gate never waits on or fails because of itself. - - name: Wait for CI; skip the paid review if any check failed - id: gate - env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - REPO: ${{ github.repository }} - SHA: ${{ github.event.pull_request.head.sha }} - run: | - decision=skip - for i in $(seq 1 72); do # ~24 min max - json=$(gh api "repos/$REPO/commits/$SHA/check-runs" --paginate \ - --jq '[.check_runs[] | select(.name != "claude-review")]' 2>/dev/null) || json='' - [ -z "$json" ] && { sleep 20; continue; } - total=$(jq 'length' <<<"$json") - if [ "$total" -eq 0 ]; then - [ "$i" -ge 9 ] && { decision=run; break; } # ~3 min grace: nothing to gate on - sleep 20; continue - fi - pending=$(jq '[.[]|select(.status!="completed")]|length' <<<"$json") - if [ "$pending" -eq 0 ]; then - bad=$(jq '[.[]|select((.conclusion//"")|test("^(failure|cancelled|timed_out|action_required|stale)$"))]|length' <<<"$json") - [ "$bad" -eq 0 ] && decision=run || decision=skip - break - fi - sleep 20 - done - echo "decision=$decision" >> "$GITHUB_OUTPUT" - echo "gate decision: $decision" - - - name: Check out base branch - if: steps.gate.outputs.decision == 'run' - # Intentionally tracks the major-version tag (not a pinned SHA) so - # upstream fixes are picked up automatically. - # - # No `repository:`/`ref:` here on purpose — this checks out the base - # branch (master), never the fork's PR head. See the SECURITY note - # above. - uses: actions/checkout@v7 - with: - fetch-depth: 1 - persist-credentials: false - - - name: Run Claude Code Review - if: steps.gate.outputs.decision == 'run' - # Pinned to an immutable SHA: this job runs as pull_request_target with - # pull-requests: write, so a moved upstream tag must not change what - # runs -- same rationale as github-script's pin in pgxntool's - # ci.yml/protect-label.yml. - uses: anthropics/claude-code-action@a874e9ecd7bb36efdad65429c6b35815f5a08f10 # v1 - with: - claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} - # Provide github_token so the action uses it directly for GitHub API - # calls instead of the OIDC->GitHub-App-token exchange, which 401s under - # pull_request_target. GITHUB_TOKEN is repo/workflow-scoped (independent - # of the actor's role) and has pull-requests: write here. - github_token: ${{ secrets.GITHUB_TOKEN }} - # NOTE: plugin_marketplaces can't be pinned — it tracks the - # marketplace repo's default branch (upstream anthropics/claude-code). - plugin_marketplaces: 'https://github.com/anthropics/claude-code.git' - plugins: 'code-review@claude-code-plugins' - # --comment is required: without it, the code-review plugin only - # prints its findings to the job log and never posts anything to - # the PR (confirmed by capturing the hidden SDK transcript on a - # canary PR: the review correctly found an injected bug but ended - # with "No `--comment` argument was provided, so no GitHub - # comments were posted"). Every review run before this fix has - # been silently invisible on GitHub. - prompt: '/code-review:code-review ${{ github.repository }}/pull/${{ github.event.pull_request.number }} --comment' - # A direct `prompt:` (no @claude mention) runs the action in "agent - # mode". In that mode, claude-code-action only installs the - # github_inline_comment MCP server if it sees - # mcp__github_inline_comment__create_inline_comment listed in an - # --allowedTools flag inside claude_args (src/modes/agent/parse-tools.ts) -- - # it does NOT look at the code-review plugin's own `allowed-tools` - # frontmatter to decide that. Without this, the MCP server never - # starts, the tool genuinely doesn't exist in the session, and the - # plugin silently falls back to one consolidated PR comment instead - # of real inline line comments. - claude_args: '--allowedTools mcp__github_inline_comment__create_inline_comment' + secrets: inherit + with: + trusted_authors: jnasbyupgrade From c6f7d3a64ed25ad10e5632d5042ee8e74ccd807e Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Wed, 23 Sep 2026 16:48:24 -0500 Subject: [PATCH 2/3] CLAUDE.md: document merge order rule for paired pgxntool/pgxntool-test PRs pgxntool-test PR #79 merged eight days before its paired pgxntool PR #109, which actually added the behavior #79's new tests exercised (check-test-install-error-stop.sh, test-build/installcheck gating). Since a pgxntool-test PR with no paired pgxntool branch runs CI against pgxntool master directly, this broke CI for every unrelated pgxntool-test PR in that window (e.g. #84, #85) with a misleading "No such file or directory" error. Co-Authored-By: Claude Sonnet 5 --- CLAUDE.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index 335cc48..5e201f2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -65,6 +65,14 @@ let them decide when to commit. Because of that, whether a paired PR's cross-reference actually made it onto master can only be verified *after the fact*, once both sides are already merged — see `crossref-audit` below. +### Merge Order for Paired PRs + +**When a pgxntool-test PR's new/changed tests exercise pgxntool behavior that isn't on pgxntool's master yet, merge the pgxntool PR first.** Once a pgxntool-test PR has no paired pgxntool branch (matched by branch name **and** account — see README.md's CI section), its CI runs against pgxntool master directly. Merging the test side first means every *other*, unrelated pgxntool-test PR in that window fails CI against a script/target/behavior that doesn't exist yet, with no obvious link back to the missing pgxntool PR. + +If a pgxntool-test PR's tests only cover behavior already on pgxntool's master, order doesn't matter. + +**Evidence**: pgxntool-test PR #79 (tests for `check-test-install-error-stop.sh`, `build-results`, and `test-build` ordering) merged 2026-09-08, eight days before its paired pgxntool PR #109 — which actually added `test/bin/check-test-install-error-stop.sh` and the `test-build`/`installcheck` gating those tests exercise — merged 2026-09-16. In that window, unrelated pgxntool-test PRs with no paired pgxntool branch (e.g. #84, #85) failed CI with `check-test-install-error-stop.sh: No such file or directory`, since the script wasn't on pgxntool master yet. + ### End of Each Round: Check for Missing Cross-References **At the end of each round of work in pgxntool or pgxntool-test (not just once at session start — sessions here run long), and before rebasing any branch onto a fresh master fetch**, run the `crossref-audit` skill's script: `bash .claude/skills/crossref-audit/scripts/audit.sh `. It fetches both masters, caches the last-checked SHAs, and exits immediately with a one-line "nothing new" if neither has moved since the last clean check — so repeating it every round costs near-zero tokens in the common case. Only read further into the skill's rules if it reports something flagged; follow those rules exactly, especially around when it is and isn't safe to amend an already-merged commit. From dca650666be05214bbaa9eb774e5b08ba7269056 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Wed, 23 Sep 2026 16:50:12 -0500 Subject: [PATCH 3/3] CLAUDE.md: split merge-order rule by addition vs. removal/rename The addition case (pgxntool-test's new tests reference not-yet-existing pgxntool behavior) has one safe order: pgxntool merges first, per #79/#109. Removal/rename is different: both orders leave one master's tests broken against the other's, since there's no version of the behavior both sides agree on simultaneously. #93/#72 (a variable rename) landed ~2 hours apart with no unrelated PR caught in the gap, but the rename still broke #95's own in-flight code via a plain merge from master (fixed in 42e0903) -- confirming a rename has no free lunch even when landed close together. #123/#88 (open, unmerged) is a live example of this same unresolved case. Co-Authored-By: Claude Sonnet 5 --- CLAUDE.md | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 5e201f2..bc95e9a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -67,11 +67,17 @@ Because of that, whether a paired PR's cross-reference actually made it onto mas ### Merge Order for Paired PRs -**When a pgxntool-test PR's new/changed tests exercise pgxntool behavior that isn't on pgxntool's master yet, merge the pgxntool PR first.** Once a pgxntool-test PR has no paired pgxntool branch (matched by branch name **and** account — see README.md's CI section), its CI runs against pgxntool master directly. Merging the test side first means every *other*, unrelated pgxntool-test PR in that window fails CI against a script/target/behavior that doesn't exist yet, with no obvious link back to the missing pgxntool PR. +The safe order depends on which direction the pairing changes behavior. Once a pgxntool-test PR has no paired pgxntool branch (matched by branch name **and** account — see README.md's CI section), its CI runs against pgxntool master directly, which is the mechanism behind both cases below. -If a pgxntool-test PR's tests only cover behavior already on pgxntool's master, order doesn't matter. +**Addition (pgxntool adds behavior, pgxntool-test adds coverage for it): merge the pgxntool PR first.** If the pgxntool-test PR merges first, its new tests reference something that doesn't exist on pgxntool master yet — breaking CI for every *other*, unrelated pgxntool-test PR in that window, with no obvious link back to the missing pgxntool PR. -**Evidence**: pgxntool-test PR #79 (tests for `check-test-install-error-stop.sh`, `build-results`, and `test-build` ordering) merged 2026-09-08, eight days before its paired pgxntool PR #109 — which actually added `test/bin/check-test-install-error-stop.sh` and the `test-build`/`installcheck` gating those tests exercise — merged 2026-09-16. In that window, unrelated pgxntool-test PRs with no paired pgxntool branch (e.g. #84, #85) failed CI with `check-test-install-error-stop.sh: No such file or directory`, since the script wasn't on pgxntool master yet. +*Evidence*: pgxntool-test PR #79 (tests for `check-test-install-error-stop.sh`, `build-results`, and `test-build` ordering) merged 2026-09-08, eight days before its paired pgxntool PR #109 — which actually added `test/bin/check-test-install-error-stop.sh` and the `test-build`/`installcheck` gating those tests exercise — merged 2026-09-16. In that window, unrelated pgxntool-test PRs with no paired pgxntool branch (e.g. #84, #85) failed CI with `check-test-install-error-stop.sh: No such file or directory`, since the script wasn't on pgxntool master yet. + +**Removal/rename (pgxntool removes or renames something pgxntool-test's *existing* tests already reference): no unilateral order is safe.** pgxntool-first leaves pgxntool-test's still-old-referencing master broken until the test-side update lands; pgxntool-test-first (with tests already updated to the new name) breaks the same way against pgxntool's still-old master. Minimize the window instead: land both PRs as close to back-to-back as practical, or keep the old name/behavior working alongside the new one (a deprecation window) so neither master ever breaks — and flag the sequencing to the maintainer rather than picking an order unprompted. + +*Evidence*: pgxntool PR #93 (renamed 5 internal `PGXNTOOL_*` variables to `_PGXNTOOL_*`) and its paired pgxntool-test PR #72 merged same-day, ~2h8m apart (2026-09-08) — no unrelated pgxntool-test PR happened to run CI in that gap, but the rename still broke something else with no transition period: pgxntool PR #95 (open, unrelated), whose own new code referenced the pre-rename `PGXNTOOL_CONTROL_FILES` name, broke the moment its branch merged master and picked up #93's rename (fixed in commit `42e0903`). pgxntool #123 (renames `build-results` to `results-build`) and its paired pgxntool-test #88 are open as of this writing, in the same category — an unresolved case, not a precedent to copy blindly. + +If a pgxntool-test PR's tests only cover pgxntool behavior that is unchanged on pgxntool's master, order doesn't matter. ### End of Each Round: Check for Missing Cross-References