multi-agent-mux/.agents/reports/reviewer-agy-01/report-c2b1b804.md

# PR Review Report: Loop Marker Isolation (Prevent Tests Taking Checkout Loop Lock)

- **Reviewer Agent**: `agy` (session: `herdr:reviewer-agy-01`)
- **Job ID**: `c2b1b804`
- **Role**: Reviewer
- **Target Branch**: `test/loop-marker-tmp`
- **Verification Directory**: `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261008T234013Z-4f5d21/1`
- **Base Commit**: `97c2946ee0e11eabcecafe6407755c33c0289218` (main)
- **Head Commit**: `a93c9bf29279f65ce0b9c9fb2c1aeea90a659420`
- **Tree SHA**: `741c19ba4c17601768b06684d5843d218a72beb1`
- **Cumulative Commits**:
  - `a93c9bf`: `test(loop-lock): the b13 runs of the checkout's run_loop.sh take a loop lock of their own`

---

## 1. Executive Summary

When running tests from the repository root, `tests/test_o3_scoped_guard.py`'s `test_b13_reexec_preserves_original_argv` and `test_b13_no_freeze_switch_disables_reexec` previously invoked the checkout's real `run_loop.sh` with `cwd=REPO_ROOT` without specifying `MAM_LOOP_MARKER`. Because `run_loop.sh` derives `MAM_REAL_ROOT=REPO_ROOT` and defaults `MAM_LOOP_MARKER="${MAM_LOOP_MARKER:-$MAM_REAL_ROOT/.mam/loop-guard-active}"`, `mam_acquire_loop_lock` executed `mkdir -p "$(dirname "$MAM_LOOP_MARKER")"`, creating `.mam/` in the real checkout and briefly acquiring the live loop guard.

Commit `a93c9bf` surgically resolves this issue:
1. `tests/test_o3_scoped_guard.py`: Sets `MAM_LOOP_MARKER=str(tmp_path / "loop-guard-active")` for both `b13` tests.
2. A comprehensive audit across all test files confirms no other test invokes the checkout's `run_loop.sh` past argument parsing without an isolated marker or sandbox.
3. Added `tests/test_loop_marker_isolation.py:test_l1_no_test_takes_the_loop_lock_of_its_checkout` which copies the checkout, runs all tests reaching the lock, and asserts no `.mam/` is created in the copy.
4. Added an explicit `assert not (copy / ".mam").exists()` check to `tests/test_issue17_workspace_root_isolation.py:test_w1`.
5. Executed all target and related suites under both (a) unset `WORKSPACE_ROOT` and (b) `WORKSPACE_ROOT=<sentinel>`: 57 passed, 0 failed, with the sentinel directory remaining 100% empty and no `.mam/` left behind in the worktree.
6. Product code (`.agents/skills`) remains 100% untouched.

All review criteria and task requirements are satisfied. The final verdict is **PASS**.

---

## 2. Evaluation of Task Requirements

| Requirement | Description | Status | Details |
|---|---|---|---|
| **1. Comprehensive Audit of `run_loop.sh` Tests** | Investigate all candidate tests invoking `run_loop.sh`. Verify whether they execute the checkout's script past argument parsing without custom marker/sandbox. | **PASS** | Complete audit conducted across all 16 candidate test modules (detailed in Section 3). Only the two `b13` tests in `test_o3_scoped_guard.py` were unprotected. |
| **2. Temporary Marker Isolation** | Point `MAM_LOOP_MARKER` under `tmp_path` for affected tests while preserving deliberate lock tests and avoiding harmful global/autouse fixtures. | **PASS** | Directly specified `MAM_LOOP_MARKER=str(tmp_path / "loop-guard-active")` in `test_b13_reexec_preserves_original_argv` and `test_b13_no_freeze_switch_disables_reexec`. Deliberate lock test suites (`test_o2_race_free_lock.py`, etc.) retain their intended marker semantics without interference. |
| **3. Regression Tests** | Add a regression test running candidate tests in a clean copy of the checkout and asserting no `.mam/` is created. Verify failure prior to fix. | **PASS** | Added `tests/test_loop_marker_isolation.py` (`test_l1`), and added `.mam` absence assertion in `tests/test_issue17_workspace_root_isolation.py:test_w1`. At base commit `97c2946`, `test_b13` created `$REPO_ROOT/.mam` on lock acquisition, causing `test_l1` to fail. |
| **4. Suite & Sentinel Verification** | Run modified test files + `test_o2_race_free_lock.py`, `test_o3_scoped_guard.py`, `test_issue17_workspace_root_isolation.py` under (a) unset `WORKSPACE_ROOT` and (b) `WORKSPACE_ROOT=<sentinel>`, verifying sentinel is empty and no `.mam/` exists in tree. | **PASS** | Both runs (a) and (b) passed completely (57/57 passed each). Sentinel directory remained 100% empty (`find "$SENTINEL" -type f` returned 0 files), and verification tree has no `.mam/` directory. |

