Carry the failing API's detail through the in-process SDKs by adpa-ms · Pull Request #925 · microsoft/mxc

adpa-ms · 2026-08-15T00:00:04Z

📖 Description

When a platform API call fails, the backend records which call failed, its native status, and sometimes a remediation hint. The TypeScript SDK already surfaces all three and documents the invariant that nativeCode implies operation (sdk/node/src/errors.ts). In-process callers got none of it.

wxc_common::mxc_error::MxcError carries the detail in api_failure: Option<Box<ApiFailure>>, but the public SDK error type is a facade over it — mxc_engine::error::Error — which held only { code, message } and whose From<MxcError> dropped api_failure on the floor. Everything below that conversion was blind: the Rust SDK, the C ABI over it (which mentioned none of the three field names), and the C# MxcException. Two callers observing the same failure got materially different diagnoses depending on which SDK they used, with in-process callers seeing backend_error: The provision was not found. and no way to learn which call failed or why.

This carries the detail through all three layers.

The engine facade now carries the three flat on Error — operation, native_code and remediation sitting beside code and message. That matches the wire envelope, the C ABI, the C# binding, and the TypeScript SDK's MxcError, which already carries the same three flat with the same invariant documented rather than enforced (sdk/node/src/errors.ts). Nesting them behind a sub-struct would have bought the invariant by construction at the cost of making the Rust SDK the one surface of five shaped differently from the rest. The half of that invariant which is real is documented on the field instead: a native code only ever appears alongside the operation it belongs to, because a status with no call to attribute it to is not something a producer can express. A remediation carries no such coupling — it is an actionable hint, and nothing about a hint requires an API call to have been in flight. Display renders the operation and status, so a caller that only logs the error keeps the diagnosis.

The C ABI gains one MxcErrorDetail shape used by every failing surface: MxcRunResult, MxcStateAwareResult, and the out_error parameter of the two entry points that return a live handle. One shape means a binding learns the same things from any surface and frees them all the same way.

The C# binding surfaces Operation / NativeCode / Remediation on MxcException, marshalled through one shared helper. A null pointer becomes null, never "" — that distinction is the contract, since null means the layer below supplied nothing there.

This also means the Rust SDK gains structured errors, not only C#.

Notes for reviewers

The out_error parameter changed ownership shape, from a library-allocated string to caller-provided storage for one detail. That moved a responsibility: the detail owns heap strings with no destructor, so a caller passing null would leak them — finish_spawn frees the detail itself on that path. The safety contract now requires storage holding no live detail, and says why the callee cannot simply free what was there.
The structured MxcException constructor is internal on purpose. A public overload taking three independent nullable strings would let a caller build the state the documentation says cannot exist, and ToString() would then silently drop the status. A reflection test pins that the only public constructor is the code-and-message one.
The codegen gate grew a parity assertion. csbindgen's sources and the C# project's MSBuild Inputs are two lists naming the same files; adding a source to one and not the other lets an incremental build compile against stale bindings, and cargo's own rerun-if-changed never gets consulted because the target never runs. That drift happened once in this branch, which is why the gate now checks it.
Growing REQUIRED_ENTRY_POINTS to the binding's full P/Invoke surface is deliberately not here: the C# compiler already catches rename and removal, since csbindgen emits a binding only for a fn carrying #[no_mangle] or #[export_name] and takes the EntryPoint from whichever determines the export. That belongs with the dedicated C# workstream.
The C ABI is not a stable external contract — the native library and the C# binding are co-versioned and generated together — so reshaping the #[repr(C)] types is expected rather than breaking.
Error is #[non_exhaustive]. Both the wire envelope (ErrorEnvelope) and the internal error this facades (MxcError) already carry it, so publishing the facade without it would have left the public surface less future-proofed than either thing it sits between. The asymmetry that decided it was measured rather than argued: a downstream crate compiled against a non-exhaustive type still builds after the attribute is removed — the only diagnostic is an unreachable_patterns warning on an enum's now-redundant wildcard arm, and a struct produces none at all — whereas adding it fails with E0639 and E0004. So it is cheap now, cheap to reverse, and expensive only if deferred. ErrorCode is deliberately left closed, mirroring the internal MxcErrorCode, which is also not #[non_exhaustive] while the two structs beside it are: that set tracks a closed wire union one-for-one, and a new code should break consumers loudly rather than disappear into a wildcard arm.
Every C ABI entry point is now catch_unwind-wrapped, 23 of 23. The module header already claimed exactly this; mxc_version was a silent exception to it. Wrapping it rather than qualifying the claim keeps the rule grep-checkable instead of something a reader has to re-derive by working out which bodies can panic. Its fallback returns an empty 'static string rather than null, so the documented "valid for the lifetime of the process and must not be freed" contract holds on that unreachable path too.

