fix(isolation-session): refuse ui and unsupported lifecycle rather than dropping them

[adpa-ms]

📖 Description

Follow-on to #682, same backend and same seam: policy the IsolationSession backend cannot honor was accepted and silently dropped instead of refused.

ui is refused at every phase on both surfaces, and no ui posture is truthful for this backend — there is no value combination that could be accepted instead. The section states intent about the contained code's relationship to the user's environment, and was modelled on a process/job boundary where "the clipboard" and "the desktop" are the user's. An isolation session is a separate OS session: the contained code keeps its UI capabilities but cannot reach the host's. So disable either denies capabilities the session grants or promises a GUI the user can never see, and every clipboard value describes a relationship to a clipboard the sandbox cannot touch. Only injection: false is honest, and it cannot be supplied alone because the other fields materialize to defaults that are false. With nothing truthful to accept, there is no acknowledgment-style gate as there is for network.

The check is presence-based via a new ContainerPolicy::ui_specified flag, the twin of network_specified: UiPolicy::default() is full lockdown, so an explicit lockdown ui is indistinguishable by value from an absent one. An omitted ui is accepted and applies no restriction — docs/schema.md's default-deny reading is now qualified as per-backend rather than global, since it never held for a backend that does not enforce the section.

lifecycle is refused by value on one-shot, where the defaults do match reality: the backend always stops the session and removes the agent user before returning, which is exactly destroyOnExit: true. Only destroyOnExit: false and preservePolicy: true are refused. State-aware already rejected the whole section; oneshot.md had listed destroyOnExit under "Implemented".

Docs. oneshot.md and docs/schema.md carried claims that no longer matched the code — schema.md documented no ui section at all, and said foreign backend sections are ignored when they are rejected. The honor matrix in state-aware-rust.md now covers every field a caller can express across both surfaces, rather than the three-field state-aware minimum §10.3 requires; that narrowness is why these gaps went unnoticed. Rows that are accepted-and-ignored are documented as such rather than omitted, and the matrix distinguishes per-surface error codes: policy_validation on state-aware, backend_error on one-shot, which discards the typed variant.

Scope. IsolationSession only. The single parser change is one line (policy.ui_specified = ui.is_some();) — everything else lives in the backend's own validators. No schema bump, no wire-shape change, no SDK type changes, no cross-backend rules.

Deliberately out of scope: detecting mis-slotted experimental.isolation_session payloads. Those are documented fields in undocumented positions — a caller error rather than a surprise from a correct request — and generic detection is a cross-backend concern. The matrix documents the resulting behaviour. Windows Sandbox has the same unhandled ui policy; also out of scope.

🔗 References

Follows #682 (network policy) and #683 (dev-schema drift), both on this branch.

🔍 Validation

Host: fmt; clippy --all-features -D warnings; workspace release tests with iso ON and OFF; wxc_host_prep run elevated (its tests need admin); versioning suite over 191 configs; sdk/node build + 203 unit tests.

Tests at all three tiers. Rust unit covers the refusal at every phase, absent and lockdown-equivalent cases, the filesystem → ui → network precedence, the lifecycle values, and ui_specified on both surfaces. A new e2e_isolation_session_policy.rs needs no isolation-capable host — every refusal happens in a validate_* hook before any OS-side call — and skips cleanly when the feature is off; it includes an over-rejection guard. Node integration adds guards because ui is reachable from plain JS even though the typed per-phase configs exclude it.

Isolation VM: 78 passed, 0 failed, 0 skipped, plus the three operator-judged interactive tests (TTY resize, streaming, interactive PowerShell with exit-code propagation). No leaked agent accounts or leftover directories, diffed against a pre-run baseline.

✅ Checklist

• Signed the Contributor License Agreement
• Updated documentation (if applicable)
• Updated Copilot instructions (if build, architecture, or conventions changed)

📋 Issue Type

• Bug fix

[azure-pipelines]

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

[Copilot]

Pull request overview

This PR tightens IsolationSession policy validation and adds safeguards against silently dropped wire fields.

Changes:

• Rejects unsupported UI/lifecycle policies and malformed state-aware fields.
• Adds parser, backend, SDK, E2E, and VM test coverage.
• Adds a CI wire-mapping coverage gate and updates documentation.

Reviewed changes

