Update documentation with 33 changed files (#4575)

docs/reference/engineer-worktree-isolation.md

@@ -9,6 +9,7 @@ related:
   - ../howto/spawn-engineers-from-ooda-daemon.md
   - ./engineer-loop-argv-sanitization.md
   - ./engineer-worktree-sweep-safety.md
+  - ./engineer-worktree-presence-guard.md
   - ../howto/run-ooda-daemon.md
 ---
 

src/engineer_worktree/mod.rs

@@ -47,6 +47,8 @@ mod tests_extra;
 #[cfg(test)]
 mod tests_more;
 #[cfg(test)]
+mod tests_presence_guard;
+#[cfg(test)]
 mod tests_reaping_safety;
 
 /// Subdirectory under the supervisor state root that holds all engineer worktrees.
@@ -70,8 +72,8 @@ pub const ENGINEER_CLAIM_FILE: &str = ".simard-engineer-claim";
 mod claim;
 mod discovery;
 mod precommit;
-use claim::{claim_is_live, format_engineer_claim, read_engineer_claim_full};
-pub use claim::{is_pid_alive_public, read_pid_starttime_public};
+use claim::{claim_is_live, format_engineer_claim};
+pub use claim::{is_pid_alive_public, read_engineer_claim_full, read_pid_starttime_public};
 pub(crate) use discovery::goal_id_from_worktree_dir;
 pub use discovery::{
     LiveEngineerWorktree, live_claimed_engineers, live_claimed_engineers_in_worktrees,
@@ -448,6 +450,25 @@ impl EngineerWorktree {
         &self.branch
     }
 
+    /// Returns `true` iff this worktree still holds a readable engineer-claim
+    /// sentinel (`.simard-engineer-claim`) on disk.
+    ///
+    /// Side-effect-free, fail-closed presence probe that closes the worktree
+    /// reuse TOCTOU (issue #4578). Implemented as a single claim read via
+    /// [`read_engineer_claim_full`]: if the worktree directory was reaped out
+    /// of band, or the sentinel was removed/corrupted, the read errors and
+    /// this returns `false`. Reuse sites call it immediately before they
+    /// depend on `path()` still being on disk.
+    ///
+    /// Scope: worktree presence, not engineer PID liveness — discovery
+    /// (`find_live_engineer_for_goal`) owns the liveness check. The single
+    /// claim read is the smallest sufficient seam: `read_engineer_claim_full`
+    /// already fails closed when the directory is gone, so a separate `lstat`
+    /// buys nothing.
+    pub fn is_present(&self) -> bool {
+        read_engineer_claim_full(&self.path).is_some()
+    }
+
     /// Remove the worktree, prune its registration, delete its branch.
     ///
     /// Idempotent — second and subsequent calls are `Ok(())` no-ops.

src/engineer_worktree/tests_presence_guard.rs

@@ -0,0 +1,184 @@
+//! Red-phase TDD tests for the engineer-worktree **presence guard**
+//! (issue #4578).
+//!
+//! Context: goal-session cycles crashed with a bare missing-workspace fault
+//! because discovery / stored-map reuse handed back a worktree path that a
+//! concurrent GC/reaper had already removed (a TOCTOU between "found a live
+//! claim" and "use the checkout dir"). The fix introduces a single,
+//! side-effect-free presence seam owned by the worktree module —
+//! [`EngineerWorktree::is_present`] — that every reuse site consults
+//! immediately before it depends on the checkout still being on disk.
+//!
+//! These tests specify the contract for that new accessor. They MUST fail in
+//! the red phase (the method does not exist yet ⇒ the crate will not compile)
+//! and MUST pass once `is_present()` lands, without further test edits.
+//!
+//! Contract (aligned with `docs/reference/engineer-worktree-presence-guard.md`):
+//!   `is_present()` performs a single **fail-closed** claim read of
+//!   `self.path()/.simard-engineer-claim`. It returns `true` iff the checkout
+//!   directory still exists AND its claim sentinel is readable; any absence
+//!   (dir reaped, sentinel gone, unreadable) returns `false`. It never mutates
+//!   the filesystem and never re-provisions.
+
+use std::fs;
+use std::path::{Path, PathBuf};
+use std::process::Command;
+
+use tempfile::tempdir;
+
+use super::{ENGINEER_CLAIM_FILE, EngineerWorktree};
+
+// ---------------------------------------------------------------------------
+// Fixtures (self-contained: the sibling `tests` module's helpers are private).
+// ---------------------------------------------------------------------------
+
+fn git_cmd(repo: &Path, args: &[&str]) -> Command {
+    let mut cmd = Command::new("git");
+    cmd.args(args).current_dir(repo).env_clear();
+    if let Ok(p) = std::env::var("PATH") {
+        cmd.env("PATH", p);
+    }
+    if let Ok(h) = std::env::var("HOME") {
+        cmd.env("HOME", h);
+    }
+    cmd
+}
+
+fn run_git(repo: &Path, args: &[&str]) {
+    let out = git_cmd(repo, args).output().expect("spawn git");
+    assert!(
+        out.status.success(),
+        "git {:?} failed in {}: {}",
+        args,
+        repo.display(),
+        String::from_utf8_lossy(&out.stderr)
+    );
+}
+
+/// A parent repo with a committed `main` so `EngineerWorktree::allocate`
+/// (which branches off `main` HEAD) succeeds.
+fn init_parent_repo(dir: &Path) -> PathBuf {
+    fs::create_dir_all(dir).expect("create parent repo dir");
+    run_git(dir, &["init", "--initial-branch=main", "--quiet"]);
+    run_git(dir, &["config", "user.email", "test@example.com"]);
+    run_git(dir, &["config", "user.name", "test"]);
+    run_git(dir, &["config", "commit.gpgsign", "false"]);
+    fs::write(dir.join("README.md"), "seed\n").expect("seed file");
+    run_git(dir, &["add", "README.md"]);
+    run_git(dir, &["commit", "-m", "seed", "--quiet"]);
+    dir.to_path_buf()
+}
+
+// ---------------------------------------------------------------------------
+// Tests
+// ---------------------------------------------------------------------------
+
+/// A freshly allocated worktree is present: `allocate()` creates the checkout
+/// dir and writes the claim sentinel with the current PID, so the guard must
+/// report `true`.
+#[test]
+fn is_present_true_after_allocate() {
+    let parent = tempdir().expect("tempdir");
+    let state = tempdir().expect("tempdir");
+    init_parent_repo(parent.path());
+
+    let wt = EngineerWorktree::allocate(parent.path(), state.path(), "goal-present")
+        .expect("allocate engineer worktree");
+
+    assert!(
+        wt.path().is_dir(),
+        "precondition: freshly allocated worktree must exist on disk"
+    );
+    assert!(
+        wt.is_present(),
+        "a freshly allocated worktree with its claim sentinel must be present"
+    );
+}
+
+/// After the worktree's own `cleanup()` removes the checkout, the guard must
+/// report `false` — the core signal a reuse site needs to re-provision instead
+/// of returning a stale success.
+#[test]
+fn is_present_false_after_cleanup() {
+    let parent = tempdir().expect("tempdir");
+    let state = tempdir().expect("tempdir");
+    init_parent_repo(parent.path());
+
+    let wt = EngineerWorktree::allocate(parent.path(), state.path(), "goal-cleaned")
+        .expect("allocate engineer worktree");
+    assert!(wt.is_present(), "precondition: present before cleanup");
+
+    wt.cleanup().expect("cleanup engineer worktree");
+
+    assert!(
+        !wt.is_present(),
+        "after cleanup removes the checkout dir, is_present() must be false"
+    );
+}
+
+/// The #4578 fault exactly: a concurrent reaper removes the checkout dir out
+/// of band (no `cleanup()` call on this handle). The guard must still detect
+/// the absence so callers never hand back / reuse a reaped worktree.
+#[test]
+fn is_present_false_when_dir_reaped_out_of_band() {
+    let parent = tempdir().expect("tempdir");
+    let state = tempdir().expect("tempdir");
+    init_parent_repo(parent.path());
+
+    let wt = EngineerWorktree::allocate(parent.path(), state.path(), "goal-reaped")
+        .expect("allocate engineer worktree");
+    assert!(wt.is_present(), "precondition: present before reap");
+
+    // Simulate the GC/reaper removing the worktree dir underneath us, WITHOUT
+    // going through this handle's cleanup(). This is the TOCTOU window.
+    fs::remove_dir_all(wt.path()).expect("simulate out-of-band reap");
+
+    assert!(
+        !wt.is_present(),
+        "a worktree whose dir was reaped out of band must report not-present"
+    );
+}
+
+/// Fail-closed: if the checkout dir survives but the claim sentinel is gone
+/// (a partially reaped / corrupted worktree), the guard must return `false`
+/// rather than optimistically treating the checkout as reusable.
+#[test]
+fn is_present_false_when_claim_sentinel_removed() {
+    let parent = tempdir().expect("tempdir");
+    let state = tempdir().expect("tempdir");
+    init_parent_repo(parent.path());
+
+    let wt = EngineerWorktree::allocate(parent.path(), state.path(), "goal-noclaim")
+        .expect("allocate engineer worktree");
+    assert!(wt.is_present(), "precondition: present with claim");
+
+    fs::remove_file(wt.path().join(ENGINEER_CLAIM_FILE)).expect("remove claim sentinel");
+    assert!(
+        wt.path().is_dir(),
+        "precondition: only the sentinel is gone, the dir remains"
+    );
+
+    assert!(
+        !wt.is_present(),
+        "missing claim sentinel must fail closed to not-present"
+    );
+}
+
+/// The guard is a pure observation: calling it must not create, delete, or
+/// otherwise mutate the worktree. Repeated calls are stable.
+#[test]
+fn is_present_is_side_effect_free_and_idempotent() {
+    let parent = tempdir().expect("tempdir");
+    let state = tempdir().expect("tempdir");
+    init_parent_repo(parent.path());
+
+    let wt = EngineerWorktree::allocate(parent.path(), state.path(), "goal-pure")
+        .expect("allocate engineer worktree");
+
+    assert!(wt.is_present());
+    assert!(wt.is_present());
+    assert!(
+        wt.path().is_dir() && wt.path().join(ENGINEER_CLAIM_FILE).exists(),
+        "is_present() must not have mutated the worktree or its sentinel"
+    );
+}

src/ooda_actions/advance_goal/mod.rs

@@ -29,6 +29,8 @@ mod overlap;
 // so `spawn::dispatch_spawn_engineer` can invoke the gate.
 pub(crate) mod resource_admission;
 mod subordinate;
+#[cfg(test)]
+mod tests_presence_guard;
 #[cfg(not(test))]
 pub(crate) mod typed_goal_session;
 #[cfg(test)]

src/ooda_actions/advance_goal/subordinate.rs

@@ -29,11 +29,29 @@ pub fn advance_goal_with_subordinate(
     // available so artifact validation looks at the engineer's own scope,
     // not the parent checkout. Falls back to "." for legacy/manual paths
     // that pre-date worktree isolation.
-    let worktree_path = state
-        .engineer_worktrees
-        .get(goal_id)
-        .map(|w| w.path().to_path_buf())
-        .unwrap_or_else(|| std::path::PathBuf::from("."));
+    //
+    // Presence guard (issue #4578): a stored worktree can be reaped out of
+    // band between cycles. Consult is_present() before trusting the stored
+    // path; when the checkout is gone, warn, drop the stale entry (the next
+    // spawn cycle re-provisions a clean worktree) and fall back to the parent
+    // scope instead of dereferencing a missing path.
+    let worktree_path = match state.engineer_worktrees.get(goal_id) {
+        Some(worktree) if worktree.is_present() => worktree.path().to_path_buf(),
+        Some(worktree) => {
+            let stale_path = worktree.path().to_path_buf();
+            tracing::warn!(
+                target: "simard::engineer_worktree",
+                event = "engineer_worktree.reaped_before_reuse",
+                goal_id,
+                worktree = %stale_path.display(),
+                action = "reprovision",
+                "stored engineer worktree reaped before reuse; dropping stale entry",
+            );
+            state.engineer_worktrees.remove(goal_id);
+            std::path::PathBuf::from(".")
+        }
+        None => std::path::PathBuf::from("."),
+    };
     let handle = crate::agent_supervisor::SubordinateHandle {
         pid: 0,
         agent_name: sub_name.to_string(),