Repository navigation
Codex/plugin capability sync - #1222
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds comprehensive plugin support to device capability synchronization across the backend, executor, and frontend. Changes extend API schemas with ChangesDevice Capability Plugin Sync
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
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 |
FicoHu
force-pushed
the
codex/plugin-capability-sync
branch
from
June 1, 2026 07:39
c443acb to
2824ced
Compare
![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: 7
🧹 Nitpick comments (4)
backend/tests/services/device/test_capability_sync_service.py (1)
101-155: ⚡ Quick winAdd a negative-path test for unauthorized or missing plugin IDs.
This only covers the happy path. Please add a case where
installed_plugin_idscontains another user's or a nonexistentInstalledPluginID and assertDeviceCapabilityResolutionError(404)so the new authorization path stays protected.As per coding guidelines,
**/*test*.{py,ts,tsx,js,jsx}: Test edge cases and error conditions.🤖 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 `@backend/tests/services/device/test_capability_sync_service.py` around lines 101 - 155, Add a negative-path test that ensures resolve_payload rejects unauthorized or missing plugin IDs: create a test (e.g., alongside test_resolve_payload_includes_selected_installed_plugins) that adds no InstalledPlugin or adds a Kind/InstalledPlugin with a different user_id, then call device_capability_sync_service.resolve_payload with installed_plugin_ids containing that other/nonexistent ID and assert it raises DeviceCapabilityResolutionError with a 404 status; use the same test DB setup, User(...) and reference device_capability_sync_service.resolve_payload and DeviceCapabilityResolutionError to locate where to add the assertion.backend/tests/services/test_cloud_device_provider.py (1)
43-68: ⚡ Quick winCover
get_status()too.The production change adds
bind_shellin bothlist_devices()andget_status(), but this test module only verifies the list path. A small companion test forget_status()would keep the two response shapes from drifting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/services/test_cloud_device_provider.py` around lines 43 - 68, Add a companion async test in the same test module that verifies CloudDeviceProvider.get_status preserves the bind_shell value: create cloud devices in test_db (e.g., using _cloud_device("cloud-claude","claudecode")), monkeypatch cache.mget and executor_version_service.get_latest_version like the existing test, call await CloudDeviceProvider().get_status(test_db, device_id="cloud-claude", user_id=7) and assert the returned status dict contains "bind_shell": "claudecode"; ensure you cover at least one cloud device and mirror the setup used in test_list_devices_preserves_cloud_device_bind_shell so response shapes don’t drift.backend/tests/api/ws/test_device_capabilities_state.py (1)
12-62: ⚡ Quick winAdd a partial-heartbeat regression case for
plugins.This only locks down the
full=Truepath. The branch that reuses existing state on same-digest partial heartbeats is the one that can silently drop previously stored plugin lists if it regresses, so it deserves a companion assertion here.As per coding guidelines, "Test edge cases and error conditions".
🤖 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 `@backend/tests/api/ws/test_device_capabilities_state.py` around lines 12 - 62, Add a companion regression case to ensure plugins are preserved when a partial heartbeat reuses existing state: in the test (test_store_device_capabilities_state_preserves_plugin_report) set device_service.get_device_capabilities_state to return an existing state containing a plugins list with the same digest, call device_namespace._store_device_capabilities_state with full=False and the same "digest" value, and assert that the stored capabilities (captured via the fake_store or AsyncMock for device_service.store_device_capabilities_state) still include the original plugins array; reference the existing test function name and the _store_device_capabilities_state helper to locate where to add this partial-heartbeat assertion.wework/src/components/layout/DesktopSidebar.tsx (1)
503-509: 💤 Low valueConsider adding error logging or user feedback for refresh failures.
The
try/finallypattern ensures the dialog opens even ifonRefreshDevicesfails, which is good for UX. However, there's no catch block to log errors or notify users when the refresh fails silently. Consider adding error handling:💡 Optional enhancement for error visibility
const openProjectCreateDialog = async (mode: ProjectCreateMode) => { try { await onRefreshDevices?.() + } catch (error) { + console.error('Failed to refresh devices before opening project dialog:', error) + // Optionally: show toast notification to user } finally { setProjectCreateMode(mode) } }🤖 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 `@wework/src/components/layout/DesktopSidebar.tsx` around lines 503 - 509, The openProjectCreateDialog function currently uses try/finally so onRefreshDevices failures are silent; add a catch to handle errors from onRefreshDevices and surface them (e.g., call processLogger/errorLogger or show a user toast) before continuing to setProjectCreateMode. Specifically, wrap await onRefreshDevices?.() with try { await onRefreshDevices?.() } catch (err) { /* log via your logger or call a notification/toast API with err.message */ } finally { setProjectCreateMode(mode) } so failures are logged and optionally displayed to the user while preserving the dialog opening 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 `@backend/app/services/device/capability_sync_service.py`:
- Around line 711-725: The _should_include_installed_plugin function is logging
raw installation metadata (spec["source"]) which may contain private
URLs/credentials; remove or sanitize that field in the logger call and instead
log stable, non-sensitive identifiers (e.g. spec.get("marketplace"), row.name,
spec.get("installState")/spec.get("state") or a normalized status). Update the
logger.info in _should_include_installed_plugin to omit spec.get("source") (or
replace it with a redacted marker) and include only
marketplace/name/state/enable flags to avoid leaking sensitive plugin source
data.
In `@executor/agents/claude_code/local_mode_strategy.py`:
- Around line 233-240: After the fallback copytree path, reapply the owner-only
permissions to the copied capability tree: after the
shutil.copytree(global_capability_dir, task_capability_dir, dirs_exist_ok=True)
call, walk task_capability_dir and set file mode/ownership to the same
owner-only policy used elsewhere (e.g., chmod 0700 on dirs and 0600 on files or
call the existing helper that enforces owner-only perms), ensuring the code that
creates/removes symlinks (task_capability_dir.is_symlink()/unlink()) and
variables task_capability_dir and global_capability_dir are preserved and the
permission enforcement runs only when the symlink fallback was used.
In `@executor/modes/local/capabilities.py`:
- Around line 723-792: _sync_plugins currently calls
self.store._load_installed_plugins() and later
self.store._save_installed_plugins() without holding the store lock, which
allows races; wrap the entire load-modify-save sequence in the same lock used by
the store (or a dedicated plugin cache lock) so the call to
self.store._load_installed_plugins(), the mutations to
installed_plugins/installed_map/records, and the subsequent
self.store._save_installed_plugins(...) are executed atomically; locate this
logic in _sync_plugins and use the store's existing lock object/method (or add
one) to acquire before load and release after save.
In `@wework/src/features/workbench/WorkbenchProvider.test.tsx`:
- Around line 272-283: The test invokes handlers.onDeviceOnline which triggers
React state updates outside of Testing Library’s act, so wrap the call in
act(...) (e.g., use act(() => handlers.onDeviceOnline({...}))) before awaiting
waitFor and assertions; ensure you still await/wrap any async assertions
(waitFor + expect(screen.getByTestId('device-list'))...) and keep the
listDevices assertion (expect(listDevices).toHaveBeenCalledTimes(2)) after the
act/waitFor sequence so all state updates are flushed.
In `@wework/src/features/workbench/WorkbenchProvider.tsx`:
- Around line 315-325: The refreshDevices callback can race so an older
resolvedServices.deviceApi.listDevices() response may overwrite newer state; fix
by adding a monotonically increasing requestId/sequence (or reuse an
AbortController) stored in component scope and captured by refreshDevices and
the bootstrap fetch, incrementing it before each fetch and only dispatching when
the captured id equals the current latest id; update both refreshDevices and the
bootstrap device fetch logic (references: refreshDevices,
resolvedServices.deviceApi.listDevices, devices_refreshed dispatch, and the
bootstrap fetch path mentioned in the comment) to perform this check before
calling dispatch so stale responses are ignored.
- Around line 328-330: handleDeviceChanged currently calls refreshDevices() and
drops its promise, risking unhandled rejections when listDevices() fails; update
handleDeviceChanged to await or attach a rejection handler to the
refreshDevices() call so any error from refreshDevices()/listDevices() is caught
and logged/handled (e.g., call refreshDevices().catch(err => process/logging) or
make handleDeviceChanged async and use try/catch around await refreshDevices()).
Ensure you update the handler that references refreshDevices and the underlying
listDevices flow to prevent unhandled promise rejections.
---
Nitpick comments:
In `@backend/tests/api/ws/test_device_capabilities_state.py`:
- Around line 12-62: Add a companion regression case to ensure plugins are
preserved when a partial heartbeat reuses existing state: in the test
(test_store_device_capabilities_state_preserves_plugin_report) set
device_service.get_device_capabilities_state to return an existing state
containing a plugins list with the same digest, call
device_namespace._store_device_capabilities_state with full=False and the same
"digest" value, and assert that the stored capabilities (captured via the
fake_store or AsyncMock for device_service.store_device_capabilities_state)
still include the original plugins array; reference the existing test function
name and the _store_device_capabilities_state helper to locate where to add this
partial-heartbeat assertion.
In `@backend/tests/services/device/test_capability_sync_service.py`:
- Around line 101-155: Add a negative-path test that ensures resolve_payload
rejects unauthorized or missing plugin IDs: create a test (e.g., alongside
test_resolve_payload_includes_selected_installed_plugins) that adds no
InstalledPlugin or adds a Kind/InstalledPlugin with a different user_id, then
call device_capability_sync_service.resolve_payload with installed_plugin_ids
containing that other/nonexistent ID and assert it raises
DeviceCapabilityResolutionError with a 404 status; use the same test DB setup,
User(...) and reference device_capability_sync_service.resolve_payload and
DeviceCapabilityResolutionError to locate where to add the assertion.
In `@backend/tests/services/test_cloud_device_provider.py`:
- Around line 43-68: Add a companion async test in the same test module that
verifies CloudDeviceProvider.get_status preserves the bind_shell value: create
cloud devices in test_db (e.g., using
_cloud_device("cloud-claude","claudecode")), monkeypatch cache.mget and
executor_version_service.get_latest_version like the existing test, call await
CloudDeviceProvider().get_status(test_db, device_id="cloud-claude", user_id=7)
and assert the returned status dict contains "bind_shell": "claudecode"; ensure
you cover at least one cloud device and mirror the setup used in
test_list_devices_preserves_cloud_device_bind_shell so response shapes don’t
drift.
In `@wework/src/components/layout/DesktopSidebar.tsx`:
- Around line 503-509: The openProjectCreateDialog function currently uses
try/finally so onRefreshDevices failures are silent; add a catch to handle
errors from onRefreshDevices and surface them (e.g., call
processLogger/errorLogger or show a user toast) before continuing to
setProjectCreateMode. Specifically, wrap await onRefreshDevices?.() with try {
await onRefreshDevices?.() } catch (err) { /* log via your logger or call a
notification/toast API with err.message */ } finally {
setProjectCreateMode(mode) } so failures are logged and optionally displayed to
the user while preserving the dialog opening 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
Run ID: 29947f2d-90ea-4a13-8a75-ef397600b8d7
📒 Files selected for processing (25)
backend/app/api/endpoints/local_executor.pybackend/app/api/ws/device_namespace.pybackend/app/schemas/device.pybackend/app/services/device/capability_sync_service.pybackend/app/services/device/cloud_provider.pybackend/tests/api/ws/test_device_capabilities_state.pybackend/tests/services/device/test_capability_sync_service.pybackend/tests/services/test_cloud_device_provider.pybackend/tests/services/test_device_capability_sync_service.pydocs/en/developer-guide/local-device-architecture.mddocs/zh/developer-guide/local-device-architecture.mdexecutor/agents/claude_code/local_mode_strategy.pyexecutor/modes/local/capabilities.pyexecutor/tests/modes/local/test_global_capabilities.pyexecutor/tests/test_local_capability_sync_handler.pywework/src/components/layout/DesktopSidebar.tsxwework/src/components/layout/DesktopWorkbenchLayout.tsxwework/src/components/settings/ConnectionsSettingsPage.test.tsxwework/src/features/workbench/WorkbenchProvider.test.tsxwework/src/features/workbench/WorkbenchProvider.tsxwework/src/features/workbench/workbenchReducer.tswework/src/pages/PluginManagementPage.tsxwework/src/pages/PluginsPage.tsxwework/src/pages/WorkbenchPage.tsxwework/src/stream/chatStream.ts
Comment on lines
+711
to
+725
| def _should_include_installed_plugin(self, user_id: int, row: Kind) -> bool: | ||
| spec = row.json.get("spec", {}) | ||
| include = row.is_active and self._is_enabled_install(row) | ||
| logger.info( | ||
| "Desired-state InstalledPlugin candidate: user_id=%s installed_id=%s name=%s active=%s enabled=%s state=%s source=%s include=%s", | ||
| user_id, | ||
| row.id, | ||
| row.name, | ||
| row.is_active, | ||
| spec.get("enabled", True), | ||
| spec.get("installState", "installed"), | ||
| spec.get("source"), | ||
| include, | ||
| ) | ||
| return include |
Contributor
There was a problem hiding this comment.
Avoid logging raw plugin source metadata.
spec["source"] is installation metadata and can carry private URLs or credentials for custom plugin sources. Logging the whole dict at info level leaks that data into backend logs; log stable identifiers such as marketplace/name/state instead.
🛡️ Proposed fix
def _should_include_installed_plugin(self, user_id: int, row: Kind) -> bool:
spec = row.json.get("spec", {})
include = row.is_active and self._is_enabled_install(row)
logger.info(
- "Desired-state InstalledPlugin candidate: user_id=%s installed_id=%s name=%s active=%s enabled=%s state=%s source=%s include=%s",
+ "Desired-state InstalledPlugin candidate: user_id=%s installed_id=%s name=%s active=%s enabled=%s state=%s marketplace=%s include=%s",
user_id,
row.id,
row.name,
row.is_active,
spec.get("enabled", True),
spec.get("installState", "installed"),
- spec.get("source"),
+ spec.get("marketplace")
+ or (spec.get("source") or {}).get("marketplace"),
include,
)
return includeAs per coding guidelines, backend/**/*.py: Backend encrypts Git tokens and API keys using AES-256-CBC.
📝 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
| def _should_include_installed_plugin(self, user_id: int, row: Kind) -> bool: | |
| spec = row.json.get("spec", {}) | |
| include = row.is_active and self._is_enabled_install(row) | |
| logger.info( | |
| "Desired-state InstalledPlugin candidate: user_id=%s installed_id=%s name=%s active=%s enabled=%s state=%s source=%s include=%s", | |
| user_id, | |
| row.id, | |
| row.name, | |
| row.is_active, | |
| spec.get("enabled", True), | |
| spec.get("installState", "installed"), | |
| spec.get("source"), | |
| include, | |
| ) | |
| return include | |
| def _should_include_installed_plugin(self, user_id: int, row: Kind) -> bool: | |
| spec = row.json.get("spec", {}) | |
| include = row.is_active and self._is_enabled_install(row) | |
| logger.info( | |
| "Desired-state InstalledPlugin candidate: user_id=%s installed_id=%s name=%s active=%s enabled=%s state=%s marketplace=%s include=%s", | |
| user_id, | |
| row.id, | |
| row.name, | |
| row.is_active, | |
| spec.get("enabled", True), | |
| spec.get("installState", "installed"), | |
| spec.get("marketplace") | |
| or (spec.get("source") or {}).get("marketplace"), | |
| include, | |
| ) | |
| return include |
🤖 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 `@backend/app/services/device/capability_sync_service.py` around lines 711 -
725, The _should_include_installed_plugin function is logging raw installation
metadata (spec["source"]) which may contain private URLs/credentials; remove or
sanitize that field in the logger call and instead log stable, non-sensitive
identifiers (e.g. spec.get("marketplace"), row.name,
spec.get("installState")/spec.get("state") or a normalized status). Update the
logger.info in _should_include_installed_plugin to omit spec.get("source") (or
replace it with a redacted marker) and include only
marketplace/name/state/enable flags to avoid leaking sensitive plugin source
data.
Comment on lines
+233
to
+240
| if task_capability_dir.exists() or task_capability_dir.is_symlink(): | ||
| if task_capability_dir.is_symlink(): | ||
| task_capability_dir.unlink() | ||
| else: | ||
| shutil.rmtree(task_skills_dir) | ||
| shutil.copytree(global_skills_dir, task_skills_dir, dirs_exist_ok=True) | ||
| shutil.rmtree(task_capability_dir) | ||
| shutil.copytree( | ||
| global_capability_dir, task_capability_dir, dirs_exist_ok=True | ||
| ) |
Contributor
There was a problem hiding this comment.
Reapply owner-only permissions after the copy fallback.
When symlink creation fails, the fallback copytree() path never tightens permissions on the copied skills/ or plugins/ tree. On systems where symlinks are unavailable, that leaves task capability code with default filesystem permissions instead of the owner-only policy used elsewhere.
Suggested fix
shutil.copytree(
global_capability_dir, task_capability_dir, dirs_exist_ok=True
)
+ permissions_manager.set_owner_only(
+ str(task_capability_dir), is_directory=True
+ )🤖 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 `@executor/agents/claude_code/local_mode_strategy.py` around lines 233 - 240,
After the fallback copytree path, reapply the owner-only permissions to the
copied capability tree: after the shutil.copytree(global_capability_dir,
task_capability_dir, dirs_exist_ok=True) call, walk task_capability_dir and set
file mode/ownership to the same owner-only policy used elsewhere (e.g., chmod
0700 on dirs and 0600 on files or call the existing helper that enforces
owner-only perms), ensuring the code that creates/removes symlinks
(task_capability_dir.is_symlink()/unlink()) and variables task_capability_dir
and global_capability_dir are preserved and the permission enforcement runs only
when the symlink fallback was used.
Comment on lines
+256
to
+277
| def remove_stale_managed_plugins(self, desired_keys: set[str]) -> list[str]: | ||
| with _file_lock(self._lock_path): | ||
| manifest = self.manifest.load() | ||
| installed_plugins = self._load_installed_plugins() | ||
| installed_map = installed_plugins.setdefault("plugins", {}) | ||
| removed = [] | ||
| changed = False | ||
| for key, record in list((manifest.get("plugins") or {}).items()): | ||
| managed = ( | ||
| record.get("managed", True) if isinstance(record, dict) else False | ||
| ) | ||
| if key in desired_keys or not managed: | ||
| continue | ||
| if key in installed_map: | ||
| installed_map.pop(key, None) | ||
| removed.append(key) | ||
| changed = True | ||
| manifest.setdefault("plugins", {}).pop(key, None) | ||
| changed = True | ||
| if changed: | ||
| self._save_installed_plugins(installed_plugins) | ||
| self.manifest.save(self.manifest.bump_revision(manifest)) |
Contributor
There was a problem hiding this comment.
Preserve local installs when a managed plugin shares the same key.
remove_stale_managed_plugins() deletes the entire installed_map[key] bucket for stale managed plugins. If a user-installed copy of the same <name>@<marketplace> exists, replace mode removes that too, so local-user plugins are not actually preserved for key collisions. Persist ownership per install record and strip only the managed entries here.
Comment on lines
+723
to
+792
| installed_plugins = self.store._load_installed_plugins() | ||
| installed_map = installed_plugins.setdefault("plugins", {}) | ||
| self.plugins_dir.mkdir(parents=True, exist_ok=True) | ||
| _set_owner_only(self.plugins_dir, is_directory=True) | ||
|
|
||
| for item in plugins: | ||
| if not isinstance(item, dict): | ||
| logger.warning( | ||
| "Ignoring invalid Plugin entry in capability sync: %s", item | ||
| ) | ||
| continue | ||
| name = item.get("name") | ||
| marketplace = self._plugin_marketplace(item) | ||
| if not name: | ||
| results.append( | ||
| {"name": None, "status": "failed", "error": "Invalid Plugin entry"} | ||
| ) | ||
| continue | ||
| key = self._plugin_key(name, marketplace) | ||
| install_path = self._plugin_install_path(item, name, marketplace) | ||
| if not install_path.exists() or not install_path.is_dir(): | ||
| logger.warning( | ||
| "Plugin package not found in local cache: name=%s marketplace=%s path=%s", | ||
| name, | ||
| marketplace, | ||
| install_path, | ||
| ) | ||
| results.append( | ||
| { | ||
| "id": item.get("installed_plugin_id"), | ||
| "name": name, | ||
| "status": "failed", | ||
| "error": "Plugin package not found in local cache", | ||
| } | ||
| ) | ||
| continue | ||
|
|
||
| scope = item.get("scope", "user") | ||
| existing_installs = [ | ||
| install | ||
| for install in installed_map.get(key, []) | ||
| if isinstance(install, dict) and install.get("scope", "user") != scope | ||
| ] | ||
| existing_installs.append( | ||
| { | ||
| "scope": scope, | ||
| "installPath": str(install_path), | ||
| "version": item.get("version"), | ||
| "installedAt": item.get("installed_at") or utc_now_iso(), | ||
| "lastUpdated": utc_now_iso(), | ||
| } | ||
| ) | ||
| installed_map[key] = existing_installs | ||
| records[key] = self._plugin_record(item, key, marketplace, install_path) | ||
| logger.info( | ||
| "Configured global plugin: key=%s installed_plugin_id=%s path=%s", | ||
| key, | ||
| item.get("installed_plugin_id"), | ||
| install_path, | ||
| ) | ||
| results.append( | ||
| { | ||
| "id": item.get("installed_plugin_id"), | ||
| "name": name, | ||
| "status": "synced", | ||
| } | ||
| ) | ||
|
|
||
| if records: | ||
| self.store._save_installed_plugins(installed_plugins) |
Contributor
There was a problem hiding this comment.
Lock the plugin cache read-modify-write.
_sync_plugins() updates installed_plugins.json outside _file_lock, so concurrent syncs can lose entries when both callers load the same old file and the later save wins. Guard the whole load/update/save block with the same store lock, or a dedicated lock for the plugin cache.
Suggested direction
def _sync_plugins(
self, plugins: list[dict[str, Any]]
) -> tuple[list[dict[str, Any]], dict[str, dict[str, Any]]]:
results: list[dict[str, Any]] = []
records: dict[str, dict[str, Any]] = {}
if not plugins:
logger.info("No plugins requested in capability sync")
return results, records
- installed_plugins = self.store._load_installed_plugins()
- installed_map = installed_plugins.setdefault("plugins", {})
- self.plugins_dir.mkdir(parents=True, exist_ok=True)
- _set_owner_only(self.plugins_dir, is_directory=True)
-
- for item in plugins:
- ...
-
- if records:
- self.store._save_installed_plugins(installed_plugins)
+ with _file_lock(self.store._lock_path):
+ installed_plugins = self.store._load_installed_plugins()
+ installed_map = installed_plugins.setdefault("plugins", {})
+ self.plugins_dir.mkdir(parents=True, exist_ok=True)
+ _set_owner_only(self.plugins_dir, is_directory=True)
+
+ for item in plugins:
+ ...
+
+ if records:
+ self.store._save_installed_plugins(installed_plugins)
return results, records🤖 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 `@executor/modes/local/capabilities.py` around lines 723 - 792, _sync_plugins
currently calls self.store._load_installed_plugins() and later
self.store._save_installed_plugins() without holding the store lock, which
allows races; wrap the entire load-modify-save sequence in the same lock used by
the store (or a dedicated plugin cache lock) so the call to
self.store._load_installed_plugins(), the mutations to
installed_plugins/installed_map/records, and the subsequent
self.store._save_installed_plugins(...) are executed atomically; locate this
logic in _sync_plugins and use the store's existing lock object/method (or add
one) to acquire before load and release after save.
Comment on lines
+272
to
+283
| handlers.onDeviceOnline?.({ | ||
| device_id: 'c78d176b-3f32-4598-9758-cb8262d8f25a', | ||
| name: 'macOS-Device-cb8262d8f25a', | ||
| status: 'online', | ||
| }) | ||
|
|
||
| await waitFor(() => | ||
| expect(screen.getByTestId('device-list')).toHaveTextContent( | ||
| 'macOS-Device-cb8262d8f25a' | ||
| ) | ||
| ) | ||
| expect(listDevices).toHaveBeenCalledTimes(2) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="wework/src/features/workbench/WorkbenchProvider.test.tsx"
# Basic sanity: confirm file exists and show the relevant region with line numbers
ls -la "$FILE"
sed -n '240,320p' "$FILE" | nl -ba
# Search for act usage in the file
rg -n "(\bact\b|waitFor\(|render\()" "$FILE" || true
# Show how handlers is obtained / mocked in the test(s) around this snippet
# We'll grab a bit more context above the snippet
sed -n '160,260p' "$FILE" | nl -ba
# Find the test name(s) containing onDeviceOnline
rg -n "onDeviceOnline" "$FILE" -nRepository: wecode-ai/Wegent
Length of output: 228
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="wework/src/features/workbench/WorkbenchProvider.test.tsx"
ls -la "$FILE"
echo "---- Lines 240-320 (with line numbers) ----"
sed -n '240,320p' "$FILE" | cat -n
echo "---- Search for act/render/waitFor usage ----"
rg -n "(\\bact\\b|waitFor\\(|render\\()" "$FILE" || true
echo "---- Context lines 160-260 (with line numbers) ----"
sed -n '160,260p' "$FILE" | cat -n
echo "---- Search for onDeviceOnline usage (with line numbers) ----"
rg -n "onDeviceOnline" "$FILE" || trueRepository: wecode-ai/Wegent
Length of output: 8203
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="wework/src/features/workbench/WorkbenchProvider.test.tsx"
echo "---- Imports (top of file) ----"
sed -n '1,80p' "$FILE" | cat -n
echo "---- Lines around test (250-310) ----"
sed -n '250,310p' "$FILE" | cat -nRepository: wecode-ai/Wegent
Length of output: 5391
Wrap the manual handlers.onDeviceOnline callback in act(...)
This callback triggers React state updates outside Testing Library’s act, which can lead to act(...) warnings and occasional timing flakiness.
💡 Suggested fix
- import { render, screen, waitFor } from '`@testing-library/react`'
+ import { act, render, screen, waitFor } from '`@testing-library/react`'
...
- handlers.onDeviceOnline?.({
- device_id: 'c78d176b-3f32-4598-9758-cb8262d8f25a',
- name: 'macOS-Device-cb8262d8f25a',
- status: 'online',
- })
+ await act(async () => {
+ handlers.onDeviceOnline?.({
+ device_id: 'c78d176b-3f32-4598-9758-cb8262d8f25a',
+ name: 'macOS-Device-cb8262d8f25a',
+ status: 'online',
+ })
+ })📝 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
| handlers.onDeviceOnline?.({ | |
| device_id: 'c78d176b-3f32-4598-9758-cb8262d8f25a', | |
| name: 'macOS-Device-cb8262d8f25a', | |
| status: 'online', | |
| }) | |
| await waitFor(() => | |
| expect(screen.getByTestId('device-list')).toHaveTextContent( | |
| 'macOS-Device-cb8262d8f25a' | |
| ) | |
| ) | |
| expect(listDevices).toHaveBeenCalledTimes(2) | |
| await act(async () => { | |
| handlers.onDeviceOnline?.({ | |
| device_id: 'c78d176b-3f32-4598-9758-cb8262d8f25a', | |
| name: 'macOS-Device-cb8262d8f25a', | |
| status: 'online', | |
| }) | |
| }) | |
| await waitFor(() => | |
| expect(screen.getByTestId('device-list')).toHaveTextContent( | |
| 'macOS-Device-cb8262d8f25a' | |
| ) | |
| ) | |
| expect(listDevices).toHaveBeenCalledTimes(2) |
🤖 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 `@wework/src/features/workbench/WorkbenchProvider.test.tsx` around lines 272 -
283, The test invokes handlers.onDeviceOnline which triggers React state updates
outside of Testing Library’s act, so wrap the call in act(...) (e.g., use act(()
=> handlers.onDeviceOnline({...}))) before awaiting waitFor and assertions;
ensure you still await/wrap any async assertions (waitFor +
expect(screen.getByTestId('device-list'))...) and keep the listDevices assertion
(expect(listDevices).toHaveBeenCalledTimes(2)) after the act/waitFor sequence so
all state updates are flushed.
Comment on lines
+315
to
+325
| const refreshDevices = useCallback(async () => { | ||
| const devices = await resolvedServices.deviceApi.listDevices() | ||
| dispatch({ | ||
| type: 'devices_refreshed', | ||
| devices, | ||
| standaloneDeviceId: getPreferredStandaloneDeviceId( | ||
| devices, | ||
| state.standaloneDeviceId | ||
| ), | ||
| }) | ||
| }, [resolvedServices, state.standaloneDeviceId]) |
Contributor
There was a problem hiding this comment.
Prevent older listDevices() responses from overwriting fresher device state.
These refreshes can overlap with each other and with the bootstrap fetch. Right now every completion dispatches unconditionally, so a slower older request can win last and roll the device list back to a stale snapshot.
💡 Suggested direction
+ const latestDeviceRefreshId = useRef(0)
+
const refreshDevices = useCallback(async () => {
+ const refreshId = ++latestDeviceRefreshId.current
const devices = await resolvedServices.deviceApi.listDevices()
+ if (refreshId !== latestDeviceRefreshId.current) return
dispatch({
type: 'devices_refreshed',
devices,
standaloneDeviceId: getPreferredStandaloneDeviceId(
devices,
state.standaloneDeviceId
),
})
}, [resolvedServices, state.standaloneDeviceId])Use the same guard for the bootstrap path if it can also race with live refreshes.
Also applies to: 327-335
🤖 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 `@wework/src/features/workbench/WorkbenchProvider.tsx` around lines 315 - 325,
The refreshDevices callback can race so an older
resolvedServices.deviceApi.listDevices() response may overwrite newer state; fix
by adding a monotonically increasing requestId/sequence (or reuse an
AbortController) stored in component scope and captured by refreshDevices and
the bootstrap fetch, incrementing it before each fetch and only dispatching when
the captured id equals the current latest id; update both refreshDevices and the
bootstrap device fetch logic (references: refreshDevices,
resolvedServices.deviceApi.listDevices, devices_refreshed dispatch, and the
bootstrap fetch path mentioned in the comment) to perform this check before
calling dispatch so stale responses are ignored.
Comment on lines
+328
to
+330
| const handleDeviceChanged = () => { | ||
| void refreshDevices() | ||
| } |
Contributor
There was a problem hiding this comment.
Catch rejected background refreshes from socket events.
handleDeviceChanged fires refreshDevices() and drops the promise. If listDevices() rejects during reconnect or a transient API failure, this turns into an unhandled rejection from the socket callback path.
💡 Suggested fix
const handleDeviceChanged = () => {
- void refreshDevices()
+ void refreshDevices().catch(() => {
+ // Keep the current device list until the next successful refresh.
+ })
}🤖 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 `@wework/src/features/workbench/WorkbenchProvider.tsx` around lines 328 - 330,
handleDeviceChanged currently calls refreshDevices() and drops its promise,
risking unhandled rejections when listDevices() fails; update
handleDeviceChanged to await or attach a rejection handler to the
refreshDevices() call so any error from refreshDevices()/listDevices() is caught
and logged/handled (e.g., call refreshDevices().catch(err => process/logging) or
make handleDeviceChanged async and use try/catch around await refreshDevices()).
Ensure you update the handler that references refreshDevices and the underlying
listDevices flow to prevent unhandled promise rejections.
This was referenced Jun 2, 2026
Merged
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
2 participants
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
Documentation
Tests