Skip to content

Commit e30ac71

Browse files
GudgeCopilot
andcommitted
Add actionable configuration parse errors
This PR adds actionable configuration parse errors: the parser reports malformed JSON separately from typed policy-shape errors and includes the full policy path plus whole-file source location in every diagnostic, without leaking secrets or letting untrusted text corrupt diagnostics or the state-aware stdout response envelope. Details * New config_deserialize module: a path-aware deserialize layer that distinguishes JSON syntax errors from typed policy errors, attaching the full JSON path and whole-file line/column, escaping control and invisible formatting characters (incl. U+2028/U+2029 and bidi overrides), and redacting secret-bearing values. * State-aware per-backend per-phase configs deserialize positionally from the retained request text, so typed errors carry whole-file coordinates matching base-config errors; navigation uses owned-key maps so escaped sibling keys never silently drop the location, with graceful fallback to the value-based path. * Request loading borrows RawValue to discriminate one-shot vs state-aware without building a full untyped AST, masking the experimental subtree to preserve base-config source locations. * Diagnostic routing keeps stdout reserved for the state-aware envelope and script output; parse and dispatch errors go to auxiliary sinks (log file / diagnostic pipe) only and are emitted exactly once. Filesystem-path and schema-version diagnostics escape untrusted text via the shared escape_diagnostic_text helper. * Exec dispatch drops the parsed request (and its retained source text) before the blocking child run and stdio relay. * Docs updated: versioning.md, the state-aware API doc, and the copilot-instructions config-flow description. Tests * Added parser coverage: syntax-vs-typed classification, whole-file line/column (computed, not hardcoded), escaped-sibling-key location preservation, exact column, CRLF line stability, positional secret redaction, source-present locator fallback, malformed-telemetry single auxiliary emission with an empty primary buffer, and a pinned serde_json positioned-error display-suffix contract. * cargo fmt --all -- --check: passed. * cargo clippy -p wxc_common -p wxc --all-targets -- -D warnings: clean. * cargo test -p wxc_common -p wxc: 514 + 26 passed. * node scripts/versioning/check-schema-codegen.js, check-sdk-types-codegen.js, check-schema-versions.js: passed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Generated-with: gpt-5.6-sol Generated-with: claude-opus-4.8 Copilot-Session: dd87e65c-7e38-4d0e-8238-147c2153a0e8
1 parent a101c5e commit e30ac71

15 files changed

Lines changed: 2026 additions & 160 deletions

File tree

