feat(desktop): support adaptive tool timeouts#404
Open
karry-0803 wants to merge 17 commits into
Open
Conversation
…nd fix code corruption
github-actions
Bot
added
area:frontend
|
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:
📝 WalkthroughWalkthroughThis PR adds model-driven timeout selection for tool execution and reorganizes the conversation panel's scroll and event handling. Tool definitions now advertise an optional timeout metadata field; during execution, the model's requested timeout is parsed, validated, clamped to safe bounds (1s–10m), and enforced in both MCP and built-in tool execution engines. The conversation panel removes its toolbar, refactors auto-scroll logic with ResizeObserver, and updates event contracts and exposed methods. ChangesAdaptive tool timeout feature
Conversation panel UI reorganization
Sequence Diagram(s)sequenceDiagram
participant executeToolCall
participant parseRequestedTimeoutMs
participant builtInToolService
participant mcpManager
executeToolCall->>parseRequestedTimeoutMs: extract timeoutMs from _meta
parseRequestedTimeoutMs-->>executeToolCall: requestedTimeoutMs + sanitizedToolArgs
executeToolCall->>builtInToolService: executeTool with requestedTimeoutMs
executeToolCall->>mcpManager: executeTool with requestedTimeoutMs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
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: 5
♻️ Duplicate comments (1)
apps/desktop/src/services/BuiltInToolService/tools/bash/index.ts (1)
157-161: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDuplicates timeout clamping logic from MCP manager.
This clamping logic is identical to
McpManager.tslines 459-463. As suggested in the MCP manager review comment, extract this to a shared utility function to ensure consistency and maintainability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/services/BuiltInToolService/tools/bash/index.ts` around lines 157 - 161, The timeout clamping logic duplicated here should be extracted to a shared utility (e.g., create a function named clampTimeoutMs(defaultMs: number, requestedMs?: number): number) and used from both BuiltInToolService/tools/bash/index.ts and McpManager.ts; replace the inline computation of effectiveTimeout (currently using config.timeoutMs and context.requestedTimeoutMs) with a call to clampTimeoutMs(config.timeoutMs, context.requestedTimeoutMs) so both modules share the same implementation and bounds (1_000 to 600_000 ms).
🤖 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 `@apps/desktop/src/services/AgentService/catalog/tools.ts`:
- Around line 14-24: Update TOOL_TIMEOUT_META_PROPERTY to document the minimum
timeout and enforce it in the schema: amend the description for timeoutMs to
mention "Minimum: 1000 (1 second); Maximum: 600000 (10 minutes)" and add a
minimum: 1000 numeric constraint to the timeoutMs property so the schema matches
the runtime clamping in McpManager.ts and bash/index.ts; change references in
TOOL_TIMEOUT_META_PROPERTY and the timeoutMs property accordingly.
In `@apps/desktop/src/services/AgentService/execution/executor.ts`:
- Around line 326-353: The runtime validation in parseRequestedTimeoutMs
currently allows floats because it only checks Number.isFinite(timeoutMs);
update the check for the timeoutMs variable in parseRequestedTimeoutMs to
require an integer (e.g., ensure typeof timeoutMs === 'number' &&
Number.isFinite(timeoutMs) && Number.isInteger(timeoutMs)) so only integer
timeoutMs values (matching the 'integer' schema in tools.ts) are accepted; leave
the sanitizedToolArgs flow and deletion of TOOL_TIMEOUT_META_KEY unchanged.
In `@apps/desktop/src/services/AgentService/infrastructure/mcp/McpManager.ts`:
- Around line 459-463: Extract the timeout clamping into a shared utility (e.g.,
clampToolTimeout with constants MIN_TOOL_TIMEOUT_MS and MAX_TOOL_TIMEOUT_MS) and
replace the inline expression Math.min(Math.max(options.requestedTimeoutMs,
1000), 600000) in McpManager (where effectiveTimeout is computed using
resolved.toolTimeout and options.requestedTimeoutMs) and the duplicate in
bash/index.ts with a call to clampToolTimeout(options.requestedTimeoutMs);
ensure the utility is exported and imported where used so both code paths use
the same bounds.
In `@apps/desktop/src/views/SearchView/components/ConversationPanel/index.vue`:
- Around line 239-247: handleTimelineJump interpolates messageId directly into
the selector which will break for IDs containing CSS metacharacters; update
handleTimelineJump to escape the messageId before using it in querySelector
(mirror the approach used in ConversationTimeline.detectActiveMarker), e.g. use
CSS.escape(messageId) with a safe fallback if CSS.escape is unavailable, and
then build the selector `[data-message-id="${escaped}"]` so timeline jumps work
for all valid IDs.
- Around line 317-332: Seed the scroll metrics on mount by calling
refreshScrollToBottomVisibility immediately after you start observing
messageListRef (in the onMounted block, right after
messageListObserver.observe(messageListRef.value)) in addition to the existing
nextTick call, so ConversationTimeline gets real
scrollTop/scrollHeight/clientHeight values before any user scroll; reference
messageListRef, messageListObserver, refreshScrollToBottomVisibility, onMounted
and ConversationTimeline when making this change.
---
Duplicate comments:
In `@apps/desktop/src/services/BuiltInToolService/tools/bash/index.ts`:
- Around line 157-161: The timeout clamping logic duplicated here should be
extracted to a shared utility (e.g., create a function named
clampTimeoutMs(defaultMs: number, requestedMs?: number): number) and used from
both BuiltInToolService/tools/bash/index.ts and McpManager.ts; replace the
inline computation of effectiveTimeout (currently using config.timeoutMs and
context.requestedTimeoutMs) with a call to clampTimeoutMs(config.timeoutMs,
context.requestedTimeoutMs) so both modules share the same implementation and
bounds (1_000 to 600_000 ms).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 72d24a40-0e50-428d-bc71-cd7df2ccfc3a
📒 Files selected for processing (8)
apps/desktop/src/services/AgentService/catalog/tools.tsapps/desktop/src/services/AgentService/execution/executor.tsapps/desktop/src/services/AgentService/infrastructure/mcp/McpManager.tsapps/desktop/src/services/BuiltInToolService/service.tsapps/desktop/src/services/BuiltInToolService/tools/bash/index.tsapps/desktop/src/services/BuiltInToolService/types.tsapps/desktop/src/views/SearchView/components/ConversationPanel/components/ConversationTimeline.vueapps/desktop/src/views/SearchView/components/ConversationPanel/index.vue
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-05-30T12:53:52.487Z
Learnt from: hiqiancheng
Repo: TouchAI-org/TouchAI PR: 301
File: apps/desktop/src/views/SearchView/components/ConversationPanel/components/ToolApprovalHistoryCard.vue:68-80
Timestamp: 2026-05-30T12:53:52.487Z
Learning: In `apps/desktop/src/views/SearchView/components/ConversationPanel/components/ToolApprovalHistoryCard.vue` (Vue 3, desktop app), `ToolApprovalHistoryCard` intentionally only renders `rejected` and `cancelled` tool approval statuses. Approved approvals are excluded by design — they have no actionable content and do not need to appear in the conversation history. `isResolved` checking only `rejected`/`cancelled` is correct behavior, not a bug.
Applied to files:
apps/desktop/src/views/SearchView/components/ConversationPanel/index.vue
🔇 Additional comments (7)
apps/desktop/src/services/AgentService/catalog/tools.ts (1)
55-61: LGTM!apps/desktop/src/services/AgentService/execution/executor.ts (1)
747-749: LGTM!Also applies to: 753-753, 763-763, 772-772, 778-778
apps/desktop/src/services/AgentService/infrastructure/mcp/McpManager.ts (1)
441-441: LGTM!Also applies to: 467-471
apps/desktop/src/services/BuiltInToolService/types.ts (1)
45-45: LGTM!apps/desktop/src/services/BuiltInToolService/service.ts (1)
52-52: LGTM!Also applies to: 261-261
apps/desktop/src/services/BuiltInToolService/tools/bash/index.ts (1)
163-173: LGTM!apps/desktop/src/views/SearchView/components/ConversationPanel/components/ConversationTimeline.vue (1)
171-182: LGTM!
Comment on lines
239
to
+247
| function handleTimelineJump(messageId: string) { | ||
| const didJump = scrollToUserMessageTop( | ||
| messageId, | ||
| USER_MESSAGE_SCROLL_GAP + TIMELINE_JUMP_OFFSET | ||
| const element = conversationContainer.value?.querySelector( | ||
| `[data-message-id="${messageId}"]` | ||
| ); | ||
| if (!didJump) { | ||
| return; | ||
| if (element) { | ||
| element.scrollIntoView({ behavior: 'smooth', block: 'start' }); | ||
| markUserScrollIntent(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Escape messageId before building the selector.
messageId is interpolated directly into [data-message-id="..."] here. If an ID ever contains selector metacharacters, timeline jumps will silently fail even though ConversationTimeline.detectActiveMarker() already escapes this same lookup path.
Proposed fix
function handleTimelineJump(messageId: string) {
- const element = conversationContainer.value?.querySelector(
- `[data-message-id="${messageId}"]`
- );
+ const escapedId =
+ typeof CSS !== 'undefined' && typeof CSS.escape === 'function'
+ ? CSS.escape(messageId)
+ : messageId;
+ const element = conversationContainer.value?.querySelector(
+ `[data-message-id="${escapedId}"]`
+ );
if (element) {
element.scrollIntoView({ behavior: 'smooth', block: 'start' });
markUserScrollIntent();
}
}📝 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
| function handleTimelineJump(messageId: string) { | |
| const didJump = scrollToUserMessageTop( | |
| messageId, | |
| USER_MESSAGE_SCROLL_GAP + TIMELINE_JUMP_OFFSET | |
| const element = conversationContainer.value?.querySelector( | |
| `[data-message-id="${messageId}"]` | |
| ); | |
| if (!didJump) { | |
| return; | |
| if (element) { | |
| element.scrollIntoView({ behavior: 'smooth', block: 'start' }); | |
| markUserScrollIntent(); | |
| } | |
| } | |
| function handleTimelineJump(messageId: string) { | |
| const escapedId = | |
| typeof CSS !== 'undefined' && typeof CSS.escape === 'function' | |
| ? CSS.escape(messageId) | |
| : messageId; | |
| const element = conversationContainer.value?.querySelector( | |
| `[data-message-id="${escapedId}"]` | |
| ); | |
| if (element) { | |
| element.scrollIntoView({ behavior: 'smooth', block: 'start' }); | |
| markUserScrollIntent(); | |
| } | |
| } |
🤖 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 `@apps/desktop/src/views/SearchView/components/ConversationPanel/index.vue`
around lines 239 - 247, handleTimelineJump interpolates messageId directly into
the selector which will break for IDs containing CSS metacharacters; update
handleTimelineJump to escape the messageId before using it in querySelector
(mirror the approach used in ConversationTimeline.detectActiveMarker), e.g. use
CSS.escape(messageId) with a safe fallback if CSS.escape is unavailable, and
then build the selector `[data-message-id="${escaped}"]` so timeline jumps work
for all valid IDs.
Comment on lines
+317
to
+332
| // 监听消息列表高度变化，用于自动滚动 | ||
| onMounted(() => { | ||
| if (messageListRef.value) { | ||
| messageListObserver = new ResizeObserver(() => { | ||
| if (!shouldAutoScrollOnOutput()) { | ||
| emitLatestContentVisibility(); | ||
| return; | ||
| } | ||
|
|
||
| nextTick(() => { | ||
| if (isAutoScrollEnabled.value && !props.historyOpen) { | ||
| syncToBottom(); | ||
| }); | ||
| } | ||
| refreshScrollToBottomVisibility(); | ||
| }); | ||
| messageListObserver.observe(messageListRef.value); | ||
| } | ||
|
|
||
| // 初始化滚动条状态 | ||
| nextTick(() => { | ||
| refreshScrollToBottomVisibility(); | ||
| }); |
There was a problem hiding this comment.
Seed the scroll metrics once on mount.
The new observer path only updates scrollTop/scrollHeight/clientHeight after a scroll event. Until then they stay at 0, so ConversationTimeline starts from stale metrics and can render with no active marker on first paint.
Proposed fix
+function syncContainerMetrics() {
+ const container = conversationContainer.value;
+ if (!container) return;
+
+ scrollTop.value = container.scrollTop;
+ scrollHeight.value = container.scrollHeight;
+ clientHeight.value = container.clientHeight;
+ lastScrollTop.value = container.scrollTop;
+}
+
// 监听消息列表高度变化，用于自动滚动
onMounted(() => {
if (messageListRef.value) {
messageListObserver = new ResizeObserver(() => {
if (isAutoScrollEnabled.value && !props.historyOpen) {
syncToBottom();
}
refreshScrollToBottomVisibility();
});
messageListObserver.observe(messageListRef.value);
}
// 初始化滚动条状态
nextTick(() => {
+ syncContainerMetrics();
+ emitLatestContentVisibility();
refreshScrollToBottomVisibility();
});
});🤖 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 `@apps/desktop/src/views/SearchView/components/ConversationPanel/index.vue`
around lines 317 - 332, Seed the scroll metrics on mount by calling
refreshScrollToBottomVisibility immediately after you start observing
messageListRef (in the onMounted block, right after
messageListObserver.observe(messageListRef.value)) in addition to the existing
nextTick call, so ConversationTimeline gets real
scrollTop/scrollHeight/clientHeight values before any user scroll; reference
messageListRef, messageListObserver, refreshScrollToBottomVisibility, onMounted
and ConversationTimeline when making this change.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/desktop/src/services/AgentService/execution/executor.ts (1)
366-366:⚠️ Potential issue | 🟡 Minor
timeoutMsvalidation accepts non-integer values.The
_meta.timeoutMsschema advertisestype: 'integer', but the runtime check here only requires a finite number, so floats (e.g.1500.5) slip through. AddingNumber.isInteger(timeoutMs)keeps runtime validation aligned with the schema. Downstream clamping still bounds the value, so this is defensive only.🤖 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 `@apps/desktop/src/services/AgentService/execution/executor.ts` at line 366, The runtime validation for timeoutMs in executor.ts is too permissive and allows floats despite _meta.timeoutMs being declared as an integer; update the check that currently uses typeof timeoutMs !== 'number' || !Number.isFinite(timeoutMs) to also require Number.isInteger(timeoutMs) so only integer values are accepted (e.g., use !Number.isInteger(timeoutMs) as part of the conditional) and keep the existing error path/clamping logic intact for downstream handling of out-of-range values.
🤖 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.
Duplicate comments:
In `@apps/desktop/src/services/AgentService/execution/executor.ts`:
- Line 366: The runtime validation for timeoutMs in executor.ts is too
permissive and allows floats despite _meta.timeoutMs being declared as an
integer; update the check that currently uses typeof timeoutMs !== 'number' ||
!Number.isFinite(timeoutMs) to also require Number.isInteger(timeoutMs) so only
integer values are accepted (e.g., use !Number.isInteger(timeoutMs) as part of
the conditional) and keep the existing error path/clamping logic intact for
downstream handling of out-of-range values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c6b06a0b-7e01-48d9-bb11-01ead2ce92fe
📒 Files selected for processing (1)
apps/desktop/src/services/AgentService/execution/executor.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Rust Checks
- GitHub Check: Frontend Tests
- GitHub Check: CodeQL (rust)
- GitHub Check: Desktop E2E Smoke (Windows)
🔇 Additional comments (5)
apps/desktop/src/services/AgentService/execution/executor.ts (5)
26-31: LGTM!
282-299: LGTM!
478-602: LGTM!
741-804: LGTM!
845-862: LGTM!
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/services/AgentService/execution/executor.ts (1)
354-360:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlways strip the injected
_metakey, even when it isn't a valid timeout object.The early-return path returns
sanitizedToolArgs: toolArgswithout removing_meta. SincewithAdaptiveTimeoutSchemainjects_metainto every tool schema, a model could populate it with a non-object value (string/number/array) ornull, and that key would then be forwarded to the tool. MCP servers validating withadditionalProperties: falsecould reject the call. Strip_metawhenever it's present, regardless oftimeoutMsvalidity.🛡️ Proposed fix to strip `_meta` unconditionally
const meta = toolArgs[TOOL_TIMEOUT_META_KEY]; + const sanitizedToolArgs = + TOOL_TIMEOUT_META_KEY in toolArgs ? { ...toolArgs } : toolArgs; + if (TOOL_TIMEOUT_META_KEY in sanitizedToolArgs) { + delete sanitizedToolArgs[TOOL_TIMEOUT_META_KEY]; + } if (!meta || typeof meta !== 'object' || Array.isArray(meta)) { return { requestedTimeoutMs: undefined, - sanitizedToolArgs: toolArgs, + sanitizedToolArgs, }; } - const sanitizedToolArgs = { ...toolArgs }; - delete sanitizedToolArgs[TOOL_TIMEOUT_META_KEY]; - const timeoutMs = (meta as Record<string, unknown>).timeoutMs;🤖 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 `@apps/desktop/src/services/AgentService/execution/executor.ts` around lines 354 - 360, The early-return currently returns sanitizedToolArgs: toolArgs leaving the injected TOOL_TIMEOUT_META_KEY present when meta is invalid; always remove that key regardless of meta validity. Copy toolArgs into a new sanitizedToolArgs, delete sanitizedToolArgs[TOOL_TIMEOUT_META_KEY], then inspect meta (from toolArgs[TOOL_TIMEOUT_META_KEY]) to set requestedTimeoutMs only if meta is an object and has a valid timeoutMs; finally return { requestedTimeoutMs, sanitizedToolArgs } so the _meta key is never forwarded to tools.
🤖 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.
Outside diff comments:
In `@apps/desktop/src/services/AgentService/execution/executor.ts`:
- Around line 354-360: The early-return currently returns sanitizedToolArgs:
toolArgs leaving the injected TOOL_TIMEOUT_META_KEY present when meta is
invalid; always remove that key regardless of meta validity. Copy toolArgs into
a new sanitizedToolArgs, delete sanitizedToolArgs[TOOL_TIMEOUT_META_KEY], then
inspect meta (from toolArgs[TOOL_TIMEOUT_META_KEY]) to set requestedTimeoutMs
only if meta is an object and has a valid timeoutMs; finally return {
requestedTimeoutMs, sanitizedToolArgs } so the _meta key is never forwarded to
tools.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3ab63ac0-9a28-430e-8a25-dacdf14de4e2
📒 Files selected for processing (5)
apps/desktop/src/services/AgentService/catalog/tools.tsapps/desktop/src/services/AgentService/execution/executor.tsapps/desktop/src/services/AgentService/infrastructure/mcp/McpManager.tsapps/desktop/src/services/BuiltInToolService/tools/bash/index.tsapps/desktop/src/utils/timeouts.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: CodeQL (rust)
- GitHub Check: Desktop E2E Smoke (Windows)
- GitHub Check: Rust Checks
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-05-30T12:53:52.512Z
Learnt from: hiqiancheng
Repo: TouchAI-org/TouchAI PR: 301
File: apps/desktop/src/services/AgentService/contracts/tooling.ts:86-94
Timestamp: 2026-05-30T12:53:52.512Z
Learning: In `apps/desktop/src/services/AgentService/contracts/tooling.ts`, the `AskUserAnswer` interface intentionally uses a flat shape (`selectedLabels: string[]`, `skipped: boolean`) rather than a discriminated union. The single-select constraint (only one element when `multiSelect` is false) and the empty-array invariant when `skipped: true` are enforced at runtime, not at the type level. JSDoc additions to document these contracts were deferred as a follow-up.
Applied to files:
apps/desktop/src/services/AgentService/catalog/tools.ts
🔇 Additional comments (7)
apps/desktop/src/services/AgentService/catalog/tools.ts (2)
14-26: LGTM!
44-64: LGTM!apps/desktop/src/services/AgentService/execution/executor.ts (2)
366-370: LGTM!
775-807: LGTM!apps/desktop/src/services/AgentService/infrastructure/mcp/McpManager.ts (1)
460-474: LGTM!apps/desktop/src/services/BuiltInToolService/tools/bash/index.ts (1)
158-176: LGTM!apps/desktop/src/utils/timeouts.ts (1)
1-13: LGTM!
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/services/AgentService/execution/executor.ts (1)
354-384:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLogic is correct; fix the Prettier formatting to unblock CI.
The clone-only-when-present pattern correctly avoids mutating the original
toolArgswhen no_metais supplied, and the integer guard from the prior review is in place. However, ESLint (prettier/prettier) flags the line break at 355–356, which will fail lint in CI.🎨 Proposed formatting fix
const meta = toolArgs[TOOL_TIMEOUT_META_KEY]; - const sanitizedToolArgs = - TOOL_TIMEOUT_META_KEY in toolArgs ? { ...toolArgs } : toolArgs; + const sanitizedToolArgs = TOOL_TIMEOUT_META_KEY in toolArgs ? { ...toolArgs } : toolArgs; if (TOOL_TIMEOUT_META_KEY in sanitizedToolArgs) { delete sanitizedToolArgs[TOOL_TIMEOUT_META_KEY]; }Note: negative or zero
timeoutMsvalues still pass this validation, but the downstreamclampTimeoutMs(..., 1000, 600000)in bothMcpManager.executeToolandexecuteBashToolbounds them to the 1s floor, so no separate guard is needed here.🤖 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 `@apps/desktop/src/services/AgentService/execution/executor.ts` around lines 354 - 384, Prettier is failing on the split assignment for sanitizedToolArgs; replace the broken two-line pattern with a single well-formatted statement using the existing TOOL_TIMEOUT_META_KEY and sanitizedToolArgs symbols (e.g., let sanitizedToolArgs = TOOL_TIMEOUT_META_KEY in toolArgs ? { ...toolArgs } : toolArgs;) then keep the subsequent conditional delete as-is (if (TOOL_TIMEOUT_META_KEY in sanitizedToolArgs) delete sanitizedToolArgs[TOOL_TIMEOUT_META_KEY];) so formatting/ESLint passes without changing the logic that clones only when the meta key is present.
🤖 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.
Outside diff comments:
In `@apps/desktop/src/services/AgentService/execution/executor.ts`:
- Around line 354-384: Prettier is failing on the split assignment for
sanitizedToolArgs; replace the broken two-line pattern with a single
well-formatted statement using the existing TOOL_TIMEOUT_META_KEY and
sanitizedToolArgs symbols (e.g., let sanitizedToolArgs = TOOL_TIMEOUT_META_KEY
in toolArgs ? { ...toolArgs } : toolArgs;) then keep the subsequent conditional
delete as-is (if (TOOL_TIMEOUT_META_KEY in sanitizedToolArgs) delete
sanitizedToolArgs[TOOL_TIMEOUT_META_KEY];) so formatting/ESLint passes without
changing the logic that clones only when the meta key is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 25a47468-f707-4ccc-adca-7d86acb12af9
📒 Files selected for processing (1)
apps/desktop/src/services/AgentService/execution/executor.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Desktop E2E Smoke (Windows)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (rust)
- GitHub Check: Frontend Quality
- GitHub Check: Rust Checks
- GitHub Check: Frontend Tests
🧰 Additional context used
🪛 ESLint
apps/desktop/src/services/AgentService/execution/executor.ts
[error] 355-356: Delete ⏎·······
(prettier/prettier)
🔇 Additional comments (1)
apps/desktop/src/services/AgentService/execution/executor.ts (1)
778-810: LGTM!
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
1 participant
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
This change allows the LLM to suggest tool call timeout values dynamically instead of relying only on static defaults.
User-facing impact:
_meta.timeoutMsas execution metadataImplementation summary:
_meta.timeoutMsin tool schemas before tools are provided to the modelRelated issue or RFC
Link the issue or RFC:
For code changes, link the tracking issue. Only documentation wording, link fixes, or comment-only cleanups may skip the issue-first flow.
AI assistance disclosure
AgentService, MCP timeout handling, tool schema exposure, and built-in tool execution boundariesTesting evidence
List the commands you ran and the results you observed.
pnpm type:checkpnpm test:prpnpm test:coverage:rustcargo-tarpaulinis not installed locally, so CI coverage evidence should be used before mergepnpm test:e2eDid you follow TDD (test-first) for feature and fix work? Strongly recommended. See docs/testing/testing.md.
Risk notes
AgentService, runtime, MCP, or schema impact:_meta.timeoutMs1000msand maximum of600000msScreenshots or recordings
_meta.timeoutMsexposed in tool schemaChecklist
[WIP]or similar title prefixes.AgentService, runtime, MCP, or schema boundaries, there is an accepted RFC.pnpm test:prfor this code PR, or this is a docs-only change.pnpm test:coverage:rustor relied on CI coverage evidence.pnpm test:e2elocally or documented why CI is the first valid proof.