From 78abdec887a3115cd606cafa2d3ff2cbb6075558 Mon Sep 17 00:00:00 2001 From: viettranx Date: Wed, 4 Mar 2026 23:14:31 +0700 Subject: [PATCH] feat(mcp+skills): per-agent registry isolation, skill access filtering, managed skill lifecycle (#57) Security: fix cross-agent MCP tool leak by cloning tool registry before MCP registration. MCP: enforce mcp_ prefix on all tool names, add cache invalidation on server/grant changes, add grant management endpoints, add group:mcp policy support for per-agent allowlisting. Skills: persist full YAML frontmatter, auto-promote/demote visibility on grant/revoke, simplify versioning, handle ZIP wrapper directories, expand tilde in skillsDir path. Fixes: wrap DeleteSkill cascade in transaction, use atomic NOT EXISTS for revoke-demote, create cancel context before storing server in map. Co-Authored-By: Claude Opus 4.6 --- cmd/gateway.go | 10 ++ cmd/gateway_managed.go | 47 +++++-- internal/agent/loop_history.go | 30 ++--- internal/agent/resolver.go | 133 ++++++++++++++----- internal/bus/types.go | 30 +++-- internal/config/config_channels.go | 144 ++++++++++----------- internal/gateway/methods/skills.go | 9 +- internal/http/mcp.go | 54 +++++++- internal/http/skills.go | 41 ++++-- internal/http/skills_grants.go | 34 +++-- internal/mcp/bridge_tool.go | 40 ++++-- internal/mcp/bridge_tool_test.go | 49 ++++++- internal/mcp/manager_connect.go | 20 ++- internal/skills/loader.go | 129 +++++++++++++++++- internal/skills/search.go | 4 +- internal/store/mcp_store.go | 67 +++++----- internal/store/pg/mcp_servers_access.go | 23 ++++ internal/store/pg/skills.go | 63 +++++++-- internal/store/pg/skills_grants.go | 40 +++++- internal/store/skill_store.go | 1 + internal/tools/policy.go | 8 +- internal/tools/policy_mcp_test.go | 6 +- internal/tools/skill_search.go | 5 +- ui/web/src/pages/mcp/hooks/use-mcp.ts | 8 +- ui/web/src/pages/mcp/mcp-form-dialog.tsx | 21 +-- ui/web/src/pages/mcp/mcp-grants-dialog.tsx | 56 +++++--- ui/web/src/pages/mcp/mcp-page.tsx | 4 +- 27 files changed, 774 insertions(+), 302 deletions(-) diff --git a/cmd/gateway.go b/cmd/gateway.go index 3b5745c0..783dec93 100644 --- a/cmd/gateway.go +++ b/cmd/gateway.go @@ -478,6 +478,16 @@ func runGateway() { toolsReg.Register(skillSearchTool) slog.Info("skill_search tool registered", "skills", len(skillsLoader.ListSkills())) + // Managed mode: wire skills-store directory into filesystem loader so agents + // can discover uploaded skills in their system prompt and BM25 search index. + if managedStores != nil && managedStores.Skills != nil { + storeDirs := managedStores.Skills.Dirs() + if len(storeDirs) > 0 { + skillsLoader.SetManagedDir(storeDirs[0]) + slog.Info("managed mode: skills-store directory wired into loader", "dir", storeDirs[0]) + } + } + // Managed mode: wire embedding-based skill search + per-agent access filtering if managedStores != nil && managedStores.Skills != nil { if sas, ok := managedStores.Skills.(store.SkillAccessStore); ok { diff --git a/cmd/gateway_managed.go b/cmd/gateway_managed.go index 791cbea9..d7f15ae7 100644 --- a/cmd/gateway_managed.go +++ b/cmd/gateway_managed.go @@ -83,20 +83,26 @@ func wireManagedExtras( } // 5. Set up agent resolver: lazy-creates Loops from DB + var skillAccessStore store.SkillAccessStore + if sas, ok := stores.Skills.(store.SkillAccessStore); ok { + skillAccessStore = sas + } + resolver := agent.NewManagedResolver(agent.ResolverDeps{ - AgentStore: stores.Agents, - ProviderReg: providerReg, - Bus: msgBus, - Sessions: sessStore, - Tools: toolsReg, - ToolPolicy: toolPE, - Skills: skillsLoader, - HasMemory: hasMemory, - TraceCollector: traceCollector, - EnsureUserFiles: ensureUserFiles, - ContextFileLoader: contextFileLoader, - BootstrapCleanup: buildBootstrapCleanup(stores.Agents), - InjectionAction: injectionAction, + AgentStore: stores.Agents, + ProviderReg: providerReg, + Bus: msgBus, + Sessions: sessStore, + Tools: toolsReg, + ToolPolicy: toolPE, + Skills: skillsLoader, + SkillAccessStore: skillAccessStore, + HasMemory: hasMemory, + TraceCollector: traceCollector, + EnsureUserFiles: ensureUserFiles, + ContextFileLoader: contextFileLoader, + BootstrapCleanup: buildBootstrapCleanup(stores.Agents), + InjectionAction: injectionAction, MaxMessageChars: appCfg.Gateway.MaxMessageChars, CompactionCfg: appCfg.Agents.Defaults.Compaction, ContextPruningCfg: appCfg.Agents.Defaults.ContextPruning, @@ -107,6 +113,7 @@ func wireManagedExtras( AgentLinkStore: stores.AgentLinks, TeamStore: stores.Teams, BuiltinToolStore: stores.BuiltinTools, + MCPStore: stores.MCP, GroupWriterCache: groupWriterCache, OnEvent: func(event agent.AgentEvent) { msgBus.Broadcast(bus.Event{ @@ -245,6 +252,18 @@ func wireManagedExtras( agentRouter.InvalidateAll() }) + // MCP cache: invalidate all agent caches when MCP servers/grants change + msgBus.Subscribe(bus.TopicCacheMCP, func(event bus.Event) { + if event.Name != protocol.EventCacheInvalidate { + return + } + payload, ok := event.Payload.(bus.CacheInvalidatePayload) + if !ok || payload.Kind != bus.CacheKindMCP { + return + } + agentRouter.InvalidateAll() + }) + // Cron cache: invalidate job cache on cron changes if ci, ok := stores.Cron.(store.CacheInvalidatable); ok { msgBus.Subscribe(bus.TopicCacheCron, func(event bus.Event) { @@ -476,7 +495,7 @@ func wireManagedHTTP(stores *store.Stores, token string, msgBus *bus.MessageBus, } if stores != nil && stores.MCP != nil { - mcpH = httpapi.NewMCPHandler(stores.MCP, token) + mcpH = httpapi.NewMCPHandler(stores.MCP, token, msgBus) } if stores != nil && stores.CustomTools != nil { diff --git a/internal/agent/loop_history.go b/internal/agent/loop_history.go index b5efc936..22ae48c1 100644 --- a/internal/agent/loop_history.go +++ b/internal/agent/loop_history.go @@ -65,21 +65,21 @@ func (l *Loop) buildMessages(ctx context.Context, history []providers.Message, s } systemPrompt := BuildSystemPrompt(SystemPromptConfig{ - AgentID: l.id, - Model: l.model, - Workspace: promptWorkspace, - Channel: channel, - OwnerIDs: l.ownerIDs, - Mode: mode, - ToolNames: l.tools.List(), - SkillsSummary: l.resolveSkillsSummary(skillFilter), - HasMemory: l.hasMemory, - HasSpawn: l.tools != nil && hasSpawn, - HasSkillSearch: hasSkillSearch, - ContextFiles: contextFiles, - ExtraPrompt: extraSystemPrompt, - SandboxEnabled: l.sandboxEnabled, - SandboxContainerDir: l.sandboxContainerDir, + AgentID: l.id, + Model: l.model, + Workspace: promptWorkspace, + Channel: channel, + OwnerIDs: l.ownerIDs, + Mode: mode, + ToolNames: l.tools.List(), + SkillsSummary: l.resolveSkillsSummary(skillFilter), + HasMemory: l.hasMemory, + HasSpawn: l.tools != nil && hasSpawn, + HasSkillSearch: hasSkillSearch, + ContextFiles: contextFiles, + ExtraPrompt: extraSystemPrompt, + SandboxEnabled: l.sandboxEnabled, + SandboxContainerDir: l.sandboxContainerDir, SandboxWorkspaceAccess: l.sandboxWorkspaceAccess, }) diff --git a/internal/agent/resolver.go b/internal/agent/resolver.go index b416a45d..475a4f34 100644 --- a/internal/agent/resolver.go +++ b/internal/agent/resolver.go @@ -12,6 +12,7 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/bootstrap" "github.com/nextlevelbuilder/goclaw/internal/bus" "github.com/nextlevelbuilder/goclaw/internal/config" + mcpbridge "github.com/nextlevelbuilder/goclaw/internal/mcp" "github.com/nextlevelbuilder/goclaw/internal/providers" "github.com/nextlevelbuilder/goclaw/internal/skills" "github.com/nextlevelbuilder/goclaw/internal/store" @@ -21,13 +22,13 @@ import ( // ResolverDeps holds shared dependencies for the managed-mode agent resolver. type ResolverDeps struct { - AgentStore store.AgentStore - ProviderReg *providers.Registry - Bus bus.EventPublisher - Sessions store.SessionStore - Tools *tools.Registry - ToolPolicy *tools.PolicyEngine - Skills *skills.Loader + AgentStore store.AgentStore + ProviderReg *providers.Registry + Bus bus.EventPublisher + Sessions store.SessionStore + Tools *tools.Registry + ToolPolicy *tools.PolicyEngine + Skills *skills.Loader HasMemory bool OnEvent func(AgentEvent) TraceCollector *tracing.Collector @@ -60,6 +61,12 @@ type ResolverDeps struct { // Builtin tool settings (managed mode) BuiltinToolStore store.BuiltinToolStore // nil if not managed + // MCP server store (managed mode) — for per-agent MCP tool loading + MCPStore store.MCPServerStore // nil if not managed or no MCP + + // Skill access store (managed mode) — for per-agent skill visibility filtering + SkillAccessStore store.SkillAccessStore // nil if not managed + // Group file writer cache (managed mode) GroupWriterCache *store.GroupWriterCache } @@ -239,6 +246,30 @@ func NewManagedResolver(deps ResolverDeps) ResolverFunc { } } + // Per-agent MCP servers: connect to granted MCP servers and register their tools. + // Uses a per-agent MCP Manager that queries the MCPServerStore for accessible servers. + // + // IMPORTANT: Always clone the registry before MCP registration to prevent + // cross-agent tool leaks. Without cloning, MCP BridgeTools registered for + // one agent pollute the shared deps. Tools and become visible to ALL agents + // (even those without MCP grants), because FilterTools reads from registry.List(). + hasMCPTools := false + if deps.MCPStore != nil { + if toolsReg == deps.Tools { + toolsReg = deps.Tools.Clone() + } + mcpMgr := mcpbridge.NewManager(toolsReg, mcpbridge.WithStore(deps.MCPStore)) + if err := mcpMgr.LoadForAgent(ctx, ag.ID, ""); err != nil { + slog.Warn("failed to load MCP servers for agent", "agent", agentKey, "error", err) + } else { + toolNames := mcpMgr.ToolNames() + if len(toolNames) > 0 { + hasMCPTools = true + slog.Info("mcp.agent.tools_loaded", "agent", agentKey, "tools", len(toolNames)) + } + } + } + // Per-agent memory: enabled if global memory manager exists AND // per-agent config doesn't explicitly disable it. hasMemory := deps.HasMemory @@ -261,33 +292,46 @@ func NewManagedResolver(deps ResolverDeps) ResolverFunc { } } - // Managed mode: SkillAllowList is nil (all filesystem skills available). - // Per-agent DB skill filtering is handled by skill_search tool via SkillAccessStore. + // Managed mode: filter skills by visibility + agent grants. + // Only public skills and explicitly granted internal skills appear in the system prompt. + var skillAllowList []string + if deps.SkillAccessStore != nil { + if accessible, err := deps.SkillAccessStore.ListAccessible(ctx, ag.ID, ""); err == nil { + skillAllowList = make([]string, 0, len(accessible)) + for _, sk := range accessible { + skillAllowList = append(skillAllowList, sk.Slug) + } + slog.Debug("skill visibility filter", "agent", agentKey, "accessible", len(skillAllowList)) + } else { + slog.Warn("failed to load accessible skills, falling back to all", "agent", agentKey, "error", err) + // nil = fallback to all (better than blocking all skills) + } + } loop := NewLoop(LoopConfig{ - ID: ag.AgentKey, - AgentUUID: ag.ID, - AgentType: ag.AgentType, - Provider: provider, - Model: ag.Model, - ContextWindow: contextWindow, - MaxIterations: maxIter, - Workspace: workspace, - Bus: deps.Bus, - Sessions: deps.Sessions, - Tools: toolsReg, - ToolPolicy: deps.ToolPolicy, - AgentToolPolicy: ag.ParseToolsConfig(), - SkillsLoader: deps.Skills, - // SkillAllowList: nil = all filesystem skills (managed DB skill filtering via skill_search) - HasMemory: hasMemory, - ContextFiles: contextFiles, - EnsureUserFiles: deps.EnsureUserFiles, - ContextFileLoader: deps.ContextFileLoader, - BootstrapCleanup: deps.BootstrapCleanup, - OnEvent: deps.OnEvent, - TraceCollector: deps.TraceCollector, - InjectionAction: deps.InjectionAction, + ID: ag.AgentKey, + AgentUUID: ag.ID, + AgentType: ag.AgentType, + Provider: provider, + Model: ag.Model, + ContextWindow: contextWindow, + MaxIterations: maxIter, + Workspace: workspace, + Bus: deps.Bus, + Sessions: deps.Sessions, + Tools: toolsReg, + ToolPolicy: deps.ToolPolicy, + AgentToolPolicy: agentToolPolicyWithMCP(ag.ParseToolsConfig(), hasMCPTools), + SkillsLoader: deps.Skills, + SkillAllowList: skillAllowList, + HasMemory: hasMemory, + ContextFiles: contextFiles, + EnsureUserFiles: deps.EnsureUserFiles, + ContextFileLoader: deps.ContextFileLoader, + BootstrapCleanup: deps.BootstrapCleanup, + OnEvent: deps.OnEvent, + TraceCollector: deps.TraceCollector, + InjectionAction: deps.InjectionAction, MaxMessageChars: deps.MaxMessageChars, CompactionCfg: compactionCfg, ContextPruningCfg: contextPruningCfg, @@ -295,9 +339,9 @@ func NewManagedResolver(deps ResolverDeps) ResolverFunc { SandboxContainerDir: sandboxContainerDir, SandboxWorkspaceAccess: sandboxWorkspaceAccess, BuiltinToolSettings: builtinSettings, - ThinkingLevel: ag.ParseThinkingLevel(), - GroupWriterCache: deps.GroupWriterCache, - TeamStore: deps.TeamStore, + ThinkingLevel: ag.ParseThinkingLevel(), + GroupWriterCache: deps.GroupWriterCache, + TeamStore: deps.TeamStore, }) slog.Info("resolved agent from DB", "agent", agentKey, "model", ag.Model, "provider", ag.Provider) @@ -474,3 +518,22 @@ func buildTeamMD(team *store.TeamData, members []store.TeamMemberData, selfID uu return sb.String() } + +// agentToolPolicyWithMCP injects "group:mcp" into the agent's alsoAllow list +// when MCP tools are loaded, ensuring the PolicyEngine doesn't block them. +func agentToolPolicyWithMCP(policy *config.ToolPolicySpec, hasMCP bool) *config.ToolPolicySpec { + if !hasMCP { + return policy + } + if policy == nil { + policy = &config.ToolPolicySpec{} + } + // Check if group:mcp is already present + for _, a := range policy.AlsoAllow { + if a == "group:mcp" { + return policy + } + } + policy.AlsoAllow = append(policy.AlsoAllow, "group:mcp") + return policy +} diff --git a/internal/bus/types.go b/internal/bus/types.go index 8bfdec45..58ae6ee3 100644 --- a/internal/bus/types.go +++ b/internal/bus/types.go @@ -9,10 +9,10 @@ type InboundMessage struct { ChatID string `json:"chat_id"` Content string `json:"content"` Media []string `json:"media,omitempty"` - SessionKey string `json:"session_key"` // deprecated: gateway builds canonical key - PeerKind string `json:"peer_kind,omitempty"` // "direct" or "group" (used for session key) - AgentID string `json:"agent_id,omitempty"` // target agent (for multi-agent routing) - UserID string `json:"user_id,omitempty"` // external user ID for per-user scoping (memory, bootstrap) + SessionKey string `json:"session_key"` // deprecated: gateway builds canonical key + PeerKind string `json:"peer_kind,omitempty"` // "direct" or "group" (used for session key) + AgentID string `json:"agent_id,omitempty"` // target agent (for multi-agent routing) + UserID string `json:"user_id,omitempty"` // external user ID for per-user scoping (memory, bootstrap) HistoryLimit int `json:"history_limit,omitempty"` // max turns to keep in context (0=unlimited, from channel config) ToolAllow []string `json:"tool_allow,omitempty"` // per-group tool allow list (nil = no restriction) Metadata map[string]string `json:"metadata,omitempty"` @@ -31,12 +31,12 @@ type OutboundMessage struct { type MediaAttachment struct { URL string `json:"url"` // file path or URL ContentType string `json:"content_type,omitempty"` // MIME type (e.g. "image/jpeg", "video/mp4") - Caption string `json:"caption,omitempty"` // optional caption for media + Caption string `json:"caption,omitempty"` // optional caption for media } // Event represents a server-side event to broadcast to WebSocket clients. type Event struct { - Name string `json:"name"` // event name (e.g. "agent", "chat", "health") + Name string `json:"name"` // event name (e.g. "agent", "chat", "health") Payload interface{} `json:"payload,omitempty"` } @@ -50,9 +50,10 @@ const ( CacheKindChannelInstances = "channel_instances" CacheKindBuiltinTools = "builtin_tools" CacheKindTeam = "team" - CacheKindUserWorkspace = "user_workspace" - CacheKindGroupFileWriters = "group_file_writers" - CacheKindSkillGrants = "skill_grants" + CacheKindUserWorkspace = "user_workspace" + CacheKindGroupFileWriters = "group_file_writers" + CacheKindSkillGrants = "skill_grants" + CacheKindMCP = "mcp" ) // Topic constants for msgBus.Subscribe() / Broadcast(). @@ -65,11 +66,12 @@ const ( TopicCacheBuiltinTools = "cache:builtin_tools" TopicCacheTeam = "cache:team" TopicCacheUserWorkspace = "cache:user_workspace" - TopicCacheChannelInstances = "cache:channel_instances" - TopicCacheGroupFileWriters = "cache:group_file_writers" - TopicCacheSkillGrants = "cache:skill_grants" - TopicChannelStreaming = "channel-streaming" - TopicConfigChanged = "config:changed" + TopicCacheChannelInstances = "cache:channel_instances" + TopicCacheGroupFileWriters = "cache:group_file_writers" + TopicCacheSkillGrants = "cache:skill_grants" + TopicCacheMCP = "cache:mcp" + TopicChannelStreaming = "channel-streaming" + TopicConfigChanged = "config:changed" ) // CacheInvalidatePayload signals cache layers to evict stale entries. diff --git a/internal/config/config_channels.go b/internal/config/config_channels.go index 53b57a4a..541f3e91 100644 --- a/internal/config/config_channels.go +++ b/internal/config/config_channels.go @@ -2,10 +2,10 @@ package config // ChannelsConfig contains per-channel configuration. type ChannelsConfig struct { - Telegram TelegramConfig `json:"telegram"` - Discord DiscordConfig `json:"discord"` - Slack SlackConfig `json:"slack"` - WhatsApp WhatsAppConfig `json:"whatsapp"` + Telegram TelegramConfig `json:"telegram"` + Discord DiscordConfig `json:"discord"` + Slack SlackConfig `json:"slack"` + WhatsApp WhatsAppConfig `json:"whatsapp"` Zalo ZaloConfig `json:"zalo"` ZaloPersonal ZaloPersonalConfig `json:"zalo_personal"` Feishu FeishuConfig `json:"feishu"` @@ -16,15 +16,15 @@ type TelegramConfig struct { Token string `json:"token"` Proxy string `json:"proxy,omitempty"` AllowFrom FlexibleStringSlice `json:"allow_from"` - DMPolicy string `json:"dm_policy,omitempty"` // "pairing" (default), "allowlist", "open", "disabled" - GroupPolicy string `json:"group_policy,omitempty"` // "open" (default), "allowlist", "disabled" - RequireMention *bool `json:"require_mention,omitempty"` // require @bot mention in groups (default true) - HistoryLimit int `json:"history_limit,omitempty"` // max pending group messages for context (default 50, 0=disabled) - DMStream *bool `json:"dm_stream,omitempty"` // enable streaming for DMs (default false) — edits placeholder progressively; disabled pending Telegram draft API fixes (tdesktop#10315) - GroupStream *bool `json:"group_stream,omitempty"` // enable streaming for groups (default false) — sends new message, edits progressively - ReactionLevel string `json:"reaction_level,omitempty"` // "off" (default), "minimal", "full" — status emoji reactions - MediaMaxBytes int64 `json:"media_max_bytes,omitempty"` // max media download size in bytes (default 20MB) - LinkPreview *bool `json:"link_preview,omitempty"` // enable URL previews in messages (default true) + DMPolicy string `json:"dm_policy,omitempty"` // "pairing" (default), "allowlist", "open", "disabled" + GroupPolicy string `json:"group_policy,omitempty"` // "open" (default), "allowlist", "disabled" + RequireMention *bool `json:"require_mention,omitempty"` // require @bot mention in groups (default true) + HistoryLimit int `json:"history_limit,omitempty"` // max pending group messages for context (default 50, 0=disabled) + DMStream *bool `json:"dm_stream,omitempty"` // enable streaming for DMs (default false) — edits placeholder progressively; disabled pending Telegram draft API fixes (tdesktop#10315) + GroupStream *bool `json:"group_stream,omitempty"` // enable streaming for groups (default false) — sends new message, edits progressively + ReactionLevel string `json:"reaction_level,omitempty"` // "off" (default), "minimal", "full" — status emoji reactions + MediaMaxBytes int64 `json:"media_max_bytes,omitempty"` // max media download size in bytes (default 20MB) + LinkPreview *bool `json:"link_preview,omitempty"` // enable URL previews in messages (default true) // Optional STT (Speech-to-Text) pipeline for voice/audio inbound messages. // When stt_proxy_url is set, audio/voice messages are transcribed before being forwarded to the agent. @@ -45,15 +45,15 @@ type TelegramConfig struct { // TelegramGroupConfig defines per-group overrides for a Telegram channel. // Matching TS TelegramGroupConfig in src/config/types.telegram.ts. type TelegramGroupConfig struct { - GroupPolicy string `json:"group_policy,omitempty"` // override group policy for this group - RequireMention *bool `json:"require_mention,omitempty"` // override require_mention for this group - AllowFrom FlexibleStringSlice `json:"allow_from,omitempty"` // override allow_from for this group - Enabled *bool `json:"enabled,omitempty"` // disable bot for this group (default: true) - Skills []string `json:"skills,omitempty"` // skill whitelist (nil = all, [] = none) - Tools []string `json:"tools,omitempty"` // tool allow list (nil = all, supports "group:xxx") - SystemPrompt string `json:"system_prompt,omitempty"` // extra system prompt for this group - Topics map[string]*TelegramTopicConfig `json:"topics,omitempty"` // per-topic overrides (key: thread ID string) - Quota *QuotaWindow `json:"quota,omitempty"` // per-group quota override + GroupPolicy string `json:"group_policy,omitempty"` // override group policy for this group + RequireMention *bool `json:"require_mention,omitempty"` // override require_mention for this group + AllowFrom FlexibleStringSlice `json:"allow_from,omitempty"` // override allow_from for this group + Enabled *bool `json:"enabled,omitempty"` // disable bot for this group (default: true) + Skills []string `json:"skills,omitempty"` // skill whitelist (nil = all, [] = none) + Tools []string `json:"tools,omitempty"` // tool allow list (nil = all, supports "group:xxx") + SystemPrompt string `json:"system_prompt,omitempty"` // extra system prompt for this group + Topics map[string]*TelegramTopicConfig `json:"topics,omitempty"` // per-topic overrides (key: thread ID string) + Quota *QuotaWindow `json:"quota,omitempty"` // per-group quota override } // TelegramTopicConfig defines per-topic overrides within a Telegram group. @@ -85,7 +85,7 @@ type SlackConfig struct { AllowFrom FlexibleStringSlice `json:"allow_from"` DMPolicy string `json:"dm_policy,omitempty"` // "open" (default), "allowlist", "disabled" GroupPolicy string `json:"group_policy,omitempty"` // "open" (default), "allowlist", "disabled" - RequireMention bool `json:"require_mention,omitempty"` // only respond to @bot in channels (default true) + RequireMention bool `json:"require_mention,omitempty"` // only respond to @bot in channels (default true) } type WhatsAppConfig struct { @@ -100,7 +100,7 @@ type ZaloConfig struct { Enabled bool `json:"enabled"` Token string `json:"token"` AllowFrom FlexibleStringSlice `json:"allow_from"` - DMPolicy string `json:"dm_policy,omitempty"` // "pairing" (default), "allowlist", "open", "disabled" + DMPolicy string `json:"dm_policy,omitempty"` // "pairing" (default), "allowlist", "open", "disabled" WebhookURL string `json:"webhook_url,omitempty"` WebhookSecret string `json:"webhook_secret,omitempty"` MediaMaxMB int `json:"media_max_mb,omitempty"` // default 5 @@ -121,13 +121,13 @@ type FeishuConfig struct { AppSecret string `json:"app_secret"` EncryptKey string `json:"encrypt_key,omitempty"` VerificationToken string `json:"verification_token,omitempty"` - Domain string `json:"domain,omitempty"` // "lark" (default/global), "feishu" (China), or custom URL - ConnectionMode string `json:"connection_mode,omitempty"` // "websocket" (default), "webhook" - WebhookPort int `json:"webhook_port,omitempty"` // default 3000 - WebhookPath string `json:"webhook_path,omitempty"` // default "/feishu/events" + Domain string `json:"domain,omitempty"` // "lark" (default/global), "feishu" (China), or custom URL + ConnectionMode string `json:"connection_mode,omitempty"` // "websocket" (default), "webhook" + WebhookPort int `json:"webhook_port,omitempty"` // default 3000 + WebhookPath string `json:"webhook_path,omitempty"` // default "/feishu/events" AllowFrom FlexibleStringSlice `json:"allow_from"` - DMPolicy string `json:"dm_policy,omitempty"` // "pairing" (default) - GroupPolicy string `json:"group_policy,omitempty"` // "open" (default) + DMPolicy string `json:"dm_policy,omitempty"` // "pairing" (default) + GroupPolicy string `json:"group_policy,omitempty"` // "open" (default) GroupAllowFrom FlexibleStringSlice `json:"group_allow_from,omitempty"` RequireMention *bool `json:"require_mention,omitempty"` // default true (groups) TopicSessionMode string `json:"topic_session_mode,omitempty"` // "disabled" (default) @@ -201,45 +201,45 @@ type QuotaConfig struct { // GatewayConfig controls the gateway server. type GatewayConfig struct { - Host string `json:"host"` - Port int `json:"port"` - Token string `json:"token,omitempty"` // bearer token for WS/HTTP auth - OwnerIDs []string `json:"owner_ids,omitempty"` // sender IDs considered "owner" - AllowedOrigins []string `json:"allowed_origins,omitempty"` // WebSocket CORS whitelist (empty = allow all) - MaxMessageChars int `json:"max_message_chars,omitempty"` // max user message characters (default 32000) - RateLimitRPM int `json:"rate_limit_rpm,omitempty"` // rate limit: requests per minute per user (default 20, 0 = disabled) - InjectionAction string `json:"injection_action,omitempty"` // prompt injection action: "log", "warn" (default), "block", "off" - InboundDebounceMs int `json:"inbound_debounce_ms,omitempty"` // merge rapid messages from same sender (default 1000ms, -1 = disabled) + Host string `json:"host"` + Port int `json:"port"` + Token string `json:"token,omitempty"` // bearer token for WS/HTTP auth + OwnerIDs []string `json:"owner_ids,omitempty"` // sender IDs considered "owner" + AllowedOrigins []string `json:"allowed_origins,omitempty"` // WebSocket CORS whitelist (empty = allow all) + MaxMessageChars int `json:"max_message_chars,omitempty"` // max user message characters (default 32000) + RateLimitRPM int `json:"rate_limit_rpm,omitempty"` // rate limit: requests per minute per user (default 20, 0 = disabled) + InjectionAction string `json:"injection_action,omitempty"` // prompt injection action: "log", "warn" (default), "block", "off" + InboundDebounceMs int `json:"inbound_debounce_ms,omitempty"` // merge rapid messages from same sender (default 1000ms, -1 = disabled) Quota *QuotaConfig `json:"quota,omitempty"` // per-user/group request quotas (managed mode only) } // ToolsConfig controls tool availability, policy, and web search. type ToolsConfig struct { - Profile string `json:"profile,omitempty"` // global profile: "minimal", "coding", "messaging", "full" - Allow []string `json:"allow,omitempty"` // global allow list (tool names or "group:xxx") - Deny []string `json:"deny,omitempty"` // global deny list - AlsoAllow []string `json:"alsoAllow,omitempty"` // additive: adds without removing existing - ByProvider map[string]*ToolPolicySpec `json:"byProvider,omitempty"` // per-provider overrides - ExecApproval ExecApprovalCfg `json:"execApproval,omitempty"` // exec command approval settings - WebFetch WebFetchPolicyConfig `json:"web_fetch"` // domain policy for URL fetching - Web WebToolsConfig `json:"web"` - Browser BrowserToolConfig `json:"browser"` - RateLimitPerHour int `json:"rate_limit_per_hour,omitempty"` // max tool executions per hour per session (0 = disabled) - ScrubCredentials *bool `json:"scrub_credentials,omitempty"` // auto-redact API keys/tokens in tool output (default true) - McpServers map[string]*MCPServerConfig `json:"mcp_servers,omitempty"` // external MCP server connections + Profile string `json:"profile,omitempty"` // global profile: "minimal", "coding", "messaging", "full" + Allow []string `json:"allow,omitempty"` // global allow list (tool names or "group:xxx") + Deny []string `json:"deny,omitempty"` // global deny list + AlsoAllow []string `json:"alsoAllow,omitempty"` // additive: adds without removing existing + ByProvider map[string]*ToolPolicySpec `json:"byProvider,omitempty"` // per-provider overrides + ExecApproval ExecApprovalCfg `json:"execApproval,omitempty"` // exec command approval settings + WebFetch WebFetchPolicyConfig `json:"web_fetch"` // domain policy for URL fetching + Web WebToolsConfig `json:"web"` + Browser BrowserToolConfig `json:"browser"` + RateLimitPerHour int `json:"rate_limit_per_hour,omitempty"` // max tool executions per hour per session (0 = disabled) + ScrubCredentials *bool `json:"scrub_credentials,omitempty"` // auto-redact API keys/tokens in tool output (default true) + McpServers map[string]*MCPServerConfig `json:"mcp_servers,omitempty"` // external MCP server connections } // MCPServerConfig configures a single external MCP server connection. type MCPServerConfig struct { - Transport string `json:"transport"` // "stdio", "sse", "streamable-http" - Command string `json:"command,omitempty"` // stdio: command to spawn - Args []string `json:"args,omitempty"` // stdio: command arguments - Env map[string]string `json:"env,omitempty"` // stdio: extra environment variables - URL string `json:"url,omitempty"` // sse/http: server URL - Headers map[string]string `json:"headers,omitempty"` // sse/http: extra HTTP headers - Enabled *bool `json:"enabled,omitempty"` // default true - ToolPrefix string `json:"tool_prefix,omitempty"` // prefix for tool names (avoids collisions) - TimeoutSec int `json:"timeout_sec,omitempty"` // per-tool-call timeout in seconds (default 60) + Transport string `json:"transport"` // "stdio", "sse", "streamable-http" + Command string `json:"command,omitempty"` // stdio: command to spawn + Args []string `json:"args,omitempty"` // stdio: command arguments + Env map[string]string `json:"env,omitempty"` // stdio: extra environment variables + URL string `json:"url,omitempty"` // sse/http: server URL + Headers map[string]string `json:"headers,omitempty"` // sse/http: extra HTTP headers + Enabled *bool `json:"enabled,omitempty"` // default true + ToolPrefix string `json:"tool_prefix,omitempty"` // prefix for tool names (avoids collisions) + TimeoutSec int `json:"timeout_sec,omitempty"` // per-tool-call timeout in seconds (default 60) } // IsEnabled returns whether this MCP server is enabled (default true). @@ -256,9 +256,9 @@ type ExecApprovalCfg struct { // WebFetchPolicyConfig controls domain filtering for the web_fetch tool. type WebFetchPolicyConfig struct { - Policy string `json:"policy,omitempty"` // "allow_all" (default), "allowlist" - AllowedDomains []string `json:"allowed_domains,omitempty"` // e.g. ["github.com", "*.example.com"] - BlockedDomains []string `json:"blocked_domains,omitempty"` // always checked regardless of policy + Policy string `json:"policy,omitempty"` // "allow_all" (default), "allowlist" + AllowedDomains []string `json:"allowed_domains,omitempty"` // e.g. ["github.com", "*.example.com"] + BlockedDomains []string `json:"blocked_domains,omitempty"` // always checked regardless of policy } // BrowserToolConfig controls the browser automation tool. @@ -311,20 +311,20 @@ type DuckDuckGoConfig struct { // SessionsConfig controls session behavior. // Matching TS src/config/sessions/types.ts + src/config/types.base.ts. type SessionsConfig struct { - Storage string `json:"storage"` // directory for session files - Scope string `json:"scope,omitempty"` // "per-sender" (default), "global" - DmScope string `json:"dm_scope,omitempty"` // "main", "per-peer", "per-channel-peer" (default), "per-account-channel-peer" - MainKey string `json:"main_key,omitempty"` // main session key suffix (default "main", used when dm_scope="main") + Storage string `json:"storage"` // directory for session files + Scope string `json:"scope,omitempty"` // "per-sender" (default), "global" + DmScope string `json:"dm_scope,omitempty"` // "main", "per-peer", "per-channel-peer" (default), "per-account-channel-peer" + MainKey string `json:"main_key,omitempty"` // main session key suffix (default "main", used when dm_scope="main") } // TtsConfig configures text-to-speech. // Matching TS src/config/types.tts.ts. type TtsConfig struct { - Provider string `json:"provider,omitempty"` // "openai", "elevenlabs", "edge", "minimax" - Auto string `json:"auto,omitempty"` // "off" (default), "always", "inbound", "tagged" - Mode string `json:"mode,omitempty"` // "final" (default), "all" - MaxLength int `json:"max_length,omitempty"` // max text length before truncation (default 1500) - TimeoutMs int `json:"timeout_ms,omitempty"` // API timeout in ms (default 30000) + Provider string `json:"provider,omitempty"` // "openai", "elevenlabs", "edge", "minimax" + Auto string `json:"auto,omitempty"` // "off" (default), "always", "inbound", "tagged" + Mode string `json:"mode,omitempty"` // "final" (default), "all" + MaxLength int `json:"max_length,omitempty"` // max text length before truncation (default 1500) + TimeoutMs int `json:"timeout_ms,omitempty"` // API timeout in ms (default 30000) OpenAI TtsOpenAIConfig `json:"openai,omitempty"` ElevenLabs TtsElevenLabsConfig `json:"elevenlabs,omitempty"` Edge TtsEdgeConfig `json:"edge,omitempty"` diff --git a/internal/gateway/methods/skills.go b/internal/gateway/methods/skills.go index b1a06158..b61e7ed4 100644 --- a/internal/gateway/methods/skills.go +++ b/internal/gateway/methods/skills.go @@ -32,11 +32,16 @@ func (m *SkillsMethods) handleList(_ context.Context, client *gateway.Client, re result := make([]map[string]interface{}, 0, len(allSkills)) for _, s := range allSkills { - result = append(result, map[string]interface{}{ + entry := map[string]interface{}{ "name": s.Name, + "slug": s.Slug, "description": s.Description, "source": s.Source, - }) + } + if s.ID != "" { + entry["id"] = s.ID + } + result = append(result, entry) } client.SendResponse(protocol.NewOKResponse(req.ID, map[string]interface{}{ diff --git a/internal/http/mcp.go b/internal/http/mcp.go index 3321e6e9..da76d780 100644 --- a/internal/http/mcp.go +++ b/internal/http/mcp.go @@ -7,18 +7,31 @@ import ( "github.com/google/uuid" + "github.com/nextlevelbuilder/goclaw/internal/bus" "github.com/nextlevelbuilder/goclaw/internal/store" + "github.com/nextlevelbuilder/goclaw/pkg/protocol" ) // MCPHandler handles MCP server management HTTP endpoints (managed mode). type MCPHandler struct { - store store.MCPServerStore - token string + store store.MCPServerStore + token string + msgBus *bus.MessageBus } // NewMCPHandler creates a handler for MCP server management endpoints. -func NewMCPHandler(s store.MCPServerStore, token string) *MCPHandler { - return &MCPHandler{store: s, token: token} +func NewMCPHandler(s store.MCPServerStore, token string, msgBus *bus.MessageBus) *MCPHandler { + return &MCPHandler{store: s, token: token, msgBus: msgBus} +} + +func (h *MCPHandler) emitCacheInvalidate() { + if h.msgBus == nil { + return + } + h.msgBus.Broadcast(bus.Event{ + Name: protocol.EventCacheInvalidate, + Payload: bus.CacheInvalidatePayload{Kind: bus.CacheKindMCP}, + }) } // RegisterRoutes registers all MCP management routes on the given mux. @@ -31,6 +44,7 @@ func (h *MCPHandler) RegisterRoutes(mux *http.ServeMux) { mux.HandleFunc("DELETE /v1/mcp/servers/{id}", h.auth(h.handleDeleteServer)) // Agent grants + mux.HandleFunc("GET /v1/mcp/servers/{id}/grants", h.auth(h.handleListServerGrants)) mux.HandleFunc("POST /v1/mcp/servers/{id}/grants/agent", h.auth(h.handleGrantAgent)) mux.HandleFunc("DELETE /v1/mcp/servers/{id}/grants/agent/{agentID}", h.auth(h.handleRevokeAgent)) mux.HandleFunc("GET /v1/mcp/grants/agent/{agentID}", h.auth(h.handleListAgentGrants)) @@ -101,6 +115,7 @@ func (h *MCPHandler) handleCreateServer(w http.ResponseWriter, r *http.Request) return } + h.emitCacheInvalidate() writeJSON(w, http.StatusCreated, srv) } @@ -146,6 +161,7 @@ func (h *MCPHandler) handleUpdateServer(w http.ResponseWriter, r *http.Request) return } + h.emitCacheInvalidate() writeJSON(w, http.StatusOK, map[string]string{"status": "updated"}) } @@ -162,6 +178,7 @@ func (h *MCPHandler) handleDeleteServer(w http.ResponseWriter, r *http.Request) return } + h.emitCacheInvalidate() writeJSON(w, http.StatusOK, map[string]string{"status": "deleted"}) } @@ -175,7 +192,7 @@ func (h *MCPHandler) handleGrantAgent(w http.ResponseWriter, r *http.Request) { } var req struct { - AgentID string `json:"agent_id"` + AgentID string `json:"agent_id"` ToolAllow json.RawMessage `json:"tool_allow,omitempty"` ToolDeny json.RawMessage `json:"tool_deny,omitempty"` } @@ -205,6 +222,7 @@ func (h *MCPHandler) handleGrantAgent(w http.ResponseWriter, r *http.Request) { return } + h.emitCacheInvalidate() writeJSON(w, http.StatusCreated, map[string]string{"status": "granted"}) } @@ -227,6 +245,7 @@ func (h *MCPHandler) handleRevokeAgent(w http.ResponseWriter, r *http.Request) { return } + h.emitCacheInvalidate() writeJSON(w, http.StatusOK, map[string]string{"status": "revoked"}) } @@ -247,6 +266,23 @@ func (h *MCPHandler) handleListAgentGrants(w http.ResponseWriter, r *http.Reques writeJSON(w, http.StatusOK, map[string]interface{}{"grants": grants}) } +func (h *MCPHandler) handleListServerGrants(w http.ResponseWriter, r *http.Request) { + serverID, err := uuid.Parse(r.PathValue("id")) + if err != nil { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": "invalid server ID"}) + return + } + + grants, err := h.store.ListServerGrants(r.Context(), serverID) + if err != nil { + slog.Error("mcp.list_server_grants", "error", err) + writeJSON(w, http.StatusInternalServerError, map[string]string{"error": "failed to list grants"}) + return + } + + writeJSON(w, http.StatusOK, map[string]interface{}{"grants": grants}) +} + // --- User grants --- func (h *MCPHandler) handleGrantUser(w http.ResponseWriter, r *http.Request) { @@ -257,7 +293,7 @@ func (h *MCPHandler) handleGrantUser(w http.ResponseWriter, r *http.Request) { } var req struct { - UserID string `json:"user_id"` + UserID string `json:"user_id"` ToolAllow json.RawMessage `json:"tool_allow,omitempty"` ToolDeny json.RawMessage `json:"tool_deny,omitempty"` } @@ -290,6 +326,7 @@ func (h *MCPHandler) handleGrantUser(w http.ResponseWriter, r *http.Request) { return } + h.emitCacheInvalidate() writeJSON(w, http.StatusCreated, map[string]string{"status": "granted"}) } @@ -312,6 +349,7 @@ func (h *MCPHandler) handleRevokeUser(w http.ResponseWriter, r *http.Request) { return } + h.emitCacheInvalidate() writeJSON(w, http.StatusOK, map[string]string{"status": "revoked"}) } @@ -380,6 +418,10 @@ func (h *MCPHandler) handleReviewRequest(w http.ResponseWriter, r *http.Request) return } + if req.Approved { + h.emitCacheInvalidate() + } + status := "rejected" if req.Approved { status = "approved" diff --git a/internal/http/skills.go b/internal/http/skills.go index fbf5c66a..dda9a088 100644 --- a/internal/http/skills.go +++ b/internal/http/skills.go @@ -27,7 +27,7 @@ var slugRegexp = regexp.MustCompile(`^[a-z0-9][a-z0-9-]*[a-z0-9]$`) // SkillsHandler handles skill management HTTP endpoints (managed mode). type SkillsHandler struct { - skills *pg.PGSkillStore + skills *pg.PGSkillStore baseDir string // filesystem base for skill content token string msgBus *bus.MessageBus @@ -181,16 +181,27 @@ func (h *SkillsHandler) handleUpload(w http.ResponseWriter, r *http.Request) { } defer zr.Close() - // Validate: must have SKILL.md at root + // Validate: must have SKILL.md at root or inside a single top-level directory. + // Many ZIP tools wrap contents in a folder (e.g. "my-skill/SKILL.md"). var skillMD *zip.File + var stripPrefix string for _, f := range zr.File { - if f.Name == "SKILL.md" || f.Name == "./SKILL.md" { + name := strings.TrimPrefix(f.Name, "./") + if name == "SKILL.md" { skillMD = f + stripPrefix = "" + break + } + // Allow one level of directory nesting: "dirname/SKILL.md" + parts := strings.SplitN(name, "/", 3) + if len(parts) == 2 && parts[1] == "SKILL.md" && !f.FileInfo().IsDir() { + skillMD = f + stripPrefix = parts[0] + "/" break } } if skillMD == nil { - writeJSON(w, http.StatusBadRequest, map[string]string{"error": "ZIP must contain SKILL.md at root"}) + writeJSON(w, http.StatusBadRequest, map[string]string{"error": "ZIP must contain SKILL.md at root (or inside a single top-level directory)"}) return } @@ -201,7 +212,7 @@ func (h *SkillsHandler) handleUpload(w http.ResponseWriter, r *http.Request) { return } - name, description, slug := parseSkillFrontmatter(skillContent) + name, description, slug, frontmatter := parseSkillFrontmatter(skillContent) if name == "" { writeJSON(w, http.StatusBadRequest, map[string]string{"error": "SKILL.md must have a name in frontmatter"}) return @@ -214,12 +225,8 @@ func (h *SkillsHandler) handleUpload(w http.ResponseWriter, r *http.Request) { return } - // Determine version (increment if slug already exists) - version := 1 - if existing, ok := h.skills.GetSkill(slug); ok { - _ = existing - version = h.skills.GetNextVersion(slug) - } + // Determine version (always increment — includes archived skills so re-upload gets v2+) + version := h.skills.GetNextVersion(slug) // Extract to filesystem: baseDir/slug/version/ destDir := filepath.Join(h.baseDir, slug, fmt.Sprintf("%d", version)) @@ -232,8 +239,16 @@ func (h *SkillsHandler) handleUpload(w http.ResponseWriter, r *http.Request) { if f.FileInfo().IsDir() { continue } + // Strip wrapper directory prefix if ZIP had one + entryName := strings.TrimPrefix(f.Name, "./") + if stripPrefix != "" { + entryName = strings.TrimPrefix(entryName, stripPrefix) + if entryName == "" { + continue + } + } // Security: prevent path traversal - name := filepath.Clean(f.Name) + name := filepath.Clean(entryName) if strings.Contains(name, "..") { continue } @@ -260,6 +275,7 @@ func (h *SkillsHandler) handleUpload(w http.ResponseWriter, r *http.Request) { FilePath: destDir, FileSize: size, FileHash: &fileHash, + Frontmatter: frontmatter, } id, err := h.skills.CreateSkillManaged(r.Context(), skill) @@ -296,4 +312,3 @@ func (h *SkillsHandler) handleListAgentSkills(w http.ResponseWriter, r *http.Req writeJSON(w, http.StatusOK, map[string]interface{}{"skills": skills}) } - diff --git a/internal/http/skills_grants.go b/internal/http/skills_grants.go index f54522ff..6ef1c877 100644 --- a/internal/http/skills_grants.go +++ b/internal/http/skills_grants.go @@ -150,29 +150,39 @@ func readZipFile(f *zip.File) (string, error) { } // parseSkillFrontmatter extracts name, description, and slug from SKILL.md YAML frontmatter. -func parseSkillFrontmatter(content string) (name, description, slug string) { +// Also returns the full parsed frontmatter as a map for DB storage. +func parseSkillFrontmatter(content string) (name, description, slug string, allFields map[string]string) { + allFields = make(map[string]string) if !strings.HasPrefix(content, "---") { - return "", "", "" + return "", "", "", allFields } end := strings.Index(content[3:], "---") if end < 0 { - return "", "", "" + return "", "", "", allFields } fm := content[3 : 3+end] for _, line := range strings.Split(fm, "\n") { line = strings.TrimSpace(line) - if strings.HasPrefix(line, "name:") { - name = strings.TrimSpace(strings.TrimPrefix(line, "name:")) - name = strings.Trim(name, `"'`) + if line == "" || strings.HasPrefix(line, "#") { + continue } - if strings.HasPrefix(line, "description:") { - description = strings.TrimSpace(strings.TrimPrefix(line, "description:")) - description = strings.Trim(description, `"'`) + parts := strings.SplitN(line, ":", 2) + if len(parts) != 2 { + continue } - if strings.HasPrefix(line, "slug:") { - slug = strings.TrimSpace(strings.TrimPrefix(line, "slug:")) - slug = strings.Trim(slug, `"'`) + key := strings.TrimSpace(parts[0]) + val := strings.TrimSpace(parts[1]) + val = strings.Trim(val, `"'`) + allFields[key] = val + + switch key { + case "name": + name = val + case "description": + description = val + case "slug": + slug = val } } return diff --git a/internal/mcp/bridge_tool.go b/internal/mcp/bridge_tool.go index 3100836d..91593068 100644 --- a/internal/mcp/bridge_tool.go +++ b/internal/mcp/bridge_tool.go @@ -16,8 +16,8 @@ import ( // It delegates Execute calls to the MCP server via the client. type BridgeTool struct { serverName string - toolName string // original MCP tool name - registeredName string // may include prefix: "{prefix}__{toolName}" + toolName string // original MCP tool name + registeredName string // may include prefix: "{prefix}__{toolName}" description string inputSchema map[string]interface{} // JSON Schema for parameters client *mcpclient.Client @@ -26,12 +26,12 @@ type BridgeTool struct { } // NewBridgeTool creates a BridgeTool from an MCP Tool definition. +// The tool name is always prefixed with "mcp_" to distinguish MCP tools from native tools. +// If prefix is empty, it is auto-derived from the server name. func NewBridgeTool(serverName string, mcpTool mcpgo.Tool, client *mcpclient.Client, prefix string, timeoutSec int, connected *atomic.Bool) *BridgeTool { name := mcpTool.Name - registered := name - if prefix != "" { - registered = prefix + "__" + name - } + effectivePrefix := ensureMCPPrefix(prefix, serverName) + registered := effectivePrefix + "__" + name if timeoutSec <= 0 { timeoutSec = 60 @@ -51,9 +51,31 @@ func NewBridgeTool(serverName string, mcpTool mcpgo.Tool, client *mcpclient.Clie } } -func (t *BridgeTool) Name() string { return t.registeredName } -func (t *BridgeTool) Description() string { return t.description } -func (t *BridgeTool) Parameters() map[string]interface{} { return t.inputSchema } +// ensureMCPPrefix guarantees the tool prefix starts with "mcp_". +// - Empty prefix → "mcp_{sanitizedServerName}" +// - Prefix without "mcp_" → "mcp_{prefix}" +// - Prefix already starting with "mcp_" → unchanged +// +// Server name hyphens are converted to underscores for tool name compatibility. +func ensureMCPPrefix(prefix, serverName string) string { + const mcpPfx = "mcp_" + + if prefix == "" { + // Auto-derive from server name: "my-server" → "mcp_my_server" + sanitized := strings.ReplaceAll(serverName, "-", "_") + return mcpPfx + sanitized + } + + if !strings.HasPrefix(prefix, mcpPfx) { + return mcpPfx + prefix + } + + return prefix +} + +func (t *BridgeTool) Name() string { return t.registeredName } +func (t *BridgeTool) Description() string { return t.description } +func (t *BridgeTool) Parameters() map[string]interface{} { return t.inputSchema } // ServerName returns the name of the MCP server this tool belongs to. func (t *BridgeTool) ServerName() string { return t.serverName } diff --git a/internal/mcp/bridge_tool_test.go b/internal/mcp/bridge_tool_test.go index b64334ac..01e19d6d 100644 --- a/internal/mcp/bridge_tool_test.go +++ b/internal/mcp/bridge_tool_test.go @@ -79,10 +79,10 @@ func TestBridgeToolNaming(t *testing.T) { InputSchema: mcpgo.ToolInputSchema{Type: "object"}, } - // Without prefix + // Without prefix → auto-derived from server name bt := NewBridgeTool("myserver", mcpTool, nil, "", 30, nil) - if bt.Name() != "query" { - t.Errorf("expected name=query, got %s", bt.Name()) + if bt.Name() != "mcp_myserver__query" { + t.Errorf("expected name=mcp_myserver__query, got %s", bt.Name()) } if bt.ServerName() != "myserver" { t.Errorf("expected serverName=myserver, got %s", bt.ServerName()) @@ -91,17 +91,54 @@ func TestBridgeToolNaming(t *testing.T) { t.Errorf("expected originalName=query, got %s", bt.OriginalName()) } - // With prefix + // With non-mcp_ prefix → gets mcp_ prepended bt2 := NewBridgeTool("myserver", mcpTool, nil, "pg", 0, nil) - if bt2.Name() != "pg__query" { - t.Errorf("expected name=pg__query, got %s", bt2.Name()) + if bt2.Name() != "mcp_pg__query" { + t.Errorf("expected name=mcp_pg__query, got %s", bt2.Name()) } if bt2.OriginalName() != "query" { t.Errorf("expected originalName=query, got %s", bt2.OriginalName()) } + // With mcp_ prefix → unchanged + bt3 := NewBridgeTool("myserver", mcpTool, nil, "mcp_pg", 0, nil) + if bt3.Name() != "mcp_pg__query" { + t.Errorf("expected name=mcp_pg__query, got %s", bt3.Name()) + } + + // Server name with hyphens → sanitized to underscores + bt4 := NewBridgeTool("my-server", mcpTool, nil, "", 0, nil) + if bt4.Name() != "mcp_my_server__query" { + t.Errorf("expected name=mcp_my_server__query, got %s", bt4.Name()) + } + // Default timeout if bt2.timeoutSec != 60 { t.Errorf("expected default timeout=60, got %d", bt2.timeoutSec) } } + +func TestEnsureMCPPrefix(t *testing.T) { + tests := []struct { + name string + prefix string + serverName string + want string + }{ + {"empty prefix", "", "vnstock", "mcp_vnstock"}, + {"empty prefix hyphenated server", "", "my-server", "mcp_my_server"}, + {"non-mcp prefix", "pg", "postgres", "mcp_pg"}, + {"already mcp_ prefix", "mcp_pg", "postgres", "mcp_pg"}, + {"mcp prefix without underscore", "mcp", "x", "mcp_mcp"}, + {"custom prefix with underscores", "vnstock", "vnstock", "mcp_vnstock"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ensureMCPPrefix(tt.prefix, tt.serverName) + if got != tt.want { + t.Errorf("ensureMCPPrefix(%q, %q) = %q, want %q", tt.prefix, tt.serverName, got, tt.want) + } + }) + } +} diff --git a/internal/mcp/manager_connect.go b/internal/mcp/manager_connect.go index 969a6602..b7606c89 100644 --- a/internal/mcp/manager_connect.go +++ b/internal/mcp/manager_connect.go @@ -80,21 +80,27 @@ func (m *Manager) connectServer(ctx context.Context, name, transportType, comman } ss.toolNames = registeredNames + // Create health monitoring context BEFORE storing server in map, + // so DisconnectServer() can safely call ss.cancel even if called immediately. + hctx, hcancel := context.WithCancel(context.Background()) + ss.cancel = hcancel + + // Store server state BEFORE updating MCP group, because updateMCPGroup() + // calls ToolNames() which iterates m.servers. If we store after, the current + // server's tools are invisible to ToolNames() and toolGroups["mcp"] ends up + // incomplete — causing the policy engine to block MCP tools. + m.mu.Lock() + m.servers[name] = ss + m.mu.Unlock() + // Register dynamic tool groups for policy filtering if len(registeredNames) > 0 { tools.RegisterToolGroup("mcp:"+name, registeredNames) m.updateMCPGroup() } - // Start health monitoring - hctx, hcancel := context.WithCancel(context.Background()) - ss.cancel = hcancel go m.healthLoop(hctx, ss) - m.mu.Lock() - m.servers[name] = ss - m.mu.Unlock() - slog.Info("mcp.server.connected", "server", name, "transport", transportType, diff --git a/internal/skills/loader.go b/internal/skills/loader.go index 0d5a0f07..4fa19c6c 100644 --- a/internal/skills/loader.go +++ b/internal/skills/loader.go @@ -15,6 +15,8 @@ import ( "os" "path/filepath" "regexp" + "sort" + "strconv" "strings" "sync" "sync/atomic" @@ -41,11 +43,15 @@ type Info struct { type Loader struct { // Skill directories in priority order (highest first). // Matches TS loadSkillEntries() 5-tier hierarchy. - workspaceSkills string // /skills/ - projectAgentSkills string // /.agents/skills/ - personalAgentSkills string // ~/.agents/skills/ - globalSkills string // ~/.goclaw/skills/ - builtinSkills string // bundled with binary + workspaceSkills string // /skills/ + projectAgentSkills string // /.agents/skills/ + personalAgentSkills string // ~/.agents/skills/ + globalSkills string // ~/.goclaw/skills/ + builtinSkills string // bundled with binary + + // Managed skills directory (set via SetManagedDir in managed mode). + // Uses versioned subdirectory structure: ///SKILL.md + managedSkillsDir string mu sync.RWMutex cache map[string]*Info // name → info (lazily populated) @@ -84,6 +90,14 @@ func NewLoader(workspace, globalSkills, builtinSkills string) *Loader { } } +// SetManagedDir sets the managed skills directory (skills-store). +// Managed skills use versioned subdirectories: ///SKILL.md. +// Called in managed mode after PG stores are created. +func (l *Loader) SetManagedDir(dir string) { + l.managedSkillsDir = dir + l.BumpVersion() // trigger re-scan +} + // ListSkills returns all available skills, respecting the priority hierarchy. // Higher-priority sources override lower ones by name. func (l *Loader) ListSkills() []Info { @@ -139,12 +153,100 @@ func (l *Loader) ListSkills() []Info { } } + // Managed skills: versioned subdirectories ///SKILL.md + // Only include skills not already seen from higher-priority sources. + if l.managedSkillsDir != "" { + for _, info := range l.listManagedSkills() { + if seen[info.Slug] { + continue + } + skills = append(skills, info) + seen[info.Slug] = true + l.cache[info.Slug] = &info + } + } + return skills } +// listManagedSkills scans the managed skills directory for versioned skill directories. +// Structure: ///SKILL.md +// Returns the latest version of each skill found. +func (l *Loader) listManagedSkills() []Info { + dirs, err := os.ReadDir(l.managedSkillsDir) + if err != nil { + return nil + } + + var skills []Info + for _, d := range dirs { + if !d.IsDir() { + continue + } + slug := d.Name() + + // Find the latest version subdirectory + latestVersion, latestDir := l.findLatestVersion(slug) + if latestVersion < 0 { + continue + } + + skillFile := filepath.Join(latestDir, "SKILL.md") + if _, err := os.Stat(skillFile); err != nil { + continue + } + + info := Info{ + Name: slug, + Slug: slug, + Path: skillFile, + BaseDir: latestDir, + Source: "managed", + } + if meta := parseMetadata(skillFile); meta != nil { + info.Description = meta.Description + if meta.Name != "" { + info.Name = meta.Name + } + } + skills = append(skills, info) + } + return skills +} + +// findLatestVersion finds the highest-numbered version subdirectory for a skill slug. +// Returns (version, path) or (-1, "") if no valid version found. +func (l *Loader) findLatestVersion(slug string) (int, string) { + slugDir := filepath.Join(l.managedSkillsDir, slug) + entries, err := os.ReadDir(slugDir) + if err != nil { + return -1, "" + } + + var versions []int + for _, e := range entries { + if !e.IsDir() { + continue + } + v, err := strconv.Atoi(e.Name()) + if err != nil || v < 1 { + continue + } + versions = append(versions, v) + } + if len(versions) == 0 { + return -1, "" + } + + sort.Sort(sort.Reverse(sort.IntSlice(versions))) + latestVer := versions[0] + return latestVer, filepath.Join(slugDir, strconv.Itoa(latestVer)) +} + // LoadSkill reads and returns the content of a skill by name (frontmatter stripped). // The {baseDir} placeholder in SKILL.md is replaced with the skill's absolute directory path. func (l *Loader) LoadSkill(name string) (string, bool) { + // Check standard (flat) skill directories first for _, dir := range []string{l.workspaceSkills, l.projectAgentSkills, l.personalAgentSkills, l.globalSkills, l.builtinSkills} { if dir == "" { continue @@ -159,6 +261,21 @@ func (l *Loader) LoadSkill(name string) (string, bool) { content = strings.ReplaceAll(content, "{baseDir}", baseDir) return content, true } + + // Check managed skills directory (versioned structure) + if l.managedSkillsDir != "" { + latestVer, latestDir := l.findLatestVersion(name) + if latestVer >= 0 { + path := filepath.Join(latestDir, "SKILL.md") + data, err := os.ReadFile(path) + if err == nil { + content := stripFrontmatter(string(data)) + content = strings.ReplaceAll(content, "{baseDir}", latestDir) + return content, true + } + } + } + return "", false } @@ -253,7 +370,7 @@ func (l *Loader) BumpVersion() { // Dirs returns all non-empty skill directories (for the watcher to monitor). func (l *Loader) Dirs() []string { var dirs []string - for _, d := range []string{l.workspaceSkills, l.projectAgentSkills, l.personalAgentSkills, l.globalSkills, l.builtinSkills} { + for _, d := range []string{l.workspaceSkills, l.projectAgentSkills, l.personalAgentSkills, l.globalSkills, l.builtinSkills, l.managedSkillsDir} { if d != "" { dirs = append(dirs, d) } diff --git a/internal/skills/search.go b/internal/skills/search.go index e1ef97a9..8aab2ff6 100644 --- a/internal/skills/search.go +++ b/internal/skills/search.go @@ -10,10 +10,11 @@ import ( // SkillSearchResult is a single result from a skill search. type SkillSearchResult struct { Name string `json:"name"` + Slug string `json:"slug"` // directory name (unique identifier, used for access filtering) Description string `json:"description"` Location string `json:"location"` // absolute path to SKILL.md BaseDir string `json:"baseDir"` // skill directory (for {baseDir} references) - Source string `json:"source"` // "workspace", "global", "builtin" + Source string `json:"source"` // "workspace", "global", "builtin", "managed" Score float64 `json:"score"` } @@ -151,6 +152,7 @@ func (idx *Index) Search(query string, maxResults int) []SkillSearchResult { for i, r := range results { out[i] = SkillSearchResult{ Name: r.doc.info.Name, + Slug: r.doc.info.Slug, Description: r.doc.info.Description, Location: r.doc.info.Path, BaseDir: r.doc.info.BaseDir, diff --git a/internal/store/mcp_store.go b/internal/store/mcp_store.go index 8cd1f6c0..6167de8f 100644 --- a/internal/store/mcp_store.go +++ b/internal/store/mcp_store.go @@ -11,41 +11,41 @@ import ( // MCPServerData represents an MCP server in the database. type MCPServerData struct { BaseModel - Name string `json:"name"` - DisplayName string `json:"display_name,omitempty"` - Transport string `json:"transport"` // "stdio", "sse", "streamable-http" - Command string `json:"command,omitempty"` // stdio + Name string `json:"name"` + DisplayName string `json:"display_name,omitempty"` + Transport string `json:"transport"` // "stdio", "sse", "streamable-http" + Command string `json:"command,omitempty"` // stdio Args json.RawMessage `json:"args,omitempty"` // JSONB - URL string `json:"url,omitempty"` // sse/http - Headers json.RawMessage `json:"headers,omitempty"` // JSONB + URL string `json:"url,omitempty"` // sse/http + Headers json.RawMessage `json:"headers,omitempty"` // JSONB Env json.RawMessage `json:"env,omitempty"` // JSONB (stdio) - APIKey string `json:"api_key,omitempty"` // encrypted - ToolPrefix string `json:"tool_prefix,omitempty"` - TimeoutSec int `json:"timeout_sec"` + APIKey string `json:"api_key,omitempty"` // encrypted + ToolPrefix string `json:"tool_prefix,omitempty"` + TimeoutSec int `json:"timeout_sec"` Settings json.RawMessage `json:"settings,omitempty"` // JSONB - Enabled bool `json:"enabled"` - CreatedBy string `json:"created_by"` + Enabled bool `json:"enabled"` + CreatedBy string `json:"created_by"` } // MCPAgentGrant represents an MCP server grant to an agent. type MCPAgentGrant struct { - ID uuid.UUID `json:"id"` - ServerID uuid.UUID `json:"server_id"` - AgentID uuid.UUID `json:"agent_id"` - Enabled bool `json:"enabled"` + ID uuid.UUID `json:"id"` + ServerID uuid.UUID `json:"server_id"` + AgentID uuid.UUID `json:"agent_id"` + Enabled bool `json:"enabled"` ToolAllow json.RawMessage `json:"tool_allow,omitempty"` // JSONB ToolDeny json.RawMessage `json:"tool_deny,omitempty"` // JSONB ConfigOverrides json.RawMessage `json:"config_overrides,omitempty"` // JSONB - GrantedBy string `json:"granted_by"` - CreatedAt time.Time `json:"created_at"` + GrantedBy string `json:"granted_by"` + CreatedAt time.Time `json:"created_at"` } // MCPUserGrant represents an MCP server grant to a user. type MCPUserGrant struct { - ID uuid.UUID `json:"id"` - ServerID uuid.UUID `json:"server_id"` - UserID string `json:"user_id"` - Enabled bool `json:"enabled"` + ID uuid.UUID `json:"id"` + ServerID uuid.UUID `json:"server_id"` + UserID string `json:"user_id"` + Enabled bool `json:"enabled"` ToolAllow json.RawMessage `json:"tool_allow,omitempty"` // JSONB ToolDeny json.RawMessage `json:"tool_deny,omitempty"` // JSONB GrantedBy string `json:"granted_by"` @@ -54,19 +54,19 @@ type MCPUserGrant struct { // MCPAccessRequest represents a request for MCP server access. type MCPAccessRequest struct { - ID uuid.UUID `json:"id"` - ServerID uuid.UUID `json:"server_id"` - AgentID *uuid.UUID `json:"agent_id,omitempty"` - UserID string `json:"user_id,omitempty"` - Scope string `json:"scope"` // "agent" or "user" - Status string `json:"status"` // "pending", "approved", "rejected" - Reason string `json:"reason,omitempty"` + ID uuid.UUID `json:"id"` + ServerID uuid.UUID `json:"server_id"` + AgentID *uuid.UUID `json:"agent_id,omitempty"` + UserID string `json:"user_id,omitempty"` + Scope string `json:"scope"` // "agent" or "user" + Status string `json:"status"` // "pending", "approved", "rejected" + Reason string `json:"reason,omitempty"` ToolAllow json.RawMessage `json:"tool_allow,omitempty"` // JSONB - RequestedBy string `json:"requested_by"` - ReviewedBy string `json:"reviewed_by,omitempty"` - ReviewedAt *time.Time `json:"reviewed_at,omitempty"` - ReviewNote string `json:"review_note,omitempty"` - CreatedAt time.Time `json:"created_at"` + RequestedBy string `json:"requested_by"` + ReviewedBy string `json:"reviewed_by,omitempty"` + ReviewedAt *time.Time `json:"reviewed_at,omitempty"` + ReviewNote string `json:"review_note,omitempty"` + CreatedAt time.Time `json:"created_at"` } // MCPAccessInfo combines server data with grant-level tool filters for runtime resolution. @@ -90,6 +90,7 @@ type MCPServerStore interface { GrantToAgent(ctx context.Context, g *MCPAgentGrant) error RevokeFromAgent(ctx context.Context, serverID, agentID uuid.UUID) error ListAgentGrants(ctx context.Context, agentID uuid.UUID) ([]MCPAgentGrant, error) + ListServerGrants(ctx context.Context, serverID uuid.UUID) ([]MCPAgentGrant, error) // User grants GrantToUser(ctx context.Context, g *MCPUserGrant) error diff --git a/internal/store/pg/mcp_servers_access.go b/internal/store/pg/mcp_servers_access.go index 75bce614..2afa8de8 100644 --- a/internal/store/pg/mcp_servers_access.go +++ b/internal/store/pg/mcp_servers_access.go @@ -63,6 +63,29 @@ func (s *PGMCPServerStore) ListAgentGrants(ctx context.Context, agentID uuid.UUI return result, nil } +func (s *PGMCPServerStore) ListServerGrants(ctx context.Context, serverID uuid.UUID) ([]store.MCPAgentGrant, error) { + rows, err := s.db.QueryContext(ctx, + `SELECT id, server_id, agent_id, enabled, + COALESCE(tool_allow, '[]'::jsonb), COALESCE(tool_deny, '[]'::jsonb), + COALESCE(config_overrides, '{}'::jsonb), granted_by, created_at + FROM mcp_agent_grants WHERE server_id = $1 ORDER BY created_at`, serverID) + if err != nil { + return nil, err + } + defer rows.Close() + + result := make([]store.MCPAgentGrant, 0) + for rows.Next() { + var g store.MCPAgentGrant + if err := rows.Scan(&g.ID, &g.ServerID, &g.AgentID, &g.Enabled, + &g.ToolAllow, &g.ToolDeny, &g.ConfigOverrides, &g.GrantedBy, &g.CreatedAt); err != nil { + continue + } + result = append(result, g) + } + return result, nil +} + // --- User Grants --- func (s *PGMCPServerStore) GrantToUser(ctx context.Context, g *store.MCPUserGrant) error { diff --git a/internal/store/pg/skills.go b/internal/store/pg/skills.go index 629e8d27..f54dbfb3 100644 --- a/internal/store/pg/skills.go +++ b/internal/store/pg/skills.go @@ -3,6 +3,7 @@ package pg import ( "context" "database/sql" + "encoding/json" "fmt" "log/slog" "os" @@ -60,7 +61,7 @@ func (s *PGSkillStore) ListSkills() []store.SkillInfo { // Cache miss or TTL expired → query DB rows, err := s.db.Query( - `SELECT name, slug, description, version FROM skills WHERE status = 'active' ORDER BY name`) + `SELECT id, name, slug, description, version FROM skills WHERE status = 'active' ORDER BY name`) if err != nil { return nil } @@ -68,13 +69,14 @@ func (s *PGSkillStore) ListSkills() []store.SkillInfo { var result []store.SkillInfo for rows.Next() { + var id uuid.UUID var name, slug string var desc *string var version int - if err := rows.Scan(&name, &slug, &desc, &version); err != nil { + if err := rows.Scan(&id, &name, &slug, &desc, &version); err != nil { continue } - result = append(result, buildSkillInfo(name, slug, desc, version, s.baseDir)) + result = append(result, buildSkillInfo(id.String(), name, slug, desc, version, s.baseDir)) } s.mu.Lock() @@ -155,7 +157,7 @@ func (s *PGSkillStore) GetSkill(name string) (*store.SkillInfo, bool) { if err != nil { return nil, false } - info := buildSkillInfo(skillName, slug, desc, version, s.baseDir) + info := buildSkillInfo("", skillName, slug, desc, version, s.baseDir) return &info, true } @@ -180,9 +182,9 @@ func (s *PGSkillStore) FilterSkills(allowList []string) []store.SkillInfo { return filtered } -func (s *PGSkillStore) Version() int64 { return s.version.Load() } -func (s *PGSkillStore) BumpVersion() { s.version.Store(time.Now().UnixMilli()) } -func (s *PGSkillStore) Dirs() []string { return []string{s.baseDir} } +func (s *PGSkillStore) Version() int64 { return s.version.Load() } +func (s *PGSkillStore) BumpVersion() { s.version.Store(time.Now().UnixMilli()) } +func (s *PGSkillStore) Dirs() []string { return []string{s.baseDir} } // --- CRUD for managed skill upload --- @@ -208,10 +210,30 @@ func (s *PGSkillStore) UpdateSkill(id uuid.UUID, updates map[string]interface{}) } func (s *PGSkillStore) DeleteSkill(id uuid.UUID) error { - _, err := s.db.Exec("UPDATE skills SET status = 'archived' WHERE id = $1", id) + tx, err := s.db.Begin() if err != nil { return err } + defer tx.Rollback() + + // Cascade: remove all agent grants for this skill + if _, err := tx.Exec("DELETE FROM skill_agent_grants WHERE skill_id = $1", id); err != nil { + return fmt.Errorf("delete skill grants: %w", err) + } + + // Cascade: remove all user grants for this skill + if _, err := tx.Exec("DELETE FROM skill_user_grants WHERE skill_id = $1", id); err != nil { + return fmt.Errorf("delete skill user grants: %w", err) + } + + // Soft-delete the skill itself + if _, err := tx.Exec("UPDATE skills SET status = 'archived' WHERE id = $1", id); err != nil { + return fmt.Errorf("archive skill: %w", err) + } + + if err := tx.Commit(); err != nil { + return err + } s.BumpVersion() return nil } @@ -227,6 +249,7 @@ type SkillCreateParams struct { FilePath string FileSize int64 FileHash *string + Frontmatter map[string]string // parsed YAML frontmatter from SKILL.md } // CreateSkillManaged creates a skill from upload parameters. @@ -235,15 +258,25 @@ func (s *PGSkillStore) CreateSkillManaged(ctx context.Context, p SkillCreatePara return uuid.Nil, err } id := store.GenNewID() + // Marshal frontmatter to JSON for DB storage + fmJSON := []byte("{}") + if len(p.Frontmatter) > 0 { + if b, err := json.Marshal(p.Frontmatter); err == nil { + fmJSON = b + } + } _, err := s.db.ExecContext(ctx, - `INSERT INTO skills (id, name, slug, description, owner_id, visibility, version, status, file_path, file_size, file_hash, created_at, updated_at) - VALUES ($1, $2, $3, $4, $5, $6, $7, 'active', $8, $9, $10, NOW(), NOW()) + `INSERT INTO skills (id, name, slug, description, owner_id, visibility, version, status, frontmatter, file_path, file_size, file_hash, created_at, updated_at) + VALUES ($1, $2, $3, $4, $5, $6, $7, 'active', $8, $9, $10, $11, NOW(), NOW()) ON CONFLICT (slug) DO UPDATE SET - version = EXCLUDED.version, file_path = EXCLUDED.file_path, + name = EXCLUDED.name, description = EXCLUDED.description, + version = EXCLUDED.version, frontmatter = EXCLUDED.frontmatter, + file_path = EXCLUDED.file_path, file_size = EXCLUDED.file_size, file_hash = EXCLUDED.file_hash, - updated_at = NOW()`, + visibility = CASE WHEN skills.status = 'archived' THEN 'private' ELSE skills.visibility END, + status = 'active', updated_at = NOW()`, id, p.Name, p.Slug, p.Description, p.OwnerID, p.Visibility, p.Version, - p.FilePath, p.FileSize, p.FileHash, + fmJSON, p.FilePath, p.FileSize, p.FileHash, ) if err == nil { s.BumpVersion() @@ -283,6 +316,7 @@ func (s *PGSkillStore) SearchByEmbedding(ctx context.Context, embedding []float3 1 - (embedding <=> $1::vector) AS score FROM skills WHERE status = 'active' AND embedding IS NOT NULL + AND visibility != 'private' ORDER BY embedding <=> $2::vector LIMIT $3`, vecStr, vecStr, limit, @@ -392,12 +426,13 @@ func (s *PGSkillStore) generateEmbedding(ctx context.Context, slug, name, descri // --- Helpers --- -func buildSkillInfo(name, slug string, desc *string, version int, baseDir string) store.SkillInfo { +func buildSkillInfo(id, name, slug string, desc *string, version int, baseDir string) store.SkillInfo { d := "" if desc != nil { d = *desc } return store.SkillInfo{ + ID: id, Name: name, Slug: slug, Path: fmt.Sprintf("%s/%s/%d/SKILL.md", baseDir, slug, version), diff --git a/internal/store/pg/skills_grants.go b/internal/store/pg/skills_grants.go index e1b5ddcc..b780fe04 100644 --- a/internal/store/pg/skills_grants.go +++ b/internal/store/pg/skills_grants.go @@ -11,6 +11,8 @@ import ( ) // GrantToAgent grants a skill to an agent with version pinning. +// Auto-promotes visibility from 'private' to 'internal' so the skill +// becomes accessible via ListAccessible for granted agents. func (s *PGSkillStore) GrantToAgent(ctx context.Context, skillID, agentID uuid.UUID, version int, grantedBy string) error { if err := store.ValidateUserID(grantedBy); err != nil { return err @@ -21,14 +23,46 @@ func (s *PGSkillStore) GrantToAgent(ctx context.Context, skillID, agentID uuid.U ON CONFLICT (skill_id, agent_id) DO UPDATE SET pinned_version = EXCLUDED.pinned_version`, store.GenNewID(), skillID, agentID, version, grantedBy, time.Now(), ) - return err + if err != nil { + return err + } + + // Auto-promote: private → internal (so ListAccessible query includes it for granted agents) + _, err = s.db.ExecContext(ctx, + `UPDATE skills SET visibility = 'internal', updated_at = NOW() WHERE id = $1 AND visibility = 'private'`, + skillID) + if err != nil { + slog.Warn("skill_grants: failed to auto-promote visibility", "skill_id", skillID, "error", err) + // Non-fatal: grant was already created successfully + } + + s.BumpVersion() + return nil } // RevokeFromAgent revokes a skill grant from an agent. +// Auto-demotes visibility from 'internal' back to 'private' when no agent grants remain. func (s *PGSkillStore) RevokeFromAgent(ctx context.Context, skillID, agentID uuid.UUID) error { _, err := s.db.ExecContext(ctx, "DELETE FROM skill_agent_grants WHERE skill_id = $1 AND agent_id = $2", skillID, agentID) - return err + if err != nil { + return err + } + + // Atomic auto-demote: set internal → private only if zero remaining grants. + // Uses NOT EXISTS subquery so the check + update is a single atomic SQL statement, + // avoiding a race window between COUNT and UPDATE. + _, err = s.db.ExecContext(ctx, + `UPDATE skills SET visibility = 'private', updated_at = NOW() + WHERE id = $1 AND visibility = 'internal' + AND NOT EXISTS (SELECT 1 FROM skill_agent_grants WHERE skill_id = $1)`, + skillID) + if err != nil { + slog.Warn("skill_grants: failed to auto-demote visibility", "skill_id", skillID, "error", err) + } + + s.BumpVersion() + return nil } // ListAgentGrants returns all skill grants for an agent. @@ -103,7 +137,7 @@ func (s *PGSkillStore) ListAccessible(ctx context.Context, agentID uuid.UUID, us slog.Warn("skill_grants: scan error in ListAccessible", "error", err) continue } - result = append(result, buildSkillInfo(name, slug, desc, version, s.baseDir)) + result = append(result, buildSkillInfo("", name, slug, desc, version, s.baseDir)) } return result, rows.Err() } diff --git a/internal/store/skill_store.go b/internal/store/skill_store.go index a05ed7e6..03dd6b86 100644 --- a/internal/store/skill_store.go +++ b/internal/store/skill_store.go @@ -8,6 +8,7 @@ import ( // SkillInfo describes a discovered skill. type SkillInfo struct { + ID string `json:"id,omitempty"` // DB UUID (managed mode only) Name string `json:"name"` Slug string `json:"slug"` Path string `json:"path"` diff --git a/internal/tools/policy.go b/internal/tools/policy.go index 5efa3325..3935a5ac 100644 --- a/internal/tools/policy.go +++ b/internal/tools/policy.go @@ -60,10 +60,10 @@ var toolProfiles = map[string][]string{ // Tool aliases map alternative names to canonical names. var toolAliases = map[string]string{ - "bash": "exec", - "apply-patch": "apply_patch", - "edit_file": "edit", - "sessions_spawn": "spawn", + "bash": "exec", + "apply-patch": "apply_patch", + "edit_file": "edit", + "sessions_spawn": "spawn", } // Subagent deny lists — tools subagents cannot use. diff --git a/internal/tools/policy_mcp_test.go b/internal/tools/policy_mcp_test.go index 2fa5bcab..8e710f31 100644 --- a/internal/tools/policy_mcp_test.go +++ b/internal/tools/policy_mcp_test.go @@ -4,7 +4,7 @@ import "testing" func TestRegisterToolGroup(t *testing.T) { // Register a new MCP group - RegisterToolGroup("mcp:postgres", []string{"pg__query", "pg__list_tables"}) + RegisterToolGroup("mcp:postgres", []string{"mcp_pg__query", "mcp_pg__list_tables"}) members, ok := toolGroups["mcp:postgres"] if !ok { @@ -22,10 +22,10 @@ func TestRegisterToolGroup(t *testing.T) { } func TestRegisterToolGroup_UsedInExpand(t *testing.T) { - RegisterToolGroup("mcp:test", []string{"test__tool_a", "test__tool_b"}) + RegisterToolGroup("mcp:test", []string{"mcp_test__tool_a", "mcp_test__tool_b"}) defer UnregisterToolGroup("mcp:test") - available := []string{"test__tool_a", "test__tool_b", "read_file", "exec"} + available := []string{"mcp_test__tool_a", "mcp_test__tool_b", "read_file", "exec"} expanded := expandSpec(available, []string{"group:mcp:test"}) if len(expanded) != 2 { diff --git a/internal/tools/skill_search.go b/internal/tools/skill_search.go index 839dbdd3..e92d4b67 100644 --- a/internal/tools/skill_search.go +++ b/internal/tools/skill_search.go @@ -166,8 +166,10 @@ func (t *SkillSearchTool) filterByAccess(ctx context.Context, results []skills.S for _, r := range results { if r.Source != "managed" { filtered = append(filtered, r) - } else if _, ok := allowed[r.Name]; ok { + } else if _, ok := allowed[r.Slug]; ok { filtered = append(filtered, r) + } else { + slog.Debug("skill_search: filtered out inaccessible managed skill", "slug", r.Slug, "name", r.Name) } } return filtered @@ -238,6 +240,7 @@ func (t *SkillSearchTool) hybridSearch(ctx context.Context, query string, bm25Re seen[r.Name] = &merged{ result: skills.SkillSearchResult{ Name: r.Name, + Slug: r.Slug, Description: r.Description, Location: r.Path, Source: "managed", diff --git a/ui/web/src/pages/mcp/hooks/use-mcp.ts b/ui/web/src/pages/mcp/hooks/use-mcp.ts index f566f017..5d5482df 100644 --- a/ui/web/src/pages/mcp/hooks/use-mcp.ts +++ b/ui/web/src/pages/mcp/hooks/use-mcp.ts @@ -49,11 +49,11 @@ export function useMCP() { ); const listAgentGrants = useCallback( - async (_serverId: string) => { - // Backend provides per-agent listing, not per-server. - return [] as MCPAgentGrant[]; + async (serverId: string) => { + const res = await http.get<{ grants: MCPAgentGrant[] }>(`/v1/mcp/servers/${serverId}/grants`); + return res.grants ?? []; }, - [], + [http], ); const grantAgent = useCallback( diff --git a/ui/web/src/pages/mcp/mcp-form-dialog.tsx b/ui/web/src/pages/mcp/mcp-form-dialog.tsx index e4202017..4752a6c2 100644 --- a/ui/web/src/pages/mcp/mcp-form-dialog.tsx +++ b/ui/web/src/pages/mcp/mcp-form-dialog.tsx @@ -49,7 +49,7 @@ export function MCPFormDialog({ open, onOpenChange, server, onSubmit }: MCPFormD setArgs(Array.isArray(server?.args) ? server.args.join(", ") : ""); setUrl(server?.url ?? ""); setHeaders(server?.headers ? JSON.stringify(server.headers, null, 2) : ""); - setToolPrefix(server?.tool_prefix ?? ""); + setToolPrefix((server?.tool_prefix ?? "").replace(/^mcp_/, "")); setTimeout(server?.timeout_sec ?? 60); setEnabled(server?.enabled ?? true); setError(""); @@ -173,15 +173,18 @@ export function MCPFormDialog({ open, onOpenChange, server, onSubmit }: MCPFormD )} -
-
- - setToolPrefix(e.target.value)} placeholder="mcp_" /> -
-
- - setTimeout(Number(e.target.value))} min={1} /> +
+ +
+ mcp_ + setToolPrefix(e.target.value.replace(/[^a-z0-9_]/g, ""))} placeholder={name.replace(/-/g, "_") || "auto"} className="rounded-l-none font-mono text-sm" />
+

