From 9868a077b32cf07288a6e349ce6c8966943d330d Mon Sep 17 00:00:00 2001 From: adpa-ms <> Date: Mon, 27 Jul 2026 18:27:27 -0700 Subject: [PATCH 1/3] feat(iso): surface structured error fields on the state-aware wire envelope Promote the components of an IsolationSession failure out of the concatenated `message` string and into discrete fields on the wire error envelope: `operation`, `nativeCode` and `remediation`, alongside the existing `code` and `message`. On the state-aware path `message` becomes the bare human-readable text; for a semantic API failure that is the API's own message, passed through verbatim. Wire model (`wxc_common::mxc_error`) - `ApiFailure { operation, native_code?, remediation? }`, held boxed on `MxcError`. Grouping makes the envelope invariant unrepresentable to violate -- `nativeCode` and `remediation` cannot exist without `operation` -- and keeps `MxcError` small enough that every `Result<_, MxcError>` in the workspace stays under clippy's `result_large_err` threshold. - `ErrorEnvelope` gains the three fields, each omitted when unset; `native_code` serialises as camelCase `nativeCode`. IsolationSession backend - `Lifecycle`/`Stale` carry the components structurally instead of a pre-formatted string; `LifecycleFailure::Internal` makes an MXC-side failure structurally incapable of naming an API operation. - Classification split into a pure function so the rules are unit-testable -- `IsoSessionError` is WinRT-activated and cannot be constructed in a test. Same split applied to the activation-failure mapping, which now reports `backend_unavailable` with its operation and HRESULT. - `operation` is interface-qualified, low-cardinality and parameter-free (a failing environment insert names the variable in `message`). - Fixes a latent bug: the `ERROR_NOT_FOUND` -> `stale_id` promotion applied to provision too, which cannot produce a stale id because it mints the sandbox. It is now restricted to non-provision operations, and stays semantic-path only -- a transport HRESULT of the same value has none of the provenance that gives it that meaning, so promoting it would emit a false `stale_id` and tell the caller to destroy a healthy sandbox. One-shot is deliberately untouched: `Display` still composes the full human string, including the category prefix, because that path has no structured envelope to read the fields from. TypeScript SDK - `MxcError` gains a constructor overload taking a flat `MxcErrorFields` object mirroring the wire shape. The positional signature is retained and declared last, so existing callers and `ConstructorParameters` are unaffected. - `mxcErrorFromEnvelope` is the single wire-to-error boundary, including the unknown-code passthrough; all envelope-parsing sites route through it. Also removes several pre-existing OS-internal names from prose in the files touched, per repo convention. Verified on the retail host: cargo fmt, clippy (--all-features), build and test with isolation_session ON and OFF, SDK unit, SDK integration, the versioning gate suite, and wxc_host_prep in an elevated shell -- all green. The iso E2E suites still need a VM run. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 67a135de-4549-4c4e-8428-6f2a993e9bf2 --- docs/isolation-session/state-aware-rust.md | 53 +- .../mxc-state-aware-sandbox-api-overview.md | 14 +- .../mxc-state-aware-sandbox-api.md | 38 +- sdk/node/README.md | 24 +- sdk/node/src/errors.ts | 99 ++- sdk/node/src/index.ts | 3 + sdk/node/src/sandbox.ts | 4 +- sdk/node/src/state-aware-helper.ts | 6 +- sdk/node/src/state-aware.ts | 5 +- .../isolation-session-state-aware.test.ts | 32 + sdk/node/tests/unit/errors.test.ts | 96 ++- sdk/node/tests/unit/state-aware.test.ts | 32 + .../isolation_session/common/src/error.rs | 637 +++++++++++++++--- .../isolation_session/common/src/manager.rs | 162 +++-- .../common/src/process_options.rs | 28 +- src/core/wxc_common/src/mxc_error.rs | 237 ++++++- ...un_isolation_session_state_aware_tests.ps1 | 32 + 17 files changed, 1292 insertions(+), 210 deletions(-) diff --git a/docs/isolation-session/state-aware-rust.md b/docs/isolation-session/state-aware-rust.md index 753b87246..fac571181 100644 --- a/docs/isolation-session/state-aware-rust.md +++ b/docs/isolation-session/state-aware-rust.md @@ -223,12 +223,55 @@ wire-format `MxcError` codes via `map_lifecycle_error`: | `IsolationSessionError` variant | Wire `error.code` | Trigger | |---|---|---| | `Policy(...)` | `policy_validation` | Caller-supplied policy field that this phase does not accept — see the honor matrix above. Rejected by `validate_` hooks (state-aware) or `validate_runner` (one-shot). | -| `ServiceUnavailable(...)` | `backend_unavailable` | `IsoSessionOps` activation failure: the `Windows.AI.IsolationSession.Preview` API is unavailable on this OS build (not registered, or the OS feature gate is off). HRESULTs `CLASS_E_CLASSNOTAVAILABLE` (`0x80040111`) or `REGDB_E_CLASSNOTREG` (`0x80040154`). | -| `Stale(...)` | `stale_id` | OS-side `AgentManager::FindActiveAgentUserByProvisionId` returns `HRESULT_FROM_WIN32(ERROR_NOT_FOUND)` (`0x80070490`) — the `provisionId` is missing from both the in-memory cache and the persisted registry. After `deprovision`, every non-provision op against the dead `sandboxId` triggers this. | -| `Lifecycle(...)` | `backend_error` | Any other HRESULT from a lifecycle op. The error message embeds the operation name, HRESULT, OS-side message, and remediation hint where present. | +| `ServiceUnavailable(...)` | `backend_unavailable` | Activation failure of the in-proc IsolationSession runtime API: it is unavailable on this OS build (not registered, or the OS feature gate is off). HRESULTs `CLASS_E_CLASSNOTAVAILABLE` (`0x80040111`) or `REGDB_E_CLASSNOTREG` (`0x80040154`). | +| `Stale(...)` | `stale_id` | The OS service reports `HRESULT_FROM_WIN32(ERROR_NOT_FOUND)` (`0x80070490`) — the agent user is unknown to it. After `deprovision`, every non-provision op against the dead `sandboxId` triggers this. | +| `Lifecycle(...)` | `backend_error` | Any other failure of a lifecycle op, whether the API reported it semantically or the call itself could not be completed. | -`error.details` is empty in v1. The HRESULT and OS-side message live inside -`error.message` rather than as a structured field. +### Structured failure fields + +The components of an API failure travel as **discrete fields** on the wire error +envelope — `operation`, `nativeCode` and `remediation` — rather than being concatenated +into `message`. `message` holds the bare human-readable text; for a semantic API failure +that is the API's own message, passed through verbatim. + +| Failure | `operation` | `nativeCode` | `remediation` | +|---|---|---|---| +| Semantic API failure (the call completed and reported an error) | ✅ | ✅ | when the API supplies one | +| Transport failure (the call could not be completed, or a result property could not be read) | ✅ | ✅ | — | +| Activation failure (`backend_unavailable`) | ✅ | ✅ | — | +| The API's status code itself could not be read | ✅ | — | best-effort | +| MXC-internal failure (relay threads, console handles) | — | — | — | +| `Policy` and the MXC-side `malformed_*` rejections | — | — | — | + +**Invariant:** `nativeCode` implies `operation`, and `remediation` implies `operation`. +`operation` marks that an API operation was in flight; neither refinement appears alone. + +`operation` is the interface-qualified member name — for example +`IsoSessionOps.StopSessionAsync`. It is deliberately low-cardinality and free of call +parameters (a failing environment-variable insert names the variable in `message`, not +in `operation`) so that consumers can aggregate on it. Where a lifecycle call succeeds +but reading one of its result properties fails, `operation` stays the lifecycle call and +the finer step is described in `message`. + +`nativeCode` is the HRESULT rendered as lowercase hex, e.g. `0x80070490`. + +`error.details` is unused by this backend. It remains the escape hatch for +backend-specific structured data that has no cross-backend meaning; the three named +fields above are backend-neutral and so live on the envelope itself. + +### The `stale_id` promotion is semantic-path only + +`ERROR_NOT_FOUND` is promoted to `stale_id` **only** when it arrives through the API's +semantic error channel, and **only** for non-provision operations. + +- *Semantic only:* the in-proc client maps its internal codes to standard HRESULTs when + it builds the error object, and that mapping is what gives `0x80070490` the meaning + "agent user not provisioned". The same value arriving as a transport failure has no + such provenance — it could be any "not found" from activation or RPC — so promoting it + would emit a false `stale_id`, whose remediation is "re-provision; treat the id as + dead", and destroy a healthy sandbox. +- *Non-provision only:* provision mints the agent user. There is no `sandboxId` yet, so + reporting a stale one would be incoherent. ## Cancellation diff --git a/docs/state-aware-lifecycle/mxc-state-aware-sandbox-api-overview.md b/docs/state-aware-lifecycle/mxc-state-aware-sandbox-api-overview.md index 1244f9a6f..23a2d4ec9 100644 --- a/docs/state-aware-lifecycle/mxc-state-aware-sandbox-api-overview.md +++ b/docs/state-aware-lifecycle/mxc-state-aware-sandbox-api-overview.md @@ -43,7 +43,7 @@ on the response, and neither shape carries `containerId`. | TypeScript SDK (reference §6) | Five new functions: `provisionSandbox`, `startSandbox`, `execInSandbox` / `execInSandboxAsync`, `stopSandbox`, `deprovisionSandbox`. Branded `SandboxId` type tagging ids by backend (`containment` named once at provision, inferred from the id thereafter). Per-(backend, phase) typed `*Config` interfaces (e.g. `IsolationSessionProvisionConfig`) that absorb cross-cutting fields directly — no separate policy parameter. Per-phase typed `*Result` types per backend. `AbortSignal` cancellation via the existing `SandboxSpawnOptions`. Typed `MxcError` class carrying a closed-enum `code`. | `spawnSandbox` family preserved. `ContainmentBackend` extension reused. The wire-format-aligned `Process` / `Filesystem` / `Network` / `UiConfig` interfaces from `sdk/node/src/types.ts` are reused as field types inside state-aware Configs. `SandboxSpawnOptions` reused as the third-arg options bag (gains `signal?: AbortSignal`). `*Config` naming convention reused. | | JSON wire format (reference §7) | Top-level `phase` discriminator. Top-level `sandboxId`. `containment` carried on provision only; non-provision phases route via the `sandboxId` prefix. Per-phase nesting under `experimental..`. Named envelope types as a TypeScript discriminated union. | One-shot configs (no `phase`) work unchanged. Cross-cutting `filesystem` / `network` / `ui` at top level for state-aware too — backends declare per-phase honor. | | Rust executor (reference §9) | Dispatch arm for state-aware. New `StatefulSandboxBackend` trait. Rust mirror of the wire envelope (the `wire::MxcConfig` parse target). | `ScriptRunner` trait. Existing one-shot dispatch path. Existing backends unchanged. | -| Error model (reference §8) | Closed enum of 12 codes. `MxcError` class with `code: ErrorCode`. `details` open object. | Existing one-shot error paths preserved. | +| Error model (reference §8) | Closed enum of 12 codes. `MxcError` class with `code: ErrorCode`. Named structured fields `operation` / `nativeCode` / `remediation` for failures raised by an underlying platform API, plus the open `details` object for backend-specific data. | Existing one-shot error paths preserved. | | Plug-in surface (reference §11) | Implement `StatefulSandboxBackend`. Define typed per-(backend, phase) `*Config` interfaces. Declare the trait's `ID_PREFIX` and `BACKEND_KEY` consts. Document the cross-cutting honor matrix. | Ephemeral-only backends require no changes. | ## Lifecycle @@ -144,7 +144,8 @@ The wire envelope is a TypeScript discriminated union over `phase`, JSON-seriali The Rust executor parses the same shape into the typed wire model (`wire::MxcConfig`, reference §9.1). The only `Record` in the contract is `ErrorEnvelope.details` — the escape hatch for backend-specific structured failure -information. +information. Backend-neutral failure detail travels in the error envelope's named +fields (`operation`, `nativeCode`, `remediation`) instead. ```typescript interface OneShotRequest { @@ -374,8 +375,11 @@ rejected. ## Error codes -Closed enum at the MXC layer; backend-specific failures use `backend_error` with -structured `details`. Reference §8 has the full list and the `MxcError` mapping. +Closed enum at the MXC layer; backend-specific failures use `backend_error`, with the +detail carried in the error envelope's named structured fields (`operation`, +`nativeCode`, `remediation`) and, where a backend needs data with no cross-backend +meaning, in `details`. Reference §8 has the full list and the `MxcError` mapping, and +§7.3 the field invariant. | Group | Codes | |---|---| @@ -384,7 +388,7 @@ structured `details`. Reference §8 has the full list and the `MxcError` mapping | Id problems | `malformed_id`, `stale_id` | | State-machine violations | `not_provisioned`, `not_started`, `already_started`, `already_stopped` | | Config / policy | `policy_validation` | -| Catch-all | `backend_error` (with structured `details`) | +| Catch-all | `backend_error` | Process-runtime kill conditions (timeouts, backend-initiated termination) surface as sentinel exit codes from the exec process, not as typed wire-format errors. Each code diff --git a/docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md b/docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md index ed67b27cb..d55575ba7 100644 --- a/docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md +++ b/docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md @@ -578,7 +578,8 @@ The wire contract is a typed envelope, JSON-serialised, that flows from the SDK executor (`wxc-exec` on Windows, `lxc-exec` on Linux) via the existing `--config-base64` CLI argument. Both ends agree on the same shape: the SDK serialises a TypeScript value, the executor parses the same value into a Rust struct (§9.1). The only open content in -the envelope is at the leaves of `ErrorEnvelope.details`. +the envelope is at the leaves of `ErrorEnvelope.details`; every other field, including +the error envelope's named structured fields, is statically typed. ### 7.1 Request envelope @@ -750,6 +751,9 @@ output to a file and leaves stderr as pure script content. interface ErrorEnvelope { code: ErrorCode; message: string; + operation?: string; + nativeCode?: string; + remediation?: string; details?: Record; } @@ -778,10 +782,36 @@ Because MXC diagnostic output is routed to `stderr` in state-aware mode, this stdout-based discrimination has no false positives or negatives — the content is always either pure envelope or pure script output. +`code` and `message` are always present. `code` is the machine-readable category a +consumer branches on; `message` is the human-readable description, and for a failure +raised by an underlying platform API it is that API's own message, passed through +verbatim rather than concatenated with the other fields. + +The three optional named fields describe a failure that originated in an underlying +platform API: + +| Field | Meaning | +|---|---| +| `operation` | The API call that failed, namespaced by its interface — e.g. `IsoSessionOps.RunProcessWithOptionsAsync`. Low-cardinality and free of call parameters, so it is safe to aggregate on in telemetry. | +| `nativeCode` | The underlying platform status as a string. An HRESULT such as `0x80070490` on Windows; the field is platform-neutral, so another backend can carry an errno or equivalent. | +| `remediation` | The API's actionable "how to fix it" hint, when it supplies one. | + +**Invariant:** `nativeCode` implies `operation`, and `remediation` implies `operation`. +`operation` marks that an API operation was in flight; the other two refine it, and +neither ever appears alone. 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`. + +**Which fields earn a place here.** A named top-level field is for a **backend-neutral** +concept: `operation`, `nativeCode` and `remediation` all apply equally to a Windows +HRESULT, a Linux errno, or any other backend's failure. **Backend-specific** structured +data belongs in `details` instead. That is what keeps `details` from becoming vestigial +as named fields are added — it remains the designated home for anything without a +cross-backend meaning. + `ErrorEnvelope.details` is the only `Record` in the contract. It's the -escape hatch backends use to convey structured failure information that's -per-error-code (a backend's native HRESULT, partial output captured before a timeout, -etc.). Each backend's plan doc (§11) specifies what `details` contains for which error +escape hatch backends use to convey structured failure information that has no dedicated +field. Each backend's plan doc (§11) specifies what `details` contains for which error codes. ### 7.4 Worked example: IsolationSession end-to-end diff --git a/sdk/node/README.md b/sdk/node/README.md index 12a36adcf..09d150d3e 100644 --- a/sdk/node/README.md +++ b/sdk/node/README.md @@ -265,6 +265,26 @@ await deprovisionSandbox(sandboxId, undefined, opts); `windows_sandbox` follows the same shape (substitute the containment string and provide `filesystem.readwritePaths` / `readonlyPaths` at provision if needed). See [`docs/windows-sandbox/windows-sandbox.md`](https://github.com/microsoft/mxc/blob/main/docs/windows-sandbox/windows-sandbox.md) for the per-phase config matrix. +**Handling failures.** Every lifecycle call rejects with a typed `MxcError`. Branch on `code` first; when the failure came from an underlying platform API, the error also carries discrete diagnostic fields rather than a prose blob: + +```typescript +import { MxcError } from '@microsoft/mxc-sdk'; + +try { + await startSandbox(sandboxId, {}, { experimental: true }); +} catch (err) { + if (err instanceof MxcError) { + if (err.code === 'stale_id') { /* the sandbox is gone -- re-provision */ } + console.error(err.message); // bare, human-readable + console.error(err.operation); // e.g. 'IsoSessionOps.StartSessionAsync' + console.error(err.nativeCode); // e.g. '0x80070490' + console.error(err.remediation); // the API's own fix-it hint, when it supplies one + } +} +``` + +`operation`, `nativeCode` and `remediation` are optional and travel together: `nativeCode` and `remediation` never appear without `operation`. A failure MXC raises before reaching the backend — a malformed request or id, or a policy rejection — carries only `code` and `message`. + Full design and API: [`docs/state-aware-lifecycle/`](https://github.com/microsoft/mxc/tree/main/docs/state-aware-lifecycle/). @@ -391,7 +411,9 @@ getTemporaryFilesPolicy(env?) → FilesystemPolicyResult UiCapabilitySupport // Errors (typed wire-format errors from wxc-exec) -ErrorCode, MxcError, mxcErrorFromCode(code) +ErrorCode, MxcError, MxcErrorFields, WireError +mxcErrorFromCode(code, message, details?) → MxcError +mxcErrorFromEnvelope(wireError) → MxcError ``` Full TypeScript definitions ship with the package (`dist/index.d.ts`). All exports are named exports from `@microsoft/mxc-sdk`. diff --git a/sdk/node/src/errors.ts b/sdk/node/src/errors.ts index 6f08332cd..324ca500e 100644 --- a/sdk/node/src/errors.ts +++ b/sdk/node/src/errors.ts @@ -5,8 +5,8 @@ * Closed set of MXC wire-format error codes. Mirrors `MxcErrorCode` on the * Rust side one-for-one and serialises as the same snake_case strings on * the wire. Backend-specific failures that don't fit one of these codes - * surface as `backend_error`, with structured information carried in - * `details`. + * surface as `backend_error`, with structured information carried in the + * named fields below (or in `details`). */ export type ErrorCode = | 'malformed_request' @@ -22,6 +22,51 @@ export type ErrorCode = | 'policy_validation' | 'backend_error'; +/** + * Every field an `MxcError` can carry, in the same flat shape as the wire + * error envelope — `operation`, `nativeCode` and `remediation` sit alongside + * `code` and `message`, not nested inside `details`. + * + * **Invariant:** `nativeCode` implies `operation`, and `remediation` implies + * `operation`. `operation` marks that an underlying API call was in flight; + * the other two refine it, and neither appears on its own. A failure MXC + * raises before or outside any API call carries only `code` and `message`. + */ +export interface MxcErrorFields { + /** Machine-readable category. Branch on this first. */ + code: ErrorCode; + /** Human-readable description of the failure. */ + message: string; + /** + * The underlying API call that failed, namespaced by its interface — e.g. + * `IsoSessionOps.RunProcessWithOptionsAsync`. Low-cardinality and free of + * call parameters, so it is safe to group on in telemetry. + */ + operation?: string; + /** + * The underlying platform status as a string — an HRESULT such as + * `0x80070490` on Windows, an errno or equivalent elsewhere. + */ + nativeCode?: string; + /** The API's actionable "how to fix it" hint, when it supplied one. */ + remediation?: string; + /** + * Open extension point for backend-specific structured data that has no + * dedicated field. Named fields are reserved for backend-neutral concepts. + */ + details?: Record; +} + +/** + * The `error` arm of a wire response envelope, as received from the + * executor. Identical to {@link MxcErrorFields} except that `code` is an + * open `string`: an unrecognised code is passed through verbatim rather than + * being coerced or dropped. + */ +export interface WireError extends Omit { + code: string; +} + /** * Typed error thrown by the MXC SDK in response to a wire-format error * envelope. Discriminate by comparing `.code` to a wire-format error code @@ -30,12 +75,33 @@ export type ErrorCode = */ export class MxcError extends Error { readonly code: ErrorCode; + readonly operation?: string; + readonly nativeCode?: string; + readonly remediation?: string; readonly details?: Record; - constructor(code: ErrorCode, message: string, details?: Record) { - super(message); - this.code = code; - this.details = details; + /** Canonical form: pass the full field set as one object. */ + constructor(fields: MxcErrorFields); + /** + * Positional form, retained for compatibility. Declared last so that + * `ConstructorParameters` keeps resolving to this shape. + */ + constructor(code: ErrorCode, message: string, details?: Record); + constructor( + codeOrFields: ErrorCode | MxcErrorFields, + message?: string, + details?: Record, + ) { + const fields: MxcErrorFields = + typeof codeOrFields === 'string' + ? { code: codeOrFields, message: message as string, details } + : codeOrFields; + super(fields.message); + this.code = fields.code; + this.operation = fields.operation; + this.nativeCode = fields.nativeCode; + this.remediation = fields.remediation; + this.details = fields.details; // Restore the prototype chain so `instanceof MxcError` keeps working // after the TypeScript ES2020 → ES5-compatible class downlevelling. Object.setPrototypeOf(this, new.target.prototype); @@ -48,6 +114,9 @@ export class MxcError extends Error { * `string` so callers parsing a wire envelope don't need to narrow first; * unknown codes still produce an `MxcError` with `.code` set to whatever * was on the wire. + * + * For a complete wire envelope prefer {@link mxcErrorFromEnvelope}, which + * carries the structured fields too. */ export function mxcErrorFromCode( code: string, @@ -56,3 +125,21 @@ export function mxcErrorFromCode( ): MxcError { return new MxcError(code as ErrorCode, message, details); } + +/** + * Constructs an `MxcError` from the `error` arm of a wire response envelope. + * + * This is the single place the wire's open `code` string is widened to the + * closed `ErrorCode` union, so unknown-code passthrough behaves identically + * everywhere the SDK parses an envelope. + */ +export function mxcErrorFromEnvelope(error: WireError): MxcError { + return new MxcError({ + code: error.code as ErrorCode, + message: error.message, + operation: error.operation, + nativeCode: error.nativeCode, + remediation: error.remediation, + details: error.details, + }); +} diff --git a/sdk/node/src/index.ts b/sdk/node/src/index.ts index 54df8e634..6ff2a8977 100644 --- a/sdk/node/src/index.ts +++ b/sdk/node/src/index.ts @@ -71,7 +71,10 @@ export { export { ErrorCode, MxcError, + MxcErrorFields, + WireError, mxcErrorFromCode, + mxcErrorFromEnvelope, } from './errors.js'; // Export state-aware lifecycle types diff --git a/sdk/node/src/sandbox.ts b/sdk/node/src/sandbox.ts index e8b6b74ca..b820ac5f0 100644 --- a/sdk/node/src/sandbox.ts +++ b/sdk/node/src/sandbox.ts @@ -9,7 +9,7 @@ import { parse as semverParse } from 'semver'; import { SandboxPolicy, ContainerConfig, ContainmentType, ContainmentBackend } from './types.js'; import { prepareSpawn, diagLogVersion, applyLinuxNetworkPolicy } from './helper.js'; import { diagLog } from './diagnostic.js'; -import { MxcError, mxcErrorFromCode } from './errors.js'; +import { MxcError, mxcErrorFromEnvelope } from './errors.js'; const SUPPORTED_VERSION = '0.8.0-alpha'; const MIN_VERSION = '0.6.0-alpha'; @@ -730,7 +730,7 @@ function tryParseErrorEnvelopeFromLines(output: string): MxcError | null { if (parsed && typeof parsed === 'object' && 'error' in parsed) { const env = parsed.error; if (env && typeof env.code === 'string' && typeof env.message === 'string') { - return mxcErrorFromCode(env.code, env.message, env.details); + return mxcErrorFromEnvelope(env); } } } catch { diff --git a/sdk/node/src/state-aware-helper.ts b/sdk/node/src/state-aware-helper.ts index e7b09f807..0fc072981 100644 --- a/sdk/node/src/state-aware-helper.ts +++ b/sdk/node/src/state-aware-helper.ts @@ -4,7 +4,7 @@ import { spawn } from 'child_process'; import { resolveBinaryAndCommonArgs } from './helper.js'; import { SandboxSpawnOptions } from './sandbox.js'; -import { mxcErrorFromCode } from './errors.js'; +import { mxcErrorFromCode, mxcErrorFromEnvelope, WireError } from './errors.js'; import { diagLog } from './diagnostic.js'; import { Phase, StateAwareContainmentBackend } from './state-aware-types.js'; @@ -99,7 +99,7 @@ export function buildStateAwareEnvelope(args: BuildEnvelopeArgs): Record }; + error: WireError; } export interface WireResultEnvelope { @@ -121,7 +121,7 @@ export function parseNonExecResponse(stdout: string): T { if (parsed && typeof parsed === 'object') { if ('error' in parsed) { const env = (parsed as WireErrorEnvelope).error; - throw mxcErrorFromCode(env.code, env.message, env.details); + throw mxcErrorFromEnvelope(env); } if ('result' in parsed) { return (parsed as WireResultEnvelope).result; diff --git a/sdk/node/src/state-aware.ts b/sdk/node/src/state-aware.ts index 59c663ad0..89fa8f7d8 100644 --- a/sdk/node/src/state-aware.ts +++ b/sdk/node/src/state-aware.ts @@ -4,7 +4,7 @@ import pty from 'node-pty'; import { resolveBinaryAndCommonArgs } from './helper.js'; import { SandboxSpawnOptions } from './sandbox.js'; -import { mxcErrorFromCode } from './errors.js'; +import { mxcErrorFromEnvelope } from './errors.js'; import { diagLog } from './diagnostic.js'; import { DeprovisionConfigFor, @@ -142,8 +142,7 @@ export async function execInSandboxAsync if (exitCode !== 0) { const errorEnvelope = tryParseErrorEnvelope(stdout); if (errorEnvelope) { - const e = errorEnvelope.error; - throw mxcErrorFromCode(e.code, e.message, e.details); + throw mxcErrorFromEnvelope(errorEnvelope.error); } } diff --git a/sdk/node/tests/integration/isolation-session-state-aware.test.ts b/sdk/node/tests/integration/isolation-session-state-aware.test.ts index 47a888155..7ff1633ce 100644 --- a/sdk/node/tests/integration/isolation-session-state-aware.test.ts +++ b/sdk/node/tests/integration/isolation-session-state-aware.test.ts @@ -20,6 +20,7 @@ import path from 'node:path'; import os from 'os'; import { execInSandboxAsync, + IsolationSessionUserConfig, MxcError, provisionSandbox, startSandbox, @@ -200,4 +201,35 @@ describe('IsolationSession state-aware lifecycle E2E', { skip: skipReason }, () (err: unknown) => err instanceof MxcError && err.code === 'policy_validation', ); }); + + // Full chain, negative case. A malformed Entra UPN is rejected by MXC's + // own validation, before any IsolationSession API call is made. The + // structured failure fields describe an API operation that was in flight; + // none was, so they must reach the caller absent rather than empty — + // `nativeCode` and `remediation` never appear without `operation`. + // + // The canonical network acknowledgment is supplied so the only thing wrong + // with this request is the UPN; that keeps the assertion on the message + // independent of the order in which the backend runs its validations. + it('a policy rejection reaches the SDK with no structured failure fields', async () => { + await assert.rejects( + () => provisionSandbox( + 'isolation_session', + { + network: { defaultPolicy: 'allow', allowLocalNetwork: true }, + user: new IsolationSessionUserConfig('missing-the-at-sign', 'token'), + }, + { experimental: true }, + ), + (err: unknown) => { + assert.ok(err instanceof MxcError, `expected MxcError, got ${String(err)}`); + assert.strictEqual(err.code, 'policy_validation'); + assert.match(err.message, /upn/i, `expected the message to name upn: ${err.message}`); + assert.strictEqual(err.operation, undefined, 'operation must be absent'); + assert.strictEqual(err.nativeCode, undefined, 'nativeCode must be absent'); + assert.strictEqual(err.remediation, undefined, 'remediation must be absent'); + return true; + }, + ); + }); }); diff --git a/sdk/node/tests/unit/errors.test.ts b/sdk/node/tests/unit/errors.test.ts index baec5c120..bbd47fc89 100644 --- a/sdk/node/tests/unit/errors.test.ts +++ b/sdk/node/tests/unit/errors.test.ts @@ -3,7 +3,7 @@ import { describe, it } from 'node:test'; import assert from 'node:assert'; -import { ErrorCode, MxcError, mxcErrorFromCode } from '../../src/errors.js'; +import { ErrorCode, MxcError, mxcErrorFromCode, mxcErrorFromEnvelope } from '../../src/errors.js'; const codes: ErrorCode[] = [ 'malformed_request', @@ -65,3 +65,97 @@ describe('mxcErrorFromCode', () => { assert.strictEqual(err.code, 'not_a_real_code'); }); }); + +describe('MxcError structured fields', () => { + // The positional signature predates the object form. Any consumer still + // calling it must keep compiling and behaving identically -- this is the + // non-breaking guarantee for the overload. + it('still accepts the legacy positional form', () => { + const err = new MxcError('backend_error', 'boom', { hresult: '0x80004005' }); + assert.strictEqual(err.code, 'backend_error'); + assert.strictEqual(err.message, 'boom'); + assert.deepStrictEqual(err.details, { hresult: '0x80004005' }); + assert.strictEqual(err.operation, undefined); + assert.strictEqual(err.nativeCode, undefined); + assert.strictEqual(err.remediation, undefined); + assert.ok(err instanceof MxcError); + assert.ok(err instanceof Error); + }); + + // `ConstructorParameters` resolves to the LAST overload. Keeping the + // positional signature last preserves the pre-existing type-level result. + it('keeps ConstructorParameters resolving to the positional form', () => { + const args: ConstructorParameters = ['stale_id', 'boom']; + const err = new MxcError(...args); + assert.strictEqual(err.code, 'stale_id'); + }); + + it('accepts the object form and exposes every field', () => { + const err = new MxcError({ + code: 'stale_id', + message: 'agent user not found', + operation: 'IsoSessionOps.StopSessionAsync', + nativeCode: '0x80070490', + remediation: 'Re-provision the sandbox.', + details: { phase: 'stop' }, + }); + assert.strictEqual(err.code, 'stale_id'); + assert.strictEqual(err.message, 'agent user not found'); + assert.strictEqual(err.operation, 'IsoSessionOps.StopSessionAsync'); + assert.strictEqual(err.nativeCode, '0x80070490'); + assert.strictEqual(err.remediation, 'Re-provision the sandbox.'); + assert.deepStrictEqual(err.details, { phase: 'stop' }); + assert.ok(err instanceof MxcError); + assert.ok(err instanceof Error); + assert.strictEqual(err.name, 'MxcError'); + }); + + it('leaves structured fields undefined when the object omits them', () => { + const err = new MxcError({ code: 'policy_validation', message: 'bad policy' }); + assert.strictEqual(err.operation, undefined); + assert.strictEqual(err.nativeCode, undefined); + assert.strictEqual(err.remediation, undefined); + assert.strictEqual(err.details, undefined); + }); +}); + +describe('mxcErrorFromEnvelope', () => { + it('maps every field off the wire envelope', () => { + const err = mxcErrorFromEnvelope({ + code: 'backend_error', + message: 'the operation failed', + operation: 'IsoSessionOps.AddUserAsync', + nativeCode: '0x80004005', + remediation: 'Check the host configuration.', + details: { extra: true }, + }); + assert.ok(err instanceof MxcError); + assert.strictEqual(err.code, 'backend_error'); + assert.strictEqual(err.message, 'the operation failed'); + assert.strictEqual(err.operation, 'IsoSessionOps.AddUserAsync'); + assert.strictEqual(err.nativeCode, '0x80004005'); + assert.strictEqual(err.remediation, 'Check the host configuration.'); + assert.deepStrictEqual(err.details, { extra: true }); + }); + + it('omits fields the wire envelope did not carry', () => { + const err = mxcErrorFromEnvelope({ code: 'policy_validation', message: 'bad policy' }); + assert.strictEqual(err.operation, undefined); + assert.strictEqual(err.nativeCode, undefined); + assert.strictEqual(err.remediation, undefined); + assert.strictEqual(err.details, undefined); + }); + + it('passes an unknown wire code through verbatim', () => { + const err = mxcErrorFromEnvelope({ code: 'not_a_real_code', message: 'boom' }); + assert.ok(err instanceof MxcError); + assert.strictEqual(err.code, 'not_a_real_code'); + }); + + for (const code of codes) { + it(`maps '${code}' from an envelope`, () => { + const err = mxcErrorFromEnvelope({ code, message: 'boom' }); + assert.strictEqual(err.code, code); + }); + } +}); diff --git a/sdk/node/tests/unit/state-aware.test.ts b/sdk/node/tests/unit/state-aware.test.ts index 254836e22..4328a1dfe 100644 --- a/sdk/node/tests/unit/state-aware.test.ts +++ b/sdk/node/tests/unit/state-aware.test.ts @@ -183,6 +183,38 @@ describe('parseNonExecResponse', () => { }); }); + it('surfaces the structured failure fields from the wire envelope', () => { + const stdout = JSON.stringify({ + error: { + code: 'stale_id', + message: 'agent user not found', + operation: 'IsoSessionOps.StopSessionAsync', + nativeCode: '0x80070490', + remediation: 'Re-provision the sandbox.', + }, + }); + assert.throws(() => parseNonExecResponse(stdout), (err: unknown) => { + return err instanceof MxcError && + err.code === 'stale_id' && + err.message === 'agent user not found' && + err.operation === 'IsoSessionOps.StopSessionAsync' && + err.nativeCode === '0x80070490' && + err.remediation === 'Re-provision the sandbox.'; + }); + }); + + // An MXC-side rejection has no API call in flight, so the structured + // fields must stay absent rather than arriving as empty strings. + it('leaves the structured fields undefined when the envelope omits them', () => { + const stdout = JSON.stringify({ error: { code: 'policy_validation', message: 'bad policy' } }); + assert.throws(() => parseNonExecResponse(stdout), (err: unknown) => { + return err instanceof MxcError && + err.operation === undefined && + err.nativeCode === undefined && + err.remediation === undefined; + }); + }); + it('throws a plain Error on unparseable stdout', () => { assert.throws(() => parseNonExecResponse('not json'), (err: unknown) => { return err instanceof Error && !(err instanceof MxcError); diff --git a/src/backends/isolation_session/common/src/error.rs b/src/backends/isolation_session/common/src/error.rs index 5e5404003..db310fae4 100644 --- a/src/backends/isolation_session/common/src/error.rs +++ b/src/backends/isolation_session/common/src/error.rs @@ -3,40 +3,172 @@ //! Typed error model for the IsolationSession backend and the conversions to //! `ScriptResponse` (one-shot) and `MxcError` (state-aware dispatch). +//! +//! Failures raised by the IsolationSession API are carried **structurally** +//! — operation, HRESULT, message and remediation stay separate fields all the +//! way to the wire, so a caller can react to them without parsing prose. The +//! one-shot path has no structured envelope, so [`IsolationSessionError`]'s +//! `Display` folds the same components back into one human-readable string. use wxc_common::models::ScriptResponse; -use wxc_common::mxc_error::MxcError; +use wxc_common::mxc_error::{ApiFailure, MxcError, MxcErrorCode}; use isolation_session_bindings::bindings::{IsoSessionError, IsoSessionResult}; +/// Interface-qualified names of the API operations this backend invokes. +/// +/// These are the values that reach the wire as `error.operation`. They are +/// deliberately constants rather than formatted strings: the field must stay +/// low-cardinality and free of call parameters so it can be grouped in +/// telemetry. +pub(super) mod op { + pub(crate) const ACTIVATE: &str = "IsoSessionOps.ActivateInstance"; + pub(crate) const ADD_USER: &str = "IsoSessionOps.AddUserAsync"; + pub(crate) const START_SESSION: &str = "IsoSessionOps.StartSessionAsync"; + pub(crate) const RUN_PROCESS: &str = "IsoSessionOps.RunProcessWithOptionsAsync"; + pub(crate) const STOP_SESSION: &str = "IsoSessionOps.StopSessionAsync"; + pub(crate) const REMOVE_USER: &str = "IsoSessionOps.RemoveUserAsync"; + + pub(crate) const OPTIONS_NEW: &str = "IsoSessionProcessOptions.new"; + pub(crate) const OPTIONS_TIMEOUT: &str = "IsoSessionProcessOptions.SetTimeoutMilliseconds"; + pub(crate) const OPTIONS_WORKING_DIR: &str = "IsoSessionProcessOptions.SetWorkingDirectory"; + pub(crate) const OPTIONS_INTERACTIVE: &str = "IsoSessionProcessOptions.SetInteractiveConsole"; + pub(crate) const OPTIONS_REDIRECT_STDIN: &str = + "IsoSessionProcessOptions.SetRedirectStandardInput"; + pub(crate) const OPTIONS_REDIRECT_STDOUT: &str = + "IsoSessionProcessOptions.SetRedirectStandardOutput"; + pub(crate) const OPTIONS_REDIRECT_STDERR: &str = + "IsoSessionProcessOptions.SetRedirectStandardError"; + pub(crate) const OPTIONS_ENVIRONMENT: &str = "IsoSessionProcessOptions.Environment"; +} + +/// `HRESULT_FROM_WIN32(ERROR_NOT_FOUND)`. Every non-provision lifecycle op +/// (start / exec / stop / deprovision) surfaces this HRESULT when the +/// agent user is unknown to the OS API; we promote it to `Stale` so a +/// deprovisioned `sandbox_id` reads as `MxcError::StaleId` at the dispatch +/// boundary, not a generic backend error. +const ERROR_NOT_FOUND_HRESULT: u32 = 0x80070490; + +/// `CLASS_E_CLASSNOTAVAILABLE` — the runtime class is known but cannot be +/// activated on this OS build. +const CLASS_E_CLASSNOTAVAILABLE_HRESULT: u32 = 0x80040111; + +/// `REGDB_E_CLASSNOTREG` — the runtime class is not registered at all. +const REGDB_E_CLASSNOTREG_HRESULT: u32 = 0x80040154; + +/// Renders an HRESULT for the wire `nativeCode` field. +fn format_native_code(code: u32) -> String { + format!("{code:#010x}") +} + +/// The components of a failure raised by the IsolationSession API, kept +/// separate rather than pre-formatted. +/// +/// `operation` is always present — this type only describes failures where an +/// API call was in flight. `code` is absent only when the status could not be +/// read; `remediation` only when the API supplied one. That is what upholds +/// the `MxcError` invariant that `nativeCode` and `remediation` never appear +/// without `operation`. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(super) struct IsoApiFailure { + /// Interface-qualified operation, e.g. `IsoSessionOps.AddUserAsync`. + pub operation: String, + /// The underlying HRESULT, when it could be read. + pub code: Option, + /// The bare human-readable message — no operation prefix, no HRESULT, no + /// remediation folded in. + pub message: String, + /// The API-supplied "how to fix it" hint, when it provided one. + pub remediation: Option, +} + +impl IsoApiFailure { + /// Folds the components back into one human-readable string. + /// + /// Only the one-shot path consumes this: it has no structured error + /// envelope, so the string is the sole carrier of the detail. The + /// state-aware path reads the fields directly and must not use this. + fn describe(&self) -> String { + let mut out = format!("{}: {}", self.operation, self.message); + if let Some(code) = self.code { + out.push_str(&format!(" (HRESULT: {})", format_native_code(code))); + } + if let Some(remediation) = &self.remediation { + out.push_str(&format!(" -- remediation: {remediation}")); + } + out + } + + /// Builds the wire error, attaching the structured components. + fn into_mxc_error(self, code: MxcErrorCode) -> MxcError { + let mut failure = ApiFailure::new(self.operation); + if let Some(hresult) = self.code { + failure = failure.with_native_code(format_native_code(hresult)); + } + if let Some(remediation) = self.remediation { + failure = failure.with_remediation(remediation); + } + MxcError::new(code, self.message).with_api_failure(failure) + } +} + +/// A lifecycle step failed. The two arms exist so that an MXC-internal +/// failure *cannot* carry an API operation: there was no API call in flight, +/// so there is nothing to name. +#[derive(Debug)] +pub(super) enum LifecycleFailure { + /// An IsolationSession API call failed. + Api(IsoApiFailure), + /// MXC's own machinery failed (thread creation, console handles, and + /// other work that is not an API call). Message-only by construction. + Internal(String), +} + /// Categorised errors from the IsolationSession backend. #[derive(Debug)] pub(super) enum IsolationSessionError { /// Caller-supplied container policy carries a field this backend does - /// not support (filesystem rules, network rules, proxy). + /// not support (filesystem rules, network rules, proxy). Raised by MXC + /// before any API call, so it carries no structured components. Policy(String), - /// The in-proc `Windows.AI.IsolationSession.Preview` `IsoSessionOps` API is not - /// available on this host (DLL not registered or the OS feature gate - /// is off). - ServiceUnavailable(String), + /// The in-proc IsolationSession runtime API is not available on this + /// host (not registered, or the OS feature gate is off). This is a real + /// COM activation failure, so it does carry the operation and the + /// HRESULT. + ServiceUnavailable(IsoApiFailure), /// A lifecycle step (provision / start / exec / stop / deprovision) - /// returned a failure from the OS API. - Lifecycle(String), + /// failed. + Lifecycle(LifecycleFailure), /// The OS API could not find the agent user — the sandbox has been /// deprovisioned (or never existed in this user's session). Surfaces /// as `MxcError::StaleId` at the dispatch boundary. - Stale(String), + Stale(IsoApiFailure), } impl std::fmt::Display for IsolationSessionError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { Self::Policy(msg) => write!(f, "Isolation Session policy error: {}", msg), - Self::ServiceUnavailable(msg) => { - write!(f, "Isolation Session service unavailable: {}", msg) + Self::ServiceUnavailable(failure) => { + write!( + f, + "Isolation Session service unavailable: {}", + failure.describe() + ) + } + Self::Lifecycle(LifecycleFailure::Api(failure)) => { + write!( + f, + "Isolation Session lifecycle error: {}", + failure.describe() + ) + } + Self::Lifecycle(LifecycleFailure::Internal(msg)) => { + write!(f, "Isolation Session lifecycle error: {}", msg) + } + Self::Stale(failure) => { + write!(f, "Isolation Session stale id: {}", failure.describe()) } - Self::Lifecycle(msg) => write!(f, "Isolation Session lifecycle error: {}", msg), - Self::Stale(msg) => write!(f, "Isolation Session stale id: {}", msg), } } } @@ -47,106 +179,189 @@ impl From for ScriptResponse { } } +/// An MXC-side lifecycle failure, with no API operation in flight. pub(super) fn lifecycle_err(msg: impl Into) -> IsolationSessionError { - IsolationSessionError::Lifecycle(msg.into()) + IsolationSessionError::Lifecycle(LifecycleFailure::Internal(msg.into())) } -/// `HRESULT_FROM_WIN32(ERROR_NOT_FOUND)`. Every non-provision lifecycle op -/// (start / exec / stop / deprovision) surfaces this HRESULT when the -/// agent user is unknown to the OS API; we promote it to `Stale` so a -/// deprovisioned `sandbox_id` reads as `MxcError::StaleId` at the dispatch -/// boundary, not a generic backend error. -const ERROR_NOT_FOUND_HRESULT: u32 = 0x80070490; +/// A transport failure: the API call itself could not be completed (the +/// channel dropped, the object is gone, a property could not be read). +/// +/// `step` names the sub-operation within `operation` — the operation stays +/// the lifecycle call in flight so a consumer has a stable value to branch +/// on, while the finer detail rides in the message. +pub(super) fn transport_err( + operation: &str, + step: &str, + err: &windows_core::Error, +) -> IsolationSessionError { + IsolationSessionError::Lifecycle(LifecycleFailure::Api(IsoApiFailure { + operation: operation.to_string(), + code: Some(err.code().0 as u32), + message: format!("{}: {}", step, err.message()), + remediation: None, + })) +} -/// Formats an `IsoSessionError` into a typed `IsolationSessionError`. -/// Promotes `ERROR_NOT_FOUND` to `Stale`. -pub(super) fn format_iso_error(op: &str, err: &IsoSessionError) -> IsolationSessionError { - // Read `Code()` first and propagate its failure honestly: it is the - // classification-critical field (it drives the `Stale` promotion below), - // so fabricating 0 on a getter failure would silently downgrade a stale - // sandbox to a generic lifecycle error. `Message`/`Remediation` are - // cosmetic and stay best-effort. - let code = match err.Code() { - Ok(c) => c.0 as u32, - Err(e) => { - // Code() is gone, but Message() may still carry signal — fold in - // the best-effort text so the failure is diagnosable. - let msg = err.Message().map(|h| h.to_string()).unwrap_or_default(); - return IsolationSessionError::Lifecycle(format!( - "{} failed: {} (could not read HRESULT code: {})", - op, msg, e - )); - } - }; - let msg = err.Message().map(|h| h.to_string()).unwrap_or_default(); - let remediation = err.Remediation().map(|h| h.to_string()).unwrap_or_default(); - let suffix = if remediation.is_empty() { - String::new() - } else { - format!(" -- remediation: {}", remediation) - }; - let formatted = format!("{} failed: {} (HRESULT: {:#010x}){}", op, msg, code, suffix); - if code == ERROR_NOT_FOUND_HRESULT { - IsolationSessionError::Stale(formatted) +/// Maps an activation failure of the in-proc IsolationSession runtime API to +/// `ServiceUnavailable`. +/// +/// Pure over the HRESULT: activation itself depends on whether the host +/// supports isolation sessions, but this mapping does not, so it stays +/// testable on any machine. +pub(super) fn activation_error(code: u32, detail: &str) -> IsolationSessionError { + let message = + if code == CLASS_E_CLASSNOTAVAILABLE_HRESULT || code == REGDB_E_CLASSNOTREG_HRESULT { + "the in-proc IsolationSession runtime API is not available on this OS build. Ensure \ + the OS feature gate is enabled and the platform supports isolation sessions." + .to_string() + } else { + format!("IsolationSession runtime API activation failed: {detail}") + }; + IsolationSessionError::ServiceUnavailable(IsoApiFailure { + operation: op::ACTIVATE.to_string(), + code: Some(code), + message, + remediation: None, + }) +} + +/// Whether an `ERROR_NOT_FOUND` from this operation means "the sandbox is +/// gone". +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum StalePromotion { + /// Non-provision operations address an existing agent user, so + /// `ERROR_NOT_FOUND` means that user is gone. + Eligible, + /// Provision *mints* the agent user. There is no `sandboxId` yet, so + /// reporting `stale_id` — whose remediation is "re-provision; treat the + /// id as dead" — would be incoherent. + NotEligible, +} + +/// Classifies an API failure whose components have already been extracted. +/// +/// Pure, and deliberately separate from [`format_iso_error`]: `IsoSessionError` +/// is a WinRT interface obtained by activation and cannot be constructed in a +/// unit test, so the classification rules would otherwise be untestable. +/// +/// The `ERROR_NOT_FOUND` promotion is **semantic-path only** by design. The +/// in-proc client maps its internal codes to standard HRESULTs when it builds +/// the `IsoSessionError`, and that mapping is what gives `0x80070490` the +/// meaning "agent user not provisioned". A transport-path HRESULT of the same +/// value has no such provenance — it could be any "not found" from activation +/// or RPC — so promoting it would emit a false `stale_id` and tell the caller +/// to destroy a healthy sandbox. Do not "fix" the asymmetry. +pub(super) fn classify_api_failure( + failure: IsoApiFailure, + promotion: StalePromotion, +) -> IsolationSessionError { + if failure.code == Some(ERROR_NOT_FOUND_HRESULT) && promotion == StalePromotion::Eligible { + IsolationSessionError::Stale(failure) } else { - IsolationSessionError::Lifecycle(formatted) + IsolationSessionError::Lifecycle(LifecycleFailure::Api(failure)) + } +} + +/// Reads an `IsoSessionError`'s components and classifies them. +/// +/// Thin by design — the rules live in [`classify_api_failure`]; this only +/// crosses the WinRT boundary. +pub(super) fn format_iso_error( + operation: &str, + err: &IsoSessionError, + promotion: StalePromotion, +) -> IsolationSessionError { + let message = err.Message().map(|h| h.to_string()).unwrap_or_default(); + let remediation = err + .Remediation() + .map(|h| h.to_string()) + .ok() + .filter(|r| !r.is_empty()); + + // `Code()` is the classification-critical field: it drives the `Stale` + // promotion, so fabricating 0 when the getter fails would silently + // downgrade a stale sandbox to a generic lifecycle error. Report the + // read failure instead and leave the code unknown, which also keeps + // `nativeCode` off the wire rather than carrying the getter's own + // HRESULT — that would describe reading the field, not the operation. + match err.Code() { + Ok(code) => classify_api_failure( + IsoApiFailure { + operation: operation.to_string(), + code: Some(code.0 as u32), + message, + remediation, + }, + promotion, + ), + Err(read_err) => { + let note = format!("could not read HRESULT code: {read_err}"); + IsolationSessionError::Lifecycle(LifecycleFailure::Api(IsoApiFailure { + operation: operation.to_string(), + code: None, + message: if message.is_empty() { + note + } else { + format!("{message} ({note})") + }, + remediation, + })) + } } } /// Checks the `Error` property of an `IsoSessionResult`. `Ok(())` on no -/// error; lifecycle (or stale) error with formatted details otherwise. +/// error; lifecycle (or stale) error with structured details otherwise. pub(super) fn check_result( result: &IsoSessionResult, - op: &str, + operation: &str, + promotion: StalePromotion, ) -> Result<(), IsolationSessionError> { let err = result .Error() - .map_err(|e| lifecycle_err(format!("{}: get Error failed: {}", op, e)))?; + .map_err(|e| transport_err(operation, "get Error failed", &e))?; let is_error = err .IsError() - .map_err(|e| lifecycle_err(format!("{}: get IsError failed: {}", op, e)))?; + .map_err(|e| transport_err(operation, "get IsError failed", &e))?; if is_error { - Err(format_iso_error(op, &err)) + Err(format_iso_error(operation, &err, promotion)) } else { Ok(()) } } pub(super) fn map_lifecycle_error(err: IsolationSessionError) -> MxcError { - let message = err.to_string(); match err { - IsolationSessionError::Policy(_) => MxcError::policy_validation(message), - IsolationSessionError::ServiceUnavailable(_) => MxcError::backend_unavailable(message), - IsolationSessionError::Lifecycle(_) => MxcError::backend_error(message), - IsolationSessionError::Stale(_) => MxcError::stale_id(message), + IsolationSessionError::Policy(msg) => MxcError::policy_validation(msg), + IsolationSessionError::ServiceUnavailable(failure) => { + failure.into_mxc_error(MxcErrorCode::BackendUnavailable) + } + IsolationSessionError::Lifecycle(LifecycleFailure::Internal(msg)) => { + MxcError::backend_error(msg) + } + IsolationSessionError::Lifecycle(LifecycleFailure::Api(failure)) => { + failure.into_mxc_error(MxcErrorCode::BackendError) + } + IsolationSessionError::Stale(failure) => failure.into_mxc_error(MxcErrorCode::StaleId), } } #[cfg(test)] mod tests { use super::*; - use wxc_common::mxc_error::MxcErrorCode; - #[test] - fn map_lifecycle_error_categorises_each_variant() { - assert_eq!( - map_lifecycle_error(IsolationSessionError::Policy("x".into())).code, - MxcErrorCode::PolicyValidation, - ); - assert_eq!( - map_lifecycle_error(IsolationSessionError::ServiceUnavailable("x".into())).code, - MxcErrorCode::BackendUnavailable, - ); - assert_eq!( - map_lifecycle_error(IsolationSessionError::Lifecycle("x".into())).code, - MxcErrorCode::BackendError, - ); - assert_eq!( - map_lifecycle_error(IsolationSessionError::Stale("x".into())).code, - MxcErrorCode::StaleId, - ); + fn api_failure(code: Option) -> IsoApiFailure { + IsoApiFailure { + operation: op::STOP_SESSION.to_string(), + code, + message: "agent user not found".to_string(), + remediation: Some("Re-provision the sandbox.".to_string()), + } } + // ── Constants pinned to the OS values they mirror ──────────────────── + #[test] fn error_not_found_hresult_constant_matches_win32() { // HRESULT_FROM_WIN32(ERROR_NOT_FOUND) = 0x80070000 | (1168 & 0xFFFF) @@ -157,4 +372,266 @@ mod tests { assert_eq!(ERROR_NOT_FOUND_HRESULT, expected); assert_eq!(ERROR_NOT_FOUND_HRESULT, 0x80070490); } + + #[test] + fn activation_hresult_constants_match_win32() { + use windows::Win32::Foundation::{CLASS_E_CLASSNOTAVAILABLE, REGDB_E_CLASSNOTREG}; + assert_eq!( + CLASS_E_CLASSNOTAVAILABLE_HRESULT, + CLASS_E_CLASSNOTAVAILABLE.0 as u32 + ); + assert_eq!(REGDB_E_CLASSNOTREG_HRESULT, REGDB_E_CLASSNOTREG.0 as u32); + } + + // ── nativeCode rendering ───────────────────────────────────────────── + + #[test] + fn native_code_renders_as_lowercase_hex() { + assert_eq!(format_native_code(0x80070490), "0x80070490"); + assert_eq!(format_native_code(0x8004005a), "0x8004005a"); + } + + // ── Stale promotion ────────────────────────────────────────────────── + + #[test] + fn error_not_found_promotes_to_stale_for_non_provision_ops() { + let err = classify_api_failure( + api_failure(Some(ERROR_NOT_FOUND_HRESULT)), + StalePromotion::Eligible, + ); + assert!(matches!(err, IsolationSessionError::Stale(_))); + assert_eq!(map_lifecycle_error(err).code, MxcErrorCode::StaleId); + } + + /// Provision mints the agent user, so it has no sandbox id to be stale. + #[test] + fn error_not_found_does_not_promote_for_provision() { + let mut failure = api_failure(Some(ERROR_NOT_FOUND_HRESULT)); + failure.operation = op::ADD_USER.to_string(); + let err = classify_api_failure(failure, StalePromotion::NotEligible); + assert!(matches!( + err, + IsolationSessionError::Lifecycle(LifecycleFailure::Api(_)) + )); + assert_eq!(map_lifecycle_error(err).code, MxcErrorCode::BackendError); + } + + #[test] + fn other_hresults_do_not_promote_to_stale() { + let err = classify_api_failure(api_failure(Some(0x80004005)), StalePromotion::Eligible); + assert_eq!(map_lifecycle_error(err).code, MxcErrorCode::BackendError); + } + + /// A transport-path `ERROR_NOT_FOUND` has none of the provenance that + /// gives the semantic one its meaning, so it must stay `backend_error`. + #[test] + fn transport_error_not_found_does_not_promote_to_stale() { + let err = windows_core::Error::from_hresult(windows_core::HRESULT( + ERROR_NOT_FOUND_HRESULT as i32, + )); + let mapped = map_lifecycle_error(transport_err(op::STOP_SESSION, "call failed", &err)); + assert_eq!(mapped.code, MxcErrorCode::BackendError); + assert_eq!(mapped.native_code(), Some("0x80070490")); + } + + // ── Field population and the MxcError invariant ────────────────────── + + #[test] + fn semantic_failure_populates_all_structured_fields() { + let err = classify_api_failure( + api_failure(Some(ERROR_NOT_FOUND_HRESULT)), + StalePromotion::Eligible, + ); + let mapped = map_lifecycle_error(err); + assert_eq!(mapped.code, MxcErrorCode::StaleId); + assert_eq!(mapped.message, "agent user not found"); + assert_eq!(mapped.operation(), Some("IsoSessionOps.StopSessionAsync")); + assert_eq!(mapped.native_code(), Some("0x80070490")); + assert_eq!(mapped.remediation(), Some("Re-provision the sandbox.")); + } + + #[test] + fn transport_failure_populates_operation_and_native_code_only() { + let err = windows_core::Error::from_hresult(windows_core::HRESULT(0x800706ba_u32 as i32)); + let mapped = map_lifecycle_error(transport_err(op::ADD_USER, "call failed", &err)); + assert_eq!(mapped.code, MxcErrorCode::BackendError); + assert_eq!(mapped.operation(), Some("IsoSessionOps.AddUserAsync")); + assert_eq!(mapped.native_code(), Some("0x800706ba")); + assert_eq!(mapped.remediation(), None); + assert!(mapped.message.starts_with("call failed: ")); + } + + #[test] + fn activation_failure_populates_operation_and_native_code() { + let mapped = map_lifecycle_error(activation_error( + CLASS_E_CLASSNOTAVAILABLE_HRESULT, + "unused", + )); + assert_eq!(mapped.code, MxcErrorCode::BackendUnavailable); + assert_eq!(mapped.operation(), Some("IsoSessionOps.ActivateInstance")); + assert_eq!(mapped.native_code(), Some("0x80040111")); + assert!(mapped.message.contains("not available")); + } + + #[test] + fn unknown_activation_hresult_keeps_the_underlying_detail() { + let mapped = map_lifecycle_error(activation_error(0x80004005, "catastrophic failure")); + assert_eq!(mapped.code, MxcErrorCode::BackendUnavailable); + assert_eq!(mapped.native_code(), Some("0x80004005")); + assert!(mapped.message.contains("catastrophic failure")); + } + + #[test] + fn mxc_internal_failure_carries_no_structured_fields() { + let mapped = map_lifecycle_error(lifecycle_err("create stdout relay: out of memory")); + assert_eq!(mapped.code, MxcErrorCode::BackendError); + assert_eq!(mapped.operation(), None); + assert_eq!(mapped.native_code(), None); + assert_eq!(mapped.remediation(), None); + } + + #[test] + fn policy_failure_carries_no_structured_fields() { + let mapped = map_lifecycle_error(IsolationSessionError::Policy("no proxy".into())); + assert_eq!(mapped.code, MxcErrorCode::PolicyValidation); + assert_eq!(mapped.operation(), None); + assert_eq!(mapped.native_code(), None); + assert_eq!(mapped.remediation(), None); + } + + /// `nativeCode` implies `operation`, and `remediation` implies + /// `operation`. Neither may ever appear alone. + #[test] + fn every_variant_upholds_the_field_invariant() { + let com = windows_core::Error::from_hresult(windows_core::HRESULT(0x80004005_u32 as i32)); + let cases = vec![ + IsolationSessionError::Policy("x".into()), + lifecycle_err("internal"), + transport_err(op::RUN_PROCESS, "call failed", &com), + activation_error(REGDB_E_CLASSNOTREG_HRESULT, "x"), + classify_api_failure(api_failure(Some(0x80004005)), StalePromotion::Eligible), + classify_api_failure( + api_failure(Some(ERROR_NOT_FOUND_HRESULT)), + StalePromotion::Eligible, + ), + classify_api_failure(api_failure(None), StalePromotion::Eligible), + ]; + for case in cases { + let label = case.to_string(); + let mapped = map_lifecycle_error(case); + if mapped.native_code().is_some() { + assert!( + mapped.operation().is_some(), + "nativeCode without operation: {label}" + ); + } + if mapped.remediation().is_some() { + assert!( + mapped.operation().is_some(), + "remediation without operation: {label}" + ); + } + } + } + + #[test] + fn unreadable_hresult_yields_operation_without_native_code() { + let mapped = map_lifecycle_error(classify_api_failure( + api_failure(None), + StalePromotion::Eligible, + )); + assert_eq!(mapped.code, MxcErrorCode::BackendError); + assert!(mapped.operation().is_some()); + assert_eq!(mapped.native_code(), None); + } + + // ── One-shot rendering (Display) ───────────────────────────────────── + + /// The one-shot path has no structured envelope, so `Display` must keep + /// folding every component back into the message — including the + /// category prefix, which is the only place the category appears there. + #[test] + fn display_composes_the_full_human_string() { + let rendered = classify_api_failure( + api_failure(Some(ERROR_NOT_FOUND_HRESULT)), + StalePromotion::Eligible, + ) + .to_string(); + assert!( + rendered.starts_with("Isolation Session stale id: "), + "{rendered}" + ); + assert!( + rendered.contains("IsoSessionOps.StopSessionAsync"), + "{rendered}" + ); + assert!(rendered.contains("agent user not found"), "{rendered}"); + assert!(rendered.contains("0x80070490"), "{rendered}"); + assert!( + rendered.contains("remediation: Re-provision the sandbox."), + "{rendered}" + ); + } + + #[test] + fn display_keeps_the_category_prefix_for_every_variant() { + let com = windows_core::Error::from_hresult(windows_core::HRESULT(0x80004005_u32 as i32)); + assert!(IsolationSessionError::Policy("x".into()) + .to_string() + .starts_with("Isolation Session policy error: ")); + assert!(activation_error(REGDB_E_CLASSNOTREG_HRESULT, "x") + .to_string() + .starts_with("Isolation Session service unavailable: ")); + assert!(lifecycle_err("x") + .to_string() + .starts_with("Isolation Session lifecycle error: ")); + assert!(transport_err(op::ADD_USER, "call failed", &com) + .to_string() + .starts_with("Isolation Session lifecycle error: ")); + } + + /// The one-shot conversion still carries the full composed detail — it + /// is the only carrier on that path. + #[test] + fn script_response_conversion_keeps_the_rich_message() { + let response: ScriptResponse = + classify_api_failure(api_failure(Some(0x80004005)), StalePromotion::Eligible).into(); + assert!(response + .error_message + .contains("IsoSessionOps.StopSessionAsync")); + assert!(response.error_message.contains("0x80004005")); + assert!(response.error_message.contains("remediation")); + } + + // ── operation values ───────────────────────────────────────────────── + + /// `operation` must stay interface-qualified and free of call + /// parameters, so it can be aggregated in telemetry. + #[test] + fn operation_constants_are_qualified_and_parameter_free() { + for value in [ + op::ACTIVATE, + op::ADD_USER, + op::START_SESSION, + op::RUN_PROCESS, + op::STOP_SESSION, + op::REMOVE_USER, + op::OPTIONS_NEW, + op::OPTIONS_TIMEOUT, + op::OPTIONS_WORKING_DIR, + op::OPTIONS_INTERACTIVE, + op::OPTIONS_REDIRECT_STDIN, + op::OPTIONS_REDIRECT_STDOUT, + op::OPTIONS_REDIRECT_STDERR, + op::OPTIONS_ENVIRONMENT, + ] { + assert!( + value.starts_with("IsoSessionOps.") + || value.starts_with("IsoSessionProcessOptions."), + "unqualified: {value}" + ); + assert!(!value.contains('('), "carries parameters: {value}"); + assert!(!value.contains(' '), "not a bare member name: {value}"); + } + } } diff --git a/src/backends/isolation_session/common/src/manager.rs b/src/backends/isolation_session/common/src/manager.rs index d11a5952f..7e0b44b29 100644 --- a/src/backends/isolation_session/common/src/manager.rs +++ b/src/backends/isolation_session/common/src/manager.rs @@ -11,7 +11,7 @@ use wxc_common::process_util::OwnedHandle; use isolation_session_bindings::bindings::{ IsoSessionOps, IsoSessionProcess, IsoSessionProcessResult, IsoSessionUserResult, }; -use windows::Win32::Foundation::{CLASS_E_CLASSNOTAVAILABLE, HANDLE, REGDB_E_CLASSNOTREG}; +use windows::Win32::Foundation::HANDLE; use windows::Win32::System::Console::{ GetStdHandle, STD_ERROR_HANDLE, STD_INPUT_HANDLE, STD_OUTPUT_HANDLE, }; @@ -20,32 +20,24 @@ use windows_core::{HSTRING, PCWSTR}; use super::console_mode::{get_local_console_size, ConsoleModeRestorer, CtrlHandlerGuard}; use super::console_relay::{create_console_relay_thread, ConsoleRelayParams}; -use super::error::{check_result, format_iso_error, lifecycle_err, IsolationSessionError}; +use super::error::{ + activation_error, check_result, format_iso_error, lifecycle_err, op, transport_err, + IsolationSessionError, StalePromotion, +}; use super::pipe_relay::{ create_relay_thread, create_relay_thread_with_stop, PipeRelayParams, PipeRelayWithStopParams, }; use super::process_options::{build_iso_process_options, ProcessOptions}; -/// Activates the in-proc `IsoSessionOps` factory and returns the instance. +/// Activates the in-proc IsolationSession runtime factory and returns the +/// instance. fn check_service_available_and_activate() -> Result { match IsoSessionOps::new() { Ok(ops) => Ok(ops), - Err(e) => { - let code = e.code(); - if code == CLASS_E_CLASSNOTAVAILABLE || code == REGDB_E_CLASSNOTREG { - Err(IsolationSessionError::ServiceUnavailable(format!( - "in-proc Windows.AI.IsolationSession.Preview IsoSessionOps API is not \ - available on this OS build (HRESULT: {:#010x}). Ensure the OS feature \ - gate is enabled and the platform supports isolation sessions.", - code.0 as u32 - ))) - } else { - Err(IsolationSessionError::ServiceUnavailable(format!( - "IsoSessionOps activation failed (HRESULT: {:#010x}): {}", - code.0 as u32, e - ))) - } - } + // The HRESULT→error mapping lives in `activation_error` so it stays + // testable without depending on whether this host can activate the + // API at all. + Err(e) => Err(activation_error(e.code().0 as u32, &e.message())), } } @@ -117,33 +109,36 @@ impl IsolationSessionManager { &HSTRING::from(opt_entra_account_name), &HSTRING::from(opt_wam_token), ) - .map_err(|e| lifecycle_err(format!("AddUserAsync call failed: {}", e)))?; + .map_err(|e| transport_err(op::ADD_USER, "call failed", &e))?; let user_result: IsoSessionUserResult = async_op .join() - .map_err(|e| lifecycle_err(format!("AddUserAsync wait failed: {}", e)))?; + .map_err(|e| transport_err(op::ADD_USER, "wait failed", &e))?; let err = user_result .Error() - .map_err(|e| lifecycle_err(format!("AddUserAsync: get Error failed: {}", e)))?; + .map_err(|e| transport_err(op::ADD_USER, "get Error failed", &e))?; let is_error = err .IsError() - .map_err(|e| lifecycle_err(format!("AddUserAsync: get IsError failed: {}", e)))?; + .map_err(|e| transport_err(op::ADD_USER, "get IsError failed", &e))?; if is_error { - return Err(format_iso_error("AddUserAsync", &err)); + // Provision mints the agent user, so `ERROR_NOT_FOUND` here can + // never mean "the sandbox is gone" — there is no sandbox id yet. + return Err(format_iso_error( + op::ADD_USER, + &err, + StalePromotion::NotEligible, + )); } let agent_user_name = user_result .AgentUserName() - .map_err(|e| lifecycle_err(format!("AddUserAsync: get AgentUserName failed: {}", e)))?; + .map_err(|e| transport_err(op::ADD_USER, "get AgentUserName failed", &e))?; let agent_user_sid = user_result .AgentUserSid() - .map_err(|e| lifecycle_err(format!("AddUserAsync: get AgentUserSid failed: {}", e)))?; - let ephemeral_workspace_path = user_result.EphemeralWorkspacePath().map_err(|e| { - lifecycle_err(format!( - "AddUserAsync: get EphemeralWorkspacePath failed: {}", - e - )) - })?; + .map_err(|e| transport_err(op::ADD_USER, "get AgentUserSid failed", &e))?; + let ephemeral_workspace_path = user_result + .EphemeralWorkspacePath() + .map_err(|e| transport_err(op::ADD_USER, "get EphemeralWorkspacePath failed", &e))?; Ok(ProvisionedUser { agent_user_name: agent_user_name.to_string(), @@ -160,11 +155,11 @@ impl IsolationSessionManager { let async_op = self .ops .StartSessionAsync(&self.agent_user_name, &HSTRING::from(opt_wam_token)) - .map_err(|e| lifecycle_err(format!("StartSessionAsync call failed: {}", e)))?; + .map_err(|e| transport_err(op::START_SESSION, "call failed", &e))?; let result = async_op .join() - .map_err(|e| lifecycle_err(format!("StartSessionAsync wait failed: {}", e)))?; - check_result(&result, "StartSessionAsync") + .map_err(|e| transport_err(op::START_SESSION, "wait failed", &e))?; + check_result(&result, op::START_SESSION, StalePromotion::Eligible) } /// Step 3: Create a process inside the started isolation session. @@ -184,33 +179,28 @@ impl IsolationSessionManager { &HSTRING::from(&options.arguments), &proc_options, ) - .map_err(|e| lifecycle_err(format!("RunProcessWithOptionsAsync call failed: {}", e)))?; + .map_err(|e| transport_err(op::RUN_PROCESS, "call failed", &e))?; let result: IsoSessionProcessResult = async_op .join() - .map_err(|e| lifecycle_err(format!("RunProcessWithOptionsAsync wait failed: {}", e)))?; - - let err = result.Error().map_err(|e| { - lifecycle_err(format!( - "RunProcessWithOptionsAsync: get Error failed: {}", - e - )) - })?; - let is_error = err.IsError().map_err(|e| { - lifecycle_err(format!( - "RunProcessWithOptionsAsync: get IsError failed: {}", - e - )) - })?; + .map_err(|e| transport_err(op::RUN_PROCESS, "wait failed", &e))?; + + let err = result + .Error() + .map_err(|e| transport_err(op::RUN_PROCESS, "get Error failed", &e))?; + let is_error = err + .IsError() + .map_err(|e| transport_err(op::RUN_PROCESS, "get IsError failed", &e))?; if is_error { - return Err(format_iso_error("RunProcessWithOptionsAsync", &err)); + return Err(format_iso_error( + op::RUN_PROCESS, + &err, + StalePromotion::Eligible, + )); } - let process: IsoSessionProcess = result.Process().map_err(|e| { - lifecycle_err(format!( - "RunProcessWithOptionsAsync: get Process failed: {}", - e - )) - })?; + let process: IsoSessionProcess = result + .Process() + .map_err(|e| transport_err(op::RUN_PROCESS, "get Process failed", &e))?; // Three pipe relay threads bridge wxc-exec's stdio with the pipe // handles owned by `IsoSessionProcess`, crossing the desktop-session @@ -234,24 +224,15 @@ impl IsolationSessionManager { // propagate it rather than coercing to 0, which downstream treats as // "no handle" and silently skips the corresponding stdio relay. A // genuinely returned 0 still means absent and is preserved. - let stdout_handle_val = process.OutputHandle().map_err(|e| { - lifecycle_err(format!( - "RunProcessWithOptionsAsync: get OutputHandle failed: {}", - e - )) - })?; - let stderr_handle_val = process.ErrorHandle().map_err(|e| { - lifecycle_err(format!( - "RunProcessWithOptionsAsync: get ErrorHandle failed: {}", - e - )) - })?; - let stdin_handle_val = process.InputHandle().map_err(|e| { - lifecycle_err(format!( - "RunProcessWithOptionsAsync: get InputHandle failed: {}", - e - )) - })?; + let stdout_handle_val = process + .OutputHandle() + .map_err(|e| transport_err(op::RUN_PROCESS, "get OutputHandle failed", &e))?; + let stderr_handle_val = process + .ErrorHandle() + .map_err(|e| transport_err(op::RUN_PROCESS, "get ErrorHandle failed", &e))?; + let stdin_handle_val = process + .InputHandle() + .map_err(|e| transport_err(op::RUN_PROCESS, "get InputHandle failed", &e))?; let wxc_stdout = unsafe { GetStdHandle(STD_OUTPUT_HANDLE) } .map_err(|e| lifecycle_err(format!("GetStdHandle(stdout) failed: {}", e)))?; @@ -392,7 +373,7 @@ impl IsolationSessionManager { // code. let _ = process .WaitForExit(options.timeout_ms) - .map_err(|e| lifecycle_err(format!("WaitForExit failed: {}", e)))?; + .map_err(|e| transport_err(op::RUN_PROCESS, "WaitForExit failed", &e))?; let exit_code = wait_with_graceful_shutdown(&process)?; @@ -432,11 +413,11 @@ impl IsolationSessionManager { let async_op = self .ops .StopSessionAsync(&self.agent_user_name) - .map_err(|e| lifecycle_err(format!("StopSessionAsync call failed: {}", e)))?; + .map_err(|e| transport_err(op::STOP_SESSION, "call failed", &e))?; let result = async_op .join() - .map_err(|e| lifecycle_err(format!("StopSessionAsync wait failed: {}", e)))?; - check_result(&result, "StopSessionAsync") + .map_err(|e| transport_err(op::STOP_SESSION, "wait failed", &e))?; + check_result(&result, op::STOP_SESSION, StalePromotion::Eligible) } /// Step 5: Deprovision the agent user. @@ -444,11 +425,11 @@ impl IsolationSessionManager { let async_op = self .ops .RemoveUserAsync(&self.agent_user_name) - .map_err(|e| lifecycle_err(format!("RemoveUserAsync call failed: {}", e)))?; + .map_err(|e| transport_err(op::REMOVE_USER, "call failed", &e))?; let result = async_op .join() - .map_err(|e| lifecycle_err(format!("RemoveUserAsync wait failed: {}", e)))?; - check_result(&result, "RemoveUserAsync") + .map_err(|e| transport_err(op::REMOVE_USER, "wait failed", &e))?; + check_result(&result, op::REMOVE_USER, StalePromotion::Eligible) } } @@ -471,7 +452,7 @@ fn wait_with_graceful_shutdown(process: &IsoSessionProcess) -> Result { + Err(IsolationSessionError::ServiceUnavailable(failure)) => { // Service is NOT available. Verify the error is clean and - // descriptive (not a panic or cryptic COM error). + // descriptive (not a panic or cryptic COM error), and that + // it names the activation operation it failed on. assert!( - msg.contains("not available") || msg.contains("activation failed"), + failure.message.contains("not available") + || failure.message.contains("activation failed"), "Expected descriptive error message, got: {}", - msg + failure.message + ); + assert_eq!(failure.operation, op::ACTIVATE); + assert!( + failure.code.is_some(), + "activation failure carries no HRESULT" ); } Err(other) => { diff --git a/src/backends/isolation_session/common/src/process_options.rs b/src/backends/isolation_session/common/src/process_options.rs index 399bb20cf..84fc4ada2 100644 --- a/src/backends/isolation_session/common/src/process_options.rs +++ b/src/backends/isolation_session/common/src/process_options.rs @@ -10,7 +10,7 @@ use wxc_common::models::ExecutionRequest; use isolation_session_bindings::bindings::IsoSessionProcessOptions; use windows_core::HSTRING; -use super::error::{lifecycle_err, IsolationSessionError}; +use super::error::{op, transport_err, IsolationSessionError}; const REDIRECT_STDIN: u32 = 0x1; const REDIRECT_STDOUT: u32 = 0x2; @@ -96,39 +96,47 @@ pub(super) fn build_iso_process_options( options: &ProcessOptions, ) -> Result { let proc_options = IsoSessionProcessOptions::new() - .map_err(|e| lifecycle_err(format!("IsoSessionProcessOptions::new failed: {}", e)))?; + .map_err(|e| transport_err(op::OPTIONS_NEW, "activation failed", &e))?; proc_options .SetTimeoutMilliseconds(options.timeout_ms) - .map_err(|e| lifecycle_err(format!("SetTimeoutMilliseconds: {}", e)))?; + .map_err(|e| transport_err(op::OPTIONS_TIMEOUT, "set failed", &e))?; if !options.working_directory.is_empty() { proc_options .SetWorkingDirectory(&HSTRING::from(&options.working_directory)) - .map_err(|e| lifecycle_err(format!("SetWorkingDirectory: {}", e)))?; + .map_err(|e| transport_err(op::OPTIONS_WORKING_DIR, "set failed", &e))?; } proc_options .SetInteractiveConsole(options.interactive) - .map_err(|e| lifecycle_err(format!("SetInteractiveConsole: {}", e)))?; + .map_err(|e| transport_err(op::OPTIONS_INTERACTIVE, "set failed", &e))?; proc_options .SetRedirectStandardInput(options.redirect_flags & REDIRECT_STDIN != 0) - .map_err(|e| lifecycle_err(format!("SetRedirectStandardInput: {}", e)))?; + .map_err(|e| transport_err(op::OPTIONS_REDIRECT_STDIN, "set failed", &e))?; proc_options .SetRedirectStandardOutput(options.redirect_flags & REDIRECT_STDOUT != 0) - .map_err(|e| lifecycle_err(format!("SetRedirectStandardOutput: {}", e)))?; + .map_err(|e| transport_err(op::OPTIONS_REDIRECT_STDOUT, "set failed", &e))?; proc_options .SetRedirectStandardError(options.redirect_flags & REDIRECT_STDERR != 0) - .map_err(|e| lifecycle_err(format!("SetRedirectStandardError: {}", e)))?; + .map_err(|e| transport_err(op::OPTIONS_REDIRECT_STDERR, "set failed", &e))?; if !options.env_vars.is_empty() { let env = proc_options .Environment() - .map_err(|e| lifecycle_err(format!("get Environment IMap: {}", e)))?; + .map_err(|e| transport_err(op::OPTIONS_ENVIRONMENT, "get failed", &e))?; for (name, value) in &options.env_vars { + // The variable name rides in the message, never in `operation` — + // that field stays low-cardinality for telemetry grouping. env.Insert(&HSTRING::from(name), &HSTRING::from(value)) - .map_err(|e| lifecycle_err(format!("Environment.Insert({}): {}", name, e)))?; + .map_err(|e| { + transport_err( + op::OPTIONS_ENVIRONMENT, + &format!("insert {name} failed"), + &e, + ) + })?; } } diff --git a/src/core/wxc_common/src/mxc_error.rs b/src/core/wxc_common/src/mxc_error.rs index 9221a2fe8..b29794cef 100644 --- a/src/core/wxc_common/src/mxc_error.rs +++ b/src/core/wxc_common/src/mxc_error.rs @@ -57,17 +57,77 @@ impl std::fmt::Display for MxcErrorCode { } } +/// Structured detail for a failure that originated in an underlying platform +/// API. +/// +/// Grouping these makes the envelope invariant unrepresentable to violate: +/// `native_code` and `remediation` cannot exist without `operation`, because +/// they live inside the same value. `MxcError` holds this boxed, so adding +/// detail costs one pointer rather than widening every `Result<_, MxcError>` +/// in the codebase. +#[derive(Debug, Clone, PartialEq, Eq, Default)] +pub struct ApiFailure { + /// The API call that failed, namespaced by its interface — e.g. + /// `IsoSessionOps.RunProcessWithOptionsAsync`. Kept low-cardinality and + /// free of call parameters so it can be grouped in telemetry. + pub operation: String, + /// The underlying platform status as a string, e.g. `0x80070490`. + pub native_code: Option, + /// The API's actionable "how to fix it" hint, when it supplies one. + pub remediation: Option, +} + +impl ApiFailure { + /// A failure that names its operation but carries no status or hint. + pub fn new(operation: impl Into) -> Self { + Self { + operation: operation.into(), + native_code: None, + remediation: None, + } + } + + pub fn with_native_code(mut self, native_code: impl Into) -> Self { + self.native_code = Some(native_code.into()); + self + } + + pub fn with_remediation(mut self, remediation: impl Into) -> Self { + self.remediation = Some(remediation.into()); + self + } +} + /// Typed Rust equivalent of the SDK `MxcError`. /// /// Constructed via `MxcError::new(code, message)` or one of the per-code /// convenience constructors (e.g. `MxcError::stale_id("...")`); attach -/// structured failure information with `.with_details(json!({...}))`. +/// structured failure information with `with_details` or `with_api_failure`. +/// +/// # Structured failure fields +/// +/// An [`ApiFailure`] describes a failure that originated in an underlying +/// platform API. It is deliberately backend-neutral: `operation` names the +/// API call that failed, `native_code` carries the platform status as a +/// string (an HRESULT on Windows, an errno or equivalent elsewhere), and +/// `remediation` carries an actionable hint when the API supplies one. On the +/// wire these are flat siblings of `code` and `message`. +/// +/// A failure MXC raises itself — a malformed request, a policy rejection, or +/// an internal failure with no API call in flight — leaves it unset and so +/// carries only `code` and `message`. +/// +/// A new *backend-neutral* concept earns a field on `ApiFailure`; +/// *backend-specific* structured data belongs in `details`, which stays open +/// for that purpose. #[derive(Debug, Clone, PartialEq, Eq, Error)] #[error("{code}: {message}")] pub struct MxcError { pub code: MxcErrorCode, pub message: String, pub details: Option, + /// Present only when an underlying API operation was in flight. + pub api_failure: Option>, } impl MxcError { @@ -76,6 +136,7 @@ impl MxcError { code, message: message.into(), details: None, + api_failure: None, } } @@ -84,11 +145,41 @@ impl MxcError { self } + /// Attaches the structured detail of an underlying API failure. + pub fn with_api_failure(mut self, failure: ApiFailure) -> Self { + self.api_failure = Some(Box::new(failure)); + self + } + + /// The API call that failed, when one was in flight. + pub fn operation(&self) -> Option<&str> { + self.api_failure.as_ref().map(|f| f.operation.as_str()) + } + + /// The underlying platform status, when known. Never present without + /// [`MxcError::operation`]. + pub fn native_code(&self) -> Option<&str> { + self.api_failure + .as_ref() + .and_then(|f| f.native_code.as_deref()) + } + + /// The API's remediation hint, when it supplied one. Never present + /// without [`MxcError::operation`]. + pub fn remediation(&self) -> Option<&str> { + self.api_failure + .as_ref() + .and_then(|f| f.remediation.as_deref()) + } + pub fn to_envelope(&self) -> ErrorEnvelope { ErrorEnvelope { code: self.code, message: self.message.clone(), details: self.details.clone(), + operation: self.operation().map(str::to_string), + native_code: self.native_code().map(str::to_string), + remediation: self.remediation().map(str::to_string), } } } @@ -134,14 +225,27 @@ impl MxcError { } /// Wire shape of the `error` arm. `code` is a closed `MxcErrorCode` that -/// serialises to its snake_case wire string; `details` is omitted from JSON -/// when absent. +/// serialises to its snake_case wire string; every optional field is omitted +/// from JSON when absent. +/// +/// See [`MxcError`] for the meaning of the structured failure fields and the +/// invariant relating them. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct ErrorEnvelope { pub code: MxcErrorCode, pub message: String, #[serde(skip_serializing_if = "Option::is_none", default)] pub details: Option, + #[serde(skip_serializing_if = "Option::is_none", default)] + pub operation: Option, + #[serde( + rename = "nativeCode", + skip_serializing_if = "Option::is_none", + default + )] + pub native_code: Option, + #[serde(skip_serializing_if = "Option::is_none", default)] + pub remediation: Option, } /// Top-level non-exec response envelope: `{"result": }` on success, or @@ -269,6 +373,9 @@ mod tests { code: MxcErrorCode::StaleId, message: "session expired".into(), details: Some(json!({"k": "v"})), + operation: Some("IsoSessionOps.StopSessionAsync".into()), + native_code: Some("0x80070490".into()), + remediation: Some("Re-provision the sandbox.".into()), }; let s = serde_json::to_string(&env).unwrap(); let back: ErrorEnvelope = serde_json::from_str(&s).unwrap(); @@ -281,6 +388,9 @@ mod tests { code: MxcErrorCode::StaleId, message: "x".into(), details: None, + operation: None, + native_code: None, + remediation: None, }; let s = serde_json::to_string(&env).unwrap(); assert!(!s.contains("details")); @@ -299,6 +409,9 @@ mod tests { code: MxcErrorCode::StaleId, message: "x".into(), details: None, + operation: None, + native_code: None, + remediation: None, }; let env: ResponseEnvelope<()> = ResponseEnvelope::Error(inner.clone()); let json = serde_json::to_value(&env).unwrap(); @@ -311,6 +424,9 @@ mod tests { code: MxcErrorCode::BackendError, message: "boom".into(), details: Some(json!({"x": 1})), + operation: Some("IsoSessionOps.AddUserAsync".into()), + native_code: Some("0x80004005".into()), + remediation: None, }; let env: ResponseEnvelope<()> = ResponseEnvelope::Error(inner); let s = serde_json::to_string(&env).unwrap(); @@ -334,4 +450,119 @@ mod tests { }) ); } + + // ── Structured failure fields ──────────────────────────────────────── + + fn full_api_failure() -> ApiFailure { + ApiFailure::new("IsoSessionOps.AddUserAsync") + .with_native_code("0x80070490") + .with_remediation("Re-provision the sandbox.") + } + + #[test] + fn new_leaves_structured_fields_unset() { + let err = MxcError::backend_error("boom"); + assert_eq!(err.operation(), None); + assert_eq!(err.native_code(), None); + assert_eq!(err.remediation(), None); + } + + #[test] + fn structured_builders_set_their_fields() { + let err = MxcError::backend_error("boom").with_api_failure(full_api_failure()); + assert_eq!(err.operation(), Some("IsoSessionOps.AddUserAsync")); + assert_eq!(err.native_code(), Some("0x80070490")); + assert_eq!(err.remediation(), Some("Re-provision the sandbox.")); + } + + /// `native_code` and `remediation` live inside `ApiFailure`, so they + /// cannot be set without an `operation` — the envelope invariant holds + /// by construction rather than by convention. + #[test] + fn structured_detail_always_carries_an_operation() { + let err = MxcError::backend_error("boom") + .with_api_failure(ApiFailure::new("IsoSessionOps.AddUserAsync")); + assert_eq!(err.operation(), Some("IsoSessionOps.AddUserAsync")); + assert_eq!(err.native_code(), None); + assert_eq!(err.remediation(), None); + } + + #[test] + fn to_envelope_copies_structured_fields_through() { + let env = MxcError::backend_error("boom") + .with_api_failure(full_api_failure()) + .to_envelope(); + assert_eq!(env.operation.as_deref(), Some("IsoSessionOps.AddUserAsync")); + assert_eq!(env.native_code.as_deref(), Some("0x80070490")); + assert_eq!( + env.remediation.as_deref(), + Some("Re-provision the sandbox.") + ); + } + + /// The wire key is camelCase `nativeCode`, not the Rust field name + /// `native_code`. The SDK reads `nativeCode`; a lost serde rename would + /// silently strip the field from every consumer. + #[test] + fn native_code_serialises_as_camel_case_key() { + let env = MxcError::backend_error("boom") + .with_api_failure( + ApiFailure::new("IsoSessionOps.AddUserAsync").with_native_code("0x80070490"), + ) + .to_envelope(); + let s = serde_json::to_string(&env).unwrap(); + assert!( + s.contains("\"nativeCode\""), + "expected camelCase key in {s}" + ); + assert!(!s.contains("native_code"), "found snake_case key in {s}"); + } + + #[test] + fn structured_envelope_serialises_all_fields() { + let env = MxcError::stale_id("agent user not found") + .with_api_failure( + ApiFailure::new("IsoSessionOps.StopSessionAsync") + .with_native_code("0x80070490") + .with_remediation("Re-provision the sandbox."), + ) + .to_envelope(); + let json = serde_json::to_value(&env).unwrap(); + assert_eq!( + json, + json!({ + "code": "stale_id", + "message": "agent user not found", + "operation": "IsoSessionOps.StopSessionAsync", + "nativeCode": "0x80070490", + "remediation": "Re-provision the sandbox.", + }) + ); + } + + #[test] + fn envelope_omits_each_structured_field_when_unset() { + // Only `operation` set: the other two must not appear at all. + let env = MxcError::backend_error("boom") + .with_api_failure(ApiFailure::new("IsoSessionOps.AddUserAsync")) + .to_envelope(); + let s = serde_json::to_string(&env).unwrap(); + assert!(s.contains("\"operation\"")); + assert!( + !s.contains("nativeCode"), + "unset nativeCode leaked into {s}" + ); + assert!( + !s.contains("remediation"), + "unset remediation leaked into {s}" + ); + } + + /// An MXC-side rejection has no API call in flight, so it carries neither + /// `operation` nor its refinements — see the invariant on `MxcError`. + #[test] + fn mxc_side_rejection_carries_no_structured_fields() { + let json = serde_json::to_value(MxcError::policy_validation("bad").to_envelope()).unwrap(); + assert_eq!(json, json!({"code": "policy_validation", "message": "bad"})); + } } diff --git a/tests/scripts/run_isolation_session_state_aware_tests.ps1 b/tests/scripts/run_isolation_session_state_aware_tests.ps1 index ab291aad3..2465c0b6e 100644 --- a/tests/scripts/run_isolation_session_state_aware_tests.ps1 +++ b/tests/scripts/run_isolation_session_state_aware_tests.ps1 @@ -625,6 +625,11 @@ try { # `MxcError::StaleId` (wire `error.code = "stale_id"`), proving the # Rust-layer ERROR_NOT_FOUND HRESULT detection is wired through the # backend impl all the way to the wire envelope. + # + # This is also the one place the structured failure fields are asserted + # end-to-end against the live API: the OS-side mapping of "agent user not + # provisioned" to HRESULT_FROM_WIN32(ERROR_NOT_FOUND) is a documented + # cross-repo contract, so `nativeCode` has a stable expected value here. if ($deprovisionedOk) { Run-StateAwareTest "stale_id (stop on previously-deprovisioned sandbox)" { $r = Invoke-StateAware -ConfigFile 'isolation_session_state_aware_stop.json' -SandboxId $script:sandboxId -Experimental @@ -633,6 +638,23 @@ try { Assert-True ($null -ne $envObj) "stdout is a parseable envelope" $code = if ($envObj) { $envObj.error.code } else { '' } Assert-True ($code -eq 'stale_id') "error.code is 'stale_id' (got '$code')" + + # Structured failure fields: an API failure names the operation + # that failed and carries the underlying HRESULT. + $operation = if ($envObj) { [string]$envObj.error.operation } else { '' } + Assert-True ($operation -eq 'IsoSessionOps.StopSessionAsync') ` + "error.operation is 'IsoSessionOps.StopSessionAsync' (got '$operation')" + $nativeCode = if ($envObj) { [string]$envObj.error.nativeCode } else { '' } + Assert-True ($nativeCode -eq '0x80070490') ` + "error.nativeCode is '0x80070490' (got '$nativeCode')" + + # `message` is the bare API message -- the operation and HRESULT + # live in their own fields and must not be concatenated into it. + $msg = if ($envObj) { [string]$envObj.error.message } else { '' } + Assert-True (-not $msg.Contains('0x80070490')) ` + "error.message does not repeat the HRESULT (got '$msg')" + Assert-True (-not $msg.Contains('IsoSessionOps.')) ` + "error.message does not repeat the operation (got '$msg')" } | Out-Null } @@ -670,6 +692,16 @@ Run-StateAwareTest "filesystem: provision rejected" { Assert-True ($null -ne $envObj) "stdout is a parseable envelope" $code = if ($envObj) { $envObj.error.code } else { '' } Assert-True ($code -eq 'policy_validation') "error.code is 'policy_validation' (got '$code')" + + # MXC rejects this before any API call is made, so the structured + # failure fields must be absent entirely -- they describe an API + # operation that was in flight, and none was. + $hasOperation = if ($envObj) { $null -ne $envObj.error.PSObject.Properties['operation'] } else { $true } + Assert-True (-not $hasOperation) "error.operation is absent on an MXC-side rejection" + $hasNativeCode = if ($envObj) { $null -ne $envObj.error.PSObject.Properties['nativeCode'] } else { $true } + Assert-True (-not $hasNativeCode) "error.nativeCode is absent on an MXC-side rejection" + $hasRemediation = if ($envObj) { $null -ne $envObj.error.PSObject.Properties['remediation'] } else { $true } + Assert-True (-not $hasRemediation) "error.remediation is absent on an MXC-side rejection" } | Out-Null From c810f241ac61607ae8c783d226a0a263aa9975fc Mon Sep 17 00:00:00 2001 From: adpa-ms <> Date: Wed, 29 Jul 2026 15:35:03 -0700 Subject: [PATCH 2/3] fix(iso): never emit an empty error message; state operation-value stability Addresses both optional findings from the review of #708. O1 -- the wire `message` could be empty. `Message()` is a best-effort WinRT getter, and with the operation and HRESULT now in their own fields nothing backfills `message`, so a failed or empty getter reached the wire as `"message": ""`. The change was internally inconsistent about it: the `Err(Code())` arm already guarded the empty case, and `remediation` normalised empty-to-absent, but the `Ok(code)` arm passed the raw string through. Both best-effort getters now collapse to `Option` at the WinRT boundary and `IsoApiFailure::new` decides what absent means per field -- a stand-in for `message`, which the wire requires, and absence for `remediation`, which is optional. Normalising at construction rather than per branch is what keeps the guarantee from having to be restated at each call site; every construction path routes through it. O2 -- `operation` values are now published in the SDK README, recommended for telemetry aggregation, and pinned by an E2E assertion, but nothing said whether they are stable. They mirror the projected WinRT class and method names, which this repo does not own and cannot version, so they are now documented as best-effort diagnostics rather than a versioned contract, in the cross-backend contract, the backend spec, and the SDK README. The E2E assertion that pins an exact value carries a note explaining why pinning is correct there specifically: it verifies MXC's own mapping and moves with the constant. Also verified the boxing rationale the review could not check without running clippy: `MxcError` is 72 bytes as written and would be 136 inlined, against the default 128-byte `result_large_err` threshold. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 67a135de-4549-4c4e-8428-6f2a993e9bf2 --- docs/isolation-session/state-aware-rust.md | 9 + .../mxc-state-aware-sandbox-api.md | 4 +- sdk/node/README.md | 2 + .../isolation_session/common/src/error.rs | 157 +++++++++++++----- ...un_isolation_session_state_aware_tests.ps1 | 6 + 5 files changed, 140 insertions(+), 38 deletions(-) diff --git a/docs/isolation-session/state-aware-rust.md b/docs/isolation-session/state-aware-rust.md index fac571181..efdfc1f2e 100644 --- a/docs/isolation-session/state-aware-rust.md +++ b/docs/isolation-session/state-aware-rust.md @@ -253,8 +253,17 @@ in `operation`) so that consumers can aggregate on it. Where a lifecycle call su but reading one of its result properties fails, `operation` stays the lifecycle call and the finer step is described in `message`. +These values are **best-effort diagnostics, not a versioned contract**: they mirror the +projected WinRT class and method names, which this repo does not own. Branch on `code`; +treat `operation` as telemetry and log detail. See the +[cross-backend contract](../state-aware-lifecycle/mxc-state-aware-sandbox-api.md) §7.3. + `nativeCode` is the HRESULT rendered as lowercase hex, e.g. `0x80070490`. +`message` is the API's own text, passed through verbatim, and is never empty: when the +API reports a failure without a message, a short stand-in is substituted, because the +operation and status now live in their own fields and no longer backfill it. + `error.details` is unused by this backend. It remains the escape hatch for backend-specific structured data that has no cross-backend meaning; the three named fields above are backend-neutral and so live on the envelope itself. diff --git a/docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md b/docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md index d55575ba7..8c1f7a313 100644 --- a/docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md +++ b/docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md @@ -792,10 +792,12 @@ platform API: | Field | Meaning | |---|---| -| `operation` | The API call that failed, namespaced by its interface — e.g. `IsoSessionOps.RunProcessWithOptionsAsync`. Low-cardinality and free of call parameters, so it is safe to aggregate on in telemetry. | +| `operation` | The API call that failed, namespaced by its interface — e.g. `IsoSessionOps.RunProcessWithOptionsAsync`. Low-cardinality and free of call parameters, so it is safe to aggregate on in telemetry. **Best-effort diagnostic, not a versioned contract** — see below. | | `nativeCode` | The underlying platform status as a string. An HRESULT such as `0x80070490` on Windows; the field is platform-neutral, so another backend can carry an errno or equivalent. | | `remediation` | The API's actionable "how to fix it" hint, when it supplies one. | +**Stability.** Unlike `code`, which is a closed and versioned enum, the *values* of `operation` and `nativeCode` are **best-effort diagnostics and may change without a schema version bump**. They are derived from the underlying platform API — for IsolationSession, from the projected WinRT class and method names — which MXC does not own and cannot version. Consumers should aggregate on them for telemetry and log them for diagnosis, but branch program logic on `code`, and should not treat a particular `operation` value as a guarantee. (MXC's own end-to-end tests do pin exact values; that is deliberate — they verify MXC's mapping, and move with it in the same change.) + **Invariant:** `nativeCode` implies `operation`, and `remediation` implies `operation`. `operation` marks that an API operation was in flight; the other two refine it, and neither ever appears alone. A failure MXC raises before or outside any API call — a diff --git a/sdk/node/README.md b/sdk/node/README.md index 09d150d3e..6dcfaa39a 100644 --- a/sdk/node/README.md +++ b/sdk/node/README.md @@ -285,6 +285,8 @@ try { `operation`, `nativeCode` and `remediation` are optional and travel together: `nativeCode` and `remediation` never appear without `operation`. A failure MXC raises before reaching the backend — a malformed request or id, or a policy rejection — carries only `code` and `message`. +Branch program logic on `code`, which is a closed, versioned union. The *values* of `operation` and `nativeCode` are best-effort diagnostics derived from the underlying platform API and may change without a version bump — use them for telemetry, logging and diagnosis rather than control flow. + Full design and API: [`docs/state-aware-lifecycle/`](https://github.com/microsoft/mxc/tree/main/docs/state-aware-lifecycle/). diff --git a/src/backends/isolation_session/common/src/error.rs b/src/backends/isolation_session/common/src/error.rs index db310fae4..12297ac5c 100644 --- a/src/backends/isolation_session/common/src/error.rs +++ b/src/backends/isolation_session/common/src/error.rs @@ -61,6 +61,15 @@ fn format_native_code(code: u32) -> String { format!("{code:#010x}") } +/// Substituted when the API reports a failure but supplies no message text. +/// +/// `message` is a required field on the wire envelope. Before the components +/// were split out it was always non-empty because it embedded the operation +/// and HRESULT; now that those have their own fields, nothing backfills it, so +/// a failed or empty `Message()` getter would otherwise surface as +/// `"message": ""`. +const NO_API_MESSAGE: &str = "the IsolationSession API reported a failure without a message"; + /// The components of a failure raised by the IsolationSession API, kept /// separate rather than pre-formatted. /// @@ -68,7 +77,8 @@ fn format_native_code(code: u32) -> String { /// API call was in flight. `code` is absent only when the status could not be /// read; `remediation` only when the API supplied one. That is what upholds /// the `MxcError` invariant that `nativeCode` and `remediation` never appear -/// without `operation`. +/// without `operation`. `message` is likewise never empty — see +/// [`IsoApiFailure::new`]. #[derive(Debug, Clone, PartialEq, Eq)] pub(super) struct IsoApiFailure { /// Interface-qualified operation, e.g. `IsoSessionOps.AddUserAsync`. @@ -76,13 +86,39 @@ pub(super) struct IsoApiFailure { /// The underlying HRESULT, when it could be read. pub code: Option, /// The bare human-readable message — no operation prefix, no HRESULT, no - /// remediation folded in. + /// remediation folded in. Never empty. pub message: String, /// The API-supplied "how to fix it" hint, when it provided one. pub remediation: Option, } impl IsoApiFailure { + /// Builds a failure, normalising the two best-effort text fields. + /// + /// `Message()` and `Remediation()` are fallible getters that the API may + /// also answer with an empty string, so both arrive as `Option`. An absent + /// or empty `message` is replaced with [`NO_API_MESSAGE`] — the wire + /// requires the field, and no other field backfills it. An absent or empty + /// `remediation` stays absent, since that field is optional on the wire. + /// + /// Normalising here rather than at each call site is what keeps the + /// guarantee from having to be re-stated (and re-remembered) per branch. + fn new( + operation: &str, + code: Option, + message: Option, + remediation: Option, + ) -> Self { + Self { + operation: operation.to_string(), + code, + message: message + .filter(|m| !m.is_empty()) + .unwrap_or_else(|| NO_API_MESSAGE.to_string()), + remediation: remediation.filter(|r| !r.is_empty()), + } + } + /// Folds the components back into one human-readable string. /// /// Only the one-shot path consumes this: it has no structured error @@ -195,12 +231,12 @@ pub(super) fn transport_err( step: &str, err: &windows_core::Error, ) -> IsolationSessionError { - IsolationSessionError::Lifecycle(LifecycleFailure::Api(IsoApiFailure { - operation: operation.to_string(), - code: Some(err.code().0 as u32), - message: format!("{}: {}", step, err.message()), - remediation: None, - })) + IsolationSessionError::Lifecycle(LifecycleFailure::Api(IsoApiFailure::new( + operation, + Some(err.code().0 as u32), + Some(format!("{}: {}", step, err.message())), + None, + ))) } /// Maps an activation failure of the in-proc IsolationSession runtime API to @@ -218,12 +254,12 @@ pub(super) fn activation_error(code: u32, detail: &str) -> IsolationSessionError } else { format!("IsolationSession runtime API activation failed: {detail}") }; - IsolationSessionError::ServiceUnavailable(IsoApiFailure { - operation: op::ACTIVATE.to_string(), - code: Some(code), - message, - remediation: None, - }) + IsolationSessionError::ServiceUnavailable(IsoApiFailure::new( + op::ACTIVATE, + Some(code), + Some(message), + None, + )) } /// Whether an `ERROR_NOT_FOUND` from this operation means "the sandbox is @@ -272,12 +308,15 @@ pub(super) fn format_iso_error( err: &IsoSessionError, promotion: StalePromotion, ) -> IsolationSessionError { - let message = err.Message().map(|h| h.to_string()).unwrap_or_default(); - let remediation = err - .Remediation() + // Both getters are best-effort and may also answer with an empty string, + // so both collapse to `None` here and `IsoApiFailure::new` decides what an + // absent value means for each field. + let message = err + .Message() .map(|h| h.to_string()) .ok() - .filter(|r| !r.is_empty()); + .filter(|m| !m.is_empty()); + let remediation = err.Remediation().map(|h| h.to_string()).ok(); // `Code()` is the classification-critical field: it drives the `Stale` // promotion, so fabricating 0 when the getter fails would silently @@ -287,26 +326,23 @@ pub(super) fn format_iso_error( // HRESULT — that would describe reading the field, not the operation. match err.Code() { Ok(code) => classify_api_failure( - IsoApiFailure { - operation: operation.to_string(), - code: Some(code.0 as u32), - message, - remediation, - }, + IsoApiFailure::new(operation, Some(code.0 as u32), message, remediation), promotion, ), Err(read_err) => { + // No usable status, so the note is the only signal that + // classification is degraded — keep it in the message. let note = format!("could not read HRESULT code: {read_err}"); - IsolationSessionError::Lifecycle(LifecycleFailure::Api(IsoApiFailure { - operation: operation.to_string(), - code: None, - message: if message.is_empty() { - note - } else { - format!("{message} ({note})") - }, + let message = Some(match message { + Some(m) => format!("{m} ({note})"), + None => note, + }); + IsolationSessionError::Lifecycle(LifecycleFailure::Api(IsoApiFailure::new( + operation, + None, + message, remediation, - })) + ))) } } } @@ -352,14 +388,61 @@ mod tests { use super::*; fn api_failure(code: Option) -> IsoApiFailure { - IsoApiFailure { - operation: op::STOP_SESSION.to_string(), + IsoApiFailure::new( + op::STOP_SESSION, code, - message: "agent user not found".to_string(), - remediation: Some("Re-provision the sandbox.".to_string()), + Some("agent user not found".to_string()), + Some("Re-provision the sandbox.".to_string()), + ) + } + + // ── Best-effort text fields are normalised at construction ─────────── + + /// `message` is required on the wire, and since the operation and status + /// moved to their own fields nothing else backfills it. A failed or empty + /// `Message()` getter must not reach a consumer as `"message": ""`. + #[test] + fn absent_or_empty_api_message_is_replaced_with_a_stand_in() { + for supplied in [None, Some(String::new())] { + let failure = IsoApiFailure::new(op::ADD_USER, Some(0x80004005), supplied, None); + assert_eq!(failure.message, NO_API_MESSAGE); + assert!(!failure.message.is_empty()); + } + } + + #[test] + fn a_supplied_api_message_is_kept_verbatim() { + let failure = IsoApiFailure::new( + op::ADD_USER, + Some(0x80004005), + Some("The provision was not found.".to_string()), + None, + ); + assert_eq!(failure.message, "The provision was not found."); + } + + /// `remediation` is optional on the wire, so an empty one stays absent + /// rather than being stood in for -- the opposite treatment to `message`, + /// and the reason both go through one constructor. + #[test] + fn absent_or_empty_remediation_stays_absent() { + for supplied in [None, Some(String::new())] { + let failure = IsoApiFailure::new(op::ADD_USER, None, None, supplied); + assert_eq!(failure.remediation, None); } } + /// The stand-in must survive to the wire, not just to the internal type. + #[test] + fn stand_in_message_reaches_the_envelope() { + let mapped = map_lifecycle_error(classify_api_failure( + IsoApiFailure::new(op::STOP_SESSION, Some(0x80004005), None, None), + StalePromotion::Eligible, + )); + assert_eq!(mapped.message, NO_API_MESSAGE); + assert!(!mapped.to_envelope().message.is_empty()); + } + // ── Constants pinned to the OS values they mirror ──────────────────── #[test] diff --git a/tests/scripts/run_isolation_session_state_aware_tests.ps1 b/tests/scripts/run_isolation_session_state_aware_tests.ps1 index 2465c0b6e..0b8b28f7e 100644 --- a/tests/scripts/run_isolation_session_state_aware_tests.ps1 +++ b/tests/scripts/run_isolation_session_state_aware_tests.ps1 @@ -641,6 +641,12 @@ try { # Structured failure fields: an API failure names the operation # that failed and carries the underlying HRESULT. + # + # Pinning the exact operation string is deliberate here: this + # verifies MXC's own mapping (that the right `op::` constant + # reaches the wire), so it is expected to move together with that + # constant. Consumers should not pin these values -- they mirror + # the projected WinRT names, which MXC does not own. $operation = if ($envObj) { [string]$envObj.error.operation } else { '' } Assert-True ($operation -eq 'IsoSessionOps.StopSessionAsync') ` "error.operation is 'IsoSessionOps.StopSessionAsync' (got '$operation')" From 478776a54a50b4f8e3b79f81d5da71875a3f07eb Mon Sep 17 00:00:00 2001 From: adpa-ms <> Date: Fri, 31 Jul 2026 16:12:11 -0700 Subject: [PATCH 3/3] fix(errors): address PR review round 2 Fixes found by review of the structured-error-fields change. - transport_err no longer emits a dangling "step: " when the platform supplies no message text. An HRESULT with no OS message-table entry (0xDEADBEEF, and any custom facility code) returns an empty message(), and joining unconditionally produced a technically-non-empty string that slipped past the empty-message guard in IsoApiFailure::new. Fall back to the step alone. ~34 call sites route through this one join. - MxcError::Display now renders the API detail when present, so a consumer that only logs the error keeps the operation and status that used to be concatenated into message. Rendering only: the wire envelope still carries message bare, with the components in their own fields. Replaces the thiserror derive with explicit Display + Error. - The Code()-getter-failure branch moves into unreadable_code_failure, a pure function, so its composition is reachable from a unit test. format_iso_error stays a thin WinRT adapter. - Correct the ApiFailure doc comment: grouping makes the invariant the easy path, not an unrepresentable-to-violate one (Default was derived and the fields are pub). Drop the unused Default derive. - Add #[non_exhaustive] to MxcError and ErrorEnvelope so future fields are a non-event for other workspace crates. - Guard the MxcError constructor against a nullish argument, which took the object branch and failed inside super() with a TypeError naming "message". Default the positional message rather than asserting it. - Un-export WireError and mxcErrorFromEnvelope: they exist so the SDK's own parse sites share one widening point. MxcErrorFields stays exported because it is the parameter type of a public constructor overload -- hiding the name leaves the type usable but unnameable. - Lift the four host-independent policy-validation cases out of the probe-gated suite. Both CI systems set MXC_SKIP_OS_BUILD_DEPENDENT_TESTS=1, so nothing in that suite ran in CI; these need the isolation_session feature compiled in but not a host that can run isolation sessions. - Document that the structured fields are currently populated only by IsolationSession state-aware operations. Gates: fmt; clippy --all-targets --all-features -D warnings; Rust build + test iso ON and iso OFF (wxc_host_prep 16/16 elevated); SDK unit 231/0; SDK integration 45/0 with the four lifted cases now executing under the CI skip flag; versioning + dotnet parity 7/7; VM suites 73 passed / 0 failed with an empty leak delta; manual TTY tests confirmed by the operator. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 67a135de-4549-4c4e-8428-6f2a993e9bf2 --- .../mxc-state-aware-sandbox-api.md | 7 + sdk/node/README.md | 5 +- sdk/node/src/errors.ts | 12 +- sdk/node/src/index.ts | 11 +- .../isolation-session-state-aware.test.ts | 28 +++- sdk/node/tests/unit/errors.test.ts | 27 ++++ .../isolation_session/common/src/error.rs | 144 ++++++++++++++++-- src/core/wxc_common/src/mxc_error.rs | 106 +++++++++++-- 8 files changed, 301 insertions(+), 39 deletions(-) diff --git a/docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md b/docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md index 8c1f7a313..abbb1b5cd 100644 --- a/docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md +++ b/docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md @@ -796,6 +796,13 @@ platform API: | `nativeCode` | The underlying platform status as a string. An HRESULT such as `0x80070490` on Windows; the field is platform-neutral, so another backend can carry an errno or equivalent. | | `remediation` | The API's actionable "how to fix it" hint, when it supplies one. | +**Availability.** These fields are currently populated only by **IsolationSession +state-aware** operations. Windows Sandbox has no semantic error channel to derive them +from, and the one-shot surface composes its full detail into `message` instead, so all +three are uniformly absent there. Other backends may adopt them as they grow an +equivalent channel — treat all three as optional on every backend, and branch program +logic on `code` first. + **Stability.** Unlike `code`, which is a closed and versioned enum, the *values* of `operation` and `nativeCode` are **best-effort diagnostics and may change without a schema version bump**. They are derived from the underlying platform API — for IsolationSession, from the projected WinRT class and method names — which MXC does not own and cannot version. Consumers should aggregate on them for telemetry and log them for diagnosis, but branch program logic on `code`, and should not treat a particular `operation` value as a guarantee. (MXC's own end-to-end tests do pin exact values; that is deliberate — they verify MXC's mapping, and move with it in the same change.) **Invariant:** `nativeCode` implies `operation`, and `remediation` implies `operation`. diff --git a/sdk/node/README.md b/sdk/node/README.md index 6dcfaa39a..02053fac0 100644 --- a/sdk/node/README.md +++ b/sdk/node/README.md @@ -285,6 +285,8 @@ try { `operation`, `nativeCode` and `remediation` are optional and travel together: `nativeCode` and `remediation` never appear without `operation`. A failure MXC raises before reaching the backend — a malformed request or id, or a policy rejection — carries only `code` and `message`. +These three are currently populated only by **IsolationSession state-aware** operations. Windows Sandbox has no semantic error channel to derive them from, and the one-shot surface folds the same detail into `message` instead, so they are uniformly absent there — always treat them as optional. + Branch program logic on `code`, which is a closed, versioned union. The *values* of `operation` and `nativeCode` are best-effort diagnostics derived from the underlying platform API and may change without a version bump — use them for telemetry, logging and diagnosis rather than control flow. Full design and API: [`docs/state-aware-lifecycle/`](https://github.com/microsoft/mxc/tree/main/docs/state-aware-lifecycle/). @@ -413,9 +415,8 @@ getTemporaryFilesPolicy(env?) → FilesystemPolicyResult UiCapabilitySupport // Errors (typed wire-format errors from wxc-exec) -ErrorCode, MxcError, MxcErrorFields, WireError +ErrorCode, MxcError, MxcErrorFields mxcErrorFromCode(code, message, details?) → MxcError -mxcErrorFromEnvelope(wireError) → MxcError ``` Full TypeScript definitions ship with the package (`dist/index.d.ts`). All exports are named exports from `@microsoft/mxc-sdk`. diff --git a/sdk/node/src/errors.ts b/sdk/node/src/errors.ts index 324ca500e..f221ef14f 100644 --- a/sdk/node/src/errors.ts +++ b/sdk/node/src/errors.ts @@ -92,9 +92,19 @@ export class MxcError extends Error { message?: string, details?: Record, ) { + // The overload discriminates on `typeof === 'string'`, so anything else + // — including `null` and `undefined` from an `any` cast or an absent wire + // envelope — would take the object branch and fail inside `super()` with + // a `TypeError` naming `message`, which says nothing about the real + // mistake. Reject it here instead, where the message can. + if (codeOrFields === null || codeOrFields === undefined) { + throw new TypeError( + `MxcError: expected an error code string or a field object, got ${String(codeOrFields)}`, + ); + } const fields: MxcErrorFields = typeof codeOrFields === 'string' - ? { code: codeOrFields, message: message as string, details } + ? { code: codeOrFields, message: message ?? '', details } : codeOrFields; super(fields.message); this.code = fields.code; diff --git a/sdk/node/src/index.ts b/sdk/node/src/index.ts index 6ff2a8977..6d565c8b5 100644 --- a/sdk/node/src/index.ts +++ b/sdk/node/src/index.ts @@ -67,14 +67,19 @@ export { ToolsPolicyOptions, } from './policy.js'; -// Export typed wire-format errors +// Export typed wire-format errors. +// +// `WireError` and `mxcErrorFromEnvelope` are deliberately NOT re-exported: +// they exist so the SDK's own envelope-parsing sites share one widening +// point, and keeping them module-internal leaves the wire-parsing internals +// free to change. `MxcErrorFields` *is* exported because it is the parameter +// type of a public `MxcError` constructor overload — hiding the name would +// leave the type usable via an object literal but impossible to name. export { ErrorCode, MxcError, MxcErrorFields, - WireError, mxcErrorFromCode, - mxcErrorFromEnvelope, } from './errors.js'; // Export state-aware lifecycle types diff --git a/sdk/node/tests/integration/isolation-session-state-aware.test.ts b/sdk/node/tests/integration/isolation-session-state-aware.test.ts index 7ff1633ce..13bd1c6d9 100644 --- a/sdk/node/tests/integration/isolation-session-state-aware.test.ts +++ b/sdk/node/tests/integration/isolation-session-state-aware.test.ts @@ -28,9 +28,14 @@ import { } from '@microsoft/mxc-sdk'; import { probeStateAwareRuntime, safeDeprovision, sandboxSkipReason } from './test-helpers.js'; -const skipReason = os.platform() !== 'win32' - ? 'IsolationSession is Windows-only' - : sandboxSkipReason ?? await probeStateAwareRuntime('isolation_session'); +const platformSkipReason = + os.platform() !== 'win32' ? 'IsolationSession is Windows-only' : undefined; + +// Host-dependent gate: adds the runtime probe (and the CI opt-out) on top of +// the platform check. `??` short-circuits, so the probe is never spawned on a +// non-Windows host. +const skipReason = + platformSkipReason ?? sandboxSkipReason ?? (await probeStateAwareRuntime('isolation_session')); describe('IsolationSession state-aware lifecycle E2E', { skip: skipReason }, () => { it('runs full lifecycle: provision -> start -> exec -> stop -> deprovision', async () => { @@ -159,13 +164,26 @@ describe('IsolationSession state-aware lifecycle E2E', { skip: skipReason }, () await safeDeprovision(sandboxId); } }); +}); - // --- Runtime backend guard for the network acknowledgment ---------------- +// Policy rejections are raised by MXC's own validation, before any +// IsolationSession API call: the dispatcher runs `validate_provision` ahead of +// `provision`, and `IsolationSessionRunner` is a stateless marker whose +// construction touches no WinRT. So these need a `wxc-exec.exe` built with +// `--features isolation_session` (which CI builds) but *not* a host that can +// actually run isolation sessions. +// +// Keeping them out of the probe-gated suite above is deliberate. That suite is +// additionally gated on `sandboxSkipReason`, and both CI systems set +// `MXC_SKIP_OS_BUILD_DEPENDENT_TESTS=1`, so anything inside it never runs in +// CI. These assertions cover the full chain this feature depends on — Rust +// envelope serialisation → dispatcher → SDK parse → typed `MxcError` — which +// is exactly the path where drift would otherwise go unnoticed. +describe('IsolationSession state-aware policy validation', { skip: platformSkipReason }, () => { // The TypeScript type makes `network` required (and pins its value) at // provision, but a plain-JS caller can bypass that. These assert the backend // itself refuses a missing or non-canonical network acknowledgment — the // "respect or refuse" guarantee must not rest on the compile-time type alone. - // Validation runs before the OS service is touched, so no cleanup is needed. type UntypedProvision = ( containment: 'isolation_session', config: unknown, diff --git a/sdk/node/tests/unit/errors.test.ts b/sdk/node/tests/unit/errors.test.ts index bbd47fc89..149942671 100644 --- a/sdk/node/tests/unit/errors.test.ts +++ b/sdk/node/tests/unit/errors.test.ts @@ -117,6 +117,33 @@ describe('MxcError structured fields', () => { assert.strictEqual(err.remediation, undefined); assert.strictEqual(err.details, undefined); }); + + // The overload discriminates on `typeof === 'string'`, so a nullish + // argument takes the object branch. Without a guard it fails inside + // `super()` with a TypeError naming `message`, which tells the reader + // nothing about what actually went wrong. + for (const bad of [undefined, null]) { + it(`rejects ${String(bad)} with a message naming the real problem`, () => { + assert.throws( + () => new (MxcError as unknown as new (arg: unknown) => MxcError)(bad), + (err: unknown) => { + assert.ok(err instanceof TypeError, `expected TypeError, got ${String(err)}`); + assert.match(err.message, /MxcError: expected an error code string or a field object/); + assert.match(err.message, new RegExp(String(bad))); + return true; + }, + ); + }); + } + + // `message` was an unchecked `as string`. Omitting it yields an empty + // string (the `Error` constructor ignores an undefined message), not the + // literal text "undefined". + it('yields an empty message when the positional message is omitted', () => { + const err = new (MxcError as unknown as new (code: string) => MxcError)('stale_id'); + assert.strictEqual(err.code, 'stale_id'); + assert.strictEqual(err.message, ''); + }); }); describe('mxcErrorFromEnvelope', () => { diff --git a/src/backends/isolation_session/common/src/error.rs b/src/backends/isolation_session/common/src/error.rs index 12297ac5c..6a6e86d94 100644 --- a/src/backends/isolation_session/common/src/error.rs +++ b/src/backends/isolation_session/common/src/error.rs @@ -234,11 +234,27 @@ pub(super) fn transport_err( IsolationSessionError::Lifecycle(LifecycleFailure::Api(IsoApiFailure::new( operation, Some(err.code().0 as u32), - Some(format!("{}: {}", step, err.message())), + Some(compose_transport_message(step, &err.message())), None, ))) } +/// Joins `step` with the platform's text, tolerating an absent text. +/// +/// `windows_core::Error::message()` answers with an empty string for an +/// HRESULT that has no entry in the OS message table (`0xDEADBEEF`, and any +/// custom facility code a service invents). Formatting unconditionally would +/// then yield a dangling `"wait failed: "` — non-empty, so it sails past the +/// empty-message guard in [`IsoApiFailure::new`] and reaches the caller as a +/// trailing colon with no explanation. Fall back to the step alone instead. +fn compose_transport_message(step: &str, platform_message: &str) -> String { + if platform_message.is_empty() { + step.to_string() + } else { + format!("{step}: {platform_message}") + } +} + /// Maps an activation failure of the in-proc IsolationSession runtime API to /// `ServiceUnavailable`. /// @@ -301,8 +317,8 @@ pub(super) fn classify_api_failure( /// Reads an `IsoSessionError`'s components and classifies them. /// -/// Thin by design — the rules live in [`classify_api_failure`]; this only -/// crosses the WinRT boundary. +/// Thin by design — the rules live in [`classify_api_failure`] and +/// [`unreadable_code_failure`]; this only crosses the WinRT boundary. pub(super) fn format_iso_error( operation: &str, err: &IsoSessionError, @@ -330,23 +346,40 @@ pub(super) fn format_iso_error( promotion, ), Err(read_err) => { - // No usable status, so the note is the only signal that - // classification is degraded — keep it in the message. - let note = format!("could not read HRESULT code: {read_err}"); - let message = Some(match message { - Some(m) => format!("{m} ({note})"), - None => note, - }); - IsolationSessionError::Lifecycle(LifecycleFailure::Api(IsoApiFailure::new( - operation, - None, - message, - remediation, - ))) + unreadable_code_failure(operation, message, remediation, &read_err.to_string()) } } } +/// Builds the failure for the case where the status getter itself failed. +/// +/// Pure, and split out of [`format_iso_error`] for the same reason +/// [`classify_api_failure`] is: that function takes an `IsoSessionError`, a +/// WinRT interface obtained by activation, so nothing inside it can be reached +/// from a unit test. +/// +/// With no usable status there is nothing to classify on, so this never +/// promotes to `Stale` — the note is the only signal that classification is +/// degraded, which is why it is folded into the message rather than dropped. +fn unreadable_code_failure( + operation: &str, + message: Option, + remediation: Option, + read_err: &str, +) -> IsolationSessionError { + let note = format!("could not read HRESULT code: {read_err}"); + let message = Some(match message { + Some(m) => format!("{m} ({note})"), + None => note, + }); + IsolationSessionError::Lifecycle(LifecycleFailure::Api(IsoApiFailure::new( + operation, + None, + message, + remediation, + ))) +} + /// Checks the `Error` property of an `IsoSessionResult`. `Ok(())` on no /// error; lifecycle (or stale) error with structured details otherwise. pub(super) fn check_result( @@ -517,6 +550,85 @@ mod tests { assert_eq!(mapped.native_code(), Some("0x80070490")); } + // ── Transport messages tolerate an absent platform text ────────────── + + /// An HRESULT with no OS message-table entry yields an empty + /// `message()`. Joining unconditionally produced `"wait failed: "`, which + /// is non-empty and therefore slipped past the guard in + /// `IsoApiFailure::new` — the caller got a trailing colon and no + /// explanation. + #[test] + fn transport_message_without_platform_text_is_the_step_alone() { + assert_eq!(compose_transport_message("wait failed", ""), "wait failed"); + } + + #[test] + fn transport_message_with_platform_text_keeps_the_step_prefix() { + assert_eq!( + compose_transport_message("wait failed", "Unspecified error"), + "wait failed: Unspecified error" + ); + } + + /// End-to-end over a real `windows_core::Error`: `0xDEADBEEF` has no + /// message-table entry on any Windows build, so this pins the whole path + /// rather than just the helper. + #[test] + fn transport_failure_with_unmapped_hresult_has_no_dangling_separator() { + let err = windows_core::Error::from_hresult(windows_core::HRESULT(0xDEADBEEF_u32 as i32)); + assert!( + err.message().is_empty(), + "0xDEADBEEF unexpectedly has a message-table entry: {:?}", + err.message() + ); + let mapped = map_lifecycle_error(transport_err(op::ADD_USER, "wait failed", &err)); + assert_eq!(mapped.message, "wait failed"); + assert!(!mapped.message.ends_with(": ")); + assert_eq!(mapped.native_code(), Some("0xdeadbeef")); + } + + // ── The status getter itself can fail ──────────────────────────────── + + /// With no readable status there is nothing to classify on, so the + /// failure stays a plain lifecycle error and `nativeCode` stays off the + /// wire — carrying the getter's own HRESULT would describe reading the + /// field, not the operation. + #[test] + fn unreadable_code_keeps_the_message_and_omits_native_code() { + let err = unreadable_code_failure( + op::STOP_SESSION, + Some("agent user not found".to_string()), + Some("Re-provision the sandbox.".to_string()), + "RPC_E_DISCONNECTED", + ); + let mapped = map_lifecycle_error(err); + assert_eq!(mapped.code, MxcErrorCode::BackendError); + assert_eq!( + mapped.message, + "agent user not found (could not read HRESULT code: RPC_E_DISCONNECTED)" + ); + assert_eq!(mapped.operation(), Some("IsoSessionOps.StopSessionAsync")); + assert_eq!(mapped.native_code(), None); + assert_eq!(mapped.remediation(), Some("Re-provision the sandbox.")); + } + + /// Even an `ERROR_NOT_FOUND`-shaped failure cannot promote here: the code + /// is precisely what could not be read. + #[test] + fn unreadable_code_never_promotes_to_stale() { + let err = unreadable_code_failure(op::STOP_SESSION, None, None, "RPC_E_DISCONNECTED"); + assert!(matches!( + err, + IsolationSessionError::Lifecycle(LifecycleFailure::Api(_)) + )); + let mapped = map_lifecycle_error(err); + assert_eq!(mapped.code, MxcErrorCode::BackendError); + assert_eq!( + mapped.message, + "could not read HRESULT code: RPC_E_DISCONNECTED" + ); + } + // ── Field population and the MxcError invariant ────────────────────── #[test] diff --git a/src/core/wxc_common/src/mxc_error.rs b/src/core/wxc_common/src/mxc_error.rs index b29794cef..6dfa123f8 100644 --- a/src/core/wxc_common/src/mxc_error.rs +++ b/src/core/wxc_common/src/mxc_error.rs @@ -11,7 +11,7 @@ use serde::{Deserialize, Serialize}; use serde_json::Value; -use thiserror::Error; +use std::fmt; /// Closed set of wire-format error codes. Matches the SDK's `ErrorCode` string /// union one-for-one; serialised as snake_case strings on the wire. @@ -60,12 +60,17 @@ impl std::fmt::Display for MxcErrorCode { /// Structured detail for a failure that originated in an underlying platform /// API. /// -/// Grouping these makes the envelope invariant unrepresentable to violate: -/// `native_code` and `remediation` cannot exist without `operation`, because -/// they live inside the same value. `MxcError` holds this boxed, so adding -/// detail costs one pointer rather than widening every `Result<_, MxcError>` -/// in the codebase. -#[derive(Debug, Clone, PartialEq, Eq, Default)] +/// Grouping these is what carries the envelope invariant: `native_code` and +/// `remediation` live beside `operation` rather than as independent optionals, +/// so a failure that names a status without naming the call it came from is +/// not something the normal construction path can produce. Build one with +/// [`ApiFailure::new`], which requires the operation up front, and add the +/// optional parts with the `with_*` builders. (The fields are `pub` for +/// destructuring, so a hand-rolled literal *can* still put an empty string in +/// `operation` — the type makes the invariant the easy path, not an enforced +/// one.) `MxcError` holds this boxed, so adding detail costs one pointer +/// rather than widening every `Result<_, MxcError>` in the codebase. +#[derive(Debug, Clone, PartialEq, Eq)] pub struct ApiFailure { /// The API call that failed, namespaced by its interface — e.g. /// `IsoSessionOps.RunProcessWithOptionsAsync`. Kept low-cardinality and @@ -120,8 +125,8 @@ impl ApiFailure { /// A new *backend-neutral* concept earns a field on `ApiFailure`; /// *backend-specific* structured data belongs in `details`, which stays open /// for that purpose. -#[derive(Debug, Clone, PartialEq, Eq, Error)] -#[error("{code}: {message}")] +#[derive(Debug, Clone, PartialEq, Eq)] +#[non_exhaustive] pub struct MxcError { pub code: MxcErrorCode, pub message: String, @@ -130,6 +135,31 @@ pub struct MxcError { pub api_failure: Option>, } +/// Renders `code: message`, then the API detail in brackets when present — +/// e.g. `backend_error: The provision was not found. [IsoSessionOps.StopSessionAsync 0x80070490]`. +/// +/// The bracketed suffix exists because `message` is now the API's own text +/// alone; without it a consumer that only logs the error (`error!("{e}")`, +/// `e.to_string()`) would lose the operation and status that used to be +/// concatenated into the message. This affects **rendering only** — the wire +/// envelope still carries `message` bare, with the components in their own +/// fields. +impl fmt::Display for MxcError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}: {}", self.code, self.message)?; + if let Some(api) = &self.api_failure { + write!(f, " [{}", api.operation)?; + if let Some(native_code) = &api.native_code { + write!(f, " {native_code}")?; + } + f.write_str("]")?; + } + Ok(()) + } +} + +impl std::error::Error for MxcError {} + impl MxcError { pub fn new(code: MxcErrorCode, message: impl Into) -> Self { Self { @@ -231,6 +261,7 @@ impl MxcError { /// See [`MxcError`] for the meaning of the structured failure fields and the /// invariant relating them. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[non_exhaustive] pub struct ErrorEnvelope { pub code: MxcErrorCode, pub message: String, @@ -475,9 +506,10 @@ mod tests { assert_eq!(err.remediation(), Some("Re-provision the sandbox.")); } - /// `native_code` and `remediation` live inside `ApiFailure`, so they - /// cannot be set without an `operation` — the envelope invariant holds - /// by construction rather than by convention. + /// `native_code` and `remediation` live inside `ApiFailure`, so the + /// normal construction path cannot set them without an `operation` — + /// the envelope invariant holds by construction rather than by + /// convention. #[test] fn structured_detail_always_carries_an_operation() { let err = MxcError::backend_error("boom") @@ -487,6 +519,56 @@ mod tests { assert_eq!(err.remediation(), None); } + // ── Display keeps the diagnostic detail a logger would otherwise lose ─ + + /// `message` is the API's own text alone now, so a consumer that only + /// logs the error would lose the operation and status that used to be + /// concatenated into it. `Display` re-attaches them. + #[test] + fn display_appends_operation_and_native_code() { + let err = MxcError::backend_error("The provision was not found.") + .with_api_failure(full_api_failure()); + assert_eq!( + err.to_string(), + "backend_error: The provision was not found. \ + [IsoSessionOps.AddUserAsync 0x80070490]" + ); + } + + /// A status that could not be read leaves `native_code` absent; the + /// operation alone is still worth rendering. + #[test] + fn display_renders_operation_without_native_code() { + let err = MxcError::backend_error("boom") + .with_api_failure(ApiFailure::new("IsoSessionOps.AddUserAsync")); + assert_eq!( + err.to_string(), + "backend_error: boom [IsoSessionOps.AddUserAsync]" + ); + } + + /// A failure MXC raises itself has no API detail, so the rendering is + /// unchanged from before the structured fields existed. + #[test] + fn display_without_api_failure_is_code_and_message_only() { + assert_eq!( + MxcError::policy_validation("user.upn must contain '@'").to_string(), + "policy_validation: user.upn must contain '@'" + ); + } + + /// `Display` is a rendering concern only — the wire `message` stays the + /// bare API text, with the components in their own fields. + #[test] + fn display_enrichment_does_not_leak_into_the_envelope() { + let env = MxcError::backend_error("The provision was not found.") + .with_api_failure(full_api_failure()) + .to_envelope(); + assert_eq!(env.message, "The provision was not found."); + assert!(!env.message.contains("IsoSessionOps")); + assert!(!env.message.contains("0x80070490")); + } + #[test] fn to_envelope_copies_structured_fields_through() { let env = MxcError::backend_error("boom")