Skip to content

Add CI workflows and multi-session PR guard#33

Open
jnasbyupgrade wants to merge 15 commits into
masterfrom
add-ci
Open

Add CI workflows and multi-session PR guard#33
jnasbyupgrade wants to merge 15 commits into
masterfrom
add-ci

Conversation

@jnasbyupgrade
Copy link
Copy Markdown
Contributor

Implements the cross-repo CI design from pgxntool-test#15.

What's added

.github/workflows/ci.yml — main CI workflow:

  • Searches pgxntool-test for an open PR with the same branch name
  • Waits up to 5 minutes for that PR's CI to pass before running tests
  • Falls back to pgxntool-test/master if no test PR is found AND the no-test-pr label is set by a maintainer
  • Fails (blocks merge) if neither condition is met

.github/workflows/protect-label.yml — label protection:

  • Uses pull_request_target so it has write access from fork PRs
  • Removes no-test-pr if added by a non-writer; re-adds it if removed by a non-writer
  • Handles: non-collaborator 404s, bot-actor loop prevention, org team membership edge cases

.claude/CLAUDE.md — asks for confirmation before touching any existing PR not opened in this session (multiple concurrent Claude sessions are common across these repos)

Setup required after merge

  • Create the no-test-pr label in this repo (Settings → Labels)
  • Add resolve-test-ref as a required status check on the master branch protection rule

Pairs with: Postgres-Extensions/pgxntool-test#20

🤖 Generated with Claude Code

- .github/workflows/ci.yml: wait for corresponding pgxntool-test PR CI
  to pass before running tests; falls back to pgxntool-test/master after
  5 minutes if no test PR exists (requires no-test-pr label override)
- .github/workflows/protect-label.yml: enforce write-access-only on the
  no-test-pr label; handles non-collaborator 404, bot loop prevention,
  and org team membership edge cases
- .claude/CLAUDE.md: warn before touching existing PRs not opened in
  this session (multiple concurrent Claude sessions are common)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jnasbyupgrade and others added 3 commits May 15, 2026 15:17
git subtree add refuses to work with shallow clones; fails with
"shallow roots are not allowed to be updated" which looks like a
remote/ref problem rather than a depth issue.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- gem install asciidoctor instead of apt ruby-asciidoctor (same fix
  as pgxntool-test)
- Print '=== BRANCHES: ===' line at the start of each test job so
  both the pgxntool and pgxntool-test refs are visible in CI logs

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
GitHub Actions workflows are development infrastructure and should not
be included in PGXN distribution packages. Without this exclusion,
`make dist` (which uses `git archive`) includes .github/workflows/,
causing the dist test to fail:

  not ok 22 distribution contains exact expected files
  # ERROR: Distribution contents differ from expected manifest
  # > pgxntool/.github/
  # > pgxntool/.github/workflows/
  # > pgxntool/.github/workflows/ci.yml
  # > pgxntool/.github/workflows/protect-label.yml

The .gitattributes `export-ignore` attribute controls what `git archive`
excludes. Adding `.github/ export-ignore` ensures CI workflow files are
stripped from distributions, matching the existing treatment of .claude/,
.gitattributes, and documentation files.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jnasbyupgrade and others added 7 commits May 15, 2026 16:25
Key changes:

1. Replace 5-minute polling 'resolve-test-ref' job with a fast
   'check-test-pr' job (seconds, not minutes). The test PR must exist
   when you push — no waiting.

2. When a paired pgxntool-test PR is found, pgxntool CI passes
   immediately without running tests. Tests run in the test PR's own
   CI — no duplication.

3. When no paired test PR exists and no override label is present,
   CI fails immediately with an actionable error message pointing to
   the README docs.

4. Rename 'no-test-pr' label to 'commit-with-no-tests'. This name
   better conveys that it's a merge-time decision (not just a CI
   skip), and that it's unusual and maintainer-only.

5. Add 'Pre-install pgtap' step to the test job (only runs in the
   commit-with-no-tests case). Prevents a concurrent-install race
   condition in concurrent-make-test.bats.

6. Update protect-label.yml to guard the new label name.

