task-reviewer: tracker-aware spec-issue cross-check (claude-jira/notion/local) (#144)

[martin-conur]

Summary

Fixes #144. task-reviewer's spec-issue cross-check was GitHub-only — both the bash dispatcher (numeric/URL parsing, Closes #N PR-body auto-detect, GH URL synthesis) and the /reviewer slash-command prompt (hardcoded gh issue view <M>). In claude-jira / claude-notion / claude-local, task-reviewer <pr> <spec-id> silently degraded to a diff-only review.

Dispatcher (Part A) — internal-dispatch: 4 dispatcher bodies stay byte-identical (task-reviewer-claude drift group untouched), detect the loadout from the dispatcher's own file path, and branch on $AW_LOADOUT. claude-gh keeps numeric / GitHub-issue-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 slug) and pass it through to /reviewer verbatim. With no 2nd arg, non-gh emits a "PR-body auto-detect is GitHub-only" advisory and proceeds diff-only.

Prompts (Part B) — commands/reviewer.md now legitimately differs per loadout (the previous byte-identical state was accidental — never part of the drift manifest). claude-jira uses mcp__atlassian__getJiraIssue; claude-notion uses mcp__notion__notion-fetch; claude-local reads tasks/<id>-<slug>.md via the Read tool. The shared spine (authority boundaries, radio handoff intents, verdict structure) stays consistent — pinned by two new assertions in tests/drift_check.bats that replaced the old byte-identical check.

Tests (Part C) — tests/task_reviewer.bats extended with per-loadout pass-through (Jira key / Notion URL / local slug), ignore-GH-Closes-in-PR-body diff-only behavior, and the new advisory copy. claude-gh path stays green; a regression test pins the original GH-flavored advisory.

Docs (Part D) — README's task-reviewer section lists each loadout's spec-id shape and the non-gh constraint. CHANGELOG [Unreleased] gets a Fixed entry referencing #144 (lands before #143's v0.3.0 cut).

Test plan

• ./run_tests.sh task_reviewer → 75/75 green (10 new task-reviewer: spec-issue cross-check is GitHub-only — breaks claude-jira / claude-notion / claude-local #144 cases)
• ./run_tests.sh drift_check → 8/8 green (replaces old byte-identical reviewer.md check)
• ./run_tests.sh (full suite) → 753/753 green
• tools/check-drift.sh → 21 groups checked, all pass (task-reviewer-claude drift sentinel still intact)
• Manual smoke in a claude-jira scratch repo: task-reviewer <pr> PROJ-N spawns a reviewer that reads the Jira issue via MCP
• Manual smoke in claude-gh: behavior unchanged (regression)

Upgrading

Re-run task-init <loadout> (claude-jira / claude-notion / claude-local) after pulling so the updated /reviewer prompt lands in .claude/commands/. Without the re-run, the spawned reviewer would still use the old GitHub-only prompt and try gh issue view PROJ-123.

Closes #144

🤖 Generated with Claude Code

[martin-conur]

Spec compliance (vs #144)

PR delivers all four parts of the spec:

• Part A (bash dispatcher) — internal-dispatch shape chosen; AW_LOADOUT detected from ${BASH_SOURCE[0]} path at line 104; parse_issue_number branches on $AW_LOADOUT; PR-body auto-detect gated behind [[ "$AW_LOADOUT" == "claude-gh" ]]; all 4 bodies byte-identical (sentinel preserved). ✓
• Part B (slash-command prompts) — claude-jira/commands/reviewer.md → mcp__atlassian__getJiraIssue; claude-notion/commands/reviewer.md → mcp__notion__notion-fetch; claude-local/commands/reviewer.md → Read tool; claude-gh unchanged. Authority-boundaries spine stays consistent across all 4. ✓
• Part C (tests) — 10 new bats tests covering per-loadout passthrough (Jira key, Notion URL, local slug), ignore-GH-Closes-in-body diff-only behavior, and advisory copy. claude-gh regression test added. Drift check replaced with per-loadout content assertions + shared-spine loop assertions. ✓
• Part D (README + CHANGELOG) — README task-reviewer section updated with per-loadout spec-id shapes and examples; CHANGELOG [Unreleased] gets a Fixed entry referencing task-reviewer: spec-issue cross-check is GitHub-only — breaks claude-jira / claude-notion / claude-local #144. ✓

Manual smoke tests (claude-jira scratch repo + claude-gh regression) are explicitly marked unchecked in the PR — acceptable given they require a live Jira environment.

---

Code-review findings

1. bash -ic history expansion: ! in SPEC_IDENTIFIER silently corrupts the Jira key

File: claude-gh/bin/task-reviewer line 292 (identical in all 4 dispatchers)

build_claude_cmd builds:

echo "claude ${mode_prefix}\"/reviewer $slash_args\""

This string is passed verbatim to aw_launch_tab, which calls:

zellij action new-tab --name "$slug" --cwd "$cwd" -- bash -ic "$cmd"

Bash interactive mode (-i) enables history expansion. If SPEC_IDENTIFIER contains ! — e.g., a local slug like auth-login!v2 or a Jira key mistakenly typed with ! — bash expands it to the last matching history entry before claude ever sees the argument. No error is surfaced; the reviewer proceeds with a garbage spec identifier and either fails the MCP call or opens the wrong task file. Jira keys and canonical Notion URLs don't normally contain !, but the acceptance of any non-empty opaque string (the whole point of this PR) widens the blast radius.

Fix: set histexpand off in the subshell, or wrap SPEC_IDENTIFIER in single-quotes in the generated command string.

2. kiro-gh/bin/task-reviewer not updated — opaque identifiers still rejected

File: kiro-gh/bin/task-reviewer line 145

The four claude-* dispatchers were migrated to SPEC_IDENTIFIER + AW_LOADOUT; kiro-gh/bin/task-reviewer was not. It still declares ISSUE_URL="" (line 145), has the old parse_issue_number (GitHub-numeric/URL only), and lacks any AW_LOADOUT detection. The spec says "kiro variants — out of scope," and kiro-gh is a GitHub-only tracker today, so there is no immediate regression. However, if someone passes an opaque identifier expecting the new behavior (or when kiro-jira/notion/local are added), kiro-gh will reject it with "could not parse issue number" — inconsistent UX with the claude path.

Note: The shared bin/task-reviewer dispatcher correctly resolves symlinks and execs the impl-specific script, so ${BASH_SOURCE[0]} in the claude-* scripts always resolves to the real path. The AW_LOADOUT detection via basename "$(dirname "$(dirname "${BASH_SOURCE[0]}")")" is safe.

3. claude-local/commands/reviewer.md: "glob" is a bare verb — tool unspecified

File: claude-local/commands/reviewer.md line 35

If the arg is a bare slug, glob `tasks/<arg>*.md` to locate the file.

Every other tool call in the prompt is explicit (Read(...), gh pr view, mcp__atlassian__getJiraIssue). "Glob" here is only a verb — it doesn't say to use the Glob tool, find, or ls. Under --permission-mode auto the agent has discretion; if it reaches for a Bash find call and the harness prompts for it, the reviewer could fall through to a diff-only review with no diagnostic message.

Fix: replace with an explicit tool call, e.g.:

Glob("<repo-root>/tasks/<arg>*.md")

4. tests/task_reviewer.bats: claude-notion advisory test missing "diff-only review" assertion

File: tests/task_reviewer.bats line ~565

The claude-jira and claude-local advisory tests both assert --partial "diff-only review". The claude-notion version is missing this assertion. The production code path that prints "Reviewer will proceed with a diff-only review." is unconditional (outside the if/else, same for all non-gh loadouts), so a future regression that accidentally drops that line for notion-only would be invisible to CI.

5. .claude/gh-workflow.md (project-internal) not updated

File: .claude/gh-workflow.md line 117

This file is auto-loaded into every Claude Code session for this repo. It still describes task-reviewer with the old signature:

task-reviewer <pr-url-or-number> [<issue-url-or-number>]

and says "auto-detected from the PR body" without the loadout qualification. For this project (which is a claude-gh project), the behavior is unchanged, so no functional breakage. But the PM agent reading this doc could dispatch a reviewer without the explicit spec-id for a non-gh tracker, believing auto-detect is universal.

---

Verdict: clean-with-nits

The core fix is correct and well-structured. The AW_LOADOUT detection via ${BASH_SOURCE[0]} is safe in practice (the shared dispatcher resolves symlinks before exec'ing the impl-specific script). The drift-check replacement is sound — per-file grep loops correctly catch individual file regressions. Spec compliance is complete on all four parts.

Finding #1 (bash -ic history expansion) is the most likely to surface in real usage and is worth a targeted fix before merge — a one-liner to suppress histexpand in the generated command. Findings #3 and #4 are low-effort cleanup. Findings #2 and #5 can reasonably track in follow-up issues.

🤖 Generated with Claude Code

[martin-conur]

Changes requested — three pre-merge fixes; two findings filed as follow-ups

Reviewer's 5 findings triaged. Three to fix before merge; two to track separately.

Pre-merge

1. bash -ic history expansion on ! in SPEC_IDENTIFIER (reviewer #1)

build_claude_cmd builds a string that lands in zellij action ... -- bash -ic "$cmd". Interactive bash does history expansion on !, so a spec identifier containing ! gets silently mangled before /reviewer ever sees it. The PR widens this blast radius by accepting opaque strings.

Cheapest fix: set +H in the dispatched subshell, OR single-quote SPEC_IDENTIFIER in build_claude_cmd's output. The single-quoting is more local — only changes the one string assembly — but set +H is more bulletproof if other paths grow user-input passthroughs later. Worker's call.

Add a bats test that passes a !-containing identifier and asserts it lands unmangled in the recorded claude stub invocation.

3. claude-local/commands/reviewer.md line 35: "glob" as a bare verb (reviewer #3)

If the arg is a bare slug, glob `tasks/<arg>*.md` to locate the file.

Other tools in the same prompt are explicit (Read(...), gh pr view, mcp__atlassian__getJiraIssue). Replace with an explicit Glob tool call — e.g. Glob("tasks/<arg>*.md") — to keep the prompt style consistent and rule out a fall-through path where the agent reaches for Bash(find ...) and trips a permission prompt under --permission-mode auto.

4. tests/task_reviewer.bats claude-notion advisory test: missing "diff-only review" assertion (reviewer #4)

The claude-jira and claude-local advisory tests both assert --partial "diff-only review". The claude-notion equivalent is missing it. Add the assertion — a future regression that drops the line for notion-only would be invisible to CI.

Follow-ups (filed separately, NOT for this PR)

• kiro-gh/bin/task-reviewer: migrate to SPEC_IDENTIFIER + AW_LOADOUT to match claude-* (post-#144) #146 — kiro-gh/bin/task-reviewer migration to the SPEC_IDENTIFIER + AW_LOADOUT model. (Reviewer kiro notion follows claude jira auto install path, and now allows a seconds parametr for naming worktrees #2 — out-of-spec for task-reviewer: spec-issue cross-check is GitHub-only — breaks claude-jira / claude-notion / claude-local #144, inconsistent UX.)
• .claude/gh-workflow.md: refresh task-reviewer signature + per-loadout spec-id guidance (post-#144) #147 — .claude/gh-workflow.md (this repo's own steering doc) describes the old task-reviewer signature. (Reviewer workflow: base branch tracking, worker PR creation, interactive task-init, bats tests #5 — doc drift.)

Both are P2 and not v0.3.0 blockers. Don't touch them in this PR.

Out of scope for this round

Resist the urge to add other refactors. Push the three fixes above, run the suite, and radio re-review-requested.

[martin-conur]

Re-review: PR #145 (round 2, addressing PM's changes-requested)

Pre-merge fixes from round-1: status

All three items from PM's changes-requested are addressed in this push:

• install script add path automatically, when task-work command is executed it will create a new tab with hash name as suffix, so we can add multiple agents to the same ticket in parallel #1 histexpand / ! — set +H; prepended to build_claude_cmd() output. ✓ (but see Blocker below)
• Surface Notion slug from agents; auto-transition tickets on work start/done #3 bare glob verb in claude-local/commands/reviewer.md — replaced with explicit Glob("tasks/<arg>*.md") and an explicit note not to fall back to Bash(find ...). ✓
• kiro task-work receives trus-all and model parameters #4 claude-notion advisory test missing "diff-only review" assertion — assertion added with an inline comment explaining the intent. ✓

New tests for set +H behavior and !-containing identifiers are present and well-described.

---

Blocker found in round-2 changes

RADIO_ENV_PREFIX vars no longer reach claude — regression introduced by the set +H; fix.

Before this PR the assembled command was:

TASK_FORCE_ROLE=... ANTHROPIC_MODEL=... claude --permission-mode auto "/reviewer ..."

RADIO_ENV_PREFIX vars are prefix assignments to claude (an external command) — they go into claude's environment. ✓

After this PR build_claude_cmd returns set +H; claude ..., so the assembled command is:

TASK_FORCE_ROLE=... ANTHROPIC_MODEL=... set +H; claude --permission-mode auto "/reviewer ..."

The prefix vars are now assignments to set +H (a shell builtin). In bash, variable assignments before a simple command are scoped to that command only. After the ; separator, the current-shell environment is restored and the vars are gone:

$ bash -c 'MYVAR=hello set +H; printf "[%s]\n" "$MYVAR"'
[]    # ← empty: MYVAR never reached printf

So when bash -ic "$cmd" executes, claude runs without TASK_FORCE_ROLE, ZELLIJ_TAB, ANTHROPIC_MODEL, TASK_FORCE_AUTO_SUBMIT, etc. Radio routing, model selection, and auto-submit are all broken.

The tests don't catch this because assert_stub_called zellij verifies the string passed to zellij, not what executes inside the bash -ic subshell.

Fix — move set +H; out of build_claude_cmd and prepend it at the call site, BEFORE ${RADIO_ENV_PREFIX}:

-  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"

And build_claude_cmd returns the old form (without set +H;):

-  echo "set +H; claude ${mode_prefix}\"/reviewer $slash_args\""
+  echo "claude ${mode_prefix}\"/reviewer $slash_args\""

With this ordering: set +H disables histexpand for the subshell, then RADIO_ENV_PREFIX vars are assigned before claude (an external command) and reach it correctly.

Add a bats assertion that the assembled command has set +H BEFORE the first TASK_FORCE_ROLE assignment, not after.

---

Nit (non-blocking)

claude-local advisory test missing "diff-only review" assertion — the claude-notion advisory test's inline comment says "Mirror claude-jira / claude-local: pin the 'diff-only review' trailer" but the claude-local advisory test (~line 1058) only asserts "No spec identifier passed" and "auto-detect is GitHub-only". It's missing assert_output --partial "diff-only review". The production code unconditionally prints that trailer for all non-gh loadouts, so a regression that drops it for local-only would be invisible to CI.

---

Verdict: changes-requested

The core spec-compliance work (Parts A–D) is solid. The round-1 nits are addressed. The blocker is a one-liner — move set +H; from inside build_claude_cmd to the call site, ahead of ${RADIO_ENV_PREFIX}. Once that's in (with a test assertion confirming the ordering), the PR is mergeable.

🤖 Generated with Claude Code

[martin-conur]

Changes requested — round 3

Blocker (regression introduced in round-2)

set +H; placement broke RADIO_ENV_PREFIX env-passing. With the current code, the launched command is:

TASK_FORCE_ROLE=... ANTHROPIC_MODEL=... set +H; claude ...

Bash scopes VAR=value prefix assignments to the next simple command. That command is set +H (a builtin), not claude. After the ;, the env vars are gone. Confirmed with the reviewer's reproduction:

$ bash -c 'MYVAR=hello set +H; printf "[%s]\n" "$MYVAR"'
[]

Result: TASK_FORCE_ROLE, ZELLIJ_TAB, ANTHROPIC_MODEL, TASK_FORCE_AUTO_SUBMIT never reach claude. Radio routing dies; Sonnet override dies; --auto auto-submit dies. The tests didn't catch it because assert_stub_called zellij inspects the string handed to zellij, not what the inner bash -ic subshell actually evaluates.

Fix — move set +H; out of build_claude_cmd and prepend it at the aw_launch_tab call site, before ${RADIO_ENV_PREFIX}. Apply to both task-reviewer and (if the same idiom exists there) task-work:

- 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 "set +H; claude ${mode_prefix}\"/reviewer $slash_args\""
+   echo "claude ${mode_prefix}\"/reviewer $slash_args\""

That ordering: set +H first turns off histexpand for the subshell, then the env prefix vars correctly attach to claude (an external command) and reach its env.

Apply the same fix in all 4 claude- task-reviewer dispatchers* (drift-grouped — fix one, the others should follow via the drift check).

Add a bats test that confirms the ordering: set +H appears in the assembled command string before the first TASK_FORCE_ROLE= assignment. That test would have caught this regression.

Nit (small, take while you're in there)

claude-local advisory test is missing "diff-only review" assertion (tests/task_reviewer.bats ~line 1058). claude-jira and claude-notion already assert this trailer; claude-local doesn't. Same fix shape as the round-1 claude-notion fix:

assert_output --partial "diff-only review"

Out of scope for round 3

Don't touch anything else. Round 3 is: re-order set +H, add the ordering test, add the missing claude-local assertion.

Radio re-review-requested when ready.

[martin-conur]

Round-3 review: PR #145

Pre-merge items from PM's round-3 changes-requested: status

All three explicitly required items are addressed:

• set +H; moved to call site — build_claude_cmd no longer emits it; the aw_launch_tab call is now "set +H; ${RADIO_ENV_PREFIX}$(build_claude_cmd)". Ordering is correct: histexpand disabled first, then env prefix vars, then claude. ✓
• Ordering test added — 'set +H' precedes RADIO_ENV_PREFIX so env vars reach claude uses prefix-substring comparison to verify positioning. Logic is correct: if set +H; is absent the test fails; if it appears after TASK_FORCE_ROLE= it fails. ✓
• claude-local advisory test "diff-only review" assertion added — matching the pattern already in claude-jira and claude-notion. ✓

Round-2 pre-merge items (bare glob verb in claude-local/commands/reviewer.md, claude-notion advisory assertion) remain correctly fixed. ✓

---

New finding from this round

task-work not updated — PM's conditional was met but the fix wasn't applied

PM's round-3 comment said: "Apply to both task-reviewer and (if the same idiom exists there) task-work."

task-work at line 229 uses the identical pattern:

aw_launch_tab "$SLUG" "$WORKTREE_DIR" "${RADIO_ENV_PREFIX}$(build_claude_cmd)" "${AUTO_MODE:-}"

And its build_claude_cmd embeds $GH_URL directly in the command string:

echo "claude ${mode_prefix}\"/worker Implement task: $GH_URL\""

GH_URL is user-provided. A GitHub PR URL won't normally contain !, but the same bash -ic path without set +H is live. The PM's condition ("if the same idiom exists there") is satisfied — same aw_launch_tab pattern, same bash -ic exposure, same env-prefix concern. The fix was not applied.

Fix is a one-liner at task-work's launch site, mirroring task-reviewer's fix. Since task-work is a drift-group member, a single fix propagates to all 4 loadouts via check-drift.

---

Nit (non-blocking)

Ordering test edge case — false pass when TASK_FORCE_ROLE= is absent

The ordering test uses prefix-substring comparison:

local set_pos="${cmd_line%%set +H;*}"
local role_pos="${cmd_line%%TASK_FORCE_ROLE=*}"
[[ ${#set_pos} -lt ${#role_pos} ]] || ...

If TASK_FORCE_ROLE= is somehow absent from cmd_line, ${cmd_line%%TASK_FORCE_ROLE=*} returns the full string unchanged, making ${#role_pos} equal to ${#cmd_line}. If set +H; IS present, ${#set_pos} is shorter, so the test passes — even without env vars in the command. A regression that drops TASK_FORCE_ROLE= from the assembled command would not be caught by this test. This is mitigated by the separate assert_stub_called zellij "TASK_FORCE_ROLE=" tests, so it's not a coverage hole in aggregate.

---

Verdict: clean-with-nits

The three round-3 blockers are correctly implemented. The set +H; placement is right, the explanation comment is thorough, and the tests are well-structured.

The task-work gap is the one remaining actionable item — PM explicitly asked about it and the same idiom is present. Whether to fix in this PR or track as a follow-up is PM's call. The nit on the ordering test is non-blocking.

🤖 Generated with Claude Code

[martin-conur]

Changes requested — round 4

Required: task-work is missing the same fix

My round-3 comment said: "Apply to both task-reviewer and (if the same idiom exists there) task-work." The idiom is in task-work at line 229 — same aw_launch_tab call, same bash -ic exposure, same RADIO_ENV_PREFIX env-prefix ordering concern. build_claude_cmd in task-work embeds $GH_URL (user-provided) directly into the assembled command string. GitHub URLs don't normally contain !, but the unprotected path is live.

Fix is a one-line mirror of the task-reviewer change — prepend set +H; at the aw_launch_tab call site, before ${RADIO_ENV_PREFIX}. Same drift group as the task-reviewer copies, so a single fix propagates across all 4 claude-* loadouts (and the kiro-* dispatchers — verify against the task-work drift manifest entries).

Extend the ordering-test pattern from the round-3 task-reviewer test into task-work's equivalent test (assert set +H; precedes TASK_FORCE_ROLE= in the assembled command).

Optional: ordering-test edge case (nit, non-blocking)

The new 'set +H' precedes RADIO_ENV_PREFIX test would silently false-pass if TASK_FORCE_ROLE= were somehow absent from the assembled command (the substring-prefix comparison degenerates). The aggregate coverage is fine (separate assert_stub_called zellij "TASK_FORCE_ROLE=" tests pin presence), so this is genuinely non-blocking — but if it's a one-liner to harden (e.g., explicit assert_output --partial "TASK_FORCE_ROLE=" BEFORE the position check), worth taking while you're in the file.

Out of scope for round 4

Nothing else. Just the task-work mirror + ordering test. Radio re-review-requested when ready.

[martin-conur]

Round-4 review: PR #145

Round-4 pre-merge items: status

All three items from PM's round-4 changes-requested are addressed:

• task-work set +H; fix applied — aw_launch_tab call in all 7 affected dispatchers updated: claude-gh, claude-jira, claude-local, claude-notion, kiro-gh, kiro-local, kiro-notion. Placement is correct (set +H; precedes ${RADIO_ENV_PREFIX} in every case). ✓
• Ordering test added in tests/claude_gh_task_work.bats — mirrors the round-3 task-reviewer test; includes the TASK_FORCE_ROLE= presence guard so the prefix-length comparison can't false-pass if the env prefix is missing. ✓
• claude-local advisory "diff-only review" assertion — added, matching claude-jira and claude-notion. ✓

---

New findings from round-4

1. SPEC_IDENTIFIER not protected against $var / $() expansion in the child shell (non-gh loadouts)

build_claude_cmd builds:

echo "claude ${mode_prefix}\"/reviewer $slash_args\""

where $slash_args contains $SPEC_IDENTIFIER. The resulting string is passed to bash -ic, which re-parses it as shell code. Inside the double-quoted /reviewer ... argument, any $var or $() in SPEC_IDENTIFIER is expanded by the child shell before /reviewer ever receives it.

set +H; (the round-3 fix) only disables ! history expansion — it has no effect on $var or $() expansion. The PR comment at line ~314 acknowledges this implicitly by only naming ! as the defended character class.

For canonical Jira keys (PROJ-123) and well-formed Notion URLs, $ is not expected, so practical risk is low. But the spec explicitly widens the accepted input to "any non-empty opaque string," and a Notion URL or local slug containing a $ (e.g. copied from a URL with %24 decoded, or a local path with $REPO) would be silently mangled or worse.

The RADIO_ENV_PREFIX values already use printf %q for exactly this reason (lines 301–304). Applying the same treatment to SPEC_IDENTIFIER would be a one-liner fix and make the escaping scheme consistent:

-  [[ -n "$SPEC_IDENTIFIER" ]] && slash_args+=" $SPEC_IDENTIFIER"
+  [[ -n "$SPEC_IDENTIFIER" ]] && slash_args+=" $(printf %q "$SPEC_IDENTIFIER")"

2. Dead code: "Error: spec identifier cannot be empty" is unreachable

parse_issue_number for non-gh loadouts only returns 1 when $input is empty ([[ -n "$input" ]] || return 1). The call site guards on [[ -n "$ISSUE_INPUT" ]] before invoking parse_issue_number "$ISSUE_INPUT", guaranteeing $input is always non-empty when the function is called. The else branch printing "Error: spec identifier cannot be empty" can never fire — and if it somehow did, the message would be factually wrong (the user did pass an argument). Non-blocking nit.

3. kiro-gh/bin/task-reviewer still missing set +H; (pre-existing, out-of-scope)

kiro-gh/bin/task-reviewer has no set +H; in its aw_launch_tab call. This was already identified in round-3 as a pre-existing gap (filed as follow-up #146) and is not a regression introduced by this PR. Noting it here only for completeness.

---

Verdict: clean-with-nits

The round-4 work is correct and complete. The set +H; placement is right across all 7 task-work dispatchers, the ordering test is solid (the TASK_FORCE_ROLE= presence guard addresses the round-3 nit), and the claude-local advisory test is now consistent with the other loadouts.

Finding #1 ($var/$() in SPEC_IDENTIFIER) is real and worth a pre-merge fix — it's a one-liner, and the inconsistency with printf %q already used for RADIO_ENV_PREFIX makes it easy to spot. Finding #2 is a nit. Finding #3 is a known follow-up. PM decides.

🤖 Generated with Claude Code

[martin-conur]

Changes requested — round 5 (closes the shell-quoting class)

Required: printf %q on user-input strings in both task-reviewer and task-work

set +H (rounds 3 + 4) only blocks ! history expansion. The child bash -ic shell still does $var and $() expansion inside double-quoted strings — so SPEC_IDENTIFIER and GH_URL are still vulnerable.

The codebase already uses printf %q for exactly this reason on RADIO_ENV_PREFIX values (see task-reviewer:301-304 and task-work:215-217). The inconsistency is the bug: env vars get quoted, user-input args don't. Closing this completes the round-1 ask in its proper form.

Apply to task-reviewer (build_claude_cmd):

-  [[ -n "$SPEC_IDENTIFIER" ]] && slash_args+=" $SPEC_IDENTIFIER"
+  [[ -n "$SPEC_IDENTIFIER" ]] && slash_args+=" $(printf %q "$SPEC_IDENTIFIER")"

Apply to task-work (build_claude_cmd, same idiom — GH_URL is user-provided and gets embedded into the claude "/worker Implement task: $GH_URL" string):

-  if [[ -n "$GH_URL" ]]; then
-    echo "claude ${mode_prefix}\"/worker Implement task: $GH_URL\""
+  if [[ -n "$GH_URL" ]]; then
+    local q_gh_url
+    q_gh_url=$(printf %q "$GH_URL")
+    echo "claude ${mode_prefix}\"/worker Implement task: $q_gh_url\""
   else

Both are drift-grouped, so a single fix in each propagates to all loadout copies (4 claude-* for task-reviewer; 7 for task-work).

Add bats tests that pass a $-containing identifier / URL and assert it lands UN-expanded in the recorded claude stub args. Use a value like '$HOME-test' or 'PROJ-$(date)' so a regression would visibly mangle the assertion.

Optional take-while-you're-in-there

Dead-code nit (reviewer finding #2): parse_issue_number's else branch printing "Error: spec identifier cannot be empty" is unreachable — the call site already guards on [[ -n "$ISSUE_INPUT" ]]. Either delete the branch or convert it to an assert-style internal sanity comment. Genuinely non-blocking; skip if it expands the patch scope.

Out of scope for round 5

• kiro-gh task-reviewer's missing set +H — already tracked as kiro-gh/bin/task-reviewer: migrate to SPEC_IDENTIFIER + AW_LOADOUT to match claude-* (post-#144) #146 (follow-up).
• Anything else.

Why this is the last round

After printf %q on SPEC_IDENTIFIER + GH_URL, all user-input entering bash -ic is fully shell-escaped. The remaining strings in build_claude_cmd are static literals or ${mode_prefix} (also literal). That closes the shell-quoting class — there shouldn't be a round 6 on this PR.

Radio re-review-requested when ready.

[martin-conur]

Round-5 review: PR #145

Round-5 pre-merge items: status

All three items from PM's round-5 changes-requested are correctly addressed:

• printf %q on SPEC_IDENTIFIER in task-reviewer — applied in build_claude_cmd for all 4 claude-* dispatchers. The escape runs at command-assembly time in the dispatcher (not in the child subshell), so the already-escaped value is embedded before bash -ic re-parses the string. ✓
• printf %q on user-supplied inputs in task-work — applied across all 7 affected dispatchers: GH_URL in claude-gh and kiro-gh, JIRA_REF in claude-jira, TASK_FILE in claude-local and kiro-local, NOTION_URL in claude-notion and kiro-notion. The claude-* variants use local q_* intermediates; kiro-* use inline command substitution — both are equivalent for this use case. ✓
• New tests for \$ and \$(...) in SPEC_IDENTIFIER and GH_URL — three new bats tests for \$HOME-test and PROJ-\$(date) in task-reviewer (jira loadout), plus two for GH_URL in task-work. Tests verify the escaped forms land in the zellij stub invocation. ✓

The dead-code nit from round-4 (parse_issue_number's unreachable "Error: spec identifier cannot be empty" branch) was listed as optional and was not removed — acceptable.

---

New finding from round-5 analysis

.info file: ISSUE_NUMBER written unquoted; task-done sources it — new regression surface for non-gh loadouts

task-reviewer writes the .info file using:

printf 'ISSUE_NUMBER=%s\n' "$ISSUE_NUMBER"

which produces an unquoted assignment line:

ISSUE_NUMBER=PROJ-$(date)

task-done reads it via source "$INFO_FILE" (line 54 of claude-gh/bin/task-done). When the source runs, bash re-parses the value — $(date) executes, $VAR expands, backticks execute.

Before this PR, ISSUE_NUMBER was always numeric for every loadout (non-gh loadouts couldn't reach a set ISSUE_NUMBER because parse_issue_number rejected non-integers). This PR opens the path for non-gh ISSUE_NUMBER values to hold arbitrary opaque strings. The .info write/read path was not updated to handle this.

Practical risk is low — real Jira keys (PROJ-123), well-formed Notion URLs, and local slugs (kebab-case filenames) contain no $ or backtick, so the sourcing is safe in all real-world cases. The issue only triggers for explicitly crafted inputs and would be a self-inflicted local operation. It is worth tracking but does not block the merge.

Straightforward fix (can be a follow-up): in task-done, replace source "$INFO_FILE" with field-specific awk -F= '/^ISSUE_NUMBER=/{print $2}' reads, matching how TAB_ID is already read non-source at lines 116/121 of task-done. Or quote the value on write: printf 'ISSUE_NUMBER=%q\n' "$ISSUE_NUMBER".

---

Minor nit (informational)

printf %q inside double-quoted bash -ic strings: backslash artifacts for paren-containing inputs

When SPEC_IDENTIFIER contains ( or ) (e.g., PROJ-$(date)), printf %q produces PROJ-\$\(date\). Embedded inside the double-quoted /reviewer ... argument in the child bash -ic context:

• \$ → $ (bash consumes the escape — correct)
• \( → \( (bash does NOT consume the backslash before ( inside double quotes — it's not a recognized dquote escape)

So claude receives PROJ-$\(date\) rather than the original PROJ-$(date). Injection is fully prevented (no command executes), but the literal value seen by the reviewer prompt has extra backslashes. For real-world inputs (Jira keys are [A-Z]+-[0-9]+, URLs use %-encoding for special chars, local slugs are kebab-case), this never fires. Informational only.

---

Verdict: clean-with-nits

Round-5 requirements are correctly implemented. The printf %q escaping correctly defends the bash -ic launch path against $var/$(...) expansion, complementing the earlier set +H defense for ! history expansion. The shell-quoting class is now closed for the bash -ic path on all 7 task-work dispatchers and all 4 task-reviewer dispatchers.

The .info file sourcing regression is real but has zero practical impact for real-world identifier formats. PM can decide whether to address it in this PR or file a follow-up.

🤖 Generated with Claude Code