From 5588ae3648a46d19300a7d40df9f206a4aee3830 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 11 Sep 2026 13:18:02 -0600 Subject: [PATCH] Run the agent's MCP servers and the service listing through the driver's services facet The agent launched a sandbox MCP server with its own setsid wrapper and PID handling, polled ss for the port in a shell loop, and read a fixed log file for failures; the server listed a sandbox's services with its own ss and procfs scripts and parsers, and told the API which one it had used. The driver's services facet now does both: the agent spawns the server as a service, waits for the port, reads the service's logs on failure, and stops it on cancellation; the server lists the driver's listening ports, grouped by port with the process names the sandbox can give. The discovery source leaves the API and the web panel's iproute2 tip goes with it. Co-Authored-By: Claude Fable 5.1 --- Cargo.lock | 1 + .../run-sandbox/services-panel.test.tsx | 45 +-- .../app/routes/run-sandbox/services-panel.tsx | 13 - docs/public/api-reference/fabro-api.yaml | 27 +- lib/apps/fabro-server/src/demo/mod.rs | 8 +- .../src/server/handler/sandbox.rs | 354 +++--------------- lib/components/fabro-agent/Cargo.toml | 1 + lib/components/fabro-agent/src/session.rs | 246 ++++-------- .../fabro-sandbox/src/driver_sandbox.rs | 14 + lib/foundation/fabro-api/build.rs | 10 - .../tests/sandbox_services_round_trip.rs | 29 +- lib/foundation/fabro-types/src/lib.rs | 5 +- .../fabro-types/src/sandbox_services.rs | 13 - .../src/.openapi-generator/FILES | 2 - .../fabro-api-client/src/models/index.ts | 2 - .../sandbox-service-discovery-source.ts | 26 -- .../src/models/sandbox-service-list-meta.ts | 25 -- .../models/sandbox-service-list-response.ts | 4 - .../src/models/sandbox-service.ts | 6 +- 19 files changed, 156 insertions(+), 675 deletions(-) delete mode 100644 lib/packages/fabro-api-client/src/models/sandbox-service-discovery-source.ts delete mode 100644 lib/packages/fabro-api-client/src/models/sandbox-service-list-meta.ts diff --git a/Cargo.lock b/Cargo.lock index f70c0515b..bcc5170f0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2294,6 +2294,7 @@ dependencies = [ "libc", "lithos-llm", "paste", + "sandbox-driver", "sandbox-driver-testing", "serde", "serde_json", diff --git a/apps/fabro-web/app/routes/run-sandbox/services-panel.test.tsx b/apps/fabro-web/app/routes/run-sandbox/services-panel.test.tsx index 38403196e..a20c95a49 100644 --- a/apps/fabro-web/app/routes/run-sandbox/services-panel.test.tsx +++ b/apps/fabro-web/app/routes/run-sandbox/services-panel.test.tsx @@ -26,10 +26,7 @@ function makeIdlePreview(): PreviewMutationShape { } function makeServicesData(data: SandboxService[]) { - return { - data, - meta: { source: "ss" as const }, - }; + return { data }; } const mountedRenderers: TestRenderer.ReactTestRenderer[] = []; @@ -116,46 +113,6 @@ describe("ServicesPanelView", () => { expect(titles).toHaveLength(1); }); - test("shows an iproute2 tip when services were discovered from procfs", () => { - const service: SandboxService = { - port: 3000, - addresses: ["0.0.0.0:3000"], - processes: [], - preview_supported: true, - }; - const renderer = renderView({ - servicesQuery: { - ...makeIdleQuery(), - data: { - data: [service], - meta: { source: "procfs" }, - }, - }, - previewMutation: makeIdlePreview(), - }); - - const tipLabels = renderer.root.findAll( - (node) => - node.type === "span" && - Array.isArray(node.children) && - node.children.includes("Tip:"), - ); - expect(tipLabels).toHaveLength(1); - - const commands = renderer.root.findAll( - (node) => - node.type === "code" && - Array.isArray(node.children) && - node.children.includes("apt-get install iproute2"), - ); - expect(commands).toHaveLength(1); - - const tipText = JSON.stringify(renderer.toJSON()); - expect(tipText).toContain("Install "); - expect(tipText).toContain("ss"); - expect(tipText).toContain(" in the sandbox for improved services listing:"); - }); - test("shows API error state with the error message", () => { const renderer = renderView({ servicesQuery: { diff --git a/apps/fabro-web/app/routes/run-sandbox/services-panel.tsx b/apps/fabro-web/app/routes/run-sandbox/services-panel.tsx index 313cac6ba..400cb6365 100644 --- a/apps/fabro-web/app/routes/run-sandbox/services-panel.tsx +++ b/apps/fabro-web/app/routes/run-sandbox/services-panel.tsx @@ -77,7 +77,6 @@ export function ServicesPanelView({ const [previewError, setPreviewError] = useState(null); const services = servicesQuery.data?.data ?? []; - const discoverySource = servicesQuery.data?.meta.source; const queryErrorMessage = describeQueryError(servicesQuery.error); const showLoading = servicesQuery.isLoading && !servicesQuery.data; const showError = queryErrorMessage !== null && !servicesQuery.data; @@ -150,7 +149,6 @@ export function ServicesPanelView({ ) : ( <> - {discoverySource === "procfs" ? : null} - Tip:{" "} - Install ss in the sandbox - for improved services listing:{" "} - apt-get install iproute2 - - ); -} - function ServicesTable({ services, pendingPort, diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index cef7dc3be..f1647b3b2 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -14119,7 +14119,7 @@ components: $ref: "#/components/schemas/SandboxFileEntry" SandboxService: - description: A listening TCP service discovered inside a run sandbox. + description: A TCP port a process inside a run sandbox listens on, as the sandbox driver reports it. type: object required: - port @@ -14135,16 +14135,16 @@ components: example: 3000 addresses: type: array - description: Local bind addresses discovered from `ss` or `/proc/net/tcp*`. + description: Local bind addresses the sandbox reports for the port. items: type: string example: ["127.0.0.1:3000", "[::]:3000"] processes: type: array - description: Visible process summaries when available. Empty when the sandbox only supports `/proc/net/tcp*` discovery. + description: The listening processes, when the sandbox can name them (`node`, or `pid=1234`). Empty when it cannot. items: type: string - example: ['users:(("node",pid=42,fd=23))'] + example: ["node"] preview_supported: type: boolean description: Whether the provider supports an external preview URL for this port. @@ -14155,30 +14155,11 @@ components: type: object required: - data - - meta properties: data: type: array items: $ref: "#/components/schemas/SandboxService" - meta: - $ref: "#/components/schemas/SandboxServiceListMeta" - - SandboxServiceListMeta: - description: Metadata about sandbox service discovery. - type: object - required: - - source - properties: - source: - $ref: "#/components/schemas/SandboxServiceDiscoverySource" - - SandboxServiceDiscoverySource: - description: Tool or kernel interface used to discover sandbox services. - type: string - enum: - - ss - - procfs VncPreviewResponse: description: Response containing a signed noVNC preview URL for a Daytona sandbox. diff --git a/lib/apps/fabro-server/src/demo/mod.rs b/lib/apps/fabro-server/src/demo/mod.rs index 7d23d8c7a..4d890b591 100644 --- a/lib/apps/fabro-server/src/demo/mod.rs +++ b/lib/apps/fabro-server/src/demo/mod.rs @@ -23,7 +23,6 @@ use fabro_api::types::{ RunFilesMeta, RunFilesMetaScope, RunFilesMetaSource, SandboxService, SandboxServiceListResponse, }; -use fabro_types::{SandboxServiceDiscoverySource, SandboxServiceListMeta}; use serde_json::json; use crate::error::ApiError; @@ -405,19 +404,16 @@ pub(crate) async fn list_sandbox_services_stub( SandboxService { port: 3000, addresses: vec!["0.0.0.0:3000".to_string()], - processes: vec![r#"users:(("node",pid=42,fd=23))"#.to_string()], + processes: vec!["node".to_string()], preview_supported: true, }, SandboxService { port: 2500, addresses: vec!["127.0.0.1:2500".to_string()], - processes: vec![r#"users:(("debug",pid=84,fd=19))"#.to_string()], + processes: vec!["debug".to_string()], preview_supported: false, }, ], - meta: SandboxServiceListMeta { - source: SandboxServiceDiscoverySource::Ss, - }, }), ) .into_response() diff --git a/lib/apps/fabro-server/src/server/handler/sandbox.rs b/lib/apps/fabro-server/src/server/handler/sandbox.rs index af65b4f71..6e12be966 100644 --- a/lib/apps/fabro-server/src/server/handler/sandbox.rs +++ b/lib/apps/fabro-server/src/server/handler/sandbox.rs @@ -1,5 +1,4 @@ use std::collections::BTreeMap; -use std::net::{Ipv4Addr, Ipv6Addr}; use std::num::NonZeroU64; use std::sync::Arc; use std::time::Duration; @@ -8,11 +7,10 @@ use axum::extract::ws::{Message as WsMessage, WebSocket, WebSocketUpgrade}; use fabro_sandbox::{ FileKind, ProviderAccess, PtySize, RunSandbox, open_terminal_for_run, reconnect_driver_for_run, }; -use fabro_types::{ - RunSandboxInstance, SandboxProviderKind, SandboxServiceDiscoverySource, SandboxServiceListMeta, -}; +use fabro_types::{RunSandboxInstance, SandboxProviderKind}; use futures_util::FutureExt; use futures_util::future::BoxFuture; +use sandbox_driver::{ListeningPort, Services as _}; use super::super::{ ApiError, AppState, Bytes, HeaderMap, IntoResponse, Json, NamedTempFile, Path, @@ -28,20 +26,7 @@ const DEFAULT_VNC_NO_VNC_PORT: u16 = 6080; const DEFAULT_VNC_TTL_SECS: i32 = 3600; /// Header a Daytona unsigned preview needs; surfaced as the response token. const PREVIEW_TOKEN_HEADER: &str = "x-daytona-preview-token"; -const LIST_SANDBOX_SERVICES_COMMAND: &str = r#"if command -v ss >/dev/null 2>&1; then - ss -H -ltnp && exit 0 -fi -printf 'FABRO_PROC_NET_TCP procfs\n' -for file in /proc/net/tcp /proc/net/tcp6; do - if [ -r "$file" ]; then - printf 'FABRO_PROC_NET_TCP %s\n' "$file" - while IFS= read -r line; do - printf '%s\n' "$line" - done < "$file" - fi -done"#; -const LIST_SANDBOX_SERVICES_FAILURE_LABEL: &str = "sandbox service discovery command"; -const LIST_SANDBOX_SERVICES_TIMEOUT_MS: u64 = 5_000; +const LIST_SANDBOX_SERVICES_FAILURE_LABEL: &str = "sandbox service discovery"; // Daytona's signed preview points at the noVNC service root, which serves a // directory listing. Force the iframe to the actual viewer page with // autoconnect+scale so the user lands on the desktop, not a file index. @@ -586,145 +571,43 @@ async fn list_sandbox_services( Ok(sandbox) => sandbox, Err(response) => return response, }; - let result = match sandbox - .exec_command( - LIST_SANDBOX_SERVICES_COMMAND, - LIST_SANDBOX_SERVICES_TIMEOUT_MS, - None, - None, - None, - ) - .await - { - Ok(result) => result, + let services = match sandbox.services() { + Ok(services) => services, Err(err) => { - return ApiError::new(StatusCode::CONFLICT, err.display_with_causes()).into_response(); + return ApiError::new(StatusCode::NOT_IMPLEMENTED, err.display_with_causes()) + .into_response(); + } + }; + let ports = match services.listening_ports().await { + Ok(ports) => ports, + Err(err) => { + return ApiError::new( + StatusCode::CONFLICT, + format!("{LIST_SANDBOX_SERVICES_FAILURE_LABEL} failed: {err}"), + ) + .into_response(); } }; - if !result.success() { - return ApiError::new( - StatusCode::CONFLICT, - sandbox_service_command_failure_detail(&result), - ) - .into_response(); - } - - let discovery = parse_sandbox_services(&result.stdout_lossy(), &provider); Json(SandboxServiceListResponse { - data: discovery.services, - meta: SandboxServiceListMeta { - source: discovery.source, - }, + data: services_from_ports(ports, &provider), }) .into_response() } -fn sandbox_service_command_failure_detail(result: &fabro_sandbox::ExecResult) -> String { - let stderr = result.stderr_lossy(); - let stderr = stderr.trim(); - if !stderr.is_empty() { - return stderr.to_string(); - } - let stdout = result.stdout_lossy(); - let stdout = stdout.trim(); - if !stdout.is_empty() { - return stdout.to_string(); - } - format!("{LIST_SANDBOX_SERVICES_FAILURE_LABEL} failed") -} - -struct SandboxServiceDiscovery { - services: Vec, - source: SandboxServiceDiscoverySource, -} - -fn parse_sandbox_services(output: &str, provider: &SandboxProviderKind) -> SandboxServiceDiscovery { - if output - .lines() - .any(|line| line.trim_start().starts_with("FABRO_PROC_NET_TCP ")) - { - SandboxServiceDiscovery { - services: parse_proc_net_listening_services(output, provider), - source: SandboxServiceDiscoverySource::Procfs, - } - } else { - SandboxServiceDiscovery { - services: parse_ss_listening_services(output, provider), - source: SandboxServiceDiscoverySource::Ss, - } - } -} - -fn parse_ss_listening_services( - output: &str, +/// The driver's listeners grouped by port, previewable ports first. +fn services_from_ports( + ports: Vec, provider: &SandboxProviderKind, ) -> Vec { let mut services = BTreeMap::::new(); - for line in output - .lines() - .map(str::trim) - .filter(|line| !line.is_empty()) - { - let fields = line.split_whitespace().collect::>(); - let Some(address) = fields.get(3).copied() else { - continue; - }; - let Some(port) = parse_ss_local_port(address) else { - continue; - }; - let process = (fields.len() > 5).then(|| fields[5..].join(" ")); - push_service(&mut services, provider, port, address.to_string(), process); - } - sorted_services(services) -} - -fn parse_ss_local_port(address: &str) -> Option { - let port = address.rsplit_once(':')?.1.parse::().ok()?; - (port > 0).then_some(port) -} - -#[derive(Clone, Copy)] -enum ProcNetFamily { - Ipv4, - Ipv6, -} - -fn parse_proc_net_listening_services( - output: &str, - provider: &SandboxProviderKind, -) -> Vec { - let mut services = BTreeMap::::new(); - let mut family = None; - for line in output - .lines() - .map(str::trim) - .filter(|line| !line.is_empty()) - { - if let Some(path) = line.strip_prefix("FABRO_PROC_NET_TCP ") { - family = if path.ends_with("/tcp6") { - Some(ProcNetFamily::Ipv6) - } else { - Some(ProcNetFamily::Ipv4) - }; - continue; - } - if line.starts_with("sl") { - continue; - } - let Some(family) = family else { - continue; - }; - let fields = line.split_whitespace().collect::>(); - let (Some(local_address), Some(state)) = (fields.get(1), fields.get(3)) else { - continue; - }; - if *state != "0A" { - continue; - } - let Some((address, port)) = parse_proc_net_local_address(local_address, family) else { - continue; - }; - push_service(&mut services, provider, port, address, None); + for listener in ports { + push_service( + &mut services, + provider, + listener.port, + listener.address, + listener.process, + ); } sorted_services(services) } @@ -735,40 +618,6 @@ fn sorted_services(services: BTreeMap) -> Vec Option<(String, u16)> { - let (address_hex, port_hex) = value.split_once(':')?; - let port = u16::from_str_radix(port_hex, 16).ok()?; - if port == 0 { - return None; - } - let address = match family { - ProcNetFamily::Ipv4 => format!("{}:{port}", parse_proc_net_ipv4(address_hex)?), - ProcNetFamily::Ipv6 => format!("[{}]:{port}", parse_proc_net_ipv6(address_hex)?), - }; - Some((address, port)) -} - -fn parse_proc_net_ipv4(value: &str) -> Option { - if value.len() != 8 { - return None; - } - let raw = u32::from_str_radix(value, 16).ok()?; - Some(Ipv4Addr::from(raw.to_le_bytes())) -} - -fn parse_proc_net_ipv6(value: &str) -> Option { - if value.len() != 32 { - return None; - } - let mut bytes = [0_u8; 16]; - for (chunk_index, chunk) in value.as_bytes().chunks_exact(8).enumerate() { - let chunk = std::str::from_utf8(chunk).ok()?; - let raw = u32::from_str_radix(chunk, 16).ok()?; - bytes[chunk_index * 4..chunk_index * 4 + 4].copy_from_slice(&raw.to_le_bytes()); - } - Some(Ipv6Addr::from(bytes)) -} - fn push_service( services: &mut BTreeMap, provider: &SandboxProviderKind, @@ -927,8 +776,6 @@ async fn load_run_sandbox_instance( #[cfg(test)] mod tests { use axum::http::{HeaderMap, HeaderValue}; - use fabro_sandbox::Termination; - use fabro_sandbox::test_support::exec_result; use futures_util::FutureExt; use super::*; @@ -1017,104 +864,28 @@ mod tests { } #[test] - fn ss_parser_extracts_addresses_processes_and_preview_support() { - let services = parse_ss_listening_services( - r#" -LISTEN 0 4096 127.0.0.1:3000 0.0.0.0:* users:(("node",pid=42,fd=23)) -LISTEN 0 4096 0.0.0.0:5173 0.0.0.0:* users:(("vite",pid=84,fd=19)) -LISTEN 0 4096 [::]:8080 [::]:* users:(("server",pid=126,fd=9)) -LISTEN 0 4096 [::1]:2500 [::]:* users:(("debug",pid=168,fd=7)) -"#, - &SandboxProviderKind::DAYTONA, - ); - - assert_eq!(services.len(), 4); - assert_eq!(services[0].port, 3000); - assert_eq!(services[0].addresses, vec!["127.0.0.1:3000"]); - assert_eq!(services[0].processes, vec![ - r#"users:(("node",pid=42,fd=23))"# - ]); - assert!(services[0].preview_supported); - assert_eq!(services[1].port, 5173); - assert_eq!(services[1].addresses, vec!["0.0.0.0:5173"]); - assert!(services[1].preview_supported); - assert_eq!(services[2].port, 8080); - assert_eq!(services[2].addresses, vec!["[::]:8080"]); - assert!(services[2].preview_supported); - assert_eq!(services[3].port, 2500); - assert_eq!(services[3].addresses, vec!["[::1]:2500"]); - assert_eq!(services[3].processes, vec![ - r#"users:(("debug",pid=168,fd=7))"# - ]); - assert!(!services[3].preview_supported); - } - - #[test] - fn ss_parser_ignores_malformed_and_non_numeric_ports() { - let services = parse_ss_listening_services( - r#" -LISTEN 0 4096 127.0.0.1:not-a-port 0.0.0.0:* users:(("node",pid=42,fd=23)) -LISTEN 0 4096 missing-peer -not enough fields -LISTEN 0 4096 127.0.0.1:0 0.0.0.0:* users:(("zero",pid=1,fd=2)) -LISTEN 0 4096 127.0.0.1:65536 0.0.0.0:* users:(("large",pid=1,fd=2)) -"#, - &SandboxProviderKind::DAYTONA, - ); - - assert!(services.is_empty()); - } - - #[test] - fn ss_parser_groups_duplicate_ports_and_deduplicates_values() { - let services = parse_ss_listening_services( - r#" -LISTEN 0 4096 127.0.0.1:3000 0.0.0.0:* users:(("node",pid=42,fd=23)) -LISTEN 0 4096 0.0.0.0:3000 0.0.0.0:* users:(("node",pid=42,fd=23)) -LISTEN 0 4096 127.0.0.1:3000 0.0.0.0:* users:(("node",pid=42,fd=23)) -LISTEN 0 4096 [::]:3000 [::]:* users:(("vite",pid=84,fd=19)) -"#, - &SandboxProviderKind::DAYTONA, - ); - - assert_eq!(services, vec![SandboxService { - port: 3000, - addresses: vec![ - "127.0.0.1:3000".to_string(), - "0.0.0.0:3000".to_string(), - "[::]:3000".to_string(), + fn listening_ports_group_by_port_and_sort_previewable_first() { + let mut node = ListeningPort::new(3000, "127.0.0.1:3000"); + node.process = Some("node".to_string()); + let mut node_v6 = ListeningPort::new(3000, "[::]:3000"); + node_v6.process = Some("node".to_string()); + let mut debug = ListeningPort::new(2500, "[::1]:2500"); + debug.process = Some("pid=168".to_string()); + let services = services_from_ports( + vec![ + debug, + node, + node_v6, + ListeningPort::new(5173, "0.0.0.0:5173"), ], - processes: vec![ - r#"users:(("node",pid=42,fd=23))"#.to_string(), - r#"users:(("vite",pid=84,fd=19))"#.to_string(), - ], - preview_supported: true, - }]); - } - - #[test] - fn proc_net_parser_extracts_listening_tcp_services_without_processes() { - let discovery = parse_sandbox_services( - r" -FABRO_PROC_NET_TCP /proc/net/tcp - sl local_address rem_address st tx_queue rx_queue tr tm->when retrnsmt uid timeout inode - 0: 0100007F:0BB8 00000000:0000 0A 00000000:00000000 00:00000000 00000000 501 0 11111 - 1: 00000000:1435 00000000:0000 0A 00000000:00000000 00:00000000 00000000 501 0 22222 - 2: 0100007F:2328 00000000:0000 01 00000000:00000000 00:00000000 00000000 501 0 33333 -FABRO_PROC_NET_TCP /proc/net/tcp6 - sl local_address remote_address st tx_queue rx_queue tr tm->when retrnsmt uid timeout inode - 0: 00000000000000000000000000000000:1F90 00000000000000000000000000000000:0000 0A 00000000:00000000 00:00000000 00000000 501 0 44444 - 1: 00000000000000000000000001000000:09C4 00000000000000000000000000000000:0000 0A 00000000:00000000 00:00000000 00000000 501 0 55555 -", &SandboxProviderKind::DAYTONA, ); - assert_eq!(discovery.source, SandboxServiceDiscoverySource::Procfs); - assert_eq!(discovery.services, vec![ + assert_eq!(services, vec![ SandboxService { port: 3000, - addresses: vec!["127.0.0.1:3000".to_string()], - processes: vec![], + addresses: vec!["127.0.0.1:3000".to_string(), "[::]:3000".to_string()], + processes: vec!["node".to_string()], preview_supported: true, }, SandboxService { @@ -1123,16 +894,10 @@ FABRO_PROC_NET_TCP /proc/net/tcp6 processes: vec![], preview_supported: true, }, - SandboxService { - port: 8080, - addresses: vec!["[::]:8080".to_string()], - processes: vec![], - preview_supported: true, - }, SandboxService { port: 2500, addresses: vec!["[::1]:2500".to_string()], - processes: vec![], + processes: vec!["pid=168".to_string()], preview_supported: false, }, ]); @@ -1147,33 +912,6 @@ FABRO_PROC_NET_TCP /proc/net/tcp6 assert!(!preview_supported(&SandboxProviderKind::DOCKER, 3000)); } - #[test] - fn sandbox_service_command_failure_prefers_stderr_then_stdout() { - let mut result = exec_result( - "stdout detail", - "stderr detail", - Some(127), - Termination::Exited, - 10, - ); - assert_eq!( - sandbox_service_command_failure_detail(&result), - "stderr detail" - ); - - result.stderr.clear(); - assert_eq!( - sandbox_service_command_failure_detail(&result), - "stdout detail" - ); - - result.stdout.clear(); - assert_eq!( - sandbox_service_command_failure_detail(&result), - "sandbox service discovery command failed" - ); - } - struct FakeVncSandbox { error: Option<&'static str>, viewer_url: &'static str, diff --git a/lib/components/fabro-agent/Cargo.toml b/lib/components/fabro-agent/Cargo.toml index 53aa693ac..73d05998e 100644 --- a/lib/components/fabro-agent/Cargo.toml +++ b/lib/components/fabro-agent/Cargo.toml @@ -29,6 +29,7 @@ lithos-llm = { workspace = true, features = ["runtime"] } fabro-llm = { path = "../fabro-llm" } fabro-mcp = { path = "../fabro-mcp" } fabro-sandbox = { path = "../fabro-sandbox" } +sandbox-driver.workspace = true fabro-static.workspace = true fabro-template = { path = "../../foundation/fabro-template" } fabro-util = { path = "../../foundation/fabro-util" } diff --git a/lib/components/fabro-agent/src/session.rs b/lib/components/fabro-agent/src/session.rs index 152de768a..60a5d6975 100644 --- a/lib/components/fabro-agent/src/session.rs +++ b/lib/components/fabro-agent/src/session.rs @@ -22,6 +22,7 @@ use lithos_llm::types::{ ContentPart, Message as LlmMessage, ReasoningEffort, Role, Speed, TokenCounts, ToolCall, ToolChoice, }; +use sandbox_driver::{ServiceId, ServiceSpec, Services as _, ServicesFacet}; use tokio::sync::{Notify, broadcast}; use tokio::time; use tokio_util::sync::CancellationToken; @@ -852,14 +853,14 @@ impl Session { Ok(resolved) } - /// Start an MCP server inside the sandbox and return (url, headers) for - /// HTTP connection. + /// Start an MCP server inside the sandbox as a driver service and return + /// (url, headers) for HTTP connection. /// /// The outer `Result` surfaces fatal cancellation as - /// `Error::Interrupted(InterruptReason::Cancelled)` (the running MCP - /// process group is terminated before returning). The inner `Result` - /// captures non-fatal startup failures that the caller logs and turns - /// into an `McpServerFailed` event. + /// `Error::Interrupted(InterruptReason::Cancelled)` (a service already + /// started is stopped before returning). The inner `Result` captures + /// non-fatal startup failures that the caller logs and turns into an + /// `McpServerFailed` event. async fn start_sandbox_mcp_server( &self, command: &[String], @@ -868,82 +869,56 @@ impl Session { cancel_token: &CancellationToken, ) -> Result), String>, Error> { let sandbox = self.sandbox.as_ref(); - - let launch_script = sandbox_mcp_launch_script(command); - let env_ref = if env.is_empty() { None } else { Some(env) }; + let services = match sandbox.services() { + Ok(services) => services, + Err(error) => { + return Ok(Err(format!( + "Failed to launch MCP server: {}", + error.display_with_causes() + ))); + } + }; + let mut spec = ServiceSpec::new(mcp_service_command(command)); + for (key, value) in env { + spec = spec.env_var(key.clone(), value.clone()); + } if cancel_token.is_cancelled() { return Err(Error::Interrupted(InterruptReason::Cancelled)); } - let launch_result = match sandbox - .exec_command( - &launch_script, - 30_000, - None, - env_ref, - Some(cancel_token.child_token()), - ) - .await - { - Ok(result) => result, - Err(e) => { + let service = match services.spawn(&spec).await { + Ok(service) => service, + Err(error) => { if cancel_token.is_cancelled() { return Err(Error::Interrupted(InterruptReason::Cancelled)); } - return Ok(Err(format!( - "Failed to launch MCP server: {}", - e.display_with_causes() - ))); + return Ok(Err(format!("Failed to launch MCP server: {error}"))); } }; - - let pid = launch_result.stdout_lossy().trim().to_string(); - info!(pid = %pid, port, "MCP server process launched in sandbox"); - - // Wait for the server to start listening on the port - let poll_cmd = format!( - "for i in $(seq 1 30); do ss -tln | grep -q ':{port} ' && echo ready && exit 0; sleep 1; done; echo timeout" + info!( + service = service.as_str(), + port, "MCP server started as a sandbox service" ); - let poll_result = sandbox - .exec_command( - &poll_cmd, - 60_000, - None, - None, - Some(cancel_token.child_token()), - ) - .await; - if cancel_token.is_cancelled() { - kill_mcp_pid(sandbox, &pid).await; - return Err(Error::Interrupted(InterruptReason::Cancelled)); - } - - let poll_result = match poll_result { - Ok(result) => result, - Err(e) => { - return Ok(Err(format!( - "Failed to poll MCP server readiness: {}", - e.display_with_causes() - ))); + // Wait for the server to listen; a cancellation stops it. + let ready = tokio::select! { + () = cancel_token.cancelled() => { + stop_mcp_service(&services, &service).await; + return Err(Error::Interrupted(InterruptReason::Cancelled)); } + ready = services.wait_for_port(port, MCP_SERVER_READY_TIMEOUT) => ready, }; - - if poll_result.stdout_lossy().trim() != "ready" { - // Grab stderr for debugging - let stderr = sandbox - .exec_command( - "cat /tmp/mcp_server_stderr.log 2>/dev/null | tail -20", - 10_000, - None, - None, - Some(cancel_token.child_token()), - ) + if let Err(error) = ready { + let logs = services + .logs(&service, MCP_SERVER_LOG_TAIL_BYTES) .await - .map(|r| r.stdout_lossy()) + .map(|bytes| String::from_utf8_lossy(&bytes).into_owned()) .unwrap_or_default(); + stop_mcp_service(&services, &service).await; return Ok(Err(format!( - "MCP server did not start listening on port {port} within 30s. stderr:\n{stderr}" + "MCP server did not start listening on port {port} within {}s ({error}). \ + logs:\n{logs}", + MCP_SERVER_READY_TIMEOUT.as_secs() ))); } @@ -955,7 +930,7 @@ impl Session { }; if cancel_token.is_cancelled() { - kill_mcp_pid(sandbox, &pid).await; + stop_mcp_service(&services, &service).await; return Err(Error::Interrupted(InterruptReason::Cancelled)); } @@ -2192,49 +2167,31 @@ impl Session { } } -/// Build the script that launches a sandbox MCP server detached and echoes its -/// PID. +/// How long a sandbox MCP server gets to start listening on its port. +const MCP_SERVER_READY_TIMEOUT: Duration = Duration::from_secs(30); +/// How much of a failed MCP server's output the failure message carries. +const MCP_SERVER_LOG_TAIL_BYTES: usize = 4096; + +/// The Bash source a sandbox MCP server runs as a service. /// -/// `setsid` fully detaches the server so Daytona's exec doesn't block on it. -/// The inner command is shell-quoted for the wrapper so a single quote or -/// metacharacter in any argv element can't break out, and the wrapper itself is -/// the current `$BASH` because the sandbox evaluates this string as non-login -/// Bash and may resolve that executable outside `/bin` (for example on NixOS). -fn sandbox_mcp_launch_script(command: &[String]) -> String { - let command_source = match command { - // Sandbox MCP `script` entries resolve to this exact argv shape. The - // surrounding launcher is already the provider-selected Bash, so - // evaluate the source in that process instead of PATH-resolving a - // second interpreter. Grouping keeps the log redirections scoped to - // the whole script, including multi-command and trailing-comment - // forms. - [interpreter, flag, source] if interpreter == "bash" && flag == "-c" => { - format!("{{\n{source}\n}}") - } +/// Sandbox MCP `script` entries resolve to the argv shape `bash -c `. +/// The service already runs in the provider-selected Bash, so the source +/// runs there as it is instead of PATH-resolving a second interpreter (which +/// may live outside `/bin`, for example on NixOS). Any other argv is quoted +/// into one command line, so a quote or metacharacter in an element stays +/// inert. +fn mcp_service_command(command: &[String]) -> String { + match command { + [interpreter, flag, source] if interpreter == "bash" && flag == "-c" => source.clone(), _ => shell::shell_join(command), - }; - let inner = - format!("{command_source} > /tmp/mcp_server_stdout.log 2>/tmp/mcp_server_stderr.log"); - format!( - "setsid \"$BASH\" -c {quoted} /dev/null 2>&1 &\necho $!", - quoted = shell::shell_quote(&inner) - ) + } } -/// Best-effort kill of a sandbox MCP server process group. Used when -/// `start_sandbox_mcp_server` is cancelled after spawning a detached -/// `setsid` child but before reporting readiness. Errors from the sandbox -/// are logged and swallowed; the caller is already returning a Cancelled -/// error. -async fn kill_mcp_pid(sandbox: &RunSandbox, pid: &str) { - let pid = pid.trim(); - if pid.is_empty() { - return; - } - let script = - format!("kill -TERM -{pid} 2>/dev/null; sleep 1; kill -KILL -{pid} 2>/dev/null; true"); - if let Err(err) = sandbox.exec_command(&script, 5_000, None, None, None).await { - warn!(pid, error = %err.display_with_causes(), "Failed to kill MCP server process group during cancellation"); +/// Best-effort stop of a sandbox MCP service that will not be used: the +/// caller is already returning a cancellation or a startup failure. +async fn stop_mcp_service(services: &ServicesFacet<'_>, service: &ServiceId) { + if let Err(error) = services.stop(service).await { + warn!(service = service.as_str(), error = %error, "Failed to stop the MCP server service"); } } @@ -2267,82 +2224,33 @@ mod tests { use crate::tool_registry::{RegisteredTool, ToolContext, ToolRegistry, ToolSource}; #[test] - fn sandbox_mcp_launch_wrapper_uses_bash() { - // The sandbox evaluates this string as non-login Bash, so the detached - // wrapper reuses the executable selected by the provider. - let script = sandbox_mcp_launch_script(&[ - "npx".to_string(), - "@playwright/mcp@latest".to_string(), - "--port".to_string(), - "3100".to_string(), - ]); - - assert!( - script.starts_with("setsid \"$BASH\" -c "), - "launch wrapper should detach through the provider-selected Bash: {script}" - ); - assert!( - script.ends_with(" /dev/null 2>&1 &\necho $!"), - "launch wrapper should stay detached and report its PID: {script}" - ); - assert!( - script.contains("/tmp/mcp_server_stdout.log") - && script.contains("2>/tmp/mcp_server_stderr.log"), - "launch wrapper should keep its log redirection: {script}" - ); - } - - #[test] - fn sandbox_mcp_launch_wrapper_evaluates_scripts_in_the_selected_bash() { + fn mcp_service_command_runs_script_entries_in_the_service_bash() { let source = "PATH=/mcp-only\nprintf 'starting server\\n'\nexec my-server --port 3100 # ready"; - let script = - sandbox_mcp_launch_script(&["bash".to_string(), "-c".to_string(), source.to_string()]); - - let wrapper_argument = script - .strip_prefix("setsid \"$BASH\" -c ") - .and_then(|rest| rest.strip_suffix(" /dev/null 2>&1 &\necho $!")) - .expect("launch wrapper should have the canonical shape"); - let unwrapped = shlex::split(wrapper_argument).expect("wrapper argument should parse"); - - assert_eq!(unwrapped, vec![format!( - "{{\n{source}\n}} > /tmp/mcp_server_stdout.log 2>/tmp/mcp_server_stderr.log" - )]); - assert!( - !unwrapped[0].contains("bash -c"), - "script entries must not PATH-resolve a nested Bash: {}", - unwrapped[0] + let command = + mcp_service_command(&["bash".to_string(), "-c".to_string(), source.to_string()]); + assert_eq!( + command, source, + "script entries must not PATH-resolve a nested Bash" ); } #[test] - fn sandbox_mcp_launch_wrapper_quotes_arbitrary_argv() { - // A quote or metacharacter in any argv element must not break out of - // the wrapper; it has to arrive as one argument. - let script = sandbox_mcp_launch_script(&[ + fn mcp_service_command_quotes_arbitrary_argv() { + // A quote or metacharacter in any argv element must not break out + // of the command line; it has to arrive as one argument. + let command = mcp_service_command(&[ "my-server".to_string(), "--flag=it's a value".to_string(), "$(touch /tmp/pwned)".to_string(), ]); - - let wrapper_argument = script - .strip_prefix("setsid \"$BASH\" -c ") - .and_then(|rest| rest.strip_suffix(" /dev/null 2>&1 &\necho $!")) - .expect("launch wrapper should have the canonical shape"); - - // Unwrap the wrapper's own quoting: the whole inner script must arrive - // as one argument to `bash -c`, with each argv element still quoted so - // the substitution stays inert. - let unwrapped = shlex::split(wrapper_argument).expect("wrapper argument should parse"); assert_eq!( - unwrapped.len(), - 1, - "the command must stay a single argument" + command, + "my-server \"--flag=it's a value\" '$(touch /tmp/pwned)'" ); assert_eq!( - unwrapped[0], - "my-server \"--flag=it's a value\" '$(touch /tmp/pwned)' > \ - /tmp/mcp_server_stdout.log 2>/tmp/mcp_server_stderr.log" + mcp_service_command(&["npx".to_string(), "@playwright/mcp@latest".to_string()]), + "npx @playwright/mcp@latest" ); } diff --git a/lib/components/fabro-sandbox/src/driver_sandbox.rs b/lib/components/fabro-sandbox/src/driver_sandbox.rs index 7daa4dc92..d66e76c12 100644 --- a/lib/components/fabro-sandbox/src/driver_sandbox.rs +++ b/lib/components/fabro-sandbox/src/driver_sandbox.rs @@ -438,6 +438,20 @@ impl RunSandbox { }) } + /// The driver's services facet for this sandbox: background processes + /// that outlive their exec (the agent's MCP servers, dev servers), the + /// wait for a port to answer, and the list of listeners. Absent until a + /// pending sandbox is initialized, or when the provider has no + /// services. + pub fn services(&self) -> crate::Result> { + self.handle()?.services().ok_or_else(|| { + crate::Error::message(format!( + "sandbox provider `{}` does not support background services", + self.kind + )) + }) + } + fn search(&self) -> crate::Result> { self.handle()?.search().ok_or_else(|| { crate::Error::message(format!( diff --git a/lib/foundation/fabro-api/build.rs b/lib/foundation/fabro-api/build.rs index a05845c2b..df08c012c 100644 --- a/lib/foundation/fabro-api/build.rs +++ b/lib/foundation/fabro-api/build.rs @@ -671,16 +671,6 @@ fn main() { &[], ), ("SandboxService", "fabro_types::SandboxService", &[]), - ( - "SandboxServiceDiscoverySource", - "fabro_types::SandboxServiceDiscoverySource", - &[], - ), - ( - "SandboxServiceListMeta", - "fabro_types::SandboxServiceListMeta", - &[], - ), ( "SandboxServiceListResponse", "fabro_types::SandboxServiceListResponse", diff --git a/lib/foundation/fabro-api/tests/sandbox_services_round_trip.rs b/lib/foundation/fabro-api/tests/sandbox_services_round_trip.rs index 706ef59f2..4c62059ec 100644 --- a/lib/foundation/fabro-api/tests/sandbox_services_round_trip.rs +++ b/lib/foundation/fabro-api/tests/sandbox_services_round_trip.rs @@ -4,10 +4,7 @@ use fabro_api::types::{ SandboxService as ApiSandboxService, SandboxServiceListResponse as ApiSandboxServiceListResponse, }; -use fabro_types::{ - SandboxService, SandboxServiceDiscoverySource, SandboxServiceListMeta, - SandboxServiceListResponse, -}; +use fabro_types::{SandboxService, SandboxServiceListResponse}; use serde_json::json; #[test] @@ -22,15 +19,9 @@ fn sandbox_services_json_matches_openapi_shape() { data: vec![SandboxService { port: 3000, addresses: vec!["127.0.0.1:3000".to_string(), "[::]:3000".to_string()], - processes: vec![ - r#"users:(("node",pid=42,fd=23))"#.to_string(), - r#"users:(("vite",pid=84,fd=19))"#.to_string(), - ], + processes: vec!["node".to_string()], preview_supported: true, }], - meta: SandboxServiceListMeta { - source: SandboxServiceDiscoverySource::Ss, - }, }; assert_eq!( @@ -39,27 +30,19 @@ fn sandbox_services_json_matches_openapi_shape() { "data": [{ "port": 3000, "addresses": ["127.0.0.1:3000", "[::]:3000"], - "processes": [ - r#"users:(("node",pid=42,fd=23))"#, - r#"users:(("vite",pid=84,fd=19))"#, - ], + "processes": ["node"], "preview_supported": true - }], - "meta": { - "source": "ss" - } + }] }) ); } #[test] fn sandbox_services_deserializes_empty_response() { - let response: SandboxServiceListResponse = - serde_json::from_value(json!({ "data": [], "meta": { "source": "procfs" } })) - .expect("empty service response should deserialize"); + let response: SandboxServiceListResponse = serde_json::from_value(json!({ "data": [] })) + .expect("empty service response should deserialize"); assert!(response.data.is_empty()); - assert_eq!(response.meta.source, SandboxServiceDiscoverySource::Procfs); } fn assert_same_type() { diff --git a/lib/foundation/fabro-types/src/lib.rs b/lib/foundation/fabro-types/src/lib.rs index 0926cbdfd..d6fb7b9b6 100644 --- a/lib/foundation/fabro-types/src/lib.rs +++ b/lib/foundation/fabro-types/src/lib.rs @@ -164,10 +164,7 @@ pub use sandbox_inventory::{ pub use sandbox_provider::{ BundledProvider, InvalidSandboxProviderKind, SandboxProviderKind, WorkspacePolicy, }; -pub use sandbox_services::{ - SandboxService, SandboxServiceDiscoverySource, SandboxServiceListMeta, - SandboxServiceListResponse, -}; +pub use sandbox_services::{SandboxService, SandboxServiceListResponse}; pub use secret::{OAuthConfig, OAuthCredential, OAuthTokens, SecretMetadata, SecretType}; pub use session::{ PermissionLevel, SessionDetail, SessionId, SessionMessage, SessionRecord, SessionStatus, diff --git a/lib/foundation/fabro-types/src/sandbox_services.rs b/lib/foundation/fabro-types/src/sandbox_services.rs index 48f61be32..7212c174b 100644 --- a/lib/foundation/fabro-types/src/sandbox_services.rs +++ b/lib/foundation/fabro-types/src/sandbox_services.rs @@ -1,15 +1,3 @@ -#[derive(Debug, Clone, Copy, PartialEq, Eq, serde::Serialize, serde::Deserialize)] -#[serde(rename_all = "snake_case")] -pub enum SandboxServiceDiscoverySource { - Ss, - Procfs, -} - -#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)] -pub struct SandboxServiceListMeta { - pub source: SandboxServiceDiscoverySource, -} - #[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)] pub struct SandboxService { pub port: u16, @@ -21,5 +9,4 @@ pub struct SandboxService { #[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)] pub struct SandboxServiceListResponse { pub data: Vec, - pub meta: SandboxServiceListMeta, } diff --git a/lib/packages/fabro-api-client/src/.openapi-generator/FILES b/lib/packages/fabro-api-client/src/.openapi-generator/FILES index 664fc9e7e..7f6af8f50 100644 --- a/lib/packages/fabro-api-client/src/.openapi-generator/FILES +++ b/lib/packages/fabro-api-client/src/.openapi-generator/FILES @@ -443,8 +443,6 @@ models/sandbox-network-policy.ts models/sandbox-plugin-settings.ts models/sandbox-provider-lookup-error.ts models/sandbox-resources.ts -models/sandbox-service-discovery-source.ts -models/sandbox-service-list-meta.ts models/sandbox-service-list-response.ts models/sandbox-service.ts models/sandbox-state.ts diff --git a/lib/packages/fabro-api-client/src/models/index.ts b/lib/packages/fabro-api-client/src/models/index.ts index c407a60b6..669a9c8a8 100644 --- a/lib/packages/fabro-api-client/src/models/index.ts +++ b/lib/packages/fabro-api-client/src/models/index.ts @@ -414,8 +414,6 @@ export * from './sandbox-plugin-settings'; export * from './sandbox-provider-lookup-error'; export * from './sandbox-resources'; export * from './sandbox-service'; -export * from './sandbox-service-discovery-source'; -export * from './sandbox-service-list-meta'; export * from './sandbox-service-list-response'; export * from './sandbox-state'; export * from './sandbox-status'; diff --git a/lib/packages/fabro-api-client/src/models/sandbox-service-discovery-source.ts b/lib/packages/fabro-api-client/src/models/sandbox-service-discovery-source.ts deleted file mode 100644 index 1c6db31fe..000000000 --- a/lib/packages/fabro-api-client/src/models/sandbox-service-discovery-source.ts +++ /dev/null @@ -1,26 +0,0 @@ -/* tslint:disable */ -/* eslint-disable */ -/** - * Fabro Run API - * HTTP API for managing Fabro workflow run executions. - * - * The version of the OpenAPI document: 0.2.0 - * - * - * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech). - * https://openapi-generator.tech - * Do not edit the class manually. - */ - - - -/** - * Tool or kernel interface used to discover sandbox services. - */ - -export const SandboxServiceDiscoverySource = { - SS: 'ss', - PROCFS: 'procfs' -} as const; - -export type SandboxServiceDiscoverySource = typeof SandboxServiceDiscoverySource[keyof typeof SandboxServiceDiscoverySource]; diff --git a/lib/packages/fabro-api-client/src/models/sandbox-service-list-meta.ts b/lib/packages/fabro-api-client/src/models/sandbox-service-list-meta.ts deleted file mode 100644 index c24d6c81d..000000000 --- a/lib/packages/fabro-api-client/src/models/sandbox-service-list-meta.ts +++ /dev/null @@ -1,25 +0,0 @@ -/* tslint:disable */ -/* eslint-disable */ -/** - * Fabro Run API - * HTTP API for managing Fabro workflow run executions. - * - * The version of the OpenAPI document: 0.2.0 - * - * - * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech). - * https://openapi-generator.tech - * Do not edit the class manually. - */ - - -// May contain unused imports in some cases -// @ts-ignore -import type { SandboxServiceDiscoverySource } from './sandbox-service-discovery-source'; - -/** - * Metadata about sandbox service discovery. - */ -export interface SandboxServiceListMeta { - 'source': SandboxServiceDiscoverySource; -} diff --git a/lib/packages/fabro-api-client/src/models/sandbox-service-list-response.ts b/lib/packages/fabro-api-client/src/models/sandbox-service-list-response.ts index a41676b3b..e68ad21df 100644 --- a/lib/packages/fabro-api-client/src/models/sandbox-service-list-response.ts +++ b/lib/packages/fabro-api-client/src/models/sandbox-service-list-response.ts @@ -16,14 +16,10 @@ // May contain unused imports in some cases // @ts-ignore import type { SandboxService } from './sandbox-service'; -// May contain unused imports in some cases -// @ts-ignore -import type { SandboxServiceListMeta } from './sandbox-service-list-meta'; /** * Non-paginated list of listening TCP services in a run sandbox. */ export interface SandboxServiceListResponse { 'data': Array; - 'meta': SandboxServiceListMeta; } diff --git a/lib/packages/fabro-api-client/src/models/sandbox-service.ts b/lib/packages/fabro-api-client/src/models/sandbox-service.ts index 9e89797e7..d0089d47c 100644 --- a/lib/packages/fabro-api-client/src/models/sandbox-service.ts +++ b/lib/packages/fabro-api-client/src/models/sandbox-service.ts @@ -15,7 +15,7 @@ /** - * A listening TCP service discovered inside a run sandbox. + * A TCP port a process inside a run sandbox listens on, as the sandbox driver reports it. */ export interface SandboxService { /** @@ -23,11 +23,11 @@ export interface SandboxService { */ 'port': number; /** - * Local bind addresses discovered from `ss` or `/proc/net/tcp*`. + * Local bind addresses the sandbox reports for the port. */ 'addresses': Array; /** - * Visible process summaries when available. Empty when the sandbox only supports `/proc/net/tcp*` discovery. + * The listening processes, when the sandbox can name them (`node`, or `pid=1234`). Empty when it cannot. */ 'processes': Array; /**