fix(engine): probe real containment in platform_support() - #752
Open
Carlos Alexandro Becker (caarlos0) wants to merge 7 commits into
Open
fix(engine): probe real containment in platform_support()#752Carlos Alexandro Becker (caarlos0) wants to merge 7 commits into
Carlos Alexandro Becker (caarlos0) wants to merge 7 commits into
Conversation
platform_support() reported hosts as supported without testing what the sandbox actually needs, so callers discovered the gap at spawn time instead of at detection time. On Linux it ran `bwrap --version`, which only prints a banner and never creates a namespace. Hosts with unprivileged user namespaces disabled, or with AppArmor denying bwrap, passed detection and then failed at every spawn. It now runs a trivial sandbox with the same namespace set a real run unshares, and surfaces bwrap's own diagnostic as the reason. On Windows it returned is_supported: true unconditionally, with no check against the documented 26100 (24H2) product floor. It now gates on the OS build, failing open when the build cannot be read so a detection failure never declares a supported host unsupported. The SDK's getPlatformSupport() had both defects and gets the same fixes, including a unit test that previously pinned the missing Windows gate as intended behaviour. Below the floor the SDK still reports Windows Sandbox and IsolationSession in availableMethods, since they have their own lower floors, but they no longer set isSupported: that flag guards the default processcontainer spawn. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 89d7c4bf-cda2-4835-95a3-311c5c494b1b Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
Carlos Alexandro Becker (caarlos0)
requested a review
from a team
as a code owner
August 5, 2026 18:16
Copilot started reviewing on behalf of
Carlos Alexandro Becker (caarlos0)
August 5, 2026 18:17
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens platform detection so it reflects actual containment prerequisites.
Changes:
- Probes Bubblewrap by creating a minimal sandbox.
- Enforces the Windows 26100 build floor.
- Adds tests and updates platform-support documentation.
Show a summary per file
| File | Description |
|---|---|
src/core/mxc-sdk/README.md |
Documents Rust platform probes. |
src/core/mxc_engine/src/platform.rs |
Adds cached Linux and Windows probes. |
sdk/node/src/platform.ts |
Mirrors hardened detection in TypeScript. |
sdk/node/tests/unit/platform.test.ts |
Tests the Windows build gate. |
sdk/node/README.md |
Updates troubleshooting guidance. |
docs/process-container/os-version-support.md |
Documents detection-time enforcement. |
Review details
Suppressed comments (1)
sdk/node/src/platform.ts:329
- Below build 26100 this returns with
isSupported: falseeven whenavailableMethodscontains an explicitly selected experimental backend. However, both one-shot and state-aware execution ultimately callresolveBinaryAndCommonArgs, whose unconditional support check rejects wheneverisSupportedis false (sdk/node/src/helper.ts:151-153). The earlier one-shot experimental bypass is therefore undone, and state-aware calls are rejected immediately, so the alternatives advertised here are not actually reachable without the unrelatedskipPlatformCheckescape hatch. Make the shared resolver aware of the requested backend and allow an explicitly selected available experimental backend, then cover both execution surfaces.
if (!methods.includes('processcontainer')) {
const alternatives = methods.length > 0 ? ` (experimental backends available: ${methods.join(', ')})` : '';
support.reason =
`Windows build ${build?.major} is below ${MIN_PROCESSCONTAINER_BUILD}, ` +
`the minimum supported build (Windows 11 24H2)${alternatives}`;
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Balanced
Reconciles the host-capability probe with two upstream changes to the same files. Upstream added a `bwrap` minimum-version gate. That is complementary rather than overlapping: a version check cannot tell whether the host will actually let bwrap create a namespace, and a namespace probe cannot produce the "upgrade your bwrap" message. Both now run, version first, so the more actionable reason wins. Upstream also made WSLC an available Windows backend. It is reported wherever the runtime is present, including below the 26100 processcontainer floor, but like the other opt-in backends it does not set `is_supported` — that flag guards the default processcontainer spawn. The SDK's Bubblewrap sandbox probe is injectable via `_setBwrapSandboxRunner`, matching the `_setBwrapVersionRunner` pattern introduced upstream, so the version-gate tests run on hosts without bwrap. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 89d7c4bf-cda2-4835-95a3-311c5c494b1b Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
Copilot started reviewing on behalf of
Carlos Alexandro Becker (caarlos0)
August 5, 2026 18:25
View session
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (1)
src/core/mxc_engine/src/platform.rs:82
- The 5-second deadline does not bound
platform_support(): this version check runs first, andprobe_bwrap()usesCommand::output()without a timeout. A hanging PATH wrapper or executable on a stalled mount therefore blocks detection indefinitely and never reachesprobe_bubblewrap(), contrary to the PR's bounded-probe behavior. Apply the same deadline to the version subprocess as well.
let unavailable = bwrap_common::bwrap_version::probe_bwrap()
.map_err(|err| err.to_string())
.and_then(|_| probe_bubblewrap());
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
The Windows build floor added in the previous commit ties `isSupported` to `processcontainer`, which is correct for the default spawn path but was also vetoing every state-aware phase: `resolveBinaryAndCommonArgs` checked `isSupported` unconditionally, so a sub-26100 host could list `windows_sandbox` in `availableMethods` and still be refused when calling its state-aware API. Thread the selected containment through the shared helper and apply the same experimental bypass `resolveExecutableAndArgs` already had — which the shared check was silently undoing on the one-shot path too. Also bound `probe_bwrap` with the deadline the engine probe already used, so platform detection as a whole is bounded rather than just its second half, and drop the engine's now-duplicate copy of the helper. Two doc corrections: IsolationSession pins build 26300, so it can never appear below the processcontainer floor (only Windows Sandbox can), and the SDK troubleshooting row described macOS support as schema-version dependent when it is really `/usr/bin/sandbox-exec` presence. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 89d7c4bf-cda2-4835-95a3-311c5c494b1b Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
Copilot started reviewing on behalf of
Carlos Alexandro Becker (caarlos0)
August 5, 2026 18:47
View session
The file is stored with CRLF upstream, and my earlier edits rewrote it as LF — Python's text mode does universal-newline translation on read and writes back `\n`. That turned a +153/-9 change into a +901/-757 whole-file rewrite, burying the actual diff. No content change: `git diff --ignore-cr-at-eol` against the merge base is identical before and after. Note `core.autocrlf=input` strips CRLF on commit, so this had to be staged with the conversion disabled to keep the blob matching upstream. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 89d7c4bf-cda2-4835-95a3-311c5c494b1b Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
Copilot started reviewing on behalf of
Carlos Alexandro Becker (caarlos0)
August 5, 2026 19:15
View session
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (3)
src/backends/bubblewrap/common/src/bwrap_version.rs:221
try_waitonly bounds how long the direct child runs; it does not bound theseread_to_endcalls. Abwrapwrapper can exit after spawning a background process that inherits stdout/stderr, leaving the pipe open (for example,sleep 60 & echo bubblewrap 0.11.2). In that case the parent is already reaped, butread_to_endblocks past the five-second deadline, soplatform_support()can still hang. Drain the pipes concurrently from process start (and coordinate those readers with timeout/kill cleanup) so the deadline covers output collection too.
let stdout = read_pipe(child.stdout.as_mut().map(|p| p as &mut dyn Read))?;
let stderr = read_pipe(child.stderr.as_mut().map(|p| p as &mut dyn Read))?;
sdk/node/src/platform.ts:686
- When this probe times out,
execFileSynccommonly has no stderr but does provide the timeout in the thrown error's message/code. Discarding that information produces only “it failed with no diagnostic output,” hiding whether the five-second deadline fired. Fall back to the exception message (or explicitly formatETIMEDOUT) before using the generic text, while retaining the single-line length cap.
const { stderr } = (error ?? {}) as { stderr?: Buffer | string };
const line = (stderr?.toString() ?? '')
.split('\n')
.map((l) => l.trim())
.find((l) => l.length > 0);
if (!line) {
return 'it failed with no diagnostic output';
sdk/node/src/helper.ts:160
- The backend-aware bypass that fixes sub-26100 state-aware Windows Sandbox calls has no regression coverage. The platform tests only verify the reported fields, while existing state-aware round-trip tests run with the host's normal supported result; dropping any of the newly threaded
containmentarguments would therefore compile and restore the original rejection without failing a test. Add a Windows unit test that stubs build 22631, resets the support cache, and verifies the Windows Sandbox state-aware phases reach the fake spawn withexperimental: true.
const isExperimental =
!!containment && (ExperimentalBackends as readonly string[]).includes(containment);
if (!platformSupport.isSupported && !isExperimental && !options.skipPlatformCheck) {
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…olds The wait deadline bounded `try_wait`, but not the drain that follows it. A pipe only reaches EOF once every write end is closed, so a `bwrap` wrapper that backgrounds a process inheriting stdout keeps `read_to_end` blocked long after the direct child exits — the probe still hung, just at a different line. Collect through unlinked temporary files instead. A file read always terminates, and a descendant that keeps writing after we return is harmless. This also folds the spawn into the helper, so a caller cannot reintroduce the bug by wiring up pipes itself. The regression test asserts the call returns promptly for `sleep 10 & echo 'bubblewrap 0.11.0'`; against the pipe implementation it blocks the full 10s and fails. `tempfile` moves from dev-dependencies to dependencies. It was already in the workspace and already used by this crate's tests, so Cargo.lock is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 89d7c4bf-cda2-4835-95a3-311c5c494b1b Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
Copilot started reviewing on behalf of
Carlos Alexandro Becker (caarlos0)
August 5, 2026 19:24
View session
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (3)
src/backends/bubblewrap/common/src/bwrap_version.rs:206
- The deadline starts only after
Command::spawn()returns. Process launch is synchronous and can itself block while resolving or executingbwrapfrom a stalled network mount—the exact case this helper's documentation says is bounded. Put launch under the timeout supervisor as well, or narrow the guarantee and remove the stalled-launch claim.
let mut child = command
.stdin(Stdio::null())
.stdout(stdout.try_clone()?)
.stderr(stderr.try_clone()?)
.spawn()?;
src/backends/bubblewrap/common/src/bwrap_version.rs:215
- This blocking
wait()can still defeat the deadline. On Linux,SIGKILLremains pending while a process is stuck in uninterruptible I/O, so abwraphung on a stalled mount may never reach reapable state. Issue the kill and move reaping off the caller's critical path (or supervise the process group) sorun_with_deadlineactually returns at the deadline.
let _ = child.kill();
let _ = child.wait();
sdk/node/src/helper.ts:160
- The new bypass is not exercised under the condition it fixes. The state-aware unit suite sets
platformSkipwhenevergetPlatformSupport().isSupportedis false, so it cannot catch this check becoming unconditional again or one lifecycle phase failing to pass its backend. Add a regression that forces unsupported default-platform support, selectswindows_sandbox, and verifies provision/start/exec/stop/deprovision all reach the resolver/spawn path.
const isExperimental =
!!containment && (ExperimentalBackends as readonly string[]).includes(containment);
if (!platformSupport.isSupported && !isExperimental && !options.skipPlatformCheck) {
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Balanced
`read_to_end` on the collected output let the allocation follow the file, so an unusually verbose `bwrap` — or a wrapper backgrounding a writer that keeps growing the file after the direct child exits — could be read without bound. Retain a 64 KiB snapshot instead; `--version` prints one line and a failure prints a short diagnostic, so anything past that is a runaway writer rather than something worth parsing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 89d7c4bf-cda2-4835-95a3-311c5c494b1b Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
Copilot started reviewing on behalf of
Carlos Alexandro Becker (caarlos0)
August 5, 2026 19:47
View session
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (2)
sdk/node/src/helper.ts:160
- The state-aware bypass that motivated this parameter is not regression-tested. Existing state-aware tests run only when the current host is already supported, while the new Windows build-gate tests stop at
getPlatformSupport(), so omittingcontainmentfrom any phase would reintroduce the sub-26100 Windows Sandbox failure unnoticed. Add a below-floor Windows test that drives the state-aware phases through this helper and verifies the explicit experimental backend bypasses the default-backend gate.
const isExperimental =
!!containment && (ExperimentalBackends as readonly string[]).includes(containment);
if (!platformSupport.isSupported && !isExperimental && !options.skipPlatformCheck) {
sdk/node/src/platform.ts:690
- A sandbox-probe timeout with empty stderr is reduced to “failed with no diagnostic output,” hiding the actual five-second timeout. Preserve
ETIMEDOUThere, as the version probe already does, so callers receive an actionable reason.
/** Reduce a failed bwrap run to a single length-capped line for a `reason`. */
function bwrapFailureDetail(error: unknown): string {
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Balanced
The deadline sent a kill and then waited on it. A process wedged in uninterruptible I/O -- a stalled mount, which is one of the cases this deadline exists for -- leaves the signal pending, so that wait would never return and `platform_support()` was still unbounded on the path that matters most. Signal the child and hand it to a detached thread to reap, so the zombie is still collected whenever the kernel lets go without the caller waiting for it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 89d7c4bf-cda2-4835-95a3-311c5c494b1b Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
Copilot started reviewing on behalf of
Carlos Alexandro Becker (caarlos0)
August 5, 2026 20:01
View session
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (1)
sdk/node/src/helper.ts:160
- The state-aware regression fixed by this bypass is not covered. Existing lifecycle tests that exercise the resolver are skipped via
platformSkipwhenisSupportedis false—the exact sub-26100 condition this code handles—and the new build-gate tests only inspectgetPlatformSupport(). Add a regression test that forces a below-floor Windows result and verifies an explicitly experimentalwindows_sandboxlifecycle call reaches the fake spawn instead of being rejected here.
const isExperimental =
!!containment && (ExperimentalBackends as readonly string[]).includes(containment);
if (!platformSupport.isSupported && !isExperimental && !options.skipPlatformCheck) {
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📖 Description
platform_support()reported hosts as supported without testing what the sandbox actually needs, so callers discovered the gap at spawn time instead of at detection time.Linux ran
bwrap --version, which only prints a banner and never creates a namespace. Hosts with unprivileged user namespaces disabled (kernel.unprivileged_userns_clone=0) or with AppArmor denyingbwrap(Ubuntu 23.10+) passed detection and then failed at every spawn. It now runs a trivial sandbox with the same namespace set a real run unshares, and surfacesbwrap's own diagnostic as thereason:--ro-bind-tryrather than--ro-bind / /, becausebwraptreats a failed submount remount as fatal and binding the whole root would make the probe fail on any host with an awkward mount.--clearenvkeeps the verdict independent of the caller'sPATH—execvpthen falls back to its built-in/bin:/usr/bin, both of which the probe binds.This runs after the minimum-version gate added in #714, not instead of it. The two are complementary: a version check can't tell whether the host will let
bwrapcreate a namespace, and a namespace probe can't produce the actionable "upgrade your bwrap" message. Version first, so the more specific reason wins.Windows returned
is_supported: trueunconditionally, with no check against the 26100 (24H2) product floor documented in the README and indocs/process-container/os-version-support.md. It now gates on the OS build, failing open when the build cannot be read so a detection failure never declares a supported host unsupported.The SDK's
getPlatformSupport()had both defects and gets the same fixes. Below the floor the SDK still reportswindows_sandboxandisolation_sessioninavailableMethods, and the engine still reportswslc(from #687), since those have their own separate requirements — but none of them setisSupported. That flag is what guards the defaultprocesscontainerspawn (helper.tsthrows on!isSupportedbeforeavailableMethodsis ever consulted), and all of them are opt-in backends reached explicitly.isSupportedis also what the state-aware path checks, via the sharedresolveBinaryAndCommonArgs. That helper had no experimental bypass, so tyingisSupportedtoprocesscontainerwould have refused every state-aware Windows Sandbox phase on a sub-floor host — the exact hosts where Windows Sandbox is the only thing that works. It now takes the selected containment and bypasses the check for experimental backends, matchingresolveExecutableAndArgs. That also repairs a latent bug predating this PR: the one-shot bypass was being undone by the unconditional check inside the shared helper.Notable details
bwrapsubprocesses are bounded by the same 5s deadline. The namespace probe needs one because it mounts and forks;probe_bwrap()from [Bubblewrap] Gate platform detection on a minimum bwrap version #714 needs one for the same reason--versionlooks safe but isn't — a wrapper script onPATH, or a binary on a stalled network mount, would hang detection outright. The helper moved intobwrap_versionso both callers share it, which also removed the engine's copy.sleep 60 & echo 'bubblewrap 0.11.0') leavesread_to_endblocked long after the direct child exits. A file read always terminates.run_with_deadlineowns the spawn so a caller cannot reintroduce the bug by wiring up pipes itself, and a regression test pins it. On timeout the child is signalled and reaped on a detached thread rather than waited on inline, so a process the kernel will not interrupt cannot hold the deadline open either.tempfilemoves from dev-dependencies to dependencies inbwrap_common; it was already in the workspace and already used by that crate's tests, soCargo.lockis unchanged. The read is capped at 64 KiB:--versionprints one line and a failure prints a short diagnostic, so anything past that is a runaway writer rather than something worth parsing, and the allocation should not follow the file.OnceLockmemoization, matching the SDK's existingcachedSupport.defaultWindowsBuildQuery()no longer discardsCurrentBuildwhenUBRis unreadable.UBRis only needed for the IsolationSession minor-build gate, so it now degrades tominor: 0instead of returningnull— otherwise a missing revision value silently bypassed the new build floor.--unshare-*flags againstbwrap_command::build_args(), so a namespace the production path unshares can't silently drop out of the probe again._setBwrapSandboxRunner, mirroring the_setBwrapVersionRunnerpattern from [Bubblewrap] Gate platform detection on a minimum bwrap version #714, so the version-gate tests still run on hosts withoutbwrap.always reports processcontainer as the default on Windows (no build gate)encoded the missing gate as intended behaviour and is replaced.Known gap, deliberately out of scope
isLxcAvailable()in the SDK is still anlxc-ls --versionbanner check — the same class of defect, but a different backend. A hardened host withlxc-utilsinstalled will still reportisSupported: trueon the LXC arm. Worth a follow-up.🔗 References
mainis merged in and both are preserved.docs/process-container/os-version-support.md— the 26100 product floor this now enforces at detection time.🔍 Validation
Host is macOS (aarch64); the Windows and Linux arms are compile-verified only — see limitations below.
cargo fmt -p mxc_engine -- --checkcargo test -p mxc_engine -p bwrap_commoncargo clippy -p mxc_engine -p bwrap_common --all-targets -- -D warnings--target x86_64-unknown-linux-gnu--target x86_64-pc-windows-msvcnpx tsc --noEmit(sdk/node)npm test(sdk/node)policy.test.tsPowerShell-discovery failures on a macOS host, unrelated)platform_support()→true ["seatbelt"] None; a second call exercises the memoization pathgetPlatformSupport()→{"isSupported":true,"availableMethods":["seatbelt"]}New tests:
platform.rs:windows_build_at_or_above_floor_is_supported,windows_build_below_floor_is_unsupported,wslc_is_reported_but_does_not_carry_support,probe_unshares_every_production_namespace(Linux),probe_clears_the_environment_and_binds_the_paths_it_execs_from,probe_never_binds_the_host_root, plus threebwrap_failure_detailcases (first non-empty line, silence, char-boundary-safe truncation).sdk/node/tests/unit/platform.test.ts: aprocesscontainer build gateblock covering below-floor (19045 / 22000 / 22631 / 26099), at-or-above (26100 / 26200 / 26600), and an unreadable build (fails open); plusrejects a new enough bwrap that cannot create a sandboxfor the case--versioncannot see.Limitations. The bwrap probe argument list has not been executed — there is no Linux runtime on this machine. It was cross-checked against upstream
bubblewrap.c(--handling,--ro-bind-try,--clearenvsemantics,execvpfallback) and against MXC's ownbwrap_command::build_args, and the drift test pins the namespace set, but a run on a hardened Linux host would be the real confirmation. The Windows SDK tests areskip: !isWindows, so they only execute in CI.✅ Checklist
Cargo.lock, thedependency-feed-checkcheck passes (see docs/pull-requests.md)📋 Issue Type