# Code Review Report (2nd Round): `fix/test-herdr-leak`

- **Job ID**: `e375099a`
- **Reviewer**: `agy` (Session: `herdr:reviewer-agy-01`, Role: `Reviewer`)
- **Date**: 2026-10-13
- **Target Branch**: `fix/test-herdr-leak`
- **Base Commit**: `325b056057fcdd717d2c81017fc5ef57183cb4d0` (1st round review head)
- **Head Commit**: `db7f4cab9c60ff5db009cf3fc625e9a86a465ef6` (Fix commit)
- **Verification Path**: `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T010208Z-f90dd4/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 2nd-round review evaluated commit `db7f4cab9c60ff5db009cf3fc625e9a86a465ef6`, addressing non-blocking feedback from 1st-round reviews (claude `63919c5d` and grok `191f0ec3`):
- **Finding**: While `HERDR_SOCKET_PATH`, `HERDR_PANE_ID`, and `HERDR_TAB_ID` were stripped in `mam_sandbox`, `HERDR_SESSION` was still inherited from the ambient host environment.
- **Impact**: Although `HERDR_SESSION` on its own resolves under `$HOME/.config/herdr/sessions/<session>` (so sandboxing `$HOME` prevented socket file leaks), ambient-server helpers like [`_herdr_caller_server`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T010208Z-f90dd4/1/.agents/skills/lib.sh#L1453) and [`pane_lock.caller_server()`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T010208Z-f90dd4/1/.agents/skills/lib_py/pane_lock.py#L671) read `HERDR_SESSION` first. Consequently, tests branched on whether they were executed from within an agent pane or outside a pane.
- **The Fix**:
  1. [`tests/conftest.py:mam_sandbox`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T010208Z-f90dd4/1/tests/conftest.py#L296): added `monkeypatch.delenv("HERDR_SESSION", raising=False)`.
  2. [`tests/test_herdr_real_server_leak.py`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T010208Z-f90dd4/1/tests/test_herdr_real_server_leak.py#L64): added `"HERDR_SESSION": "real"` to `PANE_VARS`, covered by `test_l2_sandbox_drops_the_pane_routing`.

**Verdict: PASS**.

---

## 2. Technical Evaluation

### 2.1 Environmental Isolation & Branching Consistency
- In `lib_py/pane_lock.py:caller_server()`:
  ```python
  def caller_server(env: Mapping[str, str]) -> str:
      if env.get("HERDR_SESSION"):
          return env["HERDR_SESSION"]
  ```
  And in `lib.sh`:
  `HERDR_SESSION` indicates that the current process runs inside an active herdr pane.
  By stripping `HERDR_SESSION` in `mam_sandbox`, every test begins with a clean non-pane baseline environment. Test behavior is now strictly reproducible regardless of whether the test runner runs inside an interactive terminal, CI, or an autonomous agent herdr session.

### 2.2 Test Compatibility
- Tests that explicitly test `HERDR_SESSION` configuration (e.g. `test_issue50_dispatch_herdr.py:769`, `test_issue45_restore.py`, `test_tier1_unit.py`) set the environment variable directly within the test body or pass it via `env=...` in subprocess calls. Because test bodies execute after fixture setup, their explicit settings take precedence and function properly.
- All 6 unit tests in `tests/test_issue50_pane_lock.py` (`test_p1` to `test_p5`) and contract tests in `tests/test_herdr_shim_contract.py` pass cleanly.

---

## 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 HERDR_SESSION -u WORKSPACE_ROOT -u MAM_ENV_FILE`):

| Test Suite / Target | Result | Execution Time |
|---|---|---|
| `tests/test_herdr_real_server_leak.py` | **3 Passed** | 1.44s |
| `tests/test_herdr_shim_contract.py:test_h26` | **2 Passed** | 3.98s |
| `tests/test_issue50_pane_lock.py:test_p*` | **6 Passed** | 4.57s |

- `test_l2_sandbox_drops_the_pane_routing`: Verifies that `HERDR_SESSION` is stripped by `mam_sandbox` alongside `HERDR_SOCKET_PATH`, `HERDR_PANE_ID`, `HERDR_TAB_ID`, `HERDR_WORKSPACE_ID`, and `HERDR_SESSION_NAME`.

---

[VERDICT: PASS]