Copilot reviewed 27 out of 28 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/scripts/run_isolation_session_tests.ps1	Adds one-shot refusal tests.
tests/scripts/run_isolation_session_state_aware_tests.ps1	Adds state-aware rejection tests.
tests/configs/isolation_session_state_aware_start_rejected_process.json	Tests non-exec process rejection.
tests/configs/isolation_session_state_aware_provision_rejected_ui.json	Tests provision UI rejection.
tests/configs/isolation_session_state_aware_provision_rejected_flat_user.json	Tests flat user rejection.
tests/configs/isolation_session_one_shot_ui_rejected.json	Tests one-shot UI rejection.
tests/configs/isolation_session_one_shot_lifecycle_rejected.json	Tests lifecycle rejection.
src/testing/wxc_e2e_tests/tests/e2e_state_aware.rs	Expands parser E2E coverage.
src/testing/wxc_e2e_tests/tests/e2e_isolation_session_policy.rs	Adds policy-refusal E2E suite.
src/core/wxc_common/src/wire.rs	Clarifies IsolationSession wire fields.
src/core/wxc_common/src/state_aware_dispatch.rs	Exposes prefix resolution internally.
src/core/wxc_common/src/models.rs	Tracks explicit UI presence.
src/core/wxc_common/src/config_parser.rs	Tightens mapping and phase validation.
src/backends/isolation_session/common/src/state_aware.rs	Tests UI refusal across phases.
src/backends/isolation_session/common/src/policy.rs	Rejects unsupported UI policy.
src/backends/isolation_session/common/src/one_shot.rs	Rejects unsupported lifecycle values.
sdk/node/tests/integration/isolation-session-state-aware.test.ts	Adds SDK runtime guards.
sdk/node/src/generated/wire.ts	Regenerates wire documentation.
scripts/versioning/package.json	Registers mapping checks.
scripts/versioning/check-wire-mapping-coverage.test.js	Self-tests the mapping gate.
scripts/versioning/check-wire-mapping-coverage.js	Adds wire-field coverage analysis.
schemas/dev/mxc-config.schema.0.8.0-dev.json	Regenerates schema descriptions.
docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md	Clarifies lifecycle contract.
docs/schema.md	Documents UI and backend behavior.
docs/isolation-session/state-aware-rust.md	Expands the policy honor matrix.
docs/isolation-session/oneshot.md	Corrects one-shot behavior documentation.
.github/workflows/Versioning.Checks.Job.yml	Runs new checks in CI.
.github/copilot-instructions.md	Records architecture and validation rules.

[Copilot]

Pull request overview

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

Suppressed comments (3)

docs/isolation-session/state-aware-rust.md:221

• This now contradicts both the honor-matrix rows above and the parser behavior. convert_wire_state_aware makes process optional outside exec, but convert_wire_config still accepts and maps a present block; the non-exec backend methods simply do not consume those fields. Document this as accepted-and-ignored rather than rejected.

  rejected at non-exec state-aware phases (the parser refuses a `process`
  section on any phase other than exec).

docs/isolation-session/oneshot.md:253

• This payload is not rejected on one-shot. The permissive experimental.isolation_session wire type accepts provision/start, while one-shot conversion reads only the flat user field, so these nested blocks are silently ignored. This also conflicts with the matrix in state-aware-rust.md and the PR's stated out-of-scope behavior.

| `experimental.isolation_session.{provision,start}` | rejected — per-phase config is state-aware-only |

docs/schema.md:68

• This otherwise unchanged example line now contains an embedded carriage return before the comma, producing mixed line endings and potentially splitting the rendered code sample. Remove the stray character.

        "capabilities": ["internetClient"]
,

[Copilot]

Pull request overview

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

Suppressed comments (1)

src/backends/isolation_session/common/src/one_shot.rs:43

• This reason is inaccurate: every accepted one-shot request must carry the canonical network policy, so the backend does not reject network policy outright. The relevant distinction is that it installs no persistent filesystem or network enforcement to retain; state that instead so callers are not told their required network acknowledgment was rejected.

            "lifecycle.preservePolicy=true is not supported by the isolation session backend; \
             it rejects filesystem and network policy outright, so there is none to preserve",

[Gudge (MGudgin)]

[High · correctness] This refuses every one-shot request built through either SDK

ui_specified is set from cfg.ui.is_some(), which is correct for the wire. The problem
is upstream: both SDKs emit a ui block unconditionally, even when the caller's
SandboxPolicy has no UI policy at all.

src/core/mxc_engine/src/policy.rs:668-671 — inside the base json! literal, not behind
any if let Some(ui):

"ui": {
    "disable": !policy.ui.as_ref().map(|u| u.allow_windows).unwrap_or(false),
    "clipboard": policy.ui.as_ref().map(|u| u.clipboard).unwrap_or_default().wire(),
    "injection": policy.ui.as_ref().map(|u| u.allow_input_injection).unwrap_or(false),
},

