You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Emit errors and warnings as structured ScriptResponse JSON - #1482
Every backend now reports failure through one contract: a structured ScriptResponse
envelope on stderr, a wire error code that says who can act on the failure, and an exit
code that follows from that code. Previously each executor invented its own prose and its
own exit convention, so a caller could not tell a refused request from an unusable host
without string-matching backend-specific text.
The wire contract
Code
Meaning
Exit
policy_validation
the request is at fault — a field was refused
1
backend_unavailable
the host is at fault — no tier/provider could be built, so the policy was never judged
-1
backend_error
a runtime fault during an already-accepted run
-1
backend_unavailable is deliberately broader than "fall back a tier": it also covers a
missing sandbox, an absent provider, and other host conditions that no change to the
request can resolve. That is why it is not an exit 1.
Warnings.ScriptResponse gains a warnings field, emitted as one JSON envelope per
line, so advisories (for example the Bubblewrap process.cwd advisory) are machine-readable
instead of free prose. The field is Option<Box<Vec<String>>> to keep ScriptResponse at
120 bytes; going over 128 trips clippy::result_large_err in the release gate, and a size
test locks that in.
The one subtle classification.DispatchError in the process-container backend carries
two genuinely different failures, and collapsing them either way broke a test suite:
warnings is added to the executor's ScriptResponse only. The versioned v1 Rust/.NET/Node
SDK result types are unchanged; surfacing warnings there is a separate versioned-API change.
Backends still print their human-readable prose alongside the envelope on stderr. That
double-emission predates this branch and is left as-is.
Future work could include sending errors through a separate channel to differentiate them from sandboxed stdout/stderr.
Exit codes are consistent cross-backend w/ structured ScriptResponse JSON #1336 was the starting point for the exit-code and envelope work, but it was not a
complete implementation for the process-container backend; that backend's dispatcher,
fallback, and capture-denials paths are classified here for the first time.
Contract documented in docs/development/architecture/versioning.md (## Error Contract),
with the per-backend notes updated under docs/backends/.
🔍 Validation
Local, after merging main (Windows 11 build 26700, PSEC 1.1 + base-container tier):
CI: scheduled validation run 38377047465 on ad5b809c
This shows that it fixes the remaining errors related to BaseContainer and AppContainer. The [P8d]/[P8e]hostLoopback failures are fixed by this branch.
The debian13-bubblewrap failure is pre-existing in main, and the ubuntu failure is transient.
Entry-point failures printed a bare line and exited 1, which a caller could
not tell apart from a typed policy rejection, and warnings were written as
unstructured stderr text.
Classify every wxc-exec failure by whose problem it is, emit it as a typed
JSON envelope, and carry warnings on ScriptResponse.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Backends reported failures inconsistently: some flattened everything to a
generic backend error, the Bubblewrap and LXC runners set no failure phase at
all, and WSLC rejections exited -1 while ProcessContainer rejections exited 1.
A caller could not tell a refused request from an unavailable host, and the
PowerShell suites had to match on prose banners to find out.
Classify each failure by whose problem it is and derive both the wire code and
the exit code from that:
the request is at fault Rejected policy_validation exit 1
the host is at fault BackendUnavailable backend_unavailable exit -1
the runtime faulted mid-run other phases backend_error exit -1
Every backend now sets a phase, and `ScriptResponse::failure_exit_code` is the
single place the exit code comes from.
Two consequences are worth calling out. A capability the host's Windows build
cannot represent -- `enumeratePaths`, `hostLoopback='allow'` -- now reaches the
wire as `policy_validation`, because dropping the field runs; it used to be
reported as an unavailable backend, which told the caller to give up on the
host. Conversely, a backend this build did not compile in now reports
`backend_unavailable` from the one-shot path too, matching what the state-aware
path already returned, because only a rebuilt executor can serve it.
Add an optional `warnings` array to `ScriptResponse` so the executors stop
interleaving advisory diagnostics with the error channel. It is boxed to keep
the struct under clippy's `result_large_err` threshold, and omitted from the
wire when a run produced none.
Fixes#612
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The P14b and P14c behavioural assertions gate on the `nativeCaptureAvailable`
probe, which answers whether the Learning Mode API is present. That is not the
same question as whether it is implemented: on a host whose exports resolve but
return E_NOTIMPL, the probe says the provider is usable, both runs then fail
before an output path exists, and P14c reports three failures on empty strings.
The runtime already classifies that outcome correctly -- `is_api_unavailable()`
maps it to `BackendUnavailable`, which reaches stderr as a typed
`backend_unavailable` envelope. Gate on what the run reported rather than on
the probe alone, via a shared `Test-BackendUnavailable`. Keying on the typed
code covers every reason a host cannot provide capture, not just the ones a
probe knows to ask about.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Dispatch runs after the policy is accepted, so a failure there is the host
failing to build the tier the policy asked for, not a verdict on the request.
Classifying fallback and capture-provider failures as policy_validation made
wxc-exec log them as unsupported_field_for_backend and exit 1, which the
process-container suites read as a validation rejection of a config they
require to be accepted. Only a SID that could not be derived or ACEs that
could not be applied keep their own codes.
Update the harnesses that encoded the previous exit codes: WSLC and isolation
session rejections now exit 1, and the two process-container dispatch refusals
now exit -1. Match the Bubblewrap cwd advisory piecewise, since the advisory is
carried in a JSON warning envelope that escapes the quotes around the path.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tier
Two different failures share DispatchError. A field the selected tier cannot
represent names that field and is documented as a rejection, so collapsing it
into backend_unavailable made hostLoopback=allow and enumeratePaths exit -1,
which the suites read as 'accepted and silently dropped' -- the one outcome
P8d and P8e exist to rule out. A tier that could not be built at all, or a
missing denial-capture provider, stays backend_unavailable: no tier existed to
judge the policy against.
Restore the enumeratePaths harness to exit 1 to match.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Audit finalization raised its cleanup diagnostics as bare stderr prose, so a
caller had to scrape them while every other warning arrived as a JSON
envelope. They now travel on the response and are emitted with the rest.
The finalization failure arm hard-coded exit -1 regardless of how the backend
had already classified the run, which could pair a rejection envelope with an
exit status that contradicts it. Deriving the status from the failure phase
keeps the two in agreement, and the appended message reaches stderr through
the error envelope rather than a separate prose line.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Failure classification remains inconsistent across validation and streaming paths, and the Seatbelt availability check can reject valid default executions.
Elliot (theelliotm)
changed the title
Classify every backend failure by who can act on it
Emit errors and warnings as structured ScriptResponse JSON
Oct 13, 2026
Three surfaces reached the same condition by a different route and reported
it differently.
Shared request validation returned a runtime error, so an empty script was a
rejection through `validate` but a `backend_error` through `spawn`, which is
the path callers actually take. Both shared checks judge the request, so they
are rejections. `enumeratePaths` on a backend that cannot represent it now
also matches how the process-container dispatcher classifies the same field.
The streaming and PTY arms for a backend this build omits returned
`unsupported_containment`, while run-to-completion resolution called the same
build and host `backend_unavailable`. The code no longer depends on which
operation the caller chose. Arms for a backend belonging to another operating
system are untouched.
Seatbelt probed for Apple's sandbox CLI on every launch, but only the `Open`
method shells out to it; `Exec`, the default, applies the profile through
`sandbox_init`. A host without the CLI no longer loses the launch method that
does not need it.
Documents the stderr warning envelope alongside the response array, since the
two shapes differ and only the array was described, and records why a missing
experimental opt-in is a host-side code.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Drain retained telemetry warnings before early exits
src/tools/wxc/src/main.rs:1424
This early exit occurs after telemetry::init may have retained a warning (for example, ETW registration failure), but it bypasses the only one-shot warning drain at line 1660. The error envelope is emitted while that warning is silently lost from stderr, despite warnings being documented as emitted on failures too. Drain structured warnings before exiting; the same is needed at the other post-telemetry early exits.
This issue also appears in the following locations of the same file:
I reviewed the 43-file diff from base 8188df6 to head 9b7a541 and confirmed that its normalized contents match gh pr diff 1482. The inline comments below are anchored only on added lines; unchanged supporting files were byte-compared with the base. The strongest gap is that Unix executable tests do not assert the new JSON-code/exit contract. This is a COMMENT review, not a request for changes based on claim-only findings. No local build, platform-dependent integration suite, or end-to-end test was run as part of filing this review.
Findings on existing open threads
#3, live Bubblewrap DNS failures mislabeled as policy refusal: continued in discussion_r4277475899.
#6, state-aware error/status mismatch with the PR's blanket claim: continued in discussion_r4277476083 as claim_mismatch, not an introduced runtime regression.
#13, warning JSON test can miss a malformed line: continued in discussion_r4277475994.
#20, ProcessContainer DACL warnings plus other unstructured wxc-exec advisories despite “Resolves #612”: continued in discussion_r4277476034 as claim_mismatch.
Claim-only and unchanged supporting locations
#15 (claim_mismatch, Medium): new docs/development/architecture/versioning.md:718-719 promises wrong-OS unsupported_containment, but the byte-identical state-aware WSLC/IsolationSession gates still return backend_unavailable. The inline comment targets the new claim, not the old gates.
#16-19 (newly_exposed_by_change, Medium): the changed IsolationSession policy mapping, one-shot parser, Hyperlight policy phase, and Windows WSLC feature-off arm leave docs/development/architecture/backends/isolation-session/oneshot.md, the one-shot paragraph of docs/development/architecture/container-lifecycle.md, the earlier Hyperlight exit row, and docs/backends/wslc/wsl-container-getting-started.md stale. Each comment is anchored to the changed behavior rather than forcing an anchor into an unchanged paragraph.
#22 (claim_mismatch, Low): the PR's all-failures envelope wording includes dry-run failures, but the unchanged handle_dry_run_exit path in wxc, lxc, and mxc_darwin exits before emit_backend_error_envelope; dry-run callers still get a human summary rather than an error JSON line. Either emit the envelope on a failing dry run or limit the claim explicitly. This pre-existing early-exit path is not asserted as a new regression.
Verified pre-existing behavior, not attributed as a new regression
mxc_engine/state_aware.rs is byte-identical between base and head; it already returned backend_unavailable for certain missing-feature paths. The state-aware terminal branches already hard-coded exit 1. Finding #6 addresses only the PR's new universal promise.
The host DNS lookup in bubblewrap/common/proxy_network.rs is byte-identical. Finding #3 concerns the newrejected conversion of its errors at bwrap_runner.rs:472, not the resolver itself.
The older Seatbelt Exec-versus-Open rejection and the streaming feature-off classification have resolved review threads. New comments #11, #15, #19, and #24 address separate residual test, state-aware, and documentation gaps; they are not requests to reopen the fixed behavior.
The reason will be displayed to describe this comment to others. Learn more.
Medium (security) - workload stderr can impersonate MXC's JSON error.
Attribution:newly_exposed_by_change - the new backend_unavailable code is presented as a machine-readable host-failure signal on the same stderr channel the sandboxed program can write.
wxc-exec relays workload stderr before calling emit_backend_error_envelope; that emitter writes nothing for an ordinary nonzero workload exit because error_message is empty (script_runner.rs:104-120). A workload can print a plausible {"error":{"code":"backend_unavailable",...}} and exit nonzero. A CLI consumer parsing these lines cannot establish whether this diagnostic came from MXC or from contained code. This is conditional on a consumer trusting stderr; I did not find an in-repo SDK making fallback decisions from it.
Fix: Put executor-originated results on a host-owned channel the workload cannot write to. Until then, document these shared-stream lines as untrusted and do not recommend switching containment based solely on them.
The reason will be displayed to describe this comment to others. Learn more.
Medium (maintainability) - centralize phase-aware ScriptResponse construction.
Attribution:introduced_by_change - this PR adds the same response/phase assembly here and in lxc_runner.rs:47, appcontainer_runner.rs:95, seatbelt_runner.rs:1104, and isolation_session/common/sandbox.rs:91.
The implementations already differ in whether they copy the diagnostic into standard_err, which affects bare-prose duplication alongside the new JSON envelope. Maintaining one error contract through five near-identical constructors invites drift.
Fix: Put the phase-aware response construction in a small shared ScriptResponse constructor and use it at the backend call sites; preserve intentionally different stderr behavior explicitly.
The reason will be displayed to describe this comment to others. Learn more.
Changes requested
This follow-up changes my verdict from the COMMENT review 5534936733 on the same PR head (9b7a541). The 17 inline comments in that review and four subsequent replies already contain the detailed evidence; I am not opening duplicate threads.
Please address these change-attributable issues before merge:
High - Unix executor contract coverage (discussion_r4284388975, introduced_by_change): no Unix executor suite asserts both the structured error code and its exact process exit status for the new failure contract. Add a real refusal and unavailable-backend subprocess case, plus an envelope-serialization assertion.
Medium - Bubblewrap host DNS misclassified as request refusal (discussion_r4277475899, introduced_by_change): SandboxProxy::resolve can fail on host DNS timeout/thread failure, but the new rejected mapping emits policy_validation/exit 1. Keep static invalid-input errors as policy refusals and classify host lookup failures separately.
Medium - response warnings are missing for in-process callers (discussion_r4284389019, introduced_by_change): the new ScriptResponse.warnings field is not populated by mxc_engine::run even when the logger records advisories. Populate it at the shared run boundary or narrow the published response-level contract.
The state-aware exit mismatch in discussion_r4277476083 predates this PR and is a claim_mismatch, not a merge condition by itself. Other Medium/Low comments remain actionable but are not the basis for this verdict. The PR base and head are unchanged since the earlier review; no local build or host-dependent test suite was run for this follow-up.
…exit codes
- IsolationSession one-shot reports a wait timeout as FailurePhase::Timeout instead of ProcessExited.
- Warning envelopes start on a fresh stderr line.
- Lifecycle error envelopes exit with their code's status instead of always 1.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1c677fc2-77c3-43b0-a06f-da11d11447c8
Preserving ExecOutcome fixes local wait timeouts, but service-enforced timeouts still bypass the new timeout response. create_process gives start_process and the later started.wait the same timeout (manager.rs:451,582). As documented in process_options.rs:111–118, the earlier service timer can kill the workload with exit 1, which is returned as Exited(1) without an error envelope. Apply with_service_timeout_grace to the service options while retaining the original local wait deadline, and update the timeout tests and documentation accordingly.
Preserving ExecOutcome fixes locally detected timeouts, but not service-enforced ones. create_process passes the original timeout to start_process (manager.rs:451), then starts the same-duration local wait after relay setup (582). The service can terminate the workload first with exit 1, so this becomes Exited(1) without a timeout envelope. Apply with_service_timeout_grace to the service options while retaining the original local timeout, as the piped path does. Update the helper's streaming-only documentation and tighten the host timeout test, which still accepts either 1 or -1.
The new IsolationSession timeout response reaches this emitter after workload stderr has been relayed. If the workload writes partial without a newline before timing out, and no later diagnostic supplies a newline, this produces partial{"error":...} rather than a parseable JSON line. Start the error envelope on a fresh line, as the warning emitter and lifecycle exec-error path already do.
- Seatbelt profile building separates policy errors from host setup errors; a missing HOME is LaunchFailed rather than a policy rejection.
- Failed create-and-run dry runs emit the typed error envelope on stderr.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1c677fc2-77c3-43b0-a06f-da11d11447c8
Preserving ExecOutcome fixes local wait timeouts, but not service-side timeouts. create_process still passes the original timeout to start_process (manager.rs:451), then starts a local wait for the same duration (line 582). As process_options.rs documents, the earlier service timer can terminate the workload with exit 1. That arrives here as Exited(1), leaving no timeout message or error envelope. Apply with_service_timeout_grace only to the service options, keep the original local deadline, and require backend_error/exit -1 in the host timeout test.
- create_process arms the service timer with the grace margin and enforces the caller's deadline locally, so a timeout reports backend_error/exit -1.
- Error envelopes start on a fresh stderr line.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1c677fc2-77c3-43b0-a06f-da11d11447c8
Copilot's 'Preserve local timeout while extending service timeout grace' finding is fixed in 2ed5f6f: create_process arms the service timer with the grace margin, and the host test now requires backend_error/exit -1.
DaclFallbackDisabled produces policy_validation and exit 1, consistent with the new tests and versioning documentation. The PR description instead lists it under backend_unavailable and exit -1, giving consumers conflicting contracts. Move this variant into the description's policy-refusal row to match the implemented behavior.
🧠 Review effort: Balanced
This branch has not been deployed
No deployments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Needs-AttentionRequires attention or a decision from the MXC maintainers.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Elliot (theelliotm)
changed the title
Gudge (MGudgin)
left a comment
microsoft-github-policy-service
Bot
added
the
Needs-Author-Feedback
📖 Description
Every backend now reports failure through one contract: a structured
ScriptResponseenvelope on stderr, a wire error code that says who can act on the failure, and an exit
code that follows from that code. Previously each executor invented its own prose and its
own exit convention, so a caller could not tell a refused request from an unusable host
without string-matching backend-specific text.
The wire contract
policy_validation1backend_unavailable-1backend_error-1backend_unavailableis deliberately broader than "fall back a tier": it also covers amissing sandbox, an absent provider, and other host conditions that no change to the
request can resolve. That is why it is not an exit
1.Warnings.
ScriptResponsegains awarningsfield, emitted as one JSON envelope perline, so advisories (for example the Bubblewrap
process.cwdadvisory) are machine-readableinstead of free prose. The field is
Option<Box<Vec<String>>>to keepScriptResponseat120 bytes; going over 128 trips
clippy::result_large_errin the release gate, and a sizetest locks that in.
The one subtle classification.
DispatchErrorin the process-container backend carriestwo genuinely different failures, and collapsing them either way broke a test suite:
Fallback(EnumeratePathsUnsupported | IngressUnsupported | DaclFallbackDisabled)policy_validation, exit1Fallback(WriteDacUnavailable | SystemRootUnresolved),CaptureDenialsUnsupported,CaptureDenialsUnavailablebackend_unavailable, exit-1Sid(e)Dacl { .. }backend_error, exit-1Scope notes
warningsis added to the executor'sScriptResponseonly. The versionedv1Rust/.NET/NodeSDK result types are unchanged; surfacing warnings there is a separate versioned-API change.
double-emission predates this branch and is left as-is.
🔗 References
Resolves #612
complete implementation for the process-container backend; that backend's dispatcher,
fallback, and capture-denials paths are classified here for the first time.
docs/development/architecture/versioning.md(## Error Contract),with the per-backend notes updated under
docs/backends/.🔍 Validation
Local, after merging
main(Windows 11 build 26700, PSEC 1.1 +base-containertier):cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warningscargo test --workspacecargo test -p mxc-sdk --lib -- dispatcher::teststests/scripts/run_processcontainer_all_tests.ps1CI: scheduled validation run
38377047465 on
ad5b809cThis shows that it fixes the remaining errors related to BaseContainer and AppContainer. The
[P8d]/[P8e]hostLoopbackfailures are fixed by this branch.The debian13-bubblewrap failure is pre-existing in main, and the ubuntu failure is transient.
✅ Checklist
Cargo.lock, thedependency-feed-checkcheck passes (see pull request builds)📋 Issue Type
Microsoft Reviewers: Open in CodeFlow