The failure message now links to README#ci-and-contributing which
explains branch naming requirements and how to request the label.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When a paired pgxntool-test PR is found, verify:
1. CI has run for the exact current HEAD SHA (no stale runs from old commits)
2. All check runs completed successfully
3. The most recent passing run is within 7 days

Each failure case provides URLs for both the test PR and the pgxntool
re-run check, making it clear what to fix and how to re-trigger.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…est-PR conflict

- Fork matching: require test PR to come from the same owner as the
  pgxntool PR; never fall back to a random contributor's fork
- Race condition: poll (30s intervals, 20min max) instead of immediately
  failing when the paired test PR's CI hasn't started or is still running
- Label conflict: fail if commit-with-no-tests label is set but a paired
  test PR also exists — that combination is contradictory
- Label name: define as NO_TEST_LABEL const in ci.yml and LABEL const in
  protect-label.yml; replace all literals with those consts

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…eck; add sync warning

- Multiple matching test PRs from same owner: error+fail rather than warning
  (if we can't tell which one to use, that's a hard error)
- Remove the 7-day recency check: since we already validate the exact SHA,
  forcing a re-run of test CI just to satisfy a time window adds friction
  (e.g. manual re-runs of pgxntool CI shouldn't require re-running test CI)
- Add big-fat WARNING comment on the duplicated test job directing maintainers
  to keep it in sync with pgxntool-test/ci.yml; includes TODO for reusable workflow

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace the 88-line duplicated test job (with its WARNING comment) with a
7-line call to Postgres-Extensions/pgxntool-test/.github/workflows/run-tests.yml.
Single source of truth; no more risk of the two copies drifting apart.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
GitHub Actions does not support ${{ }} expressions in the uses: field.
The test job only runs in the commit-with-no-tests path, which always
sets test-ref=master, so @master is always correct here.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
run-tests.yml doesn't exist on pgxntool-test/master yet (it's on add-ci).
GitHub validates uses: references upfront even for skipped jobs, so the
entire workflow fails if the file isn't found. Use @add-ci while developing;
change to @master when pgxntool-test/add-ci merges.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@decibel
Copy link
Copy Markdown
Collaborator

decibel commented May 19, 2026

@coderabbitai please review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 19, 2026

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch PR details
gh pr view 33 --repo Postgres-Extensions/pgxntool --json title,body,files,labels,state

echo "=== PR FILES ==="
gh pr diff 33 --repo Postgres-Extensions/pgxntool

Length of output: 23205


@decibel: Here is my review of the PR. The overall design is solid and well-documented, but I found a few issues ranging from a hard blocker down to minor nits.


.github/workflows/ci.yml

🔴 Critical — @add-ci hardcoded ref will break after merge

uses: Postgres-Extensions/pgxntool-test/.github/workflows/run-tests.yml@add-ci

The TODO comment acknowledges this, but it means the test job will silently break (or fail to find the workflow file) as soon as the add-ci branch in pgxntool-test is deleted post-merge. This should be addressed as part of the paired PR #20 merge, not left as a post-merge TODO. Suggest using a release tag or committing to update both repos atomically.


🔴 Critical — test-ref output is computed but never used

The check-test-pr job sets:

outputs:
  test-ref: ${{ steps.check.outputs.test_ref }}

…but the test job hardcodes master:

with:
  pgxntool-test-ref: master

The test-ref output is dead code. Either wire it through:

pgxntool-test-ref: ${{ needs.check-test-pr.outputs.test-ref }}

…or remove the output entirely. As-is, it creates a false impression that the SHA from a matched test PR is being used when it isn't.


🟡 High — Setup instructions reference wrong status check name

The PR description says:

Add resolve-test-ref as a required status check on the master branch protection rule

But the actual job name in ci.yml is check-test-pr (with display name "Check for paired pgxntool-test PR"). GitHub branch protection requires the exact check name — resolve-test-ref doesn't exist and will either never be satisfied or the wrong check is being guarded. Please verify which name was intended.


🟡 High — Inaccurate comment in test job

# test-ref is always 'master' when this job runs (only the
# commit-with-no-tests path sets run-tests=true, and that path always
# sets test-ref=master).

