GodopuandClaude Fable 5.1
9fb037c084
Issues #23 and #24: cross-server herdr scope, agent-agnostic start-up dialogs, onboarding delivery judged by the agent's own started event, verified rollback. VERSIONS.md carries the entry; the merge-side reports (round 1: claude/agy/grok; round 2 on05b0ea4: all four, with identity sidecars, basef4ba205-> head05b0ea4, evidence review-tree) are archived under .agents/reports/<session>/. The round-1 codex report was lost to the host sleeping mid-review; its two probes are recorded in the changelog. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
124 lines
10 KiB
Markdown
124 lines
10 KiB
Markdown
# 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" <timeout> "$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/<name>/herdr.sock` -> `<name>`
|
||
* 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 <token> --attempt-id <attempt>`.
|
||
- `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: `<registry>/<job_id>.events.log` requires `event == "started"` and `data.dispatch_token == <token>`.
|
||
- Remote events: `<logs_dir>/<job_id>/events.ndjson` requires `source_event == "started"`, `dispatch_verified is True`, and `dispatch_fingerprint == sha256(<token>)`.
|
||
- 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]
|