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 resolves CLI commands before request parsing, preserves original policy
diagnostics and typed-parser error precedence, and requires SDK requests to
carry their command at construction time.

Details

Probe request phase and backend before parsing so commands use the correct
quoting and error-routing context.
Splice CLI commands with source-preserving raw-value edits and remap typed
error coordinates to the caller's original policy.
Defer CLI-specific failures until typed request validation succeeds, using a
synthetic command only when validation needs the CLI-supplied command.
Treat an explicit null process section as absent when the CLI supplies the
command, without logging a policy-command override.
Preserve invalid policy commands for typed validation rather than allowing a
CLI override to hide embedded NUL characters.
Fail closed when a typed process representation cannot be safely source-
spliced instead of silently retaining the policy command.
Require commands in request-builder APIs, remove post-build script mutation,
and update the Rust architecture guidance.

Tests

cargo fmt --all -- --check
cargo check --workspace --all-targets
cargo clippy --workspace --all-targets -- -D warnings
cargo test --workspace

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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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: 2 · 2

New issues introduced by this change (3)

Severity	Finding
	`src/core/wxc_common/src/splice.rs` — This source-edit strategy does not preserve columns in the caller's policy when the…
	`src/core/mxc_engine/src/policy.rs` — Add a regression test for the newly documented empty-script rejection. The existing migrated tests…
	`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…

Pre-existing issues (1)

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

Issues resolved since last review (2)

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

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` — The override can now hide an invalid policy command. A non-empty string containing `\u0000`…

Pre-existing issues (2)

Severity	Finding
	`src/core/wxc_common/src/splice.rs` — This source-edit strategy does not preserve columns in the caller's policy when the… View comment
	`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 comment

Issues resolved since last review (2)

Severity	Finding
	`src/core/mxc_engine/src/policy.rs` — Add a regression test for the newly documented empty-script rejection. The existing migrated tests… View resolved comment
	`src/core/wxc_common/src/splice.rs` — `process: null` now bypasses the CLI override: deserializing it as `RawObject` fails, so the… View resolved comment

Suppressed comments (1)

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

This edit does not preserve diagnostic columns for fields later on the same source line. For example, overriding commandLine in a minified {"process":{"commandLine":"x","timeout":"bad"}} changes the byte offset of timeout, so serde reports its column in the generated document rather than the user's policy. Track/remap the edit offset (as fragment errors already do) or validate errors against the original source before returning them.

                    let range = raw_value_range(process_source, command_line_raw)?;
                    (
                        replace_range(process_source, range, &command),

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

This PR resolves CLI commands before request parsing, preserves original policy diagnostics and typed-parser error precedence, and requires SDK requests to carry their command at construction time. Details * Probe request phase and backend before parsing so commands use the correct quoting and error-routing context. * Splice CLI commands with source-preserving raw-value edits and remap typed error coordinates to the caller's original policy. * Defer CLI-specific failures until typed request validation succeeds, using a synthetic command only when validation needs the CLI-supplied command. * Treat an explicit null process section as absent when the CLI supplies the command, without logging a policy-command override. * Preserve invalid policy commands for typed validation rather than allowing a CLI override to hide embedded NUL characters. * Fail closed when a typed process representation cannot be safely source- spliced instead of silently retaining the policy command. * Require commands in request-builder APIs, remove post-build script mutation, and update the Rust architecture guidance. Tests * cargo fmt --all -- --check * cargo check --workspace --all-targets * cargo clippy --workspace --all-targets -- -D warnings * cargo test --workspace Co-authored-by: Copilot <[email protected]> Copilot-Session: 14e8ceef-7940-4224-aeea-cfb2381ae6b2

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

+    if spliced.replaced_existing {
+        logger.log_line(&format!(
+            "Overriding policy process.commandLine with CLI command: {command}"
+        ));


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

Darren Hoehna (dhoehna) reviewed Aug 20, 2026

View reviewed changes

Comment thread src/core/mxc_engine/src/policy.rs Outdated

Darren Hoehna (dhoehna) reviewed Aug 20, 2026

View reviewed changes

Comment thread src/core/wxc/src/main.rs Outdated

Darren Hoehna (dhoehna) reviewed Aug 20, 2026

View reviewed changes

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

Darren Hoehna (dhoehna) reviewed Aug 20, 2026

View reviewed changes

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

Darren Hoehna (dhoehna) reviewed Aug 20, 2026

View reviewed changes

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

Darren Hoehna (dhoehna) previously approved these changes Aug 20, 2026

View reviewed changes

Copilot started reviewing on behalf of Gudge (MGudgin) August 20, 2026 21:15 View session

Copilot AI review requested due to automatic review settings August 24, 2026 15:33

Gudge (MGudgin) dismissed Darren Hoehna (dhoehna)’s stale review via 6581290 August 24, 2026 15:33

Gudge (MGudgin) force-pushed the user/gudge/version_specific_config_parsers_phase7a branch from 3b44ef1 to 6581290 Compare August 24, 2026 15:33

Copilot started reviewing on behalf of Gudge (MGudgin) August 24, 2026 15:34 View session

Copilot AI reviewed Aug 24, 2026

View reviewed changes

Comment thread src/core/mxc_engine/src/policy.rs

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

Copilot AI review requested due to automatic review settings August 25, 2026 20:38

Copilot started reviewing on behalf of Gudge (MGudgin) August 25, 2026 20:38 View session

Copilot AI reviewed Aug 25, 2026

View reviewed changes

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

Comment thread src/core/mxc_engine/src/policy.rs Outdated

Copilot AI review requested due to automatic review settings August 31, 2026 14:31

Gudge (MGudgin) force-pushed the user/gudge/version_specific_config_parsers_phase7a branch from e26201e to 3a295c0 Compare August 31, 2026 14:31

Copilot started reviewing on behalf of Gudge (MGudgin) August 31, 2026 14:31 View session