From 5b035d3f75f15206eaf972a7ae3d95c614d93e6b Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 9 Sep 2026 20:13:50 -0600 Subject: [PATCH] Name plugins by their configured kind and drop out-of-process bundled kinds sandbox-driver PR #9 removes the check that a plugin's declared kind match the configured one: an operator who configures a path and pins its checksum has already chosen the executable, so the configured kind is fabro's name for whatever it serves. With that in the driver, fabro no longer needs plugin settings on a bundled kind to reach Docker over the wire. Bundled kinds reject plugin keys again, `connect_provider` links a bundled kind in-process and launches everything else, and the CLI scenarios run the Docker executable under the non-bundled `docker-plugin` kind. The driver pin moves to the PR head until it merges. Co-Authored-By: Claude Fable 5.1 --- Cargo.lock | 14 ++--- Cargo.toml | 17 +++--- .../administration/server-configuration.mdx | 8 +-- lib/apps/fabro-cli/tests/it/workflow/mod.rs | 4 +- .../fabro-cli/tests/it/workflow/plugin.rs | 12 ++-- lib/components/fabro-sandbox/src/driver.rs | 26 ++++---- lib/components/fabro-sandbox/src/reconnect.rs | 11 +--- .../fabro-sandbox/tests/plugin_provider.rs | 20 +++--- .../fabro-config/src/resolve/server.rs | 61 +++++++++++-------- .../fabro-config/src/tests/resolve_server.rs | 30 ++------- 10 files changed, 95 insertions(+), 108 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d2922c3e1..7a1060669 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7008,7 +7008,7 @@ dependencies = [ [[package]] name = "sandbox-driver" version = "0.1.0" -source = "git+https://github.com/lithoscomputer/sandbox-driver?rev=b9d07cf3ef1861173f8134498e2115b041f00b7d#b9d07cf3ef1861173f8134498e2115b041f00b7d" +source = "git+https://github.com/lithoscomputer/sandbox-driver?rev=e01d786a26a62291bb3716eca968d9c8838ad551#e01d786a26a62291bb3716eca968d9c8838ad551" dependencies = [ "async-trait", "globset", @@ -7024,7 +7024,7 @@ dependencies = [ [[package]] name = "sandbox-driver-daytona" version = "0.1.0" -source = "git+https://github.com/lithoscomputer/sandbox-driver?rev=b9d07cf3ef1861173f8134498e2115b041f00b7d#b9d07cf3ef1861173f8134498e2115b041f00b7d" +source = "git+https://github.com/lithoscomputer/sandbox-driver?rev=e01d786a26a62291bb3716eca968d9c8838ad551#e01d786a26a62291bb3716eca968d9c8838ad551" dependencies = [ "anyhow", "async-trait", @@ -7049,7 +7049,7 @@ dependencies = [ [[package]] name = "sandbox-driver-daytona-config" version = "0.1.0" -source = "git+https://github.com/lithoscomputer/sandbox-driver?rev=b9d07cf3ef1861173f8134498e2115b041f00b7d#b9d07cf3ef1861173f8134498e2115b041f00b7d" +source = "git+https://github.com/lithoscomputer/sandbox-driver?rev=e01d786a26a62291bb3716eca968d9c8838ad551#e01d786a26a62291bb3716eca968d9c8838ad551" dependencies = [ "sandbox-driver-docker-config", "serde", @@ -7059,7 +7059,7 @@ dependencies = [ [[package]] name = "sandbox-driver-docker" version = "0.1.0" -source = "git+https://github.com/lithoscomputer/sandbox-driver?rev=b9d07cf3ef1861173f8134498e2115b041f00b7d#b9d07cf3ef1861173f8134498e2115b041f00b7d" +source = "git+https://github.com/lithoscomputer/sandbox-driver?rev=e01d786a26a62291bb3716eca968d9c8838ad551#e01d786a26a62291bb3716eca968d9c8838ad551" dependencies = [ "anyhow", "async-trait", @@ -7080,7 +7080,7 @@ dependencies = [ [[package]] name = "sandbox-driver-docker-config" version = "0.1.0" -source = "git+https://github.com/lithoscomputer/sandbox-driver?rev=b9d07cf3ef1861173f8134498e2115b041f00b7d#b9d07cf3ef1861173f8134498e2115b041f00b7d" +source = "git+https://github.com/lithoscomputer/sandbox-driver?rev=e01d786a26a62291bb3716eca968d9c8838ad551#e01d786a26a62291bb3716eca968d9c8838ad551" dependencies = [ "serde", "serde_json", @@ -7089,7 +7089,7 @@ dependencies = [ [[package]] name = "sandbox-driver-host" version = "0.1.0" -source = "git+https://github.com/lithoscomputer/sandbox-driver?rev=b9d07cf3ef1861173f8134498e2115b041f00b7d#b9d07cf3ef1861173f8134498e2115b041f00b7d" +source = "git+https://github.com/lithoscomputer/sandbox-driver?rev=e01d786a26a62291bb3716eca968d9c8838ad551#e01d786a26a62291bb3716eca968d9c8838ad551" dependencies = [ "anyhow", "async-trait", @@ -7107,7 +7107,7 @@ dependencies = [ [[package]] name = "sandbox-driver-protocol" version = "0.1.0" -source = "git+https://github.com/lithoscomputer/sandbox-driver?rev=b9d07cf3ef1861173f8134498e2115b041f00b7d#b9d07cf3ef1861173f8134498e2115b041f00b7d" +source = "git+https://github.com/lithoscomputer/sandbox-driver?rev=e01d786a26a62291bb3716eca968d9c8838ad551#e01d786a26a62291bb3716eca968d9c8838ad551" dependencies = [ "async-trait", "base64", diff --git a/Cargo.toml b/Cargo.toml index 430e334c3..ccb2cc771 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -97,14 +97,15 @@ tokio-tungstenite = { version = "0.26", features = ["rustls-tls-webpki-roots"] } futures-util = "0.3" # sandbox-driver: the sandbox provider layer. Bundled Host, Docker, and # Daytona providers link in-process; third-party providers run as stdio -# plugins through sandbox-driver-protocol. Pinned by rev like the Daytona SDK. -sandbox-driver = { git = "https://github.com/lithoscomputer/sandbox-driver", rev = "b9d07cf3ef1861173f8134498e2115b041f00b7d" } -sandbox-driver-protocol = { git = "https://github.com/lithoscomputer/sandbox-driver", rev = "b9d07cf3ef1861173f8134498e2115b041f00b7d" } -sandbox-driver-host = { git = "https://github.com/lithoscomputer/sandbox-driver", rev = "b9d07cf3ef1861173f8134498e2115b041f00b7d" } -sandbox-driver-docker = { git = "https://github.com/lithoscomputer/sandbox-driver", rev = "b9d07cf3ef1861173f8134498e2115b041f00b7d" } -sandbox-driver-docker-config = { git = "https://github.com/lithoscomputer/sandbox-driver", rev = "b9d07cf3ef1861173f8134498e2115b041f00b7d" } -sandbox-driver-daytona = { git = "https://github.com/lithoscomputer/sandbox-driver", rev = "b9d07cf3ef1861173f8134498e2115b041f00b7d" } -sandbox-driver-daytona-config = { git = "https://github.com/lithoscomputer/sandbox-driver", rev = "b9d07cf3ef1861173f8134498e2115b041f00b7d" } +# plugins through sandbox-driver-protocol. Pinned by rev; currently the head of +# sandbox-driver PR #9 (trust the configured plugin kind), to move to main on merge. +sandbox-driver = { git = "https://github.com/lithoscomputer/sandbox-driver", rev = "e01d786a26a62291bb3716eca968d9c8838ad551" } +sandbox-driver-protocol = { git = "https://github.com/lithoscomputer/sandbox-driver", rev = "e01d786a26a62291bb3716eca968d9c8838ad551" } +sandbox-driver-host = { git = "https://github.com/lithoscomputer/sandbox-driver", rev = "e01d786a26a62291bb3716eca968d9c8838ad551" } +sandbox-driver-docker = { git = "https://github.com/lithoscomputer/sandbox-driver", rev = "e01d786a26a62291bb3716eca968d9c8838ad551" } +sandbox-driver-docker-config = { git = "https://github.com/lithoscomputer/sandbox-driver", rev = "e01d786a26a62291bb3716eca968d9c8838ad551" } +sandbox-driver-daytona = { git = "https://github.com/lithoscomputer/sandbox-driver", rev = "e01d786a26a62291bb3716eca968d9c8838ad551" } +sandbox-driver-daytona-config = { git = "https://github.com/lithoscomputer/sandbox-driver", rev = "e01d786a26a62291bb3716eca968d9c8838ad551" } sentry = { version = "0.35", default-features = false, features = ["backtrace", "contexts", "ureq", "rustls"] } fork = "0.2" exec = "0.3" diff --git a/docs/public/administration/server-configuration.mdx b/docs/public/administration/server-configuration.mdx index 8adb4362c..9c0f53508 100644 --- a/docs/public/administration/server-configuration.mdx +++ b/docs/public/administration/server-configuration.mdx @@ -201,12 +201,8 @@ enabled = true Any other key names a [sandbox-driver](https://github.com/lithoscomputer/sandbox-driver) plugin: an executable that speaks the sandbox-driver JSON-RPC protocol on stdin and stdout. The kind must be lowercase ASCII letters, digits, and interior hyphens. The plugin starts with a scrubbed -environment: only `env` and the ambient variables listed in `inherit_env` reach it. - -A bundled provider accepts the same plugin keys. Setting any of them runs that provider out of -process through the driver's executable for the kind (`fabro-sandbox-docker` for `docker`), which -isolates the server from provider crashes at the cost of a process per connection. Without them -the bundled provider links in-process. +environment: only `env` and the ambient variables listed in `inherit_env` reach it. Bundled +providers reject these plugin keys. ```toml title="settings.toml" [server.sandbox.providers.e2b] diff --git a/lib/apps/fabro-cli/tests/it/workflow/mod.rs b/lib/apps/fabro-cli/tests/it/workflow/mod.rs index 7ea94125a..ba180288c 100644 --- a/lib/apps/fabro-cli/tests/it/workflow/mod.rs +++ b/lib/apps/fabro-cli/tests/it/workflow/mod.rs @@ -174,8 +174,8 @@ fn run_events(run_dir: &Path) -> Vec { /// - `daytona`: the bundled Daytona provider, live credentials required. /// - `host-plugin`: the driver's Host executable over stdio under the /// non-bundled `host` kind, a clone-based managed workspace. -/// - `docker-plugin`: the bundled `docker` kind served out of process by the -/// driver's Docker executable. +/// - `docker-plugin`: the driver's Docker executable over stdio under the +/// non-bundled `docker-plugin` kind. /// /// The plugin variants need the executables `cargo` builds for /// `fabro-sandbox`; without them (or without a Docker daemon) they skip, diff --git a/lib/apps/fabro-cli/tests/it/workflow/plugin.rs b/lib/apps/fabro-cli/tests/it/workflow/plugin.rs index 26d26c484..907b35c9e 100644 --- a/lib/apps/fabro-cli/tests/it/workflow/plugin.rs +++ b/lib/apps/fabro-cli/tests/it/workflow/plugin.rs @@ -2,7 +2,9 @@ //! workflow scenarios. //! //! The executables come from the `fabro-sandbox` package's `[[bin]]` targets, -//! which `cargo` places beside the `fabro` binary under test. A scenario +//! which `cargo` places beside the `fabro` binary under test. Each runs under +//! a kind of the scenario's choosing (`host`, `docker-plugin`): the configured +//! kind names the plugin, whatever the executable declares. A scenario //! configured here runs against its own server so the plugin settings and the //! environment it creates never leak into the shared session server. @@ -32,8 +34,8 @@ const DOCKER_IMAGE: &str = "buildpack-deps:noble"; pub(crate) enum Plugin { /// The driver's Host executable under the non-bundled `host` kind. Host, - /// The driver's Docker executable serving the bundled `docker` kind out - /// of process. + /// The driver's Docker executable under the non-bundled `docker-plugin` + /// kind: the same containers, reached over stdio. Docker, } @@ -41,7 +43,7 @@ impl Plugin { fn kind(self) -> &'static str { match self { Self::Host => "host", - Self::Docker => "docker", + Self::Docker => "docker-plugin", } } @@ -117,7 +119,7 @@ root = "{storage}" [server.auth] methods = ["dev-token"] -[server.sandbox.providers.docker] +[server.sandbox.providers.docker-plugin] path = "{path}" dev = true inherit_env = ["PATH", "HOME", "DOCKER_HOST", "DOCKER_CERT_PATH", "DOCKER_TLS_VERIFY"] diff --git a/lib/components/fabro-sandbox/src/driver.rs b/lib/components/fabro-sandbox/src/driver.rs index d6b54df04..12fb872d8 100644 --- a/lib/components/fabro-sandbox/src/driver.rs +++ b/lib/components/fabro-sandbox/src/driver.rs @@ -154,12 +154,12 @@ pub enum ConnectError { /// Connects the provider behind `kind`. /// -/// Bundled kinds return the in-process driver provider unless their entry -/// names a plugin executable, in which case the same kind is served out of -/// process. Any other kind launches the plugin named by `settings.plugin`. -/// A plugin returns a supervised handle that relaunches it after a crash -/// for new work only. Disabled entries are refused here so no caller has to -/// remember the policy check. +/// Bundled kinds return the in-process driver provider. Any other kind +/// launches the plugin named by `settings.plugin` and returns a supervised +/// handle that relaunches it after a crash for new work only. The +/// configured kind is fabro's name for whatever the executable serves; the +/// kind the plugin declares is not compared against it. Disabled entries +/// are refused here so no caller has to remember the policy check. pub async fn connect_provider( kind: &SandboxProviderKind, settings: &ServerSandboxProviderSettings, @@ -168,12 +168,6 @@ pub async fn connect_provider( if !settings.enabled { return Err(ConnectError::Disabled { kind: kind.clone() }); } - if let Some(plugin) = &settings.plugin { - return Ok(ConnectedProvider { - kind: kind.clone(), - provider: Arc::new(PluginBackedProvider::launch(kind, plugin).await?), - }); - } let driver = |source| ConnectError::Driver { kind: kind.clone(), source, @@ -208,7 +202,13 @@ pub async fn connect_provider( .map_err(driver)?, ) } - None => return Err(ConnectError::MissingPluginSettings { kind: kind.clone() }), + None => { + let plugin = settings + .plugin + .as_ref() + .ok_or_else(|| ConnectError::MissingPluginSettings { kind: kind.clone() })?; + Arc::new(PluginBackedProvider::launch(kind, plugin).await?) + } }; Ok(ConnectedProvider { kind: kind.clone(), diff --git a/lib/components/fabro-sandbox/src/reconnect.rs b/lib/components/fabro-sandbox/src/reconnect.rs index ce7d2162e..2eb655ac1 100644 --- a/lib/components/fabro-sandbox/src/reconnect.rs +++ b/lib/components/fabro-sandbox/src/reconnect.rs @@ -46,14 +46,7 @@ pub async fn reconnect_driver_for_run( event_callback: Option, ) -> Result { let runtime = &record.runtime; - let settings = access.settings_for(&record.provider); - // A bundled kind whose entry names a plugin executable is served out of - // process, exactly as a third-party kind is. - let plugin_served = settings - .as_ref() - .is_some_and(|settings| settings.plugin.is_some()); - let bundled = record.provider.bundled().filter(|_| !plugin_served); - match bundled { + match record.provider.bundled() { // A local sandbox is its working directory: rebuilding the handle // over that directory is the reconnect. The per-process Host // registry holds no state worth attaching to. @@ -107,7 +100,7 @@ pub async fn reconnect_driver_for_run( Ok(sandbox) } None => { - let settings = settings.with_context(|| { + let settings = access.settings_for(&record.provider).with_context(|| { format!( "sandbox provider `{}` is not configured; add [server.sandbox.providers.{}] to settings.toml", record.provider, record.provider diff --git a/lib/components/fabro-sandbox/tests/plugin_provider.rs b/lib/components/fabro-sandbox/tests/plugin_provider.rs index 1a5224489..ddabc2e02 100644 --- a/lib/components/fabro-sandbox/tests/plugin_provider.rs +++ b/lib/components/fabro-sandbox/tests/plugin_provider.rs @@ -90,21 +90,21 @@ async fn host_plugin_under_a_non_bundled_kind_creates_and_reattaches_by_persiste ); } +/// The configured kind is fabro's name for the executable it points at; the +/// plugin's own declared kind is information, not a gate. #[tokio::test] -async fn a_plugin_that_declares_another_kind_is_rejected() { +async fn the_configured_kind_names_the_plugin_whatever_it_declares() { let registry = tempfile::tempdir().expect("registry tempdir"); - let kind = SandboxProviderKind::try_new("e2b").expect("valid kind"); - let error = connect_provider( + let kind = SandboxProviderKind::try_new("host-alias").expect("valid kind"); + let connected = connect_provider( &kind, &host_plugin_settings(registry.path()), &ProviderConnectOptions::default(), ) .await - .err() - .expect("the host executable declares `host`, not `e2b`"); - let rendered = format!( - "{error}: {:?}", - std::error::Error::source(&error).map(ToString::to_string) - ); - assert!(rendered.contains("e2b"), "{rendered}"); + .expect("an aliased plugin launches"); + assert_eq!(connected.kind, kind); + // Fabro's handle on the plugin carries the configured name, so records, + // events, and errors all speak of the kind the operator wrote down. + assert_eq!(connected.provider.kind().as_str(), "host-alias"); } diff --git a/lib/foundation/fabro-config/src/resolve/server.rs b/lib/foundation/fabro-config/src/resolve/server.rs index 6e097a0fc..a4321c1a4 100644 --- a/lib/foundation/fabro-config/src/resolve/server.rs +++ b/lib/foundation/fabro-config/src/resolve/server.rs @@ -72,18 +72,20 @@ fn resolve_sandbox( layer: Option<&ServerSandboxLayer>, errors: &mut Vec, ) -> ServerSandboxSettings { - let _ = errors; let configured = layer .and_then(|sandbox| sandbox.providers.as_ref()) .map(|providers| &providers.entries); let mut entries = BTreeMap::new(); - // Bundled providers always have a policy entry; missing means enabled - // and served in-process. + // Bundled providers always have a policy entry; missing means enabled. for kind in SandboxProviderKind::bundled_kinds() { let layer = configured.and_then(|entries| entries.get(&kind)); + let path = format!("server.sandbox.providers.{kind}"); + if let Some(layer) = layer { + reject_plugin_fields_for_bundled(layer, &path, errors); + } entries.insert(kind, ServerSandboxProviderSettings { enabled: layer.and_then(|provider| provider.enabled).unwrap_or(true), - plugin: layer.and_then(plugin_settings), + plugin: None, }); } for (kind, layer) in configured.into_iter().flatten() { @@ -92,7 +94,14 @@ fn resolve_sandbox( } entries.insert(kind.clone(), ServerSandboxProviderSettings { enabled: layer.enabled.unwrap_or(true), - plugin: Some(plugin_settings(layer).unwrap_or_default()), + plugin: Some(SandboxPluginSettings { + path: layer.path.clone(), + sha256: layer.sha256.clone(), + dev: layer.dev.unwrap_or(false), + args: layer.args.clone().unwrap_or_default(), + env: layer.env.clone().unwrap_or_default(), + inherit_env: layer.inherit_env.clone().unwrap_or_default(), + }), }); } ServerSandboxSettings { @@ -100,33 +109,37 @@ fn resolve_sandbox( } } -/// Plugin launch settings when the entry names any. A bundled kind with -/// none runs in-process; a bundled kind with any runs out of process -/// through the driver's executable for that kind. -fn plugin_settings(layer: &ServerSandboxProviderLayer) -> Option { +fn reject_plugin_fields_for_bundled( + layer: &ServerSandboxProviderLayer, + path: &str, + errors: &mut Vec, +) { let ServerSandboxProviderLayer { enabled: _, - path, + path: plugin_path, sha256, dev, args, env, inherit_env, } = layer; - let any_set = path.is_some() - || sha256.is_some() - || dev.is_some() - || args.is_some() - || env.is_some() - || inherit_env.is_some(); - any_set.then(|| SandboxPluginSettings { - path: path.clone(), - sha256: sha256.clone(), - dev: dev.unwrap_or(false), - args: args.clone().unwrap_or_default(), - env: env.clone().unwrap_or_default(), - inherit_env: inherit_env.clone().unwrap_or_default(), - }) + let set = [ + ("path", plugin_path.is_some()), + ("sha256", sha256.is_some()), + ("dev", dev.is_some()), + ("args", args.is_some()), + ("env", env.is_some()), + ("inherit_env", inherit_env.is_some()), + ]; + for (field, is_set) in set { + if is_set { + errors.push(ResolveError::Invalid { + path: format!("{path}.{field}"), + reason: "bundled sandbox providers run in-process and take no plugin settings" + .to_string(), + }); + } + } } fn resolve_storage(layer: Option<&ServerStorageLayer>) -> ServerStorageSettings { diff --git a/lib/foundation/fabro-config/src/tests/resolve_server.rs b/lib/foundation/fabro-config/src/tests/resolve_server.rs index c066825a4..0fc982b98 100644 --- a/lib/foundation/fabro-config/src/tests/resolve_server.rs +++ b/lib/foundation/fabro-config/src/tests/resolve_server.rs @@ -285,8 +285,8 @@ E2B_API_URL = "https://api.e2b.example" } #[test] -fn server_sandbox_plugin_settings_on_a_bundled_provider_serve_it_out_of_process() { - let settings = ServerSettingsBuilder::from_toml( +fn server_sandbox_rejects_plugin_settings_on_bundled_providers() { + let err = ServerSettingsBuilder::from_toml( r#" _version = 1 @@ -295,33 +295,15 @@ methods = ["dev-token"] [server.sandbox.providers.docker] path = "/usr/local/bin/fabro-sandbox-docker" -dev = true -inherit_env = ["PATH", "DOCKER_HOST"] "#, ) - .expect("bundled providers accept plugin settings"); + .expect_err("bundled providers take no plugin settings"); - let providers = &settings.server.sandbox.providers; - let docker = providers - .get(&SandboxProviderKind::DOCKER) - .expect("docker entry"); - assert!(docker.enabled); - let plugin = docker.plugin.as_ref().expect("docker runs as a plugin"); - assert_eq!( - plugin.path.as_deref(), - Some("/usr/local/bin/fabro-sandbox-docker") - ); - assert!(plugin.dev); - assert_eq!(plugin.inherit_env, vec!["PATH", "DOCKER_HOST"]); assert!( - providers - .get(&SandboxProviderKind::LOCAL) - .expect("local entry") - .plugin - .is_none(), - "an entry without plugin keys stays in-process" + err.to_string() + .contains("server.sandbox.providers.docker.path"), + "unexpected error: {err}" ); - assert_eq!(providers.enabled_plugins().count(), 1); } #[test]