Skip to content

[SDK] Add caller-controlled PTY process APIs - #1377

Merged
Branden Bonaby (bbonaby) merged 14 commits into
mainfrom
user/bbonaby/add-pty-api-through-rust-and-all-sdks
Oct 3, 2026
Merged

Branden Bonaby (bbonaby) merged 14 commits into
mainfrom
user/bbonaby/add-pty-api-through-rust-and-all-sdks

Conversation

@bbonaby

@bbonaby Branden Bonaby (bbonaby) commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator

📖 Description

  • Adds caller-controlled MxcPtyProcess APIs across the Rust, Node.js, and .NET SDKs.
  • Supports caller-driven input, merged terminal output, resize, wait, timeout, kill, and disposal.
  • Supports one-shot and existing-container IsolationSession PTY execution.
  • Routes Node.js and .NET through the existing opaque mxc_ffi live-process handle.
  • Accepts ContainerConfig in the Node.js spawnWithPty API so containment, command, and policy remain config-owned.
  • Rejects unsupported backends and unenforceable IsolationSession policy before sandbox creation.
  • Keeps exec_attached as the blocking API for attaching directly to the caller's current console.
  • Limits this PR to IsolationSession; additional backend implementations remain separate follow-up changes.
flowchart LR
    Caller[Caller]
    Rust[Rust SDK]
    Node[Node.js SDK]
    DotNet[.NET SDK]
    FFI[mxc_ffi opaque handle]
    Engine[mxc_engine]
    Isolation[IsolationSession]
    ConPTY[Service-owned ConPTY]
    Pty[MxcPtyProcess]

    Caller --> Rust
    Caller --> Node
    Caller --> DotNet
    Rust --> Engine
    Node --> FFI
    DotNet --> FFI
    FFI --> Engine
    Engine --> Isolation
    Isolation --> ConPTY
    ConPTY --> Pty
    Pty -->|input / resize / wait / kill| Caller
    Pty -->|merged output| Caller
Loading

🔗 References

🔍 Validation

  • .\build.bat --with-isolation-session — passed.
  • npm run build from sdk/node — passed with Node.js 24.
  • cargo fmt --all -- --check from src — passed.
  • Node.js IsolationSession PTY smoke test — passed:
    • progressive terminal output and input;
    • resize from 100 × 30 to 120 × 40, confirmed by mode con;
    • Ctrl-C interrupted the foreground command without terminating the shell;
    • input continued after Ctrl-C;
    • exit 7 produced exitCode=7;
    • timedOut=false.
  • .NET IsolationSession ConsoleDriver manual validation — passed:
    • progressive output and input relay;
    • live resize;
    • Ctrl-C forwarding;
    • continued input after Ctrl-C;
    • exit-code propagation;
    • teardown and deprovision.

✅ 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

@bbonaby
Branden Bonaby (bbonaby) requested review from a team and a balanced review from Copilot October 2, 2026 21:39
@bbonaby
Branden Bonaby (bbonaby) requested a review from a team as a code owner October 2, 2026 21:39
@azure-pipelines

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

@bbonaby Branden Bonaby (bbonaby) changed the title [SDK] Add caller-owned PTY APIs for IsolationSession [SDK] Add caller-owned PTY APIs for backends Oct 2, 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.

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: 2 Medium severity

Open (2)
What changed in this PR

Adds caller-owned pseudo-terminal (PTY) execution paths for IsolationSession across the Rust engine/public SDK surface, the C FFI layer, and the Node.js/.NET SDKs, including resize and merged output support.

Changes:

  • Introduces MxcPty / PtySize abstractions and PTY-aware dispatch in wxc_common + mxc_engine, with IsolationSession backend implementations.
  • Extends mxc_ffi with PTY spawn/exec + resize APIs and routes Node.js/.NET PTY features through the existing lifecycle handle.
  • Adds Node.js and .NET SDK APIs, docs, and unit tests for PTY spawning and in-container PTY exec.
