diff --git a/claude-gh/bin/radio b/claude-gh/bin/radio index 02cb6bf..a091d7b 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,38 @@ _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") + # 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 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 +234,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. @@ -332,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" @@ -348,6 +425,7 @@ cmd_register() { } | _atomic_write "$f" mkdir -p "$(_inbox_dir "$role")" "$(_processed_dir "$role")" + _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 +452,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 +463,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..a091d7b 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,38 @@ _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") + # 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 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 +234,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. @@ -332,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" @@ -348,6 +425,7 @@ cmd_register() { } | _atomic_write "$f" mkdir -p "$(_inbox_dir "$role")" "$(_processed_dir "$role")" + _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 +452,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 +463,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..a091d7b 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,38 @@ _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") + # 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 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 +234,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. @@ -332,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" @@ -348,6 +425,7 @@ cmd_register() { } | _atomic_write "$f" mkdir -p "$(_inbox_dir "$role")" "$(_processed_dir "$role")" + _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 +452,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 +463,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..a091d7b 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,38 @@ _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") + # 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 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 +234,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. @@ -332,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" @@ -348,6 +425,7 @@ cmd_register() { } | _atomic_write "$f" mkdir -p "$(_inbox_dir "$role")" "$(_processed_dir "$role")" + _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 +452,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 +463,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..a091d7b 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,38 @@ _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") + # 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 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 +234,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. @@ -332,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" @@ -348,6 +425,7 @@ cmd_register() { } | _atomic_write "$f" mkdir -p "$(_inbox_dir "$role")" "$(_processed_dir "$role")" + _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 +452,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 +463,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..a091d7b 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,38 @@ _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") + # 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 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 +234,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. @@ -332,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" @@ -348,6 +425,7 @@ cmd_register() { } | _atomic_write "$f" mkdir -p "$(_inbox_dir "$role")" "$(_processed_dir "$role")" + _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 +452,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 +463,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..a091d7b 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,38 @@ _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") + # 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 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 +234,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. @@ -332,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" @@ -348,6 +425,7 @@ cmd_register() { } | _atomic_write "$f" mkdir -p "$(_inbox_dir "$role")" "$(_processed_dir "$role")" + _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 +452,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 +463,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/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 0110273..b58af7b 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: 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}]' -@test "register: re-register preserves existing TAB_ID when zellij list-tabs misses (#117)" { + "$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,231 @@ 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 "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. + # + # 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" + + chmod 0 "$sidecar" + + 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" + # 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 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" + + 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" ] + 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" + 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=" +}