Add refinements to the Claude "update pull request" command#755
Add refinements to the Claude "update pull request" command#755forstmeier merged 5 commits intomasterfrom
Conversation
📝 WalkthroughWalkthroughReplaces a single Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Script as "update-pull-request.sh"
participant Git as "git remote"
participant GitHub as "GitHub GraphQL API"
participant FS as "Scratchpad (FS)"
User->>Script: invoke command
Script->>FS: ensure/create SCRATCHPAD dir (private, writable)
Script->>Git: read remote URL to extract OWNER/REPO
Script->>GitHub: fetch PR data (GraphQL)
GitHub-->>Script: PR payload
Script->>FS: write metadata.json, review_threads.json, outdated_threads.json, pr_comments.json, check_failures.json
Script->>User: print counts and instructions (IDs for mutations)
User->>Script: run suggested GraphQL mutations using IDs from files
Script->>GitHub: send addPullRequestReviewComment / resolve mutations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
There was a problem hiding this comment.
Pull request overview
Refines the “update pull request” command to reduce redundant querying and to use intermediate, structured JSON files for subsequent steps.
Changes:
- Adds scratchpad setup/validation and exports
SCRATCHPADearly for consistent temp-file usage. - Replaces scattered
jqparsing with a single extraction pass into focused JSON artifacts (metadata, threads, comments, check failures). - Updates later steps to consume the extracted files rather than re-parsing the large GraphQL response.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.claude/commands/update-pull-request.md:
- Around line 357-361: Add a blank line before and after the fenced code block
that starts with "```bash" and contains the COMMENT_ID=$(jq -r '.[] |
select(.threadId == "PRRT_xxx") | .comments[0].id'
"${SCRATCHPAD}/review_threads.json") line so the Markdown complies with MD031;
specifically insert an empty line between the preceding "Example to get comment
node ID:" text and the opening ```bash, and another empty line after the closing
``` fence.
- Around line 212-220: The jq pipeline is iterating check runs directly so
.checkSuite is unavailable and workflowRunId ends up null; instead iterate check
suites first and capture the parent suite (e.g., .checkSuites.nodes[] as $suite)
then iterate its .checkRuns.nodes[] and build each entry using
$suite.workflowRun.databaseId (e.g., .checkSuites.nodes[] as $suite |
$suite.checkRuns.nodes[] | {name: .name, conclusion: .conclusion, detailsUrl:
.detailsUrl, workflowRunId: ($suite.workflowRun.databaseId // null)}), keeping
the existing select(...) filter and unique_by(.name) logic.
Greptile OverviewGreptile SummaryThis PR updates the Claude Main issues to address before merge:
Confidence Score: 2/5
|
| Filename | Overview |
|---|---|
| .claude/commands/update-pull-request.md | Refactors the update-pull-request command to pre-extract PR data into smaller JSON files and adds scratchpad setup, but introduces a non-isolated scratchpad default, a broken jq reference for workflowRunId extraction, and invalid jq string quoting in examples. |
…bustness issues
Implemented fixes for all 9 review comments on the update-pull-request command:
1. SCRATCHPAD Security & Isolation (HIGH PRIORITY)
- Replaced hardcoded /tmp/claude-scratchpad with mktemp -d for unique session directories
- Added umask 077 and chmod 700 for restrictive permissions
- Prevents cross-session collisions and data leakage
- Maintains fallback to caller-provided SCRATCHPAD
2. jq workflowRunId Extraction Bug (CRITICAL)
- Fixed invalid .checkSuite reference in checkRuns iteration
- Used 'as $suite' variable binding to capture parent context
- Now correctly extracts $suite.workflowRun.databaseId
- Prevents always-null workflowRunId values
3. Git Remote Parsing Edge Case
- Made .git suffix optional in sed pattern
- Handles both "owner/repo.git" and "owner/repo" formats
- Added validation to ensure OWNER/REPO are non-empty
- Prevents silent extraction failures
4. Shell Quoting Robustness
- Added quotes around all ${SCRATCHPAD} references in jq calls
- Protects against paths with spaces or special characters
- Defensive shell scripting best practices
5. jq Nested Quote Escaping Bug (CRITICAL)
- Fixed invalid nested double quotes in jq strings
- Replaced 'path // "N/A"' with conditional: if .path then .path else "N/A" end
- Valid jq syntax that handles null values correctly
- Affects review threads and outdated threads queries
6. Markdown Formatting
- Added blank line before code fence for MD031 compliance
- Markdown linting rule satisfaction
All changes maintain backward compatibility while improving security,
correctness, and robustness of the update-pull-request command.
Files modified:
- .claude/commands/update-pull-request.md (6 groups of fixes across 7 sections)
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Greptile OverviewGreptile SummaryThis PR updates the However, the updated examples introduce jq filters that will not parse due to nested quoting in interpolated strings, and the GraphQL Confidence Score: 2/5
|
| Filename | Overview |
|---|---|
| .claude/commands/update-pull-request.md | Refactors update-pull-request command docs to create a unique scratchpad dir and pre-extract PR data into smaller JSON files, but introduces broken jq quoting and a GitHub GraphQL mutation quoting issue that will fail at runtime. |
There was a problem hiding this comment.
Actionable comments posted: 2
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)
155-156:⚠️ Potential issue | 🟡 MinorQuote the SCRATCHPAD path in redirection to avoid breakage.
If
SCRATCHPADcontains spaces or glob characters, the redirection target will fail or point to the wrong path.🔧 Proposed fix
- ' -f owner="${OWNER}" -f repo="${REPO}" -F number=${ARGUMENTS} > ${SCRATCHPAD}/pr_data.json + ' -f owner="${OWNER}" -f repo="${REPO}" -F number="${ARGUMENTS}" > "${SCRATCHPAD}/pr_data.json"
🤖 Fix all issues with AI agents
In @.claude/commands/update-pull-request.md:
- Around line 49-52: The script calls git remote get-url origin twice to derive
OWNER and REPO; change it to capture the remote once (e.g., store it in a REMOTE
variable) and reuse that variable when extracting OWNER and REPO so parsing is
consistent and you avoid the duplicate external call; update the assignments
that set OWNER and REPO to use the single captured REMOTE value.
- Around line 32-45: Save the current umask before setting umask 077, then
restore it immediately after the scratchpad directory is created and hardening
(the branches that create SCRATCHPAD: the mktemp branch and the caller-provided
branch that runs mkdir -p and chmod 700). Concretely: capture the prior umask
into a variable (e.g., old_umask=$(umask)), set umask 077 for
creation/hardening, perform mktemp or mkdir -p and chmod 700 on SCRATCHPAD, and
then call umask "$old_umask" to restore the original mask so later commands are
unaffected.
…bustness issues
Fixed three categories of issues identified in code review:
1. Critical correctness fixes (security impact):
- Fixed broken jq string quoting in review thread and outdated thread
listing commands (lines 277, 306). The nested quotes in
'if .comments[0].path then .comments[0].path else "N/A" end'
would cause jq parse failures. Replaced with the cleaner '//'
operator: '.comments[0].path // "N/A"'.
- Fixed invalid GraphQL mutation quoting for pullRequestId (line 370).
The pattern '"'"${PR_ID}"'"' created overly complex nested quoting.
Simplified to "'$PR_ID'" for clean shell interpolation.
2. Shell robustness improvements:
- Added umask save/restore around scratchpad creation (lines 32-43)
to prevent global umask changes from affecting subsequent file
operations in the session.
- Optimized git remote lookup (lines 54-56) to call 'git remote
get-url origin' once instead of twice, improving efficiency and
ensuring consistent parsing.
All fixes address automated review feedback from greptile-apps and
coderabbitai on PR #755, ensuring the update-pull-request command
documentation contains correct, robust shell examples.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Thank you for the comprehensive reviews! All feedback has been addressed: fixed jq quoting issues, simplified GraphQL mutation quoting, added umask restore, and optimized git remote calls. Changes committed in c100702. |
Added fixes for two issues encountered during PR #755 execution: 1. Repository name extraction now properly removes .git suffix: - Added explicit sed filter to strip .git from REPO variable (line 56) - Previous regex made .git optional but didn't exclude it from capture - Ensures clean repo names like "fund" instead of "fund.git" 2. Added environment variable persistence guidance: - Documented that Bash tool invocations don't share variables (line 24) - Added note that SCRATCHPAD/OWNER/REPO must be combined or re-declared - Updated validation error messages for clarity (line 71) - Prevents confusion when variables appear unset between commands These fixes prevent common execution failures when following the command. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile OverviewGreptile SummaryThis PR significantly improves the Key improvements:
Remaining minor issue: Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| .claude/commands/update-pull-request.md | Improved PR command with structured data extraction, SCRATCHPAD validation, and better error handling. Previous critical issues (scratchpad isolation, jq quoting, GraphQL references) have been fixed. Minor risk: no validation that extracted JSON files were created successfully. |
There was a problem hiding this comment.
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)
375-385:⚠️ Potential issue | 🔴 CriticalVariable expansion bug in GraphQL mutation.
The
$PR_IDvariable on line 378 will not expand because it's inside single quotes (-f query='...'). In bash, single quotes preserve literal strings and prevent variable expansion. The GraphQL API will receive the literal text$PR_IDinstead of the actual pull request ID, causing the mutation to fail.Fix by switching to double quotes and escaping the inner quotes:
Proposed fix using double quotes
- gh api graphql -f query=' + gh api graphql -f query=" mutation { addPullRequestReviewComment(input: { - pullRequestId: "'$PR_ID'", + pullRequestId: \"$PR_ID\", body: "<response_text>", inReplyTo: "<comment_node_id>" }) { comment { id } } } - ' + "Alternatively, use the
-Fflag to pass variables safely:gh api graphql -f query=' mutation($prId: ID!) { addPullRequestReviewComment(input: { pullRequestId: $prId, body: "<response_text>", inReplyTo: "<comment_node_id>" }) { comment { id } } } ' -F prId="$PR_ID"
Overview
Changes
Context
I noticed a lot of sub-optimal performance from the command and this is a fix for that.
Summary by CodeRabbit