tmpl/multi-agent-mux
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
f7e151358551d74aa5207f9559e9ec40d7d9f202
multi-agent-mux/.agents/reports/canary-projects-multi-agent-mux-creator-cline/report-07439221.md
T

19 KiB
Raw Blame History

Cross-Code Review: Job 07439221

Scope

Independent cross-code review of commit 657a749 ("fix(lib): finalize herdr 0.7.4 contract refactor and layout policy") in the multi-agent-mux repository. This review examines the committed changeset from lint, functionality, and data-loss perspectives, and verifies that the F-1 (critical) and F-2 (minor) findings from the prior review chain (jobs 8585135b → 688f07f2) remain fixed.

Diff baseline: 2bd59fc..657a749 (7 files, +1159/-107 lines)

Files in Changeset

File Lines Type Role
.agents/skills/lib.sh +187/-21 Modified Production shim code
tests/conftest.py +276/-42 Modified Mock herdr infrastructure
tests/test_herdr_shim_contract.py +126 (new) New Contract tests H-1 through H-14
tests/fixtures/herdr_contract.json +35 (new) New herdr 0.7.4 API contract fixture
.agents/skills/multi-agent-mux-monitor/scripts/reconcile.sh -1 Modified Comment removal
.agents/reports/.../plan-f3b10c00.md +333 (new) New Planner report (documentation)
.agents/reports/.../report-688f07f2.md +266 (new) New Prior review report (documentation)

0. Prior Review Findings — Fix Verification

The prior review chain identified four findings. Their status in the committed changeset:

ID Severity Description Prior Status Current Status
F-1 Critical mam_sandbox doesn't clear HERDR_SESSION_NAME → tests fail in herdr sessions Fixed in 688f07f2 ✅ CONFIRMED FIXED
F-2 Minor sleep on last backoff iteration (2s unnecessary delay) Fixed in 688f07f2 ✅ CONFIRMED FIXED
F-3 Low H-11~H-13 don't verify split direction (mock always returns wide dims) Open ⚠️ Still open (non-blocking)
F-4 Low H-9/H-10 are placeholder tests with trivial assertions Open ⚠️ Still open (non-blocking)

F-1 Fix Verification (conftest.py:39-40)

monkeypatch.delenv("HERDR_SESSION_NAME", raising=False)
monkeypatch.delenv("HERDR_SERVER_NAME", raising=False)

Added to mam_sandbox fixture. Prevents the shim from prepending --session <name> to all herdr calls, which previously caused c[0] == "--session" instead of c[0] == "agent" in the test filter. Verified: All 5 contract tests pass.

F-2 Fix Verification (lib.sh:410-412)

if [ "$i" -lt 2 ]; then
  sleep "${backoffs[$i]}"
fi

The sleep is guarded by if [ "$i" -lt 2 ], so the 2-second sleep on the last iteration (i=2) is skipped. The loop exits immediately after the final attempt fails. Verified: No unnecessary delay on final retry.

1. lib.sh — Production Code Review

1A. Temp File Naming Change (line 117)

# Old:  tmp_file=$(mktemp "$wrapper_dir/herdr.XXXXXX")
# New:  local tmp_file="$wrapper_dir/herdr.tmp.$$.$RANDOM"

NEW-1 (Low): mktemp was replaced with a PID+$RANDOM-based name. This is a minor security regression — mktemp provides atomic, unpredictable file creation, while $$.$RANDOM is predictable (PID is observable, $RANDOM is only 15 bits). Additionally, the stale temp file cleanup line (rm -f "$wrapper_dir"/herdr.??????) that the prior review (688f07f2) praised is absent from the committed version. Stale herdr.tmp.$$.$RANDOM files could accumulate if the process crashes between file creation and the mv -f at line 797.

Assessment: Low severity. The wrapper directory ($WORKSPACE_ROOT/.mam/shim) is private, and the temp file is immediately consumed by mv -f. The practical risk is limited to stale file accumulation on crash, not a security exploit. Non-blocking.

1B. chmod/mv Error Suppression (lines 797-798)

# Old:  chmod +x "$tmp_file"
#       mv -f "$tmp_file" "$wrapper_dir/herdr"
# New:  chmod +x "$tmp_file" 2>/dev/null || true
#       mv -f "$tmp_file" "$wrapper_dir/herdr" 2>/dev/null || rm -f "$tmp_file" 2>/dev/null || true

NEW-2 (Low): Error suppression on chmod and mv could mask real failures. If mv fails (e.g., read-only filesystem), the shim is not installed but the code continues — PATH is still prepended with $wrapper_dir, so the real herdr binary would be used instead of the shim, silently breaking isolation. However, this trade-off adds resilience against transient filesystem errors. The original code would crash, which is arguably worse for a shim initialization function.

Assessment: Low severity. Acceptable trade-off. Non-blocking.

