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):

  # 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:

  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:

  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:

  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:

.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:

.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]