Resolve the command before the request is parsed by MGudgin · Pull Request #969 · microsoft/mxc

Gudge (MGudgin) · 2026-08-20T18:15:16Z

Resolve the command before the request is parsed

This PR changes CLI command handling so trailing commands are resolved and
inserted before request parsing, and Rust SDK requests receive their command at
construction time. It preserves behavior for valid requests and requests
without a trailing CLI command; command-preparation failures remain entry-point
errors when the command and policy are independently invalid.

Details

Probe the request phase and reuse one duplicate-preserving raw source pass to
select backend-specific quoting and edit process.commandLine.
Keep typed validation authoritative for the effective document, route request
errors through one production helper, and log replacements only after parsing
succeeds.
Require commands in request-builder APIs, remove post-build script mutation,
and migrate SDK and FFI callers with direct validation coverage.
Document native CLI syntax, state-aware exec restrictions, and the distinction
between complete schema-valid requests and CLI templates.

Tests

cargo fmt --all -- --check
cargo check --workspace --all-targets
cargo clippy --workspace --all-targets -- -D warnings
cargo test --workspace
cargo check -p mxc_engine --tests --target x86_64-apple-darwin

Microsoft Reviewers: Open in CodeFlow

azure-pipelines · 2026-08-20T18:15:26Z

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

Copilot

Pull request overview

Moves CLI command resolution before request parsing and makes Rust SDK requests complete at construction.

Changes:

Adds backend/phase probing and command splicing.
Requires commands in request-builder APIs and removes set_script.
Updates tests, FFI callers, and documentation.

Show a summary per file

File	Description
`src/ffi/mxc_ffi/src/streaming.rs`	Passes commands during request construction.
`src/ffi/mxc_ffi/src/lib.rs`	Updates run-to-completion request creation.
`src/core/wxc/src/main.rs`	Integrates pre-parse CLI command resolution and tests.
`src/core/wxc_common/src/state_aware_dispatch.rs`	Exposes backend-prefix resolution internally.
`src/core/wxc_common/src/splice.rs`	Adds JSON command splicing.
`src/core/wxc_common/src/probe.rs`	Adds backend and phase probes.
`src/core/wxc_common/src/lib.rs`	Registers new internal modules.
`src/core/wxc_common/src/config_parser.rs`	Applies CLI commands before typed parsing.
`src/core/mxc-sdk/tests/streaming.rs`	Updates streaming test request construction.
`src/core/mxc-sdk/tests/streaming_processcontainer.rs`	Updates ProcessContainer streaming test.
`src/core/mxc-sdk/tests/sdk_helpers.rs`	Updates SDK helper tests.
`src/core/mxc-sdk/tests/sandbox.rs`	Updates sandbox tests.
`src/core/mxc-sdk/src/lib.rs`	Revises SDK examples.
`src/core/mxc-sdk/README.md`	Documents command-at-build usage.
`src/core/mxc_engine/src/policy.rs`	Requires commands in builder APIs.
`src/core/mxc_engine/src/dispatch.rs`	Updates dispatch tests.

Review details

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Files reviewed: 16/16 changed files
Comments generated: 3
Review effort level: Balanced

Darren Hoehna (dhoehna)

Besides the comments about de-duplication the code is sound. I am concerned to see duplicate methods for one-shot and state-aware. Merging those paths is a future issue.

