fix: three bug fixes for MCP tools and background embedding generation#219
fix: three bug fixes for MCP tools and background embedding generation#219syn-zhu wants to merge 1 commit intoagentregistry-dev:mainfrom
Conversation
…eneration
1. Use auth.WithSystemContext for background embedding goroutines
When generating embeddings asynchronously after server/agent creation,
the goroutine creates a bare context.Background() with no auth session.
Downstream database operations check IsSystemSession() and fail silently.
Fix: wrap with auth.WithSystemContext() to match the pattern used in
embeddings_sse.go, embeddings.go, and registry_app.go.
2. Initialize nil Config map in deployment responses
When a deployment has no config, the Config field is nil. JSON-serializing
a nil map produces `null` instead of `{}`, which breaks MCP tool consumers
that expect an object. Also applies case-insensitive comparison for
resource type filtering in list_deployments.
3. Default port 3000 for stdio transport in kagent translator
When translating a stdio-based MCP server to a kagent deployment, the
port is left at 0 if not explicitly set. This is invalid for the transport
adapter. Default to 3000 to match the KMCP CLI default.
|
Hi, @ilackarms gentle bump on this :) Should be a super quick review! |
| outIdx := 0 | ||
| for _, d := range deployments { | ||
| if args.ResourceType != "" && d.ResourceType != args.ResourceType { | ||
| if args.ResourceType != "" && !strings.EqualFold(d.ResourceType, args.ResourceType) { |
There was a problem hiding this comment.
nit: i'm personally a bit iffy on this. it makes sense, but as far as I could tell this is the only place in the code we'd support case-insensitive
| dep := *d | ||
| if dep.Config == nil { | ||
| dep.Config = map[string]string{} | ||
| } | ||
| resp.Deployments[outIdx] = dep |
There was a problem hiding this comment.
to avoid a nil Config, would it be better to update the list method so it gets instantiated there and we wouldn't need to do this check here?
so in registry_service.go, when listing k8s deployments listKubernetesDeployments, we'd simply
config := labels
if config == nil {
config = make(map[string]string)
}
d := {
...
config: config
...
}for what it's worth, our db listing code already does something similar to this, so we'd essentially be matching its behavior in the k8s listing
if d.Config == nil {
d.Config = make(map[string]string)
}
|
This pull request has been marked as stale due to no activity in the last 14 days. It will be closed in 3 days unless it is tagged "no stalebot" or other activity occurs. |
|
This pull request has been closed due to inactivity. |
2 participants
Description
Three bug fixes found while integrating AgentRegistry into a production Kubernetes platform.
auth.WithSystemContextfor background embedding goroutines — barecontext.Background()lacks the system auth session, causing embedding updates to fail silently. Matches the pattern already used inembeddings_sse.goandembeddings.go.Nil Config map initialization —
get_deploymentandlist_deploymentsMCP tools returnnullfor Config when the map is nil, breaking downstream JSON consumers. Also adds case-insensitiveresource_typefiltering.Default port 3000 for stdio transport — the kagent translator leaves
Deployment.Portat 0 for stdio servers, which is invalid. Defaults to 3000 to match the KMCP CLI.Fixes #217
Changes
internal/registry/service/registry_service.gocontext.Background()→auth.WithSystemContext(context.Background())in 2 goroutinesinternal/mcp/registryserver/server.gostrings.EqualFoldfor resource type filterinternal/runtime/translation/kagent/kagent_translator.goTest plan
EMBEDDINGS_ENABLED=true→ verify embedding column is populated (not NULL)get_deploymentfor a deployment with no config → verify Config is{}notnulllist_deploymentswithresource_type=server(lowercase) → verify results returned