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

# Peer Review Report: Branch `fix/issue-12-8-7` (Round 2 — Commits `0fedae2` & `ea911d7`)

- **Reviewer**: agy (session: `herdr:reviewer-agy-01`)
- **Job ID**: `3f5d1cd4`
- **Role**: Reviewer
- **Target Branch**: `fix/issue-12-8-7`
- **Base Commit**: `8a09e02` (v4.4.3)
- **Evaluated Commits**:
  - `e629cc1` — `fix(issue-12,8,7): keep broker password out of records, keep fresh install stamps, pack sibling workspaces`
  - `4176d75` — `release(v4.4.4): bump framework and eight skills to 4.4.4 in lockstep`
  - `0fedae2` — `fix(issue-12): scrub-secrets selects only records with a stored password`
  - `ea911d7` — `fix(issue-8,7): harden install_state merge and pin sibling-workspace edge cases`

---

## 1. Executive Summary

This Round 2 peer review evaluated the complete branch `fix/issue-12-8-7` up to `ea911d7`, focusing specifically on the resolutions introduced in commits `0fedae2` (N1 fix from Claude's Round 1 review: value-based filtering in `scrub-secrets`) and `ea911d7` (non-blocking hardening recommendations for `update.sh` `install_state` merge and two new edge-case tests for Issue #7 workspace packing).

Key verifications completed:
1. **Commit `0fedae2` (N1 Resolution)**: `_cmd_scrub_secrets` now strictly tests `broker.get("password")` (value truthiness) rather than key presence. Unauthenticated records with `"password": null` (~900 in realistic registries) are ignored, preventing spurious rewrite churn and lock contention. Dry-run verified on live `.mam/jobs` (0 records reported).
2. **Commit `ea911d7` (Update Hardening)**: `deploy/update.sh` ensures trailing newline on preexisting `install_state`, skips lines missing `=`, and utilizes `awk` with literal variable passing for exact key comparison (`$1 == k`). Failure recovery via `restore_on_failure` is intact.
3. **Commit `ea911d7` (Issue #7 Workspace Edge Cases)**: Two new automated tests in `test_issue7_workspace_packing.py` confirm:
   - An idle pane in a subsequent candidate workspace is reused before attempting splits or workspace creation.
   - `MAM_WS_LABEL` rename applies exclusively to the chosen target workspace.
4. **Version Lockstep v4.4.4**: Maintained consistently across `lib.sh`, `eager.py`, 8 `SKILL.md` manifests, `VERSIONS.md`, and documentation files.
5. **Test Suite Verification**: 13/13 targeted tests and 231/231 regression tests passed cleanly with 0 job registry leaks.

---

## 2. Review of Commit `0fedae2` (Issue #12: Value-Based Password Scrubbing)

### 2.1 Value-Based Record Selection
- In `.agents/skills/lib_py/delegation/registry.py` (`_cmd_scrub_secrets`):
  ```python
  # Old:
  if not (isinstance(broker, dict) and "password" in broker):
      continue

  # New (0fedae2):
  if not (isinstance(broker, dict) and broker.get("password")):
      continue
  ```
- **Analysis**:
  - Legacy records created under previous versions without authentication stored `"password": null`.
  - Testing `broker.get("password")` ensures only truthy values (non-empty strings containing actual passwords) trigger migration.
  - Records with `None`, `""`, or omitted `"password"` keys are untouched.
  - Verified on live workspace: `registry.py --registry-dir .mam/jobs scrub-secrets --dry-run` reports `would scrub 0 record(s) under .mam/jobs`, avoiding unnecessary rewrites.

### 2.2 Documentation & Specification Alignment
- `registry.md`: Clarifies that `scrub-secrets` strips stored passwords, leaving empty/null keys to be dropped naturally on the record's next write via `_atomic_write_record`.
- `VERSIONS.md`: Section 2 updated to emphasize that `"password": null` is not treated as a secret and is neither reported nor rewritten.

### 2.3 Automated Test Coverage
- In `tests/test_issue12_registry_secrets.py`, `test_scrub_secrets_cli_migrates_a_registry` was expanded to create an unauthenticated job record with `"password": None`.
- Verifies:
  - `empty not in dry.stdout` (dry run ignores null password).
  - `empty_path.read_text() == empty_before` (file is not modified).
  - Only genuine leaked passwords are reported and scrubbed.

---

## 3. Review of Commit `ea911d7` (Issue #8: `update.sh` Merge Hardening)

### 3.1 Install State Trailing Newline Guard
- In `deploy/update.sh`:
  ```bash
  if [ -s .mam/install_state ] && [ -n "$(tail -c1 .mam/install_state)" ]; then
    printf '\n' >> .mam/install_state
  fi
  ```
  Guarantees that an existing `install_state` missing a final newline does not concatenate appended keys onto the last line.

### 3.2 Non-KV Line Rejection
- Lines lacking `=` are skipped safely:
  ```bash
  case "$line" in
    *=*) ;;
    *) continue ;;
  esac
  ```

### 3.3 Exact Key Matching without Regex Side-Effects
- Previously used `grep -q "^${key}="`, which could interpret regex metacharacters in key names.
- Now uses `awk`:
  ```bash
  if ! awk -F= -v k="$key" '$1 == k { found = 1 } END { exit !found }' .mam/install_state; then
    printf '%s\n' "$line" >> .mam/install_state
  fi
  ```
- Exact literal string equality `$1 == k` prevents false matches and escapes.
- Full cycle verified via `tests/test_issue8_update_state.py`.

---

## 4. Review of Commit `ea911d7` (Issue #7: Workspace Packing Edge Cases)

Two new regression tests were added to `tests/test_issue7_workspace_packing.py`:

1. `test_idle_pane_in_a_later_workspace_is_reused`:
   - Configures `w1` as full (4 agents) and `w2` with 1 active agent and 1 idle pane (`w2:p_idle`).
   - Starts 7th agent: verifies the candidate traversal checks `w1`, continues past overflow, checks `w2`, finds the idle pane, and starts the agent directly on `w2:p_idle` without splitting or creating a workspace.
2. `test_ws_label_rename_targets_only_the_chosen_workspace`:
   - Configures `w1` (4 agents, full) and `w2` (1 agent).
   - Starts 8th agent with `MAM_WS_LABEL="team-b"`.
   - Confirms that only the target workspace `w2` is renamed: `[["workspace", "rename", "w2", "team-b"]]`. Preceding full candidate workspaces are untouched.

Both tests pass and accurately pin critical edge-case behaviors.

---

## 5. Version Lockstep v4.4.4 Verification

All components remain strictly locked at `4.4.4`:
- `.agents/skills/lib.sh`: `MAM_VERSION="4.4.4"`
- `.agents/skills/lib_py/loop/eager.py`: `FRAMEWORK_VERSION = "4.4.4"`
- Frontmatter `version: 4.4.4` across all 8 skills (`create`, `stop`, `resume`, `status`, `monitor`, `delegate-job`, `loop`, `orc-onboard`).
- `VERSIONS.md`: Current release v4.4.4, 8-row skills matrix, v4.4.4 changelog.
- Badges and headers in `README.md`, `README.ko.md`, `BOOTSTRAP.md`, `BOOTSTRAP.ko.md`, and `MESSAGING.md`.
- `lib_py.loop.eager.verify_framework_version()`: Verified OK.

---

## 6. Test Suite & Registry Integrity Audit

### 6.1 Targeted Test Execution
Command:
```bash
.venv/bin/python -m pytest tests/test_issue7_workspace_packing.py tests/test_issue8_update_state.py tests/test_issue12_registry_secrets.py tests/test_version_consistency.py
```
**Result**: 13 passed in 19.47s (100% success).

### 6.2 Regression Test Execution
Command:
```bash
.venv/bin/python -m pytest tests/test_herdr_shim_contract.py tests/test_layout.py tests/test_deploy_layout.py tests/test_issue5_phase1_delegation.py tests/test_issue5_phase2a_loop.py tests/test_loop_cli.py tests/test_issue5_phase4_runtime.py
```
**Result**: 231 passed in 128.99s (100% success).

### 6.3 Sandboxing & Leak Verification
- Baseline `.mam/jobs` file count before execution: **2405**
- File count after full test execution: **2405**
- Leak delta: **0 files created or modified** in live registry.

---

## 7. Conclusion

The updates in `0fedae2` and `ea911d7` cleanly resolve the N1 finding and further harden update and workspace packing operations. All verification criteria are fully satisfied.

[VERDICT: PASS]