GodopuandClaude Fable 5.1
cf5d1142e9
Issue #21 items 4, 5, 7 (worktree helpers, review identity, acceptance A1-A8), #20 test isolation, #27 .gitignore managed block, #28 host portability. VERSIONS.md carries the entry; the eight review reports of PR #29 (two rounds plus the codex re-review) are archived under .agents/reports/<session>/. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
123 lines
13 KiB
Markdown
123 lines
13 KiB
Markdown
# PR #29 Review Report: Worktree Operating Model Phase 2 (Items 4, 5, 7, Test Isolation #20, Deploy #27)
|
||
|
||
- **Reviewer**: Antigravity (`herdr:reviewer-agy-01`)
|
||
- **Job ID**: `f27519ff`
|
||
- **Target PR**: [PR #29](https://git.godopu.com/tmpl/multi-agent-mux/pulls/29) (`feat/issue-21-items-4-5-7` @ `46ea4cc`, base `main` `1134d74` / v4.6.0)
|
||
- **Review Target Worktree**: `/Users/godopu16/PuKi/laa/canary_projects/multi-agent-mux-wt/pr-29` (detached @ `46ea4cc`)
|
||
- **Base Repository**: `/Users/godopu16/PuKi/laa/canary_projects/multi-agent-mux` (`main` @ `1134d74`, unmodified)
|
||
- **Status**: Review Complete
|
||
|
||
---
|
||
|
||
## 1. Executive Summary
|
||
|
||
PR #29 implements the Phase 2 components of the Worktree Creator Operating Model (Issue #21 items 4, 5, 7), comprehensive test suite isolation (#20), and installer `.gitignore` handling (#27) across 20 commits (+3,229 / -108 lines, 25 files changed).
|
||
|
||
An independent review was performed from the dedicated worktree `/Users/godopu16/PuKi/laa/canary_projects/multi-agent-mux-wt/pr-29` without modifying any files in either the worktree or the main working copy.
|
||
|
||
All 8 verification items have been thoroughly analyzed and verified:
|
||
- **Worktree Helpers (`worktree_prepare.sh`, `worktree_remove.sh`)**: Rigorously safe against data loss (defenses R1–R6) with opt-in submodules, rollback on failed init, and guarded `--force` execution only after verifying that zero uncommitted changes, unpushed commits (including tags/detached HEAD), or stashes exist.
|
||
- **Resume CWD Guard**: Safely enforces resuming in original cwd (exit 2) while maintaining 100% backward compatibility for single-workspace users, and persists control-plane bindings on already-running paths.
|
||
- **Review Identity & Isolation**: Robust per-round snapshots `(base_sha, head_sha, tree_sha)`, detached review worktrees (`--review-isolation`), stale-round re-review on creator commit, pre-promotion re-check, and durable sidecar metadata (`.identity.json`). Default behavior without options remains byte-for-byte identical.
|
||
- **Test State Isolation (#20)**: Zero test leakage into real `.mam` verified live on the control plane. Ambient bindings are dropped before test collection, test-specific tmp roots are injected via autouse fixture, and session-scoped leak guards detect anomalies.
|
||
- **Test Suite Results**: 100% pass across all new and prerequisite test suites (132 tests passing deterministically). `git diff --check` passes with 0 errors.
|
||
- **Known Limitation**: Version bump to 4.7.0 (MINOR) and `VERSIONS.md` update are properly scheduled for the orchestrator release commit.
|
||
|
||
---
|
||
|
||
## 2. Item-by-Item Verification
|
||
|
||
### (1) Worktree Helpers & Data Loss Defenses (Issue #21 Item 4)
|
||
- **`worktree_prepare.sh`**:
|
||
- **`.venv` Handling**: Creates a symlink `ln -s "$MAIN/.venv" "$WT/.venv"` (linking to the control root's virtual environment rather than duplicating packages). If an existing non-symlink `.venv` is present, it issues a warning and leaves it intact. Commit `fadd38c` (#27) updated the managed `.gitignore` rule from `/.venv/` to `/.venv`, ensuring git ignores the `.venv` symlink in worktrees across all forms (file, directory, or symlink).
|
||
- **Shared Excludes**: Adds `/.venv` and `.pytest_cache/` directly into `$GIT_COMMON_DIR/info/exclude`.
|
||
- **Opt-in Submodules**: Submodules are initialized only when `--submodules` is explicitly supplied. Without `--submodules`, no submodule checkout or `modules/` directory is created.
|
||
- **Submodule Rollback (R3 & R4)**: If `git submodule update --init --recursive` fails, the helper rolls back any partially created submodule checkouts, removes `.git` pointer files, deletes newly created module gitdirs in `$gd/modules/`, and restores `.git/config` keys. Pre-existing submodule checkouts are preserved. If an uninitialized submodule path already contains files prior to init, it refuses upfront (`exit 3`) before modifying anything (R4).
|
||
- **`worktree_remove.sh` & Data Loss Defenses (R1–R6)**:
|
||
- Pre-flight checks run before any mutations:
|
||
1. Linked worktree check & lock check (`is-locked`, exit 3 if locked).
|
||
2. Running sessions check: refuses unless `--stop` is specified.
|
||
3. Open jobs check: verifies no open jobs exist for sessions in the worktree.
|
||
4. Clean working tree check: `git status --porcelain --ignore-submodules=none`.
|
||
5. Submodule inspection: verifies `dirty`, `unpushed`, and `stashes` across all repositories under `modules/` (including deinitialized and nested submodules via `_module_gitdirs`).
|
||
- **Post-Stop Re-Check (R1)**: After `--stop` halts sessions, open jobs are re-checked, and `judge_tree` is executed a second time immediately before removal. This prevents data loss if a session being stopped wrote files, committed, or created stashes during termination.
|
||
- **Unpushed Commits (R6)**: Uses `UNPUSHED_REVS = ("HEAD", "--exclude=refs/stash", "--all", "--not", "--remotes")`. This inspects commits reachable from HEAD, detached HEAD, local branches, and tags. Commits preserved only by local tags in submodules are protected.
|
||
- **Single `--force` Execution**: Git by design refuses plain `git worktree remove` on any worktree containing a `modules/` directory. `worktree_remove.sh` performs a single `git worktree remove --force` *only* after confirming R1–R6 pass completely. If any check fails, it exits with code 3 and leaves the entire worktree untouched. Manual, unguarded `git worktree remove --force` is explicitly warned against.
|
||
|
||
### (2) Resume Contract & Binding Persistence (`2679ddf`)
|
||
- **Original CWD Validation**:
|
||
- `resume_session.sh` verifies `mam_abs_workspace "$WORKSPACE"` matches `mam_abs_workspace "$ROW_CWD"`. If called with an altered workspace path, it terminates with `exit 2` and a clear error message (`Pass --workspace '$ROW_CWD' to resume it in its original workspace`).
|
||
- If the directory recorded in `pane.cwd` no longer exists (e.g. after worktree removal), it exits with code 2 explaining that recreating the worktree at that path makes the session resumable again.
|
||
- **Backward Compatibility**: For standard single-workspace users, the workspace matches `ROW_CWD`, so resume executes normally without disruption.
|
||
- **Already-Running Path Binding Persistence**: When resuming an already-running session, `CONTROL_PLANE_JSON="$CP_JSON" update_yaml_resumed.sh` is invoked, persisting the control-plane binding into `agent-sessions.yaml`.
|
||
- **Loud Binding Failure**: If control-plane binding computation fails:
|
||
- For a worktree (workspace != control root), it fails closed with `exit 2` ("refusing to resume a session that would use the wrong control plane").
|
||
- For the control root itself, it issues a WARNING and continues without `control_plane`, preserving resiliency.
|
||
|
||
### (3) Review Identity & Isolation (Issue #21 Item 5)
|
||
- **Snapshot Identity**:
|
||
- `lib_py/loop/runner.py` records `(loop_id, base_sha, head_sha, tree_sha)` for each review round.
|
||
- Verification targets are embedded directly into reviewer prompts.
|
||
- **`--review-isolation worktree`**:
|
||
- Creates a temporary detached worktree at the snapshot's review commit (with uncommitted/untracked changes pinned) so reviewers run tests without contaminating the creator's working tree.
|
||
- The review worktree is cleanly closed via `_close_review_tree` at the conclusion of each round, even upon failure (`_release_review_trees`).
|
||
- **Stale Round Detection**: If the creator commits or modifies files during an active review round, the round is identified as stale, approvals are discarded, and a fresh review round is triggered.
|
||
- **Pre-Promotion Re-Check**: Before promoting passed reviews to durable artifacts (`.mam/reviews/...`), `_identity_now()` is verified against the approved round's `(head_sha, tree_sha)`. If the tree changed after review approval, promotion fails closed with `exit 1`.
|
||
- **Durable Identity Evidence**: Promoted reports are accompanied by a JSON sidecar `report-{jid}.identity.json` detailing loop ID, SHAs, reviewer identity, review commit, and verification timestamp.
|
||
- **Remote Reviewers (`--remote-review`)**: Since remote reviewers cannot access local worktree paths, identity mode refuses remote reviewers by default, or allows them strictly as diff-only via `--remote-review diff-only`.
|
||
- **Dirty Submodules**: Submodule dirtiness is detected and causes reviews to be refused or marked stale.
|
||
- **Zero Drift Without Options**: When run without isolation/identity options, `identity` evaluates to `False`, no review worktree is created, and the prompt text matches previous releases byte-for-byte.
|
||
|
||
### (4) Test State Isolation (#20 Commits `3433ffd` ~ `46ea4cc`)
|
||
- **Ambient Binding Drop**: `tests/conftest.py`'s `pytest_configure` records and unsets ambient `AGENT_SESSIONS_YAML`, `MAM_JOBS_DIR`, `DELEGATE_JOB_LOGS_DIR`, and foreign `WORKSPACE_ROOT`.
|
||
- **Per-Test TMP Roots**: The autouse fixture `_isolate_mam_state_dirs` redirects `MAM_JOBS_DIR` and `DELEGATE_JOB_LOGS_DIR` to `tmp_path / ".mam-test-state"` for every individual test.
|
||
- **Resolution Tests Opt-Out**: Tests in `tests/test_issue15_registry_isolation.py` that test default fallback resolution explicitly opt out by calling `monkeypatch.delenv`.
|
||
- **Control Plane Leak Guard**: `mam_state_leak_guard` snapshots all real `.mam/jobs` and `.mam/delegate_job_logs` directories before test session execution and asserts at session finish that zero unexpected records were leaked.
|
||
- **Live Verification**: Inspection of the host control plane `.mam/jobs` and `.mam/delegate_job_logs` confirmed **0** leaked test records. Only active delegation job `f27519ff` is present.
|
||
|
||
### (5) Acceptance5 Stabilization Commits (`7bc3e1a`, `371099a`, `e6d2168`)
|
||
- **Session & Secret Randomization**: Uses a unique randomized session name (`sim-worker-{tag}`) and secret (`issue18-shared-secret-{tag}`) per run. This eliminates the collision failure mode where public MQTT broker retained messages from prior test runs were served to new tests.
|
||
- **Widened Idle Window & Terminal Event Recognition**: Expands missed heartbeat tolerance (10 misses x 2s) and recognizes delivery confirmed by retained terminal events (`advanced to 'completed'`).
|
||
- **Targeted Skip on Broker Non-Delivery**: Skips only when provable broker non-delivery occurs (e.g. no CONNACK from broker, or broker packet drop).
|
||
- **Contract Maintained**: Protocol assertions (`assert proc.returncode == 0`, `status == "completed"`, `transport == "remote"`) remain fully intact; the contract is not weakened.
|
||
|
||
### (6) Acceptance Cases A1–A8 & Test Suite Performance
|
||
- **Hermeticity & Determinism**:
|
||
- All 9 acceptance tests in `tests/test_issue21_acceptance.py` run against `mock_herdr` and `mam_sandbox`/`tmp_path`.
|
||
- Zero requirement for a live `herdr` daemon or external MQTT network broker.
|
||
- **Test Execution Metrics (executed in `pr-29`)**:
|
||
- `tests/test_version_consistency.py`: **3 passed** in 0.22s
|
||
- `tests/test_issue20_state_isolation.py`: **7 passed, 1 skipped** in 0.34s
|
||
- `tests/test_issue21_acceptance.py`: **9 passed** in 37.23s
|
||
- `tests/test_issue21_loop_b.py`: **32 passed** in 10.01s
|
||
- `tests/test_issue21_phase2_loop_a.py`: **25 passed** in 122.37s
|
||
- `tests/test_issue15_registry_isolation.py`: **7 passed** in 27.11s
|
||
- `tests/test_issue21_worktree_prereqs.py`: **35 passed** in 112.28s
|
||
- `tests/test_issue6_turn_handshake.py`: **14 passed** in 69.39s
|
||
- **Total**: **132 passed, 1 skipped**, 100% pass rate.
|
||
|
||
### (7) Documentation Integrity & Non-Blocking Recommendations
|
||
- **Documentation Alignment**:
|
||
- `docs/design/worktree-creator-model.md` accurately updates the implementation status matrix: items 1, 2, 3 (v4.6.0), items 4, 6 (Loop A), items 5, 7 (Loop B), with item 8 appropriately held for future work.
|
||
- Skill docs (`multi-agent-mux-create`, `multi-agent-mux-stop`, `multi-agent-mux-resume`, `multi-agent-mux-loop`) document helper script invocations, `--stop`, `--review-isolation`, and `--remote-review`.
|
||
- Compatibility matrix clearly defines runtime behavior across create, resume, stop, status, reconcile, delegate-job, and loop.
|
||
- **4 Non-Blocking Recommendations**:
|
||
1. A4 untracked comparison (pre-applying info/exclude).
|
||
2. info/exclude pre-reflection.
|
||
3. Prune global guidance when worktree directory is deleted externally.
|
||
4. Deduplication of submodule refusal messages.
|
||
All 4 are sound, low-severity improvements properly deferred to Issue #30.
|
||
|
||
### (8) Merge Readiness Assessment
|
||
- `git diff 1134d74...HEAD --check` completed with **0** errors (no trailing whitespace, no merge artifacts).
|
||
- Known limitation: Version bump to 4.7.0 (MINOR) and entry in `VERSIONS.md` are reserved for the orchestrator release commit.
|
||
- No functional regressions, security hazards, or data loss vulnerabilities found.
|
||
|
||
---
|
||
|
||
## 3. Final Conclusion
|
||
|
||
PR #29 completes Phase 2 of the Worktree Creator Operating Model with exemplary rigor in data safety, deterministic test isolation, and contract preservation.
|
||
|
||
[VERDICT: PASS]
|