From 6b8094c4920f2e73a15b449a2bd67c7de064192a Mon Sep 17 00:00:00 2001 From: Martin conur Date: Mon, 1 Jun 2026 19:04:57 -0400 Subject: [PATCH 1/5] task-reviewer: tracker-aware spec-issue cross-check (claude-jira/notion/local) (#144) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dispatcher and the `/reviewer` prompt were both GitHub-only: numeric / URL parsing in `parse_issue_number`, `Closes/Fixes/Resolves #N` body auto-detect, and the GH-flavored URL synthesis — plus the prompt hardcoded `gh issue view ` for spec lookup. Net result: `task-reviewer ` in claude-jira / claude-notion / claude-local silently degraded to a diff-only review. Dispatcher: keep the 4 bodies byte-identical (`task-reviewer-claude` drift group stays intact); detect the loadout from the dispatcher's own file path and branch internally. claude-gh keeps numeric / GH-issue-URL parsing and PR-body auto-detect; non-gh loadouts accept any non-empty 2nd positional arg as an opaque spec identifier and pass it through to /reviewer unchanged. With no 2nd arg, non-gh emits a clear "PR-body auto-detect is GitHub-only — pass the spec id explicitly" advisory and proceeds diff-only. Prompts: legitimately differ per loadout (the previous byte-identical state was accidental — never part of the drift manifest). claude-jira points spec lookup at `mcp__atlassian__getJiraIssue`; claude-notion at `mcp__notion__notion-fetch`; claude-local at `Read("tasks/-.md")`. Shared spine (authority boundaries, radio handoff intents, verdict structure) stays consistent — pinned by two new assertions in `tests/drift_check.bats` that replace the old byte-identical check. Tests: extend `tests/task_reviewer.bats` with per-loadout pass-through coverage for Jira keys / Notion URLs / local slugs; ignore-GH-Closes-in-PR- body diff-only behavior; and the new advisory copy. claude-gh path stays green (regression test pins the original GitHub-flavored advisory). Docs: README's `task-reviewer` section now lists each loadout's spec-id shape and notes that PR-body auto-detect is GH-only. CHANGELOG `Unreleased` gets a `Fixed` entry referencing #144 (lands before #143's v0.3.0 cut). Closes #144 Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 4 ++ README.md | 11 ++-- claude-gh/bin/task-reviewer | 94 ++++++++++++++++++++++-------- claude-jira/bin/task-reviewer | 94 ++++++++++++++++++++++-------- claude-jira/commands/reviewer.md | 22 +++---- claude-local/bin/task-reviewer | 94 ++++++++++++++++++++++-------- claude-local/commands/reviewer.md | 31 +++++----- claude-notion/bin/task-reviewer | 94 ++++++++++++++++++++++-------- claude-notion/commands/reviewer.md | 30 +++++----- tests/drift_check.bats | 35 +++++++++-- tests/task_reviewer.bats | 85 +++++++++++++++++++++++++++ 11 files changed, 447 insertions(+), 147 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9eae52e..ed8041a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), ## [Unreleased] +### Fixed + +- **`task-reviewer`'s spec-issue cross-check now works on `claude-jira` / `claude-notion` / `claude-local`.** The dispatcher and the `/reviewer` prompt were both GitHub-only: `parse_issue_number` rejected anything that wasn't a bare integer or `github.com/.../issues/N`; PR-body auto-detect only matched `Closes #N`; the URL-synthesis step always emitted a GitHub issues URL; and the prompt hardcoded `gh issue view ` for spec lookup. Net result: in any non-gh loadout, `task-reviewer ` silently degraded to a diff-only review. The dispatcher is now loadout-aware (detects from its own file path) — `claude-gh` keeps numeric / URL parsing and PR-body auto-detect; `claude-jira` / `claude-notion` / `claude-local` accept any non-empty 2nd positional arg as an opaque spec identifier (Jira key, Notion URL, local task slug) and pass it through to `/reviewer` unchanged. The 4 dispatcher bodies stay byte-identical (drift-checked). Each loadout's `commands/reviewer.md` now points the spec-lookup step at the right tool: GH stays on `gh issue view`; `claude-jira` uses `mcp__atlassian__getJiraIssue`; `claude-notion` uses `mcp__notion__notion-fetch`; `claude-local` reads `tasks/-.md` via the `Read` tool. With no 2nd arg in a non-gh loadout, the wrapper emits a clear "PR-body auto-detect is GitHub-only — pass the spec id explicitly" advisory and proceeds diff-only. **Upgrading**: re-run `task-init ` after pulling this change so the updated `/reviewer` prompt lands in `.claude/commands/`. (#144) + ### Changed - **`task-reviewer` redesigned from a long-lived listener-tab model into a dispatch-style worker.** New surface: `task-reviewer [] [--no-auto] [--no-launch]` (claude variants) / `[--no-trust-all] [--no-launch]` (kiro). Each invocation spawns a fresh zellij tab + git worktree on the PR's head ref, runs a thorough review (spec-compliance cross-check + `code-review` skill on the diff), posts a single PR comment with findings, and radios PM back with `review-complete-clean` or `review-complete-with-findings`. **Auto-permission is the default** — the `/reviewer` prompt's authority boundaries rule out merge / push / approve / close / Status edits, so hands-off dispatch is safe; pass `--no-auto` (or `--no-trust-all` for kiro) to drop into the interactive permission path. PM no longer needs a pre-spawned reviewer tab — the previous no-arg, in-place tab takeover (shipped in #135, rolled out in #137) is gone. Affects `claude-gh`, `claude-jira`, `claude-notion`, `claude-local`, and `kiro-gh`. The per-loadout `bin/task-reviewer`, the `/reviewer` slash command + kiro agent, PM's reviewer-delegation guidance, and the README / workflow docs were all rewritten. (#138) diff --git a/README.md b/README.md index eea6eec..48abd12 100644 --- a/README.md +++ b/README.md @@ -504,13 +504,13 @@ On its next turn the worker sees the ping, sets the project Status field to `Don To shift PR review off the PM's (Opus) tab and onto a cheaper Sonnet model, dispatch a one-shot reviewer worker per PR: ```bash -task-reviewer [] +task-reviewer [] ``` `task-reviewer` spawns a fresh zellij tab + git worktree on the PR's head ref, then runs the `/reviewer` slash command (or the kiro `reviewer` agent) inside it on Sonnet (`ANTHROPIC_MODEL=claude-sonnet-4-6` by default — pre-set the env var to override). The PM's tab stays focused. The reviewer: -1. Reads the spec issue (passed as the second arg, or auto-detected from the PR body's first `Closes #N` / `Fixes #N` / `Resolves #N` line, case-insensitive). +1. Reads the spec (passed as the second arg — issue number / URL for `claude-gh` / `kiro-gh`; Jira key for `claude-jira`; Notion page URL for `claude-notion`; local task slug or path for `claude-local`). On `claude-gh` / `kiro-gh` only, the wrapper also auto-detects from the PR body's first `Closes #N` / `Fixes #N` / `Resolves #N` line (case-insensitive); non-gh loadouts require the spec identifier explicitly because PR bodies don't carry their tracker's linking convention. 2. Reads the PR diff + comments. 3. Cross-checks the diff against the spec, then runs the `code-review` skill on top (claude variants — kiro stays prompt-driven). 4. Posts **one** thorough PR comment with spec-compliance findings, code-review findings, and a verdict (`clean`, `clean-with-nits`, or `changes-requested`). @@ -519,9 +519,12 @@ The reviewer: PM still decides whether to merge or request changes — the reviewer never approves, merges, closes, or mutates Status. The tab stays open showing the analysis; clean up the worktree later with `task-done --remove-worktree`. ```bash -task-reviewer 42 # PR by number, auto-detect issue (auto-permission default) +task-reviewer 42 # claude-gh / kiro-gh: PR by number, auto-detect issue task-reviewer https://github.com/owner/repo/pull/42 # PR by URL -task-reviewer 42 38 # PR + spec issue explicit +task-reviewer 42 38 # claude-gh / kiro-gh: PR + GitHub issue explicit +task-reviewer 42 PROJ-123 # claude-jira: PR + Jira key explicit (no auto-detect) +task-reviewer 42 https://notion.so/page-id # claude-notion: PR + Notion page URL explicit +task-reviewer 42 042-add-login # claude-local: PR + local task slug explicit task-reviewer 42 --no-auto # opt out of auto-permission (interactive review) ``` diff --git a/claude-gh/bin/task-reviewer b/claude-gh/bin/task-reviewer index 4d3273b..d70f35f 100755 --- a/claude-gh/bin/task-reviewer +++ b/claude-gh/bin/task-reviewer @@ -35,11 +35,17 @@ PR comment, and radios PM back with a verdict — clean or with findings. Positional arguments: The PR to review. Accepts a GitHub PR URL or a bare number (resolved in the current repo's `origin`). - Optional — the spec issue this PR claims to close. - If omitted, auto-detected from the PR body's - `Closes #N` / `Fixes #N` / `Resolves #N` line. - With no detectable issue, the reviewer proceeds with - a diff-only review. + Optional — the spec this PR claims to close. + Shape depends on the loadout: + claude-gh — issue number or GitHub issue URL. + If omitted, auto-detected from the + PR body's Closes/Fixes/Resolves #N. + claude-jira — Jira issue key (e.g. PROJ-123). + claude-notion — Notion page URL. + claude-local — local task slug or filename. + PR-body auto-detect is GitHub-only — non-gh loadouts + must pass the 2nd arg explicitly. With no spec id, + the reviewer proceeds with a diff-only review. Options: --no-auto Opt out of --permission-mode auto. Default is auto @@ -88,6 +94,15 @@ done [[ ${#POSITIONAL[@]} -lt 1 ]] && { echo "Error: PR url or number is required" >&2; usage 1; } +# Detect which loadout this dispatcher is running as (claude-gh / claude-jira +# / claude-notion / claude-local). The 4 dispatcher bodies are kept +# byte-identical (sentinel-checked by tools/check-drift.sh) and branch on +# this for tracker-aware behavior — only claude-gh resolves spec issues via +# the GitHub `Closes #N` convention, the others accept any opaque 2nd +# positional arg (Jira key, Notion URL, local slug) and pass it through to +# /reviewer untouched (#144). +AW_LOADOUT="$(basename "$(dirname "$(dirname "${BASH_SOURCE[0]}")")")" + parse_pr_number() { local input="$1" if [[ "$input" =~ ^[0-9]+$ ]]; then @@ -100,16 +115,24 @@ parse_pr_number() { return 1 } +# Spec identifier parsing is tracker-aware. claude-gh keeps numeric / GitHub +# issue URL parsing. claude-jira / claude-notion / claude-local accept any +# non-empty string and pass it through opaquely — the /reviewer prompt knows +# how to resolve it via the loadout-appropriate MCP / file read. parse_issue_number() { local input="$1" - if [[ "$input" =~ ^[0-9]+$ ]]; then - echo "$input"; return 0 - fi - if [[ "$input" == *"github.com"*"/issues/"* ]]; then - echo "$input" | sed 's|.*/issues/||' | sed 's|[^0-9].*||' - return 0 + if [[ "$AW_LOADOUT" == "claude-gh" ]]; then + if [[ "$input" =~ ^[0-9]+$ ]]; then + echo "$input"; return 0 + fi + if [[ "$input" == *"github.com"*"/issues/"* ]]; then + echo "$input" | sed 's|.*/issues/||' | sed 's|[^0-9].*||' + return 0 + fi + return 1 fi - return 1 + [[ -n "$input" ]] || return 1 + printf '%s\n' "$input" } PR_INPUT="${POSITIONAL[0]}" @@ -136,31 +159,54 @@ PR_URL=$(gh pr view "$PR_NUMBER" --json url --jq .url 2>/dev/null) || { PR_BASE_REF=$(gh pr view "$PR_NUMBER" --json baseRefName --jq .baseRefName 2>/dev/null || echo "main") PR_BODY=$(gh pr view "$PR_NUMBER" --json body --jq .body 2>/dev/null || echo "") -# Issue resolution: explicit arg → parse; else scan PR body for first -# Closes/Fixes/Resolves #N line. +# Spec identifier resolution: +# - explicit 2nd arg always wins (parsed by tracker-aware parse_issue_number) +# - claude-gh additionally falls back to scanning the PR body for +# Closes/Fixes/Resolves #N (the GitHub linking convention) +# - non-gh loadouts have no PR-body convention to scan — Jira keys / Notion +# URLs / local slugs don't appear in PR bodies in a standard form — so +# the 2nd positional arg is the only path (#144). +# +# ISSUE_NUMBER is the raw identifier (numeric for gh, opaque string elsewhere) +# persisted into the .info file for task-done. SPEC_IDENTIFIER is what +# /reviewer receives as $ARGUMENTS[1] (a GitHub issues URL for gh, the opaque +# identifier as-is for others). ISSUE_NUMBER="" -ISSUE_URL="" +SPEC_IDENTIFIER="" if [[ -n "$ISSUE_INPUT" ]]; then ISSUE_NUMBER=$(parse_issue_number "$ISSUE_INPUT") || { - echo "Error: could not parse issue number from '$ISSUE_INPUT'" >&2 + if [[ "$AW_LOADOUT" == "claude-gh" ]]; then + echo "Error: could not parse issue number from '$ISSUE_INPUT'" >&2 + else + echo "Error: spec identifier cannot be empty" >&2 + fi exit 1 } - if [[ "$ISSUE_INPUT" == *"github.com"* ]]; then - ISSUE_URL="$ISSUE_INPUT" + if [[ "$AW_LOADOUT" == "claude-gh" ]]; then + [[ "$ISSUE_INPUT" == *"github.com"* ]] && SPEC_IDENTIFIER="$ISSUE_INPUT" + else + SPEC_IDENTIFIER="$ISSUE_INPUT" fi -elif [[ -n "$PR_BODY" ]]; then +elif [[ "$AW_LOADOUT" == "claude-gh" && -n "$PR_BODY" ]]; then ISSUE_NUMBER=$(printf '%s\n' "$PR_BODY" \ | grep -iEo '(close[sd]?|fix(e[sd])?|resolve[sd]?)[[:space:]]+#[0-9]+' \ | head -1 | grep -Eo '[0-9]+' || true) fi -# Build a fallback ISSUE_URL from PR_URL when we only have a number. -if [[ -n "$ISSUE_NUMBER" && -z "$ISSUE_URL" && -n "$PR_URL" ]]; then - ISSUE_URL=$(printf '%s\n' "$PR_URL" | sed "s|/pull/[0-9]*|/issues/$ISSUE_NUMBER|") +# claude-gh: synthesize the GitHub issues URL from PR_URL + ISSUE_NUMBER when +# the caller passed only a number (or we auto-detected from the body). +if [[ "$AW_LOADOUT" == "claude-gh" && -n "$ISSUE_NUMBER" && -z "$SPEC_IDENTIFIER" && -n "$PR_URL" ]]; then + SPEC_IDENTIFIER=$(printf '%s\n' "$PR_URL" | sed "s|/pull/[0-9]*|/issues/$ISSUE_NUMBER|") fi if [[ -z "$ISSUE_NUMBER" ]]; then - echo "⚠ No spec issue associated with PR #$PR_NUMBER (no second arg, no Closes/Fixes in body)." >&2 + if [[ "$AW_LOADOUT" == "claude-gh" ]]; then + echo "⚠ No spec issue associated with PR #$PR_NUMBER (no second arg, no Closes/Fixes in body)." >&2 + else + echo "⚠ No spec identifier passed for PR #$PR_NUMBER." >&2 + echo " PR-body auto-detect is GitHub-only — pass the spec id (Jira key, Notion URL," >&2 + echo " local task slug) as the 2nd positional arg to enable spec-compliance review." >&2 + fi echo " Reviewer will proceed with a diff-only review." >&2 fi @@ -242,7 +288,7 @@ build_claude_cmd() { local mode_prefix="" [[ -n "$AUTO_MODE" ]] && mode_prefix="--permission-mode auto " local slash_args="$PR_URL" - [[ -n "$ISSUE_URL" ]] && slash_args+=" $ISSUE_URL" + [[ -n "$SPEC_IDENTIFIER" ]] && slash_args+=" $SPEC_IDENTIFIER" echo "claude ${mode_prefix}\"/reviewer $slash_args\"" } diff --git a/claude-jira/bin/task-reviewer b/claude-jira/bin/task-reviewer index 4d3273b..d70f35f 100755 --- a/claude-jira/bin/task-reviewer +++ b/claude-jira/bin/task-reviewer @@ -35,11 +35,17 @@ PR comment, and radios PM back with a verdict — clean or with findings. Positional arguments: The PR to review. Accepts a GitHub PR URL or a bare number (resolved in the current repo's `origin`). - Optional — the spec issue this PR claims to close. - If omitted, auto-detected from the PR body's - `Closes #N` / `Fixes #N` / `Resolves #N` line. - With no detectable issue, the reviewer proceeds with - a diff-only review. + Optional — the spec this PR claims to close. + Shape depends on the loadout: + claude-gh — issue number or GitHub issue URL. + If omitted, auto-detected from the + PR body's Closes/Fixes/Resolves #N. + claude-jira — Jira issue key (e.g. PROJ-123). + claude-notion — Notion page URL. + claude-local — local task slug or filename. + PR-body auto-detect is GitHub-only — non-gh loadouts + must pass the 2nd arg explicitly. With no spec id, + the reviewer proceeds with a diff-only review. Options: --no-auto Opt out of --permission-mode auto. Default is auto @@ -88,6 +94,15 @@ done [[ ${#POSITIONAL[@]} -lt 1 ]] && { echo "Error: PR url or number is required" >&2; usage 1; } +# Detect which loadout this dispatcher is running as (claude-gh / claude-jira +# / claude-notion / claude-local). The 4 dispatcher bodies are kept +# byte-identical (sentinel-checked by tools/check-drift.sh) and branch on +# this for tracker-aware behavior — only claude-gh resolves spec issues via +# the GitHub `Closes #N` convention, the others accept any opaque 2nd +# positional arg (Jira key, Notion URL, local slug) and pass it through to +# /reviewer untouched (#144). +AW_LOADOUT="$(basename "$(dirname "$(dirname "${BASH_SOURCE[0]}")")")" + parse_pr_number() { local input="$1" if [[ "$input" =~ ^[0-9]+$ ]]; then @@ -100,16 +115,24 @@ parse_pr_number() { return 1 } +# Spec identifier parsing is tracker-aware. claude-gh keeps numeric / GitHub +# issue URL parsing. claude-jira / claude-notion / claude-local accept any +# non-empty string and pass it through opaquely — the /reviewer prompt knows +# how to resolve it via the loadout-appropriate MCP / file read. parse_issue_number() { local input="$1" - if [[ "$input" =~ ^[0-9]+$ ]]; then - echo "$input"; return 0 - fi - if [[ "$input" == *"github.com"*"/issues/"* ]]; then - echo "$input" | sed 's|.*/issues/||' | sed 's|[^0-9].*||' - return 0 + if [[ "$AW_LOADOUT" == "claude-gh" ]]; then + if [[ "$input" =~ ^[0-9]+$ ]]; then + echo "$input"; return 0 + fi + if [[ "$input" == *"github.com"*"/issues/"* ]]; then + echo "$input" | sed 's|.*/issues/||' | sed 's|[^0-9].*||' + return 0 + fi + return 1 fi - return 1 + [[ -n "$input" ]] || return 1 + printf '%s\n' "$input" } PR_INPUT="${POSITIONAL[0]}" @@ -136,31 +159,54 @@ PR_URL=$(gh pr view "$PR_NUMBER" --json url --jq .url 2>/dev/null) || { PR_BASE_REF=$(gh pr view "$PR_NUMBER" --json baseRefName --jq .baseRefName 2>/dev/null || echo "main") PR_BODY=$(gh pr view "$PR_NUMBER" --json body --jq .body 2>/dev/null || echo "") -# Issue resolution: explicit arg → parse; else scan PR body for first -# Closes/Fixes/Resolves #N line. +# Spec identifier resolution: +# - explicit 2nd arg always wins (parsed by tracker-aware parse_issue_number) +# - claude-gh additionally falls back to scanning the PR body for +# Closes/Fixes/Resolves #N (the GitHub linking convention) +# - non-gh loadouts have no PR-body convention to scan — Jira keys / Notion +# URLs / local slugs don't appear in PR bodies in a standard form — so +# the 2nd positional arg is the only path (#144). +# +# ISSUE_NUMBER is the raw identifier (numeric for gh, opaque string elsewhere) +# persisted into the .info file for task-done. SPEC_IDENTIFIER is what +# /reviewer receives as $ARGUMENTS[1] (a GitHub issues URL for gh, the opaque +# identifier as-is for others). ISSUE_NUMBER="" -ISSUE_URL="" +SPEC_IDENTIFIER="" if [[ -n "$ISSUE_INPUT" ]]; then ISSUE_NUMBER=$(parse_issue_number "$ISSUE_INPUT") || { - echo "Error: could not parse issue number from '$ISSUE_INPUT'" >&2 + if [[ "$AW_LOADOUT" == "claude-gh" ]]; then + echo "Error: could not parse issue number from '$ISSUE_INPUT'" >&2 + else + echo "Error: spec identifier cannot be empty" >&2 + fi exit 1 } - if [[ "$ISSUE_INPUT" == *"github.com"* ]]; then - ISSUE_URL="$ISSUE_INPUT" + if [[ "$AW_LOADOUT" == "claude-gh" ]]; then + [[ "$ISSUE_INPUT" == *"github.com"* ]] && SPEC_IDENTIFIER="$ISSUE_INPUT" + else + SPEC_IDENTIFIER="$ISSUE_INPUT" fi -elif [[ -n "$PR_BODY" ]]; then +elif [[ "$AW_LOADOUT" == "claude-gh" && -n "$PR_BODY" ]]; then ISSUE_NUMBER=$(printf '%s\n' "$PR_BODY" \ | grep -iEo '(close[sd]?|fix(e[sd])?|resolve[sd]?)[[:space:]]+#[0-9]+' \ | head -1 | grep -Eo '[0-9]+' || true) fi -# Build a fallback ISSUE_URL from PR_URL when we only have a number. -if [[ -n "$ISSUE_NUMBER" && -z "$ISSUE_URL" && -n "$PR_URL" ]]; then - ISSUE_URL=$(printf '%s\n' "$PR_URL" | sed "s|/pull/[0-9]*|/issues/$ISSUE_NUMBER|") +# claude-gh: synthesize the GitHub issues URL from PR_URL + ISSUE_NUMBER when +# the caller passed only a number (or we auto-detected from the body). +if [[ "$AW_LOADOUT" == "claude-gh" && -n "$ISSUE_NUMBER" && -z "$SPEC_IDENTIFIER" && -n "$PR_URL" ]]; then + SPEC_IDENTIFIER=$(printf '%s\n' "$PR_URL" | sed "s|/pull/[0-9]*|/issues/$ISSUE_NUMBER|") fi if [[ -z "$ISSUE_NUMBER" ]]; then - echo "⚠ No spec issue associated with PR #$PR_NUMBER (no second arg, no Closes/Fixes in body)." >&2 + if [[ "$AW_LOADOUT" == "claude-gh" ]]; then + echo "⚠ No spec issue associated with PR #$PR_NUMBER (no second arg, no Closes/Fixes in body)." >&2 + else + echo "⚠ No spec identifier passed for PR #$PR_NUMBER." >&2 + echo " PR-body auto-detect is GitHub-only — pass the spec id (Jira key, Notion URL," >&2 + echo " local task slug) as the 2nd positional arg to enable spec-compliance review." >&2 + fi echo " Reviewer will proceed with a diff-only review." >&2 fi @@ -242,7 +288,7 @@ build_claude_cmd() { local mode_prefix="" [[ -n "$AUTO_MODE" ]] && mode_prefix="--permission-mode auto " local slash_args="$PR_URL" - [[ -n "$ISSUE_URL" ]] && slash_args+=" $ISSUE_URL" + [[ -n "$SPEC_IDENTIFIER" ]] && slash_args+=" $SPEC_IDENTIFIER" echo "claude ${mode_prefix}\"/reviewer $slash_args\"" } diff --git a/claude-jira/commands/reviewer.md b/claude-jira/commands/reviewer.md index 3fcec6f..434df74 100644 --- a/claude-jira/commands/reviewer.md +++ b/claude-jira/commands/reviewer.md @@ -1,11 +1,11 @@ --- -description: Reviewer — dispatch-spawned PR reviewer; cross-checks PR against spec issue, runs code-review, posts a single thorough PR comment, radios PM back with verdict -argument-hint: [] +description: Reviewer — dispatch-spawned PR reviewer; cross-checks PR against Jira spec issue, runs code-review, posts a single thorough PR comment, radios PM back with verdict +argument-hint: [] --- You are now in Reviewer mode. -You are a single-shot PR reviewer worker. `task-reviewer` spawned this tab with a fresh worktree on the PR's head ref and handed you the PR URL (and, when known, the spec issue URL) as `$ARGUMENTS`. By default the dispatcher runs you in `--permission-mode auto`, so you can read, post the PR comment, and radio PM back without permission prompts on every tool call — the authority boundaries below stay enforced regardless. Your job is to: +You are a single-shot PR reviewer worker. `task-reviewer` spawned this tab with a fresh worktree on the PR's head ref and handed you the PR URL (and, when passed, the Jira issue key) as `$ARGUMENTS`. By default the dispatcher runs you in `--permission-mode auto`, so you can read, post the PR comment, and radio PM back without permission prompts on every tool call — the authority boundaries below stay enforced regardless. Your job is to: 1. Read the spec issue (if any) — what was the requirement? 2. Read the PR diff + description + existing comments. @@ -15,22 +15,22 @@ You are a single-shot PR reviewer worker. `task-reviewer` spawned this tab with 6. Radio PM back with `review-complete-clean` or `review-complete-with-findings` and a short summary. 7. Idle — the tab stays open so the user can scroll through the analysis. Do NOT auto-cleanup. -Refer to the project's `.claude/gh-workflow.md` for GitHub owner/repo conventions. Use the `gh` CLI for PR + issue I/O: `gh issue view`, `gh pr view`, `gh pr diff`, `gh pr comment`. Read-only `gh` patterns are pre-allowed in `.claude/settings.json`; mutations stay confirmation-gated. +PRs always live in GitHub; specs live in Jira. Refer to the project's `.claude/jira-workflow.md` for Jira project conventions. Use the Atlassian MCP (`mcp__atlassian__getJiraIssue`) for spec lookup and the `gh` CLI for PR I/O: `gh pr view`, `gh pr diff`, `gh pr comment`. Read-only `gh` and the Atlassian read MCPs are pre-allowed in `.claude/settings.json`; mutations stay confirmation-gated. ### Inputs (from `$ARGUMENTS`) `task-reviewer` passes 1–2 positional args: -- **PR url** (required) — the PR you are reviewing. -- **Issue url** (optional) — the spec issue this PR claims to close. The shell wrapper auto-detects from the PR body's first `Closes #N` / `Fixes #N` / `Resolves #N` line (case-insensitive) when this arg is omitted. If neither path produced an issue (no second arg, no link in body) the wrapper emits a diff-only warning and passes you only the PR url. In that case, run a diff-only review and call it out in the verdict body. +- **PR url** (required) — the GitHub PR you are reviewing. +- **Jira issue key** (optional) — the Jira spec issue this PR claims to close (e.g. `PROJ-123`). PR-body auto-detect is GitHub-only and is **not** performed in this loadout; if no 2nd arg is passed, the wrapper emits a diff-only warning and passes you only the PR url. In that case, run a diff-only review and call it out in the verdict body. $ARGUMENTS ### Loop -1. Parse the PR # (and issue #, if present) from `$ARGUMENTS`. -2. If an issue is associated, read it: - ```bash - gh issue view +1. Parse the PR # (and Jira key, if present) from `$ARGUMENTS`. +2. If a Jira issue key is associated, fetch it via the Atlassian MCP: + ``` + mcp__atlassian__getJiraIssue(cloudId: , issueIdOrKey: "") ``` 3. Read the PR: ```bash @@ -62,6 +62,6 @@ You are explicitly NOT authorized to: - Merge PRs (`gh pr merge`) - Close PRs - Push commits or edit branches -- Modify project Status fields +- Modify project Status fields (Jira or otherwise) PM holds those authorities. A `clean` verdict just means "I found nothing worth blocking"; PM decides when to merge. If you think the PR is great, say so in the verdict body — but still escalate the merge decision to PM. diff --git a/claude-local/bin/task-reviewer b/claude-local/bin/task-reviewer index 4d3273b..d70f35f 100755 --- a/claude-local/bin/task-reviewer +++ b/claude-local/bin/task-reviewer @@ -35,11 +35,17 @@ PR comment, and radios PM back with a verdict — clean or with findings. Positional arguments: The PR to review. Accepts a GitHub PR URL or a bare number (resolved in the current repo's `origin`). - Optional — the spec issue this PR claims to close. - If omitted, auto-detected from the PR body's - `Closes #N` / `Fixes #N` / `Resolves #N` line. - With no detectable issue, the reviewer proceeds with - a diff-only review. + Optional — the spec this PR claims to close. + Shape depends on the loadout: + claude-gh — issue number or GitHub issue URL. + If omitted, auto-detected from the + PR body's Closes/Fixes/Resolves #N. + claude-jira — Jira issue key (e.g. PROJ-123). + claude-notion — Notion page URL. + claude-local — local task slug or filename. + PR-body auto-detect is GitHub-only — non-gh loadouts + must pass the 2nd arg explicitly. With no spec id, + the reviewer proceeds with a diff-only review. Options: --no-auto Opt out of --permission-mode auto. Default is auto @@ -88,6 +94,15 @@ done [[ ${#POSITIONAL[@]} -lt 1 ]] && { echo "Error: PR url or number is required" >&2; usage 1; } +# Detect which loadout this dispatcher is running as (claude-gh / claude-jira +# / claude-notion / claude-local). The 4 dispatcher bodies are kept +# byte-identical (sentinel-checked by tools/check-drift.sh) and branch on +# this for tracker-aware behavior — only claude-gh resolves spec issues via +# the GitHub `Closes #N` convention, the others accept any opaque 2nd +# positional arg (Jira key, Notion URL, local slug) and pass it through to +# /reviewer untouched (#144). +AW_LOADOUT="$(basename "$(dirname "$(dirname "${BASH_SOURCE[0]}")")")" + parse_pr_number() { local input="$1" if [[ "$input" =~ ^[0-9]+$ ]]; then @@ -100,16 +115,24 @@ parse_pr_number() { return 1 } +# Spec identifier parsing is tracker-aware. claude-gh keeps numeric / GitHub +# issue URL parsing. claude-jira / claude-notion / claude-local accept any +# non-empty string and pass it through opaquely — the /reviewer prompt knows +# how to resolve it via the loadout-appropriate MCP / file read. parse_issue_number() { local input="$1" - if [[ "$input" =~ ^[0-9]+$ ]]; then - echo "$input"; return 0 - fi - if [[ "$input" == *"github.com"*"/issues/"* ]]; then - echo "$input" | sed 's|.*/issues/||' | sed 's|[^0-9].*||' - return 0 + if [[ "$AW_LOADOUT" == "claude-gh" ]]; then + if [[ "$input" =~ ^[0-9]+$ ]]; then + echo "$input"; return 0 + fi + if [[ "$input" == *"github.com"*"/issues/"* ]]; then + echo "$input" | sed 's|.*/issues/||' | sed 's|[^0-9].*||' + return 0 + fi + return 1 fi - return 1 + [[ -n "$input" ]] || return 1 + printf '%s\n' "$input" } PR_INPUT="${POSITIONAL[0]}" @@ -136,31 +159,54 @@ PR_URL=$(gh pr view "$PR_NUMBER" --json url --jq .url 2>/dev/null) || { PR_BASE_REF=$(gh pr view "$PR_NUMBER" --json baseRefName --jq .baseRefName 2>/dev/null || echo "main") PR_BODY=$(gh pr view "$PR_NUMBER" --json body --jq .body 2>/dev/null || echo "") -# Issue resolution: explicit arg → parse; else scan PR body for first -# Closes/Fixes/Resolves #N line. +# Spec identifier resolution: +# - explicit 2nd arg always wins (parsed by tracker-aware parse_issue_number) +# - claude-gh additionally falls back to scanning the PR body for +# Closes/Fixes/Resolves #N (the GitHub linking convention) +# - non-gh loadouts have no PR-body convention to scan — Jira keys / Notion +# URLs / local slugs don't appear in PR bodies in a standard form — so +# the 2nd positional arg is the only path (#144). +# +# ISSUE_NUMBER is the raw identifier (numeric for gh, opaque string elsewhere) +# persisted into the .info file for task-done. SPEC_IDENTIFIER is what +# /reviewer receives as $ARGUMENTS[1] (a GitHub issues URL for gh, the opaque +# identifier as-is for others). ISSUE_NUMBER="" -ISSUE_URL="" +SPEC_IDENTIFIER="" if [[ -n "$ISSUE_INPUT" ]]; then ISSUE_NUMBER=$(parse_issue_number "$ISSUE_INPUT") || { - echo "Error: could not parse issue number from '$ISSUE_INPUT'" >&2 + if [[ "$AW_LOADOUT" == "claude-gh" ]]; then + echo "Error: could not parse issue number from '$ISSUE_INPUT'" >&2 + else + echo "Error: spec identifier cannot be empty" >&2 + fi exit 1 } - if [[ "$ISSUE_INPUT" == *"github.com"* ]]; then - ISSUE_URL="$ISSUE_INPUT" + if [[ "$AW_LOADOUT" == "claude-gh" ]]; then + [[ "$ISSUE_INPUT" == *"github.com"* ]] && SPEC_IDENTIFIER="$ISSUE_INPUT" + else + SPEC_IDENTIFIER="$ISSUE_INPUT" fi -elif [[ -n "$PR_BODY" ]]; then +elif [[ "$AW_LOADOUT" == "claude-gh" && -n "$PR_BODY" ]]; then ISSUE_NUMBER=$(printf '%s\n' "$PR_BODY" \ | grep -iEo '(close[sd]?|fix(e[sd])?|resolve[sd]?)[[:space:]]+#[0-9]+' \ | head -1 | grep -Eo '[0-9]+' || true) fi -# Build a fallback ISSUE_URL from PR_URL when we only have a number. -if [[ -n "$ISSUE_NUMBER" && -z "$ISSUE_URL" && -n "$PR_URL" ]]; then - ISSUE_URL=$(printf '%s\n' "$PR_URL" | sed "s|/pull/[0-9]*|/issues/$ISSUE_NUMBER|") +# claude-gh: synthesize the GitHub issues URL from PR_URL + ISSUE_NUMBER when +# the caller passed only a number (or we auto-detected from the body). +if [[ "$AW_LOADOUT" == "claude-gh" && -n "$ISSUE_NUMBER" && -z "$SPEC_IDENTIFIER" && -n "$PR_URL" ]]; then + SPEC_IDENTIFIER=$(printf '%s\n' "$PR_URL" | sed "s|/pull/[0-9]*|/issues/$ISSUE_NUMBER|") fi if [[ -z "$ISSUE_NUMBER" ]]; then - echo "⚠ No spec issue associated with PR #$PR_NUMBER (no second arg, no Closes/Fixes in body)." >&2 + if [[ "$AW_LOADOUT" == "claude-gh" ]]; then + echo "⚠ No spec issue associated with PR #$PR_NUMBER (no second arg, no Closes/Fixes in body)." >&2 + else + echo "⚠ No spec identifier passed for PR #$PR_NUMBER." >&2 + echo " PR-body auto-detect is GitHub-only — pass the spec id (Jira key, Notion URL," >&2 + echo " local task slug) as the 2nd positional arg to enable spec-compliance review." >&2 + fi echo " Reviewer will proceed with a diff-only review." >&2 fi @@ -242,7 +288,7 @@ build_claude_cmd() { local mode_prefix="" [[ -n "$AUTO_MODE" ]] && mode_prefix="--permission-mode auto " local slash_args="$PR_URL" - [[ -n "$ISSUE_URL" ]] && slash_args+=" $ISSUE_URL" + [[ -n "$SPEC_IDENTIFIER" ]] && slash_args+=" $SPEC_IDENTIFIER" echo "claude ${mode_prefix}\"/reviewer $slash_args\"" } diff --git a/claude-local/commands/reviewer.md b/claude-local/commands/reviewer.md index 3fcec6f..a0473ed 100644 --- a/claude-local/commands/reviewer.md +++ b/claude-local/commands/reviewer.md @@ -1,46 +1,47 @@ --- -description: Reviewer — dispatch-spawned PR reviewer; cross-checks PR against spec issue, runs code-review, posts a single thorough PR comment, radios PM back with verdict -argument-hint: [] +description: Reviewer — dispatch-spawned PR reviewer; cross-checks PR against local task spec, runs code-review, posts a single thorough PR comment, radios PM back with verdict +argument-hint: [] --- You are now in Reviewer mode. -You are a single-shot PR reviewer worker. `task-reviewer` spawned this tab with a fresh worktree on the PR's head ref and handed you the PR URL (and, when known, the spec issue URL) as `$ARGUMENTS`. By default the dispatcher runs you in `--permission-mode auto`, so you can read, post the PR comment, and radio PM back without permission prompts on every tool call — the authority boundaries below stay enforced regardless. Your job is to: +You are a single-shot PR reviewer worker. `task-reviewer` spawned this tab with a fresh worktree on the PR's head ref and handed you the PR URL (and, when passed, the local task identifier) as `$ARGUMENTS`. By default the dispatcher runs you in `--permission-mode auto`, so you can read, post the PR comment, and radio PM back without permission prompts on every tool call — the authority boundaries below stay enforced regardless. Your job is to: -1. Read the spec issue (if any) — what was the requirement? +1. Read the spec task (if any) — what was the requirement? 2. Read the PR diff + description + existing comments. -3. **Cross-check**: does the PR actually satisfy the issue's spec? Spec-compliance findings are first-class, alongside diff-correctness. +3. **Cross-check**: does the PR actually satisfy the task's spec? Spec-compliance findings are first-class, alongside diff-correctness. 4. Run the `code-review` skill on the diff. 5. Post **one** substantive PR comment with the full analysis (spec compliance + code-review findings + verdict). 6. Radio PM back with `review-complete-clean` or `review-complete-with-findings` and a short summary. 7. Idle — the tab stays open so the user can scroll through the analysis. Do NOT auto-cleanup. -Refer to the project's `.claude/gh-workflow.md` for GitHub owner/repo conventions. Use the `gh` CLI for PR + issue I/O: `gh issue view`, `gh pr view`, `gh pr diff`, `gh pr comment`. Read-only `gh` patterns are pre-allowed in `.claude/settings.json`; mutations stay confirmation-gated. +PRs always live in GitHub; specs live in the repo's `tasks/` directory. Refer to the project's `.claude/local-workflow.md` for the local task-file convention (`tasks/NNN-slug.md` with YAML frontmatter). Use the `Read` tool for spec lookup and the `gh` CLI for PR I/O: `gh pr view`, `gh pr diff`, `gh pr comment`. Read-only `gh` patterns are pre-allowed in `.claude/settings.json`; mutations stay confirmation-gated. ### Inputs (from `$ARGUMENTS`) `task-reviewer` passes 1–2 positional args: -- **PR url** (required) — the PR you are reviewing. -- **Issue url** (optional) — the spec issue this PR claims to close. The shell wrapper auto-detects from the PR body's first `Closes #N` / `Fixes #N` / `Resolves #N` line (case-insensitive) when this arg is omitted. If neither path produced an issue (no second arg, no link in body) the wrapper emits a diff-only warning and passes you only the PR url. In that case, run a diff-only review and call it out in the verdict body. +- **PR url** (required) — the GitHub PR you are reviewing. +- **Local task identifier** (optional) — the task spec this PR claims to close. Accepts a bare slug (`042-add-login`), a filename (`042-add-login.md`), or a full path (`tasks/042-add-login.md`). PR-body auto-detect is GitHub-only and is **not** performed in this loadout; if no 2nd arg is passed, the wrapper emits a diff-only warning and passes you only the PR url. In that case, run a diff-only review and call it out in the verdict body. $ARGUMENTS ### Loop -1. Parse the PR # (and issue #, if present) from `$ARGUMENTS`. -2. If an issue is associated, read it: - ```bash - gh issue view +1. Parse the PR # (and task identifier, if present) from `$ARGUMENTS`. +2. If a task identifier is associated, resolve it to `tasks/-.md` in the repo root and read it: + ``` + Read("/tasks/-.md") ``` + If the arg is a bare slug, glob `tasks/*.md` to locate the file. 3. Read the PR: ```bash gh pr view --comments gh pr diff ``` -4. Cross-check the diff against the spec (if any). Note any deliverables the issue called for that are missing, partial, or implemented differently than specified — these are first-class findings, not nits. +4. Cross-check the diff against the spec (if any). Note any deliverables the task called for that are missing, partial, or implemented differently than specified — these are first-class findings, not nits. 5. Run the `code-review` skill on the diff. The skill orchestrates sub-agents and produces correctness findings. 6. Compose a **single PR comment** with sections: - - **Spec compliance** (omit if diff-only) — did the PR deliver what the issue asked for? List anything missing or off-spec. + - **Spec compliance** (omit if diff-only) — did the PR deliver what the task asked for? List anything missing or off-spec. - **Code-review findings** — correctness, security, edge cases, anything substantive from the skill's output. - **Verdict** — `clean`, `clean-with-nits`, or `changes-requested`. Be explicit. 7. Post the comment: @@ -62,6 +63,6 @@ You are explicitly NOT authorized to: - Merge PRs (`gh pr merge`) - Close PRs - Push commits or edit branches -- Modify project Status fields +- Modify task-file frontmatter (the worker / PM own the status lifecycle) PM holds those authorities. A `clean` verdict just means "I found nothing worth blocking"; PM decides when to merge. If you think the PR is great, say so in the verdict body — but still escalate the merge decision to PM. diff --git a/claude-notion/bin/task-reviewer b/claude-notion/bin/task-reviewer index 4d3273b..d70f35f 100755 --- a/claude-notion/bin/task-reviewer +++ b/claude-notion/bin/task-reviewer @@ -35,11 +35,17 @@ PR comment, and radios PM back with a verdict — clean or with findings. Positional arguments: The PR to review. Accepts a GitHub PR URL or a bare number (resolved in the current repo's `origin`). - Optional — the spec issue this PR claims to close. - If omitted, auto-detected from the PR body's - `Closes #N` / `Fixes #N` / `Resolves #N` line. - With no detectable issue, the reviewer proceeds with - a diff-only review. + Optional — the spec this PR claims to close. + Shape depends on the loadout: + claude-gh — issue number or GitHub issue URL. + If omitted, auto-detected from the + PR body's Closes/Fixes/Resolves #N. + claude-jira — Jira issue key (e.g. PROJ-123). + claude-notion — Notion page URL. + claude-local — local task slug or filename. + PR-body auto-detect is GitHub-only — non-gh loadouts + must pass the 2nd arg explicitly. With no spec id, + the reviewer proceeds with a diff-only review. Options: --no-auto Opt out of --permission-mode auto. Default is auto @@ -88,6 +94,15 @@ done [[ ${#POSITIONAL[@]} -lt 1 ]] && { echo "Error: PR url or number is required" >&2; usage 1; } +# Detect which loadout this dispatcher is running as (claude-gh / claude-jira +# / claude-notion / claude-local). The 4 dispatcher bodies are kept +# byte-identical (sentinel-checked by tools/check-drift.sh) and branch on +# this for tracker-aware behavior — only claude-gh resolves spec issues via +# the GitHub `Closes #N` convention, the others accept any opaque 2nd +# positional arg (Jira key, Notion URL, local slug) and pass it through to +# /reviewer untouched (#144). +AW_LOADOUT="$(basename "$(dirname "$(dirname "${BASH_SOURCE[0]}")")")" + parse_pr_number() { local input="$1" if [[ "$input" =~ ^[0-9]+$ ]]; then @@ -100,16 +115,24 @@ parse_pr_number() { return 1 } +# Spec identifier parsing is tracker-aware. claude-gh keeps numeric / GitHub +# issue URL parsing. claude-jira / claude-notion / claude-local accept any +# non-empty string and pass it through opaquely — the /reviewer prompt knows +# how to resolve it via the loadout-appropriate MCP / file read. parse_issue_number() { local input="$1" - if [[ "$input" =~ ^[0-9]+$ ]]; then - echo "$input"; return 0 - fi - if [[ "$input" == *"github.com"*"/issues/"* ]]; then - echo "$input" | sed 's|.*/issues/||' | sed 's|[^0-9].*||' - return 0 + if [[ "$AW_LOADOUT" == "claude-gh" ]]; then + if [[ "$input" =~ ^[0-9]+$ ]]; then + echo "$input"; return 0 + fi + if [[ "$input" == *"github.com"*"/issues/"* ]]; then + echo "$input" | sed 's|.*/issues/||' | sed 's|[^0-9].*||' + return 0 + fi + return 1 fi - return 1 + [[ -n "$input" ]] || return 1 + printf '%s\n' "$input" } PR_INPUT="${POSITIONAL[0]}" @@ -136,31 +159,54 @@ PR_URL=$(gh pr view "$PR_NUMBER" --json url --jq .url 2>/dev/null) || { PR_BASE_REF=$(gh pr view "$PR_NUMBER" --json baseRefName --jq .baseRefName 2>/dev/null || echo "main") PR_BODY=$(gh pr view "$PR_NUMBER" --json body --jq .body 2>/dev/null || echo "") -# Issue resolution: explicit arg → parse; else scan PR body for first -# Closes/Fixes/Resolves #N line. +# Spec identifier resolution: +# - explicit 2nd arg always wins (parsed by tracker-aware parse_issue_number) +# - claude-gh additionally falls back to scanning the PR body for +# Closes/Fixes/Resolves #N (the GitHub linking convention) +# - non-gh loadouts have no PR-body convention to scan — Jira keys / Notion +# URLs / local slugs don't appear in PR bodies in a standard form — so +# the 2nd positional arg is the only path (#144). +# +# ISSUE_NUMBER is the raw identifier (numeric for gh, opaque string elsewhere) +# persisted into the .info file for task-done. SPEC_IDENTIFIER is what +# /reviewer receives as $ARGUMENTS[1] (a GitHub issues URL for gh, the opaque +# identifier as-is for others). ISSUE_NUMBER="" -ISSUE_URL="" +SPEC_IDENTIFIER="" if [[ -n "$ISSUE_INPUT" ]]; then ISSUE_NUMBER=$(parse_issue_number "$ISSUE_INPUT") || { - echo "Error: could not parse issue number from '$ISSUE_INPUT'" >&2 + if [[ "$AW_LOADOUT" == "claude-gh" ]]; then + echo "Error: could not parse issue number from '$ISSUE_INPUT'" >&2 + else + echo "Error: spec identifier cannot be empty" >&2 + fi exit 1 } - if [[ "$ISSUE_INPUT" == *"github.com"* ]]; then - ISSUE_URL="$ISSUE_INPUT" + if [[ "$AW_LOADOUT" == "claude-gh" ]]; then + [[ "$ISSUE_INPUT" == *"github.com"* ]] && SPEC_IDENTIFIER="$ISSUE_INPUT" + else + SPEC_IDENTIFIER="$ISSUE_INPUT" fi -elif [[ -n "$PR_BODY" ]]; then +elif [[ "$AW_LOADOUT" == "claude-gh" && -n "$PR_BODY" ]]; then ISSUE_NUMBER=$(printf '%s\n' "$PR_BODY" \ | grep -iEo '(close[sd]?|fix(e[sd])?|resolve[sd]?)[[:space:]]+#[0-9]+' \ | head -1 | grep -Eo '[0-9]+' || true) fi -# Build a fallback ISSUE_URL from PR_URL when we only have a number. -if [[ -n "$ISSUE_NUMBER" && -z "$ISSUE_URL" && -n "$PR_URL" ]]; then - ISSUE_URL=$(printf '%s\n' "$PR_URL" | sed "s|/pull/[0-9]*|/issues/$ISSUE_NUMBER|") +# claude-gh: synthesize the GitHub issues URL from PR_URL + ISSUE_NUMBER when +# the caller passed only a number (or we auto-detected from the body). +if [[ "$AW_LOADOUT" == "claude-gh" && -n "$ISSUE_NUMBER" && -z "$SPEC_IDENTIFIER" && -n "$PR_URL" ]]; then + SPEC_IDENTIFIER=$(printf '%s\n' "$PR_URL" | sed "s|/pull/[0-9]*|/issues/$ISSUE_NUMBER|") fi if [[ -z "$ISSUE_NUMBER" ]]; then - echo "⚠ No spec issue associated with PR #$PR_NUMBER (no second arg, no Closes/Fixes in body)." >&2 + if [[ "$AW_LOADOUT" == "claude-gh" ]]; then + echo "⚠ No spec issue associated with PR #$PR_NUMBER (no second arg, no Closes/Fixes in body)." >&2 + else + echo "⚠ No spec identifier passed for PR #$PR_NUMBER." >&2 + echo " PR-body auto-detect is GitHub-only — pass the spec id (Jira key, Notion URL," >&2 + echo " local task slug) as the 2nd positional arg to enable spec-compliance review." >&2 + fi echo " Reviewer will proceed with a diff-only review." >&2 fi @@ -242,7 +288,7 @@ build_claude_cmd() { local mode_prefix="" [[ -n "$AUTO_MODE" ]] && mode_prefix="--permission-mode auto " local slash_args="$PR_URL" - [[ -n "$ISSUE_URL" ]] && slash_args+=" $ISSUE_URL" + [[ -n "$SPEC_IDENTIFIER" ]] && slash_args+=" $SPEC_IDENTIFIER" echo "claude ${mode_prefix}\"/reviewer $slash_args\"" } diff --git a/claude-notion/commands/reviewer.md b/claude-notion/commands/reviewer.md index 3fcec6f..58499a5 100644 --- a/claude-notion/commands/reviewer.md +++ b/claude-notion/commands/reviewer.md @@ -1,46 +1,46 @@ --- -description: Reviewer — dispatch-spawned PR reviewer; cross-checks PR against spec issue, runs code-review, posts a single thorough PR comment, radios PM back with verdict -argument-hint: [] +description: Reviewer — dispatch-spawned PR reviewer; cross-checks PR against Notion spec page, runs code-review, posts a single thorough PR comment, radios PM back with verdict +argument-hint: [] --- You are now in Reviewer mode. -You are a single-shot PR reviewer worker. `task-reviewer` spawned this tab with a fresh worktree on the PR's head ref and handed you the PR URL (and, when known, the spec issue URL) as `$ARGUMENTS`. By default the dispatcher runs you in `--permission-mode auto`, so you can read, post the PR comment, and radio PM back without permission prompts on every tool call — the authority boundaries below stay enforced regardless. Your job is to: +You are a single-shot PR reviewer worker. `task-reviewer` spawned this tab with a fresh worktree on the PR's head ref and handed you the PR URL (and, when passed, the Notion spec page URL) as `$ARGUMENTS`. By default the dispatcher runs you in `--permission-mode auto`, so you can read, post the PR comment, and radio PM back without permission prompts on every tool call — the authority boundaries below stay enforced regardless. Your job is to: -1. Read the spec issue (if any) — what was the requirement? +1. Read the spec page (if any) — what was the requirement? 2. Read the PR diff + description + existing comments. -3. **Cross-check**: does the PR actually satisfy the issue's spec? Spec-compliance findings are first-class, alongside diff-correctness. +3. **Cross-check**: does the PR actually satisfy the spec? Spec-compliance findings are first-class, alongside diff-correctness. 4. Run the `code-review` skill on the diff. 5. Post **one** substantive PR comment with the full analysis (spec compliance + code-review findings + verdict). 6. Radio PM back with `review-complete-clean` or `review-complete-with-findings` and a short summary. 7. Idle — the tab stays open so the user can scroll through the analysis. Do NOT auto-cleanup. -Refer to the project's `.claude/gh-workflow.md` for GitHub owner/repo conventions. Use the `gh` CLI for PR + issue I/O: `gh issue view`, `gh pr view`, `gh pr diff`, `gh pr comment`. Read-only `gh` patterns are pre-allowed in `.claude/settings.json`; mutations stay confirmation-gated. +PRs always live in GitHub; specs live in Notion. Refer to the project's `.claude/notion-workflow.md` for Notion database conventions. Use the Notion MCP (`mcp__notion__notion-fetch`) for spec lookup and the `gh` CLI for PR I/O: `gh pr view`, `gh pr diff`, `gh pr comment`. Read-only `gh` and the Notion read MCPs are pre-allowed in `.claude/settings.json`; mutations stay confirmation-gated. ### Inputs (from `$ARGUMENTS`) `task-reviewer` passes 1–2 positional args: -- **PR url** (required) — the PR you are reviewing. -- **Issue url** (optional) — the spec issue this PR claims to close. The shell wrapper auto-detects from the PR body's first `Closes #N` / `Fixes #N` / `Resolves #N` line (case-insensitive) when this arg is omitted. If neither path produced an issue (no second arg, no link in body) the wrapper emits a diff-only warning and passes you only the PR url. In that case, run a diff-only review and call it out in the verdict body. +- **PR url** (required) — the GitHub PR you are reviewing. +- **Notion page URL** (optional) — the Notion spec page this PR claims to close. PR-body auto-detect is GitHub-only and is **not** performed in this loadout; if no 2nd arg is passed, the wrapper emits a diff-only warning and passes you only the PR url. In that case, run a diff-only review and call it out in the verdict body. $ARGUMENTS ### Loop -1. Parse the PR # (and issue #, if present) from `$ARGUMENTS`. -2. If an issue is associated, read it: - ```bash - gh issue view +1. Parse the PR # (and Notion page URL, if present) from `$ARGUMENTS`. +2. If a Notion page URL is associated, fetch it via the Notion MCP: + ``` + mcp__notion__notion-fetch(urls: [""]) ``` 3. Read the PR: ```bash gh pr view --comments gh pr diff ``` -4. Cross-check the diff against the spec (if any). Note any deliverables the issue called for that are missing, partial, or implemented differently than specified — these are first-class findings, not nits. +4. Cross-check the diff against the spec (if any). Note any deliverables the spec called for that are missing, partial, or implemented differently than specified — these are first-class findings, not nits. 5. Run the `code-review` skill on the diff. The skill orchestrates sub-agents and produces correctness findings. 6. Compose a **single PR comment** with sections: - - **Spec compliance** (omit if diff-only) — did the PR deliver what the issue asked for? List anything missing or off-spec. + - **Spec compliance** (omit if diff-only) — did the PR deliver what the spec asked for? List anything missing or off-spec. - **Code-review findings** — correctness, security, edge cases, anything substantive from the skill's output. - **Verdict** — `clean`, `clean-with-nits`, or `changes-requested`. Be explicit. 7. Post the comment: @@ -62,6 +62,6 @@ You are explicitly NOT authorized to: - Merge PRs (`gh pr merge`) - Close PRs - Push commits or edit branches -- Modify project Status fields +- Modify project Status fields (Notion or otherwise) PM holds those authorities. A `clean` verdict just means "I found nothing worth blocking"; PM decides when to merge. If you think the PR is great, say so in the verdict body — but still escalate the merge decision to PM. diff --git a/tests/drift_check.bats b/tests/drift_check.bats index f4ce292..48bfd4f 100644 --- a/tests/drift_check.bats +++ b/tests/drift_check.bats @@ -125,16 +125,39 @@ EOF assert_output --partial "group(s) checked" } -@test "claude reviewer.md files are byte-identical across loadouts (#138)" { - # The 4 claude `/reviewer` slash commands ship identical prompts. The - # check-drift sentinel-region mechanism doesn't fit markdown cleanly, so - # we just diff the files directly. - local ref="$REPO_ROOT_REAL/claude-gh/commands/reviewer.md" +@test "claude reviewer.md prompts use the right tracker-specific spec-lookup tool (#144)" { + # Per #144, the 4 reviewer.md files are NO LONGER byte-identical — each + # loadout's spec-lookup step legitimately differs (gh issue view vs. Jira + # MCP vs. Notion MCP vs. local file Read). Replace the byte-identical + # assertion (introduced in #138 by accident — the prompts were never + # drift-grouped in the check-drift manifest) with per-loadout content + # assertions: each prompt must reference its tracker's spec-lookup tool. + run grep -q "gh issue view" "$REPO_ROOT_REAL/claude-gh/commands/reviewer.md" + assert_success + run grep -q "mcp__atlassian__getJiraIssue" "$REPO_ROOT_REAL/claude-jira/commands/reviewer.md" + assert_success + run grep -q "mcp__notion__notion-fetch" "$REPO_ROOT_REAL/claude-notion/commands/reviewer.md" + assert_success + run grep -q "tasks/" "$REPO_ROOT_REAL/claude-local/commands/reviewer.md" + assert_success +} + +@test "claude reviewer.md prompts share the same authority-boundaries spine (#144)" { + # Shared invariant across all 4 loadouts: the authority boundaries block + # (no merge, no push, no Status edits, etc.) and the radio handoff intents + # must stay consistent. Drift here would be a real bug. for f in \ + "$REPO_ROOT_REAL/claude-gh/commands/reviewer.md" \ "$REPO_ROOT_REAL/claude-jira/commands/reviewer.md" \ "$REPO_ROOT_REAL/claude-notion/commands/reviewer.md" \ "$REPO_ROOT_REAL/claude-local/commands/reviewer.md"; do - run diff -u "$ref" "$f" + run grep -q "### Authority boundaries" "$f" + assert_success + run grep -q "Merge PRs" "$f" + assert_success + run grep -q "review-complete-clean" "$f" + assert_success + run grep -q "review-complete-with-findings" "$f" assert_success done } diff --git a/tests/task_reviewer.bats b/tests/task_reviewer.bats index de9beb7..ad0756c 100644 --- a/tests/task_reviewer.bats +++ b/tests/task_reviewer.bats @@ -362,6 +362,34 @@ teardown() { assert_stub_called zellij "ANTHROPIC_MODEL=claude-sonnet-4-6" } +# #144: jira loadout must accept a Jira issue key as opaque 2nd-arg and pass +# it through to /reviewer verbatim (no GitHub URL synthesis). +@test "claude-jira task-reviewer: passes Jira issue key through to /reviewer (#144)" { + run "$TASK_REVIEWER_JIRA" 42 PROJ-123 + assert_success + assert_stub_called zellij "/reviewer https://github.com/owner/repo/pull/42 PROJ-123" +} + +# #144: jira loadout must NOT scan PR body for `Closes #N` — that's a +# GitHub-only convention. With no 2nd arg, behavior is diff-only regardless +# of what's in the body. +@test "claude-jira task-reviewer: ignores GitHub Closes/Fixes in PR body — diff-only without 2nd arg (#144)" { + export GH_STUB_PR_BODY="Closes #38" + run "$TASK_REVIEWER_JIRA" 42 + assert_success + # /reviewer must receive only the PR URL, not a synthesized GitHub issues URL. + run grep -F "/reviewer https://github.com/owner/repo/pull/42 https" "$STUB_CALLS_DIR/zellij.calls" + assert_failure +} + +@test "claude-jira task-reviewer: emits 'GitHub-only' advisory when no 2nd arg (#144)" { + run "$TASK_REVIEWER_JIRA" 42 + assert_success + assert_output --partial "No spec identifier passed" + assert_output --partial "auto-detect is GitHub-only" + assert_output --partial "diff-only review" +} + @test "claude-notion task-reviewer: PR by number opens review tab" { run "$TASK_REVIEWER_NOTION" 42 assert_success @@ -381,6 +409,29 @@ teardown() { assert_stub_called zellij "ANTHROPIC_MODEL=claude-sonnet-4-6" } +# #144: notion loadout must accept a Notion page URL as opaque 2nd-arg. +@test "claude-notion task-reviewer: passes Notion page URL through to /reviewer (#144)" { + local notion_url="https://www.notion.so/myws/Spec-Page-1234abcd" + run "$TASK_REVIEWER_NOTION" 42 "$notion_url" + assert_success + assert_stub_called zellij "/reviewer https://github.com/owner/repo/pull/42 $notion_url" +} + +@test "claude-notion task-reviewer: ignores GitHub Closes/Fixes in PR body — diff-only without 2nd arg (#144)" { + export GH_STUB_PR_BODY="Fixes #38" + run "$TASK_REVIEWER_NOTION" 42 + assert_success + run grep -F "/reviewer https://github.com/owner/repo/pull/42 https" "$STUB_CALLS_DIR/zellij.calls" + assert_failure +} + +@test "claude-notion task-reviewer: emits 'GitHub-only' advisory when no 2nd arg (#144)" { + run "$TASK_REVIEWER_NOTION" 42 + assert_success + assert_output --partial "No spec identifier passed" + assert_output --partial "auto-detect is GitHub-only" +} + @test "claude-local task-reviewer: PR by number opens review tab" { run "$TASK_REVIEWER_LOCAL" 42 assert_success @@ -400,6 +451,40 @@ teardown() { assert_stub_called zellij "ANTHROPIC_MODEL=claude-sonnet-4-6" } +# #144: local loadout must accept a local task slug as opaque 2nd-arg. +@test "claude-local task-reviewer: passes local task slug through to /reviewer (#144)" { + run "$TASK_REVIEWER_LOCAL" 42 042-add-login + assert_success + assert_stub_called zellij "/reviewer https://github.com/owner/repo/pull/42 042-add-login" +} + +@test "claude-local task-reviewer: ignores GitHub Closes/Fixes in PR body — diff-only without 2nd arg (#144)" { + export GH_STUB_PR_BODY="Resolves #38" + run "$TASK_REVIEWER_LOCAL" 42 + assert_success + run grep -F "/reviewer https://github.com/owner/repo/pull/42 https" "$STUB_CALLS_DIR/zellij.calls" + assert_failure +} + +@test "claude-local task-reviewer: emits 'GitHub-only' advisory when no 2nd arg (#144)" { + run "$TASK_REVIEWER_LOCAL" 42 + assert_success + assert_output --partial "No spec identifier passed" + assert_output --partial "auto-detect is GitHub-only" +} + +# #144: claude-gh path stays green — the advisory copy is GitHub-flavored, not +# the non-gh "auto-detect is GitHub-only" one (regression for the gh branch). +@test "claude-gh task-reviewer: 'no spec issue' advisory keeps the original GitHub-flavored copy (#144 regression)" { + export GH_STUB_PR_BODY="Just some prose, no spec link." + run "$TASK_REVIEWER_CLAUDE" 42 + assert_success + assert_output --partial "No spec issue associated" + assert_output --partial "no Closes/Fixes in body" + # Must not regress to the non-gh advisory. + refute_output --partial "auto-detect is GitHub-only" +} + # --------------------------------------------------------------------------- # kiro variant # --------------------------------------------------------------------------- From cb44f375a3d18bf41022d9a2649e9a99bcc63d80 Mon Sep 17 00:00:00 2001 From: Martin conur Date: Mon, 1 Jun 2026 20:00:48 -0400 Subject: [PATCH 2/5] =?UTF-8?q?task-reviewer:=20address=20review=20nits=20?= =?UTF-8?q?=E2=80=94=20disable=20histexpand,=20explicit=20Glob,=20notion?= =?UTF-8?q?=20advisory=20assertion=20(#144)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three pre-merge fixes from PM review: 1. **`set +H` in build_claude_cmd**: prepend `set +H;` to the assembled `claude /reviewer ...` command so the spawned `bash -ic` subshell has history expansion disabled. SPEC_IDENTIFIER values containing `!` (Jira key with v2 suffix, local slug like `auth-login!v2`, etc.) would otherwise risk being silently history-substituted before /reviewer ever saw the arg. The widening to opaque non-gh identifiers in this PR is what made the existing surface a real concern. Synced across the 4 dispatcher bodies (drift sentinel preserved). Two new bats tests pin the defense: `!`-in-SPEC_IDENTIFIER lands unmangled in the recorded command, and the assembled command starts with `set +H;` on both non-gh and gh paths. 2. **claude-local prompt: explicit `Glob` tool**: replace the bare verb "glob `tasks/*.md`" with an explicit `Glob("tasks/*.md")` tool call, matching the style of every other tool invocation in the prompt (`Read(...)`, `gh pr view`, `mcp__atlassian__getJiraIssue`). Adds a "do not fall back to Bash(find)" guardrail so the reviewer doesn't trip a permission prompt under `--permission-mode auto` on a path the harness wouldn't pre-allow. 3. **claude-notion advisory test: missing assertion**: add the `--partial "diff-only review"` assertion that claude-jira and claude-local already have. A future regression that dropped the trailer for notion-only would otherwise be invisible to CI. Out of scope for this PR (filed as follow-ups, per PM): kiro-gh migration to SPEC_IDENTIFIER + AW_LOADOUT (#146); .claude/gh-workflow.md doc drift (#147). Full suite: 755/755 green. Drift check: 21 groups passing. Co-Authored-By: Claude Opus 4.7 (1M context) --- claude-gh/bin/task-reviewer | 9 ++++++++- claude-jira/bin/task-reviewer | 9 ++++++++- claude-local/bin/task-reviewer | 9 ++++++++- claude-local/commands/reviewer.md | 6 +++++- claude-notion/bin/task-reviewer | 9 ++++++++- tests/task_reviewer.bats | 25 +++++++++++++++++++++++++ 6 files changed, 62 insertions(+), 5 deletions(-) diff --git a/claude-gh/bin/task-reviewer b/claude-gh/bin/task-reviewer index d70f35f..aae0701 100755 --- a/claude-gh/bin/task-reviewer +++ b/claude-gh/bin/task-reviewer @@ -289,7 +289,14 @@ build_claude_cmd() { [[ -n "$AUTO_MODE" ]] && mode_prefix="--permission-mode auto " local slash_args="$PR_URL" [[ -n "$SPEC_IDENTIFIER" ]] && slash_args+=" $SPEC_IDENTIFIER" - echo "claude ${mode_prefix}\"/reviewer $slash_args\"" + # `set +H` disables bash history expansion (`!foo` event references) in + # the spawned `bash -ic` subshell. Without it, a SPEC_IDENTIFIER that + # contains `!` — e.g. a local task slug like `auth-login!v2` or a Jira + # key mistakenly typed with `!` — can be silently mangled into a history + # substitution before /reviewer ever sees the arg. Non-gh loadouts widen + # this blast radius by accepting opaque strings, so the defense lives in + # the shared assembled command. (#144 round-2 review.) + echo "set +H; claude ${mode_prefix}\"/reviewer $slash_args\"" } # ---------------- radio env injection ---------------- diff --git a/claude-jira/bin/task-reviewer b/claude-jira/bin/task-reviewer index d70f35f..aae0701 100755 --- a/claude-jira/bin/task-reviewer +++ b/claude-jira/bin/task-reviewer @@ -289,7 +289,14 @@ build_claude_cmd() { [[ -n "$AUTO_MODE" ]] && mode_prefix="--permission-mode auto " local slash_args="$PR_URL" [[ -n "$SPEC_IDENTIFIER" ]] && slash_args+=" $SPEC_IDENTIFIER" - echo "claude ${mode_prefix}\"/reviewer $slash_args\"" + # `set +H` disables bash history expansion (`!foo` event references) in + # the spawned `bash -ic` subshell. Without it, a SPEC_IDENTIFIER that + # contains `!` — e.g. a local task slug like `auth-login!v2` or a Jira + # key mistakenly typed with `!` — can be silently mangled into a history + # substitution before /reviewer ever sees the arg. Non-gh loadouts widen + # this blast radius by accepting opaque strings, so the defense lives in + # the shared assembled command. (#144 round-2 review.) + echo "set +H; claude ${mode_prefix}\"/reviewer $slash_args\"" } # ---------------- radio env injection ---------------- diff --git a/claude-local/bin/task-reviewer b/claude-local/bin/task-reviewer index d70f35f..aae0701 100755 --- a/claude-local/bin/task-reviewer +++ b/claude-local/bin/task-reviewer @@ -289,7 +289,14 @@ build_claude_cmd() { [[ -n "$AUTO_MODE" ]] && mode_prefix="--permission-mode auto " local slash_args="$PR_URL" [[ -n "$SPEC_IDENTIFIER" ]] && slash_args+=" $SPEC_IDENTIFIER" - echo "claude ${mode_prefix}\"/reviewer $slash_args\"" + # `set +H` disables bash history expansion (`!foo` event references) in + # the spawned `bash -ic` subshell. Without it, a SPEC_IDENTIFIER that + # contains `!` — e.g. a local task slug like `auth-login!v2` or a Jira + # key mistakenly typed with `!` — can be silently mangled into a history + # substitution before /reviewer ever sees the arg. Non-gh loadouts widen + # this blast radius by accepting opaque strings, so the defense lives in + # the shared assembled command. (#144 round-2 review.) + echo "set +H; claude ${mode_prefix}\"/reviewer $slash_args\"" } # ---------------- radio env injection ---------------- diff --git a/claude-local/commands/reviewer.md b/claude-local/commands/reviewer.md index a0473ed..94454b2 100644 --- a/claude-local/commands/reviewer.md +++ b/claude-local/commands/reviewer.md @@ -32,7 +32,11 @@ $ARGUMENTS ``` Read("/tasks/-.md") ``` - If the arg is a bare slug, glob `tasks/*.md` to locate the file. + If the arg is a bare slug or partial id, locate the file with `Glob` first: + ``` + Glob("tasks/*.md") + ``` + then `Read` the match. Do **not** fall back to `Bash(find ...)` or `Bash(ls ...)` — `Glob` is the canonical tool for this and is already permitted; the bash variants would trip a permission prompt under `--permission-mode auto`. 3. Read the PR: ```bash gh pr view --comments diff --git a/claude-notion/bin/task-reviewer b/claude-notion/bin/task-reviewer index d70f35f..aae0701 100755 --- a/claude-notion/bin/task-reviewer +++ b/claude-notion/bin/task-reviewer @@ -289,7 +289,14 @@ build_claude_cmd() { [[ -n "$AUTO_MODE" ]] && mode_prefix="--permission-mode auto " local slash_args="$PR_URL" [[ -n "$SPEC_IDENTIFIER" ]] && slash_args+=" $SPEC_IDENTIFIER" - echo "claude ${mode_prefix}\"/reviewer $slash_args\"" + # `set +H` disables bash history expansion (`!foo` event references) in + # the spawned `bash -ic` subshell. Without it, a SPEC_IDENTIFIER that + # contains `!` — e.g. a local task slug like `auth-login!v2` or a Jira + # key mistakenly typed with `!` — can be silently mangled into a history + # substitution before /reviewer ever sees the arg. Non-gh loadouts widen + # this blast radius by accepting opaque strings, so the defense lives in + # the shared assembled command. (#144 round-2 review.) + echo "set +H; claude ${mode_prefix}\"/reviewer $slash_args\"" } # ---------------- radio env injection ---------------- diff --git a/tests/task_reviewer.bats b/tests/task_reviewer.bats index ad0756c..cd28ed9 100644 --- a/tests/task_reviewer.bats +++ b/tests/task_reviewer.bats @@ -370,6 +370,27 @@ teardown() { assert_stub_called zellij "/reviewer https://github.com/owner/repo/pull/42 PROJ-123" } +# #144 round-2: SPEC_IDENTIFIER containing `!` must land in the assembled +# command unmangled, and the assembled command must include `set +H` to +# disable bash history expansion in the spawned interactive subshell. Without +# the defense, a Jira / Notion / local id like `auth-login!v2` would be +# silently history-substituted before /reviewer ever saw it. +@test "claude-jira task-reviewer: SPEC_IDENTIFIER with '!' lands unmangled; cmd disables histexpand (#144 round-2)" { + run "$TASK_REVIEWER_JIRA" 42 'PROJ-123!v2' + assert_success + assert_stub_called zellij "PROJ-123!v2" + assert_stub_called zellij "set +H; claude" +} + +@test "claude-gh task-reviewer: assembled command disables histexpand (#144 round-2)" { + # Same defense applies on the claude-gh path even though SPEC_IDENTIFIER + # there is a GitHub issues URL (no `!`) — the 4 dispatcher bodies stay + # byte-identical, and `set +H` is cheap and harmless on gh. + run "$TASK_REVIEWER_CLAUDE" 42 + assert_success + assert_stub_called zellij "set +H; claude" +} + # #144: jira loadout must NOT scan PR body for `Closes #N` — that's a # GitHub-only convention. With no 2nd arg, behavior is diff-only regardless # of what's in the body. @@ -430,6 +451,10 @@ teardown() { assert_success assert_output --partial "No spec identifier passed" assert_output --partial "auto-detect is GitHub-only" + # Mirror claude-jira / claude-local: pin the "diff-only review" trailer so + # a future regression that accidentally drops it for notion-only would be + # visible to CI (#144 round-2 review). + assert_output --partial "diff-only review" } @test "claude-local task-reviewer: PR by number opens review tab" { From 9aa7586f994b9aa6b3bd24198cc772cbb080c0fe Mon Sep 17 00:00:00 2001 From: Martin conur Date: Mon, 1 Jun 2026 20:43:25 -0400 Subject: [PATCH 3/5] task-reviewer: move set +H to call site so env prefix reaches claude (#144 round-3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 introduced `set +H;` inside `build_claude_cmd`'s output, between the RADIO_ENV_PREFIX assignments and `claude`. Bash scopes `VAR=val cmd1; cmd2` assignments to `cmd1` only — so the env prefix attached to the `set` builtin and `claude` never received `TASK_FORCE_ROLE`, `ZELLIJ_TAB`, `ANTHROPIC_MODEL`, or `TASK_FORCE_AUTO_SUBMIT`. Radio routing, Sonnet override, and --auto AUTO_SUBMIT all silently broke. The existing `assert_stub_called zellij` checks only see the command string handed to zellij — they don't evaluate what `bash -ic` actually runs, so the regression was invisible to CI. Fix: drop `set +H;` from `build_claude_cmd` and prepend it at the `aw_launch_tab` call site BEFORE `${RADIO_ENV_PREFIX}`: aw_launch_tab ... "set +H; ${RADIO_ENV_PREFIX}$(build_claude_cmd)" "1" Now the spawned shell sees: `set +H; TASK_FORCE_ROLE=... claude ...`. `set +H` runs (disabling histexpand for the subshell), then the env prefix correctly binds to `claude` (an external command). Synced across the 4 dispatcher bodies (drift sentinel still intact). New bats coverage: - Ordering assertion: `set +H;` must precede `TASK_FORCE_ROLE=` in the assembled command. This regression test would have caught round-2. Also fixed (round-3 nit): claude-local advisory test was missing `assert_output --partial "diff-only review"` — the assertion was added to claude-jira / claude-notion in earlier rounds but the claude-local sibling was missed. A regression that dropped the trailer for local would have been invisible to CI. Full suite: 756/756 green. Drift check: 21 groups passing. Co-Authored-By: Claude Opus 4.7 (1M context) --- claude-gh/bin/task-reviewer | 21 ++++++++------ claude-jira/bin/task-reviewer | 21 ++++++++------ claude-local/bin/task-reviewer | 21 ++++++++------ claude-notion/bin/task-reviewer | 21 ++++++++------ tests/task_reviewer.bats | 51 ++++++++++++++++++++++++++++----- 5 files changed, 92 insertions(+), 43 deletions(-) diff --git a/claude-gh/bin/task-reviewer b/claude-gh/bin/task-reviewer index aae0701..896599e 100755 --- a/claude-gh/bin/task-reviewer +++ b/claude-gh/bin/task-reviewer @@ -289,14 +289,7 @@ build_claude_cmd() { [[ -n "$AUTO_MODE" ]] && mode_prefix="--permission-mode auto " local slash_args="$PR_URL" [[ -n "$SPEC_IDENTIFIER" ]] && slash_args+=" $SPEC_IDENTIFIER" - # `set +H` disables bash history expansion (`!foo` event references) in - # the spawned `bash -ic` subshell. Without it, a SPEC_IDENTIFIER that - # contains `!` — e.g. a local task slug like `auth-login!v2` or a Jira - # key mistakenly typed with `!` — can be silently mangled into a history - # substitution before /reviewer ever sees the arg. Non-gh loadouts widen - # this blast radius by accepting opaque strings, so the defense lives in - # the shared assembled command. (#144 round-2 review.) - echo "set +H; claude ${mode_prefix}\"/reviewer $slash_args\"" + echo "claude ${mode_prefix}\"/reviewer $slash_args\"" } # ---------------- radio env injection ---------------- @@ -316,12 +309,22 @@ RADIO_ENV_PREFIX+="ANTHROPIC_MODEL=$(printf %q "$RESOLVED_MODEL") " # caller's tab (PM or wherever this was dispatched from) instead of snapping # into the new reviewer tab. Same convention task-work --auto adopted in # #133; reviewer always opts in since it's always a dispatched worker. +# +# `set +H; ` prefix disables bash history expansion in the spawned `bash -ic` +# subshell — defends against SPEC_IDENTIFIER values containing `!` (Jira / +# Notion / local slugs with `auth-login!v2` shape, etc.) from being silently +# history-substituted before /reviewer ever sees the arg. Critically, this +# prefix must come BEFORE $RADIO_ENV_PREFIX, not inside build_claude_cmd's +# output, because `VAR=val cmd; cmd2` scopes VAR=val to `cmd` only — putting +# `set +H` between RADIO_ENV_PREFIX and `claude` would attach the env +# assignments to the `set` builtin and they'd never reach `claude` after the +# `;` (radio routing, model selection, AUTO_SUBMIT all dead). (#144 round-3.) echo "Opening zellij tab '$SLUG'..." if [[ -n "$NO_LAUNCH" ]]; then aw_launch_tab "$SLUG" "$WORKTREE_DIR" "" "1" echo "Tab opened at $WORKTREE_DIR — claude NOT launched (--no-launch)." else - aw_launch_tab "$SLUG" "$WORKTREE_DIR" "${RADIO_ENV_PREFIX}$(build_claude_cmd)" "1" + aw_launch_tab "$SLUG" "$WORKTREE_DIR" "set +H; ${RADIO_ENV_PREFIX}$(build_claude_cmd)" "1" echo "Started reviewer in $WORKTREE_DIR for PR #$PR_NUMBER (issue: ${ISSUE_NUMBER:-none}, model: $RESOLVED_MODEL)" fi diff --git a/claude-jira/bin/task-reviewer b/claude-jira/bin/task-reviewer index aae0701..896599e 100755 --- a/claude-jira/bin/task-reviewer +++ b/claude-jira/bin/task-reviewer @@ -289,14 +289,7 @@ build_claude_cmd() { [[ -n "$AUTO_MODE" ]] && mode_prefix="--permission-mode auto " local slash_args="$PR_URL" [[ -n "$SPEC_IDENTIFIER" ]] && slash_args+=" $SPEC_IDENTIFIER" - # `set +H` disables bash history expansion (`!foo` event references) in - # the spawned `bash -ic` subshell. Without it, a SPEC_IDENTIFIER that - # contains `!` — e.g. a local task slug like `auth-login!v2` or a Jira - # key mistakenly typed with `!` — can be silently mangled into a history - # substitution before /reviewer ever sees the arg. Non-gh loadouts widen - # this blast radius by accepting opaque strings, so the defense lives in - # the shared assembled command. (#144 round-2 review.) - echo "set +H; claude ${mode_prefix}\"/reviewer $slash_args\"" + echo "claude ${mode_prefix}\"/reviewer $slash_args\"" } # ---------------- radio env injection ---------------- @@ -316,12 +309,22 @@ RADIO_ENV_PREFIX+="ANTHROPIC_MODEL=$(printf %q "$RESOLVED_MODEL") " # caller's tab (PM or wherever this was dispatched from) instead of snapping # into the new reviewer tab. Same convention task-work --auto adopted in # #133; reviewer always opts in since it's always a dispatched worker. +# +# `set +H; ` prefix disables bash history expansion in the spawned `bash -ic` +# subshell — defends against SPEC_IDENTIFIER values containing `!` (Jira / +# Notion / local slugs with `auth-login!v2` shape, etc.) from being silently +# history-substituted before /reviewer ever sees the arg. Critically, this +# prefix must come BEFORE $RADIO_ENV_PREFIX, not inside build_claude_cmd's +# output, because `VAR=val cmd; cmd2` scopes VAR=val to `cmd` only — putting +# `set +H` between RADIO_ENV_PREFIX and `claude` would attach the env +# assignments to the `set` builtin and they'd never reach `claude` after the +# `;` (radio routing, model selection, AUTO_SUBMIT all dead). (#144 round-3.) echo "Opening zellij tab '$SLUG'..." if [[ -n "$NO_LAUNCH" ]]; then aw_launch_tab "$SLUG" "$WORKTREE_DIR" "" "1" echo "Tab opened at $WORKTREE_DIR — claude NOT launched (--no-launch)." else - aw_launch_tab "$SLUG" "$WORKTREE_DIR" "${RADIO_ENV_PREFIX}$(build_claude_cmd)" "1" + aw_launch_tab "$SLUG" "$WORKTREE_DIR" "set +H; ${RADIO_ENV_PREFIX}$(build_claude_cmd)" "1" echo "Started reviewer in $WORKTREE_DIR for PR #$PR_NUMBER (issue: ${ISSUE_NUMBER:-none}, model: $RESOLVED_MODEL)" fi diff --git a/claude-local/bin/task-reviewer b/claude-local/bin/task-reviewer index aae0701..896599e 100755 --- a/claude-local/bin/task-reviewer +++ b/claude-local/bin/task-reviewer @@ -289,14 +289,7 @@ build_claude_cmd() { [[ -n "$AUTO_MODE" ]] && mode_prefix="--permission-mode auto " local slash_args="$PR_URL" [[ -n "$SPEC_IDENTIFIER" ]] && slash_args+=" $SPEC_IDENTIFIER" - # `set +H` disables bash history expansion (`!foo` event references) in - # the spawned `bash -ic` subshell. Without it, a SPEC_IDENTIFIER that - # contains `!` — e.g. a local task slug like `auth-login!v2` or a Jira - # key mistakenly typed with `!` — can be silently mangled into a history - # substitution before /reviewer ever sees the arg. Non-gh loadouts widen - # this blast radius by accepting opaque strings, so the defense lives in - # the shared assembled command. (#144 round-2 review.) - echo "set +H; claude ${mode_prefix}\"/reviewer $slash_args\"" + echo "claude ${mode_prefix}\"/reviewer $slash_args\"" } # ---------------- radio env injection ---------------- @@ -316,12 +309,22 @@ RADIO_ENV_PREFIX+="ANTHROPIC_MODEL=$(printf %q "$RESOLVED_MODEL") " # caller's tab (PM or wherever this was dispatched from) instead of snapping # into the new reviewer tab. Same convention task-work --auto adopted in # #133; reviewer always opts in since it's always a dispatched worker. +# +# `set +H; ` prefix disables bash history expansion in the spawned `bash -ic` +# subshell — defends against SPEC_IDENTIFIER values containing `!` (Jira / +# Notion / local slugs with `auth-login!v2` shape, etc.) from being silently +# history-substituted before /reviewer ever sees the arg. Critically, this +# prefix must come BEFORE $RADIO_ENV_PREFIX, not inside build_claude_cmd's +# output, because `VAR=val cmd; cmd2` scopes VAR=val to `cmd` only — putting +# `set +H` between RADIO_ENV_PREFIX and `claude` would attach the env +# assignments to the `set` builtin and they'd never reach `claude` after the +# `;` (radio routing, model selection, AUTO_SUBMIT all dead). (#144 round-3.) echo "Opening zellij tab '$SLUG'..." if [[ -n "$NO_LAUNCH" ]]; then aw_launch_tab "$SLUG" "$WORKTREE_DIR" "" "1" echo "Tab opened at $WORKTREE_DIR — claude NOT launched (--no-launch)." else - aw_launch_tab "$SLUG" "$WORKTREE_DIR" "${RADIO_ENV_PREFIX}$(build_claude_cmd)" "1" + aw_launch_tab "$SLUG" "$WORKTREE_DIR" "set +H; ${RADIO_ENV_PREFIX}$(build_claude_cmd)" "1" echo "Started reviewer in $WORKTREE_DIR for PR #$PR_NUMBER (issue: ${ISSUE_NUMBER:-none}, model: $RESOLVED_MODEL)" fi diff --git a/claude-notion/bin/task-reviewer b/claude-notion/bin/task-reviewer index aae0701..896599e 100755 --- a/claude-notion/bin/task-reviewer +++ b/claude-notion/bin/task-reviewer @@ -289,14 +289,7 @@ build_claude_cmd() { [[ -n "$AUTO_MODE" ]] && mode_prefix="--permission-mode auto " local slash_args="$PR_URL" [[ -n "$SPEC_IDENTIFIER" ]] && slash_args+=" $SPEC_IDENTIFIER" - # `set +H` disables bash history expansion (`!foo` event references) in - # the spawned `bash -ic` subshell. Without it, a SPEC_IDENTIFIER that - # contains `!` — e.g. a local task slug like `auth-login!v2` or a Jira - # key mistakenly typed with `!` — can be silently mangled into a history - # substitution before /reviewer ever sees the arg. Non-gh loadouts widen - # this blast radius by accepting opaque strings, so the defense lives in - # the shared assembled command. (#144 round-2 review.) - echo "set +H; claude ${mode_prefix}\"/reviewer $slash_args\"" + echo "claude ${mode_prefix}\"/reviewer $slash_args\"" } # ---------------- radio env injection ---------------- @@ -316,12 +309,22 @@ RADIO_ENV_PREFIX+="ANTHROPIC_MODEL=$(printf %q "$RESOLVED_MODEL") " # caller's tab (PM or wherever this was dispatched from) instead of snapping # into the new reviewer tab. Same convention task-work --auto adopted in # #133; reviewer always opts in since it's always a dispatched worker. +# +# `set +H; ` prefix disables bash history expansion in the spawned `bash -ic` +# subshell — defends against SPEC_IDENTIFIER values containing `!` (Jira / +# Notion / local slugs with `auth-login!v2` shape, etc.) from being silently +# history-substituted before /reviewer ever sees the arg. Critically, this +# prefix must come BEFORE $RADIO_ENV_PREFIX, not inside build_claude_cmd's +# output, because `VAR=val cmd; cmd2` scopes VAR=val to `cmd` only — putting +# `set +H` between RADIO_ENV_PREFIX and `claude` would attach the env +# assignments to the `set` builtin and they'd never reach `claude` after the +# `;` (radio routing, model selection, AUTO_SUBMIT all dead). (#144 round-3.) echo "Opening zellij tab '$SLUG'..." if [[ -n "$NO_LAUNCH" ]]; then aw_launch_tab "$SLUG" "$WORKTREE_DIR" "" "1" echo "Tab opened at $WORKTREE_DIR — claude NOT launched (--no-launch)." else - aw_launch_tab "$SLUG" "$WORKTREE_DIR" "${RADIO_ENV_PREFIX}$(build_claude_cmd)" "1" + aw_launch_tab "$SLUG" "$WORKTREE_DIR" "set +H; ${RADIO_ENV_PREFIX}$(build_claude_cmd)" "1" echo "Started reviewer in $WORKTREE_DIR for PR #$PR_NUMBER (issue: ${ISSUE_NUMBER:-none}, model: $RESOLVED_MODEL)" fi diff --git a/tests/task_reviewer.bats b/tests/task_reviewer.bats index cd28ed9..a2fe49e 100644 --- a/tests/task_reviewer.bats +++ b/tests/task_reviewer.bats @@ -370,16 +370,24 @@ teardown() { assert_stub_called zellij "/reviewer https://github.com/owner/repo/pull/42 PROJ-123" } -# #144 round-2: SPEC_IDENTIFIER containing `!` must land in the assembled -# command unmangled, and the assembled command must include `set +H` to -# disable bash history expansion in the spawned interactive subshell. Without -# the defense, a Jira / Notion / local id like `auth-login!v2` would be -# silently history-substituted before /reviewer ever saw it. +# #144 round-2 + round-3: SPEC_IDENTIFIER containing `!` must land in the +# assembled command unmangled, and the assembled command must include +# `set +H` to disable bash history expansion in the spawned interactive +# subshell. Without the defense, a Jira / Notion / local id like +# `auth-login!v2` would be silently history-substituted before /reviewer +# ever saw it. +# +# Round-3 follow-up: `set +H;` must appear BEFORE the RADIO_ENV_PREFIX +# assignments (TASK_FORCE_ROLE=, ANTHROPIC_MODEL=, etc.), not after. +# `VAR=val cmd1; cmd2` in bash scopes the assignments to `cmd1` only — if +# `set +H` sits between the env prefix and `claude`, the env vars attach +# to the `set` builtin and never reach `claude` (radio routing / +# model selection / AUTO_SUBMIT all dead). @test "claude-jira task-reviewer: SPEC_IDENTIFIER with '!' lands unmangled; cmd disables histexpand (#144 round-2)" { run "$TASK_REVIEWER_JIRA" 42 'PROJ-123!v2' assert_success assert_stub_called zellij "PROJ-123!v2" - assert_stub_called zellij "set +H; claude" + assert_stub_called zellij "set +H;" } @test "claude-gh task-reviewer: assembled command disables histexpand (#144 round-2)" { @@ -388,7 +396,31 @@ teardown() { # byte-identical, and `set +H` is cheap and harmless on gh. run "$TASK_REVIEWER_CLAUDE" 42 assert_success - assert_stub_called zellij "set +H; claude" + assert_stub_called zellij "set +H;" +} + +# #144 round-3: `set +H;` must precede the RADIO_ENV_PREFIX assignments. +# Regression check: the round-2 placement put `set +H;` BETWEEN the env +# prefix and `claude`, which silently neutered env passthrough because +# bash scoped `VAR=val set +H;` to the builtin only. This test pins the +# correct ordering — `set +H` first, then env, then claude. +@test "claude task-reviewer: 'set +H' precedes RADIO_ENV_PREFIX so env vars reach claude (#144 round-3)" { + run "$TASK_REVIEWER_CLAUDE" 42 + assert_success + # Extract the assembled command string from the most recent zellij + # new-tab call. It's the last token after `--` on the line. + local cmd_line + cmd_line=$(grep -m1 -F "new-tab --name review-pr42" "$STUB_CALLS_DIR/zellij.calls") + # `set +H;` must appear before `TASK_FORCE_ROLE=` in the same line. + local set_pos="${cmd_line%%set +H;*}" + local role_pos="${cmd_line%%TASK_FORCE_ROLE=*}" + # If `set +H;` comes first, the prefix-substring-stripped result is + # shorter than for `TASK_FORCE_ROLE=`. Compare lengths. + [[ ${#set_pos} -lt ${#role_pos} ]] || { + echo "ordering wrong: 'set +H;' must come before 'TASK_FORCE_ROLE='" >&2 + echo "cmd: $cmd_line" >&2 + return 1 + } } # #144: jira loadout must NOT scan PR body for `Closes #N` — that's a @@ -496,6 +528,11 @@ teardown() { assert_success assert_output --partial "No spec identifier passed" assert_output --partial "auto-detect is GitHub-only" + # Mirror claude-jira / claude-notion: pin the "diff-only review" trailer + # so a future regression that drops it for local-only would be visible + # to CI (#144 round-3 review — missed in round-1 when the assertion was + # added to claude-jira / claude-notion). + assert_output --partial "diff-only review" } # #144: claude-gh path stays green — the advisory copy is GitHub-flavored, not From d7bed3affa78beb117887b28270574cabf0acde1 Mon Sep 17 00:00:00 2001 From: Martin conur Date: Mon, 1 Jun 2026 21:08:30 -0400 Subject: [PATCH 4/5] task-work: mirror set +H histexpand defense from task-reviewer (#144 round-4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PM's round-3 ask explicitly extended to task-work: "Apply to both task-reviewer and (if the same idiom exists there) task-work." The idiom exists at line ~229 across all 7 task-work dispatchers — same aw_launch_tab call, same bash -ic exposure, same RADIO_ENV_PREFIX env-prefix ordering concern. build_claude_cmd / build_kiro_cmd embeds user-provided URLs ($GH_URL, Jira URL, Notion URL) directly in the assembled command string; GitHub URLs don't normally contain `!`, but the unprotected path is live. Fix: prepend `set +H; ` at the aw_launch_tab call site, BEFORE ${RADIO_ENV_PREFIX}, in all 7 dispatchers (claude-gh, claude-jira, claude-local, claude-notion, kiro-gh, kiro-local, kiro-notion). The 6 drift-grouped files (task-work-claude + task-work-kiro post-info regions) keep their drift-group byte-equality intact; claude-jira's launch site isn't drift-grouped so it carries its own copy. Drift check passes. New bats coverage in tests/claude_gh_task_work.bats: ordering assertion mirroring the task-reviewer test — pins `set +H;` before TASK_FORCE_ROLE= in the assembled command. A regression that re-introduces `set +H` inside build_claude_cmd's output would now fail the test. Also hardened (round-3 nit): the task_reviewer ordering test now explicitly pins presence of both `set +H;` and `TASK_FORCE_ROLE=` before the prefix-length comparison, so a regression that drops TASK_FORCE_ROLE= from the assembled command no longer false-passes the ordering check. Full suite: 757/757 green. Drift check: 21 groups passing. Co-Authored-By: Claude Opus 4.7 (1M context) --- claude-gh/bin/task-work | 12 +++++++++++- claude-jira/bin/task-work | 11 ++++++++++- claude-local/bin/task-work | 12 +++++++++++- claude-notion/bin/task-work | 12 +++++++++++- kiro-gh/bin/task-work | 11 ++++++++++- kiro-local/bin/task-work | 11 ++++++++++- kiro-notion/bin/task-work | 11 ++++++++++- tests/claude_gh_task_work.bats | 25 +++++++++++++++++++++++++ tests/task_reviewer.bats | 8 ++++++++ 9 files changed, 106 insertions(+), 7 deletions(-) diff --git a/claude-gh/bin/task-work b/claude-gh/bin/task-work index 9e49fef..f452c42 100755 --- a/claude-gh/bin/task-work +++ b/claude-gh/bin/task-work @@ -223,10 +223,20 @@ RADIO_ENV_PREFIX="TASK_FORCE_ROLE=$(printf %q "$TASK_FORCE_ROLE_VALUE") ZELLIJ_T # region:worktree-creation-post-info echo "Opening zellij tab '$SLUG'..." +# `set +H; ` prefix disables bash history expansion in the spawned `bash -ic` +# subshell — defends against `!` in user-supplied $GH_URL (or future +# user-input passthroughs that grow on this code path) from being silently +# history-substituted before claude / the worker prompt ever sees the arg. +# Critically, this prefix must come BEFORE $RADIO_ENV_PREFIX, not between +# it and `claude`: `VAR=val cmd1; cmd2` scopes VAR=val to `cmd1` only, so +# putting `set +H` between the env prefix and `claude` would attach the +# env assignments to the `set` builtin and they'd never reach `claude` +# after the `;` (radio routing / model / AUTO_SUBMIT all dead). Mirrors +# the task-reviewer fix. (#144 round-4 review.) if [[ -n "$NO_LAUNCH" ]]; then aw_launch_tab "$SLUG" "$WORKTREE_DIR" "" "${AUTO_MODE:-}" else - aw_launch_tab "$SLUG" "$WORKTREE_DIR" "${RADIO_ENV_PREFIX}$(build_claude_cmd)" "${AUTO_MODE:-}" + aw_launch_tab "$SLUG" "$WORKTREE_DIR" "set +H; ${RADIO_ENV_PREFIX}$(build_claude_cmd)" "${AUTO_MODE:-}" fi if [[ -n "$NO_LAUNCH" ]]; then diff --git a/claude-jira/bin/task-work b/claude-jira/bin/task-work index 7c29275..254056f 100755 --- a/claude-jira/bin/task-work +++ b/claude-jira/bin/task-work @@ -168,7 +168,16 @@ RADIO_ENV_PREFIX="TASK_FORCE_ROLE=$(printf %q "$TASK_FORCE_ROLE_VALUE") ZELLIJ_T # endregion:radio-env-injection echo "Opening zellij tab '$SLUG'..." -aw_launch_tab "$SLUG" "$WORKTREE_DIR" "${RADIO_ENV_PREFIX}$(build_claude_cmd)" "${AUTO_MODE:-}" +# `set +H; ` prefix disables bash history expansion in the spawned `bash -ic` +# subshell — defends against `!` in user-supplied $JIRA_URL / $GH_URL (or +# future user-input passthroughs) from being silently history-substituted +# before claude / the worker prompt ever sees the arg. The prefix must come +# BEFORE $RADIO_ENV_PREFIX: `VAR=val cmd1; cmd2` scopes VAR=val to `cmd1` +# only, so putting `set +H` between the env prefix and `claude` would attach +# the env assignments to the `set` builtin and they'd never reach `claude` +# (radio routing / model / AUTO_SUBMIT all dead). Mirrors task-reviewer. +# (#144 round-4 review.) +aw_launch_tab "$SLUG" "$WORKTREE_DIR" "set +H; ${RADIO_ENV_PREFIX}$(build_claude_cmd)" "${AUTO_MODE:-}" echo "Worker started in $WORKTREE_DIR (branch: $BRANCH, base: $BASE_BRANCH)" diff --git a/claude-local/bin/task-work b/claude-local/bin/task-work index 2b290d2..196da06 100755 --- a/claude-local/bin/task-work +++ b/claude-local/bin/task-work @@ -240,10 +240,20 @@ RADIO_ENV_PREFIX="TASK_FORCE_ROLE=$(printf %q "$TASK_FORCE_ROLE_VALUE") ZELLIJ_T # region:worktree-creation-post-info echo "Opening zellij tab '$SLUG'..." +# `set +H; ` prefix disables bash history expansion in the spawned `bash -ic` +# subshell — defends against `!` in user-supplied $GH_URL (or future +# user-input passthroughs that grow on this code path) from being silently +# history-substituted before claude / the worker prompt ever sees the arg. +# Critically, this prefix must come BEFORE $RADIO_ENV_PREFIX, not between +# it and `claude`: `VAR=val cmd1; cmd2` scopes VAR=val to `cmd1` only, so +# putting `set +H` between the env prefix and `claude` would attach the +# env assignments to the `set` builtin and they'd never reach `claude` +# after the `;` (radio routing / model / AUTO_SUBMIT all dead). Mirrors +# the task-reviewer fix. (#144 round-4 review.) if [[ -n "$NO_LAUNCH" ]]; then aw_launch_tab "$SLUG" "$WORKTREE_DIR" "" "${AUTO_MODE:-}" else - aw_launch_tab "$SLUG" "$WORKTREE_DIR" "${RADIO_ENV_PREFIX}$(build_claude_cmd)" "${AUTO_MODE:-}" + aw_launch_tab "$SLUG" "$WORKTREE_DIR" "set +H; ${RADIO_ENV_PREFIX}$(build_claude_cmd)" "${AUTO_MODE:-}" fi if [[ -n "$NO_LAUNCH" ]]; then diff --git a/claude-notion/bin/task-work b/claude-notion/bin/task-work index 7f38788..adb0dc9 100755 --- a/claude-notion/bin/task-work +++ b/claude-notion/bin/task-work @@ -220,10 +220,20 @@ RADIO_ENV_PREFIX="TASK_FORCE_ROLE=$(printf %q "$TASK_FORCE_ROLE_VALUE") ZELLIJ_T # region:worktree-creation-post-info echo "Opening zellij tab '$SLUG'..." +# `set +H; ` prefix disables bash history expansion in the spawned `bash -ic` +# subshell — defends against `!` in user-supplied $GH_URL (or future +# user-input passthroughs that grow on this code path) from being silently +# history-substituted before claude / the worker prompt ever sees the arg. +# Critically, this prefix must come BEFORE $RADIO_ENV_PREFIX, not between +# it and `claude`: `VAR=val cmd1; cmd2` scopes VAR=val to `cmd1` only, so +# putting `set +H` between the env prefix and `claude` would attach the +# env assignments to the `set` builtin and they'd never reach `claude` +# after the `;` (radio routing / model / AUTO_SUBMIT all dead). Mirrors +# the task-reviewer fix. (#144 round-4 review.) if [[ -n "$NO_LAUNCH" ]]; then aw_launch_tab "$SLUG" "$WORKTREE_DIR" "" "${AUTO_MODE:-}" else - aw_launch_tab "$SLUG" "$WORKTREE_DIR" "${RADIO_ENV_PREFIX}$(build_claude_cmd)" "${AUTO_MODE:-}" + aw_launch_tab "$SLUG" "$WORKTREE_DIR" "set +H; ${RADIO_ENV_PREFIX}$(build_claude_cmd)" "${AUTO_MODE:-}" fi if [[ -n "$NO_LAUNCH" ]]; then diff --git a/kiro-gh/bin/task-work b/kiro-gh/bin/task-work index 62ec065..485607a 100755 --- a/kiro-gh/bin/task-work +++ b/kiro-gh/bin/task-work @@ -218,10 +218,19 @@ RADIO_ENV_PREFIX="TASK_FORCE_ROLE=$(printf %q "$TASK_FORCE_ROLE_VALUE") ZELLIJ_T # region:worktree-creation-post-info echo "Opening zellij tab '$SLUG'..." +# `set +H; ` prefix disables bash history expansion in the spawned `bash -ic` +# subshell — defends against `!` in user-supplied $GH_URL (or future +# user-input passthroughs) from being silently history-substituted before +# kiro / the worker prompt ever sees the arg. The prefix must come BEFORE +# $RADIO_ENV_PREFIX: `VAR=val cmd1; cmd2` scopes VAR=val to `cmd1` only, so +# putting `set +H` between the env prefix and `kiro-cli` would attach the +# env assignments to the `set` builtin and they'd never reach `kiro-cli` +# (radio routing / AUTO_SUBMIT all dead). Mirrors task-reviewer. +# (#144 round-4 review.) if [[ -n "$NO_LAUNCH" ]]; then aw_launch_tab "$SLUG" "$WORKTREE_DIR" "" "${AUTO_MODE:-}" else - aw_launch_tab "$SLUG" "$WORKTREE_DIR" "${RADIO_ENV_PREFIX}$(build_kiro_cmd)" "${AUTO_MODE:-}" + aw_launch_tab "$SLUG" "$WORKTREE_DIR" "set +H; ${RADIO_ENV_PREFIX}$(build_kiro_cmd)" "${AUTO_MODE:-}" fi if [[ -n "$NO_LAUNCH" ]]; then diff --git a/kiro-local/bin/task-work b/kiro-local/bin/task-work index 117b4fd..8d33313 100755 --- a/kiro-local/bin/task-work +++ b/kiro-local/bin/task-work @@ -239,10 +239,19 @@ RADIO_ENV_PREFIX="TASK_FORCE_ROLE=$(printf %q "$TASK_FORCE_ROLE_VALUE") ZELLIJ_T # region:worktree-creation-post-info echo "Opening zellij tab '$SLUG'..." +# `set +H; ` prefix disables bash history expansion in the spawned `bash -ic` +# subshell — defends against `!` in user-supplied $GH_URL (or future +# user-input passthroughs) from being silently history-substituted before +# kiro / the worker prompt ever sees the arg. The prefix must come BEFORE +# $RADIO_ENV_PREFIX: `VAR=val cmd1; cmd2` scopes VAR=val to `cmd1` only, so +# putting `set +H` between the env prefix and `kiro-cli` would attach the +# env assignments to the `set` builtin and they'd never reach `kiro-cli` +# (radio routing / AUTO_SUBMIT all dead). Mirrors task-reviewer. +# (#144 round-4 review.) if [[ -n "$NO_LAUNCH" ]]; then aw_launch_tab "$SLUG" "$WORKTREE_DIR" "" "${AUTO_MODE:-}" else - aw_launch_tab "$SLUG" "$WORKTREE_DIR" "${RADIO_ENV_PREFIX}$(build_kiro_cmd)" "${AUTO_MODE:-}" + aw_launch_tab "$SLUG" "$WORKTREE_DIR" "set +H; ${RADIO_ENV_PREFIX}$(build_kiro_cmd)" "${AUTO_MODE:-}" fi if [[ -n "$NO_LAUNCH" ]]; then diff --git a/kiro-notion/bin/task-work b/kiro-notion/bin/task-work index beb71c5..acd361f 100755 --- a/kiro-notion/bin/task-work +++ b/kiro-notion/bin/task-work @@ -220,10 +220,19 @@ RADIO_ENV_PREFIX="TASK_FORCE_ROLE=$(printf %q "$TASK_FORCE_ROLE_VALUE") ZELLIJ_T # region:worktree-creation-post-info echo "Opening zellij tab '$SLUG'..." +# `set +H; ` prefix disables bash history expansion in the spawned `bash -ic` +# subshell — defends against `!` in user-supplied $GH_URL (or future +# user-input passthroughs) from being silently history-substituted before +# kiro / the worker prompt ever sees the arg. The prefix must come BEFORE +# $RADIO_ENV_PREFIX: `VAR=val cmd1; cmd2` scopes VAR=val to `cmd1` only, so +# putting `set +H` between the env prefix and `kiro-cli` would attach the +# env assignments to the `set` builtin and they'd never reach `kiro-cli` +# (radio routing / AUTO_SUBMIT all dead). Mirrors task-reviewer. +# (#144 round-4 review.) if [[ -n "$NO_LAUNCH" ]]; then aw_launch_tab "$SLUG" "$WORKTREE_DIR" "" "${AUTO_MODE:-}" else - aw_launch_tab "$SLUG" "$WORKTREE_DIR" "${RADIO_ENV_PREFIX}$(build_kiro_cmd)" "${AUTO_MODE:-}" + aw_launch_tab "$SLUG" "$WORKTREE_DIR" "set +H; ${RADIO_ENV_PREFIX}$(build_kiro_cmd)" "${AUTO_MODE:-}" fi if [[ -n "$NO_LAUNCH" ]]; then diff --git a/tests/claude_gh_task_work.bats b/tests/claude_gh_task_work.bats index a1ca38e..bf8ebb7 100644 --- a/tests/claude_gh_task_work.bats +++ b/tests/claude_gh_task_work.bats @@ -548,3 +548,28 @@ _setup_stale_local_base() { run "$CLAUDE_GH_TASK_WORK" --unknown-flag assert_failure } + +# #144 round-4: task-work mirrors task-reviewer's `set +H;` defense. The +# prefix must precede $RADIO_ENV_PREFIX so the env assignments bind to +# `claude` (an external command), not the `set` builtin — `VAR=val cmd1; +# cmd2` scopes VAR=val to `cmd1` only. Pins the ordering to catch a future +# regression where someone re-introduces `set +H` inside build_claude_cmd's +# output (which is what broke round-2 in #144). +@test "task-work: 'set +H' precedes RADIO_ENV_PREFIX so env vars reach claude (#144 round-4)" { + local url="https://github.com/owner/repo/issues/42" + run "$CLAUDE_GH_TASK_WORK" --auto my-feature "$url" + assert_success + local cmd_line + cmd_line=$(grep -m1 -F "new-tab --name my-feature" "$STUB_CALLS_DIR/zellij.calls") + [[ "$cmd_line" == *"set +H;"* ]] || { + echo "missing 'set +H;' in cmd: $cmd_line" >&2; return 1; } + [[ "$cmd_line" == *"TASK_FORCE_ROLE="* ]] || { + echo "missing 'TASK_FORCE_ROLE=' in cmd: $cmd_line" >&2; return 1; } + local set_pos="${cmd_line%%set +H;*}" + local role_pos="${cmd_line%%TASK_FORCE_ROLE=*}" + [[ ${#set_pos} -lt ${#role_pos} ]] || { + echo "ordering wrong: 'set +H;' must come before 'TASK_FORCE_ROLE='" >&2 + echo "cmd: $cmd_line" >&2 + return 1 + } +} diff --git a/tests/task_reviewer.bats b/tests/task_reviewer.bats index a2fe49e..80fcb74 100644 --- a/tests/task_reviewer.bats +++ b/tests/task_reviewer.bats @@ -411,6 +411,14 @@ teardown() { # new-tab call. It's the last token after `--` on the line. local cmd_line cmd_line=$(grep -m1 -F "new-tab --name review-pr42" "$STUB_CALLS_DIR/zellij.calls") + # Harden against the edge case noted in round-3 review: a missing + # TASK_FORCE_ROLE= would make `${cmd_line%%TASK_FORCE_ROLE=*}` equal to + # cmd_line itself, and the prefix-length comparison below would false-pass + # if `set +H;` is present but TASK_FORCE_ROLE= is gone. Pin presence first. + [[ "$cmd_line" == *"set +H;"* ]] || { + echo "missing 'set +H;' in cmd: $cmd_line" >&2; return 1; } + [[ "$cmd_line" == *"TASK_FORCE_ROLE="* ]] || { + echo "missing 'TASK_FORCE_ROLE=' in cmd: $cmd_line" >&2; return 1; } # `set +H;` must appear before `TASK_FORCE_ROLE=` in the same line. local set_pos="${cmd_line%%set +H;*}" local role_pos="${cmd_line%%TASK_FORCE_ROLE=*}" From ac6dd412769e4acf7e34c933e7a34634c50a65af Mon Sep 17 00:00:00 2001 From: Martin conur Date: Tue, 2 Jun 2026 09:17:36 -0400 Subject: [PATCH 5/5] task-reviewer + task-work: printf %q user-input strings; close shell-quoting class (#144 round-5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `set +H` (rounds 3-4) only blocked `!` history expansion. The child `bash -ic` shell still expanded `$var`, `$(...)`, backticks, and other metacharacters inside double-quoted strings — so SPEC_IDENTIFIER (in task-reviewer) and the per-loadout user-input vars in task-work (GH_URL / JIRA_REF / TASK_FILE / NOTION_URL) were still vulnerable. RADIO_ENV_PREFIX values already use `printf %q` for exactly this reason; the inconsistency was the bug. Fix: apply `printf %q` to every user-input string embedded into the assembled bash -ic command. - task-reviewer (drift-grouped — fixed once, propagated to 4 copies): SPEC_IDENTIFIER → `printf %q` before concatenation into slash_args. - task-work (NOT drift-grouped; per-loadout vars differ): - claude-gh: $GH_URL (used in --plan and worker paths) - claude-jira: $JIRA_REF - claude-local: $TASK_FILE - claude-notion: $NOTION_URL - kiro-gh: $GH_URL - kiro-local: $TASK_FILE - kiro-notion: $NOTION_URL Each gets a local q_ = $(printf %q "$") shadow used in the echoed command. New bats coverage: - tests/task_reviewer.bats: SPEC_IDENTIFIER with `$HOME-test` and `PROJ-$(date)` lands escaped in the recorded zellij call (i.e. the child shell can't expand them). Existing `!`-test updated to expect the bash-3.2 `\!` escaped form. - tests/claude_gh_task_work.bats: same shape for GH_URL embedded in query strings (`?$HOME`, `?q=$(date)`). Also took the optional dead-code nit: dropped the unreachable `"spec identifier cannot be empty"` else-branch in task-reviewer's parse_issue_number failure path (the call site already guards on [[ -n "$ISSUE_INPUT" ]], so the empty-input return path is claude-gh-only). After this round, all user-input entering bash -ic is shell-escaped. The remaining strings in build_*_cmd are static literals or ${mode_prefix} (also literal). Shell-quoting class is closed. Drift check: 21 groups passing. Targeted suites green; task_reviewer + claude_gh_task_work both pass with new round-5 tests. Co-Authored-By: Claude Opus 4.7 (1M context) --- claude-gh/bin/task-reviewer | 20 ++++++++++----- claude-gh/bin/task-work | 12 +++++++-- claude-jira/bin/task-reviewer | 20 ++++++++++----- claude-jira/bin/task-work | 10 ++++++-- claude-local/bin/task-reviewer | 20 ++++++++++----- claude-local/bin/task-work | 10 ++++++-- claude-notion/bin/task-reviewer | 20 ++++++++++----- claude-notion/bin/task-work | 10 ++++++-- kiro-gh/bin/task-work | 5 +++- kiro-local/bin/task-work | 5 +++- kiro-notion/bin/task-work | 5 +++- tests/claude_gh_task_work.bats | 31 ++++++++++++++++++++++++ tests/task_reviewer.bats | 43 +++++++++++++++++++++++++++------ 13 files changed, 168 insertions(+), 43 deletions(-) diff --git a/claude-gh/bin/task-reviewer b/claude-gh/bin/task-reviewer index 896599e..5cbf591 100755 --- a/claude-gh/bin/task-reviewer +++ b/claude-gh/bin/task-reviewer @@ -174,12 +174,13 @@ PR_BODY=$(gh pr view "$PR_NUMBER" --json body --jq .body 2>/dev/null || echo "") ISSUE_NUMBER="" SPEC_IDENTIFIER="" if [[ -n "$ISSUE_INPUT" ]]; then + # Only claude-gh's parse_issue_number can fail with a non-empty input + # (numeric / URL parse rejection). Non-gh accepts any non-empty string, + # and the [[ -n "$ISSUE_INPUT" ]] guard above means parse_issue_number + # is never called with an empty input — so the failure path here is + # claude-gh-only (#144 round-5). ISSUE_NUMBER=$(parse_issue_number "$ISSUE_INPUT") || { - if [[ "$AW_LOADOUT" == "claude-gh" ]]; then - echo "Error: could not parse issue number from '$ISSUE_INPUT'" >&2 - else - echo "Error: spec identifier cannot be empty" >&2 - fi + echo "Error: could not parse issue number from '$ISSUE_INPUT'" >&2 exit 1 } if [[ "$AW_LOADOUT" == "claude-gh" ]]; then @@ -288,7 +289,14 @@ build_claude_cmd() { local mode_prefix="" [[ -n "$AUTO_MODE" ]] && mode_prefix="--permission-mode auto " local slash_args="$PR_URL" - [[ -n "$SPEC_IDENTIFIER" ]] && slash_args+=" $SPEC_IDENTIFIER" + # printf %q the user-supplied SPEC_IDENTIFIER so `$var`, `$(...)`, and + # other metacharacters in non-gh tracker identifiers (Jira keys, Notion + # URLs, local slugs) survive the `bash -ic` re-parse without being + # expanded by the child shell. Mirrors how RADIO_ENV_PREFIX values are + # already protected. `set +H` in the launch-site prefix handles `!` + # history expansion; printf %q closes the rest of the metachar class. + # (#144 round-5 review.) + [[ -n "$SPEC_IDENTIFIER" ]] && slash_args+=" $(printf %q "$SPEC_IDENTIFIER")" echo "claude ${mode_prefix}\"/reviewer $slash_args\"" } diff --git a/claude-gh/bin/task-work b/claude-gh/bin/task-work index f452c42..b0a7728 100755 --- a/claude-gh/bin/task-work +++ b/claude-gh/bin/task-work @@ -150,9 +150,17 @@ WORKTREE_DIR="${WORKTREE_BASE}/${SLUG}" # ---------------- command builder ---------------- build_claude_cmd() { + # printf %q the user-supplied GH_URL so `$var`, `$(...)`, backticks, and + # other shell metacharacters survive the `bash -ic` re-parse without + # being expanded by the child shell. Mirrors RADIO_ENV_PREFIX which + # already uses %q for its values. `set +H` in the launch-site prefix + # handles `!` history expansion; printf %q closes the rest of the + # metachar class. (#144 round-5 review.) + local q_gh_url="" + [[ -n "$GH_URL" ]] && q_gh_url=$(printf %q "$GH_URL") if [[ -n "$PLAN_MODE" ]]; then if [[ -n "$GH_URL" ]]; then - echo "claude --permission-mode plan \"/planner $GH_URL\"" + echo "claude --permission-mode plan \"/planner $q_gh_url\"" else echo "claude --permission-mode plan \"/planner\"" fi @@ -161,7 +169,7 @@ build_claude_cmd() { local mode_prefix="" [[ -n "$AUTO_MODE" ]] && mode_prefix="--permission-mode auto " if [[ -n "$GH_URL" ]]; then - echo "claude ${mode_prefix}\"/worker Implement task: $GH_URL\"" + echo "claude ${mode_prefix}\"/worker Implement task: $q_gh_url\"" else echo "claude ${mode_prefix}\"/worker\"" fi diff --git a/claude-jira/bin/task-reviewer b/claude-jira/bin/task-reviewer index 896599e..5cbf591 100755 --- a/claude-jira/bin/task-reviewer +++ b/claude-jira/bin/task-reviewer @@ -174,12 +174,13 @@ PR_BODY=$(gh pr view "$PR_NUMBER" --json body --jq .body 2>/dev/null || echo "") ISSUE_NUMBER="" SPEC_IDENTIFIER="" if [[ -n "$ISSUE_INPUT" ]]; then + # Only claude-gh's parse_issue_number can fail with a non-empty input + # (numeric / URL parse rejection). Non-gh accepts any non-empty string, + # and the [[ -n "$ISSUE_INPUT" ]] guard above means parse_issue_number + # is never called with an empty input — so the failure path here is + # claude-gh-only (#144 round-5). ISSUE_NUMBER=$(parse_issue_number "$ISSUE_INPUT") || { - if [[ "$AW_LOADOUT" == "claude-gh" ]]; then - echo "Error: could not parse issue number from '$ISSUE_INPUT'" >&2 - else - echo "Error: spec identifier cannot be empty" >&2 - fi + echo "Error: could not parse issue number from '$ISSUE_INPUT'" >&2 exit 1 } if [[ "$AW_LOADOUT" == "claude-gh" ]]; then @@ -288,7 +289,14 @@ build_claude_cmd() { local mode_prefix="" [[ -n "$AUTO_MODE" ]] && mode_prefix="--permission-mode auto " local slash_args="$PR_URL" - [[ -n "$SPEC_IDENTIFIER" ]] && slash_args+=" $SPEC_IDENTIFIER" + # printf %q the user-supplied SPEC_IDENTIFIER so `$var`, `$(...)`, and + # other metacharacters in non-gh tracker identifiers (Jira keys, Notion + # URLs, local slugs) survive the `bash -ic` re-parse without being + # expanded by the child shell. Mirrors how RADIO_ENV_PREFIX values are + # already protected. `set +H` in the launch-site prefix handles `!` + # history expansion; printf %q closes the rest of the metachar class. + # (#144 round-5 review.) + [[ -n "$SPEC_IDENTIFIER" ]] && slash_args+=" $(printf %q "$SPEC_IDENTIFIER")" echo "claude ${mode_prefix}\"/reviewer $slash_args\"" } diff --git a/claude-jira/bin/task-work b/claude-jira/bin/task-work index 254056f..d012f9b 100755 --- a/claude-jira/bin/task-work +++ b/claude-jira/bin/task-work @@ -100,9 +100,15 @@ BRANCH="task/${SLUG}" WORKTREE_DIR="${WORKTREE_BASE}/${SLUG}" build_claude_cmd() { + # printf %q the user-supplied JIRA_REF so `$var`, `$(...)`, backticks, and + # other shell metacharacters survive the `bash -ic` re-parse without + # being expanded by the child shell. Mirrors task-reviewer's + # SPEC_IDENTIFIER and RADIO_ENV_PREFIX. (#144 round-5 review.) + local q_jira_ref="" + [[ -n "$JIRA_REF" ]] && q_jira_ref=$(printf %q "$JIRA_REF") if [[ -n "$PLAN_MODE" ]]; then if [[ -n "$JIRA_REF" ]]; then - echo "claude --permission-mode plan \"/planner $JIRA_REF\"" + echo "claude --permission-mode plan \"/planner $q_jira_ref\"" else echo "claude --permission-mode plan \"/planner\"" fi @@ -111,7 +117,7 @@ build_claude_cmd() { local mode_prefix="" [[ -n "$AUTO_MODE" ]] && mode_prefix="--permission-mode auto " if [[ -n "$JIRA_REF" ]]; then - echo "claude ${mode_prefix}\"/worker Implement Jira issue: $JIRA_REF\"" + echo "claude ${mode_prefix}\"/worker Implement Jira issue: $q_jira_ref\"" else echo "claude ${mode_prefix}\"/worker\"" fi diff --git a/claude-local/bin/task-reviewer b/claude-local/bin/task-reviewer index 896599e..5cbf591 100755 --- a/claude-local/bin/task-reviewer +++ b/claude-local/bin/task-reviewer @@ -174,12 +174,13 @@ PR_BODY=$(gh pr view "$PR_NUMBER" --json body --jq .body 2>/dev/null || echo "") ISSUE_NUMBER="" SPEC_IDENTIFIER="" if [[ -n "$ISSUE_INPUT" ]]; then + # Only claude-gh's parse_issue_number can fail with a non-empty input + # (numeric / URL parse rejection). Non-gh accepts any non-empty string, + # and the [[ -n "$ISSUE_INPUT" ]] guard above means parse_issue_number + # is never called with an empty input — so the failure path here is + # claude-gh-only (#144 round-5). ISSUE_NUMBER=$(parse_issue_number "$ISSUE_INPUT") || { - if [[ "$AW_LOADOUT" == "claude-gh" ]]; then - echo "Error: could not parse issue number from '$ISSUE_INPUT'" >&2 - else - echo "Error: spec identifier cannot be empty" >&2 - fi + echo "Error: could not parse issue number from '$ISSUE_INPUT'" >&2 exit 1 } if [[ "$AW_LOADOUT" == "claude-gh" ]]; then @@ -288,7 +289,14 @@ build_claude_cmd() { local mode_prefix="" [[ -n "$AUTO_MODE" ]] && mode_prefix="--permission-mode auto " local slash_args="$PR_URL" - [[ -n "$SPEC_IDENTIFIER" ]] && slash_args+=" $SPEC_IDENTIFIER" + # printf %q the user-supplied SPEC_IDENTIFIER so `$var`, `$(...)`, and + # other metacharacters in non-gh tracker identifiers (Jira keys, Notion + # URLs, local slugs) survive the `bash -ic` re-parse without being + # expanded by the child shell. Mirrors how RADIO_ENV_PREFIX values are + # already protected. `set +H` in the launch-site prefix handles `!` + # history expansion; printf %q closes the rest of the metachar class. + # (#144 round-5 review.) + [[ -n "$SPEC_IDENTIFIER" ]] && slash_args+=" $(printf %q "$SPEC_IDENTIFIER")" echo "claude ${mode_prefix}\"/reviewer $slash_args\"" } diff --git a/claude-local/bin/task-work b/claude-local/bin/task-work index 196da06..c3b46ed 100755 --- a/claude-local/bin/task-work +++ b/claude-local/bin/task-work @@ -138,9 +138,15 @@ WORKTREE_DIR="${WORKTREE_BASE}/${SLUG}" # ---------------- command builder ---------------- build_claude_cmd() { + # printf %q the user-supplied TASK_FILE so `$var`, `$(...)`, backticks, and + # other shell metacharacters survive the `bash -ic` re-parse without + # being expanded by the child shell. Mirrors task-reviewer's + # SPEC_IDENTIFIER and RADIO_ENV_PREFIX. (#144 round-5 review.) + local q_task_file="" + [[ -n "$TASK_FILE" ]] && q_task_file=$(printf %q "$TASK_FILE") if [[ -n "$PLAN_MODE" ]]; then if [[ -n "$TASK_FILE" ]]; then - echo "claude --permission-mode plan \"/planner $TASK_FILE\"" + echo "claude --permission-mode plan \"/planner $q_task_file\"" else echo "claude --permission-mode plan \"/planner\"" fi @@ -149,7 +155,7 @@ build_claude_cmd() { local mode_prefix="" [[ -n "$AUTO_MODE" ]] && mode_prefix="--permission-mode auto " if [[ -n "$TASK_FILE" ]]; then - echo "claude ${mode_prefix}\"/worker $TASK_FILE\"" + echo "claude ${mode_prefix}\"/worker $q_task_file\"" else echo "claude ${mode_prefix}\"/worker\"" fi diff --git a/claude-notion/bin/task-reviewer b/claude-notion/bin/task-reviewer index 896599e..5cbf591 100755 --- a/claude-notion/bin/task-reviewer +++ b/claude-notion/bin/task-reviewer @@ -174,12 +174,13 @@ PR_BODY=$(gh pr view "$PR_NUMBER" --json body --jq .body 2>/dev/null || echo "") ISSUE_NUMBER="" SPEC_IDENTIFIER="" if [[ -n "$ISSUE_INPUT" ]]; then + # Only claude-gh's parse_issue_number can fail with a non-empty input + # (numeric / URL parse rejection). Non-gh accepts any non-empty string, + # and the [[ -n "$ISSUE_INPUT" ]] guard above means parse_issue_number + # is never called with an empty input — so the failure path here is + # claude-gh-only (#144 round-5). ISSUE_NUMBER=$(parse_issue_number "$ISSUE_INPUT") || { - if [[ "$AW_LOADOUT" == "claude-gh" ]]; then - echo "Error: could not parse issue number from '$ISSUE_INPUT'" >&2 - else - echo "Error: spec identifier cannot be empty" >&2 - fi + echo "Error: could not parse issue number from '$ISSUE_INPUT'" >&2 exit 1 } if [[ "$AW_LOADOUT" == "claude-gh" ]]; then @@ -288,7 +289,14 @@ build_claude_cmd() { local mode_prefix="" [[ -n "$AUTO_MODE" ]] && mode_prefix="--permission-mode auto " local slash_args="$PR_URL" - [[ -n "$SPEC_IDENTIFIER" ]] && slash_args+=" $SPEC_IDENTIFIER" + # printf %q the user-supplied SPEC_IDENTIFIER so `$var`, `$(...)`, and + # other metacharacters in non-gh tracker identifiers (Jira keys, Notion + # URLs, local slugs) survive the `bash -ic` re-parse without being + # expanded by the child shell. Mirrors how RADIO_ENV_PREFIX values are + # already protected. `set +H` in the launch-site prefix handles `!` + # history expansion; printf %q closes the rest of the metachar class. + # (#144 round-5 review.) + [[ -n "$SPEC_IDENTIFIER" ]] && slash_args+=" $(printf %q "$SPEC_IDENTIFIER")" echo "claude ${mode_prefix}\"/reviewer $slash_args\"" } diff --git a/claude-notion/bin/task-work b/claude-notion/bin/task-work index adb0dc9..58756e2 100755 --- a/claude-notion/bin/task-work +++ b/claude-notion/bin/task-work @@ -147,9 +147,15 @@ WORKTREE_DIR="${WORKTREE_BASE}/${SLUG}" # ---------------- command builder ---------------- build_claude_cmd() { + # printf %q the user-supplied NOTION_URL so `$var`, `$(...)`, backticks, and + # other shell metacharacters survive the `bash -ic` re-parse without + # being expanded by the child shell. Mirrors task-reviewer's + # SPEC_IDENTIFIER and RADIO_ENV_PREFIX. (#144 round-5 review.) + local q_notion_url="" + [[ -n "$NOTION_URL" ]] && q_notion_url=$(printf %q "$NOTION_URL") if [[ -n "$PLAN_MODE" ]]; then if [[ -n "$NOTION_URL" ]]; then - echo "claude --permission-mode plan \"/planner $NOTION_URL\"" + echo "claude --permission-mode plan \"/planner $q_notion_url\"" else echo "claude --permission-mode plan \"/planner\"" fi @@ -158,7 +164,7 @@ build_claude_cmd() { local mode_prefix="" [[ -n "$AUTO_MODE" ]] && mode_prefix="--permission-mode auto " if [[ -n "$NOTION_URL" ]]; then - echo "claude ${mode_prefix}\"/worker Implement task: $NOTION_URL\"" + echo "claude ${mode_prefix}\"/worker Implement task: $q_notion_url\"" else echo "claude ${mode_prefix}\"/worker\"" fi diff --git a/kiro-gh/bin/task-work b/kiro-gh/bin/task-work index 485607a..d56df7d 100755 --- a/kiro-gh/bin/task-work +++ b/kiro-gh/bin/task-work @@ -157,7 +157,10 @@ build_kiro_cmd() { [[ -n "$MODEL" ]] && cmd+=" --model $MODEL" [[ -n "$TRUST_ALL" ]] && cmd+=" --trust-all-tools" if [[ -n "$GH_URL" ]]; then - cmd+=" \"Implement task: $GH_URL\"" + # printf %q the user-supplied GH_URL so `$var`, `$(...)`, backticks, and + # other shell metacharacters survive the `bash -ic` re-parse without + # being expanded by the child shell. (#144 round-5 review.) + cmd+=" \"Implement task: $(printf %q "$GH_URL")\"" fi echo "$cmd" } diff --git a/kiro-local/bin/task-work b/kiro-local/bin/task-work index 8d33313..8be33d5 100755 --- a/kiro-local/bin/task-work +++ b/kiro-local/bin/task-work @@ -149,7 +149,10 @@ build_kiro_cmd() { [[ -n "$MODEL" ]] && cmd+=" --model $MODEL" [[ -n "$TRUST_ALL" ]] && cmd+=" --trust-all-tools" if [[ -n "$TASK_FILE" ]]; then - cmd+=" \"Implement task: $TASK_FILE\"" + # printf %q the user-supplied TASK_FILE so `$var`, `$(...)`, backticks, + # and other shell metacharacters survive the `bash -ic` re-parse without + # being expanded by the child shell. (#144 round-5 review.) + cmd+=" \"Implement task: $(printf %q "$TASK_FILE")\"" fi echo "$cmd" } diff --git a/kiro-notion/bin/task-work b/kiro-notion/bin/task-work index acd361f..921f006 100755 --- a/kiro-notion/bin/task-work +++ b/kiro-notion/bin/task-work @@ -157,7 +157,10 @@ build_kiro_cmd() { [[ -n "$MODEL" ]] && cmd+=" --model $MODEL" [[ -n "$TRUST_ALL" ]] && cmd+=" --trust-all-tools" if [[ -n "$NOTION_URL" ]]; then - cmd+=" \"Implement task: $NOTION_URL\"" + # printf %q the user-supplied NOTION_URL so `$var`, `$(...)`, backticks, + # and other shell metacharacters survive the `bash -ic` re-parse without + # being expanded by the child shell. (#144 round-5 review.) + cmd+=" \"Implement task: $(printf %q "$NOTION_URL")\"" fi echo "$cmd" } diff --git a/tests/claude_gh_task_work.bats b/tests/claude_gh_task_work.bats index bf8ebb7..c18196a 100644 --- a/tests/claude_gh_task_work.bats +++ b/tests/claude_gh_task_work.bats @@ -549,6 +549,37 @@ _setup_stale_local_base() { assert_failure } +# #144 round-5: GH_URL containing shell metacharacters must reach the +# /worker prompt un-expanded. `printf %q` in build_claude_cmd shell-escapes +# the URL before it's embedded in the assembled `claude "/worker ..."` +# string. Without %q, the child `bash -ic` re-parse would expand `$var`, +# `$(...)`, backticks, etc. inside the double-quoted /worker payload before +# the agent ever saw the arg. GitHub URLs don't normally carry these +# characters, but the dispatcher passes any URL matching `is_github_url` +# through verbatim — so a URL fragment / query string with a stray `$` is +# theoretically possible. +@test "task-work: GH_URL with '\$' in path lands escaped in /worker command (#144 round-5)" { + # is_github_url only checks for `github.com.../issues/` substring, so the + # query string is free to carry arbitrary characters that survive into + # build_claude_cmd. printf %q must escape the `$` before it lands in the + # double-quoted /worker payload. + local url='https://github.com/owner/repo/issues/42?$HOME' + run "$CLAUDE_GH_TASK_WORK" --auto my-feature "$url" + assert_success + # printf %q on bash 3.2 renders `$` as `\$`. + assert_stub_called zellij '\$HOME' + # And the literal /worker prefix is still present (sanity). + assert_stub_called zellij "/worker Implement task:" +} + +@test "task-work: GH_URL with '\$(...)' lands escaped in /worker command (#144 round-5)" { + local url='https://github.com/owner/repo/issues/42?q=$(date)' + run "$CLAUDE_GH_TASK_WORK" --auto my-feature "$url" + assert_success + # `$(date)` becomes `\$\(date\)`. + assert_stub_called zellij '\$\(date\)' +} + # #144 round-4: task-work mirrors task-reviewer's `set +H;` defense. The # prefix must precede $RADIO_ENV_PREFIX so the env assignments bind to # `claude` (an external command), not the `set` builtin — `VAR=val cmd1; diff --git a/tests/task_reviewer.bats b/tests/task_reviewer.bats index 80fcb74..362c6f2 100644 --- a/tests/task_reviewer.bats +++ b/tests/task_reviewer.bats @@ -370,12 +370,15 @@ teardown() { assert_stub_called zellij "/reviewer https://github.com/owner/repo/pull/42 PROJ-123" } -# #144 round-2 + round-3: SPEC_IDENTIFIER containing `!` must land in the -# assembled command unmangled, and the assembled command must include -# `set +H` to disable bash history expansion in the spawned interactive -# subshell. Without the defense, a Jira / Notion / local id like -# `auth-login!v2` would be silently history-substituted before /reviewer -# ever saw it. +# #144 round-2 + round-3 + round-5: SPEC_IDENTIFIER containing shell +# metacharacters must reach /reviewer un-expanded. The defenses combine: +# - `set +H;` (round-3) disables bash history expansion in the spawned +# `bash -ic` subshell, blocking the `!foo` event-reference class. +# - `printf %q` on SPEC_IDENTIFIER (round-5) shell-escapes the rest of +# the metachar class: `$var`, `$(...)`, backticks, etc. +# Without printf %q, a child shell re-parse of the assembled string would +# expand `$var` inside the double-quoted `/reviewer ...` payload before +# /reviewer ever saw the arg. # # Round-3 follow-up: `set +H;` must appear BEFORE the RADIO_ENV_PREFIX # assignments (TASK_FORCE_ROLE=, ANTHROPIC_MODEL=, etc.), not after. @@ -383,13 +386,37 @@ teardown() { # `set +H` sits between the env prefix and `claude`, the env vars attach # to the `set` builtin and never reach `claude` (radio routing / # model selection / AUTO_SUBMIT all dead). -@test "claude-jira task-reviewer: SPEC_IDENTIFIER with '!' lands unmangled; cmd disables histexpand (#144 round-2)" { +@test "claude-jira task-reviewer: SPEC_IDENTIFIER with '!' lands escaped; cmd disables histexpand (#144 round-2 + round-5)" { run "$TASK_REVIEWER_JIRA" 42 'PROJ-123!v2' assert_success - assert_stub_called zellij "PROJ-123!v2" + # printf %q on bash 3.2 (macOS default) renders `PROJ-123!v2` as + # `PROJ-123\!v2`. The escape is what we want — the child shell sees + # the backslash and forwards the literal `!` to /reviewer. + assert_stub_called zellij 'PROJ-123\!v2' assert_stub_called zellij "set +H;" } +# #144 round-5: SPEC_IDENTIFIER containing `$` must reach /reviewer un-expanded. +# Without printf %q, the child `bash -ic` shell would expand `$HOME` inside +# the double-quoted `/reviewer ...` payload before /reviewer saw the arg. +@test "claude-jira task-reviewer: SPEC_IDENTIFIER with '\$' lands escaped (#144 round-5)" { + run "$TASK_REVIEWER_JIRA" 42 '$HOME-test' + assert_success + # printf %q renders `$HOME-test` as `\$HOME-test`. The escape stops the + # child shell from expanding `$HOME`. + assert_stub_called zellij '\$HOME-test' +} + +# #144 round-5: SPEC_IDENTIFIER containing `$(...)` command-substitution +# syntax must reach /reviewer un-expanded. +@test "claude-jira task-reviewer: SPEC_IDENTIFIER with '\$(...)' lands escaped (#144 round-5)" { + run "$TASK_REVIEWER_JIRA" 42 'PROJ-$(date)' + assert_success + # printf %q renders `PROJ-$(date)` as `PROJ-\$\(date\)`. The escapes + # stop the child shell from executing `date`. + assert_stub_called zellij 'PROJ-\$\(date\)' +} + @test "claude-gh task-reviewer: assembled command disables histexpand (#144 round-2)" { # Same defense applies on the claude-gh path even though SPEC_IDENTIFIER # there is a GitHub issues URL (no `!`) — the 4 dispatcher bodies stay