Skip to content

Add dedicated --allow-testing-features gate for builtinTestServer#542

Merged
MGudgin merged 1 commit into
mainfrom
user/gudge/allow-testing-features-gate
Jun 24, 2026
Merged

Add dedicated --allow-testing-features gate for builtinTestServer#542
MGudgin merged 1 commit into
mainfrom
user/gudge/allow-testing-features-gate

Conversation

@MGudgin

@MGudgin MGudgin commented Jun 19, 2026

Copy link
Copy Markdown
Member

Summary

This PR fixes a fail-open parity gap where the builtin test proxy
(network.proxy.builtinTestServer) could be activated on the Windows
process-container backends (AppContainer and BaseContainer) without any gate,
while bubblewrap gated it behind the overloaded --experimental flag. It
introduces a dedicated testing-only axis, --allow-testing-features, enforced
uniformly across all backends.

The root cause was that --experimental is a single overloaded axis (backend
admission + experimental features + testing-only helpers) — on the Windows
process-container family it is even self-contradictory as a gate, since it also
flips AppContainer to BaseContainer. The fix decouples "not-for-production
testing scaffolding" into its own flag.

Details

  • Add ExecutionRequest.testing_features_enabled, wired from a new
    --allow-testing-features flag on wxc-exec, lxc-exec, and mxc-exec-mac.
  • Enforce the gate centrally in validate_common (called by ScriptRunner::run
    for every backend): reject network.proxy.builtinTestServer unless the flag
    is set. This is a distinct axis from --experimental ("unstable/new") versus
    "not-for-production testing scaffolding".
  • Remove bubblewrap's now-redundant local --experimental gate; the central
    check covers it.
  • The SDK forwards --allow-testing-features automatically when a one-shot
    policy sets builtinTestServer, preserving SDK ergonomics while the direct
    CLI/wxc-exec surface stays fail-closed.
  • wxc-test-driver auto-passes the flag for configs that use
    builtinTestServer; the bwrap proxy test script is updated.
  • Docs updated: bubblewrap backend, examples, schema, policy v1, and SDK type
    JSDoc.

Tests

  • cargo fmt --check, cargo check --workspace --all-targets, and
    cargo clippy --workspace --all-targets -- -D warnings all pass.
  • Rust unit tests pass, including two new validate_common tests (reject
    without the flag / accept with it) and an updated bwrap test (wxc_common 340
    passed; plus wxc, lxc, appcontainer_common, wxc_test_driver).
  • SDK: tsc build clean and npm test 178 passed, including two new
    flag-forwarding tests.
  • macOS mxc-exec-mac compiles on Windows but its runtime path was not
    exercised on this host.
Microsoft Reviewers: Open in CodeFlow

Copilot AI review requested due to automatic review settings June 19, 2026 19:52
@MGudgin MGudgin requested a review from a team as a code owner June 19, 2026 19:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a dedicated --allow-testing-features CLI flag and a corresponding ExecutionRequest.testing_features_enabled bit to fail-closed gate the testing-only network.proxy.builtinTestServer feature across one-shot runners, replacing the prior ad-hoc/overloaded gating (notably Bubblewrap’s --experimental-based check).

Changes:

  • Add --allow-testing-features to native executors and plumb it into ExecutionRequest.testing_features_enabled.
  • Enforce network.proxy.builtinTestServer gating centrally in wxc_common::validator::validate_common, and remove Bubblewrap’s redundant local gate.
  • Forward the flag automatically from the SDK (and update docs/tests/scripts to reflect the new gating).
Show a summary per file
File Description
tests/scripts/run_bwrap_network_proxy_test.sh Updates Bubblewrap proxy test invocation to pass --allow-testing-features.
src/testing/wxc_test_driver/src/main.rs Auto-adds --allow-testing-features when configs appear to opt into builtinTestServer.
src/core/wxc/src/main.rs Adds --allow-testing-features CLI flag and plumbs into one-shot ExecutionRequest.
src/core/wxc_common/src/validator.rs Centralized validation rejects builtinTestServer unless testing features are enabled; adds unit tests.
src/core/wxc_common/src/models.rs Adds testing_features_enabled to ExecutionRequest with documentation.
src/core/wxc_common/src/config_parser.rs Defaults testing_features_enabled to false in parsed requests.
src/core/mxc_darwin/src/main.rs Adds --allow-testing-features flag and plumbs into request.
src/core/lxc/src/main.rs Adds --allow-testing-features flag and plumbs into request.
src/backends/bubblewrap/common/src/bwrap_runner.rs Removes backend-local builtinTestServer gating and adjusts runner test accordingly.
sdk/tests/unit/sandbox.test.ts Adds unit coverage ensuring the SDK forwards --allow-testing-features when needed.
sdk/src/types.ts Updates type/JSDoc to document the new gate and SDK auto-forwarding.
sdk/src/helper.ts Implements SDK auto-forwarding of --allow-testing-features when builtinTestServer is set.
docs/schema.md Documents builtinTestServer as requiring --allow-testing-features.
docs/sandbox-policy/v1/policy.md Updates policy docs to mention the new gate for builtinTestServer.
docs/examples.md Expands examples/docs to explain the new testing-only flag and its rationale.
docs/bwrap-support/bubblewrap-backend.md Updates Bubblewrap docs to reflect the new flag gating.

