multi-agent-mux/.agents/reports/canary-projects-multi-agent-mux-reviewer-cline/report-skill-optimization-analysis.md

# Report: Multi-Agent Mux Skill Optimization Analysis

- **Author**: Reviewer Cline (`canary-projects-multi-agent-mux-reviewer-cline`)
- **Scope**: All shell scripts under `.agents/skills/` (8 files, ~3,422 lines)
- **Audited at**: `25de01e fix(skills): solve set -e error propagation in create_session.sh and include final PASS reviews`
- **Brief**: `.mam/reports/brief-skill-optimization-analysis.md`

## Files audited

| File | Lines | Role |
|---|---|---|
| `lib.sh` | 1212 | Shared library (session-name, atomic YAML dump, UUID resolver, tmux isolation, send_keys_safe) |
| `multi-agent-mux-create/scripts/create_session.sh` | 408 | Session creation + onboarding job injection |
| `multi-agent-mux-delegate-job/multi-agent-mux-delegate-job` | 440 | User-facing job orchestrator (submit/status/list/verify/wait/logs) |
| `multi-agent-mux-monitor/scripts/reconcile.sh` | 644 | YAML<->tmux<->disk drift detection + MQTT subscriber / polling fallback |
| `multi-agent-mux-stop/scripts/stop_session.sh` | 370 | Graceful/hard stop + conversation purge |
| `multi-agent-mux-status/scripts/status.sh` | 140 | Read-only status table (reuses reconcile --dry-run) |
| `multi-agent-mux-resume/scripts/resolve_session_id.sh` | 44 | UUID resolver wrapper |
| `multi-agent-mux-resume/scripts/update_yaml_resumed.sh` | 164 | Resume YAML updater |

All scripts declare `#!/usr/bin/env bash` and `set -euo pipefail`, and `source` `lib.sh` via a `BASH_SOURCE`-relative path. The codebase is internally consistent under bash; findings below distinguish "broken under bash" (real bugs) from "non-portable / latent risk" (works today, breaks if assumptions change).

---

## Focus Area 1 - Inefficient Polling / Sleeps

### 1.1 `reconcile.sh:243-256` - MQTT event loop spins on `time.sleep(0.5)` (highest impact)

```python
start = time.time()
try:
    while True:
        now = time.time()
        if timeout and (now - start) >= timeout: ...
        if idle_timeout and (now - state['last_msg']) >= idle_timeout: ...
        time.sleep(0.5)     # <-- busy-wait defeating the event loop
finally:
    client.loop_stop()
```

`paho-mqtt` already runs a background network thread via `client.loop_start()` (line 241). The foreground `while True: time.sleep(0.5)` is a **busy poll layered on top of an event-driven client**. It wakes every 0.5 s solely to compare timestamps.

**Diagnosis**: This is the clearest "polling where an event-driven primitive exists" case. The loop does no I/O; it only enforces two deadlines (overall `timeout`, `idle_timeout`).

**Proposed fix**: Replace the spin with `client.loop_forever()` for the blocking model, or compute the next deadline and `client.loop(min/max)` / `select` on the broker socket with a computed timeout. The cleanest minimal change keeps `loop_start()` and blocks the main thread on a condition variable instead of polling `time.time()`:

```python
import threading
stop = threading.Event()
# set stop when timeout/idle reached (timer callback or on_message watchdog)
stop.wait(timeout=next_deadline - now)
```

Either removes the 0.5 s wake-ups entirely while preserving the two deadline semantics.

### 1.2 `multi-agent-mux-delegate-job:119` and `:205` - `sleep 1` to win a race

```bash
"$PY" "$SCRIPT_DIR/scripts/job_subscriber.py" ... >"$logf" 2>&1 &
local sub_pid=$!
sleep 1  # give the subscriber time to CONNACK + SUBSCRIBE before the agent runs
run_agent "$JOB_ID" "$instructions"
```

A fixed 1 s sleep papers over the Subscribe-before-Publish ordering dependency (MQTT does not queue non-retained messages for absent subscribers). 1 s is simultaneously too short on a slow/loaded broker and wastefully long on a fast one.

**Diagnosis**: Genuine readiness race, not a renderer wait. The subscriber knows when it has SUBSCRIBED (`on_connect` fires, sets `state['connected']=True`), but that signal is trapped inside the subscriber process and never surfaces to the orchestrator.

**Proposed fix**: Have `job_subscriber.py` write a readiness token (e.g. create `$logf.ready` or print a `READY <jid>` line and flush) once `on_connect` succeeds and the SUBSCRIBE ack returns. The orchestrator then polls for that token with a short deadline (up to 5 s at 0.1 s granularity) instead of a blind `sleep 1`. Falls back to the existing 1 s if the token never appears. This is still polling, but it is **evidence-driven** (the subscriber asserts it is ready) rather than time-driven.

### 1.3 `stop_session.sh:193,200` - fixed `sleep 3` / `sleep 5` in graceful fallback chain

```bash
send_keys_safe "$SESSION_NAME" "$exitkey" "stop$$" || ...
sleep 3
if ! tmux has-session ...; then return 0; fi
tmux kill-session -t "$SESSION_NAME" ...
sleep 5
if ! tmux has-session ...; then return 0; fi
kill -9 "$pane_pid"
```

Two fixed waits after actions that have an observable outcome (`tmux has-session` flips false).

**Diagnosis**: Fixed 3 s + 5 s = up to 8 s of unconditional waiting even when the session dies in 50 ms.

**Proposed fix**: Replace each fixed sleep with a bounded poll:

```bash
_wait_gone() { # <sess> <deadline_sec>
  local sess="$1" deadline=$(( $(date +%s) + "$2" ))
  while [ "$(date +%s)" -lt "$deadline" ]; do
    tmux has-session -t "$sess" 2>/dev/null || return 0
    sleep 0.3
  done
  return 1
}
```

Then `_wait_gone "$SESSION_NAME" 3 || tmux kill-session ...`. Total worst case stays 8 s, but the happy path returns in ~0.3 s.

### 1.4 `lib.sh:1048-1084` `wait_for_tui_ready` - fixed 1 s poll, bash brace expansion

```bash
for i in {1..15}; do
  if _pane_dialog_open "$sess"; then sleep 1; continue; fi
  content=$($local_tmux capture-pane -p -t "$sess" 2>/dev/null || echo "")
  ...grep for banner...
  sleep 1
done
```

15 x 1 s = 15 s worst case. The per-iteration `sleep 1` is coarse; a TUI that renders in 0.4 s still costs 1 s of dead time per miss.

**Diagnosis**: Polling is unavoidable here (a TUI emits no readiness event), but the 1 s granularity is arbitrary. Also uses bash-only `{1..15}` brace expansion.

**Proposed fix**: Switch to a deadline-bounded loop with a 0.3-0.5 s interval:

```bash
local deadline=$(( $(date +%s) + 15 )) i
while [ "$(date +%s)" -lt "$deadline" ]; do
  ...checks...
  sleep 0.3
done
```

Keeps the 15 s ceiling, halves the average detection latency, drops the bash brace expansion. (Minor - already works; included for completeness.)

### 1.5 `lib.sh:1174` - magic `sleep 0.5` after paste-buffer

```bash
_tmux paste-buffer -b "sks_$job_id" -t "$sess"
_tmux delete-buffer -b "sks_$job_id" 2>/dev/null || true
sleep 0.5
_pane_capture "$sess" | grep -Fq "$marker" || { ... return 3; }
```

A fixed 0.5 s wait so the pasted text becomes visible before the marker grep.

**Diagnosis**: Could reuse the existing `_pane_quiescent` primitive (lib.sh:1122) - two consecutive identical captures imply the renderer settled, which is the actual precondition for the marker being readable.

**Proposed fix**: `if ! _pane_quiescent "$sess" 8 0.2; then ... return 3; fi` before the grep, or drop the `sleep 0.5` and retry the marker grep a few times with 0.1 s spacing. Removes the magic constant in favor of an evidence-based wait already in the library.

### 1.6 Summary - sleeps that are fine

- `lib.sh:1128` `_pane_quiescent` `sleep "$interval"` (0.5 s) - already a parameterized, evidence-bounded poll (returns on two identical captures). OK.
- `lib.sh:1159-1168` dialog-wait loop - already deadline-bounded (`SKS_DIALOG_TIMEOUT`). Ok.
- `reconcile.sh:236` `time.sleep(0.1)` connect-wait - bounded by 5 s. Ok.
- `reconcile.sh:273` `sleep "$POLL_INTERVAL"` (15 s) - documented polling fallback when broker is down; acceptable, though it could also probe for broker recovery between polls.