sdk/node/src/sandbox.ts:268-272 does the same:

config.ui = {
    disable: !(policy.ui?.allowWindows ?? false),
    clipboard: policy.ui?.clipboard ?? "none",
    injection: policy.ui?.allowInputInjection ?? false,
};

So cfg.ui.is_some() is true for every SDK-built one-shot request, ui_specified
is true, and this check refuses it — including the plainest possible call with no UI
policy specified. The state-aware path escapes only because provisionSandbox builds
its own envelope without going through createConfigFromPolicy.

Reproduction — drop this into src/core/mxc_engine/src/policy.rs and run
cargo test -p mxc_engine test_isolation_session_via_mxc_engine:

#[test]
fn test_isolation_session_via_mxc_engine() {
    let policy = crate::SandboxPolicy {
        version: "0.6.0-alpha".to_string(),
        timeout_ms: None,
        network: None,
        filesystem: None,
        ui: None,
    };
    let req = crate::policy::build_request(&policy, None).unwrap();
    assert!(!req.inner.policy.ui_specified, "UI should not be specified");
}

It fails on this branch: panicked at ... UI should not be specified. (I ran it against
ada5b4cc; it is the check above that then rejects.)

The PR's own tests do not catch this because they build wire JSON directly rather than
going through the SDK policy builder, so the gap sits exactly between the two layers that
are each individually tested.

Two ways to fix, and I would suggest the second:

1. Make both SDKs omit "ui" when the caller's SandboxPolicy has none. Correct, but it
   is a behavioural change in a cross-backend code path, and any other producer that
   emits a default ui block hits this again.
2. Accept the canonical "no restriction requested" shape here, mirroring what
   validate_provision_network_policy already does for the canonical network allow. That
   keeps the refusal meaningful (a caller who genuinely asks for a UI restriction is still
   refused) while letting a default-valued block through. It also matches the precedent
   set immediately below in this same file, so the two policy checks stay symmetrical.

Whichever you pick, worth adding a regression test that goes through build_request /
createConfigFromPolicy rather than raw JSON — that is the seam this slipped through.

One related question on intent: as written this also refuses ui: { allowWindows: true },
i.e. a caller explicitly asking for no restriction. If that is deliberate, a line in the
doc comment saying so would help; if not, option 2 handles it.

[adpa-ms]

Thank you for the reproduction — running it verbatim is what made this quick to settle, and the observation underneath it is correct. The conclusion doesn't hold, though, and I want to lay out the evidence rather than just assert that.

Your repro reports two things. I added a print for the second:

ui_specified = true          <- exactly as you said
containment  = ProcessContainer

ui_specified really is true for every request built through build_wire_config — that block is unconditional, as you quoted. But the request is ProcessContainer, not IsolationSession, so reject_ui_policy is never invoked. Neither SDK can select this backend:

• Rust — SandboxPolicy has no containment field at all (version, filesystem, network, ui, timeout_ms). apply_backend hardcodes config["containment"] = json!("process"), with a #[cfg(target_os = "macos")] override to "seatbelt". There is no iso branch and no caller-facing way to ask for one.
• Node — createConfigFromPolicy sets wslc / bubblewrap / seatbelt / microvm / lxc / process, and otherwise throws Containment type '...' is not yet supported. No isolation_session branch.

So ui_specified: true on an SDK-built request is inert: it reaches ProcessContainer, which honors ui. IsolationSession one-shot is reachable only from hand-written JSON, where a ui block exists only if its author wrote one.

On your option 2 — I think it's unsound, and the reason is worth recording. With SandboxPolicy.ui = None, the block that gets emitted is disable:true, clipboard:none, injection:false — full lockdown. Accepting that as a canonical "no restriction requested" shape would mean accepting precisely the assertion this backend cannot honor. It isn't symmetric with the network gate either: the canonical network acknowledgment is a true statement about the container (the network really is unrestricted), whereas there is no true ui statement available here — disable:false asserts a GUI the user can drive, and every clipboard value asserts a relationship to the user's clipboard the sandbox cannot reach. Only injection:false is honest, and it can't be supplied alone. That reasoning is now written up in docs/isolation-session/state-aware-rust.md.

If iso ever does become SDK-reachable, your option 1 is the right fix — omit ui when the caller's policy has none.

On your last question — yes, refusing ui: { allowWindows: true } is deliberate. It maps to disable: false, which asserts the sandbox may drive a GUI the user can see; in a separate session the windows are real but unreachable and invisible. The docs now say so explicitly.

