feat(mcp): embed read-only MCP server for agent access to starred repos#250
Merged
Conversation
Add Streamable HTTP + legacy SSE MCP endpoints so agents can search AI-enriched stars without a separate CLI. Backend mounts /mcp (token-gated, config read per request); Electron hosts loopback-only local MCP via IPC snapshot. Settings panel enables toggle, viewable token, and one-click agent JSON. Vector tool is listed only when vector search is configured. Additive settings only — no data-loss migrations. Security/audit hardening: restore helmet CSP, no token mint on mount, stateless Streamable HTTP, loopback bind enforcement, log sanitization for mcp tokens, nginx prefix proxy for /mcp paths.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional MCP access for backend and Electron deployments, including authenticated Streamable HTTP/SSE transports, read-only repository tools, vector-search integration, settings UI, persisted configuration, Docker/nginx exposure, and shared repository search utilities. ChangesMCP repository search
MCP data layer
MCP transports and clients
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client as AI Agent
participant AuthMiddleware
participant MCPTransport
participant MCPTools
participant DataProvider
Client->>AuthMiddleware: POST /mcp with Bearer token
AuthMiddleware->>MCPTransport: Validate enabled & token match
MCPTransport->>MCPTools: Forward JSON-RPC (initialize/tools/call)
MCPTools->>DataProvider: Execute tool (search/stats/vector-search)
DataProvider-->>MCPTools: Return filtered/projected results
MCPTools-->>Client: JSON-RPC result or Streamable HTTP/SSE chunk
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
src/services/electronProxy.ts (1)
3-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate type: reuse
McpServiceConfiginstead of redefining it.
McpLocalConfighas the exact same shape asMcpServiceConfig(src/types/index.ts), which this file already imports from'../types'. Defining a second, structurally-identical type risks the two silently drifting apart on a future field change.-import type { ProxyConfig, Repository, Category, VectorSearchConfig } from '../types'; - -export interface McpLocalConfig { - enabled: boolean; - host: string; - port: number; - token: string; -} +import type { ProxyConfig, Repository, Category, VectorSearchConfig, McpServiceConfig } from '../types'; + +export type McpLocalConfig = McpServiceConfig;🤖 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/services/electronProxy.ts` around lines 3 - 8, Remove the duplicate McpLocalConfig interface from electronProxy.ts and reuse the already imported McpServiceConfig type from ../types wherever McpLocalConfig is referenced, preserving the existing configuration behavior.src/store/useAppStore.ts (1)
594-620: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport the canonical host normalizer for reuse.
normalizeMcpHost/normalizeMcpConfigcorrectly enforce the loopback-only whitelist, but aren't exported.McpSettingsPanel.tsx(same renderer bundle) re-implements a weaker, ad-hoc version of this check inline in two places instead of importing this one. Exporting it would let consumers share one source of truth for a security-relevant rule instead of maintaining parallel implementations that can drift.🤖 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/store/useAppStore.ts` around lines 594 - 620, Export normalizeMcpHost from useAppStore so McpSettingsPanel.tsx can reuse the canonical loopback-only validation. Update both inline host-normalization checks in McpSettingsPanel.tsx to import and call normalizeMcpHost, removing the weaker duplicated logic while preserving the existing configuration behavior.src/components/settings/McpSettingsPanel.tsx (1)
51-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate, weaker host-normalization logic.
mcpConfig.host === '0.0.0.0' || !mcpConfig.host ? '127.0.0.1' : mcpConfig.host(repeated at line 121-122) only guards against the wildcard/empty case, unlikeuseAppStore.ts'snormalizeMcpHost, which enforces the full loopback whitelist. In practicemcpConfig.hostis already sanitized by the store'ssetMcpConfig/normalizeMcpConfigbefore it reaches this component, so this isn't exploitable today — but it's a second, weaker implementation of a security-relevant rule that could silently diverge.🤖 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/components/settings/McpSettingsPanel.tsx` around lines 51 - 63, The baseUrl logic in the useMemo duplicates weaker MCP host normalization. Reuse the existing normalizeMcpHost helper from useAppStore.ts when determining the Electron local MCP host, removing the inline wildcard/empty fallback while preserving the loopback whitelist behavior and existing URL construction.
🤖 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 `@electron/main.js`:
- Around line 128-151: Update the mcp:setConfig handler to stop the currently
running mcpServer whenever the MCP configuration changes while enabled, not only
when the new configuration disables MCP. Ensure the existing mcpServer.stop()
lifecycle allows the renderer’s subsequent mcp:start call to bind the updated
host and port, while preserving the disabled configuration behavior.
In `@electron/mcpLocalServer.js`:
- Around line 418-429: Update getStatus to safely handle a missing state.config,
matching the optional-chaining guard used by start. Apply the guard to both host
and port access while preserving the existing localhost fallback, default port,
and returned status structure.
In `@nginx.conf.template`:
- Around line 53-74: Update the nginx MCP routing locations so only the exact
/mcp path and paths beginning with /mcp/ are proxied. Replace the broad location
/mcp rule with separate exact and /mcp/ prefix handling, or an equivalent
segment-anchored regex, while preserving the existing proxy settings and nested
SSE/message support.
In `@README.md`:
- Line 34: The MCP feature descriptions must state that MCP settings are
available only in backend or packaged/Electron deployments, not pure frontend
deployments. Update the English description in README.md lines 34-34 and add the
equivalent qualification to the Chinese description in README_zh.md lines 31-31;
keep the existing feature details unchanged.
In `@server/src/mcp/http.ts`:
- Around line 85-133: Update the legacy /sse handler’s res.on('close') cleanup
to close its SSEServerTransport after removing the session, matching the cleanup
in the /mcp/sse handler. Prefer extracting the duplicated SSE connection setup
into a shared helper used by both routes, preserving each route’s distinct
message endpoint while ensuring disconnected clients close their transport (and
connected server resources).
In `@server/src/mcp/provider.ts`:
- Around line 227-235: Add a shared fetchWithTimeout helper in the provider flow
that aborts requests after the configured timeout, then use it for both the
embedding provider request in embedQuery and the vector worker /query request in
vectorSearch. Preserve the existing request options and error handling while
ensuring neither outbound fetch can hang indefinitely.
In `@src/components/settings/McpSettingsPanel.tsx`:
- Around line 118-157: Move the MCP lifecycle and snapshot synchronization
currently handled by the useEffect in McpSettingsPanel into a subscription owned
by a long-lived app component. Keep start/stop driven by MCP configuration, and
refresh pushSnapshot whenever repositories, customCategories, or
vectorSearchConfig changes so switching tabs or unmounting McpSettingsPanel
cannot leave the Electron snapshot stale.
In `@src/utils/repoSearch.ts`:
- Around line 136-145: Update the guard in the isEdited filtering block to use
allCategories.length > 0, so the filter runs only after categories are loaded;
leave the existing customized filtering behavior unchanged.
---
Nitpick comments:
In `@src/components/settings/McpSettingsPanel.tsx`:
- Around line 51-63: The baseUrl logic in the useMemo duplicates weaker MCP host
normalization. Reuse the existing normalizeMcpHost helper from useAppStore.ts
when determining the Electron local MCP host, removing the inline wildcard/empty
fallback while preserving the loopback whitelist behavior and existing URL
construction.
In `@src/services/electronProxy.ts`:
- Around line 3-8: Remove the duplicate McpLocalConfig interface from
electronProxy.ts and reuse the already imported McpServiceConfig type from
../types wherever McpLocalConfig is referenced, preserving the existing
configuration behavior.
In `@src/store/useAppStore.ts`:
- Around line 594-620: Export normalizeMcpHost from useAppStore so
McpSettingsPanel.tsx can reuse the canonical loopback-only validation. Update
both inline host-normalization checks in McpSettingsPanel.tsx to import and call
normalizeMcpHost, removing the weaker duplicated logic while preserving the
existing configuration behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 86568fe3-8703-40f8-a8c7-c436c6857785
⛔ Files ignored due to path filters (1)
server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (32)
.gitignoreDOCKER.mdREADME.mdREADME_zh.mdelectron/main.jselectron/mcpLocalServer.jselectron/package.jsonelectron/preload.jsnginx.conf.templatescripts/build-desktop.jsserver/package.jsonserver/src/index.tsserver/src/mcp/http.tsserver/src/mcp/provider.tsserver/src/mcp/repoSearch.tsserver/src/mcp/server.tsserver/src/mcp/settings.tsserver/src/mcp/tools.tsserver/src/routes/mcp.tsserver/src/services/logSanitizer.tsserver/tests/mcp/repoSearch.test.tsserver/tests/mcp/routes.test.tssrc/components/SearchBar.tsxsrc/components/SettingsPanel.tsxsrc/components/settings/McpSettingsPanel.tsxsrc/components/settings/index.tssrc/services/backendAdapter.tssrc/services/electronProxy.tssrc/store/useAppStore.tssrc/types/index.tssrc/utils/repoSearch.test.tssrc/utils/repoSearch.ts
- Electron: stop MCP server when bind host/port changes so start rebinds - Electron getStatus: null-safe config access - nginx: exact /mcp + ^~ /mcp/ only (no /mcpxyz leak) - SSE: shared connectSse helper closes transport on disconnect for all aliases - provider: 15s timeout on embedding and vector worker fetch - App-level mcpElectronBridge keeps snapshots live after leaving settings tab - Reuse McpServiceConfig type; drop dead allCategories.length >= 0 guard - README: document MCP requires backend or Electron
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/services/mcpElectronBridge.ts`:
- Around line 31-34: Normalize the displayed MCP endpoint’s host consistently
with the server host logic: update the endpoint construction in McpSettingsPanel
to map an unset host, 0.0.0.0, or :: to 127.0.0.1, preferably by reusing a
shared host-normalization helper. Preserve configured non-wildcard hosts
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: 3c0497c8-6127-41e0-a12f-57925f9346fb
📒 Files selected for processing (12)
README.mdREADME_zh.mdelectron/main.jselectron/mcpLocalServer.jsnginx.conf.templateserver/src/mcp/http.tsserver/src/mcp/provider.tssrc/App.tsxsrc/components/settings/McpSettingsPanel.tsxsrc/services/electronProxy.tssrc/services/mcpElectronBridge.tssrc/utils/repoSearch.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- README_zh.md
- src/utils/repoSearch.ts
- src/services/electronProxy.ts
- electron/main.js
- server/src/mcp/provider.ts
- server/src/mcp/http.ts
- electron/mcpLocalServer.js
Unify MCP host handling for unset / 0.0.0.0 / :: / [::] → 127.0.0.1 across settings UI copy URL, Electron bridge, and store persistence so agents never see invalid http://:::port URLs.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/utils/mcpHost.ts`:
- Around line 14-15: Normalize the IPv6 loopback consistently: update
normalizeMcpHost in src/utils/mcpHost.ts#L14-L15 to return MCP_DEFAULT_HOST for
::1, and update the corresponding expectation in
src/utils/mcpHost.test.ts#L14-L18 so normalizeMcpHost('::1') is expected to
return MCP_DEFAULT_HOST.
🪄 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: a5da2af8-f19e-458b-b5ff-6d277810d74b
📒 Files selected for processing (5)
src/components/settings/McpSettingsPanel.tsxsrc/services/mcpElectronBridge.tssrc/store/useAppStore.tssrc/utils/mcpHost.test.tssrc/utils/mcpHost.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/services/mcpElectronBridge.ts
- src/store/useAppStore.ts
- src/components/settings/McpSettingsPanel.tsx
IPv6 loopback was preserved as ::1, producing invalid unbracketed http://::1:port URLs in the settings panel. Map ::1/[::1] to 127.0.0.1.
Installed client had mcpLocalServer.js but main.js/preload never exposed IPC or started the listener, so Settings could show a token while nothing bound 127.0.0.1:3927. Merge production proxy/menu shell with MCP setConfig/start/stop/pushSnapshot handlers and preload electronAPI.mcp.
Electron mcpLocalServer previously only accepted /sse as a path alias and returned JSON health on GET. Add real HTTP+SSE: GET /sse (or /mcp/sse) emits endpoint event; POST /messages?sessionId= returns 202 and writes JSON-RPC results on the open stream. Settings UI shows SSE copy helper; docs list desktop and docker SSE URLs.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
electron/main.js (1)
335-342: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
normalizeMcpHostlogic is redundant.The final
return '127.0.0.1'(Line 341) already covers every input that isn't the exact literal'127.0.0.1', so the explicit alias checks for0.0.0.0,::,localhost, etc. never change the outcome — the function always returns'127.0.0.1'regardless of input. Functionally correct (loopback-only enforced), but the branching implies intent that isn't real.♻️ Suggested simplification
-function normalizeMcpHost(rawHost) { - const host = typeof rawHost === 'string' && rawHost.trim() ? rawHost.trim() : '127.0.0.1'; - if (host === '0.0.0.0' || host === '::' || host === '[::]' || host === '::1' || host === '[::1]' || host === 'localhost') { - return '127.0.0.1'; - } - if (host === '127.0.0.1') return host; - return '127.0.0.1'; -} +// Loopback-only: any value other than the exact '127.0.0.1' literal is coerced to it. +function normalizeMcpHost(_rawHost) { + return '127.0.0.1'; +}🤖 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 `@electron/main.js` around lines 335 - 342, Simplify normalizeMcpHost by removing the redundant alias checks and conditional branches; since every input must resolve to the loopback address, return '127.0.0.1' directly while preserving the function’s current behavior.
🤖 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 `@electron/main.js`:
- Around line 218-239: Update applyProxy so the “[Proxy] Applied” log never
includes proxy credentials from config.username or config.password. Preserve the
full authenticated proxyUrl for setProxy, but log a redacted URL or equivalent
representation with credentials removed while retaining useful host and port
context.
- Around line 35-43: Update the did-fail-load handler on mainWindow.webContents
to avoid reloading the same fallbackPath when the failed validatedURL already
refers to it, preventing a self-triggered infinite loop. Preserve the existing
fallback load for failures from other paths, and use the existing
possiblePaths/fallbackPath values rather than introducing unrelated retry
behavior.
- Around line 15-23: Update the BrowserWindow webPreferences in createWindow()
so webSecurity, allowRunningInsecureContent, and devTools are enabled only when
isDev is true, or otherwise disabled/removed for production; preserve the
existing secure settings and preload configuration.
- Around line 262-295: Update the SOCKS5 handshake Promise in the config.type
=== 'socks5' branch to accumulate incoming data across data events and parse
each response only once the required bytes are available. Preserve the greeting
and authentication state transitions, and resolve success only after a complete
valid response; incomplete fragments must wait for more data rather than falling
through to success.
---
Nitpick comments:
In `@electron/main.js`:
- Around line 335-342: Simplify normalizeMcpHost by removing the redundant alias
checks and conditional branches; since every input must resolve to the loopback
address, return '127.0.0.1' directly while preserving the function’s current
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2fab6151-e6fb-4481-bfe5-58c765dcc67c
📒 Files selected for processing (2)
electron/main.jselectron/preload.js
🚧 Files skipped from review as they are similar to previous changes (1)
- electron/preload.js
- webSecurity true, no mixed content, DevTools only in isDev - did-fail-load fallback only once (no reload loop) - redact proxy credentials in console logs - buffer SOCKS5 handshake TCP fragments before parse - stop MCP server on quit; simplify loopback host normalizer
build-desktop.yml regenerated electron/main.js and preload.js without MCP IPC, so workflow packages never listened on :3927 despite UI tokens. Verify committed electron/* (main/preload/mcpLocalServer) instead.
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
electron/mcpLocalServer.js (1)
315-317: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: early-return URL skips the loopback normalization applied elsewhere.
The already-running branch reports
state.config.hostverbatim, whereas the bind path (Lines 320-324) andgetStatus(Lines 508-511) normalize0.0.0.0/::to127.0.0.1. In the Electron flowconfig.hostis always127.0.0.1, so there's no live bug, but since this module is standalone a non-loopback config value would produce a misleading URL here. Consider reusing the same normalization for consistency.🤖 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 `@electron/mcpLocalServer.js` around lines 315 - 317, Update the already-running branch in the server startup flow to normalize state.config.host using the same 0.0.0.0/:: to 127.0.0.1 logic used by the bind path and getStatus before constructing the returned URL. Keep the existing success response and port/path unchanged.src/utils/repoSearch.test.ts (1)
1-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd filter-branch coverage to both
applyRepoFilterstest suites. Both mirrored implementations ofapplyRepoFiltershave several untested conditional branches (isAnalyzed, isSubscribed, isCategoryLocked, isEdited, analysisFailed, maxStars, sort variants for the frontend; the same set minus isEdited for the MCP server). This is the same class of gap that let a real bug (tautologicalisEditedguard) ship undetected previously.
src/utils/repoSearch.test.ts#L1-L122: add cases forisAnalyzed,isSubscribed,isCategoryLocked,isEdited(withallCategories),analysisFailed,maxStars, andsortBy=updated/name/starred.server/tests/mcp/repoSearch.test.ts#L1-L56: import and directly testapplyRepoFiltersforlanguages,tags,platforms,isAnalyzed,isSubscribed,isCategoryLocked,analysisFailed,minStars/maxStars, since this function backs the externally-facinggsm_search_repostool 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 `@src/utils/repoSearch.test.ts` around lines 1 - 122, Add direct filter-branch coverage to src/utils/repoSearch.test.ts lines 1-122 for applyRepoFilters: test isAnalyzed, isSubscribed, isCategoryLocked, isEdited with allCategories, analysisFailed, maxStars, and updated/name/starred sorting. In server/tests/mcp/repoSearch.test.ts lines 1-56, import and test applyRepoFilters for languages, tags, platforms, isAnalyzed, isSubscribed, isCategoryLocked, analysisFailed, and minStars/maxStars; use fixtures that distinguish matching and non-matching repositories.server/src/mcp/tools.ts (1)
20-21: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
getVectorAvailability()re-queried on every stateless request.Since
createMcpServer()/registerMcpTools()runs fresh per-request under the stateless Streamable HTTP transport (perhandleStreamableinserver/src/mcp/http.ts, which usessessionIdGenerator: undefined), this availability check (2 SQLite queries) executes on every MCP call, not justgsm_vector_searchinvocations. Consider memoizing the result briefly (e.g., short TTL cache) to avoid repeated DB round-trips on the hot path.Also applies to: 147-169
🤖 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 `@server/src/mcp/tools.ts` around lines 20 - 21, Memoize the result of getVectorAvailability() with a short TTL shared across fresh registerMcpTools() invocations, so stateless requests do not repeat the two SQLite queries on every MCP call. Update the availability checks in registerMcpTools, including the path around lines 147-169, while preserving refresh behavior after the TTL expires.nginx.conf.template (1)
53-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider de-duplicating the repeated proxy directives across the four MCP location blocks.
= /mcp,^~ /mcp/,= /sse, and= /messageseach repeat ~15 lines of near-identicalproxy_set_header/timeout/buffering config, and have already drifted slightly (e.g./messagesomitsConnection '',proxy_cache off,chunked_transfer_encoding onthat the other blocks set). An nginx snippet file (include mcp_proxy_common.conf;) would keep these in sync going forward.🤖 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 `@nginx.conf.template` around lines 53 - 126, The four MCP-related location blocks duplicate shared proxy configuration and have drifted. Create an nginx snippet containing the common proxy directives, include it from = /mcp, ^~ /mcp/, = /sse, and = /messages, and preserve any location-specific settings such as differing timeouts or body-size limits.
🤖 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 `@server/src/mcp/http.ts`:
- Around line 55-72: The `/mcp` endpoint currently accepts methods beyond POST,
allowing stateless GET and DELETE requests into handleStreamable without session
validation. Replace the app.all('/mcp', ...) routing with POST-only handling,
and add method handling that returns HTTP 405 for GET and DELETE requests while
preserving the existing handleStreamable behavior for POST.
In `@server/src/routes/mcp.ts`:
- Around line 50-69: Update the resetToken branch in the MCP configuration
handler to call resetMcpToken() only when isMcpEnabled() is true; when MCP is
disabled, preserve the existing token without minting or rotating one, while
retaining the current enabled-path behavior.
In `@src/App.tsx`:
- Around line 133-138: The App-level bridge effect must wait for backend
discovery and hydration before starting the Electron MCP bridge. Coordinate
startMcpElectronBridge with the backend.init() completion or emit an explicit
backend-readiness transition that invokes pushLifecycle(), ensuring a
successfully initialized backend stops any locally started MCP bridge while
preserving session-wide cleanup via stopMcpElectronBridge.
In `@src/components/settings/McpSettingsPanel.tsx`:
- Around line 178-195: Update handleResetToken to return without generating or
resetting a token when mcpConfig.enabled is false, and disable the associated
reset action in the UI. In the backend updateMcpConfig/resetMcpToken flow,
reject or ignore resetToken requests while MCP is disabled so direct API calls
preserve the same behavior.
- Around line 403-424: Add accessible aria-labels to the icon-only copy buttons
in the MCP settings URL rows, identifying the HTTP endpoint for the button using
mcpHttpUrl and the SSE endpoint for the button using mcpSseUrl; leave their
existing click behavior unchanged.
In `@src/services/backendAdapter.ts`:
- Around line 770-800: Redact the token returned by getMcpStatus and
updateMcpConfig before it can reach fetchWithTimeout’s debug responseBody
logging. Update the shared response-preview/logging path to mask the token for
the /mcp/status and /mcp/config endpoints, or suppress response-body logging for
those requests, while preserving the methods’ returned response objects.
In `@src/services/mcpElectronBridge.ts`:
- Around line 22-57: Update pushLifecycle to serialize Electron MCP IPC
operations through an async queue or generation guard, ensuring each enabled
cycle awaits setConfig, pushSnapshot, then start, while disable/backend paths
await stop and supersede stale work. Catch and handle rejected IPC promises so
lifecycle failures do not become unhandled rejections.
In `@src/store/useAppStore.ts`:
- Around line 2233-2234: The persistence configuration around mcpConfig must
stop storing backend MCP bearer tokens. Update the partialize state selection so
only safe Electron-local preferences are persisted, while backend connection
settings and tokens remain memory-only; preserve any non-sensitive preferences
required by the existing flow.
---
Nitpick comments:
In `@electron/mcpLocalServer.js`:
- Around line 315-317: Update the already-running branch in the server startup
flow to normalize state.config.host using the same 0.0.0.0/:: to 127.0.0.1 logic
used by the bind path and getStatus before constructing the returned URL. Keep
the existing success response and port/path unchanged.
In `@nginx.conf.template`:
- Around line 53-126: The four MCP-related location blocks duplicate shared
proxy configuration and have drifted. Create an nginx snippet containing the
common proxy directives, include it from = /mcp, ^~ /mcp/, = /sse, and =
/messages, and preserve any location-specific settings such as differing
timeouts or body-size limits.
In `@server/src/mcp/tools.ts`:
- Around line 20-21: Memoize the result of getVectorAvailability() with a short
TTL shared across fresh registerMcpTools() invocations, so stateless requests do
not repeat the two SQLite queries on every MCP call. Update the availability
checks in registerMcpTools, including the path around lines 147-169, while
preserving refresh behavior after the TTL expires.
In `@src/utils/repoSearch.test.ts`:
- Around line 1-122: Add direct filter-branch coverage to
src/utils/repoSearch.test.ts lines 1-122 for applyRepoFilters: test isAnalyzed,
isSubscribed, isCategoryLocked, isEdited with allCategories, analysisFailed,
maxStars, and updated/name/starred sorting. In
server/tests/mcp/repoSearch.test.ts lines 1-56, import and test applyRepoFilters
for languages, tags, platforms, isAnalyzed, isSubscribed, isCategoryLocked,
analysisFailed, and minStars/maxStars; use fixtures that distinguish matching
and non-matching repositories.
🪄 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: a60866cd-e6b2-4bb9-a832-a713d3762e07
⛔ Files ignored due to path filters (1)
server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (37)
.github/workflows/build-desktop.yml.gitignoreDOCKER.mdREADME.mdREADME_zh.mdelectron/main.jselectron/mcpLocalServer.jselectron/package.jsonelectron/preload.jsnginx.conf.templatescripts/build-desktop.jsserver/package.jsonserver/src/index.tsserver/src/mcp/http.tsserver/src/mcp/provider.tsserver/src/mcp/repoSearch.tsserver/src/mcp/server.tsserver/src/mcp/settings.tsserver/src/mcp/tools.tsserver/src/routes/mcp.tsserver/src/services/logSanitizer.tsserver/tests/mcp/repoSearch.test.tsserver/tests/mcp/routes.test.tssrc/App.tsxsrc/components/SearchBar.tsxsrc/components/SettingsPanel.tsxsrc/components/settings/McpSettingsPanel.tsxsrc/components/settings/index.tssrc/services/backendAdapter.tssrc/services/electronProxy.tssrc/services/mcpElectronBridge.tssrc/store/useAppStore.tssrc/types/index.tssrc/utils/mcpHost.test.tssrc/utils/mcpHost.tssrc/utils/repoSearch.test.tssrc/utils/repoSearch.ts
- Streamable /mcp is POST-only; GET/DELETE return 405 (legacy SSE on /mcp/sse) - Do not mint/reset MCP token while disabled (API + UI) - Start Electron MCP bridge after backend.init; serialize IPC lifecycle - Persist MCP prefs without token; keep desktop token in sessionStorage - Redact gsm_mcp_* in frontend logs and backendAdapter debug bodies - URL copy buttons get aria-labels; normalize already-running MCP URL
- Disable Reset Token button while MCP is off (handler + API already guard) - Redact token fields and gsm_mcp_* values in backendAdapter responseBody debug previews so /mcp/status and /mcp/config cannot leak into logs
Local MCP previously always returned "vector search not supported". Pass embedding + worker runtime config via IPC snapshot and implement embed + Vectorize query in mcpLocalServer when Vector Search is enabled in the app. tools/list only exposes gsm_vector_search when ready.
SiliconFlow/OpenAI baseUrl in the app is the host (e.g. api.siliconflow.cn); the client appends /v1/embeddings. Local MCP was appending only /embeddings (or wrongly defaulting base to .../v1), producing 404 page not found and breaking gsm_vector_search. Match vectorSearchService.ts for all providers.
Desktop MCP tokens were session-only and auto-regenerated after restart, forcing users to update agent configs silently. Persist full mcpConfig (including token) via Zustand IndexedDB partialize. Backend already stores encrypted mcp_token in SQLite; ensure paths only mint when missing and document that tokens change solely on explicit Reset Token.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/mcp), and Electron standalone (loopback HTTP + IPC snapshot). Pure SPA hides the MCP settings tab.gsm_vector_searchis listed only when vector search is configured/enabled.Audit fixes (this PR)
0.0.0.0)./mcpcovering/mcp/ssepaths; token redaction in log sanitizer.Test plan
serverunit tests:tests/mcp/repoSearch+ pure token testsrepoSearch+SearchBartests passbetter-sqlite3native (Node LTS): runserver/tests/mcp/routes.test.ts(DB suite skips on Node 26 without rebuild)gsm_status/gsm_search_reposhttp://localhost:8080/mcpwith Bearer token127.0.0.1onlySummary by CodeRabbit