---

## Focus Area 2 - Helper Duplication & Modularization

### 2.1 Three reimplementations of "tmux with `-L <server>`" (highest duplication)

The same 4-line "pick bare `tmux` vs `tmux -L $TMUX_SERVER_NAME`" decision appears as:

| Location | Form |
|---|---|
| `lib.sh:89-96` | `_tmux()` function (calls `_init_tmux_isolation` first, resolves real binary) |
| `lib.sh:1105-1111` | `_sks_tmux()` function (server-aware, no shim init) |
| `lib.sh:1043-1046` | inline `local_tmux="tmux"; if ...; then local_tmux="tmux -L ..."; fi` inside `wait_for_tui_ready` |
| `stop_session.sh:182,188,195` | bare `tmux ...` calls (relies on the PATH shim from `_init_tmux_isolation`, which `stop_session.sh` never calls - it only exports `TMUX_SERVER_NAME`) |

**Diagnosis**: `_tmux` and `_sks_tmux` differ only in whether they run the shim-init side effect. `wait_for_tui_ready` reinvents the wheel inline. `stop_session.sh` quietly depends on a shim that may not be installed (it sources `lib.sh`, which calls `_init_tmux_isolation` only lazily inside `_tmux()` - and `stop_session.sh` calls bare `tmux`, never `_tmux`).

**Proposed fix**: Collapse to a single server-aware dispatcher in `lib.sh`:

```bash
# One entry point. Callers that need the shim auto-install use _tmux;
# _sks_tmux stays as the no-init variant for hot paths. Both delegate to
# _tmux_with_server below.
_tmux_with_server() {
  if [ -n "${TMUX_SERVER_NAME:-}" ] && [ "$TMUX_SERVER_NAME" != "default" ]; then
    tmux -L "$TMUX_SERVER_NAME" "$@"
  else
    tmux "$@"
  fi
}
```

Then make `wait_for_tui_ready` call `_tmux_with_server` (drop the inline `local_tmux`), and convert `stop_session.sh`'s bare `tmux` calls to `_tmux_with_server` so they no longer depend on the PATH shim. This also fixes the latent bug where `stop_session.sh` kills the wrong server if the shim isn't on PATH.

### 2.2 DB + YAML state-load boilerplate duplicated 7+ times

This ~20-line block is copy-pasted nearly verbatim:

```python
db_path = os.path.splitext(yaml_path)[0] + '.db'
d = {}
try:
    if os.path.exists(db_path):
        conn = sqlite3.connect(db_path, timeout=60.0)
        row = conn.execute('SELECT data FROM state WHERE id=1').fetchone()
        if row: d = json.loads(row[0])
        try:
            cursor = conn.execute('SELECT data FROM sessions')
            for r in cursor.fetchall(): db_sessions.append(json.loads(r[0]))
            d['tmux_sessions'] = db_sessions
        except sqlite3.OperationalError: pass
        conn.close()
    elif os.path.exists(yaml_path):
        with open(yaml_path) as f: d = yaml.safe_load(f) or {}
except Exception: pass
```

Instances (line ranges are the full block):

| File | Lines | Notes |
|---|---|---|
| `lib.sh` (`resolve_tmux_server`) | 112-152 | per-row lookup variant |
| `lib.sh` (`find_workspace_uuid`) | 585-611 | |
| `lib.sh` (agent_identities load) | 748-761 | |
| `stop_session.sh` | 87-113 | |
| `status.sh` | 42-62 | |
| `update_yaml_resumed.sh` | 66-91 | |
| `reconcile.sh` | 298-321 | |

