diff --git a/docs/isolation-session/state-aware-rust.md b/docs/isolation-session/state-aware-rust.md index 753b87246..efdfc1f2e 100644 --- a/docs/isolation-session/state-aware-rust.md +++ b/docs/isolation-session/state-aware-rust.md @@ -223,12 +223,64 @@ 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`. + +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. + +### 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..abbb1b5cd 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,45 @@ 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. **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. | + +**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`. +`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..02053fac0 100644 --- a/sdk/node/README.md +++ b/sdk/node/README.md @@ -265,6 +265,30 @@ 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`. + +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/). @@ -391,7 +415,8 @@ getTemporaryFilesPolicy(env?) → FilesystemPolicyResult UiCapabilitySupport // Errors (typed wire-format errors from wxc-exec) -ErrorCode, MxcError, mxcErrorFromCode(code) +ErrorCode, MxcError, MxcErrorFields +mxcErrorFromCode(code, message, details?) → 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..f221ef14f 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,43 @@ 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, + ) { + // 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 ?? '', 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 +124,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 +135,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..6d565c8b5 100644 --- a/sdk/node/src/index.ts +++ b/sdk/node/src/index.ts @@ -67,10 +67,18 @@ 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, mxcErrorFromCode, } from './errors.js'; 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..13bd1c6d9 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, @@ -27,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 () => { @@ -158,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, @@ -200,4 +219,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..149942671 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,124 @@ 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); + }); + + // 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', () => { + 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..6a6e86d94 100644 --- a/src/backends/isolation_session/common/src/error.rs +++ b/src/backends/isolation_session/common/src/error.rs @@ -3,40 +3,208 @@ //! 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}") +} + +/// 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. +/// +/// `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`. `message` is likewise never empty — see +/// [`IsoApiFailure::new`]. +#[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. 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 + /// 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 +215,269 @@ 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::new( + operation, + Some(err.code().0 as u32), + Some(compose_transport_message(step, &err.message())), + 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() +/// 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!(" -- remediation: {}", remediation) - }; - let formatted = format!("{} failed: {} (HRESULT: {:#010x}){}", op, msg, code, suffix); - if code == ERROR_NOT_FOUND_HRESULT { - IsolationSessionError::Stale(formatted) + format!("{step}: {platform_message}") + } +} + +/// 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::new( + op::ACTIVATE, + Some(code), + Some(message), + 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`] and +/// [`unreadable_code_failure`]; this only crosses the WinRT boundary. +pub(super) fn format_iso_error( + operation: &str, + err: &IsoSessionError, + promotion: StalePromotion, +) -> IsolationSessionError { + // 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(|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 + // 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::new(operation, Some(code.0 as u32), message, remediation), + promotion, + ), + Err(read_err) => { + 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 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; + fn api_failure(code: Option) -> IsoApiFailure { + IsoApiFailure::new( + op::STOP_SESSION, + code, + 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 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 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] fn error_not_found_hresult_constant_matches_win32() { // HRESULT_FROM_WIN32(ERROR_NOT_FOUND) = 0x80070000 | (1168 & 0xFFFF) @@ -157,4 +488,345 @@ 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")); + } + + // ── 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] + 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..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. @@ -57,25 +57,116 @@ impl std::fmt::Display for MxcErrorCode { } } +/// Structured detail for a failure that originated in an underlying platform +/// API. +/// +/// 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 + /// 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!({...}))`. -#[derive(Debug, Clone, PartialEq, Eq, Error)] -#[error("{code}: {message}")] +/// 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)] +#[non_exhaustive] 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>, } +/// 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 { code, message: message.into(), details: None, + api_failure: None, } } @@ -84,11 +175,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 +255,28 @@ 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)] +#[non_exhaustive] 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 +404,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 +419,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 +440,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 +455,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 +481,170 @@ 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 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") + .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); + } + + // ── 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") + .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..0b8b28f7e 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,29 @@ 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. + # + # 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')" + $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 +698,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