From 9b300dce4d348a0ff6077a0413ee4988802de994 Mon Sep 17 00:00:00 2001 From: "copilot-cli-release-app[bot]" Date: Mon, 28 Sep 2026 19:23:15 +0000 Subject: [PATCH 1/4] Update SDK snapshot for Copilot CLI 1.0.89 --- nodejs/package.json | 2 +- nodejs/src/cliVersion.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/nodejs/package.json b/nodejs/package.json index 1232724c8c..ce9461d63e 100644 --- a/nodejs/package.json +++ b/nodejs/package.json @@ -5,7 +5,7 @@ "url": "https://github.com/github/copilot-sdk.git" }, "version": "0.0.0-dev", - "copilotCliVersion": "1.0.89-7", + "copilotCliVersion": "1.0.89", "description": "TypeScript SDK for programmatic control of GitHub Copilot CLI via JSON-RPC", "main": "./dist/cjs/index.js", "types": "./dist/index.d.ts", diff --git a/nodejs/src/cliVersion.ts b/nodejs/src/cliVersion.ts index 95551eddda..55a8f380c7 100644 --- a/nodejs/src/cliVersion.ts +++ b/nodejs/src/cliVersion.ts @@ -1,3 +1,3 @@ -export const COPILOT_CLI_VERSION = "1.0.89-7"; +export const COPILOT_CLI_VERSION = "1.0.89"; export const COPILOT_CLI_USE_NPM_PACKAGE = false; From 824bf9f873ab369492e70fd2e2890a9d47deca99 Mon Sep 17 00:00:00 2001 From: "copilot-cli-release-app[bot]" Date: Mon, 28 Sep 2026 21:28:54 +0000 Subject: [PATCH 2/4] Update SDK snapshot for Copilot CLI 1.0.90-0 --- CONTRIBUTING.md | 10 + dotnet/README.md | 9 + dotnet/src/Client.cs | 31 +- dotnet/src/Generated/Rpc.cs | 5 +- dotnet/src/Generated/SessionEvents.cs | 10 + dotnet/src/JsonRpc.cs | 11 +- dotnet/test/Harness/E2ETestBase.cs | 2 +- .../test/Unit/CanvasHandlerLifetimeTests.cs | 135 +++++++++ dotnet/test/Unit/StdioShutdownTests.cs | 88 ++++++ go/README.md | 2 +- go/client.go | 34 ++- go/client_shutdown_test.go | 127 ++++++++ go/rpc/zrpc.go | 11 +- go/rpc/zsession_events.go | 4 + .../SessionMcpServerStatusChangedEvent.java | 6 +- .../generated/rpc/SandboxConfigSource.java | 6 +- .../com/github/copilot/CopilotClient.java | 51 +++- .../com/github/copilot/CopilotClientTest.java | 27 +- .../com/github/copilot/StdioShutdownIT.java | 133 +++++++++ nodejs/README.md | 3 + nodejs/package.json | 2 +- nodejs/src/cliVersion.ts | 2 +- nodejs/src/client.ts | 39 ++- nodejs/src/generated/rpc.ts | 14 +- nodejs/src/generated/session-events.ts | 8 + nodejs/src/index.ts | 1 + nodejs/src/types.ts | 1 + nodejs/test/sandbox-config.test.ts | 8 + nodejs/test/stdio-shutdown.test.ts | 99 +++++++ nodejs/tsconfig.test.json | 1 + python/README.md | 6 + python/copilot/client.py | 35 ++- python/copilot/generated/rpc.py | 5 +- python/copilot/generated/session_events.py | 10 + python/test_client.py | 11 +- python/test_stdio_shutdown.py | 166 +++++++++++ rust/README.md | 8 + rust/src/generated/api_types.rs | 18 +- rust/src/generated/session_events.rs | 6 + rust/src/jsonrpc.rs | 7 +- rust/src/lib.rs | 97 ++++++- rust/tests/e2e/client_options.rs | 274 ++++++++++++++++++ rust/tests/e2e/rpc_workspace_checkpoints.rs | 53 ++-- test/harness/stdio-shutdown-runtime.cjs | 59 ++++ .../permission_handler_errors.yaml | 2 +- ...ations_when_handler_explicitly_denies.yaml | 2 +- ...andler_explicitly_denies_after_resume.yaml | 2 +- ..._permission_handler_errors_gracefully.yaml | 2 +- 48 files changed, 1504 insertions(+), 139 deletions(-) create mode 100644 dotnet/test/Unit/CanvasHandlerLifetimeTests.cs create mode 100644 dotnet/test/Unit/StdioShutdownTests.cs create mode 100644 go/client_shutdown_test.go create mode 100644 java/sdk/src/test/java/com/github/copilot/StdioShutdownIT.java create mode 100644 nodejs/test/stdio-shutdown.test.ts create mode 100644 python/test_stdio_shutdown.py create mode 100644 test/harness/stdio-shutdown-runtime.cjs diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4e7b09d0ce..e6d22d4d22 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -294,6 +294,16 @@ follow [the Rust SDK workflow](.github/workflows/sdk-rust.yml) for rustdoc. ### Recording and replaying SDK tests +Owned-stdio shutdown regressions share +`test/harness/stdio-shutdown-runtime.cjs` across all six SDKs. Launch it with +Node and arguments ` `. The fixture acknowledges +`runtime.shutdown`, but writes its cleanup marker only after stdin EOF, matching +the native wrapper's host-finalization boundary. Language-native tests exercise +graceful stop/disposal, force-stop where exposed, a child that ignores EOF, and +failed-startup cleanup. Keep those lifecycle expectations aligned when changing +an SDK transport; test watchdogs must allow all cleanup phases their separate +budgets, rather than treating the graceful-exit timeout as a total shutdown cap. + The shared harness records real inference responses under `test/snapshots`. Record new captures with `GITHUB_TOKEN` set and `GITHUB_ACTIONS` unset; never author model responses by hand. Rerun with `GITHUB_ACTIONS=true` and real diff --git a/dotnet/README.md b/dotnet/README.md index 276b2151d6..17fa8c8cb1 100644 --- a/dotnet/README.md +++ b/dotnet/README.md @@ -166,6 +166,10 @@ Start the CLI server and establish connection. ##### `StopAsync(): Task` Stop the server and close all sessions. Throws if errors are encountered during cleanup. +For an owned stdio runtime, graceful shutdown closes stdin and waits up to 10 seconds +for host cleanup, including telemetry export. This cleanup is best-effort: if the wait +times out, the process is terminated and that timeout alone is not reported as a cleanup +error. A successful return does not guarantee that all telemetry was exported. ##### `ForceStopAsync(): Task` @@ -195,6 +199,11 @@ Create a new conversation session. - `OnUserInputRequest` - Handler for legacy question-and-answer requests from the agent. Enables the legacy `ask_user` tool. See [User Input Requests](#user-input-requests) section. - `AskUserVariant` - Selects the model-facing `ask_user` tool shape. Defaults to `AskUserVariant.Legacy`; use `AskUserVariant.Elicitation` with `OnElicitationRequest`. - `Hooks` - Hook handlers for session lifecycle events. See [Session Hooks](#session-hooks) section. +- `CanvasHandler` - Handles canvas open, close, and action callbacks. The SDK awaits + asynchronous callbacks before replying, including callbacks without a result, unless + the runtime cancels the request first. A cancellation response can be sent while the + callback is still running. Their cancellation token is canceled by a per-request + `$/cancelRequest`, when the runtime connection closes, or when the client is disposed. ##### `ResumeSessionAsync(string sessionId, ResumeSessionConfig? config = null): Task` diff --git a/dotnet/src/Client.cs b/dotnet/src/Client.cs index 5e07c1ef60..c3cc66c048 100644 --- a/dotnet/src/Client.cs +++ b/dotnet/src/Client.cs @@ -692,7 +692,7 @@ or IOException if (ctx.CliProcess is { } childProcess) { - await CleanupCliProcessAsync(childProcess, ctx.StderrPump, errors, _logger); + await CleanupCliProcessAsync(childProcess, ctx.StderrPump, errors, _logger, gracefulRuntimeShutdown); } if (ctx.FfiHost is { } ffiHost) @@ -703,20 +703,35 @@ or IOException } } - private static async Task CleanupCliProcessAsync(Process childProcess, ProcessStderrPump? stderrPump, List? errors, ILogger? logger) + private static async Task CleanupCliProcessAsync(Process childProcess, ProcessStderrPump? stderrPump, List? errors, ILogger? logger, bool gracefulRuntimeShutdown = false) { var processExited = false; try { + if (gracefulRuntimeShutdown && childProcess.StartInfo.RedirectStandardInput && !childProcess.HasExited) + { + try + { + // The native wrapper finalizes host telemetry after stdin EOF, + // not when it acknowledges runtime.shutdown. + childProcess.StandardInput.Close(); + await childProcess.WaitForExitAsync().WaitAsync(s_runtimeShutdownTimeout); + } + catch (Exception ex) when (ex is TimeoutException or IOException or ObjectDisposedException) + { + logger?.LogDebug(ex, "Graceful stdio runtime exit did not complete; terminating the process"); + } + catch (Exception ex) when (ex is InvalidOperationException or System.ComponentModel.Win32Exception or NotSupportedException) + { + AddCleanupError(errors, ex, logger); + } + } + if (!childProcess.HasExited) { - // The runtime completes all cleanup before responding to - // runtime.shutdown and then leaves termination to us; it - // deliberately keeps its JSON-RPC server alive to send the - // response and never self-exits. Waiting for a self-exit that - // will never come just wastes time, so terminate the child - // immediately and only wait to reap it. + // Force-stop, failed startup, and runtimes that ignore EOF still + // require explicit termination. childProcess.Kill(entireProcessTree: true); // Kill is asynchronous; wait for the root CLI process to exit so cleanup callers // do not observe StopAsync/DisposeAsync completion while it is still tearing down. diff --git a/dotnet/src/Generated/Rpc.cs b/dotnet/src/Generated/Rpc.cs index d73e661b36..bbc87c24a7 100644 --- a/dotnet/src/Generated/Rpc.cs +++ b/dotnet/src/Generated/Rpc.cs @@ -17487,6 +17487,7 @@ internal sealed class SessionUpdateOptionsParams public SandboxConfig? SandboxConfig { get; set; } /// Origin of the sandbox choice. Settings-derived origins (never_configured, user_enabled, user_disabled, repository_policy) let managed policy floor a host preference; explicit below-floor changes remain policy conflicts unless a session opt-out is authorized. Also used for telemetry provenance. + [Experimental(global::GitHub.Copilot.Diagnostics.Experimental)] [JsonPropertyName("sandboxConfigSource")] public SandboxConfigSource? SandboxConfigSource { get; set; } @@ -35902,7 +35903,7 @@ public override void Write(Utf8JsonWriter writer, OptionsUpdateReasoningSummary } -/// Origin of the sandbox choice supplied by the host. Settings-derived origins let managed policy floor the host preference; do not tag explicit session overrides as settings-derived. +/// Origin of the sandbox choice supplied by the host. This value describes preference or session intent; it does not authorize bypassing managed policy. [Experimental(global::GitHub.Copilot.Diagnostics.Experimental)] [JsonConverter(typeof(Converter))] [DebuggerDisplay("{Value,nq}")] @@ -35931,7 +35932,7 @@ public SandboxConfigSource(string value) /// The user's persisted settings disabled the sandbox. public static SandboxConfigSource UserDisabled { get; } = new("user_disabled"); - /// A command-line flag selected the sandbox state for this session. + /// An explicit session-scoped choice selected the sandbox state, such as a command-line flag. public static SandboxConfigSource SessionFlag { get; } = new("session_flag"); /// The user disabled the sandbox for the current session. diff --git a/dotnet/src/Generated/SessionEvents.cs b/dotnet/src/Generated/SessionEvents.cs index 4a16e65a13..454b3edeb4 100644 --- a/dotnet/src/Generated/SessionEvents.cs +++ b/dotnet/src/Generated/SessionEvents.cs @@ -6663,11 +6663,21 @@ public sealed partial class SessionMcpServersLoadedData /// Payload of `session.mcp_server_status_changed` for one MCP server's status and optional failure error. public sealed partial class SessionMcpServerStatusChangedData { + /// Runtime configuration provenance for a failed connection, or unknown when unavailable. Additional string values may be introduced. + [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] + [JsonPropertyName("configSource")] + public string? ConfigSource { get; set; } + /// Error message if the server entered a failed state. [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] [JsonPropertyName("error")] public string? Error { get; set; } + /// Runtime-produced classification for the final failed connection; unclassified means no classification was supplied. Additional string values may be introduced. + [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] + [JsonPropertyName("errorClassification")] + public string? ErrorClassification { get; set; } + /// Name of the MCP server whose status changed. [JsonPropertyName("serverName")] public required string ServerName { get; set; } diff --git a/dotnet/src/JsonRpc.cs b/dotnet/src/JsonRpc.cs index 7419ef0e9e..26d4fe297a 100644 --- a/dotnet/src/JsonRpc.cs +++ b/dotnet/src/JsonRpc.cs @@ -852,15 +852,22 @@ await SendErrorResponseAsync( { var result = registration.Handler.DynamicInvoke(invokeArgs); - // Handlers return one of: a synchronous value, Task (void async), or ValueTask. + // Handlers return a synchronous value, Task, ValueTask, or ValueTask. if (result is Task task) { // Task handlers are not supported — use ValueTask for results. - Debug.Assert(!task.GetType().IsGenericType, "Task handlers are not supported; use ValueTask."); + // An async Task method can return a generic runtime state-machine box. + Debug.Assert(registration.Handler.Method.ReturnType == typeof(Task), "Task handlers are not supported; use ValueTask."); await task.ConfigureAwait(false); return null; } + if (result is ValueTask valueTask) + { + await valueTask.ConfigureAwait(false); + return null; + } + if (result is not null && registration.ValueTaskAsTaskMethod is { } valueTaskAsTaskMethod) { var asTask = (Task)valueTaskAsTaskMethod.Invoke(result, null)!; diff --git a/dotnet/test/Harness/E2ETestBase.cs b/dotnet/test/Harness/E2ETestBase.cs index 852e6a694a..a9b18bf6cf 100644 --- a/dotnet/test/Harness/E2ETestBase.cs +++ b/dotnet/test/Harness/E2ETestBase.cs @@ -202,7 +202,7 @@ protected static Dictionary CreateTestMcpServers(params }); } - protected static string FindTestHarnessDir() + protected internal static string FindTestHarnessDir() { var relativePath = Path.Join("test", "harness", "test-mcp-server.mjs"); var dir = new DirectoryInfo(AppContext.BaseDirectory); diff --git a/dotnet/test/Unit/CanvasHandlerLifetimeTests.cs b/dotnet/test/Unit/CanvasHandlerLifetimeTests.cs new file mode 100644 index 0000000000..a7b014cb08 --- /dev/null +++ b/dotnet/test/Unit/CanvasHandlerLifetimeTests.cs @@ -0,0 +1,135 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + *--------------------------------------------------------------------------------------------*/ + +#if NET8_0_OR_GREATER +using GitHub.Copilot.Rpc; +using Xunit; + +namespace GitHub.Copilot.Test.Unit; + +public sealed partial class ClientSessionLifetimeTests +{ + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task CanvasClose_Awaits_Handler_Completion_And_Propagates_Errors(bool fail) + { + await using var server = await FakeCopilotServer.StartAsync(); + server.ResponseFactory = _ => new Dictionary(); + await using var client = new CopilotClient(new CopilotClientOptions + { + Connection = RuntimeConnection.ForUri(server.Url) + }); + var started = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + await using var session = await client.CreateSessionAsync(new SessionConfig + { + CanvasHandler = new CloseCallbackCanvasHandler(async token => + { + started.SetResult(); + await release.Task.WaitAsync(token); + if (fail) + { + throw new InvalidOperationException("close handler failed"); + } + }) + }); + var close = server.SendRequestAsync("canvas.close", CanvasCloseParams(session)); + try + { + await started.Task.WaitAsync(TimeSpan.FromSeconds(5)); + // The ping response fences dispatch of the earlier canvas callback. + await client.PingAsync().WaitAsync(TimeSpan.FromSeconds(5)); + Assert.False(close.IsCompleted); + release.SetResult(); + + if (fail) + { + var error = await Assert.ThrowsAsync(() => + close.WaitAsync(TimeSpan.FromSeconds(5))); + Assert.Contains("close handler failed", error.Message); + } + else + { + await close.WaitAsync(TimeSpan.FromSeconds(5)); + } + } + finally + { + release.TrySetResult(); + } + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task CanvasClose_Cancels_Handler_When_Connection_Closes(bool disposeClient) + { + await using var server = await FakeCopilotServer.StartAsync(); + server.ResponseFactory = _ => new Dictionary(); + await using var client = new CopilotClient(new CopilotClientOptions + { + Connection = RuntimeConnection.ForUri(server.Url) + }); + var started = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var cancelled = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + await using var session = await client.CreateSessionAsync(new SessionConfig + { + CanvasHandler = new CloseCallbackCanvasHandler(async token => + { + using var registration = token.Register(() => cancelled.TrySetResult()); + started.SetResult(token); + await release.Task; + }) + }); + var close = server.SendRequestAsync("canvas.close", CanvasCloseParams(session)); + _ = close.ContinueWith( + static task => _ = task.Exception, + CancellationToken.None, + TaskContinuationOptions.OnlyOnFaulted | TaskContinuationOptions.ExecuteSynchronously, + TaskScheduler.Default); + try + { + var token = await started.Task.WaitAsync(TimeSpan.FromSeconds(5)); + await client.PingAsync().WaitAsync(TimeSpan.FromSeconds(5)); + Assert.False(token.IsCancellationRequested); + + if (disposeClient) + { + await client.DisposeAsync(); + } + else + { + server.CloseConnection(); + } + + await cancelled.Task.WaitAsync(TimeSpan.FromSeconds(5)); + Assert.True(token.IsCancellationRequested); + } + finally + { + release.TrySetResult(); + } + } + + private static Dictionary CanvasCloseParams(CopilotSession session) => new() + { + ["sessionId"] = session.SessionId, + ["canvasId"] = "test-canvas", + ["instanceId"] = "test-instance" + }; + + private sealed class CloseCallbackCanvasHandler(Func close) : CanvasHandlerBase + { + public override Task OnOpenAsync( + CanvasProviderOpenRequest context, CancellationToken cancellationToken) => + Task.FromResult(new CanvasProviderOpenResult { Status = "ready" }); + + public override Task OnCloseAsync( + CanvasProviderCloseRequest context, CancellationToken cancellationToken) => + close(cancellationToken); + } +} +#endif diff --git a/dotnet/test/Unit/StdioShutdownTests.cs b/dotnet/test/Unit/StdioShutdownTests.cs new file mode 100644 index 0000000000..595e5bbb95 --- /dev/null +++ b/dotnet/test/Unit/StdioShutdownTests.cs @@ -0,0 +1,88 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + *--------------------------------------------------------------------------------------------*/ + +#if NET8_0_OR_GREATER +using System.Diagnostics; +using GitHub.Copilot.Test.Harness; +using Xunit; + +namespace GitHub.Copilot.Test.Unit; + +public sealed class StdioShutdownTests +{ + [Theory] + [InlineData("stop")] + [InlineData("dispose")] + [InlineData("force")] + [InlineData("fallback")] + [InlineData("start-failure")] + public async Task Owned_Stdio_Runtime_Finishes_Host_Cleanup_Before_Graceful_Stop_Returns(string operation) + { + var directory = Path.Combine(Path.GetTempPath(), $"copilot-shutdown-{Guid.NewGuid():N}"); + Directory.CreateDirectory(directory); + var script = Path.Combine(E2ETestBase.FindTestHarnessDir(), "stdio-shutdown-runtime.cjs"); + var marker = Path.Combine(directory, "telemetry.jsonl"); + var pidPath = Path.Combine(directory, "runtime.pid"); + + try + { + await using var client = new CopilotClient(new CopilotClientOptions + { + Connection = RuntimeConnection.ForStdio(path: "node", args: [script, marker, operation, pidPath]), + UseLoggedInUser = false, + }); + if (operation == "start-failure") + { + var error = await Assert.ThrowsAsync(() => + client.StartAsync().WaitAsync(TimeSpan.FromSeconds(5))); + Assert.Contains("protocol version mismatch", error.Message); + var pid = int.Parse(await File.ReadAllTextAsync(pidPath), System.Globalization.CultureInfo.InvariantCulture); + Assert.Throws(() => Process.GetProcessById(pid)); + Assert.False(File.Exists(marker)); + return; + } + + await client.StartAsync().WaitAsync(TimeSpan.FromSeconds(5)); + using var process = Process.GetProcessById( + int.Parse(await File.ReadAllTextAsync(pidPath), System.Globalization.CultureInfo.InvariantCulture)); + var elapsed = Stopwatch.StartNew(); + + switch (operation) + { + case "stop": + await client.StopAsync().WaitAsync(TimeSpan.FromSeconds(5)); + break; + case "dispose": + await client.DisposeAsync().AsTask().WaitAsync(TimeSpan.FromSeconds(5)); + break; + case "fallback": + // Allow shutdown RPC, graceful exit, kill/reap, and stderr drain their separate budgets. + await client.StopAsync().WaitAsync(TimeSpan.FromSeconds(40)); + Assert.True(elapsed.Elapsed >= TimeSpan.FromSeconds(10), + "Graceful stop must wait for its exit timeout before terminating the child."); + break; + default: + await client.ForceStopAsync().WaitAsync(TimeSpan.FromSeconds(5)); + break; + } + + Assert.True(process.HasExited); + await client.DisposeAsync(); + await client.DisposeAsync(); + if (operation == "force") + { + Assert.False(File.Exists(marker)); + } + else + { + Assert.Equal("{\"type\":\"span\"}\n", await File.ReadAllTextAsync(marker)); + } + } + finally + { + Directory.Delete(directory, recursive: true); + } + } +} +#endif diff --git a/go/README.md b/go/README.md index f301ed9d19..d7545a9573 100644 --- a/go/README.md +++ b/go/README.md @@ -244,7 +244,7 @@ Implemented with pure-Go FFI (via [purego](https://github.com/ebitengine/purego) - `NewClient(options *ClientOptions) *Client` - Create a new client - `Start(ctx context.Context) error` - Start the CLI server -- `Stop() error` - Stop the CLI server +- `Stop() error` - Gracefully stop the CLI server. For an owned stdio process, requests runtime shutdown, closes stdin, and waits up to 10 seconds for host cleanup (including telemetry) and natural exit before falling back to a forced termination. - `ForceStop()` - Forcefully stop without graceful cleanup - `CreateSession(ctx context.Context, config *SessionConfig) (*Session, error)` - Create a new session - `ResumeSession(ctx context.Context, sessionID string, config *ResumeSessionConfig) (*Session, error)` - Resume an existing session diff --git a/go/client.go b/go/client.go index 52443f09aa..3fcc40c80b 100644 --- a/go/client.go +++ b/go/client.go @@ -34,6 +34,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "log" "net" "net/netip" @@ -148,6 +149,7 @@ func validateEnvironmentOptions(connection RuntimeConnection, opts *ClientOption type Client struct { options ClientOptions process *exec.Cmd + processStdin io.WriteCloser client *jsonrpc2.Client actualPort int actualHost string @@ -570,8 +572,8 @@ func (c *Client) Start(ctx context.Context) error { // This method performs graceful cleanup: // 1. Closes all active sessions (releases in-memory resources) // 2. Requests runtime shutdown for SDK-owned CLI processes -// 3. Closes the JSON-RPC connection -// 4. Terminates the CLI server process (if spawned by this client) +// 3. Closes owned stdio input and waits up to 10 seconds for host cleanup and exit +// 4. Terminates any remaining owned CLI process and closes the JSON-RPC connection // // Note: session data on disk is preserved, so sessions can be resumed later. // To permanently remove session data before stopping, call [Client.DeleteSession] @@ -634,11 +636,24 @@ func (c *Client) Stop() error { } } - // The runtime completes all cleanup before responding to runtime.shutdown - // and then leaves termination to us; it deliberately keeps its JSON-RPC - // server alive to send the response and never self-exits. Waiting for a - // self-exit that will never come just wastes time, so terminate the child - // immediately and only wait to reap it. + // The stdio host finalizes telemetry after EOF, not after runtime.shutdown. + // Keep stdout open while allowing the child to finish that cleanup naturally. + if c.process != nil && !c.isExternalServer && c.processStdin != nil { + processExitStart := time.Now() + if err := c.processStdin.Close(); err != nil && !errors.Is(err, os.ErrClosed) { + errs = append(errs, fmt.Errorf("failed to close CLI stdin: %w", err)) + } + c.processStdin = nil + select { + case <-c.processDone: + c.logDebugTiming(processExitStart, "CopilotClient.Stop CLI process exited gracefully") + c.osProcess.Store(nil) + c.process = nil + case <-time.After(processExitTimeout): + c.logDebugTiming(processExitStart, "CopilotClient.Stop CLI process exit timed out; killing process") + } + } + if c.process != nil && !c.isExternalServer { if err := c.killProcessAndWait(); err != nil { errs = append(errs, err) @@ -2214,6 +2229,7 @@ func (c *Client) startCLIServer(ctx context.Context) error { return fmt.Errorf("failed to start CLI server: %w", err) } + c.processStdin = stdin c.monitorProcess() // Create JSON-RPC client immediately @@ -2396,6 +2412,10 @@ func (c *Client) killProcess() error { return fmt.Errorf("failed to kill CLI process: %w", err) } } + if c.processStdin != nil { + _ = c.processStdin.Close() + c.processStdin = nil + } c.process = nil return nil } diff --git a/go/client_shutdown_test.go b/go/client_shutdown_test.go new file mode 100644 index 0000000000..6bcef7c2ef --- /dev/null +++ b/go/client_shutdown_test.go @@ -0,0 +1,127 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. + +package copilot_test + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" + + copilot "github.com/github/copilot-sdk/go" +) + +func TestOwnedStdioShutdown(t *testing.T) { + node, err := exec.LookPath("node") + if err != nil { + t.Fatal("shutdown fixture requires Node.js:", err) + } + fixture, err := filepath.Abs("../test/harness/stdio-shutdown-runtime.cjs") + if err != nil { + t.Fatal(err) + } + + for _, mode := range []string{"stop", "force", "fallback", "start-failure"} { + t.Run(mode, func(t *testing.T) { + directory := t.TempDir() + marker := filepath.Join(directory, "telemetry.jsonl") + pidFile := filepath.Join(directory, "runtime.pid") + client := copilot.NewClient(&copilot.ClientOptions{ + Connection: copilot.StdioConnection{ + Path: node, + Args: []string{fixture, marker, mode, pidFile}, + }, + UseLoggedInUser: copilot.Bool(false), + }) + t.Cleanup(client.ForceStop) + + ctx, cancel := context.WithTimeout(t.Context(), 10*time.Second) + defer cancel() + err := client.Start(ctx) + if mode == "start-failure" { + if err == nil || !strings.Contains(err.Error(), "protocol version") { + t.Fatalf("expected protocol version failure, got %v", err) + } + } else { + if err != nil { + t.Fatal("Start failed:", err) + } + started := time.Now() + if mode == "force" { + runShutdownWithWatchdog(t, func() error { + client.ForceStop() + return nil + }) + if elapsed := time.Since(started); elapsed >= 10*time.Second { + t.Fatalf("ForceStop waited for graceful timeout: %s", elapsed) + } + } else { + runShutdownWithWatchdog(t, client.Stop) + } + if mode == "fallback" && time.Since(started) < 10*time.Second { + t.Fatal("Stop did not allow the full graceful exit timeout") + } + } + + // Force-stop and failed startup kill without waiting for the child to be reaped. + exitWait := "0" + if mode == "force" || mode == "start-failure" { + exitWait = "5000" + } + assertShutdownChildExited(t, node, pidFile, exitWait) + contents, err := os.ReadFile(marker) + if mode == "force" || mode == "start-failure" { + if !os.IsNotExist(err) { + t.Fatalf("forced termination unexpectedly finalized telemetry: %q (error: %v)", contents, err) + } + } else if err != nil || string(contents) != "{\"type\":\"span\"}\n" { + t.Fatalf("Stop returned without EOF cleanup: %q (error: %v)", contents, err) + } + runShutdownWithWatchdog(t, client.Stop) + }) + } +} + +func runShutdownWithWatchdog(t *testing.T, stop func() error) { + t.Helper() + done := make(chan error, 1) + go func() { done <- stop() }() + select { + case err := <-done: + if err != nil { + t.Fatal("shutdown failed:", err) + } + case <-time.After(40 * time.Second): + // Cover shutdown RPC, graceful exit, and forced reap budgets, plus scheduling slack. + t.Fatal("shutdown exceeded all cleanup budgets") + } +} + +func assertShutdownChildExited(t *testing.T, node, pidFile, waitMillis string) { + t.Helper() + ctx, cancel := context.WithTimeout(t.Context(), 10*time.Second) + defer cancel() + // Node's process probe is portable, unlike os.Process.Signal(0) on Windows. + cmd := exec.CommandContext(ctx, node, "-e", ` +const fs = require("node:fs"); +const pid = Number(fs.readFileSync(process.argv[1], "utf8")); +const deadline = Date.now() + Number(process.argv[2]); +function check() { + try { + process.kill(pid, 0); + } catch (error) { + if (error.code === "ESRCH") return; + throw error; + } + if (Date.now() >= deadline) throw new Error("Child still running"); + setTimeout(check, 25); +} +check(); +`, pidFile, waitMillis) + if output, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("child process did not exit: %v\n%s", err, output) + } +} diff --git a/go/rpc/zrpc.go b/go/rpc/zrpc.go index 0926597716..3ccb7e8432 100644 --- a/go/rpc/zrpc.go +++ b/go/rpc/zrpc.go @@ -14588,6 +14588,8 @@ type SessionOpenOptions struct { // user_disabled, repository_policy) let managed policy floor a host preference; explicit // below-floor changes remain policy conflicts unless a session opt-out is authorized. Also // used for telemetry provenance. + // Experimental: SandboxConfigSource is part of an experimental API and may change or be + // removed. SandboxConfigSource *SandboxConfigSource `json:"sandboxConfigSource,omitempty"` // Capabilities enabled for this session. SessionCapabilities []SessionCapability `json:"sessionCapabilities,omitzero"` @@ -15840,6 +15842,8 @@ type SessionUpdateOptionsParams struct { // user_disabled, repository_policy) let managed policy floor a host preference; explicit // below-floor changes remain policy conflicts unless a session opt-out is authorized. Also // used for telemetry provenance. + // Experimental: SandboxConfigSource is part of an experimental API and may change or be + // removed. SandboxConfigSource *SandboxConfigSource `json:"sandboxConfigSource,omitempty"` // Replaces the session's capability set with the given list. Use to enable or disable // capabilities mid-session (e.g., remove `memory` for reproducible scripted runs). Omit the @@ -23664,9 +23668,8 @@ const ( ResponseFormatTypeJSONSchema ResponseFormatType = "json_schema" ) -// Origin of the sandbox choice supplied by the host. Settings-derived origins let managed -// policy floor the host preference; do not tag explicit session overrides as -// settings-derived. +// Origin of the sandbox choice supplied by the host. This value describes preference or +// session intent; it does not authorize bypassing managed policy. // Experimental: SandboxConfigSource is part of an experimental API and may change or be // removed. type SandboxConfigSource string @@ -23678,7 +23681,7 @@ const ( SandboxConfigSourceRepositoryPolicy SandboxConfigSource = "repository_policy" // The user disabled the sandbox for the current session. SandboxConfigSourceSessionDisabled SandboxConfigSource = "session_disabled" - // A command-line flag selected the sandbox state for this session. + // An explicit session-scoped choice selected the sandbox state, such as a command-line flag. SandboxConfigSourceSessionFlag SandboxConfigSource = "session_flag" // The client disabled the sandbox because the host cannot enforce it. SandboxConfigSourceUnsupportedHost SandboxConfigSource = "unsupported_host" diff --git a/go/rpc/zsession_events.go b/go/rpc/zsession_events.go index dad3151d74..b70b94522d 100644 --- a/go/rpc/zsession_events.go +++ b/go/rpc/zsession_events.go @@ -2178,8 +2178,12 @@ func (*SessionMCPServerRemovedData) Type() SessionEventType { // Payload of `session.mcp_server_status_changed` for one MCP server's status and optional failure error. type SessionMCPServerStatusChangedData struct { + // Runtime configuration provenance for a failed connection, or unknown when unavailable. Additional string values may be introduced. + ConfigSource *string `json:"configSource,omitempty"` // Error message if the server entered a failed state Error *string `json:"error,omitempty"` + // Runtime-produced classification for the final failed connection; unclassified means no classification was supplied. Additional string values may be introduced. + ErrorClassification *string `json:"errorClassification,omitempty"` // Name of the MCP server whose status changed ServerName string `json:"serverName"` // Connection status: connected, failed, needs-auth, pending, disabled, stopped, or not_configured diff --git a/java/sdk/src/generated/java/com/github/copilot/generated/SessionMcpServerStatusChangedEvent.java b/java/sdk/src/generated/java/com/github/copilot/generated/SessionMcpServerStatusChangedEvent.java index b084652db1..a1c411e297 100644 --- a/java/sdk/src/generated/java/com/github/copilot/generated/SessionMcpServerStatusChangedEvent.java +++ b/java/sdk/src/generated/java/com/github/copilot/generated/SessionMcpServerStatusChangedEvent.java @@ -39,7 +39,11 @@ public record SessionMcpServerStatusChangedEventData( /** Connection status: connected, failed, needs-auth, pending, disabled, stopped, or not_configured */ @JsonProperty("status") McpServerStatus status, /** Error message if the server entered a failed state */ - @JsonProperty("error") String error + @JsonProperty("error") String error, + /** Runtime-produced classification for the final failed connection; unclassified means no classification was supplied. Additional string values may be introduced. */ + @JsonProperty("errorClassification") String errorClassification, + /** Runtime configuration provenance for a failed connection, or unknown when unavailable. Additional string values may be introduced. */ + @JsonProperty("configSource") String configSource ) { } } diff --git a/java/sdk/src/generated/java/com/github/copilot/generated/rpc/SandboxConfigSource.java b/java/sdk/src/generated/java/com/github/copilot/generated/rpc/SandboxConfigSource.java index 2e570b83d3..f7d55e6c71 100644 --- a/java/sdk/src/generated/java/com/github/copilot/generated/rpc/SandboxConfigSource.java +++ b/java/sdk/src/generated/java/com/github/copilot/generated/rpc/SandboxConfigSource.java @@ -7,13 +7,17 @@ package com.github.copilot.generated.rpc; +import com.github.copilot.CopilotExperimental; import javax.annotation.processing.Generated; /** - * Origin of the sandbox choice supplied by the host. Settings-derived origins let managed policy floor the host preference; do not tag explicit session overrides as settings-derived. + * Origin of the sandbox choice supplied by the host. This value describes preference or session intent; it does not authorize bypassing managed policy. + * + * @apiNote This type is experimental and may change in a future version. * * @since 1.0.0 */ +@CopilotExperimental @javax.annotation.processing.Generated("copilot-sdk-codegen") public enum SandboxConfigSource { /** The {@code never_configured} variant. */ diff --git a/java/sdk/src/main/java/com/github/copilot/CopilotClient.java b/java/sdk/src/main/java/com/github/copilot/CopilotClient.java index 21a1c0fc9e..0783e6871e 100644 --- a/java/sdk/src/main/java/com/github/copilot/CopilotClient.java +++ b/java/sdk/src/main/java/com/github/copilot/CopilotClient.java @@ -93,11 +93,12 @@ public final class CopilotClient implements AutoCloseable { private static final Logger LOG = Logger.getLogger(CopilotClient.class.getName()); /** - * Timeout, in seconds, used by {@link #close()} when waiting for graceful - * shutdown via {@link #stop()}. + * Timeout, in seconds, allowed by {@link #close()} for session and executor + * cleanup, in addition to the bounded runtime shutdown phases. */ public static final int AUTOCLOSEABLE_TIMEOUT_SECONDS = 10; private static final int RUNTIME_SHUTDOWN_TIMEOUT_SECONDS = 10; + private static final int PROCESS_EXIT_TIMEOUT_SECONDS = 10; private static final int FORCE_KILL_TIMEOUT_SECONDS = 10; /** @@ -735,8 +736,10 @@ private static boolean isUnsupportedConnectMethod(JsonRpcException ex) { *
    *
  1. Closes all active sessions (releases in-memory resources)
    2. *
  2. Requests runtime shutdown for SDK-owned CLI processes
    3. - *
  3. Closes the JSON-RPC connection
    4. - *
  4. Terminates the CLI server process (if spawned by this client)
    5. + *
  5. Closes stdin for an owned stdio process and waits for its host + * cleanup
    6. + *
  6. Closes the JSON-RPC connection, terminating an owned process if + * needed
    7. *
