From dd809e02746a39484dd873b7a8b9b3c3556623e9 Mon Sep 17 00:00:00 2001 From: JUN Date: Mon, 9 Nov 2026 09:37:46 +0900 Subject: [PATCH] fix(copilot): keep Auto negotiation off real DNS in tests and survive a mid-negotiation account switch (#8389) * fix(copilot): keep Auto negotiation off real DNS in account-origin tests The account-origin fixture stubbed globalThis.fetch, but Copilot Auto's /models negotiation runs on providerOutboundGet, which resolves and pins a real peer address before calling the provider executor. Every case therefore performed a real DNS/TLS request to .githubcopilot.com/models, bypassing the fixture. Under CI network latency that request took 5s or 10s, timing out the test in whichever case happened to be negotiating. Hand the fixture's fetch to the provider via the existing stubbedProviderFetch helper, serve /models from the fixture, and reset the Auto catalog cache per case. This also exposed a production bug: when the account changed while the negotiation was queued, beforeSend cancelled the control-plane send and resolveCopilotAuto treated that as fatal. A manual selection change now falls back to the requested model so dispatch can revalidate and rebuild on the new account; a client abort still rejects. Failing-first: the new regression asserts the fixture saw /models and failed with modelsRequests=[] before the provider executor was attached. * fix(copilot): fall back on an account switch during the Auto session and intent sends too (cherry picked from commit 1176557e31e0ca4750768f9bed53213c929fa94b) --- src/providers/github-copilot-auto.ts | 30 +++++++++++++--- .../github-copilot-account-origin.test.ts | 36 +++++++++++++++++-- .../github-copilot-auto.test.ts | 28 +++++++++++++-- 3 files changed, 85 insertions(+), 9 deletions(-) diff --git a/src/providers/github-copilot-auto.ts b/src/providers/github-copilot-auto.ts index 29797761aa6..e42347342be 100644 --- a/src/providers/github-copilot-auto.ts +++ b/src/providers/github-copilot-auto.ts @@ -130,15 +130,32 @@ export async function resolveCopilotAuto(provider: OcxProviderConfig, requestedM let models: CopilotModel[]; try { models = await fetchCopilotAutoModels(transport, signal, beforeSend, 60_000, concurrency); } catch (error) { - if (signal?.aborted || error instanceof ProviderOutboundSendCancelledError) throw error; + if (signal?.aborted) throw error; + // A manual account switch makes `beforeSend` return false, which cancels this + // control-plane send. That is not a client abort: fall back to the requested model + // so the dispatch path revalidates the selection and rebuilds on the new account. + if (error instanceof ProviderOutboundSendCancelledError) { + return { provider: transport, modelId: requestedModel, auto: false }; + } // Permission discovery is additive for legacy paid configurations. A temporary // discovery outage must not turn a previously usable named route into a refusal. return { provider: transport, modelId: requestedModel, auto: false }; } if (!copilotRequiresAuto(provider, models)) return { provider: transport, modelId: requestedModel, auto: false }; - const session = await requestJson(transport, "/models/session", signal, - { auto_mode: { model_hints: ["auto"] } }, undefined, beforeSend, concurrency); + // The same authority fence guards the later control-plane sends: a manual switch that + // lands during the session or intent step falls back too, so dispatch rebuilds on the + // new account instead of failing the whole negotiation. A client abort still rejects. + const fallBackOnSwitch = (error: unknown) => { + if (!signal?.aborted && error instanceof ProviderOutboundSendCancelledError) + return { provider: transport, modelId: requestedModel, auto: false } as const; + throw error; + }; + let session: Awaited>; + try { + session = await requestJson(transport, "/models/session", signal, + { auto_mode: { model_hints: ["auto"] } }, undefined, beforeSend, concurrency); + } catch (error) { return fallBackOnSwitch(error); } const pool = strings(session.available_models); if (typeof session.session_token !== "string" || !session.session_token || session.session_token.length > 32_768 || !/^[\x21-\x7e]+$/.test(session.session_token) || !pool.length) throw new Error("GitHub Copilot Auto session omitted its routing credentials"); @@ -153,8 +170,11 @@ export async function resolveCopilotAuto(provider: OcxProviderConfig, requestedM ? typeof lastUser.content === "string" ? lastUser.content : lastUser.content.flatMap(/** Extract text parts for intent negotiation without forwarding image or other multimodal payloads. */ part => part.type === "text" ? [part.text] : []).join("\n") : ""; - const intent = await requestJson(transport, "/models/session/intent", signal, - { prompt: prompt.slice(0, 32_768), available_models: pool }, session.session_token, beforeSend, concurrency); + let intent: Awaited>; + try { + intent = await requestJson(transport, "/models/session/intent", signal, + { prompt: prompt.slice(0, 32_768), available_models: pool }, session.session_token, beforeSend, concurrency); + } catch (error) { return fallBackOnSwitch(error); } const modelId = strings(intent.candidate_models).find(/** Choose the first server-ranked candidate that belongs to this credential-bound session pool. */ candidate => pool.includes(candidate)); if (!modelId) throw new Error("GitHub Copilot Auto intent returned no eligible model"); diff --git a/tests/providers/github-copilot/github-copilot-account-origin.test.ts b/tests/providers/github-copilot/github-copilot-account-origin.test.ts index e5d704cd9ed..db62fb00594 100644 --- a/tests/providers/github-copilot/github-copilot-account-origin.test.ts +++ b/tests/providers/github-copilot/github-copilot-account-origin.test.ts @@ -7,9 +7,11 @@ import { getAccountSet, saveCredential as persistCredential, setActiveAccount as import { handleResponses as dispatchResponses } from "../../../src/server/responses"; import { saveConfig } from "../../../src/config"; import { setActiveProviderApiKey } from "../../../src/providers/api-keys"; +import { clearCopilotAutoModelsForTests } from "../../../src/providers/github-copilot-auto"; import type { OcxConfig } from "../../../src/types"; import { removeTreeWithRetry } from "../../helpers/remove-tree"; import { acquireOwnedSpendHome } from "../../helpers/owned-spend-home"; +import { stubbedProviderFetch } from "../../helpers/catalog-provider-fetch"; const ACCOUNT_A_ORIGIN = "https://a.githubcopilot.com"; const ACCOUNT_B_ORIGIN = "https://b.githubcopilot.com"; @@ -136,6 +138,9 @@ function config(wire: Wire): OcxConfig { adapter: "openai-chat", authMode: "oauth", baseUrl: "https://api.githubcopilot.com", + // Copilot Auto negotiates /models on the pinned outbound transport. Give it the + // fixture executor so the negotiation never resolves a real peer address. + fetch: stubbedProviderFetch, models: [model], ...(wire === "responses" ? { modelAdapters: { [model]: "openai-responses" } } : {}), }, @@ -209,8 +214,9 @@ function installFetch(options: { switchToAccountId?: string; switchOn: "refresh" | "first-dispatch" | "never"; emptyFirst?: boolean; -}): { dispatches: { origin: string; authorization: string }[] } { +}): { dispatches: { origin: string; authorization: string }[]; modelsRequests: { origin: string }[] } { const dispatches: { origin: string; authorization: string }[] = []; + const modelsRequests: { origin: string }[] = []; let refreshSwitched = false; globalThis.fetch = (async (input, init) => { const url = input instanceof Request ? input.url : String(input); @@ -228,6 +234,13 @@ function installFetch(options: { if (url === GITHUB_USER_URL) return Response.json({ id: 101 }); const parsed = new URL(url); + if (parsed.pathname === "/models") { + modelsRequests.push({ origin: parsed.origin }); + return Response.json({ data: [ + { id: "gpt-4o", model_picker_enabled: true }, + { id: "gpt-5.4", model_picker_enabled: true }, + ] }); + } if (parsed.hostname.endsWith(".githubcopilot.com") || parsed.hostname === "api.githubcopilot.com") { dispatches.push({ origin: parsed.origin, @@ -255,7 +268,7 @@ function installFetch(options: { } return originalFetch(input, init); }) as typeof fetch; - return { dispatches }; + return { dispatches, modelsRequests }; } beforeEach(() => { @@ -275,6 +288,7 @@ beforeEach(() => { )); process.env.OPENCODEX_HOME = home; clearGenericFailoverHealth(); + clearCopilotAutoModelsForTests(); // Dispatches without starting a server, so it takes the spend-journal lease itself. Taken // last because the lease binds the home in effect at the moment it is taken. releaseSpendHome = acquireOwnedSpendHome(); @@ -450,6 +464,12 @@ describe("GitHub Copilot bearer/origin snapshot atomicity", () => { const url = new URL(input instanceof Request ? input.url : String(input)); if (url.hostname === "api.exa.ai") return Response.json({ results: [] }); if (!url.hostname.endsWith(".githubcopilot.com")) throw new Error("Unexpected fixture request"); + if (url.pathname === "/models") { + return Response.json({ data: [ + { id: "gpt-4o", model_picker_enabled: true }, + { id: "gpt-5.4", model_picker_enabled: true }, + ] }); + } sent.push(new Headers(init?.headers).get("authorization") ?? ""); bodies.push(JSON.parse(String(init?.body))); const delta = sent.length === 1 @@ -480,6 +500,18 @@ describe("GitHub Copilot bearer/origin snapshot atomicity", () => { expect(sends).toBe(0); }); + test("Copilot Auto model negotiation is served by the fixture fetch, never real DNS", async () => { + await seedAccounts(); + const observed = installFetch({ wire: "chat", statuses: [200], switchOn: "never" }); + const response = await handleResponses(request("chat"), config("chat"), { model: "", provider: "" }); + await responseText(response); + expect(response.status).toBe(200); + // The negotiation runs on providerOutboundGet, which resolves and pins a real peer + // address unless the provider hands it an executor. This fixture must see the request. + expect(observed.modelsRequests).toEqual([{ origin: ACCOUNT_A_ORIGIN }]); + expect(observed.dispatches).toEqual([{ origin: ACCOUNT_A_ORIGIN, authorization: bearer("copilot-access-a") }]); + }); + for (const wire of ["chat", "responses"] as const) { test(`${wire} revalidates selection after pacing and before physical dispatch`, async () => { const accounts = await seedAccounts(); diff --git a/tests/providers/github-copilot/github-copilot-auto.test.ts b/tests/providers/github-copilot/github-copilot-auto.test.ts index 625f2f1c17a..ef9cf814699 100644 --- a/tests/providers/github-copilot/github-copilot-auto.test.ts +++ b/tests/providers/github-copilot/github-copilot-auto.test.ts @@ -198,13 +198,37 @@ describe("Copilot Auto permissions and negotiation", () => { expect((await resolveCopilotAuto(provider, "named", parsed)).modelId).toBe("named"); expect(calls).toBe(1); }); - test("cancellation and authority guard prevent negotiation sends", async () => { + for (const cancelAt of ["/models/session", "/models/session/intent"] as const) { + test(`an authority change during ${cancelAt} also falls back instead of failing negotiation`, async () => { + const seen: string[] = []; + let allowed = true; + const provider = { authMode: "oauth", baseUrl: "https://api.githubcopilot.com", fetch: (async (url: string | URL | Request) => { + const path = new URL(String(url instanceof Request ? url.url : url)).pathname; + seen.push(path); + if (path.endsWith("/models")) return Response.json({ data: [{ id: "eligible", model_picker_enabled: false, supported_endpoints: ["/responses"] }] }); + if (path.endsWith("/models/session")) return Response.json({ session_token: "private-session", available_models: ["eligible"], expires_at: Math.floor(Date.now() / 1000) + 3600 }); + return Response.json({ candidate_models: ["eligible"] }); + }) as typeof fetch } as OcxProviderConfig; + // The switch lands just before the named control-plane send. + const beforeSend = () => { + if (seen.length === (cancelAt === "/models/session" ? 1 : 2)) allowed = false; + return allowed; + }; + await expect(resolveCopilotAuto(provider, "auto", parsed, undefined, beforeSend)) + .resolves.toMatchObject({ modelId: "auto", auto: false }); + expect(seen.some(path => path.endsWith(cancelAt))).toBe(false); + }); + } + test("cancellation aborts, while an authority change falls back without a negotiation send", async () => { let sends = 0; const provider = { authMode: "oauth", baseUrl: "https://api.githubcopilot.com", fetch: (async () => { sends++; return Response.json({}); }) as typeof fetch } as OcxProviderConfig; await expect(resolveCopilotAuto(provider, "auto", parsed, AbortSignal.abort())).rejects.toThrow(); - await expect(resolveCopilotAuto(provider, "auto", parsed, undefined, () => false)).rejects.toThrow(); + // A manual account switch cancels the queued control-plane send. That is not a client + // abort: the caller revalidates the selection and rebuilds before dispatch. + await expect(resolveCopilotAuto(provider, "auto", parsed, undefined, () => false)) + .resolves.toMatchObject({ modelId: "auto", auto: false }); expect(sends).toBe(0); }); });