What I'm not doing, and why. The latent risk you've identified is real: the day someone adds an iso branch to either SDK, this becomes a live bug with no guard. I'd normally take your suggestion of a build_request-level regression test — but the invariant to pin lives in mxc_engine, a crate this PR doesn't touch, and both the unconditional ui emission and the missing iso branch are pre-existing. This PR has already been rescoped twice to keep it off cross-backend code, so I've recorded it as a tracked follow-up instead. The current change provably alters nothing on that path: the validator is unreachable from it either way.

[Gudge (MGudgin)]

[Medium · cross-platform-parity] The flag is cross-platform; the fix is Windows-only

The doc comment here is genuinely good — it explains why a presence bit is needed
better than most such comments do, and the network_specified symmetry is the right
model.

The observation is about blast radius rather than this code: ui_specified lands in the
shared ContainerPolicy, but only IsolationSession reads it. LXC and Bubblewrap neither
honour policy.ui nor refuse it, so on Linux the exact defect this PR fixes — a caller
supplies a ui policy, believes a security control is applied, and silently receives
none of it — is still present. Same for Seatbelt on macOS.

That is a reasonable scope call for one PR and I am not asking you to widen it here. Two
small things that would keep it from being forgotten:

• A tracking issue for the Linux/macOS backends, referenced from this doc comment, so the
  next person reading the field knows the story is incomplete rather than assuming the
  flag is universally honoured.
• One line in the comment noting it is currently consumed only by IsolationSession —
  otherwise the phrase "Used by backends (e.g. IsolationSession)" reads as though several
  already do.

Worth deciding explicitly, because "supported everywhere except silently ignored on two
platforms" is the failure mode this PR exists to eliminate.

[adpa-ms]

Agreed, and the "e.g." was doing misleading work — it implies several backends consume this when exactly one does. Fixed in 878af37: the comment now states plainly that it's consumed only by IsolationSession today, names the backends that accept-and-ignore policy.ui (LXC, Bubblewrap, Seatbelt, Windows Sandbox), and spells out the consequence — the flag being set does not mean a UI policy was honored anywhere, only that the caller supplied one.

On the wider gap — you're right that on Linux and macOS the exact defect this PR fixes is still present, and I agree it shouldn't be silently forgotten. It's tracked as a follow-up alongside the Windows Sandbox case, which has the same shape (and whose row in the cross-backend design doc currently claims it rejects ui, which is wrong — also tracked).

I'd push back gently on one framing, though: "supported everywhere except silently ignored on two platforms" isn't quite the state. policy.ui was never honored by those backends before this PR either — this change doesn't extend a guarantee unevenly, it makes one backend stop pretending. The unevenness is pre-existing and this PR narrows it by one.

[Gudge (MGudgin)]

[Medium · testability] These refusal tests do not need a host, but are gated as if they do

The comment a few lines above says it explicitly — "Validation runs before the OS service
is touched, so nothing is provisioned and no cleanup is needed" — and that is exactly
right: every refusal in this PR happens in a validate_* hook before any OS-side call.

But these it blocks sit inside the describe that carries { skip: skipReason }, where
skipReason resolves via sandboxSkipReason / probeStateAwareRuntime. On any CI agent
without an isolation-capable host, the whole suite skips — including these, which had no
need of one. The net effect is that the SDK-facing half of this PR's central behaviour has
no automated gate on ordinary runs, which is a shame given the tests themselves are well
targeted (the presence-vs-value case on line 235 in particular is the right test).

Suggested: split the host-independent refusal tests into their own describe with no
skip condition, and leave the genuine lifecycle tests behind the probe. Something like:

describe('IsolationSession policy refusals (no host required)', () => {
  it('backend refuses a provision that supplies a ui policy', async () => { ... });
  it('backend refuses a lockdown-equivalent ui policy too (presence, not value)', async () => { ... });
});

They still need wxc-exec built with --features isolation_session, so if the default
SDK integration job does not build with it, that is the other half of making this run.

[adpa-ms]

The observation is correct and the goal is right, but I'd like to defer this, with evidence that nothing is currently ungated.

These refusals do have an automated CI gate today — src/testing/wxc_e2e_tests/tests/e2e_isolation_session_policy.rs asserts the same ui refusals on both surfaces, needs no isolation-capable host for exactly the reason you quoted, and runs in the normal cargo test path. Build.Windows.Job.yml builds with --features "hyperlight isolation_session microvm wslc", so it executes on every PR. The Node skip is therefore a redundancy gap, not a coverage gap.