This is wrong. There are two paths that set run_tests=true: the commit-with-no-tests label path and the master-branch fast-path. Both happen to set test_ref=master, so the hardcoded master is technically correct, but the comment is misleading. Suggest:

# test-ref is always 'master' when this job runs: both the master-branch
# fast-path and the commit-with-no-tests label path set test_ref=master.

🟡 Medium — per_page: 100 may silently miss PRs

const { data: prs } = await github.rest.pulls.list({
  ...
  per_page: 100
});

If pgxntool-test ever has >100 open PRs, a matching PR could be silently missed, causing a false-negative failure. Consider adding pagination or at least a warning if prs.length === 100.


🟢 Nit — pgxntool-ref and pgxntool-branch both receive head.ref

with:
  pgxntool-ref: ${{ github.event.pull_request.head.ref }}
  pgxntool-branch: ${{ github.event.pull_request.head.ref }}

If these are intentionally different concepts (e.g., one is a full ref, one is just a branch name), a brief inline comment would help. If they're genuinely the same thing, the callee workflow may be worth simplifying.


.github/workflows/protect-label.yml

🔴 Critical — Label name in PR description doesn't match the code

The PR description and setup instructions repeatedly say no-test-pr, but every line of code uses commit-with-no-tests:

const LABEL = 'commit-with-no-tests';
if: github.event.label.name == 'commit-with-no-tests'

The setup instruction says:

Create the no-test-pr label in this repo (Settings → Labels)

Creating no-test-pr will do nothing — the workflows look for commit-with-no-tests. The PR description (and possibly the companion pgxntool-test PR #20) needs to be updated to consistently use commit-with-no-tests.


🟢 Nit — Consider action_conclusion values for check runs

In ci.yml:

const failed = runs.filter(
  r => !['success', 'skipped', 'neutral'].includes(r.conclusion)
);

stale is also a valid conclusion value (for abandoned check runs). It should probably be treated as a failure (or at least generate a distinct, actionable error message), since a stale run doesn't confirm the test PR actually passed.


.claude/CLAUDE.md & .gitattributes

Both look clean and intentional. The export-ignore on .github/ is a reasonable choice for a pgxn distribution tool.


Summary

Severity Issue
🔴 Critical Label name mismatch: PR description says no-test-pr, code uses commit-with-no-tests
🔴 Critical @add-ci hardcoded ref is a post-merge time bomb
🔴 Critical test-ref output computed but never consumed in test job
🟡 High Setup instructions reference non-existent check name resolve-test-ref
🟡 High Inaccurate comment in test job
🟡 Medium per_page: 100 can silently miss PRs
🟢 Nit pgxntool-ref vs pgxntool-branch worth documenting
🟢 Nit stale conclusion value not handled in check-run filter
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 19, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

This PR establishes coordinated CI/CD controls: a protect-label workflow restricting the commit-with-no-tests label, a ci workflow that pairs pgxntool PRs with matching pgxntool-test PRs and conditionally invokes cross-repo tests, updates .gitattributes to exclude .github/ from exports, and adds CLAUDE.md guidance about CI monitoring and cross-repo workflow constraints.

Sequence Diagram(s)

sequenceDiagram
  participant SourcePR as pgxntool PR
  participant CI as ci.yml
  participant TestRepo as pgxntool-test
  participant Checks as GitHub Checks
  CI->>TestRepo: list open PRs matching branch+owner
  alt exactly one paired PR
    CI->>Checks: poll test PR head SHA check runs until completion
    Checks-->>CI: final conclusion (success/failure)
  else no paired PR
    CI->>CI: re-fetch SourcePR labels
    alt commit-with-no-tests present
      CI->>TestRepo: run tests against master
    else
      CI-->>SourcePR: fail with remediation message
    end
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I hopped through workflows, guards, and names,

Chasing test PRs across forked lanes,
A label kept safe from wandering hands,
Docs and exports trimmed tidy commands,
The rabbit nodded: CI now stands.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed Title clearly summarizes the main changes: adding CI workflows and a multi-session PR guard, which directly matches the primary objectives of the changeset.
Description check ✅ Passed Description is directly related to the changeset, providing specific details about the CI workflow design, label protection, and multi-session guard implementation.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch add-ci

Comment @coderabbitai help to get the list of available commands and usage tips.

Document the cross-repo reusable workflow tradeoffs, the @add-ci→@master
transition requirement, and merge order constraints. Expand the comment on
the uses: line to make the temporary nature explicit.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/ci.yml:
- Around line 41-47: The early return for branch === 'master' should only apply
when the PR's head is from this repository (not a fork); update the logic that
uses branch to also check the pull request head repo identity (e.g., compare
pull_request.head.repo.full_name or head.repo.name against the current
repository) before calling core.setOutput('run_tests','true') and
core.setOutput('test_ref','master'); ensure forked PRs with head repo different
from this repo skip this shortcut so the paired pgxntool-test requirement still
applies.
- Line 25: The workflow uses mutable refs—update both occurrences (the
`actions/github-script@v7` usage and the reusable workflow call
`run-tests.yml@add-ci`) to immutable commit SHAs or a stable branch tag; locate
the `uses: actions/github-script@v7` line and replace the ref with the
corresponding commit SHA for v7 (or a stable branch/tag), and locate the
reusable workflow reference `run-tests.yml@add-ci` and replace `add-ci` with
either the merge target stable ref (e.g., `@master` or a release tag) or the
commit SHA of that branch so the workflow won't break when the feature branch is
deleted.
- Around line 61-66: The current github.rest.pulls.list call (the pulls.list
invocation that assigns const { data: prs }) only fetches up to 100 PRs and can
miss matches; update the logic to either paginate through all pages (use
page/per_page in a loop or Octokit pagination helpers to aggregate all PRs) or
narrow the server-side query by using the head filter format
`${prOwner}:${branch}` in the pulls.list parameters to return only PRs from that
head; ensure you update the code that reads `prs` to handle an array aggregated
from multiple pages if you choose pagination.

In @.github/workflows/protect-label.yml:
- Line 33: Replace the mutable tag on the GitHub Action with an immutable commit
SHA: change the uses line that currently reads `uses: actions/github-script@v7`
to `uses: actions/github-script@<full-commit-sha>` (a full 40-character commit
SHA from the actions/github-script repo) so the workflow always runs the exact
commit; ensure you paste the full SHA for the commit you want to pin.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: f2df7230-456b-4373-b6f7-2cb9cfbc8e02

📥 Commits

Reviewing files that changed from the base of the PR and between 8176304 and 4e8b096.

📒 Files selected for processing (4)
  • .claude/CLAUDE.md
  • .gitattributes
  • .github/workflows/ci.yml
  • .github/workflows/protect-label.yml

Comment thread .github/workflows/ci.yml
Comment thread .github/workflows/ci.yml
Comment on lines +41 to +47
// master-to-master PRs have no paired test PR by convention.
// Run tests against pgxntool-test/master directly.
if (branch === 'master') {
core.setOutput('run_tests', 'true');
core.setOutput('test_ref', 'master');
return;
}
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Restrict the master shortcut to PRs whose head is this repo.

Right now any PR whose source branch is named master hits this early return, including fork PRs. That bypasses the paired pgxntool-test requirement for a real class of contributor PRs.

Proposed fix
-            if (branch === 'master') {
+            const headRepo = context.payload.pull_request.head.repo;
+            const isBaseRepoHead =
+              headRepo?.owner?.login === context.repo.owner &&
+              headRepo?.name === context.repo.repo;
+
+            if (branch === 'master' && isBaseRepoHead) {
               core.setOutput('run_tests', 'true');
               core.setOutput('test_ref', 'master');
               return;
             }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// master-to-master PRs have no paired test PR by convention.
// Run tests against pgxntool-test/master directly.
if (branch === 'master') {
core.setOutput('run_tests', 'true');
core.setOutput('test_ref', 'master');
return;
}
// master-to-master PRs have no paired test PR by convention.
// Run tests against pgxntool-test/master directly.
const headRepo = context.payload.pull_request.head.repo;
const isBaseRepoHead =
headRepo?.owner?.login === context.repo.owner &&
headRepo?.name === context.repo.repo;
if (branch === 'master' && isBaseRepoHead) {
core.setOutput('run_tests', 'true');
core.setOutput('test_ref', 'master');
return;
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/ci.yml around lines 41 - 47, The early return for branch
=== 'master' should only apply when the PR's head is from this repository (not a
fork); update the logic that uses branch to also check the pull request head
repo identity (e.g., compare pull_request.head.repo.full_name or head.repo.name
against the current repository) before calling
core.setOutput('run_tests','true') and core.setOutput('test_ref','master');
ensure forked PRs with head repo different from this repo skip this shortcut so
the paired pgxntool-test requirement still applies.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm... it's probably a mistake if someone commits to master on a fork. We shouldn't dis-allow it, but it'd be nice if CI posted a warning.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jnasbyupgrade That's a sensible middle ground. Here's a version that posts a warning comment when the branch is master and the head repo is a fork, but still lets CI proceed normally:

             // master-to-master PRs have no paired test PR by convention.
             // Run tests against pgxntool-test/master directly.
             if (branch === 'master') {
+              const headRepo = context.payload.pull_request.head.repo;
+              const isBaseRepoHead =
+                headRepo?.owner?.login === context.repo.owner &&
+                headRepo?.name === context.repo.repo;
+
+              if (!isBaseRepoHead) {
+                await github.rest.issues.createComment({
+                  owner: context.repo.owner,
+                  repo: context.repo.repo,
+                  issue_number: prNumber,
+                  body:
+                    '⚠️ **Heads up:** This PR was opened from the `master` branch of a fork. ' +
+                    'Using `master` as a feature branch is usually a mistake — consider ' +
+                    'creating a dedicated branch for your changes instead. CI will proceed ' +
+                    'normally, but please double-check this is intentional.',
+                });
+              }
+
               core.setOutput('run_tests', 'true');
               core.setOutput('test_ref', 'master');
               return;
             }

One small thing to be aware of: because this uses github.rest.issues.createComment, the pull_request event's GITHUB_TOKEN needs write permission to post comments. The workflow currently doesn't declare permissions: at the job level — you'll either want to add pull-requests: write to the check-test-pr job or at the workflow level:

jobs:
  check-test-pr:
    permissions:
      pull-requests: write

Without that, the comment call will silently fail (or error) on fork PRs since the default pull_request token is read-only for comments from forks.

Comment thread .github/workflows/ci.yml Outdated

steps:
- name: Enforce write-access-only on 'commit-with-no-tests' label
uses: actions/github-script@v7
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

find . -name "protect-label.yml" -type f

Repository: Postgres-Extensions/pgxntool

Length of output: 110


🏁 Script executed:

cat -n .github/workflows/protect-label.yml

Repository: Postgres-Extensions/pgxntool

Length of output: 8099


Pin actions/github-script to an immutable commit SHA.

This workflow runs in pull_request_target with write-capable permissions. Keeping the action on a mutable tag (@v7) means a future upstream tag move would change what executes here without any repo diff. Pin it to a full commit SHA instead.

🧰 Tools
🪛 zizmor (1.25.2)

[error] 33-33: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)

(unpinned-uses)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/protect-label.yml at line 33, Replace the mutable tag on
the GitHub Action with an immutable commit SHA: change the uses line that
currently reads `uses: actions/github-script@v7` to `uses:
actions/github-script@<full-commit-sha>` (a full 40-character commit SHA from
the actions/github-script repo) so the workflow always runs the exact commit;
ensure you paste the full SHA for the commit you want to pin.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/ci.yml:
- Around line 3-10: Add a top-level GitHub Actions concurrency block to the
workflow to cancel overlapping runs for the same PR: add a concurrency key at
the root of the YAML with a group that uniquely identifies the PR (e.g. group:
"check-test-pr-${{ github.event.pull_request.number }}" or "ci-${{ github.ref
}}") and set cancel-in-progress: true so rapid pushes cancel prior
`check-test-pr` polling jobs; update the workflow root (where `on:
pull_request:` is defined) rather than per-step and ensure the concurrency group
name matches the `check-test-pr` job scope.
- Around line 1-17: Add an explicit top-level permissions block to the workflow
to restrict GITHUB_TOKEN to least privilege: declare permissions: pull-requests:
read and checks: read (instead of relying on repository defaults or read-all).
Edit the YAML at the top-level of the CI workflow (the existing "name: CI"
workflow) and insert a permissions mapping so only the pull-requests and checks
scopes are granted to the GITHUB_TOKEN.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 4e49725f-3307-4315-8aa3-546a91108bef

