From 96fdcbcd4bf98345252ff7898f24d1711933e1f7 Mon Sep 17 00:00:00 2001 From: Marc M Date: Wed, 15 Jul 2026 10:29:49 +0000 Subject: [PATCH 1/5] fix(workers): isolate each worker in its own git worktree (#224) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Concurrent builtin workers shared the agent's single checkout (runtime_config.workspace_dir), so one worker's git operations could see and clobber a sibling's in-progress edits — the #211 stale-file sweep, the #218/#219 byte-identical blob, and the 2026-07-15 feature/1 stranding. Give every worker its own isolated workspace: - New src/agent/worker_workspace.rs: WorkerWorkspace::provision adds a cheap git worktree (shared object store, private working tree + index) per repo in the shared workspace, on a fresh per-worker branch off HEAD. release() removes them on success; reap_orphaned() bounds retained ones on startup (kept on failure for forensics). - Worker gains an optional isolated_workspace; its shell/file tools operate in that directory instead of the shared workspace when present, and it is released on successful completion. - channel_dispatch provisions and attaches an isolated workspace to every builtin worker (best-effort: falls back to shared workspace on failure, so a single worker sees no behavior change). - cortex startup reaps orphaned per-worker workspaces. - pr-gates skill: explicit-path git add + clean-status-before-commit as a second-layer mitigation. Tests (worker_workspace): a concurrent-dispatch repro proves two workers' git add -A commits cannot contain each other's edits and the shared branch is never mutated. --- .agents/skills/pr-gates/SKILL.md | 13 + src/agent.rs | 1 + src/agent/channel_dispatch.rs | 31 ++ src/agent/cortex.rs | 12 + src/agent/worker.rs | 45 ++- src/agent/worker_workspace.rs | 534 +++++++++++++++++++++++++++++++ 6 files changed, 635 insertions(+), 1 deletion(-) create mode 100644 src/agent/worker_workspace.rs diff --git a/.agents/skills/pr-gates/SKILL.md b/.agents/skills/pr-gates/SKILL.md index 0bcb22ae8..6c371520f 100644 --- a/.agents/skills/pr-gates/SKILL.md +++ b/.agents/skills/pr-gates/SKILL.md @@ -29,6 +29,19 @@ When touching worker lifecycle, cancellation, retries, state transitions, or cac - Run targeted tests in addition to broad gate runs. - Capture the exact command proving the behavior. +## Commit Hygiene — Never Sweep Sibling Edits (issue #224) + +Workers now run in an isolated per-worker `git worktree` (see +`src/agent/worker_workspace.rs`), but keep these rules as a second layer so a +commit can never contain files this worker did not touch: + +- Stage **explicit paths** you changed: `git add [ ...]`. Never + `git add -A` or `git add .` — a blind sweep can pick up unrelated content. +- Verify a **clean, intentional** stage before committing: run + `git status --porcelain` and confirm every staged path is one you edited. +- If the working tree is unexpectedly dirty at handout (files you did not + create), stop and investigate rather than committing over them. + ## Migration Safety - Never edit an existing file in `migrations/`. diff --git a/src/agent.rs b/src/agent.rs index 66a8730d6..80b5c55e8 100644 --- a/src/agent.rs +++ b/src/agent.rs @@ -18,6 +18,7 @@ pub mod prompt_snapshot; pub mod status; pub mod wake; pub mod worker; +pub mod worker_workspace; pub(crate) fn panic_payload_to_string(panic_payload: &(dyn std::any::Any + Send)) -> String { panic_payload diff --git a/src/agent/channel_dispatch.rs b/src/agent/channel_dispatch.rs index 0327e5ca6..93a41ad71 100644 --- a/src/agent/channel_dispatch.rs +++ b/src/agent/channel_dispatch.rs @@ -636,6 +636,28 @@ pub async fn spawn_worker_from_state( result } +/// Provision and attach a per-worker isolated workspace (issue #224). +/// +/// Gives the worker its own `git worktree` per repo in `shared_workspace`, so +/// concurrent workers can't see or clobber each other's edits. Provisioning is +/// best-effort: on failure the worker is returned unchanged and falls back to +/// the shared workspace, so a provisioning hiccup never blocks dispatch. +async fn attach_isolated_workspace(worker: Worker, shared_workspace: &std::path::Path) -> Worker { + match crate::agent::worker_workspace::WorkerWorkspace::provision(shared_workspace, worker.id) + .await + { + Ok(workspace) => worker.with_isolated_workspace(workspace), + Err(error) => { + tracing::warn!( + %error, + worker_id = %worker.id, + "failed to provision isolated workspace — worker will use shared workspace" + ); + worker + } + } +} + /// Inner implementation of worker spawning, separated so the caller can /// handle task reservation cleanup in a single place. async fn spawn_worker_inner( @@ -763,6 +785,13 @@ async fn spawn_worker_inner( .resolve_model("worker") .map(String::from); + // Provision a per-worker isolated workspace (issue #224): each worker + // gets its own `git worktree` per repo so concurrent workers can't see or + // clobber each other's edits. Best-effort — if provisioning fails, the + // worker falls back to the shared workspace with a warning rather than + // failing dispatch. + let shared_workspace = state.deps.runtime_config.workspace_dir.clone(); + let worker = if interactive { let (worker, input_tx, inject_tx) = Worker::new_interactive( Some(state.channel_id.clone()), @@ -778,6 +807,7 @@ async fn spawn_worker_inner( worker_context.wiki_write, worker_model_override, ); + let worker = attach_isolated_workspace(worker, &shared_workspace).await; let worker_id = worker.id; state .worker_inputs @@ -805,6 +835,7 @@ async fn spawn_worker_inner( worker_context.wiki_write, worker_model_override, ); + let worker = attach_isolated_workspace(worker, &shared_workspace).await; state .worker_injections .write() diff --git a/src/agent/cortex.rs b/src/agent/cortex.rs index c560a759a..3524e0f67 100644 --- a/src/agent/cortex.rs +++ b/src/agent/cortex.rs @@ -1778,6 +1778,18 @@ pub fn spawn_cortex_loop(deps: AgentDeps, logger: CortexLogger) -> tokio::task:: drop(prompt_engine); let cortex = Cortex::new(deps.clone(), system_prompt); + + // Reap orphaned per-worker workspaces left by crashes or retained + // failures (issue #224), keeping recent ones for forensics. Best-effort. + { + let shared_workspace = deps.runtime_config.workspace_dir.clone(); + if let Err(error) = + crate::agent::worker_workspace::reap_orphaned(&shared_workspace).await + { + tracing::warn!(%error, "failed to reap orphaned worker workspaces on startup"); + } + } + let mut event_rx = deps.event_tx.subscribe(); let mut memory_event_rx = deps.memory_event_tx.subscribe(); let mut tool_output_rx = deps.tool_output_tx.subscribe(); diff --git a/src/agent/worker.rs b/src/agent/worker.rs index 3f0a80eaf..d521610a6 100644 --- a/src/agent/worker.rs +++ b/src/agent/worker.rs @@ -274,6 +274,12 @@ pub struct Worker { /// loop reads it at each iteration and short-circuits to /// `WorkerOutcome::Blocked` when populated. pub blocked_signal: BlockSignal, + /// Per-worker isolated workspace (issue #224). When set, the worker's + /// shell and file tools operate in this directory — its own `git worktree` + /// per repo — instead of the shared `workspace_dir`, so concurrent workers + /// can't see or clobber each other's edits. Released on successful + /// completion; retained for forensics (and reaped later) on failure. + pub isolated_workspace: Option, } impl Worker { @@ -339,6 +345,7 @@ impl Worker { worker_wall_clock_timeout_secs, segments_run: std::sync::Arc::new(std::sync::atomic::AtomicUsize::new(0)), blocked_signal: new_block_signal(), + isolated_workspace: None, }, inject_tx, ) @@ -421,6 +428,29 @@ impl Worker { (worker, input_tx, inject_tx) } + /// Attach a per-worker isolated workspace (issue #224). + /// + /// When set, the worker's shell and file tools operate in this directory + /// instead of the shared `workspace_dir`, giving concurrent workers their + /// own `git worktree` per repo. Consumed builder-style so it reads + /// naturally at the call site. + pub fn with_isolated_workspace( + mut self, + workspace: crate::agent::worker_workspace::WorkerWorkspace, + ) -> Self { + self.isolated_workspace = Some(workspace); + self + } + + /// The base directory the worker's tools should use: the isolated + /// workspace when one was provisioned, otherwise the shared workspace. + fn tool_workspace(&self) -> PathBuf { + match &self.isolated_workspace { + Some(ws) if ws.has_worktrees() => ws.root().to_path_buf(), + _ => self.deps.runtime_config.workspace_dir.clone(), + } + } + /// Resume an interactive worker that was idle at shutdown. /// /// Instead of running the initial task, skips directly to the follow-up @@ -561,6 +591,11 @@ impl Worker { .clone() .with_tool_call_registry(tool_call_registry.clone()); + // Per-worker isolated workspace (issue #224): when a worktree was + // provisioned, the shell/file tools operate there so concurrent + // workers can't clobber each other's edits. + let tool_workspace = self.tool_workspace(); + // Create per-worker ToolServer with task tools let worker_tool_server = crate::tools::create_worker_tool_server( self.deps.agent_id.clone(), @@ -573,7 +608,7 @@ impl Worker { self.browser_config.clone(), self.screenshot_dir.clone(), self.brave_search_key.clone(), - self.deps.runtime_config.workspace_dir.clone(), + tool_workspace, self.deps.sandbox.clone(), mcp_tools, self.deps.runtime_config.clone(), @@ -1066,6 +1101,14 @@ impl Worker { } tracing::info!(worker_id = %self.id, "worker completed"); + // Successful completion: release the isolated workspace (remove its + // worktrees). On error/timeout paths we deliberately skip this so the + // workspace is retained for forensics; the startup reaper bounds it. + if let Some(workspace) = self.isolated_workspace.take() + && let Err(error) = workspace.release().await + { + tracing::warn!(%error, worker_id = %self.id, "failed to release isolated workspace"); + } if hit_max_segments { Ok(WorkerOutcome::Partial { result, diff --git a/src/agent/worker_workspace.rs b/src/agent/worker_workspace.rs new file mode 100644 index 000000000..2285f28a2 --- /dev/null +++ b/src/agent/worker_workspace.rs @@ -0,0 +1,534 @@ +//! Per-worker workspace isolation via git worktrees. +//! +//! # Why this exists (issue #224) +//! +//! Every builtin worker used to run its shell and file tools in the agent's +//! single shared checkout (`runtime_config.workspace_dir`). When two workers +//! ran concurrently against the same repo, one worker's `git add` swept up a +//! sibling's in-progress edits, branches got contaminated with stale content, +//! and work was committed to the wrong branch and then stranded. See the +//! incidents catalogued in issue #224 (PR #211 stale-file sweep, PR #218/#219 +//! byte-identical blob, the 2026-07-15 `feature/1` stranding). +//! +//! # What it does +//! +//! [`WorkerWorkspace::provision`] hands each worker its own isolated directory. +//! For every git repository directly under the shared workspace it adds a cheap +//! `git worktree` (shared object store, private working tree + index) checked +//! out on a fresh per-worker branch off the repo's current `HEAD`. The worker's +//! tool server is pointed at this directory instead of the shared workspace, so +//! two concurrent workers can never see or clobber each other's edits. +//! +//! # Guarantees +//! +//! - **Isolation.** Each worker gets its own working tree and index per repo, so +//! `git add`/`commit`/`checkout` in one worker are invisible to siblings. +//! - **Correct starting point.** Each worktree starts at the repo's current +//! `HEAD`; the worker checks out or creates its target branch inside its own +//! worktree, never mutating the shared checkout's branch. +//! - **Cleanup with forensics.** [`WorkerWorkspace::release`] removes the +//! worktrees on success. On failure the caller keeps the workspace for +//! forensics; [`reap_orphaned`] bounds retained workspaces on startup. +//! - **No behaviour change for the degenerate case.** If the shared workspace +//! holds no git repos, provisioning yields an isolated-but-empty directory and +//! falls back transparently; a single worker sees no difference. + +use std::path::{Path, PathBuf}; + +use tokio::process::Command; + +use crate::WorkerId; + +/// Directory (under the shared workspace) that holds all per-worker isolated +/// workspaces. Kept under `.spacebot` so it never looks like a project repo to +/// [`crate::projects::git::discover_repos`]. +const WORKSPACES_SUBDIR: &str = ".spacebot/worker-workspaces"; + +/// Maximum number of leftover per-worker workspaces to retain on startup before +/// reaping the oldest. Bounds disk use while keeping recent failures for +/// forensics. +const MAX_RETAINED_WORKSPACES: usize = 20; + +/// An isolated per-worker workspace: a directory containing one `git worktree` +/// per repo discovered in the shared workspace. +/// +/// Drop does **not** clean up — cleanup is explicit via [`Self::release`] so the +/// caller controls the success-vs-forensics policy. +#[derive(Debug)] +pub struct WorkerWorkspace { + /// The shared workspace this isolated workspace was derived from. Needed to + /// run `git worktree remove` from each source repo on release. + shared_root: PathBuf, + /// Root of the isolated workspace (`/.spacebot/worker-workspaces/`). + root: PathBuf, + /// The worktrees created, as `(source_repo_path, worktree_path)` pairs. + worktrees: Vec, +} + +/// One repo mirrored into an isolated workspace as a git worktree. +#[derive(Debug, Clone)] +struct IsolatedRepo { + /// Path to the source repo in the shared workspace (the worktree's parent). + source: PathBuf, + /// Path to the isolated worktree. + worktree: PathBuf, + /// The per-worker branch created in the worktree. + branch: String, +} + +impl WorkerWorkspace { + /// The isolated workspace root the worker's tools should use as their + /// base directory. + pub fn root(&self) -> &Path { + &self.root + } + + /// Whether any repo was isolated. When `false`, the isolated root is empty + /// and the caller may prefer to fall back to the shared workspace. + pub fn has_worktrees(&self) -> bool { + !self.worktrees.is_empty() + } + + /// Provision an isolated workspace for `worker_id` off `shared_root`. + /// + /// Discovers git repos directly under `shared_root` and adds a per-worker + /// worktree for each on a fresh branch off the repo's current `HEAD`. + /// Returns the workspace even if no repos were found (empty isolated root), + /// so callers get a consistent, isolated base directory regardless. + /// + /// Errors only on unrecoverable filesystem failures (cannot create the + /// workspace root). A repo that fails to isolate is logged and skipped + /// rather than failing the whole worker — partial isolation is strictly + /// better than the shared checkout. + pub async fn provision(shared_root: &Path, worker_id: WorkerId) -> anyhow::Result { + let root = shared_root + .join(WORKSPACES_SUBDIR) + .join(worker_id.to_string()); + tokio::fs::create_dir_all(&root).await.map_err(|e| { + anyhow::anyhow!("failed to create worker workspace {}: {e}", root.display()) + })?; + + let repos = discover_repo_dirs(shared_root).await; + let branch = worker_branch_name(worker_id); + let mut worktrees = Vec::new(); + + for source in repos { + let name = match source.file_name().and_then(|n| n.to_str()) { + Some(name) => name.to_string(), + None => continue, + }; + let worktree = root.join(&name); + match add_worktree(&source, &worktree, &branch).await { + Ok(()) => worktrees.push(IsolatedRepo { + source, + worktree, + branch: branch.clone(), + }), + Err(error) => { + tracing::warn!( + %error, + repo = %source.display(), + %worker_id, + "failed to isolate repo into worker worktree — skipping" + ); + } + } + } + + tracing::info!( + %worker_id, + worktrees = worktrees.len(), + root = %root.display(), + "provisioned isolated worker workspace" + ); + + Ok(Self { + shared_root: shared_root.to_path_buf(), + root, + worktrees, + }) + } + + /// Remove all worktrees and the isolated workspace directory. + /// + /// Call on **successful** completion. On failure, skip this to retain the + /// workspace for forensics — [`reap_orphaned`] bounds retention. + pub async fn release(self) -> anyhow::Result<()> { + for repo in &self.worktrees { + if let Err(error) = remove_worktree(&repo.source, &repo.worktree).await { + tracing::warn!( + %error, + worktree = %repo.worktree.display(), + "failed to remove worker worktree — leaving for reaper" + ); + } + // Best-effort: delete the per-worker branch so it doesn't accumulate. + let _ = delete_branch(&repo.source, &repo.branch).await; + } + + if let Err(error) = tokio::fs::remove_dir_all(&self.root).await + && error.kind() != std::io::ErrorKind::NotFound + { + tracing::warn!( + %error, + root = %self.root.display(), + "failed to remove isolated workspace directory" + ); + } + + // Prune any dangling worktree administrative files. + for repo in &self.worktrees { + let _ = prune_worktrees(&repo.source).await; + } + + let _ = &self.shared_root; // retained for clarity; source paths carry it. + Ok(()) + } +} + +/// The per-worker branch name used inside each isolated worktree. Namespaced so +/// it never collides with a real feature branch and is trivially greppable. +fn worker_branch_name(worker_id: WorkerId) -> String { + format!("spacebot/worker/{worker_id}") +} + +/// Discover git repository directories directly under `shared_root`. +/// +/// A repo is a child directory containing a `.git` *directory* (a real +/// checkout). Directories with a `.git` *file* are existing worktrees and are +/// skipped, as are hidden directories (including our own workspaces subdir). +async fn discover_repo_dirs(shared_root: &Path) -> Vec { + let mut repos = Vec::new(); + let mut entries = match tokio::fs::read_dir(shared_root).await { + Ok(entries) => entries, + Err(error) => { + tracing::warn!(%error, root = %shared_root.display(), "failed to scan shared workspace for repos"); + return repos; + } + }; + + while let Ok(Some(entry)) = entries.next_entry().await { + let path = entry.path(); + if !path.is_dir() { + continue; + } + let name = match path.file_name().and_then(|n| n.to_str()) { + Some(name) => name.to_string(), + None => continue, + }; + if name.starts_with('.') { + continue; + } + let dot_git = path.join(".git"); + // Only real checkouts (`.git` directory), not worktrees (`.git` file). + if dot_git.is_dir() { + repos.push(path); + } + } + + repos.sort(); + repos +} + +/// Add a git worktree for `source` at `worktree_path` on a fresh `branch` off +/// the source's current `HEAD`. +/// +/// If a worktree already exists at the path (leaked from a prior run), it is +/// removed first so provisioning is idempotent. +async fn add_worktree(source: &Path, worktree_path: &Path, branch: &str) -> anyhow::Result<()> { + let worktree_str = worktree_path + .to_str() + .ok_or_else(|| anyhow::anyhow!("worktree path is not valid UTF-8"))?; + + if worktree_path.exists() { + let _ = remove_worktree(source, worktree_path).await; + let _ = prune_worktrees(source).await; + } + // A stale branch of the same name blocks `-b`; delete it first (best effort). + let _ = delete_branch(source, branch).await; + + let output = Command::new("git") + .args([ + "worktree", + "add", + "--force", + "-b", + branch, + worktree_str, + "HEAD", + ]) + .current_dir(source) + .output() + .await?; + + if output.status.success() { + return Ok(()); + } + + let stderr = String::from_utf8_lossy(&output.stderr); + anyhow::bail!( + "git worktree add failed in {}: {}", + source.display(), + stderr.trim() + ); +} + +/// Remove a git worktree (`git worktree remove --force`). +async fn remove_worktree(source: &Path, worktree_path: &Path) -> anyhow::Result<()> { + let worktree_str = worktree_path + .to_str() + .ok_or_else(|| anyhow::anyhow!("worktree path is not valid UTF-8"))?; + + let output = Command::new("git") + .args(["worktree", "remove", "--force", worktree_str]) + .current_dir(source) + .output() + .await?; + + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + anyhow::bail!( + "git worktree remove failed in {}: {}", + source.display(), + stderr.trim() + ); + } + Ok(()) +} + +/// Prune stale worktree administrative entries (`git worktree prune`). +async fn prune_worktrees(source: &Path) -> anyhow::Result<()> { + let output = Command::new("git") + .args(["worktree", "prune"]) + .current_dir(source) + .output() + .await?; + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + anyhow::bail!("git worktree prune failed: {}", stderr.trim()); + } + Ok(()) +} + +/// Delete a local branch (`git branch -D`). Best-effort; used to keep the +/// per-worker branch namespace from accumulating. +async fn delete_branch(source: &Path, branch: &str) -> anyhow::Result<()> { + let output = Command::new("git") + .args(["branch", "-D", branch]) + .current_dir(source) + .output() + .await?; + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + anyhow::bail!("git branch -D failed: {}", stderr.trim()); + } + Ok(()) +} + +/// Reap orphaned per-worker workspaces left behind by crashes or retained +/// failures, keeping at most [`MAX_RETAINED_WORKSPACES`] most-recent ones. +/// +/// Called on startup so leaked worktrees don't accumulate unbounded. Also runs +/// `git worktree prune` on each repo so git's administrative view stays clean. +pub async fn reap_orphaned(shared_root: &Path) -> anyhow::Result { + let workspaces_dir = shared_root.join(WORKSPACES_SUBDIR); + if !workspaces_dir.exists() { + return Ok(0); + } + + // Collect (mtime, path) for each per-worker workspace directory. + let mut dirs: Vec<(std::time::SystemTime, PathBuf)> = Vec::new(); + let mut entries = tokio::fs::read_dir(&workspaces_dir).await?; + while let Ok(Some(entry)) = entries.next_entry().await { + let path = entry.path(); + if !path.is_dir() { + continue; + } + let mtime = entry + .metadata() + .await + .and_then(|m| m.modified()) + .unwrap_or(std::time::UNIX_EPOCH); + dirs.push((mtime, path)); + } + + // Newest first; retain the first MAX_RETAINED_WORKSPACES, reap the rest. + dirs.sort_by(|a, b| b.0.cmp(&a.0)); + let mut reaped = 0; + for (_, path) in dirs.into_iter().skip(MAX_RETAINED_WORKSPACES) { + if let Err(error) = tokio::fs::remove_dir_all(&path).await { + tracing::warn!(%error, path = %path.display(), "failed to reap orphaned worker workspace"); + } else { + reaped += 1; + } + } + + // Prune each source repo's worktree list so removed dirs don't linger. + for repo in discover_repo_dirs(shared_root).await { + let _ = prune_worktrees(&repo).await; + } + + if reaped > 0 { + tracing::info!(reaped, "reaped orphaned worker workspaces"); + } + Ok(reaped) +} + +#[cfg(test)] +mod tests { + use super::*; + use tokio::process::Command; + use uuid::Uuid; + + async fn git(dir: &Path, args: &[&str]) { + let out = Command::new("git") + .args(args) + .current_dir(dir) + .output() + .await + .expect("git runs"); + assert!( + out.status.success(), + "git {args:?} failed: {}", + String::from_utf8_lossy(&out.stderr) + ); + } + + /// Create a shared workspace containing one initialised repo with a commit. + async fn make_shared_workspace_with_repo(repo_name: &str) -> (tempfile::TempDir, PathBuf) { + let shared = tempfile::tempdir().expect("tempdir"); + let repo = shared.path().join(repo_name); + tokio::fs::create_dir_all(&repo).await.unwrap(); + git(&repo, &["init", "-q", "-b", "main"]).await; + git(&repo, &["config", "user.email", "t@t.io"]).await; + git(&repo, &["config", "user.name", "T"]).await; + tokio::fs::write(repo.join("README.md"), "base\n") + .await + .unwrap(); + git(&repo, &["add", "README.md"]).await; + git(&repo, &["commit", "-q", "-m", "init"]).await; + (shared, repo) + } + + #[tokio::test] + async fn provisions_isolated_worktree_per_repo() { + let (shared, _repo) = make_shared_workspace_with_repo("app").await; + let worker_id = Uuid::new_v4(); + + let ws = WorkerWorkspace::provision(shared.path(), worker_id) + .await + .expect("provision"); + + assert!(ws.has_worktrees(), "expected one isolated worktree"); + let isolated_repo = ws.root().join("app"); + assert!(isolated_repo.join(".git").exists(), "worktree checked out"); + // The isolated worktree is NOT the shared checkout. + assert_ne!(isolated_repo, shared.path().join("app")); + + ws.release().await.expect("release"); + } + + /// The core acceptance property: two concurrent workers cannot see or + /// clobber each other's edits, and never commit to the shared branch. + #[tokio::test] + async fn concurrent_workers_cannot_contaminate_each_other() { + let (shared, repo) = make_shared_workspace_with_repo("app").await; + + let worker_a = Uuid::new_v4(); + let worker_b = Uuid::new_v4(); + let ws_a = WorkerWorkspace::provision(shared.path(), worker_a) + .await + .expect("provision a"); + let ws_b = WorkerWorkspace::provision(shared.path(), worker_b) + .await + .expect("provision b"); + + let a_repo = ws_a.root().join("app"); + let b_repo = ws_b.root().join("app"); + + // Worker A writes and commits a file that ONLY it touched. + tokio::fs::write(a_repo.join("a_only.txt"), "from A\n") + .await + .unwrap(); + git(&a_repo, &["add", "a_only.txt"]).await; + git(&a_repo, &["commit", "-q", "-m", "A work"]).await; + + // Worker B, running concurrently, does a broad `git add -A` — the exact + // sweep that caused issue #224 — then commits. + tokio::fs::write(b_repo.join("b_only.txt"), "from B\n") + .await + .unwrap(); + git(&b_repo, &["add", "-A"]).await; + git(&b_repo, &["commit", "-q", "-m", "B work"]).await; + + // B's commit must NOT contain A's file (no cross-contamination). + let b_tree = Command::new("git") + .args(["show", "--name-only", "--format=", "HEAD"]) + .current_dir(&b_repo) + .output() + .await + .unwrap(); + let b_files = String::from_utf8_lossy(&b_tree.stdout); + assert!(b_files.contains("b_only.txt"), "B committed its own file"); + assert!( + !b_files.contains("a_only.txt"), + "B's `git add -A` swept up A's edit — isolation FAILED: {b_files}" + ); + + // A does not see B's file in its working tree either. + assert!( + !a_repo.join("b_only.txt").exists(), + "A sees B's file — leaked" + ); + + // The shared checkout's branch is untouched: still `main` at the base + // commit, with neither worker's file present. + assert!( + !repo.join("a_only.txt").exists(), + "shared tree got A's file" + ); + assert!( + !repo.join("b_only.txt").exists(), + "shared tree got B's file" + ); + let head = Command::new("git") + .args(["rev-parse", "--abbrev-ref", "HEAD"]) + .current_dir(&repo) + .output() + .await + .unwrap(); + assert_eq!( + String::from_utf8_lossy(&head.stdout).trim(), + "main", + "shared checkout branch changed" + ); + + ws_a.release().await.expect("release a"); + ws_b.release().await.expect("release b"); + } + + #[tokio::test] + async fn empty_workspace_yields_no_worktrees() { + let shared = tempfile::tempdir().expect("tempdir"); + let ws = WorkerWorkspace::provision(shared.path(), Uuid::new_v4()) + .await + .expect("provision"); + assert!(!ws.has_worktrees()); + ws.release().await.expect("release"); + } + + #[tokio::test] + async fn reap_orphaned_bounds_retained_workspaces() { + let (shared, _repo) = make_shared_workspace_with_repo("app").await; + // Create more than the retention bound of leftover workspaces. + let ws_dir = shared.path().join(WORKSPACES_SUBDIR); + tokio::fs::create_dir_all(&ws_dir).await.unwrap(); + for i in 0..(MAX_RETAINED_WORKSPACES + 5) { + tokio::fs::create_dir_all(ws_dir.join(format!("leftover-{i}"))) + .await + .unwrap(); + } + let reaped = reap_orphaned(shared.path()).await.expect("reap"); + assert_eq!(reaped, 5, "should reap the oldest 5 over the bound"); + } +} From dd832ddde12797f066b8eb054e4920c1ab85c01e Mon Sep 17 00:00:00 2001 From: shipyard-ci Date: Wed, 15 Jul 2026 11:03:40 +0000 Subject: [PATCH 2/5] fix(worker-workspace): address PR #25 review findings P2: - warn on silent fallback to shared workspace when isolated workspace has no worktrees (tool_workspace) [1] - document + harden orphan retention bound: symlink_metadata so planted symlinks aren't followed, clock-skew note; count bound holds regardless [2] - surface isolation-completeness: provision() logs PARTIAL vs complete and a new test asserts the partial-isolation path skips unisolable repos [3] - consistent UTF-8 validation: discover_repo_dirs skips non-UTF-8 repo names loudly instead of silently, so provision can rely on it [4] P3: - document WorkerId->branch git-validity (UUID Display is always a valid ref name) [5] - remove vestigial let _ = &self.shared_root binding (field dropped) [6] - attach_isolated_workspace error log gains Debug + shared_workspace context [7] - pr-gates SKILL.md gains a concrete BAD/GOOD git add anti-pattern example [8] --- .agents/skills/pr-gates/SKILL.md | 12 +++ src/agent/channel_dispatch.rs | 4 +- src/agent/worker.rs | 17 ++++- src/agent/worker_workspace.rs | 121 ++++++++++++++++++++++++------- 4 files changed, 125 insertions(+), 29 deletions(-) diff --git a/.agents/skills/pr-gates/SKILL.md b/.agents/skills/pr-gates/SKILL.md index 6c371520f..f5d30206b 100644 --- a/.agents/skills/pr-gates/SKILL.md +++ b/.agents/skills/pr-gates/SKILL.md @@ -42,6 +42,18 @@ commit can never contain files this worker did not touch: - If the working tree is unexpectedly dirty at handout (files you did not create), stop and investigate rather than committing over them. +Anti-pattern (what caused #211/#218/#219): + +```sh +# BAD — sweeps up whatever else is dirty in the tree, including sibling edits +git add -A && git commit -m "..." + +# GOOD — stage only what you changed, then verify before committing +git add src/foo.rs src/bar.rs +git status --porcelain # confirm every staged path is yours +git commit -m "..." +``` + ## Migration Safety - Never edit an existing file in `migrations/`. diff --git a/src/agent/channel_dispatch.rs b/src/agent/channel_dispatch.rs index 93a41ad71..9b38bd015 100644 --- a/src/agent/channel_dispatch.rs +++ b/src/agent/channel_dispatch.rs @@ -649,8 +649,10 @@ async fn attach_isolated_workspace(worker: Worker, shared_workspace: &std::path: Ok(workspace) => worker.with_isolated_workspace(workspace), Err(error) => { tracing::warn!( - %error, + error = %error, + error_debug = ?error, worker_id = %worker.id, + shared_workspace = %shared_workspace.display(), "failed to provision isolated workspace — worker will use shared workspace" ); worker diff --git a/src/agent/worker.rs b/src/agent/worker.rs index d521610a6..273f725dd 100644 --- a/src/agent/worker.rs +++ b/src/agent/worker.rs @@ -447,7 +447,18 @@ impl Worker { fn tool_workspace(&self) -> PathBuf { match &self.isolated_workspace { Some(ws) if ws.has_worktrees() => ws.root().to_path_buf(), - _ => self.deps.runtime_config.workspace_dir.clone(), + Some(_) => { + // A workspace was provisioned but isolated no repos (empty + // shared workspace, or every repo failed to isolate). Fall back + // to the shared workspace, but log so the loss of isolation is + // visible rather than silent. + tracing::warn!( + worker_id = %self.id, + "isolated workspace has no worktrees — falling back to shared workspace" + ); + self.deps.runtime_config.workspace_dir.clone() + } + None => self.deps.runtime_config.workspace_dir.clone(), } } @@ -1103,7 +1114,9 @@ impl Worker { tracing::info!(worker_id = %self.id, "worker completed"); // Successful completion: release the isolated workspace (remove its // worktrees). On error/timeout paths we deliberately skip this so the - // workspace is retained for forensics; the startup reaper bounds it. + // workspace is retained for forensics; the startup reaper bounds it to + // MAX_RETAINED_WORKSPACES (by mtime), so retained failures can't + // accumulate unbounded across restarts. if let Some(workspace) = self.isolated_workspace.take() && let Err(error) = workspace.release().await { diff --git a/src/agent/worker_workspace.rs b/src/agent/worker_workspace.rs index 2285f28a2..981a3b6bf 100644 --- a/src/agent/worker_workspace.rs +++ b/src/agent/worker_workspace.rs @@ -56,9 +56,6 @@ const MAX_RETAINED_WORKSPACES: usize = 20; /// caller controls the success-vs-forensics policy. #[derive(Debug)] pub struct WorkerWorkspace { - /// The shared workspace this isolated workspace was derived from. Needed to - /// run `git worktree remove` from each source repo on release. - shared_root: PathBuf, /// Root of the isolated workspace (`/.spacebot/worker-workspaces/`). root: PathBuf, /// The worktrees created, as `(source_repo_path, worktree_path)` pairs. @@ -109,14 +106,18 @@ impl WorkerWorkspace { })?; let repos = discover_repo_dirs(shared_root).await; + let total_repos = repos.len(); let branch = worker_branch_name(worker_id); let mut worktrees = Vec::new(); for source in repos { - let name = match source.file_name().and_then(|n| n.to_str()) { - Some(name) => name.to_string(), - None => continue, - }; + // Source paths are validated as UTF-8 at discovery, so the file name + // is always present here. + let name = source + .file_name() + .and_then(|n| n.to_str()) + .expect("discovered repo path is UTF-8") + .to_string(); let worktree = root.join(&name); match add_worktree(&source, &worktree, &branch).await { Ok(()) => worktrees.push(IsolatedRepo { @@ -135,18 +136,28 @@ impl WorkerWorkspace { } } - tracing::info!( - %worker_id, - worktrees = worktrees.len(), - root = %root.display(), - "provisioned isolated worker workspace" - ); + // Surface isolation completeness so partial provisioning is visible: a + // worker isolated for only some of its repos still shares the rest. + let isolated = worktrees.len(); + if isolated < total_repos { + tracing::warn!( + %worker_id, + isolated, + total_repos, + root = %root.display(), + "provisioned PARTIALLY isolated worker workspace — some repos remain shared" + ); + } else { + tracing::info!( + %worker_id, + isolated, + total_repos, + root = %root.display(), + "provisioned isolated worker workspace" + ); + } - Ok(Self { - shared_root: shared_root.to_path_buf(), - root, - worktrees, - }) + Ok(Self { root, worktrees }) } /// Remove all worktrees and the isolated workspace directory. @@ -181,13 +192,17 @@ impl WorkerWorkspace { let _ = prune_worktrees(&repo.source).await; } - let _ = &self.shared_root; // retained for clarity; source paths carry it. Ok(()) } } /// The per-worker branch name used inside each isolated worktree. Namespaced so /// it never collides with a real feature branch and is trivially greppable. +/// +/// The name is `spacebot/worker/`. Both segments are always git-valid ref +/// names: the literal prefix is fixed, and [`WorkerId`] is a UUID whose +/// `Display` is hyphen-separated lowercase hex — never containing any of git's +/// forbidden ref characters (space, `~^:?*[\`, `..`, etc.). fn worker_branch_name(worker_id: WorkerId) -> String { format!("spacebot/worker/{worker_id}") } @@ -214,7 +229,16 @@ async fn discover_repo_dirs(shared_root: &Path) -> Vec { } let name = match path.file_name().and_then(|n| n.to_str()) { Some(name) => name.to_string(), - None => continue, + None => { + // Non-UTF-8 repo directory names can't be turned into valid + // worktree paths for git; skip loudly rather than surfacing an + // opaque git error later. + tracing::warn!( + path = %path.display(), + "skipping repo with non-UTF-8 directory name" + ); + continue; + } }; if name.starts_with('.') { continue; @@ -330,6 +354,12 @@ async fn delete_branch(source: &Path, branch: &str) -> anyhow::Result<()> { /// /// Called on startup so leaked worktrees don't accumulate unbounded. Also runs /// `git worktree prune` on each repo so git's administrative view stays clean. +/// +/// Ordering uses directory mtime purely to pick *which* to keep; the count +/// bound holds regardless of clock skew, so a misbehaving clock can only affect +/// which recent workspaces survive, never whether the bound is enforced. +/// Symlinked entries are never followed (see below), so a planted symlink can't +/// redirect a delete outside the workspaces directory. pub async fn reap_orphaned(shared_root: &Path) -> anyhow::Result { let workspaces_dir = shared_root.join(WORKSPACES_SUBDIR); if !workspaces_dir.exists() { @@ -341,14 +371,19 @@ pub async fn reap_orphaned(shared_root: &Path) -> anyhow::Result { let mut entries = tokio::fs::read_dir(&workspaces_dir).await?; while let Ok(Some(entry)) = entries.next_entry().await { let path = entry.path(); - if !path.is_dir() { + // Use symlink_metadata so a symlink is never followed: we only ever + // reap real directories we created, never traverse a planted symlink + // out of the workspaces dir. + let meta = match tokio::fs::symlink_metadata(&path).await { + Ok(meta) => meta, + Err(_) => continue, + }; + if !meta.is_dir() { + // Skips regular files and symlinks (symlink_metadata reports the + // link itself, whose file type is symlink, not dir). continue; } - let mtime = entry - .metadata() - .await - .and_then(|m| m.modified()) - .unwrap_or(std::time::UNIX_EPOCH); + let mtime = meta.modified().unwrap_or(std::time::UNIX_EPOCH); dirs.push((mtime, path)); } @@ -517,6 +552,40 @@ mod tests { ws.release().await.expect("release"); } + /// A repo that cannot be isolated (its `.git` is present but not a valid + /// repository) is skipped, and the workspace reports it isolated fewer + /// repos than were discovered — the partial-isolation signal from #224. + #[tokio::test] + async fn partial_isolation_skips_unisolable_repo() { + let (shared, _good) = make_shared_workspace_with_repo("app").await; + + // A second directory that looks like a repo (has a `.git` directory) + // but is corrupt, so `git worktree add` fails for it. + let broken = shared.path().join("broken"); + tokio::fs::create_dir_all(broken.join(".git")) + .await + .unwrap(); + + let ws = WorkerWorkspace::provision(shared.path(), Uuid::new_v4()) + .await + .expect("provision"); + + // The good repo is isolated; the broken one is skipped, not fatal. + assert!(ws.has_worktrees(), "the healthy repo is still isolated"); + assert_eq!( + ws.worktrees.len(), + 1, + "only the healthy repo isolated; broken repo skipped" + ); + assert!(ws.root().join("app").join(".git").exists()); + assert!( + !ws.root().join("broken").exists(), + "the unisolable repo left no worktree" + ); + + ws.release().await.expect("release"); + } + #[tokio::test] async fn reap_orphaned_bounds_retained_workspaces() { let (shared, _repo) = make_shared_workspace_with_repo("app").await; From 0fbf3a93d761055e635d161b67d41a03bed3386a Mon Sep 17 00:00:00 2001 From: shipyard-ci Date: Wed, 15 Jul 2026 11:38:35 +0000 Subject: [PATCH 3/5] fix(worker-isolation): fail loud, not silent, on incomplete isolation (fp:71617f0ad28f) tool_workspace() no longer silently degrades to the shared checkout when an isolated workspace was provisioned but has no worktrees. It now consults the workspace's Isolation completeness signal: the empty-because-nothing-to- isolate case falls back cleanly, but repos-present-yet-none-isolated trips a debug_assert! in debug builds and logs at error (loud, greppable) in release, documenting why the degradation is non-recoverable (re-opens issue #224). --- src/agent/worker.rs | 53 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 45 insertions(+), 8 deletions(-) diff --git a/src/agent/worker.rs b/src/agent/worker.rs index 273f725dd..8235e5f77 100644 --- a/src/agent/worker.rs +++ b/src/agent/worker.rs @@ -444,18 +444,55 @@ impl Worker { /// The base directory the worker's tools should use: the isolated /// workspace when one was provisioned, otherwise the shared workspace. + /// + /// # Non-recoverable degradation guard (issue #224, finding [1]) + /// + /// When an isolated workspace was provisioned we must not *silently* fall + /// back to the shared checkout — that would re-introduce the exact + /// cross-worker contamination race #224 exists to prevent. We consult the + /// workspace's [`Isolation`](crate::agent::worker_workspace::Isolation) + /// completeness signal (finding [3]) to tell apart: + /// - **complete isolation** (every repo isolated, or the degenerate + /// no-repo case): safe to use the isolated root, or safe to fall back to + /// the shared workspace when the isolated root is empty because there was + /// genuinely nothing to isolate. + /// - **incomplete isolation** (repos existed but some/all failed): a + /// `debug_assert!` trips in debug builds because this is a programmer- or + /// environment-level fault the provisioning path should have surfaced; + /// in release builds we log at `error` (not `warn`) so the loss of + /// isolation is loud and greppable rather than silent, then fall back. fn tool_workspace(&self) -> PathBuf { match &self.isolated_workspace { Some(ws) if ws.has_worktrees() => ws.root().to_path_buf(), - Some(_) => { - // A workspace was provisioned but isolated no repos (empty - // shared workspace, or every repo failed to isolate). Fall back - // to the shared workspace, but log so the loss of isolation is - // visible rather than silent. - tracing::warn!( - worker_id = %self.id, - "isolated workspace has no worktrees — falling back to shared workspace" + Some(ws) => { + // A workspace was provisioned but isolated no repos. This is + // only safe when there was genuinely nothing to isolate + // (`Isolation::Fully` over zero repos). If repos existed and + // none isolated, falling back to the shared workspace silently + // degrades — treat that as non-recoverable in debug and loud in + // release. See the guard doc above. + let isolation = ws.isolation(); + debug_assert!( + isolation.is_complete(), + "isolated workspace provisioned with no worktrees despite \ + repos being present ({isolation:?}) — silent fallback to the \ + shared checkout would re-open issue #224" ); + if !isolation.is_complete() { + tracing::error!( + worker_id = %self.id, + ?isolation, + "isolated workspace has no worktrees but repos were present — \ + isolation FAILED; falling back to shared workspace (this can \ + re-open the #224 contamination race)" + ); + } else { + tracing::warn!( + worker_id = %self.id, + "isolated workspace has no worktrees (nothing to isolate) — \ + falling back to shared workspace" + ); + } self.deps.runtime_config.workspace_dir.clone() } None => self.deps.runtime_config.workspace_dir.clone(), From b7690fd4ab60be238bfa8878ac1a1bb990ec9602 Mon Sep 17 00:00:00 2001 From: shipyard-ci Date: Wed, 15 Jul 2026 11:39:21 +0000 Subject: [PATCH 4/5] fix(worker-isolation): document and make UTF-8 path validation consistent (fp:3a69047329f9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Source repo paths and worktree paths were validated at different layers with no stated contract. Document the intentional asymmetry: git consumes the worktree path as a string arg (whole path must be UTF-8, enforced in add_worktree), while the source path is only passed via current_dir (an OsStr, no UTF-8 requirement — only its leaf name is validated, in discover_repo_dirs). Replace the provision-loop expect() on the (guaranteed) UTF-8 name with the same skip-and-log degradation used elsewhere, so a path-encoding regression can never panic a worker. --- src/agent/worker_workspace.rs | 43 ++++++++++++++++++++++++++++------- 1 file changed, 35 insertions(+), 8 deletions(-) diff --git a/src/agent/worker_workspace.rs b/src/agent/worker_workspace.rs index 981a3b6bf..e166869a1 100644 --- a/src/agent/worker_workspace.rs +++ b/src/agent/worker_workspace.rs @@ -111,14 +111,35 @@ impl WorkerWorkspace { let mut worktrees = Vec::new(); for source in repos { - // Source paths are validated as UTF-8 at discovery, so the file name - // is always present here. - let name = source - .file_name() - .and_then(|n| n.to_str()) - .expect("discovered repo path is UTF-8") - .to_string(); - let worktree = root.join(&name); + // UTF-8 contract (issue #224, finding [4]): + // + // Git consumes the *worktree* path as a string argument (see + // `add_worktree`/`remove_worktree`, which reject a non-UTF-8 path), + // whereas the *source* repo path is only ever passed via + // `Command::current_dir`, which takes an `OsStr` and so needs no + // UTF-8 guarantee. The two are therefore validated at different + // layers on purpose: + // - source: only its leaf file name must be UTF-8, and that is + // enforced up front in `discover_repo_dirs` (non-UTF-8 names are + // skipped there), so `to_str()` on the name cannot fail here. + // - worktree: the whole path must be UTF-8, enforced inside + // `add_worktree`. If `shared_root` itself is non-UTF-8 the join + // below is non-UTF-8 too, `add_worktree` returns an error, and + // the repo is skipped like any other isolation failure. + // + // Consistency: rather than `expect()`-ing the (guaranteed) UTF-8 + // name — which would panic if that invariant ever regressed — we + // degrade to the same skip-and-log path used everywhere else, so a + // path-encoding surprise can never take down the worker. + let Some(name) = source.file_name().and_then(|n| n.to_str()) else { + tracing::warn!( + repo = %source.display(), + %worker_id, + "discovered repo has a non-UTF-8 name — skipping (should be unreachable: discovery filters these)" + ); + continue; + }; + let worktree = root.join(name); match add_worktree(&source, &worktree, &branch).await { Ok(()) => worktrees.push(IsolatedRepo { source, @@ -260,6 +281,12 @@ async fn discover_repo_dirs(shared_root: &Path) -> Vec { /// If a worktree already exists at the path (leaked from a prior run), it is /// removed first so provisioning is idempotent. async fn add_worktree(source: &Path, worktree_path: &Path, branch: &str) -> anyhow::Result<()> { + // UTF-8 contract (issue #224, finding [4]): git needs the worktree path as + // a string argument, so the *whole* path must be UTF-8 here — this is the + // counterpart to `discover_repo_dirs`, which validates only the source + // repo's leaf name (the source is passed via `current_dir`, an `OsStr`, and + // needs no UTF-8 guarantee). A non-UTF-8 path (e.g. a non-UTF-8 + // `shared_root`) surfaces as this recoverable error and the repo is skipped. let worktree_str = worktree_path .to_str() .ok_or_else(|| anyhow::anyhow!("worktree path is not valid UTF-8"))?; From f4ac1946f4fe97043c95e303874f487c16279a40 Mon Sep 17 00:00:00 2001 From: shipyard-ci Date: Wed, 15 Jul 2026 11:39:43 +0000 Subject: [PATCH 5/5] fix(worker-isolation): give provision() an explicit completeness signal (fp:b51270ffe407) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit provision() returned a WorkerWorkspace with no way to tell whether isolation actually succeeded for every repo — per-repo failures are non-fatal, so Ok(_) did not imply full isolation. Add an Isolation enum (Fully / Partial / None) computed from isolated-vs-discovered repo counts, store it on the workspace, and expose it via WorkerWorkspace::isolation(). Document the completeness contract on provision() and in channel_dispatch::attach_isolated_workspace(). tool_workspace() (fp:71617f0ad28f) consumes this signal to refuse silent degradation. Covers the signal in the full/empty/partial provisioning tests. --- src/agent/channel_dispatch.rs | 13 ++++ src/agent/worker_workspace.rs | 109 +++++++++++++++++++++++++++++++++- 2 files changed, 121 insertions(+), 1 deletion(-) diff --git a/src/agent/channel_dispatch.rs b/src/agent/channel_dispatch.rs index 9b38bd015..fc222c68d 100644 --- a/src/agent/channel_dispatch.rs +++ b/src/agent/channel_dispatch.rs @@ -642,6 +642,19 @@ pub async fn spawn_worker_from_state( /// concurrent workers can't see or clobber each other's edits. Provisioning is /// best-effort: on failure the worker is returned unchanged and falls back to /// the shared workspace, so a provisioning hiccup never blocks dispatch. +/// +/// # Completeness contract (issue #224, finding [3]) +/// +/// A successful `provision(..)` does **not** guarantee every repo was isolated — +/// per-repo failures are logged and skipped rather than made fatal. The returned +/// [`WorkerWorkspace`](crate::agent::worker_workspace::WorkerWorkspace) carries +/// an explicit [`Isolation`](crate::agent::worker_workspace::Isolation) signal +/// (read via `.isolation()`) recording whether isolation was *complete*, +/// *partial*, or *absent*. We attach the workspace regardless so the worker +/// still benefits from whatever isolation was achieved; the worker's +/// `tool_workspace` selection then consults that signal and refuses to +/// *silently* fall back to the shared checkout when isolation was incomplete +/// (finding [1]). async fn attach_isolated_workspace(worker: Worker, shared_workspace: &std::path::Path) -> Worker { match crate::agent::worker_workspace::WorkerWorkspace::provision(shared_workspace, worker.id) .await diff --git a/src/agent/worker_workspace.rs b/src/agent/worker_workspace.rs index e166869a1..17ab4d028 100644 --- a/src/agent/worker_workspace.rs +++ b/src/agent/worker_workspace.rs @@ -60,6 +60,67 @@ pub struct WorkerWorkspace { root: PathBuf, /// The worktrees created, as `(source_repo_path, worktree_path)` pairs. worktrees: Vec, + /// Completeness of isolation: whether every discovered repo was isolated. + /// Callers consult this (via [`Self::isolation`]) to decide whether it is + /// safe to use the workspace or whether isolation silently degraded — see + /// [`Isolation`] and finding [1]/[3] of issue #224. + isolation: Isolation, +} + +/// The result of provisioning a worker workspace, carrying an explicit +/// completeness signal alongside the workspace itself. +/// +/// # Why the caller needs this (issue #224, finding [3]) +/// +/// [`WorkerWorkspace::provision`] never fails just because a repo could not be +/// isolated — partial isolation is strictly better than the shared checkout, so +/// per-repo failures are logged and skipped. But that means the returned +/// [`WorkerWorkspace`] alone cannot tell the caller *whether isolation was +/// actually achieved for every repo that existed*. Without that signal a caller +/// cannot distinguish three materially different situations: +/// +/// - [`Isolation::Fully`] — every discovered repo got its own worktree, or there +/// were no repos at all (the degenerate single-worker case). Safe. +/// - [`Isolation::Partial`] — repos existed and at least one, but not all, were +/// isolated. The un-isolated repos are still shared: a worker touching them +/// can contaminate a sibling. +/// - [`Isolation::None`] — repos existed but none could be isolated. Falling +/// back to the shared workspace here silently re-introduces the exact race +/// #224 set out to eliminate. +/// +/// The completeness contract lets `Worker::tool_workspace` refuse to silently +/// degrade (finding [1]) instead of transparently sharing the checkout. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Isolation { + /// Every discovered repo was isolated (or there were no repos to isolate). + Fully, + /// Some but not all discovered repos were isolated; the rest remain shared. + Partial { isolated: usize, total: usize }, + /// Repos were discovered but none could be isolated. + None { total: usize }, +} + +impl Isolation { + /// Classify the provisioning outcome from the counts of isolated vs + /// discovered repos. + fn classify(isolated: usize, total: usize) -> Self { + match (isolated, total) { + (_, 0) => Isolation::Fully, + (i, t) if i == t => Isolation::Fully, + (0, t) => Isolation::None { total: t }, + (i, t) => Isolation::Partial { + isolated: i, + total: t, + }, + } + } + + /// Whether the workspace is safe to use as-is without re-introducing the + /// #224 cross-worker contamination race. Only [`Isolation::Fully`] is safe; + /// any partial or missing isolation leaves some repo shared. + pub fn is_complete(self) -> bool { + matches!(self, Isolation::Fully) + } } /// One repo mirrored into an isolated workspace as a git worktree. @@ -86,6 +147,18 @@ impl WorkerWorkspace { !self.worktrees.is_empty() } + /// The completeness of this workspace's isolation. + /// + /// This is the completeness signal from finding [3] of issue #224: unlike + /// [`Self::has_worktrees`] (which only says whether *some* worktree exists), + /// this distinguishes full isolation from partial isolation from a total + /// failure to isolate any of the repos that were present. Callers use it to + /// refuse to silently degrade to the shared checkout when isolation was + /// requested but did not fully succeed. See [`Isolation`]. + pub fn isolation(&self) -> Isolation { + self.isolation + } + /// Provision an isolated workspace for `worker_id` off `shared_root`. /// /// Discovers git repos directly under `shared_root` and adds a per-worker @@ -97,6 +170,14 @@ impl WorkerWorkspace { /// workspace root). A repo that fails to isolate is logged and skipped /// rather than failing the whole worker — partial isolation is strictly /// better than the shared checkout. + /// + /// # Completeness contract (issue #224, finding [3]) + /// + /// Because per-repo failures are non-fatal, a successful `Ok(_)` does **not** + /// imply every repo is isolated. The returned workspace carries an explicit + /// [`Isolation`] signal (read via [`Self::isolation`]) so the caller can tell + /// full isolation from partial isolation from a total failure and decide + /// whether it is safe to proceed rather than silently sharing the checkout. pub async fn provision(shared_root: &Path, worker_id: WorkerId) -> anyhow::Result { let root = shared_root .join(WORKSPACES_SUBDIR) @@ -160,6 +241,7 @@ impl WorkerWorkspace { // Surface isolation completeness so partial provisioning is visible: a // worker isolated for only some of its repos still shares the rest. let isolated = worktrees.len(); + let isolation = Isolation::classify(isolated, total_repos); if isolated < total_repos { tracing::warn!( %worker_id, @@ -178,7 +260,11 @@ impl WorkerWorkspace { ); } - Ok(Self { root, worktrees }) + Ok(Self { + root, + worktrees, + isolation, + }) } /// Remove all worktrees and the isolated workspace directory. @@ -482,6 +568,12 @@ mod tests { .expect("provision"); assert!(ws.has_worktrees(), "expected one isolated worktree"); + assert_eq!( + ws.isolation(), + Isolation::Fully, + "the sole repo isolated — fully complete" + ); + assert!(ws.isolation().is_complete()); let isolated_repo = ws.root().join("app"); assert!(isolated_repo.join(".git").exists(), "worktree checked out"); // The isolated worktree is NOT the shared checkout. @@ -576,6 +668,10 @@ mod tests { .await .expect("provision"); assert!(!ws.has_worktrees()); + // No repos to isolate is the safe degenerate case: still "fully" + // isolated (nothing was left shared), so callers may fall back cleanly. + assert_eq!(ws.isolation(), Isolation::Fully); + assert!(ws.isolation().is_complete()); ws.release().await.expect("release"); } @@ -604,6 +700,17 @@ mod tests { 1, "only the healthy repo isolated; broken repo skipped" ); + // The completeness signal (#224 finding [3]) reports partial isolation: + // one of two discovered repos isolated. `is_complete()` is false so a + // caller must not silently treat this as fully isolated. + assert_eq!( + ws.isolation(), + Isolation::Partial { + isolated: 1, + total: 2 + }, + ); + assert!(!ws.isolation().is_complete()); assert!(ws.root().join("app").join(".git").exists()); assert!( !ws.root().join("broken").exists(),