feat: add custom JSON unmarshaling for MCP duration fields and update config schema#2181
feat: add custom JSON unmarshaling for MCP duration fields and update config schema#2181Pratham-Mishra04 wants to merge 1 commit intographite-base/2181from
Conversation
|
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdded custom JSON unmarshalling for MCP config types to accept durations as strings or integers; updated transport schema to deprecate Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🧹 Nitpick comments (4)
core/schemas/mcp.go (3)
119-138: Logic is correct; consider addingMarshalJSONfor consistency.The seconds-to-Duration conversion aligns with the schema definition. Same round-trip concern applies—without
MarshalJSON, re-serialization will output nanoseconds rather than seconds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mcp.go` around lines 119 - 138, The custom UnmarshalJSON on MCPToolManagerConfig parses tool_execution_timeout as integer seconds but there is no corresponding MarshalJSON, so re-serializing will emit time.Duration nanoseconds; add a MarshalJSON method on MCPToolManagerConfig that formats ToolExecutionTimeout as an integer number of seconds (int64) under the "tool_execution_timeout" JSON key, mirror the Alias pattern used in UnmarshalJSON, and ensure zero/omitted durations are handled consistently with the Unmarshal behavior.
77-96: Consider addingMarshalJSONfor round-trip consistency.Without a corresponding
MarshalJSON,ToolSyncIntervalwill serialize as nanoseconds (Go's default fortime.Duration). If the original JSON had"10m", re-marshaling produces600000000000. This asymmetry can break config round-tripping.♻️ Proposed MarshalJSON for MCPConfig
// MarshalJSON implements custom JSON encoding for MCPConfig, outputting // tool_sync_interval as a Go duration string. func (c MCPConfig) MarshalJSON() ([]byte, error) { type Alias MCPConfig aux := &struct { ToolSyncInterval string `json:"tool_sync_interval,omitempty"` *Alias }{Alias: (*Alias)(&c)} if c.ToolSyncInterval != 0 { aux.ToolSyncInterval = c.ToolSyncInterval.String() } return Marshal(aux) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mcp.go` around lines 77 - 96, Add a complementary MarshalJSON for MCPConfig so ToolSyncInterval round-trips as a human-readable duration string: implement func (c MCPConfig) MarshalJSON() ([]byte, error) using the same Alias struct pattern as UnmarshalJSON (referencing MCPConfig, UnmarshalJSON and ToolSyncInterval), expose ToolSyncInterval as a string (c.ToolSyncInterval.String()) when non-zero, and return the marshaled bytes via the same JSON marshal helper used elsewhere (e.g. sonic.Marshal or project Marshal). Ensure the auxiliary struct tags include `json:"tool_sync_interval,omitempty"` so zero durations are omitted.
98-117: Same round-trip concern applies here.Consider adding a corresponding
MarshalJSONtoMCPClientConfigas noted forMCPConfigabove.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mcp.go` around lines 98 - 117, Add a corresponding MarshalJSON for MCPClientConfig to mirror UnmarshalJSON: implement a MarshalJSON method on MCPClientConfig that creates an alias struct matching the aux used in UnmarshalJSON, converts c.ToolSyncInterval back into the same JSON form (a Go duration string) using the inverse of parseMCPDurationJSON, and then calls sonic.Marshal on that alias; this ensures ToolSyncInterval round-trips correctly with the existing UnmarshalJSON logic in MCPClientConfig and keeps encoding/decoding consistent.transports/config.schema.json (1)
1552-1581: Schema doesn't enforce the mutual exclusivity constraint.The description states "Exactly one of mcp_client_id or mcp_client_name must be provided," but the schema allows both, neither, or one. Consider adding a
oneOfconstraint to enforce this at validation time:♻️ Proposed schema constraint
"virtual_key_mcp_config": { "type": "object", "description": "MCP configuration for a virtual key. Exactly one of mcp_client_id or mcp_client_name must be provided.", "properties": { ... }, - "additionalProperties": false + "additionalProperties": false, + "oneOf": [ + { "required": ["mcp_client_id"], "not": { "required": ["mcp_client_name"] } }, + { "required": ["mcp_client_name"], "not": { "required": ["mcp_client_id"] } } + ] },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/config.schema.json` around lines 1552 - 1581, The schema for the virtual_key_mcp_config object does not enforce the declared "exactly one of mcp_client_id or mcp_client_name" constraint; update the virtual_key_mcp_config definition to add a oneOf clause that expresses two alternatives: one where "mcp_client_id" is required and "mcp_client_name" is prohibited, and the other where "mcp_client_name" is required and "mcp_client_id" is prohibited (use required and not/required semantics) so validation fails if both or neither are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/schemas/mcp.go`:
- Around line 63-75: The integer parsing currently treats raw integer JSON as
nanoseconds in parseMCPDurationJSON, causing tool_sync_interval submitted as
e.g. 10 (minutes) to be interpreted as 10ns; fix by changing
MCPClientConfig.UnmarshalJSON (the code path that reads tool_sync_interval) to
interpret integer values as minutes (i.e., convert int -> time.Duration(value) *
time.Minute) instead of using parseMCPDurationJSON's nanoseconds interpretation,
or add a dedicated parse helper (e.g., parseMCPDurationJSONMinutes) and call it
from MCPClientConfig.UnmarshalJSON for tool_sync_interval; mirror the
explicit-unit approach used in MCPToolManagerConfig.UnmarshalJSON for
tool_execution_timeout and consider adding corresponding MarshalJSON to keep API
round-trips consistent.
---
Nitpick comments:
In `@core/schemas/mcp.go`:
- Around line 119-138: The custom UnmarshalJSON on MCPToolManagerConfig parses
tool_execution_timeout as integer seconds but there is no corresponding
MarshalJSON, so re-serializing will emit time.Duration nanoseconds; add a
MarshalJSON method on MCPToolManagerConfig that formats ToolExecutionTimeout as
an integer number of seconds (int64) under the "tool_execution_timeout" JSON
key, mirror the Alias pattern used in UnmarshalJSON, and ensure zero/omitted
durations are handled consistently with the Unmarshal behavior.
- Around line 77-96: Add a complementary MarshalJSON for MCPConfig so
ToolSyncInterval round-trips as a human-readable duration string: implement func
(c MCPConfig) MarshalJSON() ([]byte, error) using the same Alias struct pattern
as UnmarshalJSON (referencing MCPConfig, UnmarshalJSON and ToolSyncInterval),
expose ToolSyncInterval as a string (c.ToolSyncInterval.String()) when non-zero,
and return the marshaled bytes via the same JSON marshal helper used elsewhere
(e.g. sonic.Marshal or project Marshal). Ensure the auxiliary struct tags
include `json:"tool_sync_interval,omitempty"` so zero durations are omitted.
- Around line 98-117: Add a corresponding MarshalJSON for MCPClientConfig to
mirror UnmarshalJSON: implement a MarshalJSON method on MCPClientConfig that
creates an alias struct matching the aux used in UnmarshalJSON, converts
c.ToolSyncInterval back into the same JSON form (a Go duration string) using the
inverse of parseMCPDurationJSON, and then calls sonic.Marshal on that alias;
this ensures ToolSyncInterval round-trips correctly with the existing
UnmarshalJSON logic in MCPClientConfig and keeps encoding/decoding consistent.
In `@transports/config.schema.json`:
- Around line 1552-1581: The schema for the virtual_key_mcp_config object does
not enforce the declared "exactly one of mcp_client_id or mcp_client_name"
constraint; update the virtual_key_mcp_config definition to add a oneOf clause
that expresses two alternatives: one where "mcp_client_id" is required and
"mcp_client_name" is prohibited, and the other where "mcp_client_name" is
required and "mcp_client_id" is prohibited (use required and not/required
semantics) so validation fails if both or neither are present.
🪄 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: CHILL
Plan: Pro
Run ID: d7faf93e-efa4-402c-bf04-abc651f55ece
📒 Files selected for processing (2)
core/schemas/mcp.gotransports/config.schema.json
| // parseMCPDurationJSON parses a JSON duration value that is either a Go duration | ||
| // string (e.g. "10m", "1h30m") or, as a fallback, an integer number of nanoseconds. | ||
| func parseMCPDurationJSON(raw json.RawMessage) (time.Duration, error) { | ||
| var s string | ||
| if sonic.Unmarshal(raw, &s) == nil { | ||
| return time.ParseDuration(s) | ||
| } | ||
| var n int64 | ||
| if err := sonic.Unmarshal(raw, &n); err != nil { | ||
| return 0, fmt.Errorf("expected Go duration string (e.g. \"10m\") or integer nanoseconds, got %s", string(raw)) | ||
| } | ||
| return time.Duration(n), nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how tool_sync_interval is used in UI/frontend to confirm expected units
rg -n "tool_sync_interval" --type=ts -C 3Repository: maximhq/bifrost
Length of output: 11212
🏁 Script executed:
cat -n core/schemas/mcp.go | sed -n '1,15p'Repository: maximhq/bifrost
Length of output: 433
🏁 Script executed:
cat -n core/schemas/mcp.go | sed -n '60,145p'Repository: maximhq/bifrost
Length of output: 3543
🏁 Script executed:
rg -n "MarshalJSON" core/schemas/mcp.goRepository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Also check what the struct fields are for MCPConfig and MCPClientConfig
ast-grep --pattern $'type MCPConfig struct {
$$$
}'Repository: maximhq/bifrost
Length of output: 1983
🏁 Script executed:
ast-grep --pattern $'type MCPClientConfig struct {
$$$
}'Repository: maximhq/bifrost
Length of output: 3779
🏁 Script executed:
# Check the toolSyncIntervalToMinutes function implementation in detail
sed -n '33,41p' ui/app/workspace/mcp-registry/views/mcpClientSheet.tsxRepository: maximhq/bifrost
Length of output: 409
🏁 Script executed:
# Check how the form data flows when submitting - what units does the UI send?
sed -n '100,110p' ui/app/workspace/mcp-registry/views/mcpClientSheet.tsxRepository: maximhq/bifrost
Length of output: 426
🏁 Script executed:
# Also check if there are any MarshalJSON methods for duration fields
rg -A 5 "func.*MarshalJSON" core/schemas/mcp.goRepository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Check what the default JSON serialization does for time.Duration
# Also verify if there are any custom marshaling methods for the config structs
rg -B 5 -A 10 "json.Marshal" core/schemas/mcp.go | head -50Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Let's understand the actual flow: when API sends tool_sync_interval back to UI,
# what format is it in? Check if there's any conversion in the API responses
rg -n "tool_sync_interval" core/schemas/mcp.goRepository: maximhq/bifrost
Length of output: 809
🏁 Script executed:
# Check the mcpApi.ts to see how data flows from API to UI
sed -n '60,75p' ui/lib/store/apis/mcpApi.tsRepository: maximhq/bifrost
Length of output: 1124
🏁 Script executed:
# Let's trace the complete flow - check what happens when API returns MCPClientConfig
# Look for where MCPClientConfig is serialized back to JSON
rg -n "MCPClientConfig" core/schemas/mcp.go | head -20Repository: maximhq/bifrost
Length of output: 928
🏁 Script executed:
# Check if there's any explicit conversion/transformation when returning API responses
rg -n "ToolSyncInterval\|tool_sync_interval" core/ -t go | grep -v "test" | head -30Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Verify the actual default JSON serialization behavior of time.Duration
# by checking if there are any tests or documentation
fd ".*test.*\.go" core/schemas/ | head -5Repository: maximhq/bifrost
Length of output: 200
🏁 Script executed:
# Let's check what actually gets sent from UI to the API when updating tool_sync_interval
# Look at the API request/response handling
rg -B 5 -A 5 "tool_sync_interval.*nanoseconds" ui/Repository: maximhq/bifrost
Length of output: 1078
🏁 Script executed:
# Check if there are any API endpoint handlers that show what format is expected
rg -n "tool_sync_interval" core/ -t go | grep -i "handler\|api\|endpoint\|request"Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Let's verify the actual round-trip - when the form sends data.tool_sync_interval
# is it sending minutes? Check the form validation or comments
sed -n '320,360p' ui/app/workspace/mcp-registry/views/mcpClientSheet.tsxRepository: maximhq/bifrost
Length of output: 1877
Tool sync interval unit mismatch causes data loss in API round-trips.
The parseMCPDurationJSON function interprets integer values as nanoseconds, but the UI sends tool_sync_interval as minutes (confirmed by schema at ui/lib/types/schemas.ts:854 and form labels). When the form submits a value like 10 (minutes), Go interprets it as time.Duration(10) nanoseconds—effectively zero.
The UI already has a workaround (toolSyncIntervalToMinutes) that converts API responses from nanoseconds to minutes, but this creates a broken round-trip:
- API sends nanoseconds (from default time.Duration JSON serialization)
- UI converts to minutes for form
- Form submits minutes back
- Go interprets minutes as nanoseconds (60 billion times smaller)
Fix: Update MCPClientConfig.UnmarshalJSON to handle integer values as minutes (not nanoseconds) when parsing tool_sync_interval. Alternatively, add corresponding MarshalJSON implementations to serialize durations consistently and document the expected unit (minutes vs. nanoseconds) across the API boundary.
Also note: MCPToolManagerConfig.UnmarshalJSON correctly treats integer values as seconds for tool_execution_timeout—apply similar clarity for tool_sync_interval.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/schemas/mcp.go` around lines 63 - 75, The integer parsing currently
treats raw integer JSON as nanoseconds in parseMCPDurationJSON, causing
tool_sync_interval submitted as e.g. 10 (minutes) to be interpreted as 10ns; fix
by changing MCPClientConfig.UnmarshalJSON (the code path that reads
tool_sync_interval) to interpret integer values as minutes (i.e., convert int ->
time.Duration(value) * time.Minute) instead of using parseMCPDurationJSON's
nanoseconds interpretation, or add a dedicated parse helper (e.g.,
parseMCPDurationJSONMinutes) and call it from MCPClientConfig.UnmarshalJSON for
tool_sync_interval; mirror the explicit-unit approach used in
MCPToolManagerConfig.UnmarshalJSON for tool_execution_timeout and consider
adding corresponding MarshalJSON to keep API round-trips consistent.
Pratham-Mishra04
force-pushed
the
03-20-fix_config_json_schema_fixes
branch
from
572c5f5 to
efc055d
Compare
Pratham-Mishra04
force-pushed
the
03-20-fix_adds_support_for_bedrock_stable-image_models
branch
from
3f5a8bc to
311efe5
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
core/schemas/mcp.go (1)
63-117:⚠️ Potential issue | 🟠 Major
tool_sync_intervalnumeric unit is still ambiguous and causes broken round-trips.Line 65-Line 75 parse integer JSON as nanoseconds, while
ui/lib/types/mcp.tsdocumentstool_sync_intervalas minutes. A payload like10is decoded as10nsin both Line 89 and Line 110 paths. Also, only custom unmarshalers were added; without matching marshalers, serialized output still uses rawtime.Durationencoding and keeps unit asymmetry.Suggested direction
-func parseMCPDurationJSON(raw json.RawMessage) (time.Duration, error) { +func parseMCPToolSyncIntervalJSON(raw json.RawMessage) (time.Duration, error) { var s string if sonic.Unmarshal(raw, &s) == nil { return time.ParseDuration(s) } - var n int64 - if err := sonic.Unmarshal(raw, &n); err != nil { - return 0, fmt.Errorf("expected Go duration string (e.g. \"10m\") or integer nanoseconds, got %s", string(raw)) + var minutes int64 + if err := sonic.Unmarshal(raw, &minutes); err != nil { + return 0, fmt.Errorf("expected Go duration string (e.g. \"10m\") or integer minutes, got %s", string(raw)) } - return time.Duration(n), nil + return time.Duration(minutes) * time.Minute, nil }Also add matching
MarshalJSONforMCPConfigandMCPClientConfigso the API emits a single documented unit/format consistently.As per coding guidelines, "Custom time.Duration fields in config structs must use custom MarshalJSON/UnmarshalJSON to convert between nanoseconds (Go) and milliseconds (JSON)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mcp.go` around lines 63 - 117, The integer parsing currently treats numeric values as nanoseconds causing unit mismatch and no MarshalJSON exists to emit a consistent JSON unit; update parseMCPDurationJSON so numeric raw JSON is interpreted as milliseconds (return time.Duration(n) * time.Millisecond) and accept both Go duration strings and integer milliseconds, and add MarshalJSON methods on MCPConfig and MCPClientConfig that serialize ToolSyncInterval as an integer number of milliseconds (convert c.ToolSyncInterval to milliseconds) so decoding and encoding use the same documented unit; reference parseMCPDurationJSON, MCPConfig.UnmarshalJSON, MCPClientConfig.UnmarshalJSON and implement MCPConfig.MarshalJSON and MCPClientConfig.MarshalJSON to enforce the millisecond convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@core/schemas/mcp.go`:
- Around line 63-117: The integer parsing currently treats numeric values as
nanoseconds causing unit mismatch and no MarshalJSON exists to emit a consistent
JSON unit; update parseMCPDurationJSON so numeric raw JSON is interpreted as
milliseconds (return time.Duration(n) * time.Millisecond) and accept both Go
duration strings and integer milliseconds, and add MarshalJSON methods on
MCPConfig and MCPClientConfig that serialize ToolSyncInterval as an integer
number of milliseconds (convert c.ToolSyncInterval to milliseconds) so decoding
and encoding use the same documented unit; reference parseMCPDurationJSON,
MCPConfig.UnmarshalJSON, MCPClientConfig.UnmarshalJSON and implement
MCPConfig.MarshalJSON and MCPClientConfig.MarshalJSON to enforce the millisecond
convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 921a0cc2-e0a7-4d0d-b112-e49bd8cefa21
📒 Files selected for processing (2)
core/schemas/mcp.gotransports/config.schema.json
🚧 Files skipped from review as they are similar to previous changes (1)
- transports/config.schema.json
Pratham-Mishra04
force-pushed
the
03-20-fix_config_json_schema_fixes
branch
from
efc055d to
6d5aff7
Compare
Pratham-Mishra04
force-pushed
the
03-20-fix_adds_support_for_bedrock_stable-image_models
branch
2 times, most recently
from
6554a67 to
c12b50f
Compare
Pratham-Mishra04
force-pushed
the
03-20-fix_config_json_schema_fixes
branch
from
6d5aff7 to
56cb80a
Compare
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)
transports/config.schema.json (1)
1554-1581:⚠️ Potential issue | 🟠 MajorAdd
oneOfconstraint to enforce the declared mcp_client_id / mcp_client_name exclusivity.The schema description at line 1554 states "Exactly one of mcp_client_id or mcp_client_name must be provided," but the schema currently allows both fields to be set or both to be absent. Add a structural constraint to match the documented contract.
Proposed fix
"virtual_key_mcp_config": { "type": "object", "description": "MCP configuration for a virtual key. Exactly one of mcp_client_id or mcp_client_name must be provided.", "properties": { "id": { "type": "integer", "description": "MCP config ID" }, "virtual_key_id": { "type": "string", "description": "Associated virtual key ID" }, "mcp_client_id": { "type": "integer", "description": "Associated MCP client ID (use this or mcp_client_name)" }, "mcp_client_name": { "type": "string", "description": "MCP client name (config file alternative to mcp_client_id; resolved to mcp_client_id at load time)" }, "tools_to_execute": { "type": "array", "description": "Tools to execute for this MCP config", "items": { "type": "string" } } }, + "oneOf": [ + { "required": ["mcp_client_id"] }, + { "required": ["mcp_client_name"] } + ], "additionalProperties": false },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/config.schema.json` around lines 1554 - 1581, The schema block describing the MCP configuration must enforce that exactly one of mcp_client_id or mcp_client_name is present; update the object schema (the JSON object that currently lists properties id, virtual_key_id, mcp_client_id, mcp_client_name, tools_to_execute and has additionalProperties:false) to include a oneOf constraint with two subschemas—one requiring ["mcp_client_id"] and the other requiring ["mcp_client_name"]—so the validator accepts only configs that provide exactly one of those fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@transports/config.schema.json`:
- Around line 1554-1581: The schema block describing the MCP configuration must
enforce that exactly one of mcp_client_id or mcp_client_name is present; update
the object schema (the JSON object that currently lists properties id,
virtual_key_id, mcp_client_id, mcp_client_name, tools_to_execute and has
additionalProperties:false) to include a oneOf constraint with two
subschemas—one requiring ["mcp_client_id"] and the other requiring
["mcp_client_name"]—so the validator accepts only configs that provide exactly
one of those fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 280930e2-933e-4c73-86bc-ff5ca1f2d812
📒 Files selected for processing (2)
core/schemas/mcp.gotransports/config.schema.json
🚧 Files skipped from review as they are similar to previous changes (1)
- core/schemas/mcp.go
Pratham-Mishra04
force-pushed
the
03-20-fix_config_json_schema_fixes
branch
from
56cb80a to
3c27241
Compare
Pratham-Mishra04
force-pushed
the
03-20-fix_adds_support_for_bedrock_stable-image_models
branch
from
c12b50f to
85ba95e
Compare
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)
transports/config.schema.json (1)
1554-1581:⚠️ Potential issue | 🟠 MajorEnforce the "exactly one" rule in schema (not only in description).
The description at line 1554 states an exclusive requirement, but the schema allows both or neither of
mcp_client_id/mcp_client_name. Add a structural constraint so invalid configs fail schema validation.Proposed fix
"virtual_key_mcp_config": { "type": "object", "description": "MCP configuration for a virtual key. Exactly one of mcp_client_id or mcp_client_name must be provided.", "properties": { "id": { "type": "integer", "description": "MCP config ID" }, "virtual_key_id": { "type": "string", "description": "Associated virtual key ID" }, "mcp_client_id": { "type": "integer", "description": "Associated MCP client ID (use this or mcp_client_name)" }, "mcp_client_name": { "type": "string", "description": "MCP client name (config file alternative to mcp_client_id; resolved to mcp_client_id at load time)" }, "tools_to_execute": { "type": "array", "description": "Tools to execute for this MCP config", "items": { "type": "string" } } }, + "oneOf": [ + { + "required": ["mcp_client_id"], + "not": { "required": ["mcp_client_name"] } + }, + { + "required": ["mcp_client_name"], + "not": { "required": ["mcp_client_id"] } + } + ], "additionalProperties": false },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/config.schema.json` around lines 1554 - 1581, The schema description says exactly one of mcp_client_id or mcp_client_name must be present but the schema doesn't enforce it; add a oneOf constraint to the MCP configuration object (the object with description "MCP configuration for a virtual key") that requires either ["mcp_client_id"] or ["mcp_client_name"] so configs with both or neither fail validation; reference the existing property names mcp_client_id and mcp_client_name when adding this oneOf.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@transports/config.schema.json`:
- Around line 1554-1581: The schema description says exactly one of
mcp_client_id or mcp_client_name must be present but the schema doesn't enforce
it; add a oneOf constraint to the MCP configuration object (the object with
description "MCP configuration for a virtual key") that requires either
["mcp_client_id"] or ["mcp_client_name"] so configs with both or neither fail
validation; reference the existing property names mcp_client_id and
mcp_client_name when adding this oneOf.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d383e34e-bcff-4e7e-88cd-fee447df0898
📒 Files selected for processing (2)
core/schemas/mcp.gotransports/config.schema.json
🚧 Files skipped from review as they are similar to previous changes (1)
- core/schemas/mcp.go
Pratham-Mishra04
force-pushed
the
03-20-fix_adds_support_for_bedrock_stable-image_models
branch
from
85ba95e to
32ed269
Compare
Pratham-Mishra04
force-pushed
the
03-20-fix_config_json_schema_fixes
branch
2 times, most recently
from
85d1302 to
f955b3d
Compare
Pratham-Mishra04
force-pushed
the
03-20-fix_adds_support_for_bedrock_stable-image_models
branch
2 times, most recently
from
458d8cf to
f76f7be
Compare
Pratham-Mishra04
force-pushed
the
03-20-fix_config_json_schema_fixes
branch
from
f955b3d to
f25ea39
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
core/schemas/mcp.go (1)
63-75:⚠️ Potential issue | 🟠 Major
tool_sync_intervalnumeric fallback is still unit-ambiguous.Line 64-75 still interprets raw integers as nanoseconds. Any client posting plain numeric “minutes” values will be misread as near-zero durations, causing broken round-trips.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mcp.go` around lines 63 - 75, parseMCPDurationJSON currently treats numeric JSON values as nanoseconds which makes plain numeric minute/second values wrong; change the numeric fallback to parse numbers as seconds (or a float seconds) instead of nanoseconds: in parseMCPDurationJSON, unmarshal into a float64 (or int64 if you only want integers) and return time.Duration(seconds * float64(time.Second)) (i.e. time.Duration(n)*time.Second for ints), and update the error message to reflect "Go duration string or number of seconds" and adjust any callers/tests that assumed nanoseconds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/schemas/mcp.go`:
- Around line 77-138: Add symmetric MarshalJSON methods to avoid asymmetric
encoding: implement MCPConfig.MarshalJSON and MCPClientConfig.MarshalJSON to
serialize c.ToolSyncInterval as an integer JSON value in milliseconds (convert
time.Duration to int64(milliseconds)), and implement
MCPToolManagerConfig.MarshalJSON to serialize ToolExecutionTimeout as an integer
JSON value in seconds (convert time.Duration to int64(seconds)); in each
MarshalJSON, build an alias struct matching the Unmarshal aux (including the raw
duration fields), set the converted numeric fields, and then use sonic.Marshal
to produce the output so encoding mirrors the existing Unmarshal logic.
- Around line 130-136: The current Unmarshal/parse block reads
aux.ToolExecutionTimeout into seconds and converts it to time.Duration allowing
negative values; add a validation after unmarshalling that returns an error if
seconds < 0 (e.g., fmt.Errorf("invalid tool_execution_timeout: must be
non-negative")) so that negative integers are rejected before assigning to
c.ToolExecutionTimeout; update the block that unmarshals
aux.ToolExecutionTimeout, checks the err, then checks seconds < 0, and only then
sets c.ToolExecutionTimeout = time.Duration(seconds) * time.Second.
---
Duplicate comments:
In `@core/schemas/mcp.go`:
- Around line 63-75: parseMCPDurationJSON currently treats numeric JSON values
as nanoseconds which makes plain numeric minute/second values wrong; change the
numeric fallback to parse numbers as seconds (or a float seconds) instead of
nanoseconds: in parseMCPDurationJSON, unmarshal into a float64 (or int64 if you
only want integers) and return time.Duration(seconds * float64(time.Second))
(i.e. time.Duration(n)*time.Second for ints), and update the error message to
reflect "Go duration string or number of seconds" and adjust any callers/tests
that assumed nanoseconds.
🪄 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: CHILL
Plan: Pro
Run ID: 316f77b7-75fe-4e87-8ab9-49f98034fcc0
📒 Files selected for processing (2)
core/schemas/mcp.gotransports/config.schema.json
🚧 Files skipped from review as they are similar to previous changes (1)
- transports/config.schema.json
| // UnmarshalJSON implements custom JSON decoding for MCPConfig, handling | ||
| // tool_sync_interval as a Go duration string (e.g. "10m", "1h"). | ||
| func (c *MCPConfig) UnmarshalJSON(data []byte) error { | ||
| type Alias MCPConfig | ||
| aux := &struct { | ||
| ToolSyncInterval json.RawMessage `json:"tool_sync_interval,omitempty"` | ||
| *Alias | ||
| }{Alias: (*Alias)(c)} | ||
| if err := sonic.Unmarshal(data, aux); err != nil { | ||
| return err | ||
| } | ||
| if len(aux.ToolSyncInterval) > 0 { | ||
| d, err := parseMCPDurationJSON(aux.ToolSyncInterval) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid tool_sync_interval: %w", err) | ||
| } | ||
| c.ToolSyncInterval = d | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // UnmarshalJSON implements custom JSON decoding for MCPClientConfig, handling | ||
| // tool_sync_interval as a Go duration string (e.g. "10m", "1h"). | ||
| func (c *MCPClientConfig) UnmarshalJSON(data []byte) error { | ||
| type Alias MCPClientConfig | ||
| aux := &struct { | ||
| ToolSyncInterval json.RawMessage `json:"tool_sync_interval,omitempty"` | ||
| *Alias | ||
| }{Alias: (*Alias)(c)} | ||
| if err := sonic.Unmarshal(data, aux); err != nil { | ||
| return err | ||
| } | ||
| if len(aux.ToolSyncInterval) > 0 { | ||
| d, err := parseMCPDurationJSON(aux.ToolSyncInterval) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid tool_sync_interval: %w", err) | ||
| } | ||
| c.ToolSyncInterval = d | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // UnmarshalJSON implements custom JSON decoding for MCPToolManagerConfig, handling | ||
| // tool_execution_timeout as an integer number of seconds (as documented in the schema). | ||
| func (c *MCPToolManagerConfig) UnmarshalJSON(data []byte) error { | ||
| type Alias MCPToolManagerConfig | ||
| aux := &struct { | ||
| ToolExecutionTimeout json.RawMessage `json:"tool_execution_timeout,omitempty"` | ||
| *Alias | ||
| }{Alias: (*Alias)(c)} | ||
| if err := sonic.Unmarshal(data, aux); err != nil { | ||
| return err | ||
| } | ||
| if len(aux.ToolExecutionTimeout) > 0 { | ||
| var seconds int64 | ||
| if err := sonic.Unmarshal(aux.ToolExecutionTimeout, &seconds); err != nil { | ||
| return fmt.Errorf("invalid tool_execution_timeout: expected integer (seconds): %w", err) | ||
| } | ||
| c.ToolExecutionTimeout = time.Duration(seconds) * time.Second | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Add matching MarshalJSON for these duration fields; decode-only support is asymmetric.
These structs now have custom decode paths, but default marshaling will still emit time.Duration as raw nanoseconds. That makes request/response units inconsistent and unstable across edit/save cycles.
As per coding guidelines "Custom time.Duration fields in config structs must use custom MarshalJSON/UnmarshalJSON to convert between nanoseconds (Go) and milliseconds (JSON)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/schemas/mcp.go` around lines 77 - 138, Add symmetric MarshalJSON methods
to avoid asymmetric encoding: implement MCPConfig.MarshalJSON and
MCPClientConfig.MarshalJSON to serialize c.ToolSyncInterval as an integer JSON
value in milliseconds (convert time.Duration to int64(milliseconds)), and
implement MCPToolManagerConfig.MarshalJSON to serialize ToolExecutionTimeout as
an integer JSON value in seconds (convert time.Duration to int64(seconds)); in
each MarshalJSON, build an alias struct matching the Unmarshal aux (including
the raw duration fields), set the converted numeric fields, and then use
sonic.Marshal to produce the output so encoding mirrors the existing Unmarshal
logic.
| if len(aux.ToolExecutionTimeout) > 0 { | ||
| var seconds int64 | ||
| if err := sonic.Unmarshal(aux.ToolExecutionTimeout, &seconds); err != nil { | ||
| return fmt.Errorf("invalid tool_execution_timeout: expected integer (seconds): %w", err) | ||
| } | ||
| c.ToolExecutionTimeout = time.Duration(seconds) * time.Second | ||
| } |
There was a problem hiding this comment.
Reject negative tool_execution_timeout values.
Line 131-136 accepts negative integers and converts them into negative durations. That can cause immediate timeout/cancellation behavior and hard-to-debug failures; add a seconds < 0 validation error.
Suggested guard
if len(aux.ToolExecutionTimeout) > 0 {
var seconds int64
if err := sonic.Unmarshal(aux.ToolExecutionTimeout, &seconds); err != nil {
return fmt.Errorf("invalid tool_execution_timeout: expected integer (seconds): %w", err)
}
+ if seconds < 0 {
+ return fmt.Errorf("invalid tool_execution_timeout: must be >= 0")
+ }
c.ToolExecutionTimeout = time.Duration(seconds) * time.Second
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/schemas/mcp.go` around lines 130 - 136, The current Unmarshal/parse
block reads aux.ToolExecutionTimeout into seconds and converts it to
time.Duration allowing negative values; add a validation after unmarshalling
that returns an error if seconds < 0 (e.g., fmt.Errorf("invalid
tool_execution_timeout: must be non-negative")) so that negative integers are
rejected before assigning to c.ToolExecutionTimeout; update the block that
unmarshals aux.ToolExecutionTimeout, checks the err, then checks seconds < 0,
and only then sets c.ToolExecutionTimeout = time.Duration(seconds) *
time.Second.
f25ea39 to
6081007
Compare
Pratham-Mishra04
force-pushed
the
03-20-fix_adds_support_for_bedrock_stable-image_models
branch
from
f76f7be to
af59c05
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (4)
core/schemas/mcp.go (4)
130-136:⚠️ Potential issue | 🟡 MinorReject negative
tool_execution_timeoutvalues to prevent undefined behavior.Negative durations can cause immediate context cancellation or unpredictable timeout behavior. Add validation before the conversion.
Suggested fix
if len(aux.ToolExecutionTimeout) > 0 { var seconds int64 if err := sonic.Unmarshal(aux.ToolExecutionTimeout, &seconds); err != nil { return fmt.Errorf("invalid tool_execution_timeout: expected integer (seconds): %w", err) } + if seconds < 0 { + return fmt.Errorf("invalid tool_execution_timeout: must be non-negative") + } c.ToolExecutionTimeout = time.Duration(seconds) * time.Second }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mcp.go` around lines 130 - 136, When unmarshalling aux.ToolExecutionTimeout into seconds in the current custom Unmarshal logic, validate that the parsed int64 is non-negative before converting to time.Duration to prevent negative durations; after sonic.Unmarshal into the local variable seconds, check if seconds < 0 and return a descriptive error (e.g., "tool_execution_timeout must be non-negative") instead of assigning to c.ToolExecutionTimeout, then only set c.ToolExecutionTimeout = time.Duration(seconds) * time.Second when the value is valid.
98-117:⚠️ Potential issue | 🟠 MajorImplementation pattern is correct; same
MarshalJSONgap applies.The Alias pattern and sonic usage are appropriate. However, this struct also lacks symmetric
MarshalJSON, causing the same round-trip issues noted above.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mcp.go` around lines 98 - 117, Add a symmetric MarshalJSON implementation for MCPClientConfig to mirror the custom UnmarshalJSON behavior: when serializing MCPClientConfig, encode ToolSyncInterval as a string duration (matching parseMCPDurationJSON's expected format) instead of raw duration bytes so round-trips succeed; implement func (c *MCPClientConfig) MarshalJSON() ([]byte, error) that uses an Alias struct similar to UnmarshalJSON, converts c.ToolSyncInterval to the appropriate JSON string (or omits if zero) and then calls sonic.Marshal on the alias, ensuring names match the existing ToolSyncInterval and MCPClientConfig symbols.
77-96:⚠️ Potential issue | 🟠 MajorMissing symmetric
MarshalJSONcauses asymmetric encode/decode.Without a corresponding
MarshalJSON,time.Durationfields will serialize as raw nanoseconds. This breaks round-trips: API returns nanoseconds → UI converts to minutes → UI submits minutes → Go interprets as nanoseconds (60 billion times smaller).As per coding guidelines: "Custom time.Duration fields in config structs must use custom MarshalJSON/UnmarshalJSON to convert between nanoseconds (Go) and milliseconds (JSON)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mcp.go` around lines 77 - 96, Add a symmetric MarshalJSON method for MCPConfig to mirror UnmarshalJSON: implement func (c *MCPConfig) MarshalJSON() ([]byte, error) that uses an Alias struct (matching the aux in UnmarshalJSON) and sets the tool_sync_interval JSON field to the duration in milliseconds (int64) instead of raw nanoseconds, preserving the "tool_sync_interval" key and omitempty behavior; return sonic.Marshal on the alias to avoid recursion. This ensures round-trip encoding/decoding between MCPConfig.MarshalJSON and MCPConfig.UnmarshalJSON.
63-75:⚠️ Potential issue | 🟠 MajorInteger fallback interprets values as nanoseconds, but UI sends minutes.
The PR summary states integer fallback is nanoseconds, but the UI TypeScript interface documents
tool_sync_intervalas minutes:// Per-client override in minutes (0 = use global, -1 = disabled). When the UI sends10(meaning 10 minutes), this code interprets it as 10 nanoseconds—effectively zero.Consider treating integer fallback as minutes for
tool_sync_intervalto match UI expectations, or update the UI to send duration strings exclusively.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mcp.go` around lines 63 - 75, The integer fallback currently treats numeric values as nanoseconds, causing UI-sent minute values (e.g. tool_sync_interval = 10) to be misinterpreted; update parseMCPDurationJSON so that when it unmarshals an integer it returns time.Duration(n) * time.Minute (and adjust the error text to reflect "integer minutes" instead of "integer nanoseconds") so the server interprets UI minute values correctly (or alternatively add a separate helper for tool_sync_interval that multiplies by time.Minute and keep the generic behavior unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@core/schemas/mcp.go`:
- Around line 130-136: When unmarshalling aux.ToolExecutionTimeout into seconds
in the current custom Unmarshal logic, validate that the parsed int64 is
non-negative before converting to time.Duration to prevent negative durations;
after sonic.Unmarshal into the local variable seconds, check if seconds < 0 and
return a descriptive error (e.g., "tool_execution_timeout must be non-negative")
instead of assigning to c.ToolExecutionTimeout, then only set
c.ToolExecutionTimeout = time.Duration(seconds) * time.Second when the value is
valid.
- Around line 98-117: Add a symmetric MarshalJSON implementation for
MCPClientConfig to mirror the custom UnmarshalJSON behavior: when serializing
MCPClientConfig, encode ToolSyncInterval as a string duration (matching
parseMCPDurationJSON's expected format) instead of raw duration bytes so
round-trips succeed; implement func (c *MCPClientConfig) MarshalJSON() ([]byte,
error) that uses an Alias struct similar to UnmarshalJSON, converts
c.ToolSyncInterval to the appropriate JSON string (or omits if zero) and then
calls sonic.Marshal on the alias, ensuring names match the existing
ToolSyncInterval and MCPClientConfig symbols.
- Around line 77-96: Add a symmetric MarshalJSON method for MCPConfig to mirror
UnmarshalJSON: implement func (c *MCPConfig) MarshalJSON() ([]byte, error) that
uses an Alias struct (matching the aux in UnmarshalJSON) and sets the
tool_sync_interval JSON field to the duration in milliseconds (int64) instead of
raw nanoseconds, preserving the "tool_sync_interval" key and omitempty behavior;
return sonic.Marshal on the alias to avoid recursion. This ensures round-trip
encoding/decoding between MCPConfig.MarshalJSON and MCPConfig.UnmarshalJSON.
- Around line 63-75: The integer fallback currently treats numeric values as
nanoseconds, causing UI-sent minute values (e.g. tool_sync_interval = 10) to be
misinterpreted; update parseMCPDurationJSON so that when it unmarshals an
integer it returns time.Duration(n) * time.Minute (and adjust the error text to
reflect "integer minutes" instead of "integer nanoseconds") so the server
interprets UI minute values correctly (or alternatively add a separate helper
for tool_sync_interval that multiplies by time.Minute and keep the generic
behavior unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 28ba5d32-be16-446f-b2c7-f6889550d976
📒 Files selected for processing (2)
core/schemas/mcp.gotransports/config.schema.json
🚧 Files skipped from review as they are similar to previous changes (1)
- transports/config.schema.json
Pratham-Mishra04
force-pushed
the
03-20-fix_adds_support_for_bedrock_stable-image_models
branch
from
af59c05 to
0d15eae
Compare
Pratham-Mishra04
force-pushed
the
03-20-fix_config_json_schema_fixes
branch
from
6081007 to
47b1c60
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
core/schemas/mcp.go (3)
130-136:⚠️ Potential issue | 🟡 MinorReject negative
tool_execution_timeoutvalues.Negative integers are currently accepted and converted to negative durations. Please validate
seconds >= 0before assignment.Proposed guard
if len(aux.ToolExecutionTimeout) > 0 { var seconds int64 if err := sonic.Unmarshal(aux.ToolExecutionTimeout, &seconds); err != nil { return fmt.Errorf("invalid tool_execution_timeout: expected integer (seconds): %w", err) } + if seconds < 0 { + return fmt.Errorf("invalid tool_execution_timeout: must be non-negative") + } c.ToolExecutionTimeout = time.Duration(seconds) * time.Second }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mcp.go` around lines 130 - 136, The JSON unmarshal block for aux.ToolExecutionTimeout accepts negative integers and assigns them to c.ToolExecutionTimeout; after decoding into the local seconds variable (via sonic.Unmarshal on aux.ToolExecutionTimeout) add a guard that returns an error when seconds < 0 (e.g., fmt.Errorf("invalid tool_execution_timeout: must be non-negative")) instead of assigning a negative time.Duration to c.ToolExecutionTimeout, ensuring only seconds >= 0 are converted to time.Duration.
63-75:⚠️ Potential issue | 🟠 MajorInteger fallback unit is incompatible with MCP client
tool_sync_intervalcontract.Numeric
tool_sync_intervalvalues are parsed as nanoseconds here, but the client contract uses minutes (ui/lib/types/mcp.ts). A value like10is currently interpreted as10ns, causing broken edit/save round-trips.Suggested direction
+func parseMCPDurationJSONMinutes(raw json.RawMessage) (time.Duration, error) { + var s string + if sonic.Unmarshal(raw, &s) == nil { + return time.ParseDuration(s) + } + var n int64 + if err := sonic.Unmarshal(raw, &n); err != nil { + return 0, fmt.Errorf("expected Go duration string (e.g. \"10m\") or integer minutes, got %s", string(raw)) + } + return time.Duration(n) * time.Minute, nil +}Then use this parser for
MCPClientConfig.tool_sync_interval(andMCPConfigtoo if its numeric API contract is also minutes).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mcp.go` around lines 63 - 75, parseMCPDurationJSON currently treats numeric JSON values as nanoseconds which conflicts with the MCP client contract that uses minutes; update parseMCPDurationJSON so that when the numeric fallback path (variable n) is used it interprets the value as minutes (i.e., return time.Duration(n) * time.Minute) instead of time.Duration(n), and ensure this parser is used for MCPClientConfig.tool_sync_interval (and MCPConfig if applicable) so numeric values like 10 are parsed as 10 minutes.
77-138:⚠️ Potential issue | 🟠 MajorAdd matching
MarshalJSONfor these duration fields.These types now customize decoding only; encoding still uses default
time.DurationJSON (nanoseconds), which makes units asymmetric and unstable across API round-trips.As per coding guidelines "core/schemas/*.go: Custom duration fields in config structs must implement
MarshalJSON/UnmarshalJSONto convert between Go nanoseconds and JSON milliseconds." Based on learnings "In the core/schemas package, any custom MarshalJSON methods must invoke the package-level Marshal() wrapper rather than encoding/json.Marshal directly."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/mcp.go` around lines 77 - 138, Add matching MarshalJSON implementations for MCPConfig, MCPClientConfig and MCPToolManagerConfig so encoding matches the custom decoding: for MCPConfig and MCPClientConfig serialize ToolSyncInterval as an integer number of milliseconds (convert time.Duration to int64 milliseconds) and for MCPToolManagerConfig serialize ToolExecutionTimeout as an integer number of seconds (convert time.Duration to int64 seconds) to mirror the existing UnmarshalJSON behavior in MCPConfig.UnmarshalJSON, MCPClientConfig.UnmarshalJSON and MCPToolManagerConfig.UnmarshalJSON; each MarshalJSON must build an aux struct mirroring the Unmarshal aux (including ToolSyncInterval or ToolExecutionTimeout), call the package-level Marshal() wrapper (not encoding/json.Marshal) to encode the aux, and return its bytes/errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@core/schemas/mcp.go`:
- Around line 130-136: The JSON unmarshal block for aux.ToolExecutionTimeout
accepts negative integers and assigns them to c.ToolExecutionTimeout; after
decoding into the local seconds variable (via sonic.Unmarshal on
aux.ToolExecutionTimeout) add a guard that returns an error when seconds < 0
(e.g., fmt.Errorf("invalid tool_execution_timeout: must be non-negative"))
instead of assigning a negative time.Duration to c.ToolExecutionTimeout,
ensuring only seconds >= 0 are converted to time.Duration.
- Around line 63-75: parseMCPDurationJSON currently treats numeric JSON values
as nanoseconds which conflicts with the MCP client contract that uses minutes;
update parseMCPDurationJSON so that when the numeric fallback path (variable n)
is used it interprets the value as minutes (i.e., return time.Duration(n) *
time.Minute) instead of time.Duration(n), and ensure this parser is used for
MCPClientConfig.tool_sync_interval (and MCPConfig if applicable) so numeric
values like 10 are parsed as 10 minutes.
- Around line 77-138: Add matching MarshalJSON implementations for MCPConfig,
MCPClientConfig and MCPToolManagerConfig so encoding matches the custom
decoding: for MCPConfig and MCPClientConfig serialize ToolSyncInterval as an
integer number of milliseconds (convert time.Duration to int64 milliseconds) and
for MCPToolManagerConfig serialize ToolExecutionTimeout as an integer number of
seconds (convert time.Duration to int64 seconds) to mirror the existing
UnmarshalJSON behavior in MCPConfig.UnmarshalJSON, MCPClientConfig.UnmarshalJSON
and MCPToolManagerConfig.UnmarshalJSON; each MarshalJSON must build an aux
struct mirroring the Unmarshal aux (including ToolSyncInterval or
ToolExecutionTimeout), call the package-level Marshal() wrapper (not
encoding/json.Marshal) to encode the aux, and return its bytes/errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f50f36af-3434-4b9d-ac58-30a3144621c2
📒 Files selected for processing (2)
core/schemas/mcp.gotransports/config.schema.json
🚧 Files skipped from review as they are similar to previous changes (1)
- transports/config.schema.json
Radheshg04
changed the base branch from
03-20-fix_adds_support_for_bedrock_stable-image_models
to
graphite-base/2181
2 participants
Summary
Enhances MCP (Model Context Protocol) configuration handling by adding custom JSON unmarshaling for duration fields and improves the configuration schema with better validation and documentation.
Changes
MCPConfig,MCPClientConfig, andMCPToolManagerConfigto handle duration fields properlytool_sync_intervalnow accepts Go duration strings (e.g. "10m", "1h30m") with fallback to nanosecond integerstool_execution_timeoutaccepts integer seconds as documented in the schemamcp_client_idandmcp_client_nameoptionsenforce_governance_headerin favor ofenforce_auth_on_inferenceType of change
Affected areas
How to test
Validate MCP configuration parsing with duration strings:
Test configuration files with the new duration format:
{ "mcp": { "tool_sync_interval": "10m", "tool_execution_timeout": 30 } }Screenshots/Recordings
N/A
Breaking changes
The
virtual_key_mcp_configno longer requiresmcp_client_idas a mandatory field sincemcp_client_namecan be used as an alternative. Provider-level pricing overrides have been removed from provider configurations.Related issues
N/A
Security considerations
No security implications - this change only affects configuration parsing and validation.
Checklist
docs/contributing/README.mdand followed the guidelines