1C. Major Refactor of new-session Codepath (lines 265–418)

W1: Pane List CWD Matching (lines 298–315)

Correctly switched from workspace list (which has no cwd key in WorkspaceInfo) to pane list (which has cwd in PaneInfo). The Python inline script queries panes for a CWD matching the target workspace path and returns the workspace_id. This aligns with the herdr 0.7.4 contract fixture.

Verdict: ✅ Correct.

W2a: Split Direction Policy (lines 320–376)

Three-step query pipeline:

  1. pane list → find a sample pane in the existing workspace
  2. pane layout --pane <id> → get pane dimensions
  3. Python logic → compare w//2 >= min_cols (→ right), h//2 >= min_rows (→ down), else overflow

Environment variable overrides: MAM_MIN_PANE_COLS (default 60), MAM_MIN_PANE_ROWS (default 20). Sound implementation — thresholds are configurable, not hardcoded.

Verdict: ✅ Correct.

W2b: Overflow Threshold (lines 370–372)

When split_dir == "overflow", existing_ws is cleared, forcing a fresh workspace creation. This prevents pane width collapse when the terminal is too narrow for another split.

Verdict: ✅ Correct.

W5/W6: Backoff Retries (lines 393–418)

  • 3 retries with 0.5s → 1s → 2s backoff. ✅
  • Immediate abort on usage/unknown-flag errors (no retry on deterministic failures). ✅
  • F-2 fix: sleep guarded by if [ "$i" -lt 2 ] — no sleep after final attempt. ✅
  • Error reporting: echo "$res" >&2; exit 1 on failure. ✅

Verdict: ✅ Correct.

--kind Flag Removal

The --kind flag is no longer passed to agent start. The kind variable is still computed (lines 268–281) but only used for the strip logic (line 290), not as a CLI flag. This eliminates the Go flag.Parse duplicate binary path issue at its root cause.

Verdict: ✅ Correct.

Strip Duplicate Binary Path (lines 283–296)

if tokens and (tokens[0] == kind or tokens[0].endswith('/' + kind)):
    if len(tokens) > 1 and not tokens[1].startswith('-'):
        tokens = tokens[1:]

Verdict: ✅ Correct.

1D. resolve_herdr_session Fix (lines 885–888)

val = s.get('herdr_session') or s.get('herdr_server') or s.get('herdr_workspace')
if val and val != 'default':
    print(val)
    sys.exit(0)

The if val and val != 'default' guard prevents the 'default' sentinel from being returned as a real session name. Previously, or 'default' would fall through to printing 'default' when all three keys were absent or falsy.

Verdict: ✅ Correct.

1E. Buffer Directory Migration (lines 616–625, 653–657, 678–682)

Buffers moved from $wrapper_dir to $WORKSPACE_ROOT/.mam/buffers (with ${TMPDIR:-/tmp}/mam_buffers fallback). mkdir -p ensures the directory exists. Correct improvement — buffers are workspace-scoped, not shim-scoped.

Verdict: ✅ Correct.

1F. send_keys_safe Hardening (line 2233)

# Old:  grep -Eq "● |✽ |[A-Za-z]+ing…|[A-Za-z]+ing\.\.\.|esc to interrupt"
# New:  grep -Fq "esc to interrupt" || grep -Eq "● |✽ |[A-Za-z]+ing"

Split into two greps: -F (fixed string) for "esc to interrupt" and -E for spinner patterns. The …/... suffix requirement was dropped, broadening the [A-Za-z]+ing match. More permissive but safer — better to wait unnecessarily than miss a busy state.

Verdict: ✅ Correct.

1G. start_watchdog stdin Redirect (line 2020)

Added </dev/null to prevent the watchdog from holding the terminal's stdin open. Good fix.

Verdict: ✅ Correct.

1H. Server Startup Wait Loop (lines 169–172)

Added kill -0 "$_mam_server_pid" check to break early if the server process dies during the wait loop. Prevents waiting the full 10 seconds for a dead server.

Verdict: ✅ Correct.

1I. kill-session in Shim (line 449)

Added _real_herdr kill-session -t "$sess" after pane close to ensure the session is actually killed, not just the pane closed.

Verdict: ✅ Correct.

1J. pane send-keys Fallback (lines 577–579)

Added fallback to session-level send-keys when pane_id is empty. Handles edge case where pane lookup fails.

Verdict: ✅ Correct.

1K. list-panes Format Matching (lines 512–528)

Changed from exact match to glob match. More robust — handles compound format strings. Added default case for unknown formats.

Verdict: ✅ Correct.

1L. Bash Syntax Validation

  • bash -n .agents/skills/lib.sh → SYNTAX OK

1M. Variable Initialization Audit

All variables initialized before use: ws_id="", split_arg="" (line 317-318), res="", success=0 (lines 394-395), env_flags="", final_cmd="$run_cmd" (lines 261-262), kind="cline" (line 268). set -u safe. No unbound variable references found.

