GodopuandClaude Fable 5.1
d34b532ccf
Issue #21 phase-1 prerequisites (PR #22) add loop runner options and the
control-plane module: MINOR. VERSIONS.md gains the v4.6.0 entry, the
archived design reviews lose their markdown trailing whitespace so
`git diff --check` is clean, and the three merge-side PASS reports
(claude, agy, codex @ 4a81d15) are archived under .agents/reports/.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
131 lines
10 KiB
Markdown
131 lines
10 KiB
Markdown
# PR #22 Review Report: Worktree Creator Operating Model Prerequisites (Phase 1)
|
|
|
|
- **Reviewer**: Antigravity (`herdr:reviewer-agy-01`)
|
|
- **Job ID**: `de72b12e`
|
|
- **Target PR**: [PR #22](https://git.godopu.com/tmpl/multi-agent-mux/pulls/22) (`feat/issue-21-worktree-prereqs` @ `4a81d15`, base `main` `5611859`)
|
|
- **Target Worktree**: `/Users/godopu16/PuKi/laa/canary_projects/multi-agent-mux-wt/pr-22` (detached @ `4a81d15`)
|
|
- **Base Repo**: `/Users/godopu16/PuKi/laa/canary_projects/multi-agent-mux` (`main` @ `5611859`, unmodified)
|
|
- **Status**: Review Complete
|
|
|
|
---
|
|
|
|
## 1. Executive Summary
|
|
|
|
PR #22 implements Phase 1 prerequisites for the Worktree Creator Operating Model (addressing Issue #21 items 1, 2, 3, and 6). This review was conducted independently in worktree `/Users/godopu16/PuKi/laa/canary_projects/multi-agent-mux-wt/pr-22` without modifying any files in either the worktree or the main working copy.
|
|
|
|
All 9 checklist verification items have been thoroughly inspected and verified against the codebase and test suites.
|
|
|
|
Two known limitations (pre-identified and documented in the brief) were verified:
|
|
1. **Version Lockstep**: Currently at 4.5.0. New CLI flags (`--worktree`, `--base`, `--review-only`) and new module `lib_py/control_plane.py` require a MINOR bump to 4.6.0 and an entry in `VERSIONS.md`. This is scheduled to be performed by the orchestrator in the merge release commit.
|
|
2. **Markdown Trailing Whitespace**: `git diff 5611859...HEAD --check` flags trailing whitespace on lines in `docs/design/reviews/review-agy-fd384d6b.md` used for markdown line breaks (` \n`). No source code (`.py`, `.sh`) contains trailing whitespace or syntax issues. This is also scheduled for cleanup in the orchestrator release commit.
|
|
|
|
Beyond these two orchestrator release tasks, PR #22 is clean, robust, backwards-compatible, and passes all prerequisite test suites (100% pass rate on relevant tests).
|
|
|
|
---
|
|
|
|
## 2. Checklist Item-by-Item Verification
|
|
|
|
### (1) Wrapper Absolute Paths (`d3d3a76`)
|
|
- **Path Resolution**: In `multi-agent-mux-delegate-job`, the brief file path, the `--registry-dir` argument in injected commands (`start`, `success`, `error`), the `Output Report Path`, and the audit log root (`DELEGATE_JOB_LOGS_DIR`) are strictly resolved to absolute paths rooted at `$REGISTRY_DIR` / canonical root, independent of the Creator's working directory (`$CWD`).
|
|
- **Shell Quoting**: All path interpolations are enclosed in double quotes (`"$REGISTRY_DIR"`, `"$BRIEF_FILE"`), preventing word splitting on paths with spaces or special characters.
|
|
- **Contract Alignment**: Conforms with existing prompt injection contracts:
|
|
- Issue 14 (Delivery ACK): Start command with `--event started` and token verification remains intact.
|
|
- Issue 6 (Turn Handshake): Handshake status sequence and wrapper idle checks remain compatible.
|
|
- Issue 18 (Liveness Contract): `--heartbeat <N>` argument in start command is properly formatted.
|
|
- **Tests**: `tests/test_issue21_worktree_prereqs.py` tests `test_t1_1_wrapper_emits_absolute_brief_path`, `test_t1_2_wrapper_emits_absolute_start_command_registry_dir`, `test_t1_3_wrapper_emits_absolute_audit_log_path`, `test_t1_4_wrapper_emits_absolute_output_report_path`, and `test_t1_5_wrapper_handles_spaces_in_root_path` all pass.
|
|
|
|
### (2) Loop Runner Worktree Integration (`d907359`)
|
|
- **CLI Flags**: `lib_py/loop/runner.py` introduces `--worktree <path>`, `--base <ref>`, and `--review-only`.
|
|
- **Worktree vs Control Plane Separation**:
|
|
- `collect_changes_diff()` and `base_commit` resolution execute with `cwd=str(self.worktree_root)`. The diff passed to peer reviewers reflects changes in the worktree against base.
|
|
- Control plane artifacts—the delegation wrapper (`multi-agent-mux-delegate-job`), jobs registry (`$MAIN/.mam/jobs`), loop checkpoint file (`$MAIN/.mam/loop-checkpoints/...`), and promoted reports (`$MAIN/.mam/reviews/...`)—remain strictly anchored to the main repository root.
|
|
- **Fail-Closed Empty Change-Set Guard**:
|
|
- `collect_changes_diff()` raises a fail-closed error if the diff between base and worktree is completely empty, preventing reviewers from hallucinating reviews over non-existent changes.
|
|
- Documents-only changes produce non-empty diffs and correctly pass through to review.
|
|
- **`run_loop.sh` & Freeze Mode**:
|
|
- `run_loop.sh` passes `--worktree`, `--base`, and `--review-only` directly to `lib_py.loop.runner`.
|
|
- The legacy bash loop path (`MAM_LOOP_PY=0`) explicitly rejects `--worktree` with an informative error message directing users to the Python runner, preventing silent misconfiguration.
|
|
- **Tests**: Tests `test_t2_1` through `test_t2_14` in `test_issue21_worktree_prereqs.py` verify all flag combinations, diff scoping, empty diff fail-closed behavior, and review promotion.
|
|
|
|
### (3) Control Plane Binding & Persistence (`ee1d896`, `af8a54c`, `13259ed`)
|
|
- **Binding Schema**: `lib_py/control_plane.py` defines clean dataclass and serialization helpers for control plane bindings:
|
|
- `control_workspace_root`
|
|
- `control_agent_sessions_yaml`
|
|
- `control_mam_jobs_dir`
|
|
- `control_delegate_job_logs_dir`
|
|
- **Persistence & Forwarding**:
|
|
- `create_session.sh`, `resume_session.sh`, and `stop_session.sh` propagate and persist binding fields into `agent-sessions.yaml`.
|
|
- `status` and `reconcile` commands respect `MAM_JOBS_DIR` without corrupting state.
|
|
- **Reused Pane vs New Pane Contract (h25 & af8a54c)**:
|
|
- Reused panes do not receive environment variables as CLI arguments (`af8a54c`), avoiding argument pollution.
|
|
- Per `13259ed` (h25 contract update): when env flags are present during `new-session`, the idle pane wait is skipped and environment variables are passed directly to the pane creation call.
|
|
- **Relative `MAM_JOBS_DIR`**: Relative paths are normalized against `control_workspace_root`.
|
|
- **Backwards Compatibility**: Rows in `agent-sessions.yaml` without binding fields fall back cleanly to default paths relative to the current workspace root.
|
|
- **Tests**: Tests `test_t3_1` through `test_t3_13` in `test_issue21_worktree_prereqs.py` pass without regression.
|
|
|
|
### (4) `lib.sh` & Herdr Shim Contract (`4a81d15`, `13259ed`)
|
|
- **Scope of Changes**: Changes in `lib.sh` (29 lines) are narrowly focused on the `new-session` path to forward environment flags to `herdr split-window` / `new-session`.
|
|
- **Side Effects**: Critical primitives `_sks_*` and `send_keys_safe` are completely untouched.
|
|
- **Contract Integrity**: `tests/test_herdr_shim_contract.py` (17 tests) passed 100% in 21.51s, including `test_h25_new_session_with_env_flags_skips_the_idle_pane_and_uses_env_on_split`. The contract is maintained and strengthened, not weakened.
|
|
|
|
### (5) Design Document Corrections (`b61c8b1`)
|
|
- **Review Feedback Incorporation**: `docs/design/worktree-creator-model.md` was updated in commit `b61c8b1` to address all points raised in prior review rounds:
|
|
- Replaced overgeneralized claims with explicit Phase 1 boundaries (review-only worktree workflow).
|
|
- Formalized the 3 distinct filesystem roots: (1) Control Workspace Root, (2) Target Workspace Root, and (3) Worktree Root.
|
|
- Explicitly defined reviewer session identity preserving `herdr` pane mappings.
|
|
- Added compatibility table between control plane modes and agent roles.
|
|
|
|
### (6) New Test Suite Quality & Hermeticity (`test_issue21_worktree_prereqs.py`)
|
|
- **Size & Coverage**: 780 lines, 35 unit and integration tests covering wrapper paths, runner worktree flags, control plane bindings, and freeze-mode guards.
|
|
- **Hermetic Isolation**:
|
|
- Uses `mam_sandbox` fixtures and `tmp_path`.
|
|
- Never writes to the actual `.mam` directory or real session registries.
|
|
- UUIDs generated via `uuid.uuid4()`.
|
|
- Zero external MQTT broker dependency (no network flakiness).
|
|
- **Execution**: 35 passed in 145.14s on the target worktree.
|
|
|
|
### (7) Regression & Test Suite Verification
|
|
Targeted test execution in worktree `/Users/godopu16/PuKi/laa/canary_projects/multi-agent-mux-wt/pr-22`:
|
|
| Test Suite | Result | Time |
|
|
| :--- | :---: | :---: |
|
|
| `tests/test_issue21_worktree_prereqs.py` | **35 / 35 PASS** | 145.14s |
|
|
| `tests/test_herdr_shim_contract.py` | **17 / 17 PASS** | 21.51s |
|
|
| `tests/test_issue5_phase2a_loop.py` | **91 / 91 PASS** | 2.75s |
|
|
| `tests/test_version_consistency.py` | **3 / 3 PASS** | 0.87s |
|
|
| `tests/test_issue6_turn_handshake.py` | **14 / 14 PASS** | 56.44s |
|
|
|
|
- **Verification of Author's "33 Failures" Claim**:
|
|
- Investigated test failures in suites outside PR #22's diff (e.g. `test_issue18_liveness_contract.py::test_acceptance5` and `test_issue14_delivery_ack.py::test_wrapper_idle_grace_never_outlives_the_wall_clock`).
|
|
- Analysis confirmed:
|
|
- `test_acceptance5` failure was caused by collisions on the public shared MQTT broker (`python/mqtt/jobs/+/dispatch`) receiving dispatches from concurrent runs on other machines (`pu-macbook-pro.local`).
|
|
- `test_issue14` timing test failure was a 56ms jitter (28.05s vs 28.00s threshold) under multi-process CPU contention with `slow-python sleep 0.7`.
|
|
- Neither test was modified in PR #22 (zero diff against `5611859`).
|
|
- All core subsystem and prerequisite tests for PR #22 pass 100%. No regressions introduced.
|
|
|
|
### (8) Issue #21 Scope Adherence & Deferrals
|
|
- **Phase 1 Complete**:
|
|
- Item 1: Wrapper absolute paths (Complete, verified).
|
|
- Item 2: Loop runner worktree options & diff scoping (Complete, verified).
|
|
- Item 3: Control plane binding propagation & persistence (Complete, verified).
|
|
- Item 6: Dedicated test suite (Complete, verified).
|
|
- **Properly Deferred to Phase 2**:
|
|
- Item 4: Multi-loop worktree contention and queuing.
|
|
- Item 5: Reviewer test artifact isolation / sandbox.
|
|
- Item 7: Worktree deletion and cleanup safety script.
|
|
- Item 8: Autonomous end-to-end worktree creator lifecycle.
|
|
- **Follow-up Recommendations**: The 5 follow-up recommendations (Phase 2 worktree lifecycle script, isolated scratch spaces for reviewer test runs, worktree removal safety validation, loop lock file, telemetry) are appropriate and correctly planned for subsequent PRs.
|
|
|
|
### (9) Merge Readiness Assessment
|
|
- **Blocker Scan**: No unhandled edge cases, no security vulnerabilities, no broken imports, no regressions.
|
|
- **Known Limitations Acknowledged**:
|
|
1. Version bump to 4.6.0 + `VERSIONS.md` entry (orchestrator release commit).
|
|
2. Trailing markdown whitespace in review notes (orchestrator release commit).
|
|
|
|
---
|
|
|
|
## 3. Final Conclusion
|
|
|
|
PR #22 successfully delivers the Phase 1 prerequisites for the Worktree Creator Operating Model with high code quality, robust test isolation, and strict backwards compatibility.
|
|
|
|
[VERDICT: PASS]
|