perf: optimize client repository and transports - #152
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
9 issues found across 23 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/transports/sse/sse_client_transport_additional2_test.go">
<violation number="1" location="src/transports/sse/sse_client_transport_additional2_test.go:45">
P2: The `time.Sleep(20 * time.Millisecond)` makes this test timing-dependent and doesn't actually exercise the scenario described by the test name. Since the goroutine reads from an in-memory `strings.Reader`, all data is consumed almost instantly — the sleep provides no guarantee that the goroutine is mid-read when `Close()` is called. A sleep at this granularity can also cause flaky CI failures on slow runners. Suggestion: consume a known event from the stream (which proves the goroutine is running) before calling `Close()`, or replace the sleep with a deterministic synchronization point.</violation>
</file>
<file name="src/repository/repo.go">
<violation number="1" location="src/repository/repo.go:160">
P2: Tools added through the still-exported `Tools` map after the first repository operation become invisible to name lookup/removal because the index is never rebuilt. Either make map mutation impossible through an encapsulated API, or invalidate/rebuild the index for legacy map updates.</violation>
</file>
<file name="src/tag/tag_search.go">
<violation number="1" location="src/tag/tag_search.go:42">
P2: A cancelled search against an empty repository returns `(nil, nil)` because the early return precedes the cancellation check. Check `ctx.Err()` immediately after retrieval so cancellation behavior does not depend on repository size.</violation>
</file>
<file name="src/transports/http/http_transport.go">
<violation number="1" location="src/transports/http/http_transport.go:221">
P1: Registering an OpenAPI 2 spec with ordinary query/path parameters now panics instead of returning discovered tools. `openapi.NewConverter(...).Convert()` reaches an unchecked `param["schema"]` assertion; handle OpenAPI 2 parameter types or validate before asserting.</violation>
</file>
<file name="utcp_client.go">
<violation number="1" location="utcp_client.go:297">
P2: `SearchTools` changed from prefix-based filtering (`providerPrefix string`) to a mixed lookup strategy (`query string`). Previously, `SearchTools("http", 10)` returned tools whose name started with `"http."`. Now it first tries an exact provider-name lookup.
This breaks callers that passed partial prefixes. If the provider-lookup fails and no `searchStrategy` is configured, the method now returns `(nil, nil)` instead of a filtered subset. The limit is also newly applied to the empty-query case, which the old behavior did not cap.
If this breaking change is intentional, callers and the `UtcpClientInterface` contract should be updated. If the old prefix-filter behavior needs to be preserved, consider keeping the direct `strings.HasPrefix` iteration as a fallback after the search strategy.</violation>
<violation number="2" location="utcp_client.go:679">
P3: Loading a large providers file now repeatedly copies every previously cached tool and provider, making registration scale quadratically. Keep per-provider updates incremental or use a separately published index so adding one provider does not rebuild the entire cache.</violation>
</file>
<file name="src/providers/helpers/unmarshal.go">
<violation number="1" location="src/providers/helpers/unmarshal.go:82">
P2: The three new functions `ProviderName`, `SetProviderName`, and `NewProvider` each contain a nearly identical 13-case switch statement listing every provider type. Adding a new provider currently requires editing all three functions — an easy source of drift. Since every provider already implements `Type()` and has a `Name` field (either directly or via `BaseProvider` embedding), consider a unified approach: adding `GetName()/SetName()` to the `Provider` interface or using a registry map from `ProviderType` to constructor/name accessors. This would eliminate the duplication and make adding new providers single-touch.</violation>
<violation number="2" location="src/providers/helpers/unmarshal.go:170">
P2: In `NewProvider`, the `ProviderMCP` case returns `&MCPProvider{}` without a `BaseProvider`, unlike every other provider type which initializes with `BaseProvider: base`. This works because `MCPProvider` has its own `Type()` method, but the inconsistency means MCPProvider won't pick up future `BaseProvider` fields, and any code accessing `ProviderType` directly instead of through `Type()` would break for MCP. Consider aligning MCPProvider with the other providers by embedding `BaseProvider` and removing the custom `Type()` override.</violation>
</file>
<file name="src/transports/mcp/mcp_transport.go">
<violation number="1" location="src/transports/mcp/mcp_transport.go:924">
P2: contentAsMap's fast path for TextContent silently drops the embedded `Annotated` fields (Annotations with Audience and Priority). The previous code path marshaled the full TextContent to JSON and back, which preserved any `annotations` data. The new fast path constructs a map with only `type` and `text`, discarding annotation metadata that callers previously received.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if resp.Request != nil && resp.Request.URL != nil { | ||
| specURL = resp.Request.URL.String() | ||
| } | ||
| return openapi.NewConverter(raw, specURL, hp.Name).Convert().Tools, nil |
There was a problem hiding this comment.
P1: Registering an OpenAPI 2 spec with ordinary query/path parameters now panics instead of returning discovered tools. openapi.NewConverter(...).Convert() reaches an unchecked param["schema"] assertion; handle OpenAPI 2 parameter types or validate before asserting.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/transports/http/http_transport.go, line 221:
<comment>Registering an OpenAPI 2 spec with ordinary query/path parameters now panics instead of returning discovered tools. `openapi.NewConverter(...).Convert()` reaches an unchecked `param["schema"]` assertion; handle OpenAPI 2 parameter types or validate before asserting.</comment>
<file context>
@@ -166,36 +189,36 @@ func (t *HttpClientTransport) RegisterToolProvider(ctx context.Context, p Provid
+ if resp.Request != nil && resp.Request.URL != nil {
+ specURL = resp.Request.URL.String()
+ }
+ return openapi.NewConverter(raw, specURL, hp.Name).Convert().Tools, nil
}
</file context>
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| time.Sleep(20 * time.Millisecond) |
There was a problem hiding this comment.
P2: The time.Sleep(20 * time.Millisecond) makes this test timing-dependent and doesn't actually exercise the scenario described by the test name. Since the goroutine reads from an in-memory strings.Reader, all data is consumed almost instantly — the sleep provides no guarantee that the goroutine is mid-read when Close() is called. A sleep at this granularity can also cause flaky CI failures on slow runners. Suggestion: consume a known event from the stream (which proves the goroutine is running) before calling Close(), or replace the sleep with a deterministic synchronization point.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/transports/sse/sse_client_transport_additional2_test.go, line 45:
<comment>The `time.Sleep(20 * time.Millisecond)` makes this test timing-dependent and doesn't actually exercise the scenario described by the test name. Since the goroutine reads from an in-memory `strings.Reader`, all data is consumed almost instantly — the sleep provides no guarantee that the goroutine is mid-read when `Close()` is called. A sleep at this granularity can also cause flaky CI failures on slow runners. Suggestion: consume a known event from the stream (which proves the goroutine is running) before calling `Close()`, or replace the sleep with a deterministic synchronization point.</comment>
<file context>
@@ -19,3 +21,36 @@ func TestHandleSSE(t *testing.T) {
+ if err != nil {
+ t.Fatal(err)
+ }
+ time.Sleep(20 * time.Millisecond)
+ if err := stream.Close(); err != nil {
+ t.Fatal(err)
</file context>
| r.mu.RLock() | ||
| ready := r.toolIndex != nil && r.toolProviders != nil | ||
| r.mu.RUnlock() | ||
| if ready { |
There was a problem hiding this comment.
P2: Tools added through the still-exported Tools map after the first repository operation become invisible to name lookup/removal because the index is never rebuilt. Either make map mutation impossible through an encapsulated API, or invalidate/rebuild the index for legacy map updates.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/repository/repo.go, line 160:
<comment>Tools added through the still-exported `Tools` map after the first repository operation become invisible to name lookup/removal because the index is never rebuilt. Either make map mutation impossible through an encapsulated API, or invalidate/rebuild the index for legacy map updates.</comment>
<file context>
@@ -115,45 +123,67 @@ func (r *InMemoryToolRepository) SaveProviderWithTools(ctx context.Context, prov
+ r.mu.RLock()
+ ready := r.toolIndex != nil && r.toolProviders != nil
+ r.mu.RUnlock()
+ if ready {
+ return
+ }
</file context>
| type scoredTool struct { | ||
| tool Tool | ||
| score float64 | ||
| if len(tools) == 0 { |
There was a problem hiding this comment.
P2: A cancelled search against an empty repository returns (nil, nil) because the early return precedes the cancellation check. Check ctx.Err() immediately after retrieval so cancellation behavior does not depend on repository size.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/tag/tag_search.go, line 42:
<comment>A cancelled search against an empty repository returns `(nil, nil)` because the early return precedes the cancellation check. Check `ctx.Err()` immediately after retrieval so cancellation behavior does not depend on repository size.</comment>
<file context>
@@ -15,102 +15,152 @@ import (
- type scoredTool struct {
- tool Tool
- score float64
+ if len(tools) == 0 {
+ return nil, nil
}
</file context>
| if len(tools) == 0 { | |
| if err := ctx.Err(); err != nil { | |
| return nil, err | |
| } | |
| if len(tools) == 0 { |
| @@ -77,3 +77,101 @@ func UnmarshalProvider(data []byte) (Provider, error) { | |||
| return nil, fmt.Errorf("unsupported provider_type %q", base.ProviderType) | |||
There was a problem hiding this comment.
P2: The three new functions ProviderName, SetProviderName, and NewProvider each contain a nearly identical 13-case switch statement listing every provider type. Adding a new provider currently requires editing all three functions — an easy source of drift. Since every provider already implements Type() and has a Name field (either directly or via BaseProvider embedding), consider a unified approach: adding GetName()/SetName() to the Provider interface or using a registry map from ProviderType to constructor/name accessors. This would eliminate the duplication and make adding new providers single-touch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/providers/helpers/unmarshal.go, line 82:
<comment>The three new functions `ProviderName`, `SetProviderName`, and `NewProvider` each contain a nearly identical 13-case switch statement listing every provider type. Adding a new provider currently requires editing all three functions — an easy source of drift. Since every provider already implements `Type()` and has a `Name` field (either directly or via `BaseProvider` embedding), consider a unified approach: adding `GetName()/SetName()` to the `Provider` interface or using a registry map from `ProviderType` to constructor/name accessors. This would eliminate the duplication and make adding new providers single-touch.</comment>
<file context>
@@ -77,3 +77,101 @@ func UnmarshalProvider(data []byte) (Provider, error) {
}
+
+// ProviderName returns the configured name for a built-in provider.
+func ProviderName(provider Provider) (string, bool) {
+ switch p := provider.(type) {
+ case *CliProvider:
</file context>
| return &UDPProvider{BaseProvider: base}, nil | ||
| case ProviderWebRTC: | ||
| return &WebRTCProvider{BaseProvider: base}, nil | ||
| case ProviderMCP: |
There was a problem hiding this comment.
P2: In NewProvider, the ProviderMCP case returns &MCPProvider{} without a BaseProvider, unlike every other provider type which initializes with BaseProvider: base. This works because MCPProvider has its own Type() method, but the inconsistency means MCPProvider won't pick up future BaseProvider fields, and any code accessing ProviderType directly instead of through Type() would break for MCP. Consider aligning MCPProvider with the other providers by embedding BaseProvider and removing the custom Type() override.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/providers/helpers/unmarshal.go, line 170:
<comment>In `NewProvider`, the `ProviderMCP` case returns `&MCPProvider{}` without a `BaseProvider`, unlike every other provider type which initializes with `BaseProvider: base`. This works because `MCPProvider` has its own `Type()` method, but the inconsistency means MCPProvider won't pick up future `BaseProvider` fields, and any code accessing `ProviderType` directly instead of through `Type()` would break for MCP. Consider aligning MCPProvider with the other providers by embedding `BaseProvider` and removing the custom `Type()` override.</comment>
<file context>
@@ -77,3 +77,101 @@ func UnmarshalProvider(data []byte) (Provider, error) {
+ return &UDPProvider{BaseProvider: base}, nil
+ case ProviderWebRTC:
+ return &WebRTCProvider{BaseProvider: base}, nil
+ case ProviderMCP:
+ return &MCPProvider{}, nil
+ case ProviderText:
</file context>
| return respMap, nil | ||
| } | ||
|
|
||
| func contentAsMap(content mcpapi.Content) (map[string]any, error) { |
There was a problem hiding this comment.
P2: contentAsMap's fast path for TextContent silently drops the embedded Annotated fields (Annotations with Audience and Priority). The previous code path marshaled the full TextContent to JSON and back, which preserved any annotations data. The new fast path constructs a map with only type and text, discarding annotation metadata that callers previously received.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/transports/mcp/mcp_transport.go, line 924:
<comment>contentAsMap's fast path for TextContent silently drops the embedded `Annotated` fields (Annotations with Audience and Priority). The previous code path marshaled the full TextContent to JSON and back, which preserved any `annotations` data. The new fast path constructs a map with only `type` and `text`, discarding annotation metadata that callers previously received.</comment>
<file context>
@@ -917,6 +921,25 @@ func (t *MCPTransport) callHTTPTool(
return respMap, nil
}
+func contentAsMap(content mcpapi.Content) (map[string]any, error) {
+ switch value := content.(type) {
+ case mcpapi.TextContent:
</file context>
| // If providerPrefix is empty, return all tools. | ||
| if providerPrefix == "" { | ||
| return c.toolRepository.GetTools(context.Background()) | ||
| func (c *UtcpClient) SearchTools(query string, limit int) ([]Tool, error) { |
There was a problem hiding this comment.
P2: SearchTools changed from prefix-based filtering (providerPrefix string) to a mixed lookup strategy (query string). Previously, SearchTools("http", 10) returned tools whose name started with "http.". Now it first tries an exact provider-name lookup.
This breaks callers that passed partial prefixes. If the provider-lookup fails and no searchStrategy is configured, the method now returns (nil, nil) instead of a filtered subset. The limit is also newly applied to the empty-query case, which the old behavior did not cap.
If this breaking change is intentional, callers and the UtcpClientInterface contract should be updated. If the old prefix-filter behavior needs to be preserved, consider keeping the direct strings.HasPrefix iteration as a fallback after the search strategy.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At utcp_client.go, line 297:
<comment>`SearchTools` changed from prefix-based filtering (`providerPrefix string`) to a mixed lookup strategy (`query string`). Previously, `SearchTools("http", 10)` returned tools whose name started with `"http."`. Now it first tries an exact provider-name lookup.
This breaks callers that passed partial prefixes. If the provider-lookup fails and no `searchStrategy` is configured, the method now returns `(nil, nil)` instead of a filtered subset. The limit is also newly applied to the empty-query case, which the old behavior did not cap.
If this breaking change is intentional, callers and the `UtcpClientInterface` contract should be updated. If the old prefix-filter behavior needs to be preserved, consider keeping the direct `strings.HasPrefix` iteration as a fallback after the search strategy.</comment>
<file context>
@@ -466,75 +277,48 @@ func (c *UtcpClient) DeregisterToolProvider(ctx context.Context, providerName st
- // If providerPrefix is empty, return all tools.
- if providerPrefix == "" {
- return c.toolRepository.GetTools(context.Background())
+func (c *UtcpClient) SearchTools(query string, limit int) ([]Tool, error) {
+ ctx := context.Background()
+ if query == "" {
</file context>
| return func(ctx context.Context, args map[string]any) (transports.StreamResult, error) { | ||
| return t.CallToolStream(ctx, cn, args, p) | ||
|
|
||
| resolved := make(map[string]*resolvedTool, len(current.tools)+len(tools)) |
There was a problem hiding this comment.
P3: Loading a large providers file now repeatedly copies every previously cached tool and provider, making registration scale quadratically. Keep per-provider updates incremental or use a separately published index so adding one provider does not rebuild the entire cache.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At utcp_client.go, line 679:
<comment>Loading a large providers file now repeatedly copies every previously cached tool and provider, making registration scale quadratically. Keep per-provider updates incremental or use a separately published index so adding one provider does not rebuild the entire cache.</comment>
<file context>
@@ -831,160 +579,173 @@ func (c *UtcpClient) CallToolStream(
- return func(ctx context.Context, args map[string]any) (transports.StreamResult, error) {
- return t.CallToolStream(ctx, cn, args, p)
+
+ resolved := make(map[string]*resolvedTool, len(current.tools)+len(tools))
+ for toolName, cached := range current.tools {
+ resolved[toolName] = cached
</file context>
1 participant
Summary by cubic
Optimizes tool lookup and streaming performance across the client and transports. Adds an indexed repository, snapshot-based client cache, safer streaming shutdown, and tighter HTTP auth/error handling.
Performance
Bug Fixes
gopkg.in/yaml.v3, path params no longer mutate caller args, and POST sends JSON body.CallToolStreamreturnsStreamResult; tests cover shutdown behavior.net.JoinHostPort, context‑aware dial, pooled UDP buffers, and simpler decoders for robustness.ProviderName,SetProviderName,NewProvider) for safer naming/creation and cleaner repo/client code;SearchToolsnow query‑based (pass a provider name to filter or use repository APIs directly).Written for commit 5b91851. Summary will update on new commits.