From 6548b107d9a7a46c61671c22cc729307a59acb57 Mon Sep 17 00:00:00 2001 From: Gudge Date: Fri, 10 Jul 2026 15:51:39 -0700 Subject: [PATCH] Decouple ProcessContainer backend selection from schema version This change makes the Windows ProcessContainer AppContainer-vs-BaseContainer choice entirely host-capability driven: BaseContainer is preferred when usable, and AppContainer+BFS is the downlevel fallback. Schema version and --experimental no longer affect this selection. It also incorporates updates that clarify diagnostics and make the streaming fallback explicit. Details: - Route the executor through the capability-driven dispatcher unconditionally and remove the schema-version/experimental gate and obsolete parser helper. - Select the SDK streaming backend using the same BaseContainer usability probe; on downlevel hosts it explicitly creates an AppContainer+BFS runner. - Reword BaseContainer E_NOTIMPL diagnostics to describe an OS-disabled feature without internal terminology, and remove stale schema-version guidance. - Update backend-agnostic regression coverage and tidy related comments. Tests: - cargo test -p wxc_common -p appcontainer_common -p mxc-sdk - cargo clippy --workspace --all-targets -- -D warnings - cargo fmt --all -- --check - check-schema-codegen.js and check-sdk-types-codegen.js Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6c58a97a-f189-43eb-b159-78965a028dc9 Generated-with: gpt-5.6-terra --- .../common/src/appcontainer_runner.rs | 17 ++-- .../common/src/base_container_runner.rs | 23 ++--- .../common/src/launch_diagnostics.rs | 24 +++-- src/core/mxc-sdk/src/dispatch.rs | 42 ++++---- src/core/mxc-sdk/src/policy.rs | 3 +- src/core/mxc-sdk/tests/sandbox.rs | 50 ++++------ src/core/wxc/src/main.rs | 95 ++++++++----------- src/core/wxc_common/src/config_parser.rs | 39 -------- src/core/wxc_common/src/models.rs | 6 +- 9 files changed, 111 insertions(+), 188 deletions(-) diff --git a/src/backends/appcontainer/common/src/appcontainer_runner.rs b/src/backends/appcontainer/common/src/appcontainer_runner.rs index 5afd944ad..c0e0a860d 100644 --- a/src/backends/appcontainer/common/src/appcontainer_runner.rs +++ b/src/backends/appcontainer/common/src/appcontainer_runner.rs @@ -1137,17 +1137,12 @@ impl AppContainerScriptRunner { FileSystemBfsManager::new(self.app_container_name.clone(), bfscfg_path); if self.filesystem_mode == FilesystemMode::Bfs { if let Err(e) = bfs_manager.configure(&request.policy, logger) { - let msg = if matches!(&e, WxcError::BfsNotAvailable) - && request.schema_version.starts_with("0.4.0") - { - format!( - "Filesystem policy error: bfscfg.exe is not available on this Windows build. \ - Your config uses schema version '{}', which requires BFS support. \ - Either update your Windows build to one that includes bfscfg.exe, \ - or update your config to schema version '0.6.0-alpha' or later \ - (which uses the BaseContainer backend and does not require bfscfg.exe).", - request.schema_version - ) + let msg = if matches!(&e, WxcError::BfsNotAvailable) { + "Filesystem policy error: bfscfg.exe is not available on this Windows \ + build, so the AppContainer + BFS filesystem tier cannot enforce your \ + policy. Use an OS build that includes bfscfg.exe, or run on a host that \ + supports the BaseContainer backend (which does not require bfscfg.exe)." + .to_string() } else { e.to_string() }; diff --git a/src/backends/appcontainer/common/src/base_container_runner.rs b/src/backends/appcontainer/common/src/base_container_runner.rs index e107a0924..cbbf386f3 100644 --- a/src/backends/appcontainer/common/src/base_container_runner.rs +++ b/src/backends/appcontainer/common/src/base_container_runner.rs @@ -1234,22 +1234,13 @@ impl SandboxBackend for BaseContainerRunner { )); } Self::is_base_container_api_present().map_err(|e| { - let hint = if !request.experimental_enabled { - format!( - "BaseContainer API unavailable: {e}\n\ - Hint: Config schema version '{}' requires the BaseContainer backend, \ - but this OS build does not support it. \ - Use schema version '0.4.0-alpha' to fall back to AppContainer.", - request.schema_version - ) - } else { - format!( - "BaseContainer API unavailable: {e}\n\ - Hint: --experimental requested BaseContainer, but this OS build \ - does not support it. Remove --experimental to use the AppContainer \ - backend, or use an OS build with BaseContainer support." - ) - }; + let hint = format!( + "BaseContainer API unavailable: {e}\n\ + Hint: this OS build does not support the BaseContainer backend. \ + MXC selects BaseContainer automatically when the host supports it \ + and otherwise uses AppContainer; use an OS build with BaseContainer \ + support if you require it." + ); ScriptResponse { // Symbol absent: report BackendUnavailable, not a hard error. failure_phase: FailurePhase::BackendUnavailable, diff --git a/src/backends/appcontainer/common/src/launch_diagnostics.rs b/src/backends/appcontainer/common/src/launch_diagnostics.rs index ffa806696..38a762b92 100644 --- a/src/backends/appcontainer/common/src/launch_diagnostics.rs +++ b/src/backends/appcontainer/common/src/launch_diagnostics.rs @@ -167,25 +167,29 @@ fn diagnose_api_not_implemented() -> LaunchDiagnostic { let message = if key_status.is_empty() { "Experimental_CreateProcessInSandbox returned E_NOTIMPL. \ The BaseContainer feature is not enabled on this OS build. \ - Ensure the required velocity keys are enabled, or use schema \ - version '0.4.0-alpha' to fall back to the AppContainer backend." + It may be possible to enable it through the Windows experimental \ + features settings, or run on a host that supports the BaseContainer \ + backend (MXC falls back to AppContainer automatically on builds \ + without it)." .to_string() } else { let disabled: Vec<_> = key_status.iter().filter(|(_, enabled)| !enabled).collect(); if disabled.is_empty() { "Experimental_CreateProcessInSandbox returned E_NOTIMPL. \ - All known velocity keys appear enabled, but the feature is \ - still gated. The OS build may require additional enablement. \ - Use schema version '0.4.0-alpha' to fall back to the AppContainer backend." + The BaseContainer feature is not enabled on this OS build; it may \ + require additional enablement. MXC falls back to AppContainer \ + automatically on builds without BaseContainer support." .to_string() } else { let disabled_list: Vec = disabled.iter().map(|(id, _)| id.to_string()).collect(); format!( "Experimental_CreateProcessInSandbox returned E_NOTIMPL. \ - The following velocity keys are not enabled: {}. \ - Enable them and retry, or use schema version '0.4.0-alpha' \ - to fall back to the AppContainer backend.", + The BaseContainer feature is not enabled on this OS build \ + (disabled feature flags: {}). It may be possible to enable it \ + through the Windows experimental features settings, or run on a \ + host that supports the BaseContainer backend (MXC falls back to \ + AppContainer automatically on builds without it).", disabled_list.join(", ") ) } @@ -424,7 +428,9 @@ mod tests { fn api_not_implemented_triggers_feature_diagnostic() { let diag = diagnose_create_process_failure(ERROR_CALL_NOT_IMPLEMENTED.0, "pwsh.exe", &[]); assert_eq!(diag.kind, "feature_not_enabled"); - assert!(diag.message.contains("velocity")); + assert!(diag + .message + .contains("BaseContainer feature is not enabled")); } #[test] diff --git a/src/core/mxc-sdk/src/dispatch.rs b/src/core/mxc-sdk/src/dispatch.rs index 7cacb9e4b..65cf674c8 100644 --- a/src/core/mxc-sdk/src/dispatch.rs +++ b/src/core/mxc-sdk/src/dispatch.rs @@ -148,35 +148,37 @@ fn spawn_process_container( request: &ExecutionRequest, logger: &mut Logger, ) -> Result, MxcError> { - use appcontainer_common::appcontainer_runner::AppContainerScriptRunner; - use wxc_common::config_parser::is_base_container_version; + use appcontainer_common::appcontainer_runner::{AppContainerScriptRunner, FilesystemMode}; + use appcontainer_common::fallback_detector::is_base_container_usable; use wxc_common::sandbox_process::{SandboxBackend, StdioMode}; - // The AppContainer fast path vs the native BaseContainer (OS sandbox API): - // unlike the executor binaries' run-to-completion fallback, streaming does - // NOT route through `dispatch_with_fallback` — there is no AppContainer-BFS - // / AppContainer-DACL fallback for streaming. + // ProcessContainer resolves to a concrete backend purely by host + // capability: prefer the native BaseContainer (OS sandbox API) when usable, + // otherwise AppContainer. The schema version does not influence this choice. // - // Why: `dispatch_with_fallback` yields a run-to-completion - // `Box` plus a `DaclManager` guard, neither of which - // fits the streaming handle (the DACL tier would require the returned - // `SandboxProcess` to own the guard so ACE restore outlives the child). - // - // Consequence (intentional, fail-closed): an experimental / newer-schema - // config on a host that lacks the native BaseContainer API fails here with - // a clear "BaseContainer API unavailable" error from - // `BaseContainerRunner`'s validation, whereas the binaries' fallback would - // drop to an AppContainer tier. Streaming therefore requires the native - // BaseContainer API for those configs. - let version_implies_base_container = is_base_container_version(&request.schema_version); - if request.experimental_enabled || version_implies_base_container { + // Unlike the executor binaries' run-to-completion path, streaming does NOT + // route through `dispatch_with_fallback` — that yields a run-to-completion + // `Box` plus a `DaclManager` guard, neither of which fits + // the streaming handle (the DACL tier would require the returned + // `SandboxProcess` to own the guard so ACE restore outlives the child). So + // streaming offers only two of the dispatcher's tiers: BaseContainer when + // the API is usable, otherwise AppContainer in BFS mode (equivalent to the + // dispatcher's Tier 2 AppContainer-BFS path, which still requires + // `bfscfg.exe`). The Tier 3 AppContainer-DACL fallback is not available on + // the streaming path. The BaseContainer-vs-AppContainer choice uses the same + // `is_base_container_usable()` probe as the dispatcher's Tier 1 selection, so + // the two paths agree on that decision. + if is_base_container_usable() { let mut runner = appcontainer_common::base_container_runner::BaseContainerRunner::new(); return runner .spawn(request, logger, StdioMode::Pipes) .map_err(map_spawn_error); } - let mut runner = AppContainerScriptRunner::new(); + // Select BFS mode explicitly so this does not silently change if + // `AppContainerScriptRunner::new()`'s default filesystem mode is ever + // altered. + let mut runner = AppContainerScriptRunner::with_filesystem_mode(FilesystemMode::Bfs); runner .spawn(request, logger, StdioMode::Pipes) .map_err(map_spawn_error) diff --git a/src/core/mxc-sdk/src/policy.rs b/src/core/mxc-sdk/src/policy.rs index 703cfc4a1..80ac8533b 100644 --- a/src/core/mxc-sdk/src/policy.rs +++ b/src/core/mxc-sdk/src/policy.rs @@ -727,7 +727,8 @@ fn proxy_to_wire(proxy: &ProxySpec) -> serde_json::Value { /// Apply backend-specific fields, resolving the abstract `Process` intent the /// same way the SDK does (Bubblewrap on Linux, Seatbelt on macOS, -/// BaseContainer on Windows). +/// ProcessContainer on Windows — which itself resolves to BaseContainer or +/// AppContainer at runtime by host capability). fn apply_backend(config: &mut serde_json::Value, policy: &SandboxPolicy, container_id: &str) { use serde_json::json; diff --git a/src/core/mxc-sdk/tests/sandbox.rs b/src/core/mxc-sdk/tests/sandbox.rs index 01c0e97a3..90c406280 100644 --- a/src/core/mxc-sdk/tests/sandbox.rs +++ b/src/core/mxc-sdk/tests/sandbox.rs @@ -39,7 +39,7 @@ fn seatbelt_request(command: &str, timeout_ms: u32) -> SandboxRequest { } /// A Windows ProcessContainer request exposing `C:\Windows\Temp` read-write. -/// The policy `version` selects the tier (>= 0.5 implies BaseContainer). +/// `version` is the schema version stamped on the policy, not a backend selector. #[cfg(target_os = "windows")] fn process_container_request(version: &str, command: &str, timeout_ms: u32) -> SandboxRequest { let policy = SandboxPolicy { @@ -240,9 +240,11 @@ fn seatbelt_defaults_cwd_to_allowed_path_without_getcwd_leak() { // --------------------------------------------------------------------------- // Windows ProcessContainer (AppContainer + BaseContainer) — integration tests. // -// These exercise the capture and timeout paths that regressed as review items -// #1 (BaseContainer ran with an already-closed process handle) and #2 -// (AppContainer timeout killed only the direct child, so it never fired). +// These exercise two capture/timeout regressions: the process handle being +// closed before the wait completed, and a finite timeout killing only the +// direct child so it never fired. ProcessContainer resolves to BaseContainer or +// AppContainer by host capability; these guards hold for whichever backend the +// host selects. // They run a real sandbox, so they require an elevated, host-prepped Windows // host (see docs/host-prep.md) and are therefore `#[ignore]`d — run them with // `cargo test -p mxc-sdk -- --ignored` on such a host. @@ -251,19 +253,18 @@ fn seatbelt_defaults_cwd_to_allowed_path_without_getcwd_leak() { #[cfg(target_os = "windows")] #[test] #[ignore = "requires an elevated, host-prepped Windows host (see docs/host-prep.md)"] -fn base_container_captures_stdout() { - // Schema >= 0.5 implies the BaseContainer fallback. Regression guard for - // #1: a valid exit code and captured stdout prove the process handle was - // not closed out from under the wait. +fn process_container_captures_stdout() { + // Regression guard: a valid exit code and captured stdout prove the + // process handle was not closed out from under the wait. let result = spawn_and_wait(process_container_request( "0.7.0-alpha", - "cmd /c echo hello-base-container", + "cmd /c echo hello-process-container", 30000, )) - .expect("BaseContainer run should succeed"); + .expect("ProcessContainer run should succeed"); assert_eq!(result.exit_code, 0, "stderr: {}", result.standard_err); assert!( - result.standard_out.contains("hello-base-container"), + result.standard_out.contains("hello-process-container"), "stdout should be captured, got: {:?}", result.standard_out ); @@ -272,36 +273,17 @@ fn base_container_captures_stdout() { #[cfg(target_os = "windows")] #[test] #[ignore = "requires an elevated, host-prepped Windows host (see docs/host-prep.md)"] -fn appcontainer_captures_stdout() { - // Schema 0.4 keeps us on the AppContainer fast path (no BaseContainer). - let result = spawn_and_wait(process_container_request( - "0.4.0-alpha", - "cmd /c echo hello-appcontainer", - 30000, - )) - .expect("AppContainer run should succeed"); - assert_eq!(result.exit_code, 0, "stderr: {}", result.standard_err); - assert!( - result.standard_out.contains("hello-appcontainer"), - "stdout should be captured, got: {:?}", - result.standard_out - ); -} - -#[cfg(target_os = "windows")] -#[test] -#[ignore = "requires an elevated, host-prepped Windows host (see docs/host-prep.md)"] -fn appcontainer_finite_timeout_fires() { - // Regression guard for #2: a finite timeout must fire even when the command +fn process_container_finite_timeout_fires() { + // Regression guard: a finite timeout must fire even when the command // spawns a descendant that keeps the inherited stdout write-end open. If // the timeout only killed the direct child, the capture reader would block // forever and this test would hang past the bounded wall-clock below. let result = spawn_and_wait(process_container_request( - "0.4.0-alpha", + "0.7.0-alpha", "cmd /c start /b ping -n 60 127.0.0.1 >nul & ping -n 60 127.0.0.1 >nul", 2000, )) - .expect("AppContainer run should return a response"); + .expect("ProcessContainer run should return a response"); assert!(result.timed_out, "a timed-out run must report a timeout"); // The bounded wait is enforced by the test harness; a hang here is the // failure mode the regression guards against. diff --git a/src/core/wxc/src/main.rs b/src/core/wxc/src/main.rs index 39829dfeb..748eced03 100644 --- a/src/core/wxc/src/main.rs +++ b/src/core/wxc/src/main.rs @@ -13,9 +13,7 @@ use std::sync::atomic::Ordering; use std::sync::{Mutex, OnceLock}; use std::time::Instant; -use appcontainer_common::appcontainer_runner::{ - delete_app_container_profile, AppContainerScriptRunner, -}; +use appcontainer_common::appcontainer_runner::delete_app_container_profile; use clap::Parser; #[cfg(all(feature = "hyperlight", target_arch = "x86_64"))] use hyperlight_common::HyperlightScriptRunner; @@ -26,13 +24,12 @@ use nanvix_runner::NanVixScriptRunner; use windows_sandbox_common::windows_sandbox_runner::WindowsSandboxScriptRunner; use wxc_common::cmdline::{cmdline_from_argv_for_context, CommandLineContext, CommandLineError}; use wxc_common::config_parser::{ - is_base_container_version, load_mxc_request_with_options, load_request, LoadOptions, ParseError, + load_mxc_request_with_options, load_request, LoadOptions, ParseError, }; use wxc_common::diagnostic::DiagnosticConfig; use wxc_common::logger::{Logger, Mode}; use wxc_common::models::{ContainmentBackend, ExecutionRequest, ScriptResponse}; use wxc_common::mxc_error::{MxcError, ResponseEnvelope}; -use wxc_common::sandbox_process::Runner; use wxc_common::script_runner::{handle_dry_run_exit, ScriptRunner}; #[cfg(all(target_os = "windows", feature = "isolation_session"))] use wxc_common::state_aware_dispatch::dispatch_state_aware; @@ -950,64 +947,50 @@ fn main() { // slot returns `None` if no DACL augmentation was required. // Run script in selected containment backend. - // BaseContainer is used when --experimental is passed or schema version >= 0.5. - // Sandbox and MicroVM require --experimental flag. + // ProcessContainer resolves to BaseContainer or AppContainer by host + // capability (see below). Sandbox and MicroVM require --experimental flag. let mut runner: Box = match request.containment { ContainmentBackend::ProcessContainer => { - // Compute fallback eligibility on the ProcessContainer arm - // only — every other `ContainmentBackend` variant is - // unaffected by `use_base_container` and does not need to - // pay the (trivial) semver parse cost. - let version_implies_base_container = is_base_container_version(&request.schema_version); - let use_base_container = request.experimental_enabled || version_implies_base_container; - - if use_base_container { - let reason = if version_implies_base_container { - format!("schema version {}", request.schema_version) - } else { - "--experimental".to_string() - }; - let _ = writeln!(logger, "Using BaseContainer-fallback dispatcher ({reason})"); + // ProcessContainer resolves to a concrete Windows backend purely by + // host capability: `dispatch_with_fallback` prefers the native + // BaseContainer (OS sandbox API) when usable and otherwise falls + // back to AppContainer tiers (BFS / DACL). The schema version no + // longer influences this choice. + match appcontainer_common::dispatcher::dispatch_with_fallback(&request) { + Ok(dispatched) => { + for w in &dispatched.warnings { + let _ = writeln!(logger, "warning: {w}"); + } + let _ = writeln!( + logger, + "selected isolation tier: {}", + dispatched.tier.as_str() + ); - match appcontainer_common::dispatcher::dispatch_with_fallback(&request) { - Ok(dispatched) => { - for w in &dispatched.warnings { - let _ = writeln!(logger, "warning: {w}"); - } - let _ = writeln!( - logger, - "selected isolation tier: {}", - dispatched.tier.as_str() - ); - - let (dispatched_runner, dacl_manager) = dispatched.into_runner_and_guard(); - if let Some(mgr) = dacl_manager { - park_dacl_for_cleanup(mgr); - } - dispatched_runner + let (dispatched_runner, dacl_manager) = dispatched.into_runner_and_guard(); + if let Some(mgr) = dacl_manager { + park_dacl_for_cleanup(mgr); } - Err(e) => { - eprintln!("error: {e}"); - if let appcontainer_common::dispatcher::DispatchError::Dacl { - warnings, - .. - } = &e - { - for w in warnings { - eprintln!(" dacl warning: {w}"); - } + dispatched_runner + } + Err(e) => { + eprintln!("error: {e}"); + if let appcontainer_common::dispatcher::DispatchError::Dacl { + warnings, .. + } = &e + { + for w in warnings { + eprintln!(" dacl warning: {w}"); } - eprint!("{}", logger.get_buffer()); - telemetry::emit_early_exit( - telemetry_active, - &request.containment, - telemetry::FailureReason::InitError, - ); - process::exit(1); } + eprint!("{}", logger.get_buffer()); + telemetry::emit_early_exit( + telemetry_active, + &request.containment, + telemetry::FailureReason::InitError, + ); + process::exit(1); } - } else { - Box::new(Runner::new(AppContainerScriptRunner::new())) } } ContainmentBackend::Wslc => { diff --git a/src/core/wxc_common/src/config_parser.rs b/src/core/wxc_common/src/config_parser.rs index 9696e5aac..e903a6157 100644 --- a/src/core/wxc_common/src/config_parser.rs +++ b/src/core/wxc_common/src/config_parser.rs @@ -210,33 +210,12 @@ const SUPPORTED_VERSION: &str = ">=0.4, <=0.8"; #[cfg(test)] const CURRENT_SCHEMA_VERSION: &str = "0.8.0-alpha"; -/// The minimum schema version that implies BaseContainer backend usage. -const BASE_CONTAINER_MIN_VERSION: &str = "0.5.0"; - /// Known `experimental.` keys. Used by validation code to flag /// experimental backend sections that don't match the selected /// `containment`. Add a new entry when promoting a backend to a top-level /// section or graduating one from experimental. const KNOWN_EXPERIMENTAL_BACKENDS: &[&str] = &["windows_sandbox", "wslc", "isolation_session"]; -/// Returns `true` if `version` is a BaseContainer-era schema version (>= 0.5.0). -/// -/// Pre-release labels are stripped before comparison, so `"0.5.0-alpha"` is -/// treated identically to `"0.5.0"`. Returns `false` for empty or -/// unparseable version strings. -pub fn is_base_container_version(version: &str) -> bool { - if version.is_empty() { - return false; - } - let parsed = match semver::Version::parse(version) { - Ok(v) => v, - Err(_) => return false, - }; - let comparable = semver::Version::new(parsed.major, parsed.minor, parsed.patch); - let threshold = semver::Version::parse(BASE_CONTAINER_MIN_VERSION).unwrap(); - comparable >= threshold -} - /// Validate that the schema version (semver) is supported by this binary. /// Compares major.minor only — patch and pre-release labels are ignored. fn validate_schema_version(version: &str, logger: &mut Logger) -> Result<(), WxcError> { @@ -3289,24 +3268,6 @@ mod tests { assert_eq!(req.policy.ui.clipboard, ClipboardPolicy::All); } - #[test] - fn is_base_container_version_recognizes_050() { - assert!(is_base_container_version("0.5.0-alpha")); - assert!(is_base_container_version("0.5.0")); - assert!(is_base_container_version("0.5.1")); - assert!(is_base_container_version("0.6.0")); - assert!(is_base_container_version("1.0.0")); - } - - #[test] - fn is_base_container_version_rejects_040() { - assert!(!is_base_container_version("0.4.0-alpha")); - assert!(!is_base_container_version("0.4.0")); - assert!(!is_base_container_version("0.4.9")); - assert!(!is_base_container_version("")); - assert!(!is_base_container_version("not-a-version")); - } - // ====== Isolation Session containment and config tests ====== #[test] diff --git a/src/core/wxc_common/src/models.rs b/src/core/wxc_common/src/models.rs index f6e403a5f..d8efb1317 100644 --- a/src/core/wxc_common/src/models.rs +++ b/src/core/wxc_common/src/models.rs @@ -10,8 +10,10 @@ pub enum ContainmentBackend { #[default] /// Windows process-level containment. Resolves at runtime to either /// AppContainer (legacy OS API) or BaseContainer (newer Windows - /// sandbox API exposed via `Experimental_CreateProcessInSandbox`), - /// based on `--experimental` and the schema version of the request. + /// sandbox API exposed via `Experimental_CreateProcessInSandbox`) + /// based purely on host capability — BaseContainer is preferred when + /// the OS supports it, AppContainer is the downlevel fallback. The + /// schema version does not influence this choice. /// Selected on the wire as `"processcontainer"`. ProcessContainer, /// Linux container via WSL Container SDK (WSLC SDK).