Copilot's findings

  • Files reviewed: 16/16 changed files
  • Comments generated: 2

Comment thread src/testing/wxc_test_driver/src/main.rs Outdated
Comment thread docs/sandbox-policy/v1/policy.md Outdated
@MGudgin

MGudgin commented Jun 19, 2026

Copy link
Copy Markdown
Member Author

Addressed both review comments in f726343:

  • wxc-test-driver: narrowed the match from the bare "builtinTestServer" substring to the concrete "builtinTestServer": true JSON pattern (both spacings), so it can't false-positive on the string appearing in process.commandLine and mask fail-closed coverage.
  • policy v1 docs: added { localhost: <port> } as a proxy option in both the type definition and the network table to match the schema/SDK wire format.

Comment thread src/testing/wxc_test_driver/src/main.rs Outdated
Comment thread sdk/src/helper.ts
jsidewhite
jsidewhite previously approved these changes Jun 23, 2026

@jsidewhite jsidewhite left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

MGudgin pushed a commit that referenced this pull request Jun 24, 2026
…er JSON

This commit addresses jsidewhite's review feedback on PR #542.

Details

* SDK no longer auto-forwards --allow-testing-features. Auto-forwarding
  whenever a policy set builtinTestServer undermined the gate (requesting
  the feature silently enabled the gate guarding it). Callers now opt in
  explicitly via a new `allowTestingFeatures` SandboxSpawnOptions flag,
  mirroring the existing `experimental` gate; resolveExecutableAndArgs
  throws a clear error if builtinTestServer is used without it.
* wxc-test-driver now parses each config as JSON and reads the real
  `containment` and `network.proxy.builtinTestServer` fields instead of
  sniffing for substrings (added serde_json dependency).
* Updated SDK type JSDoc, examples.md, and policy v1 docs to describe the
  explicit opt-in. Updated SDK unit tests (opt-in forwards the flag, and a
  new test asserts it throws without the option) and the four integration
  tests that use builtinTestServer.

Tests

* cargo fmt --check and cargo check -p wxc_test_driver pass.
* SDK tsc build clean; npm test 179 passed (4 platform-skipped),
  including the updated/added testing-features gate tests. Integration
  tests are platform-gated and were not run on this host.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@jsidewhite jsidewhite dismissed their stale review June 24, 2026 17:34

revoking review

@jsidewhite jsidewhite left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

This PR fixes a fail-open parity gap where the builtin test proxy
(network.proxy.builtinTestServer) could be activated on the Windows
process-container backends (AppContainer and BaseContainer) without any
gate, while bubblewrap gated it behind the overloaded --experimental
flag. It introduces a dedicated testing-only axis,
--allow-testing-features, enforced uniformly across all backends.

Details

* Add ExecutionRequest.testing_features_enabled, wired from a new
  --allow-testing-features flag on wxc-exec, lxc-exec, and mxc-exec-mac.
* Enforce the gate centrally in validate_common (called for every
  backend): reject network.proxy.builtinTestServer unless the flag is
  set. This is a distinct axis from --experimental ("unstable/new")
  versus "not-for-production testing scaffolding".
* Remove bubblewrap's now-redundant local --experimental gate; the
  central check covers it.
* SDK requires an explicit opt-in: callers set a new allowTestingFeatures
  SandboxSpawnOptions flag (mirroring the existing experimental gate).
  resolveExecutableAndArgs throws a clear error if builtinTestServer is
  used without it, then forwards --allow-testing-features. Auto-forwarding
  was dropped because it would undermine the gate (requesting the feature
  would silently enable the gate guarding it).
* wxc-test-driver parses each config as JSON and reads the real
  containment and network.proxy.builtinTestServer fields instead of
  sniffing substrings.
* Docs updated: bubblewrap backend, examples, schema, policy v1, and SDK
  type JSDoc.

Tests

* cargo fmt --check, cargo check --workspace --all-targets, and
  cargo clippy --workspace --all-targets -- -D warnings all pass.
* Rust unit tests pass, including new validate_common gate tests and an
  updated bwrap test.
* SDK: tsc build clean and npm test passes, including the testing-features
  gate tests (opt-in forwards the flag; throws without it). Integration
  tests are platform-gated and were not run on this host.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@MGudgin MGudgin force-pushed the user/gudge/allow-testing-features-gate branch from 1957dfe to a4d6cb3 Compare June 24, 2026 18:05
@MGudgin MGudgin merged commit e1d1e4c into main Jun 24, 2026
18 checks passed
@MGudgin MGudgin deleted the user/gudge/allow-testing-features-gate branch June 24, 2026 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants