From 5cba5989af0ed4da5a3ae1fd3c154fda6a0ee941 Mon Sep 17 00:00:00 2001 From: Martin conur Date: Tue, 2 Jun 2026 10:30:16 -0400 Subject: [PATCH 1/3] =?UTF-8?q?Radio=20session=20corruption:=20unregister?= =?UTF-8?q?=20cascade=20=E2=86=92=20re-seed=20loses=20TAB=5FID=20+=20LOADO?= =?UTF-8?q?UT=20(breaks=20--auto=20wake-up)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Long-running --auto workers degraded their own session file mid-life: after an unregister cascade wiped .info, _ensure_session_file's re-seed wrote TAB_ID= empty (Bug B) and LOADOUT=unknown (Bug C). cmd_send then silently no-op'd on the next PM ping because zellij write-chars had no --tab-id target, and queued radio messages sat unread until a human keypress. Bug B (TAB_ID lost): the rebuild called _zellij_tab_id_by_name with the bare $ZELLIJ_TAB slug, but _rename_tab had already painted the visible tab name to "⏸️ " / "▶️ " / "❓︎ ". Exact-match missed. cmd_register had #117 protection that preserved a prior TAB_ID on miss, but _ensure_session_file didn't. Fix: make _zellij_tab_id_by_name match both the bare slug and the three emoji-prefixed variants — symmetric fix across cmd_register, _ensure_session_file, and _rename_tab. Bug C (LOADOUT lost): _ensure_session_file fell back to "${TASK_FORCE_LOADOUT:-unknown}", but the hook subshell doesn't inherit task-work's launch-time env, so the literal string "unknown" was written on every re-seed. Fix: persist the loadout to a per-role sidecar at cmd_register time (/.loadout); _ensure_session_file reads from sidecar. cmd_unregister sweeps it on real exit; the skip- list (clear|resume) leaves it intact, matching the existing .info semantics. Bug A (the cascade itself): instrument cmd_unregister to log the full hook payload (one-line) when it proceeds past the skip-list. The 20+ unregister calls in 30s aren't normal — capturing the payload gives us the data to investigate what reason (or non-reason) is driving them. Tests: 770/770 passing. 12 new tests in radio_lifecycle.bats cover the emoji-prefixed lookup in cmd_register and _ensure_session_file, the sidecar lifecycle (write, read, sweep, no-sweep on skip), and the new payload-logging instrumentation. The pre-existing #117 preservation test now uses an empty list-tabs fixture (true lookup miss) instead of relying on emoji-prefix-miss as its trigger, since post-#140 the emoji-prefixed lookup succeeds for real. Closes #140 Co-Authored-By: Claude Opus 4.7 (1M context) --- claude-gh/bin/radio | 76 ++++++++++++++-- claude-jira/bin/radio | 76 ++++++++++++++-- claude-local/bin/radio | 76 ++++++++++++++-- claude-notion/bin/radio | 76 ++++++++++++++-- kiro-gh/bin/radio | 76 ++++++++++++++-- kiro-local/bin/radio | 76 ++++++++++++++-- kiro-notion/bin/radio | 76 ++++++++++++++-- tests/radio_lifecycle.bats | 177 ++++++++++++++++++++++++++++++++++--- 8 files changed, 662 insertions(+), 47 deletions(-) diff --git a/claude-gh/bin/radio b/claude-gh/bin/radio index 02cb6bf..d4f0f0d 100755 --- a/claude-gh/bin/radio +++ b/claude-gh/bin/radio @@ -111,13 +111,29 @@ _strip_tab_prefix() { # Resolve a zellij tab's stable id from its visible name. Empty stdout means # "lookup failed" (zellij not reachable, jq absent, name not found) — callers # must treat that as a hard skip so we never operate on the wrong tab (#102). +# +# Emoji-prefix awareness (#140): _rename_tab paints the visible tab name to +# "⏸️ " / "▶️ " / "❓︎ " reflecting STATE. Callers always +# query by the bare slug ($ZELLIJ_TAB / cmd_register's --tab arg) — that's +# the stable identifier captured at pane creation and never mutates. So a +# literal name match against the painted name misses on every flip past +# idle. We compare against both the bare name AND any of the three emoji- +# prefixed variants so the lookup survives _rename_tab's repaints. This is +# the symmetric fix for #117 (cmd_register) AND the same loss in +# _ensure_session_file's re-seed path (Bug B in #140). _zellij_tab_id_by_name() { local target="$1" [[ -n "$target" ]] || return 0 command -v jq >/dev/null 2>&1 || return 0 zellij action list-tabs --json 2>/dev/null \ - | jq -r --arg n "$target" \ - '[.[] | select(.name == $n) | .tab_id] | .[0] // empty' 2>/dev/null + | jq -r --arg n "$target" ' + [.[] | select( + .name == $n + or .name == ("⏸️ " + $n) + or .name == ("▶️ " + $n) + or .name == ("❓︎ " + $n) + ) | .tab_id] | .[0] // empty + ' 2>/dev/null } # Pick a writable pane on the given tab. Prefer the tab's own focused pane @@ -139,6 +155,18 @@ _session_file() { printf '%s/%s.info' "$SESSIONS_DIR" "$1" } +# Sidecar for the per-role loadout name (#140). cmd_register writes it from +# its --loadout CLI arg; _ensure_session_file reads from it during a re-seed. +# Separate from the main session file so it survives cmd_unregister's `rm -f` +# of .info — by definition, we re-seed because the .info file was just +# wiped, so we can't read the previous LOADOUT= line out of it. The bare env +# var ($TASK_FORCE_LOADOUT) isn't available either: hook subshells inherit +# Claude Code's env, not task-work's launch-time exports — that fallback to +# the literal string "unknown" was Bug C in #140. +_loadout_sidecar() { + printf '%s/%s.loadout' "$SESSIONS_DIR" "$1" +} + _inbox_dir() { printf '%s/%s/inbox' "$MAILBOX_DIR" "$1"; } _processed_dir(){ printf '%s/%s/processed' "$MAILBOX_DIR" "$1"; } @@ -163,13 +191,27 @@ _ensure_session_file() { tab_id=$(_zellij_tab_id_by_name "$ZELLIJ_TAB" || true) fi + # LOADOUT recovery (#140 Bug C): the hook subshell doesn't inherit + # $TASK_FORCE_LOADOUT, so the previous fallback to "${TASK_FORCE_LOADOUT:-unknown}" + # wrote the literal string "unknown" on every re-seed. Read from the + # sidecar that cmd_register persisted at register time. Fall back to the + # env var only as a last resort (covers test harnesses that exercise + # _ensure_session_file without going through register first). + local loadout="" + local sidecar + sidecar=$(_loadout_sidecar "$TASK_FORCE_ROLE") + if [[ -f "$sidecar" ]]; then + loadout=$(<"$sidecar") + fi + [[ -n "$loadout" ]] || loadout="${TASK_FORCE_LOADOUT:-unknown}" + 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}" "${TASK_FORCE_LOADOUT:-unknown}" + "${TASK_FORCE_AGENT:-claude}" "$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 @@ -181,7 +223,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" + _log "ensure_session: re-seeded $TASK_FORCE_ROLE (file was missing) tab_id=$tab_id loadout=$loadout" } # Update STATE=… and LAST_HEARTBEAT=… on the current session file. @@ -348,6 +390,16 @@ cmd_register() { } | _atomic_write "$f" mkdir -p "$(_inbox_dir "$role")" "$(_processed_dir "$role")" + + # Persist the loadout name to a sidecar so _ensure_session_file can recover + # it during a mid-life re-seed (#140 Bug C). Skipping on empty values keeps + # the sidecar's 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. + if [[ -n "$loadout" ]]; then + printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" + fi + _log "register role=$role tab=$tab tab_id=$tab_id agent=$agent loadout=$loadout" # First paint of the idle prefix without waiting for a Stop hook cycle (#95). @@ -374,7 +426,7 @@ cmd_unregister() { # in that case. We only filter when both (a) stdin is not a terminal and # (b) a parseable `reason` is present. if [[ ! -t 0 ]]; then - local payload reason + local payload reason payload_oneline payload=$(cat 2>/dev/null || true) if [[ -n "$payload" ]] && command -v jq >/dev/null 2>&1; then reason=$(printf '%s' "$payload" | jq -r '.reason // empty' 2>/dev/null || true) @@ -385,11 +437,25 @@ cmd_unregister() { ;; esac fi + # Instrumentation for #140: when the hook fires with a payload that + # *doesn't* match the skip-list, capture the full payload so we can see + # 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. + 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" + fi fi 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). + rm -f "$(_loadout_sidecar "$TASK_FORCE_ROLE")" _log "unregister role=$TASK_FORCE_ROLE" } diff --git a/claude-jira/bin/radio b/claude-jira/bin/radio index 02cb6bf..d4f0f0d 100755 --- a/claude-jira/bin/radio +++ b/claude-jira/bin/radio @@ -111,13 +111,29 @@ _strip_tab_prefix() { # Resolve a zellij tab's stable id from its visible name. Empty stdout means # "lookup failed" (zellij not reachable, jq absent, name not found) — callers # must treat that as a hard skip so we never operate on the wrong tab (#102). +# +# Emoji-prefix awareness (#140): _rename_tab paints the visible tab name to +# "⏸️ " / "▶️ " / "❓︎ " reflecting STATE. Callers always +# query by the bare slug ($ZELLIJ_TAB / cmd_register's --tab arg) — that's +# the stable identifier captured at pane creation and never mutates. So a +# literal name match against the painted name misses on every flip past +# idle. We compare against both the bare name AND any of the three emoji- +# prefixed variants so the lookup survives _rename_tab's repaints. This is +# the symmetric fix for #117 (cmd_register) AND the same loss in +# _ensure_session_file's re-seed path (Bug B in #140). _zellij_tab_id_by_name() { local target="$1" [[ -n "$target" ]] || return 0 command -v jq >/dev/null 2>&1 || return 0 zellij action list-tabs --json 2>/dev/null \ - | jq -r --arg n "$target" \ - '[.[] | select(.name == $n) | .tab_id] | .[0] // empty' 2>/dev/null + | jq -r --arg n "$target" ' + [.[] | select( + .name == $n + or .name == ("⏸️ " + $n) + or .name == ("▶️ " + $n) + or .name == ("❓︎ " + $n) + ) | .tab_id] | .[0] // empty + ' 2>/dev/null } # Pick a writable pane on the given tab. Prefer the tab's own focused pane @@ -139,6 +155,18 @@ _session_file() { printf '%s/%s.info' "$SESSIONS_DIR" "$1" } +# Sidecar for the per-role loadout name (#140). cmd_register writes it from +# its --loadout CLI arg; _ensure_session_file reads from it during a re-seed. +# Separate from the main session file so it survives cmd_unregister's `rm -f` +# of .info — by definition, we re-seed because the .info file was just +# wiped, so we can't read the previous LOADOUT= line out of it. The bare env +# var ($TASK_FORCE_LOADOUT) isn't available either: hook subshells inherit +# Claude Code's env, not task-work's launch-time exports — that fallback to +# the literal string "unknown" was Bug C in #140. +_loadout_sidecar() { + printf '%s/%s.loadout' "$SESSIONS_DIR" "$1" +} + _inbox_dir() { printf '%s/%s/inbox' "$MAILBOX_DIR" "$1"; } _processed_dir(){ printf '%s/%s/processed' "$MAILBOX_DIR" "$1"; } @@ -163,13 +191,27 @@ _ensure_session_file() { tab_id=$(_zellij_tab_id_by_name "$ZELLIJ_TAB" || true) fi + # LOADOUT recovery (#140 Bug C): the hook subshell doesn't inherit + # $TASK_FORCE_LOADOUT, so the previous fallback to "${TASK_FORCE_LOADOUT:-unknown}" + # wrote the literal string "unknown" on every re-seed. Read from the + # sidecar that cmd_register persisted at register time. Fall back to the + # env var only as a last resort (covers test harnesses that exercise + # _ensure_session_file without going through register first). + local loadout="" + local sidecar + sidecar=$(_loadout_sidecar "$TASK_FORCE_ROLE") + if [[ -f "$sidecar" ]]; then + loadout=$(<"$sidecar") + fi + [[ -n "$loadout" ]] || loadout="${TASK_FORCE_LOADOUT:-unknown}" + 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}" "${TASK_FORCE_LOADOUT:-unknown}" + "${TASK_FORCE_AGENT:-claude}" "$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 @@ -181,7 +223,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" + _log "ensure_session: re-seeded $TASK_FORCE_ROLE (file was missing) tab_id=$tab_id loadout=$loadout" } # Update STATE=… and LAST_HEARTBEAT=… on the current session file. @@ -348,6 +390,16 @@ cmd_register() { } | _atomic_write "$f" mkdir -p "$(_inbox_dir "$role")" "$(_processed_dir "$role")" + + # Persist the loadout name to a sidecar so _ensure_session_file can recover + # it during a mid-life re-seed (#140 Bug C). Skipping on empty values keeps + # the sidecar's 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. + if [[ -n "$loadout" ]]; then + printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" + fi + _log "register role=$role tab=$tab tab_id=$tab_id agent=$agent loadout=$loadout" # First paint of the idle prefix without waiting for a Stop hook cycle (#95). @@ -374,7 +426,7 @@ cmd_unregister() { # in that case. We only filter when both (a) stdin is not a terminal and # (b) a parseable `reason` is present. if [[ ! -t 0 ]]; then - local payload reason + local payload reason payload_oneline payload=$(cat 2>/dev/null || true) if [[ -n "$payload" ]] && command -v jq >/dev/null 2>&1; then reason=$(printf '%s' "$payload" | jq -r '.reason // empty' 2>/dev/null || true) @@ -385,11 +437,25 @@ cmd_unregister() { ;; esac fi + # Instrumentation for #140: when the hook fires with a payload that + # *doesn't* match the skip-list, capture the full payload so we can see + # 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. + 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" + fi fi 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). + rm -f "$(_loadout_sidecar "$TASK_FORCE_ROLE")" _log "unregister role=$TASK_FORCE_ROLE" } diff --git a/claude-local/bin/radio b/claude-local/bin/radio index 02cb6bf..d4f0f0d 100755 --- a/claude-local/bin/radio +++ b/claude-local/bin/radio @@ -111,13 +111,29 @@ _strip_tab_prefix() { # Resolve a zellij tab's stable id from its visible name. Empty stdout means # "lookup failed" (zellij not reachable, jq absent, name not found) — callers # must treat that as a hard skip so we never operate on the wrong tab (#102). +# +# Emoji-prefix awareness (#140): _rename_tab paints the visible tab name to +# "⏸️ " / "▶️ " / "❓︎ " reflecting STATE. Callers always +# query by the bare slug ($ZELLIJ_TAB / cmd_register's --tab arg) — that's +# the stable identifier captured at pane creation and never mutates. So a +# literal name match against the painted name misses on every flip past +# idle. We compare against both the bare name AND any of the three emoji- +# prefixed variants so the lookup survives _rename_tab's repaints. This is +# the symmetric fix for #117 (cmd_register) AND the same loss in +# _ensure_session_file's re-seed path (Bug B in #140). _zellij_tab_id_by_name() { local target="$1" [[ -n "$target" ]] || return 0 command -v jq >/dev/null 2>&1 || return 0 zellij action list-tabs --json 2>/dev/null \ - | jq -r --arg n "$target" \ - '[.[] | select(.name == $n) | .tab_id] | .[0] // empty' 2>/dev/null + | jq -r --arg n "$target" ' + [.[] | select( + .name == $n + or .name == ("⏸️ " + $n) + or .name == ("▶️ " + $n) + or .name == ("❓︎ " + $n) + ) | .tab_id] | .[0] // empty + ' 2>/dev/null } # Pick a writable pane on the given tab. Prefer the tab's own focused pane @@ -139,6 +155,18 @@ _session_file() { printf '%s/%s.info' "$SESSIONS_DIR" "$1" } +# Sidecar for the per-role loadout name (#140). cmd_register writes it from +# its --loadout CLI arg; _ensure_session_file reads from it during a re-seed. +# Separate from the main session file so it survives cmd_unregister's `rm -f` +# of .info — by definition, we re-seed because the .info file was just +# wiped, so we can't read the previous LOADOUT= line out of it. The bare env +# var ($TASK_FORCE_LOADOUT) isn't available either: hook subshells inherit +# Claude Code's env, not task-work's launch-time exports — that fallback to +# the literal string "unknown" was Bug C in #140. +_loadout_sidecar() { + printf '%s/%s.loadout' "$SESSIONS_DIR" "$1" +} + _inbox_dir() { printf '%s/%s/inbox' "$MAILBOX_DIR" "$1"; } _processed_dir(){ printf '%s/%s/processed' "$MAILBOX_DIR" "$1"; } @@ -163,13 +191,27 @@ _ensure_session_file() { tab_id=$(_zellij_tab_id_by_name "$ZELLIJ_TAB" || true) fi + # LOADOUT recovery (#140 Bug C): the hook subshell doesn't inherit + # $TASK_FORCE_LOADOUT, so the previous fallback to "${TASK_FORCE_LOADOUT:-unknown}" + # wrote the literal string "unknown" on every re-seed. Read from the + # sidecar that cmd_register persisted at register time. Fall back to the + # env var only as a last resort (covers test harnesses that exercise + # _ensure_session_file without going through register first). + local loadout="" + local sidecar + sidecar=$(_loadout_sidecar "$TASK_FORCE_ROLE") + if [[ -f "$sidecar" ]]; then + loadout=$(<"$sidecar") + fi + [[ -n "$loadout" ]] || loadout="${TASK_FORCE_LOADOUT:-unknown}" + 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}" "${TASK_FORCE_LOADOUT:-unknown}" + "${TASK_FORCE_AGENT:-claude}" "$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 @@ -181,7 +223,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" + _log "ensure_session: re-seeded $TASK_FORCE_ROLE (file was missing) tab_id=$tab_id loadout=$loadout" } # Update STATE=… and LAST_HEARTBEAT=… on the current session file. @@ -348,6 +390,16 @@ cmd_register() { } | _atomic_write "$f" mkdir -p "$(_inbox_dir "$role")" "$(_processed_dir "$role")" + + # Persist the loadout name to a sidecar so _ensure_session_file can recover + # it during a mid-life re-seed (#140 Bug C). Skipping on empty values keeps + # the sidecar's 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. + if [[ -n "$loadout" ]]; then + printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" + fi + _log "register role=$role tab=$tab tab_id=$tab_id agent=$agent loadout=$loadout" # First paint of the idle prefix without waiting for a Stop hook cycle (#95). @@ -374,7 +426,7 @@ cmd_unregister() { # in that case. We only filter when both (a) stdin is not a terminal and # (b) a parseable `reason` is present. if [[ ! -t 0 ]]; then - local payload reason + local payload reason payload_oneline payload=$(cat 2>/dev/null || true) if [[ -n "$payload" ]] && command -v jq >/dev/null 2>&1; then reason=$(printf '%s' "$payload" | jq -r '.reason // empty' 2>/dev/null || true) @@ -385,11 +437,25 @@ cmd_unregister() { ;; esac fi + # Instrumentation for #140: when the hook fires with a payload that + # *doesn't* match the skip-list, capture the full payload so we can see + # 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. + 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" + fi fi 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). + rm -f "$(_loadout_sidecar "$TASK_FORCE_ROLE")" _log "unregister role=$TASK_FORCE_ROLE" } diff --git a/claude-notion/bin/radio b/claude-notion/bin/radio index 02cb6bf..d4f0f0d 100755 --- a/claude-notion/bin/radio +++ b/claude-notion/bin/radio @@ -111,13 +111,29 @@ _strip_tab_prefix() { # Resolve a zellij tab's stable id from its visible name. Empty stdout means # "lookup failed" (zellij not reachable, jq absent, name not found) — callers # must treat that as a hard skip so we never operate on the wrong tab (#102). +# +# Emoji-prefix awareness (#140): _rename_tab paints the visible tab name to +# "⏸️ " / "▶️ " / "❓︎ " reflecting STATE. Callers always +# query by the bare slug ($ZELLIJ_TAB / cmd_register's --tab arg) — that's +# the stable identifier captured at pane creation and never mutates. So a +# literal name match against the painted name misses on every flip past +# idle. We compare against both the bare name AND any of the three emoji- +# prefixed variants so the lookup survives _rename_tab's repaints. This is +# the symmetric fix for #117 (cmd_register) AND the same loss in +# _ensure_session_file's re-seed path (Bug B in #140). _zellij_tab_id_by_name() { local target="$1" [[ -n "$target" ]] || return 0 command -v jq >/dev/null 2>&1 || return 0 zellij action list-tabs --json 2>/dev/null \ - | jq -r --arg n "$target" \ - '[.[] | select(.name == $n) | .tab_id] | .[0] // empty' 2>/dev/null + | jq -r --arg n "$target" ' + [.[] | select( + .name == $n + or .name == ("⏸️ " + $n) + or .name == ("▶️ " + $n) + or .name == ("❓︎ " + $n) + ) | .tab_id] | .[0] // empty + ' 2>/dev/null } # Pick a writable pane on the given tab. Prefer the tab's own focused pane @@ -139,6 +155,18 @@ _session_file() { printf '%s/%s.info' "$SESSIONS_DIR" "$1" } +# Sidecar for the per-role loadout name (#140). cmd_register writes it from +# its --loadout CLI arg; _ensure_session_file reads from it during a re-seed. +# Separate from the main session file so it survives cmd_unregister's `rm -f` +# of .info — by definition, we re-seed because the .info file was just +# wiped, so we can't read the previous LOADOUT= line out of it. The bare env +# var ($TASK_FORCE_LOADOUT) isn't available either: hook subshells inherit +# Claude Code's env, not task-work's launch-time exports — that fallback to +# the literal string "unknown" was Bug C in #140. +_loadout_sidecar() { + printf '%s/%s.loadout' "$SESSIONS_DIR" "$1" +} + _inbox_dir() { printf '%s/%s/inbox' "$MAILBOX_DIR" "$1"; } _processed_dir(){ printf '%s/%s/processed' "$MAILBOX_DIR" "$1"; } @@ -163,13 +191,27 @@ _ensure_session_file() { tab_id=$(_zellij_tab_id_by_name "$ZELLIJ_TAB" || true) fi + # LOADOUT recovery (#140 Bug C): the hook subshell doesn't inherit + # $TASK_FORCE_LOADOUT, so the previous fallback to "${TASK_FORCE_LOADOUT:-unknown}" + # wrote the literal string "unknown" on every re-seed. Read from the + # sidecar that cmd_register persisted at register time. Fall back to the + # env var only as a last resort (covers test harnesses that exercise + # _ensure_session_file without going through register first). + local loadout="" + local sidecar + sidecar=$(_loadout_sidecar "$TASK_FORCE_ROLE") + if [[ -f "$sidecar" ]]; then + loadout=$(<"$sidecar") + fi + [[ -n "$loadout" ]] || loadout="${TASK_FORCE_LOADOUT:-unknown}" + 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}" "${TASK_FORCE_LOADOUT:-unknown}" + "${TASK_FORCE_AGENT:-claude}" "$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 @@ -181,7 +223,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" + _log "ensure_session: re-seeded $TASK_FORCE_ROLE (file was missing) tab_id=$tab_id loadout=$loadout" } # Update STATE=… and LAST_HEARTBEAT=… on the current session file. @@ -348,6 +390,16 @@ cmd_register() { } | _atomic_write "$f" mkdir -p "$(_inbox_dir "$role")" "$(_processed_dir "$role")" + + # Persist the loadout name to a sidecar so _ensure_session_file can recover + # it during a mid-life re-seed (#140 Bug C). Skipping on empty values keeps + # the sidecar's 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. + if [[ -n "$loadout" ]]; then + printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" + fi + _log "register role=$role tab=$tab tab_id=$tab_id agent=$agent loadout=$loadout" # First paint of the idle prefix without waiting for a Stop hook cycle (#95). @@ -374,7 +426,7 @@ cmd_unregister() { # in that case. We only filter when both (a) stdin is not a terminal and # (b) a parseable `reason` is present. if [[ ! -t 0 ]]; then - local payload reason + local payload reason payload_oneline payload=$(cat 2>/dev/null || true) if [[ -n "$payload" ]] && command -v jq >/dev/null 2>&1; then reason=$(printf '%s' "$payload" | jq -r '.reason // empty' 2>/dev/null || true) @@ -385,11 +437,25 @@ cmd_unregister() { ;; esac fi + # Instrumentation for #140: when the hook fires with a payload that + # *doesn't* match the skip-list, capture the full payload so we can see + # 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. + 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" + fi fi 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). + rm -f "$(_loadout_sidecar "$TASK_FORCE_ROLE")" _log "unregister role=$TASK_FORCE_ROLE" } diff --git a/kiro-gh/bin/radio b/kiro-gh/bin/radio index 02cb6bf..d4f0f0d 100755 --- a/kiro-gh/bin/radio +++ b/kiro-gh/bin/radio @@ -111,13 +111,29 @@ _strip_tab_prefix() { # Resolve a zellij tab's stable id from its visible name. Empty stdout means # "lookup failed" (zellij not reachable, jq absent, name not found) — callers # must treat that as a hard skip so we never operate on the wrong tab (#102). +# +# Emoji-prefix awareness (#140): _rename_tab paints the visible tab name to +# "⏸️ " / "▶️ " / "❓︎ " reflecting STATE. Callers always +# query by the bare slug ($ZELLIJ_TAB / cmd_register's --tab arg) — that's +# the stable identifier captured at pane creation and never mutates. So a +# literal name match against the painted name misses on every flip past +# idle. We compare against both the bare name AND any of the three emoji- +# prefixed variants so the lookup survives _rename_tab's repaints. This is +# the symmetric fix for #117 (cmd_register) AND the same loss in +# _ensure_session_file's re-seed path (Bug B in #140). _zellij_tab_id_by_name() { local target="$1" [[ -n "$target" ]] || return 0 command -v jq >/dev/null 2>&1 || return 0 zellij action list-tabs --json 2>/dev/null \ - | jq -r --arg n "$target" \ - '[.[] | select(.name == $n) | .tab_id] | .[0] // empty' 2>/dev/null + | jq -r --arg n "$target" ' + [.[] | select( + .name == $n + or .name == ("⏸️ " + $n) + or .name == ("▶️ " + $n) + or .name == ("❓︎ " + $n) + ) | .tab_id] | .[0] // empty + ' 2>/dev/null } # Pick a writable pane on the given tab. Prefer the tab's own focused pane @@ -139,6 +155,18 @@ _session_file() { printf '%s/%s.info' "$SESSIONS_DIR" "$1" } +# Sidecar for the per-role loadout name (#140). cmd_register writes it from +# its --loadout CLI arg; _ensure_session_file reads from it during a re-seed. +# Separate from the main session file so it survives cmd_unregister's `rm -f` +# of .info — by definition, we re-seed because the .info file was just +# wiped, so we can't read the previous LOADOUT= line out of it. The bare env +# var ($TASK_FORCE_LOADOUT) isn't available either: hook subshells inherit +# Claude Code's env, not task-work's launch-time exports — that fallback to +# the literal string "unknown" was Bug C in #140. +_loadout_sidecar() { + printf '%s/%s.loadout' "$SESSIONS_DIR" "$1" +} + _inbox_dir() { printf '%s/%s/inbox' "$MAILBOX_DIR" "$1"; } _processed_dir(){ printf '%s/%s/processed' "$MAILBOX_DIR" "$1"; } @@ -163,13 +191,27 @@ _ensure_session_file() { tab_id=$(_zellij_tab_id_by_name "$ZELLIJ_TAB" || true) fi + # LOADOUT recovery (#140 Bug C): the hook subshell doesn't inherit + # $TASK_FORCE_LOADOUT, so the previous fallback to "${TASK_FORCE_LOADOUT:-unknown}" + # wrote the literal string "unknown" on every re-seed. Read from the + # sidecar that cmd_register persisted at register time. Fall back to the + # env var only as a last resort (covers test harnesses that exercise + # _ensure_session_file without going through register first). + local loadout="" + local sidecar + sidecar=$(_loadout_sidecar "$TASK_FORCE_ROLE") + if [[ -f "$sidecar" ]]; then + loadout=$(<"$sidecar") + fi + [[ -n "$loadout" ]] || loadout="${TASK_FORCE_LOADOUT:-unknown}" + 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}" "${TASK_FORCE_LOADOUT:-unknown}" + "${TASK_FORCE_AGENT:-claude}" "$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 @@ -181,7 +223,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" + _log "ensure_session: re-seeded $TASK_FORCE_ROLE (file was missing) tab_id=$tab_id loadout=$loadout" } # Update STATE=… and LAST_HEARTBEAT=… on the current session file. @@ -348,6 +390,16 @@ cmd_register() { } | _atomic_write "$f" mkdir -p "$(_inbox_dir "$role")" "$(_processed_dir "$role")" + + # Persist the loadout name to a sidecar so _ensure_session_file can recover + # it during a mid-life re-seed (#140 Bug C). Skipping on empty values keeps + # the sidecar's 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. + if [[ -n "$loadout" ]]; then + printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" + fi + _log "register role=$role tab=$tab tab_id=$tab_id agent=$agent loadout=$loadout" # First paint of the idle prefix without waiting for a Stop hook cycle (#95). @@ -374,7 +426,7 @@ cmd_unregister() { # in that case. We only filter when both (a) stdin is not a terminal and # (b) a parseable `reason` is present. if [[ ! -t 0 ]]; then - local payload reason + local payload reason payload_oneline payload=$(cat 2>/dev/null || true) if [[ -n "$payload" ]] && command -v jq >/dev/null 2>&1; then reason=$(printf '%s' "$payload" | jq -r '.reason // empty' 2>/dev/null || true) @@ -385,11 +437,25 @@ cmd_unregister() { ;; esac fi + # Instrumentation for #140: when the hook fires with a payload that + # *doesn't* match the skip-list, capture the full payload so we can see + # 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. + 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" + fi fi 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). + rm -f "$(_loadout_sidecar "$TASK_FORCE_ROLE")" _log "unregister role=$TASK_FORCE_ROLE" } diff --git a/kiro-local/bin/radio b/kiro-local/bin/radio index 02cb6bf..d4f0f0d 100755 --- a/kiro-local/bin/radio +++ b/kiro-local/bin/radio @@ -111,13 +111,29 @@ _strip_tab_prefix() { # Resolve a zellij tab's stable id from its visible name. Empty stdout means # "lookup failed" (zellij not reachable, jq absent, name not found) — callers # must treat that as a hard skip so we never operate on the wrong tab (#102). +# +# Emoji-prefix awareness (#140): _rename_tab paints the visible tab name to +# "⏸️ " / "▶️ " / "❓︎ " reflecting STATE. Callers always +# query by the bare slug ($ZELLIJ_TAB / cmd_register's --tab arg) — that's +# the stable identifier captured at pane creation and never mutates. So a +# literal name match against the painted name misses on every flip past +# idle. We compare against both the bare name AND any of the three emoji- +# prefixed variants so the lookup survives _rename_tab's repaints. This is +# the symmetric fix for #117 (cmd_register) AND the same loss in +# _ensure_session_file's re-seed path (Bug B in #140). _zellij_tab_id_by_name() { local target="$1" [[ -n "$target" ]] || return 0 command -v jq >/dev/null 2>&1 || return 0 zellij action list-tabs --json 2>/dev/null \ - | jq -r --arg n "$target" \ - '[.[] | select(.name == $n) | .tab_id] | .[0] // empty' 2>/dev/null + | jq -r --arg n "$target" ' + [.[] | select( + .name == $n + or .name == ("⏸️ " + $n) + or .name == ("▶️ " + $n) + or .name == ("❓︎ " + $n) + ) | .tab_id] | .[0] // empty + ' 2>/dev/null } # Pick a writable pane on the given tab. Prefer the tab's own focused pane @@ -139,6 +155,18 @@ _session_file() { printf '%s/%s.info' "$SESSIONS_DIR" "$1" } +# Sidecar for the per-role loadout name (#140). cmd_register writes it from +# its --loadout CLI arg; _ensure_session_file reads from it during a re-seed. +# Separate from the main session file so it survives cmd_unregister's `rm -f` +# of .info — by definition, we re-seed because the .info file was just +# wiped, so we can't read the previous LOADOUT= line out of it. The bare env +# var ($TASK_FORCE_LOADOUT) isn't available either: hook subshells inherit +# Claude Code's env, not task-work's launch-time exports — that fallback to +# the literal string "unknown" was Bug C in #140. +_loadout_sidecar() { + printf '%s/%s.loadout' "$SESSIONS_DIR" "$1" +} + _inbox_dir() { printf '%s/%s/inbox' "$MAILBOX_DIR" "$1"; } _processed_dir(){ printf '%s/%s/processed' "$MAILBOX_DIR" "$1"; } @@ -163,13 +191,27 @@ _ensure_session_file() { tab_id=$(_zellij_tab_id_by_name "$ZELLIJ_TAB" || true) fi + # LOADOUT recovery (#140 Bug C): the hook subshell doesn't inherit + # $TASK_FORCE_LOADOUT, so the previous fallback to "${TASK_FORCE_LOADOUT:-unknown}" + # wrote the literal string "unknown" on every re-seed. Read from the + # sidecar that cmd_register persisted at register time. Fall back to the + # env var only as a last resort (covers test harnesses that exercise + # _ensure_session_file without going through register first). + local loadout="" + local sidecar + sidecar=$(_loadout_sidecar "$TASK_FORCE_ROLE") + if [[ -f "$sidecar" ]]; then + loadout=$(<"$sidecar") + fi + [[ -n "$loadout" ]] || loadout="${TASK_FORCE_LOADOUT:-unknown}" + 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}" "${TASK_FORCE_LOADOUT:-unknown}" + "${TASK_FORCE_AGENT:-claude}" "$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 @@ -181,7 +223,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" + _log "ensure_session: re-seeded $TASK_FORCE_ROLE (file was missing) tab_id=$tab_id loadout=$loadout" } # Update STATE=… and LAST_HEARTBEAT=… on the current session file. @@ -348,6 +390,16 @@ cmd_register() { } | _atomic_write "$f" mkdir -p "$(_inbox_dir "$role")" "$(_processed_dir "$role")" + + # Persist the loadout name to a sidecar so _ensure_session_file can recover + # it during a mid-life re-seed (#140 Bug C). Skipping on empty values keeps + # the sidecar's 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. + if [[ -n "$loadout" ]]; then + printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" + fi + _log "register role=$role tab=$tab tab_id=$tab_id agent=$agent loadout=$loadout" # First paint of the idle prefix without waiting for a Stop hook cycle (#95). @@ -374,7 +426,7 @@ cmd_unregister() { # in that case. We only filter when both (a) stdin is not a terminal and # (b) a parseable `reason` is present. if [[ ! -t 0 ]]; then - local payload reason + local payload reason payload_oneline payload=$(cat 2>/dev/null || true) if [[ -n "$payload" ]] && command -v jq >/dev/null 2>&1; then reason=$(printf '%s' "$payload" | jq -r '.reason // empty' 2>/dev/null || true) @@ -385,11 +437,25 @@ cmd_unregister() { ;; esac fi + # Instrumentation for #140: when the hook fires with a payload that + # *doesn't* match the skip-list, capture the full payload so we can see + # 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. + 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" + fi fi 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). + rm -f "$(_loadout_sidecar "$TASK_FORCE_ROLE")" _log "unregister role=$TASK_FORCE_ROLE" } diff --git a/kiro-notion/bin/radio b/kiro-notion/bin/radio index 02cb6bf..d4f0f0d 100755 --- a/kiro-notion/bin/radio +++ b/kiro-notion/bin/radio @@ -111,13 +111,29 @@ _strip_tab_prefix() { # Resolve a zellij tab's stable id from its visible name. Empty stdout means # "lookup failed" (zellij not reachable, jq absent, name not found) — callers # must treat that as a hard skip so we never operate on the wrong tab (#102). +# +# Emoji-prefix awareness (#140): _rename_tab paints the visible tab name to +# "⏸️ " / "▶️ " / "❓︎ " reflecting STATE. Callers always +# query by the bare slug ($ZELLIJ_TAB / cmd_register's --tab arg) — that's +# the stable identifier captured at pane creation and never mutates. So a +# literal name match against the painted name misses on every flip past +# idle. We compare against both the bare name AND any of the three emoji- +# prefixed variants so the lookup survives _rename_tab's repaints. This is +# the symmetric fix for #117 (cmd_register) AND the same loss in +# _ensure_session_file's re-seed path (Bug B in #140). _zellij_tab_id_by_name() { local target="$1" [[ -n "$target" ]] || return 0 command -v jq >/dev/null 2>&1 || return 0 zellij action list-tabs --json 2>/dev/null \ - | jq -r --arg n "$target" \ - '[.[] | select(.name == $n) | .tab_id] | .[0] // empty' 2>/dev/null + | jq -r --arg n "$target" ' + [.[] | select( + .name == $n + or .name == ("⏸️ " + $n) + or .name == ("▶️ " + $n) + or .name == ("❓︎ " + $n) + ) | .tab_id] | .[0] // empty + ' 2>/dev/null } # Pick a writable pane on the given tab. Prefer the tab's own focused pane @@ -139,6 +155,18 @@ _session_file() { printf '%s/%s.info' "$SESSIONS_DIR" "$1" } +# Sidecar for the per-role loadout name (#140). cmd_register writes it from +# its --loadout CLI arg; _ensure_session_file reads from it during a re-seed. +# Separate from the main session file so it survives cmd_unregister's `rm -f` +# of .info — by definition, we re-seed because the .info file was just +# wiped, so we can't read the previous LOADOUT= line out of it. The bare env +# var ($TASK_FORCE_LOADOUT) isn't available either: hook subshells inherit +# Claude Code's env, not task-work's launch-time exports — that fallback to +# the literal string "unknown" was Bug C in #140. +_loadout_sidecar() { + printf '%s/%s.loadout' "$SESSIONS_DIR" "$1" +} + _inbox_dir() { printf '%s/%s/inbox' "$MAILBOX_DIR" "$1"; } _processed_dir(){ printf '%s/%s/processed' "$MAILBOX_DIR" "$1"; } @@ -163,13 +191,27 @@ _ensure_session_file() { tab_id=$(_zellij_tab_id_by_name "$ZELLIJ_TAB" || true) fi + # LOADOUT recovery (#140 Bug C): the hook subshell doesn't inherit + # $TASK_FORCE_LOADOUT, so the previous fallback to "${TASK_FORCE_LOADOUT:-unknown}" + # wrote the literal string "unknown" on every re-seed. Read from the + # sidecar that cmd_register persisted at register time. Fall back to the + # env var only as a last resort (covers test harnesses that exercise + # _ensure_session_file without going through register first). + local loadout="" + local sidecar + sidecar=$(_loadout_sidecar "$TASK_FORCE_ROLE") + if [[ -f "$sidecar" ]]; then + loadout=$(<"$sidecar") + fi + [[ -n "$loadout" ]] || loadout="${TASK_FORCE_LOADOUT:-unknown}" + 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}" "${TASK_FORCE_LOADOUT:-unknown}" + "${TASK_FORCE_AGENT:-claude}" "$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 @@ -181,7 +223,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" + _log "ensure_session: re-seeded $TASK_FORCE_ROLE (file was missing) tab_id=$tab_id loadout=$loadout" } # Update STATE=… and LAST_HEARTBEAT=… on the current session file. @@ -348,6 +390,16 @@ cmd_register() { } | _atomic_write "$f" mkdir -p "$(_inbox_dir "$role")" "$(_processed_dir "$role")" + + # Persist the loadout name to a sidecar so _ensure_session_file can recover + # it during a mid-life re-seed (#140 Bug C). Skipping on empty values keeps + # the sidecar's 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. + if [[ -n "$loadout" ]]; then + printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" + fi + _log "register role=$role tab=$tab tab_id=$tab_id agent=$agent loadout=$loadout" # First paint of the idle prefix without waiting for a Stop hook cycle (#95). @@ -374,7 +426,7 @@ cmd_unregister() { # in that case. We only filter when both (a) stdin is not a terminal and # (b) a parseable `reason` is present. if [[ ! -t 0 ]]; then - local payload reason + local payload reason payload_oneline payload=$(cat 2>/dev/null || true) if [[ -n "$payload" ]] && command -v jq >/dev/null 2>&1; then reason=$(printf '%s' "$payload" | jq -r '.reason // empty' 2>/dev/null || true) @@ -385,11 +437,25 @@ cmd_unregister() { ;; esac fi + # Instrumentation for #140: when the hook fires with a payload that + # *doesn't* match the skip-list, capture the full payload so we can see + # 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. + 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" + fi fi 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). + rm -f "$(_loadout_sidecar "$TASK_FORCE_ROLE")" _log "unregister role=$TASK_FORCE_ROLE" } diff --git a/tests/radio_lifecycle.bats b/tests/radio_lifecycle.bats index 0110273..89a9d45 100644 --- a/tests/radio_lifecycle.bats +++ b/tests/radio_lifecycle.bats @@ -204,19 +204,50 @@ teardown() { assert_output --partial "TAB_ID=7" } -# ----- re-register preserves TAB_ID on zellij lookup miss (#117) ------------ +# ----- re-register preserves TAB_ID on zellij lookup miss (#117, #140) ------ # # The #117 failure mode: a worker's $ZELLIJ_TAB env var sticks to the bare # slug captured at pane creation, but the visible tab name has since been # painted to "⏸️ " / "▶️ " by _rename_tab. When SessionStart # fires a second time (because of /compact, /clear, /resume, or a fresh # `claude` in the same tab), the hook re-runs `radio register --tab $ZELLIJ_TAB` -# with the bare slug and the literal-name lookup in zellij misses. Pre-fix, -# we'd blindly rewrite TAB_ID= to empty — killing _rename_tab and cmd_send -# for the rest of the worker's life, and finally leaking the tab when -# task-done read the now-empty TAB_ID. +# with the bare slug. Pre-#140, the literal-name lookup missed and we relied +# on the in-file preservation fallback (which only works for register, not +# for _ensure_session_file). Post-#140, _zellij_tab_id_by_name itself matches +# both the bare slug and the emoji-prefixed variants — so the lookup +# succeeds for real, and the preservation path only fires when zellij is +# genuinely unreachable. + +@test "register: lookup resolves through emoji-prefixed visible name (#140 Fix B)" { + setup_stubs + export ZELLIJ=fake + # Only the painted name is advertised — no bare-slug entry. This is the + # actual state of zellij after _rename_tab has flipped the tab to "▶️". + export STUB_ZELLIJ_TABS_JSON='[{"name":"▶️ worker-foo","tab_id":7}]' + export STUB_ZELLIJ_PANES_JSON='[{"id":700,"is_plugin":false,"is_focused":true,"tab_id":7}]' + + "$RADIO" register --role worker-foo --tab worker-foo --agent claude + local sess="$TASK_FORCE_HOME/radio/sessions/worker-foo.info" + run grep "^TAB_ID=" "$sess" + assert_output "TAB_ID=7" + run cat "$TASK_FORCE_HOME/radio/log" + # Direct resolution — no "preserving" path needed. + refute_output --partial "could not resolve tab_id for tab=worker-foo" +} -@test "register: re-register preserves existing TAB_ID when zellij list-tabs misses (#117)" { +@test "register: lookup resolves through idle ⏸️ prefixed visible name (#140 Fix B)" { + setup_stubs + export ZELLIJ=fake + export STUB_ZELLIJ_TABS_JSON='[{"name":"⏸️ worker-foo","tab_id":7}]' + export STUB_ZELLIJ_PANES_JSON='[{"id":700,"is_plugin":false,"is_focused":true,"tab_id":7}]' + + "$RADIO" register --role worker-foo --tab worker-foo --agent claude + local sess="$TASK_FORCE_HOME/radio/sessions/worker-foo.info" + run grep "^TAB_ID=" "$sess" + assert_output "TAB_ID=7" +} + +@test "register: re-register preserves existing TAB_ID when zellij is truly unreachable (#117)" { setup_stubs seed_zellij_tabs worker-foo export ZELLIJ=fake @@ -227,11 +258,11 @@ teardown() { run grep "^TAB_ID=" "$sess" assert_output --partial "TAB_ID=7" - # Now simulate the second SessionStart: the visible tab has been painted - # to "▶️ worker-foo", so the bare-name lookup misses. Re-seed the stub - # fixture so only the prefixed entry is advertised. - export STUB_ZELLIJ_TABS_JSON='[{"name":"▶️ worker-foo","tab_id":7}]' - export STUB_ZELLIJ_PANES_JSON='[{"id":700,"is_plugin":false,"is_focused":true,"tab_id":7}]' + # Now simulate zellij being completely unreachable for the second + # register: empty list-tabs JSON (zellij died, jq misparsed, anything that + # leaves the lookup with zero matches). The #117 in-file preservation + # path must still kick in. + export STUB_ZELLIJ_TABS_JSON='[]' "$RADIO" register --role worker-foo --tab worker-foo --agent claude # TAB_ID must still be 7 — preserved, not clobbered to empty. @@ -239,7 +270,6 @@ teardown() { assert_output "TAB_ID=7" run cat "$TASK_FORCE_HOME/radio/log" assert_output --partial "preserving existing TAB_ID=7" - refute_output --partial "could not resolve tab_id for tab=worker-foo" } @test "register: first-time lookup miss with no existing file still logs the resolve-failure (#117)" { @@ -277,3 +307,126 @@ teardown() { assert_output --partial "could not resolve tab_id for tab=worker-empty" refute_output --partial "preserving existing TAB_ID" } + +# ----- #140: re-seed after unregister cascade preserves TAB_ID + LOADOUT ---- +# +# Reproduces the corruption documented in #140: an unregister cascade wipes +# the session file mid-life; _ensure_session_file re-seeds when the next +# busy/ready hook fires. Before #140, the re-seed lost two fields: +# - TAB_ID: empty, because _zellij_tab_id_by_name did exact-match against +# the painted (emoji-prefixed) tab name and missed. +# - LOADOUT: literal "unknown", because $TASK_FORCE_LOADOUT isn't set in +# Claude Code's hook subshell (task-work exports it at launch time only). +# +# Combined effect: cmd_send couldn't wake the worker (no TAB_ID for the +# zellij write-chars target), so --auto workers stopped being auto. + +@test "re-seed after unregister recovers TAB_ID through emoji-prefixed tab name (#140 Bug B)" { + setup_stubs + seed_zellij_tabs worker-foo + export ZELLIJ=fake + + # Initial register: file has TAB_ID=7, LOADOUT=claude-gh. + "$RADIO" register --role worker-foo --tab worker-foo --agent claude --loadout claude-gh + local sess="$TASK_FORCE_HOME/radio/sessions/worker-foo.info" + run grep "^TAB_ID=" "$sess" + assert_output "TAB_ID=7" + + # Cascade wipe — the same unregister storm reproduced in #140's log. + rm -f "$sess" + + # By now the visible tab is painted "▶️ worker-foo" by _rename_tab. Re-seed + # the stub fixture so only the painted variant is advertised. Pre-#140 + # the lookup would miss and TAB_ID= would be written empty. + export STUB_ZELLIJ_TABS_JSON='[{"name":"▶️ worker-foo","tab_id":7}]' + export STUB_ZELLIJ_PANES_JSON='[{"id":700,"is_plugin":false,"is_focused":true,"tab_id":7}]' + + TASK_FORCE_ROLE=worker-foo ZELLIJ_TAB=worker-foo run "$RADIO" busy + assert_success + run grep "^TAB_ID=" "$sess" + assert_output "TAB_ID=7" +} + +@test "re-seed after unregister recovers LOADOUT from sidecar (#140 Bug C)" { + # _ensure_session_file's previous fallback "${TASK_FORCE_LOADOUT:-unknown}" + # wrote the literal "unknown" because hook subshells don't inherit + # task-work's launch-time env. The sidecar persists the loadout name + # alongside the session file at register time so the re-seed can recover. + "$RADIO" register --role worker-foo --tab worker-foo --agent claude --loadout claude-gh + local sess="$TASK_FORCE_HOME/radio/sessions/worker-foo.info" + run grep "^LOADOUT=" "$sess" + assert_output "LOADOUT=claude-gh" + + # Cascade wipe. + rm -f "$sess" + + # Re-seed via busy hook — note: $TASK_FORCE_LOADOUT is *not* exported (it + # wouldn't be, in Claude Code's hook subshell). The sidecar is the only + # available source for the loadout name. + TASK_FORCE_ROLE=worker-foo ZELLIJ_TAB=worker-foo run env -u TASK_FORCE_LOADOUT "$RADIO" busy + assert_success + run grep "^LOADOUT=" "$sess" + assert_output "LOADOUT=claude-gh" + refute_output --partial "unknown" +} + +@test "register: writes loadout sidecar alongside session file (#140 Bug C)" { + "$RADIO" register --role worker-foo --tab worker-foo --agent claude --loadout claude-gh + local sidecar="$TASK_FORCE_HOME/radio/sessions/worker-foo.loadout" + assert [ -f "$sidecar" ] + run cat "$sidecar" + assert_output "claude-gh" +} + +@test "unregister: sweeps loadout sidecar so a stale value can't leak to the next role lifecycle (#140)" { + "$RADIO" register --role worker-foo --tab worker-foo --agent claude --loadout claude-gh + local sess="$TASK_FORCE_HOME/radio/sessions/worker-foo.info" + local sidecar="$TASK_FORCE_HOME/radio/sessions/worker-foo.loadout" + assert [ -f "$sidecar" ] + + # Real exit unregister (reason=logout slips past the skip-list, file + # deleted, sidecar deleted). + TASK_FORCE_ROLE=worker-foo run bash -c "echo '{\"reason\":\"logout\"}' | '$RADIO' unregister" + assert_success + assert [ ! -f "$sess" ] + assert [ ! -f "$sidecar" ] +} + +@test "unregister: skipped unregister (reason=clear) does NOT sweep loadout sidecar (#140)" { + # The skip-list applies symmetrically to both: if we don't delete .info, + # we mustn't delete .loadout either. Otherwise a subsequent re-seed would + # write LOADOUT=unknown despite the unregister having been a no-op. + "$RADIO" register --role worker-foo --tab worker-foo --agent claude --loadout claude-gh + local sidecar="$TASK_FORCE_HOME/radio/sessions/worker-foo.loadout" + assert [ -f "$sidecar" ] + + TASK_FORCE_ROLE=worker-foo run bash -c "echo '{\"reason\":\"clear\"}' | '$RADIO' unregister" + assert_success + assert [ -f "$sidecar" ] +} + +@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. + "$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 + run cat "$TASK_FORCE_HOME/radio/log" + assert_output --partial "unregister: proceeding (reason=weird_event)" + assert_output --partial "payload=" + assert_output --partial "weird_event" + assert_output --partial "extra" +} + +@test "unregister: logs payload even when reason is missing (#140 Fix A instrumentation)" { + # The smoking gun in #140 was unregister calls with no parseable reason. + # Make sure we log the payload then too. + "$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 "payload=" +} From c2c6c44c9a2697114a45cd604c493c2e4767948f Mon Sep 17 00:00:00 2001 From: Martin conur Date: Tue, 2 Jun 2026 14:30:50 -0400 Subject: [PATCH 2/3] =?UTF-8?q?Radio=20session=20corruption:=20PR=20#150?= =?UTF-8?q?=20review=20round=202=20=E2=80=94=20TOCTOU=20guard=20+=20lib=20?= =?UTF-8?q?emoji=20sync?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two reviewer-flagged pre-merge fixes: 1. TOCTOU on _ensure_session_file's sidecar read (#150 review finding 3): the sidecar can be unlinked by a concurrent cmd_unregister between the `[[ -f ]]` check and the read — exactly the cascade race documented in #140. set -e would abort _ensure_session_file and STATE tracking would die for the worker's lifetime. Switch the read to `loadout=$(cat "$sidecar" 2>/dev/null || true)` which collapses "file absent / unlinked mid-read / unreadable" into "empty string" without tripping set -e. Env-var fallback below handles the empty case. Notable: the reviewer's literal suggestion `loadout=$(<"$sidecar" || true)` does NOT work — bash's ` --- claude-gh/bin/radio | 17 +++++++-- claude-jira/bin/radio | 17 +++++++-- claude-local/bin/radio | 17 +++++++-- claude-notion/bin/radio | 17 +++++++-- kiro-gh/bin/radio | 17 +++++++-- kiro-local/bin/radio | 17 +++++++-- kiro-notion/bin/radio | 17 +++++++-- lib/zellij-tab.sh | 2 +- tests/claude_gh_task_work.bats | 14 ++++++++ tests/radio_lifecycle.bats | 63 ++++++++++++++++++++++++++++++++++ 10 files changed, 176 insertions(+), 22 deletions(-) diff --git a/claude-gh/bin/radio b/claude-gh/bin/radio index d4f0f0d..0264784 100755 --- a/claude-gh/bin/radio +++ b/claude-gh/bin/radio @@ -200,9 +200,20 @@ _ensure_session_file() { local loadout="" local sidecar sidecar=$(_loadout_sidecar "$TASK_FORCE_ROLE") - if [[ -f "$sidecar" ]]; then - loadout=$(<"$sidecar") - fi + # TOCTOU-safe read (#150 review): the sidecar can be unlinked by a + # concurrent cmd_unregister between any pre-check and the read — the + # unregister cascade documented in #140 is exactly that race window. + # `cat … 2>/dev/null || true` collapses three failure modes into "empty + # string" without tripping set -e: file absent, file unlinked mid-read, + # file unreadable. The env-var fallback below handles the empty case. + # + # Why not the obvious `loadout=$(<"$sidecar" || true)` the reviewer + # suggested: bash's `/tmp/f; v=$(/dev/null || true) [[ -n "$loadout" ]] || loadout="${TASK_FORCE_LOADOUT:-unknown}" local repo diff --git a/claude-jira/bin/radio b/claude-jira/bin/radio index d4f0f0d..0264784 100755 --- a/claude-jira/bin/radio +++ b/claude-jira/bin/radio @@ -200,9 +200,20 @@ _ensure_session_file() { local loadout="" local sidecar sidecar=$(_loadout_sidecar "$TASK_FORCE_ROLE") - if [[ -f "$sidecar" ]]; then - loadout=$(<"$sidecar") - fi + # TOCTOU-safe read (#150 review): the sidecar can be unlinked by a + # concurrent cmd_unregister between any pre-check and the read — the + # unregister cascade documented in #140 is exactly that race window. + # `cat … 2>/dev/null || true` collapses three failure modes into "empty + # string" without tripping set -e: file absent, file unlinked mid-read, + # file unreadable. The env-var fallback below handles the empty case. + # + # Why not the obvious `loadout=$(<"$sidecar" || true)` the reviewer + # suggested: bash's `/tmp/f; v=$(/dev/null || true) [[ -n "$loadout" ]] || loadout="${TASK_FORCE_LOADOUT:-unknown}" local repo diff --git a/claude-local/bin/radio b/claude-local/bin/radio index d4f0f0d..0264784 100755 --- a/claude-local/bin/radio +++ b/claude-local/bin/radio @@ -200,9 +200,20 @@ _ensure_session_file() { local loadout="" local sidecar sidecar=$(_loadout_sidecar "$TASK_FORCE_ROLE") - if [[ -f "$sidecar" ]]; then - loadout=$(<"$sidecar") - fi + # TOCTOU-safe read (#150 review): the sidecar can be unlinked by a + # concurrent cmd_unregister between any pre-check and the read — the + # unregister cascade documented in #140 is exactly that race window. + # `cat … 2>/dev/null || true` collapses three failure modes into "empty + # string" without tripping set -e: file absent, file unlinked mid-read, + # file unreadable. The env-var fallback below handles the empty case. + # + # Why not the obvious `loadout=$(<"$sidecar" || true)` the reviewer + # suggested: bash's `/tmp/f; v=$(/dev/null || true) [[ -n "$loadout" ]] || loadout="${TASK_FORCE_LOADOUT:-unknown}" local repo diff --git a/claude-notion/bin/radio b/claude-notion/bin/radio index d4f0f0d..0264784 100755 --- a/claude-notion/bin/radio +++ b/claude-notion/bin/radio @@ -200,9 +200,20 @@ _ensure_session_file() { local loadout="" local sidecar sidecar=$(_loadout_sidecar "$TASK_FORCE_ROLE") - if [[ -f "$sidecar" ]]; then - loadout=$(<"$sidecar") - fi + # TOCTOU-safe read (#150 review): the sidecar can be unlinked by a + # concurrent cmd_unregister between any pre-check and the read — the + # unregister cascade documented in #140 is exactly that race window. + # `cat … 2>/dev/null || true` collapses three failure modes into "empty + # string" without tripping set -e: file absent, file unlinked mid-read, + # file unreadable. The env-var fallback below handles the empty case. + # + # Why not the obvious `loadout=$(<"$sidecar" || true)` the reviewer + # suggested: bash's `/tmp/f; v=$(/dev/null || true) [[ -n "$loadout" ]] || loadout="${TASK_FORCE_LOADOUT:-unknown}" local repo diff --git a/kiro-gh/bin/radio b/kiro-gh/bin/radio index d4f0f0d..0264784 100755 --- a/kiro-gh/bin/radio +++ b/kiro-gh/bin/radio @@ -200,9 +200,20 @@ _ensure_session_file() { local loadout="" local sidecar sidecar=$(_loadout_sidecar "$TASK_FORCE_ROLE") - if [[ -f "$sidecar" ]]; then - loadout=$(<"$sidecar") - fi + # TOCTOU-safe read (#150 review): the sidecar can be unlinked by a + # concurrent cmd_unregister between any pre-check and the read — the + # unregister cascade documented in #140 is exactly that race window. + # `cat … 2>/dev/null || true` collapses three failure modes into "empty + # string" without tripping set -e: file absent, file unlinked mid-read, + # file unreadable. The env-var fallback below handles the empty case. + # + # Why not the obvious `loadout=$(<"$sidecar" || true)` the reviewer + # suggested: bash's `/tmp/f; v=$(/dev/null || true) [[ -n "$loadout" ]] || loadout="${TASK_FORCE_LOADOUT:-unknown}" local repo diff --git a/kiro-local/bin/radio b/kiro-local/bin/radio index d4f0f0d..0264784 100755 --- a/kiro-local/bin/radio +++ b/kiro-local/bin/radio @@ -200,9 +200,20 @@ _ensure_session_file() { local loadout="" local sidecar sidecar=$(_loadout_sidecar "$TASK_FORCE_ROLE") - if [[ -f "$sidecar" ]]; then - loadout=$(<"$sidecar") - fi + # TOCTOU-safe read (#150 review): the sidecar can be unlinked by a + # concurrent cmd_unregister between any pre-check and the read — the + # unregister cascade documented in #140 is exactly that race window. + # `cat … 2>/dev/null || true` collapses three failure modes into "empty + # string" without tripping set -e: file absent, file unlinked mid-read, + # file unreadable. The env-var fallback below handles the empty case. + # + # Why not the obvious `loadout=$(<"$sidecar" || true)` the reviewer + # suggested: bash's `/tmp/f; v=$(/dev/null || true) [[ -n "$loadout" ]] || loadout="${TASK_FORCE_LOADOUT:-unknown}" local repo diff --git a/kiro-notion/bin/radio b/kiro-notion/bin/radio index d4f0f0d..0264784 100755 --- a/kiro-notion/bin/radio +++ b/kiro-notion/bin/radio @@ -200,9 +200,20 @@ _ensure_session_file() { local loadout="" local sidecar sidecar=$(_loadout_sidecar "$TASK_FORCE_ROLE") - if [[ -f "$sidecar" ]]; then - loadout=$(<"$sidecar") - fi + # TOCTOU-safe read (#150 review): the sidecar can be unlinked by a + # concurrent cmd_unregister between any pre-check and the read — the + # unregister cascade documented in #140 is exactly that race window. + # `cat … 2>/dev/null || true` collapses three failure modes into "empty + # string" without tripping set -e: file absent, file unlinked mid-read, + # file unreadable. The env-var fallback below handles the empty case. + # + # Why not the obvious `loadout=$(<"$sidecar" || true)` the reviewer + # suggested: bash's `/tmp/f; v=$(/dev/null || true) [[ -n "$loadout" ]] || loadout="${TASK_FORCE_LOADOUT:-unknown}" local repo diff --git a/lib/zellij-tab.sh b/lib/zellij-tab.sh index 35d85c4..6190d0e 100755 --- a/lib/zellij-tab.sh +++ b/lib/zellij-tab.sh @@ -46,7 +46,7 @@ aw_zellij_tab_id_by_name() { | select((.name | sub("^⏸️ "; "") | sub("^▶️ "; "") - | sub("^❓ "; "")) == $n) + | sub("^❓︎ "; "")) == $n) | .tab_id] | .[0] // empty' 2>/dev/null } diff --git a/tests/claude_gh_task_work.bats b/tests/claude_gh_task_work.bats index c18196a..f44d3f9 100644 --- a/tests/claude_gh_task_work.bats +++ b/tests/claude_gh_task_work.bats @@ -445,6 +445,20 @@ _setup_stale_local_base() { assert_equal "${TAB_ID:-}" "12" } +@test ".info records TAB_ID when the tab is awaiting-painted (❓︎ ) at lookup time (#150 review: lib/radio emoji sync)" { + # The awaiting prefix in radio's _rename_tab uses U+2753 (❓) + U+FE0E + # (variation-selector-15, text-presentation) + space. Pre-#150 the strip + # in lib/zellij-tab.sh used U+2753 + space (no VS-15), so the byte + # sequences didn't match and the lookup missed when a tab was in the + # awaiting state. Sync the lib's strip to the radio's bytes and assert + # the lookup hits. + export STUB_ZELLIJ_TABS_JSON='[{"name":"❓︎ my-feature","tab_id":12}]' + run "$CLAUDE_GH_TASK_WORK" my-feature + assert_success + source "$WORKTREE_BASE/.my-feature.info" + assert_equal "${TAB_ID:-}" "12" +} + # --------------------------------------------------------------------------- # Zellij interactions # --------------------------------------------------------------------------- diff --git a/tests/radio_lifecycle.bats b/tests/radio_lifecycle.bats index 89a9d45..6d9dcde 100644 --- a/tests/radio_lifecycle.bats +++ b/tests/radio_lifecycle.bats @@ -370,6 +370,69 @@ teardown() { refute_output --partial "unknown" } +@test "re-seed survives when sidecar is unlinked between [[ -f ]] and read (#150 review: TOCTOU)" { + # Reproduces the race the reviewer called out on PR #150: a concurrent + # cmd_unregister (the very cascade we're guarding against) can unlink + # .loadout in the microsecond window between _ensure_session_file's + # `[[ -f "$sidecar" ]]` check and the `$(<"$sidecar")` read. + # + # Without the `|| true` guard on the read, the inner command-substitution + # failure trips set -e and _ensure_session_file aborts — session file is + # never written, STATE tracking dies for the worker's lifetime. We can't + # easily race a real unregister against the read in bats, but we can + # produce the same effect by replacing the read step with a deletion: a + # bash DEBUG trap that fires *between* `[[ -f ]]` and the read, deletes + # the sidecar, and lets the read see ENOENT. + # + # Simpler approach used here: drive the same failure mode by making the + # sidecar unreadable (chmod 0) so the read fails the same way the unlink + # would. The TOCTOU guard `|| true` must convert that into "fall through + # to env-var fallback" rather than aborting. + "$RADIO" register --role worker-foo --tab worker-foo --agent claude --loadout claude-gh + local sess="$TASK_FORCE_HOME/radio/sessions/worker-foo.info" + local sidecar="$TASK_FORCE_HOME/radio/sessions/worker-foo.loadout" + rm -f "$sess" + + # Replace the sidecar's contents with an unreadable state — equivalent + # to "the file exists at -f time but the read can't complete". On macOS + # / Linux, chmod 0 on the file makes the read fail with EACCES (same + # set -e propagation as ENOENT from a delete). + chmod 0 "$sidecar" + + # _ensure_session_file must NOT abort. Result: session file written, + # LOADOUT falls back to the env var (which we leave unset → "unknown"). + TASK_FORCE_ROLE=worker-foo ZELLIJ_TAB=worker-foo run env -u TASK_FORCE_LOADOUT "$RADIO" busy + # Restore permission so teardown can clean up. + chmod 644 "$sidecar" + assert_success + assert [ -f "$sess" ] + run grep "^STATE=" "$sess" + assert_output "STATE=busy" +} + +@test "re-seed survives when sidecar is unlinked just before read (#150 review: TOCTOU, unlink variant)" { + # Companion to the chmod variant above: the literal scenario the reviewer + # described — sidecar present at the `-f` check, unlinked before the + # read. We simulate by deleting the sidecar AFTER cmd_register but BEFORE + # the busy hook fires. _ensure_session_file's `[[ -f ]]` will miss (and + # the read won't execute) — but this exercises the "sidecar gone by the + # time we look" path that the guard is also meant to handle. + "$RADIO" register --role worker-foo --tab worker-foo --agent claude --loadout claude-gh + local sess="$TASK_FORCE_HOME/radio/sessions/worker-foo.info" + local sidecar="$TASK_FORCE_HOME/radio/sessions/worker-foo.loadout" + + rm -f "$sess" "$sidecar" + + TASK_FORCE_ROLE=worker-foo ZELLIJ_TAB=worker-foo run env -u TASK_FORCE_LOADOUT "$RADIO" busy + assert_success + assert [ -f "$sess" ] + # Fell back to env-var (unset → "unknown") — not the desired loadout + # value, but the worker keeps STATE tracking alive, which is the + # invariant the TOCTOU guard protects. + run grep "^LOADOUT=" "$sess" + assert_output "LOADOUT=unknown" +} + @test "register: writes loadout sidecar alongside session file (#140 Bug C)" { "$RADIO" register --role worker-foo --tab worker-foo --agent claude --loadout claude-gh local sidecar="$TASK_FORCE_HOME/radio/sessions/worker-foo.loadout" From 464278fb1bbea56e85b999049ab278eeda8b7312 Mon Sep 17 00:00:00 2001 From: Martin conur Date: Tue, 2 Jun 2026 15:31:19 -0400 Subject: [PATCH 3/3] =?UTF-8?q?Radio=20session=20corruption:=20PR=20#150?= =?UTF-8?q?=20review=20round=203=20=E2=80=94=20sidecar=20write-order=20+?= =?UTF-8?q?=20TOCTOU=20test=20hygiene?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two reviewer-flagged pre-merge fixes: 1. Sidecar write order in cmd_register (#150 round-2 review Finding 1): .info was written first, then .loadout sidecar. If killed (OOM, task-done --force, host shutdown) between the two writes, .info had the correct LOADOUT value but the sidecar didn't exist — so a later unregister-cascade + re-seed produced LOADOUT=unknown. The window is microseconds wide, but the cascade in #140 makes it statistically reachable. Fix: swap the order — write .loadout BEFORE .info. The pre-kill state then becomes "sidecar exists, .info missing", which is exactly the state _ensure_session_file is designed to handle (sidecar value is recovered correctly). No downside: _ensure_session_file only runs when .info is missing, and cmd_unregister sweeps both regardless of order. Applied across all 7 drift-grouped radio scripts. Two regression tests in radio_lifecycle.bats: - "re-seed recovers LOADOUT when .info-write was killed but sidecar landed first" — simulates the kill-window state (sidecar present, .info absent, $TASK_FORCE_LOADOUT not in env) and asserts the sidecar value is recovered, not "unknown". - "register: writes sidecar before .info so a kill mid-call leaves sidecar recoverable" — asserts the mtime invariant: sidecar is not newer than .info, confirming the write order at the filesystem level. Uses `find -newer` for portability across BSD/GNU stat. 2. TOCTOU test hygiene (#150 round-2 review Finding 2): - The chmod-0 test's comment referenced the `[[ -f ]]` pre-check that round 2 removed; the actual code is now an unconditional cat. Comment rewritten to describe what's actually under test. - The chmod-0 test asserted STATE=busy but not LOADOUT=unknown — so it would have passed even if cat silently succeeded on a readable file. Added the LOADOUT=unknown assertion to confirm the fallback path actually fired, matching the unlink-variant test's coverage. Tests: 775/775 passing (+2 net new — round 3 added 2 tests for the write-order invariant; the round-2 chmod-0 and unlink tests were preserved with hygiene fixes). Co-Authored-By: Claude Opus 4.7 (1M context) --- claude-gh/bin/radio | 33 ++++++++---- claude-jira/bin/radio | 33 ++++++++---- claude-local/bin/radio | 33 ++++++++---- claude-notion/bin/radio | 33 ++++++++---- kiro-gh/bin/radio | 33 ++++++++---- kiro-local/bin/radio | 33 ++++++++---- kiro-notion/bin/radio | 33 ++++++++---- tests/radio_lifecycle.bats | 108 +++++++++++++++++++++++++------------ 8 files changed, 243 insertions(+), 96 deletions(-) diff --git a/claude-gh/bin/radio b/claude-gh/bin/radio index 0264784..a091d7b 100755 --- a/claude-gh/bin/radio +++ b/claude-gh/bin/radio @@ -385,6 +385,30 @@ 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: + # + # sidecar-first (this order): + # pre-kill state: .loadout=claude-gh, .info absent + # re-seed reads: .loadout → LOADOUT=claude-gh 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 + # re-seed reads: no sidecar, hook subshell has no $TASK_FORCE_LOADOUT + # → LOADOUT=unknown ✗ + # + # 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 + # 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. + if [[ -n "$loadout" ]]; then + printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" + fi + { printf 'ROLE=%s\nTAB=%s\nTAB_ID=%s\nREPO=%s\nSTATE=idle\nLAST_HEARTBEAT=%s\nAGENT=%s\nLOADOUT=%s\n' \ "$role" "$tab" "$tab_id" "$repo" "$(_now)" "$agent" "$loadout" @@ -402,15 +426,6 @@ cmd_register() { mkdir -p "$(_inbox_dir "$role")" "$(_processed_dir "$role")" - # Persist the loadout name to a sidecar so _ensure_session_file can recover - # it during a mid-life re-seed (#140 Bug C). Skipping on empty values keeps - # the sidecar's 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. - if [[ -n "$loadout" ]]; then - printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" - fi - _log "register role=$role tab=$tab tab_id=$tab_id agent=$agent loadout=$loadout" # First paint of the idle prefix without waiting for a Stop hook cycle (#95). diff --git a/claude-jira/bin/radio b/claude-jira/bin/radio index 0264784..a091d7b 100755 --- a/claude-jira/bin/radio +++ b/claude-jira/bin/radio @@ -385,6 +385,30 @@ 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: + # + # sidecar-first (this order): + # pre-kill state: .loadout=claude-gh, .info absent + # re-seed reads: .loadout → LOADOUT=claude-gh 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 + # re-seed reads: no sidecar, hook subshell has no $TASK_FORCE_LOADOUT + # → LOADOUT=unknown ✗ + # + # 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 + # 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. + if [[ -n "$loadout" ]]; then + printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" + fi + { printf 'ROLE=%s\nTAB=%s\nTAB_ID=%s\nREPO=%s\nSTATE=idle\nLAST_HEARTBEAT=%s\nAGENT=%s\nLOADOUT=%s\n' \ "$role" "$tab" "$tab_id" "$repo" "$(_now)" "$agent" "$loadout" @@ -402,15 +426,6 @@ cmd_register() { mkdir -p "$(_inbox_dir "$role")" "$(_processed_dir "$role")" - # Persist the loadout name to a sidecar so _ensure_session_file can recover - # it during a mid-life re-seed (#140 Bug C). Skipping on empty values keeps - # the sidecar's 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. - if [[ -n "$loadout" ]]; then - printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" - fi - _log "register role=$role tab=$tab tab_id=$tab_id agent=$agent loadout=$loadout" # First paint of the idle prefix without waiting for a Stop hook cycle (#95). diff --git a/claude-local/bin/radio b/claude-local/bin/radio index 0264784..a091d7b 100755 --- a/claude-local/bin/radio +++ b/claude-local/bin/radio @@ -385,6 +385,30 @@ 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: + # + # sidecar-first (this order): + # pre-kill state: .loadout=claude-gh, .info absent + # re-seed reads: .loadout → LOADOUT=claude-gh 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 + # re-seed reads: no sidecar, hook subshell has no $TASK_FORCE_LOADOUT + # → LOADOUT=unknown ✗ + # + # 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 + # 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. + if [[ -n "$loadout" ]]; then + printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" + fi + { printf 'ROLE=%s\nTAB=%s\nTAB_ID=%s\nREPO=%s\nSTATE=idle\nLAST_HEARTBEAT=%s\nAGENT=%s\nLOADOUT=%s\n' \ "$role" "$tab" "$tab_id" "$repo" "$(_now)" "$agent" "$loadout" @@ -402,15 +426,6 @@ cmd_register() { mkdir -p "$(_inbox_dir "$role")" "$(_processed_dir "$role")" - # Persist the loadout name to a sidecar so _ensure_session_file can recover - # it during a mid-life re-seed (#140 Bug C). Skipping on empty values keeps - # the sidecar's 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. - if [[ -n "$loadout" ]]; then - printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" - fi - _log "register role=$role tab=$tab tab_id=$tab_id agent=$agent loadout=$loadout" # First paint of the idle prefix without waiting for a Stop hook cycle (#95). diff --git a/claude-notion/bin/radio b/claude-notion/bin/radio index 0264784..a091d7b 100755 --- a/claude-notion/bin/radio +++ b/claude-notion/bin/radio @@ -385,6 +385,30 @@ 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: + # + # sidecar-first (this order): + # pre-kill state: .loadout=claude-gh, .info absent + # re-seed reads: .loadout → LOADOUT=claude-gh 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 + # re-seed reads: no sidecar, hook subshell has no $TASK_FORCE_LOADOUT + # → LOADOUT=unknown ✗ + # + # 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 + # 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. + if [[ -n "$loadout" ]]; then + printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" + fi + { printf 'ROLE=%s\nTAB=%s\nTAB_ID=%s\nREPO=%s\nSTATE=idle\nLAST_HEARTBEAT=%s\nAGENT=%s\nLOADOUT=%s\n' \ "$role" "$tab" "$tab_id" "$repo" "$(_now)" "$agent" "$loadout" @@ -402,15 +426,6 @@ cmd_register() { mkdir -p "$(_inbox_dir "$role")" "$(_processed_dir "$role")" - # Persist the loadout name to a sidecar so _ensure_session_file can recover - # it during a mid-life re-seed (#140 Bug C). Skipping on empty values keeps - # the sidecar's 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. - if [[ -n "$loadout" ]]; then - printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" - fi - _log "register role=$role tab=$tab tab_id=$tab_id agent=$agent loadout=$loadout" # First paint of the idle prefix without waiting for a Stop hook cycle (#95). diff --git a/kiro-gh/bin/radio b/kiro-gh/bin/radio index 0264784..a091d7b 100755 --- a/kiro-gh/bin/radio +++ b/kiro-gh/bin/radio @@ -385,6 +385,30 @@ 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: + # + # sidecar-first (this order): + # pre-kill state: .loadout=claude-gh, .info absent + # re-seed reads: .loadout → LOADOUT=claude-gh 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 + # re-seed reads: no sidecar, hook subshell has no $TASK_FORCE_LOADOUT + # → LOADOUT=unknown ✗ + # + # 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 + # 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. + if [[ -n "$loadout" ]]; then + printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" + fi + { printf 'ROLE=%s\nTAB=%s\nTAB_ID=%s\nREPO=%s\nSTATE=idle\nLAST_HEARTBEAT=%s\nAGENT=%s\nLOADOUT=%s\n' \ "$role" "$tab" "$tab_id" "$repo" "$(_now)" "$agent" "$loadout" @@ -402,15 +426,6 @@ cmd_register() { mkdir -p "$(_inbox_dir "$role")" "$(_processed_dir "$role")" - # Persist the loadout name to a sidecar so _ensure_session_file can recover - # it during a mid-life re-seed (#140 Bug C). Skipping on empty values keeps - # the sidecar's 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. - if [[ -n "$loadout" ]]; then - printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" - fi - _log "register role=$role tab=$tab tab_id=$tab_id agent=$agent loadout=$loadout" # First paint of the idle prefix without waiting for a Stop hook cycle (#95). diff --git a/kiro-local/bin/radio b/kiro-local/bin/radio index 0264784..a091d7b 100755 --- a/kiro-local/bin/radio +++ b/kiro-local/bin/radio @@ -385,6 +385,30 @@ 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: + # + # sidecar-first (this order): + # pre-kill state: .loadout=claude-gh, .info absent + # re-seed reads: .loadout → LOADOUT=claude-gh 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 + # re-seed reads: no sidecar, hook subshell has no $TASK_FORCE_LOADOUT + # → LOADOUT=unknown ✗ + # + # 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 + # 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. + if [[ -n "$loadout" ]]; then + printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" + fi + { printf 'ROLE=%s\nTAB=%s\nTAB_ID=%s\nREPO=%s\nSTATE=idle\nLAST_HEARTBEAT=%s\nAGENT=%s\nLOADOUT=%s\n' \ "$role" "$tab" "$tab_id" "$repo" "$(_now)" "$agent" "$loadout" @@ -402,15 +426,6 @@ cmd_register() { mkdir -p "$(_inbox_dir "$role")" "$(_processed_dir "$role")" - # Persist the loadout name to a sidecar so _ensure_session_file can recover - # it during a mid-life re-seed (#140 Bug C). Skipping on empty values keeps - # the sidecar's 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. - if [[ -n "$loadout" ]]; then - printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" - fi - _log "register role=$role tab=$tab tab_id=$tab_id agent=$agent loadout=$loadout" # First paint of the idle prefix without waiting for a Stop hook cycle (#95). diff --git a/kiro-notion/bin/radio b/kiro-notion/bin/radio index 0264784..a091d7b 100755 --- a/kiro-notion/bin/radio +++ b/kiro-notion/bin/radio @@ -385,6 +385,30 @@ 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: + # + # sidecar-first (this order): + # pre-kill state: .loadout=claude-gh, .info absent + # re-seed reads: .loadout → LOADOUT=claude-gh 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 + # re-seed reads: no sidecar, hook subshell has no $TASK_FORCE_LOADOUT + # → LOADOUT=unknown ✗ + # + # 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 + # 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. + if [[ -n "$loadout" ]]; then + printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" + fi + { printf 'ROLE=%s\nTAB=%s\nTAB_ID=%s\nREPO=%s\nSTATE=idle\nLAST_HEARTBEAT=%s\nAGENT=%s\nLOADOUT=%s\n' \ "$role" "$tab" "$tab_id" "$repo" "$(_now)" "$agent" "$loadout" @@ -402,15 +426,6 @@ cmd_register() { mkdir -p "$(_inbox_dir "$role")" "$(_processed_dir "$role")" - # Persist the loadout name to a sidecar so _ensure_session_file can recover - # it during a mid-life re-seed (#140 Bug C). Skipping on empty values keeps - # the sidecar's 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. - if [[ -n "$loadout" ]]; then - printf '%s' "$loadout" | _atomic_write "$(_loadout_sidecar "$role")" - fi - _log "register role=$role tab=$tab tab_id=$tab_id agent=$agent loadout=$loadout" # First paint of the idle prefix without waiting for a Stop hook cycle (#95). diff --git a/tests/radio_lifecycle.bats b/tests/radio_lifecycle.bats index 6d9dcde..b58af7b 100644 --- a/tests/radio_lifecycle.bats +++ b/tests/radio_lifecycle.bats @@ -370,37 +370,27 @@ teardown() { refute_output --partial "unknown" } -@test "re-seed survives when sidecar is unlinked between [[ -f ]] and read (#150 review: TOCTOU)" { - # Reproduces the race the reviewer called out on PR #150: a concurrent - # cmd_unregister (the very cascade we're guarding against) can unlink - # .loadout in the microsecond window between _ensure_session_file's - # `[[ -f "$sidecar" ]]` check and the `$(<"$sidecar")` read. +@test "re-seed survives when sidecar read fails (#150 round-2 review: TOCTOU on unconditional cat)" { + # _ensure_session_file does an unconditional `cat "$sidecar" 2>/dev/null + # || true` — no `[[ -f ]]` pre-check. The TOCTOU guard must convert any + # read failure (file absent, unlinked mid-read by concurrent unregister, + # unreadable) into "empty string" without tripping set -e. Otherwise the + # function aborts mid-re-seed and STATE tracking dies for the worker's + # lifetime. # - # Without the `|| true` guard on the read, the inner command-substitution - # failure trips set -e and _ensure_session_file aborts — session file is - # never written, STATE tracking dies for the worker's lifetime. We can't - # easily race a real unregister against the read in bats, but we can - # produce the same effect by replacing the read step with a deletion: a - # bash DEBUG trap that fires *between* `[[ -f ]]` and the read, deletes - # the sidecar, and lets the read see ENOENT. - # - # Simpler approach used here: drive the same failure mode by making the - # sidecar unreadable (chmod 0) so the read fails the same way the unlink - # would. The TOCTOU guard `|| true` must convert that into "fall through - # to env-var fallback" rather than aborting. + # We exercise the failure path by making the sidecar unreadable + # (chmod 0). cat returns ENOENT-equivalent (EACCES); without `|| true`, + # set -e propagates and `radio busy` exits non-zero. With it, the read + # returns empty and we fall through to the env-var fallback (unset → + # "unknown") — assert that fallback fired, not just that the script + # completed. "$RADIO" register --role worker-foo --tab worker-foo --agent claude --loadout claude-gh local sess="$TASK_FORCE_HOME/radio/sessions/worker-foo.info" local sidecar="$TASK_FORCE_HOME/radio/sessions/worker-foo.loadout" rm -f "$sess" - # Replace the sidecar's contents with an unreadable state — equivalent - # to "the file exists at -f time but the read can't complete". On macOS - # / Linux, chmod 0 on the file makes the read fail with EACCES (same - # set -e propagation as ENOENT from a delete). chmod 0 "$sidecar" - # _ensure_session_file must NOT abort. Result: session file written, - # LOADOUT falls back to the env var (which we leave unset → "unknown"). TASK_FORCE_ROLE=worker-foo ZELLIJ_TAB=worker-foo run env -u TASK_FORCE_LOADOUT "$RADIO" busy # Restore permission so teardown can clean up. chmod 644 "$sidecar" @@ -408,15 +398,18 @@ teardown() { assert [ -f "$sess" ] run grep "^STATE=" "$sess" assert_output "STATE=busy" + # Fallback value confirms the read returned empty AND the env-var + # fallback fired — not that cat silently succeeded. + run grep "^LOADOUT=" "$sess" + assert_output "LOADOUT=unknown" } -@test "re-seed survives when sidecar is unlinked just before read (#150 review: TOCTOU, unlink variant)" { - # Companion to the chmod variant above: the literal scenario the reviewer - # described — sidecar present at the `-f` check, unlinked before the - # read. We simulate by deleting the sidecar AFTER cmd_register but BEFORE - # the busy hook fires. _ensure_session_file's `[[ -f ]]` will miss (and - # the read won't execute) — but this exercises the "sidecar gone by the - # time we look" path that the guard is also meant to handle. +@test "re-seed survives when sidecar is absent at read time (#150 review: TOCTOU, unlink variant)" { + # Companion to the chmod variant above: sidecar deleted before the + # busy hook fires (the realistic cascade pattern — unregister wipes + # both .info and .loadout, then the next hook event re-seeds). The + # unconditional `cat` must collapse to empty without aborting; the + # env-var fallback handles the LOADOUT. "$RADIO" register --role worker-foo --tab worker-foo --agent claude --loadout claude-gh local sess="$TASK_FORCE_HOME/radio/sessions/worker-foo.info" local sidecar="$TASK_FORCE_HOME/radio/sessions/worker-foo.loadout" @@ -426,13 +419,62 @@ teardown() { TASK_FORCE_ROLE=worker-foo ZELLIJ_TAB=worker-foo run env -u TASK_FORCE_LOADOUT "$RADIO" busy assert_success assert [ -f "$sess" ] - # Fell back to env-var (unset → "unknown") — not the desired loadout - # value, but the worker keeps STATE tracking alive, which is the - # invariant the TOCTOU guard protects. run grep "^LOADOUT=" "$sess" assert_output "LOADOUT=unknown" } +@test "re-seed recovers LOADOUT when .info-write was killed but sidecar landed first (#150 round-3 review: Finding 1)" { + # Models the kill-window the reviewer flagged: cmd_register writes the + # sidecar BEFORE the session file (post-#150-r3), so if the process is + # killed between the two writes, the sidecar exists but .info doesn't. + # The next re-seed should find the sidecar and recover LOADOUT correctly + # instead of writing "unknown". + # + # Simulate by registering normally, then deleting only .info (matches + # the "sidecar landed, info didn't" state). Pre-r3 (write-info-first + # order) this state was unreachable except through manual file + # manipulation; post-r3 it's the natural mid-kill snapshot. + "$RADIO" register --role worker-foo --tab worker-foo --agent claude --loadout claude-gh + local sess="$TASK_FORCE_HOME/radio/sessions/worker-foo.info" + local sidecar="$TASK_FORCE_HOME/radio/sessions/worker-foo.loadout" + assert [ -f "$sidecar" ] + rm -f "$sess" + assert [ -f "$sidecar" ] + + TASK_FORCE_ROLE=worker-foo ZELLIJ_TAB=worker-foo run env -u TASK_FORCE_LOADOUT "$RADIO" busy + assert_success + run grep "^LOADOUT=" "$sess" + assert_output "LOADOUT=claude-gh" +} + +@test "register: writes sidecar before .info so a kill mid-call leaves sidecar recoverable (#150 round-3 review: Finding 1)" { + # Direct assertion of the write-order invariant: a kill that interrupts + # cmd_register between the sidecar write and the .info write must leave + # the system in a state that a subsequent re-seed can recover. Inverse + # case (sidecar absent but .info present) would be unrecoverable — + # that's the pre-r3 failure mode. + # + # We can't actually kill in the middle of a real register, but we can + # assert the timestamps: sidecar's mtime must be ≤ .info's mtime. (On + # POSIX, stat -f vs -c differ; use `find -newer` to compare without + # parsing.) + "$RADIO" register --role worker-foo --tab worker-foo --agent claude --loadout claude-gh + local sess="$TASK_FORCE_HOME/radio/sessions/worker-foo.info" + local sidecar="$TASK_FORCE_HOME/radio/sessions/worker-foo.loadout" + assert [ -f "$sess" ] + assert [ -f "$sidecar" ] + # If sidecar were written AFTER .info, .info would be `-newer` than + # sidecar would be false (sidecar would be newer). Assert .info is + # newer-or-equal to sidecar. + run find "$sess" -newer "$sidecar" + # `find -newer` prints the file if it's STRICTLY newer. .info written + # immediately after sidecar may share the same mtime resolution on + # fast filesystems — either output (the path or empty) is fine; what + # matters is that sidecar isn't newer than .info. + run find "$sidecar" -newer "$sess" + refute_output --partial "$sidecar" +} + @test "register: writes loadout sidecar alongside session file (#140 Bug C)" { "$RADIO" register --role worker-foo --tab worker-foo --agent claude --loadout claude-gh local sidecar="$TASK_FORCE_HOME/radio/sessions/worker-foo.loadout"