GodopuandClaude Opus 5.5
cd82ab6e72
Merge-workspace review loop (--all-reviewer): round 1 (@8241bd5) claude grok PASS, agy NOT PASS (two static wrapper-literal tests did not follow _decided/_exit_for), codex NOT PASS (R1: a yes/no decision that could not run read as "no"); fixed in541c3feand65b78b6. Round 2 (@65b78b6) 4/4 PASS. Full suite at65b78b6on macOS / Python 3.9 / herdr 0.9.3: 1730 passed / 0 failed / 56 skipped; 18 of the skips were the uninitialized nats-docker submodule in the fresh worktree, and with it tests/test_deploy_freshness.py is 31 passed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
94 lines
6.7 KiB
Markdown
94 lines
6.7 KiB
Markdown
# PR Review Report: Issue #17 Followups (Round 2 Review)
|
|
|
|
- **Reviewer Agent**: `agy` (session: `herdr:reviewer-agy-01`)
|
|
- **Job ID**: `5a0c16dd`
|
|
- **Verification Directory**: `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261007T224638Z-cf1efb/2`
|
|
- **Base Commit**: `56d0bfda54e4f4fe1ccc138cc262edfb14cfb4a3` (main)
|
|
- **Head Commit**: `65b78b6ec44e44904afe34915563522d4631767f` (fix/issue17-followups)
|
|
- **Cumulative Commits**: 8 commits (`25c9970` ~ `65b78b6`)
|
|
- **Delta Under Inspection**: `8241bd5`..`65b78b6` (commits `541c3fe` and `65b78b6`)
|
|
|
|
---
|
|
|
|
## 1. Executive Summary
|
|
|
|
In Round 1 (`67a528ff`), this reviewer returned `[VERDICT: NOT PASS]` due to test regressions in `tests/test_tier1_unit.py:847` and `tests/test_issue5_phase1_delegation.py:390` where frozen literal assertions still expected the pre-refactor `_decide sub-status` and `_decide exit-code` call signatures. Additionally, `codex` noted that boolean decisions (`subscriber-up` and `stall-watch`) that failed at runtime would mistakenly be treated as a negative decision (`"no"`) rather than triggering a fail-closed exit.
|
|
|
|
In this round, the Creator has completely resolved all issues across commits `541c3fe` and `65b78b6`:
|
|
1. Updated `test_tier1_unit.py` and `test_issue5_phase1_delegation.py` to freeze the new helper signatures (`_decided job_status sub-status` and `_exit_for`).
|
|
2. Implemented `--print` flag for boolean subcommands in `wrapper.py` and introduced `_decided_yes` in `multi-agent-mux-delegate-job`, ensuring failed yes/no decisions fail-closed with exit code 1.
|
|
3. Added comprehensive test coverage in `tests/test_issue17_item4_decisions.py` verifying that failed boolean decisions exit with 1.
|
|
|
|
All tests across affected and regression suites executed cleanly (**656 passed, 1 skipped in 1147.89s**). The changes are surgical, robust, and fully conform to all behavioral invariance contracts.
|
|
|
|
---
|
|
|
|
## 2. Resolution of Round 1 Findings Matrix
|
|
|
|
| Finding | Origin | Issue & Location | Resolution in Round 2 (`541c3fe`, `65b78b6`) | Status |
|
|
|---|---|---|---|---|
|
|
| **1. Frozen Literal Assertion in `test_tier1_unit.py`** | `agy` (Round 1) | `tests/test_tier1_unit.py:847` asserted `_decide sub-status "$sub_rc"`, which was replaced by `_decided` in `multi-agent-mux-delegate-job`. | Updated assertion to `_decided job_status sub-status "$sub_rc"` in commit `541c3fe`. | **CLOSED** (Verified) |
|
|
| **2. Frozen Literal Assertion in `test_issue5_phase1_delegation.py`** | `agy` (Round 1) | `tests/test_issue5_phase1_delegation.py:390` asserted literals `_decide sub-status "$sub_rc"` and `_decide exit-code "$job_status"`. | Updated assertions to `_decided job_status sub-status "$sub_rc"` and `_exit_for "$job_status"` in commit `541c3fe`. | **CLOSED** (Verified) |
|
|
| **3. Failed Boolean Decisions Passing as "No"** | `codex` (Round 1) | In bash, `_decide subscriber-up` or `_decide stall-watch` failing with rc > 0 evaluated as false (`"no"`), bypassing fail-closed error handling and potentially masking failures. | In commit `65b78b6`, added `--print` flag to `wrapper.py` (prints `"yes"`/`"no"` and exits 0) and added `_decided_yes` helper in bash wrapper to enforce fail-closed exit 1 on execution failure. | **CLOSED** (Verified) |
|
|
|
|
---
|
|
|
|
## 3. Evaluation of Core Task Requirements
|
|
|
|
### 3.1. herdr 0.9.3 Compatibility (Item 1)
|
|
- **`tests/test_issue45_restore.py:275`**: Accommodates both `herdr 0.9.1` (`$HOME` fallback) and `herdr 0.9.3` (retaining removed directory path with placeholder) via `assert _pane_with(rig, ids["Y"])["cwd"] in (str(rig.root), str(rig.root / "wsY"))`.
|
|
- **`lib_py/herdr_restore.py:_nothing_running`**: Handles placeholders restored by `herdr 0.9.3` where `pane process-info` errors with `pane_not_found` and `restore_error` is set, allowing resume to cleanly close dead placeholder panes.
|
|
- **`tests/test_issue45_restore_unit.py`**: Stubbed tests `test_a_placeholder_for_a_missing_directory_is_closed` and `test_any_other_process_info_failure_still_stops_the_run` verify that only authentic placeholders are closed, while unexpected errors continue to halt execution.
|
|
|
|
### 3.2. Fail-Closed on Decision Failure (Item 2 & N2)
|
|
- **`_decision_failed()`**: Emits explicit error diagnostic to stderr and terminates with `exit 1`.
|
|
- **`_decided()`**: Executes `_decide`, verifies non-zero exit and non-empty output, routing failures to `_decision_failed`.
|
|
- **`_decided_yes()`**: Executes `_decide ... --print`, strictly validating that stdout contains `"yes"` (returns 0) or `"no"` (returns 1). Any execution error or unexpected output triggers `_decision_failed`.
|
|
- **`_exit_for()`**: Validates that numeric exit code was returned, terminating with `exit "$code"`.
|
|
- **`finalize_unfinished` & `cmd_wait`**: Checked return codes on `finalize-end` and validated numeric formatting on `wait-rc`.
|
|
- **Coverage**: Verified via extensive unit and integration tests in `tests/test_issue17_item4_decisions.py` covering failure modes (`error`, `gone`, `empty`) across `exit-code`, `sub-status`, `finalize-begin`, `finalize-end`, `subscriber-up`, and `stall-watch`.
|
|
|
|
### 3.3. Documentation Updates (Items 3 & 4)
|
|
- **`lib_py/delegation/wrapper.py:21-29`**: Accurately clarifies that while `wrapper.py` itself imports standard library modules only, executing as a module (`-m lib_py.delegation.wrapper`) triggers package initialization loading `paho-mqtt`.
|
|
- **`lib_int.sh:9-11`**: Clearly documents that standalone sourcing executes `python3` from `PATH`.
|
|
|
|
---
|
|
|
|
## 4. Comprehensive Test Suite Execution Results
|
|
|
|
All tests were executed directly in the review verification environment (`/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261007T224638Z-cf1efb/2`):
|
|
|
|
```bash
|
|
AGENT_PYTHON_BIN=/Users/godopu16/PuKi/laa/canary_projects/multi-agent-mux-wt/issue17-followups/.venv/bin/python \
|
|
PYTHONDONTWRITEBYTECODE=1 \
|
|
/Users/godopu16/PuKi/laa/canary_projects/multi-agent-mux-wt/issue17-followups/.venv/bin/python \
|
|
-m pytest -p no:cacheprovider -q --basetemp=/tmp/pytest-issue17-r2 \
|
|
tests/test_tier1_unit.py \
|
|
tests/test_issue5_phase1_delegation.py \
|
|
tests/test_issue17_item4_decisions.py \
|
|
tests/test_issue45_restore.py \
|
|
tests/test_issue45_restore_unit.py \
|
|
tests/test_issue33_budget.py \
|
|
tests/test_issue26_ints.py \
|
|
tests/test_issue30_stall_watch.py \
|
|
tests/test_issue30_stall_watch_stop.py \
|
|
tests/test_issue18_liveness_contract.py \
|
|
tests/test_issue14_delivery_ack.py
|
|
```
|
|
|
|
### Results Summary
|
|
- **Total Tests**: 657
|
|
- **Passed**: 656
|
|
- **Skipped**: 1 (environment-specific non-root test)
|
|
- **Failed**: 0
|
|
- **Duration**: 1147.89s (0:19:07)
|
|
- **Exit Code**: `0`
|
|
|
|
---
|
|
|
|
## 5. Conclusion & Final Verdict
|
|
|
|
All blockers from Round 1 have been completely resolved. The codebase is clean, robust, thoroughly covered by deterministic tests, and preserves 100% behavioral invariance.
|
|
|
|
[VERDICT: PASS]
|