Skip to content

Commit 987a697

Browse files
justin808claude
andcommitted
Address review: trap, printf -v, r all, jq null guard, step order
- Fix trap overwrite in `f+i`: Step 10 now cleans up both `issue_body_file` and `summary_body_file` so the Step 9 temp file is not leaked (commands/address-review.md Step 10). - Replace bash-only `printf -v` with plain variable assignment in the follow-up issue template (commands/address-review.md Step 9). - Define bare `r` and bare `r all` handling in Action `r`: both are rejected with guidance instead of being silently ignored. - Null-guard the summary-cutoff `jq ... | last` snippet so an empty result yields `""` (not JSON `null`) and document the empty-cutoff = full-history semantics; mirrored in prompts/address-review.md. - Reorder Step 1/Step 2 in commands/address-review.md so input parsing runs before repo detection, matching prompts/address-review.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent bd809c2 commit 987a697

2 files changed

Lines changed: 38 additions & 23 deletions

File tree

commands/address-review.md

Lines changed: 34 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -6,18 +6,7 @@ Fetch review comments from a GitHub PR in this repository, triage them, and crea
66

77
# Instructions
88

9-
## Step 1: Determine the Repository
10-
11-
If the user input is a full GitHub URL, extract `org/repo` from the URL and use that as `REPO`.
12-
Otherwise, detect the repository from the current checkout:
13-
14-
```bash
15-
REPO=$(gh repo view --json nameWithOwner -q .nameWithOwner)
16-
```
17-
18-
If this command fails, ensure `gh` CLI is installed and authenticated (`gh auth status`).
19-
20-
## Step 2: Parse User Input
9+
## Step 1: Parse User Input
2110

2211
The user's input is: $ARGUMENTS
2312

@@ -41,7 +30,18 @@ Then extract the PR number and optional review/comment ID from the remaining inp
4130

4231
- Extract org/repo from URL path: `github.com/{org}/{repo}/pull/{PR_NUMBER}`
4332
- Extract fragment ID after `#` (e.g., `pullrequestreview-123456789``123456789`)
44-
- If a full GitHub URL is provided, use the org/repo from the URL instead of the current repo
33+
- If a full GitHub URL is provided, capture the URL's `org/repo` now so Step 2 can use it without calling `gh repo view`.
34+
35+
## Step 2: Determine the Repository
36+
37+
- If Step 1 extracted `org/repo` from a full GitHub URL, use that as `REPO`.
38+
- Otherwise, detect the repository from the current checkout:
39+
40+
```bash
41+
REPO=$(gh repo view --json nameWithOwner -q .nameWithOwner)
42+
```
43+
44+
If `gh repo view` fails, ensure `gh` CLI is installed and authenticated (`gh auth status`).
4545

4646
## Step 3: Determine Scan Window and Summary Cutoff
4747

