tmpl/multi-agent-mux
1
0 Fork 0
Code Issues 8 Pull Requests Actions Packages Projects Releases Wiki Activity

release(v4.9.1): tests no longer reach a real herdr server - archive the review reports and bump the lockstep version

Browse Source 
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
This commit is contained in:
GodopuandClaude Opus 5.5 committed 2026-10-13 10:33:04 +09:00
1 parent db7f4cab9c
commit 50c8da8269
32 files changed
+607 -26
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

.agents/reports/reviewer-agy-01/report-a86376f9.identity.json
+14
View File
@@ -0,0 +1,14 @@
{
"loop_id": "loop-20261013T001819Z-131fa6",
"base_sha": "8b0b4a07dc76a4915895bdde6170dc8eed9b5f74",
"head_sha": "325b056057fcdd717d2c81017fc5ef57183cb4d0",
"tree_sha": "7905896f005159d596e3872d0ee2c3c12061f09a",
"review_commit": "325b056057fcdd717d2c81017fc5ef57183cb4d0",
"reviewer": "reviewer-agy-01",
"job_id": "a86376f9",
"verdict": "PASS",
"transport": "herdr",
"evidence": "review-tree",
"isolation": "worktree",
"verified_at": "2026-10-13T00:57:46Z"
}
.agents/reports/reviewer-agy-01/report-a86376f9.md
+87
View File
@@ -0,0 +1,87 @@
# 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]
.agents/reports/reviewer-agy-01/report-e375099a.identity.json
+14
View File
@@ -0,0 +1,14 @@
{
"loop_id": "loop-20261013T010208Z-f90dd4",
"base_sha": "325b056057fcdd717d2c81017fc5ef57183cb4d0",
"head_sha": "db7f4cab9c60ff5db009cf3fc625e9a86a465ef6",
"tree_sha": "f32cabd3a6c8d775c9223509967f1a810f0cf827",
"review_commit": "db7f4cab9c60ff5db009cf3fc625e9a86a465ef6",
"reviewer": "reviewer-agy-01",
"job_id": "e375099a",
"verdict": "PASS",
"transport": "herdr",
"evidence": "review-tree",
"isolation": "worktree",
"verified_at": "2026-10-13T01:21:50Z"
}
.agents/reports/reviewer-agy-01/report-e375099a.md
+60
View File
@@ -0,0 +1,60 @@
# 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]