Skip to content

refactor(chat): migrate main-process chat to Effect domain services#2475

Open
samuv wants to merge 11 commits into
mainfrom
chat-effect
Open

refactor(chat): migrate main-process chat to Effect domain services#2475
samuv wants to merge 11 commits into
mainfrom
chat-effect

Conversation

@samuv

@samuv samuv commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

Closes #2474

Migrates the main-process chat subsystem onto a stable Effect architecture while keeping preload/renderer IPC contracts unchanged.

  • Add the effect package and an app-scoped ManagedRuntime with tagged domain errors, lifecycle wiring, and a chat:runtime:health probe
  • Reorganize chat into runtime, threads, settings, agents, providers, pricing, mcp, and streaming domain services behind thin compatibility facades
  • Make StreamRegistry the sole message-persistence owner; run chat turns as scoped Fibers; keep quit/destroy cleanup safe after runtime disposal
  • Move pricing and MCP UI metadata to runtime-owned Ref state with clearer MCP degradation / fail-closed policies

Commits

  1. chore(deps): add effect
  2. refactor(chat): migrate chat subsystem to Effect domain services
  3. feat(chat): expose Effect runtime health over IPC
  4. test(chat): adapt unit tests for Effect chat runtime

Test plan

  • pnpm run type-check
  • Focused Electron Vitest suites for main/src/chat and chat IPC handlers
  • Manual: quit the app without uncaught ChatUnavailableError from purgeSender
  • Automated (active-streams.test.ts): cancel mid-stream persists partial snapshot; shutdown flushes debounced write
  • Automated (mcp-tools.test.ts): partial discovery continues when one server fails; fail-closed when no usable tools remain
  • Playground E2E smoke with Docker/ToolHive available

Made with Cursor

samuv and others added 4 commits July 17, 2026 11:23
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>
Copilot AI review requested due to automatic review settings July 17, 2026 09:29
@samuv samuv self-assigned this Jul 17, 2026
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>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ManagedRuntime with typed domain errors, adapters, logging, lifecycle hooks, and a chat:runtime:health IPC 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

  • getThreadCount can throw when the runtime isn't ready (via runChatSync), but the chat:get-thread-count IPC handler doesn't catch. Return 0 when unavailable to avoid rejected IPC calls.

Comment thread main/src/chat/streaming/chat-stream-service.ts
Comment thread main/src/chat/settings-storage.ts
Comment thread main/src/chat/settings-storage.ts
Comment thread main/src/chat/settings-storage.ts
Comment thread main/src/chat/settings-storage.ts
Comment thread main/src/chat/settings-storage.ts
Comment thread main/src/chat/threads-storage.ts
Comment thread main/src/chat/threads-storage.ts
samuv and others added 3 commits July 17, 2026 11:39
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>
@samuv

samuv commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator Author

/build-test

@github-actions

Copy link
Copy Markdown
Contributor

Build Artifacts for PR #2475

Platform Architecture Status
macOS arm64 ✅ Ready
macOS x64 ✅ Ready
Windows arm64 ✅ Ready
Windows x64 ✅ Ready
Linux arm64 ✅ Ready
Linux x64 ✅ Ready

Download artifacts from workflow run

Version: pr-2475 | Artifacts expire in 7 days | Triggered by @samuv

Note: macOS builds are signed (not notarized). Windows and Linux builds are unsigned.

@kantord
kantord self-requested a review July 17, 2026 11:04

@kantord kantord left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. The runChatSyncOr/runChatPromiseOr fix in 4bb714dd doesn'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/runChatPromise pattern — which throws/rejects when the chat runtime isn't ready — still exists in agents/registry.ts, active-streams.ts, mcp-tools.ts, providers/providers-facade.ts, and thread-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.
  2. 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.
  3. 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.

Comment thread main/src/chat/agents/registry.ts Outdated
Comment thread main/src/chat/active-streams.ts Outdated
Comment thread main/src/chat/mcp-tools.ts Outdated
Comment thread main/src/chat/providers/providers-facade.ts Outdated
Comment thread main/src/chat/thread-integration.ts Outdated
toolParts: Map<string, ToolPartReplay>
}

const streams = new Map<string, ActiveStream>()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up done in 3eed5105 (+ readability split in 17b133b5).

What we changed for this comment:

  1. Persist-on-exit is structuralrunManagedStream is now Effect.scoped + forkScoped + Fiber.join, with Effect.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.

  2. Removed the module-level persist callback — no more initStreamRegistryPersistence / persistMessagesImpl. The service closes over ThreadsService and persists via getManagedRuntime()?.runSyncExit(...).

  3. Service-owned registry state — streams map is created in StreamRegistryService (with an Effect Ref) and published through a small registry holder so purgeSender / shutdown can still run after runtime dispose without going through adapters.

  4. 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.

samuv and others added 3 commits July 17, 2026 15:41
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 kantord left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. 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')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 mergeThreadsRepository.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 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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': {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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<

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor(chat): migrate main-process chat to Effect domain services

3 participants