Files
multi-agent-mux/.agents/reports/reviewer-grok-01/report-dee00bc0.md
T

17 KiB
Raw Blame History

Review dee00bc0 — PR #69 / Issue #63, fourth Mac pass

PASS. Head 1614af5 keeps a dead-owner marker whose session name is the caller's own until herdr returns a parsed agent list that omits that name, or until the row is registered for the attempt. A live owner of that name is kept. An empty session name is still dropped. A failed lookup, server_not_running included, and a wait cap of 0 leave the marker in place. A normal create or resume whose has-session already sees the name returns before this wait. A retry whose probe says the session is gone returns on that probe.

Reviewed only the snapshot /var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261023T035928Z-2ccc02/1 (loop-20261023T035928Z-2ccc02). Head 1614af5b673d7519d3a38f63e3636055592b4f14, tree f1af6fdd548ee5d6f6b81e2202833edfaca98188. Base dc886ecd6b38c01032a55a2d1c230c73362ae630 (main, v4.9.9) is an ancestor and is the control-plane HEAD. Thirteen commits, 14 files, +2234/−102. Against the previous review head 7cac6f6 the delta is 2 files, +42/−16: .agents/skills/lib_py/agents/id_order.py and tests/test_issue63_same_cwd_ids.py. No uncommitted changes. git diff --check is clean. Snapshot porcelain is empty and the snapshot has no .mam. Control-plane porcelain is empty. No product file was edited. The source worktree mam-wt-pr69 was not run.

The new commit is 1614af5 fix(id_order): a dead-owner marker on the caller's own session follows the orphan rules. It closes the residual recorded on 7cac6f6, where a dead owner of the waiter's own session name was dropped before herdr was asked. 8f64174 and 7cac6f6 stay underneath it. HEAD still prunes only on an authoritative gone, still treats server_not_running as unknown, and still asks herdr nothing at cap 0. The same-name case now uses that branch.

Host tools: control-plane venv Python 3.9.6, sqlite 3.51.0, /bin/bash 3.2.57(1)-release. The real agent databases were not opened.

Same-name orphan

_read_and_prune_markers no longer takes the waiter session name (id_order.py:183-186, call at 308-309). A dead owner's marker (id_order.py:217-229):

  • A missing owner pid, or an owner pid equal to this waiter, drops the marker (219-220).
  • A live owner keeps it, including a live attempt on the caller's session (221-222).
  • An empty session name drops it (223-224).
  • A row registered for the attempt drops it without a probe. Registered means status running and herdr_session_epoch greater than the marker's prev_epoch (177-180, 225-226).
  • Otherwise the probe runs. drop is true only when it returns false. True and None keep the marker, and _orphan is "alive" or "unknown" (227-229). A marker left by an earlier attempt on the caller's own session name takes this path.

herdr_session_state (id_order.py:58-86) is unchanged in this commit. False is only a successful, parsed _herdr agent list whose agents omit the session name and sanitize_herdr_agent_name of that name (84-86). A non-zero exit, a timeout, an OSError, or JSON that has no result.agents list returns None (75-83). timeout <= 0 returns None before any subprocess (64-65). Measured on this snapshot: herdr_session_state(..., 0) and herdr_session_state(..., -1) are both None. wait_s({}) is 600, wait_s of "0" is 0, and wait_s of "abc" is 600 (36-41).

The child environment sets MAM_NO_AUTOSTART=1 and HERDR_SESSION_NAME to the marker's server, or default when that field is empty (67). The shim skips autostart when that variable is 1 (lib.sh:203-208). bounded_probe passes the time left on the deadline (287-291). The call sits inside fcntl.flock(LOCK_EX). macOS has no flock(1). The ordering tests passed with fcntl.

blockers includes a kept marker of the caller's own session when that row is absent (133-147). The waiter then holds until the marker is pruned or the cap expires (325-347). On cap expiry the message uses the last definite answer from this wait (333-344). An unknown orphan at cap 0 adds MAM_ID_ORDER_WAIT_S=0 leaves no time to ask herdr.

Ordinary create, resume, and retry

Create calls has-session and exits 3 when the name is already present (create_session.sh:168-170). The id-order wait is later (287). Resume sets HERDR_ALIVE from has-session (resume_session.sh:125-128) and, when the session is alive, updates the row and exits (158-169) before the wait (243). The mock has-session exits 0 when the agent name is in its state (conftest.py:1345-1355). A retry that herdr still lists under that name therefore stops at has-session and does not enter the 600-second cap.

The wait itself returns 0 on the first lock hold when blockers is empty (310-323). That is a create or resume with no live same-cwd marker and no running row that still lacks an id. The default 600 is the cap for a marker that stays, and the function does not sleep it out on an empty blocker list.

When the caller has already passed has-session and a dead-owner marker of the same name remains:

  • A parsed list that omits the name removes the marker and returns 0 on that pass (228-229, then 310-323). The real-probe test does this with cap 2 on the marker's own session name and checks that the replacement marker's owner pid is the waiter (test_issue63_same_cwd_ids.py:1315-1317).
  • A parsed list that contains the name keeps the dead owner's pid and returns 1 inside the cap (1290-1292). The unit test at wait_s_val=0 with an injected pane_state prints its herdr session is alive and kill-session -t same-agy (1133-1140).
  • None, which covers a failed lookup, server_not_running, and a real cap of 0, keeps the marker and the waiter uses the cap. That is the same rule 7cac6f6 already applies to every other orphan. The real-probe cap-0 assertion is on the other session name in the same test (1306-1310); the same-name branch calls the same herdr_session_state.

The head suite finished in 1301.73 s, in line with the previous pass on 7cac6f6 (1263.90 s). A common create or resume that now slept out 600 seconds would have stretched that run. It did not.

Tests for this commit

no_herdr_probe (test_issue63_same_cwd_ids.py:32-36) makes an in-process herdr_session_state return false unless the test passes pane_state. The shell tests start a child, and that child runs the real function.

test_id_order_dead_owner_marker_blocks_while_its_unregistered_herdr_session_is_alive (1047) still covers another session's orphan: alive keeps and prints the kill line, unknown keeps, a registered running row drops the marker with an empty probe list, and a false answer admits the next session. The same-name block (1120-1148) keeps a live owner's pid, keeps a dead owner's pid while the injected probe returns true or None, and on false replaces the owner pid with the waiter's pid.

test_id_order_real_probe_against_mock_herdr_fails_closed (1250) sources the sandbox lib.sh and calls mam_id_order_wait. A listed agent returns 1 for both the other session name and the marker's own name, and the dead owner's pid stays (1287-1292). protocol_mismatch and server_not_running return 1 with herdr could not tell whether its session is gone (1294-1304). The mock exits 1 for force_error_code before the subcommand (conftest.py:486-488). Cap 0 returns 1 with MAM_ID_ORDER_WAIT_S=0 leaves no time to ask herdr and keeps the marker (1306-1310). An unlisted agent returns 0 for the other name, and a following wait on the marker's own name returns 0 with a new owner pid (1312-1317). The mock agent list prints {"result": {"agents": [...]}} with each agent's name (conftest.py:794-806). A parsed list that contains the name is what prints its herdr session is alive.

mock_herdr prepends its bin (conftest.py:1411). These shell tests passed in the head run below. The unlisted own-name case returning 0, and the listed own-name case returning 1, are the evidence the probe reached the mock.

test_id_order_create_waits_for_orphan_herdr_session_of_a_killed_create (1160) and test_id_order_herdr_probes_stay_within_the_wait_cap (1207) are unchanged by this commit and passed. The create test uses a different session name and cap 3. The cap test's cap 0 finishes with every budget <= 0 and both markers still on disk.

Rest of base..head

The twelve commits through 7cac6f6 are the tree reviewed in job 737239ab. This commit does not change session_ids.py, lib.sh, the stop, resume, or create scripts, or the skill exit tables. session_ids.py still uses #40's capture and claimed_elsewhere. Stop still decides the id before kill. Resume still resolves the id and refuses a claimed id before spawn. git grep for capture_conversation_id and CAPTURE_ID, excluding .agents/reports and IMPROVEMENTS.md, finds no caller. The diff names no loop or job-tree file, so the author's one-off tests/test_issue34_job_tree.py::test_t5b_a_*[False] failure is outside this change. /bin/bash -n accepts the snapshot lib.sh. Python 3.9.6 imports id_order. The annotations sit behind from __future__ import annotations.

