feat(isolation-session): carry an optional appId in a structured sandboxId - #746
Merged
adpa-ms merged 5 commits intoAug 5, 2026
Conversation
added 4 commits
August 4, 2026 14:26
The one-shot IsolationSession path takes no configuration. Its only field, `user`, existed solely because the state-aware `StartConfig` reused the one-shot domain struct -- so one-shot had to reject its own struct's only field at runtime. That rejection was guard code compensating for the deliberately permissive `experimental` block (no `deny_unknown_fields`). Such guards are scaffolding that graduation to the closed stable surface deletes anyway, so ignoring is the correct behaviour and matches every other unrecognised key there. - wire `IsolationSession` loses `user`; it now carries only the state-aware `provision` / `start` nesting. - domain `IsolationSessionConfig` becomes `IsolationSessionStartConfig`, which is what it always was in practice. - `ExperimentalConfig.isolation_session` is deleted outright. Nothing is lost: the multi-backend conflict check reads the *wire* struct (`present_backend_sections` takes `&wire::MxcConfig`), which survives. Caller-visible behaviour change: `experimental.isolation_session.user` on a one-shot request changes from a loud error to being silently ignored. It is unreachable from the typed Node SDK, whose one-shot `ContainerConfig.experimental` exposes only `wslc` and `telemetry`. Tests: the deleted rejection tests are replaced, not dropped -- one asserting the field is now accepted and ignored, and one pinning that a lone `experimental.isolation_session` section still marks a configured backend so the conflict check cannot silently regress. Schema and SDK wire types regenerated (not hand-edited).
…t the OS call `validate_isolation_session_user` trimmed the UPN before its shape check, but provision and start handed the OS the untrimmed value. A padded UPN such as " alice@contoso.com " therefore passed validation and reached the OS with its surrounding spaces intact -- validation and transmission disagreed about what the accepted value was. Extract `os_credentials`, which produces the exact (entraAccountName, wamToken) pair given to the OS, and apply the trim there so the two agree. An absent bundle maps to the local-agent empty pair. The WAM token is deliberately NOT trimmed: it is an opaque bearer credential and trimming could corrupt it. The helper exists because the previous inline `match` offered no seam -- the behaviour could not be asserted without a live OS service. It is now covered by unit tests for the trim, the verbatim token, the absent bundle, and the interior-whitespace case.
`wire::IsolationSessionPhase` was shared by provision and start, so the generated schema advertised every per-phase field on both phases regardless of which one actually accepts it. The domain configs and the Node SDK types were already split per phase; only the wire model pooled them. Replace it with `IsolationSessionProvisionPhase` and `IsolationSessionStartPhase`. The JSON keys (`provision`, `start`, `user`) are unchanged, so this is invisible on the wire -- it only makes the generated schema and SDK wire types state truthfully where each field is legal. The SDK conformance oracle is now per-phase rather than a single pooled key set, which is strictly stronger: a field legal only on provision can no longer satisfy it by appearing on the start config. The phases whose Rust associated type is `()` are asserted to expose no backend-specific field at all. Also add a non-vacuity guard to that oracle. Every assertion is of the form `Exclude<A, B> extends never`, which passes trivially if `A` resolves to `never` -- so a mistake in the derivation would have silently disabled the check instead of failing it. The derived key sets are now pinned to their expected contents. Schema and SDK wire types regenerated (not hand-edited).
Accept an optional `appId` on the state-aware provision phase -- the Package
Family Name for a packaged application, any string for an unpackaged one --
and carry it inside the returned `sandboxId`.
Motivation: future OS API changes will act on the calling application's PFN,
and those calls are expected to be spread across lifecycle phases. It is not
guaranteed that the OS will propagate a PFN supplied at provision to a
session's other calls. MXC holds no cross-phase state (each phase is a fresh
process), so the only carrier that survives without the caller re-supplying the
value on every phase is the sandboxId itself. Embedding works whether or not
the OS ends up retaining it.
Nothing consumes appId yet. It is accepted, encoded, and decoded back into an
internal struct, deliberately exposed nowhere -- scaffolding for a future OS
contract, so adopting it later is not a breaking change.
New id format, replacing the plaintext `iso:<agentUserName>` tail:
iso:<base64url-nopad( JSON object )>
with v1 keys `version`, `agentUserName`, and optional `appId`. Encoded rather
than delimited because the parser must know which fields are present without
assuming anything about separator characters: the agentUserName is OS-assigned
with no charset guarantee, so a delimited form would mis-parse a name
containing the delimiter *silently*. The base64url alphabet makes that entire
class of bug unrepresentable rather than merely prevented.
The envelope is frozen (always base64url of a JSON object; all evolution
happens as keys inside). The version gate is one-directional -- a payload from
a newer MXC is rejected with a message that says so, since the remediation is
"upgrade MXC", not "this id is corrupt" -- and is bumped only for changes an
old reader must not silently mishandle. Unknown keys are ignored.
appId validation is structural only (no control characters, at most 256
characters). MXC is a pass-through carrier and does not judge what a valid
application identity looks like; a PFN grammar check would risk rejecting forms
a future OS API accepts. The value is preserved verbatim, and an explicitly
empty string is a value distinct from absent -- a future OS API may assign it
meaning, so MXC neither collapses the two nor ever synthesizes an empty string
the caller did not send.
Legacy plaintext ids no longer decode and surface as `malformed_id`. Intended:
they refer to OS resources that do not survive the change either.
Tests: exhaustive codec unit tests (round-trips, empty-vs-absent distinctness,
verbatim preservation, hostile agent-user names containing colons and path
separators, determinism, the alphabet property, every decode failure mode, the
version gate); provision-hook validation tests; SDK type and envelope tests
including a compile-time assertion that appId is rejected at start; and E2E
coverage for the round-trip, the empty case, both rejections, legacy ids, and
newer-version ids.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Five rounds of review against the four preceding commits. Grouped by what
they fix rather than by the round they surfaced in.
Correctness -- the id codec
- Restore the non-empty agentUserName invariant. The base format guaranteed
it structurally (`!rest.is_empty()` applied to the tail, which WAS the
name); the rewrite applied that check to the base64 tail, catching only a
bare `iso:`. {"version":1,"agentUserName":""} decoded cleanly and handed an
empty string to the OS lifecycle calls, which answer "not found" --
surfacing as stale_id ("re-provision") for a request that was never
well-formed.
- Re-validate appId on decode. sandboxId is caller-supplied on every
post-provision phase, so provision is not the only way a value arrives; the
guarantee now holds by value rather than by provenance. Mapped to
malformed_id, NOT policy_validation: every other decode failure is
malformed_id, a bad id is an id problem, and the phases that consume an id
accept no policy for a policy error to belong to.
- Decode the id in validate_exec / validate_stop / validate_deprovision.
Previously only validate_start decoded, so --dry-run (which stops after
validation) reported success for ids the real call rejects. The asymmetry
is pre-existing -- all three hooks ignored the id at base -- but this change
widens the class of ids that fail, so it is closed here.
Documentation -- four false or incomplete claims
- The legacy-id justification was wrong three times in succession, each
correction exposing the next. It is not true that the referenced resources
do not survive the change: the agent user account persists until explicit
deprovision. It is not true that the session does not outlive the binary:
outliving the process is the premise of the state-aware lifecycle, and
nothing in MXC stops a session when the binary is replaced. It is not true
that such a sandbox becomes unaddressable through MXC: decode binds nothing
to the minting binary, so re-encoding the old agent user name -- which a
legacy id carries in the clear -- yields a valid id for the same sandbox,
making recovery unconditional rather than contingent on having recorded
anything. Also corrected: a session ends at deprovision too, since removing
the agent user terminates any session still running under it.
- A doc this change edits still claimed one-shot rejects
experimental.isolation_session.user; the correction had been applied at one
location and missed at the parallel statement 200 lines later.
- The appId JSDoc promised `null` as a spelling of absent on a `string`
property, so a caller following it got a compile error. Claim removed; the
wire-level behaviour is unchanged and documented where it applies.
- state-aware-typescript.md enumerated the provision config but omitted appId.
Tests -- the recurring defect, and the rule that ends it
Four rounds surfaced the same class of flaw: a test that exercises the fixed
path without discriminating it from the fix's absence. Each would have passed
unchanged against the pre-fix head.
- A case titled "every id-consuming phase rejects a legacy id" never issued a
dry run -- the harness had no dry-run parameter at all. Added -DryRun to
Invoke-StateAware and exercised each phase both ways, plus the missing
counterpart: a well-formed id must be ACCEPTED by --dry-run on every phase,
or the agreement would be satisfied trivially by refusing everything.
- The crafted-appId case spliced a raw U+0007 into the JSON text, which RFC
8259 forbids inside a string, so serde_json rejected it at parse time and
the payload never reached validate_app_id. Written as \u0007 so the document
is valid and the control character survives into the decoded string, and the
message is asserted to name `appId` -- which is what distinguishes
validate_app_id running from the parser refusing. An oversized-appId case is
added, having no JSON-level analogue and so unable to pass for the wrong
reason.
- The dry-run tests asserted only exit codes, which are identical either way.
They now assert the result envelope, and the exec command prints a marker
and exits 1 so a dropped flag is caught three independent ways.
- That envelope assertion in turn overclaimed: start/stop/deprovision return
metadata: None, rendered as the same {"result":{}} the dry-run
short-circuit produces, so it discriminates nothing for those three. The
comment is scoped to what it proves, and the real observable is added where
it can live -- wxc_common's dispatch tests, whose call-counting StubBackend
already pinned dry-run for provision and exec. start, stop and deprovision
were simply missing. Dry-run skipping is now pinned for all five phases at
the layer where dry_run actually lives, in Rust tests that run in the local
review loop rather than only on the VM.
Verified by mutation rather than by reasoning: making the Start arm ignore
dry_run fails dispatch_start_dry_run_skips_start_call_but_runs_validate, and
only that test. The rule going forward is to make the fix's absence produce a
failure and then observe it.
Harness -- a leak inside the leak discipline
The positive dry-run case provisions a real sandbox, and Run-StateAwareTest
swallows throws while the suite's finally reclaims only $script:sandboxId,
which that case never set. A throw between provision and cleanup therefore
leaked an Indefinite-lifetime agent user outside the harness's own leak
discipline -- the discipline cited as evidence elsewhere in this review. The
id is now reclaimed in a dedicated try/finally, and the cleanup's exit code is
asserted rather than discarded.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0c82ca12-a566-4257-9de7-164be299375e
adpa-ms
force-pushed
the
user/adibpa/copilot-appid-sandboxid
branch
from
August 5, 2026 04:15
079ab8b to
0a222a1
Compare
adpa-ms
added a commit
that referenced
this pull request
Aug 5, 2026
…boxId (#746) * refactor(isolation-session): remove the one-shot backend config surface The one-shot IsolationSession path takes no configuration. Its only field, `user`, existed solely because the state-aware `StartConfig` reused the one-shot domain struct -- so one-shot had to reject its own struct's only field at runtime. That rejection was guard code compensating for the deliberately permissive `experimental` block (no `deny_unknown_fields`). Such guards are scaffolding that graduation to the closed stable surface deletes anyway, so ignoring is the correct behaviour and matches every other unrecognised key there. - wire `IsolationSession` loses `user`; it now carries only the state-aware `provision` / `start` nesting. - domain `IsolationSessionConfig` becomes `IsolationSessionStartConfig`, which is what it always was in practice. - `ExperimentalConfig.isolation_session` is deleted outright. Nothing is lost: the multi-backend conflict check reads the *wire* struct (`present_backend_sections` takes `&wire::MxcConfig`), which survives. Caller-visible behaviour change: `experimental.isolation_session.user` on a one-shot request changes from a loud error to being silently ignored. It is unreachable from the typed Node SDK, whose one-shot `ContainerConfig.experimental` exposes only `wslc` and `telemetry`. Tests: the deleted rejection tests are replaced, not dropped -- one asserting the field is now accepted and ignored, and one pinning that a lone `experimental.isolation_session` section still marks a configured backend so the conflict check cannot silently regress. Schema and SDK wire types regenerated (not hand-edited). * fix(isolation-session): trim the UPN consistently at validation and at the OS call `validate_isolation_session_user` trimmed the UPN before its shape check, but provision and start handed the OS the untrimmed value. A padded UPN such as " alice@contoso.com " therefore passed validation and reached the OS with its surrounding spaces intact -- validation and transmission disagreed about what the accepted value was. Extract `os_credentials`, which produces the exact (entraAccountName, wamToken) pair given to the OS, and apply the trim there so the two agree. An absent bundle maps to the local-agent empty pair. The WAM token is deliberately NOT trimmed: it is an opaque bearer credential and trimming could corrupt it. The helper exists because the previous inline `match` offered no seam -- the behaviour could not be asserted without a live OS service. It is now covered by unit tests for the trim, the verbatim token, the absent bundle, and the interior-whitespace case. * refactor(isolation-session): split the wire phase type per phase `wire::IsolationSessionPhase` was shared by provision and start, so the generated schema advertised every per-phase field on both phases regardless of which one actually accepts it. The domain configs and the Node SDK types were already split per phase; only the wire model pooled them. Replace it with `IsolationSessionProvisionPhase` and `IsolationSessionStartPhase`. The JSON keys (`provision`, `start`, `user`) are unchanged, so this is invisible on the wire -- it only makes the generated schema and SDK wire types state truthfully where each field is legal. The SDK conformance oracle is now per-phase rather than a single pooled key set, which is strictly stronger: a field legal only on provision can no longer satisfy it by appearing on the start config. The phases whose Rust associated type is `()` are asserted to expose no backend-specific field at all. Also add a non-vacuity guard to that oracle. Every assertion is of the form `Exclude<A, B> extends never`, which passes trivially if `A` resolves to `never` -- so a mistake in the derivation would have silently disabled the check instead of failing it. The derived key sets are now pinned to their expected contents. Schema and SDK wire types regenerated (not hand-edited). * feat(isolation-session): carry an optional appId inside the sandboxId Accept an optional `appId` on the state-aware provision phase -- the Package Family Name for a packaged application, any string for an unpackaged one -- and carry it inside the returned `sandboxId`. Motivation: future OS API changes will act on the calling application's PFN, and those calls are expected to be spread across lifecycle phases. It is not guaranteed that the OS will propagate a PFN supplied at provision to a session's other calls. MXC holds no cross-phase state (each phase is a fresh process), so the only carrier that survives without the caller re-supplying the value on every phase is the sandboxId itself. Embedding works whether or not the OS ends up retaining it. Nothing consumes appId yet. It is accepted, encoded, and decoded back into an internal struct, deliberately exposed nowhere -- scaffolding for a future OS contract, so adopting it later is not a breaking change. New id format, replacing the plaintext `iso:<agentUserName>` tail: iso:<base64url-nopad( JSON object )> with v1 keys `version`, `agentUserName`, and optional `appId`. Encoded rather than delimited because the parser must know which fields are present without assuming anything about separator characters: the agentUserName is OS-assigned with no charset guarantee, so a delimited form would mis-parse a name containing the delimiter *silently*. The base64url alphabet makes that entire class of bug unrepresentable rather than merely prevented. The envelope is frozen (always base64url of a JSON object; all evolution happens as keys inside). The version gate is one-directional -- a payload from a newer MXC is rejected with a message that says so, since the remediation is "upgrade MXC", not "this id is corrupt" -- and is bumped only for changes an old reader must not silently mishandle. Unknown keys are ignored. appId validation is structural only (no control characters, at most 256 characters). MXC is a pass-through carrier and does not judge what a valid application identity looks like; a PFN grammar check would risk rejecting forms a future OS API accepts. The value is preserved verbatim, and an explicitly empty string is a value distinct from absent -- a future OS API may assign it meaning, so MXC neither collapses the two nor ever synthesizes an empty string the caller did not send. Legacy plaintext ids no longer decode and surface as `malformed_id`. Intended: they refer to OS resources that do not survive the change either. Tests: exhaustive codec unit tests (round-trips, empty-vs-absent distinctness, verbatim preservation, hostile agent-user names containing colons and path separators, determinism, the alphabet property, every decode failure mode, the version gate); provision-hook validation tests; SDK type and envelope tests including a compile-time assertion that appId is rejected at start; and E2E coverage for the round-trip, the empty case, both rejections, legacy ids, and newer-version ids. * fix(isolation-session): address review findings on appId/sandboxId Five rounds of review against the four preceding commits. Grouped by what they fix rather than by the round they surfaced in. Correctness -- the id codec - Restore the non-empty agentUserName invariant. The base format guaranteed it structurally (`!rest.is_empty()` applied to the tail, which WAS the name); the rewrite applied that check to the base64 tail, catching only a bare `iso:`. {"version":1,"agentUserName":""} decoded cleanly and handed an empty string to the OS lifecycle calls, which answer "not found" -- surfacing as stale_id ("re-provision") for a request that was never well-formed. - Re-validate appId on decode. sandboxId is caller-supplied on every post-provision phase, so provision is not the only way a value arrives; the guarantee now holds by value rather than by provenance. Mapped to malformed_id, NOT policy_validation: every other decode failure is malformed_id, a bad id is an id problem, and the phases that consume an id accept no policy for a policy error to belong to. - Decode the id in validate_exec / validate_stop / validate_deprovision. Previously only validate_start decoded, so --dry-run (which stops after validation) reported success for ids the real call rejects. The asymmetry is pre-existing -- all three hooks ignored the id at base -- but this change widens the class of ids that fail, so it is closed here. Documentation -- four false or incomplete claims - The legacy-id justification was wrong three times in succession, each correction exposing the next. It is not true that the referenced resources do not survive the change: the agent user account persists until explicit deprovision. It is not true that the session does not outlive the binary: outliving the process is the premise of the state-aware lifecycle, and nothing in MXC stops a session when the binary is replaced. It is not true that such a sandbox becomes unaddressable through MXC: decode binds nothing to the minting binary, so re-encoding the old agent user name -- which a legacy id carries in the clear -- yields a valid id for the same sandbox, making recovery unconditional rather than contingent on having recorded anything. Also corrected: a session ends at deprovision too, since removing the agent user terminates any session still running under it. - A doc this change edits still claimed one-shot rejects experimental.isolation_session.user; the correction had been applied at one location and missed at the parallel statement 200 lines later. - The appId JSDoc promised `null` as a spelling of absent on a `string` property, so a caller following it got a compile error. Claim removed; the wire-level behaviour is unchanged and documented where it applies. - state-aware-typescript.md enumerated the provision config but omitted appId. Tests -- the recurring defect, and the rule that ends it Four rounds surfaced the same class of flaw: a test that exercises the fixed path without discriminating it from the fix's absence. Each would have passed unchanged against the pre-fix head. - A case titled "every id-consuming phase rejects a legacy id" never issued a dry run -- the harness had no dry-run parameter at all. Added -DryRun to Invoke-StateAware and exercised each phase both ways, plus the missing counterpart: a well-formed id must be ACCEPTED by --dry-run on every phase, or the agreement would be satisfied trivially by refusing everything. - The crafted-appId case spliced a raw U+0007 into the JSON text, which RFC 8259 forbids inside a string, so serde_json rejected it at parse time and the payload never reached validate_app_id. Written as \u0007 so the document is valid and the control character survives into the decoded string, and the message is asserted to name `appId` -- which is what distinguishes validate_app_id running from the parser refusing. An oversized-appId case is added, having no JSON-level analogue and so unable to pass for the wrong reason. - The dry-run tests asserted only exit codes, which are identical either way. They now assert the result envelope, and the exec command prints a marker and exits 1 so a dropped flag is caught three independent ways. - That envelope assertion in turn overclaimed: start/stop/deprovision return metadata: None, rendered as the same {"result":{}} the dry-run short-circuit produces, so it discriminates nothing for those three. The comment is scoped to what it proves, and the real observable is added where it can live -- wxc_common's dispatch tests, whose call-counting StubBackend already pinned dry-run for provision and exec. start, stop and deprovision were simply missing. Dry-run skipping is now pinned for all five phases at the layer where dry_run actually lives, in Rust tests that run in the local review loop rather than only on the VM. Verified by mutation rather than by reasoning: making the Start arm ignore dry_run fails dispatch_start_dry_run_skips_start_call_but_runs_validate, and only that test. The rule going forward is to make the fix's absence produce a failure and then observe it. Harness -- a leak inside the leak discipline The positive dry-run case provisions a real sandbox, and Run-StateAwareTest swallows throws while the suite's finally reclaims only $script:sandboxId, which that case never set. A throw between provision and cleanup therefore leaked an Indefinite-lifetime agent user outside the harness's own leak discipline -- the discipline cited as evidence elsewhere in this review. The id is now reclaimed in a dedicated try/finally, and the cleanup's exit code is asserted rather than discarded. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0c82ca12-a566-4257-9de7-164be299375e --------- Co-authored-by: adpa-ms <> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0c82ca12-a566-4257-9de7-164be299375e
adpa-ms
added a commit
that referenced
this pull request
Aug 6, 2026
…boxId (#746) * refactor(isolation-session): remove the one-shot backend config surface The one-shot IsolationSession path takes no configuration. Its only field, `user`, existed solely because the state-aware `StartConfig` reused the one-shot domain struct -- so one-shot had to reject its own struct's only field at runtime. That rejection was guard code compensating for the deliberately permissive `experimental` block (no `deny_unknown_fields`). Such guards are scaffolding that graduation to the closed stable surface deletes anyway, so ignoring is the correct behaviour and matches every other unrecognised key there. - wire `IsolationSession` loses `user`; it now carries only the state-aware `provision` / `start` nesting. - domain `IsolationSessionConfig` becomes `IsolationSessionStartConfig`, which is what it always was in practice. - `ExperimentalConfig.isolation_session` is deleted outright. Nothing is lost: the multi-backend conflict check reads the *wire* struct (`present_backend_sections` takes `&wire::MxcConfig`), which survives. Caller-visible behaviour change: `experimental.isolation_session.user` on a one-shot request changes from a loud error to being silently ignored. It is unreachable from the typed Node SDK, whose one-shot `ContainerConfig.experimental` exposes only `wslc` and `telemetry`. Tests: the deleted rejection tests are replaced, not dropped -- one asserting the field is now accepted and ignored, and one pinning that a lone `experimental.isolation_session` section still marks a configured backend so the conflict check cannot silently regress. Schema and SDK wire types regenerated (not hand-edited). * fix(isolation-session): trim the UPN consistently at validation and at the OS call `validate_isolation_session_user` trimmed the UPN before its shape check, but provision and start handed the OS the untrimmed value. A padded UPN such as " alice@contoso.com " therefore passed validation and reached the OS with its surrounding spaces intact -- validation and transmission disagreed about what the accepted value was. Extract `os_credentials`, which produces the exact (entraAccountName, wamToken) pair given to the OS, and apply the trim there so the two agree. An absent bundle maps to the local-agent empty pair. The WAM token is deliberately NOT trimmed: it is an opaque bearer credential and trimming could corrupt it. The helper exists because the previous inline `match` offered no seam -- the behaviour could not be asserted without a live OS service. It is now covered by unit tests for the trim, the verbatim token, the absent bundle, and the interior-whitespace case. * refactor(isolation-session): split the wire phase type per phase `wire::IsolationSessionPhase` was shared by provision and start, so the generated schema advertised every per-phase field on both phases regardless of which one actually accepts it. The domain configs and the Node SDK types were already split per phase; only the wire model pooled them. Replace it with `IsolationSessionProvisionPhase` and `IsolationSessionStartPhase`. The JSON keys (`provision`, `start`, `user`) are unchanged, so this is invisible on the wire -- it only makes the generated schema and SDK wire types state truthfully where each field is legal. The SDK conformance oracle is now per-phase rather than a single pooled key set, which is strictly stronger: a field legal only on provision can no longer satisfy it by appearing on the start config. The phases whose Rust associated type is `()` are asserted to expose no backend-specific field at all. Also add a non-vacuity guard to that oracle. Every assertion is of the form `Exclude<A, B> extends never`, which passes trivially if `A` resolves to `never` -- so a mistake in the derivation would have silently disabled the check instead of failing it. The derived key sets are now pinned to their expected contents. Schema and SDK wire types regenerated (not hand-edited). * feat(isolation-session): carry an optional appId inside the sandboxId Accept an optional `appId` on the state-aware provision phase -- the Package Family Name for a packaged application, any string for an unpackaged one -- and carry it inside the returned `sandboxId`. Motivation: future OS API changes will act on the calling application's PFN, and those calls are expected to be spread across lifecycle phases. It is not guaranteed that the OS will propagate a PFN supplied at provision to a session's other calls. MXC holds no cross-phase state (each phase is a fresh process), so the only carrier that survives without the caller re-supplying the value on every phase is the sandboxId itself. Embedding works whether or not the OS ends up retaining it. Nothing consumes appId yet. It is accepted, encoded, and decoded back into an internal struct, deliberately exposed nowhere -- scaffolding for a future OS contract, so adopting it later is not a breaking change. New id format, replacing the plaintext `iso:<agentUserName>` tail: iso:<base64url-nopad( JSON object )> with v1 keys `version`, `agentUserName`, and optional `appId`. Encoded rather than delimited because the parser must know which fields are present without assuming anything about separator characters: the agentUserName is OS-assigned with no charset guarantee, so a delimited form would mis-parse a name containing the delimiter *silently*. The base64url alphabet makes that entire class of bug unrepresentable rather than merely prevented. The envelope is frozen (always base64url of a JSON object; all evolution happens as keys inside). The version gate is one-directional -- a payload from a newer MXC is rejected with a message that says so, since the remediation is "upgrade MXC", not "this id is corrupt" -- and is bumped only for changes an old reader must not silently mishandle. Unknown keys are ignored. appId validation is structural only (no control characters, at most 256 characters). MXC is a pass-through carrier and does not judge what a valid application identity looks like; a PFN grammar check would risk rejecting forms a future OS API accepts. The value is preserved verbatim, and an explicitly empty string is a value distinct from absent -- a future OS API may assign it meaning, so MXC neither collapses the two nor ever synthesizes an empty string the caller did not send. Legacy plaintext ids no longer decode and surface as `malformed_id`. Intended: they refer to OS resources that do not survive the change either. Tests: exhaustive codec unit tests (round-trips, empty-vs-absent distinctness, verbatim preservation, hostile agent-user names containing colons and path separators, determinism, the alphabet property, every decode failure mode, the version gate); provision-hook validation tests; SDK type and envelope tests including a compile-time assertion that appId is rejected at start; and E2E coverage for the round-trip, the empty case, both rejections, legacy ids, and newer-version ids. * fix(isolation-session): address review findings on appId/sandboxId Five rounds of review against the four preceding commits. Grouped by what they fix rather than by the round they surfaced in. Correctness -- the id codec - Restore the non-empty agentUserName invariant. The base format guaranteed it structurally (`!rest.is_empty()` applied to the tail, which WAS the name); the rewrite applied that check to the base64 tail, catching only a bare `iso:`. {"version":1,"agentUserName":""} decoded cleanly and handed an empty string to the OS lifecycle calls, which answer "not found" -- surfacing as stale_id ("re-provision") for a request that was never well-formed. - Re-validate appId on decode. sandboxId is caller-supplied on every post-provision phase, so provision is not the only way a value arrives; the guarantee now holds by value rather than by provenance. Mapped to malformed_id, NOT policy_validation: every other decode failure is malformed_id, a bad id is an id problem, and the phases that consume an id accept no policy for a policy error to belong to. - Decode the id in validate_exec / validate_stop / validate_deprovision. Previously only validate_start decoded, so --dry-run (which stops after validation) reported success for ids the real call rejects. The asymmetry is pre-existing -- all three hooks ignored the id at base -- but this change widens the class of ids that fail, so it is closed here. Documentation -- four false or incomplete claims - The legacy-id justification was wrong three times in succession, each correction exposing the next. It is not true that the referenced resources do not survive the change: the agent user account persists until explicit deprovision. It is not true that the session does not outlive the binary: outliving the process is the premise of the state-aware lifecycle, and nothing in MXC stops a session when the binary is replaced. It is not true that such a sandbox becomes unaddressable through MXC: decode binds nothing to the minting binary, so re-encoding the old agent user name -- which a legacy id carries in the clear -- yields a valid id for the same sandbox, making recovery unconditional rather than contingent on having recorded anything. Also corrected: a session ends at deprovision too, since removing the agent user terminates any session still running under it. - A doc this change edits still claimed one-shot rejects experimental.isolation_session.user; the correction had been applied at one location and missed at the parallel statement 200 lines later. - The appId JSDoc promised `null` as a spelling of absent on a `string` property, so a caller following it got a compile error. Claim removed; the wire-level behaviour is unchanged and documented where it applies. - state-aware-typescript.md enumerated the provision config but omitted appId. Tests -- the recurring defect, and the rule that ends it Four rounds surfaced the same class of flaw: a test that exercises the fixed path without discriminating it from the fix's absence. Each would have passed unchanged against the pre-fix head. - A case titled "every id-consuming phase rejects a legacy id" never issued a dry run -- the harness had no dry-run parameter at all. Added -DryRun to Invoke-StateAware and exercised each phase both ways, plus the missing counterpart: a well-formed id must be ACCEPTED by --dry-run on every phase, or the agreement would be satisfied trivially by refusing everything. - The crafted-appId case spliced a raw U+0007 into the JSON text, which RFC 8259 forbids inside a string, so serde_json rejected it at parse time and the payload never reached validate_app_id. Written as \u0007 so the document is valid and the control character survives into the decoded string, and the message is asserted to name `appId` -- which is what distinguishes validate_app_id running from the parser refusing. An oversized-appId case is added, having no JSON-level analogue and so unable to pass for the wrong reason. - The dry-run tests asserted only exit codes, which are identical either way. They now assert the result envelope, and the exec command prints a marker and exits 1 so a dropped flag is caught three independent ways. - That envelope assertion in turn overclaimed: start/stop/deprovision return metadata: None, rendered as the same {"result":{}} the dry-run short-circuit produces, so it discriminates nothing for those three. The comment is scoped to what it proves, and the real observable is added where it can live -- wxc_common's dispatch tests, whose call-counting StubBackend already pinned dry-run for provision and exec. start, stop and deprovision were simply missing. Dry-run skipping is now pinned for all five phases at the layer where dry_run actually lives, in Rust tests that run in the local review loop rather than only on the VM. Verified by mutation rather than by reasoning: making the Start arm ignore dry_run fails dispatch_start_dry_run_skips_start_call_but_runs_validate, and only that test. The rule going forward is to make the fix's absence produce a failure and then observe it. Harness -- a leak inside the leak discipline The positive dry-run case provisions a real sandbox, and Run-StateAwareTest swallows throws while the suite's finally reclaims only $script:sandboxId, which that case never set. A throw between provision and cleanup therefore leaked an Indefinite-lifetime agent user outside the harness's own leak discipline -- the discipline cited as evidence elsewhere in this review. The id is now reclaimed in a dedicated try/finally, and the cleanup's exit code is asserted rather than discarded. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0c82ca12-a566-4257-9de7-164be299375e --------- Co-authored-by: adpa-ms <> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0c82ca12-a566-4257-9de7-164be299375e
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📖 Description
State-aware provision accepts an optional
appId— the Package Family Name for a packaged application, any string otherwise — and carries it inside the returnedsandboxId.Why the id rather than a per-phase field. Future OS API changes will act on the calling application's PFN, across several lifecycle phases, and it is not guaranteed the OS will propagate a PFN supplied at provision to a session's other calls. MXC holds no cross-phase state — each phase is a separate process — so the id is the only carrier that survives without the caller re-supplying the value every phase. Embedding works whether or not the OS ends up retaining it.
Nothing consumes
appIdyet. It is accepted, encoded, and decoded back into an internal struct, exposed nowhere. Accepting it now means the future OS contract will not require a breaking change then.The plaintext
iso:<agentUserName>tail becomesiso:<base64url-nopad(JSON)>, with v1 keysversion,agentUserName, and optionalappId. Encoded rather than delimited because the parser must know which fields are present without assuming anything about separator characters:agentUserNameis OS-assigned with no charset guarantee, so a delimited form would silently mis-parse a name containing the delimiter. The base64url alphabet makes that class of bug unrepresentable. The envelope is frozen — always base64url of a JSON object, all evolution as keys inside — and theversiongate is one-directional, rejecting payloads from a newer MXC with a message saying so, and bumped only for changes an old reader must not silently mishandle.appIdvalidation is structural only (no control characters, ≤256 characters). MXC is a pass-through carrier and does not judge what a valid application identity looks like; a PFN grammar check would risk rejecting forms a future OS API accepts. An explicitly empty string is a value distinct from absent, because a future OS API may assign it meaning. The same structural check runs again when asandboxIdis decoded, so the guarantee holds by value rather than by provenance — a caller-supplied id cannot smuggle in anappIdthat provision would have refused.decodelikewise rejects an emptyagentUserName, and every id-consuming phase validates the id in its hook, so--dry-runand a real invocation agree on which ids are acceptable.Accompanying cleanups, taken now because this branch is the breaking-change window:
The one-shot backend config surface is removed. Its only field,
user, existed so one-shot could reject it — guard code compensating for the permissiveexperimentalblock that graduation to the closed stable surface would delete anyway. It is now ignored like any other unrecognised key there.The wire phase type is split per phase, so the generated schema states truthfully which fields are legal where.
The UPN is trimmed consistently at validation and at the OS call; previously a padded UPN passed validation and reached the OS with its spaces intact.
Breaking: legacy plaintext sandbox ids no longer decode (
malformed_id) on any phase that takes an id. Both the session and its agent user account survive an in-place binary replacement — outliving the process is the premise of the state-aware lifecycle, and nothing in MXC stops a session when the binary changes; the account is removed only at an explicitdeprovision, which also terminates any session still running under it. So stop and deprovision before upgrading. If that is missed, recovery is unconditional rather than contingent: a legacy id carries the agent user name in the clear anddecodebinds nothing to the minting binary, so re-encoding that name as a v1 payload yields a working id for the same sandbox. No such sandbox exists in any environment that will take this change.Not touched: the C# SDK. It cannot reach any experimental backend —
experimental_enabledis never set on themxc-sdk→mxc_ffipath — so IsolationSession is unreachable from it today. No wire keys were renamed, so it continues to build and pass unchanged.🔗 References
None.
🔍 Validation
Full local CI-superset gauntlet green on Windows x64 (fmt, clippy, build+test with isolation_session ON and OFF, arm64 cross-build, all six versioning/codegen gates, SDK build/unit/pack/integration): 3668 Rust tests passed, 0 failed.
wxc_host_preppasses 16/16 in an elevated session (its tests require admin).Isolation-session E2E on a live iso-capable VM: 92 passed, 0 failed (one-shot 16, state-aware 64, SDK integration 12), against 79 on the pre-change baseline — the delta is new coverage, with the one-shot rejection test replaced rather than dropped. Manual interactive TTY suite passed at the VM console. Post-run account leak check clean, against a snapshot taken before the run.
New coverage includes codec round-trips, empty-vs-absent
appIddistinctness, verbatim preservation, agent-user names containing colons and path separators, every decode failure mode, the version gate, and a compile-time assertion thatappIdis rejected at start. Dispatcher-level tests pin that--dry-runskips the phase body while still running validation, for all five phases.src/Cargo.lockchanges by one line —base64added toisolation_session_common's dependency list, with no new[[package]]entries. The dependency feed check was reproduced locally the way CI runs it (cargo fetch --lockedthrough the MxcDependencies feed across all five target triples): passes, no 401s, so no feed seeding is required.✅ Checklist
Cargo.lock, thedependency-feed-checkcheck passes (see docs/pull-requests.md)📋 Issue Type
Microsoft Reviewers: Open in CodeFlow