This PR updates the version-specific parser plan for the current state: Phase 5 is fully merged, Phase 6 is awaiting signoff in PR #968, and Phase 6.5 is rebased onto it as a single commit. Details * Rewrite the status paragraph: #966 merged, #968 awaiting a final signoff and now carrying the directional networking port, Phase 6.5 rebased onto it, and Phase 7a open as PR #969. * Mark steps 1 through 5 of the publication sequence with their outcomes, and record that the step 2 port landed inside #968 rather than separately because a review of that pull request identified the same gap. * Add a "Phase 6.5 as rebased" section describing what the branch contains and the two version-marker errors the rebase surfaced, both found by a failing test rather than by inspection. * Correct the development version suffix from 0.9.0-dev to 0.9.0-alpha throughout the contract-side text, leaving the rolling-artifact references unchanged since -dev remains correct there. * Mark the fixture schema reference and the registry path items of the immediate remediation as done, noting that the stable artifact itself is published by the old stack rather than by this work. Tests * Documentation-only change; no build, lint, or test gate applies. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2a8e0d88-21dd-48e5-8e44-df2393b8a664

Copilot

Review details

Suppressed comments (4)

src/core/mxc-sdk/tests/streaming.rs:36

This identifier is misspelled, so the mxc-sdk streaming integration-test target does not compile. Use the existing command parameter.

    let request = build_request(&policy, commmand, None).expect("build_request should succeed");

src/core/wxc_common/src/splice.rs:14

Parsing into serde_json::Value before the real typed parser silently collapses duplicate object keys. With a CLI command, a request containing duplicate process, network, or other policy sections is therefore accepted using the last value instead of being rejected as a duplicate field, potentially changing the enforced policy. Preserve the original structure during splicing or explicitly reject duplicate keys before converting to Value.

    let mut doc: Value = serde_json::from_str(json).ok()?;

src/core/wxc_common/src/splice.rs:22

