feat: add GitHub Copilot as an AI provider#253
Merged
Conversation
Covers SDK integration (@github/copilot-sdk), event mapping, session lifecycle, 17-model registry, settings schema changes, and UX fixes for model selector overflow with large model lists.
10-task plan covering SDK integration, DI registration, settings schema, 17-model registry, provider icon, and UX fixes for model selector overflow.
- Fix Copilot brand color collision with Gemini (sky-400 -> violet-400) - Group Copilot's 17 models by vendor (OpenAI / Anthropic / Google / xAI) in both the composer submenu and settings Select dropdowns - Add Check icon to selected model in submenu and provider-locked list - Fix misleading reasoning hint when Copilot provider is selected - Add group field to ModelDefinition interface
The SDK resolves @github/copilot as an npm package (not an external
binary), so checking `gh --version` was a false positive when GitHub
CLI happened to be installed.
- Use require.resolve('@github/copilot') when no custom cliPath is set,
matching what the SDK itself does
- Translate "CLI server exited with code 0" into an actionable message
about running `gh auth login` and verifying the Copilot subscription
- Translate "Could not find @github/copilot" into the install message
When the @github/copilot process exits unexpectedly (auth failure, etc.), the client enters an error/disconnected state. The old check only looked at whether this.client was non-null, so dead clients were reused and every subsequent message would fail. - refreshClient() now checks getState() and rebuilds if not "connected" - sendMessage() nulls out this.client on "CLI server exited" so the next attempt gets a fresh process instead of failing on the dead one
findProviderForModel() always returned the first provider matching a model ID. Since Codex comes before Copilot in MODEL_PROVIDERS, selecting Copilot + GPT-5.3 Codex silently routed to the Codex provider instead. - Add getDefaultProviderId() helper (reads settings.model.defaults.provider) - ModelSelector.onSelect now passes (modelId, providerId) so callers know which provider the model was selected from, not guessing from ID alone - Add selectedProviderId prop to ModelSelector for correct icon/label display and accurate checkmark placement when providers share model IDs - Composer tracks explicit provider state; initialises from settings provider, restores from thread record on switch, and saves/restores with drafts - Replace all findProviderForModel(modelId) send-path calls with provider state - ComposerDraft gains optional provider field for persistence
CliErrorBanner only matched 'CLI not found' and 'not found at', so Copilot-specific errors were silently swallowed with no visible feedback. - Add 'package not found' marker (covers @github/copilot npm package missing) - Add 'exited unexpectedly' marker (covers auth / subscription failures) - Extend extractInstallCommand to also extract backtick-quoted commands so the auth error surfaces 'gh auth login' as a copyable code block
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds GitHub Copilot support end-to-end: new CopilotProvider with SDK/CLI session management on the server, DI/registry wiring, a provider.listModels RPC and transport, dynamic model discovery and UI changes (icons, selectors, settings, drafts), contracts/types for ProviderModelInfo, docs, tests, and package deps. Changes
Sequence Diagram(s)sequenceDiagram
participant Web as Web Composer
participant WS as WS Transport/Router
participant Server as Agent Service
participant Provider as CopilotProvider
participant SDK as Copilot SDK
participant CLI as Copilot CLI
Web->>WS: RPC sendMessage({ providerId: "copilot", sessionId, message, ... })
WS->>Server: dispatch sendMessage
Server->>Provider: sendMessage(params)
Note over Provider: ensure SDK client exists / refresh if CLI path changed
Provider->>SDK: create/get SDK client & session (or resume)
Provider->>SDK: register callbacks (textDelta, message, tool events, usage, idle)
SDK->>CLI: communicate via JSON-RPC over stdio
loop streamed events
CLI-->>SDK: stream events (deltas, tool events, usage, idle)
SDK-->>Provider: invoke callbacks
Provider->>Server: emit AgentEvent (textDelta / tool / usage)
Server-->>WS: forward events
WS-->>Web: deliver events to client
end
SDK-->>Provider: session.idle
Provider->>Provider: deregister callbacks, persist sdkSessionId
Provider->>Server: emit AgentEvent(ended)
Server-->>Web: end stream
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/components/chat/Composer.tsx (1)
938-938:⚠️ Potential issue | 🟠 MajorAdd
providerto the dependency array at line 938.The
handleSendcallback usesproviderat lines 819, 885, 910, and 921, but it's missing from the dependency array. Without it, the callback may use a staleprovidervalue if the provider changes.Dependency array missing `provider`
}, [input, attachments, isAgentRunning, isNewThread, newThreadMode, newThreadBranch, workspaceId, threadId, sendMessage, modelId, reasoning, mode, access, namingMode, customBranchName, selectedWorktree, injectFileContent, collectAndClearAttachments, clearDraftFromStore, preparingWorktree, branchFromMessageId, branchExecMode, branchTargetBranch, branchNamingMode, branchCustomName, branchWorktreePath, activeThread, branchThread, autoPreviewBranch, onBranchModeExit]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/chat/Composer.tsx` at line 938, The handleSend callback uses the provider variable but the useCallback dependency array (the one ending with onBranchModeExit) does not include provider, which can cause stale closures; update the dependency array for the handleSend useCallback to include provider so the callback is recreated when provider changes (locate the handleSend function and its useCallback dependency array and add provider to that array).
🧹 Nitpick comments (3)
apps/web/src/components/chat/CliErrorBanner.tsx (1)
17-21: Greedy backtick match may extract unintended content.The regex
/([^]+)/will match the first backtick-quoted string in the error message, which could be a path or other non-command content. For example, if an error contains "File/usr/bin/copilotnot found", it would extract/usr/bin/copilot` instead of an install command.Consider making the extraction more specific, e.g., matching command-like patterns starting with known tools:
♻️ Suggested improvement
function extractInstallCommand(error: string): string | null { const npmMatch = error.match(/npm install[^\n]+/); if (npmMatch) return npmMatch[0]; - const backtickMatch = error.match(/`([^`]+)`/); - return backtickMatch ? backtickMatch[1] : null; + // Match backtick-quoted commands starting with common CLI tools + const backtickMatch = error.match(/`((?:gh|npm|npx|brew|pip)\s[^`]+)`/); + return backtickMatch ? backtickMatch[1] : null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/chat/CliErrorBanner.tsx` around lines 17 - 21, extractInstallCommand currently uses a generic backtick capture that may grab non-command paths; update it to only capture backtick-quoted strings that look like install/command patterns (e.g., start with npm|yarn|pnpm|npx|pnpx|brew|curl|pip or include typical install verbs like "install") or match command-like tokens (alphanumerics, dashes, slashes, spaces, and flags) so paths like `/usr/bin/copilot` are ignored; modify the backtickMatch regex in extractInstallCommand to require a command-prefix or command-like pattern inside backticks and keep the existing npm install detection as the primary match, falling back to the stricter backtick command match if needed.apps/web/src/components/settings/sections/ModelSection.tsx (1)
188-222: Consider extracting the duplicated grouping logic into a helper.The grouped Select rendering logic (Map-based grouping, conditional
SelectLabel, item mapping) is duplicated between the Default model and Fallback model selectors. This is functional but increases maintenance surface.♻️ Optional: Extract a reusable grouped select renderer
// Helper outside the component function renderGroupedOptions(options: Array<{ value: string; label: string; group?: string }>) { const groups = new Map<string, typeof options>(); for (const m of options) { const g = m.group ?? ""; if (!groups.has(g)) groups.set(g, []); groups.get(g)!.push(m); } return Array.from(groups.entries()).map(([g, items]) => ( <SelectGroup key={g}> {g && <SelectLabel>{g}</SelectLabel>} {items.map((m) => ( <SelectItem key={m.value} value={m.value}> {m.label} </SelectItem> ))} </SelectGroup> )); } // Then in JSX: {modelOptions.some((m) => m.group) ? renderGroupedOptions(modelOptions) : modelOptions.map((m) => ( <SelectItem key={m.value} value={m.value}>{m.label}</SelectItem> ))}Also applies to: 230-271
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/settings/sections/ModelSection.tsx` around lines 188 - 222, The Select rendering duplicates Map-based grouping logic for modelOptions in the Default and Fallback selectors; extract that logic into a helper (e.g., renderGroupedOptions) that accepts options: Array<{value:string;label:string;group?:string}> and returns the grouped JSX using SelectGroup/SelectLabel/SelectItem, then replace the inline IIFE in both places with a call to renderGroupedOptions(modelOptions) while keeping the non-grouped fallback mapping and preserving props like value=modelId and onValueChange/handleModelChange (and the SegControl branch) to avoid changing behavior.docs/plans/2026-04-11-gh-copilot-provider-design.md (1)
13-18: Minor: Add language specifiers to fenced code blocks.Static analysis flagged three code blocks without language specifiers. For documentation hygiene, consider adding
textor removing the fences for plain prose content.📝 Suggested fix
-``` +```text CopilotProvider (implements IAgentProvider) └── CopilotClient (from `@github/copilot-sdk`) └── spawns `@github/copilot` CLI via JSON-RPC over stdio └── uses gh CLI auth (pre-authenticated by user)Apply similar changes to the error message blocks at lines 188 and 197. </details> Also applies to: 188-191, 197-199 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/plans/2026-04-11-gh-copilot-provider-design.mdaround lines 13 - 18,
Update the three fenced code blocks that contain plain text to include a
language specifier (e.g., changetotext) or remove the fences entirely:
the block starting with "CopilotProvider (implements IAgentProvider) └──
CopilotClient (from@github/copilot-sdk) ..." and the two plain error message
blocks (the short error strings around the later messages) should be updated so
static analysis no longer flags them; ensure each fence uses "text" if you want
to keep the block formatting.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@apps/web/src/lib/model-registry.ts:
- Around line 103-108: The Copilot Claude model entries in the model registry
use inconsistent dot vs hyphen version formatting which breaks exact lookups
(see findModelById and matchDatedVariant). Update the id strings in the registry
entries for the Copilot models so they use hyphens: change "claude-opus-4.6" ->
"claude-opus-4-6", "claude-opus-4.5" -> "claude-opus-4-5", "claude-sonnet-4.6"
-> "claude-sonnet-4-6", and "claude-sonnet-4.5" -> "claude-sonnet-4-5"; leave
"claude-haiku-4-5" and "claude-opus-4-6-fast" unchanged. Ensure these updated
ids match the native Claude provider naming so findModelById and
matchDatedVariant will resolve consistently.In
@package.json:
- Around line 39-41: Remove the
@github/copilot-sdkentry from the root
package.json dependencies and add it to apps/server/package.json dependencies
with version "^0.2.2"; the SDK is only imported/used in
apps/server/src/providers/copilot/copilot-provider.ts (imports CopilotClient and
CopilotSession), so declaring it in the server package prevents installing it
into unrelated workspaces—after updating package.json files, run your workspace
install (pnpm/yarn/npm) to update the lockfile and node_modules.
Outside diff comments:
In@apps/web/src/components/chat/Composer.tsx:
- Line 938: The handleSend callback uses the provider variable but the
useCallback dependency array (the one ending with onBranchModeExit) does not
include provider, which can cause stale closures; update the dependency array
for the handleSend useCallback to include provider so the callback is recreated
when provider changes (locate the handleSend function and its useCallback
dependency array and add provider to that array).
Nitpick comments:
In@apps/web/src/components/chat/CliErrorBanner.tsx:
- Around line 17-21: extractInstallCommand currently uses a generic backtick
capture that may grab non-command paths; update it to only capture
backtick-quoted strings that look like install/command patterns (e.g., start
with npm|yarn|pnpm|npx|pnpx|brew|curl|pip or include typical install verbs like
"install") or match command-like tokens (alphanumerics, dashes, slashes, spaces,
and flags) so paths like/usr/bin/copilotare ignored; modify the
backtickMatch regex in extractInstallCommand to require a command-prefix or
command-like pattern inside backticks and keep the existing npm install
detection as the primary match, falling back to the stricter backtick command
match if needed.In
@apps/web/src/components/settings/sections/ModelSection.tsx:
- Around line 188-222: The Select rendering duplicates Map-based grouping logic
for modelOptions in the Default and Fallback selectors; extract that logic into
a helper (e.g., renderGroupedOptions) that accepts options:
Array<{value:string;label:string;group?:string}> and returns the grouped JSX
using SelectGroup/SelectLabel/SelectItem, then replace the inline IIFE in both
places with a call to renderGroupedOptions(modelOptions) while keeping the
non-grouped fallback mapping and preserving props like value=modelId and
onValueChange/handleModelChange (and the SegControl branch) to avoid changing
behavior.In
@docs/plans/2026-04-11-gh-copilot-provider-design.md:
- Around line 13-18: Update the three fenced code blocks that contain plain text
to include a language specifier (e.g., changetotext) or remove the
fences entirely: the block starting with "CopilotProvider (implements
IAgentProvider) └── CopilotClient (from@github/copilot-sdk) ..." and the two
plain error message blocks (the short error strings around the later messages)
should be updated so static analysis no longer flags them; ensure each fence
uses "text" if you want to keep the block formatting.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `6eacefe0-6d08-4bcd-bd12-946b5bf08464` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between ae34100ece7645790fb9b38d4acd6d279a34f432 and e10095236204f7bd700bbe2b4b828ed7854cb8fd. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `bun.lock` is excluded by `!**/*.lock` </details> <details> <summary>📒 Files selected for processing (14)</summary> * `apps/server/src/container.ts` * `apps/server/src/providers/copilot/copilot-provider.ts` * `apps/server/src/services/agent-service.ts` * `apps/web/src/components/chat/CliErrorBanner.tsx` * `apps/web/src/components/chat/Composer.tsx` * `apps/web/src/components/chat/ModelSelector.tsx` * `apps/web/src/components/chat/ProviderIcons.tsx` * `apps/web/src/components/settings/sections/ModelSection.tsx` * `apps/web/src/lib/model-registry.ts` * `apps/web/src/stores/composerDraftStore.ts` * `docs/plans/2026-04-11-gh-copilot-provider-design.md` * `docs/plans/2026-04-11-gh-copilot-provider-plan.md` * `package.json` * `packages/contracts/src/models/settings.ts` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+39
to
+41
| "dependencies": { | ||
| "@github/copilot-sdk": "^0.2.2" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the package exists on npm and get latest version info
curl -s https://registry.npmjs.org/@github/copilot-sdk | jq '{name: .name, latest: ."dist-tags".latest, versions: (.versions | keys | .[-5:])}'Repository: Mzeey-Emipre/mcode
Length of output: 244
🏁 Script executed:
#!/bin/bash
# Find all package.json files to understand the monorepo structure
echo "=== Package.json files in repo ==="
find . -name "package.json" -type f | head -20
# Check the root package.json to see the actual content
echo -e "\n=== Root package.json (lines 35-45) ==="
sed -n '35,45p' package.json
# Search for imports of `@github/copilot-sdk`
echo -e "\n=== Imports of `@github/copilot-sdk` ==="
rg "@github/copilot-sdk" --type ts --type tsx --type jsRepository: Mzeey-Emipre/mcode
Length of output: 510
🏁 Script executed:
#!/bin/bash
# Check if copilot-provider.ts exists
echo "=== Looking for copilot provider file ==="
find . -name "*copilot*" -type f
# Search for any references to copilot-sdk with broader patterns
echo -e "\n=== Searching for copilot-sdk references (all files) ==="
rg "copilot-sdk"
# Also search for any imports from `@github` scope
echo -e "\n=== Searching for `@github` imports ==="
rg "@github" --type js --type ts 2>/dev/null || rg "@github"Repository: Mzeey-Emipre/mcode
Length of output: 2790
Move @github/copilot-sdk to apps/server/package.json.
The dependency is only used in apps/server/src/providers/copilot/copilot-provider.ts where it's imported as CopilotClient and CopilotSession. Since this is a monorepo, the dependency should be declared in apps/server/package.json rather than at the workspace root to avoid unnecessary installation in apps/desktop, apps/web, and packages/*.
The package version ^0.2.2 is confirmed valid—it's the latest published version on npm.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 39 - 41, Remove the `@github/copilot-sdk` entry from
the root package.json dependencies and add it to apps/server/package.json
dependencies with version "^0.2.2"; the SDK is only imported/used in
apps/server/src/providers/copilot/copilot-provider.ts (imports CopilotClient and
CopilotSession), so declaring it in the server package prevents installing it
into unrelated workspaces—after updating package.json files, run your workspace
install (pnpm/yarn/npm) to update the lockfile and node_modules.
…very Defines ProviderModelInfoSchema and ModelPolicyStateSchema in a new providers/models.ts module, and re-exports them from the contracts index.
Adds the provider.listModels WS method that takes a providerId and returns an array of ProviderModelInfo, enabling dynamic model discovery over the WebSocket RPC protocol.
Makes dynamic model discovery opt-in per provider. Copilot will implement this; other providers (Claude, Codex, Gemini) use static model lists and can leave the method unimplemented.
Add listModels() to CopilotProvider using the SDK client, with a helper to infer vendor group from model ID prefixes. Wire the provider.listModels RPC method through the router by adding providerRegistry to RouterDeps and dispatching to the resolved provider's listModels().
…c registry - Add listProviderModels to McodeTransport interface and ws-transport RPC - Add ProviderModelInfo re-export through transport/types and index - Create providerModelStore for caching dynamically fetched provider models - Add dynamic flag and multiplier/policyState fields to model-registry types - Replace hardcoded Copilot models with empty dynamic entry - Add toModelDefinition converter for ProviderModelInfo -> ModelDefinition - Update mock transport with listProviderModels stub
…cy indicators ModelSelector fetches Copilot (and other dynamic provider) models on dropdown open and merges them into the resolved provider list. Model rows now show a multiplier badge (e.g. "3x") and a Lock icon for org-disabled models. ModelSection settings panel fetches dynamic models on mount so Copilot models appear in the default/fallback model selects with multiplier annotations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/web/src/stores/providerModelStore.ts (1)
51-56: Consider resetting loading state inclearModels.If
clearModelsis called while a fetch is in-flight (e.g., on disconnect),loading[providerId]remainstrue, which will block futurefetchModelscalls for that provider. Consider also resetting the loading state.♻️ Proposed fix
clearModels: (providerId: string) => { set((s) => ({ models: { ...s.models, [providerId]: [] }, errors: { ...s.errors, [providerId]: null }, + loading: { ...s.loading, [providerId]: false }, })); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/stores/providerModelStore.ts` around lines 51 - 56, The clearModels function currently resets models and errors but leaves loading for the provider unchanged; update clearModels to also reset loading for the provider (set loading[providerId] = false) so an in-flight or cancelled fetch doesn't block future fetchModels calls; modify the state update in clearModels (referencing clearModels and the models, errors, loading keys) to include loading: { ...s.loading, [providerId]: false } in the same set call.apps/server/src/providers/copilot/copilot-provider.ts (1)
405-413:toolProgressemits emptytoolNamewhen SDK doesn't provide it.The
tool.execution_progressSDK event apparently doesn't includetoolName, so the emittedtoolProgressevent hastoolName: "". While this satisfies the schema, it may cause confusion in the UI if the tool name is displayed. Consider looking up the tool name from a stored mapping keyed bytoolCallId, or documenting this limitation.♻️ Possible enhancement to track tool names
+ // Track tool names by toolCallId for progress events + const toolNames = new Map<string, string>(); // tool.execution_start — assistant is invoking a tool unsubscribers.push( session.on("tool.execution_start", (event) => { const { toolCallId, toolName, arguments: toolArgs } = event.data; toolStartTimes.set(toolCallId, Date.now()); + toolNames.set(toolCallId, toolName); this.emit("event", { // ... }); }), ); // tool.execution_progress — heartbeat while a tool runs unsubscribers.push( session.on("tool.execution_progress", (event) => { const { toolCallId } = event.data; const startedAt = toolStartTimes.get(toolCallId) ?? Date.now(); const elapsedSeconds = (Date.now() - startedAt) / 1000; this.emit("event", { type: "toolProgress", threadId, toolCallId, - toolName: "", + toolName: toolNames.get(toolCallId) ?? "", elapsedSeconds, } satisfies AgentEvent); }), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/providers/copilot/copilot-provider.ts` around lines 405 - 413, The emitted toolProgress event sets toolName to an empty string because the SDK's tool.execution_progress doesn't include it; update the handler that emits the "toolProgress" event to look up the tool name from a stored mapping keyed by toolCallId (e.g., toolNamesByCallId or a similar map maintained when the tool call was created) and set toolName to that value, falling back to an empty string only if the lookup fails; ensure this change is applied where the code calls this.emit("event", { type: "toolProgress", threadId, toolCallId, toolName: "", elapsedSeconds }) so the UI receives a meaningful toolName when available.apps/web/src/components/chat/ModelSelector.tsx (1)
62-69: useEffect dependency array may cause unnecessary iterations.The effect includes
dynamicModelsanddynamicLoadingin the dependency array, which means the loop overMODEL_PROVIDERSruns on every store update. WhilefetchModelshas internal guards against duplicate requests, this could be optimized to only run once per dropdown open.♻️ Suggested optimization
+ const hasFetchedRef = useRef(false); + // Fetch dynamic models when the dropdown opens useEffect(() => { - if (!open) return; + if (!open) { + hasFetchedRef.current = false; + return; + } + if (hasFetchedRef.current) return; + hasFetchedRef.current = true; for (const p of MODEL_PROVIDERS) { - if (p.dynamic && !dynamicModels[p.id]?.length && !dynamicLoading[p.id]) { + if (p.dynamic) { fetchModels(p.id); } } - }, [open, fetchModels, dynamicModels, dynamicLoading]); + }, [open, fetchModels]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/chat/ModelSelector.tsx` around lines 62 - 69, The effect in useEffect is rerunning on any store update because dynamicModels and dynamicLoading are in the dependency array; restrict it to run only when the dropdown opens by removing those store-derived dependencies and depending only on open (and fetchModels if not stable). Keep the same loop over MODEL_PROVIDERS and the guards (p.dynamic && !dynamicModels[p.id]?.length && !dynamicLoading[p.id]) inside the effect, but ensure fetchModels is memoized (or include it) so the effect executes only once per open event and still triggers fetchModels(p.id) when needed; reference useEffect, MODEL_PROVIDERS, dynamicModels, dynamicLoading, fetchModels, and open when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/providers/copilot/copilot-provider.ts`:
- Around line 124-135: The current sdkModels.map transformation in
copilot-provider.ts unsafely casts supportedReasoningEfforts,
defaultReasoningEffort and policy.state to ProviderModelInfo types; update this
mapping to explicitly validate and sanitize those fields against the allowed
enums (e.g., allowed reasoning efforts ["low","medium","high","xhigh"] and
policy states ["enabled","disabled","unconfigured"]) instead of using `as`
casts: filter or map unsupported reasoning values out (or replace with a safe
default/undefined), ensure defaultReasoningEffort is set only if it matches the
allowed set, and set policy to undefined or a default state when policy.state is
unrecognized; keep references to sdkModels.map, inferModelGroup, and
ProviderModelInfo to locate the change.
---
Nitpick comments:
In `@apps/server/src/providers/copilot/copilot-provider.ts`:
- Around line 405-413: The emitted toolProgress event sets toolName to an empty
string because the SDK's tool.execution_progress doesn't include it; update the
handler that emits the "toolProgress" event to look up the tool name from a
stored mapping keyed by toolCallId (e.g., toolNamesByCallId or a similar map
maintained when the tool call was created) and set toolName to that value,
falling back to an empty string only if the lookup fails; ensure this change is
applied where the code calls this.emit("event", { type: "toolProgress",
threadId, toolCallId, toolName: "", elapsedSeconds }) so the UI receives a
meaningful toolName when available.
In `@apps/web/src/components/chat/ModelSelector.tsx`:
- Around line 62-69: The effect in useEffect is rerunning on any store update
because dynamicModels and dynamicLoading are in the dependency array; restrict
it to run only when the dropdown opens by removing those store-derived
dependencies and depending only on open (and fetchModels if not stable). Keep
the same loop over MODEL_PROVIDERS and the guards (p.dynamic &&
!dynamicModels[p.id]?.length && !dynamicLoading[p.id]) inside the effect, but
ensure fetchModels is memoized (or include it) so the effect executes only once
per open event and still triggers fetchModels(p.id) when needed; reference
useEffect, MODEL_PROVIDERS, dynamicModels, dynamicLoading, fetchModels, and open
when making this change.
In `@apps/web/src/stores/providerModelStore.ts`:
- Around line 51-56: The clearModels function currently resets models and errors
but leaves loading for the provider unchanged; update clearModels to also reset
loading for the provider (set loading[providerId] = false) so an in-flight or
cancelled fetch doesn't block future fetchModels calls; modify the state update
in clearModels (referencing clearModels and the models, errors, loading keys) to
include loading: { ...s.loading, [providerId]: false } in the same set call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3cf8a5b3-464e-4d9f-b27c-52d2fcdca82a
📒 Files selected for processing (14)
apps/server/src/index.tsapps/server/src/providers/copilot/copilot-provider.tsapps/server/src/transport/ws-router.tsapps/web/src/__tests__/mocks/transport.tsapps/web/src/components/chat/ModelSelector.tsxapps/web/src/components/settings/sections/ModelSection.tsxapps/web/src/lib/model-registry.tsapps/web/src/stores/providerModelStore.tsapps/web/src/transport/types.tsapps/web/src/transport/ws-transport.tspackages/contracts/src/index.tspackages/contracts/src/providers/interfaces.tspackages/contracts/src/providers/models.tspackages/contracts/src/ws/methods.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/src/components/settings/sections/ModelSection.tsx
- apps/web/src/lib/model-registry.ts
Comment on lines
+124
to
+135
| return sdkModels.map((m) => ({ | ||
| id: m.id, | ||
| name: m.name, | ||
| group: inferModelGroup(m.id), | ||
| contextWindow: m.capabilities?.limits?.max_context_window_tokens, | ||
| supportsVision: m.capabilities?.supports?.vision, | ||
| supportsReasoning: m.capabilities?.supports?.reasoningEffort, | ||
| supportedReasoningEfforts: m.supportedReasoningEfforts as ProviderModelInfo["supportedReasoningEfforts"], | ||
| defaultReasoningEffort: m.defaultReasoningEffort as ProviderModelInfo["defaultReasoningEffort"], | ||
| policy: m.policy ? { state: m.policy.state as "enabled" | "disabled" | "unconfigured" } : undefined, | ||
| multiplier: m.billing?.multiplier, | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@github/copilot-sdk npm package API documentation
💡 Result:
The @github/copilot-sdk npm package is the Node.js/TypeScript SDK for the GitHub Copilot CLI, enabling programmatic control via JSON-RPC. It is in public preview (version 0.2.1 as of April 2026) and may have breaking changes. Official documentation and API reference are in the GitHub repository at https://github.com/github/copilot-sdk, specifically the nodejs/README.md: https://github.com/github/copilot-sdk/blob/main/nodejs/README.md. Key API details from the README: CopilotClient constructor: new CopilotClient(options?: CopilotClientOptions) Options include: - cliPath: string (path to CLI executable) - cliArgs: string[] - cliUrl: string (connect to existing CLI server) - autoStart: boolean (default true) Methods: - start: Promise - Start CLI server and connect. - stop: Promise - Stop the CLI server. - createSession(config?: SessionConfig): Promise - Create conversation session. Session example: const session = await client.createSession({ model: "gpt-5", onPermissionRequest: approveAll, }); Supports custom providers (BYOK) like OpenAI, Azure, Anthropic, Ollama with ProviderConfig (type, baseUrl, apiKey, etc.). Requirements: Node.js >=18, GitHub Copilot CLI (bundled). Installation: npm install @github/copilot-sdk npm page: https://www.npmjs.com/package/@github/copilot-sdk GitHub Docs getting started: https://docs.github.com/en/copilot/how-tos/copilot-sdk/sdk-getting-started
Citations:
- 1: https://github.com/github/copilot-sdk/blob/main/docs/getting-started.md
- 2: https://www.npmjs.com/package/@github/copilot-sdk
- 3: https://docs.github.com/copilot/how-tos/copilot-sdk/sdk-getting-started
- 4: https://registry.npmjs.org/%40github%2Fcopilot-sdk
- 5: https://github.com/github/copilot-sdk
- 6: https://github.com/github/copilot-sdk/blob/main/nodejs/README.md
- 7: https://github.com/github/copilot-sdk/tree/main/nodejs
🏁 Script executed:
# First, find the structure of the codebase
git ls-files | grep -E "(copilot-provider|ws-router|ProviderModelInfo)" | head -20Repository: Mzeey-Emipre/mcode
Length of output: 256
🏁 Script executed:
# Get context around the lines mentioned in the review
cat -n apps/server/src/providers/copilot/copilot-provider.ts | sed -n '100,150p'Repository: Mzeey-Emipre/mcode
Length of output: 2541
🏁 Script executed:
# Search for ProviderModelInfo type definition
rg "type ProviderModelInfo|interface ProviderModelInfo" -A 20Repository: Mzeey-Emipre/mcode
Length of output: 189
🏁 Script executed:
# Search for ws-router.ts and its validation logic
find . -name "ws-router.ts" -o -name "ws-router.js"Repository: Mzeey-Emipre/mcode
Length of output: 102
🏁 Script executed:
# Find and inspect ProviderModelInfoSchema
cat -n packages/contracts/src/providers/models.tsRepository: Mzeey-Emipre/mcode
Length of output: 1952
🏁 Script executed:
# Look at ws-router.ts to see how validation is done
cat -n apps/server/src/transport/ws-router.ts | head -100Repository: Mzeey-Emipre/mcode
Length of output: 4395
🏁 Script executed:
# Search for validation of supportedReasoningEfforts and related fields
rg "supportedReasoningEfforts|defaultReasoningEffort" -B 3 -A 3Repository: Mzeey-Emipre/mcode
Length of output: 2085
🏁 Script executed:
# Search for where listModels is called and if responses are validated
rg "listModels|providerRegistry" -B 2 -A 5 apps/server/src/transport/ws-router.tsRepository: Mzeey-Emipre/mcode
Length of output: 713
🏁 Script executed:
# Look for WS_METHODS definition and response validation
rg "WS_METHODS|resultSchema|validate.*result" --type ts | head -30Repository: Mzeey-Emipre/mcode
Length of output: 1068
🏁 Script executed:
# Check how the provider registry methods are called and if results are validated
cat -n apps/server/src/transport/ws-router.ts | sed -n '100,200p'Repository: Mzeey-Emipre/mcode
Length of output: 3560
🏁 Script executed:
# Look at WS_METHODS definition to see the result schema for provider.listModels
rg "provider.listModels" packages/contracts/src/ws/methods.ts -B 5 -A 10Repository: Mzeey-Emipre/mcode
Length of output: 456
🏁 Script executed:
# Check the WS_METHODS file to understand response validation better
cat -n packages/contracts/src/ws/methods.ts | grep -A 30 "provider"Repository: Mzeey-Emipre/mcode
Length of output: 3362
Type casts assume SDK contract stability, but validation drift is tolerated.
The as casts on supportedReasoningEfforts, defaultReasoningEffort, and policy.state assume the SDK returns values matching the expected enums (["low", "medium", "high", "xhigh"] for reasoning effort; ["enabled", "disabled", "unconfigured"] for policy state). If the SDK introduces new values, the response validation in ws-router.ts will log a warning but still return the unvalidated result to the client (line 131 of ws-router.ts: "Still return the result - schema drift should not block responses"). Consider documenting this tolerance for schema drift or add explicit validation/filtering at the point of transformation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/providers/copilot/copilot-provider.ts` around lines 124 -
135, The current sdkModels.map transformation in copilot-provider.ts unsafely
casts supportedReasoningEfforts, defaultReasoningEffort and policy.state to
ProviderModelInfo types; update this mapping to explicitly validate and sanitize
those fields against the allowed enums (e.g., allowed reasoning efforts
["low","medium","high","xhigh"] and policy states
["enabled","disabled","unconfigured"]) instead of using `as` casts: filter or
map unsupported reasoning values out (or replace with a safe default/undefined),
ensure defaultReasoningEffort is set only if it matches the allowed set, and set
policy to undefined or a default state when policy.state is unrecognized; keep
references to sdkModels.map, inferModelGroup, and ProviderModelInfo to locate
the change.
…ity check
- Add @github/copilot as explicit root dependency so bun hoists it from
.bun/ to node_modules/@github/copilot (was installed as transitive dep
of copilot-sdk but never symlinked, causing all require.resolve calls to fail)
- Replace _require.resolve('@github/copilot') with existsSync probe matching
the SDK's own getBundledCliPath() strategy — the package has no main export
so require.resolve always throws even when the package is present
- Show AlertCircle + RefreshCw icons in ModelSelector when dynamic model
fetch fails; clicking the provider row clears the error and retries
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/server/src/providers/copilot/copilot-provider.ts (1)
36-43: Consider makinginferModelGroupmore maintainable.The prefix matching works but may drift as vendors introduce new model naming schemes. This is acceptable for now, but consider extracting the vendor prefixes to a constant map for easier updates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/providers/copilot/copilot-provider.ts` around lines 36 - 43, Extract the hard-coded prefix checks in inferModelGroup into a single constant map (e.g., MODEL_PREFIX_MAP or MODEL_VENDOR_PREFIXES) that maps vendor names ("OpenAI", "Anthropic", "Google", "xAI") to an array of prefixes, then rewrite inferModelGroup to iterate the map and return the vendor when modelId.startsWith(any prefix); keep function name inferModelGroup and preserve current behavior (including undefined fallback), make the prefixes constant/exported for easy updates, and add/update any tests that rely on these mappings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/server/src/providers/copilot/copilot-provider.ts`:
- Around line 36-43: Extract the hard-coded prefix checks in inferModelGroup
into a single constant map (e.g., MODEL_PREFIX_MAP or MODEL_VENDOR_PREFIXES)
that maps vendor names ("OpenAI", "Anthropic", "Google", "xAI") to an array of
prefixes, then rewrite inferModelGroup to iterate the map and return the vendor
when modelId.startsWith(any prefix); keep function name inferModelGroup and
preserve current behavior (including undefined fallback), make the prefixes
constant/exported for easy updates, and add/update any tests that rely on these
mappings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ad52d9a-1763-4fcf-818c-4ad1a4902cbc
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
apps/server/src/providers/copilot/copilot-provider.tsapps/web/src/components/chat/ModelSelector.tsxapps/web/src/stores/providerModelStore.tspackage.json
✅ Files skipped from review due to trivial changes (2)
- package.json
- apps/web/src/stores/providerModelStore.ts
The SDK's createSession() auto-starts the connection via autoStart, but listModels() does not -- it throws 'Client not connected' if start() hasn't been called. Eagerly start the client in refreshClient() so both sendMessage and listModels work.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/server/src/providers/copilot/copilot-provider.ts (1)
137-139:⚠️ Potential issue | 🟡 MinorSanitize SDK enum fields instead of force-casting them.
Line [137]–Line [139] trusts SDK values via
ascasts; unknown enum values can drift through to clients and break contract expectations.What are the currently documented allowed values for `supportedReasoningEfforts`, `defaultReasoningEffort`, and `policy.state` in `@github/copilot-sdk` model metadata returned by `listModels()`?✅ Safer mapping pattern
+ const allowedReasoning = new Set(["low", "medium", "high", "xhigh"] as const); + const allowedPolicy = new Set(["enabled", "disabled", "unconfigured"] as const); ... - supportedReasoningEfforts: m.supportedReasoningEfforts as ProviderModelInfo["supportedReasoningEfforts"], - defaultReasoningEffort: m.defaultReasoningEffort as ProviderModelInfo["defaultReasoningEffort"], - policy: m.policy ? { state: m.policy.state as "enabled" | "disabled" | "unconfigured" } : undefined, + supportedReasoningEfforts: m.supportedReasoningEfforts?.filter((v) => + allowedReasoning.has(v as (typeof m.supportedReasoningEfforts)[number]), + ) as ProviderModelInfo["supportedReasoningEfforts"] | undefined, + defaultReasoningEffort: allowedReasoning.has(m.defaultReasoningEffort as never) + ? (m.defaultReasoningEffort as ProviderModelInfo["defaultReasoningEffort"]) + : undefined, + policy: m.policy && allowedPolicy.has(m.policy.state as never) + ? { state: m.policy.state as "enabled" | "disabled" | "unconfigured" } + : undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/providers/copilot/copilot-provider.ts` around lines 137 - 139, The current code force-casts SDK enum fields (supportedReasoningEfforts, defaultReasoningEffort, and policy.state) from the Copilot SDK model metadata (variable m) into ProviderModelInfo types, which can pass unknown values to clients; update the mapping in copilot-provider.ts to validate and sanitize these fields instead of using `as` casts: for supportedReasoningEfforts filter m.supportedReasoningEfforts against the explicit allowed list (whitelist) and map only known values, for defaultReasoningEffort check that m.defaultReasoningEffort is one of the allowed enum values before assigning (or set undefined/default if not), and for policy use a guarded check on m.policy?.state to only accept "enabled" | "disabled" | "unconfigured" (otherwise set policy undefined or a safe default); reference the fields supportedReasoningEfforts, defaultReasoningEffort, and policy.state and ProviderModelInfo when implementing these safe mappings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/providers/copilot/copilot-provider.ts`:
- Around line 373-381: The toolProgress events currently emit an empty toolName;
add a per-call-name map and populate it when handling
session.on("tool.execution_start") (use the same toolCallId key you store in
toolStartTimes) so you can later look up the real tool name when emitting
"toolProgress" events; update the two places that set toolName: "" (around the
current tool progress emission logic) to use toolNameMap.get(toolCallId) || ""
(or similar) and ensure you clear the entry on tool completion/stop to avoid
leaks.
- Around line 249-255: The current logic starts a new runTurn(sessionId, ...)
immediately when an existing session is present, causing overlapping runTurn
executions and duplicate listeners; instead serialize turns per session by
adding a per-session queue or "inFlight" flag on the session entry returned by
this.sessions.get(sessionId) and enqueue the new message or await the prior
runTurn to finish before calling runTurn. Concretely: extend the session
container (the object from this.sessions.get(sessionId)) with an inFlight
Promise/boolean or a queue array, update the existing branch that currently does
existing.lastUsedAt = Date.now() to push the message into that queue or await
existing.inFlight, and modify runTurn to resolve the next queued item and clear
the inFlight marker when finished so no two runTurn(sessionId, ...) execute
concurrently and duplicate listeners on existing.session are not registered.
- Around line 106-109: The code unconditionally passes shell: true into
execFileAsync when probing cliPath (using execFileAsync and cliPath), which
risks shell injection; change the probe to set shell: process.platform ===
"win32" so shell mode is only used on Windows, preserve timeout: 5000 and
existing behavior (return null on success), and apply the same platform-gated
change in the corresponding probe that uses execFileAsync for the Codex CLI
(e.g., provider.cli.codex).
---
Duplicate comments:
In `@apps/server/src/providers/copilot/copilot-provider.ts`:
- Around line 137-139: The current code force-casts SDK enum fields
(supportedReasoningEfforts, defaultReasoningEffort, and policy.state) from the
Copilot SDK model metadata (variable m) into ProviderModelInfo types, which can
pass unknown values to clients; update the mapping in copilot-provider.ts to
validate and sanitize these fields instead of using `as` casts: for
supportedReasoningEfforts filter m.supportedReasoningEfforts against the
explicit allowed list (whitelist) and map only known values, for
defaultReasoningEffort check that m.defaultReasoningEffort is one of the allowed
enum values before assigning (or set undefined/default if not), and for policy
use a guarded check on m.policy?.state to only accept "enabled" | "disabled" |
"unconfigured" (otherwise set policy undefined or a safe default); reference the
fields supportedReasoningEfforts, defaultReasoningEffort, and policy.state and
ProviderModelInfo when implementing these safe mappings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f61ecf90-2ae3-4474-b3ad-983261027daf
📒 Files selected for processing (1)
apps/server/src/providers/copilot/copilot-provider.ts
Comment on lines
+249
to
+255
| const existing = this.sessions.get(sessionId); | ||
| if (existing) { | ||
| existing.lastUsedAt = Date.now(); | ||
| // Abort in-flight turn by sending a new message on the existing session. | ||
| // The previous runTurn promise will resolve when session.idle fires. | ||
| void this.runTurn(sessionId, threadId, existing.session, message); | ||
| return; |
There was a problem hiding this comment.
Prevent overlapping runTurn() executions per sessionId.
Line [249] starts a new runTurn() even if a previous turn is still active on the same session. That can register duplicate listeners and double-emit textDelta/message/ended events.
💡 Suggested direction (serialize turns per session)
+ private inFlightTurns = new Map<string, Promise<void>>();
...
const existing = this.sessions.get(sessionId);
if (existing) {
existing.lastUsedAt = Date.now();
- void this.runTurn(sessionId, threadId, existing.session, message);
+ const prev = this.inFlightTurns.get(sessionId) ?? Promise.resolve();
+ const next = prev.finally(() =>
+ this.runTurn(sessionId, threadId, existing.session, message),
+ );
+ this.inFlightTurns.set(sessionId, next);
+ void next.finally(() => {
+ if (this.inFlightTurns.get(sessionId) === next) this.inFlightTurns.delete(sessionId);
+ });
return;
}
...
- void this.runTurn(sessionId, threadId, session, message);
+ const firstTurn = this.runTurn(sessionId, threadId, session, message);
+ this.inFlightTurns.set(sessionId, firstTurn);
+ void firstTurn.finally(() => {
+ if (this.inFlightTurns.get(sessionId) === firstTurn) this.inFlightTurns.delete(sessionId);
+ });Also applies to: 332-510
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/providers/copilot/copilot-provider.ts` around lines 249 -
255, The current logic starts a new runTurn(sessionId, ...) immediately when an
existing session is present, causing overlapping runTurn executions and
duplicate listeners; instead serialize turns per session by adding a per-session
queue or "inFlight" flag on the session entry returned by
this.sessions.get(sessionId) and enqueue the new message or await the prior
runTurn to finish before calling runTurn. Concretely: extend the session
container (the object from this.sessions.get(sessionId)) with an inFlight
Promise/boolean or a queue array, update the existing branch that currently does
existing.lastUsedAt = Date.now() to push the message into that queue or await
existing.inFlight, and modify runTurn to resolve the next queued item and clear
the inFlight marker when finished so no two runTurn(sessionId, ...) execute
concurrently and duplicate listeners on existing.session are not registered.
Comment on lines
+373
to
+381
| session.on("tool.execution_start", (event) => { | ||
| const { toolCallId, toolName, arguments: toolArgs } = event.data; | ||
| toolStartTimes.set(toolCallId, Date.now()); | ||
| this.emit("event", { | ||
| type: "toolUse", | ||
| threadId, | ||
| toolCallId, | ||
| toolName, | ||
| toolInput: toolArgs ?? {}, |
There was a problem hiding this comment.
Emit a real toolName in toolProgress events.
Line [419] currently sends toolName: "". Consumers correlating progress by tool name lose fidelity and may misrender progress rows.
🧭 Keep a per-tool name map for progress events
- const toolStartTimes = new Map<string, number>();
+ const toolStartTimes = new Map<string, number>();
+ const toolNames = new Map<string, string>();
...
session.on("tool.execution_start", (event) => {
const { toolCallId, toolName, arguments: toolArgs } = event.data;
toolStartTimes.set(toolCallId, Date.now());
+ toolNames.set(toolCallId, toolName);
...
session.on("tool.execution_complete", (event) => {
const { toolCallId, success, result } = event.data;
toolStartTimes.delete(toolCallId);
+ toolNames.delete(toolCallId);
...
session.on("tool.execution_progress", (event) => {
const { toolCallId } = event.data;
...
this.emit("event", {
type: "toolProgress",
threadId,
toolCallId,
- toolName: "",
+ toolName: toolNames.get(toolCallId) ?? "unknown",
elapsedSeconds,
} satisfies AgentEvent);
}),Also applies to: 417-420
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/providers/copilot/copilot-provider.ts` around lines 373 -
381, The toolProgress events currently emit an empty toolName; add a
per-call-name map and populate it when handling
session.on("tool.execution_start") (use the same toolCallId key you store in
toolStartTimes) so you can later look up the real tool name when emitting
"toolProgress" events; update the two places that set toolName: "" (around the
current tool progress emission logic) to use toolNameMap.get(toolCallId) || ""
(or similar) and ensure you clear the entry on tool completion/stop to avoid
leaks.
…try is empty Revert dynamic model loading (listModels RPC) and restore hardcoded 17-model Copilot registry. The dynamic path caused cascading issues: client not connected before start(), package not hoisted by bun, require.resolve failing on packages without a main export, and infinite retry loops on fetch errors. The server-side listModels implementation and transport plumbing are retained for future use; only the UI components stop calling them.
Contributor
Author
Copilot Provider: CLI Bootstrapping Needs a RewriteAfter reading the SDK's compiled source ( Root CauseThe SDK's internal ...which fails silently (code 0). Every fix we've applied has been a workaround for this one bug, and they keep undoing each other. What the SDK Already Handles (that we're reimplementing)
Proposed Rewrite:
|
Stop reimplementing SDK internals (CLI resolution, availability checks). Patch the root cause: process.execPath is electron.exe inside utilityProcess.fork, so temporarily override it with the real node binary during client construction. Pass githubToken explicitly from gh auth token for reliable headless auth. Let client.start() surface errors rather than probing with a bespoke checkCliAvailable() check.
- Remove shell: true from gh auth token call (injection risk, Windows
cmd.exe can corrupt tokens with metacharacters)
- Assign this.client only after start() succeeds so a failed startup
never leaves a stale non-started client
- Move this.lastCliPath assignment after start() succeeds
- Replace this.client! with null guard in doSendMessage
- Cache which("node") result to avoid re-probing PATH on every rebuild
- Replace process.execPath override with a local savedExecPath variable
instead of attaching _mcodeOrigExecPath to the global process object
- Use typed opts interface instead of Record<string, unknown>
- Update stale docstring on execFileAsync
- Log debug message when gh auth token is unavailable
- Import Mock type in test file
Show OpenAI/Anthropic/Google/xAI labels inline on each model row in the Copilot fly-out so users can identify the vendor at a glance. Also bump the submenu max-height from a fixed 280px to min(480px, 100vh-8rem) so the xAI group is no longer cut off on typical screen sizes.
…ed view The per-row vendor badge was redundant when a section header already labels the group. Remove it. Extract renderGroupedModels so both the fly-out submenu and the provider-locked flat list share the same grouped rendering - users can now see vendor sections in both contexts.
Both the new-thread submenu and the provider-locked (existing thread) view now call listProviderModels() to get the live model list from the server instead of relying solely on the static registry. The fetch is lazy on hover for new threads and immediate on mount for locked threads. Falls back to the static registry if the RPC fails.
Add supportsModelListing flag to ModelProvider and set it on Copilot. Gate fetchProviderModels calls behind this flag so Claude, Codex, and other providers that lack listModels() do not trigger a server error.
Submenu was anchored at top-0, extending downward and overflowing the viewport because the main picker opens upward (bottom-full). Changed to bottom-0 so the submenu grows upward, consistent with the main menu.
- Merge prDraftService, threadRepo, workspaceRepo into ws-router and index - Merge PrDraft/CreatePrResult imports alongside ProviderModelInfo in transport types - Merge isCompletionCapable export alongside ProviderModelInfo exports in contracts - Merge prDraftProvider/prDraftModel settings alongside copilotCliPath in ModelSection - Add supportsCompletion = false to CopilotProvider to satisfy updated IAgentProvider
Show multiplier badge on every model regardless of value (including 1x) so the billing rate is always visible. Replace the static-then-live flicker with a centered spinner while listProviderModels is in flight, switching to the live list once the fetch resolves.
This was referenced Apr 13, 2026
Add @ts-expect-error for the fs import since @types/node is not in the web package tsconfig. The test runs under vitest-environment node which provides fs at runtime.
The live listProviderModels() fetch is the source of truth. The static list now holds only GPT-4.1, Claude Sonnet 4.6, and Gemini 2.5 Pro as a minimal fallback while the spinner loads or if the fetch fails.
…on.error hang Instead of mutating the global process.execPath (fragile, affects the entire process), prepend the real node binary directory to PATH in the env option passed to CopilotClient. Also fix session.error handler to call resolve() so the turnPromise settles, the finally block runs cleanup, and the thread exits the 'running' state in the UI.
…rror The test uses Node's fs module with @vitest-environment node, but the web tsconfig doesn't include @types/node. In CI, @types/node resolves from the monorepo root causing the @ts-expect-error directive to be unused (itself a TS error). Exclude the file from tsconfig instead so both local and CI environments behave consistently.
- Evict dead session on session.error to prevent reuse of errored SDK session - Add double-checked locking comment after refreshClient() await to prevent concurrent sendMessage calls from creating duplicate SDK sessions - Extract toThreadId() helper to deduplicate prefix-stripping logic - Add TODO noting approveAll bypasses permissionMode (SDK limitation) - Fix inferModelGroup to match exact o1/o3/o4 IDs, not just any id starting with those chars - Fix fetchProviderModels: record failed fetches to prevent retry spam on hover - Fix fetchProviderModels: reuse Map instance to avoid double-copy on success - Fix shortLabel: guard against undefined displayProvider to avoid 'undefined ' prefix - Precompute sorted flat model list in model-registry to avoid per-call allocation - Expand ProviderId union to include cursor and opencode (comingSoon placeholders) - Replace Function type and any casts in test with proper typed signatures - Fix duplicate semicolon in test assertion
This was referenced Apr 14, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Integrates GitHub Copilot as a first-class AI provider via `@github/copilot-sdk`, giving users access to models from OpenAI, Anthropic, Google, and xAI families from within the app.
Why
GitHub Copilot subscribers already pay for access to a wide range of frontier models. Surfacing that as a selectable provider avoids redundant API key setup and broadens model coverage without adding new billing.
Key Changes
CopilotProvider- implementsIAgentProviderusing the Copilot SDK's long-running stdio process pattern; rewrites bootstrap to fix an Electron executor bug whereutilityProcess.forksetsprocess.execPathtoelectron.exe; temporarily overridesprocess.execPathwith the realnodebinary duringclient.start()and restores it in afinallyblock; fetchesgh auth tokenexplicitly so the headless subprocess does not need to discover auth from its environmentprovideras explicit state inComposer(not derived from model ID) so Copilot and Codex models that share IDs route to the correct backendModelSelectorcallslistProviderModels()on hover (new thread) and on mount (existing thread) to show the live model list from GitHub's API; falls back to the static registry if the fetch fails; gated behind asupportsModelListingflag so providers withoutlistModels()do not trigger server errorsmin(480px, 100vh-8rem)so all groups are reachablecopilot.cliPathsettingCliErrorBannerdetects Copilot-specific "package not found" and "exited unexpectedly" errors and renders a copyable install/auth commandprimer/octiconscopilot-24ComposerDraft- persistsprovideralongside model so draft restore round-trips the correct backendConfiguration Changes
No new environment variables or Vault secrets. The optional
copilot.cliPathsetting defaults to auto-discovery via the npm package.Closes #90