Repository navigation
Codex/fix/workbench device workspaces - #1524
Merged
qdaxb merged 8 commits intoJun 23, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds Claude Code device filtering to the API and session-storage cache, introduces optimistic ChangesBackend: local device type normalization and streaming tool block test
Executor: Responses API event streaming and Claude tool-argument streaming
Frontend: Claude device filtering and optimistic project/workspace mutations
Sequence Diagram(s)sequenceDiagram
participant Dialog as ProjectCreateDialog
participant Provider as WorkbenchProvider
participant Reducer as workbenchReducer
participant API as Backend API
Dialog->>Provider: onCreateProject(data, { refreshWorkLists: false })
Provider->>API: POST /projects
API-->>Provider: ProjectWithTasks
Provider->>Reducer: dispatch({ type: 'project_created', project })
Reducer-->>Provider: updated projects list (optimistic)
Dialog->>Provider: onPrepareDeviceWorkspace(data, { refreshWorkLists: false })
Provider->>API: POST /device-workspaces/prepare
API-->>Provider: DeviceWorkspaceResponse
Provider->>Reducer: dispatch({ type: 'device_workspace_prepared', mapping })
Reducer-->>Provider: updated runtimeWork + selectedDeviceWorkspaceId
Provider->>Provider: refreshWorkLists() in background (swallows failures)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wework/src/features/workbench/WorkbenchProvider.tsx (1)
191-204:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid serving stale cached devices when the live fetch succeeds with zero Claude devices.
On Line 198, cache fallback runs whenever
claudeCodeDevicesis empty. Since the API now pre-filters to Claude devices, a successful/devicesresponse can legitimately be empty; this path then revives stale cache and hides the real live state.💡 Suggested fix
-function resolveDeviceListWithCache(devices: DeviceInfo[]): DeviceInfo[] { +function resolveDeviceListWithCache( + devices: DeviceInfo[], + allowCachedFallback: boolean +): DeviceInfo[] { const claudeCodeDevices = filterClaudeCodeDevices(devices) if (claudeCodeDevices.length > 0) { writeCachedDeviceList(claudeCodeDevices) return claudeCodeDevices } - const cachedDevices = readCachedDeviceList() - if (cachedDevices.length > 0) { - return cachedDevices + if (allowCachedFallback) { + const cachedDevices = readCachedDeviceList() + if (cachedDevices.length > 0) { + return cachedDevices + } } - return devices + return claudeCodeDevices }And pass
allowCachedFallbackonly when the network read failed (e.g., bootstrap rejected path), not on successful empty responses.🤖 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 191 - 204, The resolveDeviceListWithCache function cannot distinguish between a successful empty response (which is now legitimate since the API pre-filters to Claude devices) and a failed network call, causing it to incorrectly resurrect stale cached devices on successful empty responses. Add a parameter to resolveDeviceListWithCache (e.g., allowCachedFallback) that indicates whether the network fetch succeeded or failed, and only use the cached device fallback when allowCachedFallback is true and claudeCodeDevices is empty, ensuring that legitimate empty live responses are not masked by stale cache.
🧹 Nitpick comments (7)
backend/tests/services/test_local_device_provider.py (1)
40-53: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd explicit type hints to new async test functions.
The new test function and nested async helpers are missing parameter/return type hints. Please annotate them to match the Python typing rule used in this repo.
Suggested patch
import pytest +from typing import Any from app.models.kind import Kind from app.schemas.device import DeviceInfo from app.services.device.local_provider import LocalDeviceProvider @@ `@pytest.mark.asyncio` async def test_list_devices_normalizes_legacy_remote_device_type( - test_db, - monkeypatch, -): + test_db: Any, + monkeypatch: pytest.MonkeyPatch, +) -> None: @@ - async def fake_mget(keys): - return {} + async def fake_mget(keys: list[str]) -> dict[str, Any]: + return {} - async def fake_latest_version(): + async def fake_latest_version() -> str: return "1.0.0"As per coding guidelines,
**/*.py: “Use type hints for all function parameters and return values”.🤖 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_local_device_provider.py` around lines 40 - 53, The test function test_list_devices_normalizes_legacy_remote_device_type and its nested async helper functions fake_mget and fake_latest_version are missing type hints for their parameters and return values. Add explicit type annotations to all parameters and return types for these functions following the repository's Python typing conventions. This includes annotating the test function parameters (test_db and monkeypatch), the fake_mget parameter (keys) and return type, and the fake_latest_version return type to comply with the coding guidelines that require type hints for all function parameters and return values.Source: Coding guidelines
backend/tests/api/ws/test_device_capabilities_state.py (1)
398-403: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winType-annotate the new async test and helper function.
Please add explicit parameter and return annotations for the newly introduced test and nested helper.
Suggested patch
`@pytest.mark.asyncio` -async def test_local_task_streaming_tool_arguments_emit_generating_block(monkeypatch): +async def test_local_task_streaming_tool_arguments_emit_generating_block( + monkeypatch: pytest.MonkeyPatch, +) -> None: @@ - async def fake_get_session(sid): + async def fake_get_session(sid: str) -> dict[str, object]: return {"user_id": 7, "device_id": "device-1"}As per coding guidelines,
**/*.py: “Use type hints for all function parameters and return values”.🤖 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 398 - 403, The newly introduced test function test_local_task_streaming_tool_arguments_emit_generating_block and its nested async helper function fake_get_session are missing explicit type annotations. Add type hint annotations to both functions: the test function should have a return type of None, and the fake_get_session helper should have the sid parameter typed (as str) and include a return type annotation for the dictionary it returns. This aligns with the project coding guideline requiring type hints for all function parameters and return values.Source: Coding guidelines
executor/runtime_work/agent_adapter.py (1)
33-40: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd explicit
-> Noneon updated constructors.The updated
__init__signatures should include explicit return annotations for typing consistency.Suggested fix
- def __init__( + def __init__( self, store: LocalTaskStore, local_task_id: str, *, runtime: str, emit_event: Optional[Callable[[str, dict[str, Any]], Any]] = None, - ): + ) -> None: @@ - def __init__( + def __init__( self, *, runtime: str, store: LocalTaskStore, execute_agent: Optional[ExecuteAgent] = None, run_background: bool = True, responses_event_emitter: Optional[Callable[[str, dict[str, Any]], Any]] = None, - ): + ) -> None:As per coding guidelines,
**/*.py: “Use type hints for all function parameters and return values.”Also applies to: 212-220
🤖 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/runtime_work/agent_adapter.py` around lines 33 - 40, Add explicit `-> None` return type annotations to the `__init__` constructor methods in the AgentAdapter class. For the `__init__` method shown in the diff (with parameters store, local_task_id, runtime, and emit_event), add `-> None` after the closing parenthesis and before the colon. Apply the same change to the other `__init__` method mentioned at lines 212-220 to ensure all constructors have consistent typing that aligns with the coding guidelines requiring type hints for all function return values.Source: Coding guidelines
executor/tests/runtime_work/test_local_task_store.py (1)
2238-2241: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winType-annotate the new async test functions.
The new test and nested async stub omit parameter/return annotations required by repo typing rules.
Suggested fix
-async def test_runtime_work_handler_streams_claude_runtime_task_events( - tmp_path, - monkeypatch, -): +async def test_runtime_work_handler_streams_claude_runtime_task_events( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: @@ - async def execute_agent(self, request, emitter): + async def execute_agent( + self: RuntimeAgentAdapter, request: Any, emitter: Any + ) -> None: await emitter.start(shell_type="ClaudeCode") await emitter.text_delta("Created") await emitter.done(content="Created")As per coding guidelines,
**/*.py: “Use type hints for all function parameters and return values.”Also applies to: 2246-2249
🤖 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/tests/runtime_work/test_local_task_store.py` around lines 2238 - 2241, Add type annotations to the test function test_runtime_work_handler_streams_claude_runtime_task_events and its parameters. Annotate the tmp_path parameter with the appropriate pytest fixture type, the monkeypatch parameter with the MonkeyPatch type from pytest, and add a return type annotation of None to the async function. Also apply the same type annotation requirements to the nested async stub function mentioned in lines 2246-2249, ensuring all parameters and return types are properly annotated according to the repository's typing standards.Source: Coding guidelines
executor/agents/claude_code/response_processor.py (1)
983-989: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider using simpler
getattrpattern for attribute access.The current pattern
getattr(emitter, "__dict__", {}).get(ATTR)works but is unnecessarily indirect. The idiomatic Python approach would be:state = getattr(emitter, CLAUDE_STREAM_TOOL_STATE_ATTR, None)This applies to
_stream_tool_state,_stream_tool_started, and_stream_tool_finalizedhelpers.♻️ Suggested simplification
def _stream_tool_state(emitter: ResponsesAPIEmitter) -> dict[str, dict[str, Any]]: - state = getattr(emitter, "__dict__", {}).get(CLAUDE_STREAM_TOOL_STATE_ATTR) + state = getattr(emitter, CLAUDE_STREAM_TOOL_STATE_ATTR, None) if isinstance(state, dict): return state state = {} setattr(emitter, CLAUDE_STREAM_TOOL_STATE_ATTR, state) return state🤖 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/response_processor.py` around lines 983 - 989, Replace the indirect attribute access pattern in the three helper functions _stream_tool_state, _stream_tool_started, and _stream_tool_finalized that currently use getattr(emitter, "__dict__", {}).get(ATTR) with the simpler idiomatic Python approach using getattr(emitter, ATTR, None) or getattr(emitter, ATTR, {}) depending on whether a None or empty dict default makes sense for that function. This makes the code more readable and follows Python conventions for attribute access with fallback defaults.wework/src/components/layout/DesktopWorkbenchLayout.test.tsx (1)
1516-1528: 🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy liftSplit this oversized test file before it grows further.
The changed assertions are already past Line 1500, so this test module remains well over the repository’s 1000-line limit. Please move the workbench project-creation/workspace-selection coverage into a focused test file.
As per coding guidelines, “Files should not exceed 1000 lines; split large files into multiple sub-modules for better maintainability.”
🤖 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/DesktopWorkbenchLayout.test.tsx` around lines 1516 - 1528, The DesktopWorkbenchLayout.test.tsx file exceeds the 1000-line limit and needs to be split. Identify all test cases in DesktopWorkbenchLayout.test.tsx that cover workbench project-creation and workspace-selection functionality (including the test containing the onPrepareDeviceWorkspace assertions around line 1516), extract those tests into a new, focused test file dedicated to workspace and project selection scenarios, and update any shared test utilities or mock dependencies to ensure both the original and new test files can access them properly.Source: Coding guidelines
wework/src/features/workbench/workbenchReducer.test.ts (1)
116-159: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winCover local task preservation in this optimistic move test.
This fixture only uses
localTasks: [], so the reducer can pass while dropping existing runtime tasks from the promoted unmapped workspace. Seed one local task and assert it remains under the mapped workspace.Suggested test strengthening
unmappedDeviceWorkspaces: [ { deviceId: 'device-1', workspacePath: '/workspace/repo', available: true, mapped: false, - localTasks: [], + localTasks: [ + { + localTaskId: 'runtime-1', + workspacePath: '/workspace/repo', + title: 'Existing local task', + runtime: 'codex', + }, + ], }, ], @@ deviceName: 'Device 1', workspacePath: '/workspace/repo', mapped: true, + localTasks: [ + expect.objectContaining({ + localTaskId: 'runtime-1', + }), + ], }), ])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 `@wework/src/features/workbench/workbenchReducer.test.ts` around lines 116 - 159, The test for the device_workspace_prepared action only uses empty localTasks arrays, which doesn't verify that local tasks are properly preserved when promoting an unmapped device workspace to a mapped one. In the fixture setup where unmappedDeviceWorkspaces is initialized, add a sample local task to the localTasks array instead of leaving it empty, and then add an assertion after the reducer call to verify that the mapped workspace in updated.runtimeWork?.projects[0].deviceWorkspaces contains the expected local tasks that were preserved from the unmapped workspace.Source: Coding guidelines
🤖 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/tests/services/test_local_device_provider.py`:
- Around line 63-64: The test code accesses devices[0] without first verifying
that the devices list contains any elements, which can result in an IndexError
that masks the actual contract failure being tested. Before the assertions that
access devices[0]["device_type"] and the DeviceInfo(**devices[0]) call, add an
explicit assertion to ensure the devices list is not empty, such as checking
that its length is greater than zero or that it is truthy.
In `@executor/runtime_work/agent_adapter.py`:
- Around line 154-174: The _forward_event() method does not handle exceptions
that may be raised by the emit_event callback, which can cause the runtime flow
to be interrupted and prevent _mark_not_running() from being executed. Wrap the
self.emit_event(event_type, payload) call and the subsequent coroutine handling
(the asyncio.iscoroutine check and await) in a try-except block to gracefully
catch and log any exceptions without allowing them to bubble up and disrupt the
task's running state management.
In `@wework/src/features/workbench/workbenchReducer.ts`:
- Around line 128-167: When promoting a workspace from unmapped to mapped state,
the existing local tasks are lost because nextWorkspace is created with empty
localTasks. Find the matching unmapped workspace in
currentRuntimeWork.unmappedDeviceWorkspaces before it gets filtered out
(matching by deviceId and workspacePath), extract its localTasks property, and
ensure those local tasks are preserved when creating nextWorkspace via
runtimeWorkspaceFromMapping instead of starting with an empty array, so the
optimistic state retains the existing local tasks until a refresh completes.
---
Outside diff comments:
In `@wework/src/features/workbench/WorkbenchProvider.tsx`:
- Around line 191-204: The resolveDeviceListWithCache function cannot
distinguish between a successful empty response (which is now legitimate since
the API pre-filters to Claude devices) and a failed network call, causing it to
incorrectly resurrect stale cached devices on successful empty responses. Add a
parameter to resolveDeviceListWithCache (e.g., allowCachedFallback) that
indicates whether the network fetch succeeded or failed, and only use the cached
device fallback when allowCachedFallback is true and claudeCodeDevices is empty,
ensuring that legitimate empty live responses are not masked by stale cache.
---
Nitpick comments:
In `@backend/tests/api/ws/test_device_capabilities_state.py`:
- Around line 398-403: The newly introduced test function
test_local_task_streaming_tool_arguments_emit_generating_block and its nested
async helper function fake_get_session are missing explicit type annotations.
Add type hint annotations to both functions: the test function should have a
return type of None, and the fake_get_session helper should have the sid
parameter typed (as str) and include a return type annotation for the dictionary
it returns. This aligns with the project coding guideline requiring type hints
for all function parameters and return values.
In `@backend/tests/services/test_local_device_provider.py`:
- Around line 40-53: The test function
test_list_devices_normalizes_legacy_remote_device_type and its nested async
helper functions fake_mget and fake_latest_version are missing type hints for
their parameters and return values. Add explicit type annotations to all
parameters and return types for these functions following the repository's
Python typing conventions. This includes annotating the test function parameters
(test_db and monkeypatch), the fake_mget parameter (keys) and return type, and
the fake_latest_version return type to comply with the coding guidelines that
require type hints for all function parameters and return values.
In `@executor/agents/claude_code/response_processor.py`:
- Around line 983-989: Replace the indirect attribute access pattern in the
three helper functions _stream_tool_state, _stream_tool_started, and
_stream_tool_finalized that currently use getattr(emitter, "__dict__",
{}).get(ATTR) with the simpler idiomatic Python approach using getattr(emitter,
ATTR, None) or getattr(emitter, ATTR, {}) depending on whether a None or empty
dict default makes sense for that function. This makes the code more readable
and follows Python conventions for attribute access with fallback defaults.
In `@executor/runtime_work/agent_adapter.py`:
- Around line 33-40: Add explicit `-> None` return type annotations to the
`__init__` constructor methods in the AgentAdapter class. For the `__init__`
method shown in the diff (with parameters store, local_task_id, runtime, and
emit_event), add `-> None` after the closing parenthesis and before the colon.
Apply the same change to the other `__init__` method mentioned at lines 212-220
to ensure all constructors have consistent typing that aligns with the coding
guidelines requiring type hints for all function return values.
In `@executor/tests/runtime_work/test_local_task_store.py`:
- Around line 2238-2241: Add type annotations to the test function
test_runtime_work_handler_streams_claude_runtime_task_events and its parameters.
Annotate the tmp_path parameter with the appropriate pytest fixture type, the
monkeypatch parameter with the MonkeyPatch type from pytest, and add a return
type annotation of None to the async function. Also apply the same type
annotation requirements to the nested async stub function mentioned in lines
2246-2249, ensuring all parameters and return types are properly annotated
according to the repository's typing standards.
In `@wework/src/components/layout/DesktopWorkbenchLayout.test.tsx`:
- Around line 1516-1528: The DesktopWorkbenchLayout.test.tsx file exceeds the
1000-line limit and needs to be split. Identify all test cases in
DesktopWorkbenchLayout.test.tsx that cover workbench project-creation and
workspace-selection functionality (including the test containing the
onPrepareDeviceWorkspace assertions around line 1516), extract those tests into
a new, focused test file dedicated to workspace and project selection scenarios,
and update any shared test utilities or mock dependencies to ensure both the
original and new test files can access them properly.
In `@wework/src/features/workbench/workbenchReducer.test.ts`:
- Around line 116-159: The test for the device_workspace_prepared action only
uses empty localTasks arrays, which doesn't verify that local tasks are properly
preserved when promoting an unmapped device workspace to a mapped one. In the
fixture setup where unmappedDeviceWorkspaces is initialized, add a sample local
task to the localTasks array instead of leaving it empty, and then add an
assertion after the reducer call to verify that the mapped workspace in
updated.runtimeWork?.projects[0].deviceWorkspaces contains the expected local
tasks that were preserved from the unmapped workspace.
🪄 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: 0b9d1a6d-52fa-4e77-b823-077eed544be5
📒 Files selected for processing (19)
backend/app/services/device/local_provider.pybackend/tests/api/ws/test_device_capabilities_state.pybackend/tests/services/test_local_device_provider.pyexecutor/agents/claude_code/response_processor.pyexecutor/runtime_work/agent_adapter.pyexecutor/runtime_work/rpc_handler.pyexecutor/tests/agents/test_claude_response_processor.pyexecutor/tests/runtime_work/test_local_task_store.pyfrontend/public/fonts/SourceHanSansSC-VF.ttf.downloadingfrontend/src/__tests__/features/inbox/QueueEditDialog.test.tsxwework/src/api/devices.test.tswework/src/api/devices.tswework/src/components/layout/DesktopWorkbenchLayout.test.tsxwework/src/components/projects/ProjectCreateDialog.test.tsxwework/src/components/projects/ProjectCreateDialog.tsxwework/src/features/workbench/WorkbenchProvider.tsxwework/src/features/workbench/workbenchReducer.test.tswework/src/features/workbench/workbenchReducer.tswework/src/lib/device-capabilities.ts
Comment on lines
+63
to
+64
| assert devices[0]["device_type"] == "local" | ||
| DeviceInfo(**devices[0]) |
Contributor
There was a problem hiding this comment.
Guard against empty results before indexing devices[0].
This test currently fails with IndexError if filtering/regression returns no devices, which hides the actual contract failure. Add an explicit non-empty assertion first.
Suggested patch
devices = await LocalDeviceProvider().list_devices(test_db, user_id=7)
+ assert len(devices) == 1
assert devices[0]["device_type"] == "local"
DeviceInfo(**devices[0])🤖 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_local_device_provider.py` around lines 63 - 64,
The test code accesses devices[0] without first verifying that the devices list
contains any elements, which can result in an IndexError that masks the actual
contract failure being tested. Before the assertions that access
devices[0]["device_type"] and the DeviceInfo(**devices[0]) call, add an explicit
assertion to ensure the devices list is not empty, such as checking that its
length is greater than zero or that it is truthy.
Comment on lines
+128
to
+167
| const nextWorkspace = runtimeWorkspaceFromMapping(mapping, devices) | ||
| const hasProject = currentRuntimeWork.projects.some(item => item.project.id === mapping.projectId) | ||
| const projectsWithTarget = hasProject | ||
| ? currentRuntimeWork.projects.map(item => { | ||
| if (item.project.id !== mapping.projectId) return item | ||
| const workspaces = item.deviceWorkspaces.filter( | ||
| workspace => | ||
| !( | ||
| workspace.deviceId === mapping.deviceId && | ||
| workspace.workspacePath === mapping.workspacePath | ||
| ) | ||
| ) | ||
| return { | ||
| ...item, | ||
| project: { | ||
| ...item.project, | ||
| ...projectRef, | ||
| }, | ||
| deviceWorkspaces: [...workspaces, nextWorkspace], | ||
| } | ||
| }) | ||
| : ([ | ||
| ...currentRuntimeWork.projects, | ||
| { | ||
| project: projectRef, | ||
| deviceWorkspaces: [nextWorkspace], | ||
| }, | ||
| ] as RuntimeProjectWork[]) | ||
|
|
||
| return { | ||
| ...currentRuntimeWork, | ||
| projects: projectsWithTarget, | ||
| unmappedDeviceWorkspaces: currentRuntimeWork.unmappedDeviceWorkspaces.filter( | ||
| workspace => | ||
| !( | ||
| workspace.deviceId === mapping.deviceId && | ||
| workspace.workspacePath === mapping.workspacePath | ||
| ) | ||
| ), | ||
| } |
Contributor
There was a problem hiding this comment.
Preserve existing local tasks when promoting a workspace.
Line 128 rebuilds nextWorkspace with localTasks: [], then Lines 160-166 remove the matching unmapped workspace. Any existing local tasks for that device/path disappear from optimistic state unless a later refresh succeeds.
Proposed fix
const currentRuntimeWork = runtimeWork ?? {
projects: [],
unmappedDeviceWorkspaces: [],
totalLocalTasks: 0,
}
- const nextWorkspace = runtimeWorkspaceFromMapping(mapping, devices)
+ const matchesMapping = (workspace: RuntimeDeviceWorkspace) =>
+ workspace.deviceId === mapping.deviceId &&
+ workspace.workspacePath === mapping.workspacePath
+ const existingWorkspace = [
+ ...currentRuntimeWork.unmappedDeviceWorkspaces,
+ ...currentRuntimeWork.projects.flatMap(item => item.deviceWorkspaces),
+ ].find(matchesMapping)
+ const nextWorkspace = {
+ ...runtimeWorkspaceFromMapping(mapping, devices),
+ localTasks: existingWorkspace?.localTasks ?? [],
+ }
const hasProject = currentRuntimeWork.projects.some(item => item.project.id === mapping.projectId)
const projectsWithTarget = hasProject
? currentRuntimeWork.projects.map(item => {
if (item.project.id !== mapping.projectId) return item
- const workspaces = item.deviceWorkspaces.filter(
- workspace =>
- !(
- workspace.deviceId === mapping.deviceId &&
- workspace.workspacePath === mapping.workspacePath
- )
- )
+ const workspaces = item.deviceWorkspaces.filter(workspace => !matchesMapping(workspace))
+ const nextDeviceWorkspaces = [...workspaces, nextWorkspace]
return {
...item,
project: {
...item.project,
...projectRef,
},
- deviceWorkspaces: [...workspaces, nextWorkspace],
+ deviceWorkspaces: nextDeviceWorkspaces,
+ totalLocalTasks: nextDeviceWorkspaces.reduce(
+ (total, workspace) => total + workspace.localTasks.length,
+ 0
+ ),
}
})
: ([
...currentRuntimeWork.projects,
{
project: projectRef,
deviceWorkspaces: [nextWorkspace],
+ totalLocalTasks: nextWorkspace.localTasks.length,
},
] as RuntimeProjectWork[])
return {
...currentRuntimeWork,
projects: projectsWithTarget,
unmappedDeviceWorkspaces: currentRuntimeWork.unmappedDeviceWorkspaces.filter(
- workspace =>
- !(
- workspace.deviceId === mapping.deviceId &&
- workspace.workspacePath === mapping.workspacePath
- )
+ workspace => !matchesMapping(workspace)
),
}
}🤖 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/workbenchReducer.ts` around lines 128 - 167,
When promoting a workspace from unmapped to mapped state, the existing local
tasks are lost because nextWorkspace is created with empty localTasks. Find the
matching unmapped workspace in currentRuntimeWork.unmappedDeviceWorkspaces
before it gets filtered out (matching by deviceId and workspacePath), extract
its localTasks property, and ensure those local tasks are preserved when
creating nextWorkspace via runtimeWorkspaceFromMapping instead of starting with
an empty array, so the optimistic state retains the existing local tasks until a
refresh completes.
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 `@executor/runtime_work/agent_adapter.py`:
- Around line 202-205: The event type condition at lines 202-205 only routes
FUNCTION_CALL_ARGUMENTS_DELTA and FUNCTION_CALL_ARGUMENTS_DONE events to
_record_tool_arguments, but it's missing the corresponding
MCP_CALL_ARGUMENTS_DELTA and MCP_CALL_ARGUMENTS_DONE events. Add
MCP_CALL_ARGUMENTS_DELTA and MCP_CALL_ARGUMENTS_DONE to the set of event types
being checked in the condition at lines 202-205. Additionally, locate the status
logic around line 298 that sets "generating_arguments" for function-call deltas
and update it to also handle MCP argument delta events, ensuring mcp_call blocks
receive proper argument updates and correct status transitions.
🪄 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: 10774004-fcb0-40ed-8d00-ae565d385268
📒 Files selected for processing (2)
executor/runtime_work/agent_adapter.pyexecutor/tests/runtime_work/test_local_task_store.py
🚧 Files skipped from review as they are similar to previous changes (1)
- executor/tests/runtime_work/test_local_task_store.py
Comment on lines
+202
to
+205
| if event_type in { | ||
| ResponsesAPIStreamEvents.FUNCTION_CALL_ARGUMENTS_DELTA.value, | ||
| ResponsesAPIStreamEvents.FUNCTION_CALL_ARGUMENTS_DONE.value, | ||
| }: |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the structure and find the agent_adapter.py file
git ls-files | grep -E "agent_adapter|responses_api"Repository: wecode-ai/Wegent
Length of output: 370
🏁 Script executed:
# Read the relevant section of agent_adapter.py around lines 200-310
sed -n '195,310p' executor/runtime_work/agent_adapter.pyRepository: wecode-ai/Wegent
Length of output: 4569
🏁 Script executed:
# Check the ResponsesAPIStreamEvents enum definition
cat shared/models/responses_api.py | head -300Repository: wecode-ai/Wegent
Length of output: 7212
🏁 Script executed:
# Search for the ResponsesAPIStreamEvents enum definition
grep -n "class ResponsesAPIStreamEvents" shared/models/responses_api.pyRepository: wecode-ai/Wegent
Length of output: 201
🏁 Script executed:
# Get the full enum definition
sed -n '/class ResponsesAPIStreamEvents/,/^class /p' shared/models/responses_api.py | head -100Repository: wecode-ai/Wegent
Length of output: 3687
🏁 Script executed:
# Search for MCP_CALL_ARGUMENTS in responses_api.py and related files
grep -r "MCP_CALL_ARGUMENTS" shared/models/Repository: wecode-ai/Wegent
Length of output: 685
🏁 Script executed:
# Check if MCP events are used/referenced anywhere in agent_adapter
grep -n "MCP_CALL_ARGUMENTS\|mcp_call" executor/runtime_work/agent_adapter.pyRepository: wecode-ai/Wegent
Length of output: 308
🏁 Script executed:
# Get lines around 298 to verify the second claim
sed -n '290,310p' executor/runtime_work/agent_adapter.pyRepository: wecode-ai/Wegent
Length of output: 895
Add MCP argument event routing to tool-argument tracking.
Lines 202–205 route only FUNCTION_CALL_ARGUMENTS_* events to _record_tool_arguments, missing MCP_CALL_ARGUMENTS_* events. Similarly, the status logic at line 298 only sets "generating_arguments" for function-call deltas. Since mcp_call blocks are supported (lines 243, 311), they will not receive argument updates or correct status transitions when MCP argument streaming events are emitted.
Suggested fix
if event_type in {
ResponsesAPIStreamEvents.FUNCTION_CALL_ARGUMENTS_DELTA.value,
ResponsesAPIStreamEvents.FUNCTION_CALL_ARGUMENTS_DONE.value,
+ ResponsesAPIStreamEvents.MCP_CALL_ARGUMENTS_DELTA.value,
+ ResponsesAPIStreamEvents.MCP_CALL_ARGUMENTS_DONE.value,
}:
self._record_tool_arguments(data)
return block["status"] = (
"generating_arguments"
- if data.get("type")
- == ResponsesAPIStreamEvents.FUNCTION_CALL_ARGUMENTS_DELTA.value
+ if data.get("type")
+ in {
+ ResponsesAPIStreamEvents.FUNCTION_CALL_ARGUMENTS_DELTA.value,
+ ResponsesAPIStreamEvents.MCP_CALL_ARGUMENTS_DELTA.value,
+ }
else "pending"
)📝 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 event_type in { | |
| ResponsesAPIStreamEvents.FUNCTION_CALL_ARGUMENTS_DELTA.value, | |
| ResponsesAPIStreamEvents.FUNCTION_CALL_ARGUMENTS_DONE.value, | |
| }: | |
| if event_type in { | |
| ResponsesAPIStreamEvents.FUNCTION_CALL_ARGUMENTS_DELTA.value, | |
| ResponsesAPIStreamEvents.FUNCTION_CALL_ARGUMENTS_DONE.value, | |
| ResponsesAPIStreamEvents.MCP_CALL_ARGUMENTS_DELTA.value, | |
| ResponsesAPIStreamEvents.MCP_CALL_ARGUMENTS_DONE.value, | |
| }: |
🤖 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/runtime_work/agent_adapter.py` around lines 202 - 205, The event
type condition at lines 202-205 only routes FUNCTION_CALL_ARGUMENTS_DELTA and
FUNCTION_CALL_ARGUMENTS_DONE events to _record_tool_arguments, but it's missing
the corresponding MCP_CALL_ARGUMENTS_DELTA and MCP_CALL_ARGUMENTS_DONE events.
Add MCP_CALL_ARGUMENTS_DELTA and MCP_CALL_ARGUMENTS_DONE to the set of event
types being checked in the condition at lines 202-205. Additionally, locate the
status logic around line 298 that sets "generating_arguments" for function-call
deltas and update it to also handle MCP argument delta events, ensuring mcp_call
blocks receive proper argument updates and correct status transitions.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
executor/agents/claude_code/response_processor.py (1)
959-962: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tracing to the new async helper.
_emit_tool_use_startis async but is not decorated with@trace_async, so this new path won’t be traced consistently.🔧 Proposed fix
+@trace_async async def _emit_tool_use_start( block: ToolUseBlock, emitter: ResponsesAPIEmitter, ) -> None:If it is not already imported in this module, add:
from shared.telemetry.decorators import trace_asyncAs per coding guidelines, “Use
@trace_asyncor@trace_syncdecorators from shared.telemetry.decorators for tracing async and sync functions respectively.”🤖 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/response_processor.py` around lines 959 - 962, The async function _emit_tool_use_start is missing the `@trace_async` decorator, which violates the coding guidelines for consistent tracing. First, check if trace_async is already imported from shared.telemetry.decorators in this module, and if not, add the import statement. Then add the `@trace_async` decorator directly above the _emit_tool_use_start function definition to ensure this async function is properly traced.Source: Coding guidelines
🤖 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 `@executor/agents/claude_code/response_processor.py`:
- Around line 986-987: The bare except Exception clause in the
response_processor.py file's tool_start event handler violates Ruff's BLE001
rule against catching overly broad exceptions. Either narrow the exception type
being caught to be more specific (like EventEmitterError or a more targeted
exception), or if catching all exceptions for this logging case is intentional,
add an explicit `# noqa: BLE001` comment with a rationale explaining why broad
exception suppression is necessary for the tool_start event emission failure
handling in that location.
---
Nitpick comments:
In `@executor/agents/claude_code/response_processor.py`:
- Around line 959-962: The async function _emit_tool_use_start is missing the
`@trace_async` decorator, which violates the coding guidelines for consistent
tracing. First, check if trace_async is already imported from
shared.telemetry.decorators in this module, and if not, add the import
statement. Then add the `@trace_async` decorator directly above the
_emit_tool_use_start function definition to ensure this async function is
properly traced.
🪄 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: 660696cf-3630-4607-accc-3bb7b2271058
📒 Files selected for processing (2)
executor/agents/claude_code/response_processor.pyexecutor/tests/agents/test_claude_response_processor.py
🚧 Files skipped from review as they are similar to previous changes (1)
- executor/tests/agents/test_claude_response_processor.py
Comment on lines
+986
to
+987
| except Exception as e: | ||
| logger.warning(f"Failed to send tool_start event: {e}") |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Resolve the Ruff BLE001 violation.
If suppressing all emitter failures is intentional, add an explicit noqa rationale; otherwise narrow the exception type.
🔧 Proposed fix when broad suppression is intentional
- except Exception as e:
+ except Exception as e: # noqa: BLE001 - event forwarding failures must not abort response processing.
logger.warning(f"Failed to send tool_start event: {e}")📝 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
| except Exception as e: | |
| logger.warning(f"Failed to send tool_start event: {e}") | |
| except Exception as e: # noqa: BLE001 - event forwarding failures must not abort response processing. | |
| logger.warning(f"Failed to send tool_start event: {e}") |
🧰 Tools
🪛 Ruff (0.15.18)
[warning] 986-986: Do not catch blind exception: Exception
(BLE001)
🤖 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/response_processor.py` around lines 986 - 987,
The bare except Exception clause in the response_processor.py file's tool_start
event handler violates Ruff's BLE001 rule against catching overly broad
exceptions. Either narrow the exception type being caught to be more specific
(like EventEmitterError or a more targeted exception), or if catching all
exceptions for this logging case is intentional, add an explicit `# noqa:
BLE001` comment with a rationale explaining why broad exception suppression is
necessary for the tool_start event emission failure handling in that location.
Source: Linters/SAST tools
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
refreshWorkListsbehavior for project creation and device workspace preparation, including optimistic updates.