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)
📝 Coding Plan
Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: SDK stream fiber is detached and unmanaged
- Replaced
Effect.runForkwithEffect.forkChildto keep the fiber in the managed runtime, stored the fiber reference in session context, and added explicitFiber.interruptinstopSessionInternalbefore queue teardown to eliminate the race window.
- Replaced
- ✅ Fixed: Identity function adds unnecessary indirection
- Removed the no-op
asCanonicalTurnIdidentity function and inlined theTurnIdvalue directly at all 10 call sites.
- Removed the no-op
Or push these changes by commenting:
@cursor push 2efc9d6c0c
Preview (2efc9d6c0c)
diff --git a/apps/server/src/provider/Layers/ClaudeCodeAdapter.ts b/apps/server/src/provider/Layers/ClaudeCodeAdapter.ts
--- a/apps/server/src/provider/Layers/ClaudeCodeAdapter.ts
+++ b/apps/server/src/provider/Layers/ClaudeCodeAdapter.ts
@@ -34,7 +34,7 @@
ThreadId,
TurnId,
} from "@t3tools/contracts";
-import { Cause, DateTime, Deferred, Effect, Layer, Queue, Random, Ref, Stream } from "effect";
+import { Cause, DateTime, Deferred, Effect, Fiber, Layer, Queue, Random, Ref, Stream } from "effect";
import {
ProviderAdapterProcessError,
@@ -106,6 +106,7 @@
lastAssistantUuid: string | undefined;
lastThreadStartedId: string | undefined;
stopped: boolean;
+ streamFiber: Fiber.Fiber<void, never> | undefined;
}
interface ClaudeQueryRuntime extends AsyncIterable<SDKMessage> {
@@ -144,10 +145,6 @@
return RuntimeItemId.makeUnsafe(value);
}
-function asCanonicalTurnId(value: TurnId): TurnId {
- return value;
-}
-
function asRuntimeRequestId(value: ApprovalRequestId): RuntimeRequestId {
return RuntimeRequestId.makeUnsafe(value);
}
@@ -505,7 +502,7 @@
...(typeof message.session_id === "string"
? { providerThreadId: message.session_id }
: {}),
- ...(context.turnState ? { turnId: asCanonicalTurnId(context.turnState.turnId) } : {}),
+ ...(context.turnState ? { turnId: context.turnState.turnId } : {}),
...(itemId ? { itemId: ProviderItemId.makeUnsafe(itemId) } : {}),
payload: message,
},
@@ -613,7 +610,7 @@
provider: PROVIDER,
createdAt: stamp.createdAt,
threadId: context.session.threadId,
- ...(turnState ? { turnId: asCanonicalTurnId(turnState.turnId) } : {}),
+ ...(turnState ? { turnId: turnState.turnId } : {}),
payload: {
message,
class: "provider_error",
@@ -640,7 +637,7 @@
provider: PROVIDER,
createdAt: stamp.createdAt,
threadId: context.session.threadId,
- ...(turnState ? { turnId: asCanonicalTurnId(turnState.turnId) } : {}),
+ ...(turnState ? { turnId: turnState.turnId } : {}),
payload: {
message,
...(detail !== undefined ? { detail } : {}),
@@ -855,7 +852,7 @@
provider: PROVIDER,
createdAt: stamp.createdAt,
threadId: context.session.threadId,
- ...(context.turnState ? { turnId: asCanonicalTurnId(context.turnState.turnId) } : {}),
+ ...(context.turnState ? { turnId: context.turnState.turnId } : {}),
itemId: asRuntimeItemId(tool.itemId),
payload: {
itemType: tool.itemType,
@@ -896,7 +893,7 @@
provider: PROVIDER,
createdAt: stamp.createdAt,
threadId: context.session.threadId,
- ...(context.turnState ? { turnId: asCanonicalTurnId(context.turnState.turnId) } : {}),
+ ...(context.turnState ? { turnId: context.turnState.turnId } : {}),
itemId: asRuntimeItemId(tool.itemId),
payload: {
itemType: tool.itemType,
@@ -1006,7 +1003,7 @@
provider: PROVIDER,
createdAt: stamp.createdAt,
threadId: context.session.threadId,
- ...(context.turnState ? { turnId: asCanonicalTurnId(context.turnState.turnId) } : {}),
+ ...(context.turnState ? { turnId: context.turnState.turnId } : {}),
providerRefs: {
...providerThreadRef(context),
...(context.turnState ? { providerTurnId: context.turnState.turnId } : {}),
@@ -1165,7 +1162,7 @@
provider: PROVIDER,
createdAt: stamp.createdAt,
threadId: context.session.threadId,
- ...(context.turnState ? { turnId: asCanonicalTurnId(context.turnState.turnId) } : {}),
+ ...(context.turnState ? { turnId: context.turnState.turnId } : {}),
providerRefs: {
...providerThreadRef(context),
...(context.turnState ? { providerTurnId: context.turnState.turnId } : {}),
@@ -1295,6 +1292,11 @@
context.stopped = true;
+ if (context.streamFiber) {
+ yield* Fiber.interrupt(context.streamFiber);
+ context.streamFiber = undefined;
+ }
+
for (const [requestId, pending] of context.pendingApprovals) {
yield* Deferred.succeed(pending.decision, "cancel");
const stamp = yield* makeEventStamp();
@@ -1304,7 +1306,7 @@
provider: PROVIDER,
createdAt: stamp.createdAt,
threadId: context.session.threadId,
- ...(context.turnState ? { turnId: asCanonicalTurnId(context.turnState.turnId) } : {}),
+ ...(context.turnState ? { turnId: context.turnState.turnId } : {}),
requestId: asRuntimeRequestId(requestId),
payload: {
requestType: pending.requestType,
@@ -1442,7 +1444,7 @@
provider: PROVIDER,
createdAt: requestedStamp.createdAt,
threadId: context.session.threadId,
- ...(context.turnState ? { turnId: asCanonicalTurnId(context.turnState.turnId) } : {}),
+ ...(context.turnState ? { turnId: context.turnState.turnId } : {}),
requestId: asRuntimeRequestId(requestId),
payload: {
requestType,
@@ -1494,7 +1496,7 @@
provider: PROVIDER,
createdAt: resolvedStamp.createdAt,
threadId: context.session.threadId,
- ...(context.turnState ? { turnId: asCanonicalTurnId(context.turnState.turnId) } : {}),
+ ...(context.turnState ? { turnId: context.turnState.turnId } : {}),
requestId: asRuntimeRequestId(requestId),
payload: {
requestType,
@@ -1610,6 +1612,7 @@
lastAssistantUuid: resumeState?.resumeSessionAt,
lastThreadStartedId: undefined,
stopped: false,
+ streamFiber: undefined,
};
yield* Ref.set(contextRef, context);
sessions.set(threadId, context);
@@ -1658,7 +1661,7 @@
providerRefs: {},
});
- Effect.runFork(runSdkStream(context));
+ context.streamFiber = yield* Effect.forkChild(runSdkStream(context));
return {
...session,![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
juliusmarminge
force-pushed
the
codething/648ca884-claude
branch
from
2b53034 to
1beeff2
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Native event logger missing threadId in event payload
- Added
threadId: context.session.threadIdto the event object and passedcontext.session.threadIdinstead ofnullas the second argument tonativeEventLogger.write(), enabling per-thread log routing consistent with the Codex adapter.
- Added
Or push these changes by commenting:
@cursor push 8489584240
Preview (8489584240)
diff --git a/apps/server/src/provider/Layers/ClaudeCodeAdapter.ts b/apps/server/src/provider/Layers/ClaudeCodeAdapter.ts
--- a/apps/server/src/provider/Layers/ClaudeCodeAdapter.ts
+++ b/apps/server/src/provider/Layers/ClaudeCodeAdapter.ts
@@ -502,6 +502,7 @@
provider: PROVIDER,
createdAt: observedAt,
method: sdkNativeMethod(message),
+ threadId: context.session.threadId,
...(typeof message.session_id === "string"
? { providerThreadId: message.session_id }
: {}),
@@ -510,7 +511,7 @@
payload: message,
},
},
- null,
+ context.session.threadId,
);
});
juliusmarminge
force-pushed
the
codething/648ca884-claude
branch
4 times, most recently
from
4afd04a to
3af67f5
Compare
| customClaudeModels: Schema.Array(Schema.String).pipe( | ||
| Schema.withConstructorDefault(() => Option.some([])), | ||
| ), |
There was a problem hiding this comment.
🟢 Low src/appSettings.ts:29
Adding customClaudeModels as a required field causes Schema.decodeSync to throw when decoding existing localStorage data that lacks this key. The caught error triggers fallback to DEFAULT_APP_SETTINGS, permanently discarding the user's saved configuration. Mark the field optional with a decode-time default instead of using withConstructorDefault.
- customClaudeModels: Schema.Array(Schema.String).pipe(
- Schema.withConstructorDefault(() => Option.some([])),
- ),Also found in 1 other location(s)
apps/web/src/routes/_chat.settings.tsx:65
The
getCustomModelsForProviderfunction directly returnssettings.customClaudeModels. SincecustomClaudeModelsis a new property added to the schema in this PR, the persistedsettingsobject in local storage for existing users will not contain this key. This causes the function to returnundefined. Consuming code (e.g.,SettingsRouteViewat lines 164 and 398) assumes an array and will crash with aTypeErrorwhen accessing.lengthor.includes()onundefined. The accessor should default to an empty array (e.g.,settings.customClaudeModels ?? []) to handle the schema migration for existing data.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/appSettings.ts around lines 29-31:
Adding `customClaudeModels` as a required field causes `Schema.decodeSync` to throw when decoding existing `localStorage` data that lacks this key. The caught error triggers fallback to `DEFAULT_APP_SETTINGS`, permanently discarding the user's saved configuration. Mark the field optional with a decode-time default instead of using `withConstructorDefault`.
Evidence trail:
apps/web/src/appSettings.ts lines 29-31 (customClaudeModels definition with withConstructorDefault), lines 134-143 (parsePersistedSettings using decodeSync with catch fallback to DEFAULT_APP_SETTINGS). Git diff (MERGE_BASE..REVIEWED_COMMIT) shows customClaudeModels being added. Effect-TS issue #1997 (https://github.com/Effect-TS/effect/issues/1997) confirms withConstructorDefault provides defaults at construction time only, not during decode: "I often find myself needing to provide default values at construction, but not at parsing."
Also found in 1 other location(s):
- apps/web/src/routes/_chat.settings.tsx:65 -- The `getCustomModelsForProvider` function directly returns `settings.customClaudeModels`. Since `customClaudeModels` is a new property added to the schema in this PR, the persisted `settings` object in local storage for existing users will not contain this key. This causes the function to return `undefined`. Consuming code (e.g., `SettingsRouteView` at lines 164 and 398) assumes an array and will crash with a `TypeError` when accessing `.length` or `.includes()` on `undefined`. The accessor should default to an empty array (e.g., `settings.customClaudeModels ?? []`) to handle the schema migration for existing data.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Redundant duplicate
threadIdassignment in session construction- Removed the redundant
...(threadId ? { threadId } : {})spread at line 1587 sincethreadIdis already set directly at line 1581 and is always defined.
- Removed the redundant
Or push these changes by commenting:
@cursor push 1970102289
Preview (1970102289)
diff --git a/apps/server/src/provider/Layers/ClaudeCodeAdapter.ts b/apps/server/src/provider/Layers/ClaudeCodeAdapter.ts
--- a/apps/server/src/provider/Layers/ClaudeCodeAdapter.ts
+++ b/apps/server/src/provider/Layers/ClaudeCodeAdapter.ts
@@ -1584,7 +1584,6 @@
runtimeMode: input.runtimeMode,
...(input.cwd ? { cwd: input.cwd } : {}),
...(input.model ? { model: input.model } : {}),
- ...(threadId ? { threadId } : {}),
resumeCursor: {
...(threadId ? { threadId } : {}),
...(resumeState?.resume ? { resume: resumeState.resume } : {}),
| async respondToUserInput( | ||
| threadId: ThreadId, | ||
| requestId: ApprovalRequestId, | ||
| answers: ProviderUserInputAnswers, | ||
| ): Promise<void> { | ||
| const context = this.requireSession(threadId); | ||
| const pendingRequest = context.pendingUserInputs.get(requestId); | ||
| if (!pendingRequest) { | ||
| throw new Error(`Unknown pending user input request: ${requestId}`); | ||
| } | ||
|
|
||
| context.pendingUserInputs.delete(requestId); | ||
| const codexAnswers = toCodexUserInputAnswers(answers); | ||
| this.writeMessage(context, { |
There was a problem hiding this comment.
🟡 Medium src/codexAppServerManager.ts:807
In respondToUserInput, context.pendingUserInputs.delete(requestId) is called before validating answers via toCodexUserInputAnswers. If conversion throws, the request record is permanently lost but no response reaches the provider, leaving the session corrupted and blocking retries. Move the deletion after successful validation and conversion.
async respondToUserInput(
threadId: ThreadId,
requestId: ApprovalRequestId,
answers: ProviderUserInputAnswers,
): Promise<void> {
const context = this.requireSession(threadId);
const pendingRequest = context.pendingUserInputs.get(requestId);
if (!pendingRequest) {
throw new Error(`Unknown pending user input request: ${requestId}`);
}
- context.pendingUserInputs.delete(requestId);
const codexAnswers = toCodexUserInputAnswers(answers);
+ context.pendingUserInputs.delete(requestId);
this.writeMessage(context, {
id: pendingRequest.jsonRpcId,
result: {🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/codexAppServerManager.ts around lines 807-820:
In `respondToUserInput`, `context.pendingUserInputs.delete(requestId)` is called before validating `answers` via `toCodexUserInputAnswers`. If conversion throws, the request record is permanently lost but no response reaches the provider, leaving the session corrupted and blocking retries. Move the deletion after successful validation and conversion.
Evidence trail:
apps/server/src/codexAppServerManager.ts lines 807-825 (REVIEWED_COMMIT): `respondToUserInput` method showing delete at line 818, conversion at line 819
apps/server/src/codexAppServerManager.ts lines 358-381 (REVIEWED_COMMIT): `toCodexUserInputAnswer` function that throws Error at line 370, and `toCodexUserInputAnswers` that calls it
Line 818: `context.pendingUserInputs.delete(requestId);`
Line 819: `const codexAnswers = toCodexUserInputAnswers(answers);`
Line 370: `throw new Error("User input answers must be strings or arrays of strings.");`
| this.runPromise = services ? Effect.runPromiseWith(services) : Effect.runPromise; | ||
| } | ||
|
|
||
| async startSession(input: CodexAppServerStartSessionInput): Promise<ProviderSession> { |
There was a problem hiding this comment.
🟠 High src/codexAppServerManager.ts:428
startSession overwrites this.sessions.set(threadId, context) without checking if a session already exists for that threadId. This orphans the previous process (which keeps running) and leaves its exit handler active. When the orphaned process later exits, that handler calls this.sessions.delete(threadId), which removes the new session from the map. Subsequent calls like sendTurn then fail with "Unknown session" because the entry was deleted by the stale handler.
Also found in 1 other location(s)
apps/server/src/provider/Layers/CodexAdapter.ts:1262
The
nativeEventLoggercreated whenoptions.nativeEventLogPathis provided is never closed. Whilemanageris wrapped inEffect.acquireReleasefor cleanup,nativeEventLoggeris instantiated separately and itsclose()method is never called. This causes file handles opened by the logger to leak every time theCodexAdapterlayer is released (e.g., during configuration reloads or tests).
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/codexAppServerManager.ts around line 428:
`startSession` overwrites `this.sessions.set(threadId, context)` without checking if a session already exists for that `threadId`. This orphans the previous process (which keeps running) and leaves its `exit` handler active. When the orphaned process later exits, that handler calls `this.sessions.delete(threadId)`, which removes the *new* session from the map. Subsequent calls like `sendTurn` then fail with "Unknown session" because the entry was deleted by the stale handler.
Evidence trail:
apps/server/src/codexAppServerManager.ts: lines 428-471 show `startSession` calls `this.sessions.set(threadId, context)` without checking for existing session; line 943 shows exit handler calls `this.sessions.delete(context.session.threadId)` where `context` is captured in closure; lines 890-893 show `requireSession` throws 'Unknown session' when entry not in map; lines 606-607 show `sendTurn` uses `requireSession`.
Also found in 1 other location(s):
- apps/server/src/provider/Layers/CodexAdapter.ts:1262 -- The `nativeEventLogger` created when `options.nativeEventLogPath` is provided is never closed. While `manager` is wrapped in `Effect.acquireRelease` for cleanup, `nativeEventLogger` is instantiated separately and its `close()` method is never called. This causes file handles opened by the logger to leak every time the `CodexAdapter` layer is released (e.g., during configuration reloads or tests).
| export const OpenCodeIcon: Icon = (props) => ( | ||
| <svg {...props} viewBox="0 0 32 40" fill="none" xmlns="http://www.w3.org/2000/svg"> | ||
| <g clipPath="url(#opencode__clip0_1311_94969)"> | ||
| <path d="M24 32H8V16H24V32Z" fill="#BCBBBB" /> | ||
| <path d="M24 8H8V32H24V8ZM32 40H0V0H32V40Z" fill="#211E1E" /> | ||
| </g> | ||
| <defs> | ||
| <clipPath id="opencode__clip0_1311_94969"> | ||
| <rect width="32" height="40" fill="white" /> | ||
| </clipPath> | ||
| </defs> | ||
| </svg> | ||
| ); |
There was a problem hiding this comment.
🟢 Low components/Icons.tsx:329
The OpenCodeIcon component uses hardcoded fill attributes (#211E1E and #BCBBBB) on its SVG paths. In dark mode, the #211E1E dark grey fill renders the icon nearly invisible due to low contrast against dark backgrounds. Unlike GitHubIcon and OpenAI, which use currentColor to inherit the surrounding text color, this icon fails to adapt to the theme.
- <svg {...props} viewBox="0 0 32 40" fill="none" xmlns="http://www.w3.org/2000/svg">
+ <svg {...props} viewBox="0 0 32 40" fill="none" xmlns="http://www.w3.org/2000/svg" fill="currentColor">
<g clipPath="url(#opencode__clip0_1311_94969)">
- <path d="M24 32H8V16H24V32Z" fill="#BCBBBB" />
- <path d="M24 8H8V32H24V8ZM32 40H0V0H32V40Z" fill="#211E1E" />
+ <path d="M24 32H8V16H24V32Z" fill="currentColor" />
+ <path d="M24 8H8V32H24V8ZM32 40H0V0H32V40Z" fill="currentColor" />🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/components/Icons.tsx around lines 329-341:
The `OpenCodeIcon` component uses hardcoded `fill` attributes (`#211E1E` and `#BCBBBB`) on its SVG paths. In dark mode, the `#211E1E` dark grey fill renders the icon nearly invisible due to low contrast against dark backgrounds. Unlike `GitHubIcon` and `OpenAI`, which use `currentColor` to inherit the surrounding text color, this icon fails to adapt to the theme.
Evidence trail:
apps/web/src/components/Icons.tsx lines 329-342 (OpenCodeIcon with hardcoded fills #211E1E and #BCBBBB), lines 5-15 (GitHubIcon with fill="currentColor" at line 12), lines 146-150 (OpenAI with fill="currentColor" at line 147). Verified at commit REVIEWED_COMMIT.
|
I prepared a CI fix PR targeting this branch: #243\n\nIt updates stale ClaudeCodeAdapter test expectations for thread identity/providerThreadId behavior and aligns with current adapter semantics. |
Fix detached SDK stream fiber by replacing Effect.runFork with Effect.forkChild and adding explicit Fiber.interrupt on session stop. Add missing threadId to native event logger for per-thread log routing. Remove no-op asCanonicalTurnId identity function. Remove redundant threadId spread in session construction. Fix stale test expectations for thread identity behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…apters Merge the Claude Code adapter (PR pingdotgg#179) into main, resolving 45 conflicts caused by the deliberate split of provider stacks on March 5. Key additions: - ClaudeCodeAdapter with full session, turn, and resume lifecycle - Cursor provider support (model catalog, UI, routing) - ProviderKind expanded from "codex" to "codex" | "claudeCode" | "cursor" - Provider model catalogs, aliases, and slug resolution across all providers - UI support for Claude Code and Cursor in ChatView, settings, and composer Conflict resolution strategy: - Kept HEAD's refactored patterns (scoped finalizers, telemetry, serviceTier) - Added PR's new provider routing, adapters, and UI components - Fixed duplicate declarations, missing props, and type mismatches Typecheck: 7/7 packages pass Lint: 0 warnings, 0 errors Tests: 419/422 pass (3 pre-existing failures in ClaudeCodeAdapter.test.ts) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove filtro que escondia o Claude Code do seletor de providers, tornando-o selecionável após o merge do adapter (PR pingdotgg#179). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add first-class Claude Code support alongside existing Codex provider: - Add ClaudeCodeAdapter (1857 lines) backed by @anthropic-ai/claude-agent-sdk - Extend ProviderKind to accept "codex" | "claudeCode" - Add Claude model catalog (Opus 4.6, Sonnet 4.6, Haiku 4.5) - Register ClaudeCodeAdapter in provider registry and server layers - Add Claude SDK event sources to provider runtime events - Enable Claude Code in UI provider picker - Add ClaudeCodeProviderStartOptions to contracts - Update all tests for multi-provider support Based on upstream PR pingdotgg#179 by juliusmarminge, surgically applied to current main. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Merges codething/648ca884-claude branch which adds full Claude Code provider adapter, orchestration enhancements, and UI surface. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
bot
added
the
vouch:trusted
- Include Claude Code in available provider options in the picker - Add Cursor as a disabled placeholder provider with icon mapping - Update session-logic tests to assert provider availability and ordering
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
There are 5 total unresolved issues (including 2 from previous reviews).
Autofix Details
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: Map mutation during iteration in stopAll and finalizer
- Snapshot the sessions Map with Array.from(sessions) before iterating in both stopAll and the finalizer to prevent entries being skipped when stopSessionInternal deletes during iteration.
- ✅ Fixed: Custom model slugs resolve to built-in aliases unexpectedly
- Changed normalizeCustomModelSlugs to trim and check the raw slug literally against built-ins instead of resolving aliases first, so dated model versions like claude-sonnet-4-6-20251117 are no longer silently dropped.
- ✅ Fixed: Test asserts wrong model for claudeCode provider start
- Removed the incorrect model: "gpt-5-codex" assertion from the claudeCode provider selection test since it was a copy-paste artifact and the test validates provider selection, not model resolution.
Or push these changes by commenting:
@cursor push 0cebffe67a
Preview (0cebffe67a)
diff --git a/apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts b/apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts
--- a/apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts
+++ b/apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts
@@ -416,7 +416,6 @@
expect(harness.startSession.mock.calls[0]?.[1]).toMatchObject({
provider: "claudeCode",
cwd: "/tmp/provider-project",
- model: "gpt-5-codex",
runtimeMode: "approval-required",
});
diff --git a/apps/server/src/provider/Layers/ClaudeCodeAdapter.ts b/apps/server/src/provider/Layers/ClaudeCodeAdapter.ts
--- a/apps/server/src/provider/Layers/ClaudeCodeAdapter.ts
+++ b/apps/server/src/provider/Layers/ClaudeCodeAdapter.ts
@@ -1816,7 +1816,7 @@
const stopAll: ClaudeCodeAdapterShape["stopAll"] = () =>
Effect.forEach(
- sessions,
+ Array.from(sessions),
([, context]) =>
stopSessionInternal(context, {
emitExitEvent: true,
@@ -1826,7 +1826,7 @@
yield* Effect.addFinalizer(() =>
Effect.forEach(
- sessions,
+ Array.from(sessions),
([, context]) =>
stopSessionInternal(context, {
emitExitEvent: false,
diff --git a/apps/web/src/appSettings.test.ts b/apps/web/src/appSettings.test.ts
--- a/apps/web/src/appSettings.test.ts
+++ b/apps/web/src/appSettings.test.ts
@@ -8,12 +8,11 @@
} from "./appSettings";
describe("normalizeCustomModelSlugs", () => {
- it("normalizes aliases, removes built-ins, and deduplicates values", () => {
+ it("removes literal built-ins, trims whitespace, and deduplicates values", () => {
expect(
normalizeCustomModelSlugs([
" custom/internal-model ",
"gpt-5.3-codex",
- "5.3",
"custom/internal-model",
"",
null,
@@ -21,8 +20,10 @@
).toEqual(["custom/internal-model"]);
});
- it("normalizes provider-specific aliases for claude", () => {
- expect(normalizeCustomModelSlugs(["sonnet"], "claudeCode")).toEqual([]);
+ it("preserves aliases and dated versions as custom models", () => {
+ expect(normalizeCustomModelSlugs(["claude-sonnet-4-6-20251117"], "claudeCode")).toEqual([
+ "claude-sonnet-4-6-20251117",
+ ]);
expect(normalizeCustomModelSlugs(["claude/custom-sonnet"], "claudeCode")).toEqual([
"claude/custom-sonnet",
]);
diff --git a/apps/web/src/appSettings.ts b/apps/web/src/appSettings.ts
--- a/apps/web/src/appSettings.ts
+++ b/apps/web/src/appSettings.ts
@@ -57,18 +57,19 @@
const builtInModelSlugs = BUILT_IN_MODEL_SLUGS_BY_PROVIDER[provider];
for (const candidate of models) {
- const normalized = normalizeModelSlug(candidate, provider);
+ if (typeof candidate !== "string") continue;
+ const trimmed = candidate.trim();
if (
- !normalized ||
- normalized.length > MAX_CUSTOM_MODEL_LENGTH ||
- builtInModelSlugs.has(normalized) ||
- seen.has(normalized)
+ !trimmed ||
+ trimmed.length > MAX_CUSTOM_MODEL_LENGTH ||
+ builtInModelSlugs.has(trimmed) ||
+ seen.has(trimmed)
) {
continue;
}
- seen.add(normalized);
- normalizedModels.push(normalized);
+ seen.add(trimmed);
+ normalizedModels.push(trimmed);
if (normalizedModels.length >= MAX_CUSTOM_MODEL_COUNT) {
break;
}| emitExitEvent: true, | ||
| }), | ||
| { discard: true }, | ||
| ); |
There was a problem hiding this comment.
Map mutation during iteration in stopAll and finalizer
Medium Severity
stopAll and the finalizer iterate over the sessions Map using Effect.forEach, but stopSessionInternal calls sessions.delete(context.session.threadId) on line 1352 during iteration. Mutating a Map while iterating over it can cause entries to be skipped, leading to sessions not being properly cleaned up. The iteration collection needs to be snapshot (e.g., Array.from(sessions)) before iterating.
Additional Locations (1)
| return normalizedModels; | ||
| } | ||
|
|
||
| function normalizeAppSettings(settings: AppSettings): AppSettings { |
There was a problem hiding this comment.
Custom model slugs resolve to built-in aliases unexpectedly
Medium Severity
normalizeCustomModelSlugs calls normalizeModelSlug which resolves aliases (e.g., "sonnet" → "claude-sonnet-4-6"). Since the built-in slug set check then matches, user-entered aliases like "sonnet" get silently dropped instead of being stored as custom models. This is correct behavior per the test, but normalizeModelSlug for Claude also means any short alias input (like "opus") will resolve to a built-in slug and get rejected — the user can never add an alias as a custom model. However, a real custom slug like "claude-sonnet-4-6-20251117" would also be resolved to "claude-sonnet-4-6" (a built-in) and silently dropped, preventing users from pinning dated model versions.
- Map Claude reasoning deltas, streamed tool input JSON, and tool results into runtime item/content events - Classify Agent/read-only Claude tools for correct approval request types - Add Claude CLI provider health checks and auth-status parsing coverage Co-authored-by: codex <codex@users.noreply.github.com>
| normalized.includes("glob") || | ||
| normalized.includes("search") | ||
| ); | ||
| } |
There was a problem hiding this comment.
isReadOnlyToolName misclassifies web search tools as read-only
Medium Severity
isReadOnlyToolName uses normalized.includes("search") which matches tool names like "WebSearch" or "web search". Since classifyRequestType checks isReadOnlyToolName first and short-circuits, any tool containing "search" gets classified as file_read_approval instead of being routed through classifyToolItemType which would correctly identify it as web_search. This causes web search tools to get the wrong approval request type.
Additional Locations (1)
| ...(resumeState?.resumeSessionAt ? { resumeSessionAt: resumeState.resumeSessionAt } : {}), | ||
| includePartialMessages: true, | ||
| canUseTool, | ||
| env: process.env, |
There was a problem hiding this comment.
Entire process.env passed to Claude query options
Medium Severity
The Claude query options pass env: process.env directly, forwarding the server's entire environment (including secrets, API keys, and internal configuration) to the Claude Code subprocess. This could expose sensitive server-side environment variables to the Claude runtime and any tools it invokes.
| ? `Could not verify Claude Code authentication status. ${detail}` | ||
| : "Could not verify Claude Code authentication status.", | ||
| }; | ||
| } |
There was a problem hiding this comment.
parseClaudeAuthStatusFromOutput duplicates parseAuthStatusFromOutput logic entirely
Low Severity
parseClaudeAuthStatusFromOutput is a near-exact copy of parseAuthStatusFromOutput, with only minor string differences in user-facing messages. Similarly, runClaudeCommand duplicates runCodexCommand with only the binary name changed. A shared parameterized function would reduce maintenance burden and risk of inconsistent fixes.
Additional Locations (1)
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 8 total unresolved issues (including 7 from previous reviews).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Empty
content_block_stophandler does nothing with retrieved tool- The content_block_stop handler now finalizes accumulated partial JSON input, updates the tool in inFlightTools, and emits an item.updated event with the complete tool state, following the same pattern as nearby handlers.
Or push these changes by commenting:
@cursor push f5a4bbcb1c
Preview (f5a4bbcb1c)
diff --git a/apps/server/src/provider/Layers/ClaudeCodeAdapter.ts b/apps/server/src/provider/Layers/ClaudeCodeAdapter.ts
--- a/apps/server/src/provider/Layers/ClaudeCodeAdapter.ts
+++ b/apps/server/src/provider/Layers/ClaudeCodeAdapter.ts
@@ -1148,6 +1148,52 @@
if (!tool) {
return;
}
+
+ let finalTool = tool;
+ if (tool.partialInputJson.length > 0) {
+ const parsedInput = tryParseJsonRecord(tool.partialInputJson);
+ if (parsedInput) {
+ const detail = summarizeToolRequest(tool.toolName, parsedInput);
+ finalTool = {
+ ...tool,
+ input: parsedInput,
+ ...(detail ? { detail } : {}),
+ };
+ context.inFlightTools.set(index, finalTool);
+ }
+ }
+
+ const stamp = yield* makeEventStamp();
+ yield* offerRuntimeEvent({
+ type: "item.updated",
+ eventId: stamp.eventId,
+ provider: PROVIDER,
+ createdAt: stamp.createdAt,
+ threadId: context.session.threadId,
+ ...(context.turnState ? { turnId: asCanonicalTurnId(context.turnState.turnId) } : {}),
+ itemId: asRuntimeItemId(finalTool.itemId),
+ payload: {
+ itemType: finalTool.itemType,
+ status: "inProgress",
+ title: finalTool.title,
+ ...(finalTool.detail ? { detail: finalTool.detail } : {}),
+ data: {
+ toolName: finalTool.toolName,
+ input: finalTool.input,
+ },
+ },
+ providerRefs: {
+ ...providerThreadRef(context),
+ ...(context.turnState ? { providerTurnId: String(context.turnState.turnId) } : {}),
+ providerItemId: ProviderItemId.makeUnsafe(finalTool.itemId),
+ },
+ raw: {
+ source: "claude.sdk.message",
+ method: "claude/stream_event/content_block_stop",
+ payload: message,
+ },
+ });
+ return;
}
});| if (!tool) { | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Empty content_block_stop handler does nothing with retrieved tool
Medium Severity
The content_block_stop handler retrieves a tool from context.inFlightTools but does absolutely nothing with it — no event emission, no state update, no cleanup. After the if (!tool) return; guard, the code just falls through to the end of the enclosing function. This looks like an incomplete implementation where item.completed or some finalization was intended. Currently, in-flight tools are only cleaned up via handleUserMessage (on tool results) or completeTurn (flush at turn end), making this block dead code.
|
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
Or push these changes by commenting: Preview (b8b3926669)diff --git a/apps/server/src/provider/Layers/ClaudeCodeAdapter.ts b/apps/server/src/provider/Layers/ClaudeCodeAdapter.ts
--- a/apps/server/src/provider/Layers/ClaudeCodeAdapter.ts
+++ b/apps/server/src/provider/Layers/ClaudeCodeAdapter.ts
@@ -134,6 +134,19 @@
readonly nativeEventLogger?: EventNdjsonLogger;
}
+const CLAUDE_ENV_PREFIX_BLOCKLIST = ["T3CODE_", "VITE_"];
+
+function createClaudeSpawnEnv(baseEnv: NodeJS.ProcessEnv): NodeJS.ProcessEnv {
+ const filtered: NodeJS.ProcessEnv = {};
+ for (const [key, value] of Object.entries(baseEnv)) {
+ if (value === undefined) continue;
+ const upper = key.toUpperCase();
+ if (CLAUDE_ENV_PREFIX_BLOCKLIST.some((prefix) => upper.startsWith(prefix))) continue;
+ filtered[key] = value;
+ }
+ return filtered;
+}
+
function isUuid(value: string): boolean {
return /^[0-9a-f]{8}-[0-9a-f]{4}-[1-8][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i.test(value);
}
@@ -256,7 +269,7 @@
normalized.includes("view") ||
normalized.includes("grep") ||
normalized.includes("glob") ||
- normalized.includes("search")
+ (normalized.includes("search") && !normalized.includes("web"))
);
}
@@ -1917,7 +1930,7 @@
...(resumeState?.resumeSessionAt ? { resumeSessionAt: resumeState.resumeSessionAt } : {}),
includePartialMessages: true,
canUseTool,
- env: process.env,
+ env: createClaudeSpawnEnv(process.env),
...(input.cwd ? { additionalDirectories: [input.cwd] } : {}),
};
diff --git a/apps/server/src/provider/Layers/ProviderHealth.ts b/apps/server/src/provider/Layers/ProviderHealth.ts
--- a/apps/server/src/provider/Layers/ProviderHealth.ts
+++ b/apps/server/src/provider/Layers/ProviderHealth.ts
@@ -84,7 +84,39 @@
return undefined;
}
-export function parseAuthStatusFromOutput(result: CommandResult): {
+interface ProviderAuthLabels {
+ readonly unknownCommandMessage: string;
+ readonly unauthenticatedMessage: string;
+ readonly jsonParseWarning: string;
+ readonly verifyFailurePrefix: string;
+ readonly loginHints: ReadonlyArray<string>;
+}
+
+const CODEX_AUTH_LABELS: ProviderAuthLabels = {
+ unknownCommandMessage:
+ "Codex CLI authentication status command is unavailable in this Codex version.",
+ unauthenticatedMessage: "Codex CLI is not authenticated. Run `codex login` and try again.",
+ jsonParseWarning:
+ "Could not verify Codex authentication status from JSON output (missing auth marker).",
+ verifyFailurePrefix: "Could not verify Codex authentication status",
+ loginHints: ["run `codex login`", "run codex login"],
+};
+
+const CLAUDE_AUTH_LABELS: ProviderAuthLabels = {
+ unknownCommandMessage:
+ "Claude Code authentication status command is unavailable in this version of Claude Code.",
+ unauthenticatedMessage:
+ "Claude Code is not authenticated. Run `claude auth login` and try again.",
+ jsonParseWarning:
+ "Could not verify Claude Code authentication status from JSON output (missing auth marker).",
+ verifyFailurePrefix: "Could not verify Claude Code authentication status",
+ loginHints: ["run `claude login`", "run claude login"],
+};
+
+function parseAuthStatusWithLabels(
+ result: CommandResult,
+ labels: ProviderAuthLabels,
+): {
readonly status: ServerProviderStatusState;
readonly authStatus: ServerProviderAuthStatus;
readonly message?: string;
@@ -99,7 +131,7 @@
return {
status: "warning",
authStatus: "unknown",
- message: "Codex CLI authentication status command is unavailable in this Codex version.",
+ message: labels.unknownCommandMessage,
};
}
@@ -107,13 +139,12 @@
lowerOutput.includes("not logged in") ||
lowerOutput.includes("login required") ||
lowerOutput.includes("authentication required") ||
- lowerOutput.includes("run `codex login`") ||
- lowerOutput.includes("run codex login")
+ labels.loginHints.some((hint) => lowerOutput.includes(hint))
) {
return {
status: "error",
authStatus: "unauthenticated",
- message: "Codex CLI is not authenticated. Run `codex login` and try again.",
+ message: labels.unauthenticatedMessage,
};
}
@@ -139,15 +170,14 @@
return {
status: "error",
authStatus: "unauthenticated",
- message: "Codex CLI is not authenticated. Run `codex login` and try again.",
+ message: labels.unauthenticatedMessage,
};
}
if (parsedAuth.attemptedJsonParse) {
return {
status: "warning",
authStatus: "unknown",
- message:
- "Could not verify Codex authentication status from JSON output (missing auth marker).",
+ message: labels.jsonParseWarning,
};
}
if (result.code === 0) {
@@ -158,12 +188,14 @@
return {
status: "warning",
authStatus: "unknown",
- message: detail
- ? `Could not verify Codex authentication status. ${detail}`
- : "Could not verify Codex authentication status.",
+ message: detail ? `${labels.verifyFailurePrefix}. ${detail}` : `${labels.verifyFailurePrefix}.`,
};
}
+export function parseAuthStatusFromOutput(result: CommandResult) {
+ return parseAuthStatusWithLabels(result, CODEX_AUTH_LABELS);
+}
+
// ── Codex CLI config detection ──────────────────────────────────────
/**
@@ -239,10 +271,10 @@
(acc, chunk) => acc + new TextDecoder().decode(chunk),
);
-const runCodexCommand = (args: ReadonlyArray<string>) =>
+const runCliCommand = (binary: string, args: ReadonlyArray<string>) =>
Effect.gen(function* () {
const spawner = yield* ChildProcessSpawner.ChildProcessSpawner;
- const command = ChildProcess.make("codex", [...args], {
+ const command = ChildProcess.make(binary, [...args], {
shell: process.platform === "win32",
});
@@ -260,27 +292,10 @@
return { stdout, stderr, code: exitCode } satisfies CommandResult;
}).pipe(Effect.scoped);
-const runClaudeCommand = (args: ReadonlyArray<string>) =>
- Effect.gen(function* () {
- const spawner = yield* ChildProcessSpawner.ChildProcessSpawner;
- const command = ChildProcess.make("claude", [...args], {
- shell: process.platform === "win32",
- });
+const runCodexCommand = (args: ReadonlyArray<string>) => runCliCommand("codex", args);
- const child = yield* spawner.spawn(command);
+const runClaudeCommand = (args: ReadonlyArray<string>) => runCliCommand("claude", args);
- const [stdout, stderr, exitCode] = yield* Effect.all(
- [
- collectStreamAsString(child.stdout),
- collectStreamAsString(child.stderr),
- child.exitCode.pipe(Effect.map(Number)),
- ],
- { concurrency: "unbounded" },
- );
-
- return { stdout, stderr, code: exitCode } satisfies CommandResult;
- }).pipe(Effect.scoped);
-
// ── Health check ────────────────────────────────────────────────────
export const checkCodexProviderStatus: Effect.Effect<
@@ -409,86 +424,8 @@
// ── Claude Code health check ────────────────────────────────────────
-export function parseClaudeAuthStatusFromOutput(result: CommandResult): {
- readonly status: ServerProviderStatusState;
- readonly authStatus: ServerProviderAuthStatus;
- readonly message?: string;
-} {
- const lowerOutput = `${result.stdout}\n${result.stderr}`.toLowerCase();
-
- if (
- lowerOutput.includes("unknown command") ||
- lowerOutput.includes("unrecognized command") ||
- lowerOutput.includes("unexpected argument")
- ) {
- return {
- status: "warning",
- authStatus: "unknown",
- message:
- "Claude Code authentication status command is unavailable in this version of Claude Code.",
- };
- }
-
- if (
- lowerOutput.includes("not logged in") ||
- lowerOutput.includes("login required") ||
- lowerOutput.includes("authentication required") ||
- lowerOutput.includes("run `claude login`") ||
- lowerOutput.includes("run claude login")
- ) {
- return {
- status: "error",
- authStatus: "unauthenticated",
- message: "Claude Code is not authenticated. Run `claude auth login` and try again.",
- };
- }
-
- // `claude auth status` returns JSON with a `loggedIn` boolean.
- const parsedAuth = (() => {
- const trimmed = result.stdout.trim();
- if (!trimmed || (!trimmed.startsWith("{") && !trimmed.startsWith("["))) {
- return { attemptedJsonParse: false as const, auth: undefined as boolean | undefined };
- }
- try {
- return {
- attemptedJsonParse: true as const,
- auth: extractAuthBoolean(JSON.parse(trimmed)),
- };
- } catch {
- return { attemptedJsonParse: false as const, auth: undefined as boolean | undefined };
- }
- })();
-
- if (parsedAuth.auth === true) {
- return { status: "ready", authStatus: "authenticated" };
- }
- if (parsedAuth.auth === false) {
- return {
- status: "error",
- authStatus: "unauthenticated",
- message: "Claude Code is not authenticated. Run `claude auth login` and try again.",
- };
- }
- if (parsedAuth.attemptedJsonParse) {
- return {
- status: "warning",
- authStatus: "unknown",
- message:
- "Could not verify Claude Code authentication status from JSON output (missing auth marker).",
- };
- }
- if (result.code === 0) {
- return { status: "ready", authStatus: "authenticated" };
- }
-
- const detail = detailFromResult(result);
- return {
- status: "warning",
- authStatus: "unknown",
- message: detail
- ? `Could not verify Claude Code authentication status. ${detail}`
- : "Could not verify Claude Code authentication status.",
- };
+export function parseClaudeAuthStatusFromOutput(result: CommandResult) {
+ return parseAuthStatusWithLabels(result, CLAUDE_AUTH_LABELS);
}
export const checkClaudeCodeProviderStatus: Effect.Effect< |
|
@juliusmarminge i can strip out and put it in a sperate branch. give me a sec |
|
i did a lot of work making ti compatible with all the reverse engineered docs that are available. |
|
@juliusmarminge So I filled PR #1146 targeting this branch, excluding the Teams feature which is now in a seperate branch #1145 in drafts targeting main. Will update and fix the teams feature tmr. |
| if (!tool) { | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Dead code in tool content_block_stop handler
Low Severity
In handleStreamEvent, when a content_block_stop event fires for a tool-use block (not an assistant text block), the code looks up the tool via context.inFlightTools.get(index) but does absolutely nothing with the result. The if (!tool) { return; } guard exits early if the tool is missing, but when the tool IS found, execution falls through to the end of the function with no tool-specific action taken. This dead branch is confusing and suggests missing logic for handling tool stream closure.
| return `${toolName}: ${command.trim().slice(0, 400)}`; | ||
| } | ||
|
|
||
| const serialized = JSON.stringify(input); |
There was a problem hiding this comment.
🟢 Low Layers/ClaudeAdapter.ts:298
JSON.stringify(input) throws when input contains circular references or BigInt values, causing summarizeToolRequest to propagate the exception to callers. Unlike toolInputFingerprint which wraps stringification in a try-catch to handle this case, this function has no error handling and will crash.
let serialized: string;
try {
serialized = JSON.stringify(input);
} catch {
serialized = "[unserializable input]";
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/Layers/ClaudeAdapter.ts around line 298:
`JSON.stringify(input)` throws when `input` contains circular references or `BigInt` values, causing `summarizeToolRequest` to propagate the exception to callers. Unlike `toolInputFingerprint` which wraps stringification in a try-catch to handle this case, this function has no error handling and will crash.
Evidence trail:
apps/server/src/provider/Layers/ClaudeAdapter.ts lines 291-302 (summarizeToolRequest with unprotected JSON.stringify at line 298), apps/server/src/provider/Layers/ClaudeAdapter.ts lines 452-458 (toolInputFingerprint with try-catch around JSON.stringify), apps/server/src/provider/Layers/ClaudeAdapter.ts lines 1316-1318 (both functions called with the same toolInput)
| yield* Queue.offer(context.promptQueue, { | ||
| type: "message", | ||
| message, | ||
| }).pipe(Effect.mapError((cause) => toRequestError(input.threadId, "turn/start", cause))); |
There was a problem hiding this comment.
🟡 Medium Layers/ClaudeAdapter.ts:2409
Queue.offer returns Effect<boolean> where false means the queue was shutdown. The current code ignores this boolean and only maps the error channel, so when the session is stopping and the queue is shutdown, the message is silently dropped but sendTurn still returns a successful ProviderTurnStartResult. The caller believes the turn started when the message was never queued. Consider checking the boolean result and failing with an appropriate error when the queue is shutdown.
- yield* Queue.offer(context.promptQueue, {
- type: "message",
- message,
- }).pipe(Effect.mapError((cause) => toRequestError(input.threadId, "turn/start", cause)));
+ const offered = yield* Queue.offer(context.promptQueue, {
+ type: "message",
+ message,
+ });
+ if (!offered) {
+ return yield* new ProviderAdapterRequestError({
+ provider: PROVIDER,
+ method: "turn/start",
+ detail: "Failed to queue message: prompt queue is shutdown.",
+ });
+ }🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/Layers/ClaudeAdapter.ts around lines 2409-2412:
`Queue.offer` returns `Effect<boolean>` where `false` means the queue was shutdown. The current code ignores this boolean and only maps the error channel, so when the session is stopping and the queue is shutdown, the message is silently dropped but `sendTurn` still returns a successful `ProviderTurnStartResult`. The caller believes the turn started when the message was never queued. Consider checking the boolean result and failing with an appropriate error when the queue is shutdown.
Evidence trail:
- Reviewed code: `apps/server/src/provider/Layers/ClaudeAdapter.ts` lines 2409-2420 (REVIEWED_COMMIT)
- Same codebase pattern for handling Queue.offer boolean: `packages/shared/src/DrainableWorker.ts` lines 90-93 - shows `const accepted = yield* Queue.offer(queue, item); if (!accepted) { ... }`
- Effect-TS Queue.offer behavior is confirmed by the codebase's own usage pattern which captures and checks the boolean return value
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 7 total unresolved issues (including 6 from previous reviews).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Approval events use orchestration ID as provider thread ID
- Replaced four direct uses of
context.session.threadIdasproviderThreadIdwith theproviderThreadRef(context)helper, consistent with all other event emissions in the file.
- Replaced four direct uses of
Or push these changes by commenting:
@cursor push 241180f045
Preview (241180f045)
diff --git a/apps/server/src/provider/Layers/ClaudeAdapter.ts b/apps/server/src/provider/Layers/ClaudeAdapter.ts
--- a/apps/server/src/provider/Layers/ClaudeAdapter.ts
+++ b/apps/server/src/provider/Layers/ClaudeAdapter.ts
@@ -1995,7 +1995,7 @@
requestId: asRuntimeRequestId(requestId),
payload: { questions },
providerRefs: {
- ...(context.session.threadId ? { providerThreadId: context.session.threadId } : {}),
+ ...providerThreadRef(context),
...(context.turnState ? { providerTurnId: String(context.turnState.turnId) } : {}),
providerRequestId: requestId,
},
@@ -2034,7 +2034,7 @@
requestId: asRuntimeRequestId(requestId),
payload: { answers },
providerRefs: {
- ...(context.session.threadId ? { providerThreadId: context.session.threadId } : {}),
+ ...providerThreadRef(context),
...(context.turnState ? { providerTurnId: String(context.turnState.turnId) } : {}),
providerRequestId: requestId,
},
@@ -2116,9 +2116,7 @@
},
},
providerRefs: {
- ...(context.session.threadId
- ? { providerThreadId: context.session.threadId }
- : {}),
+ ...providerThreadRef(context),
...(context.turnState
? { providerTurnId: String(context.turnState.turnId) }
: {}),
@@ -2167,9 +2165,7 @@
decision,
},
providerRefs: {
- ...(context.session.threadId
- ? { providerThreadId: context.session.threadId }
- : {}),
+ ...providerThreadRef(context),
...(context.turnState
? { providerTurnId: String(context.turnState.turnId) }
: {}),
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 7 total unresolved issues (including 6 from previous reviews).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Codex model name passed to Claude on provider switch
- When a provider is explicitly requested and differs from the current provider (or no session exists), the code now falls back to DEFAULT_MODEL_BY_PROVIDER for the target provider instead of carrying over the thread's existing model slug.
Or push these changes by commenting:
@cursor push 5b9f3fb2e9
Preview (5b9f3fb2e9)
diff --git a/apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts b/apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts
--- a/apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts
+++ b/apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts
@@ -416,7 +416,7 @@
expect(harness.startSession.mock.calls[0]?.[1]).toMatchObject({
provider: "claudeAgent",
cwd: "/tmp/provider-project",
- model: "gpt-5-codex",
+ model: "claude-sonnet-4-6",
runtimeMode: "approval-required",
});
diff --git a/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts b/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
--- a/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
+++ b/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
@@ -1,6 +1,7 @@
import {
type ChatAttachment,
CommandId,
+ DEFAULT_MODEL_BY_PROVIDER,
EventId,
type OrchestrationEvent,
type ProviderModelOptions,
@@ -211,7 +212,11 @@
? thread.session.providerName
: undefined;
const preferredProvider: ProviderKind | undefined = options?.provider ?? currentProvider;
- const desiredModel = options?.model ?? thread.model;
+ const providerIsChanging =
+ options?.provider !== undefined && options.provider !== currentProvider;
+ const desiredModel =
+ options?.model ??
+ (providerIsChanging ? DEFAULT_MODEL_BY_PROVIDER[options.provider!] : thread.model);
const effectiveCwd = resolveThreadWorkspaceCwd({
thread,
projects: readModel.projects,| thread.session?.providerName, | ||
| ) | ||
| ? thread.session.providerName | ||
| : undefined; |
There was a problem hiding this comment.
Codex model name passed to Claude on provider switch
High Severity
When a turn requests provider: "claudeAgent" without explicitly specifying a model, ensureSessionForThread resolves desiredModel via options?.model ?? thread.model, which falls through to the thread's existing Codex model (e.g. "gpt-5-codex"). This Codex model slug is then passed to the Claude adapter's startSession, which forwards it to the Claude SDK. The Claude SDK doesn't understand Codex model names and will fail or behave unpredictably. The unit test at line 419 confirms this — it asserts model: "gpt-5-codex" is sent when provider: "claudeAgent" is requested.
Additional Locations (1)
|
|
||
| context.stopped = true; | ||
|
|
||
| for (const [requestId, pending] of context.pendingApprovals) { |
There was a problem hiding this comment.
🟡 Medium Layers/ClaudeAdapter.ts:1795
In stopSessionInternal, pendingApprovals are resolved and cleaned up (lines 1795-1813), but pendingUserInputs is not handled. If a session is stopped while handleAskUserQuestion is waiting for user input, and the SDK's abort signal does not fire, the Deferred.await(answersDeferred) at line 1969 will never resolve, causing the fiber inside Effect.runPromise (line 2005) to hang indefinitely.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/Layers/ClaudeAdapter.ts around line 1795:
In `stopSessionInternal`, `pendingApprovals` are resolved and cleaned up (lines 1795-1813), but `pendingUserInputs` is not handled. If a session is stopped while `handleAskUserQuestion` is waiting for user input, and the SDK's abort signal does not fire, the `Deferred.await(answersDeferred)` at line 1969 will never resolve, causing the fiber inside `Effect.runPromise` (line 2005) to hang indefinitely.
Evidence trail:
apps/server/src/provider/Layers/ClaudeAdapter.ts lines 1785-1848 (`stopSessionInternal` function showing `pendingApprovals` cleanup at 1794-1813 but no `pendingUserInputs` handling); lines 120-130 (context type definition showing both `pendingApprovals` and `pendingUserInputs`); lines 1920-1975 (`handleAskUserQuestion` showing `Deferred.await(answersDeferred)` at line 1969 and abort signal handler at lines 1959-1966)
- Reject turn starts that request a provider switch from the thread-bound provider - Reject models that do not belong to the thread provider and log start failures - Add shared `inferProviderForModel` helper and tests; reuse it in web store inference
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 6 total unresolved issues (including 5 from previous reviews).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: preferredProvider fallback chain contains dead code path
- Removed the dead
options?.provideroperand from the nullish coalescing chain, simplifying it tocurrentProvider ?? threadProviderwhich is behaviorally identical.
- Removed the dead
Or push these changes by commenting:
@cursor push a98a939d9b
Preview (a98a939d9b)
diff --git a/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts b/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
--- a/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
+++ b/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
@@ -229,7 +229,7 @@
detail: `Model '${options.model}' does not belong to provider '${threadProvider}' for thread '${threadId}'.`,
});
}
- const preferredProvider: ProviderKind = currentProvider ?? options?.provider ?? threadProvider;
+ const preferredProvider: ProviderKind = currentProvider ?? threadProvider;
const desiredModel = options?.model ?? thread.model;
const effectiveCwd = resolveThreadWorkspaceCwd({
thread,- Use thread provider as the preferred provider during command handling - Update `ProviderModelPicker` to show direct model choices when provider is locked - Add browser tests for locked/unlocked picker behavior and widen browser test glob
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 6 total unresolved issues (including 5 from previous reviews).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Turn start error handler catches interrupts as failures
- Added a Cause.hasInterruptsOnly check in the inner catchCause handler to re-propagate interrupt causes instead of converting them into spurious provider.turn.start.failed activities.
Or push these changes by commenting:
@cursor push ae48a85907
Preview (ae48a85907)
diff --git a/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts b/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
--- a/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
+++ b/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
@@ -498,16 +498,19 @@
interactionMode: event.payload.interactionMode,
createdAt: event.payload.createdAt,
}).pipe(
- Effect.catchCause((cause) =>
- appendProviderFailureActivity({
+ Effect.catchCause((cause) => {
+ if (Cause.hasInterruptsOnly(cause)) {
+ return Effect.failCause(cause);
+ }
+ return appendProviderFailureActivity({
threadId: event.payload.threadId,
kind: "provider.turn.start.failed",
summary: "Provider turn start failed",
detail: Cause.pretty(cause),
turnId: null,
createdAt: event.payload.createdAt,
- }),
- ),
+ });
+ }),
);
});| createdAt: event.payload.createdAt, | ||
| }), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Turn start error handler catches interrupts as failures
Low Severity
The new Effect.catchCause wrapper around sendTurnForThread catches all causes including interrupts. When a fiber is interrupted (e.g., during shutdown), this creates a spurious provider.turn.start.failed activity instead of allowing the interrupt to propagate. The outer processDomainEventSafely handler already filters interrupts via Cause.hasInterruptsOnly, but this inner handler runs first and swallows them.
- derive `defaultModel` from `DEFAULT_MODEL_BY_PROVIDER` using harness provider - replace hardcoded `gpt-5-codex` in seeded project and thread setup
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 6 total unresolved issues (including 5 from previous reviews).
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Redundant variable always equals existing value
- Removed the redundant
preferredProvidervariable and replaced its two usages withthreadProvider, which is always equal.
- Removed the redundant
Or push these changes by commenting:
@cursor push 7b50561688
Preview (7b50561688)
diff --git a/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts b/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
--- a/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
+++ b/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
@@ -229,7 +229,6 @@
detail: `Model '${options.model}' does not belong to provider '${threadProvider}' for thread '${threadId}'.`,
});
}
- const preferredProvider: ProviderKind = currentProvider ?? threadProvider;
const desiredModel = options?.model ?? thread.model;
const effectiveCwd = resolveThreadWorkspaceCwd({
thread,
@@ -247,8 +246,8 @@
}) =>
providerService.startSession(threadId, {
threadId,
- ...((input?.provider ?? preferredProvider)
- ? { provider: input?.provider ?? preferredProvider }
+ ...((input?.provider ?? threadProvider)
+ ? { provider: input?.provider ?? threadProvider }
: {}),
...(effectiveCwd ? { cwd: effectiveCwd } : {}),
...(desiredModel ? { model: desiredModel } : {}),| detail: `Model '${options.model}' does not belong to provider '${threadProvider}' for thread '${threadId}'.`, | ||
| }); | ||
| } | ||
| const preferredProvider: ProviderKind = currentProvider ?? threadProvider; |
There was a problem hiding this comment.
Redundant variable always equals existing value
Low Severity
preferredProvider on line 232 is always equal to threadProvider. Since threadProvider is defined as currentProvider ?? inferProviderForModel(thread.model), the expression currentProvider ?? threadProvider collapses: when currentProvider is defined they're the same, and when it's undefined both resolve to inferProviderForModel(thread.model). The extra variable adds confusion without any distinct semantic.
Additional Locations (1)
11 participants
Summary
This PR adds the Claude Code adapter on top of the core orchestration branch in #103.
It includes:
Stack
codething/648ca884Validation
bun lintbun typecheckcd apps/server && bun run test -- --run src/provider/Layers/ProviderAdapterRegistry.test.tscd apps/web && bun run test -- --run src/session-logic.test.tsNote
Medium Risk
Adds a new Claude provider backed by an external SDK and threads it through orchestration session lifecycle (start/stream/approvals/rollback), plus stricter provider/model validation that can block turn starts if misconfigured.
Overview
Adds Claude (
claudeAgent) as a first-class provider: introduces a new server-side Claude adapter (backed by@anthropic-ai/claude-agent-sdk) that starts sessions, streams SDK messages as canonicalProviderRuntimeEvents, supports interrupts, approvals and user-input prompts, resume cursors, and provider-side rollback semantics.Updates orchestration to be provider-aware on turn start and to enforce invariants: threads infer/bind a provider from their model, explicit provider requests cannot switch once bound, and model slugs must belong to the thread’s provider; failures now emit
provider.turn.start.failedactivities.Extends/adjusts tests and harnesses to run against multiple providers (Codex + Claude), adds new claude-focused integration/unit tests for session start, recovery after
stopAll, approvals, interrupts, checkpoint capture, and checkpoint revert/rollback, and refreshes docs/plan plus server deps.Written by Cursor Bugbot for commit b04132d. This will update automatically on new commits. Configure here.
Note
Add Claude Code adapter to support
claudeAgentas a provider alongside CodexClaudeAdapterLiveimplementing the full provider adapter contract (startSession,sendTurn,stopAll, etc.) backed by@anthropic-ai/claude-agent-sdk.ProviderAdapterRegistryandserverLayerssoclaudeAgentsessions are routed correctly.ProviderKind,ProviderStartOptions,ProviderModelOptions) inpackages/contractsto includeclaudeAgentwith modelsclaude-opus-4-6,claude-sonnet-4-6, andclaude-haiku-4-5.inferProviderForModelutility and updates model resolution, alias normalization, and reasoning effort lookups inpackages/shared/src/model.tsto be provider-scoped.ProviderModelPickerUI to show models directly when a provider is locked and adds Claude entries to settings, app settings, and draft store normalization.ProviderHealth.ts, probingclaude --versionandclaude auth statusalongside the existing Codex check.ProviderCommandReactornow rejects turn starts when the requested provider or model conflicts with the thread's bound provider, which is a new failure mode for existing clients that pass mismatched provider/model combinations.Macroscope summarized b04132d.