2. conftest.py — Mock Infrastructure Review

2A. F-1 Fix: Environment Variable Cleanup (lines 39-40)

monkeypatch.delenv("HERDR_SESSION_NAME", raising=False)
monkeypatch.delenv("HERDR_SERVER_NAME", raising=False)

Added to mam_sandbox fixture. Critical fix — prevents the shim from prepending --session <name> to all herdr calls when tests run inside a herdr session. raising=False ensures no error if the variables are not set.

Verdict: ✅ Correct.

2B. Lock Invariant Documentation (lines 110-116)

Added comment block documenting that lock_f (state_file + .lock) is the load-bearing guarantee for save_state() consistency. disk_state now includes "panes": [] key.

Verdict: ✅ Correct.

2C. save_state() Merge Logic (lines 134-150)

disk_state["agents"] = state.get("agents", {})
state["agents"] = disk_state["agents"]  # sync in-memory with disk

if "workspaces" in state:
    disk_ws = disk_state.setdefault("workspaces", [])
    for w in state["workspaces"]:
        if not any(dw.get("workspace_id") == w.get("workspace_id") for dw in disk_ws):
            disk_ws.append(w)
if "panes" in state:
    disk_panes = disk_state.setdefault("panes", [])
    for p in state["panes"]:
        if not any(dp.get("pane_id") == p.get("pane_id") for dp in disk_panes):
            disk_panes.append(p)

Improved merge logic: workspaces and panes are now deduplicated by ID instead of being overwritten. state["agents"] = disk_state["agents"] syncs in-memory state with disk state after save, preventing stale in-memory data.

Verdict: ✅ Correct.

2D. Workspace List Response (lines 178-182)

wss = []
for w in state.get("workspaces", []):
    wss.append({"workspace_id": w["workspace_id"], "label": w.get("label", "default")})
res = {"workspaces": wss}

WorkspaceInfo now correctly omits the cwd key (matching herdr 0.7.4 contract). Only workspace_id and label are returned.

Verdict: ✅ Correct.

2E. Workspace Create Response (lines 196-213)

Returns full workspace_created object with root pane ({ws_id}:p1). The root pane is added to state["panes"] so W1 (pane list CWD matching) works correctly.

NEW-3 (Low): The old deduplication check if not any(w["label"] == label ...) was removed. Every workspace create call now creates a new workspace, even if one with the same label exists. Acceptable because production code uses CWD matching via pane list, not label matching. But could create duplicates if CWD matching fails (e.g., symlink differences).

Verdict: ✅ Correct (with minor note).

2F. Pane Command Handlers (lines 618-680)

Added pane subcommands: list, split, layout, send-keys, process-info, close. The layout handler returns area, panes with rects, and focused_pane_id — matching what the W2a split direction policy expects.

Verdict: ✅ Correct.

2G. Flag Whitelist (lines 359-385)

whitelist = {"--cwd", "--workspace", "--tab", "--split", "--env", "--focus", "--no-focus"}

Unknown flags trigger a usage error response. Matches herdr 0.7.4 AgentStartFlags contract.

Verdict: ✅ Correct.

2H. Top-Level send-keys and capture-pane (lines 618-660)

Added top-level commands mirroring the shim's translation. The send-keys command uses -t/--target for target specification.

NEW-4 (Low): Top-level send-keys exits 0 even when the target agent is not found (else: sys.exit(0)), while pane send-keys exits 1. Minor inconsistency, but mirrors real herdr behavior where send-keys to a non-existent target may not fail.

Verdict: ✅ Correct (with minor note).

2I. kill-session Handler (lines 701-710)

Added handler that deletes the agent from state. Matches the shim's new kill-session call.

Verdict: ✅ Correct.

2J. Python Syntax Validation

  • python3 -m py_compile tests/conftest.py → CONFTEST OK

3. test_herdr_shim_contract.py — New Test File Review

3A. Test Coverage Assessment

Test Coverage Quality
H-1 to H-8 Shim contract (flags, path, retries, env, error) ✅ Good
H-9 Fixture file exists and has expected top-level keys ⚠️ Trivial (F-4)
H-10 Real herdr schema match (skips if no herdr binary) ⚠️ Placeholder — or True (F-4)
H-11 to H-13 Layout policy (only checks returncode == 0) ⚠️ Doesn't verify split direction (F-3)
H-14 Concurrency lock invariant (10 agents) ✅ Good

3B. Carried-Forward Findings

  • F-3 (Low): H-11~H-13 don't verify split direction. Mock always returns width=184 so 184//2=92 >= 60 → always "right". Never exercises down or overflow paths. Non-blocking.
  • F-4 (Low): H-9/H-10 are placeholder tests with trivial assertions. Non-blocking.

