GodopuandClaude Opus 5.5
8fde39be33
Merge-workspace review: round 1 (@518beda) claude agy grok PASS, codex NOT PASS (finalize wait measured on the wall clock); fixed in4c86107. Round 2 delta (@4c86107) claude agy grok PASS; codex (24b21b9c, after 817d1650 closed deadline_timeout with no report) found R1 closed and no new defect, but withheld PASS only because its sandbox could not bind the suite's loopback broker. The same two-file suite run here at4c86107: 278 passed. Full suite at518bedaon macOS / Python 3.9: 1711 passed / 1 failed / 38 skipped; the failure (test_issue45_restore, herdr 0.9.3 cwd restore) also fails on main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
71 lines
10 KiB
Markdown
71 lines
10 KiB
Markdown
# Review: PR #51 @ 518beda
|
||
|
||
- Role: Reviewer (grok)
|
||
- Identity: loop `loop-20261007T142403Z-10d77f`, base `7209750ac7e3ab9a7f6b0abc3670dbe0b1060f88` (main, v4.8.5), head `518bedac5d8ab832347bfc6682b146f3201021f1`, tree `137b4e46c58b4f27e03dc19e6f9c6b42f574baa3`, ancestor of head, clean snapshot
|
||
- Scope: `git diff 7209750 HEAD`. 15 files, +1353/−292, `git diff --check` clean. Main stayed `7209750`.
|
||
- Verified read-only at `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261007T142403Z-10d77f/1`. Python 3.9.6, bash 3.2.57.
|
||
|
||
The contract is behavior invariance with 7209750: exit codes, terminal labels, registry writes, event publish, and the stall watcher. The frozen copy in `tests/data/issue17_item4_old_decisions.sh` matches that base byte for byte: `mam_int` and `mam_int_knob` are the old `lib_int.sh` bodies, and `start_stall_watch`, `_disk_status`, `_recent_disk_heartbeat`, `_map_sub_rc`, `_rc_for`, `finalize_unfinished`, and `cmd_wait` are the old wrapper bodies.
|
||
|
||
## 1. Decisions
|
||
|
||
`finalize_begin` (`wrapper.py:161-187`) and `finalize_end` (`:196-273`) follow the old function's order.
|
||
|
||
- `MAM_DELEGATE_FAIL_CLOSED=0` or status `completed` returns `keep`. The wrapper returns before any close (`multi-agent-mux-delegate-job:812-813`).
|
||
- A disk status of `completed` returns `done completed` (`:176-177`) and the wrapper sets `FINAL_JOB_STATUS` and returns (`:814`).
|
||
- Broker re-observe sleeps `min(left, 3)` first, then reads the disk, and stops at the wall deadline (`:201-216`). A disk `completed` during that wait returns immediately. `error` or `cancelled` becomes the final status and leaves the loop.
|
||
- The timeout grace opens `evidence_until` before the herdr probe (`:181`, then the probe at `:817-821`), same order as the old function. Each pass reads the disk, re-opens the idle window from a fresh events.log line (`recent_disk_heartbeat`, `:102-115`), and otherwise stops when the idle window or the wall deadline is reached (`:223-238`). The sleep inside that loop is 1 s.
|
||
- The label is `wall_clock_timeout` when the clock is the wall and the phase is anything but `inject`; otherwise `deadline_timeout` (`:245`). Subscriber rc 4 forces the wall clock (`:243-244`). `preflight` and `idle_wait` close as `cancelled` / `session_busy`, and a non-empty preflight reason replaces `session_busy` (`:247-251`).
|
||
- Registry close rc 4 suppresses the event and returns the final status (`:258-262`). A disk `completed` after a successful close returns `completed` with no event (`:263-264`). Only `type=direct` publishes, and an error publish is skipped when the subscriber rc is 1 unless the phase is `inject`, which publishes `agent_blocked` (`:266-272`).
|
||
- `fail_closed` is off only for the exact value `0` (`:73-74`). `exit_code` then returns 0 (`:77-78`). The inject failure path still exits 1 (`delegate-job:384`, `:563`), which is the exit 7209750 used there.
|
||
- Remote is `remote:*`, the same case as 7209750's `_is_remote` (that line is `remote:*)`). `stall_watch` uses `session.startswith("remote:")` (`wrapper.py:93-95`). Human is the agent or the bare session (`:96-98`).
|
||
|
||
The comparison test runs the sourced old functions and the new wrapper on the same input (`tests/test_issue17_item4_decisions.py:232-253`). The matrix is 6 statuses × 6 phases past the wall clock (`:274-284`), plus idle clock, human, remote, the fifth-arg session, loop, discuss, disk completed/error/cancelled/missing/garbage, empty token, empty and `target_not_found` preflight reasons, `idle_wait`, close rc 4 and 1, `MAM_DELEGATE_FAIL_CLOSED=0`, a completed status with a disk error, and a 1 s broker re-observe (`:287-322`). Heartbeats that stay fresh run to the wall clock (`:325`), and a record that becomes `completed` during the grace ends as `completed` with no close (`:337`). Those tests passed.
|
||
|
||
## 2. Bash / Python boundary
|
||
|
||
`_decide` is the only helper (`delegate-job:99-101`): `python -m lib_py.delegation.wrapper` with the caller's arguments. On this bash, an empty session and agent, a session containing a space, and an agent containing a quote are one argv each. `stall-watch` accepts them and returns 0, which is "watch" for a session that is neither remote nor human.
|
||
|
||
A missing module inside an assignment aborts under the wrapper's `set -euo pipefail` (rc 1). That covers `sub-status` and `exit-code`. `read` of a failed `finalize-begin` does not itself abort; `step` is empty, and the following `finalize-end` assignment then aborts, so the close does not run. `exit "$(failing python)"` is rc 255 (`numeric argument required`). `subscriber-up` is in an `if`, so a failed interpreter is "not up", and the assignment that follows abort the script. No decision name remains in the wrapper: the static guard strips comments and rejects `_map_sub_rc`, `_rc_for`, `_disk_status`, `_recent_disk_heartbeat`, `FINALIZE_CLOCK`, the two knob expansions, subscriber-rc comparisons, the four close labels, and `BASH_REMATCH` in `lib_int.sh` (`test_issue17_item4_decisions.py:471-482`).
|
||
|
||
A positional or option value that itself starts with `-` is parsed as a flag (`stall-watch` of `-herdr:x` exits 2, and `|| return 0` at `delegate-job:692` then skips the watcher). Live sessions are `herdr:` or `remote:`. If a leading dash should stay positional, the call wants `--` before the session and agent.
|
||
|
||
## 3. Wrapper liveness
|
||
|
||
`_alive` (`wrapper.py:122-137`) is `kill(0)`, then `ps -o stat=`. `ProcessLookupError` is dead, `PermissionError` is alive, an empty state or a state starting with `Z` is dead, and a `ps` error is alive. That is the same rule as `tests/conftest.py:100-109`. macOS and Linux both mark a zombie `Z` or `Z+`, so the prefix covers both. The check runs inside every sleep (`:144-153`), immediately before close (`:256`), and immediately before publish (`:267`). `main` catches `OwnerGone`, writes nothing on fd 3, and returns 0 (`:336-340`), so the wrapper's `FINAL_JOB_STATUS` stays empty and no close line was captured.
|
||
|
||
`_sleep` checks the owner, sleeps at most 0.5 s, and returns when the requested slice is over (`:144-153`). The grace slices stay 1 s and `min(left, 3)`. The task text says 0.2 s; the code uses 0.5 s. The check sits inside the existing slice, so the close clock does not gain a step. TERM during either grace, an unreaped zombie, and a real registry record of type `loop` all leave the record unchanged and publish nothing (`test_issue17_item4_decisions.py:357-392`, `:517-593`). Those tests passed on this Mac.
|
||
|
||
## 4. Integers
|
||
|
||
`positive_int` (`ints.py:25-34`) is the body that `runner.py` used to inline. The runner now aliases it (`runner.py` imports `lib_py.ints`). ASCII blanks only, ASCII digits only, leading zeros stripped, more than 9 significant digits rejected before `int()`, then `minimum <= n`. `knob` returns the default for an empty value with no warning, and warns on an invalid value (`:37-45`).
|
||
|
||
`lib_int.sh:15-24` is only `python3 -I` of that file. Isolated CLI on this host: `0700` is 700, `0999999999` is 999999999, `1000000000` is rejected, the Arabic digit `٥` is rejected, an empty knob prints the default, and a whitespace-only knob warns and prints the default. `test_c_mam_int_and_knob` compares the old bash functions with `lib_int.sh` under `C` and `en_US.UTF-8`.
|
||
|
||
`diff_collect.sh:14` and `loop_lock.sh:7` source `lib_int.sh` when `mam_int_knob` is not already defined. The script path is next to `lib_int.sh`, independent of the caller. `diff_collect.sh` reads two knobs at source time (`:15-16`); `loop_lock.sh` reads one (`:8`). A missing `python3` returns that command's status. The rest of the wrapper already requires Python. `test_l15_truncated_diff_refuses_diff_only_but_not_local` copies `ints.py` beside the copied `lib_int.sh` (`test_issue21_loop_b.py:397-398`) and passed in 1.02 s (`--basetemp=/tmp/c7eel15.inE8zI`).
|
||
|
||
## 5. Static pins
|
||
|
||
The four edited tests still pin the contract, and they execute it.
|
||
|
||
- `test_issue5_phase1_delegation.py:385-404` requires `_decide sub-status`, `_decide exit-code`, and `--sub-rc`. `EXIT_CODES` is completed 0, error 1, timeout 2, broker_unavailable 3, cancelled 5. `sub_status` for rc 0..5 is completed, error, timeout, broker_unavailable, timeout, timeout. The wrapper's `SUB_RC_*` tuple matches `job_subscriber`'s `RC_*`, and the wrapper does not compare subscriber codes to raw digits.
|
||
- `test_tier1_unit.py` `test_g10` calls `sub_status(3)` and gets `broker_unavailable`, then `completed` once the disk record is `completed`.
|
||
- `test_issue26_ints.py` drops `MAM_DELEGATE_FAIL_CLOSED` and `MAM_STALL_WATCH` from `NOT_ARITHMETIC` because wrapper.py reads them. B5 still fails if a new `${MAM_…:-digit}` appears unclassified. The wrapper guard rejects those two expansions.
|
||
- `test_issue21_loop_b.py` only adds the `ints.py` copy the new `lib_int.sh` runs. The l15 test passed.
|
||
|
||
`tests/data/skill_code_blocks.json` keeps class `c` and kind `invocation`. The anchors moved with the deletions: `main()` is `delegate-job:1046-1057`, the runner usage line is `runner.py:155`, and `runner.main` is `runner.py:1779`. `publish_event.py` and `MESSAGING.md` rename `_recent_disk_heartbeat` to `recent_disk_heartbeat`.
|
||
|
||
## 6. Performance
|
||
|
||
A direct job that reaches the subscriber starts Python five times: `stall-watch`, `sub-status`, `finalize-begin`, `finalize-end`, and `exit-code`. Five `exit-code` calls took 0.35 s on this host, about 70 ms each. `finalize-end` is the process that sleeps; the 1 s and 3 s waits are not a new interpreter each time. Disk status and the heartbeat read used to be a `python -c` per poll. They are in-process now (`disk_status`, `recent_disk_heartbeat`). `cmd_wait` adds one `wait-rc` after the subscriber returns (`delegate-job:1042`). The integer knobs above are once per source of `diff_collect.sh` or `loop_lock.sh`.
|
||
|
||
## 7. Portability
|
||
|
||
Added Python is stdlib, `from __future__ import annotations`, and `Optional` / `List`. It compiles with the 3.9.6 venv named in the brief. No `tomllib`, no `match` statement, and no `/proc` read (the `_alive` comment says `ps` is used). The wrapper diff adds no `mapfile`, `readarray`, `&>`, or associative array. The suite ran on this macOS with that interpreter and the system bash 3.2.57.
|
||
|
||
## Tests
|
||
|
||
`AGENT_PYTHON_BIN` is the pr-51 venv. MQTT variables and `MAM_ENV_FILE` were unset. `--basetemp=/tmp/c7eefollow.h4VCjP`. The nine files in the brief: **604 passed, 1 skipped** in 1006.68 s, exit 0. The skip is `tests/test_issue30_stall_watch.py:40`, real process trees via `/proc`, Linux only. Snapshot `git status` was empty after the run, and again after the l15 test. No product file was edited.
|
||
|
||
[VERDICT: PASS]
|