From 4509af8a87f1680e770d61bb68d5e448a47742fa Mon Sep 17 00:00:00 2001 From: Yue Chen Date: Sun, 24 May 2026 19:55:53 -0700 Subject: [PATCH 1/2] feat(spawn): isolate workers in git worktrees --- crates/octos-agent/src/tools/spawn.rs | 434 +++++++++++++++++++++++-- crates/octos-cli/src/commands/clean.rs | 108 +++++- crates/octos-core/src/session_scope.rs | 54 +++ 3 files changed, 565 insertions(+), 31 deletions(-) diff --git a/crates/octos-agent/src/tools/spawn.rs b/crates/octos-agent/src/tools/spawn.rs index 56bc43b285..8408679489 100644 --- a/crates/octos-agent/src/tools/spawn.rs +++ b/crates/octos-agent/src/tools/spawn.rs @@ -1,13 +1,14 @@ //! Spawn tool for background subagent execution. use std::path::{Path, PathBuf}; +use std::process::Command; use std::sync::Arc; use std::sync::atomic::{AtomicU32, Ordering}; use async_trait::async_trait; use eyre::{Result, WrapErr}; use metrics::counter; -use octos_core::{AgentId, InboundMessage, Task, TaskContext, TaskKind, TaskResult}; +use octos_core::{AgentId, InboundMessage, SessionScope, Task, TaskContext, TaskKind, TaskResult}; use octos_llm::{ContextWindowOverride, LlmProvider, ProviderRouter}; use octos_memory::EpisodeStore; use regex::Regex; @@ -79,6 +80,180 @@ pub type ChildSessionLifecycleSender = Arc< pub type ChildToolFactory = Arc Arc + Send + Sync>; +#[derive(Clone, Copy, Debug, Default, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +enum WorkerIsolation { + #[default] + Shared, + Worktree, +} + +#[derive(Clone, Debug)] +struct WorkerWorktree { + slug: String, + branch: String, + path: PathBuf, +} + +impl WorkerWorktree { + fn mark_status(&self, status: &str) { + if let Err(error) = write_worker_worktree_status(self, status) { + warn!( + slug = %self.slug, + branch = %self.branch, + path = %self.path.display(), + error = %error, + "spawn: failed to update worker worktree status" + ); + } + } +} + +fn validate_worker_worktree_slug(slug: &str) -> Result<(), String> { + if slug.is_empty() { + return Err("worker worktree slug must not be empty".to_string()); + } + if slug.len() > 64 { + return Err("worker worktree slug must be at most 64 characters".to_string()); + } + if slug.starts_with('/') || slug.starts_with('\\') || slug.contains('\\') { + return Err("worker worktree slug must be relative and use '/' separators".to_string()); + } + for segment in slug.split('/') { + if segment.is_empty() || segment == "." || segment == ".." { + return Err("worker worktree slug contains an unsafe path segment".to_string()); + } + if !segment + .chars() + .all(|ch| ch.is_ascii_alphanumeric() || matches!(ch, '.' | '_' | '-')) + { + return Err(format!( + "worker worktree slug segment {segment:?} contains unsupported characters" + )); + } + } + Ok(()) +} + +fn git_stdout(repo: &Path, args: &[&str]) -> Result { + let output = Command::new("git") + .arg("-C") + .arg(repo) + .args(args) + .output() + .wrap_err_with(|| format!("failed to run git {}", args.join(" ")))?; + if !output.status.success() { + return Err(eyre::eyre!( + "git {} failed: {}", + args.join(" "), + String::from_utf8_lossy(&output.stderr).trim() + )); + } + Ok(String::from_utf8_lossy(&output.stdout).trim().to_string()) +} + +fn git_ref_exists(repo: &Path, refname: &str) -> Result { + let status = Command::new("git") + .arg("-C") + .arg(repo) + .args(["show-ref", "--verify", "--quiet", refname]) + .status() + .wrap_err("failed to run git show-ref")?; + match status.code() { + Some(0) => Ok(true), + Some(1) => Ok(false), + _ => Err(eyre::eyre!("git show-ref exited with {status}")), + } +} + +fn allocate_worker_worktree( + parent_working_dir: &Path, + worker_id: &AgentId, +) -> Result { + let repo_root = PathBuf::from(git_stdout( + parent_working_dir, + &["rev-parse", "--show-toplevel"], + )?); + let base_slug = worker_id.to_string(); + let work_root = repo_root.join(".octos").join("work"); + std::fs::create_dir_all(&work_root).wrap_err_with(|| { + format!( + "failed to create worker worktree root {}", + work_root.display() + ) + })?; + + for attempt in 0..=32 { + let slug = if attempt == 0 { + base_slug.clone() + } else { + format!("{base_slug}-{attempt}") + }; + validate_worker_worktree_slug(&slug).map_err(eyre::Report::msg)?; + let branch = format!("octos/worker/{slug}"); + let refname = format!("refs/heads/{branch}"); + let path = work_root.join(&slug); + if path.exists() || git_ref_exists(&repo_root, &refname)? { + continue; + } + let output = Command::new("git") + .arg("-C") + .arg(&repo_root) + .args(["worktree", "add", "-b"]) + .arg(&branch) + .arg(&path) + .arg("HEAD") + .output() + .wrap_err("failed to run git worktree add")?; + if !output.status.success() { + return Err(eyre::eyre!( + "git worktree add failed: {}", + String::from_utf8_lossy(&output.stderr).trim() + )); + } + let allocation = WorkerWorktree { slug, branch, path }; + allocation.mark_status("spawned"); + return Ok(allocation); + } + + Err(eyre::eyre!( + "could not allocate a unique worker worktree slug for {base_slug:?}" + )) +} + +fn write_worker_worktree_status(worktree: &WorkerWorktree, status: &str) -> Result<()> { + let state_dir = worktree.path.join(".octos"); + std::fs::create_dir_all(&state_dir)?; + let payload = serde_json::json!({ + "schema_version": 1, + "agent_id": worktree.slug, + "branch": worktree.branch, + "path": worktree.path, + "status": status, + }); + std::fs::write( + state_dir.join("worker-worktree.json"), + serde_json::to_vec_pretty(&payload)?, + )?; + Ok(()) +} + +fn scoped_child_session_scope( + parent_scope: Option<&Arc>, + working_dir: &Path, +) -> Result>> { + parent_scope + .map(|scope| { + scope + .as_ref() + .clone() + .with_workspace(working_dir.to_path_buf()) + .map(Arc::new) + .map_err(|error| eyre::eyre!(error)) + }) + .transpose() +} + #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum BackgroundResultKind { Notification, @@ -950,6 +1125,10 @@ struct Input { /// "background" (default) or "sync". #[serde(default = "default_mode")] mode: String, + /// Worker filesystem isolation strategy. Shared preserves legacy behavior; + /// worktree gives the child a dedicated git worktree under `.octos/work`. + #[serde(default)] + isolation: WorkerIsolation, /// Tool names the subagent is allowed to use. Empty = all builtins. #[serde(default)] allowed_tools: Vec, @@ -1869,6 +2048,12 @@ impl Tool for SpawnTool { "description": "background: returns immediately, result announced later. sync: waits and returns the result.", "default": "background" }, + "isolation": { + "type": "string", + "enum": ["shared", "worktree"], + "description": "Filesystem isolation for builtin subagents. shared uses the parent workspace; worktree creates a dedicated git worktree under .octos/work.", + "default": "shared" + }, "allowed_tools": { "type": "array", "items": { "type": "string" }, @@ -2006,6 +2191,13 @@ impl Tool for SpawnTool { let workflow = input.workflow.clone(); let is_sync = input.mode == "sync"; let is_agent_mcp = input.backend == "agent_mcp"; + if is_agent_mcp && input.isolation == WorkerIsolation::Worktree { + return Ok(ToolResult { + output: "Status: FAILED\nworktree isolation is currently supported only for builtin subagents".to_string(), + success: false, + ..Default::default() + }); + } info!( worker_id = %worker_id, @@ -2434,6 +2626,43 @@ impl Tool for SpawnTool { }); } + let worker_worktree = match input.isolation { + WorkerIsolation::Shared => None, + WorkerIsolation::Worktree => { + match allocate_worker_worktree(&self.working_dir, &worker_id) { + Ok(worktree) => Some(worktree), + Err(error) => { + return Ok(ToolResult { + output: format!( + "Status: FAILED\nfailed to allocate worker git worktree: {error}" + ), + success: false, + ..Default::default() + }); + } + } + } + }; + let child_working_dir = worker_worktree + .as_ref() + .map(|worktree| worktree.path.clone()) + .unwrap_or_else(|| self.working_dir.clone()); + let child_session_scope = match scoped_child_session_scope( + ctx.session_scope.as_ref(), + &child_working_dir, + ) { + Ok(scope) => scope, + Err(error) => { + return Ok(ToolResult { + output: format!( + "Status: FAILED\nfailed to inherit session scope for worker worktree: {error}" + ), + success: false, + ..Default::default() + }); + } + }; + let sub_llm = self.resolve_sub_provider(input.model.as_deref(), input.context_window)?; // Review A F-004: snapshot the parent workspace policy once so both the @@ -2457,7 +2686,7 @@ impl Tool for SpawnTool { if is_sync { // Sync mode: run subagent inline and return the result directly - let mut tools = ToolRegistry::with_builtins(&self.working_dir); + let mut tools = ToolRegistry::with_builtins(&child_working_dir); // Load plugin tools so subagents can use fm_tts, etc. // Section B (codex review P1.1): honour the parent's // require_signed policy so unsigned plugins are rejected here @@ -2468,7 +2697,7 @@ impl Tool for SpawnTool { &self.plugin_dirs, &self.plugin_extra_env, crate::plugins::PluginLoadOptions { - work_dir: Some(&self.working_dir), + work_dir: Some(&child_working_dir), synthesis_config: None, require_signed: self.plugin_require_signed, verified_cache_dir: None, @@ -2505,10 +2734,9 @@ impl Tool for SpawnTool { // parent's scope into the child Agent so the child's tools // (shell, read_file, write_file, edit_file, plugin tool, // pipeline workers) all see the same filesystem contract. - // The child runs in a sub-session of the same parent — for - // now it shares the parent's workspace; a future enhancement - // can carve out per-child subdirs if isolation is required. - if let Some(scope) = ctx.session_scope.as_ref() { + // Worktree-isolated children inherit the same root/mode policy + // with their workspace rebound to the worker worktree. + if let Some(scope) = child_session_scope.as_ref() { worker = worker.with_session_scope(scope.clone()); } // Keep an Arc handle to the child's tool registry so we can run @@ -2585,7 +2813,7 @@ impl Tool for SpawnTool { files: vec![], }, TaskContext { - working_dir: self.working_dir.clone(), + working_dir: child_working_dir.clone(), ..Default::default() }, ); @@ -2594,7 +2822,7 @@ impl Tool for SpawnTool { // M8.9 recovery so the synchronous spawn path mirrors the // session-actor recovery contract. let result = run_task_with_m8_9_recovery(&worker, &subtask, &task_desc).await; - match result { + let tool_result = match result { Ok(r) => { // Review A F-004: run declared completion-phase validators // against the child's artifacts before surfacing success. @@ -2608,7 +2836,7 @@ impl Tool for SpawnTool { if !policy.validation.validators.is_empty() { match crate::workspace_contract::run_declared_validators( child_tools_handle.as_ref(), - &self.working_dir, + &child_working_dir, &policy.validation.validators, "spawn", crate::validators::ValidatorPhase::Completion, @@ -2645,7 +2873,7 @@ impl Tool for SpawnTool { workflow.as_ref().and_then(workflow_contract_project_kind); let report = crate::workspace_contract::run_project_root_validators( child_tools_handle.as_ref(), - &self.working_dir, + &child_working_dir, expected_kind, &r.files_to_send, ) @@ -2658,19 +2886,27 @@ impl Tool for SpawnTool { } } - Ok(ToolResult { + ToolResult { output, success, tokens_used: Some(r.token_usage), ..Default::default() - }) + } } - Err(e) => Ok(ToolResult { + Err(e) => ToolResult { output: format!("Subagent failed: {e}"), success: false, ..Default::default() - }), + }, + }; + if let Some(worktree) = worker_worktree.as_ref() { + worktree.mark_status(if tool_result.success { + "completed" + } else { + "failed" + }); } + Ok(tool_result) } else { // Background mode: fire-and-forget let (origin_channel, origin_chat_id) = self @@ -2735,9 +2971,10 @@ impl Tool for SpawnTool { }; let llm = sub_llm; let memory = self.memory.clone(); - let working_dir = self.working_dir.clone(); + let working_dir = child_working_dir.clone(); let inbound_tx = self.inbound_tx.clone(); let wid = worker_id.clone(); + let detached_worker_worktree = worker_worktree.clone(); let provider_policy = self.provider_policy.clone(); let additional_instructions = input.additional_instructions; let default_worker_prompt = self.worker_prompt.clone(); @@ -2824,11 +3061,9 @@ impl Tool for SpawnTool { let child_spawn_depth = ctx.spawn_depth.saturating_add(1); // Phase 2-D of the SessionScope migration: snapshot the // parent's scope before crossing into the detached - // `tokio::spawn` task. The background child shares the - // parent workspace — same contract as the sync branch above - // — so subprocesses spawned by the child's shell tool see - // the same CWD the foreground would. - let child_session_scope = ctx.session_scope.clone(); + // `tokio::spawn` task. Worktree-isolated children get a + // precomputed scope with the same policy and a worker CWD. + let child_session_scope = child_session_scope.clone(); tokio::spawn(async move { // codex round-5: the terminal guard was armed in the @@ -3282,6 +3517,14 @@ impl Tool for SpawnTool { } else { classify_child_session_lifecycle_kind(&result) }; + if let Some(worktree) = detached_worker_worktree.as_ref() { + worktree.mark_status(match terminal_kind { + ChildSessionLifecycleKind::Completed => "completed", + ChildSessionLifecycleKind::RetryableFailed + | ChildSessionLifecycleKind::TerminalFailed => "failed", + ChildSessionLifecycleKind::Spawned => "spawned", + }); + } if let (Some(supervisor), Some(task_id)) = (task_supervisor.as_ref(), tracked_task_id.as_ref()) @@ -5525,6 +5768,155 @@ PY } } + struct EditSameFileProvider; + + #[async_trait] + impl LlmProvider for EditSameFileProvider { + async fn chat( + &self, + messages: &[octos_core::Message], + _tools: &[octos_llm::ToolSpec], + _config: &octos_llm::ChatConfig, + ) -> Result { + let edit_already_run = messages + .iter() + .any(|msg| matches!(msg.role, octos_core::MessageRole::Tool)); + if edit_already_run { + return Ok(octos_llm::ChatResponse { + content: Some("done".into()), + reasoning_content: None, + tool_calls: vec![], + stop_reason: octos_llm::StopReason::EndTurn, + usage: octos_llm::TokenUsage::default(), + provider_index: None, + }); + } + + Ok(octos_llm::ChatResponse { + content: None, + reasoning_content: None, + tool_calls: vec![octos_core::ToolCall { + id: "call_edit_file".into(), + name: "edit_file".into(), + arguments: serde_json::json!({ + "path": "shared.txt", + "old_string": "base\n", + "new_string": "worker content\n" + }), + metadata: None, + }], + stop_reason: octos_llm::StopReason::ToolUse, + usage: octos_llm::TokenUsage::default(), + provider_index: None, + }) + } + + fn model_id(&self) -> &str { + "mock" + } + + fn provider_name(&self) -> &str { + "mock" + } + } + + fn run_git(repo: &std::path::Path, args: &[&str]) { + let status = Command::new("git") + .arg("-C") + .arg(repo) + .args(args) + .status() + .expect("git command starts"); + assert!(status.success(), "git {args:?} failed with {status}"); + } + + #[test] + fn worker_worktree_slug_validation_rejects_traversal() { + for valid in ["subagent-0", "abc.DEF_123", "parent/child-1"] { + validate_worker_worktree_slug(valid).expect("valid slug accepted"); + } + + for invalid in [ + "", + ".", + "..", + "../escape", + "escape/..", + "/absolute", + "\\absolute", + "bad\\slash", + "bad space", + ] { + assert!( + validate_worker_worktree_slug(invalid).is_err(), + "invalid slug {invalid:?} was accepted" + ); + } + } + + #[tokio::test] + async fn worktree_isolation_runs_concurrent_writers_on_separate_branches() { + let repo = tempfile::tempdir().unwrap(); + run_git(repo.path(), &["init"]); + std::fs::write(repo.path().join("shared.txt"), "base\n").unwrap(); + run_git(repo.path(), &["add", "shared.txt"]); + run_git( + repo.path(), + &[ + "-c", + "user.name=Octos Test", + "-c", + "user.email=octos@example.invalid", + "commit", + "-m", + "base", + ], + ); + + let scope = octos_core::SessionScope::solo(repo.path().to_path_buf(), vec![]) + .expect("scope construction"); + let (in_tx, _in_rx) = tokio::sync::mpsc::channel(16); + let tool = SpawnTool::new( + Arc::new(EditSameFileProvider), + Arc::new(create_test_store().await), + repo.path().to_path_buf(), + in_tx, + ); + let mut ctx = super::super::ToolContext::zero(); + ctx.session_scope = Some(Arc::new(scope)); + + let args = serde_json::json!({ + "task": "edit shared.txt", + "mode": "sync", + "isolation": "worktree", + "allowed_tools": ["edit_file"] + }); + let (first, second) = tokio::join!( + tool.execute_with_context(&ctx, &args), + tool.execute_with_context(&ctx, &args) + ); + let first = first.expect("first spawn returns"); + let second = second.expect("second spawn returns"); + assert!(first.success, "first spawn failed: {}", first.output); + assert!(second.success, "second spawn failed: {}", second.output); + + assert_eq!( + std::fs::read_to_string(repo.path().join("shared.txt")).unwrap(), + "base\n", + "parent workspace must not be mutated by isolated workers" + ); + for slug in ["subagent-0", "subagent-1"] { + let worker_root = repo.path().join(".octos/work").join(slug); + assert_eq!( + std::fs::read_to_string(worker_root.join("shared.txt")).unwrap(), + "worker content\n" + ); + let status = + std::fs::read_to_string(worker_root.join(".octos/worker-worktree.json")).unwrap(); + assert!(status.contains("\"status\": \"completed\"")); + } + } + #[tokio::test] async fn spawn_propagates_scope_to_sub_agent() { // When the parent `ToolContext` carries a `SessionScope`, the diff --git a/crates/octos-cli/src/commands/clean.rs b/crates/octos-cli/src/commands/clean.rs index dd16912a6c..e2b1ef0409 100644 --- a/crates/octos-cli/src/commands/clean.rs +++ b/crates/octos-cli/src/commands/clean.rs @@ -1,6 +1,8 @@ //! Clean command: remove stale state files. +use std::collections::HashSet; use std::path::PathBuf; +use std::process::Command; use clap::Args; use colored::Colorize; @@ -40,8 +42,11 @@ impl Executable for CleanCommand { return Ok(()); } - let mut files_to_remove = Vec::new(); + let mut paths_to_remove = collect_orphaned_worker_worktrees(&cwd, &data_dir)?; let mut total_size: u64 = 0; + for path in &paths_to_remove { + total_size += path_size(path); + } // Find database files if --all if self.all { @@ -56,13 +61,13 @@ impl Executable for CleanCommand { if let Ok(meta) = entry.metadata() { total_size += meta.len(); } - files_to_remove.push(path); + paths_to_remove.push(path); } } } } - if files_to_remove.is_empty() { + if paths_to_remove.is_empty() { println!("{}", "Nothing to clean.".green()); return Ok(()); } @@ -77,18 +82,18 @@ impl Executable for CleanCommand { }; println!( - "{} {} files ({}):", + "{} {} paths ({}):", if self.dry_run { "Would remove" } else { "Removing" }, - files_to_remove.len(), + paths_to_remove.len(), size_str ); println!(); - for path in &files_to_remove { + for path in &paths_to_remove { let relative = path.strip_prefix(&cwd).unwrap_or(path); println!(" {}", relative.display()); } @@ -98,14 +103,18 @@ impl Executable for CleanCommand { println!("{}", "Dry run - no files were deleted.".yellow()); println!("Run without --dry-run to actually delete files."); } else { - for path in &files_to_remove { - std::fs::remove_file(path)?; + for path in &paths_to_remove { + if path.is_dir() { + std::fs::remove_dir_all(path)?; + } else { + std::fs::remove_file(path)?; + } } println!( - "{} {} files, freed {}", + "{} {} paths, freed {}", "Cleaned".green(), - files_to_remove.len(), + paths_to_remove.len(), size_str ); } @@ -113,3 +122,82 @@ impl Executable for CleanCommand { Ok(()) } } + +fn collect_orphaned_worker_worktrees( + cwd: &std::path::Path, + data_dir: &std::path::Path, +) -> Result> { + let work_root = data_dir.join("work"); + if !work_root.exists() { + return Ok(Vec::new()); + } + + let active = active_git_worktree_paths(cwd).unwrap_or_default(); + let mut orphaned = Vec::new(); + for entry in std::fs::read_dir(&work_root)? { + let path = entry?.path(); + if !path.is_dir() { + continue; + } + let canonical = canonicalize_lossy(&path); + if !active.contains(&canonical) { + orphaned.push(path); + } + } + Ok(orphaned) +} + +fn active_git_worktree_paths(cwd: &std::path::Path) -> Result> { + let output = Command::new("git") + .arg("-C") + .arg(cwd) + .args(["worktree", "list", "--porcelain"]) + .output() + .wrap_err("failed to run git worktree list")?; + if !output.status.success() { + return Err(eyre::eyre!( + "git worktree list failed: {}", + String::from_utf8_lossy(&output.stderr).trim() + )); + } + Ok(String::from_utf8_lossy(&output.stdout) + .lines() + .filter_map(|line| line.strip_prefix("worktree ")) + .map(PathBuf::from) + .map(|path| canonicalize_lossy(&path)) + .collect()) +} + +fn canonicalize_lossy(path: &std::path::Path) -> PathBuf { + path.canonicalize().unwrap_or_else(|_| path.to_path_buf()) +} + +fn path_size(path: &std::path::Path) -> u64 { + if path.is_file() { + return path.metadata().map(|meta| meta.len()).unwrap_or(0); + } + let mut total = 0; + let Ok(entries) = std::fs::read_dir(path) else { + return total; + }; + for entry in entries.flatten() { + total += path_size(&entry.path()); + } + total +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn clean_collects_orphaned_worker_worktrees_only() { + let temp = tempfile::tempdir().unwrap(); + let cwd = temp.path(); + std::fs::create_dir_all(cwd.join(".octos/work/orphan/.octos")).unwrap(); + std::fs::write(cwd.join(".octos/work/orphan/file.txt"), "leftover").unwrap(); + + let orphaned = collect_orphaned_worker_worktrees(cwd, &cwd.join(".octos")).unwrap(); + assert_eq!(orphaned, vec![cwd.join(".octos/work/orphan")]); + } +} diff --git a/crates/octos-core/src/session_scope.rs b/crates/octos-core/src/session_scope.rs index bc87beec21..4b697e4b60 100644 --- a/crates/octos-core/src/session_scope.rs +++ b/crates/octos-core/src/session_scope.rs @@ -644,6 +644,34 @@ impl SessionScope { &self.workspace } + /// Return a scope with the same root and mode policy but a different + /// per-session workspace. + /// + /// Spawn isolation uses this to inherit the parent's filesystem policy + /// while moving a child agent's CWD into its git worktree. The replacement + /// workspace must be absolute and remain under the existing root so + /// multi-tenant and solo boundaries are preserved. + pub fn with_workspace(mut self, workspace: PathBuf) -> Result { + if !workspace.is_absolute() { + return Err(SessionScopeError::RootNotAbsolute(workspace)); + } + let canonical_root = self + .root + .canonicalize() + .unwrap_or_else(|_| self.root.clone()); + let canonical_workspace = workspace + .canonicalize() + .unwrap_or_else(|_| workspace.clone()); + if !workspace.starts_with(&self.root) && !canonical_workspace.starts_with(&canonical_root) { + return Err(SessionScopeError::WorkspaceEscapesRoot { + root: self.root.clone(), + workspace, + }); + } + self.workspace = workspace; + Ok(self) + } + /// Return the declared cross-session shared zones (multi-tenant /// only). Empty slice for solo. Callers should prefer this over /// reaching into [`ScopeMode::MultiTenant::shared_zones`] @@ -1117,6 +1145,32 @@ mod tests { assert!(scope.shared_zones().is_empty()); } + #[test] + fn with_workspace_preserves_root_and_mode_policy() { + let cwd = abs("/home/yc/my-project"); + let child = cwd.join(".octos/work/subagent-0"); + let scope = SessionScope::solo(cwd.clone(), vec![]) + .unwrap() + .with_workspace(child.clone()) + .unwrap(); + assert_eq!(scope.root(), cwd); + assert_eq!(scope.workspace(), child); + assert!(matches!(scope.mode(), ScopeMode::Solo { .. })); + } + + #[test] + fn with_workspace_rejects_workspace_escape() { + let cwd = abs("/home/yc/my-project"); + let err = SessionScope::solo(cwd, vec![]) + .unwrap() + .with_workspace(abs("/home/yc/other-project")) + .unwrap_err(); + assert!(matches!( + err, + SessionScopeError::WorkspaceEscapesRoot { .. } + )); + } + #[test] fn refuses_relative_root() { let err = SessionScope::multi_tenant_with_default_zones( From 76d6ff6f98325689f563cc18bddb31d024a45ab4 Mon Sep 17 00:00:00 2001 From: ymote <151983+ymote@users.noreply.github.com> Date: Sun, 21 Jun 2026 01:44:33 +0800 Subject: [PATCH 2/2] fix(spawn): partial pre-merge hardening (2 of 3 P1s) Resolves the first two codex-gate P1s on the worktree-isolation path; a third (validate-worktree-path-before-git-creates-it) remains and needs restructuring, so this PR is NOT yet mergeable. - session_scope::with_workspace: reject `..` and require the workspace to be under root by CANONICAL form (canonicalize_lossy walks ancestors), closing the lexical-only escape + the missing-symlinked-leaf hole. - spawn: only rebind the session scope for Worktree isolation; shared spawns keep the parent scope (default spawns were failing in scoped gateway sessions). Tests: with_workspace_rejects_{parent_dir,symlinked_workspace}_escape. --- crates/octos-agent/src/tools/spawn.rs | 33 ++++++----- crates/octos-core/src/session_scope.rs | 80 +++++++++++++++++++++++--- 2 files changed, 92 insertions(+), 21 deletions(-) diff --git a/crates/octos-agent/src/tools/spawn.rs b/crates/octos-agent/src/tools/spawn.rs index 8408679489..47e347cc84 100644 --- a/crates/octos-agent/src/tools/spawn.rs +++ b/crates/octos-agent/src/tools/spawn.rs @@ -2647,20 +2647,27 @@ impl Tool for SpawnTool { .as_ref() .map(|worktree| worktree.path.clone()) .unwrap_or_else(|| self.working_dir.clone()); - let child_session_scope = match scoped_child_session_scope( - ctx.session_scope.as_ref(), - &child_working_dir, - ) { - Ok(scope) => scope, - Err(error) => { - return Ok(ToolResult { - output: format!( - "Status: FAILED\nfailed to inherit session scope for worker worktree: {error}" - ), - success: false, - ..Default::default() - }); + // Only rebind the session scope when this spawn gets its OWN git + // worktree. A shared spawn runs in the parent's workspace, so rebasing + // the scope onto `self.working_dir` — which in gateway/session-actor + // wiring is the factory cwd, outside the per-profile scope root — would + // wrongly fail the default (shared) spawn. Shared keeps the parent scope. + let child_session_scope = match worker_worktree.as_ref() { + Some(worktree) => { + match scoped_child_session_scope(ctx.session_scope.as_ref(), &worktree.path) { + Ok(scope) => scope, + Err(error) => { + return Ok(ToolResult { + output: format!( + "Status: FAILED\nfailed to inherit session scope for worker worktree: {error}" + ), + success: false, + ..Default::default() + }); + } + } } + None => ctx.session_scope.clone(), }; let sub_llm = self.resolve_sub_provider(input.model.as_deref(), input.context_window)?; diff --git a/crates/octos-core/src/session_scope.rs b/crates/octos-core/src/session_scope.rs index 4b697e4b60..75afc97b55 100644 --- a/crates/octos-core/src/session_scope.rs +++ b/crates/octos-core/src/session_scope.rs @@ -655,14 +655,36 @@ impl SessionScope { if !workspace.is_absolute() { return Err(SessionScopeError::RootNotAbsolute(workspace)); } - let canonical_root = self - .root - .canonicalize() - .unwrap_or_else(|_| self.root.clone()); - let canonical_workspace = workspace - .canonicalize() - .unwrap_or_else(|_| workspace.clone()); - if !workspace.starts_with(&self.root) && !canonical_workspace.starts_with(&canonical_root) { + // Reject `..` outright: `Path::starts_with` is purely lexical, so + // `/../escape` passes it while resolving outside the root — and + // `canonicalize` can't catch that when the leaf doesn't exist yet (it + // falls back to the unresolved path). + if workspace + .components() + .any(|component| matches!(component, std::path::Component::ParentDir)) + { + return Err(SessionScopeError::WorkspaceEscapesRoot { + root: self.root.clone(), + workspace, + }); + } + // Resolve symlinks on both sides, walking existing ancestors when the + // workspace leaf doesn't exist yet (`canonicalize_lossy` requires the + // `..` guard above). The canonical workspace must stay under the + // canonical root: the canonical form is the security-relevant one — a + // symlinked component (e.g. `/.octos/work` -> outside) only shows + // up after resolution, and the ancestor walk closes the missing-leaf + // hole that a raw `.canonicalize().unwrap_or(raw)` fallback would leave + // open. A lexical `starts_with` is NOT used — neither necessary (macOS + // `/var` vs `/private/var`) nor sufficient (the symlink escape bypassed + // it). + // Use the SAME ancestor-walking resolver on both sides so they resolve + // any shared existing ancestor identically (mixing a non-walking root + // resolver with a walking workspace resolver diverges for not-yet-created + // paths). + let canonical_root = canonicalize_lossy(&self.root); + let canonical_workspace = canonicalize_lossy(&workspace); + if !canonical_workspace.starts_with(&canonical_root) { return Err(SessionScopeError::WorkspaceEscapesRoot { root: self.root.clone(), workspace, @@ -1171,6 +1193,48 @@ mod tests { )); } + #[test] + fn with_workspace_rejects_parent_dir_escape() { + // `/../escape` lexically passes `starts_with(root)` but resolves + // outside; the `..` guard must reject it. + let cwd = abs("/home/yc/my-project"); + let err = SessionScope::solo(cwd.clone(), vec![]) + .unwrap() + .with_workspace(cwd.join("../other/subagent")) + .unwrap_err(); + assert!(matches!( + err, + SessionScopeError::WorkspaceEscapesRoot { .. } + )); + } + + #[cfg(unix)] + #[test] + fn with_workspace_rejects_symlinked_workspace_escape() { + // A workspace lexically under root but whose component is a symlink + // resolving OUTSIDE root must be rejected by the canonical check — + // the original lexical-only acceptance was a sandbox escape (#1250). + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path().join("root"); + let outside = tmp.path().join("outside"); + std::fs::create_dir_all(root.join(".octos")).unwrap(); + std::fs::create_dir_all(&outside).unwrap(); + // root/.octos/work -> outside (symlinked component) + std::os::unix::fs::symlink(&outside, root.join(".octos/work")).unwrap(); + // Lexically `/.octos/work/subagent`; canonically `/subagent`. + let escaping = root.join(".octos/work/subagent"); + std::fs::create_dir_all(&escaping).unwrap(); + let root = root.canonicalize().unwrap(); + let err = SessionScope::solo(root, vec![]) + .unwrap() + .with_workspace(escaping) + .unwrap_err(); + assert!(matches!( + err, + SessionScopeError::WorkspaceEscapesRoot { .. } + )); + } + #[test] fn refuses_relative_root() { let err = SessionScope::multi_tenant_with_default_zones(