---
phase: 25.2-doyle-cluster-fix-candidates
reviewed: 2026-05-22T00:00:00Z
depth: standard
files_reviewed: 11
files_reviewed_list:
  - src/common/tracked.rs
  - src/common/wrapper_state.rs
  - src/live/start.rs
  - src/live/signoff.rs
  - src/live/wrapper/mod.rs
  - src/live/wrapper/lifecycle.rs
  - src/live/wrapper/claude.rs
  - src/live/wrapper/echo_fire.rs
  - src/live/wrapper/orphan.rs
  - tests/native_tracked_stale_lock.rs
  - tests/native_wrapper_state_retry.rs
  - tests/native_wrapper_path_resolver.rs
  - tests/native_latent_signoff_deliver_then_die.rs
findings:
  blocker: 0
  warning: 4
  info: 3
  total: 7
dirty: false
status: findings
---

# Phase 25.2: Code Review Report

**Reviewed:** 2026-05-22
**Depth:** standard (focused per review scope)
**Files Reviewed:** 13 (9 production + 4 test)
**Status:** findings (advisory — no blockers)

## Summary

Phase 25.2 ships five fix candidates against the doyle listener-poll regression cluster. Implementation hews tight to the LOCKED CONTEXT decisions (D-01..D-12) and the soft-fail / structured-warning posture in PATTERNS S-1. Verification artifact's claims about line numbers, helper signatures, predicate disjointness, and test coverage all check out under direct file inspection.

Review focus areas held up well:
- **`catch_unwind` placement** in `drain_stale_signoff_file` is correct — wraps only `deliver_body_anonymous`; file-delete is gated on the `.is_ok()` boolean and runs OUTSIDE the closure.
- **Ordering** (deliver → delete) matches D-10 verbatim; panic path preserves file with a "(continuing)" WARNING.
- **Stale-lock probe race** is correctly defended via the `len()==0 AND mtime>60s` conjunction — a live `git commit` either holds a non-zero lock or holds a sub-60s lock; either condition trips the guard.
- **`wrapper_state_path_resolved` fall-through** semantics are clean: nested-first via `file.exists()`, flat-fallback unconditional. Tests E/F/G pin all three branches.
- **D-07 exhaustion** correctly returns `None` from the helper, with caller-side `match` arms that swallow and continue.
- **Test isolation** (`SptHomeSnapshot` + `ENV_LOCK`) is applied consistently across all four new `native_*` tests; mirrors the in-module pattern from `src/common/wrapper_state.rs::tests`.
- **Best-effort cleanup idiom** is followed everywhere — single `eprintln!` ending with `(continuing)` / `(next attempt)`; no panic, no `?` propagation.
- **D-11 envelope escape**: `event_attr_escape` covers `& < > "`; `event_body_escape` adds `\n -> <br>`. No injection vector for `</EVENT>` breakout via signoff body content.

Findings below are quality-tier (no blockers). The most significant is WR-01 — the deliver-then-die ordering is correct against PANICS but `deliver_body_anonymous` silently swallows spool-write errors via `let _ = spool::spool_message(...)`, so a disk-full or SQLite-corruption failure on the spool path will still delete the signoff file. This is documented as accepted risk in Plan 02 SUMMARY ("Open Question 1 RESOLVED — spool itself is the durable destination"), but the production status line still reports `LATENT-SIGNOFF-FORWARDED` even when the spool write silently failed. Surfacing this as a quality concern, not a contract regression.

## Warnings

### WR-01: `delivered` boolean reflects only panic-safety, not spool-write success

**File:** `src/live/start.rs:206-209`
**Issue:** `delivered = catch_unwind(...).is_ok()` returns `true` whenever the closure completes without unwinding. But `send::deliver_body_anonymous` → `deliver_message` → `let _ = spool::spool_message(...)` silently discards any `Err` from the spool write (`src/owl/send.rs:96`). So a failed spool insert (disk full, SQLite IO error, lock contention, permission denied on the perch directory) leaves the signoff file deleted AND nothing queued. The `LATENT-SIGNOFF-FORWARDED:{id} (body queued to {psyche_id} via TCP/spool)` status line at L213-219 fires unconditionally before the gate, lying about a queue that may not exist.

Plan 02 SUMMARY documents this as "accepted per RESEARCH Open Question 1 RESOLVED" — but the decision rests on "spool fallback ALWAYS reaches", which is only true if the spool write itself succeeds. The DTD contract from D-10 ("never delete before the body is durably queued") is materially weakened.

**Fix:** Either (a) thread a `Result` through `deliver_body_anonymous` so the caller can distinguish "queued" from "spool-write-failed", and gate the delete on `Ok`; OR (b) at minimum, move the `LATENT-SIGNOFF-FORWARDED` status line into the `if delivered` arm so a panic doesn't double-emit a success line + an error line. Suggested minimal change:

