Conversation
github-actions
bot
added
size:XXL
|
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 Tip You can make CodeRabbit's review stricter and more nitpicky using the `assertive` profile, if that's what you prefer.Change the |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟢 Low
t3code/apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts
Lines 222 to 228 in 0a91001
The local const runtime declared at line 222 shadows the outer let runtime at line 64, so the outer variable remains null. Consequently, afterEach never calls runtime.dispose(), leaking the ManagedRuntime and its resources (SQLite connections, layer finalizers) on every test. Rename the local variable so the outer runtime is assigned.
- const runtime = ManagedRuntime.make(layer);
+ const testRuntime = ManagedRuntime.make(layer);
- const engine = await runtime.runPromise(Effect.service(OrchestrationEngineService));
- const reactor = await runtime.runPromise(Effect.service(ProviderCommandReactor));
+ const engine = await testRuntime.runPromise(Effect.service(OrchestrationEngineService));
+ const reactor = await testRuntime.runPromise(Effect.service(ProviderCommandReactor));
scope = await Effect.runPromise(Scope.make("sequential"));
await Effect.runPromise(reactor.start.pipe(Scope.provide(scope)));
const drain = () => Effect.runPromise(reactor.drain);🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts around lines 222-228:
The local `const runtime` declared at line 222 shadows the outer `let runtime` at line 64, so the outer variable remains `null`. Consequently, `afterEach` never calls `runtime.dispose()`, leaking the `ManagedRuntime` and its resources (SQLite connections, layer finalizers) on every test. Rename the local variable so the outer `runtime` is assigned.
Evidence trail:
apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts lines 64-67 (outer `let runtime = null`), lines 71-79 (`afterEach` that checks `if (runtime)` and calls `runtime.dispose()`), lines 86-269 (`createHarness` function), line 222 (`const runtime = ManagedRuntime.make(layer);` - local shadowing), lines 258-269 (return object does not include runtime)
| export function clearTurnTracking(state: CopilotTurnTrackingState): void { | ||
| state.currentTurnId = undefined; | ||
| state.currentProviderTurnId = undefined; | ||
| state.pendingCompletionTurnId = undefined; | ||
| state.pendingCompletionProviderTurnId = undefined; | ||
| state.pendingTurnUsage = undefined; | ||
| } |
There was a problem hiding this comment.
🟠 High Layers/copilotTurnTracking.ts:43
clearTurnTracking does not clear pendingTurnIds, so stale turn IDs persist when a session is aborted or goes idle before assistant.turn_start. On the next turn, beginCopilotTurn shifts the old stale ID instead of the new one, causing a turn ID mismatch. Consider adding state.pendingTurnIds = [] to clear the array.
export function clearTurnTracking(state: CopilotTurnTrackingState): void {
state.currentTurnId = undefined;
state.currentProviderTurnId = undefined;
state.pendingCompletionTurnId = undefined;
state.pendingCompletionProviderTurnId = undefined;
state.pendingTurnUsage = undefined;
+ state.pendingTurnIds = [];
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/Layers/copilotTurnTracking.ts around lines 43-49:
`clearTurnTracking` does not clear `pendingTurnIds`, so stale turn IDs persist when a session is aborted or goes idle before `assistant.turn_start`. On the next turn, `beginCopilotTurn` shifts the old stale ID instead of the new one, causing a turn ID mismatch. Consider adding `state.pendingTurnIds = []` to clear the array.
Evidence trail:
apps/server/src/provider/Layers/copilotTurnTracking.ts lines 43-49: `clearTurnTracking` function clears all state fields except `pendingTurnIds`. Lines 6-13: `CopilotTurnTrackingState` interface shows `pendingTurnIds: Array<TurnId>` is part of the state. Lines 22-28: `beginCopilotTurn` uses `state.pendingTurnIds.shift()` to obtain the current turn ID. Line 65: `isCopilotTurnTerminalEvent` checks for 'abort' or 'session.idle' events.
| threadId: input.threadId, | ||
| createdAt: input.createdAt, | ||
| ...(input.turnId ? { turnId: input.turnId } : {}), | ||
| ...(input.itemId ? { itemId: toRuntimeItemId(input.itemId) } : {}), |
There was a problem hiding this comment.
🟢 Low Layers/CopilotAdapter.ts:313
The truthy-check at line 313 (input.itemId ?) allows whitespace-only strings like " " to pass, but toRuntimeItemId returns undefined for these. This spreads { itemId: undefined } into the result, explicitly setting the field rather than omitting it. Consider also checking input.itemId.trim() before calling toRuntimeItemId, or have toRuntimeItemId throw on invalid input instead of returning undefined.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/Layers/CopilotAdapter.ts around line 313:
The truthy-check at line 313 (`input.itemId ?`) allows whitespace-only strings like `" "` to pass, but `toRuntimeItemId` returns `undefined` for these. This spreads `{ itemId: undefined }` into the result, explicitly setting the field rather than omitting it. Consider also checking `input.itemId.trim()` before calling `toRuntimeItemId`, or have `toRuntimeItemId` throw on invalid input instead of returning `undefined`.
Evidence trail:
apps/server/src/provider/Layers/CopilotAdapter.ts:313 - `...(input.itemId ? { itemId: toRuntimeItemId(input.itemId) } : {})`
apps/server/src/provider/Layers/CopilotAdapter.ts:162-165 - `toRuntimeItemId` implementation that returns `undefined` for whitespace-only strings via `value.trim().length === 0` check
| const attachmentPath = resolveAttachmentPath({ | ||
| stateDir: serverConfig.stateDir, | ||
| attachment, | ||
| }); |
There was a problem hiding this comment.
🟠 High Layers/CopilotAdapter.ts:1403
Using throw inside Effect.gen at lines 1405-1409 causes ProviderAdapterRequestError to become an untyped defect (Die) rather than a typed failure (Fail). Downstream error handlers expecting ProviderAdapterError won't catch it. Consider using yield* Effect.fail(new ProviderAdapterRequestError(...)) instead.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/Layers/CopilotAdapter.ts around line 1403:
Using `throw` inside `Effect.gen` at lines 1405-1409 causes `ProviderAdapterRequestError` to become an untyped defect (`Die`) rather than a typed failure (`Fail`). Downstream error handlers expecting `ProviderAdapterError` won't catch it. Consider using `yield* Effect.fail(new ProviderAdapterRequestError(...))` instead.
Evidence trail:
apps/server/src/provider/Layers/CopilotAdapter.ts lines 1388-1420 (REVIEWED_COMMIT): `sendTurn` defined as `Effect.gen(function* () {...})` with `throw new ProviderAdapterRequestError(...)` at line 1405 inside a `.map()` callback.
apps/server/src/provider/Layers/CopilotAdapter.ts lines 1448-1454: Contrasting pattern showing correct usage with `Effect.tryPromise({ catch: (cause) => new ProviderAdapterRequestError(...) })`.
apps/server/src/provider/Layers/CopilotAdapter.ts lines 1500, 1532: Examples showing correct pattern `return yield* new ProviderAdapterRequestError(...)`.
apps/server/src/provider/Services/ProviderAdapter.ts lines 62-64: Type definition showing `sendTurn: (input) => Effect.Effect<ProviderTurnStartResult, TError>` where TError is ProviderAdapterError.
apps/server/src/provider/Services/CopilotAdapter.ts lines 6-9: CopilotAdapterShape extends ProviderAdapterShape<ProviderAdapterError>.
1 participant
What Changed
Why
UI Changes
Checklist
Note
Add GitHub Copilot as a supported AI provider
CopilotAdapterservice in CopilotAdapter.ts that integrates@github/copilot-sdk, supporting session start/resume, turn sending/aborting, interactive permission prompts, user-input requests, model/reasoning-effort switching, and thread reading.claude-sonnet-4.6), reasoning effort options, and persisted settings forcopilotCliPathandcopilotConfigDir.PROVIDER_OPTIONSentry labeled 'GitHub Copilot'.@github/copilot-sdk@0.1.32to fix an ESM import path indist/session.js.📊 Macroscope summarized 0a91001. 33 files reviewed, 8 issues evaluated, 1 issue filtered, 4 comments posted
🗂️ Filtered Issues
apps/web/src/appSettings.test.ts — 0 comments posted, 1 evaluated, 1 filtered
claude-sonnet-4.6(with a period) for the copilot provider, but the analogous claudeAgent test on line 227 expectsclaude-sonnet-4-6(with a hyphen). This inconsistency suggests a copy-paste typo - either the model slug format is wrong (period vs hyphen), or the copilot test should be checking for a copilot-specific normalized slug rather than a claude model slug. [ Out of scope (triage) ]