multi-agent-mux/.agents/reports/reviewer-grok-01/report-667d5542.md

Review: PR #48 (fix/issue-45) @ 35935f9

• Role: Reviewer (grok)
• Identity: loop loop-20261005T064728Z-a4e9c8, base c5c7eff6f758c76ac37a138dbc3c301f3f7e10cf (main, v4.8.4), head 35935f9e8a052ca9e9d9b806a62d17eb72afea78, tree dff9db13f3e018528d04a024acda942f501513ff, 10 commits, clean snapshot
• Verified read-only at /var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261005T064728Z-a4e9c8/1. Main stayed c5c7eff. No product edit, no commit, no push.
• Diff vs base: 25 files, tests/test_issue10_13_purge.py absent. Interpreter for the required pytest: pr-48/.venv Python 3.9.6. The shim's python3 is /usr/bin/python3, also 3.9.6. CI (deploy/gitea-ci.yml:94) is Python 3.10. tomllib is stdlib only from 3.11. requirements-dev.txt is pytest>=8.0. /usr/bin/python3 has no tomli. The venv has tomli and no tomllib.

The required command exits at collection. With --continue-on-collection-errors added only so the rest of the list could run: 5 failed, 192 passed, 1 skipped, 1 error in 254.58s (--basetemp=/tmp/667follow.YrMavO). The first command, as written in the brief, was 1 collection error in 0.22s, exit 2, and ran no test.

What fails

1. The merge tests do not load on the Python this repo runs

tests/test_issue45_herdr_config.py:16 is a top-level import tomllib. Collection raises ModuleNotFoundError. tests/test_issue9_server_env.py:157 imports it inside test_bootstrap_hands_the_server_a_no_restore_config and fails the same way.

lib_py/herdr_config.py:54-57 catches that import and sets tomllib = None. server_config_text then returns MINIMAL plus a warning (:146-147). A throwaway HOME=/tmp/m45probe-min run of /usr/bin/python3 -m lib_py.herdr_config server-config printed that warning and wrote only:

# generated by multi-agent-mux (Issue #45) - do not edit
# source: /tmp/m45probe-min/home/.config/herdr/config.toml
[session]
resume_agents_on_restore = false

The throwaway user file's checksum was unchanged. That is the accepted "no parser, restore still off" path. It is also the path the shim takes on this host and on CI 3.10.

VERSIONS.md:49 and multi-agent-mux-create/SKILL.md:120 describe the generated file as the user's config with that one key forced false. test_issue9_server_env.py:171 asserts the merged document (ui.confirm_close kept). On this python3 the file is MINIMAL, so fixing only the test import still fails :171.

Alternative: in herdr_config.py:54-57 and in both test imports, use tomli when tomllib is missing (tomli.loads / tomli.TOMLDecodeError match). Add tomli to requirements-dev.txt, and install it for the python3 that lib.sh:241 actually runs (/usr/bin/python3 here, not the venv). Keep the MINIMAL warning only when neither library imports. With tomli assigned onto the module, the merge kept inline tables, dotted keys, quoted keys, arrays, and comments semantically, and a broken document or a non-table session fell back to MINIMAL. herdr config check on confirm_close = "no" in a throwaway file exited 1 with parse error and using defaults, and herdr_rejects (:169-180) reported it.

2. The #45 positive control never runs on macOS

tests/test_issue45_restore.py:227-228 reads /proc/<pid>/environ. Both parametrizations of test_a_mam_server_does_not_relaunch_saved_agents raised FileNotFoundError there. The asserts above that line had already passed: the fake claude did not log --resume, the restored pane had no agent, and the foreground pid was the shell. The positive control at :230-236 (plain server must relaunch) did not execute.

Alternative: build the HERDR_CONFIG_PATH=... needle, read /proc/<pid>/environ when that file exists, and otherwise use ps eww -p <pid> -o command= and search the raw output. Run :230-236 even when the environ read fails, so a missing /proc cannot skip the control that shows restore-on still relaunches.

3. Restored cwd is the resolved path

tests/test_issue45_restore.py:263:

AssertionError: assert '/private/tmp/m45swco8dxx' == '/tmp/m45swco8dxx'

