docs(v4.8.5): archive PR #48 reviews (three rounds) and record the merge-workspace review
PR #48 already carries the 4.8.5 lockstep bump. Merge-workspace review: round 1 (@35935f9) claude agy grok NOT PASS (test portability: tomllib on Python 3.9, /proc and /tmp on macOS), codex no verdict (model at capacity, budget spent); fixed in36f0b8dande8d9151. Round 2 (@e8d9151) codex NOT PASS (unrunnable config check passed an unchecked merge, odd pids closed a pane, nan dropped the user's settings); fixed inab93776. Round 3 (@ab93776) 4/4 PASS. Full suite atab93776on macOS / Python 3.9: 1456 passed / 0 failed / 38 skipped. Reports and the round-3 identity sidecars go to .agents/reports/<session>/. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
This commit is contained in:
GodopuandClaude Opus 5.5 committed
1 parent
ab93776d6d
commit
bf8a583afb
16 files changed
+898
No files matched your search
@@ -0,0 +1,14 @@
|
||||
{
|
||||
"loop_id": "loop-20261005T085016Z-c0f6eb",
|
||||
"base_sha": "e8d9151226d8139c7b5688e83d035a3a747aeb2d",
|
||||
"head_sha": "ab93776d6d1b8ca534c14b6be53f61dc3a0fa2a9",
|
||||
"tree_sha": "f49ffdc8e95dc316f752fd81b2d55a0efddf1a12",
|
||||
"review_commit": "ab93776d6d1b8ca534c14b6be53f61dc3a0fa2a9",
|
||||
"reviewer": "reviewer-grok-01",
|
||||
"job_id": "2a661116",
|
||||
"verdict": "PASS",
|
||||
"transport": "herdr",
|
||||
"evidence": "review-tree",
|
||||
"isolation": "worktree",
|
||||
"verified_at": "2026-10-05T09:05:24Z"
|
||||
}
|
||||
@@ -0,0 +1,40 @@
|
||||
# Review: PR #48 round 3 @ ab93776
|
||||
|
||||
- Role: Reviewer (grok)
|
||||
- Identity: loop `loop-20261005T085016Z-c0f6eb`, base `e8d9151226d8139c7b5688e83d035a3a747aeb2d`, head `ab93776d6d1b8ca534c14b6be53f61dc3a0fa2a9`, tree `f49ffdc8e95dc316f752fd81b2d55a0efddf1a12`, one commit, clean snapshot
|
||||
- Scope: `git diff e8d9151 HEAD`. Four files, +125/−4, `git diff --check` clean. Main stayed `c5c7eff`.
|
||||
- Verified read-only at `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261005T085016Z-c0f6eb/1`.
|
||||
|
||||
## Codex R1–R3
|
||||
|
||||
| Codex | ab93776 |
|
||||
|---|---|
|
||||
| R1. `herdr_rejects` raising (`TimeoutExpired`, `OSError`) was treated as a pass, so an unchecked merge could be handed to the server. A schema error in that file makes herdr drop the whole file and turn restore back on. | `_write_checked` (`herdr_config.py:247-258`) no longer returns success on that exception. If the candidate is the merged body, it warns `not using an unchecked merge` and continues. The loop's next candidate is `MINIMAL` (`:241`). The merged text is not what remains: `test_an_unrunnable_check_never_hands_over_an_unchecked_merge` writes `confirm_close = "invalid-bool"`, raises on the check, and the file on disk parses as only `resume_agents_on_restore = false`. The user file is unchanged. |
|
||||
| R2. `_shell_in_foreground` used `==`, so `0/0`, `False/False`, `""/""`, and `-1/-1` closed a pane. | A value counts only when it is an `int`, not a `bool`, and `> 0` (`herdr_restore.py:46-55`). Both pids must pass, and they must be equal. `tests/test_issue45_restore_unit.py` stubs herdr: those eight odd pairs, a missing `process_info`, and a busy pane (`4242` vs `4300`) do not close; `4242/4242` does. |
|
||||
| R3. Reparse used `==`, and `nan != nan`, so one valid `nan` dropped the rest of the user's settings. | `_same_doc` (`herdr_config.py:140-149`) treats two floats as equal when both are `nan`, and walks dicts and lists. `server_config_text` uses it at `:171`. The three cases (top-level, array including `-nan`, nested inline table) keep `confirm_close` and warn nothing. |
|
||||
|
||||
## MINIMAL when the check cannot run
|
||||
|
||||
Codex asked to return no path if the check of `MINIMAL` also cannot run. This commit writes `MINIMAL` and warns `using the fixed minimal config unchecked` (`:257-258`).
|
||||
|
||||
That is the right call. `lib.sh` starts the server with restore on when the config path is empty. Returning `None` would reopen #45 in the one case where no check can be obtained (timeout, missing binary, `OSError`). `MINIMAL` is the constant `[session]\nresume_agents_on_restore = false\n` (`:51`), not a user document, and `test_real_herdr_accepts_every_result` already shows herdr 0.9.1 accepts it whenever `config check` can run. The unchecked write happens only when that command cannot be executed, so there is no schema verdict to wait for. Unknown nonzero exits are still ignored unless the output contains `parse error` or `using defaults` (`herdr_rejects`, `:190-191`); herdr uses a nonzero status for a dropped setting that does not drop the file.
|
||||
|
||||
## `_same_doc` beside `==`
|
||||
|
||||
For values a TOML round trip produces, it matches `==`, plus `nan`. Dict keys are compared as a set (`a.keys() == b.keys()`), list order is kept (`zip`), datetimes use `==`, and `inf` / `-inf` follow `==`.
|
||||
|
||||
Two pairs differ, both stricter: `True == 1` is true and `_same_doc(True, 1)` is false; `1 == 1.0` is true and `_same_doc(1, 1.0)` is false. The writer emits a bool before the int branch (`_value`, `:108-111`) and emits a float with `repr` (`:117`), so a document `tomllib` parsed does not come back as the other type. A round trip that did change type fails closed into `MINIMAL` with a warning, which `==` would have accepted.
|
||||
|
||||
## Scope
|
||||
|
||||
The delta is `herdr_config.py`, `herdr_restore.py`, the two new tests in `test_issue45_herdr_config.py`, and `tests/test_issue45_restore_unit.py`. No skill text, inventory, or version line. 4.8.5 is not on main; the fallback to the key-only file is already what `VERSIONS.md` describes.
|
||||
|
||||
## Tests
|
||||
|
||||
Python 3.9.6, the four files in the brief, `--basetemp=/tmp/2a66follow.bPQAFP`: **53 passed, 34 skipped** in 77.84s. The skips are the `needs tomllib` cases, including the new R1 and R3 tests. `test_issue45_restore_unit.py` ran.
|
||||
|
||||
Python 3.11, the same venv named in the brief, `tests/test_issue45_herdr_config.py` and `tests/test_issue45_restore_unit.py`, `HOME` pointed at an empty temp directory: **67 passed** in 0.41s.
|
||||
|
||||
No `mamt45` process remained. `~/.config/herdr/config.toml` mtime stayed `1784456394`, the directory mtime stayed `1791183576`, and `mam-server-config.toml` was not created there. Snapshot `git status` was empty.
|
||||
|
||||
[VERDICT: PASS]
|
||||
@@ -0,0 +1,69 @@
|
||||
# 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:
|
||||
|
||||
```toml
|
||||
# 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`:
|
||||
|
||||
```text
|
||||
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.
|
||||
|
||||
5. `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.
|
||||
|
||||
6. `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.
|
||||
|
||||
7. 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]
|
||||
@@ -0,0 +1,32 @@
|
||||
# Review: PR #48 round 2 @ e8d9151
|
||||
|
||||
- Role: Reviewer (grok)
|
||||
- Identity: loop `loop-20261005T081844Z-e92124`, base `c5c7eff6f758c76ac37a138dbc3c301f3f7e10cf`, head `e8d9151226d8139c7b5688e83d035a3a747aeb2d`, tree `8b39188453ff8a7263aa4400792fabc99188dad6`, clean snapshot, ancestor of both `c5c7eff` and round-1 `35935f9`
|
||||
- Scope for this reviewer: `git diff 35935f9 e8d9151` only (commits `36f0b8d`, `e8d9151`). Five files, +56/−9, `git diff --check` clean. No product code. Main stayed `c5c7eff`.
|
||||
- Verified read-only at `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261005T081844Z-e92124/1`.
|
||||
|
||||
## Round-1 findings
|
||||
|
||||
| Round 1 | e8d9151 |
|
||||
|---|---|
|
||||
| `tests/test_issue45_herdr_config.py:16` imported `tomllib` at module level, so Python 3.9/3.10 could not collect the file. `tests/test_issue9_server_env.py:157` did the same inside the bootstrap test, and expected the merged document from a 3.9 `python3` that writes only the key. | Import is `try/except` (`test_issue45_herdr_config.py:18-21`). `needs_tomllib` skips the 29 tests that parse a result or expect the user's other settings (`:36`, applied at `:71` and the other parse sites). `test_without_tomllib_restore_is_still_off` still runs. `test_issue9_server_env.py:173-183` parses with the `python3` on `PATH` and, when that interpreter has no `tomllib`, expects the key-only file and `no tomllib` in the shim stderr. `create/SKILL.md:120` and `VERSIONS.md:49` say a `python3` older than 3.11 leaves restore off, drops the other herdr settings, and warns. |
|
||||
| `tests/test_issue45_restore.py:227` read `/proc/<pid>/environ` before the restore-on control, so macOS never reached that control. | `_process_env` (`:160-168`) reads `/proc/<pid>/environ` when it exists, otherwise `ps -E -ww -o command=`. The control stays after that check (`:242-248`). On this Mac the herdr server's `ps -E` line contains `HERDR_CONFIG_PATH=<resolved>/mam-server-config.toml`, and both `test_a` cases passed, so the control ran. |
|
||||
| `:263` and `:325` compared pane `cwd` to the unresolved `/tmp/...` from `mkdtemp`. herdr reports `/private/tmp/...`. | The fixture resolves the temp root (`:175`) before `HOME` is set, so later `str(rig.root)` and `str(ws)` match herdr. `test_b_only_the_resumed_sessions_restored_shell_is_closed` and `test_b_resume_leaves_no_restored_shell_behind` passed. |
|
||||
|
||||
The tomli backport suggested in round 1 is not in this delta. The docs now describe the path this host's `/usr/bin/python3` (3.9.6, no `tomli`) actually takes. That matches the decision to document it.
|
||||
|
||||
`tests/data/skill_code_blocks.json` is unchanged. The create skill edit is the same line 120, so no fence moved. `test_issue42_skill_blocks` passed, with the usual worktree skip at `:266` because `.claude/skills` is absent in the snapshot.
|
||||
|
||||
## Tests
|
||||
|
||||
Required command, PR venv Python 3.9.6, `--basetemp=/tmp/b471follow.eYGqXM` (no `mam-review` in the path), MQTT variables unset:
|
||||
|
||||
**152 passed, 30 skipped** in 214.33s. The 30 skips are the 29 `needs tomllib` cases plus `test_issue42_skill_blocks.py:266`.
|
||||
|
||||
Same file on a throwaway Python 3.11.15 venv (`/opt/homebrew/bin/python3.11`), `HOME` set to an empty temp directory:
|
||||
|
||||
**51 passed** in 0.38s, none skipped.
|
||||
|
||||
Afterward no `mamt45` process remained. `~/.config/herdr/config.toml` mtime stayed `1784456394`, the directory mtime stayed `1791183576`, and `mam-server-config.toml` was not created there. Snapshot `git status` was empty.
|
||||
|
||||
[VERDICT: PASS]
|
||||
Reference in new issue
Block a user