GodopuandClaude Fable 5.1
22586d2a01
Issue #34: the loop waits for the creator job's sub-jobs, reviewers get the sub-job table as evidence, a run with sub-jobs voids a round whose source changed during review, and a pane caller reaches an agent of another workspace on its own server. VERSIONS.md carries the entry; the merge-side reports (round 1 @ed293dc and round 2 @e80186f with identity sidecars, baseed293dc-> heade80186f, evidence review-tree) are archived under .agents/reports/<session>/. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
84 lines
6.1 KiB
Markdown
84 lines
6.1 KiB
Markdown
# Review Report: PR #38 Delta Review Round 2 (fix(#34) @ e80186f)
|
||
|
||
- **Job ID**: `f17a85df`
|
||
- **Reviewer**: Antigravity (`agy`, session: `herdr:reviewer-agy-01`, Role: `Reviewer`)
|
||
- **PR Target**: #38 (`fix/issue-34` @ `e80186f`, previous review target `ed293dc`, base `main` `7a4a0c2` / v4.7.3)
|
||
- **Review Identity & Tree**:
|
||
- `loop_id`: `loop-20260925T072328Z-2f5853`
|
||
- `base`: `ed293dcef4c346e0050a31cb46812bcdcf1d67cf`
|
||
- `head`: `e80186f56064eaf93a2f32c48d31d84816894774`
|
||
- `tree`: `3501868872d828894f031911b8ca31b01ce77a72`
|
||
- **Verification Worktree**: `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20260925T072328Z-2f5853/1`
|
||
- **Verdict**: **PASS**
|
||
|
||
---
|
||
|
||
## 1. Executive Summary & Verification Context
|
||
|
||
This is the Round 2 Delta Review of PR #38, evaluating the two commits (`8a9101e` and `e80186f`) added on top of `ed293dc` to resolve the review findings from Round 1:
|
||
1. **N1/R1 (grok / codex, blocking)**: `registry.get_feedback` previously only parsed `completed` and `error` events, discarding `cancelled` event details. Sub-jobs cancelled prior to injection (e.g., `target_not_found` or `session_busy`) surfaced in the review table as bare uninformative `cancelled` rather than `NOT-DISPATCHED`.
|
||
2. **R2 (codex, blocking)**: Remote runner deadlines publish `error --detail "deadline_timeout (remote runner)"`. Exact-string matching in `_subjob_state` failed to identify this as an orchestrator timeout, missing `CLOSED-UNCONFIRMED` categorization and late-write warnings.
|
||
3. **K1 (claude, non-blocking)**: The `[귀속되지 않은 열린 잡]` section displayed orphan jobs without explicit adjudication instructions.
|
||
|
||
### Verification Identity & Git Cleanliness
|
||
- `git rev-parse HEAD` = `e80186f56064eaf93a2f32c48d31d84816894774`
|
||
- `git rev-parse 'HEAD^{tree}'` = `3501868872d828894f031911b8ca31b01ce77a72`
|
||
- `git diff ed293dc...HEAD --check` = Clean (0 issues)
|
||
- `git diff 7a4a0c2...HEAD --check` = Clean (0 issues)
|
||
- Worktree Status: Clean
|
||
|
||
---
|
||
|
||
## 2. Detailed Verification of Round 1 Corrective Changes
|
||
|
||
### (1) N1/R1 Fix: `get_feedback` Event Filtering & `cancelled` Detail Capture
|
||
- **Files Modified**:
|
||
- `lib_py/delegation/registry.py`: `get_feedback(job_id, registry_dir=..., events=("completed", "error"))`
|
||
- `lib_py/loop/runner.py`: `_subjob_state()` passes `events=("completed", "error", "cancelled")`
|
||
- **Audit Findings**:
|
||
- **Preserved Default Semantics**: `events` parameter defaults to `("completed", "error")`. Existing callers (such as `registry.py` CLI `feedback` / line 669) that pass only `job_id` and `registry_dir` remain 100% backward-compatible.
|
||
- **Dual-Path Support**: Both the primary unified ndjson audit log path (`e.get("source_event") in events` / `e.get("event") in events`) and the fallback local `.events.log` path (`payload.get("event") in events`) correctly check against `events`.
|
||
- **Regression Test T13**: Added `test_t13_a_sub_job_cancelled_before_injection_is_not_dispatched_in_the_table` parametrized over `["target_not_found", "session_busy"]`. Both cases pass and confirm classification into `| NOT-DISPATCHED | <reason> |`. Without the fix, `reason` is empty and the table displays `cancelled | -`, failing the assertion.
|
||
|
||
### (2) R2 Fix: Prefix Matching for Orchestrator Timeout Strings
|
||
- **File Modified**:
|
||
- `lib_py/loop/runner.py`:
|
||
```python
|
||
closed_kind = reason.split(" ", 1)[0] if reason else ""
|
||
if closed_kind in ORCHESTRATOR_CLOSED or (status == "error" and not reason):
|
||
return "CLOSED-UNCONFIRMED", reason or "-"
|
||
```
|
||
- **Audit Findings**:
|
||
- **Accurate Prefix Extraction**: `reason.split(" ", 1)[0]` extracts the initial keyword (e.g., `"deadline_timeout"` from `"deadline_timeout (remote runner)"`).
|
||
- **No Misclassification**: The candidate set `ORCHESTRATOR_CLOSED` strictly equals `{"deadline_timeout", "agent_blocked"}`. Other failure or cancellation reasons (such as `session_busy`, `target_not_found`, or `rolled back...`) have prefixes not in this set and therefore are never misclassified as `CLOSED-UNCONFIRMED`.
|
||
- **Regression Test T14**: Added `test_t14_the_remote_runners_deadline_wording_is_closed_unconfirmed` using the real registry and event publisher. Verifies `CLOSED-UNCONFIRMED` state and `미확인 종결 1` warning in the prompt. Without `closed_kind`, this test fails.
|
||
|
||
### (3) K1 Fix: `STRAY_RULE` Guidance for Unparented Open Jobs
|
||
- **File Modified**:
|
||
- `lib_py/loop/runner.py`: Added `STRAY_RULE` constant and conditional injection into `stray_line` if and only if `stray` is non-empty.
|
||
- **Audit Findings**:
|
||
- **Rule Coherence**: `SUBJOB_RULE` instructs reviewers to fail the round if any known sub-job is open, unconfirmed, or failed. `STRAY_RULE` instructs reviewers to fail the round if any unparented open job's deliverables were necessary for the task objectives. The two rules operate harmoniously and without contradiction.
|
||
- **Prompt Hygiene**: When neither registered sub-jobs nor stray jobs exist, `_subjobs_block()` continues to return `""`, ensuring byte-for-byte prompt parity for non-delegating tasks.
|
||
- **Regression Test T6 Assertion**: `test_t6_an_unattributed_open_job_is_shown_and_waited_for_only_on_request` now asserts that `STRAY_RULE` is included in the reviewer prompt.
|
||
|
||
---
|
||
|
||
## 3. Test Suite & Regression Verification
|
||
|
||
All tests were executed against the verification worktree (`loop-20260925T072328Z-2f5853/1`):
|
||
1. **`tests/test_issue34_job_tree.py`**:
|
||
- **20 / 20 PASSED** (including T13 parametrized × 2, T14, and updated T6) in 84.48s.
|
||
2. **`tests/test_version_consistency.py` & `tests/test_issue5_phase1_delegation.py`**:
|
||
- **69 / 69 PASSED** in 12.07s.
|
||
3. **`tests/test_issue23_24_startup_path.py` & `tests/test_loop_cli.py`**:
|
||
- **79 / 79 PASSED** in 195.92s.
|
||
4. **Overall Total**: **168 tests passed, 0 failures, 0 regressions**.
|
||
|
||
---
|
||
|
||
## 4. Conclusion
|
||
|
||
All Round 1 reviewer findings (N1, R1, R2, K1) have been accurately addressed with clean, minimal code changes and rigorous regression test coverage. No edge-case misclassifications, prompt pollution, or regressions were detected. Version lockstep bump to v4.7.4 is properly reserved for the orchestrator release commit.
|
||
|
||
[VERDICT: PASS]
|