Skip to content

Commit 042b08f

Browse files
KarthickMahalingamKarthickMahalingam
committed
fix(api): address review feedback on GitHub Copilot provider
Key in-flight sign-ins by forceNewSession so Reconnect is not answered by a plain sign-in. Cap the pre-report context window at the enforced input limit. Treat vision as unreported unless the host reports it. Drop the unenforceable per-model tool threshold. Add a visual snapshot for the provider settings.
1 parent 82a5ebe commit 042b08f

19 files changed

Lines changed: 219 additions & 127 deletions

File tree

‎README.md‎

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -69,16 +69,15 @@ Capabilities come from what VS Code reports for each Copilot model, so new model
6969
changes, and nothing about a model is saved into your settings where it could go stale.
7070

7171
- **Context window:** the model's reported input limit, capped by Zoo Code's curated limit where one
72-
is known.
73-
- **Images:** read from the host's reported capability, falling back to Zoo Code's curated catalog
74-
only when the host reports nothing. A capability that neither states is shown as **Image support
75-
not reported** rather than assumed unsupported. Images are sent as image data, not text
76-
placeholders, and need a VS Code version that supports image parts; Zoo Code's minimum supported
77-
VS Code version stays 1.100.
72+
is known. Before the host reports, the curated limit is shown, matching what Zoo Code enforces.
73+
- **Images:** read from the capability the host reports for each live model. A model that reports
74+
nothing shows **Image support not reported** instead of being assumed unsupported or borrowed from
75+
Zoo Code's curated catalog, which only describes a model the host has not listed yet. Images are
76+
sent as image data, not text placeholders, and need a VS Code version that supports image parts;
77+
Zoo Code's minimum supported VS Code version stays 1.100.
7878
- **Tools:** VS Code tells extensions only whether a model supports tools, not how many it accepts,
79-
so Zoo Code cannot enforce a per-model limit. The same advisory shown for any provider applies when
80-
many MCP tools are enabled. GitHub plan limits, premium-request allowances, organization policies,
81-
and backend limits still apply.
79+
so Zoo Code applies the same advisory it shows for any provider when many MCP tools are enabled.
80+
GitHub plan limits, premium-request allowances, organization policies, and backend limits still apply.
8281
- **Prompt caching:** Copilot owns any backend caching. The public VS Code Language Model API exposes
8382
no cache-control directives or cache-hit usage, so Zoo Code neither enables caching nor reports
8483
savings; the provider-managed label states this and does not promise a cache hit.

‎packages/types/src/mcp.ts‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,6 @@ import { z } from "zod"
66
*/
77
export const MAX_MCP_TOOLS_THRESHOLD = 60
88

9-
/** A provider-supplied `maxTools` replaces the generic advisory, so the warning tracks a real limit. */
10-
export function getMcpToolThreshold(modelInfo?: { maxTools?: number }): number {
11-
const limit = modelInfo?.maxTools
12-
return limit !== undefined && Number.isSafeInteger(limit) && limit > 0 ? limit : MAX_MCP_TOOLS_THRESHOLD
13-
}
14-
159
/**
1610
* McpServerUse
1711
*/