🔗 References

Resolves #924

🔍 Validation

Full local gauntlet on this commit, run elevated so the wxc_host_prep tests were covered rather than skipped — 27 gates, all green: cargo fmt, cargo clippy --all-targets -- -D warnings, the workspace test suites with the isolation-session feature both on and off, an arm64 cross-build, all six versioning gates, both C#-SDK gates (check-dotnet-errorcode-parity.js, check-dotnet-bindings-codegen.js), dotnet test (47 tests), the dependency-feed resolution check, and the Node SDK build, unit, pack and integration legs. Corroborated independently of the runner's own summary: zero test result: FAILED and zero error: test failed across the full log.

Isolation-session E2E on a VM running the OS-side service, from a package whose recorded commit_sha is this commit: 90 passed, 0 failed, 0 skipped — 16 one-shot, 62 state-aware, 12 SDK-integration. Cross-checked against the JUnit output rather than the runner's own TOTAL: line: 90 testcase elements, and zero failure, error or skipped elements. The agent-account leak check, diffed against a baseline snapshot taken before the first test, came back empty and IsoSessionCli list-users agreed at Found 0 agent user(s). The three interactive TTY tests were run by an operator at the VM console and passed.

Each of the three Rust commits also builds on its own under cargo check --workspace --all-targets, so the series is bisectable; the remaining two are C# and documentation.

ErrorCode parity is unaffected — this adds no new MXC_STATUS_* codes.

✅ Checklist

Signed the Contributor License Agreement
Linked to an issue
Updated documentation (if applicable)
Updated Copilot instructions (if build, architecture, or conventions changed)

📋 Issue Type

Bug fix

Microsoft Reviewers: Open in CodeFlow

azure-pipelines · 2026-08-15T00:00:28Z

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

Copilot

Pull request overview

Carries structured platform API failure details through the Rust, C ABI, and C# SDK layers, addressing #924.

Changes:

Preserves ApiFailure in the engine and Rust SDK.
Adds shared C ABI and C# structured error models.
Updates bindings generation, documentation, and tests.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`src/core/mxc_engine/src/error.rs`	Adds structured API failure details.
`src/core/mxc_engine/src/lib.rs`	Exports `ApiFailure`.
`src/core/mxc-sdk/src/lib.rs`	Exposes and documents structured errors.
`src/ffi/mxc_ffi/src/error_detail.rs`	Defines the shared FFI error shape.
`src/ffi/mxc_ffi/src/lib.rs`	Integrates details into run results.
`src/ffi/mxc_ffi/src/streaming.rs`	Returns details from streaming spawn.
`src/ffi/mxc_ffi/src/state_aware.rs`	Returns details from lifecycle calls.
`src/ffi/mxc_ffi/build.rs`	Adds error-detail binding generation.
`src/ffi/mxc_ffi/tests/ffi.rs`	Updates FFI result assertions.
`sdk/dotnet/Microsoft.Mxc.Sdk/Native/NativeError.cs`	Marshals native error details.
`sdk/dotnet/Microsoft.Mxc.Sdk/MxcException.cs`	Exposes structured exception properties.
`sdk/dotnet/Microsoft.Mxc.Sdk/MxcSandbox.cs`	Propagates run and spawn details.
`sdk/dotnet/Microsoft.Mxc.Sdk/MxcLifecycle.cs`	Propagates lifecycle details.
`sdk/dotnet/Microsoft.Mxc.Sdk/Microsoft.Mxc.Sdk.csproj`	Tracks the new binding input.
`sdk/dotnet/Microsoft.Mxc.Sdk.Tests/MxcExceptionTests.cs`	Tests structured exceptions.
`sdk/dotnet/README.md`	Documents failure diagnostics.
`scripts/check-dotnet-bindings-codegen.js`	Checks binding-input parity.

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

Copilot

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

src/ffi/mxc_ffi/src/error_detail.rs:117

