Refactor Claude "update pull request" command#776
Merged
forstmeier merged 3 commits intomasterfrom Mar 2, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughReworks the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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)
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the .claude “update pull request” command to streamline the workflow for fetching PR data, analyzing feedback/check failures, planning fixes, and then committing/pushing before responding and resolving threads.
Changes:
- Consolidates PR data setup/fetch into a single bash block and standardizes JSON extraction into focused scratchpad files.
- Reorganizes workflow steps (analyze PR state → plan → implement → verify → commit+push → respond/resolve).
- Adds explicit handling for outdated review threads and updates response/resolution guidance.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Greptile SummaryThis PR refactors the Claude "update pull request" command with several improvements: Key improvements:
The refactor maintains all functionality while improving both code clarity and logical flow. No issues found. Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| .claude/commands/update-pull-request.md | Refactored PR update workflow with improved logic (commit/push before respond/resolve), consolidated steps, simplified environment setup, and clearer documentation - all changes improve clarity and correctness |
Last reviewed commit: b140a6f
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.claude/commands/update-pull-request.md (1)
28-130:⚠️ Potential issue | 🔴 CriticalCritical: Environment variables won't persist for later steps.
Step 1 correctly combines all initial setup into a single bash execution (following line 25's guidance), but the
SCRATCHPAD,OWNER, andREPOvariables set here won't be available in subsequent bash tool invocations in steps 2, 6, 8, etc.Multiple later sections reference these variables (lines 226-241, 246, 256, 362) and will fail when the variables are undefined.
🔧 Proposed solution: Persist variables to files
Add this at the end of the step 1 bash block (after line 129):
echo "PR data fetched to ${SCRATCHPAD}/pr_data.json" + + # Persist variables for later bash invocations + echo "${SCRATCHPAD}" > ~/.claude-update-pr-scratchpad + echo "${OWNER}" > "${SCRATCHPAD}/owner" + echo "${REPO}" > "${SCRATCHPAD}/repo"Then update later sections to source these values. For example, in step 2 (line 226), add:
```bash + SCRATCHPAD=$(cat ~/.claude-update-pr-scratchpad) + OWNER=$(cat "${SCRATCHPAD}/owner") + REPO=$(cat "${SCRATCHPAD}/repo") + echo "=== Check Failures ===" jq -r '.[] | "[\(.conclusion)] \(.name) - \(.detailsUrl)"' "${SCRATCHPAD}/check_failures.json"Apply similar sourcing at the start of each bash block that uses these variables (lines 246, 256, 302-318, 329-382).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/commands/update-pull-request.md around lines 28 - 130, The shell step sets SCRATCHPAD, OWNER and REPO but those environment variables do not persist across separate bash tool invocations; persist them to disk at the end of the initial step by writing the SCRATCHPAD path to a well-known file (e.g. ~/.claude-update-pr-scratchpad) and creating files inside ${SCRATCHPAD} for owner and repo (e.g. ${SCRATCHPAD}/owner and ${SCRATCHPAD}/repo), and then update all later bash blocks that reference SCRATCHPAD, OWNER or REPO to rehydrate them by reading those files (e.g. SCRATCHPAD=$(cat ~/.claude-update-pr-scratchpad); OWNER=$(cat "${SCRATCHPAD}/owner"); REPO=$(cat "${SCRATCHPAD}/repo")). Ensure this change is applied where the script reads check_failures.json, review/comment handling, and any other blocks that use SCRATCHPAD/OWNER/REPO so they no longer assume the upstream environment variables remain set.
🤖 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/update-pull-request.md:
- Line 384: The sentence "Map each comment back to its parent thread using the
data structure from step 2 parsing" is misleading because the structured JSON
files are created in step 1; update the wording to reference the JSON creation
step and the actual filenames (e.g., review_threads.json) instead of "step 2
parsing" so it reads like: map each comment back to its parent thread using the
structured JSON produced in step 1 (review_threads.json, etc.), and ensure any
mention of "step 2" reflects that step 2 analyzes those files rather than
creating them.
---
Outside diff comments:
In @.claude/commands/update-pull-request.md:
- Around line 28-130: The shell step sets SCRATCHPAD, OWNER and REPO but those
environment variables do not persist across separate bash tool invocations;
persist them to disk at the end of the initial step by writing the SCRATCHPAD
path to a well-known file (e.g. ~/.claude-update-pr-scratchpad) and creating
files inside ${SCRATCHPAD} for owner and repo (e.g. ${SCRATCHPAD}/owner and
${SCRATCHPAD}/repo), and then update all later bash blocks that reference
SCRATCHPAD, OWNER or REPO to rehydrate them by reading those files (e.g.
SCRATCHPAD=$(cat ~/.claude-update-pr-scratchpad); OWNER=$(cat
"${SCRATCHPAD}/owner"); REPO=$(cat "${SCRATCHPAD}/repo")). Ensure this change is
applied where the script reads check_failures.json, review/comment handling, and
any other blocks that use SCRATCHPAD/OWNER/REPO so they no longer assume the
upstream environment variables remain set.
…776 review Three documentation corrections based on automated reviewer feedback: 1. Line 251: Corrected outdated_threads.json field description. The jq extraction intentionally omits databaseId and position (not needed for resolution), so the previous "Same fields" claim was inaccurate. Now accurately states the fields that are included: thread ID and comment metadata (body, author, file path). 2. Line 384: Fixed wrong step number reference. Structured JSON files are created in step 1 (fetch PR data), not step 2 (analyze). Updated to reference step 1 and specifically name review_threads.json for clarity. 3. Lines 25-26: Improved env var persistence note. Previous text only said to combine setup+fetch into one block, but subsequent separate bash blocks also reference SCRATCHPAD, OWNER, and REPO without guidance. Now explicitly instructs re-declaring those variables at the top of each subsequent bash block. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/update-pull-request.md:
- Around line 31-37: The umask is saved to _old_umask, changed to 077 for
mktemp, then immediately restored with umask "${_old_umask}"; since the
directory permissions are determined at creation, remove the restoration line if
there are no later operations in this function/block that rely on the original
umask—i.e., delete the umask "${_old_umask}" statement and keep
SCRATCHPAD="$(mktemp -d ...)" with umask 077 (or leave the save/restore as-is if
you prefer defensive coding).
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
Overview
Changes
Context
I realize that this is a tool that basically I only use so it could probably be moved into my personal Claude settings but I can do that later.
Summary by CodeRabbit