Skip to content

Commit d486efd

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. Format-character detection uses the unicode-general-category crate (Format | LineSeparator | ParagraphSeparator) rather than a hand-maintained codepoint table, so the Cf/Zl/Zp set tracks the crate's Unicode tables. * 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, a pinned serde_json positioned-error display-suffix contract, and format-character escaping (U+202E/U+200B/U+2028/U+2029) that also pins the crate-based check. * cargo fmt --all -- --check: passed. * cargo clippy -p wxc_common -p mxc_engine -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> Copilot-Session: 4cc05a95-546f-4acb-913a-b946127fa01f
1 parent 3b910d9 commit d486efd

15 files changed

Lines changed: 2041 additions & 183 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

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

215215
### Config parser pattern
216216

217-
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.
217+
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.
218218

219219
### TypeScript conventions
220220

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: 19 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: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,8 @@ windows = { version = "0.62", features = [
9696
windows-core = "0.62"
9797
serde = { version = "1", features = ["derive"] }
9898
serde_json = "1"
99+
serde_path_to_error = "0.1"
100+
unicode-general-category = "1"
99101
thiserror = "2"
100102
anyhow = "1"
101103
base64 = "0.22"

src/core/mxc_engine/src/state_aware.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,8 @@
55
//!
66
//! The single home for resolving a parsed state-aware request to its backend
77
//! and driving the per-phase flow. It centralizes the backend-specific
8-
//! construction that previously lived inline in `wxc-exec` so the binary can
9-
//! shrink to a thin CLI shell.
8+
//! construction that would otherwise live inline in `wxc-exec` so the binary
9+
//! can shrink to a thin CLI shell.
1010
//!
1111
//! Backends whose `StatefulSandboxBackend` impl lives in a `backends/*` crate
1212
//! (which depends on `wxc_common`, so the construction can't live inside
@@ -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: 60 additions & 18 deletions
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();
@@ -1212,17 +1229,16 @@ fn main() {
12121229
}
12131230
mark_audit_active();
12141231
_audit_guard = Some(AuditTraceGuard);
1215-
// previously this was
1216-
// `let _ = run_plm_command(...)`, which discarded the failure
1217-
// status. If plm start failed (missing plm.exe, wpr session
1218-
// conflict not resolved, etc.), the workload ran with
1219-
// `permissiveLearningMode` injected into the sandbox policy
1220-
// but with zero WPR recording — an empty Adjusted_*.json
1221-
// looked like "no denials." Bail explicitly on start failure
1222-
// so the operator sees the error and the policy isn't
1223-
// silently relaxed.
1232+
// Bail explicitly on `plm start` failure rather than
1233+
// discarding the failure status. If plm start failed
1234+
// (missing plm.exe, wpr session conflict not resolved,
1235+
// etc.), the workload would run with `permissiveLearningMode`
1236+
// injected into the sandbox policy but with zero WPR
1237+
// recording — an empty Adjusted_*.json looks like "no
1238+
// denials." Bailing lets the operator see the error and the
1239+
// policy isn't silently relaxed.
12241240
//
1225-
// bracket the spawn with
1241+
// Bracket the spawn with
12261242
// AUDIT_START_IN_FLIGHT so the console-control handler waits
12271243
// for it to drain before deciding whether to issue `wpr
12281244
// -cancel` (closes the Ctrl+C race where cancel arrives
@@ -1267,13 +1283,13 @@ fn main() {
12671283
// its exit code. Done before the runner is dropped so the trace
12681284
// tooling sees a fully-quiesced workload.
12691285
//
1270-
// only clear `AUDIT_ACTIVE` when `plm stop` actually
1271-
// succeeded. Previously the flag was cleared unconditionally,
1272-
// which silently leaked the kernel ETW session whenever stop
1273-
// failed (missing plm.exe, spawn fail, wpr -stop non-zero) and
1274-
// simultaneously turned `AuditTraceGuard::drop` and the Ctrl-C
1275-
// handler into no-ops. On failure we now leave the flag set so
1276-
// the stack guard's `Drop` runs `wpr -cancel` for us.
1286+
// Only clear `AUDIT_ACTIVE` when `plm stop` actually
1287+
// succeeded. Clearing it unconditionally would silently leak
1288+
// the kernel ETW session whenever stop failed (missing
1289+
// plm.exe, spawn fail, wpr -stop non-zero) and simultaneously
1290+
// turn `AuditTraceGuard::drop` and the Ctrl-C handler into
1291+
// no-ops. On failure, leave the flag set so the stack guard's
1292+
// `Drop` runs `wpr -cancel` for us.
12771293
#[cfg(target_os = "windows")]
12781294
if cli.audit {
12791295
let mut stop_args: Vec<std::ffi::OsString> = vec![std::ffi::OsString::from("stop")];
@@ -1381,6 +1397,31 @@ mod tests {
13811397
Logger::new(Mode::Buffer)
13821398
}
13831399

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

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

src/core/wxc_common/Cargo.toml

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,9 @@ 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 }
17+
unicode-general-category = { workspace = true }
1618
thiserror = { workspace = true }
1719
base64 = { workspace = true }
1820
url = { workspace = true }

0 commit comments

Comments
 (0)