refactor(chat): migrate main-process chat to Effect domain services#2475
refactor(chat): migrate main-process chat to Effect domain services#2475samuv wants to merge 11 commits into
Conversation
Add the stable Effect package so the main-process chat subsystem can adopt typed errors, Layers, and a managed runtime without pulling in unstable @effect/platform APIs. Co-authored-by: Cursor <cursoragent@cursor.com>
Reorganize main-process chat into runtime, threads, settings, agents, providers, pricing, mcp, and streaming domains backed by Effect.Service Layers and one app-scoped ManagedRuntime. Keep preload/renderer IPC contracts stable via thin Promise facades, make StreamRegistry the sole message-persistence owner, and keep WebContents cleanup safe after runtime disposal on quit. Co-authored-by: Cursor <cursoragent@cursor.com>
Register a chat:runtime:health probe so renderers and diagnostics can detect ManagedRuntime readiness, and keep chat handlers from calling into an uninitialized runtime after a failed bootstrap. Co-authored-by: Cursor <cursoragent@cursor.com>
Install shared Electron/Sentry mocks and ManagedRuntime hooks so chat characterization suites exercise domain services through the new compatibility facades instead of module-global mocks. Co-authored-by: Cursor <cursoragent@cursor.com>
Delete empty barrel files and stop exporting internal helpers that are only used within domain modules, while wiring StreamConflictError and runtime health status into their real call sites. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
Migrates the Electron main-process chat subsystem to an Effect-based architecture (ManagedRuntime + domain services) while aiming to keep existing IPC contracts stable, and adds a runtime health probe over IPC.
Changes:
- Introduces an app-scoped Effect
ManagedRuntimewith typed domain errors, adapters, logging, lifecycle hooks, and achat:runtime:healthIPC probe. - Refactors chat functionality into Effect services/repositories (threads, settings, agents, providers, MCP, pricing, streaming) with compatibility facades.
- Updates unit tests to run against the new chat runtime and updated module boundaries.
Reviewed changes
Copilot reviewed 61 out of 62 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds effect (and transitive deps) lock entries. |
| package.json | Adds effect dependency. |
| main/src/ipc-handlers/chat/threads.ts | Updates imports to new chat module boundaries. |
| main/src/ipc-handlers/chat/streaming.ts | Points IPC streaming handler to new streaming entrypoint; hardens purge on destroyed WebContents. |
| main/src/ipc-handlers/chat/runtime-health.ts | Adds IPC health probe + helper wrapper for runtime readiness. |
| main/src/ipc-handlers/chat/mcp-tools.ts | Updates MCP tools IPC handler imports to new MCP module. |
| main/src/ipc-handlers/chat/index.ts | Registers new runtime health IPC handler. |
| main/src/ipc-handlers/chat/tests/runtime-health.test.ts | Adds coverage for the new chat:runtime:health IPC handler. |
| main/src/ipc-handlers/chat/tests/index.test.ts | Updates handler registration test to include runtime health. |
| main/src/db/reconcile-from-store.ts | Switches legacy store access to lazy getters and updates thread type import. |
| main/src/chat/threads/types.ts | Introduces thread types split out from legacy storage module. |
| main/src/chat/threads/threads-service.ts | New Effect ThreadsService encapsulating thread operations with tagged errors. |
| main/src/chat/threads/threads-repository.ts | New Effect repository wrapper for DB thread readers/writers. |
| main/src/chat/threads-storage.ts | Refactors legacy threads facade to call ThreadsService via runtime adapters. |
| main/src/chat/thread-settings-storage.ts | Refactors per-thread settings facade to ThreadSettingsService via runtime adapters. |
| main/src/chat/thread-integration.ts | Refactors thread integration facade to use ThreadsService. |
| main/src/chat/streaming/title-service.ts | Adds Effect TitleService for title generation. |
| main/src/chat/streaming/chat-stream-service.ts | Adds Effect ChatStreamService wrapper around streaming implementation. |
| main/src/chat/streaming/chat-stream-service-impl.ts | Extracts the core streaming implementation with injected dependencies. |
| main/src/chat/streaming.ts | Converts old streaming module to a thin runtime-backed facade. |
| main/src/chat/settings/thread-settings-service.ts | Adds Effect ThreadSettingsService for per-thread settings persistence. |
| main/src/chat/settings/settings-service.ts | Adds Effect SettingsService + SettingsRepository for chat settings and MCP reconciliation. |
| main/src/chat/settings/legacy-store-service.ts | Adds Effect wrapper for accessing legacy stores. |
| main/src/chat/settings/legacy-store-access.ts | Adds lazy singleton accessors for legacy electron-store instances. |
| main/src/chat/settings-storage.ts | Refactors legacy settings facade to call SettingsService via runtime adapters. |
| main/src/chat/runtime/test-runtime.ts | Adds helpers/hooks for initializing/disposing runtime in tests. |
| main/src/chat/runtime/managed-runtime.ts | Defines chat ManagedRuntime and live Layer wiring. |
| main/src/chat/runtime/logging.ts | Adds Effect logger bridge to existing Electron logger. |
| main/src/chat/runtime/lifecycle.ts | Adds bootstrap/shutdown lifecycle utilities for the chat runtime. |
| main/src/chat/runtime/index.ts | Re-exports runtime modules for chat callers. |
| main/src/chat/runtime/health.ts | Adds runtime health state tracking. |
| main/src/chat/runtime/errors.ts | Defines tagged domain errors + safe user-facing error mapping. |
| main/src/chat/runtime/adapters.ts | Adds runtime execution helpers and IPC-safe result mapping. |
| main/src/chat/runtime/tests/setup.ts | Adds common mocks for runtime-centric tests. |
| main/src/chat/runtime/tests/health.test.ts | Adds tests for runtime health and adapter behavior. |
| main/src/chat/providers/providers-service.ts | Adds Effect ProvidersService for provider/model discovery. |
| main/src/chat/providers/providers-facade.ts | Adds facade functions backed by the Effect providers service. |
| main/src/chat/providers/providers-catalog.ts | Splits provider catalog into its own module. |
| main/src/chat/providers/index.ts | Adds providers barrel exports. |
| main/src/chat/providers.ts | Converts old providers module to re-export new provider modules. |
| main/src/chat/pricing/pricing-service.ts | Adds Effect PricingService with Ref-backed cache + disk persistence. |
| main/src/chat/pricing.ts | Converts pricing module into a runtime-backed facade. |
| main/src/chat/mcp/mcp-ui-metadata-cache.ts | Adds Ref-backed MCP UI metadata cache. |
| main/src/chat/mcp/mcp-service.ts | Adds Effect McpService wrapper that enforces MCP policies (fail-closed when enabled servers yield no tools). |
| main/src/chat/mcp/mcp-service-impl.ts | Extracts MCP implementation with UI metadata persistence + sanitization. |
| main/src/chat/mcp-tools.ts | Converts MCP tools module into runtime-backed facade + impl re-exports. |
| main/src/chat/index.ts | Narrows chat barrel exports to types/constants/helpers. |
| main/src/chat/generate-thread-title.ts | Converts title generation to use Effect TitleService + adapters. |
| main/src/chat/agents/registry.ts | Converts agent registry facade to runtime-backed AgentsService. |
| main/src/chat/agents/agents-service.ts | Adds Effect AgentsService for agent CRUD + seeding. |
| main/src/chat/agents/tests/registry.test.ts | Updates agent tests to use chat test runtime hooks. |
| main/src/chat/tests/threads-storage.test.ts | Updates threads tests to run against runtime-backed facade. |
| main/src/chat/tests/thread-settings-storage.test.ts | Updates per-thread settings tests for runtime-backed facade. |
| main/src/chat/tests/thread-integration.test.ts | Updates integration tests for runtime-backed thread service behavior. |
| main/src/chat/tests/streaming.test.ts | Updates streaming tests to target extracted impl with injected deps (and removes competing persistence). |
| main/src/chat/tests/pricing.test.ts | Updates pricing tests to run through runtime-backed pricing service. |
| main/src/chat/tests/mcp-tools.test.ts | Updates MCP tools tests for new runtime/service split and fail-closed policy. |
| main/src/chat/tests/generate-thread-title.test.ts | Updates title tests to use runtime-backed TitleService behavior/errors. |
| main/src/chat/tests/active-streams.test.ts | Updates active streams tests to match new persistence ownership and shutdown behavior. |
| main/src/app-events/when-ready.ts | Boots the Effect chat runtime during app initialization. |
| main/src/app-events/block-quit.ts | Disposes the Effect chat runtime during app shutdown. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Comments suppressed due to low confidence (1)
main/src/chat/threads-storage.ts:98
getThreadCountcan throw when the runtime isn't ready (viarunChatSync), but thechat:get-thread-countIPC handler doesn't catch. Return0when unavailable to avoid rejected IPC calls.
Use Fiber.join so chat-turn failures surface through runChatPromise, and return safe defaults from read facades when the ManagedRuntime is not ready so IPC handlers do not reject during startup or failed bootstrap. Co-authored-by: Cursor <cursoragent@cursor.com>
Make readiness and runtime retrieval atomic, type adapter programs against ChatServices, and tighten domain-error narrowing plus agent/settings edge cases. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
/build-test |
Build Artifacts for PR #2475
Download artifacts from workflow run Version:
|
kantord
left a comment
There was a problem hiding this comment.
Disclosure: This review comment (and the inline comments below) was fully or partially written by an AI agent, reading through the migration commits and cross-checking IPC handler wiring against the pattern Copilot already flagged. This is a partial pass, not a complete/exhaustive review of the PR — please don't treat an absence of a comment elsewhere as a clean bill of health.
What's covered below:
- The
runChatSyncOr/runChatPromiseOrfix in4bb714dddoesn't appear to have been applied consistently. It fixed exactly the 8 spots Copilot commented on (settings-storage.ts,threads-storage.ts), but the same bare-runChatSync/runChatPromisepattern — which throws/rejects when the chat runtime isn't ready — still exists inagents/registry.ts,active-streams.ts,mcp-tools.ts,providers/providers-facade.ts, andthread-integration.ts, and their IPC handlers (e.g.chat:agents:list,chat:stream:resume,chat:stream:cancel) have no try/catch either. Flagged individually below. - Two of three manual test-plan checkboxes are still unchecked ("cancel mid-stream, confirm cleanup and persistence" and "MCP partial discovery / fail-closed") — worth resolving before merge, especially since (1) touches exactly those code paths.
- A design suggestion on
stream-registry-service.ts— flagged inline.
Happy to expand this into a fuller pass if useful, just flagging what surfaced so far.
| toolParts: Map<string, ToolPartReplay> | ||
| } | ||
|
|
||
| const streams = new Map<string, ActiveStream>() |
There was a problem hiding this comment.
This is the file where Effect's structured-concurrency primitives would earn their cost the most, but the actual engine (runManagedStream, pumpChunks, this streams Map, the persistTimer debounce) is plain imperative code — Effect only wraps the edges (Effect.sync/Effect.tryPromise around lines 858-880).
Sibling services added in this same PR already lean on Ref for shared state (pricing/pricing-service.ts:67-68, mcp/mcp-ui-metadata-cache.ts:20-21) — applying that pattern here, plus forking the stream as a real Fiber with Effect.addFinalizer guarding flushPersist, would make "the debounced write always flushes on every exit path" a structural guarantee instead of three separate manual call sites (finalizeSuccess, finalizeError, shutdownAllActiveStreams) that each have to independently remember to call it. It's also exactly what the still-unchecked "cancel mid-stream, confirm persistence" test-plan item is trying to verify by hand — worth having the type system guarantee it instead.
There was a problem hiding this comment.
Acknowledged — soft-fail consistency is in b241f0bf. The stream-registry Effect/Ref/Fiber-finalizer redesign is larger; we’ll treat that as a follow-up plan rather than folding it into this soft-fail pass.
There was a problem hiding this comment.
Follow-up done in 3eed5105 (+ readability split in 17b133b5).
What we changed for this comment:
-
Persist-on-exit is structural —
runManagedStreamis nowEffect.scoped+forkScoped+Fiber.join, withEffect.addFinalizer(() => flushPersist(stream)), so cancel / error / success / scope teardown all flush. Explicit flushes on success/error remain for broadcast ordering; the finalizer is the safety net. -
Removed the module-level persist callback — no more
initStreamRegistryPersistence/persistMessagesImpl. The service closes overThreadsServiceand persists viagetManagedRuntime()?.runSyncExit(...). -
Service-owned registry state — streams map is created in
StreamRegistryService(with an EffectRef) and published through a small registry holder sopurgeSender/ shutdown can still run after runtime dispose without going through adapters. -
Coverage for the hand-test concern — Vitest now covers cancel mid-stream persistence, shutdown flushing a pending debounce, and MCP partial discovery (one server fails, another succeeds).
Still out of scope for this PR (as noted earlier): rewriting pumpChunks / replay maps into pure Effect streams, and putting WebContents subscriber sets into a fully Effect concurrent structure.
Extend runChatSyncOr/runChatPromiseOr to agents, streams, MCP, providers, thread-integration, and pricing reads so runtime-not-ready no longer rejects uncaught IPC handlers. Co-authored-by: Cursor <cursoragent@cursor.com>
Own stream state in StreamRegistryService with Ref-backed map, scoped Fiber + flush finalizer, and direct ThreadsService persist. Add Vitest coverage for cancel/shutdown flush and MCP partial discovery. Co-authored-by: Cursor <cursoragent@cursor.com>
Break the 900+ line stream-registry-service into types, state, broadcast, persist, replay, and engine modules with a thin Effect service facade. Co-authored-by: Cursor <cursoragent@cursor.com>
kantord
left a comment
There was a problem hiding this comment.
Disclosure: This review was fully or partially written by an AI agent, run as an 8-angle structured code review (line-by-line scan, removed-behavior audit, cross-file trace, reuse/simplification/efficiency/altitude/conventions passes) with each candidate independently re-verified against the actual code before being reported. This covers the 3 newest commits (b241f0bf, 3eed5105, 17b133b5) on top of the state our last review round covered — it is a focused pass, not a guaranteed-complete audit of the whole PR.
8 findings survived verification (6 CONFIRMED, 2 PLAUSIBLE-but-weakened), most severe first — flagged individually below. Two are worth resolving before merge:
- Shutdown-flush ordering bug (
runtime/lifecycle.ts) — defeats the exact guarantee the persistence-hardening commit added; the new regression test doesn't catch it because it bypasses the real shutdown path. - Thread-hydration data-loss path (
thread-integration.ts) — a transient runtime-unavailable window during thread load can lead to real conversation history being overwritten.
The rest are real but lower-impact (a dead Ref that doesn't provide the atomicity its own comment claims, a silent no-op on agent duplication, a latent registry bug that's currently unreachable in production, a duplicated/weaker error-classification helper, a replay-state edge case with no visible UI impact today, and a fallback-value inconsistency that's already mitigated by a downstream gate).
| export async function shutdownChatRuntime(): Promise<void> { | ||
| // Mark unavailable before dispose so IPC adapters never observe ready=true | ||
| // with a null ManagedRuntime mid-teardown. | ||
| markChatRuntimeUnavailable('runtime_disposing') |
There was a problem hiding this comment.
Confirmed bug. markChatRuntimeUnavailable('runtime_disposing') runs before shutdownAllActiveStreams(). shutdownAllActiveStreams() calls flushPersist(stream) for every active stream, which goes through the makePersistMessages closure calling getManagedRuntime() — which now returns null purely because health just flipped (the underlying ManagedRuntime instance is still alive; disposeChatRuntime() hasn't run yet). So every shutdown-time flush returns {success: false}, and the pending debounced snapshot (up to ~250ms of assistant text/tool state) is never persisted on quit-during-stream — silently defeating the exact guarantee 3eed5105 ("harden stream registry persistence lifecycle") was written to add.
The new regression test ("shutdownAllActiveStreams flushes a pending debounced snapshot") calls shutdownAllActiveStreams() directly without going through shutdownChatRuntime()/markChatRuntimeUnavailable() first, so runtime health stays 'ready' for the whole test and it passes despite this ordering bug. Suggest flushing pending streams before flipping health to unavailable, or making the flush path check getManagedRuntimeInstance() (the raw instance) rather than the health-gated getManagedRuntime().
| // Return stored messages directly | ||
| return thread.messages | ||
| export async function getThreadMessagesForTransport(threadId: string) { | ||
| return runChatPromiseOr( |
There was a problem hiding this comment.
Confirmed, real data-loss path. getThreadMessagesForTransport now falls back to [] via runChatPromiseOr instead of throwing when the runtime is unavailable. The renderer (chatThreadQueryOptions) forwards messages ?? [] with no success/failure discriminator, so a user opening an existing thread during a transient runtime-unavailable window (e.g. right after launch) hydrates as if the thread has zero messages, with no error shown. The write path is confirmed overwrite, not merge — ThreadsRepository.updateMessages does {...existing, messages, ...}, a full replace of the messages field. If the user sends a message from this falsely-empty state, the first persistence flush overwrites the thread's real stored history in SQLite with just the new exchange — permanent loss of prior conversation history, not just a transient UI glitch.
| }) | ||
| } | ||
|
|
||
| export function duplicateAgent(id: string): AgentConfig | null { |
There was a problem hiding this comment.
Confirmed, traced end-to-end. duplicateAgent now resolves to null (via runChatSyncOr(..., null)) instead of throwing when the runtime is unavailable. Both renderer call sites (agent-detail-page.tsx, agents-page.tsx) only branch on truthy copy (if (copy) { toast.success(...); navigate(...) }) with no else, and the surrounding try/catch never fires since nothing rejected. Clicking "Duplicate" while the runtime is down produces a completely silent no-op — no toast, no navigation, no error, nothing visible. Worth either giving this one a typed {success, error} result like deleteAgent already has, or adding an explicit empty-branch toast on the renderer side.
| const streamsMap = new Map<string, ActiveStream>() | ||
| // Keep an Effect Ref so the service owns the map; imperative helpers | ||
| // read the same Map instance via setActiveRegistry. | ||
| yield* Ref.make(streamsMap) |
There was a problem hiding this comment.
Confirmed still present. yield* Ref.make(streamsMap) creates a Ref whose result is discarded — never bound, never read via Ref.get, never written via Ref.set/Ref.update anywhere in the codebase. The commit message for 3eed5105 states the goal as "Own stream state in StreamRegistryService with Ref-backed map," and the comment right above this line says "Keep an Effect Ref so the service owns the map" — but all real reads/writes still go through the plain mutable Map shared via setActiveRegistry/getStreams(). This was flagged in our last review round and the reply said it was addressed in this commit, but the dead line survived the module split in 17b133b5 unchanged. Suggest either actually threading the Ref through getStreams()/setActiveRegistry, or dropping the Ref.make call and the misleading comment — a Ref nobody reads is worse than no Ref, since it signals a guarantee that doesn't exist.
| StreamRegistryRuntime, | ||
| } from './stream-registry-types' | ||
|
|
||
| let activeRegistry: StreamRegistryRuntime | null = null |
There was a problem hiding this comment.
Plausible, currently latent. The active-streams registry Map is recreated fresh on every StreamRegistryService construction (i.e. on chat-runtime restart) via setActiveRegistry, rather than being one process-lifetime-stable object like the pre-split code had. If the runtime is ever restarted mid-process and a user immediately reuses the same chatId, a still-finishing old stream's teardownStream() calls getStreams().delete(stream.chatId) — but getStreams() by then resolves to the swapped-in new registry, deleting the new stream's live entry and making it invisible to cancel/status IPC calls while it keeps running in the background.
Verified this is not reachable via any production code path today — initializeChatRuntime()/shutdownChatRuntime() are each called exactly once in the app lifecycle (app.whenReady() / blockQuit() immediately followed by app.quit()); only Vitest's beforeEach/afterEach hooks restart the runtime repeatedly. So this is a latent landmine, not an active bug — but it would activate the moment any feature (e.g. a settings-triggered runtime restart) reuses this shutdown/reinit pair without addressing the stale-teardown race.
| } | ||
| } | ||
|
|
||
| export function makePersistMessages( |
There was a problem hiding this comment.
Confirmed duplication, currently latent risk. makePersistMessages hand-rolls an Effect-Exit-to-{success,error} conversion (get runtime, runSyncExit, unwrap Cause.failureOption, duck-type check for a userMessage property) instead of reusing runChatToResultSync from runtime/adapters.ts, which already implements this exact conversion with proper domain-error detection (isDomainError checking _tag against DOMAIN_ERROR_TAGS). Notably, threads-storage.ts's updateThreadMessages wrapper already uses the shared helper correctly for the same underlying operation — this call site bypasses it and reimplements a weaker version by hand. Every error reachable here today is already a properly tagged domain error, so there's no live misclassification yet, but the duplicated logic will drift from the shared helper's classification rules over time. Worth switching to runChatToResultSync directly.
| } | ||
| return | ||
| } | ||
| case 'tool-input-error': { |
There was a problem hiding this comment.
Confirmed, low current impact. recordChunkForReplay's tool-input-error handling sets rawInput/errorText and state: 'output-error', but emitToolReplay has no branch that ever emits a tool-input-error-typed chunk or reads part.rawInput — a late-subscribing renderer for a tool call that failed during input parsing gets tool-input-start -> tool-output-error (skipping the input-error step entirely), losing the model's actual malformed arguments, whereas the live subscriber saw them. Traced the AI SDK's processUIMessageStream — it doesn't crash on this sequence, and no component in this repo currently reads rawInput from tool parts, so there's no visible UI regression today. But the state genuinely diverges between live and reconnected clients, and would surface the moment anything starts surfacing rawInput for debugging/support purposes.
| ) | ||
| } | ||
|
|
||
| export async function getAllProvidersHandler(): Promise< |
There was a problem hiding this comment.
Plausible, already mitigated downstream. getAllProvidersHandler falls back to the full static discoverToolSupportedModels().providers catalog when the runtime is unavailable, inconsistent with every sibling facade in this same commit choosing an honest empty default (getPricingMap -> {}, getThreadInfo -> empty object, getMcpServerTools -> null). Verified this doesn't currently cause the severe harm it looks like it would (a healthy-looking model picker during an outage) — use-available-models.ts's providersWithCredentials independently filters via getChatSettings, which itself soft-fails to an empty apiKey, so the picker still ends up correctly empty. Still worth aligning for consistency, since any future consumer of getAllProviders that doesn't route through that same credential gate would be misled into thinking providers are live when they aren't.
Summary
Closes #2474
Migrates the main-process chat subsystem onto a stable Effect architecture while keeping preload/renderer IPC contracts unchanged.
effectpackage and an app-scopedManagedRuntimewith tagged domain errors, lifecycle wiring, and achat:runtime:healthproberuntime,threads,settings,agents,providers,pricing,mcp, andstreamingdomain services behind thin compatibility facadesStreamRegistrythe sole message-persistence owner; run chat turns as scoped Fibers; keep quit/destroy cleanup safe after runtime disposalRefstate with clearer MCP degradation / fail-closed policiesCommits
chore(deps): add effectrefactor(chat): migrate chat subsystem to Effect domain servicesfeat(chat): expose Effect runtime health over IPCtest(chat): adapt unit tests for Effect chat runtimeTest plan
pnpm run type-checkmain/src/chatand chat IPC handlersChatUnavailableErrorfrompurgeSenderactive-streams.test.ts): cancel mid-stream persists partial snapshot; shutdown flushes debounced writemcp-tools.test.ts): partial discovery continues when one server fails; fail-closed when no usable tools remainMade with Cursor