From d1df9d1600af866f34700723407592089948667a Mon Sep 17 00:00:00 2001 From: John Forstmeier Date: Tue, 10 Feb 2026 22:29:10 -0500 Subject: [PATCH 1/5] Add refinements to the Claude "update pull request" command --- .claude/commands/update-pull-request.md | 170 +++++++++++++++++++----- 1 file changed, 138 insertions(+), 32 deletions(-) diff --git a/.claude/commands/update-pull-request.md b/.claude/commands/update-pull-request.md index 684a9246..7a335d85 100644 --- a/.claude/commands/update-pull-request.md +++ b/.claude/commands/update-pull-request.md @@ -22,10 +22,32 @@ Follow these steps: ### 1. Fetch Pull Request Data - Accept the pull request ID from ${ARGUMENTS}; error if no argument is provided with a clear message that a pull request number is required. -- Determine the scratchpad directory path from the system reminder message (shown at session start, format: `/private/tmp/claude-*/scratchpad`). Use this for all temporary file storage instead of `/tmp/` to ensure session isolation and automatic cleanup. -- Determine repository owner and name from git remote: extract from `git remote get-url origin` (format: `https://github.com/owner/repo.git` or `git@github.com:owner/repo.git`) and export variables: - - `OWNER=` - - `REPO=` +- **CRITICAL: Set up SCRATCHPAD environment variable FIRST** before any file operations: + + ```bash + # Determine scratchpad from system or use default + SCRATCHPAD="${SCRATCHPAD:-/tmp/claude-scratchpad}" + mkdir -p "${SCRATCHPAD}" + export SCRATCHPAD + + # Verify scratchpad is writable + if [ ! -w "${SCRATCHPAD}" ]; then + echo "Error: Scratchpad directory ${SCRATCHPAD} is not writable" + exit 1 + fi + + echo "Using scratchpad: ${SCRATCHPAD}" + ``` + +- Determine repository owner and name from git remote and export as environment variables: + + ```bash + export OWNER=$(git remote get-url origin | sed -E 's|.*[:/]([^/]+)/([^/]+)\.git|\1|') + export REPO=$(git remote get-url origin | sed -E 's|.*[:/]([^/]+)/([^/]+)\.git|\2|') + + echo "Repository: ${OWNER}/${REPO}" + ``` + - Fetch comprehensive pull request data using a single GraphQL query, saving to a file to avoid token limit issues: ```bash @@ -138,37 +160,113 @@ Follow these steps: - This single query replaces multiple REST API calls and includes thread IDs needed for later resolution. - **Important**: Save output to a file (`${SCRATCHPAD}/pr_data.json`) to avoid token limit errors when reading large responses. Parse this file using `jq` for subsequent processing. -- **Critical**: The pull request data file will be too large to read directly with the Read tool. Always use `jq` to parse and extract specific fields. Never attempt to read the entire file. +- **Critical**: The pull request data file will be too large to read directly with the Read tool. Extract structured data once into smaller files (see below). + +- **Extract structured data into focused files** (replaces all scattered jq calls throughout command): + + ```bash + echo "Extracting structured data from PR response..." + + # Extract PR metadata + jq '.data.repository.pullRequest | { + id: .id, + title: .title, + headRefName: .headRefName, + baseRefName: .baseRefName + }' "${SCRATCHPAD}/pr_data.json" > "${SCRATCHPAD}/metadata.json" + + # Extract unresolved review threads + jq '[.data.repository.pullRequest.reviewThreads.nodes[] | + select(.isResolved == false and .isOutdated == false) | { + threadId: .id, + comments: [.comments.nodes[] | { + id: .id, + databaseId: .databaseId, + body: .body, + author: .author.login, + path: .path, + position: .position + }] + }]' "${SCRATCHPAD}/pr_data.json" > "${SCRATCHPAD}/review_threads.json" + + # Extract outdated threads + jq '[.data.repository.pullRequest.reviewThreads.nodes[] | + select(.isResolved == false and .isOutdated == true) | { + threadId: .id, + comments: [.comments.nodes[] | { + id: .id, + body: .body, + author: .author.login, + path: .path + }] + }]' "${SCRATCHPAD}/pr_data.json" > "${SCRATCHPAD}/outdated_threads.json" + + # Extract PR-level comments + jq '[.data.repository.pullRequest.comments.nodes[] | { + id: .id, + databaseId: .databaseId, + body: .body, + author: .author.login + }]' "${SCRATCHPAD}/pr_data.json" > "${SCRATCHPAD}/pr_comments.json" + + # Extract check failures + jq '[.data.repository.pullRequest.commits.nodes[].commit.checkSuites.nodes[] | + .checkRuns.nodes[] | + select(.conclusion == "FAILURE" or .conclusion == "TIMED_OUT") | { + name: .name, + conclusion: .conclusion, + detailsUrl: .detailsUrl, + workflowRunId: (.checkSuite.workflowRun.databaseId // null) + }] | unique_by(.name)' "${SCRATCHPAD}/pr_data.json" > "${SCRATCHPAD}/check_failures.json" + + echo "Data extraction complete:" + echo " - metadata.json (PR info)" + echo " - review_threads.json ($(jq 'length' ${SCRATCHPAD}/review_threads.json) unresolved threads)" + echo " - outdated_threads.json ($(jq 'length' ${SCRATCHPAD}/outdated_threads.json) outdated threads)" + echo " - pr_comments.json ($(jq 'length' ${SCRATCHPAD}/pr_comments.json) PR comments)" + echo " - check_failures.json ($(jq 'length' ${SCRATCHPAD}/check_failures.json) failed checks)" + ``` + +- These smaller structured files can be read with Read tool if needed, and eliminate redundant jq parsing throughout the command. ### 2. Analyze Check Failures -- Identify failing checks (Python or Rust checks specifically). Note that check-runs and workflow runs are distinct; to fetch logs, first obtain the workflow run ID from the check-run's check_suite, then use `gh api repos/${OWNER}/${REPO}/actions/runs/{run_id}/logs` (replacing `{run_id}` with the actual run ID). +- Review failing checks from the extracted data: + + ```bash + echo "=== Check Failures ===" + jq -r '.[] | "[\(.conclusion)] \(.name) - \(.detailsUrl)"' "${SCRATCHPAD}/check_failures.json" + ``` + +- Note that check-runs and workflow runs are distinct; to fetch logs, first obtain the workflow run ID, then use `gh api repos/${OWNER}/${REPO}/actions/runs/{run_id}/logs`. - If logs are inaccessible via API, run `mask development python all` or `mask development rust all` locally to replicate the errors and capture the failure details. - Add check failures to the list of items that need fixes. ### 3. Group and Analyze Feedback -- Parse the saved pull request data from `${SCRATCHPAD}/pr_data.json` using these extraction rules: - - From `data.repository.pullRequest.reviewThreads.nodes[]`: - - **First, identify outdated threads**: Filter for `isResolved: false` AND `isOutdated: true` - - Review these manually (see step 3a below) as they may still contain relevant feedback - - **Then, extract unresolved threads**: Filter for `isResolved: false` AND `isOutdated: false` - - For each unresolved thread, extract: - - Thread ID: `.id` (format: `PRRT_*`) for later resolution - - For each comment in `.comments.nodes[]`: - - Comment database ID: `.databaseId` (integer) - - Comment node ID: `.id` (format: `PRRC_*`) for GraphQL replies - - Comment body: `.body` - - Author: `.author.login` - - File location: `.path` and `.position` - - From `data.repository.pullRequest.comments.nodes[]`: - - Extract issue-level comments (pull request conversation): - - Comment database ID: `.databaseId` - - Comment body: `.body` - - Author: `.author.login` - - From check runs in `commits.nodes[].commit.checkSuites.nodes[].checkRuns.nodes[]`: - - Filter where `conclusion: "FAILURE"` or `conclusion: "TIMED_OUT"` -- Store complete metadata for each feedback item to use in later steps. +- Review the extracted feedback data from structured files: + + ```bash + echo "=== Unresolved Review Threads ===" + jq -r '.[] | "\(.threadId) | \(.comments[0].path // "N/A") | \(.comments[0].author)"' "${SCRATCHPAD}/review_threads.json" + + echo "" + echo "=== PR-level Comments ===" + jq -r '.[] | "\(.databaseId) | \(.author) | \(.body[:80])"' "${SCRATCHPAD}/pr_comments.json" + ``` + +- For detailed review of specific threads, use: + + ```bash + # Get full details of a specific thread + jq '.[] | select(.threadId == "PRRT_xxx")' "${SCRATCHPAD}/review_threads.json" + ``` + +- The structured files contain all necessary metadata: + - `review_threads.json`: Thread ID, comment IDs (both node and database), body, author, file path/position + - `pr_comments.json`: Comment IDs, body, author + - `check_failures.json`: Check name, conclusion, details URL + - Group related feedback using judgement: by file, by theme, by type of change, or whatever makes most sense for the specific pull request; ensure each group maintains the full metadata for all comments it contains. - Analyze dependencies between feedback groups to determine which are independent (can be worked in parallel) and which are interdependent (must be handled sequentially). - For each piece of feedback, evaluate whether to address it (make code changes) or reject it (explain why the feedback doesn't apply); provide clear reasoning for each decision. @@ -178,9 +276,8 @@ Follow these steps: - Before processing feedback, identify outdated threads that are still unresolved: ```bash - # Log outdated threads for review echo "=== Outdated threads (require manual review) ===" - jq -r '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false and .isOutdated == true) | "\(.id) | \(.comments.nodes[0].path) | \(.comments.nodes[0].author.login) | \(.comments.nodes[0].body[:80])"' ${SCRATCHPAD}/pr_data.json + jq -r '.[] | "\(.threadId) | \(.comments[0].path // "N/A") | \(.comments[0].author) | \(.comments[0].body[:80])"' "${SCRATCHPAD}/outdated_threads.json" ``` - **Important**: "Outdated" means the code was modified, not that feedback is irrelevant. Review each outdated thread manually during Step 3 to determine if the feedback still applies or should be addressed. @@ -234,6 +331,9 @@ Follow these steps: - For review comments (code-level), use GraphQL `addPullRequestReviewComment` mutation: ```bash + # Extract PR node ID from metadata + PR_ID=$(jq -r '.id' "${SCRATCHPAD}/metadata.json") + # IMPORTANT: Keep response text simple - avoid newlines, code blocks, and special characters # GraphQL string literals cannot contain raw newlines; use spaces or simple sentences # If complex formatting is needed, save response to a variable first and ensure proper escaping @@ -241,7 +341,7 @@ Follow these steps: gh api graphql -f query=' mutation { addPullRequestReviewComment(input: { - pullRequestId: "", + pullRequestId: "'"${PR_ID}"'", body: "", inReplyTo: "" }) { @@ -251,8 +351,14 @@ Follow these steps: ' ``` - Use the pull request's node ID from step 1's query (`data.repository.pullRequest.id`) for `pullRequestId`. - Use the comment's node ID (format: `PRRC_*`) for `inReplyTo` parameter. + Use the PR node ID from `metadata.json` for `pullRequestId`. + Use the comment node ID (format: `PRRC_*`) from `review_threads.json` for `inReplyTo` parameter. + + Example to get comment node ID: + ```bash + # Get first comment's node ID from a specific thread + COMMENT_ID=$(jq -r '.[] | select(.threadId == "PRRT_xxx") | .comments[0].id' "${SCRATCHPAD}/review_threads.json") + ``` **Response formatting guidelines**: - Keep responses concise and single-line when possible From a75bbbfe7694aba2a8543eb941fe7d168ee7c81e Mon Sep 17 00:00:00 2001 From: John Forstmeier Date: Tue, 10 Feb 2026 22:42:19 -0500 Subject: [PATCH 2/5] Address pull request #755 feedback: fix security, correctness, and robustness 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 --- .claude/commands/update-pull-request.md | 51 +++++++++++++++---------- 1 file changed, 31 insertions(+), 20 deletions(-) diff --git a/.claude/commands/update-pull-request.md b/.claude/commands/update-pull-request.md index 7a335d85..6e6738ee 100644 --- a/.claude/commands/update-pull-request.md +++ b/.claude/commands/update-pull-request.md @@ -25,16 +25,21 @@ Follow these steps: - **CRITICAL: Set up SCRATCHPAD environment variable FIRST** before any file operations: ```bash - # Determine scratchpad from system or use default - SCRATCHPAD="${SCRATCHPAD:-/tmp/claude-scratchpad}" - mkdir -p "${SCRATCHPAD}" - export SCRATCHPAD - - # Verify scratchpad is writable - if [ ! -w "${SCRATCHPAD}" ]; then - echo "Error: Scratchpad directory ${SCRATCHPAD} is not writable" - exit 1 + # Set up scratchpad directory with session isolation + : "${TMPDIR:=/tmp}" + if [ -z "${SCRATCHPAD:-}" ]; then + # Create a unique, private scratchpad directory + umask 077 + SCRATCHPAD="$(mktemp -d "${TMPDIR%/}/claude-scratchpad.XXXXXX")" || { + echo "Error: Failed to create scratchpad directory under ${TMPDIR}" + exit 1 + } + else + # Use caller-provided SCRATCHPAD, hardening it + mkdir -p "${SCRATCHPAD}" + chmod 700 "${SCRATCHPAD}" 2>/dev/null || true fi + export SCRATCHPAD echo "Using scratchpad: ${SCRATCHPAD}" ``` @@ -42,8 +47,13 @@ Follow these steps: - Determine repository owner and name from git remote and export as environment variables: ```bash - export OWNER=$(git remote get-url origin | sed -E 's|.*[:/]([^/]+)/([^/]+)\.git|\1|') - export REPO=$(git remote get-url origin | sed -E 's|.*[:/]([^/]+)/([^/]+)\.git|\2|') + export OWNER=$(git remote get-url origin | sed -E 's|.*[:/]([^/]+)/([^/]+)(\.git)?$|\1|') + export REPO=$(git remote get-url origin | sed -E 's|.*[:/]([^/]+)/([^/]+)(\.git)?$|\2|') + + if [ -z "${OWNER}" ] || [ -z "${REPO}" ]; then + echo "Error: Failed to determine repository owner and name from git remote \"origin\"" + exit 1 + fi echo "Repository: ${OWNER}/${REPO}" ``` @@ -210,21 +220,21 @@ Follow these steps: }]' "${SCRATCHPAD}/pr_data.json" > "${SCRATCHPAD}/pr_comments.json" # Extract check failures - jq '[.data.repository.pullRequest.commits.nodes[].commit.checkSuites.nodes[] | - .checkRuns.nodes[] | + jq '[.data.repository.pullRequest.commits.nodes[].commit.checkSuites.nodes[] as $suite | + $suite.checkRuns.nodes[] | select(.conclusion == "FAILURE" or .conclusion == "TIMED_OUT") | { name: .name, conclusion: .conclusion, detailsUrl: .detailsUrl, - workflowRunId: (.checkSuite.workflowRun.databaseId // null) + workflowRunId: ($suite.workflowRun.databaseId // null) }] | unique_by(.name)' "${SCRATCHPAD}/pr_data.json" > "${SCRATCHPAD}/check_failures.json" echo "Data extraction complete:" echo " - metadata.json (PR info)" - echo " - review_threads.json ($(jq 'length' ${SCRATCHPAD}/review_threads.json) unresolved threads)" - echo " - outdated_threads.json ($(jq 'length' ${SCRATCHPAD}/outdated_threads.json) outdated threads)" - echo " - pr_comments.json ($(jq 'length' ${SCRATCHPAD}/pr_comments.json) PR comments)" - echo " - check_failures.json ($(jq 'length' ${SCRATCHPAD}/check_failures.json) failed checks)" + echo " - review_threads.json ($(jq 'length' "${SCRATCHPAD}/review_threads.json") unresolved threads)" + echo " - outdated_threads.json ($(jq 'length' "${SCRATCHPAD}/outdated_threads.json") outdated threads)" + echo " - pr_comments.json ($(jq 'length' "${SCRATCHPAD}/pr_comments.json") PR comments)" + echo " - check_failures.json ($(jq 'length' "${SCRATCHPAD}/check_failures.json") failed checks)" ``` - These smaller structured files can be read with Read tool if needed, and eliminate redundant jq parsing throughout the command. @@ -248,7 +258,7 @@ Follow these steps: ```bash echo "=== Unresolved Review Threads ===" - jq -r '.[] | "\(.threadId) | \(.comments[0].path // "N/A") | \(.comments[0].author)"' "${SCRATCHPAD}/review_threads.json" + jq -r '.[] | "\(.threadId) | \(if .comments[0].path then .comments[0].path else "N/A" end) | \(.comments[0].author)"' "${SCRATCHPAD}/review_threads.json" echo "" echo "=== PR-level Comments ===" @@ -277,7 +287,7 @@ Follow these steps: ```bash echo "=== Outdated threads (require manual review) ===" - jq -r '.[] | "\(.threadId) | \(.comments[0].path // "N/A") | \(.comments[0].author) | \(.comments[0].body[:80])"' "${SCRATCHPAD}/outdated_threads.json" + jq -r '.[] | "\(.threadId) | \(if .comments[0].path then .comments[0].path else "N/A" end) | \(.comments[0].author) | \(.comments[0].body[:80])"' "${SCRATCHPAD}/outdated_threads.json" ``` - **Important**: "Outdated" means the code was modified, not that feedback is irrelevant. Review each outdated thread manually during Step 3 to determine if the feedback still applies or should be addressed. @@ -355,6 +365,7 @@ Follow these steps: Use the comment node ID (format: `PRRC_*`) from `review_threads.json` for `inReplyTo` parameter. Example to get comment node ID: + ```bash # Get first comment's node ID from a specific thread COMMENT_ID=$(jq -r '.[] | select(.threadId == "PRRT_xxx") | .comments[0].id' "${SCRATCHPAD}/review_threads.json") From 0ff2e999f3527bb8961771550766f20991f5ff20 Mon Sep 17 00:00:00 2001 From: John Forstmeier Date: Tue, 10 Feb 2026 22:46:42 -0500 Subject: [PATCH 3/5] Add fix for SCRATCHPAD references --- .claude/commands/update-pull-request.md | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/.claude/commands/update-pull-request.md b/.claude/commands/update-pull-request.md index 6e6738ee..fd46abf3 100644 --- a/.claude/commands/update-pull-request.md +++ b/.claude/commands/update-pull-request.md @@ -58,10 +58,26 @@ Follow these steps: echo "Repository: ${OWNER}/${REPO}" ``` -- Fetch comprehensive pull request data using a single GraphQL query, saving to a file to avoid token limit issues: +- **Verify SCRATCHPAD is set before file operations**: ```bash + # Critical: Validate SCRATCHPAD is set and accessible before proceeding + if [ -z "${SCRATCHPAD}" ]; then + echo "Error: SCRATCHPAD variable is not set. This should have been set in the previous step." + exit 1 + fi + + if [ ! -d "${SCRATCHPAD}" ] || [ ! -w "${SCRATCHPAD}" ]; then + echo "Error: SCRATCHPAD directory ${SCRATCHPAD} does not exist or is not writable" + exit 1 + fi + echo "SCRATCHPAD validated: ${SCRATCHPAD}" + ``` + +- Fetch comprehensive pull request data using a single GraphQL query, saving to a file to avoid token limit issues: + + ```bash gh api graphql -f query=' query($owner: String!, $repo: String!, $number: Int!) { repository(owner: $owner, name: $repo) { From c100702f55c11a58be221fd87fefcef9c66ac259 Mon Sep 17 00:00:00 2001 From: John Forstmeier Date: Tue, 10 Feb 2026 22:59:07 -0500 Subject: [PATCH 4/5] Address pull request #755 feedback: fix security, correctness, and robustness 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 --- .claude/commands/update-pull-request.md | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/.claude/commands/update-pull-request.md b/.claude/commands/update-pull-request.md index fd46abf3..261feb9b 100644 --- a/.claude/commands/update-pull-request.md +++ b/.claude/commands/update-pull-request.md @@ -29,15 +29,19 @@ Follow these steps: : "${TMPDIR:=/tmp}" if [ -z "${SCRATCHPAD:-}" ]; then # Create a unique, private scratchpad directory + _old_umask=$(umask) umask 077 SCRATCHPAD="$(mktemp -d "${TMPDIR%/}/claude-scratchpad.XXXXXX")" || { echo "Error: Failed to create scratchpad directory under ${TMPDIR}" exit 1 } + umask "${_old_umask}" else # Use caller-provided SCRATCHPAD, hardening it + _old_umask=$(umask) mkdir -p "${SCRATCHPAD}" chmod 700 "${SCRATCHPAD}" 2>/dev/null || true + umask "${_old_umask}" fi export SCRATCHPAD @@ -47,8 +51,9 @@ Follow these steps: - Determine repository owner and name from git remote and export as environment variables: ```bash - export OWNER=$(git remote get-url origin | sed -E 's|.*[:/]([^/]+)/([^/]+)(\.git)?$|\1|') - export REPO=$(git remote get-url origin | sed -E 's|.*[:/]([^/]+)/([^/]+)(\.git)?$|\2|') + _remote_url=$(git remote get-url origin) + export OWNER=$(echo "${_remote_url}" | sed -E 's|.*[:/]([^/]+)/([^/]+)(\.git)?$|\1|') + export REPO=$(echo "${_remote_url}" | sed -E 's|.*[:/]([^/]+)/([^/]+)(\.git)?$|\2|') if [ -z "${OWNER}" ] || [ -z "${REPO}" ]; then echo "Error: Failed to determine repository owner and name from git remote \"origin\"" @@ -274,7 +279,7 @@ Follow these steps: ```bash echo "=== Unresolved Review Threads ===" - jq -r '.[] | "\(.threadId) | \(if .comments[0].path then .comments[0].path else "N/A" end) | \(.comments[0].author)"' "${SCRATCHPAD}/review_threads.json" + jq -r '.[] | "\(.threadId) | \(.comments[0].path // "N/A") | \(.comments[0].author)"' "${SCRATCHPAD}/review_threads.json" echo "" echo "=== PR-level Comments ===" @@ -303,7 +308,7 @@ Follow these steps: ```bash echo "=== Outdated threads (require manual review) ===" - jq -r '.[] | "\(.threadId) | \(if .comments[0].path then .comments[0].path else "N/A" end) | \(.comments[0].author) | \(.comments[0].body[:80])"' "${SCRATCHPAD}/outdated_threads.json" + jq -r '.[] | "\(.threadId) | \(.comments[0].path // "N/A") | \(.comments[0].author) | \(.comments[0].body[:80])"' "${SCRATCHPAD}/outdated_threads.json" ``` - **Important**: "Outdated" means the code was modified, not that feedback is irrelevant. Review each outdated thread manually during Step 3 to determine if the feedback still applies or should be addressed. @@ -367,7 +372,7 @@ Follow these steps: gh api graphql -f query=' mutation { addPullRequestReviewComment(input: { - pullRequestId: "'"${PR_ID}"'", + pullRequestId: "'$PR_ID'", body: "", inReplyTo: "" }) { From fe6d0917fe1182980923d3cc7ea82db2cc06fe75 Mon Sep 17 00:00:00 2001 From: John Forstmeier Date: Tue, 10 Feb 2026 23:05:06 -0500 Subject: [PATCH 5/5] Fix recurring execution issues in update-pull-request command 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 --- .claude/commands/update-pull-request.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/.claude/commands/update-pull-request.md b/.claude/commands/update-pull-request.md index 261feb9b..2eb7bb8c 100644 --- a/.claude/commands/update-pull-request.md +++ b/.claude/commands/update-pull-request.md @@ -22,6 +22,7 @@ Follow these steps: ### 1. Fetch Pull Request Data - Accept the pull request ID from ${ARGUMENTS}; error if no argument is provided with a clear message that a pull request number is required. +- **IMPORTANT: Environment variable persistence** - When using the Bash tool, environment variables do not persist between separate tool invocations. The SCRATCHPAD, OWNER, and REPO setup commands below should be combined into a single bash execution along with the PR data fetch, or re-declared at the start of each subsequent bash command that needs them. - **CRITICAL: Set up SCRATCHPAD environment variable FIRST** before any file operations: ```bash @@ -53,7 +54,7 @@ Follow these steps: ```bash _remote_url=$(git remote get-url origin) export OWNER=$(echo "${_remote_url}" | sed -E 's|.*[:/]([^/]+)/([^/]+)(\.git)?$|\1|') - export REPO=$(echo "${_remote_url}" | sed -E 's|.*[:/]([^/]+)/([^/]+)(\.git)?$|\2|') + export REPO=$(echo "${_remote_url}" | sed -E 's|.*[:/]([^/]+)/([^/]+)(\.git)?$|\2|' | sed 's/\.git$//') if [ -z "${OWNER}" ] || [ -z "${REPO}" ]; then echo "Error: Failed to determine repository owner and name from git remote \"origin\"" @@ -67,8 +68,10 @@ Follow these steps: ```bash # Critical: Validate SCRATCHPAD is set and accessible before proceeding + # Note: If running these commands in separate bash invocations, SCRATCHPAD must be + # re-declared or the setup block must be combined with this validation block. if [ -z "${SCRATCHPAD}" ]; then - echo "Error: SCRATCHPAD variable is not set. This should have been set in the previous step." + echo "Error: SCRATCHPAD variable is not set. Ensure SCRATCHPAD setup and this validation are in the same bash execution context." exit 1 fi