Godopu
94f2e213d2
- Add Rev. 3 technical analysis report from creator-agy-01 - Add implementation plan from planner-reviewer-claude-01 - Add peer review reports across analysis and implementation review loops (Claude, Grok, OpenCode)
57 lines
7.2 KiB
Markdown
57 lines
7.2 KiB
Markdown
# 🔍 Cross-Review: Issue #3 Fix Implementation (Job 8bffb70b)
|
|
|
|
- **Reviewer**: `reviewer-opencode-01` (role: reviewer)
|
|
- **Target**: Working-tree implementation of the Issue #3 fixes per the Rev.3 analysis and planner plan (`plan-a9c8d6d3.md` Rev.2, incorporating the `c4b0a075` epoch challenge): (1) daemon process-group detachment in `lib.sh`, (2) Class A 0-turn resume fresh-spawn fallback in `resume_session.sh`/`update_yaml_resumed.sh`, (3) four new tests, plus the carried analysis/plan/review documents.
|
|
- **Method**: Every fix verified in live source and by my own runs — including a from-scratch empirical PGID-detachment experiment, fail-old/pass-new verification for each new test (by temporarily reverting the fixed files to HEAD), the full 455-test suite, and byte-level checks that Class B code paths are untouched.
|
|
|
|
---
|
|
|
|
## 0. My Verification Runs
|
|
|
|
| Check | Result |
|
|
|---|---|
|
|
| `bash -n` on all 3 modified scripts | OK |
|
|
| Targeted: `test_uuid_target.py` + `test_herdr_shim_contract.py` | **32 passed** |
|
|
| Full suite `pytest tests/ -q` | **455 passed / 0 failed** (730s, my own run) — 451 prior + 4 new (t14, t15, t16, h26) |
|
|
| Fail-old/pass-new, t14 | **Failed against HEAD** (old hard-fail) → passes with fix (verified by swapping files, not stash) |
|
|
| Fail-old/pass-new, t16 | **Failed against HEAD** (epoch not refreshed) → passes with fix |
|
|
| Class B guards: t15 + t8 | **Both pass** with fix in place |
|
|
| Empirical daemon detachment (my own experiment) | Spawned child **PGID 27542 ≠ shell PGID 27511** — genuine `setsid()` detachment, verified outside the test harness |
|
|
| `lib_py/` diff | **Zero** — `verify_session.py`/`workspace_uuid.py` untouched per the plan's constraint |
|
|
|
|
## 1. Item 1 — Daemon process-group detachment — CORRECT
|
|
|
|
- Live `lib.sh:212-226`: the `nohup ... & disown` pattern is gone, replaced by the `python3 -c '...Popen(..., start_new_session=True)...'` spawner, with a comment accurately explaining the PGID/SIGTERM mechanism and the macOS `setsid`-absence rationale — exactly the portable idiom the Rev.3 analysis recommended (and that I independently endorsed in `8c093047`).
|
|
- The `kill -0` liveness-wait loop is unchanged; only PID acquisition changed (stdout capture instead of `$!`) — minimal-diff discipline.
|
|
- `test_h26` does more than a text check: it extracts the *actual shipped* spawner statement from the generated shim and proves behavioral detachment (child PGID ≠ caller PGID). I additionally ran my own equivalent experiment outside the test harness — same result.
|
|
- Note: the currently-running production daemon (PID 7623) still shows the old un-detached state — expected: it was spawned before this change; the fix applies to the next bootstrap. Not a defect of the fix.
|
|
|
|
## 2. Item 2 — Class A fresh-spawn fallback — CORRECT, Class B provably untouched
|
|
|
|
- `resume_session.sh`: `FRESH_SPAWN` is set only for `agy|hermes|opencode`; the `*)` arm reproduces the original hard-exit **byte-identically** (message + RC=1). claude/grok cannot enter the fallback — verified in source, and `test_t15` locks the guard (a never-created claude session still fails exactly as before).
|
|
- `FRESH_SPAWN=1` path uses `spawn-spec` (the same call create uses for first launch) with an inline fallback matching the three Class A agents only — the recovery happens inside `resume_session.sh`, bypassing the broken `--role`-required handoff as planned.
|
|
- `update_yaml_resumed.sh`: `--uuid` requirement relaxed (documented in usage), `--cmd-full` passthrough added; the per-agent `agy`/`hermes`/`opencode` blocks gained `if uuid: ... else:` guards. **The `claude`/`grok` blocks are untouched** — the only diff line mentioning them is the usage string listing agent names. `verify_session.py:99-101` escape hatch and `test_t8` both verified passing — the Class B contract is preserved exactly as the brief demanded.
|
|
- **The epoch refinement (challenge `c4b0a075`) is the best part of this implementation**: the challenge (real job, registry-verified) found that a fresh-spawn resume would leave the row's discovery watermark at the original create timestamp, letting `reconcile.sh` pin a stale transcript to the still-silent resumed agent. The fix — gated strictly on `if not uuid:`, placed after the shared `status='running'` so it applies to both new-row and existing-row branches — resets `herdr_session_epoch`/`created_at` and re-stamps `pending-discovery`/`verified: false`. `test_t16` locks it with a live (non-dry-run) resume and asserts the epoch advanced. I verified fail-old/pass-new for t16 myself (fails at HEAD, passes with fix). The planner's Rev.2 even honestly notes the challenge's own precision slip (`session_id_source` was never set on Class A rows before — introducing it is forward-consistency, not a reset) — correct reading of live `create_session.sh`.
|
|
|
|
## 3. Fail-old/pass-new discipline — verified by me, not taken from the plan
|
|
|
|
My first attempt to verify this was itself flawed (`git stash push` with pathspec silently failed, so I tested the fix against itself). I redid it properly with `git checkout HEAD -- <files>`: **t14 and t16 both genuinely fail against the pre-fix code** and pass with it. This also validates the plan's own fail-old/pass-new claims.
|
|
|
|
## 4. Documentation carry-over integrity
|
|
|
|
- The analysis (Rev.3), plan (Rev.2), and the four review reports in the diff match their `.mam/jobs/` sources — the review chain (my `8c093047` PASS → `7b6c16df` NOT PASS → `d8c851a1` PASS, Claude's `0338e7de` NOT PASS → `9406c304` PASS, Grok's `1a4f5236` NOT PASS → `1da8fd5c` PASS-withdrawal) is carried intact, including the self-corrections.
|
|
- Worker job `a9c8d6d3` and challenge job `c4b0a075` are both real and registry-verified.
|
|
|
|
## 5. Residual notes (non-blocking)
|
|
|
|
- Plan §3 cites "454-passed baseline" for the original implementation run; my authoritative full-suite count for the current tree is **455** (451 prior + 4 new). The plan's targeted-count arithmetic (164 pre-existing + 3 new = 167) predates the t16 addition in Rev.2 — the final state is internally consistent (I ran it), just the intermediate numbers tell the revision story. No action needed.
|
|
- `test_h26`'s `ps -o pgid=` polling has a small race window if the spawned process dies within 0.3s; with `sleep 30` as the fake daemon this is stable. Noted for the record.
|
|
- The live PID-7623 daemon remains in the old state until the next server bootstrap — a natural follow-up is restarting the `multi-agent-mux` herdr session to put the fix into production; out of this job's scope.
|
|
|
|
---
|
|
|
|
## 6. Verdict
|
|
|
|
Both fixes are implemented exactly per the validated analysis and refined plan: daemon detachment is real (empirically proven at the process level, not just textually), the Class A fallback is correctly gated with Class B provably untouched (byte-level diff check + t15/t8 guards + zero `lib_py/` changes), the challenge-driven epoch refinement closes a genuine subtle bug the first implementation draft introduced, all four new tests pass with genuine fail-old/pass-new behavior (which I verified myself by reverting files), and the full suite passes 455/455 in my own run. The review chain is carried honestly. No lint, functionality, or data-loss issues; no design-level rework; no planner escalation.
|
|
|
|
[VERDICT: PASS] |