From 4555aace28fb744c16be559438556c778c05456e Mon Sep 17 00:00:00 2001 From: Hare Date: Tue, 20 Oct 2026 03:01:22 +0900 Subject: [PATCH] feat: capture bounded Bash output over replayable byte transport --- Cargo.lock | 1 + crates/standalone/src/host.rs | 16 +- .../journal/tests/connected_compaction.rs | 2 +- crates/tools/Cargo.toml | 3 +- crates/tools/src/bash.rs | 478 +++++---- crates/tools/src/bash_output.rs | 599 +++++++++++ crates/tools/src/lib.rs | 19 +- crates/tools/tests/edge_cases.rs | 46 +- crates/tools/tests/integration.rs | 502 ++++++--- crates/workdir/Cargo.toml | 2 +- crates/workdir/src/command_test_support.rs | 43 + crates/workdir/src/external.rs | 174 +++- crates/workdir/src/external_local_tests.rs | 20 +- crates/workdir/src/http.rs | 84 +- crates/workdir/src/lib.rs | 3 + crates/workdir/src/local.rs | 975 ++---------------- crates/workdir/src/local/command_transport.rs | 377 +++++++ .../src/local/command_transport/tests.rs | 490 +++++++++ crates/workdir/src/operation.rs | 181 +++- crates/workdir/src/router.rs | 19 +- crates/workdir/src/scope.rs | 333 +++++- .../tests/command_denial_diagnostics.rs | 5 +- crates/workdir/tests/denial_reasons.rs | 5 +- crates/worker-runtime/Cargo.toml | 1 + crates/worker-runtime/src/http_server.rs | 13 +- .../tests/command_transport_tests.rs | 408 ++++++++ crates/worker-runtime/src/worker_backend.rs | 60 +- crates/worker/examples/worker_protocol.rs | 4 +- crates/worker/src/bootstrap.rs | 65 +- crates/worker/src/checkout_wip_tests.rs | 6 +- crates/worker/src/controller.rs | 95 +- .../src/controller_subject_release_tests.rs | 1 - crates/worker/src/entrypoint.rs | 3 +- .../src/feature/builtin/manage_workdir.rs | 18 +- crates/worker/src/lib.rs | 2 +- crates/worker/src/spawn/registry.rs | 34 +- crates/worker/src/spawn/tool.rs | 75 +- crates/worker/src/subjektiv.rs | 4 - crates/worker/src/worker.rs | 2 - crates/worker/tests/compact_events_test.rs | 27 +- crates/worker/tests/controller_test.rs | 82 +- crates/workspace-server/src/server.rs | 576 ++++++++--- .../src/server/tests/api_logging_tests.rs | 19 +- .../server/tests/command_transport_tests.rs | 223 ++++ .../src/server_assignment_attention_tests.rs | 1 - .../src/server_assignment_delivery_tests.rs | 1 - 46 files changed, 4183 insertions(+), 1914 deletions(-) create mode 100644 crates/tools/src/bash_output.rs create mode 100644 crates/workdir/src/command_test_support.rs create mode 100644 crates/workdir/src/local/command_transport.rs create mode 100644 crates/workdir/src/local/command_transport/tests.rs create mode 100644 crates/worker-runtime/src/http_server/tests/command_transport_tests.rs create mode 100644 crates/workspace-server/src/server/tests/command_transport_tests.rs diff --git a/Cargo.lock b/Cargo.lock index 122e578dc..c0de6ab86 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5266,6 +5266,7 @@ dependencies = [ "thiserror 2.0.18", "tokio", "tracing", + "uuid", "workdir", ] diff --git a/crates/standalone/src/host.rs b/crates/standalone/src/host.rs index 60c239b91..6d55edb67 100644 --- a/crates/standalone/src/host.rs +++ b/crates/standalone/src/host.rs @@ -23,9 +23,7 @@ use session_store::{ CombinedStore, FsStore, FsWorkerStore, WorkerActiveSegmentRef, WorkerMetadataStore, }; use thiserror::Error; -use worker::bootstrap::{ - WorkerBootstrap, WorkerBootstrapError, WorkerBootstrapLayout, bash_output_dir_for_worker_id, -}; +use worker::bootstrap::{WorkerBootstrap, WorkerBootstrapError, WorkerBootstrapLayout}; use worker::controller::WorkerControllerTransport; use worker::ipc::protocol_session::{ WorkerProtocolSessionStreams, dispatch_worker_protocol_method, live_log_entry_event, @@ -216,7 +214,6 @@ impl StandaloneHost { WorkerFilesystemAuthority::local(launch.cwd.clone(), launch.cwd.clone()); let workspace_context = WorkerWorkspaceContext::local_filesystem(None); let runtime_base = store.runtime_dir(worker_id); - let bash_output_dir = bash_output_dir_for_worker_id(worker_id); let mut bootstrap = WorkerBootstrap::new( bootstrap_manifest, @@ -224,10 +221,7 @@ impl StandaloneHost { launch.prompt_catalog, workspace_context, filesystem_authority, - WorkerBootstrapLayout::Direct { - runtime_base, - bash_output_dir, - }, + WorkerBootstrapLayout::Direct { runtime_base }, WorkerControllerTransport::InProcess, ); if let Some(subject) = &subject { @@ -428,7 +422,6 @@ impl StandaloneHost { ); let workspace_context = WorkerWorkspaceContext::local_filesystem(None); let runtime_base = store.runtime_dir(worker_id); - let bash_output_dir = bash_output_dir_for_worker_id(worker_id); let mut bootstrap = WorkerBootstrap::new( manifest, @@ -436,10 +429,7 @@ impl StandaloneHost { worker::PromptCatalogSource::builtins_only(), workspace_context, filesystem_authority, - WorkerBootstrapLayout::Direct { - runtime_base, - bash_output_dir, - }, + WorkerBootstrapLayout::Direct { runtime_base }, WorkerControllerTransport::InProcess, ); if let Some(subject) = &subject { diff --git a/crates/standalone/src/subjektiv/journal/tests/connected_compaction.rs b/crates/standalone/src/subjektiv/journal/tests/connected_compaction.rs index 04032a8d7..7dc575f6d 100644 --- a/crates/standalone/src/subjektiv/journal/tests/connected_compaction.rs +++ b/crates/standalone/src/subjektiv/journal/tests/connected_compaction.rs @@ -126,7 +126,7 @@ async fn local_host_compaction_freezes_pending_journal_through_request_and_durab worker::PromptCatalogSource::builtins_only(), worker::WorkerWorkspaceContext::no_workspace(), worker::WorkerFilesystemAuthority::None, - worker::WorkerBootstrapLayout::Direct { runtime_base: fixture._temp.path().join("runtime"), bash_output_dir: fixture._temp.path().join("bash") }, + worker::WorkerBootstrapLayout::Direct { runtime_base: fixture._temp.path().join("runtime") }, worker::WorkerControllerTransport::InProcess, ).with_model_client(model.clone()).with_subjektiv_host(fixture.host().clone()).prepare_restored("journal-test").await.unwrap(); assert_eq!(prepared.worker().session_id().to_string(), fixture.context.session_id); diff --git a/crates/tools/Cargo.toml b/crates/tools/Cargo.toml index f10a68197..22edee14c 100644 --- a/crates/tools/Cargo.toml +++ b/crates/tools/Cargo.toml @@ -23,7 +23,8 @@ thiserror = { workspace = true } tokio = { workspace = true, features = ["process", "rt", "sync", "time"] } tracing = { workspace = true } workdir = { workspace = true } +uuid = { workspace = true, features = ["v7"] } [dev-dependencies] filetime = "0.2.27" -tokio = { workspace = true, features = ["macros", "rt-multi-thread"] } +tokio = { workspace = true, features = ["macros", "rt-multi-thread", "test-util"] } diff --git a/crates/tools/src/bash.rs b/crates/tools/src/bash.rs index 801507c0b..ae911bb71 100644 --- a/crates/tools/src/bash.rs +++ b/crates/tools/src/bash.rs @@ -1,19 +1,19 @@ //! Bash Tool using WorkdirSession command capability. //! -//! The selected session owns cwd, process launch, bounded output, timeout, and cancellation. +//! Workdir owns launch and byte transport; Bash owns bounded preview and retained evidence. //! Bash is not a sandbox; the Runtime process and OS user define the execution boundary. //! Per-execution handles and nonces prevent cancellation of another command. Dropping a future -//! does not confirm termination; active registrations remain until cleanup requests cancellation -//! and observes terminal output. +//! does not confirm termination. Registrations remain during bounded cancellation/drain cleanup; +//! an unconfirmed provider outcome leaves partial evidence, never a fabricated terminal fence. use std::collections::{HashMap, HashSet}; -use std::path::PathBuf; use std::sync::{Arc, Mutex}; +use crate::bash_output::{BashOutputMetadata, BashOutputView, BashOutputWriter}; use agen::tool::{Tool, ToolDefinition, ToolError, ToolMeta, ToolOutput}; use async_trait::async_trait; use schemars::JsonSchema; -use serde::Deserialize; +use serde::{Deserialize, Serialize}; use workdir::{ CommandHandle, CommandOutputRequest, CommandRequest, WorkdirPath, WorkdirSessionHandle, WorkdirSessionRouter, @@ -35,9 +35,9 @@ struct BashParams { timeout: Option, } -pub(crate) struct BashTool { +pub struct BashTool { router: Arc, - output_dir: PathBuf, + output: BashOutputView, state: Arc>, } @@ -63,6 +63,8 @@ struct CommandGuard { execution_id: String, execution_nonce: u64, handle: Option, + cursor: u64, + retained: Option, } impl Drop for CommandGuard { @@ -74,20 +76,66 @@ impl Drop for CommandGuard { let state = Arc::clone(&self.state); let execution_id = self.execution_id.clone(); let execution_nonce = self.execution_nonce; + let mut retained = self.retained.take(); + let cursor = self.cursor; // A dropped provider future is not terminal confirmation. Keep the live // execution registered until cleanup has both requested cancellation and // observed terminal command output, so cancellation/session teardown // cannot race with an apparently empty registry. tokio::spawn(async move { - let _ = workdir.cancel_command(handle.clone()).await; - let _ = workdir - .command_output(CommandOutputRequest { - handle, - cursor: 0, - limit: INLINE_BYTE_BUDGET, - wait: true, - }) - .await; + let result = tokio::time::timeout(std::time::Duration::from_secs(10), async { + let _ = workdir.cancel_command(handle.clone()).await; + let mut cursor = cursor; + loop { + let page = command_page( + &workdir, + CommandOutputRequest { + handle: handle.clone(), + cursor, + limit: workdir::MAX_COMMAND_READ_BYTES, + wait: true, + }, + ) + .await?; + if let Some(writer) = &mut retained { + let mut next = cursor; + for chunk in &page.chunks { + if chunk.sequence != next { + return Err(workdir::WorkdirError::Unavailable( + "command cleanup sequence gap".into(), + )); + } + writer + .append_chunk(chunk) + .map_err(workdir::WorkdirError::Unavailable)?; + next = next.checked_add(chunk.bytes.len() as u64).ok_or_else(|| { + workdir::WorkdirError::Unavailable( + "command cleanup cursor overflow".into(), + ) + })?; + } + if page.next_cursor != next { + return Err(workdir::WorkdirError::Unavailable( + "command cleanup cursor gap".into(), + )); + } + } + cursor = page.next_cursor; + if let Some(terminal) = page.terminal { + if let Some(writer) = &mut retained { + writer + .finish_transport(&terminal) + .map_err(workdir::WorkdirError::Unavailable)?; + } + return Ok::<(), workdir::WorkdirError>(()); + } + } + }) + .await; + // Bounded cleanup can end with an unknown provider outcome. The writer then + // terminalizes partial evidence on drop; retaining a dead collector in `active` + // would leak registrations and falsely suggest it could still collect output. + let _ = result; let mut state = state.lock().unwrap(); if state .active @@ -101,15 +149,45 @@ impl Drop for CommandGuard { } } -#[async_trait] -impl Tool for BashTool { - async fn execute( +// Output pages are replayable only with the exact same command/cursor/limit. +// Retry read transport failures, never command start, and cap both attempts and +// backoff. The caller's deadline bounds a disconnected or unresponsive provider. +async fn command_page( + session: &workdir::WorkdirSessionHandle, + request: CommandOutputRequest, +) -> Result { + for attempt in 0..3 { + match session.command_output(request.clone()).await { + Err( + workdir::WorkdirError::Transport(_) + | workdir::WorkdirError::Unavailable(_) + | workdir::WorkdirError::OutcomeUnknown(_), + ) if attempt < 2 => { + tokio::time::sleep(std::time::Duration::from_millis(100 << attempt)).await; + } + result => return result, + } + } + unreachable!("bounded retry loop returns its last result") +} + +impl BashTool { + pub fn new(router: Arc, output: BashOutputView) -> Self { + Self { + router, + output, + state: Arc::new(Mutex::new(BashExecutionState::default())), + } + } + + pub async fn execute_result( &self, input_json: &str, ctx: agen::tool::ToolExecutionContext, - ) -> Result { + ) -> Result { let params: BashParams = serde_json::from_str(input_json) .map_err(|error| ToolError::InvalidArgument(format!("invalid Bash input: {error}")))?; + let retained = self.output.begin().map_err(ToolError::ExecutionFailed)?; let selected = crate::routing::resolve_session( &self.router, params.target_workdir.as_deref(), @@ -134,14 +212,15 @@ impl Tool for BashTool { execution_id: execution_id.clone(), execution_nonce, handle: None, + cursor: 0, + retained: Some(retained), }; let handle = session .start_command(CommandRequest { command: params.command, timeout_secs, - output_limit: INLINE_BYTE_BUDGET, + transport_byte_budget: self.output.byte_budget(), cwd: params.cwd, - spill_dir: Some(self.output_dir.clone()), tool_call_id: Some(call_id.clone()), }) .await @@ -167,15 +246,101 @@ impl Tool for BashTool { .await .map_err(crate::ToolsError::from)?; } - let output = session - .command_output(CommandOutputRequest { - handle, - cursor: 0, - limit: INLINE_BYTE_BUDGET, - wait: true, - }) + let mut cursor = 0; + let mut stdout_preview = Vec::new(); + let mut stderr_preview = Vec::new(); + let mut seen = [0_u64; 2]; + let mut empty_polls = 0; + let deadline = + tokio::time::Instant::now() + std::time::Duration::from_secs(timeout_secs + 30); + let terminal = loop { + let page = tokio::time::timeout_at( + deadline, + command_page( + &session, + CommandOutputRequest { + handle: handle.clone(), + cursor, + limit: workdir::MAX_COMMAND_READ_BYTES, + wait: true, + }, + ), + ) .await + .map_err(|_| { + ToolError::ExecutionFailed( + "command byte transport deadline exceeded; terminal outcome unknown".into(), + ) + })? .map_err(crate::ToolsError::from)?; + let mut next = cursor; + for chunk in &page.chunks { + if chunk.sequence != next || chunk.bytes.is_empty() { + return Err(ToolError::ExecutionFailed( + "command byte transport sequence gap".into(), + )); + } + next = next + .checked_add(chunk.bytes.len() as u64) + .filter(|n| *n <= self.output.byte_budget()) + .ok_or_else(|| { + ToolError::ExecutionFailed( + "command byte transport exceeds delivery budget".into(), + ) + })?; + let index = if chunk.stream == workdir::CommandStream::Stdout { + 0 + } else { + 1 + }; + if chunk.offset != seen[index] { + return Err(ToolError::ExecutionFailed( + "command byte transport offset gap".into(), + )); + } + seen[index] += chunk.bytes.len() as u64; + guard + .retained + .as_mut() + .unwrap() + .append_chunk(chunk) + .map_err(ToolError::ExecutionFailed)?; + let available = + INLINE_BYTE_BUDGET.saturating_sub(stdout_preview.len() + stderr_preview.len()); + let preview = if index == 0 { + &mut stdout_preview + } else { + &mut stderr_preview + }; + preview.extend_from_slice(&chunk.bytes[..available.min(chunk.bytes.len())]); + } + if page.next_cursor != next { + return Err(ToolError::ExecutionFailed( + "command byte transport cursor does not fence the page".into(), + )); + } + if page.chunks.is_empty() && page.terminal.is_none() { + empty_polls += 1; + if empty_polls > 32 { + return Err(ToolError::ExecutionFailed( + "command byte transport made no progress after bounded polls".into(), + )); + } + } else { + empty_polls = 0; + } + cursor = page.next_cursor; + guard.cursor = cursor; + if let Some(terminal) = page.terminal { + break terminal; + } + }; + let metadata = guard + .retained + .as_mut() + .unwrap() + .finish_transport(&terminal) + .map_err(ToolError::ExecutionFailed)?; let cancellation_requested = { let mut state = self.state.lock().unwrap(); let owns_registration = state @@ -193,41 +358,78 @@ impl Tool for BashTool { }; guard.handle = None; - let timed_out = output.timed_out; - let summary = if cancellation_requested { - format!("$ {cmd_summary} (cancelled)") - } else if output.timed_out { - format!("$ {cmd_summary} (timed out after {timeout_secs}s)") - } else { - match output.exit_code { - Some(0) => format!("$ {cmd_summary}"), - Some(code) => format!("$ {cmd_summary} (exit {code})"), - None => format!("$ {cmd_summary} (terminated)"), - } - }; - let content = if output.content.is_empty() { - None - } else if output.truncated { - let notice = match output.output_path { - Some(path) => format!( - "[showing bounded WorkdirSession command output; full output saved to {}]", - path.display() - ), - None => "[showing bounded WorkdirSession command output; additional output was truncated]" - .to_owned(), + let timed_out = terminal.status == workdir::CommandStatus::TimedOut; + let summary = + if cancellation_requested || terminal.status == workdir::CommandStatus::Cancelled { + format!("$ {cmd_summary} (cancelled)") + } else if timed_out { + format!("$ {cmd_summary} (timed out after {timeout_secs}s)") + } else { + match terminal.exit_code { + Some(0) => format!("$ {cmd_summary}"), + Some(code) => format!("$ {cmd_summary} (exit {code})"), + None => format!("$ {cmd_summary} (terminated)"), + } }; - Some(format!("{notice}\n{}", output.content)) - } else { - Some(output.content) - }; - let output = ToolOutput { + Ok(BashResult { summary, - content, - attachments: Vec::new(), - }; - if cancellation_requested { + stdout_preview: String::from_utf8_lossy(&stdout_preview).into_owned(), + stderr_preview: String::from_utf8_lossy(&stderr_preview).into_owned(), + preview_omitted: terminal.stdout_bytes.saturating_add(terminal.stderr_bytes) + > (stdout_preview.len() + stderr_preview.len()) as u64, + metadata, + cancelled: cancellation_requested + || terminal.status == workdir::CommandStatus::Cancelled, + }) + } +} + +#[derive(Debug, Serialize)] +pub struct BashResult { + pub summary: String, + pub stdout_preview: String, + pub stderr_preview: String, + pub preview_omitted: bool, + pub metadata: BashOutputMetadata, + pub cancelled: bool, +} + +impl BashResult { + pub fn stdout_entry(&self) -> String { + format!("/bash-output/{}/stdout", self.metadata.id) + } + pub fn stderr_entry(&self) -> String { + format!("/bash-output/{}/stderr", self.metadata.id) + } + pub fn order_entry(&self) -> String { + format!("/bash-output/{}/order", self.metadata.id) + } + pub fn tool_output(&self) -> ToolOutput { + let content = serde_json::json!({ + "stdout_preview": self.stdout_preview, "stderr_preview": self.stderr_preview, + "preview_omitted": self.preview_omitted, "output": self.metadata, + "stdout_entry": self.stdout_entry(), "stderr_entry": self.stderr_entry(), "order_entry": self.order_entry(), + }); + ToolOutput { + summary: self.summary.clone(), + content: Some(content.to_string()), + attachments: vec![], + } + } +} + +#[async_trait] +impl Tool for BashTool { + async fn execute( + &self, + input: &str, + ctx: agen::tool::ToolExecutionContext, + ) -> Result { + let result = self.execute_result(input, ctx).await?; + let output = result.tool_output(); + if result.cancelled { Err(ToolError::Cancelled(output)) - } else if timed_out { + } else if result.metadata.terminal.status == Some(workdir::CommandStatus::TimedOut) { Err(ToolError::Interrupted(output)) } else { Ok(output) @@ -288,164 +490,20 @@ fn truncate_for_summary(command: &str) -> String { summary } -pub fn bash_tool(session: WorkdirSessionHandle, output_dir: PathBuf) -> ToolDefinition { - routed_bash_tool(crate::routing::singleton_router(session), output_dir) +pub fn bash_tool(session: WorkdirSessionHandle, output: BashOutputView) -> ToolDefinition { + routed_bash_tool(crate::routing::singleton_router(session), output) } pub(crate) fn routed_bash_tool( router: Arc, - output_dir: PathBuf, + output: BashOutputView, ) -> ToolDefinition { Arc::new(move || { let schema = schemars::schema_for!(BashParams); let meta = ToolMeta::new("Bash") - .description("Execute a shell command in one selected Workdir. Process start, bounded inline output, full-output spill, timeout and cancellation are owned by that WorkdirSession provider. This is not a sandbox.") + .description("Execute a shell command in one selected Workdir. Workdir owns launch, lossless bounded byte transport, timeout and cancel; Bash owns preview, quota-limited ephemeral output and read-only virtual references. This is not a sandbox.") .input_schema(serde_json::to_value(schema).expect("Bash schema serialization")); - let tool: Arc = Arc::new(BashTool { - router: router.clone(), - output_dir: output_dir.clone(), - state: Arc::new(Mutex::new(BashExecutionState::default())), - }); + let tool: Arc = Arc::new(BashTool::new(router.clone(), output.clone())); (meta, tool) }) } - -#[cfg(test)] -mod tests { - use std::sync::Arc; - - use manifest::{Permission, Scope, ScopeConfig, ScopeRule}; - use tempfile::TempDir; - use workdir::{LocalWorkdirSession, WorkdirSessionHandle}; - - use super::bash_tool; - use crate::{grep::grep_tool, read::read_tool, tracker::Tracker}; - - fn session_with_output_scope(root: &TempDir, output: &TempDir) -> WorkdirSessionHandle { - let scope = Scope::from_config(&ScopeConfig { - allow: vec![ - ScopeRule { - target: root.path().to_path_buf(), - permission: Permission::Write, - recursive: true, - symlink_policy: Default::default(), - }, - ScopeRule { - target: output.path().to_path_buf(), - permission: Permission::Read, - recursive: true, - symlink_policy: Default::default(), - }, - ], - deny: Vec::new(), - }) - .unwrap(); - Arc::new(LocalWorkdirSession::new(scope, root.path().to_path_buf())) - } - - #[tokio::test] - async fn long_output_is_spilled_and_available_to_read_and_grep() { - let root = TempDir::new().unwrap(); - let output = TempDir::new().unwrap(); - let session = session_with_output_scope(&root, &output); - let (_, bash) = bash_tool(session.clone(), output.path().to_path_buf())(); - let command = "i=0; while [ $i -lt 2000 ]; do printf 'line-%04d\\n' \"$i\"; i=$((i+1)); done; printf 'FINAL-NEEDLE\\n'"; - let result = bash - .execute( - &serde_json::json!({ "command": command, "cwd": "." }).to_string(), - Default::default(), - ) - .await - .unwrap(); - let rendered = result.content.expect("bounded Bash output"); - let artifact = std::fs::read_dir(output.path()) - .unwrap() - .next() - .expect("artifact entry") - .unwrap() - .path(); - - assert!(rendered.contains("full output saved to")); - assert!(rendered.contains(&artifact.display().to_string())); - let retained = std::fs::read_to_string(&artifact).unwrap(); - assert!(retained.starts_with("line-0000\n")); - assert!(retained.ends_with("FINAL-NEEDLE\n")); - assert_eq!(retained.lines().count(), 2001); - - let (_, read) = read_tool(session.clone(), Tracker::new())(); - let read_result = read - .execute( - &serde_json::json!({ - "file_path": artifact, - "offset": 2000, - "limit": 1, - }) - .to_string(), - Default::default(), - ) - .await - .unwrap(); - assert!( - read_result - .content - .expect("Read content") - .contains("FINAL-NEEDLE") - ); - - let (_, grep) = grep_tool(session)(); - let grep_result = grep - .execute( - &serde_json::json!({ - "pattern": "FINAL-NEEDLE", - "path": artifact, - "output_mode": "content", - }) - .to_string(), - Default::default(), - ) - .await - .unwrap(); - let grep_content = grep_result.content.expect("Grep content"); - assert!( - grep_content.contains("FINAL-NEEDLE"), - "unexpected Grep content: {grep_content:?}" - ); - } - - #[tokio::test] - async fn missing_cwd_is_rejected_before_command_start() { - let root = TempDir::new().unwrap(); - let output = TempDir::new().unwrap(); - let session = session_with_output_scope(&root, &output); - let (_, bash) = bash_tool(session, output.path().to_path_buf())(); - - let error = bash - .execute( - &serde_json::json!({ "command": "printf hidden" }).to_string(), - Default::default(), - ) - .await - .unwrap_err(); - - assert!(error.to_string().contains("missing field `cwd`")); - } - - #[tokio::test] - async fn short_output_does_not_leave_a_spill_artifact() { - let root = TempDir::new().unwrap(); - let output = TempDir::new().unwrap(); - let session = session_with_output_scope(&root, &output); - let (_, bash) = bash_tool(session, output.path().to_path_buf())(); - - let result = bash - .execute( - &serde_json::json!({ "command": "printf short", "cwd": "." }).to_string(), - Default::default(), - ) - .await - .unwrap(); - - assert_eq!(result.content.as_deref(), Some("short")); - assert_eq!(std::fs::read_dir(output.path()).unwrap().count(), 0); - } -} diff --git a/crates/tools/src/bash_output.rs b/crates/tools/src/bash_output.rs new file mode 100644 index 000000000..3f31fcb44 --- /dev/null +++ b/crates/tools/src/bash_output.rs @@ -0,0 +1,599 @@ +//! Bash-owned bounded output, independent of Workdir transport and Console subscribers. +//! +//! Output is ephemeral evidence, not a filesystem grant. Every lookup checks the live view; +//! children can see their own subtree and ancestors can retain evidence after child revocation. +//! Root teardown clears all bytes. A new root (including restore) has a new incarnation and +//! never reconstructs grants from history. Expiry is measured from admission, enforced on +//! lookup and by a weak reaper once commands run in Tokio. Parent and all descendants share +//! one pool: admission reserves a whole command budget, evicts the oldest terminal evidence +//! first, and refuses rather than evicting a live collector. Revoking a child denies access +//! but leaves ancestor evidence until pool eviction/expiry; root teardown clears the pool. + +use std::collections::VecDeque; +use std::sync::{ + Arc, Mutex, Weak, + atomic::{AtomicBool, Ordering}, +}; +use std::time::{Duration, Instant}; + +use serde::Serialize; +use workdir::{CommandStatus, CommandStream}; + +const COMMAND_BYTES: usize = 4 * 1024 * 1024; +const STORE_BYTES: usize = 32 * 1024 * 1024; +const STORE_COUNT: usize = 64; +const ORDER_COUNT: usize = 4096; +const RETENTION: Duration = Duration::from_secs(3600); + +#[derive(Clone)] +pub struct BashOutputView { + store: Arc>, + lineage: Vec, + active: Arc, + authority: Arc bool + Send + Sync>, +} + +struct Store { + records: VecDeque>>, + reserved: usize, + retention: Duration, + reaper_started: bool, + _shutdown: tokio::sync::watch::Sender<()>, +} + +struct Record { + id: String, + owner: Vec, + created: Instant, + stdout: Vec, + stderr: Vec, + order: Vec, + stdout_seen: u64, + stderr_seen: u64, + truncated: bool, + limit: Option, + transport_counts: Option<(u64, u64)>, + transport_truncation: Option, + terminal: Option, +} + +#[derive(Clone, Debug, Serialize)] +pub struct OutputRange { + pub stream: CommandStream, + pub start: u64, + pub end: u64, +} + +#[derive(Clone, Debug, Serialize)] +pub struct OutputTerminal { + pub status: Option, + pub exit_code: Option, + /// False on a forcibly dropped collector or failed transport: received bytes are retained, + /// but they are not a claim about the unobserved process tail. + pub transport_complete: bool, +} + +#[derive(Clone, Copy, Debug, Serialize)] +#[serde(rename_all = "snake_case")] +pub enum RetentionLimit { + ByteQuota, + OrderQuota, +} + +#[derive(Clone, Debug, Serialize)] +pub struct BashOutputMetadata { + pub id: String, + pub stdout_saved: usize, + pub stderr_saved: usize, + pub stdout_seen: u64, + pub stderr_seen: u64, + pub stdout_delivered: u64, + pub stderr_delivered: u64, + pub retention_limit: Option, + pub transport_truncation: Option, + pub storage_truncated: bool, + pub terminal: OutputTerminal, + pub text_encoding: &'static str, + pub ordering: &'static str, +} + +/// A bounded reservation acquired before command start. Drop terminalizes partial evidence. +pub struct BashOutputWriter { + record: Arc>, +} + +impl Default for BashOutputView { + fn default() -> Self { + Self::new() + } +} + +impl BashOutputView { + pub fn new() -> Self { + let (shutdown, _) = tokio::sync::watch::channel(()); + let store = Arc::new(Mutex::new(Store { + records: VecDeque::new(), + reserved: 0, + retention: RETENTION, + reaper_started: false, + _shutdown: shutdown, + })); + let view = Self { + store, + lineage: vec![uuid::Uuid::now_v7().to_string()], + active: Arc::new(AtomicBool::new(true)), + authority: Arc::new(|| true), + }; + view.start_reaper(); + view + } + + fn start_reaper(&self) { + let Ok(runtime) = tokio::runtime::Handle::try_current() else { + return; + }; + let mut store = self.store.lock().unwrap(); + if store.reaper_started { + return; + } + let receiver = store._shutdown.subscribe(); + store.reaper_started = true; + let weak = Arc::downgrade(&self.store); + runtime.spawn(reap(weak, receiver)); + } + + pub fn byte_budget(&self) -> u64 { + COMMAND_BYTES as u64 + } + + pub fn child(&self, authority: Arc bool + Send + Sync>) -> Self { + let parent = self.clone(); + let mut lineage = self.lineage.clone(); + lineage.push(uuid::Uuid::now_v7().to_string()); + Self { + store: self.store.clone(), + lineage, + active: Arc::new(AtomicBool::new(true)), + authority: Arc::new(move || parent.is_active() && authority()), + } + } + + pub fn is_active(&self) -> bool { + self.active.load(Ordering::Acquire) && (self.authority)() + } + + pub fn is_root(&self) -> bool { + self.lineage.len() == 1 + } + + pub fn revoke(&self) { + self.active.store(false, Ordering::Release); + if self.lineage.len() == 1 { + let mut store = self.store.lock().unwrap(); + store.records.clear(); + store.reserved = 0; + } + } + + pub fn begin(&self) -> Result { + if !self.is_active() { + return Err("Bash output authority revoked".into()); + } + // A host may construct the Feature before entering Tokio. Command admission + // happens inside the runtime, so idle expiry must start here as well. + self.start_reaper(); + let mut store = self.store.lock().unwrap(); + store.expire(Instant::now()); + while store.records.len() >= STORE_COUNT || store.reserved + COMMAND_BYTES > STORE_BYTES { + let Some(index) = store + .records + .iter() + .position(|r| r.lock().unwrap().terminal.is_some()) + else { + return Err("Bash output capacity is occupied by live commands".into()); + }; + store.records.remove(index); + store.reserved -= COMMAND_BYTES; + } + let record = Arc::new(Mutex::new(Record { + id: uuid::Uuid::now_v7().to_string(), + owner: self.lineage.clone(), + created: Instant::now(), + stdout: Vec::new(), + stderr: Vec::new(), + order: Vec::new(), + stdout_seen: 0, + stderr_seen: 0, + truncated: false, + limit: None, + transport_counts: None, + transport_truncation: None, + terminal: None, + })); + store.records.push_back(record.clone()); + store.reserved += COMMAND_BYTES; + Ok(BashOutputWriter { record }) + } + + fn record(&self, id: &str) -> Option>> { + if !self.is_active() { + return None; + } + let mut store = self.store.lock().unwrap(); + store.expire(Instant::now()); + store + .records + .iter() + .find(|r| { + let r = r.lock().unwrap(); + r.id == id && r.owner.starts_with(&self.lineage) && r.terminal.is_some() + }) + .cloned() + } + + pub fn metadata(&self, id: &str) -> Option { + let record = self.record(id)?; + let r = record.lock().unwrap(); + Some(r.metadata()) + } + + /// Copies at most the caller's bounded range; never publishes output bodies in descriptors. + pub fn read( + &self, + id: &str, + stream: CommandStream, + offset: usize, + limit: usize, + ) -> Option> { + let record = self.record(id)?; + let r = record.lock().unwrap(); + let bytes = match stream { + CommandStream::Stdout => &r.stdout, + CommandStream::Stderr => &r.stderr, + }; + let start = offset.min(bytes.len()); + Some(bytes[start..start.saturating_add(limit.min(64 * 1024)).min(bytes.len())].to_vec()) + } + + /// Scan the bounded raw source without cloning it, decoding only the requested line window. + /// A long single line uses the common byte reader instead of allocating an oversized text page. + pub fn line_window( + &self, + id: &str, + stream: CommandStream, + offset: usize, + limit: usize, + ) -> Option> { + let record = self.record(id)?; + let r = record.lock().unwrap(); + let bytes = match stream { + CommandStream::Stdout => &r.stdout, + CommandStream::Stderr => &r.stderr, + }; + let mut lines = bytes.split_inclusive(|b| *b == b'\n').skip(offset); + let mut window = Vec::new(); + for line in lines.by_ref().take(limit.min(2000)) { + if window.len() + line.len() > 64 * 1024 { + return Some(Err( + "text source window exceeds 64 KiB; use the common byte reader".into(), + )); + } + window.extend_from_slice(line); + } + let more = lines.next().is_some(); + Some(Ok((String::from_utf8_lossy(&window).into_owned(), more))) + } + + pub fn order(&self, id: &str) -> Option> { + let record = self.record(id)?; + let r = record.lock().unwrap(); + Some(r.order.clone()) + } +} + +impl Record { + fn metadata(&self) -> BashOutputMetadata { + BashOutputMetadata { + id: self.id.clone(), + stdout_saved: self.stdout.len(), + stderr_saved: self.stderr.len(), + stdout_seen: self.transport_counts.map_or(self.stdout_seen, |c| c.0), + stderr_seen: self.transport_counts.map_or(self.stderr_seen, |c| c.1), + stdout_delivered: self.stdout_seen, + stderr_delivered: self.stderr_seen, + retention_limit: self.limit, + transport_truncation: self.transport_truncation.clone(), + storage_truncated: self.truncated || self.transport_truncation.is_some(), + terminal: self.terminal.clone().expect("finalized record"), + text_encoding: "utf8_lossy", + ordering: "provider_read_observation", + } + } +} + +impl Store { + fn expire(&mut self, now: Instant) { + let before = self.records.len(); + self.records + .retain(|r| now.duration_since(r.lock().unwrap().created) < self.retention); + self.reserved -= (before - self.records.len()) * COMMAND_BYTES; + } +} + +async fn reap(weak: Weak>, mut shutdown: tokio::sync::watch::Receiver<()>) { + loop { + tokio::select! { + _ = tokio::time::sleep(Duration::from_secs(30)) => {}, + _ = shutdown.changed() => return, + } + let Some(store) = weak.upgrade() else { + return; + }; + store.lock().unwrap().expire(Instant::now()); + } +} + +impl BashOutputWriter { + pub fn append_chunk(&mut self, chunk: &workdir::CommandByteChunk) -> Result<(), String> { + let expected = { + let r = self.record.lock().unwrap(); + match chunk.stream { + CommandStream::Stdout => r.stdout_seen, + CommandStream::Stderr => r.stderr_seen, + } + }; + if chunk.offset != expected || chunk.bytes.is_empty() { + return Err("command byte transport stream gap or empty chunk".into()); + } + self.append(chunk.stream, &chunk.bytes); + Ok(()) + } + + pub fn append(&mut self, stream: CommandStream, bytes: &[u8]) { + let mut r = self.record.lock().unwrap(); + let seen = match stream { + CommandStream::Stdout => &mut r.stdout_seen, + CommandStream::Stderr => &mut r.stderr_seen, + }; + let start = *seen; + *seen = seen.saturating_add(bytes.len() as u64); + let available = COMMAND_BYTES.saturating_sub(r.stdout.len() + r.stderr.len()); + let n = if r.truncated || r.order.len() >= ORDER_COUNT { + 0 + } else { + available.min(bytes.len()) + }; + if n < bytes.len() { + r.truncated = true; + if r.limit.is_none() { + r.limit = Some(if r.order.len() >= ORDER_COUNT { + RetentionLimit::OrderQuota + } else { + RetentionLimit::ByteQuota + }); + } + } + if n > 0 { + match stream { + CommandStream::Stdout => r.stdout.extend_from_slice(&bytes[..n]), + CommandStream::Stderr => r.stderr.extend_from_slice(&bytes[..n]), + } + if let Some(last) = r + .order + .last_mut() + .filter(|last| last.stream == stream && last.end == start) + { + last.end += n as u64; + } else { + r.order.push(OutputRange { + stream, + start, + end: start + n as u64, + }); + } + } + } + + pub fn finish_transport( + &mut self, + terminal: &workdir::CommandTerminal, + ) -> Result { + let mut r = self.record.lock().unwrap(); + if terminal.stdout_delivered_bytes != r.stdout_seen + || terminal.stderr_delivered_bytes != r.stderr_seen + || terminal.stdout_bytes < r.stdout_seen + || terminal.stderr_bytes < r.stderr_seen + || ((terminal.stdout_bytes != r.stdout_seen || terminal.stderr_bytes != r.stderr_seen) + != terminal.truncation_reason.is_some()) + { + return Err("command terminal counts do not fence delivered bytes".into()); + } + r.transport_counts = Some((terminal.stdout_bytes, terminal.stderr_bytes)); + r.transport_truncation = terminal.truncation_reason.clone(); + r.terminal = Some(OutputTerminal { + status: Some(terminal.status), + exit_code: terminal.exit_code, + transport_complete: true, + }); + Ok(r.metadata()) + } + + pub fn finish(&mut self, terminal: OutputTerminal) -> BashOutputMetadata { + let mut r = self.record.lock().unwrap(); + r.terminal = Some(terminal); + r.metadata() + } +} + +impl Drop for BashOutputWriter { + fn drop(&mut self) { + let mut r = self.record.lock().unwrap(); + if r.terminal.is_none() { + r.terminal = Some(OutputTerminal { + status: None, + exit_code: None, + transport_complete: false, + }); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn finish(view: &BashOutputView, text: &[u8]) -> String { + let mut writer = view.begin().unwrap(); + writer.append(CommandStream::Stdout, text); + writer + .finish(OutputTerminal { + status: Some(CommandStatus::Completed), + exit_code: Some(0), + transport_complete: true, + }) + .id + } + + #[test] + fn revoked_child_is_denied_while_parent_keeps_evidence_and_sibling_is_denied() { + let root = BashOutputView::new(); + let lease = Arc::new(AtomicBool::new(true)); + let child = root.child({ + let lease = lease.clone(); + Arc::new(move || lease.load(Ordering::Acquire)) + }); + let sibling = root.child(Arc::new(|| true)); + let id = finish(&child, b"child"); + assert_eq!( + child.read(&id, CommandStream::Stdout, 0, 20).unwrap(), + b"child" + ); + assert!(sibling.metadata(&id).is_none()); + assert!(BashOutputView::new().metadata(&id).is_none()); + lease.store(false, Ordering::Release); + assert!(child.metadata(&id).is_none()); + assert_eq!( + root.read(&id, CommandStream::Stdout, 0, 20).unwrap(), + b"child" + ); + root.revoke(); + let restored = BashOutputView::new(); + assert!(root.metadata(&id).is_none()); + assert!(restored.metadata(&id).is_none()); + let new_id = finish(&restored, b"new incarnation"); + assert_ne!(new_id, id); + assert!(child.metadata(&new_id).is_none()); + assert!(root.begin().is_err()); + } + + #[test] + fn parent_and_descendants_share_admission_without_evicting_live_evidence() { + let root = BashOutputView::new(); + let child = root.child(Arc::new(|| true)); + let sibling = root.child(Arc::new(|| true)); + let mut writers = (0..STORE_BYTES / COMMAND_BYTES) + .map(|index| { + if index % 2 == 0 { + root.begin() + } else { + child.begin() + } + .unwrap() + }) + .collect::>(); + assert!(sibling.begin().is_err()); + let mut first = writers.remove(0); + first.append(CommandStream::Stdout, b"finished"); + let metadata = first.finish(OutputTerminal { + status: Some(CommandStatus::Completed), + exit_code: Some(0), + transport_complete: true, + }); + assert!(root.metadata(&metadata.id).is_some()); + let _replacement = sibling.begin().unwrap(); + assert!(root.metadata(&metadata.id).is_none()); + assert!(root.begin().is_err()); + assert_eq!(writers.len(), STORE_BYTES / COMMAND_BYTES - 1); + } + + #[test] + fn quota_and_forced_drop_are_distinct_from_preview_omission() { + let root = BashOutputView::new(); + let mut writer = root.begin().unwrap(); + writer.append(CommandStream::Stdout, &vec![b'x'; COMMAND_BYTES + 1]); + writer.append(CommandStream::Stderr, b"tail"); + let id = writer.record.lock().unwrap().id.clone(); + drop(writer); + let m = root.metadata(&id).unwrap(); + assert_eq!(m.stdout_saved, COMMAND_BYTES); + assert_eq!(m.stdout_seen, COMMAND_BYTES as u64 + 1); + assert_eq!(m.stderr_seen, 4); + assert!(m.storage_truncated); + assert!(!m.terminal.transport_complete); + assert_eq!( + root.read(&id, CommandStream::Stdout, 0, usize::MAX) + .unwrap() + .len(), + 64 * 1024 + ); + } + + #[test] + fn expiry_and_count_eviction_never_reuse_old_references() { + let root = BashOutputView::new(); + let id = finish(&root, b"old"); + root.store.lock().unwrap().retention = Duration::ZERO; + assert!(root.metadata(&id).is_none()); + root.store.lock().unwrap().retention = RETENTION; + let first = finish(&root, b"first"); + for _ in 0..STORE_COUNT { + finish(&root, b"later"); + } + assert!(root.metadata(&first).is_none()); + assert!(root.store.lock().unwrap().reserved <= STORE_BYTES); + } + #[tokio::test(start_paused = true)] + async fn expiry_reclaims_bytes_without_another_read_and_reaper_ends_with_owner() { + // Exercise host construction outside a runtime, followed by real async admission. + let root = std::thread::spawn(BashOutputView::new).join().unwrap(); + finish(&root, b"expires without lookup"); + let mut closed = root.store.lock().unwrap()._shutdown.subscribe(); + tokio::task::yield_now().await; + // The store uses monotonic wall time for publication and Tokio time only for scheduling. + // Age the record deterministically rather than delaying this test in real time. + root.store.lock().unwrap().records[0] + .lock() + .unwrap() + .created = Instant::now() - RETENTION; + tokio::time::advance(Duration::from_secs(31)).await; + tokio::task::yield_now().await; + assert_eq!(root.store.lock().unwrap().reserved, 0); + drop(root); + assert!(closed.changed().await.is_err()); + } + #[test] + fn terminal_counts_must_fence_the_received_prefix_before_marking_evidence_complete() { + let root = BashOutputView::new(); + let mut writer = root.begin().unwrap(); + writer.append(CommandStream::Stdout, b"abc"); + let mut terminal = serde_json::json!({"status":"completed","exit_code":0,"stdout_bytes":9,"stderr_bytes":0,"stdout_delivered_bytes":4,"stderr_delivered_bytes":0,"truncation_reason":"byte_budget"}); + assert!( + writer + .finish_transport(&serde_json::from_value(terminal.clone()).unwrap()) + .is_err() + ); + terminal["stdout_delivered_bytes"] = serde_json::json!(3); + let metadata = writer + .finish_transport(&serde_json::from_value(terminal).unwrap()) + .unwrap(); + assert_eq!(metadata.stdout_saved, 3); + assert_eq!(metadata.stdout_delivered, 3); + assert_eq!(metadata.stdout_seen, 9); + assert!(metadata.storage_truncated); + assert!(metadata.retention_limit.is_none()); + assert_eq!( + metadata.transport_truncation, + Some(workdir::CommandOutputTruncation::ByteBudget) + ); + } +} diff --git a/crates/tools/src/lib.rs b/crates/tools/src/lib.rs index 3bdb410d5..6efd98698 100644 --- a/crates/tools/src/lib.rs +++ b/crates/tools/src/lib.rs @@ -5,6 +5,7 @@ pub mod error; pub mod tracker; mod bash; +pub mod bash_output; mod edit; mod file_target; mod glob; @@ -16,7 +17,7 @@ pub mod view_image; mod web; mod write; -pub use bash::bash_tool; +pub use bash::{BashResult, BashTool, bash_tool}; pub use checkout::{CheckoutToolOutput, execute_checkout_tool}; pub use edit::edit_tool; pub use error::ToolsError; @@ -35,7 +36,7 @@ pub use write::write_tool; pub fn core_builtin_tools( session: workdir::WorkdirSessionHandle, tracker: Tracker, - bash_output_dir: std::path::PathBuf, + bash_output: bash_output::BashOutputView, ) -> Vec { use workdir::WorkdirSessionCapability; @@ -58,7 +59,7 @@ pub fn core_builtin_tools( tools.push(grep::routed_grep_tool(router.clone())); } if capabilities.supports(WorkdirSessionCapability::Command) { - tools.push(bash::routed_bash_tool(router, bash_output_dir)); + tools.push(bash::routed_bash_tool(router, bash_output)); } tools } @@ -71,7 +72,7 @@ pub fn core_builtin_tools( pub fn routed_builtin_tools( router: std::sync::Arc, tracker: Tracker, - bash_output_dir: std::path::PathBuf, + bash_output: bash_output::BashOutputView, ) -> Vec { vec![ read::routed_read_tool(router.clone(), tracker.clone()), @@ -79,14 +80,14 @@ pub fn routed_builtin_tools( edit::routed_edit_tool(router.clone(), tracker), glob::routed_glob_tool(router.clone()), grep::routed_grep_tool(router.clone()), - bash::routed_bash_tool(router, bash_output_dir), + bash::routed_bash_tool(router, bash_output), ] } pub fn routed_builtin_tools_for_current_capabilities( router: std::sync::Arc, tracker: Tracker, - bash_output_dir: std::path::PathBuf, + bash_output: bash_output::BashOutputView, ) -> Vec { use workdir::WorkdirSessionCapability; @@ -108,7 +109,7 @@ pub fn routed_builtin_tools_for_current_capabilities( tools.push(grep::routed_grep_tool(router.clone())); } if capabilities.supports(WorkdirSessionCapability::Command) { - tools.push(bash::routed_bash_tool(router, bash_output_dir)); + tools.push(bash::routed_bash_tool(router, bash_output)); } tools } @@ -121,7 +122,7 @@ pub fn read_only_builtin_tools( workdir::WorkdirSessionCapabilities::READ_ONLY, "read-only tool projection requires a read-only Workdir session" ); - core_builtin_tools(session, Tracker::new(), std::path::PathBuf::new()) + core_builtin_tools(session, Tracker::new(), bash_output::BashOutputView::new()) } pub fn read_only_routed_builtin_tools( @@ -173,7 +174,7 @@ mod workdir_tool_tests { SharedScope::new(Scope::writable(dir.path()).unwrap()), WorkdirSessionCapabilities::READ_ONLY, )); - let names = core_builtin_tools(session, Tracker::new(), dir.path().join("output")) + let names = core_builtin_tools(session, Tracker::new(), bash_output::BashOutputView::new()) .into_iter() .map(|definition| definition().0.name) .collect::>(); diff --git a/crates/tools/tests/edge_cases.rs b/crates/tools/tests/edge_cases.rs index 252f6001c..8d5abd689 100644 --- a/crates/tools/tests/edge_cases.rs +++ b/crates/tools/tests/edge_cases.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use agen::tool::{Tool, ToolDefinition}; -use manifest::{Permission, Scope, ScopeConfig, ScopeRule}; +use manifest::Scope; use serde_json::json; use tempfile::TempDir; use tools::{Tracker, core_builtin_tools}; @@ -28,31 +28,23 @@ impl Registry { } } -fn setup() -> (TempDir, TempDir, Registry) { +fn setup() -> (TempDir, Registry) { let dir = TempDir::new().unwrap(); - let spill = TempDir::new().unwrap(); - let base = Scope::writable(dir.path()).unwrap(); - let mut config = ScopeConfig { - allow: base.allow_rules(), - deny: base.deny_rules(), - }; - config.allow.push(ScopeRule { - target: spill.path().to_path_buf(), - permission: Permission::Read, - recursive: true, - symlink_policy: Default::default(), - }); - let scope = Scope::from_config(&config).unwrap(); + let scope = Scope::writable(dir.path()).unwrap(); let fs: WorkdirSessionHandle = std::sync::Arc::new(LocalWorkdirSession::new(scope, dir.path().to_path_buf())); let tracker = Tracker::new(); - let reg = Registry::new(core_builtin_tools(fs, tracker, spill.path().to_path_buf())); - (dir, spill, reg) + let reg = Registry::new(core_builtin_tools( + fs, + tracker, + tools::bash_output::BashOutputView::new(), + )); + (dir, reg) } #[tokio::test] async fn unicode_path_and_content() { - let (dir, _spill, reg) = setup(); + let (dir, reg) = setup(); let file = dir.path().join("日本語ファイル.txt"); let content = "こんにちは 🦀 世界\nabc\n"; @@ -87,7 +79,7 @@ async fn unicode_path_and_content() { async fn symlink_to_outside_scope_is_rejected_for_write() { use std::os::unix::fs::symlink; - let (dir, _spill, reg) = setup(); + let (dir, reg) = setup(); let outside = TempDir::new().unwrap(); let outside_target = outside.path().join("secret.txt"); std::fs::write(&outside_target, "secret").unwrap(); @@ -151,7 +143,7 @@ async fn symlink_to_outside_scope_is_rejected_for_write() { async fn broken_symlink_reports_target_and_repair_hint() { use std::os::unix::fs::symlink; - let (dir, _spill, reg) = setup(); + let (dir, reg) = setup(); let link = dir.path().join("external-project"); let target = dir.path().join("missing-target"); symlink(&target, &link).unwrap(); @@ -173,7 +165,7 @@ async fn broken_symlink_reports_target_and_repair_hint() { #[tokio::test] async fn empty_file_read_and_edit() { - let (dir, _spill, reg) = setup(); + let (dir, reg) = setup(); let file = dir.path().join("empty.txt"); std::fs::write(&file, "").unwrap(); @@ -207,7 +199,7 @@ async fn empty_file_read_and_edit() { #[tokio::test] async fn very_long_single_line() { - let (dir, _spill, reg) = setup(); + let (dir, reg) = setup(); let file = dir.path().join("long.txt"); let big: String = "x".repeat(1024 * 1024); // 1 MiB, no newlines std::fs::write(&file, &big).unwrap(); @@ -226,7 +218,7 @@ async fn very_long_single_line() { #[tokio::test] async fn absolute_path_requires_matching_read_scope() { - let (_dir, _spill, reg) = setup(); + let (_dir, reg) = setup(); let outside = tempfile::tempdir().unwrap(); let outside_file = outside.path().join("outside.txt"); std::fs::write(&outside_file, "secret").unwrap(); @@ -247,7 +239,7 @@ async fn absolute_path_requires_matching_read_scope() { #[tokio::test] async fn directory_target_is_rejected_for_read() { - let (_dir, _spill, reg) = setup(); + let (_dir, reg) = setup(); let read = reg.get("Read"); let err = read .execute(&json!({ "file_path": "." }).to_string(), Default::default()) @@ -258,7 +250,7 @@ async fn directory_target_is_rejected_for_read() { #[tokio::test] async fn deeply_nested_new_file_is_created() { - let (dir, _spill, reg) = setup(); + let (dir, reg) = setup(); let deep = dir.path().join("a/b/c/d/e/deep.txt"); let write = reg.get("Write"); write @@ -277,7 +269,7 @@ async fn deeply_nested_new_file_is_created() { #[tokio::test] async fn replace_preserves_unicode() { - let (dir, _spill, reg) = setup(); + let (dir, reg) = setup(); let file = dir.path().join("u.txt"); std::fs::write(&file, "🦀 rust 🦀\n").unwrap(); @@ -306,7 +298,7 @@ async fn replace_preserves_unicode() { #[tokio::test] async fn grep_handles_unicode_pattern() { - let (dir, _spill, reg) = setup(); + let (dir, reg) = setup(); let file = dir.path().join("u.txt"); std::fs::write(&file, "English\n日本語\nрусский\n").unwrap(); diff --git a/crates/tools/tests/integration.rs b/crates/tools/tests/integration.rs index c8c55e8d6..accfb59ef 100644 --- a/crates/tools/tests/integration.rs +++ b/crates/tools/tests/integration.rs @@ -11,7 +11,7 @@ use agen::tool::{ Tool, ToolDefinition, ToolError, ToolExecutionContext, ToolExecutionHandle, ToolExecutionTerminal, ToolMeta, }; -use manifest::{Permission, Scope, ScopeConfig, ScopeRule}; +use manifest::Scope; use serde_json::json; use tempfile::TempDir; use tools::{ @@ -23,21 +23,6 @@ use workdir::{ WorkdirToolScopeRule, }; -fn scope_with_spill(workspace: &Path, spill: &Path) -> Scope { - let base = Scope::writable(workspace).unwrap(); - let mut config = ScopeConfig { - allow: base.allow_rules(), - deny: base.deny_rules(), - }; - config.allow.push(ScopeRule { - target: spill.to_path_buf(), - permission: Permission::Read, - recursive: true, - symlink_policy: Default::default(), - }); - Scope::from_config(&config).unwrap() -} - struct Registry { entries: Vec<(ToolMeta, Arc)>, } @@ -61,39 +46,38 @@ impl Registry { } } -fn setup() -> (TempDir, TempDir, Registry) { +fn setup() -> (TempDir, Registry) { let dir = TempDir::new().unwrap(); - let spill = TempDir::new().unwrap(); - let scope = scope_with_spill(dir.path(), spill.path()); + let scope = Scope::writable(dir.path()).unwrap(); let fs: WorkdirSessionHandle = Arc::new(LocalWorkdirSession::new(scope, dir.path().to_path_buf())); let tracker = Tracker::new(); - let reg = Registry::new(core_builtin_tools(fs, tracker, spill.path().to_path_buf())); - (dir, spill, reg) + let reg = Registry::new(core_builtin_tools( + fs, + tracker, + tools::bash_output::BashOutputView::new(), + )); + (dir, reg) } -fn setup_routed() -> ( - TempDir, - TempDir, - TempDir, - Arc, - Registry, -) { +fn setup_routed() -> (TempDir, TempDir, Arc, Registry) { let left = TempDir::new().unwrap(); let right = TempDir::new().unwrap(); - let spill = TempDir::new().unwrap(); let router = Arc::new(WorkdirSessionRouter::new()); for (alias, dir) in [("left", &left), ("right", &right)] { - let scope = scope_with_spill(dir.path(), spill.path()); + let scope = Scope::writable(dir.path()).unwrap(); let session: WorkdirSessionHandle = Arc::new(LocalWorkdirSession::new(scope, dir.path().to_path_buf())); router .attach(WorkdirAttachmentAlias::new(alias).unwrap(), session) .unwrap(); } - let definitions = - routed_builtin_tools(router.clone(), Tracker::new(), spill.path().to_path_buf()); - (left, right, spill, router, Registry::new(definitions)) + let definitions = routed_builtin_tools( + router.clone(), + Tracker::new(), + tools::bash_output::BashOutputView::new(), + ); + (left, right, router, Registry::new(definitions)) } fn valid_gif_with_len(target_len: usize) -> Vec { @@ -137,7 +121,7 @@ async fn call_err(tool: &Arc, input: serde_json::Value) -> agen::tool: #[test] fn core_builtin_tools_registers_full_set() { - let (_dir, _spill, reg) = setup(); + let (_dir, reg) = setup(); let mut names = reg.names(); names.sort(); assert_eq!(names, vec!["Bash", "Edit", "Glob", "Grep", "Read", "Write"]); @@ -145,7 +129,7 @@ fn core_builtin_tools_registers_full_set() { #[test] fn meta_has_description_and_schema() { - let (_dir, _spill, reg) = setup(); + let (_dir, reg) = setup(); for (meta, _) in ®.entries { assert!( !meta.description.is_empty(), @@ -163,7 +147,7 @@ fn meta_has_description_and_schema() { #[test] fn routed_tool_schemas_share_optional_target_workdir() { - let (_left, _right, _spill, router, mut reg) = setup_routed(); + let (_left, _right, router, mut reg) = setup_routed(); reg.entries.push(routed_view_image_tool(router)()); for (meta, _) in ®.entries { let target = &meta.input_schema["properties"]["target_workdir"]; @@ -174,7 +158,7 @@ fn routed_tool_schemas_share_optional_target_workdir() { #[tokio::test] async fn routed_tools_require_alias_only_when_attachment_set_is_ambiguous() { - let (_left, _right, _spill, router, reg) = setup_routed(); + let (_left, _right, router, reg) = setup_routed(); let error = call_err(®.get("Read"), json!({ "file_path": "same.txt" })).await; let message = error.to_string(); assert!(message.contains("target_workdir_required"), "{message}"); @@ -199,7 +183,7 @@ async fn empty_routed_surface_returns_no_attachment_error() { let reg = Registry::new(routed_builtin_tools( router, Tracker::new(), - Path::new("unused").to_path_buf(), + tools::bash_output::BashOutputView::new(), )); let message = call_err(®.get("Read"), json!({ "file_path": "file.txt" })) .await @@ -209,7 +193,7 @@ async fn empty_routed_surface_returns_no_attachment_error() { #[tokio::test] async fn target_alias_routes_each_tool_and_isolates_read_before_edit() { - let (left, right, _spill, router, mut reg) = setup_routed(); + let (left, right, router, mut reg) = setup_routed(); std::fs::write(left.path().join("same.txt"), "left value\n").unwrap(); std::fs::write(right.path().join("same.txt"), "right value\n").unwrap(); std::fs::write(left.path().join("only-left.log"), "LEFT-NEEDLE\n").unwrap(); @@ -282,7 +266,7 @@ async fn target_alias_routes_each_tool_and_isolates_read_before_edit() { ) .await; assert_eq!( - std::fs::canonicalize(bash.content.unwrap().trim()).unwrap(), + std::fs::canonicalize(serde_json::from_str::(&bash.content.unwrap()).unwrap()["stdout_preview"].as_str().unwrap().trim()).unwrap(), std::fs::canonicalize(right.path()).unwrap() ); @@ -304,13 +288,12 @@ async fn target_alias_routes_each_tool_and_isolates_read_before_edit() { async fn selected_attachment_capability_is_checked_without_fallback() { let left = TempDir::new().unwrap(); let right = TempDir::new().unwrap(); - let spill = TempDir::new().unwrap(); let router = Arc::new(WorkdirSessionRouter::new()); router .attach( WorkdirAttachmentAlias::new("writable").unwrap(), Arc::new(LocalWorkdirSession::new( - scope_with_spill(left.path(), spill.path()), + Scope::writable(left.path()).unwrap(), left.path().to_path_buf(), )), ) @@ -322,7 +305,7 @@ async fn selected_attachment_capability_is_checked_without_fallback() { workdir::Workdir::new("readonly"), right.path().to_path_buf(), right.path().to_path_buf(), - manifest::SharedScope::new(scope_with_spill(right.path(), spill.path())), + manifest::SharedScope::new(Scope::writable(right.path()).unwrap()), workdir::WorkdirSessionCapabilities::READ_ONLY, )), ) @@ -330,7 +313,7 @@ async fn selected_attachment_capability_is_checked_without_fallback() { let reg = Registry::new(routed_builtin_tools( router.clone(), Tracker::new(), - spill.path().to_path_buf(), + tools::bash_output::BashOutputView::new(), )); let message = call_err( @@ -352,7 +335,7 @@ async fn selected_attachment_capability_is_checked_without_fallback() { #[tokio::test] async fn routed_tools_observe_detach_after_registration() { - let (_left, _right, _spill, router, reg) = setup_routed(); + let (_left, _right, router, reg) = setup_routed(); let alias = WorkdirAttachmentAlias::new("left").unwrap(); router.detach(&alias).await.unwrap(); @@ -372,7 +355,6 @@ async fn routed_tools_observe_detach_after_registration() { async fn detach_and_reattach_same_alias_requires_a_fresh_read() { let first = TempDir::new().unwrap(); let second = TempDir::new().unwrap(); - let spill = TempDir::new().unwrap(); std::fs::write(first.path().join("same.txt"), "same bytes\n").unwrap(); std::fs::write(second.path().join("same.txt"), "same bytes\n").unwrap(); @@ -382,7 +364,7 @@ async fn detach_and_reattach_same_alias_requires_a_fresh_read() { .attach( alias.clone(), Arc::new(LocalWorkdirSession::new( - scope_with_spill(first.path(), spill.path()), + Scope::writable(first.path()).unwrap(), first.path().to_path_buf(), )), ) @@ -390,7 +372,7 @@ async fn detach_and_reattach_same_alias_requires_a_fresh_read() { let reg = Registry::new(routed_builtin_tools( router.clone(), Tracker::new(), - spill.path().to_path_buf(), + tools::bash_output::BashOutputView::new(), )); call( ®.get("Read"), @@ -403,7 +385,7 @@ async fn detach_and_reattach_same_alias_requires_a_fresh_read() { .attach( alias.clone(), Arc::new(LocalWorkdirSession::new( - scope_with_spill(second.path(), spill.path()), + Scope::writable(second.path()).unwrap(), second.path().to_path_buf(), )), ) @@ -432,6 +414,8 @@ async fn detach_and_reattach_same_alias_requires_a_fresh_read() { struct ExternalValidatingSession { inner: LocalWorkdirSession, truncate_first_binary_response: bool, + command_response_losses: std::sync::atomic::AtomicUsize, + lose_terminal_only: bool, mutate_after_first_binary_response: Option, } @@ -550,7 +534,23 @@ impl workdir::WorkdirSession for ExternalValidatingSession { &self, request: workdir::CommandOutputRequest, ) -> Result { - self.inner.command_output(request).await + let output = self.inner.command_output(request).await?; + if (!self.lose_terminal_only || output.terminal.is_some()) + && self + .command_response_losses + .fetch_update( + std::sync::atomic::Ordering::AcqRel, + std::sync::atomic::Ordering::Acquire, + |remaining| remaining.checked_sub(1), + ) + .is_ok() + { + // The production provider has committed a page, but its response is lost. + return Err(workdir::WorkdirError::Transport( + "injected lost response".into(), + )); + } + Ok(output) } async fn cancel_command( @@ -568,11 +568,12 @@ impl workdir::WorkdirSession for ExternalValidatingSession { #[tokio::test] async fn external_view_image_uses_a_protocol_bounded_request() { let dir = TempDir::new().unwrap(); - let spill = TempDir::new().unwrap(); - let scope = scope_with_spill(dir.path(), spill.path()); + let scope = Scope::writable(dir.path()).unwrap(); let session: WorkdirSessionHandle = Arc::new(ExternalValidatingSession { inner: LocalWorkdirSession::new(scope, dir.path().to_path_buf()), truncate_first_binary_response: false, + command_response_losses: std::sync::atomic::AtomicUsize::new(0), + lose_terminal_only: false, mutate_after_first_binary_response: None, }); let png = include_bytes!("fixtures/view-image/valid.png"); @@ -590,13 +591,14 @@ async fn external_view_image_uses_a_protocol_bounded_request() { #[tokio::test] async fn external_view_image_rejects_truncated_and_cross_version_chunks() { let dir = TempDir::new().unwrap(); - let spill = TempDir::new().unwrap(); let image_path = dir.path().join("image.png"); - let scope = scope_with_spill(dir.path(), spill.path()); + let scope = Scope::writable(dir.path()).unwrap(); std::fs::write(&image_path, include_bytes!("fixtures/view-image/valid.png")).unwrap(); let truncated: WorkdirSessionHandle = Arc::new(ExternalValidatingSession { inner: LocalWorkdirSession::new(scope.clone(), dir.path().to_path_buf()), truncate_first_binary_response: true, + command_response_losses: std::sync::atomic::AtomicUsize::new(0), + lose_terminal_only: false, mutate_after_first_binary_response: None, }); let (_meta, tool) = view_image_tool(truncated)(); @@ -613,6 +615,8 @@ async fn external_view_image_rejects_truncated_and_cross_version_chunks() { let changing: WorkdirSessionHandle = Arc::new(ExternalValidatingSession { inner: LocalWorkdirSession::new(scope, dir.path().to_path_buf()), truncate_first_binary_response: false, + command_response_losses: std::sync::atomic::AtomicUsize::new(0), + lose_terminal_only: false, mutate_after_first_binary_response: Some(image_path), }); let (_meta, tool) = view_image_tool(changing)(); @@ -626,8 +630,7 @@ async fn external_view_image_rejects_truncated_and_cross_version_chunks() { async fn view_image_enforces_the_ten_mib_boundary_without_partial_attachments() { const LIMIT: usize = 10 * 1024 * 1024; let dir = TempDir::new().unwrap(); - let spill = TempDir::new().unwrap(); - let scope = scope_with_spill(dir.path(), spill.path()); + let scope = Scope::writable(dir.path()).unwrap(); let session: WorkdirSessionHandle = Arc::new(LocalWorkdirSession::new(scope, dir.path().to_path_buf())); let (_meta, tool) = view_image_tool(session)(); @@ -650,8 +653,7 @@ async fn view_image_enforces_the_ten_mib_boundary_without_partial_attachments() #[tokio::test] async fn view_image_attaches_valid_supported_image_fixtures() { let dir = TempDir::new().unwrap(); - let spill = TempDir::new().unwrap(); - let scope = scope_with_spill(dir.path(), spill.path()); + let scope = Scope::writable(dir.path()).unwrap(); let session: WorkdirSessionHandle = Arc::new(LocalWorkdirSession::new(scope, dir.path().to_path_buf())); let (_meta, tool) = view_image_tool(session)(); @@ -726,8 +728,7 @@ async fn delegated_view_image_preserves_caller_relative_path_identity() { #[tokio::test] async fn view_image_reads_scoped_bytes_into_durable_tool_detail() { let dir = TempDir::new().unwrap(); - let spill = TempDir::new().unwrap(); - let scope = scope_with_spill(dir.path(), spill.path()); + let scope = Scope::writable(dir.path()).unwrap(); let session: WorkdirSessionHandle = Arc::new(LocalWorkdirSession::new(scope, dir.path().to_path_buf())); let png = include_bytes!("fixtures/view-image/valid.png"); @@ -752,7 +753,7 @@ async fn view_image_reads_scoped_bytes_into_durable_tool_detail() { #[tokio::test] async fn read_then_edit_then_read_roundtrip() { - let (dir, _spill, reg) = setup(); + let (dir, reg) = setup(); let file = dir.path().join("a.txt"); std::fs::write(&file, "hello world\n").unwrap(); let p = "a.txt"; @@ -784,7 +785,7 @@ async fn read_then_edit_then_read_roundtrip() { #[tokio::test] async fn write_then_grep_finds_content() { - let (dir, _spill, reg) = setup(); + let (dir, reg) = setup(); let write = reg.get("Write"); let grep = reg.get("Grep"); @@ -813,7 +814,7 @@ async fn write_then_grep_finds_content() { #[tokio::test] async fn glob_finds_written_files() { - let (_dir, _spill, reg) = setup(); + let (_dir, reg) = setup(); let write = reg.get("Write"); let glob = reg.get("Glob"); @@ -837,7 +838,7 @@ async fn glob_finds_written_files() { #[tokio::test] async fn absolute_path_is_rejected() { - let (_dir, _spill, reg) = setup(); + let (_dir, reg) = setup(); let outside = TempDir::new().unwrap(); let write = reg.get("Write"); @@ -859,7 +860,7 @@ async fn absolute_path_is_rejected() { #[tokio::test] async fn write_to_existing_without_read_fails() { - let (dir, _spill, reg) = setup(); + let (dir, reg) = setup(); let file = dir.path().join("exists.txt"); std::fs::write(&file, "preexisting").unwrap(); @@ -880,7 +881,7 @@ async fn write_to_existing_without_read_fails() { async fn shared_workdir_across_tools() { // The key invariant: all builtin tools share the same WorkdirSession instance, // so read-history set by Read is visible to Edit and Write. - let (dir, _spill, reg) = setup(); + let (dir, reg) = setup(); let file = dir.path().join("shared.txt"); std::fs::write(&file, "one\n").unwrap(); @@ -907,7 +908,7 @@ async fn shared_workdir_across_tools() { #[tokio::test] async fn edit_requires_read_across_tools() { - let (dir, _spill, reg) = setup(); + let (dir, reg) = setup(); let file = dir.path().join("a.txt"); std::fs::write(&file, "foo\n").unwrap(); @@ -928,7 +929,7 @@ async fn edit_requires_read_across_tools() { #[tokio::test] async fn deterministic_tool_order_is_registration_order() { - let (_dir, _spill, reg) = setup(); + let (_dir, reg) = setup(); // Registration order from core_builtin_tools(): Read, Write, Edit, Glob, Grep, Bash let names: Vec<&str> = reg.entries.iter().map(|(m, _)| m.name.as_str()).collect(); assert_eq!( @@ -940,7 +941,7 @@ async fn deterministic_tool_order_is_registration_order() { // Regression: tool name capitalization matches Claude Code reference #[test] fn tool_names_match_reference_spec() { - let (_dir, _spill, reg) = setup(); + let (_dir, reg) = setup(); for expected in ["Read", "Write", "Edit", "Glob", "Grep", "Bash"] { assert!( reg.entries.iter().any(|(m, _)| m.name == expected), @@ -953,15 +954,14 @@ fn tool_names_match_reference_spec() { async fn tracker_recent_files_tracks_read_write_edit() { // Build a fresh registry that shares a tracker we can query afterwards. let dir = TempDir::new().unwrap(); - let spill = TempDir::new().unwrap(); - let scope = scope_with_spill(dir.path(), spill.path()); + let scope = Scope::writable(dir.path()).unwrap(); let fs: WorkdirSessionHandle = Arc::new(LocalWorkdirSession::new(scope, dir.path().to_path_buf())); let tracker = Tracker::new(); let reg = Registry::new(core_builtin_tools( fs, tracker.clone(), - spill.path().to_path_buf(), + tools::bash_output::BashOutputView::new(), )); let a = dir.path().join("a.txt"); @@ -1003,118 +1003,314 @@ async fn tracker_recent_files_tracks_read_write_edit() { async fn bash_inherits_workdir_cwd() { // The Bash tool starts at the WorkdirSession's pwd. Without any `cd`, its // `pwd` should canonicalize to the workspace root we set up. - let (dir, _spill, reg) = setup(); + let (dir, reg) = setup(); let bash = reg.get("Bash"); let out = call(&bash, json!({ "command": "pwd", "cwd": "." })).await; let body = out.content.unwrap(); - let actual = std::fs::canonicalize(body.trim()).unwrap(); + let parsed: serde_json::Value = serde_json::from_str(&body).unwrap(); + let actual = std::fs::canonicalize(parsed["stdout_preview"].as_str().unwrap().trim()).unwrap(); let expected = std::fs::canonicalize(dir.path()).unwrap(); assert_eq!(actual, expected); } +fn output_fixture() -> ( + TempDir, + WorkdirSessionHandle, + tools::bash_output::BashOutputView, + Arc, +) { + let root = TempDir::new().unwrap(); + let session: WorkdirSessionHandle = Arc::new(LocalWorkdirSession::new( + Scope::writable(root.path()).unwrap(), + root.path().into(), + )); + let view = tools::bash_output::BashOutputView::new(); + let (_, bash) = tools::bash_tool(session.clone(), view.clone())(); + (root, session, view, bash) +} + #[tokio::test] -async fn bash_provider_output_exposes_readable_retained_path() { - let (_dir, spill, reg) = setup(); - let bash = reg.get("Bash"); +async fn bash_output_is_retained_without_a_filesystem_grant_or_telemetry_subscriber() { + let (root, _session, view, bash) = output_fixture(); let out = call( &bash, - json!({ "command": "printf 'x%.0s' {1..20480}", "cwd": "." }), + json!({ "command": "printf 'x%.0s' {1..20480}; printf '\\377€' >&2; exit 7", "cwd": "." }), ) .await; - let body = out.content.unwrap(); - assert!(body.contains("bounded WorkdirSession command output")); - assert!(body.contains("full output saved to")); - assert!(body.contains(spill.path().to_str().unwrap())); - let artifact = std::fs::read_dir(spill.path()) - .unwrap() - .next() - .expect("retained output") - .unwrap() - .path(); - assert_eq!(std::fs::metadata(artifact).unwrap().len(), 20_480); + let body: serde_json::Value = serde_json::from_str(&out.content.unwrap()).unwrap(); + assert_eq!(body["output"]["terminal"]["exit_code"], 7); + assert_eq!(body["preview_omitted"], true); + assert_eq!(body["output"]["storage_truncated"], false); + let id = body["output"]["id"].as_str().unwrap(); + assert_eq!( + view.read(id, workdir::CommandStream::Stdout, 0, 65536) + .unwrap(), + vec![b'x'; 20480] + ); + assert_eq!( + view.read(id, workdir::CommandStream::Stderr, 0, 65536) + .unwrap(), + [vec![255], "€".as_bytes().to_vec()].concat() + ); + assert!( + body["stdout_entry"] + .as_str() + .unwrap() + .starts_with("/bash-output/") + ); + assert!(!body.to_string().contains(root.path().to_str().unwrap())); + assert_eq!(std::fs::read_dir(root.path()).unwrap().count(), 0); +} + +async fn await_command_output( + events: &mut tokio::sync::broadcast::Receiver, +) { + tokio::time::timeout(std::time::Duration::from_secs(5), async { + loop { + if matches!( + events.recv().await.unwrap(), + workdir::CommandEvent::Output { .. } + ) { + break; + } + } + }) + .await + .expect("command output readiness"); +} +async fn await_command_terminal( + events: &mut tokio::sync::broadcast::Receiver, +) { + tokio::time::timeout(std::time::Duration::from_secs(5), async { + loop { + if let workdir::CommandEvent::Terminal { status, .. } = events.recv().await.unwrap() { + assert_eq!(status, workdir::CommandStatus::Cancelled); + break; + } + } + }) + .await + .expect("command terminal fence"); } #[tokio::test] async fn bash_cancellation_returns_bounded_progress_as_terminal_output() { - let (dir, _spill, reg) = setup(); - let marker = dir.path().join("must-not-run-after-cancel"); - let command = format!( - "printf 'before\\n'; printf 'err-before\\n' >&2; sleep 1; touch {}; printf 'after\\n'", - marker.display() - ); - let input = serde_json::to_string(&json!({ "command": command, "cwd": "." })).unwrap(); + let (_root, session, _view, bash) = output_fixture(); + let mut events = session.subscribe_command_events().unwrap(); let context = ToolExecutionContext::new("call-heavy", "attempt-heavy", 0); - let bash = reg.get("Bash"); let executing = bash.clone(); let execution_context = context.clone(); - let execution = tokio::spawn(async move { executing.execute(&input, execution_context).await }); - - tokio::time::sleep(std::time::Duration::from_millis(100)).await; - bash.cancel_execution(&context) - .await - .expect("signal exact execution cancellation"); - let error = tokio::time::timeout(std::time::Duration::from_secs(2), execution) - .await - .expect("cancelled Bash should terminate inside the Engine grace budget") - .expect("Bash task join"); - - let ToolError::Cancelled(output) = error.expect_err("cancelled command is non-success") else { - panic!("expected typed cancellation result"); + let execution = tokio::spawn(async move { + executing.execute(r#"{"command":"printf 'before'; printf 'err-before' >&2; while :; do :; done","cwd":"."}"#, execution_context).await + }); + await_command_output(&mut events).await; + bash.cancel_execution(&context).await.unwrap(); + let ToolError::Cancelled(output) = + tokio::time::timeout(std::time::Duration::from_secs(5), execution) + .await + .unwrap() + .unwrap() + .unwrap_err() + else { + panic!("expected cancellation"); }; - let content = output.content.expect("bounded progress output"); - assert!( - content.contains("before"), - "missing pre-cancel stdout: {content}" - ); - assert!( - content.contains("err-before"), - "missing pre-cancel stderr: {content}" - ); - assert!( - !content.contains("after"), - "post-cancel output leaked: {content}" - ); - assert!(content.len() <= 16 * 1024, "output must remain bounded"); - - tokio::time::sleep(std::time::Duration::from_millis(1_100)).await; - assert!( - !marker.exists(), - "the cancelled command continued executing after terminal confirmation" - ); + let body: serde_json::Value = serde_json::from_str(&output.content.unwrap()).unwrap(); + assert_eq!(body["stdout_preview"], "before"); + assert_eq!(body["stderr_preview"], "err-before"); + assert_eq!(body["output"]["terminal"]["status"], "cancelled"); + assert_eq!(body["output"]["terminal"]["transport_complete"], true); } #[tokio::test] async fn bash_force_close_cleanup_stops_command_and_keeps_session_reusable() { - let (dir, _spill, reg) = setup(); - let marker = dir.path().join("must-not-survive-force-close"); - let command = format!("sleep 1; touch {}", marker.display()); - let input = serde_json::to_string(&json!({ "command": command, "cwd": "." })).unwrap(); - let bash = reg.get("Bash"); + let (_root, session, _view, bash) = output_fixture(); + let mut events = session.subscribe_command_events().unwrap(); let context = ToolExecutionContext::new("call-force", "attempt-force", 0); - let (handle, terminal) = ToolExecutionHandle::start(bash.clone(), input, context); - - tokio::time::sleep(std::time::Duration::from_millis(100)).await; + let (handle, terminal) = ToolExecutionHandle::start( + bash.clone(), + r#"{"command":"printf ready; while :; do :; done","cwd":"."}"#.into(), + context, + ); + await_command_output(&mut events).await; handle.force_close(); assert!(matches!( terminal.await, ToolExecutionTerminal::OutcomeUnknown )); - - tokio::time::sleep(std::time::Duration::from_millis(1_100)).await; - assert!( - !marker.exists(), - "CommandGuard cleanup allowed a force-closed command to continue" - ); - - let output = bash - .execute( - r#"{"command":"printf 'reused'","cwd":"."}"#, - Default::default(), - ) - .await - .expect("workdir session remains reusable after cleanup"); - assert_eq!(output.content.as_deref(), Some("reused")); + await_command_terminal(&mut events).await; + let output = call(&bash, json!({"command":"printf reused", "cwd":"."})).await; + let body: serde_json::Value = serde_json::from_str(&output.content.unwrap()).unwrap(); + assert_eq!(body["stdout_preview"], "reused"); } // Sanity: unused Path import guard const _: fn() -> &'static Path = || Path::new("/"); + +#[tokio::test] +async fn parent_and_sibling_broker_commands_keep_output_ownership_after_revoke() { + let (root, source, parent_view, parent_bash) = output_fixture(); + let broker = WorkdirToolBroker::new(source); + let mut leases = Vec::new(); + let mut views = Vec::new(); + let mut commands = Vec::new(); + for name in ["left", "right"] { + std::fs::create_dir(root.path().join(name)).unwrap(); + let lease = Arc::new( + broker + .scope(WorkdirToolScope { + rules: vec![WorkdirToolScopeRule { + target: WorkdirPath::new(name).unwrap(), + permission: WorkdirToolScopePermission::Write, + recursive: true, + symlink_policy: Default::default(), + }], + cwd: WorkdirPath::new(name).unwrap(), + command: true, + }) + .await + .unwrap(), + ); + let authority = lease.clone(); + let view = parent_view.child(Arc::new(move || authority.is_active())); + let (_, bash) = tools::bash_tool(lease.tool_session(), view.clone())(); + leases.push(lease); + views.push(view); + commands.push(bash); + } + let left_input = + json!({"command":"printf 'left-%.0s' {1..10000}; printf left-err >&2", "cwd":"."}); + let right_input = + json!({"command":"printf 'right-%.0s' {1..10000}; printf right-err >&2", "cwd":"."}); + let parent_input = json!({"command":"printf parent; printf parent-err >&2", "cwd":"."}); + let (left, right, parent) = tokio::join!( + call(&commands[0], left_input), + call(&commands[1], right_input), + call(&parent_bash, parent_input) + ); + let outputs = [left, right, parent] + .map(|out| serde_json::from_str::(&out.content.unwrap()).unwrap()); + for (index, label) in ["left", "right", "parent"].iter().enumerate() { + let id = outputs[index]["output"]["id"].as_str().unwrap(); + let bytes = parent_view + .read(id, workdir::CommandStream::Stderr, 0, 65536) + .unwrap(); + assert_eq!(bytes, format!("{label}-err").as_bytes()); + for (child_index, view) in views.iter().enumerate() { + assert_eq!(view.metadata(id).is_some(), child_index == index); + } + } + let left_id = outputs[0]["output"]["id"].as_str().unwrap(); + leases[0].close().await.unwrap(); + assert!(views[0].metadata(left_id).is_none()); + assert!(parent_view.metadata(left_id).is_some()); + assert!( + views[1] + .metadata(outputs[1]["output"]["id"].as_str().unwrap()) + .is_some() + ); + leases[1].close().await.unwrap(); + broker.close().await.unwrap(); + parent_view.revoke(); + assert!(parent_view.metadata(left_id).is_none()); +} + +#[tokio::test] +async fn utf8_quota_boundary_preserves_raw_prefix_and_reports_saved_vs_seen() { + let (_root, _session, view, bash) = output_fixture(); + let out = call( + &bash, + json!({"command":"printf '%4194303s' ''; printf '€'", "cwd":"."}), + ) + .await; + let body: serde_json::Value = serde_json::from_str(&out.content.unwrap()).unwrap(); + let id = body["output"]["id"].as_str().unwrap(); + let saved = body["output"]["stdout_saved"].as_u64().unwrap() as usize; + assert_eq!(saved, 4 * 1024 * 1024); + assert_eq!(body["output"]["stdout_seen"], 4 * 1024 * 1024 + 2); + assert_eq!(body["output"]["stderr_seen"], 0); + assert_eq!(body["output"]["storage_truncated"], true); + assert_eq!( + view.read(id, workdir::CommandStream::Stdout, saved - 1, 10) + .unwrap(), + vec![0xe2] + ); + assert!( + view.line_window(id, workdir::CommandStream::Stdout, 0, 1) + .unwrap() + .is_err() + ); + assert!(out.summary.starts_with("$ ")); +} + +#[tokio::test] +async fn timeout_terminal_fences_retained_stdout_and_stderr() { + let (_root, _session, view, bash) = output_fixture(); + let error = tokio::time::timeout(std::time::Duration::from_secs(5), bash.execute(r#"{"command":"printf before; printf err-before >&2; while :; do :; done","cwd":".","timeout":1}"#, Default::default())).await.unwrap().unwrap_err(); + let ToolError::Interrupted(out) = error else { + panic!("expected timeout output"); + }; + let body: serde_json::Value = serde_json::from_str(&out.content.unwrap()).unwrap(); + let id = body["output"]["id"].as_str().unwrap(); + assert_eq!(body["output"]["terminal"]["status"], "timed_out"); + assert_eq!(body["output"]["terminal"]["transport_complete"], true); + assert_eq!( + view.read(id, workdir::CommandStream::Stdout, 0, 65536) + .unwrap(), + b"before" + ); + assert_eq!( + view.read(id, workdir::CommandStream::Stderr, 0, 65536) + .unwrap(), + b"err-before" + ); +} + +// Integration: real process pipes and provider replay into the production Bash collector. +// Only response delivery is fault-injected; Runtime's HTTP suite covers actual wire routing. +#[tokio::test] +async fn lost_page_and_terminal_responses_replay_without_restarting_or_double_appending() { + for lose_terminal_only in [false, true] { + let dir = TempDir::new().unwrap(); + let session = Arc::new(ExternalValidatingSession { + inner: LocalWorkdirSession::new( + Scope::writable(dir.path()).unwrap(), + dir.path().into(), + ), + truncate_first_binary_response: false, + mutate_after_first_binary_response: None, + command_response_losses: std::sync::atomic::AtomicUsize::new(1), + lose_terminal_only, + }); + let view = tools::bash_output::BashOutputView::new(); + let definitions = core_builtin_tools(session.clone(), Tracker::new(), view.clone()); + let bash = Registry::new(definitions).get("Bash"); + let out = call(&bash, json!({"cwd":".","command":"printf once >> effect.txt; printf 'x%.0s' {1..200000}; printf err >&2; exit 3"})).await; + let body: serde_json::Value = serde_json::from_str(&out.content.unwrap()).unwrap(); + assert_eq!( + std::fs::read(dir.path().join("effect.txt")).unwrap(), + b"once" + ); + assert_eq!( + session + .command_response_losses + .load(std::sync::atomic::Ordering::Acquire), + 0 + ); + assert_eq!(body["output"]["stdout_saved"], 200000); + assert_eq!(body["output"]["stdout_delivered"], 200000); + assert_eq!(body["output"]["stderr_saved"], 3); + assert_eq!(body["output"]["terminal"]["exit_code"], 3); + assert_eq!(body["output"]["terminal"]["transport_complete"], true); + assert_eq!(body["output"]["storage_truncated"], false); + let id = body["output"]["id"].as_str().unwrap(); + for offset in (0..200000).step_by(65536) { + assert_eq!( + view.read(id, workdir::CommandStream::Stdout, offset, 65536) + .unwrap(), + vec![b'x'; (200000 - offset).min(65536)] + ); + } + use workdir::WorkdirSession as _; + session.close().await.unwrap(); + } +} diff --git a/crates/workdir/Cargo.toml b/crates/workdir/Cargo.toml index 0afb851e7..b8541897a 100644 --- a/crates/workdir/Cargo.toml +++ b/crates/workdir/Cargo.toml @@ -20,7 +20,7 @@ serde_json.workspace = true sha2.workspace = true tempfile.workspace = true thiserror.workspace = true -tokio = { workspace = true, features = ["process", "rt", "sync", "time"] } +tokio = { workspace = true, features = ["io-util", "process", "rt", "sync", "time"] } [dev-dependencies] serde_json.workspace = true diff --git a/crates/workdir/src/command_test_support.rs b/crates/workdir/src/command_test_support.rs new file mode 100644 index 000000000..62748a4d4 --- /dev/null +++ b/crates/workdir/src/command_test_support.rs @@ -0,0 +1,43 @@ +//! Bounded collection for tests whose subject is command routing/lifecycle, +//! rather than the byte-page protocol itself. + +use crate::*; + +#[derive(Debug)] +pub(crate) struct CollectedCommand { + pub content: String, + pub status: CommandStatus, + pub exit_code: Option, + pub timed_out: bool, +} + +#[async_trait::async_trait] +pub(crate) trait CollectCommand: WorkdirSession { + async fn collect_command_output( + &self, + mut request: CommandOutputRequest, + ) -> Result { + let mut bytes = Vec::new(); + request.wait = true; + loop { + let page = self.command_output(request.clone()).await?; + for chunk in page.chunks { + bytes.extend(chunk.bytes); + assert!( + bytes.len() <= 16 * 1024 * 1024, + "fixture output exceeded bounded test collection" + ); + } + request.cursor = page.next_cursor; + if let Some(terminal) = page.terminal { + return Ok(CollectedCommand { + content: String::from_utf8_lossy(&bytes).into_owned(), + status: terminal.status, + exit_code: terminal.exit_code, + timed_out: terminal.status == CommandStatus::TimedOut, + }); + } + } + } +} +impl CollectCommand for T {} diff --git a/crates/workdir/src/external.rs b/crates/workdir/src/external.rs index 203701bc0..aeed5a96f 100644 --- a/crates/workdir/src/external.rs +++ b/crates/workdir/src/external.rs @@ -9,7 +9,8 @@ //! COMMAND remains separate explicit authority. General session capability intersections may form //! subsets independently of this grant hierarchy; the codec never implicitly expands nonhierarchical //! grants. Commands run with the shared host's OS permissions, not a sandbox confining processes -//! to a directory. Output is bounded and inline; provider-local spill paths are not exposed. +//! to a directory. Output uses bounded byte pages with cursor acknowledgements; +//! providers never receive caller-selected output paths. use std::path::{Component, Path}; @@ -18,7 +19,7 @@ use serde::{Deserialize, Serialize}; use crate::http::{WorkdirSessionOperation, WorkdirSessionOperationResult, WorkdirTransportError}; use crate::{BoundedReadLimits, WorkdirId, WorkdirPath, WorkdirSessionCapabilities}; -pub const EXTERNAL_WORKDIR_PROVIDER_PROTOCOL_VERSION: u16 = 2; +pub const EXTERNAL_WORKDIR_PROVIDER_PROTOCOL_VERSION: u16 = 3; /// Maximum serialized Backend-to-provider command frame size. pub const MAX_EXTERNAL_SERVER_FRAME_BYTES: usize = 1024 * 1024; /// Maximum serialized provider-to-Backend result frame size. Read bytes are @@ -33,9 +34,9 @@ const MAX_EXTERNAL_GREP_CONTEXT: usize = 100; const MAX_EXTERNAL_SCOPE_RULES: usize = 64; pub const MAX_EXTERNAL_WRITE_BYTES: usize = 512 * 1024; pub const MAX_EXTERNAL_COMMAND_BYTES: usize = 64 * 1024; -pub const MAX_EXTERNAL_COMMAND_OUTPUT_BYTES: usize = 512 * 1024; +pub const MAX_EXTERNAL_COMMAND_OUTPUT_BYTES: usize = crate::MAX_COMMAND_READ_BYTES; pub const MAX_EXTERNAL_COMMAND_TIMEOUT_SECS: u64 = 600; -const MAX_EXTERNAL_COMMAND_CURSOR: usize = 1_000_000; +const MAX_EXTERNAL_COMMAND_CURSOR: u64 = crate::MAX_COMMAND_TRANSPORT_BYTES; fn valid_command_handle(handle: &crate::CommandHandle) -> bool { !handle.0.is_empty() @@ -267,11 +268,10 @@ fn validate_external_operation(operation: &WorkdirSessionOperation) -> Result<() } WorkdirSessionOperation::CommandStart(request) => { !request.command.is_empty() + && request.validate().is_ok() && request.command.len() <= MAX_EXTERNAL_COMMAND_BYTES && (1..=MAX_EXTERNAL_COMMAND_TIMEOUT_SECS).contains(&request.timeout_secs) - && (1..=MAX_EXTERNAL_COMMAND_OUTPUT_BYTES).contains(&request.output_limit) && is_root_relative(&request.cwd) - && request.spill_dir.is_none() && request.tool_call_id.as_ref().is_none_or(|tool_call_id| { !tool_call_id.is_empty() && tool_call_id.len() <= MAX_PROTOCOL_ID_BYTES @@ -450,11 +450,19 @@ fn validate_external_result(result: &WorkdirSessionOperationResult) -> Result<() WorkdirSessionOperationResult::CommandStart(handle) => valid_command_handle(handle), WorkdirSessionOperationResult::CommandStatus(_) => true, WorkdirSessionOperationResult::CommandOutput(output) => { - output.content.len() <= MAX_EXTERNAL_COMMAND_OUTPUT_BYTES + output.transport_bounds_valid() + && output.chunks.len() <= MAX_EXTERNAL_COMMAND_OUTPUT_BYTES && output - .next_cursor - .is_none_or(|cursor| cursor <= MAX_EXTERNAL_COMMAND_CURSOR) - && output.output_path.is_none() + .chunks + .iter() + .map(|chunk| chunk.bytes.len()) + .sum::() + <= MAX_EXTERNAL_COMMAND_OUTPUT_BYTES + && output.next_cursor <= MAX_EXTERNAL_COMMAND_CURSOR + && output + .terminal + .as_ref() + .is_none_or(|terminal| terminal.status != crate::CommandStatus::Running) } WorkdirSessionOperationResult::CommandCancel => true, }; @@ -801,11 +809,16 @@ mod tests { #[test] fn unsupported_protocol_versions_and_invalid_read_limits_fail_closed() { - let future = r#"{ - "version": 3, - "message": {"kind": "heartbeat", "generation": 1, "sequence": 1} - }"#; - assert!(serde_json::from_str::(future).is_err()); + for version in [ + EXTERNAL_WORKDIR_PROVIDER_PROTOCOL_VERSION - 1, + EXTERNAL_WORKDIR_PROVIDER_PROTOCOL_VERSION + 1, + ] { + let unsupported = serde_json::json!({ + "version": version, + "message": {"kind": "heartbeat", "generation": 1, "sequence": 1} + }); + assert!(serde_json::from_value::(unsupported).is_err()); + } let invalid_limits = r#"{ "max_source_bytes": 1024, @@ -825,32 +838,35 @@ mod tests { assert!(ExternalWorkdirOperation::try_from(absolute).is_err()); let command = WorkdirSessionOperation::CommandStart(CommandRequest { - command: "cat /home/operator/private/session.log".to_string(), + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, + command: "printf unsafe".to_string(), timeout_secs: 1, - output_limit: 1024, - cwd: WorkdirPath::root(), - spill_dir: Some("/home/operator/private/output".into()), + + cwd: serde_json::from_value(serde_json::json!("/absolute")).unwrap(), + tool_call_id: None, }); assert!(ExternalWorkdirOperation::try_from(command).is_err()); let command = WorkdirSessionOperation::CommandStart(CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: "printf bounded".to_string(), timeout_secs: 30, - output_limit: 12 * 1024, + cwd: WorkdirPath::root(), - spill_dir: None, + tool_call_id: Some("tool-call-1".to_string()), }); assert!(ExternalWorkdirOperation::try_from(command).is_ok()); assert!( ExternalWorkdirOperation::try_from(WorkdirSessionOperation::CommandStart( CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: "x".repeat(MAX_EXTERNAL_COMMAND_BYTES + 1), timeout_secs: 30, - output_limit: 12 * 1024, + cwd: WorkdirPath::root(), - spill_dir: None, + tool_call_id: None, } )) @@ -887,13 +903,17 @@ mod tests { assert!( ExternalWorkdirOperationResult::try_from(WorkdirSessionOperationResult::CommandOutput( crate::CommandOutput { - status: crate::CommandStatus::Completed, - exit_code: Some(0), - timed_out: false, - content: "bounded".to_string(), - next_cursor: None, - truncated: false, - output_path: None, + chunks: vec![], + next_cursor: 0, + terminal: Some(crate::CommandTerminal { + status: crate::CommandStatus::Completed, + exit_code: Some(0), + stdout_bytes: 0, + stderr_bytes: 0, + stdout_delivered_bytes: 0, + stderr_delivered_bytes: 0, + truncation_reason: None, + }) } )) .is_ok() @@ -901,13 +921,14 @@ mod tests { assert!( ExternalWorkdirOperationResult::try_from(WorkdirSessionOperationResult::CommandOutput( crate::CommandOutput { - status: crate::CommandStatus::Completed, - exit_code: Some(0), - timed_out: false, - content: "bounded".to_string(), - next_cursor: None, - truncated: true, - output_path: Some("/provider/private/output".into()), + chunks: vec![crate::CommandByteChunk { + sequence: 0, + stream: crate::CommandStream::Stdout, + offset: 0, + bytes: vec![0; MAX_EXTERNAL_COMMAND_OUTPUT_BYTES + 1] + }], + next_cursor: 0, + terminal: None } )) .is_err() @@ -958,7 +979,7 @@ mod tests { #[test] fn protocol_rejects_unknown_nested_fields_and_unbounded_requests() { let unknown = r#"{ - "version": 2, + "version": 3, "message": { "kind": "heartbeat", "generation": 1, @@ -1012,4 +1033,81 @@ mod tests { assert!(ExternalWorkdirOperationId::new("x".repeat(129)).is_err()); assert!(ExternalWorkdirOperationId::new("operation-1").is_ok()); } + + #[test] + fn command_budget_and_page_metadata_obey_external_hard_bounds() { + fn request(budget: u64) -> WorkdirSessionOperation { + WorkdirSessionOperation::CommandStart(CommandRequest { + command: "true".into(), + timeout_secs: 1, + cwd: WorkdirPath::root(), + transport_byte_budget: budget, + tool_call_id: None, + }) + } + for budget in [0, crate::MAX_COMMAND_TRANSPORT_BYTES] { + let operation = ExternalWorkdirOperation::try_from(request(budget)).unwrap(); + let wire = serde_json::to_value(operation).unwrap(); + assert!(serde_json::from_value::(wire).is_ok()); + } + assert!( + ExternalWorkdirOperation::try_from(request(crate::MAX_COMMAND_TRANSPORT_BYTES + 1)) + .is_err() + ); + fn page() -> crate::CommandOutput { + crate::CommandOutput { + chunks: vec![crate::CommandByteChunk { + sequence: 0, + stream: crate::CommandStream::Stdout, + offset: 0, + bytes: b"abc".to_vec(), + }], + next_cursor: 3, + terminal: Some(crate::CommandTerminal { + status: crate::CommandStatus::Completed, + exit_code: Some(0), + stdout_bytes: 100 * 1024 * 1024, + stderr_bytes: 7, + stdout_delivered_bytes: 3, + stderr_delivered_bytes: 0, + truncation_reason: Some(crate::CommandOutputTruncation::ByteBudget), + }), + } + } + let accepted = ExternalWorkdirOperationResult::try_from( + WorkdirSessionOperationResult::CommandOutput(page()), + ) + .unwrap(); + assert!( + serde_json::from_value::( + serde_json::to_value(accepted).unwrap() + ) + .is_ok() + ); + for mutate in [ + |p: &mut crate::CommandOutput| p.next_cursor = crate::MAX_COMMAND_TRANSPORT_BYTES + 1, + |p: &mut crate::CommandOutput| p.chunks[0].offset = crate::MAX_COMMAND_TRANSPORT_BYTES, + |p: &mut crate::CommandOutput| p.chunks[0].sequence = u64::MAX, + |p: &mut crate::CommandOutput| p.terminal.as_mut().unwrap().stdout_delivered_bytes = 4, + |p: &mut crate::CommandOutput| p.terminal.as_mut().unwrap().stdout_bytes = 2, + |p: &mut crate::CommandOutput| p.terminal.as_mut().unwrap().truncation_reason = None, + ] { + let mut invalid = page(); + mutate(&mut invalid); + let wire = serde_json::to_value(WorkdirSessionOperationResult::CommandOutput(invalid)) + .unwrap(); + assert!(serde_json::from_value::(wire).is_err()); + } + assert!( + ExternalWorkdirOperation::try_from(WorkdirSessionOperation::CommandOutput( + crate::CommandOutputRequest { + handle: crate::CommandHandle("command-1".into()), + cursor: crate::MAX_COMMAND_TRANSPORT_BYTES + 1, + limit: 1, + wait: false, + } + )) + .is_err() + ); + } } diff --git a/crates/workdir/src/external_local_tests.rs b/crates/workdir/src/external_local_tests.rs index 0a316586f..7332d6a06 100644 --- a/crates/workdir/src/external_local_tests.rs +++ b/crates/workdir/src/external_local_tests.rs @@ -1,6 +1,9 @@ +#[cfg(test)] +use crate::command_test_support::CollectCommand as _; use crate::http::{ WorkdirSessionOperation, WorkdirSessionOperationResult, dispatch_workdir_session_operation, }; + use crate::{ BoundedReadLimits, CommandOutputRequest, CommandRequest, EditRequest, ExternalWorkdirRoot, GlobRequest, GrepOutputMode, GrepRequest, LocalWorkdirSession, ReadRequest, Workdir, @@ -59,11 +62,12 @@ async fn external_local_provider_is_strictly_read_only() { let command = WorkdirSession::start_command( &session, CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: "echo changed > item.txt".to_string(), timeout_secs: 1, - output_limit: 1024, + cwd: WorkdirPath::root(), - spill_dir: None, + tool_call_id: None, }, ) @@ -145,11 +149,12 @@ async fn external_local_provider_read_write_is_filesystem_only() { let command = WorkdirSession::start_command( &session, CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: "touch command-ran".to_string(), timeout_secs: 1, - output_limit: 1024, + cwd: WorkdirPath::root(), - spill_dir: None, + tool_call_id: None, }, ) @@ -193,17 +198,18 @@ async fn external_command_permission_is_explicit_and_independent_from_file_acces let handle = session .start_command(CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: "printf command-ok".to_string(), timeout_secs: 5, - output_limit: 1024, + cwd: WorkdirPath::root(), - spill_dir: None, + tool_call_id: None, }) .await .unwrap(); let output = session - .command_output(CommandOutputRequest { + .collect_command_output(CommandOutputRequest { handle, cursor: 0, limit: 1024, diff --git a/crates/workdir/src/http.rs b/crates/workdir/src/http.rs index f9ff246db..32bcc6c60 100644 --- a/crates/workdir/src/http.rs +++ b/crates/workdir/src/http.rs @@ -822,6 +822,7 @@ mod client { &self, request: CommandRequest, ) -> Result { + request.validate()?; match self .operate(WorkdirSessionOperation::CommandStart(request)) .await? @@ -848,17 +849,39 @@ mod client { &self, request: CommandOutputRequest, ) -> Result { + if !(1..=crate::MAX_COMMAND_READ_BYTES).contains(&request.limit) { + return Err(WorkdirError::InvalidArgument( + "command byte limit is outside transport bounds".into(), + )); + } let wait = request.wait; - loop { - match self - .operate(WorkdirSessionOperation::CommandOutput(request.clone())) - .await? - { - WorkdirSessionOperationResult::CommandOutput(result) - if wait && result.status == CommandStatus::Running => {} - WorkdirSessionOperationResult::CommandOutput(result) => return Ok(result), - _ => return Err(Self::mismatch("command_output")), + match self + .operate(WorkdirSessionOperation::CommandOutput(request.clone())) + .await? + { + WorkdirSessionOperationResult::CommandOutput(result) => { + let size = result + .chunks + .iter() + .try_fold(0usize, |size, chunk| size.checked_add(chunk.bytes.len())); + if !result.transport_bounds_valid() + || size.is_none_or(|size| { + size > request.limit + || request.cursor.checked_add(size as u64) + != Some(result.next_cursor) + }) + { + return Err(Self::mismatch("bounded command byte page")); + } + if wait && result.chunks.is_empty() && result.terminal.is_none() { + // A waiting request must block at the provider. Never spin HTTP + // requests against an immediate empty response. A caller may retry + // this exact cursor with its own bounded transport retry policy. + return Err(Self::mismatch("waiting command byte page")); + } + return Ok(result); } + _ => return Err(Self::mismatch("command_output")), } } @@ -917,20 +940,47 @@ mod client { response: reqwest::Response, ) -> Result { if response.status().is_success() { - response.json().await.map_err(http_unavailable) + let bytes = bounded_response_bytes(response).await?; + serde_json::from_slice(&bytes) + .map_err(|_| WorkdirError::Transport("Runtime Workdir response was invalid".into())) } else { Err(decode_error(response).await) } } + async fn bounded_response_bytes( + mut response: reqwest::Response, + ) -> Result, WorkdirError> { + const MAX_RESPONSE_BYTES: usize = crate::external::MAX_EXTERNAL_PROVIDER_FRAME_BYTES; + if response + .content_length() + .is_some_and(|size| size > MAX_RESPONSE_BYTES as u64) + { + return Err(WorkdirError::Transport( + "Runtime Workdir response exceeded transport bound".into(), + )); + } + let mut bytes = Vec::new(); + while let Some(chunk) = response.chunk().await.map_err(http_unavailable)? { + if chunk.len() > MAX_RESPONSE_BYTES.saturating_sub(bytes.len()) { + return Err(WorkdirError::Transport( + "Runtime Workdir response exceeded transport bound".into(), + )); + } + bytes.extend_from_slice(&chunk); + } + Ok(bytes) + } + async fn decode_error(response: reqwest::Response) -> WorkdirError { - response - .json::() - .await - .map(WorkdirTransportError::into_workdir_error) - .unwrap_or_else(|_| { - WorkdirError::Transport("Runtime Workdir error response was invalid".to_string()) - }) + match bounded_response_bytes(response).await { + Ok(bytes) => serde_json::from_slice::(&bytes) + .map(WorkdirTransportError::into_workdir_error) + .unwrap_or_else(|_| { + WorkdirError::Transport("Runtime Workdir error response was invalid".into()) + }), + Err(error) => error, + } } fn http_unavailable(_error: reqwest::Error) -> WorkdirError { diff --git a/crates/workdir/src/lib.rs b/crates/workdir/src/lib.rs index de9e7f8e5..7f052d5be 100644 --- a/crates/workdir/src/lib.rs +++ b/crates/workdir/src/lib.rs @@ -618,3 +618,6 @@ mod capability_tests { assert_eq!(bits(WorkdirSessionCapabilities::ALL), 63); } } + +#[cfg(test)] +mod command_test_support; diff --git a/crates/workdir/src/local.rs b/crates/workdir/src/local.rs index f750bf0ee..2b819964a 100644 --- a/crates/workdir/src/local.rs +++ b/crates/workdir/src/local.rs @@ -13,7 +13,8 @@ use std::collections::{BTreeMap, HashMap}; use std::fmt::Debug; -use std::io::{Read as _, Seek as _, SeekFrom, Write as _}; +#[cfg(test)] +use std::io::Write as _; #[cfg(unix)] use std::os::unix::process::CommandExt as _; use std::path::{Path, PathBuf}; @@ -54,15 +55,8 @@ fn command_observed_at_ms() -> u64 { .unwrap_or(u64::MAX) } -#[derive(Debug)] -enum LocalCommand { - Running { - task: JoinHandle>, - completion: watch::Receiver, - cancel: watch::Sender, - }, - Completed(CommandOutput), -} +mod command_transport; +use command_transport::LocalCommand; #[derive(Debug, Clone)] struct CommandTelemetry { @@ -643,7 +637,6 @@ struct LocalWorkdirSessionInner { root: PathBuf, pinned_root: Option>, scope: SharedScope, - command_output_scope: SharedScope, cwd: PathBuf, capabilities: WorkdirSessionCapabilities, read_limits: Option, @@ -662,9 +655,7 @@ impl Drop for LocalWorkdirSessionInner { fn drop(&mut self) { if let Ok(mut commands) = self.commands.try_lock() { for (_, command) in commands.drain() { - if let LocalCommand::Running { task, .. } = command { - task.abort(); - } + command.task.abort(); } } } @@ -864,37 +855,12 @@ impl LocalWorkdirSession { capabilities: WorkdirSessionCapabilities, command_environment: BTreeMap, resources: Vec>, - ) -> Self { - Self::materialized_bound_with_environment_and_command_output_scope( - workdir, - root, - cwd, - scope.clone(), - scope, - capabilities, - command_environment, - resources, - ) - } - - /// Construct a local provider session whose Workdir filesystem authority is - /// independent from the provider-owned command output authority. - pub fn materialized_bound_with_environment_and_command_output_scope( - workdir: Workdir, - root: PathBuf, - cwd: PathBuf, - scope: SharedScope, - command_output_scope: SharedScope, - capabilities: WorkdirSessionCapabilities, - command_environment: BTreeMap, - resources: Vec>, ) -> Self { Self::materialized_bound_with_policy( workdir, root, cwd, scope, - command_output_scope, capabilities, command_environment, resources, @@ -1024,7 +990,6 @@ impl LocalWorkdirSession { workdir, root.clone(), root, - scope.clone(), scope, capabilities, BTreeMap::new(), @@ -1052,7 +1017,6 @@ impl LocalWorkdirSession { root: PathBuf, cwd: PathBuf, scope: SharedScope, - command_output_scope: SharedScope, capabilities: WorkdirSessionCapabilities, command_environment: BTreeMap, resources: Vec>, @@ -1066,7 +1030,6 @@ impl LocalWorkdirSession { root, pinned_root, scope, - command_output_scope, cwd, capabilities, read_limits, @@ -1831,20 +1794,12 @@ impl WorkdirSession for LocalWorkdirSession { } async fn start_command(&self, request: CommandRequest) -> Result { + request.validate()?; self.ensure_capability(WorkdirSessionCapability::Command)?; // Command admission, resource capture and registration must be atomic // with terminal close, which drains commands and releases resources. let _admission = self.inner.close_lock.lock().await; self.ensure_open()?; - if let Some(spill_dir) = request.spill_dir.as_deref() - && !self - .inner - .command_output_scope - .snapshot() - .is_readable(spill_dir) - { - return Err(WorkdirError::OutOfScope(spill_dir.to_path_buf())); - } let cwd = self.resolve(&request.cwd); let scope = self.inner.scope.snapshot(); if !scope.is_readable(&cwd) @@ -1854,48 +1809,45 @@ impl WorkdirSession for LocalWorkdirSession { } let id = self.inner.next_command_id.fetch_add(1, Ordering::Relaxed); let handle = CommandHandle(format!("command-{id}")); - let (completion_tx, completion) = watch::channel(false); - let command_id = handle.0.clone(); - let telemetry = self.inner.command_telemetry.clone(); - let command_environment = self.inner.command_environment.clone(); - let (cancel, cancel_rx) = watch::channel(false); - // A session drop only schedules command cancellation. Keep its Runtime - // resources (including cleanup exclusion) alive until the task actually - // finishes or its future is dropped, not just until the handle disappears. + let mut commands = self.inner.commands.lock().await; + // Retain bounded retry tombstones, evicting only fully delivered output. + if commands.len() >= command_transport::MAX_COMMANDS { + let mut evict = None; + for (id, command) in commands.iter() { + if command.task.is_finished() + && command + .transport + .try_lock() + .is_ok_and(|state| state.terminal_delivered()) + { + evict = Some(id.clone()); + break; + } + } + if let Some(id) = evict { + commands.remove(&id); + self.inner.command_telemetry.remove(&id); + } else { + return Err(WorkdirError::Unavailable( + "command transport capacity reached; drain existing commands".into(), + )); + } + } let resources = self .inner .resources .lock() .unwrap_or_else(|error| error.into_inner()) .clone(); - let task = tokio::spawn(async move { - let output = run_command( - cwd, - request, - command_id, - telemetry, - command_environment, - cancel_rx, - resources, - ) - .await; - let _ = completion_tx.send(true); - output - }); - let mut commands = self.inner.commands.lock().await; - if let Err(error) = self.ensure_open() { - let _ = cancel.send(true); - task.abort(); - return Err(error); - } - commands.insert( + let command = command_transport::start( + cwd, + request, handle.0.clone(), - LocalCommand::Running { - task, - completion, - cancel, - }, + self.inner.command_telemetry.clone(), + self.inner.command_environment.clone(), + resources, ); + commands.insert(handle.0.clone(), command); Ok(handle) } @@ -1905,17 +1857,11 @@ impl WorkdirSession for LocalWorkdirSession { let command = commands .get(&handle.0) .ok_or_else(|| WorkdirError::UnknownCommand(handle.0.clone()))?; - Ok(match command { - LocalCommand::Running { task, .. } if !task.is_finished() => CommandStatus::Running, - LocalCommand::Running { .. } => self - .inner - .command_telemetry - .snapshot() - .into_iter() - .find(|snapshot| snapshot.command_id == handle.0) - .map_or(CommandStatus::Completed, |snapshot| snapshot.status), - LocalCommand::Completed(output) => output.status, - }) + Ok(command + .status + .borrow() + .as_ref() + .map_or(CommandStatus::Running, |terminal| terminal.status)) } async fn command_output( @@ -1923,70 +1869,22 @@ impl WorkdirSession for LocalWorkdirSession { request: CommandOutputRequest, ) -> Result { self.ensure_capability(WorkdirSessionCapability::Command)?; - let command = loop { - self.ensure_open()?; - let mut commands = self.inner.commands.lock().await; - let Some(command) = commands.get(&request.handle.0) else { - return Err(WorkdirError::UnknownCommand(request.handle.0.clone())); - }; - let completion = match command { - LocalCommand::Running { completion, .. } - if !*completion.borrow() && completion.has_changed().is_ok() => - { - Some(completion.clone()) - } - _ => None, - }; - if let Some(mut completion) = completion { - if !request.wait { - return Ok(CommandOutput { - status: CommandStatus::Running, - exit_code: None, - timed_out: false, - content: String::new(), - next_cursor: None, - truncated: false, - output_path: None, - }); - } - drop(commands); - let _ = completion.changed().await; - continue; - } - if !request.wait - && matches!(command, LocalCommand::Running { task, .. } if !task.is_finished()) - { - return Ok(CommandOutput { - status: CommandStatus::Running, - exit_code: None, - timed_out: false, - content: String::new(), - next_cursor: None, - truncated: false, - output_path: None, - }); - } - break commands - .remove(&request.handle.0) - .expect("command checked above"); - }; - - let output = match command { - LocalCommand::Running { task, .. } => task - .await - .map_err(|error| WorkdirError::Unavailable(error.to_string()))??, - LocalCommand::Completed(output) => output, - }; - let page = command_output_page(&output, request.cursor, request.limit); - if page.next_cursor.is_some() { - let mut commands = self.inner.commands.lock().await; - if !self.inner.closed.load(Ordering::Acquire) { - commands.insert(request.handle.0, LocalCommand::Completed(output)); - } - } else { + self.ensure_open()?; + let transport = self + .inner + .commands + .lock() + .await + .get(&request.handle.0) + .ok_or_else(|| WorkdirError::UnknownCommand(request.handle.0.clone()))? + .transport + .clone(); + let result = transport.lock().await.read(&request).await; + self.ensure_open()?; + if result.as_ref().is_ok_and(|page| page.terminal.is_some()) { self.inner.command_telemetry.remove(&request.handle.0); } - Ok(page) + result } async fn cancel_command(&self, handle: CommandHandle) -> Result<(), WorkdirError> { @@ -1996,12 +1894,7 @@ impl WorkdirSession for LocalWorkdirSession { let command = commands .get(&handle.0) .ok_or_else(|| WorkdirError::UnknownCommand(handle.0.clone()))?; - match command { - LocalCommand::Running { task, cancel, .. } if !task.is_finished() => { - Some(cancel.clone()) - } - _ => None, - } + Some(command.cancel.clone()) }; if let Some(cancel) = cancel { let _ = cancel.send(true); @@ -2041,13 +1934,9 @@ impl WorkdirSession for LocalWorkdirSession { .collect::>() }; for command in commands { - match command { - LocalCommand::Running { task, cancel, .. } => { - let _ = cancel.send(true); - let _ = task.await; - } - LocalCommand::Completed(_) => {} - } + let _ = command.cancel.send(true); + command.transport.lock().await.close(); + let _ = command.task.await; } if let Ok(mut resources) = self.inner.resources.lock() { resources.clear(); @@ -2056,27 +1945,6 @@ impl WorkdirSession for LocalWorkdirSession { } } -fn command_output_page(output: &CommandOutput, cursor: usize, limit: usize) -> CommandOutput { - let total_chars = output.content.chars().count(); - let start = cursor.min(total_chars); - let content = output - .content - .chars() - .skip(start) - .take(limit.max(1)) - .collect::(); - let end = start + content.chars().count(); - CommandOutput { - status: output.status, - exit_code: output.exit_code, - timed_out: output.timed_out, - content, - next_cursor: (end < total_chars).then_some(end), - truncated: output.truncated || end < total_chars, - output_path: output.output_path.clone(), - } -} - // Discovery of an absent route is not a stale-validator execution attempt. fn checkout_observe_io(path: &WorkdirPath, error: std::io::Error) -> WorkdirError { if matches!( @@ -2202,167 +2070,6 @@ fn terminate_process_group(process_group_id: u32) { #[cfg(not(unix))] fn terminate_process_group(_process_group_id: u32) {} -async fn run_command( - cwd: PathBuf, - request: CommandRequest, - command_id: String, - telemetry: CommandTelemetry, - command_environment: BTreeMap, - mut cancel: watch::Receiver, - resources: Vec>, -) -> Result { - let stdout = tempfile::NamedTempFile::new().map_err(|error| WorkdirError::io(&cwd, error))?; - let stderr = tempfile::NamedTempFile::new().map_err(|error| WorkdirError::io(&cwd, error))?; - let stdout_path = stdout.into_temp_path(); - let stderr_path = stderr.into_temp_path(); - let stdout_file = std::fs::File::create(&stdout_path) - .map_err(|error| WorkdirError::io(&stdout_path, error))?; - let stderr_file = std::fs::File::create(&stderr_path) - .map_err(|error| WorkdirError::io(&stderr_path, error))?; - - telemetry.started(&command_id, request.tool_call_id.clone()); - let mut command = Command::new("bash"); - command - .arg("-c") - .arg(&request.command) - .current_dir(&cwd) - .envs(command_environment) - .stdin(Stdio::null()) - .stdout(Stdio::from(stdout_file)) - .stderr(Stdio::from(stderr_file)) - .kill_on_drop(true); - #[cfg(unix)] - command.as_std_mut().process_group(0); - let mut child = match command.spawn() { - Ok(child) => child, - Err(error) => { - telemetry.terminal(&command_id, CommandStatus::Failed, None); - return Err(WorkdirError::io(&cwd, error)); - } - }; - let mut process_group = CommandProcessGroupGuard::new(child.id()); - - let mut stdout_reader = - std::fs::File::open(&stdout_path).map_err(|error| WorkdirError::io(&stdout_path, error))?; - let mut stderr_reader = - std::fs::File::open(&stderr_path).map_err(|error| WorkdirError::io(&stderr_path, error))?; - let mut stdout_decoder = CommandOutputDecoder::default(); - let mut stderr_decoder = CommandOutputDecoder::default(); - let mut timeout = Box::pin(tokio::time::sleep(Duration::from_secs( - request.timeout_secs.max(1), - ))); - let mut interval = tokio::time::interval(Duration::from_millis(50)); - interval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip); - interval.tick().await; - - let (status, exit_code) = loop { - tokio::select! { - exit = child.wait() => { - let exit = exit.map_err(|error| WorkdirError::io(&cwd, error))?; - process_group.terminate(); - break ( - if exit.success() { CommandStatus::Completed } else { CommandStatus::Failed }, - exit.code(), - ); - } - _ = &mut timeout => { - process_group.terminate(); - let _ = child.start_kill(); - let exit_code = child.wait().await.ok().and_then(|status| status.code()); - break (CommandStatus::TimedOut, exit_code); - } - changed = cancel.changed() => { - if changed.is_err() || *cancel.borrow() { - process_group.terminate(); - let _ = child.start_kill(); - let exit_code = child.wait().await.ok().and_then(|status| status.code()); - break (CommandStatus::Cancelled, exit_code); - } - } - _ = interval.tick() => { - publish_available_output( - &mut stdout_reader, - &mut stdout_decoder, - &telemetry, - &command_id, - CommandStream::Stdout, - &stdout_path, - false, - )?; - publish_available_output( - &mut stderr_reader, - &mut stderr_decoder, - &telemetry, - &command_id, - CommandStream::Stderr, - &stderr_path, - false, - )?; - } - } - }; - - publish_available_output( - &mut stdout_reader, - &mut stdout_decoder, - &telemetry, - &command_id, - CommandStream::Stdout, - &stdout_path, - true, - )?; - publish_available_output( - &mut stderr_reader, - &mut stderr_decoder, - &telemetry, - &command_id, - CommandStream::Stderr, - &stderr_path, - true, - )?; - telemetry.terminal(&command_id, status, exit_code); - - let (content, truncated) = - read_command_output_files(&stdout_path, &stderr_path, request.output_limit.max(1))?; - let output_path = match (truncated, request.spill_dir) { - (true, Some(spill_dir)) => { - let stdout_path = stdout_path.to_path_buf(); - let stderr_path = stderr_path.to_path_buf(); - Some( - spawn_resource_owned_blocking(resources.clone(), move || { - persist_command_output(&stdout_path, &stderr_path, &spill_dir) - }) - .await - .map_err(|error| { - WorkdirError::Unavailable(format!("Bash output spill task failed: {error}")) - })??, - ) - } - _ => None, - }; - Ok(CommandOutput { - status, - exit_code, - timed_out: status == CommandStatus::TimedOut, - content, - next_cursor: None, - truncated, - output_path, - }) -} - -// Blocking spills cannot be aborted with their async waiter. Transfer resource -// ownership into the actual filesystem task so cleanup waits for its completion. -fn spawn_resource_owned_blocking( - resources: Vec>, - effect: impl FnOnce() -> T + Send + 'static, -) -> JoinHandle { - tokio::task::spawn_blocking(move || { - let _resources = resources; - effect() - }) -} - #[derive(Debug, Default)] struct CommandOutputDecoder { read_offset: u64, @@ -2370,38 +2077,6 @@ struct CommandOutputDecoder { pending: Vec, } -fn publish_available_output( - file: &mut std::fs::File, - decoder: &mut CommandOutputDecoder, - telemetry: &CommandTelemetry, - command_id: &str, - stream: CommandStream, - path: &Path, - flush: bool, -) -> Result<(), WorkdirError> { - file.seek(SeekFrom::Start(decoder.read_offset)) - .map_err(|error| WorkdirError::io(path, error))?; - loop { - let mut buffer = vec![0; COMMAND_EVENT_CHUNK_BYTES]; - let read = file - .read(&mut buffer) - .map_err(|error| WorkdirError::io(path, error))?; - if read == 0 { - publish_decoded_output(decoder, telemetry, command_id, stream, flush); - return Ok(()); - } - decoder.pending.extend_from_slice(&buffer[..read]); - decoder.read_offset = decoder.read_offset.saturating_add(read as u64); - publish_decoded_output(decoder, telemetry, command_id, stream, false); - if read < COMMAND_EVENT_CHUNK_BYTES { - if flush { - publish_decoded_output(decoder, telemetry, command_id, stream, true); - } - return Ok(()); - } - } -} - fn publish_decoded_output( decoder: &mut CommandOutputDecoder, telemetry: &CommandTelemetry, @@ -2428,7 +2103,7 @@ fn publish_decoded_output( } /// Return the byte prefix that can be decoded now without replacing a valid -/// UTF-8 scalar whose remaining bytes may arrive in a later file read. Definite +/// UTF-8 scalar whose remaining bytes may arrive in a later pipe read. Definite /// invalid sequences remain in the prefix and are rendered lossily, preserving /// the existing arbitrary-byte output behavior. fn stable_utf8_prefix_len(bytes: &[u8]) -> usize { @@ -2448,110 +2123,6 @@ fn stable_utf8_prefix_len(bytes: &[u8]) -> usize { inspected } -fn persist_command_output( - stdout_path: &Path, - stderr_path: &Path, - spill_dir: &Path, -) -> Result { - std::fs::create_dir_all(spill_dir).map_err(|error| WorkdirError::io(spill_dir, error))?; - let mut artifact = tempfile::Builder::new() - .prefix("bash-") - .suffix(".log") - .tempfile_in(spill_dir) - .map_err(|error| WorkdirError::io(spill_dir, error))?; - let artifact_path = artifact.path().to_path_buf(); - - let mut stdout = - std::fs::File::open(stdout_path).map_err(|error| WorkdirError::io(stdout_path, error))?; - let stdout_len = stdout - .metadata() - .map_err(|error| WorkdirError::io(stdout_path, error))? - .len(); - std::io::copy(&mut stdout, &mut artifact) - .map_err(|error| WorkdirError::io(&artifact_path, error))?; - - let mut stderr = - std::fs::File::open(stderr_path).map_err(|error| WorkdirError::io(stderr_path, error))?; - let stderr_len = stderr - .metadata() - .map_err(|error| WorkdirError::io(stderr_path, error))? - .len(); - if stdout_len > 0 && stderr_len > 0 { - stdout - .seek(SeekFrom::End(-1)) - .map_err(|error| WorkdirError::io(stdout_path, error))?; - let mut last = [0_u8; 1]; - stdout - .read_exact(&mut last) - .map_err(|error| WorkdirError::io(stdout_path, error))?; - if last[0] != b'\n' { - artifact - .write_all(b"\n") - .map_err(|error| WorkdirError::io(&artifact_path, error))?; - } - } - std::io::copy(&mut stderr, &mut artifact) - .map_err(|error| WorkdirError::io(&artifact_path, error))?; - artifact - .flush() - .map_err(|error| WorkdirError::io(&artifact_path, error))?; - artifact - .keep() - .map(|(_, path)| path) - .map_err(|error| WorkdirError::io(&artifact_path, error.error)) -} - -fn read_command_output_files( - stdout_path: &Path, - stderr_path: &Path, - limit: usize, -) -> Result<(String, bool), WorkdirError> { - let stdout_size = std::fs::metadata(stdout_path) - .map_err(|error| WorkdirError::io(stdout_path, error))? - .len() as usize; - let stderr_size = std::fs::metadata(stderr_path) - .map_err(|error| WorkdirError::io(stderr_path, error))? - .len() as usize; - let total = stdout_size.saturating_add(stderr_size); - let stderr_budget = stderr_size.min(limit / 2); - let stdout_budget = stdout_size.min(limit.saturating_sub(stderr_budget)); - let remaining = limit.saturating_sub(stdout_budget + stderr_budget); - let stdout_budget = (stdout_budget + remaining.min(stdout_size - stdout_budget)).min(limit); - let stderr_budget = limit.saturating_sub(stdout_budget).min(stderr_size); - - let stdout = read_tail(stdout_path, stdout_budget)?; - let stderr = read_tail(stderr_path, stderr_budget)?; - let mut content = String::new(); - if !stdout.is_empty() { - content.push_str(&String::from_utf8_lossy(&stdout)); - } - if !stderr.is_empty() { - if !content.is_empty() && !content.ends_with('\n') { - content.push('\n'); - } - content.push_str(&String::from_utf8_lossy(&stderr)); - } - Ok((content, total > limit)) -} - -fn read_tail(path: &Path, limit: usize) -> Result, WorkdirError> { - if limit == 0 { - return Ok(Vec::new()); - } - let mut file = std::fs::File::open(path).map_err(|error| WorkdirError::io(path, error))?; - let len = file - .metadata() - .map_err(|error| WorkdirError::io(path, error))? - .len(); - let start = len.saturating_sub(limit as u64); - file.seek(SeekFrom::Start(start)) - .map_err(|error| WorkdirError::io(path, error))?; - let mut bytes = Vec::with_capacity((len - start) as usize); - file.read_to_end(&mut bytes) - .map_err(|error| WorkdirError::io(path, error))?; - Ok(bytes) -} - /// Return the first symlink component in `path`, if one exists. /// /// The function only inspects existing path components. It intentionally uses @@ -2928,11 +2499,12 @@ mod tests { let command = WorkdirSession::start_command( &session, CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: "sleep 30".to_owned(), timeout_secs: 60, - output_limit: 1024, + cwd: WorkdirPath::root(), - spill_dir: None, + tool_call_id: None, }, ) @@ -2941,7 +2513,7 @@ mod tests { let waiting_session = session.clone(); let waiting_command = command.clone(); let waiter = tokio::spawn(async move { - WorkdirSession::command_output( + crate::command_test_support::CollectCommand::collect_command_output( &waiting_session, CommandOutputRequest { handle: waiting_command, @@ -3626,17 +3198,18 @@ mod tests { let handle = WorkdirSession::start_command( &workdir, CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: "pwd && printf provider-command".into(), timeout_secs: 5, - output_limit: 4096, + cwd: WorkdirPath::root(), - spill_dir: None, + tool_call_id: None, }, ) .await .unwrap(); - let output = WorkdirSession::command_output( + let output = crate::command_test_support::CollectCommand::collect_command_output( &workdir, CommandOutputRequest { handle, @@ -3728,11 +3301,12 @@ mod tests { let error = WorkdirSession::start_command( &workdir, CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: "printf hidden".into(), timeout_secs: 5, - output_limit: 1024, + cwd, - spill_dir: None, + tool_call_id: None, }, ) @@ -3742,319 +3316,6 @@ mod tests { assert!(matches!(error, WorkdirError::OutOfScope(path) if path == outside.path())); } - #[tokio::test] - async fn command_rejects_spill_directory_without_read_scope() { - let dir = TempDir::new().unwrap(); - let spill = TempDir::new().unwrap(); - let workdir = make_fs(&dir); - - let error = WorkdirSession::start_command( - &workdir, - CommandRequest { - command: "printf hidden".into(), - timeout_secs: 5, - output_limit: 1, - cwd: WorkdirPath::root(), - spill_dir: Some(spill.path().to_path_buf()), - tool_call_id: None, - }, - ) - .await - .unwrap_err(); - - assert!(matches!(error, WorkdirError::OutOfScope(path) if path == spill.path())); - } - - #[tokio::test] - async fn truncated_command_output_is_retained_in_the_requested_spill_directory() { - let dir = TempDir::new().unwrap(); - let spill = TempDir::new().unwrap(); - let scope = Scope::from_config(&ScopeConfig { - allow: vec![ - ScopeRule { - target: dir.path().to_path_buf(), - permission: Permission::Write, - recursive: true, - symlink_policy: Default::default(), - }, - ScopeRule { - target: spill.path().to_path_buf(), - permission: Permission::Read, - recursive: true, - symlink_policy: Default::default(), - }, - ], - deny: Vec::new(), - }) - .unwrap(); - let workdir = LocalWorkdirSession::new(scope, dir.path().to_path_buf()); - let handle = WorkdirSession::start_command( - &workdir, - CommandRequest { - command: "i=0; while [ $i -lt 200 ]; do printf 'line-%03d\\n' \"$i\"; i=$((i+1)); done; printf 'FINAL-NEEDLE\\n'".into(), - timeout_secs: 5, - output_limit: 64, - cwd: WorkdirPath::root(), - spill_dir: Some(spill.path().to_path_buf()), - tool_call_id: None, - }, - ) - .await - .unwrap(); - let output = WorkdirSession::command_output( - &workdir, - CommandOutputRequest { - handle, - cursor: 0, - limit: 4096, - wait: true, - }, - ) - .await - .unwrap(); - - assert!(output.truncated); - let output_path = output.output_path.expect("retained output path"); - assert_eq!(output_path.parent(), Some(spill.path())); - let retained = std::fs::read_to_string(&output_path).unwrap(); - assert!(retained.starts_with("line-000\n")); - assert!(retained.ends_with("FINAL-NEEDLE\n")); - assert_eq!(retained.lines().count(), 201); - - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt as _; - assert_eq!( - std::fs::metadata(output_path).unwrap().permissions().mode() & 0o777, - 0o600 - ); - } - } - - #[tokio::test] - async fn completed_command_output_can_be_read_in_bounded_unicode_pages() { - let dir = TempDir::new().unwrap(); - let workdir = make_fs(&dir); - let handle = WorkdirSession::start_command( - &workdir, - CommandRequest { - command: "printf 'aéz'".into(), - timeout_secs: 5, - output_limit: 1024, - cwd: WorkdirPath::root(), - spill_dir: None, - tool_call_id: None, - }, - ) - .await - .unwrap(); - let first = WorkdirSession::command_output( - &workdir, - CommandOutputRequest { - handle: handle.clone(), - cursor: 0, - limit: 2, - wait: true, - }, - ) - .await - .unwrap(); - assert_eq!(first.content, "aé"); - assert_eq!(first.next_cursor, Some(2)); - - let second = WorkdirSession::command_output( - &workdir, - CommandOutputRequest { - handle: handle.clone(), - cursor: first.next_cursor.unwrap(), - limit: 2, - wait: false, - }, - ) - .await - .unwrap(); - assert_eq!(second.content, "z"); - assert_eq!(second.next_cursor, None); - assert!(matches!( - WorkdirSession::command_status(&workdir, handle).await, - Err(WorkdirError::UnknownCommand(_)) - )); - } - - #[test] - fn command_output_decoder_preserves_utf8_split_across_file_reads() { - let dir = TempDir::new().unwrap(); - let path = dir.path().join("command.out"); - let mut first_write = vec![b'a'; COMMAND_EVENT_CHUNK_BYTES - 1]; - first_write.push(0xe2); - std::fs::write(&path, first_write).unwrap(); - - let telemetry = CommandTelemetry::new(); - let mut events = telemetry.subscribe(); - telemetry.started("command-utf8", None); - let mut decoder = CommandOutputDecoder::default(); - let mut reader = std::fs::File::open(&path).unwrap(); - publish_available_output( - &mut reader, - &mut decoder, - &telemetry, - "command-utf8", - CommandStream::Stdout, - &path, - false, - ) - .unwrap(); - assert_eq!(decoder.pending, vec![0xe2]); - - let mut writer = std::fs::OpenOptions::new() - .append(true) - .open(&path) - .unwrap(); - writer.write_all(&[0x82, 0xac]).unwrap(); - writer.flush().unwrap(); - publish_available_output( - &mut reader, - &mut decoder, - &telemetry, - "command-utf8", - CommandStream::Stdout, - &path, - false, - ) - .unwrap(); - - let output = std::iter::from_fn(|| events.try_recv().ok()) - .filter_map(|event| match event { - CommandEvent::Output { - stream: CommandStream::Stdout, - content, - .. - } => Some(content), - _ => None, - }) - .collect::(); - assert_eq!(output.len(), COMMAND_EVENT_CHUNK_BYTES - 1 + "€".len()); - assert!(output.ends_with('€')); - assert!(!output.contains('\u{fffd}')); - assert!(decoder.pending.is_empty()); - } - - #[tokio::test] - async fn command_output_does_not_rewait_before_join_handle_finishes() { - let dir = TempDir::new().unwrap(); - let workdir = make_fs(&dir); - let handle = CommandHandle("command-completion-race".into()); - let (completion_tx, completion) = watch::channel(false); - let completion_observer = completion_tx.clone(); - let (cancel, _cancel_rx) = watch::channel(false); - let (start_tx, start_rx) = tokio::sync::oneshot::channel::<()>(); - let (completion_sent_tx, completion_sent_rx) = tokio::sync::oneshot::channel::<()>(); - let (release_tx, release_rx) = tokio::sync::oneshot::channel::<()>(); - let task = tokio::spawn(async move { - start_rx.await.unwrap(); - completion_tx.send(true).unwrap(); - completion_sent_tx.send(()).unwrap(); - release_rx.await.unwrap(); - Ok(CommandOutput { - status: CommandStatus::Completed, - exit_code: Some(0), - timed_out: false, - content: "done".into(), - next_cursor: None, - truncated: false, - output_path: None, - }) - }); - workdir.inner.commands.lock().await.insert( - handle.0.clone(), - LocalCommand::Running { - task, - completion, - cancel, - }, - ); - - let waiting_workdir = workdir.clone(); - let waiting_handle = handle.clone(); - let waiter = tokio::spawn(async move { - WorkdirSession::command_output( - &waiting_workdir, - CommandOutputRequest { - handle: waiting_handle, - cursor: 0, - limit: 1024, - wait: true, - }, - ) - .await - }); - for _ in 0..100 { - if completion_observer.receiver_count() >= 2 { - break; - } - tokio::task::yield_now().await; - } - assert_eq!( - completion_observer.receiver_count(), - 2, - "waiter must subscribe before completion is published" - ); - - start_tx.send(()).unwrap(); - completion_sent_rx.await.unwrap(); - tokio::task::yield_now().await; - assert!( - !waiter.is_finished(), - "command_output should await the task after observing completion state" - ); - release_tx.send(()).unwrap(); - - let output = tokio::time::timeout(Duration::from_secs(1), waiter) - .await - .expect("command output must not wait for a second completion notification") - .unwrap() - .unwrap(); - assert_eq!(output.status, CommandStatus::Completed); - assert_eq!(output.content, "done"); - } - - #[tokio::test] - async fn command_output_does_not_wait_forever_when_completion_sender_drops() { - let dir = TempDir::new().unwrap(); - let workdir = make_fs(&dir); - let handle = CommandHandle("command-completion-drop".into()); - let (completion_tx, completion) = watch::channel(false); - let (cancel, _cancel_rx) = watch::channel(false); - let task: JoinHandle> = tokio::spawn(async move { - drop(completion_tx); - panic!("simulated command task panic"); - }); - workdir.inner.commands.lock().await.insert( - handle.0.clone(), - LocalCommand::Running { - task, - completion, - cancel, - }, - ); - - let result = tokio::time::timeout( - Duration::from_secs(1), - WorkdirSession::command_output( - &workdir, - CommandOutputRequest { - handle, - cursor: 0, - limit: 1024, - wait: true, - }, - ), - ) - .await - .expect("closed completion channel must wake the waiter"); - assert!(matches!(result, Err(WorkdirError::Unavailable(_)))); - } - #[tokio::test] async fn provider_streams_bounded_command_lifecycle_and_distinct_output() { let dir = TempDir::new().unwrap(); @@ -4064,11 +3325,12 @@ mod tests { let handle = WorkdirSession::start_command( &workdir, CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: "printf ready; printf warning >&2; sleep 0.2; printf done".into(), timeout_secs: 5, - output_limit: 1024, + cwd: WorkdirPath::root(), - spill_dir: None, + tool_call_id: Some("tool-7".into()), }, ) @@ -4146,7 +3408,7 @@ mod tests { assert_eq!(snapshot[0].stdout.content, "readydone"); assert_eq!(snapshot[0].stderr.content, "warning"); - let output = WorkdirSession::command_output( + let output = crate::command_test_support::CollectCommand::collect_command_output( &workdir, CommandOutputRequest { handle, @@ -4169,17 +3431,18 @@ mod tests { let handle = WorkdirSession::start_command( &workdir, CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: "sleep 30".into(), timeout_secs: 1, - output_limit: 1024, + cwd: WorkdirPath::root(), - spill_dir: None, + tool_call_id: None, }, ) .await .unwrap(); - let output = WorkdirSession::command_output( + let output = crate::command_test_support::CollectCommand::collect_command_output( &workdir, CommandOutputRequest { handle: handle.clone(), @@ -4227,11 +3490,12 @@ mod tests { WorkdirSession::start_command( start_session.as_ref(), CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: "true".into(), timeout_secs: 5, - output_limit: 1024, + cwd: WorkdirPath::root(), - spill_dir: None, + tool_call_id: None, }, ) @@ -4261,50 +3525,6 @@ mod tests { ); } - #[tokio::test] - async fn blocking_output_spill_retains_resources_after_waiter_is_cancelled() { - #[derive(Debug)] - struct Resource(Option>); - impl Drop for Resource { - fn drop(&mut self) { - let _ = self.0.take().unwrap().send(()); - } - } - let (released, mut release) = tokio::sync::oneshot::channel(); - let resources: Vec> = - vec![Arc::new(Resource(Some(released)))]; - let (entered, started) = tokio::sync::oneshot::channel(); - let (finish, wait) = std::sync::mpsc::channel(); - // Exercise the resource-transfer policy at the same blocking boundary - // used by output persistence, without filesystem timing assumptions. - let waiter = tokio::spawn(async move { - spawn_resource_owned_blocking(resources, move || { - entered.send(()).unwrap(); - wait.recv_timeout(Duration::from_secs(5)).unwrap(); - }) - .await - .unwrap(); - }); - tokio::time::timeout(Duration::from_secs(5), started) - .await - .unwrap() - .unwrap(); - waiter.abort(); - assert!(waiter.await.unwrap_err().is_cancelled()); - assert!( - matches!( - release.try_recv(), - Err(tokio::sync::oneshot::error::TryRecvError::Empty) - ), - "blocking spill lost its resources when the waiter was cancelled" - ); - finish.send(()).unwrap(); - tokio::time::timeout(Duration::from_secs(5), release) - .await - .unwrap() - .unwrap(); - } - #[tokio::test(flavor = "current_thread")] async fn session_drop_retains_resources_until_command_task_cancellation_completes() { #[derive(Debug)] @@ -4328,11 +3548,12 @@ mod tests { WorkdirSession::start_command( &session, CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: "true".into(), timeout_secs: 5, - output_limit: 1024, + cwd: WorkdirPath::root(), - spill_dir: None, + tool_call_id: None, }, ) @@ -4387,12 +3608,13 @@ mod tests { let handle = WorkdirSession::start_command( &workdir, CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: "(sleep 0.4; echo leaked > child-marker) & echo ready > child-ready; wait" .into(), timeout_secs: 60, - output_limit: 1024, + cwd: WorkdirPath::root(), - spill_dir: None, + tool_call_id: None, }, ) @@ -4409,7 +3631,7 @@ mod tests { WorkdirSession::cancel_command(&workdir, handle.clone()) .await .unwrap(); - let output = WorkdirSession::command_output( + let output = crate::command_test_support::CollectCommand::collect_command_output( &workdir, CommandOutputRequest { handle, @@ -4435,11 +3657,12 @@ mod tests { let handle = WorkdirSession::start_command( &workdir, CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: "sleep 30".into(), timeout_secs: 60, - output_limit: 1024, + cwd: WorkdirPath::root(), - spill_dir: None, + tool_call_id: None, }, ) @@ -4454,7 +3677,7 @@ mod tests { let waiting_workdir = workdir.clone(); let waiting_handle = handle.clone(); let waiter = tokio::spawn(async move { - WorkdirSession::command_output( + crate::command_test_support::CollectCommand::collect_command_output( &waiting_workdir, CommandOutputRequest { handle: waiting_handle, @@ -4475,9 +3698,11 @@ mod tests { .unwrap() .unwrap(); assert_eq!(output.status, CommandStatus::Cancelled); - assert!(matches!( - WorkdirSession::command_status(&workdir, handle).await, - Err(WorkdirError::UnknownCommand(_)) - )); + assert_eq!( + WorkdirSession::command_status(&workdir, handle) + .await + .unwrap(), + CommandStatus::Cancelled + ); } } diff --git a/crates/workdir/src/local/command_transport.rs b/crates/workdir/src/local/command_transport.rs new file mode 100644 index 000000000..5b800c0ab --- /dev/null +++ b/crates/workdir/src/local/command_transport.rs @@ -0,0 +1,377 @@ +//! Captured process bytes use a bounded queue, never the lossy Console channel. +//! Pipe backpressure bounds unread process output without temporary-file growth. +//! A replay page survives a lost HTTP response until its next cursor is sent. + +use super::*; +use tokio::io::{AsyncRead, AsyncReadExt}; +use tokio::sync::mpsc; + +const QUEUE_CHUNKS: usize = 8; +pub(super) const MAX_COMMANDS: usize = 64; + +#[derive(Debug)] +enum Message { + Bytes(crate::CommandByteChunk), +} + +#[derive(Debug)] +pub(super) struct LocalCommand { + pub task: JoinHandle<()>, + pub cancel: watch::Sender, + pub status: watch::Receiver>, + pub transport: Arc>, +} + +#[derive(Debug)] +pub(super) struct Transport { + receiver: mpsc::Receiver, + pending: Option, + cursor: u64, + replay: Option<(u64, CommandOutput)>, + terminal: Option, + terminal_status: watch::Receiver>, +} + +impl Transport { + pub async fn read( + &mut self, + request: &CommandOutputRequest, + ) -> Result { + if !(1..=crate::MAX_COMMAND_READ_BYTES).contains(&request.limit) { + return Err(WorkdirError::Conflict( + "command byte limit is outside transport bounds".into(), + )); + } + if let Some((cursor, page)) = &self.replay { + if request.cursor == *cursor { + if page + .chunks + .iter() + .map(|chunk| chunk.bytes.len()) + .sum::() + > request.limit + { + return Err(WorkdirError::Conflict( + "retry byte limit is smaller than retained page".into(), + )); + } + return Ok(page.clone()); + } + if request.cursor != page.next_cursor { + return Err(WorkdirError::Conflict( + "command cursor is not the current page acknowledgement".into(), + )); + } + self.cursor = page.next_cursor; + self.replay = None; + } + if request.cursor != self.cursor { + return Err(WorkdirError::Conflict( + "command cursor was acknowledged or is ahead of output".into(), + )); + } + let mut page = CommandOutput { + chunks: Vec::new(), + next_cursor: self.cursor, + terminal: self.terminal.clone(), + }; + let mut remaining = request.limit; + loop { + let message = if let Some(chunk) = self.pending.take() { + Some(Message::Bytes(chunk)) + } else if page.chunks.is_empty() && page.terminal.is_none() && request.wait { + self.receiver.recv().await + } else { + match self.receiver.try_recv() { + Ok(message) => Some(message), + Err(mpsc::error::TryRecvError::Empty) => break, + Err(mpsc::error::TryRecvError::Disconnected) => None, + } + }; + match message { + Some(Message::Bytes(mut chunk)) => { + let count = remaining.min(chunk.bytes.len()); + if count < chunk.bytes.len() { + let tail = chunk.bytes.split_off(count); + self.pending = Some(crate::CommandByteChunk { + sequence: chunk.sequence + count as u64, + stream: chunk.stream, + offset: chunk.offset + count as u64, + bytes: tail, + }); + } + page.next_cursor += count as u64; + remaining -= count; + page.chunks.push(chunk); + if remaining == 0 { + break; + } + } + None if page.terminal.is_some() => break, + None => { + let terminal = self.terminal_status.borrow().clone().ok_or_else(|| { + WorkdirError::Unavailable( + "command transport ended before terminal fence".into(), + ) + })?; + self.terminal = Some(terminal.clone()); + page.terminal = Some(terminal); + break; + } + } + } + // An empty poll must not become a permanently replayed empty page. + if !page.chunks.is_empty() || page.terminal.is_some() { + self.replay = Some((self.cursor, page.clone())); + } + Ok(page) + } + + pub fn terminal_delivered(&self) -> bool { + self.replay + .as_ref() + .is_some_and(|(_, page)| page.terminal.is_some()) + } + + pub fn close(&mut self) { + self.receiver.close(); + } +} + +pub(super) fn start( + cwd: PathBuf, + request: CommandRequest, + command_id: String, + telemetry: CommandTelemetry, + environment: BTreeMap, + resources: Vec>, +) -> LocalCommand { + let (sender, receiver) = mpsc::channel(QUEUE_CHUNKS); + let (cancel, cancel_rx) = watch::channel(false); + let (status_tx, status) = watch::channel(None); + let transport = Arc::new(Mutex::new(Transport { + receiver, + pending: None, + cursor: 0, + replay: None, + terminal: None, + terminal_status: status.clone(), + })); + let task = tokio::spawn(async move { + let _resources = resources; + let terminal = execute( + cwd, + request, + &command_id, + &telemetry, + environment, + cancel_rx, + &sender, + ) + .await; + telemetry.terminal(&command_id, terminal.status, terminal.exit_code); + // Closing the byte queue fences this already-published terminal, without + // requiring spare queue capacity or a receiver to release task resources. + let _ = status_tx.send(Some(terminal)); + }); + LocalCommand { + task, + cancel, + status, + transport, + } +} + +#[derive(Default)] +struct ByteAccounting { + stdout: u64, + stderr: u64, + stdout_delivered: u64, + stderr_delivered: u64, +} + +impl ByteAccounting { + fn terminal(&self, status: CommandStatus, exit_code: Option) -> crate::CommandTerminal { + crate::CommandTerminal { + status, + exit_code, + stdout_bytes: self.stdout, + stderr_bytes: self.stderr, + stdout_delivered_bytes: self.stdout_delivered, + stderr_delivered_bytes: self.stderr_delivered, + truncation_reason: (self.stdout > self.stdout_delivered + || self.stderr > self.stderr_delivered) + .then_some(crate::CommandOutputTruncation::ByteBudget), + } + } +} + +async fn pump( + mut stdout: R, + mut stderr: impl AsyncRead + Unpin, + sender: &mpsc::Sender, + telemetry: &CommandTelemetry, + command_id: &str, + mut remaining: u64, + accounting: &mut ByteAccounting, +) -> std::io::Result<()> { + let mut stdout_done = false; + let mut stderr_done = false; + let mut stdout_decoder = CommandOutputDecoder::default(); + let mut stderr_decoder = CommandOutputDecoder::default(); + let mut sequence = 0; + let mut stdout_buffer = [0; COMMAND_EVENT_CHUNK_BYTES]; + let mut stderr_buffer = [0; COMMAND_EVENT_CHUNK_BYTES]; + while !stdout_done || !stderr_done { + // After quota, drain locally even if nobody acknowledges the queue. + // Before quota, reserve first to retain bounded pipe backpressure. + let permit = if remaining > 0 { + let Ok(permit) = sender.reserve().await else { + return Ok(()); + }; + Some(permit) + } else { + None + }; + let (stream, result) = tokio::select! { + result = stdout.read(&mut stdout_buffer), if !stdout_done => (CommandStream::Stdout, result), + result = stderr.read(&mut stderr_buffer), if !stderr_done => (CommandStream::Stderr, result), + }; + let buffer = match stream { + CommandStream::Stdout => &stdout_buffer, + CommandStream::Stderr => &stderr_buffer, + }; + let count = result?; + let decoder = match stream { + CommandStream::Stdout => &mut stdout_decoder, + CommandStream::Stderr => &mut stderr_decoder, + }; + if count == 0 { + match stream { + CommandStream::Stdout => stdout_done = true, + CommandStream::Stderr => stderr_done = true, + } + publish_decoded_output(decoder, telemetry, command_id, stream, true); + continue; + } + let (seen, delivered) = match stream { + CommandStream::Stdout => (&mut accounting.stdout, &mut accounting.stdout_delivered), + CommandStream::Stderr => (&mut accounting.stderr, &mut accounting.stderr_delivered), + }; + *seen = seen.saturating_add(count as u64); + let captured = remaining.min(count as u64) as usize; + if captured > 0 { + let offset = *delivered; + decoder.read_offset += captured as u64; + decoder.pending.extend_from_slice(&buffer[..captured]); + publish_decoded_output(decoder, telemetry, command_id, stream, false); + permit + .expect("reserved while budget remains") + .send(Message::Bytes(crate::CommandByteChunk { + sequence, + stream, + offset, + bytes: buffer[..captured].to_vec(), + })); + *delivered += captured as u64; + sequence += captured as u64; + remaining -= captured as u64; + } + } + Ok(()) +} + +async fn execute( + cwd: PathBuf, + request: CommandRequest, + command_id: &str, + telemetry: &CommandTelemetry, + environment: BTreeMap, + mut cancel: watch::Receiver, + sender: &mpsc::Sender, +) -> crate::CommandTerminal { + telemetry.started(command_id, request.tool_call_id); + let mut command = Command::new("bash"); + command + .arg("-c") + .arg(request.command) + .current_dir(cwd) + .envs(environment) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .kill_on_drop(true); + #[cfg(unix)] + command.as_std_mut().process_group(0); + let mut accounting = ByteAccounting::default(); + let Ok(mut child) = command.spawn() else { + return accounting.terminal(CommandStatus::Failed, None); + }; + let mut process_group = CommandProcessGroupGuard::new(child.id()); + let stdout = child.stdout.take().expect("piped stdout"); + let stderr = child.stderr.take().expect("piped stderr"); + let (status, exit_code) = { + let output = pump( + stdout, + stderr, + sender, + telemetry, + command_id, + request.transport_byte_budget, + &mut accounting, + ); + tokio::pin!(output); + let mut drained = false; + let timeout = tokio::time::sleep(Duration::from_secs(request.timeout_secs.max(1))); + tokio::pin!(timeout); + let terminal = loop { + tokio::select! { + result = &mut output, if !drained => { + drained = true; + if result.is_err() || sender.is_closed() { + process_group.terminate(); + let _ = child.start_kill(); + let _ = child.wait().await; + break (CommandStatus::Failed, None); + } + } + exit = child.wait() => { + process_group.terminate(); + break match exit { + Ok(exit) => (if exit.success() { CommandStatus::Completed } else { CommandStatus::Failed }, exit.code()), + Err(_) => (CommandStatus::Failed, None), + }; + } + _ = &mut timeout => { + process_group.terminate(); + let _ = child.start_kill(); + let code = child.wait().await.ok().and_then(|exit| exit.code()); + break (CommandStatus::TimedOut, code); + } + _ = sender.closed() => { + process_group.terminate(); + let _ = child.start_kill(); + let _ = child.wait().await; + break (CommandStatus::Cancelled, None); + } + changed = cancel.changed() => { + if changed.is_err() || *cancel.borrow() { + process_group.terminate(); + let _ = child.start_kill(); + let code = child.wait().await.ok().and_then(|exit| exit.code()); + break (CommandStatus::Cancelled, code); + } + } + } + }; + // Even after kill, preserve the bounded pipe tail before publishing terminal. + if !drained { + let _ = output.await; + } + terminal + }; + accounting.terminal(status, exit_code) +} + +#[cfg(test)] +mod tests; diff --git a/crates/workdir/src/local/command_transport/tests.rs b/crates/workdir/src/local/command_transport/tests.rs new file mode 100644 index 000000000..796a1f93f --- /dev/null +++ b/crates/workdir/src/local/command_transport/tests.rs @@ -0,0 +1,490 @@ +//! Process/pipe integration through the production local provider. Temporary roots +//! and provider tasks are RAII-owned; byte pulls and bounded timeouts synchronize tests. + +use super::*; +use tempfile::TempDir; + +struct Harness { + session: LocalWorkdirSession, + _root: TempDir, +} + +impl Harness { + fn new() -> Self { + let root = TempDir::new().unwrap(); + let session = LocalWorkdirSession::new( + Scope::writable(root.path()).unwrap(), + root.path().to_path_buf(), + ); + Self { + session, + _root: root, + } + } + + async fn start(&self, command: &str, timeout_secs: u64) -> CommandHandle { + self.start_budget(command, timeout_secs, crate::MAX_COMMAND_TRANSPORT_BYTES) + .await + } + + fn request(command: &str, timeout_secs: u64, budget: u64) -> CommandRequest { + CommandRequest { + transport_byte_budget: budget, + command: command.into(), + timeout_secs, + cwd: WorkdirPath::root(), + tool_call_id: None, + } + } + + async fn start_budget(&self, command: &str, timeout_secs: u64, budget: u64) -> CommandHandle { + self.session + .start_command(Self::request(command, timeout_secs, budget)) + .await + .unwrap() + } + + async fn page(&self, handle: &CommandHandle, cursor: u64, limit: usize) -> CommandOutput { + tokio::time::timeout( + Duration::from_secs(10), + self.session.command_output(CommandOutputRequest { + handle: handle.clone(), + cursor, + limit, + wait: true, + }), + ) + .await + .expect("bounded command pull") + .unwrap() + } + + async fn collect( + &self, + handle: &CommandHandle, + mut cursor: u64, + ) -> (Vec, crate::CommandTerminal) { + let mut chunks = Vec::new(); + loop { + let page = self + .page(handle, cursor, crate::MAX_COMMAND_READ_BYTES) + .await; + assert!( + page.chunks + .iter() + .map(|chunk| chunk.bytes.len()) + .sum::() + <= crate::MAX_COMMAND_READ_BYTES + ); + cursor = page.next_cursor; + chunks.extend(page.chunks); + assert!(chunks.iter().map(|chunk| chunk.bytes.len()).sum::() <= 8 * 1024 * 1024); + if let Some(terminal) = page.terminal { + return (chunks, terminal); + } + } + } +} + +fn stream_bytes(chunks: &[crate::CommandByteChunk], stream: CommandStream) -> Vec { + let mut bytes = Vec::new(); + for chunk in chunks.iter().filter(|chunk| chunk.stream == stream) { + assert_eq!(chunk.offset, bytes.len() as u64); + bytes.extend_from_slice(&chunk.bytes); + } + bytes +} + +#[tokio::test] +async fn high_single_line_output_survives_absent_and_lagged_console_subscribers() { + for lagged in [false, true] { + let harness = Harness::new(); + let _lagged_receiver = lagged.then(|| harness.session.subscribe_command_events().unwrap()); + let handle = harness.start("head -c 3145728 /dev/zero | tr '\\0' x; head -c 65536 /dev/zero | tr '\\0' e >&2; exit 7", 20).await; + let (chunks, terminal) = harness.collect(&handle, 0).await; + assert_eq!(terminal.status, CommandStatus::Failed); + assert_eq!(terminal.exit_code, Some(7)); + assert_eq!( + stream_bytes(&chunks, CommandStream::Stdout), + vec![b'x'; 3145728] + ); + assert_eq!( + stream_bytes(&chunks, CommandStream::Stderr), + vec![b'e'; 65536] + ); + let mut cursor = 0; + for chunk in chunks { + assert_eq!(chunk.sequence, cursor); + cursor += chunk.bytes.len() as u64; + } + harness.session.close().await.unwrap(); + } +} + +#[tokio::test] +async fn one_byte_pages_preserve_raw_utf8_invalid_bytes_and_replay_until_acknowledged() { + let harness = Harness::new(); + let handle = harness + .start( + "printf '\\342\\202\\254\\377\\000'; printf '\\346\\227\\245' >&2", + 5, + ) + .await; + let mut chunks = Vec::new(); + let mut cursor = 0; + loop { + let page = harness.page(&handle, cursor, 1).await; + assert_eq!(harness.page(&handle, cursor, 1).await, page); + cursor = page.next_cursor; + chunks.extend(page.chunks); + if let Some(terminal) = page.terminal { + assert_eq!(terminal.status, CommandStatus::Completed); + break; + } + } + assert_eq!( + stream_bytes(&chunks, CommandStream::Stdout), + b"\xe2\x82\xac\xff\0" + ); + assert_eq!( + stream_bytes(&chunks, CommandStream::Stderr), + b"\xe6\x97\xa5" + ); + assert!( + harness + .session + .command_output(CommandOutputRequest { + handle, + cursor: 0, + limit: 1, + wait: false + }) + .await + .is_err() + ); +} + +#[tokio::test] +async fn cancellation_while_backpressured_drains_bytes_before_terminal_fence() { + let harness = Harness::new(); + let handle = harness.start("printf ready; exec yes x", 30).await; + let first = harness.page(&handle, 0, 1).await; + harness + .session + .cancel_command(handle.clone()) + .await + .unwrap(); + let (mut chunks, terminal) = harness.collect(&handle, first.next_cursor).await; + let mut all = first.chunks; + all.append(&mut chunks); + assert!(stream_bytes(&all, CommandStream::Stdout).starts_with(b"ready")); + assert_eq!(terminal.status, CommandStatus::Cancelled); + let final_page = harness + .page( + &handle, + all.iter().map(|chunk| chunk.bytes.len() as u64).sum(), + 1, + ) + .await; + assert!(final_page.chunks.is_empty()); + assert_eq!(final_page.terminal, Some(terminal)); +} + +#[tokio::test] +async fn timeout_and_session_close_finalize_without_an_output_subscriber() { + let harness = Harness::new(); + let handle = harness + .start("printf ready; exec tail -f /dev/null", 1) + .await; + let (chunks, terminal) = harness.collect(&handle, 0).await; + assert_eq!(stream_bytes(&chunks, CommandStream::Stdout), b"ready"); + assert_eq!(terminal.status, CommandStatus::TimedOut); + let handle = harness.start("exec yes x", 30).await; + // Receiving one byte establishes actual process output, without a timing sleep. + harness.page(&handle, 0, 1).await; + tokio::time::timeout(Duration::from_secs(5), harness.session.close()) + .await + .unwrap() + .unwrap(); + assert!(harness.session.command_status(handle).await.is_err()); +} + +#[tokio::test] +async fn interleaved_streams_keep_independent_offsets_and_bounded_read_validation() { + let harness = Harness::new(); + let handle = harness + .start( + "for ((i=0;i<100;i++)); do printf 'out%s;' \"$i\"; printf 'err%s;' \"$i\" >&2; done", + 5, + ) + .await; + for limit in [0, crate::MAX_COMMAND_READ_BYTES + 1] { + assert!( + harness + .session + .command_output(CommandOutputRequest { + handle: handle.clone(), + cursor: 0, + limit, + wait: false + }) + .await + .is_err() + ); + } + let (chunks, terminal) = harness.collect(&handle, 0).await; + let expected = |prefix| { + (0..100) + .map(|i| format!("{prefix}{i};")) + .collect::() + .into_bytes() + }; + assert_eq!( + stream_bytes(&chunks, CommandStream::Stdout), + expected("out") + ); + assert_eq!( + stream_bytes(&chunks, CommandStream::Stderr), + expected("err") + ); + assert_eq!(terminal.status, CommandStatus::Completed); +} + +#[tokio::test] +async fn byte_budget_zero_exact_partial_and_shared_keep_seen_and_delivered_distinct() { + let harness = Harness::new(); + for (command, budget, stdout_seen, stderr_seen, exit_code) in [ + ("printf abc; printf defg >&2", 0, 3, 4, 0), + ("printf abc", 3, 3, 0, 0), + ("printf abcdefg", 3, 7, 0, 0), + ("printf abcdefg; exit 7", 3, 7, 0, 7), + ( + "head -c 32768 /dev/zero; head -c 32768 /dev/zero >&2", + 12345, + 32768, + 32768, + 0, + ), + ] { + let handle = harness.start_budget(command, 5, budget).await; + let (chunks, terminal) = harness.collect(&handle, 0).await; + let stdout = stream_bytes(&chunks, CommandStream::Stdout); + let stderr = stream_bytes(&chunks, CommandStream::Stderr); + let mut cursor = 0; + for chunk in &chunks { + assert_eq!(chunk.sequence, cursor); + cursor += chunk.bytes.len() as u64; + } + assert_eq!(cursor, budget.min(stdout_seen + stderr_seen)); + assert_eq!(terminal.stdout_bytes, stdout_seen); + assert_eq!(terminal.stderr_bytes, stderr_seen); + assert_eq!(terminal.stdout_delivered_bytes, stdout.len() as u64); + assert_eq!(terminal.stderr_delivered_bytes, stderr.len() as u64); + assert_eq!(terminal.exit_code, Some(exit_code)); + assert_eq!( + terminal.status, + if exit_code == 0 { + CommandStatus::Completed + } else { + CommandStatus::Failed + } + ); + assert_eq!( + terminal.truncation_reason, + (cursor < stdout_seen + stderr_seen) + .then_some(crate::CommandOutputTruncation::ByteBudget) + ); + } +} + +#[tokio::test] +async fn exhausted_budget_finishes_without_receiver_and_keeps_timeout_cancel_distinct() { + let harness = Harness::new(); + for budget in [0, 1, (QUEUE_CHUNKS * COMMAND_EVENT_CHUNK_BYTES) as u64] { + let handle = harness + .start_budget("head -c 1048576 /dev/zero; printf error >&2", 5, budget) + .await; + let mut status = harness.session.inner.commands.lock().await[&handle.0] + .status + .clone(); + let terminal = tokio::time::timeout( + Duration::from_secs(5), + status.wait_for(|value| value.is_some()), + ) + .await + .unwrap() + .unwrap() + .clone() + .unwrap(); + assert_eq!(terminal.status, CommandStatus::Completed); + assert_eq!(terminal.stdout_bytes, 1048576); + assert_eq!(terminal.stderr_bytes, 5); + assert_eq!( + terminal.stdout_delivered_bytes + terminal.stderr_delivered_bytes, + budget + ); + // Full capture queue cannot retain the process task or its resources. + { + let mut commands = harness.session.inner.commands.lock().await; + tokio::time::timeout( + Duration::from_secs(5), + &mut commands.get_mut(&handle.0).unwrap().task, + ) + .await + .unwrap() + .unwrap(); + } + // Only now acknowledge the captured queue and terminal fence. + let (_, delivered_terminal) = harness.collect(&handle, 0).await; + assert_eq!(delivered_terminal, terminal); + } + for (cancel, expected) in [ + (false, CommandStatus::TimedOut), + (true, CommandStatus::Cancelled), + ] { + let handle = harness + .start_budget( + "printf ready-extra; head -c 32768 /dev/zero; exec tail -f /dev/null", + 1, + 5, + ) + .await; + let first = harness.page(&handle, 0, 5).await; + assert_eq!(first.next_cursor, 5); + if cancel { + harness + .session + .cancel_command(handle.clone()) + .await + .unwrap(); + } + let (rest, terminal) = harness.collect(&handle, first.next_cursor).await; + assert!(rest.is_empty()); + assert_eq!(terminal.status, expected); + assert_eq!(terminal.stdout_delivered_bytes, 5); + assert!(terminal.stdout_bytes >= 11); + assert_eq!( + terminal.truncation_reason, + Some(crate::CommandOutputTruncation::ByteBudget) + ); + if !cancel { + assert_eq!(terminal.stdout_bytes, 32779); + assert_eq!( + terminal.truncation_reason, + Some(crate::CommandOutputTruncation::ByteBudget) + ); + } + } +} + +#[tokio::test] +async fn budget_replay_preserves_unique_bytes_and_rejects_future_and_old_cursors() { + let harness = Harness::new(); + let handle = harness.start_budget("printf abcdefghij", 5, 7).await; + let first = harness.page(&handle, 0, 2).await; + for _ in 0..3 { + assert_eq!(harness.page(&handle, 0, 2).await, first); + } + assert!( + harness + .session + .command_output(CommandOutputRequest { + handle: handle.clone(), + cursor: 8, + limit: 2, + wait: false, + }) + .await + .is_err() + ); + let (rest, terminal) = harness.collect(&handle, first.next_cursor).await; + let mut all = first.chunks; + all.extend(rest); + assert_eq!(stream_bytes(&all, CommandStream::Stdout), b"abcdefg"); + assert_eq!(terminal.stdout_bytes, 10); + assert_eq!(terminal.stdout_delivered_bytes, 7); + assert!( + harness + .session + .command_output(CommandOutputRequest { + handle, + cursor: 0, + limit: 2, + wait: false, + }) + .await + .is_err() + ); +} + +#[tokio::test] +async fn invalid_budget_is_rejected_before_process_start() { + let harness = Harness::new(); + assert!(matches!( + harness + .session + .start_command(Harness::request( + "touch must-not-exist", + 5, + crate::MAX_COMMAND_TRANSPORT_BYTES + 1, + )) + .await, + Err(WorkdirError::InvalidArgument(_)) + )); + assert!(!harness._root.path().join("must-not-exist").exists()); + assert!(harness.session.inner.commands.lock().await.is_empty()); +} + +#[tokio::test] +async fn slow_receiver_backpressures_before_quota_but_full_queue_does_not_block_discard() { + use std::future::Future; + use std::task::Poll; + let telemetry = CommandTelemetry::new(); + let input = vec![b'x'; 1024 * 1024]; + for budget in [ + crate::MAX_COMMAND_TRANSPORT_BYTES, + (QUEUE_CHUNKS * COMMAND_EVENT_CHUNK_BYTES) as u64, + ] { + let (sender, mut receiver) = mpsc::channel(QUEUE_CHUNKS); + let mut accounting = ByteAccounting::default(); + { + let output = pump( + input.as_slice(), + &b""[..], + &sender, + &telemetry, + "test", + budget, + &mut accounting, + ); + tokio::pin!(output); + let polled = std::future::poll_fn(|cx| Poll::Ready(output.as_mut().poll(cx))).await; + if budget == crate::MAX_COMMAND_TRANSPORT_BYTES { + assert!(polled.is_pending(), "a slow receiver must stop pipe reads"); + } else { + assert!( + matches!(polled, Poll::Ready(Ok(()))), + "discard must not reserve queue capacity" + ); + } + } + assert_eq!(receiver.len(), QUEUE_CHUNKS); + assert_eq!( + accounting.stdout_delivered, + (QUEUE_CHUNKS * COMMAND_EVENT_CHUNK_BYTES) as u64 + ); + assert_eq!( + accounting.stdout, + if budget == crate::MAX_COMMAND_TRANSPORT_BYTES { + accounting.stdout_delivered + } else { + input.len() as u64 + } + ); + let mut queued_bytes = 0; + while let Ok(Message::Bytes(chunk)) = receiver.try_recv() { + queued_bytes += chunk.bytes.len(); + } + assert_eq!(queued_bytes, QUEUE_CHUNKS * COMMAND_EVENT_CHUNK_BYTES); + } +} diff --git a/crates/workdir/src/operation.rs b/crates/workdir/src/operation.rs index 3c2eb2318..7e180e7aa 100644 --- a/crates/workdir/src/operation.rs +++ b/crates/workdir/src/operation.rs @@ -1,5 +1,3 @@ -use std::path::PathBuf; - use schemars::JsonSchema; use serde::{Deserialize, Serialize}; @@ -8,18 +6,18 @@ use serde::{Deserialize, Serialize}; pub struct CommandHandle(pub String); #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct CommandRequest { pub command: String, #[schemars(range(min = 0, max = 9_007_199_254_740_991_u64))] pub timeout_secs: u64, - #[schemars(range(min = 0, max = 9_007_199_254_740_991_usize))] - pub output_limit: usize, + /// Aggregate unique-byte capture allowance, independent of consumer storage. + /// Providers drain and count excess bytes without delivering their payload. + #[schemars(range(min = 0, max = 67_108_864_u64))] + pub transport_byte_budget: u64, /// Workdir-relative command directory. Providers validate it against the /// active session before process start. pub cwd: fs_operation::FsPath, - /// Provider-local directory where complete output is retained when the - /// inline result exceeds `output_limit`. - pub spill_dir: Option, /// Optional caller-owned correlation id. Bash supplies its tool-call id so /// user-facing command telemetry can update the corresponding Console row /// without exposing provider/session handles. @@ -27,12 +25,23 @@ pub struct CommandRequest { pub tool_call_id: Option, } +impl CommandRequest { + pub fn validate(&self) -> Result<(), crate::WorkdirError> { + if self.transport_byte_budget > MAX_COMMAND_TRANSPORT_BYTES { + return Err(crate::WorkdirError::InvalidArgument( + "command transport byte budget exceeds provider maximum".into(), + )); + } + Ok(()) + } +} + #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] pub struct CommandOutputRequest { pub handle: CommandHandle, - #[schemars(range(min = 0, max = 9_007_199_254_740_991_usize))] - pub cursor: usize, - #[schemars(range(min = 0, max = 9_007_199_254_740_991_usize))] + #[schemars(range(min = 0, max = 67_108_864_u64))] + pub cursor: u64, + #[schemars(range(min = 1, max = 65_536_usize))] pub limit: usize, pub wait: bool, } @@ -107,16 +116,150 @@ pub enum CommandEvent { }, } +/// Sequence is the merged transport byte position. Offset addresses raw bytes +/// within the named stream; neither ordering nor offsets depend on UTF-8 decoding. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] -pub struct CommandOutput { +pub struct CommandByteChunk { + pub sequence: u64, + pub stream: CommandStream, + pub offset: u64, + pub bytes: Vec, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] +#[serde(rename_all = "snake_case")] +pub enum CommandOutputTruncation { + ByteBudget, +} + +/// Raw totals include discarded pipe bytes. Delivered totals count only prefixes +/// admitted to the transport queue, not page reads or retries by a consumer. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] +pub struct CommandTerminal { pub status: CommandStatus, - #[schemars(range(min = -2147483648_i32, max = 2147483647_i32))] pub exit_code: Option, - pub timed_out: bool, - pub content: String, - #[schemars(range(min = 0, max = 9_007_199_254_740_991_usize))] - pub next_cursor: Option, - pub truncated: bool, - /// Complete output retained by the provider when `truncated` is true. - pub output_path: Option, + pub stdout_bytes: u64, + pub stderr_bytes: u64, + pub stdout_delivered_bytes: u64, + pub stderr_delivered_bytes: u64, + pub truncation_reason: Option, +} + +/// Sending next_cursor acknowledges this page. Only the current page may be +/// retried; acknowledged bytes expire. Terminal fences all captured bytes, +/// including on timeout/cancel. Consumers must pull while the process runs. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] +pub struct CommandOutput { + pub chunks: Vec, + pub next_cursor: u64, + pub terminal: Option, +} + +impl CommandOutput { + pub(crate) fn transport_bounds_valid(&self) -> bool { + let mut end = None; + let mut stream_ends = [None, None]; + for chunk in &self.chunks { + let stream_index = match chunk.stream { + CommandStream::Stdout => 0, + CommandStream::Stderr => 1, + }; + if chunk.bytes.is_empty() + || chunk.offset > chunk.sequence + || stream_ends[stream_index].is_some_and(|end| chunk.offset != end) + || end.is_some_and(|end| chunk.sequence != end) + || chunk + .sequence + .checked_add(chunk.bytes.len() as u64) + .is_none_or(|end| end > MAX_COMMAND_TRANSPORT_BYTES) + || chunk + .offset + .checked_add(chunk.bytes.len() as u64) + .is_none_or(|end| end > MAX_COMMAND_TRANSPORT_BYTES) + { + return false; + } + end = chunk.sequence.checked_add(chunk.bytes.len() as u64); + stream_ends[stream_index] = chunk.offset.checked_add(chunk.bytes.len() as u64); + } + self.next_cursor <= MAX_COMMAND_TRANSPORT_BYTES + && end.is_none_or(|end| end == self.next_cursor) + && self.terminal.as_ref().is_none_or(|terminal| { + let truncated = terminal.stdout_bytes > terminal.stdout_delivered_bytes + || terminal.stderr_bytes > terminal.stderr_delivered_bytes; + stream_ends[0].is_none_or(|end| end == terminal.stdout_delivered_bytes) + && stream_ends[1].is_none_or(|end| end == terminal.stderr_delivered_bytes) + && terminal.status != CommandStatus::Running + && terminal.stdout_delivered_bytes <= terminal.stdout_bytes + && terminal.stderr_delivered_bytes <= terminal.stderr_bytes + && terminal + .stdout_delivered_bytes + .checked_add(terminal.stderr_delivered_bytes) + == Some(self.next_cursor) + && terminal.truncation_reason + == truncated.then_some(CommandOutputTruncation::ByteBudget) + }) + } +} + +pub const MAX_COMMAND_TRANSPORT_BYTES: u64 = 64 * 1024 * 1024; + +pub const MAX_COMMAND_READ_BYTES: usize = 64 * 1024; + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn command_wire_rejects_legacy_output_host_paths() { + let command = serde_json::json!({ + "command": "printf bytes", "timeout_secs": 1, "cwd": "", "transport_byte_budget": 0 + }); + assert!(serde_json::from_value::(command.clone()).is_ok()); + for field in ["spill_dir", "output_limit"] { + let mut legacy = command.clone(); + legacy[field] = serde_json::json!("/host/private"); + assert!(serde_json::from_value::(legacy).is_err()); + } + let output = serde_json::json!({"chunks": [], "next_cursor": 0, "terminal": null, "output_path": "/host/private"}); + assert!(serde_json::from_value::(output).is_err()); + } + + #[test] + fn command_budget_wire_requires_field_and_checks_provider_hard_bound() { + let wire = serde_json::json!({"command": "true", "timeout_secs": 1, "cwd": ""}); + assert!(serde_json::from_value::(wire.clone()).is_err()); + for budget in [ + 0, + MAX_COMMAND_TRANSPORT_BYTES, + MAX_COMMAND_TRANSPORT_BYTES + 1, + ] { + let mut value = wire.clone(); + value["transport_byte_budget"] = serde_json::json!(budget); + let request: CommandRequest = serde_json::from_value(value).unwrap(); + assert_eq!( + request.validate().is_ok(), + budget <= MAX_COMMAND_TRANSPORT_BYTES + ); + } + let mut negative = wire; + negative["transport_byte_budget"] = serde_json::json!(-1); + assert!(serde_json::from_value::(negative).is_err()); + assert_eq!( + serde_json::to_value(CommandOutputTruncation::ByteBudget).unwrap(), + "byte_budget" + ); + let schema = serde_json::to_value(schemars::schema_for!(CommandRequest)).unwrap(); + assert_eq!( + schema["properties"]["transport_byte_budget"]["maximum"], + MAX_COMMAND_TRANSPORT_BYTES + ); + assert!( + schema["required"] + .as_array() + .unwrap() + .contains(&serde_json::json!("transport_byte_budget")) + ); + } } diff --git a/crates/workdir/src/router.rs b/crates/workdir/src/router.rs index 9eb190121..362504eab 100644 --- a/crates/workdir/src/router.rs +++ b/crates/workdir/src/router.rs @@ -341,7 +341,9 @@ impl WorkdirSession for RoutedWorkdirSession { let _active = self.state.enter()?; let handle = request.handle.clone(); let output = self.state.session.command_output(request).await?; - self.state.mark_terminal(&handle, output.status); + if let Some(terminal) = &output.terminal { + self.state.mark_terminal(&handle, terminal.status); + } Ok(output) } @@ -789,11 +791,12 @@ mod tests { .unwrap(); let handle = routed .start_command(CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: "printf done".to_string(), timeout_secs: 5, - output_limit: 1024, + cwd: crate::WorkdirPath::root(), - spill_dir: None, + tool_call_id: None, }) .await @@ -823,11 +826,12 @@ mod tests { let routed = router.attach(alias, Arc::clone(&underlying)).unwrap(); let handle = routed .start_command(CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: "sleep 30".to_string(), timeout_secs: 60, - output_limit: 1024, + cwd: crate::WorkdirPath::root(), - spill_dir: None, + tool_call_id: None, }) .await @@ -850,11 +854,12 @@ mod tests { let routed = router.attach(alias.clone(), session("wd", &dir)).unwrap(); let handle = routed .start_command(CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: "sleep 30".to_string(), timeout_secs: 60, - output_limit: 1024, + cwd: crate::WorkdirPath::root(), - spill_dir: None, + tool_call_id: None, }) .await diff --git a/crates/workdir/src/scope.rs b/crates/workdir/src/scope.rs index 6fd71cfd3..12b187230 100644 --- a/crates/workdir/src/scope.rs +++ b/crates/workdir/src/scope.rs @@ -10,6 +10,9 @@ //! provider-side before name enumeration, descent, or content reads. Returned coordinates are not //! prefixed with cwd again. +#[cfg(test)] +use crate::command_test_support::CollectCommand as _; + use std::collections::{BTreeMap, HashMap, HashSet}; use std::path::Path; use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; @@ -124,6 +127,7 @@ impl WorkdirToolBroker { next_lease_id: AtomicU64::new(1), close_lock: Arc::new(tokio::sync::Mutex::new(())), owned_commands: Arc::new(Mutex::new(HashSet::new())), + command_reads: Mutex::new(HashMap::new()), pending_command_events: Arc::new(Mutex::new(HashMap::new())), starting_tool_calls: Arc::new(Mutex::new(HashSet::new())), forwarded_starts: Arc::new(Mutex::new(HashSet::new())), @@ -230,25 +234,48 @@ impl WorkdirScopeLease { .source .cancel_command(handle.clone()) .await; - let terminal = self + let read_cursor = self .broker .authority - .source - .command_output(CommandOutputRequest { - handle, - cursor: 0, - limit: 1, - wait: true, - }) - .await; + .command_reads + .lock() + .expect("command cursor mutex poisoned") + .get(&command_id) + .cloned() + .expect("owned command cursor"); + let mut read_cursor = read_cursor.lock().await; + let mut cursor = *read_cursor; + let terminal = loop { + let output = self + .broker + .authority + .source + .command_output(CommandOutputRequest { + handle: handle.clone(), + cursor, + limit: crate::MAX_COMMAND_READ_BYTES, + wait: true, + }) + .await; + match output { + Ok(output) => { + cursor = output.next_cursor; + *read_cursor = cursor; + if let Some(terminal) = output.terminal { + break Ok(terminal); + } + } + Err(error) => break Err(error), + } + }; match (cancel, terminal) { (_, Ok(output)) => { self.broker.authority.publish_terminal_if_missing( &command_id, output.status, output.exit_code, - output.next_cursor.unwrap_or(output.content.len()) as u64, - &output.content, + cursor, + "", ); self.broker .authority @@ -582,7 +609,12 @@ struct ScopedWorkdirSession { scope_lock: tokio::sync::Mutex<()>, next_lease_id: AtomicU64, close_lock: Arc>, + // Live cleanup responsibility ends at terminal delivery. Read ownership is + // retained separately so a lost terminal response can still be replayed. owned_commands: Arc>>, + // Starting commands prunes completed entries at the bounded replay ceiling; + // current scope validity is still checked before every read or retry. + command_reads: Mutex>>>, pending_command_events: Arc>>>, starting_tool_calls: Arc>>, forwarded_starts: Arc>>, @@ -700,6 +732,11 @@ impl ScopedWorkdirSession { .lock() .expect("scoped command set mutex poisoned") .contains(&handle.0) + || self + .command_reads + .lock() + .expect("command cursor mutex poisoned") + .contains_key(&handle.0) { Ok(()) } else { @@ -1013,6 +1050,7 @@ impl ScopedWorkdirSession { next_lease_id: AtomicU64::new(1), close_lock: close_lock.clone(), owned_commands, + command_reads: Mutex::new(HashMap::new()), pending_command_events, starting_tool_calls, forwarded_starts, @@ -1407,6 +1445,16 @@ impl WorkdirSession for ScopedWorkdirSession { .lock() .expect("scoped command set mutex poisoned"); owned.insert(handle.0.clone()); + { + let mut reads = self + .command_reads + .lock() + .expect("command cursor mutex poisoned"); + if reads.len() >= 64 { + reads.retain(|id, _| owned.contains(id)); + } + reads.insert(handle.0.clone(), Arc::new(tokio::sync::Mutex::new(0))); + } if let Some(tool_call_id) = &tool_call_id { self.starting_tool_calls .lock() @@ -1457,14 +1505,28 @@ impl WorkdirSession for ScopedWorkdirSession { ) -> Result { self.ensure_owned_command(&request.handle)?; let command_id = request.handle.0.clone(); + let read_cursor = self + .command_reads + .lock() + .expect("command cursor mutex poisoned") + .get(&command_id) + .cloned(); + let mut read_cursor = match &read_cursor { + Some(cursor) => Some(cursor.lock().await), + None => None, + }; + self.ensure_owned_command(&request.handle)?; let output = self.source.command_output(request).await?; - if !matches!(output.status, CommandStatus::Running) { + if let Some(cursor) = &mut read_cursor { + **cursor = output.next_cursor; + } + if let Some(terminal) = &output.terminal { self.publish_terminal_if_missing( &command_id, - output.status, - output.exit_code, - output.next_cursor.unwrap_or(output.content.len()) as u64, - &output.content, + terminal.status, + terminal.exit_code, + output.next_cursor, + "", ); self.owned_commands .lock() @@ -2080,20 +2142,21 @@ mod tests { session: &WorkdirSessionHandle, command: impl Into, tool_call_id: impl Into, - ) -> CommandOutput { + ) -> crate::command_test_support::CollectedCommand { let handle = session .start_command(CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: command.into(), timeout_secs: 5, - output_limit: 1024, + cwd: FsPath::root(), - spill_dir: None, + tool_call_id: Some(tool_call_id.into()), }) .await .unwrap(); session - .command_output(CommandOutputRequest { + .collect_command_output(CommandOutputRequest { handle, cursor: 0, limit: 1024, @@ -2112,11 +2175,12 @@ mod tests { .expect("delegation wrapper must preserve command observation"); let handle = parent .start_command(CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: "printf ready; sleep 0.2; printf done".into(), timeout_secs: 5, - output_limit: 1024, + cwd: FsPath::root(), - spill_dir: None, + tool_call_id: Some("tool-delegated".into()), }) .await @@ -2139,7 +2203,7 @@ mod tests { assert_eq!(snapshots[0].stdout.content, "ready"); let output = parent - .command_output(CommandOutputRequest { + .collect_command_output(CommandOutputRequest { handle, cursor: 0, limit: 1024, @@ -2179,11 +2243,12 @@ mod tests { ); let error = child .start_command(CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: "pwd".into(), timeout_secs: 5, - output_limit: 1024, + cwd: FsPath::root(), - spill_dir: None, + tool_call_id: None, }) .await @@ -2211,11 +2276,12 @@ mod tests { let handle = first .start_command(CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: "pwd; sleep 0.2".into(), timeout_secs: 5, - output_limit: 4096, + cwd: FsPath::root(), - spill_dir: None, + tool_call_id: Some("first-command".into()), }) .await @@ -2239,7 +2305,7 @@ mod tests { )); let output = first - .command_output(CommandOutputRequest { + .collect_command_output(CommandOutputRequest { handle, cursor: 0, limit: 4096, @@ -2316,11 +2382,12 @@ mod tests { assert!(matches!( child .start_command(CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: "printf denied".into(), timeout_secs: 5, - output_limit: 1024, + cwd: FsPath::root(), - spill_dir: None, + tool_call_id: Some("read-only-command".into()), }) .await, @@ -2622,11 +2689,12 @@ mod tests { assert!(matches!( child .start_command(CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: "printf revoked".into(), timeout_secs: 5, - output_limit: 1024, + cwd: FsPath::root(), - spill_dir: None, + tool_call_id: Some("revoked-child-command".into()), }) .await, @@ -2724,11 +2792,12 @@ mod tests { for index in 0..MAX_SCOPED_COMMANDS { child .start_command(CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: "sleep 30".into(), timeout_secs: 60, - output_limit: 1024, + cwd: FsPath::root(), - spill_dir: None, + tool_call_id: Some(format!("command-{index}")), }) .await @@ -2737,11 +2806,12 @@ mod tests { let error = child .start_command(CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: "sleep 30".into(), timeout_secs: 60, - output_limit: 1024, + cwd: FsPath::root(), - spill_dir: None, + tool_call_id: Some("command-over-limit".into()), }) .await @@ -2771,11 +2841,12 @@ mod tests { let command = tokio::spawn(async move { command_child .start_command(CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: "sleep 30".into(), timeout_secs: 60, - output_limit: 1024, + cwd: FsPath::root(), - spill_dir: None, + tool_call_id: Some("racing-command".into()), }) .await @@ -2817,11 +2888,12 @@ mod tests { let mut events = child.subscribe_command_events().unwrap(); let handle = child .start_command(CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: "sleep 30; printf leaked > marker".into(), timeout_secs: 60, - output_limit: 1024, + cwd: FsPath::root(), - spill_dir: None, + tool_call_id: Some("owned-command".into()), }) .await @@ -3012,11 +3084,12 @@ mod tests { assert!(matches!( parent .start_command(CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, command: "printf closed".into(), timeout_secs: 5, - output_limit: 1024, + cwd: FsPath::root(), - spill_dir: None, + tool_call_id: Some("closed-parent-command".into()), }) .await, @@ -3028,4 +3101,180 @@ mod tests { "child result after parent close: {child_result:?}" ); } + #[tokio::test] + async fn scoped_byte_cursor_survives_partial_ack_then_revocation_and_terminal_retry() { + let root = TempDir::new().unwrap(); + fs::create_dir(root.path().join("child")).unwrap(); + let parent = session(root.path()); + let child = parent + .scope(request("child", WorkdirToolScopePermission::Write)) + .await + .unwrap(); + let tool = child.tool_session(); + let handle = tool + .start_command(CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, + command: "printf ready; exec yes x".into(), + timeout_secs: 10, + cwd: FsPath::root(), + tool_call_id: None, + }) + .await + .unwrap(); + let mut cursor = 0; + for _ in 0..2 { + let page = tool + .command_output(CommandOutputRequest { + handle: handle.clone(), + cursor, + limit: 1, + wait: true, + }) + .await + .unwrap(); + cursor = page.next_cursor; + } + tokio::time::timeout(std::time::Duration::from_secs(5), child.close()) + .await + .unwrap() + .unwrap(); + assert!( + tool.command_output(CommandOutputRequest { + handle, + cursor, + limit: 1, + wait: false + }) + .await + .is_err() + ); + let handle = parent + .start_command(CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, + command: "printf parent".into(), + timeout_secs: 5, + cwd: FsPath::root(), + tool_call_id: None, + }) + .await + .unwrap(); + let mut cursor = 0; + loop { + let request = CommandOutputRequest { + handle: handle.clone(), + cursor, + limit: 1024, + wait: true, + }; + let page = parent.command_output(request.clone()).await.unwrap(); + assert_eq!(parent.command_output(request).await.unwrap(), page); + cursor = page.next_cursor; + if page.terminal.is_some() { + break; + } + } + } + #[tokio::test] + async fn scoped_terminal_response_loss_replays_without_duplicate_capture_and_rejects_invalid_cursors() + { + tokio::time::timeout(std::time::Duration::from_secs(5), async { + let root = TempDir::new().unwrap(); + fs::create_dir(root.path().join("child")).unwrap(); + let parent = session(root.path()); + let child = parent + .scope(request("child", WorkdirToolScopePermission::Write)) + .await + .unwrap(); + let tool = child.tool_session(); + let handle = tool + .start_command(CommandRequest { + transport_byte_budget: crate::MAX_COMMAND_TRANSPORT_BYTES, + command: "printf abcdef".into(), + timeout_secs: 5, + cwd: FsPath::root(), + tool_call_id: None, + }) + .await + .unwrap(); + let mut cursor = 0; + let mut captured = Vec::new(); + let terminal_request = loop { + let request = CommandOutputRequest { + handle: handle.clone(), + cursor, + limit: 2, + wait: true, + }; + // Simulate losing each produced HTTP response, including terminal: + // append only the retried page, never both copies. + let lost = tool.command_output(request.clone()).await.unwrap(); + let replay = tool.command_output(request.clone()).await.unwrap(); + assert_eq!(replay, lost); + for chunk in &replay.chunks { + captured.extend_from_slice(&chunk.bytes); + } + cursor = replay.next_cursor; + if let Some(terminal) = replay.terminal { + assert_eq!(terminal.status, CommandStatus::Completed); + break request; + } + }; + assert_eq!(captured, b"abcdef"); + assert!( + !child + .broker + .authority + .owned_commands + .lock() + .unwrap() + .contains(&handle.0) + ); + assert!( + child + .broker + .authority + .command_reads + .lock() + .unwrap() + .contains_key(&handle.0) + ); + assert_eq!( + tool.command_status(handle.clone()).await.unwrap(), + CommandStatus::Completed + ); + for invalid in [0, cursor + 1] { + assert!( + tool.command_output(CommandOutputRequest { + handle: handle.clone(), + cursor: invalid, + limit: 2, + wait: false + }) + .await + .is_err() + ); + } + // Invalid cursor requests do not destroy the retained terminal page. + assert!( + tool.command_output(terminal_request) + .await + .unwrap() + .terminal + .is_some() + ); + child.close().await.unwrap(); + assert!( + tool.command_output(CommandOutputRequest { + handle, + cursor, + limit: 2, + wait: false + }) + .await + .is_err() + ); + }) + .await + .expect("bounded scoped terminal replay"); + } } diff --git a/crates/workdir/tests/command_denial_diagnostics.rs b/crates/workdir/tests/command_denial_diagnostics.rs index cfc4c5231..8db54686d 100644 --- a/crates/workdir/tests/command_denial_diagnostics.rs +++ b/crates/workdir/tests/command_denial_diagnostics.rs @@ -103,11 +103,12 @@ impl WorkdirSession for CommandFailureSession { } fn command() -> CommandRequest { CommandRequest { + transport_byte_budget: workdir::MAX_COMMAND_TRANSPORT_BYTES, command: "fake command, never executed".into(), timeout_secs: 1, - output_limit: 100, + cwd: WorkdirPath::root(), - spill_dir: None, + tool_call_id: None, } } diff --git a/crates/workdir/tests/denial_reasons.rs b/crates/workdir/tests/denial_reasons.rs index 53c64d5dd..41b031f7a 100644 --- a/crates/workdir/tests/denial_reasons.rs +++ b/crates/workdir/tests/denial_reasons.rs @@ -167,11 +167,12 @@ async fn read_only_and_scoped_capability_refusals_do_not_collapse() { assert_reason( child .start_command(workdir::CommandRequest { + transport_byte_budget: workdir::MAX_COMMAND_TRANSPORT_BYTES, command: "must-not-run".into(), timeout_secs: 1, - output_limit: 1, + cwd: WorkdirPath::root(), - spill_dir: None, + tool_call_id: None, }) .await diff --git a/crates/worker-runtime/Cargo.toml b/crates/worker-runtime/Cargo.toml index cbc0ad393..0eba6a589 100644 --- a/crates/worker-runtime/Cargo.toml +++ b/crates/worker-runtime/Cargo.toml @@ -56,6 +56,7 @@ server-api = { path = "../server-api" } workdir.workspace = true [dev-dependencies] +workdir = { workspace = true, features = ["http-client"] } futures.workspace = true agen.workspace = true serial_test = "3.4.0" diff --git a/crates/worker-runtime/src/http_server.rs b/crates/worker-runtime/src/http_server.rs index a916a5f69..57b7d0eb6 100644 --- a/crates/worker-runtime/src/http_server.rs +++ b/crates/worker-runtime/src/http_server.rs @@ -67,7 +67,7 @@ use std::path::PathBuf; use std::sync::{Arc, Mutex}; use tokio::net::TcpListener; use workdir::{ - CommandOutput, CommandStatus, WorkdirSessionHandle, dispatch_workdir_session_operation, + CommandOutput, WorkdirSessionHandle, dispatch_workdir_session_operation, http::{ OpenWorkdirSessionRequest, OpenWorkdirSessionResponse, WorkdirSessionId, WorkdirSessionOperation, WorkdirSessionOperationRequest, WorkdirSessionOperationResult, @@ -914,13 +914,9 @@ async fn run_workdir_session_operation( { Ok(result) => result?, Err(_) => CommandOutput { - exit_code: None, - timed_out: false, - status: CommandStatus::Running, - content: String::new(), - next_cursor: Some(cursor), - truncated: false, - output_path: None, + chunks: Vec::new(), + next_cursor: cursor, + terminal: None, }, }; WorkdirSessionOperationResult::CommandOutput(output) @@ -2426,6 +2422,7 @@ pub enum RuntimeHttpServerError { #[cfg(test)] mod tests { + mod command_transport_tests; #[cfg(feature = "fs-store")] mod session_material_tests; diff --git a/crates/worker-runtime/src/http_server/tests/command_transport_tests.rs b/crates/worker-runtime/src/http_server/tests/command_transport_tests.rs new file mode 100644 index 000000000..643e722ab --- /dev/null +++ b/crates/worker-runtime/src/http_server/tests/command_transport_tests.rs @@ -0,0 +1,408 @@ +//! Loopback integration: production RemoteWorkdirSession, signed HTTP middleware, +//! command operation handler, local process pipes, and JSON DTOs. Only opening the +//! pre-seeded session is a fixture; no Runtime Worker/materializer is claimed here. + +use super::*; +use std::time::Duration; +use workdir::http::{ + RemoteWorkdirSession, WorkdirHttpAuthorization, WorkdirHttpAuthorizationFuture, +}; +use workdir::{CommandOutputRequest, CommandRequest, CommandStream, WorkdirSession}; + +struct SignedAuthorization { + identity: RuntimeIdentityMaterial, + trust: WorkspaceIssuerTrustRecord, +} +impl std::fmt::Debug for SignedAuthorization { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("SignedAuthorization") + } +} +impl WorkdirHttpAuthorization for SignedAuthorization { + fn bearer_token( + &self, + method: String, + path: String, + body: Vec, + ) -> WorkdirHttpAuthorizationFuture { + let claims = signed_claims(&self.trust, &method, &path, &body); + let token = + issue_workspace_capability_token(&self.identity.signing_key().unwrap(), &claims) + .unwrap(); + Box::pin(async move { Ok(token) }) + } +} + +struct Harness { + server: tokio::task::JoinHandle<()>, + session: WorkdirSessionHandle, + _root: tempfile::TempDir, + remote: RemoteWorkdirSession, + client: reqwest::Client, + operation_url: url::Url, + authorization: Arc, +} +impl Drop for Harness { + fn drop(&mut self) { + self.server.abort(); + } +} +impl Harness { + async fn raw_operation(&self, operation: serde_json::Value) -> reqwest::Response { + let body = serde_json::to_vec(&serde_json::json!({"operation": operation})).unwrap(); + let token = self + .authorization + .bearer_token( + "POST".into(), + self.operation_url.path().into(), + body.clone(), + ) + .await + .unwrap(); + self.client + .post(self.operation_url.clone()) + .bearer_auth(token) + .header("content-type", "application/json") + .body(body) + .send() + .await + .unwrap() + } + + async fn new() -> Self { + let root = tempfile::tempdir().unwrap(); + let session: WorkdirSessionHandle = Arc::new(LocalWorkdirSession::materialized_bound( + Workdir::new("wd-transport"), + root.path().to_path_buf(), + root.path().to_path_buf(), + SharedScope::new(Scope::writable(root.path()).unwrap()), + WorkdirSessionCapabilities::ALL, + )); + let identity = RuntimeIdentityMaterial::generate("transport-key").unwrap(); + let trust = workspace_trust(&identity); + let state = RuntimeHttpState { + runtime: Runtime::new_memory(), + local_token: None, + workspace_auth: Some(Arc::new(workspace_test_auth(vec![trust.clone()]))), + workdir_sessions: Arc::new(Mutex::new(HashMap::from([( + "session-transport".into(), + RuntimeHttpWorkdirSession { + owner: RuntimeWorkspaceScope::new("workspace-a", "https://backend.test"), + session: session.clone(), + }, + )]))), + ssh_keyscan_program: Arc::new(PathBuf::from("ssh-keyscan")), + }; + let app = Router::new() + .route( + "/v1/working-directories/{id}/sessions", + post(|| async { + Json(OpenWorkdirSessionResponse { + session_id: WorkdirSessionId::new("session-transport").unwrap(), + workdir_id: Workdir::new("wd-transport").id().clone(), + capabilities: WorkdirSessionCapabilities::ALL, + }) + }), + ) + .route( + "/v1/workdir-sessions/{session_id}/operations", + post(run_workdir_session_operation), + ) + .route( + "/v1/workdir-sessions/{session_id}", + delete(close_workdir_session), + ) + .with_state(state.clone()) + .layer(middleware::from_fn_with_state(state, require_runtime_auth)); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let base: url::Url = format!("http://{}", listener.local_addr().unwrap()) + .parse() + .unwrap(); + let server = tokio::spawn(async move { + axum::serve(listener, app).await.unwrap(); + }); + let mut headers = reqwest::header::HeaderMap::new(); + headers.insert( + RUNTIME_WORKSPACE_SCOPE_HEADER, + "workspace-a".parse().unwrap(), + ); + let client = reqwest::Client::builder() + .no_proxy() + .default_headers(headers) + .build() + .unwrap(); + let authorization = Arc::new(SignedAuthorization { identity, trust }); + let operation_url = base + .join("/v1/workdir-sessions/session-transport/operations") + .unwrap(); + let remote = RemoteWorkdirSession::open_with_authorization( + client.clone(), + base, + authorization.clone(), + Workdir::new("wd-transport").id().clone(), + OpenWorkdirSessionRequest::default(), + ) + .await + .unwrap(); + Self { + server, + session, + _root: root, + remote, + client, + operation_url, + authorization, + } + } +} + +#[tokio::test] +async fn real_http_command_pages_preserve_binary_streams_replay_and_nonzero_exit() { + tokio::time::timeout(Duration::from_secs(20), async { + let harness = Harness::new().await; + let handle = harness.remote.start_command(CommandRequest { + transport_byte_budget: workdir::MAX_COMMAND_TRANSPORT_BYTES, + command: "head -c 2097152 /dev/zero | tr '\\0' x; printf '\\342\\202\\254\\377\\000'; printf 'stderr\\346\\227\\245' >&2; exit 9".into(), + timeout_secs: 15, cwd: WorkdirPath::root(), tool_call_id: Some("http-tool-call".into()), + }).await.unwrap(); + let mut cursor = 0; + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + let terminal = loop { + let request = CommandOutputRequest { handle: handle.clone(), cursor, limit: 16384, wait: true }; + let page = harness.remote.command_output(request.clone()).await.unwrap(); + assert_eq!(harness.remote.command_output(request).await.unwrap(), page); + assert!(page.chunks.iter().map(|chunk| chunk.bytes.len()).sum::() <= 16384); + for chunk in page.chunks { + assert_eq!(chunk.sequence, cursor); + cursor += chunk.bytes.len() as u64; + let target = match chunk.stream { CommandStream::Stdout => &mut stdout, CommandStream::Stderr => &mut stderr }; + assert_eq!(chunk.offset, target.len() as u64); + target.extend(chunk.bytes); + } + assert_eq!(cursor, page.next_cursor); + if let Some(terminal) = page.terminal { break terminal; } + }; + let mut expected = vec![b'x'; 2097152]; + expected.extend_from_slice(b"\xe2\x82\xac\xff\0"); + assert_eq!(stdout, expected); + assert_eq!(stderr, b"stderr\xe6\x97\xa5"); + assert_eq!(terminal.status, workdir::CommandStatus::Failed); + assert_eq!(terminal.exit_code, Some(9)); + assert!(harness.remote.command_output(CommandOutputRequest { handle, cursor: 0, limit: 1, wait: false }).await.is_err()); + harness.remote.close().await.unwrap(); + assert!(harness.session.command_snapshot().is_empty()); + }).await.expect("bounded HTTP command transport"); +} + +#[tokio::test] +async fn real_http_cancel_drains_tail_before_terminal_and_timeout_is_distinct() { + tokio::time::timeout(Duration::from_secs(15), async { + let harness = Harness::new().await; + for (command, timeout, cancel, expected) in [ + ( + "printf ready; exec yes x", + 10, + true, + workdir::CommandStatus::Cancelled, + ), + ( + "printf ready; exec tail -f /dev/null", + 1, + false, + workdir::CommandStatus::TimedOut, + ), + ] { + let handle = harness + .remote + .start_command(CommandRequest { + transport_byte_budget: workdir::MAX_COMMAND_TRANSPORT_BYTES, + command: command.into(), + timeout_secs: timeout, + cwd: WorkdirPath::root(), + tool_call_id: None, + }) + .await + .unwrap(); + let mut cursor = 0; + let first = harness + .remote + .command_output(CommandOutputRequest { + handle: handle.clone(), + cursor, + limit: 5, + wait: true, + }) + .await + .unwrap(); + assert_eq!( + first + .chunks + .iter() + .flat_map(|chunk| chunk.bytes.iter().copied()) + .collect::>(), + b"ready" + ); + cursor = first.next_cursor; + if cancel { + harness.remote.cancel_command(handle.clone()).await.unwrap(); + } + loop { + let page = harness + .remote + .command_output(CommandOutputRequest { + handle: handle.clone(), + cursor, + limit: workdir::MAX_COMMAND_READ_BYTES, + wait: true, + }) + .await + .unwrap(); + cursor = page.next_cursor; + if let Some(terminal) = page.terminal { + assert_eq!(terminal.status, expected); + break; + } + } + } + harness.remote.close().await.unwrap(); + }) + .await + .expect("bounded cancellation and timeout over HTTP"); +} + +#[tokio::test] +async fn real_http_command_budget_bounds_unique_payload_and_reports_larger_seen_totals() { + tokio::time::timeout(Duration::from_secs(20), async { + let harness = Harness::new().await; + for budget in [0, 12345] { + let handle = harness + .remote + .start_command(CommandRequest { + command: "head -c 1048576 /dev/zero; head -c 65536 /dev/zero >&2; exit 9" + .into(), + timeout_secs: 10, + transport_byte_budget: budget, + cwd: WorkdirPath::root(), + tool_call_id: None, + }) + .await + .unwrap(); + let mut cursor = 0; + let mut delivered = [0_u64; 2]; + let terminal = loop { + let request = CommandOutputRequest { + handle: handle.clone(), + cursor, + limit: 997, + wait: true, + }; + let page = harness + .remote + .command_output(request.clone()) + .await + .unwrap(); + assert_eq!(harness.remote.command_output(request).await.unwrap(), page); + for chunk in page.chunks { + assert_eq!(chunk.sequence, cursor); + cursor += chunk.bytes.len() as u64; + let index = match chunk.stream { + CommandStream::Stdout => 0, + CommandStream::Stderr => 1, + }; + assert_eq!(chunk.offset, delivered[index]); + delivered[index] += chunk.bytes.len() as u64; + } + assert_eq!(cursor, page.next_cursor); + assert!( + cursor <= budget, + "RemoteWorkdirSession must not receive bytes beyond budget" + ); + if let Some(terminal) = page.terminal { + break terminal; + } + }; + assert_eq!(cursor, budget); + assert_eq!(terminal.stdout_bytes, 1048576); + assert_eq!(terminal.stderr_bytes, 65536); + assert_eq!(terminal.stdout_delivered_bytes, delivered[0]); + assert_eq!(terminal.stderr_delivered_bytes, delivered[1]); + assert_eq!( + terminal.truncation_reason, + Some(workdir::CommandOutputTruncation::ByteBudget) + ); + assert_eq!(terminal.status, workdir::CommandStatus::Failed); + assert_eq!(terminal.exit_code, Some(9)); + assert!( + harness + .remote + .command_output(CommandOutputRequest { + handle: handle.clone(), + cursor: budget + 1, + limit: 1, + wait: false, + }) + .await + .is_err() + ); + if budget > 0 { + assert!( + harness + .remote + .command_output(CommandOutputRequest { + handle, + cursor: 0, + limit: 1, + wait: false, + }) + .await + .is_err() + ); + } + } + harness.remote.close().await.unwrap(); + }) + .await + .expect("bounded HTTP byte-budget transport"); +} + +#[tokio::test] +async fn real_http_rejects_invalid_or_legacy_budget_requests_before_process_start() { + tokio::time::timeout(Duration::from_secs(10), async { + let harness = Harness::new().await; + let request = CommandRequest { + command: "touch must-not-exist".into(), + timeout_secs: 1, + transport_byte_budget: workdir::MAX_COMMAND_TRANSPORT_BYTES + 1, + cwd: WorkdirPath::root(), + tool_call_id: None, + }; + let oversize = + serde_json::to_value(WorkdirSessionOperation::CommandStart(request)).unwrap(); + assert_eq!( + harness.raw_operation(oversize.clone()).await.status(), + reqwest::StatusCode::BAD_REQUEST + ); + let mut missing = oversize.clone(); + // CommandStart is externally tagged by operation / request. + let object = missing.as_object_mut().unwrap(); + let request = object.get_mut("request").unwrap().as_object_mut().unwrap(); + request.remove("transport_byte_budget"); + assert_eq!( + harness.raw_operation(missing).await.status(), + reqwest::StatusCode::BAD_REQUEST + ); + let mut legacy = oversize; + let request = legacy["request"].as_object_mut().unwrap(); + request.insert("transport_byte_budget".into(), serde_json::json!(0)); + request.insert("spill_dir".into(), serde_json::json!("/private/host")); + assert_eq!( + harness.raw_operation(legacy).await.status(), + reqwest::StatusCode::BAD_REQUEST + ); + assert!(!harness._root.path().join("must-not-exist").exists()); + harness.remote.close().await.unwrap(); + }) + .await + .expect("bounded HTTP budget admission"); +} diff --git a/crates/worker-runtime/src/worker_backend.rs b/crates/worker-runtime/src/worker_backend.rs index 74044cde9..f431f3dad 100644 --- a/crates/worker-runtime/src/worker_backend.rs +++ b/crates/worker-runtime/src/worker_backend.rs @@ -147,7 +147,6 @@ use worker::{ Worker, WorkerBootstrap, WorkerBootstrapError, WorkerBootstrapLayout, WorkerControllerTransport, WorkerError, WorkerFilesystemAuthority, WorkerHandle, WorkerSharedState, WorkerWorkspaceContext, WorkspaceClient, WorkspaceId, - bash_output_dir_for_worker_id, }; #[cfg(feature = "ws-server")] @@ -1090,29 +1089,24 @@ fn runtime_local_workdir_session( root: &Path, cwd: &Path, scope: manifest::SharedScope, - command_output_scope: manifest::SharedScope, capabilities: WorkdirSessionCapabilities, command_environment: std::collections::BTreeMap, resources: Vec>, ) -> WorkdirSessionHandle { - Arc::new( - LocalWorkdirSession::materialized_bound_with_environment_and_command_output_scope( - Workdir::new(workdir_id), - root.to_path_buf(), - cwd.to_path_buf(), - scope, - command_output_scope, - capabilities, - command_environment, - resources, - ), - ) + Arc::new(LocalWorkdirSession::materialized_bound_with_environment( + Workdir::new(workdir_id), + root.to_path_buf(), + cwd.to_path_buf(), + scope, + capabilities, + command_environment, + resources, + )) } fn runtime_local_workdir_router( attachments: &BTreeMap, capabilities: &BTreeMap, - command_output_scope: manifest::SharedScope, ) -> Result, String> { let router = Arc::new(WorkdirSessionRouter::new()); for (alias, binding) in attachments { @@ -1129,7 +1123,6 @@ fn runtime_local_workdir_router( binding.root(), binding.cwd(), scope, - command_output_scope.clone(), capabilities, binding.command_environment(), binding.session_resources().map_err(|error| { @@ -1146,7 +1139,6 @@ fn restored_workdir_router( local_attachments: &BTreeMap, logical_attachments: &[crate::catalog::LogicalWorkdirAttachment], claims: &[crate::catalog::WorkingDirectoryAttachmentClaim], - scope: manifest::SharedScope, workspace_client: Arc, ) -> Result, String> { let mut local_capabilities = local_attachments @@ -1163,7 +1155,7 @@ fn restored_workdir_router( local_capabilities.insert(attachment.alias.clone(), attachment.capabilities); } } - let router = runtime_local_workdir_router(local_attachments, &local_capabilities, scope)?; + let router = runtime_local_workdir_router(local_attachments, &local_capabilities)?; for attachment in logical_attachments { if let Some(local) = local_attachments.get(&attachment.alias) { if local.working_directory.id != attachment.working_directory_id { @@ -1517,7 +1509,6 @@ impl RuntimeWorkerFactory for ProfileRuntimeWorkerFactory { let store = CombinedStore::new(session_store, worker_metadata_store); let run_dir = self.worker_run_dir(&request.worker_ref)?; - let bash_output_dir = bash_output_dir_for_worker_id(&request.worker_ref.worker_id); let bootstrap = WorkerBootstrap::new( manifest, store, @@ -1526,7 +1517,6 @@ impl RuntimeWorkerFactory for ProfileRuntimeWorkerFactory { filesystem_authority, WorkerBootstrapLayout::RuntimeManagedRun { run_dir: run_dir.clone(), - bash_output_dir, }, self.controller_transport, ); @@ -1571,7 +1561,6 @@ impl RuntimeWorkerFactory for ProfileRuntimeWorkerFactory { worker.bind_workdir_sessions(runtime_local_workdir_router( &request.workdir_attachments, &workdir_capabilities, - worker.scope().clone(), )?); if let (Some(runtime_id), Some(workspace_id)) = (observation_runtime_id, observation_workspace_id.clone()) @@ -1838,7 +1827,6 @@ impl RuntimeWorkerFactory for ProfileRuntimeWorkerFactory { &request.workdir_attachments, &request.logical_workdir_attachments, &request.request.workdir_attachments, - worker.scope().clone(), worker.workspace_client_handle(), )?; worker.bind_workdir_sessions(workdir_sessions); @@ -1875,13 +1863,11 @@ impl RuntimeWorkerFactory for ProfileRuntimeWorkerFactory { let workspace_client = worker.workspace_client_handle(); let run_dir = self.worker_restore_run_dir(&request.worker_ref, request.operation_id)?; - let bash_output_dir = bash_output_dir_for_worker_id(&request.worker_ref.worker_id); let cleanup_sessions = worker.workdir_sessions(); let started = PreparedWorker::new( worker, WorkerBootstrapLayout::RuntimeManagedRun { run_dir: run_dir.clone(), - bash_output_dir, }, self.controller_transport, ) @@ -3755,7 +3741,6 @@ where working_directory_id, binding.root(), binding.cwd(), - scope.clone(), scope, WorkdirSessionCapabilities::ALL, binding.command_environment(), @@ -6804,8 +6789,6 @@ mod tests { #[test] fn restored_router_uses_workspace_proxy_for_logical_only_attachment() { - let root = tempfile::tempdir().unwrap(); - let scope = manifest::SharedScope::new(manifest::Scope::writable(root.path()).unwrap()); let client = WorkerWorkspaceContext::local_filesystem(Some( WorkspaceId::new("workspace-a").unwrap(), )) @@ -6820,7 +6803,6 @@ mod tests { capabilities: workdir::WorkdirSessionCapabilities::READ_ONLY, }], &[], - scope, client, ) .unwrap(); @@ -6865,7 +6847,6 @@ mod tests { .unwrap(); let alias = WorkdirAttachmentAlias::new("docs").unwrap(); let workdir_id = binding.working_directory.id.clone(); - let command_output_scope = manifest::SharedScope::new(manifest::Scope::empty()); let attachments = BTreeMap::from([(alias.clone(), binding)]); let logical = [LogicalWorkdirAttachment { alias: alias.clone(), @@ -6877,9 +6858,7 @@ mod tests { )) .client_handle(); - let router = - restored_workdir_router(&attachments, &logical, &[], command_output_scope, client) - .unwrap(); + let router = restored_workdir_router(&attachments, &logical, &[], client).unwrap(); let session = router.session(&alias).unwrap(); assert_eq!( @@ -7566,14 +7545,10 @@ mod tests { ) .await .map_err(|err| err.to_string())?; - let bash_output_dir = self.runtime_base.join("bash-output"); - let (handle, shutdown_rx) = WorkerController::spawn_runtime_managed( - worker, - &self.runtime_base, - &bash_output_dir, - ) - .await - .map_err(|err| err.to_string())?; + let (handle, shutdown_rx) = + WorkerController::spawn_runtime_managed(worker, &self.runtime_base) + .await + .map_err(|err| err.to_string())?; Ok(RuntimeWorkerController { handle, shutdown: Arc::new(tokio::sync::Mutex::new(Some(shutdown_rx))), @@ -8697,7 +8672,6 @@ mod tests { "working-directory-42", root.path(), root.path(), - spawned_scope.clone(), spawned_scope, WorkdirSessionCapabilities::READ_ONLY, Default::default(), @@ -8708,7 +8682,6 @@ mod tests { "working-directory-42", root.path(), root.path(), - restored_scope.clone(), restored_scope, WorkdirSessionCapabilities::READ_ONLY, Default::default(), @@ -10233,8 +10206,7 @@ mod tests { let alias = WorkdirAttachmentAlias::new("checkout").unwrap(); let attachments = BTreeMap::from([(alias, binding.clone())]); - let scope = manifest::SharedScope::new(manifest::Scope::writable(binding.root()).unwrap()); - let router = runtime_local_workdir_router(&attachments, &BTreeMap::new(), scope).unwrap(); + let router = runtime_local_workdir_router(&attachments, &BTreeMap::new()).unwrap(); session_clone.close().await.unwrap(); assert_eq!( materializer.cleanup_working_directory(id).unwrap_err().code, diff --git a/crates/worker/examples/worker_protocol.rs b/crates/worker/examples/worker_protocol.rs index 53ec846d7..b893515da 100644 --- a/crates/worker/examples/worker_protocol.rs +++ b/crates/worker/examples/worker_protocol.rs @@ -47,9 +47,7 @@ async fn main() -> Result<(), Box> { let worker = worker::Worker::from_manifest_toml(&toml, store).await?; let runtime_tmp = tempfile::tempdir()?; - let bash_output_dir = runtime_tmp.path().join("bash-output"); - let (handle, _shutdown_rx) = - WorkerController::spawn(worker, runtime_tmp.path(), &bash_output_dir).await?; + let (handle, _shutdown_rx) = WorkerController::spawn(worker, runtime_tmp.path()).await?; // Check initial status via shared state println!("[shared_state] {}", handle.shared_state.status_json()); diff --git a/crates/worker/src/bootstrap.rs b/crates/worker/src/bootstrap.rs index d027aa8c9..aa8943b6d 100644 --- a/crates/worker/src/bootstrap.rs +++ b/crates/worker/src/bootstrap.rs @@ -17,28 +17,9 @@ use manifest::WorkerManifest; #[derive(Debug, Clone)] pub enum WorkerBootstrapLayout { /// A direct Worker rooted below the supplied runtime base directory. - Direct { - runtime_base: PathBuf, - bash_output_dir: PathBuf, - }, + Direct { runtime_base: PathBuf }, /// A runtime-managed Worker with an exact persisted run directory. - RuntimeManagedRun { - run_dir: PathBuf, - bash_output_dir: PathBuf, - }, -} - -/// Return the temporary Bash spill directory owned by a stable Worker identity. -/// -/// The directory deliberately lives outside session/run-generation storage so a -/// restarted controller for the same Worker keeps the same readable artifact -/// boundary. -pub fn bash_output_dir_for_worker_id(worker_id: impl std::fmt::Display) -> PathBuf { - std::env::temp_dir() - .join("yoi") - .join("workers") - .join(worker_id.to_string()) - .join("bash-output") + RuntimeManagedRun { run_dir: PathBuf }, } /// Construction and controller inputs that are stable for one Worker launch. @@ -272,27 +253,12 @@ where { let cleanup_sessions = worker.workdir_sessions(); let controller = match layout { - WorkerBootstrapLayout::Direct { - runtime_base, - bash_output_dir, - } => { - WorkerController::spawn_with_transport_owned( - worker, - &runtime_base, - &bash_output_dir, - transport, - ) - .await + WorkerBootstrapLayout::Direct { runtime_base } => { + WorkerController::spawn_with_transport_owned(worker, &runtime_base, transport).await } - WorkerBootstrapLayout::RuntimeManagedRun { - run_dir, - bash_output_dir, - } => { + WorkerBootstrapLayout::RuntimeManagedRun { run_dir } => { WorkerController::spawn_runtime_managed_run_with_transport_owned( - worker, - &run_dir, - &bash_output_dir, - transport, + worker, &run_dir, transport, ) .await } @@ -313,22 +279,3 @@ where } } } - -#[cfg(test)] -mod tests { - use super::bash_output_dir_for_worker_id; - - #[test] - fn bash_output_directory_is_stable_per_worker_below_system_temp() { - let path = bash_output_dir_for_worker_id("019c1234-worker"); - - assert_eq!( - path, - std::env::temp_dir() - .join("yoi") - .join("workers") - .join("019c1234-worker") - .join("bash-output") - ); - } -} diff --git a/crates/worker/src/checkout_wip_tests.rs b/crates/worker/src/checkout_wip_tests.rs index c4ca3698f..f9bb7cc95 100644 --- a/crates/worker/src/checkout_wip_tests.rs +++ b/crates/worker/src/checkout_wip_tests.rs @@ -446,7 +446,11 @@ async fn checkout_wip_policy_is_not_feature_authority_and_compatibility_is_repla let mut registry = WipMountRegistry::new(); crate::checkout::mount_checkouts(&mut registry, router.clone(), tools::Tracker::new(), None) .unwrap(); - for def in tools::routed_builtin_tools(router, tools::Tracker::new(), dir.path().join("bash")) { + for def in tools::routed_builtin_tools( + router, + tools::Tracker::new(), + tools::bash_output::BashOutputView::new(), + ) { let (meta, tool) = def(); let p = compatibility_projection(meta, tool, None).unwrap(); registry.mount(p).unwrap(); diff --git a/crates/worker/src/controller.rs b/crates/worker/src/controller.rs index d8be6da1a..9f233f95f 100644 --- a/crates/worker/src/controller.rs +++ b/crates/worker/src/controller.rs @@ -692,7 +692,6 @@ impl WorkerController { pub async fn spawn( worker: Worker, runtime_base: &Path, - bash_output_dir: &Path, ) -> Result<(WorkerHandle, ShutdownReceiver), std::io::Error> where C: LlmClient + Clone + 'static, @@ -701,7 +700,6 @@ impl WorkerController { let (handle, shutdown, _task) = Self::spawn_inner( worker, runtime_base, - bash_output_dir, false, None, WorkerControllerTransport::UnixSocket, @@ -715,7 +713,7 @@ impl WorkerController { pub async fn spawn_with_transport( worker: Worker, runtime_base: &Path, - bash_output_dir: &Path, + transport: WorkerControllerTransport, ) -> Result<(WorkerHandle, ShutdownReceiver), std::io::Error> where @@ -723,42 +721,32 @@ impl WorkerController { St: Store + WorkerMetadataStore + Clone + Send + Sync + 'static, { let (handle, shutdown, _task) = - Self::spawn_with_transport_owned(worker, runtime_base, bash_output_dir, transport) - .await?; + Self::spawn_with_transport_owned(worker, runtime_base, transport).await?; Ok((handle, shutdown)) } pub(crate) async fn spawn_with_transport_owned( worker: Worker, runtime_base: &Path, - bash_output_dir: &Path, + transport: WorkerControllerTransport, ) -> Result<(WorkerHandle, ShutdownReceiver, tokio::task::JoinHandle<()>), std::io::Error> where C: LlmClient + Clone + 'static, St: Store + WorkerMetadataStore + Clone + Send + Sync + 'static, { - Self::spawn_inner( - worker, - runtime_base, - bash_output_dir, - false, - None, - transport, - ) - .await + Self::spawn_inner(worker, runtime_base, false, None, transport).await } /// Spawn a Worker owned by `worker-runtime`. /// - /// The controller uses an ephemeral directory for Unix sockets while tool - /// spill artifacts use the separately supplied Worker-owned temporary path. + /// The controller uses an ephemeral directory for Unix sockets. Bash output is + /// owned by its live Feature instance, independently of filesystem layout. /// Runtime-managed Workers do not write legacy pid/status/manifest liveness /// projections. pub async fn spawn_runtime_managed( worker: Worker, runtime_base: &Path, - bash_output_dir: &Path, ) -> Result<(WorkerHandle, ShutdownReceiver), std::io::Error> where C: LlmClient + Clone + 'static, @@ -767,7 +755,6 @@ impl WorkerController { let (handle, shutdown, _task) = Self::spawn_inner( worker, runtime_base, - bash_output_dir, true, None, WorkerControllerTransport::UnixSocket, @@ -780,7 +767,6 @@ impl WorkerController { pub async fn spawn_runtime_managed_run( worker: Worker, run_dir: &Path, - bash_output_dir: &Path, ) -> Result<(WorkerHandle, ShutdownReceiver), std::io::Error> where C: LlmClient + Clone + 'static, @@ -789,7 +775,6 @@ impl WorkerController { Self::spawn_runtime_managed_run_with_transport( worker, run_dir, - bash_output_dir, WorkerControllerTransport::UnixSocket, ) .await @@ -800,27 +785,23 @@ impl WorkerController { pub async fn spawn_runtime_managed_run_with_transport( worker: Worker, run_dir: &Path, - bash_output_dir: &Path, + transport: WorkerControllerTransport, ) -> Result<(WorkerHandle, ShutdownReceiver), std::io::Error> where C: LlmClient + Clone + 'static, St: Store + WorkerMetadataStore + Clone + Send + Sync + 'static, { - let (handle, shutdown, _task) = Self::spawn_runtime_managed_run_with_transport_owned( - worker, - run_dir, - bash_output_dir, - transport, - ) - .await?; + let (handle, shutdown, _task) = + Self::spawn_runtime_managed_run_with_transport_owned(worker, run_dir, transport) + .await?; Ok((handle, shutdown)) } pub(crate) async fn spawn_runtime_managed_run_with_transport_owned( worker: Worker, run_dir: &Path, - bash_output_dir: &Path, + transport: WorkerControllerTransport, ) -> Result<(WorkerHandle, ShutdownReceiver, tokio::task::JoinHandle<()>), std::io::Error> where @@ -830,21 +811,13 @@ impl WorkerController { let parent = run_dir .parent() .ok_or_else(|| std::io::Error::other("run path has no parent"))?; - Self::spawn_inner( - worker, - parent, - bash_output_dir, - true, - Some(run_dir), - transport, - ) - .await + Self::spawn_inner(worker, parent, true, Some(run_dir), transport).await } async fn spawn_inner( worker: Worker, runtime_base: &Path, - bash_output_dir: &Path, + runtime_managed: bool, runtime_run: Option<&Path>, transport: WorkerControllerTransport, @@ -857,7 +830,6 @@ impl WorkerController { let result = Self::spawn_initialized( worker, runtime_base, - bash_output_dir, runtime_managed, runtime_run, transport, @@ -874,7 +846,7 @@ impl WorkerController { async fn spawn_initialized( mut worker: Worker, runtime_base: &Path, - bash_output_dir: &Path, + runtime_managed: bool, runtime_run: Option<&Path>, transport: WorkerControllerTransport, @@ -888,8 +860,7 @@ impl WorkerController { .map_err(std::io::Error::other)?; // === 1. Initialization (channels / RuntimeDir / worker-immutable - // snapshots / SpawnedWorkerRegistry / alerter attach / - // bash-output scope) === + // snapshots / SpawnedWorkerRegistry / alerter attach) === let (shutdown_tx, shutdown_rx) = oneshot::channel::<()>(); let (method_tx, method_rx) = mpsc::channel::(32); let (working_event_tx, _) = broadcast::channel::(256); @@ -897,8 +868,8 @@ impl WorkerController { let in_flight = InFlightEvents::new(working_event_tx.clone()); worker.attach_in_flight_events(in_flight.clone()); - // Runtime directory is created before tool registration because it owns - // bounded tool artifacts, and before initial status/history writes consume + // Runtime directory is created before tool registration and before + // initial status/history writes consume // the greeting we build after registration is complete. let runtime_dir = Arc::new(if let Some(run_dir) = runtime_run { RuntimeDir::create_worker_run(run_dir).await? @@ -935,26 +906,6 @@ impl WorkerController { worker.attach_internal_worker_registry(spawned_registry.clone()); worker.attach_working_event_tx(working_event_tx.clone()); - // Bash spill artifacts are owned by the stable Worker identity rather - // than a controller session/run generation. Push a recursive - // `allow(Read)` for the exact tool output path into the Worker's shared - // runtime scope so the Workdir session and system prompt stay aligned. - let bash_output_dir = bash_output_dir.to_path_buf(); - std::fs::create_dir_all(&bash_output_dir).map_err(|e| { - std::io::Error::other(format!( - "create bash output dir {}: {e}", - bash_output_dir.display() - )) - })?; - worker - .add_scope_rules([manifest::ScopeRule { - target: bash_output_dir.clone(), - permission: manifest::Permission::Read, - recursive: true, - symlink_policy: Default::default(), - }]) - .map_err(std::io::Error::other)?; - // === 1.5. Direct writer wiring === // // Engine callbacks fire `on_history_append` for each assistant @@ -974,7 +925,6 @@ impl WorkerController { // === 3. Tool registration (builtin / memory / spawn-orchestration) === let fs_for_view = register_worker_tools( &mut worker, - bash_output_dir, runtime_base.to_path_buf(), spawned_registry.clone(), Some(method_tx.downgrade()), @@ -1440,7 +1390,7 @@ fn validate_memory_lifecycle_targets( /// the shared state. pub(crate) async fn register_worker_tools( worker: &mut Worker, - bash_output_dir: PathBuf, + runtime_base: PathBuf, spawned_registry: Arc, parent_method_tx: Option>, @@ -1485,6 +1435,7 @@ where let lifecycle_enabled = worker.manifest_lifecycle_features_enabled() && backend_job.is_none(); let prompts = worker.prompts().clone(); let tracker = tools::Tracker::new(); + let bash_output = spawned_registry.bash_output(); { let paste_store = worker.store().clone(); let paste_session_id = worker.session_id(); @@ -1507,13 +1458,13 @@ where tools::routed_builtin_tools_for_current_capabilities( workdir_sessions.clone(), tracker.clone(), - bash_output_dir.clone(), + bash_output.clone(), ) } else { tools::routed_builtin_tools( workdir_sessions.clone(), tracker.clone(), - bash_output_dir.clone(), + bash_output.clone(), ) }; worker.engine_mut().register_tools(workdir_tools); @@ -2040,7 +1991,6 @@ where spawner_workspace_context, parent_notifications, runtime_base.clone(), - bash_output_dir.clone(), spawner_workspace_root, workdir_tool_broker, spawned_registry.clone(), @@ -3326,6 +3276,8 @@ async fn controller_loop( } } + spawned_registry.bash_output().revoke(); + if let Some(observer) = command_observer.take() { observer.abort(); let _ = observer.await; @@ -4212,7 +4164,6 @@ mod tests { .descriptor(); register_worker_tools( &mut worker, - root.path().join("bash-output"), root.path().join("runtime"), SpawnedWorkerRegistry::new_for_internal_services(), None, diff --git a/crates/worker/src/controller_subject_release_tests.rs b/crates/worker/src/controller_subject_release_tests.rs index 3cda546fe..d57dc4530 100644 --- a/crates/worker/src/controller_subject_release_tests.rs +++ b/crates/worker/src/controller_subject_release_tests.rs @@ -64,7 +64,6 @@ async fn prepared( WorkerFilesystemAuthority::None, WorkerBootstrapLayout::Direct { runtime_base: temp.path().join("runtime"), - bash_output_dir: temp.path().join("bash"), }, WorkerControllerTransport::InProcess, ) diff --git a/crates/worker/src/entrypoint.rs b/crates/worker/src/entrypoint.rs index 88745da61..15f15407b 100644 --- a/crates/worker/src/entrypoint.rs +++ b/crates/worker/src/entrypoint.rs @@ -635,12 +635,11 @@ async fn run_cli_inner(cli: Cli) -> ExitCode { return ExitCode::FAILURE; } }; - let bash_output_dir = crate::bash_output_dir_for_worker_id(&worker_name); + let started = match start_worker_controller( worker, WorkerBootstrapLayout::Direct { runtime_base: runtime_base.clone(), - bash_output_dir, }, WorkerControllerTransport::UnixSocket, ) diff --git a/crates/worker/src/feature/builtin/manage_workdir.rs b/crates/worker/src/feature/builtin/manage_workdir.rs index 8cd5477be..ad35374e2 100644 --- a/crates/worker/src/feature/builtin/manage_workdir.rs +++ b/crates/worker/src/feature/builtin/manage_workdir.rs @@ -505,13 +505,7 @@ impl WorkdirSession for WorkspaceAttachedWorkdirSession { } } - async fn start_command( - &self, - mut request: CommandRequest, - ) -> Result { - // `spill_dir` belongs to the Worker host running BashTool. This remote - // WorkdirSession provider cannot safely resolve or write that host path. - request.spill_dir = None; + async fn start_command(&self, request: CommandRequest) -> Result { match self.operate(WorkdirSessionOperation::CommandStart(request))? { WorkdirSessionOperationResult::CommandStart(result) => Ok(result), _ => Err(Self::mismatch("command_start")), @@ -1660,9 +1654,9 @@ mod tests { .start_command(CommandRequest { command: "true".to_string(), timeout_secs: 120, - output_limit: 1024, + + transport_byte_budget: workdir::MAX_COMMAND_TRANSPORT_BYTES, cwd: workdir::WorkdirPath::root(), - spill_dir: Some("/worker-local/bash-output".into()), tool_call_id: Some("call-1".to_string()), }) .await @@ -1675,7 +1669,7 @@ mod tests { assert_eq!(body["operation"]["operation"], "command_start"); assert_eq!(body["operation"]["request"]["command"], "true"); assert_eq!(body["operation"]["request"]["cwd"], ""); - assert!(body["operation"]["request"]["spill_dir"].is_null()); + assert!(body["operation"]["request"].get("spill_dir").is_none()); assert_eq!(body["operation"]["request"]["tool_call_id"], "call-1"); assert!( !requests[0] @@ -1942,9 +1936,9 @@ mod tests { .start_command(CommandRequest { command: "printf done".to_string(), timeout_secs: 5, - output_limit: 1024, + + transport_byte_budget: workdir::MAX_COMMAND_TRANSPORT_BYTES, cwd: workdir::WorkdirPath::root(), - spill_dir: None, tool_call_id: None, }) .await diff --git a/crates/worker/src/lib.rs b/crates/worker/src/lib.rs index 10cf5a6c5..fac6cd82b 100644 --- a/crates/worker/src/lib.rs +++ b/crates/worker/src/lib.rs @@ -39,7 +39,7 @@ mod worker; pub use bootstrap::{ BootstrappedWorker, PreparedWorker, WorkerBootstrap, WorkerBootstrapError, - WorkerBootstrapLayout, bash_output_dir_for_worker_id, start_worker_controller, + WorkerBootstrapLayout, start_worker_controller, }; pub use compact::token_counter::{EstimateSource, SplitPoint, TokenEstimate}; pub use controller::{ShutdownReceiver, WorkerController, WorkerControllerTransport, WorkerHandle}; diff --git a/crates/worker/src/spawn/registry.rs b/crates/worker/src/spawn/registry.rs index 05317f491..443e86281 100644 --- a/crates/worker/src/spawn/registry.rs +++ b/crates/worker/src/spawn/registry.rs @@ -88,7 +88,7 @@ impl InternalSpawnedWorkerRecord { pub(crate) fn new( worker_name: String, scope_delegated: Vec, - workdir_tool_scope: WorkdirScopeLeaseSet, + workdir_tool_scope: Arc, #[cfg(test)] installed_tools: Vec, session: InternalWorkerSessionHandle, child_registry: Arc, @@ -97,7 +97,7 @@ impl InternalSpawnedWorkerRecord { Self { worker_name: worker_name.clone(), scope_delegated, - workdir_tool_scope: Arc::new(workdir_tool_scope), + workdir_tool_scope, #[cfg(test)] installed_tools: installed_tools.into(), published: Arc::new(super::published::PublishedProtocol::new( @@ -329,6 +329,7 @@ pub struct SpawnedWorkerRegistry { pending_internal_notify: Notify, internal_spawn_cleanup_failed: AtomicBool, parent_scope: Option, + bash_output: Mutex, parent_protocol: Mutex, String)>>, protocol_publish_lock: Arc>, } @@ -340,6 +341,14 @@ pub struct SpawnedWorkerRegistryLoad { } impl SpawnedWorkerRegistry { + pub(crate) fn bash_output(&self) -> tools::bash_output::BashOutputView { + self.bash_output.lock().unwrap().clone() + } + + pub(crate) fn inherit_bash_output(&self, view: tools::bash_output::BashOutputView) { + *self.bash_output.lock().unwrap() = view; + } + pub(crate) fn new_for_internal_services() -> Arc { Arc::new(Self { internal_records: std::sync::Mutex::new(Vec::new()), @@ -352,6 +361,7 @@ impl SpawnedWorkerRegistry { pending_internal_notify: Notify::new(), internal_spawn_cleanup_failed: AtomicBool::new(false), parent_scope: None, + bash_output: Mutex::new(tools::bash_output::BashOutputView::new()), parent_protocol: Mutex::new(None), protocol_publish_lock: Arc::new(Mutex::new(())), }) @@ -370,6 +380,7 @@ impl SpawnedWorkerRegistry { pending_internal_notify: Notify::new(), internal_spawn_cleanup_failed: AtomicBool::new(false), parent_scope: None, + bash_output: Mutex::new(tools::bash_output::BashOutputView::new()), parent_protocol: Mutex::new(None), protocol_publish_lock: Arc::new(Mutex::new(())), }) @@ -387,6 +398,7 @@ impl SpawnedWorkerRegistry { pending_internal_notify: Notify::new(), internal_spawn_cleanup_failed: AtomicBool::new(false), parent_scope: Some(parent_scope), + bash_output: Mutex::new(tools::bash_output::BashOutputView::new()), parent_protocol: Mutex::new(None), protocol_publish_lock: Arc::new(Mutex::new(())), }) @@ -473,6 +485,7 @@ impl SpawnedWorkerRegistry { pending_internal_notify: Notify::new(), internal_spawn_cleanup_failed: AtomicBool::new(false), parent_scope, + bash_output: Mutex::new(tools::bash_output::BashOutputView::new()), parent_protocol: Mutex::new(None), protocol_publish_lock: Arc::new(Mutex::new(())), }), @@ -966,6 +979,10 @@ impl SpawnedWorkerRegistry { "an in-flight Internal SubWorker failed cleanup during shutdown", )); } + let output = self.bash_output(); + if !output.is_root() { + output.revoke(); + } first_error.map_or(Ok(()), Err) } @@ -1111,6 +1128,7 @@ fn internal_worker_live_snapshot(snapshot: InternalWorkerSnapshot) -> Option, registry: Arc, @@ -358,7 +358,7 @@ impl SubWorkerSpawnTool { workspace_context, parent_notifications, runtime_base, - bash_output_dir, + workspace_root, workdir_tool_broker, registry, @@ -430,15 +430,6 @@ impl Tool for SubWorkerSpawnTool { .map_err(|error| ToolError::InvalidArgument(error.to_string()))?; let workdir_rules = parse_workdir_scope(&input.scope)?; - let child_bash_output_dir = self.bash_output_dir.join("sub-workers").join(&input.name); - tokio::fs::create_dir_all(&child_bash_output_dir) - .await - .map_err(|error| { - ToolError::ExecutionFailed(format!( - "create Internal Worker Bash output directory {}: {error}", - child_bash_output_dir.display() - )) - })?; let workdir_tool_broker = require_workdir_tool_broker(self.workdir_tool_broker.as_ref())?; let tool_scopes = routed_workdir_tool_scopes( workdir_tool_broker, @@ -452,6 +443,7 @@ impl Tool for SubWorkerSpawnTool { .map_err(|error| { ToolError::InvalidArgument(format!("scope parent-owned Workdir tools: {error}")) })?; + let workdir_scope = Arc::new(workdir_scope); let child_workdir_tool_broker = workdir_scope.broker_router(); let spawn_selector = @@ -552,24 +544,17 @@ impl Tool for SubWorkerSpawnTool { ) .await .map_err(|error| ToolError::ExecutionFailed(format!("build Internal Worker: {error}")))?; - child - .add_scope_rules([ScopeRule { - target: child_bash_output_dir.clone(), - permission: manifest::Permission::Read, - recursive: true, - symlink_policy: Default::default(), - }]) - .map_err(|error| { - ToolError::ExecutionFailed(format!( - "grant Internal Worker Bash output scope: {error}" - )) - })?; let child_scope = child.scope().clone(); child.bind_workdir_sessions(workdir_scope.session_router()); let child_registry = SpawnedWorkerRegistry::new_internal(input.name.clone(), child_scope); + let output_lease = workdir_scope.clone(); + child_registry.inherit_bash_output( + self.registry + .bash_output() + .child(Arc::new(move || output_lease.is_active())), + ); register_worker_tools( &mut child, - child_bash_output_dir, self.runtime_base.clone(), child_registry.clone(), None, @@ -1052,7 +1037,7 @@ pub(crate) fn sub_worker_spawn_tool( workspace_context: crate::worker::WorkerWorkspaceContext, parent_notifications: ParentNotificationTarget, runtime_base: PathBuf, - bash_output_dir: PathBuf, + workspace_root: PathBuf, workdir_tool_broker: Option, registry: Arc, @@ -1064,7 +1049,6 @@ pub(crate) fn sub_worker_spawn_tool( workspace_context, parent_notifications, runtime_base, - bash_output_dir, workspace_root, workdir_tool_broker, registry, @@ -1078,7 +1062,7 @@ fn sub_worker_spawn_tool_impl( workspace_context: crate::worker::WorkerWorkspaceContext, parent_notifications: ParentNotificationTarget, runtime_base: PathBuf, - bash_output_dir: PathBuf, + workspace_root: PathBuf, workdir_tool_broker: Option, registry: Arc, @@ -1110,7 +1094,6 @@ fn sub_worker_spawn_tool_impl( workspace_context.clone(), parent_notifications.clone(), runtime_base.clone(), - bash_output_dir.clone(), workspace_root.clone(), workdir_tool_broker.clone(), registry.clone(), @@ -1374,7 +1357,6 @@ enabled = false ParentNotificationTarget::Durable(Arc::new(|_| {})), ), runtime.path().to_path_buf(), - bash_output_dir.clone(), workspace_root.clone(), Some(workdir_tool_broker), registry.clone(), @@ -1425,12 +1407,7 @@ enabled = false .await .expect("spawn project reviewer as Internal Worker"); assert!(output.summary.contains("internal worker `reviewer-child`")); - assert!( - bash_output_dir - .join("sub-workers") - .join("reviewer-child") - .is_dir() - ); + assert!(!bash_output_dir.join("sub-workers").exists()); assert!(spawner_scope.snapshot().is_writable(&workspace_root)); let record = registry .get_internal("reviewer-child") @@ -1460,6 +1437,27 @@ enabled = false record.session.wait_until_idle().await, crate::internal_worker::InternalWorkerSessionStatus::Idle ); + let child_output_view = record.child_registry.bash_output(); + let child_bash = tools::BashTool::new( + record.workdir_tool_scope.session_router(), + child_output_view.clone(), + ); + let child_output = child_bash + .execute_result( + r#"{"command":"printf retained-child-evidence","cwd":"."}"#, + Default::default(), + ) + .await + .unwrap(); + let output_id = child_output.metadata.id.clone(); + assert!(child_output_view.metadata(&output_id).is_some()); + assert_eq!( + registry + .bash_output() + .read(&output_id, workdir::CommandStream::Stdout, 0, 65536) + .unwrap(), + b"retained-child-evidence" + ); assert_eq!(calls.load(Ordering::SeqCst), 1); assert!(!observed_parent_write_revoked.load(Ordering::SeqCst)); assert!(observed_instruction_override.load(Ordering::SeqCst)); @@ -1563,6 +1561,8 @@ enabled = false !record.workdir_tool_scope.is_active(), "stopped child must revoke cloned scoped sessions" ); + assert!(child_output_view.metadata(&output_id).is_none()); + assert!(registry.bash_output().metadata(&output_id).is_some()); assert!(registry.get_internal("reviewer-child").is_some()); assert!( @@ -1573,6 +1573,7 @@ enabled = false .is_some() ); assert!(registry.get_internal("reviewer-child").is_none()); + assert!(registry.bash_output().metadata(&output_id).is_some()); assert!(spawner_scope.snapshot().is_writable(&workspace_root)); fail_requests.store(false, Ordering::SeqCst); @@ -1672,7 +1673,6 @@ enabled = false ), ParentNotificationTarget::Durable(Arc::new(|_| {})), runtime.path().to_path_buf(), - bash_output_dir, workspace_root.clone(), Some(workdir_tool_broker), registry.clone(), @@ -1805,7 +1805,6 @@ enabled = false ParentNotificationTarget::Durable(Arc::new(|_| {})), ), runtime.path().to_path_buf(), - bash_output_dir.clone(), workspace_root.clone(), Some(workdir_tool_broker), registry.clone(), diff --git a/crates/worker/src/subjektiv.rs b/crates/worker/src/subjektiv.rs index 40d4836c6..2009679ce 100644 --- a/crates/worker/src/subjektiv.rs +++ b/crates/worker/src/subjektiv.rs @@ -919,7 +919,6 @@ enabled = true WorkerFilesystemAuthority::None, WorkerBootstrapLayout::Direct { runtime_base: temp.path().join("runtime"), - bash_output_dir: temp.path().join("bash-output"), }, WorkerControllerTransport::InProcess, ) @@ -1113,7 +1112,6 @@ enabled = true WorkerFilesystemAuthority::None, WorkerBootstrapLayout::Direct { runtime_base: temp.path().join("runtime"), - bash_output_dir: temp.path().join("bash"), }, WorkerControllerTransport::InProcess, ) @@ -1279,7 +1277,6 @@ enabled = true WorkerFilesystemAuthority::None, WorkerBootstrapLayout::Direct { runtime_base: temp.path().join("runtime"), - bash_output_dir: temp.path().join("bash"), }, WorkerControllerTransport::InProcess, ) @@ -1339,7 +1336,6 @@ enabled = true WorkerFilesystemAuthority::None, WorkerBootstrapLayout::Direct { runtime_base: temp.path().join("runtime"), - bash_output_dir: temp.path().join("bash-output"), }, WorkerControllerTransport::InProcess, ) diff --git a/crates/worker/src/worker.rs b/crates/worker/src/worker.rs index 6af8d337b..b6c9c6766 100644 --- a/crates/worker/src/worker.rs +++ b/crates/worker/src/worker.rs @@ -17214,12 +17214,10 @@ permission = "write" let layout = if runtime_managed { crate::WorkerBootstrapLayout::RuntimeManagedRun { run_dir: temp.path().join("runtime/runs/1"), - bash_output_dir: temp.path().join("bash"), } } else { crate::WorkerBootstrapLayout::Direct { runtime_base: temp.path().join("runtime"), - bash_output_dir: temp.path().join("bash"), } }; crate::WorkerBootstrap::new( diff --git a/crates/worker/tests/compact_events_test.rs b/crates/worker/tests/compact_events_test.rs index 2aeb8f521..26caca683 100644 --- a/crates/worker/tests/compact_events_test.rs +++ b/crates/worker/tests/compact_events_test.rs @@ -1495,11 +1495,9 @@ async fn post_compact_pause_bills_partial_without_rearming_or_consuming_correlat let session_id = worker.session_id(); let runtime_tmp = tempfile::tempdir().unwrap(); - let bash_output_dir = runtime_tmp.path().join("bash-output"); - let (handle, shutdown_receiver) = - WorkerController::spawn(worker, runtime_tmp.path(), &bash_output_dir) - .await - .unwrap(); + let (handle, shutdown_receiver) = WorkerController::spawn(worker, runtime_tmp.path()) + .await + .unwrap(); let mut rx = handle.subscribe(); handle .send(Method::submit_text( @@ -1705,11 +1703,9 @@ async fn controller_does_not_repeat_a_persisted_mid_run_compaction_failure() { let (worker, store) = make_worker_with_manifest_and_store(MID_TURN_MANIFEST_TOML, client).await; let session_id = worker.session_id(); let runtime_tmp = tempfile::tempdir().unwrap(); - let bash_output_dir = runtime_tmp.path().join("bash-output"); - let (handle, shutdown_receiver) = - WorkerController::spawn(worker, runtime_tmp.path(), &bash_output_dir) - .await - .unwrap(); + let (handle, shutdown_receiver) = WorkerController::spawn(worker, runtime_tmp.path()) + .await + .unwrap(); let mut rx = handle.subscribe(); handle @@ -1830,11 +1826,9 @@ async fn manual_compact_cancel_clears_progress_before_returning_idle() { let source_session = worker.session_id(); let source_segment = worker.segment_id(); let runtime_tmp = tempfile::tempdir().unwrap(); - let bash_output_dir = runtime_tmp.path().join("bash-output"); - let (handle, shutdown_receiver) = - WorkerController::spawn(worker, runtime_tmp.path(), &bash_output_dir) - .await - .unwrap(); + let (handle, shutdown_receiver) = WorkerController::spawn(worker, runtime_tmp.path()) + .await + .unwrap(); let mut rx = handle.subscribe(); handle @@ -1996,8 +1990,7 @@ async fn controller_compact_method_publishes_progress_and_clear() { ]); let worker = make_worker_with_manifest(MANUAL_ONLY_MANIFEST_TOML, client).await; let runtime_tmp = tempfile::tempdir().unwrap(); - let bash_output_dir = runtime_tmp.path().join("bash-output"); - let (handle, _shutdown) = WorkerController::spawn(worker, runtime_tmp.path(), &bash_output_dir) + let (handle, _shutdown) = WorkerController::spawn(worker, runtime_tmp.path()) .await .unwrap(); let mut rx = handle.subscribe(); diff --git a/crates/worker/tests/controller_test.rs b/crates/worker/tests/controller_test.rs index 1938f0b18..e1ab29fbe 100644 --- a/crates/worker/tests/controller_test.rs +++ b/crates/worker/tests/controller_test.rs @@ -311,8 +311,7 @@ async fn spawn_controller(worker: Worker) -> WorkerHandle let tmp = tempfile::tempdir().unwrap(); let runtime_base = tmp.path().to_owned(); std::mem::forget(tmp); - let bash_output_dir = runtime_base.join("bash-output"); - let (handle, _shutdown_rx) = WorkerController::spawn(worker, &runtime_base, &bash_output_dir) + let (handle, _shutdown_rx) = WorkerController::spawn(worker, &runtime_base) .await .unwrap(); handle @@ -342,13 +341,9 @@ async fn canonical_worker_handle_uploads_and_submits_attachment() { ) .await .unwrap(); - let (handle, shutdown_rx) = WorkerController::spawn( - worker, - &root.path().join("runtime"), - &root.path().join("bash-output"), - ) - .await - .unwrap(); + let (handle, shutdown_rx) = WorkerController::spawn(worker, &root.path().join("runtime")) + .await + .unwrap(); let mut rx = handle.subscribe(); let file = handle .upload_file_with_context( @@ -408,24 +403,20 @@ async fn canonical_worker_handle_uploads_and_submits_attachment() { } #[tokio::test] -async fn controller_grants_read_scope_for_exact_bash_output_directory() { +async fn controller_does_not_grant_or_create_a_model_visible_bash_output_directory() { let worker = make_worker(MockClient::new(simple_text_events())).await; let shared_scope = worker.scope().clone(); let runtime_base = tempfile::tempdir().unwrap(); - let worker_tmp = tempfile::tempdir().unwrap(); - let bash_output_dir = worker_tmp.path().join("worker-1").join("bash-output"); + let original_rules = shared_scope.snapshot().allow_rules().to_vec(); - let (handle, shutdown_rx) = - WorkerController::spawn(worker, runtime_base.path(), &bash_output_dir) - .await - .unwrap(); + let (handle, shutdown_rx) = WorkerController::spawn(worker, runtime_base.path()) + .await + .unwrap(); - assert!(bash_output_dir.is_dir()); - assert!(shared_scope.snapshot().allow_rules().iter().any(|rule| { - rule.target == bash_output_dir - && rule.permission == manifest::Permission::Read - && rule.recursive - })); + assert_eq!( + shared_scope.snapshot().allow_rules(), + original_rules.as_slice() + ); assert!(!handle.runtime_dir.path().join("bash-output").exists()); handle @@ -451,9 +442,9 @@ async fn shutdown_closes_bound_workdir_session() { .start_command(CommandRequest { command: "sleep 30".to_owned(), timeout_secs: 60, - output_limit: 1024, + + transport_byte_budget: workdir::MAX_COMMAND_TRANSPORT_BYTES, cwd: workdir::WorkdirPath::root(), - spill_dir: None, tool_call_id: None, }) .await @@ -461,11 +452,9 @@ async fn shutdown_closes_bound_workdir_session() { worker.bind_single_workdir_session(Some(Arc::clone(&session))); let runtime_base = tempfile::tempdir().unwrap(); - let bash_output_dir = runtime_base.path().join("bash-output"); - let (handle, shutdown_rx) = - WorkerController::spawn(worker, runtime_base.path(), &bash_output_dir) - .await - .unwrap(); + let (handle, shutdown_rx) = WorkerController::spawn(worker, runtime_base.path()) + .await + .unwrap(); handle .send(Method::Shutdown { command: worker_command(&handle), @@ -501,9 +490,9 @@ async fn controller_filters_workdir_command_not_owned_by_worker() { .start_command(CommandRequest { command: "sleep 0.2".to_owned(), timeout_secs: 5, - output_limit: 1024, + + transport_byte_budget: workdir::MAX_COMMAND_TRANSPORT_BYTES, cwd: workdir::WorkdirPath::root(), - spill_dir: None, tool_call_id: Some("foreign-tool-call".into()), }) .await @@ -529,7 +518,10 @@ async fn controller_filters_workdir_command_not_owned_by_worker() { }) .await .unwrap(); - assert_eq!(output.status, workdir::CommandStatus::Completed); + assert_eq!( + output.terminal.unwrap().status, + workdir::CommandStatus::Completed + ); handle .send(Method::Shutdown { command: worker_command(&handle), @@ -738,9 +730,8 @@ async fn controller_startup_failure_closes_bound_workdir_session() { let invalid_runtime_base = runtime_base.path().join("not-a-directory"); std::fs::write(&invalid_runtime_base, "file").unwrap(); - let bash_output_dir = runtime_base.path().join("bash-output"); assert!( - WorkerController::spawn(worker, &invalid_runtime_base, &bash_output_dir) + WorkerController::spawn(worker, &invalid_runtime_base) .await .is_err() ); @@ -749,9 +740,9 @@ async fn controller_startup_failure_closes_bound_workdir_session() { .start_command(CommandRequest { command: "printf unreachable".to_owned(), timeout_secs: 5, - output_limit: 1024, + + transport_byte_budget: workdir::MAX_COMMAND_TRANSPORT_BYTES, cwd: workdir::WorkdirPath::root(), - spill_dir: None, tool_call_id: None, }) .await, @@ -1553,8 +1544,7 @@ permission = "write" let client = MockClient::new(simple_text_events()); let worker = make_worker_with_pwd_and_manifest(client, manifest).await.0; let tmp = tempfile::tempdir().unwrap(); - let bash_output_dir = tmp.path().join("bash-output"); - let result = WorkerController::spawn(worker, tmp.path(), &bash_output_dir).await; + let result = WorkerController::spawn(worker, tmp.path()).await; assert!( result.is_ok(), "feature exposure must not imply delegation authority" @@ -2927,11 +2917,9 @@ async fn status_json_reflects_worker_name() { async fn shutdown_closes_method_admission_before_terminal_confirmation() { let worker = make_worker(MockClient::new(simple_text_events())).await; let runtime_base = tempfile::tempdir().unwrap(); - let bash_output_dir = runtime_base.path().join("bash-output"); - let (handle, mut shutdown_rx) = - WorkerController::spawn(worker, runtime_base.path(), &bash_output_dir) - .await - .unwrap(); + let (handle, mut shutdown_rx) = WorkerController::spawn(worker, runtime_base.path()) + .await + .unwrap(); handle .send(Method::Shutdown { command: worker_command(&handle), @@ -2966,11 +2954,9 @@ async fn shutdown_joins_socket_server_with_active_connection() { let worker = make_worker(MockClient::new(simple_text_events())).await; let runtime_base = tempfile::tempdir().unwrap(); - let bash_output_dir = runtime_base.path().join("bash-output"); - let (handle, shutdown_rx) = - WorkerController::spawn(worker, runtime_base.path(), &bash_output_dir) - .await - .unwrap(); + let (handle, shutdown_rx) = WorkerController::spawn(worker, runtime_base.path()) + .await + .unwrap(); let socket_path = handle.runtime_dir.socket_path(); let _connection = UnixStream::connect(&socket_path).await.unwrap(); diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index dd256fea9..96ff86839 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -475,6 +475,7 @@ struct WorkdirCommandSession { attachment_alias: String, source: WorkdirSessionHandle, provider_handle: CommandHandle, + terminal_at: Option, } enum RegisteredWorkdirSession { @@ -515,6 +516,7 @@ impl WorkdirSessionRegistry { source: WorkdirSessionHandle, provider_handle: CommandHandle, ) -> CommandHandle { + self.prune_terminal_commands(&worker); let external_handle = loop { let candidate = CommandHandle(Uuid::now_v7().to_string()); if !self @@ -530,6 +532,7 @@ impl WorkdirSessionRegistry { attachment_alias, source, provider_handle, + terminal_at: None, }, ); external_handle @@ -547,6 +550,45 @@ impl WorkdirSessionRegistry { .cloned() } + fn active_command_count(&self, worker: &RuntimeWorkerRef) -> usize { + self.commands + .iter() + .filter(|((owner, _), command)| owner == worker && command.terminal_at.is_none()) + .count() + } + + fn mark_terminal(&mut self, worker: &RuntimeWorkerRef, external_handle: &CommandHandle) { + if let Some(command) = self + .commands + .get_mut(&(worker.clone(), external_handle.clone())) + { + command + .terminal_at + .get_or_insert_with(std::time::Instant::now); + } + self.prune_terminal_commands(worker); + } + + fn prune_terminal_commands(&mut self, worker: &RuntimeWorkerRef) { + let mut terminal = self + .commands + .iter() + .filter_map(|((owner, handle), command)| { + (owner == worker) + .then_some(command.terminal_at) + .flatten() + .map(|at| (handle.clone(), at)) + }) + .collect::>(); + terminal.sort_by_key(|(_, at)| *at); + let excess = terminal + .len() + .saturating_sub(MAX_RETAINED_TERMINAL_COMMANDS); + for (handle, _) in terminal.into_iter().take(excess) { + self.commands.remove(&(worker.clone(), handle)); + } + } + fn take_worker_commands_for_alias( &mut self, worker: &RuntimeWorkerRef, @@ -895,7 +937,27 @@ struct ExternalProviderWorkdirSession { audit: Option, closed: std::sync::atomic::AtomicBool, close_lock: tokio::sync::Mutex<()>, - active_commands: tokio::sync::Mutex>, + active_commands: tokio::sync::Mutex>>, +} + +const MAX_RETAINED_TERMINAL_COMMANDS: usize = 128; +const MAX_ACTIVE_SERVER_COMMANDS: usize = 128; + +struct ExternalProviderCommandState { + handle: workdir::CommandHandle, + // Serialize output ACKs, not cancellation: cancel must unblock a waiting reader. + cursor: tokio::sync::Mutex, + terminal_at: Mutex>, +} + +impl ExternalProviderCommandState { + fn new(handle: workdir::CommandHandle) -> Self { + Self { + handle, + cursor: tokio::sync::Mutex::new(0), + terminal_at: Mutex::new(None), + } + } } impl std::fmt::Debug for ExternalProviderWorkdirSession { @@ -986,17 +1048,24 @@ impl ExternalProviderWorkdirSession { "External Workdir grant has expired".to_string(), )); } - let _admission = self - .connection - .admission - .clone() - .acquire_owned() - .await - .map_err(|_| { - workdir::WorkdirError::Unavailable( - "External Workdir provider admission is closed".to_string(), - ) - })?; + // Cancellation must reach the provider even when waiting output readers + // occupy every data-operation slot. The socket separately bounds control requests. + let _admission = if matches!(&operation, WorkdirSessionOperation::CommandCancel(_)) { + None + } else { + Some( + self.connection + .admission + .clone() + .acquire_owned() + .await + .map_err(|_| { + workdir::WorkdirError::Unavailable( + "External Workdir provider admission is closed".to_string(), + ) + })?, + ) + }; if self .connection .expires_at @@ -1073,10 +1142,10 @@ impl ExternalProviderWorkdirSession { } } - async fn provider_command_handle( + async fn provider_command( &self, external_handle: &workdir::CommandHandle, - ) -> std::result::Result { + ) -> std::result::Result, workdir::WorkdirError> { if self.closed.load(Ordering::Acquire) { return Err(workdir::WorkdirError::SessionClosed); } @@ -1088,51 +1157,111 @@ impl ExternalProviderWorkdirSession { .ok_or_else(|| workdir::WorkdirError::UnknownCommand(external_handle.0.clone())) } + async fn provider_command_handle( + &self, + external_handle: &workdir::CommandHandle, + ) -> std::result::Result { + Ok(self.provider_command(external_handle).await?.handle.clone()) + } + async fn remove_active_command_if_matches( &self, external_handle: &workdir::CommandHandle, - provider_handle: &workdir::CommandHandle, + command: &Arc, ) { let mut commands = self.active_commands.lock().await; - if commands.get(&external_handle.0) == Some(provider_handle) { + if commands + .get(&external_handle.0) + .is_some_and(|current| Arc::ptr_eq(current, command)) + { commands.remove(&external_handle.0); } } + async fn prune_terminal_commands(&self) { + let mut commands = self.active_commands.lock().await; + let mut terminal = commands + .iter() + .filter_map(|(key, command)| { + command + .terminal_at + .lock() + .unwrap() + .map(|at| (key.clone(), at)) + }) + .collect::>(); + terminal.sort_by_key(|(_, at)| *at); + let excess = terminal + .len() + .saturating_sub(MAX_RETAINED_TERMINAL_COMMANDS); + for (key, _) in terminal.into_iter().take(excess) { + commands.remove(&key); + } + } + async fn terminate_provider_command( &self, - provider_handle: workdir::CommandHandle, + command: Arc, + reused_handle: bool, ) -> std::result::Result<(), workdir::WorkdirError> { - // Cancellation is best effort because the command may already be terminal. - // Terminal output is the proof that the provider reaped the process. + if !reused_handle && command.terminal_at.lock().unwrap().is_some() { + return Ok(()); + } + // Do not acquire the output lock until cancellation has unblocked any reader. + // The reader records its last known cursor before cleanup acknowledges bytes. let _ = self .operate(WorkdirSessionOperation::CommandCancel( - provider_handle.clone(), + command.handle.clone(), )) .await; - match self - .operate(WorkdirSessionOperation::CommandOutput( - workdir::CommandOutputRequest { - handle: provider_handle, - cursor: 0, - limit: workdir::external::MAX_EXTERNAL_COMMAND_OUTPUT_BYTES, - wait: true, - }, - )) - .await? - { - WorkdirSessionOperationResult::CommandOutput(output) - if output.status != workdir::CommandStatus::Running => - { - Ok(()) - } - WorkdirSessionOperationResult::CommandOutput(_) => { - Err(workdir::WorkdirError::Unavailable( - "External Workdir provider did not confirm command termination".to_string(), - )) - } - _ => Err(Self::mismatch("command_output")), + let mut cursor = + tokio::time::timeout(std::time::Duration::from_secs(30), command.cursor.lock()) + .await + .map_err(|_| { + workdir::WorkdirError::Unavailable( + "External Workdir reader did not stop after cancellation".to_string(), + ) + })?; + if !reused_handle && command.terminal_at.lock().unwrap().is_some() { + return Ok(()); } + let drain = async { + loop { + match self + .operate(WorkdirSessionOperation::CommandOutput( + workdir::CommandOutputRequest { + handle: command.handle.clone(), + cursor: *cursor, + limit: workdir::MAX_COMMAND_READ_BYTES, + wait: true, + }, + )) + .await? + { + WorkdirSessionOperationResult::CommandOutput(output) => { + *cursor = output.next_cursor; + if output.terminal.is_some() { + *command.terminal_at.lock().unwrap() = Some(std::time::Instant::now()); + return Ok(()); + } + if output.chunks.is_empty() { + return Err(workdir::WorkdirError::Unavailable( + "External Workdir provider did not confirm command termination" + .to_string(), + )); + } + } + _ => return Err(Self::mismatch("command_output")), + } + } + }; + tokio::time::timeout(std::time::Duration::from_secs(30), drain) + .await + .map_err(|_| { + workdir::WorkdirError::Unavailable( + "External Workdir command drain timed out".to_string(), + ) + })? } fn mismatch(expected: &str) -> workdir::WorkdirError { @@ -1410,17 +1539,27 @@ impl workdir::WorkdirSession for ExternalProviderWorkdirSession { } async fn start_command( &self, - mut request: workdir::CommandRequest, + request: workdir::CommandRequest, ) -> std::result::Result { self.ensure_capability(workdir::WorkdirSessionCapability::Command)?; let _close_guard = self.close_lock.lock().await; if self.closed.load(Ordering::Acquire) { return Err(workdir::WorkdirError::SessionClosed); } - // Bash supplies a Worker-host spill directory. It is neither meaningful - // nor safe to send to a client-hosted provider, so External commands use - // bounded inline output and report truncation without a provider path. - request.spill_dir = None; + self.prune_terminal_commands().await; + if self + .active_commands + .lock() + .await + .values() + .filter(|command| command.terminal_at.lock().unwrap().is_none()) + .count() + >= MAX_ACTIVE_SERVER_COMMANDS + { + return Err(workdir::WorkdirError::Unavailable( + "External Workdir active command limit reached".to_string(), + )); + } match self .operate(WorkdirSessionOperation::CommandStart(request)) .await? @@ -1430,7 +1569,7 @@ impl workdir::WorkdirSession for ExternalProviderWorkdirSession { let commands = self.active_commands.lock().await; commands .iter() - .filter(|(_, active)| *active == &provider_handle) + .filter(|(_, active)| active.handle == provider_handle) .map(|(external, _)| external.clone()) .collect::>() }; @@ -1439,20 +1578,29 @@ impl workdir::WorkdirSession for ExternalProviderWorkdirSession { // generation. Remove every ambiguous route before attempting // cleanup so no caller can address the wrong process. let mut commands = self.active_commands.lock().await; + let mut quarantined = None; for external_handle in duplicate_handles { - commands.remove(&external_handle); + if let Some(command) = commands.remove(&external_handle) { + quarantined.get_or_insert(command); + } } let quarantine_handle = workdir::CommandHandle(format!("quarantined-command-{}", Uuid::now_v7())); - commands.insert(quarantine_handle.0.clone(), provider_handle.clone()); + let command = quarantined.expect("duplicate command route disappeared"); + *command.terminal_at.lock().unwrap() = None; + commands.insert(quarantine_handle.0.clone(), command.clone()); drop(commands); if self - .terminate_provider_command(provider_handle.clone()) + .terminate_provider_command(command.clone(), true) .await .is_ok() { - self.remove_active_command_if_matches(&quarantine_handle, &provider_handle) + self.remove_active_command_if_matches(&quarantine_handle, &command) .await; + } else { + // An earlier read of the reused handle cannot establish that + // the newly started, ambiguous process has been cleaned up. + *command.terminal_at.lock().unwrap() = None; } return Err(workdir::WorkdirError::Unavailable( "External Workdir provider reused an active command handle".to_string(), @@ -1460,10 +1608,10 @@ impl workdir::WorkdirSession for ExternalProviderWorkdirSession { } let external_handle = workdir::CommandHandle(format!("external-command-{}", Uuid::now_v7())); - self.active_commands - .lock() - .await - .insert(external_handle.0.clone(), provider_handle); + self.active_commands.lock().await.insert( + external_handle.0.clone(), + Arc::new(ExternalProviderCommandState::new(provider_handle)), + ); Ok(external_handle) } _ => Err(Self::mismatch("command_start")), @@ -1489,21 +1637,29 @@ impl workdir::WorkdirSession for ExternalProviderWorkdirSession { ) -> std::result::Result { self.ensure_capability(workdir::WorkdirSessionCapability::Command)?; let external_handle = request.handle.clone(); - let provider_handle = self.provider_command_handle(&external_handle).await?; - request.handle = provider_handle.clone(); + let command = self.provider_command(&external_handle).await?; + let mut cursor = command.cursor.lock().await; + request.handle = command.handle.clone(); + // If the response is lost, this request cursor still replays the provider page. match self .operate(WorkdirSessionOperation::CommandOutput(request)) - .await? + .await { - WorkdirSessionOperationResult::CommandOutput(output) => { - if output.status != workdir::CommandStatus::Running && output.next_cursor.is_none() - { - self.remove_active_command_if_matches(&external_handle, &provider_handle) - .await; + Ok(WorkdirSessionOperationResult::CommandOutput(output)) => { + *cursor = output.next_cursor; + if output.terminal.is_some() { + command + .terminal_at + .lock() + .unwrap() + .get_or_insert_with(std::time::Instant::now); } + drop(cursor); + self.prune_terminal_commands().await; Ok(output) } - _ => Err(Self::mismatch("command_output")), + Ok(_) => Err(Self::mismatch("command_output")), + Err(error) => Err(error), } } async fn cancel_command( @@ -1540,21 +1696,25 @@ impl workdir::WorkdirSession for ExternalProviderWorkdirSession { }) .collect::>(); let mut cleanup_errors = Vec::new(); - for (external_handle, provider_handle) in commands { + for (external_handle, command) in commands { // Detach/session replacement is terminal authority for commands // started through this exact provider generation. Do not report a // successful close until the provider confirms terminal output. match self - .terminate_provider_command(provider_handle.clone()) + .terminate_provider_command(command.clone(), false) .await { Ok(()) => { - self.remove_active_command_if_matches(&external_handle, &provider_handle) + self.remove_active_command_if_matches(&external_handle, &command) .await; } Err(error) => { - let still_active = self.active_commands.lock().await.get(&external_handle.0) - == Some(&provider_handle); + let still_active = self + .active_commands + .lock() + .await + .get(&external_handle.0) + .is_some_and(|current| Arc::ptr_eq(current, &command)); if still_active { cleanup_errors.push(error.to_string()); } @@ -20811,6 +20971,9 @@ async fn scoped_execute_current_worker_workdir_operation( &api, &worker, &target_workdir, Some(&_session_guard), ).await?; context.workdir_id_hash = Some(crate::worker_source::diagnostic_id_hash(&link.workdir_id)); + if api.workdir_sessions.lock().unwrap().active_command_count(&worker) >= MAX_ACTIVE_SERVER_COMMANDS { + return Err(InternalWorkdirOperationError::Provider(workdir_operation_transport_error(workdir::WorkdirError::Unavailable("Worker active command limit reached".to_string())))); + } context.stage = "session_open"; let source = open_current_worker_workdir_session_locked(&api, &worker, &link, &_session_guard).await?; context.stage = "provider_dispatch"; @@ -20852,14 +21015,16 @@ async fn scoped_execute_current_worker_workdir_operation( .map_err(workdir_operation_transport_error)? } WorkdirSessionOperation::CommandOutput(mut output) => { + let external_handle = output.handle.clone(); let (session, provider_handle) = - current_worker_command_session(&api, &worker, &target_workdir, &output.handle, &mut context).await?; + current_worker_command_session(&api, &worker, &target_workdir, &external_handle, &mut context).await?; output.handle = provider_handle; - session - .command_output(output) - .await - .map(WorkdirSessionOperationResult::CommandOutput) - .map_err(workdir_operation_transport_error)? + let page = session.command_output(output).await.map_err(workdir_operation_transport_error)?; + if page.terminal.is_some() { + // Retain the binding for a retry after an unknown HTTP outcome. + api.workdir_sessions.lock().unwrap().mark_terminal(&worker, &external_handle); + } + WorkdirSessionOperationResult::CommandOutput(page) } WorkdirSessionOperation::CommandCancel(external_handle) => { let (session, provider_handle) = @@ -26838,7 +27003,12 @@ async fn serve_external_workdir_provider( })); continue; } - if pending.len() >= 16 { + let pending_limit = if matches!(&operation, WorkdirSessionOperation::CommandCancel(_)) { + 16 + MAX_ACTIVE_SERVER_COMMANDS + } else { + 16 + }; + if pending.len() >= pending_limit { let _ = response.send(Err(WorkdirTransportError { denial_reason: None, code: workdir::http::WorkdirTransportErrorCode::Unavailable, @@ -37009,6 +37179,7 @@ mod tests { mod api_logging_tests; mod auth_logging_tests; + mod command_transport_tests; mod drive_api_tests; mod drive_tests; mod drive_web_roundtrip_tests; @@ -37358,9 +37529,8 @@ mod tests { .start_command(workdir::CommandRequest { command: "touch forbidden".to_string(), timeout_secs: 1, - output_limit: 1024, + transport_byte_budget: workdir::MAX_COMMAND_TRANSPORT_BYTES, cwd: workdir::WorkdirPath::root(), - spill_dir: None, tool_call_id: None, }) .await; @@ -37509,9 +37679,8 @@ mod tests { .start_command(workdir::CommandRequest { command: "printf forbidden".to_string(), timeout_secs: 30, - output_limit: 1024, + transport_byte_budget: workdir::MAX_COMMAND_TRANSPORT_BYTES, cwd: workdir::WorkdirPath::root(), - spill_dir: None, tool_call_id: None, }) .await; @@ -37563,9 +37732,8 @@ mod tests { .start_command(workdir::CommandRequest { command: "printf routed".to_string(), timeout_secs: 30, - output_limit: 1024, + transport_byte_budget: workdir::MAX_COMMAND_TRANSPORT_BYTES, cwd: workdir::WorkdirPath::root(), - spill_dir: Some("/worker/private/spill".into()), tool_call_id: Some("call-1".to_string()), }) .await @@ -37581,10 +37749,7 @@ mod tests { }; assert!(matches!( operation, - WorkdirSessionOperation::CommandStart(workdir::CommandRequest { - spill_dir: None, - .. - }) + WorkdirSessionOperation::CommandStart(workdir::CommandRequest { .. }) )); let provider_handle = workdir::CommandHandle("command-1".to_string()); response @@ -37637,13 +37802,9 @@ mod tests { response .send(Ok(WorkdirSessionOperationResult::CommandOutput( workdir::CommandOutput { - status: workdir::CommandStatus::Running, - exit_code: None, - timed_out: false, - content: String::new(), - next_cursor: None, - truncated: false, - output_path: None, + chunks: Vec::new(), + next_cursor: 0, + terminal: None, }, ))) .unwrap(); @@ -37716,13 +37877,22 @@ mod tests { response .send(Ok(WorkdirSessionOperationResult::CommandOutput( workdir::CommandOutput { - status: workdir::CommandStatus::Cancelled, - exit_code: None, - timed_out: false, - content: String::new(), - next_cursor: None, - truncated: false, - output_path: None, + chunks: Vec::new(), + next_cursor: 0, + terminal: Some(workdir::CommandTerminal { + status: workdir::CommandStatus::Cancelled, + exit_code: None, + + stdout_bytes: 0, + + stderr_bytes: 0, + + stdout_delivered_bytes: 0, + + stderr_delivered_bytes: 0, + + truncation_reason: None, + }), }, ))) .unwrap(); @@ -37751,9 +37921,8 @@ mod tests { let request = || workdir::CommandRequest { command: "sleep 30".to_string(), timeout_secs: 30, - output_limit: 1024, + transport_byte_budget: workdir::MAX_COMMAND_TRANSPORT_BYTES, cwd: workdir::WorkdirPath::root(), - spill_dir: None, tool_call_id: None, }; let provider_handle = workdir::CommandHandle("command-reused".to_string()); @@ -37810,13 +37979,22 @@ mod tests { response .send(Ok(WorkdirSessionOperationResult::CommandOutput( workdir::CommandOutput { - status: workdir::CommandStatus::Cancelled, - exit_code: None, - timed_out: false, - content: String::new(), - next_cursor: None, - truncated: false, - output_path: None, + chunks: Vec::new(), + next_cursor: 0, + terminal: Some(workdir::CommandTerminal { + status: workdir::CommandStatus::Cancelled, + exit_code: None, + + stdout_bytes: 0, + + stderr_bytes: 0, + + stdout_delivered_bytes: 0, + + stderr_delivered_bytes: 0, + + truncation_reason: None, + }), }, ))) .unwrap(); @@ -37867,9 +38045,8 @@ mod tests { let request = || workdir::CommandRequest { command: "sleep 30".to_string(), timeout_secs: 30, - output_limit: 1024, + transport_byte_budget: workdir::MAX_COMMAND_TRANSPORT_BYTES, cwd: workdir::WorkdirPath::root(), - spill_dir: None, tool_call_id: None, }; let provider_handle = workdir::CommandHandle("command-1".to_string()); @@ -38766,9 +38943,8 @@ mod tests { .start_command(workdir::CommandRequest { command: "printf forbidden".to_string(), timeout_secs: 30, - output_limit: 1024, + transport_byte_budget: workdir::MAX_COMMAND_TRANSPORT_BYTES, cwd: workdir::WorkdirPath::root(), - spill_dir: None, tool_call_id: None, }) .await; @@ -38789,9 +38965,8 @@ mod tests { .start_command(workdir::CommandRequest { command: "printf attached".to_string(), timeout_secs: 30, - output_limit: 1024, + transport_byte_budget: workdir::MAX_COMMAND_TRANSPORT_BYTES, cwd: workdir::WorkdirPath::root(), - spill_dir: None, tool_call_id: None, }) .await @@ -38894,13 +39069,22 @@ mod tests { response .send(Ok(WorkdirSessionOperationResult::CommandOutput( workdir::CommandOutput { - status: workdir::CommandStatus::Cancelled, - exit_code: None, - timed_out: false, - content: String::new(), - next_cursor: None, - truncated: false, - output_path: None, + chunks: Vec::new(), + next_cursor: 0, + terminal: Some(workdir::CommandTerminal { + status: workdir::CommandStatus::Cancelled, + exit_code: None, + + stdout_bytes: 0, + + stderr_bytes: 0, + + stdout_delivered_bytes: 0, + + stderr_delivered_bytes: 0, + + truncation_reason: None, + }), }, ))) .unwrap(); @@ -38985,11 +39169,9 @@ mod tests { }; let (mut old_provider, _) = connect_async(&url).await.unwrap(); - let old_registration = serde_json::to_string(®ister(1)).unwrap().replacen( - r#""version":2"#, - r#""version":1"#, - 1, - ); + let mut old_registration = serde_json::to_value(register(1)).unwrap(); + old_registration["version"] = serde_json::json!(1); + let old_registration = serde_json::to_string(&old_registration).unwrap(); old_provider .send(Message::Text(old_registration.into())) .await @@ -39003,7 +39185,10 @@ mod tests { .reason .contains("unsupported External Workdir protocol version 1") ); - assert!(frame.reason.contains("requires 2")); + assert!(frame.reason.contains(&format!( + "requires {}", + workdir::external::EXTERNAL_WORKDIR_PROVIDER_PROTOCOL_VERSION + ))); let (mut smaller_provider, _) = connect_async(&url).await.unwrap(); let mut smaller_registration = serde_json::to_value(register(1)).unwrap(); @@ -39244,37 +39429,106 @@ mod tests { fs::read_to_string(provider_root.path().join("created.txt")).unwrap(), "created" ); + let binary_handle = broker_session + .start_command(workdir::CommandRequest { + command: "head -c 196609 /dev/zero".to_string(), + timeout_secs: 30, + transport_byte_budget: workdir::MAX_COMMAND_TRANSPORT_BYTES, + cwd: workdir::WorkdirPath::root(), + tool_call_id: None, + }) + .await + .unwrap(); + let (terminal_request, terminal_page) = + tokio::time::timeout(std::time::Duration::from_secs(5), async { + let mut cursor = 0; + let mut bytes = Vec::new(); + loop { + let request = workdir::CommandOutputRequest { + handle: binary_handle.clone(), + cursor, + limit: workdir::MAX_COMMAND_READ_BYTES, + wait: true, + }; + let page = broker_session + .command_output(request.clone()) + .await + .unwrap(); + for chunk in &page.chunks { + assert_eq!(chunk.stream, workdir::CommandStream::Stdout); + assert_eq!(chunk.sequence, bytes.len() as u64); + assert_eq!(chunk.offset, bytes.len() as u64); + bytes.extend_from_slice(&chunk.bytes); + } + assert!( + page.chunks + .iter() + .map(|chunk| chunk.bytes.len()) + .sum::() + <= workdir::MAX_COMMAND_READ_BYTES + ); + cursor = page.next_cursor; + if let Some(terminal) = &page.terminal { + assert_eq!(terminal.status, workdir::CommandStatus::Completed); + assert_eq!(terminal.exit_code, Some(0)); + assert_eq!(bytes, vec![0; 196609]); + break (request, page); + } + } + }) + .await + .expect("incremental provider output did not terminate"); + assert_eq!( + broker_session + .command_output(terminal_request.clone()) + .await + .unwrap(), + terminal_page + ); + for cursor in [0, terminal_page.next_cursor + 1] { + assert!( + broker_session + .command_output(workdir::CommandOutputRequest { + cursor, + ..terminal_request.clone() + }) + .await + .is_err() + ); + } + let command_handle = broker_session .start_command(workdir::CommandRequest { - command: "sleep 30 & child=$!; echo $child > child.pid; wait $child".to_string(), + command: "sleep 30 & child=$!; echo $child > child.pid; printf ready; wait $child" + .to_string(), timeout_secs: 30, - output_limit: 1024, + transport_byte_budget: workdir::MAX_COMMAND_TRANSPORT_BYTES, cwd: workdir::WorkdirPath::root(), - spill_dir: Some("/worker/private/spill".into()), tool_call_id: Some("call-e2e".to_string()), }) .await .unwrap(); - for _ in 0..100 { - if provider_root.path().join("child.pid").is_file() { - break; - } - tokio::time::sleep(std::time::Duration::from_millis(10)).await; - } - assert!(provider_root.path().join("child.pid").is_file()); - let command_output = broker_session .command_output(workdir::CommandOutputRequest { handle: command_handle, cursor: 0, limit: 1024, - wait: false, + wait: true, }) .await .unwrap(); - assert_eq!(command_output.status, workdir::CommandStatus::Running); - assert_eq!(command_output.output_path, None); + assert!(command_output.terminal.is_none()); + + assert_eq!( + command_output + .chunks + .iter() + .flat_map(|chunk| chunk.bytes.clone()) + .collect::>(), + b"ready" + ); + assert!(provider_root.path().join("child.pid").is_file()); let Json(revoked) = scoped_revoke_external_workdir_grant( State(api.clone()), @@ -40477,9 +40731,8 @@ mod tests { .start_command(workdir::CommandRequest { command: "printf ready; sleep 30".to_string(), timeout_secs: 60, - output_limit: 4096, + transport_byte_budget: workdir::MAX_COMMAND_TRANSPORT_BYTES, cwd: workdir::WorkdirPath::root(), - spill_dir: None, tool_call_id: Some("tool-call-command-session".to_string()), }) .await @@ -40526,7 +40779,7 @@ mod tests { }) .await .unwrap(); - assert!(matches!(output.status, workdir::CommandStatus::Running)); + assert!(output.terminal.is_none()); assert!(matches!( command .source @@ -40538,14 +40791,21 @@ mod tests { let waiting_source = command.source.clone(); let waiting_handle = command.provider_handle.clone(); let waiting = tokio::spawn(async move { - waiting_source - .command_output(workdir::CommandOutputRequest { - handle: waiting_handle, - cursor: output.next_cursor.unwrap_or(0), - limit: 4096, - wait: true, - }) - .await + let mut cursor = output.next_cursor; + loop { + let page = waiting_source + .command_output(workdir::CommandOutputRequest { + handle: waiting_handle.clone(), + cursor, + limit: 4096, + wait: true, + }) + .await?; + cursor = page.next_cursor; + if page.terminal.is_some() { + return Ok::<_, workdir::WorkdirError>(page); + } + } }); tokio::task::yield_now().await; command @@ -40558,7 +40818,7 @@ mod tests { .expect("cancel should unblock a waiting output read") .unwrap() .unwrap(); - assert!(!matches!(terminal.status, workdir::CommandStatus::Running)); + assert!(terminal.terminal.is_some()); let registered = registry.take_worker(&worker); assert_eq!(registered.len(), 1); @@ -40591,9 +40851,8 @@ mod tests { let request = || workdir::CommandRequest { command: "sleep 30".to_string(), timeout_secs: 60, - output_limit: 4096, + transport_byte_budget: workdir::MAX_COMMAND_TRANSPORT_BYTES, cwd: workdir::WorkdirPath::root(), - spill_dir: None, tool_call_id: None, }; let left_provider = left.start_command(request()).await.unwrap(); @@ -60061,9 +60320,8 @@ mod tests { .start_command(workdir::CommandRequest { command: "sleep 30".to_string(), timeout_secs: 60, - output_limit: 4096, + transport_byte_budget: workdir::MAX_COMMAND_TRANSPORT_BYTES, cwd: workdir::WorkdirPath::root(), - spill_dir: None, tool_call_id: None, }) .await diff --git a/crates/workspace-server/src/server/tests/api_logging_tests.rs b/crates/workspace-server/src/server/tests/api_logging_tests.rs index 3c9864195..95b104522 100644 --- a/crates/workspace-server/src/server/tests/api_logging_tests.rs +++ b/crates/workspace-server/src/server/tests/api_logging_tests.rs @@ -471,11 +471,10 @@ async fn generated_workdir_provider_errors_preserve_response_and_log_details() { )); // Seed an already registered command, without launching a real process. let provider_handle = CommandHandle("logging-command".to_string()); - session - .active_commands - .lock() - .await - .insert(provider_handle.0.clone(), provider_handle.clone()); + session.active_commands.lock().await.insert( + provider_handle.0.clone(), + Arc::new(ExternalProviderCommandState::new(provider_handle.clone())), + ); let handle = fixture .api .workdir_sessions @@ -720,7 +719,7 @@ async fn self_workdir_provider_refusals_log_operation_identity_without_request_t Reason::ScopedCapabilityDenied, ), ( - json!({"operation": "command_start", "request": {"command": secret, "timeout_secs": 1, "output_limit": 1024, "cwd": "", "tool_call_id": secret}}), + json!({"operation": "command_start", "request": {"command": secret, "timeout_secs": 1, "transport_byte_budget": 1048576, "cwd": "", "tool_call_id": secret}}), "command_start", "denied", Reason::ScopedCapabilityDenied, @@ -942,8 +941,8 @@ async fn self_workdir_post_start_registration_failure_does_not_claim_pre_dispatc serde_json::to_vec(&json!({ "target_workdir": "checkout", "operation": {"operation": "command_start", "request": { - "command": "body-secret", "timeout_secs": 1, "output_limit": 1024, - "cwd": "", "spill_dir": null, "tool_call_id": "body-secret" + "command": "body-secret", "timeout_secs": 1, "transport_byte_budget": 1048576, + "cwd": "", "tool_call_id": "body-secret" }} })) .unwrap(), @@ -1071,8 +1070,8 @@ async fn saturated_host_before_command_start_returns_availability_without_a_prov serde_json::to_vec(&json!({ "target_workdir": "checkout", "operation": {"operation": "command_start", "request": { - "command": "not-dispatched", "timeout_secs": 1, "output_limit": 1024, - "cwd": "", "spill_dir": null, "tool_call_id": "not-dispatched" + "command": "not-dispatched", "timeout_secs": 1, "transport_byte_budget": 1048576, + "cwd": "", "tool_call_id": "not-dispatched" }} })) .unwrap(), diff --git a/crates/workspace-server/src/server/tests/command_transport_tests.rs b/crates/workspace-server/src/server/tests/command_transport_tests.rs new file mode 100644 index 000000000..af9d6416b --- /dev/null +++ b/crates/workspace-server/src/server/tests/command_transport_tests.rs @@ -0,0 +1,223 @@ +//! Provider-bridge lifecycle tests. The production bridge runs against a controlled +//! provider boundary; the websocket/CLI-pump test in the parent module covers real byte transport. +use super::*; +use workdir::WorkdirSession; + +fn command_fixture() -> ( + Arc, + tokio::sync::mpsc::Receiver, +) { + let (sender, receiver) = tokio::sync::mpsc::channel(16); + let session = Arc::new(ExternalProviderWorkdirSession::new( + Arc::new(ExternalProviderConnection { + grant_id: "command-grant".into(), + workdir_id: "command-workdir".into(), + provider_instance_id: "command-provider".into(), + generation: 1, + expires_at: None, + capabilities: workdir::WorkdirSessionCapabilities::COMMAND_ONLY, + admission: Arc::new(tokio::sync::Semaphore::new(1)), + shutdown_confirmed: Arc::new(std::sync::atomic::AtomicBool::new(false)), + sender, + }), + None, + )); + (session, receiver) +} + +fn page(cursor: u64, bytes: &[u8], terminal: bool) -> workdir::CommandOutput { + workdir::CommandOutput { + chunks: if bytes.is_empty() { + Vec::new() + } else { + vec![workdir::CommandByteChunk { + sequence: cursor, + stream: workdir::CommandStream::Stdout, + offset: cursor, + bytes: bytes.to_vec(), + }] + }, + next_cursor: cursor + bytes.len() as u64, + terminal: terminal.then_some(workdir::CommandTerminal { + status: workdir::CommandStatus::Cancelled, + exit_code: None, + stdout_bytes: cursor + bytes.len() as u64, + stderr_bytes: 0, + stdout_delivered_bytes: cursor + bytes.len() as u64, + stderr_delivered_bytes: 0, + truncation_reason: None, + }), + } +} + +async fn receive( + receiver: &mut tokio::sync::mpsc::Receiver, +) -> ( + WorkdirSessionOperation, + tokio::sync::oneshot::Sender< + std::result::Result, + >, +) { + let command = tokio::time::timeout(std::time::Duration::from_secs(2), receiver.recv()) + .await + .unwrap() + .unwrap(); + let ExternalProviderCommand::Operation { + operation, + response, + .. + } = command + else { + panic!("expected provider operation") + }; + (operation, response) +} + +#[tokio::test] +async fn close_cancels_waiting_reader_then_drains_from_latest_cursor() { + let (session, mut receiver) = command_fixture(); + session.active_commands.lock().await.insert( + "external".into(), + Arc::new(ExternalProviderCommandState::new(CommandHandle( + "provider".into(), + ))), + ); + let reader = { + let session = session.clone(); + tokio::spawn(async move { + session + .command_output(workdir::CommandOutputRequest { + handle: CommandHandle("external".into()), + cursor: 0, + limit: 3, + wait: true, + }) + .await + }) + }; + let (_, response) = receive(&mut receiver).await; + response + .send(Ok(WorkdirSessionOperationResult::CommandOutput(page( + 0, b"one", false, + )))) + .unwrap(); + assert_eq!(reader.await.unwrap().unwrap().next_cursor, 3); + + let reader = { + let session = session.clone(); + tokio::spawn(async move { + session + .command_output(workdir::CommandOutputRequest { + handle: CommandHandle("external".into()), + cursor: 3, + limit: 3, + wait: true, + }) + .await + }) + }; + let (_, pending_read) = receive(&mut receiver).await; + let closer = { + let session = session.clone(); + tokio::spawn(async move { session.close().await }) + }; + let (operation, response) = receive(&mut receiver).await; + assert!( + matches!(operation, WorkdirSessionOperation::CommandCancel(ref handle) if handle.0 == "provider") + ); + response + .send(Ok(WorkdirSessionOperationResult::CommandCancel)) + .unwrap(); + pending_read + .send(Ok(WorkdirSessionOperationResult::CommandOutput(page( + 3, b"two", false, + )))) + .unwrap(); + assert_eq!(reader.await.unwrap().unwrap().next_cursor, 6); + + for (cursor, bytes, terminal) in [(6, &b"end"[..], false), (9, &b""[..], true)] { + let (operation, response) = receive(&mut receiver).await; + assert!( + matches!(operation, WorkdirSessionOperation::CommandOutput(ref request) if request.handle.0 == "provider" && request.cursor == cursor && request.wait) + ); + response + .send(Ok(WorkdirSessionOperationResult::CommandOutput(page( + cursor, bytes, terminal, + )))) + .unwrap(); + } + tokio::time::timeout(std::time::Duration::from_secs(2), closer) + .await + .unwrap() + .unwrap() + .unwrap(); +} + +#[tokio::test] +async fn terminal_output_retains_same_binding_for_current_page_replay() { + let (session, mut receiver) = command_fixture(); + session.active_commands.lock().await.insert( + "external".into(), + Arc::new(ExternalProviderCommandState::new(CommandHandle( + "provider".into(), + ))), + ); + for _ in 0..2 { + let reader = { + let session = session.clone(); + tokio::spawn(async move { + session + .command_output(workdir::CommandOutputRequest { + handle: CommandHandle("external".into()), + cursor: 0, + limit: 3, + wait: true, + }) + .await + }) + }; + let (operation, response) = receive(&mut receiver).await; + assert!( + matches!(operation, WorkdirSessionOperation::CommandOutput(ref request) if request.handle.0 == "provider" && request.cursor == 0) + ); + response + .send(Ok(WorkdirSessionOperationResult::CommandOutput(page( + 0, b"end", true, + )))) + .unwrap(); + assert_eq!(reader.await.unwrap().unwrap(), page(0, b"end", true)); + } + assert_eq!(session.active_commands.lock().await.len(), 1); +} + +#[tokio::test] +async fn terminal_registry_retention_is_bounded_and_excludes_active_commands() { + let (source, _) = command_fixture(); + let worker = RuntimeWorkerRef::new("runtime", "worker"); + let mut registry = WorkdirSessionRegistry::default(); + let active = registry.register_command( + worker.clone(), + "checkout".into(), + source.clone(), + CommandHandle("active".into()), + ); + let mut last = None; + for index in 0..MAX_RETAINED_TERMINAL_COMMANDS + 4 { + let handle = registry.register_command( + worker.clone(), + "checkout".into(), + source.clone(), + CommandHandle(format!("terminal-{index}")), + ); + registry.mark_terminal(&worker, &handle); + last = Some(handle); + } + assert_eq!(registry.active_command_count(&worker), 1); + assert_eq!(registry.commands.len(), MAX_RETAINED_TERMINAL_COMMANDS + 1); + assert!(registry.command(&worker, "checkout", &active).is_some()); + assert!( + registry + .command(&worker, "checkout", &last.unwrap()) + .is_some() + ); +} diff --git a/crates/workspace-server/src/server_assignment_attention_tests.rs b/crates/workspace-server/src/server_assignment_attention_tests.rs index 2f5c36ca8..ae622fe8b 100644 --- a/crates/workspace-server/src/server_assignment_attention_tests.rs +++ b/crates/workspace-server/src/server_assignment_attention_tests.rs @@ -466,7 +466,6 @@ impl AttentionWorkerFactory { worker.with_notification_coalesce_delay(std::time::Duration::from_millis(10)), worker::WorkerBootstrapLayout::RuntimeManagedRun { run_dir: root.join(uuid::Uuid::now_v7().to_string()), - bash_output_dir: root.join("bash-output"), }, worker::WorkerControllerTransport::InProcess, ) diff --git a/crates/workspace-server/src/server_assignment_delivery_tests.rs b/crates/workspace-server/src/server_assignment_delivery_tests.rs index 5d397814a..af1948942 100644 --- a/crates/workspace-server/src/server_assignment_delivery_tests.rs +++ b/crates/workspace-server/src/server_assignment_delivery_tests.rs @@ -799,7 +799,6 @@ impl DeliveryWorkerFactory { worker::WorkerFilesystemAuthority::None, worker::WorkerBootstrapLayout::RuntimeManagedRun { run_dir: root.join(uuid::Uuid::now_v7().to_string()), - bash_output_dir: root.join("bash-output"), }, worker::WorkerControllerTransport::InProcess, )