File Description
src/​ffi/​mxc_ffi/​src/​streaming.rs Switches sandbox handle to a trait object to support both piped and PTY-backed live processes; exposes PTY resize hook.
src/​ffi/​mxc_ffi/​src/​state_aware.rs Adds state-aware exec entry point that returns a caller-owned PTY handle.
src/​ffi/​mxc_ffi/​src/​pty.rs New FFI entry points for one-shot PTY spawn + PTY resize.
src/​ffi/​mxc_ffi/​src/​lib.rs Exposes new PTY module from the FFI crate.
src/​ffi/​mxc_ffi/​build.rs Adds PTY extern file to C# bindings generation + rerun-if-changed list.
src/​core/​wxc_common/​src/​state_aware_dispatch.rs Adds dispatcher for state-aware exec returning a PTY-backed sandbox process.
src/​core/​wxc_common/​src/​state_aware_backend.rs Extends backend trait with optional exec_pty defaulting to unsupported.
src/​core/​wxc_common/​src/​sandbox_process.rs Adds PTY capabilities (is_pty, reader/writer/resize/size) and StdioMode::Pty.
src/​core/​mxc_engine/​src/​state_aware.rs Adds engine entry points for state-aware PTY exec (typed + JSON).
src/​core/​mxc_engine/​src/​lib.rs Exports new PTY exec APIs and adds spawn_with_pty engine helper.
src/​core/​mxc_engine/​src/​dispatch.rs Adds PTY spawn dispatch path and a unit test for early rejection of unsupported backends.
src/​core/​mxc-sdk/​src/​sandbox.rs Introduces public MxcPty wrapper (merged I/O, resize, wait/kill) and MxcPtySize.
src/​core/​mxc-sdk/​src/​lib.rs Exposes PTY APIs/types, adds v1::spawn_with_pty and in-container PTY spawn helpers, updates docs.
src/​core/​mxc-sdk/​README.md Documents state-aware spawn_in_container_with_pty.
src/​backends/​wslc/​common/​src/​wsl_container_runner.rs Adds explicit unreachable arm for PTY mode (rejected earlier).
src/​backends/​wslc/​common/​src/​sandbox.rs Rejects PTY mode early for WSLC.
src/​backends/​seatbelt/​common/​src/​seatbelt_runner.rs Rejects PTY mode early for Seatbelt.
src/​backends/​lxc/​common/​src/​lxc_runner.rs Rejects PTY mode early for LXC.
src/​backends/​isolation_session/​common/​src/​state_aware.rs Implements exec_pty for existing-container IsolationSession.
src/​backends/​isolation_session/​common/​src/​sandbox.rs Adds one-shot and existing-session PTY process wiring and implements PTY-capable SandboxProcess.
src/​backends/​isolation_session/​common/​src/​manager.rs Adds PTY process creation + resize/close stdin/wait/terminate helpers on the manager process wrapper.
src/​backends/​isolation_session/​common/​src/​lib.rs Re-exports one-shot PTY spawn entry point.
src/​backends/​bubblewrap/​common/​src/​bwrap_runner.rs Rejects PTY mode early for Bubblewrap.
sdk/​node/​tests/​unit/​v1-sandbox-policy.test.ts Adds unit coverage for IsolationSession UI policy rejection/omission behavior.
sdk/​node/​tests/​unit/​state-aware.test.ts Adds unit coverage for early validation of PTY dimensions in state-aware PTY exec helper.
sdk/​node/​tests/​unit/​pty-binding.test.ts New unit test for Node PTY binding stream construction + resize + disposal behavior.
sdk/​node/​tests/​unit/​inprocess-run.test.ts Verifies spawnWithPty routes containment and policy shaping through native request building.
sdk/​node/​src/​v1.ts Exports new PTY APIs and spawnInContainerWithPty from the v1 entrypoint.
sdk/​node/​src/​state-aware.ts Adds spawnInContainerWithPty API with validation and binding hookup.
sdk/​node/​src/​sandbox.ts Adds spawnWithPty and prevents emitting/enforcing unsupported UI policy for IsolationSession configs.
sdk/​node/​src/​sandbox-process.ts Refactors stream field names (input/output/error) to avoid collisions and clarify meaning.
sdk/​node/​src/​mxc-pty.ts New MxcPty class extending lifecycle semantics with PTY-specific input/output/resize.
sdk/​node/​src/​bindings/​streaming.ts Splits lifecycle vs streaming native facade so PTY can reuse lifecycle operations.
sdk/​node/​src/​bindings/​pty.ts New PTY native binding layer + injectable test seams + PTY resize integration.
sdk/​node/​package.json Wires new PTY unit test into the test runner list.
sdk/​node/​README.md Documents spawnWithPty and spawnInContainerWithPty and PTY merged output behavior.
sdk/​dotnet/​README.md Documents new PTY APIs and usage patterns.
sdk/​dotnet/​Microsoft.Mxc.Sdk/​V1/​SandboxAdapters.cs Adds SpawnInContainerWithPty to the v1 adapter interface and implementation.
sdk/​dotnet/​Microsoft.Mxc.Sdk/​V1/​MxcSandbox.cs Adds SpawnWithPty that calls the new FFI PTY spawn entry point.
sdk/​dotnet/​Microsoft.Mxc.Sdk/​V1/​MxcLifecycle.cs Adds SpawnInContainerWithPty that calls the new FFI state-aware PTY exec entry point.
sdk/​dotnet/​Microsoft.Mxc.Sdk/​MxcSandboxProcess.cs Makes sandbox process inheritable and adds internal PTY resize call-through.
sdk/​dotnet/​Microsoft.Mxc.Sdk/​MxcPty.cs New MxcPty type wrapping merged streams and resize validation.
sdk/​dotnet/​Microsoft.Mxc.Sdk.Tests/​V1/​MxcLifecycleTests.cs Adds tests for PTY exec null/size validation without native calls.
sdk/​dotnet/​Microsoft.Mxc.Sdk.Tests/​MxcPtyTests.cs Adds tests for default size and PTY spawn argument validation.
sdk/​dotnet/​Microsoft.Mxc.Sdk.ConsoleDriver/​Program.cs Updates console driver to run via PTY and relay I/O + resize events to the console.
docs/​state-aware-lifecycle/​mxc-state-aware-sandbox-api.md Updates lifecycle API docs to include spawnInContainerWithPty.

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

Comment thread sdk/dotnet/Microsoft.Mxc.Sdk.ConsoleDriver/Program.cs
Comment thread sdk/node/README.md Outdated
Copilot AI balanced review requested due to automatic review settings October 2, 2026 21:57
Comment thread sdk/dotnet/Microsoft.Mxc.Sdk/MxcPtyProcess.cs
Comment thread sdk/dotnet/README.md Outdated
Comment thread sdk/dotnet/README.md Outdated

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.

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: 4 Medium severity

Open (4)

Comment thread sdk/dotnet/Microsoft.Mxc.Sdk.ConsoleDriver/Program.cs Outdated
Comment thread sdk/dotnet/Microsoft.Mxc.Sdk/V1/MxcLifecycle.cs
}
}

// Attach the caller-owned PTY to this process's console by relaying its

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

caller-owned PTY

it's backend-owned now, right?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

backend owned, but I'd say caller controlled


// Attach the caller-owned PTY to this process's console by relaying its
// streams and forwarding console input, control characters, and resize events.
private static SandboxWaitResult AttachToCurrentConsole(MxcPty terminal)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

AttachToCurrentConsole

Hrm - kind of shifting the burden to callers.

@bbonaby Branden Bonaby (bbonaby) Oct 3, 2026 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Im not sure that was a burden MXC should take on to begin with tbh

Comment thread sdk/dotnet/Microsoft.Mxc.Sdk/MxcPty.cs Outdated
}

/// <summary>A live sandbox process attached to a caller-driven pseudo-terminal.</summary>
public sealed class MxcPty : MxcSandboxProcess

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

MxcPty : MxcSandboxProcess

nit: node-pty calls this ptyProcess (because it's a PTY and a process)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

MxcPty was technically the proposed name we came up with but perhaps MxcPtyProcess is more descriptive

});

unsafe { finish_handle(outcome, out_handle, out_error) }
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: I think code like this just "goes away" if we accept PTY as an argument instead of managing it in the backend.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, passing the terminal in could simplify some backends, but I'm not sure it would make all of this code go away. Each type of sandbox connects a process to a terminal differently. In particular, IsolationSession creates that terminal connection inside its own service and gives us access to it afterward, so the caller does not already have something it can pass in in that case no?

we could technically support caller provided terminals as an additional option where it works but then we end up with some fragmentation for the ones that don't support it.

@jsidewhite

Copy link
Copy Markdown
Member

Design: I will restate that I do no think this is the right approach.

  • 1: PTY should be an input parameter to a Spawn function. (This simplifies everything, is more general, and less code.)

  • 2: I don't think this is correctly generalizing the "attached" behavior that the IsoSession wants.
    They want to spawn process in "the current TTY" and maybe get a pipe to drive the console (they don't necessarily want to manage a PTY handles). I think they'd be fine with CreateProcessW(inherit=TRUE), tbh.

That said, I think this can work and enable the scenario.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:shipit:

@bbonaby

Branden Bonaby (bbonaby) commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator Author

Jeff Whiteside (@jsidewhite) Passing a PTY makes sense for backends that can consume one, but what concrete cross-SDK object would we pass? ProcessContainer would probably need HPCON , Unix needs file descriptors, and IsolationSession has no PTY-injection API since the service creates its own ConPTY. Are you proposing backend-specific PTY inputs for ProcessContainer/Unix while retaining the separate attached path for IsolationSession?

Add cross-SDK MxcPty support for caller-driven input, merged output, resize, lifecycle control, and one-shot or existing-container IsolationSession execution.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 433e5b70-c2bd-496d-9b8d-bd2085bb2706
Remove the private portable-pty adapter and delegate directly to MXC's PTY process contract so Cargo builds use only feed-approved dependencies. Import the V1 .NET namespace in the PTY tests.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 433e5b70-c2bd-496d-9b8d-bd2085bb2706
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 433e5b70-c2bd-496d-9b8d-bd2085bb2706
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 433e5b70-c2bd-496d-9b8d-bd2085bb2706

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.

Comment thread src/ffi/mxc_ffi/src/streaming.rs
Comment thread src/core/mxc-sdk/src/lib.rs Outdated
Comment thread src/core/mxc_engine/src/lib.rs Outdated
Comment thread src/core/mxc_engine/src/lib.rs Outdated
Comment thread src/core/wxc_common/src/sandbox_process.rs Outdated
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 433e5b70-c2bd-496d-9b8d-bd2085bb2706
Copilot AI balanced review requested due to automatic review settings October 3, 2026 01: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.

Copilot review overview

🟡 Changes recommended

PTY setup failures can return without confirming termination, leaving an untracked process running in an existing IsolationSession.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Low severity Include state-aware PTY handles in the ABI safety contract

src/​ffi/​mxc_ffi/​src/​pty.rs:99

This safety contract excludes handles returned by mxc_state_aware_exec_pty, even though that API returns the same PTY-backed MxcSandbox and the Node/.NET state-aware paths resize it through this function. Include both valid handle-producing APIs so C callers are not told that a supported state-aware resize violates the ABI contract.

Comment thread src/backends/isolation_session/common/src/sandbox.rs Outdated
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 433e5b70-c2bd-496d-9b8d-bd2085bb2706
Copilot AI balanced review requested due to automatic review settings October 3, 2026 02:10

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.

Copilot review overview

🟡 Changes recommended

PTY waits can hang on inherited output, and initial dimensions are applied only after workload launch.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Initial terminal resize occurs after workload launch

src/​backends/​isolation_session/​common/​src/​sandbox.rs:226

pty_process starts the workload before this initial ResizeConsole call. A fast command can therefore observe the service's default dimensions—or perform side effects before a resize failure is returned—even though the SDK exposes this value as the initial terminal size. The dimensions need to be applied as part of launch, or workload execution must be gated until the resize succeeds.

Comment thread src/core/mxc-sdk/src/sandbox.rs Outdated
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 433e5b70-c2bd-496d-9b8d-bd2085bb2706
Copilot AI balanced review requested due to automatic review settings October 3, 2026 02:53
@bbonaby Branden Bonaby (bbonaby) changed the title [SDK] Add caller-owned PTY APIs for backends [SDK] Add caller-controlled PTY process APIs for IsolationSession Oct 3, 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.

Copilot review overview

🟡 Changes recommended

PTY wrappers lose output cancellation and the .NET relay can hang on descendant-held terminal output.

Review effort: Balanced
Findings: 2 High severity · 1 Low severity

Open (3)
Resolved since last review (1)
Previously missed (2)

In code that hasn't changed since last review

Low severity Close ConPTY output before awaiting relay

sdk/​dotnet/​README.md:1006

This existing-container example can block forever after WaitAsync() when a background descendant retains the ConPTY output. The PTY exposes StandardOutputCloser specifically to end that read after foreground completion; acquire it after Output, close it after the wait, and only then await outputTask.

Low severity Fix broken intra-doc link to state-aware entry point

src/​ffi/​mxc_ffi/​src/​state_aware.rs:286

The referenced function does not exist; the streaming state-aware entry point is mxc_exec_state_aware_json. This creates a broken intra-doc link and fails to direct C callers to the ownership contract being reused.

Comment thread sdk/dotnet/Microsoft.Mxc.Sdk.ConsoleDriver/Program.cs
Comment thread src/core/mxc_engine/src/lib.rs
Comment thread docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md Outdated
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 433e5b70-c2bd-496d-9b8d-bd2085bb2706
Copilot AI balanced review requested due to automatic review settings October 3, 2026 03:05

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.

Copilot review overview

🟡 Changes recommended

Shared interactive startup gating and .NET PTY drain cleanup can block existing execution and disposal paths.

Review effort: Balanced
Findings: 2 High severity

Open (2)
Resolved since last review (3)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Restrict PTY API to supported IsolationSession backend

sdk/​node/​src/​state-aware.ts:383

The new public type signature advertises PTY support for every state-aware backend, including SandboxId<'wslc'>, even though native dispatch and the updated lifecycle documentation make this API IsolationSession-only. This defeats the SDK's backend-branded ID typing and turns an unsupported call into a runtime error. Restrict the signature to the supported backend until another implementation is added.

Comment thread sdk/dotnet/Microsoft.Mxc.Sdk/MxcPtyProcess.cs
Comment thread src/backends/isolation_session/common/src/process_options.rs Outdated
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 433e5b70-c2bd-496d-9b8d-bd2085bb2706
Copilot AI balanced review requested due to automatic review settings October 3, 2026 03:25
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 433e5b70-c2bd-496d-9b8d-bd2085bb2706

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.

Copilot review overview

🔵 Needs a closer look

PTY wait/drain paths can hang or leak resources, and Node lacks required runtime preflight and backend typing.

Review effort: Balanced
Findings: None

Resolved since last review (2)
Previously missed (3)

In code that hasn't changed since last review

Medium severity Preflight Node version before spawning PTY processes

sdk/​node/​src/​bindings/​pty.ts:184

The shared PTY creation path invokes the native spawn before applying the native-stdio Node version gate used by the existing streaming APIs (streaming.ts:467-490,559). On Windows Node 24 versions below 24.21.0, this can create an IsolationSession and only then fail while adopting its handles. Run the same ensureSupportedNodeVersion preflight before both PTY spawn entry points so unsupported runtimes fail without launch side effects.

Medium severity Constrain API generics to isolation_session branded IDs and configs

sdk/​node/​src/​state-aware.ts:380

The public signature accepts branded WSLC IDs/configs even though this API is IsolationSession-only, while the updated lifecycle documentation declares SandboxId<'isolation_session'>. This defeats the SDK's compile-time backend branding and turns a statically invalid call into a native runtime failure. Constrain both generic arguments to isolation_session.

Medium severity PTY poll error leaves uncancelled drain thread and pipe handle

src/​core/​mxc-sdk/​src/​sandbox.rs:356

A non-timeout try_wait error returns here before the automatically spawned output drain is cancelled or joined. Because dropping the closer does not call close, a blocked PTY reader (especially one held open by a descendant) leaves a detached thread and native pipe handle indefinitely. Preserve the poll error, run the same drain cleanup used after final wait, and then return it.

Copilot AI balanced review requested due to automatic review settings October 3, 2026 03:30
@bbonaby Branden Bonaby (bbonaby) changed the title [SDK] Add caller-controlled PTY process APIs for IsolationSession [SDK] Add caller-controlled PTY process APIs Oct 3, 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.

Copilot review overview

🔵 Needs a closer look

The Node state-aware PTY signature incorrectly advertises support for backends that always reject the operation.

Review effort: Balanced
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Medium severity Narrow generic backend type to supported IsolationSession backend

sdk/​node/​src/​state-aware.ts:383

The public generic advertises PTY support for every state-aware backend, so TypeScript accepts wslc: and Windows Sandbox IDs even though this implementation and the lifecycle documentation are IsolationSession-only and those calls always fail natively. Narrow the signature to the supported backend, as execInSandbox already narrows its backend set.

@bbonaby
Branden Bonaby (bbonaby) merged commit 2044eac into main Oct 3, 2026
31 checks passed
@bbonaby
Branden Bonaby (bbonaby) deleted the user/bbonaby/add-pty-api-through-rust-and-all-sdks branch October 3, 2026 04:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants