release(v4.7.2): bump framework and eight skills to 4.7.2 in lockstep; archive PR #35 merge-side reviews
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
This commit is contained in:
GodopuandClaude Fable 5.1 committed
1 parent
05b0ea4762
commit
9fb037c084
27 files changed
+727
-26
No files matched your search
@@ -0,0 +1,14 @@
|
||||
{
|
||||
"loop_id": "loop-20260923T220355Z-76dd1d",
|
||||
"base_sha": "f4ba205c9f755d817e2206c7a9cb390a3e61fd2f",
|
||||
"head_sha": "05b0ea4762de3fad013e8986d45db21c4e6aa465",
|
||||
"tree_sha": "8f5e8b0402017d2cc809364e60400c4c25b922f3",
|
||||
"review_commit": "05b0ea4762de3fad013e8986d45db21c4e6aa465",
|
||||
"reviewer": "reviewer-agy-01",
|
||||
"job_id": "471806f8",
|
||||
"verdict": "PASS",
|
||||
"transport": "herdr",
|
||||
"evidence": "review-tree",
|
||||
"isolation": "worktree",
|
||||
"verified_at": "2026-09-23T23:35:19Z"
|
||||
}
|
||||
@@ -0,0 +1,69 @@
|
||||
# Pre-Merge Independent Delta Review Report: PR #35 (`fix/issue-23-24` Round 2)
|
||||
|
||||
- **Job ID**: `471806f8`
|
||||
- **Target Agent**: `agy` (Session: `herdr:reviewer-agy-01`)
|
||||
- **Role**: Reviewer
|
||||
- **Target Worktree Context**:
|
||||
- **Loop ID**: `loop-20260923T220355Z-76dd1d`
|
||||
- **Base Commit**: `f4ba205c9f755d817e2206c7a9cb390a3e61fd2f` (Round 1 target)
|
||||
- **Head Commit**: `05b0ea4762de3fad013e8986d45db21c4e6aa465` (1 delta commit: +76 / −5)
|
||||
- **Tree SHA**: `8f5e8b0402017d2cc809364e60400c4c25b922f3`
|
||||
- **Verification Path**: `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20260923T220355Z-76dd1d/1`
|
||||
|
||||
---
|
||||
|
||||
## 1. Executive Summary
|
||||
|
||||
This second review round examines delta commit `05b0ea4` ("fix(issue-23-24): merge review - the environment form of the auto-trust switch reaches the handler; a rate-limited dialog does not hide the next one"). The commit addresses two specific edge-case probes identified during the merge review of PR #35:
|
||||
1. **Probe P1**: In `create_session.sh` and `resume_session.sh`, hardcoded `AUTO_TRUST=1` unconditionally overwrote the documented environment switch `MAM_AUTO_TRUST=0`.
|
||||
2. **Probe P2**: In `lib.sh::_mam_dialog_pick`, hitting a dialog within its resend cooldown triggered `sys.exit(1)`, preventing subsequent modal notices (e.g. codex update skip) on the same screen from being answered.
|
||||
|
||||
Both fixes, their regression test coverage (`test_issue23_24_startup_path.py` and `test_issue24_resume_dialogs.py`), and documentation updates in `multi-agent-mux-create/SKILL.md` were evaluated. All 18 targeted tests and git diff validation passed with zero regressions.
|
||||
|
||||
---
|
||||
|
||||
## 2. Detailed Verification Against Checklist Items
|
||||
|
||||
### (1) Resolution of Codex Probes P1 & P2 and Side-Effect Analysis
|
||||
- **P1: Environment Variable Inheritance (`AUTO_TRUST="${MAM_AUTO_TRUST:-1}"`)**:
|
||||
- Previously, `create_session.sh` and `resume_session.sh` set `AUTO_TRUST=1` and subsequently exported `MAM_AUTO_TRUST="$AUTO_TRUST"`, clobbering caller-provided `MAM_AUTO_TRUST=0` when no CLI flag was supplied.
|
||||
- Initializing `AUTO_TRUST="${MAM_AUTO_TRUST:-1}"` allows `MAM_AUTO_TRUST=0` in the environment to take effect naturally.
|
||||
- An explicit CLI flag (`--auto-trust` or `--no-auto-trust`) re-assigns `AUTO_TRUST` to `1` or `0`, correctly asserting command-line precedence over environment variables.
|
||||
- Regression verified by `test_r3b` (resume) and `test_c3` (create) across all combinations (env=0 alone, env=0 with `--auto-trust`, and env=1 with `--no-auto-trust`).
|
||||
- **P2: Dialog Inspection Fallthrough via `continue`**:
|
||||
- In `lib.sh::_mam_dialog_pick`, when a dialog matched on screen is within its cooldown window (`0 <= now - last < cooldown`), changing the behavior from `sys.exit(1)` to `continue` permits evaluation of subsequent dialog signatures in the adapter's list.
|
||||
- If a trust dialog was skipped via `--no-auto-trust` (recorded in `_MAM_DIALOG_SENT`), or if it was answered on the preceding frame and is now cooling, a co-located notice dialog (such as the codex update prompt) is properly evaluated and answered on subsequent iterations.
|
||||
- Verified by new test `test_s6c` (skipped trust dialog does not hide notice below it) and `test_s5f` (dialog inside cooldown does not block answering the next dialog).
|
||||
- **Preservation of Cooldown Protection**:
|
||||
- `test_s5` and `test_s5b` continue to verify that an individual dialog cannot be resent while its cooldown is active, and can only be retried once the cooldown window expires.
|
||||
- Multi-dialog ordering and execution count remain deterministic: trust dialog answered first (`Enter`), followed by notice dialog (`2`), each sent exactly once (`test_s5f`).
|
||||
|
||||
### (2) Environment Precedence & Value Semantics
|
||||
- In shell scripts (`create_session.sh` / `resume_session.sh`):
|
||||
- `AUTO_TRUST="${MAM_AUTO_TRUST:-1}"` defaults to `1` when unset or empty.
|
||||
- Any user-specified value (e.g. `0`) is preserved and exported.
|
||||
- Command-line flags `--auto-trust` and `--no-auto-trust` overwrite `AUTO_TRUST` to `1` and `0` respectively, ensuring CLI options win over environment variables.
|
||||
- In Python (`_mam_dialog_pick`):
|
||||
- `auto_trust = os.environ.get("MAM_DLG_TRUST", "1") != "0"` cleanly maps string `"0"` to `False`, while any other non-"0" value defaults to `True`.
|
||||
- Behavior between shell scripts and Python dialog picker is fully aligned.
|
||||
- Documentation in `multi-agent-mux-create/SKILL.md` explicitly specifies `MAM_AUTO_TRUST=0` and documents that explicit flags take precedence over the environment.
|
||||
|
||||
### (3) Overall Merge Readiness
|
||||
- Commit `05b0ea4` reinforces the robustness and reliability of PR #35 without altering any public interfaces or state machine rules verified in Round 1.
|
||||
- Formatting and git diff checks (`git diff f4ba205...HEAD --check`) reported 0 whitespace or formatting issues.
|
||||
- Version bump to `v4.7.2` remains scheduled for the orchestrator release commit.
|
||||
- No blockers or regressions detected.
|
||||
|
||||
---
|
||||
|
||||
## 3. Test Execution Summary
|
||||
|
||||
| Test Suite / Target | Result | Duration | Notes |
|
||||
| :--- | :--- | :--- | :--- |
|
||||
| `test_issue23_24_startup_path.py` (`s6c`, `s5f`, `s5`, `s5b`) | **11 / 11 PASS** | 182s | Probe P2 cooldown fallthrough & multi-dialog handling |
|
||||
| `test_issue24_resume_dialogs.py` (`r3b`, `c3`, `r3`) | **7 / 7 PASS** | 8s | Probe P1 environment variable inheritance & CLI flag precedence |
|
||||
| `git diff f4ba205...HEAD --check` | **0 errors** | 0.05s | Clean formatting and no whitespace errors |
|
||||
|
||||
---
|
||||
|
||||
[VERDICT: PASS]
|
||||
@@ -0,0 +1,123 @@
|
||||
# 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]
|
||||
Reference in new issue
Block a user