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
61 lines
4.1 KiB
Markdown
61 lines
4.1 KiB
Markdown
# 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]
|