From 8ed79aab02883ece4ecaaa3b2a5041126bd6bddc Mon Sep 17 00:00:00 2001 From: New Future Date: Wed, 26 Aug 2026 16:13:37 +0800 Subject: [PATCH] fix(github-copilot): preserve reasoning during tool calls (#112659) * fix(github-copilot): preserve active encrypted reasoning Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * fix(github-copilot): normalize null reasoning status Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * test(github-copilot): cover adjacent reasoning cleanup Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Peter Steinberger --- .../connection-bound-ids.test.ts | 75 +++++++--------- .../github-copilot/connection-bound-ids.ts | 41 +++++++-- extensions/github-copilot/index.test.ts | 16 +++- .../github-copilot/replay-policy.test.ts | 13 ++- extensions/github-copilot/replay-policy.ts | 2 +- extensions/github-copilot/stream.test.ts | 85 ++++++++++++------- extensions/github-copilot/stream.ts | 2 +- 7 files changed, 146 insertions(+), 88 deletions(-) diff --git a/extensions/github-copilot/connection-bound-ids.test.ts b/extensions/github-copilot/connection-bound-ids.test.ts index 985bc15aa5fa..d63725ca1857 100644 --- a/extensions/github-copilot/connection-bound-ids.test.ts +++ b/extensions/github-copilot/connection-bound-ids.test.ts @@ -22,7 +22,7 @@ describe("github-copilot connection-bound response IDs", () => { const functionCallId = Buffer.from(`function-call-${"y".repeat(20)}`).toString("base64"); const messageId = Buffer.from(`message-${"z".repeat(24)}`).toString("base64"); const input = [ - { id: "rs_existing", type: "reasoning" }, + { id: "rs_existing", type: "reasoning", encrypted_content: "active" }, { id: "msg_existing", type: "message" }, { id: "fc_existing", type: "function_call" }, { id: functionCallId, type: "function_call" }, @@ -37,53 +37,42 @@ describe("github-copilot connection-bound response IDs", () => { expect(input[4]?.id).toMatch(/^msg_[a-f0-9]{16}$/); }); - it("preserves valid reasoning IDs but strips encrypted_content", () => { - const withEncrypted = Buffer.from(`reasoning-${"e".repeat(24)}`).toString("base64"); - const withNull = Buffer.from(`reasoning-${"n".repeat(24)}`).toString("base64"); - const withoutField = Buffer.from(`reasoning-${"a".repeat(24)}`).toString("base64"); + it("preserves complete active reasoning and omits connection-bound IDs", () => { + const connectionBoundId = Buffer.from(`reasoning-${"e".repeat(24)}`).toString("base64"); const input = [ - { id: withEncrypted, type: "reasoning", encrypted_content: "opaque-encrypted-payload" }, - { id: withNull, type: "reasoning", encrypted_content: null }, - { id: withoutField, type: "reasoning" }, - ]; - - expect(rewriteInputIds(input)).toBe(true); - expect(input[0]).toEqual({ id: withEncrypted, type: "reasoning" }); - expect(input[1]).toEqual({ id: withNull, type: "reasoning" }); - expect(input[2]).toEqual({ id: withoutField, type: "reasoning" }); - }); - - it("strips encrypted_content from valid reasoning IDs at payload send time", () => { - const withEncrypted = "abcDEF0123+/="; - const withoutEncrypted = "reasoning/abc+123="; - const input = [ - { id: withEncrypted, type: "reasoning", encrypted_content: "opaque-encrypted-payload" }, - { id: withoutEncrypted, type: "reasoning" }, - ]; - - expect(rewriteInputIds(input)).toBe(true); - expect(input[0]).toEqual({ id: withEncrypted, type: "reasoning" }); - expect(input[1]).toEqual({ id: withoutEncrypted, type: "reasoning" }); - }); - - it("drops unsafe reasoning IDs and strips encrypted_content from kept items", () => { - const overlongId = `5PX6gLHXT5wE+Y2tPmUV4gn+${"B".repeat(384)}`; - const input = [ - { - id: overlongId, - type: "reasoning", - encrypted_content: "encrypted-replay-payload", - summary: [], - }, - { type: "reasoning", encrypted_content: "missing-id", summary: [] }, - { id: 123, type: "reasoning", encrypted_content: "non-string-id", summary: [] }, - { id: "rs_valid", type: "reasoning", encrypted_content: "valid", summary: [] }, + { id: "rs_active", type: "reasoning", encrypted_content: "native", summary: [] }, + { type: "reasoning", status: "completed", encrypted_content: "idless" }, + { id: connectionBoundId, type: "reasoning", status: null, encrypted_content: "connection" }, ]; expect(rewriteInputIds(input)).toBe(true); expect(input).toEqual([ - { type: "reasoning", summary: [] }, - { id: "rs_valid", type: "reasoning", summary: [] }, + { id: "rs_active", type: "reasoning", encrypted_content: "native", summary: [] }, + { type: "reasoning", status: "completed", encrypted_content: "idless" }, + { type: "reasoning", encrypted_content: "connection" }, + ]); + expect(rewriteInputIds(input)).toBe(false); + }); + + it("drops adjacent incomplete or foreign reasoning and only clears dependent assistant IDs", () => { + const invalidReasoning = [ + { id: "thinking_0", type: "reasoning", encrypted_content: "foreign" }, + { id: `rs_${"r".repeat(64)}`, type: "reasoning", encrypted_content: "oversized" }, + { id: "rs_missing", type: "reasoning" }, + { id: "rs_empty", type: "reasoning", encrypted_content: "" }, + { id: "rs_i", type: "reasoning", status: "incomplete", encrypted_content: "partial" }, + { id: 123, type: "reasoning", encrypted_content: "malformed" }, + ]; + const input: Array> = [ + ...invalidReasoning, + { id: "msg_signed", type: "message", role: "assistant" }, + { id: "msg_user", type: "message", role: "user" }, + ]; + + expect(rewriteInputIds(input)).toBe(true); + expect(input).toEqual([ + { type: "message", role: "assistant" }, + { id: "msg_user", type: "message", role: "user" }, ]); }); diff --git a/extensions/github-copilot/connection-bound-ids.ts b/extensions/github-copilot/connection-bound-ids.ts index 419cf7c33dba..ed4452732b17 100644 --- a/extensions/github-copilot/connection-bound-ids.ts +++ b/extensions/github-copilot/connection-bound-ids.ts @@ -31,7 +31,20 @@ function isInputItem(value: unknown): value is InputItem { } function isValidReasoningReplayId(id: unknown): id is string { - return typeof id === "string" && id.length > 0 && id.length <= 64; + return typeof id === "string" && id.length <= 64 && /^rs_[A-Za-z0-9_-]+$/.test(id); +} + +function dropReasoningItem(input: unknown[], index: number): void { + input.splice(index, 1); + const dependentMessage = input[index]; + // Assistant replay IDs are signed with preceding reasoning; keeping one after a drop is invalid. + if ( + isInputItem(dependentMessage) && + dependentMessage.type === "message" && + dependentMessage.role === "assistant" + ) { + delete dependentMessage.id; + } } function sanitizeCopilotReplayResponseIds(input: unknown): boolean { @@ -39,21 +52,33 @@ function sanitizeCopilotReplayResponseIds(input: unknown): boolean { return false; } let rewrote = false; + // Walk backward because dropping reasoning splices input and must not skip adjacent items. for (let index = input.length - 1; index >= 0; index -= 1) { const item = input[index]; if (!isInputItem(item)) { continue; } const id = item.id; - // Reasoning encrypted_content is tied to the Copilot connection token, - // which rotates per request. Drop items with unsafe IDs; strip - // encrypted_content from kept items so summary-only replay is sent. if (item.type === "reasoning") { - if (id !== undefined && !isValidReasoningReplayId(id)) { - input.splice(index, 1); + // Cold reasoning is removed earlier; normalize null status and never synthesize active IDs. + if (item.status === null) { + delete item.status; rewrote = true; - } else if ("encrypted_content" in item) { - delete item.encrypted_content; + } + const isComplete = + typeof item.encrypted_content === "string" && + item.encrypted_content.length > 0 && + (item.status === undefined || item.status === "completed"); + if (!isComplete) { + dropReasoningItem(input, index); + rewrote = true; + } else if (id === undefined || isValidReasoningReplayId(id)) { + continue; + } else if (typeof id === "string" && looksLikeConnectionBoundId(id)) { + delete item.id; + rewrote = true; + } else { + dropReasoningItem(input, index); rewrote = true; } continue; diff --git a/extensions/github-copilot/index.test.ts b/extensions/github-copilot/index.test.ts index 90c7fab7bcc2..9fd0e348be08 100644 --- a/extensions/github-copilot/index.test.ts +++ b/extensions/github-copilot/index.test.ts @@ -332,7 +332,7 @@ describe("github-copilot plugin", () => { }); }); - it("owns Claude replay thinking cleanup", () => { + it("owns session-bound replay thinking cleanup", () => { const provider = registerProviderWithPluginConfig({}); const messages = [ { @@ -368,8 +368,20 @@ describe("github-copilot plugin", () => { ]); expect( provider.sanitizeReplayHistory?.({ - modelId: "gpt-5.4", modelApi: "openai-responses", + modelId: "gpt-5.4", + messages, + } as never), + ).toEqual([ + { + role: "assistant", + content: [{ type: "text", text: "visible" }], + }, + ]); + expect( + provider.sanitizeReplayHistory?.({ + modelApi: "openai-completions", + modelId: "gpt-5.4", messages, } as never), ).toBe(messages); diff --git a/extensions/github-copilot/replay-policy.test.ts b/extensions/github-copilot/replay-policy.test.ts index cec1cd7a68d5..2f4075496093 100644 --- a/extensions/github-copilot/replay-policy.test.ts +++ b/extensions/github-copilot/replay-policy.test.ts @@ -87,7 +87,7 @@ describe("sanitizeGithubCopilotReplayHistory", () => { ]); }); - it("passes history through on OpenAI-compatible transports", () => { + it("strips replayed thinking on the OpenAI Responses transport", () => { expect( sanitizeGithubCopilotReplayHistory({ provider: "github-copilot", @@ -95,6 +95,17 @@ describe("sanitizeGithubCopilotReplayHistory", () => { modelId: "gpt-5.4", messages, } as never), + ).toEqual([{ role: "assistant", content: [{ type: "text", text: "visible" }] }]); + }); + + it("passes history through on the OpenAI Completions transport", () => { + expect( + sanitizeGithubCopilotReplayHistory({ + provider: "github-copilot", + modelApi: "openai-completions", + modelId: "gpt-5.4", + messages, + } as never), ).toBe(messages); }); }); diff --git a/extensions/github-copilot/replay-policy.ts b/extensions/github-copilot/replay-policy.ts index 62f0ec2a9f1f..06a2c68231d1 100644 --- a/extensions/github-copilot/replay-policy.ts +++ b/extensions/github-copilot/replay-policy.ts @@ -66,7 +66,7 @@ export function buildGithubCopilotReplayPolicy( } export function sanitizeGithubCopilotReplayHistory(ctx: ProviderSanitizeReplayHistoryContext) { - return isCopilotAnthropicTransport(ctx.modelApi) + return ctx.modelApi === "openai-responses" || isCopilotAnthropicTransport(ctx.modelApi) ? stripCopilotAssistantThinkingMessages(ctx.messages) : ctx.messages; } diff --git a/extensions/github-copilot/stream.test.ts b/extensions/github-copilot/stream.test.ts index 33dac38a3a5a..e06f5783d998 100644 --- a/extensions/github-copilot/stream.test.ts +++ b/extensions/github-copilot/stream.test.ts @@ -8,6 +8,8 @@ import { describe, expect, it, vi } from "vitest"; import { COPILOT_RUNTIME_INTEGRATION_ID } from "./runtime-identity.js"; import { wrapCopilotAnthropicStream, wrapCopilotProviderStream } from "./stream.js"; +type ResponsesTestPayload = { input: Array> }; + function requireStreamFn(streamFn: ReturnType) { expect(streamFn).toBeTypeOf("function"); if (!streamFn) { @@ -591,20 +593,21 @@ describe("wrapCopilotAnthropicStream", () => { it("adds Copilot headers, sanitizes reasoning replay, and rewrites message IDs before payload send", () => { const reasoningId = Buffer.from(`reasoning-${"x".repeat(24)}`).toString("base64"); - const overlongReasoningId = `5PX6gLHXT5wE+Y2tPmUV4gn+${"B".repeat(384)}`; const messageId = Buffer.from(`message-${"y".repeat(24)}`).toString("base64"); - const payloads: Array<{ input: Array> }> = []; + const payloads: ResponsesTestPayload[] = []; const baseStreamFn = vi.fn((_model, _context, options) => { const payload = { input: [ + { id: "rs_active", type: "reasoning", encrypted_content: "native-encrypted" }, + { type: "reasoning", status: null, encrypted_content: "idless-encrypted", summary: [] }, { id: reasoningId, type: "reasoning", encrypted_content: "valid-encrypted-payload" }, - { type: "reasoning", encrypted_content: "idless-encrypted-payload", summary: [] }, { - id: overlongReasoningId, + id: "thinking_0", type: "reasoning", encrypted_content: "invalid-encrypted-payload", summary: [], }, + { id: "msg_signed", type: "message", role: "assistant" }, { id: messageId, type: "message" }, ], }; @@ -649,45 +652,63 @@ describe("wrapCopilotAnthropicStream", () => { }, onPayload: options.onPayload, }); - expect(payloads[0]?.input[0]?.id).toBe(reasoningId); + expect(payloads[0]?.input[0]?.id).toBe("rs_active"); expect(payloads[0]?.input.map((item) => item.type)).toEqual([ "reasoning", "reasoning", + "reasoning", + "message", "message", ]); expect(payloads[0]?.input[1]?.id).toBeUndefined(); - expect(payloads[0]?.input[2]?.id).toMatch(/^msg_[a-f0-9]{16}$/); - expect(payloads[0]?.input[0]).not.toHaveProperty("encrypted_content"); - expect(payloads[0]?.input[1]).not.toHaveProperty("encrypted_content"); + expect(payloads[0]?.input[2]?.id).toBeUndefined(); + expect(payloads[0]?.input[3]?.id).toBeUndefined(); + expect(payloads[0]?.input[4]?.id).toMatch(/^msg_[a-f0-9]{16}$/); + expect(payloads[0]?.input.slice(0, 3).every((item) => item.encrypted_content)).toBe(true); }); - it("rewrites Copilot Responses IDs returned by an existing payload hook", async () => { - const connectionBoundId = Buffer.from(`message-${"y".repeat(24)}`).toString("base64"); - let returnedPayload: unknown; - const baseStreamFn = vi.fn(async (_model, _context, options) => { - returnedPayload = await options?.onPayload?.({ input: [] }, _model); - return { - async *[Symbol.asyncIterator]() {}, - } as never; - }); + it("sanitizes all sync and async Copilot Responses hook outcomes", async () => { + const model = { + provider: "github-copilot", + api: "openai-responses", + id: "gpt-5.4", + } as never; + const context = { messages: [{ role: "user", content: "hi" }] } as never; - const wrapped = requireStreamFn(wrapCopilotProviderStream({ streamFn: baseStreamFn } as never)); + for (const asyncHook of [false, true]) { + for (const replace of [false, true]) { + const connectionBoundId = Buffer.from(`message-${"y".repeat(24)}`).toString("base64"); + let originalPayload: ResponsesTestPayload = { input: [] }; + let hookResult: unknown; + const baseStreamFn = vi.fn((_model, _context, options) => { + originalPayload = { input: [] }; + hookResult = options?.onPayload?.(originalPayload, _model); + return { + async *[Symbol.asyncIterator]() {}, + } as never; + }); - await wrapped( - { - provider: "github-copilot", - api: "openai-responses", - id: "gpt-5.4", - } as never, - { messages: [{ role: "user", content: "hi" }] } as never, - { - onPayload: () => ({ input: [{ id: connectionBoundId, type: "message" }] }), - } as never, - ); + const wrapped = requireStreamFn( + wrapCopilotProviderStream({ streamFn: baseStreamFn } as never), + ); + const mutatePayload = (payload: ResponsesTestPayload) => { + const target: ResponsesTestPayload = replace ? { input: [] } : payload; + target.input.push({ id: connectionBoundId, type: "message" }); + return replace ? target : undefined; + }; + const onPayload = asyncHook + ? async (payload: ResponsesTestPayload) => mutatePayload(payload) + : (payload: ResponsesTestPayload) => mutatePayload(payload); - expect((returnedPayload as { input: Array> }).input[0]?.id).toMatch( - /^msg_[a-f0-9]{16}$/, - ); + void wrapped(model, context, { onPayload } as never); + + expect(hookResult instanceof Promise).toBe(asyncHook); + const replacement = await hookResult; + expect(replacement !== undefined).toBe(replace); + const returnedPayload = (replacement ?? originalPayload) as ResponsesTestPayload; + expect(returnedPayload.input[0]?.id).toMatch(/^msg_[a-f0-9]{16}$/); + } + } }); it("adds Copilot headers for Chat Completions models", () => { diff --git a/extensions/github-copilot/stream.ts b/extensions/github-copilot/stream.ts index 63286a356da1..3cd8b88e912a 100644 --- a/extensions/github-copilot/stream.ts +++ b/extensions/github-copilot/stream.ts @@ -236,7 +236,7 @@ function wrapCopilotOpenAIResponsesStream( headers: buildCopilotRequestHeaders(context, options?.headers), onPayload: (payload, payloadModel) => { sanitizeCopilotReplayResponsePayload(payload); - return patchOnPayloadResult(originalOnPayload?.(payload, payloadModel)); + return patchOnPayloadResult(originalOnPayload?.(payload, payloadModel), undefined, payload); }, }; return underlying(model, context, wrappedOptions);