Carry the failing API's detail through the in-process SDKs - #925
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Carries structured platform API failure details through the Rust, C ABI, and C# SDK layers, addressing #924.
Changes:
- Preserves
ApiFailurein 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.
392dbe6 to
8637dd9
Compare
There was a problem hiding this comment.
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, andstate_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 andsrc/core/mxc-sdk/README.md. This newApiFailureexport and its accessors are covered only in the crate docs; the README still mentions bareError/ErrorCodebehavior 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_IsCarriedAlongsideTheCodeAndMessagebypasses this helper by constructingMxcExceptiondirectly. A field-order or UTF-8 mapping regression here would therefore pass. Add a unit test that builds anMxcErrorDetailwith unmanaged UTF-8 values, callsToException, 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));
8637dd9 to
b76d2d7
Compare
b76d2d7 to
9c149f5
Compare
There was a problem hiding this comment.
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 createError::new(...)and then setnative_codewhile leavingoperationasNone;Displaywill silently omit that status, whileMxcErrorDetail::from_errorpropagates the invalid pair. Please use the groupedOption<Box<ApiFailure>>representation described in the PR, withoperationrequired, 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>,
Branden Bonaby (bbonaby)
left a comment
There was a problem hiding this comment.
Two serious issues found in the Rust/adversarial review.
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
9c149f5 to
cb4e2b0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
sdk/node/src/errors.ts:32
- This still implies that
remediationcannot exist withoutoperation, while this PR explicitly makes remediation independent (and adds a remediation-only Rust test). It also drops the remainingnativeCode⇒operationinvariant that the PR description says is still guaranteed. Please document those two rules directly so TypeScript consumers get the intended contract.
* **Invariant:** `operation` marks that an underlying API call was in flight.
* A failure MXC raises before or outside any API call carries only `code` and
* `message`.
sdk/node/README.md:288
- Saying pre-backend failures carry only
codeandmessagereintroduces the removedremediation⇒operationcoupling. It also leaves the actualnativeCode⇒operationinvariant undocumented here. Align this README with the contract described by the PR and the Rust remediation-only case.
`operation`, `nativeCode` and `remediation` are optional. A failure MXC raises before reaching the backend — a malformed request or id, or a policy rejection — carries only `code` and `message`.
docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md:822
- This wording still makes remediation impossible without an operation because non-API failures are said to carry only
codeandmessage, contrary to this PR's remediation-only public shape. It also no longer states the retainednativeCode⇒operationinvariant. Please describe both rules explicitly.
**Invariant:** `operation` marks that an API operation was in flight. A failure
MXC raises before or outside any API call — a malformed request or id, a policy
rejection, or an internal failure of MXC's own machinery — carries only `code`
and `message`.
scripts/check-dotnet-bindings-codegen.js:179
- The new parity gate checks csbindgen inputs against MSBuild inputs but not against Cargo's
rerun-if-changedlist. If a future extern source is added to both checked lists but omitted fromrerun-if-changed, MSBuild will invoke Cargo after edits while Cargo skips the build script, leaving stale bindings—the same incremental failure this gate is intended to prevent. Parse and compare thecargo:rerun-if-changedsource paths as a third set.
const notDeclared = csbindgenInputs.filter((rel) => {
const expected = `${CRATE_REL_ROOT}/${rel}`;
return !declaredInputs.some((declared) => declared.endsWith(expected));
});
The docs stated that `nativeCode` and `remediation` never appear without `operation`. Nothing enforced it, and this change had already decoupled `remediation` on the flattened public error, so the statements were false wherever they still appeared. Removed rather than enforced: the coupling was never wanted. That reaches the TypeScript SDK and both wire docs because they carried the same claim. `from_sdk_error` and the `alloc_cstring` non-null guarantee had no tests. The guarantee is load-bearing: a null message means success, so a failure that allocated null would read as one.
cb4e2b0 to
c8a8ec0
Compare
📖 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. In-process callers get none of it.
wxc_common::mxc_error::MxcErrorcarries that detail inapi_failure, but the public SDK facademxc_engine::error::Errorholds only{ code, message }and itsFrom<MxcError>dropsapi_failureon the floor. Everything below is blind: the Rust SDK, the C ABI, and the C#MxcException. Two callers observing the same failure get materially different diagnoses — in-process callers seebackend_error: The provision was not found.and no way to learn which call failed.This carries the detail through all three layers.
Error, besidecodeandmessage, matching the wire envelope, the C ABI, the C# binding and the TypeScript SDK. Nesting them behind a sub-struct would make the Rust SDK the one surface of five shaped differently.MxcErrorDetailused by every failing surface —MxcRunResult,MxcStateAwareResult, and theout_errorparameter — so a binding learns the same things from any of them and frees them all the same way.Operation/NativeCode/RemediationonMxcException. A null pointer becomesnull, never""; null means the layer below supplied nothing.The Rust SDK gains structured errors too, not only C#.
Notes for reviewers
out_errorchanges ownership shape, from a library-allocated string to caller-provided storage for one detail. The detail owns heap strings with no destructor, so a caller passing null would leak them;finish_spawnfrees it on that path.nativeCodeandremediationdo not appear withoutoperation. This decouplesremediation, so the claims no longer hold — including in the TypeScript SDK and both wire docs.Erroris#[non_exhaustive], mirroring the wire envelope andMxcError. Adding it later is a breaking change; removing it is not.ErrorCodestays closed, mirroringMxcErrorCode.Inputsname the same files; adding to one and not the other compiles against stale bindings.mxc_versionis the one C ABI entry point notcatch_unwind-wrapped, contradicting the module header. Now wrapped, 23 of 23.REQUIRED_ENTRY_POINTSto the full P/Invoke surface belongs with the C# workstream: the compiler already catches rename and removal.#[repr(C)]types is expected.🔗 References
Resolves #924
🔍 Validation
wxc_host_prep(16 tests) run elevated: fmt, clippy-D warnings, workspace tests with the isolation-session feature on and off, arm64 cross-build, six versioning gates, both C# SDK gates,dotnet test(47), and the Node build, unit, pack and integration legs.IsoSessionCli list-usersagreed atFound 0 agent user(s).ErrorCodeparity is unaffected; this adds no newMXC_STATUS_*codes.✅ Checklist
📋 Issue Type
Microsoft Reviewers: Open in CodeFlow