tmpl/multi-agent-mux
1
0 Fork 0
Code Issues 9 Pull Requests Actions Packages Projects Releases Wiki Activity
Files
e32c9574644a9185813a4dc881812694d62b6204
multi-agent-mux/.agents/reports/reviewer-agy-01/report-4be97273.md
T
GodopuandClaude Opus 5.5 121eab71d4 release(v4.9.3): u1e no longer writes the checkout's .mam/shim/herdr - archive the review reports and bump the lockstep version 
Review loop (--review-only, --all-reviewer): round 1 (@3a30d7d) 4/4 PASS;
each reviewer reproduced W1 failing on the pre-fix tree. Seven #50/#55 and
leak modules (112 tests) from a clean worktree left no .mam in the
checkout.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
2026-10-13 12:33:02 +09:00

87 lines
6.9 KiB
Markdown
Raw Blame History

# Code Review Report: `fix/u1e-workspace-root` (Test Isolation Fix for `test_u1e`)
- **Job ID**: `4be97273`
- **Reviewer**: `agy` (Session: `herdr:reviewer-agy-01`, Role: `Reviewer`)
- **Date**: 2026-10-13
- **Target Branch**: `fix/u1e-workspace-root`
- **Base Commit**: `42e2a7ea9bd3e2770ed8626d789193428ca3aa8b`
- **Head Commit**: `3a30d7da894cdac26b7ec054b4e66fe9fa3dc8db`
- **Verification Path**: `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T025803Z-c69408/1`
- **Verification Environment**: macOS (Darwin 24.3.0, arm64), Python 3.9.6, pytest-8.4.2
---
## 1. Executive Summary
This review evaluated commit [`3a30d7da894cdac26b7ec054b4e66fe9fa3dc8db`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T025803Z-c69408/1), which fixes a test workspace isolation leak discovered during earlier review runs in [`tests/test_issue50_dispatch.py::test_u1e_a_dry_run_record_is_not_an_open_job`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T025803Z-c69408/1/tests/test_issue50_dispatch.py#L162-L174) and adds regression coverage to [`tests/test_issue17_workspace_root_isolation.py`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T025803Z-c69408/1/tests/test_issue17_workspace_root_isolation.py).
The root cause was confirmed, the fix was verified both positively and negatively (reproducing failure on base, success on head), and an exhaustive audit across `tests/` confirmed no other tests suffer from this defect.
**Verdict: PASS**.
---
## 2. Root Cause Analysis
In [`tests/test_issue50_dispatch.py`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T025803Z-c69408/1/tests/test_issue50_dispatch.py), `test_u1e_a_dry_run_record_is_not_an_open_job` invokes `subprocess.run` on `WRAPPER` (`multi-agent-mux-delegate-job`).
1. `WRAPPER` sources [`.agents/skills/lib.sh`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T025803Z-c69408/1/.agents/skills/lib.sh).
2. In `lib.sh:26`, if `WORKSPACE_ROOT` is unset in the execution environment, it defaults to:
```bash
WORKSPACE_ROOT="${WORKSPACE_ROOT:-$(cd "$SKILL_DIR/../.." && pwd)}"
```
which points to the repository checkout root.
3. In `lib.sh:149-155` and `lib.sh:3168`, `_init_herdr_isolation` automatically creates and populates `$WORKSPACE_ROOT/.mam/shim/herdr`.
4. Because `u1e` previously created a custom `env` containing `HOME` and `MAM_ENV_FILE` but omitted `WORKSPACE_ROOT`, executing `u1e` wrote directly into `<checkout>/.mam/shim/herdr`.
---
## 3. Verification of Fix & Regression Coverage
### 3.1 The Fix
In [`tests/test_issue50_dispatch.py:169-170`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T025803Z-c69408/1/tests/test_issue50_dispatch.py#L169-L170), `WORKSPACE_ROOT=str(tmp_path)` was added to `env`:
```python
env = dict(os.environ, HOME=str(tmp_path), MAM_ENV_FILE=str(tmp_path / ".mam.env.absent"),
WORKSPACE_ROOT=str(tmp_path))
```
This guarantees that any `.mam/shim` creation is strictly contained within `tmp_path`.
In [`tests/test_issue17_workspace_root_isolation.py:43`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T025803Z-c69408/1/tests/test_issue17_workspace_root_isolation.py#L43), `"tests/test_issue50_dispatch.py::test_u1e_a_dry_run_record_is_not_an_open_job"` was appended to `NODES`. Test W1 copies the checkout into a temporary sandbox and executes every node in `NODES`, asserting that no `.mam` directory is created in the copy.
### 3.2 Negative Reproduction (Pre-fix Validation)
In an isolated temporary clone checked out at base commit `42e2a7ea9bd3e2770ed8626d789193428ca3aa8b`, `u1e` was added to `NODES` before the fix. Running W1 failed immediately with:
```text
AssertionError: tests wrote into the checkout they ran in: ['.mam/shim', '.mam/shim/herdr']
assert ['.mam/shim',...m/shim/herdr'] == []
FAILED tests/test_issue17_workspace_root_isolation.py::test_w1_no_test_writes_into_the_checkout_it_runs_in
```
This confirms that W1 effectively catches the defect.
### 3.3 Positive Execution (Post-fix Validation)
Ran [`tests/test_issue17_workspace_root_isolation.py`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T025803Z-c69408/1/tests/test_issue17_workspace_root_isolation.py) in the verification directory with sanitized environment and isolated `--basetemp`:
```text
tests/test_issue17_workspace_root_isolation.py .. [100%]
============================== 2 passed in 30.47s ==============================
```
Verified that no `.mam` was created in the verification tree (`ls: .mam: No such file or directory`, git status clean).
---
## 4. Codebase Audit for Similar Invocations
An automated search across all test modules in [`tests/`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T025803Z-c69408/1/tests) identified all other locations where `WRAPPER` or `lib.sh` are invoked:
- [`test_issue14_delivery_ack.py`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T025803Z-c69408/1/tests/test_issue14_delivery_ack.py): `_base_env` explicitly sets `env["WORKSPACE_ROOT"] = str(cwd)`.
- [`test_issue17_item4_decisions.py`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T025803Z-c69408/1/tests/test_issue17_item4_decisions.py): `_env` explicitly sets `WORKSPACE_ROOT=str(tmp_path)`.
- [`test_issue18_liveness_contract.py`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T025803Z-c69408/1/tests/test_issue18_liveness_contract.py): `_env` explicitly sets `env["WORKSPACE_ROOT"] = str(cwd)`.
- [`test_issue26_envfile.py`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T025803Z-c69408/1/tests/test_issue26_envfile.py): `_env` explicitly sets `"WORKSPACE_ROOT": str(home)`.
- [`test_issue26_ints.py`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T025803Z-c69408/1/tests/test_issue26_ints.py): `_base_env` explicitly sets `"WORKSPACE_ROOT": str(home)`.
- [`test_issue33_budget.py`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T025803Z-c69408/1/tests/test_issue33_budget.py): `_env` explicitly sets `env["WORKSPACE_ROOT"] = str(home)`.
- [`test_issue50_dispatch_herdr.py`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T025803Z-c69408/1/tests/test_issue50_dispatch_herdr.py): `Box.env` explicitly sets `WORKSPACE_ROOT=str(self.ctl)`.
- [`test_issue6_turn_handshake.py`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261013T025803Z-c69408/1/tests/test_issue6_turn_handshake.py): `_run_lib_sh` and `_run_wrapper` explicitly set `run_env["WORKSPACE_ROOT"] = str(cwd)`.
- Modules using `mam_sandbox` or `own_workspace_root` fixtures: correctly receive isolated `WORKSPACE_ROOT`.
No other un-isolated invocations exist in the test suite.
---
[VERDICT: PASS]
Reference in new issue View Git Blame Copy Permalink