```rust
// Move success status into the delivered arm:
if delivered {
    output::live_status(
        output::S_READY,
        &format!(
            "LATENT-SIGNOFF-FORWARDED:{} (body queued to {} via TCP/spool)",
            id, psyche_id
        ),
    );
    if let Err(e) = fs::remove_file(&signoff_path) { /* ... */ }
} else {
    output::owl_err(&format!(
        "drain_stale_signoff_file: forward panicked for {}; \
         preserving signoff file at {} for next attempt",
        id, owlery::to_forward_slash(&signoff_path),
    ));
}
```

For (a), surface a `Result<(), DeliverError>` from `deliver_message` and propagate up through `deliver_body_anonymous`. Would catch the disk-full case the current contract papers over.

### WR-02: Stale-nested `wrapper-state.json` can shadow a live flat write

**File:** `src/common/wrapper_state.rs:142-150`
**Issue:** `wrapper_state_path_resolved` returns the nested path whenever `nested.exists()`. Per the LOCKED decision, all CURRENT production writers go flat (claude.rs:181, lifecycle.rs:83). So in normal operation the nested branch only fires for files written by gen-NEW wrappers that don't yet exist. Risk vector: if at any point an ancestor wrapper, a test fixture, an external tool, or a Phase 26+ writer writes the nested file once and abandons it without unlinking, every subsequent reader will pick up the STALE nested copy and ignore the live flat re-publish from the current wrapper. The reader does not compare mtimes between nested and flat.

This isn't exercised by any current writer in the audited tree, but the resolver's "first-exists wins" semantics make it a latent landmine for the next phase that does start writing nested. CONTEXT D-04 acknowledges this asymmetry ("Plan 1 must resolve this asymmetry — either flip readers to nested with flat fallback OR force writers back to flat"). The chosen direction (readers-flexible, writers-flat) is correct for migration, but the resolver should prefer the FRESHER file once both exist.

**Fix:** When both `nested` and `flat` exist, compare mtimes and return the fresher path. Soft-fail to nested on mtime read error to preserve the current behavior. Approx:

```rust
pub fn wrapper_state_path_resolved(self_id: &str, psyche_id: &str) -> PathBuf {
    let nested = owlery::nested_perch_dir(self_id, psyche_id).join("wrapper-state.json");
    let flat = owlery::perch_dir(psyche_id).join("wrapper-state.json");
    match (nested.exists(), flat.exists()) {
        (true, true) => {
            // Prefer fresher mtime; tie-break to nested.
            let nm = std::fs::metadata(&nested).and_then(|m| m.modified()).ok();
            let fm = std::fs::metadata(&flat).and_then(|m| m.modified()).ok();
            match (nm, fm) {
                (Some(n), Some(f)) if f > n => flat,
                _ => nested,
            }
        }
        (true, false) => nested,
        _ => flat,
    }
}
```

Alternatively, document the "stale-nested wins" risk inline as an explicit invariant the caller must enforce by unlinking nested before switching back to flat. The current doc comment ("Returns the FIRST path whose file exists") implies but doesn't make the staleness risk explicit.

### WR-03: Test name `drain_stale_signoff_file_preserves_file_when_no_body` contradicts its assertion

**File:** `tests/native_latent_signoff_deliver_then_die.rs:227, 240-244`
**Issue:** The test is named `..._preserves_file_when_no_body` but asserts `!sp.exists()` ("file deleted"). This matches the production behavior (Step 3 in `drain_stale_signoff_file`: empty-body → delete) and the Plan 02 SUMMARY description ("empty file deleted, no spool row created"), but the test name reads as the opposite contract. Future readers diffing failing-test names against the implementation will be misled.

**Fix:** Rename to `drain_stale_signoff_file_deletes_empty_signoff` (or `..._deletes_file_when_empty`). One-line rename, no logic change:

```rust
#[test]
fn drain_stale_signoff_file_deletes_empty_signoff() {
    // ...
}
```

### WR-04: `LATENT-SIGNOFF-FORWARDED` status line emits before delivery is gated

**File:** `src/live/start.rs:213-219`
**Issue:** Step 7 (`live_status` print) runs UNCONDITIONALLY between Step 6 (`catch_unwind` delivery attempt) and Step 8 (delete-on-success). So on the panic path, the operator sees:
```
[live] READY: LATENT-SIGNOFF-FORWARDED:doyle (body queued to doyle-psyche via TCP/spool)
[owl ERR] drain_stale_signoff_file: forward panicked for doyle; preserving signoff file at ... for next attempt
```
Confusing — first line says "forwarded", second says "panicked / preserved for next attempt". The first line should not fire on the panic path.

