# Code Review Report (4th Round Re-Review): PR #56 (`fix/issue-50-55`)

- **Job ID**: `05f28276`
- **Reviewer**: `agy` (Session: `herdr:reviewer-agy-01`, Role: `Reviewer`)
- **Date**: 2026-10-12
- **Target Pull Request**: PR #56 (`fix/issue-50-55`, Closes #50, #55)
- **Base Commit**: `efd27def47a4d67684694ccf43a6ebeede01620f` (3rd round review head)
- **Head Commit**: `21f0c12b9e2195cba8e196d6ccc2469173302e8a` (Fix commit)
- **Verification Path**: `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261012T013821Z-656dfe/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 4th-round re-review evaluated commit `21f0c12b9e2195cba8e196d6ccc2469173302e8a`, which addresses the regression discovered on macOS during full-suite testing where [`tests/test_tier4_e2e.py::test_e2e_scenario4_parallel_flock_locking`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261012T013821Z-656dfe/1/tests/test_tier4_e2e.py) (6 concurrent creates into the same folder) failed with exit code 4 (`already running in agent-sessions.yaml`).

### Root Cause
During parallel `create_session.sh` invocations, `create_session.sh` runs `reconcile.sh --once` under ambient `HERDR_SESSION_NAME=<slug>`.
1. In `reconcile.sh`, `unique_servers` always includes `'default'`.
2. The scan for `'default'` executes `herdr ls` without `-L`.
3. The `.mam/shim/herdr` wrapper inspects `HERDR_SESSION_NAME`; because it is set to `<slug>`, `_real_herdr` delegates to `"$HERDR_BIN" --session "$session" "$@"`.
4. Thus, the pane list returned by the `'default'` scan actually reflects the panes on server `<slug>`.
5. Previously, `reconcile.sh` drift B labeled these newly discovered panes with `herdr_server: default`.
6. When concurrent `create_session.sh` processes attempted to adopt the auto-registered pane via `_adopted_own`, they required `(s.get('herdr_server') or ...) == server_name` (where `server_name` was `<slug>`). Because the monitor row recorded `'default'`, the comparison failed and `create_session.sh` aborted with exit code 4.

### The Fix
In [`multi-agent-mux-monitor/scripts/reconcile.sh:659-661`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261012T013821Z-656dfe/1/.agents/skills/multi-agent-mux-monitor/scripts/reconcile.sh#L659-L661):
```python
_ambient = os.environ.get('HERDR_SESSION_NAME', '')
if srv == 'default' and _ambient and _ambient != 'default':
    srv = _ambient
server_opt = f"-L {srv} " if srv != 'default' else ""
```
If the sweep is querying `'default'` but `HERDR_SESSION_NAME` is set to an explicit server in the environment, `reconcile.sh` recognizes that the shim returned panes from that server, and records `herdr_session: _ambient`, `herdr_server: _ambient`, and `-L _ambient` in the entry.
Additionally, [`tests/data/skill_code_blocks.json`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261012T013821Z-656dfe/1/tests/data/skill_code_blocks.json) was updated to line 550 for `SystemExit(4)` in `create_session.sh`.

**Verdict: PASS**.

---

## 2. Detailed Technical Verification

### 2.1 Correctness of the Ambient Server Attribution
- In `.agents/skills/lib.sh:271-280`:
  ```bash
  _real_herdr() {
    local session="${HERDR_SESSION_NAME:-}"
    if [ "$session" = "default" ]; then session=""; fi
    if [ -n "$session" ]; then
      if [ "${1:-}" = "--session" ] || [ "${1:-}" = "-s" ]; then "$HERDR_BIN" "$@"; else "$HERDR_BIN" --session "$session" "$@"; fi
    else
      "$HERDR_BIN" "$@"
    fi
  }
  ```
  Calls to `herdr` without `-L` or `--session` are answered by `herdr --session "$session"`. Thus, attributing panes found in a `'default'` scan to `HERDR_SESSION_NAME` is strictly faithful to the actual server queried by `_real_herdr`.

### 2.2 Side-Effect Analysis
1. **Drift A (Termination Detection)**:
   - When a row is registered with `herdr_server: <slug>`, subsequent sweeps populate `unique_servers` from `d.get('herdr_sessions')`, adding `<slug>`.
   - `reconcile.sh` explicitly scans `-L <slug>` and places `(name, '<slug>')` into `alive_set`.
   - In drift A, `(s.get('name'), s.get('herdr_server'))` matches `alive_set`. The row is **not** falsely terminated.
2. **Daemon Mode (No Ambient Server)**:
   - In persistent monitor daemon runs (`reconcile.sh --subscribe`), `HERDR_SESSION_NAME` is empty or `'default'`.
   - `_ambient and _ambient != 'default'` evaluates to `False`, preserving exact default behavior.
3. **Duplicate Registration Prevention**:
   - In drift B, [`yaml_session_names.add(name)`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261012T013821Z-656dfe/1/.agents/skills/multi-agent-mux-monitor/scripts/reconcile.sh#L715) is updated immediately when a pane is registered.
   - If the same pane is enumerated in both the `'default'` scan and an explicit `-L <slug>` scan within the same sweep, line 611 skips the duplicate (`if (name, srv) in yaml_session_names or any(_sanitize(y) == name for y in yaml_session_names): continue`). No duplicate entries are created.

### 2.3 Verification of Fixed Tests
1. **`test_e2e_scenario4_parallel_flock_locking`**:
   - Executed [`tests/test_tier4_e2e.py`](file:///var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261012T013821Z-656dfe/1/tests/test_tier4_e2e.py): all 5 scenarios passed in 364.51s, with scenario 4 completing in 42.28s without exit code 4.
2. **`tests/test_issue50_pane_lock.py`**:
   - Executed full test suite: 11 / 11 passed in 53.20s.
3. **`tests/test_issue42_skill_blocks.py`**:
   - Executed: 37 passed, 1 skipped. Line 550 in `tests/data/skill_code_blocks.json` matches `create_session.sh` exactly.

---

## 3. Test Execution Summary

| Test Target | Result | Duration |
|---|---|---|
| `tests/test_tier4_e2e.py::test_e2e_scenario4_parallel_flock_locking` | **1 Passed** | 42.28s |
| `tests/test_tier4_e2e.py` (Full Suite, 5 Scenarios) | **5 Passed** | 364.51s |
| `tests/test_issue50_pane_lock.py` (Full Suite) | **11 Passed** | 53.20s |
| `tests/test_issue42_skill_blocks.py` | **37 Passed, 1 Skipped** | 0.17s |

---

[VERDICT: PASS]