Godopu
7eeb4b709a
- OP-1: Implement reactive _wait_session_gone in lib.sh and stop_session.sh with set -e || true guard - OP-2: Event-driven MQTT subscribe handshake with sub_pid liveness in delegate-job - OP-3: Replace CPU time.sleep(0.5) spin with threading.Event wait in reconcile.sh - OP-4: Define mam_tmux dispatcher targeting resolved _REAL_TMUX_PATH to prevent recursion - OP-6 & OP-7: Add token variables and bash version source check in lib.sh - Integrate approved optimization plan and PASS review reports from all agents
164 lines
14 KiB
Markdown
164 lines
14 KiB
Markdown
# MAM Skill Optimization Analysis (Creator Claude)
|
||
|
||
- **Analyst**: Creator Claude (`canary-projects-multi-agent-mux-creator-claude`)
|
||
- **Date**: 2026-07-11
|
||
- **Brief**: `.mam/reports/brief-skill-optimization-analysis.md`
|
||
- **Scope**: all 8 shell entry points under `.agents/skills/` — 3,422 lines total (`lib.sh` 1212, `reconcile.sh` 644, delegate-job wrapper 440, `create_session.sh` 408, `stop_session.sh` 370, `update_yaml_resumed.sh` 164, `status.sh` 140, `resolve_session_id.sh` 44)
|
||
- **Audit baseline**: working tree on `da895fc` + the uncommitted prompt-lock F5 fix in `create_session.sh` (reviewed PASS, awaiting commit)
|
||
- **Method**: full-tree greps (sleeps, tmux-resolution variants, `|| true`/`2>/dev/null`, BASH_SOURCE, heredocs, eval), shellcheck run, targeted reads of every flagged site.
|
||
|
||
## Baseline strengths (for calibration)
|
||
|
||
Uniform `#!/usr/bin/env bash` + `set -euo pipefail` across all 7 executables (lib.sh correctly bare as a sourced library); zero `eval` in any executable script; zero warning-level shellcheck findings beyond 6 pre-existing ones; the worst sleep offenders (resume's blind `sleep 5/3`, inject's `sleep 0.5`+blind C-m) were already eliminated by the FW-W2 `send_keys_safe` work. The findings below are the next tier.
|
||
|
||
---
|
||
|
||
## Focus 1 — Inefficient Polling / Sleeps
|
||
|
||
### S1 (HIGH VALUE) — `stop_session.sh:193,200`: fixed post-kill waits
|
||
```bash
|
||
tmux send-keys … # graceful exitkey
|
||
sleep 3 # ← always pays 3 s
|
||
…
|
||
tmux kill-session …
|
||
sleep 5 # ← always pays 5 s
|
||
```
|
||
Every graceful stop pays the full 3 s even when the agent exits in 200 ms, and the kill path always pays 5 s. Worse than slow: on a loaded host an agent needing >3 s to flush **falsely escalates** to SIGTERM. **Proposal**: add to lib.sh —
|
||
```bash
|
||
# _wait_session_gone <sess> <max_sec> — returns 0 as soon as the session dies
|
||
_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
|
||
}
|
||
```
|
||
Replace `sleep 3` with `_wait_session_gone "$SESSION_NAME" 5` and `sleep 5` with `_wait_session_gone "$SESSION_NAME" 8`. Reactive (typical stop drops from ~8 s to <1 s), *and* more tolerant of slow exits.
|
||
|
||
### S2 (HIGH VALUE) — delegate-job `:119` and `:205`: `sleep 1` as MQTT handshake
|
||
The comment admits the ordering dependency ("MQTT does not queue non-retained messages for absent subscribers"), then guesses: if CONNACK+SUBACK takes >1 s (public broker `broker.hivemq.com` over WAN — entirely realistic), the agent's `started` event is **lost silently** and the job idles to timeout; on a local broker the 1 s ×2 per review-loop iteration is pure waste. **Proposal** (event-driven handshake, also fixes E4): `job_subscriber.py` already logs to `$logf` — have it print a sentinel line (e.g. `SUBSCRIBED <topic>`) from its `on_subscribe` callback (flush immediately), then in the wrapper replace both sleeps with:
|
||
```bash
|
||
for _ in {1..25}; do grep -q '^SUBSCRIBED ' "$logf" 2>/dev/null && break; sleep 0.2; done
|
||
grep -q '^SUBSCRIBED ' "$logf" || { echo "ERROR: subscriber never reached SUBACK (see $logf)" >&2; exit 1; }
|
||
```
|
||
Removes the race instead of betting on it, and converts a dead-on-arrival subscriber (see E4) into a loud failure.
|
||
|
||
### S3 (minor) — `lib.sh:1042-1085` `wait_for_tui_ready`
|
||
Two `capture-pane` invocations per iteration (`_pane_dialog_open` + its own `content=$(…)`) with a hand-rolled `local_tmux`. Capture once per iteration into a variable and test both predicates on it; use `_pane_capture` (see D4). The 15×1 s poll budget itself is fine.
|
||
|
||
### S4 (accepted as-is) — `reconcile.sh:273` `sleep "$POLL_INTERVAL"` broker-down fallback loop is by design; see E1 for its real problem (silence, not pacing).
|
||
|
||
---
|
||
|
||
## Focus 2 — Helper Duplication & Modularization
|
||
|
||
### D1 (HIGH VALUE) — four divergent server-aware tmux resolutions
|
||
| Site | Form |
|
||
|---|---|
|
||
| `lib.sh:1104-1110` `_sks_tmux()` | function — **canonical, word-split-safe** |
|
||
| `lib.sh:1043-1046` (`wait_for_tui_ready`) | `local_tmux="tmux -L $NAME"` string |
|
||
| `create_session.sh:212-215` | same string pattern (file also has a `_tmux` helper used by its trap — two mechanisms in one script) |
|
||
| delegate-job `:347-350` | `_tmux="tmux -L $NAME"` string |
|
||
|
||
The string variants rely on unquoted word-splitting (`$local_tmux send-keys …`) — the exact idiom class shellcheck SC2086 exists for, and each future call site must re-remember the `!= default` rule. **Proposal**: rename/promote `_sks_tmux` to `mam_tmux()` (keep `_sks_tmux` as an alias for compatibility) and replace all three string variants. Mechanical, ~10 lines net deletion.
|
||
|
||
### D2 — delegate-job wrapper: duplicated subscriber-spawn + instruction template
|
||
The register→spawn-subscriber→sleep→build-`instructions` block appears twice (direct path `:113-131`, review-loop path `:199-215`) and the copies have already drifted (log filename schema differs; the review-loop copy carries iteration metadata the direct copy lacks). Extract `_spawn_job_subscriber <job_id> <logf>` and `_job_instruction_block <job_id> <pub_cmd> <task_text>`; combine with S2 so the handshake logic exists exactly once.
|
||
|
||
### D3 — ready/dialog token lists duplicated inside lib.sh
|
||
Claude ready-tokens `'Anthropic|Assistant|Chat|Welcome|projects'` at **both** `lib.sh:1058` (`wait_for_tui_ready`) and `lib.sh:1205` (`handle_startup_dialogs`); trust-dialog tokens split between `_pane_dialog_open` (`:1137-1139`) and `handle_startup_dialogs`' specific greps (`:1199,1201`). Token-list drift between two grep sites is **precisely the class of defect just fixed in the prompt-lock round** — next TUI release, someone updates one list and not the other. **Proposal**: single-source constants near the top of lib.sh —
|
||
```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'
|
||
```
|
||
plus a `_ready_regex_for <agent>` case-helper so `wait_for_tui_ready`'s per-agent regexes live in one lookup. All grep sites reference the variables.
|
||
|
||
### D4 — `wait_for_tui_ready` predates its own library's capture helpers
|
||
It hand-builds `local_tmux` and calls raw `capture-pane` instead of `_pane_capture`/`_pane_tail`. Folding it onto the helpers (with S3's single-capture-per-iteration) deletes ~8 lines and closes D1's second row for free.
|
||
|
||
### D5 (micro) — `stop_session.sh:128-129`: two `python3 -c` processes to read two JSON fields from the same `$MAPPED_DATA`. One process printing both (`'…; d=json.load(sys.stdin); print(d.get("cwd",""), d.get("job_id",""), sep="\t")'`) halves the fork cost; or add a tiny `json_get` helper to lib.sh if more call sites appear.
|
||
|
||
---
|
||
|
||
## Focus 3 — Portability & POSIX Compliance
|
||
|
||
Shebang discipline means raw-`sh` execution is not a real exposure; the genuine gaps are three, and two are **already roadmapped** — listed here with confirmations, not double-counted as new:
|
||
|
||
### P1 (= FW-P1 / FW-D3, confirmed at `lib.sh:254-255`)
|
||
```bash
|
||
mountpoint="$(df --output=target "$f" 2>/dev/null | tail -1)" || return 1
|
||
if mount | grep -q "$mountpoint.*nfs|…"
|
||
```
|
||
GNU-only `df --output` + Linux `mount` output format. On macOS/BSD, `df` errors → suppressed by `2>/dev/null` → `_check_is_nfs` silently returns "not NFS" → **the NFS/WAL safety switch is dead exactly where flock is least reliable**. Portable replacement: `mountpoint="$(df -P "$f" 2>/dev/null | awk 'NR==2{print $6}')"` (`df -P` is POSIX) and probe filesystem type via `stat -f -c %T` on Linux / `stat -f %T` on BSD behind a `case "$(uname)"` — or at minimum log a warning when detection is unavailable instead of silently passing.
|
||
|
||
### P2 (= FW-P5, confirmed at `lib.sh:17`)
|
||
`SKILL_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"` — under zsh sourcing (documented agent workflow: `source .agents/skills/lib.sh`), `BASH_SOURCE` is empty → `dirname ""` → `.` → `SKILL_DIR` silently becomes the caller's cwd, and every relative resolution downstream (`:950`, `:966`) misroutes. **Proposal (fail-loud, 3 lines at the top of lib.sh)**:
|
||
```bash
|
||
if [ -z "${BASH_SOURCE:-}" ]; then
|
||
echo "lib.sh must be sourced from bash (zsh/sh detected)" >&2; return 1 2>/dev/null || exit 1
|
||
fi
|
||
```
|
||
Silent misresolution becomes an immediate, explained failure. (Full zsh support via `${(%):-%N}` is possible but not worth the dual-dialect maintenance.)
|
||
|
||
### P3 (= FW-P6, confirmed) — depth-hardcoded root resolution
|
||
`status.sh:29` (`…/../../../../`), `reconcile.sh:48`, and the `../..` sourcing prologue in all 6 skill scripts. One directory-layout refactor breaks all of them at once. FW-P6's marker-walk (`find_workspace_root()` ascending to `.git`/`.mam`/`.env`, exported once as `WORKSPACE_ROOT`) remains the right fix; the sourcing prologues can stay relative (they express a true structural invariant *within* the skills tree) — it's the **workspace-root** hops that should go through the marker walk.
|
||
|
||
### P4 (non-issues, verified): fractional `sleep 0.5/0.25` (GNU+BSD+busybox all accept), `head -c`/`tail -c`/`awk NF` (POSIX), no `grep -P`, no `sed -i`, no `readlink -f`, no `eval` — clean.
|
||
|
||
---
|
||
|
||
## Focus 4 — Error Handling & Robustness
|
||
|
||
### E1 (HIGHEST SEVERITY in this audit) — `reconcile.sh:269`: degraded mode is fully silent
|
||
```bash
|
||
bash "$_self" --once --emit-diff >/dev/null 2>&1 || true
|
||
```
|
||
This runs *only* in the broker-down fallback — the mode whose entire purpose is "keep reconciling when eventing is gone" — and it discards stdout, stderr, **and** the exit code. If reconciliation itself is failing every cycle (locked DB, missing python module, corrupted YAML), the operator sees a healthy-looking monitor while drift accumulates unboundedly. **Proposal**:
|
||
```bash
|
||
if ! out=$(bash "$_self" --once --emit-diff 2>&1); then
|
||
fails=$((fails + 1))
|
||
echo "[$(date -u +%FT%TZ)] poll-reconcile failed ($fails consecutive): ${out##*$'\n'}" >&2
|
||
[ "$fails" -ge 5 ] && { echo "FATAL: 5 consecutive reconcile failures — exiting for supervisor restart" >&2; exit 1; }
|
||
else
|
||
fails=0
|
||
fi
|
||
```
|
||
|
||
### E2 — deliberate vs. accidental suppression (survey result)
|
||
The 18 `|| true` / 31 `2>/dev/null` sites were individually reviewed. The large majority are **legitimate idempotency guards** (stop's kill-chain probing possibly-absent sessions; `_pane_capture`'s probe semantics) — no action. The accidental class is E1 (above) and E4 (below).
|
||
|
||
### E3 — `stop_session.sh:128-129`: unguarded JSON parse under `set -e`, no EXIT trap
|
||
Malformed registry JSON kills the stop mid-flight with a bare Python traceback; unlike `create_session.sh`, `stop_session.sh` has **no cleanup/context trap**, so the operator gets no indication of what state the stop reached (exitkey sent? captured? row updated?). Cheap fix: wrap the parse (`… || { echo "ERROR: corrupt registry row for '$SESSION_NAME' — run monitor reconcile" >&2; exit 1; }`) and add a minimal `trap 'echo "stop aborted at stage $STAGE" >&2' ERR` with a `STAGE` variable advanced at each phase.
|
||
|
||
### E4 — delegate-job subscriber spawned fire-and-forget
|
||
`"$PY" job_subscriber.py … >"$logf" 2>&1 &` followed only by `sleep 1`: if the subscriber dies instantly (bad `--registry-dir`, missing paho-mqtt in the venv), the wrapper proceeds, the agent publishes into the void, and the job "runs" with zero audit trail until timeout. The S2 SUBACK-sentinel handshake converts this to a loud early failure — one fix, two findings (S2+E4).
|
||
|
||
### E5 — shellcheck backlog (6 warnings, pre-existing)
|
||
`SC2034` ×2 (`ONCE`, `EMIT_DIFF` "unused" in reconcile.sh — likely consumed inside the python heredoc via env; verify and either export or rename with `_` prefix to document intent), `SC2155` ×2, `SC2164` ×2 (`cd` without `|| exit` — real hazard under odd cwd removal). All are ≤2-line fixes; clearing them makes future "0 new findings" review gates strict.
|
||
|
||
---
|
||
|
||
## Prioritized Optimization Plan
|
||
|
||
| # | Item | Files | Effort | Impact |
|
||
|---|---|---|---|---|
|
||
| 1 | E1 silent degraded loop → logged + bounded failures | reconcile.sh | S | Correctness/observability of the safety net |
|
||
| 2 | S2+E4 SUBACK sentinel handshake replaces both `sleep 1` | delegate-job wrapper, job_subscriber.py | M | Eliminates event-loss race on slow brokers; loud subscriber failures |
|
||
| 3 | S1 `_wait_session_gone` reactive stop | lib.sh, stop_session.sh | S | ~7 s faster stops; no false SIGTERM escalation |
|
||
| 4 | D1+D4 `mam_tmux()` unification (retire 3 string variants) | lib.sh, create_session.sh, delegate-job | S | Single source of truth; kills word-split hazard |
|
||
| 5 | D3 token-list constants + `_ready_regex_for` | lib.sh | S | Prevents recurrence of the F1-class drift bug |
|
||
| 6 | P2 fail-loud non-bash source guard | lib.sh | S | Converts silent misresolution to instant diagnosis (FW-P5) |
|
||
| 7 | E3 stop parse guard + stage trap | stop_session.sh | S | Debuggable partial-stop states |
|
||
| 8 | D2 delegate-job block extraction | delegate-job wrapper | M | Stops copy drift (already observable) |
|
||
| 9 | P1 POSIX NFS detection (FW-P1/FW-D3) | lib.sh | M | Restores the WAL safety switch on macOS/BSD |
|
||
| 10 | P3 marker-walk root resolution (FW-P6) | lib.sh, status.sh, reconcile.sh | M | Layout-refactor resilience |
|
||
| 11 | E5 shellcheck backlog + D5 micro | reconcile.sh, lib.sh, stop_session.sh | S | Strict lint gate for future reviews |
|
||
|
||
Items 1–7 are low-risk and independently landable; 9–10 discharge existing roadmap entries (FW-P1/FW-D3/FW-P5/FW-P6 — update FUTURE_WORKS on landing). Every DoD should include the scratch-server functional suite from the prompt-lock review (T-A…T-E) plus `bash -n` + shellcheck zero-new.
|
||
|
||
## Summary
|
||
|
||
The tree is in good structural shape — consistent strict-mode headers, no eval, and the recent FW-W2 work already modernized the highest-risk delivery path. The remaining debt clusters into: two **timing bets** that should be handshakes (stop waits, MQTT subscribe), one **silent failure mode** in exactly the code path that exists for resilience (reconcile fallback), and **four copies** of the tmux-server rule plus **two copies** of the TUI token lists — the same drift pattern that caused the last production bug. Eleven changes, mostly small, none speculative.
|