**Fix:** Move the `output::live_status(...)` call inside the `if delivered { ... }` arm. See WR-01 for the combined patch.

## Info

### IN-01: `next_generation` ignores its own intermediate binding

**File:** `src/live/start.rs:100-104`
**Issue:** Not introduced by this phase, but exposed by the surrounding reads:
```rust
fn next_generation(self_id: &str) -> u32 {
    let prev = super::context::read_status(self_id);
    let new_gen = prev.map(|s| s.generation).unwrap_or(0) + 1;
    new_gen
}
```
The `let new_gen` binding is purely cosmetic. `clippy::let_and_return`. Cosmetic only; not a phase-25.2 concern but visible during review.
**Fix:** Return the expression directly. Skip if outside review scope.

### IN-02: `next_generation` reads-without-writing risks gen drift across concurrent boots

**File:** `src/live/start.rs:100-104`
**Issue:** The function reads `PsycheStatus` and returns `prev.generation + 1`, but the caller (`start::run` at L420-432) is responsible for writing the new status back. Between the read here and the write in `write_status`, a second concurrent `$LIVE start` for the same id would also compute `prev+1` and write the same generation. Boundary case under the collision-check at L329-345 — collision check happens before `next_generation` and exits early on live pid, so the concurrent case requires the wrapper PID file to be gone or the wrapper to be dead. Low real-world likelihood; not introduced by Phase 25.2.
**Fix:** Out of scope for 25.2; flag for a future phase if observed.

### IN-03: `ghost cleanup` runs on every `migrate_legacy_if_needed` invocation

**File:** `src/common/tracked.rs:1524-1545`
**Issue:** The ghost-cleanup block runs unconditionally inside the migration entry point, gated only by `ghost.exists()`. After the first successful removal, every subsequent call is one extra `stat()` syscall. Comment cites RESEARCH §"#2 Idempotency — no sentinel needed; sentinels add a partial-state hazard on Windows file-locking with no cost saving" — design decision is documented and defensible. Not a bug; flagged so the choice (no sentinel) is visible in the review record.
**Fix:** None. Documenting the deliberate trade-off.

## Notes on Items Flagged for Review but Not Found Defective

- **`catch_unwind` placement (`src/live/start.rs:206-209`)** — Correctly scoped to the `deliver_body_anonymous` call only. File-delete runs outside the closure. `move ||` captures `psyche_id_for_send`/`envelope_for_send` clones so the closure is `UnwindSafe`. No issue.
- **Stale-lock probe race vs concurrent git** — `meta.len() == 0 && mtime > 60s` conjunction correctly preserves any live commit (which holds either non-zero contents or a sub-60s timestamp). A racing `git commit` that creates a fresh lock immediately AFTER the probe removes a stale one acquires the lock with `O_CREAT|O_EXCL` cleanly; race window is benign.
- **D-12 envelope-disjointness** — `latent signoff` (space) vs `init_signoff` (underscore) — substring search is correctly disjoint. Both the in-module unit test at `mod.rs:2904` and the integration-test `drain_stale_signoff_file_no_stop_loop_regression` at L269 pin the contract. `debug_assert!` at start.rs:194-197 adds a third defense line. Triple-guard appropriate.
- **D-07 exhaustion** — `read_wrapper_state_with_retry_with_budget` correctly returns `None`; `start.rs::emit_boot_trigger_after_spawn` and `signoff.rs::emit_signoff_trigger` both `match` on this and the `None` arm has the inline comment `helper already emitted the exhaustion WARNING`. No double-warning, no abort.
- **Test isolation** — All four new `native_*` test files declare `static ENV_LOCK: Mutex<()> = Mutex::new(());` + `struct SptHomeSnapshot` + acquire pattern `let _g = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner());`. Consistent with `src/common/wrapper_state.rs::tests`. `unwrap_or_else(|e| e.into_inner())` correctly recovers from poisoned locks (other test failure won't cascade-poison subsequent tests).
- **`event_attr_escape` / `event_body_escape`** — Cover `& < > "`; body adds `\n -> <br>`. Defends against `</EVENT>` injection, attribute breakout, and HTML-shape confusion in the signoff body. Order of substitution puts `&` first (correct — avoids double-escaping `&lt;` into `&amp;lt;`). No injection vector identified.

---

## REVIEW COMPLETE — status: findings

_Reviewed: 2026-05-22_
_Reviewer: Claude (gsd-code-reviewer)_
_Depth: standard (review-scope-targeted)_
_Note: Advisory only per phase mandate. No findings block phase advancement._