feat(mcp): add per-MCP-server tool execution timeout - #4472
Merged
Pratham-Mishra04 merged 11 commits intoJul 2, 2026
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds per-client ChangesPer-server tool execution timeout
Related issue: 4446 Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant UI as MCP Client Sheet
participant Handler as MCP HTTP Handler
participant ConfigStore as configstore RDB
participant Manager as MCP Tool Manager
UI->>Handler: submit tool_execution_timeout
Handler->>Handler: resolve PATCH value
Handler->>ConfigStore: persist timeout seconds
Handler->>Manager: UpdateMCPClient(timeout)
Manager->>Manager: copy timeout into execution config
Manager->>Manager: executeToolInternal selects per-client timeout
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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: 1
🧹 Nitpick comments (1)
core/schemas/mcp_json_test.go (1)
86-95: ⚡ Quick winAdd an explicit
tool_execution_timeout: 0test case.The contract for this PR includes “omitted or set to 0 uses global,” but this file currently validates only the omitted-path. Please add a dedicated
{"tool_execution_timeout":0}unmarshal test to lock that behavior.🤖 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 `@core/schemas/mcp_json_test.go` around lines 86 - 95, Add a new test function to explicitly validate the behavior when tool_execution_timeout is set to 0 in the JSON. Create a test similar to TestMCPClientConfigUnmarshalToolExecutionTimeoutNotSet (which tests the omitted case) but with JSON that includes "tool_execution_timeout":0, and verify that cfg.ToolExecutionTimeout equals 0 after unmarshaling. This ensures the contract that both omitted and explicitly zero-valued tool_execution_timeout fields use the global timeout is tested at both code paths.
🤖 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 `@core/schemas/mcp.go`:
- Around line 353-373: The tool execution timeout parsing logic currently
accepts negative values for both the string duration format (after
time.ParseDuration) and the bare integer seconds format (after json.Unmarshal of
n as int64), which violates the schema contract requiring minimum: 0. Add
validation checks after parsing both formats to reject negative values by
returning an error if the parsed duration or integer is less than zero before
assigning to c.ToolExecutionTimeout. Apply this validation consistently wherever
tool_execution_timeout is parsed during unmarshal.
---
Nitpick comments:
In `@core/schemas/mcp_json_test.go`:
- Around line 86-95: Add a new test function to explicitly validate the behavior
when tool_execution_timeout is set to 0 in the JSON. Create a test similar to
TestMCPClientConfigUnmarshalToolExecutionTimeoutNotSet (which tests the omitted
case) but with JSON that includes "tool_execution_timeout":0, and verify that
cfg.ToolExecutionTimeout equals 0 after unmarshaling. This ensures the contract
that both omitted and explicitly zero-valued tool_execution_timeout fields use
the global timeout is tested at both code paths.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 55d39fe3-5ffe-48dd-b0dd-e6807790f865
📒 Files selected for processing (4)
core/mcp/toolmanager.gocore/schemas/mcp.gocore/schemas/mcp_json_test.gotransports/config.schema.json
Contributor
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Purvi09
force-pushed
the
feat/per-mcp-server-tool-timeout
branch
from
June 16, 2026 19:01
46993c5 to
87bc6e1
Compare
Contributor
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
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 `@transports/bifrost-http/lib/config.go`:
- Around line 1792-1797: The tool_execution_timeout validation in the
bifrost-http config is incorrectly rejecting valid sub-second duration values
like "500ms" instead of accepting and rounding them for database persistence.
Rather than checking if clientConfig.ToolExecutionTimeout is not a whole number
of seconds and returning an error, remove the rejection logic and instead
convert the duration to whole seconds by rounding up using math.Ceil, storing
the result as an integer for the database (where 0 represents "use global
default"). This same fix needs to be applied in two locations within the file:
at the primary validation site (lines 1792-1797) and at the secondary validation
site (line 1817), ensuring both enforce the same rounding behavior rather than
rejection.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c7cd3b99-22a0-4580-94a0-f6889e2509c0
📒 Files selected for processing (5)
core/mcp/clientmanager.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/mcp.gotransports/bifrost-http/lib/config.go
Collaborator
|
Hey @Purvi09 thanks for the PR! The direction looks good - just a few pointers before its merge ready
|
Purvi09
force-pushed
the
feat/per-mcp-server-tool-timeout
branch
from
June 17, 2026 13:38
62947b3 to
c3abcba
Compare
Contributor
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 (2)
framework/configstore/rdb.go (1)
2144-2154:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce non-negative timeout on update path for DB invariant parity
Line 2153 writes
tool_execution_timeoutwithout validation, but create path (Line 1983-1986) rejects negatives. This can persist invalid values and break field-level contract consistency between create/update flows.Suggested fix
// Update only editable fields using a map to avoid updating connection info // Connection info (ConnectionType, ConnectionString, StdioConfig) is read-only and should not be modified via API + if clientConfigCopy.ToolExecutionTimeout < 0 { + return fmt.Errorf("tool_execution_timeout must be non-negative, got %d", clientConfigCopy.ToolExecutionTimeout) + } + updates := map[string]interface{}{ "name": clientConfigCopy.Name, "is_code_mode_client": clientConfigCopy.IsCodeModeClient, "tools_to_execute_json": string(toolsToExecuteJSON), "tools_to_auto_execute_json": string(toolsToAutoExecuteJSON), "headers_json": headersJSONStr, "allowed_extra_headers_json": string(allowedExtraHeadersJSON), "tool_pricing_json": string(toolPricingJSON), "tool_sync_interval": clientConfigCopy.ToolSyncInterval, "tool_execution_timeout": clientConfigCopy.ToolExecutionTimeout, "allow_on_all_virtual_keys": clientConfigCopy.AllowOnAllVirtualKeys, "disabled": clientConfigCopy.Disabled, "updated_at": time.Now(), }🤖 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 `@framework/configstore/rdb.go` around lines 2144 - 2154, The update path in the method containing the updates map assignment does not validate that tool_execution_timeout is non-negative before adding it to the updates map, while the create path (around lines 1983-1986) enforces this validation. Add a check to ensure clientConfigCopy.ToolExecutionTimeout is non-negative and reject or handle negative values appropriately before including it in the updates map to maintain consistency between the create and update flows and preserve the field-level contract invariant.transports/bifrost-http/lib/config.go (1)
1786-1812:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject negative
ToolExecutionTimeoutbefore ceil conversion.
math.Ceilcan turn invalid negative sub-second durations into0(e.g.,-500ms), which collides with the valid sentinel meaning “use global timeout.” Add an explicit< 0check before conversion.💡 Suggested fix
func mcpClientConfigToTable(clientConfig *schemas.MCPClientConfig) (configstoreTables.TableMCPClient, error) { if clientConfig == nil { return configstoreTables.TableMCPClient{}, nil } if clientConfig.ToolSyncInterval%time.Second != 0 { return configstoreTables.TableMCPClient{}, fmt.Errorf( "tool_sync_interval must be a whole number of seconds, got %q", clientConfig.ToolSyncInterval.String(), ) } + if clientConfig.ToolExecutionTimeout < 0 { + return configstoreTables.TableMCPClient{}, fmt.Errorf( + "tool_execution_timeout must be >= 0, got %q", + clientConfig.ToolExecutionTimeout.String(), + ) + } authType := string(clientConfig.AuthType)As per coding guidelines,
tool_execution_timeoutmust preserve the schema contract (minimum: 0, with0reserved for global fallback).🤖 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 `@transports/bifrost-http/lib/config.go` around lines 1786 - 1812, Add an explicit validation check to reject negative ToolExecutionTimeout values before the math.Ceil conversion, similar to the existing ToolSyncInterval validation pattern. Insert a check that verifies clientConfig.ToolExecutionTimeout is not less than zero and returns a descriptive error if it is, ensuring negative durations cannot be silently converted to zero (which is a reserved sentinel value for using global timeout). This validation should occur before the return statement that constructs the configstoreTables.TableMCPClient object.Source: Coding guidelines
🧹 Nitpick comments (1)
core/schemas/mcp.go (1)
354-385: 💤 Low valueConsider extracting
ToolExecutionTimeoutparsing into a helper.The parsing logic for
tool_execution_timeout(string vs integer, negative check, overflow check) is duplicated across both code paths. This could be consolidated into a helper likeparseSecondsBasedDurationField(*json.RawMessage, string) (time.Duration, error).Not blocking since functionality is correct, but would reduce maintenance surface and potential for divergence.
Also applies to: 407-436
🤖 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 `@core/schemas/mcp.go` around lines 354 - 385, The ToolExecutionTimeout parsing logic contains duplicated code for handling both string duration format (like "30s") and integer seconds format, along with shared validation checks for negative values and overflow. Extract this duplicated logic into a helper function that accepts a json.RawMessage and field name, then returns the parsed time.Duration and any error. The helper should handle both the string duration parsing path and the integer seconds conversion path with all their validation rules, then replace both instances in the UnmarshalJSON method with calls to this single helper function.
🤖 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 `@framework/configstore/rdb.go`:
- Around line 2144-2154: The update path in the method containing the updates
map assignment does not validate that tool_execution_timeout is non-negative
before adding it to the updates map, while the create path (around lines
1983-1986) enforces this validation. Add a check to ensure
clientConfigCopy.ToolExecutionTimeout is non-negative and reject or handle
negative values appropriately before including it in the updates map to maintain
consistency between the create and update flows and preserve the field-level
contract invariant.
In `@transports/bifrost-http/lib/config.go`:
- Around line 1786-1812: Add an explicit validation check to reject negative
ToolExecutionTimeout values before the math.Ceil conversion, similar to the
existing ToolSyncInterval validation pattern. Insert a check that verifies
clientConfig.ToolExecutionTimeout is not less than zero and returns a
descriptive error if it is, ensuring negative durations cannot be silently
converted to zero (which is a reserved sentinel value for using global timeout).
This validation should occur before the return statement that constructs the
configstoreTables.TableMCPClient object.
---
Nitpick comments:
In `@core/schemas/mcp.go`:
- Around line 354-385: The ToolExecutionTimeout parsing logic contains
duplicated code for handling both string duration format (like "30s") and
integer seconds format, along with shared validation checks for negative values
and overflow. Extract this duplicated logic into a helper function that accepts
a json.RawMessage and field name, then returns the parsed time.Duration and any
error. The helper should handle both the string duration parsing path and the
integer seconds conversion path with all their validation rules, then replace
both instances in the UnmarshalJSON method with calls to this single helper
function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d4a8b9e-84f2-4202-a8a4-5ac1ee8a61e1
📒 Files selected for processing (9)
core/mcp/clientmanager.gocore/mcp/toolmanager.gocore/schemas/mcp.gocore/schemas/mcp_json_test.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/mcp.gotransports/bifrost-http/lib/config.gotransports/config.schema.json
🚧 Files skipped from review as they are similar to previous changes (6)
- framework/configstore/tables/mcp.go
- core/mcp/toolmanager.go
- framework/configstore/migrations.go
- core/schemas/mcp_json_test.go
- core/mcp/clientmanager.go
- transports/config.schema.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@core/internal/mcptests/per_server_timeout_test.go`:
- Around line 57-159: Migrate the three test functions
TestPerServerTimeout_OverridesGlobal,
TestPerServerTimeout_AllowsLongerThanGlobal, and
TestPerServerTimeout_FallsBackToGlobal from the custom setup pattern to the
standard test harness pattern required for core/internal/mcptests. Replace the
custom setupMCPManager and setupBifrost function calls with DynamicLLMMocker and
SetupAgentTest helpers to maintain consistency with the test suite. Preserve the
existing test logic and assertions while adapting the setup to use the
declarative pattern, then remove any custom setup functions that are no longer
needed once all tests have been migrated.
- Around line 79-86: The test is discarding the return values from
ExecuteChatMCPTool (line 81) and relying only on the elapsed time assertion,
which means the test could pass even if a different fast failure occurs instead
of a timeout. Capture the returned error/result from ExecuteChatMCPTool instead
of using blank identifiers, and add an explicit assertion that verifies the
timeout or cancellation error was actually returned (e.g., check for context
cancellation or timeout error). Apply the same fix to the second timeout-path
test at lines 151-158 where the same pattern occurs.
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 1021-1024: The updateMCPClient handler is not validating that
tool_execution_timeout values are non-negative before conversion and
persistence. Add a validation guard immediately after checking if
req.ToolExecutionTimeout is not nil to ensure the value is not negative, and
return a 400 error response if a negative value is provided. This validation
should occur before the time.Duration conversion to align with the schema
requirement that tool_execution_timeout has minimum: 0. Apply the same
validation logic at the other location mentioned (line 1195) where similar
timeout resolution occurs.
In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx`:
- Around line 824-834: Add a data-testid attribute to the Input component with
type="number" in the mcpClientSheet.tsx file. The attribute should have a
descriptive value that identifies this as a timeout input field, following the
naming convention used elsewhere in the codebase for E2E testing. Place the
data-testid attribute alongside the existing className, placeholder, value,
onChange, min, and type attributes.
- Around line 829-832: The onChange handler for the timeout input field uses
parseInt which silently truncates decimal values instead of rejecting them.
Replace the parseInt approach with strict numeric parsing by first parsing the
input value to a number, then validating that it is actually an integer value.
If the parsed value is not an integer, either reject the input or handle it
appropriately rather than silently truncating to an integer. This ensures that
decimal inputs like "1.9" are properly validated rather than being silently
converted to "1".
In `@ui/lib/types/schemas.ts`:
- Line 1080: The tool_execution_timeout field validation currently accepts any
number including fractional values, but the API contract expects integer seconds
only. Update the schema validation for the tool_execution_timeout field by
adding the int() method to the z.number() chain to enforce that only integer
values are accepted, preventing fractional numbers from passing validation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2180f493-8c26-4d96-8949-09adda8c511b
📒 Files selected for processing (8)
core/internal/mcptests/per_server_timeout_test.gocore/schemas/mcp.goframework/configstore/rdb.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.goui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/lib/types/mcp.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (1)
- ui/lib/types/mcp.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- transports/bifrost-http/lib/config.go
- core/schemas/mcp.go
- framework/configstore/rdb.go
Contributor
Author
Hi @Pratham-Mishra04, I have addressed all the points, please take a look. |
Purvi09
force-pushed
the
feat/per-mcp-server-tool-timeout
branch
from
June 18, 2026 13:56
f6d587c to
33291b8
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx (1)
829-836:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid silently truncating decimal timeout input values.
The current handler still coerces decimals via
Math.trunc, so1.9becomes1without user awareness. Prefer accepting only integer input and rejecting invalid/non-integer entries explicitly.Suggested fix
onChange={(e) => { if (e.target.value === "") { field.onChange(undefined); return; } const n = Number(e.target.value); - field.onChange(Number.isInteger(n) ? n : Math.trunc(n)); + field.onChange(Number.isInteger(n) && n >= 0 ? n : undefined); }}🤖 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 `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx` around lines 829 - 836, The onChange handler for the timeout field is silently truncating decimal values using Math.trunc, which provides no user feedback when entering values like 1.9. Replace the current logic that coerces decimals via Math.trunc with explicit integer validation: check if the parsed number is an integer using Number.isInteger, and if not, either reject the input entirely or display an explicit validation error/message to inform the user that only integers are accepted rather than silently truncating the value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx`:
- Around line 82-83: Replace Math.round(total) with Math.ceil(total) in the
return statement to ensure sub-second duration values are rounded up to at least
1 second instead of being collapsed to 0, which would incorrectly trigger the
global timeout fallback behavior on save and clear per-server timeout overrides.
---
Duplicate comments:
In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx`:
- Around line 829-836: The onChange handler for the timeout field is silently
truncating decimal values using Math.trunc, which provides no user feedback when
entering values like 1.9. Replace the current logic that coerces decimals via
Math.trunc with explicit integer validation: check if the parsed number is an
integer using Number.isInteger, and if not, either reject the input entirely or
display an explicit validation error/message to inform the user that only
integers are accepted rather than silently truncating the value.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e3995af5-c711-45db-be75-086c9597ef1a
📒 Files selected for processing (14)
core/internal/mcptests/per_server_timeout_test.gocore/mcp/clientmanager.gocore/mcp/toolmanager.gocore/schemas/mcp.gocore/schemas/mcp_json_test.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/mcp.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.gotransports/config.schema.jsonui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/lib/types/mcp.tsui/lib/types/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- core/mcp/clientmanager.go
- framework/configstore/tables/mcp.go
- core/mcp/toolmanager.go
- ui/lib/types/schemas.ts
- transports/bifrost-http/lib/config.go
- transports/config.schema.json
- transports/bifrost-http/handlers/mcp.go
- framework/configstore/migrations.go
- ui/lib/types/mcp.ts
- core/schemas/mcp_json_test.go
- core/internal/mcptests/per_server_timeout_test.go
- core/schemas/mcp.go
- framework/configstore/rdb.go
akshaydeo
dismissed
coderabbitai[bot]’s stale review
June 19, 2026 07:23
The merge-base changed after approval.
|
|
akshaydeo
dismissed
coderabbitai[bot]’s stale review
June 21, 2026 11:44
The merge-base changed after approval.
Pratham-Mishra04
requested changes
Jul 2, 2026
Pratham-Mishra04
left a comment
Pratham-Mishra04
left a comment
Collaborator
There was a problem hiding this comment.
Hey @Purvi09 added 2 comments, rest looks good!
Purvi09
force-pushed
the
feat/per-mcp-server-tool-timeout
branch
from
July 2, 2026 10:42
5691819 to
eb77584
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx (1)
795-846: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTimeout placeholder doesn't show the actual global default.
Line 827 hardcodes
placeholder="0", while the analogoustool_sync_intervalfield above (line 782) shows the real global value viaplaceholder={String(globalToolSyncInterval)}sourced frombifrostConfig?.client_config?.mcp_tool_sync_interval. Since Bifrost exposes an equivalent globaltool_execution_timeoutsetting undertool_manager_config(aliased asclient_config.mcp_tool_execution_timeoutat runtime), showing "0" as the placeholder is inconsistent with the sibling field and doesn't tell users what value will actually apply when "Using global setting" is shown.♻️ Suggested fix
+ const globalToolExecutionTimeout = bifrostConfig?.client_config?.mcp_tool_execution_timeout ?? 30; ... <Input type="number" className={`w-24 ${isUsingGlobal ? "text-muted-foreground" : ""}`} - placeholder="0" + placeholder={String(globalToolExecutionTimeout)}As per path instructions, "Preserve workspace UI patterns: use existing shared components/constants... before introducing one-off UI conventions." Please confirm the exact field name/units exposed for the global tool execution timeout in
bifrostConfig.client_configbefore applying this fix.🤖 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 `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx` around lines 795 - 846, The Tool Execution Timeout field is hardcoding a placeholder of 0 instead of showing the actual global default, which makes it inconsistent with the neighboring tool_sync_interval input. Update the mcpClientSheet.tsx FormField for tool_execution_timeout to source the placeholder from the same bifrostConfig.client_config value used for the global setting, and verify the exact runtime field name/units for the global tool execution timeout before wiring it in. Keep the existing isUsingGlobal logic and shared UI pattern intact, matching the sibling field’s behavior.core/schemas/mcp.go (1)
427-437: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the schema package
Marshal()wrapper here.
core/schemascustom marshalers should call the package-level wrapper instead ofjson.Marshal.♻️ Proposed fix
- return json.Marshal(s) + return Marshal(s)Based on learnings, custom
MarshalJSONmethods incore/schemasmust invoke the package-levelMarshal()wrapper rather thanencoding/json.Marshaldirectly.🤖 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 `@core/schemas/mcp.go` around lines 427 - 437, The MCPClientConfig.MarshalJSON method is bypassing the schema package’s custom marshal path by calling encoding/json.Marshal directly. Update this method to use the package-level Marshal() wrapper for the shadow struct instead, keeping the ToolExecutionTimeout formatting logic intact and preserving the alias-based serialization in MCPClientConfig.Source: Learnings
🤖 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 `@framework/configstore/rdb.go`:
- Around line 129-130: `CreateMCPClientConfig` is allowing negative
`ToolExecutionTimeout` values to be persisted because it converts them via
`toolExecutionTimeoutDurationToStoredSeconds` without validation, and
`math.Ceil` can hide small negative durations as 0. Add the same non-negative
validation used on update before storing the config, and reject invalid timeouts
in the create path so direct configstore callers cannot save them. Keep the fix
centered around `CreateMCPClientConfig` and the timeout conversion helper in
`rdb.go`.
---
Nitpick comments:
In `@core/schemas/mcp.go`:
- Around line 427-437: The MCPClientConfig.MarshalJSON method is bypassing the
schema package’s custom marshal path by calling encoding/json.Marshal directly.
Update this method to use the package-level Marshal() wrapper for the shadow
struct instead, keeping the ToolExecutionTimeout formatting logic intact and
preserving the alias-based serialization in MCPClientConfig.
In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx`:
- Around line 795-846: The Tool Execution Timeout field is hardcoding a
placeholder of 0 instead of showing the actual global default, which makes it
inconsistent with the neighboring tool_sync_interval input. Update the
mcpClientSheet.tsx FormField for tool_execution_timeout to source the
placeholder from the same bifrostConfig.client_config value used for the global
setting, and verify the exact runtime field name/units for the global tool
execution timeout before wiring it in. Keep the existing isUsingGlobal logic and
shared UI pattern intact, matching the sibling field’s behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 423803c2-2b59-444a-bdf1-1e305c28426b
📒 Files selected for processing (14)
core/internal/mcptests/per_server_timeout_test.gocore/mcp/clientmanager.gocore/mcp/toolmanager.gocore/schemas/mcp.gocore/schemas/mcp_json_test.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/mcp.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.gotransports/config.schema.jsonui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/lib/types/mcp.tsui/lib/types/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- core/mcp/clientmanager.go
- ui/lib/types/mcp.ts
- framework/configstore/tables/mcp.go
- framework/configstore/migrations.go
- transports/config.schema.json
- transports/bifrost-http/lib/config.go
- ui/lib/types/schemas.ts
- core/schemas/mcp_json_test.go
- core/mcp/toolmanager.go
- core/internal/mcptests/per_server_timeout_test.go
- transports/bifrost-http/handlers/mcp.go
Pratham-Mishra04
requested changes
Jul 2, 2026
Pratham-Mishra04
left a comment
Pratham-Mishra04
left a comment
Collaborator
There was a problem hiding this comment.
hey @Purvi09 one more comment
| Migrate: func(tx *gorm.DB) error { | ||
| tx = tx.WithContext(ctx) | ||
| migrator := tx.Migrator() | ||
| if !migrator.HasColumn(&tables.TableMCPClient{}, "tool_execution_timeout") { |
Collaborator
There was a problem hiding this comment.
we have added new helper methods for this
Contributor
Author
There was a problem hiding this comment.
Hi @Pratham-Mishra04 addressed this as well.
Pratham-Mishra04
approved these changes
Jul 2, 2026
… schema, and assert timeout error in tests
…reject decimal timeout input
Purvi09
force-pushed
the
feat/per-mcp-server-tool-timeout
branch
from
July 2, 2026 13:57
e60aa03 to
d1ba436
Compare
yangtuooc
added a commit
to yangtuooc/bifrost
that referenced
this pull request
Jul 2, 2026
* upstream/dev: feat(mcp): add per-MCP-server tool execution timeout (maximhq#4472) fix: billing on failed responses stream requests anthropic and bedrock (maximhq#4842) fix: gemini openai through signature compatibility (maximhq#4810) fix: cancelled state in logs (maximhq#4831) fix: perplexity responses api compatibility (maximhq#4813) docs: clarify two-layer token refresh behavior and disabled-client refresh token expiry (maximhq#4849) fix: skip background token refresh for disabled/unconfigured MCP clients and guarantee non-nil logger in sync workers (maximhq#4848)
2 tasks
2 tasks
This was referenced Jul 8, 2026
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
3 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
Adds a per-MCP-server
tool_execution_timeoutfield toMCPClientConfig, allowing each MCP server to declare its own tool execution timeout instead of sharing the single global value fromtool_manager_config. When set, it overrides the global for that server only. Omitting it (or setting0) falls back to the global default.Changes
ToolExecutionTimeout time.DurationtoMCPClientConfigincore/schemas/mcp.goMCPClientConfig.UnmarshalJSONto parse the new field — accepts a Go duration string (e.g."30s","2m") or a bare integer treated as seconds, matching the behaviour oftool_manager_config.tool_execution_timeoutexecuteToolInternalincore/mcp/toolmanager.goto checkexecutionConfig.ToolExecutionTimeoutbefore falling back to the global timeouttool_execution_timeoutto themcp_client_configdefinition intransports/config.schema.jsonfor IDE autocomplete/validationtool_execution_timeouttoconfig_mcp_clientswith migrationadd_mcp_client_tool_execution_timeout_columntool_sync_intervaltool_execution_timeoutto the update request handler intransports/bifrost-http/handlers/mcp.gowith negative value validationcore/schemas/mcp_json_test.gocovering: duration string, bare integer, field not set, explicit zero, invalid string, negative integer, and negative stringcore/internal/mcptests/per_server_timeout_test.gocovering: per-server timeout overrides global, per-server timeout allows tool to complete, and zero timeout falls back to globalDesign decision: Bare integers are treated as seconds (not nanoseconds) to match
tool_manager_config.tool_execution_timeoutbehaviour and to be practical for timeout values. This intentionally differs fromtool_sync_interval, which treats bare integers as nanoseconds following Go'stime.Durationconvention.Type of change
Affected areas
How to test
To test end-to-end via config file, add
tool_execution_timeoutto a server in yourconfig.json:{ "client_configs": [ { "name": "myslowserver", "connection_type": "http", "connection_string": "http://localhost:8080", "tool_execution_timeout": "5s" } ] }Tools on
myslowserverwill now time out after 5 seconds regardless of the global timeout. Other servers continue to use the global default.You can also set the timeout via the UI — open the MCP client sheet and set "Tool Execution Timeout (seconds)". The value is stored in Postgres and survives restarts.
Screenshots/Recordings
N/A
Breaking changes
Related issues
Closes #4446
Security considerations
None — this is a timeout configuration field only. No auth, secrets, or PII involved.
Checklist
docs/contributing/README.mdand followed the guidelines