From 3d57bb2857fc52c309398abc78bd2207b841d135 Mon Sep 17 00:00:00 2001 From: oxoxDev <164490987+oxoxDev@users.noreply.github.com> Date: Fri, 12 Jun 2026 01:25:44 +0530 Subject: [PATCH] feat(worktree): git worktree isolation for parallel coding agents (#3376 PR1) (#3601) --- app/src/store/chatRuntimeSlice.ts | 19 + src/openhuman/agent/harness/mod.rs | 2 + .../harness/subagent_runner/ops/runner.rs | 35 +- .../harness/subagent_runner/ops_tests.rs | 2 + .../agent/harness/subagent_runner/types.rs | 10 + .../agent/harness/worktree_context.rs | 74 +++ .../agent/tools/delegate_to_personality.rs | 1 + src/openhuman/agent_orchestration/mod.rs | 2 + src/openhuman/agent_orchestration/ops.rs | 1 + .../tools/continue_subagent.rs | 1 + .../agent_orchestration/tools/dispatch.rs | 1 + .../tools/spawn_async_subagent.rs | 1 + .../tools/spawn_parallel_agents.rs | 256 +++++++++- .../tools/spawn_parallel_agents_tests.rs | 79 ++++ .../tools/spawn_subagent.rs | 1 + .../tools/spawn_worker_thread.rs | 1 + src/openhuman/agent_orchestration/worktree.rs | 445 ++++++++++++++++++ .../agent_orchestration/worktree_tests.rs | 235 +++++++++ .../tools/impl/filesystem/git_operations.rs | 22 +- src/openhuman/tools/impl/system/shell.rs | 16 +- 20 files changed, 1183 insertions(+), 21 deletions(-) create mode 100644 src/openhuman/agent/harness/worktree_context.rs create mode 100644 src/openhuman/agent_orchestration/worktree.rs create mode 100644 src/openhuman/agent_orchestration/worktree_tests.rs diff --git a/app/src/store/chatRuntimeSlice.ts b/app/src/store/chatRuntimeSlice.ts index c0cf5851d..d83f7397f 100644 --- a/app/src/store/chatRuntimeSlice.ts +++ b/app/src/store/chatRuntimeSlice.ts @@ -91,6 +91,25 @@ export interface SubagentActivity { * tool sequence. Absent on legacy/test rows that predate streaming. */ transcript?: SubagentTranscriptItem[]; + /** + * Absolute path to this worker's isolated `git worktree` checkout, when it + * ran with `isolation = "worktree"` (#3376). `undefined` for non-isolated + * (read-only or shared-workspace) workers. Scaffold-only: the open/diff/ + * remove action buttons that consume this land in a follow-up PR. + */ + worktreePath?: string; + /** + * Files (relative to the worktree root) this worker changed, collected from + * `git status` after the run. Drives the future diff/overlap UI. Absent or + * empty for non-isolated workers and clean worktrees. + */ + changedFiles?: string[]; + /** + * `true` when the worker's worktree had uncommitted changes after the run. + * A dirty worktree must not be auto-removed — the cleanup UI will require an + * explicit user choice. `undefined` for non-isolated workers. + */ + isDirty?: boolean; } /** diff --git a/src/openhuman/agent/harness/mod.rs b/src/openhuman/agent/harness/mod.rs index 379eb189c..a3b4e64f6 100644 --- a/src/openhuman/agent/harness/mod.rs +++ b/src/openhuman/agent/harness/mod.rs @@ -44,6 +44,7 @@ mod token_budget; pub(crate) mod tool_filter; mod tool_loop; pub(crate) mod tool_result_artifacts; +pub mod worktree_context; pub use definition::{ AgentDefinition, AgentDefinitionRegistry, DefinitionSource, ModelSpec, PromptSource, @@ -55,6 +56,7 @@ pub use model_vision_context::{current_model_vision, with_current_model_vision}; pub use sandbox_context::{current_sandbox_mode, with_current_sandbox_mode}; pub(crate) use spawn_depth_context::{current_spawn_depth, with_spawn_depth, MAX_SPAWN_DEPTH}; pub use subagent_runner::{run_subagent, SubagentRunError, SubagentRunOptions}; +pub use worktree_context::{current_action_dir_override, with_action_dir_override}; pub(crate) use instructions::build_tool_instructions_filtered; pub(crate) use parse::parse_tool_calls; diff --git a/src/openhuman/agent/harness/subagent_runner/ops/runner.rs b/src/openhuman/agent/harness/subagent_runner/ops/runner.rs index 72ca9401b..8bafc5fb6 100644 --- a/src/openhuman/agent/harness/subagent_runner/ops/runner.rs +++ b/src/openhuman/agent/harness/subagent_runner/ops/runner.rs @@ -102,17 +102,36 @@ pub async fn run_subagent( // Install the sub-agent's declared `sandbox_mode` as the active // task-local for every tool invocation inside this run. + // + // When the worker opted into git-worktree isolation, also install + // its isolated checkout path as the `action_dir` override so acting + // tools (shell, git) operate inside that worktree instead of the + // shared `Config.action_dir`. When `worktree_action_dir` is `None` + // (the default / non-isolated path), no override scope is entered and + // behaviour is unchanged. + let worktree_action_dir = options.worktree_action_dir.clone(); + if let Some(ref wt_dir) = worktree_action_dir { + tracing::debug!( + agent_id = %definition.id, + task_id = %task_id, + worktree = %wt_dir.display(), + "[subagent_runner] installing worktree action_dir override" + ); + } let mut outcome = with_spawn_depth(attempted_depth, async { with_file_state_agent_id(task_id.clone(), async { with_current_sandbox_mode(definition.sandbox_mode, async { - Box::pin(run_typed_mode( - definition, - task_prompt, - &options, - &parent, - &task_id, - )) - .await + let run = run_typed_mode(definition, task_prompt, &options, &parent, &task_id); + match worktree_action_dir { + Some(wt_dir) => { + crate::openhuman::agent::harness::with_action_dir_override( + wt_dir, + Box::pin(run), + ) + .await + } + None => Box::pin(run).await, + } }) .await }) diff --git a/src/openhuman/agent/harness/subagent_runner/ops_tests.rs b/src/openhuman/agent/harness/subagent_runner/ops_tests.rs index 9db62a92e..d6a8d6322 100644 --- a/src/openhuman/agent/harness/subagent_runner/ops_tests.rs +++ b/src/openhuman/agent/harness/subagent_runner/ops_tests.rs @@ -479,6 +479,7 @@ async fn typed_mode_returns_text_through_runner() { worker_thread_id: None, initial_history: None, checkpoint_dir: None, + worktree_action_dir: None, }, ) .await @@ -590,6 +591,7 @@ async fn typed_mode_filters_tools_by_skill_filter() { worker_thread_id: None, initial_history: None, checkpoint_dir: None, + worktree_action_dir: None, }, ) .await diff --git a/src/openhuman/agent/harness/subagent_runner/types.rs b/src/openhuman/agent/harness/subagent_runner/types.rs index 71bbc2bbc..267701978 100644 --- a/src/openhuman/agent/harness/subagent_runner/types.rs +++ b/src/openhuman/agent/harness/subagent_runner/types.rs @@ -56,6 +56,16 @@ pub struct SubagentRunOptions { /// sub-agent pauses for user input. Defaults to /// `{workspace_dir}/.openhuman/subagent_checkpoints/`. pub checkpoint_dir: Option, + + /// Per-worker `action_dir` override for git-worktree isolation. + /// + /// When `Some`, the runner installs this path as the + /// `current_action_dir_override` task-local around the inner tool-call + /// loop, so acting tools (shell, git) operate inside the worker's + /// isolated worktree checkout instead of the shared `Config.action_dir`. + /// When `None` (the default), behaviour is unchanged — tools fall through + /// to `security.action_dir`. + pub worktree_action_dir: Option, } /// Terminal status of a sub-agent run. diff --git a/src/openhuman/agent/harness/worktree_context.rs b/src/openhuman/agent/harness/worktree_context.rs new file mode 100644 index 000000000..1bfeafacf --- /dev/null +++ b/src/openhuman/agent/harness/worktree_context.rs @@ -0,0 +1,74 @@ +//! Task-local carrier for a **per-worker `action_dir` override**. +//! +//! Sibling of [`super::sandbox_context`] and [`super::fork_context`]. When an +//! edit-capable worker opts into git-worktree isolation +//! (`isolation = "worktree"`), the subagent runner installs the worktree's +//! checkout path here for the duration of that worker's run. Acting tools +//! (shell, git) read it via [`current_action_dir_override`] and, when present, +//! redirect their working directory to the isolated worktree instead of the +//! shared `Config.action_dir`. +//! +//! Why a task-local instead of rebuilding every tool with a new `action_dir`: +//! the subagent runner reuses the parent's already-constructed tool instances +//! (`parent.all_tools`), each of which captured `security.action_dir` at build +//! time. Threading a fresh `action_dir` into those instances would mean +//! reconstructing the entire tool set per worker. A task-local keeps the change +//! additive and scoped to exactly the worker turn that needs it — and the scope +//! does not leak into detached tasks (standard [`tokio::task_local!`] +//! semantics). When unset, tools fall through to `security.action_dir`, so the +//! non-isolated path is byte-for-byte unchanged. + +use std::path::PathBuf; + +tokio::task_local! { + /// Absolute path to the isolated worktree checkout for the currently + /// running worker. `None`-equivalent: the scope is simply not active. + pub static CURRENT_ACTION_DIR_OVERRIDE: PathBuf; +} + +/// Returns the active per-worker `action_dir` override, if one is installed. +/// +/// Returns `None` when called outside [`with_action_dir_override`] — e.g. the +/// non-isolated parallel path, the main agent turn, CLI / JSON-RPC tool +/// dispatch, or unit tests that invoke a [`crate::openhuman::tools::Tool`] +/// directly. +pub fn current_action_dir_override() -> Option { + CURRENT_ACTION_DIR_OVERRIDE.try_with(|p| p.clone()).ok() +} + +/// Run `future` with `action_dir` installed as the worker's action-dir +/// override. Intended call site is the subagent runner, wrapping the inner +/// tool-call loop for a worktree-isolated worker. +pub async fn with_action_dir_override(action_dir: PathBuf, future: F) -> R +where + F: std::future::Future, +{ + CURRENT_ACTION_DIR_OVERRIDE.scope(action_dir, future).await +} + +#[cfg(test)] +mod tests { + use super::*; + + #[tokio::test] + async fn override_absent_outside_scope() { + assert_eq!(current_action_dir_override(), None); + } + + #[tokio::test] + async fn override_visible_inside_scope() { + let dir = PathBuf::from("/tmp/worker-xyz"); + let seen = + with_action_dir_override(dir.clone(), async { current_action_dir_override() }).await; + assert_eq!(seen, Some(dir)); + } + + #[tokio::test] + async fn override_does_not_leak() { + with_action_dir_override(PathBuf::from("/tmp/a"), async { + assert_eq!(current_action_dir_override(), Some(PathBuf::from("/tmp/a"))); + }) + .await; + assert_eq!(current_action_dir_override(), None); + } +} diff --git a/src/openhuman/agent/tools/delegate_to_personality.rs b/src/openhuman/agent/tools/delegate_to_personality.rs index 5916e2fc1..84ede24f7 100644 --- a/src/openhuman/agent/tools/delegate_to_personality.rs +++ b/src/openhuman/agent/tools/delegate_to_personality.rs @@ -217,6 +217,7 @@ impl Tool for DelegateToPersonalityTool { worker_thread_id: None, initial_history: None, checkpoint_dir: None, + worktree_action_dir: None, }; match run_subagent(&definition, &prompt, options).await { diff --git a/src/openhuman/agent_orchestration/mod.rs b/src/openhuman/agent_orchestration/mod.rs index d83f55976..208cf922a 100644 --- a/src/openhuman/agent_orchestration/mod.rs +++ b/src/openhuman/agent_orchestration/mod.rs @@ -11,6 +11,7 @@ mod ops; pub mod tools; pub mod types; pub mod workflow_runs; +pub mod worktree; #[cfg(test)] mod ops_tests; @@ -28,3 +29,4 @@ pub use types::{ pub use workflow_runs::{ all_workflow_run_controller_schemas, all_workflow_run_registered_controllers, }; +pub use worktree::{BaseRef, WorktreeError, WorktreeStatus}; diff --git a/src/openhuman/agent_orchestration/ops.rs b/src/openhuman/agent_orchestration/ops.rs index 5ce97ba9d..83e68e26e 100644 --- a/src/openhuman/agent_orchestration/ops.rs +++ b/src/openhuman/agent_orchestration/ops.rs @@ -464,6 +464,7 @@ impl AgentOrchestrationSession { worker_thread_id: None, initial_history: None, checkpoint_dir: None, + worktree_action_dir: None, }; let task_session = self.clone(); diff --git a/src/openhuman/agent_orchestration/tools/continue_subagent.rs b/src/openhuman/agent_orchestration/tools/continue_subagent.rs index 62d1b4ea3..9aa42be8e 100644 --- a/src/openhuman/agent_orchestration/tools/continue_subagent.rs +++ b/src/openhuman/agent_orchestration/tools/continue_subagent.rs @@ -228,6 +228,7 @@ impl Tool for ContinueSubagentTool { worker_thread_id: checkpoint.worker_thread_id.clone(), initial_history: Some(history), checkpoint_dir: Some(checkpoint_dir.clone()), + worktree_action_dir: None, }; // Run the sub-agent from its checkpoint diff --git a/src/openhuman/agent_orchestration/tools/dispatch.rs b/src/openhuman/agent_orchestration/tools/dispatch.rs index 458194bcb..1179dcb6d 100644 --- a/src/openhuman/agent_orchestration/tools/dispatch.rs +++ b/src/openhuman/agent_orchestration/tools/dispatch.rs @@ -108,6 +108,7 @@ pub(crate) async fn dispatch_subagent( worker_thread_id: None, initial_history: None, checkpoint_dir: None, + worktree_action_dir: None, }; match run_subagent(definition, prompt, options).await { diff --git a/src/openhuman/agent_orchestration/tools/spawn_async_subagent.rs b/src/openhuman/agent_orchestration/tools/spawn_async_subagent.rs index cfffb1dfa..60a3d94dc 100644 --- a/src/openhuman/agent_orchestration/tools/spawn_async_subagent.rs +++ b/src/openhuman/agent_orchestration/tools/spawn_async_subagent.rs @@ -244,6 +244,7 @@ impl Tool for SpawnAsyncSubagentTool { worker_thread_id: background_worker_thread_id.clone(), initial_history: None, checkpoint_dir: None, + worktree_action_dir: None, }; let result = with_parent_context(background_parent, async move { diff --git a/src/openhuman/agent_orchestration/tools/spawn_parallel_agents.rs b/src/openhuman/agent_orchestration/tools/spawn_parallel_agents.rs index 02529fdf5..fcf262fe1 100644 --- a/src/openhuman/agent_orchestration/tools/spawn_parallel_agents.rs +++ b/src/openhuman/agent_orchestration/tools/spawn_parallel_agents.rs @@ -36,6 +36,19 @@ struct ParallelAgentTask { toolkit: Option, #[serde(default)] ownership: Option, + /// File-isolation strategy for this worker: `"none"` (default — share the + /// parent's `action_dir`) or `"worktree"` (run inside a dedicated + /// `git worktree` checkout). Read-only workers should stay `"none"`; + /// edit-capable workers opt into `"worktree"` explicitly. We never + /// auto-promote a worker to worktree isolation without this flag — the + /// approval UX for auto-isolation lands in a later PR. + #[serde(default)] + isolation: Option, + /// When `isolation = "worktree"`, which ref the worktree branches from: + /// `"head"` (default — continue the parent's in-progress state) or + /// `"fresh"` (start from the repo's default branch). + #[serde(default)] + base_ref: Option, } #[derive(Debug, Serialize)] @@ -54,6 +67,20 @@ struct ParallelAgentResult { iterations: u32, #[serde(skip_serializing_if = "Vec::is_empty")] stale_parent_reads: Vec, + /// Absolute path to the worker's isolated `git worktree` checkout, when + /// it ran with `isolation = "worktree"`. `None` for non-isolated workers. + #[serde(skip_serializing_if = "Option::is_none")] + worktree_path: Option, + /// Files (relative to the worktree root) the worker changed, collected + /// from `git status` after the run. Empty for non-isolated workers or a + /// clean worktree. + #[serde(skip_serializing_if = "Vec::is_empty")] + changed_files: Vec, + /// Whether the worker's worktree had uncommitted changes after the run. + /// A dirty worktree must not be auto-removed (surfaced to the UI so the + /// user can choose). `None` for non-isolated workers. + #[serde(skip_serializing_if = "Option::is_none")] + dirty_status: Option, } #[async_trait] @@ -96,6 +123,16 @@ impl Tool for SpawnParallelAgentsTool { "ownership": { "type": "string", "description": "Disjoint file/module/responsibility boundary for this worker." + }, + "isolation": { + "type": "string", + "enum": ["none", "worktree"], + "description": "File-isolation strategy. `none` (default) shares the workspace; `worktree` gives this edit-capable worker its own git worktree checkout so parallel edits never collide. Use `worktree` only for edit-capable coding workers, not read-only ones." + }, + "base_ref": { + "type": "string", + "enum": ["head", "fresh"], + "description": "For `isolation = worktree`: branch the worktree from current HEAD (`head`, default) or the repo's default branch (`fresh`)." } } } @@ -174,6 +211,16 @@ impl Tool for SpawnParallelAgentsTool { let mut immediate_results = Vec::new(); let mut prepared = Vec::new(); + // Resolve the agent sandbox root once — used as the repo root when a + // task opts into git-worktree isolation. This is `Config.action_dir` + // (the user's project repo the coding agent edits), NOT openhuman's + // own tree. Loaded lazily; only consulted for worktree-isolated tasks. + let action_root: Option = + crate::openhuman::config::Config::load_or_init() + .await + .ok() + .map(|cfg| cfg.action_dir.clone()); + for task in tasks { let agent_id = task.agent_id.trim().to_string(); let prompt = task.prompt.trim().to_string(); @@ -195,6 +242,9 @@ impl Tool for SpawnParallelAgentsTool { elapsed_ms: 0, iterations: 0, stale_parent_reads: Vec::new(), + worktree_path: None, + changed_files: Vec::new(), + dirty_status: None, }); continue; } @@ -216,6 +266,9 @@ impl Tool for SpawnParallelAgentsTool { elapsed_ms: 0, iterations: 0, stale_parent_reads: Vec::new(), + worktree_path: None, + changed_files: Vec::new(), + dirty_status: None, }); continue; }; @@ -242,6 +295,9 @@ impl Tool for SpawnParallelAgentsTool { elapsed_ms: 0, iterations: 0, stale_parent_reads: Vec::new(), + worktree_path: None, + changed_files: Vec::new(), + dirty_status: None, }); continue; } @@ -269,6 +325,9 @@ impl Tool for SpawnParallelAgentsTool { elapsed_ms: 0, iterations: 0, stale_parent_reads: Vec::new(), + worktree_path: None, + changed_files: Vec::new(), + dirty_status: None, }); continue; } @@ -311,7 +370,88 @@ impl Tool for SpawnParallelAgentsTool { ); } } - prepared.push((definition, prompt, task, task_id)); + // ── Optional git-worktree isolation ──────────────────────────── + // When the task requests `isolation = "worktree"`, create a + // dedicated worktree under the user's project repo and run this + // worker with its `action_dir` pointed there. On any failure we + // surface an immediate error result rather than silently falling + // back to the shared workspace (which is the exact collision this + // feature prevents). + let wants_worktree = task + .isolation + .as_deref() + .map(str::trim) + .map(|s| s.eq_ignore_ascii_case("worktree")) + .unwrap_or(false); + let worktree_path = if wants_worktree { + use crate::openhuman::agent_orchestration::worktree; + let base_ref = worktree::BaseRef::parse(task.base_ref.as_deref()); + match action_root.as_ref() { + Some(repo_root) => match worktree::create(repo_root, &task_id, base_ref) { + Ok(status) => { + tracing::debug!( + parent_session = %parent_session, + task_id = %task_id, + worktree = %status.path.display(), + base_ref = base_ref.as_str(), + "[spawn_parallel_agents] created isolated worktree" + ); + Some(status.path) + } + Err(err) => { + tracing::warn!( + parent_session = %parent_session, + task_id = %task_id, + error = %err, + "[spawn_parallel_agents] worktree_create_failed" + ); + immediate_results.push(ParallelAgentResult { + task_id, + agent_id: definition.id.clone(), + success: false, + output: None, + error: Some(format!("worktree isolation failed: {err}")), + ownership: task.ownership, + elapsed_ms: 0, + iterations: 0, + stale_parent_reads: Vec::new(), + worktree_path: None, + changed_files: Vec::new(), + dirty_status: None, + }); + continue; + } + }, + None => { + tracing::warn!( + parent_session = %parent_session, + task_id = %task_id, + "[spawn_parallel_agents] worktree_requested_but_no_action_dir" + ); + immediate_results.push(ParallelAgentResult { + task_id, + agent_id: definition.id.clone(), + success: false, + output: None, + error: Some( + "worktree isolation requested but action_dir is unavailable" + .to_string(), + ), + ownership: task.ownership, + elapsed_ms: 0, + iterations: 0, + stale_parent_reads: Vec::new(), + worktree_path: None, + changed_files: Vec::new(), + dirty_status: None, + }); + continue; + } + } + } else { + None + }; + prepared.push((definition, prompt, task, task_id, worktree_path)); } tracing::debug!( parent_session = %parent_session, @@ -320,11 +460,23 @@ impl Tool for SpawnParallelAgentsTool { "[spawn_parallel_agents] prepared_tasks" ); - let futures = prepared - .into_iter() - .map(|(definition, prompt, task, task_id)| async move { - run_one_parallel_task(definition, prompt, task, task_id).await - }); + let futures = + prepared + .into_iter() + .map(|(definition, prompt, task, task_id, worktree_path)| { + let repo_root = action_root.clone(); + async move { + run_one_parallel_task( + definition, + prompt, + task, + task_id, + worktree_path, + repo_root, + ) + .await + } + }); let mut results = immediate_results; for result in join_all(futures).await { match &result { @@ -442,12 +594,49 @@ impl Tool for SpawnParallelAgentsTool { } } + // Cross-worker overlap detection: when two isolated workers changed + // the SAME file, surface a warning so the parent reconciles before + // synthesis/merge instead of silently clobbering. Keyed on the + // changed-file snapshot collected from each worker's worktree. + let per_worker: Vec<(String, Vec)> = results + .iter() + .filter(|r| !r.changed_files.is_empty()) + .map(|r| { + ( + r.task_id.clone(), + r.changed_files + .iter() + .map(std::path::PathBuf::from) + .collect(), + ) + }) + .collect(); + let overlaps = + crate::openhuman::agent_orchestration::worktree::detect_overlaps(&per_worker); + let overlap_warnings: Vec = overlaps + .iter() + .map(|(file, workers)| { + json!({ + "file": file.to_string_lossy(), + "workers": workers, + }) + }) + .collect(); + if !overlap_warnings.is_empty() { + tracing::warn!( + parent_session = %parent_session, + overlap_count = overlap_warnings.len(), + "[spawn_parallel_agents] detected overlapping changed files across workers" + ); + } + let failures = results.iter().filter(|r| !r.success).count(); tracing::debug!( parent_session = %parent_session, total = results.len(), succeeded = results.len().saturating_sub(failures), failed = failures, + overlaps = overlap_warnings.len(), "[spawn_parallel_agents] execute exit" ); Ok(ToolResult::success( @@ -457,6 +646,7 @@ impl Tool for SpawnParallelAgentsTool { "succeeded": results.len() - failures, "failed": failures, "results": results, + "overlap_warnings": overlap_warnings, } })) .unwrap_or_else(|_| "{}".to_string()), @@ -469,6 +659,8 @@ async fn run_one_parallel_task( prompt: String, task: ParallelAgentTask, task_id: String, + worktree_path: Option, + repo_root: Option, ) -> ParallelAgentResult { let started = std::time::Instant::now(); tracing::debug!( @@ -477,6 +669,7 @@ async fn run_one_parallel_task( toolkit = task.toolkit.as_deref().unwrap_or(""), context_chars = task.context.as_ref().map(|s| s.chars().count()).unwrap_or(0), prompt_chars = prompt.chars().count(), + isolated = worktree_path.is_some(), "[spawn_parallel_agents] task_start" ); let options = SubagentRunOptions { @@ -488,8 +681,51 @@ async fn run_one_parallel_task( worker_thread_id: None, initial_history: None, checkpoint_dir: None, + worktree_action_dir: worktree_path.clone(), }; - match run_subagent(&definition, &prompt, options).await { + let run_result = run_subagent(&definition, &prompt, options).await; + + // After the worker finishes, snapshot the worktree's changed files + + // dirty status so the parent can detect cross-worker overlaps and the UI + // can surface diff/cleanup actions. Best-effort: a status error degrades + // to "no changes recorded" rather than failing the task. + let worktree_str = worktree_path + .as_ref() + .map(|p| p.to_string_lossy().to_string()); + let (changed_files, dirty_status) = match (&worktree_path, &repo_root) { + (Some(wt), Some(root)) => { + use crate::openhuman::agent_orchestration::worktree; + match worktree::status(root, wt) { + Ok(st) => { + tracing::debug!( + task_id = %task_id, + worktree = %wt.display(), + is_dirty = st.is_dirty, + changed = st.changed_files.len(), + "[spawn_parallel_agents] worktree_post_run_status" + ); + let files = st + .changed_files + .iter() + .map(|p| p.to_string_lossy().to_string()) + .collect(); + (files, Some(st.is_dirty)) + } + Err(err) => { + tracing::warn!( + task_id = %task_id, + worktree = %wt.display(), + error = %err, + "[spawn_parallel_agents] worktree_status_failed" + ); + (Vec::new(), None) + } + } + } + _ => (Vec::new(), None), + }; + + match run_result { Ok(outcome) => { tracing::debug!( task_id = %outcome.task_id, @@ -509,6 +745,9 @@ async fn run_one_parallel_task( elapsed_ms: outcome.elapsed.as_millis() as u64, iterations: outcome.iterations as u32, stale_parent_reads: Vec::new(), + worktree_path: worktree_str, + changed_files, + dirty_status, } } Err(err) => { @@ -529,6 +768,9 @@ async fn run_one_parallel_task( elapsed_ms: started.elapsed().as_millis() as u64, iterations: 0, stale_parent_reads: Vec::new(), + worktree_path: worktree_str, + changed_files, + dirty_status, } } } diff --git a/src/openhuman/agent_orchestration/tools/spawn_parallel_agents_tests.rs b/src/openhuman/agent_orchestration/tools/spawn_parallel_agents_tests.rs index c959b86bd..098b42267 100644 --- a/src/openhuman/agent_orchestration/tools/spawn_parallel_agents_tests.rs +++ b/src/openhuman/agent_orchestration/tools/spawn_parallel_agents_tests.rs @@ -46,6 +46,85 @@ fn ownership_boundary_is_prepended_when_present() { assert!(prompt.contains("[Task]\nimplement tests")); } +#[test] +fn schema_advertises_isolation_and_base_ref() { + let tool = SpawnParallelAgentsTool::default(); + let schema = tool.parameters_schema(); + let props = &schema["properties"]["tasks"]["items"]["properties"]; + assert_eq!(props["isolation"]["enum"][0], "none"); + assert_eq!(props["isolation"]["enum"][1], "worktree"); + assert_eq!(props["base_ref"]["enum"][0], "head"); + assert_eq!(props["base_ref"]["enum"][1], "fresh"); +} + +#[test] +fn task_deserializes_isolation_and_base_ref() { + let task: ParallelAgentTask = serde_json::from_value(json!({ + "agent_id": "coder", + "prompt": "do it", + "isolation": "worktree", + "base_ref": "fresh" + })) + .expect("deserialize task"); + assert_eq!(task.isolation.as_deref(), Some("worktree")); + assert_eq!(task.base_ref.as_deref(), Some("fresh")); +} + +#[test] +fn task_isolation_defaults_to_none() { + let task: ParallelAgentTask = serde_json::from_value(json!({ + "agent_id": "researcher", + "prompt": "read it" + })) + .expect("deserialize task"); + assert!(task.isolation.is_none()); + assert!(task.base_ref.is_none()); +} + +#[test] +fn result_omits_worktree_fields_when_absent() { + let result = ParallelAgentResult { + task_id: "t1".into(), + agent_id: "a".into(), + success: true, + output: Some("ok".into()), + error: None, + ownership: None, + elapsed_ms: 5, + iterations: 1, + stale_parent_reads: Vec::new(), + worktree_path: None, + changed_files: Vec::new(), + dirty_status: None, + }; + let v = serde_json::to_value(&result).unwrap(); + assert!(v.get("worktreePath").is_none()); + assert!(v.get("changedFiles").is_none()); + assert!(v.get("dirtyStatus").is_none()); +} + +#[test] +fn result_serializes_worktree_fields_when_present() { + let result = ParallelAgentResult { + task_id: "t2".into(), + agent_id: "coder".into(), + success: true, + output: None, + error: None, + ownership: None, + elapsed_ms: 9, + iterations: 2, + stale_parent_reads: Vec::new(), + worktree_path: Some("/repo/.claude/worktrees/t2".into()), + changed_files: vec!["src/a.rs".into()], + dirty_status: Some(true), + }; + let v = serde_json::to_value(&result).unwrap(); + assert_eq!(v["worktreePath"], "/repo/.claude/worktrees/t2"); + assert_eq!(v["changedFiles"][0], "src/a.rs"); + assert_eq!(v["dirtyStatus"], true); +} + #[tokio::test] async fn rejects_single_task() { let tool = SpawnParallelAgentsTool::new(); diff --git a/src/openhuman/agent_orchestration/tools/spawn_subagent.rs b/src/openhuman/agent_orchestration/tools/spawn_subagent.rs index d5eca947e..ead481bb4 100644 --- a/src/openhuman/agent_orchestration/tools/spawn_subagent.rs +++ b/src/openhuman/agent_orchestration/tools/spawn_subagent.rs @@ -458,6 +458,7 @@ impl Tool for SpawnSubagentTool { worker_thread_id: worker_thread_id.clone(), initial_history: None, checkpoint_dir: None, + worktree_action_dir: None, }; let progress_sink = current_parent().and_then(|p| p.on_progress.clone()); diff --git a/src/openhuman/agent_orchestration/tools/spawn_worker_thread.rs b/src/openhuman/agent_orchestration/tools/spawn_worker_thread.rs index b8d37dccd..e836e8704 100644 --- a/src/openhuman/agent_orchestration/tools/spawn_worker_thread.rs +++ b/src/openhuman/agent_orchestration/tools/spawn_worker_thread.rs @@ -227,6 +227,7 @@ impl Tool for SpawnWorkerThreadTool { worker_thread_id: Some(worker_thread_id.clone()), initial_history: None, checkpoint_dir: None, + worktree_action_dir: None, }; tracing::debug!( diff --git a/src/openhuman/agent_orchestration/worktree.rs b/src/openhuman/agent_orchestration/worktree.rs new file mode 100644 index 000000000..d77ea2ba2 --- /dev/null +++ b/src/openhuman/agent_orchestration/worktree.rs @@ -0,0 +1,445 @@ +//! Git-worktree isolation for parallel, edit-capable agent workers. +//! +//! Parallel coding workers spawned via [`super::tools::spawn_parallel_agents`] +//! historically shared one workspace (`Config.action_dir`). Two workers editing +//! overlapping files left the parent with stale assumptions and silent +//! clobbers. This module gives each isolated worker its own `git worktree` +//! checkout of the **user's project repo** (the directory the coding agent +//! edits), so file edits never collide. +//! +//! ## Scope and safety +//! +//! - This targets the **user's project repository** rooted at the agent's +//! `action_dir` — it never operates on OpenHuman's own source tree. +//! - Every operation validates that `repo_root` is a real git repository +//! first (via `git rev-parse --is-inside-work-tree`), so a stray path can +//! never be mutated. +//! - [`remove`] refuses to delete a **dirty** worktree unless `force = true`. +//! Clean worktrees can be auto-reclaimed; dirty ones require an explicit +//! user decision (acceptance criterion of #3376). +//! +//! The wrapper shells out to `git` through [`std::process::Command`] with an +//! explicit, validated working directory. It does not inherit ambient git +//! configuration that could redirect operations elsewhere. + +use std::path::{Path, PathBuf}; +use std::process::Command; + +use serde::{Deserialize, Serialize}; + +/// Directory (relative to the repo root) under which isolated worker +/// worktrees are created — mirrors Claude Code's `.claude/worktrees/` +/// convention so the layout is familiar and easy to `.gitignore`. +pub const WORKTREE_SUBDIR: &str = ".claude/worktrees"; + +/// Which ref a new worktree branches from. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum BaseRef { + /// Branch off the repo's current `HEAD` — the worker sees the parent's + /// in-progress state. + Head, + /// Branch off the repository's default branch (origin/HEAD, else the + /// local default) — the worker starts from a clean, known baseline. + Fresh, +} + +impl BaseRef { + /// Parse the spawn-request string form. Unknown / empty values default + /// to [`BaseRef::Head`] (the least-surprising baseline for a worker that + /// continues the parent's work). + pub fn parse(value: Option<&str>) -> Self { + match value.map(str::trim).map(str::to_ascii_lowercase).as_deref() { + Some("fresh") => Self::Fresh, + _ => Self::Head, + } + } + + pub fn as_str(self) -> &'static str { + match self { + Self::Head => "head", + Self::Fresh => "fresh", + } + } +} + +/// Snapshot of a single worktree's state. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct WorktreeStatus { + /// Absolute path to the worktree checkout. + pub path: PathBuf, + /// Branch the worktree currently has checked out (or a detached-HEAD + /// label like `(detached HEAD)`), if resolvable. + pub branch: Option, + /// Whether the worktree has uncommitted changes (staged, unstaged, or + /// untracked). A dirty worktree must not be auto-removed. + pub is_dirty: bool, + /// Paths (relative to the worktree root) that differ from HEAD. + pub changed_files: Vec, +} + +/// Errors surfaced by the worktree manager. Stringified at the RPC / tool +/// boundary. +#[derive(Debug, thiserror::Error)] +pub enum WorktreeError { + #[error("path is not inside a git repository: {0}")] + NotAGitRepo(PathBuf), + + #[error("worktree is dirty and force=false; refusing to remove: {0}")] + DirtyRefused(PathBuf), + + #[error("git command `{command}` failed: {stderr}")] + GitFailed { command: String, stderr: String }, + + #[error("io error running git: {0}")] + Io(#[from] std::io::Error), +} + +type Result = std::result::Result; + +/// Run `git ` in `cwd`, returning trimmed stdout on success. +fn git(cwd: &Path, args: &[&str]) -> Result { + tracing::debug!( + cwd = %cwd.display(), + args = ?args, + "[worktree] git_invoke" + ); + let output = Command::new("git").current_dir(cwd).args(args).output()?; + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); + tracing::debug!( + cwd = %cwd.display(), + args = ?args, + stderr = %stderr, + "[worktree] git_failed" + ); + return Err(WorktreeError::GitFailed { + command: format!("git {}", args.join(" ")), + stderr, + }); + } + Ok(String::from_utf8_lossy(&output.stdout).trim().to_string()) +} + +/// Like [`git`] but returns stdout **without** trimming, preserving the +/// leading whitespace that porcelain v1 status lines depend on for column +/// alignment. Trailing newline is left intact; callers iterate `.lines()`. +fn git_raw(cwd: &Path, args: &[&str]) -> Result { + let output = Command::new("git").current_dir(cwd).args(args).output()?; + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); + return Err(WorktreeError::GitFailed { + command: format!("git {}", args.join(" ")), + stderr, + }); + } + Ok(String::from_utf8_lossy(&output.stdout).to_string()) +} + +/// Validate that `repo_root` is inside a real git work tree. Returns the +/// repository's top-level directory on success. +fn validate_repo_root(repo_root: &Path) -> Result { + if !repo_root.exists() { + return Err(WorktreeError::NotAGitRepo(repo_root.to_path_buf())); + } + // `--is-inside-work-tree` prints "true" when inside a work tree. We then + // resolve the canonical top level so all later operations anchor on it. + let inside = git(repo_root, &["rev-parse", "--is-inside-work-tree"]) + .map_err(|_| WorktreeError::NotAGitRepo(repo_root.to_path_buf()))?; + if inside.trim() != "true" { + return Err(WorktreeError::NotAGitRepo(repo_root.to_path_buf())); + } + let top = git(repo_root, &["rev-parse", "--show-toplevel"]) + .map_err(|_| WorktreeError::NotAGitRepo(repo_root.to_path_buf()))?; + Ok(PathBuf::from(top.trim())) +} + +/// Resolve the repo's default branch ref for `BaseRef::Fresh`. Prefers +/// `origin/HEAD`; falls back to the local `HEAD` symbolic ref, then `main`. +fn resolve_fresh_base(repo_top: &Path) -> String { + // origin/HEAD → e.g. "origin/main" + if let Ok(sym) = git( + repo_top, + &["symbolic-ref", "--short", "refs/remotes/origin/HEAD"], + ) { + if !sym.is_empty() { + return sym; + } + } + // Local default branch name. + if let Ok(head) = git(repo_top, &["symbolic-ref", "--short", "HEAD"]) { + if !head.is_empty() { + return head; + } + } + "main".to_string() +} + +/// Create an isolated worktree for `run_id` under +/// `/.claude/worktrees/`, branching off `base_ref`. +/// +/// The new branch is named `worker/`. Returns the worktree's status +/// snapshot (freshly created, so `is_dirty = false`). +pub fn create(repo_root: &Path, run_id: &str, base_ref: BaseRef) -> Result { + let repo_top = validate_repo_root(repo_root)?; + let run_slug = sanitize_run_id(run_id); + let worktree_path = repo_top.join(WORKTREE_SUBDIR).join(&run_slug); + let branch = format!("worker/{run_slug}"); + + let base = match base_ref { + BaseRef::Head => "HEAD".to_string(), + BaseRef::Fresh => resolve_fresh_base(&repo_top), + }; + + tracing::debug!( + repo = %repo_top.display(), + worktree = %worktree_path.display(), + branch = %branch, + base = %base, + base_ref = base_ref.as_str(), + "[worktree] create_start" + ); + + // Ensure the parent dir exists; `git worktree add` creates the leaf. + if let Some(parent) = worktree_path.parent() { + std::fs::create_dir_all(parent)?; + } + + let worktree_str = worktree_path.to_string_lossy().to_string(); + git( + &repo_top, + &["worktree", "add", "-b", &branch, &worktree_str, &base], + )?; + + tracing::debug!( + worktree = %worktree_path.display(), + branch = %branch, + "[worktree] create_done" + ); + + status(&repo_top, &worktree_path) +} + +/// List all worktrees registered on the repo at `repo_root`, parsed from +/// `git worktree list --porcelain`. +pub fn list(repo_root: &Path) -> Result> { + let repo_top = validate_repo_root(repo_root)?; + let porcelain = git(&repo_top, &["worktree", "list", "--porcelain"])?; + let mut out = Vec::new(); + let mut cur_path: Option = None; + let mut cur_branch: Option = None; + + let mut flush = |path: &mut Option, branch: &mut Option| { + if let Some(p) = path.take() { + // status() re-derives dirty + changed files for the worktree. + let (is_dirty, changed_files) = dirty_state(&p).unwrap_or((false, Vec::new())); + out.push(WorktreeStatus { + path: p, + branch: branch.take(), + is_dirty, + changed_files, + }); + } else { + *branch = None; + } + }; + + for line in porcelain.lines() { + if let Some(rest) = line.strip_prefix("worktree ") { + // New record begins — flush the previous one. + flush(&mut cur_path, &mut cur_branch); + cur_path = Some(PathBuf::from(rest.trim())); + } else if let Some(rest) = line.strip_prefix("branch ") { + // "branch refs/heads/foo" → "foo" + cur_branch = Some( + rest.trim() + .strip_prefix("refs/heads/") + .unwrap_or(rest.trim()) + .to_string(), + ); + } else if line.trim() == "detached" { + cur_branch = Some("(detached HEAD)".to_string()); + } + } + flush(&mut cur_path, &mut cur_branch); + + tracing::debug!( + repo = %repo_top.display(), + count = out.len(), + "[worktree] list_done" + ); + Ok(out) +} + +/// Branch + dirty + changed-file snapshot for a single worktree. +pub fn status(repo_root: &Path, worktree_path: &Path) -> Result { + validate_repo_root(repo_root)?; + if !worktree_path.exists() { + return Err(WorktreeError::NotAGitRepo(worktree_path.to_path_buf())); + } + let branch = git(worktree_path, &["rev-parse", "--abbrev-ref", "HEAD"]) + .ok() + .map(|b| { + if b == "HEAD" { + "(detached HEAD)".to_string() + } else { + b + } + }); + let (is_dirty, changed_files) = dirty_state(worktree_path)?; + Ok(WorktreeStatus { + path: worktree_path.to_path_buf(), + branch, + is_dirty, + changed_files, + }) +} + +/// Human-readable diff stat of a worktree's working changes vs HEAD, +/// including untracked files. Returns an empty string for a clean worktree. +pub fn diff_summary(repo_root: &Path, worktree_path: &Path) -> Result { + validate_repo_root(repo_root)?; + // `--stat` over both staged and unstaged changes vs HEAD. + let stat = git(worktree_path, &["diff", "HEAD", "--stat"])?; + let untracked = git( + worktree_path, + &["ls-files", "--others", "--exclude-standard"], + )?; + let mut parts = Vec::new(); + if !stat.is_empty() { + parts.push(stat); + } + if !untracked.is_empty() { + let lines: Vec = untracked + .lines() + .map(|l| format!(" {l} (untracked)")) + .collect(); + parts.push(lines.join("\n")); + } + Ok(parts.join("\n")) +} + +/// Remove a worktree. **Refuses to remove a dirty worktree unless +/// `force = true`** — the core safety guarantee of #3376. +/// +/// On `force = true`, a dirty worktree is removed with `git worktree remove +/// --force`. The associated `worker/` branch is left intact so the +/// caller can still inspect or merge the work. +pub fn remove(repo_root: &Path, worktree_path: &Path, force: bool) -> Result<()> { + let repo_top = validate_repo_root(repo_root)?; + let (is_dirty, _changed) = dirty_state(worktree_path).unwrap_or((false, Vec::new())); + + tracing::debug!( + repo = %repo_top.display(), + worktree = %worktree_path.display(), + is_dirty, + force, + "[worktree] remove_start" + ); + + if is_dirty && !force { + tracing::warn!( + worktree = %worktree_path.display(), + "[worktree] remove_refused_dirty" + ); + return Err(WorktreeError::DirtyRefused(worktree_path.to_path_buf())); + } + + let worktree_str = worktree_path.to_string_lossy().to_string(); + let mut args = vec!["worktree", "remove", &worktree_str]; + if force { + args.push("--force"); + } + git(&repo_top, &args)?; + tracing::debug!( + worktree = %worktree_path.display(), + "[worktree] remove_done" + ); + Ok(()) +} + +/// Compute `(is_dirty, changed_files)` for a worktree via +/// `git status --porcelain`. A worktree is dirty when any tracked file is +/// modified/staged or any untracked (non-ignored) file is present. +/// +/// Uses the raw (un-trimmed) command output: porcelain v1 lines are +/// column-aligned (`XY␠PATH`, status = 2 chars), so a global `.trim()` would +/// corrupt the leading space of a `␠M file` line and shift the path. We parse +/// the raw bytes and slice the fixed 3-char `XY␠` prefix per line. +fn dirty_state(worktree_path: &Path) -> Result<(bool, Vec)> { + let porcelain = git_raw(worktree_path, &["status", "--porcelain"])?; + let mut changed = Vec::new(); + for line in porcelain.lines() { + // Porcelain v1 line: "XY " — status is exactly 2 chars, then a + // single separator space, so the path always starts at byte index 3. + if line.len() > 3 { + let path = line[3..].trim_end(); + // Rename lines look like "old -> new"; record the new path. + let path = path.rsplit(" -> ").next().unwrap_or(path); + changed.push(PathBuf::from(path)); + } + } + changed.sort(); + changed.dedup(); + Ok((!changed.is_empty(), changed)) +} + +/// Sanitize a `run_id` into a filesystem-safe single path segment. +fn sanitize_run_id(run_id: &str) -> String { + let cleaned: String = run_id + .chars() + .map(|c| { + if c.is_ascii_alphanumeric() || c == '_' || c == '-' { + c + } else { + '-' + } + }) + .collect(); + let trimmed = cleaned.trim_matches('-'); + if trimmed.is_empty() { + "worker".to_string() + } else { + trimmed.to_string() + } +} + +/// Detect changed files touched by more than one sibling worker. +/// +/// Input maps each worker's id to the set of files it changed (relative +/// paths, as produced by [`WorktreeStatus::changed_files`]). Output maps each +/// overlapping file to the sorted list of worker ids that touched it — empty +/// when there is no overlap. Used to surface a pre-merge conflict warning. +pub fn detect_overlaps( + per_worker: &[(String, Vec)], +) -> std::collections::BTreeMap> { + use std::collections::BTreeMap; + let mut by_file: BTreeMap> = BTreeMap::new(); + for (worker_id, files) in per_worker { + let mut seen = std::collections::BTreeSet::new(); + for f in files { + if seen.insert(f.clone()) { + by_file + .entry(f.clone()) + .or_default() + .push(worker_id.clone()); + } + } + } + by_file + .into_iter() + .filter_map(|(file, mut workers)| { + workers.sort(); + workers.dedup(); + if workers.len() > 1 { + Some((file, workers)) + } else { + None + } + }) + .collect() +} + +#[cfg(test)] +#[path = "worktree_tests.rs"] +mod tests; diff --git a/src/openhuman/agent_orchestration/worktree_tests.rs b/src/openhuman/agent_orchestration/worktree_tests.rs new file mode 100644 index 000000000..8c5eb95b2 --- /dev/null +++ b/src/openhuman/agent_orchestration/worktree_tests.rs @@ -0,0 +1,235 @@ +//! Tests for the git-worktree isolation manager. +//! +//! Each test stands up a real temporary git repository (`git init`) so the +//! `git worktree` plumbing is exercised end-to-end. Tests are skipped (pass +//! trivially) when `git` is not on PATH, so CI without git doesn't hard-fail. + +use super::*; +use std::path::Path; +use std::process::Command; + +/// `true` when `git` is invokable on this host. +fn git_available() -> bool { + Command::new("git") + .arg("--version") + .output() + .map(|o| o.status.success()) + .unwrap_or(false) +} + +fn run(dir: &Path, args: &[&str]) { + let status = Command::new("git") + .current_dir(dir) + .args(args) + .output() + .expect("git invocation"); + assert!( + status.status.success(), + "git {:?} failed: {}", + args, + String::from_utf8_lossy(&status.stderr) + ); +} + +/// Initialise a temp git repo with one committed file. Returns the tempdir +/// guard (kept alive by the caller) and the repo root path. +fn init_repo() -> (tempfile::TempDir, std::path::PathBuf) { + let tmp = tempfile::tempdir().expect("tempdir"); + let root = tmp.path().to_path_buf(); + run(&root, &["init", "-b", "main"]); + run(&root, &["config", "user.email", "test@example.com"]); + run(&root, &["config", "user.name", "Test User"]); + std::fs::write(root.join("README.md"), "hello\n").unwrap(); + run(&root, &["add", "README.md"]); + run(&root, &["commit", "-m", "initial"]); + (tmp, root) +} + +#[test] +fn validate_repo_root_rejects_non_repo() { + if !git_available() { + return; + } + let tmp = tempfile::tempdir().unwrap(); + let err = create(tmp.path(), "run-1", BaseRef::Head).unwrap_err(); + assert!(matches!(err, WorktreeError::NotAGitRepo(_))); +} + +#[test] +fn create_then_status_reports_clean_worktree() { + if !git_available() { + return; + } + let (_tmp, root) = init_repo(); + let st = create(&root, "run-1", BaseRef::Head).expect("create"); + assert!(st.path.exists(), "worktree dir should exist"); + assert_eq!(st.branch.as_deref(), Some("worker/run-1")); + assert!(!st.is_dirty, "fresh worktree is clean"); + assert!(st.changed_files.is_empty()); + assert!( + st.path.ends_with(Path::new(".claude/worktrees/run-1")), + "worktree under .claude/worktrees/, got {}", + st.path.display() + ); +} + +#[test] +fn list_includes_created_worktree() { + if !git_available() { + return; + } + let (_tmp, root) = init_repo(); + create(&root, "run-a", BaseRef::Head).expect("create a"); + create(&root, "run-b", BaseRef::Fresh).expect("create b"); + let all = list(&root).expect("list"); + // main worktree + the two we created + assert!(all.len() >= 3, "expected >=3 worktrees, got {}", all.len()); + let branches: Vec<_> = all.iter().filter_map(|w| w.branch.clone()).collect(); + assert!(branches.iter().any(|b| b == "worker/run-a")); + assert!(branches.iter().any(|b| b == "worker/run-b")); +} + +#[test] +fn status_detects_dirty_changes() { + if !git_available() { + return; + } + let (_tmp, root) = init_repo(); + let st = create(&root, "run-dirty", BaseRef::Head).expect("create"); + // Touch a tracked file + add an untracked one. + std::fs::write(st.path.join("README.md"), "changed\n").unwrap(); + std::fs::write(st.path.join("new.txt"), "fresh\n").unwrap(); + + let st2 = status(&root, &st.path).expect("status"); + assert!(st2.is_dirty, "worktree with edits must be dirty"); + let names: Vec = st2 + .changed_files + .iter() + .map(|p| p.to_string_lossy().to_string()) + .collect(); + assert!(names.iter().any(|n| n.contains("README.md"))); + assert!(names.iter().any(|n| n.contains("new.txt"))); +} + +#[test] +fn diff_summary_lists_tracked_and_untracked() { + if !git_available() { + return; + } + let (_tmp, root) = init_repo(); + let st = create(&root, "run-diff", BaseRef::Head).expect("create"); + std::fs::write(st.path.join("README.md"), "changed body\n").unwrap(); + std::fs::write(st.path.join("brand_new.txt"), "x\n").unwrap(); + + let summary = diff_summary(&root, &st.path).expect("diff"); + assert!( + summary.contains("README.md"), + "diff should mention tracked change: {summary}" + ); + assert!( + summary.contains("brand_new.txt") && summary.contains("untracked"), + "diff should list untracked file: {summary}" + ); +} + +#[test] +fn remove_refuses_dirty_without_force() { + if !git_available() { + return; + } + let (_tmp, root) = init_repo(); + let st = create(&root, "run-keep", BaseRef::Head).expect("create"); + std::fs::write(st.path.join("README.md"), "dirty\n").unwrap(); + + let err = remove(&root, &st.path, false).expect_err("must refuse dirty"); + assert!(matches!(err, WorktreeError::DirtyRefused(_))); + assert!(st.path.exists(), "dirty worktree must NOT be deleted"); +} + +#[test] +fn remove_force_deletes_dirty_worktree() { + if !git_available() { + return; + } + let (_tmp, root) = init_repo(); + let st = create(&root, "run-force", BaseRef::Head).expect("create"); + std::fs::write(st.path.join("README.md"), "dirty\n").unwrap(); + + remove(&root, &st.path, true).expect("force remove"); + assert!(!st.path.exists(), "force remove deletes the worktree dir"); +} + +#[test] +fn remove_clean_worktree_succeeds() { + if !git_available() { + return; + } + let (_tmp, root) = init_repo(); + let st = create(&root, "run-clean", BaseRef::Head).expect("create"); + remove(&root, &st.path, false).expect("clean remove"); + assert!(!st.path.exists(), "clean worktree removed without force"); +} + +#[test] +fn base_ref_parse_defaults_to_head() { + assert_eq!(BaseRef::parse(None), BaseRef::Head); + assert_eq!(BaseRef::parse(Some("head")), BaseRef::Head); + assert_eq!(BaseRef::parse(Some("HEAD")), BaseRef::Head); + assert_eq!(BaseRef::parse(Some("fresh")), BaseRef::Fresh); + assert_eq!(BaseRef::parse(Some(" Fresh ")), BaseRef::Fresh); + assert_eq!(BaseRef::parse(Some("garbage")), BaseRef::Head); +} + +#[test] +fn sanitize_run_id_strips_unsafe_chars() { + assert_eq!(sanitize_run_id("sub-1234"), "sub-1234"); + assert_eq!(sanitize_run_id("a/b\\c"), "a-b-c"); + assert_eq!(sanitize_run_id("///"), "worker"); + assert_eq!(sanitize_run_id(""), "worker"); +} + +#[test] +fn detect_overlaps_flags_shared_files() { + let per_worker = vec![ + ( + "w1".to_string(), + vec![PathBuf::from("src/a.rs"), PathBuf::from("src/b.rs")], + ), + ( + "w2".to_string(), + vec![PathBuf::from("src/b.rs"), PathBuf::from("src/c.rs")], + ), + ("w3".to_string(), vec![PathBuf::from("src/c.rs")]), + ]; + let overlaps = detect_overlaps(&per_worker); + // b.rs touched by w1+w2; c.rs touched by w2+w3; a.rs only w1 (no overlap). + assert_eq!(overlaps.len(), 2); + assert_eq!( + overlaps.get(&PathBuf::from("src/b.rs")).unwrap(), + &vec!["w1".to_string(), "w2".to_string()] + ); + assert_eq!( + overlaps.get(&PathBuf::from("src/c.rs")).unwrap(), + &vec!["w2".to_string(), "w3".to_string()] + ); + assert!(!overlaps.contains_key(&PathBuf::from("src/a.rs"))); +} + +#[test] +fn detect_overlaps_empty_when_disjoint() { + let per_worker = vec![ + ("w1".to_string(), vec![PathBuf::from("a.rs")]), + ("w2".to_string(), vec![PathBuf::from("b.rs")]), + ]; + assert!(detect_overlaps(&per_worker).is_empty()); +} + +#[test] +fn detect_overlaps_ignores_intra_worker_duplicates() { + // A single worker listing the same file twice must not self-overlap. + let per_worker = vec![( + "w1".to_string(), + vec![PathBuf::from("a.rs"), PathBuf::from("a.rs")], + )]; + assert!(detect_overlaps(&per_worker).is_empty()); +} diff --git a/src/openhuman/tools/impl/filesystem/git_operations.rs b/src/openhuman/tools/impl/filesystem/git_operations.rs index 84f16008f..61309c324 100644 --- a/src/openhuman/tools/impl/filesystem/git_operations.rs +++ b/src/openhuman/tools/impl/filesystem/git_operations.rs @@ -19,6 +19,18 @@ impl GitOperationsTool { } } + /// Resolve the working directory for git operations. + /// + /// Returns the per-worker git-worktree override when one is installed via + /// [`crate::openhuman::agent::harness::with_action_dir_override`] (an + /// edit-capable worker running with `isolation = "worktree"`), otherwise + /// the tool's configured `action_dir`. Keeping `self.action_dir` as the + /// fallback preserves the non-isolated behaviour exactly. See #3376. + fn effective_action_dir(&self) -> std::path::PathBuf { + crate::openhuman::agent::harness::current_action_dir_override() + .unwrap_or_else(|| self.action_dir.clone()) + } + /// Sanitize git arguments to prevent injection attacks fn sanitize_git_args(&self, args: &str) -> anyhow::Result> { let mut result = Vec::new(); @@ -68,7 +80,7 @@ impl GitOperationsTool { async fn run_git_command(&self, args: &[&str]) -> anyhow::Result { let output = tokio::process::Command::new("git") .args(args) - .current_dir(&self.action_dir) + .current_dir(self.effective_action_dir()) .output() .await?; @@ -479,10 +491,12 @@ impl Tool for GitOperationsTool { } }; - // Check if we're in a git repository - if !self.action_dir.join(".git").exists() { + // Check if we're in a git repository. A linked worktree's `.git` is a + // file (a gitdir pointer), not a directory — `exists()` covers both. + let effective_dir = self.effective_action_dir(); + if !effective_dir.join(".git").exists() { // Try to find .git in parent directories - let mut current_dir = self.action_dir.as_path(); + let mut current_dir = effective_dir.as_path(); let mut found_git = false; while current_dir.parent().is_some() { if current_dir.join(".git").exists() { diff --git a/src/openhuman/tools/impl/system/shell.rs b/src/openhuman/tools/impl/system/shell.rs index 83fdb9e33..9cb164d4b 100644 --- a/src/openhuman/tools/impl/system/shell.rs +++ b/src/openhuman/tools/impl/system/shell.rs @@ -140,6 +140,18 @@ impl ShellTool { ); } } + + /// Resolve the working directory for this shell invocation. + /// + /// Returns the per-worker git-worktree override when one is installed via + /// [`crate::openhuman::agent::harness::with_action_dir_override`] (an + /// edit-capable worker running with `isolation = "worktree"`), otherwise + /// the shared `self.security.action_dir`. Keeping `security.action_dir` as + /// the fallback preserves the non-isolated behaviour exactly. See #3376. + fn effective_action_dir(&self) -> std::path::PathBuf { + crate::openhuman::agent::harness::current_action_dir_override() + .unwrap_or_else(|| self.security.action_dir.clone()) + } } #[async_trait] @@ -279,7 +291,7 @@ impl ShellTool { // (CWE-200), then re-add only safe, functional variables. let mut cmd = match self .runtime - .build_shell_command(command, &self.security.action_dir) + .build_shell_command(command, &self.effective_action_dir()) { Ok(cmd) => cmd, Err(e) => { @@ -383,7 +395,7 @@ impl ShellTool { let config = crate::openhuman::config::RuntimeConfig::default(); let policy = sandbox::resolve_sandbox_policy( crate::openhuman::agent::harness::definition::SandboxMode::Sandboxed, - &self.security.action_dir, + &self.effective_action_dir(), &config, false, );