diff --git a/.github/workflows/required-checks.yml b/.github/workflows/required-checks.yml index 020ce41969..b204934974 100644 --- a/.github/workflows/required-checks.yml +++ b/.github/workflows/required-checks.yml @@ -26,11 +26,18 @@ jobs: java: ${{ steps.select.outputs.java }} rust: ${{ steps.select.outputs.rust }} steps: + - name: Check out merge group + if: github.event_name == 'merge_group' + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + fetch-depth: 2 + - name: Detect changed paths id: filter - if: github.event_name == 'pull_request' + if: github.event_name == 'pull_request' || github.event_name == 'merge_group' uses: dorny/paths-filter@6852f92c20ea7fd3b0c25de3b5112db3a98da050 # v3 with: + base: ${{ github.event_name == 'merge_group' && github.event.merge_group.base_sha || '' }} predicate-quantifier: every filters: | orchestrator: @@ -90,7 +97,7 @@ jobs: JAVA_CHANGED: ${{ steps.filter.outputs.java }} RUST_CHANGED: ${{ steps.filter.outputs.rust }} run: | - if [[ "$EVENT_NAME" != "pull_request" || "$ORCHESTRATOR_CHANGED" == "true" ]]; then + if [[ "$EVENT_NAME" == "workflow_dispatch" || "$ORCHESTRATOR_CHANGED" == "true" ]]; then for workflow in nodejs python go dotnet java rust; do echo "$workflow=true" >> "$GITHUB_OUTPUT" done diff --git a/dotnet/test/E2E/SessionE2ETests.cs b/dotnet/test/E2E/SessionE2ETests.cs index fab84bc439..288ba1e65f 100644 --- a/dotnet/test/E2E/SessionE2ETests.cs +++ b/dotnet/test/E2E/SessionE2ETests.cs @@ -757,7 +757,10 @@ public async Task Handler_Exception_Does_Not_Halt_Event_Delivery() [Fact] public async Task DisposeAsync_From_Handler_Does_Not_Deadlock() { - var session = await CreateSessionAsync(); + var client = Ctx.CreateClient(); + var session = await Ctx.CreateSessionAsync( + client, + new SessionConfig { OnPermissionRequest = PermissionHandler.ApproveAll }); var disposed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); session.On(evt => @@ -774,7 +777,7 @@ public async Task DisposeAsync_From_Handler_Does_Not_Deadlock() // If this times out, we deadlocked. await disposed.Task.WaitAsync(TimeSpan.FromSeconds(10)); - await Client.ForceStopAsync(); + await client.ForceStopAsync(); } [Fact] diff --git a/dotnet/test/Harness/E2ETestContext.cs b/dotnet/test/Harness/E2ETestContext.cs index 7bdfeca124..6f03cbaf37 100644 --- a/dotnet/test/Harness/E2ETestContext.cs +++ b/dotnet/test/Harness/E2ETestContext.cs @@ -31,6 +31,7 @@ public sealed class E2ETestContext : IAsyncDisposable private readonly object _clientsLock = new(); private readonly List _persistentClients = []; private readonly List _transientClients = []; + private readonly List _testSessions = []; private E2ETestContext(string homeDir, string workDir, string proxyUrl, ReplayProxy proxy, string repoRoot) { @@ -389,23 +390,33 @@ public CopilotClient CreateClient( return client; } - public Task CreateSessionAsync( + public async Task CreateSessionAsync( CopilotClient client, SessionConfig? config = null) { config ??= new SessionConfig(); E2ETestBackendConfiguration.Current.ApplyProvider(config, ProxyUrl); - return client.CreateSessionAsync(config); + var session = await client.CreateSessionAsync(config); + lock (_clientsLock) + { + _testSessions.Add(session); + } + return session; } - public Task ResumeSessionAsync( + public async Task ResumeSessionAsync( CopilotClient client, string sessionId, ResumeSessionConfig? config = null) { config ??= new ResumeSessionConfig(); E2ETestBackendConfiguration.Current.ApplyProvider(config, ProxyUrl); - return client.ResumeSessionAsync(sessionId, config); + var session = await client.ResumeSessionAsync(sessionId, config); + lock (_clientsLock) + { + _testSessions.Add(session); + } + return session; } internal void PrepareForTest() @@ -445,14 +456,29 @@ public async Task CleanupAfterTestAsync() // Per-test cleanup only stops clients created for a specific test. // The shared persistent client and temp directories are cleaned when the fixture is disposed. var errors = new List(); + CopilotSession[] testSessions; CopilotClient[] transientClients; lock (_clientsLock) { + testSessions = [.. _testSessions]; + _testSessions.Clear(); transientClients = [.. _transientClients]; _transientClients.Clear(); } + foreach (var session in testSessions) + { + try + { + await session.DisposeAsync(); + } + catch (Exception ex) when (IsTransientCleanupException(ex)) + { + errors.Add(ex); + } + } + foreach (var client in transientClients) { try @@ -478,15 +504,30 @@ public async Task CleanupAfterTestAsync() public async ValueTask DisposeAsync() { var errors = new List(); + CopilotSession[] testSessions; CopilotClient[] clients; lock (_clientsLock) { + testSessions = [.. _testSessions]; + _testSessions.Clear(); clients = [.. _persistentClients.Concat(_transientClients)]; _persistentClients.Clear(); _transientClients.Clear(); } + foreach (var session in testSessions) + { + try + { + await session.DisposeAsync(); + } + catch (Exception ex) when (IsTransientCleanupException(ex)) + { + errors.Add(ex); + } + } + foreach (var client in clients) { try diff --git a/go/internal/e2e/testharness/context.go b/go/internal/e2e/testharness/context.go index 060997d264..56752c2d74 100644 --- a/go/internal/e2e/testharness/context.go +++ b/go/internal/e2e/testharness/context.go @@ -202,7 +202,9 @@ func NewTestContext(t *testing.T) *TestContext { } t.Cleanup(func() { - ctx.Close(t.Failed()) + if err := ctx.Close(t.Failed()); err != nil { + t.Errorf("Failed to close E2E test context: %v", err) + } }) return ctx @@ -266,11 +268,17 @@ func (c *TestContext) ConfigureWithoutSnapshot(t *testing.T) { } // Close cleans up the test context resources. -func (c *TestContext) Close(testFailed bool) { +func (c *TestContext) Close(testFailed bool) error { + if c.inProcess { + if err := waitForInProcessCleanup(); err != nil { + return err + } + } c.restoreInProcessEnvironment() + var proxyErr error if c.proxy != nil { if err := c.proxy.StopWithOptions(testFailed); err != nil { - fmt.Fprintf(os.Stderr, "Failed to stop E2E proxy: %v\n", err) + proxyErr = fmt.Errorf("failed to stop E2E proxy: %w", err) } } if c.HomeDir != "" { @@ -279,6 +287,7 @@ func (c *TestContext) Close(testFailed bool) { if c.WorkDir != "" { os.RemoveAll(c.WorkDir) } + return proxyErr } // applyInProcessEnvironment mirrors the isolated test environment onto the real diff --git a/go/internal/e2e/testharness/inprocess_cleanup_disabled.go b/go/internal/e2e/testharness/inprocess_cleanup_disabled.go new file mode 100644 index 0000000000..404bcd9285 --- /dev/null +++ b/go/internal/e2e/testharness/inprocess_cleanup_disabled.go @@ -0,0 +1,7 @@ +//go:build !copilot_inprocess || (!darwin && !linux && !windows) + +package testharness + +func waitForInProcessCleanup() error { + return nil +} diff --git a/go/internal/e2e/testharness/inprocess_cleanup_enabled.go b/go/internal/e2e/testharness/inprocess_cleanup_enabled.go new file mode 100644 index 0000000000..c65ae15324 --- /dev/null +++ b/go/internal/e2e/testharness/inprocess_cleanup_enabled.go @@ -0,0 +1,18 @@ +//go:build copilot_inprocess && (darwin || linux || windows) + +package testharness + +import ( + "fmt" + "time" + + "github.com/github/copilot-sdk/go/internal/ffihost" +) + +func waitForInProcessCleanup() error { + const timeout = 10 * time.Second + if !ffihost.WaitForCleanup(timeout) { + return fmt.Errorf("timed out after %s waiting for deferred in-process cleanup", timeout) + } + return nil +} diff --git a/go/internal/ffihost/ffihost.go b/go/internal/ffihost/ffihost.go index 4add70cb5d..afdfd7c89f 100644 --- a/go/internal/ffihost/ffihost.go +++ b/go/internal/ffihost/ffihost.go @@ -76,6 +76,55 @@ var ( nextOutboundToken atomic.Uint64 ) +var pendingCleanup = struct { + sync.Mutex + count int + idle chan struct{} +}{ + idle: closedChannel(), +} + +func closedChannel() chan struct{} { + ch := make(chan struct{}) + close(ch) + return ch +} + +func beginPendingCleanup() { + pendingCleanup.Lock() + defer pendingCleanup.Unlock() + if pendingCleanup.count == 0 { + pendingCleanup.idle = make(chan struct{}) + } + pendingCleanup.count++ +} + +func finishPendingCleanup() { + pendingCleanup.Lock() + defer pendingCleanup.Unlock() + pendingCleanup.count-- + if pendingCleanup.count == 0 { + close(pendingCleanup.idle) + } +} + +// WaitForCleanup waits for all deferred connection cleanup to finish. +// In-process test harnesses use this before changing process-global state. +func WaitForCleanup(timeout time.Duration) bool { + pendingCleanup.Lock() + idle := pendingCleanup.idle + pendingCleanup.Unlock() + + timer := time.NewTimer(timeout) + defer timer.Stop() + select { + case <-idle: + return true + case <-timer.C: + return false + } +} + func sharedOutboundCallback() uintptr { outboundCallbackOnce.Do(func() { outboundCallbackHandle = purego.NewCallback(routeOutbound) @@ -374,7 +423,9 @@ func (h *Host) scheduleCleanupRetryLocked() { return } h.cleanupScheduled = true + beginPendingCleanup() go func() { + defer finishPendingCleanup() timer := time.NewTimer(100 * time.Millisecond) defer timer.Stop() for range timer.C { diff --git a/go/internal/ffihost/ffihost_test.go b/go/internal/ffihost/ffihost_test.go index 3bb7555a6f..49d4a1311b 100644 --- a/go/internal/ffihost/ffihost_test.go +++ b/go/internal/ffihost/ffihost_test.go @@ -56,15 +56,13 @@ func TestDisposeRetainsOutboundTargetUntilConnectionCloseSucceeds(t *testing.T) if got := shutdownCalls.Load(); got != 0 { t.Fatalf("Expected host shutdown to be deferred, got %d calls", got) } + if WaitForCleanup(20 * time.Millisecond) { + t.Fatal("Expected cleanup wait to remain blocked before connection close succeeds") + } allowClose.Store(true) - deadline := time.Now().Add(5 * time.Second) - for time.Now().Before(deadline) { - _, registered := outboundTargets.Load(token) - if !registered && shutdownCalls.Load() == 1 { - break - } - time.Sleep(10 * time.Millisecond) + if !WaitForCleanup(5 * time.Second) { + t.Fatal("Timed out waiting for deferred cleanup") } if _, ok := outboundTargets.Load(token); ok { diff --git a/nodejs/test/rust-codegen.test.ts b/nodejs/test/rust-codegen.test.ts new file mode 100644 index 0000000000..577ace1dc0 --- /dev/null +++ b/nodejs/test/rust-codegen.test.ts @@ -0,0 +1,43 @@ +import type { ApiSchema } from "../../scripts/codegen/utils.ts"; +import { describe, expect, it } from "vitest"; + +import { generateApiTypesCode } from "../../scripts/codegen/rust.ts"; + +describe("Rust API type codegen", () => { + it("distinguishes a protocol-defined unknown value from the forward-compatible fallback", () => { + const code = generateApiTypesCode({ + definitions: { + CatalogTrustEligibility: { + type: "string", + enum: ["default", "expanded", "hidden", "unknown"], + }, + }, + } as ApiSchema); + + expect(code).toContain(`#[serde(rename = "unknown")] + UnknownValue, + /// Unknown variant for forward compatibility. + #[default] + #[serde(other)] + Unknown,`); + }); + + it("publicly re-exports API types moved into the shared session-events schema", () => { + const code = generateApiTypesCode({ + definitions: { + PermissionDecision: { + type: "object", + required: ["source"], + properties: { + source: { + $ref: "session-events.schema.json#/definitions/PermissionDecisionSource", + }, + }, + }, + }, + } as ApiSchema); + + expect(code).toContain("pub use super::session_events::{PermissionDecisionSource};"); + expect(code).toContain("use crate::types::{RequestId, SessionId};"); + }); +}); diff --git a/rust/src/generated/api_types.rs b/rust/src/generated/api_types.rs index 7500c30745..6af82a11f6 100644 --- a/rust/src/generated/api_types.rs +++ b/rust/src/generated/api_types.rs @@ -9,7 +9,7 @@ use std::collections::HashMap; use serde::{Deserialize, Serialize}; -use super::session_events::{ +pub use super::session_events::{ AbortReason, AgentModelPolicy, AutoTier, ContextTier, McpOauthHttpResponse, McpOauthWWWAuthenticateParams, McpServerMetadata, McpServerSource, McpServerStatus, ModelChangeSource, OmittedBinaryOmittedReason, PermissionMode, PermissionPromptRequest, diff --git a/scripts/codegen/rust.ts b/scripts/codegen/rust.ts index b3cc5d5753..03ac5adcec 100644 --- a/scripts/codegen/rust.ts +++ b/scripts/codegen/rust.ts @@ -85,6 +85,12 @@ const STRING_NEWTYPE_OVERRIDES: Record = { requestId: "RequestId", }; +const STRING_ENUM_VARIANT_OVERRIDES: Record> = { + CatalogTrustEligibility: { + unknown: "UnknownValue", + }, +}; + // ── Naming helpers ────────────────────────────────────────────────────────── function toPascalCase(s: string): string { @@ -115,8 +121,9 @@ function uniqueRustPascalIdentifier( used: Set, fallback: string, reserved: Set = new Set(), + override?: string, ): string { - const identifier = toRustPascalIdentifier(value, fallback); + const identifier = override ?? toRustPascalIdentifier(value, fallback); if (used.has(identifier) || reserved.has(identifier)) { throw new Error( `Generated Rust enum variant identifier "${identifier}" is not unique for value "${value}". Add an explicit naming rule instead of stabilizing an arbitrary public variant name.`, @@ -1045,6 +1052,7 @@ function emitRustStringEnum( usedVariantNames, "Value", reservedVariantNames, + STRING_ENUM_VARIANT_OVERRIDES[enumName]?.[value], ); pushRustDoc(lines, enumValueDescriptions?.[value], " "); if (variantName !== value) { @@ -1449,7 +1457,7 @@ function isNullableParamsSchema( return !!resolved && !!getNullableInner(resolved); } -function generateApiTypesCode( +export function generateApiTypesCode( apiSchema: ApiSchema, nonDefaultableTypes: Iterable = [], ): string { @@ -1680,7 +1688,11 @@ function generateApiTypesCode( for (const [module, typeNames] of [...externalImports].sort(([left], [right]) => left.localeCompare(right), )) { - out.push(`use ${module}::{${[...typeNames].sort().join(", ")}};`); + // Preserve API module paths when a definition moves into a shared schema. + const importKeyword = Object.values(EXTERNAL_SCHEMA_RUST_MODULE).includes(module) + ? "pub use" + : "use"; + out.push(`${importKeyword} ${module}::{${[...typeNames].sort().join(", ")}};`); } out.push(""); diff --git a/test/snapshots/session/should_accept_blob_attachments.yaml b/test/snapshots/session/should_accept_blob_attachments.yaml index 4caf7c8707..14fc888cfe 100644 --- a/test/snapshots/session/should_accept_blob_attachments.yaml +++ b/test/snapshots/session/should_accept_blob_attachments.yaml @@ -10,55 +10,4 @@ conversations: test-pixel.png [image] - role: assistant - content: I'll view the image file to describe it for you. - - role: assistant - tool_calls: - - id: toolcall_0 - type: function - function: - name: report_intent - arguments: '{"intent":"Viewing image file"}' - - role: assistant - tool_calls: - - id: toolcall_1 - type: function - function: - name: view - arguments: '{"path":"${workdir}/test-pixel.png"}' - - messages: - - role: system - content: ${system} - - role: user - content: |- - Describe this image - test-pixel.png - [image] - - role: assistant - content: I'll view the image file to describe it for you. - tool_calls: - - id: toolcall_0 - type: function - function: - name: report_intent - arguments: '{"intent":"Viewing image file"}' - - id: toolcall_1 - type: function - function: - name: view - arguments: '{"path":"${workdir}/test-pixel.png"}' - - role: tool - tool_call_id: toolcall_0 - content: Tool 'report_intent' does not exist. Available tools that can be called are ${shell}, ${read_shell}, - ${stop_shell}, ${list_shell}, view, create, edit, web_fetch, skill, sql, read_agent, list_agents, grep, glob, - task. - - role: tool - tool_call_id: toolcall_1 - content: Viewed image file successfully. - - role: user - content: |- - Image file at path ${workdir}/test-pixel.png - [image] - - role: assistant - content: This is a very small image - essentially a **single yellow/gold pixel** or a tiny square. It appears to be a - minimal test image, likely 1x1 pixel in size, which matches its filename "test-pixel.png". The color is a - bright yellow or golden hue. + content: This is a one-pixel test image.