.github/copilot-instructions.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ The Rust workspace (`src/`) implements multiple sandboxing backends behind the `
120120

121121
### Config flow
122122

123-
1. User provides JSON config (file or base64) → `config_parser.rs` deserializes into the typed wire model (`wxc_common::wire`) → validates and maps to `ExecutionRequest` (the internal execution model in `models.rs`)
123+
1. User provides JSON config (file or base64) → `config_deserialize.rs` performs path-aware typed deserialization into the wire model (`wxc_common::wire`) → `config_parser.rs` validates and maps it to `ExecutionRequest` (the internal execution model in `models.rs`)
124124
2. `ExecutionRequest` includes the containment backend selection, process config, filesystem/network policies, and optional experimental features
125125
3. The appropriate `ScriptRunner` implementation executes the process and returns `ScriptResponse`
126126

@@ -212,7 +212,7 @@ The workspace is organized into six top-level directories under `src/`:
212212

213213
### Config parser pattern
214214

215-
The parser deserializes JSON directly into the typed wire model (`wxc_common::wire`), the single source of truth for the config shape (it also generates the JSON schema). `config_parser.rs` then maps the wire types to the validated domain structs in `models.rs`. The stable surface uses `deny_unknown_fields` (closed); the `experimental` block is permissive.
215+
The parser deserializes JSON directly into the typed wire model (`wxc_common::wire`), the single source of truth for the config shape (it also generates the JSON schema). All typed config deserialization goes through `config_deserialize.rs`, which distinguishes syntax errors from typed policy errors and adds the complete JSON path plus source line/column when available; state-aware backend errors are prefixed with their full `experimental.<backend>.<phase>` location. `config_parser.rs` then maps the wire types to the validated domain structs in `models.rs`. The stable surface uses `deny_unknown_fields` (closed); the `experimental` block is permissive.
216216

217217
### TypeScript conventions
218218

docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md

Lines changed: 49 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -722,6 +722,24 @@ state-aware mode so `stdout` remains parseable without sentinels. (One-shot disp
722722
keeps its existing `stdout` logger behaviour — the stricter routing applies to
723723
state-aware only.)
724724

725+
Configuration parse-phase failures that occur **after** the request is
726+
discriminated as state-aware (i.e. the `phase` field was recognized) follow the
727+
state-aware contract: the typed `{error}` envelope is the only primary output,
728+
while the human-readable actionable parse diagnostic is written only to
729+
configured auxiliary sinks (`--log-file` and the Windows diagnostic console). It
730+
is not duplicated to the logger's primary console/buffer output, so such a parse
731+
failure does not add stderr noise even with `--debug`. Dispatch-time failures,
732+
including typed per-backend configuration errors, use the same auxiliary-only
733+
diagnostic routing before the executor emits their typed `{error}` envelope.
734+
735+
Failures that occur **before** discrimination is possible — malformed base64,
736+
non-UTF-8 bytes, or JSON so malformed that the `phase` field cannot be read —
737+
cannot be attributed to the state-aware path, so they retain the legacy
738+
behavior: the diagnostic is written to the primary output (stderr) and **no**
739+
`{error}` envelope is emitted. Callers that require an envelope even for
740+
unparseable input should validate that the payload is well-formed JSON before
741+
invoking `wxc-exec`.
742+
725743
For exec specifically, MXC diagnostic output mixes with the script's own stderr when
726744
`--debug` is passed. This is a small amount of pre- and post-dispatch noise; consumers
727745
wanting clean separation should use `--log-file <path>` instead, which routes diagnostic
@@ -1029,26 +1047,30 @@ implements one trait, the other, or both, depending on its declared participatio
10291047

10301048
### 9.1 Wire envelope (Rust mirror)
10311049

1032-
MXC's parser at `src/core/wxc_common/src/config_parser.rs` deserializes the
1033-
wire-format JSON directly into the typed wire model in
1034-
`src/core/wxc_common/src/wire.rs` (`wire::MxcConfig`), then maps it into the
1035-
typed domain models (`convert_wire_config``ExecutionRequest`, with `From`
1036-
impls beside the domain types for the trivial enum/struct conversions) before
1037-
dispatch. The state-aware path reuses this same wire model.
1050+
`src/core/wxc_common/src/config_deserialize.rs` performs path-aware JSON
1051+
deserialization into the typed wire model in
1052+
`src/core/wxc_common/src/wire.rs` (`wire::MxcConfig`).
1053+
`src/core/wxc_common/src/config_parser.rs` discriminates request shape,
1054+
validates the wire model, and maps it into the typed domain models
1055+
(`convert_wire_config``ExecutionRequest`, with `From` impls beside the
1056+
domain types for trivial enum/struct conversions). The state-aware path reuses
1057+
this wire model while retaining its per-backend `experimental` subtree for
1058+
dispatch-time typing.
10381059

10391060
```rust
10401061
// In config_parser.rs — discrimination is by presence of the `phase` key in
1041-
// the decoded JSON; both shapes deserialize into the one wire::MxcConfig type,
1042-
// which declares `phase` / `sandboxId` and the per-backend `experimental` block.
1043-
let value: serde_json::Value = serde_json::from_str(&json_str)?;
1044-
if value.get("phase").is_some() {
1045-
// state-aware: peel off the raw `experimental` block (typed per-backend at
1046-
// dispatch), deserialize the rest into wire::MxcConfig, then map.
1047-
convert_wire_state_aware(value, logger, allow_missing_command)
1062+
// the source JSON without building a full untyped request tree.
1063+
let discriminator: RequestDiscriminator<'_> =
1064+
config_deserialize::from_str(&json_str)?;
1065+
if discriminator.phase.is_some() {
1066+
convert_wire_state_aware(
1067+
&json_str,
1068+
discriminator.experimental,
1069+
logger,
1070+
allow_missing_command,
1071+
)
10481072
} else {
1049-
// one-shot: deserialize wire::MxcConfig from the source text (preserving
1050-
// serde line/column diagnostics) and map.
1051-
let cfg: wire::MxcConfig = serde_json::from_str(&json_str)?;
1073+
let cfg: wire::MxcConfig = config_deserialize::from_str(&json_str)?;
10521074
convert_wire_config(cfg, logger, true, allow_missing_command)
10531075
}
10541076
```
@@ -1073,8 +1095,10 @@ state-aware-only fields (`phase`, `sandboxId`, `experimental.<backend>.<phase>`)
10731095
are extracted alongside the `ExecutionRequest` and bundled into a
10741096
`ParsedStateAwareRequest` domain model — `{ request: ExecutionRequest, phase:
10751097
Phase, containment: Option<ContainmentBackend>, sandbox_id: Option<String>,
1076-
experimental_raw: Option<serde_json::Value> }` — that the dispatcher consumes
1077-
(§9.3). The bundling does not modify `ExecutionRequest`'s shape. Domain models
1098+
experimental_raw: Option<serde_json::Value>, source_text: Option<Box<str>> }` —
1099+
that the dispatcher consumes (§9.3). `source_text` retains the decoded request
1100+
text so per-backend per-phase config errors can be reported with whole-file
1101+
source location (§9.3). The bundling does not modify `ExecutionRequest`'s shape. Domain models
10781102
are exposed to the dispatch layer; the wire types are an implementation detail of
10791103
the parser and schema generation.
10801104

@@ -1426,7 +1450,13 @@ prefix) or `malformed_id` (no prefix structure) per §8.
14261450
`Result<Option<C>, MxcError>`: it navigates the wire `experimental.<backend_key>.<phase_name>`
14271451
JSON value and deserialises it into `C` when present, returns `Ok(None)` when absent,
14281452
and surfaces malformed JSON as `malformed_request`. The dispatcher passes
1429-
`B::BACKEND_KEY` so each backend reads from its own slot.
1453+
`B::BACKEND_KEY` so each backend reads from its own slot. Typed errors carry the
1454+
complete `experimental.<backend>.<phase>.<field>` JSON path **and** whole-file
1455+
source location (line/column), at parity with base-config errors: the phase-config
1456+
sub-slice is deserialised positionally out of the retained request text and its
1457+
fragment-local serde location is translated back to whole-file coordinates. If the
1458+
sub-slice cannot be located, deserialisation falls back to the value-based path,
1459+
which still reports the JSON path (without a source location).
14301460
`sandbox_id_required()` enforces that non-provision phases carry a `sandboxId`,
14311461
returning `&str` on success or `malformed_request` on absence. `validate_exec_common`
14321462
is a free function in `validator.rs` that checks cross-backend per-phase invariants

