Repository navigation
Codex/feature/wework source header - #1441
Conversation
📝 WalkthroughWalkthroughDevice disconnections now include a configurable grace period to avoid spurious offline cleanup on transient reconnects. Message reducer clears stale error state when assistant updates arrive, enabling recovery after disconnects. Workspace file opening is threaded from chat tool blocks through the workbench hierarchy via callbacks and request objects, with path validation and Safari-friendly terminal sizing added. ChangesProject ID zero in standalone chat
Device disconnect grace period and transience
Message state recovery after disconnect
Opening workspace files from chat messages
Terminal iframe sizing
Sequence Diagram(s)sequenceDiagram
participant Device as Device<br/>(socket_id)
participant Executor as DeviceNamespace<br/>on_disconnect
participant Check as Check Online<br/>Socket
participant Cleanup as Offline Cleanup<br/>& Fail Tasks
Device->>Executor: disconnect (socket_id="old")
Executor->>Executor: await grace period<br/>(2 sec)
Executor->>Check: get_device_online_info()
Check-->>Executor: current socket_id
alt socket_id unchanged (stale)
Executor->>Cleanup: mark offline, fail tasks
Cleanup-->>Device: tasks marked FAILED
else socket_id changed (reconnected)
Executor-->>Device: skip cleanup, device online
end
sequenceDiagram
participant Chat as Chat<br/>MessageList
participant ToolBlock as ToolBlockItem<br/>(file-based tool)
participant Workbench as DesktopWorkbenchMain
participant FilePanel as FileWorkspacePanel
participant Workspace as Workspace<br/>File System
Chat->>ToolBlock: onOpenWorkspaceFile callback
ToolBlock->>ToolBlock: getWorkspaceFilePath(block)<br/>→ path
ToolBlock->>Workbench: onOpenWorkspaceFile(path)
Workbench->>Workbench: openWorkspaceFileFromMessage<br/>→ create openFileRequest
Workbench->>FilePanel: pass openFileRequest prop
FilePanel->>FilePanel: resolveWorkspaceFilePath<br/>(validate, prevent traversal)
FilePanel->>FilePanel: openFilePath(resolved_path)
FilePanel->>Workspace: openFile request
Workspace-->>FilePanel: file content
FilePanel->>Chat: display preview
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
executor/modes/local/capabilities.py (1)
92-127: 💤 Low valueConsider documenting the new standalone chat workspace behavior.
The docstring for
get_project_iddoesn't mention thatproject_id="0"is now preserved whenstandalone_chat_workspace=True. Adding this detail would help maintainers understand the special handling.Similarly, the private helper
_normalize_project_idgained anallow_zeroparameter but has no docstring explaining its purpose.📝 Suggested docstring improvements
For
get_project_id:def get_project_id(task_data: ExecutionRequest) -> str: - """Return the project ID for a project-backed execution request.""" + """Return the project ID for a project-backed execution request. + + In standalone chat workspace mode, project_id="0" is preserved as a valid + project identifier. Otherwise, "0" is normalized to empty string. + """For
_normalize_project_id:def _normalize_project_id(project_id: Any, *, allow_zero: bool = False) -> str: + """Normalize project_id to a string, returning empty string for invalid values. + + Args: + project_id: Raw project ID value (int, str, or None). + allow_zero: If True, preserve "0" as valid; otherwise normalize to "". + + Returns: + Normalized project ID string, or "" if invalid. + """🤖 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 92 - 127, Update the docstring for the get_project_id function to document the new behavior where project_id="0" is preserved when standalone_chat_workspace is True. Additionally, add or update the docstring for the _normalize_project_id helper function to explain the purpose of its allow_zero parameter and how it affects the normalization logic. This will help future maintainers understand the special handling in both functions.executor/tests/modes/local/test_global_capabilities.py (1)
19-27: ⚡ Quick winConsider consolidating the duplicate WeWork standalone chat tests.
Both
test_wework_standalone_chat_uses_global_capabilities(lines 19-27) andtest_wework_standalone_chat_project_zero_keeps_project_header_id(lines 39-47) set up identical request objects withproject_id=0andstandalone_chat_workspace=True. The first test only checksis_project_task, while the second checks bothget_project_idandis_project_task.Consolidating them into a single test that verifies both assertions would reduce duplication.
♻️ Proposed consolidation
-def test_wework_standalone_chat_uses_global_capabilities(): +def test_wework_standalone_chat_with_project_zero_uses_global_capabilities(): request = ExecutionRequest( task_id=1901, project_id=0, standalone_chat_workspace=True, ) + assert get_project_id(request) == "0" assert is_project_task(request) is True def test_frontend_device_chat_project_zero_does_not_use_project_capabilities(): request = ExecutionRequest( task_id=1902, project_id=0, ) assert get_project_id(request) == "" assert is_project_task(request) is False - - -def test_wework_standalone_chat_project_zero_keeps_project_header_id(): - request = ExecutionRequest( - task_id=1904, - project_id=0, - standalone_chat_workspace=True, - ) - - assert get_project_id(request) == "0" - assert is_project_task(request) is TrueAlso applies to: 39-47
🤖 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/modes/local/test_global_capabilities.py` around lines 19 - 27, Consolidate the two WeWork standalone chat tests by removing test_wework_standalone_chat_uses_global_capabilities and updating test_wework_standalone_chat_project_zero_keeps_project_header_id to include all assertions from both tests. The consolidated test should create a single ExecutionRequest with project_id=0 and standalone_chat_workspace=True, then verify both the is_project_task and get_project_id assertions in one test function to eliminate the duplicate setup code and test redundancy.
🤖 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 `@executor/modes/local/capabilities.py`:
- Around line 92-127: Update the docstring for the get_project_id function to
document the new behavior where project_id="0" is preserved when
standalone_chat_workspace is True. Additionally, add or update the docstring for
the _normalize_project_id helper function to explain the purpose of its
allow_zero parameter and how it affects the normalization logic. This will help
future maintainers understand the special handling in both functions.
In `@executor/tests/modes/local/test_global_capabilities.py`:
- Around line 19-27: Consolidate the two WeWork standalone chat tests by
removing test_wework_standalone_chat_uses_global_capabilities and updating
test_wework_standalone_chat_project_zero_keeps_project_header_id to include all
assertions from both tests. The consolidated test should create a single
ExecutionRequest with project_id=0 and standalone_chat_workspace=True, then
verify both the is_project_task and get_project_id assertions in one test
function to eliminate the duplicate setup code and test redundancy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ffbf8e07-9316-4a35-97d4-099a98bc6cc2
📒 Files selected for processing (2)
executor/modes/local/capabilities.pyexecutor/tests/modes/local/test_global_capabilities.py
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/chat-core/src/workbench-message-reducer.ts (1)
207-217:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDerive active-state from effective block status, not only
updates.statusOn Line 211, recovery depends solely on
action.updates.status, butstatusis optional inProcessingBlockUpdate. Ablock_updatedcarrying only incremental fields (e.g.,toolOutput) for an already-streaming block will not clear staleerror/errorTypeand can leave message status stuck atfailed.Proposed fix
case 'block_updated': return state.map(message => - message.subtaskId === action.subtaskId - ? { - ...withActiveStreamState(message, isActiveBlockStatus(action.updates.status)), - blocks: (message.blocks ?? []).map(block => - block.id === action.blockId - ? ({ ...block, ...action.updates } as WorkbenchProcessingBlock) - : block - ), - } - : message + message.subtaskId === action.subtaskId + ? (() => { + const currentBlock = (message.blocks ?? []).find( + block => block.id === action.blockId + ) + const effectiveStatus = action.updates.status ?? currentBlock?.status + const nextMessage = withActiveStreamState( + message, + isActiveBlockStatus(effectiveStatus) + ) + + return { + ...nextMessage, + blocks: (message.blocks ?? []).map(block => + block.id === action.blockId + ? ({ ...block, ...action.updates } as WorkbenchProcessingBlock) + : block + ), + } + })() + : message )🤖 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 `@packages/chat-core/src/workbench-message-reducer.ts` around lines 207 - 217, The issue is that the call to withActiveStreamState with isActiveBlockStatus(action.updates.status) on the block_updated case relies solely on the optional status field from action.updates. When a block_updated action contains only incremental fields (like toolOutput) without a status field, action.updates.status will be undefined, causing isActiveBlockStatus to receive undefined and failing to properly clear stale error/errorType fields. Fix this by deriving the effective block status more comprehensively by combining the current block's status with the updates provided, rather than relying only on the optional action.updates.status field when calling isActiveBlockStatus.wework/src/components/layout/DesktopWorkbenchMain.tsx (1)
430-444:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear stale file-open requests on session switch
openFileRequestis not reset whenrightPanelSessionKeychanges. That can replay a previous chat-triggered file-open in a different task/project when the Files tab mounts again.Suggested fix
useLayoutEffect(() => { if (previousRightPanelSessionKey.current === rightPanelSessionKey) { return } previousRightPanelSessionKey.current = rightPanelSessionKey reviewRequestSequence.current += 1 + setOpenFileRequest(null) setRightPanelView('launcher') setRightPanelTabs([]) setReviewState({ loading: false, diff: '', error: undefined, }) }, [rightPanelSessionKey])🤖 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/DesktopWorkbenchMain.tsx` around lines 430 - 444, The useLayoutEffect hook that responds to rightPanelSessionKey changes is not resetting the openFileRequest state, which allows stale file-open requests from previous sessions to be replayed when switching between tasks or projects. Add a line to reset openFileRequest to null or undefined in the useLayoutEffect hook (after the previousRightPanelSessionKey.current assignment and alongside the other state resets like setRightPanelView and setReviewState) to ensure no stale file-open requests persist across session switches.
🧹 Nitpick comments (2)
packages/chat-core/src/workbench-message-reducer.test.ts (1)
178-205: ⚡ Quick winStrengthen regression signal by asserting pre-update error state (Line 178 onward).
This test only checks the final
resumedstate. Add precondition assertions onwithTool(status: 'failed',error,errorType) so the test proves the clear/recovery happens specifically onblock_updated.Suggested diff
const withTool = reduceWorkbenchMessages(state, { type: 'block_created', subtaskId: 9, block: { id: 'call_1', subtaskId: 9, type: 'tool', toolName: 'bash', status: 'pending', createdAt: 1770000000000, }, }) + expect(withTool[0].status).toBe('failed') + expect(withTool[0].error).toBe('Device disconnected') + expect(withTool[0].errorType).toBe('container_error') + const resumed = reduceWorkbenchMessages(withTool, { type: 'block_updated', subtaskId: 9, blockId: 'call_1', updates: {🤖 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 `@packages/chat-core/src/workbench-message-reducer.test.ts` around lines 178 - 205, The test only verifies the final state after block_updated is called but lacks precondition assertions on the withTool state. Add assertions immediately after the withTool constant is assigned to verify that the block has status as 'failed' and contains error and errorType properties. This strengthens the regression signal by proving that the subsequent block_updated action with status 'streaming' specifically clears these error states rather than just setting a passing end state, making the error recovery behavior explicit and testable.backend/tests/api/ws/test_device_reconnect_storm.py (1)
207-207: ⚡ Quick winAssert the configured grace-period value, not just await count.
At Line 207,
assert_awaited_once()verifies a sleep happened, but not that it usedDEVICE_DISCONNECT_FAILURE_GRACE_SECONDS. This test can pass even if the grace window is accidentally changed.Suggested diff
- device_namespace.asyncio.sleep.assert_awaited_once() + device_namespace.asyncio.sleep.assert_awaited_once_with( + device_namespace.DEVICE_DISCONNECT_FAILURE_GRACE_SECONDS + )🤖 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_reconnect_storm.py` at line 207, The assertion on device_namespace.asyncio.sleep only verifies that sleep was called once using assert_awaited_once(), but does not validate that it was called with the correct grace period value. Replace assert_awaited_once() with assert_awaited_once_with() and pass the expected DEVICE_DISCONNECT_FAILURE_GRACE_SECONDS constant as an argument to ensure the test fails if someone accidentally changes the grace window value.
🤖 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 `@wework/src/components/chat/blocks/ToolBlockItem.tsx`:
- Around line 50-51: The aria-label attributes in the ToolBlockItem component
contain hardcoded Chinese strings instead of using i18n keys. Replace the
hardcoded Chinese strings ('收起工具详情' and '展开工具详情') in the aria-label props with
translation keys from useTranslation('chat'), such as t('collapseToolDetails')
and t('expandToolDetails'). Import useTranslation from '`@/hooks/useTranslation`'
if not already imported, then add the corresponding translation keys to both the
Chinese and English language files in the chat namespace without using fallback
literals.
In `@wework/src/components/layout/workspace-panels/FileWorkspacePanel.tsx`:
- Around line 27-42: The function `resolveWorkspaceFilePath` has a security
issue where absolute paths starting with `/` are returned immediately without
validation at line 30, allowing access to files outside the workspace root like
`/etc/passwd`. Remove the early return for paths starting with `/` and instead
process all paths through the same segment validation logic that handles
relative paths. After constructing the final resolved path, add validation to
ensure the resolved path stays within the current workspace root (target.path)
and return null if the path would escape the workspace boundaries.
---
Outside diff comments:
In `@packages/chat-core/src/workbench-message-reducer.ts`:
- Around line 207-217: The issue is that the call to withActiveStreamState with
isActiveBlockStatus(action.updates.status) on the block_updated case relies
solely on the optional status field from action.updates. When a block_updated
action contains only incremental fields (like toolOutput) without a status
field, action.updates.status will be undefined, causing isActiveBlockStatus to
receive undefined and failing to properly clear stale error/errorType fields.
Fix this by deriving the effective block status more comprehensively by
combining the current block's status with the updates provided, rather than
relying only on the optional action.updates.status field when calling
isActiveBlockStatus.
In `@wework/src/components/layout/DesktopWorkbenchMain.tsx`:
- Around line 430-444: The useLayoutEffect hook that responds to
rightPanelSessionKey changes is not resetting the openFileRequest state, which
allows stale file-open requests from previous sessions to be replayed when
switching between tasks or projects. Add a line to reset openFileRequest to null
or undefined in the useLayoutEffect hook (after the
previousRightPanelSessionKey.current assignment and alongside the other state
resets like setRightPanelView and setReviewState) to ensure no stale file-open
requests persist across session switches.
---
Nitpick comments:
In `@backend/tests/api/ws/test_device_reconnect_storm.py`:
- Line 207: The assertion on device_namespace.asyncio.sleep only verifies that
sleep was called once using assert_awaited_once(), but does not validate that it
was called with the correct grace period value. Replace assert_awaited_once()
with assert_awaited_once_with() and pass the expected
DEVICE_DISCONNECT_FAILURE_GRACE_SECONDS constant as an argument to ensure the
test fails if someone accidentally changes the grace window value.
In `@packages/chat-core/src/workbench-message-reducer.test.ts`:
- Around line 178-205: The test only verifies the final state after
block_updated is called but lacks precondition assertions on the withTool state.
Add assertions immediately after the withTool constant is assigned to verify
that the block has status as 'failed' and contains error and errorType
properties. This strengthens the regression signal by proving that the
subsequent block_updated action with status 'streaming' specifically clears
these error states rather than just setting a passing end state, making the
error recovery behavior explicit and testable.
🪄 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: db691014-f0e9-4695-8d4f-84de51db3520
📒 Files selected for processing (18)
backend/app/api/ws/device_namespace.pybackend/tests/api/ws/test_device_reconnect_storm.pydocs/en/user-guide/ai-devices/local-device-support.mddocs/zh/user-guide/ai-devices/local-device-support.mdfrontend/src/__tests__/features/settings/components/SkillListWithScope.default-enabled.test.tsxpackages/chat-core/src/workbench-message-reducer.test.tspackages/chat-core/src/workbench-message-reducer.tswework/src/components/chat/MessageList.tsxwework/src/components/chat/ScrollableMessageArea.tsxwework/src/components/chat/blocks/ToolBlockItem.tsxwework/src/components/chat/blocks/ToolBlocksDisplay.tsxwework/src/components/layout/DesktopWorkbenchLayout.test.tsxwework/src/components/layout/DesktopWorkbenchMain.tsxwework/src/components/layout/workspace-panels/FileWorkspacePanel.tsxwework/src/components/layout/workspace-panels/RightWorkspacePanel.tsxwework/src/components/layout/workspace-panels/WorkspacePanelCards.test.tsxwework/src/components/layout/workspace-panels/WorkspacePanelCards.tsxwework/src/types/workspace-files.ts
✅ Files skipped from review due to trivial changes (3)
- docs/en/user-guide/ai-devices/local-device-support.md
- wework/src/components/layout/workspace-panels/WorkspacePanelCards.tsx
- wework/src/types/workspace-files.ts
| aria-label={expanded ? '收起工具详情' : '展开工具详情'} | ||
| aria-expanded={expanded} |
There was a problem hiding this comment.
Replace hardcoded aria labels with i18n keys
These new aria-label literals are hardcoded Chinese strings, which breaks localization consistency and screen-reader language expectations in non-Chinese locales. Please route them through useTranslation('chat') keys instead.
Suggested fix
- aria-label={expanded ? '收起工具详情' : '展开工具详情'}
+ aria-label={
+ expanded ? t('tool_block.collapse_details') : t('tool_block.expand_details')
+ }As per coding guidelines, “Import from '@/hooks/useTranslation' for i18n in application code” and “Do not add fallback literals to hide missing translations in new code. Add the key to the correct namespace in both languages instead.”
📝 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.
| aria-label={expanded ? '收起工具详情' : '展开工具详情'} | |
| aria-expanded={expanded} | |
| aria-label={ | |
| expanded ? t('tool_block.collapse_details') : t('tool_block.expand_details') | |
| } | |
| aria-expanded={expanded} |
🤖 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/chat/blocks/ToolBlockItem.tsx` around lines 50 - 51,
The aria-label attributes in the ToolBlockItem component contain hardcoded
Chinese strings instead of using i18n keys. Replace the hardcoded Chinese
strings ('收起工具详情' and '展开工具详情') in the aria-label props with translation keys
from useTranslation('chat'), such as t('collapseToolDetails') and
t('expandToolDetails'). Import useTranslation from '`@/hooks/useTranslation`' if
not already imported, then add the corresponding translation keys to both the
Chinese and English language files in the chat namespace without using fallback
literals.
Source: Coding guidelines
| function resolveWorkspaceFilePath(target: WorkspaceTarget, path: string): string | null { | ||
| const normalizedPath = path.trim().replace(/\\/g, '/') | ||
| if (!normalizedPath) return null | ||
| if (normalizedPath.startsWith('/')) return normalizedPath | ||
|
|
||
| const segments: string[] = [] | ||
| for (const segment of normalizedPath.split('/')) { | ||
| if (!segment || segment === '.') continue | ||
| if (segment === '..') return null | ||
| segments.push(segment) | ||
| } | ||
| if (segments.length === 0) return null | ||
|
|
||
| const root = target.path.replace(/\/+$/, '') || '/' | ||
| const child = segments.join('/') | ||
| return root === '/' ? `/${child}` : `${root}/${child}` |
There was a problem hiding this comment.
Constrain absolute requests to the workspace root.
Line 30 returns any leading-slash path unchanged, so /etc/passwd, /workspace/../secret, or another workspace path bypasses the target.path anchoring before openFilePath() sends it to readWorkspaceTextFile(). Normalize absolute and relative requests through the same segment validation, then reject resolved paths outside the current workspace root.
🛡️ Proposed fix
function resolveWorkspaceFilePath(target: WorkspaceTarget, path: string): string | null {
const normalizedPath = path.trim().replace(/\\/g, '/')
if (!normalizedPath) return null
- if (normalizedPath.startsWith('/')) return normalizedPath
+
+ const root = target.path.trim().replace(/\\/g, '/').replace(/\/+$/, '') || '/'
+ const candidate = normalizedPath.startsWith('/')
+ ? normalizedPath
+ : root === '/'
+ ? `/${normalizedPath}`
+ : `${root}/${normalizedPath}`
const segments: string[] = []
- for (const segment of normalizedPath.split('/')) {
+ for (const segment of candidate.split('/')) {
if (!segment || segment === '.') continue
if (segment === '..') return null
segments.push(segment)
}
if (segments.length === 0) return null
- const root = target.path.replace(/\/+$/, '') || '/'
- const child = segments.join('/')
- return root === '/' ? `/${child}` : `${root}/${child}`
+ const resolvedPath = `/${segments.join('/')}`
+ if (root !== '/') {
+ const rootWithSlash = `${root}/`
+ if (resolvedPath !== root && !resolvedPath.startsWith(rootWithSlash)) {
+ return null
+ }
+ }
+
+ return resolvedPath
}🤖 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/workspace-panels/FileWorkspacePanel.tsx` around
lines 27 - 42, The function `resolveWorkspaceFilePath` has a security issue
where absolute paths starting with `/` are returned immediately without
validation at line 30, allowing access to files outside the workspace root like
`/etc/passwd`. Remove the early return for paths starting with `/` and instead
process all paths through the same segment validation logic that handles
relative paths. After constructing the final resolved path, add validation to
ensure the resolved path stays within the current workspace root (target.path)
and return null if the path would escape the workspace boundaries.
f7e823c to
b5b5983
Compare
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
UI Improvements
Tests