Add ACP provider support and session management#30
Merged
Conversation
- Add ACP runtime plumbing for Claude, Cursor, and OpenCode - Update model picker, provider registry, and server settings - Improve Claude usage normalization and session replacement handling
github-actions
Bot
added
size:XXL
vouch:trusted
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds multi-provider support (Cursor, OpenCode), a full ACP session/runtime stack and Codex app-server runtimes, provider registry persistence and reaper, extensive provider adapters/providers, native ACP logging, CLI/version utilities, numerous tests, and package/workspace dependency additions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant PS as ProviderService
participant Adapter as ProviderAdapter
participant Runtime as SessionRuntime
participant Backend as Backend/CLI/SDK
Client->>PS: startSession(threadId, provider)
PS->>PS: stopStaleSessionsForThread(threadId)
PS->>Adapter: startSession(...)
Adapter->>Runtime: create scoped session
Runtime->>Backend: spawn/initialize
Backend-->>Runtime: session started
Runtime->>Runtime: start event consumer
Runtime-->>Adapter: emit session.started
Adapter-->>PS: persist binding + emit events
PS-->>Client: session active
Client->>PS: sendTurn(threadId, prompt)
PS->>Adapter: sendTurn(...)
Adapter->>Runtime: prompt request
Runtime->>Backend: send prompt
Backend-->>Runtime: stream events
Runtime->>Runtime: parse & emit runtime events
Runtime-->>Adapter: content.delta, turn.completed
Adapter-->>PS: canonical events
PS-->>Client: thread updated
sequenceDiagram
participant Reaper as SessionReaper
participant Dir as SessionDirectory
participant Engine as OrchestrationEngine
participant Service as ProviderService
Reaper->>Reaper: scheduled sweep
Reaper->>Engine: load read model
Engine-->>Reaper: thread/session mapping
Reaper->>Dir: listBindings()
Dir-->>Reaper: bindings (with lastSeenAt)
loop each binding
Reaper->>Reaper: compute idle > threshold?
alt idle exceeded & no activeTurn
Reaper->>Service: stopSession(threadId)
Service-->>Reaper: success / error
else skip
end
end
Reaper->>Reaper: log sweep complete
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (8)
apps/server/src/provider/cliVersion.test.ts (1)
14-15: Rename this test to match actual behavior.Line 14 says “rejects malformed numeric segments”, but Line 15 validates fallback ordering (
> 0) rather than rejection/throwing. Consider renaming for precision.Proposed wording tweak
- it("rejects malformed numeric segments", () => { + it("falls back to lexical ordering for malformed numeric segments", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/cliVersion.test.ts` around lines 14 - 15, Rename the test description string to reflect actual behavior: update the it("rejects malformed numeric segments", ...) to something like it("falls back to ordering for malformed numeric segments", ...) so the test accurately documents that compareCliVersions("1.2.3abc", "1.2.10") > 0 validates fallback ordering rather than expecting a rejection/throw; keep the assertion and function name compareCliVersions unchanged.apps/server/src/provider/acp/AcpAdapterSupport.ts (1)
28-41: Consider consolidating redundant error mapping branches.The
AcpRequestErrorcase (lines 28-35) and the default fallback (lines 36-41) produce identicalProviderAdapterRequestErrorwith the same structure. Unless you anticipate handlingAcpRequestErrordifferently in the future, you can simplify by removing the explicit check.♻️ Suggested simplification
- if (Schema.is(EffectAcpErrors.AcpRequestError)(error)) { - return new ProviderAdapterRequestError({ - provider, - method, - detail: error.message, - cause: error, - }); - } return new ProviderAdapterRequestError({ provider, method, detail: error.message, cause: error, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/acp/AcpAdapterSupport.ts` around lines 28 - 41, The two branches that check Schema.is(EffectAcpErrors.AcpRequestError)(error) and the default both construct the same ProviderAdapterRequestError; remove the redundant conditional and always return the unified ProviderAdapterRequestError({ provider, method, detail: error.message, cause: error }) so the mapping is consolidated into a single return path (update the code in AcpAdapterSupport where Schema.is(EffectAcpErrors.AcpRequestError) is used and ensure ProviderAdapterRequestError remains constructed the same).apps/server/src/provider/acp/AcpAdapterSupport.test.ts (1)
13-26: Missing test coverage forAcpProcessExitedError→ProviderAdapterSessionClosedErrormapping.The
mapAcpToAdapterErrorfunction has distinct handling forAcpProcessExitedErrorthat producesProviderAdapterSessionClosedError, but this branch is not tested. Consider adding a test case to ensure this mapping works correctly.💚 Suggested additional test case
it("maps ACP process exited errors to provider adapter session closed errors", () => { const error = mapAcpToAdapterError( "cursor", "thread-1" as never, "session/prompt", new EffectAcpErrors.AcpProcessExitedError({ code: 1, }), ); expect(error._tag).toBe("ProviderAdapterSessionClosedError"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/acp/AcpAdapterSupport.test.ts` around lines 13 - 26, Add a unit test that verifies mapAcpToAdapterError maps an AcpProcessExitedError to a ProviderAdapterSessionClosedError: in AcpAdapterSupport.test.ts create a test (e.g., "maps ACP process exited errors to provider adapter session closed errors") that calls mapAcpToAdapterError with the same arguments style as the existing test but passing new EffectAcpErrors.AcpProcessExitedError({ code: 1 }) and assert that the returned error._tag equals "ProviderAdapterSessionClosedError"; ensure you import/use EffectAcpErrors and mirror the structure of the existing "maps ACP request errors..." test.apps/server/src/provider/Layers/CodexSessionRuntime.ts (2)
1173-1188: Resource cleanup order inclosemay cause issues.The scope is closed before shutting down the queues. However,
Scope.closemay interrupt fibers that are still trying to offer events to the queues. Consider shutting down queues first to prevent any race conditions where event offers fail after scope close.Suggested reordering
const close = Effect.gen(function* () { const alreadyClosed = yield* Ref.getAndSet(closedRef, true); if (alreadyClosed) { return; } yield* settlePendingApprovals("cancel"); yield* settlePendingUserInputs({}); yield* updateSession(sessionRef, { status: "closed", activeTurnId: undefined, }); yield* emitSessionEvent("session/closed", "Session stopped"); + yield* Queue.shutdown(serverNotifications); + yield* Queue.shutdown(events); yield* Scope.close(runtimeScope, Exit.void); - yield* Queue.shutdown(serverNotifications); - yield* Queue.shutdown(events); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/CodexSessionRuntime.ts` around lines 1173 - 1188, The close function currently closes runtimeScope before shutting down the queues, which can cause fibers to be interrupted while offering to serverNotifications/events; update close (the Effect.gen function named close) to call Queue.shutdown(serverNotifications) and Queue.shutdown(events) before calling Scope.close(runtimeScope, Exit.void), keeping the existing settlePendingApprovals, settlePendingUserInputs, updateSession, and emitSessionEvent calls as appropriate so that queues are drained/closed prior to closing the scope to avoid race conditions.
59-65: TODO: Track upstream schema fix.This TODO indicates a schema generation issue in
effect-codex-app-server. Consider creating an issue to track this technical debt so it doesn't get lost.Would you like me to open a GitHub issue to track the
V2TurnStartParamsschema fix needed inpackages/effect-codex-app-server/scripts/generate.ts?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/CodexSessionRuntime.ts` around lines 59 - 65, Add a tracked GitHub issue for the upstream schema generation bug where EffectCodexSchema.V2TurnStartParams does not include collaborationMode (currently patched locally by CodexTurnStartParamsWithCollaborationMode using Schema.fieldsAssign and V2TurnStartParams__CollaborationMode); in the issue describe the expected schema change (V2TurnStartParams should directly include collaborationMode as optional), point to packages/effect-codex-app-server/scripts/generate.ts as the generator to modify, and note this file (CodexSessionRuntime.ts) can remove the manual fieldsAssign workaround once the generator is fixed so we can delete CodexTurnStartParamsWithCollaborationMode and rely on EffectCodexSchema.V2TurnStartParams.apps/server/src/provider/acp/AcpSessionRuntime.ts (1)
581-584: Non-null assertion onparsed.modeIdafter truthiness check.The code checks
if (parsed.modeId)then usesparsed.modeId!. TypeScript's control flow analysis should narrow this, but the!assertion is redundant given the guard.Remove unnecessary assertion
const parsed = parseSessionUpdateEvent(params); if (parsed.modeId) { + const modeId = parsed.modeId; yield* Ref.update(modeStateRef, (current) => - current === undefined ? current : updateModeState(current, parsed.modeId!), + current === undefined ? current : updateModeState(current, modeId), ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/acp/AcpSessionRuntime.ts` around lines 581 - 584, The non-null assertion on parsed.modeId is redundant because you already guarded with if (parsed.modeId); remove the exclamation and pass parsed.modeId directly to updateModeState inside the Ref.update call (i.e., in the branch that updates modeStateRef call Ref.update(..., current === undefined ? current : updateModeState(current, parsed.modeId))). This affects the parsed.modeId usage in the Ref.update invocation and keeps TypeScript happy without the unnecessary `!`.apps/server/src/provider/Layers/OpenCodeAdapter.ts (1)
392-414: Mutable session update pattern uses unsafe cast.The
updateProviderSessionfunction casts toRecord<string, unknown>to delete properties, which bypasses TypeScript's type safety. While the pattern works, it's fragile ifProviderSessiontype changes.Consider using explicit property handling
function updateProviderSession( context: OpenCodeSessionContext, patch: Partial<ProviderSession>, options?: { readonly clearActiveTurnId?: boolean; readonly clearLastError?: boolean; }, ): ProviderSession { - const nextSession = { - ...context.session, - ...patch, - updatedAt: nowIso(), - } as ProviderSession & Record<string, unknown>; - const mutableSession = nextSession as Record<string, unknown>; - if (options?.clearActiveTurnId) { - delete mutableSession.activeTurnId; - } - if (options?.clearLastError) { - delete mutableSession.lastError; - } - context.session = nextSession; - return nextSession; + const { activeTurnId, lastError, ...rest } = context.session; + const nextSession: ProviderSession = { + ...rest, + ...patch, + updatedAt: nowIso(), + ...(options?.clearActiveTurnId ? {} : activeTurnId !== undefined ? { activeTurnId } : {}), + ...(options?.clearLastError ? {} : lastError !== undefined ? { lastError } : {}), + }; + context.session = nextSession; + return nextSession; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/OpenCodeAdapter.ts` around lines 392 - 414, The function updateProviderSession uses an unsafe cast to Record<string, unknown> and deletes fields to clear properties; instead build a properly typed ProviderSession object without mutating via cast: create nextSession as a ProviderSession (spread context.session and patch, set updatedAt), then if options.clearActiveTurnId or options.clearLastError, construct a new object that omits those keys (e.g., by destructuring to separate activeTurnId/lastError and reassembling only the remaining fields) or use a typed helper that returns Omit<ProviderSession, 'activeTurnId'|'lastError'> before assigning to context.session; ensure the returned value is typed ProviderSession and remove any Record<string, unknown> casts (references: updateProviderSession, ProviderSession, OpenCodeSessionContext, options.clearActiveTurnId, options.clearLastError).apps/server/src/provider/Layers/CursorProvider.ts (1)
583-586: Regex from variable - limited risk but worth noting.The static analysis flagged this regex constructed from the
keyparameter. While this function is internal andkeycomes from controlled sources (hardcoded strings like"CLI Version","User Email"), the pattern could theoretically be vulnerable ifkeycontained regex metacharacters.Consider escaping the key for safety
+function escapeRegExp(str: string): string { + return str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} + function extractAboutField(plain: string, key: string): string | undefined { - const regex = new RegExp(`^${key}\\s{2,}(.+)$`, "mi"); + const regex = new RegExp(`^${escapeRegExp(key)}\\s{2,}(.+)$`, "mi"); const match = regex.exec(plain); return match?.[1]?.trim(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/CursorProvider.ts` around lines 583 - 586, The regex in extractAboutField builds a pattern from the key parameter which can be unsafe if key contains regex metacharacters; update extractAboutField to escape key before interpolating into the RegExp (e.g., add or reuse an escapeRegExp helper that replaces [/.*+?^${}()|[\]\\]/g with escaped versions) and use the escapedKey in new RegExp(`^${escapedKey}\\s{2,}(.+)$`, "mi") so keys like "CLI Version" or "User Email" are treated literally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/package.json`:
- Around line 44-45: The packages "effect-acp" and "effect-codex-app-server" are
required at runtime by production modules (e.g., AcpSessionRuntime.ts,
CodexAdapter.ts, CodexSessionRuntime.ts), so move their entries from
devDependencies into dependencies in package.json (keep the same "workspace:*"
specifiers), then run install to update the lockfile; ensure no other references
rely on them being dev-only.
In `@apps/server/src/provider/acp/CursorAcpCliProbe.test.ts`:
- Around line 47-67: The test currently logs entire ACP responses via
console.log calls around result, configOptions, modelConfig and
parameterizedOptions (see variables result, configOptions, modelConfig,
parameterizedOptions and started.sessionId); remove or replace those verbose
console.log statements with minimal/redacted output (e.g., log only
started.sessionId, counts or boolean presence checks) so sensitive
session/account metadata is not printed in CI logs, and ensure assertions
(expect(...)) remain unchanged.
- Around line 54-69: The test currently guards assertions with if
(Array.isArray(configOptions)) which lets the test silently pass when
configOptions has the wrong shape; replace that conditional with a hard
assertion that configOptions is an array (e.g.,
expect(Array.isArray(configOptions)).toBe(true) or
expect(configOptions).toBeInstanceOf(Array)) so the test fails fast on invalid
shape, then perform the existing checks on modelConfig, parameterizedOptions and
modelConfig.id outside any conditional; reference the configOptions variable and
the modelConfig/parameterizedOptions checks in CursorAcpCliProbe.test.ts.
In `@apps/server/src/provider/acp/CursorAcpExtension.ts`:
- Around line 1-4: Top-of-file comment in CursorAcpExtension.ts exposes an
internal Slack URL; remove that URL and replace it with either a checked-in docs
path or a neutral note (e.g., "internal design doc available in repo/docs" or
"contact team for internal reference") in the header comment for the
CursorAcpExtension file; update the comment block that currently contains the
Slack link so it only references public docs and the local/checked-in
documentation and do not leave any internal workspace/file links.
- Around line 85-89: The code treats whitespace-only todo.content as present and
prevents falling back to todo.title; update the logic that computes step in
CursorAcpExtension (where plan is built from params.todos) so that you use the
trimmed content only if it is non-empty, otherwise fall back to the trimmed
title (and finally to empty string). Concretely, replace the current expression
that sets step from todo.content?.trim() ?? todo.title?.trim() with a check that
treats an empty trimmed string as absent (e.g., prefer todo.content?.trim() when
truthy, else todo.title?.trim(), else ""), keeping the subsequent empty-step
filter unchanged.
In `@apps/server/src/provider/cliVersion.ts`:
- Around line 10-21: normalizeCliVersion currently leaves spaces inside the
prerelease suffix (e.g., " rc .1"); update the function to trim and canonicalize
the prerelease identifier: after splitting into main and prerelease, call
prerelease = prerelease?.trim(), then split prerelease on "." into parts, map
each part to part.trim(), filter out empty parts, and join with "." to form the
cleaned prerelease; finally return either
`${segments.join(".")}-${cleanPrerelease}` or segments.join(".") (handling the
case where cleaning yields an empty string). Reference: normalizeCliVersion, the
prerelease variable and the final return expression.
In `@apps/server/src/provider/Layers/ClaudeAdapter.test.ts`:
- Around line 1736-1748: The test is clamping the earlier progress snapshot
instead of the completion snapshot: locate the runtimeEvents -> usageEvents
filter and the finalUsageEvent check in ClaudeAdapter.test.ts (identify
runtimeEvents, usageEvents, finalUsageEvent and its payload) and update the
assertion for the final "thread.token-usage.updated" payload so that usedTokens
and lastUsedTokens are clamped to the contextWindow (200000) rather than the
earlier 190000 value, ensuring totalProcessedTokens remains 535000 and
maxTokens/contextWindow are asserted as 200000.
In `@apps/server/src/provider/Layers/CodexAdapter.test.ts`:
- Around line 104-109: Tests currently re-run startup because
FakeCodexRuntime.getSession calls startImpl(); change start() to call
startImpl(), store the resulting session in a private cachedSession field, and
update getSession to return Effect.promise(() => cachedSession) (or an Effect
that reads cachedSession and fails if unset) instead of invoking startImpl();
ensure the symbol names start, startImpl, getSession, and cachedSession are used
so callers read the previously started session rather than restarting.
In `@apps/server/src/provider/Layers/CodexAdapter.ts`:
- Around line 1335-1445: startSession currently reads/writes the shared sessions
map without mutual exclusion, allowing concurrent calls for the same threadId to
both create runtimes; introduce per-thread serialization so only one
startSession runs for a given threadId at a time. Fix by adding a per-thread
lock (e.g., a Map<string, Mutex/Semaphore> or an Effect-based lock) that
startSession acquires before reading sessions.get(input.threadId) and
creating/setting sessions; ensure the lock is released after the
creation/cleanup logic (including calls to stopSessionInternal,
sessionScope/sessionScopeTransferred handling, runtime creation via
makeCodexSessionRuntime, eventFiber setup and runtime.start) so concurrent
invocations for the same threadId queue and only the winner installs the
sessions.set entry while losers observe the installed session or fail cleanly.
In `@apps/server/src/provider/Layers/CodexProvider.ts`:
- Around line 230-239: The initialize request is using a hardcoded version
"0.1.0" which conflicts with buildCodexInitializeParams() that uses
packageJson.version; update the client.request("initialize", ...) call in
CodexProvider (the const initialize assignment) to use the same dynamic version
source—either call buildCodexInitializeParams() to supply the full params or
replace the literal with packageJson.version—so the clientInfo.version is
consistent with buildCodexInitializeParams().
In `@apps/server/src/provider/Layers/CursorAdapter.test.ts`:
- Around line 420-422: Remove the direct mutation of
process.env.T3_ACP_EMIT_TOOL_CALLS in the CursorAdapter.test and rely on the
existing test harness that passes this flag via makeMockAgentWrapper;
specifically, delete the lines that set and restore
process.env.T3_ACP_EMIT_TOOL_CALLS and ensure the test calls use
makeMockAgentWrapper with the proper { T3_ACP_EMIT_TOOL_CALLS: "1" } argument
(and do the same cleanup for the similar block around the makeMockAgentWrapper
usage repeated later in the file).
In `@apps/server/src/provider/Layers/CursorAdapter.ts`:
- Around line 815-904: The code marks ctx.activeTurnId, updates
ctx.session.activeTurnId and emits the turn.started event before validating
attachments and running ctx.acp.prompt, which can leave an active turn when
prompt fails; fix by deferring the assignments to ctx.activeTurnId and
ctx.session.activeTurnId and the offerRuntimeEvent({ type: "turn.started", ...
}) call until after promptParts are validated and ctx.acp.prompt(...) completes
successfully (or alternatively wrap the existing section in a try/finally that
clears ctx.activeTurnId and emits turn.completed on error); locate and update
the block using symbols ctx.activeTurnId, ctx.session, offerRuntimeEvent, and
ctx.acp.prompt to implement the change.
- Around line 813-931: The code mutates a shared mutable ctx.activeTurnId inside
sendTurn which races when sendTurn runs concurrently; stop using the shared
field for runtime event attribution and instead use the local turnId for this
invocation and pass it explicitly to offerRuntimeEvent and any runtime calls
(e.g., use local turnId variable when emitting "turn.started" and
"turn.completed" and when storing the turn in ctx.turns), only update
ctx.activeTurnId / ctx.session.activeTurnId at a deterministic point (for
example after the runtime call completes or under an explicit lock) so
concurrent sendTurn calls cannot overwrite each other's active id; update all
references to ctx.activeTurnId in sendTurn to use the local turnId and ensure
ctx.session.activeTurnId is updated once (or via a compare-and-set) to avoid
misattribution.
In `@apps/server/src/provider/Layers/CursorProvider.test.ts`:
- Around line 530-534: The test fixture embeds a real-looking email in the JSON
stdout (the stdout: JSON.stringify block in CursorProvider.test.ts), which
introduces PII; update the userEmail value from "jmarminge@gmail.com" to a
reserved example address such as "user@example.com" (or "example@example.com")
inside the JSON.stringify call so tests use a placeholder instead of a real
email.
In `@apps/server/src/provider/Layers/OpenCodeProvider.ts`:
- Around line 280-287: The code computes isExternalServer using
input.settings.serverUrl.trim() but later uses the untrimmed
input.settings.serverUrl in connectToOpenCodeServer() and
formatOpenCodeProbeError(), causing mismatches for URLs with surrounding
whitespace; normalize once by assigning a trimmed value (e.g., const serverUrl =
input.settings.serverUrl.trim()) and then use that trimmed serverUrl
everywhere—update uses in isExternalServer, connectToOpenCodeServer,
fallback/formatOpenCodeProbeError and the other occurrences around the block
referenced (lines ~356-359) to ensure dialing and error messages use the same
normalized string.
In `@apps/server/src/provider/Layers/ProviderRegistry.test.ts`:
- Around line 447-449: The test is asserting spawnCount synchronously before the
background probe may have started; move the spawnCount assertion into the
existing polling/wait path (the generator that produces refreshed via
Effect.gen) or perform it after the ProviderRegistry reaches ready so the
background probe has time to run; specifically, remove the immediate
assert.strictEqual(spawnCount > 0, true) and instead assert spawnCount > 0
inside the polling callback that checks refreshed (or after the provider-ready
check) so the assertion happens only once the background work has been awaited.
- Line 26: The test currently mutates process.env.T3CODE_CURSOR_ENABLED at
module scope in ProviderRegistry.test.ts which can leak into other specs; move
this environment variable change into the specific test or a test-local
setup/teardown (e.g., inside the it/test block or using beforeEach/afterEach for
that describe) and restore the original value afterwards so other tests are
unaffected; locate the module-scope assignment to
process.env.T3CODE_CURSOR_ENABLED in ProviderRegistry.test.ts and change it to
set the var at test start and reset it to its previous value at test end.
In `@apps/server/src/provider/Layers/ProviderRegistry.ts`:
- Around line 50-70: mergeProviderModels currently preserves any previousModels
whose slugs are missing from nextModels (via the early return and the final
spread of previousModels.filter), preventing providers from removing models;
update the logic so the cache mirrors nextModels exactly: when nextModels.length
=== 0 return [] (not previousModels), and remove the final concatenation of
previousModels.filter(... ) so the function returns only mergedModels (with the
existing capability-preservation behavior that copies previousModel.capabilities
when appropriate). Reference: mergeProviderModels, previousModels, nextModels,
previousBySlug, mergedModels, nextSlugs.
In `@apps/server/src/provider/Layers/ProviderService.test.ts`:
- Around line 246-248: The fake adapters (codex, claude, cursor) are created at
module scope causing shared mutable state from makeProviderServiceLayer()
(notably its sessions maps and vi.fn histories) to leak between tests; update
the tests to instantiate makeFakeCodexAdapter() for codex,
makeFakeCodexAdapter("claudeAgent") for claude, and
makeFakeCodexAdapter("cursor") for cursor inside a per-test setup (e.g.,
beforeEach or within each test) so that each test gets fresh adapter instances
and the makeProviderServiceLayer() state is isolated per test run.
In `@apps/server/src/provider/Layers/ProviderService.ts`:
- Around line 317-329: The current pipeline around adapter.stopSession (used in
ProviderService) swallows failures via Effect.catchCause and only logs
"provider.session.stop-stale-failed", allowing startSession to proceed and
rebind the thread while the old provider remains active; change this so
stopSession failures are not ignored: make adapter.stopSession's error propagate
(remove or convert the Effect.catchCause into a failing Effect) so startSession
cannot run when stopSession fails, or if you prefer a safer flow, mark the
thread as quarantined/locked (update thread state via ProviderManager APIs in
providerManager.ts) and abort the handoff until a successful stale-session
cleanup completes and analytics.record("provider.session.stopped") only runs
after a confirmed stop; ensure all references to adapter.stopSession,
startSession, provider.session.stopped and provider.session.stop-stale-failed
are updated accordingly.
---
Nitpick comments:
In `@apps/server/src/provider/acp/AcpAdapterSupport.test.ts`:
- Around line 13-26: Add a unit test that verifies mapAcpToAdapterError maps an
AcpProcessExitedError to a ProviderAdapterSessionClosedError: in
AcpAdapterSupport.test.ts create a test (e.g., "maps ACP process exited errors
to provider adapter session closed errors") that calls mapAcpToAdapterError with
the same arguments style as the existing test but passing new
EffectAcpErrors.AcpProcessExitedError({ code: 1 }) and assert that the returned
error._tag equals "ProviderAdapterSessionClosedError"; ensure you import/use
EffectAcpErrors and mirror the structure of the existing "maps ACP request
errors..." test.
In `@apps/server/src/provider/acp/AcpAdapterSupport.ts`:
- Around line 28-41: The two branches that check
Schema.is(EffectAcpErrors.AcpRequestError)(error) and the default both construct
the same ProviderAdapterRequestError; remove the redundant conditional and
always return the unified ProviderAdapterRequestError({ provider, method,
detail: error.message, cause: error }) so the mapping is consolidated into a
single return path (update the code in AcpAdapterSupport where
Schema.is(EffectAcpErrors.AcpRequestError) is used and ensure
ProviderAdapterRequestError remains constructed the same).
In `@apps/server/src/provider/acp/AcpSessionRuntime.ts`:
- Around line 581-584: The non-null assertion on parsed.modeId is redundant
because you already guarded with if (parsed.modeId); remove the exclamation and
pass parsed.modeId directly to updateModeState inside the Ref.update call (i.e.,
in the branch that updates modeStateRef call Ref.update(..., current ===
undefined ? current : updateModeState(current, parsed.modeId))). This affects
the parsed.modeId usage in the Ref.update invocation and keeps TypeScript happy
without the unnecessary `!`.
In `@apps/server/src/provider/cliVersion.test.ts`:
- Around line 14-15: Rename the test description string to reflect actual
behavior: update the it("rejects malformed numeric segments", ...) to something
like it("falls back to ordering for malformed numeric segments", ...) so the
test accurately documents that compareCliVersions("1.2.3abc", "1.2.10") > 0
validates fallback ordering rather than expecting a rejection/throw; keep the
assertion and function name compareCliVersions unchanged.
In `@apps/server/src/provider/Layers/CodexSessionRuntime.ts`:
- Around line 1173-1188: The close function currently closes runtimeScope before
shutting down the queues, which can cause fibers to be interrupted while
offering to serverNotifications/events; update close (the Effect.gen function
named close) to call Queue.shutdown(serverNotifications) and
Queue.shutdown(events) before calling Scope.close(runtimeScope, Exit.void),
keeping the existing settlePendingApprovals, settlePendingUserInputs,
updateSession, and emitSessionEvent calls as appropriate so that queues are
drained/closed prior to closing the scope to avoid race conditions.
- Around line 59-65: Add a tracked GitHub issue for the upstream schema
generation bug where EffectCodexSchema.V2TurnStartParams does not include
collaborationMode (currently patched locally by
CodexTurnStartParamsWithCollaborationMode using Schema.fieldsAssign and
V2TurnStartParams__CollaborationMode); in the issue describe the expected schema
change (V2TurnStartParams should directly include collaborationMode as
optional), point to packages/effect-codex-app-server/scripts/generate.ts as the
generator to modify, and note this file (CodexSessionRuntime.ts) can remove the
manual fieldsAssign workaround once the generator is fixed so we can delete
CodexTurnStartParamsWithCollaborationMode and rely on
EffectCodexSchema.V2TurnStartParams.
In `@apps/server/src/provider/Layers/CursorProvider.ts`:
- Around line 583-586: The regex in extractAboutField builds a pattern from the
key parameter which can be unsafe if key contains regex metacharacters; update
extractAboutField to escape key before interpolating into the RegExp (e.g., add
or reuse an escapeRegExp helper that replaces [/.*+?^${}()|[\]\\]/g with escaped
versions) and use the escapedKey in new RegExp(`^${escapedKey}\\s{2,}(.+)$`,
"mi") so keys like "CLI Version" or "User Email" are treated literally.
In `@apps/server/src/provider/Layers/OpenCodeAdapter.ts`:
- Around line 392-414: The function updateProviderSession uses an unsafe cast to
Record<string, unknown> and deletes fields to clear properties; instead build a
properly typed ProviderSession object without mutating via cast: create
nextSession as a ProviderSession (spread context.session and patch, set
updatedAt), then if options.clearActiveTurnId or options.clearLastError,
construct a new object that omits those keys (e.g., by destructuring to separate
activeTurnId/lastError and reassembling only the remaining fields) or use a
typed helper that returns Omit<ProviderSession, 'activeTurnId'|'lastError'>
before assigning to context.session; ensure the returned value is typed
ProviderSession and remove any Record<string, unknown> casts (references:
updateProviderSession, ProviderSession, OpenCodeSessionContext,
options.clearActiveTurnId, options.clearLastError).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ce0ca04-2bb5-4845-a3c3-3bcdde64a312
⛔ Files ignored due to path filters (6)
bun.lockis excluded by!**/*.lockpackages/effect-acp/src/_generated/meta.gen.tsis excluded by!**/_generated/**packages/effect-acp/src/_generated/schema.gen.tsis excluded by!**/_generated/**packages/effect-codex-app-server/src/_generated/meta.gen.tsis excluded by!**/_generated/**packages/effect-codex-app-server/src/_generated/namespaces.gen.tsis excluded by!**/_generated/**packages/effect-codex-app-server/src/_generated/schema.gen.tsis excluded by!**/_generated/**
📒 Files selected for processing (136)
apps/server/package.jsonapps/server/src/config.tsapps/server/src/git/Layers/CodexTextGeneration.tsapps/server/src/git/Services/TextGeneration.tsapps/server/src/provider/CodexDeveloperInstructions.tsapps/server/src/provider/Layers/ClaudeAdapter.test.tsapps/server/src/provider/Layers/ClaudeAdapter.tsapps/server/src/provider/Layers/ClaudeProvider.tsapps/server/src/provider/Layers/CodexAdapter.test.tsapps/server/src/provider/Layers/CodexAdapter.tsapps/server/src/provider/Layers/CodexProvider.tsapps/server/src/provider/Layers/CodexSessionRuntime.test.tsapps/server/src/provider/Layers/CodexSessionRuntime.tsapps/server/src/provider/Layers/CursorAdapter.test.tsapps/server/src/provider/Layers/CursorAdapter.tsapps/server/src/provider/Layers/CursorProvider.test.tsapps/server/src/provider/Layers/CursorProvider.tsapps/server/src/provider/Layers/OpenCodeAdapter.test.tsapps/server/src/provider/Layers/OpenCodeAdapter.tsapps/server/src/provider/Layers/OpenCodeProvider.test.tsapps/server/src/provider/Layers/OpenCodeProvider.tsapps/server/src/provider/Layers/ProviderAdapterRegistry.test.tsapps/server/src/provider/Layers/ProviderAdapterRegistry.tsapps/server/src/provider/Layers/ProviderRegistry.test.tsapps/server/src/provider/Layers/ProviderRegistry.tsapps/server/src/provider/Layers/ProviderService.test.tsapps/server/src/provider/Layers/ProviderService.tsapps/server/src/provider/Layers/ProviderSessionDirectory.test.tsapps/server/src/provider/Layers/ProviderSessionDirectory.tsapps/server/src/provider/Layers/ProviderSessionReaper.test.tsapps/server/src/provider/Layers/ProviderSessionReaper.tsapps/server/src/provider/Services/ClaudeProvider.tsapps/server/src/provider/Services/CodexProvider.tsapps/server/src/provider/Services/CursorAdapter.tsapps/server/src/provider/Services/CursorProvider.tsapps/server/src/provider/Services/OpenCodeAdapter.tsapps/server/src/provider/Services/OpenCodeProvider.tsapps/server/src/provider/Services/ProviderSessionDirectory.tsapps/server/src/provider/Services/ProviderSessionReaper.tsapps/server/src/provider/acp/AcpAdapterSupport.test.tsapps/server/src/provider/acp/AcpAdapterSupport.tsapps/server/src/provider/acp/AcpCoreRuntimeEvents.test.tsapps/server/src/provider/acp/AcpCoreRuntimeEvents.tsapps/server/src/provider/acp/AcpJsonRpcConnection.test.tsapps/server/src/provider/acp/AcpNativeLogging.tsapps/server/src/provider/acp/AcpRuntimeModel.test.tsapps/server/src/provider/acp/AcpRuntimeModel.tsapps/server/src/provider/acp/AcpSessionRuntime.tsapps/server/src/provider/acp/CursorAcpCliProbe.test.tsapps/server/src/provider/acp/CursorAcpExtension.test.tsapps/server/src/provider/acp/CursorAcpExtension.tsapps/server/src/provider/acp/CursorAcpSupport.test.tsapps/server/src/provider/acp/CursorAcpSupport.tsapps/server/src/provider/cliVersion.test.tsapps/server/src/provider/cliVersion.tsapps/server/src/provider/makeManagedServerProvider.test.tsapps/server/src/provider/makeManagedServerProvider.tsapps/server/src/provider/opencodeRuntime.tsapps/server/src/provider/providerSnapshot.test.tsapps/server/src/provider/providerSnapshot.tsapps/server/src/provider/providerStatusCache.test.tsapps/server/src/provider/providerStatusCache.tsapps/server/src/server.tsapps/server/src/serverSettings.test.tsapps/server/src/serverSettings.tsapps/web/src/components/ChatView.tsxapps/web/src/components/KeybindingsToast.browser.tsxapps/web/src/components/chat/ChatComposer.tsxapps/web/src/components/chat/CompactComposerControlsMenu.browser.tsxapps/web/src/components/chat/CompactComposerControlsMenu.tsxapps/web/src/components/chat/ModelListRow.tsxapps/web/src/components/chat/ModelPickerContent.tsxapps/web/src/components/chat/ModelPickerSidebar.tsxapps/web/src/components/chat/ProviderLimitsButton.tsxapps/web/src/components/chat/ProviderModelPicker.browser.tsxapps/web/src/components/chat/ProviderModelPicker.tsxapps/web/src/components/chat/TraitsPicker.browser.tsxapps/web/src/components/chat/TraitsPicker.tsxapps/web/src/components/chat/composerProviderRegistry.tsxapps/web/src/components/chat/modelPickerModelHighlights.tsapps/web/src/components/chat/modelPickerSearch.test.tsapps/web/src/components/chat/modelPickerSearch.tsapps/web/src/components/chat/providerIconUtils.tsapps/web/src/components/providerBrandIcon.test.tsxapps/web/src/components/providerBrandIcon.tsxapps/web/src/components/settings/SettingsPanels.tsxapps/web/src/components/settings/UsageSettingsPanel.tsxapps/web/src/composerDraftStore.tsapps/web/src/hooks/useSettings.tsapps/web/src/modelPickerOpenState.tsapps/web/src/modelSelection.tsapps/web/src/providerModels.tsapps/web/src/session-logic.tsapps/web/src/store.tspackage.jsonpackages/contracts/src/model.tspackages/contracts/src/orchestration.tspackages/contracts/src/providerRuntime.tspackages/contracts/src/server.tspackages/contracts/src/settings.tspackages/effect-acp/package.jsonpackages/effect-acp/scripts/generate.tspackages/effect-acp/src/_internal/shared.tspackages/effect-acp/src/_internal/stdio.tspackages/effect-acp/src/agent.test.tspackages/effect-acp/src/agent.tspackages/effect-acp/src/client.test.tspackages/effect-acp/src/client.tspackages/effect-acp/src/errors.tspackages/effect-acp/src/protocol.test.tspackages/effect-acp/src/protocol.tspackages/effect-acp/src/rpc.tspackages/effect-acp/src/schema.tspackages/effect-acp/src/terminal.tspackages/effect-acp/test/examples/cursor-acp-client.example.tspackages/effect-acp/test/fixtures/acp-mock-peer.tspackages/effect-acp/tsconfig.jsonpackages/effect-codex-app-server/package.jsonpackages/effect-codex-app-server/scripts/generate.tspackages/effect-codex-app-server/src/_internal/shared.tspackages/effect-codex-app-server/src/_internal/stdio.tspackages/effect-codex-app-server/src/client.test.tspackages/effect-codex-app-server/src/client.tspackages/effect-codex-app-server/src/errors.tspackages/effect-codex-app-server/src/protocol.test.tspackages/effect-codex-app-server/src/protocol.tspackages/effect-codex-app-server/src/rpc.tspackages/effect-codex-app-server/src/schema.tspackages/effect-codex-app-server/test/examples/codex-app-server-probe.tspackages/effect-codex-app-server/test/fixtures/codex-app-server-mock-peer.tspackages/effect-codex-app-server/tsconfig.jsonpackages/shared/package.jsonpackages/shared/src/cliArgs.tspackages/shared/src/model.tspackages/shared/src/serverSettings.tspackages/shared/src/toolActivity.ts
Comment on lines
+54
to
+69
| if (Array.isArray(configOptions)) { | ||
| const modelConfig = configOptions.find((opt) => opt.category === "model"); | ||
| const parameterizedOptions = configOptions.filter( | ||
| (opt) => | ||
| opt.category === "thought_level" || | ||
| opt.category === "model_option" || | ||
| opt.category === "model_config", | ||
| ); | ||
| console.log("Model config option:", JSON.stringify(modelConfig, null, 2)); | ||
| console.log( | ||
| "Parameterized model config options:", | ||
| JSON.stringify(parameterizedOptions, null, 2), | ||
| ); | ||
| expect(modelConfig).toBeDefined(); | ||
| expect(typeof modelConfig?.id).toBe("string"); | ||
| } |
There was a problem hiding this comment.
Fail fast on unexpected configOptions shape to avoid false-positive passes.
At Line 54 and Line 113, assertions are nested behind if (Array.isArray(...)); if the shape is wrong, the test still passes without checking the contract.
✅ Proposed assertion hardening
const configOptions = result.configOptions;
- if (Array.isArray(configOptions)) {
- const modelConfig = configOptions.find((opt) => opt.category === "model");
+ expect(Array.isArray(configOptions)).toBe(true);
+ if (Array.isArray(configOptions)) {
+ const modelConfig = configOptions.find((opt) => opt.category === "model");
const parameterizedOptions = configOptions.filter(
@@
expect(modelConfig).toBeDefined();
expect(typeof modelConfig?.id).toBe("string");
}
@@
- if (Array.isArray(setResult.configOptions)) {
- const modelConfig = setResult.configOptions.find((opt) => opt.category === "model");
+ expect(Array.isArray(setResult.configOptions)).toBe(true);
+ if (Array.isArray(setResult.configOptions)) {
+ const modelConfig = setResult.configOptions.find((opt) => opt.category === "model");
const parameterizedOptions = setResult.configOptions.filter(
@@
expect(parameterizedOptions.length).toBeGreaterThan(0);
}Also applies to: 113-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/provider/acp/CursorAcpCliProbe.test.ts` around lines 54 - 69,
The test currently guards assertions with if (Array.isArray(configOptions))
which lets the test silently pass when configOptions has the wrong shape;
replace that conditional with a hard assertion that configOptions is an array
(e.g., expect(Array.isArray(configOptions)).toBe(true) or
expect(configOptions).toBeInstanceOf(Array)) so the test fails fast on invalid
shape, then perform the existing checks on modelConfig, parameterizedOptions and
modelConfig.id outside any conditional; reference the configOptions variable and
the modelConfig/parameterizedOptions checks in CursorAcpCliProbe.test.ts.
- Serialize Codex and Cursor work per thread - Fix session cleanup, token usage updates, and provider snapshot merging - Improve CLI parsing and error propagation
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
apps/server/src/provider/Layers/ProviderService.test.ts (1)
356-362:⚠️ Potential issue | 🟠 MajorRecreate the test layer per case; resetting adapters alone still leaks shared state.
routingis still built once at module scope, so the sharedSqlitePersistenceMemory/ProviderSessionDirectory/ProviderServicestate survives across tests. Clearing only the fake adapters leaves persisted bindings and other scoped state behind, which makes the repeatedthread-1assertions order-dependent. The same pattern is repeated forfanoutandvalidation.Suggested test setup pattern
-const routing = makeProviderServiceLayer(); - -beforeEach(() => { - routing.codex.reset(); - routing.claude.reset(); - routing.cursor.reset(); -}); +let routing: ReturnType<typeof makeProviderServiceLayer>; + +beforeEach(() => { + routing = makeProviderServiceLayer(); +});Apply the same change to the
fanoutandvalidationfixtures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/ProviderService.test.ts` around lines 356 - 362, The test currently constructs routing once via makeProviderServiceLayer() at module scope so stateful components (SqlitePersistenceMemory, ProviderSessionDirectory, ProviderService) leak across tests; instead, move the call to makeProviderServiceLayer() into each test's setup (e.g., beforeEach) so a fresh layer is created per test and drop relying solely on .reset() for adapters; apply the same change for the fanout and validation fixtures (replace module-scoped makeProviderServiceLayer()/fanout/validation initialization with per-test creation in beforeEach and then use routing/fanout/validation from that fresh instance).apps/server/src/provider/Layers/CodexAdapter.ts (1)
1367-1373:⚠️ Potential issue | 🟠 MajorUse the same thread lock for stop paths too.
startSessionis serialized now, butstopSession/stopAllstill bypasswithThreadLockwhile mutating the samesessionsmap. A concurrent stop can race a replacement start and return successfully even though the new runtime finishes installing afterward.Also applies to: 1649-1672
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/CodexAdapter.ts` around lines 1367 - 1373, startSession is already serialized with withThreadLock but stopSession and stopAll mutate the same sessions map without locking; wrap the mutating logic in stopSession and stopAll with the same withThreadLock(threadId, ...) pattern (use the same threadId used by startSession or derive it per-session) so both stopping a session and stopping all sessions acquire the semaphore from getThreadSemaphore before touching the sessions map, ensuring races between startSession and stopSession/stopAll are serialized; update stopAll to iterate/stop under the per-thread lock or acquire each thread's semaphore when removing entries from sessions.apps/server/src/provider/Layers/ClaudeAdapter.ts (1)
2475-2504:⚠️ Potential issue | 🟠 MajorSerialize Claude session replacement per thread.
This replacement path still reads
sessions, stops the old context, creates a new runtime, and writes back without any thread-local lock. Two concurrentstartSessioncalls for the samethreadIdcan both get past this block and leave the losing query/fiber running after itssessionsentry is overwritten.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/ClaudeAdapter.ts` around lines 2475 - 2504, Concurrent startSession calls can race when replacing sessions; wrap the read/stop/create/write sequence in a per-thread mutex to serialize replacements. Modify startSession to acquire a lock keyed by input.threadId (e.g., a Map<string, Mutex> or similar), then re-check sessions.get(threadId) inside the lock, call stopSessionInternal and await its best-effort cleanup inside the lock, create the new runtime and write sessions.set(threadId, newContext) while holding the lock, and finally release and cleanup the mutex entry; use the existing symbols startSession, sessions, stopSessionInternal and ensure the lock prevents two fibers from both proceeding to overwrite the same sessions entry.
🧹 Nitpick comments (4)
apps/server/src/provider/Layers/OpenCodeProvider.ts (2)
155-157: Redundant?? undefinedin return expression.
agents[0]?.namealready evaluates toundefinedwhen the array is empty or whennameis undefined, making the?? undefinedfallback unnecessary.✨ Suggested simplification
function inferDefaultAgent(agents: ReadonlyArray<Agent>): string | undefined { - return agents.find((agent) => agent.name === "build")?.name ?? agents[0]?.name ?? undefined; + return agents.find((agent) => agent.name === "build")?.name ?? agents[0]?.name; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/OpenCodeProvider.ts` around lines 155 - 157, The return expression in inferDefaultAgent contains a redundant "?? undefined"; remove the trailing "?? undefined" and simplify the return to use the existing optional chaining fallbacks (i.e., return agents.find((agent) => agent.name === "build")?.name ?? agents[0]?.name). Update the function inferDefaultAgent accordingly so it relies on ?.name yielding undefined when missing instead of explicitly coalescing to undefined.
174-189: Consider using spread syntax for conditional properties.The
Object.assignpattern works but is less idiomatic in modern TypeScript. Spread syntax with conditional properties is more readable.✨ Alternative using spread syntax
const variantOptions: ModelCapabilities["variantOptions"] = variantValues.map((value) => - Object.assign( - { value, label: titleCaseSlug(value) }, - defaultVariant === value ? { isDefault: true } : {}, - ), + ({ + value, + label: titleCaseSlug(value), + ...(defaultVariant === value && { isDefault: true }), + }), ); // ... similar for agentOptions const agentOptions: ModelCapabilities["agentOptions"] = primaryAgents.map((agent) => - Object.assign( - { value: agent.name, label: titleCaseSlug(agent.name) }, - defaultAgent === agent.name ? { isDefault: true } : {}, - ), + ({ + value: agent.name, + label: titleCaseSlug(agent.name), + ...(defaultAgent === agent.name && { isDefault: true }), + }), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/OpenCodeProvider.ts` around lines 174 - 189, The code builds variantOptions and agentOptions using Object.assign with conditional isDefault objects; replace these with spread syntax for readability: construct each option as { value: ..., label: titleCaseSlug(...) , ...(defaultVariant === value && { isDefault: true }) } for variantOptions and similarly for agentOptions using primaryAgents, defaultAgent and inferDefaultAgent to determine the conditional property; update the map callbacks for variantValues and primaryAgents to use spread-based object literals instead of Object.assign.apps/server/src/provider/Layers/CodexProvider.ts (1)
181-197: Consider adding a pagination limit to prevent unbounded loops.The
requestAllCodexModelsfunction paginates through all models without an upper bound. If the server returns an unexpectedly large number of pages or has a bug causing infinite pagination, this could run indefinitely.💡 Suggested safeguard
const requestAllCodexModels = Effect.fn("requestAllCodexModels")(function* ( client: CodexClient.CodexAppServerClientShape, ) { const models: ServerProviderModel[] = []; let cursor: string | null | undefined = undefined; + const MAX_PAGES = 100; + let pageCount = 0; do { + if (pageCount++ >= MAX_PAGES) { + break; + } const response: CodexSchema.V2ModelListResponse = yield* client.request( "model/list", cursor ? { cursor } : {}, ); models.push(...parseCodexModelListResponse(response)); cursor = response.nextCursor; } while (cursor); return models; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/CodexProvider.ts` around lines 181 - 197, The pagination in requestAllCodexModels can loop indefinitely; add a max-pages safeguard (e.g., const MAX_PAGES = N) and count iterations inside the do/while, calling break or throwing an error when the page count exceeds MAX_PAGES to prevent unbounded loops; update the logic around cursor, client.request("model/list", ...) and parseCodexModelListResponse(response) to respect this limit and surface a clear error/log message if the limit is reached so callers know pagination was truncated.apps/server/src/provider/acp/AcpSessionRuntime.ts (1)
566-634: Consider adding error handling for malformed session notifications.The
handleSessionUpdatefunction processes parsed events in a loop. IfparseSessionUpdateEventthrows or returns unexpected data, the entire notification handling could fail silently.💡 Suggested defensive wrapper
const handleSessionUpdate = ({ queue, modeStateRef, toolCallsRef, assistantSegmentRef, params, }: { readonly queue: Queue.Queue<AcpParsedSessionEvent>; readonly modeStateRef: Ref.Ref<AcpSessionModeState | undefined>; readonly toolCallsRef: Ref.Ref<Map<string, AcpToolCallState>>; readonly assistantSegmentRef: Ref.Ref<AcpAssistantSegmentState>; readonly params: EffectAcpSchema.SessionNotification; }): Effect.Effect<void> => - Effect.gen(function* () { + Effect.gen(function* () { + const parsed = yield* Effect.try({ + try: () => parseSessionUpdateEvent(params), + catch: (error) => error, + }).pipe( + Effect.catchAll((error) => + Effect.logWarning("Failed to parse session update", { error, params }).pipe( + Effect.as({ modeId: undefined, events: [] }), + ), + ), + ); - const parsed = parseSessionUpdateEvent(params); // ... rest of implementation });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/acp/AcpSessionRuntime.ts` around lines 566 - 634, handleSessionUpdate currently calls parseSessionUpdateEvent without guarding against exceptions or malformed shapes; wrap the parse step in an Effect try/catch (e.g., Effect.tryCatch/Effect.sync with Effect.catchAll) and validate the returned shape (ensure parsed.events is an array and parsed.modeId has expected form) before proceeding. On error or invalid shape, emit a deterministic fallback (e.g., Queue.offer(queue, { _tag: "SessionUpdateMalformed", sessionId: params.sessionId, error: <error message> })) or early-return after logging, so subsequent refs/ensures (ensureActiveAssistantSegment, Ref.modify on toolCallsRef, etc.) are not executed with bad data; update handleSessionUpdate, parseSessionUpdateEvent usage, and the caller-side queue consumers to handle the new malformed event tag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/provider/Layers/CodexAdapter.ts`:
- Around line 1347-1365: threadLocksRef currently stores a Semaphore per thread
and never removes entries; change the registry to store a small usage-tracking
object (e.g., { semaphore: Semaphore.Semaphore, count: number } or a Ref for
count) instead of raw Semaphore in threadLocksRef, update getThreadSemaphore to
increment the count when returning the semaphore and to return or expose a
release/decrement operation that decrements the count and, when count reaches
zero, removes the threadId entry from the Map (using
SynchronizedRef.modifyEffect to perform the decrement-and-remove atomically).
Keep the SynchronizedRef and Semaphore usage but ensure every acquire path
increments the count and every release path decrements and cleans up the Map to
prevent unbounded growth.
In `@apps/server/src/provider/Layers/ProviderService.ts`:
- Around line 390-393: The current ordering starts a new provider session via
adapter.startSession and then calls stopStaleSessionsForThread, which can fail
and leave the new session live; either reorder the operations so
stopStaleSessionsForThread(...) runs and succeeds before calling
adapter.startSession(...), or wrap the stopStaleSessionsForThread(...) call in a
try/catch and on failure explicitly roll back the newly started session by
calling the adapter's stop/teardown method (e.g., adapter.stopSession(...) or
adapter.endSession(...) corresponding to adapter.startSession) before rethrowing
the error so the handoff is atomic.
---
Duplicate comments:
In `@apps/server/src/provider/Layers/ClaudeAdapter.ts`:
- Around line 2475-2504: Concurrent startSession calls can race when replacing
sessions; wrap the read/stop/create/write sequence in a per-thread mutex to
serialize replacements. Modify startSession to acquire a lock keyed by
input.threadId (e.g., a Map<string, Mutex> or similar), then re-check
sessions.get(threadId) inside the lock, call stopSessionInternal and await its
best-effort cleanup inside the lock, create the new runtime and write
sessions.set(threadId, newContext) while holding the lock, and finally release
and cleanup the mutex entry; use the existing symbols startSession, sessions,
stopSessionInternal and ensure the lock prevents two fibers from both proceeding
to overwrite the same sessions entry.
In `@apps/server/src/provider/Layers/CodexAdapter.ts`:
- Around line 1367-1373: startSession is already serialized with withThreadLock
but stopSession and stopAll mutate the same sessions map without locking; wrap
the mutating logic in stopSession and stopAll with the same
withThreadLock(threadId, ...) pattern (use the same threadId used by
startSession or derive it per-session) so both stopping a session and stopping
all sessions acquire the semaphore from getThreadSemaphore before touching the
sessions map, ensuring races between startSession and stopSession/stopAll are
serialized; update stopAll to iterate/stop under the per-thread lock or acquire
each thread's semaphore when removing entries from sessions.
In `@apps/server/src/provider/Layers/ProviderService.test.ts`:
- Around line 356-362: The test currently constructs routing once via
makeProviderServiceLayer() at module scope so stateful components
(SqlitePersistenceMemory, ProviderSessionDirectory, ProviderService) leak across
tests; instead, move the call to makeProviderServiceLayer() into each test's
setup (e.g., beforeEach) so a fresh layer is created per test and drop relying
solely on .reset() for adapters; apply the same change for the fanout and
validation fixtures (replace module-scoped
makeProviderServiceLayer()/fanout/validation initialization with per-test
creation in beforeEach and then use routing/fanout/validation from that fresh
instance).
---
Nitpick comments:
In `@apps/server/src/provider/acp/AcpSessionRuntime.ts`:
- Around line 566-634: handleSessionUpdate currently calls
parseSessionUpdateEvent without guarding against exceptions or malformed shapes;
wrap the parse step in an Effect try/catch (e.g., Effect.tryCatch/Effect.sync
with Effect.catchAll) and validate the returned shape (ensure parsed.events is
an array and parsed.modeId has expected form) before proceeding. On error or
invalid shape, emit a deterministic fallback (e.g., Queue.offer(queue, { _tag:
"SessionUpdateMalformed", sessionId: params.sessionId, error: <error message>
})) or early-return after logging, so subsequent refs/ensures
(ensureActiveAssistantSegment, Ref.modify on toolCallsRef, etc.) are not
executed with bad data; update handleSessionUpdate, parseSessionUpdateEvent
usage, and the caller-side queue consumers to handle the new malformed event
tag.
In `@apps/server/src/provider/Layers/CodexProvider.ts`:
- Around line 181-197: The pagination in requestAllCodexModels can loop
indefinitely; add a max-pages safeguard (e.g., const MAX_PAGES = N) and count
iterations inside the do/while, calling break or throwing an error when the page
count exceeds MAX_PAGES to prevent unbounded loops; update the logic around
cursor, client.request("model/list", ...) and
parseCodexModelListResponse(response) to respect this limit and surface a clear
error/log message if the limit is reached so callers know pagination was
truncated.
In `@apps/server/src/provider/Layers/OpenCodeProvider.ts`:
- Around line 155-157: The return expression in inferDefaultAgent contains a
redundant "?? undefined"; remove the trailing "?? undefined" and simplify the
return to use the existing optional chaining fallbacks (i.e., return
agents.find((agent) => agent.name === "build")?.name ?? agents[0]?.name). Update
the function inferDefaultAgent accordingly so it relies on ?.name yielding
undefined when missing instead of explicitly coalescing to undefined.
- Around line 174-189: The code builds variantOptions and agentOptions using
Object.assign with conditional isDefault objects; replace these with spread
syntax for readability: construct each option as { value: ..., label:
titleCaseSlug(...) , ...(defaultVariant === value && { isDefault: true }) } for
variantOptions and similarly for agentOptions using primaryAgents, defaultAgent
and inferDefaultAgent to determine the conditional property; update the map
callbacks for variantValues and primaryAgents to use spread-based object
literals instead of Object.assign.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 836d8e99-2227-480b-b1ca-357e8f7c1247
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
apps/server/package.jsonapps/server/src/provider/Layers/ClaudeAdapter.test.tsapps/server/src/provider/Layers/ClaudeAdapter.tsapps/server/src/provider/Layers/CodexAdapter.test.tsapps/server/src/provider/Layers/CodexAdapter.tsapps/server/src/provider/Layers/CodexProvider.tsapps/server/src/provider/Layers/CodexSessionRuntime.tsapps/server/src/provider/Layers/CursorAdapter.test.tsapps/server/src/provider/Layers/CursorAdapter.tsapps/server/src/provider/Layers/CursorProvider.test.tsapps/server/src/provider/Layers/CursorProvider.tsapps/server/src/provider/Layers/OpenCodeAdapter.tsapps/server/src/provider/Layers/OpenCodeProvider.tsapps/server/src/provider/Layers/ProviderRegistry.test.tsapps/server/src/provider/Layers/ProviderRegistry.tsapps/server/src/provider/Layers/ProviderService.test.tsapps/server/src/provider/Layers/ProviderService.tsapps/server/src/provider/acp/AcpAdapterSupport.test.tsapps/server/src/provider/acp/AcpAdapterSupport.tsapps/server/src/provider/acp/AcpSessionRuntime.tsapps/server/src/provider/acp/CursorAcpCliProbe.test.tsapps/server/src/provider/acp/CursorAcpExtension.tsapps/server/src/provider/cliVersion.test.tsapps/server/src/provider/cliVersion.ts
✅ Files skipped from review due to trivial changes (2)
- apps/server/package.json
- apps/server/src/provider/cliVersion.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/server/src/provider/acp/AcpAdapterSupport.ts
- apps/server/src/provider/cliVersion.ts
- apps/server/src/provider/Layers/CodexAdapter.test.ts
- apps/server/src/provider/acp/AcpAdapterSupport.test.ts
- apps/server/src/provider/Layers/OpenCodeAdapter.ts
- apps/server/src/provider/Layers/CursorAdapter.test.ts
- apps/server/src/provider/Layers/ProviderRegistry.ts
- add a mock ACP agent script for protocol/session testing - lock Claude thread session startup to prevent overlap
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
apps/server/src/provider/Layers/OpenCodeProvider.ts (1)
381-387: 🛠️ Refactor suggestion | 🟠 MajorGate
directorybehind external server check to prevent potential workspace path leakage.Line 383 passes
input.cwdunconditionally, whileserverPasswordis correctly gated behindisExternalServer. This asymmetry is suspicious—if the SDK serializesdirectoryin requests to external servers, the local workspace path could be exposed. The same pattern appears inOpenCodeAdapter.ts(line 1030). For consistency and to prevent potential information leaks, omitdirectorywhen connecting to external servers:...(isExternalServer ? {} : { directory: input.cwd }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/OpenCodeProvider.ts` around lines 381 - 387, The call to openCodeRuntime.createOpenCodeSdkClient in OpenCodeProvider passes input.cwd unconditionally, risking workspace path leakage; change it to only include directory when not connecting to an external server (i.e., include directory when !isExternalServer), mirroring how serverPassword is gated, and apply the same fix for the corresponding call in OpenCodeAdapter so both places use the pattern: include directory only for internal servers.
🧹 Nitpick comments (1)
apps/server/src/provider/Layers/CursorAdapter.test.ts (1)
184-185: Drop the leftover debug print.This
console.logwill add noise to CI/test output without adding signal once the spec is stable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/CursorAdapter.test.ts` around lines 184 - 185, Remove the leftover debug print statement after calling adapter.stopSession in the CursorAdapter.test (the line with console.log("debug:first:stopSession")); keep the yield* adapter.stopSession(threadId); call and any surrounding assertions/cleanup, and run tests to ensure no other debug prints remain; reference the adapter.stopSession invocation and the console.log string to locate the line to delete.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/scripts/acp-mock-agent.ts`:
- Around line 135-156: The mocked Claude config always returns a hardcoded
"high" for the reasoning select, causing updateConfig("reasoning", ...) to write
state.reasoning but the UI to still show "high"; change the select's
currentValue in the branch that builds base for model "claude-opus-4-6" to use
state.reasoning (or fallback appropriately) instead of the literal "high" and
ensure the select's id "reasoning" matches the key updateConfig uses so updates
are reflected in the mock state and returned configuration.
In `@apps/server/src/provider/Layers/CodexProvider.ts`:
- Around line 127-143: The code currently assigns fallbackCapabilities from the
first built-in model and applies it to every custom model (variable
fallbackCapabilities used when building customEntries), which can surface
unsupported controls; instead ensure custom models start capability-less by
removing the derived fallback and setting capabilities to null for each custom
entry (update the block that constructs customEntries inside CodexProvider to
stop using fallbackCapabilities and set capabilities: null for each pushed
entry).
In `@apps/server/src/provider/Layers/CursorAdapter.ts`:
- Around line 824-883: The code calls applyRequestedSessionConfiguration(...)
before validating the turn and resolving attachments, which can mutate ctx.acp
even if the turn is later rejected; change the flow so you first build and
validate promptParts (trim input, iterate input.attachments using
resolveAttachmentPath and fileSystem.readFile with the same
ProviderAdapterRequestError mapping) and if promptParts is non-empty proceed to
call applyRequestedSessionConfiguration(...) and continue; keep the same error
returns (ProviderAdapterRequestError / ProviderAdapterValidationError) and
unique symbols (applyRequestedSessionConfiguration, resolveAttachmentPath,
promptParts, ProviderAdapterValidationError) so the session config is only
applied after validation succeeds.
- Around line 300-330: The threadLocksRef Map currently stores a Semaphore per
threadId forever; update getThreadSemaphore and withThreadLock to use a
ref-counted entry (e.g., store { semaphore, count } or a tuple) so new entries
start with count=1, increment the count when a waiter acquires the semaphore,
and decrement and remove the map entry when the waiter finishes (ensure the
decrement/removal happens after permit is released). Specifically change
SynchronizedRef.modifyEffect in getThreadSemaphore to create or increment a
ref-counted entry instead of just the Semaphore, and adjust withThreadLock to
decrement/remove the entry when semaphore.withPermit completes (or on failure)
to avoid unbounded growth of threadLocksRef. Ensure Semaphore.make is still used
to create the semaphore and that removal happens inside the SynchronizedRef to
avoid races.
- Around line 900-918: The catch block chained on ctx.acp.prompt currently
resets ctx.activeTurnId and ctx.session but rethrows the caught
ProviderAdapterError by throwing inside Effect.sync, which turns the typed error
into a defect; instead, perform the state cleanup first then re-raise the same
ProviderAdapterError into the typed error channel by returning
Effect.fail(error) (not throw) so callers of sendTurn() receive the error;
update the Effect.catch handler that references ProviderAdapterError to call the
state resets and then Effect.fail(error) (preserving the original error from
mapAcpToAdapterError/PROVIDER/input.threadId).
In `@apps/server/src/provider/Layers/OpenCodeProvider.ts`:
- Around line 79-91: The probe message in OpenCodeProvider (inside the if block
that checks error strings) currently echoes input.serverUrl into the returned
message, risking leakage of credentials; change the message construction so it
does not include the raw input.serverUrl—parse input.serverUrl to derive a
sanitized host/origin (using URL parsing) and insert that, or if parsing fails
use a generic label like "configured OpenCode server"; update the code that
returns the object (the returned message near input.serverUrl) to use the
sanitized value or fallback instead of the raw URL.
In `@apps/server/src/provider/Layers/ProviderService.ts`:
- Around line 306-330: The current stale-cleanup uses adapter.hasSession(...)
before adapter.stopSession(...), causing a TOCTOU race that can surface
ProviderAdapterSessionNotFoundError and abort subsequent startSession(); remove
the pre-check and always call adapter.stopSession(input.threadId) (or if you
prefer to keep the check, update the Effect.catchCause after adapter.stopSession
to treat ProviderAdapterSessionNotFoundError as a success and not rethrow).
Update the code around adapter.hasSession and adapter.stopSession in
ProviderService.ts so stopSession is invoked directly and its catch handler
swallows ProviderAdapterSessionNotFoundError (log it as info/warning) instead of
calling Effect.failCause, ensuring startSession() is not aborted by a
stale-not-found error.
---
Duplicate comments:
In `@apps/server/src/provider/Layers/OpenCodeProvider.ts`:
- Around line 381-387: The call to openCodeRuntime.createOpenCodeSdkClient in
OpenCodeProvider passes input.cwd unconditionally, risking workspace path
leakage; change it to only include directory when not connecting to an external
server (i.e., include directory when !isExternalServer), mirroring how
serverPassword is gated, and apply the same fix for the corresponding call in
OpenCodeAdapter so both places use the pattern: include directory only for
internal servers.
---
Nitpick comments:
In `@apps/server/src/provider/Layers/CursorAdapter.test.ts`:
- Around line 184-185: Remove the leftover debug print statement after calling
adapter.stopSession in the CursorAdapter.test (the line with
console.log("debug:first:stopSession")); keep the yield*
adapter.stopSession(threadId); call and any surrounding assertions/cleanup, and
run tests to ensure no other debug prints remain; reference the
adapter.stopSession invocation and the console.log string to locate the line to
delete.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 821c98eb-d683-4b26-abb5-754110dd8726
📒 Files selected for processing (12)
apps/server/scripts/acp-mock-agent.tsapps/server/src/provider/Layers/ClaudeAdapter.tsapps/server/src/provider/Layers/CodexAdapter.tsapps/server/src/provider/Layers/CodexProvider.tsapps/server/src/provider/Layers/CursorAdapter.test.tsapps/server/src/provider/Layers/CursorAdapter.tsapps/server/src/provider/Layers/OpenCodeProvider.tsapps/server/src/provider/Layers/ProviderService.test.tsapps/server/src/provider/Layers/ProviderService.tsapps/server/src/provider/acp/AcpJsonRpcConnection.test.tsapps/server/src/provider/acp/AcpRuntimeModel.tsapps/server/src/provider/acp/AcpSessionRuntime.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/server/src/provider/Layers/ClaudeAdapter.ts
Comment on lines
+900
to
+918
| const result = yield* ctx.acp | ||
| .prompt({ | ||
| prompt: promptParts, | ||
| }) | ||
| .pipe( | ||
| Effect.mapError((error) => | ||
| mapAcpToAdapterError(PROVIDER, input.threadId, "session/prompt", error), | ||
| ), | ||
| Effect.catch((error: ProviderAdapterError) => | ||
| Effect.sync(() => { | ||
| ctx.activeTurnId = undefined; | ||
| ctx.session = { | ||
| ...ctx.session, | ||
| activeTurnId: undefined, | ||
| updatedAt: new Date().toISOString(), | ||
| }; | ||
| throw error; | ||
| }), | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd apps/server && cat -n src/provider/Layers/CursorAdapter.ts | sed -n '895,925p'Repository: Ffinnis/capycode
Length of output: 1310
🏁 Script executed:
wc -l apps/server/src/provider/Layers/CursorAdapter.tsRepository: Ffinnis/capycode
Length of output: 113
🏁 Script executed:
find . -name "package.json" -path "*/apps/server/*" -exec cat {} \; | grep -A 5 -B 5 "effect"Repository: Ffinnis/capycode
Length of output: 1098
🏁 Script executed:
rg "Effect.catch\|Effect.catchAll" apps/server/src --max-count 10 -B 2 -A 5Repository: Ffinnis/capycode
Length of output: 42
🏁 Script executed:
rg "ProviderAdapterError" apps/server/src/provider --max-count 10 -B 1 -A 1Repository: Ffinnis/capycode
Length of output: 9400
🏁 Script executed:
rg "Effect.fail" apps/server/src/provider --max-count 10 -B 2 -A 2Repository: Ffinnis/capycode
Length of output: 13490
🏁 Script executed:
rg "Effect.andThen" apps/server/src/provider --max-count 10 -B 1 -A 1Repository: Ffinnis/capycode
Length of output: 2995
🏁 Script executed:
rg "Effect\.sync.*throw" apps/server/src --max-count 5 -B 3 -A 3Repository: Ffinnis/capycode
Length of output: 42
🌐 Web query:
Effect.ts Effect.sync throw inside error handling defect vs fail
💡 Result:
In Effect.ts, Effect.sync creates an Effect< A, never, never > that executes a synchronous thunk expected not to throw. If the thunk throws, it produces a defect (unrecoverable error, Cause.Die), not a typed failure in the error channel. Effect.fail creates a typed, recoverable failure in the error channel (Effect< never, E, never >). Standard error handlers like Effect.catchAll or Effect.catchTag only catch typed failures from the error channel (Cause.Fail). They do not catch defects from Effect.sync throws. To catch defects, use Effect.catchAllCause((cause) => { if (Cause.isDie(cause)) { ... } }) or Effect.catchAllDefect. Throwing inside an Effect.sync thunk during error handling (e.g., inside a catchAll on another effect) bypasses typed error handling, creating a defect instead of a recoverable fail. This is a defect in design/logic, as sync assumes no-throw, leading to unrecoverable failure rather than propagating as typed error. Use Effect.try for potentially throwing sync code to capture throws as typed UnknownException or custom E. Sources confirm: Effect.sync docs repeatedly state "if it does, the error will be treated as a defect" {2,3,10}; defects vs failures {6,11}; catchAllCause for defects {11}.
Citations:
- 1: https://effect.website/docs/error-management/expected-errors/
- 2: https://effect.website/docs/getting-started/creating-effects/
- 3: https://effect.website/docs/guides/essentials/running-effects
- 4: https://www.mintlify.com/Effect-TS/effect/core/creating-effects
- 5: https://effect.website/docs/error-management/two-error-types/
- 6: https://effect.website/docs/getting-started/running-effects/
- 7: https://www.mintlify.com/Effect-TS/effect/core/error-handling
- 8: https://effect.website/docs/guides/error-management/expected-errors
- 9: https://fredcollman.co.uk/courses/daily-effect/effect.try/
🌐 Web query:
Effect.ts catch vs catchAll error channel throw in Effect.sync
💡 Result:
In Effect.ts, catchAll handles all errors (failures) in the Effect's error channel (E type parameter), providing a fallback Effect. It recovers from expected, typed errors like those from Effect.fail. catch is not a standard Effect method in the current API (v2+). Older versions or tutorials may reference it, but official docs emphasize catchAll, catchTag (for specific tagged errors), and catchTags. Use catchAllCause for defects too. throw in Effect.sync(thunk) creates a "defect" (unexpected error, Cause.Die), not a typed failure in the error channel. Defects indicate programmer errors and aren't caught by catchAll (which targets failures). Use Effect.try(thunk) for safe sync code that might throw, converting to typed error. catchAllCause handles both failures and defects. Example: import { Effect } from "effect" const safeSync = Effect.try( => { if (Math.random > 0.5) throw new Error("boom") return "success" }) // Handles typed errors from try/throw const handled = safeSync.pipe(Effect.catchAll(e => Effect.succeed("recovered"))) const unsafeSync = Effect.sync( => { throw new Error("defect") }) // Throws defect, use catchAllCause((cause) => ...) Official docs prioritize catchAll for error channel; defects from sync/throw need cause handling. No direct catch vs catchAll distinction found; catchAll is primary.
Citations:
- 1: https://effect.website/docs/error-management/expected-errors/
- 2: https://www.mintlify.com/Effect-TS/effect/core/error-handling
- 3: https://ybogomolov.me/02-effect-handling-errors
- 4: https://effect.website/docs/error-management/unexpected-errors/
- 5: https://effect.website/docs/getting-started/creating-effects/
- 6: https://effect-ts.github.io/effect/effect/Effect.ts.html
🏁 Script executed:
rg "Effect\.catch\(" apps/server/src/provider --max-count 10 -B 1 -A 1Repository: Ffinnis/capycode
Length of output: 938
🏁 Script executed:
rg "export.*catch|function.*catch" apps/server/src --max-count 5 -B 1 -A 2Repository: Ffinnis/capycode
Length of output: 42
🏁 Script executed:
grep -r "\.catch\s*(" apps/server/src/provider/Layers/CursorAdapter.ts | head -5Repository: Ffinnis/capycode
Length of output: 119
🏁 Script executed:
rg "import.*catch|from.*effect" apps/server/src/provider/Layers/CursorAdapter.ts | head -20Repository: Ffinnis/capycode
Length of output: 198
🏁 Script executed:
rg "catch\s*:" apps/server/src --type typescript --max-count 5 -B 2 -A 2Repository: Ffinnis/capycode
Length of output: 94
🏁 Script executed:
cd apps/server && cat -n src/provider/Layers/CursorAdapter.ts | head -50Repository: Ffinnis/capycode
Length of output: 1541
🏁 Script executed:
cat -n apps/server/src/provider/Layers/CursorAdapter.ts | sed -n '1,100p' | grep -E "import|from"Repository: Ffinnis/capycode
Length of output: 1385
🏁 Script executed:
rg "Effect\.catch\(" apps/server/src/provider/Layers --max-count 15 -A 3 | head -60Repository: Ffinnis/capycode
Length of output: 1156
🏁 Script executed:
rg "catch.*=|catch.*:" apps/server/src/provider --max-count 10 -B 3 -A 1Repository: Ffinnis/capycode
Length of output: 8633
🏁 Script executed:
fd "package.json" -path "*/effect/*" | xargs cat 2>/dev/null | grep -A 5 "version" | head -20Repository: Ffinnis/capycode
Length of output: 228
🏁 Script executed:
cat apps/server/pnpm-lock.yaml | grep -A 5 "effect:" | head -20Repository: Ffinnis/capycode
Length of output: 118
🏁 Script executed:
fd "Makefile|pnpm-lock|yarn.lock|package-lock" apps/server -o -type f | head -5Repository: Ffinnis/capycode
Length of output: 176
🏁 Script executed:
cat apps/server/package.json | grep -A 10 "dependencies"Repository: Ffinnis/capycode
Length of output: 456
🏁 Script executed:
rg "export.*Effect" apps/server/src --max-count 5 -B 1 -A 2Repository: Ffinnis/capycode
Length of output: 26274
🏁 Script executed:
python3 << 'EOF'
import subprocess
result = subprocess.run(
["rg", "Effect\.catch", "apps/server/src/provider/Layers/CursorAdapter.ts", "-B 5", "-A 10"],
capture_output=True,
text=True,
cwd="/root"
)
print(result.stdout)
EOFRepository: Ffinnis/capycode
Length of output: 613
🏁 Script executed:
rg "Effect\.catch\(" apps/server/src/provider/Layers/ProviderService.ts -B 5 -A 5Repository: Ffinnis/capycode
Length of output: 274
🏁 Script executed:
rg "catch\s*\(" node_modules/effect/dist --max-count 3 2>/dev/null || echo "checking effect module"Repository: Ffinnis/capycode
Length of output: 82
🏁 Script executed:
python3 << 'EOF'
import json
import subprocess
# Read package.json to find effect version
result = subprocess.run(
["cat", "apps/server/package.json"],
capture_output=True,
text=True,
cwd="."
)
# Extract effect version
lines = result.stdout.split('\n')
for i, line in enumerate(lines):
if '"effect"' in line:
print("Effect dependency found:")
print(line)
if i+1 < len(lines):
print(lines[i+1])
EOFRepository: Ffinnis/capycode
Length of output: 143
🏁 Script executed:
fd "pnpm-workspace.yaml" -o "pnpm-lock.yaml" -o "package-lock.json" | head -5Repository: Ffinnis/capycode
Length of output: 207
🏁 Script executed:
rg "export.*catch|catch.*export" --max-count 10 | head -20Repository: Ffinnis/capycode
Length of output: 42
🏁 Script executed:
cat -n apps/server/src/provider/Layers/ProviderService.ts | sed -n '1,50p'Repository: Ffinnis/capycode
Length of output: 2153
🏁 Script executed:
rg "^\s*catch\s*[:=]|function\s+catch|export.*catch" apps/server/src --max-count 20 -B 2 -A 2Repository: Ffinnis/capycode
Length of output: 22035
🏁 Script executed:
fd "index.ts" apps/server/src -x grep -l "catch" {} \;Repository: Ffinnis/capycode
Length of output: 42
🏁 Script executed:
rg "\.catch\s*=" apps/server/src/provider --max-count 5 -B 3 -A 3Repository: Ffinnis/capycode
Length of output: 42
🏁 Script executed:
cat -n apps/server/src/provider/Layers/ProviderSessionReaper.ts | sed -n '1,100p'Repository: Ffinnis/capycode
Length of output: 4196
Preserve the typed error channel after cleanup.
This handler catches ProviderAdapterError but throws it from Effect.sync(), which converts the typed error into a defect (uncaught exception). Callers of sendTurn() will miss the error entirely. Reset state first, then restore the error to the typed error channel using Effect.fail().
Fix
const result = yield* ctx.acp
.prompt({
prompt: promptParts,
})
.pipe(
Effect.mapError((error) =>
mapAcpToAdapterError(PROVIDER, input.threadId, "session/prompt", error),
),
- Effect.catch((error: ProviderAdapterError) =>
- Effect.sync(() => {
+ Effect.catchAll((error: ProviderAdapterError) =>
+ Effect.sync(() => {
ctx.activeTurnId = undefined;
ctx.session = {
...ctx.session,
activeTurnId: undefined,
updatedAt: new Date().toISOString(),
};
- throw error;
- }),
+ }).pipe(Effect.andThen(Effect.fail(error))),
),
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/provider/Layers/CursorAdapter.ts` around lines 900 - 918, The
catch block chained on ctx.acp.prompt currently resets ctx.activeTurnId and
ctx.session but rethrows the caught ProviderAdapterError by throwing inside
Effect.sync, which turns the typed error into a defect; instead, perform the
state cleanup first then re-raise the same ProviderAdapterError into the typed
error channel by returning Effect.fail(error) (not throw) so callers of
sendTurn() receive the error; update the Effect.catch handler that references
ProviderAdapterError to call the state resets and then Effect.fail(error)
(preserving the original error from
mapAcpToAdapterError/PROVIDER/input.threadId).
Comment on lines
+306
to
+330
| yield* Effect.forEach( | ||
| adapters, | ||
| (adapter) => | ||
| adapter.provider === input.currentProvider | ||
| ? Effect.void | ||
| : Effect.gen(function* () { | ||
| const hasSession = yield* adapter.hasSession(input.threadId); | ||
| if (!hasSession) { | ||
| return; | ||
| } | ||
|
|
||
| yield* adapter.stopSession(input.threadId).pipe( | ||
| Effect.tap(() => | ||
| analytics.record("provider.session.stopped", { | ||
| provider: adapter.provider, | ||
| }), | ||
| ), | ||
| Effect.catchCause((cause) => | ||
| Effect.logWarning("provider.session.stop-stale-failed", { | ||
| threadId: input.threadId, | ||
| provider: adapter.provider, | ||
| cause, | ||
| }).pipe(Effect.andThen(Effect.failCause(cause))), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Avoid the hasSession() / stopSession() race during stale cleanup.
This is a TOCTOU check. In the current PR, apps/server/src/provider/Layers/CursorAdapter.ts:1021-1043 can still return ProviderAdapterSessionNotFoundError if another path stops the stale session after hasSession() succeeds, and that failure aborts the new startSession() even though the stale session is already gone. Treat the not-found case as success, or call stopSession() directly without the pre-check.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/provider/Layers/ProviderService.ts` around lines 306 - 330,
The current stale-cleanup uses adapter.hasSession(...) before
adapter.stopSession(...), causing a TOCTOU race that can surface
ProviderAdapterSessionNotFoundError and abort subsequent startSession(); remove
the pre-check and always call adapter.stopSession(input.threadId) (or if you
prefer to keep the check, update the Effect.catchCause after adapter.stopSession
to treat ProviderAdapterSessionNotFoundError as a success and not rethrow).
Update the code around adapter.hasSession and adapter.stopSession in
ProviderService.ts so stopSession is invoked directly and its catch handler
swallows ProviderAdapterSessionNotFoundError (log it as info/warning) instead of
calling Effect.failCause, ensuring startSession() is not aborted by a
stale-not-found error.
- Avoid stale-session stop checks and log missing sessions cleanly - Handle external OpenCode servers without leaking local directories - Normalize model aliases and update release smoke package coverage
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/server/src/provider/Layers/CodexProvider.ts (1)
119-143: Extract shared custom-model normalization to one helper.
appendCustomCodexModelsandemptyCodexModelsFromSettingsboth trim/dedupe custom slugs independently. A single helper will prevent drift and keep custom model handling consistent.Also applies to: 273-282
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/CodexProvider.ts` around lines 119 - 143, The code duplicates trimming/deduping logic for custom model slugs in appendCustomCodexModels and emptyCodexModelsFromSettings; extract that into a single helper (e.g., normalizeCustomModelSlugs or parseCustomModelSlugs) that takes ReadonlyArray<string> and returns a deduplicated, trimmed array of slugs, then update appendCustomCodexModels to call this helper (use the returned slugs to build customEntries and update the seen set logic) and replace the similar logic in emptyCodexModelsFromSettings to call the same helper so both functions share consistent normalization behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/provider/Layers/CodexProvider.ts`:
- Around line 395-412: The provider probe currently leaks upstream error.message
into the client-facing message (inside the probe.message returned in
buildServerProvider when Result.isFailure(probeResult)), so replace the
interpolated error.message with a generic, sanitized message like "Codex
app-server provider probe failed; see server logs for details" and ensure the
full error is logged server-side (e.g., via the existing logger or
processLogger) when handling the failure of probeResult; keep the
installed-branch and CLI-missing branch behavior but move any sensitive details
out of probe.message while logging error and stack for debugging in the error
handling code around probeResult and the CodexErrors.CodexAppServerSpawnError
check.
---
Nitpick comments:
In `@apps/server/src/provider/Layers/CodexProvider.ts`:
- Around line 119-143: The code duplicates trimming/deduping logic for custom
model slugs in appendCustomCodexModels and emptyCodexModelsFromSettings; extract
that into a single helper (e.g., normalizeCustomModelSlugs or
parseCustomModelSlugs) that takes ReadonlyArray<string> and returns a
deduplicated, trimmed array of slugs, then update appendCustomCodexModels to
call this helper (use the returned slugs to build customEntries and update the
seen set logic) and replace the similar logic in emptyCodexModelsFromSettings to
call the same helper so both functions share consistent normalization behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c456d667-974f-4850-b0b3-5b64f403d4e3
📒 Files selected for processing (10)
apps/server/scripts/acp-mock-agent.tsapps/server/src/provider/Layers/CodexProvider.tsapps/server/src/provider/Layers/CursorAdapter.test.tsapps/server/src/provider/Layers/CursorAdapter.tsapps/server/src/provider/Layers/OpenCodeAdapter.tsapps/server/src/provider/Layers/OpenCodeProvider.test.tsapps/server/src/provider/Layers/OpenCodeProvider.tsapps/server/src/provider/Layers/ProviderService.tspackages/contracts/src/model.tsscripts/release-smoke.ts
✅ Files skipped from review due to trivial changes (2)
- apps/server/src/provider/Layers/OpenCodeProvider.test.ts
- apps/server/scripts/acp-mock-agent.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/server/src/provider/Layers/ProviderService.ts
- apps/server/src/provider/Layers/CursorAdapter.test.ts
- apps/server/src/provider/Layers/CursorAdapter.ts
- Emit structured error details when the Codex probe fails - Replace the user-facing message with a log reference
- Reflect current provider list and badges in session logic tests
- Guard desktop bridge calls by method availability - Surface a clear error when RPC methods are unavailable - Reuse the client settings storage key in tests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Summary by CodeRabbit
New Features
Improvements
Tests