Results

Pytest used the control-plane venv, PYTHONDONTWRITEBYTECODE=1, -p no:cacheprovider, and --basetemp /tmp/mamdee-dee00bc0-pytest (head) and /tmp/mamdee-dee00bc0-base-pytest (base). Neither path contains mam-review or shim. Inherited HERDR_*, MAM_*, MQTT_*, SKS_*, DELEGATE_JOB_LOGS_DIR, HOME_DIR, CLAUDE_*, CODEX_HOME, PYTHONPATH, WORKSPACE_ROOT, MAM_ENV_FILE, MAM_JOBS_DIR, AGENT_SESSIONS_YAML, the four XDG_* homes, and AGENT_PYTHON_BIN were dropped.

Head, cwd the snapshot: tests/test_issue63_same_cwd_ids.py, test_issue40_auto_pin.py, test_issue64_ro_connect.py, test_issue10_13_purge.py, test_issue31_purge_residual.py, test_issue42_skill_blocks.py, test_tier1_unit.py, test_tier2_component.py, test_uuid_target.py, test_issue24_resume_dialogs.py. 359 passed, 1 skipped in 1301.73 s. Exit 0. The log closed at 2026-10-23 14:09:03. .claude/skills is absent in this snapshot, and test_claude_skills_link_resolves_to_agents_skills skips in that case (test_issue42_skill_blocks.py:266). That file is outside this delta. The same-name assertions are inside the two existing probe tests, so the pass count matches the previous head.

Base, this turn: a throwaway git clone --shared checked out at dc886ecd6b38c01032a55a2d1c230c73362ae630, with only tests/test_issue63_same_cwd_ids.py copied from head. id_order.py is absent there. 27 errors in 0.11 s. Exit 1. Each error is ImportError: cannot import name 'id_order' from 'lib_py.agents' at setup of no_herdr_probe, including test_id_order_dead_owner_marker_blocks_while_its_unregistered_herdr_session_is_alive, test_id_order_real_probe_against_mock_herdr_fails_closed, and test_c4. The snapshot was not checked out. Snapshot porcelain stayed empty.

The behavioral base result for this same base, on the test file from before that fixture imported id_order, remains the one recorded by job ba3eeb7b: 22 failed, 1 passed in 181.15 s. The pass was test_c4_pinned_post_discovered_and_preassigned_agents_unchanged. On that run the second stop row received A's id, purge printed purged:, the unpinned resume carried A's id, and cap 0 still spawned. Those tests passed on this head inside the 359.

  1. Stop order A→B and B→A, including herdr already dead, records only a verified own id. test_c1_stop_same_cwd_agy_sessions_records_only_verified_owner passed on head.
  2. Purge of B leaves A's files. test_c2_purge_never_deletes_another_rows_conversation passed on head.
  3. Resume of an unpinned B stays off A's conversation, and the already-running branch keeps the epoch. test_c3_resume_unpinned_second_agy_row_starts_fresh_and_running_branch_keeps_epoch passed on head.
  4. A row that already has an own id, and claude/grok, stay on the previous path. test_c4_pinned_post_discovered_and_preassigned_agents_unchanged passed on head, along with test_issue40_auto_pin.py, test_issue64_ro_connect.py, and test_issue10_13_purge.py.
  5. A same-cwd create waits while another post-discovered row of the same agent has no id. test_c6_create_waits_or_refuses_while_same_cwd_post_discovered_row_has_no_id passed on head. The orphan tests cover a dead owner, including one on the caller's own session name, whose herdr session is alive, unknown, or gone.
  6. Resume refuses before spawn when the resolved id belongs to another row. test_c7_resume_refuses_when_recorded_id_is_claimed_by_another_row passed on head.

Blocking defects

None.

Non-blocking

  • While herdr keeps failing the lookup, the orphan marker stays, and every later create or resume that reaches the wait — a retry of the marker's own session name included — waits out MAM_ID_ORDER_WAIT_S again (id_order.py:75-76, 227-229). The wait is bounded. Create and resume still return at has-session when that command already sees the name (create_session.sh:168-170, resume_session.sh:158-169). The cap message names herdr kill-session.
  • The probe holds the id-order lock until it returns. The budget is the time left on the deadline, and at most 30 seconds (id_order.py:73, 287-309).
  • An empty herdr_server is probed on the default server (id_order.py:67). Create and resume export HERDR_SESSION_NAME before they write a new marker (create_session.sh:185-186, resume_session.sh:111-114). The cap line prints default for an empty server and leaves the server and session unquoted (id_order.py:335-344).
  • A successful agent list that is briefly empty while a pane is still registering would prune the marker. The tests cover a list that already contains the name, and a list that does not.
  • One remaining same-type row with herdr dead still uses find_workspace_uuid (session_ids.py:274-275).
  • A running row with an empty own id and a missing pane pid blocks until the cap (id_order.py:128-131). The unit test asserts that return of 1 (test_issue63_same_cwd_ids.py:1150-1154).
  • A marker whose owner pid has been reused by an unrelated live process stays until that process exits (id_order.py:221-222).
  • With herdr alive, a recorded id that fails revalidation is still returned (session_ids.py:240-241).
  • An unparsable MAM_ID_ORDER_WAIT_S becomes 600 (id_order.py:36-41).
  • stop_session.sh:220 uses eval "$(mam_session_ids stop ...)" || true. A normal decision returns 0. A crashed helper leaves the captured fields empty and the stop continues.
  • --no-onboard waits out the cap. Presence-lock id discovery is not in this PR. Both are the deferrals named by the issue and the PR body.
  • resolve_session_id.sh:4-5 still describes an empty line and exit 0. The script calls mam_session_ids under set -euo pipefail (line 10). The resume skill exit table documents exit 1 when another row claims the id.
  • Read-only connect residuals from #64 (immutable race, mode=ro sidecar rewrite, purge DELETE staying read-write) are outside this diff. _ro_connect is unchanged.

Host check

No process has a first command token of opencode or a path ending in /opencode. lsof on ~/.local/share/opencode/opencode.db exited 1. That database was not opened. Its mtime is 1792641027 and its size is 36712448. opencode.log is absent. ~/.config/opencode/service.json mtime is 1792246838 and its size is 64. ~/.local/state/opencode/service.json is absent. The herdr config mtime is 1784456394. ~/.local/state/mam mtime is 1791810241. ~/.local/state/mam/id-order is absent. These stats match the check taken before the suite.

Eight herdr processes were running immediately before the suite. After the suite log closed at 2026-10-23 14:09:03, the 14:09:37 check found seven. Two pids from the before-set were gone: one /opt/homebrew/bin/herdr whose session argument was multi-agent-mux (about 40 seconds old at suite start) and one whose session argument was mam-pylecturenotes (about 3 seconds old at suite start). The new process is /opt/homebrew/bin/herdr with --session multi-agent-mux, elapsed time about 54 seconds at 14:09:37, so it started near 14:08:43, during the last half-minute of the suite. No herdr command line contained the review basetemp, the snapshot path, or the test session names probe-ws and orphan-ws. This review did not start or stop a herdr server.

After this report file was written, the same check was repeated at 2026-10-23 14:11:38. Snapshot porcelain is empty and the snapshot has no .mam. Control-plane porcelain is empty at dc886ecd6b38c01032a55a2d1c230c73362ae630. No process has a first command token of opencode or a path ending in /opencode, and lsof still reports no database holder. The database mtime is still 1792641027 and its size is still 36712448. opencode.log is still absent. The config service file mtime is still 1792246838. The state service file is still absent. The herdr config mtime is still 1784456394. ~/.local/state/mam mtime is still 1791810241, and id-order is still absent. Seven herdr processes were present. The pid set differs from the 14:09:37 set by one homebrew herdr process whose arguments include --session multi-agent-mux; its elapsed time was about 54 seconds, so that process had turned over again. No herdr command line contained the review basetemp, the snapshot path, or probe-ws or orphan-ws. This review did not start or stop it.

[VERDICT: PASS]