diff --git a/CHANGELOG.md b/CHANGELOG.md index 092cfea..7b8df78 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), ### Fixed - **`task-done --remove-worktree` now force-deletes reviewer branches so `task-reviewer` re-dispatch isn't blocked.** The cleanup's `git branch -d` is the safe-delete variant that requires the branch to be fully merged into HEAD — correct for worker branches (don't drop unmerged work), but wrong for reviewer worktrees, whose `task/review-pr` branch is forked from the PR's head ref and is never going to land in main (the PR is). Result: every reviewer cleanup left `task/review-pr` behind as an orphan, and the next `task-reviewer ` refused to dispatch via its per-PR guard with `Error: a review worktree or branch already exists`. The user had to manually `git branch -D task/review-pr` between every re-review round. `task-done` now branches on the `PR_NUMBER=` marker that `task-reviewer` writes into the worktree's `.info` file: when present it does `git branch -D` (scaffold-only, safe to force); when absent worker behavior is unchanged. Lands across the `task-done-std` and `task-done-local` drift groups (7 binaries). (#148) +- **Radio session corruption on long-running workers — `LOADOUT` / `AGENT` / `TAB_ID` no longer lost mid-life.** Three-bug cluster surfaced by `--auto` workers running for hours: Claude Code's `SessionEnd` hook fires repeatedly with non-`clear|resume` reasons (the diagnostic added in #150 caught bursts of 144 invocations in ~25s, payload literally a single `y` character, `reason=` — phantom calls upstream of the documented hook contract), each one wiping `~/.task-force/radio/sessions/.info`. The next `busy`/`ready` hook re-seeded the file with `LOADOUT=unknown`, empty `TAB_ID=`, and `AGENT=claude` (silently flipping kiro-* workers), which then broke `radio send`'s tab-id-based wake-up — PM pings landed in the mailbox silently and never woke the worker. Three concrete fixes: **(#140 Fix B)** `_zellij_tab_id_by_name` now matches the emoji-prefixed visible name (`⏸️ ` / `▶️ ` / `❓︎ `) so the re-seed's tab-id lookup survives `_rename_tab`'s repaints; **(#140 Fix C / #150 / #151)** `cmd_register` writes a `.loadout` (and now `.agent`) sidecar atomically BEFORE the `.info` write (so a kill mid-call leaves a recoverable state), `_ensure_session_file` reads from those sidecars on re-seed (TOCTOU-safe via `cat || true`), and `cmd_unregister`'s skip-list now also short-circuits on empty `reason` — treating the cascade pattern the same as `clear` / `resume`. Real-exit reasons (`logout` / `prompt_input_exit` / `other`) still flow through and clean up normally. Net result: a worker that previously degraded after 4–8 hours (or sooner under heavy subagent use) now keeps its full identity through the cascade, and PM `radio send` keeps waking it via the original `TAB_ID`. (#140, #150, #151) - **`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 diff --git a/claude-gh/bin/radio b/claude-gh/bin/radio index a091d7b..5ae3e6b 100755 --- a/claude-gh/bin/radio +++ b/claude-gh/bin/radio @@ -167,6 +167,17 @@ _loadout_sidecar() { printf '%s/%s.loadout' "$SESSIONS_DIR" "$1" } +# Agent sidecar (#151). Same problem as LOADOUT: $TASK_FORCE_AGENT isn't +# exported into Claude Code's hook subshells, so _ensure_session_file's +# previous fallback to "${TASK_FORCE_AGENT:-claude}" silently wrote +# AGENT=claude for kiro-* workers on every re-seed. Parallel sidecar, parallel +# write/read/sweep semantics — no current runtime consumer, but a kiro worker +# whose re-seeded session reports AGENT=claude is a footgun if anyone wires +# AGENT-conditional behaviour later. +_agent_sidecar() { + printf '%s/%s.agent' "$SESSIONS_DIR" "$1" +} + _inbox_dir() { printf '%s/%s/inbox' "$MAILBOX_DIR" "$1"; } _processed_dir(){ printf '%s/%s/processed' "$MAILBOX_DIR" "$1"; } @@ -216,13 +227,26 @@ _ensure_session_file() { loadout=$(cat "$sidecar" 2>/dev/null || true) [[ -n "$loadout" ]] || loadout="${TASK_FORCE_LOADOUT:-unknown}" + # AGENT recovery (#151): same hook-subshell unset story as LOADOUT — the + # cascade wipes .info, and the next re-seed used to default to "claude" + # regardless of whether this was a kiro-* worker. Read the agent sidecar + # cmd_register persisted; fall back to the env var only for test paths that + # exercise _ensure_session_file directly. Same TOCTOU-safe `cat … || true` + # pattern as the loadout read above for the same reason (concurrent + # unregister race window). + local agent="" + local agent_sidecar + agent_sidecar=$(_agent_sidecar "$TASK_FORCE_ROLE") + agent=$(cat "$agent_sidecar" 2>/dev/null || true) + [[ -n "$agent" ]] || agent="${TASK_FORCE_AGENT:-claude}" + local repo repo=$(git rev-parse --show-toplevel 2>/dev/null || echo "") { printf 'ROLE=%s\nTAB=%s\nTAB_ID=%s\nREPO=%s\nSTATE=idle\nLAST_HEARTBEAT=%s\nAGENT=%s\nLOADOUT=%s\n' \ "$TASK_FORCE_ROLE" "$ZELLIJ_TAB" "$tab_id" "$repo" "$(_now)" \ - "${TASK_FORCE_AGENT:-claude}" "$loadout" + "$agent" "$loadout" # Preserve the --auto opt-in across self-heal so cmd_send keeps using CR # for wake-up after an intra-session re-seed (#128). Use `if` rather than # `&&` so the brace group exits 0 when the flag is absent — otherwise @@ -234,7 +258,7 @@ _ensure_session_file() { } | _atomic_write "$f" mkdir -p "$(_inbox_dir "$TASK_FORCE_ROLE")" "$(_processed_dir "$TASK_FORCE_ROLE")" - _log "ensure_session: re-seeded $TASK_FORCE_ROLE (file was missing) tab_id=$tab_id loadout=$loadout" + _log "ensure_session: re-seeded $TASK_FORCE_ROLE (file was missing) tab_id=$tab_id loadout=$loadout agent=$agent" } # Update STATE=… and LAST_HEARTBEAT=… on the current session file. @@ -385,29 +409,33 @@ cmd_register() { fi fi - # Persist the loadout sidecar BEFORE the session file (#150 round-2 review: - # Finding 1). If the process is killed (OOM, task-done --force, host - # shutdown) between these two _atomic_write calls, the order matters: + # Persist the loadout and agent sidecars BEFORE the session file (#150 + # round-2 review: Finding 1; extended for AGENT in #151). If the process is + # killed (OOM, task-done --force, host shutdown) between these writes, the + # order matters: # # sidecar-first (this order): - # pre-kill state: .loadout=claude-gh, .info absent - # re-seed reads: .loadout → LOADOUT=claude-gh recovered ✓ + # pre-kill state: .loadout=claude-gh, .agent=claude, .info absent + # re-seed reads: sidecars → LOADOUT/AGENT recovered ✓ # # info-first (the OLD order — pre-#150-r3): - # pre-kill state: .info has LOADOUT=claude-gh, .loadout absent - # unregister cascade deletes .info; .loadout never existed + # pre-kill state: .info has LOADOUT/AGENT, sidecars absent + # unregister cascade deletes .info; sidecars never existed # re-seed reads: no sidecar, hook subshell has no $TASK_FORCE_LOADOUT - # → LOADOUT=unknown ✗ + # / $TASK_FORCE_AGENT → LOADOUT=unknown / AGENT=claude ✗ # # The window is microseconds wide, but the cascade in #140 makes it # statistically reachable. Reversing the order eliminates the failure mode - # entirely. Skipping the sidecar on empty $loadout keeps the sidecar's mere + # entirely. Skipping the loadout sidecar on empty $loadout keeps its mere # existence equivalent to "we have a real loadout to restore" — otherwise # a re-seed could read an empty sidecar and clobber the env-var fallback - # that test paths still rely on. + # that test paths still rely on. The agent sidecar is always written + # because --agent is required (cmd_register exits 2 above if absent), so + # $agent is always a real value here. if [[ -n "$loadout" ]]; then printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" fi + printf '%s' "$agent" | _atomic_write "$(_agent_sidecar "$role")" { printf 'ROLE=%s\nTAB=%s\nTAB_ID=%s\nREPO=%s\nSTATE=idle\nLAST_HEARTBEAT=%s\nAGENT=%s\nLOADOUT=%s\n' \ @@ -461,6 +489,22 @@ cmd_unregister() { _log "unregister: skipping (reason=$reason — intra-session event) role=$TASK_FORCE_ROLE" return 0 ;; + "") + # Cascade pattern (#151): payload is present but contains no + # parseable `.reason` field. The PR-#150 diagnostic showed bursts of + # 144 unregister calls in ~25s with payload literally "y" and + # reason= — phantom invocations from somewhere upstream of + # Claude Code's documented SessionEnd hook (which always includes + # reason ∈ {clear, resume, logout, prompt_input_exit, other}). + # Treat these the same as clear/resume: known non-real-exit events + # that must not wipe the session file or the sidecars. Real exits + # still flow through the logout/prompt_input_exit/other branches + # below and clean up normally; manual invocations (no stdin) also + # proceed because `[[ ! -t 0 ]]` is false then. + payload_oneline=$(printf '%s' "$payload" | tr '\n' ' ' | tr -s ' ') + _log "unregister: skipping (reason= — cascade, no real-exit signal) role=$TASK_FORCE_ROLE payload=$payload_oneline" + return 0 + ;; esac fi # Instrumentation for #140: when the hook fires with a payload that @@ -468,7 +512,10 @@ cmd_unregister() { # what reason (or non-reason) is driving the unregister cascade. Empirically # observed as bursts of 20+ unregister calls in ~30s on long-running # worker sessions; the root cause needs the payload to investigate. Log - # is compacted to one line via tr so it doesn't fragment radio.log. + # is compacted to one line via tr so it doesn't fragment radio.log. Post- + # #151, an empty reason is already short-circuited above, so this line + # only fires for non-empty named reasons (logout, prompt_input_exit, + # other, or anything new Claude Code adds in the future). if [[ -n "$payload" ]]; then payload_oneline=$(printf '%s' "$payload" | tr '\n' ' ' | tr -s ' ') _log "unregister: proceeding (reason=${reason:-}) role=$TASK_FORCE_ROLE payload=$payload_oneline" @@ -478,10 +525,11 @@ cmd_unregister() { local f f=$(_session_file "$TASK_FORCE_ROLE") rm -f "$f" - # Sweep the loadout sidecar too — leaving it behind would let a future - # _ensure_session_file resurrect a stale loadout for a role whose tab has - # really exited and been re-used (#140). + # Sweep the loadout + agent sidecars too — leaving them behind would let a + # future _ensure_session_file resurrect a stale loadout / agent for a role + # whose tab has really exited and been re-used (#140, #151). rm -f "$(_loadout_sidecar "$TASK_FORCE_ROLE")" + rm -f "$(_agent_sidecar "$TASK_FORCE_ROLE")" _log "unregister role=$TASK_FORCE_ROLE" } diff --git a/claude-gh/bin/task-done b/claude-gh/bin/task-done index b44dfc6..c627525 100755 --- a/claude-gh/bin/task-done +++ b/claude-gh/bin/task-done @@ -131,8 +131,14 @@ fi # `|| true` is a safety net: $TASK_FORCE_ROLE should be set inside a worker # tab, but if it isn't (e.g., task-done invoked from an unexpected shell) # the no-op must not abort cleanup. `radio unregister` itself already -# silently exits when $TASK_FORCE_ROLE is unset (#93). -radio unregister 2>/dev/null || true +# silently exits when $TASK_FORCE_ROLE is unset (#93). Redirect stdin from +# /dev/null so `radio unregister` doesn't inherit task-done's stdin — under +# `--force` the calling shell may have piped `y` (or any other content) +# to confirm something else, and post-#151 a non-empty stdin without a +# parseable JSON reason triggers the cascade-skip path. That's correct +# behaviour for SessionEnd hook invocations but wrong for explicit +# task-done cleanup, which must always proceed with the unregister. +radio unregister /dev/null || true # Move out of worktree before removing it cd "$MAIN_WORKTREE" diff --git a/claude-jira/bin/radio b/claude-jira/bin/radio index a091d7b..5ae3e6b 100755 --- a/claude-jira/bin/radio +++ b/claude-jira/bin/radio @@ -167,6 +167,17 @@ _loadout_sidecar() { printf '%s/%s.loadout' "$SESSIONS_DIR" "$1" } +# Agent sidecar (#151). Same problem as LOADOUT: $TASK_FORCE_AGENT isn't +# exported into Claude Code's hook subshells, so _ensure_session_file's +# previous fallback to "${TASK_FORCE_AGENT:-claude}" silently wrote +# AGENT=claude for kiro-* workers on every re-seed. Parallel sidecar, parallel +# write/read/sweep semantics — no current runtime consumer, but a kiro worker +# whose re-seeded session reports AGENT=claude is a footgun if anyone wires +# AGENT-conditional behaviour later. +_agent_sidecar() { + printf '%s/%s.agent' "$SESSIONS_DIR" "$1" +} + _inbox_dir() { printf '%s/%s/inbox' "$MAILBOX_DIR" "$1"; } _processed_dir(){ printf '%s/%s/processed' "$MAILBOX_DIR" "$1"; } @@ -216,13 +227,26 @@ _ensure_session_file() { loadout=$(cat "$sidecar" 2>/dev/null || true) [[ -n "$loadout" ]] || loadout="${TASK_FORCE_LOADOUT:-unknown}" + # AGENT recovery (#151): same hook-subshell unset story as LOADOUT — the + # cascade wipes .info, and the next re-seed used to default to "claude" + # regardless of whether this was a kiro-* worker. Read the agent sidecar + # cmd_register persisted; fall back to the env var only for test paths that + # exercise _ensure_session_file directly. Same TOCTOU-safe `cat … || true` + # pattern as the loadout read above for the same reason (concurrent + # unregister race window). + local agent="" + local agent_sidecar + agent_sidecar=$(_agent_sidecar "$TASK_FORCE_ROLE") + agent=$(cat "$agent_sidecar" 2>/dev/null || true) + [[ -n "$agent" ]] || agent="${TASK_FORCE_AGENT:-claude}" + local repo repo=$(git rev-parse --show-toplevel 2>/dev/null || echo "") { printf 'ROLE=%s\nTAB=%s\nTAB_ID=%s\nREPO=%s\nSTATE=idle\nLAST_HEARTBEAT=%s\nAGENT=%s\nLOADOUT=%s\n' \ "$TASK_FORCE_ROLE" "$ZELLIJ_TAB" "$tab_id" "$repo" "$(_now)" \ - "${TASK_FORCE_AGENT:-claude}" "$loadout" + "$agent" "$loadout" # Preserve the --auto opt-in across self-heal so cmd_send keeps using CR # for wake-up after an intra-session re-seed (#128). Use `if` rather than # `&&` so the brace group exits 0 when the flag is absent — otherwise @@ -234,7 +258,7 @@ _ensure_session_file() { } | _atomic_write "$f" mkdir -p "$(_inbox_dir "$TASK_FORCE_ROLE")" "$(_processed_dir "$TASK_FORCE_ROLE")" - _log "ensure_session: re-seeded $TASK_FORCE_ROLE (file was missing) tab_id=$tab_id loadout=$loadout" + _log "ensure_session: re-seeded $TASK_FORCE_ROLE (file was missing) tab_id=$tab_id loadout=$loadout agent=$agent" } # Update STATE=… and LAST_HEARTBEAT=… on the current session file. @@ -385,29 +409,33 @@ cmd_register() { fi fi - # Persist the loadout sidecar BEFORE the session file (#150 round-2 review: - # Finding 1). If the process is killed (OOM, task-done --force, host - # shutdown) between these two _atomic_write calls, the order matters: + # Persist the loadout and agent sidecars BEFORE the session file (#150 + # round-2 review: Finding 1; extended for AGENT in #151). If the process is + # killed (OOM, task-done --force, host shutdown) between these writes, the + # order matters: # # sidecar-first (this order): - # pre-kill state: .loadout=claude-gh, .info absent - # re-seed reads: .loadout → LOADOUT=claude-gh recovered ✓ + # pre-kill state: .loadout=claude-gh, .agent=claude, .info absent + # re-seed reads: sidecars → LOADOUT/AGENT recovered ✓ # # info-first (the OLD order — pre-#150-r3): - # pre-kill state: .info has LOADOUT=claude-gh, .loadout absent - # unregister cascade deletes .info; .loadout never existed + # pre-kill state: .info has LOADOUT/AGENT, sidecars absent + # unregister cascade deletes .info; sidecars never existed # re-seed reads: no sidecar, hook subshell has no $TASK_FORCE_LOADOUT - # → LOADOUT=unknown ✗ + # / $TASK_FORCE_AGENT → LOADOUT=unknown / AGENT=claude ✗ # # The window is microseconds wide, but the cascade in #140 makes it # statistically reachable. Reversing the order eliminates the failure mode - # entirely. Skipping the sidecar on empty $loadout keeps the sidecar's mere + # entirely. Skipping the loadout sidecar on empty $loadout keeps its mere # existence equivalent to "we have a real loadout to restore" — otherwise # a re-seed could read an empty sidecar and clobber the env-var fallback - # that test paths still rely on. + # that test paths still rely on. The agent sidecar is always written + # because --agent is required (cmd_register exits 2 above if absent), so + # $agent is always a real value here. if [[ -n "$loadout" ]]; then printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" fi + printf '%s' "$agent" | _atomic_write "$(_agent_sidecar "$role")" { printf 'ROLE=%s\nTAB=%s\nTAB_ID=%s\nREPO=%s\nSTATE=idle\nLAST_HEARTBEAT=%s\nAGENT=%s\nLOADOUT=%s\n' \ @@ -461,6 +489,22 @@ cmd_unregister() { _log "unregister: skipping (reason=$reason — intra-session event) role=$TASK_FORCE_ROLE" return 0 ;; + "") + # Cascade pattern (#151): payload is present but contains no + # parseable `.reason` field. The PR-#150 diagnostic showed bursts of + # 144 unregister calls in ~25s with payload literally "y" and + # reason= — phantom invocations from somewhere upstream of + # Claude Code's documented SessionEnd hook (which always includes + # reason ∈ {clear, resume, logout, prompt_input_exit, other}). + # Treat these the same as clear/resume: known non-real-exit events + # that must not wipe the session file or the sidecars. Real exits + # still flow through the logout/prompt_input_exit/other branches + # below and clean up normally; manual invocations (no stdin) also + # proceed because `[[ ! -t 0 ]]` is false then. + payload_oneline=$(printf '%s' "$payload" | tr '\n' ' ' | tr -s ' ') + _log "unregister: skipping (reason= — cascade, no real-exit signal) role=$TASK_FORCE_ROLE payload=$payload_oneline" + return 0 + ;; esac fi # Instrumentation for #140: when the hook fires with a payload that @@ -468,7 +512,10 @@ cmd_unregister() { # what reason (or non-reason) is driving the unregister cascade. Empirically # observed as bursts of 20+ unregister calls in ~30s on long-running # worker sessions; the root cause needs the payload to investigate. Log - # is compacted to one line via tr so it doesn't fragment radio.log. + # is compacted to one line via tr so it doesn't fragment radio.log. Post- + # #151, an empty reason is already short-circuited above, so this line + # only fires for non-empty named reasons (logout, prompt_input_exit, + # other, or anything new Claude Code adds in the future). if [[ -n "$payload" ]]; then payload_oneline=$(printf '%s' "$payload" | tr '\n' ' ' | tr -s ' ') _log "unregister: proceeding (reason=${reason:-}) role=$TASK_FORCE_ROLE payload=$payload_oneline" @@ -478,10 +525,11 @@ cmd_unregister() { local f f=$(_session_file "$TASK_FORCE_ROLE") rm -f "$f" - # Sweep the loadout sidecar too — leaving it behind would let a future - # _ensure_session_file resurrect a stale loadout for a role whose tab has - # really exited and been re-used (#140). + # Sweep the loadout + agent sidecars too — leaving them behind would let a + # future _ensure_session_file resurrect a stale loadout / agent for a role + # whose tab has really exited and been re-used (#140, #151). rm -f "$(_loadout_sidecar "$TASK_FORCE_ROLE")" + rm -f "$(_agent_sidecar "$TASK_FORCE_ROLE")" _log "unregister role=$TASK_FORCE_ROLE" } diff --git a/claude-jira/bin/task-done b/claude-jira/bin/task-done index 17af7c1..c5574a9 100755 --- a/claude-jira/bin/task-done +++ b/claude-jira/bin/task-done @@ -137,8 +137,14 @@ fi # `|| true` is a safety net: $TASK_FORCE_ROLE should be set inside a worker # tab, but if it isn't (e.g., task-done invoked from an unexpected shell) # the no-op must not abort cleanup. `radio unregister` itself already -# silently exits when $TASK_FORCE_ROLE is unset (#93). -radio unregister 2>/dev/null || true +# silently exits when $TASK_FORCE_ROLE is unset (#93). Redirect stdin from +# /dev/null so `radio unregister` doesn't inherit task-done's stdin — under +# `--force` the calling shell may have piped `y` (or any other content) +# to confirm something else, and post-#151 a non-empty stdin without a +# parseable JSON reason triggers the cascade-skip path. That's correct +# behaviour for SessionEnd hook invocations but wrong for explicit +# task-done cleanup, which must always proceed with the unregister. +radio unregister /dev/null || true # Move out of worktree before removing it cd "$MAIN_WORKTREE" diff --git a/claude-local/bin/radio b/claude-local/bin/radio index a091d7b..5ae3e6b 100755 --- a/claude-local/bin/radio +++ b/claude-local/bin/radio @@ -167,6 +167,17 @@ _loadout_sidecar() { printf '%s/%s.loadout' "$SESSIONS_DIR" "$1" } +# Agent sidecar (#151). Same problem as LOADOUT: $TASK_FORCE_AGENT isn't +# exported into Claude Code's hook subshells, so _ensure_session_file's +# previous fallback to "${TASK_FORCE_AGENT:-claude}" silently wrote +# AGENT=claude for kiro-* workers on every re-seed. Parallel sidecar, parallel +# write/read/sweep semantics — no current runtime consumer, but a kiro worker +# whose re-seeded session reports AGENT=claude is a footgun if anyone wires +# AGENT-conditional behaviour later. +_agent_sidecar() { + printf '%s/%s.agent' "$SESSIONS_DIR" "$1" +} + _inbox_dir() { printf '%s/%s/inbox' "$MAILBOX_DIR" "$1"; } _processed_dir(){ printf '%s/%s/processed' "$MAILBOX_DIR" "$1"; } @@ -216,13 +227,26 @@ _ensure_session_file() { loadout=$(cat "$sidecar" 2>/dev/null || true) [[ -n "$loadout" ]] || loadout="${TASK_FORCE_LOADOUT:-unknown}" + # AGENT recovery (#151): same hook-subshell unset story as LOADOUT — the + # cascade wipes .info, and the next re-seed used to default to "claude" + # regardless of whether this was a kiro-* worker. Read the agent sidecar + # cmd_register persisted; fall back to the env var only for test paths that + # exercise _ensure_session_file directly. Same TOCTOU-safe `cat … || true` + # pattern as the loadout read above for the same reason (concurrent + # unregister race window). + local agent="" + local agent_sidecar + agent_sidecar=$(_agent_sidecar "$TASK_FORCE_ROLE") + agent=$(cat "$agent_sidecar" 2>/dev/null || true) + [[ -n "$agent" ]] || agent="${TASK_FORCE_AGENT:-claude}" + local repo repo=$(git rev-parse --show-toplevel 2>/dev/null || echo "") { printf 'ROLE=%s\nTAB=%s\nTAB_ID=%s\nREPO=%s\nSTATE=idle\nLAST_HEARTBEAT=%s\nAGENT=%s\nLOADOUT=%s\n' \ "$TASK_FORCE_ROLE" "$ZELLIJ_TAB" "$tab_id" "$repo" "$(_now)" \ - "${TASK_FORCE_AGENT:-claude}" "$loadout" + "$agent" "$loadout" # Preserve the --auto opt-in across self-heal so cmd_send keeps using CR # for wake-up after an intra-session re-seed (#128). Use `if` rather than # `&&` so the brace group exits 0 when the flag is absent — otherwise @@ -234,7 +258,7 @@ _ensure_session_file() { } | _atomic_write "$f" mkdir -p "$(_inbox_dir "$TASK_FORCE_ROLE")" "$(_processed_dir "$TASK_FORCE_ROLE")" - _log "ensure_session: re-seeded $TASK_FORCE_ROLE (file was missing) tab_id=$tab_id loadout=$loadout" + _log "ensure_session: re-seeded $TASK_FORCE_ROLE (file was missing) tab_id=$tab_id loadout=$loadout agent=$agent" } # Update STATE=… and LAST_HEARTBEAT=… on the current session file. @@ -385,29 +409,33 @@ cmd_register() { fi fi - # Persist the loadout sidecar BEFORE the session file (#150 round-2 review: - # Finding 1). If the process is killed (OOM, task-done --force, host - # shutdown) between these two _atomic_write calls, the order matters: + # Persist the loadout and agent sidecars BEFORE the session file (#150 + # round-2 review: Finding 1; extended for AGENT in #151). If the process is + # killed (OOM, task-done --force, host shutdown) between these writes, the + # order matters: # # sidecar-first (this order): - # pre-kill state: .loadout=claude-gh, .info absent - # re-seed reads: .loadout → LOADOUT=claude-gh recovered ✓ + # pre-kill state: .loadout=claude-gh, .agent=claude, .info absent + # re-seed reads: sidecars → LOADOUT/AGENT recovered ✓ # # info-first (the OLD order — pre-#150-r3): - # pre-kill state: .info has LOADOUT=claude-gh, .loadout absent - # unregister cascade deletes .info; .loadout never existed + # pre-kill state: .info has LOADOUT/AGENT, sidecars absent + # unregister cascade deletes .info; sidecars never existed # re-seed reads: no sidecar, hook subshell has no $TASK_FORCE_LOADOUT - # → LOADOUT=unknown ✗ + # / $TASK_FORCE_AGENT → LOADOUT=unknown / AGENT=claude ✗ # # The window is microseconds wide, but the cascade in #140 makes it # statistically reachable. Reversing the order eliminates the failure mode - # entirely. Skipping the sidecar on empty $loadout keeps the sidecar's mere + # entirely. Skipping the loadout sidecar on empty $loadout keeps its mere # existence equivalent to "we have a real loadout to restore" — otherwise # a re-seed could read an empty sidecar and clobber the env-var fallback - # that test paths still rely on. + # that test paths still rely on. The agent sidecar is always written + # because --agent is required (cmd_register exits 2 above if absent), so + # $agent is always a real value here. if [[ -n "$loadout" ]]; then printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" fi + printf '%s' "$agent" | _atomic_write "$(_agent_sidecar "$role")" { printf 'ROLE=%s\nTAB=%s\nTAB_ID=%s\nREPO=%s\nSTATE=idle\nLAST_HEARTBEAT=%s\nAGENT=%s\nLOADOUT=%s\n' \ @@ -461,6 +489,22 @@ cmd_unregister() { _log "unregister: skipping (reason=$reason — intra-session event) role=$TASK_FORCE_ROLE" return 0 ;; + "") + # Cascade pattern (#151): payload is present but contains no + # parseable `.reason` field. The PR-#150 diagnostic showed bursts of + # 144 unregister calls in ~25s with payload literally "y" and + # reason= — phantom invocations from somewhere upstream of + # Claude Code's documented SessionEnd hook (which always includes + # reason ∈ {clear, resume, logout, prompt_input_exit, other}). + # Treat these the same as clear/resume: known non-real-exit events + # that must not wipe the session file or the sidecars. Real exits + # still flow through the logout/prompt_input_exit/other branches + # below and clean up normally; manual invocations (no stdin) also + # proceed because `[[ ! -t 0 ]]` is false then. + payload_oneline=$(printf '%s' "$payload" | tr '\n' ' ' | tr -s ' ') + _log "unregister: skipping (reason= — cascade, no real-exit signal) role=$TASK_FORCE_ROLE payload=$payload_oneline" + return 0 + ;; esac fi # Instrumentation for #140: when the hook fires with a payload that @@ -468,7 +512,10 @@ cmd_unregister() { # what reason (or non-reason) is driving the unregister cascade. Empirically # observed as bursts of 20+ unregister calls in ~30s on long-running # worker sessions; the root cause needs the payload to investigate. Log - # is compacted to one line via tr so it doesn't fragment radio.log. + # is compacted to one line via tr so it doesn't fragment radio.log. Post- + # #151, an empty reason is already short-circuited above, so this line + # only fires for non-empty named reasons (logout, prompt_input_exit, + # other, or anything new Claude Code adds in the future). if [[ -n "$payload" ]]; then payload_oneline=$(printf '%s' "$payload" | tr '\n' ' ' | tr -s ' ') _log "unregister: proceeding (reason=${reason:-}) role=$TASK_FORCE_ROLE payload=$payload_oneline" @@ -478,10 +525,11 @@ cmd_unregister() { local f f=$(_session_file "$TASK_FORCE_ROLE") rm -f "$f" - # Sweep the loadout sidecar too — leaving it behind would let a future - # _ensure_session_file resurrect a stale loadout for a role whose tab has - # really exited and been re-used (#140). + # Sweep the loadout + agent sidecars too — leaving them behind would let a + # future _ensure_session_file resurrect a stale loadout / agent for a role + # whose tab has really exited and been re-used (#140, #151). rm -f "$(_loadout_sidecar "$TASK_FORCE_ROLE")" + rm -f "$(_agent_sidecar "$TASK_FORCE_ROLE")" _log "unregister role=$TASK_FORCE_ROLE" } diff --git a/claude-local/bin/task-done b/claude-local/bin/task-done index a5a1b63..21f923a 100755 --- a/claude-local/bin/task-done +++ b/claude-local/bin/task-done @@ -133,8 +133,14 @@ fi # `|| true` is a safety net: $TASK_FORCE_ROLE should be set inside a worker # tab, but if it isn't (e.g., task-done invoked from an unexpected shell) # the no-op must not abort cleanup. `radio unregister` itself already -# silently exits when $TASK_FORCE_ROLE is unset (#93). -radio unregister 2>/dev/null || true +# silently exits when $TASK_FORCE_ROLE is unset (#93). Redirect stdin from +# /dev/null so `radio unregister` doesn't inherit task-done's stdin — under +# `--force` the calling shell may have piped `y` (or any other content) +# to confirm something else, and post-#151 a non-empty stdin without a +# parseable JSON reason triggers the cascade-skip path. That's correct +# behaviour for SessionEnd hook invocations but wrong for explicit +# task-done cleanup, which must always proceed with the unregister. +radio unregister /dev/null || true # Move out of worktree before removing it cd "$MAIN_WORKTREE" diff --git a/claude-notion/bin/radio b/claude-notion/bin/radio index a091d7b..5ae3e6b 100755 --- a/claude-notion/bin/radio +++ b/claude-notion/bin/radio @@ -167,6 +167,17 @@ _loadout_sidecar() { printf '%s/%s.loadout' "$SESSIONS_DIR" "$1" } +# Agent sidecar (#151). Same problem as LOADOUT: $TASK_FORCE_AGENT isn't +# exported into Claude Code's hook subshells, so _ensure_session_file's +# previous fallback to "${TASK_FORCE_AGENT:-claude}" silently wrote +# AGENT=claude for kiro-* workers on every re-seed. Parallel sidecar, parallel +# write/read/sweep semantics — no current runtime consumer, but a kiro worker +# whose re-seeded session reports AGENT=claude is a footgun if anyone wires +# AGENT-conditional behaviour later. +_agent_sidecar() { + printf '%s/%s.agent' "$SESSIONS_DIR" "$1" +} + _inbox_dir() { printf '%s/%s/inbox' "$MAILBOX_DIR" "$1"; } _processed_dir(){ printf '%s/%s/processed' "$MAILBOX_DIR" "$1"; } @@ -216,13 +227,26 @@ _ensure_session_file() { loadout=$(cat "$sidecar" 2>/dev/null || true) [[ -n "$loadout" ]] || loadout="${TASK_FORCE_LOADOUT:-unknown}" + # AGENT recovery (#151): same hook-subshell unset story as LOADOUT — the + # cascade wipes .info, and the next re-seed used to default to "claude" + # regardless of whether this was a kiro-* worker. Read the agent sidecar + # cmd_register persisted; fall back to the env var only for test paths that + # exercise _ensure_session_file directly. Same TOCTOU-safe `cat … || true` + # pattern as the loadout read above for the same reason (concurrent + # unregister race window). + local agent="" + local agent_sidecar + agent_sidecar=$(_agent_sidecar "$TASK_FORCE_ROLE") + agent=$(cat "$agent_sidecar" 2>/dev/null || true) + [[ -n "$agent" ]] || agent="${TASK_FORCE_AGENT:-claude}" + local repo repo=$(git rev-parse --show-toplevel 2>/dev/null || echo "") { printf 'ROLE=%s\nTAB=%s\nTAB_ID=%s\nREPO=%s\nSTATE=idle\nLAST_HEARTBEAT=%s\nAGENT=%s\nLOADOUT=%s\n' \ "$TASK_FORCE_ROLE" "$ZELLIJ_TAB" "$tab_id" "$repo" "$(_now)" \ - "${TASK_FORCE_AGENT:-claude}" "$loadout" + "$agent" "$loadout" # Preserve the --auto opt-in across self-heal so cmd_send keeps using CR # for wake-up after an intra-session re-seed (#128). Use `if` rather than # `&&` so the brace group exits 0 when the flag is absent — otherwise @@ -234,7 +258,7 @@ _ensure_session_file() { } | _atomic_write "$f" mkdir -p "$(_inbox_dir "$TASK_FORCE_ROLE")" "$(_processed_dir "$TASK_FORCE_ROLE")" - _log "ensure_session: re-seeded $TASK_FORCE_ROLE (file was missing) tab_id=$tab_id loadout=$loadout" + _log "ensure_session: re-seeded $TASK_FORCE_ROLE (file was missing) tab_id=$tab_id loadout=$loadout agent=$agent" } # Update STATE=… and LAST_HEARTBEAT=… on the current session file. @@ -385,29 +409,33 @@ cmd_register() { fi fi - # Persist the loadout sidecar BEFORE the session file (#150 round-2 review: - # Finding 1). If the process is killed (OOM, task-done --force, host - # shutdown) between these two _atomic_write calls, the order matters: + # Persist the loadout and agent sidecars BEFORE the session file (#150 + # round-2 review: Finding 1; extended for AGENT in #151). If the process is + # killed (OOM, task-done --force, host shutdown) between these writes, the + # order matters: # # sidecar-first (this order): - # pre-kill state: .loadout=claude-gh, .info absent - # re-seed reads: .loadout → LOADOUT=claude-gh recovered ✓ + # pre-kill state: .loadout=claude-gh, .agent=claude, .info absent + # re-seed reads: sidecars → LOADOUT/AGENT recovered ✓ # # info-first (the OLD order — pre-#150-r3): - # pre-kill state: .info has LOADOUT=claude-gh, .loadout absent - # unregister cascade deletes .info; .loadout never existed + # pre-kill state: .info has LOADOUT/AGENT, sidecars absent + # unregister cascade deletes .info; sidecars never existed # re-seed reads: no sidecar, hook subshell has no $TASK_FORCE_LOADOUT - # → LOADOUT=unknown ✗ + # / $TASK_FORCE_AGENT → LOADOUT=unknown / AGENT=claude ✗ # # The window is microseconds wide, but the cascade in #140 makes it # statistically reachable. Reversing the order eliminates the failure mode - # entirely. Skipping the sidecar on empty $loadout keeps the sidecar's mere + # entirely. Skipping the loadout sidecar on empty $loadout keeps its mere # existence equivalent to "we have a real loadout to restore" — otherwise # a re-seed could read an empty sidecar and clobber the env-var fallback - # that test paths still rely on. + # that test paths still rely on. The agent sidecar is always written + # because --agent is required (cmd_register exits 2 above if absent), so + # $agent is always a real value here. if [[ -n "$loadout" ]]; then printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" fi + printf '%s' "$agent" | _atomic_write "$(_agent_sidecar "$role")" { printf 'ROLE=%s\nTAB=%s\nTAB_ID=%s\nREPO=%s\nSTATE=idle\nLAST_HEARTBEAT=%s\nAGENT=%s\nLOADOUT=%s\n' \ @@ -461,6 +489,22 @@ cmd_unregister() { _log "unregister: skipping (reason=$reason — intra-session event) role=$TASK_FORCE_ROLE" return 0 ;; + "") + # Cascade pattern (#151): payload is present but contains no + # parseable `.reason` field. The PR-#150 diagnostic showed bursts of + # 144 unregister calls in ~25s with payload literally "y" and + # reason= — phantom invocations from somewhere upstream of + # Claude Code's documented SessionEnd hook (which always includes + # reason ∈ {clear, resume, logout, prompt_input_exit, other}). + # Treat these the same as clear/resume: known non-real-exit events + # that must not wipe the session file or the sidecars. Real exits + # still flow through the logout/prompt_input_exit/other branches + # below and clean up normally; manual invocations (no stdin) also + # proceed because `[[ ! -t 0 ]]` is false then. + payload_oneline=$(printf '%s' "$payload" | tr '\n' ' ' | tr -s ' ') + _log "unregister: skipping (reason= — cascade, no real-exit signal) role=$TASK_FORCE_ROLE payload=$payload_oneline" + return 0 + ;; esac fi # Instrumentation for #140: when the hook fires with a payload that @@ -468,7 +512,10 @@ cmd_unregister() { # what reason (or non-reason) is driving the unregister cascade. Empirically # observed as bursts of 20+ unregister calls in ~30s on long-running # worker sessions; the root cause needs the payload to investigate. Log - # is compacted to one line via tr so it doesn't fragment radio.log. + # is compacted to one line via tr so it doesn't fragment radio.log. Post- + # #151, an empty reason is already short-circuited above, so this line + # only fires for non-empty named reasons (logout, prompt_input_exit, + # other, or anything new Claude Code adds in the future). if [[ -n "$payload" ]]; then payload_oneline=$(printf '%s' "$payload" | tr '\n' ' ' | tr -s ' ') _log "unregister: proceeding (reason=${reason:-}) role=$TASK_FORCE_ROLE payload=$payload_oneline" @@ -478,10 +525,11 @@ cmd_unregister() { local f f=$(_session_file "$TASK_FORCE_ROLE") rm -f "$f" - # Sweep the loadout sidecar too — leaving it behind would let a future - # _ensure_session_file resurrect a stale loadout for a role whose tab has - # really exited and been re-used (#140). + # Sweep the loadout + agent sidecars too — leaving them behind would let a + # future _ensure_session_file resurrect a stale loadout / agent for a role + # whose tab has really exited and been re-used (#140, #151). rm -f "$(_loadout_sidecar "$TASK_FORCE_ROLE")" + rm -f "$(_agent_sidecar "$TASK_FORCE_ROLE")" _log "unregister role=$TASK_FORCE_ROLE" } diff --git a/claude-notion/bin/task-done b/claude-notion/bin/task-done index b44dfc6..c627525 100755 --- a/claude-notion/bin/task-done +++ b/claude-notion/bin/task-done @@ -131,8 +131,14 @@ fi # `|| true` is a safety net: $TASK_FORCE_ROLE should be set inside a worker # tab, but if it isn't (e.g., task-done invoked from an unexpected shell) # the no-op must not abort cleanup. `radio unregister` itself already -# silently exits when $TASK_FORCE_ROLE is unset (#93). -radio unregister 2>/dev/null || true +# silently exits when $TASK_FORCE_ROLE is unset (#93). Redirect stdin from +# /dev/null so `radio unregister` doesn't inherit task-done's stdin — under +# `--force` the calling shell may have piped `y` (or any other content) +# to confirm something else, and post-#151 a non-empty stdin without a +# parseable JSON reason triggers the cascade-skip path. That's correct +# behaviour for SessionEnd hook invocations but wrong for explicit +# task-done cleanup, which must always proceed with the unregister. +radio unregister /dev/null || true # Move out of worktree before removing it cd "$MAIN_WORKTREE" diff --git a/kiro-gh/bin/radio b/kiro-gh/bin/radio index a091d7b..5ae3e6b 100755 --- a/kiro-gh/bin/radio +++ b/kiro-gh/bin/radio @@ -167,6 +167,17 @@ _loadout_sidecar() { printf '%s/%s.loadout' "$SESSIONS_DIR" "$1" } +# Agent sidecar (#151). Same problem as LOADOUT: $TASK_FORCE_AGENT isn't +# exported into Claude Code's hook subshells, so _ensure_session_file's +# previous fallback to "${TASK_FORCE_AGENT:-claude}" silently wrote +# AGENT=claude for kiro-* workers on every re-seed. Parallel sidecar, parallel +# write/read/sweep semantics — no current runtime consumer, but a kiro worker +# whose re-seeded session reports AGENT=claude is a footgun if anyone wires +# AGENT-conditional behaviour later. +_agent_sidecar() { + printf '%s/%s.agent' "$SESSIONS_DIR" "$1" +} + _inbox_dir() { printf '%s/%s/inbox' "$MAILBOX_DIR" "$1"; } _processed_dir(){ printf '%s/%s/processed' "$MAILBOX_DIR" "$1"; } @@ -216,13 +227,26 @@ _ensure_session_file() { loadout=$(cat "$sidecar" 2>/dev/null || true) [[ -n "$loadout" ]] || loadout="${TASK_FORCE_LOADOUT:-unknown}" + # AGENT recovery (#151): same hook-subshell unset story as LOADOUT — the + # cascade wipes .info, and the next re-seed used to default to "claude" + # regardless of whether this was a kiro-* worker. Read the agent sidecar + # cmd_register persisted; fall back to the env var only for test paths that + # exercise _ensure_session_file directly. Same TOCTOU-safe `cat … || true` + # pattern as the loadout read above for the same reason (concurrent + # unregister race window). + local agent="" + local agent_sidecar + agent_sidecar=$(_agent_sidecar "$TASK_FORCE_ROLE") + agent=$(cat "$agent_sidecar" 2>/dev/null || true) + [[ -n "$agent" ]] || agent="${TASK_FORCE_AGENT:-claude}" + local repo repo=$(git rev-parse --show-toplevel 2>/dev/null || echo "") { printf 'ROLE=%s\nTAB=%s\nTAB_ID=%s\nREPO=%s\nSTATE=idle\nLAST_HEARTBEAT=%s\nAGENT=%s\nLOADOUT=%s\n' \ "$TASK_FORCE_ROLE" "$ZELLIJ_TAB" "$tab_id" "$repo" "$(_now)" \ - "${TASK_FORCE_AGENT:-claude}" "$loadout" + "$agent" "$loadout" # Preserve the --auto opt-in across self-heal so cmd_send keeps using CR # for wake-up after an intra-session re-seed (#128). Use `if` rather than # `&&` so the brace group exits 0 when the flag is absent — otherwise @@ -234,7 +258,7 @@ _ensure_session_file() { } | _atomic_write "$f" mkdir -p "$(_inbox_dir "$TASK_FORCE_ROLE")" "$(_processed_dir "$TASK_FORCE_ROLE")" - _log "ensure_session: re-seeded $TASK_FORCE_ROLE (file was missing) tab_id=$tab_id loadout=$loadout" + _log "ensure_session: re-seeded $TASK_FORCE_ROLE (file was missing) tab_id=$tab_id loadout=$loadout agent=$agent" } # Update STATE=… and LAST_HEARTBEAT=… on the current session file. @@ -385,29 +409,33 @@ cmd_register() { fi fi - # Persist the loadout sidecar BEFORE the session file (#150 round-2 review: - # Finding 1). If the process is killed (OOM, task-done --force, host - # shutdown) between these two _atomic_write calls, the order matters: + # Persist the loadout and agent sidecars BEFORE the session file (#150 + # round-2 review: Finding 1; extended for AGENT in #151). If the process is + # killed (OOM, task-done --force, host shutdown) between these writes, the + # order matters: # # sidecar-first (this order): - # pre-kill state: .loadout=claude-gh, .info absent - # re-seed reads: .loadout → LOADOUT=claude-gh recovered ✓ + # pre-kill state: .loadout=claude-gh, .agent=claude, .info absent + # re-seed reads: sidecars → LOADOUT/AGENT recovered ✓ # # info-first (the OLD order — pre-#150-r3): - # pre-kill state: .info has LOADOUT=claude-gh, .loadout absent - # unregister cascade deletes .info; .loadout never existed + # pre-kill state: .info has LOADOUT/AGENT, sidecars absent + # unregister cascade deletes .info; sidecars never existed # re-seed reads: no sidecar, hook subshell has no $TASK_FORCE_LOADOUT - # → LOADOUT=unknown ✗ + # / $TASK_FORCE_AGENT → LOADOUT=unknown / AGENT=claude ✗ # # The window is microseconds wide, but the cascade in #140 makes it # statistically reachable. Reversing the order eliminates the failure mode - # entirely. Skipping the sidecar on empty $loadout keeps the sidecar's mere + # entirely. Skipping the loadout sidecar on empty $loadout keeps its mere # existence equivalent to "we have a real loadout to restore" — otherwise # a re-seed could read an empty sidecar and clobber the env-var fallback - # that test paths still rely on. + # that test paths still rely on. The agent sidecar is always written + # because --agent is required (cmd_register exits 2 above if absent), so + # $agent is always a real value here. if [[ -n "$loadout" ]]; then printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" fi + printf '%s' "$agent" | _atomic_write "$(_agent_sidecar "$role")" { printf 'ROLE=%s\nTAB=%s\nTAB_ID=%s\nREPO=%s\nSTATE=idle\nLAST_HEARTBEAT=%s\nAGENT=%s\nLOADOUT=%s\n' \ @@ -461,6 +489,22 @@ cmd_unregister() { _log "unregister: skipping (reason=$reason — intra-session event) role=$TASK_FORCE_ROLE" return 0 ;; + "") + # Cascade pattern (#151): payload is present but contains no + # parseable `.reason` field. The PR-#150 diagnostic showed bursts of + # 144 unregister calls in ~25s with payload literally "y" and + # reason= — phantom invocations from somewhere upstream of + # Claude Code's documented SessionEnd hook (which always includes + # reason ∈ {clear, resume, logout, prompt_input_exit, other}). + # Treat these the same as clear/resume: known non-real-exit events + # that must not wipe the session file or the sidecars. Real exits + # still flow through the logout/prompt_input_exit/other branches + # below and clean up normally; manual invocations (no stdin) also + # proceed because `[[ ! -t 0 ]]` is false then. + payload_oneline=$(printf '%s' "$payload" | tr '\n' ' ' | tr -s ' ') + _log "unregister: skipping (reason= — cascade, no real-exit signal) role=$TASK_FORCE_ROLE payload=$payload_oneline" + return 0 + ;; esac fi # Instrumentation for #140: when the hook fires with a payload that @@ -468,7 +512,10 @@ cmd_unregister() { # what reason (or non-reason) is driving the unregister cascade. Empirically # observed as bursts of 20+ unregister calls in ~30s on long-running # worker sessions; the root cause needs the payload to investigate. Log - # is compacted to one line via tr so it doesn't fragment radio.log. + # is compacted to one line via tr so it doesn't fragment radio.log. Post- + # #151, an empty reason is already short-circuited above, so this line + # only fires for non-empty named reasons (logout, prompt_input_exit, + # other, or anything new Claude Code adds in the future). if [[ -n "$payload" ]]; then payload_oneline=$(printf '%s' "$payload" | tr '\n' ' ' | tr -s ' ') _log "unregister: proceeding (reason=${reason:-}) role=$TASK_FORCE_ROLE payload=$payload_oneline" @@ -478,10 +525,11 @@ cmd_unregister() { local f f=$(_session_file "$TASK_FORCE_ROLE") rm -f "$f" - # Sweep the loadout sidecar too — leaving it behind would let a future - # _ensure_session_file resurrect a stale loadout for a role whose tab has - # really exited and been re-used (#140). + # Sweep the loadout + agent sidecars too — leaving them behind would let a + # future _ensure_session_file resurrect a stale loadout / agent for a role + # whose tab has really exited and been re-used (#140, #151). rm -f "$(_loadout_sidecar "$TASK_FORCE_ROLE")" + rm -f "$(_agent_sidecar "$TASK_FORCE_ROLE")" _log "unregister role=$TASK_FORCE_ROLE" } diff --git a/kiro-gh/bin/task-done b/kiro-gh/bin/task-done index b44dfc6..c627525 100755 --- a/kiro-gh/bin/task-done +++ b/kiro-gh/bin/task-done @@ -131,8 +131,14 @@ fi # `|| true` is a safety net: $TASK_FORCE_ROLE should be set inside a worker # tab, but if it isn't (e.g., task-done invoked from an unexpected shell) # the no-op must not abort cleanup. `radio unregister` itself already -# silently exits when $TASK_FORCE_ROLE is unset (#93). -radio unregister 2>/dev/null || true +# silently exits when $TASK_FORCE_ROLE is unset (#93). Redirect stdin from +# /dev/null so `radio unregister` doesn't inherit task-done's stdin — under +# `--force` the calling shell may have piped `y` (or any other content) +# to confirm something else, and post-#151 a non-empty stdin without a +# parseable JSON reason triggers the cascade-skip path. That's correct +# behaviour for SessionEnd hook invocations but wrong for explicit +# task-done cleanup, which must always proceed with the unregister. +radio unregister /dev/null || true # Move out of worktree before removing it cd "$MAIN_WORKTREE" diff --git a/kiro-local/bin/radio b/kiro-local/bin/radio index a091d7b..5ae3e6b 100755 --- a/kiro-local/bin/radio +++ b/kiro-local/bin/radio @@ -167,6 +167,17 @@ _loadout_sidecar() { printf '%s/%s.loadout' "$SESSIONS_DIR" "$1" } +# Agent sidecar (#151). Same problem as LOADOUT: $TASK_FORCE_AGENT isn't +# exported into Claude Code's hook subshells, so _ensure_session_file's +# previous fallback to "${TASK_FORCE_AGENT:-claude}" silently wrote +# AGENT=claude for kiro-* workers on every re-seed. Parallel sidecar, parallel +# write/read/sweep semantics — no current runtime consumer, but a kiro worker +# whose re-seeded session reports AGENT=claude is a footgun if anyone wires +# AGENT-conditional behaviour later. +_agent_sidecar() { + printf '%s/%s.agent' "$SESSIONS_DIR" "$1" +} + _inbox_dir() { printf '%s/%s/inbox' "$MAILBOX_DIR" "$1"; } _processed_dir(){ printf '%s/%s/processed' "$MAILBOX_DIR" "$1"; } @@ -216,13 +227,26 @@ _ensure_session_file() { loadout=$(cat "$sidecar" 2>/dev/null || true) [[ -n "$loadout" ]] || loadout="${TASK_FORCE_LOADOUT:-unknown}" + # AGENT recovery (#151): same hook-subshell unset story as LOADOUT — the + # cascade wipes .info, and the next re-seed used to default to "claude" + # regardless of whether this was a kiro-* worker. Read the agent sidecar + # cmd_register persisted; fall back to the env var only for test paths that + # exercise _ensure_session_file directly. Same TOCTOU-safe `cat … || true` + # pattern as the loadout read above for the same reason (concurrent + # unregister race window). + local agent="" + local agent_sidecar + agent_sidecar=$(_agent_sidecar "$TASK_FORCE_ROLE") + agent=$(cat "$agent_sidecar" 2>/dev/null || true) + [[ -n "$agent" ]] || agent="${TASK_FORCE_AGENT:-claude}" + local repo repo=$(git rev-parse --show-toplevel 2>/dev/null || echo "") { printf 'ROLE=%s\nTAB=%s\nTAB_ID=%s\nREPO=%s\nSTATE=idle\nLAST_HEARTBEAT=%s\nAGENT=%s\nLOADOUT=%s\n' \ "$TASK_FORCE_ROLE" "$ZELLIJ_TAB" "$tab_id" "$repo" "$(_now)" \ - "${TASK_FORCE_AGENT:-claude}" "$loadout" + "$agent" "$loadout" # Preserve the --auto opt-in across self-heal so cmd_send keeps using CR # for wake-up after an intra-session re-seed (#128). Use `if` rather than # `&&` so the brace group exits 0 when the flag is absent — otherwise @@ -234,7 +258,7 @@ _ensure_session_file() { } | _atomic_write "$f" mkdir -p "$(_inbox_dir "$TASK_FORCE_ROLE")" "$(_processed_dir "$TASK_FORCE_ROLE")" - _log "ensure_session: re-seeded $TASK_FORCE_ROLE (file was missing) tab_id=$tab_id loadout=$loadout" + _log "ensure_session: re-seeded $TASK_FORCE_ROLE (file was missing) tab_id=$tab_id loadout=$loadout agent=$agent" } # Update STATE=… and LAST_HEARTBEAT=… on the current session file. @@ -385,29 +409,33 @@ cmd_register() { fi fi - # Persist the loadout sidecar BEFORE the session file (#150 round-2 review: - # Finding 1). If the process is killed (OOM, task-done --force, host - # shutdown) between these two _atomic_write calls, the order matters: + # Persist the loadout and agent sidecars BEFORE the session file (#150 + # round-2 review: Finding 1; extended for AGENT in #151). If the process is + # killed (OOM, task-done --force, host shutdown) between these writes, the + # order matters: # # sidecar-first (this order): - # pre-kill state: .loadout=claude-gh, .info absent - # re-seed reads: .loadout → LOADOUT=claude-gh recovered ✓ + # pre-kill state: .loadout=claude-gh, .agent=claude, .info absent + # re-seed reads: sidecars → LOADOUT/AGENT recovered ✓ # # info-first (the OLD order — pre-#150-r3): - # pre-kill state: .info has LOADOUT=claude-gh, .loadout absent - # unregister cascade deletes .info; .loadout never existed + # pre-kill state: .info has LOADOUT/AGENT, sidecars absent + # unregister cascade deletes .info; sidecars never existed # re-seed reads: no sidecar, hook subshell has no $TASK_FORCE_LOADOUT - # → LOADOUT=unknown ✗ + # / $TASK_FORCE_AGENT → LOADOUT=unknown / AGENT=claude ✗ # # The window is microseconds wide, but the cascade in #140 makes it # statistically reachable. Reversing the order eliminates the failure mode - # entirely. Skipping the sidecar on empty $loadout keeps the sidecar's mere + # entirely. Skipping the loadout sidecar on empty $loadout keeps its mere # existence equivalent to "we have a real loadout to restore" — otherwise # a re-seed could read an empty sidecar and clobber the env-var fallback - # that test paths still rely on. + # that test paths still rely on. The agent sidecar is always written + # because --agent is required (cmd_register exits 2 above if absent), so + # $agent is always a real value here. if [[ -n "$loadout" ]]; then printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" fi + printf '%s' "$agent" | _atomic_write "$(_agent_sidecar "$role")" { printf 'ROLE=%s\nTAB=%s\nTAB_ID=%s\nREPO=%s\nSTATE=idle\nLAST_HEARTBEAT=%s\nAGENT=%s\nLOADOUT=%s\n' \ @@ -461,6 +489,22 @@ cmd_unregister() { _log "unregister: skipping (reason=$reason — intra-session event) role=$TASK_FORCE_ROLE" return 0 ;; + "") + # Cascade pattern (#151): payload is present but contains no + # parseable `.reason` field. The PR-#150 diagnostic showed bursts of + # 144 unregister calls in ~25s with payload literally "y" and + # reason= — phantom invocations from somewhere upstream of + # Claude Code's documented SessionEnd hook (which always includes + # reason ∈ {clear, resume, logout, prompt_input_exit, other}). + # Treat these the same as clear/resume: known non-real-exit events + # that must not wipe the session file or the sidecars. Real exits + # still flow through the logout/prompt_input_exit/other branches + # below and clean up normally; manual invocations (no stdin) also + # proceed because `[[ ! -t 0 ]]` is false then. + payload_oneline=$(printf '%s' "$payload" | tr '\n' ' ' | tr -s ' ') + _log "unregister: skipping (reason= — cascade, no real-exit signal) role=$TASK_FORCE_ROLE payload=$payload_oneline" + return 0 + ;; esac fi # Instrumentation for #140: when the hook fires with a payload that @@ -468,7 +512,10 @@ cmd_unregister() { # what reason (or non-reason) is driving the unregister cascade. Empirically # observed as bursts of 20+ unregister calls in ~30s on long-running # worker sessions; the root cause needs the payload to investigate. Log - # is compacted to one line via tr so it doesn't fragment radio.log. + # is compacted to one line via tr so it doesn't fragment radio.log. Post- + # #151, an empty reason is already short-circuited above, so this line + # only fires for non-empty named reasons (logout, prompt_input_exit, + # other, or anything new Claude Code adds in the future). if [[ -n "$payload" ]]; then payload_oneline=$(printf '%s' "$payload" | tr '\n' ' ' | tr -s ' ') _log "unregister: proceeding (reason=${reason:-}) role=$TASK_FORCE_ROLE payload=$payload_oneline" @@ -478,10 +525,11 @@ cmd_unregister() { local f f=$(_session_file "$TASK_FORCE_ROLE") rm -f "$f" - # Sweep the loadout sidecar too — leaving it behind would let a future - # _ensure_session_file resurrect a stale loadout for a role whose tab has - # really exited and been re-used (#140). + # Sweep the loadout + agent sidecars too — leaving them behind would let a + # future _ensure_session_file resurrect a stale loadout / agent for a role + # whose tab has really exited and been re-used (#140, #151). rm -f "$(_loadout_sidecar "$TASK_FORCE_ROLE")" + rm -f "$(_agent_sidecar "$TASK_FORCE_ROLE")" _log "unregister role=$TASK_FORCE_ROLE" } diff --git a/kiro-local/bin/task-done b/kiro-local/bin/task-done index a5a1b63..21f923a 100755 --- a/kiro-local/bin/task-done +++ b/kiro-local/bin/task-done @@ -133,8 +133,14 @@ fi # `|| true` is a safety net: $TASK_FORCE_ROLE should be set inside a worker # tab, but if it isn't (e.g., task-done invoked from an unexpected shell) # the no-op must not abort cleanup. `radio unregister` itself already -# silently exits when $TASK_FORCE_ROLE is unset (#93). -radio unregister 2>/dev/null || true +# silently exits when $TASK_FORCE_ROLE is unset (#93). Redirect stdin from +# /dev/null so `radio unregister` doesn't inherit task-done's stdin — under +# `--force` the calling shell may have piped `y` (or any other content) +# to confirm something else, and post-#151 a non-empty stdin without a +# parseable JSON reason triggers the cascade-skip path. That's correct +# behaviour for SessionEnd hook invocations but wrong for explicit +# task-done cleanup, which must always proceed with the unregister. +radio unregister /dev/null || true # Move out of worktree before removing it cd "$MAIN_WORKTREE" diff --git a/kiro-notion/bin/radio b/kiro-notion/bin/radio index a091d7b..5ae3e6b 100755 --- a/kiro-notion/bin/radio +++ b/kiro-notion/bin/radio @@ -167,6 +167,17 @@ _loadout_sidecar() { printf '%s/%s.loadout' "$SESSIONS_DIR" "$1" } +# Agent sidecar (#151). Same problem as LOADOUT: $TASK_FORCE_AGENT isn't +# exported into Claude Code's hook subshells, so _ensure_session_file's +# previous fallback to "${TASK_FORCE_AGENT:-claude}" silently wrote +# AGENT=claude for kiro-* workers on every re-seed. Parallel sidecar, parallel +# write/read/sweep semantics — no current runtime consumer, but a kiro worker +# whose re-seeded session reports AGENT=claude is a footgun if anyone wires +# AGENT-conditional behaviour later. +_agent_sidecar() { + printf '%s/%s.agent' "$SESSIONS_DIR" "$1" +} + _inbox_dir() { printf '%s/%s/inbox' "$MAILBOX_DIR" "$1"; } _processed_dir(){ printf '%s/%s/processed' "$MAILBOX_DIR" "$1"; } @@ -216,13 +227,26 @@ _ensure_session_file() { loadout=$(cat "$sidecar" 2>/dev/null || true) [[ -n "$loadout" ]] || loadout="${TASK_FORCE_LOADOUT:-unknown}" + # AGENT recovery (#151): same hook-subshell unset story as LOADOUT — the + # cascade wipes .info, and the next re-seed used to default to "claude" + # regardless of whether this was a kiro-* worker. Read the agent sidecar + # cmd_register persisted; fall back to the env var only for test paths that + # exercise _ensure_session_file directly. Same TOCTOU-safe `cat … || true` + # pattern as the loadout read above for the same reason (concurrent + # unregister race window). + local agent="" + local agent_sidecar + agent_sidecar=$(_agent_sidecar "$TASK_FORCE_ROLE") + agent=$(cat "$agent_sidecar" 2>/dev/null || true) + [[ -n "$agent" ]] || agent="${TASK_FORCE_AGENT:-claude}" + local repo repo=$(git rev-parse --show-toplevel 2>/dev/null || echo "") { printf 'ROLE=%s\nTAB=%s\nTAB_ID=%s\nREPO=%s\nSTATE=idle\nLAST_HEARTBEAT=%s\nAGENT=%s\nLOADOUT=%s\n' \ "$TASK_FORCE_ROLE" "$ZELLIJ_TAB" "$tab_id" "$repo" "$(_now)" \ - "${TASK_FORCE_AGENT:-claude}" "$loadout" + "$agent" "$loadout" # Preserve the --auto opt-in across self-heal so cmd_send keeps using CR # for wake-up after an intra-session re-seed (#128). Use `if` rather than # `&&` so the brace group exits 0 when the flag is absent — otherwise @@ -234,7 +258,7 @@ _ensure_session_file() { } | _atomic_write "$f" mkdir -p "$(_inbox_dir "$TASK_FORCE_ROLE")" "$(_processed_dir "$TASK_FORCE_ROLE")" - _log "ensure_session: re-seeded $TASK_FORCE_ROLE (file was missing) tab_id=$tab_id loadout=$loadout" + _log "ensure_session: re-seeded $TASK_FORCE_ROLE (file was missing) tab_id=$tab_id loadout=$loadout agent=$agent" } # Update STATE=… and LAST_HEARTBEAT=… on the current session file. @@ -385,29 +409,33 @@ cmd_register() { fi fi - # Persist the loadout sidecar BEFORE the session file (#150 round-2 review: - # Finding 1). If the process is killed (OOM, task-done --force, host - # shutdown) between these two _atomic_write calls, the order matters: + # Persist the loadout and agent sidecars BEFORE the session file (#150 + # round-2 review: Finding 1; extended for AGENT in #151). If the process is + # killed (OOM, task-done --force, host shutdown) between these writes, the + # order matters: # # sidecar-first (this order): - # pre-kill state: .loadout=claude-gh, .info absent - # re-seed reads: .loadout → LOADOUT=claude-gh recovered ✓ + # pre-kill state: .loadout=claude-gh, .agent=claude, .info absent + # re-seed reads: sidecars → LOADOUT/AGENT recovered ✓ # # info-first (the OLD order — pre-#150-r3): - # pre-kill state: .info has LOADOUT=claude-gh, .loadout absent - # unregister cascade deletes .info; .loadout never existed + # pre-kill state: .info has LOADOUT/AGENT, sidecars absent + # unregister cascade deletes .info; sidecars never existed # re-seed reads: no sidecar, hook subshell has no $TASK_FORCE_LOADOUT - # → LOADOUT=unknown ✗ + # / $TASK_FORCE_AGENT → LOADOUT=unknown / AGENT=claude ✗ # # The window is microseconds wide, but the cascade in #140 makes it # statistically reachable. Reversing the order eliminates the failure mode - # entirely. Skipping the sidecar on empty $loadout keeps the sidecar's mere + # entirely. Skipping the loadout sidecar on empty $loadout keeps its mere # existence equivalent to "we have a real loadout to restore" — otherwise # a re-seed could read an empty sidecar and clobber the env-var fallback - # that test paths still rely on. + # that test paths still rely on. The agent sidecar is always written + # because --agent is required (cmd_register exits 2 above if absent), so + # $agent is always a real value here. if [[ -n "$loadout" ]]; then printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" fi + printf '%s' "$agent" | _atomic_write "$(_agent_sidecar "$role")" { printf 'ROLE=%s\nTAB=%s\nTAB_ID=%s\nREPO=%s\nSTATE=idle\nLAST_HEARTBEAT=%s\nAGENT=%s\nLOADOUT=%s\n' \ @@ -461,6 +489,22 @@ cmd_unregister() { _log "unregister: skipping (reason=$reason — intra-session event) role=$TASK_FORCE_ROLE" return 0 ;; + "") + # Cascade pattern (#151): payload is present but contains no + # parseable `.reason` field. The PR-#150 diagnostic showed bursts of + # 144 unregister calls in ~25s with payload literally "y" and + # reason= — phantom invocations from somewhere upstream of + # Claude Code's documented SessionEnd hook (which always includes + # reason ∈ {clear, resume, logout, prompt_input_exit, other}). + # Treat these the same as clear/resume: known non-real-exit events + # that must not wipe the session file or the sidecars. Real exits + # still flow through the logout/prompt_input_exit/other branches + # below and clean up normally; manual invocations (no stdin) also + # proceed because `[[ ! -t 0 ]]` is false then. + payload_oneline=$(printf '%s' "$payload" | tr '\n' ' ' | tr -s ' ') + _log "unregister: skipping (reason= — cascade, no real-exit signal) role=$TASK_FORCE_ROLE payload=$payload_oneline" + return 0 + ;; esac fi # Instrumentation for #140: when the hook fires with a payload that @@ -468,7 +512,10 @@ cmd_unregister() { # what reason (or non-reason) is driving the unregister cascade. Empirically # observed as bursts of 20+ unregister calls in ~30s on long-running # worker sessions; the root cause needs the payload to investigate. Log - # is compacted to one line via tr so it doesn't fragment radio.log. + # is compacted to one line via tr so it doesn't fragment radio.log. Post- + # #151, an empty reason is already short-circuited above, so this line + # only fires for non-empty named reasons (logout, prompt_input_exit, + # other, or anything new Claude Code adds in the future). if [[ -n "$payload" ]]; then payload_oneline=$(printf '%s' "$payload" | tr '\n' ' ' | tr -s ' ') _log "unregister: proceeding (reason=${reason:-}) role=$TASK_FORCE_ROLE payload=$payload_oneline" @@ -478,10 +525,11 @@ cmd_unregister() { local f f=$(_session_file "$TASK_FORCE_ROLE") rm -f "$f" - # Sweep the loadout sidecar too — leaving it behind would let a future - # _ensure_session_file resurrect a stale loadout for a role whose tab has - # really exited and been re-used (#140). + # Sweep the loadout + agent sidecars too — leaving them behind would let a + # future _ensure_session_file resurrect a stale loadout / agent for a role + # whose tab has really exited and been re-used (#140, #151). rm -f "$(_loadout_sidecar "$TASK_FORCE_ROLE")" + rm -f "$(_agent_sidecar "$TASK_FORCE_ROLE")" _log "unregister role=$TASK_FORCE_ROLE" } diff --git a/kiro-notion/bin/task-done b/kiro-notion/bin/task-done index b44dfc6..c627525 100755 --- a/kiro-notion/bin/task-done +++ b/kiro-notion/bin/task-done @@ -131,8 +131,14 @@ fi # `|| true` is a safety net: $TASK_FORCE_ROLE should be set inside a worker # tab, but if it isn't (e.g., task-done invoked from an unexpected shell) # the no-op must not abort cleanup. `radio unregister` itself already -# silently exits when $TASK_FORCE_ROLE is unset (#93). -radio unregister 2>/dev/null || true +# silently exits when $TASK_FORCE_ROLE is unset (#93). Redirect stdin from +# /dev/null so `radio unregister` doesn't inherit task-done's stdin — under +# `--force` the calling shell may have piped `y` (or any other content) +# to confirm something else, and post-#151 a non-empty stdin without a +# parseable JSON reason triggers the cascade-skip path. That's correct +# behaviour for SessionEnd hook invocations but wrong for explicit +# task-done cleanup, which must always proceed with the unregister. +radio unregister /dev/null || true # Move out of worktree before removing it cd "$MAIN_WORKTREE" diff --git a/tests/radio_lifecycle.bats b/tests/radio_lifecycle.bats index b58af7b..722b5fe 100644 --- a/tests/radio_lifecycle.bats +++ b/tests/radio_lifecycle.bats @@ -84,20 +84,58 @@ teardown() { assert [ ! -f "$sess" ] } -@test "unregister proceeds when stdin payload is not valid JSON" { +@test "unregister skips when stdin payload is not valid JSON (cascade pattern #151)" { + # Pre-#151 this proceeded with the delete: jq returned an empty reason, + # nothing matched the skip-list, the file got wiped. The diagnostic logging + # added in PR #150 surfaced the actual cascade payload — literally a single + # "y" character, 144 instances in 25s on one long-running worker session. + # That's not a Claude Code SessionEnd hook firing (those always include a + # parseable JSON reason); it's a phantom invocation we don't want to act on. + # Treat any payload-present-but-no-reason input the same as clear/resume. "$RADIO" register --role worker-foo --tab w-foo --agent claude local sess="$TASK_FORCE_HOME/radio/sessions/worker-foo.info" TASK_FORCE_ROLE=worker-foo run bash -c "echo 'not json' | '$RADIO' unregister" assert_success - assert [ ! -f "$sess" ] + assert [ -f "$sess" ] + run cat "$TASK_FORCE_HOME/radio/log" + assert_output --partial "skipping (reason=" + assert_output --partial "cascade" } -@test "unregister proceeds when payload JSON has no reason field" { +@test "unregister skips when payload JSON has no reason field (cascade pattern #151)" { + # Same as above, but with valid JSON whose `.reason` is missing. jq -r + # '.reason // empty' returns empty here too; the cascade case matches. "$RADIO" register --role worker-foo --tab w-foo --agent claude local sess="$TASK_FORCE_HOME/radio/sessions/worker-foo.info" TASK_FORCE_ROLE=worker-foo run bash -c "echo '{\"other_field\":\"value\"}' | '$RADIO' unregister" assert_success - assert [ ! -f "$sess" ] + assert [ -f "$sess" ] + run cat "$TASK_FORCE_HOME/radio/log" + assert_output --partial "skipping (reason=" +} + +@test "unregister skips when payload is the literal 'y' observed in #151 logs (cascade pattern)" { + # The smoking-gun payload from #151's PR-#150-diagnostic logs: a single + # "y" character. 144 invocations in 25s, all identical, all reason=. + # Source uncertain (likely a Claude Code subagent termination leaking the + # parent's TUI confirmation keystroke into the SessionEnd hook stdin), but + # the fix is the same regardless of source: skip when reason is unparseable. + "$RADIO" register --role worker-foo --tab w-foo --agent claude --loadout claude-gh + local sess="$TASK_FORCE_HOME/radio/sessions/worker-foo.info" + local loadout_sidecar="$TASK_FORCE_HOME/radio/sessions/worker-foo.loadout" + local agent_sidecar="$TASK_FORCE_HOME/radio/sessions/worker-foo.agent" + assert [ -f "$sess" ] + assert [ -f "$loadout_sidecar" ] + assert [ -f "$agent_sidecar" ] + + # Single non-JSON character on stdin, exactly as the cascade fires it. + TASK_FORCE_ROLE=worker-foo run bash -c "printf 'y' | '$RADIO' unregister" + assert_success + # Session file AND both sidecars must survive — otherwise the next + # _ensure_session_file would write LOADOUT=unknown / AGENT=claude. + assert [ -f "$sess" ] + assert [ -f "$loadout_sidecar" ] + assert [ -f "$agent_sidecar" ] } # ----- register-if-missing (self-healing) ----------------------------------- @@ -510,11 +548,143 @@ teardown() { assert [ -f "$sidecar" ] } +# ----- #151: AGENT sidecar (parallel to LOADOUT) ---------------------------- +# +# Same hook-subshell unset problem _ensure_session_file had for $TASK_FORCE_LOADOUT +# applies to $TASK_FORCE_AGENT. Pre-#151 a kiro-* worker that survived the +# cascade and re-seeded ended up with AGENT=claude (the literal fallback in +# the printf format string), silently flipping its identity. No runtime +# consumer breaks today, but the sidecar is cheap and the alternative is a +# trap for whoever adds AGENT-conditional behaviour next. + +@test "register: writes agent sidecar alongside session file (#151)" { + "$RADIO" register --role worker-foo --tab worker-foo --agent kiro --loadout kiro-gh + local sidecar="$TASK_FORCE_HOME/radio/sessions/worker-foo.agent" + assert [ -f "$sidecar" ] + run cat "$sidecar" + assert_output "kiro" +} + +@test "re-seed after unregister recovers AGENT from sidecar (#151)" { + # Mirror of the LOADOUT recovery test above: register as kiro, wipe the + # .info, re-seed via busy with $TASK_FORCE_AGENT unset (matches the hook + # subshell). Without the sidecar, the printf format string's "${...:-claude}" + # fallback would write AGENT=claude — silently misrepresenting the worker's + # identity. + "$RADIO" register --role worker-foo --tab worker-foo --agent kiro --loadout kiro-gh + local sess="$TASK_FORCE_HOME/radio/sessions/worker-foo.info" + run grep "^AGENT=" "$sess" + assert_output "AGENT=kiro" + + rm -f "$sess" + + TASK_FORCE_ROLE=worker-foo ZELLIJ_TAB=worker-foo run env -u TASK_FORCE_AGENT "$RADIO" busy + assert_success + run grep "^AGENT=" "$sess" + assert_output "AGENT=kiro" + refute_output --partial "claude" +} + +@test "re-seed AGENT falls back to env var when sidecar is absent (#151)" { + # Test-harness escape hatch: directly poke _ensure_session_file without a + # prior register (so no sidecar exists). The env var fallback must still + # work — same shape as the loadout fallback at the same point. + local sess="$TASK_FORCE_HOME/radio/sessions/worker-foo.info" + assert [ ! -f "$sess" ] + + TASK_FORCE_ROLE=worker-foo ZELLIJ_TAB=worker-foo TASK_FORCE_AGENT=kiro run "$RADIO" busy + assert_success + run grep "^AGENT=" "$sess" + assert_output "AGENT=kiro" +} + +@test "re-seed AGENT defaults to 'claude' when no sidecar and no env var (#151)" { + # Last-resort fallback for the no-identity-context case (preserves the + # pre-#151 default so we don't regress non-task-force test paths). + local sess="$TASK_FORCE_HOME/radio/sessions/worker-foo.info" + TASK_FORCE_ROLE=worker-foo ZELLIJ_TAB=worker-foo run env -u TASK_FORCE_AGENT "$RADIO" busy + assert_success + run grep "^AGENT=" "$sess" + assert_output "AGENT=claude" +} + +@test "unregister: sweeps agent sidecar so a stale value can't leak (#151)" { + # Symmetric to the loadout sweep: real-exit unregister must remove the + # agent sidecar too, otherwise a tab reused for a different role would + # inherit the previous AGENT value on its first re-seed. + "$RADIO" register --role worker-foo --tab worker-foo --agent kiro --loadout kiro-gh + local sess="$TASK_FORCE_HOME/radio/sessions/worker-foo.info" + local agent_sidecar="$TASK_FORCE_HOME/radio/sessions/worker-foo.agent" + assert [ -f "$agent_sidecar" ] + + TASK_FORCE_ROLE=worker-foo run bash -c "echo '{\"reason\":\"logout\"}' | '$RADIO' unregister" + assert_success + assert [ ! -f "$sess" ] + assert [ ! -f "$agent_sidecar" ] +} + +@test "unregister: skipped unregister (reason=clear) does NOT sweep agent sidecar (#151)" { + # Symmetric to the loadout no-sweep-on-skip test. If we skipped the .info + # delete, we must skip the sidecar delete too — otherwise the next re-seed + # rebuilds with the wrong AGENT. + "$RADIO" register --role worker-foo --tab worker-foo --agent kiro --loadout kiro-gh + local agent_sidecar="$TASK_FORCE_HOME/radio/sessions/worker-foo.agent" + assert [ -f "$agent_sidecar" ] + + TASK_FORCE_ROLE=worker-foo run bash -c "echo '{\"reason\":\"clear\"}' | '$RADIO' unregister" + assert_success + assert [ -f "$agent_sidecar" ] +} + +@test "unregister: cascade-skipped unregister (reason=) does NOT sweep sidecars (#151)" { + # The new empty-reason skip branch must preserve both sidecars too — same + # invariant as clear/resume. This is the realistic worker-life scenario: + # 144 of these fire in 25s, each must be a no-op. + "$RADIO" register --role worker-foo --tab worker-foo --agent kiro --loadout kiro-gh + local sess="$TASK_FORCE_HOME/radio/sessions/worker-foo.info" + local loadout_sidecar="$TASK_FORCE_HOME/radio/sessions/worker-foo.loadout" + local agent_sidecar="$TASK_FORCE_HOME/radio/sessions/worker-foo.agent" + + TASK_FORCE_ROLE=worker-foo run bash -c "printf 'y' | '$RADIO' unregister" + assert_success + assert [ -f "$sess" ] + assert [ -f "$loadout_sidecar" ] + assert [ -f "$agent_sidecar" ] +} + +@test "end-to-end: cascade burst leaves LOADOUT + AGENT + TAB_ID intact (#151)" { + # Replays the #140/#151 cascade pattern: 20 unregister calls with the "y" + # payload, fired in rapid succession. The session file plus both sidecars + # must still be there at the end, and the values must be the original + # registration values — not the "unknown" / "claude" fallbacks that would + # land if any of the 20 calls had slipped through. + setup_stubs + seed_zellij_tabs worker-foo + export ZELLIJ=fake + + "$RADIO" register --role worker-foo --tab worker-foo --agent kiro --loadout kiro-gh + local sess="$TASK_FORCE_HOME/radio/sessions/worker-foo.info" + + for _ in $(seq 1 20); do + TASK_FORCE_ROLE=worker-foo run bash -c "printf 'y' | '$RADIO' unregister" + assert_success + done + + # Original identity preserved end-to-end. + run grep "^LOADOUT=" "$sess" + assert_output "LOADOUT=kiro-gh" + run grep "^AGENT=" "$sess" + assert_output "AGENT=kiro" + run grep "^TAB_ID=" "$sess" + assert_output "TAB_ID=7" +} + @test "unregister: logs full payload when proceeding past the skip-list (#140 Fix A instrumentation)" { - # When the cascade fires with a reason we don't recognise (or no reason - # at all), we still proceed with the delete — but log the full payload so - # we can investigate what's driving the cascade. Without the payload in - # the log, the cascade is invisible. + # For non-empty reasons we don't explicitly recognise (anything outside + # the clear|resume skip-list and the empty-reason cascade case added in + # #151), we still proceed with the delete — but log the full payload so + # the next time Claude Code adds a real-exit reason we can spot it. The + # empty-reason path now skips (#151) and is exercised by the test below. "$RADIO" register --role worker-foo --tab worker-foo --agent claude TASK_FORCE_ROLE=worker-foo run bash -c "echo '{\"reason\":\"weird_event\",\"extra\":\"data\"}' | '$RADIO' unregister" assert_success @@ -525,13 +695,16 @@ teardown() { assert_output --partial "extra" } -@test "unregister: logs payload even when reason is missing (#140 Fix A instrumentation)" { +@test "unregister: logs payload even when reason is missing (#140 Fix A → #151 skip)" { # The smoking gun in #140 was unregister calls with no parseable reason. - # Make sure we log the payload then too. + # Post-#151 those route to the skip branch (cascade pattern) — but we still + # log the payload, so a future shift in the cascade source can be diagnosed + # without losing the diagnostic surface. "$RADIO" register --role worker-foo --tab worker-foo --agent claude TASK_FORCE_ROLE=worker-foo run bash -c "echo '{\"other\":\"value\"}' | '$RADIO' unregister" assert_success run cat "$TASK_FORCE_HOME/radio/log" - assert_output --partial "unregister: proceeding (reason=)" + assert_output --partial "unregister: skipping (reason=" assert_output --partial "payload=" + assert_output --partial "other" }