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 Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
93bc16e to
1cc00a5
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
|
This pr can now be reviewed im done with my stuff |
| }); | ||
| } | ||
|
|
||
| const existing = sessions.get(input.threadId); |
There was a problem hiding this comment.
🟢 Low Layers/CopilotAdapter.ts:1679
Concurrent startSession calls for the same threadId race at the sessions.get check (line 1679): both callers find no existing session, both create new clients and sessions, and the second sessions.set (line 1778) overwrites the first. The first session's client and session become unreachable and leak. Consider synchronizing in-flight session creation, e.g., by tracking pending promises in a separate map keyed by threadId.
🤖 Copy this AI Prompt to have your agent fix this:
In file apps/server/src/provider/Layers/CopilotAdapter.ts around line 1679:
Concurrent `startSession` calls for the same `threadId` race at the `sessions.get` check (line 1679): both callers find no existing session, both create new clients and sessions, and the second `sessions.set` (line 1778) overwrites the first. The first session's client and session become unreachable and leak. Consider synchronizing in-flight session creation, e.g., by tracking pending promises in a separate map keyed by `threadId`.
Evidence trail:
apps/server/src/provider/Layers/CopilotAdapter.ts line 844: `const sessions = new Map<ThreadId, ActiveCopilotSession>();` - plain Map with no synchronization
apps/server/src/provider/Layers/CopilotAdapter.ts line 1679: `const existing = sessions.get(input.threadId);` - the check
apps/server/src/provider/Layers/CopilotAdapter.ts lines 1720-1726: `yield* validateSessionConfiguration(...)` - first async yield point between check and set
apps/server/src/provider/Layers/CopilotAdapter.ts lines 1728-1763: `yield* Effect.tryPromise(...)` - second async yield point between check and set
apps/server/src/provider/Layers/CopilotAdapter.ts line 1778: `sessions.set(input.threadId, record);` - the set that overwrites any racing calls
| ...(input.cwd ? { cwd: input.cwd } : {}), | ||
| logLevel: "error", | ||
| }; | ||
| const client = options?.clientFactory?.(clientOptions) ?? new CopilotClient(clientOptions); |
There was a problem hiding this comment.
🟡 Medium Layers/CopilotAdapter.ts:1705
If session creation fails after client is instantiated (lines 1720–1778), the CopilotClientHandle is never cleaned up because client.stop() is only called in stopRecord, which requires the session to be in the sessions map. Since CopilotClient spawns a subprocess, this leaks orphan processes when validateSessionConfiguration fails or Effect.tryPromise throws. Consider wrapping client creation in Effect.acquireRelease to ensure client.stop() runs on failure.
- const client = options?.clientFactory?.(clientOptions) ?? new CopilotClient(clientOptions);
+ const client = yield* Effect.acquireRelease(
+ Effect.sync(() => options?.clientFactory?.(clientOptions) ?? new CopilotClient(clientOptions)),
+ (client) => Effect.promise(() => client.stop())
+ );🤖 Copy this AI Prompt to have your agent fix this:
In file apps/server/src/provider/Layers/CopilotAdapter.ts around line 1705:
If session creation fails after `client` is instantiated (lines 1720–1778), the `CopilotClientHandle` is never cleaned up because `client.stop()` is only called in `stopRecord`, which requires the session to be in the `sessions` map. Since `CopilotClient` spawns a subprocess, this leaks orphan processes when `validateSessionConfiguration` fails or `Effect.tryPromise` throws. Consider wrapping client creation in `Effect.acquireRelease` to ensure `client.stop()` runs on failure.
Evidence trail:
apps/server/src/provider/Layers/CopilotAdapter.ts lines 1700-1705 (client instantiation), lines 1720-1725 (validateSessionConfiguration call), lines 1460-1527 (validateSessionConfiguration definition with client.start() at line 1471 and multiple failure points), lines 1727-1759 (session creation that can fail), lines 1764-1772 (record added to sessions only after all succeeds), lines 1639-1661 (stopRecord function definition), line 1647 (only location where client.stop() is called). No Effect.acquireRelease/ensuring/onInterrupt patterns found via git_grep.

What Changed
Why
UI Changes
Checklist
Note
Add GitHub Copilot as a supported provider across server, web, and contracts
CopilotAdapter.tsthat wires@github/copilot-sdkinto the provider system, handling session lifecycle, turn tracking, event streaming, and model switching.model.tsand updates contract schemas inorchestration.tsto accept'copilot'as a valid provider kind.appSettings.ts), the settings UI (_chat.settings.tsx), the composer (composerProviderRegistry.tsx), and the provider/model picker to expose Copilot configuration including CLI path, config directory, and custom models.copilotCliPath.ts) that locates the bundled Copilot binary or npm loader across platforms and packaging layouts (Node/Electron).Macroscope summarized 5c6b51e.
Note
High Risk
Introduces a new first-class provider that integrates the
@github/copilot-sdkinto core session/turn/event handling and health checks; mistakes could affect orchestration event mapping, permission gating, and provider routing across the app.Overview
Adds GitHub Copilot as a first-class provider end-to-end.
On the server, introduces
CopilotAdapterLive(with turn tracking, interaction-mode switching, model/reasoning-effort configuration, tool-event mapping, permission + user-input request bridging, and event streaming), wires it intoProviderAdapterRegistry, persisted session directory decoding, and provider health via a Copilot SDK probe plus bundled CLI path resolution.On the web, extends settings/UI and composer plumbing to support Copilot (custom models, provider install overrides for
cliPath/configDir, provider picker + icons, model-selection construction, and effort/options handling), and improves timeline tool rendering (better icon fallbacks and preferringchangedFilesover raw diff text). Extensive new tests cover Copilot adapter behavior and CLI path/turn-tracking utilities.Written by Cursor Bugbot for commit cb92885. This will update automatically on new commits. Configure here.