# 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]