Refactor provider runtimes for sessions, auth, and MCP management#666
Merged
Conversation
- Add provider registry to manage LLM providers (Claude, Codex, Cursor, Gemini). - Create provider routes for MCP server operations (list, upsert, delete, run). - Implement MCP service for handling server operations and validations. - Introduce abstract provider class and MCP provider base for shared functionality. - Add tests for MCP server operations across different providers and scopes. - Define shared interfaces and types for MCP functionality. - Implement utility functions for handling JSON config files and API responses.
… from provider.routes.ts
Extract MCP server settings out of the settings controller and agents tab into a dedicated frontend MCP module. The settings UI now delegates MCP rendering and behavior to a single module that only needs the selected provider and current projects. Changes: - Add `src/components/mcp` as the single frontend MCP module - Move MCP server list rendering into `McpServers` - Move MCP add/edit modal into `McpServerFormModal` - Move MCP API/state logic into `useMcpServers` - Move MCP form state/validation logic into `useMcpServerForm` - Add provider-specific MCP constants, types, and formatting helpers - Use the unified `/api/providers/:provider/mcp/servers` API for all providers - Support MCP management for Claude, Cursor, Codex, and Gemini - Remove old settings-owned Claude/Codex MCP modal components - Remove old provider-specific `McpServersContent` branching from settings - Strip MCP server state, fetch, save, delete, and modal ownership from `useSettingsController` - Simplify agents settings props so MCP only receives `selectedProvider` and `currentProjects` - Keep Claude working-directory unsupported while preserving cwd support for Cursor, Codex, and Gemini - Add progressive MCP loading: - render user/global scope first - load project/local scopes in the background - append project results as they resolve - cache MCP lists briefly to avoid slow tab-switch refetches - ignore stale async responses after provider switches Verification: - `npx eslint src/components/mcp` - `npm run typecheck` - `npm run build:client`
Add a separate global MCP add path in the settings MCP module so users can create one shared MCP server configuration across Claude, Cursor, Codex, and Gemini from the same screen. The provider-specific add flow is still kept next to it because these two actions have different intent. A global MCP server must be constrained to the subset of configuration that every provider can accept, while a provider-specific server can still use that provider's own supported scopes, transports, and fields. Naming the buttons as "Add Global MCP Server" and "Add <Provider> MCP Server" makes that distinction explicit without forcing users to infer it from the selected tab. This also moves the explanatory copy to button hover text to keep the MCP toolbar compact while still documenting the difference between global and provider-only adds at the point of action. Implementation details: - Add global MCP form mode with shared user/project scopes and stdio/http transports. - Submit global creates through `/api/providers/mcp/servers/global`. - Reuse the existing MCP form modal with configurable scopes, transports, labels, and descriptions instead of duplicating form logic. - Disable provider-only fields for the global flow because those fields cannot be safely written to every provider. - Clear the MCP server cache globally after a global add because every provider tab may have changed. - Surface partial global add failures with provider-specific error messages. Validation: - npx eslint src/components/mcp/view/McpServers.tsx - npm run typecheck - npm run build:client
Move provider-specific normalizeMessage and fetchHistory logic out of the legacy server/providers adapters and into the refactored provider classes so callers can depend on the main provider contract instead of parallel adapter plumbing. Add a providers service to resolve concrete providers through the registry and delegate message normalization/history loading from realtime handlers and the unified messages route. Add shared TypeScript message/history types and normalized message helpers so provider implementations and callers use the same contract. Remove the old adapter registry/files now that Claude, Codex, Cursor, and Gemini implement the required behavior directly.
Move provider authentication status logic out of the CLI auth route so auth checks live with the provider implementations that understand each provider's install and credential model. Add provider-specific auth runtime classes for Claude, Codex, Cursor, and Gemini, and expose them through the shared provider contract as `provider.auth`. Add a provider auth service that resolves providers through the registry and delegates status checks via `auth.getStatus()`. Keep the existing `/api/cli/<provider>/status` endpoints, but make them thin route adapters over the new provider auth service. This removes duplicated route-local credential parsing and makes auth status a first-class provider capability beside MCP and message handling.
Rename provider auth/MCP contracts to remove the overloaded Runtime suffix so the shared interfaces read as stable provider capabilities instead of execution implementation details. Add a consistent provider-first auth class naming convention by renaming ClaudeAuthProvider, CodexAuthProvider, CursorAuthProvider, and GeminiAuthProvider to ClaudeProviderAuth, CodexProviderAuth, CursorProviderAuth, and GeminiProviderAuth. This keeps the provider module API easier to scan and aligns auth naming with the main provider ownership model.
…rvice Move provider-backed session history and message normalization calls out of the generic providers service so the service name reflects the behavior it owns. Add a dedicated sessions service for listing session-capable providers, normalizing live provider events, and fetching persisted session history through the provider registry. Update realtime handlers and the unified messages route to depend on `sessionsService` instead of `providersService`. This separates session message operations from other provider concerns such as auth and MCP, keeping the provider services easier to navigate as the module grows.
Move provider authentication status endpoints out of the legacy `/api/cli` route namespace so auth status is exposed through the same provider module that owns provider auth and MCP behavior. Add `GET /api/providers/:provider/auth/status` to the provider router and route it through the provider auth service. Remove the old `cli-auth` route file and `/api/cli` mount now that provider auth status is handled by the unified provider API. Update the frontend provider auth endpoint map to call the new provider-scoped routes and rename the endpoint constant to reflect that it is no longer CLI specific.
Remove legacy backend routes that no longer have frontend or internal callers, including the old Claude/Codex MCP APIs, unused Cursor and Codex helper endpoints, stale TaskMaster detection/next/initialize routes, and unused command/project helpers. This reduces duplicated MCP behavior now handled by the provider-based MCP API, shrinks the exposed backend surface, and removes probe/service code that only existed for deleted endpoints. Add an MCP settings API audit document to capture the route-usage analysis and explain why the legacy MCP endpoints were considered safe to remove.
# Conflicts: # server/claude-sdk.js # server/cursor-cli.js # server/gemini-cli.js # server/openai-codex.js # server/providers/cursor/adapter.js # server/providers/registry.js # server/providers/types.js # server/routes/cli-auth.js # server/routes/cursor.js
Contributor
|
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 a typed provider abstraction and centralized provider services (sessions, auth, MCP); implements per-provider auth/MCP/session providers for Claude/Codex/Cursor/Gemini; replaces legacy provider adapters/registry/status modules; introduces MCP frontend UI/hooks/tests; updates server routes, error middleware, and provider integration points. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant ProviderAuthService
participant ProviderImpl
participant OS_FS
Client->>Server: GET /api/providers/:provider/auth/status
Server->>ProviderAuthService: getProviderAuthStatus(provider)
ProviderAuthService->>ProviderImpl: auth.getStatus()
ProviderImpl->>OS_FS: exec/read config files
OS_FS-->>ProviderImpl: exec/file results
ProviderImpl-->>ProviderAuthService: ProviderAuthStatus
ProviderAuthService-->>Server: ProviderAuthStatus
Server-->>Client: JSON success envelope
sequenceDiagram
participant ProviderProcess
participant Server
participant SessionsService
participant ProviderImpl
participant WebSocketClient
ProviderProcess->>Server: stream events (stdout/NDJSON)
Server->>SessionsService: normalizeMessage(provider, raw, sessionId)
SessionsService->>ProviderImpl: provider.sessions.normalizeMessage(raw, sessionId)
ProviderImpl-->>SessionsService: NormalizedMessage[]
SessionsService-->>Server: NormalizedMessage[]
Server->>WebSocketClient: send normalized messages
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
blackmammoth
force-pushed
the
refactor/unified-provider-runtime
branch
from
0495afa to
e0298ac
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/components/settings/hooks/useSettingsController.ts (1)
200-208:⚠️ Potential issue | 🟡 MinorMinor: error-path doesn't reset
geminiPermissionMode.In the catch block (Lines 200-207),
claudePermissions,cursorPermissions,notificationPreferences,codexPermissionMode, andprojectSortOrderare reset to defaults, butgeminiPermissionModeis not. For consistency, reset it as well so a load failure leaves all provider modes in a known state.Proposed fix
} catch (error) { console.error('Error loading settings:', error); setClaudePermissions(createEmptyClaudePermissions()); setCursorPermissions(createEmptyCursorPermissions()); setNotificationPreferences(createDefaultNotificationPreferences()); setCodexPermissionMode('default'); + setGeminiPermissionMode('default'); setProjectSortOrder('name'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/settings/hooks/useSettingsController.ts` around lines 200 - 208, The catch block resets several state values but omits geminiPermissionMode; add a reset for it (e.g., call setGeminiPermissionMode('default') alongside setCodexPermissionMode('default')) so geminiPermissionMode is returned to a known default on load failure; update the catch in useSettingsController (referencing setGeminiPermissionMode and geminiPermissionMode) to include this line.server/routes/messages.js (1)
35-39:⚠️ Potential issue | 🟡 MinorMinor: validate parsed
limit/offsetbefore handing them tofetchHistory.
parseInt('abc', 10)yieldsNaN, and both values currently flow intosessionsService.fetchHistorywithout a guard. Depending on how each provider implements pagination (SQLLIMIT ? OFFSET ?, array.slice, etc.), NaN can produce empty results, throw, or be silently coerced — surfacing as a 500 to the client. Suggest rejecting with 400 on non-integer input.🛡️ Proposed fix
const limitParam = req.query.limit; - const limit = limitParam !== undefined && limitParam !== null && limitParam !== '' - ? parseInt(limitParam, 10) - : null; - const offset = parseInt(req.query.offset || '0', 10); + let limit = null; + if (limitParam !== undefined && limitParam !== null && limitParam !== '') { + const parsedLimit = parseInt(limitParam, 10); + if (!Number.isFinite(parsedLimit) || parsedLimit < 0) { + return res.status(400).json({ error: 'Invalid limit' }); + } + limit = parsedLimit; + } + const parsedOffset = parseInt(req.query.offset || '0', 10); + if (!Number.isFinite(parsedOffset) || parsedOffset < 0) { + return res.status(400).json({ error: 'Invalid offset' }); + } + const offset = parsedOffset;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/messages.js` around lines 35 - 39, The parsed pagination values (limit from limitParam and offset via parseInt on req.query.offset) can be NaN and are passed directly into sessionsService.fetchHistory; add validation after parsing to ensure both limit and offset are integers (or null for optional limit) and reject with a 400 Bad Request when parseInt returns NaN or when limit/offset are negative/non-integer; update the handler to check the parsed values and call res.status(400).json(...) on invalid input before calling sessionsService.fetchHistory so fetchHistory always receives safe numeric values.src/components/settings/view/tabs/agents-settings/sections/AgentSelectorSection.tsx (1)
22-42:⚠️ Potential issue | 🟡 MinorGuard against missing
agentContextById[agent]entries.Now that
agentsis supplied dynamically (and may be filtered by runtime platform), any agent id that isn't present as a key inagentContextByIdwill causeagentContextById[agent].authStatusto throw at render. A cheap defensive read avoids hard-to-debug crashes if the two lists ever drift.🛡️ Proposed guard
- {agentContextById[agent].authStatus.authenticated && ( + {agentContextById[agent]?.authStatus?.authenticated && ( <span className={`h-1.5 w-1.5 flex-shrink-0 rounded-full ${dotColor}`} /> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/settings/view/tabs/agents-settings/sections/AgentSelectorSection.tsx` around lines 22 - 42, AgentSelectorSection currently accesses agentContextById[agent].authStatus directly during agents.map, which can crash if agentContextById lacks that key; update the render to defensively read the context (e.g., const ctx = agentContextById[agent] || undefined) and replace agentContextById[agent].authStatus.authenticated with a safe check (e.g., ctx?.authStatus?.authenticated or Boolean(ctx && ctx.authStatus && ctx.authStatus.authenticated)) so missing entries simply render without the authenticated dot; ensure key symbols referenced are AgentSelectorSection, agents.map callback, agentContextById, and authStatus.server/cursor-cli.js (1)
291-308:⚠️ Potential issue | 🟡 MinorMinor: async
errorlistener can swallow rejections.If
providerAuthService.isProviderInstalled('cursor')ever rejects (e.g., underlyingwhich/fs error), this async handler will surface as an unhandled promise rejection andsettleOnce(reject)won't be called, leaving the outer Promise pending. Consider wrapping the check in try/catch and falling back toerror.messageon failure.🛡️ Proposed hardening
cursorProcess.on('error', async (error) => { console.error('Cursor CLI process error:', error); // Clean up process reference on error const finalSessionId = capturedSessionId || sessionId || processKey; activeCursorProcesses.delete(finalSessionId); // Check if Cursor CLI is installed for a clearer error message - const installed = await providerAuthService.isProviderInstalled('cursor'); - const errorContent = !installed - ? 'Cursor CLI is not installed. Please install it from https://cursor.com' - : error.message; + let installed = true; + try { + installed = await providerAuthService.isProviderInstalled('cursor'); + } catch (checkError) { + console.error('Failed to determine cursor installation state:', checkError); + } + const errorContent = !installed + ? 'Cursor CLI is not installed. Please install it from https://cursor.com' + : error.message; ws.send(createNormalizedMessage({ kind: 'error', content: errorContent, sessionId: capturedSessionId || sessionId || null, provider: 'cursor' })); notifyTerminalState({ error }); settleOnce(() => reject(error)); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/cursor-cli.js` around lines 291 - 308, The async error handler attached to cursorProcess (the callback starting with cursorProcess.on('error', async (error) => { ... })) can let a rejection from providerAuthService.isProviderInstalled('cursor') escape and prevent settleOnce(reject) from running; wrap the call to providerAuthService.isProviderInstalled in a try/catch (or use Promise.resolve().catch) and if it throws/fails, use error.message as the fallback errorContent, then continue to call ws.send(...), notifyTerminalState({ error }) and finally ensure settleOnce(() => reject(error)) always executes even if the installed check fails.server/gemini-cli.js (2)
402-418:⚠️ Potential issue | 🟠 MajorSame hang risk in the async
errorhandler.If
providerAuthService.isProviderInstalled('gemini')throws inside this handler,reject(error)on line 417 won't run and thespawnGeminipromise never settles. Wrap the check in try/catch (defaultinginstalledtotrue) so we always reach the finalreject(error).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/gemini-cli.js` around lines 402 - 418, The async error handler attached to geminiProcess may never reach reject(error) if providerAuthService.isProviderInstalled('gemini') throws; wrap the providerAuthService.isProviderInstalled call in a try/catch (defaulting installed = true on error) or otherwise catch errors when computing errorContent so that errorContent is set safely, then proceed to call ws.send(createNormalizedMessage(...)), notifyTerminalState({ error }) and reject(error) unconditionally; update the geminiProcess.on('error', async (error) => { ... }) block to ensure any exception from providerAuthService.isProviderInstalled or message sending does not prevent the final reject(error).
384-399:⚠️ Potential issue | 🟠 MajorAsync
closehandler can hang the outer promise ifisProviderInstalledthrows.If
providerAuthService.isProviderInstalled('gemini')rejects (e.g. unexpected FS/parse error inside the provider auth class), theawaitaborts this handler beforereject(...)on line 397 runs. Because the outerPromise(returned byspawnGemini) is only resolved/rejected inside these handlers, that promise will never settle and the caller (chat WebSocket) will be left waiting indefinitely.🛠️ Suggested guard
- if (code === 127) { - const installed = await providerAuthService.isProviderInstalled('gemini'); - if (!installed) { + if (code === 127) { + let installed = true; + try { + installed = await providerAuthService.isProviderInstalled('gemini'); + } catch (err) { + console.error('[Gemini] isProviderInstalled check failed:', err); + } + if (!installed) { const socketSessionId = typeof ws.getSessionId === 'function' ? ws.getSessionId() : finalSessionId; ws.send(createNormalizedMessage({ kind: 'error', content: 'Gemini CLI is not installed. Please install it first: https://github.com/google-gemini/gemini-cli', sessionId: socketSessionId, provider: 'gemini' })); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/gemini-cli.js` around lines 384 - 399, The close handler in spawnGemini can hang if providerAuthService.isProviderInstalled('gemini') throws—wrap the isProviderInstalled call in a try/catch (or handle its Promise with .catch) inside the close event so any error does not abort the handler; on error, still call notifyTerminalState(...) and reject(...) with a meaningful Error (and optionally log/send an error to ws via ws.send) so the outer Promise always settles; update the handler around providerAuthService.isProviderInstalled, socketSessionId/ws.send, notifyTerminalState, and reject to ensure reject(...) is executed regardless of providerAuthService failures.
🧹 Nitpick comments (29)
server/shared/interfaces.ts (3)
45-47: ExtractremoveServerreturn type for reuse.The inline
{ removed: boolean; provider: LLMProvider; name: string; scope: McpScope }shape is likely re-stated at call sites (service, routes). Promoting it to a named type in@/shared/types.js(e.g.,RemoveProviderMcpServerResult) improves DX and keeps the contract DRY.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/shared/interfaces.ts` around lines 45 - 47, Extract the inline return shape used by removeServer into a reusable named type (e.g., RemoveProviderMcpServerResult) in the shared types module and update the removeServer signature to return Promise<RemoveProviderMcpServerResult>; specifically, add the type definition for { removed: boolean; provider: LLMProvider; name: string; scope: McpScope } to "@/shared/types.js" and replace the inline anonymous return type in the removeServer declaration in server/shared/interfaces.ts (and any duplicated typings at call sites such as services or routes) to reference RemoveProviderMcpServerResult.
42-42:Record<McpScope, ProviderMcpServer[]>forces every scope key to be returned.
Record<K, V>(whereKis a finite union) is total: implementations must return everyMcpScopekey, even when a provider doesn't support a given scope (e.g., Cursor on a scope that isn't applicable). This can lead to accidental missing-key runtime bugs if TS is bypassed, or noisy boilerplate returning empty arrays.If scopes are optional per provider, prefer
Partial<Record<McpScope, ProviderMcpServer[]>>. If totality is intentional (and consumers rely on it), leave a brief JSDoc note so implementers know empty arrays are required for unsupported scopes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/shared/interfaces.ts` at line 42, The current listServers signature forces returning every McpScope key by using Record<McpScope, ProviderMcpServer[]>, which is too strict for providers that don't support all scopes; update the return type of listServers to Partial<Record<McpScope, ProviderMcpServer[]>> so unsupported scopes can be omitted, or alternatively add a short JSDoc on the listServers declaration (and the McpScope/ProviderMcpServer types) stating that the mapping is total and implementers must return empty arrays for unsupported scopes if totality is intentional; modify the signature in server/shared/interfaces.ts where listServers is declared and adjust implementations accordingly.
23-23: Current synchronous design is confirmed and appropriate for all providers.All four implementations (Claude, Gemini, Cursor, Codex) are purely synchronous event-mapping operations with no async calls. Async context resolution (fetching, decryption, cached state) is already handled separately in
fetchHistory(), which aligns with the architectural intent documented in the code. The current signature works without workarounds.If future providers need async preprocessing of events before normalization, consider making this change—but no current need exists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/shared/interfaces.ts` at line 23, The current synchronous signature normalizeMessage(raw: unknown, sessionId: string | null): NormalizedMessage[] is correct — no code change required; keep the interface method as a synchronous event-mapping operation, ensure implementations (Claude/Gemini/Cursor/Codex) remain purely synchronous and rely on fetchHistory() for any async fetching/decryption/caching, and add a short comment near normalizeMessage explaining that async preprocessing should be handled upstream (fetchHistory) and that providers should only perform synchronous mapping unless future async needs arise.eslint.config.js (1)
163-172:backend-legacy-runtimeelement is classified but unreferenced in rules.The element is defined but no rule in
boundaries/dependenciesexplicitly mentions it—so its only effect is lettingboundaries/no-unknownrecognize these paths. That's a valid use, but consider adding a comment clarifying this intent or a rule restricting which modules may depend on legacy runtime to support the staged migration goal stated in the inline comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eslint.config.js` around lines 163 - 172, The "backend-legacy-runtime" element in the eslint.config.js boundaries list is declared but not referenced by any rule in boundaries/dependencies, so it only helps boundaries/no-unknown; either add a clarifying comment next to the "backend-legacy-runtime" object explaining that its sole purpose is to satisfy boundaries/no-unknown during migration, or add an explicit rule in boundaries/dependencies that restricts which modules can depend on "backend-legacy-runtime" (for example a dependency rule listing "backend-legacy-runtime" as an allowed target for a specific source group) so the staged migration intent is enforced; locate the "backend-legacy-runtime" entry and the boundaries/dependencies block to implement the chosen change.src/components/mcp/hooks/useMcpServers.ts (2)
477-479: Minor:window.confirmbypasses i18n used elsewhere in this flow.Other user-visible messages in this file and the companion form hook go through
t(...)/ translated validation keys, but the delete confirmation is a hard-coded English string. Consider routing it through the same i18n bundle for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mcp/hooks/useMcpServers.ts` around lines 477 - 479, Replace the hard-coded English confirmation string used in the window.confirm call with the i18n translation key used elsewhere in this flow; locate the window.confirm in useMcpServers (the delete confirmation branch) and call the translator (t('your.translation.key') or the existing t function from the same hook/context) instead of the literal 'Are you sure you want to delete this MCP server?', ensuring the appropriate translation key is added to the i18n files and used consistently with other user-visible strings in this module.
375-398: Optional: sort intermediate server updates and bound project-scope concurrency.Two small polish items while project scopes stream in:
- Line 383 sets
nextServerswithout sorting, so users see entries reshuffle when the final sort is applied on Line 395. Passing throughsortServerson each incremental set keeps ordering stable during progressive paint.Promise.allfans out one request per(project × scope); for users with many projects this can produce a burst. Consider a small concurrency limit (e.g., p-limit-style, or chunked batches).Non-blocking — file as a follow-up if useful.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mcp/hooks/useMcpServers.ts` around lines 375 - 398, The incremental update logic inside the project scope loading loop should sort and throttle requests: after replacing scoped servers with replaceScopedServers(updateNextServers, scopedServers, scope, project.path) call sortServers(nextServers) before calling setServers so the UI sees stable ordering, and replace the Promise.all over projectScopeRequests with a bounded concurrency strategy (e.g., use p-limit or process projectScopeRequests in small chunks) when calling fetchProviderScopeServers to avoid fanning out too many simultaneous requests; keep checks against activeLoadIdRef.current and use setLoadError/setIsLoadingProjectScopes as before.src/components/mcp/utils/mcpFormatting.ts (1)
130-153: Minor: JSON path doesn't gatecommand/urlby transport like the form path does.
parseJsonMcpPayloadunconditionally includescommandandurlfrom parsed JSON, whilecreateMcpPayloadFromFormclears them based ontransport. If a user pastes JSON that has stale fields from another transport (e.g., bothcommandandurl), the payload will carry both to the backend. Consider mirroring the transport-gating used on the form path for consistency and to avoid confusing upstream validation.♻️ Proposed tweak
- command: readString(parsed.command), - args: readStringArray(parsed.args) ?? [], + command: transport === 'stdio' ? readString(parsed.command) : undefined, + args: transport === 'stdio' ? (readStringArray(parsed.args) ?? []) : undefined, env: readStringRecord(parsed.env) ?? {}, cwd: (options?.supportsWorkingDirectory ?? MCP_SUPPORTS_WORKING_DIRECTORY[provider]) ? readString(parsed.cwd) : undefined, - url: readString(parsed.url), - headers: readStringRecord(parsed.headers ?? parsed.http_headers) ?? {}, + url: transport !== 'stdio' ? readString(parsed.url) : undefined, + headers: transport !== 'stdio' + ? (readStringRecord(parsed.headers ?? parsed.http_headers) ?? {}) + : undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mcp/utils/mcpFormatting.ts` around lines 130 - 153, parseJsonMcpPayload currently always reads command and url from parsed JSON which can leave stale fields when transport differs; mirror the transport-gating logic used in createMcpPayloadFromForm by only populating command when transport === 'exec' (or the form's exec condition) and only populating url when transport === 'http' (or the form's http condition). Update parseJsonMcpPayload to check the parsed transport value before calling readString(parsed.command) and readString(parsed.url), and set those fields to undefined when the transport does not match, keeping other parsing (args, headers, etc.) unchanged.src/components/mcp/hooks/useMcpServerForm.ts (1)
217-230: Consider surfacing submission errors via UI state instead ofalert().Using
window.alertblocks the main thread, isn't themeable, bypasses i18n, and is hard to test. Since the hook already managesjsonValidationErrorand other UI state, exposing asubmitError(or delegating to the caller) would integrate better with the modal and keep messaging consistent with the rest of the form.♻️ Proposed shape
- const [isSubmitting, setIsSubmitting] = useState(false); + const [isSubmitting, setIsSubmitting] = useState(false); + const [submitError, setSubmitError] = useState<string | null>(null); @@ - try { - await onSubmit(createSubmitFormData(), editingServer); - } catch (error) { - alert(`Error: ${getErrorMessage(error)}`); - } finally { + setSubmitError(null); + try { + await onSubmit(createSubmitFormData(), editingServer); + } catch (error) { + setSubmitError(getErrorMessage(error)); + } finally { setIsSubmitting(false); }…and return
submitErrorfrom the hook for the modal to render.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mcp/hooks/useMcpServerForm.ts` around lines 217 - 230, The catch in handleSubmit currently calls alert(); replace this with hook-managed UI state by adding a submitError state (e.g., const [submitError, setSubmitError]) in useMcpServerForm, setSubmitError(getErrorMessage(error)) inside the catch of handleSubmit, clear submitError before a new submit (or when inputs change), return submitError from the hook alongside existing jsonValidationError/isSubmitting, and have the caller/modal render the error instead of relying on window.alert; keep setIsSubmitting(false) in finally and keep using createSubmitFormData() and onSubmit(editingServer) unchanged.src/hooks/useServerPlatform.ts (2)
1-40: Optional: cache or lift the platform fetch to avoid duplicate requests.Multiple consumers (
ProviderSelectionEmptyState,AgentsSettingsTab) each trigger an independent/api/settings/server-envfetch on mount. A module-level memo/promise or a context provider would dedupe the request and keep the derivedisWindowsServerstable across components.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useServerPlatform.ts` around lines 1 - 40, The hook useServerPlatform triggers duplicate network calls; add a module-level cache/promise (e.g., cachedPlatform: string | null | undefined and platformPromise: Promise<string | null> | null) and update useServerPlatform to first read cachedPlatform (set state immediately if present) or reuse platformPromise to await a single in-flight fetch for '/api/settings/server-env', populate cachedPlatform when resolved, then setServerPlatform if not cancelled; ensure existing cancellation flag and error handling remain so multiple mounts reuse the cached value or shared promise and avoid duplicate requests across ProviderSelectionEmptyState and AgentsSettingsTab.
15-39: Consider a safer default on fetch failure.When
/api/settings/server-enverrors or returns non-OK,serverPlatformstaysnullandisWindowsServerbecomesfalse. For Windows servers whose settings endpoint momentarily fails (or auth races), the UI will showcursoras an option even though it's unsupported — and the auto-switch inProviderSelectionEmptyStatewon't fire. If this is acceptable, a short JSDoc note documenting the "on failure → not-Windows" behavior would help callers reason about it; otherwise, consider retrying once or returning a tri-state (unknown|windows|other).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useServerPlatform.ts` around lines 15 - 39, The hook useServerPlatform currently leaves serverPlatform null on fetch errors or non-OK responses which causes callers (like ProviderSelectionEmptyState) to treat the host as non-Windows; change the behavior to a safe tri-state: on any fetch error or non-OK response setServerPlatform('unknown') (ensure this is done only when not cancelled) instead of leaving null, and keep isWindowsServer as serverPlatform === 'win32' so callers can explicitly check for 'unknown' and avoid treating it as non-Windows; update any callers (e.g., ProviderSelectionEmptyState) to handle the 'unknown' state or fall back to retry logic if needed.server/modules/providers/services/provider-auth.service.ts (1)
8-25: Minor type-consistency nit on the two public methods.
getProviderAuthStatusaccepts a loosestringwhileisProviderInstalledrequiresLLMProvider. Since internally both end up inproviderRegistry.resolveProvider()(which validates), narrowinggetProviderAuthStatustoLLMProviderwould give callers earlier compile-time checks and make the HTTP route (/:provider/auth/status) the only place where an unvalidated string is intentionally accepted.♻️ Proposed fix
- async getProviderAuthStatus(providerName: string): Promise<ProviderAuthStatus> { + async getProviderAuthStatus(providerName: LLMProvider): Promise<ProviderAuthStatus> { const provider = providerRegistry.resolveProvider(providerName); return provider.auth.getStatus(); },If the HTTP route really needs to accept arbitrary strings, keep an overload or call
resolveProviderdirectly there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/providers/services/provider-auth.service.ts` around lines 8 - 25, getProviderAuthStatus currently accepts a loose string while isProviderInstalled requires LLMProvider, causing inconsistent typing; change getProviderAuthStatus(providerName: string) to getProviderAuthStatus(providerName: LLMProvider) so both public methods use the validated LLMProvider type and callers get earlier compile-time checks, and if the HTTP route needs to accept raw strings, handle validation there by calling providerRegistry.resolveProvider(...) or add an overload for getProviderAuthStatus that accepts string and resolves to LLMProvider before delegating to the typed method.server/shared/types.ts (2)
58-79:toolUseResultappears both nested insidetoolResultand as a top-level field.toolResult?: { ...; toolUseResult?: unknown }; ... toolUseResult?: unknown;Consumers will not know which field to read, and providers that emit only one path risk invisible divergence. If both forms exist for historical reasons, worth a doc comment; otherwise prefer collapsing to a single location (nested under
toolResultis likely the clearer home).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/shared/types.ts` around lines 58 - 79, The type currently defines toolUseResult in two places (top-level toolUseResult and toolResult.toolUseResult) which causes ambiguity; collapse to a single location by removing the top-level toolUseResult and keeping toolUseResult nested under the toolResult object (update the interface so only toolResult?: { toolUseResult?: unknown; ... } exists), add a short doc comment above toolResult explaining it's the authoritative place for tool results, and then search for usages of the top-level toolUseResult (e.g., places that read/write message.toolUseResult) and update them to message.toolResult?.toolUseResult or adjust producers to populate toolResult.toolUseResult accordingly.
79-80: The[key: string]: unknownindex signature weakens the envelope contract.All other declared optional fields become effectively
unknownat the read site (any access resolves via the index signature), which defeats much of the value of enumerating them above. If the goal is to allow provider-specific extras, consider a dedicatedmeta?: Record<string, unknown>sub-field instead so the declared properties keep their precise types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/shared/types.ts` around lines 79 - 80, The interface in server/shared/types.ts currently has a broad index signature "[key: string]: unknown" which weakens the typed properties; remove that index signature and instead add a dedicated "meta?: Record<string, unknown>" (or similarly named) optional field to hold provider-specific extras so declared properties retain their precise types; update any call sites or utility code that relied on the index signature to read/write provider extras from the new meta field (e.g., adjust usages referencing the interface where arbitrary keys were accessed).server/modules/providers/services/sessions.service.ts (1)
27-44: TightenproviderNametype toLLMProviderfor compile-time safety.
listProviderIds()returnsLLMProvider[], butnormalizeMessageandfetchHistoryacceptproviderName: string. Callers already have (or should have) a validatedLLMProvider; widening tostringpushes the validation burden ontoproviderRegistry.resolveProviderat runtime only.♻️ Proposed change
normalizeMessage( - providerName: string, + providerName: LLMProvider, raw: unknown, sessionId: string | null, ): NormalizedMessage[] { return providerRegistry.resolveProvider(providerName).normalizeMessage(raw, sessionId); }, fetchHistory( - providerName: string, + providerName: LLMProvider, sessionId: string, options?: FetchHistoryOptions, ): Promise<FetchHistoryResult> { return providerRegistry.resolveProvider(providerName).fetchHistory(sessionId, options); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/providers/services/sessions.service.ts` around lines 27 - 44, Both normalizeMessage and fetchHistory accept providerName: string but listProviderIds() and the domain model use LLMProvider; change the parameter type from string to LLMProvider on normalizeMessage and fetchHistory so callers get compile-time validation. Update the signatures in sessions.service.ts (normalizeMessage and fetchHistory) to use LLMProvider, adjust any local references that pass providerName to providerRegistry.resolveProvider(providerName) unchanged, and fix any callers that currently pass raw strings to cast or (better) validate/convert to LLMProvider before calling these methods.server/modules/providers/list/claude/claude-auth.provider.ts (1)
99-116: OAuth token validation: treat missingexpiresAtexplicitly.
!expiresAt || Date.now() < expiresAttreats a missing/invalidexpiresAtas "never expires". That's likely fine for Claude's credentials file format, but worth a short comment explaining the intent so future readers don't interpret it as a bug. No code change necessary if the intent is confirmed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/providers/list/claude/claude-auth.provider.ts` around lines 99 - 116, The current conditional treats a missing or non-number oauth.expiresAt as "never expires" (const expiresAt = typeof oauth?.expiresAt === 'number' ? oauth.expiresAt : undefined; if (!expiresAt || Date.now() < expiresAt) ...) which can be misread as a bug; add a short comment above the expiresAt declaration and the conditional explaining the intentional behavior (missing/invalid expiresAt => token considered non-expiring for Claude credentials file), referencing the expiresAt variable and the surrounding accessToken check in claude-auth.provider.ts so future maintainers understand this is deliberate and not an oversight.server/modules/providers/provider.routes.ts (1)
76-132:readOptionalQueryStringon request body is misleading.Body fields (
name,command,cwd,url,workspacePath,bearerTokenEnvVar) are parsed throughreadOptionalQueryString, which is named for query strings and silently trims the value. It works, but renaming the helper (e.g.readOptionalString) or introducing a body-specific variant would make the intent clearer and avoid surprise if trimming semantics ever diverge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/providers/provider.routes.ts` around lines 76 - 132, In parseMcpUpsertPayload, the helper readOptionalQueryString is used to parse body fields (name, command, cwd, url, workspacePath, bearerTokenEnvVar) which is misleading because it’s intended for query strings and implicitly trims; replace those calls with a clearly named body helper (e.g., readOptionalString or readOptionalBodyString) or add a body-specific variant that preserves the same trimming/validation semantics you need, then update parseMcpUpsertPayload to call that new helper for the listed fields so intent matches usage and future changes to query-string logic won’t affect body parsing.server/modules/providers/list/cursor/cursor.provider.ts (2)
176-197: Liveassistantevent only surfacescontent[0].text.If Cursor ever emits multi-part assistant content (e.g. interleaved text/tool deltas), only the first text part is streamed. Consider iterating all
raw.message.contententries and emitting astream_deltaper textual part, or at least logging/dropping non-text parts explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/providers/list/cursor/cursor.provider.ts` around lines 176 - 197, normalizeMessage currently only uses raw.message.content[0].text which drops additional content parts; update normalizeMessage (and the raw = readRawProviderMessage(...) handling) to iterate over raw.message.content array and for each entry that has a textual part emit a createNormalizedMessage({ kind: 'stream_delta', content: <text>, sessionId, provider: PROVIDER }); for non-text parts (tools/deltas) either log a debug message or explicitly drop them so it’s clear they were ignored; ensure you preserve ordering of parts and return an array of NormalizedMessage for all text pieces instead of only the first one.
126-142: O(n·m) blob-id search can be slow on large sessions.For every non-JSON blob in topological order, this scans its bytes for each JSON blob's id buffer (
blob.data.includes(idBytes)). On sessions with thousands of blobs this becomes quadratic. A cheaper approach is to derive the parent/child references you already collected inparentRefs/childRefsand use those to assign order, or build a single Aho–Corasick/multi-pattern scan over binary blobs. Not a blocker for typical sizes, but worth profiling if users report slow history loads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/providers/list/cursor/cursor.provider.ts` around lines 126 - 142, The nested loop over sorted and jsonBlobs that builds messageOrder by checking blob.data.includes(idBytes) is O(n·m) and causes quadratic scans; replace this by deriving message order from the already-collected graph references (parentRefs/childRefs) or a single multi-pattern scan instead of per-id includes: traverse the topologically-sorted blobs (sorted) and assign orderIndex using relationships in parentRefs/childRefs (or run one Aho–Corasick-like pass over each blob once) to set messageOrder for jsonBlobs, avoiding per-blob per-id Buffer.includes calls in the block that handles non-JSON blobs.server/modules/providers/list/claude/claude-mcp.provider.ts (1)
39-66: EmptymcpServerspersisted on project-scope deletions.When the last project-scoped server is removed, this still writes
{mcpServers: {}}to<workspacePath>/.mcp.json(creating the file if missing). Consider skipping the write (or deleting the key) whenObject.keys(servers).length === 0to avoid leaving empty config files in user workspaces.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/providers/list/claude/claude-mcp.provider.ts` around lines 39 - 66, In writeScopedServers, avoid persisting an empty mcpServers object when servers is empty: check Object.keys(servers).length === 0 and for scope === 'project' simply skip writing (return) instead of creating/overwriting .mcp.json; for scope === 'user' and the global projects branch, either remove the mcpServers key from the loaded config/projectConfig (delete config.mcpServers or delete projectConfig.mcpServers) before calling writeJsonConfig or skip the write entirely if nothing else changed. Update the logic around readJsonConfig/writeJsonConfig, and the projects handling (readObjectRecord, config.projects, projects[workspacePath]) to ensure you don’t create empty keys/files when servers is empty.server/modules/providers/list/codex/codex-mcp.provider.ts (1)
17-29: RedundantreadObjectRecordon TOML parse result.
TOML.parsealready returns a plain object (or throws), soreadObjectRecord(parsed) ?? {}never takes the fallback branch in practice. Not a bug, just noise — considerreturn parsed ?? {}or justreturn parsed;for readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/providers/list/codex/codex-mcp.provider.ts` around lines 17 - 29, The parsed TOML object returned by TOML.parse is already a plain object, so in readTomlConfig replace the unnecessary call to readObjectRecord(parsed) and simply return the parsed value (or parsed ?? {} if you want a defensive fallback); update the readTomlConfig function to return parsed directly and remove the readObjectRecord usage in that return path while keeping the existing error handling intact.server/modules/providers/shared/mcp/mcp.provider.ts (2)
40-52: Returning empty arrays for unsupported scopes may leak capability info.
listServersalways shapes the response as{ user, local, project }even for providers that don't supportlocal(gemini/codex/cursor) orprojectetc. That's fine if clients know to ignore them, but it's slightly misleading vs.listServersForScopewhich returns[]for unsupported scopes. Consider only including supported scopes, or documenting the contract onIProviderMcp.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/providers/shared/mcp/mcp.provider.ts` around lines 40 - 52, listServers currently always returns keys for user/local/project which can leak unsupported capabilities; change it to only include keys from this.supportedScopes by iterating this.supportedScopes and assigning grouped[scope] = await this.listServersForScope(scope, options) into a grouped object initialized empty (or as a Map) rather than prepopulating user/local/project. Update references to the grouped variable in the function to reflect the dynamic keys and keep use of listServersForScope and supportedScopes unchanged.
69-94:upsertServerreturns unpersisted input fields instead of the normalized server.The return object is assembled from the raw
UpsertProviderMcpServerInput, bypassing the subclass'snormalizeServerConfig. A subclass that intentionally drops or transforms a field duringbuildServerConfig(e.g., dropping emptyheaders, defaulting transport type, normalizing paths) will have that transformation reflected on disk but not in the value returned to the caller / HTTP response. This is an easy source of "the list shows X but the upsert response said Y" bugs.Consider re-normalizing after persist so the read-back is authoritative:
♻️ Suggested refactor
const workspacePath = resolveWorkspacePath(input.workspacePath); const normalizedName = normalizeServerName(input.name); const scopedServers = await this.readScopedServers(scope, workspacePath); - scopedServers[normalizedName] = this.buildServerConfig(input); + const persisted = this.buildServerConfig(input); + scopedServers[normalizedName] = persisted; await this.writeScopedServers(scope, workspacePath, scopedServers); - return { - provider: this.provider, - name: normalizedName, - scope, - transport: input.transport, - command: input.command, - args: input.args, - env: input.env, - cwd: input.cwd, - url: input.url, - headers: input.headers, - envVars: input.envVars, - bearerTokenEnvVar: input.bearerTokenEnvVar, - envHttpHeaders: input.envHttpHeaders, - }; + const normalized = this.normalizeServerConfig(scope, normalizedName, persisted); + if (!normalized) { + throw new AppError('Failed to normalize freshly persisted MCP server.', { + code: 'MCP_NORMALIZE_FAILED', + statusCode: 500, + }); + } + return normalized;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/providers/shared/mcp/mcp.provider.ts` around lines 69 - 94, upsertServer currently returns fields from the raw UpsertProviderMcpServerInput instead of the authoritative, persisted/normalized config, causing mismatches when subclasses transform data in buildServerConfig/normalizeServerConfig; after calling writeScopedServers, re-read the persisted config (e.g., via readScopedServers or by retrieving scopedServers[normalizedName] after persistence) or use the normalized result of buildServerConfig/normalizeServerConfig and use that object to populate the returned ProviderMcpServer (keep provider/name/scope as now but source the rest from the persisted/normalized server config).server/modules/providers/list/codex/codex.provider.ts (1)
22-26: Double-cast viaunknownhides the real signature drift.
as unknown as (...) => Promise<CodexHistoryResult>silently papers over any future signature change ingetCodexSessionMessagesfrom@/projects.js. If that module is TS or has JSDoc types, import the actual type (or let TS infer); otherwise at least add a// FIXME: projects.js is JS — remove cast once typednote so this doesn't calcify.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/providers/list/codex/codex.provider.ts` around lines 22 - 26, The double-cast using "as unknown as" for getCodexSessionMessages (assigning to loadCodexSessionMessages) hides real signature drift; replace the cast by importing or referencing the actual function/type from the source (use the real exported type for getCodexSessionMessages or the CodexHistoryResult type from the projects.js module) so TypeScript can check signatures, or if projects.js is untyped JS, add a clear FIXME comment like "// FIXME: projects.js is JS — remove cast once typed" next to the loadCodexSessionMessages assignment to prevent this silent mismatch from calcifying.server/modules/providers/tests/mcp.test.ts (1)
12-18: Globalos.homedirpatch is brittle against parallel/imported-at-startup callers.
patchHomeDirmutates the exportedos.homedirreference. It works here because all provider classes callos.homedir()at request time, andnode:testruns tests in this file sequentially. Two caveats worth a comment or a more targeted fix:
- Any module that snapshots
os.homedir()at import time will not be affected by the patch, silently writing to the real home directory during tests.- If this suite ever runs with
--test-concurrency>1alongside other files that also rely onos.homedir(), cross-file races will occur.Consider centralizing via
process.env.HOME/process.env.USERPROFILEoverride (whichos.homedir()already reads on first call in some Node versions — verify) or adding aworkspacePath/userPathparameter threaded through the providers so tests don't need monkey-patching at all.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/providers/tests/mcp.test.ts` around lines 12 - 18, The patchHomeDir helper mutates os.homedir and is brittle; instead, stop monkey-patching os.homedir and either set process.env.HOME (and process.env.USERPROFILE on Windows) inside the test helper or add a configurable workspace/user path parameter to the provider constructors so tests can inject a fake home; update tests to use the new helper (or pass the workspacePath/userPath into the providers) and remove calls to patchHomeDir, keeping a restore for env vars if you change process.env. Ensure references to patchHomeDir, os.homedir, and any provider constructors (the provider class names used in these tests) are updated accordingly.src/components/mcp/view/modals/McpServerFormModal.tsx (1)
122-130: Consider adding dialog accessibility primitives.The outer
<div>acts as a modal but is missingrole="dialog",aria-modal="true", anaria-labelledbypointing at the<h3>, an Escape-key handler bound toonClose, and a focus trap / initial-focus target. Screen reader and keyboard-only users cannot reliably dismiss or scope to this modal.♻️ Sketch
- <div className="fixed inset-0 z-[110] flex items-center justify-center bg-black/50 p-4"> - <div className="max-h-[90vh] w-full max-w-2xl overflow-y-auto rounded-lg border border-border bg-background"> + <div + className="fixed inset-0 z-[110] flex items-center justify-center bg-black/50 p-4" + role="dialog" + aria-modal="true" + aria-labelledby="mcp-server-form-title" + onKeyDown={(e) => { if (e.key === 'Escape') onClose(); }} + > + <div className="max-h-[90vh] w-full max-w-2xl overflow-y-auto rounded-lg border border-border bg-background">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mcp/view/modals/McpServerFormModal.tsx` around lines 122 - 130, McpServerFormModal currently renders the modal container as a plain div — add proper dialog accessibility by giving the outer container role="dialog" and aria-modal="true", add an id on the <h3> (e.g., connect to modalTitle via aria-labelledby), wire the Escape key to call the existing onClose handler, and implement a simple focus trap/initial focus (set focus to a sensible element inside the modal, e.g., the first input or the close Button referenced by onClose) so keyboard and screen-reader users can dismiss and stay scoped to the modal; update McpServerFormModal to set these attributes and behaviours around the elements that include modalTitle, onClose and the close <Button>.server/shared/utils.ts (2)
156-170:readStringRecordcollapses "empty-but-authored" maps toundefined.If a user explicitly persists
"env": {}in their config,readStringRecordreturnsundefined, and then a subsequent upsert that re-reads → re-normalizes → re-writes will drop theenvkey entirely from disk. The same holds forheaders,env_http_headers, etc. That round-trip erasure is usually harmless, but it does mean CodeRabbit users can observe their config file mutating on the next write even when they made no changes through the UI.Consider returning the empty record (vs.
undefined) so the distinction between "field absent" and "field present but empty" is preserved:- return Object.keys(normalized).length > 0 ? normalized : undefined; + return normalized;Double-check the call sites in
gemini-mcp.provider.ts/codex-mcp.provider.tsstill behave correctly if this flips.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/shared/utils.ts` around lines 156 - 170, The function readStringRecord currently collapses explicitly authored empty objects to undefined, losing the distinction between "absent" and "present-but-empty"; update readStringRecord so that when readObjectRecord(value) returns an object (even if it has no string entries) it returns an empty Record<string,string> rather than undefined, i.e. only return undefined when the input is not an object record, and ensure callers of readStringRecord (e.g., usages in gemini-mcp.provider.ts and codex-mcp.provider.ts) still handle an empty object result correctly by preserving fields like env/headers during read->write round-trips.
88-96:createNormalizedMessagesilently coercessessionId: nullto''.
fields.sessionId || ''treatsnull/undefinedidentically to'', which is fine, but several providernormalizeMessageimplementations in this PR passsessionId: nullthrough for bootstrap/pre-session events. Downstream consumers that checkif (msg.sessionId)will getfalseeither way, but those checkingmsg.sessionId === null(to distinguish "no session yet" from "empty session") will break. If this coercion is intentional, a JSDoc line noting it would help; otherwise preservenull.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/shared/utils.ts` around lines 88 - 96, The createNormalizedMessage function currently coerces sessionId null to an empty string via `fields.sessionId || ''`; change this to preserve explicit nulls by only defaulting when sessionId is undefined (e.g. `sessionId: fields.sessionId === undefined ? '' : fields.sessionId`) so callers that pass `null` remain distinguishable; update the NormalizedMessageInput/NormalizedMessage handling here (and keep generateMessageId(fields.kind) usage intact) and add a brief JSDoc note if the conversion was intentional instead of changing behavior.server/modules/providers/list/gemini/gemini.provider.ts (2)
130-234:fetchHistorysilently ignoreslimit/offsetfromFetchHistoryOptions.The parameter is renamed to
_optionsand returnsoffset: 0, limit: null, hasMore: falseunconditionally. For large Gemini sessions this loads and normalizes every message on every call, and upstream UI pagination will be a no-op. If pagination is genuinely unsupported for this provider, returningtotal: normalized.lengthwhile discardingoffset/limitis still fine, but please document that contract more explicitly (the JSDoc only mentions the source order, not pagination semantics) or slicenormalizedbyoffset/limitbefore returning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/providers/list/gemini/gemini.provider.ts` around lines 130 - 234, fetchHistory currently ignores FetchHistoryOptions (renamed to _options) and always returns offset: 0, limit: null, hasMore: false, causing no pagination; restore/use the options param in fetchHistory, extract offset and limit (default 0 and null/undefined), compute total = normalized.length before slicing, then slice normalized by offset/limit to produce the messages array, set hasMore = limit ? total > offset + limit : false, and return the sliced messages with the original total and the passed offset/limit; alternatively, if you intend to explicitly not support pagination, update the JSDoc for fetchHistory to state pagination is unsupported and keep returning full results but still return total = normalized.length and the passed offset/limit values (or null) so callers can detect the contract.
134-145: Defensive guard ifsessionManager.getSessionMessagesreturns a non-array.The cast
as RawProviderMessage[]is unchecked. If the session manager returnsundefined/null, therawMessages.length === 0check on Line 138 throws and is swallowed into the "failed to load" branch, which masks the real reason. Consider normalizing the result:- rawMessages = sessionManager.getSessionMessages(sessionId) as RawProviderMessage[]; - - if (rawMessages.length === 0) { + const fromMemory = sessionManager.getSessionMessages(sessionId); + rawMessages = Array.isArray(fromMemory) ? (fromMemory as RawProviderMessage[]) : []; + + if (rawMessages.length === 0) { rawMessages = await getGeminiCliSessionMessages(sessionId) as RawProviderMessage[]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/providers/list/gemini/gemini.provider.ts` around lines 134 - 145, The current unchecked cast of sessionManager.getSessionMessages(sessionId) into rawMessages can be non-array (null/undefined) and cause a thrown error that gets masked; change the logic in the GeminiProvider block to normalize the result before using .length: call sessionManager.getSessionMessages(sessionId) into a temp (e.g., msgs), set rawMessages = Array.isArray(msgs) ? msgs : [], then only call getGeminiCliSessionMessages(sessionId) when rawMessages.length === 0; reference sessionManager.getSessionMessages, rawMessages, and getGeminiCliSessionMessages when making the change so the fix is easy to locate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5a3b18ef-3456-411f-9611-f4666fed11f9
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (77)
eslint.config.jspackage.jsonserver/claude-sdk.jsserver/cursor-cli.jsserver/gemini-cli.jsserver/gemini-response-handler.jsserver/index.jsserver/modules/providers/list/claude/claude-auth.provider.tsserver/modules/providers/list/claude/claude-mcp.provider.tsserver/modules/providers/list/claude/claude.provider.tsserver/modules/providers/list/codex/codex-auth.provider.tsserver/modules/providers/list/codex/codex-mcp.provider.tsserver/modules/providers/list/codex/codex.provider.tsserver/modules/providers/list/cursor/cursor-auth.provider.tsserver/modules/providers/list/cursor/cursor-mcp.provider.tsserver/modules/providers/list/cursor/cursor.provider.tsserver/modules/providers/list/gemini/gemini-auth.provider.tsserver/modules/providers/list/gemini/gemini-mcp.provider.tsserver/modules/providers/list/gemini/gemini.provider.tsserver/modules/providers/provider.registry.tsserver/modules/providers/provider.routes.tsserver/modules/providers/services/mcp.service.tsserver/modules/providers/services/provider-auth.service.tsserver/modules/providers/services/sessions.service.tsserver/modules/providers/shared/base/abstract.provider.tsserver/modules/providers/shared/mcp/mcp.provider.tsserver/modules/providers/tests/mcp.test.tsserver/openai-codex.jsserver/providers/claude/adapter.jsserver/providers/claude/status.jsserver/providers/codex/adapter.jsserver/providers/codex/status.jsserver/providers/cursor/adapter.jsserver/providers/cursor/status.jsserver/providers/gemini/adapter.jsserver/providers/gemini/status.jsserver/providers/registry.jsserver/providers/types.jsserver/providers/utils.jsserver/routes/cli-auth.jsserver/routes/codex.jsserver/routes/commands.jsserver/routes/cursor.jsserver/routes/mcp-utils.jsserver/routes/mcp.jsserver/routes/messages.jsserver/routes/settings.jsserver/routes/taskmaster.jsserver/shared/interfaces.tsserver/shared/types.tsserver/shared/utils.tsserver/utils/mcp-detector.jssrc/components/chat/hooks/useChatMessages.tssrc/components/chat/view/subcomponents/ProviderSelectionEmptyState.tsxsrc/components/mcp/constants.tssrc/components/mcp/hooks/useMcpServerForm.tssrc/components/mcp/hooks/useMcpServers.tssrc/components/mcp/index.tssrc/components/mcp/types.tssrc/components/mcp/utils/mcpFormatting.tssrc/components/mcp/view/McpServers.tsxsrc/components/mcp/view/modals/McpServerFormModal.tsxsrc/components/provider-auth/hooks/useProviderAuthStatus.tssrc/components/provider-auth/types.tssrc/components/settings/constants/constants.tssrc/components/settings/hooks/useSettingsController.tssrc/components/settings/types/types.tssrc/components/settings/view/Settings.tsxsrc/components/settings/view/modals/ClaudeMcpFormModal.tsxsrc/components/settings/view/modals/CodexMcpFormModal.tsxsrc/components/settings/view/tabs/agents-settings/AgentsSettingsTab.tsxsrc/components/settings/view/tabs/agents-settings/sections/AgentCategoryContentSection.tsxsrc/components/settings/view/tabs/agents-settings/sections/AgentSelectorSection.tsxsrc/components/settings/view/tabs/agents-settings/sections/content/McpServersContent.tsxsrc/components/settings/view/tabs/agents-settings/types.tssrc/hooks/useServerPlatform.tssrc/utils/api.js
💤 Files with no reviewable changes (21)
- src/utils/api.js
- server/providers/utils.js
- server/utils/mcp-detector.js
- server/providers/cursor/status.js
- server/routes/cli-auth.js
- server/providers/types.js
- server/providers/registry.js
- server/routes/commands.js
- server/routes/taskmaster.js
- server/providers/gemini/adapter.js
- server/providers/gemini/status.js
- server/providers/codex/adapter.js
- server/providers/cursor/adapter.js
- server/providers/codex/status.js
- server/providers/claude/adapter.js
- src/components/settings/view/tabs/agents-settings/sections/content/McpServersContent.tsx
- server/providers/claude/status.js
- src/components/settings/view/modals/CodexMcpFormModal.tsx
- src/components/settings/view/modals/ClaudeMcpFormModal.tsx
- src/components/settings/types/types.ts
- server/routes/mcp.js
…m/siteboon/claudecodeui into refactor/unified-provider-runtime
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/modules/providers/provider.routes.ts`:
- Around line 103-130: The env/headers/envHttpHeaders (and similarly
envVars/env) validation in provider.routes.ts currently treats any object
(including arrays) as valid, causing arrays like env: ["TOKEN"] to be coerced
into numeric-keyed objects; change the validation to explicitly reject
non-plain-record shapes: add or use a helper (e.g., isPlainStringRecord(obj))
that ensures the value is a non-null object, not an array, and that every value
is a string, then use it in place of the current typeof checks for env, headers,
envHttpHeaders (and any other record-like fields) to either return undefined or
immediately return a 400/bad-request error when the payload fails the check so
malformed MCP config cannot be persisted.
In `@server/shared/types.ts`:
- Around line 115-129: ProviderMcpServer currently exposes secret-bearing fields
(env, headers, bearerTokenEnvVar, envHttpHeaders) that would leak API keys to
the frontend; remove those secret fields from the ProviderMcpServer type and
replace them with a redacted/list-safe shape (e.g., keep envVars or headerNames
as string[] or boolean flags) and move the full secret-bearing fields into the
write-only/internal types such as UpsertProviderMcpServerInput or the internal
persisted config type; update any serializers/validators that construct
ProviderMcpServer (look for usages of ProviderMcpServer,
UpsertProviderMcpServerInput) to ensure secrets are only accepted/stored via the
input/internal type and never returned to the frontend.
In `@server/shared/utils.ts`:
- Around line 186-188: The writeJsonConfig function must ensure secret-bearing
files are created with strict permissions: when creating the parent directory
use a restrictive mode (e.g., 0o700) and when writing the file pass a mode of
0o600 to writeFile, then normalize any existing permissions by calling
chmod(filePath, 0o600) after the write; update the writeJsonConfig
implementation to set these modes for the directory and file and to call
chmod(filePath, 0o600) to enforce 0o600 regardless of process umask or prior
permissions.
- Around line 72-79: createNormalizedMessage currently coerces null/undefined
sessionId to an empty string which creates ambiguous fake session keys; update
createNormalizedMessage to preserve null for sessionId when fields.sessionId is
null or undefined (i.e., set sessionId: fields.sessionId ?? null rather than
''), and ensure the NormalizedMessage/NormalizedMessageInput type annotations
reflect sessionId: string | null so callers and downstream code treat absent
sessions as null (use generateMessageId(fields.kind) and id/sessionId/timestamp
handling as before).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 74ca4018-5d8e-45fe-b4df-79e88daa4dcc
📒 Files selected for processing (4)
server/modules/providers/provider.routes.tsserver/shared/types.tsserver/shared/utils.tssrc/components/provider-auth/hooks/useProviderAuthStatus.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/provider-auth/hooks/useProviderAuthStatus.ts
…ider-runtime # Conflicts: # src/components/chat/view/subcomponents/ProviderSelectionEmptyState.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/modules/providers/list/claude/claude-sessions.provider.ts`:
- Around line 72-100: User text parts currently all get the same id
`${baseId}_text` inside the loop that processes raw.message.content, causing
duplicate normalized IDs; update the ID generation in the branch handling
part.type === 'text' (the block that calls createNormalizedMessage for kind
'text') to append a unique discriminator (e.g., the loop index, part-specific
id, or a sequence) to `${baseId}_text` so each text part produces a unique id
while keeping sessionId, timestamp, provider, kind and role unchanged.
In `@server/modules/providers/list/cursor/cursor-sessions.provider.ts`:
- Around line 37-40: Validate and sanitize sessionId before building storeDbPath
to prevent path traversal: ensure the sessionId used in the construction of
storeDbPath (and passed to new Database(...)) contains only allowed characters
(e.g. alphanumeric, hyphen, underscore) or use path.basename to strip segments
and reject values containing path separators or '..'; alternatively resolve the
computed path and assert it is inside the intended base directory (constructed
from os.homedir(), '.cursor', 'chats', cwdId') and throw an error if validation
fails so Database is never opened with a crafted sessionId.
- Around line 190-206: The code currently treats limit === 0 as “no limit” and
returns the full history; update the pagination branch in the function that uses
loadCursorBlobs and normalizeCursorBlobs so zero is honored as an empty page:
change the condition that checks limit (currently `limit !== null && limit > 0`)
to detect `limit !== null` and then handle `limit === 0` by returning an empty
messages array with correct total, hasMore (start < total), offset and limit;
otherwise slice with `start = offset` and `allNormalized.slice(start, start +
limit)` as before and compute hasMore as `start + limit < total`. Ensure the
same symbols are used: `options` destructuring (`projectPath`, `limit`,
`offset`), `loadCursorBlobs(sessionId, projectPath)`,
`normalizeCursorBlobs(blobs, sessionId)`, and the local `start`, `page`,
`total`, `hasMore`, `offset`, `limit`.
- Around line 45-83: The loop currently builds blobMap and parses parents in one
pass, so parent references that point to later blobs are ignored; change to a
two-pass approach in the cursor-sessions provider: first iterate allBlobs and
populate blobMap (and collect JSON blobs into jsonBlobs), then in a second loop
iterate allBlobs again to parse binary parent hashes and populate parentRefs and
childRefs using the now-complete blobMap; ensure you reference
CursorDbBlob/CursorJsonBlob, allBlobs, blobMap, parentRefs and childRefs when
locating and moving the parsing logic so parent existence checks use the fully
populated blobMap.
In `@server/modules/providers/list/gemini/gemini-sessions.provider.ts`:
- Around line 114-117: The fetchHistory implementation in
gemini-sessions.provider.ts ignores pagination; update async
fetchHistory(sessionId: string, _options: FetchHistoryOptions = {}) (and the
duplicate implementation around the 211-217 range) to read options.offset and
options.limit (default offset=0, limit=null), apply them to the normalized
history array (slice from offset, apply limit if non-null), and return the
FetchHistoryResult with the sliced history plus the returned offset and limit
values; ensure you reference the FetchHistoryOptions.offset/limit and set
FetchHistoryResult.offset/limit accordingly when returning.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8b82aaea-c02d-4ef0-b0ad-07515d16da7c
📒 Files selected for processing (13)
server/modules/providers/list/claude/claude-sessions.provider.tsserver/modules/providers/list/claude/claude.provider.tsserver/modules/providers/list/codex/codex-sessions.provider.tsserver/modules/providers/list/codex/codex.provider.tsserver/modules/providers/list/cursor/cursor-sessions.provider.tsserver/modules/providers/list/cursor/cursor.provider.tsserver/modules/providers/list/gemini/gemini-sessions.provider.tsserver/modules/providers/list/gemini/gemini.provider.tsserver/modules/providers/services/sessions.service.tsserver/modules/providers/shared/base/abstract.provider.tsserver/shared/interfaces.tsserver/shared/types.tsserver/shared/utils.ts
✅ Files skipped from review due to trivial changes (1)
- server/modules/providers/list/codex/codex.provider.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- server/modules/providers/list/gemini/gemini.provider.ts
- server/modules/providers/shared/base/abstract.provider.ts
- server/shared/types.ts
- server/shared/utils.ts
…on/pagination
Address correctness and safety issues in provider session adapters while
preserving existing normalized message shapes.
Claude sessions:
- Ensure user text content parts generate unique normalized message ids.
- Replace duplicate `${baseId}_text` ids with index-suffixed ids to avoid
collisions when one user message contains multiple text segments.
Cursor sessions:
- Add session id sanitization before constructing SQLite paths to prevent
path traversal via crafted session ids.
- Enforce containment by resolving the computed DB path and asserting it stays
under ~/.cursor/chats/<cwdId>.
- Refactor blob parsing to a two-pass flow: first build blobMap and collect
JSON blobs, then parse binary parent refs against the fully populated map.
- Fix pagination semantics so limit=0 returns an empty page instead of full
history, with consistent total/hasMore/offset/limit metadata.
Gemini sessions:
- Honor FetchHistoryOptions pagination by reading limit/offset and slicing
normalized history accordingly.
- Return consistent hasMore/offset/limit metadata for paged responses.
Validation:
- eslint passed for touched files.
- server TypeScript check passed (tsc --noEmit -p server/tsconfig.json).
This was referenced Apr 29, 2026
This was referenced May 12, 2026
This was referenced May 12, 2026
stud20
pushed a commit
to stud20/claudecodeui
that referenced
this pull request
May 25, 2026
…teboon#666) * feat: implement MCP provider registry and service - Add provider registry to manage LLM providers (Claude, Codex, Cursor, Gemini). - Create provider routes for MCP server operations (list, upsert, delete, run). - Implement MCP service for handling server operations and validations. - Introduce abstract provider class and MCP provider base for shared functionality. - Add tests for MCP server operations across different providers and scopes. - Define shared interfaces and types for MCP functionality. - Implement utility functions for handling JSON config files and API responses. * chore: remove dead code related to MCP server * refactor: put /api/providers in index.js and remove /providers prefix from provider.routes.ts * refactor(settings): move MCP server management into provider module Extract MCP server settings out of the settings controller and agents tab into a dedicated frontend MCP module. The settings UI now delegates MCP rendering and behavior to a single module that only needs the selected provider and current projects. Changes: - Add `src/components/mcp` as the single frontend MCP module - Move MCP server list rendering into `McpServers` - Move MCP add/edit modal into `McpServerFormModal` - Move MCP API/state logic into `useMcpServers` - Move MCP form state/validation logic into `useMcpServerForm` - Add provider-specific MCP constants, types, and formatting helpers - Use the unified `/api/providers/:provider/mcp/servers` API for all providers - Support MCP management for Claude, Cursor, Codex, and Gemini - Remove old settings-owned Claude/Codex MCP modal components - Remove old provider-specific `McpServersContent` branching from settings - Strip MCP server state, fetch, save, delete, and modal ownership from `useSettingsController` - Simplify agents settings props so MCP only receives `selectedProvider` and `currentProjects` - Keep Claude working-directory unsupported while preserving cwd support for Cursor, Codex, and Gemini - Add progressive MCP loading: - render user/global scope first - load project/local scopes in the background - append project results as they resolve - cache MCP lists briefly to avoid slow tab-switch refetches - ignore stale async responses after provider switches Verification: - `npx eslint src/components/mcp` - `npm run typecheck` - `npm run build:client` * fix(mcp): form with multiline text handling for args, env, headers, and envVars * feat(mcp): add global MCP server creation flow Add a separate global MCP add path in the settings MCP module so users can create one shared MCP server configuration across Claude, Cursor, Codex, and Gemini from the same screen. The provider-specific add flow is still kept next to it because these two actions have different intent. A global MCP server must be constrained to the subset of configuration that every provider can accept, while a provider-specific server can still use that provider's own supported scopes, transports, and fields. Naming the buttons as "Add Global MCP Server" and "Add <Provider> MCP Server" makes that distinction explicit without forcing users to infer it from the selected tab. This also moves the explanatory copy to button hover text to keep the MCP toolbar compact while still documenting the difference between global and provider-only adds at the point of action. Implementation details: - Add global MCP form mode with shared user/project scopes and stdio/http transports. - Submit global creates through `/api/providers/mcp/servers/global`. - Reuse the existing MCP form modal with configurable scopes, transports, labels, and descriptions instead of duplicating form logic. - Disable provider-only fields for the global flow because those fields cannot be safely written to every provider. - Clear the MCP server cache globally after a global add because every provider tab may have changed. - Surface partial global add failures with provider-specific error messages. Validation: - npx eslint src/components/mcp/view/McpServers.tsx - npm run typecheck - npm run build:client * feat: implement platform-specific provider visibility for cursor agent * refactor(providers): centralize message handling in provider module Move provider-specific normalizeMessage and fetchHistory logic out of the legacy server/providers adapters and into the refactored provider classes so callers can depend on the main provider contract instead of parallel adapter plumbing. Add a providers service to resolve concrete providers through the registry and delegate message normalization/history loading from realtime handlers and the unified messages route. Add shared TypeScript message/history types and normalized message helpers so provider implementations and callers use the same contract. Remove the old adapter registry/files now that Claude, Codex, Cursor, and Gemini implement the required behavior directly. * refactor(providers): move auth status checks into provider runtimes Move provider authentication status logic out of the CLI auth route so auth checks live with the provider implementations that understand each provider's install and credential model. Add provider-specific auth runtime classes for Claude, Codex, Cursor, and Gemini, and expose them through the shared provider contract as `provider.auth`. Add a provider auth service that resolves providers through the registry and delegates status checks via `auth.getStatus()`. Keep the existing `/api/cli/<provider>/status` endpoints, but make them thin route adapters over the new provider auth service. This removes duplicated route-local credential parsing and makes auth status a first-class provider capability beside MCP and message handling. * refactor(providers): clarify provider auth and MCP naming Rename provider auth/MCP contracts to remove the overloaded Runtime suffix so the shared interfaces read as stable provider capabilities instead of execution implementation details. Add a consistent provider-first auth class naming convention by renaming ClaudeAuthProvider, CodexAuthProvider, CursorAuthProvider, and GeminiAuthProvider to ClaudeProviderAuth, CodexProviderAuth, CursorProviderAuth, and GeminiProviderAuth. This keeps the provider module API easier to scan and aligns auth naming with the main provider ownership model. * refactor(providers): move session message delegation into sessions service Move provider-backed session history and message normalization calls out of the generic providers service so the service name reflects the behavior it owns. Add a dedicated sessions service for listing session-capable providers, normalizing live provider events, and fetching persisted session history through the provider registry. Update realtime handlers and the unified messages route to depend on `sessionsService` instead of `providersService`. This separates session message operations from other provider concerns such as auth and MCP, keeping the provider services easier to navigate as the module grows. * refactor(providers): move auth status routes under provider API Move provider authentication status endpoints out of the legacy `/api/cli` route namespace so auth status is exposed through the same provider module that owns provider auth and MCP behavior. Add `GET /api/providers/:provider/auth/status` to the provider router and route it through the provider auth service. Remove the old `cli-auth` route file and `/api/cli` mount now that provider auth status is handled by the unified provider API. Update the frontend provider auth endpoint map to call the new provider-scoped routes and rename the endpoint constant to reflect that it is no longer CLI specific. * chore(api): remove unused backend endpoints after MCP audit Remove legacy backend routes that no longer have frontend or internal callers, including the old Claude/Codex MCP APIs, unused Cursor and Codex helper endpoints, stale TaskMaster detection/next/initialize routes, and unused command/project helpers. This reduces duplicated MCP behavior now handled by the provider-based MCP API, shrinks the exposed backend surface, and removes probe/service code that only existed for deleted endpoints. Add an MCP settings API audit document to capture the route-usage analysis and explain why the legacy MCP endpoints were considered safe to remove. * refactor(providers): remove debug logging from Claude authentication status checks * refactor(cursor): lazy-load better-sqlite3 and remove unused type definitions * refactor(cursor): remove SSE from CursorMcpProvider constructor and error message * refactor(auth): standardize API response structure and remove unused error handling * refactor: make providers use dedicated session handling classes * refactor: remove legacy provider selection UI and logic * fix(server/providers): harden and correct session history normalization/pagination Address correctness and safety issues in provider session adapters while preserving existing normalized message shapes. Claude sessions: - Ensure user text content parts generate unique normalized message ids. - Replace duplicate `${baseId}_text` ids with index-suffixed ids to avoid collisions when one user message contains multiple text segments. Cursor sessions: - Add session id sanitization before constructing SQLite paths to prevent path traversal via crafted session ids. - Enforce containment by resolving the computed DB path and asserting it stays under ~/.cursor/chats/<cwdId>. - Refactor blob parsing to a two-pass flow: first build blobMap and collect JSON blobs, then parse binary parent refs against the fully populated map. - Fix pagination semantics so limit=0 returns an empty page instead of full history, with consistent total/hasMore/offset/limit metadata. Gemini sessions: - Honor FetchHistoryOptions pagination by reading limit/offset and slicing normalized history accordingly. - Return consistent hasMore/offset/limit metadata for paged responses. Validation: - eslint passed for touched files. - server TypeScript check passed (tsc --noEmit -p server/tsconfig.json). ---------
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
4 participants
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
This PR moves provider-specific behavior into the refactored provider module so Claude, Cursor, Codex, and Gemini all expose the same runtime contract for session history, realtime message normalization, auth status, and MCP server management.
The main reason for this change is that provider logic had become split across legacy adapter files, route-local auth checks, frontend-specific MCP settings code, and runtime files. That made it harder to add provider behavior consistently because each new capability needed changes in multiple unrelated places. This PR makes provider capabilities first-class parts of the provider contract instead.
Why
Provider-specific behavior needs to live with the provider implementation that understands it.
Before this change:
That made the system harder to extend because callers needed to know where each provider capability happened to live.
This PR introduces a more consistent provider runtime model where each provider owns its own implementation, and shared services delegate through the registry.
What Changed
Provider Runtime Contract
Added provider-level contracts for:
normalizeMessagefetchHistoryauthmcpEach provider class now implements the runtime behavior it owns.
Key files:
interfaces.tsabstract.provider.tsprovider.registry.tsclaude.provider.tscursor.provider.tscodex.provider.tsgemini.provider.tsSession Message Handling
Moved message normalization and history fetching into provider classes and exposed them through a sessions service.
This lets runtime code and routes call a single service instead of importing provider-specific adapters directly.
Key files:
sessions.service.tsmessages.jsclaude-sdk.jscursor-cli.jsopenai-codex.jsgemini-response-handler.jsProvider Auth
Moved provider auth status checks into provider-specific auth classes.
The auth API now resolves a provider from the registry and calls the provider’s auth runtime instead of relying on the old CLI auth route implementation.
Key files:
provider-auth.service.tsclaude-auth.provider.tscursor-auth.provider.tscodex-auth.provider.tsgemini-auth.provider.tsprovider.routes.tstypes.tsThe frontend auth endpoints now use: