Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 79 additions & 0 deletions core/schemas/mcp.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ package schemas

import (
"context"
"encoding/json"
"errors"
"fmt"
"strings"
"time"

Expand Down Expand Up @@ -58,6 +60,83 @@ const (
DefaultToolExecutionTimeout = 30 * time.Second
)

// 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
}
Comment on lines +63 to +75
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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 3

Repository: 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.go

Repository: 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.tsx

Repository: 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.tsx

Repository: 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.go

Repository: 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 -50

Repository: 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.go

Repository: 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.ts

Repository: 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 -20

Repository: 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 -30

Repository: 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 -5

Repository: 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.tsx

Repository: 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:

  1. API sends nanoseconds (from default time.Duration JSON serialization)
  2. UI converts to minutes for form
  3. Form submits minutes back
  4. 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.


// 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
}
Comment on lines +130 to +136
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

return nil
}
Comment on lines +77 to +138
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.


// CodeModeBindingLevel defines how tools are exposed in the VFS for code execution
type CodeModeBindingLevel string

Expand Down
13 changes: 7 additions & 6 deletions transports/config.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@
},
"enforce_governance_header": {
"type": "boolean",
"description": "Enforce governance header. This will require every incoming request to include x-bf-vk header."
"description": "Deprecated: use enforce_auth_on_inference instead. Enforce governance header. This will require every incoming request to include x-bf-vk header."
},
"enforce_auth_on_inference": {
"type": "boolean",
Expand Down Expand Up @@ -1551,7 +1551,7 @@
},
"virtual_key_mcp_config": {
"type": "object",
"description": "MCP configuration for a virtual key",
"description": "MCP configuration for a virtual key. Exactly one of mcp_client_id or mcp_client_name must be provided.",
"properties": {
"id": {
"type": "integer",
Expand All @@ -1563,7 +1563,11 @@
},
"mcp_client_id": {
"type": "integer",
"description": "Associated MCP client ID"
"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",
Expand All @@ -1573,9 +1577,6 @@
}
}
},
"required": [
"mcp_client_id"
],
"additionalProperties": false
},
"auth_config": {
Expand Down