refactor(create-pr): improve quality, DRY code, strengthen tests - #580
Conversation
…provement Skill for improving a file (prompt, config, template) toward reference quality through repeated single-change A/B testing. Each iteration makes exactly one change, tests it against reference examples via parallel generation + judge evaluation, and adopts only what works. Key design decisions: - One change per iteration (bundled changes hide what helped/hurt) - Dynamic next-change based on judge feedback (not pre-planned list) - Same inputs every iteration for valid comparison - Adopt on TIE (change that doesn't hurt is worth keeping) - Stop after 2+ consecutive rejections (rethink approach)
Replace eval-specific iterative-eval with a general-purpose iterate skill. Works for any incremental improvement: prompts, code, configs, docs — anything where you can verify whether a change helped. Key design: - One change per iteration (clear signal) - Pluggable verification (test, build, judge, user, metric, composite) - Feedback-driven next change (not pre-planned) - Stop on 2+ consecutive rejections BREAKING CHANGE: removes iterative-eval skill
Baseline: 90/100 — shellcheck 0, tests 100%, code_quality 15/25, skillmd 25/25
Result: {"status":"keep","quality_score":95,"code_quality":20}
New baseline: 93/100 with advanced quality checks
DRY: resolve_base_branch() replaces 3x duplicated detection logic
Result: {"status":"keep","quality_score":98,"advanced_quality":19}
…th-base tests
SKILL.md now documents sync-with-base and verify-pr-status recovery paths.
Added 5 new BATS tests for sync-with-base.sh.
Result: {"status":"keep","quality_score":100,"total_tests":28}
…ests
DRY: git-repo check now shared via require_git_repo().
Added 5 lib.sh tests (33 total).
Result: {"status":"keep","quality_score":100,"total_tests":33}
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis pull request refactors the create-pr script suite by extracting shared utility functions ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Test now recognizes both direct git rev-parse and shared require_git_repo.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
tests/skills/test_create_pr_verify_status.bats (3)
26-33: Consider using teardown for temp directory cleanup.If the test assertions fail, the
rm -rf "$TEMP_DIR"line won't execute, leaving orphaned temp directories. Consider using BATS's teardown mechanism or a trap.🧹 More robust cleanup pattern
`@test` "lib.sh: require_git_repo exits 2 outside git repo" { TEMP_DIR=$(mktemp -d) + # Ensure cleanup on test exit + trap "rm -rf '$TEMP_DIR'" RETURN cd "$TEMP_DIR" run env -u GIT_DIR -u GIT_WORK_TREE bash -c "source '$LIB_SCRIPT' && require_git_repo" [ "$status" -eq 2 ] [[ "$output" =~ "Not in a git repository" ]] - rm -rf "$TEMP_DIR" }Alternatively, consider storing temp dirs in a test-level variable and cleaning up in a shared teardown function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/skills/test_create_pr_verify_status.bats` around lines 26 - 33, The test "lib.sh: require_git_repo exits 2 outside git repo" currently removes TEMP_DIR at the end of the test which will not run if assertions fail; replace the inline rm -rf cleanup with a BATS teardown (or a shell trap) that removes the TEMP_DIR after each test: create a teardown function (or add a trap) that checks and deletes the TEMP_DIR variable set in the test before exiting, and ensure the test still sets TEMP_DIR and calls require_git_repo so cleanup runs regardless of assertion outcomes.
95-102: Same cleanup concern as lib.sh tests.Apply the same
trappattern here for robust temp directory cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/skills/test_create_pr_verify_status.bats` around lines 95 - 102, The test "sync-with-base.sh: exits 2 when not in a git repository" creates a TEMP_DIR but doesn’t guarantee cleanup on failure; add the same trap pattern used in lib.sh tests: after creating TEMP_DIR with TEMP_DIR=$(mktemp -d) register a trap like trap 'rm -rf "$TEMP_DIR"' EXIT (or using a cleanup function) so the directory is removed on test exit, then cd into "$TEMP_DIR" and run the test using SYNC_SCRIPT; ensure you remove the explicit rm -rf "$TEMP_DIR" at the end since the trap covers cleanup.
143-151: Test assertion may be fragile.This test runs
preflight-check.shwithout a base branch argument outside a git repo. However, with the lib.sh integration,require_git_reporuns first (line 15 of preflight-check.sh) and exits with "Not in a git repository", not with a generic "ERROR" about the base branch. The test passes because[[ "$output" =~ "ERROR" ]]matches, but the description "exits 2 when no base branch given and gh fails" doesn't match the actual behavior being tested.Consider updating the test name and assertion to match the actual behavior, or restructure to test base branch resolution separately (as done in
lib.sh: resolve_base_branchtests).📝 Suggested clarification
-@test "preflight-check.sh: exits 2 when no base branch given and gh fails" { +@test "preflight-check.sh: exits 2 outside git repo (no args)" { PREFLIGHT_SCRIPT="${BATS_TEST_DIRNAME}/../../plugins/me/skills/create-pr/scripts/preflight-check.sh" TEMP_DIR=$(mktemp -d) cd "$TEMP_DIR" run env -u GIT_DIR -u GIT_WORK_TREE "$PREFLIGHT_SCRIPT" [ "$status" -eq 2 ] - [[ "$output" =~ "ERROR" ]] + [[ "$output" =~ "Not in a git repository" ]] rm -rf "$TEMP_DIR" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/skills/test_create_pr_verify_status.bats` around lines 143 - 151, The test "preflight-check.sh: exits 2 when no base branch given and gh fails" is asserting a generic "ERROR" but actually triggers require_git_repo in preflight-check.sh which prints "Not in a git repository"; either update the test name and assertion to expect the repo-related behavior (change the test title and assert output contains "Not in a git repository" and status 2), or modify the test to run inside a git repo (git init in TEMP_DIR) so preflight-check.sh proceeds to base-branch resolution and gh failure (to exercise resolve_base_branch/lib.sh logic); reference preflight-check.sh and the require_git_repo/resolve_base_branch behavior when making the change.plugins/me/skills/create-pr/scripts/verify-pr-status.sh (1)
45-52: Minor inconsistency: blank line before error message goes to stdout.Line 46 outputs a blank
echo ""to stdout while the subsequent error messages go to stderr. This could cause interleaved output if stdout and stderr are directed to different destinations.🔧 Suggested fix
if [[ $FAILED_REQUIRED -gt 0 ]]; then - echo "" + echo "" >&2 echo "✗ Required CI checks failed" >&2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/me/skills/create-pr/scripts/verify-pr-status.sh` around lines 45 - 52, In verify-pr-status.sh the blank echo before the error block writes to stdout while the rest of the error messages use stderr; change that blank echo to redirect to stderr (use echo "" >&2) or remove it so all error-related output for the FAILED_REQUIRED branch consistently goes to stderr; update the echo invocation near the FAILED_REQUIRED check that precedes the "✗ Required CI checks failed" and subsequent jq output.plugins/me/skills/create-pr/SKILL.md (1)
51-61: Good addition of Recovery section.The documentation clearly explains the recovery workflow for BEHIND/conflict scenarios.
Per static analysis hints, add blank lines around the fenced code blocks for better markdown compatibility.
📝 Markdown formatting fix
## Recovery If preflight-check reports BEHIND or conflicts, sync first: + ```bash "${CLAUDE_PLUGIN_ROOT}/skills/create-pr/scripts/sync-with-base.sh"To check PR status without modifying anything:
+"${CLAUDE_PLUGIN_ROOT}/skills/create-pr/scripts/verify-pr-status.sh"</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@plugins/me/skills/create-pr/SKILL.mdaround lines 51 - 61, In SKILL.md, add
a blank line before and after each fenced code block to satisfy markdown
parsers: specifically surround thebash blocks that contain "${CLAUDE_PLUGIN_ROOT}/skills/create-pr/scripts/sync-with-base.sh" and "${CLAUDE_PLUGIN_ROOT}/skills/create-pr/scripts/verify-pr-status.sh" with an empty line above the openingbash and an empty line below the closing ``` so
both code blocks have blank lines separating them from the surrounding
paragraphs.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In@plugins/me/skills/create-pr/scripts/verify-pr-status.sh:
- Around line 45-52: In verify-pr-status.sh the blank echo before the error
block writes to stdout while the rest of the error messages use stderr; change
that blank echo to redirect to stderr (use echo "" >&2) or remove it so all
error-related output for the FAILED_REQUIRED branch consistently goes to stderr;
update the echo invocation near the FAILED_REQUIRED check that precedes the "✗
Required CI checks failed" and subsequent jq output.In
@plugins/me/skills/create-pr/SKILL.md:
- Around line 51-61: In SKILL.md, add a blank line before and after each fenced
code block to satisfy markdown parsers: specifically surround thebash blocks that contain "${CLAUDE_PLUGIN_ROOT}/skills/create-pr/scripts/sync-with-base.sh" and "${CLAUDE_PLUGIN_ROOT}/skills/create-pr/scripts/verify-pr-status.sh" with an empty line above the openingbash and an empty line below the closing ``` so
both code blocks have blank lines separating them from the surrounding
paragraphs.In
@tests/skills/test_create_pr_verify_status.bats:
- Around line 26-33: The test "lib.sh: require_git_repo exits 2 outside git
repo" currently removes TEMP_DIR at the end of the test which will not run if
assertions fail; replace the inline rm -rf cleanup with a BATS teardown (or a
shell trap) that removes the TEMP_DIR after each test: create a teardown
function (or add a trap) that checks and deletes the TEMP_DIR variable set in
the test before exiting, and ensure the test still sets TEMP_DIR and calls
require_git_repo so cleanup runs regardless of assertion outcomes.- Around line 95-102: The test "sync-with-base.sh: exits 2 when not in a git
repository" creates a TEMP_DIR but doesn’t guarantee cleanup on failure; add the
same trap pattern used in lib.sh tests: after creating TEMP_DIR with
TEMP_DIR=$(mktemp -d) register a trap like trap 'rm -rf "$TEMP_DIR"' EXIT (or
using a cleanup function) so the directory is removed on test exit, then cd into
"$TEMP_DIR" and run the test using SYNC_SCRIPT; ensure you remove the explicit
rm -rf "$TEMP_DIR" at the end since the trap covers cleanup.- Around line 143-151: The test "preflight-check.sh: exits 2 when no base branch
given and gh fails" is asserting a generic "ERROR" but actually triggers
require_git_repo in preflight-check.sh which prints "Not in a git repository";
either update the test name and assertion to expect the repo-related behavior
(change the test title and assert output contains "Not in a git repository" and
status 2), or modify the test to run inside a git repo (git init in TEMP_DIR) so
preflight-check.sh proceeds to base-branch resolution and gh failure (to
exercise resolve_base_branch/lib.sh logic); reference preflight-check.sh and the
require_git_repo/resolve_base_branch behavior when making the change.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `dfa691c5-65a1-4800-8511-e7e1b9e10f68` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 2cf2983baba2ffc961397d95c7383684a05e9a5d and 18f60c1b6a05b6e91c8e735b4350c4897af32639. </details> <details> <summary>📒 Files selected for processing (7)</summary> * `plugins/me/skills/create-pr/SKILL.md` * `plugins/me/skills/create-pr/scripts/lib.sh` * `plugins/me/skills/create-pr/scripts/preflight-check.sh` * `plugins/me/skills/create-pr/scripts/sync-with-base.sh` * `plugins/me/skills/create-pr/scripts/verify-pr-status.sh` * `plugins/me/skills/create-pr/scripts/wait-for-merge.sh` * `tests/skills/test_create_pr_verify_status.bats` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
## [16.0.1](v16.0.0...v16.0.1) (2026-03-26) ### Code Refactoring * **create-pr:** improve quality, DRY code, strengthen tests ([#580](#580)) ([a018fb8](a018fb8)) ### BREAKING CHANGES * **create-pr:** removes iterative-eval skill * chore(autoresearch): setup create-pr quality experiment Baseline: 90/100 — shellcheck 0, tests 100%, code_quality 15/25, skillmd 25/25 * fix(create-pr): send all error/failure messages to stderr Result: {"status":"keep","quality_score":95,"code_quality":20} * chore(autoresearch): expand metrics with DRY, test depth, error recovery New baseline: 93/100 with advanced quality checks * refactor(create-pr): extract shared base branch detection to lib.sh DRY: resolve_base_branch() replaces 3x duplicated detection logic Result: {"status":"keep","quality_score":98,"advanced_quality":19} * feat(create-pr): add recovery section to SKILL.md, strengthen sync-with-base tests SKILL.md now documents sync-with-base and verify-pr-status recovery paths. Added 5 new BATS tests for sync-with-base.sh. Result: {"status":"keep","quality_score":100,"total_tests":28} * refactor(create-pr): extract require_git_repo to lib.sh, add lib.sh tests DRY: git-repo check now shared via require_git_repo(). Added 5 lib.sh tests (33 total). Result: {"status":"keep","quality_score":100,"total_tests":33} * docs(autoresearch): update dashboard, worklog, and what's-been-tried * chore(create-pr): remove autoresearch scaffolding * fix(tests): accept require_git_repo as valid git repo validation Test now recognizes both direct git rev-parse and shared require_git_repo.
1 participant
Summary
Improve create-pr skill quality: fix stderr routing, extract shared utilities to lib.sh, add recovery documentation, and strengthen test coverage.
Changes
scripts/lib.shwith sharedresolve_base_branch()andrequire_git_repo()functions, replacing 3x duplicated base branch detection and 2x duplicated git repo checksTests
bats tests/skills/test_create_pr_verify_status.bats)bats tests/)Breaking
None
Summary by CodeRabbit
Documentation
Tests
Chores