release(v4.8.7): Issue #17 follow-ups - archive the review reports and bump the lockstep version
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
This commit is contained in:
GodopuandClaude Opus 5.5 committed
1 parent
65b78b6ec4
commit
cd82ab6e72
28 files changed
+594
-25
No files matched your search
@@ -0,0 +1,14 @@
|
||||
{
|
||||
"loop_id": "loop-20261007T224638Z-cf1efb",
|
||||
"base_sha": "56d0bfda54e4f4fe1ccc138cc262edfb14cfb4a3",
|
||||
"head_sha": "65b78b6ec44e44904afe34915563522d4631767f",
|
||||
"tree_sha": "b805d9f9e6639c7359115ea9336eac95d7ab85bc",
|
||||
"review_commit": "65b78b6ec44e44904afe34915563522d4631767f",
|
||||
"reviewer": "reviewer-agy-01",
|
||||
"job_id": "5a0c16dd",
|
||||
"verdict": "PASS",
|
||||
"transport": "herdr",
|
||||
"evidence": "review-tree",
|
||||
"isolation": "worktree",
|
||||
"verified_at": "2026-10-08T01:16:15Z"
|
||||
}
|
||||
@@ -0,0 +1,93 @@
|
||||
# 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]
|
||||
@@ -0,0 +1,118 @@
|
||||
# PR Review Report: Issue #17 Followups (PR #51 Post-Merge Fixes)
|
||||
|
||||
- **Reviewer Agent**: `agy` (session: `herdr:reviewer-agy-01`)
|
||||
- **Job ID**: `67a528ff`
|
||||
- **Verification Directory**: `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261007T224638Z-cf1efb/1`
|
||||
- **Base Commit**: `56d0bfda54e4f4fe1ccc138cc262edfb14cfb4a3` (main)
|
||||
- **Head Commit**: `8241bd53b179cf8332541264c3bf12b8e3eca44a` (fix/issue17-followups)
|
||||
- **Cumulative Commits**: 6 commits (`25c9970` ~ `8241bd5`)
|
||||
|
||||
---
|
||||
|
||||
## 1. Executive Summary
|
||||
|
||||
This review covers the Issue #17 follow-up work bundle addressing:
|
||||
1. `herdr 0.9.3` compatibility for removed pane directories (`test_issue45_restore.py`, `lib_py/herdr_restore.py`, `test_issue45_restore_unit.py`).
|
||||
2. N2: Fail-closed error handling for failed `_decide` calls (preventing `exit ""` -> rc 255 on bash 3.2).
|
||||
3. N1: Docstring clarification in `wrapper.py` regarding package-level paho import.
|
||||
4. N3: Comment update in `lib_int.sh` documenting `python3` PATH dependency.
|
||||
|
||||
While items 1, 3, and 4 are well-implemented and the new unit tests pass cleanly, **item 2 (commit `a670ef5`) introduces regressions in existing test suites** (`test_tier1_unit.py` and `test_issue5_phase1_delegation.py`). The commit replaced raw `_decide` subshell invocations with helper functions `_decided` and `_exit_for` in `multi-agent-mux-delegate-job`, but failed to update the frozen literal assertions in the existing test suite, causing 2 test failures.
|
||||
|
||||
Therefore, the verdict for this round is **NOT PASS**.
|
||||
|
||||
---
|
||||
|
||||
## 2. Evaluation of Task Requirements
|
||||
|
||||
| Item | Description | Implementation Status | Assessment |
|
||||
|---|---|---|---|
|
||||
| **1. herdr 0.9.3 Compatibility** | `test_b_only_the_resumed_sessions_restored_shell_is_closed` failed on herdr 0.9.3 because removed pane directories retain their old path rather than resetting to `$HOME`. `lib_py/herdr_restore.py` must close placeholders with `restore_error` and `pane_not_found`. | `tests/test_issue45_restore.py:275`, `.agents/skills/lib_py/herdr_restore.py:130-138`, `tests/test_issue45_restore_unit.py:424-463` | **PASS**: Correctly handles both herdr 0.9.1 and 0.9.3 behavior; unit tests and full restore test pass without touching live sessions. |
|
||||
| **2. N2 Fail-Closed on `_decide` Failure** | When `_decide` fails, `exit "$(_decide exit-code ...)"` evaluated to `exit ""` (rc 255 in bash 3.2). The wrapper must exit 1 (fail-closed) on failure, never 255, 0, or empty string. | `multi-agent-mux-delegate-job:100-128`, `tests/test_issue17_item4_decisions.py:309-382` | **NOT PASS (Test Regression)**: Logic in `multi-agent-mux-delegate-job` is sound, but broke existing freeze tests in `test_tier1_unit.py` and `test_issue5_phase1_delegation.py`. |
|
||||
| **3. N1 Docstring Clarification** | `wrapper.py` docstring claimed "Stdlib only", but running via `-m lib_py.delegation.wrapper` loads `lib_py/delegation/__init__.py` and transitively requires `paho`. | `lib_py/delegation/wrapper.py:21-29` | **PASS**: Accurate docstring explanation added. |
|
||||
| **4. N3 `lib_int.sh` PATH Comment** | Document that `lib_int.sh` uses `python3` from PATH. | `lib_int.sh:9-11` | **PASS**: Comment added cleanly without code churn. |
|
||||
|
||||
---
|
||||
|
||||
## 3. Test Failures & Detailed Findings
|
||||
|
||||
Running the regression suite in the review verification directory (`/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261007T224638Z-cf1efb/1`):
|
||||
```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-failed-inspect \
|
||||
tests/test_tier1_unit.py::test_g10_delegate_job_rc3_not_mistaken_for_error \
|
||||
tests/test_issue5_phase1_delegation.py::test_wrapper_exit_code_mapping_literals_are_frozen
|
||||
```
|
||||
|
||||
### Failure 1: `tests/test_tier1_unit.py:847`
|
||||
- **Test**: `test_g10_delegate_job_rc3_not_mistaken_for_error`
|
||||
- **Failure**:
|
||||
```python
|
||||
> assert '_decide sub-status "$sub_rc"' in delegate_script.read_text()
|
||||
E AssertionError: assert '_decide sub-status "$sub_rc"' in ...
|
||||
```
|
||||
- **Cause**: Commit `a670ef5` replaced line 405 (and line 574) of `multi-agent-mux-delegate-job`:
|
||||
```bash
|
||||
# Before (base 56d0bfd):
|
||||
job_status="$(_decide sub-status "$sub_rc" --job "$JOB_ID" --registry-dir "$REGISTRY_DIR")"
|
||||
|
||||
# After (commit a670ef5):
|
||||
_decided job_status sub-status "$sub_rc" --job "$JOB_ID" --registry-dir "$REGISTRY_DIR"
|
||||
```
|
||||
Because `_decide sub-status "$sub_rc"` no longer appears literally in `multi-agent-mux-delegate-job`, the assertion in `test_tier1_unit.py:847` fails.
|
||||
|
||||
### Failure 2: `tests/test_issue5_phase1_delegation.py:390`
|
||||
- **Test**: `test_wrapper_exit_code_mapping_literals_are_frozen`
|
||||
- **Failure**:
|
||||
```python
|
||||
> for literal in ('_decide sub-status "$sub_rc"', '_decide exit-code "$job_status"', "--sub-rc \"$sub_rc\""):
|
||||
> assert literal in text, literal
|
||||
E AssertionError: _decide sub-status "$sub_rc"
|
||||
```
|
||||
- **Cause**: Commit `a670ef5` replaced all occurrences of:
|
||||
- `_decide sub-status "$sub_rc"` with `_decided job_status sub-status "$sub_rc"`
|
||||
- `exit "$(_decide exit-code "$job_status")"` with `_exit_for "$job_status"`
|
||||
Neither literal is present in the wrapper script anymore, causing the freeze verification in `test_issue5_phase1_delegation.py:390` to fail.
|
||||
|
||||
---
|
||||
|
||||
## 4. Required Fixes for Next Round
|
||||
|
||||
In worktree `/Users/godopu16/PuKi/laa/canary_projects/multi-agent-mux-wt/issue17-followups`:
|
||||
|
||||
1. **Update `tests/test_tier1_unit.py:847`**:
|
||||
Change:
|
||||
```python
|
||||
assert '_decide sub-status "$sub_rc"' in delegate_script.read_text()
|
||||
```
|
||||
To match the new helper signature:
|
||||
```python
|
||||
assert '_decided job_status sub-status "$sub_rc"' in delegate_script.read_text()
|
||||
```
|
||||
|
||||
2. **Update `tests/test_issue5_phase1_delegation.py:390`**:
|
||||
Change:
|
||||
```python
|
||||
for literal in ('_decide sub-status "$sub_rc"', '_decide exit-code "$job_status"', "--sub-rc \"$sub_rc\""):
|
||||
assert literal in text, literal
|
||||
```
|
||||
To match the new helper signatures:
|
||||
```python
|
||||
for literal in ('_decided job_status sub-status "$sub_rc"', '_exit_for "$job_status"', "--sub-rc \"$sub_rc\""):
|
||||
assert literal in text, literal
|
||||
```
|
||||
|
||||
3. **Verify Full Test Suite**:
|
||||
Run:
|
||||
```bash
|
||||
AGENT_PYTHON_BIN=.venv/bin/python PYTHONDONTWRITEBYTECODE=1 .venv/bin/python -m pytest -p no:cacheprovider -q 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
|
||||
```
|
||||
Confirm all tests pass without errors.
|
||||
|
||||
---
|
||||
|
||||
## 5. Final Verdict
|
||||
|
||||
[VERDICT: NOT PASS]
|
||||
Reference in new issue
Block a user