diff --git a/crates/tools/src/bash.rs b/crates/tools/src/bash.rs index a8cff604..8224472b 100644 --- a/crates/tools/src/bash.rs +++ b/crates/tools/src/bash.rs @@ -5,9 +5,7 @@ use agen::tool::{Tool, ToolDefinition, ToolError, ToolMeta, ToolOutput}; use async_trait::async_trait; use schemars::JsonSchema; use serde::Deserialize; -use workdir::{ - CommandHandle, CommandOutputRequest, CommandRequest, WorkdirPath, WorkdirSessionHandle, -}; +use workdir::{CommandHandle, CommandOutputRequest, CommandRequest, WorkdirSessionHandle}; const DEFAULT_TIMEOUT_SECS: u64 = 120; const MAX_TIMEOUT_SECS: u64 = 600; @@ -16,10 +14,6 @@ const INLINE_BYTE_BUDGET: usize = 12 * 1024; #[derive(Debug, Deserialize, JsonSchema)] struct BashParams { command: String, - /// Optional logical working directory relative to the bound session cwd. - /// Supplying it lets delegation guards prove the command is disjoint from child write scopes. - #[serde(default)] - cwd: Option, #[serde(default)] timeout: Option, } @@ -57,18 +51,11 @@ impl Tool for BashTool { .timeout .unwrap_or(DEFAULT_TIMEOUT_SECS) .clamp(1, MAX_TIMEOUT_SECS); - let cwd = params - .cwd - .as_deref() - .map(WorkdirPath::new) - .transpose() - .map_err(crate::ToolsError::from)?; let cmd_summary = truncate_for_summary(¶ms.command); let handle = self .session .start_command(CommandRequest { command: params.command, - cwd, timeout_secs, output_limit: INLINE_BYTE_BUDGET, tool_call_id: Some(ctx.call_id), diff --git a/crates/tools/tests/integration.rs b/crates/tools/tests/integration.rs index fd444c7a..f8761aa8 100644 --- a/crates/tools/tests/integration.rs +++ b/crates/tools/tests/integration.rs @@ -390,18 +390,6 @@ async fn bash_inherits_workdir_cwd() { assert_eq!(actual, expected); } -#[tokio::test] -async fn bash_uses_explicit_logical_cwd() { - let (dir, _spill, reg) = setup(); - std::fs::create_dir_all(dir.path().join("nested")).unwrap(); - let bash = reg.get("Bash"); - let out = call(&bash, json!({ "command": "pwd", "cwd": "nested" })).await; - let body = out.content.unwrap(); - let actual = std::fs::canonicalize(body.trim()).unwrap(); - let expected = std::fs::canonicalize(dir.path().join("nested")).unwrap(); - assert_eq!(actual, expected); -} - #[tokio::test] async fn bash_provider_output_does_not_expose_internal_paths() { let (_dir, spill, reg) = setup(); diff --git a/crates/workdir/src/delegation.rs b/crates/workdir/src/delegation.rs index 4ee2b356..031ac9b5 100644 --- a/crates/workdir/src/delegation.rs +++ b/crates/workdir/src/delegation.rs @@ -251,43 +251,8 @@ impl DelegatingWorkdirSession { self.ensure_path(path, WorkdirDelegationPermission::Write) } - fn resolve_command_cwd(&self, cwd: Option<&FsPath>) -> Result { - match cwd { - Some(cwd) => self.resolve_path(cwd), - None => Ok(self.cwd.clone()), - } - } - - fn ensure_command_start(&self, cwd: &FsPath) -> Result<(), WorkdirError> { - self.ensure_capability(WorkdirSessionCapability::Command, "command execution")?; - if let Some(scope) = &self.scope - && !scope.iter().any(|rule| { - rule.permission == WorkdirDelegationPermission::Write - && rule_allows_path(rule, cwd, WorkdirDelegationPermission::Write) - }) - { - return Err(WorkdirError::Denied(format!( - "command cwd `{cwd}` is outside the delegated write scope" - ))); - } - - let mut leases = self - .child_write_leases - .lock() - .expect("workdir delegation lease mutex poisoned"); - leases.retain(|_, lease| lease.validity.upgrade().is_some_and(|v| v.is_active())); - if leases.values().any(|lease| { - lease.rules.iter().any(|rule| { - rule.permission == WorkdirDelegationPermission::Write - && command_cwd_overlaps_rule(cwd, rule) - }) - }) { - Err(WorkdirError::Denied(format!( - "command cwd `{cwd}` overlaps a child write delegation" - ))) - } else { - Ok(()) - } + fn ensure_command(&self) -> Result<(), WorkdirError> { + self.ensure_capability(WorkdirSessionCapability::Command, "command execution") } fn ensure_parent_write_available(&self, path: &FsPath) -> Result<(), WorkdirError> { @@ -517,20 +482,13 @@ impl WorkdirSession for DelegatingWorkdirSession { self.source.grep(request).await } - async fn start_command( - &self, - mut request: CommandRequest, - ) -> Result { - let cwd = self.resolve_command_cwd(request.cwd.as_ref())?; - self.ensure_command_start(&cwd)?; - if !self.source.transports_delegation_context() { - request.cwd = Some(cwd); - } + async fn start_command(&self, request: CommandRequest) -> Result { + self.ensure_command()?; self.source.start_command(request).await } async fn command_status(&self, handle: CommandHandle) -> Result { - self.ensure_capability(WorkdirSessionCapability::Command, "command execution")?; + self.ensure_command()?; self.source.command_status(handle).await } @@ -538,12 +496,12 @@ impl WorkdirSession for DelegatingWorkdirSession { &self, request: CommandOutputRequest, ) -> Result { - self.ensure_capability(WorkdirSessionCapability::Command, "command execution")?; + self.ensure_command()?; self.source.command_output(request).await } async fn cancel_command(&self, handle: CommandHandle) -> Result<(), WorkdirError> { - self.ensure_capability(WorkdirSessionCapability::Command, "command execution")?; + self.ensure_command()?; self.source.cancel_command(handle).await } @@ -671,11 +629,6 @@ impl WorkdirSession for ReadOnlyWorkdirSession { } } -fn command_cwd_overlaps_rule(cwd: &FsPath, rule: &WorkdirDelegationRule) -> bool { - rule_allows_path(rule, cwd, WorkdirDelegationPermission::Write) - || Path::new(rule.target.as_str()).starts_with(Path::new(cwd.as_str())) -} - fn rule_allows_path( rule: &WorkdirDelegationRule, path: &FsPath, @@ -780,6 +733,31 @@ mod tests { } } + async fn run_command( + session: &WorkdirSessionHandle, + command: impl Into, + tool_call_id: impl Into, + ) -> CommandOutput { + let handle = session + .start_command(CommandRequest { + command: command.into(), + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some(tool_call_id.into()), + }) + .await + .unwrap(); + session + .command_output(CommandOutputRequest { + handle, + cursor: 0, + limit: 1024, + wait: true, + }) + .await + .unwrap() + } + #[tokio::test] async fn delegation_capable_session_forwards_command_telemetry() { let root = TempDir::new().unwrap(); @@ -790,7 +768,6 @@ mod tests { let handle = parent .start_command(CommandRequest { command: "printf ready; sleep 0.2; printf done".into(), - cwd: None, timeout_secs: 5, output_limit: 1024, tool_call_id: Some("tool-delegated".into()), @@ -874,6 +851,18 @@ mod tests { ); assert!(child.scoped_session.subscribe_command_events().is_none()); assert!(child.scoped_session.command_snapshot().is_empty()); + assert!(matches!( + child + .scoped_session + .start_command(CommandRequest { + command: "printf denied".into(), + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("read-only-command".into()), + }) + .await, + Err(WorkdirError::Denied(_)) + )); } #[cfg(unix)] @@ -945,18 +934,6 @@ mod tests { .await, Err(WorkdirError::Denied(_)) )); - assert!(matches!( - parent - .start_command(CommandRequest { - command: "printf escaped".into(), - cwd: Some(fs_path("granted/outside")), - timeout_secs: 5, - output_limit: 1024, - tool_call_id: Some("symlink-cwd".into()), - }) - .await, - Err(WorkdirError::Denied(message)) if message.contains("traverses a symlink") - )); parent .write(write("secret/parent", "still-authoritative")) .await @@ -964,7 +941,7 @@ mod tests { } #[tokio::test] - async fn write_lease_blocks_only_overlapping_parent_command_cwds_until_release() { + async fn write_lease_keeps_typed_parent_writes_exclusive_without_blocking_commands() { let root = TempDir::new().unwrap(); fs::create_dir_all(root.path().join("leased")).unwrap(); fs::create_dir_all(root.path().join("other")).unwrap(); @@ -978,86 +955,24 @@ mod tests { .capabilities .supports(WorkdirSessionCapability::Command) ); - let command = child - .scoped_session - .start_command(CommandRequest { - command: "pwd; printf child-command".into(), - cwd: None, - timeout_secs: 5, - output_limit: 1024, - tool_call_id: Some("delegated-child-command".into()), - }) - .await - .unwrap(); - let command_output = child - .scoped_session - .command_output(CommandOutputRequest { - handle: command, - cursor: 0, - limit: 1024, - wait: true, - }) - .await - .unwrap(); - assert!( - command_output.content.ends_with("leased\nchild-command"), - "child command must run from its delegated cwd: {}", - command_output.content - ); - let denied = parent - .start_command(CommandRequest { - command: "printf parent-command".into(), - cwd: None, - timeout_secs: 5, - output_limit: 1024, - tool_call_id: Some("blocked-parent-command".into()), - }) - .await - .unwrap_err(); - assert!(matches!( - denied, - WorkdirError::Denied(message) - if message.contains("command cwd `.` overlaps a child write delegation") - )); - let denied = parent - .start_command(CommandRequest { - command: "printf still-denied".into(), - cwd: Some(fs_path("leased")), - timeout_secs: 5, - output_limit: 1024, - tool_call_id: Some("overlapping-parent-command".into()), - }) - .await - .unwrap_err(); - assert!(matches!( - denied, - WorkdirError::Denied(message) - if message.contains("command cwd `leased` overlaps a child write delegation") - )); - - let unrelated = parent - .start_command(CommandRequest { - command: "pwd; printf parent-command".into(), - cwd: Some(fs_path("other")), - timeout_secs: 5, - output_limit: 1024, - tool_call_id: Some("unrelated-parent-command".into()), - }) - .await - .unwrap(); - let unrelated_output = parent - .command_output(CommandOutputRequest { - handle: unrelated, - cursor: 0, - limit: 1024, - wait: true, - }) - .await - .unwrap(); - assert!( - unrelated_output.content.ends_with("other\nparent-command"), - "parent command must run from its explicit disjoint cwd: {}", - unrelated_output.content + let child_output = run_command( + &child.scoped_session, + "printf child-command", + "delegated-child-command", + ) + .await; + assert_eq!(child_output.content, "child-command"); + let parent_output = run_command( + &parent, + "printf parent-write > leased/from-command; printf parent-command", + "parent-command-during-child-write", + ) + .await; + assert_eq!(parent_output.status, CommandStatus::Completed); + assert_eq!(parent_output.content, "parent-command"); + assert_eq!( + fs::read_to_string(root.path().join("leased/from-command")).unwrap(), + "parent-write" ); assert!(matches!( @@ -1071,26 +986,18 @@ mod tests { .await .unwrap(); child.release(); - let resumed = parent - .start_command(CommandRequest { - command: "printf resumed".into(), - cwd: None, - timeout_secs: 5, - output_limit: 1024, - tool_call_id: Some("resumed-parent-command".into()), - }) - .await - .unwrap(); - let resumed_output = parent - .command_output(CommandOutputRequest { - handle: resumed, - cursor: 0, - limit: 1024, - wait: true, - }) - .await - .unwrap(); - assert_eq!(resumed_output.content, "resumed"); + assert!(matches!( + child + .scoped_session + .start_command(CommandRequest { + command: "printf revoked".into(), + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("revoked-child-command".into()), + }) + .await, + Err(WorkdirError::SessionClosed) + )); parent .write(write("leased/parent", "parent")) .await @@ -1143,150 +1050,75 @@ mod tests { } #[tokio::test] - async fn nested_write_delegation_uses_each_session_cwd_without_widening_scope() { + async fn nested_write_leases_do_not_block_command_capable_ancestors() { let root = TempDir::new().unwrap(); - fs::create_dir_all(root.path().join("top/nested")).unwrap(); - fs::create_dir_all(root.path().join("top/peer")).unwrap(); - fs::create_dir_all(root.path().join("other")).unwrap(); + fs::create_dir_all(root.path().join("docs/sub")).unwrap(); let root_session = session(root.path()); let child = root_session - .delegate(request("top", WorkdirDelegationPermission::Write)) + .delegate(request("docs", WorkdirDelegationPermission::Write)) .await .unwrap(); let nested = child .scoped_session - .delegate(WorkdirDelegationRequest { - rules: vec![WorkdirDelegationRule { - target: fs_path("top/nested"), - permission: WorkdirDelegationPermission::Write, - recursive: true, - }], - cwd: fs_path("top/nested"), - }) + .delegate(request("docs/sub", WorkdirDelegationPermission::Write)) .await .unwrap(); - let nested_handle = nested - .scoped_session - .start_command(CommandRequest { - command: "pwd; printf nested".into(), - cwd: None, - timeout_secs: 5, - output_limit: 1024, - tool_call_id: Some("nested-command".into()), - }) - .await - .unwrap(); - let nested_output = nested - .scoped_session - .command_output(CommandOutputRequest { - handle: nested_handle, - cursor: 0, - limit: 1024, - wait: true, - }) - .await - .unwrap(); - assert!(nested_output.content.ends_with("top/nested\nnested")); + for (session, label) in [ + (&root_session, "root"), + (&child.scoped_session, "child"), + (&nested.scoped_session, "nested"), + ] { + let output = run_command( + session, + format!("printf {label}"), + format!("{label}-command-during-nested-write"), + ) + .await; + assert_eq!(output.status, CommandStatus::Completed); + assert_eq!(output.content, label); + } - let denied = child - .scoped_session - .start_command(CommandRequest { - command: "printf blocked".into(), - cwd: None, - timeout_secs: 5, - output_limit: 1024, - tool_call_id: Some("nested-overlap".into()), - }) - .await - .unwrap_err(); assert!(matches!( - denied, - WorkdirError::Denied(message) - if message.contains("command cwd `top` overlaps a child write delegation") + root_session.write(write("docs/root", "blocked")).await, + Err(WorkdirError::Denied(_)) )); - - let peer_handle = child + assert!(matches!( + child + .scoped_session + .write(write("sub/child", "blocked")) + .await, + Err(WorkdirError::Denied(_)) + )); + nested .scoped_session - .start_command(CommandRequest { - command: "pwd; printf peer".into(), - cwd: Some(fs_path("peer")), - timeout_secs: 5, - output_limit: 1024, - tool_call_id: Some("nested-peer".into()), - }) + .write(write("nested", "allowed")) .await .unwrap(); - let peer_output = child - .scoped_session - .command_output(CommandOutputRequest { - handle: peer_handle, - cursor: 0, - limit: 1024, - wait: true, - }) - .await - .unwrap(); - assert!(peer_output.content.ends_with("top/peer\npeer")); - - let outside_handle = root_session - .start_command(CommandRequest { - command: "pwd; printf outside".into(), - cwd: Some(fs_path("other")), - timeout_secs: 5, - output_limit: 1024, - tool_call_id: Some("root-outside".into()), - }) - .await - .unwrap(); - let outside_output = root_session - .command_output(CommandOutputRequest { - handle: outside_handle, - cursor: 0, - limit: 1024, - wait: true, - }) - .await - .unwrap(); - assert!(outside_output.content.ends_with("other\noutside")); nested.release(); child.release(); } #[tokio::test] - async fn reapplied_delegation_chain_preserves_command_cwd() { + async fn reapplied_write_delegation_chain_forwards_command_lifecycle() { let root = TempDir::new().unwrap(); fs::create_dir_all(root.path().join("delegated")).unwrap(); - let parent = session(root.path()); let applied = apply_delegation_chain( - parent, + session(root.path()), [request("delegated", WorkdirDelegationPermission::Write)], ) .await .unwrap(); - let scoped = &applied.scoped_session; - let handle = scoped - .start_command(CommandRequest { - command: "pwd; printf reapplied".into(), - cwd: None, - timeout_secs: 5, - output_limit: 1024, - tool_call_id: Some("reapplied-command".into()), - }) - .await - .unwrap(); - let output = scoped - .command_output(CommandOutputRequest { - handle, - cursor: 0, - limit: 1024, - wait: true, - }) - .await - .unwrap(); - assert!(output.content.ends_with("delegated\nreapplied")); + let output = run_command( + &applied.scoped_session, + "printf reapplied", + "reapplied-command", + ) + .await; + assert_eq!(output.status, CommandStatus::Completed); + assert_eq!(output.content, "reapplied"); } #[tokio::test] @@ -1333,6 +1165,17 @@ mod tests { .unwrap(); parent.close().await.unwrap(); + assert!(matches!( + parent + .start_command(CommandRequest { + command: "printf closed".into(), + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("closed-parent-command".into()), + }) + .await, + Err(WorkdirError::SessionClosed) + )); assert!(matches!( child.scoped_session.read(read("a")).await, Err(WorkdirError::SessionClosed) diff --git a/crates/workdir/src/local.rs b/crates/workdir/src/local.rs index 8b422970..16686cb4 100644 --- a/crates/workdir/src/local.rs +++ b/crates/workdir/src/local.rs @@ -548,40 +548,6 @@ impl LocalWorkdirSession { self.inner.root.join(path.as_str()) } } - - fn resolve_command_cwd(&self, cwd: Option<&WorkdirPath>) -> Result { - let Some(cwd) = cwd else { - return Ok(self.inner.cwd.clone()); - }; - let host_cwd = self.resolve(cwd); - let canonical_root = self - .inner - .root - .canonicalize() - .map_err(|error| WorkdirError::io(&self.inner.root, error))?; - let expected = if cwd.is_root() { - canonical_root - } else { - canonical_root.join(cwd.as_str()) - }; - let resolved = host_cwd - .canonicalize() - .map_err(|error| WorkdirError::io(&host_cwd, error))?; - if resolved != expected { - return Err(WorkdirError::Denied(format!( - "command cwd `{cwd}` traverses a symlink" - ))); - } - let scope = self.inner.scope.snapshot(); - if !scope.is_readable(&resolved) - || !std::fs::metadata(&resolved).is_ok_and(|metadata| metadata.is_dir()) - { - return Err(WorkdirError::Denied(format!( - "command cwd `{cwd}` is not a readable Workdir directory" - ))); - } - Ok(resolved) - } } #[async_trait] @@ -727,7 +693,7 @@ impl WorkdirSession for LocalWorkdirSession { self.ensure_open()?; let id = self.inner.next_command_id.fetch_add(1, Ordering::Relaxed); let handle = CommandHandle(format!("command-{id}")); - let cwd = self.resolve_command_cwd(request.cwd.as_ref())?; + let cwd = self.inner.cwd.clone(); let (completion_tx, completion) = watch::channel(false); let command_id = handle.0.clone(); let telemetry = self.inner.command_telemetry.clone(); @@ -1472,7 +1438,6 @@ mod tests { &session, CommandRequest { command: "sleep 30".to_owned(), - cwd: None, timeout_secs: 60, output_limit: 1024, tool_call_id: None, @@ -1999,7 +1964,6 @@ mod tests { &workdir, CommandRequest { command: "pwd && printf provider-command".into(), - cwd: None, timeout_secs: 5, output_limit: 4096, tool_call_id: None, @@ -2035,7 +1999,6 @@ mod tests { &workdir, CommandRequest { command: "printf 'aéz'".into(), - cwd: None, timeout_secs: 5, output_limit: 1024, tool_call_id: None, @@ -2259,7 +2222,6 @@ mod tests { &workdir, CommandRequest { command: "printf ready; printf warning >&2; sleep 0.2; printf done".into(), - cwd: None, timeout_secs: 5, output_limit: 1024, tool_call_id: Some("tool-7".into()), @@ -2363,7 +2325,6 @@ mod tests { &workdir, CommandRequest { command: "sleep 30".into(), - cwd: None, timeout_secs: 1, output_limit: 1024, tool_call_id: None, @@ -2433,7 +2394,6 @@ mod tests { &workdir, CommandRequest { command: "sleep 30".into(), - cwd: None, timeout_secs: 60, output_limit: 1024, tool_call_id: None, diff --git a/crates/workdir/src/operation.rs b/crates/workdir/src/operation.rs index fe19763c..47527ad8 100644 --- a/crates/workdir/src/operation.rs +++ b/crates/workdir/src/operation.rs @@ -1,4 +1,3 @@ -use fs_operation::FsPath; use serde::{Deserialize, Serialize}; #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] @@ -8,10 +7,6 @@ pub struct CommandHandle(pub String); #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct CommandRequest { pub command: String, - /// Optional logical working directory relative to the calling session's cwd. - /// Providers must resolve and validate it before starting the process. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub cwd: Option, pub timeout_secs: u64, pub output_limit: usize, /// Optional caller-owned correlation id. Bash supplies its tool-call id so diff --git a/crates/worker/tests/controller_test.rs b/crates/worker/tests/controller_test.rs index c4a226a3..3e7e750b 100644 --- a/crates/worker/tests/controller_test.rs +++ b/crates/worker/tests/controller_test.rs @@ -230,7 +230,6 @@ async fn shutdown_closes_bound_workdir_session() { let command = session .start_command(CommandRequest { command: "sleep 30".to_owned(), - cwd: None, timeout_secs: 60, output_limit: 1024, tool_call_id: None, @@ -272,7 +271,6 @@ async fn controller_projects_workdir_command_events_and_snapshot_state() { let command = session .start_command(CommandRequest { command: "printf ready; sleep 0.3; printf done".to_owned(), - cwd: None, timeout_secs: 5, output_limit: 1024, tool_call_id: Some("tool-command-1".into()), @@ -380,7 +378,6 @@ async fn controller_refreshes_command_snapshot_after_high_output_provider_lag() .start_command(CommandRequest { command: "dd if=/dev/zero bs=8192 count=300 2>/dev/null | tr '\\0' x; sleep 5" .to_owned(), - cwd: None, timeout_secs: 10, output_limit: 1024, tool_call_id: Some("tool-high-output".into()), @@ -455,7 +452,6 @@ async fn controller_startup_failure_closes_bound_workdir_session() { session .start_command(CommandRequest { command: "printf unreachable".to_owned(), - cwd: None, timeout_secs: 5, output_limit: 1024, tool_call_id: None, diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index c18546fb..49909a57 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -15271,7 +15271,6 @@ mod tests { let provider_handle = source .start_command(workdir::CommandRequest { command: "printf ready; sleep 30".to_string(), - cwd: None, timeout_secs: 60, output_limit: 4096, tool_call_id: Some("tool-call-command-session".to_string()),