Auto-derived from name if empty. Tools: mcp_{prefix}__{tool}

+
+ +
+ + setTimeout(Number(e.target.value))} min={1} />
diff --git a/ui/web/src/pages/mcp/mcp-grants-dialog.tsx b/ui/web/src/pages/mcp/mcp-grants-dialog.tsx index 2618a694..b5bc803d 100644 --- a/ui/web/src/pages/mcp/mcp-grants-dialog.tsx +++ b/ui/web/src/pages/mcp/mcp-grants-dialog.tsx @@ -18,7 +18,7 @@ interface MCPGrantsDialogProps { server: MCPServerData; onGrant: (agentId: string, toolAllow?: string[], toolDeny?: string[]) => Promise; onRevoke: (agentId: string) => Promise; - onLoadGrants: (agentId: string) => Promise; + onLoadGrants: (serverId: string) => Promise; } export function MCPGrantsDialog({ @@ -27,6 +27,7 @@ export function MCPGrantsDialog({ server, onGrant, onRevoke, + onLoadGrants, }: MCPGrantsDialogProps) { const [agentId, setAgentId] = useState(""); const [toolAllow, setToolAllow] = useState(""); @@ -40,35 +41,56 @@ export function MCPGrantsDialog({ setAgentId(""); setToolAllow(""); setToolDeny(""); - setGrants([]); setError(""); + // Load existing grants from API + setLoading(true); + onLoadGrants(server.id) + .then((existing) => setGrants(existing)) + .catch(() => setGrants([])) + .finally(() => setLoading(false)); } - }, [open]); + }, [open, server.id, onLoadGrants]); const handleGrant = async () => { if (!agentId.trim()) { setError("Agent ID is required"); return; } + + const trimmedId = agentId.trim(); + const existing = grants.find((g) => g.agent_id === trimmedId); + setLoading(true); setError(""); try { const allow = toolAllow.trim() ? toolAllow.split(",").map((s) => s.trim()).filter(Boolean) : undefined; const deny = toolDeny.trim() ? toolDeny.split(",").map((s) => s.trim()).filter(Boolean) : undefined; - await onGrant(agentId.trim(), allow, deny); - setGrants((prev) => [ - ...prev, - { - id: crypto.randomUUID(), - server_id: server.id, - agent_id: agentId.trim(), - enabled: true, - tool_allow: allow ?? null, - tool_deny: deny ?? null, - granted_by: "", - created_at: new Date().toISOString(), - }, - ]); + await onGrant(trimmedId, allow, deny); + + if (existing) { + // Upsert: update existing grant in list + setGrants((prev) => + prev.map((g) => + g.agent_id === trimmedId + ? { ...g, tool_allow: allow ?? null, tool_deny: deny ?? null } + : g + ) + ); + } else { + setGrants((prev) => [ + ...prev, + { + id: crypto.randomUUID(), + server_id: server.id, + agent_id: trimmedId, + enabled: true, + tool_allow: allow ?? null, + tool_deny: deny ?? null, + granted_by: "", + created_at: new Date().toISOString(), + }, + ]); + } setAgentId(""); setToolAllow(""); setToolDeny(""); diff --git a/ui/web/src/pages/mcp/mcp-page.tsx b/ui/web/src/pages/mcp/mcp-page.tsx index a46638c7..c2e9a494 100644 --- a/ui/web/src/pages/mcp/mcp-page.tsx +++ b/ui/web/src/pages/mcp/mcp-page.tsx @@ -22,7 +22,7 @@ const transportBadge: Record = { }; export function MCPPage() { - const { servers, loading, refresh, createServer, updateServer, deleteServer, grantAgent, revokeAgent, listGrantsByAgent } = useMCP(); + const { servers, loading, refresh, createServer, updateServer, deleteServer, grantAgent, revokeAgent, listAgentGrants } = useMCP(); const spinning = useMinLoading(loading); const showSkeleton = useDeferredLoading(loading && servers.length === 0); const [search, setSearch] = useState(""); @@ -197,7 +197,7 @@ export function MCPPage() { server={grantsServer} onGrant={(agentId, allow, deny) => grantAgent(grantsServer.id, agentId, allow, deny)} onRevoke={(agentId) => revokeAgent(grantsServer.id, agentId)} - onLoadGrants={listGrantsByAgent} + onLoadGrants={listAgentGrants} /> )}