feat(copilot): add GitHub Copilot provider (via @github/copilot-sdk) - #7656
feat(copilot): add GitHub Copilot provider (via @github/copilot-sdk)#7656NSExceptional wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| turnId: input.turnId, | ||
| itemId: RuntimeItemId.make(input.data.toolCallId), | ||
| payload: { | ||
| itemType: "dynamic_tool_call", |
There was a problem hiding this comment.
🟡 Medium sdk/CopilotSdkRuntimeEvents.ts:205
makeSdkToolCompleteEvent emits itemType: "dynamic_tool_call" for every completed tool, so the lifecycle collapse overwrites the start event's command_execution, file_change, or mcp_tool_call classification and renders completed tools as generic tools. Preserve the classification for each toolCallId and reuse it when emitting progress and completion events.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/sdk/CopilotSdkRuntimeEvents.ts around line 205:
`makeSdkToolCompleteEvent` emits `itemType: "dynamic_tool_call"` for every completed tool, so the lifecycle collapse overwrites the start event's `command_execution`, `file_change`, or `mcp_tool_call` classification and renders completed tools as generic tools. Preserve the classification for each `toolCallId` and reuse it when emitting progress and completion events.
| const generateThreadTitle: TextGenerationShape["generateThreadTitle"] = Effect.fn( | ||
| "CopilotTextGeneration.generateThreadTitle", | ||
| )(function* (input) { | ||
| const { prompt, outputSchema } = buildThreadTitlePrompt({ |
There was a problem hiding this comment.
🟡 Medium textGeneration/CopilotTextGeneration.ts:202
The Copilot provider ignores configured generation context: generateThreadTitle drops previousTitle, while generateBranchName, generatePrContent, and generateCommitMessage drop their respective policy values; generatePrContent also drops changeRequestTemplate. As a result, title regeneration uses the initial-title prompt, and branch names, PR content, and commit messages are generated without the caller's naming, template, or instruction constraints. Pass these omitted fields through to the corresponding build*Prompt calls.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/textGeneration/CopilotTextGeneration.ts around line 202:
The Copilot provider ignores configured generation context: `generateThreadTitle` drops `previousTitle`, while `generateBranchName`, `generatePrContent`, and `generateCommitMessage` drop their respective `policy` values; `generatePrContent` also drops `changeRequestTemplate`. As a result, title regeneration uses the initial-title prompt, and branch names, PR content, and commit messages are generated without the caller's naming, template, or instruction constraints. Pass these omitted fields through to the corresponding `build*Prompt` calls.
| const { settings, snapshot, publishSnapshot } = input; | ||
| const stampIdentity = input.stampIdentity ?? ((value) => value); | ||
|
|
||
| const enrichVersionAdvisory = enrichProviderSnapshotWithVersionAdvisory( |
There was a problem hiding this comment.
🟡 Medium Layers/CopilotProvider.ts:349
enrichCopilotSnapshot ignores the user's provider update-check setting, so Copilot still requests the latest version from the npm registry and publishes version advisories when checks are disabled. Add enableProviderUpdateChecks to this function's input, pass it as the options argument to enrichProviderSnapshotWithVersionAdvisory, and forward the setting from the caller.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/CopilotProvider.ts around line 349:
`enrichCopilotSnapshot` ignores the user's provider update-check setting, so Copilot still requests the latest version from the npm registry and publishes version advisories when checks are disabled. Add `enableProviderUpdateChecks` to this function's input, pass it as the `options` argument to `enrichProviderSnapshotWithVersionAdvisory`, and forward the setting from the caller.
| const seen = new Set<string>(); | ||
| for (const dir of [...pathDirs, ...commonDirs]) { | ||
| if (!dir || seen.has(dir)) continue; | ||
| seen.add(dir); | ||
| const candidate = NodePath.join(dir, binary); | ||
| if (await isExecutable(candidate)) return candidate; | ||
| } |
There was a problem hiding this comment.
🟠 High sdk/CopilotSdkClient.ts:107
On Windows, resolveBinaryPath fails to resolve a normal npm-installed copilot.cmd, so it passes bare copilot to RuntimeConnection.forStdio and SDK-backed Copilot operations fail even though the CLI is installed. The probe only checks NodePath.join(dir, binary); probe the PATHEXT suffixes (including .CMD) before falling back to the bare name.
| const seen = new Set<string>(); | |
| for (const dir of [...pathDirs, ...commonDirs]) { | |
| if (!dir || seen.has(dir)) continue; | |
| seen.add(dir); | |
| const candidate = NodePath.join(dir, binary); | |
| if (await isExecutable(candidate)) return candidate; | |
| } | |
| const extensions = | |
| process.platform === "win32" | |
| ? (env?.PATHEXT || process.env.PATHEXT || ".COM;.EXE;.BAT;.CMD").split(";") | |
| : [""]; | |
| const seen = new Set<string>(); | |
| for (const dir of [...pathDirs, ...commonDirs]) { | |
| if (!dir || seen.has(dir)) continue; | |
| seen.add(dir); | |
| for (const extension of extensions) { | |
| const candidate = NodePath.join(dir, `${binary}${extension}`); | |
| if (await isExecutable(candidate)) return candidate; | |
| } | |
| } |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/sdk/CopilotSdkClient.ts around lines 107-113:
On Windows, `resolveBinaryPath` fails to resolve a normal npm-installed `copilot.cmd`, so it passes bare `copilot` to `RuntimeConnection.forStdio` and SDK-backed Copilot operations fail even though the CLI is installed. The probe only checks `NodePath.join(dir, binary)`; probe the `PATHEXT` suffixes (including `.CMD`) before falling back to the bare name.
There was a problem hiding this comment.
Effect Service Conventions — 6 findings
The new Copilot provider code mostly follows the repo's Effect conventions, but a few error-handling details in the new files diverge from them (and from the sibling Grok/Cursor/Aether implementations):
CopilotSdkErroris aData.TaggedErrorwhose only payload is a stringified cause.CopilotTextGenerationhand-rolls a_tagpredicate, adds a pass-through error factory, and usesEffect.catchTaginstead ofEffect.catchTags.CopilotDriver/CopilotAdapterfoldcause.messageinto the caller-visibledetail.
Details inline.
Posted via Macroscope — Effect Service Conventions
| new ProviderDriverError({ | ||
| driver: DRIVER_KIND, | ||
| instanceId, | ||
| detail: `Failed to build Copilot snapshot: ${cause.message ?? String(cause)}`, |
There was a problem hiding this comment.
ProviderDriverError.message is built from detail, so interpolating cause.message here derives the caller-visible message from the cause. Keep detail a stable structural phrase and let cause carry the dynamic part (same as AetherDriver / OpenCode2Driver).
| detail: `Failed to build Copilot snapshot: ${cause.message ?? String(cause)}`, | |
| detail: "Failed to build the Copilot provider snapshot.", |
Posted via Macroscope — Effect Service Conventions
56d5f9a to
c10b8ef
Compare
|
Thanks for the automated review — addressed the findings (force-pushed Correctness
Effect conventions
One I left as-is: the |
There was a problem hiding this comment.
One outstanding convention issue remains in the Copilot SDK error-wrapping path. Earlier findings on CopilotTextGeneration.ts (redundant error factory, hand-rolled _tag predicate, catchTag) are addressed. The still-open threads on CopilotDriver.ts:160 and CopilotSdkClient.ts:32-45 also remain applicable.
Posted via Macroscope — Effect Service Conventions
c10b8ef to
45ca7b9
Compare
|
Second pass on the re-review (
Typecheck + lint clean, 76 tests green. |
| makeProviderMaintenanceCapabilities({ | ||
| provider: DRIVER_KIND, | ||
| packageName: null, | ||
| updateExecutable: "copilot", |
There was a problem hiding this comment.
🟡 Medium Drivers/CopilotDriver.ts:53
The update action ignores an instance's configured binaryPath, so custom or absolute Copilot installations are updated using the PATH-resolved copilot binary—or fail when copilot is not on PATH. makeStaticProviderMaintenanceResolver always returns the hard-coded executable from UPDATE; use a resolver that derives the update executable from the supplied binaryPath.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Drivers/CopilotDriver.ts around line 53:
The update action ignores an instance's configured `binaryPath`, so custom or absolute Copilot installations are updated using the PATH-resolved `copilot` binary—or fail when `copilot` is not on `PATH`. `makeStaticProviderMaintenanceResolver` always returns the hard-coded executable from `UPDATE`; use a resolver that derives the update executable from the supplied `binaryPath`.
|
Round 3 (
Not changed — flagging as consistent-with-siblings rather than a Copilot-specific bug:
Typecheck + lint clean, 77 tests green. |
45ca7b9 to
1e7bc57
Compare
|
Round 4 ( |
1e7bc57 to
a91e1c5
Compare
There was a problem hiding this comment.
One retained convention issue in the new Copilot SDK layer: CopilotSdkError is still modeled with Data.TaggedError and its detail is a stringified cause. See the inline note. (The earlier findings in CopilotTextGeneration.ts and CopilotAdapter.ts look addressed.)
Posted via Macroscope — Effect Service Conventions
|
Round 5 ( I believe that leaves only the |
a91e1c5 to
6d40d6e
Compare
|
Round 6 ( |
6d40d6e to
6cd5f28
Compare
| }); | ||
| } | ||
| const nextLength = Math.max(0, ctx.turns.length - numTurns); | ||
| ctx.turns.splice(nextLength); |
There was a problem hiding this comment.
🟡 Medium Layers/CopilotAdapter.ts:826
rollbackThread reports success after only truncating local ctx.turns, while subsequent ctx.sdkSession.send calls still use the SDK conversation containing the rolled-back turns. This makes regenerated answers retain context the user asked to remove; if provider-side rollback is unavailable, return an explicit unsupported-operation error instead of claiming success.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/CopilotAdapter.ts around line 826:
`rollbackThread` reports success after only truncating local `ctx.turns`, while subsequent `ctx.sdkSession.send` calls still use the SDK conversation containing the rolled-back turns. This makes regenerated answers retain context the user asked to remove; if provider-side rollback is unavailable, return an explicit unsupported-operation error instead of claiming success.
| if (/(write|edit|str_replace|create_file|apply_patch|patch|delete|move|rename)/.test(name)) { | ||
| return "file_change"; | ||
| } | ||
| if (/(search|grep|find|fetch|web|glob)/.test(name)) return "web_search"; |
There was a problem hiding this comment.
🟡 Medium sdk/CopilotSdkRuntimeEvents.ts:55
Repository/code-search tools whose names contain grep, find, or glob are emitted as web_search timeline items, mislabeling ordinary local searches. The matcher treats those generic filesystem/search tokens as web searches; remove them so only web-specific tool names use web_search and the others remain dynamic_tool_call.
| if (/(search|grep|find|fetch|web|glob)/.test(name)) return "web_search"; | |
| if (/(search|fetch|web)/.test(name)) return "web_search"; |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/sdk/CopilotSdkRuntimeEvents.ts around line 55:
Repository/code-search tools whose names contain `grep`, `find`, or `glob` are emitted as `web_search` timeline items, mislabeling ordinary local searches. The matcher treats those generic filesystem/search tokens as web searches; remove them so only web-specific tool names use `web_search` and the others remain `dynamic_tool_call`.
| Effect.gen(function* () { | ||
| const spawner = yield* ChildProcessSpawner.ChildProcessSpawner; | ||
| const hostPlatform = yield* HostProcessPlatform; | ||
| const command = ChildProcess.make(copilotSettings.binaryPath, ["version"], { |
There was a problem hiding this comment.
🟡 Medium Layers/CopilotProvider.ts:252
The health probe reports copilot as missing when a GUI-launched process has a restricted PATH, even though the SDK can locate the CLI in its usual install locations. runCopilotVersionCommand passes copilotSettings.binaryPath directly to ChildProcess.make instead of using the SDK's binary resolver, so the status check marks an installed provider as errored; resolve the binary before constructing the version command.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/CopilotProvider.ts around line 252:
The health probe reports `copilot` as missing when a GUI-launched process has a restricted `PATH`, even though the SDK can locate the CLI in its usual install locations. `runCopilotVersionCommand` passes `copilotSettings.binaryPath` directly to `ChildProcess.make` instead of using the SDK's binary resolver, so the status check marks an installed provider as errored; resolve the binary before constructing the version command.
There was a problem hiding this comment.
One retained convention issue in apps/server/src/provider/sdk/CopilotSdkClient.ts: the wrapper's message is still derived from a detail field that copies cause.message. The previously flagged detail interpolation in apps/server/src/provider/Drivers/CopilotDriver.ts:160 is also still open (existing comment).
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
UI consistency review of the changed web files (providerIconUtils.ts, AddProviderInstanceDialog.tsx, providerDriverMeta.ts, session-logic.ts).
Provider registration is consistent: the new copilot driver is added to PROVIDER_CLIENT_DEFINITIONS, PROVIDER_ICON_BY_PROVIDER and PROVIDER_OPTIONS, the stale githubCopilot "Coming Soon" entry is removed alongside its now-unused icon import, and GithubCopilotIcon uses the same fill-black dark:fill-white treatment as the other brand icons, so light/dark tone matches the existing provider rows. No primitive reconstruction, class-override, or CSS-ownership issues found.
One finding: unrelated re-wrapping in providerDriverMeta.ts (details inline).
Posted via Macroscope — UI Consistency
Adds GitHub Copilot as a first-class provider, driven by the first-party
`@github/copilot-sdk`. The SDK spawns and drives the installed `copilot`
runtime binary over its typed JSON-RPC protocol (`RuntimeConnection.forStdio`),
so no extra runtime download is required.
Highlights:
- Provider / driver / adapter under `apps/server/src/provider/` plus a small
`provider/sdk/` layer:
- `CopilotSdkClient` — scoped Effect wrapper around `CopilotClient`.
- `CopilotSdkModels` — maps `client.listModels()` to per-model capabilities:
reasoning effort from each model's `supportedReasoningEfforts`, and a
context-window tier gated on the model's `longContext` billing block.
- `CopilotSdkRuntimeEvents` — translates SDK `SessionEvent`s into the
canonical `ProviderRuntimeEvent` stream.
- Session lifecycle: one shared client, per-thread `CopilotSession`, a
callback→Effect event bridge, permission requests wired into the existing
approval flow, and `send` + `session.idle` turn handling. Reasoning effort
and context tier are applied via `SessionConfig` / `session.setModel`.
- Model discovery via `client.listModels()`.
- Git text generation (commit messages, PR content, branch names, thread
titles) uses the SDK's one-shot `sendAndWait`.
- Contracts: `copilot.sdk.event` / `copilot.sdk.permission` runtime sources and
Copilot settings/model schemas; web UI wiring for settings + model picker.
- Resolves the `copilot` binary to an absolute path before spawning (a GUI app
inherits a minimal PATH), and passes env only on the stdio connection.
Co-authored-by: its-hmny <enea.guidi@n26.com>
|
Round 7 (
|
6cd5f28 to
525c76b
Compare
| detail: `Unknown pending approval request: ${requestId}`, | ||
| }); | ||
| } | ||
| yield* Effect.sync(() => pending.resolve(decision)); |
There was a problem hiding this comment.
🟡 Medium Layers/CopilotAdapter.ts:792
A second respondToRequest call can find the same pending request and return success even though its decision is discarded by the already-resolved promise. Delete the entry from ctx.pendingApprovals before calling pending.resolve(decision) so the request is consumed atomically.
| yield* Effect.sync(() => pending.resolve(decision)); | |
| yield* Effect.sync(() => { | |
| ctx.pendingApprovals.delete(requestId); | |
| pending.resolve(decision); | |
| }); |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/CopilotAdapter.ts around line 792:
A second `respondToRequest` call can find the same pending request and return success even though its decision is discarded by the already-resolved promise. Delete the entry from `ctx.pendingApprovals` before calling `pending.resolve(decision)` so the request is consumed atomically.
| const client = new CopilotClient({ | ||
| // Env goes ONLY on the stdio connection — the SDK rejects setting it at | ||
| // both the client level and the connection level, and prefers the | ||
| // connection-level env for child-process transports. | ||
| connection: RuntimeConnection.forStdio({ path, ...(env ? { env } : {}) }), | ||
| ...(input.logLevel ? { logLevel: input.logLevel } : {}), | ||
| }); | ||
| await client.start(); | ||
| return client; |
There was a problem hiding this comment.
🟠 High sdk/CopilotSdkClient.ts:147
Failed client.start() calls leak the spawned Copilot process, so repeated startup failures can exhaust child-process and system resources. Because start() rejects before Effect.acquireRelease receives client, the release action is never registered; catch the failure, stop (or force-stop) the client, then rethrow it.
const client = new CopilotClient({
@@
- await client.start();
+ try {
+ await client.start();
+ } catch (error) {
+ await client.stop().catch(() => client.forceStop());
+ throw error;
+ }🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/sdk/CopilotSdkClient.ts around lines 147-155:
Failed `client.start()` calls leak the spawned Copilot process, so repeated startup failures can exhaust child-process and system resources. Because `start()` rejects before `Effect.acquireRelease` receives `client`, the release action is never registered; catch the failure, stop (or force-stop) the client, then rethrow it.
| const turnModelSelection = | ||
| input.modelSelection?.instanceId === boundInstanceId ? input.modelSelection : undefined; | ||
| const model = turnModelSelection?.model ?? ctx.session.model; | ||
| const tunables = resolveCopilotSdkTunables(turnModelSelection?.options); |
There was a problem hiding this comment.
🟡 Medium Layers/CopilotAdapter.ts:662
A turn that omits modelSelection clears the reasoning-effort and context-tier settings applied at session start. resolveCopilotSdkTunables(undefined) returns {}, so applyModelSelection detects a change and calls setModel without those tunables; preserve ctx's applied tunables when no per-turn selection is supplied.
- const tunables = resolveCopilotSdkTunables(turnModelSelection?.options);
+ const tunables = turnModelSelection
+ ? resolveCopilotSdkTunables(turnModelSelection.options)
+ : {
+ reasoningEffort:
+ ctx.appliedReasoningEffort as CopilotSdkSessionTunables["reasoningEffort"],
+ contextTier: ctx.appliedContextTier as CopilotSdkSessionTunables["contextTier"],
+ };🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/CopilotAdapter.ts around line 662:
A turn that omits `modelSelection` clears the reasoning-effort and context-tier settings applied at session start. `resolveCopilotSdkTunables(undefined)` returns `{}`, so `applyModelSelection` detects a change and calls `setModel` without those tunables; preserve `ctx`'s applied tunables when no per-turn selection is supplied.
|
All three Macroscope review checks are green now (Correctness, Effect Service Conventions, UI Consistency). The only remaining red check is |
1 participant
What
Adds GitHub Copilot as a first-class provider, alongside Codex / Claude / Cursor / Grok / OpenCode. It's driven by GitHub's first-party
@github/copilot-sdk, which spawns and drives the installedcopilotruntime binary over its typed JSON-RPC protocol (RuntimeConnection.forStdio) — the same engine the Copilot CLI/IDE use. No extra runtime download; it reuses whatevercopilotthe user already has (e.g. Homebrew).How
apps/server/src/provider/, plus a smallprovider/sdk/layer:CopilotSdkClient— scoped Effect wrapper aroundCopilotClient(start on acquire, stop on release; one shared client per provider instance).CopilotSdkModels— mapsclient.listModels()into per-model capabilities: reasoning effort from each model'ssupportedReasoningEfforts, and a context-window tier gated on the model'slongContextbilling block. Applied viaSessionConfig/session.setModel.CopilotSdkRuntimeEvents— translates SDKSessionEvents into the canonicalProviderRuntimeEventstream.CopilotSession, a callback→Effect event bridge (SDK is callback-based, unlike the async-iterable providers), permission requests wired into the existing approval flow, andsend+session.idleturn handling.client.listModels().sendAndWait.copilot.sdk.event/copilot.sdk.permissionruntime sources and Copilot settings/model schemas; web wiring for the provider settings + model picker (reuses the generic option-descriptor UI).copilotbinary to an absolute path before spawning (a GUI-launched app inherits a minimal PATH), and passes env only on the stdio connection (the SDK rejects env in both places).Testing
tsgotypecheck clean (contracts / server / web); targeted lint clean.copilot1.0.80:createSession→ streamingsend→session.idle,setModelwithlong_context, and the permission callback.listModels()returns per-model reasoning efforts / context tiers as expected.Notes / open questions
Co-authored-by); I ported it to the SDK and reworked model discovery + tunables.Authored with Claude Opus 4.8 via Claude Code.
Note
Add GitHub Copilot provider via
@github/copilot-sdkcopilotprovider driver registered in builtInDrivers.ts, wiring together a SDK client, adapter, text generation, and provider snapshot pipeline.copilotkind, default togpt-4.1/gpt-4.1-mini, and surface the provider in pickers and settings dialogs.@github/copilot-sdkdependency and disallowskoffibuilds in pnpm-workspace.yaml;COPILOT_DRIVER_KINDdefaults in model.ts may affect model selection if overrides are absent.📊 Macroscope summarized 56d5f9a. 17 files reviewed, 16 issues evaluated, 2 issues filtered, 10 comments posted
🗂️ Filtered Issues
apps/server/src/provider/Layers/CopilotAdapter.ts — 3 comments posted, 5 evaluated, 1 filtered
nativeEventLogger, but never invokes itswritemethod. Therefore configuringnativeEventLogPathor injecting a native logger creates/manages the logger while every Copilot SDK event is silently omitted from the native event log. [ Out of scope (post-validation triage) ]apps/server/src/provider/Layers/CopilotProvider.ts — 2 comments posted, 3 evaluated, 1 filtered
detectCopilotAuthFromEnvironmentuses nullish coalescing before checking whether a token is nonempty. IfCOPILOT_GITHUB_TOKENis defined as""or whitespace whileGH_TOKENorGITHUB_TOKENcontains a valid token, the empty first value wins and the function returnsunknowninstead ofauthenticated. Select the first nonblank value rather than the first non-nullish one. [ Out of scope (post-validation triage) ]