chore: split /review-pr and /merge-pr into separate commands#245
chore: split /review-pr and /merge-pr into separate commands#245
Conversation
Update description to indicate /review-pr no longer merges — use /merge-pr separately. https://claude.ai/code/session_01CH5eCG1syCXdVHh3WvQdJs
/review-pr was auto-merging at the end of every review, which caused repeated failures when branch protection blocked the merge (e.g. outstanding requesting-changes reviews). Now /review-pr stops after reviewing and fixing issues. A new /merge-pr command handles merging with proper pre-flight checks (PR open, approved, checks passing) and fails fast with a clear message instead of retrying. https://claude.ai/code/session_01CH5eCG1syCXdVHh3WvQdJs
📝 Walkthrough🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/commands/merge-pr.md:
- Line 9: Update the merge pre-flight validation to explicitly inspect the
GraphQL statusCheckRollup object's fields rather than treating it as an enum:
check both checkRunCountsByState.FAILURE and statusContextCountsByState.FAILURE
are zero and also iterate statusCheckRollup.nodes to ensure no node.state ===
"FAILURE"; modify the merge validation function that currently reads
statusCheckRollup (e.g., the preflight/merge validation method) to use these
exact fields and fail the merge if any FAILURE count > 0 or any node has state
"FAILURE", while treating SUCCESS and NEUTRAL as acceptable.
- Line 5: Add a validation step after running `gh pr view --json
number,state,reviewDecision,mergeStateStatus,statusCheckRollup` to abort if
`mergeStateStatus` is not "MERGEABLE"; update the pre-flight checks logic that
currently validates PR state (`state`) and `reviewDecision` to also verify
`mergeStateStatus === "MERGEABLE"` and `statusCheckRollup` is `"SUCCESS"`, and
ensure the command reports a clear error and exits if any check fails (PR must
be OPEN, `reviewDecision` must be APPROVED, `mergeStateStatus` must be
MERGEABLE, and `statusCheckRollup` must be SUCCESS).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f55a0e98-7f74-4093-a879-3c6460e351e8
📒 Files selected for processing (2)
.claude/commands/merge-pr.md.claude/commands/review-pr.md
.claude/commands/merge-pr.md
Outdated
| 2. **Pre-flight checks** — abort and report if any of these fail: | ||
| - PR state must be `OPEN`. | ||
| - `reviewDecision` must be `APPROVED` (no outstanding requesting-changes reviews). | ||
| - All required status checks must pass (`statusCheckRollup` items are `SUCCESS` or `NEUTRAL`). |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the actual structure of statusCheckRollup returned by gh CLI
gh pr view --json statusCheckRollup | jq '.statusCheckRollup'Repository: bd73-com/fetchthechange
Length of output: 139
🌐 Web query:
GitHub GraphQL API statusCheckRollup field structure and possible values
💡 Result:
The statusCheckRollup field is available on the Commit object in GitHub's GraphQL API. It returns a StatusCheckRollup object that aggregates the status checks (both legacy StatusContext and modern CheckRun) for the commit. Structure (key fields from usage examples): - contexts: StatusCheckRollupContextConnection - Connection to aggregated contexts. - checkRunCount: Int - Total number of check runs. - checkRunCountsByState: [CheckRunCountsByState!]! - Counts of check runs grouped by state (e.g., {state: CheckRunState!, count: Int!}). - statusContextCount: Int - Total number of status contexts. - statusContextCountsByState: [StatusContextCountsByState!]! - Counts of status contexts grouped by state. - nodes: [StatusCheckRollupContext!] - Individual context details (StatusContext or CheckRun). - Accessed via commit.statusCheckRollup (often queried as commits(last: 1).nodes[0].commit.statusCheckRollup for PR head commits). Possible values for states (from related enums used in counts): CheckRunState: WAITING, TIMED_OUT, SUCCESS, STARTUP_FAILURE, STALE, SKIPPED, QUEUED, PENDING, NEUTRAL, IN_PROGRESS, FAILURE, COMPLETED, CANCELLED. StatusCheckState (likely similar for status contexts): PENDING, SUCCESS, FAILURE, ERROR. This field may require specific GitHub Enterprise versions or features for full subfields like checkRunCountsByState (available since GHE 3.8). Official reference is in GitHub's schema at GraphOS Studio or downloadable schema. Example query snippet: commit { statusCheckRollup { contexts { checkRunCount checkRunCountsByState { state count } statusContextCount statusContextCountsByState { state count } } } }
Citations:
- 1: https://studio.apollographql.com/public/github/variant/current/schema/reference/objects/StatusCheckRollup
- 2:
gh pr statusGraphQL query requests moreStatusCheckRollupdata than required, causing timeouts in large repositories cli/cli#7421 - 3: https://docs.github.com/en/graphql/reference/enums
- 4: https://docs.github.com/en/graphql/reference/objects
- 5: Use new GQL fields that support CheckRun and StatusContext counts by state cli/cli#7462
Clarify statusCheckRollup validation — ambiguous API reference.
GitHub's GraphQL API statusCheckRollup is a complex object, not a simple enum or array. It contains nested fields including checkRunCountsByState, statusContextCountsByState, and nodes — each potentially containing status items with SUCCESS, NEUTRAL, or FAILURE states.
The phrase "statusCheckRollup items are SUCCESS or NEUTRAL" is ambiguous:
- Which field within the object are you checking (e.g.,
nodes,checkRunCountsByState,statusContextCountsByState)? - Are all items required to pass, or just the aggregate state?
- How should failures in individual checks be handled?
This ambiguity could cause the pre-flight check to validate the wrong field or miss failing status checks. Specify exactly which statusCheckRollup field and aggregation logic the merge validation uses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/commands/merge-pr.md at line 9, Update the merge pre-flight
validation to explicitly inspect the GraphQL statusCheckRollup object's fields
rather than treating it as an enum: check both checkRunCountsByState.FAILURE and
statusContextCountsByState.FAILURE are zero and also iterate
statusCheckRollup.nodes to ensure no node.state === "FAILURE"; modify the merge
validation function that currently reads statusCheckRollup (e.g., the
preflight/merge validation method) to use these exact fields and fail the merge
if any FAILURE count > 0 or any node has state "FAILURE", while treating SUCCESS
and NEUTRAL as acceptable.
Address CodeRabbit review feedback: - Add missing mergeStateStatus === MERGEABLE pre-flight check - Clarify statusCheckRollup validation to specify checking each entry's conclusion (SUCCESS/NEUTRAL/SKIPPED) rather than ambiguous "items" https://claude.ai/code/session_01CH5eCG1syCXdVHh3WvQdJs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/commands/merge-pr.md:
- Line 1: The first line "Squash-merge the current branch's PR and delete the
remote branch." must be a top-level heading to satisfy MD041; update that line
in .claude/commands/merge-pr.md by prefixing it with "# " so the first line
becomes "# Squash-merge the current branch's PR and delete the remote branch."
ensuring the file starts with an H1 heading.
- Around line 6-11: Add a pre-merge gate that verifies the PR has one of the
required release labels (feature, fix, breaking, chore, docs, security) before
running the existing status checks and the final gh pr merge --squash
--delete-branch step; implement this by checking the PR's labels array for
membership in the set {feature, fix, breaking, chore, docs, security} (in the
same pre-flight section that inspects reviewDecision, mergeStateStatus, and
statusCheckRollup) and abort with an explicit error if none of those labels are
present.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ff9fe2a3-8c93-494d-bb43-adfdc78dbf01
📒 Files selected for processing (1)
.claude/commands/merge-pr.md
| @@ -0,0 +1,14 @@ | |||
| Squash-merge the current branch's PR and delete the remote branch. | |||
There was a problem hiding this comment.
Use an H1 on the first line to satisfy markdown linting.
Line 1 should be a top-level heading (# ...) to fix MD041.
Proposed fix
-Squash-merge the current branch's PR and delete the remote branch.
+# Squash-merge the current branch's PR and delete the remote branch📝 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.
| Squash-merge the current branch's PR and delete the remote branch. | |
| # Squash-merge the current branch's PR and delete the remote branch |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/commands/merge-pr.md at line 1, The first line "Squash-merge the
current branch's PR and delete the remote branch." must be a top-level heading
to satisfy MD041; update that line in .claude/commands/merge-pr.md by prefixing
it with "# " so the first line becomes "# Squash-merge the current branch's PR
and delete the remote branch." ensuring the file starts with an H1 heading.
Address CodeRabbit feedback: verify PR has exactly one release label (feature/fix/breaking/chore/docs/security) before merging, consistent with the project convention documented in CLAUDE.md. https://claude.ai/code/session_01CH5eCG1syCXdVHh3WvQdJs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.claude/commands/merge-pr.md (1)
11-11:⚠️ Potential issue | 🟠 MajorDisambiguate
statusCheckRollupevaluation criteria.“Every entry ... conclusion” is underspecified for mixed status contexts/check runs; document the exact field(s) and failure conditions used, or agents may validate the wrong structure.
#!/bin/bash # Inspect actual shape returned by gh for this repo/PR to define exact checks gh pr view --json statusCheckRollup | jq '.statusCheckRollup' # Use this output to specify exact pass/fail logic fields in the command doc.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/commands/merge-pr.md at line 11, Update the sentence about statusCheckRollup in .claude/commands/merge-pr.md to explicitly state which JSON fields from GitHub's API are being checked (use the `statusCheckRollup` array items returned by `gh pr view --json statusCheckRollup`), specify the exact field name used (e.g., `conclusion`) and the allowed values (`"SUCCESS"`, `"NEUTRAL"`, `"SKIPPED"`), and define failure conditions (any item missing the `conclusion` field or with values `"FAILURE"`, `"PENDING"`, or `null` should cause the check to fail); also mention how mixed contexts are handled (require all entries in `statusCheckRollup` to meet allowed values) and include a brief note recommending the provided `gh pr view --json statusCheckRollup` command for verifying the exact shape before relying on the rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/commands/merge-pr.md:
- Around line 5-12: Add base branch validation by including baseRefName in the
`gh pr view --json ...` query and aborting if the PR's baseRefName is not
"main"; update the pre-flight checks (where you currently validate `state`,
`reviewDecision`, `mergeStateStatus`, `statusCheckRollup`, and release label) to
also enforce `baseRefName === "main"` before calling `gh pr merge --squash
--delete-branch`, so merges to unintended branches are prevented.
---
Duplicate comments:
In @.claude/commands/merge-pr.md:
- Line 11: Update the sentence about statusCheckRollup in
.claude/commands/merge-pr.md to explicitly state which JSON fields from GitHub's
API are being checked (use the `statusCheckRollup` array items returned by `gh
pr view --json statusCheckRollup`), specify the exact field name used (e.g.,
`conclusion`) and the allowed values (`"SUCCESS"`, `"NEUTRAL"`, `"SKIPPED"`),
and define failure conditions (any item missing the `conclusion` field or with
values `"FAILURE"`, `"PENDING"`, or `null` should cause the check to fail); also
mention how mixed contexts are handled (require all entries in
`statusCheckRollup` to meet allowed values) and include a brief note
recommending the provided `gh pr view --json statusCheckRollup` command for
verifying the exact shape before relying on the rule.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 979e0914-a686-49c5-bdd1-a37141178fb9
📒 Files selected for processing (1)
.claude/commands/merge-pr.md
| 1. Run `gh pr view --json number,state,reviewDecision,mergeStateStatus,statusCheckRollup,labels` to check the PR's current status. | ||
| 2. **Pre-flight checks** — abort and report if any of these fail: | ||
| - PR state must be `OPEN`. | ||
| - `reviewDecision` must be `APPROVED` (no outstanding requesting-changes reviews). | ||
| - `mergeStateStatus` must be `MERGEABLE` (not `CONFLICTING`, `BEHIND`, or `BLOCKED`). | ||
| - PR must have exactly one release label from: `feature`, `fix`, `breaking`, `chore`, `docs`, `security`. | ||
| - All required status checks must pass: every entry in `statusCheckRollup` must have a conclusion of `SUCCESS`, `NEUTRAL`, or `SKIPPED` — fail if any entry is `FAILURE`, `PENDING`, or missing. | ||
| 3. If all checks pass, merge with: `gh pr merge --squash --delete-branch`. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the current PR includes baseRefName and targets main before merge
gh pr view --json number,baseRefName,state,reviewDecision,mergeStateStatus,labels \
| jq '{number, baseRefName, state, reviewDecision, labels: [.labels[].name]}'
# Expected: baseRefName == "main"Repository: bd73-com/fetchthechange
Length of output: 139
🏁 Script executed:
find . -name "merge-pr.md" -type fRepository: bd73-com/fetchthechange
Length of output: 97
🏁 Script executed:
cat -n ./.claude/commands/merge-pr.mdRepository: bd73-com/fetchthechange
Length of output: 1258
Add mandatory base-branch validation to prevent merging to wrong target.
The current merge pre-flight checks do not validate the PR's target branch. This is a security risk—a compliant PR could be merged into an unintended branch. Add baseRefName to the query and enforce the base branch is main before proceeding with merge.
Required change
-1. Run `gh pr view --json number,state,reviewDecision,mergeStateStatus,statusCheckRollup,labels` to check the PR's current status.
+1. Run `gh pr view --json number,baseRefName,state,reviewDecision,mergeStateStatus,statusCheckRollup,labels` to check the PR's current status.
2. **Pre-flight checks** — abort and report if any of these fail:
+ - PR base branch must be `main`.
- PR state must be `OPEN`.
- `reviewDecision` must be `APPROVED` (no outstanding requesting-changes reviews).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/commands/merge-pr.md around lines 5 - 12, Add base branch validation
by including baseRefName in the `gh pr view --json ...` query and aborting if
the PR's baseRefName is not "main"; update the pre-flight checks (where you
currently validate `state`, `reviewDecision`, `mergeStateStatus`,
`statusCheckRollup`, and release label) to also enforce `baseRefName === "main"`
before calling `gh pr merge --squash --delete-branch`, so merges to unintended
branches are prevented.
bd73-com
dismissed stale reviews from coderabbitai[bot], coderabbitai[bot], and coderabbitai[bot]
Addressing in follow-up
The branch claude/fix-issue-eWlh3 bumped the extension version to 1.0.1, but main already has manifest.json at 1.0.2. This syncs package.json to match. The other changes on that branch (regressions to auth validation, campaign email race condition fixes, and webhook lock timeouts) were all superseded by PRs #239, #241, and #245. https://claude.ai/code/session_019h1ZcdmqQm4mE8EQFRA2bg
2 participants
Summary
The
/review-prcommand was auto-merging PRs at the end of every review session. When branch protection blocked the merge (e.g. outstanding requesting-changes reviews), Claude would get stuck in a retry loop asking the user how to proceed — this happened on every new session. This PR separates reviewing and merging into two distinct commands so reviews complete cleanly without attempting to merge.Changes
.claude/commands/review-pr.md: Removed steps 8–10 (ask for confirmation, squash-merge, confirm merge). Updated the description to clarify it does not merge. The command now ends after outputting the review summary..claude/commands/merge-pr.md(new): Dedicated merge command with pre-flight checks — verifies the PR is open, has an approved review decision, and all status checks pass before attemptinggh pr merge --squash --delete-branch. Fails fast with a clear message if any check fails.How to test
/review-pron any open PR — verify it completes after the review summary without attempting to merge./merge-pron a PR that has outstanding requesting-changes reviews — verify it reports the blocker and does not attempt the merge./merge-pron a fully approved PR with passing checks — verify it merges successfully.https://claude.ai/code/session_01CH5eCG1syCXdVHh3WvQdJs
Summary by CodeRabbit
New Features
Documentation