feat(cua): add cross-platform runtimes#1776
Open
zerob13 wants to merge 8 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe CUA plugin is migrated from a macOS-only Swift fork to the upstream Rust driver v0.5.5, adding cross-platform support for darwin, win32, and linux (excluding linux/arm64). The change rewrites the runtime staging script to download, verify, and extract pinned upstream release assets; updates packaging to filter per-target runtime subtrees with executable permission preservation; extends the plugin manifest type and plugin host with arch-aware support evaluation; wires cua bundle/verify into Windows and Linux CI jobs; adds runtime arch awareness to settings navigation; and updates skill documentation for cross-platform guidance. Additional fixes address GitHub Actions build failures on Windows arm64 and Linux x64 by rewriting the ACP registry fetcher to use Node HTTPS with sequential icon downloads. ChangesCUA cross-platform runtime expansion
Build action platform failures remediation
Sequence Diagram(s)sequenceDiagram
participant CI as CI Workflow
participant build as build-cua-plugin-runtime.mjs
participant gh as GitHub Releases
participant pkg as package-plugin.mjs
participant host as PluginPresenter
participant nav as settingsNavigation
CI->>build: pnpm run plugin:cua:build --platform win32 --arch x64
build->>gh: GET checksums.txt (cua-driver-rs-v0.5.5)
build->>gh: GET cua-driver-win32-x64.zip
gh-->>build: archive + checksums
build->>build: SHA-256 verify
build->>build: extract (ZIP path-safety check)
build->>build: stage binary → plugins/cua/runtime/win32/x64/
build-->>CI: staged runtime path
CI->>pkg: pnpm run plugin:bundle --name cua --platform win32 --arch x64
pkg->>pkg: skip runtime/darwin/*, runtime/linux/*
pkg->>pkg: collectFiles with mode bits
pkg->>pkg: validateCuaRuntime for win32/x64
pkg->>pkg: set zip executable attributes
pkg-->>CI: cua-win32-x64.dcplugin
CI->>CI: pnpm run plugin:verify --name cua --platform win32 --arch x64
host->>host: isPluginPlatformSupported(engines.targets, platform/arch)
host->>host: resolveRuntimeCandidate (this.arch placeholder)
host->>host: checkRuntimePermissions (skip probe on non-darwin)
nav->>nav: getSettingsRouteItems(platform, arch)
nav->>nav: isSettingsNavigationItemSupported with supportedTargets
nav-->>nav: filter settings-plugins for win32/x64
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/guides/plugin-packaging.md (1)
180-190: 💤 Low valueClarify the scope of the embedded files example.
The label "Expected embedded files (macOS example):" followed by win32 and linux plugin filenames is ambiguous. Either label this as an all-platforms example or split it into platform-specific examples so readers understand the context.
✏️ Example clarification
Option 1: Change label to reflect cross-platform scope:
-Expected embedded files (macOS example): +Expected embedded files (example across all platforms):Option 2: Split into platform-specific examples:
-Expected embedded files (macOS example): +Expected embedded files by platform: + +macOS:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/guides/plugin-packaging.md` around lines 180 - 190, The section labeled "Expected embedded files (macOS example):" contains file paths for multiple platforms (macOS, Windows, and Linux) which contradicts the "macOS example" designation and creates ambiguity for readers. Either retitle the section to "Expected embedded files (All platforms example):" or similar language that accurately reflects the cross-platform scope, or alternatively split the example into separate platform-specific subsections (one for macOS, one for Windows, one for Linux) so readers understand which files apply to their target platform.scripts/build-cua-plugin-runtime.mjs (1)
151-163: 💤 Low valueConsider invalidating cache on checksum failure.
If a download is interrupted, a partial file may remain in the cache. While
verifyChecksumwill catch this and fail, the user must manually delete the cached file to retry. Consider adding cache invalidation on checksum mismatch or removing the partial file on download failure.♻️ Optional improvement to auto-invalidate on checksum failure
async function verifyChecksum(checksumsPath, assetPath, assetName) { const checksums = parseChecksums(await fs.readFile(checksumsPath, 'utf8')) const expected = checksums.get(assetName) if (!expected) { throw new Error(`checksums.txt does not contain ${assetName}`) } const actual = await sha256File(assetPath) if (actual !== expected) { + await fs.rm(assetPath, { force: true }) throw new Error(`Checksum mismatch for ${assetName}. Expected ${expected}, got ${actual}`) } }🤖 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 151 - 163, The downloadFile function does not clean up partial files if a download fails or if subsequent checksum verification fails, leaving corrupted cache entries. Wrap the fetch and file write operations in a try-catch block within downloadFile to delete the outputPath file if an error occurs during download, ensuring the partial file is removed from cache. Additionally, identify where verifyChecksum is called after downloadFile and add error handling there to also delete the outputPath file if checksum verification fails, so the cached file is invalidated and the user can retry the download without manual intervention.
🤖 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 `@scripts/plugin.mjs`:
- Around line 85-91: The isPluginSupported function compares lowercased manifest
targets against potentially uppercase input values from CLI or environment
variables, causing false-negative matches with inputs like WIN32 or X64.
Normalize both targetPlatform and targetArch to lowercase at the start of the
function before using them in the aliases comparison and the target inclusion
check to ensure consistent case-insensitive matching throughout.
In `@test/main/presenter/pluginPresenter.test.ts`:
- Around line 796-813: The constants expectedAllow and expectedAsk in the test
do not follow the SCREAMING_SNAKE_CASE naming convention required by the project
coding guidelines. Rename expectedAllow to EXPECTED_ALLOW and expectedAsk to
EXPECTED_ASK, then update all references to these constants throughout the test
file to use the new names instead of the old camelCase versions.
---
Nitpick comments:
In `@docs/guides/plugin-packaging.md`:
- Around line 180-190: The section labeled "Expected embedded files (macOS
example):" contains file paths for multiple platforms (macOS, Windows, and
Linux) which contradicts the "macOS example" designation and creates ambiguity
for readers. Either retitle the section to "Expected embedded files (All
platforms example):" or similar language that accurately reflects the
cross-platform scope, or alternatively split the example into separate
platform-specific subsections (one for macOS, one for Windows, one for Linux) so
readers understand which files apply to their target platform.
In `@scripts/build-cua-plugin-runtime.mjs`:
- Around line 151-163: The downloadFile function does not clean up partial files
if a download fails or if subsequent checksum verification fails, leaving
corrupted cache entries. Wrap the fetch and file write operations in a try-catch
block within downloadFile to delete the outputPath file if an error occurs
during download, ensuring the partial file is removed from cache. Additionally,
identify where verifyChecksum is called after downloadFile and add error
handling there to also delete the outputPath file if checksum verification
fails, so the cached file is invalidated and the user can retry the download
without manual intervention.
🪄 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: bd5df07b-423c-46cd-a0e7-1e9b9f739cc3
📒 Files selected for processing (22)
.github/workflows/build.yml.github/workflows/release.ymldocs/features/cua-cross-platform-computer-use/plan.mddocs/features/cua-cross-platform-computer-use/spec.mddocs/features/cua-cross-platform-computer-use/tasks.mddocs/guides/plugin-packaging.mdpackage.jsonplugins/cua/plugin.jsonplugins/cua/policies/tool-policy.jsonplugins/cua/settings/assets/index.jsplugins/cua/skills/cua-driver/README.mdplugins/cua/skills/cua-driver/RECORDING.mdplugins/cua/skills/cua-driver/SKILL.mdplugins/cua/skills/cua-driver/TESTS.mdplugins/cua/skills/cua-driver/WEB_APPS.mdplugins/cua/vendor/cua-driver/upstream.jsonscripts/build-cua-plugin-runtime.mjsscripts/package-plugin.mjsscripts/plugin.mjssrc/main/presenter/pluginPresenter/index.tssrc/shared/types/plugin.tstest/main/presenter/pluginPresenter.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/main/presenter/pluginPresenter.test.ts (1)
889-896: ⚡ Quick winHarden implementation-coupled string checks to reduce false CI failures.
These assertions are currently tied to exact source text/newline indentation (
toContain('- name: Build Windows\n shell: bash')and specific loop text), so harmless refactors can break tests without behavior changes. Prefer regex/structure-based checks for intent-level validation.Suggested refactor
- expect(source).toContain('for (const agent of iconAgents)') + expect(source).toMatch(/for\s*\(\s*const\s+\w+\s+of\s+iconAgents\s*\)/) - expect(buildWorkflow).toContain('- name: Build Windows\n shell: bash') + expect(buildWorkflow).toMatch(/- name:\s*Build Windows[\s\S]*?shell:\s*bash/)Also applies to: 1018-1018
🤖 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/presenter/pluginPresenter.test.ts` around lines 889 - 896, The test assertions in the 'keeps ACP registry build-time fetching compatible with Windows arm64' test case use exact string matching with toContain(), which is brittle and can fail due to harmless formatting or indentation changes. Replace these toContain() calls with regex-based toMatch() checks that validate the intent of the code (such as checking for the presence of node:https imports and the loop iteration pattern) without coupling to exact whitespace and newlines. This applies to all similar assertion patterns in the test file that use toContain() for source code validation, making the tests more resilient to refactoring.
🤖 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.
Nitpick comments:
In `@test/main/presenter/pluginPresenter.test.ts`:
- Around line 889-896: The test assertions in the 'keeps ACP registry build-time
fetching compatible with Windows arm64' test case use exact string matching with
toContain(), which is brittle and can fail due to harmless formatting or
indentation changes. Replace these toContain() calls with regex-based toMatch()
checks that validate the intent of the code (such as checking for the presence
of node:https imports and the loop iteration pattern) without coupling to exact
whitespace and newlines. This applies to all similar assertion patterns in the
test file that use toContain() for source code validation, making the tests more
resilient to refactoring.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29a16b4a-9cdd-418b-9dc7-c92cd41a2e0b
📒 Files selected for processing (10)
.github/workflows/build.ymldocs/features/cua-cross-platform-computer-use/plan.mddocs/features/cua-cross-platform-computer-use/spec.mddocs/features/cua-cross-platform-computer-use/tasks.mddocs/issues/build-action-platform-failures/plan.mddocs/issues/build-action-platform-failures/spec.mddocs/issues/build-action-platform-failures/tasks.mdscripts/build-cua-plugin-runtime.mjsscripts/fetch-acp-registry.mjstest/main/presenter/pluginPresenter.test.ts
✅ Files skipped from review due to trivial changes (6)
- docs/issues/build-action-platform-failures/tasks.md
- docs/issues/build-action-platform-failures/plan.md
- docs/issues/build-action-platform-failures/spec.md
- docs/features/cua-cross-platform-computer-use/tasks.md
- docs/features/cua-cross-platform-computer-use/spec.md
- docs/features/cua-cross-platform-computer-use/plan.md
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/build.yml
- scripts/build-cua-plugin-runtime.mjs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/shared/settingsNavigation.ts`:
- Around line 294-297: The fail-closed target gating logic in the
settingsNavigation.ts file is inverted. When supportedTargets is defined and
either platform or arch is missing, the function currently returns true (making
the item visible), which is fail-open behavior. To implement proper fail-closed
behavior, change the return statement in the condition checking (!platform ||
!arch) within the item.supportedTargets?.length block to return false instead of
true, so that items are hidden by default when target platform information is
unavailable.
🪄 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: a35b9450-bab7-48ca-9c0c-339a5154e0ac
📒 Files selected for processing (10)
docs/features/cua-cross-platform-computer-use/spec.mdsrc/main/presenter/windowPresenter/index.tssrc/preload/index.d.tssrc/preload/index.tssrc/renderer/api/runtime.tssrc/renderer/settings/App.vuesrc/renderer/settings/components/SettingsOverview.vuesrc/renderer/settings/main.tssrc/shared/settingsNavigation.tstest/main/shared/settingsNavigation.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/preload/index.d.ts
- docs/features/cua-cross-platform-computer-use/spec.md
Comment on lines
+294
to
+297
| if (item.supportedTargets?.length) { | ||
| if (!platform || !arch) { | ||
| return true | ||
| } |
Contributor
There was a problem hiding this comment.
Fail-closed target gating is currently implemented as fail-open
When supportedTargets is defined, returning true for missing platform/arch makes settings-plugins visible by default. That contradicts the stated fail-closed behavior for unsupported targets.
Suggested fix
if (item.supportedTargets?.length) {
- if (!platform || !arch) {
- return true
- }
+ if (!platform || !arch) {
+ return false
+ }
const normalizedArch = arch.trim().toLowerCase()
const aliases = getPlatformAliases(platform)
const targets = item.supportedTargets.map((target) => target.trim().toLowerCase())
return [...aliases].some((platformAlias) =>
targets.includes(`${platformAlias}/${normalizedArch}`)
)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (item.supportedTargets?.length) { | |
| if (!platform || !arch) { | |
| return true | |
| } | |
| if (item.supportedTargets?.length) { | |
| if (!platform || !arch) { | |
| return false | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/shared/settingsNavigation.ts` around lines 294 - 297, The fail-closed
target gating logic in the settingsNavigation.ts file is inverted. When
supportedTargets is defined and either platform or arch is missing, the function
currently returns true (making the item visible), which is fail-open behavior.
To implement proper fail-closed behavior, change the return statement in the
condition checking (!platform || !arch) within the item.supportedTargets?.length
block to return false instead of true, so that items are hidden by default when
target platform information is unavailable.
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
cua-driver-rs-v0.5.5release assets.darwin/arm64,darwin/x64,win32/x64,win32/arm64, andlinux/x64targets; keeplinux/arm64hidden and fail-closed.Validation
pnpm run formatpnpm run i18npnpm run lintpnpm run typecheckpnpm test -- test/main/presenter/pluginPresenter.test.ts test/main/scriptspnpm run plugin:bundle -- --name cua --platform win32 --arch x64pnpm run plugin:verify -- --name cua --platform win32 --arch x64 --plugin-root build\\bundled-pluginspnpm run plugin:bundle -- --name cua --platform win32 --arch arm64pnpm run plugin:verify -- --name cua --platform win32 --arch arm64 --plugin-root build\\bundled-pluginspnpm run build && pnpm exec electron-builder --win --x64 --dir --publish=neverdist\\win-unpacked\\resources\\app.asar.unpacked\\pluginsplugins\\cua\\runtime\\win32\\x64\\cua-driver.exe --versionplugins\\cua\\runtime\\win32\\x64\\cua-driver.exe check_permissionsSummary by CodeRabbit
Release Notes
New Features
Bug Fixes & Improvements
install_ffmpeg.Documentation
UI