# Pre-Merge Independent Review Report: PR #35 (`fix/issue-23-24`) - **Job ID**: `c786c83f` - **Target Agent**: `agy` (Session: `herdr:reviewer-agy-01`) - **Role**: Reviewer - **Target Worktree Context**: - **Loop ID**: `loop-20260923T142448Z-609954` - **Base Commit**: `1e78dae6ebcb01773df7c4715c98b1786965a24a` (v4.7.1) - **Head Commit**: `f4ba205c9f755d817e2206c7a9cb390a3e61fd2f` - **Tree SHA**: `88cb4f34f1e9f8220a373350cfc600a53735f181` - **Verification Path**: `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20260923T142448Z-609954/1` --- ## 1. Executive Summary PR #35 addresses Issue #23 and Issue #24 ("교차 서버 herdr 스코프와 에이전트 무관 시작 다이얼로그, 실제 에이전트가 충족하는 온보딩 전달 판정") across 13 commits and 22 files (+2197 / −118). The branch eliminates cross-server scope leaks (`HERDR_WORKSPACE_ID` / `HERDR_PANE_ID` / `HERDR_TAB_ID`), externalizes agent start-up modal handling into declarative adapter facts, standardizes protocol-level `started` acknowledgement for agent onboarding, and implements robust fail-closed rollback handling (exit code 43 on unkillable sessions). All verification tasks were performed in the isolated verification path. All 73 tests in the new test suites (`test_issue23_24_startup_path.py`, `test_issue24_resume_dialogs.py`) and all 177 regression tests in the adjusted suites passed without error. --- ## 2. Detailed Verification Against Checklist Items ### (1) Issue #24 Startup Dialogs - **Adapter-Driven Declarative Modals**: - Signatures, response keystrokes, and dialog classifications (`trust` vs. `notice`) are defined per agent in `lib_py/agents/adapters/*.py` (`startup_dialogs` property) and exposed via `lib_py.agents facts` (`MAM_STARTUP_DIALOGS`). - Supported keys match target agent TUI specifications: * `agy`: `'Do you trust the contents of this project|Yes, I trust this folder'` -> `['Enter']` (`trust`). * `grok`: `'Do you trust the contents of this directory'` -> `['y']` (`trust`). * `codex`: `'Do you trust the contents of this directory|1\\. Yes, continue'` -> `['Enter']` (`trust`); `'1\\. Update now|Skip until next version'` -> `['2']` (`notice`, selects "2. Skip"). * `claude`: Full legacy ladder (`Enter` for trust/proceed, `Escape` for fullscreen upsell modal to preserve execution flags, `Down`+`Enter` for proceed). - **Cooldown & Ready Token Gating**: - Because herdr `--source visible` capture retains modal text on-screen temporarily after acceptance, re-sending keystrokes is rate-limited by `MAM_DIALOG_RESEND_S` (default 5s cooldown). - TUI readiness is strictly judged by adapter ready tokens (`MAM_STRONG_READY_TOKENS` / `MAM_READY_TOKENS`), never by modal disappearance (`test_s5c`, `test_s5d`). - **`--no-auto-trust` Flag & Workspace Scope**: - Setting `--no-auto-trust` (`MAM_AUTO_TRUST=0`) leaves dialogs of kind `trust` unanswered (warning printed to stderr for operator confirmation) while continuing to auto-handle `notice` dialogs (`test_s6`). - Crucial for `codex` security, where directory trust activates project-local configuration, hooks, and execution policies. - Automatic trust is only sent to sessions launched inside the designated `--workspace`. - **Contract Parity across Create and Resume**: - `create_session.sh` and `resume_session.sh` both invoke `handle_startup_dialogs "$SESSION_NAME" "$AGENT"`, parse `--no-auto-trust`, and share `_mam_load_dialog_facts` (`test_r1`, `test_r2`, `test_r3`, `test_r4`). ### (2) Issue #23 Cross-Server Herdr Scoping - **Targeted Scope Variable Stripping**: - In `lib.sh` (`mam_herdr`), `_herdr_call_is_cross_server` strips `HERDR_WORKSPACE_ID`, `HERDR_PANE_ID`, and `HERDR_TAB_ID` via `env -u ...` **only** when targeting a different server (`caller != target`). - Same-server calls retain workspace/pane scoping, protecting workspace packing and pane isolation (`test_s2`, `test_s1e`). Calls outside panes retain explicit scopes (`test_s2b`). - **Caller Server Identification**: - `_herdr_caller_server` checks `HERDR_SESSION` first. If unset (e.g. herdr 0.8.2 on the default server), it parses `HERDR_SOCKET_PATH`: * `*/.config/herdr/herdr.sock` -> `default` * `*/.config/herdr/sessions//herdr.sock` -> `` * Unknown formats -> `""` (treated safely as non-pane). - Verified across both default and named session socket layouts on herdr 0.8.2 and 0.9.0 (`test_s1c`, `test_s1d`, `test_s1f`, `test_s1g`, `test_s1h`). - **Python Watcher Parity**: - `lib_py/loop/watcher.py` implements identical logic (`caller_server()` and `scoped_env()`) for direct subprocess executions of herdr (`test_s13`). ### (3) Issue #23 Onboarding Delivery Verification - **Unified Command Builder & Predicate**: - `delegate_publish_cmd` in `lib.sh` generates the canonical publish command with `--expect-dispatch-token --attempt-id `. - `test_s7l` confirms `delegate_publish_cmd` produces identical arguments to the delegate-job wrapper's `_pub_cmd`. - Both `create_session.sh` and the wrapper (`_wait_started`) share `delegate_started_recorded` from `lib.sh`. - **Dispatch Attribution & Single-Injection Contract**: - Local events: `/.events.log` requires `event == "started"` and `data.dispatch_token == `. - Remote events: `//events.ndjson` requires `source_event == "started"`, `dispatch_verified is True`, and `dispatch_fingerprint == sha256()`. - Prevents a prior attempt's started record from acknowledging a subsequent re-dispatch (`test_w2`, `test_w7`). - Legacy records lacking a fingerprint fail closed when a token is demanded (`test_w8`). - **Elimination of Status Bypass**: - The previous wrapper fallback (`disk_status != pending -> success`) was completely removed. Both create and wrapper now strictly verify `started` event attribution (`test_w1`, `test_w4`). - **Terminal State Protection**: - If a job has already transitioned to a terminal status (`completed`, `error`, `cancelled`), `create_session.sh` leaves the job untouched rather than issuing a conflicting state change (`test_s7f`, `test_s7m`). - **Liveness Parity**: - Respects Issue #18 liveness contract (T=180s timeout, configurable via `MAM_ONBOARD_ACK_S`). ### (4) Issue #23 Rollback & Error Handling - **Kill Confirmation & Exit Code 43**: - `cleanup_herdr_on_error` checks the returncode of `kill-session` and affirmatively verifies session termination via `has-session`. - If the session remains alive, the script updates `agent-sessions.yaml` to `status: running` with `last_visible_status: rollback-failed`, clears `delegate_job_id`, prints manual cleanup instructions, and exits with code **43** (`test_s9`). - If cleanly killed, it marks `status: terminated`, `last_visible_status: rolled-back`, publishes `cancelled` for the onboarding job, and exits (`test_s10`). - **Session Preservation on Handshake Timeout**: - If an onboarding `started` event is not observed within `ONBOARD_ACK_S`, the onboarding job is marked `error`, but the session and its pane are **preserved** (`test_s7d`). Operators can re-submit onboarding via `multi-agent-mux-delegate-job submit` once the agent finishes initializing. ### (5) Impact on Existing Test Suites (6 Files) - `tests/conftest.py`: Injects `MAM_ONBOARD_ACK_S=3` into the `mam_sandbox` fixture to prevent sandbox runs without real agents from waiting 180s. - `tests/test_b19_headless_reconcile_fixes.py`: Relocated modal assertion to inspect `claude.py` adapter facts. - `tests/test_c1_tui_readiness.py`: Updated `test_c1b_hint_press_enter_sends_enter` to reflect time-based cooldown semantics (`MAM_DIALOG_RESEND_S`), verifying `1 <= count <= 30`. - `tests/test_issue14_delivery_ack.py`: Updated out-of-band agent acknowledgement to publish a genuine `started` event with token via `publish_event.py`. - `tests/test_issue18_liveness_contract.py`: Assertions match the protocol requirement (`recorded 'started'`). - `tests/test_issue5_phase2a_loop.py`: Refined G9 guard using Python AST parsing (`_code_only`) to strip only docstring expression nodes while preserving same-line code statements. Rigorously tested by `test_g9b_the_guard_still_catches_an_invocation_and_ignores_prose`. ### (6) Test Isolation & Reliability - New suites (`test_issue23_24_startup_path.py` [1268 lines] and `test_issue24_resume_dialogs.py` [127 lines]) operate completely within isolated sandboxes. - Real `registry.py` and `publish_event.py` utilities are exercised against isolated directories. - Zero mutation of control plane `.mam`, live agent sessions, or daemon processes. ### (7) Documentation Integrity - `multi-agent-mux-create/SKILL.md` and `multi-agent-mux-resume/SKILL.md` accurately document `--no-auto-trust`, exit codes 42/43, and `MAM_ONBOARD_ACK_S`. - Event and audit log schemas (`dispatch_fingerprint`, `dispatch_verified`) align with `MESSAGING.md`. ### (8) Merge Readiness - Version bump to `v4.7.2` is scheduled for the orchestrator release commit. - Code style, formatting, and diff integrity checks (`git diff 1e78dae...HEAD --check`) reported 0 issues. - Zero architectural, functional, or regression defects found. --- ## 3. Test Execution Summary | Test Suite | Result | Duration | Notes | | :--- | :--- | :--- | :--- | | `tests/test_issue23_24_startup_path.py` | **68 / 68 PASS** | 358s | S1–S13, S4–S6, S7–S10, W1–W8 test matrix | | `tests/test_issue24_resume_dialogs.py` | **5 / 5 PASS** | 8s | R1–R4 resume dialog parity | | `tests/test_issue5_phase2a_loop.py` | **91 / 91 PASS** | 3.1s | Loop orchestration & updated G9 AST guard | | `tests/test_issue18_liveness_contract.py` | **31 / 31 PASS** | 112s | Protocol liveness, subscriber audit & broker fallback | | `tests/test_c1_tui_readiness.py` | **23 / 23 PASS** | 45s | TUI readiness & rate-limited dialog responses | | `tests/test_b19_headless_reconcile_fixes.py` | **16 / 16 PASS** | 1.8s | Reconcile fixes & dialog exclusion assertions | | `tests/test_issue14_delivery_ack.py` | **12 / 12 PASS** | 98s | Delivery acknowledgement contracts | | `tests/test_version_consistency.py` | **3 / 3 PASS** | 0.2s | Version consistency checks | | `git diff 1e78dae...HEAD --check` | **0 errors** | 0.05s | Clean whitespace and formatting | --- [VERDICT: PASS]