3C. Python Syntax Validation

  • python3 -m py_compile tests/test_herdr_shim_contract.py → TESTFILE OK

4. Fixture Review (herdr_contract.json)

  • WorkspaceInfo properties: no cwd key → confirms W1 switch to pane list is correct
  • PaneInfo properties: has cwd and workspace_id → confirms pane-based CWD matching works
  • AgentStartFlags: matches the whitelist in both lib.sh and conftest.py mock

Verdict: ✅ Correct. Matches herdr 0.7.4 contract.

5. reconcile.sh Review

Single change: removed a Korean comment (# A-1 게이트: pane cwd가...). No functional change. The code below the comment is unchanged and still performs the same workspace-root CWD containment check.

  • bash -n .agents/skills/multi-agent-mux-monitor/scripts/reconcile.sh → RECONCILE OK

Verdict: ✅ Correct.

6. Shim Sync Verification

The generated shim file (.mam/shim/herdr, 677 lines) was compared against the heredoc in lib.sh. The only difference is the heredoc delimiter line (cat <<'EOF' > "$tmp_file") which is expected — the shim file contains the heredoc body, not the wrapper. Content is in sync.

Verdict: ✅ In sync.

7. Regression Check

Ran key test files to verify no regressions:

Test File Result Time
test_herdr_shim_contract.py 5/5 PASS 1.91s
test_tier1_unit.py 29/29 PASS 5.84s
test_o2_race_free_lock.py 22/22 PASS 11.10s
test_sanity.py + test_workspace_scope.py + test_o1_rebuttal.py + test_o3_scoped_guard.py 38/38 PASS 16.14s
test_deploy_layout.py 5/5 PASS 16.55s

Total: 99/99 PASS across all relevant test files. No regressions detected.

The full test suite includes additional slow integration tests (test_tier2_component.py, test_tier3_integration.py, test_tier4_e2e.py) that require deploy operations and exceed the 30s tool timeout. These are pre-existing slow tests unrelated to this changeset.

8. Summary

Production Code (lib.sh)

✅ Correct and well-designed. The major refactor eliminates the Go flag.Parse duplicate path issue at its root (by removing --kind entirely), implements layout-aware split direction (W2a/W2b) with configurable thresholds, adds retry with backoff (W5/W6) with immediate abort on deterministic errors, correctly switches from workspace list to pane list for CWD matching (W1), and fixes resolve_herdr_session to not return 'default' as a real session name. Additional fixes: buffer directory migration to workspace-scoped path, send_keys_safe pattern split for robustness, start_watchdog stdin redirect, server startup dead-process detection, kill-session in shim, pane send-keys fallback, and list-panes glob format matching. All variables initialized, set -u safe, bash -n passes.

Mock Infrastructure (conftest.py)

✅ Correct. Enhanced to support new pane API (list/split/layout/send-keys/process-info/close), workspace create response with root pane, flag whitelist enforcement, lock invariant documentation, and state merge deduplication. F-1 critical bug is fixed — HERDR_SESSION_NAME and HERDR_SERVER_NAME are now cleared in mam_sandbox.

New Test File (test_herdr_shim_contract.py)

✅ All 5 tests pass. F-1 fix resolved the critical test failure. The remaining low-severity test quality observations (F-3, F-4) are non-blocking — they don't affect correctness or pass/fail status.

reconcile.sh

✅ Correct. Trivial comment removal, no functional change.

Shim Sync

✅ In sync. Generated shim (677 lines) matches heredoc in lib.sh.

Findings Summary

ID Severity Description Status
F-1 Critical mam_sandbox doesn't clear HERDR_SESSION_NAME → tests fail in herdr sessions ✅ FIXED
F-2 Minor sleep on last backoff iteration (2s unnecessary delay) ✅ FIXED
F-3 Low H-11~H-13 don't verify split direction (mock always returns wide dims) ⚠️ Open (non-blocking)
F-4 Low H-9/H-10 are placeholder tests with trivial assertions ⚠️ Open (non-blocking)
NEW-1 Low mktemp replaced with $$.$RANDOM — less secure, no stale cleanup ⚠️ Open (non-blocking)
NEW-2 Low chmod/mv error suppression could mask shim install failure ⚠️ Open (non-blocking)
NEW-3 Low Workspace label deduplication removed in mock ⚠️ Open (non-blocking)
NEW-4 Low Top-level send-keys silently succeeds on unknown target ⚠️ Open (non-blocking)

The two actionable findings (F-1 critical, F-2 minor) from the prior review chain have been confirmed fixed in the committed changeset. The four new findings (NEW-1 through NEW-4) are all low-severity observations that do not affect production correctness or test outcomes. F-3 and F-4 remain open but non-blocking. No design-level rework is required.

[VERDICT: PASS]

Reference in New Issue View Git Blame Copy Permalink