fix(cua): upgrade to 0.12.6 with supervised lifecycle - #2052
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds supervised lifecycle management for plugin-owned runtimes, upgrades CUA to embedded driver 0.12.6 operation, introduces static catalog and integrity validation, updates MCP ownership and policies, adds runtime diagnostics UI, refreshes CUA documentation, and updates provider model metadata. ChangesPlugin External Runtime Lifecycle and CUA 0.12.6
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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}
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/mcp/serverManager.ts (1)
250-288: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReuse the adapter’s config override on guarded restarts.
CuaEmbeddedRuntimeAdapter.start()emitscommand/argsviaconfigOverride, butServerManager.startServer()falls back toexistingClient.serverConfigwhen a non-running client still exists. A stale guarded restart can connect the MCP proxy with the persisted config instead of the adapter-supplied endpoint config; stop/evict the existing client before reconnecting with the override. Also mergeenvkeys deeply instead of replacing them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/mcp/serverManager.ts` around lines 250 - 288, Update startServer so a non-running existing client is stopped and evicted before restarting, ensuring the supplied configOverride is applied instead of reusing existingClient.serverConfig. Preserve the existing running-client fast path, and merge configOverride.env with the persisted environment keys rather than replacing the entire env object.
🧹 Nitpick comments (11)
test/main/plugin/cuaEmbeddedAdapter.test.ts (1)
315-332: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: inject
requestMetadatahere to avoid a real connect against a fixed/tmppath.This case relies on the default dependency attempting a real connection to the hardcoded
/tmp/deepchat-cua-123-aabbccddeeff.sock. It passes because the path is normally absent, but a leftover socket/file on a developer or CI machine would change the outcome. The neighbouring tests already stubrequestMetadata; doing the same here keeps it hermetic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/plugin/cuaEmbeddedAdapter.test.ts` around lines 315 - 332, Update the test around CuaEmbeddedRuntimeAdapter.recoverStaleLaunch to inject a requestMetadata stub, matching the neighboring tests. Ensure the stub avoids any real connection to the fixed endpoint while preserving the identity-matching recovery assertions.src/main/plugin/cuaEmbeddedAdapter.ts (1)
386-386: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNit: local
endpointIdentityshadows the module-level helper of the same name.No behavioral issue here (the helper is reached via
dependencies.captureEndpointIdentity), but the shadowing makes Lines 439/453 harder to read and is a trap for future edits. Consider renaming the local tocapturedIdentity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/plugin/cuaEmbeddedAdapter.ts` at line 386, Rename the local endpointIdentity variable in the relevant adapter flow to capturedIdentity, and update all references such as those near lines 439 and 453 while preserving the existing dependencies.captureEndpointIdentity helper usage.src/main/plugin/cuaRuntimeIntegrity.ts (1)
299-308: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a timeout for
codesign/plutilinvocations.
runCommandruns on the pre-spawn verification path for every guarded start. Withouttimeout, a hungcodesign(e.g. blocked on a stapling/keychain prompt) leaves the runtime start pending indefinitely with no error surfaced to the supervisor.♻️ Proposed change
const runCommand = async (command: string, args: string[]): Promise<CommandResult> => { const result = await execFileAsync(command, args, { encoding: 'utf8', - maxBuffer: COMMAND_OUTPUT_LIMIT + maxBuffer: COMMAND_OUTPUT_LIMIT, + timeout: COMMAND_TIMEOUT_MS })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/plugin/cuaRuntimeIntegrity.ts` around lines 299 - 308, Update runCommand to provide a finite timeout when invoking codesign/plutil through execFileAsync, ensuring hung verification commands reject and surface an error to the supervisor instead of blocking startup indefinitely. Preserve the existing output conversion and COMMAND_OUTPUT_LIMIT settings.src/main/plugin/cuaToolAdapter.ts (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
CUA_PLUGIN_IDconstant.
src/main/plugin/index.ts(Line 42) declares its ownconst CUA_PLUGIN_ID = 'com.deepchat.plugins.cua'. Import the exported one here instead to keep a single source of truth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/plugin/cuaToolAdapter.ts` at line 3, Remove the duplicate CUA_PLUGIN_ID declaration from src/main/plugin/index.ts and import the exported CUA_PLUGIN_ID from cuaToolAdapter.ts instead, preserving all existing usages and the single shared value.test/main/plugin/pluginService.test.ts (1)
1366-1375: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer behavioral assertions over source-text matching.
Grepping
src/main/plugin/index.tsfor identifiers likethis.runtimeSupervisor.registerServercouples the suite to source formatting: destructuring the supervisor or renaming a private helper breaks this test without any behavior change. The supervisor wiring is already covered behaviorally at Lines 762-775 and 894-919 via theruntimeSupervisormock; consider dropping the string checks (or keeping only thestartPluginMcpServersIfReadyremoval check as a temporary migration guard).As per coding guidelines, "Keep committed tests lean and focused on project reliability, stability, and observable contracts; remove temporary checks that only test implementation internals before handoff."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/plugin/pluginService.test.ts` around lines 1366 - 1375, Replace the source-text identifier assertions in the test case with behavioral coverage using the existing runtimeSupervisor mock and related tests around plugin registration and reconciliation. Remove checks for helper names and registerServer/reconcilePlugin source strings; retain the startPluginMcpServersIfReady absence check only if it is still required as a temporary migration guard.Source: Coding guidelines
test/main/scripts/packagePlugin.test.ts (1)
277-299: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd the negative case for a missing Apple Team ID.
createDarwinSigningContracthard-fails distribution packaging whenDEEPCHAT_APPLE_NOTARY_TEAM_IDis absent or malformed, but no test covers it — a regression there would silently emit anad-hoc/null-team descriptor for a release artifact.As per coding guidelines, "Add the smallest regression test for user-visible behavior or a documented contract."
💚 Suggested test
it('rejects macOS distribution packaging without a valid Apple Team ID', async () => { const fixture = await createCuaPluginFixture() const outDir = path.join(fixture.root, 'out') const result = runPackagePlugin(fixture.pluginDir, outDir, 'darwin', 'arm64', { purpose: 'distribution', env: { DEEPCHAT_APPLE_NOTARY_TEAM_ID: '' } }) expect(result.status).not.toBe(0) expect(result.stderr).toContain('requires a valid Apple Team ID') })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/scripts/packagePlugin.test.ts` around lines 277 - 299, Add a regression test alongside the existing macOS distribution packaging test that invokes runPackagePlugin with an empty DEEPCHAT_APPLE_NOTARY_TEAM_ID, then assert the process fails and stderr contains “requires a valid Apple Team ID.”Source: Coding guidelines
scripts/build-cua-plugin-runtime.mjs (1)
714-718: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the native-target guard before download/stage/sign.
This condition depends only on
targetPlatform/targetArchversus the host, yet it fires after downloading, extracting, staging, codesigning, and smoke-checking. Hoisting it next to the arg parsing (around Line 679) fails fast and avoids leaving a signed-but-unusable runtime dir behind for cross-target invocations.Also note Line 727:
fs.staton Line 726 already throws for a missing executable, so thefsSync.existsSynccheck is unreachable.♻️ Proposed hoist
- smokeCheck(executable, targetPlatform, targetArch) - if (!canRunTarget(targetPlatform, targetArch)) { - throw new Error( - `CUA MCP tool catalog must be generated on its native target; host is ${process.platform}/${process.arch}, target is ${targetPlatform}/${targetArch}` - ) - } + smokeCheck(executable, targetPlatform, targetArch)Add near the top of
main()(aftertargetArchis normalized):if (!canRunTarget(targetPlatform, targetArch)) { throw new Error( `CUA MCP tool catalog must be generated on its native target; host is ${process.platform}/${process.arch}, target is ${targetPlatform}/${targetArch}` ) }🤖 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 `@scripts/build-cua-plugin-runtime.mjs` around lines 714 - 718, Move the canRunTarget guard from the late build flow into main(), immediately after targetArch is normalized, so cross-target invocations fail before download, staging, signing, or smoke checks. Preserve the existing error message and remove the later duplicate guard; also remove the redundant fsSync.existsSync check near fs.stat because fs.stat already rejects missing executables.src/main/mcp/toolManager.ts (3)
1021-1027: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the now-unused
_serverConfigparameter.
prepareToolArgumentsno longer reads the server config; removing the parameter (and the argument at Line 851) keeps the signature honest.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/mcp/toolManager.ts` around lines 1021 - 1027, Remove the unused _serverConfig parameter from prepareToolArguments and remove the corresponding argument at its call site around line 851, preserving the remaining parameter order and behavior.
433-449: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate predicate, and plugin-owned servers bypass
enabledServerIdsgating.
isServerConfigAllowedByContextis now byte-identical toisServerAllowedByContextapart from an unused_serverConfigparameter — collapse the two and drop the dead parameter at the call sites (Lines 626, 789).Separately, please confirm the exemption at Line 434/445 is intended: any supervisor-available plugin server is allowed regardless of an agent's configured
enabledServerIds, so an agent with a restricted MCP allowlist can still reach CUA tools.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/mcp/toolManager.ts` around lines 433 - 449, Collapse isServerConfigAllowedByContext into isServerAllowedByContext, remove the unused _serverConfig parameter, and update the call sites around the referenced usages to call the shared predicate. Preserve the current pluginOwnership.isServerAvailable exemption so supervisor-available plugin servers remain allowed regardless of enabledServerIds.
162-189: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDiagnostics call has no timeout/abort.
client.callTool('check_permissions', …)at Line 180 (and thelistTools()insideverifyCatalogTool) can hang indefinitely if the runtime is wedged, leaving the settings "Test runtime" action spinning with no way to cancel. Consider passing anAbortSignalwith a bounded timeout through toverifyCatalogTool/callTool.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/mcp/toolManager.ts` around lines 162 - 189, Add a bounded timeout with an AbortSignal to checkPluginRuntimePermissions, and propagate that signal through verifyCatalogTool and the client.callTool('check_permissions', ...) invocation. Ensure both the listTools validation and diagnostics call terminate when the timeout expires, preserving the existing permission and error handling behavior.test/main/mcp/toolManager.test.ts (1)
92-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHelper can't express "owned but unavailable", leaving the quarantine path untested.
isServerAvailableis hardwired toownsServer, so no test exercises the branches this PR adds for a quarantined/unavailable plugin server: the skip inloadToolSources(toolManager.ts Lines 344-350) and the"is no longer available"throw inresolveToolClient(Lines 939-941). Allowing anunavailableset in the helper would make that regression cheap to cover.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/mcp/toolManager.test.ts` around lines 92 - 131, The createToolManager test helper currently couples server ownership with availability, preventing tests from modeling quarantined servers. Add an optional unavailable-server set to createToolManager and make isServerAvailable return false for those names while preserving ownership behavior; use it to cover the unavailable branches in loadToolSources and resolveToolClient.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@resources/model-db/providers.json`:
- Around line 258595-258614: Add a meaningful type field with value "tts" to
every listed TTS model object, including tts-1, tts-1-hd, tts-1-1106, and
tts-1-hd-1106 across the referenced ranges. Place it alongside the existing cost
and model metadata so sanitizeAggregate preserves the TTS classification.
In `@src/main/mcp/index.ts`:
- Around line 781-808: Update the startup flow spanning startServerDirect,
ServerManager, and McpClient.connect so waitForConnection preserves access to
the underlying connection completion after a soft timeout; do not read
getConnectionCompletion() only after connectPromise may have been cleared by
finally. Propagate the wait requirement through ServerManager/McpClient.connect,
or retain the completion until startServerDirect has accounted for it, ensuring
the CUA path waits for and handles the real connection result instead of
throwing a missing-completion error.
In `@src/main/plugin/index.ts`:
- Around line 327-332: Clear the plugin’s entry from activationErrors when
explicitly disabling it in the flow containing this.upsertInstallation and
disableByOwner(pluginId). Ensure buildPluginListItem no longer surfaces the
stale activation error after disable, while preserving the existing disable and
installation-update behavior.
---
Outside diff comments:
In `@src/main/mcp/serverManager.ts`:
- Around line 250-288: Update startServer so a non-running existing client is
stopped and evicted before restarting, ensuring the supplied configOverride is
applied instead of reusing existingClient.serverConfig. Preserve the existing
running-client fast path, and merge configOverride.env with the persisted
environment keys rather than replacing the entire env object.
---
Nitpick comments:
In `@scripts/build-cua-plugin-runtime.mjs`:
- Around line 714-718: Move the canRunTarget guard from the late build flow into
main(), immediately after targetArch is normalized, so cross-target invocations
fail before download, staging, signing, or smoke checks. Preserve the existing
error message and remove the later duplicate guard; also remove the redundant
fsSync.existsSync check near fs.stat because fs.stat already rejects missing
executables.
In `@src/main/mcp/toolManager.ts`:
- Around line 1021-1027: Remove the unused _serverConfig parameter from
prepareToolArguments and remove the corresponding argument at its call site
around line 851, preserving the remaining parameter order and behavior.
- Around line 433-449: Collapse isServerConfigAllowedByContext into
isServerAllowedByContext, remove the unused _serverConfig parameter, and update
the call sites around the referenced usages to call the shared predicate.
Preserve the current pluginOwnership.isServerAvailable exemption so
supervisor-available plugin servers remain allowed regardless of
enabledServerIds.
- Around line 162-189: Add a bounded timeout with an AbortSignal to
checkPluginRuntimePermissions, and propagate that signal through
verifyCatalogTool and the client.callTool('check_permissions', ...) invocation.
Ensure both the listTools validation and diagnostics call terminate when the
timeout expires, preserving the existing permission and error handling behavior.
In `@src/main/plugin/cuaEmbeddedAdapter.ts`:
- Line 386: Rename the local endpointIdentity variable in the relevant adapter
flow to capturedIdentity, and update all references such as those near lines 439
and 453 while preserving the existing dependencies.captureEndpointIdentity
helper usage.
In `@src/main/plugin/cuaRuntimeIntegrity.ts`:
- Around line 299-308: Update runCommand to provide a finite timeout when
invoking codesign/plutil through execFileAsync, ensuring hung verification
commands reject and surface an error to the supervisor instead of blocking
startup indefinitely. Preserve the existing output conversion and
COMMAND_OUTPUT_LIMIT settings.
In `@src/main/plugin/cuaToolAdapter.ts`:
- Line 3: Remove the duplicate CUA_PLUGIN_ID declaration from
src/main/plugin/index.ts and import the exported CUA_PLUGIN_ID from
cuaToolAdapter.ts instead, preserving all existing usages and the single shared
value.
In `@test/main/mcp/toolManager.test.ts`:
- Around line 92-131: The createToolManager test helper currently couples server
ownership with availability, preventing tests from modeling quarantined servers.
Add an optional unavailable-server set to createToolManager and make
isServerAvailable return false for those names while preserving ownership
behavior; use it to cover the unavailable branches in loadToolSources and
resolveToolClient.
In `@test/main/plugin/cuaEmbeddedAdapter.test.ts`:
- Around line 315-332: Update the test around
CuaEmbeddedRuntimeAdapter.recoverStaleLaunch to inject a requestMetadata stub,
matching the neighboring tests. Ensure the stub avoids any real connection to
the fixed endpoint while preserving the identity-matching recovery assertions.
In `@test/main/plugin/pluginService.test.ts`:
- Around line 1366-1375: Replace the source-text identifier assertions in the
test case with behavioral coverage using the existing runtimeSupervisor mock and
related tests around plugin registration and reconciliation. Remove checks for
helper names and registerServer/reconcilePlugin source strings; retain the
startPluginMcpServersIfReady absence check only if it is still required as a
temporary migration guard.
In `@test/main/scripts/packagePlugin.test.ts`:
- Around line 277-299: Add a regression test alongside the existing macOS
distribution packaging test that invokes runPackagePlugin with an empty
DEEPCHAT_APPLE_NOTARY_TEAM_ID, then assert the process fails and stderr contains
“requires a valid Apple Team ID.”
🪄 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: 2da7a71a-297f-419b-8584-878b19de7178
📒 Files selected for processing (88)
docs/architecture/plugin-external-runtime-lifecycle/plan.mddocs/architecture/plugin-external-runtime-lifecycle/spec.mddocs/architecture/plugin-external-runtime-lifecycle/tasks.mddocs/features/cua-cross-platform-computer-use/plan.mddocs/features/cua-cross-platform-computer-use/spec.mddocs/features/cua-cross-platform-computer-use/tasks.mdplugins/cua/build/entitlements.plistplugins/cua/mcp/cua-driver.jsonplugins/cua/plugin.jsonplugins/cua/policies/tool-policy.jsonplugins/cua/settings/assets/index.cssplugins/cua/settings/assets/index.jsplugins/cua/settings/index.htmlplugins/cua/skills/computer-use/README.mdplugins/cua/skills/computer-use/SKILL.mdplugins/cua/skills/computer-use/TESTS.mdplugins/cua/skills/computer-use/WEB_APPS.mdplugins/cua/vendor/cua-driver/upstream.jsonresources/acp-registry/registry.jsonresources/model-db/providers.jsonscripts/build-cua-plugin-runtime.mjsscripts/cua-macos-contract.mjsscripts/cua-tool-catalog-contract.mjsscripts/package-plugin.mjsscripts/plugin.mjssrc/main/agent/deepchat/loop/ports.tssrc/main/agent/deepchat/runtime/deferredToolExecutor.tssrc/main/agent/deepchat/runtime/dispatch.tssrc/main/agent/deepchat/runtime/toolAdapters.tssrc/main/agent/deepchat/runtime/toolRuntimeBindings.tssrc/main/agent/shared/process/processTree.tssrc/main/app/composition.tssrc/main/mcp/index.tssrc/main/mcp/mcpClient.tssrc/main/mcp/processEnvironment.tssrc/main/mcp/serverManager.tssrc/main/mcp/toolManager.tssrc/main/plugin/cuaEmbeddedAdapter.tssrc/main/plugin/cuaRuntimeIntegrity.tssrc/main/plugin/cuaToolAdapter.tssrc/main/plugin/index.tssrc/main/plugin/runtimeSupervisor.tssrc/main/plugin/toolCatalog.tssrc/main/plugin/toolPolicyStore.tssrc/preload/plugin-settings-preload.tssrc/renderer/settings/components/PluginsSettings.vuesrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/de-DE/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/es-ES/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/id-ID/settings.jsonsrc/renderer/src/i18n/it-IT/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/ms-MY/settings.jsonsrc/renderer/src/i18n/pl-PL/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/tr-TR/settings.jsonsrc/renderer/src/i18n/vi-VN/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/renderer/src/pages/plugins/OfficialPluginDetailPage.vuesrc/shared/types/core/mcp.tssrc/shared/types/mcp.tssrc/shared/types/plugin.tstest/main/agent/deepchat/runtime/toolAdapters.test.tstest/main/agent/shared/process/processTree.test.tstest/main/mcp/mcpClient.test.tstest/main/mcp/mcpService.test.tstest/main/mcp/processEnvironment.test.tstest/main/mcp/toolManager.test.tstest/main/plugin/cuaEmbeddedAdapter.test.tstest/main/plugin/cuaRuntimeIntegrity.test.tstest/main/plugin/cuaToolAdapter.test.tstest/main/plugin/pluginService.test.tstest/main/plugin/runtimeSupervisor.test.tstest/main/plugin/toolCatalog.test.tstest/main/scripts/buildCuaPluginRuntime.test.tstest/main/scripts/packagePlugin.test.tstest/main/scripts/verifyCuaMacosHelper.test.tstest/renderer/components/OfficialPluginDetailPage.test.tstest/renderer/plugins/cuaSettings.test.tstest/renderer/stores/mcpStore.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/plugin/cuaEmbeddedAdapter.ts (1)
353-380: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftSerialize start and stop transitions.
Two concurrent
start()calls can both observe no running daemon and spawn independently; the final assignment at Line 446 overwrites ownership of the first process. Likewise,stop()during startup sees norunningstate and returns, allowing the daemon to come up after shutdown was requested. Use one lifecycle mutex/queue for both operations and add concurrent start/stop regression coverage.Also applies to: 430-447, 488-521
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/plugin/cuaEmbeddedAdapter.ts` around lines 353 - 380, Serialize the lifecycle transitions in the CuaEmbeddedAdapter so concurrent start() calls cannot spawn multiple daemons and stop() cannot return before an in-progress startup completes. Use a shared mutex or promise queue around the full start() and stop() flows, including process creation and running-state assignment, then add regression coverage for concurrent starts and stop-during-startup 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 `@src/main/mcp/mcpClient.ts`:
- Around line 1298-1303: Update the listTools catch block after
awaitWithAbort(this.checkAndHandleSessionError(error), options?.signal) to call
options?.signal?.throwIfAborted(), matching callTool. Ensure cancellation that
occurs during session-error handling is propagated before the code continues to
cache, return an empty result, or rethrow the original error.
In `@test/main/scripts/packagePlugin.test.ts`:
- Around line 287-299: Extend the policy-projection test matrix in the
platform/expectedPolicy loop to include a Darwin case with the expected macOS
target-scoped policy, ensuring Linux- and Windows-only entries are not accepted
in the Darwin artifact.
---
Outside diff comments:
In `@src/main/plugin/cuaEmbeddedAdapter.ts`:
- Around line 353-380: Serialize the lifecycle transitions in the
CuaEmbeddedAdapter so concurrent start() calls cannot spawn multiple daemons and
stop() cannot return before an in-progress startup completes. Use a shared mutex
or promise queue around the full start() and stop() flows, including process
creation and running-state assignment, then add regression coverage for
concurrent starts and stop-during-startup 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: 0323a36b-8f0c-4735-80b5-2de63be7ff4a
📒 Files selected for processing (30)
docs/architecture/plugin-external-runtime-lifecycle/plan.mddocs/architecture/plugin-external-runtime-lifecycle/spec.mddocs/architecture/plugin-external-runtime-lifecycle/tasks.mdplugins/cua/plugin.jsonplugins/cua/policies/tool-policy.jsonresources/model-db/providers.jsonscripts/build-cua-plugin-runtime.mjsscripts/fetch-provider-db.mjsscripts/package-plugin.mjssrc/main/agent/deepchat/runtime/toolAdapters.tssrc/main/mcp/index.tssrc/main/mcp/mcpClient.tssrc/main/mcp/serverManager.tssrc/main/mcp/toolManager.tssrc/main/plugin/cuaEmbeddedAdapter.tssrc/main/plugin/cuaRuntimeIntegrity.tssrc/main/plugin/cuaToolAdapter.tssrc/main/plugin/index.tssrc/main/plugin/toolPolicyStore.tssrc/renderer/src/pages/plugins/OfficialPluginDetailPage.vuesrc/renderer/src/pages/plugins/PluginsCatalogPage.vuesrc/shared/types/plugin.tstest/main/mcp/mcpClient.test.tstest/main/mcp/mcpService.test.tstest/main/mcp/serverManager.test.tstest/main/mcp/toolManager.test.tstest/main/plugin/cuaEmbeddedAdapter.test.tstest/main/plugin/pluginService.test.tstest/main/scripts/fetchProviderDb.test.tstest/main/scripts/packagePlugin.test.ts
💤 Files with no reviewable changes (1)
- src/main/plugin/cuaToolAdapter.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- test/main/mcp/mcpClient.test.ts
- plugins/cua/plugin.json
- src/renderer/src/pages/plugins/OfficialPluginDetailPage.vue
- src/shared/types/plugin.ts
- test/main/mcp/mcpService.test.ts
- scripts/build-cua-plugin-runtime.mjs
- src/main/plugin/toolPolicyStore.ts
- src/main/agent/deepchat/runtime/toolAdapters.ts
- docs/architecture/plugin-external-runtime-lifecycle/plan.md
- test/main/mcp/toolManager.test.ts
- docs/architecture/plugin-external-runtime-lifecycle/spec.md
- scripts/package-plugin.mjs
- docs/architecture/plugin-external-runtime-lifecycle/tasks.md
- src/main/mcp/toolManager.ts
- src/main/plugin/index.ts
- test/main/plugin/pluginService.test.ts
- src/main/mcp/index.ts
Cover Darwin-specific policy projection when packaging CUA.
1 participant
Summary
Fixes #2039 by upgrading the bundled CUA runtime to
0.12.6and introducing a single lifecycle boundary for plugin-owned external processes.CUA now starts on demand instead of during plugin activation, preventing the legacy eager-start behavior that could disrupt Linux/X11 desktop sessions.
Changes
0.12.6, including macOS signing contracts and removal of the Windows UIAccess worker.element_token, structured tool results, and opt-in screenshot grounding.Closes #2039
Summary by CodeRabbit