docs/versioning.md

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -438,6 +438,16 @@ HRESULT EnumerateSandboxSpecVersionInfo(
438438

439439
Negotiation failures are **typed and actionable** — never a silent fallback:
440440

441+
- **JSON and policy-shape failures** distinguish malformed JSON syntax from
442+
valid JSON that does not match the typed wire contract. Typed failures name
443+
the full policy path (for example, `network.proxy.localhost`), retain Serde's
444+
expected type/value information, and include source line/column when parsing
445+
directly from request text. State-aware per-backend configuration errors are
446+
prefixed with their full `experimental.<backend>.<phase>` location. Diagnostic
447+
text escapes control characters, and errors at secret-bearing paths redact the
448+
submitted value. After the root JSON value, only whitespace is accepted;
449+
trailing JSON values or other trailing content are rejected as malformed
450+
syntax.
441451
- **Schema-range failures** (Stage 1) carry a clear "older than supported" /
442452
"newer than supported" message telling the caller whether to update the config
443453
or upgrade `wxc-exec`.

src/Cargo.lock

Lines changed: 12 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,7 @@ windows = { version = "0.62", features = [
9595
windows-core = "0.62"
9696
serde = { version = "1", features = ["derive"] }
9797
serde_json = "1"
98+
serde_path_to_error = "0.1"
9899
thiserror = "2"
99100
anyhow = "1"
100101
base64 = "0.22"

src/core/mxc_engine/src/state_aware.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,7 @@ mod tests {
174174
sandbox_id: None,
175175
correlation_vector: None,
176176
experimental_raw: None,
177+
source_text: None,
177178
};
178179

179180
let error = run_state_aware(parsed, false).unwrap_err();

src/core/wxc/src/main.rs

Lines changed: 44 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,7 @@ fn apply_command_override(
233233
}
234234
}
235235

236-
/// The correlation-vector action a state-aware phase should take, decided purely
236+
/// The plan for producing this phase's correlation vector, derived
237237
/// from the phase and the relayed value. Returned by [`plan_correlation_vector`]
238238
/// so the seed-vs-spin decision is unit-testable without touching the RNG/clock;
239239
/// the caller executes the plan against the (nondeterministic) operators.
@@ -301,6 +301,16 @@ fn inject_correlation_vector(outcome: &mut Result<DispatchOutcome, MxcError>, cv
301301
}
302302
}
303303

304+
/// On a state-aware dispatch failure, record the error only on the auxiliary
305+
/// diagnostic sinks (`--log-file` and the diagnostic pipe) via
306+
/// [`Logger::log_diagnostic_line`]. It is deliberately kept out of the primary
307+
/// console/buffer output so it never interleaves with the stdout response
308+
/// envelope; the failure reason still reaches the client through the JSON error
309+
/// envelope on stdout.
310+
fn log_state_aware_dispatch_error(logger: &mut Logger, error: &MxcError) {
311+
logger.log_diagnostic_line(&error.to_string());
312+
}
313+
304314
/// Drives the state-aware dispatch flow. On envelope success, writes the
305315
/// JSON to stdout and exits 0. On exec success, exits with the script's
306316
/// exit code (output already streamed). On failure, writes a JSON error
@@ -401,6 +411,13 @@ fn run_state_aware_main(
401411
elapsed,
402412
);
403413

414+
// On dispatch failure, route the error to the auxiliary diagnostic sinks
415+
// only (log file / diagnostic pipe) — never the primary buffer/stderr — so
416+
// the stdout error envelope written below stays the single client-facing
417+
// channel and is not shadowed by a duplicate on stderr.
418+
if let Err(error) = &outcome {
419+
log_state_aware_dispatch_error(logger, error);
420+
}
404421
// Diagnostic buffer flushes to stderr regardless of success/failure so it
405422
// never interleaves with the stdout envelope.
406423
let buffered = logger.get_buffer().to_string();
@@ -1381,6 +1398,31 @@ mod tests {
13811398
Logger::new(Mode::Buffer)
13821399
}
13831400

1401+
#[test]
1402+
fn state_aware_dispatch_errors_use_only_auxiliary_diagnostic_sinks() {
1403+
let directory = tempfile::tempdir().unwrap();
1404+
let log_path = directory.path().join("mxc.log");
1405+
let mut logger = test_logger();
1406+
logger.enable_file_sink(&log_path).unwrap();
1407+
let error = MxcError::malformed_request(
1408+
"Invalid configuration at `experimental.wslc.start.portMappings[0].windowsPort`",
1409+
);
1410+
1411+
log_state_aware_dispatch_error(&mut logger, &error);
1412+
1413+
assert!(
1414+
logger.get_buffer().is_empty(),
1415+
"the JSON envelope must retain primary output ownership"
1416+
);
1417+
drop(logger);
1418+
let log = std::fs::read_to_string(log_path).unwrap();
1419+
assert_eq!(
1420+
log.matches("experimental.wslc.start.portMappings[0].windowsPort")
1421+
.count(),
1422+
1
1423+
);
1424+
}
1425+
13841426
#[test]
13851427
fn cli_parses_flags_after_config_path_without_command_override() {
13861428
let cli = parse_cli(&["wxc-exec", "policy.json", "--experimental", "--debug"]);
@@ -1671,6 +1713,7 @@ mod tests {
16711713
sandbox_id: Some("iso:wxc-1234".into()),
16721714
correlation_vector: None,
16731715
experimental_raw: None,
1716+
source_text: None,
16741717
};
16751718

16761719
let err = command_override_context_for_state_aware(&parsed, true).unwrap_err();

src/core/wxc_common/Cargo.toml

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,8 @@ schema-gen = ["dep:schemars"]
1212

1313
[dependencies]
1414
serde = { workspace = true }
15-
serde_json = { workspace = true }
15+
serde_json = { workspace = true, features = ["raw_value"] }
16+
serde_path_to_error = { workspace = true }
1617
thiserror = { workspace = true }
1718
base64 = { workspace = true }
1819
url = { workspace = true }

0 commit comments

Comments
 (0)