Harden Bubblewrap version probing

[Gudge (MGudgin)]

• I have signed the Contributor License Agreement.
• This pull request is related to an issue.
• If this PR changes build commands, project architecture, or key conventions, I have updated .github/copilot-instructions.md.

---

Summary

This PR hardens Bubblewrap availability probing so hung, noisy, broken, or instrumented launchers remain bounded and produce actionable diagnostics in the Rust executor and Node SDK.

Details

• Apply total wall-clock deadlines and output caps across PATH resolution, process launch, worker setup, supervision, and cleanup.
• Retain process-group ownership through descendant termination and reaping, with Node result publication gated on completed anchor cleanup.
• Bound concurrent Rust probes, cache successful advisory results, and preserve uncached validation for execution and direct runner calls.
• Package Node supervision as compiled helper modules with bundled-ESM asset resolution and sanitized NODE_OPTIONS.
• Add lifecycle, failure-classification, concurrency, deadline, cleanup-ordering, and diagnostic coverage.

Tests

• cargo fmt --all -- --check.
• cargo check --workspace --all-targets.
• cargo clippy --workspace --all-targets -- -D warnings.
• cargo test --workspace.
• cargo check -p bwrap_common --tests --target x86_64-unknown-linux-gnu.
• cargo clippy -p bwrap_common --all-targets --target x86_64-unknown-linux-gnu -- -D warnings.
• npm run build; npm test (224 passed, 19 skipped); npm pack --dry-run.

Related Issues

• [Bubblewrap] Gate platform detection on a minimum bwrap version #714

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

🟡 Not ready to approve

PATH resolution remains unbounded, and descendant processes can bypass the timeout or leak probe resources.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Hardens Bubblewrap availability probing across Rust and the Node.js SDK.

Changes:

• Adds bounded output, timeout handling, successful-result caching, and injectable probes.
• Aligns diagnostics and expands regression coverage.
• Updates SDK troubleshooting guidance.

File summaries

File	Description
src/core/mxc_engine/src/platform.rs	Adds injectable Linux support detection tests.
src/backends/bubblewrap/common/src/bwrap_version.rs	Implements bounded, cached version probing.
src/backends/bubblewrap/common/src/bwrap_runner.rs	Adds an injectable validation probe and tests.
sdk/node/src/platform.ts	Hardens SDK probing and diagnostics.
sdk/node/tests/unit/platform.test.ts	Expands probe and support tests.
sdk/node/README.md	Updates Linux troubleshooting guidance.

Review details

Suppressed comments (1)

src/backends/bubblewrap/common/src/bwrap_version.rs:408

• The timeout starts only after this PATH scan and Command::spawn, so candidate.is_file() on a stalled network-mounted PATH entry can block the probe indefinitely before the deadline exists. Please move resolution into an operation governed by the same timeout (or otherwise bound the metadata lookup), so probe_bwrap actually has a hard execution-time limit.

        std::env::split_paths(path)
            .map(|dir| dir.join("bwrap"))
            .find(|candidate| candidate.is_file())

• Files reviewed: 6/6 changed files
• Comments generated: 2
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Gudge (MGudgin)]

Also fixed the initial CI failures in 5c76454: removed the Linux/macOS needless_return lint violations and relaxed the Linux SDK diagnostic assertion to match the actual Bubblewrap (bwrap) 0.4.1 message.

[Copilot]

🟡 Not ready to approve

The Rust probe has an output-readiness race, while the SDK can still hang on descendants and silently discard Linux diagnostics.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (2)

src/backends/bubblewrap/common/src/bwrap_version.rs:278

• This tuple evaluates both take() calls before pattern matching. If one reader finishes before the child or the other reader (for example, a wrapper closes stdout while keeping stderr open), that captured value is removed even though the pattern fails; the next poll then sees a disconnected channel and reports an internal probe failure. Check that all three results are present before taking any of them.

        if let (Some(status), Some(stdout), Some(stderr)) = (status, stdout.take(), stderr.take()) {

sdk/node/src/platform.ts:285

• The production logger here is diagLog, whose transport returns no socket on non-Windows hosts (sdk/node/src/diagnostic.ts:85-88). Therefore, on Linux with LXC available, reason remains empty and this call drops the Bubblewrap failure instead of surfacing it; the injected test logger masks that production behavior. Preserve the per-backend reason in a Linux-visible API/diagnostic channel rather than routing it to the Windows-only console.

      platformDiagnosticLogger(`getPlatformSupport: bubblewrap unavailable — ${bubblewrap.reason}`);

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Gudge (MGudgin)]

