Cross-reference rules: allow GitHub URLs, add crossref-audit skill for website merges - #39
Merged
jnasbyupgrade merged 4 commits intoJul 27, 2026
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
jnasbyupgrade
changed the title
…ss-reference A direct PR/commit URL to the other repo is now an accepted, and preferred, substitute for the [PGXNTOOL_COMMIT_HASH] placeholder -- it stays valid if the referenced commit is later rebased or amended, where a bare hash embedded in already-committed text would silently go stale. Also documents the fallback for when a raw hash is used anyway: update it before merge if the referenced commit gets rebased, but never rewrite an already-merged commit to fix a stale reference without explicit instruction. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…site merges /commit's two-phase cross-reference process only covers composing a PR branch's own commits before merge -- it can't help once merging happens via the GitHub website, outside AI control, which is how PRs actually land here now. There was no process for catching a paired commit that merged without its cross-reference until this was caught manually on issue #57's fix (pgxntool commit a3e4062 landed without referencing its pgxntool-test coverage in PR Postgres-Extensions#30). Adds a new crossref-audit skill: find the last release tag in each repo, list commits since it, identify genuinely paired commits (a real code+test relationship, not just similar-sounding independent fixes), and check each side references the other. Single missing tip-of-master commit newer than the last release can be fixed directly (amend + force-push, verified content-identical first); more than one missing, or anything at/before the last release, always stops and asks rather than acting. Also documents in CLAUDE.md that PR merges happen outside AI control in this workflow, and points to the new skill at session startup and before any master-rebase. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…run every round Session-startup-only was too sparse for long-running sessions. Move the mechanical parts (find last release tag, list commits since it, correlate by "issue #N" phrasing, check for a reference pattern) into audit.sh so most rounds cost a one-line read, not fresh LLM reasoning every time: - Caches the last-checked master SHAs in /tmp/pgxntool-crossref-audit-state, updated only on a clean result -- a flagged issue keeps resurfacing every round until actually fixed, rather than being silently forgotten - Correlates via "(issue #N)" phrasing specifically, not bare "(#N)" (which is usually just the local repo's own PR number and would cause false cross-repo pairings, since PR numbering is independent per repo) - Verified against real history: reports clean on the current a3e4062 fix (already carries its cross-reference) and correctly flags a synthetic missing-reference case SKILL.md and CLAUDE.md updated to point at the script and change the trigger from session-start to end-of-round. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
jnasbyupgrade
force-pushed
the
commit-format-crossref-rule
branch
from
July 26, 2026 23:30
7abd601 to
971376b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 @.claude/skills/crossref-audit/scripts/audit.sh:
- Around line 1-125: Add BATS coverage for the correlation and boundary-matching
logic in audit.sh, using fixture synthetic git repositories with commits
containing numerically overlapping issue references. Verify matching requires
the exact “issue `#N`” boundary and does not incorrectly correlate larger issue
numbers, including the regression around the matching logic near test_sha
selection.
- Line 103: Update the URL validation condition in audit.sh to derive the GitHub
organization/repository pattern from the configured upstream remote via git -C
"$PGXNTOOL_DIR" remote get-url upstream, instead of hard-coding
Postgres-Extensions/pgxntool. Preserve the existing commit/pull URL matching and
hexadecimal-reference validation behavior.
- Around line 91-108: Update the test_sha extraction in the audit loop to apply
the same non-digit-or-end boundary used by the preceding grep guard, so issue
numbers are matched exactly rather than as numeric prefixes. Preserve the
existing case-insensitive awk lookup and first-match behavior while ensuring the
selected commit corresponds to the requested issue.
- Around line 54-68: Update the commit-range construction around
last_release_tag, PGXN_TAG, TEST_TAG, and the two git log calls to explicitly
handle an empty release tag. When a repository has no usable release tag, use an
unbounded history range rather than allowing an empty tag to form
HEAD..upstream/master; preserve the existing tag-based range when a tag is
available.
In @.claude/skills/crossref-audit/SKILL.md:
- Line 12: The crossref-audit skill’s documented auto-fix flow must not write
directly to upstream/master. Update Step 4 and its associated commands to use a
reviewable branch/PR workflow, or remove the auto-fix push behavior; do not rely
on the unrestricted Bash(git:*) permission to bypass the repository’s
no-direct-master rule. Ensure the instructions no longer amend or force-push the
shared master branch.
🪄 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: b958f395-2a50-4d79-95e1-894f4ec40a4a
📒 Files selected for processing (4)
.claude/skills/commit/guides/commit-message-format.md.claude/skills/crossref-audit/SKILL.md.claude/skills/crossref-audit/scripts/audit.shCLAUDE.md
Comment on lines
+1
to
+125
| #!/usr/bin/env bash | ||
| # audit.sh [pgxntool-dir] [pgxntool-test-dir] | ||
| # | ||
| # Cheap, stateful audit for missing cross-references between paired commits | ||
| # on master in pgxntool and pgxntool-test. Meant to run every round of a | ||
| # long session, not just once at startup, without burning real tokens on | ||
| # repeat rounds: it fetches both masters, and if neither has moved since the | ||
| # last CLEAN (nothing-flagged) check, exits immediately. State is only | ||
| # updated on a clean result -- a flagged issue keeps resurfacing every round | ||
| # until it's actually fixed, rather than being silently forgotten. | ||
| # | ||
| # This is a heuristic aid, not authoritative: it correlates commits via the | ||
| # "(issue #N)" phrasing this project's commits use for cross-repo issue | ||
| # references (not bare "(#N)", which is usually just the local PR number). | ||
| # A flagged result still needs a real judgment call before acting -- see | ||
| # SKILL.md. Absence of a flag means "nothing obviously wrong found", not a | ||
| # guarantee. | ||
| # | ||
| # Arguments: | ||
| # pgxntool-dir : path to a pgxntool checkout (default: ../pgxntool) | ||
| # pgxntool-test-dir : path to a pgxntool-test checkout (default: .) | ||
| # | ||
| # Exit codes: | ||
| # 0 : clean (nothing new since last clean check, or nothing flagged) | ||
| # 1 : one or more candidate pairings flagged for review | ||
| # 2 : usage/environment error | ||
|
|
||
| set -euo pipefail | ||
|
|
||
| PGXNTOOL_DIR="${1:-${PGXNTOOL_DIR:-../pgxntool}}" | ||
| PGXNTOOL_TEST_DIR="${2:-.}" | ||
| STATE_FILE="${CROSSREF_AUDIT_STATE:-/tmp/pgxntool-crossref-audit-state}" | ||
|
|
||
| for d in "$PGXNTOOL_DIR" "$PGXNTOOL_TEST_DIR"; do | ||
| # -e (not -d): a linked worktree's .git is a FILE pointing at the main | ||
| # repo's git-dir, not a directory. | ||
| [ -e "$d/.git" ] || { echo "ERROR: '$d' is not a git checkout" >&2; exit 2; } | ||
| done | ||
|
|
||
| git -C "$PGXNTOOL_DIR" fetch upstream master --tags -q | ||
| git -C "$PGXNTOOL_TEST_DIR" fetch upstream master --tags -q | ||
|
|
||
| PGXN_HEAD=$(git -C "$PGXNTOOL_DIR" rev-parse upstream/master) | ||
| TEST_HEAD=$(git -C "$PGXNTOOL_TEST_DIR" rev-parse upstream/master) | ||
|
|
||
| if [ -f "$STATE_FILE" ]; then | ||
| read -r LAST_PGXN LAST_TEST < "$STATE_FILE" || true | ||
| if [ "${LAST_PGXN:-}" = "$PGXN_HEAD" ] && [ "${LAST_TEST:-}" = "$TEST_HEAD" ]; then | ||
| echo "crossref-audit: no new commits on either master since last clean check. Nothing to do." | ||
| exit 0 | ||
| fi | ||
| fi | ||
|
|
||
| last_release_tag() { | ||
| git -C "$1" tag --sort=-creatordate | grep -vi '^release$' | head -1 | ||
| } | ||
|
|
||
| PGXN_TAG=$(last_release_tag "$PGXNTOOL_DIR") | ||
| TEST_TAG=$(last_release_tag "$PGXNTOOL_TEST_DIR") | ||
|
|
||
| pgxn_commits=$(git -C "$PGXNTOOL_DIR" log --format='%H %s' "$PGXN_TAG"..upstream/master) | ||
| test_commits=$(git -C "$PGXNTOOL_TEST_DIR" log --format='%H %s' "$TEST_TAG"..upstream/master) | ||
|
|
||
| if [ -z "$pgxn_commits" ] && [ -z "$test_commits" ]; then | ||
| echo "crossref-audit: no commits since last release ($PGXN_TAG / $TEST_TAG) in either repo." | ||
| printf '%s %s\n' "$PGXN_HEAD" "$TEST_HEAD" > "$STATE_FILE" | ||
| exit 0 | ||
| fi | ||
|
|
||
| # Extract issue numbers from "(issue #N)"-style phrasing specifically (not | ||
| # bare "#N", which is usually just the local repo's own PR number and would | ||
| # cause false cross-repo pairings since PR numbering is independent per repo). | ||
| # | ||
| # `|| true` is required here, not cosmetic: under `set -e`, a `grep` (or a | ||
| # pipeline ending in one) that matches nothing exits 1, and since this | ||
| # function's return status is that pipeline's status, calling it for a | ||
| # subject line with no "issue #N" (the common case -- most commits don't | ||
| # reference one) would otherwise abort the whole script immediately. | ||
| extract_issue_refs() { grep -oiE 'issue #[0-9]+' <<<"$1" | grep -oE '[0-9]+' | sort -u || true; } | ||
|
|
||
| flagged=0 | ||
| report="" | ||
|
|
||
| while IFS= read -r line; do | ||
| [ -z "$line" ] && continue | ||
| sha=${line%% *} | ||
| subj=${line#* } | ||
| issues=$(extract_issue_refs "$subj") | ||
| [ -z "$issues" ] && continue | ||
| for issue in $issues; do | ||
| match=$(grep -iE "issue #${issue}([^0-9]|$)" <<<"$test_commits" || true) | ||
| [ -z "$match" ] && continue | ||
|
|
||
| body=$(git -C "$PGXNTOOL_DIR" log -1 --format='%b' "$sha") | ||
| if ! grep -qi 'pgxntool-test' <<<"$body"; then | ||
| report+=$'\n'" PGXNTOOL $sha (issue #$issue): \"$subj\" -- no mention of pgxntool-test in body" | ||
| flagged=1 | ||
| fi | ||
|
|
||
| test_sha=$(awk -v i="issue #$issue" 'BEGIN{IGNORECASE=1} $0 ~ i {print $1; exit}' <<<"$test_commits") | ||
| if [ -n "$test_sha" ]; then | ||
| test_body=$(git -C "$PGXNTOOL_TEST_DIR" log -1 --format='%b' "$test_sha") | ||
| if ! grep -qiE '[0-9a-f]{7,40}|github\.com/Postgres-Extensions/pgxntool/(pull|commit)/' <<<"$test_body"; then | ||
| report+=$'\n'" PGXNTOOL-TEST $test_sha (issue #$issue): no hash or pgxntool PR/commit URL found in body" | ||
| flagged=1 | ||
| fi | ||
| fi | ||
| done | ||
| done <<<"$pgxn_commits" | ||
|
|
||
| if [ "$flagged" -eq 1 ]; then | ||
| echo "crossref-audit: FLAGGED -- possible missing cross-reference(s):" | ||
| echo "$report" | ||
| echo | ||
| echo "Confirm each is a genuine code+test pairing before acting (a shared 'issue #N' string is a strong signal but still worth a sanity check) -- see SKILL.md for the fix-vs-ask rules. State file NOT updated; this will resurface every round until resolved." | ||
| exit 1 | ||
| fi | ||
|
|
||
| pgxn_count=$(grep -c . <<<"$pgxn_commits" || true) | ||
| test_count=$(grep -c . <<<"$test_commits" || true) | ||
| echo "crossref-audit: clean. Checked $pgxn_count pgxntool / $test_count pgxntool-test commit(s) since last release ($PGXN_TAG / $TEST_TAG)." | ||
| printf '%s %s\n' "$PGXN_HEAD" "$TEST_HEAD" > "$STATE_FILE" | ||
| exit 0 | ||
|
|
||
| # vi: expandtab ts=2 sw=2 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
No automated test coverage for the correlation/heuristic logic.
The issue-correlation and boundary-matching logic (the exact area with the bug flagged above) has no BATS coverage despite this repo already hosting a BATS test harness. A small fixture-based test (synthetic git repos with numerically-overlapping issue numbers) would have caught the line 100 regression.
🤖 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 @.claude/skills/crossref-audit/scripts/audit.sh around lines 1 - 125, Add
BATS coverage for the correlation and boundary-matching logic in audit.sh, using
fixture synthetic git repositories with commits containing numerically
overlapping issue references. Verify matching requires the exact “issue `#N`”
boundary and does not incorrectly correlate larger issue numbers, including the
regression around the matching logic near test_sha selection.
Comment on lines
+54
to
+68
| last_release_tag() { | ||
| git -C "$1" tag --sort=-creatordate | grep -vi '^release$' | head -1 | ||
| } | ||
|
|
||
| PGXN_TAG=$(last_release_tag "$PGXNTOOL_DIR") | ||
| TEST_TAG=$(last_release_tag "$PGXNTOOL_TEST_DIR") | ||
|
|
||
| pgxn_commits=$(git -C "$PGXNTOOL_DIR" log --format='%H %s' "$PGXN_TAG"..upstream/master) | ||
| test_commits=$(git -C "$PGXNTOOL_TEST_DIR" log --format='%H %s' "$TEST_TAG"..upstream/master) | ||
|
|
||
| if [ -z "$pgxn_commits" ] && [ -z "$test_commits" ]; then | ||
| echo "crossref-audit: no commits since last release ($PGXN_TAG / $TEST_TAG) in either repo." | ||
| printf '%s %s\n' "$PGXN_HEAD" "$TEST_HEAD" > "$STATE_FILE" | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
No-release-tag case silently falls back to HEAD..upstream/master instead of "all history".
If last_release_tag returns empty (no tags yet, or all tags are literally named release), "$PGXN_TAG"..upstream/master collapses to ..upstream/master, which git treats as HEAD..upstream/master — not the intended unbounded range. In a checkout whose local HEAD differs from upstream/master, this silently narrows or skews the audited commit window instead of erroring or scanning full history.
🔧 Suggested guard
PGXN_TAG=$(last_release_tag "$PGXNTOOL_DIR")
TEST_TAG=$(last_release_tag "$PGXNTOOL_TEST_DIR")
+
+if [ -z "$PGXN_TAG" ] || [ -z "$TEST_TAG" ]; then
+ echo "ERROR: no release tag found in one or both repos" >&2
+ exit 2
+fi📝 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
| last_release_tag() { | |
| git -C "$1" tag --sort=-creatordate | grep -vi '^release$' | head -1 | |
| } | |
| PGXN_TAG=$(last_release_tag "$PGXNTOOL_DIR") | |
| TEST_TAG=$(last_release_tag "$PGXNTOOL_TEST_DIR") | |
| pgxn_commits=$(git -C "$PGXNTOOL_DIR" log --format='%H %s' "$PGXN_TAG"..upstream/master) | |
| test_commits=$(git -C "$PGXNTOOL_TEST_DIR" log --format='%H %s' "$TEST_TAG"..upstream/master) | |
| if [ -z "$pgxn_commits" ] && [ -z "$test_commits" ]; then | |
| echo "crossref-audit: no commits since last release ($PGXN_TAG / $TEST_TAG) in either repo." | |
| printf '%s %s\n' "$PGXN_HEAD" "$TEST_HEAD" > "$STATE_FILE" | |
| exit 0 | |
| fi | |
| last_release_tag() { | |
| git -C "$1" tag --sort=-creatordate | grep -vi '^release$' | head -1 | |
| } | |
| PGXN_TAG=$(last_release_tag "$PGXNTOOL_DIR") | |
| TEST_TAG=$(last_release_tag "$PGXNTOOL_TEST_DIR") | |
| if [ -z "$PGXN_TAG" ] || [ -z "$TEST_TAG" ]; then | |
| echo "ERROR: no release tag found in one or both repos" >&2 | |
| exit 2 | |
| fi | |
| pgxn_commits=$(git -C "$PGXNTOOL_DIR" log --format='%H %s' "$PGXN_TAG"..upstream/master) | |
| test_commits=$(git -C "$PGXNTOOL_TEST_DIR" log --format='%H %s' "$TEST_TAG"..upstream/master) | |
| if [ -z "$pgxn_commits" ] && [ -z "$test_commits" ]; then | |
| echo "crossref-audit: no commits since last release ($PGXN_TAG / $TEST_TAG) in either repo." | |
| printf '%s %s\n' "$PGXN_HEAD" "$TEST_HEAD" > "$STATE_FILE" | |
| exit 0 | |
| fi |
🤖 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 @.claude/skills/crossref-audit/scripts/audit.sh around lines 54 - 68, Update
the commit-range construction around last_release_tag, PGXN_TAG, TEST_TAG, and
the two git log calls to explicitly handle an empty release tag. When a
repository has no usable release tag, use an unbounded history range rather than
allowing an empty tag to form HEAD..upstream/master; preserve the existing
tag-based range when a tag is available.
Comment on lines
+91
to
+108
| match=$(grep -iE "issue #${issue}([^0-9]|$)" <<<"$test_commits" || true) | ||
| [ -z "$match" ] && continue | ||
|
|
||
| body=$(git -C "$PGXNTOOL_DIR" log -1 --format='%b' "$sha") | ||
| if ! grep -qi 'pgxntool-test' <<<"$body"; then | ||
| report+=$'\n'" PGXNTOOL $sha (issue #$issue): \"$subj\" -- no mention of pgxntool-test in body" | ||
| flagged=1 | ||
| fi | ||
|
|
||
| test_sha=$(awk -v i="issue #$issue" 'BEGIN{IGNORECASE=1} $0 ~ i {print $1; exit}' <<<"$test_commits") | ||
| if [ -n "$test_sha" ]; then | ||
| test_body=$(git -C "$PGXNTOOL_TEST_DIR" log -1 --format='%b' "$test_sha") | ||
| if ! grep -qiE '[0-9a-f]{7,40}|github\.com/Postgres-Extensions/pgxntool/(pull|commit)/' <<<"$test_body"; then | ||
| report+=$'\n'" PGXNTOOL-TEST $test_sha (issue #$issue): no hash or pgxntool PR/commit URL found in body" | ||
| flagged=1 | ||
| fi | ||
| fi | ||
| done |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Unbounded regex in test_sha extraction can pick the wrong commit for issue numbers that are numeric prefixes of each other.
Line 91's guard correctly uses a boundary (issue #${issue}([^0-9]|$)), but line 100's awk extraction drops that boundary and matches issue #$issue as a plain substring. For issue=2, a commit subject with (issue #21) also matches issue #2`` as a substring. Since git log lists newest-first and `awk` exits on the first match, if the `#21` commit is newer than the true `#2` match, `test_sha` resolves to the wrong commit — and its body then gets checked (for hash/URL presence) instead of the actual paired commit's body, producing an incorrect flagged/clean result.
🐛 Proposed fix: reuse the bounded pattern
- test_sha=$(awk -v i="issue #$issue" 'BEGIN{IGNORECASE=1} $0 ~ i {print $1; exit}' <<<"$test_commits")
+ test_sha=$(grep -iE "issue #${issue}([^0-9]|\$)" <<<"$test_commits" | head -1 | cut -d' ' -f1)📝 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
| match=$(grep -iE "issue #${issue}([^0-9]|$)" <<<"$test_commits" || true) | |
| [ -z "$match" ] && continue | |
| body=$(git -C "$PGXNTOOL_DIR" log -1 --format='%b' "$sha") | |
| if ! grep -qi 'pgxntool-test' <<<"$body"; then | |
| report+=$'\n'" PGXNTOOL $sha (issue #$issue): \"$subj\" -- no mention of pgxntool-test in body" | |
| flagged=1 | |
| fi | |
| test_sha=$(awk -v i="issue #$issue" 'BEGIN{IGNORECASE=1} $0 ~ i {print $1; exit}' <<<"$test_commits") | |
| if [ -n "$test_sha" ]; then | |
| test_body=$(git -C "$PGXNTOOL_TEST_DIR" log -1 --format='%b' "$test_sha") | |
| if ! grep -qiE '[0-9a-f]{7,40}|github\.com/Postgres-Extensions/pgxntool/(pull|commit)/' <<<"$test_body"; then | |
| report+=$'\n'" PGXNTOOL-TEST $test_sha (issue #$issue): no hash or pgxntool PR/commit URL found in body" | |
| flagged=1 | |
| fi | |
| fi | |
| done | |
| match=$(grep -iE "issue #${issue}([^0-9]|$)" <<<"$test_commits" || true) | |
| [ -z "$match" ] && continue | |
| body=$(git -C "$PGXNTOOL_DIR" log -1 --format='%b' "$sha") | |
| if ! grep -qi 'pgxntool-test' <<<"$body"; then | |
| report+=$'\n'" PGXNTOOL $sha (issue #$issue): \"$subj\" -- no mention of pgxntool-test in body" | |
| flagged=1 | |
| fi | |
| test_sha=$(grep -iE "issue #${issue}([^0-9]|\$)" <<<"$test_commits" | head -1 | cut -d' ' -f1) | |
| if [ -n "$test_sha" ]; then | |
| test_body=$(git -C "$PGXNTOOL_TEST_DIR" log -1 --format='%b' "$test_sha") | |
| if ! grep -qiE '[0-9a-f]{7,40}|github\.com/Postgres-Extensions/pgxntool/(pull|commit)/' <<<"$test_body"; then | |
| report+=$'\n'" PGXNTOOL-TEST $test_sha (issue #$issue): no hash or pgxntool PR/commit URL found in body" | |
| flagged=1 | |
| fi | |
| fi | |
| done |
🤖 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 @.claude/skills/crossref-audit/scripts/audit.sh around lines 91 - 108, Update
the test_sha extraction in the audit loop to apply the same non-digit-or-end
boundary used by the preceding grep guard, so issue numbers are matched exactly
rather than as numeric prefixes. Preserve the existing case-insensitive awk
lookup and first-match behavior while ensuring the selected commit corresponds
to the requested issue.
| test_sha=$(awk -v i="issue #$issue" 'BEGIN{IGNORECASE=1} $0 ~ i {print $1; exit}' <<<"$test_commits") | ||
| if [ -n "$test_sha" ]; then | ||
| test_body=$(git -C "$PGXNTOOL_TEST_DIR" log -1 --format='%b' "$test_sha") | ||
| if ! grep -qiE '[0-9a-f]{7,40}|github\.com/Postgres-Extensions/pgxntool/(pull|commit)/' <<<"$test_body"; then |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Hardcoded GitHub org/repo in the URL pattern.
Postgres-Extensions/pgxntool is embedded directly in the regex rather than derived from the configured upstream remote. As per coding guidelines, "Do not hard-code values that can be determined dynamically." Deriving it from git -C "$PGXNTOOL_DIR" remote get-url upstream would make the check resilient to a fork/rename without code changes.
🤖 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 @.claude/skills/crossref-audit/scripts/audit.sh at line 103, Update the URL
validation condition in audit.sh to derive the GitHub organization/repository
pattern from the configured upstream remote via git -C "$PGXNTOOL_DIR" remote
get-url upstream, instead of hard-coding Postgres-Extensions/pgxntool. Preserve
the existing commit/pull URL matching and hexadecimal-reference validation
behavior.
Source: Coding guidelines
| Use when: at the end of each round of work in this project (not just | ||
| session start — sessions run long), before rebasing a branch onto a fresh | ||
| master fetch, or when asked to check/audit cross-references. | ||
| allowed-tools: Bash(git:*), Bash(gh:*), Bash(bash .claude/skills/crossref-audit/scripts/audit.sh:*), Read |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Step 4's auto-fix pushes directly to master, contradicting the project's "never commit directly to master" rule.
The documented fix flow amends an already-merged commit and force-pushes it straight to upstream/master (line 86), enabled by the unrestricted Bash(git:*) grant in allowed-tools (line 12). This is safeguarded (single tip commit, message-only diff check, --force-with-lease with expected old SHA, immediate refetch), but it still bypasses the PR review process entirely for a write to the shared/protected branch. The repo-wide guideline is unconditional: "never commit directly to master, and treat merging as an operation performed outside AI control." If this narrow exception is intentional, the top-level guideline should be updated to document the carve-out explicitly; as written, the skill's own documented behavior conflicts with it. SkillSpector's flag on the --force-with-lease at line 86 is a secondary signal here — the lease/expected-SHA check mitigates the classic force-push data-loss risk, but doesn't address the direct-to-master write itself.
Also applies to: 76-98
🧰 Tools
🪛 SkillSpector (2.3.11)
[error] 86: [TM1] Tool Parameter Abuse: Tool parameters are crafted to achieve unintended or unsafe behavior. Parameter abuse can bypass intended safety checks (e.g. shell=True, --force, dangerous glob patterns).
Remediation: Validate all tool parameters against an allowlist. Reject dangerous parameter values (shell=True, --force, -rf /) and use safe defaults.
(Tool Misuse (TM1))
🤖 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 @.claude/skills/crossref-audit/SKILL.md at line 12, The crossref-audit
skill’s documented auto-fix flow must not write directly to upstream/master.
Update Step 4 and its associated commands to use a reviewable branch/PR
workflow, or remove the auto-fix push behavior; do not rely on the unrestricted
Bash(git:*) permission to bypass the repository’s no-direct-master rule. Ensure
the instructions no longer amend or force-push the shared master branch.
Source: Coding guidelines
Address CodeRabbit review on PR Postgres-Extensions#39: - Fix test_sha extraction (line 100) to reuse the already-bounded $match from the preceding grep instead of an unbounded awk substring match, which could pick the wrong commit when one issue number is a numeric prefix of another (e.g. issue #2 matching a commit for issue Postgres-Extensions#21). - Guard against an empty release tag collapsing "$TAG"..upstream/master to plain ..upstream/master, which git silently reinterprets as HEAD..upstream/master instead of an unbounded range. - Derive the pgxntool org/repo path from the upstream remote instead of hardcoding Postgres-Extensions/pgxntool, so URL-reference detection keeps working under a fork or rename. Also reconciles CLAUDE.md's "Claude never commits directly to master" statement with crossref-audit Step 4's narrow, safety-gated exception (single tip commit, verified content-identical, never at/before a release tag) by documenting the carve-out explicitly. Skipped: adding BATS fixture coverage for the correlation/boundary logic (nitpick, heavy lift) -- no existing precedent in this suite for testing Claude skill scripts via synthetic git repos, and the actual bug it would have caught is fixed directly above. Changes only in pgxntool-test. No related changes in pgxntool. Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
Author
|
Addressed CodeRabbit's review in 5a5a0f4:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A direct PR/commit URL to the other repo is now an accepted, and preferred, substitute for the
[PGXNTOOL_COMMIT_HASH]placeholder -- it stays valid if the referenced commit is later rebased or amended, where a bare hash embedded in already-committed text would silently go stale. Also documents the fallback for when a raw hash is used anyway: update it before merge if the referenced commit gets rebased, but never rewrite an already-merged commit to fix a stale reference without explicit instruction./commit's two-phase cross-reference process only covers composing a PR branch's own commits before merge -- it can't help once merging happens via the GitHub website, outside AI control, which is how PRs actually land here now. There was no process for catching a paired commit that merged without its cross-reference until this was caught manually on issue #57's fix (pgxntool commita3e4062landed without referencing its pgxntool-test coverage in PR #30).Adds a new
crossref-auditskill: find the last release tag in each repo, list commits since it, identify genuinely paired commits (a real code+test relationship, not just similar-sounding independent fixes), and check each side references the other.Also documents in CLAUDE.md that PR merges happen outside AI control in this workflow, and points to the new skill at session startup and before any master-rebase.
Doc/skill-only change, no code changes.