# Review Report: Issue #17 Workspace Root Isolation Followup

- **Reviewer Agent**: `agy` (session: `herdr:reviewer-agy-01`)
- **Job ID**: `2bd707b3`
- **Verification Directory**: `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261008T133858Z-9e7f5b/1`
- **Base Commit**: `6b00b6e06f55299cb60f156ccef09ede4bae5bdd` (main)
- **Head Commit**: `d198dd50aaf6119dc21f36d88704e64850e67413` (test/issue17-workspace-root)
- **Delta Under Inspection**: Commit `d198dd5` (`git diff 6b00b6e d198dd5`)

---

## 1. Executive Summary

This review evaluates the followup fix addressing claude's non-blocking feedback from the v4.8.7 review: preventing test execution from inheriting `WORKSPACE_ROOT` and inadvertently writing `.mam/shim/herdr` into the active workspace, worktree, or checkout.

Commit `d198dd5` updates `tests/test_issue17_item4_decisions.py:_env` to strictly exclude `WORKSPACE_ROOT` from `os.environ` and explicitly set `WORKSPACE_ROOT=str(tmp_path)`. It introduces a dedicated regression test (`test_c_helpers_write_nothing_to_an_inherited_workspace`) verifying that an ambient `WORKSPACE_ROOT` in the environment receives zero file mutations.

All 4 requirements from the brief were audited and verified. Dual-condition test runs (with `WORKSPACE_ROOT` unset, and with `WORKSPACE_ROOT` set to a sentinel directory) both passed completely (**290 passed**), with the sentinel directory remaining 100% empty. Auxiliary suites (`test_issue33_budget.py`, `test_issue5_phase1_delegation.py`, `test_tier1_unit.py`) passed cleanly (**443 passed**). Product code remains untouched.

---

## 2. In-Depth Evaluation of Requirements