@@ -51,17 +51,21 @@ For full-PR scans (plain PR number or PR URL with no specific review/comment anc
5151
- If the user explicitly said `check all reviews`, ignore the cutoff and scan the full PR history.
5252
- If the input is a specific review URL or specific issue-comment URL, fetch that exact target even if it predates the latest summary comment.
5353

54-
Fetch the latest summary comment before collecting review data:
54+
Fetch the latest summary comment before collecting review data. Extract only the timestamp, guarding against the empty-array case (`jq ... | last` emits JSON `null` when nothing matches):
5555

5656
```bash
57-
gh api --paginate repos/${REPO}/issues/{PR_NUMBER}/comments | jq -s '[.[].[] | select((.body // "") | contains("<!-- address-review-summary -->")) | {id: .id, created_at: .created_at, html_url: .html_url}] | sort_by(.created_at) | last'
57+
REVIEW_CUTOFF_AT=$(
58+
gh api --paginate repos/${REPO}/issues/{PR_NUMBER}/comments \
59+
| jq -rs '[.[].[] | select((.body // "") | contains("<!-- address-review-summary -->")) | {id: .id, created_at: .created_at, html_url: .html_url}] | sort_by(.created_at) | last | if . == null then "" else .created_at end'
60+
)
61+
# Empty string → no prior summary comment; scan full PR history.
5862
```
5963

6064
Cutoff rules:
6165

62-
- If a summary comment exists and `CHECK_ALL_REVIEWS` is false, set `REVIEW_CUTOFF_AT` to that comment's `created_at` timestamp.
66+
- `REVIEW_CUTOFF_AT` is empty when no summary comment exists; treat that as "scan full PR history" and do not filter by timestamp.
67+
- If `REVIEW_CUTOFF_AT` is non-empty and `CHECK_ALL_REVIEWS` is false, use it as the cutoff.
6368
- Use exact timestamps in user-facing status updates, for example: "Scanning review activity after 2026-04-01T20:14:33Z."
64-
- If no summary comment exists, scan the full PR history.
6569
- When a cutoff is active, keep enough older thread context to understand new replies, but only triage items whose own timestamp or latest thread activity is after `REVIEW_CUTOFF_AT`.
6670
- If no items survive the cutoff, say that no new review feedback was found since the last summary comment and remind the user they can say `check all reviews` to rescan the full PR.
6771

@@ -223,6 +227,9 @@ Present the requested items with full context and ask the user for a decision on
223227

224228
Post rationale replies to the specified items explaining why they are being deferred or skipped. By default, do not resolve threads in `r` unless the user explicitly asks to resolve them (for example, `r3,5 + resolve`). Accept only `SKIPPED`/`DISCUSS` item numbers, ranges, `r all skipped`, or `r all discuss`. If the selection includes any `MUST-FIX` item (including `r all must-fix`), do not post replies; direct the user to `f` or explicit deferral (`f+i` / `m`).
225229

230+
- Bare `r` (with no items and no `all` qualifier) is ambiguous. Do not reply to anything. Prompt the user to specify item numbers or ranges, or one of `r all skipped` / `r all discuss`.
231+
- Bare `r all` (without `skipped` or `discuss`) is also ambiguous. Do not reply to anything. Respond with: `"r all" is ambiguous — use "r all skipped", "r all discuss", or run both: "r all skipped" then "r all discuss"`.
232+
226233
### Action `m` — Merge as-is
227234

228235
1. Create a follow-up GitHub issue (see Step 9) bundling `MUST-FIX`, `DISCUSS`, and non-trivial `SKIPPED` items.
@@ -320,12 +327,16 @@ MUST_FIX_BLOCK="${MUST_FIX_SECTION}"
320327

321328
DISCUSS_SECTION=""
322329
if [ -n "${DISCUSS_ITEMS}" ]; then
323-
printf -v DISCUSS_SECTION '### Discuss items\n%s\n' "${DISCUSS_ITEMS}"
330+
DISCUSS_SECTION="### Discuss items
331+
${DISCUSS_ITEMS}
332+
"
324333
fi
325334

326335
SKIPPED_SECTION=""
327336
if [ -n "${SKIPPED_ITEMS}" ]; then
328-
printf -v SKIPPED_SECTION '### Skipped items (non-trivial)\n%s\n' "${SKIPPED_ITEMS}"
337+
SKIPPED_SECTION="### Skipped items (non-trivial)
338+
${SKIPPED_ITEMS}
339+
"
329340
fi
330341

331342
if [ -z "${MUST_FIX_BLOCK}${DISCUSS_SECTION}${SKIPPED_SECTION}" ]; then
@@ -382,7 +393,10 @@ Suggested structure:
382393

383394
```bash
384395
summary_body_file="$(mktemp)"
385-
trap 'rm -f "${summary_body_file}"' EXIT
396+
# Include ${issue_body_file:-} so this trap also cleans up the Step 9 temp file
397+
# when `f+i` runs both steps in the same shell (each `trap ... EXIT` replaces
398+
# the previous handler in bash).
399+
trap 'rm -f "${issue_body_file:-}" "${summary_body_file}"' EXIT
386400
{
387401
printf '<!-- address-review-summary -->\n'
388402
printf '## Address-review summary\n\n'

prompts/address-review.md

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -61,9 +61,10 @@ Execution flow when terminal access is available:
6161
- The summary marker is a PR issue comment whose body contains `<!-- address-review-summary -->`.
6262
- If `CHECK_ALL_REVIEWS` is true, ignore the cutoff and scan the full PR history.
6363
- If the input is a specific review URL or specific issue-comment URL, fetch that exact target even if it predates the latest summary comment.
64-
- Fetch the latest summary comment before collecting review data:
65-
`gh api --paginate repos/${REPO}/issues/{PR_NUMBER}/comments | jq -s '[.[].[] | select(((.body // "") | contains("<!-- address-review-summary -->"))) | {id: .id, created_at: .created_at, html_url: .html_url}] | sort_by(.created_at) | last'`
66-
- If a summary comment exists and `CHECK_ALL_REVIEWS` is false, set `REVIEW_CUTOFF_AT` to that comment's `created_at`.
64+
- Fetch the latest summary comment before collecting review data. Use a null-safe extraction so an empty result becomes an empty string (not JSON `null`):
65+
`REVIEW_CUTOFF_AT=$(gh api --paginate repos/${REPO}/issues/{PR_NUMBER}/comments | jq -rs '[.[].[] | select(((.body // "") | contains("<!-- address-review-summary -->"))) | {id: .id, created_at: .created_at, html_url: .html_url}] | sort_by(.created_at) | last | if . == null then "" else .created_at end')`
66+
- An empty `REVIEW_CUTOFF_AT` means no prior summary comment; scan full history.
67+
- If `REVIEW_CUTOFF_AT` is non-empty and `CHECK_ALL_REVIEWS` is false, use it as the cutoff.
6768
- Use exact timestamps in user-facing status updates, for example `2026-04-01T20:14:33Z`.
6869
- If no items survive the cutoff, tell me no new review feedback was found since that summary comment and remind me I can say `check all reviews`.
6970

0 commit comments

Comments
 (0)