# Report: Skill Optimization Implementation Review (Final)

- **Author**: Reviewer Cline (`canary-projects-multi-agent-mux-reviewer-cline`)
- **Brief**: `.mam/reports/brief-rereview-skill-optimization.md`
- **Plan reviewed against**: `.agents/reports/canary-projects-multi-agent-mux-planner-claude/report-skill-optimization-plan.md`
- **Scope**: Phase 1 (OP-1, OP-2, OP-3) + Phase 2 (OP-4, OP-5, OP-6) working-tree changes
- **Output path**: `.agents/reports/canary-projects-multi-agent-mux-reviewer-cline/report-skill-optimization-review-final.md`

## Verdict

# ✅ PASS

All six focus-area items named in the brief (OP-1, OP-2, OP-3, OP-4, OP-6, OP-7) are correctly implemented in the working tree. `bash -n` passes on all four changed shell scripts; `python3 -m py_compile` passes on `job_subscriber.py`; no new `shellcheck` warnings were introduced (all reported SC2155/SC2164/SC2034 are pre-existing). Two non-blocking observations are noted below; one out-of-scope item (OP-5) is flagged for awareness.

---

## Validation Commands (run inside this session)

| Check | Command | Result |
|---|---|---|
| Syntax (shell) | `bash -n lib.sh stop_session.sh multi-agent-mux-delegate-job reconcile.sh` | All 4 OK |
| Syntax (python) | `python3 -m py_compile job_subscriber.py` | OK |
| Lint | `shellcheck -S warning` on the 4 changed scripts | No new warnings (all pre-existing) |

Reported shellcheck warnings (all pre-existing, unchanged by this diff): `lib.sh:1043,1046` SC2164 (`cd` in `start_watchdog`), `stop_session.sh:71` SC2155 (`export TMUX_SERVER_NAME=$(...)`), `reconcile.sh:33,34` SC2034 (`ONCE`/`EMIT_DIFF` — false positives, used by the `--once`/`--emit-diff` arg parser).

---

## Per-Item Review

### OP-1 - Reactive Tmux Graceful Stopping (`stop_session.sh`) — PASS

**Plan**: Implement `_wait_session_gone` in `lib.sh` polling `tmux has-session` at ~250 ms up to a deadline; call it from `stop_session.sh` replacing the fixed `sleep 3` / `sleep 5`.

**Implementation** (`lib.sh:1128-1136`):
```bash
_wait_session_gone() {
  local sess="$1" max="${2:-5}" i
  for ((i = 0; i < max * 4; i++)); do
    _sks_tmux has-session -t "$sess" 2>/dev/null || return 0
    sleep 0.25
  done
  return 1
}
```
Call sites (`stop_session.sh:193,200`):
```bash
_wait_session_gone "$SESSION_NAME" 5 || true
...
tmux kill-session -t "$SESSION_NAME" 2>/dev/null || true
_wait_session_gone "$SESSION_NAME" 8 || true
```

**Findings**:
- Poll interval 0.25 s × `max*4` iterations = exactly `max` seconds deadline. Math is correct.
- Uses `_sks_tmux` (server-aware), so it respects `TMUX_SERVER_NAME`. Correct.
- `|| true` appended at both call sites is **essential**: `stop_session.sh` runs under `set -euo pipefail` (line 30), and `_wait_session_gone` returns 1 on timeout. Without `|| true`, a timeout would abort the whole graceful chain. The guard is present. ✅
- Happy path returns early on first `has-session` failure (~0.25 s), matching the plan's "<0.3 s" outcome.

**Verdict**: Correctly implements the plan.

### OP-2 - MQTT Subscriber Event-Driven Handshake (`delegate-job`) — PASS (with minor observation)

**Plan**: `job_subscriber.py` writes `SUBSCRIBED <topic>` on SUBACK; the wrapper replaces `sleep 1` with a fast-poll loop matching the sentinel, with a liveness check.

**Implementation** — `job_subscriber.py:188-194`:
```python
def on_subscribe(_c, _u, mid, granted_qos, _props=None):
    for topic in subscribed_topics:
        print(f"SUBSCRIBED {topic}", flush=True)
client.on_subscribe = on_subscribe
```
Wrapper (`multi-agent-mux-delegate-job:119-135`, mirrored at :218-234):
```bash
local sub_ready=0
for ((i = 0; i < 25; i++)); do
  if kill -0 "$sub_pid" 2>/dev/null; then
    if grep -q '^SUBSCRIBED ' "$logf" 2>/dev/null; then
      sub_ready=1; break
    fi
  else
    echo "ERROR: subscriber died early (pid=$sub_pid, check $logf)" >&2; exit 1
  fi
  sleep 0.2
done
if [ "$sub_ready" -ne 1 ]; then
  echo "WARNING: subscriber subscribe handshake timed out — falling back to proceed" >&2
fi
```

