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

PR #73 (fix/issue-71) Review Report — agy (herdr:reviewer-agy-01)

• Job ID: 0a1bb7e7
• Loop ID: loop-20261026T133440Z-c8961b (iteration 2)
• Issue / PR: Issue #71 / PR #73 (fix/issue-71, 14 commits)
• Base Commit: 29cc4fd90478936f15387a7665e2c95d13a3f14c (main, v4.10.1)
• Head Commit: 5f6065344f800c2c5ad6f802e95e9e8ff5cc7b26 (tree 98d57bdc60920238c115ece90c90cc694f2744bc)
• Review Worktree: /var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261026T133440Z-c8961b/2
• Host Environment: macOS Darwin arm64 (Darwin 25.3.0), /bin/bash 3.2.57(1)-release, /usr/bin/python3 3.9.6

---

1. Executive Summary

PR #73 resolves Issue #71 across all 10 scope items and review refinements (A–F):

1. Unifies attach_command construction in lib_py/agents/sanitize.py::attach_command (herdr --session <server> agent attach <sanitized_name>) across create_session.sh, update_yaml_resumed.sh (both branches), and reconcile.sh, and teaches the lib.sh shim's herdr agent attach branch to resolve its target via _resolve_herdr_target.
2. Removes kill_command and start_command from newly written rows, status.sh --json, and SKILL.md examples, and replaces direct herdr kill-session cleanup hints in create_session.sh (exit 43) and id_order.py with stop_session.sh --session <name> --herdr-session <srv>.
3. Adds row-less session closing (stop_rowless) to stop_session.sh with strict registry readability gating (load_state_json strict), workspace boundary enforcement (os.path.realpath prefix-safe check, exit 5 on foreign workspace), server selection parity with create_session.sh, and registry immutability.
4. Consolidates _sanitize_herdr_agent_name into a single definition in lib.sh injected into the generated shim via declare -f, achieving exact parity with Python's sanitize_herdr_agent_name across ASCII, non-ASCII/multibyte, option-like (-n, -e), and newline-containing names, while failing closed (nonzero, stderr) when no hash tool or python3 is available instead of emitting 00000000 or truncated names.
5. Introduces registry format version 2 (registry_format: 2) in lib_py/registry_format.py and lib_py/atomic_yaml.py, migrating under the writer lock (BEGIN IMMEDIATE) and repairing stale YAML copies on retry.
6. Computes herdr_name once in StallWatch.__init__ and passes it as a required keyword-only argument name to HerdrSource.

All 10 review checklist items, macOS /bin/bash 3.2.57 and /usr/bin/python3 3.9.6 compatibility checks, negative regression on base (68 failed, 2 passed), and 461 functional/integration tests on head (461 passed, 1 skipped, 0 failed) pass on macOS Darwin arm64.

---

2. Checklist Verification (Items 1–10)