Serializing the modified DOM into a compact string moves every subsequent typed parse error to line 1, contradicting parse_mxc_request_json's source-location guarantee. For example, an invalid multiline filesystem field with a CLI command no longer reports its original line/column. Splice into the original source text (or retain an offset map) rather than reserializing the whole document.

    Some(Spliced {
        json: serde_json::to_string(&doc).ok()?,

src/core/wxc_common/src/config_parser.rs:254

Command-rendering failures are always classified as Decode, even after phase identified a state-aware exec request. Previously this path emitted a state-aware stdout envelope; it now emits the pre-discrimination stderr diagnostic, contrary to ParseError's routing contract. Classify this error as StateAware when phase.is_some() (and apply the same rule to the empty-command branch below).

    let command = cmdline_from_argv_for_context(argv, context).map_err(|e| {
        ParseError::Decode(WxcError::ConfigParse(format!(
            "invalid CLI command override: {e}"
        )))
    })?;

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

This PR updates the version-specific parser plan for the current state: Phase 5 is fully merged, Phase 6 is awaiting signoff in PR #968, and Phase 6.5 is rebased onto it as a single commit. Details * Rewrite the status paragraph: #966 merged, #968 awaiting a final signoff and now carrying the directional networking port, Phase 6.5 rebased onto it, and Phase 7a open as PR #969. * Mark steps 1 through 5 of the publication sequence with their outcomes, and record that the step 2 port landed inside #968 rather than separately because a review of that pull request identified the same gap. * Add a "Phase 6.5 as rebased" section describing what the branch contains and the two version-marker errors the rebase surfaced, both found by a failing test rather than by inspection. * Correct the development version suffix from 0.9.0-dev to 0.9.0-alpha throughout the contract-side text, leaving the rolling-artifact references unchanged since -dev remains correct there. * Mark the fixture schema reference and the registry path items of the immediate remediation as done, noting that the stable artifact itself is published by the old stack rather than by this work. Tests * Documentation-only change; no build, lint, or test gate applies. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2a8e0d88-21dd-48e5-8e44-df2393b8a664

Copilot

Review details

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

This PR updates the version-specific parser plan for the current state: Phase 5 is fully merged, Phase 6 is awaiting signoff in PR #968, and Phase 6.5 is rebased onto it as a single commit. Details * Rewrite the status paragraph: #966 merged, #968 awaiting a final signoff and now carrying the directional networking port, Phase 6.5 rebased onto it, and Phase 7a open as PR #969. * Mark steps 1 through 5 of the publication sequence with their outcomes, and record that the step 2 port landed inside #968 rather than separately because a review of that pull request identified the same gap. * Add a "Phase 6.5 as rebased" section describing what the branch contains and the two version-marker errors the rebase surfaced, both found by a failing test rather than by inspection. * Correct the development version suffix from 0.9.0-dev to 0.9.0-alpha throughout the contract-side text, leaving the rolling-artifact references unchanged since -dev remains correct there. * Mark the fixture schema reference and the registry path items of the immediate remediation as done, noting that the stable artifact itself is published by the old stack rather than by this work. Tests * Documentation-only change; no build, lint, or test gate applies. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2a8e0d88-21dd-48e5-8e44-df2393b8a664

Copilot

Copilot review overview

Review tier: Balanced
Findings: 2 · 1

New issues introduced by this change (1)

Severity	Finding
	`src/core/wxc_common/src/splice.rs` — `process: null` now bypasses the CLI override: deserializing it as `RawObject` fails, so the…

Pre-existing issues (2)

Severity	Finding
	`src/core/wxc_common/src/config_parser.rs` — These branches now return probe/override errors before the typed parser runs, changing error… View comment
	`src/core/mxc_engine/src/policy.rs` — This migration replaced the test's existing `Some("test-container")` argument with `None`, leaving… View comment

Issues resolved since last review (4)

Severity	Finding
	`src/core/wxc_common/src/splice.rs` — A non-string `process.commandLine` is currently overwritten before typed deserialization, so… View resolved comment
	`src/core/mxc_engine/src/policy.rs` — The API change leaves `tests/isolation_session.rs:109,128` calling this function with the old… View resolved comment
	`src/core/wxc_common/src/config_parser.rs` — Command-rendering failures are always classified as `Decode`, even after `phase` was successfully… View resolved comment
	`src/core/wxc_common/src/splice.rs` — Round-tripping the unvalidated policy through `serde_json::Value` changes the parser's validation… View resolved comment

Suppressed comments (1)

src/core/wxc_common/src/config_parser.rs:242

The state-aware backend probe is surfaced directly, so parser-owned sandboxId failures no longer retain their existing diagnostics. For example, an exec request with "sandboxId": 42 previously went through config_deserialize and reported the sandboxId JSON path; this probe now emits plain serde text before the typed parser runs (and a missing ID also changes from phase exec requires a sandboxId to a new message). Pass through probe failures caused by malformed/missing field values so the typed parser and normal backend resolution preserve the established error text; only use the probe result when it successfully identifies a backend.

            let backend =
                crate::probe::probe_state_aware_backend(json).map_err(ParseError::StateAware)?;

Copilot

Copilot review overview

Review tier: Balanced
Findings: 1 · 1

New issues introduced by this change (1)

Severity	Finding
	`docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md` — This example propagates `apply_cli_command` failures immediately with `?`, contradicting the…

Pre-existing issues (1)

Severity	Finding
	`src/core/wxc_common/src/splice.rs` — The override can now hide an invalid policy command. A non-empty string containing `\u0000`… View comment

Issues resolved since last review (2)

Severity	Finding
	`src/core/wxc_common/src/config_parser.rs` — The Rust-layer architecture example in `docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md:11… View resolved comment
	`src/core/wxc_common/src/splice.rs` — This source-edit strategy does not preserve columns in the caller's policy when the… View resolved comment

Suppressed comments (1)

src/core/wxc_common/src/splice.rs:196

A non-empty policy command containing \u0000 is decoded here and replaced before convert_wire_config can enforce its process.commandLine must not contain null bytes check (config_parser.rs:902-906). Consequently, a malformed policy such as {"process":{"commandLine":"bad\u0000command"}} succeeds whenever a valid CLI command is supplied, whereas the previous parse-then-override flow rejected it. Treat this semantic-invalid string like the invalid command-line types and return None so typed validation retains precedence.

                            let existing: String =
                                serde_json::from_str(command_line_raw.get()).ok()?;
                            !existing.is_empty()

Copilot

Copilot review overview

Review tier: Balanced
Findings: 1

New issues introduced by this change (1)

Severity	Finding
	`src/core/wxc_common/src/config_parser.rs` — `splice_command` returning `None` does not imply the typed parser will reject the document.…

Issues resolved since last review (2)

Severity	Finding
	`docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md` — This example propagates `apply_cli_command` failures immediately with `?`, contradicting the… View resolved comment
	`src/core/wxc_common/src/splice.rs` — The override can now hide an invalid policy command. A non-empty string containing `\u0000`… View resolved comment

Copilot

Copilot review overview

Review tier: Balanced
Findings: 1

New issues introduced by this change (1)

Severity	Finding
	`src/core/wxc_common/src/config_parser.rs` — This logs the override before typed parsing has succeeded. If any unrelated policy field is…

Issues resolved since last review (1)

Severity	Finding
	`src/core/wxc_common/src/config_parser.rs` — `splice_command` returning `None` does not imply the typed parser will reject the document.… View resolved comment

Copilot

Copilot review overview

🟢 Approval recommended

The API migration is complete and the new parsing behavior has comprehensive edge-case coverage.

Review tier: Balanced
Findings: None

Issues resolved since last review (1)

Severity	Finding
	`src/core/wxc_common/src/config_parser.rs` — This logs the override before typed parsing has succeeded. If any unrelated policy field is… View resolved comment

Copilot

Copilot review overview

🔵 Needs a closer look

Core parsing, command quoting, and cross-backend error routing changes warrant final human validation despite strong test coverage.

Review tier: Balanced
Findings: None

This PR records that PR #969 now points to the verified, squashed Phase 7a redux implementation. Details * Note the local and remote branch replacement performed with --force-with-lease. * Replace the obsolete pending-push status in the version-specific parser plan. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 651084c2-3d1e-4824-917a-6994d62feba0

This PR updates the Phase 7a implementation record for the final rebase and the consolidated command-source design. Details * Record PR #969 at rebased tip 2675e62 on origin/main 878936a. * Replace deleted backend-probe names with CommandSource and its exhaustive containment parity test. Tests * git diff --check Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 651084c2-3d1e-4824-917a-6994d62feba0

This PR updates the parser roadmap for the squashed Phase 7.2 implementation and its stacked pull request. Details * Record Phase 7.2 on the phase7b branch at 401ba0c. * Link PR #1091 and its Phase 7a base PR #969 in the status and delivery table. Tests * git diff --check Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 651084c2-3d1e-4824-917a-6994d62feba0

Branden Bonaby (bbonaby) · 2026-09-03T21:51:44Z

@@ -79,6 +78,7 @@ process_container.capture_denials = Some(CaptureDenials::default());
 let request = build_request_with_containment(
    &policy,
    &Containment::ProcessContainer(process_container),
+    "echo hello",
    None,
 )?;
 # Ok::<(), mxc_sdk::Error>(())
@@ -235,9 +235,8 @@ let policy = SandboxPolicy {
    ui: None,
    timeout_ms: None,
 };
-let mut request = build_request(&policy, None)?;
-request.set_script("cat"); // echoes stdin until EOF
-
+// echoes stdin until EOF
+let request = build_request(&policy, "cat", None)?;
 let mut proc = spawn_sandbox(request)?;
 let mut stdin = proc.take_stdin().unwrap();
 let mut stdout = proc.take_stdout().unwrap();
@@ -419,8 +418,13 @@ use mxc_sdk::{
 #     filesystem: None, network: None, ui: None, timeout_ms: None,
 # };
 let wslc = WslcSection { image: "python:3.12".to_string(), ..Default::default() };
-let mut request = build_request_with_containment(&policy, &Containment::Wslc(wslc), None)?;
-request.set_script("python3 -c 'print(42)'").set_experimental(true);
+let mut request = build_request_with_containment(
+    &policy,
+    &Containment::Wslc(wslc),
+    "python3 -c 'print(42)'",
+    None,
+)?;
+request.set_experimental(true);
 let output = run(request)?;
 # Ok::<(), mxc_sdk::Error>(())


note: FYI Caarlos this is a breaking (at compile time) change for GitHub copilot in the Rust SDK. Basically you no longer build the request, and then set the script.

With the new API, you would add in the script to the new build request API directly instead of after. Also Gudge (@MGudgin), we probably would want to make sure we have parity in the other SDKs right? Same kind of calling convention I mean.

Thanks for flagging the compatibility impact. The other public SDKs already take the command at the operation boundary: Node passes the script to spawnSandbox / spawnSandboxAsync, and C# passes the command to MxcSandbox.Run / RunAsync. Requiring the Rust builder to receive the command brings its construction flow into line with those surfaces, so I do not think an additional Node or C# API change is needed here.

Copilot

Copilot review overview

🟡 Changes recommended

Two updated documents incorrectly claim command-less templates are invalid under every supported JSON schema.

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

Review tier: Balanced
Findings: 2

New issues introduced by this change (2)

Severity	Finding
	`README.md` — The “not schema-valid” claim is not true for every supported schema:…
	`docs/schema.md` — This conflates runtime completeness with JSON Schema validity. In particular, the immutable 0.8…

This PR changes CLI command handling so trailing commands are resolved and inserted before request parsing, and Rust SDK requests receive their command at construction time. It preserves behavior for valid requests and requests without a trailing CLI command; command-preparation failures remain entry-point errors when the command and policy are independently invalid. Details * Probe the request phase and reuse one duplicate-preserving raw source pass to select backend-specific quoting and edit process.commandLine. * Keep typed validation authoritative for the effective document, route request errors through one production helper, and log replacements only after parsing succeeds. * Require commands in request-builder APIs, remove post-build script mutation, and migrate SDK and FFI callers with direct validation coverage. * Document native CLI syntax, state-aware exec restrictions, and the distinction between independently executable requests and CLI templates. Tests * cargo fmt --all -- --check * cargo check --workspace --all-targets * cargo clippy --workspace --all-targets -- -D warnings * cargo test --workspace * cargo check -p mxc_engine --tests --target x86_64-apple-darwin Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0f04437d-4a05-487a-b402-4ed4c774e8e0

Copilot

Copilot review overview

🔵 Needs a closer look

The new splice path regresses trailing-command templates containing "process": null.

Review tier: Balanced
Findings: None

Issues resolved since last review (2)

Severity	Finding
	`docs/schema.md` — This conflates runtime completeness with JSON Schema validity. In particular, the immutable 0.8… View resolved comment
	`README.md` — The “not schema-valid” claim is not true for every supported schema:… View resolved comment

Suppressed comments (1)

src/core/wxc_common/src/splice.rs:172

A trailing command no longer completes a template with "process": null. The previous relaxed parse treated this as an absent optional process and applied the CLI command afterward; this splice instead returns None, so typed parsing now fails with “'process' section is required.” Handle JSON null like a missing process while leaving other non-object values for typed validation.

            MemberMatch::Unique(process_raw) => {
                let process_source = process_raw.get();
                let process: RawObject<'_> = serde_json::from_str(process_source).ok()?;

Gudge (MGudgin) requested review from a team and a balanced review from Copilot August 20, 2026 18:15

Gudge (MGudgin) requested a review from a team as a code owner August 20, 2026 18:15

Copilot AI reviewed Aug 20, 2026

View reviewed changes

Comment thread src/core/wxc_common/src/splice.rs Outdated

Comment thread src/core/wxc_common/src/config_parser.rs Outdated

Comment thread src/core/mxc-sdk/tests/streaming.rs Outdated