*

* Note: session data on disk is preserved, so sessions can be resumed later. To @@ -828,7 +831,10 @@ private CompletableFuture cleanupConnection(boolean gracefulRuntimeShutdow }); } - return shutdownFuture.handle((ignored, error) -> { + return shutdownFuture.handleAsync((ignored, error) -> { + if (gracefulRuntimeShutdown && connection.process != null && options.isUseStdio()) { + awaitStdioProcessExit(connection.process); + } try { connection.rpc.close(); } catch (Exception e) { @@ -842,10 +848,26 @@ private CompletableFuture cleanupConnection(boolean gracefulRuntimeShutdow closeRuntimeHost(connection.runtimeHost); } return (Void) null; - }); + }, SHUTDOWN_DISPATCHER); }).thenCompose(result -> result); } + private static void awaitStdioProcessExit(Process process) { + try { + // Host telemetry flushes after stdio EOF, not the shutdown RPC response. + // Keep the reader draining stdout until the child has finished. + process.getOutputStream().close(); + if (!process.waitFor(PROCESS_EXIT_TIMEOUT_SECONDS, TimeUnit.SECONDS)) { + LOG.fine("Process did not exit after stdin EOF within graceful shutdown timeout; terminating"); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + LOG.log(Level.FINE, "Interrupted while waiting for process exit", e); + } catch (IOException e) { + LOG.log(Level.FINE, "Error closing process stdin", e); + } + } + /** * Returns true only when the child had already exited and no streams were * destroyed. @@ -853,12 +875,6 @@ private CompletableFuture cleanupConnection(boolean gracefulRuntimeShutdow private static boolean cleanupCliProcess(Process process, boolean forceImmediately) { try { if (process.isAlive()) { - // The runtime completes all cleanup before responding to - // runtime.shutdown and then leaves termination to us; it - // deliberately keeps its JSON-RPC server alive to send the - // response and never self-exits. Waiting for a self-exit that - // will never come just wastes time, so terminate the child - // immediately and only wait to reap it. if (forceImmediately) { process.destroyForcibly(); if (!process.waitFor(FORCE_KILL_TIMEOUT_SECONDS, TimeUnit.SECONDS)) { @@ -1793,9 +1809,11 @@ private CompletableFuture ensureConnected() { * Closes this client using graceful shutdown semantics. *

* This method is intended for {@code try-with-resources} usage and blocks while - * waiting for {@link #stop()} to complete, up to - * {@link #AUTOCLOSEABLE_TIMEOUT_SECONDS} seconds. If shutdown fails or times - * out, the error is logged at {@link Level#FINE} and the method returns. + * waiting for {@link #stop()} to complete. The timeout includes the bounded + * runtime shutdown, natural exit, termination and kill phases, plus + * {@link #AUTOCLOSEABLE_TIMEOUT_SECONDS} for session cleanup. If shutdown fails + * or times out, the error is logged at {@link Level#FINE} and the method + * returns. *

* This method is idempotent. * @@ -1809,7 +1827,8 @@ public void close() { return; disposed = true; try { - stop().get(AUTOCLOSEABLE_TIMEOUT_SECONDS, TimeUnit.SECONDS); + stop().get(AUTOCLOSEABLE_TIMEOUT_SECONDS + RUNTIME_SHUTDOWN_TIMEOUT_SECONDS + PROCESS_EXIT_TIMEOUT_SECONDS + + 2 * FORCE_KILL_TIMEOUT_SECONDS, TimeUnit.SECONDS); } catch (Exception e) { LOG.log(Level.FINE, "Error during close", e); } finally { diff --git a/java/sdk/src/test/java/com/github/copilot/CopilotClientTest.java b/java/sdk/src/test/java/com/github/copilot/CopilotClientTest.java index 7101f56d3b..574f057467 100644 --- a/java/sdk/src/test/java/com/github/copilot/CopilotClientTest.java +++ b/java/sdk/src/test/java/com/github/copilot/CopilotClientTest.java @@ -18,6 +18,7 @@ import com.github.copilot.rpc.SessionLifecycleEventTypes; import com.github.copilot.rpc.ToolDefinition; +import java.io.OutputStream; import java.lang.reflect.Field; import java.util.ArrayList; import java.util.List; @@ -50,31 +51,6 @@ static void setup() { cliPath = TestUtil.findCliPath(); } - @Test - void testStopRequestsRuntimeShutdownForOwnedProcess() throws Exception { - var client = new CopilotClient(new CopilotClientOptions().setAutoStart(false)); - var rpc = mock(JsonRpcClient.class); - when(rpc.invoke(eq("runtime.shutdown"), any(), eq(Void.class))) - .thenReturn(CompletableFuture.completedFuture(null)); - var process = mock(Process.class); - when(process.isAlive()).thenReturn(true); - when(process.waitFor(anyLong(), any(TimeUnit.class))).thenReturn(true); - - setConnectionFuture(client, rpc, process); - - client.stop().get(); - - verify(rpc).invoke(eq("runtime.shutdown"), eq(Map.of()), eq(Void.class)); - verify(rpc).close(); - // The runtime never self-exits after runtime.shutdown (it keeps its - // JSON-RPC server alive to send the response and leaves termination to - // the caller), so stop() terminates the owned process. The mocked - // process exits on the first SIGTERM (waitFor returns true), so we - // never escalate to destroyForcibly(). - verify(process).destroy(); - verify(process, never()).destroyForcibly(); - } - @Test void testStopDoesNotThrowWhenRuntimeShutdownFails() throws Exception { var client = new CopilotClient(new CopilotClientOptions().setAutoStart(false)); @@ -83,6 +59,7 @@ void testStopDoesNotThrowWhenRuntimeShutdownFails() throws Exception { .thenReturn(CompletableFuture.failedFuture(new RuntimeException("shutdown failed"))); var process = mock(Process.class); when(process.isAlive()).thenReturn(true); + when(process.getOutputStream()).thenReturn(OutputStream.nullOutputStream()); when(process.destroyForcibly()).thenReturn(process); when(process.waitFor(anyLong(), any(TimeUnit.class))).thenReturn(true); diff --git a/java/sdk/src/test/java/com/github/copilot/StdioShutdownIT.java b/java/sdk/src/test/java/com/github/copilot/StdioShutdownIT.java new file mode 100644 index 0000000000..ed3200e671 --- /dev/null +++ b/java/sdk/src/test/java/com/github/copilot/StdioShutdownIT.java @@ -0,0 +1,133 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + *--------------------------------------------------------------------------------------------*/ + +package com.github.copilot; + +import com.github.copilot.rpc.CopilotClientOptions; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Comparator; +import java.util.UUID; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * Exercises owned-process shutdown through the public client API. + */ +@Timeout(value = 90, threadMode = Timeout.ThreadMode.SEPARATE_THREAD) +class StdioShutdownIT { + + @Test + void stopWaitsForCleanupAfterStdinEof() throws Exception { + try (var fixture = new Fixture("stop")) { + fixture.client.start().get(30, TimeUnit.SECONDS); + fixture.client.stop().get(60, TimeUnit.SECONDS); + fixture.assertCleanExit(); + } + } + + @Test + void closeWaitsForCleanupAfterStdinEof() throws Exception { + try (var fixture = new Fixture("dispose")) { + fixture.client.start().get(30, TimeUnit.SECONDS); + fixture.client.close(); + fixture.assertCleanExit(); + } + } + + @Test + void forceStopDoesNotWaitForGracefulCleanup() throws Exception { + try (var fixture = new Fixture("force")) { + fixture.client.start().get(30, TimeUnit.SECONDS); + fixture.client.forceStop().get(5, TimeUnit.SECONDS); + fixture.assertExited(); + assertFalse(Files.exists(fixture.marker)); + } + } + + @Test + void stopTerminatesChildThatDoesNotExitAfterEof() throws Exception { + try (var fixture = new Fixture("fallback")) { + fixture.client.start().get(30, TimeUnit.SECONDS); + long started = System.nanoTime(); + fixture.client.stop().get(60, TimeUnit.SECONDS); + assertTrue(System.nanoTime() - started >= TimeUnit.SECONDS.toNanos(10), + "Stop must allow the full graceful exit timeout before terminating"); + fixture.assertCleanExit(); + } + } + + @Test + void closeTerminatesChildThatDoesNotExitAfterEof() throws Exception { + try (var fixture = new Fixture("fallback")) { + fixture.client.start().get(30, TimeUnit.SECONDS); + long started = System.nanoTime(); + fixture.client.close(); + assertTrue(System.nanoTime() - started >= TimeUnit.SECONDS.toNanos(10), + "Close must allow the full graceful exit timeout before terminating"); + fixture.assertCleanExit(); + } + } + + @Test + void failedStartupTerminatesChild() throws Exception { + try (var fixture = new Fixture("start-failure")) { + assertThrows(ExecutionException.class, () -> fixture.client.start().get(30, TimeUnit.SECONDS)); + fixture.assertExited(); + assertFalse(Files.exists(fixture.marker)); + } + } + + private static final class Fixture implements AutoCloseable { + private final Path directory; + private final Path marker; + private final Path pid; + private final CopilotClient client; + + private Fixture(String mode) throws Exception { + directory = Files.createDirectories(Path.of("target", "shutdown-" + UUID.randomUUID()).toAbsolutePath()); + marker = directory.resolve("cleanup.jsonl"); + pid = directory.resolve("pid"); + Path script = Path.of("..", "..", "test", "harness", "stdio-shutdown-runtime.cjs").toAbsolutePath(); + assertTrue(Files.isRegularFile(script), "Shared shutdown fixture must exist"); + client = new CopilotClient(new CopilotClientOptions().setAutoStart(false).setCliPath("node") + .setCliArgs(new String[]{script.toString(), marker.toString(), mode, pid.toString()})); + } + + private void assertCleanExit() throws Exception { + assertEquals("{\"type\":\"span\"}\n", Files.readString(marker)); + assertExited(); + } + + private void assertExited() throws Exception { + assertTrue(Files.isRegularFile(pid), "Child must have started"); + assertFalse( + ProcessHandle.of(Long.parseLong(Files.readString(pid))).map(ProcessHandle::isAlive).orElse(false), + "Child must be reaped before shutdown returns"); + } + + @Override + public void close() throws Exception { + if (Files.isRegularFile(pid)) { + var process = ProcessHandle.of(Long.parseLong(Files.readString(pid))); + if (process.isPresent() && process.get().isAlive()) { + process.get().destroyForcibly(); + process.get().onExit().get(10, TimeUnit.SECONDS); + } + } + client.close(); + try (var paths = Files.walk(directory)) { + for (var path : paths.sorted(Comparator.reverseOrder()).toList()) { + Files.delete(path); + } + } + } + } +} diff --git a/nodejs/README.md b/nodejs/README.md index f995fa59e9..ef452e3cf5 100644 --- a/nodejs/README.md +++ b/nodejs/README.md @@ -188,6 +188,9 @@ Start the CLI server and establish connection. ##### `stop(): Promise` Stop the server and close all sessions. Returns a list of any errors encountered during cleanup. +For an owned stdio runtime, closes stdin and waits up to 10 seconds for host cleanup +(including telemetry export) and process exit before falling back to termination. +This graceful-exit timeout is separate from the shutdown RPC and post-termination wait. ##### `forceStop(): Promise` diff --git a/nodejs/package.json b/nodejs/package.json index ce9461d63e..343b193c26 100644 --- a/nodejs/package.json +++ b/nodejs/package.json @@ -5,7 +5,7 @@ "url": "https://github.com/github/copilot-sdk.git" }, "version": "0.0.0-dev", - "copilotCliVersion": "1.0.89", + "copilotCliVersion": "1.0.90-0", "description": "TypeScript SDK for programmatic control of GitHub Copilot CLI via JSON-RPC", "main": "./dist/cjs/index.js", "types": "./dist/index.d.ts", diff --git a/nodejs/src/cliVersion.ts b/nodejs/src/cliVersion.ts index 55a8f380c7..da0041a673 100644 --- a/nodejs/src/cliVersion.ts +++ b/nodejs/src/cliVersion.ts @@ -1,3 +1,3 @@ -export const COPILOT_CLI_VERSION = "1.0.89"; +export const COPILOT_CLI_VERSION = "1.0.90-0"; export const COPILOT_CLI_USE_NPM_PACKAGE = false; diff --git a/nodejs/src/client.ts b/nodejs/src/client.ts index 9f92d73cb3..cf2ba98576 100644 --- a/nodejs/src/client.ts +++ b/nodejs/src/client.ts @@ -1013,7 +1013,8 @@ export class CopilotClient { * 1. Closes all active sessions (releases in-memory resources) * 2. Requests runtime shutdown for SDK-owned CLI processes * 3. Closes the JSON-RPC connection - * 4. Terminates the CLI server process (if spawned by this client) + * 4. Signals EOF to an owned stdio process and waits for host cleanup, then + * terminates the process if it does not exit within the shutdown timeout * * Note: session data on disk is preserved, so sessions can be resumed later. * To permanently remove session data before stopping, call @@ -1166,15 +1167,33 @@ export class CopilotClient { } } - // The runtime completes all cleanup before responding to - // runtime.shutdown and then leaves termination to us; it deliberately - // keeps its JSON-RPC server alive to send the response and never - // self-exits. Waiting a grace window for a self-exit that will never - // come just wastes time, so terminate the child immediately and only - // wait to reap it. if (this.cliProcess && !this.isExternalServer) { const child = this.cliProcess; - this.cliProcess = null; + if ( + this.connectionConfig.kind === "stdio" && + child.stdin && + child.exitCode == null && + child.signalCode == null + ) { + const gracefulExitStart = Date.now(); + try { + // Host telemetry is finalized after transport EOF, not the shutdown RPC. + child.stdin.end(); + const exited = await waitForChildExit(child, RUNTIME_SHUTDOWN_TIMEOUT_MS); + this.logDebugTiming( + exited + ? "CopilotClient.stop graceful stdio exit complete" + : "CopilotClient.stop graceful stdio exit timed out; terminating child", + gracefulExitStart + ); + } catch (error) { + errors.push( + new Error( + `Failed to close CLI stdin: ${error instanceof Error ? error.message : String(error)}` + ) + ); + } + } try { if (child.exitCode == null && child.signalCode == null) { child.kill(); @@ -1192,6 +1211,10 @@ export class CopilotClient { `Failed to kill CLI process: ${error instanceof Error ? error.message : String(error)}` ) ); + } finally { + if (this.cliProcess === child) { + this.cliProcess = null; + } } } // Tear down the in-process FFI host (closes the native connection and diff --git a/nodejs/src/generated/rpc.ts b/nodejs/src/generated/rpc.ts index 8c3d96f120..bec1818662 100644 --- a/nodejs/src/generated/rpc.ts +++ b/nodejs/src/generated/rpc.ts @@ -4311,7 +4311,7 @@ export type ResponseFormat = { type: "json_schema"; }; /** - * Origin of the sandbox choice supplied by the host. Settings-derived origins let managed policy floor the host preference; do not tag explicit session overrides as settings-derived. + * Origin of the sandbox choice supplied by the host. This value describes preference or session intent; it does not authorize bypassing managed policy. * * This interface was referenced by `_RpcSchemaRoot`'s JSON-Schema * via the `definition` "SandboxConfigSource". @@ -4324,7 +4324,7 @@ export type SandboxConfigSource = | "user_enabled" /** The user's persisted settings disabled the sandbox. */ | "user_disabled" - /** A command-line flag selected the sandbox state for this session. */ + /** An explicit session-scoped choice selected the sandbox state, such as a command-line flag. */ | "session_flag" /** The user disabled the sandbox for the current session. */ | "session_disabled" @@ -22018,6 +22018,11 @@ export interface SessionOpenOptions { */ shellProcessFlags?: string[]; sandboxConfig?: SandboxConfig; + /** + * Origin of the sandbox choice. Settings-derived origins (never_configured, user_enabled, user_disabled, repository_policy) let managed policy floor a host preference; explicit below-floor changes remain policy conflicts unless a session opt-out is authorized. Also used for telemetry provenance. + * + * @experimental + */ sandboxConfigSource?: SandboxConfigSource; /** * Whether interactive shell sessions are logged. @@ -23544,6 +23549,11 @@ export interface SessionUpdateOptionsParams { */ shellProcessFlags?: string[]; sandboxConfig?: SandboxConfig; + /** + * Origin of the sandbox choice. Settings-derived origins (never_configured, user_enabled, user_disabled, repository_policy) let managed policy floor a host preference; explicit below-floor changes remain policy conflicts unless a session opt-out is authorized. Also used for telemetry provenance. + * + * @experimental + */ sandboxConfigSource?: SandboxConfigSource; /** * Whether interactive shell sessions are logged. diff --git a/nodejs/src/generated/session-events.ts b/nodejs/src/generated/session-events.ts index a2a87f0a43..15596e1d37 100644 --- a/nodejs/src/generated/session-events.ts +++ b/nodejs/src/generated/session-events.ts @@ -12577,10 +12577,18 @@ export interface McpServerStatusChangedEvent { * Payload of `session.mcp_server_status_changed` for one MCP server's status and optional failure error. */ export interface McpServerStatusChangedData { + /** + * Runtime configuration provenance for a failed connection, or unknown when unavailable. Additional string values may be introduced. + */ + configSource?: string; /** * Error message if the server entered a failed state */ error?: string; + /** + * Runtime-produced classification for the final failed connection; unclassified means no classification was supplied. Additional string values may be introduced. + */ + errorClassification?: string; /** * Name of the MCP server whose status changed */ diff --git a/nodejs/src/index.ts b/nodejs/src/index.ts index 28202b7af0..c29160f947 100644 --- a/nodejs/src/index.ts +++ b/nodejs/src/index.ts @@ -171,6 +171,7 @@ export type { ProviderModelConfig, ProviderTokenArgs, RemoteSessionMode, + SandboxConfigSource, ResumeSessionConfig, SectionOverride, SectionOverrideAction, diff --git a/nodejs/src/types.ts b/nodejs/src/types.ts index 1c9a92e26a..c8c9c0a53b 100644 --- a/nodejs/src/types.ts +++ b/nodejs/src/types.ts @@ -38,6 +38,7 @@ import type { import type { ToolSet } from "./toolSet.js"; export type { RemoteSessionMode } from "./generated/rpc.js"; export type { CurrentToolMetadata } from "./generated/rpc.js"; +export type { SandboxConfigSource } from "./generated/rpc.js"; export type { ConnectorAccountRequest, ConnectorAvailability, diff --git a/nodejs/test/sandbox-config.test.ts b/nodejs/test/sandbox-config.test.ts index 0870f9c2e3..7494807251 100644 --- a/nodejs/test/sandbox-config.test.ts +++ b/nodejs/test/sandbox-config.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from "vitest"; import type { SandboxConfig } from "../src/generated/rpc.js"; +import type { SandboxConfigSource } from "../src/index.js"; describe("SandboxConfig", () => { it("round-trips allowBypass and omits it when absent", () => { @@ -14,3 +15,10 @@ describe("SandboxConfig", () => { expect(JSON.parse(JSON.stringify(omitted))).toEqual({ enabled: true }); }); }); + +describe("SandboxConfigSource", () => { + it("is importable from the package root", () => { + const source: SandboxConfigSource = "user_disabled"; + expect(source).toBe("user_disabled"); + }); +}); diff --git a/nodejs/test/stdio-shutdown.test.ts b/nodejs/test/stdio-shutdown.test.ts new file mode 100644 index 0000000000..d17575f700 --- /dev/null +++ b/nodejs/test/stdio-shutdown.test.ts @@ -0,0 +1,99 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + *--------------------------------------------------------------------------------------------*/ + +import { existsSync, mkdtempSync, readFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { fileURLToPath } from "node:url"; +import { describe, expect, it, onTestFinished } from "vitest"; +import { CopilotClient, RuntimeConnection } from "../src/index.js"; + +const fixture = fileURLToPath( + new URL("../../test/harness/stdio-shutdown-runtime.cjs", import.meta.url) +); + +describe("owned stdio shutdown", () => { + it.each([ + "stop", + "dispose", + "force", + "fallback", + "start-failure", + "force-during-stop", + ] as const)( + "%s preserves the owned-process cleanup contract", + async (mode) => { + const directory = mkdtempSync(join(tmpdir(), "copilot-node-shutdown-")); + const marker = join(directory, "telemetry.jsonl"); + const pidFile = join(directory, "runtime.pid"); + const client = new CopilotClient({ + connection: RuntimeConnection.forStdio({ + path: process.execPath, + args: [ + fixture, + marker, + mode === "force-during-stop" ? "fallback" : mode, + pidFile, + ], + }), + useLoggedInUser: false, + }); + onTestFinished(async () => { + await client.forceStop(); + rmSync(directory, { + recursive: true, + force: true, + maxRetries: 10, + retryDelay: 100, + }); + }); + + if (mode === "start-failure") { + await expect(client.start()).rejects.toThrow(/protocol version/i); + } else { + await client.start(); + const started = performance.now(); + if (mode === "force") { + await client.forceStop(); + expect(performance.now() - started).toBeLessThan(10_000); + } else if (mode === "force-during-stop") { + const stopping = client.stop(); + await expect.poll(() => existsSync(marker), { timeout: 5000 }).toBe(true); + await client.forceStop(); + expect(await stopping).toEqual([]); + expect(performance.now() - started).toBeLessThan(10_000); + } else if (mode === "dispose") { + await client[Symbol.asyncDispose](); + } else { + expect(await client.stop()).toEqual([]); + } + if (mode === "fallback") { + expect(performance.now() - started).toBeGreaterThanOrEqual(10_000); + } + } + + const pid = Number(readFileSync(pidFile, "utf8")); + const hasExited = () => { + try { + process.kill(pid, 0); + return false; + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ESRCH") throw error; + return true; + } + }; + if (mode === "force" || mode === "start-failure") { + // Force-stop sends the kill signal without waiting to reap the child. + await expect.poll(hasExited, { timeout: 5000 }).toBe(true); + expect(existsSync(marker)).toBe(false); + } else { + expect(hasExited()).toBe(true); + expect(readFileSync(marker, "utf8")).toBe('{"type":"span"}\n'); + } + expect(await client.stop()).toEqual([]); + await client[Symbol.asyncDispose](); + }, + 40_000 + ); +}); diff --git a/nodejs/tsconfig.test.json b/nodejs/tsconfig.test.json index 5618bfd010..f3916c8264 100644 --- a/nodejs/tsconfig.test.json +++ b/nodejs/tsconfig.test.json @@ -12,6 +12,7 @@ "test/ffiRuntimeHost.test.ts", "test/session-event-types.test.ts", "test/session-config-types.test.ts", + "test/sandbox-config.test.ts", "test/message-source.test.ts", "test/legacy-request-compatibility.test.ts" ], diff --git a/python/README.md b/python/README.md index 4938228e4e..db99cd0b44 100644 --- a/python/README.md +++ b/python/README.md @@ -141,6 +141,12 @@ or supply its reply. If you need more control over the lifecycle, you can call `start()`, `stop()`, and `disconnect()` manually: +For an SDK-owned stdio runtime, `stop()` and async context-manager exit request +shutdown, close stdin, and wait up to 10 seconds for the process to finish host +cleanup, including telemetry flushing. A process that does not exit is terminated, +then killed if necessary, with bounded waits. `force_stop()` skips graceful cleanup; +externally managed runtimes are not shut down. + ```python import asyncio diff --git a/python/copilot/client.py b/python/copilot/client.py index a659fde15c..add3283e62 100644 --- a/python/copilot/client.py +++ b/python/copilot/client.py @@ -1425,6 +1425,7 @@ def _session_lifecycle_event_from_dict(data: dict) -> SessionLifecycleEvent: # Servers reporting a version below this are rejected. _MIN_PROTOCOL_VERSION = 3 _RUNTIME_SHUTDOWN_TIMEOUT_SECONDS = 10 +_CLI_PROCESS_GRACEFUL_EXIT_TIMEOUT_SECONDS = 10 _CLI_PROCESS_EXIT_TIMEOUT_SECONDS = 5 @@ -2075,8 +2076,8 @@ async def stop(self) -> None: This method performs graceful cleanup: 1. Closes all active sessions (releases in-memory resources) 2. Requests runtime shutdown for SDK-owned CLI processes - 3. Closes the JSON-RPC connection - 4. Terminates the CLI server process (if spawned by this client) + 3. Closes owned stdio input and waits for host cleanup and natural exit + 4. Closes the JSON-RPC connection and terminates any remaining owned process Note: session data on disk is preserved, so sessions can be resumed later. To permanently remove session data before stopping, call @@ -2143,6 +2144,26 @@ async def stop(self) -> None: ) errors.append(StopError(message=f"Failed to gracefully shut down runtime: {e}")) + # Host telemetry is finalized after stdio EOF, not the shutdown response. + # Keep the readers alive while the child drains its final output. + if ( + self._cli_process is not None + and not self._is_external_server + and isinstance(self._connection, StdioRuntimeConnection) + and self._cli_process.poll() is None + ): + try: + if self._cli_process.stdin is not None: + self._cli_process.stdin.close() + await asyncio.to_thread( + self._cli_process.wait, + timeout=_CLI_PROCESS_GRACEFUL_EXIT_TIMEOUT_SECONDS, + ) + except subprocess.TimeoutExpired: + logger.debug("Timed out waiting for graceful CLI exit; terminating the process") + except OSError: + logger.debug("Error while closing Copilot CLI stdin", exc_info=True) + # Close client if self._client: await self._client.stop() @@ -2171,15 +2192,7 @@ async def stop(self) -> None: logger.debug("Error while closing Copilot runtime transport", exc_info=True) self._process = None - # Terminate CLI process (only if we spawned it). - # - # Per the runtime.shutdown contract, the runtime completes all cleanup - # *before* responding and then leaves termination to the caller ("callers - # may then terminate the owned runtime process"). It deliberately keeps - # its JSON-RPC server alive to send the response and does not self-exit, - # so there is no point waiting a grace window for a self-exit that will - # never come. Once shutdown has completed (or failed) we terminate the - # child immediately and only wait to reap it. + # Terminate and reap an owned process that did not exit gracefully. if self._cli_process and not self._is_external_server: poll = getattr(self._cli_process, "poll", None) is_running = poll is None or poll() is None diff --git a/python/copilot/generated/rpc.py b/python/copilot/generated/rpc.py index 0cf3b44f5b..8811b53745 100644 --- a/python/copilot/generated/rpc.py +++ b/python/copilot/generated/rpc.py @@ -10654,9 +10654,8 @@ def to_dict(self) -> dict: # Experimental: this type is part of an experimental API and may change or be removed. class SandboxConfigSource(Enum): - """Origin of the sandbox choice supplied by the host. Settings-derived origins let managed - policy floor the host preference; do not tag explicit session overrides as - settings-derived. + """Origin of the sandbox choice supplied by the host. This value describes preference or + session intent; it does not authorize bypassing managed policy. Origin of the sandbox choice. Settings-derived origins (never_configured, user_enabled, user_disabled, repository_policy) let managed policy floor a host preference; explicit diff --git a/python/copilot/generated/session_events.py b/python/copilot/generated/session_events.py index 6f605929f1..6a71634747 100644 --- a/python/copilot/generated/session_events.py +++ b/python/copilot/generated/session_events.py @@ -9369,26 +9369,36 @@ class SessionMcpServerStatusChangedData: "Payload of `session.mcp_server_status_changed` for one MCP server's status and optional failure error." server_name: str status: McpServerStatus + config_source: str | None = None error: str | None = None + error_classification: str | None = None @staticmethod def from_dict(obj: Any) -> "SessionMcpServerStatusChangedData": assert isinstance(obj, dict) server_name = from_str(obj.get("serverName")) status = parse_enum(McpServerStatus, obj.get("status")) + config_source = from_union([from_none, from_str], obj.get("configSource")) error = from_union([from_none, from_str], obj.get("error")) + error_classification = from_union([from_none, from_str], obj.get("errorClassification")) return SessionMcpServerStatusChangedData( server_name=server_name, status=status, + config_source=config_source, error=error, + error_classification=error_classification, ) def to_dict(self) -> dict: result: dict = {} result["serverName"] = from_str(self.server_name) result["status"] = to_enum(McpServerStatus, self.status) + if self.config_source is not None: + result["configSource"] = from_union([from_none, from_str], self.config_source) if self.error is not None: result["error"] = from_union([from_none, from_str], self.error) + if self.error_classification is not None: + result["errorClassification"] = from_union([from_none, from_str], self.error_classification) return result diff --git a/python/test_client.py b/python/test_client.py index f90b1a1865..4dcb581463 100644 --- a/python/test_client.py +++ b/python/test_client.py @@ -138,7 +138,7 @@ class TestClientShutdown: async def test_stop_requests_runtime_shutdown_for_owned_process(self): calls: list[str] = [] process = Mock() - process.poll.return_value = None + process.poll.side_effect = [None, 0] process.wait.return_value = 0 class Runtime: @@ -154,12 +154,9 @@ async def shutdown(self, *, timeout=None): await client.stop() assert calls == ["runtime.shutdown"] - # The runtime never self-exits after runtime.shutdown (it keeps its - # JSON-RPC server alive to send the response and leaves termination to - # the caller), so stop() terminates the owned process. The mocked - # process exits on terminate() (wait returns immediately), so we never - # escalate to kill(). - process.terminate.assert_called_once() + process.stdin.close.assert_called_once() + process.wait.assert_called_once_with(timeout=10) + process.terminate.assert_not_called() process.kill.assert_not_called() @pytest.mark.asyncio diff --git a/python/test_stdio_shutdown.py b/python/test_stdio_shutdown.py new file mode 100644 index 0000000000..1ce8388b1f --- /dev/null +++ b/python/test_stdio_shutdown.py @@ -0,0 +1,166 @@ +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the MIT License. + +"""Public lifecycle regressions for host cleanup after stdio EOF.""" + +import asyncio +import shutil +import subprocess +import sys +import time +from pathlib import Path + +import pytest + +from copilot import CopilotClient, RuntimeConnection + +_RUNTIME = Path(__file__).parent.parent / "test" / "harness" / "stdio-shutdown-runtime.cjs" + +_EXIT_PROBE = """ +const fs = require("node:fs"); +const { spawnSync } = require("node:child_process"); +const pid = Number(fs.readFileSync(process.argv[1], "utf8")); +const deadline = Date.now() + Number(process.argv[2]); +function check() { + try { + process.kill(pid, 0); + } catch (error) { + if (error.code === "ESRCH") return; + throw error; + } + // Force-stop does not reap children; a zombie has already exited. + if (process.platform === "linux") { + try { + const stat = fs.readFileSync(`/proc/${pid}/stat`, "utf8"); + // The parenthesized command name can itself contain spaces and parentheses. + if (stat.charAt(stat.lastIndexOf(")") + 2) === "Z") return; + } catch (error) { + if (error.code === "ENOENT") return; + throw error; + } + } else if (process.platform !== "win32") { + const status = spawnSync("ps", ["-o", "stat=", "-p", String(pid)], { encoding: "utf8" }); + if (status.status === 0 && status.stdout.trim().startsWith("Z")) return; + } + if (Date.now() >= deadline) throw new Error("Child still running"); + setTimeout(check, 25); +} +check(); +""" + + +async def _assert_child_exited(directory: Path, wait_millis: int = 0) -> None: + node = shutil.which("node") + assert node is not None + result = await asyncio.to_thread( + subprocess.run, + [node, "-e", _EXIT_PROBE, str(directory / "pid"), str(wait_millis)], + capture_output=True, + text=True, + timeout=10, + check=False, + ) + assert result.returncode == 0, result.stdout + result.stderr + + +async def test_exit_probe_distinguishes_running_and_exited_child(tmp_path, monkeypatch): + node = shutil.which("node") + assert node is not None + if sys.platform == "linux": + # Linux must not depend on procps options absent from Alpine's BusyBox ps. + (tmp_path / "node").symlink_to(node) + monkeypatch.setenv("PATH", str(tmp_path)) + child = subprocess.Popen([node, "-e", "setInterval(() => {}, 1000)"]) + try: + (tmp_path / "pid").write_text(str(child.pid)) + with pytest.raises(AssertionError, match="Child still running"): + await _assert_child_exited(tmp_path) + child.kill() + # Keep the Popen alive without wait/poll so POSIX retains an exited zombie. + await _assert_child_exited(tmp_path, wait_millis=5000) + finally: + child.kill() + child.wait(timeout=10) + + +def _client(directory: Path, mode: str) -> CopilotClient: + node = shutil.which("node") + assert node is not None, "Node.js is required for the shared SDK shutdown fixture" + return CopilotClient( + connection=RuntimeConnection.for_stdio( + path=node, + args=[ + str(_RUNTIME), + str(directory / "cleanup.jsonl"), + mode, + str(directory / "pid"), + ], + ) + ) + + +@pytest.mark.parametrize("mode", ["stop", "dispose"]) +@pytest.mark.timeout(60) +async def test_graceful_shutdown_waits_for_host_cleanup(tmp_path, mode): + client = _client(tmp_path, mode) + try: + if mode == "dispose": + async with client: + assert not (tmp_path / "cleanup.jsonl").exists() + else: + await client.start() + assert not (tmp_path / "cleanup.jsonl").exists() + await client.stop() + assert (tmp_path / "cleanup.jsonl").read_text() == '{"type":"span"}\n' + await _assert_child_exited(tmp_path) + with pytest.raises(RuntimeError, match="Client is not connected"): + _ = client.rpc + finally: + await client.force_stop() + + +@pytest.mark.timeout(60) +async def test_force_stop_does_not_run_graceful_host_cleanup(tmp_path): + client = _client(tmp_path, "force") + try: + await client.start() + started = time.monotonic() + await asyncio.wait_for(client.force_stop(), timeout=30) + assert time.monotonic() - started < 10 + await _assert_child_exited(tmp_path, wait_millis=5000) + assert not (tmp_path / "cleanup.jsonl").exists() + with pytest.raises(RuntimeError, match="Client is not connected"): + _ = client.rpc + finally: + await client.force_stop() + + +@pytest.mark.timeout(60) +async def test_stop_terminates_uncooperative_child_after_grace_period(tmp_path): + client = _client(tmp_path, "fallback") + try: + await client.start() + started = time.monotonic() + await asyncio.wait_for(client.stop(), timeout=45) + assert time.monotonic() - started >= 10 + assert (tmp_path / "cleanup.jsonl").read_text() == '{"type":"span"}\n' + await _assert_child_exited(tmp_path) + with pytest.raises(RuntimeError, match="Client is not connected"): + _ = client.rpc + finally: + await client.force_stop() + + +@pytest.mark.timeout(60) +async def test_force_stop_cleans_up_after_startup_failure(tmp_path): + client = _client(tmp_path, "start-failure") + try: + with pytest.raises(RuntimeError, match="[Pp]rotocol"): + await client.start() + await asyncio.wait_for(client.force_stop(), timeout=30) + await _assert_child_exited(tmp_path, wait_millis=5000) + assert not (tmp_path / "cleanup.jsonl").exists() + with pytest.raises(RuntimeError, match="Client is not connected"): + _ = client.rpc + finally: + await client.force_stop() diff --git a/rust/README.md b/rust/README.md index 1a3bc2fce5..705162b71a 100644 --- a/rust/README.md +++ b/rust/README.md @@ -51,6 +51,14 @@ Your Application The SDK manages the CLI process lifecycle: spawning, health-checking, and graceful shutdown. Communication uses [JSON-RPC 2.0](https://www.jsonrpc.org/specification) over stdin/stdout with `Content-Length` framing (the same protocol used by LSP). TCP transport is also supported. +Await `client.stop()` to flush host-owned telemetry: after requesting runtime +shutdown, the SDK closes its owned stdio child's stdin and waits up to 10 seconds +for cleanup and exit before falling back to termination. The shutdown RPC and +final process reap each have a separate 10-second bound. `force_stop()` and +dropping the last client remain immediate termination paths, not telemetry-flush +guarantees. External servers and in-process hosts retain their existing shutdown +behavior. + ## API Reference ### Client diff --git a/rust/src/generated/api_types.rs b/rust/src/generated/api_types.rs index 188c80ed17..4868d32b76 100644 --- a/rust/src/generated/api_types.rs +++ b/rust/src/generated/api_types.rs @@ -20383,6 +20383,13 @@ pub struct SessionOpenOptions { #[serde(skip_serializing_if = "Option::is_none")] pub sandbox_config: Option, /// Origin of the sandbox choice. Settings-derived origins (never_configured, user_enabled, user_disabled, repository_policy) let managed policy floor a host preference; explicit below-floor changes remain policy conflicts unless a session opt-out is authorized. Also used for telemetry provenance. + /// + ///

+ /// + /// **Experimental.** This type is part of an experimental wire-protocol surface + /// and may change or be removed in future SDK or CLI releases. + /// + ///
#[serde(skip_serializing_if = "Option::is_none")] pub sandbox_config_source: Option, /// Capabilities enabled for this session. @@ -21885,6 +21892,13 @@ pub struct SessionUpdateOptionsParams { #[serde(skip_serializing_if = "Option::is_none")] pub sandbox_config: Option, /// Origin of the sandbox choice. Settings-derived origins (never_configured, user_enabled, user_disabled, repository_policy) let managed policy floor a host preference; explicit below-floor changes remain policy conflicts unless a session opt-out is authorized. Also used for telemetry provenance. + /// + ///
+ /// + /// **Experimental.** This type is part of an experimental wire-protocol surface + /// and may change or be removed in future SDK or CLI releases. + /// + ///
#[serde(skip_serializing_if = "Option::is_none")] pub sandbox_config_source: Option, /// Replaces the session's capability set with the given list. Use to enable or disable capabilities mid-session (e.g., remove `memory` for reproducible scripted runs). Omit the field to leave the existing capability set unchanged. @@ -40772,7 +40786,7 @@ pub enum ResponseFormatType { JsonSchema, } -/// Origin of the sandbox choice supplied by the host. Settings-derived origins let managed policy floor the host preference; do not tag explicit session overrides as settings-derived. +/// Origin of the sandbox choice supplied by the host. This value describes preference or session intent; it does not authorize bypassing managed policy. /// ///
/// @@ -40791,7 +40805,7 @@ pub enum SandboxConfigSource { /// The user's persisted settings disabled the sandbox. #[serde(rename = "user_disabled")] UserDisabled, - /// A command-line flag selected the sandbox state for this session. + /// An explicit session-scoped choice selected the sandbox state, such as a command-line flag. #[serde(rename = "session_flag")] SessionFlag, /// The user disabled the sandbox for the current session. diff --git a/rust/src/generated/session_events.rs b/rust/src/generated/session_events.rs index e1f798c858..c8b498bcbc 100644 --- a/rust/src/generated/session_events.rs +++ b/rust/src/generated/session_events.rs @@ -7635,9 +7635,15 @@ pub struct SessionMcpServersLoadedData { #[derive(Debug, Clone, Default, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct SessionMcpServerStatusChangedData { + /// Runtime configuration provenance for a failed connection, or unknown when unavailable. Additional string values may be introduced. + #[serde(skip_serializing_if = "Option::is_none")] + pub config_source: Option, /// Error message if the server entered a failed state #[serde(skip_serializing_if = "Option::is_none")] pub error: Option, + /// Runtime-produced classification for the final failed connection; unclassified means no classification was supplied. Additional string values may be introduced. + #[serde(skip_serializing_if = "Option::is_none")] + pub error_classification: Option, /// Name of the MCP server whose status changed pub server_name: String, /// Connection status: connected, failed, needs-auth, pending, disabled, stopped, or not_configured diff --git a/rust/src/jsonrpc.rs b/rust/src/jsonrpc.rs index 9fbe0394bc..90c9c2938d 100644 --- a/rust/src/jsonrpc.rs +++ b/rust/src/jsonrpc.rs @@ -356,10 +356,15 @@ impl JsonRpcClient { if let Some(task) = self.read_task.lock().take() { task.abort(); } + self.close_writer(); + self.pending_requests.write().clear(); + } + + /// Release stdin while continuing to drain the owned child's final stdout. + pub(crate) fn close_writer(&self) { if let Some(task) = self.write_task.lock().take() { task.abort(); } - self.pending_requests.write().clear(); } pub(crate) fn connection_closed_token(&self) -> CancellationToken { diff --git a/rust/src/lib.rs b/rust/src/lib.rs index 4de9cd8bc9..1b797f8855 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -1207,6 +1207,8 @@ impl std::fmt::Debug for Client { struct ClientInner { child: parking_lot::Mutex>, + owns_stdio: bool, + force_stop_requested: tokio_util::sync::CancellationToken, process_tree: parking_lot::Mutex>, #[cfg(feature = "in-process")] /// In-process FFI runtime host, set only for [`Transport::InProcess`]. @@ -1254,6 +1256,19 @@ struct ClientInner { startup_timings: OnceLock, } +struct StdioShutdownGuard<'a> { + client: &'a Client, + armed: bool, +} + +impl Drop for StdioShutdownGuard<'_> { + fn drop(&mut self) { + if self.armed { + self.client.force_stop(); + } + } +} + impl Client { /// Start a CLI server process with the given options. /// @@ -1461,6 +1476,7 @@ impl Client { effective_connection_token.clone(), options.mode, options.client_info, + false, )? } Transport::Tcp { @@ -1495,6 +1511,7 @@ impl Client { effective_connection_token.clone(), options.mode, options.client_info, + false, )? } Transport::Stdio => { @@ -1519,6 +1536,7 @@ impl Client { effective_connection_token.clone(), options.mode, options.client_info, + true, )? } Transport::InProcess => { @@ -1587,6 +1605,7 @@ impl Client { effective_connection_token.clone(), options.mode, options.client_info, + false, )?; *client.inner.ffi_host.lock() = Some(shared); client @@ -1717,6 +1736,7 @@ impl Client { None, ClientMode::default(), None, + false, ) } @@ -1745,6 +1765,7 @@ impl Client { None, ClientMode::default(), None, + false, ) } @@ -1794,6 +1815,7 @@ impl Client { None, ClientMode::default(), None, + false, ) } @@ -1822,6 +1844,7 @@ impl Client { token, ClientMode::default(), None, + false, ) } @@ -1850,6 +1873,7 @@ impl Client { None, ClientMode::default(), None, + false, ) } @@ -1889,6 +1913,7 @@ impl Client { None, ClientMode::default(), client_info, + false, ) } @@ -1910,6 +1935,7 @@ impl Client { effective_connection_token: Option, mode: ClientMode, client_info: Option, + owns_stdio: bool, ) -> Result { let setup_start = Instant::now(); let (request_tx, request_rx) = mpsc::unbounded_channel::(); @@ -1935,6 +1961,8 @@ impl Client { let client = Self { inner: Arc::new(ClientInner { child: parking_lot::Mutex::new(child), + owns_stdio, + force_stop_requested: tokio_util::sync::CancellationToken::new(), process_tree: parking_lot::Mutex::new(process_tree), #[cfg(feature = "in-process")] ffi_host: parking_lot::Mutex::new(None), @@ -2815,8 +2843,10 @@ impl Client { /// Cooperatively shut down the client and the CLI child process. /// /// Walks every still-registered session and sends `session.detach` - /// for each one, asks SDK-owned runtimes to shut down, terminates the - /// Windows-owned CLI Job Object when present, and reaps the root process. + /// for each one and asks SDK-owned runtimes to shut down. For an owned stdio + /// child, closes stdin and waits up to 10 seconds for host cleanup and exit + /// before falling back to termination. Terminates the Windows-owned CLI + /// Job Object when present and bounds the final root-process reap to 10 seconds. /// Errors from per-session detaches, runtime shutdown, and final process /// termination are collected into [`StopErrors`] rather than /// short-circuiting on the first failure — so callers see the full picture @@ -2824,7 +2854,7 @@ impl Client { /// /// If you have already called [`Session::disconnect`] on every /// session this client created, the per-session destroy step is a - /// no-op (the router map is empty); only the child-kill remains. + /// no-op (the router map is empty); runtime and process shutdown still run. /// /// [`Session::disconnect`]: crate::session::Session::disconnect /// @@ -2839,6 +2869,8 @@ impl Client { /// or call `stop()` again with a fresh future. The documented /// `tokio::time::timeout(..., client.stop())` pattern in the example /// below uses `force_stop` as the fallback for exactly this case. + /// Once owned-stdio exit waiting begins, cancelling `stop()` forcibly + /// terminates that child. Concurrent `force_stop()` also interrupts the wait. pub async fn stop(&self) -> std::result::Result<(), StopErrors> { let pid = self.pid(); info!(pid = ?pid, "stopping CLI process"); @@ -2903,10 +2935,43 @@ impl Client { } } - let child = self.inner.child.lock().take(); - let process_tree = self.inner.process_tree.lock().take(); *self.inner.state.lock() = ConnectionState::Disconnected; *self.inner.models_cache.lock() = Arc::new(tokio::sync::OnceCell::new()); + if self.inner.owns_stdio && self.inner.child.lock().is_some() { + let mut guard = StdioShutdownGuard { + client: self, + armed: true, + }; + // The host flushes telemetry after stdin EOF, not after the shutdown RPC. + // Drop ChildStdin but retain the reader and process tree until exit. + self.inner.rpc.close_writer(); + let wait = async { + tokio::select! { + result = std::future::poll_fn(|cx| { + let mut child = self.inner.child.lock(); + let Some(child) = child.as_mut() else { + return std::task::Poll::Ready(Ok(())); + }; + // Child::wait is cancel-safe; retain ownership between polls so + // synchronous force_stop can still terminate the child and its tree. + std::future::Future::poll(std::pin::pin!(child.wait()), cx) + .map(|result| result.map(|_| ())) + }) => result, + _ = self.inner.force_stop_requested.cancelled() => Ok(()), + } + }; + match tokio::time::timeout(RUNTIME_SHUTDOWN_TIMEOUT, wait).await { + Ok(Ok(_)) => {} + Ok(Err(error)) => errors.push(error.into()), + Err(_) => warn!( + timeout = ?RUNTIME_SHUTDOWN_TIMEOUT, + "CLI did not exit after stdin EOF; terminating" + ), + } + guard.armed = false; + } + let child = self.inner.child.lock().take(); + let process_tree = self.inner.process_tree.lock().take(); if let Some(process_tree) = process_tree && let Err(error) = process_tree.terminate() { @@ -2916,14 +2981,16 @@ impl Client { match child.try_wait() { Ok(Some(_status)) => {} Ok(None) => { - // The runtime completes all cleanup before responding to - // runtime.shutdown and then leaves termination to us; it - // deliberately keeps its JSON-RPC server alive to send the - // response and never self-exits. Waiting for a self-exit - // that will never come just wastes time, so terminate the - // child immediately. - if let Err(e) = child.kill().await { - errors.push(e.into()); + match tokio::time::timeout(RUNTIME_SHUTDOWN_TIMEOUT, child.kill()).await { + Ok(Ok(())) => {} + Ok(Err(error)) => errors.push(error.into()), + Err(_) => errors.push( + std::io::Error::new( + std::io::ErrorKind::TimedOut, + "CLI process reap timed out during Client::stop", + ) + .into(), + ), } } Err(e) => errors.push(e.into()), @@ -2991,6 +3058,7 @@ impl Client { { error!(pid = ?pid, error = %e, "failed to send kill signal"); } + self.inner.force_stop_requested.cancel(); self.inner.rpc.force_close(); #[cfg(feature = "in-process")] { @@ -3752,6 +3820,7 @@ mod tests { None, ClientMode::default(), None, + false, ) .unwrap(); @@ -3826,6 +3895,8 @@ mod tests { Client { inner: Arc::new(ClientInner { child: parking_lot::Mutex::new(None), + owns_stdio: false, + force_stop_requested: tokio_util::sync::CancellationToken::new(), process_tree: parking_lot::Mutex::new(None), #[cfg(feature = "in-process")] ffi_host: parking_lot::Mutex::new(None), diff --git a/rust/tests/e2e/client_options.rs b/rust/tests/e2e/client_options.rs index b39506ec02..37b5383583 100644 --- a/rust/tests/e2e/client_options.rs +++ b/rust/tests/e2e/client_options.rs @@ -651,6 +651,252 @@ async fn remote_resource_mismatch_is_observable_before_resume() { ); } +#[tokio::test] +async fn stdio_stop_waits_for_eof_cleanup() { + let fake = ShutdownCli::new("stop"); + let client = Client::start(fake.options()).await.expect("start fake CLI"); + let pid = client.pid().expect("owned child"); + tokio::time::timeout(Duration::from_secs(40), client.stop()) + .await + .expect("shutdown RPC, graceful exit, and reap must be bounded") + .expect("stop client"); + assert_eq!( + std::fs::read_to_string(&fake.marker).expect("EOF cleanup"), + "{\"type\":\"span\"}\n" + ); + assert!(!process_is_alive(pid).await); + client.stop().await.expect("repeated stop"); +} + +#[tokio::test] +async fn stdio_stop_drains_stdout_during_eof_cleanup() { + let fake = FakeCli::new(); + let client = Client::start(fake.client_options_with_behavior("token", "shutdown-output")) + .await + .expect("start fake CLI"); + tokio::time::timeout(Duration::from_secs(40), client.stop()) + .await + .expect("shutdown RPC, graceful exit, and reap must be bounded") + .expect("stop client"); + assert_eq!( + std::fs::read_to_string(fake.capture_path.with_extension("cleanup")) + .expect("cleanup after draining final stdout"), + "flushed" + ); +} + +#[tokio::test] +async fn stdio_stop_preserves_shutdown_error_after_eof_cleanup() { + let fake = FakeCli::new(); + let client = Client::start(fake.client_options_with_behavior("token", "shutdown-error")) + .await + .expect("start fake CLI"); + let errors = tokio::time::timeout(Duration::from_secs(40), client.stop()) + .await + .expect("shutdown RPC, graceful exit, and reap must be bounded") + .expect_err("shutdown error must be returned"); + assert!(errors.to_string().contains("shutdown rejected"), "{errors}"); + assert_eq!( + std::fs::read_to_string(fake.capture_path.with_extension("cleanup")) + .expect("EOF cleanup despite RPC failure"), + "flushed" + ); +} + +#[tokio::test] +async fn stdio_stop_bounds_unresponsive_shutdown_and_eof() { + let fake = FakeCli::new(); + let client = Client::start(fake.client_options_with_behavior("token", "ignore-shutdown")) + .await + .expect("start fake CLI"); + let pid = client.pid().expect("owned child"); + let start = std::time::Instant::now(); + let errors = tokio::time::timeout(Duration::from_secs(40), client.stop()) + .await + .expect("10s RPC + 10s graceful exit + 10s reap must be bounded") + .expect_err("unanswered shutdown must report its timeout"); + assert!(errors.to_string().contains("timed out"), "{errors}"); + assert!(start.elapsed() >= Duration::from_secs(20)); + assert!(!process_is_alive(pid).await); + assert!(!fake.capture_path.with_extension("cleanup").exists()); +} + +#[tokio::test] +async fn stdio_stop_terminates_child_that_does_not_exit_after_eof() { + let fake = ShutdownCli::new("fallback"); + let client = Client::start(fake.options()).await.expect("start fake CLI"); + let pid = client.pid().expect("owned child"); + let start = std::time::Instant::now(); + tokio::time::timeout(Duration::from_secs(40), client.stop()) + .await + .expect("shutdown RPC, graceful exit, and reap must be bounded") + .expect("fallback termination succeeds"); + assert!(start.elapsed() >= Duration::from_secs(10)); + assert!(!process_is_alive(pid).await); + assert!( + fake.marker.exists(), + "fixture must observe shutdown and EOF" + ); + assert_eq!( + std::fs::read_to_string(&fake.marker).expect("EOF cleanup"), + "{\"type\":\"span\"}\n" + ); +} + +#[tokio::test] +async fn stdio_force_stop_interrupts_graceful_exit_wait() { + let fake = ShutdownCli::new("fallback"); + let client = Client::start(fake.options()).await.expect("start fake CLI"); + let pid = client.pid().expect("owned child"); + let stopping = tokio::spawn({ + let client = client.clone(); + async move { client.stop().await } + }); + wait_for_cleanup_marker(&fake.marker).await; + + client.force_stop(); + tokio::time::timeout(Duration::from_secs(5), async { + stopping.await.expect("stop task").expect("stop client"); + wait_for_process_exit(pid).await; + }) + .await + .expect("force stop must interrupt grace before its 10-second deadline"); +} + +#[tokio::test] +async fn stdio_cancelled_graceful_exit_wait_terminates_child() { + let fake = ShutdownCli::new("fallback"); + let client = Client::start(fake.options()).await.expect("start fake CLI"); + let pid = client.pid().expect("owned child"); + let stopping = tokio::spawn({ + let client = client.clone(); + async move { client.stop().await } + }); + wait_for_cleanup_marker(&fake.marker).await; + + stopping.abort(); + assert!( + stopping + .await + .expect_err("stop task cancelled") + .is_cancelled() + ); + wait_for_process_exit(pid).await; + assert!(client.pid().is_none()); +} + +async fn wait_for_cleanup_marker(path: &std::path::Path) { + tokio::time::timeout(Duration::from_secs(5), async { + loop { + match std::fs::read_to_string(path) { + Ok(content) if content == "{\"type\":\"span\"}\n" => return, + Ok(_) => {} + Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} + Err(error) => panic!("read cleanup marker: {error}"), + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + }) + .await + .expect("fixture must reach EOF cleanup during graceful shutdown"); +} + +#[tokio::test] +async fn stdio_force_stop_and_drop_remain_immediate() { + for force in [true, false] { + let fake = ShutdownCli::new("force"); + let client = Client::start(fake.options()).await.expect("start fake CLI"); + let pid = client.pid().expect("owned child"); + if force { + client.force_stop(); + assert!(client.pid().is_none()); + } + drop(client); + wait_for_process_exit(pid).await; + assert!(!fake.marker.exists()); + } +} + +#[tokio::test] +async fn stdio_startup_failure_terminates_owned_child() { + let fake = ShutdownCli::new("start-failure"); + let result = tokio::time::timeout(Duration::from_secs(5), Client::start(fake.options())) + .await + .expect("startup failure must not await graceful shutdown"); + assert!(result.is_err()); + let pid = std::fs::read_to_string(&fake.pid_file) + .expect("read child PID") + .parse() + .expect("parse child PID"); + wait_for_process_exit(pid).await; + assert!(!fake.marker.exists()); +} + +struct ShutdownCli { + dir: TempDir, + marker: PathBuf, + pid_file: PathBuf, + mode: &'static str, +} + +impl ShutdownCli { + fn new(mode: &'static str) -> Self { + let dir = tempfile::tempdir().expect("create shutdown fixture directory"); + Self { + marker: dir.path().join("cleanup.jsonl"), + pid_file: dir.path().join("child.pid"), + dir, + mode, + } + } + + fn options(&self) -> ClientOptions { + let script = PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("../test/harness/stdio-shutdown-runtime.cjs"); + ClientOptions::new() + .with_program(CliProgram::Path("node".into())) + .with_prefix_args([ + script.into_os_string(), + self.marker.clone().into_os_string(), + self.mode.into(), + self.pid_file.clone().into_os_string(), + ]) + .with_cwd(self.dir.path()) + .with_use_logged_in_user(false) + .with_transport(Transport::Stdio) + } +} + +async fn process_is_alive(pid: u32) -> bool { + let output = tokio::process::Command::new("node") + .args([ + "-e", + "try { process.kill(Number(process.argv[1]), 0); } catch (e) { if (e.code === 'ESRCH') process.exit(3); throw e; }", + &pid.to_string(), + ]) + .output() + .await + .expect("query child process"); + match output.status.code() { + Some(0) => true, + Some(3) => false, + _ => panic!( + "query child process: {}", + String::from_utf8_lossy(&output.stderr) + ), + } +} + +async fn wait_for_process_exit(pid: u32) { + tokio::time::timeout(Duration::from_secs(5), async { + while process_is_alive(pid).await { + tokio::time::sleep(Duration::from_millis(10)).await; + } + }) + .await + .expect("owned child must exit promptly"); +} + struct FakeCli { _dir: TempDir, script_path: PathBuf, @@ -776,6 +1022,29 @@ process.stdin.on("data", chunk => { processBuffer(); }); process.stdin.resume(); +if (["ignore-shutdown", "shutdown-error", "shutdown-output"].includes(behavior)) { + const keepAlive = setInterval(() => {}, 1000); + process.stdin.on("end", () => { + if (behavior === "ignore-shutdown") return; + if (behavior === "shutdown-output") { + const body = JSON.stringify({ + jsonrpc: "2.0", + method: "shutdown.output", + params: { output: "x".repeat(1024 * 1024) }, + }); + process.stdout.write(`Content-Length: ${Buffer.byteLength(body)}\r\n\r\n${body}`, error => { + if (error) throw error; + fs.writeFileSync(captureFile.replace(/\.json$/, ".cleanup"), "flushed"); + clearInterval(keepAlive); + }); + return; + } + setTimeout(() => { + fs.writeFileSync(captureFile.replace(/\.json$/, ".cleanup"), "flushed"); + clearInterval(keepAlive); + }, 100); + }); +} function processBuffer() { while (true) { @@ -804,6 +1073,11 @@ function handleMessage(message) { writeResponse(message.id, { ok: true, protocolVersion: 3, version: "fake" }); return; } + if (message.method === "runtime.shutdown" && behavior === "ignore-shutdown") return; + if (message.method === "runtime.shutdown" && behavior === "shutdown-error") { + writeMessage({ jsonrpc: "2.0", id: message.id, error: { code: -32000, message: "shutdown rejected" } }); + return; + } if (message.method === "ping") { writeResponse(message.id, { message: "pong", protocolVersion: 3, timestamp: Date.now() }); return; diff --git a/rust/tests/e2e/rpc_workspace_checkpoints.rs b/rust/tests/e2e/rpc_workspace_checkpoints.rs index dab711b0f8..b781f61b2f 100644 --- a/rust/tests/e2e/rpc_workspace_checkpoints.rs +++ b/rust/tests/e2e/rpc_workspace_checkpoints.rs @@ -5,7 +5,7 @@ use std::sync::Arc; use github_copilot_sdk::ResumeSessionConfig; use github_copilot_sdk::handler::ApproveAllHandler; use github_copilot_sdk::rpc::{ - WorkspaceDiffFileChangeType, WorkspaceDiffMode, WorkspacesDiffRequest, + SessionsSaveRequest, WorkspaceDiffFileChangeType, WorkspaceDiffMode, WorkspacesDiffRequest, WorkspacesReadCheckpointRequest, WorkspacesReadFileRequest, WorkspacesSaveLargePasteRequest, WorkspacesWorkspaceDetailsHostType, }; @@ -189,7 +189,9 @@ fn normalize_path(path: &str) -> String { #[tokio::test] async fn should_record_git_context_in_a_new_session_workspace() { - super::support::with_e2e_context_no_snapshot(|ctx| { + // Reuse session.rs::should_have_stateful_conversation's cassette to create persisted history; + // keep the first-turn prompt below aligned with that owner. + super::support::with_e2e_context("session", "should_have_stateful_conversation", |ctx| { Box::pin(async move { ctx.set_default_copilot_user(); init_git_repository(ctx.work_dir()); @@ -230,23 +232,18 @@ async fn should_record_git_context_in_a_new_session_workspace() { assert_eq!(workspace.repository.as_deref(), Some("test-org/test-repo")); assert_eq!(workspace.branch.as_deref(), Some("feature/test-branch")); - // The repository was created exactly at the session's working - // directory, so the recorded root is that directory and not an - // ancestor of it. - let git_root = normalize_path(workspace.git_root.as_deref().expect("git root")); + // Git may expand Windows 8.3 paths or resolve platform directory aliases. + // Compare directory identities, not the spellings returned by Git and the host. + let work_dir = ctx.work_dir().canonicalize().expect("canonical work dir"); + let git_root = Path::new(workspace.git_root.as_deref().expect("git root")) + .canonicalize() + .expect("canonical git root"); + assert_eq!(git_root, work_dir); assert_eq!( - Some(git_root.as_str()), - workspace.cwd.as_deref().map(normalize_path).as_deref() - ); - let work_dir_name = ctx - .work_dir() - .file_name() - .expect("work dir name") - .to_string_lossy() - .into_owned(); - assert!( - git_root.ends_with(&format!("/{work_dir_name}")), - "git root {git_root} should be the test work directory {work_dir_name}" + Path::new(workspace.cwd.as_deref().expect("workspace cwd")) + .canonicalize() + .expect("canonical workspace cwd"), + work_dir ); assert_eq!( workspace.host_type, @@ -257,6 +254,22 @@ async fn should_record_git_context_in_a_new_session_workspace() { // The recorded context survives a resume rather than being dropped // or re-derived into something else. let session_id = session.id().clone(); + // Empty sessions are not persisted; complete a replay-backed turn before resuming. + let answer = session + .send_and_wait("What is 1+1?") + .await + .expect("send") + .expect("assistant message"); + // Validate the final assistant response arrived (guards against truncated captures). + assert!(super::support::assistant_message_content(&answer).contains('2')); + client + .rpc() + .sessions() + .save(SessionsSaveRequest { + session_id: session_id.clone(), + }) + .await + .expect("persist session before disconnect"); session.disconnect().await.expect("disconnect session"); let resumed = client .resume_session( @@ -282,6 +295,10 @@ async fn should_record_git_context_in_a_new_session_workspace() { resumed_workspace.branch.as_deref(), Some("feature/test-branch") ); + assert_eq!(resumed_workspace.git_root, workspace.git_root); + assert_eq!(resumed_workspace.cwd, workspace.cwd); + assert_eq!(resumed_workspace.host_type, workspace.host_type); + assert_eq!(resumed_workspace.client_name, workspace.client_name); resumed.disconnect().await.expect("disconnect resumed"); client.stop().await.expect("stop client"); diff --git a/test/harness/stdio-shutdown-runtime.cjs b/test/harness/stdio-shutdown-runtime.cjs new file mode 100644 index 0000000000..99d1c8cec2 --- /dev/null +++ b/test/harness/stdio-shutdown-runtime.cjs @@ -0,0 +1,59 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + *--------------------------------------------------------------------------------------------*/ + +// Shared SDK shutdown fixture: node