GodopuandClaude Opus 5.5
7cf728e9f8
Review loop (--review-only, --all-reviewer, focused on this Mac: macOS, /usr/bin/python3 3.9.6, bash 3.2): round 1 (@63d8164) agy and grok PASS, claude and codex NOT PASS (F1 paho via the /usr/bin/python3 fallback, F2 logs default path, R1 disk-fallback redispatch interleaving, R2 symlinked registry), sent as a PR comment and fixed by the author in four commits; round 2 (@a7cb90e) 4/4 PASS. Full suite on macOS / Python 3.9.6 @a7cb90e: 1867 passed, 56 skipped, 1 error (the known #55 false positive: WAL/SHM of an agy conversation running in another workspace). Real herdr panes and ~/.claude/projects unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
48 lines
6.5 KiB
Markdown
48 lines
6.5 KiB
Markdown
# Review d6ae03a9 — PR #58, second pass on the four Mac findings
|
||
|
||
Reviewed only the snapshot `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261015T123502Z-1b9df6/1` (`loop-20261015T123502Z-1b9df6`). Head `a7cb90e63abf454ccf22f6ff887cc25393755402`, tree `96c5c5f82cb89e4383011d52fb1dcb21d4e787e6`, base `e32c9574644a9185813a4dc881812694d62b6204` is an ancestor. Fifteen commits, 23 files, +908/−131. The four commits after the first-round head `63d8164` are `75c0042`, `151af99`, `1c598bb`, and `a7cb90e`. `git diff --check` is clean. Snapshot porcelain is empty and the snapshot has no `.mam`. Control-plane main is still `e32c9574644a9185813a4dc881812694d62b6204`, porcelain empty. No product file was edited.
|
||
|
||
Host tools: `/usr/bin/python3` 3.9.6 has no `paho`. The control-plane venv is also 3.9.6 and does have `paho`. `/bin/bash` is 3.2.57. The changed modules parse with `ast` on 3.9.6. `bash -n` accepts the wrapper.
|
||
|
||
## F1 — path rules import without paho
|
||
|
||
`lib_py/delegation/logroot.py` is standard library only. `sibling_logs_dir`, `get_logs_dir`, and `default_registry_dir` live there. `mqtt_common.py:33-40` imports `paho` inside `try/except` and re-exports the logroot names, so `mqtt_common.sibling_logs_dir` is the same function. `publisher.py:17-20` does the same for its own `paho` import. `control_plane.py:28` and both `status.sh` copies (`status.sh:171`, and the text-mode copy) import `sibling_logs_dir` from `logroot`, not from `mqtt_common`.
|
||
|
||
`/usr/bin/python3` imports `lib_py.delegation`, `lib_py.control_plane`, and `logroot` in one process. Sourcing `lib.sh` with `AGENT_PYTHON_BIN=/usr/bin/python3` and `mam_control_plane_json` on a temporary workspace exits 0 and returns a `delegate_job_logs_dir` under that workspace's `.mam`. `make_client` still raises `ImportError` when a broker is actually required (`mqtt_common.py:286-288`).
|
||
|
||
`lib_py.delegation.logroot` is not named in `eager.CORE_MODULES`. Importing `lib_py.delegation.mqtt_common`, which eager load does, loads `logroot` as a dependency. On this interpreter that module is in `sys.modules` after that import.
|
||
|
||
## F2 — `logs` keeps the workspace default
|
||
|
||
`cmd_logs` (`multi-agent-mux-delegate-job:1096-1113`) passes `--registry-dir` only when the caller gave it. The empty array uses `${reg[@]+"${reg[@]}"}`. On bash 3.2 with `set -u`, an empty array expands to nothing and a two-element array expands to both words. `registry.py` then applies its own default: `MAM_JOBS_DIR`, then `WORKSPACE_ROOT`, then the cwd.
|
||
|
||
With `WORKSPACE_ROOT` set to a temporary workspace and the cwd set to a different directory, `registry.py register` wrote the job under the workspace `.mam/jobs` and the audit log under the workspace `.mam/delegate_job_logs`. `logs --list` from that other cwd printed the job id and exited 0. The other directory gained no `.mam`.
|
||
|
||
## R1 — a later dispatch is not the earlier one's success
|
||
|
||
`_check_disk_fallback` (`job_subscriber.py:121-138`) re-reads the registry record after it has read the audit status and before it adopts that status. It adopts only when `dispatch_token`, `attempt_id` (falling back to `iteration`), and `transition_seq` are unchanged. A changed record leaves the job pending for the next poll, which reads both again. The extra read is one `load_job` per non-terminal job on the audit path. The token check below still uses the first snapshot, which this comparison has just required to match the second read, and there is no further I/O between them.
|
||
|
||
The regression test interleaves `error`, then `redispatch` to attempt 2, then `completed`, inside `read_logged_status`. The caller waiting on attempt 1's token gets an empty terminal set. The next call, waiting on attempt 2's token, gets `completed`. A third call with attempt 1's token stays pending. On an archive of `63d8164` the same test fails with `B's completion was attributed to A`.
|
||
|
||
## R2 — a symlinked registry has one audit log
|
||
|
||
`sibling_logs_dir` (`logroot.py:51`) uses `os.path.realpath` of the registry, then the sibling `delegate_job_logs`. A relative registry still resolves from the cwd. `forwarded_keys` (`control_plane.py:82-84`) compares `_abs` of both sides, so a `/tmp` path and its `/private/tmp` real path still count as the same sibling and the derivable root stays off the pane.
|
||
|
||
A probe under `/tmp` (`/tmp` is a symlink to `/private/tmp`): `from_env` with no `MAM_JOBS_DIR` equals `defaults`; `get_logs_dir()` is that same directory; a jobs path given through `/tmp` is not forwarded; a jobs directory that is itself a symlink to another parent pairs with the target's sibling, and the binding matches that directory and is not forwarded.
|
||
|
||
`submit --dry-run --registry-dir <tmp>/alias/jobs`, where that path links to `<tmp>/actual/jobs`, wrote `meta.json` under `<tmp>/actual/delegate_job_logs` and printed that directory. `<tmp>/alias/delegate_job_logs` was not created, and neither was the workspace audit root. `logs --registry-dir` of the alias printed `registered`.
|
||
|
||
## Tests
|
||
|
||
Pytest used the control-plane venv, `PYTHONDONTWRITEBYTECODE=1`, `-p no:cacheprovider`, and `--basetemp` `/tmp/mamd6-d6ae03a9-pytest`. Inherited `HERDR_*`, `MAM_*`, `MQTT_*`, `SKS_*`, `WORKSPACE_ROOT`, `DELEGATE_JOB_LOGS_DIR`, and `PYTHONPATH` were dropped. `AGENT_PYTHON_BIN` pointed at that venv.
|
||
|
||
On the snapshot, 25 tests passed in 50.76s: all of `tests/test_issue57_logs_follow_registry.py`, including the four new repros (control-plane without paho, status json and text without paho, logs from another cwd, the interleaved redispatch, and the symlinked registry), plus `test_t3_0_control_plane_module`, `test_logs_dir_follows_the_same_order`, and `test_b9_logs_dir_follows_cwd_changes`.
|
||
|
||
The six new test cases copied onto an archive of `63d8164` failed in 2.55s: the control-plane import traceback, both status modes, `logs` reading `<cwd>/.mam/delegate_job_logs`, the redispatch attributed to the earlier attempt, and `ImportError` for `logroot`. Scratch directories were removed. Snapshot porcelain stayed empty.
|
||
|
||
The full suite was left to the orchestrator. `SKILL.md`'s Audit Logs sentence still names `.mam/delegate_job_logs/<job_id>/`, which is the directory this rule produces when the registry is `.mam/jobs`. The design-doc status row now lists the sibling lookup (`docs/design/worktree-creator-model.md:128`).
|
||
|
||
The pane herdr config file is newer than the mtime seen at the end of the previous review. This review did not start herdr and did not write that file. `~/.config/herdr/config.toml` is unchanged. That mtime is not part of the verdict.
|
||
|
||
[VERDICT: PASS]
|