1. Completion Criteria & Scope Discipline (Issue #71 & Scope Comment Items 1–10)

• Verified (PASS):
  • Every requirement in Issue #71 and the "범위 정리" comment (items 1–10) is implemented and covered by regression tests.
  • Out-of-scope Issue #72 (multi-agent-mux-update skill) is not pulled into this PR; only the python3 -m lib_py.registry_format --yaml <path> entry point specified in item 9 is provided for #72 to call. MAM_VERSION (4.10.1) and VERSIONS.md are untouched as intended.

2. Regression Suite tests/test_issue71_herdr_names.py (Base vs Head & Reconcile Pane Precondition)

• Verified (PASS):
  • Base (29cc4fd90478936f15387a7665e2c95d13a3f14c): Checked out into an isolated temporary directory outside the review worktree and ran tests/test_issue71_herdr_names.py (70 tests): 68 failed, 2 passed in 104.17s (the only 2 passing tests on base are test_the_names_need_converting_and_the_plain_one_does_not and test_wrapper_attach_calls_herdr_with_the_herdr_name[lab-plain-creator-claude], matching the PR description).
  • Head (5f6065344f800c2c5ad6f802e95e9e8ff5cc7b26): All 70 passed (including all 4 hashers parameterizations: shasum, sha1sum, openssl, python3).
  • Reconcile test precondition: test_reconcile_registers_with_the_attach_command seeds mock_herdr with agents={name: {"agent": "claude", "status": "running", "cwd": str(mam_sandbox), "workspace_id": "w1", "pid": 9999, "pane_id": "w1:p71", "command": "claude", "buffer": "Ready"}} and asserts len(rows) == 1 and rows[0]["last_visible_note"] == "auto-registered by monitor" before checking _assert_new_form(rows[0], rows[0]["herdr_session"]).

3. attach_command (lib_py/agents/sanitize.py) & lib.sh Wrapper agent attach

• Verified (PASS):
  • lib_py/agents/sanitize.py:34-42 defines:

    def attach_command(server, name):
        return "herdr --session %s agent attach %s" % (
            shlex.quote(server or "default"), shlex.quote(sanitize_herdr_agent_name(name)))
    

  • Always writes --session default when server is "default" or empty/falsy, and applies shlex.quote to both the server and sanitized agent name.
  • Used by all three writers:
    • create: create_session.sh:512, 566
    • resume (new row and existing row): update_yaml_resumed.sh:119, 150, 160
    • reconcile: reconcile.sh:569, 676
  • In lib.sh:605-613, the shim's agent attach branch shifts <target> (t="$1"; shift), resolves tgt=$(_resolve_herdr_target "$t" || true), fails with exit 1 if empty, and invokes _real_herdr agent attach "$tgt" "$@", preserving trailing flags such as --takeover.

4. Removal of kill_command & start_command and Updated Cleanup Hints

• Verified (PASS):
  • git grep -E 'kill_command|start_command' across the repository confirms no production code, skill doc, or active schema assertion reads or writes kill_command or start_command (the only remaining mentions are in registry_format.py to drop them during v1 -> v2 migration and in test_issue71_herdr_names.py / test_tier2_component.py asserting their absence).
  • status.sh:221-231 and multi-agent-mux-status/SKILL.md:94-101 no longer include start_command, and nothing in the codebase reads start_command from status.sh --json.
  • The shim's new-session and kill-session translation branches in lib.sh are properly retained because create_session.sh, resume_session.sh, stop_session.sh, and reconcile.sh call them internally.
  • Both user-facing cleanup hints now point to stop_session.sh:
    • create_session.sh:364: bash .agents/skills/multi-agent-mux-stop/scripts/stop_session.sh --session $SESSION_NAME --herdr-session ${HERDR_SESSION_NAME:-default}
    • lib_py/agents/id_order.py:359-360: bash .agents/skills/multi-agent-mux-stop/scripts/stop_session.sh --session {m['session']} --herdr-session {srv}

5. Row-less stop (stop_session.sh) & Strict Registry Read (load_state_json strict)

• Verified (PASS):
  • Strict registry read gate: stop_session.sh:170-193 calls STATE_JSON=$(load_state_json strict) first. In lib.sh:1597-1631, load_state_json strict re-raises sqlite3.OperationalError on missing state or sessions tables as well as corrupt DB / malformed YAML, printing to stderr and exiting 1. Furthermore, MAPPED_DATA exits 7 only when d.get('herdr_sessions', []) was validly iterated and had no matching dict row; non-dict rows (herdr_sessions: [- 42]) exit 1 (cannot read the row).
  • Workspace boundary check: stop_rowless (stop_session.sh:145-155) resolves both PANE_CWD and WORKSPACE_ROOT via os.path.realpath and checks cwd == root or cwd.startswith(root + os.sep). Verified on macOS that this prevents prefix confusion (/tmp/ws vs /tmp/ws-other), handles symlinks (/var -> /private/var), and normalizes trailing slashes; out-of-workspace sessions are refused with exit code 5 before any kill-session.
  • Server selection order: Matches create_session.sh:184-188: --herdr-session (HERDR_SERVER_OPT) -> ambient HERDR_SESSION_NAME (when non-empty and != "default") -> workspace server via resolve_herdr_session.
  • --purge-conversation and exit codes: Refuses --purge-conversation on a row-less session with exit 1, exits 6 if herdr has-session still succeeds after kill-session, never mutates agent-sessions.yaml or agent-sessions.db, and matches multi-agent-mux-stop/SKILL.md:59-67.

6. Single Sanitize Rule in lib.sh & macOS BSD / Locale Parity

• Verified (PASS):
  • lib.sh:40-85 has the single definition of _sanitize_herdr_agent_name. _init_herdr_isolation (lib.sh:179-182) writes declare -f _sanitize_herdr_agent_name into the generated shim ($wrapper_dir/herdr).
  • Removed the duplicate definition in the shim heredoc, removed "00000000" and ${s:0:32} fallbacks, and removed has-session's relative-path sys.path.insert(0, os.path.abspath(".agents/skills")) + lambda s: s.lower()[:32] fallback (lib.sh:633-647 now computes ssess=$(_sanitize_herdr_agent_name "$sess") || exit 1 in bash and passes TARGET_SAN="$ssess").
  • macOS /bin/bash 3.2.57, BSD tr/sed, and locale (LC_ALL=C and LC_ALL=en_US.UTF-8) verification:
    • Pure-ASCII check uses LC_ALL=C tr -d '\000-\177'. For ASCII inputs, printf '%s' "$n" | tr '\n' '-' | tr '[:upper:]' '[:lower:]' | sed -E 's/[^a-z0-9_-]/-/g' avoids echo flag parsing (-n, -e, -En) and converts \n to - before sed or command substitution $(...) can strip trailing newlines.
    • For non-ASCII inputs (İ, é, ü, K, CJK/Hangul, emoji), it delegates lowercasing and re.sub(r"[^a-z0-9_-]", "-", ...) to python3 so character-by-character replacement and Unicode case folding (\u0130.lower() -> 'i\u0307' -> 'i-') match lib_py.agents.sanitize.sanitize_herdr_agent_name byte-for-byte, and fails closed (exit 1, stderr) if python3 is unavailable.

7. Registry Format Version 2 (lib_py/registry_format.py, lib_py/atomic_yaml.py)

• Verified (PASS):
  • Inside atomic_dump_yaml_main (lib_py/atomic_yaml.py:121-138), after acquiring the flock and BEGIN IMMEDIATE and loading state + sessions, if from_version < registry_format.CURRENT (2), registry_format.migrate(d) runs before the caller's mutation and sets force_yaml = True.
  • Migrates all rows in herdr_sessions (running, stopped, terminated, archived), drops kill_command and start_command, recomputes attach_command, and sets d["registry_format"] = 2.
  • If the DB was already at format 2 (elif row:) but an earlier YAML os.replace failed after conn.commit(), force_yaml = registry_format.stored_version(y) < registry_format.CURRENT detects the stale YAML and rewrites it on the next write.
  • Future format preservation: Verified that when registry_format > 2 (e.g., 3), stored_version(d) returns 3, from_version < CURRENT is False, and force_yaml is False — the writer neither downgrades registry_format nor clobbers future fields.
  • Other registry paths: Checked all references to agent-sessions.yaml across .agents/skills/; atomic_dump_yaml is the sole writer in production (create_session.sh, update_yaml_resumed.sh, stop_session.sh, reconcile.sh, orc_onboard.sh), while load_state_json (lib.sh / lib_py/state.py) is strictly read-only.
  • CLI entry point: PYTHONPATH=.agents/skills python3 -m lib_py.registry_format --yaml <path> prints registry-format: <from> -> <to>, rows changed: <n> and exits 0 (or nonzero with registry-format: error: ... on stderr when missing/failing), matching its module docstring contract.

8. stall_watch.py Single Sanitization Point

• Verified (PASS):
  • In .agents/skills/lib_py/stall_watch.py:158-163, 229-230, StallWatch.__init__ computes self.herdr_name = sanitize_herdr_agent_name(args.target) once and passes name=self.herdr_name (required keyword-only parameter) to HerdrSource.
  • tests/test_issue66_stall_watch_sanitized_name.py is unmodified in git diff and all 7 of its tests pass.

9. Test Updates, skill_code_blocks.json, and test_s11 Anchor (0fc162e)

• Verified (PASS):
  • tests/test_issue50_pane_lock.py, tests/test_issue21_worktree_prereqs.py, tests/test_tier1_unit.py, tests/test_tier2_component.py, and tests/test_tier3_integration.py preserve their original test intent after replacing assertions on removed start_command/kill_command fields.
  • tests/data/skill_code_blocks.json and tests/test_issue42_skill_blocks.py pass all 96 skill-block verification tests.
  • In tests/test_issue23_24_startup_path.py:306-335, test_s11_this_branch_does_not_change_the_generated_shim pins base = os.environ.get("MAM_BASE_SHA", "0fc162e") and extracts both _sanitize_herdr_agent_name() { ... } and the cat <<'EOF' heredoc body in _heredoc_body. Commit 0fc162e is an ancestor of HEAD (5f60653) on fix/issue-71; when merged into main via merge commit (as documented in PR #73), 0fc162e remains reachable in git history so test_s11 actively verifies the shim rather than skipping. (The if show.returncode != 0: pytest.skip(...) guard is pre-existing for shallow/archive checkouts without history.)

10. macOS Compatibility (/usr/bin/python3 3.9.6, /bin/bash 3.2.57, BSD Userland, SQLite WAL)

• Verified (PASS):
  • Parsed all 16 modified/added Python files with /usr/bin/python3 (3.9.6) ast.parse(..., feature_version=(3, 9)) and imported lib_py.agents.sanitize, lib_py.registry_format, lib_py.atomic_yaml, and lib_py.stall_watch under system Python 3.9.6.
  • Checked all 7 modified bash scripts and the generated .mam/shim/herdr with /bin/bash -n (GNU bash 3.2.57(1)-release); no bash 4+ constructs (declare -A, ${var,,}, mapfile, readarray) are used.
  • In tests/test_issue71_herdr_names.py:514-527, test_stop_does_not_take_an_unreadable_registry_for_no_row unlinks agent-sessions.db* / agent-sessions.db-* before writing corrupt DB/YAML fixtures, properly accounting for macOS SQLite's persistent -wal/-shm sidecars.

---

3. Test Execution Summary on macOS

Suite / Command	Target	Result
git diff --check 29cc4fd..5f60653	Review worktree /2	Clean (exit 0)
/bin/bash -n on all 7 modified .sh scripts	/bin/bash 3.2.57	All 7 OK (exit 0)
/usr/bin/python3 (3.9.6) AST + import + LC_ALL=C/en_US.UTF-8 sanitize & path checks	System Python 3.9.6 + /bin/bash 3.2.57	ALL_LOCALE_AND_MULTIBYTE_PARITY_OK, FUTURE_FORMAT_PRESERVED_OK, STOP_ROWLESS_PATH_BOUNDARY_CHECKS_OK (exit 0)
pytest tests/test_issue71_herdr_names.py	Base (29cc4fd in temp dir)	68 failed, 2 passed in 104.17s
pytest tests/test_issue71_herdr_names.py tests/test_issue66_stall_watch_sanitized_name.py tests/test_layout.py tests/test_issue42_skill_blocks.py	Head (5f60653, worktree /2)	152 passed, 1 skipped in 150.63s (all 70 in test_issue71 and all 7 in test_issue66 passed)
pytest tests/test_issue60_linux_fixes.py tests/test_tier1_unit.py tests/test_tier2_component.py tests/test_tier3_integration.py tests/test_issue23_24_startup_path.py tests/test_issue50_pane_lock.py tests/test_issue21_worktree_prereqs.py tests/test_issue63_same_cwd_ids.py	Head (5f60653)	309 passed, 0 failed

---

4. Defects & Observations

Blocking Defects

• None.

Non-Blocking Observations

• In .agents/skills/multi-agent-mux-delegate-job/multi-agent-mux-delegate-job:1021, the informational message after send-keys prints (연결하려면: HERDR_SESSION_NAME=$HERDR_SESSION_NAME herdr agent attach $sess — lib.sh를 source한 셸에서 실행). Because PR #73 updated the lib.sh shim's herdr agent attach handler to resolve $sess via _resolve_herdr_target, this command works as stated when lib.sh is sourced, though a future cleanup could also print the native herdr --session ... agent attach ... form.

[VERDICT: PASS]