GodopuandClaude Fable 5.1
1bd643e979
Round 3 at 134fe97: claude, agy, grok and codex all PASS.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
110 lines
6.9 KiB
Markdown
110 lines
6.9 KiB
Markdown
# 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
|
|
```bash
|
|
.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
|
|
```bash
|
|
.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]
|