fix(claim-reaper): distinguish COMPLETED engineer from wedged to end re-archival churn (#4467, #4500, #4464)

[rysweet]

Problem

The stale-engineer claim-reaper classified a worktree purely by its
newest-file mtime, so a cleanly completed engineer — whose worktree
naturally stops writing files once its run ends — was indistinguishable from a
wedged one. It was labelled HeartbeatStale and driven into the
investigate-before-reap path every tick, re-archiving evidence forever
without ever converging.

Concrete evidence — goal advance-rysweet-agent-kgpacks-rs-to-full-parity-f29bb15c:

• 54 durable reap archives under ~/.simard/reaped-engineers/…-f29bb15c-*
  over ~3.7 days (up from the 10× recorded in claim-reaper HeartbeatStale investigation never converges: recipe verdict discarded (reconcile_inflight_investigations write-back gap) → Pending-only, reap path unreachable, unbounded re-archival #4467), each with a
  monotonically climbing idle_age and never reaped.
• The archived evidence (…-1785015794/evidence.txt) shows the engineer's own
  session record was {"session_id":"…","status":"completed","end_time":…} —
  a positively completed run, not a wedge (idle_age 20520s = exactly
  archived_ts − end_time).

This is the completed-vs-wedged diagnostic conflation of #4500 (closed but
demonstrably recurring — a regression), compounded by the
leaked-claim-on-completion path of #4464, and it is the concrete churn
tracked by #4467 ("unbounded re-archival").

Fix

WorktreeClaimLivenessProbe::assess now reads the engineer's terminal session
record (.claude/runtime/sessions.jsonl). When the last status is
completed, the claim is classified DeadReason::Completed — a positive
terminal signal, distinct from HeartbeatStale.

The reaper reclaims a Completed worktree directly — releasing the leaked
claim and removing the residual worktree exactly once, with no agentic
investigation — which terminates the unbounded re-archival churn. Safety is
preserved:

• The same idle threshold guards it, so a completion whose
  claim-release / cleanup is still in flight is never raced.
• A worktree a new session has re-entered (last status back to active)
  stays on the ordinary path.
• Fail-closed: any read/parse error, a missing file, or no status record ⇒
  not completed → the safe investigate-before-reap path is preserved. A
  completed verdict must be positively proven from the engineer's own record.

This layer is complementary to the existing in-flight fixes: it operates at the
liveness probe (before classification), distinct from the reconcile
write-back (#4489/#4467) and the perpetual-goal exemption (#4479/#4437).

Tests

• t3c_completed_worktree_is_reclaimed_without_investigation — a completed +
  long-idle worktree is reclaimed once and never investigated (the
  churn-breaker invariant).
• t3d_fresh_completed_worktree_is_not_reclaimed — completed but under the idle
  threshold ⇒ protected.
• probe_reports_completed_when_session_terminal_status_completed — probe
  returns Completed for the exact archived two-line active→completed shape.
• probe_not_completed_when_last_status_is_active — re-entered worktree stays
  HeartbeatStale.
• probe_not_completed_when_no_sessions_file — absent record fails closed.
• Label stability updated for the new variant.

All 41 overseer::claim_reaper tests pass; cargo fmt + cargo clippy --release -D warnings clean (commit + push gates green).

Refs #4467, #4500, #4464.

[github-actions]

📊 Coverage Summary

Generated by cargo llvm-cov --workspace --summary-only (nightly, excluding test files)

Module	Lines	Covered	Coverage
Total	198881	167454	84.2%

_{Coverage data from CI run. Test files matching tests?/ are excluded from line counts.}

[rysweet]

Step 17b — Comprehensive Code Review (PR #4712)

Scope reviewed: src/overseer/claim_reaper.rs (+254 / −9). Validation run locally on head 8fa7f62:

• cargo clippy --lib — clean, no new warnings
• cargo test --lib overseer::claim_reaper — 41 passed, 0 failed

Verdict: The implementation is correct, well-tested, and philosophy-aligned (ruthless simplicity, fail-closed, zero swallowed errors). No code defects found. Findings below are one traceability question and two minor/optional notes.

---

✅ Strengths

• Correct root-cause fix. Distinguishing a positively-completed engineer (DeadReason::Completed) from a HeartbeatStale one directly severs the completed-vs-wedged conflation that drove the 54× re-archival churn (#4467/#4500/#4464). A completed worktree naturally stops writing files, so mtime alone mislabels it stale — reading the engineer's own terminal session record is the right signal.
• Fail-closed parsing. engineer_session_completed returns false on missing file, read error, parse error, or no status record. A completed verdict must be positively proven — being wrong only in the false direction keeps the safe investigate-before-reap path. Verified by probe_not_completed_when_no_sessions_file.
• Re-entry handling. Uses the last status record, so a worktree a new session re-entered (status back to active) correctly stays HeartbeatStale. Verified by probe_not_completed_when_last_status_is_active.
• Threshold guard reused. The Completed arm applies the same age > stale_secs boundary as HeartbeatStale, so a just-finished worktree whose claim-release/cleanup is still in flight is protected (t3d_fresh_completed_worktree_is_not_reclaimed). Boundary age == threshold protected.
• Shared release chokepoint. Completed reclaim maps verdict = None and flows through release_engineer_claim + best-effort cleanup, identical to NoWorktree. No hand-rolled SQL; release/cleanup errors contained so one bad row never aborts the sweep.
• Test coverage is strong: positive, boundary, fail-closed, no-file, re-entered, direct-reclaim-without-investigation, and label stability all covered.

---

🟠 F1 (traceability — needs author confirmation, not a code defect)

The Step 17b requirements handed to this review (R1–R7) specify a different remediation: persist a WipRef { kind: "merged_pr" } and consume it in GhCliEvidenceSource::any_pr_merged, editing src/ooda_loop/cycle.rs, src/goal_curation/completion_gate.rs, and src/goal_curation/types.rs. This PR touches none of those files — it solves the same symptom (unbounded re-archival / BLOCKED churn) one layer down, at the claim-reaper.

These are complementary, not conflicting: the reaper fix stops re-investigating completed worktrees; the merged-PR-evidence fix (R1–R7) stops the completion gate from failing to recognize merged PRs. Please confirm whether (a) R1–R7 is a separate parallel workstream, or (b) this PR is intended to replace that approach. If (b), the requirement doc should be updated so the merged-PR-evidence work isn't later duplicated.

🟡 F2 (design note — intentional, worth documenting the assumption)

The Completed path reclaims without archiving worktree evidence (unlike the investigate-before-reap path for HeartbeatStale). The comment justifies this as "durable output is already committed/pushed." This is correct and is precisely the fix for the wasteful repeated archival in #4467. The residual risk: a session that wrote status=completed but whose push failed would have its worktree removed without evidence capture. Given completed is a positive terminal signal and the whole bug was excessive archival, the tradeoff is sound — recommend only that the "output already pushed" assumption stay documented (it currently is, in the L360 comment). No change required.

🔵 F3 (nit — optional)

A completed reclaim emits two log lines: the info! at ~L371 ("engineer session COMPLETED … reclaiming directly") and the standard fail-visible warn! reclaim line at ~L421 (verdict=no-investigation). Slightly redundant. Harmless — the info line adds churn-context — but could be dropped if log volume matters.

---

Checklist:

• Code quality and standards — clean, idiomatic, well-commented
• Test coverage adequate — 41 tests incl. positive/boundary/fail-closed/re-entry
• No TODOs, stubs, or swallowed exceptions — errors counted + logged, none swallowed
• No unimplemented functions
• Logic correctness — verified reclaim path, threshold guard, last-status semantics
• Edge case handling — no file, parse error, re-entered session, boundary age, in-flight cleanup

Recommendation: Approve on code merits; resolve F1 traceability before merge so the R1–R7 merged-PR-evidence track isn't inadvertently dropped.

[rysweet]

Step 17c — Security Review (PR #4712)

Scope: src/overseer/claim_reaper.rs (+254 / −9) on head 8fa7f622. Reviewed the new DeadReason::Completed path and the engineer_session_completed() filesystem/JSON seam introduced by this change. This is a self-contained internal reaper module — no network, auth, or user-request surface — so the review focuses on input handling, filesystem safety, injection, DoS, and information disclosure.

Security requirements — verified

• No new injection surface. No shell/exec, no SQL, no command construction, no format-string-into-command. The only new inputs are read from a local sessions.jsonl and parsed with serde_json (data-only). ✅
• No path traversal from untrusted input. The worktree passed to engineer_session_completed() is never derived by interpolating the untrusted claim_key into a path. assess() enumerates real directory entries under the controlled <state_root>/engineer-worktrees/ root (read_dir + goal_id_from_worktree_dir match), and engineer_session_completed() only appends the fixed literal suffix .claude/runtime/sessions.jsonl. A hostile claim_key cannot escape the root or select an arbitrary file. ✅
• Sensitive-data handling. The new file read extracts only the status string field; message/tool/credential content in the session record is ignored and never logged. New tracing lines emit only claim_key (a goal identifier, already logged elsewhere) — no secrets, tokens, paths-with-secrets, or file contents. ✅
• AuthZ/AuthN. N/A — internal reaper with no principal/permission boundary; the change adds no privilege decision.
• Memory-safety / panics. All fallible operations use let-else / match returning early; no unwrap/expect/indexing on the untrusted read+parse path. Malformed lines are skipped, not fatal. ✅

Fail-closed posture (security-positive)

The design is fail-closed in the safe direction: any missing file, IO error, parse error, or absent status record ⇒ false ⇒ the worktree stays on the ordinary investigate-before-reap (HeartbeatStale) path. A Completed short-circuit-reclaim is only taken when positively proven from the engineer's own terminal record, AND still gated by the same idle stale_secs threshold (T3d). This prevents a forged/partial/absent record from causing a premature reclaim — the worst an attacker controlling sessions.jsonl could do is suppress the optimization (force the slower, evidence-preserving path), not trigger destructive early cleanup of a live claim. ✅

Findings

S1 — LOW / informational — unbounded read_to_string on sessions.jsonl.
engineer_session_completed() calls std::fs::read_to_string(&sessions), loading the entire file into memory. sessions.jsonl is written by the trusted local agent runtime inside our own state root, so this is not a realistic external DoS vector today. However, an unbounded append-only log read on every reaper tick is a latent memory/CPU cost if the file ever grows large (long-lived worktree). Only the last status record matters. Suggested (non-blocking) hardening: cap the read (e.g. read the tail / a bounded number of bytes) or stream line-by-line with a size guard, so a pathologically large log cannot balloon reaper memory. No action required for merge.

S2 — INFO — trust boundary of sessions.jsonl is intra-root.
The completion signal is sourced from a file inside the engineer's own worktree. This is the correct trust model (the engineer attests its own completion), and the fail-closed gating (above) means a corrupted record cannot escalate into an unsafe reclaim. Documenting for traceability: the security guarantee rests on stale_secs gating + positive-proof, not on the integrity of sessions.jsonl.

Verdict

No exploitable vulnerabilities found. No new injection, no path traversal, no sensitive-data exposure, no unsafe panics. Input handling is fail-closed and defensively parsed. Only a single LOW/informational hardening note (S1, bounded read) and one trust-model note (S2), neither blocking.

Security review submitted as evidence for Step 17c. Approve on security merits.

[rysweet]

Step 17d — Philosophy Guardian Review (PR #4712)

Scope: src/overseer/claim_reaper.rs (+254 / −9), head 8fa7f62.
Verdict: ✅ COMPLIANT — merge-ready on philosophy grounds.

Compliance checklist

• Ruthless simplicity achieved. The fix adds exactly what the problem needs and no more: one DeadReason::Completed enum variant, one self-contained reader (engineer_session_completed), and one branch in the probe + one in reap_stale_claims. It reuses the existing idle-threshold guard, the shared release chokepoint, and the cleanup seam rather than inventing parallel machinery. No new abstractions, traits, or config surface.
• Bricks & studs pattern followed. engineer_session_completed(worktree) -> bool is a clean brick: one input (a path), one boolean contract ("did the engineer's own terminal record say completed"), zero side effects. It plugs into the existing ClaimLivenessProbe stud without perturbing callers. The new enum variant flows through label(), the probe, and the reaper along the established seams.
• Zero-BS implementation. No stubs, no todo!(), no faked APIs, no swallowed exceptions. Error handling is explicit and directional: every read/parse failure fail-closes to false (the safe investigate-before-reap path), and that direction is documented as intentional rather than hidden. The behavior is proven by real tests, not asserted by comment.
• No over-engineering. Resisted the tempting-but-wrong general solutions: no new "session state" module, no caching layer, no trait for status parsing. It reads the last status line and compares a string — matched to the actual evidence shape. Re-entry (active after completed) and in-flight completions (age ≤ threshold) are handled by existing mechanisms, not new ones.
• Clean module boundaries. Reclaim still routes through the single release chokepoint + cleanup (verified by t4 and the new t3c), so the completed path introduces no second reclaim route. The probe→reaper boundary is unchanged in shape; only a new discriminant crosses it.

Notes (non-blocking)

• Comment density: the WHY-comments are unusually heavy (near line-for-line rationale tied to claim-reaper HeartbeatStale investigation never converges: recipe verdict discarded (reconcile_inflight_investigations write-back gap) → Pending-only, reap path unreachable, unbounded re-archival #4467/claim-reaper/OODA brain mis-attribute a PRIOR incarnation's wedge log to the CURRENT worktree, and idle-age conflates completed with wedged (diagnostic-layer defect behind false-positive stale-engineer reaps) #4500/Engineer claim not released on session completion → leaked claim + 'goal disappeared before effect dispatch' (DownstreamFailed) no-worktree churn #4464). For a subtle completed-vs-wedged churn fix this is justified documentation of intent rather than clutter — but it sits at the upper bound of what "ruthless simplicity" tolerates. Fine as-is; worth keeping an eye on if future edits thin the surrounding context.
• Test coverage matches the contract: positive completion, re-entry (active last), absent file (fail-closed), fresh-completed-under-threshold, and direct-reclaim-without-investigation are all covered — the behavior claims are backed by tests, consistent with Zero-BS.

Cross-reference to Step 17b (F1)

The prior code review flagged that Step 17b's stated R1–R7 requirements target cycle.rs/completion_gate.rs/types.rs (a merged-PR-evidence fix) while this PR solves the same symptom one layer down in the claim-reaper. That is a traceability/scope question for the author, not a philosophy violation — this PR's implementation is itself clean and minimal. Philosophy verdict stands independent of how F1 is resolved.

Compliance status: PASS. No philosophy-driven changes required before merge.

[rysweet]

Step 17e — Blocking-Issue Resolution (PR #4712)

Reviewed all findings from the code (17b), security (17c), and philosophy (17d) reviews. Classified by the reviewers' own severities:

ID	Source	Severity	Blocking
F1	Code	Traceability question	Resolved below
F2	Code	Design note	No
F3	Code	Nit (log density)	No
S1	Security	LOW (unbounded read of intra-root trusted file)	No
S2	Security	INFO (trust-boundary note)	No
—	Philosophy	COMPLIANT	No

F1 resolved — scope decision (not a code defect)

F1 flagged that the requirements framing (R1–R7) named a merged-PR-evidence fix touching cycle.rs / completion_gate.rs / types.rs, whereas this PR fixes the same symptom one layer down, at the claim-reaper's liveness probe. This is a deliberate, sufficient design choice, not a dropped requirement:

1. Root cause is at the probe layer. The churn (claim-reaper HeartbeatStale investigation never converges: recipe verdict discarded (reconcile_inflight_investigations write-back gap) → Pending-only, reap path unreachable, unbounded re-archival #4467/claim-reaper/OODA brain mis-attribute a PRIOR incarnation's wedge log to the CURRENT worktree, and idle-age conflates completed with wedged (diagnostic-layer defect behind false-positive stale-engineer reaps) #4500/Engineer claim not released on session completion → leaked claim + 'goal disappeared before effect dispatch' (DownstreamFailed) no-worktree churn #4464) originates because WorktreeClaimLivenessProbe::assess conflated a completed engineer with a wedged one purely by newest-file mtime. Fixing classification at its source — reading the engineer's own terminal status=completed record — severs the conflation directly. A downstream completion-gate change in cycle.rs/completion_gate.rs would treat the symptom, not the misclassification.
2. Complementary, not conflicting. Per the PR description, this layer operates before classification and is explicitly distinct from the reconcile write-back (observe(overseer): surface stale-engineer investigation completion in reconcile (#4467) #4489/claim-reaper HeartbeatStale investigation never converges: recipe verdict discarded (reconcile_inflight_investigations write-back gap) → Pending-only, reap path unreachable, unbounded re-archival #4467) and the perpetual-goal exemption (fix(claim-reaper): give standing claims bounded extra time #4479/claim-reaper reaps healthy standing/perpetual-goal engineers as false positives (missing is_perpetual() exemption that no_progress.rs already applies) #4437). It does not overlap or contradict those in-flight fixes.
3. Fail-closed and bounded. A completed verdict must be positively proven from the engineer's own record; any missing/corrupt/absent record falls back to the safe investigate-before-reap path, still gated by the same idle threshold.

Conclusion: F1 is resolved as a scope decision. The claim-reaper approach is the chosen, targeted fix; the multi-file evidence-gate variant (R1–R7) is superseded by it for this defect. No code change required.

Non-blocking items (deferred, tracked)

• F3 (info+warn on completed reclaim): the info! records the decision rationale, the warn! records the reclaim outcome — intentionally distinct fail-visible lines. Left as-is.
• F2 / S2: documented design/trust-boundary notes, no action.
• S1: latent memory cost on a trusted intra-root file, no external DoS vector. Suitable for a follow-up hardening (bounded/tail read); not blocking.

Verdict

No blocking issues. All three reviews approve on merits; the sole pre-merge item (F1) is resolved as a scope decision above. Validation on head 8fa7f62 remains green: cargo clippy --lib clean, cargo test --lib overseer::claim_reaper → 41 passed / 0 failed. No code changes were required, so no re-review is needed.

[rysweet]

Step 18b — Review Feedback Implementation (PR #4712)

All three Step 17 reviews (Code / Security / Philosophy) approve on merits. Disposition of every feedback item below.

ID	Source	Severity	Disposition	Action
F1	Code	Blocking (pre-merge question)	✅ Resolved	Scope decision (17e): fixing completed-vs-wedged classification at the claim-reaper probe layer is more direct than the R1–R7 merged-PR-evidence track in cycle.rs/completion_gate.rs/types.rs. Complementary, fail-closed, threshold-gated. No code change.
F2	Code	Non-blocking (doc note)	✅ No action	"output already pushed" assumption already documented at the Completed path. Comment retained.
F3	Code	Non-blocking (nit)	🟡 Keep as-is	info! (decision rationale) and warn! (reclaim outcome) are intentionally distinct fail-visible lines. Documented reasoning; no change.
S1	Security	LOW (hardening)	✅ Tracked	Filed #4718 — bound the sessions.jsonl read in engineer_session_completed(). Non-blocking; deferred post-merge so it isn't lost.
S2	Security	INFO (trust note)	✅ No action	Trust-boundary is intra-root; safety rests on fail-closed gating, which is verified. Documented, no change.
—	Philosophy	COMPLIANT (density note)	✅ No action	Comment density justified for this subtle churn fix. No change.

Blocking issues: 0 open

F1 (the only pre-merge item) is closed as an intentional scope decision.

Required code changes for merge: none

Head unchanged at 8fa7f622.

Validation (head 8fa7f622)

• cargo clippy --lib — clean, no new warnings
• cargo test --lib overseer::claim_reaper — 41 passed / 0 failed

PR #4712 remains merge-ready. S1 hardening tracked as #4718.