Add tests for pgxntool-version.sh and the pgxntool-version make target - #43
Add tests for pgxntool-version.sh and the pgxntool-version make target#43jnasbyupgrade wants to merge 11 commits into
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 |
Covers HISTORY.asc parsing (stamped version, STABLE/unreleased checkout, missing file, empty file, default path) in isolation, plus an integration test that make pgxntool-version matches the embedded pgxntool/HISTORY.asc inside a real foundation repo. Also adds pgxntool/pgxntool-version.sh to dist-expected-files.txt, since pgxntool now ships that new script. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Covers a lowercase 'stable', a v-prefixed version, a pre-release suffix, a truncated X.Y version, and an arbitrary non-version string. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Mark the test that runs pgxntool-version.sh against the real, unmodified ../pgxntool checkout as CRITICAL, and strengthen it to an exact match against HISTORY.asc's first line instead of a non-empty check. This is the test CI relies on to catch a release PR that stamps HISTORY.asc incorrectly, especially once the top line is no longer STABLE. Also add a Step 7 to the /release skill that exercises the real 'make pgxntool-version' target (via a symlinked scratch dir, not a copy) against the just-stamped HISTORY.asc before tagging and pushing, as a safeguard against a bad stamp shipping. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1e8f440 to
00a17d7
Compare
pgxntool-version.sh was renamed to bin/version (companion change in pgxntool PR #73). Update the test script path, dist manifest entry, and simplify the release skill's Step 7 to call the script directly instead of exercising it through a throwaway scratch Makefile. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… parallel subagents Numbered steps (Step 2, Step 6, ...) required hand-renumbering every cross-reference whenever a step was added, removed, or reordered -- exactly what happened when this same commit added a HISTORY.asc reordering step. Steps are now referenced by their heading name instead, which stays correct no matter how the list changes. Also adds a step to sort each release's STABLE entries by importance (breaking changes, then non-breaking behavior changes, then new features, then bugfixes) before stamping, and a Process Notes section noting where independent work (the two review agents, and the two repos' tag/push steps) can run as parallel subagents instead of sequentially. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Bolded prose references ('**Update HISTORY.asc and Commit**') were an
improvement over numbered steps but still just text -- nothing caught
them drifting out of sync with the actual heading, and they gave a
human reading this file on GitHub nothing to click. Real markdown
links to each heading's anchor are unambiguous (a broken one is
visibly broken) and clickable in GitHub's rendered view.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The old flow committed the stamp directly to local master and pushed straight to upstream master/tags. pgxntool's CI only triggers on pull_request, so that path never ran CI at all -- a release could ship with a broken stamp, a regressed bin/version, or anything else CI would normally catch, and nobody would know until later. Both repos now get a release-VERSION branch and a real PR. Using the same branch name on the same (upstream) account for both repos lets pgxntool-test's CI automatically pair its release PR with pgxntool's, which is normally enough to satisfy pgxntool's check-test-pr gate without needing the commit-with-no-tests label -- though the skill still tells the user to apply it if that pairing doesn't take for some reason, since this skill is not allowed to apply that label itself. Tagging moves to after the human merges both PRs, and re-reads HISTORY.asc from the merged master rather than the pre-merge branch, since a squash or rebase merge can change what actually lands. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/release/SKILL.md:
- Around line 298-315: Reverse the PR creation order in the release
instructions: execute the pgxntool-test commit, push, and gh pr create block
before the pgxntool block. Preserve the existing commands and release-version
substitutions while ensuring the companion test PR exists before pgxntool CI
starts.
- Around line 393-401: Update the release completion flow in “Verify and Report”
to run the cross-reference audit for both pgxntool and pgxntool-test at the end
of each work round, before any rebasing onto freshly fetched master and before
deleting branches or reporting success. Reuse the existing audit procedure and
ensure completion proceeds only after both repositories pass it.
- Around line 266-270: Update the release instructions around both git commit
commands to require explicit user confirmation immediately before execution.
Ensure a release request alone does not authorize either commit, and only run
the commands after the user explicitly approves committing the changes.
- Around line 182-184: Update the no-STABLE handling in the release workflow to
prevent continuation unless a STABLE section is created and validated. After
warning the user, either stop the release process or require creation of the
missing STABLE heading before proceeding to stamping; ensure the same recovery
behavior is applied to the corresponding later branch.
- Around line 254-263: Update the HISTORY example in SKILL.md by adding blank
lines immediately before and after each fenced code block and specifying a
language identifier such as text on every fence. Preserve the example’s content,
including the VERSION heading.
- Around line 300-310: Update the release instructions around the git push
commands and the referenced branch/tag/retry sections to invoke monitor-ci.sh
immediately after every push, passing the exact pushed commit SHA when
available. When pushes target both repositories, run the monitors concurrently
and verify that each reported branch matches its pushed branch.
In `@test/standard/pgxntool-version.bats`:
- Around line 21-22: Update the version test setup and history-reading logic to
use the normalized, branch-aware PGXNREPO checkout: set VERSION_SCRIPT to
"$PGXNREPO/bin/version" and read "$PGXNREPO/HISTORY.asc" instead of deriving
paths from TOPDIR. Revise the adjacent comment to describe the PGXNREPO-based
source.
🪄 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: d3d2e3cd-0e1f-4529-b137-5b9d7a100bd0
📒 Files selected for processing (3)
.claude/skills/release/SKILL.mdtest/lib/dist-expected-files.txttest/standard/pgxntool-version.bats
| Replace: | ||
| ``` | ||
| STABLE | ||
| ------ | ||
| ``` | ||
| With: | ||
| ``` | ||
| VERSION | ||
| ------- | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the Markdown fences in the HISTORY example.
The fenced examples lack language identifiers and the required blank lines around fences, triggering markdownlint MD031 and MD040. Use blank lines and a language such as text.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 255-255: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 255-255: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 258-258: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 260-260: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 260-260: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 263-263: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 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/release/SKILL.md around lines 254 - 263, Update the HISTORY
example in SKILL.md by adding blank lines immediately before and after each
fenced code block and specifying a language identifier such as text on every
fence. Preserve the example’s content, including the VERSION heading.
Source: Linters/SAST tools
| ### Commit | ||
|
|
||
| 2. Commit: | ||
| ```bash | ||
| cd ../pgxntool && git commit -am "Stamp VERSION" | ||
| ``` | ||
| ```bash | ||
| cd ../pgxntool && git commit -am "Stamp VERSION" | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Require explicit confirmation before committing.
Both git commit commands execute unconditionally. Add a user-confirmation gate immediately before these commands; a release request alone is not an explicit instruction to commit.
As per coding guidelines, never commit changes without explicit user instruction.
Also applies to: 307-310
🤖 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/release/SKILL.md around lines 266 - 270, Update the release
instructions around both git commit commands to require explicit user
confirmation immediately before execution. Ensure a release request alone does
not authorize either commit, and only run the commands after the user explicitly
approves committing the changes.
Source: Coding guidelines
| ```bash | ||
| cd ../pgxntool | ||
| git push PGXNTOOL_UPSTREAM release-VERSION | ||
| gh pr create --repo Postgres-Extensions/pgxntool \ | ||
| --base master --head release-VERSION \ | ||
| --title "Release VERSION" \ | ||
| --body "Stamps HISTORY.asc for VERSION. Companion: pgxntool-test release-VERSION." | ||
| ``` | ||
|
|
||
| ```bash | ||
| cd ../pgxntool-test | ||
| git commit --allow-empty -m "Stamp VERSION" | ||
| git push PGXNTOOL_TEST_UPSTREAM release-VERSION | ||
| gh pr create --repo Postgres-Extensions/pgxntool-test \ | ||
| --base master --head release-VERSION \ | ||
| --title "Release VERSION" \ | ||
| --body "Companion stamp for pgxntool release-VERSION." | ||
| ``` |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Open the pgxntool-test PR before the pgxntool PR.
The commands create the pgxntool PR first, while the following guidance says the test PR must be opened first to avoid check-test-pr missing its paired PR. Swap the two command blocks or otherwise guarantee the test PR exists before pgxntool CI starts.
Also applies to: 322-325
🤖 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/release/SKILL.md around lines 298 - 315, Reverse the PR
creation order in the release instructions: execute the pgxntool-test commit,
push, and gh pr create block before the pgxntool block. Preserve the existing
commands and release-version substitutions while ensuring the companion test PR
exists before pgxntool CI starts.
| git push PGXNTOOL_UPSTREAM release-VERSION | ||
| gh pr create --repo Postgres-Extensions/pgxntool \ | ||
| --base master --head release-VERSION \ | ||
| --title "Release VERSION" \ | ||
| --body "Stamps HISTORY.asc for VERSION. Companion: pgxntool-test release-VERSION." | ||
| ``` | ||
|
|
||
| ```bash | ||
| cd ../pgxntool-test | ||
| git commit --allow-empty -m "Stamp VERSION" | ||
| git push PGXNTOOL_TEST_UPSTREAM release-VERSION |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Monitor CI immediately after every push.
The branch, tag, and retry push instructions do not invoke monitor-ci.sh with the exact pushed SHA. Add that invocation immediately after each git push; when both repositories are pushed, monitor them concurrently and verify the reported branches match.
As per coding guidelines, after every git push, immediately run the CI monitor with the exact pushed SHA when available.
Also applies to: 353-370, 435-437
🤖 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/release/SKILL.md around lines 300 - 310, Update the release
instructions around the git push commands and the referenced branch/tag/retry
sections to invoke monitor-ci.sh immediately after every push, passing the exact
pushed commit SHA when available. When pushes target both repositories, run the
monitors concurrently and verify that each reported branch matches its pushed
branch.
Source: Coding guidelines
| ## Verify and Report | ||
|
|
||
| Both repos are already on master with the release branch merged in (see | ||
| [Tag Both Repos](#tag-both-repos)); delete the now-merged local release | ||
| branches: | ||
|
|
||
| ```bash | ||
| cd ../pgxntool && git checkout master | ||
| cd ../pgxntool-test && git checkout master | ||
| cd ../pgxntool && git branch -d release-VERSION | ||
| cd ../pgxntool-test && git branch -d release-VERSION |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Add the required cross-reference audit before completion.
The completion path deletes local branches and reports success without running the cross-reference audit for both repositories. Add the audit before final reporting and before any rebasing onto freshly fetched master.
As per coding guidelines, run the cross-reference audit for both repositories at the end of each work round and before rebasing onto freshly fetched master.
🤖 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/release/SKILL.md around lines 393 - 401, Update the release
completion flow in “Verify and Report” to run the cross-reference audit for both
pgxntool and pgxntool-test at the end of each work round, before any rebasing
onto freshly fetched master and before deleting branches or reporting success.
Reuse the existing audit procedure and ensure completion proceeds only after
both repositories pass it.
Source: Coding guidelines
| export VERSION_SCRIPT="$TOPDIR/../pgxntool/bin/version" | ||
| export SCRATCH_DIR="$TEST_DIR/pgxntool-version-tests" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the configured PGXNREPO checkout.
Lines 21 and 84 bypass the normalized, branch-aware PGXNREPO exported by load_test_env. A configured checkout can therefore test a different sibling clone—or fail when $TOPDIR/../pgxntool does not exist. Set VERSION_SCRIPT="$PGXNREPO/bin/version" and read "$PGXNREPO/HISTORY.asc"; update the adjacent comment accordingly.
Also applies to: 72-88
🤖 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 `@test/standard/pgxntool-version.bats` around lines 21 - 22, Update the version
test setup and history-reading logic to use the normalized, branch-aware
PGXNREPO checkout: set VERSION_SCRIPT to "$PGXNREPO/bin/version" and read
"$PGXNREPO/HISTORY.asc" instead of deriving paths from TOPDIR. Revise the
adjacent comment to describe the PGXNREPO-based source.
There was a problem hiding this comment.
Claude: I think the bigger take-away here is that you should NEVER be copying things unless absolutely needed.
There was a problem hiding this comment.
@jnasbyupgrade Agreed. The core rule is to use the canonical configured source directly and avoid copies unless isolation or mutation makes one strictly necessary. I’ll apply that principle in future reviews and test guidance.
🐇💭
✏️ Learnings added
Learnt from: jnasbyupgrade
Repo: Postgres-Extensions/pgxntool-test PR: 43
File: test/standard/pgxntool-version.bats:21-22
Timestamp: 2026-07-28T21:19:57.169Z
Learning: In the pgxntool test suite and related repository work, never copy files or repository content unless it is absolutely necessary. Prefer operating on the canonical configured source (for example, the normalized `PGXNREPO` checkout) so tests preserve real path, branch, and configuration behavior. Use isolated copies only when mutation or genuine test isolation requires them.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
@claude Code Rabbit has raised a bunch of concerns, almost all of which are legit. You missed all of this. What do we need to do to improve your win rate at reviewing PRs? |
|
Claude finished @jnasbyupgrade's task in 4m 57s —— View job Analysis complete
I checked every finding against the actual content committed at Real bugs, verified:
Why these got missed: all 7 live in the last commit of an 8-commit PR ( Concretely, to raise the win rate:
No code changes made — this was a review/analysis request, not a fix request. Separately, I noticed the working tree currently has an uncommitted, unpushed local edit to |
- Confirm HISTORY.asc: a missing STABLE section no longer has a dead-end 'continue anyway' path -- the user must either add a STABLE section now or stop, since later steps can't proceed without one (r3669108534 et al). - Fix Markdown fences lacking blank lines/language identifiers (markdownlint MD031/MD040) in the HISTORY.asc example and the final report output (r3669108538). - Open Release Pull Requests: the pgxntool-test push/PR block now actually comes first in the instructions, matching the prose that says it must (r3669108544). Each push now has an explicit monitor-ci.sh invocation immediately after it, and the CI-retry path in Error Handling does the same (r3669108551). - Tag Both Repos: run the crossref-audit script before rebasing onto a freshly fetched master, per this repo's own CLAUDE.md rule (r3669108556). - Process Notes: added a note on why the commits in this flow don't need a separate confirmation gate -- invoking /release is itself the explicit instruction, and nothing here touches master without a human-merged PR in between (r3669108542). test/standard/pgxntool-version.bats: use the normalized $PGXNREPO (set by load_test_env) instead of $TOPDIR/../pgxntool, so the test follows the same branch/path resolution as the rest of the suite instead of a hardcoded relative path (r3669108562). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The 'open pgxntool-test first' instruction from the previous commit doesn't actually solve anything: check-test-pr checks doc-only status from the PR's own file list before it ever looks for a paired PR, and a release-stamp diff (HISTORY.asc/README.asc/README.html) is always doc-only, so the bypass fires regardless of push order. Worse, the ordering requirement directly contradicted this file's own Process Notes guidance to run the two repos' pushes as parallel subagents. Also note the underlying pairing-search race is a general latent issue in check-test-pr for any paired PR, not release-specific -- just more likely to bite a release PR since those aren't iterated on the way a normal PR is. Not something this skill needs to work around, since it never relies on that search succeeding. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@claude you still working on #43 (comment) ? It's been an hour... |
|
Claude finished @jnasbyupgrade's task in 21s —— View job Status check
That task already finished — it completed in 4m57s, right after it started (comment posted at 2026-07-28T21:21:00Z: "Claude finished @jnasbyupgrade's task..."). It was a review/analysis request, not an implementation request, so per the task instructions no code changes were made — just the write-up confirming CodeRabbit's 7 findings were legitimate and explaining why they were missed. Nothing has been running in the background since then. If you'd like me to now implement fixes for those findings (PR order in |
Re-examined pgxntool's check-test-pr logic precisely: for a doc-only PR (which a release stamp always is), it skips the actual Postgres test matrix unconditionally, with no fallback to testing against master -- regardless of whether a paired pgxntool-test PR exists. So without a companion PR, the release content would get zero CI test coverage. What actually provides that coverage: an empty commit is NOT doc-only by pgxntool-test's own check (zero changed files trips a different guard there), so its own test job runs and resolves the paired pgxntool branch -- exercising the real Postgres suite against the actual release content via pgxntool-test's CI instead of pgxntool's. Since that PR never has anything worth landing, it must never be merged -- title/body now say so explicitly, and the flow closes it (gh pr close --delete-branch) instead of merging it. pgxntool-test's tag goes directly on its master, which never changed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Summary
make pgxntool-version).pgxntool-version.sh's HISTORY.asc parsing: stamped version, STABLE/unreleased checkout, missing file, empty file, default path.make pgxntool-versionmatches the embeddedpgxntool/HISTORY.ascin a real foundation repo.pgxntool/pgxntool-version.shtodist-expected-files.txtsince pgxntool now ships that file.Test plan
🤖 Generated with Claude Code