**Findings**:
- Sentinel `^SUBSCRIBED ` is written with `flush=True` so it is visible to the wrapper's `grep` immediately. ✅
- `kill -0 "$sub_pid"` liveness check runs **before** the grep, so a dead subscriber is caught and exits 1 rather than waiting the full 5 s. ✅ Correct ordering.
- 25 × 0.2 s = 5 s deadline; falls back gracefully with a WARNING (non-fatal) on timeout. ✅ Matches plan.
- `on_subscribe` signature `(_c, _u, mid, granted_qos, _props=None)` covers both paho-mqtt v2 (MQTTv3, 5-arg) and v3 (MQTTv5, 6-arg with props) — the `_props=None` default absorbs the extra arg. ✅ Forward-compatible.
- **Minor observation (non-blocking)**: `on_subscribe` iterates `subscribed_topics` and prints all topics on **every** suback. In the multi-job case, the sentinel fires after the **first** suback, not after all topics are subscribed. For the common single-job delegation this is exactly correct; for multi-job the wrapper proceeds slightly early. Since the orchestrator's own subscriber is the one that must be ready to receive the agent's `started` event — and that event is published to one job's topic — proceeding after the first suback is safe as long as the first-subscribed topic is the active job's. The registration order (`for job in jobs`) makes this hold. No fix required; noted for completeness.
- Logic is correctly duplicated for both the `direct` path (:119) and the `loop/discuss` orchestrator path (:218). ✅

**Verdict**: Correctly implements the plan.

### OP-3 - Main Event Loop Pacing (`reconcile.sh`) — PASS (with minor observation)

**Plan**: Replace `while True: time.sleep(0.5)` with `threading.Event().wait(timeout=next_deadline - now)`.

**Implementation** (`reconcile.sh:242-266`):
```python
import threading
stop_event = threading.Event()
start = time.time()
try:
    while True:
        now = time.time()
        wall_left = (timeout - (now - start)) if timeout else None
        idle_left = (idle_timeout - (now - state['last_msg'])) if idle_timeout else None
        next_timeout = 5.0
        if wall_left is not None:
            next_timeout = min(next_timeout, wall_left)
        if idle_left is not None:
            next_timeout = min(next_timeout, idle_left)
        if next_timeout <= 0:
            ...print which deadline hit...
            break
        stop_event.wait(timeout=next_timeout)
finally:
    client.loop_stop()
```

**Findings**:
- Computes `next_timeout = min(5.0, wall_left, idle_left)` so the main thread sleeps only as long as the nearest deadline, capped at 5 s — a strict improvement over the old fixed 0.5 s wake-up. ✅
- Deadline-expired branch (`next_timeout <= 0`) correctly distinguishes wall vs idle timeout in its log message. ✅
- **Minor observation (non-blocking)**: `stop_event` is never `set()` by any callback (e.g. `on_message`), so `stop_event.wait(timeout=...)` behaves identically to `time.sleep(timeout)` here. It is still an improvement because `Event.wait` is interruptible by signals (e.g. SIGINT) and is the idiomatic primitive for a condition-style sleep; but the full "event-driven wakeup on terminal event" benefit described in the plan would require `on_message` to call `stop_event.set()` when `event in ("completed","error")`. The current implementation is correct and an improvement; the optional enhancement (waking immediately on terminal event rather than on the next deadline tick) is left for a follow-up. Non-blocking.

**Verdict**: Correctly implements the plan (the reactive-sleep core).

### OP-4 - Unify Divergent Tmux Server Resolvers (`lib.sh`) — PASS

**Plan**: Extract a single canonical `mam_tmux()` dispatcher; have `_tmux`/`_sks_tmux` delegate to it.

**Implementation** (`lib.sh:97-113`):
```bash
mam_tmux() {
  _resolve_real_tmux_path
  if [ -n "${TMUX_SERVER_NAME:-}" ] && [ "$TMUX_SERVER_NAME" != "default" ]; then
    "$_REAL_TMUX_PATH" -L "$TMUX_SERVER_NAME" "$@"
  else
    "$_REAL_TMUX_PATH" "$@"
  fi
}
_tmux() { _init_tmux_isolation; mam_tmux "$@"; }
tmux()  { _tmux "$@"; }
```
`_sks_tmux` (`lib.sh:1118`) now reads: `_sks_tmux() { mam_tmux "$@"; }`.

