Skip to content

Support for passing appId to IsolationSession upon sandbox provision - #802

Draft
Dom Giandinoto (daamenik) wants to merge 8 commits into
mainfrom
user/dgiandinoto/passing-appId-on-sandbox-provision
Draft

Support for passing appId to IsolationSession upon sandbox provision#802
Dom Giandinoto (daamenik) wants to merge 8 commits into
mainfrom
user/dgiandinoto/passing-appId-on-sandbox-provision

Conversation

@daamenik

@daamenik Dom Giandinoto (daamenik) commented Aug 10, 2026

Copy link
Copy Markdown

📖 Description

Update sandbox-provision APIs to take in an appId. If "" is passed as the appId, attempt to detect the calling process's Package Family Name (PFN) and use PFN:<pfn> instead. If calling process is unpackaged, use appId verbatim.

Use the AddUserAsync2 API from the IsoSessionOps Preview API instead of AddUserAsync().

🔗 References

🔍 Validation

✅ Checklist

📋 Issue Type

  • Bug fix
  • Feature
  • Task

GitHub Actions runs the PR validation build automatically. The ADO pipeline
(MXC-PR-Build) is the Azure version of the PR pipeline, kept in parity with the GitHub
Actions build; it runs on merge to main, and Microsoft reviewers with write access can trigger it
on a PR with /azp run. See docs/pull-requests.md.

If the dependency-feed-check check fails on a new dependency, the crate must be added to
the feed before the PR can pass. See docs/pull-requests.md
for the steps.

Microsoft Reviewers: Open in CodeFlow
Microsoft Reviewers: Open in CodeFlow

Copilot AI balanced review requested due to automatic review settings August 10, 2026 18:31
@azure-pipelines

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds app-scoped IsolationSession provisioning through PFN resolution and AddUserAsync2.

Changes:

  • Resolves empty or absent appId values from the caller’s PFN.
  • Updates IsolationSession bindings and provisioning flows.
  • Revises related documentation and tests.

Reviewed changes

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

Show a summary per file
File Description
src/Cargo.toml Enables Windows package identity APIs.
src/core/wxc_common/src/models.rs Documents resolved appId behavior.
src/backends/isolation_session/common/src/app_id.rs Implements PFN resolution.
src/backends/isolation_session/common/src/lib.rs Registers the resolver module.
src/backends/isolation_session/common/src/manager.rs Uses AddUserAsync2.
src/backends/isolation_session/common/src/state_aware.rs Resolves state-aware provision IDs.
src/backends/isolation_session/common/src/one_shot.rs Adds default PFN detection.
src/backends/isolation_session/common/src/error.rs Updates operation diagnostics.
src/backends/isolation_session/bindings/src/bindings.rs Regenerates Preview API bindings.
external/windows-sdk/isolation-session/GENERATION_INFO.toml Updates binding provenance date.
docs/isolation-session/state-aware-typescript.md Documents TypeScript behavior.
docs/isolation-session/state-aware-rust.md Documents Rust lifecycle behavior.
.github/copilot-instructions.md Updates backend architecture guidance.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +63 to +64
if rc != ERROR_INSUFFICIENT_BUFFER || length == 0 {
return None;

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignoring this; if there's some kind of error when detecting the PFN, we should just continue.

// user is live rather than re-activating to find out.
let (provisioned, _manager) =
IsolationSessionManager::add_user().map_err(map_lifecycle_error)?;
IsolationSessionManager::add_user(app_id.as_deref()).map_err(map_lifecycle_error)?;
Comment thread src/core/wxc_common/src/models.rs Outdated
Comment on lines +266 to +270
/// Resolved at provision before the OS call: a non-empty value is used
/// verbatim; an empty string or an absent value opts into PFN
/// auto-detection (the caller's Package Family Name becomes `PFN:<pfn>`, or
/// the original value is kept when the caller is unpackaged). The resolved
/// value is passed to `IsoSessionOps::AddUserAsync2` and carried verbatim
@daamenik
Dom Giandinoto (daamenik) force-pushed the user/dgiandinoto/passing-appId-on-sandbox-provision branch from b546b66 to 1eb1aaf Compare August 10, 2026 20:26
Copilot AI review requested due to automatic review settings August 10, 2026 20:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (8)

src/backends/isolation_session/common/src/state_aware.rs:80

  • The resolved value is not necessarily the identity “actually used”: when the host gate chooses legacy AddUserAsync, no app ID reaches the OS, but this value is still encoded in the sandboxId. Clarify that distinction so later code does not treat the payload as proof of app association.
        // Resolve the caller-supplied `appId` into the value passed to
        // `AddUserAsync2`: a non-empty id verbatim, or `PFN:<pfn>` for a
        // packaged caller that supplied the default. The resolved value is what
        // rides inside the `sandboxId`, so later phases (and any future OS
        // consumer) see the identity actually used, not the pre-resolution
        // request.

src/core/wxc_common/src/models.rs:271

  • This contract states that every resolved value is passed to AddUserAsync2, but provisioning can select legacy AddUserAsync and omit it. Document the host-dependent fallback, especially because the value still appears in sandboxId even when no OS association was created.
    /// Resolved at provision before the OS call: a non-empty value is used
    /// verbatim; an empty string or an absent value opts into PFN
    /// auto-detection (the caller's Package Family Name becomes `PFN:<pfn>`, or
    /// the original value is kept when the caller is unpackaged). The resolved
    /// value is passed to `IsoSessionOps::AddUserAsync2` and carried verbatim
    /// into the `sandboxId`.

src/backends/isolation_session/common/src/manager.rs:55

  • Err(_) is not limited to the documented “unknown feature” case: transport, activation, and other WinRT failures are also converted into “unsupported.” That can make provisioning continue through legacy AddUserAsync and silently drop an explicitly requested appId. Treat only the expected E_INVALIDARG as an old-host signal and propagate other probe failures.
fn app_scoped_supported_from(level: windows_core::Result<i32>) -> bool {
    matches!(level, Ok(level) if level > 0)

src/backends/isolation_session/common/src/error.rs:26

  • This operation label is also used by the legacy fallback in manager.rs, so failures from AddUserAsync are now reported as IsoSessionOps.AddUserAsync2. Preserve distinct operation labels and select the matching one with the chosen overload (or use an intentionally method-neutral label) so error envelopes and telemetry identify the API that actually failed.
    pub(crate) const ADD_USER: &str = "IsoSessionOps.AddUserAsync2";

docs/isolation-session/state-aware-typescript.md:50

  • This says the resolved ID is always passed to AddUserAsync2 and represents the identity actually used, but manager.rs falls back to AddUserAsync on unsupported hosts and does not pass any app ID. Document that host gate here so TypeScript consumers do not assume the returned SandboxId proves OS-side app association.
| `appId` | string | absent | Optional identifier for the calling application — the Package Family Name for a packaged app, any string otherwise — associating the provisioned agent user with its owning app. **Resolved at provision by the native layer:** a non-empty value is used verbatim; an empty string (or omitting the field) opts into PFN auto-detection, where the calling process's Package Family Name becomes `PFN:<pfn>` (or the original value is kept when the caller is unpackaged). The resolved value is passed to the OS `AddUserAsync2` overload and carried inside the returned `SandboxId` so later phases recover the identity actually used without the caller re-supplying it. Validated structurally only (no control characters, at most 256 characters); rejections surface as `MxcError` with `code: 'policy_validation'`. Beyond PFN substitution, whitespace and case are preserved exactly, and on an unpackaged host an explicitly supplied empty string is a **distinct** value from omitting the field. Provision-phase only — it is fixed for the sandbox's lifetime, and the `IsolationSessionStartConfig` type rejects it at compile time. |

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

  • The host-gating paragraph above explicitly allows legacy AddUserAsync, so these fields are not always returned by AddUserAsync2. Mention both overloads to keep the metadata contract internally consistent.
| `agentUserName` | string | The OS-assigned agent account name returned by `AddUserAsync2`, also carried inside the `sandboxId` payload where it serves as the addressing key for every post-provision phase. Format is OS-internal and not stable across builds. |
| `agentUserSid` | string | The security identifier (SID) of the agent user, returned by `AddUserAsync2`. Diagnostic only. |

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

  • On compatibility hosts the code mints users with legacy AddUserAsync, not AddUserAsync2. Include the fallback here so this concurrency claim matches the implemented host gate.
Distinct `sandboxId`s map to distinct OS agent users (each `AddUserAsync2`

src/backends/isolation_session/common/src/manager.rs:122

  • The method documentation promises that app_id is passed to AddUserAsync2, while the implementation below can call legacy AddUserAsync without it. Describe the gated behavior here so callers understand that the argument is best-effort on older hosts.
    /// The OS interface takes an app id plus an optional enterprise account
    /// name and token. MXC resolves `app_id` (see [`super::app_id`]) and passes
    /// it to the app-scoped [`AddUserAsync2`] overload, with empty strings for
    /// the enterprise account name and token — which selects a local agent
    /// user. `app_id` is the already-resolved value (`PFN:<pfn>` for a packaged
    /// caller that supplied the default, a caller-chosen id verbatim, or empty);
    /// resolution happens in the caller so the same value can be recorded in the
    /// `sandboxId`.

@adpa-ms adpa-ms left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[ReviewAgent] Independent review by three reviewers on different model families, each covering the whole change; every delegated claim re-verified against code before inclusion. Read-only — no builds or test runs, so the author's validation results are relied upon rather than checked. Verdict: sign off with findings. Nothing blocking.

Findings that can't be anchored inline (files this PR doesn't touch, but which it makes wrong):

  • src/core/wxc_common/src/wire.rs:574-577 — still describes appId as merely "Carried inside the sandboxId", with no mention of resolution or the OS call. This text is emitted verbatim into schemas/dev/mxc-config.schema.0.8.0-dev.json, which is what config authors see via $schema. Because wire.rs didn't change, the schema doesn't drift from its source — so check-schema-codegen.js still passes and CI cannot catch this one.
  • sdk/node/src/state-aware-types.ts:46-59 — the shipped JSDoc still says "Carried verbatim" and "Nothing consumes it yet". This PR updated state-aware-typescript.md, which describes this surface, but not the surface's own documentation.
  • sandbox_id.rs:74-75 and :99-104 — "carried only — nothing in MXC consumes it today", and "MXC is a pass-through carrier here". Note :81-84 is correct as written and needs no change.

Verified correct, recorded so it isn't re-litigated: the PFN: colon cannot corrupt the sandboxId (base64url payload; only the first colon is structural); the IsoSessionFeature constant renames preserve their numeric values and have zero references anywhere in src/; AddUserAsync's arity is unchanged, so the fallback call site is unaffected; treating an empty/absent appId as opting into PFN detection matches the API's own sentinel semantics; validation ordering is safe in both directions; the Win32 buffer protocol (sizing call, allocation, terminator trim) is correct; and the seven inline resolver tests plus the feature-level test cover the decision table well.

One product-level note: on a packaged host there is no way to express "no app association" — an empty value and an omitted one both opt into PFN scoping, so every input except a non-empty literal yields a PFN-scoped registration. Flagging it in case that matters for a caller who wants an unassociated agent user.

Comment thread src/backends/isolation_session/common/src/app_id.rs Outdated
Comment thread docs/isolation-session/state-aware-rust.md Outdated
Comment on lines +140 to +151
let async_op = if app_scoped_supported_from(
ops.GetFeatureLevel(IsoSessionFeature::AppScopedRegistration),
) {
ops.AddUserAsync2(
&HSTRING::from(app_id.unwrap_or_default()),
&HSTRING::new(),
&HSTRING::new(),
)
} else {
ops.AddUserAsync(&HSTRING::new(), &HSTRING::new())
}
.map_err(|e| transport_err(op::ADD_USER, "call failed", &e))?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[ReviewAgent] On this branch the appId cannot be passed — the 2-arg AddUserAsync has no such parameter — yet state_aware.rs:93-96 still encodes the resolved value into the sandboxId. The id then asserts an app association this path did not make.

Provision has to keep succeeding on these hosts, so this isn't a request to fail. But the recorded value shouldn't disagree with what happened: consider having add_user return the identity actually applied and encoding that (None here), so the sandboxId can't over-claim.

Separately, app_scoped_supported_from maps any Err to "not supported", while the doc comment above justifies only the documented unknown-feature rejection. A transport failure on the probe is currently indistinguishable from an old host — worth separating "the host says no" from "the query failed".

pub(super) mod op {
pub(crate) const ACTIVATE: &str = "IsoSessionOps.ActivateInstance";
pub(crate) const ADD_USER: &str = "IsoSessionOps.AddUserAsync";
pub(crate) const ADD_USER: &str = "IsoSessionOps.AddUserAsync2";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[ReviewAgent] This constant now reports AddUserAsync2 for both branches, including when the fallback actually invoked the 2-arg AddUserAsync — and it covers the whole ladder, since manager.rs:151-154 and the result-property reads reuse it for "call failed" and "wait failed".

The module doc immediately above states the contract this breaks: "Interface-qualified names of the API operations this backend invokes … the values that reach the wire as error.operation." On a legacy host this groups failures under an API that was deliberately not called, which is misleading precisely where the fallback is otherwise invisible. Suggest selecting the operation constant alongside the overload.

Comment thread src/backends/isolation_session/common/src/state_aware.rs Outdated
Comment thread src/backends/isolation_session/common/src/app_id.rs Outdated
Comment thread src/backends/isolation_session/common/src/app_id.rs Outdated
Copilot AI review requested due to automatic review settings August 11, 2026 23:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

src/backends/isolation_session/common/src/error.rs:26

  • op::ADD_USER is also used for the legacy fallback at manager.rs:148-151. Consequently, failures from AddUserAsync are reported and grouped in telemetry as IsoSessionOps.AddUserAsync2, obscuring which API actually failed. Define separate operation values and select the one matching the chosen branch.
    pub(crate) const ADD_USER: &str = "IsoSessionOps.AddUserAsync2";

"properties": {
"appId": {
"description": "Optional application identifier for the calling application. For a packaged application this is the Package Family Name; for an unpackaged one it may be any string. Carried inside the `sandboxId` so later lifecycle phases can recover it without the caller re-supplying it.",
"description": "Optional identifier for the calling application.\n\n**A packaged application must supply its Package Family Name in the form `PFN:<packageFamilyName>`** (for example `PFN:Contoso.App_8wekyb3d8bbwe`) — the literal prefix `PFN:` followed by the PFN. A non-empty value is used verbatim. An unpackaged application may pass any string.\n\nAlternatively, a packaged caller may pass an empty string or omit the field to opt into best-effort PFN auto-detection: MXC reads the calling process's Package Family Name and substitutes `PFN:<pfn>` for it. On an unpackaged caller the empty/absent value is kept unchanged.\n\nResolution happens at provision, before the OS call; the resolved value is passed to `IsoSessionOps::AddUserAsync2` to associate the provisioned agent user with its owning app, and is carried inside the `sandboxId` so later lifecycle phases recover the identity actually used without the caller re-supplying it.",
Comment on lines +148 to +149
} else {
ops.AddUserAsync(&HSTRING::new(), &HSTRING::new())
Copilot AI review requested due to automatic review settings August 11, 2026 23:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (3)

src/backends/isolation_session/common/src/manager.rs:55

  • This treats every GetFeatureLevel failure as an unsupported old host, although the contract described above only identifies E_INVALIDARG as the compatibility signal. A transient RPC/service/access failure can therefore be swallowed and provisioning can continue through legacy AddUserAsync, silently dropping the requested app association. Return a Result<bool, _>, map only E_INVALIDARG to Ok(false), and propagate other failures from add_user.
fn app_scoped_supported_from(level: windows_core::Result<i32>) -> bool {
    matches!(level, Ok(level) if level > 0)

src/backends/isolation_session/common/src/error.rs:26

  • This operation label is now inaccurate on the documented compatibility path: when app-scoped registration is unavailable, manager.rs calls legacy AddUserAsync, but every call, wait, and result error is still emitted as IsoSessionOps.AddUserAsync2. Track which overload was selected (or use an overload-neutral provisioning operation) so telemetry and diagnostics identify the operation that actually failed.
    pub(crate) const ADD_USER: &str = "IsoSessionOps.AddUserAsync2";

src/core/wxc_common/src/wire.rs:592

  • This documentation promises that the resolved ID is passed to AddUserAsync2 and represents the identity actually used, but manager.rs deliberately falls back to AddUserAsync on unsupported hosts and records the resolved ID even though no app association was made. Document that host gate and fallback here; because this rustdoc generates the schema and wire types, also regenerate those artifacts and align the public TypeScript documentation, which currently repeats the unconditional guarantee.
    /// Resolution happens at provision, before the OS call; the resolved value
    /// is passed to `IsoSessionOps::AddUserAsync2` to associate the provisioned
    /// agent user with its owning app, and is carried inside the `sandboxId` so
    /// later lifecycle phases recover the identity actually used without the
    /// caller re-supplying it.

Copilot AI review requested due to automatic review settings August 12, 2026 20:57
@microsoft-github-policy-service microsoft-github-policy-service Bot added the Copilot-Instructions PR modifies Copilot instruction files (.github/copilot-instructions.md or .github/instructions/) label Aug 12, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (4)

src/backends/isolation_session/common/src/manager.rs:149

  • When an explicit appId is supplied on a host without app-scoped registration, this fallback drops the requested identity but still reports successful provisioning and stores that identity in the sandboxId. The returned state can therefore claim an association the OS never created. Reject the request as unsupported when app_id is supplied; reserve the legacy fallback for calls that do not request app-scoped registration.
        } else {
            ops.AddUserAsync(&HSTRING::new(), &HSTRING::new())

src/backends/isolation_session/common/src/manager.rs:55

  • GetFeatureLevel failures other than the documented E_INVALIDARG case are also converted to “unsupported.” A transient RPC, access, or service failure therefore silently selects the legacy unscoped registration path instead of surfacing the probe failure. Handle only the expected unsupported HRESULT as fallback and propagate other errors through the backend error mapping.
fn app_scoped_supported_from(level: windows_core::Result<i32>) -> bool {
    matches!(level, Ok(level) if level > 0)

src/backends/isolation_session/common/src/error.rs:26

  • The manager still invokes legacy AddUserAsync on fallback hosts, so failures from that path are now emitted as IsoSessionOps.AddUserAsync2. This makes the structured error.operation field and telemetry inaccurate. Use branch-specific operation constants, or a generic operation name that is truthful for both overloads.
    pub(crate) const ADD_USER: &str = "IsoSessionOps.AddUserAsync2";

docs/isolation-session/state-aware-typescript.md:50

  • This public SDK documentation guarantees that the supplied value reaches AddUserAsync2, but the implementation falls back to AddUserAsync and discards appId when the host reports no support. Either remove that silent fallback or document the host requirement and resulting unsupported behavior so SDK callers can rely on the stated association contract.
| `appId` | string | absent | Optional identifier for the calling application, associating the provisioned agent user with its owning app. **A packaged app must supply its Package Family Name in the form `PFN:<packageFamilyName>`** (e.g. `PFN:Contoso.App_8wekyb3d8bbwe`) — the literal `PFN:` prefix followed by the PFN. Because a non-empty value is used **verbatim**, a bare PFN *without* the `PFN:` prefix will **not** be treated as PFN-scoped. An unpackaged app may pass any string. **Resolved by the in-proc native client:** a non-empty value is used verbatim; an empty string (or omitting the field) opts into PFN auto-detection, where the calling process's Package Family Name becomes `PFN:<pfn>` (or the original value is kept when the caller is unpackaged) — so a packaged caller can either pass `PFN:<pfn>` explicitly or leave `appId` empty to have the `PFN:` prefix applied for it. Whatever the caller supplied is passed to the OS `AddUserAsync2` overload and carried inside the returned `SandboxId` so later phases recover exactly what was sent without re-supplying it. Validated structurally only (no control characters, at most 256 characters); rejections surface as `MxcError` with `code: 'policy_validation'`. Whitespace and case are preserved exactly, and an explicitly supplied empty string is a **distinct** value from omitting the field. Provision-phase only — it is fixed for the sandbox's lifetime, and the `IsolationSessionStartConfig` type rejects it at compile time. |

…cumentation updates about passing in aappId in that format, removing dead code from previoous refactors, ensuring correct add_user call logged in telemetry
Copilot AI review requested due to automatic review settings August 13, 2026 21:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (7)

src/backends/isolation_session/common/src/manager.rs:55

  • matches! collapses every GetFeatureLevel failure into “unsupported,” although only E_INVALIDARG identifies an older host rejecting the new enum value. A transient RPC/service failure therefore silently selects legacy AddUserAsync and drops a supplied appId instead of reporting that the capability probe failed. Treat only the documented compatibility HRESULT as unsupported and propagate other errors through the lifecycle error mapping.
fn app_scoped_supported_from(level: windows_core::Result<i32>) -> bool {
    matches!(level, Ok(level) if level > 0)

sdk/node/src/state-aware-types.ts:59

  • This public contract says the value reaches AddUserAsync2 and associates the user, but manager.rs:140-158 deliberately falls back to legacy AddUserAsync on unsupported hosts, where appId is ignored. Document that host-dependent fallback so SDK consumers do not assume successful provisioning guarantees app association.
   * caller the empty/absent value is kept unchanged. A non-empty value reaches
   * the native `AddUserAsync2` overload unchanged; an absent value is forwarded
   * as the empty string, so absent and explicit-empty are indistinguishable at
   * the API boundary — both associate the provisioned agent user with its
   * owning app via PFN auto-detection. The caller's original value is preserved

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

  • This now says every account is minted by AddUserAsync2, but the implementation can mint it through legacy AddUserAsync. Use “each provisioning call” so this section remains accurate on fallback hosts.
Distinct `sandboxId`s map to distinct OS agent users (each `AddUserAsync2`
mints a fresh account). There is no shared registration between them, so

sdk/node/src/state-aware-types.ts:43

  • The PR promises that sandbox-provision APIs can pass appId, but the C# provisioning API still cannot: ProvisionSandboxOptions has only Filesystem and User (sdk/dotnet/Microsoft.Mxc.Sdk/StateAwareTypes.cs:22-32), and BuildProvisionEnvelope never emits appId (MxcLifecycle.cs:68-80). Add an AppId option, serialize it under experimental.isolation_session.provision.appId, and cover/document that public SDK path.

This issue also appears on line 55 of the same file.

   * Optional identifier for the calling application.

src/core/wxc_common/src/wire.rs:595

  • This canonical wire documentation states unconditionally that appId reaches AddUserAsync2, but the implementation falls back to AddUserAsync when app-scoped registration is unavailable and then ignores the value. Describe that fallback here and regenerate the schema and generated TypeScript wire types; otherwise those generated API references advertise a guarantee the backend does not provide.
    /// A non-empty value reaches `IsoSessionOps::AddUserAsync2` unchanged; an
    /// absent value is forwarded as the empty string, so at the API boundary an
    /// absent `appId` and an explicit empty string are indistinguishable — both
    /// request PFN auto-detection. The caller's original value is preserved
    /// inside the `sandboxId` (absent stays absent, an explicit empty string
    /// stays empty), so later lifecycle phases recover exactly what the caller
    /// sent — including the empty-vs-absent distinction — without re-supplying

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

  • These metadata entries say the values always come from AddUserAsync2, contradicting the documented legacy-host fallback above. Refer to the provisioning API generically or name both overloads.

This issue also appears on line 356 of the same file.

| `agentUserName` | string | The OS-assigned agent account name returned by `AddUserAsync2`, also carried inside the `sandboxId` payload where it serves as the addressing key for every post-provision phase. Format is OS-internal and not stable across builds. |
| `agentUserSid` | string | The security identifier (SID) of the agent user, returned by `AddUserAsync2`. Diagnostic only. |

src/backends/isolation_session/common/src/manager.rs:158

  • The new behavior is not exercised by the added tests: the unit test only checks the feature-level helper, while the PowerShell test only decodes the caller-supplied value from sandboxId, which succeeds even if this branch passes the wrong value or invokes the legacy overload. Add coverage around the dispatch boundary (via an injectable/mock ops wrapper, or an OS-gated integration assertion) that verifies supported hosts call AddUserAsync2 with the explicit/default appId and unsupported hosts call the legacy overload.
        let async_op = if app_scoped {
            ops.AddUserAsync2(
                &HSTRING::from(app_id.unwrap_or_default()),
                &HSTRING::new(),
                &HSTRING::new(),
            )
        } else {
            ops.AddUserAsync(&HSTRING::new(), &HSTRING::new())
        }

Copilot AI review requested due to automatic review settings August 13, 2026 22:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (3)

src/core/wxc_common/src/wire.rs:635

  • This contract is unconditional, but manager.rs:140-158 falls back to legacy AddUserAsync when app-scoped registration is unavailable; that overload drops appId and performs no PFN auto-detection. Please document that host-dependent fallback here and regenerate the schema/wire types, otherwise callers can interpret a successful provision as app-associated when it is not.
    /// A non-empty value reaches `IsoSessionOps::AddUserAsync2` unchanged; an
    /// absent value is forwarded as the empty string, so at the API boundary an
    /// absent `appId` and an explicit empty string are indistinguishable — both
    /// request PFN auto-detection. The caller's original value is preserved

sdk/node/src/state-aware-types.ts:59

  • This says both forms associate the user, but on hosts without AppScopedRegistration the Rust backend uses legacy AddUserAsync and discards the value. Add the host-gating/fallback caveat so SDK consumers do not rely on app scoping merely because provision succeeded.
   * the native `AddUserAsync2` overload unchanged; an absent value is forwarded
   * as the empty string, so absent and explicit-empty are indistinguishable at
   * the API boundary — both associate the provisioned agent user with its
   * owning app via PFN auto-detection. The caller's original value is preserved

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

  • These fields are not always returned by AddUserAsync2: the fallback documented above invokes legacy AddUserAsync on unsupported hosts. Refer to both overloads (or simply “the provisioning API”) so the metadata documentation matches the implemented compatibility path.
| `agentUserName` | string | The OS-assigned agent account name returned by `AddUserAsync2`, also carried inside the `sandboxId` payload where it serves as the addressing key for every post-provision phase. Format is OS-internal and not stable across builds. |
| `agentUserSid` | string | The security identifier (SID) of the agent user, returned by `AddUserAsync2`. Diagnostic only. |

Copilot AI review requested due to automatic review settings August 14, 2026 18:43
@daamenik
Dom Giandinoto (daamenik) force-pushed the user/dgiandinoto/passing-appId-on-sandbox-provision branch from 8681bd1 to ef94d11 Compare August 14, 2026 18:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (2)

src/backends/isolation_session/common/src/manager.rs:165

  • When this branch runs for a state-aware request that supplied appId, provisioning still succeeds even though the legacy overload cannot apply that identity; the returned sandboxId then echoes an association that was never created. This silently violates the new appId contract. Please fail with an unsupported/backend-unavailable error when app-scoped registration was requested but is unavailable, and reserve the legacy fallback for a call mode that explicitly permits an unassociated user.
        } else {
            ops.AddUserAsync(&HSTRING::new(), &HSTRING::new())

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

  • This changes one-shot provisioning from always using legacy AddUserAsync to using PFN auto-detection through AddUserAsync2 on capable hosts, but the PR updates only the state-aware documentation. Please document the resulting one-shot app association and older-host fallback in docs/isolation-session/oneshot.md, or keep the legacy call here if changing one-shot identity semantics is unintended.
        // One-shot takes no backend config, so there is no caller-supplied
        // `appId`. Passing `None` selects the default registration, which the
        // in-proc client resolves to the calling process's PFN when packaged
        // (or leaves empty when unpackaged).
        let manager = match IsolationSessionManager::add_user(None) {

Copilot AI review requested due to automatic review settings August 14, 2026 18:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (2)

src/backends/isolation_session/common/src/manager.rs:55

  • This collapses every GetFeatureLevel failure into “unsupported,” although only E_INVALIDARG indicates an older host that does not know this feature. A transient RPC, authorization, or other transport failure therefore falls back to legacy AddUserAsync, silently discards a requested appId, and can report successful unscoped provisioning. Return a Result<bool, _>, treat only the documented unsupported result as false, and propagate other errors.
fn app_scoped_supported_from(level: windows_core::Result<i32>) -> bool {
    matches!(level, Ok(level) if level > 0)

sdk/node/src/state-aware-types.ts:64

  • This promise is not true on hosts without app-scoped registration support: manager.rs falls back to legacy AddUserAsync, which cannot carry appId, while provisioning still succeeds. Document that host-dependent fallback here so SDK consumers know their requested app association may not be applied; mirror the caveat in the TypeScript guide and generated wire/schema source.
   * identity and the caller runs without an app association. A non-empty value
   * reaches the native `AddUserAsync2` overload unchanged; an absent value is
   * forwarded as the empty string, so absent and explicit-empty are indistinguishable at
   * the API boundary — both associate the provisioned agent user with its
   * owning app via PFN auto-detection. The caller's original value is preserved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Copilot-Instructions PR modifies Copilot instruction files (.github/copilot-instructions.md or .github/instructions/)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants