feat(mcp): support v2 ecosystem - #2069
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR migrates DeepChat's MCP integration to v2 split SDK packages with dual-era protocol negotiation, replaces MCP's autoApprove permission model with a centralized ToolPermissionBroker, adds an MCP Apps sandboxed host/renderer bridge, introduces MCP authorization extensions (OAuth client-credentials, private-key JWT, enterprise identity), documents a gated MCP Tasks extension, and updates shared contracts, i18n, and tests. ChangesMCP v2 Migration and Ecosystem Overhaul
Estimated code review effort: 5 (Critical) | ~180 minutes Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer (McpAppView)
participant Broker as ToolPermissionBroker
participant McpService as McpService/AppHost
participant SandboxReg as McpAppSandboxRegistry
participant Sdk as MCP v2 Client SDK
Renderer->>McpService: prepareAppView(descriptor, toolInput)
McpService->>SandboxReg: create instance
SandboxReg-->>McpService: sandbox instance
McpService-->>Renderer: prepared view (sandbox, csp)
Renderer->>McpService: callAppTool(name, args)
McpService->>Broker: requestAppDecision(context)
Broker->>Renderer: mcp.app.consent.request event
Renderer->>Broker: submitAppConsent(approved)
Broker-->>McpService: decision resolved
McpService->>Sdk: callTool(toolDefinition, expectedTarget)
Sdk-->>McpService: ToolCallResult
McpService-->>Renderer: PersistedMcpToolResult
sequenceDiagram
participant User
participant Form as McpServerForm.vue
participant OAuthMgr as McpOAuthManager
participant Store as McpOAuthCredentialStore
participant IdP as OIDC Provider
User->>Form: select authorization mode + submit
Form->>OAuthMgr: setCredential / startAuth
OAuthMgr->>IdP: discover metadata / authorize
IdP-->>OAuthMgr: authorization code / tokens
OAuthMgr->>Store: saveEntry (encrypted envelope v2)
Store-->>OAuthMgr: ack
OAuthMgr-->>Form: McpServerAuthStatus (authenticated)
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📄 Knowledge reviewDosu skipped reviewing this PR because your organization has used its
|
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/features/mcp-oauth-authentication/plan.md (1)
325-332: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not show a success page for rejected callbacks.
Invalid callback input is explicitly rejected, but the documented response still says “Authentication complete.” Return a generic failure/completion-neutral page instead; otherwise users may believe authentication succeeded when no tokens were stored.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/features/mcp-oauth-authentication/plan.md` around lines 325 - 332, Update the callback response documentation so rejected or invalid callback input displays a generic failure or completion-neutral page rather than “Authentication complete.” Keep the success-page copy only for callbacks where authentication succeeds, and avoid exposing sensitive failure details in the browser response.
🟡 Minor comments (14)
src/renderer/src/stores/mcpElicitation.ts-226-236 (1)
226-236: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequired multi-select passes validation when empty.
missingonly coversundefined/null/blank strings, so a required array field with[](its own default) submits as empty unless the schema also setsminItems.🔧 Proposed fix
const missing = - value === undefined || value === null || (typeof value === 'string' && !value.trim()) + value === undefined || + value === null || + (typeof value === 'string' && !value.trim()) || + (Array.isArray(value) && value.length === 0)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/stores/mcpElicitation.ts` around lines 226 - 236, Update the validation loop over fields in mcpElicitation so missing also treats an empty array as missing, ensuring required multi-select fields with [] receive the existing required error. Preserve current handling for undefined, null, blank strings, and non-empty values.src/renderer/src/components/mcp/McpElicitationDialog.vue-88-99 (1)
88-99: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMulti-select won't reflect stored values.
Binding
:valueon a native<select multiple>doesn't set selected options (the DOMvalueproperty is a single string). Schema defaults and any programmatic state won't render as selected. Bindselectedper option instead.🔧 Proposed fix
<select v-else-if="field.type === 'multi-select'" :id="`mcp-elicit-${field.name}`" multiple class="min-h-24 w-full rounded-md border bg-background px-3 py-2 text-sm" - :value="store.values[field.name] as string[]" `@change`="updateMultiValue(field.name, $event)" > - <option v-for="option in field.options" :key="option.value" :value="option.value"> + <option + v-for="option in field.options" + :key="option.value" + :value="option.value" + :selected=" + Array.isArray(store.values[field.name]) && + (store.values[field.name] as string[]).includes(option.value) + " + > {{ option.title }} </option> </select>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/mcp/McpElicitationDialog.vue` around lines 88 - 99, Update the multi-select rendering in McpElicitationDialog’s field.options loop to set each option’s selected state by checking whether its value exists in store.values[field.name], rather than binding the select’s single :value property. Preserve updateMultiValue(field.name, $event) for change handling.src/types/i18n.d.ts-1872-1917 (1)
1872-1917: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLocale parity issue remains for removed keys.
elicitation,apps,enterpriseProfiles,diagnostics, andauthorizationHelpare present across the locale files, butautoApprove*keys are still present in all locale JSON files. If these are removed from the UI/schema, remove the locale definitions as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/types/i18n.d.ts` around lines 1872 - 1917, Remove the obsolete autoApprove* translation definitions from every locale JSON file, keeping the locale schemas synchronized with the UI/schema. Preserve the existing elicitation, apps, enterpriseProfiles, diagnostics, and authorizationHelp definitions unless they are also removed from the corresponding schema.src/main/mcp/schemaValidation.ts-10-16 (1)
10-16: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDialect allowlist is inconsistent:
http://json-schema.org/draft-07/schema(without#) is missing. Bothhttpsdraft-07 variants are allowed but only the#-suffixedhttpform is, so servers publishing the unpadded canonical-host URI get their tools rejected at Line 202.🐛 Proposed fix
const SUPPORTED_JSON_SCHEMA_DIALECTS = new Set([ 'https://json-schema.org/draft/2020-12/schema', 'https://json-schema.org/draft/2020-12/schema#', + 'http://json-schema.org/draft-07/schema', 'http://json-schema.org/draft-07/schema#', 'https://json-schema.org/draft-07/schema', 'https://json-schema.org/draft-07/schema#' ])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/mcp/schemaValidation.ts` around lines 10 - 16, Update the SUPPORTED_JSON_SCHEMA_DIALECTS allowlist to include the missing `http://json-schema.org/draft-07/schema` URI alongside the existing draft-07 variants, preserving all currently supported dialects.src/main/mcp/mcpClient.ts-1001-1023 (1)
1001-1023: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMalformed elicitation URLs throw a raw
TypeErrorinstead ofInvalidParams.
new URL(params.url)is unguarded, so a server sending a non-absolute URL surfaces as an internal error rather than theProtocolError(InvalidParams, …)used for every other validation failure here. Also consider hoisting the inline1024 * 1024at Line 1080 into a namedMCP_ELICITATION_MAX_CONTENT_BYTESconstant alongside the other host limits.🐛 Proposed fix
let url: string | undefined if (params.mode === 'url') { - const candidate = new URL(params.url) + let candidate: URL + try { + candidate = new URL(params.url) + } catch { + throw new ProtocolError(ProtocolErrorCode.InvalidParams, 'Elicitation URL is not a valid absolute URL') + } if (candidate.protocol !== 'https:' && candidate.protocol !== 'http:') {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/mcp/mcpClient.ts` around lines 1001 - 1023, Update the URL handling in the elicitation request flow around params.mode === 'url' to catch invalid input from new URL(params.url) and rethrow it as ProtocolError with ProtocolErrorCode.InvalidParams, preserving the existing validation message style. Also replace the inline 1024 * 1024 content limit near the elicitation response handling with a named MCP_ELICITATION_MAX_CONTENT_BYTES constant alongside the other host-limit constants, and use that constant for the check.src/main/mcp/index.ts-698-703 (1)
698-703: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFetch the full MCP catalog for server listing.
getMcpClients()buildsMcpClient.toolsfromtoolManager.getAllToolDefinitions(), which skips tools whose visibility does not include'model'and renames conflicting tools. Server entries are routed throughMcpsClientsRoute.handle(), so app-only tool-only apps/tools can disappear from the server UI even if the renderer later consumes all tool definitions separately. If the server config list should mirror each server’s available catalog, populate these tools without the model-visibility filter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/mcp/index.ts` around lines 698 - 703, Update getMcpClients() to populate each McpClient.tools list from the complete MCP tool catalog rather than toolManager.getAllToolDefinitions(), which filters by model visibility and renames conflicts. Use the underlying all-tool-definitions source already available to the tool manager, while preserving the existing serverName matching and client construction flow.test/setup.ts-201-205 (1)
201-205: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winIdentity
safeStoragemock makes the credential-store secrecy assertion non-protective.encryptStringreturns the input bytes unchanged, so any test asserting that a secret is absent from persisted content passes only because of the envelope's byte encoding, not because encryption happened.
test/setup.ts#L201-L205: makeencryptString/decryptStringapply a reversible but visible transform (e.g. a prefix plus reversal) so encrypted output never contains the plaintext substring.test/main/mcp/oauthCredentialStore.test.ts#L82-L82: keep thenot.toContain('protected-value')assertion, which becomes a genuine plaintext-leak detector once the mock transforms its input.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/setup.ts` around lines 201 - 205, The safeStorage mock’s encryptString/decryptString methods currently preserve plaintext, making secrecy assertions non-protective. In test/setup.ts lines 201-205, update both methods to use a reversible visible transform, such as adding a prefix and reversing the bytes, ensuring encrypted output never contains the original plaintext while decryptString restores it; in test/main/mcp/oauthCredentialStore.test.ts line 82, make no change and retain the existing not.toContain('protected-value') assertion.test/main/mcp/oauthCredentialStore.test.ts-124-136 (1)
124-136: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest name and exercised path don't match.
isEncryptionAvailableisfalsehere (thebeforeEachdefault), so this asserts the encryption-unavailable discard path — which is exactly whydecryptStringis never called. It does not cover an envelope that is genuinely unreadable/corrupt. SetisEncryptionAvailabletotrueand makedecryptStringthrow to test what the title claims, or rename the test.🐛 Suggested fix
it('removes an unreadable persistent envelope instead of resurrecting stale credentials', () => { + vi.mocked(safeStorage.isEncryptionAvailable).mockReturnValue(true) + vi.mocked(safeStorage.decryptString).mockImplementationOnce(() => { + throw new Error('cannot decrypt') + }) savedContent = JSON.stringify({ version: 2, storage: 'safeStorage', wrapped: 'encrypted', updatedAt: 42 }) const store = new McpOAuthCredentialStore('/tmp/deepchat-mcp-oauth/credentials.json') expect(store.load('stale')).toBeNull() expect(fs.unlinkSync).toHaveBeenCalledWith('/tmp/deepchat-mcp-oauth/credentials.json') - expect(safeStorage.decryptString).not.toHaveBeenCalled() })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/mcp/oauthCredentialStore.test.ts` around lines 124 - 136, Align the test case around McpOAuthCredentialStore.load with its “unreadable persistent envelope” title by setting isEncryptionAvailable to true and configuring safeStorage.decryptString to throw for the saved envelope. Preserve the assertions that load returns null and removes the file, while verifying decryption is attempted; alternatively, rename the test to describe the encryption-unavailable discard path.src/main/mcp/mcpOAuthManager.ts-431-446 (1)
431-446: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA failed post-auth restart is reported as an authorization error.
onAuthenticated(server restart) runs inside the sametryascreateRuntimeProvider, so a transient restart failure setsstate: 'error'even though machine authorization succeeded, andgetStatuswill keep returning that error state.🐛 Proposed fix
if (mode !== 'interactive') { try { await this.createRuntimeProvider(serverName, completeConfig) - await Promise.resolve(this.onAuthenticated?.(serverName)) } catch (error) { this.setStatus( this.buildStatus(serverName, binding, mode, { state: 'error', authenticated: false, error: sanitizeError(error), credential: this.getSelectedCredentialStatus(binding, mode) }) ) + return this.getStatus(serverName, config) } + void Promise.resolve(this.onAuthenticated?.(serverName)).catch((error) => { + logger.warn('[MCP OAuth] Failed to restart server after authentication:', sanitizeError(error)) + }) return this.getStatus(serverName, config) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/mcp/mcpOAuthManager.ts` around lines 431 - 446, Update the non-interactive flow around createRuntimeProvider and onAuthenticated so restart failures do not overwrite successful authorization with an authorization error. Handle provider creation failures as before, but invoke onAuthenticated separately and preserve the authenticated status when only the post-auth restart fails; ensure getStatus continues reporting the successful authorization state.src/main/mcp/oauthCredentialStore.ts-452-546 (1)
452-546: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winA transient load failure silently discards every stored credential.
The blanket
catchsetsrecords = {}and leavesloaded = true; the nextsaveRecord/clearEntrythen callspersist()and rewrites the envelope with only the new record, permanently dropping all other credentials. This also swallows failures of the in-trypersist()on line 539 after legacy migration. Tracking the failure and refusing to persist over an unread file keeps recovery possible.🛡️ Sketch
+ private loadFailed = false ... } catch { this.records = {} + this.loadFailed = true } } private persist(): void { + if (this.loadFailed) { + return + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/mcp/oauthCredentialStore.ts` around lines 452 - 546, Update ensureLoaded so any read, parse, decrypt, migration, or in-try persist failure is recorded as a load failure instead of silently treating records as empty. Make saveRecord, clearEntry, and any related persist path refuse to overwrite the credential file while that failure is present, allowing a later retry or recovery. Preserve normal empty-file behavior and successful loading, migration, and persistence.src/renderer/src/components/mcp-config/components/McpServers.vue-519-532 (1)
519-532: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPrevent stale diagnostics from being shown for another server.
If server A is loading and the dialog is reopened for server B, Line 520 suppresses B’s request; A’s response can then populate B’s dialog. Capture a request generation/server name and ignore stale responses.
Proposed fix
+let diagnosticsRequestGeneration = 0 + const refreshDiagnostics = async () => { - if (!diagnosticsServerName.value || isDiagnosticsLoading.value) { + const serverName = diagnosticsServerName.value + if (!serverName) { return } + const requestGeneration = ++diagnosticsRequestGeneration isDiagnosticsLoading.value = true diagnosticsError.value = '' try { - diagnostics.value = await mcpClient.getServerDiagnostics(diagnosticsServerName.value) + const result = await mcpClient.getServerDiagnostics(serverName) + if (requestGeneration === diagnosticsRequestGeneration) diagnostics.value = result } catch (error) { - diagnostics.value = null - diagnosticsError.value = error instanceof Error ? error.message : String(error) + if (requestGeneration === diagnosticsRequestGeneration) { + diagnostics.value = null + diagnosticsError.value = error instanceof Error ? error.message : String(error) + } } finally { - isDiagnosticsLoading.value = false + if (requestGeneration === diagnosticsRequestGeneration) isDiagnosticsLoading.value = false } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/mcp-config/components/McpServers.vue` around lines 519 - 532, Update refreshDiagnostics to track the diagnostics request generation and server name when starting a request. Allow a newly selected server to initiate its own request even while a previous server is loading, and ignore any response or error from an older request if the generation or captured server name no longer matches the current diagnostics context.src/renderer/src/i18n/da-DK/mcp.json-303-348 (1)
303-348: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTranslate the new MCP strings in every non-English locale.
The new
elicitationandappsvalues are English in these locale files, so the new MCP flows render untranslated copy:
src/renderer/src/i18n/da-DK/mcp.json#L303-L348: add Danish translations.src/renderer/src/i18n/de-DE/mcp.json#L303-L348: add German translations.src/renderer/src/i18n/es-ES/mcp.json#L303-L348: add Spanish translations.src/renderer/src/i18n/fa-IR/mcp.json#L303-L348: add Persian translations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/i18n/da-DK/mcp.json` around lines 303 - 348, Translate every newly added value under the elicitation and apps keys from English into the target locale. Apply the Danish translations in src/renderer/src/i18n/da-DK/mcp.json lines 303-348, German translations in src/renderer/src/i18n/de-DE/mcp.json lines 303-348, Spanish translations in src/renderer/src/i18n/es-ES/mcp.json lines 303-348, and Persian translations in src/renderer/src/i18n/fa-IR/mcp.json lines 303-348, preserving the existing JSON structure and keys.src/renderer/src/i18n/da-DK/settings.json-1344-1378 (1)
1344-1378: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTranslate the new authorization and diagnostics strings in every affected locale.
The newly added OAuth, credential, enterprise identity, and diagnostics values are English in these non-English settings files:
src/renderer/src/i18n/da-DK/settings.json#L1344-L1378: add Danish translations.src/renderer/src/i18n/da-DK/settings.json#L1480-L1524: add Danish translations.src/renderer/src/i18n/de-DE/settings.json#L1755-L1789: add German translations.src/renderer/src/i18n/de-DE/settings.json#L1892-L1936: add German translations.src/renderer/src/i18n/de-DE/settings.json#L3042-L3086: add German translations.src/renderer/src/i18n/es-ES/settings.json#L1755-L1789: add Spanish translations.src/renderer/src/i18n/es-ES/settings.json#L1892-L1936: add Spanish translations.src/renderer/src/i18n/es-ES/settings.json#L3042-L3086: add Spanish translations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/i18n/da-DK/settings.json` around lines 1344 - 1378, Translate all newly added authorization, credential, enterprise identity, and diagnostics strings instead of leaving English values: update Danish translations in src/renderer/src/i18n/da-DK/settings.json ranges 1344-1378 and 1480-1524; German translations in src/renderer/src/i18n/de-DE/settings.json ranges 1755-1789, 1892-1936, and 3042-3086; and Spanish translations in src/renderer/src/i18n/es-ES/settings.json ranges 1755-1789, 1892-1936, and 3042-3086. Preserve every existing key and JSON structure.src/renderer/src/components/mcp/McpAppConsentDialog.vue-18-20 (1)
18-20: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd
kindlabels to all MCP consent locales.
McpAppConsentKindhascamera,clipboard-write,geolocation,microphone,open-link,send-message,tool-call, andupdate-model-context; the component callsmcp.apps.consent.kind.${store.request.kind}inside the description. Add theapps.consent.kind.*entries for every locale so consent requests do not render raw locale keys.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/mcp/McpAppConsentDialog.vue` around lines 18 - 20, Add the missing mcp.apps.consent.kind entries for camera, clipboard-write, geolocation, microphone, open-link, send-message, tool-call, and update-model-context in every locale resource used by McpAppConsentDialog.vue. Ensure each locale provides human-readable labels matching the keys generated by the kindLabel computed property.
🧹 Nitpick comments (22)
src/renderer/src/components/mcp/McpElicitationDialog.vue (1)
68-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider shadcn-vue
Checkbox/Selectfor these native controls.The rest of the file already uses
Input/Label/Buttonprimitives; the raw checkbox and selects diverge visually and lack the primitives' focus/a11y handling. As per coding guidelines, "prefer existing shadcn-vue primitives and VueUse utilities".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/mcp/McpElicitationDialog.vue` around lines 68 - 75, Replace the native boolean checkbox and select controls in McpElicitationDialog with the existing shadcn-vue Checkbox and Select primitives, preserving their current values and update handlers. Follow the file’s established Input/Label/Button import and usage patterns, and retain the existing field-name associations and conditional rendering.Source: Coding guidelines
src/renderer/src/stores/mcpSampling.ts (1)
297-333: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winQueue lifecycle is duplicated with the elicitation store.
openNextRequest/finishRequest/queueOrOpenRequestand the cap logic mirrorsrc/renderer/src/stores/mcpElicitation.ts(lines 176-212) almost line for line. A small shared helper parameterized byrequestIdextraction, open callback, and cancel callback would keep the two in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/stores/mcpSampling.ts` around lines 297 - 333, Extract the duplicated queue lifecycle logic from openNextRequest, finishRequest, and queueOrOpenRequest into a shared helper reusable by both the MCP sampling and elicitation stores. Parameterize the helper with requestId extraction, request-opening, and cancellation callbacks, while preserving the existing duplicate detection, FIFO processing, and pending-request cap behavior.src/shared/contracts/events/mcp.events.ts (1)
72-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
BoundedMcpJsonObjectSchemais duplicated verbatim.The identical definition exists in
src/shared/contracts/routes/mcp.routes.ts(lines 126-135). Consider hosting it once insrc/shared/contracts/common.tsalongsideJsonValueSchemaso the 2 MiB bound can't drift between the event and route surfaces.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/contracts/events/mcp.events.ts` around lines 72 - 81, Move the shared BoundedMcpJsonObjectSchema definition from the event and route contract files into common.ts alongside JsonValueSchema, then import and reuse that single symbol in both surfaces. Remove the duplicated local definitions while preserving the existing validation and 2 MiB limit.src/shared/contracts/common.ts (1)
560-599: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider binding
mcpResulttoPersistedMcpToolResultand reusing the content-item schema.Two related gaps in this block:
- No
z.ZodType<PersistedMcpToolResult>annotation, so future drift between this schema andsrc/shared/types/core/mcp.ts(PersistedMcpToolResult, lines 213-231) won't fail type-check — unlike the sibling schemas insrc/shared/contracts/routes/mcp.routes.ts, which are all annotated.content/modelContext.contentaccept rawJsonValueSchema, whilemcp.routes.tsalready defines a discriminatedMcpContentItemSchema. Persisted blocks can therefore hold content items thatformatApprovedMcpAppModelContext(src/main/agent/deepchat/runtime/contextBuilder.ts) silently drops.Extracting the content-item schema into a shared module and reusing it here would keep persistence and transport validation aligned.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/contracts/common.ts` around lines 560 - 599, Annotate the mcpResult schema with z.ZodType<PersistedMcpToolResult> and align its content fields with the transport contract. Extract the existing McpContentItemSchema from the MCP route contracts into a shared module, then reuse it for both content and modelContext.content instead of JsonValueSchema, preserving the current persisted result structure and validation limits.src/main/mcp/routes.ts (1)
92-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard message is credential-specific but the guard now covers auth, diagnostics-adjacent, and enterprise-profile routes. Errors surfaced from
mcpStartServerAuthRoute,mcpEnterpriseProfiles*, etc. will read "MCP credential changes are restricted to the settings window", which misleads. Consider a neutral message such as "This MCP settings operation is restricted to the settings window".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/mcp/routes.ts` around lines 92 - 96, Update the error message in assertSettingsWindow to use neutral wording that applies to all MCP settings operations, including auth, diagnostics-adjacent, and enterprise-profile routes, rather than referring specifically to credential changes.src/main/mcp/inMemoryServers/autoPromptingServer.ts (1)
226-231: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale comments reference removed schema constants. The registration comments still name
ListToolsRequestSchema/CallToolRequestSchema, which no longer exist in this file after the route-string migration.♻️ Suggested comment update
- // 注册 ListToolsRequestSchema 处理器，返回所有工具的元数据 + // 注册 tools/list 处理器，返回所有工具的元数据 this.server.setRequestHandler('tools/list', async () => { return this.listTools() }) - // 注册 CallToolRequestSchema 处理器，根据工具名称调用相应的处理逻辑 + // 注册 tools/call 处理器，根据工具名称调用相应的处理逻辑 this.server.setRequestHandler('tools/call', async (request): Promise<CallToolResult> => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/mcp/inMemoryServers/autoPromptingServer.ts` around lines 226 - 231, Update the registration comments adjacent to the tools/list and tools/call handlers in the server setup to remove references to the deleted ListToolsRequestSchema and CallToolRequestSchema constants. Describe the handlers using their route strings, tools/list and tools/call, while leaving the handler behavior unchanged.src/main/mcp/index.ts (1)
1106-1138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated
clientExtensionsconstruction.The same
io.modelcontextprotocol/ui+ authorization-extension mapping (including theAUTH_EXTENSION_CLIENT_CREDENTIALSrevision special case) is built here and inMcpClient.getDiagnostics(src/main/mcp/mcpClient.tsLines 1761-1769). Extracting a shared helper would keep the two diagnostic payloads from drifting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/mcp/index.ts` around lines 1106 - 1138, The duplicated clientExtensions construction in the surrounding diagnostics method and McpClient.getDiagnostics should use a shared helper that combines io.modelcontextprotocol/ui with authorization extensions and preserves the AUTH_EXTENSION_CLIENT_CREDENTIALS revision. Replace both inline mappings with the helper so the diagnostic payloads remain consistent.src/main/mcp/toolManager.ts (2)
767-799: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePersisted
mcpResultduplicates the tool payload already stored incontent.Each successful call can now persist up to ~2 MB of projected content/structuredContent/meta in addition to the formatted
contenton the same assistant block. Worth confirming the block-storage path is sized for this (and consider persisting only the fields the MCP Apps host actually re-reads, e.g. descriptor + structuredContent, rather than a second copy of the text content).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/mcp/toolManager.ts` around lines 767 - 799, The successful tool-call response currently stores the same payload in both formattedResponse.content and mcpResult. Update createPersistedMcpToolResult usage in the response construction to persist only the MCP Apps host-required fields, such as the tool descriptor and structuredContent, while avoiding duplicated text content and unnecessary metadata; preserve the existing response content and ownerPluginId behavior.
492-523: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTri-state
configparameter (undefined= unchecked,null= missing) is fragile.The distinction is load-bearing for the security check but invisible at call sites — Line 582 omits it, Line 685 passes the value, Line 737 passes
currentConfig ?? null. A future caller that forwards a possibly-undefinedconfig would silently skip binding validation. An explicit shape such asbinding: { checked: false } | { checked: true; config: MCPServerConfig | null }(or a separateassertBindingCurrenthelper) would make the skip path unreachable by accident.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/mcp/toolManager.ts` around lines 492 - 523, The tri-state config argument in describeExpectedTargetMismatch makes binding validation easy to skip accidentally. Replace it with an explicit binding-validation state, such as a checked flag carrying MCPServerConfig or null, and update all callers—including the paths around the omitted, direct-value, and currentConfig ?? null usages—to declare whether binding was checked; validate the binding whenever checked is true, including missing configurations.src/main/mcp/resultProjection.ts (1)
71-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
binaryContentOmittedis dead state in this helper.It is initialized to
falseand never set here; the only writer iscreatePersistedMcpToolResult, which mutates the returned object later. Returning a mutable flag from a "clone" helper obscures ownership — either drop it from this function's return type and track it in the caller, or set it here when binary content is actually skipped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/mcp/resultProjection.ts` around lines 71 - 86, Remove binaryContentOmitted from cloneContentForPersistence’s return type and result, then track or update the flag in createPersistedMcpToolResult where binary content omission is determined. Keep cloneContentForPersistence responsible only for producing the durable cloned content.test/main/mcp/mcpAppSandboxProtocol.test.ts (1)
20-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThese negative assertions lock in the absence of hardening rather than a contract.
not.toContain("worker-src 'none'")will fail if someone later adds an explicit (stricter)worker-srcdirective, which is a legitimate hardening change.not.toContain('connect-src *')is also substring-fragile. Prefer asserting the exact directive values you want.♻️ Suggested tweak
- expect(policy).not.toContain('connect-src *') - expect(policy).not.toContain("worker-src 'none'") + expect(policy).toContain('connect-src https://api.example.com') + expect(policy).not.toMatch(/connect-src[^;]*\*/)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/mcp/mcpAppSandboxProtocol.test.ts` around lines 20 - 21, Update the assertions in the policy test around the existing expect calls to validate the intended exact directive values rather than asserting that specific substrings are absent. Replace the substring-fragile connect-src check and the worker-src negative check with assertions that preserve the required policy contract while allowing stricter directives such as worker-src 'none'.test/renderer/components/McpServers.test.ts (1)
423-435: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNit:
removeDialogis a stale wrapper for the post-click assertion.If the dialog is ever switched to
v-if(or keyed) instead of a prop-driven stub, the capturedDOMWrapperpoints at a detached node anddata-openreads would go stale rather than fail loudly. Re-querying after the click keeps this test honest about "closed".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/renderer/components/McpServers.test.ts` around lines 423 - 435, Update the dialog-close assertion in the test around removeDialog so it re-queries the current dialog wrapper after triggering the confirm button, rather than reading data-open from the pre-click wrapper. Keep the existing pre-click assertions and verify the freshly queried dialog is closed.test/main/mcp/mcpAppSandboxRegistry.test.ts (1)
106-131: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider covering the deny path for an undeclared App capability.
The suite proves a declared capability (
camera←video) can be granted via consent, but nothing pins the security-critical inverse: a sandbox request for a capability the App never declared (e.g.mediaTypes: ['audio']with onlycamera: {}declared) must be denied without ever publishing a consent prompt. That's the regression most worth guarding here.As per coding guidelines: "Add the smallest regression test for user-visible behavior or a documented contract."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/mcp/mcpAppSandboxRegistry.test.ts` around lines 106 - 131, Extend the existing undeclared-capability coverage around permissionRequestHandler so a request using an undeclared media type, such as audio when only camera/video is declared, is denied immediately. Assert appDecision is called with false and consent publishing is not invoked, without submitting or waiting for a consent request.Source: Coding guidelines
test/main/mcp/mcpOAuthManager.test.ts (1)
239-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
toHaveBeenCalledTimes(2)pins an implementation detail; thesaveEntrykey assertion pins almost nothing.The exact number of
bindingChangednotifications is internal bookkeeping — any harmless refactor (e.g. collapsing the pre/post-finalization notification) breaks this without a behavior change. Prefer asserting the observable outcome (final binding values delivered to the callback). Conversely,expect.not.stringMatching(/^old-key$/)only proves the key changed, not that the credential was re-keyed to the finalized binding identity, which is the actual contract being finalized here.As per coding guidelines: "Keep committed tests lean and focused on project reliability, stability, and observable contracts; remove temporary checks that only test implementation internals before handoff."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/mcp/mcpOAuthManager.test.ts` around lines 239 - 241, Revise the assertions in the OAuth binding-finalization test to remove the exact bindingChanged call-count check and instead assert the final binding values delivered to the callback. Strengthen the store.saveEntry assertion to verify that the credential is saved under the finalized binding identity rather than merely ensuring the key differs from old-key.Source: Coding guidelines
test/main/mcp/mcpAppHost.test.ts (1)
93-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNit:
registry.createmutating the sharedinstanceweakens the harness.
Object.assign(instance, input)folds call input into the same object returned byassertOwned, so later assertions oninstancecan't distinguish "host set this" from "fixture had this". Returning{ ...instance, ...input }keeps the fixture pristine.♻️ Suggested tweak
- create: vi.fn((input: Record<string, unknown>) => Object.assign(instance, input)), + create: vi.fn((input: Record<string, unknown>) => ({ ...instance, ...input })),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/mcp/mcpAppHost.test.ts` around lines 93 - 99, Update the test registry’s create mock to return a new object combining instance and input rather than mutating instance via Object.assign. Keep assertOwned returning the original fixture so later assertions can distinguish fixture state from host-applied values.src/main/mcp/mcpOAuthManager.ts (1)
828-845: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
onServerBindingChangedfires twice for a single update.Lines 833 and 842 both notify for the same
serverId, so app revocation/registry refresh runs twice per discovery-driven binding change. Notifying once after the settings write is enough unless the pre-write call is intentional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/mcp/mcpOAuthManager.ts` around lines 828 - 845, Remove the pre-write onServerBindingChanged call in the needsUpdate branch of the OAuth binding update flow, keeping the notification after settings.updateMcpServer and successful binding finalization. Ensure each discovery-driven binding change for the startingBinding.serverId triggers exactly one notification.src/main/mcp/enterpriseIdentityManager.ts (1)
645-691: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared token-response parsing.
exchangeAuthorizationCodeandgetValidIdentityduplicate ~45 lines of identical bounded parsing (id_token/access_token/refresh_token/scope) and expiry computation, differing only in labels. A single helper (e.g.parseTokenResponse(response, label)) would keep the two paths from diverging.Also applies to: 760-804
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/mcp/enterpriseIdentityManager.ts` around lines 645 - 691, Extract the duplicated bounded token parsing and expiry computation from exchangeAuthorizationCode and getValidIdentity into a shared helper such as parseTokenResponse, parameterized by the response label used in validation errors. Update both callers to use the helper while preserving ID-token verification and each flow’s distinct profile, issuer, client, subject, and metadata fields.src/main/plugin/index.ts (1)
1219-1234: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the directory-precedence partitioning into a small helper.
The install-root containment filter + packaged/non-packaged ordering logic is dense inline in
loadOfficialPlugins. Logic itself looks correct (no plugins are dropped, only reordered for same-id precedence), but a small named helper (e.g.partitionInstalledDirectories(directories, installRoot)) would make the precedence intent easier to follow at a glance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/plugin/index.ts` around lines 1219 - 1234, Extract the installed-versus-source directory partitioning from loadOfficialPlugins into a small named helper such as partitionInstalledDirectories(directories, installRoot), preserving the existing install-root containment checks and return values. Update loadOfficialPlugins to use the helper, while keeping the current packaged/non-packaged plugin ordering unchanged.src/main/tool/index.ts (1)
363-394: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate the execution target before prompting for permission.
createExpectedMcpTargetthrows for stale/missing definitions, but that happens afterauthorizeExecutionalready consumed the matching approval entry. A stale binding therefore costs the user a consent prompt and then fails, and re-running requires re-approval. Resolving the target first fails fast.♻️ Suggested reordering
const definition = this.getMcpDefinition(toolName, request.conversationId) + const expectedTarget = this.createExpectedMcpTarget(toolName, definition) const permissionContext = this.createMcpPermissionContext( request, definition, options?.permissionMode ) @@ - const expectedTarget = this.createExpectedMcpTarget(toolName, definition) return await this.options.mcpService.callTool(request, {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/tool/index.ts` around lines 363 - 394, Move the createExpectedMcpTarget call before the permissionContext/authorizeExecution flow so stale or missing definitions fail before any approval is consumed. Reuse the validated expectedTarget in the subsequent mcpService.callTool invocation, preserving the existing authorization behavior for valid targets.src/main/mcp/apps/sandboxProtocol.ts (1)
7-21: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
resourceDomainswideningscript-srclooks unintended.Declared resource domains are folded into
script-src/style-src, so any host allowed for images/fonts can also serve executable script into the sandbox. If the intent is subresource loading only, keepscript-src/style-srcto'self' 'unsafe-inline'. Also,quoteCspSourceis an identity function and can be dropped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/mcp/apps/sandboxProtocol.ts` around lines 7 - 21, Update buildMcpAppContentSecurityPolicy so resourceDomains are not included in script-src or style-src; keep those directives restricted to 'self' and 'unsafe-inline'. Remove the unused identity helper quoteCspSource and preserve source deduplication for directives that still accept domains.src/main/tool/permission/toolPermissionBroker.ts (1)
75-85: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePrefer codepoint ordering for hash-stable canonicalization.
localeCompareis locale/ICU-dependent, so the same arguments can canonicalize to different key orders (and thus different hashes) across environments. Since the hash is the approval-matching key, use a deterministic comparator.♻️ Deterministic sort
- .sort(([left], [right]) => left.localeCompare(right)) + .sort(([left], [right]) => (left < right ? -1 : left > right ? 1 : 0))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/tool/permission/toolPermissionBroker.ts` around lines 75 - 85, Update the key comparator in the object-handling branch of canonicalize so it uses deterministic codepoint ordering instead of localeCompare. Preserve the existing key traversal, limit enforcement, and recursive canonicalization behavior while ensuring identical inputs produce the same order across environments.src/main/session/data/tables/deepchatAssistantBlocks.ts (1)
180-263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared MCP-app row matcher.
matchesMcpAppSourceandupdateMcpAppModelContextduplicate the same parse-and-compare predicate (extra.id === blockId,mcpResult.appdeep-equal to descriptor, persistedtool_paramsdeep-equal totoolInput). A single private helper keeps the two paths from drifting, which would silently let a source pass validation but fail persistence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/session/data/tables/deepchatAssistantBlocks.ts` around lines 180 - 263, Extract the shared parse-and-compare predicate from matchesMcpAppSource and updateMcpAppModelContext into a private helper on the containing class. Have the helper parse each row’s extra_json and tool_params, validate extra.id, mcpResult.app, and persisted input, and return the parsed data needed by the update path; preserve false/continue behavior for malformed or non-matching rows and reuse the helper in both methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e66e790c-9e2e-458f-96f5-537889457df3
⛔ Files ignored due to path filters (3)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlsrc/renderer/src/lib/icons/icon-collections.generated.tsis excluded by!**/*.generated.*src/renderer/src/lib/icons/icon-whitelist.generated.tsis excluded by!**/*.generated.*
📒 Files selected for processing (172)
.oxlintrc.jsondocs/architecture/mcp-v2-protocol/manual-verification.mddocs/architecture/mcp-v2-protocol/plan.mddocs/architecture/mcp-v2-protocol/spec.mddocs/architecture/mcp-v2-protocol/tasks.mddocs/architecture/remove-mcp-permission-system/plan.mddocs/architecture/remove-mcp-permission-system/spec.mddocs/architecture/remove-mcp-permission-system/tasks.mddocs/features/acp-v1-reliability/plan.mddocs/features/acp-v1-reliability/spec.mddocs/features/mcp-apps/plan.mddocs/features/mcp-apps/spec.mddocs/features/mcp-apps/tasks.mddocs/features/mcp-authorization-extensions/plan.mddocs/features/mcp-authorization-extensions/spec.mddocs/features/mcp-authorization-extensions/tasks.mddocs/features/mcp-oauth-authentication/plan.mddocs/features/mcp-oauth-authentication/spec.mddocs/features/mcp-oauth-authentication/tasks.mddocs/features/mcp-tasks/plan.mddocs/features/mcp-tasks/spec.mddocs/features/mcp-tasks/tasks.mdpackage.jsonplugins/cua/mcp/cua-driver.jsonplugins/cua/plugin.jsonplugins/feishu/plugin.jsonresources/acp-registry/registry.jsonresources/model-db/providers.jsonsrc/main/agent/deepchat/runtime/compactionService.tssrc/main/agent/deepchat/runtime/contextBuilder.tssrc/main/agent/deepchat/runtime/dispatch.tssrc/main/agent/deepchat/runtime/interactionCoordinator.tssrc/main/app/composition.tssrc/main/app/mainProcess.tssrc/main/app/protocols.tssrc/main/appMain.tssrc/main/deeplink/index.tssrc/main/desktop/window/index.tssrc/main/mcp/apps/appHost.tssrc/main/mcp/apps/sandboxProtocol.tssrc/main/mcp/apps/sandboxRegistry.tssrc/main/mcp/enterpriseIdentityManager.tssrc/main/mcp/inMemoryServers/appleServer.tssrc/main/mcp/inMemoryServers/artifactsServer.tssrc/main/mcp/inMemoryServers/autoPromptingServer.tssrc/main/mcp/inMemoryServers/bochaSearchServer.tssrc/main/mcp/inMemoryServers/braveSearchServer.tssrc/main/mcp/inMemoryServers/builtinKnowledgeServer.tssrc/main/mcp/inMemoryServers/conversationSearchServer.tssrc/main/mcp/inMemoryServers/deepResearchServer.tssrc/main/mcp/inMemoryServers/difyKnowledgeServer.tssrc/main/mcp/inMemoryServers/fastGptKnowledgeServer.tssrc/main/mcp/inMemoryServers/ragflowKnowledgeServer.tssrc/main/mcp/index.tssrc/main/mcp/mcpClient.tssrc/main/mcp/mcpOAuthManager.tssrc/main/mcp/mcpOAuthProvider.tssrc/main/mcp/mcprouterManager.tssrc/main/mcp/oauthCredentialStore.tssrc/main/mcp/resultProjection.tssrc/main/mcp/routes.tssrc/main/mcp/schemaValidation.tssrc/main/mcp/serverIdentity.tssrc/main/mcp/settings.tssrc/main/mcp/toolManager.tssrc/main/plugin/index.tssrc/main/provider/aiSdk/toolMapper.tssrc/main/provider/auth/oauthLoopbackCallback.tssrc/main/provider/modelScopeMcp.tssrc/main/provider/providers/githubCopilotProvider.tssrc/main/session/contracts.tssrc/main/session/data/tables/deepchatAssistantBlocks.tssrc/main/tool/index.tssrc/main/tool/permission/index.tssrc/main/tool/permission/toolPermissionBroker.tssrc/renderer/api/McpClient.tssrc/renderer/src/apps/chat-main/ChatMainApp.vuesrc/renderer/src/components/mcp-config/McpServerForm.vuesrc/renderer/src/components/mcp-config/components/McpEnterpriseProfiles.vuesrc/renderer/src/components/mcp-config/components/McpServerCard.vuesrc/renderer/src/components/mcp-config/components/McpServers.vuesrc/renderer/src/components/mcp/McpAppConsentDialog.vuesrc/renderer/src/components/mcp/McpAppView.vuesrc/renderer/src/components/mcp/McpElicitationDialog.vuesrc/renderer/src/components/mcp/mcpAppDisplayCoordinator.tssrc/renderer/src/components/message/MessageBlockToolCall.vuesrc/renderer/src/features/chat-page/model/displayMessage.tssrc/renderer/src/i18n/da-DK/mcp.jsonsrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/de-DE/mcp.jsonsrc/renderer/src/i18n/de-DE/settings.jsonsrc/renderer/src/i18n/en-US/mcp.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/es-ES/mcp.jsonsrc/renderer/src/i18n/es-ES/settings.jsonsrc/renderer/src/i18n/fa-IR/mcp.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/mcp.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/mcp.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/id-ID/mcp.jsonsrc/renderer/src/i18n/id-ID/settings.jsonsrc/renderer/src/i18n/it-IT/mcp.jsonsrc/renderer/src/i18n/it-IT/settings.jsonsrc/renderer/src/i18n/ja-JP/mcp.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/mcp.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/ms-MY/mcp.jsonsrc/renderer/src/i18n/ms-MY/settings.jsonsrc/renderer/src/i18n/pl-PL/mcp.jsonsrc/renderer/src/i18n/pl-PL/settings.jsonsrc/renderer/src/i18n/pt-BR/mcp.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/mcp.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/tr-TR/mcp.jsonsrc/renderer/src/i18n/tr-TR/settings.jsonsrc/renderer/src/i18n/vi-VN/mcp.jsonsrc/renderer/src/i18n/vi-VN/settings.jsonsrc/renderer/src/i18n/zh-CN/mcp.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/mcp.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/mcp.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/renderer/src/stores/mcpAppConsent.tssrc/renderer/src/stores/mcpElicitation.tssrc/renderer/src/stores/mcpSampling.tssrc/shared/contracts/common.tssrc/shared/contracts/events.tssrc/shared/contracts/events/mcp.events.tssrc/shared/contracts/routes.tssrc/shared/contracts/routes/mcp.routes.tssrc/shared/lib/zodJsonSchema.tssrc/shared/types/agent-interface.d.tssrc/shared/types/core/mcp.tssrc/shared/types/mcp.tssrc/shared/types/plugin.tssrc/types/i18n.d.tstest/main/agent/acp/runtime/acpMcpPassthrough.test.tstest/main/deeplink/deeplinkService.test.tstest/main/mcp/builtinKnowledgeServer.test.tstest/main/mcp/mcpAppHost.test.tstest/main/mcp/mcpAppSandboxProtocol.test.tstest/main/mcp/mcpAppSandboxRegistry.test.tstest/main/mcp/mcpClient.test.tstest/main/mcp/mcpOAuthManager.test.tstest/main/mcp/mcpService.test.tstest/main/mcp/oauthCredentialStore.test.tstest/main/mcp/resultProjection.test.tstest/main/mcp/schemaValidation.test.tstest/main/mcp/serverIdentity.test.tstest/main/mcp/settings.test.tstest/main/mcp/toolManager.test.tstest/main/plugin/pluginService.test.tstest/main/provider/aiSdkToolMapper.test.tstest/main/provider/auth/oauthLoopbackCallback.test.tstest/main/routes/dispatcher.test.tstest/main/scripts/packagePlugin.test.tstest/main/session/data/tables/deepchatAssistantBlocks.test.tstest/main/settings/appSettingsDbStore.test.tstest/main/sync/configImportService.test.tstest/main/tool/toolPermissionBroker.test.tstest/main/tool/toolService.test.tstest/manual/deeplink-playground.htmltest/renderer/components/App.startup.test.tstest/renderer/components/McpServerForm.test.tstest/renderer/components/McpServers.test.tstest/renderer/stores/mcpStore.test.tstest/setup.ts
💤 Files with no reviewable changes (10)
- test/main/settings/appSettingsDbStore.test.ts
- plugins/cua/mcp/cua-driver.json
- src/main/mcp/mcprouterManager.ts
- test/main/deeplink/deeplinkService.test.ts
- test/main/scripts/packagePlugin.test.ts
- test/main/agent/acp/runtime/acpMcpPassthrough.test.ts
- test/main/sync/configImportService.test.ts
- src/main/provider/modelScopeMcp.ts
- plugins/cua/plugin.json
- test/renderer/stores/mcpStore.test.ts
| Key server-bound records by: | ||
|
|
||
| ```text | ||
| sha256( | ||
| credentialClass + "\n" + | ||
| serverId + "\n" + | ||
| configGeneration + "\n" + | ||
| bindingHash + "\n" + | ||
| endpoint + "\n" + | ||
| protectedResource + "\n" + | ||
| issuer + "\n" + | ||
| clientId | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Keep credential-key formulas consistent across the authorization plans.
This plan includes clientId in server-bound credential identity, but docs/features/mcp-oauth-authentication/plan.md lines 262-272 omits it. A client-ID change could therefore reuse credentials under the OAuth plan’s key. Include clientId in the shared key contract, or explicitly document separate keying for interactive credentials.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/features/mcp-authorization-extensions/plan.md` around lines 30 - 42,
Update the server-bound credential key contract in the authorization plans so
the OAuth authentication plan includes clientId alongside the existing identity
fields, or explicitly document why interactive credentials use separate keying.
Keep the formulas consistent across both plans to prevent client-ID changes from
reusing credentials.
| const McpAppServerToolSchema: z.ZodType<Tool> = z.object({ | ||
| name: z.string().min(1).max(256), | ||
| title: z.string().max(512).optional(), | ||
| description: z | ||
| .string() | ||
| .max(16 * 1024) | ||
| .optional(), | ||
| icons: z.array(McpAppServerIconSchema).max(32).optional(), | ||
| inputSchema: BoundedMcpJsonObjectSchema, | ||
| outputSchema: BoundedMcpJsonObjectSchema.optional(), | ||
| annotations: BoundedMcpJsonObjectSchema.optional(), | ||
| _meta: BoundedMcpJsonObjectSchema.optional(), | ||
| execution: BoundedMcpJsonObjectSchema.optional() | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the project's TS setup and run a type check on shared contracts
fd -H -t f 'tsconfig*.json' -d 3
rg -n '"(typecheck|type-check|typecheck:node|typecheck:web)"' package.jsonRepository: ThinkInAIXYZ/deepchat
Length of output: 410
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline 'src/shared/contracts/routes/mcp.routes.ts' --view expanded || true
echo "== relevant schema/type references =="
rg -n "BoundedMcpJsonObjectSchema|McpToolAnnotations|McpAppServerToolSchema|satisfies z\.ZodType<Tool>|satisfies z\.ZodType<McpAppServerToolListResult>|export type Tool" 'src/shared' 'src/main' 'src/preload' 'src/renderer' --glob '*.ts' --glob '*.tsx'
echo "== file slices =="
wc -l 'src/shared/contracts/routes/mcp.routes.ts'
sed -n '1,130p' 'src/shared/contracts/routes/mcp.routes.ts'
echo "--- contracts relevant 220-285 ---"
sed -n '220,285p' 'src/shared/contracts/routes/mcp.routes.ts'
echo "--- contracts relevant 860-920 ---"
sed -n '860,920p' 'src/shared/contracts/routes/mcp.routes.ts'
echo "== mcp types slice =="
sed -n '1,140p' 'src/shared/types/mcp.ts'Repository: ThinkInAIXYZ/deepchat
Length of output: 18091
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Zod versions =="
rg -n '"zod"\s*:|zod@\d|node_modules/zod/package.json' package.json pnpm-lock.yaml package-lock.json yarn.lock 2>/dev/null || true
echo "== JsonValueSchema definition =="
rg -n "export const JsonValueSchema|JsonValueSchema\s*=" src/shared --glob '*.ts'Repository: ThinkInAIXYZ/deepchat
Length of output: 3573
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== JsonValueSchema definition =="
rg -n "export const JsonValueSchema|JsonValueSchema\s*=" src/shared --glob '*.ts'
echo "== ts error context (if available from package scripts, but avoid installing/building repo code) =="
rg -n "typecheck:node|typecheck:web" package.jsonRepository: ThinkInAIXYZ/deepchat
Length of output: 730
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== JsonValue definitions =="
rg -n "type JsonValue|interface JsonValue|export type JsonValue|export interface JsonValue" src/shared --glob '*.ts'
echo "== common schema slice =="
sed -n '1,90p' src/shared/contracts/common.tsRepository: ThinkInAIXYZ/deepchat
Length of output: 3199
Use a typed annotations schema for MCP app tools.
BoundedMcpJsonObjectSchema is typed as Record<string, JsonValue>, so McpAppServerToolSchema.annotations is inferred with only JSON-value keys and can’t satisfy z.ZodType<Tool> unless Zod skips interface property index checks. Add a dedicated annotations schema that mirrors McpToolAnnotations and use it for both annotations and McpAppServerToolListResult.output.annotations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/shared/contracts/routes/mcp.routes.ts` around lines 258 - 271, The
McpAppServerToolSchema currently uses the generic BoundedMcpJsonObjectSchema for
annotations, losing the MCP-specific type. Define a dedicated schema matching
McpToolAnnotations, use it for McpAppServerToolSchema.annotations, and reuse the
same schema for McpAppServerToolListResult.output.annotations.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/mcp/mcpClient.ts (1)
1001-1002: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMap malformed elicitation URLs to
InvalidParams.Line 1002 throws a native
TypeErrorfor malformed URLs, unlike the other invalid URL cases which return an MCPProtocolError. Catch URL parsing failures so servers receive a consistent protocol error.Proposed fix
let url: string | undefined if (params.mode === 'url') { - const candidate = new URL(params.url) + let candidate: URL + try { + candidate = new URL(params.url) + } catch { + throw new ProtocolError( + ProtocolErrorCode.InvalidParams, + 'Elicitation URL must be a valid HTTP or HTTPS URL' + ) + } if (candidate.protocol !== 'https:' && candidate.protocol !== 'http:') {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/mcp/mcpClient.ts` around lines 1001 - 1002, Update the URL handling branch guarded by params.mode === 'url' to catch failures from new URL(params.url) and map malformed URL input to the MCP InvalidParams ProtocolError, matching the existing invalid-URL cases. Preserve normal URL parsing and downstream behavior for valid URLs.src/renderer/src/components/mcp-config/components/McpServers.vue (1)
218-231: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDuplicate i18n key used for both notification title and description.
Every other
notifyRenderercall in this file (e.g.toggleFailed,authStartFailed,managedServerReadOnly) uses distinct keys fortitlevs.description. Here both are set tosettings.mcp.serverForm.credentialSaveError, producing redundant text in the notification.💬 Proposed fix
notifyRenderer({ kind: 'error', code: 'settings.mcp.serverForm.credentialSaveError', title: t('settings.mcp.serverForm.credentialSaveError'), - description: t('settings.mcp.serverForm.credentialSaveError') + description: t('common.error.requestFailed') })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/mcp-config/components/McpServers.vue` around lines 218 - 231, Update the credential-save failure notification in the server credential submission flow to use the appropriate distinct i18n key for description instead of reusing settings.mcp.serverForm.credentialSaveError for both title and description. Keep the existing title key and error handling unchanged.src/main/mcp/enterpriseIdentityManager.ts (1)
733-785: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle refresh responses without a new
id_tokenwithout re-verifying the expired token.OIDC Core allows refresh responses to omit
id_token, but this path falls back to the existing ID token even whencurrent.expiresAtis already at/near its cap and then re-verifies the sameexp; that can fail and force full re-authentication despite returning fresh access/refresh tokens. Only re-verify and updateexpiresAt/subjectwhen a newid_tokenis returned, otherwise retain the existing identity data.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/mcp/enterpriseIdentityManager.ts` around lines 733 - 785, Update getValidIdentity so ID-token verification and subject validation occur only when the refresh response includes tokens.idToken; when absent, retain current idToken, subject, and expiration data while still applying refreshed access/refresh tokens and scope. Compute expiresAt from the new verified payload only for responses with a new ID token, avoiding re-verification of the expired token.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/mcp/apps/sandboxProtocol.ts`:
- Around line 7-8: Update normalizeCspSource, used by joinSources, to reject CSP
source tokens containing whitespace, control characters, or directive separators
such as semicolons. Ensure invalid values are rejected before joinSources
deduplicates and emits them, while preserving normalization for valid sources.
In `@src/renderer/src/i18n/es-ES/settings.json`:
- Around line 1900-1902: Update the confidentialClient translation in
src/renderer/src/i18n/es-ES/settings.json lines 1900-1902 to “Cliente
confidencial”; update src/renderer/src/i18n/fr-FR/settings.json lines 1554-1556
to “Client confidentiel” while keeping clientSecret as “Secret client”; and
replace confidentialClient in src/renderer/src/i18n/he-IL/settings.json lines
1554-1556 and src/renderer/src/i18n/ko-KR/settings.json lines 1554-1556 with
their respective translations of “Confidential client,” ensuring each remains
distinct from clientSecret.
In `@src/renderer/src/i18n/fr-FR/settings.json`:
- Around line 1429-1433: Update the French translations for removeCredential,
credentialConfiguredPlaceholder, credentialConfigured, credentialMemoryOnly, and
credentialRemoveError to use consistent French credential terminology such as
“informations d’identification” or “identifiant d’authentification” instead of
the ambiguous “identifiant”, while preserving each message’s existing meaning.
In `@src/renderer/src/i18n/pt-BR/mcp.json`:
- Around line 337-341: Update the clipboardWrite translation in the permissions
object to use Portuguese wording that clearly means “write to the clipboard,”
such as “Escrever na área de transferência,” instead of the current
recording-related label.
---
Outside diff comments:
In `@src/main/mcp/enterpriseIdentityManager.ts`:
- Around line 733-785: Update getValidIdentity so ID-token verification and
subject validation occur only when the refresh response includes tokens.idToken;
when absent, retain current idToken, subject, and expiration data while still
applying refreshed access/refresh tokens and scope. Compute expiresAt from the
new verified payload only for responses with a new ID token, avoiding
re-verification of the expired token.
In `@src/main/mcp/mcpClient.ts`:
- Around line 1001-1002: Update the URL handling branch guarded by params.mode
=== 'url' to catch failures from new URL(params.url) and map malformed URL input
to the MCP InvalidParams ProtocolError, matching the existing invalid-URL cases.
Preserve normal URL parsing and downstream behavior for valid URLs.
In `@src/renderer/src/components/mcp-config/components/McpServers.vue`:
- Around line 218-231: Update the credential-save failure notification in the
server credential submission flow to use the appropriate distinct i18n key for
description instead of reusing settings.mcp.serverForm.credentialSaveError for
both title and description. Keep the existing title key and error handling
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1145e1e0-35fa-4d9f-8c8c-420e5f61c6f1
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (84)
docs/README.mddocs/architecture/mcp-v2-protocol/manual-verification.mddocs/architecture/mcp-v2-protocol/spec.mddocs/architecture/mcp-v2-protocol/tasks.mddocs/architecture/remove-mcp-permission-system/plan.mddocs/architecture/remove-mcp-permission-system/spec.mddocs/architecture/remove-mcp-permission-system/tasks.mddocs/architecture/tool-system.mddocs/features/mcp-apps/spec.mddocs/features/mcp-apps/tasks.mddocs/features/mcp-authorization-extensions/spec.mddocs/features/mcp-authorization-extensions/tasks.mddocs/features/mcp-oauth-authentication/plan.mddocs/features/mcp-oauth-authentication/spec.mddocs/features/mcp-oauth-authentication/tasks.mddocs/features/mcp-tasks/spec.mddocs/features/mcp-tasks/tasks.mdpackage.jsonresources/acp-registry/registry.jsonresources/model-db/providers.jsonscripts/lib/i18n-validation.mjssrc/main/mcp/apps/appHost.tssrc/main/mcp/apps/sandboxProtocol.tssrc/main/mcp/apps/sandboxRegistry.tssrc/main/mcp/enterpriseIdentityManager.tssrc/main/mcp/mcpClient.tssrc/main/mcp/mcpOAuthManager.tssrc/main/mcp/resultProjection.tssrc/main/tool/permission/toolPermissionBroker.tssrc/renderer/src/components/mcp-config/McpServerForm.vuesrc/renderer/src/components/mcp-config/components/McpServerCard.vuesrc/renderer/src/components/mcp-config/components/McpServers.vuesrc/renderer/src/components/mcp/McpAppConsentDialog.vuesrc/renderer/src/components/mcp/McpAppView.vuesrc/renderer/src/i18n/da-DK/mcp.jsonsrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/de-DE/mcp.jsonsrc/renderer/src/i18n/de-DE/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/es-ES/mcp.jsonsrc/renderer/src/i18n/es-ES/settings.jsonsrc/renderer/src/i18n/fa-IR/mcp.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/mcp.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/mcp.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/id-ID/mcp.jsonsrc/renderer/src/i18n/id-ID/settings.jsonsrc/renderer/src/i18n/it-IT/mcp.jsonsrc/renderer/src/i18n/it-IT/settings.jsonsrc/renderer/src/i18n/ja-JP/mcp.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/mcp.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/ms-MY/mcp.jsonsrc/renderer/src/i18n/ms-MY/settings.jsonsrc/renderer/src/i18n/pl-PL/mcp.jsonsrc/renderer/src/i18n/pl-PL/settings.jsonsrc/renderer/src/i18n/pt-BR/mcp.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/mcp.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/tr-TR/mcp.jsonsrc/renderer/src/i18n/tr-TR/settings.jsonsrc/renderer/src/i18n/vi-VN/mcp.jsonsrc/renderer/src/i18n/vi-VN/settings.jsonsrc/renderer/src/i18n/zh-CN/mcp.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/mcp.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/renderer/src/stores/mcpElicitation.tssrc/types/i18n.d.tstest/main/mcp/enterpriseIdentityManager.test.tstest/main/mcp/mcpAppHost.test.tstest/main/mcp/mcpAppSandboxRegistry.test.tstest/main/mcp/mcpClient.test.tstest/main/mcp/mcpOAuthManager.test.tstest/main/mcp/resultProjection.test.tstest/main/scripts/i18nValidation.test.tstest/main/session/data/tables/deepchatAssistantBlocks.test.tstest/main/tool/toolPermissionBroker.test.tstest/renderer/stores/mcpElicitation.test.ts
💤 Files with no reviewable changes (6)
- docs/features/mcp-oauth-authentication/plan.md
- docs/architecture/remove-mcp-permission-system/tasks.md
- docs/architecture/remove-mcp-permission-system/plan.md
- package.json
- docs/features/mcp-oauth-authentication/spec.md
- src/renderer/src/components/mcp/McpAppView.vue
🚧 Files skipped from review as they are similar to previous changes (23)
- docs/features/mcp-oauth-authentication/tasks.md
- docs/features/mcp-tasks/tasks.md
- src/main/tool/permission/toolPermissionBroker.ts
- src/renderer/src/i18n/en-US/settings.json
- docs/architecture/mcp-v2-protocol/manual-verification.md
- docs/architecture/remove-mcp-permission-system/spec.md
- src/renderer/src/i18n/zh-CN/settings.json
- src/renderer/src/i18n/pt-BR/settings.json
- src/renderer/src/i18n/ru-RU/settings.json
- docs/architecture/mcp-v2-protocol/spec.md
- src/renderer/src/i18n/zh-TW/settings.json
- src/renderer/src/i18n/tr-TR/settings.json
- src/renderer/src/i18n/zh-HK/settings.json
- src/renderer/src/i18n/pl-PL/settings.json
- src/renderer/src/i18n/ms-MY/settings.json
- src/renderer/src/i18n/da-DK/settings.json
- src/renderer/src/i18n/fa-IR/settings.json
- src/renderer/src/i18n/ja-JP/settings.json
- src/renderer/src/i18n/it-IT/settings.json
- src/renderer/src/i18n/vi-VN/settings.json
- src/renderer/src/i18n/de-DE/settings.json
- src/renderer/src/i18n/id-ID/settings.json
- src/renderer/src/components/mcp-config/McpServerForm.vue
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/main/tool/index.ts (2)
513-535: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGuard
getMcpDefinitionagainst cross-conversation global fallback
getAllToolDefinitions()republishesglobalMcpDefinitionsfor each conversation, sogetMcpDefinition()can return another conversation’s MCP binding when the requesting conversation doesn’t define the tool. Copy the scoped-only/fallback check fromgetToolSource(), thencreateExpectedMcpTarget()will not deriveserverId/configGeneration/bindingHashfrom the wrong server.Patch
private getMcpDefinition( toolName: string, conversationId?: string ): MCPToolDefinition | undefined { const normalizedConversationId = conversationId?.trim() - return ( - (normalizedConversationId - ? this.conversationMcpDefinitions.get(normalizedConversationId)?.get(toolName) - : undefined) ?? this.globalMcpDefinitions.get(toolName) - ) + if (normalizedConversationId) { + const scoped = this.conversationMcpDefinitions.get(normalizedConversationId) + if (scoped) { + return scoped.get(toolName) + } + if (this.globalMapperConversationId !== null) { + return undefined + } + } + return this.globalMcpDefinitions.get(toolName) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/tool/index.ts` around lines 513 - 535, Update getMcpDefinition to use the same scoped-only versus fallback decision as getToolSource(), preventing a conversation lookup from falling back to globalMcpDefinitions when the requesting conversation lacks the tool. Preserve global lookup only for unscoped requests so createExpectedMcpTarget() cannot use another conversation’s MCP binding.
455-466: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
preCheckToolPermissiondoesn't validate stable execution binding, unlikecallTool.
callTool(Line 364) callscreateExpectedMcpTargetbefore authorization, which throws for an unstable/missing binding. The MCP branch ofpreCheckToolPermissionskips this check entirely and evaluates permission straight fromgetMcpDefinition's result (possiblyundefined, falling back to'unknown'serverId increateMcpPermissionContext). A precheck can therefore report "no permission needed" for a tool whose subsequentcallToolinvocation will throwno stable execution binding, which is confusing for callers relying on precheck to predict execution outcome.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/tool/index.ts` around lines 455 - 466, Update the MCP path in preCheckToolPermission to invoke createExpectedMcpTarget before shouldBrokerMcpTool or permission evaluation, using the same inputs and behavior as callTool. Ensure unstable or missing execution bindings throw the stable-binding error during precheck instead of falling through to createMcpPermissionContext with an unknown server.src/main/mcp/mcpClient.ts (2)
1501-1521: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPaginated list methods skip the unsupported-capability fallback used by their non-paginated counterparts.
listTools/listPrompts/listResourcescatchisUnsupportedCapabilityErrorand gracefully return[], butlistToolsPage,listPromptsPage,listResourcesPage, andlistResourceTemplatesPageonly guard via the staticserverDoesNotAdvertisecheck with no try/catch. If a server advertises the capability object but a specific page call still returnsMethodNotFound, these will throw uncaught instead of resolving to an empty page like their non-paginated siblings.Consider wrapping these page methods with the same
isUnsupportedCapabilityErrorhandling for consistency.Also applies to: 1562-1577, 1647-1662, 1664-1682
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/mcp/mcpClient.ts` around lines 1501 - 1521, Update listToolsPage, listPromptsPage, listResourcesPage, and listResourceTemplatesPage to catch errors from their paginated client calls, use isUnsupportedCapabilityError to identify unsupported methods, and return an empty page result consistent with the corresponding non-paginated methods; rethrow all other errors and preserve existing validation and mapping behavior.
1057-1069: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winElicitation cancellation can produce an unhandled promise rejection.
cancelElicitationRequestis invoked withvoid ...and no.catch(), unlike the analogous sampling abort handler a few hundred lines below (this.runtime.sampling.cancelSamplingRequest(...).catch((error) => console.warn(...))). IfcancelElicitationRequestrejects (e.g. IPC failure or no pending request), this becomes an unhandled rejection in the main process instead of being logged and swallowed like the sampling counterpart.🐛 Proposed fix: mirror the sampling handler's catch
if (signal) { abortListener = () => { - void this.runtime.elicitation.cancelElicitationRequest(requestId, 'cancelled by server') + void this.runtime.elicitation + .cancelElicitationRequest(requestId, 'cancelled by server') + .catch((error) => { + console.warn(`[MCP] Failed to cancel elicitation request ${requestId}:`, error) + }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/mcp/mcpClient.ts` around lines 1057 - 1069, Update the abort listener in the elicitation request flow around handleElicitationRequest so cancelElicitationRequest handles rejected promises with a catch that logs the error and prevents an unhandled rejection, matching the existing sampling abort handler behavior. Preserve the current cancellation message and abort listener registration.
♻️ Duplicate comments (1)
src/main/mcp/apps/sandboxRegistry.ts (1)
376-452: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftStill overriding
session.defaultSessionpermission handlers for the whole app, not just MCP webContents.
configureDefaultSessionPermissions()replacessession.defaultSession'ssetPermissionRequestHandlerandsetPermissionCheckHandlerentirely. For any non-MCP-app origin, both handlers deny everything except the narrow first-party main-frame audio case (Lines 391-404, 440-448), which will silently break clipboard, notifications, geolocation, camera, etc. for the rest of the application outside MCP Apps. This is the same root cause flagged in a prior review (unaddressed), now also present insetPermissionCheckHandler. Consider scoping these restrictions to a dedicated MCP-only session/webContents rather thansession.defaultSession.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/mcp/apps/sandboxRegistry.ts` around lines 376 - 452, Update configureDefaultSessionPermissions so MCP permission handlers do not replace or deny permissions for unrelated application webContents in session.defaultSession. Scope the MCP-specific request and check logic to a dedicated MCP-only session or equivalent webContents boundary, while preserving existing first-party audio handling and MCP consent validation within that scope.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/renderer/src/components/mcp-config/components/McpServers.vue`:
- Around line 525-548: Clear diagnostics.value when starting a new request in
the diagnostics-loading flow, before awaiting mcpClient.getServerDiagnostics, so
selecting a different server cannot display the previous server’s payload while
the new request is pending. Keep the existing request-generation guards and
error handling unchanged.
---
Outside diff comments:
In `@src/main/mcp/mcpClient.ts`:
- Around line 1501-1521: Update listToolsPage, listPromptsPage,
listResourcesPage, and listResourceTemplatesPage to catch errors from their
paginated client calls, use isUnsupportedCapabilityError to identify unsupported
methods, and return an empty page result consistent with the corresponding
non-paginated methods; rethrow all other errors and preserve existing validation
and mapping behavior.
- Around line 1057-1069: Update the abort listener in the elicitation request
flow around handleElicitationRequest so cancelElicitationRequest handles
rejected promises with a catch that logs the error and prevents an unhandled
rejection, matching the existing sampling abort handler behavior. Preserve the
current cancellation message and abort listener registration.
In `@src/main/tool/index.ts`:
- Around line 513-535: Update getMcpDefinition to use the same scoped-only
versus fallback decision as getToolSource(), preventing a conversation lookup
from falling back to globalMcpDefinitions when the requesting conversation lacks
the tool. Preserve global lookup only for unscoped requests so
createExpectedMcpTarget() cannot use another conversation’s MCP binding.
- Around line 455-466: Update the MCP path in preCheckToolPermission to invoke
createExpectedMcpTarget before shouldBrokerMcpTool or permission evaluation,
using the same inputs and behavior as callTool. Ensure unstable or missing
execution bindings throw the stable-binding error during precheck instead of
falling through to createMcpPermissionContext with an unknown server.
---
Duplicate comments:
In `@src/main/mcp/apps/sandboxRegistry.ts`:
- Around line 376-452: Update configureDefaultSessionPermissions so MCP
permission handlers do not replace or deny permissions for unrelated application
webContents in session.defaultSession. Scope the MCP-specific request and check
logic to a dedicated MCP-only session or equivalent webContents boundary, while
preserving existing first-party audio handling and MCP consent validation within
that scope.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 72b71b32-d3c3-4425-a4ee-9ddf2d221057
📒 Files selected for processing (39)
.oxlintrc.jsonsrc/main/mcp/apps/sandboxRegistry.tssrc/main/mcp/mcpClient.tssrc/main/mcp/mcpOAuthManager.tssrc/main/mcp/oauthCredentialStore.tssrc/main/mcp/resultProjection.tssrc/main/mcp/schemaValidation.tssrc/main/mcp/serverIdentity.tssrc/main/session/data/tables/deepchatAssistantBlocks.tssrc/main/tool/index.tssrc/main/tool/permission/toolPermissionBroker.tssrc/renderer/src/components/mcp-config/McpServerForm.vuesrc/renderer/src/components/mcp-config/components/McpServers.vuesrc/renderer/src/components/mcp/McpElicitationDialog.vuesrc/renderer/src/components/message/MessageBlockActivityGroup.vuesrc/renderer/src/components/message/MessageBlockToolCall.vuesrc/renderer/src/components/message/MessageItemAssistant.vuesrc/renderer/src/components/message/messageActivityGroups.tssrc/shared/contracts/events/mcp.events.tssrc/shared/contracts/routes/mcp.routes.tstest/main/mcp/mcpAppHost.test.tstest/main/mcp/mcpAppSandboxProtocol.test.tstest/main/mcp/mcpAppSandboxRegistry.test.tstest/main/mcp/mcpClient.test.tstest/main/mcp/mcpOAuthManager.test.tstest/main/mcp/oauthCredentialStore.test.tstest/main/mcp/schemaValidation.test.tstest/main/mcp/serverIdentity.test.tstest/main/routes/contracts.test.tstest/main/session/data/tables/deepchatAssistantBlocks.test.tstest/main/tool/toolPermissionBroker.test.tstest/main/tool/toolService.test.tstest/renderer/components/McpElicitationDialog.test.tstest/renderer/components/McpServerForm.test.tstest/renderer/components/McpServers.test.tstest/renderer/components/message/MessageBlockActivityGroup.test.tstest/renderer/components/message/MessageBlockToolCall.test.tstest/renderer/components/message/messageActivityGroups.test.tstest/setup.ts
💤 Files with no reviewable changes (1)
- test/main/mcp/mcpAppSandboxProtocol.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
resources/acp-registry/registry.json (2)
695-748: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPoint Harn and OpenCode artifacts at published releases.
burin-labs/harnhas no publishedv0.10.44release, andanomalyco/opencodecurrently publishesv1.18.8, notv1.18.10. Pin these entries to valid release versions/artifacts and verify each archive’s SHA-256 and configuredcmdpath before merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/acp-registry/registry.json` around lines 695 - 748, Update the Harn registry entry around version 0.10.44 to reference an actually published Harn release, and update the nearby OpenCode entry from the unavailable v1.18.10 to the published v1.18.8 release. Replace every platform archive URL, version reference, and SHA-256 with values verified against the corresponding release, and confirm each distribution’s configured cmd path matches the downloaded artifact.Source: MCP tools
470-479: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUpdate the Grok ACP distribution to an available npm artifact
@xai-official/npm registry references do not publish@xai-official/grok@0.2.116. This will fail ACP installation via npm for the Grok agent, so use a valid published package/version or an equivalent direct binary distribution. Also applies to lines 674-683.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/acp-registry/registry.json` around lines 470 - 479, Update the Grok ACP registry entries, including the distribution block near the visible version metadata and the corresponding entry around lines 674-683, to replace the unavailable `@xai-official/grok`@0.2.116 npm artifact with a valid published package/version or equivalent direct binary distribution. Ensure both entries use the same installable distribution configuration.Source: MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@resources/acp-registry/registry.json`:
- Around line 695-748: Update the Harn registry entry around version 0.10.44 to
reference an actually published Harn release, and update the nearby OpenCode
entry from the unavailable v1.18.10 to the published v1.18.8 release. Replace
every platform archive URL, version reference, and SHA-256 with values verified
against the corresponding release, and confirm each distribution’s configured
cmd path matches the downloaded artifact.
- Around line 470-479: Update the Grok ACP registry entries, including the
distribution block near the visible version metadata and the corresponding entry
around lines 674-683, to replace the unavailable `@xai-official/grok`@0.2.116 npm
artifact with a valid published package/version or equivalent direct binary
distribution. Ensure both entries use the same installable distribution
configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d11c1fe2-9cd6-40c4-bdde-ca26f8b89ad5
📒 Files selected for processing (12)
resources/acp-registry/registry.jsonsrc/renderer/src/components/mcp/McpAppView.vuesrc/renderer/src/components/message/MessageItemAssistant.vuesrc/renderer/src/components/message/messageActivityGroups.tssrc/renderer/src/components/sidepanel/ChatSidePanel.vuesrc/renderer/src/stores/ui/sidepanel.tssrc/shared/types/workspace.tstest/renderer/components/ChatSidePanel.test.tstest/renderer/components/McpAppView.test.tstest/renderer/components/message/MessageItemAssistant.test.tstest/renderer/components/message/messageActivityGroups.test.tstest/renderer/stores/sidepanel.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/tool/index.ts (1)
361-396: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winFail closed when MCP tool permission context cannot be created.
createMcpPermissionContextreturnsnullwheneverrequest.conversationIdis missing/blank, and bothcallToolandpreCheckToolPermissiontreat that as "no check needed".MCPToolCall.conversationIdis optional, so make the missing context a permission-check failure instead of an unconditional tool/precheck allow path, or require a non-emptyconversationIdbefore routing to this permission broker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/tool/index.ts` around lines 361 - 396, Fail closed in the MCP routing flow around createMcpPermissionContext when request.conversationId is missing or blank: do not treat a null permissionContext as permission granted. Require a non-empty conversationId before routing through the permission broker, or return a denied permission result from both callTool and preCheckToolPermission; preserve normal authorization behavior when context creation succeeds.
🧹 Nitpick comments (1)
src/main/tool/index.ts (1)
455-467: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
createExpectedMcpTargetresult discarded — used only for its throw side effect.Line 456 calls
this.createExpectedMcpTarget(toolName, definition)and ignores the result; the function is really being used as a stability-binding assertion here. Consider extracting a dedicatedassertStableMcpBinding(toolName, definition): void(or similar) so both call sites (Line 364 and Line 456) express intent clearly instead of one discarding a computed value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/tool/index.ts` around lines 455 - 467, Replace the discarded createExpectedMcpTarget call in the surrounding permission flow with a dedicated void assertion such as assertStableMcpBinding(toolName, definition). Implement the assertion to preserve the existing stability validation and throwing behavior, then update both call sites, including the one near line 364, so neither uses createExpectedMcpTarget solely for side effects.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@resources/acp-registry/registry.json`:
- Line 106: Update the Claude ACP registry entry’s version from 0.64.0 to the
published 0.63.0 release, keeping the existing package name and registry
structure unchanged.
---
Outside diff comments:
In `@src/main/tool/index.ts`:
- Around line 361-396: Fail closed in the MCP routing flow around
createMcpPermissionContext when request.conversationId is missing or blank: do
not treat a null permissionContext as permission granted. Require a non-empty
conversationId before routing through the permission broker, or return a denied
permission result from both callTool and preCheckToolPermission; preserve normal
authorization behavior when context creation succeeds.
---
Nitpick comments:
In `@src/main/tool/index.ts`:
- Around line 455-467: Replace the discarded createExpectedMcpTarget call in the
surrounding permission flow with a dedicated void assertion such as
assertStableMcpBinding(toolName, definition). Implement the assertion to
preserve the existing stability validation and throwing behavior, then update
both call sites, including the one near line 364, so neither uses
createExpectedMcpTarget solely for side effects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 33f2b4aa-ae25-47d4-af1e-d26d745c559c
📒 Files selected for processing (19)
resources/acp-registry/registry.jsonsrc/main/app/composition.tssrc/main/mcp/apps/sandboxProtocol.tssrc/main/mcp/apps/sandboxRegistry.tssrc/main/mcp/mcpClient.tssrc/main/tool/index.tssrc/main/tool/permission/toolPermissionBroker.tssrc/renderer/src/components/mcp-config/components/McpServers.vuesrc/renderer/src/i18n/es-ES/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/pt-BR/mcp.jsontest/main/mcp/mcpAppSandboxProtocol.test.tstest/main/mcp/mcpAppSandboxRegistry.test.tstest/main/mcp/mcpClient.test.tstest/main/tool/toolPermissionBroker.test.tstest/main/tool/toolService.test.tstest/renderer/components/McpServers.test.ts
💤 Files with no reviewable changes (1)
- src/main/app/composition.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- test/main/tool/toolService.test.ts
- src/renderer/src/i18n/fr-FR/settings.json
- src/renderer/src/i18n/he-IL/settings.json
- src/renderer/src/i18n/es-ES/settings.json
- src/renderer/src/i18n/ko-KR/settings.json
- src/main/mcp/mcpClient.ts
Summary
2.0.0, using modern-firstautonegotiation for external stdio and Streamable HTTP while retaining explicit legacy lanes for SSE and in-memory serversinput_required, sampling, elicitation, stable server identity, lossless schemas/results, and redacted diagnosticsmcp-appprotocol, double-iframe sandbox, bounded persistence, permission brokering, consent, lifecycle revocation, and inline/fullscreen/PiP renderingWhy
MCP 2026-07-28 replaces the legacy session-oriented lifecycle with modern per-request negotiation and introduces new discovery, continuation, caching, subscription, authorization, and Apps contracts. DeepChat needs to support the modern wire without breaking existing legacy MCP servers or weakening its renderer/main-process trust boundaries.
User impact
2026-07-28automatically and valid legacy servers fall back without manual configurationUI behavior
Root-cause fixes from manual smoke testing
common.refreshkey instead of the existing localized MCP refresh labelpnpm ... server, which collides with pnpm's ownservercommand; the runbook now usesrun serversize-changednotifications, so taller content was clipped instead of scrolling inside the message viewportValidation
Validation on the branch after merging the latest
dev:pnpm run formatpnpm run i18n:typespnpm run i18npnpm run lintpnpm run typecheckdocs/architecture/mcp-v2-protocol/tasks.mdManual development-build smoke against the official TypeScript SDK pin
cc4b41617ce3601b1290d67216ea0b194a3cd9ac:dual-eraStreamable HTTP: modern2026-07-28negotiation andgreetpasseddual-erastdio: modern2026-07-28negotiation,greet, and process cleanup passedpromptsStreamable HTTP: prompt listing and rendering passedFormal packaged-build, Apps sandbox, authorization, and public-service evidence remains tracked as pending in the manual runbook.
Summary by CodeRabbit