fix(mcp): stop enable/disable toggle from corrupting tool_sync_interval - #5049
fix(mcp): stop enable/disable toggle from corrupting tool_sync_interval#5049Shaik-Sirajuddin wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds overflow-checked duration conversion and applies it to MCP timeout and sync-interval handling across core, HTTP, and global configuration paths. The MCP registry toggle now sends only ChangesMCP duration conversion and toggle fix
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 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 |
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (4): Last reviewed commit: "Merge branch 'dev' into worktree-fix-mcp..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The MCP client table's enable/disable Switch resent tool_sync_interval
(nanoseconds from GET) into a PUT field the backend parses as minutes,
overflowing int64 into a garbage negative duration for clients with a
non-default sync interval. The toggle now sends only {disabled},
relying on the update handler's existing PATCH semantics.
Adds DurationFromUnits, an overflow-guarded minute/second-to-Duration
helper, and uses it across the MCP client and global-config handlers.
Also fixes two related unit bugs found while auditing those call
sites (global tool_sync_interval reload used seconds instead of
minutes in two places), a pointer-aliasing hazard in config.go's
updateConfig that could leave live state corrupted on a rejected
request, and a validate-after-DB-write ordering issue in the client
update handler's global-default fallback.
Fixes maximhq#5026.
c6c8c0f to
6f50870
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
transports/bifrost-http/handlers/config.go (1)
345-369: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThree near-duplicate branches now decide the same two field values.
The prospective-value computation here mirrors the logic in the later mutation blocks (
MCPToolSyncInterval/MCPToolExecutionTimeouthandling further down in this function). Since this is precisely the kind of unit/logic drift that caused the original bug, consider derivingupdatedConfig.MCPToolSyncInterval/MCPToolExecutionTimeoutdirectly fromprospectiveToolSyncInterval/prospectiveToolExecutionTimeoutright after validation, and removing the now-redundant later assignments, so there's a single source of truth for "what value wins."🤖 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/handlers/config.go` around lines 345 - 369, The config update flow in configHandler duplicates the MCPToolSyncInterval and MCPToolExecutionTimeout decision logic in two places, which risks drift and inconsistent “winning” values. Keep the existing prospective value validation in configHandler, then use prospectiveToolSyncInterval and prospectiveToolExecutionTimeout as the single source of truth when assigning updatedConfig, removing the later redundant branches for those two fields so the mutation logic is centralized and consistent.transports/bifrost-http/handlers/mcp.go (1)
677-695: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect overflow-guarded conversion, but four near-identical blocks now duplicate the same request/config resolution logic.
Each block resolves
toolSyncIntervalthe same way (request value viaDurationFromUnits→ 400 on error; else config fallback → 500 on error), just with minor variations in howGetClientConfigerrors are handled. Worth extracting into a small helper (e.g.resolveToolSyncInterval(ctx, req.ToolSyncInterval, h.store.ConfigStore)) to avoid drift across the four copies as this logic evolves again.♻️ Sketch of extracted helper
func (h *MCPHandler) resolveToolSyncInterval(ctx *fasthttp.RequestCtx, reqVal int) (time.Duration, *schemas.BifrostError) { if reqVal != 0 { dur, err := schemas.DurationFromUnits(int64(reqVal), time.Minute, "tool_sync_interval") if err != nil { return 0, &schemas.BifrostError{StatusCode: fasthttp.StatusBadRequest, Message: err.Error()} } return dur, nil } config, err := h.store.ConfigStore.GetClientConfig(ctx) if err != nil || config == nil { return mcp.DefaultToolSyncInterval, nil } dur, durErr := schemas.DurationFromUnits(int64(config.MCPToolSyncInterval), time.Minute, "tool_sync_interval") if durErr != nil { return 0, &schemas.BifrostError{StatusCode: fasthttp.StatusInternalServerError, Message: durErr.Error()} } return dur, nil }Note: the existing blocks aren't fully identical today (some silently ignore
GetClientConfigerrors, others return 500) — that inconsistency predates this diff, so consolidating would also require picking one canonical behavior.Also applies to: 788-806, 890-912, 965-987
🤖 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/handlers/mcp.go` around lines 677 - 695, The toolSyncInterval resolution logic is duplicated across multiple handler blocks and should be consolidated to prevent divergence. Extract the shared request-vs-config fallback flow into a helper such as resolveToolSyncInterval on MCPHandler, using schemas.DurationFromUnits and the store ConfigStore/GetClientConfig path, then have each MCP handler call it and map its result to the existing HTTP error responses. Make sure the helper centralizes the canonical behavior for request value, config fallback, and error handling so all copies stay consistent.ui/lib/store/apis/mcpApi.ts (1)
176-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect unit conversion; extract the magic number given this exact class of bug.
data.tool_sync_interval * 60_000_000_000correctly converts minutes (request contract) to nanoseconds (cached/GET contract) — matches the documented contract incore/schemas/mcp.go'sUnmarshalJSONcomment andtransports/bifrost-http/lib/config.go's minutes-validation message. Given the bug this PR fixes was caused by exactly this kind of unit mismatch, naming the multiplier (e.g.NANOSECONDS_PER_MINUTE) would make future edits safer to eyeball-verify.♻️ Suggested naming
+const NANOSECONDS_PER_MINUTE = 60_000_000_000; + ... -if (data.tool_sync_interval !== undefined) - draft.clients[index].config.tool_sync_interval = data.tool_sync_interval * 60_000_000_000; +if (data.tool_sync_interval !== undefined) + draft.clients[index].config.tool_sync_interval = data.tool_sync_interval * NANOSECONDS_PER_MINUTE;🤖 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/lib/store/apis/mcpApi.ts` around lines 176 - 180, The optimistic cache update in mcpApi’s client config is using a raw unit-conversion constant, which is easy to misread and reintroduce as a minutes-vs-nanoseconds bug. Replace the inline multiplier in the tool_sync_interval assignment with a clearly named shared constant such as NANOSECONDS_PER_MINUTE, and use that in the relevant update path inside the mcpApi store logic so the conversion is obvious and consistent with the documented contract.tests/e2e/features/mcp-registry/mcp-registry-tool-sync-interval.spec.ts (1)
60-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid regression coverage; consider also asserting the actual PUT request body.
The test verifies the outcome (value preserved across toggle) thoroughly, including a second round-trip. To directly pin down the described mechanism ("send only
{disabled}"), you could additionally assert the request body captured viapage.waitForResponse's.request().postData()equals exactly{"disabled":true}— this would catch a future regression where the payload grows again but happens to not corrupt the value (e.g. if the backend became more lenient).🤖 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 `@tests/e2e/features/mcp-registry/mcp-registry-tool-sync-interval.spec.ts` around lines 60 - 102, The regression test in mcp-registry-tool-sync-interval.spec.ts already checks the preserved outcome, but it should also pin down the UI toggle payload itself. In the enabledSwitch click flow, use the existing page.waitForResponse assertions around the PUT to inspect the matching request body and verify it sends only the disabled field, not the raw tool_sync_interval value. Keep the current round-trip assertions in getClientConfig and the afterDisable/afterReEnable checks so the test covers both the request shape and the preserved interval behavior.
🤖 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 `@tests/e2e/features/mcp-registry/mcp-registry-tool-sync-interval.spec.ts`:
- Around line 73-75: The test is using a brittle role-based locator for the MCP
client enabled switch instead of the stable existing test id. Update the
selector in mcp-registry-tool-sync-interval.spec.ts to target the switch exposed
by mcpClientsTable.tsx via its data-testid pattern for the client id, and keep
the lookup anchored through mcpRegistryPage/getClientRow only as needed. Ensure
the test continues to use data-testid selectors consistently, in line with the
existing fixtures/import conventions from tests/e2e/core/fixtures/base.fixture.
---
Nitpick comments:
In `@tests/e2e/features/mcp-registry/mcp-registry-tool-sync-interval.spec.ts`:
- Around line 60-102: The regression test in
mcp-registry-tool-sync-interval.spec.ts already checks the preserved outcome,
but it should also pin down the UI toggle payload itself. In the enabledSwitch
click flow, use the existing page.waitForResponse assertions around the PUT to
inspect the matching request body and verify it sends only the disabled field,
not the raw tool_sync_interval value. Keep the current round-trip assertions in
getClientConfig and the afterDisable/afterReEnable checks so the test covers
both the request shape and the preserved interval behavior.
In `@transports/bifrost-http/handlers/config.go`:
- Around line 345-369: The config update flow in configHandler duplicates the
MCPToolSyncInterval and MCPToolExecutionTimeout decision logic in two places,
which risks drift and inconsistent “winning” values. Keep the existing
prospective value validation in configHandler, then use
prospectiveToolSyncInterval and prospectiveToolExecutionTimeout as the single
source of truth when assigning updatedConfig, removing the later redundant
branches for those two fields so the mutation logic is centralized and
consistent.
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 677-695: The toolSyncInterval resolution logic is duplicated
across multiple handler blocks and should be consolidated to prevent divergence.
Extract the shared request-vs-config fallback flow into a helper such as
resolveToolSyncInterval on MCPHandler, using schemas.DurationFromUnits and the
store ConfigStore/GetClientConfig path, then have each MCP handler call it and
map its result to the existing HTTP error responses. Make sure the helper
centralizes the canonical behavior for request value, config fallback, and error
handling so all copies stay consistent.
In `@ui/lib/store/apis/mcpApi.ts`:
- Around line 176-180: The optimistic cache update in mcpApi’s client config is
using a raw unit-conversion constant, which is easy to misread and reintroduce
as a minutes-vs-nanoseconds bug. Replace the inline multiplier in the
tool_sync_interval assignment with a clearly named shared constant such as
NANOSECONDS_PER_MINUTE, and use that in the relevant update path inside the
mcpApi store logic so the conversion is obvious and consistent with the
documented contract.
🪄 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: 0646ac3c-c1d0-4b54-abe2-c5767c17c19c
📒 Files selected for processing (10)
core/bifrost.gocore/schemas/duration.gocore/schemas/duration_test.gocore/schemas/mcp.gotests/e2e/features/mcp-registry/mcp-registry-tool-sync-interval.spec.tstransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.goui/app/workspace/mcp-registry/views/mcpClientsTable.tsxui/lib/store/apis/mcpApi.ts
- e2e test: use the stable data-testid selector instead of a role-based
one, and assert the toggle's PUT payload is exactly {disabled: true}
to pin down the actual fix mechanism, not just the outcome.
- mcpApi.ts: name the minutes->nanoseconds multiplier instead of a bare
literal, since a misread multiplier here is exactly the bug class
this PR fixes.
|
|
|
Hey @Shaik-Sirajuddin this is already fixed in this pr - #4954 |
Missed to check that |
4 participants
Summary
The MCP client table's enable/disable Switch resent
tool_sync_interval(nanoseconds, fromGET) intoPUT, which parses that field as minutes — overflowingint64into a garbage negative duration for clients with a non-default sync interval.Changes
{ disabled }; the update handler already has PATCH semantics.DurationFromUnits, an overflow-guarded helper, and used it everywheretool_sync_interval/tool_execution_timeoutare derived from request or config data.tool_sync_intervalreload used seconds instead of minutes in two places (config.go,lib/config.go).config.go'supdateConfig(a rejected request could leave live state partially mutated) and a validate-after-DB-write ordering issue in the client update handler.How to test
Added
tests/e2e/features/mcp-registry/mcp-registry-tool-sync-interval.spec.ts(verified it reproduces the exact corrupted value from the bug report against pre-fix code, passes against the fix) andcore/schemas/duration_test.gofor the overflow-guard helper.Related issues
Closes #5026