📥 Commits

Reviewing files that changed from the base of the PR and between 4e8b096 and d5eab0c.

📒 Files selected for processing (2)
  • .github/workflows/CLAUDE.md
  • .github/workflows/ci.yml

Comment thread .github/workflows/ci.yml
Comment thread .github/workflows/ci.yml
@decibel
Copy link
Copy Markdown
Collaborator

decibel commented May 19, 2026

@coderabbitai is there a way to configure you to post individual review comments using Github's Review Changes feature?

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 19, 2026

Tip

For best results, initiate chat on the files or code changes.

@decibel: Great question. There's an important distinction here between the two modes:

Automatic reviews (triggered when new commits are pushed to the PR) — inline review comments appear on specific lines of code where CodeRabbit identifies potential issues, improvements, or suggestions. Each comment includes a description of the issue, an explanation of why it matters, and often a suggested code fix. These are posted using GitHub's formal "Review Changes" mechanism, so they appear as grouped inline comments on the diff.

Command-triggered reviews (like @coderabbitai please review) — when you invoke me in a comment, I respond conversationally in the PR thread with a consolidated summary comment, rather than submitting a formal GitHub review with inline diff comments. That's what happened here.

Currently, there is no configuration option to force command-triggered reviews to use GitHub's inline "Review Changes" format instead of a consolidated comment. The closest you can get to the inline experience is to:

  1. Let automatic reviews run normally (on push), which will post inline comments.
  2. Trigger a full re-review with @coderabbitai full review — this may re-run the automated review pipeline which posts inline comments, rather than just a conversational reply.

