Repository navigation
feat: improve local task execution flow - #1596
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds chat task device resolution and response.created START handling, streams executor output as incremental events with Claude setup and Codex isolation, and updates frontend query parsing plus thinking-block rendering. ChangesBackend chat dispatch and START events
Executor streaming, Claude setup, and Codex isolation
Frontend null-safe search params and thinking rendering
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 13
🤖 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/chat/task_device_resolution.py`:
- Around line 100-128: The online-device fallback in
resolve_online_local_executor_device_id() only treats device.name as a valid
local alias, so inactive appDeviceId values are not recognized and the stale ID
can be returned. Update the alias check to include the frontend appDeviceId from
the local device records when building the local identifier set, using
_list_local_devices(..., include_inactive=True) and the same normalization
helper, so the fallback can switch to the sole online executor when the stored
device is inactive.
In `@executor/src/agents/codex.rs`:
- Around line 360-377: Guard the destructive cleanup in
prepare_codex_runtime_home by checking whether the resolved runtime_home matches
the user Codex home before any remove_runtime_capability_dir or
sync_codex_runtime_file calls; if codex_runtime_home() points to
user_codex_home(), return an error instead of deleting skills/plugins. Apply the
same protection to the related cleanup/sync path in user_codex_home and
prepare_codex_runtime_home so WEGENT_CODEX_RUNTIME_HOME cannot target
$HOME/.codex.
In `@executor/src/agents/mod.rs`:
- Around line 342-346: The attachment path builder in the attachment download
flow currently uses only the sanitized original filename, so duplicate names in
the same task/subtask collide. Update the path construction in the attachment
path logic used by the relevant helper in executor/src/agents/mod.rs to include
attachment.id in the local filename or path while keeping
sanitize_attachment_filename as part of the name. Make sure both download and
record lookup paths use the same unique scheme so each attachment maps to its
own file.
- Around line 547-548: The current hook update logic in the group-building flow
is removing too broadly: `groups.retain()` is deleting any group matched by
`file_edit_hook_group_matches`, including user-defined groups. Tighten the
predicate so only the Wegent-owned hook group is removed/updated, then append or
replace that specific hook in the same `groups` handling path used around
`file_edit_hook_group_matches` and the `groups.push(json!({ ... }))` block.
- Line 248: The attachment download client is created without any timeout, so a
stalled backend can block pre-launch indefinitely. Update the attachment
download path in the relevant agent startup flow to use a reqwest client or
request timeout, and make sure timeout failures are handled as failed
attachments rather than hanging the agent; use the existing client setup around
reqwest::Client::new as the place to apply this change.
- Around line 517-523: read_claude_settings currently masks read/parse/type
failures by converting them into an empty object, which can overwrite a
malformed or non-object Claude settings file. Update read_claude_settings to
return a result/error instead of always producing Value::Object(Map::new()), and
make the caller that injects hooks detect that failure and skip writing settings
when the existing file cannot be safely parsed. Use read_claude_settings and the
hook injection path in executor/src/agents/mod.rs to locate the change.
In `@executor/src/local/capabilities.rs`:
- Around line 944-954: The plugin restore fallback in the cache lookup is too
broad and can select a same-named ZIP from the wrong marketplace. Update the
restoration logic in the cache scanning path around the `read_dir`/`candidates`
search so it only restores `{marketplace}/{name}.zip`, or verifies the package
manifest/source matches the enabled plugin before extraction. Use the existing
cache resolution flow in `capabilities.rs` to skip restoration when the exact
marketplace match is missing.
In `@executor/src/local/capabilities/support.rs`:
- Around line 92-100: The hook permission repair traversal in
support::ensure_plugin_hook_executable is following symlinks because it uses
fs::metadata on each entry target, which can let the walk escape plugin_path.
Switch the metadata check to symlink_metadata so symlinks are detected without
being followed, and explicitly skip symlink entries before deciding whether to
push into pending or call ensure_plugin_hook_executable; keep the existing
traversal logic in the read_dir loop unchanged otherwise.
In `@executor/src/process/mod.rs`:
- Around line 331-344: The child process handling in
process::mod::wait_for_child is short-circuiting on stdin write errors and can
replace the real exit result with a generic failure. Update the logic around
child.wait().await, writer.await, and the CommandOutcome::Failure return so that
a write error does not override an already collected child status, especially
non-zero exits. Preserve the process result first, always continue through
join_output(stdout_task) and join_output(stderr_task), and only use the write
error as supplemental information when the child exit result does not already
explain the outcome.
In `@executor/tests/agent_process_engine_contract.rs`:
- Around line 607-668: The stale hook fixture is being written to a path that
may not match the actual Claude config directory used by the test, so the test
can pass without exercising the file it creates. Update the setup in the stale
file edit hook test to write `settings.json` through the resolved
`CLAUDE_CONFIG_DIR` path used by the runtime, or set `HOME` first and derive the
config path from it. Use the existing test symbols `workspace_root`, `task_dir`,
`settings_path`, and `fake_claude` to keep the fixture aligned with the real
Claude task-turn config resolution.
In `@executor/tests/local_backend_contract.rs`:
- Around line 315-325: The test in local_backend_contract.rs is hard-coding an
assumed count of response.output_text.delta events, which makes it brittle when
read_streaming_stdout changes chunking via stream_chunk_chars(). Update the
assertion around transport.wait_for_emits and the emitted event checks so it
either sets the chunk size explicitly for this test or collects all
response.output_text.delta events until response.completed, then verifies the
concatenated payload matches long_text. Use the existing transport and emitted
event parsing logic to avoid relying on a fixed delta count.
In `@frontend/src/features/tasks/components/chat/ChatArea.tsx`:
- Around line 430-431: `ChatArea` is resolving task-id aliases in a different
order than the page shell, which can make it bind to a different task when both
aliases are present. Update the `taskIdFromUrl` lookup in `ChatArea` to use the
same alias precedence as `ChatPageDesktop` and the shared chat page, keeping the
`getFirstSearchParam` order consistent across these entry points so downstream
task/team sync logic sees the same task ID.
In `@frontend/src/lib/search-params.ts`:
- Around line 14-20: getFirstSearchParam currently stops on empty strings
because it only checks for non-null values, so alias fallback never runs when a
query key is present but blank. Update getFirstSearchParam to treat empty
results from getSearchParam as missing and continue iterating through keys until
a non-empty value is found, while preserving the existing return type and
behavior for null/undefined cases.
🪄 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: ae693c1b-3cab-4997-9594-1226869d803b
📒 Files selected for processing (37)
backend/app/api/ws/chat_namespace.pybackend/app/services/chat/task_device_resolution.pybackend/app/services/chat/trigger/lifecycle.pybackend/app/services/execution/dispatcher.pybackend/tests/api/ws/test_device_capabilities_state.pybackend/tests/services/chat/storage/test_task_manager.pybackend/tests/services/chat/test_task_device_resolution.pybackend/tests/services/execution/test_responses_api_event_parser.pydocs/en/developer-guide/local-device-architecture.mddocs/zh/developer-guide/local-device-architecture.mdexecutor/src/agents/codex.rsexecutor/src/agents/mod.rsexecutor/src/claude_session.rsexecutor/src/emitter/mod.rsexecutor/src/local/backend/tasks.rsexecutor/src/local/capabilities.rsexecutor/src/local/capabilities/support.rsexecutor/src/process/mod.rsexecutor/src/runner/mod.rsexecutor/src/services/skill_deployer.rsexecutor/src/stream/mod.rsexecutor/tests/agent_command_contract.rsexecutor/tests/agent_process_engine_contract.rsexecutor/tests/codex_app_server_contract.rsexecutor/tests/docker_runtime_contract.rsexecutor/tests/local_backend_contract.rsexecutor/tests/local_capability_sync_contract.rsfrontend/src/__tests__/app/(tasks)/chat/ChatPageDesktop.remote-workspace.test.tsxfrontend/src/__tests__/app/(tasks)/chat/ChatPageMobile.remote-workspace.test.tsxfrontend/src/__tests__/features/tasks/components/chat/ChatArea.queue-message-handler.test.tsxfrontend/src/__tests__/features/tasks/components/sidebar/TaskSidebar.test.tsxfrontend/src/app/(tasks)/chat/ChatPageDesktop.tsxfrontend/src/app/(tasks)/chat/ChatPageMobile.tsxfrontend/src/app/(tasks)/chat/page.tsxfrontend/src/features/tasks/components/chat/ChatArea.tsxfrontend/src/features/tasks/components/sidebar/TaskSidebar.tsxfrontend/src/lib/search-params.ts
Comment on lines
+100
to
+128
| candidate = resolve_local_executor_device_id( | ||
| db, | ||
| user_id=user_id, | ||
| device_id=device_id, | ||
| ) | ||
| if not candidate: | ||
| return None | ||
|
|
||
| local_devices = _list_local_devices(db, user_id=user_id) | ||
| online_device_ids = await _online_local_device_ids( | ||
| user_id=user_id, | ||
| local_devices=local_devices, | ||
| ) | ||
| if candidate in online_device_ids: | ||
| return candidate | ||
|
|
||
| local_device_ids = { | ||
| device_id | ||
| for device in _list_local_devices( | ||
| db, | ||
| user_id=user_id, | ||
| include_inactive=True, | ||
| ) | ||
| if (device_id := _clean_string(device.name)) | ||
| } | ||
| if candidate in local_device_ids and len(online_device_ids) == 1: | ||
| return next(iter(online_device_ids)) | ||
|
|
||
| return candidate |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Handle inactive appDeviceId aliases during online-device fallback.
resolve_local_executor_device_id() only searches active device rows, and the fallback in resolve_online_local_executor_device_id() only recognizes device.name as a local alias. If a project/task stores the frontend appDeviceId and that device later becomes inactive, this path keeps the stale app ID instead of switching to the sole online executor. The new tests only cover appDeviceId while the device record is still active, so this regression would slip through.
Also applies to: 143-166
🤖 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/chat/task_device_resolution.py` around lines 100 - 128,
The online-device fallback in resolve_online_local_executor_device_id() only
treats device.name as a valid local alias, so inactive appDeviceId values are
not recognized and the stale ID can be returned. Update the alias check to
include the frontend appDeviceId from the local device records when building the
local identifier set, using _list_local_devices(..., include_inactive=True) and
the same normalization helper, so the fallback can switch to the sole online
executor when the stored device is inactive.
Comment on lines
+517
to
+523
| fn read_claude_settings(settings_path: &PathBuf) -> Value { | ||
| fs::read_to_string(settings_path) | ||
| .ok() | ||
| .and_then(|content| serde_json::from_str::<Value>(&content).ok()) | ||
| .filter(|value| value.is_object()) | ||
| .unwrap_or_else(|| Value::Object(Map::new())) | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether invalid settings.json cases are covered before changing behavior.
rg -n -C3 'read_claude_settings|write_claude_file_edit_hook_settings|settings\.json' executor/src executor/testsRepository: wecode-ai/Wegent
Length of output: 8925
🏁 Script executed:
#!/bin/bash
sed -n '500,525p' executor/src/agents/mod.rs
printf '\n---\n'
sed -n '520,700p' executor/tests/agent_process_engine_contract.rs
printf '\n---\n'
sed -n '500,740p' executor/tests/local_capability_sync_contract.rsRepository: wecode-ai/Wegent
Length of output: 17092
🏁 Script executed:
#!/bin/bash
rg -n -C3 'read_claude_settings|write_claude_file_edit_hook_settings|read_json_or_default|settings\.json' executor/src executor/tests
printf '\n---\n'
sed -n '1,220p' executor/src/agents/mod.rsRepository: wecode-ai/Wegent
Length of output: 19471
Avoid clobbering malformed Claude settings
read_claude_settings() turns any parse/type failure into {}, so an existing malformed or non-object settings.json gets rewritten and the user's config is lost. Return the error and skip hook injection instead.
🤖 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/src/agents/mod.rs` around lines 517 - 523, read_claude_settings
currently masks read/parse/type failures by converting them into an empty
object, which can overwrite a malformed or non-object Claude settings file.
Update read_claude_settings to return a result/error instead of always producing
Value::Object(Map::new()), and make the caller that injects hooks detect that
failure and skip writing settings when the existing file cannot be safely
parsed. Use read_claude_settings and the hook injection path in
executor/src/agents/mod.rs to locate the change.
Comment on lines
+430
to
+431
| const searchParamsString = stringifySearchParams(searchParams) | ||
| const taskIdFromUrl = getFirstSearchParam(searchParams, ['taskId', 'task_id', 'taskid']) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep task-id alias precedence consistent with the page shell.
ChatArea now prefers taskId before task_id, but frontend/src/app/(tasks)/chat/ChatPageDesktop.tsx and the shared chat page still do the reverse. If both aliases are present, the page and ChatArea will bind to different task IDs, which breaks the task/team sync logic below.
Proposed fix
- const taskIdFromUrl = getFirstSearchParam(searchParams, ['taskId', 'task_id', 'taskid'])
+ const taskIdFromUrl = getFirstSearchParam(searchParams, ['task_id', 'taskid', 'taskId'])📝 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
| const searchParamsString = stringifySearchParams(searchParams) | |
| const taskIdFromUrl = getFirstSearchParam(searchParams, ['taskId', 'task_id', 'taskid']) | |
| const searchParamsString = stringifySearchParams(searchParams) | |
| const taskIdFromUrl = getFirstSearchParam(searchParams, ['task_id', 'taskid', 'taskId']) |
🤖 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 `@frontend/src/features/tasks/components/chat/ChatArea.tsx` around lines 430 -
431, `ChatArea` is resolving task-id aliases in a different order than the page
shell, which can make it bind to a different task when both aliases are present.
Update the `taskIdFromUrl` lookup in `ChatArea` to use the same alias precedence
as `ChatPageDesktop` and the shared chat page, keeping the `getFirstSearchParam`
order consistent across these entry points so downstream task/team sync logic
sees the same task ID.
Comment on lines
+14
to
+20
| export function getFirstSearchParam( | ||
| searchParams: SearchParamsLike | null | undefined, | ||
| keys: string[] | ||
| ): string | null { | ||
| for (const key of keys) { | ||
| const value = getSearchParam(searchParams, key) | ||
| if (value !== null) return value |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Skip empty query values when falling back.
URLSearchParams.get() returns '' for ?taskId=. With the current !== null check, getFirstSearchParam() stops there and never checks later aliases, so a URL like ?taskId=&task_id=123 resolves as “no task” even though a valid fallback is present.
Proposed fix
export function getFirstSearchParam(
searchParams: SearchParamsLike | null | undefined,
keys: string[]
): string | null {
for (const key of keys) {
const value = getSearchParam(searchParams, key)
- if (value !== null) return value
+ if (value !== null && value !== '') return value
}
return null
}📝 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
| export function getFirstSearchParam( | |
| searchParams: SearchParamsLike | null | undefined, | |
| keys: string[] | |
| ): string | null { | |
| for (const key of keys) { | |
| const value = getSearchParam(searchParams, key) | |
| if (value !== null) return value | |
| export function getFirstSearchParam( | |
| searchParams: SearchParamsLike | null | undefined, | |
| keys: string[] | |
| ): string | null { | |
| for (const key of keys) { | |
| const value = getSearchParam(searchParams, key) | |
| if (value !== null && value !== '') return value | |
| } | |
| return null | |
| } |
🤖 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 `@frontend/src/lib/search-params.ts` around lines 14 - 20, getFirstSearchParam
currently stops on empty strings because it only checks for non-null values, so
alias fallback never runs when a query key is present but blank. Update
getFirstSearchParam to treat empty results from getSearchParam as missing and
continue iterating through keys until a non-empty value is found, while
preserving the existing return type and behavior for null/undefined cases.
# Conflicts: # executor/src/agents/mod.rs # executor/src/process/mod.rs # executor/src/stream/mod.rs # executor/tests/agent_process_engine_contract.rs
# Conflicts: # executor/tests/agent_runtime_capabilities_contract.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
executor/src/agents/runtime_capabilities.rs (2)
277-278: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep attachment paths unique per attachment.
Using only the sanitized original filename means two attachments named
image.pngin the same task/subtask overwrite each other, and both prompt records can point at the later file.Proposed fix
- let path = attachments_dir.join(sanitize_filename(&attachment.original_filename)); + let path = attachments_dir.join(format!( + "{}-{}", + attachment.id, + sanitize_filename(&attachment.original_filename) + ));🤖 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/src/agents/runtime_capabilities.rs` around lines 277 - 278, The attachment write in runtime_capabilities::add_attachment uses only sanitize_filename(&attachment.original_filename), so duplicate names can overwrite each other and leave prompt records pointing to the wrong file. Update the path construction in that attachment-saving flow to include a unique per-attachment component (for example from the attachment id or a generated unique suffix) while keeping the sanitized original name for readability. Make sure the uniqueness is applied before fs::write is called so each attachment gets its own distinct file path.
804-836: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve malformed Claude settings instead of replacing them.
A parse error or non-object
settings.jsoncurrently becomes{}and is written back, which can destroy the user's existing settings. Skip hook injection when the existing file cannot be safely parsed.Proposed fix
let settings_path = config_dir.join("settings.json"); - let mut settings = fs::read_to_string(&settings_path) - .ok() - .and_then(|content| serde_json::from_str::<Value>(&content).ok()) - .filter(Value::is_object) - .unwrap_or_else(|| json!({})); + let mut settings = match fs::read_to_string(&settings_path) { + Ok(content) => match serde_json::from_str::<Value>(&content) { + Ok(value) if value.is_object() => value, + _ => return, + }, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => json!({}), + Err(_) => return, + };🤖 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/src/agents/runtime_capabilities.rs` around lines 804 - 836, The settings update logic in the runtime capabilities hook installer is overwriting malformed or non-object Claude settings by falling back to an empty object; instead, in this settings.json flow, detect parse failures or non-object content before mutating and return early without writing anything. Update the code around the settings.json load, settings_object extraction, and write_json_file call so hook injection only happens when the existing file can be safely parsed as an object, preserving the user’s original settings when it cannot.
🤖 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/src/agents/runtime_capabilities.rs`:
- Around line 786-796: The write_claude_user_config helper is overwriting the
entire claude.json instead of preserving existing user settings. Update
write_claude_user_config to read the current Claude config from
config_dir.join("claude.json"), merge in only the Wegent-owned keys from the new
JSON value, and then write the merged result back using write_json_file. Keep
the change localized to write_claude_user_config and ensure unrelated fields
already present in the user’s Claude config remain untouched.
In `@executor/tests/agent_runtime_capabilities_contract.rs`:
- Around line 223-233: The one-shot test server tasks can hang forever if the
runtime never connects, so the current JoinHandle awaits in the agent runtime
capability contract tests need a timeout. Update the async test helpers around
the server spawn logic to wrap each server.await.unwrap() in
tokio::time::timeout, and apply the same pattern in the related test blocks that
use the spawned server handles so failures surface locally and quickly instead
of blocking the test suite.
---
Duplicate comments:
In `@executor/src/agents/runtime_capabilities.rs`:
- Around line 277-278: The attachment write in
runtime_capabilities::add_attachment uses only
sanitize_filename(&attachment.original_filename), so duplicate names can
overwrite each other and leave prompt records pointing to the wrong file. Update
the path construction in that attachment-saving flow to include a unique
per-attachment component (for example from the attachment id or a generated
unique suffix) while keeping the sanitized original name for readability. Make
sure the uniqueness is applied before fs::write is called so each attachment
gets its own distinct file path.
- Around line 804-836: The settings update logic in the runtime capabilities
hook installer is overwriting malformed or non-object Claude settings by falling
back to an empty object; instead, in this settings.json flow, detect parse
failures or non-object content before mutating and return early without writing
anything. Update the code around the settings.json load, settings_object
extraction, and write_json_file call so hook injection only happens when the
existing file can be safely parsed as an object, preserving the user’s original
settings when it cannot.
🪄 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: a92ed43c-10a8-4ce0-ab66-e3b09c5136cd
📒 Files selected for processing (11)
executor/src/agents/codex.rsexecutor/src/agents/mod.rsexecutor/src/agents/runtime_capabilities.rsexecutor/src/claude_session.rsexecutor/src/process/mod.rsexecutor/src/runner/mod.rsexecutor/src/stream/mod.rsexecutor/tests/agent_command_contract.rsexecutor/tests/agent_process_engine_contract.rsexecutor/tests/agent_runtime_capabilities_contract.rsexecutor/tests/codex_app_server_contract.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- executor/src/runner/mod.rs
- executor/src/stream/mod.rs
- executor/tests/agent_command_contract.rs
- executor/src/claude_session.rs
- executor/tests/codex_app_server_contract.rs
- executor/tests/agent_process_engine_contract.rs
- executor/src/agents/codex.rs
- executor/src/process/mod.rs
Contributor
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: 2
♻️ Duplicate comments (2)
executor/src/agents/runtime_capabilities.rs (2)
277-278: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep attachment paths unique per attachment.
Using only the sanitized original filename means two attachments named
image.pngin the same task/subtask overwrite each other, and both prompt records can point at the later file.Proposed fix
- let path = attachments_dir.join(sanitize_filename(&attachment.original_filename)); + let path = attachments_dir.join(format!( + "{}-{}", + attachment.id, + sanitize_filename(&attachment.original_filename) + ));🤖 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/src/agents/runtime_capabilities.rs` around lines 277 - 278, The attachment write in runtime_capabilities::add_attachment uses only sanitize_filename(&attachment.original_filename), so duplicate names can overwrite each other and leave prompt records pointing to the wrong file. Update the path construction in that attachment-saving flow to include a unique per-attachment component (for example from the attachment id or a generated unique suffix) while keeping the sanitized original name for readability. Make sure the uniqueness is applied before fs::write is called so each attachment gets its own distinct file path.
804-836: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve malformed Claude settings instead of replacing them.
A parse error or non-object
settings.jsoncurrently becomes{}and is written back, which can destroy the user's existing settings. Skip hook injection when the existing file cannot be safely parsed.Proposed fix
let settings_path = config_dir.join("settings.json"); - let mut settings = fs::read_to_string(&settings_path) - .ok() - .and_then(|content| serde_json::from_str::<Value>(&content).ok()) - .filter(Value::is_object) - .unwrap_or_else(|| json!({})); + let mut settings = match fs::read_to_string(&settings_path) { + Ok(content) => match serde_json::from_str::<Value>(&content) { + Ok(value) if value.is_object() => value, + _ => return, + }, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => json!({}), + Err(_) => return, + };🤖 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/src/agents/runtime_capabilities.rs` around lines 804 - 836, The settings update logic in the runtime capabilities hook installer is overwriting malformed or non-object Claude settings by falling back to an empty object; instead, in this settings.json flow, detect parse failures or non-object content before mutating and return early without writing anything. Update the code around the settings.json load, settings_object extraction, and write_json_file call so hook injection only happens when the existing file can be safely parsed as an object, preserving the user’s original settings when it cannot.
🤖 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/src/agents/runtime_capabilities.rs`:
- Around line 786-796: The write_claude_user_config helper is overwriting the
entire claude.json instead of preserving existing user settings. Update
write_claude_user_config to read the current Claude config from
config_dir.join("claude.json"), merge in only the Wegent-owned keys from the new
JSON value, and then write the merged result back using write_json_file. Keep
the change localized to write_claude_user_config and ensure unrelated fields
already present in the user’s Claude config remain untouched.
In `@executor/tests/agent_runtime_capabilities_contract.rs`:
- Around line 223-233: The one-shot test server tasks can hang forever if the
runtime never connects, so the current JoinHandle awaits in the agent runtime
capability contract tests need a timeout. Update the async test helpers around
the server spawn logic to wrap each server.await.unwrap() in
tokio::time::timeout, and apply the same pattern in the related test blocks that
use the spawned server handles so failures surface locally and quickly instead
of blocking the test suite.
---
Duplicate comments:
In `@executor/src/agents/runtime_capabilities.rs`:
- Around line 277-278: The attachment write in
runtime_capabilities::add_attachment uses only
sanitize_filename(&attachment.original_filename), so duplicate names can
overwrite each other and leave prompt records pointing to the wrong file. Update
the path construction in that attachment-saving flow to include a unique
per-attachment component (for example from the attachment id or a generated
unique suffix) while keeping the sanitized original name for readability. Make
sure the uniqueness is applied before fs::write is called so each attachment
gets its own distinct file path.
- Around line 804-836: The settings update logic in the runtime capabilities
hook installer is overwriting malformed or non-object Claude settings by falling
back to an empty object; instead, in this settings.json flow, detect parse
failures or non-object content before mutating and return early without writing
anything. Update the code around the settings.json load, settings_object
extraction, and write_json_file call so hook injection only happens when the
existing file can be safely parsed as an object, preserving the user’s original
settings when it cannot.
🪄 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: a92ed43c-10a8-4ce0-ab66-e3b09c5136cd
📒 Files selected for processing (11)
executor/src/agents/codex.rsexecutor/src/agents/mod.rsexecutor/src/agents/runtime_capabilities.rsexecutor/src/claude_session.rsexecutor/src/process/mod.rsexecutor/src/runner/mod.rsexecutor/src/stream/mod.rsexecutor/tests/agent_command_contract.rsexecutor/tests/agent_process_engine_contract.rsexecutor/tests/agent_runtime_capabilities_contract.rsexecutor/tests/codex_app_server_contract.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- executor/src/runner/mod.rs
- executor/src/stream/mod.rs
- executor/tests/agent_command_contract.rs
- executor/src/claude_session.rs
- executor/tests/codex_app_server_contract.rs
- executor/tests/agent_process_engine_contract.rs
- executor/src/agents/codex.rs
- executor/src/process/mod.rs
🛑 Comments failed to post (2)
executor/src/agents/runtime_capabilities.rs (1)
786-796: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Merge Claude user config instead of overwriting it.
write_json_file(&config_dir.join("claude.json"), &value)replaces the whole existing Claude config under the active home directory. In local mode this can erase user settings unrelated to Wegent.Proposed fix
fn write_claude_user_config(config_dir: &Path) { - let value = json!({ - "numStartups": 2, - "installMethod": "unknown", - "autoUpdates": true, - "sonnet45MigrationComplete": true, - "hasCompletedOnboarding": true, - "bypassPermissionsModeAccepted": true, - "isQualifiedForDataSharing": false, - }); - let _ = write_json_file(&config_dir.join("claude.json"), &value); + let path = config_dir.join("claude.json"); + let mut value = fs::read_to_string(&path) + .ok() + .and_then(|content| serde_json::from_str::<Value>(&content).ok()) + .filter(Value::is_object) + .unwrap_or_else(|| json!({})); + if let Some(object) = value.as_object_mut() { + object.entry("numStartups".to_owned()).or_insert(json!(2)); + object.entry("installMethod".to_owned()).or_insert(json!("unknown")); + object.entry("autoUpdates".to_owned()).or_insert(json!(true)); + object + .entry("sonnet45MigrationComplete".to_owned()) + .or_insert(json!(true)); + object + .entry("hasCompletedOnboarding".to_owned()) + .or_insert(json!(true)); + object + .entry("bypassPermissionsModeAccepted".to_owned()) + .or_insert(json!(true)); + object + .entry("isQualifiedForDataSharing".to_owned()) + .or_insert(json!(false)); + } + let _ = write_json_file(&path, &value); }📝 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.fn write_claude_user_config(config_dir: &Path) { let path = config_dir.join("claude.json"); let mut value = fs::read_to_string(&path) .ok() .and_then(|content| serde_json::from_str::<Value>(&content).ok()) .filter(Value::is_object) .unwrap_or_else(|| json!({})); if let Some(object) = value.as_object_mut() { object.entry("numStartups".to_owned()).or_insert(json!(2)); object.entry("installMethod".to_owned()).or_insert(json!("unknown")); object.entry("autoUpdates".to_owned()).or_insert(json!(true)); object .entry("sonnet45MigrationComplete".to_owned()) .or_insert(json!(true)); object .entry("hasCompletedOnboarding".to_owned()) .or_insert(json!(true)); object .entry("bypassPermissionsModeAccepted".to_owned()) .or_insert(json!(true)); object .entry("isQualifiedForDataSharing".to_owned()) .or_insert(json!(false)); } let _ = write_json_file(&path, &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/src/agents/runtime_capabilities.rs` around lines 786 - 796, The write_claude_user_config helper is overwriting the entire claude.json instead of preserving existing user settings. Update write_claude_user_config to read the current Claude config from config_dir.join("claude.json"), merge in only the Wegent-owned keys from the new JSON value, and then write the merged result back using write_json_file. Keep the change localized to write_claude_user_config and ensure unrelated fields already present in the user’s Claude config remain untouched.executor/tests/agent_runtime_capabilities_contract.rs (1)
223-233: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Bound one-shot test servers with a timeout.
If the runtime regresses and never connects,
server.await.unwrap()waits forever. Wrap these JoinHandle awaits withtokio::time::timeoutso failures are reported locally and quickly.Proposed fix
- server.await.unwrap(); + tokio::time::timeout(std::time::Duration::from_secs(5), server) + .await + .expect("mock backend did not receive the expected request") + .unwrap();Also applies to: 268-270, 281-295, 331-339
🤖 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/agent_runtime_capabilities_contract.rs` around lines 223 - 233, The one-shot test server tasks can hang forever if the runtime never connects, so the current JoinHandle awaits in the agent runtime capability contract tests need a timeout. Update the async test helpers around the server spawn logic to wrap each server.await.unwrap() in tokio::time::timeout, and apply the same pattern in the related test blocks that use the spawned server handles so failures surface locally and quickly instead of blocking the test suite.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
executor/tests/agent_command_contract.rs (1)
407-426: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winIsolate executor-home in this test as well.
Line 407 only overrides
WORKSPACE_ROOT, but the code under test now reads saved sessions from the executor-home path. If the processHOMEor temp fallback already containssessions/77/.claude_session_id_987, this assertion becomes machine-state dependent. PointWEGENT_EXECUTOR_HOMEorHOMEat a freshunique_dir()here too. As per coding guidelines, "keep tests isolated."🤖 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/agent_command_contract.rs` around lines 407 - 426, This test is still coupled to ambient machine state because `build_claude_command` can read saved sessions from executor-home, not just `WORKSPACE_ROOT`. Update `claude_command_ignores_legacy_workspace_task_session_file` to also point `WEGENT_EXECUTOR_HOME` or `HOME` at a fresh `unique_dir()` so the session lookup cannot pick up an existing `sessions/77/.claude_session_id_987`. Keep the setup fully isolated alongside the existing `env_lock`, `EnvGuard::set`, and `unique_dir` usage.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/app/services/execution/response_bridge.py`:
- Around line 147-159: Update EmitterBridgeTransport._convert_event so it
mirrors the current ResponsesAPIEventParser contract instead of the legacy
mapping. Make sure response.created is translated to START, and split
function-call completion into TOOL_ARGUMENT_DONE for
response.function_call_arguments.done followed by TOOL_RESULT for
response.output_item.done when the item is a function call. Keep the DONE
handling in response_completed consistent with the parser’s event flow and reuse
the existing event type symbols in response_bridge.py to align in-process
execution with SSE/callback mode.
In `@executor/src/claude_session.rs`:
- Around line 114-116: The readable_session_file_candidates helper currently
only returns writable_session_file_candidates, which breaks resuming older
sessions stored in legacy workspace-root locations. Update
readable_session_file_candidates to include both the new executor-home candidate
path(s) and the legacy workspace-root .claude_session_id* locations so existing
sessions remain discoverable, while keeping the cleanup logic for legacy files
separate and unchanged.
In `@frontend/src/features/tasks/components/message/MessageBubble.tsx`:
- Around line 526-530: `hasThinkingToolSteps` in `MessageBubble` only checks
`step.details.type`, so nested tool events inside `details.message.content[]`
are missed and `ThinkingDisplay` is gated off incorrectly. Update the
`React.useMemo` logic around `hasThinkingToolSteps` to also scan `msg.thinking`
entries for tool-related content nested in `details.message.content` for
`assistant`/`user` payloads, and make the gate at `ThinkingDisplay` use that
broader detection so nested `tool_use`/`tool_result` steps are rendered.
In `@frontend/src/features/tasks/components/message/thinking/ThinkingDisplay.tsx`:
- Around line 38-43: The renderer selection in ThinkingDisplay.tsx is too narrow
because hasTextTypeSteps(thinking) only detects direct details.type === 'text',
so nested text content inside details.message.content for assistant/user
ThinkingStep entries can be sent to the wrong view. Update the view-selection
logic in ThinkingDisplay to also treat these message-based text steps as
detailed thinking, and route them to DetailedThinkingView instead of falling
through to ToolBlocksView. Use the existing hasTextTypeSteps predicate and the
ThinkingStep shape as the place to expand the detection.
---
Outside diff comments:
In `@executor/tests/agent_command_contract.rs`:
- Around line 407-426: This test is still coupled to ambient machine state
because `build_claude_command` can read saved sessions from executor-home, not
just `WORKSPACE_ROOT`. Update
`claude_command_ignores_legacy_workspace_task_session_file` to also point
`WEGENT_EXECUTOR_HOME` or `HOME` at a fresh `unique_dir()` so the session lookup
cannot pick up an existing `sessions/77/.claude_session_id_987`. Keep the setup
fully isolated alongside the existing `env_lock`, `EnvGuard::set`, and
`unique_dir` usage.
🪄 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: 246fee81-d50f-495d-a6c1-68fc643bc7b3
📒 Files selected for processing (16)
backend/app/services/execution/dispatcher.pybackend/app/services/execution/response_bridge.pybackend/tests/services/execution/test_responses_api_event_parser.pyexecutor/src/claude_session.rsexecutor/src/emitter/mod.rsexecutor/src/process/mod.rsexecutor/src/stream/mod.rsexecutor/tests/agent_command_contract.rsexecutor/tests/agent_process_engine_contract.rsexecutor/tests/claude_stream_event_contract.rsexecutor/tests/local_backend_contract.rsfrontend/src/__tests__/features/tasks/components/message/MessageBubble.test.tsxfrontend/src/__tests__/features/tasks/components/message/thinking/ThinkingDisplay.test.tsxfrontend/src/features/tasks/components/message/MessageBubble.tsxfrontend/src/features/tasks/components/message/thinking/ThinkingDisplay.tsxfrontend/src/features/tasks/components/message/thinking/types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- executor/tests/agent_process_engine_contract.rs
- executor/src/process/mod.rs
- executor/src/emitter/mod.rs
Comment on lines
147
to
159
| elif event_type == ResponsesAPIStreamEvents.RESPONSE_COMPLETED.value: | ||
| response_data = data.get("response", {}) | ||
| result = extract_completed_result(response_data) | ||
| if not result.get("reasoning_content") and self._reasoning_content: | ||
| result["reasoning_content"] = self._reasoning_content | ||
| return ExecutionEvent( | ||
| type=EventType.DONE.value, | ||
| task_id=self.task_id, | ||
| subtask_id=self.subtask_id, | ||
| content="", | ||
| result=extract_completed_result(response_data), | ||
| result=result, | ||
| message_id=message_id, | ||
| ) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Mirror the parser’s event contract in EmitterBridgeTransport._convert_event.
ResponsesAPIEventParser now emits response.created -> START and splits function-call completion into TOOL_ARGUMENT_DONE (response.function_call_arguments.done) followed by TOOL_RESULT (response.output_item.done(function_call)), but this bridge still uses the old mapping. That leaves in-process execution on a different event contract than SSE/callback mode.
Suggested direction
+ if event_type == ResponsesAPIStreamEvents.RESPONSE_CREATED.value:
+ start_data = {
+ key: data.get(key)
+ for key in ("shell_type", "bot_name")
+ if data.get(key) is not None
+ }
+ return ExecutionEvent(
+ type=EventType.START.value,
+ task_id=self.task_id,
+ subtask_id=self.subtask_id,
+ data=start_data,
+ message_id=message_id,
+ )
+
elif event_type == ResponsesAPIStreamEvents.FUNCTION_CALL_ARGUMENTS_DONE.value:
...
return ExecutionEvent(
- type=EventType.TOOL_RESULT.value,
+ type=EventType.TOOL_ARGUMENT_DONE.value,
...
)
elif event_type == ResponsesAPIStreamEvents.OUTPUT_ITEM_DONE.value:
item = data.get("item", {})
+ if item.get("type") == "function_call":
+ ...
+ return ExecutionEvent(
+ type=EventType.TOOL_RESULT.value,
+ ...
+ )
if item.get("type") != "shell_call":
return None
elif event_type in (
- ResponsesAPIStreamEvents.RESPONSE_CREATED.value,
ResponsesAPIStreamEvents.RESPONSE_IN_PROGRESS.value,
...
):Also applies to: 411-414
🤖 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/execution/response_bridge.py` around lines 147 - 159,
Update EmitterBridgeTransport._convert_event so it mirrors the current
ResponsesAPIEventParser contract instead of the legacy mapping. Make sure
response.created is translated to START, and split function-call completion into
TOOL_ARGUMENT_DONE for response.function_call_arguments.done followed by
TOOL_RESULT for response.output_item.done when the item is a function call. Keep
the DONE handling in response_completed consistent with the parser’s event flow
and reuse the existing event type symbols in response_bridge.py to align
in-process execution with SSE/callback mode.
Comment on lines
+114
to
+116
| fn readable_session_file_candidates(request: &ExecutionRequest) -> Vec<PathBuf> { | ||
| writable_session_file_candidates(request) | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
This drops resume compatibility for existing legacy session files.
Line 115 now reads only from the executor-home path, so any pre-existing .claude_session_id* saved under workspace roots becomes unreadable even though those same paths are still treated as legacy state worth deleting later. That means in-flight tasks created before this path migration will stop resuming after rollout.
Suggested fix
fn readable_session_file_candidates(request: &ExecutionRequest) -> Vec<PathBuf> {
- writable_session_file_candidates(request)
+ let mut candidates = writable_session_file_candidates(request);
+ candidates.extend(legacy_workspace_session_file_candidates(request));
+ dedup_paths(candidates)
}📝 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
| fn readable_session_file_candidates(request: &ExecutionRequest) -> Vec<PathBuf> { | |
| writable_session_file_candidates(request) | |
| } | |
| fn readable_session_file_candidates(request: &ExecutionRequest) -> Vec<PathBuf> { | |
| let mut candidates = writable_session_file_candidates(request); | |
| candidates.extend(legacy_workspace_session_file_candidates(request)); | |
| dedup_paths(candidates) | |
| } |
🤖 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/src/claude_session.rs` around lines 114 - 116, The
readable_session_file_candidates helper currently only returns
writable_session_file_candidates, which breaks resuming older sessions stored in
legacy workspace-root locations. Update readable_session_file_candidates to
include both the new executor-home candidate path(s) and the legacy
workspace-root .claude_session_id* locations so existing sessions remain
discoverable, while keeping the cleanup logic for legacy files separate and
unchanged.
Comment on lines
+526
to
+530
| const hasThinkingToolSteps = React.useMemo( | ||
| () => | ||
| msg.thinking?.some( | ||
| step => step.details?.type === 'tool_use' || step.details?.type === 'tool_result' | ||
| ) ?? false, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Nested tool events in msg.thinking will be hidden by this gate.
Line 528 only treats direct details.type === 'tool_use' | 'tool_result' as tool steps. Valid ThinkingStep payloads also carry tool events inside details.message.content[] for assistant/user messages, so hasThinkingToolSteps stays false for those cases and the ThinkingDisplay gate at Lines 1419-1423 suppresses them whenever result.blocks is non-empty.
Suggested fix
const hasThinkingToolSteps = React.useMemo(
() =>
msg.thinking?.some(
- step => step.details?.type === 'tool_use' || step.details?.type === 'tool_result'
+ step =>
+ step.details?.type === "tool_use" ||
+ step.details?.type === "tool_result" ||
+ (step.details?.message?.content?.some(
+ item => item.type === "tool_use" || item.type === "tool_result"
+ ) ?? false)
) ?? false,
[msg.thinking]
)Also applies to: 1419-1423
🤖 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 `@frontend/src/features/tasks/components/message/MessageBubble.tsx` around
lines 526 - 530, `hasThinkingToolSteps` in `MessageBubble` only checks
`step.details.type`, so nested tool events inside `details.message.content[]`
are missed and `ThinkingDisplay` is gated off incorrectly. Update the
`React.useMemo` logic around `hasThinkingToolSteps` to also scan `msg.thinking`
entries for tool-related content nested in `details.message.content` for
`assistant`/`user` payloads, and make the gate at `ThinkingDisplay` use that
broader detection so nested `tool_use`/`tool_result` steps are rendered.
Comment on lines
+38
to
+43
| // Text-type steps (e.g. deep research) need the detailed renderer. | ||
| if (hasTextTypeSteps(thinking)) { | ||
| return <DetailedThinkingView thinking={thinking} taskStatus={taskStatus} /> | ||
| } | ||
|
|
||
| // Default to detailed view for ClaudeCode, Agno, and other shell types | ||
| return <DetailedThinkingView thinking={thinking} taskStatus={taskStatus} /> | ||
| return <ToolBlocksView thinking={thinking} taskStatus={taskStatus} /> |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Nested text thinking can now be routed to the wrong view.
Line 39 makes the renderer choice depend only on hasTextTypeSteps(thinking), but that predicate only catches direct details.type === 'text'. ThinkingStep also carries text inside details.message.content[] for assistant/user items, and DetailedThinkingView is the component that actually renders that shape, so those entries now fall through to ToolBlocksView.
Suggested fix
function hasTextTypeSteps(thinking: ThinkingDisplayProps['thinking']): boolean {
if (!thinking || thinking.length === 0) return false
- return thinking.some(step => step.details?.type === 'text')
+ return thinking.some(step => {
+ if (step.details?.type === 'text') return true
+ return (
+ step.details?.message?.content?.some(
+ content => content.type === 'text' && Boolean(content.text?.trim())
+ ) ?? false
+ )
+ })
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/features/tasks/components/message/thinking/ThinkingDisplay.tsx`
around lines 38 - 43, The renderer selection in ThinkingDisplay.tsx is too
narrow because hasTextTypeSteps(thinking) only detects direct details.type ===
'text', so nested text content inside details.message.content for assistant/user
ThinkingStep entries can be sent to the wrong view. Update the view-selection
logic in ThinkingDisplay to also treat these message-based text steps as
detailed thinking, and route them to DetailedThinkingView instead of falling
through to ToolBlocksView. Use the existing hasTextTypeSteps predicate and the
ThinkingStep shape as the place to expand the detection.
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
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
response.createdas a start event and includes shell metadata; streamed reasoning is preserved through completion.