fix(ooda): liveness-verify worker_present to close fail-open reasoner bug (#4631)

[rysweet]

Summary

Fixes a fail-open bug in the per-goal OODA reasoner. gather_per_goal_cycle_ctx
computed worker_present from bare engineer_worktrees map membership, so a
leaked/orphaned claim entry (engineer SIGKILLed, OOM, host reboot, daemon
crash-reload) read as "present … alive" forever — the goal never re-spawned and
never populated stale_claim_secs, silently wedging.

The presence read is now gated on a verified-live engineer via the existing
find_live_engineer_for_goal helper (PID + starttime), mirroring the #4608 /
#4574 fail-close hardening. contains_key is retained as a short-circuit so
the filesystem scan only runs for goals that actually hold a claim. Dead/leaked
claims now read false and the existing stale_claim_secs reclaim path
re-engages automatically.

Additive and non-breaking: genuinely-live workers are unchanged, and the read
stays fail-closed on dead/unreadable claims.

Changes

• src/ooda_loop/cycle.rs — single-site liveness gate on worker_present
  at gather_per_goal_cycle_ctx; adds hermetic, serial-safe regression tests
  (leaked → absent+reclaimable, live → present, no-entry → short-circuit).
• Docs — worker-presence liveness-verification contract:
  docs/concepts/worker-presence-liveness-verification.md,
  docs/concepts/engineer-claim-liveness-lease.md,
  docs/reference/worker-presence-liveness-api.md,
  docs/reference/ooda-per-goal-cycle-api.md,
  docs/fail-open-audit.md, mkdocs.yml.

Issue

Closes #4631

[rysweet]

Step 17b — Comprehensive Code Review

Verdict: ✅ Approve (zero blocking findings). Independently re-verified below, not rubber-stamped.

Scope reviewed (true merge diff, not the misleading two-dot view)

gh pr diff renders +755 / −2184 across 19 files, which looks alarming. That is a two-dot artifact: this branch is 1 ahead / 4 behind main, so gh shows main's newer content as apparent deletions. The actual three-dot merge diff (main...HEAD) is +688 / −2 across 7 files — exactly the single commit dd76dbe3c. git merge-tree --write-tree reports a clean merge, 0 conflict markers (mergeable: MERGEABLE). No unintended reverts of no_progress.rs / gates.rs / deleted docs will occur on merge.

Checklist

• Code quality & standards ✅ — Single-site change at cycle.rs. Presence read is now contains_key(goal_id) && find_live_engineer_for_goal(&simard_state_root(), goal_id).is_some(). Cheap→expensive short-circuit preserves the IO-free fast path for the common "no claim" case. Mirrors the #4437 / #4574 / #4608 fail-close hardening exactly (Approach B), adds no new public API, no new SIMARD_*_SECS threshold, no print!/println!/dbg!, structured-tracing discipline intact.
• Logic correctness ✅ — Verified symbol resolution end-to-end: find_live_engineer_for_goal(&Path, &str) -> Option<PathBuf> at advance_goal/spawn.rs:806, re-exported via advance_goal/mod.rs:41; simard_state_root() re-exported from crate::goal_curation. Call site types (&PathBuf → &Path, &str, .is_some()) all line up. Fail-closed direction is correct: None (dead or unreadable/IO-error) → worker_present = false → existing stale_claim_secs reclaim re-engages. Never fabricates a live worker.
• Edge cases ✅ — Dead PID, recycled-PID (start-time guard inherited from the helper), missing sentinel, and read_dir IO error all resolve to "not present" (safe). Genuinely-live workers unchanged (no over-correction).
• Test coverage ✅ — Three hermetic, #[serial(cognitive_memory)] tests over real allocated worktrees + live claim sentinels: (1) leaked/dead-PID → !worker_present and stale_claim_secs.is_some() (the TDD driver that fails pre-fix); (2) live engineer → worker_present + no stale signal (over-correction guard); (3) no map entry → short-circuit absent. Env-var mutation is scoped/restored via a Drop guard; git fixtures run with a scrubbed env. Respects the #4571/#4575 de-flake precedent.
• No TODOs / stubs / swallowed exceptions / unimplemented ✅ — Grep of added lines is clean; no unwrap/expect in production code (only in test fixtures, appropriately).

Independent verification performed

• cargo test --locked --lib tests_worker_present_liveness → 3 passed, 0 failed (clean 1m compile, no warnings surfaced).
• CI: coverage, cargo-audit/deny/vet, npm-audit, scripts-tests, GitGuardian pass; pre-commit pending (must go green before merge).

Non-blocking observations (no action required)

1. Quadratic scan — N claim-holding goals each scan the full worktrees root (~N entries) per cycle. Already documented in the new API-reference #cost note and bounded by concurrent-engineer admission limits. Approach A (in-process claim check) remains a non-breaking future optimization if profiling ever flags it.
2. Branch is 4 commits behind main — merge is clean, so not required, but a rebase would make gh pr diff legible for human reviewers.

Conclusion: Correct, minimal, well-tested fix that closes the #4631 fail-open leak by mirroring the already-reviewed fail-close liveness verifier. Scope guard honored (no changes to subordinate.rs reap selection, reaper, or resource-admission). Ready to merge once pre-commit is green.

[rysweet]

Step 17c — Security Review (MANDATORY)

Verdict: ✅ Approve — net security improvement, zero new vulnerabilities. Independently analyzed against the true 3-dot merge diff (origin/main...HEAD: +688/−2, 7 files; only one production file — src/ooda_loop/cycle.rs).

Security requirements — all met

Area	Finding
New vulnerabilities	None. The only production change is a two-clause guard: contains_key(goal_id) && find_live_engineer_for_goal(state_root, goal_id).is_some(). It reuses the existing, already-hardened liveness verifier — no new code paths, no new external inputs.
Fail-open → fail-closed	This closes a security-relevant leak (#4631): a leaked/idle engineer_worktrees map entry previously read "worker present … alive" forever, so a dead engineer was never reclaimed. Now presence requires a verified-live engineer; unverifiable → not present → existing stale_claim_secs reclaim re-engages. Strictly tighter posture.
PID-spoofing / recycled-PID (TOCTOU)	Mitigated. find_live_engineer_for_goal checks is_pid_alive_public(pid) and a starttime guard (read_pid_starttime_public(pid) == recorded). A recycled PID with mismatched starttime is treated as dead — no false "alive". Pre-#1238 sentinels (PID-only) degrade safely to PID liveness.
Path traversal / injection	None. goal_id is used only as a starts_with("{goal_id}-") filter over read_dir entries; the claim path is built from the enumerated entry.path(), never by joining attacker-controlled goal_id into a filesystem path. No shell, SQL, or format-string injection surface.
Sensitive data handling	No secrets touched. The .simard-engineer-claim sentinel holds only <pid>\n<starttime>. No credentials, tokens, or PII read, logged, or emitted. No new logging of user-derived data.
AuthN/AuthZ	N/A — intra-daemon liveness reconciliation, no auth boundary crossed. Trust boundary (SIMARD_STATE_ROOT/SIMARD_HOME env) is unchanged and daemon-owned; not newly exposed by this PR.
DoS / resource exhaustion	Bounded. The filesystem scan is gated behind the cheap IO-free contains_key short-circuit (runs only when a claim exists) and the directory set is bounded by the engineer-admission limits. read_dir errors fail closed (ok()? → None), not panic.
Panics / robustness	No unwrap/expect/panic! in the production change; all IO uses .ok()/match fallthrough. Non-UTF8 dir names and unreadable sentinels are skipped, not fatal.

Notes (non-blocking)

• The verifier's directory scan is O(worktrees) per gated call — acceptable given the admission-bounded worktree count and the contains_key pre-gate. No security impact.
• SIMARD_STATE_ROOT remains an implicit-trust env var (pre-existing design), correctly serialized in tests via #[serial(cognitive_memory)].

Conclusion: No secrets exposed, no injection or traversal vectors, no new attack surface. The change is a defense-in-depth hardening that eliminates a fail-open reclaim gap. Approved from a security standpoint.

[rysweet]

Step 17d — Philosophy Guardian Review

Verdict: ✅ COMPLIANT — approved against all four principles. Assessed on the true 3-dot merge diff (+688/−2, 7 files), not the misleading 2-dot gh pr diff.

Philosophy compliance checklist

• Ruthless simplicity achieved — The entire behavioral fix is a single production expression: contains_key(goal_id) && find_live_engineer_for_goal(...).is_some(). No new abstractions, types, config flags, or indirection. It reuses the existing claim-reaper reaps healthy standing/perpetual-goal engineers as false positives (missing is_perpetual() exemption that no_progress.rs already applies) #4437 on-disk verifier rather than inventing a parallel liveness mechanism.
• Bricks & studs pattern followed — Consumes find_live_engineer_for_goal + simard_state_root() through their public "studs"; no reach into internals. Clean module boundary: cycle.rs does not duplicate PID/starttime logic, it delegates. Scope guard intact — no edits to subordinate.rs, reaper, or resource-admission code.
• Zero-BS implementation — No stubs, no todo!/unimplemented!, no faked APIs, no swallowed exceptions. Unverifiable → None → not present routes into the existing stale_claim_secs reclaim path (no silent failure). Independently scanned added lines: zero print!/dbg!, zero prod unwrap/expect. (The one unsafe block is test-only, #[serial]-guarded, and SAFETY-documented.)
• No over-engineering — Cheap→expensive short-circuit preserves the IO-free fast path for the common "no claim" case; the filesystem scan only runs when a claim exists. Fix is proportional to the bug — closes the fail-OPEN leak without adding machinery.
• Clean module boundaries — Production change confined to one call site; regression tests isolated in a #[cfg(test)] module with hermetic real-worktree fixtures. Documentation additions (5 files + mkdocs nav) are additive and non-invasive.

Notes

• The generous inline comment block is justified — it encodes the fail-OPEN/fail-CLOSE relationship to claim-reaper reaps healthy standing/perpetual-goal engineers as false positives (missing is_perpetual() exemption that no_progress.rs already applies) #4437/fix(ooda): stop no-progress breaker duplicate-issue storm; derive terminal-rung criteria #4574/docs(overseer): persist fail-closed still-alive verdict for stale engineer 70ab8541 #4608 and the exact corrected contract, which is load-bearing context for future maintainers of this liveness invariant. This is documentation-of-intent, not clutter.
• Non-blocking (already surfaced by prior passes): the liveness scan is O(n) over worktrees, bounded by admission limits — acceptable, no change required.

No remediation items. Change is minimal, verified, and philosophy-compliant.

[rysweet]

Step 18b — Review Feedback Implementation Summary

All three mandatory reviews (Code 17b, Security 17c, Philosophy 17d) returned ✅ Approve/Compliant with zero blocking findings. Actions taken per the Step 18a action plan:

#	Item	Priority	Action	Result
A1	pre-commit CI check pending (merge gate)	P0	Verified green; re-validated the pre-commit gate locally in the worktree	✅ Cleared — both pre-commit runs passed (12m49s / 15m21s). Locally confirmed: cargo fmt --check clean, check-rust-only-gate.sh pass, clippy-precommit-release.sh (--release -D warnings) clean with the #2426 lbug SUCCESS marker
A2	Misleading 2-dot gh pr diff (branch 4 behind main)	P2	Rebased onto origin/main (clean, no conflicts) and force-pushed with lease	✅ Done — branch now 0-behind/1-ahead (dd76dbe3c → f97437f9d). The phantom −2184 deletions are gone; 2-dot diff now equals the true 3-dot merge diff (+688/−2, 7 files). hooks/pre-push gate passed on push (478 tests, clippy --all-targets --all-features --locked -- -D warnings clean)
A3	Documented quadratic worker scan	P3	Deferred (no change)	Bounded by admission limits; already documented. Revisit only if worker counts exceed current limits

Net result

• No production code changes were required — every substantive review dimension (correctness, security, philosophy) passed on the first pass.
• The only true merge gate (A1: pre-commit) is green.
• A2 hygiene applied: history is now linear and the diff is unambiguous for whoever merges.
• Fix intact after rebase: src/ooda_loop/cycle.rs:1623 — worker_present gated on contains_key(goal_id) && find_live_engineer_for_goal(...).is_some() (fail-closed).
• CI has re-triggered on the rebased commit f97437f9d; fast checks already green, pre-commit/coverage re-running.

Scope guard intact — no speculative edits. Ready to merge once the re-triggered pre-commit completes green.

[github-actions]

📊 Coverage Summary

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

Module	Lines	Covered	Coverage
Total	198391	167032	84.2%

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