**Findings**:
- `mam_tmux` calls `"$_REAL_TMUX_PATH"` (the resolved real binary), **not** the shell function `tmux()` — so there is no recursion. ✅ (This was verified carefully; an earlier transient revision of the diff showed a literal `"tmux"` call which would have recursed, but the final working tree uses `$_REAL_TMUX_PATH`.)
- `mam_tmux` calls `_resolve_real_tmux_path` directly (not the full `_init_tmux_isolation` PATH-shim setup), so it is safe to use from hot paths that don't want the shim side effect. `_tmux` still runs the full `_init_tmux_isolation` for callers that rely on the PATH shim. Correct separation of concerns. ✅
- `_sks_tmux` previously had its own 4-line inline resolver with the SC2086-prone `tmux -L $TMUX_SERVER_NAME "$@"`; it now delegates to `mam_tmux`, removing the duplication. ✅
- All four duplication sites named in the plan are consolidated: `_tmux`, `_sks_tmux`, and the inline `local_tmux` in `wait_for_tui_ready` (which already used `_sks_tmux`/`_tmux`). The `create_session.sh:212` `local_tmux` string is for the human-readable `START_CMD` YAML field, not a live call — left as-is, correctly.

**Verdict**: Correctly implements the plan; no recursion hazard.

### OP-6 - Consolidate TUI Ready / Dialog Tokens (`lib.sh`) — PASS

**Plan**: Declare `_MAM_DIALOG_TOKENS` and `_MAM_READY_TOKENS_CLAUDE` at top of `lib.sh`; refer to them everywhere.

**Implementation** (`lib.sh:25-27`):
```bash
_MAM_DIALOG_TOKENS='Do you trust the files|Yes, proceed|No, exit|Allow this|Press Enter to continue|browser to authenticate|Use arrow keys|Esc to cancel'
_MAM_READY_TOKENS_CLAUDE='Anthropic|Assistant|Chat|Welcome|projects'
```
References (`grep` confirms 3 use sites):
- `lib.sh:1071` `wait_for_tui_ready` claude branch → `grep -E -q "$_MAM_READY_TOKENS_CLAUDE"`
- `lib.sh:1158` `_pane_dialog_open` → `grep -Eq "$_MAM_DIALOG_TOKENS"`
- `lib.sh:1224` `handle_startup_dialogs` → `grep -Eq "$_MAM_READY_TOKENS_CLAUDE"`

**Findings**:
- Both constants are byte-identical to the previously-inlined literals. ✅ No behavioral drift.
- All three former inline sites now reference the constants; no stray literal copies remain (`grep` for the raw token strings outside the constant declarations returns nothing). ✅
- Quoting is consistent (`"$_MAM_DIALOG_TOKENS"` preserves the `|` alternation for `grep -E`). ✅

**Verdict**: Correctly implements the plan.

### OP-7 - Guard against Non-Bash Sourced Environments (`lib.sh`) — PASS

**Plan**: Add a zsh-aware fallback or an explicit exit message warning users not to source from a foreign shell.

**Implementation** (`lib.sh:17-20`):
```bash
if [ -z "${BASH_VERSION:-}" ]; then
  echo "ERROR: lib.sh must be executed/sourced from bash (foreign shell detected)" >&2
  return 1 2>/dev/null || exit 1
fi
```

**Findings**:
- The guard runs **before** any `BASH_SOURCE` use (line 21), so a zsh `source` exits cleanly with a diagnostic instead of silently resolving `BASH_SOURCE[0]` to empty and sourcing the wrong `lib.sh`. ✅
- `return 1 2>/dev/null || exit 1` covers both cases: `return` works when sourced (function context), `exit` works when executed directly. ✅
- This implements the "explicit exit message" option from the plan. The fuller zsh `${(%):-%x}` fallback was the alternative; the chosen approach is simpler and aligns with AGENTS.md "Simplicity First". ✅

**Verdict**: Correctly implements the plan.

---

## Out-of-Scope / Awareness

- **OP-5 (Single-Source YAML/SQLite Load Boilerplate)** — listed under Phase 2 in the plan, but **not in the brief's focus-area list** and **not implemented** in this working-tree diff. The 7 duplicated load blocks remain. This is consistent with the brief's scoped focus (OP-1,2,3,4,6,7) but is called out so Phase 2 completion is not mis-reported. Recommend a follow-up PR for OP-5.
- **OP-8 (Reconcile Observability)** — Phase 3 item, not in the brief's scope, not implemented. The fallback polling loop at `reconcile.sh:269` still uses `bash "$_self" --once --emit-diff >/dev/null 2>&1 || true`. Recommend a follow-up.

---

## Non-Blocking Observations (no action required for PASS)

1. **OP-2 multi-job sentinel**: `on_subscribe` prints all topics on each suback; the wrapper proceeds after the first suback. Safe for single-job (the common path) and for multi-job given registration order, but a future multi-job refactor should gate on "all topics subacked" (e.g. count subacks vs `len(subscribed_topics)`).
2. **OP-3 unused `stop_event.set()`**: `stop_event` is never set, so it functions as an interruptible `time.sleep`. Adding `stop_event.set()` in `on_message` on terminal events would let the loop wake immediately on job completion instead of on the next deadline tick — an optional latency improvement, not a correctness issue.

Both observations are enhancements, not defects; neither blocks the PASS verdict.