multi-agent-mux/.agents/reports/reviewer-agy-01/report-e2420d1b.md

Peer Review Report: Branch fix/issue-19 (Comprehensive Review — Job e2420d1b)

• Reviewer: agy (session: herdr:reviewer-agy-01)
• Job ID: e2420d1b
• Role: Reviewer
• Target Branch: fix/issue-19
• Base Commit: 8e7282c (v4.4.4)
• Evaluated Commits:
  • ce315d8 — fix(issue-19): bound the subscriber daemon to its workspace, forward SIGTERM, keep sandboxes from leaking it
  • 38d8ccb — release(v4.4.5): bump framework and eight skills to 4.4.5 in lockstep
  • 4da4a50 — test(issue-19): run the sandbox reaper through an import-time Popen
  • c096cb4 — fix(issue-19): close the trap race, select daemons by argv shape, align docs with the polling fallback
  • 8f9745b — fix(issue-19): match the daemon's whole script path so spaced sandbox paths are reaped

---

1. Executive Summary

This peer review evaluated branch fix/issue-19 through its latest commit 8f9745b. The implementation addresses Issue #19, where subscriber daemons (reconcile.sh --subscribe --idle-timeout 0) spawned per workspace were leaking across test runs (202 orphan processes observed holding broker connections for deleted temp directories).

The complete branch includes the initial fix (ce315d8), version lockstep v4.4.5 (38d8ccb), and three peer-review hardening commits:

1. 4da4a50: Binds _REAL_POPEN = _subprocess.Popen at import time in conftest.py so test mocks cannot sabotage the sandbox reaper.
2. c096cb4: Closes the race window between background spawn and PID assignment via ${!:-} with nounset protection in bash 3.2, installs Python signal handlers before network operations, matches daemon processes strictly by argv shape to avoid false positives on prompt-bearing wrappers, and documents the polling fallback lock-presence contract.
3. 8f9745b: Extracts the full script path up to /reconcile.sh --subscribe so sandboxes or --basetemp paths containing whitespace are correctly identified and reaped.

All review criteria are satisfied with complete fidelity. A total of 144 test cases (19 targeted + 125 regression) executed with a 100% pass rate, zero orphan processes, and zero job registry leaks.

---

2. Detailed Technical Audit

2.1 Reconcile Shell Script (reconcile.sh)

• Background Execution & Return Code Contract:
  • Python subscriber runs in background: "$PYBIN" - <<'PYEOF' &; _sub_pid=$!; wait "$_sub_pid"; sub_rc=$?; _sub_pid=""; set -e.
  • Exit code 0 indicates normal exit; exit code 3 indicates broker unavailability and triggers polling fallback; lock contention returns 1 and exits cleanly.
• Trap Safety & Bash 3.2 Compatibility:
  • _sub_stop() handles TERM and INT.
  • In bash 3.2 (default /bin/bash on macOS), expanding $! before the first background child aborts with an unbound variable error under set -u. Commit c096cb4 temporarily unsets nounset (set +u; child="$!"; set -u), ensuring the trap reliably exits 143 even if a signal arrives before child launch.
  • Signal forwarding safely suppresses errors (kill -TERM "$child" 2>/dev/null || true; wait "$child" 2>/dev/null || true).
• Identity Comparison (st_dev, st_ino):
  • lock_ident = (_lock_st.st_dev, _lock_st.st_ino) checks both device and inode.
  • _workspace_gone() catches OSError on os.getcwd() and detects replaced or deleted .mam/monitor.lock files, terminating the daemon cleanly when its workspace is removed.
• Early Signal Handlers:
  • Signal handlers are installed before client.connect(...) and client.loop_start().
  • If SIGTERM arrives while awaiting CONNACK, stop_event.wait(0.1) catches it immediately, cleanly exiting 0 without triggering the broker-down fallback.
• Polling Fallback:
  • sleep "$POLL_INTERVAL" & wait $! || true allows immediate signal handling (exit 143).
  • Verifies presence of $WORKSPACE_ROOT and .mam/monitor.lock on each cycle.

2.2 Opt-Out Mechanism (daemon.py, lib.sh, cli.py)

• Literal Value Check:
  • daemon_disabled() requires literal "1". Loose values ("yes", "true", "0") do not disable the daemon.
• Deduplication & Output Parity:
  • Active daemons continue to be reported during opt-out.
  • When opting out without an existing daemon:
    • Python CLI prints nothing to stdout and emits a single note to stderr.
    • Legacy shell (lib.sh) only echoes when $pid is non-empty (if [ -n "$pid" ]; then echo "$pid"; fi), preventing extraneous blank lines on stdout and matching Python behavior byte-for-byte.

2.3 Conftest Sandboxing & Reaper Hardening

• Import-Time Popen (_REAL_POPEN):
  • Capturing _subprocess.Popen at import time insulates fixture teardown from test monkeypatching or spying of subprocess.run / subprocess.Popen.
• Argv-Shape Matching (_is_subscriber_daemon_argv):
  • Confirms shell executable (bash or sh), verifies path prefix under root, and regex matches (.*reconcile\.sh) --subscribe.
  • Excludes processes that merely quote the command line in prompt arguments (e.g. submit --prompt "...") or sibling roots (/sbx1 vs /sbx10).
  • Supports whitespace-containing paths by verifying os.path.isfile(script).
• Session Leak Guard:
  • subscriber_daemon_leak_guard scopes strictly to pytest basetemp, guaranteeing no daemon outlives the test suite.

---

3. Version Lockstep v4.4.5 Audit

All components remain strictly locked at 4.4.5:

• .agents/skills/lib.sh: MAM_VERSION="4.4.5"
• .agents/skills/lib_py/loop/eager.py: FRAMEWORK_VERSION = "4.4.5"
• Frontmatter version: 4.4.5 across all 8 skills (create, stop, resume, status, monitor, delegate-job, loop, orc-onboard).
• VERSIONS.md: Current release overview, 8-row skills matrix, and detailed v4.4.5 changelog accounting for all 16 lifecycle tests and architectural refinements.
• Badges and headers in README.md, README.ko.md, BOOTSTRAP.md, BOOTSTRAP.ko.md, and MESSAGING.md.
• Release commit title: release(v4.4.5): bump framework and eight skills to 4.4.5 in lockstep.
• Remaining occurrences of 4.4.4 in markdown files are purely historical.

---

4. Test Suite Execution & Audits

4.1 Targeted Suites

.venv/bin/python -m pytest tests/test_issue19_subscriber_daemon_lifecycle.py tests/test_version_consistency.py

• Result: 19 passed in 37.99s (16 lifecycle tests + 3 version consistency tests).

4.2 Regression Suites

.venv/bin/python -m pytest tests/test_issue5_phase1_delegation.py tests/test_tier2_component.py tests/test_herdr_shim_contract.py

• Result: 125 passed in 240.03s.

4.3 Process & Registry Integrity

• Orphan daemons under /private/var/folders: 0
• Real workspace monitor daemon: Preserved intact (PID 35505)
• Registry file count in .mam/jobs: 52 before and after (0 leaks)

---

5. Conclusion

Branch fix/issue-19 across commits ce315d8 through 8f9745b delivers a complete, robust, and well-verified solution to Issue #19. All criteria are fully satisfied.

[VERDICT: PASS]