‎packages/types/src/model.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,6 @@ export const isModelParameter = (value: string): value is ModelParameter =>
102102
export const modelInfoSchema = z.object({
103103
maxTokens: z.number().nullish(),
104104
maxThinkingTokens: z.number().nullish(),
105-
maxTools: z.number().int().positive().optional(),
106105
contextWindow: z.number(),
107106
supportsImages: z.boolean().optional(),
108107
supportsPromptCache: z.boolean(),

‎src/api/providers/__tests__/github-copilot.spec.ts‎

Lines changed: 56 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,60 @@ describe("connectGitHubCopilot", () => {
217217
expect(vscode.authentication.getSession).toHaveBeenCalledTimes(1)
218218
})
219219

220+
describe("overlapping sign-in requests with different intent", () => {
221+
const session = (label: string) => ({ id: "s", accessToken: "t", scopes: [], account: { id: "a", label } })
222+
const pendingSession = () => {
223+
let finish: (value: ReturnType<typeof session>) => void = () => {}
224+
const promise = new Promise((resolve) => (finish = resolve)) as never
225+
return { promise, finish: (label: string) => finish(session(label)) }
226+
}
227+
228+
it("gives a Reconnect its own forced session instead of the pending plain sign-in's", async () => {
229+
const plain = pendingSession()
230+
const fresh = pendingSession()
231+
vi.mocked(vscode.authentication.getSession)
232+
.mockImplementationOnce(() => plain.promise)
233+
.mockImplementationOnce(() => fresh.promise)
234+
vi.mocked(vscode.lm.selectChatModels).mockResolvedValue([])
235+
const onFresh = vi.fn()
236+
237+
const first = connectGitHubCopilot()
238+
const second = connectGitHubCopilot(onFresh, true)
239+
plain.finish("Plain User")
240+
fresh.finish("Fresh User")
241+
242+
await expect(first).resolves.toMatchObject({ account: "Plain User" })
243+
await expect(second).resolves.toMatchObject({ account: "Fresh User" })
244+
expect(vscode.authentication.getSession).toHaveBeenCalledTimes(2)
245+
expect(vscode.authentication.getSession).toHaveBeenLastCalledWith("github", ["user:email"], {
246+
forceNewSession: true,
247+
clearSessionPreference: true,
248+
})
249+
expect(onFresh).toHaveBeenCalledWith("Fresh User")
250+
})
251+
252+
it("still shares one attempt between concurrent Reconnect requests", async () => {
253+
const fresh = pendingSession()
254+
vi.mocked(vscode.authentication.getSession).mockImplementationOnce(() => fresh.promise)
255+
vi.mocked(vscode.lm.selectChatModels).mockResolvedValue([])
256+
257+
const results = [connectGitHubCopilot(undefined, true), connectGitHubCopilot(undefined, true)]
258+
fresh.finish("Fresh User")
259+
260+
await Promise.all(results)
261+
expect(vscode.authentication.getSession).toHaveBeenCalledTimes(1)
262+
})
263+
264+
it("lets a failed attempt of one kind be retried without disturbing the other", async () => {
265+
vi.mocked(vscode.authentication.getSession).mockRejectedValueOnce(new Error("cancelled"))
266+
await expect(connectGitHubCopilot(undefined, true)).rejects.toThrow("cancelled")
267+
268+
vi.mocked(vscode.authentication.getSession).mockResolvedValueOnce(session("Retry User"))
269+
vi.mocked(vscode.lm.selectChatModels).mockResolvedValue([])
270+
await expect(connectGitHubCopilot(undefined, true)).resolves.toMatchObject({ account: "Retry User" })
271+
})
272+
})
273+
220274
it("allows a new sign-in once the previous one has settled", async () => {
221275
vi.mocked(vscode.authentication.getSession).mockRejectedValueOnce(new Error("Sign-in cancelled"))
222276
await expect(connectGitHubCopilot()).rejects.toThrow("Sign-in cancelled")
@@ -306,8 +360,8 @@ describe("Copilot model capabilities", () => {
306360
expect(getVsCodeLmModelInfo(model).supportsImages).toBe(false)
307361
})
308362

309-
it("falls back to the curated catalog only when the host reports nothing", () => {
310-
expect(getVsCodeLmModelInfo(liveModel({ family: curatedFamily })).supportsImages).toBe(true)
363+
it("does not borrow the catalog's vision flag for a live model the host reports nothing about", () => {
364+
expect(getVsCodeLmModelInfo(liveModel({ family: curatedFamily })).supportsImages).toBeUndefined()
311365
})
312366

313367
it("leaves vision unset rather than guessing when no source states it", () => {
@@ -337,11 +391,6 @@ describe("Copilot model capabilities", () => {
337391
handler.dispose()
338392
}
339393
})
340-
341-
it("does not set a tool-count limit, since consumers are only told whether tools are supported", () => {
342-
const info = getVsCodeLmModelInfo(liveModel({ capabilities: { supportsToolCalling: true } }))
343-
expect(info.maxTools).toBeUndefined()
344-
})
345394
})
346395

347396
describe("context window", () => {

‎src/api/providers/github-copilot.ts‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -93,17 +93,23 @@ function getGitHubSession(options: vscode.AuthenticationGetSessionOptions) {
9393
)
9494
}
9595

96-
let signInInFlight: Promise<{ account: string; models: Awaited<ReturnType<typeof getVsCodeLmModels>> }> | undefined
96+
type SignInResult = { account: string; models: Awaited<ReturnType<typeof getVsCodeLmModels>> }
97+
98+
// Keyed by whether a fresh session was requested, so a Reconnect is never answered by a plain sign-in.
99+
const signInsInFlight = new Map<boolean, Promise<SignInResult>>()
97100

98101
/**
99-
* Signs in to GitHub and lists the Copilot models that become available. Concurrent calls share one
100-
* attempt, so a double click cannot open two sign-in prompts.
102+
* Signs in to GitHub and lists the Copilot models that become available. Concurrent calls with the
103+
* same `forceNewSession` share one attempt, so a double click cannot open two sign-in prompts.
101104
*
102105
* Authenticating does not prove a Copilot entitlement: an account without one signs in successfully
103106
* and simply sees no models, which callers must present as such.
104107
*/
105108
export function connectGitHubCopilot(onAuthenticated?: (account: string) => Promise<void>, forceNewSession = false) {
106-
signInInFlight ??= (async () => {
109+
const pending = signInsInFlight.get(forceNewSession)
110+
if (pending) return pending
111+
112+
const attempt = (async (): Promise<SignInResult> => {
107113
try {
108114
const { authProviderId, authScopeSets, chatExtensionId, selector } = githubCopilotLanguageModel
109115
const session = await vscode.authentication.getSession(
@@ -116,10 +122,11 @@ export function connectGitHubCopilot(onAuthenticated?: (account: string) => Prom
116122
await vscode.extensions.getExtension(chatExtensionId)?.activate()
117123
return { account: session.account.label, models: await getVsCodeLmModels(selector) }
118124
} finally {
119-
signInInFlight = undefined
125+
signInsInFlight.delete(forceNewSession)
120126
}
121127
})()
122-
return signInInFlight
128+
signInsInFlight.set(forceNewSession, attempt)
129+
return attempt
123130
}
124131

125132
/** The signed-in account, or undefined when there is none or the lookup is not possible. */

‎src/api/providers/vscode-lm-capabilities.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,8 @@ export function reportedImageSupport(model: vscode.LanguageModelChat): boolean |
3232
}
3333

3434
/**
35-
* Derives Zoo's model info from what VS Code reports for a live model. Reported values win; the
36-
* curated catalog only fills gaps, and anything neither source states stays unset rather than guessed.
35+
* Derives Zoo's model info from what VS Code reports for a live model. The curated catalog only fills
36+
* gaps in everything but vision, and anything neither source states stays unset rather than guessed.
3737
*/
3838
export function getVsCodeLmModelInfo(model: vscode.LanguageModelChat): ModelInfo {
3939
const curated = vscodeLlmModels[model.family as keyof typeof vscodeLlmModels]
@@ -48,8 +48,8 @@ export function getVsCodeLmModelInfo(model: vscode.LanguageModelChat): ModelInfo
4848
...curated,
4949
contextWindow:
5050
curatedInputLimit && curatedInputLimit > 0 ? Math.min(liveInputLimit, curatedInputLimit) : liveInputLimit,
51-
// Without image-part support in the host, no image can be sent whatever the model advertises.
52-
supportsImages: canCreateImageParts() ? (reportedImageSupport(model) ?? curated?.supportsImages) : false,
51+
// A live model's vision is the host's word alone; unreported stays unset rather than borrowing the catalog's.
52+
supportsImages: canCreateImageParts() ? reportedImageSupport(model) : false,
5353
supportsPromptCache: false,
5454
inputPrice: 0,
5555
outputPrice: 0,

‎src/core/task/Task.ts‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ import {
5454
MAX_CHECKPOINT_TIMEOUT_SECONDS,
5555
MIN_CHECKPOINT_TIMEOUT_SECONDS,
5656
ConsecutiveMistakeError,
57-
getMcpToolThreshold,
57+
MAX_MCP_TOOLS_THRESHOLD,
5858
countEnabledMcpTools,
5959
providerIdentifiers,
6060
} from "@roo-code/types"
@@ -2823,14 +2823,13 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
28232823

28242824
// Check for too many MCP tools and warn the user
28252825
const { enabledToolCount, enabledServerCount } = await this.getEnabledMcpToolsCount()
2826-
const threshold = getMcpToolThreshold(this.api.getModel().info)
2827-
if (enabledToolCount > threshold) {
2826+
if (enabledToolCount > MAX_MCP_TOOLS_THRESHOLD) {
28282827
await this.say(
28292828
"too_many_tools_warning",
28302829
JSON.stringify({
28312830
toolCount: enabledToolCount,
28322831
serverCount: enabledServerCount,
2833-
threshold,
2832+
threshold: MAX_MCP_TOOLS_THRESHOLD,
28342833
}),
28352834
undefined,
28362835
undefined,

‎src/core/task/__tests__/Task.spec.ts‎

Lines changed: 0 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import type { Mock } from "vitest"
1010

1111
import {
1212
providerIdentifiers,
13-
MAX_MCP_TOOLS_THRESHOLD,
1413
RooCodeEventName,
1514
type GlobalState,
1615
type HistoryItem,
@@ -5564,45 +5563,6 @@ describe("Cline", () => {
55645563
})
55655564
})
55665565

5567-
describe("startTask MCP tool warning", () => {
5568-
const startWith = async (enabledToolCount: number, maxTools?: number) => {
5569-
const task = new Task({
5570-
provider: mockProvider,
5571-
apiConfiguration: mockApiConfig,
5572-
task: "new task",
5573-
startTask: false,
5574-
})
5575-
const taskAccess = getTaskTestAccess(task)
5576-
const saySpy = vi.spyOn(task, "say").mockResolvedValue(undefined)
5577-
vi.spyOn(taskAccess, "getEnabledMcpToolsCount").mockResolvedValue({
5578-
enabledToolCount,
5579-
enabledServerCount: 1,
5580-
})
5581-
vi.spyOn(taskAccess, "initiateTaskLoop").mockResolvedValue(undefined)
5582-
vi.spyOn(task.api, "getModel").mockReturnValue({
5583-
id: "test-model",
5584-
info: { contextWindow: 1000, supportsPromptCache: false, ...(maxTools ? { maxTools } : {}) },
5585-
})
5586-
await taskAccess.startTask("new task")
5587-
return saySpy.mock.calls.find(([type]) => type === "too_many_tools_warning")
5588-
}
5589-
5590-
it("keeps the advisory warning for providers that report no tool limit", async () => {
5591-
const warning = await startWith(MAX_MCP_TOOLS_THRESHOLD + 1)
5592-
expect(JSON.parse(warning![1] as string)).toMatchObject({ threshold: MAX_MCP_TOOLS_THRESHOLD })
5593-
})
5594-
5595-
it("does not warn at or below the advisory threshold", async () => {
5596-
expect(await startWith(MAX_MCP_TOOLS_THRESHOLD)).toBeUndefined()
5597-
})
5598-
5599-
it("lets a provider-reported limit replace the advisory in both directions", async () => {
5600-
expect(await startWith(130, 200)).toBeUndefined()
5601-
const warning = await startWith(21, 20)
5602-
expect(JSON.parse(warning![1] as string)).toMatchObject({ threshold: 20 })
5603-
})
5604-
})
5605-
56065566
describe("startTask", () => {
56075567
it("posts a clean state immediately before adding the first task message", async () => {
56085568
const task = new Task({

‎webview-ui/playwright/gallery/stories.tsx‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -218,6 +218,34 @@ export const stories: Record<string, Story> = {
218218
</div>
219219
)
220220
},
221+
"github-copilot-settings": async () => {
222+
const [{ AppProviders }, { VSCodeLM }, { providerIdentifiers }] = await Promise.all([
223+
import("../AppProviders"),
224+
import("@/components/settings/providers/VSCodeLM"),
225+
import("@roo-code/types"),
226+
])
227+
function CopilotSettings() {
228+
const [config, setConfig] = useState<import("@roo-code/types").ProviderSettings>({
229+
apiProvider: providerIdentifiers.githubCopilot,
230+
vsCodeLmModelSelector: { vendor: "copilot", id: "gpt-5.5", family: "gpt-5.5", version: "1" },
231+
})
232+
return (
233+
<div data-testid="github-copilot-settings" className="w-[420px] p-4 bg-vscode-editor-background">
234+
<VSCodeLM
235+
apiConfiguration={config}
236+
setApiConfigurationField={(field, value) =>
237+
setConfig((previous) => ({ ...previous, [field]: value }))
238+
}
239+
/>
240+
</div>
241+
)
242+
}
243+
return (
244+
<AppProviders>
245+
<CopilotSettings />
246+
</AppProviders>
247+
)
248+
},
221249
"ui-settings": async () => {
222250
const { UISettingsStory } = await import("@/components/settings/__tests__/UISettings.visual.fixture")
223251
return <UISettingsStory />

‎webview-ui/src/components/chat/TooManyToolsWarning.tsx‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,11 @@ import { useTooManyTools } from "@src/hooks/useTooManyTools"
44
import WarningRow from "./WarningRow"
55

66
/**
7-
* Displays a warning when enabled MCP tools exceed the model's advertised limit.
7+
* Displays a warning when the user has too many MCP tools enabled.
8+
* LLMs get confused when offered too many tools, which can lead to errors.
89
*
910
* The warning is shown when:
10-
* - The model advertises a limit and the enabled tool count exceeds it
11+
* - The total number of enabled tools across all enabled MCP servers exceeds the threshold
1112
*
1213
* @example
1314
* <TooManyToolsWarning />

0 commit comments

Comments
 (0)