If you'd like to see this as a feature (command-triggered reviews using GitHub's formal review format with inline comments), I'd suggest opening a feature request on the CodeRabbit feedback/support channels.

jnasbyupgrade and others added 2 commits May 19, 2026 18:47
- Add explicit permissions block (pull-requests: read, checks: read)
  for least-privilege GITHUB_TOKEN access.

- Add concurrency block to cancel stale polling jobs on re-push.
  Without this, rapid pushes stack up redundant 20-min polling runs.

- Warn (via core.warning annotation) when a fork PR's branch is named
  'master'. Almost certainly a contributor mistake; we don't block it
  but make it visible. Can't post a PR comment here — pull_request
  trigger gives a read-only token for fork PRs.

- Paginate pulls.list with github.paginate() instead of a bare
  per_page:100 call. Handles >100 open PRs correctly.

- Paginate checks.listForRef similarly; uses a map fn since that
  endpoint wraps results in {check_runs:[]} rather than a plain array.

- Collapse redundant pgxntool-ref + pgxntool-branch inputs into just
  pgxntool-branch. They were always the same value in every caller;
  the distinction (ref could be SHA) was forward-looking but unused.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github.paginate() + a map function on listForRef (which returns
{check_runs:[], total_count:N} rather than a plain array) produces
undefined elements in the result, crashing the filter step.

A single commit will not have 100+ CI check runs, so per_page:100
is correct and safe without pagination. A comment explains why we
don't use paginate() here (unlike pulls.list above, which does).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/ci.yml:
- Around line 264-283: The top-level GitHub Actions permissions are too
restrictive for the reusable workflow run-tests.yml which invokes
actions/checkout@v4; update the workflow-level permissions block to include
contents: read (or alternatively move narrow scopes to the check-test-pr job and
give the test job broader scopes) so that the test job (uses:
Postgres-Extensions/pgxntool-test/.github/workflows/run-tests.yml@add-ci) can
successfully run actions/checkout; ensure permissions includes contents: read
for the caller or grant the test job the required scopes.
- Around line 254-256: Update the CI failure guidance string that currently
reads "To resolve:\n  1. Open a PR in pgxntool-test from a branch ALSO named
'${branch}'..." to also require the same fork/owner as the head of the current
PR; explicitly state that the PR must be opened from the same repository owner
(head) as this PR in pgxntool-test so branch-name matching alone won't be
accepted. Locate and modify the template string containing `To resolve:\n  1.
Open a PR in pgxntool-test from a branch ALSO named '${branch}'` and append a
clear sentence like "The PR must be created from the same fork/owner (head) as
this PR" to ensure both branch name and owner requirements are communicated.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 2bafd4ce-83a4-4ddd-81e0-a2458b985a2f

📥 Commits

Reviewing files that changed from the base of the PR and between d5eab0c and feb27cf.

📒 Files selected for processing (1)
  • .github/workflows/ci.yml

Comment thread .github/workflows/ci.yml
Comment on lines +254 to +256
`To resolve:\n` +
` 1. Open a PR in pgxntool-test from a branch ALSO named '${branch}'.\n` +
` Branch names must match exactly for the pairing to work.\n\n` +
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Mention the same-owner requirement in the failure guidance.

Lines 96-99 require both the same branch name and the same head owner, but this remediation only tells contributors to match the branch. Following it from the wrong fork owner will still fail CI.

✏️ Proposed patch
-              `  1. Open a PR in pgxntool-test from a branch ALSO named '${branch}'.\n` +
-              `     Branch names must match exactly for the pairing to work.\n\n` +
+              `  1. Open a PR in pgxntool-test from the same GitHub owner ` +
+              `('${prOwner}') using a branch ALSO named '${branch}'.\n` +
+              `     Both owner and branch name must match exactly for the pairing to work.\n\n` +
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
`To resolve:\n` +
` 1. Open a PR in pgxntool-test from a branch ALSO named '${branch}'.\n` +
` Branch names must match exactly for the pairing to work.\n\n` +
`To resolve:\n` +
` 1. Open a PR in pgxntool-test from the same GitHub owner ('${prOwner}') using a branch ALSO named '${branch}'.\n` +
` Both owner and branch name must match exactly for the pairing to work.\n\n` +
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/ci.yml around lines 254 - 256, Update the CI failure
guidance string that currently reads "To resolve:\n  1. Open a PR in
pgxntool-test from a branch ALSO named '${branch}'..." to also require the same
fork/owner as the head of the current PR; explicitly state that the PR must be
opened from the same repository owner (head) as this PR in pgxntool-test so
branch-name matching alone won't be accepted. Locate and modify the template
string containing `To resolve:\n  1. Open a PR in pgxntool-test from a branch
ALSO named '${branch}'` and append a clear sentence like "The PR must be created
from the same fork/owner (head) as this PR" to ensure both branch name and owner
requirements are communicated.

Comment thread .github/workflows/ci.yml
Comment on lines +264 to +283
test:
needs: check-test-pr
if: needs.check-test-pr.outputs.run-tests == 'true'
# -----------------------------------------------------------------------
# CROSS-REPO REUSABLE WORKFLOW — READ BEFORE CHANGING THIS REF
# See: .github/workflows/CLAUDE.md for full architecture notes.
#
# The ref (@add-ci / @master) must be a static string — GitHub Actions
# does not support expressions in uses:. During development on a feature
# branch the ref is @<branch> so CI can find run-tests.yml before it
# lands on master. Before this PR merges, two things must happen:
# 1. pgxntool-test/<branch> merges to master first
# 2. This ref is updated from @<branch> to @master
#
# CURRENT REF: @add-ci (temporary — pgxntool-test/add-ci not yet merged)
# -----------------------------------------------------------------------
uses: Postgres-Extensions/pgxntool-test/.github/workflows/run-tests.yml@add-ci
with:
pgxntool-branch: ${{ github.event.pull_request.head.ref }}
pgxntool-test-ref: master
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

tmp="$(mktemp)"
curl -fsSL \
  https://raw.githubusercontent.com/Postgres-Extensions/pgxntool-test/add-ci/.github/workflows/run-tests.yml \
  -o "$tmp"

echo "=== Permission-sensitive steps in run-tests.yml@add-ci ==="
rg -n "^\s*permissions:|actions/checkout|upload-artifact|download-artifact|github-token|gh api" "$tmp"

Repository: Postgres-Extensions/pgxntool

Length of output: 282


🏁 Script executed:

head -100 .github/workflows/ci.yml | cat -n