Addressed the two suppressed Copilot findings in 395fe43:

• Rust now checks that the child status and both reader results are present before taking either Option, so an early completed stream cannot be discarded while the other results are pending. The try_wait error path also reaps the terminated child.
• Linux PlatformSupport now exposes unavailableReasons per backend. When LXC keeps the platform supported, callers can inspect unavailableReasons.bubblewrap instead of losing the Bubblewrap failure through the Windows-only diagnostic transport; reason remains reserved for whole-platform failure.

The Node descendant fix avoids new timeout/setsid runtime dependencies: it uses /bin/sh job control plus Node's bounded execFileSync timeout.

[Gudge (MGudgin)]

Copilot review

[Copilot]

🟡 Not ready to approve

Exit status 127 can incorrectly classify a present but broken Bubblewrap wrapper as absent in both implementations.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (2)

src/backends/bubblewrap/common/src/bwrap_version.rs:295

• This classifies any exit 127 whose stderr merely mentions bwrap as “not installed.” An executable wrapper that runs but fails to launch an internal command commonly exits 127 with stderr such as /path/bwrap: ... not found, so this loses the intended present-but-broken diagnosis and gives incorrect installation advice. Restrict NotFound to the diagnostic emitted by /usr/bin/env itself; other 127 results should remain ProbeFailed.

            if status.code() == Some(127)
                && String::from_utf8_lossy(&stderr.bytes).contains("bwrap")
            {
                return Err(BwrapUnavailable::NotFound);

sdk/node/src/platform.ts:452

• This broad check also mislabels a present wrapper as absent whenever the wrapper itself exits 127 and its shell diagnostic includes its bwrap pathname. That contradicts the missing-vs-broken distinction and sends users to install a package that is already present. Only treat stderr originating from /usr/bin/env as command lookup failure; preserve all other 127 exits as failed.

    const error = err as BwrapExecError;
    if (error.status === 127 && error.stderr?.toString().includes('bwrap')) {
      return { kind: 'notFound' };
    }

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

The SDK incorrectly classifies every exit status 124 as a timeout, losing the actual probe failure.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

sdk/node/src/platform.ts:412

• Treating every exit status 124 as a timeout loses the real subprocess failure. A bwrap wrapper can exit 124 immediately, in which case the SDK reports “timed out after 5000ms” while the Rust probe correctly preserves status 124. Only Node's timeout metadata (ETIMEDOUT/killed) can distinguish a deadline expiry; preserve a standalone status 124 as an ordinary failed exit.

  if (
    error.code === 'ETIMEDOUT' ||
    error.killed ||
    error.status === BWRAP_TIMEOUT_EXIT_STATUS
  ) {

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

Post-timeout Rust cleanup can still block indefinitely, and the SDK regression does not verify descendant termination.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

sdk/node/tests/unit/platform.test.ts:496

• This regression only proves that the supervisor returns. Because the wrapper's streams are redirected to temporary files, the background sleep cannot hold Node's pipes, so the test still passes if terminate_tree is a no-op and the descendant leaks. Record the background PID and assert with bounded polling that it no longer exists to cover the new SIGKILL cleanup behavior.

        const result = _runBwrapVersionCommand(1000);
        assert.ok(Date.now() - started < 2000, 'probe must remain bounded');
        assert.deepStrictEqual(result, {
          kind: 'output',
          stdout: 'bubblewrap 0.5.0\n',

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Gudge (MGudgin)]

Addressed the latest suppressed Copilot findings in 5bd8e51:

• Exit 127 is now classified as missing only when stderr identifies env (by path or basename) as the reporter and names bwrap. A present wrapper whose own diagnostic mentions bwrap remains a failed probe in both Rust and Node.
• Node timeout mapping now relies only on ETIMEDOUT/killed; a wrapper that exits 124 is preserved as an ordinary status-124 failure, with a regression test.
• The Linux descendant regression records the background PID and polls for ESRCH, so it verifies process-group termination rather than merely proving that redirected pipes let the supervisor return.

The Node supervisor also now uses setsid rather than non-interactive shell job control, and captures size-limited output through temporary files so descendants cannot retain Node's pipes.

[Gudge (MGudgin)]

Copilot review

[Copilot]

🟡 Not ready to approve

The SDK supervisor can miss a child before setsid establishes its process group, allowing the probe to outlive its timeout.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

Copilot review

Addressed in the latest commit. terminate_tree() now kills the direct child PID before the group kill:

terminate_tree() {
  if [ -n "$child" ]; then
    kill -KILL "$child" 2>/dev/null || true
    kill -KILL -"$child" 2>/dev/null || true
  fi
}

Before setsid has run, PGID $child doesn't exist yet, so only the direct kill lands. After setsid, both kills fire; the group kill covers all descendants. This mirrors the Rust terminate_probe_tree which calls both child.kill() and killpg(process_group, SIGKILL).

[Gudge (MGudgin)]

Addressed the remaining cold-cache advisory race in f4101a9. I integrated the Copilot-authored fallback commit, then strengthened it by serializing advisory cache population and rechecking the cache after ownership is acquired. Queued callers now reuse the first successful version before launching another probe; execution validation remains intentionally uncached. Added a deterministic concurrent regression, and Rust format, targeted tests (54 bwrap_common + 556 wxc_common), and Linux-target clippy all pass.

[Gudge (MGudgin)]

Copilot review

[Copilot]

🟡 Human review recommended

Cross-thread and process-group supervision has subtle lifecycle behavior that merits final human validation despite extensive regression coverage.

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

Concurrent failed Rust probes can queue outside the timeout and exceed the intended per-call bound.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

Copilot review

The concurrent cold-cache fix in f4101a9 is sound.

probe_bwrap_cached double-check locking: the fast path reads the OnceLock without any lock; the slow path acquires CACHED_BWRAP_PROBE_LOCK, re-checks under the lock, runs one probe, and populates the cache. Queued advisory callers pick up the first successful result without launching a redundant probe.

Lock ordering — no deadlock: the only lock order is CACHED_BWRAP_PROBE_LOCK → probe_gate. Execution validation (bwrap_runner.rs:93) goes directly to probe_gate and never touches CACHED_BWRAP_PROBE_LOCK, so the order can't be reversed.

Failure handling: a failed probe never populates the cache, _guard releases the mutex on normal return and on panic, and the next caller retries the full slow path — the intended retryable-failure behaviour.

Node.js worker: the PID-safety protocol is correct — publish() (no signal) on 'close'; complete() (kill + publish) while the anchor is live. The Atomics.compareExchange(header, 0, 0, -1) fence is sound: if the worker already published (header == 1), the CAS fails and the parent consumes the worker's result instead of a stale timeout.

Regression test: concurrent_cached_callers_reuse_the_first_success correctly proves the contract — thread 1 signals started_tx from inside the probe closure while holding the mutex; thread 2 blocks on the mutex; when thread 1 sets the cache and releases, thread 2's re-check under the lock short-circuits without calling its panicking closure.

No correctness issues found. Cross-platform process-group supervision and timeout concurrency still warrant final human validation.

[Gudge (MGudgin)]

Copilot review

[Copilot]

🟡 Human review recommended

The low-level cross-process timeout and process-group cleanup logic warrants final human validation despite strong regression coverage.

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

The anchor can signal a reused PID or process group after its helper has already been reaped.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

• Files reviewed: 15/15 changed files
• Comments generated: 1
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[Gudge (MGudgin)]

Copilot review

[Copilot]

🟡 Not ready to approve

The Node fallback wait can exceed its documented timeout by 1.5 seconds.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

sdk/node/src/platform.ts:475

• The fallback wait exceeds the configured probe deadline by 1.5 seconds. If the worker fails before publishing (for example, it cannot load its script), this synchronous API blocks for about 6.5 seconds but reports timed out after 5000ms, contrary to the documented five-second bound. Wait only for the caller's remaining timeout; the unref'd worker can still observe the cancellation marker and perform cleanup asynchronously.

  const waitResult = Atomics.wait(header, 0, 0, timeoutMs + 1500);

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

Copilot review

Addressed the fallback wait overrun in the latest commit.

The supervision deadlines are now derived from the caller's budget rather than stacked on top of it: _bwrapProbeDeadlines(timeoutMs) yields a probe deadline (what the anchor/helper enforce on bwrap) and an earlier-than-caller publish deadline (when the worker publishes its own timeout). The parent now waits exactly timeoutMs instead of timeoutMs + 1500, so _runBwrapVersionCommand(timeoutMs) returns inside the documented bound even if every inner layer stalls. Budgets smaller than twice the margin split in half instead of starving the probe.

Two regressions were added: one asserting the deadline nesting (probe < publish <= caller across 5000/1000/100/2 ms), and a Linux subprocess test where a hanging wrapper must return timed out after 1000ms in under 1500 ms.

[Gudge (MGudgin)]

Copilot review

[Copilot]

🟡 Human review recommended

Low-level cross-process timeout, signalling, and reaping behavior warrants final human review.

Review details

Suppressed comments (1)

sdk/node/src/platform.ts:476

• This lifecycle guarantee does not match the implementation: complete() publishes immediately (and the caller then unref()s the worker), while the anchor performs group teardown asynchronously afterward. The later comment at lines 523–525 explicitly relies on the worker remaining alive for that cleanup. Please document the actual ordering so future changes do not assume that a returned result proves descendants have already been reaped.

 * Run `bwrap --version` beneath a detached Node sentinel that anchors the
 * process group. A child helper performs asynchronous, bounded I/O; the
 * supervising worker terminates the sentinel-owned group before publishing
 * the helper's result so descendants cannot outlive the probe.

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

Copilot review

Addressed in c288b96. The Node Bubblewrap probe lifecycle comment now documents the actual ordering: the worker publishes once the helper has a bounded result/timeout, and the still-live anchor remains responsible for tearing down the process group afterward. Validated with npm run build:test-unit, targeted platform.test.js, secret scanning, and parallel_validation.

[Copilot]

🟡 Not ready to approve

The Node worker publishes results before process-group cleanup completes, allowing descendants to outlive the probe.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

• Files reviewed: 15/15 changed files
• Comments generated: 1
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

The public direct execution path can now bypass Bubblewrap validation and its uncached availability probe.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

src/core/wxc_common/src/sandbox_process.rs:415

• ScriptRunner::execute remains publicly callable (including through the public ResolvedRunner.runner), but this now routes Bubblewrap directly to spawn_validated. Unlike the previous spawn call, that skips validate_common, the empty-script check, and the uncached PATH/version probe unless the caller happened to invoke validate_runner first. Keep execute safe for direct callers and use a separate private validated execution path from an overridden run, or otherwise encode/enforce the validation precondition.

        let mut child = match self.0.spawn_validated(request, logger, StdioMode::Inherit) {

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Gudge (MGudgin)]

Copilot review

[Copilot]

🟡 Not ready to approve

The probe gate can grant a permit after its deadline and start an unintended retry.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (2)

src/backends/bubblewrap/common/src/bwrap_version.rs:99

• Check the deadline before granting an idle gate. A waiter can wake after the previous owner releases just past its deadline; because availability is checked first, it still acquires the permit and, if the first probe failed, starts a new worker with an already-expired deadline. That violates the per-call bound and the no-retry-after-deadline guarantee.

            if !*in_flight {
                *in_flight = true;
                return true;
            }

docs/bwrap-support/bubblewrap-backend.md:35

• This retry claim is not true for the Node SDK: getPlatformSupport() caches the entire result, including Bubblewrap failures, for the module lifetime (sdk/node/src/platform.ts:156-165). Scope the success-only caching statement to Rust and document the Node behavior so users do not expect remediation to be detected without restarting.

  advisory results are cached for the process lifetime; failures are not
  cached, so availability is retried after the host is remediated. Execution

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

Copilot review

Addressed in 2fae01d. ProbeGate::acquire_until now checks the caller deadline before granting an idle permit and treats condvar timeout as terminal, so an expired caller cannot start a retry. Added a focused expired-deadline regression and validated with cargo fmt --all -- --check, cargo test -p bwrap_common, cargo clippy -p bwrap_common --all-targets -- -D warnings, secret scanning, and parallel validation.

[Copilot]

🟡 Not ready to approve

The backend documentation incorrectly claims failed probes are never cached, while the Node SDK caches failed platform support for the module lifetime.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

docs/bwrap-support/bubblewrap-backend.md:35

• This new caching statement is not true for the Node SDK: getPlatformSupport() still stores the entire PlatformSupport object for the module lifetime (sdk/node/src/platform.ts:156-162), so an unavailable Bubblewrap result remains cached and is not retried after remediation. Please either qualify these sentences as Rust-executor behavior or avoid caching failed Bubblewrap probes in the SDK as well.

  advisory results are cached for the process lifetime; failures are not
  cached, so availability is retried after the host is remediated. Execution
  validation probes again immediately before launch so a changed PATH target
  cannot reuse an earlier advisory result.

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Gudge (MGudgin)]

Addressed the remaining suppressed findings in defcc3f:

• Runner::execute uses the backend's validation-safe public spawn path for direct callers, while the overridden Runner::run uses a private validated path after shared/backend validation and dry-run handling to avoid duplicate probes.
• Bubblewrap documentation distinguishes Rust success-only advisory caching and uncached execution validation from the Node SDK's module-lifetime cache of the complete platform-support result.

Validation: cargo fmt --all -- --check, cargo check --workspace --all-targets, cargo clippy -p wxc_common --all-targets -- -D warnings, and cargo test -p wxc_common (562 passed).

Copilot review

[Copilot]

🟡 Not ready to approve

The Node probe’s setup occurs outside its advertised caller-visible timeout.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

sdk/node/src/platform.ts:491

• The caller-visible deadline starts too late: script resolution, the 1 MiB allocation, and new Worker(...) all happen before Atomics.wait(..., timeoutMs). If the worker never publishes, this call takes setup time plus the full timeout (and synchronous script resolution/worker startup can itself stall), contradicting the documented total wall-clock bound. Compute an absolute deadline at function entry and use only the remaining budget for setup-derived deadlines and the final wait; avoid synchronous filesystem work on this path if the bound must be strict.

  const { probeTimeoutMs, publishTimeoutMs } = _bwrapProbeDeadlines(timeoutMs);
  const shared = new SharedArrayBuffer(12 + BWRAP_HELPER_RESULT_BYTES);
  const header = new Int32Array(shared, 0, 3);
  let worker: Worker;
  try {
    worker = new Worker(resolveBwrapProbeScript('bwrap-probe-worker.js'), {

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Gudge (MGudgin)]

Addressed the caller-visible setup deadline finding in 076761c. _runBwrapVersionCommand() establishes a monotonic absolute deadline at function entry, resolves probe-script locations outside the per-call path, derives worker budgets from remaining setup time, and limits Atomics.wait to the budget left after worker construction. A controlled non-publishing worker regression proves worker setup time is subtracted from the final wait.

Validation: npm run build, five consecutive platform unit-test runs, npm test (224 passed, 19 skipped), and npm pack --dry-run.

Copilot review

[Copilot]

🟡 Not ready to approve

Node probe helpers remain vulnerable to NODE_OPTIONS interference and cannot be reliably located in bundled ESM applications.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (2)

sdk/node/src/bwrap-probe-worker.ts:75

• The anchor inherits NODE_OPTIONS, so common instrumentation such as NODE_OPTIONS=--require <preload> also runs inside this protocol process. If that preload writes a banner to stdout, the worker parses that first line as the helper JSON, kills the probe, and reports Bubblewrap unavailable; a hanging/noisy preload can likewise consume the probe budget before the anchor code runs. Launch this internal Node process with NODE_OPTIONS cleared (the helper will then inherit the sanitized environment) so application-level Node hooks cannot corrupt the control channel.

      { detached: true, stdio: ['ignore', 'pipe', 'pipe'] },

sdk/node/src/platform.ts:33

• This asset lookup fails when the SDK is bundled into an ESM application. In that case __dirname is the bundle directory (which does not contain these emitted helper files), while getSdkPackageRoot() calls require.resolve; require is unavailable in native ESM and its catch fallback points beside the application bundle rather than to @microsoft/mxc-sdk. Unlike native-binary discovery, there is no environment override for these scripts, so a bundled Linux consumer will report Bubblewrap unavailable even when the installed package and bwrap are valid. Resolve the package through createRequire(import.meta.url) or provide an explicit helper-asset location that bundlers can preserve.

const bwrapProbeScriptDirectory = fs.existsSync(
  path.join(__dirname, 'bwrap-probe-worker.js'),
)
  ? __dirname
  : path.join(getSdkPackageRoot(), 'dist');

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

Addressed the caller-visible setup deadline finding in 076761c. _runBwrapVersionCommand() establishes a monotonic absolute deadline at function entry, resolves probe-script locations outside the pe...

Addressed in the latest commit. getSdkPackageRoot()'s require.resolve was ambient CommonJS require in this ESM module, so it always threw ReferenceError and silently used the __dirname-relative fallback instead — now uses createRequire(import.meta.url) so bundled consumers resolve the real package location. The anchor and worker now strip NODE_OPTIONS from the environment passed to the spawned Node subprocesses so a host's loader/inspector flags can't interfere with probe supervision.

[Copilot]

🟡 Human review recommended

Cross-process timeout, signal, and process-group supervision warrants final human validation despite comprehensive coverage.

Review details

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Human review recommended

Process-group ownership, signal handling, and cross-runtime timeout behavior warrant final human validation despite comprehensive tests.

Review details

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

The validated-spawn hook permits validation bypass, and undrained anchor stderr can stall cleanup.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

• Files reviewed: 15/15 changed files
• Comments generated: 2
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.