GodopuandClaude Opus 5.5
50c8da8269
Review loop (--review-only, --all-reviewer): round 1 (@325b056) 4/4 PASS,
round 2 (@db7f4ca, HERDR_SESSION) 4/4 PASS. Full suite at 325b056 on
macOS / Python 3.9 / bash 3.2 / herdr 0.9.3 from an agent pane:
1845 passed / 0 failed / 56 skipped (18 test_deploy_freshness skips: no
nats-docker submodule in the fresh worktree); the panes of the real
servers (default, multi-agent-mux) and ~/.claude/projects were unchanged.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
88 lines
6.4 KiB
Markdown
88 lines
6.4 KiB
Markdown
# Code Review Report: `fix/test-herdr-leak`
|
|
|
|
- **Job ID**: `a86376f9`
|
|
- **Reviewer**: `agy` (Session: `herdr:reviewer-agy-01`, Role: `Reviewer`)
|
|
- **Date**: 2026-10-13
|
|
- **Target Branch**: `fix/test-herdr-leak`
|
|
- **Base Commit**: `8b0b4a07dc76a4915895bdde6170dc8eed9b5f74` (`main`)
|
|
- **Head Commit**: `325b056057fcdd717d2c81017fc5ef57183cb4d0`
|
|
- **Verification Path**: `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T001819Z-131fa6/1`
|
|
- **Verification Environment**: macOS (Darwin 24.3.0, arm64), Python 3.9.6, bash 3.2.57(1)-release, `herdr` 0.9.3
|
|
|
|
---
|
|
|
|
## 1. Executive Summary
|
|
|
|
This code review evaluated the fixes in `fix/test-herdr-leak` (`8b0b4a0..325b056`, 2 commits, 4 files), resolving an incident where test suites executed from an agent pane with `--basetemp .../pytest-pr56-shim` inadvertently created 7 workspaces on the pane's real herdr server (`multi-agent-mux`).
|
|
|
|
### Root Causes Addressed
|
|
1. **Overspecified `-shim` Pattern Match**:
|
|
In [`lib.sh:_is_shim_path`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T001819Z-131fa6/1/.agents/skills/lib.sh#L53) and the generated shim heredoc, the pattern `*-shim/*` matched *any* path having `-shim/` anywhere in its ancestor hierarchy. When pytest was invoked with `--basetemp .../pytest-pr56-shim`, any mock herdr binary placed under `$basetemp/.../bin/herdr` was misclassified as a shim and skipped. The real-herdr resolver continued down `$PATH` until finding system `/opt/homebrew/bin/herdr`.
|
|
2. **Host Socket Leaked Through `mam_sandbox`**:
|
|
`mam_sandbox` previously unset `HERDR_SESSION_NAME`, `HERDR_SERVER_NAME`, and `HERDR_WORKSPACE_ID`, but left `HERDR_SOCKET_PATH`, `HERDR_PANE_ID`, and `HERDR_TAB_ID` untouched. In `herdr` 0.9.3, `HERDR_SOCKET_PATH` overrides `$HOME`, so any unintended call to the real herdr binary connected directly to the host pane's herdr daemon socket.
|
|
|
|
### The Fix
|
|
1. In [`lib.sh`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T001819Z-131fa6/1/.agents/skills/lib.sh):
|
|
- Refined `_is_shim_path()`: removed `*-shim/*` from substring globbing and added `case "${1%/}" in *-shim) return 0 ;; esac` so only directories whose leaf name matches `*-shim` are considered shims.
|
|
- Passed `$(dirname "$cand")` into `_is_shim_path` in `_resolve_real_herdr_path`.
|
|
- Updated generated shim heredoc: replaced `[[ "$dir" != *"-shim"* ]]` with `[[ "${dir%/}" != *-shim ]]`.
|
|
2. In [`tests/conftest.py:mam_sandbox`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T001819Z-131fa6/1/tests/conftest.py#L293):
|
|
- Explicitly strips `HERDR_SOCKET_PATH`, `HERDR_PANE_ID`, and `HERDR_TAB_ID`.
|
|
3. In [`tests/test_issue23_24_startup_path.py`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T001819Z-131fa6/1/tests/test_issue23_24_startup_path.py):
|
|
- Updated the generated shim byte-identity pin to base commit `525df2b`.
|
|
4. In [`tests/test_herdr_real_server_leak.py`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T001819Z-131fa6/1/tests/test_herdr_real_server_leak.py):
|
|
- Added unit and regression tests reproducing both leak vectors and verifying proper resolution.
|
|
|
|
**Verdict: PASS**.
|
|
|
|
---
|
|
|
|
## 2. Technical Evaluation
|
|
|
|
### 2.1 Diagnosis & Resolution Accuracy
|
|
- **Pattern Match Tightening**:
|
|
Testing with mock binaries located under `/path/to/pytest-x-shim/case0/bin/herdr` confirms that:
|
|
- `$(dirname "$cand")` is `/path/to/pytest-x-shim/case0/bin`.
|
|
- Leaf directory name is `bin`, which does not match `*-shim`.
|
|
- The mock binary is accepted and executed; system `herdr` is never reached.
|
|
- Dedicated wrapper directories whose leaf name is `*-shim` (e.g. `tmp / "herdr-shim" / "herdr"`) are still properly skipped.
|
|
- **Routing Variable Cleansing**:
|
|
Unsetting `HERDR_SOCKET_PATH`, `HERDR_PANE_ID`, and `HERDR_TAB_ID` eliminates the socket bypass vector in herdr 0.9.3. If a test ever fails to mock herdr, the real binary fails with `server_not_running` rather than attaching to the active multi-agent session.
|
|
|
|
### 2.2 Secondary Leak Paths & Environmental Isolation
|
|
- All known `HERDR_*` variables are accounted for in `mam_sandbox`:
|
|
- `HERDR_SESSION_NAME`, `HERDR_SERVER_NAME`, `HERDR_WORKSPACE_ID`
|
|
- `HERDR_SOCKET_PATH`, `HERDR_PANE_ID`, `HERDR_TAB_ID`
|
|
- `HERDR_CONFIG_PATH`, `XDG_CONFIG_HOME`
|
|
- Non-sandbox integration tests (such as [`tests/test_issue50_dispatch_herdr.py`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T001819Z-131fa6/1/tests/test_issue50_dispatch_herdr.py)) explicitly scrub all environment variables starting with `HERDR_` and isolate `$HOME` under `/tmp`.
|
|
|
|
### 2.3 Exclusion of Real Shim Directories
|
|
The narrowed `_is_shim_path` pattern continues to exclude all designated shim locations:
|
|
- `*/.mam/shim/*`: matched by `*"/.mam/shim/"*`.
|
|
- `multi-agent-herdr-shim`: matched by `*"$_HERDR_SHIM_DIR_PATTERN"*`.
|
|
- `.agents/skills/.bin`: matched by `*"$_HERDR_SKILLS_BIN_PATTERN"/*`.
|
|
- Arbitrary test wrapper directories ending in `-shim`: matched by `${1%/}` -> `*-shim`.
|
|
|
|
### 2.4 Control Plane Impact
|
|
The generated shim in `.mam/shim/herdr` is modified only to use `[[ "${dir%/}" != *-shim ]]`. The byte-identity assertion in [`test_issue23_24_startup_path.py::test_s11`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T001819Z-131fa6/1/tests/test_issue23_24_startup_path.py#L200) was updated to compare against commit `525df2b`, consistent with historical updates for intentional shim modifications (#45).
|
|
|
|
---
|
|
|
|
## 3. Test Verification Results
|
|
|
|
All tests executed with sanitized environment variables (`env -u HERDR_SOCKET_PATH -u HERDR_PANE_ID -u HERDR_TAB_ID -u HERDR_WORKSPACE_ID -u HERDR_SESSION_NAME -u WORKSPACE_ROOT -u MAM_ENV_FILE`):
|
|
|
|
| Test Suite / Target | Result | Execution Time |
|
|
|---|---|---|
|
|
| `tests/test_herdr_real_server_leak.py` (New regression suite) | **3 Passed** | 1.39s |
|
|
| `tests/test_issue23_24_startup_path.py` (Full suite, 70 tests) | **70 Passed** | 141.34s |
|
|
| `tests/test_herdr_shim_contract.py` (Full suite, 18 tests) | **18 Passed** | 27.48s |
|
|
|
|
- `test_l1_ancestor_named_shim_does_not_hide_the_mock`: Verifies mock binary under `...-shim/.../bin` is used, decoy is not touched.
|
|
- `test_l1_a_directory_named_shim_is_still_skipped`: Verifies wrapper directories named `*-shim` remain excluded.
|
|
- `test_l2_sandbox_drops_the_pane_routing`: Verifies `mam_sandbox` purges all pane routing variables.
|
|
|
|
---
|
|
|
|
[VERDICT: PASS]
|