### Requirement 1: `_env` Isolation in `tests/test_issue17_item4_decisions.py`
In [`tests/test_issue17_item4_decisions.py:_env`](file:///tests/test_issue17_item4_decisions.py#L222-L233):
```python
def _env(tmp_path: Path, **extra) -> dict:
    # WORKSPACE_ROOT is this test's tmp_path, never inherited: sourcing the
    # wrapper sources lib.sh, which rewrites $WORKSPACE_ROOT/.mam/shim/herdr -
    # and unset, lib.sh would derive the checkout itself (Issue #17 follow-up).
    env = {k: v for k, v in os.environ.items()
           if not k.startswith(("HERDR_", "MAM_", "SKS_")) and k not in ("PYTHONPATH", "WORKSPACE_ROOT")}
    env.update(MAM_DELEGATE_JOB_NO_MAIN="1", MAM_ENV_FILE=str(tmp_path / ".mam.env.absent"), HOME=str(tmp_path),
               WORKSPACE_ROOT=str(tmp_path), DELEGATE_JOB_LOGS_DIR=str(tmp_path / "logs"))
    env.update({k: str(v) for k, v in extra.items() if v is not None})
    return env
```
- **Rationale for Setting `WORKSPACE_ROOT=str(tmp_path)` vs Unsetting**:
  `lib.sh:26` specifies `WORKSPACE_ROOT="${WORKSPACE_ROOT:-...}"`. If `WORKSPACE_ROOT` is simply unset, `lib.sh` derives the repository checkout directory containing `.agents/skills` and writes `.mam/shim/herdr` there. Explicitly assigning `WORKSPACE_ROOT=str(tmp_path)` ensures `lib.sh` writes its shim inside the test's isolated temporary directory, shielding both other workspaces and the real checkout.

---

### Requirement 2: Survey of Other Tests for `WORKSPACE_ROOT` Leakage

An audit of all tests across the repository was conducted to inspect how `WORKSPACE_ROOT` and `os.environ` copies are managed:

| Test File & Line | Pattern | Protection Mechanism | Status |
|---|---|---|---|
| `tests/conftest.py:144-146` | `pytest_configure` | Drops `WORKSPACE_ROOT` only if it names *another* checkout (`os.path.realpath(ws) != str(_REPO_ROOT)`). When run inside an agent's worktree pane where `ws == _REPO_ROOT`, `WORKSPACE_ROOT` is kept by design for standard tests. | Normal Framework Behavior |
| `tests/conftest.py:215` | `mam_sandbox` fixture | Calls `monkeypatch.setenv("WORKSPACE_ROOT", str(tmp_path))`. Every test using `mam_sandbox` is automatically sandboxed. | **SAFE** (Sandboxed) |
| `tests/test_issue17_item4_decisions.py:224` | `_env` helper | Does *not* use `mam_sandbox` (lightweight tempdir runner). Previously copied `os.environ` without dropping `WORKSPACE_ROOT`. | **FIXED** (Commit `d198dd5`) |
| `tests/test_issue17_item4_decisions.py:694` | `_human_submit` | Explicitly excludes `WORKSPACE_ROOT` from `os.environ` and sets `WORKSPACE_ROOT=str(tmp_path)`. | **SAFE** (Isolated) |
| `tests/test_issue15_registry_isolation.py:75` | Explicit `env = dict(os.environ, WORKSPACE_ROOT=str(tmp_path))` | Explicitly overrides `WORKSPACE_ROOT` with `tmp_path`. | **SAFE** (Isolated) |
| `tests/test_issue20_state_isolation.py:150` | Control plane tests | Specifically tests control-plane isolation and foreign checkout dropping. | **SAFE** (Isolated) |
| `tests/test_issue18_liveness_contract.py:58` | Subprocess env | Explicitly sets `env["WORKSPACE_ROOT"] = str(cwd)`. | **SAFE** (Isolated) |
| `tests/test_loop_cli.py:97` | Loop CLI test | Uses `"WORKSPACE_ROOT": str(mam_sandbox)`. | **SAFE** (Isolated) |
| `tests/test_tier3_integration.py:346` | Tier 3 test | Sets `"WORKSPACE_ROOT": str(tmp_path)`. | **SAFE** (Isolated) |

**Conclusion**: `tests/test_issue17_item4_decisions.py` was the sole location where tests sourced `lib.sh` via subprocess without utilizing `mam_sandbox` or overriding `WORKSPACE_ROOT`. All other suites either utilize `mam_sandbox` or explicitly bind `WORKSPACE_ROOT` to temporary paths.

---

### Requirement 3: Regression Test Verification
[`tests/test_issue17_item4_decisions.py:test_c_helpers_write_nothing_to_an_inherited_workspace`](file:///tests/test_issue17_item4_decisions.py#L476-L509) introduces a regression test:
1. Creates a `sentinel-ws` directory and exports `WORKSPACE_ROOT=str(sentinel)`.
2. Records mtime of `REPO_ROOT/.mam/shim/herdr`.
3. Runs both `_run_both` and direct script sourcing with `_env(work)`.
4. Asserts:
   - `sentinel.rglob("*")` is empty (`assert sorted(...) == []`).
   - `REPO_ROOT/.mam/shim/herdr` mtime is unchanged (`assert _mtime(checkout_shim) == before`).
   - The test's own `work` directory receives `.mam/shim/herdr` (`assert (work / ".mam" / "shim" / "herdr").is_file()`).
5. In pre-fix code, `_env` would inherit the sentinel `WORKSPACE_ROOT`, writing `.mam/shim/herdr` into `sentinel` and failing assertion 1 and 3. The test is deterministic and reproduces the defect.

---

### Requirement 4: Verification Under Dual Conditions & Related Suites

Executed within the verification directory (`/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261008T133858Z-9e7f5b/1`):

#### 4.1. Condition (a): `WORKSPACE_ROOT` Explicitly Unset
```bash
AGENT_PYTHON_BIN=/Users/godopu16/PuKi/laa/canary_projects/multi-agent-mux-wt/issue17-wsroot/.venv/bin/python \
PYTHONDONTWRITEBYTECODE=1 \
env -u WORKSPACE_ROOT \
/Users/godopu16/PuKi/laa/canary_projects/multi-agent-mux-wt/issue17-wsroot/.venv/bin/python \
  -m pytest -p no:cacheprovider -q --basetemp=/tmp/pytest-wsroot-a \
  tests/test_issue17_item4_decisions.py
```
- **Result**: `290 passed in 314.26s (0:05:14)` (Exit code 0).

#### 4.2. Condition (b): `WORKSPACE_ROOT` Bound to External Sentinel Directory
```bash
AGENT_PYTHON_BIN=/Users/godopu16/PuKi/laa/canary_projects/multi-agent-mux-wt/issue17-wsroot/.venv/bin/python \
PYTHONDONTWRITEBYTECODE=1 \
WORKSPACE_ROOT=/tmp/test-sentinel-wsroot \
/Users/godopu16/PuKi/laa/canary_projects/multi-agent-mux-wt/issue17-wsroot/.venv/bin/python \
  -m pytest -p no:cacheprovider -q --basetemp=/tmp/pytest-wsroot-b \
  tests/test_issue17_item4_decisions.py
```
- **Result**: `290 passed in 319.46s (0:05:19)` (Exit code 0).
- **Sentinel Directory Verification**: `/tmp/test-sentinel-wsroot` remained completely empty (`total 0`).

#### 4.3. Auxiliary Suites
```bash
AGENT_PYTHON_BIN=/Users/godopu16/PuKi/laa/canary_projects/multi-agent-mux-wt/issue17-wsroot/.venv/bin/python \
PYTHONDONTWRITEBYTECODE=1 \
/Users/godopu16/PuKi/laa/canary_projects/multi-agent-mux-wt/issue17-wsroot/.venv/bin/python \
  -m pytest -p no:cacheprovider -q --basetemp=/tmp/pytest-wsroot \
  tests/test_issue17_item4_decisions.py \
  tests/test_issue33_budget.py \
  tests/test_issue5_phase1_delegation.py \
  tests/test_tier1_unit.py
```
- **Result**: `443 passed in 476.61s (0:07:56)` (Exit code 0).

---

## 3. Scope and Product Code Integrity

- Product code (`.agents/skills`) contains zero changes.
- Modifications are strictly confined to test infrastructure in `tests/test_issue17_item4_decisions.py` (+34 lines, -2 lines).

---

## 4. Conclusion & Final Verdict

The workspace root isolation issue is thoroughly resolved. Ambient or inherited workspace roots cannot be polluted, regression tests are deterministic, and all test suites pass.

[VERDICT: PASS]