---

## 3. Comprehensive Audit Table of `run_loop.sh` Candidate Modules

| Test File | Reference / Invocation | Reaches Loop Lock? | Exclusion / Remediation Rationale |
|---|---|---|---|
| [`tests/test_b7_diff_untracked.py`](file:///tests/test_b7_diff_untracked.py#L174) | Lines 174, 189: `read_text()`, `bash -n` | No | Static code inspection and bash syntax check only; does not execute `run_loop.sh`. |
| [`tests/test_deploy_freshness.py`](file:///tests/test_deploy_freshness.py#L473) | Line 473: `read_text()` | No | Static text inspection of `MAM_ENV_FILE` export; does not execute script. |
| [`tests/test_issue17_workspace_root_isolation.py`](file:///tests/test_issue17_workspace_root_isolation.py#L25) | Runs tests in `tmp_path / "checkout"` copy | Yes (in copy) | Runs isolated nodes in a temporary checkout copy; protected by test-level markers in the sub-tests. |
| [`tests/test_issue21_acceptance.py`](file:///tests/test_issue21_acceptance.py#L298) | Line 298: subprocess call | Yes | Already sets `env["MAM_LOOP_MARKER"] = str(marker)` where `marker` is in `tmp_path`. |
| `tests/test_issue21_loop_b.py` | N/A | No | File does not exist / references removed in prior refactors. |
| [`tests/test_issue21_worktree_prereqs.py`](file:///tests/test_issue21_worktree_prereqs.py#L405) | Line 405: `test_t2_8` | Yes | Already sets `"MAM_LOOP_MARKER": str(marker)` where `marker` is in `mam_sandbox`. |
| [`tests/test_issue25_plan_reuse.py`](file:///tests/test_issue25_plan_reuse.py#L367) | Line 355: `--help`; Line 367: `test_p7b` | Yes | `--help` exits before lock; `test_p7b` already sets `"MAM_LOOP_MARKER": str(marker)` in `tmp_path`. |
| [`tests/test_issue36_prompt_transport.py`](file:///tests/test_issue36_prompt_transport.py#L540) | Line 540: `_run_shell_loop`; Line 630: `read_text()` | Yes | Runs inside temporary fixture workspace `ctl` with explicit `"MAM_LOOP_MARKER": ctl / ".mam" / "loop-marker"`. |
| [`tests/test_issue5_phase0_contracts.py`](file:///tests/test_issue5_phase0_contracts.py#L481) | Line 481: `read_text()` | No | Static assertion in baseline text check. |
| [`tests/test_issue5_phase3_loop.py`](file:///tests/test_issue5_phase3_loop.py#L151) | Line 151: `--help`; Lines 165, 183 | Yes | `--help` exits before lock; execution tests already pass `env["MAM_LOOP_MARKER"] = str(tmp_path / "loop-marker")`. |
| [`tests/test_issue5_phase4_runtime.py`](file:///tests/test_issue5_phase4_runtime.py#L142) | Lines 142, 166: `--help` | No | Only executes `--help` to test freeze directory bypass logic; exits before lock acquisition. |
| [`tests/test_loop_cli.py`](file:///tests/test_loop_cli.py#L23) | Line 51: `test_creator_parses...`; others: `--help` / bad args | Yes | Argument errors and `--help` exit before lock; `test_creator_parses...` already sets `"MAM_LOOP_MARKER": str(tmp_path / "loop-guard")`. |
| [`tests/test_o1_rebuttal.py`](file:///tests/test_o1_rebuttal.py#L24) | Line 24: function extraction; Line 166: `read_text()` | No | Extracts helper functions to execute standalone or reads text; never runs `run_loop.sh` main entry point. |
| [`tests/test_o2_race_free_lock.py`](file:///tests/test_o2_race_free_lock.py#L203) | Line 203: `test_o2_12` | Yes | Already sets `run_env["MAM_LOOP_MARKER"] = str(tmp_path / "loop-guard-active")`. |
| [`tests/test_o3_scoped_guard.py`](file:///tests/test_o3_scoped_guard.py#L387) | Lines 387, 471: `test_b13_*` | **Yes (Defect)** | **Remediated**: Added `MAM_LOOP_MARKER=str(tmp_path / "loop-guard-active")` to both invocations. |
| [`tests/test_tier4_e2e.py`](file:///tests/test_tier4_e2e.py#L285) | Line 285: `mam_sandbox` copy | Yes | Runs inside isolated `mam_sandbox` directory tree, not the repository checkout. |

---

## 4. Test Suite Execution & Verification Results

All tests were executed within the verification tree:
`/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261008T234013Z-4f5d21/1`

### 4.1. Run (a): Without WORKSPACE_ROOT (Requirement 4)
```bash
AGENT_PYTHON_BIN=/Users/godopu16/PuKi/laa/canary_projects/multi-agent-mux/.venv/bin/python \
PYTHONDONTWRITEBYTECODE=1 \
env -u WORKSPACE_ROOT \
/Users/godopu16/PuKi/laa/canary_projects/multi-agent-mux/.venv/bin/python \
  -m pytest -p no:cacheprovider -q --basetemp=/tmp/pytest-loop-marker-a \
  tests/test_loop_marker_isolation.py \
  tests/test_o3_scoped_guard.py \
  tests/test_o2_race_free_lock.py \
  tests/test_issue17_workspace_root_isolation.py
```
- **Exit Code**: `0`
- **Output**: `57 passed in 46.27s`
- **Tree Check**: `.mam` does not exist in the verification directory.

### 4.2. Run (b): With WORKSPACE_ROOT=<sentinel> (Requirement 4)
```bash
SENTINEL=/tmp/sentinel-review-b
mkdir -p "$SENTINEL"
AGENT_PYTHON_BIN=/Users/godopu16/PuKi/laa/canary_projects/multi-agent-mux/.venv/bin/python \
WORKSPACE_ROOT="$SENTINEL" \
PYTHONDONTWRITEBYTECODE=1 \
/Users/godopu16/PuKi/laa/canary_projects/multi-agent-mux/.venv/bin/python \
  -m pytest -p no:cacheprovider -q --basetemp=/tmp/pytest-loop-marker-b \
  tests/test_loop_marker_isolation.py \
  tests/test_o3_scoped_guard.py \
  tests/test_o2_race_free_lock.py \
  tests/test_issue17_workspace_root_isolation.py
```
- **Exit Code**: `0`
- **Output**: `57 passed in 48.74s`
- **Sentinel Contents**: `find "$SENTINEL" -type f` returned 0 files (100% clean).
- **Tree Check**: `.mam` does not exist in the verification directory.

### 4.3. Auxiliary Related Loop Suites
Executed `test_issue5_phase3_loop.py`, `test_issue5_phase4_runtime.py`, `test_loop_cli.py`, `test_issue25_plan_reuse.py`, `test_o1_rebuttal.py`:
- **Exit Code**: `0`
- **Output**: `53 passed in 44.25s`

---

## 5. Non-Functional & Safety Checks

1. **Surgical Precision**:
   - `git diff --stat 97c2946..a93c9bf` shows 3 files changed (+55 lines), strictly confined to `tests/`.
   - Zero changes to `.agents/skills`.
2. **Portability**:
   - Compatible with macOS, Python 3.9+, and bash 3.2.
   - Preserves POSIX shell semantics and subshell environment isolation.
3. **Safety & Cleanliness**:
   - No interaction with real `.mam.env`, live daemon, or active sessions.
   - Clean git status; follows `test(loop-lock): ...` commit naming convention.

---

## 6. Conclusion & Final Verdict

The changes resolve the checkout loop lock acquisition defect surgically and completely. Tests executing `run_loop.sh` are thoroughly isolated, regression tests verify zero `.mam` creation, and all suites pass with zero regressions.

[VERDICT: PASS]