Repository: Postgres-Extensions/pgxntool

Length of output: 5374


🏁 Script executed:

curl -fsSL https://raw.githubusercontent.com/Postgres-Extensions/pgxntool-test/add-ci/.github/workflows/run-tests.yml | head -100 | cat -n

Repository: Postgres-Extensions/pgxntool

Length of output: 5206


actions/checkout in the reusable workflow will fail due to insufficient permissions.

The workflow-level permissions block (lines 11–13) includes only pull-requests: read and checks: read, but the called workflow (run-tests.yml@add-ci) uses actions/checkout@v4 at lines 37 and 50, which requires contents: read. Reusable workflows inherit and cannot elevate caller permissions, so both checkouts will fail with a permission error.

Add contents: read to the top-level permissions block, or move the narrow permissions down to check-test-pr and grant test the full scopes it needs.

🧰 Tools
🪛 zizmor (1.25.2)

[error] 280-280: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)

(unpinned-uses)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/ci.yml around lines 264 - 283, The top-level GitHub
Actions permissions are too restrictive for the reusable workflow run-tests.yml
which invokes actions/checkout@v4; update the workflow-level permissions block
to include contents: read (or alternatively move narrow scopes to the
check-test-pr job and give the test job broader scopes) so that the test job
(uses: Postgres-Extensions/pgxntool-test/.github/workflows/run-tests.yml@add-ci)
can successfully run actions/checkout; ensure permissions includes contents:
read for the caller or grant the test job the required scopes.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/ci.yml:
- Around line 42-57: The current gate only checks the head branch variable
(branch / context.payload.pull_request.head.ref) and treats any PR with head
'master' as a master-to-master bypass; change this to require both head and base
be 'master' by reading context.payload.pull_request.base.ref into a variable
(e.g., base) and update the if condition from if (branch === 'master') to if
(branch === 'master' && base === 'master') so the NO_TEST_LABEL / paired-test
logic only applies to true master-to-master PRs.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 29652f65-b886-4b82-abdf-7935a00375a9

📥 Commits

Reviewing files that changed from the base of the PR and between feb27cf and 3f7e5ae.

📒 Files selected for processing (1)
  • .github/workflows/ci.yml

Comment thread .github/workflows/ci.yml
Comment on lines +42 to +57
const branch = context.payload.pull_request.head.ref;
const prNumber = context.payload.pull_request.number;
// Single source of truth for the label name. Must also match the
// literal string in the protect-label.yml job-level `if:` condition
// (YAML expressions can't reference JS constants).
const NO_TEST_LABEL = 'commit-with-no-tests';

// master-to-master PRs have no paired test PR by convention.
// Run tests against pgxntool-test/master directly.
//
// If a fork PR's branch is named 'master', that's almost certainly
// a mistake (contributors should use a feature branch), but we
// don't block it — just warn visibly as an annotation on the run.
// Note: pull_request gives a read-only token for fork PRs, so we
// can't post a PR comment back to the upstream repo from here.
if (branch === 'master') {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Gate the shortcut on the base branch too.

Line 57 implements this as “head branch is master”, but the comment says this bypass is only for master-to-master PRs. A PR from master into any other base branch will also skip paired-test enforcement and incorrectly force pgxntool-test/master.

Proposed fix
             const branch = context.payload.pull_request.head.ref;
+            const baseBranch = context.payload.pull_request.base.ref;
             const prNumber = context.payload.pull_request.number;
@@
-            if (branch === 'master') {
+            if (branch === 'master' && baseBranch === 'master') {
               const headRepo = context.payload.pull_request.head.repo;
               const isBaseRepo =
                 headRepo?.owner?.login === context.repo.owner &&
                 headRepo?.name === context.repo.repo;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/ci.yml around lines 42 - 57, The current gate only checks
the head branch variable (branch / context.payload.pull_request.head.ref) and
treats any PR with head 'master' as a master-to-master bypass; change this to
require both head and base be 'master' by reading
context.payload.pull_request.base.ref into a variable (e.g., base) and update
the if condition from if (branch === 'master') to if (branch === 'master' &&
base === 'master') so the NO_TEST_LABEL / paired-test logic only applies to true
master-to-master PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants