feat(agent): make MCP tool call timeout configurable - #18016
Open
var4dev wants to merge 6 commits into
Open
Conversation
- add Tool timeout field to agent settings (Advanced Settings, shown when MCP tools are connected, default 10s) - pass the value through to the actual MCP tool call instead of the hardcoded 10s - keep 10s default for existing agents without the field - add unit test verifying default_timeout is applied
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:
📝 WalkthroughWalkthroughThe change adds configurable MCP tool-call timeouts. The backend applies normalized defaults, the agent form exposes the setting when MCP tools exist, and English, Russian, and Chinese translations describe the field. ChangesTool timeout configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant LLMToolPluginCallSession
participant MCPTool
Agent->>LLMToolPluginCallSession: Create session with default_timeout
LLMToolPluginCallSession->>MCPTool: Call with resolved timeout
MCPTool-->>LLMToolPluginCallSession: Return tool result
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 2
🧹 Nitpick comments (1)
agent/tools/base.py (1)
51-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the resolved timeout.
The new timeout selection changes runtime behavior, but the existing tool-call logs do not record it. Include the resolved timeout in the invocation or completion log so operators can confirm whether an agent uses the configured value or the 10-second default.
As per coding guidelines,
**/*.py: Add logging for new flows.🤖 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 `@agent/tools/base.py` around lines 51 - 60, Update tool_call_async to log the resolved request_timeout after applying the default_timeout fallback, including it in the existing invocation or completion log so both configured and default timeout values are observable. Use the established logging flow and preserve the current timeout behavior.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 `@agent/tools/base.py`:
- Around line 51-60: Validate the timeout in the tool component constructor
before creating LLMToolPluginCallSession, rejecting values below the minimum of
1 instead of allowing invalid defaults to propagate. In agent/tools/base.py
around the constructor and default_timeout assignment, enforce this validation;
update agent/component/agent_with_tools.py lines 115-117 to stop converting
falsy tool_timeout values to 10 and pass the validated value consistently. The
form field at web/src/pages/agent/form/agent-form/index.tsx line 79 requires no
direct change unless needed to preserve submission of zero for server-side
validation.
In `@test/unit_test/agent/tools/test_llm_tool_plugin_session.py`:
- Around line 48-64: Update _BlockingSession to record the timeout supplied by
MCP calls, then assert exact propagation for the session default and explicit
request timeout instead of relying on elapsed-time thresholds. Add coverage for
the 10-second constructor default and the synchronous tool_call wrapper, using a
non-blocking session behavior so tests do not wait 10 seconds.
---
Nitpick comments:
In `@agent/tools/base.py`:
- Around line 51-60: Update tool_call_async to log the resolved request_timeout
after applying the default_timeout fallback, including it in the existing
invocation or completion log so both configured and default timeout values are
observable. Use the established logging flow and preserve the current timeout
behavior.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: a86f16de-ed27-4938-a6da-4a58aff1e8d2
📒 Files selected for processing (9)
agent/component/agent_with_tools.pyagent/tools/base.pytest/unit_test/agent/tools/test_llm_tool_plugin_session.pyweb/src/locales/en.tsweb/src/locales/ru.tsweb/src/locales/zh.tsweb/src/pages/agent/constant/index.tsxweb/src/pages/agent/form/agent-form/index.tsxweb/src/pages/agent/form/agent-form/use-values.ts
Comment on lines
+51
to
+60
| def __init__(self, tools_map: dict[str, object], callback: partial, default_timeout: float = 10): | ||
| self.tools_map = tools_map | ||
| self.callback = callback | ||
| self.default_timeout = default_timeout | ||
|
|
||
| def tool_call(self, name: str, arguments: dict[str, Any], timeout: float | int = 10) -> Any: | ||
| def tool_call(self, name: str, arguments: dict[str, Any], timeout: float | int | None = None) -> Any: | ||
| return asyncio.run(self.tool_call_async(name, arguments, request_timeout=timeout)) | ||
|
|
||
| async def tool_call_async(self, name: str, arguments: dict[str, Any], request_timeout: float | int = 10) -> Any: | ||
| async def tool_call_async(self, name: str, arguments: dict[str, Any], request_timeout: float | int | None = None) -> Any: | ||
| request_timeout = self.default_timeout if request_timeout is None else request_timeout |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)(base\.py$|agent_with_tools\.py$|agent-form/index\.tsx$)' || true
echo "== base.py outline =="
ast-grep outline agent/tools/base.py 2>/dev/null || true
echo "== base.py relevant lines =="
cat -n agent/tools/base.py | sed -n '1,140p'
echo "== agent_with_tools.py relevant lines =="
cat -n agent/component/agent_with_tools.py | sed -n '1,180p'
echo "== agent-form schema relevant lines =="
cat -n web/src/pages/agent/form/agent-form/index.tsx | sed -n '1,140p'
echo "== search timeout usages =="
rg -n "tool_timeout|default_timeout|request_timeout|toolcall_session|ToolPluginCallSession|LLMToolPluginCallSession|NumberInputStepper|min" agent web -SRepository: infiniflow/ragflow
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tracked target files =="
git ls-files | rg '(^|/)agent/tools/base\.py$|(^|/)agent/component/agent_with_tools\.py$|(^|/)web/src/pages/agent/form/agent-form/index\.tsx$' || true
echo "== base.py relevant lines =="
sed -n '1,160p' agent/tools/base.py
echo "== agent_with_tools.py relevant lines =="
sed -n '80,145p' agent/component/agent_with_tools.py
echo "== agent-form relevant lines =="
sed -n '55,95p' web/src/pages/agent/form/agent-form/index.tsx
echo "== focused timeout/tool_timeout usages (files only first) =="
rg -n -S --glob '!docs/**' --glob '!*.png' --glob '!*.jpg' --glob '!*.svg' --glob '!*.txt' \
'tool_timeout|default_timeout|request_timeout|LLMToolPluginCallSession|NumberInputStepper|min' agent web -s || true
echo "== behavioral probe: current truthiness/default expressions =="
python3 - <<'PY'
cases = [None, 0, "0", "14", -1, float("nan"), float("inf")]
for case in cases:
agent_fallback = float(case or 10) if case is not None else float(10)
base_default = 10 if case is None else case
print({"case": case, "agent_with_tools_fallback_value": agent_fallback, "tool_call_default_if_none": base_default})
PYRepository: infiniflow/ragflow
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== exact git status for target files =="
git status --short -- agent/tools/base.py agent/component/agent_with_tools.py web/src/pages/agent/form/agent-form/index.tsx || true
echo "== exact tracked target files =="
git ls-files -- agent/tools/base.py agent/component/agent_with_tools.py web/src/pages/agent/form/agent-form/index.tsx
echo "== exact target file sizes =="
wc -l agent/tools/base.py agent/component/agent_with_tools.py web/src/pages/agent/form/agent-form/index.tsx
echo "== sed exact target ranges =="
sed -n '1,145p' agent/tools/base.py
printf '\n--- agent_with_tools.py ---\n'
sed -n '90,130p' agent/component/agent_with_tools.py
printf '\n--- agent-form/schema ---\n'
sed -n '60,90p' web/src/pages/agent/form/agent-form/index.tsx
echo "== focused timeout/tool_timeout matches in tracked files =="
rg -n -S \
'tool_timeout|default_timeout|request_timeout|LLMToolPluginCallSession|NumberInputStepper|z\.coerce\.number\(\)\.optional|z\.number\(\).*optional|NumberInputStepper' \
agent/tools/base.py agent/component/agent_with_tools.py web/src/pages/agent/form/agent-form/index.tsx || true
printf '%s\n' '--- behavioral probe: truthiness/default expressions ---'
python3 - <<'PY'
cases = [
("missing", None, 10),
("0", 0, float(0)),
("empty string", "", float(0)),
("14", 14, float(14)),
("negative", -1, float(-1)),
("nan", float("nan"), float("nan")),
("inf", float("inf"), float("inf")),
]
print("agent/component fallback: float(value or 10) if value is not None else float(10)")
print("agent/tools default: 10 if request_timeout is None else request_timeout")
for label, value, fallback in cases:
agent_value_or_10 = float(value or 10)
default_value = 10 if value is None else value
print(label, "value=", repr(value), "agent fallback=", repr(fallback), "base default=", repr(default_value), "Python timeout passes?", timeout_is_valid(default_value))
PYRepository: infiniflow/ragflow
Length of output: 11777
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== common/mcp_tool_call_conn.py outline =="
ast-grep outline common/mcp_tool_call_conn.py 2>/dev/null || true
echo "== common/mcp_tool_call_conn.py relevant lines =="
sed -n '1,240p' common/mcp_tool_call_conn.py
echo "== focused call signature checks =="
rg -n -S \
'class MCPToolBinding|class MCPToolCallSession|class ToolCallSession|def (tool_call|tool_call_async|invoke)|thread_pool_exec|request_timeout|timeout' \
common/mcp_tool_call_conn.pyRepository: infiniflow/ragflow
Length of output: 13718
Validate tool_timeout at component construction.
The form accepts optional values, including zero, and agent/component/agent_with_tools.py#L115-L117 converts falsy values to 10. agent/tools/base.py#L60 then uses whatever default came from construction, so invalid settings can hide behind the fallback or propagate to MCP calls. Reject values below the intended minimum (for example, less than 1) before creating LLMToolPluginCallSession.
📍 Affects 3 files
agent/tools/base.py#L51-L60(this comment)agent/component/agent_with_tools.py#L115-L117web/src/pages/agent/form/agent-form/index.tsx#L79-L79
🤖 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 `@agent/tools/base.py` around lines 51 - 60, Validate the timeout in the tool
component constructor before creating LLMToolPluginCallSession, rejecting values
below the minimum of 1 instead of allowing invalid defaults to propagate. In
agent/tools/base.py around the constructor and default_timeout assignment,
enforce this validation; update agent/component/agent_with_tools.py lines
115-117 to stop converting falsy tool_timeout values to 10 and pass the
validated value consistently. The form field at
web/src/pages/agent/form/agent-form/index.tsx line 79 requires no direct change
unless needed to preserve submission of zero for server-side validation.
- fall back to the 10s default when tool_timeout is missing or below 1 - stop converting falsy tool_timeout values to 10 in agent_with_tools - log the resolved request_timeout on tool invocation - rewrite unit tests to record and assert the exact timeout received by the MCP session instead of relying on elapsed-time thresholds
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/unit_test/agent/tools/test_llm_tool_plugin_session.py (1)
64-68: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover configured values below
1.
LLMToolPluginCallSessionmapsNoneand values below1to the 10-second default. This test covers onlyNone. Add cases for0and a negative value so the required fallback remains protected.🤖 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 `@test/unit_test/agent/tools/test_llm_tool_plugin_session.py` around lines 64 - 68, Extend the timeout coverage in test_constructor_default_timeout_is_ten_seconds for LLMToolPluginCallSession by adding cases with configured timeouts of 0 and a negative value. Verify each call still returns "done" and recording.timeouts contains [10], preserving the existing None/default case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/unit_test/agent/tools/test_llm_tool_plugin_session.py`:
- Around line 64-68: Extend the timeout coverage in
test_constructor_default_timeout_is_ten_seconds for LLMToolPluginCallSession by
adding cases with configured timeouts of 0 and a negative value. Verify each
call still returns "done" and recording.timeouts contains [10], preserving the
existing None/default case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fea0c115-8176-4ba8-838f-cb41bca5c21f
📒 Files selected for processing (3)
agent/component/agent_with_tools.pyagent/tools/base.pytest/unit_test/agent/tools/test_llm_tool_plugin_session.py
🚧 Files skipped from review as they are similar to previous changes (1)
- agent/tools/base.py
Contributor
|
…espace conflict
The test lived in test/unit_test/agent/tools/, where pytest's prepend
import mode resolves 'agent' to the test folder as a namespace package
(no __init__.py), breaking 'from agent.tools.base import ...' at
collection ('cannot import name ... (unknown location)'). Move it to
test/unit_test/agent/ so the root 'agent' package is imported correctly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/unit_test/agent/test_llm_tool_plugin_session.py`:
- Around line 64-69: Extend test_constructor_default_timeout_is_ten_seconds to
include 0.5 in the configured values, preserving the existing assertion that a
positive timeout below one second falls back to a 10-second tool-call timeout.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: 1aa1c99b-668d-4367-9512-0ad684649d03
📒 Files selected for processing (1)
test/unit_test/agent/test_llm_tool_plugin_session.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #18016 +/- ##
=======================================
Coverage 90.65% 90.65%
=======================================
Files 10 10
Lines 717 717
Branches 118 118
=======================================
Hits 650 650
Misses 39 39
Partials 28 28 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Make the MCP tool call timeout configurable per agent.
Previously LLMToolPluginCallSession hardcoded a 10s timeout for every
tool call (including MCP tools). This PR:
when MCP tools are connected, default 10s);
the hardcoded 10s;
that an explicit per-call timeout overrides it.
Files touched: \�gent/tools/base.py, \�gent/component/agent_with_tools.py,
web agent form + locales (en/ru/zh), new unit test under
\ est/unit_test/agent/tools/.