fix: rtk status - #1541
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughCentralizes builtin knowledge config storage and migration via ConfigPresenter and a new config API, updates MCP in-memory servers to read enabled configs at runtime, adds concurrency-safe KnowledgeStorePresenter initialization, and simplifies RTK health verification to version-only probes (failureStage now 'version'). Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Vue as BuiltinKnowledgeSettings
participant ConfigClient
participant Route as ConfigRouteHandler
participant ConfigPresenter
participant McpHelper as McpConfHelper
participant MCP as MCP Store
participant EventBus
participant KnowledgeP as KnowledgePresenter
User->>Vue: Edit/save builtin config
Vue->>ConfigClient: setKnowledgeConfigs(nextConfigs)
ConfigClient->>Route: POST config.setKnowledgeConfigs
Route->>ConfigPresenter: setKnowledgeConfigs(configs)
ConfigPresenter->>McpHelper: migrateBuiltinKnowledgeConfigsFromEnv()
McpHelper->>MCP: read mcpServers.builtinKnowledge.env
McpHelper->>MCP: persist cleared env
McpHelper-->>ConfigPresenter: migrated configs
ConfigPresenter->>MCP: persist configs
ConfigPresenter->>EventBus: emit CONFIG_CHANGED
EventBus->>KnowledgeP: CONFIG_CHANGED
KnowledgeP->>ConfigPresenter: getKnowledgeConfigs()
KnowledgeP->>KnowledgeP: diff & apply updates
ConfigPresenter-->>Route: return updated configs
Route-->>ConfigClient: response
ConfigClient-->>Vue: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/presenter/knowledgePresenter/index.ts (1)
140-157:⚠️ Potential issue | 🟡 MinorRejected in-flight init will throw out of
update().
await this.storePresenterInitTasks.get(config.id)re-throws if the originalcreateStorePresenterrejected (e.g., DuckDB open failure). Sinceupdateis called fromPromise.all(diffs.updated.map(...))insyncKnowledgeConfigChanges, a single failed init poisons the whole reconciliation batch. Consider guarding the await so a previously-failed init doesn't break unrelated updates.🛡️ Possible guard
- const initializingRag = await this.storePresenterInitTasks.get(config.id) - if (initializingRag) { - initializingRag.updateConfig(config) - } + const initTask = this.storePresenterInitTasks.get(config.id) + if (initTask) { + try { + const initializingRag = await initTask + initializingRag.updateConfig(config) + } catch (error) { + console.warn(`[RAG] Skipping update for in-flight failed init ${config.id}:`, error) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/knowledgePresenter/index.ts` around lines 140 - 157, The update method can throw when awaiting a previously-rejected init promise from storePresenterInitTasks which poisons the whole reconciliation; change the await of this.storePresenterInitTasks.get(config.id) inside update to be guarded so a rejected init does not rethrow (for example, retrieve the promise, await it with a try/catch or use .catch(() => undefined) and proceed only if it resolved to an object), keep the existing behavior of calling initializingRag.updateConfig(config) when the init succeeded, and leave closeStorePresenterIfExists/config-disabled path unchanged; refer to update, storePresenterInitTasks, getStorePresenter and createStorePresenter to locate the logic to modify.
🧹 Nitpick comments (9)
src/main/lib/agentRuntime/rtkRuntimeService.ts (1)
600-611: Skip the redundantrtk --versionprobe for the system candidate.When
candidate.source === 'system',candidate.commandis'rtk', so the first--versioncall (lines 589–598) and this second call (lines 600–611) execute the exact same process with the same env. The current test casetries system RTK when bundled RTK version check failsalready documents this with the sequence['rtk', ['--version']], ['rtk', ['--version']]after the bundled probe.The PATH-resolvability assertion is only meaningful for the bundled candidate (i.e., to confirm
prependBundledRuntimeToEnvactually surfacedrtkon PATH). Consider short-circuiting for the system candidate.♻️ Proposed refactor
- const resolvedRtk = await this.runCommandImpl('rtk', ['--version'], { - env: baseEnv, - timeoutMs: RTK_HEALTH_TIMEOUT_MS - }) - if (resolvedRtk.code !== 0) { - throw new RtkHealthCheckError( - 'version', - resolvedRtk.stderr.trim() || - resolvedRtk.stdout.trim() || - 'rtk is not resolvable via injected PATH' - ) - } + if (candidate.source === 'bundled') { + const resolvedRtk = await this.runCommandImpl('rtk', ['--version'], { + env: baseEnv, + timeoutMs: RTK_HEALTH_TIMEOUT_MS + }) + if (resolvedRtk.code !== 0) { + throw new RtkHealthCheckError( + 'version', + resolvedRtk.stderr.trim() || + resolvedRtk.stdout.trim() || + 'rtk is not resolvable via injected PATH' + ) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/lib/agentRuntime/rtkRuntimeService.ts` around lines 600 - 611, The second redundant probe calling this.runCommandImpl('rtk', ['--version'], ...) should be skipped when the runtime candidate is the system one; detect candidate.source === 'system' (and/or candidate.command === 'rtk') and short-circuit before calling runCommandImpl a second time so you only perform the PATH-resolvability check for bundled candidates (where prependBundledRuntimeToEnv was applied). Ensure the existing RtkHealthCheckError logic remains for failures of the bundled check and do not change error shape—only avoid invoking runCommandImpl for system candidates.test/main/lib/agentRuntime/rtkRuntimeService.test.ts (1)
69-118: Helper trio is clean and tightens regression coverage.
createHealthCheckRunCommanddefaults make the happy-path tests very readable, andexpectNoHealthCommandProbesis a useful guardrail against accidentally re-introducingrewrite/read/gain/find/ls/rgprobes during health checks. One nit:createHealthCheckServicediffers fromcreateServiceonly in thereplaceWithRuntimeCommandmock; consider parameterizingcreateServiceinstead to drop the duplication. Optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/lib/agentRuntime/rtkRuntimeService.test.ts` around lines 69 - 118, The helper duplication can be removed by parameterizing the existing createService factory so tests reuse it instead of a separate createHealthCheckService: update createService to accept an optional overrides object (e.g., replaceWithRuntimeCommand, getShellEnvironment, runCommand, getPath) and move the unique replaceWithRuntimeCommand mock currently in createHealthCheckService into that overrides parameter; then replace createHealthCheckService usages with calls to createService({ replaceWithRuntimeCommand: vi.fn((command)=> command === 'rtk' ? '/runtime/rtk/rtk.exe' : command), runCommand }) so tests keep behavior while eliminating the duplicate function. Ensure symbols referenced are createService, createHealthCheckService, and replaceWithRuntimeCommand when making the change.src/renderer/settings/components/BuiltinKnowledgeSettings.vue (1)
936-942: Surface load failures to the user.
loadBuiltinConfigsilently swallows errors with only aconsole.error. If the IPC call fails (or returns a validation error), the panel will appear empty with no indication to the user, and any subsequent save will overwrite the (unloaded) configs with whatever is currently inbuiltinConfigs.value(i.e.[]). Consider toasting on failure and/or guarding saves until at least one successful load has happened.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/settings/components/BuiltinKnowledgeSettings.vue` around lines 936 - 942, loadBuiltinConfig currently swallows errors with console.error, so surface failures to the user and prevent accidental overwrites: when catching errors from configClient.getKnowledgeConfigs() (in loadBuiltinConfig), call the app's user-facing notification/toast API to show the failure (include the error message) and set a boolean flag like builtinConfigsLoaded/hasLoadedBuiltinConfigs to false; on successful load set it to true and populate builtinConfigs.value; update the save path (e.g., the save handler or saveBuiltinConfig and any save button disabled state) to guard against saving when builtinConfigsLoaded is false so saves are disabled or rejected until a successful load completes.src/shared/contracts/domainSchemas.ts (1)
21-33: Consider tightening numeric fields againstNaN/ negative values.
dimensions,chunkSize,chunkOverlap, andfragmentsNumberare declared as barez.number(), which in Zod acceptsNaNand negative values. The renderer initializeseditingBuiltinConfig.dimensionstoNaNand serializingNaNover IPC turns intonull, which would then fail validation in confusing ways. Adding.int().positive()(or at minimum.finite()/.nonnegative()where appropriate) would catch invalid configs at the contract boundary instead of letting them propagate.Does Zod v4 z.number() accept NaN by default?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/contracts/domainSchemas.ts` around lines 21 - 33, BuiltinKnowledgeConfigSchema uses bare z.number() which accepts NaN and negatives (Zod v4 z.number() does accept NaN by default), so update the schema to validate numeric fields: change dimensions and fragmentsNumber to z.number().int().positive(), change chunkSize and chunkOverlap to z.number().finite().nonnegative().optional() (or .int().nonnegative() if integers are expected), and keep any optional chaining (.optional()) as needed so invalid NaN/negative values are rejected at the contract boundary; apply these changes on the BuiltinKnowledgeConfigSchema definition to prevent NaN/null propagation.src/main/presenter/knowledgePresenter/index.ts (1)
80-85: Listener fires for every MCP config change, not only knowledge-config changes.
MCP_EVENTS.CONFIG_CHANGEDis emitted bysetMcpServers,setMcpEnabled,setMcpServerEnabled, batch import, etc. — not just knowledge-config saves. Each of those will now trigger a fullconfigP.getKnowledgeConfigs()(which can itself run the migration helper) and a diff. It's idempotent, but it is a fair amount of work per mcp change. If you add a dedicated knowledge-config event upstream (see the comment onconfigPresenter.setKnowledgeConfigs), this listener can be narrowed accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/knowledgePresenter/index.ts` around lines 80 - 85, The current listener on eventBus for MCP_EVENTS.CONFIG_CHANGED calls syncKnowledgeConfigChanges() for every MCP config change, causing unnecessary work; update the code to only react to knowledge-config saves by either (a) subscribing to a new dedicated event emitted by configPresenter.setKnowledgeConfigs (e.g., MCP_EVENTS.KNOWLEDGE_CONFIG_CHANGED) or (b) inspecting the CONFIG_CHANGED event payload inside the eventBus.on callback and only invoking this.syncKnowledgeConfigChanges() when the change pertains to knowledge configs; locate the handler using eventBus.on(MCP_EVENTS.CONFIG_CHANGED, ...) and the method syncKnowledgeConfigChanges to implement the conditional or switch to the new event name.src/main/presenter/configPresenter/mcpConfHelper.ts (1)
461-491: Avoid creating a new array when nothing was actually migrated.When
env.configsexists but is empty or fully deduplicated againstexistingConfigs, the function still returns a brand-new array ([...existingConfigs]) and writes the mcp store. The caller inconfigPresenter.getKnowledgeConfigs()(lines 2633–2635) uses a reference check (migratedConfigs !== configs) and will trigger a redundantsetKnowledgeConfigs(which itself fires off a heavygetMcpServers()and broadcastsMCP_EVENTS.CONFIG_CHANGED). It is a one-time cost (env.configs is removed after the first run) but easy to avoid.♻️ Proposed fix — only return a new array when something was actually merged
- const legacyConfigs = Array.isArray(env.configs) - ? (env.configs.filter( - (config): config is BuiltinKnowledgeConfig => - Boolean(config) && - typeof config === 'object' && - typeof (config as { id?: unknown }).id === 'string' - ) as BuiltinKnowledgeConfig[]) - : [] - const mergedConfigs = [...existingConfigs] - const existingIds = new Set(existingConfigs.map((config) => config.id)) - - for (const config of legacyConfigs) { - if (!existingIds.has(config.id)) { - mergedConfigs.push(config) - existingIds.add(config.id) - } - } - - const migratedEnv = { ...env } - delete migratedEnv.configs - mcpServers.builtinKnowledge = { - ...builtinKnowledge, - env: migratedEnv - } - this.mcpStore.set('mcpServers', mcpServers) - - return mergedConfigs + const legacyConfigs = Array.isArray(env.configs) + ? (env.configs.filter( + (config): config is BuiltinKnowledgeConfig => + Boolean(config) && + typeof config === 'object' && + typeof (config as { id?: unknown }).id === 'string' + ) as BuiltinKnowledgeConfig[]) + : [] + const existingIds = new Set(existingConfigs.map((config) => config.id)) + const newlyMerged = legacyConfigs.filter((config) => !existingIds.has(config.id)) + + const migratedEnv = { ...env } + delete migratedEnv.configs + mcpServers.builtinKnowledge = { + ...builtinKnowledge, + env: migratedEnv + } + this.mcpStore.set('mcpServers', mcpServers) + + return newlyMerged.length > 0 ? [...existingConfigs, ...newlyMerged] : existingConfigs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/configPresenter/mcpConfHelper.ts` around lines 461 - 491, The code always creates mergedConfigs = [...existingConfigs] and writes mcpServers to the store even when no legacy configs are added; change the logic in the block handling env.configs so you first build legacyConfigs, check whether any config.id in legacyConfigs is not present in existingIds, and only if at least one new id exists create mergedConfigs, mutate mcpServers.builtinKnowledge.env (delete configs) and call this.mcpStore.set('mcpServers', mcpServers); otherwise skip the store write and simply return existingConfigs (preserve the original array reference). Ensure you reference the existing symbols env.configs, existingConfigs, legacyConfigs, mergedConfigs, existingIds, migratedEnv, mcpServers.builtinKnowledge and this.mcpStore.set when making the change.src/main/presenter/configPresenter/index.ts (1)
2641-2653: Heavy/semantically conflated event broadcast on knowledge-config save.
getMcpServers()is not a cheap getter — it normalizes server entries, runs platform-specific filtering, awaitspresenter.knowledgePresenter.isSupported(), and may even write back to the store while computing the response. Calling it just to broadcastMCP_EVENTS.CONFIG_CHANGEDafter a knowledge-config save:
- Adds an avoidable async dependency on
knowledgePresenterfrom the config-save path.- Re-emits the full mcp servers payload despite no mcp server config actually changing — listeners that rely on this payload (e.g. anything decorating diffs from
MCP_EVENTS.CONFIG_CHANGED) will treat it as an mcp change.- The downstream listener that actually cares (
KnowledgePresenter.syncKnowledgeConfigChanges) does not consume the payload — it re-readsconfigP.getKnowledgeConfigs()on its own.Consider either (a) emitting a dedicated knowledge-config event, or (b) reading the raw mcp store directly without re-running
getMcpServers()'s normalization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/configPresenter/index.ts` around lines 2641 - 2653, The current setKnowledgeConfigs calls getMcpServers() (an expensive, side-effecting method) just to broadcast MCP_EVENTS.CONFIG_CHANGED, which wrongly couples config saves to MCP normalization and re-emits full MCP payloads; change setKnowledgeConfigs (which calls this.knowledgeConfHelper.setKnowledgeConfigs) to avoid calling getMcpServers — either emit a new dedicated event (e.g., MCP_EVENTS.KNOWLEDGE_CONFIG_CHANGED or a lightweight event) via eventBus.send with no or minimal payload, or read the raw MCP store values directly (not via getMcpServers) if listeners need raw data; ensure KnowledgePresenter.syncKnowledgeConfigChanges remains unchanged (it already re-reads configP.getKnowledgeConfigs()), and remove the Promise.all([this.getMcpServers(), this.getMcpEnabled()]) usage to eliminate the async dependency on knowledgePresenter.isSupported().test/main/presenter/mcpPresenter/builtinKnowledgeServer.test.ts (1)
1-118: Solid coverage of the dynamic-tool-listing and call dispatch paths.The
vi.hoistedmocks are set up correctly, and the disabled-config filter + suffix renumbering is well covered (knowledge-2 disabled -> knowledge-3 becomes_2).Two small notes:
- The source lives in
src/main/presenter/mcpPresenter/inMemoryServers/; placing the test attest/main/presenter/mcpPresenter/inMemoryServers/builtinKnowledgeServer.test.tswould mirror the source tree exactly. As per coding guidelines, "Vitest test suites should mirror the source structure undertest/main/**andtest/renderer/**".- Consider replacing the bare
Functiontype at lines 6 and 47/49 with a more precise signature (e.g.(input?: unknown) => Promise<unknown>) —Functionis normally flagged by@typescript-eslint/ban-types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/mcpPresenter/builtinKnowledgeServer.test.ts` around lines 1 - 118, The tests are in the wrong location and use the banned `Function` type: move the test file so its test tree mirrors the source tree under the same presenter/inMemoryServers structure (so it lives alongside the source it tests) and replace the use of bare Function for serverInstances and the mocked setRequestHandler signature with a precise function type such as (input?: unknown) => Promise<unknown> (or a synchronous variant if appropriate) to satisfy `@typescript-eslint/ban-types` and make the handler contract explicit; update references to serverInstances, mockGetKnowledgeConfigs, mockSimilarityQuery and the setRequestHandler mock implementation accordingly.src/main/presenter/mcpPresenter/inMemoryServers/builtinKnowledgeServer.ts (1)
38-67: Notools/list_changednotification is emitted when the tool list changes at runtime.Since the tool list is derived from the live
getEnabledConfigs()snapshot on everyListToolsRequest, MCP clients that cache the initial tool list (most do) won't observe enabled/disabled config flips until they re-list. Consider adding MCPnotifications/tools/list_changedemission when underlying knowledge configs change to keep active clients in sync with runtime state.This can be addressed in a follow-up as part of the broader knowledge-config event flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/mcpPresenter/inMemoryServers/builtinKnowledgeServer.ts` around lines 38 - 67, The server never notifies clients when the runtime tool list changes; update builtinKnowledgeServer to emit the MCP notification so clients can refresh: when the underlying enabled configs change (the source used by getEnabledConfigs()), call the MCP notification "notifications/tools/list_changed" with the new tools payload (same shape produced by the ListToolsRequestSchema handler) via the server notifier used by this class; hook this emission into the place where configs are mutated or watched (or add a watcher that compares snapshots from getEnabledConfigs()) so any change triggers a notification—ensure tool naming/annotation logic matches the ListToolsRequestSchema handler and reuse the same transformation used in the setRequestHandler for ListToolsRequestSchema and keep behavior for CallToolRequestSchema/performBuiltinKnowledgeSearch unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/knowledgePresenter/index.ts`:
- Around line 163-184: The delete method currently awaits
this.storePresenterInitTasks.get(id) which can throw if an in-flight
createStorePresenter rejected, aborting disk cleanup; wrap the await in a
try/catch (or use .catch()) so initialization rejections are swallowed and
treated as undefined, then proceed to check this.getStorePresenter(id) and the
on-disk removal logic; still call rag.destroy() if a valid rag exists and ensure
this.storePresenterCache.delete(id) runs in the same finally branch, and leave
the fs.rmSync calls for storageDir/id and storageDir/id + '.wal' untouched so
broken DB files are removed even after a failed init.
In `@src/shared/contracts/domainSchemas.ts`:
- Line 344: The env schema now allows non-string values but the stdio spawn and
in-memory servers don't coerce them, causing type mismatches; update the code
that copies serverConfig.env (e.g., the loop in mcpClient.ts that assigns to env
before constructing StdioClientTransport) to coerce each value with String(value
?? '') and skip undefined, and similarly update any in-memory server usages
(e.g., BochaSearchServer where env.apiKey is cast) to validate/coerce values via
String(...) before use so all places pass Record<string,string> to child
processes and handlers.
In `@test/main/lib/agentRuntime/rtkRuntimeService.test.ts`:
- Around line 263-267: The test currently asserts three explicit runCommand
invocations and is tied to the redundant system-candidate probe produced by
verifyRuntimeCandidate; update the assertion in rtkRuntimeService.test.ts so it
no longer requires the duplicated ['rtk', ['--version']] entry: either expect
only the two unique probes (for the explicit candidate and system when distinct)
or make the expectation order/duplication-agnostic (e.g., assert that the calls
include the expected unique command/arg pairs or check the call count for system
when source === 'system'); locate the assertion that maps runCommand.mock.calls
and change it to validate uniqueness or use arrayContaining/Set-style checks so
the test will still pass if you refactor verifyRuntimeCandidate to skip the
redundant probe.
---
Outside diff comments:
In `@src/main/presenter/knowledgePresenter/index.ts`:
- Around line 140-157: The update method can throw when awaiting a
previously-rejected init promise from storePresenterInitTasks which poisons the
whole reconciliation; change the await of
this.storePresenterInitTasks.get(config.id) inside update to be guarded so a
rejected init does not rethrow (for example, retrieve the promise, await it with
a try/catch or use .catch(() => undefined) and proceed only if it resolved to an
object), keep the existing behavior of calling
initializingRag.updateConfig(config) when the init succeeded, and leave
closeStorePresenterIfExists/config-disabled path unchanged; refer to update,
storePresenterInitTasks, getStorePresenter and createStorePresenter to locate
the logic to modify.
---
Nitpick comments:
In `@src/main/lib/agentRuntime/rtkRuntimeService.ts`:
- Around line 600-611: The second redundant probe calling
this.runCommandImpl('rtk', ['--version'], ...) should be skipped when the
runtime candidate is the system one; detect candidate.source === 'system'
(and/or candidate.command === 'rtk') and short-circuit before calling
runCommandImpl a second time so you only perform the PATH-resolvability check
for bundled candidates (where prependBundledRuntimeToEnv was applied). Ensure
the existing RtkHealthCheckError logic remains for failures of the bundled check
and do not change error shape—only avoid invoking runCommandImpl for system
candidates.
In `@src/main/presenter/configPresenter/index.ts`:
- Around line 2641-2653: The current setKnowledgeConfigs calls getMcpServers()
(an expensive, side-effecting method) just to broadcast
MCP_EVENTS.CONFIG_CHANGED, which wrongly couples config saves to MCP
normalization and re-emits full MCP payloads; change setKnowledgeConfigs (which
calls this.knowledgeConfHelper.setKnowledgeConfigs) to avoid calling
getMcpServers — either emit a new dedicated event (e.g.,
MCP_EVENTS.KNOWLEDGE_CONFIG_CHANGED or a lightweight event) via eventBus.send
with no or minimal payload, or read the raw MCP store values directly (not via
getMcpServers) if listeners need raw data; ensure
KnowledgePresenter.syncKnowledgeConfigChanges remains unchanged (it already
re-reads configP.getKnowledgeConfigs()), and remove the
Promise.all([this.getMcpServers(), this.getMcpEnabled()]) usage to eliminate the
async dependency on knowledgePresenter.isSupported().
In `@src/main/presenter/configPresenter/mcpConfHelper.ts`:
- Around line 461-491: The code always creates mergedConfigs =
[...existingConfigs] and writes mcpServers to the store even when no legacy
configs are added; change the logic in the block handling env.configs so you
first build legacyConfigs, check whether any config.id in legacyConfigs is not
present in existingIds, and only if at least one new id exists create
mergedConfigs, mutate mcpServers.builtinKnowledge.env (delete configs) and call
this.mcpStore.set('mcpServers', mcpServers); otherwise skip the store write and
simply return existingConfigs (preserve the original array reference). Ensure
you reference the existing symbols env.configs, existingConfigs, legacyConfigs,
mergedConfigs, existingIds, migratedEnv, mcpServers.builtinKnowledge and
this.mcpStore.set when making the change.
In `@src/main/presenter/knowledgePresenter/index.ts`:
- Around line 80-85: The current listener on eventBus for
MCP_EVENTS.CONFIG_CHANGED calls syncKnowledgeConfigChanges() for every MCP
config change, causing unnecessary work; update the code to only react to
knowledge-config saves by either (a) subscribing to a new dedicated event
emitted by configPresenter.setKnowledgeConfigs (e.g.,
MCP_EVENTS.KNOWLEDGE_CONFIG_CHANGED) or (b) inspecting the CONFIG_CHANGED event
payload inside the eventBus.on callback and only invoking
this.syncKnowledgeConfigChanges() when the change pertains to knowledge configs;
locate the handler using eventBus.on(MCP_EVENTS.CONFIG_CHANGED, ...) and the
method syncKnowledgeConfigChanges to implement the conditional or switch to the
new event name.
In `@src/main/presenter/mcpPresenter/inMemoryServers/builtinKnowledgeServer.ts`:
- Around line 38-67: The server never notifies clients when the runtime tool
list changes; update builtinKnowledgeServer to emit the MCP notification so
clients can refresh: when the underlying enabled configs change (the source used
by getEnabledConfigs()), call the MCP notification
"notifications/tools/list_changed" with the new tools payload (same shape
produced by the ListToolsRequestSchema handler) via the server notifier used by
this class; hook this emission into the place where configs are mutated or
watched (or add a watcher that compares snapshots from getEnabledConfigs()) so
any change triggers a notification—ensure tool naming/annotation logic matches
the ListToolsRequestSchema handler and reuse the same transformation used in the
setRequestHandler for ListToolsRequestSchema and keep behavior for
CallToolRequestSchema/performBuiltinKnowledgeSearch unchanged.
In `@src/renderer/settings/components/BuiltinKnowledgeSettings.vue`:
- Around line 936-942: loadBuiltinConfig currently swallows errors with
console.error, so surface failures to the user and prevent accidental
overwrites: when catching errors from configClient.getKnowledgeConfigs() (in
loadBuiltinConfig), call the app's user-facing notification/toast API to show
the failure (include the error message) and set a boolean flag like
builtinConfigsLoaded/hasLoadedBuiltinConfigs to false; on successful load set it
to true and populate builtinConfigs.value; update the save path (e.g., the save
handler or saveBuiltinConfig and any save button disabled state) to guard
against saving when builtinConfigsLoaded is false so saves are disabled or
rejected until a successful load completes.
In `@src/shared/contracts/domainSchemas.ts`:
- Around line 21-33: BuiltinKnowledgeConfigSchema uses bare z.number() which
accepts NaN and negatives (Zod v4 z.number() does accept NaN by default), so
update the schema to validate numeric fields: change dimensions and
fragmentsNumber to z.number().int().positive(), change chunkSize and
chunkOverlap to z.number().finite().nonnegative().optional() (or
.int().nonnegative() if integers are expected), and keep any optional chaining
(.optional()) as needed so invalid NaN/negative values are rejected at the
contract boundary; apply these changes on the BuiltinKnowledgeConfigSchema
definition to prevent NaN/null propagation.
In `@test/main/lib/agentRuntime/rtkRuntimeService.test.ts`:
- Around line 69-118: The helper duplication can be removed by parameterizing
the existing createService factory so tests reuse it instead of a separate
createHealthCheckService: update createService to accept an optional overrides
object (e.g., replaceWithRuntimeCommand, getShellEnvironment, runCommand,
getPath) and move the unique replaceWithRuntimeCommand mock currently in
createHealthCheckService into that overrides parameter; then replace
createHealthCheckService usages with calls to createService({
replaceWithRuntimeCommand: vi.fn((command)=> command === 'rtk' ?
'/runtime/rtk/rtk.exe' : command), runCommand }) so tests keep behavior while
eliminating the duplicate function. Ensure symbols referenced are createService,
createHealthCheckService, and replaceWithRuntimeCommand when making the change.
In `@test/main/presenter/mcpPresenter/builtinKnowledgeServer.test.ts`:
- Around line 1-118: The tests are in the wrong location and use the banned
`Function` type: move the test file so its test tree mirrors the source tree
under the same presenter/inMemoryServers structure (so it lives alongside the
source it tests) and replace the use of bare Function for serverInstances and
the mocked setRequestHandler signature with a precise function type such as
(input?: unknown) => Promise<unknown> (or a synchronous variant if appropriate)
to satisfy `@typescript-eslint/ban-types` and make the handler contract explicit;
update references to serverInstances, mockGetKnowledgeConfigs,
mockSimilarityQuery and the setRequestHandler mock implementation accordingly.
🪄 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
Run ID: 6690c786-f1d5-404f-b05d-862b1e9661fe
📒 Files selected for processing (24)
src/main/lib/agentRuntime/rtkRuntimeService.tssrc/main/presenter/configPresenter/index.tssrc/main/presenter/configPresenter/mcpConfHelper.tssrc/main/presenter/knowledgePresenter/index.tssrc/main/presenter/mcpPresenter/inMemoryServers/builder.tssrc/main/presenter/mcpPresenter/inMemoryServers/builtinKnowledgeServer.tssrc/main/routes/config/configRouteHandler.tssrc/main/routes/models/modelRouteHandler.tssrc/renderer/api/ConfigClient.tssrc/renderer/settings/components/BuiltinKnowledgeSettings.vuesrc/shared/contracts/domainSchemas.tssrc/shared/contracts/routes.tssrc/shared/contracts/routes/config.routes.tstest/main/lib/agentRuntime/rtkRuntimeService.test.tstest/main/presenter/KnowledgePresenter.test.tstest/main/presenter/configPresenter/mcpConfHelper.test.tstest/main/presenter/configPresenter/providerModelHelper.test.tstest/main/presenter/mcpPresenter/builtinKnowledgeServer.test.tstest/main/routes/dispatcher.test.tstest/main/routes/models/modelRouteHandler.test.tstest/renderer/api/clients.test.tstest/renderer/components/BuiltinKnowledgeSettings.test.tstest/renderer/stores/modelStore.test.tstest/setup.renderer.ts
| args: z.array(z.string()).optional(), | ||
| name: z.string().optional(), | ||
| env: z.record(z.string(), z.string()).optional() | ||
| env: z.record(z.string(), z.unknown()).optional() |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all reads of MCPServerConfig.env or .env[key] to confirm each handles unknown values.
rg -nP --type=ts -C3 '\b(server|config|cfg|mcpServer)\.env\b'
rg -nP --type=ts -C3 'MCPServerConfig\b'Repository: ThinkInAIXYZ/deepchat
Length of output: 42122
🏁 Script executed:
# Check the normalizeStringRecordToArray implementation
sed -n '1,27p' src/main/presenter/llmProviderPresenter/acp/mcpConfigConverter.ts
# Check agentRepository usage of env
sed -n '470,480p' src/main/presenter/agentRepository/index.ts
# Search for child process spawning with env
rg -n "spawn|exec|execFile" --type=ts src/main/presenter/mcpPresenter/ -A 5 -B 2Repository: ThinkInAIXYZ/deepchat
Length of output: 10049
🏁 Script executed:
# Look for serverManager implementation that spawns MCP processes
rg -n "serverManager\|spawn.*env" --type=ts src/main/presenter/mcpPresenter/ -l | head -5Repository: ThinkInAIXYZ/deepchat
Length of output: 47
🏁 Script executed:
# Search more broadly for env variable usage in process spawning
rg -n "env\s*:" --type=ts src/main/presenter/ | grep -i "spawn\|exec\|process" | head -10Repository: ThinkInAIXYZ/deepchat
Length of output: 1020
🏁 Script executed:
# Find serverManager implementation
find src/main/presenter/mcpPresenter -name "*manager*" -o -name "*server*" | grep -v node_modules | head -10Repository: ThinkInAIXYZ/deepchat
Length of output: 114
🏁 Script executed:
# Look for actual child process spawning with MCP servers
rg -n "spawn\|ChildProcess" --type=ts src/main/presenter/mcpPresenter/ -B 2 -A 5Repository: ThinkInAIXYZ/deepchat
Length of output: 47
🏁 Script executed:
# Check agentRepository context - what happens after env is retrieved
sed -n '470,490p' src/main/presenter/agentRepository/index.tsRepository: ThinkInAIXYZ/deepchat
Length of output: 540
🏁 Script executed:
# Search for stdio server startup code that uses config.env
rg -n "stdio\|StdioClientTransport" --type=ts src/main/presenter/ -B 3 -A 3 | head -50Repository: ThinkInAIXYZ/deepchat
Length of output: 47
🏁 Script executed:
# Examine serverManager.ts - the critical file
cat src/main/presenter/mcpPresenter/serverManager.ts | head -200Repository: ThinkInAIXYZ/deepchat
Length of output: 6776
🏁 Script executed:
# Continue reading serverManager.ts
wc -l src/main/presenter/mcpPresenter/serverManager.tsRepository: ThinkInAIXYZ/deepchat
Length of output: 118
🏁 Script executed:
# Get the startServer method and process spawning code
sed -n '200,308p' src/main/presenter/mcpPresenter/serverManager.tsRepository: ThinkInAIXYZ/deepchat
Length of output: 3282
🏁 Script executed:
# Find McpClient and check its connect method
find src/main/presenter/mcpPresenter -name "*mcpClient*" -o -name "*client*" | grep -v node_modulesRepository: ThinkInAIXYZ/deepchat
Length of output: 110
🏁 Script executed:
# Read McpClient to find spawn calls
cat src/main/presenter/mcpPresenter/mcpClient.ts | head -150Repository: ThinkInAIXYZ/deepchat
Length of output: 4910
🏁 Script executed:
# Find where StdioClientTransport is created and env is used
rg -n "StdioClientTransport\|spawn\|processCommand" src/main/presenter/mcpPresenter/mcpClient.ts -A 10 -B 2Repository: ThinkInAIXYZ/deepchat
Length of output: 47
🏁 Script executed:
# Search more broadly in mcpClient.ts
wc -l src/main/presenter/mcpPresenter/mcpClient.tsRepository: ThinkInAIXYZ/deepchat
Length of output: 115
🏁 Script executed:
# Get the middle section of mcpClient.ts where connect is likely defined
sed -n '150,350p' src/main/presenter/mcpPresenter/mcpClient.tsRepository: ThinkInAIXYZ/deepchat
Length of output: 6996
🏁 Script executed:
# Continue reading to see where serverConfig.env is used for stdio
sed -n '300,450p' src/main/presenter/mcpPresenter/mcpClient.tsRepository: ThinkInAIXYZ/deepchat
Length of output: 5106
🏁 Script executed:
# Check the inmemory server env handling
sed -n '190,210p' src/main/presenter/mcpPresenter/mcpClient.tsRepository: ThinkInAIXYZ/deepchat
Length of output: 1026
🏁 Script executed:
# Check what getInMemoryServer does with env
rg -n "getInMemoryServer" src/main/presenter/mcpPresenter/ -A 10Repository: ThinkInAIXYZ/deepchat
Length of output: 2974
🏁 Script executed:
# Check how in-memory servers use env
rg -n "env" src/main/presenter/mcpPresenter/inMemoryServers/builder.ts -B 2 -A 5Repository: ThinkInAIXYZ/deepchat
Length of output: 1700
🏁 Script executed:
# Check if BochaSearchServer or other servers coerce env values
head -100 src/main/presenter/mcpPresenter/inMemoryServers/bochaSearchServer.tsRepository: ThinkInAIXYZ/deepchat
Length of output: 2805
🏁 Script executed:
# Check BochaSearchServer constructor more to see how it handles apiKey
sed -n '75,150p' src/main/presenter/mcpPresenter/inMemoryServers/bochaSearchServer.tsRepository: ThinkInAIXYZ/deepchat
Length of output: 2139
🏁 Script executed:
# Check if there are runtime coercions when env values are actually used
rg -n "apiKey\|env\[" src/main/presenter/mcpPresenter/inMemoryServers/bochaSearchServer.ts | head -20Repository: ThinkInAIXYZ/deepchat
Length of output: 47
🏁 Script executed:
# Verify how StdioClientTransport from MCP SDK handles env types
# First check if there's type validation in the codebase
rg -n "StdioClientTransport" src/ --type=ts -B 3 -A 10 | head -50Repository: ThinkInAIXYZ/deepchat
Length of output: 3228
Coerce env values to strings in stdio server spawning and in-memory servers.
The schema change to Record<string, unknown> creates a type safety gap. While the ACP converter correctly coerces via String(value ?? ''), the stdio server path in mcpClient.ts (lines 354–365) and in-memory servers do not:
- Stdio servers (line 354–365): Directly assigns
serverConfig.envvalues to the env dict without coercion, then passes toStdioClientTransportwhich requiresRecord<string, string>for child process spawning. - In-memory servers (e.g.,
BochaSearchServerline 102): Unsafe castenv.apiKey as stringwithout validation or coercion.
When non-string values (e.g., numbers, booleans) are stored in the config, they will be passed to child processes or used directly in handlers without conversion to strings, causing silent type mismatches.
Example unsafe path (mcpClient.ts lines 354–365)
Object.entries(this.serverConfig.env as Record<string, string>).forEach(
([key, value]) => {
if (value !== undefined) {
env[key] = value // No coercion—direct assignment of potentially non-string value
}
}
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/contracts/domainSchemas.ts` at line 344, The env schema now allows
non-string values but the stdio spawn and in-memory servers don't coerce them,
causing type mismatches; update the code that copies serverConfig.env (e.g., the
loop in mcpClient.ts that assigns to env before constructing
StdioClientTransport) to coerce each value with String(value ?? '') and skip
undefined, and similarly update any in-memory server usages (e.g.,
BochaSearchServer where env.apiKey is cast) to validate/coerce values via
String(...) before use so all places pass Record<string,string> to child
processes and handlers.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
src/main/presenter/mcpPresenter/mcpClient.ts (1)
356-374: Consider skipping empty PATH values to avoid a leading empty entry.If a user-provided
env.PATHisnull(or anything that coerces to''), the current logic still merges it:env[currentPathKey] = '' + ':' + env[currentPathKey]. On *nix, a leading empty entry inPATHis interpreted as the current working directory, which is generally undesirable (and historically a PATH-injection footgun). Non-PATH variables setting an empty string is fine.♻️ Proposed guard
if (value !== undefined) { const stringValue = String(value ?? '') // 如果是PATH相关变量，合并到主PATH中 if (['PATH', 'Path', 'path'].includes(key)) { + if (!stringValue) { + return + } const currentPathKey = process.platform === 'win32' ? 'Path' : 'PATH' const separator = process.platform === 'win32' ? ';' : ':' env[currentPathKey] = env[currentPathKey] ? `${stringValue}${separator}${env[currentPathKey]}` : stringValue } else { env[key] = stringValue } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/mcpPresenter/mcpClient.ts` around lines 356 - 374, The merging logic for PATH-like keys in mcpClient.ts can introduce a leading empty PATH entry when serverConfig.env contains null/empty values; update the block that handles ['PATH','Path','path'] inside the Object.entries loop (where it computes currentPathKey, separator and sets env[currentPathKey]) to first skip merging when stringValue is empty (e.g., stringValue === ''), so that empty PATH values are ignored, while keeping the existing behavior for non-PATH keys (env[key] = stringValue) and preserving platform-specific currentPathKey/separator logic.src/main/presenter/mcpPresenter/inMemoryServers/difyKnowledgeServer.ts (1)
73-98: Disabled configs still fail hard validation, blocking all others.The validation in Lines 81–89 throws unconditionally on the first config with any missing required field, even if that config is
enabled: false. With the migration from legacy storage shapes introduced by this PR, a stale/disabled entry with an emptyapiKeywill now prevent the entire Dify server from starting and silently break every other (enabled) knowledge base in the same server.Consider either skipping disabled entries, or collecting errors and continuing for valid+enabled configs.
♻️ Sketch — skip invalid/disabled instead of throwing
for (const env of envs) { const config = env && typeof env === 'object' ? (env as Record<string, unknown>) : {} const apiKey = String(config.apiKey ?? '') const datasetId = String(config.datasetId ?? '') const description = String(config.description ?? '') const endpoint = String(config.endpoint ?? '') || 'https://api.dify.ai/v1' + const enabled = + config.enabled === true || String(config.enabled ?? '').toLowerCase() === 'true' - if (!apiKey) { - throw new Error('需要提供Dify API Key') - } - if (!datasetId) { - throw new Error('需要提供Dify Dataset ID') - } - if (!description) { - throw new Error('需要提供对这个知识库的描述，以方便ai决定是否检索此知识库') - } + if (!apiKey || !datasetId || !description) { + if (enabled) { + console.warn('DifyKnowledgeServer: skipping enabled config with missing fields', { + hasApiKey: !!apiKey, + hasDatasetId: !!datasetId, + hasDescription: !!description + }) + } + continue + } - this.configs.push({ - apiKey, - datasetId, - endpoint, - description, - enabled: config.enabled === true || String(config.enabled ?? '').toLowerCase() === 'true' - }) + this.configs.push({ apiKey, datasetId, endpoint, description, enabled }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/mcpPresenter/inMemoryServers/difyKnowledgeServer.ts` around lines 73 - 98, The loop in difyKnowledgeServer.ts currently throws on missing apiKey/datasetId/description for every env even when config.enabled is false; change the validation flow in the for (const env of envs) loop to determine the enabled flag first (use the same enabled expression used in this.configs.push) and if enabled is false, skip strict validation and either continue (preferable) or collect and log the error instead of throwing; only perform and throw on missing required fields for entries where enabled === true, and still push or register disabled entries as needed via this.configs.push with the computed enabled value.src/main/presenter/mcpPresenter/inMemoryServers/ragflowKnowledgeServer.ts (1)
70-96: Same disabled-config validation issue + cross-file duplication.Two points on this loop:
Same as in
difyKnowledgeServer.tsandfastGptKnowledgeServer.ts: a disabled config with emptyapiKey/datasetIds/descriptionwill throw at Lines 79–87 and prevent any other (valid, enabled) RAGFlow knowledge base from initializing. Consider skipping invalid entries (warn-and-continue), at least forenabled === false.The coercion + normalization block is now near-identical across all three knowledge servers (
dify,fastgpt,ragflow). Worth factoring out a small helper, e.g. in a shared module underinMemoryServers/:♻️ Sketch — shared helpers
// e.g. src/main/presenter/mcpPresenter/inMemoryServers/knowledgeConfigUtils.ts export const toStr = (v: unknown): string => String(v ?? '') export const toBool = (v: unknown): boolean => v === true || toStr(v).toLowerCase() === 'true' export const toRecord = (v: unknown): Record<string, unknown> => v && typeof v === 'object' ? (v as Record<string, unknown>) : {} export const toStringArray = (v: unknown): string[] => Array.isArray(v) ? v.map((x) => toStr(x)).filter(Boolean) : []Each server then just consumes these, keeping the per-vendor validation/defaulting logic local.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/mcpPresenter/inMemoryServers/ragflowKnowledgeServer.ts` around lines 70 - 96, The loop in ragflowKnowledgeServer.ts currently throws for empty apiKey/datasetIds/description which will abort initialization if any disabled entry is malformed; update the logic to coerce/normalize via shared helpers (e.g. extract to src/main/presenter/mcpPresenter/inMemoryServers/knowledgeConfigUtils.ts providing toStr, toBool, toRecord, toStringArray) and in the loop use toRecord(env) -> config, apiKey = toStr(...), datasetIds = toStringArray(...), description = toStr(...), enabled = toBool(config.enabled); then skip invalid entries by continuing (and optionally log/warn) when enabled === false and required fields are missing, while still throwing or failing only for enabled entries with missing required fields; also refactor similar blocks in difyKnowledgeServer.ts and fastGptKnowledgeServer.ts to consume the same helpers.src/main/presenter/knowledgePresenter/index.ts (1)
353-356:closeAllmay leave maps populated if anyrag.close()rejects.
Promise.allrejects on the first failing close, so Lines 354–355 don't run and the cache/init-task maps retain stale references. On a subsequent call (or app shutdown timing), those references can be re-used. Switch toPromise.allSettledand clear maps infinallyso cleanup is best-effort.♻️ Proposed refactor
- await Promise.all(Array.from(stores).map((rag) => rag.close())) - this.storePresenterCache.clear() - this.storePresenterInitTasks.clear() + try { + await Promise.allSettled(Array.from(stores).map((rag) => rag.close())) + } finally { + this.storePresenterCache.clear() + this.storePresenterInitTasks.clear() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/knowledgePresenter/index.ts` around lines 353 - 356, The closeAll method currently uses Promise.all on Array.from(stores).map(rag => rag.close()), which will abort on the first rejection and prevent the subsequent clearing of this.storePresenterCache and this.storePresenterInitTasks; change the logic to use Promise.allSettled for awaiting all rag.close() operations and move the two map clear calls into a finally block so they always run (reference the closeAll function, each rag.close() invocation, and the this.storePresenterCache and this.storePresenterInitTasks maps).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/knowledgePresenter/index.ts`:
- Around line 207-220: The try/catch around creating KnowledgeStorePresenter
leaks the opened DuckDBPresenter (db) if construction throws and also discards
the original error; update the initTask closure to catch exceptions from new
KnowledgeStorePresenter(db, config, this.taskP), call the appropriate DB cleanup
(e.g., db.close() or db.dispose()) before rethrowing, and rethrow using the
Error constructor with the cause option (new Error('Failed to create
storePresenter', { cause: e })) or rethrow the original error to preserve stack;
reference getVectorDatabasePresenter, KnowledgeStorePresenter, db, initTask and
storePresenterCache when making the change so you clean up the db on failure and
only cache rag after successful construction.
- Around line 148-152: The update() method can abort if a prior concurrent init
rejected because awaiting this.storePresenterInitTasks.get(config.id) will
throw; change the await to swallow a rejected init the same way delete() does
(i.e., await this.storePresenterInitTasks.get(config.id).catch(() => undefined))
so initializingRag is undefined on failure and update() proceeds to apply
enable/disable or new config; update the reference in the update() function
where initializingRag is assigned and mirror the same pattern used in delete().
- Around line 269-279: closeStorePresenterIfExists currently awaits
this.storePresenterInitTasks.get(id) directly so a rejection from the in-flight
createStorePresenter can bubble up and skip cache cleanup; change it to mirror
the defensive pattern used in delete(): retrieve the init promise without
letting its rejection escape (e.g., const initPromise =
this.storePresenterInitTasks.get(id); let initializingRag; try { initializingRag
= await initPromise } catch { initializingRag = undefined } then use const rag =
this.getStorePresenter(id) ?? initializingRag, call await rag.close() if rag
exists, and ensure this.storePresenterCache.delete(id) runs in a finally block
so the cache is always cleaned up even on init failures.
- Around line 163-184: The delete method can leave a stale promise in
storePresenterInitTasks causing races; after resolving initializingRag (from
this.storePresenterInitTasks.get(id)) and before/after destroying the rag,
remove the init-task entry from this.storePresenterInitTasks (e.g., call
this.storePresenterInitTasks.delete(id)) so concurrent getOrCreateStorePresenter
/ createStorePresenter cannot pick up a resolved-but-destroyed presenter; also
ensure you still delete the storePresenterCache entry as currently done and
perform the init-task deletion inside the same try/finally so it always runs.
In `@src/main/presenter/mcpPresenter/inMemoryServers/difyKnowledgeServer.ts`:
- Line 62: The constructor for DifyKnowledgeServer currently logs the entire env
object (console.log('DifyKnowledgeServer constructor', env)), which exposes
sensitive apiKey values; update the DifyKnowledgeServer constructor to stop
printing the raw env - either remove the console.log entirely or log a redacted
summary (e.g., only keys or mask apiKey values) and apply the same change to the
analogous log in fastGptKnowledgeServer (the constructor/log at Line 51) so no
full config or apiKey is emitted to logs.
In `@src/main/presenter/mcpPresenter/inMemoryServers/fastGptKnowledgeServer.ts`:
- Line 51: Remove the unsafe console.log in the FastGptKnowledgeServer
constructor that prints the entire env (including env.configs with apiKey
values); either drop the log entirely or replace it with a safe debug message
that omits or masks sensitive fields (e.g., iterate env.configs and replace
apiKey values with a fixed redaction token) and/or log only non-sensitive keys.
Ensure the change targets the FastGptKnowledgeServer constructor and the
env.configs structure so no API keys are written to logs.
---
Nitpick comments:
In `@src/main/presenter/knowledgePresenter/index.ts`:
- Around line 353-356: The closeAll method currently uses Promise.all on
Array.from(stores).map(rag => rag.close()), which will abort on the first
rejection and prevent the subsequent clearing of this.storePresenterCache and
this.storePresenterInitTasks; change the logic to use Promise.allSettled for
awaiting all rag.close() operations and move the two map clear calls into a
finally block so they always run (reference the closeAll function, each
rag.close() invocation, and the this.storePresenterCache and
this.storePresenterInitTasks maps).
In `@src/main/presenter/mcpPresenter/inMemoryServers/difyKnowledgeServer.ts`:
- Around line 73-98: The loop in difyKnowledgeServer.ts currently throws on
missing apiKey/datasetId/description for every env even when config.enabled is
false; change the validation flow in the for (const env of envs) loop to
determine the enabled flag first (use the same enabled expression used in
this.configs.push) and if enabled is false, skip strict validation and either
continue (preferable) or collect and log the error instead of throwing; only
perform and throw on missing required fields for entries where enabled === true,
and still push or register disabled entries as needed via this.configs.push with
the computed enabled value.
In `@src/main/presenter/mcpPresenter/inMemoryServers/ragflowKnowledgeServer.ts`:
- Around line 70-96: The loop in ragflowKnowledgeServer.ts currently throws for
empty apiKey/datasetIds/description which will abort initialization if any
disabled entry is malformed; update the logic to coerce/normalize via shared
helpers (e.g. extract to
src/main/presenter/mcpPresenter/inMemoryServers/knowledgeConfigUtils.ts
providing toStr, toBool, toRecord, toStringArray) and in the loop use
toRecord(env) -> config, apiKey = toStr(...), datasetIds = toStringArray(...),
description = toStr(...), enabled = toBool(config.enabled); then skip invalid
entries by continuing (and optionally log/warn) when enabled === false and
required fields are missing, while still throwing or failing only for enabled
entries with missing required fields; also refactor similar blocks in
difyKnowledgeServer.ts and fastGptKnowledgeServer.ts to consume the same
helpers.
In `@src/main/presenter/mcpPresenter/mcpClient.ts`:
- Around line 356-374: The merging logic for PATH-like keys in mcpClient.ts can
introduce a leading empty PATH entry when serverConfig.env contains null/empty
values; update the block that handles ['PATH','Path','path'] inside the
Object.entries loop (where it computes currentPathKey, separator and sets
env[currentPathKey]) to first skip merging when stringValue is empty (e.g.,
stringValue === ''), so that empty PATH values are ignored, while keeping the
existing behavior for non-PATH keys (env[key] = stringValue) and preserving
platform-specific currentPathKey/separator logic.
🪄 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
Run ID: c6d78016-e79b-4b05-8056-7c449218a1e5
📒 Files selected for processing (12)
src/main/presenter/knowledgePresenter/index.tssrc/main/presenter/mcpPresenter/inMemoryServers/bochaSearchServer.tssrc/main/presenter/mcpPresenter/inMemoryServers/braveSearchServer.tssrc/main/presenter/mcpPresenter/inMemoryServers/builder.tssrc/main/presenter/mcpPresenter/inMemoryServers/deepResearchServer.tssrc/main/presenter/mcpPresenter/inMemoryServers/difyKnowledgeServer.tssrc/main/presenter/mcpPresenter/inMemoryServers/fastGptKnowledgeServer.tssrc/main/presenter/mcpPresenter/inMemoryServers/ragflowKnowledgeServer.tssrc/main/presenter/mcpPresenter/mcpClient.tstest/main/lib/agentRuntime/rtkRuntimeService.test.tstest/main/presenter/KnowledgePresenter.test.tstest/main/presenter/mcpClient.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/main/presenter/mcpPresenter/inMemoryServers/braveSearchServer.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/presenter/mcpPresenter/inMemoryServers/builder.ts
- test/main/presenter/KnowledgePresenter.test.ts
- test/main/lib/agentRuntime/rtkRuntimeService.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/presenter/mcpPresenter/inMemoryServers/fastGptKnowledgeServer.ts (1)
50-86: Consider extracting the shared config-normalization logic.The constructor bodies of
DifyKnowledgeServer,FastGptKnowledgeServer, and (per the PR summary)RagflowKnowledgeServernow share an identical pattern: validateenv, iterateenv.configs, coerceapiKey/datasetId/descriptionto strings, defaultendpoint, and normalizeenabled. A small helper (e.g.normalizeKnowledgeConfigs(env, { defaultEndpoint, errorMessages })) would remove the triplicated logic and make future fixes (e.g. handling additional truthyenabledvalues) a one-liner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/mcpPresenter/inMemoryServers/fastGptKnowledgeServer.ts` around lines 50 - 86, Extract the duplicated config-validation and normalization from the constructors of DifyKnowledgeServer, FastGptKnowledgeServer, and RagflowKnowledgeServer into a single helper (e.g. normalizeKnowledgeConfigs) that accepts the raw env plus options like defaultEndpoint and errorMessages; the helper should validate env and env.configs, iterate configs coercing apiKey, datasetId, description to strings, apply the default endpoint, and normalize enabled (treat true, 'true', '1', 1, and other desired truthy forms as enabled), then return the normalized array; update each class constructor to call normalizeKnowledgeConfigs(...) and assign the returned array to this.configs, and remove the in-constructor duplication.src/main/presenter/mcpPresenter/inMemoryServers/difyKnowledgeServer.ts (1)
61-97: Good hardening; consider renaming the inner loop variable.Removing the
console.log(env)and coercing untrusted fields to strings addresses the prior secret-leak concern and is consistent with the new untyped-envcontract frombuilder.ts. One small readability nit: the loop variableenvon Line 73 shadows the outerenvparameter, making the inner block slightly harder to reason about.♻️ Optional readability tweak
- // 处理每个配置 - for (const env of envs) { - const config = env && typeof env === 'object' ? (env as Record<string, unknown>) : {} + // 处理每个配置 + for (const rawConfig of envs) { + const config = + rawConfig && typeof rawConfig === 'object' ? (rawConfig as Record<string, unknown>) : {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/mcpPresenter/inMemoryServers/difyKnowledgeServer.ts` around lines 61 - 97, The constructor currently shadows the outer parameter env by reusing the name env in the for loop; rename the loop variable (e.g., to cfg, envItem, or configRaw) to avoid shadowing and improve readability, then update all references inside the loop (the config assignment, apiKey/datasetId/description/endpoint extraction, and the this.configs.push call) to use the new loop variable name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/presenter/mcpPresenter/inMemoryServers/difyKnowledgeServer.ts`:
- Around line 61-97: The constructor currently shadows the outer parameter env
by reusing the name env in the for loop; rename the loop variable (e.g., to cfg,
envItem, or configRaw) to avoid shadowing and improve readability, then update
all references inside the loop (the config assignment,
apiKey/datasetId/description/endpoint extraction, and the this.configs.push
call) to use the new loop variable name.
In `@src/main/presenter/mcpPresenter/inMemoryServers/fastGptKnowledgeServer.ts`:
- Around line 50-86: Extract the duplicated config-validation and normalization
from the constructors of DifyKnowledgeServer, FastGptKnowledgeServer, and
RagflowKnowledgeServer into a single helper (e.g. normalizeKnowledgeConfigs)
that accepts the raw env plus options like defaultEndpoint and errorMessages;
the helper should validate env and env.configs, iterate configs coercing apiKey,
datasetId, description to strings, apply the default endpoint, and normalize
enabled (treat true, 'true', '1', 1, and other desired truthy forms as enabled),
then return the normalized array; update each class constructor to call
normalizeKnowledgeConfigs(...) and assign the returned array to this.configs,
and remove the in-constructor duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a35304f5-75c6-4109-b91f-3abacca67cd0
📒 Files selected for processing (4)
src/main/presenter/knowledgePresenter/index.tssrc/main/presenter/mcpPresenter/inMemoryServers/difyKnowledgeServer.tssrc/main/presenter/mcpPresenter/inMemoryServers/fastGptKnowledgeServer.tstest/main/presenter/KnowledgePresenter.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/main/presenter/KnowledgePresenter.test.ts
zerob13
added a commit
that referenced
this pull request
Apr 27, 2026
* fix(rtk): simplify health check * fix(knowledge): use config ipc * fix(models): persist db model status * fix: harden MCP env and knowledge delete * fix: clean knowledge presenter cleanup paths
zerob13
added a commit
that referenced
this pull request
May 1, 2026
* docs: add mac computer use spec * docs: sync provider tables * fix: rtk status (#1541) * fix(rtk): simplify health check * fix(knowledge): use config ipc * fix(models): persist db model status * fix: harden MCP env and knowledge delete * fix: clean knowledge presenter cleanup paths * refactor: enhance transition effects and performance across message components (#1542) - Updated transition classes in MessageActionButtons.vue for smoother animations. - Refactored MessageBlockContent.vue to optimize artifact snapshot handling with computed properties. - Improved transition effects in MessageBlockToolCall.vue for better user experience. - Added a mention icon map in MessageContent.vue to streamline icon retrieval. - Enhanced MessageItemUser.vue with a new line counting function for better text handling. - Optimized MessageToolbar.vue for consistent transition effects on button interactions. - Refactored BrowserPanel.vue to simplify state management for synced bounds. - Improved ChatSidePanel.vue with better resizing and visibility handling. - Updated ChatPage.vue to enhance chat search highlight scheduling. - Cleaned up ChatTabView.vue by removing legacy collapsed new chat button functionality. - Enhanced tests in ChatTabView.test.ts and WindowSideBar.test.ts for improved coverage and accuracy. * chore: update markstream-vue to 0.0.13 (#1544) * fix: preserve interleaved reasoning (#1543) * chore(release): prepare v1.0.4-beta.2 * fix(ipc): allow attachment date metadata (#1547) * chore(release): prepare v1.0.4-beta.3 * feat: add mac computer use helper * feat: enhance computer use guidance * chore: bump acp registry versions * fix: import mac signing identity for helper * build(cua): vendor CUA driver source * fix(cua): prefer element index click mode * fix(cua): route zoom clicks by coordinates * docs(cua): prefer visual fallback for sparse UI * feat: update vendored cua driver * fix(computer-use): improve error handling and testability * docs(cua): add runtime plugin spec * feat(plugin): add CUA runtime plugin * feat: migrate computer use to plugin * fix: surface plugin tools and permissions * fix(plugin): use MCP-only CUA flow * ci(plugin): release CUA dcplugin assets * fix(plugin): hide CUA on unsupported OS * feat(plugin): bundle official CUA plugin * fix: harden plugin startup and CUA telemetry * chore: update CUA driver vendor * fix(build): sign CUA plugin helper * fix: improve CUA window scoped vision fallback * fix(cua): align pixel clicks with upstream --------- Co-authored-by: yyhhyyyyyy <yyhhyyyyyy8@gmail.com> Co-authored-by: xiaomo <wegi866@gmail.com>
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 by CodeRabbit
New Features
Improvements
Tests