Two scope reasons for not restructuring here. The describe predates this PR and already contains three network refusal tests from #682 with the identical property; my ui tests followed the file's established pattern. Splitting it means relocating those too, which reaches into another PR's tests. And as you note yourself, the tests still need wxc-exec built with --features isolation_session — so the describe split alone doesn't achieve the goal; the SDK integration job would need a build change as well. That's a CI change I'd rather make deliberately than as a rider.

Worth flagging one tension for whoever picks this up: this and the proportionality comment on e2e_isolation_session_policy.rs point in opposite directions — un-skip more here, drop layers there. I'd want both resolved together rather than separately.

[Gudge (MGudgin)]

[Medium · proportionality] Four harnesses now cover the same pre-service validation

To be clear about what this file does well: it is careful work, the skip helper is honest
about why it skipped, and the over-rejection guard (asserting an absent ui is still
accepted) is the test I most wanted to see in this PR — that one earns its place.

The concern is the aggregate. The same refusals are now asserted in the backend unit
tests, in this 195-line E2E file, in the Node integration suite, and via three
tests/configs/*.json fixtures wired into two PowerShell runners. These are deterministic
validator checks that fail before any OS-side call, so the extra layers are not buying
coverage of different failure modes — they mostly re-verify the same if statements
through progressively more expensive transports, and each one is a place a future
signature change has to be updated.

Suggested: keep the unit tests plus one process-boundary case per surface (one-shot and
state-aware) to prove the refusal survives serialization and reaches the exit code, and
drop the rest. That preserves what the E2E layer uniquely proves without carrying four
parallel copies of the same matrix.

Not a blocker, and I would rather have this than too little — but worth a look before it
becomes the pattern the next policy refusal is expected to follow.

---

[Low · proportionality] Unrelated doc corrections are bundled in

Separately: docs/isolation-session/oneshot.md:159-166 and neighbouring hunks correct
pre-existing text about stdio/ConPTY, backend file locations, and teardown behaviour that
is unrelated to the ui / lifecycle refusals this PR is about. They look correct, and I
am glad someone is fixing them — but roughly 100 lines of unrelated doc churn sits between
a reviewer and the two-line behaviour change, which is a shame for a PR whose actual risk
is concentrated in a single if. A separate docs PR would land instantly and leave this
one easy to read.

[adpa-ms]

Fair challenge, and the maintenance-cost point is real — four places to update on a signature change is a genuine cost. I'd like to keep them, but on narrower grounds than "more is better", because you're right that they're not buying different failure modes.

What each layer uniquely proves:

• Unit — the validator logic, including precedence (filesystem -> ui -> network) and the presence-vs-value distinction. Cheapest, and where the real matrix lives.
• This E2E file — that the refusal survives serialization and reaches a process exit code with the designed error.code. Your point stands that it re-tests the same if, but it's the only layer that proves the wire envelope is well-formed.
• Node — that ui is reachable from plain JS despite the typed per-phase Config types excluding it. That's a different failure mode: a TypeScript-only guard would be vacuous, since the field can't be expressed in the typed API at all.
• VM fixtures — that the shipped binary on the target OS refuses, catching feature-gate and packaging mistakes the host tests can't see.

That said, the over-rejection guard you singled out is the one I'd defend hardest too, and if the aggregate needs trimming later, this file's redundant positive cases are where I'd start rather than the guard.

One piece of context: three-tier coverage (Rust unit, Rust integration, Node integration) is a standing expectation on this repo, so trimming below it is a convention change rather than a local call — better made deliberately than in a policy PR.

---

On the bundled doc corrections raised in the same comment — accepted as a fair criticism, and I'd rather acknowledge it than argue.

The reason they're here: correcting claims that no longer matched the code was part of the stated scope — the first commit is titled exactly that, and several of those claims were about the same ui/lifecycle surface (oneshot.md listed lifecycle.destroyOnExit as "Implemented" while the code ignored it, which is the defect this PR fixes, described in prose). Splitting cleanly at that boundary is harder than it looks from the diff.

That said, the stdio/ConPTY and file-location corrections genuinely aren't that, and you're right that ~100 lines of unrelated churn between a reviewer and a two-line behaviour change is a poor trade. Extracting them now would mean a second PR plus a full re-validation (this branch gates on an isolation-capable VM run, automated plus operator-judged) for text already validated — a real cost for a change that's already reviewed. I'd rather carry the lesson forward than re-cut it at this point, but say the word if you'd prefer the split and I'll do it.