**Diagnosis**: Each copy has slightly different error handling (some `pass`, some `print(WARN)`, some swallow `sqlite3.OperationalError` for the `sessions` table, some don't query it). This drift is exactly the inconsistency `lib.sh` was created to prevent (its own header, lines 4-9, calls out "four things inconsistently re-implemented"). The load logic is now a fifth.

**Proposed fix**: Add a `lib.sh` helper that emits a JSON document of the merged state to stdout, callable from any script without re-sourcing Python:

```bash
# load_state_json - prints the merged {state + sessions table / yaml} as JSON.
# Callers parse with python -c or jq. Single source of truth.
load_state_json() {
  YAML_PATH="$AGENT_SESSIONS_YAML" env_python "$AGENT_SESSIONS_YAML" <<'PYEOF'
import os, json, sqlite3, yaml
yaml_path = os.environ['YAML_PATH']
db_path = os.path.splitext(yaml_path)[0] + '.db'
d = {}
# ... one canonical implementation with structured errors ...
print(json.dumps(d, ensure_ascii=False))
PYEOF
}
```

Scripts then pipe the JSON into their per-row logic. The canonical copy owns the error policy (see Focus Area 4.1).

### 2.3 `*_exists` artifact probes trapped inside a heredoc

`jsonl_exists`, `db_exists`, `hermes_exists`, `cline_exists`, `own_exists` (lib.sh:509-540) are defined **inside** the `find_workspace_uuid` heredoc, so they vanish when that Python process exits. `status.sh:70-86` (`resume_on_disk`) and `reconcile.sh:498-623` reimplement the same existence checks inline.

**Diagnosis**: Four agent-specific "does this conversation artifact exist" predicates are a natural shared module but live in a single-use heredoc.

**Proposed fix**: Move them into a small `lib.py` (sibling of `lib.sh`) that every heredoc imports via `sys.path.append`. Then `status.sh` and `reconcile.sh` call `libpy.artifact_exists(agent, uuid, iso_root)` instead of re-rolling the paths. This is the same relocatable-path discipline `lib.sh` already uses.

### 2.4 `infer_agent_from_session` duplicated verbatim

`stop_session.sh:74-82` and `update_yaml_resumed.sh:39-47` are **byte-identical**:

```bash
case "$SESSION_NAME" in
  *-creator-claude) AGENT=claude ;;
  *-creator-agy)    AGENT=agy ;;
  *-creator-hermes) AGENT=hermes ;;
  *-creator-cline)  AGENT=cline ;;
  *) echo "ERROR: cannot infer agent from '$SESSION_NAME'; pass --agent" >&2; exit 2 ;;
esac
```

**Proposed fix**: Add to `lib.sh`:

```bash
infer_agent_from_session() {
  case "$1" in
    *-creator-claude) printf 'claude' ;; *-creator-agy) printf 'agy' ;;
    *-creator-hermes) printf 'hermes' ;; *-creator-cline) printf 'cline' ;;
    *) echo "ERROR: cannot infer agent from '$1'; pass --agent" >&2; return 2 ;;
  esac
}
```

Callers: `AGENT=$(infer_agent_from_session "$SESSION_NAME") || exit $?`.

### 2.5 `_delegate_py_bin` vs `pick_python` - divergent venv walks

- `lib.sh:944-960` `_delegate_py_bin` walks up from `BASH_SOURCE` dir looking for `.venv/bin/python`, caches in `AGENT_PYTHON_BIN` (shell var, not exported - correct per DONE.md FW-08).
- `multi-agent-mux-delegate-job:27-45` `pick_python` walks `WORKDIR/.venv` then `./.venv` then `python3`, and adds a `paho.mqtt` import check.

**Diagnosis**: Two different walk strategies for the same concept ("find the project venv python"). `_delegate_py_bin` walks *up* from the skill dir; `pick_python` walks from `WORKDIR`/cwd. They can return different interpreters. `pick_python` also re-runs the `import paho.mqtt` check on every call (cheap but redundant with caching).

**Proposed fix**: Unify - `_delegate_py_bin` should accept an optional starting dir, and `pick_python` should call it then add the `paho.mqtt` check. One walk strategy, one cache.

---

## Focus Area 3 - Portability & POSIX Compliance

The brief calls out "bash-isms when running under raw sh" and "BASH_SOURCE under non-bash shells like zsh". Every audited script has `#!/usr/bin/env bash`, so **direct execution is safe**. The risks are (a) a user sourcing a script from zsh/fish interactively, and (b) future shebang changes. Findings ordered by likelihood.

### 3.1 `BASH_SOURCE` unbound under zsh/dash if sourced (P1, latent)

`BASH_SOURCE` is used in 12 places (lib.sh:17, 950, 966; create:22; delegate-job:17; reconcile:17,48; resolve:12; update_yaml:10; status:10,12,29; stop:32). Under zsh, `${BASH_SOURCE[0]}` is unset -> `dirname ""` -> the `cd ""` either fails or lands in `$PWD`, silently sourcing the wrong `lib.sh` or none.

**Diagnosis**: The shebang protects `bash script.sh` execution. The real exposure is a user typing `source .agents/skills/lib.sh` from an interactive zsh (common on macOS where the default shell is zsh). This is the exact scenario the brief names.

**Proposed fix**: Add a portable fallback at the top of `lib.sh`:

```bash
# Portable script-dir resolution: BASH_SOURCE under bash, $0 under POSIX sh,
# ${(%):-%x} under zsh. Falls back to $0.
if [ -n "${BASH_SOURCE[0]:-}" ]; then
  _src="${BASH_SOURCE[0]}"
elif [ -n "${ZSH_VERSION:-}" ]; then
  eval '_src="${(%):-%x}"'
else
  _src="$0"
fi
SKILL_DIR="$(cd "$(dirname "$_src")" && pwd)"
```

Alternatively, guard the whole library with `if [ -z "${BASH_VERSION:-}" ]; then echo "lib.sh requires bash" >&2; return 1 2>/dev/null || exit 1; fi` and document that scripts must be executed, not sourced, from foreign shells. The marker-file root lookup (FUTURE_WORKS FW-P6) would also remove the fragile `../..` depth assumptions.

### 3.2 `status.sh:29` - 4-level relative climb, depth-assumption fragility (P1)

```bash
PROJECT_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../../../../" && pwd)"
```

This climbs exactly four levels (`scripts/ -> multi-agent-mux-status/ -> skills/ -> .agents/ -> <root>`). Every other script climbs two levels to reach `skills/` and then appends a known subpath. The four-level climb hard-codes the tree depth.

**Diagnosis**: If `multi-agent-mux-status` is ever nested one level deeper (or the `.agents` dir is relocated/symlinked), this resolves to the wrong directory and `get_job_status` silently falls back to `('jid','unknown')` because none of the candidate job paths exist.

**Proposed fix**: Resolve `PROJECT_ROOT` the same way `WORKSPACE_ROOT` is derived in `lib.sh:18` (`SKILL_DIR/../..`), i.e. `PROJECT_ROOT="$WORKSPACE_ROOT"` (which `lib.sh` already computes). Then `status.sh` doesn't need its own climb at all - `lib.sh` is the single source of truth for root resolution. This also subsumes FUTURE_WORKS FW-P6 for this file.

### 3.3 Bash-only syntax (P2, protected by shebang)

| Construct | Locations | POSIX `sh` equivalent |
|---|---|---|
| `[[ ... ]]` double brackets | lib.sh:36,41,58,76; delegate-job:20,22,29,31,33,74,85,... | `[ ... ]` (with quoting) |
| `for i in {1..15}` brace expansion | lib.sh:1048 | `seq 1 15` or a `while` loop |
| `for ((i=0; i<tries; i++))` C-style for | lib.sh:1124 | `i=0; while [ $i -lt $tries ]; do ...; i=$((i+1)); done` |
| `local` keyword | everywhere | not in POSIX (but in dash; most shells support it) |
| `read -r -d '' RECON_SRC` | reconcile.sh:284 | `-d` is bash/ksh; POSIX has no NUL-delim read |
| `+=` array/string append | delegate-job (via `+=`) | `x="$x$y"` |
| `mapfile`/`readarray` | not used OK | - |

**Diagnosis**: All protected by `#!/usr/bin/env bash`. Not bugs today. They become bugs only if (a) a shebang is changed to `#!/bin/sh`, or (b) a script is `source`d into a POSIX shell. The brief asks for these to be found, not necessarily fixed - flagging for awareness.

**Proposed fix (if POSIX portability is ever a goal)**: The two genuinely portable-blocking items are `read -r -d ''` (reconcile.sh:284) and brace expansion (lib.sh:1048). Both have trivial POSIX equivalents shown above. `[[ ]]` and `local` are widely supported (dash, ash, zsh) even though non-POSIX, so they're low priority. No change recommended unless a non-bash target is committed.

### 3.4 `set -a; source .env; set +a` (delegate-job:20-24) - fine, but unguarded

```bash
if [[ -f .env ]]; then set -a; source .env; set +a
elif [[ -f "$SCRIPT_DIR/../../.env" ]]; then set -a; source "$SCRIPT_DIR/../../.env"; set +a
fi
```

**Diagnosis**: Sourcing an arbitrary `.env` with `set -a` (export-all) executes any shell syntax in the file. If `.env` ever contains shell injection (e.g. a value with `$(...)`), it runs with the script's privileges. Standard for `.env` loaders, but worth noting given the brief's robustness focus. The `[[ -f .env ]]` also prefers cwd over the project root, which can load the wrong `.env` if run from a subdir.

**Proposed fix (optional)**: Prefer the project-root `.env` first, and consider a guarded parse (`while IFS== read key val; do ...`) instead of `source` if untrusted `.env` files are a concern. Low priority.

---

## Focus Area 4 - Error Handling & Robustness

### 4.1 Top-level `except Exception: pass` swallows data corruption (highest impact)

Seven top-level `try/except Exception: pass` blocks turn corrupt state into silent empty data:

| File | Lines | Consequence of swallowing |
|---|---|---|
| `lib.sh` (resolve_tmux_server) | 148-149 | Corrupt YAML -> silently returns `default` server -> stop/resume hit wrong tmux server |
| `lib.sh` (find_workspace_uuid) | 610-611 | Corrupt DB -> empty session list -> UUID resolution falls through to global tiers (the P0-C bug the library exists to prevent) |
| `lib.sh` (agent_identities) | 760-761 | Corrupt DB -> empty identities -> cache tier skipped silently |
| `stop_session.sh` | 112-113 | Corrupt state -> `MAPPED_DATA` empty -> stop proceeds with no target, wrong behavior |
| `status.sh` | 61-62 | Corrupt state -> prints "(no sessions registered)" instead of an error |
| `update_yaml_resumed.sh` | 84-85 | Corrupt state -> `DELEGATE_JOB_ID` empty silently |
| `reconcile.sh` | 320-321 | Corrupt state -> empty `d` -> drift report shows no drift on a corrupted file |

**Diagnosis**: These are the same pattern the brief calls "silent failures ... stderr swallowed without proper diagnostics." A corrupt `agent-sessions.yaml` or `.db` is a serious operational condition, but the code treats it identically to "file doesn't exist yet" (the legitimate empty case). There's no way for an operator to distinguish "fresh install" from "broken state."

**Proposed fix**: Distinguish "absent" from "broken":

```python
try:
    if os.path.exists(db_path):
        conn = sqlite3.connect(db_path, timeout=60.0)
        row = conn.execute('SELECT data FROM state WHERE id=1').fetchone()
        if row: d = json.loads(row[0])
        conn.close()
    elif os.path.exists(yaml_path):
        with open(yaml_path) as f: d = yaml.safe_load(f) or {}
except (sqlite3.DatabaseError, yaml.YAMLError, json.JSONDecodeError) as e:
    # State exists but is unreadable - this is an error, not "fresh."
    print(f"ERROR: state at {yaml_path} is corrupt: {e}", file=sys.stderr, flush=True)
    raise SystemExit(1)
```

The `elif` (file absent) path stays silent (legitimate first-run). Only the "exists but unreadable" path becomes loud. Apply uniformly via the `load_state_json` helper from 2.2 so the policy lives in one place.

### 4.2 `export TMUX_SERVER_NAME="$(resolve_tmux_server ...)"` masks failure (SC2155)

`stop_session.sh:71` and `update_yaml_resumed.sh:36`:

```bash
export TMUX_SERVER_NAME="$(resolve_tmux_server "$SESSION_NAME")"
```

ShellCheck SC2155: the command substitution's exit code is masked by `export`. If `resolve_tmux_server` ever exits non-zero (today it always exits 0 via the fallback at lib.sh:151, but a future change could break that), the error is lost.

**Proposed fix**:

```bash
TMUX_SERVER_NAME="$(resolve_tmux_server "$SESSION_NAME")" || exit $?
export TMUX_SERVER_NAME
```

### 4.3 `lib.sh:1030-1033` `start_watchdog` - `cd` without `|| return` (SC2164)

```bash
local orig_pwd="$PWD"
cd "$workdir"
nohup bash "$monitor_script" --subscribe --idle-timeout 0 >> "$log_file" 2>&1 &
pid=$!
cd "$orig_pwd"
```

If `cd "$workdir"` fails (dir removed between the `[ -f "$monitor_script" ]` check and here), `nohup` runs in `$PWD` with a relative `monitor_script`/`log_file`, and `cd "$orig_pwd"` also runs unconditionally.

**Proposed fix**:

```bash
cd "$workdir" || { echo "ERROR: workdir vanished: $workdir" >&2; return 1; }
nohup bash "$monitor_script" --subscribe --idle-timeout 0 >> "$log_file" 2>&1 &
pid=$!
cd "$orig_pwd" || true
```

### 4.4 `reconcile.sh:56 / 262` - broad `set +e` window

```bash
set +e
YAML_PATH=... "$PYBIN" - <<'PYEOF'
... ~180 lines of Python ...
PYEOF
sub_rc=$?
set -e
```

The `set +e` covers the entire Python heredoc plus the polling fallback (lines 263-277). This is necessary because the Python intentionally `sys.exit(3)` to signal "broker unavailable," but the window is large: any shell error in the surrounding scaffolding (env var expansion, the fallback `_self` loop) is non-fatal during that window.

**Diagnosis**: Today the only command between `set +e` and `set -e` that can fail non-Python is the fallback polling loop, which already has `|| true`. The risk is low but the pattern is fragile - a future edit that adds a shell command inside the `set +e` span will silently ignore failures.

**Proposed fix**: Narrow the `set +e` to just the Python invocation (it already is - line 262 restores before the fallback). Action item: add a comment making the invariant explicit, and don't let future edits extend the `set +e` span.

### 4.5 `delegate_publish_event` - fully silent on broker outage (by design, but unlogged)

```bash
delegate_publish_event() {
  ...
  "$py_bin" "$pub" --job "$job_id" --event "$event" --detail "$detail" || true
}
```

The `|| true` makes every publish failure non-fatal (correct per the contract: a delegate event must never abort create/stop/resume). But there's no diagnostic path: a broker outage during a stop means `stopped` is never published, the job's lifecycle event is missing, and nothing logs this.

**Proposed fix**: Log to stderr without affecting exit code:

```bash
"$py_bin" "$pub" --job "$job_id" --event "$event" --detail "$detail" \\
  || echo "WARN: delegate event '$event' for job $job_id failed (broker down?)" >&2
```

Keeps the non-fatal contract, gives operators a signal.

### 4.6 `reconcile.sh` `handle_terminal` - unvalidated MQTT job_id passed via env

```python
jid = payload.get("job_id")     # from untrusted MQTT payload
event = payload.get("event")
...
env['MQTT_JID'] = jid
env['MQTT_EVENT'] = event
cmd = ['bash', '-c', 'source "$LIB_SH"; atomic_dump_yaml "$YAML_PATH" MQTT_JID=...']
```

HMAC is verified (line 173-176, good - addresses FUTURE_WORKS FW-P7). The values reach `atomic_dump_yaml`'s Python via environment variables, not string interpolation (correct - lib.sh comment "P1-B" calls this out). But `jid`/`event` are not length- or charset-validated before being set as env vars. A malicious-but-HMAC-valid payload (requires the auth token) with a multi-MB `job_id` could exhaust environment space or cause odd `os.environ['MQTT_JID']` behavior.

**Diagnosis**: Low risk (requires the HMAC token to pass), but the brief's robustness focus applies. Defense-in-depth only.

**Proposed fix**: After HMAC verification, validate format:

```python
if not (isinstance(jid, str) and len(jid) <= 128 and jid.isascii()):
    print(f"MQTT Monitor: drop event: invalid job_id format", flush=True); return
if event not in ("completed", "error"):
    print(f"MQTT Monitor: drop event: invalid event {event!r}", flush=True); return
```

### 4.7 `except Exception` counts by file (for prioritization)

| File | `except Exception: pass` (fully silent) | `except ... as e: print(...)` (logged) |
|---|---|---|
| `reconcile.sh` | 5 (lines 257, 314, 320, 525, 550, 585, 613) | 3 (205, 229, 256) |
| `lib.sh` | 5 (128, 148, 332, 448, 463) | 0 |
| `stop_session.sh` | 2 (103, 112) | 1 (324) |
| `status.sh` | 3 (55, 61, 108) | 0 |
| `update_yaml_resumed.sh` | 1 (84) | 0 |

The `sqlite3.OperationalError: pass` cases (missing `sessions` table on older DBs) are legitimate - the table was added in a migration. Those should stay silent. The top-level `except Exception: pass` cases (4.1 above) are the ones to fix.

---

## Prioritized Action List

| ID | Finding | Area | Severity | Effort |
|---|---|---|---|---|
| **O-1** | `reconcile.sh:243-256` MQTT loop spins `time.sleep(0.5)` instead of `loop_forever`/Event | 1 | High | Medium |
| **O-2** | Top-level `except Exception: pass` swallows corrupt state (7 sites) | 4 | High | Medium (fix once in `load_state_json`) |
| **O-3** | `delegate-job:119,205` `sleep 1` subscriber-readiness race | 1 | Medium | Medium (needs subscriber readiness token) |
| **O-4** | Three `tmux -L` reimplementations; `stop_session.sh` uses bare `tmux` w/o shim | 2 | Medium | Low (collapse to `_tmux_with_server`) |
| **O-5** | DB+YAML load boilerplate duplicated 7x | 2 | Medium | Medium (extract `load_state_json`) |
| **O-6** | `status.sh:29` 4-level `../` climb -> use `WORKSPACE_ROOT` from lib.sh | 3 | Medium | Low |
| **O-7** | `stop_session.sh:193,200` fixed `sleep 3`/`sleep 5` -> bounded poll | 1 | Low | Low |
| **O-8** | `*_exists` probes trapped in heredoc -> shared `lib.py` | 2 | Low | Medium |
| **O-9** | `infer_agent_from_session` duplicated verbatim | 2 | Low | Trivial |
| **O-10** | `_delegate_py_bin` vs `pick_python` divergent venv walks | 2 | Low | Low |
| **O-11** | `export VAR="$(cmd)"` masks rc (SC2155) in stop/resume | 4 | Low | Trivial |
| **O-12** | `start_watchdog` `cd` without `|| return` (SC2164) | 4 | Low | Trivial |
| **O-13** | `delegate_publish_event` fully silent on broker outage | 4 | Low | Trivial |
| **O-14** | `lib.sh:1174` magic `sleep 0.5` -> `_pane_quiescent` | 1 | Low | Low |
| **O-15** | `BASH_SOURCE` unbound under zsh if sourced | 3 | Low (latent) | Low |
| **O-16** | `wait_for_tui_ready` coarse 1 s poll + brace expansion | 1 | Low | Trivial |
| **O-17** | `handle_terminal` unvalidated MQTT jid/event format | 4 | Low (defense-in-depth) | Trivial |

### Recommended sequencing

1. **O-2 + O-5 together** - extracting `load_state_json` is the vehicle for fixing the silent-corruption swallows. One helper, one error policy, seven call sites cleaned up. Highest ROI.
2. **O-1** - standalone, removes the busiest poller in the codebase.
3. **O-4 + O-9 + O-6** - small, mechanical de-duplication in `lib.sh`; fixes the latent `stop_session.sh` wrong-server bug as a side effect.
4. **O-3** - requires a small subscriber-side change (readiness token), then an orchestrator-side wait. Test with a flaky-broker harness.
5. The rest (O-7, O-8, O-10-O-17) are independent low-risk cleanups; bundle into one follow-up PR.

### Out of scope (noted, not actioned)

- FUTURE_WORKS FW-P6 (marker-file root lookup) would supersede O-6 and O-15; tracked separately.
- FUTURE_WORKS FW-P7 (HMAC on termination) is already implemented (reconcile.sh:173); O-17 is the remaining defense-in-depth on top.
- The `[[ ]]`, `local`, brace-expansion, and `read -d ''` bash-isms (3.3) are protected by the `#!/usr/bin/env bash` shebang and are not bugs under the current execution model. No change recommended unless a POSIX target is committed.

---

## Methodology / Verification

- All line numbers verified against `25de01e` via `grep -rn` and `sed -n` on the working tree.
- ShellCheck run at `-S warning` over all 8 scripts; reported warnings: SC2155 (x2), SC2164 (x2), SC2034 (x2, `ONCE`/`EMIT_DIFF` - these are used by the `--once`/`--emit-diff` flags via the arg parser, so ShellCheck's "unused" is a false positive for the documented CLI surface).
- Sleeps enumerated via `grep -rn '\bsleep\b'` across `--include='*.sh' --include='multi-agent-mux-delegate-job'` (18 shell sleeps + 3 Python `time.sleep`).
- `except` handlers enumerated via `grep -rn -A1 'except.*:'`.
- No code was modified; this is an analysis report only.