This new exported C ABI destructor bypasses the crate's panic barrier. The other destructors wrap their bodies in catch_unwind (src/ffi/mxc_ffi/src/lib.rs:384-391, :400-407, and state_aware.rs:154-161), matching the crate-level guarantee that every entry point is panic-contained. Wrap this body too so a future panic in cleanup cannot abort at the FFI boundary.

pub unsafe extern "C" fn mxc_error_detail_free(detail: *mut MxcErrorDetail) {
    if detail.is_null() {
        return;
    }
    // SAFETY: non-null per the check above, and valid per the caller contract.
    unsafe { (*detail).free_strings() };

src/core/mxc-sdk/src/lib.rs:142

The repository's SDK documentation rule (.github/copilot-instructions.md:248) requires public Rust SDK API changes to update both crate docs and src/core/mxc-sdk/README.md. This new ApiFailure export and its accessors are covered only in the crate docs; the README still mentions bare Error/ErrorCode behavior and gives callers no structured-diagnostics guidance. Please add the same contract and example there.

pub use mxc_engine::{
    available_backends, available_tools_policy, build_request, build_request_with_containment,
    platform_support, temporary_files_policy, user_profile_policy, ApiFailure, AvailableBackend,
    BackendCapability, Containment, Error, ErrorCode, FilesystemPolicyResult, PlatformSupport,
    SandboxPolicy, SandboxRequest, WslcSection,

sdk/dotnet/Microsoft.Mxc.Sdk/Native/NativeError.cs:39

The key populated-detail marshalling path remains untested. Existing native C# tests exercise only null operation/status fields, while ApiDetail_IsCarriedAlongsideTheCodeAndMessage bypasses this helper by constructing MxcException directly. A field-order or UTF-8 mapping regression here would therefore pass. Add a unit test that builds an MxcErrorDetail with unmanaged UTF-8 values, calls ToException, and asserts all four mapped strings plus null-versus-empty behavior.

        return new MxcException(
            (ErrorCode)status,
            ToStringOrNull(detail.message_utf8) ?? fallbackMessage,
            ToStringOrNull(detail.operation_utf8),
            ToStringOrNull(detail.native_code_utf8),
            ToStringOrNull(detail.remediation_utf8));

Copilot

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.

The doc comment on the SDK error linked `crate::spawn_sandbox`, which does not exist in `mxc_engine` -- that is `mxc-sdk`'s name for the wrapper. The engine's own streaming entry point is `spawn`. Rustdoc resolves intra-doc links against the crate being documented, so this fails `cargo rustdoc -p mxc_engine -- -D warnings`. Pre-dates the surrounding change; corrected here because this branch is the next thing to touch the file. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 02c36d36-fb3d-446f-9108-4e4a01b01af1

The public SDK error is a facade over the foundation crate's error, and it copied only the code and message -- so the operation, platform status and remediation the backend had already produced were dropped at that boundary. Every in-process caller lost the diagnosis: the Rust SDK, the C ABI over it, and the C# binding beyond that. A caller could see "backend_error: The provision was not found." with nothing to say which call failed or why. The three carry flat on Error rather than nested behind a sub-struct, matching the wire envelope, the C ABI and the C# binding. One failure then reads the same whichever of the four surfaces a caller is holding, which is worth more than making an invariant hold by construction on exactly one of them. The half of that invariant which is real is documented instead: a native code only ever appears alongside the operation it belongs to, because a status with no call to attribute it to is not something a producer can express. A remediation carries no such coupling -- it is an actionable hint, and nothing about a hint requires an API call to have been in flight. Display renders the operation and status in brackets, so a consumer that only logs the error keeps the diagnosis rather than silently losing it. A remediation with no operation renders as plain code and message, not as an empty bracket. Error is #[non_exhaustive], as both the wire envelope and the internal error it facades already are. Adding that attribute after the fact is a breaking change and removing it is not -- measured, not assumed: a downstream crate compiled against a non-exhaustive type still builds after the attribute is removed, while adding it fails with E0639 and E0004. So the choice belongs here, while nothing yet consumes the surface. The crate documentation gains a worked example of reading the detail, and says what a caller needs to know: that the operation and status are absent for a failure raised before any API call was reached, and that a native code never appears without one. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 02c36d36-fb3d-446f-9108-4e4a01b01af1

Each failing surface carried a bare message: the run result, the state-aware result, and the out-parameter of the two entry points that hand back a live handle. A binding could report that a call failed but not which call, nor with what platform status -- detail the backend had already produced and the SDK now carries. All three now carry an MxcErrorDetail. One struct means a binding learns the same things from any surface and frees them all the same way, and it is the shape the experimental opt-in rung will extend rather than reshape a second time. Each field crosses independently, and the module header says exactly which couplings are real: a native code is non-null only when the operation is, because a status with no call to attribute it to is not something a producer can express, while a remediation carries no such coupling at all. An operation with no status is a supported shape, pinned by absent_optional_fields_stay_null_rather_than_empty, so the contract does not claim the three either all cross or all stay null. The out-parameter changes from an owned string to caller-provided storage for one detail. That moves a responsibility: the error owns heap strings with no destructor, so a caller passing null would leak every one of them. finish_spawn frees the detail itself in that case rather than dropping a struct of raw pointers on the floor. The contract requires storage holding no live detail, and says why the callee cannot simply free what was there: uninitialised storage holds no pointers it could release, and nothing tells the two cases apart at runtime. Initialisation uses a write rather than an assignment to say exactly that. The module header promises that every entry point wraps its body in catch_unwind, so a panic becomes a status code rather than an unwind across the C ABI, where unwinding is undefined behaviour. Two did not: the new mxc_error_detail_free, and mxc_version. The second is benign -- its version is a compile-time constant and its only fallible step is discharged with unwrap_or_default, so it cannot unwind -- but a blanket claim with a silent exception is worse than either a smaller claim or no exception. Both are wrapped now, so the rule is exceptionless and grep-checkable at 23 of 23, rather than something a reader has to re-derive by working out which bodies can panic. mxc_version's fallback returns an empty static string rather than null, so its documented "valid for the lifetime of the process and must not be freed" contract holds on that unreachable path too. The build script's header stops claiming the generated bindings are checked in and diffed. They are gitignored and regenerated -- as the same file already says nine lines further down -- and the header now names both callers that regenerate them rather than only the gate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 02c36d36-fb3d-446f-9108-4e4a01b01af1

MxcException carried a code and a message, so the managed binding lost the same diagnosis the layers beneath it had just been taught to carry. It now exposes Operation, NativeCode and Remediation, and ToString appends the operation and status so a caller that only logs the exception keeps them. The three are documented rather than enforced where enforcement would be a lie: a native code is non-null only when an operation is, because the native layer cannot produce a status with no call to attribute it to. A remediation carries no such coupling. The five-argument constructor stays internal, which keeps the first implication true by construction rather than by convention -- a public overload taking three independent nullable strings would let a caller build the state the documentation says cannot exist, and ToString would then silently drop the status. NativeError.ToException is the one place the native detail becomes managed, and it marshals each field independently: null and the empty string stay distinct, because the native contract distinguishes "the API supplied nothing" from "it supplied an empty value". Both callers release the native detail in a finally block, so a throw during marshalling or exception construction cannot strand the strings it owns. NativeErrorTests covers that marshalling step, which nothing pinned before: transposing operation_utf8 and native_code_utf8 left the whole suite green, because the tests either side of it drive the managed exception directly or the all-null detail a library-raised failure produces. Every value in the new tests is distinct so a transposition fails. The tests fabricate the struct and free their own allocations, and say why -- a reviewer read those frees as evidence that product code must release marshalled strings by hand, when the opposite holds: the test allocated that memory so the test frees it, while a real detail goes back to the native allocator, through mxc_error_detail_free when it stands alone or the owning result's free function when it is embedded. The codegen gate additionally asserts mxc_error_detail_free, and checks that every Rust source csbindgen reads is also declared as an MSBuild input -- the lists having drifted once already, which is how an incremental C# build can compile against stale declarations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 02c36d36-fb3d-446f-9108-4e4a01b01af1

Repository convention requires a public SDK API change to update the crate documentation and the SDK README together. The Rust crate docs gained the worked example when the error type changed; the two READMEs had not caught up, and the architecture notes still described the C ABI's old result shape. The Rust SDK README gains a "Diagnosing a failure" section: which entry points return an Error, that the live Sandbox handle is the deliberate exception returning io::Result, how to read the detail, and that a native code only ever appears alongside the operation it belongs to while a remediation does not. It also states that Error is #[non_exhaustive], so a caller builds one with Error::new rather than by literal. The C# README documents the same three properties on MxcException and the same coupling, so a reader arriving from either binding is told the same thing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 02c36d36-fb3d-446f-9108-4e4a01b01af1

Copilot

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/core/mxc_engine/src/error.rs:97

These independent public fields do not enforce the documented “native code implies operation” invariant. Even with #[non_exhaustive], a downstream caller can create Error::new(...) and then set native_code while leaving operation as None; Display will silently omit that status, while MxcErrorDetail::from_error propagates the invalid pair. Please use the grouped Option<Box<ApiFailure>> representation described in the PR, with operation required, or make these fields private and expose invariant-preserving accessors/builders.

    pub operation: Option<String>,
    /// The underlying platform status, e.g. `0x80070490`. Only ever present
    /// alongside [`operation`](Self::operation): a status with no call to
    /// attribute it to is not something a producer can express.
    pub native_code: Option<String>,

adpa-ms requested review from a team and a balanced review from Copilot August 15, 2026 00:00

adpa-ms requested a review from a team as a code owner August 15, 2026 00:00

Copilot started reviewing on behalf of adpa-ms August 15, 2026 00:00 View session

Copilot AI reviewed Aug 15, 2026

View reviewed changes

Comment thread sdk/dotnet/Microsoft.Mxc.Sdk/MxcSandbox.cs Outdated

Comment thread sdk/dotnet/Microsoft.Mxc.Sdk/MxcLifecycle.cs Outdated

Comment thread sdk/dotnet/Microsoft.Mxc.Sdk/Native/NativeError.cs Outdated

Comment thread scripts/check-dotnet-bindings-codegen.js

Copilot AI review requested due to automatic review settings August 15, 2026 01:43

adpa-ms force-pushed the user/adibpa/copilot-mxc-error-structured-fields branch from 392dbe6 to 8637dd9 Compare August 15, 2026 01:43

microsoft-github-policy-service Bot added the Copilot-Instructions PR modifies Copilot instruction files (.github/copilot-instructions.md or .github/instructions/) label Aug 15, 2026

Copilot started reviewing on behalf of adpa-ms August 15, 2026 01:43 View session

Copilot AI reviewed Aug 15, 2026

View reviewed changes

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

Copilot AI review requested due to automatic review settings August 15, 2026 03:26

adpa-ms force-pushed the user/adibpa/copilot-mxc-error-structured-fields branch from 8637dd9 to b76d2d7 Compare August 15, 2026 03:26

Copilot started reviewing on behalf of adpa-ms August 15, 2026 03:26 View session

Copilot AI reviewed Aug 15, 2026

View reviewed changes

Ion Vasilian (ionvasilian) previously approved these changes Aug 15, 2026

View reviewed changes

Jeffrey Stall (jeffstall) requested changes Aug 15, 2026

View reviewed changes

Comment thread sdk/dotnet/Microsoft.Mxc.Sdk.Tests/NativeErrorTests.cs

microsoft-github-policy-service Bot assigned adpa-ms Aug 15, 2026

microsoft-github-policy-service Bot added Needs-Author-Feedback Issue needs attention from issue or PR author Needs-Attention Issue needs attention from Microsoft and removed Needs-Author-Feedback Issue needs attention from issue or PR author labels Aug 15, 2026

adpa-ms requested a review from Jeffrey Stall (jeffstall) August 15, 2026 23:14

adpa-ms added 5 commits August 16, 2026 20:21

adpa-ms dismissed Ion Vasilian (ionvasilian)’s stale review via 9c149f5 August 17, 2026 03:23

adpa-ms force-pushed the user/adibpa/copilot-mxc-error-structured-fields branch from b76d2d7 to 9c149f5 Compare August 17, 2026 03:23

Copilot AI review requested due to automatic review settings August 17, 2026 03:23

Copilot started reviewing on behalf of adpa-ms August 17, 2026 03:24 View session

Copilot AI reviewed Aug 17, 2026

View reviewed changes

Navigation Menu

Uh oh!

Carry the failing API's detail through the in-process SDKs - #925

Carry the failing API's detail through the in-process SDKs#925
adpa-ms wants to merge 5 commits into
mainfrom
user/adibpa/copilot-mxc-error-structured-fields

adpa-ms commented Aug 15, 2026 •

edited

Loading

Uh oh!

azure-pipelines Bot commented Aug 15, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

adpa-ms commented Aug 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📖 Description

Notes for reviewers

🔗 References

🔍 Validation

✅ Checklist

📋 Issue Type

Microsoft Reviewers: Open in CodeFlow

Uh oh!

azure-pipelines Bot commented Aug 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adpa-ms commented Aug 15, 2026 •

edited

Loading