Repository navigation
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR only reformats several expressions and comprehensions in runtime Codex discovery, global state loading, and RPC handler code. No control flow, data shape, or returned values change. ChangesRuntime formatting cleanup
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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}
There was a problem hiding this comment.
Actionable comments posted: 1
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)
2356-2375: 🩺 Stability & Availability | 🟡 MinorVerify stream event filtering doesn't drop early events when
localTaskIdchanges.The
isCurrentLocalTaskEventfilter requirespayload.local_task_idto matchcurrentRuntimeTaskRef.current.localTaskId. SincecurrentRuntimeTaskRefis only updated aftercreateRuntimeTaskresolves, any stream events arriving with the new device-confirmedlocalTaskIdbefore this update occurs will be rejected, potentially losingonChatStartor initialonChatChunkevents.🤖 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 2356 - 2375, The stream-event filter can drop early events because currentRuntimeTaskRef.current is only updated after createRuntimeTask resolves, so isCurrentLocalTaskEvent may reject payloads carrying the new localTaskId too soon. Update the task identity earlier in the createRuntimeTask flow, or relax the filter to accept the pending optimistic address until the confirmed address is dispatched, using the existing currentRuntimeTaskRef, optimisticAddress, and isCurrentLocalTaskEvent logic.
🤖 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/codex_discovery.py`:
- Around line 876-885: The _normalize_local_image_path helper is rejecting valid
Windows drive-letter paths because urlparse treats the drive letter as a scheme;
update the scheme validation so it only filters out real URLs by rejecting
schemes longer than one character, while still allowing single-letter drive
paths like C:/... to pass through. Keep the existing normalization flow in
_normalize_local_image_path and adjust only the parsed.scheme check accordingly.
---
Outside diff comments:
In `@wework/src/features/workbench/WorkbenchProvider.tsx`:
- Around line 2356-2375: The stream-event filter can drop early events because
currentRuntimeTaskRef.current is only updated after createRuntimeTask resolves,
so isCurrentLocalTaskEvent may reject payloads carrying the new localTaskId too
soon. Update the task identity earlier in the createRuntimeTask flow, or relax
the filter to accept the pending optimistic address until the confirmed address
is dispatched, using the existing currentRuntimeTaskRef, optimisticAddress, and
isCurrentLocalTaskEvent logic.
🪄 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: a01bdcea-6e0a-4f75-94e8-7f2c3a917444
📒 Files selected for processing (18)
backend/app/schemas/runtime_work.pybackend/app/services/runtime_work_service.pybackend/tests/services/test_runtime_work_service.pydocs/en/developer-guide/runtime-local-work.mddocs/zh/developer-guide/runtime-local-work.mdexecutor/runtime_work/codex_discovery.pyexecutor/tests/runtime_work/test_local_task_store.pywework/src/components/chat/blocks/ToolBlockItem.tsxwework/src/components/chat/blocks/ToolBlocksDisplay.test.tsxwework/src/components/chat/blocks/ToolBlocksDisplay.tsxwework/src/components/chat/blocks/toolBlockActivity.tswework/src/components/layout/DesktopWorkbenchLayout.tsxwework/src/components/layout/DesktopWorkbenchMain.tsxwework/src/components/layout/MobileWorkbenchLayout.tsxwework/src/features/workbench/WorkbenchProvider.test.tsxwework/src/features/workbench/WorkbenchProvider.tsxwework/src/pages/WorkbenchPage.tsxwework/src/types/api.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
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)
2356-2375: 🩺 Stability & Availability | 🟡 MinorVerify stream event filtering doesn't drop early events when
localTaskIdchanges.The
isCurrentLocalTaskEventfilter requirespayload.local_task_idto matchcurrentRuntimeTaskRef.current.localTaskId. SincecurrentRuntimeTaskRefis only updated aftercreateRuntimeTaskresolves, any stream events arriving with the new device-confirmedlocalTaskIdbefore this update occurs will be rejected, potentially losingonChatStartor initialonChatChunkevents.🤖 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 2356 - 2375, The stream-event filter can drop early events because currentRuntimeTaskRef.current is only updated after createRuntimeTask resolves, so isCurrentLocalTaskEvent may reject payloads carrying the new localTaskId too soon. Update the task identity earlier in the createRuntimeTask flow, or relax the filter to accept the pending optimistic address until the confirmed address is dispatched, using the existing currentRuntimeTaskRef, optimisticAddress, and isCurrentLocalTaskEvent logic.
🤖 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/codex_discovery.py`:
- Around line 876-885: The _normalize_local_image_path helper is rejecting valid
Windows drive-letter paths because urlparse treats the drive letter as a scheme;
update the scheme validation so it only filters out real URLs by rejecting
schemes longer than one character, while still allowing single-letter drive
paths like C:/... to pass through. Keep the existing normalization flow in
_normalize_local_image_path and adjust only the parsed.scheme check accordingly.
---
Outside diff comments:
In `@wework/src/features/workbench/WorkbenchProvider.tsx`:
- Around line 2356-2375: The stream-event filter can drop early events because
currentRuntimeTaskRef.current is only updated after createRuntimeTask resolves,
so isCurrentLocalTaskEvent may reject payloads carrying the new localTaskId too
soon. Update the task identity earlier in the createRuntimeTask flow, or relax
the filter to accept the pending optimistic address until the confirmed address
is dispatched, using the existing currentRuntimeTaskRef, optimisticAddress, and
isCurrentLocalTaskEvent logic.
🪄 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: a01bdcea-6e0a-4f75-94e8-7f2c3a917444
📒 Files selected for processing (18)
backend/app/schemas/runtime_work.pybackend/app/services/runtime_work_service.pybackend/tests/services/test_runtime_work_service.pydocs/en/developer-guide/runtime-local-work.mddocs/zh/developer-guide/runtime-local-work.mdexecutor/runtime_work/codex_discovery.pyexecutor/tests/runtime_work/test_local_task_store.pywework/src/components/chat/blocks/ToolBlockItem.tsxwework/src/components/chat/blocks/ToolBlocksDisplay.test.tsxwework/src/components/chat/blocks/ToolBlocksDisplay.tsxwework/src/components/chat/blocks/toolBlockActivity.tswework/src/components/layout/DesktopWorkbenchLayout.tsxwework/src/components/layout/DesktopWorkbenchMain.tsxwework/src/components/layout/MobileWorkbenchLayout.tsxwework/src/features/workbench/WorkbenchProvider.test.tsxwework/src/features/workbench/WorkbenchProvider.tsxwework/src/pages/WorkbenchPage.tsxwework/src/types/api.ts
🛑 Comments failed to post (1)
executor/runtime_work/codex_discovery.py (1)
876-885: 🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Confirm urlparse treats a Windows drive letter as a scheme python3 - <<'PY' from urllib.parse import urlparse for p in ("C:/Users/me/img.png", r"C:\Users\me\img.png", "/abs/img.png", "file:///C:/img.png"): print(repr(p), "-> scheme=", repr(urlparse(p).scheme)) PYRepository: wecode-ai/Wegent
Length of output: 299
Local Windows paths are incorrectly rejected, causing missing image previews.
urlparseinterprets the drive letter inC:/pathas a scheme (c), causing the current checkparsed.scheme != "file"to reject valid local paths. Update the check to only block schemes longer than a single character (standard URLs) to preserve Windows drive-letter handling.parsed = urlparse(path) - if parsed.scheme and parsed.scheme != "file": + # Single-letter schemes are Windows drive letters (e.g., "C:..."). + if len(parsed.scheme) > 1 and parsed.scheme != "file": return None return path📝 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.def _normalize_local_image_path(value: Any) -> Optional[str]: if not isinstance(value, str): return None path = value.strip() if not path: return None parsed = urlparse(path) # Single-letter schemes are Windows drive letters (e.g., "C:..."). if len(parsed.scheme) > 1 and parsed.scheme != "file": return None return path🤖 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/codex_discovery.py` around lines 876 - 885, The _normalize_local_image_path helper is rejecting valid Windows drive-letter paths because urlparse treats the drive letter as a scheme; update the scheme validation so it only filters out real URLs by rejecting schemes longer than one character, while still allowing single-letter drive paths like C:/... to pass through. Keep the existing normalization flow in _normalize_local_image_path and adjust only the parsed.scheme check accordingly.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/codex_discovery.py`:
- Around line 393-402: The transcript cache cleanup is using a different key
than the one used by read_transcript_page(), so deleted pages can remain cached
for relative or symlinked paths. Update the deletion loop in codex_discovery.py
to remove entries from _transcript_cache using the same str(path) key shape that
read_transcript_page() uses, rather than only the resolved deleted_path returned
by _delete_codex_owned_file(). Keep the existing deletion flow in the candidate
loop, but ensure the cache pop targets the original session path key as well so
stale transcript pages are fully cleared.
- Around line 338-341: Validate the thread ID in unarchive_thread() before
calling thread_unarchive by using the same _required_thread_id() helper that
archive_thread() and rename_thread() use. This keeps blank or whitespace-only
IDs from reaching the Codex SDK and makes unarchive_thread() consistent with the
other thread operations. Keep the fix localized to unarchive_thread() and
preserve the existing client/context flow.
In `@executor/runtime_work/codex_global_state.py`:
- Around line 279-300: Serialize the read-modify-write in
_update_global_state_payload so concurrent ensure_* / rename_* / remove_* calls
cannot overwrite each other. Add a lock around the full sequence that reads with
_read_global_state_payload, mutates, writes the temp file, and calls os.replace,
so the atomic swap is protected by mutual exclusion as well. Use the
_update_global_state_payload helper as the main place to introduce the lock and
keep the existing temporary-file replace flow.
🪄 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: a261d4f0-cdb9-4dde-95ea-e8b1126e2c7a
📒 Files selected for processing (3)
executor/runtime_work/codex_discovery.pyexecutor/runtime_work/codex_global_state.pyexecutor/runtime_work/rpc_handler.py
✅ Files skipped from review due to trivial changes (1)
- executor/runtime_work/rpc_handler.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 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/codex_discovery.py`:
- Around line 393-402: The transcript cache cleanup is using a different key
than the one used by read_transcript_page(), so deleted pages can remain cached
for relative or symlinked paths. Update the deletion loop in codex_discovery.py
to remove entries from _transcript_cache using the same str(path) key shape that
read_transcript_page() uses, rather than only the resolved deleted_path returned
by _delete_codex_owned_file(). Keep the existing deletion flow in the candidate
loop, but ensure the cache pop targets the original session path key as well so
stale transcript pages are fully cleared.
- Around line 338-341: Validate the thread ID in unarchive_thread() before
calling thread_unarchive by using the same _required_thread_id() helper that
archive_thread() and rename_thread() use. This keeps blank or whitespace-only
IDs from reaching the Codex SDK and makes unarchive_thread() consistent with the
other thread operations. Keep the fix localized to unarchive_thread() and
preserve the existing client/context flow.
In `@executor/runtime_work/codex_global_state.py`:
- Around line 279-300: Serialize the read-modify-write in
_update_global_state_payload so concurrent ensure_* / rename_* / remove_* calls
cannot overwrite each other. Add a lock around the full sequence that reads with
_read_global_state_payload, mutates, writes the temp file, and calls os.replace,
so the atomic swap is protected by mutual exclusion as well. Use the
_update_global_state_payload helper as the main place to introduce the lock and
keep the existing temporary-file replace flow.
🪄 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: a261d4f0-cdb9-4dde-95ea-e8b1126e2c7a
📒 Files selected for processing (3)
executor/runtime_work/codex_discovery.pyexecutor/runtime_work/codex_global_state.pyexecutor/runtime_work/rpc_handler.py
✅ Files skipped from review due to trivial changes (1)
- executor/runtime_work/rpc_handler.py
🛑 Comments failed to post (3)
executor/runtime_work/codex_discovery.py (2)
338-341: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate the thread ID before unarchiving.
unarchive_thread()bypasses_required_thread_id(), so blank or whitespace IDs can reach the SDK and fail differently fromarchive_thread()/rename_thread().Proposed fix
def unarchive_thread(self, thread_id: str) -> None: + normalized_thread_id = _required_thread_id(thread_id) client = self._create_codex_client() with client as codex: - codex.thread_unarchive(thread_id) + codex.thread_unarchive(normalized_thread_id)📝 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.def unarchive_thread(self, thread_id: str) -> None: normalized_thread_id = _required_thread_id(thread_id) client = self._create_codex_client() with client as codex: codex.thread_unarchive(normalized_thread_id)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/runtime_work/codex_discovery.py` around lines 338 - 341, Validate the thread ID in unarchive_thread() before calling thread_unarchive by using the same _required_thread_id() helper that archive_thread() and rename_thread() use. This keeps blank or whitespace-only IDs from reaching the Codex SDK and makes unarchive_thread() consistent with the other thread operations. Keep the fix localized to unarchive_thread() and preserve the existing client/context flow.
393-402: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clear the transcript cache with the same key shape used to populate it.
read_transcript_page()caches understr(path), but deletion only pops the resolved path returned by_delete_codex_owned_file(). Relative or symlinked session paths can leave stale pages cached after deletion.Proposed fix
for candidate in _unique_texts( session_path, _thread_state_text(thread_state, "rollout_path"), ): deleted_path = _delete_codex_owned_file(self.codex_home, candidate) if deleted_path: deleted_files.append(deleted_path) + self._transcript_cache.pop(candidate, None) self._transcript_cache.pop(deleted_path, None)📝 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.deleted_files = [] for candidate in _unique_texts( session_path, _thread_state_text(thread_state, "rollout_path"), ): deleted_path = _delete_codex_owned_file(self.codex_home, candidate) if deleted_path: deleted_files.append(deleted_path) self._transcript_cache.pop(candidate, None) self._transcript_cache.pop(deleted_path, None)🤖 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/codex_discovery.py` around lines 393 - 402, The transcript cache cleanup is using a different key than the one used by read_transcript_page(), so deleted pages can remain cached for relative or symlinked paths. Update the deletion loop in codex_discovery.py to remove entries from _transcript_cache using the same str(path) key shape that read_transcript_page() uses, rather than only the resolved deleted_path returned by _delete_codex_owned_file(). Keep the existing deletion flow in the candidate loop, but ensure the cache pop targets the original session path key as well so stale transcript pages are fully cleared.executor/runtime_work/codex_global_state.py (1)
279-300: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Serialize global-state updates.
_update_global_state_payload()is a read-modify-write cycle with no lock, so concurrentensure_*/rename_*/remove_*calls can clobber each other’s changes.os.replace()only makes the final swap atomic; it does not protect the mutation window.🤖 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/codex_global_state.py` around lines 279 - 300, Serialize the read-modify-write in _update_global_state_payload so concurrent ensure_* / rename_* / remove_* calls cannot overwrite each other. Add a lock around the full sequence that reads with _read_global_state_payload, mutates, writes the temp file, and calls os.replace, so the atomic swap is protected by mutual exclusion as well. Use the _update_global_state_payload helper as the main place to introduce the lock and keep the existing temporary-file replace flow.
1 participant
Summary by CodeRabbit