mkdtemp(dir="/tmp") returns /tmp/.... On this Mac /tmp is /private/tmp, and herdr's pane cwd is the resolved path. The close calls start at :267, so the selective-close asserts (live agent Z, busy sleep X2, user pane U, close only X and Y) did not run. test_b_resume_leaves_no_restored_shell_behind got create rc 0 and status == running, then StopIteration at :325 on p.get("cwd") == str(ws) with that same unresolved /tmp path.

test_b_default_server_is_never_touched passed: HERDR_SESSION_NAME=default returns before invoking --herdr /nonexistent/herdr (herdr_restore.py:54-55).

Alternative: compare Path(cwd).resolve() with Path(rig.root).resolve() at :263, and Path(p["cwd"]).resolve() == Path(ws).resolve() at :325.

What holds

1. User file bytes are not the output. output_path (herdr_config.py:192-205) skips the source (including samefile) and a file whose marker source is not this source. Both names taken raises OSError; the fallback directory goes through the same helper (:270-276). Writes are mkstemp + os.replace (:208-216). A generated HERDR_CONFIG_PATH is followed one hop via the # source: line (:79-87). The trace beside the user config is mam-server-config.toml (or mam-server-config-<sha256(realpath)[:12]>.toml), marker plus # source: <path>. A nan value makes tomllib.loads(text) == expected false (:159, nan != nan) and falls back to MINIMAL with a warning. The user file stays.

2. The only server spawn is the shim heredoc lib.sh:212-261 ($REAL_HERDR --session <name> server). create (create_session.sh:27 sources lib.sh, :264 _herdr new-session) and resume (resume_session.sh:8, :245) go through that shim. The bootstrap runs only when the name is not default, MAM_NO_AUTOSTART is not 1, and session list says the server is down (lib.sh:193-212). status.sh:13 sets MAM_NO_AUTOSTART=1 before sourcing. reconcile.sh:28 sets it before herdr calls; the bootstrap is in the shim, not at source time. An already-running server is left as it was. create/SKILL.md:120-121 and VERSIONS.md:50 say the file is rebuilt the next time MAM starts the server, and a server the user starts with herdr session attach does not get it.

3. A pane is closed only when the session name is set and not default, the pane has no live agent, agent_session.value equals the id being resumed, and shell_pid equals foreground_process_group_id (herdr_restore.py:54-65). Empty id and default return [] before herdr. main maps OSError, SubprocessError, and ValueError to exit 1 (:81-83). resume_session.sh:240-242 calls that and on failure only prints a warning; dry-run has already exited at :225-228. Bash does not decide which pane. There is a gap between process-info and pane close in which a command can start; a failed query still closes nothing further.

4. MAM_VERSION and FRAMEWORK_VERSION are 4.8.5. The eight SKILL.md frontmatters match. tests/data/skill_code_blocks.json changes line numbers and the resume script anchors (:237/:241/:247-250 → :245/:249/:255-258). No class, kind, or sha field changes. test_version_consistency and test_issue42_skill_blocks are in the 192 passes. S11's base moved to 811b080 (tests/test_issue23_24_startup_path.py) because the heredoc change is intentional.

5. tests/test_issue10_13_purge.py is not in the diff. Those claimed full-suite failures are not produced by this PR. This review did not run that file.

6. New judgment is in herdr_config.py and herdr_restore.py. The new bash is "config path empty → warning, start anyway" (lib.sh:243-245) and the resume warning above. The spawn snippet copies MAM_SERVER_CFG into the child as HERDR_CONFIG_PATH and drops MAM_SERVER_CFG from the child (lib.sh:253-256).

Isolation

Restore tests use mkdtemp(prefix="m45", dir="/tmp"), session mamt45-<pid>-<uuid4>, and strip HERDR_WORKSPACE_ID, HERDR_PANE_ID, HERDR_TAB_ID, HERDR_SOCKET_PATH, HERDR_SESSION, HERDR_ENV, HERDR_CONFIG_PATH, and XDG_CONFIG_HOME (test_issue45_restore.py:41-42, :160-164). After the run, no mamt45 process remained. ~/.config/herdr/config.toml mtime stayed 1784456394, the directory mtime stayed 1791183576, and no mam-server-config.toml appeared there. The live multi-agent-mux server log grew on its own during the window. Snapshot git status was empty afterward. Throwaway pytest dirs were removed.

[VERDICT: NOT PASS]