diff --git a/cmd/gateway.go b/cmd/gateway.go index 9a6ae41f..3e103179 100644 --- a/cmd/gateway.go +++ b/cmd/gateway.go @@ -262,7 +262,9 @@ func runGateway() { tools.DetectServerIPs(context.Background()) } + slog.Debug("creating mcpMgr via setupToolRegistry") toolsReg, execApprovalMgr, mcpMgr, sandboxMgr, browserMgr, webFetchTool, ttsTool, audioMgr, permPE, toolPE, dataDir, agentCfg := setupToolRegistry(cfg, workspace, providerRegistry) + slog.Debug("setupToolRegistry completed", "mcpMgr_nil", mcpMgr == nil) if browserMgr != nil { defer browserMgr.Close() } @@ -348,13 +350,25 @@ func runGateway() { } // MCP servers: load from database (single source of truth). // pgStores.MCP is nil on SQLite/desktop builds that don't support MCP tables. + // Apply store to MCP manager so ListToolsForAgent can query DB. + // mcpMgr is created before pgStores is available, so the store must be set here. + if pgStores.MCP != nil && mcpMgr != nil { + mcpMgr.SetStore(pgStores.MCP) + slog.Info("applied store to MCPManager") + } + slog.Debug("checking MCP store availability", "pgStores_MCP_nil", pgStores == nil || pgStores.MCP == nil, "mcpMgr_nil", mcpMgr == nil) if pgStores.MCP != nil { + slog.Debug("initializing MCP from database") if err := initMCPFromDB(context.Background(), mcpMgr, pgStores.MCP); err != nil { slog.Warn("mcp.db_load_errors", "error", err) + } else { + slog.Debug("initMCPFromDB completed successfully") } if mcpMgr != nil { - slog.Info("MCP servers loaded from database", "tools", len(mcpMgr.ToolNames())) + slog.Info("MCP manager started", "tools", len(mcpMgr.ToolNames())) } + } else { + slog.Debug("skipping MCP database init: pgStores.MCP is nil") } setupMemoryEmbeddings(pgStores, providerRegistry) @@ -471,6 +485,7 @@ func runGateway() { server.SetVersion(Version) server.SetDB(pgStores.DB) server.SetPolicyEngine(permPE) + server.SetToolPolicy(toolPE) server.SetPairingService(pgStores.Pairing) server.SetMessageBus(msgBus) server.SetOAuthHandler(httpapi.NewOAuthHandler(pgStores.Providers, pgStores.ConfigSecrets, providerRegistry, msgBus)) @@ -533,13 +548,16 @@ func runGateway() { // Wire dependencies for system prompt preview parity. if agentsH != nil { agentsH.SetPreviewDeps(toolsReg, skillsLoader) + agentsH.SetPreviewToolPolicy(toolPE) var skillAccess store.SkillAccessStore if pgStores.Skills != nil { skillAccess, _ = pgStores.Skills.(store.SkillAccessStore) } agentsH.SetPreviewStores(pgStores.Teams, pgStores.AgentLinks, skillAccess) + slog.Debug("wiring MCP preview manager", "mcpMgr_nil", mcpMgr == nil) if mcpMgr != nil { agentsH.SetPreviewMCPManager(httpapi.NewMCPPreviewAdapter(mcpMgr)) + slog.Debug("set MCP preview manager on agentsH") } } diff --git a/cmd/gateway_setup.go b/cmd/gateway_setup.go index 5a074a87..153175f3 100644 --- a/cmd/gateway_setup.go +++ b/cmd/gateway_setup.go @@ -527,8 +527,11 @@ func setupSkillsSystem( if pgStores.Skills != nil { storeDirs := pgStores.Skills.Dirs() if len(storeDirs) > 0 { - skillsLoader.SetManagedDir(storeDirs[0]) - slog.Info("skills-store directory wired into loader", "dir", storeDirs[0]) + // Pass the root data dir, not storeDirs[0] (which is the master + // tenant's pre-resolved skills-store path) — the loader resolves + // each tenant's own skills-store directory per request from this root. + skillsLoader.SetManagedDir(dataDir) + slog.Info("skills-store directory wired into loader", "dataDir", dataDir) // Seed system/bundled skills into DB bundledSkillsDir = os.Getenv("GOCLAW_BUNDLED_SKILLS_DIR") @@ -607,15 +610,21 @@ func setupSkillsSystem( // into the shared manager. This replaces the former config-file-based initialisation. // Non-fatal: individual server connection failures are logged as warnings. func initMCPFromDB(ctx context.Context, mgr *mcpbridge.Manager, mcpStore store.MCPServerStore) error { + slog.Debug("initMCPFromDB starting") + slog.Debug("querying mcp_servers from database") servers, err := mcpStore.ListServers(ctx) if err != nil { + slog.Error("initMCPFromDB: failed to query mcp_servers", "error", err) return fmt.Errorf("list mcp servers from db: %w", err) } + slog.Debug("found mcp_servers from database", "count", len(servers)) cfgs := make(map[string]*config.MCPServerConfig, len(servers)) for i := range servers { srv := &servers[i] + slog.Debug("initMCPFromDB: processing server", "name", srv.Name, "transport", srv.Transport, "enabled", srv.Enabled) if !srv.Enabled { + slog.Debug("initMCPFromDB: skipping disabled server", "name", srv.Name) continue } @@ -663,13 +672,18 @@ func initMCPFromDB(ctx context.Context, mgr *mcpbridge.Manager, mcpStore store.M } if len(cfgs) == 0 { - slog.Info("mcp.db: no enabled servers found") + slog.Debug("mcp.db: no enabled servers found") return nil } + slog.Debug("initMCPFromDB: building config map", "servers", len(cfgs)) + slog.Debug("initMCPFromDB: calling mgr.SetConfigs()") mgr.SetConfigs(cfgs) + slog.Debug("initMCPFromDB: calling mgr.Start()") if startErr := mgr.Start(ctx); startErr != nil { slog.Warn("mcp.db.startup_errors", "error", startErr) } + toolCount := len(mgr.ToolNames()) + slog.Debug("initMCPFromDB: MCP init complete", "tools_registered", toolCount) return nil } diff --git a/internal/agent/loop_history_skills.go b/internal/agent/loop_history_skills.go index b2b99f39..012f0688 100644 --- a/internal/agent/loop_history_skills.go +++ b/internal/agent/loop_history_skills.go @@ -1,6 +1,8 @@ package agent -import "context" +import ( + "context" +) // Hybrid skill thresholds: when skill count and total token estimate are below // these limits, inline all skills as XML in the system prompt (like TS). diff --git a/internal/agent/loop_lazy_mcp_test.go b/internal/agent/loop_lazy_mcp_test.go index 51f0ba7e..ad602655 100644 --- a/internal/agent/loop_lazy_mcp_test.go +++ b/internal/agent/loop_lazy_mcp_test.go @@ -326,7 +326,7 @@ func TestLoop_LazyMCP_DenyList_BlockedOnFirstCall(t *testing.T) { if allowedTools != nil && !allowedTools[toolName] { if reg.TryActivateDeferred(toolName) { // This is the NEW deny check added by the fix. - if pe.IsDenied(toolName, nil) { + if pe.IsDenied(reg, toolName, nil) { result = tools.ErrorResult("tool not allowed by policy: " + toolName) } else { allowedTools[toolName] = true @@ -367,10 +367,8 @@ func TestLoop_LazyMCP_DenyList_GroupDeny(t *testing.T) { pe := tools.NewPolicyEngine(&config.ToolsConfig{ Deny: []string{"group:mcp_test"}, }) - // PolicyEngine needs registry to expand group:mcp_test - pe.SetRegistry(reg) - if !pe.IsDenied("mcp_svc__get_data", nil) { + if !pe.IsDenied(reg, "mcp_svc__get_data", nil) { t.Error("tool should be denied via group: pattern") } } diff --git a/internal/agent/loop_mcp_user_test.go b/internal/agent/loop_mcp_user_test.go index 4720a97d..2e03da79 100644 --- a/internal/agent/loop_mcp_user_test.go +++ b/internal/agent/loop_mcp_user_test.go @@ -85,6 +85,9 @@ func (m *minimalMCPServerStore) SetUserCredentials(_ context.Context, _ uuid.UUI func (m *minimalMCPServerStore) DeleteUserCredentials(_ context.Context, _ uuid.UUID, _ string) error { panic("not implemented") } +func (m *minimalMCPServerStore) CacheToolDescriptions(_ context.Context, _ uuid.UUID, _ map[string]store.CachedToolInfo) error { + return nil +} // recordingOAuthProvider records calls to GetValidToken for assertion. type recordingOAuthProvider struct { diff --git a/internal/agent/loop_pipeline_callbacks.go b/internal/agent/loop_pipeline_callbacks.go index bf7d1454..e7856911 100644 --- a/internal/agent/loop_pipeline_callbacks.go +++ b/internal/agent/loop_pipeline_callbacks.go @@ -6,6 +6,7 @@ import ( "encoding/hex" "fmt" "log/slog" + "slices" "strings" "time" @@ -301,7 +302,7 @@ func (l *Loop) makeAuthorizeToolCall() func(ctx context.Context, state *pipeline if l.tools != nil && l.tools.TryActivateDeferred(name) { // Re-check deny policy to prevent a lazy-activated tool from bypassing // an explicit deny rule. - if l.toolPolicy != nil && l.toolPolicy.IsDenied(name, l.agentToolPolicy) { + if l.toolPolicy != nil && l.toolPolicy.IsDenied(tools.ResolveConcreteRegistry(l.tools), name, l.agentToolPolicy) { return false, "tool not allowed by policy: " + name } allowed[name] = true @@ -312,6 +313,18 @@ func (l *Loop) makeAuthorizeToolCall() func(ctx context.Context, state *pipeline } } +// allowedToolNamesSlice converts a policy-filtered allowed-tool set into a +// sorted slice for deterministic downstream consumption (e.g. Claude CLI +// --disallowedTools derivation). +func allowedToolNamesSlice(allowed map[string]bool) []string { + names := make([]string, 0, len(allowed)) + for name := range allowed { + names = append(names, name) + } + slices.Sort(names) + return names +} + func (l *Loop) makeCallLLM(req *RunRequest, emitRun func(AgentEvent)) func(ctx context.Context, state *pipeline.RunState, chatReq providers.ChatRequest) (*providers.ChatResponse, error) { return func(ctx context.Context, state *pipeline.RunState, chatReq providers.ChatRequest) (*providers.ChatResponse, error) { provider := state.Provider @@ -330,6 +343,16 @@ func (l *Loop) makeCallLLM(req *RunRequest, emitRun func(AgentEvent)) func(ctx c chatReq.Options[providers.OptPeerKind] = req.PeerKind chatReq.Options[providers.OptLocalKey] = req.LocalKey chatReq.Options[providers.OptWorkspace] = tools.ToolWorkspaceFromCtx(ctx) + // Pass the policy-filtered allowed tool set so the Claude CLI provider + // can restrict its native built-in tools (Bash, Edit, Write, Read, + // WebFetch, WebSearch) to what the agent's tool policy actually allows. + // A nil state.Tool.AllowedTools means BuildFilteredTools didn't run + // (not wired) — do NOT set the option in that case, so the CLI + // provider's own fail-closed default (nil -> no tools allowed) applies + // rather than silently omitting the flag. + if state.Tool.AllowedTools != nil { + chatReq.Options[providers.OptAllowedToolNames] = allowedToolNamesSlice(state.Tool.AllowedTools) + } tenantID := store.TenantIDFromContext(ctx) if tenantID != uuid.Nil { chatReq.Options[providers.OptTenantID] = tenantID.String() diff --git a/internal/agent/preview_prompt.go b/internal/agent/preview_prompt.go index 51df749d..927a8bc3 100644 --- a/internal/agent/preview_prompt.go +++ b/internal/agent/preview_prompt.go @@ -2,6 +2,7 @@ package agent import ( "context" + "encoding/json" "log/slog" "path/filepath" "slices" @@ -29,6 +30,10 @@ type MCPToolPreviewInfo struct { RegisteredName string // Description is the tool description from server tool hints, if any. Description string + // Parameters is the tool's cached JSON Schema for input parameters, if + // one was captured at connect-time. nil when no schema is cached yet + // (e.g. server never connected, or cached before schema capture existed). + Parameters json.RawMessage } // PreviewDeps holds optional dependencies for building a preview system prompt. @@ -44,6 +49,11 @@ type PreviewDeps struct { Get(name string) (tools.Tool, bool) Aliases() map[string]string } + // ToolPolicy is the gateway's live PolicyEngine, used to resolve the full + // allow/deny/alsoAllow pipeline (including global deny) for preview tool + // names. nil = skip policy filtering (only skill_manage gating and alias + // exclusion apply). + ToolPolicy *tools.PolicyEngine SkillsLoader interface { BuildPinnedSummary(ctx context.Context, names []string) string BuildSummary(ctx context.Context, allowList []string) string @@ -67,6 +77,7 @@ type PreviewResult struct { // (channel, peer kind, session context, credentials) are left at zero values — // BuildSystemPrompt already nil-checks every field. func BuildPreviewPrompt(ctx context.Context, ag *store.AgentData, mode PromptMode, userID string, deps PreviewDeps) PreviewResult { + slog.Debug("BuildPreviewPrompt called", "agent_id", ag.ID, "mode", mode, "user_id", userID, "mcp_lister_nil", deps.MCPLister == nil) // --- Context files --- var contextFiles []bootstrap.ContextFile if deps.AgentStore != nil { @@ -108,9 +119,34 @@ func BuildPreviewPrompt(ctx context.Context, ag *store.AgentData, mode PromptMod toolNames = filtered } - // --- Basic agent tool policy: deny-only (Allow/AlsoAllow/ByProvider not applied). - // Full PolicyEngine requires runtime state (provider name, channel) not available in preview. --- - if toolPolicy := ag.ParseToolsConfig(); toolPolicy != nil && len(toolPolicy.Deny) > 0 { + // --- Resolve concrete *tools.Registry (needed by WouldAllow to expand + // group:* specs like "group:mcp" in Allow/AlsoAllow). Falls back to nil + // when deps.ToolLister is a narrower test mock that doesn't fully + // implement tools.ToolExecutor — WouldAllow tolerates a nil registry by + // skipping group expansion, matching prior behavior for such mocks. + var toolRegistry *tools.Registry + if executor, ok := deps.ToolLister.(tools.ToolExecutor); ok { + toolRegistry = tools.ResolveConcreteRegistry(executor) + } + + // --- Full agent tool policy (profile→allow→deny→alsoAllow, including global deny) --- + // Uses the gateway's live PolicyEngine via WouldAllow, mirroring the real chat + // loop's resolution exactly, modulo channel-specific filtering (genuinely + // unavailable here — preview has no channel/session context). + if deps.ToolPolicy != nil { + toolPolicy := ag.ParseToolsConfig() + filtered := make([]string, 0, len(toolNames)) + for _, n := range toolNames { + if deps.ToolPolicy.WouldAllow(toolRegistry, n, ag.Provider, toolPolicy, nil) { + filtered = append(filtered, n) + } + } + toolNames = filtered + } else if toolPolicy := ag.ParseToolsConfig(); toolPolicy != nil && len(toolPolicy.Deny) > 0 { + // Fallback when no PolicyEngine is wired (e.g. caller didn't set + // ToolPolicy): degrade to per-agent deny-only, matching the + // pre-PolicyEngine preview behavior. Global deny is unavailable in + // this fallback since it requires the PolicyEngine's global policy. denySet := make(map[string]bool, len(toolPolicy.Deny)) for _, d := range toolPolicy.Deny { denySet[d] = true @@ -138,21 +174,32 @@ func BuildPreviewPrompt(ctx context.Context, ag *store.AgentData, mode PromptMod } // --- MCP tool descriptions (matches loop_history_supplement.go:44-58) --- + // mcpToolParams tracks real parameter schemas alongside descriptions, keyed + // by the same RegisteredName. Populated when a real schema is available + // (live registry tool, or cached schema from a prior live connection). // First populate from live registry (tools currently loaded in active sessions). var mcpToolDescs map[string]string + var mcpToolParams map[string]map[string]any if deps.ToolLister != nil { descs := make(map[string]string) + params := make(map[string]map[string]any) for _, name := range toolNames { if !strings.HasPrefix(name, "mcp_") || name == "mcp_tool_search" { continue } if tool, ok := deps.ToolLister.Get(name); ok { descs[name] = tool.Description() + if p := tool.Parameters(); len(p) > 0 { + params[name] = p + } } } if len(descs) > 0 { mcpToolDescs = descs } + if len(params) > 0 { + mcpToolParams = params + } } slog.Debug("preview_prompt.mcp_from_registry", "agent_id", ag.ID, "live_mcp_tools", len(mcpToolDescs)) @@ -160,25 +207,59 @@ func BuildPreviewPrompt(ctx context.Context, ag *store.AgentData, mode PromptMod // MCP servers appear in the preview even when not currently loaded in the registry. slog.Debug("preview_prompt.mcp_lister_check", "agent_id", ag.ID, "mcp_lister_nil", deps.MCPLister == nil) if deps.MCPLister != nil { + slog.Debug("preview_prompt: calling MCPLister.ListToolsForAgent", "agent_id", ag.ID, "user_id", userID) storeMCPTools, err := deps.MCPLister.ListToolsForAgent(ctx, ag.ID, userID) slog.Debug("preview_prompt.mcp_lister_result", "agent_id", ag.ID, "user_id", userID, "store_tools_count", len(storeMCPTools), "error", err) if err == nil && len(storeMCPTools) > 0 { if mcpToolDescs == nil { mcpToolDescs = make(map[string]string, len(storeMCPTools)) } + toolPolicyCfg := ag.ParseToolsConfig() for _, mt := range storeMCPTools { + // MCP tool grant/access is already fully resolved by + // ListToolsForAgent's own tool_allow/tool_deny per-server + // logic above. Here we only need to catch the narrower case + // of a literal-name deny (global or per-agent tools.deny + // listing this exact tool name). We deliberately do NOT run + // the full WouldAllow group-expansion pipeline: MCP tools are + // only ever registered into ephemeral per-agent registry + // clones at live-connection time (internal/mcp/manager_connect.go), + // never into the shared/global registry used here, so + // "group:mcp" (and any other group spec) can never resolve + // correctly against toolRegistry in this connection-free + // preview context. Passing reg=nil forces IsDenied to fall + // back to a plain literal-name match with no group + // expansion, which is exactly what's needed and is immune to + // this class of registry-dependency bug. + if deps.ToolPolicy != nil && deps.ToolPolicy.IsDenied(nil, mt.RegisteredName, toolPolicyCfg) { + slog.Debug("preview_prompt.mcp_tool_denied_by_policy", "tool", mt.RegisteredName) + continue + } if _, alreadyPresent := mcpToolDescs[mt.RegisteredName]; !alreadyPresent { mcpToolDescs[mt.RegisteredName] = mt.Description slog.Debug("preview_prompt.mcp_tool_added", "tool", mt.RegisteredName, "has_desc", mt.Description != "") } else { slog.Debug("preview_prompt.mcp_tool_already_present", "tool", mt.RegisteredName) } + // Cached parameter schema, when present, is only used when the + // live registry did not already supply a real schema above. + if _, alreadyHasParams := mcpToolParams[mt.RegisteredName]; !alreadyHasParams && len(mt.Parameters) > 0 { + var schema map[string]any + if jsonErr := json.Unmarshal(mt.Parameters, &schema); jsonErr == nil { + if mcpToolParams == nil { + mcpToolParams = make(map[string]map[string]any, len(storeMCPTools)) + } + mcpToolParams[mt.RegisteredName] = schema + } else { + slog.Debug("preview_prompt.mcp_tool_params_unmarshal_failed", "tool", mt.RegisteredName, "error", jsonErr) + } + } } } else if err != nil { - slog.Info("preview_prompt.mcp_lister_error", "agent_id", ag.ID, "error", err) + slog.Debug("preview_prompt.mcp_lister_error", "agent_id", ag.ID, "error", err) } } - slog.Info("preview_prompt.mcp_tool_descs_final", "agent_id", ag.ID, "total_mcp_tools", len(mcpToolDescs)) + slog.Debug("preview_prompt.mcp_tool_descs_final", "agent_id", ag.ID, "total_mcp_tools", len(mcpToolDescs), "total_mcp_params", len(mcpToolParams)) // --- Sandbox --- sandboxCfg := ag.ParseSandboxConfig() @@ -273,8 +354,19 @@ func BuildPreviewPrompt(ctx context.Context, ag *store.AgentData, mode PromptMod toolDefs = append(toolDefs, tools.ToProviderDef(tool)) } } - // Include alias definitions (LLM receives both canonical + aliases) + // Include alias definitions (LLM receives both canonical + aliases), + // but only for aliases whose canonical tool survived the deny/gating + // filters above (toolNames). Without this check, denied tools would + // reappear in the schema via their alias name even though they were + // correctly excluded from ToolNames/the main loop. + toolNameSet := make(map[string]bool, len(toolNames)) + for _, n := range toolNames { + toolNameSet[n] = true + } for alias, canonical := range deps.ToolLister.Aliases() { + if !toolNameSet[canonical] { + continue + } if tool, ok := deps.ToolLister.Get(canonical); ok { toolDefs = append(toolDefs, providers.ToolDefinition{ Type: "function", @@ -288,6 +380,48 @@ func BuildPreviewPrompt(ctx context.Context, ag *store.AgentData, mode PromptMod } } + // --- MCP tool schemas (store-based preview, no live connection) --- + // mcpToolParams carries the REAL cached parameter schema for a tool when + // one is available (from a live registry tool, or from the connect-time + // tool_cache captured in internal/mcp/manager_connect.go). This makes + // preview parity with the live conversation path, which always sends + // real schemas via BridgeTool.Parameters() (internal/mcp/bridge_tool.go). + // Skip names already emitted via the main toolNames loop above (those + // come from a live-loaded registry tool and already have a real schema). + if len(mcpToolDescs) > 0 { + alreadyEmitted := make(map[string]bool, len(toolNames)) + for _, n := range toolNames { + alreadyEmitted[n] = true + } + mcpNames := make([]string, 0, len(mcpToolDescs)) + for name := range mcpToolDescs { + if alreadyEmitted[name] { + continue + } + mcpNames = append(mcpNames, name) + } + slices.Sort(mcpNames) + for _, name := range mcpNames { + params, ok := mcpToolParams[name] + if !ok { + // Genuine unknown-schema fallback: no live connection has ever + // cached a real schema for this tool (e.g. server never + // connected yet, or cache predates schema capture). This is + // NOT the routine case — most tools reach here with a real + // cached schema once the server has connected at least once. + params = map[string]any{"type": "object"} + } + toolDefs = append(toolDefs, providers.ToolDefinition{ + Type: "function", + Function: &providers.ToolFunctionSchema{ + Name: name, + Description: mcpToolDescs[name], + Parameters: params, + }, + }) + } + } + // --- Build system prompt (same function as LLM pipeline) --- prompt := BuildSystemPrompt(SystemPromptConfig{ AgentID: ag.AgentKey, @@ -325,6 +459,7 @@ func BuildPreviewPrompt(ctx context.Context, ag *store.AgentData, mode PromptMod // PeerKind, OwnerIDs, ExtraPrompt, CredentialCLIContext, IsBootstrap, // SandboxWorkspaceAccess }) + slog.Debug("BuildPreviewPrompt: prompt preview built", "agent_id", ag.ID, "mcp_tools", len(mcpToolDescs), "prompt_len", len(prompt), "tool_defs", len(toolDefs)) return PreviewResult{Prompt: prompt, ToolDefs: toolDefs} } diff --git a/internal/agent/preview_prompt_test.go b/internal/agent/preview_prompt_test.go index 70177258..e860e4e3 100644 --- a/internal/agent/preview_prompt_test.go +++ b/internal/agent/preview_prompt_test.go @@ -2,13 +2,17 @@ package agent import ( "context" + "encoding/json" "errors" "strings" "testing" "github.com/google/uuid" + "github.com/nextlevelbuilder/goclaw/internal/config" + "github.com/nextlevelbuilder/goclaw/internal/providers" "github.com/nextlevelbuilder/goclaw/internal/store" + "github.com/nextlevelbuilder/goclaw/internal/tools" ) func baseAgent() *store.AgentData { @@ -105,6 +109,10 @@ func TestBuildPreviewPrompt_SkillAccessStoreError(t *testing.T) { } } +// TestBuildPreviewPrompt_MCPToolDescs proves that when MCP tools are present, +// the prompt text carries the behavioral note (prefer-MCP-over-core-tools, +// optional-parameter guidance) but no longer enumerates per-tool name+description +// lines — that's now pure duplication with the real schema in ToolDefs/`tools:`. func TestBuildPreviewPrompt_MCPToolDescs(t *testing.T) { r := BuildPreviewPrompt(context.Background(), baseAgent(), PromptFull, "", PreviewDeps{ ToolLister: &mockToolLister{ @@ -114,8 +122,14 @@ func TestBuildPreviewPrompt_MCPToolDescs(t *testing.T) { }, }, }) - if !strings.Contains(r.Prompt, "mcp_pg_query") { - t.Error("expected MCP tool description in prompt") + if !strings.Contains(r.Prompt, "## MCP Tools (prefer over core tools)") { + t.Error("expected MCP Tools behavioral section header in prompt") + } + if !strings.Contains(r.Prompt, "always prefer the MCP tool") { + t.Error("expected prefer-MCP-over-core-tools instruction in prompt") + } + if strings.Contains(r.Prompt, "mcp_pg_query:") { + t.Error("expected per-tool MCP description enumeration to be removed from prompt text (now duplicated by ToolDefs schema)") } } @@ -236,3 +250,277 @@ func TestBuildPreviewPrompt_ToolDefs(t *testing.T) { t.Error("expected alias 'Read' in tool defs") } } + +// TestBuildPreviewPrompt_DeniedToolAliasExcludedFromDefs proves that when a +// canonical tool is denied via tools_config, its alias must NOT reappear in +// ToolDefs (regression test for the alias-reinjection bug: aliases were +// previously appended unconditionally regardless of the deny filter). +func TestBuildPreviewPrompt_DeniedToolAliasExcludedFromDefs(t *testing.T) { + ag := baseAgent() + ag.ToolsConfig = []byte(`{"deny":["exec"]}`) + r := BuildPreviewPrompt(context.Background(), ag, PromptFull, "", PreviewDeps{ + ToolLister: &mockToolLister{ + tools: map[string]string{ + "read_file": "Read a file", + "exec": "Execute shell", + }, + aliases: map[string]string{ + "Bash": "exec", + }, + }, + }) + for _, td := range r.ToolDefs { + if td.Function.Name == "Bash" || td.Function.Name == "exec" { + t.Errorf("denied tool %q (or its alias) should not appear in ToolDefs, got defs: %+v", td.Function.Name, r.ToolDefs) + } + } + found := false + for _, td := range r.ToolDefs { + if td.Function.Name == "read_file" { + found = true + } + } + if !found { + t.Error("expected non-denied tool 'read_file' in ToolDefs") + } +} + +// TestBuildPreviewPrompt_MCPToolInDefs proves that store-based MCP tool +// descriptions (from MCPLister, no live registry entry) are converted into +// ToolDefinition entries with at least name+description populated, and that +// when the MCPLister entry has NO cached parameter schema, the genuine +// unknown-schema placeholder is used as a fallback. +func TestBuildPreviewPrompt_MCPToolInDefs(t *testing.T) { + ag := baseAgent() + r := BuildPreviewPrompt(context.Background(), ag, PromptFull, "u1", PreviewDeps{ + ToolLister: &mockToolLister{ + tools: map[string]string{ + "read_file": "Read a file", + }, + }, + MCPLister: &mockMCPLister{ + tools: []MCPToolPreviewInfo{ + {RegisteredName: "mcp_pg_query", Description: "Run PostgreSQL queries"}, + }, + }, + }) + var found *providers.ToolDefinition + for i := range r.ToolDefs { + if r.ToolDefs[i].Function.Name == "mcp_pg_query" { + found = &r.ToolDefs[i] + } + } + if found == nil { + t.Fatalf("expected mcp_pg_query in ToolDefs, got: %+v", r.ToolDefs) + } + if found.Function.Description != "Run PostgreSQL queries" { + t.Errorf("expected description to be populated, got %q", found.Function.Description) + } + if found.Function.Parameters == nil { + t.Error("expected placeholder parameters schema, got nil") + } +} + +// TestBuildPreviewPrompt_MCPToolRealSchemaInDefs proves that when the +// MCPLister provides a real cached parameter schema (captured from a live +// MCP connection, see buildCachedToolInfo in internal/mcp/manager_connect.go), +// BuildPreviewPrompt's ToolDefs uses that REAL schema instead of the honest +// "unknown schema" placeholder — closing the gap between preview and the +// live conversation path (which always sends real schemas via +// BridgeTool.Parameters()). +func TestBuildPreviewPrompt_MCPToolRealSchemaInDefs(t *testing.T) { + ag := baseAgent() + realSchema := `{"type":"object","properties":{"query":{"type":"string"}},"required":["query"]}` + r := BuildPreviewPrompt(context.Background(), ag, PromptFull, "u1", PreviewDeps{ + MCPLister: &mockMCPLister{ + tools: []MCPToolPreviewInfo{ + { + RegisteredName: "mcp_pg_query", + Description: "Run PostgreSQL queries", + Parameters: json.RawMessage(realSchema), + }, + }, + }, + }) + var found *providers.ToolDefinition + for i := range r.ToolDefs { + if r.ToolDefs[i].Function.Name == "mcp_pg_query" { + found = &r.ToolDefs[i] + } + } + if found == nil { + t.Fatalf("expected mcp_pg_query in ToolDefs, got: %+v", r.ToolDefs) + } + params := found.Function.Parameters + props, ok := params["properties"].(map[string]any) + if !ok { + t.Fatalf("expected real schema properties, got %+v", params) + } + if _, ok := props["query"]; !ok { + t.Errorf("expected real schema property 'query', got %+v", props) + } +} + +// TestBuildPreviewPrompt_GlobalDenyApplied proves that a globally-denied tool +// (configured via the web UI's global Tools policy, with NO per-agent deny +// config) is excluded from preview's tool list. This mirrors the bug where +// preview hand-rolled a deny-only reimplementation that only consulted +// per-agent deny and never the PolicyEngine's global deny. +func TestBuildPreviewPrompt_GlobalDenyApplied(t *testing.T) { + ag := baseAgent() // tools_config is nil — no per-agent deny + pe := tools.NewPolicyEngine(&config.ToolsConfig{ + Deny: []string{"cron", "delegate"}, + }) + r := BuildPreviewPrompt(context.Background(), ag, PromptFull, "", PreviewDeps{ + ToolLister: &mockToolLister{ + tools: map[string]string{ + "read_file": "Read a file", + "cron": "Schedule tasks", + "delegate": "Delegate to sub-agent", + }, + }, + ToolPolicy: pe, + }) + if strings.Contains(r.Prompt, "cron") { + t.Error("expected globally-denied tool 'cron' to be excluded from preview prompt") + } + if strings.Contains(r.Prompt, "delegate") { + t.Error("expected globally-denied tool 'delegate' to be excluded from preview prompt") + } + if !strings.Contains(r.Prompt, "read_file") { + t.Error("expected non-denied tool 'read_file' to remain in preview prompt") + } +} + +// TestBuildPreviewPrompt_MCPToolGlobalDenyApplied proves that MCP tools +// injected from deps.MCPLister (store-based, not live registry) are also +// subject to a literal-name deny check via deps.ToolPolicy.IsDenied. A +// globally-denied MCP tool must be excluded from both the prompt text and +// ToolDefs, while other MCP tools from the same server remain included. +// This closes the gap where the MCPLister supplement path added MCP tools +// without consulting ToolPolicy at all. +func TestBuildPreviewPrompt_MCPToolGlobalDenyApplied(t *testing.T) { + ag := baseAgent() // tools_config is nil — no per-agent deny + pe := tools.NewPolicyEngine(&config.ToolsConfig{ + Deny: []string{"mcp_cloudflare__delete_dns_record"}, + }) + r := BuildPreviewPrompt(context.Background(), ag, PromptFull, "u1", PreviewDeps{ + MCPLister: &mockMCPLister{ + tools: []MCPToolPreviewInfo{ + {RegisteredName: "mcp_cloudflare__delete_dns_record", Description: "Delete a DNS record"}, + {RegisteredName: "mcp_cloudflare__list_dns_records", Description: "List DNS records"}, + }, + }, + ToolPolicy: pe, + }) + + // Prompt text no longer enumerates per-tool names/descriptions (removed as + // pure duplication of the real schema now carried in ToolDefs/`tools:`) — + // so allow/deny is asserted against ToolDefs below, not prompt text. + if strings.Contains(r.Prompt, "mcp_cloudflare__delete_dns_record") { + t.Error("expected globally-denied MCP tool to be excluded from preview prompt") + } + + for _, def := range r.ToolDefs { + if def.Function != nil && def.Function.Name == "mcp_cloudflare__delete_dns_record" { + t.Errorf("expected globally-denied MCP tool to be excluded from ToolDefs, got: %+v", r.ToolDefs) + } + } + var foundAllowed bool + for _, def := range r.ToolDefs { + if def.Function != nil && def.Function.Name == "mcp_cloudflare__list_dns_records" { + foundAllowed = true + } + } + if !foundAllowed { + t.Errorf("expected non-denied MCP tool in ToolDefs, got: %+v", r.ToolDefs) + } +} + +// TestBuildPreviewPrompt_MCPToolAllowedViaGroupExpansion proves that an MCP +// tool granted only through a "group:mcp" AlsoAllow spec — the exact pattern +// production uses (see agentToolPolicyWithMCP in resolver_helpers.go, which +// injects AlsoAllow: ["group:mcp"] for every agent) — is correctly INCLUDED +// in BuildPreviewPrompt's ToolDefs and tool names. +// +// This is a regression test for the reg=nil bug: WouldAllow(nil, ...) cannot +// expand "group:mcp" (group expansion requires a concrete *tools.Registry to +// look up group membership), so with a restrictive Allow list that excludes +// the tool by literal name, the AlsoAllow group grant would silently fail to +// restore it — exactly what happened for every MCP tool in production preview. +// Using a *tools.Registry (not the narrower mockToolLister) here exercises the +// real reg-resolution path added by ResolveConcreteRegistry. +func TestBuildPreviewPrompt_MCPToolAllowedViaGroupExpansion(t *testing.T) { + ag := baseAgent() + // Mirrors production: Allow restricts to a literal core-tool name only, + // and AlsoAllow additively grants the whole "group:mcp" set (the pattern + // injected for every agent by resolver_helpers.go's agentToolPolicyWithMCP). + ag.ToolsConfig = []byte(`{"allow":["read_file"],"alsoAllow":["group:mcp"]}`) + + reg := tools.NewRegistry() + reg.Register(&mockTool{name: "read_file", desc: "Read a file"}) + reg.Register(&mockTool{name: "mcp_pg_query", desc: "Run PostgreSQL queries"}) + reg.RegisterToolGroup("mcp", []string{"mcp_pg_query"}) + + pe := tools.NewPolicyEngine(&config.ToolsConfig{}) + + r := BuildPreviewPrompt(context.Background(), ag, PromptFull, "u1", PreviewDeps{ + ToolLister: reg, + ToolPolicy: pe, + }) + + var found *providers.ToolDefinition + for i := range r.ToolDefs { + if r.ToolDefs[i].Function.Name == "mcp_pg_query" { + found = &r.ToolDefs[i] + } + } + if found == nil { + t.Fatalf("expected mcp_pg_query (granted via group:mcp AlsoAllow) in ToolDefs, got: %+v", r.ToolDefs) + } +} + +// TestBuildPreviewPrompt_MCPToolNotDeniedIncludedWithoutGroupExpansion proves +// that MCP tools supplied via deps.MCPLister are included in preview purely +// because they are NOT literally denied — inclusion does NOT depend on +// group-expansion machinery (e.g. "group:mcp" resolving against a registry) +// at all. This is a regression test for the preview-vs-live tool-visibility +// gap: MCP tools are only ever registered into ephemeral per-agent registry +// clones at live-connection time (internal/mcp/manager_connect.go), never +// into the shared/global registry used by preview, so any check that +// requires "group:mcp" to expand against a registry can never work correctly +// here. The agent's tools_config below deliberately has a restrictive Allow +// list (excluding the MCP tool by literal name) and no AlsoAllow group grant +// at all — under the old WouldAllow-based gate this MCP tool would have been +// incorrectly excluded, since group:mcp cannot resolve against the (empty) +// global registry. With the fix, since the tool is not in any Deny list, it +// must still appear. +func TestBuildPreviewPrompt_MCPToolNotDeniedIncludedWithoutGroupExpansion(t *testing.T) { + ag := baseAgent() + // Restrictive Allow list that does NOT include the MCP tool by literal + // name, and NO AlsoAllow group grant — the exact scenario where + // group-expansion-dependent gating would previously have failed. + ag.ToolsConfig = []byte(`{"allow":["read_file"]}`) + pe := tools.NewPolicyEngine(&config.ToolsConfig{ + Deny: []string{"mcp_cloudflare__delete_dns_record"}, // unrelated tool, different name + }) + + r := BuildPreviewPrompt(context.Background(), ag, PromptFull, "u1", PreviewDeps{ + MCPLister: &mockMCPLister{ + tools: []MCPToolPreviewInfo{ + {RegisteredName: "mcp_pg_query", Description: "Run PostgreSQL queries"}, + }, + }, + ToolPolicy: pe, + }) + + var found *providers.ToolDefinition + for i := range r.ToolDefs { + if r.ToolDefs[i].Function != nil && r.ToolDefs[i].Function.Name == "mcp_pg_query" { + found = &r.ToolDefs[i] + } + } + if found == nil { + t.Fatalf("expected mcp_pg_query (not literally denied) in ToolDefs regardless of group-expansion resolution, got: %+v", r.ToolDefs) + } +} diff --git a/internal/agent/preview_prompt_test_helpers_test.go b/internal/agent/preview_prompt_test_helpers_test.go index e5705a32..c0e68958 100644 --- a/internal/agent/preview_prompt_test_helpers_test.go +++ b/internal/agent/preview_prompt_test_helpers_test.go @@ -67,6 +67,16 @@ func (m *mockSkillsLoader) BuildSummary(_ context.Context, allowList []string) s return m.summary } +// mockMCPLister implements MCPPreviewLister for testing. +type mockMCPLister struct { + tools []MCPToolPreviewInfo + err error +} + +func (m *mockMCPLister) ListToolsForAgent(_ context.Context, _ uuid.UUID, _ string) ([]MCPToolPreviewInfo, error) { + return m.tools, m.err +} + // mockSkillAccessStore returns canned skill access lists. type mockSkillAccessStore struct { accessible []store.SkillInfo diff --git a/internal/agent/systemprompt.go b/internal/agent/systemprompt.go index 30be6533..2a15b6df 100644 --- a/internal/agent/systemprompt.go +++ b/internal/agent/systemprompt.go @@ -383,11 +383,17 @@ func BuildSystemPrompt(cfg SystemPromptConfig) string { } // 4. ## Skills — full + task (pinned skills use hybrid section) - if (isFull || isTask) && !cfg.IsBootstrap && (cfg.SkillsSummary != "" || cfg.HasSkillSearch || cfg.HasSkillManage || cfg.PinnedSkillsSummary != "") { - if cfg.PinnedSkillsSummary != "" { - // Hybrid mode: pinned skills inline + search for rest - lines = append(lines, buildSkillsHybridSection(cfg.PinnedSkillsSummary, cfg.HasSkillSearch, isFull && cfg.HasSkillManage)...) - } else if isTask { + // Pinned skills must always be inlined, even on bootstrap turns — only the + // search/manage guidance and non-pinned skill summary are suppressed then. + switch { + case (isFull || isTask) && cfg.PinnedSkillsSummary != "" && cfg.IsBootstrap: + // Bootstrap: pinned skills only, no search/manage guidance. + lines = append(lines, buildSkillsHybridSection(cfg.PinnedSkillsSummary, false, false)...) + case (isFull || isTask) && !cfg.IsBootstrap && cfg.PinnedSkillsSummary != "": + // Hybrid mode: pinned skills inline + search for rest + lines = append(lines, buildSkillsHybridSection(cfg.PinnedSkillsSummary, cfg.HasSkillSearch, isFull && cfg.HasSkillManage)...) + case (isFull || isTask) && !cfg.IsBootstrap && (cfg.SkillsSummary != "" || cfg.HasSkillSearch || cfg.HasSkillManage): + if isTask { // Task mode without pinned: search-only lines = append(lines, buildSkillsSection("", cfg.HasSkillSearch, false)...) } else { @@ -396,7 +402,7 @@ func BuildSystemPrompt(cfg SystemPromptConfig) string { } // 4.1. Pinned skills — minimal/none mode standalone (pinned skills are explicitly chosen, always relevant) - if (isMinimal || isNone) && !cfg.IsBootstrap && cfg.PinnedSkillsSummary != "" { + if (isMinimal || isNone) && cfg.PinnedSkillsSummary != "" { lines = append(lines, buildPinnedSkillsMinimalSection(cfg.PinnedSkillsSummary)...) } @@ -433,11 +439,6 @@ func BuildSystemPrompt(cfg SystemPromptConfig) string { lines = append(lines, buildTeamWorkspaceSection(cfg.TeamWorkspace)...) } - // 6.4. ## Team Members — inject roster so agent knows who to assign tasks to - if !isNone && !cfg.IsBootstrap && cfg.IsTeamContext && len(cfg.TeamMembers) > 0 { - lines = append(lines, buildTeamMembersSection(cfg.TeamMembers, cfg.TeamGuidance)...) - } - // 6.45. ## Delegation Targets — from agent_links (ModeDelegate or ModeTeam with targets) if !isNone && !cfg.IsBootstrap && len(cfg.DelegateTargets) > 0 && cfg.OrchMode != ModeSpawn { lines = append(lines, buildOrchestrationSection(OrchestrationSectionData{ @@ -611,6 +612,10 @@ func sanitizePromptContextValue(value string) string { // --- Section builders --- func buildToolingSection(toolNames []string, hasSandbox bool, shellDenyGroups map[string]bool) []string { + if len(toolNames) == 0 { + return nil + } + lines := []string{ "## Tooling", "", diff --git a/internal/agent/systemprompt_bootstrap_test.go b/internal/agent/systemprompt_bootstrap_test.go index ffdc4a11..b5beb712 100644 --- a/internal/agent/systemprompt_bootstrap_test.go +++ b/internal/agent/systemprompt_bootstrap_test.go @@ -15,10 +15,10 @@ func TestBuildSystemPrompt_BootstrapStates(t *testing.T) { populatedUserMD := "# USER.md\n\n- **Name:** Alice\n- **Language:** English\n- **Timezone:** UTC+7\n" tests := []struct { - name string - cfg SystemPromptConfig - wantIn string // substring that MUST appear - wantNotIn string // substring that MUST NOT appear (empty = skip check) + name string + cfg SystemPromptConfig + wantIn string // substring that MUST appear + wantNotIn string // substring that MUST NOT appear (empty = skip check) }{ { name: "open agent with BOOTSTRAP.md → FIRST RUN slim mode", @@ -142,10 +142,10 @@ func TestBuildSystemPrompt_PredefinedBootstrapSoftened(t *testing.T) { } tests := []struct { - name string - cfg SystemPromptConfig - wantIn []string - wantNotIn []string + name string + cfg SystemPromptConfig + wantIn []string + wantNotIn []string }{ { name: "A: ws channel uses softened copy", @@ -238,3 +238,88 @@ func TestBuildSystemPrompt_NoBootstrapNoUser(t *testing.T) { t.Error("unexpected USER PROFILE INCOMPLETE section with no context files") } } + +// TestBuildSystemPrompt_PinnedSkillsAlwaysRenderOnBootstrap verifies that +// pinned skills are inlined into the system prompt even on bootstrap turns. +// Pinned skills are advertised in the UI as "always inlined in the system +// prompt" (ui/web/src/pages/agents/agent-detail/overview-sections/pinned-skills-section.tsx) +// so IsBootstrap must not suppress them, across all prompt modes. +func TestBuildSystemPrompt_PinnedSkillsAlwaysRenderOnBootstrap(t *testing.T) { + const pinnedXML = "weather-lookup" + + tests := []struct { + name string + cfg SystemPromptConfig + }{ + { + name: "full mode bootstrap", + cfg: SystemPromptConfig{ + IsBootstrap: true, + AgentType: store.AgentTypePredefined, + ToolNames: []string{"write_file"}, + PinnedSkillsSummary: pinnedXML, + }, + }, + { + name: "task mode bootstrap", + cfg: SystemPromptConfig{ + IsBootstrap: true, + Mode: PromptTask, + AgentType: store.AgentTypePredefined, + ToolNames: []string{"write_file"}, + PinnedSkillsSummary: pinnedXML, + }, + }, + { + name: "minimal mode bootstrap", + cfg: SystemPromptConfig{ + IsBootstrap: true, + Mode: PromptMinimal, + AgentType: store.AgentTypeOpen, + ToolNames: []string{"write_file"}, + PinnedSkillsSummary: pinnedXML, + }, + }, + { + name: "none mode bootstrap", + cfg: SystemPromptConfig{ + IsBootstrap: true, + Mode: PromptNone, + AgentType: store.AgentTypeOpen, + ToolNames: []string{"write_file"}, + PinnedSkillsSummary: pinnedXML, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + prompt := BuildSystemPrompt(tt.cfg) + if !strings.Contains(prompt, pinnedXML) { + t.Errorf("expected pinned skills XML in bootstrap system prompt (mode %q), got:\n%s", tt.cfg.Mode, prompt) + } + }) + } +} + +// TestBuildSystemPrompt_PinnedSkillsNonBootstrapUnchanged verifies pinned +// skills continue to render normally on non-bootstrap turns, alongside the +// search/manage guidance that bootstrap turns must suppress. +func TestBuildSystemPrompt_PinnedSkillsNonBootstrapUnchanged(t *testing.T) { + const pinnedXML = "weather-lookup" + + prompt := BuildSystemPrompt(SystemPromptConfig{ + IsBootstrap: false, + AgentType: store.AgentTypePredefined, + ToolNames: []string{"write_file", "skill_search"}, + PinnedSkillsSummary: pinnedXML, + HasSkillSearch: true, + }) + + if !strings.Contains(prompt, pinnedXML) { + t.Error("expected pinned skills XML in non-bootstrap system prompt") + } + if !strings.Contains(prompt, "skill_search") { + t.Error("expected skill_search guidance in non-bootstrap system prompt") + } +} diff --git a/internal/agent/systemprompt_sections.go b/internal/agent/systemprompt_sections.go index ecdd3a3b..44b17523 100644 --- a/internal/agent/systemprompt_sections.go +++ b/internal/agent/systemprompt_sections.go @@ -20,11 +20,6 @@ const mcpOptionalParamInstruction = "**Optional parameters:** Only include param "WRONG: {\"url\": \"https://example.com\", \"debug\": true, \"timeout\": 10000, \"format\": \"bullet\"}\n" + "RIGHT: {\"url\": \"https://example.com\"}" -// mcpToolDescMaxLen is the max character length for MCP tool descriptions -// in the system prompt inline section. ~200 chars ≈ ~50 tokens, balancing -// discoverability with prompt budget. -const mcpToolDescMaxLen = 200 - // buildCRMFreshnessSection emits a Bitrix24-specific data-freshness reminder. // LLMs tend to recall CRM record fields from earlier conversation turns; // when admin changes the user's CRM permission mid-session, the LLM may @@ -139,32 +134,21 @@ func buildMCPToolsSearchSection() []string { } } -// buildMCPToolsInlineSection generates the MCP tools section for inline mode. -// Lists each MCP tool with its real description (truncated to mcpToolDescMaxLen). +// buildMCPToolsInlineSection generates the MCP tools behavioral note for inline mode. +// Per-tool name+description enumeration was removed — the `tools:` API parameter +// now carries the real, complete MCP tool schemas (name, description, JSON schema), +// so repeating name+description in prose was pure duplication. Only the behavioral +// instructions that aren't expressible in a JSON schema (prefer-MCP-over-core, +// optional-parameter guidance) are kept here. func buildMCPToolsInlineSection(descs map[string]string) []string { - lines := []string{ + return []string{ "## MCP Tools (prefer over core tools)", "", - "External tool integrations (MCP servers). **When an MCP tool overlaps with a core tool, always prefer the MCP tool.**", + "External tool integrations (MCP servers) are available — see their schemas in the tools list. **When an MCP tool overlaps with a core tool, always prefer the MCP tool.**", "", mcpOptionalParamInstruction, "", } - // Sort MCP tool names for deterministic ordering — critical for prompt caching. - sortedNames := make([]string, 0, len(descs)) - for name := range descs { - sortedNames = append(sortedNames, name) - } - slices.Sort(sortedNames) - for _, name := range sortedNames { - desc := descs[name] - if len(desc) > mcpToolDescMaxLen { - desc = desc[:mcpToolDescMaxLen] + "…" - } - lines = append(lines, fmt.Sprintf("- %s: %s", name, desc)) - } - lines = append(lines, "") - return lines } // buildSafetySlimSection generates a 2-line safety section for task mode. @@ -268,6 +252,9 @@ func buildSkillsHybridSection(pinnedSummary string, hasSearch, hasManage bool) [ "Pinned skills (always available — scan these first):", pinnedSummary, "", + "Pinned skills shown as `` already contain their full SKILL.md content inline — use it directly, no `use_skill`/`read_file` round trip needed. "+ + "Pinned skills shown as `` (pointer only, too large to inline) still need `use_skill` then `read_file` with the exact ``.", + "", ) } if hasSearch { @@ -809,37 +796,6 @@ func buildTeamWorkspaceSection(teamWsPath string) []string { } } -// buildTeamMembersSection lists team members so the agent knows who to assign tasks to. -// teamGuidance is injected from TeamActionPolicy.MemberGuidance() — varies by edition. -func buildTeamMembersSection(members []store.TeamMemberData, teamGuidance string) []string { - lines := []string{ - "## Team Members", - "", - "Your team (use agent_key as assignee in team_tasks):", - } - for _, m := range members { - entry := fmt.Sprintf("- %s (%s) [%s]", m.AgentKey, m.DisplayName, m.Role) - if m.Frontmatter != "" { - fm := m.Frontmatter - if len([]rune(fm)) > 80 { - fm = string([]rune(fm)[:80]) + "…" - } - entry += " — " + fm - } - lines = append(lines, entry) - } - lines = append(lines, - "", - "When creating tasks with team_tasks, set assignee to the agent_key of the best-suited member.", - "Do NOT invent agent keys — only use the keys listed above.", - ) - if teamGuidance != "" { - lines = append(lines, teamGuidance) - } - lines = append(lines, "") - return lines -} - // buildVoiceResponseSection generates guidance for triggering auto TTS in "tagged" mode. // When TTS auto mode is "tagged", agent responses containing [[tts]] are converted to voice. func buildVoiceResponseSection() []string { diff --git a/internal/agent/systemprompt_team_test.go b/internal/agent/systemprompt_team_test.go index a386c2ab..999eca25 100644 --- a/internal/agent/systemprompt_team_test.go +++ b/internal/agent/systemprompt_team_test.go @@ -34,8 +34,8 @@ func TestBuildSystemPrompt_TeamContextInjection(t *testing.T) { ContextFiles: []bootstrap.ContextFile{teamMD}, AgentType: store.AgentTypePredefined, }, - wantIn: []string{"Team Shared Workspace", "Team Members"}, - wantNotIn: []string{"Sub-Agent Spawning"}, + wantIn: []string{"Team Shared Workspace"}, + wantNotIn: []string{"Sub-Agent Spawning", "Team Members"}, }, { name: "member-only inbound chat — spawn present, no team sections", @@ -60,8 +60,8 @@ func TestBuildSystemPrompt_TeamContextInjection(t *testing.T) { ContextFiles: []bootstrap.ContextFile{teamMD}, AgentType: store.AgentTypePredefined, }, - wantIn: []string{"Team Shared Workspace", "Team Members"}, - wantNotIn: []string{"Sub-Agent Spawning"}, + wantIn: []string{"Team Shared Workspace"}, + wantNotIn: []string{"Sub-Agent Spawning", "Team Members"}, }, { name: "solo agent (no team) — spawn present, availability note", @@ -115,8 +115,8 @@ func TestBuildSystemPrompt_TeamContextInjection(t *testing.T) { ContextFiles: []bootstrap.ContextFile{teamMD}, AgentType: store.AgentTypePredefined, }, - wantIn: []string{"Team Shared Workspace", "Team Members"}, - wantNotIn: []string{"Sub-Agent Spawning"}, + wantIn: []string{"Team Shared Workspace"}, + wantNotIn: []string{"Sub-Agent Spawning", "Team Members"}, }, { name: "member-only with spawn — spawn guidance present", diff --git a/internal/channels/bitrix24/provisioner_test.go b/internal/channels/bitrix24/provisioner_test.go index c2eb6af3..900cb829 100644 --- a/internal/channels/bitrix24/provisioner_test.go +++ b/internal/channels/bitrix24/provisioner_test.go @@ -121,6 +121,9 @@ func (f *fakeMCPStore) ReviewRequest(_ context.Context, _ uuid.UUID, _ bool, _, func (f *fakeMCPStore) DeleteUserCredentials(_ context.Context, _ uuid.UUID, _ string) error { return nil } +func (f *fakeMCPStore) CacheToolDescriptions(_ context.Context, _ uuid.UUID, _ map[string]store.CachedToolInfo) error { + return nil +} // --- Test helpers -------------------------------------------------------- diff --git a/internal/gateway/server.go b/internal/gateway/server.go index 92ca99be..bec05a01 100644 --- a/internal/gateway/server.go +++ b/internal/gateway/server.go @@ -50,9 +50,10 @@ type Server struct { // Non-handler dependencies (don't implement RegisterRoutes) policyEngine *permissions.PolicyEngine pairingService store.PairingStore - apiKeyStore store.APIKeyStore // for API key auth lookup - agentStore store.AgentStore // for context injection in tools_invoke - msgBus *bus.MessageBus // for MCP bridge media delivery + apiKeyStore store.APIKeyStore // for API key auth lookup + agentStore store.AgentStore // for context injection in tools_invoke + msgBus *bus.MessageBus // for MCP bridge media delivery + toolPolicy *tools.PolicyEngine // for per-agent tool policy enforcement in MCP bridge upgrader websocket.Upgrader rateLimiter *RateLimiter @@ -82,6 +83,12 @@ func (s *Server) SetPostTurnProcessor(pt tools.PostTurnProcessor) { s.postTurn = pt } +// SetToolPolicy sets the tool policy engine used to enforce per-agent tool +// access on the MCP bridge server (see internal/mcp/bridge_server.go). +func (s *Server) SetToolPolicy(pe *tools.PolicyEngine) { + s.toolPolicy = pe +} + // NewServer creates a new gateway server. func NewServer(cfg *config.Config, eventPub bus.EventPublisher, agents *agent.Router, sess store.SessionStore, toolsReg ...*tools.Registry) *Server { s := &Server{ @@ -201,7 +208,7 @@ func (s *Server) BuildMux() *http.ServeMux { // prevent unauthenticated tool invocations if port is exposed. if s.tools != nil { if s.cfg.Gateway.Token != "" { - bridgeHandler := mcpbridge.NewBridgeServer(s.tools, "1.0.0", s.msgBus) + bridgeHandler := mcpbridge.NewBridgeServer(s.tools, "1.0.0", s.msgBus, s.toolPolicy) handler := tokenAuthMiddleware(s.cfg.Gateway.Token, bridgeContextMiddleware(s.cfg.Gateway.Token, s.agentStore, bridgeHandler)) mux.Handle("/mcp/bridge", handler) @@ -284,6 +291,11 @@ func bridgeContextMiddleware(gatewayToken string, agentStore store.AgentStore, n // bridge otherwise injects only the agent UUID, leaving the key empty // -> session tools fail with "agent context required". ctx = tools.WithToolAgentKey(ctx, ag.AgentKey) + // Propagate the agent's per-agent tool policy so the MCP bridge + // handler can enforce the same policy-filtered allowlist the + // normal agent loop uses (see bridge_server.go makeToolHandler), + // instead of exposing all BridgeToolNames unconditionally. + ctx = tools.WithToolAgentPolicy(ctx, ag.ParseToolsConfig()) groups := ag.ParseShellDenyGroups() if groups != nil { ctx = store.WithShellDenyGroups(ctx, groups) @@ -470,7 +482,7 @@ func (s *Server) SetTracesHandler(h *httpapi.TracesHandler) { s.handlers = appen func (s *Server) SetWakeHandler(h *httpapi.WakeHandler) { s.handlers = append(s.handlers, h) } // SetMCPHandler sets the MCP server management handler. -func (s *Server) SetMCPHandler(h *httpapi.MCPHandler) { s.handlers = append(s.handlers, h) } +func (s *Server) SetMCPHandler(h *httpapi.MCPHandler) { s.handlers = append(s.handlers, h) } func (s *Server) SetMCPOAuthHandler(h *httpapi.MCPOAuthHandler) { s.handlers = append(s.handlers, h) } func (s *Server) SetMCPUserCredentialsHandler(h *httpapi.MCPUserCredentialsHandler) { s.handlers = append(s.handlers, h) diff --git a/internal/http/agents.go b/internal/http/agents.go index c91097f7..799e9152 100644 --- a/internal/http/agents.go +++ b/internal/http/agents.go @@ -37,6 +37,7 @@ type AgentsHandler struct { episodicStore store.EpisodicStore // for import (nil in SQLite/lite builds) vaultStore store.VaultStore // for vault import (nil = disabled) toolsReg ToolPreviewLister // for system prompt preview tool resolution (nil = fallback) + toolPE *tools.PolicyEngine // for system prompt preview tool policy resolution (nil = skip policy filtering) skillsLoader SkillPreviewBuilder // for system prompt preview pinned skills (nil = skip) skillAccessStore store.SkillAccessStore // for system prompt preview skill filtering (nil = skip) teamStore store.TeamStore // for system prompt preview team context (nil = skip) @@ -112,6 +113,14 @@ func (h *AgentsHandler) SetPreviewDeps(tl ToolPreviewLister, sl SkillPreviewBuil h.skillsLoader = sl } +// SetPreviewToolPolicy attaches the gateway's PolicyEngine so system prompt +// preview applies the same full allow/deny/alsoAllow resolution (including +// global deny) as the live chat loop. nil is safe — preview falls back to +// the tool list unfiltered by policy. +func (h *AgentsHandler) SetPreviewToolPolicy(pe *tools.PolicyEngine) { + h.toolPE = pe +} + // SetPreviewMCPManager attaches the MCP manager for store-based MCP tool preview. // When set, configured MCP tools appear in the prompt preview even when not // currently loaded in the live tool registry. diff --git a/internal/http/agents_prompt_preview.go b/internal/http/agents_prompt_preview.go index b400f4e8..038e0074 100644 --- a/internal/http/agents_prompt_preview.go +++ b/internal/http/agents_prompt_preview.go @@ -25,6 +25,12 @@ type mcpPreviewAdapter struct { // NewMCPPreviewAdapter wraps an *mcp.Manager as an agent.MCPPreviewLister. // Use this when wiring up the prompt preview handler in cmd/gateway.go. func NewMCPPreviewAdapter(mgr *mcp.Manager) agent.MCPPreviewLister { + slog.Debug("NewMCPPreviewAdapter called", "mgr_nil", mgr == nil) + if mgr == nil { + slog.Warn("NewMCPPreviewAdapter: mgr is nil — MCP tools will not appear in prompt preview") + } else { + slog.Debug("NewMCPPreviewAdapter: MCP preview adapter created with manager") + } return &mcpPreviewAdapter{mgr: mgr} } @@ -39,6 +45,7 @@ func (a *mcpPreviewAdapter) ListToolsForAgent(ctx context.Context, agentID uuid. result = append(result, agent.MCPToolPreviewInfo{ RegisteredName: mt.RegisteredName, Description: mt.Description, + Parameters: mt.Parameters, }) } return result, nil @@ -92,6 +99,7 @@ func (h *AgentsHandler) handleSystemPromptPreview(w http.ResponseWriter, r *http AgentLinks: h.agentLinkStore, ProviderReg: h.providerReg, ToolLister: h.toolsReg, + ToolPolicy: h.toolPE, SkillsLoader: h.skillsLoader, SkillAccessStore: h.skillAccessStore, MCPLister: h.mcpPreviewMgr, @@ -109,9 +117,9 @@ func (h *AgentsHandler) handleSystemPromptPreview(w http.ResponseWriter, r *http if end > len(result.Prompt) { end = len(result.Prompt) } - slog.Info("handleSystemPromptPreview.mcp_section_found", "agent_id", ag.ID, "preview", result.Prompt[mcpStart:end]) + slog.Debug("handleSystemPromptPreview.mcp_section_found", "agent_id", ag.ID, "preview", result.Prompt[mcpStart:end]) } else { - slog.Info("handleSystemPromptPreview.no_mcp_section", "agent_id", ag.ID, "prompt_len", len(result.Prompt)) + slog.Debug("handleSystemPromptPreview.no_mcp_section", "agent_id", ag.ID, "prompt_len", len(result.Prompt)) } w.Header().Set("Content-Type", "application/json") diff --git a/internal/http/agents_prompt_preview_test.go b/internal/http/agents_prompt_preview_test.go new file mode 100644 index 00000000..b200991d --- /dev/null +++ b/internal/http/agents_prompt_preview_test.go @@ -0,0 +1,157 @@ +package http + +import ( + "context" + "encoding/json" + "testing" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/mcp" + "github.com/nextlevelbuilder/goclaw/internal/store" + "github.com/nextlevelbuilder/goclaw/internal/tools" +) + +// mockMCPStoreForAdapter is a minimal store.MCPServerStore fake exercising +// only ListAccessible, the single method mcp.Manager.ListToolsForAgent +// depends on. All other methods are no-ops — this test only verifies that +// mcpPreviewAdapter.ListToolsForAgent preserves the real cached parameter +// schema returned by the underlying *mcp.Manager. +type mockMCPStoreForAdapter struct { + accessible []store.MCPAccessInfo +} + +func (m *mockMCPStoreForAdapter) ListAccessible(_ context.Context, _ uuid.UUID, _ string) ([]store.MCPAccessInfo, error) { + return m.accessible, nil +} +func (m *mockMCPStoreForAdapter) CreateServer(context.Context, *store.MCPServerData) error { return nil } +func (m *mockMCPStoreForAdapter) GetServer(context.Context, uuid.UUID) (*store.MCPServerData, error) { + return nil, nil +} +func (m *mockMCPStoreForAdapter) GetServerByName(context.Context, string) (*store.MCPServerData, error) { + return nil, nil +} +func (m *mockMCPStoreForAdapter) ListServers(context.Context) ([]store.MCPServerData, error) { + return nil, nil +} +func (m *mockMCPStoreForAdapter) UpdateServer(context.Context, uuid.UUID, map[string]any) error { + return nil +} +func (m *mockMCPStoreForAdapter) DeleteServer(context.Context, uuid.UUID) error { return nil } +func (m *mockMCPStoreForAdapter) CacheToolDescriptions(context.Context, uuid.UUID, map[string]store.CachedToolInfo) error { + return nil +} +func (m *mockMCPStoreForAdapter) GrantToAgent(context.Context, *store.MCPAgentGrant) error { + return nil +} +func (m *mockMCPStoreForAdapter) RevokeFromAgent(context.Context, uuid.UUID, uuid.UUID) error { + return nil +} +func (m *mockMCPStoreForAdapter) ListAgentGrants(context.Context, uuid.UUID) ([]store.MCPAgentGrant, error) { + return nil, nil +} +func (m *mockMCPStoreForAdapter) ListServerGrants(context.Context, uuid.UUID) ([]store.MCPAgentGrant, error) { + return nil, nil +} +func (m *mockMCPStoreForAdapter) GrantToUser(context.Context, *store.MCPUserGrant) error { + return nil +} +func (m *mockMCPStoreForAdapter) RevokeFromUser(context.Context, uuid.UUID, string) error { + return nil +} +func (m *mockMCPStoreForAdapter) CountAgentGrantsByServer(context.Context) (map[uuid.UUID]int, error) { + return nil, nil +} +func (m *mockMCPStoreForAdapter) CreateRequest(context.Context, *store.MCPAccessRequest) error { + return nil +} +func (m *mockMCPStoreForAdapter) ListPendingRequests(context.Context) ([]store.MCPAccessRequest, error) { + return nil, nil +} +func (m *mockMCPStoreForAdapter) ReviewRequest(context.Context, uuid.UUID, bool, string, string) error { + return nil +} +func (m *mockMCPStoreForAdapter) GetUserCredentials(context.Context, uuid.UUID, string) (*store.MCPUserCredentials, error) { + return nil, nil +} +func (m *mockMCPStoreForAdapter) SetUserCredentials(context.Context, uuid.UUID, string, store.MCPUserCredentials) error { + return nil +} +func (m *mockMCPStoreForAdapter) DeleteUserCredentials(context.Context, uuid.UUID, string) error { + return nil +} + +// TestMCPPreviewAdapter_PreservesRealParameterSchema proves that +// mcpPreviewAdapter.ListToolsForAgent (which bridges *mcp.Manager's +// mcp.MCPToolPreviewInfo into agent.MCPToolPreviewInfo, the shape consumed +// by BuildPreviewPrompt) forwards the real cached JSON Schema parameters +// instead of silently dropping them. This is a regression test: the field +// conversion previously only copied RegisteredName and Description, always +// zeroing Parameters, so every MCP tool fell back to the bare +// {"type":"object"} placeholder in prompt preview even when a genuine +// schema was cached from a live MCP connection (see 1290d4f1's +// tool_cache work in internal/mcp/manager.go). +func TestMCPPreviewAdapter_PreservesRealParameterSchema(t *testing.T) { + serverID := uuid.New() + toolCache := map[string]store.CachedToolInfo{ + "update_dns_record": { + Description: "Update a DNS record", + Parameters: json.RawMessage(`{"type":"object","properties":{"zone_id":{"type":"string"},"record_id":{"type":"string"}},"required":["zone_id","record_id"]}`), + }, + } + settings, err := json.Marshal(map[string]any{"tool_cache": toolCache}) + if err != nil { + t.Fatalf("marshal settings: %v", err) + } + + mockStore := &mockMCPStoreForAdapter{ + accessible: []store.MCPAccessInfo{ + { + Server: store.MCPServerData{ + BaseModel: store.BaseModel{ID: serverID}, + Name: "cloudflare", + Enabled: true, + Settings: settings, + }, + ToolAllow: nil, // unrestricted grant — matches production's typical config + ToolDeny: nil, + }, + }, + } + + mgr := mcp.NewManager(tools.NewRegistry(), mcp.WithStore(mockStore)) + adapter := NewMCPPreviewAdapter(mgr) + + got, err := adapter.ListToolsForAgent(context.Background(), uuid.New(), "user-1") + if err != nil { + t.Fatalf("ListToolsForAgent: %v", err) + } + + var found bool + for _, mt := range got { + if mt.RegisteredName != "mcp_cloudflare__update_dns_record" { + continue + } + found = true + if len(mt.Parameters) == 0 { + t.Fatal("expected real cached parameter schema to be preserved through the adapter, got empty Parameters") + } + var schema map[string]any + if err := json.Unmarshal(mt.Parameters, &schema); err != nil { + t.Fatalf("unmarshal parameters: %v", err) + } + props, ok := schema["properties"].(map[string]any) + if !ok { + t.Fatalf("expected real schema properties, got %+v", schema) + } + if _, ok := props["zone_id"]; !ok { + t.Errorf("expected real schema property 'zone_id', got %+v", props) + } + if _, ok := props["record_id"]; !ok { + t.Errorf("expected real schema property 'record_id', got %+v", props) + } + } + if !found { + t.Fatalf("expected mcp_cloudflare__update_dns_record in adapter result, got: %+v", got) + } +} diff --git a/internal/http/channel_instance_capabilities_test.go b/internal/http/channel_instance_capabilities_test.go index bf719c87..a2cd9fc2 100644 --- a/internal/http/channel_instance_capabilities_test.go +++ b/internal/http/channel_instance_capabilities_test.go @@ -83,6 +83,9 @@ func (s *fakeChannelCapabilityMCPStore) SetUserCredentials(context.Context, uuid func (s *fakeChannelCapabilityMCPStore) DeleteUserCredentials(context.Context, uuid.UUID, string) error { return nil } +func (s *fakeChannelCapabilityMCPStore) CacheToolDescriptions(context.Context, uuid.UUID, map[string]store.CachedToolInfo) error { + return nil +} type fakeChannelCapabilityCLIStore struct { binaries []store.SecureCLIBinary diff --git a/internal/http/mcp_oauth_test.go b/internal/http/mcp_oauth_test.go index 06c08c95..d34588ea 100644 --- a/internal/http/mcp_oauth_test.go +++ b/internal/http/mcp_oauth_test.go @@ -111,6 +111,9 @@ func (m *mockMCPServerForOAuth) SetUserCredentials(_ context.Context, _ uuid.UUI func (m *mockMCPServerForOAuth) DeleteUserCredentials(_ context.Context, _ uuid.UUID, _ string) error { return nil } +func (m *mockMCPServerForOAuth) CacheToolDescriptions(_ context.Context, _ uuid.UUID, _ map[string]store.CachedToolInfo) error { + return nil +} // mockOAuthTokenStore is a minimal in-memory MCPOAuthTokenStore. type mockOAuthTokenStore struct { diff --git a/internal/mcp/bridge_server.go b/internal/mcp/bridge_server.go index 9d45d538..fd72e169 100644 --- a/internal/mcp/bridge_server.go +++ b/internal/mcp/bridge_server.go @@ -54,12 +54,23 @@ var BridgeToolNames = map[string]bool{ "delegate": true, } +// bridgeProviderName identifies the caller for per-provider tool policy +// overrides (config.ToolPolicySpec.ByProvider) when enforcing bridge access. +// All MCP bridge traffic originates from the Claude CLI subprocess. +const bridgeProviderName = "claude-cli" + // NewBridgeServer creates a StreamableHTTPServer that exposes GoClaw tools as MCP tools. // It reads tools from the registry, filters to BridgeToolNames, and serves them // over streamable-http transport (stateless mode). // msgBus is optional; when non-nil, tools that produce media (deliver:true) will // publish file attachments directly to the outbound bus. -func NewBridgeServer(reg *tools.Registry, version string, msgBus *bus.MessageBus) *mcpserver.StreamableHTTPServer { +// policyEngine is optional; when non-nil, every tool call is additionally checked +// against the calling agent's policy-filtered allowlist (see +// tools.WithToolAgentPolicy / ToolAgentPolicyFromCtx) before execution, so a +// tool present in BridgeToolNames is still rejected if the agent's own tool +// policy denies it. When nil, only the static BridgeToolNames set applies +// (legacy behavior). +func NewBridgeServer(reg *tools.Registry, version string, msgBus *bus.MessageBus, policyEngine *tools.PolicyEngine) *mcpserver.StreamableHTTPServer { srv := mcpserver.NewMCPServer("goclaw-bridge", version, mcpserver.WithToolCapabilities(false), ) @@ -73,7 +84,7 @@ func NewBridgeServer(reg *tools.Registry, version string, msgBus *bus.MessageBus } mcpTool := convertToMCPTool(t) - handler := makeToolHandler(reg, name, msgBus) + handler := makeToolHandler(reg, name, msgBus, policyEngine) srv.AddTool(mcpTool, handler) registered++ } @@ -98,8 +109,22 @@ func convertToMCPTool(t tools.Tool) mcpgo.Tool { // makeToolHandler creates a ToolHandlerFunc that delegates to the GoClaw tool registry. // When msgBus is non-nil and a tool result contains Media paths, the handler publishes // them as outbound media attachments so files reach the user (e.g. Telegram document). -func makeToolHandler(reg *tools.Registry, toolName string, msgBus *bus.MessageBus) mcpserver.ToolHandlerFunc { +func makeToolHandler(reg *tools.Registry, toolName string, msgBus *bus.MessageBus, policyEngine *tools.PolicyEngine) mcpserver.ToolHandlerFunc { return func(ctx context.Context, req mcpgo.CallToolRequest) (*mcpgo.CallToolResult, error) { + // Enforce the calling agent's own tool policy before execution. Static + // membership in BridgeToolNames only bounds what the bridge process + // COULD ever expose; it does not know which agent is calling. Without + // this check, any agent reaching the bridge (e.g. via Claude CLI) can + // invoke any bridge tool regardless of its configured tool policy. + if policyEngine != nil { + agentPolicy := tools.ToolAgentPolicyFromCtx(ctx) + if !policyEngine.WouldAllow(reg, toolName, bridgeProviderName, agentPolicy, nil) { + slog.Warn("security.mcp_bridge_denied", + "tool", toolName, "agent_key", tools.ToolAgentKeyFromCtx(ctx)) + return mcpgo.NewToolResultError("tool not allowed by policy: " + toolName), nil + } + } + args := req.GetArguments() // Pass routing context (channel, chatID, peerKind, sessionKey) so native diff --git a/internal/mcp/build_cached_tool_info_test.go b/internal/mcp/build_cached_tool_info_test.go new file mode 100644 index 00000000..44892e49 --- /dev/null +++ b/internal/mcp/build_cached_tool_info_test.go @@ -0,0 +1,83 @@ +package mcp + +import ( + "encoding/json" + "testing" + + mcpgo "github.com/mark3labs/mcp-go/mcp" +) + +// TestBuildCachedToolInfo_CapturesRealSchema proves that buildCachedToolInfo +// (used by both connectServer and connectViaPool's caching goroutines in +// manager_connect.go) captures the tool's real input JSON Schema alongside +// its description, not just a bare description string. +func TestBuildCachedToolInfo_CapturesRealSchema(t *testing.T) { + mcpTools := []mcpgo.Tool{ + { + Name: "pg_query", + Description: "Run PostgreSQL queries", + InputSchema: mcpgo.ToolInputSchema{ + Type: "object", + Properties: map[string]any{ + "query": map[string]any{"type": "string"}, + }, + Required: []string{"query"}, + }, + }, + } + + got := buildCachedToolInfo(mcpTools) + + entry, ok := got["pg_query"] + if !ok { + t.Fatalf("expected pg_query entry, got %+v", got) + } + if entry.Description != "Run PostgreSQL queries" { + t.Errorf("unexpected description: %q", entry.Description) + } + if len(entry.Parameters) == 0 { + t.Fatal("expected non-empty Parameters schema") + } + + var schema map[string]any + if err := json.Unmarshal(entry.Parameters, &schema); err != nil { + t.Fatalf("unmarshal parameters: %v", err) + } + if schema["type"] != "object" { + t.Errorf("expected type=object, got %v", schema["type"]) + } + props, ok := schema["properties"].(map[string]any) + if !ok { + t.Fatalf("expected properties in schema, got %+v", schema) + } + if _, ok := props["query"]; !ok { + t.Errorf("expected 'query' property in schema, got %+v", props) + } + required, ok := schema["required"].([]any) + if !ok || len(required) != 1 || required[0] != "query" { + t.Errorf("expected required=[query], got %v", schema["required"]) + } +} + +// TestBuildCachedToolInfo_EmptySchemaFallsBackToObject proves that a tool +// with no declared properties still gets a valid (non-nil) object schema, +// matching the same convention used by inputSchemaToMap for the live path. +func TestBuildCachedToolInfo_EmptySchemaFallsBackToObject(t *testing.T) { + mcpTools := []mcpgo.Tool{ + {Name: "no_args_tool", Description: "Takes no arguments"}, + } + + got := buildCachedToolInfo(mcpTools) + + entry, ok := got["no_args_tool"] + if !ok { + t.Fatalf("expected no_args_tool entry, got %+v", got) + } + var schema map[string]any + if err := json.Unmarshal(entry.Parameters, &schema); err != nil { + t.Fatalf("unmarshal parameters: %v", err) + } + if schema["type"] != "object" { + t.Errorf("expected type=object fallback, got %v", schema["type"]) + } +} diff --git a/internal/mcp/grant_checker_test.go b/internal/mcp/grant_checker_test.go index 6e6bb5af..d884190b 100644 --- a/internal/mcp/grant_checker_test.go +++ b/internal/mcp/grant_checker_test.go @@ -15,6 +15,9 @@ import ( type mockMCPStore struct { accessible []store.MCPAccessInfo callCount int32 + // cachedToolInfo records the last argument passed to CacheToolDescriptions, + // keyed by serverID, for tests that assert on the write path. + cachedToolInfo map[uuid.UUID]map[string]store.CachedToolInfo } func (m *mockMCPStore) ListAccessible(ctx context.Context, agentID uuid.UUID, userID string) ([]store.MCPAccessInfo, error) { @@ -37,6 +40,13 @@ func (m *mockMCPStore) UpdateServer(ctx context.Context, id uuid.UUID, updates m return nil } func (m *mockMCPStore) DeleteServer(ctx context.Context, id uuid.UUID) error { return nil } +func (m *mockMCPStore) CacheToolDescriptions(ctx context.Context, serverID uuid.UUID, toolDescriptions map[string]store.CachedToolInfo) error { + if m.cachedToolInfo == nil { + m.cachedToolInfo = make(map[uuid.UUID]map[string]store.CachedToolInfo) + } + m.cachedToolInfo[serverID] = toolDescriptions + return nil +} func (m *mockMCPStore) GrantToAgent(ctx context.Context, g *store.MCPAgentGrant) error { return nil } func (m *mockMCPStore) RevokeFromAgent(ctx context.Context, serverID, agentID uuid.UUID) error { return nil diff --git a/internal/mcp/list_tools_for_agent_test.go b/internal/mcp/list_tools_for_agent_test.go new file mode 100644 index 00000000..8aac2c76 --- /dev/null +++ b/internal/mcp/list_tools_for_agent_test.go @@ -0,0 +1,230 @@ +package mcp + +import ( + "encoding/json" + "sort" + "testing" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/store" + "github.com/nextlevelbuilder/goclaw/internal/tools" +) + +// TestListToolsForAgent_EmptyToolAllowUsesCache verifies that when an agent's +// grant on an MCP server has an unrestricted (empty) ToolAllow list, but the +// server's settings contain a populated tool_cache, ListToolsForAgent +// enumerates one MCPToolPreviewInfo per cached tool instead of collapsing the +// entire server into a single "__*" placeholder entry. +func TestListToolsForAgent_EmptyToolAllowUsesCache(t *testing.T) { + serverID := uuid.New() + + toolCache := map[string]string{ + "list_zones": "List DNS zones", + "purge_cache": "Purge the CDN cache", + } + settings, err := json.Marshal(map[string]any{ + "tool_cache": toolCache, + }) + if err != nil { + t.Fatalf("marshal settings: %v", err) + } + + mockStore := &mockMCPStore{ + accessible: []store.MCPAccessInfo{ + { + Server: store.MCPServerData{ + BaseModel: store.BaseModel{ID: serverID}, + Name: "cloudflare", + Enabled: true, + Settings: settings, + }, + ToolAllow: nil, // unrestricted grant + ToolDeny: nil, + }, + }, + } + + mgr := NewManager(tools.NewRegistry(), WithStore(mockStore)) + + got, err := mgr.ListToolsForAgent(t.Context(), uuid.New(), "user-1") + if err != nil { + t.Fatalf("ListToolsForAgent: %v", err) + } + + if len(got) != len(toolCache) { + t.Fatalf("expected %d tool entries (one per cached tool), got %d: %+v", len(toolCache), len(got), got) + } + + sort.Slice(got, func(i, j int) bool { return got[i].RegisteredName < got[j].RegisteredName }) + + wantNames := map[string]string{ + "mcp_cloudflare__list_zones": "List DNS zones", + "mcp_cloudflare__purge_cache": "Purge the CDN cache", + } + for _, entry := range got { + wantDesc, ok := wantNames[entry.RegisteredName] + if !ok { + t.Fatalf("unexpected registered name %q", entry.RegisteredName) + } + if entry.Description != wantDesc { + t.Fatalf("registered name %q: got description %q, want %q", entry.RegisteredName, entry.Description, wantDesc) + } + if entry.RegisteredName == "mcp_cloudflare__*" { + t.Fatalf("got placeholder entry despite non-empty tool_cache") + } + } +} + +// TestListToolsForAgent_CacheWithRealSchemaReturnsParameters verifies that +// when the server's tool_cache uses the new shape (map[string]CachedToolInfo, +// written by buildCachedToolInfo in manager_connect.go), ListToolsForAgent +// returns the real cached parameter schema on MCPToolPreviewInfo.Parameters, +// closing the gap between preview and the live conversation path. +func TestListToolsForAgent_CacheWithRealSchemaReturnsParameters(t *testing.T) { + serverID := uuid.New() + + toolCache := map[string]store.CachedToolInfo{ + "pg_query": { + Description: "Run PostgreSQL queries", + Parameters: json.RawMessage(`{"type":"object","properties":{"query":{"type":"string"}},"required":["query"]}`), + }, + } + settings, err := json.Marshal(map[string]any{ + "tool_cache": toolCache, + }) + if err != nil { + t.Fatalf("marshal settings: %v", err) + } + + mockStore := &mockMCPStore{ + accessible: []store.MCPAccessInfo{ + { + Server: store.MCPServerData{ + BaseModel: store.BaseModel{ID: serverID}, + Name: "postgres", + Enabled: true, + Settings: settings, + }, + ToolAllow: nil, // unrestricted grant + ToolDeny: nil, + }, + }, + } + + mgr := NewManager(tools.NewRegistry(), WithStore(mockStore)) + + got, err := mgr.ListToolsForAgent(t.Context(), uuid.New(), "user-1") + if err != nil { + t.Fatalf("ListToolsForAgent: %v", err) + } + if len(got) != 1 { + t.Fatalf("expected 1 tool entry, got %d: %+v", len(got), got) + } + entry := got[0] + if entry.RegisteredName != "mcp_postgres__pg_query" { + t.Fatalf("unexpected registered name: %q", entry.RegisteredName) + } + if entry.Description != "Run PostgreSQL queries" { + t.Fatalf("unexpected description: %q", entry.Description) + } + if len(entry.Parameters) == 0 { + t.Fatal("expected real cached parameter schema, got empty") + } + var schema map[string]any + if err := json.Unmarshal(entry.Parameters, &schema); err != nil { + t.Fatalf("unmarshal cached parameters: %v", err) + } + props, ok := schema["properties"].(map[string]any) + if !ok { + t.Fatalf("expected properties in cached schema, got %+v", schema) + } + if _, ok := props["query"]; !ok { + t.Errorf("expected 'query' property in cached schema, got %+v", props) + } +} + +// TestListToolsForAgent_LegacyCacheShapeDegradesGracefully verifies that old +// cache rows stored as a bare map[string]string (name -> description, from +// before schema caching existed) are handled gracefully: descriptions are +// preserved via the backward-compat fallback path, and Parameters is left +// nil (no schema ever cached) rather than crashing on the unmarshal error. +func TestListToolsForAgent_LegacyCacheShapeDegradesGracefully(t *testing.T) { + serverID := uuid.New() + + legacyCache := map[string]string{ + "list_zones": "List DNS zones", + } + settings, err := json.Marshal(map[string]any{ + "tool_cache": legacyCache, + }) + if err != nil { + t.Fatalf("marshal settings: %v", err) + } + + mockStore := &mockMCPStore{ + accessible: []store.MCPAccessInfo{ + { + Server: store.MCPServerData{ + BaseModel: store.BaseModel{ID: serverID}, + Name: "cloudflare", + Enabled: true, + Settings: settings, + }, + ToolAllow: nil, + ToolDeny: nil, + }, + }, + } + + mgr := NewManager(tools.NewRegistry(), WithStore(mockStore)) + + got, err := mgr.ListToolsForAgent(t.Context(), uuid.New(), "user-1") + if err != nil { + t.Fatalf("ListToolsForAgent: %v", err) + } + if len(got) != 1 { + t.Fatalf("expected 1 tool entry from legacy cache, got %d: %+v", len(got), got) + } + if got[0].Description != "List DNS zones" { + t.Fatalf("expected legacy description preserved, got %q", got[0].Description) + } + if got[0].Parameters != nil { + t.Errorf("expected nil Parameters for legacy cache entry, got %s", got[0].Parameters) + } +} + +// TestListToolsForAgent_EmptyToolAllowNoCacheFallsBackToPlaceholder verifies +// that when a server has never been connected (no tool_cache present), the +// existing single-placeholder behavior is preserved. +func TestListToolsForAgent_EmptyToolAllowNoCacheFallsBackToPlaceholder(t *testing.T) { + serverID := uuid.New() + + mockStore := &mockMCPStore{ + accessible: []store.MCPAccessInfo{ + { + Server: store.MCPServerData{ + BaseModel: store.BaseModel{ID: serverID}, + Name: "never-connected", + Enabled: true, + }, + ToolAllow: nil, + ToolDeny: nil, + }, + }, + } + + mgr := NewManager(tools.NewRegistry(), WithStore(mockStore)) + + got, err := mgr.ListToolsForAgent(t.Context(), uuid.New(), "user-1") + if err != nil { + t.Fatalf("ListToolsForAgent: %v", err) + } + + if len(got) != 1 { + t.Fatalf("expected single placeholder entry, got %d: %+v", len(got), got) + } + if got[0].RegisteredName != "mcp_never_connected__*" { + t.Fatalf("unexpected placeholder registered name: %q", got[0].RegisteredName) + } +} diff --git a/internal/mcp/manager.go b/internal/mcp/manager.go index 63f0f416..bdf51525 100644 --- a/internal/mcp/manager.go +++ b/internal/mcp/manager.go @@ -150,7 +150,9 @@ func WithConfigs(cfgs map[string]*config.MCPServerConfig) ManagerOption { // This is used by the gateway to populate configs from the database after the store // is initialised, before calling Start. func (m *Manager) SetConfigs(cfgs map[string]*config.MCPServerConfig) { + slog.Debug("mcp.Manager.SetConfigs: applying configs", "count", len(cfgs)) m.configs = cfgs + slog.Debug("mcp.Manager.SetConfigs: configs applied successfully", "count", len(cfgs)) } // WithStore sets the MCPServerStore for DB-backed MCP server loading. @@ -160,6 +162,12 @@ func WithStore(s store.MCPServerStore) ManagerOption { } } +// SetStore sets the MCP store on an already-constructed Manager. +// Use this when the store is not yet available at construction time. +func (m *Manager) SetStore(s store.MCPServerStore) { + m.store = s +} + // WithPool sets a shared connection pool for MCP servers. // When set, LoadForAgent uses the pool instead of creating per-agent connections. func WithPool(p *Pool) ManagerOption { @@ -198,17 +206,20 @@ func NewManager(registry *tools.Registry, opts ...ManagerOption) *Manager { // Start connects to all config-file MCP servers. // Non-fatal: logs warnings for servers that fail to connect and continues. func (m *Manager) Start(ctx context.Context) error { + slog.Debug("mcp.Manager.Start: called", "configs", len(m.configs)) if len(m.configs) == 0 { + slog.Debug("mcp.Manager.Start: no configs, returning early") return nil } var errs []string for name, cfg := range m.configs { if !cfg.IsEnabled() { - slog.Info("mcp.server.disabled", "server", name) + slog.Debug("mcp.server.disabled", "server", name) continue } + slog.Debug("mcp.Manager.Start: starting server", "name", name, "transport", cfg.Transport) // Config-path servers have no DB ID — pass uuid.Nil headers, err := resolveEnvVars(cfg.Headers) if err != nil { @@ -222,9 +233,19 @@ func (m *Manager) Start(ctx context.Context) error { if err := m.connectServer(ctx, name, cfg.Transport, cfg.Command, cfg.Args, cfg.Env, cfg.URL, headers, cfg.ToolPrefix, cfg.TimeoutSec, uuid.Nil, ToolHints{}, nil, nil); err != nil { slog.Warn("mcp.server.connect_failed", "server", name, "error", err) errs = append(errs, fmt.Sprintf("%s: %v", name, err)) + } else { + m.mu.RLock() + toolCount := 0 + if ss, ok := m.servers[name]; ok { + toolCount = len(ss.toolNames) + } + m.mu.RUnlock() + slog.Debug("mcp.Manager.Start: server started", "name", name, "tools", toolCount) } } + totalTools := len(m.ToolNames()) + slog.Debug("mcp.Manager.Start: fully started", "total_tools", totalTools, "errors", len(errs)) if len(errs) > 0 { return fmt.Errorf("some MCP servers failed to connect: %s", joinErrors(errs)) } @@ -646,6 +667,11 @@ type MCPToolPreviewInfo struct { RegisteredName string // Description is derived from server tool hints, if configured. Description string + // Parameters is the tool's cached JSON Schema for input parameters, captured + // at connect-time (see buildCachedToolInfo in manager_connect.go). nil when + // no schema has been cached yet (server never connected, or cache predates + // schema capture). + Parameters json.RawMessage } // ListToolsForAgent returns a best-effort list of MCP tool names and descriptions @@ -685,8 +711,69 @@ func (m *Manager) ListToolsForAgent(ctx context.Context, agentID uuid.UUID, user effectivePrefix := ensureMCPPrefix(info.Server.ToolPrefix, info.Server.Name) slog.Debug("mcp.ListToolsForAgent.server_hints", "server", info.Server.Name, "global_hint", hints.Global, "tool_hints_count", len(hints.Tools), "effective_prefix", effectivePrefix) + // Parse tool cache from settings as fallback descriptions + parameter schemas. + toolCache := make(map[string]store.CachedToolInfo) + if len(info.Server.Settings) > 0 { + var settingsMap map[string]json.RawMessage + if err := json.Unmarshal(info.Server.Settings, &settingsMap); err == nil { + if cacheRaw, ok := settingsMap["tool_cache"]; ok { + if err := json.Unmarshal(cacheRaw, &toolCache); err != nil { + // Backward-compat: pre-schema-caching rows stored a bare + // map[string]string (name -> description). Fall back to + // that shape and treat entries as description-only (no + // parameter schema). Stale rows self-heal on next connect + // since the write path always writes the new shape. + var legacyCache map[string]string + if legacyErr := json.Unmarshal(cacheRaw, &legacyCache); legacyErr == nil { + toolCache = make(map[string]store.CachedToolInfo, len(legacyCache)) + for name, desc := range legacyCache { + toolCache[name] = store.CachedToolInfo{Description: desc} + } + } else { + slog.Debug("mcp.ListToolsForAgent.tool_cache_unmarshal_failed", "server", info.Server.Name, "error", err) + } + } + } + } + } + + // Build deny set + denySet := make(map[string]struct{}, len(info.ToolDeny)) + for _, d := range info.ToolDeny { + denySet[d] = struct{}{} + } + if len(info.ToolAllow) == 0 { - // Unknown tool list — emit one placeholder entry for the server. + if len(toolCache) > 0 { + // Unrestricted grant, but we have real tool names cached from a + // prior connection — enumerate them instead of a placeholder. + var serverTools []string + for toolName := range toolCache { + if _, denied := denySet[toolName]; denied { + slog.Debug("mcp.ListToolsForAgent.tool_denied", "server", info.Server.Name, "tool", toolName) + continue + } + registeredName := effectivePrefix + "__" + toolName + cached := toolCache[toolName] + desc := hints.HintFor(toolName) + if desc == "" { + desc = cached.Description + } + if desc == "" && hints.Global != "" { + desc = hints.Global + } + serverTools = append(serverTools, registeredName) + result = append(result, MCPToolPreviewInfo{ + RegisteredName: registeredName, + Description: desc, + Parameters: cached.Parameters, + }) + } + slog.Debug("mcp.ListToolsForAgent.server_tools_added_from_cache", "server", info.Server.Name, "tools", serverTools) + continue + } + + // Unknown tool list and nothing cached — emit one placeholder entry. placeholder := effectivePrefix + "__*" desc := hints.Global if desc == "" { @@ -700,12 +787,6 @@ func (m *Manager) ListToolsForAgent(ctx context.Context, agentID uuid.UUID, user continue } - // Build deny set - denySet := make(map[string]struct{}, len(info.ToolDeny)) - for _, d := range info.ToolDeny { - denySet[d] = struct{}{} - } - var serverTools []string for _, toolName := range info.ToolAllow { if _, denied := denySet[toolName]; denied { @@ -713,7 +794,11 @@ func (m *Manager) ListToolsForAgent(ctx context.Context, agentID uuid.UUID, user continue } registeredName := effectivePrefix + "__" + toolName + cached := toolCache[toolName] desc := hints.HintFor(toolName) + if desc == "" { + desc = cached.Description + } if desc == "" && hints.Global != "" { desc = hints.Global } @@ -721,6 +806,7 @@ func (m *Manager) ListToolsForAgent(ctx context.Context, agentID uuid.UUID, user result = append(result, MCPToolPreviewInfo{ RegisteredName: registeredName, Description: desc, + Parameters: cached.Parameters, }) } slog.Debug("mcp.ListToolsForAgent.server_tools_added", "server", info.Server.Name, "tools", serverTools) diff --git a/internal/mcp/manager_connect.go b/internal/mcp/manager_connect.go index db10bbb5..d8f645eb 100644 --- a/internal/mcp/manager_connect.go +++ b/internal/mcp/manager_connect.go @@ -2,6 +2,7 @@ package mcp import ( "context" + "encoding/json" "fmt" "log/slog" "strings" @@ -11,6 +12,8 @@ import ( mcpclient "github.com/mark3labs/mcp-go/client" "github.com/mark3labs/mcp-go/client/transport" mcpgo "github.com/mark3labs/mcp-go/mcp" + + "github.com/nextlevelbuilder/goclaw/internal/store" ) // connectAndDiscover creates a client, initializes the MCP handshake, and @@ -116,6 +119,18 @@ func (m *Manager) connectServer(ctx context.Context, name, transportType, comman registeredNames := m.registerBridgeTools(ss, mcpTools, name, toolPrefix, timeoutSec, serverID, hints, toolAllow, toolDeny) ss.toolNames = registeredNames + // Cache tool descriptions + parameter schemas for prompt preview (no live connection needed) + if serverID != uuid.Nil && m.store != nil && len(mcpTools) > 0 { + toolInfo := buildCachedToolInfo(mcpTools) + go func(sid uuid.UUID, info map[string]store.CachedToolInfo) { + cacheCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if err := m.store.CacheToolDescriptions(cacheCtx, sid, info); err != nil { + slog.Debug("mcp.cache_tool_descriptions.failed", "server_id", sid, "error", err) + } + }(serverID, toolInfo) + } + // Create health monitoring context hctx, hcancel := context.WithCancel(context.Background()) ss.cancel = hcancel @@ -203,6 +218,18 @@ func (m *Manager) connectViaPool(ctx context.Context, tenantID uuid.UUID, name, // Create per-agent BridgeTools from the pool's shared connection registeredNames := m.registerPoolBridgeTools(entry, name, toolPrefix, timeoutSec, serverID, hints, toolAllow, toolDeny) + // Cache tool descriptions + parameter schemas for prompt preview (no live connection needed) + if serverID != uuid.Nil && m.store != nil && len(entry.tools) > 0 { + toolInfo := buildCachedToolInfo(entry.tools) + go func(sid uuid.UUID, info map[string]store.CachedToolInfo) { + cacheCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if err := m.store.CacheToolDescriptions(cacheCtx, sid, info); err != nil { + slog.Debug("mcp.cache_tool_descriptions.failed", "server_id", sid, "error", err) + } + }(serverID, toolInfo) + } + // Track server state and per-agent tool names. // poolServers/poolToolNames keyed by plain name for Close() iteration. // poolKeys maps plain name → pool compound key for Release(). @@ -483,3 +510,27 @@ func fullReconnect(ctx context.Context, ss *serverState) bool { _ = oldClient.Close() return true } + +// buildCachedToolInfo converts live-discovered MCP tools into the cache +// shape stored under a server's settings "tool_cache" key, capturing both +// the description and the real input JSON Schema (reusing the same +// conversion the live BridgeTool path uses, see inputSchemaToMap in +// bridge_tool.go), so preview mode can render accurate parameter schemas +// without a live connection. +func buildCachedToolInfo(mcpTools []mcpgo.Tool) map[string]store.CachedToolInfo { + toolInfo := make(map[string]store.CachedToolInfo, len(mcpTools)) + for _, t := range mcpTools { + schema := inputSchemaToMap(t.InputSchema) + schemaJSON, err := json.Marshal(schema) + if err != nil { + slog.Debug("mcp.build_cached_tool_info.marshal_schema_failed", "tool", t.Name, "error", err) + toolInfo[t.Name] = store.CachedToolInfo{Description: t.Description} + continue + } + toolInfo[t.Name] = store.CachedToolInfo{ + Description: t.Description, + Parameters: schemaJSON, + } + } + return toolInfo +} diff --git a/internal/providers/claude_cli.go b/internal/providers/claude_cli.go index 2271f5d0..ac69beb9 100644 --- a/internal/providers/claude_cli.go +++ b/internal/providers/claude_cli.go @@ -39,6 +39,14 @@ const OptTenantID = "tenant_id" // OptLocalKey passes the composite local key (e.g. "-100123:topic:42") for forum topic routing. const OptLocalKey = "local_key" +// OptAllowedToolNames passes the agent's policy-filtered canonical GoClaw tool +// names ([]string) for the current turn. Used to derive --disallowedTools for +// the Claude CLI subprocess so native CLI tools (Bash, Edit, Write, Read, +// WebFetch, WebSearch) are only permitted when their GoClaw equivalent is +// allowed by the agent's tool policy. A nil/absent value is treated as "no +// tools allowed" (fail closed). +const OptAllowedToolNames = "allowed_tool_names" + // ClaudeCLIProvider implements Provider by shelling out to the `claude` CLI binary. // It acts as a thin proxy: CLI manages session history, tool execution, and context. // GoClaw only forwards the latest user message and streams back the response. diff --git a/internal/providers/claude_cli_chat.go b/internal/providers/claude_cli_chat.go index fe8ea5d5..b1e74aaa 100644 --- a/internal/providers/claude_cli_chat.go +++ b/internal/providers/claude_cli_chat.go @@ -46,7 +46,8 @@ func (p *ClaudeCLIProvider) Chat(ctx context.Context, req ChatRequest) (*ChatRes outputFmt = "stream-json" } effortLevel := extractStringOpt(req.Options, OptThinkingLevel) - args := p.buildArgs(model, workDir, mcpPath, cliSessionID, outputFmt, len(images) > 0, disableTools, effortLevel) + allowedToolNames := extractStringSliceOpt(req.Options, OptAllowedToolNames) + args := p.buildArgs(model, workDir, mcpPath, cliSessionID, outputFmt, len(images) > 0, disableTools, effortLevel, allowedToolNames) var stdin *bytes.Reader if len(images) > 0 { @@ -108,7 +109,8 @@ func (p *ClaudeCLIProvider) ChatStream(ctx context.Context, req ChatRequest, onC bc := bridgeContextFromOpts(req.Options) mcpPath := p.resolveMCPConfigPath(ctx, sessionKey, bc) effortLevel := extractStringOpt(req.Options, OptThinkingLevel) - args := p.buildArgs(model, workDir, mcpPath, cliSessionID, "stream-json", len(images) > 0, disableTools, effortLevel) + allowedToolNames := extractStringSliceOpt(req.Options, OptAllowedToolNames) + args := p.buildArgs(model, workDir, mcpPath, cliSessionID, "stream-json", len(images) > 0, disableTools, effortLevel, allowedToolNames) var stdin *bytes.Reader if len(images) > 0 { diff --git a/internal/providers/claude_cli_session.go b/internal/providers/claude_cli_session.go index 452bfc0f..8759d82e 100644 --- a/internal/providers/claude_cli_session.go +++ b/internal/providers/claude_cli_session.go @@ -8,12 +8,54 @@ import ( "log/slog" "os" "path/filepath" + "slices" "strings" "github.com/google/uuid" "github.com/nextlevelbuilder/goclaw/internal/config" ) +// cliNativeToolGoclawEquivalent maps Claude CLI native built-in tool names to +// their GoClaw canonical registry equivalents. A native tool is only allowed +// (omitted from --disallowedTools) when its GoClaw equivalent is present in +// the agent's policy-filtered allowed tool set. +var cliNativeToolGoclawEquivalent = map[string]string{ + "Bash": "exec", + "Edit": "edit", + "Write": "write_file", + "Read": "read_file", + "WebFetch": "web_fetch", + "WebSearch": "web_search", +} + +// cliNativeToolsAlwaysBlocked lists Claude CLI native tools with no GoClaw +// policy equivalent. They are always disallowed so agent tool policy cannot +// be bypassed through them. +var cliNativeToolsAlwaysBlocked = []string{ + "Glob", "Grep", "TodoRead", "TodoWrite", "NotebookRead", "NotebookEdit", +} + +// disallowedCLITools computes the --disallowedTools value for the Claude CLI +// subprocess from the agent's policy-filtered allowed GoClaw tool names. +// allowedToolNames == nil is treated as "no tools allowed" (fail closed): +// every native tool with a GoClaw equivalent is blocked, same as an empty set. +func disallowedCLITools(allowedToolNames []string) []string { + allowed := make(map[string]bool, len(allowedToolNames)) + for _, name := range allowedToolNames { + allowed[name] = true + } + + blocked := make([]string, 0, len(cliNativeToolGoclawEquivalent)+len(cliNativeToolsAlwaysBlocked)) + for native, equivalent := range cliNativeToolGoclawEquivalent { + if !allowed[equivalent] { + blocked = append(blocked, native) + } + } + blocked = append(blocked, cliNativeToolsAlwaysBlocked...) + slices.Sort(blocked) + return blocked +} + // validCLIModels lists accepted model aliases for the Claude CLI. var validCLIModels = map[string]bool{ "sonnet": true, "opus": true, "haiku": true, @@ -30,7 +72,10 @@ func validateCLIModel(model string) error { // buildArgs constructs CLI arguments. // mcpConfigPath is the resolved per-session MCP config file (may differ per call). // effort is the reasoning effort level (low/medium/high); empty or "off" omits the flag. -func (p *ClaudeCLIProvider) buildArgs(model, workDir, mcpConfigPath string, cliSessionID uuid.UUID, outputFormat string, hasImages, disableTools bool, effort string) []string { +// allowedToolNames is the agent's policy-filtered canonical GoClaw tool set for this +// turn; it drives which Claude CLI native built-in tools are permitted (see +// disallowedCLITools). nil means "no tools allowed" (fail closed). +func (p *ClaudeCLIProvider) buildArgs(model, workDir, mcpConfigPath string, cliSessionID uuid.UUID, outputFormat string, hasImages, disableTools bool, effort string, allowedToolNames []string) []string { args := []string{ "-p", "--output-format", outputFormat, @@ -66,13 +111,17 @@ func (p *ClaudeCLIProvider) buildArgs(model, workDir, mcpConfigPath string, cliS args = append(args, "--input-format", "stream-json") } + // --disallowedTools is always computed, regardless of whether an MCP config + // path was resolved: when disableTools is set (e.g. summoner), no native + // tools are permitted; otherwise the agent's policy-filtered allowed tool + // set determines which native CLI tools (with a GoClaw equivalent) may run. + // This must never be skipped — omitting it previously let the CLI + // subprocess run with its full native toolset unrestricted whenever no MCP + // config was resolved. if disableTools { - // Summoner: disable all tools entirely via disallowedTools - args = append(args, "--disallowedTools", "Bash,Edit,Read,Write,Glob,Grep,WebFetch,WebSearch,TodoRead,TodoWrite,NotebookRead,NotebookEdit") - } else if mcpConfigPath != "" { - // Chat with MCP bridge: disable CLI built-in tools, only allow MCP bridge tools. - // This ensures all tool execution goes through GoClaw's controlled MCP bridge. - args = append(args, "--disallowedTools", "Bash,Edit,Read,Write,Glob,Grep,WebFetch,WebSearch,TodoRead,TodoWrite,NotebookRead,NotebookEdit") + args = append(args, "--disallowedTools", strings.Join(disallowedCLITools(nil), ",")) + } else { + args = append(args, "--disallowedTools", strings.Join(disallowedCLITools(allowedToolNames), ",")) } if p.hooksSettingsPath != "" { @@ -167,6 +216,24 @@ func extractBoolOpt(opts map[string]any, key string) bool { return false } +// extractStringSliceOpt gets a []string value from Options map by key. +// Returns nil if absent or of an unexpected type (fail closed: callers must +// treat nil as "no tools allowed", not "unrestricted"). +func extractStringSliceOpt(opts map[string]any, key string) []string { + if opts == nil { + return nil + } + v, ok := opts[key] + if !ok { + return nil + } + names, ok := v.([]string) + if !ok { + return nil + } + return names +} + // bridgeContextFromOpts builds a BridgeContext from the Options map. func bridgeContextFromOpts(opts map[string]any) BridgeContext { return BridgeContext{ diff --git a/internal/skills/loader.go b/internal/skills/loader.go index 393a4031..cbfc40c6 100644 --- a/internal/skills/loader.go +++ b/internal/skills/loader.go @@ -23,6 +23,11 @@ import ( "sync" "sync/atomic" "time" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/config" + "github.com/nextlevelbuilder/goclaw/internal/store" ) // Metadata holds parsed SKILL.md frontmatter. @@ -51,12 +56,14 @@ type Loader struct { globalSkills string // ~/.goclaw/skills/ builtinSkills string // bundled with binary - // DB-managed skills directory (set via SetManagedDir). - // Uses versioned subdirectory structure: ///SKILL.md - managedSkillsDir string + // dataDir is the root data directory used to resolve the tenant-scoped + // managed skills directory per-call via resolveManagedDir. Managed skills + // use versioned subdirectories: /[tenants//]skills-store///SKILL.md. + dataDir string + managedDirSet bool // true once a data dir has been configured (enables managed lookup) mu sync.RWMutex - cache map[string]*Info // name → info (lazily populated) + cache map[uuid.UUID]map[string]*Info // tenant ID → name → info (lazily populated, tenant-scoped) // Version tracking for hot-reload (matching TS bumpSkillsSnapshotVersion). // Bumped by the watcher on SKILL.md changes; consumers compare to detect staleness. @@ -88,24 +95,61 @@ func NewLoader(workspace, globalSkills, builtinSkills string) *Loader { personalAgentSkills: personalAgentSkills, globalSkills: globalSkills, builtinSkills: builtinSkills, - cache: make(map[string]*Info), + cache: make(map[uuid.UUID]map[string]*Info), } } -// SetManagedDir sets the managed skills directory (skills-store). -// Managed skills use versioned subdirectories: ///SKILL.md. -// Called after PG stores are created. +// SetManagedDir sets the root data directory used to resolve each tenant's +// managed skills directory (skills-store) on every call, via resolveManagedDir. +// dir must be the root data dir (e.g. GOCLAW_DATA_DIR), NOT a pre-resolved +// tenant-specific path — resolving a single fixed path here would leak or +// hide skills across tenants. Called after PG stores are created. func (l *Loader) SetManagedDir(dir string) { - l.managedSkillsDir = dir + l.dataDir = dir + l.managedDirSet = dir != "" l.BumpVersion() // trigger re-scan } +// resolveManagedDir computes the managed skills directory for the tenant +// bound to ctx. Resolves to exactly one tenant's directory per call — never +// enumerates or scans other tenants' directories. +func (l *Loader) resolveManagedDir(ctx context.Context) string { + if !l.managedDirSet { + return "" + } + tid := store.TenantIDFromContext(ctx) + if tid == uuid.Nil { + tid = store.MasterTenantID + } + slug := store.TenantSlugFromContext(ctx) + return config.TenantSkillsStoreDir(l.dataDir, tid, slug) +} + +// tenantCacheKey returns the tenant ID used to scope the in-memory info cache +// for ctx, defaulting to the master tenant when unset. +func tenantCacheKey(ctx context.Context) uuid.UUID { + tid := store.TenantIDFromContext(ctx) + if tid == uuid.Nil { + return store.MasterTenantID + } + return tid +} + // ListSkills returns all available skills, respecting the priority hierarchy. // Higher-priority sources override lower ones by name. -func (l *Loader) ListSkills(_ context.Context) []Info { +func (l *Loader) ListSkills(ctx context.Context) []Info { + managedDir := l.resolveManagedDir(ctx) + tid := tenantCacheKey(ctx) + l.mu.Lock() defer l.mu.Unlock() + tenantCache := l.cache[tid] + if tenantCache == nil { + tenantCache = make(map[string]*Info) + l.cache[tid] = tenantCache + } + seen := make(map[string]bool) var skills []Info @@ -152,19 +196,20 @@ func (l *Loader) ListSkills(_ context.Context) []Info { } skills = append(skills, info) seen[d.Name()] = true - l.cache[d.Name()] = &info + tenantCache[d.Name()] = &info } } // Managed skills (versioned, DB-seeded) come before builtin so their workspace paths win. - if l.managedSkillsDir != "" { - for _, info := range l.listManagedSkills() { + if managedDir != "" { + managedSkills := l.listManagedSkills(managedDir) + for _, info := range managedSkills { if seen[info.Slug] { continue } skills = append(skills, info) seen[info.Slug] = true - l.cache[info.Slug] = &info + tenantCache[info.Slug] = &info } } @@ -195,7 +240,7 @@ func (l *Loader) ListSkills(_ context.Context) []Info { } skills = append(skills, info) seen[d.Name()] = true - l.cache[d.Name()] = &info + tenantCache[d.Name()] = &info } } } @@ -203,11 +248,12 @@ func (l *Loader) ListSkills(_ context.Context) []Info { return skills } -// listManagedSkills scans the managed skills directory for versioned skill directories. -// Structure: ///SKILL.md +// listManagedSkills scans the given managed skills directory (already resolved +// to a single tenant via resolveManagedDir) 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) +func (l *Loader) listManagedSkills(managedDir string) []Info { + dirs, err := os.ReadDir(managedDir) if err != nil { return nil } @@ -220,7 +266,7 @@ func (l *Loader) listManagedSkills() []Info { slug := d.Name() // Find the latest version subdirectory - latestVersion, latestDir := l.findLatestVersion(slug) + latestVersion, latestDir := l.findLatestVersion(managedDir, slug) if latestVersion < 0 { continue } @@ -248,10 +294,11 @@ func (l *Loader) listManagedSkills() []Info { return skills } -// findLatestVersion finds the highest-numbered version subdirectory for a skill slug. +// findLatestVersion finds the highest-numbered version subdirectory for a skill slug +// within the given managed directory (already resolved to a single tenant). // Returns (version, path) or (-1, "") if no valid version found. -func (l *Loader) findLatestVersion(slug string) (int, string) { - slugDir := filepath.Join(l.managedSkillsDir, slug) +func (l *Loader) findLatestVersion(managedDir, slug string) (int, string) { + slugDir := filepath.Join(managedDir, slug) entries, err := os.ReadDir(slugDir) if err != nil { return -1, "" @@ -280,7 +327,7 @@ func (l *Loader) findLatestVersion(slug string) (int, string) { // 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. // Priority: workspace > agents > global > managed > builtin -func (l *Loader) LoadSkill(_ context.Context, name string) (string, bool) { +func (l *Loader) LoadSkill(ctx context.Context, name string) (string, bool) { // Check flat skill directories (workspace, agents, global) first for _, dir := range []string{l.workspaceSkills, l.projectAgentSkills, l.personalAgentSkills, l.globalSkills} { if dir == "" { @@ -297,8 +344,9 @@ func (l *Loader) LoadSkill(_ context.Context, name string) (string, bool) { } // Managed skills (DB-seeded, versioned) take priority over raw builtin files. - if l.managedSkillsDir != "" { - latestVer, latestDir := l.findLatestVersion(name) + // Resolved to the calling tenant's own directory only — never scans other tenants'. + if managedDir := l.resolveManagedDir(ctx); managedDir != "" { + latestVer, latestDir := l.findLatestVersion(managedDir, name) if latestVer >= 0 { path := filepath.Join(latestDir, "SKILL.md") data, err := os.ReadFile(path) @@ -410,14 +458,78 @@ func (l *Loader) BuildSummary(ctx context.Context, allowList []string) string { return strings.Join(lines, "\n") } -// BuildPinnedSummary generates XML summary for only the pinned skill names. -// Delegates to BuildSummary with pinned names as allowlist. +// skillInlineMaxBytes caps how many bytes of a single pinned skill's SKILL.md +// body may be inlined directly into the system prompt. Sampled SKILL.md sizes +// run 3.6KB-10KB typical; skills above this cap fall back to pointer-only +// format (name/description/location) so the agent uses use_skill+read_file. +const skillInlineMaxBytes = 10000 + +// skillInlineTotalMaxBytes caps the combined inlined size across all pinned +// skills in a single prompt. Skills that don't fit (in allowlist order) fall +// back to pointer-only format. +const skillInlineTotalMaxBytes = skillInlineMaxBytes * 3 + +// BuildPinnedSummary generates an XML summary for only the pinned skill +// names, with each skill's full SKILL.md content (frontmatter stripped) +// inlined directly via . Skills whose content exceeds +// skillInlineMaxBytes, or that would push the combined total over +// skillInlineTotalMaxBytes, fall back to a pointer-only entry +// (name/description/location) so the agent can use_skill+read_file instead. // Returns empty string if none match. func (l *Loader) BuildPinnedSummary(ctx context.Context, pinnedNames []string) string { if len(pinnedNames) == 0 { return "" } - return l.BuildSummary(ctx, pinnedNames) + + allSkills := l.ListSkills(ctx) + if len(allSkills) == 0 { + return "" + } + + byName := make(map[string]bool, len(pinnedNames)) + for _, name := range pinnedNames { + byName[name] = true + } + + var filtered []Info + for _, s := range allSkills { + if byName[s.Slug] { + filtered = append(filtered, s) + } + } + if len(filtered) == 0 { + return "" + } + + var lines []string + lines = append(lines, "") + totalInlined := 0 + for _, s := range filtered { + content, ok := l.LoadSkill(ctx, s.Slug) + content = strings.TrimSpace(content) + fits := ok && len(content) <= skillInlineMaxBytes && totalInlined+len(content) <= skillInlineTotalMaxBytes + if fits { + lines = append(lines, fmt.Sprintf(" ", s.Name)) + lines = append(lines, content) + lines = append(lines, " ") + totalInlined += len(content) + continue + } + + lines = append(lines, " ") + lines = append(lines, fmt.Sprintf(" %s", escapeXML(s.Name))) + desc := s.Description + if len([]rune(desc)) > skillDescMaxLen { + desc = string([]rune(desc)[:skillDescMaxLen]) + "…" + } + lines = append(lines, fmt.Sprintf(" %s", escapeXML(desc))) + lines = append(lines, fmt.Sprintf(" %s", escapeXML(s.Path))) + lines = append(lines, " content too large to inline, use use_skill+read_file") + lines = append(lines, " ") + } + lines = append(lines, "") + + return strings.Join(lines, "\n") } // Version returns the current skill snapshot version. @@ -432,13 +544,19 @@ func (l *Loader) BumpVersion() { } // Dirs returns all non-empty skill directories (for the watcher to monitor). +// The managed skills directory returned here is always the master tenant's — +// per-tenant managed directories are resolved dynamically per request via +// resolveManagedDir and are not enumerated here. func (l *Loader) Dirs() []string { var dirs []string - for _, d := range []string{l.workspaceSkills, l.projectAgentSkills, l.personalAgentSkills, l.globalSkills, l.builtinSkills, l.managedSkillsDir} { + for _, d := range []string{l.workspaceSkills, l.projectAgentSkills, l.personalAgentSkills, l.globalSkills, l.builtinSkills} { if d != "" { dirs = append(dirs, d) } } + if masterManagedDir := l.resolveManagedDir(context.Background()); masterManagedDir != "" { + dirs = append(dirs, masterManagedDir) + } return dirs } @@ -470,9 +588,11 @@ func (l *Loader) GetSkill(ctx context.Context, name string) (*Info, bool) { // Ensure cache is populated l.ListSkills(ctx) + tid := tenantCacheKey(ctx) + l.mu.RLock() defer l.mu.RUnlock() - info, ok := l.cache[name] + info, ok := l.cache[tid][name] return info, ok } diff --git a/internal/skills/loader_test.go b/internal/skills/loader_test.go index 5b0f4bbf..f8bdca11 100644 --- a/internal/skills/loader_test.go +++ b/internal/skills/loader_test.go @@ -6,6 +6,10 @@ import ( "path/filepath" "strings" "testing" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/store" ) func TestMain(m *testing.M) { @@ -522,7 +526,9 @@ func TestNormalizeLineEndings(t *testing.T) { // --- Managed skills versioning --- func TestLoader_ManagedSkills_LatestVersion(t *testing.T) { - managed := t.TempDir() + dataDir := t.TempDir() + // Master tenant's managed dir resolves to /skills-store (config.TenantSkillsStoreDir). + managed := filepath.Join(dataDir, "skills-store") // Create versioned structure: managed/my-skill/1/SKILL.md, managed/my-skill/2/SKILL.md os.MkdirAll(filepath.Join(managed, "my-skill", "1"), 0755) @@ -533,7 +539,7 @@ func TestLoader_ManagedSkills_LatestVersion(t *testing.T) { []byte("---\nname: My Skill v2\n---\nVersion 2"), 0644) l := NewLoader("", "", "") - l.SetManagedDir(managed) + l.SetManagedDir(dataDir) skills := l.ListSkills(context.Background()) if len(skills) != 1 { @@ -550,7 +556,8 @@ func TestLoader_ManagedSkills_LatestVersion(t *testing.T) { func TestLoader_ManagedSkills_WorkspaceTakesPriority(t *testing.T) { ws := t.TempDir() - managed := t.TempDir() + dataDir := t.TempDir() + managed := filepath.Join(dataDir, "skills-store") // Same slug in both workspace and managed makeSkillDir(t, filepath.Join(ws, "skills"), "shared-skill", "---\nname: Workspace Version\n---\n") @@ -559,7 +566,7 @@ func TestLoader_ManagedSkills_WorkspaceTakesPriority(t *testing.T) { []byte("---\nname: Managed Version\n---\n"), 0644) l := NewLoader(ws, "", "") - l.SetManagedDir(managed) + l.SetManagedDir(dataDir) skills := l.ListSkills(context.Background()) if len(skills) != 1 { @@ -570,6 +577,82 @@ func TestLoader_ManagedSkills_WorkspaceTakesPriority(t *testing.T) { } } +// TestLoader_ManagedSkills_TenantIsolation is the core security invariant test: +// two tenants each publish a managed skill with the SAME slug but DIFFERENT +// content. Resolving skills under tenant A's context must return ONLY A's +// content, never B's, and vice versa — proven via ListSkills, LoadSkill, and +// GetSkill (which is cache-backed, so this also proves the cache doesn't leak). +func TestLoader_ManagedSkills_TenantIsolation(t *testing.T) { + dataDir := t.TempDir() + + tenantA := uuid.MustParse("00000000-0000-0000-0000-0000000000aa") + tenantB := uuid.MustParse("00000000-0000-0000-0000-0000000000bb") + + // Master tenant dir: /skills-store///SKILL.md + masterDir := filepath.Join(dataDir, "skills-store", "shared-slug", "1") + os.MkdirAll(masterDir, 0755) + os.WriteFile(filepath.Join(masterDir, "SKILL.md"), + []byte("---\nname: Master Content\n---\nMASTER SECRET"), 0644) + + // Tenant A dir: /tenants//skills-store///SKILL.md + dirA := filepath.Join(dataDir, "tenants", tenantA.String(), "skills-store", "shared-slug", "1") + os.MkdirAll(dirA, 0755) + os.WriteFile(filepath.Join(dirA, "SKILL.md"), + []byte("---\nname: Tenant A Content\n---\nTENANT A SECRET"), 0644) + + // Tenant B dir: /tenants//skills-store///SKILL.md + dirB := filepath.Join(dataDir, "tenants", tenantB.String(), "skills-store", "shared-slug", "1") + os.MkdirAll(dirB, 0755) + os.WriteFile(filepath.Join(dirB, "SKILL.md"), + []byte("---\nname: Tenant B Content\n---\nTENANT B SECRET"), 0644) + + l := NewLoader("", "", "") + l.SetManagedDir(dataDir) + + ctxA := store.WithTenantID(context.Background(), tenantA) + ctxB := store.WithTenantID(context.Background(), tenantB) + ctxMaster := context.Background() + + // --- ListSkills: each tenant sees only its own content --- + skillsA := l.ListSkills(ctxA) + if len(skillsA) != 1 || skillsA[0].Name != "Tenant A Content" { + t.Fatalf("tenant A ListSkills leaked or missing: got %+v", skillsA) + } + skillsB := l.ListSkills(ctxB) + if len(skillsB) != 1 || skillsB[0].Name != "Tenant B Content" { + t.Fatalf("tenant B ListSkills leaked or missing: got %+v", skillsB) + } + skillsMaster := l.ListSkills(ctxMaster) + if len(skillsMaster) != 1 || skillsMaster[0].Name != "Master Content" { + t.Fatalf("master ListSkills leaked or missing: got %+v", skillsMaster) + } + + // --- LoadSkill: content bodies must not cross tenant boundaries --- + contentA, ok := l.LoadSkill(ctxA, "shared-slug") + if !ok || !strings.Contains(contentA, "TENANT A SECRET") || strings.Contains(contentA, "TENANT B SECRET") || strings.Contains(contentA, "MASTER SECRET") { + t.Fatalf("tenant A LoadSkill leaked cross-tenant content: %q", contentA) + } + contentB, ok := l.LoadSkill(ctxB, "shared-slug") + if !ok || !strings.Contains(contentB, "TENANT B SECRET") || strings.Contains(contentB, "TENANT A SECRET") || strings.Contains(contentB, "MASTER SECRET") { + t.Fatalf("tenant B LoadSkill leaked cross-tenant content: %q", contentB) + } + + // --- GetSkill: cache-backed lookup must remain tenant-scoped --- + infoA, ok := l.GetSkill(ctxA, "shared-slug") + if !ok || infoA.Name != "Tenant A Content" { + t.Fatalf("tenant A GetSkill leaked or missing: %+v", infoA) + } + infoB, ok := l.GetSkill(ctxB, "shared-slug") + if !ok || infoB.Name != "Tenant B Content" { + t.Fatalf("tenant B GetSkill leaked or missing: %+v", infoB) + } + // Re-check A again after B was populated — proves the cache didn't get overwritten. + infoA2, ok := l.GetSkill(ctxA, "shared-slug") + if !ok || infoA2.Name != "Tenant A Content" { + t.Fatalf("tenant A GetSkill got clobbered by tenant B lookup: %+v", infoA2) + } +} + // --- Dirs --- func TestParseSimpleYAMLLists(t *testing.T) { @@ -680,3 +763,86 @@ func TestLoader_Dirs(t *testing.T) { } } } + +// --- BuildPinnedSummary (inline content) --- + +// TestLoader_BuildPinnedSummary_InlinesFullContent proves a pinned skill's +// FULL SKILL.md body (not just name/description) appears in the summary. +func TestLoader_BuildPinnedSummary_InlinesFullContent(t *testing.T) { + ws := t.TempDir() + makeSkillDir(t, filepath.Join(ws, "skills"), "my-skill", + "---\nname: My Skill\ndescription: does a thing\n---\n\nTHIS IS THE FULL BODY CONTENT that must appear inline.") + + l := NewLoader(ws, "", "") + summary := l.BuildPinnedSummary(context.Background(), []string{"my-skill"}) + + if !strings.Contains(summary, "THIS IS THE FULL BODY CONTENT that must appear inline.") { + t.Fatalf("expected full body content inlined, got: %s", summary) + } + if !strings.Contains(summary, ``) { + t.Fatalf("expected wrapper, got: %s", summary) + } + if strings.Contains(summary, "") { + t.Fatalf("normal-sized skill should not fall back to pointer format: %s", summary) + } +} + +// TestLoader_BuildPinnedSummary_SizeCapFallback proves a skill whose body +// exceeds skillInlineMaxBytes falls back to pointer-only format instead of +// inlining the oversized content. +func TestLoader_BuildPinnedSummary_SizeCapFallback(t *testing.T) { + ws := t.TempDir() + bigBody := strings.Repeat("x", skillInlineMaxBytes+1) + makeSkillDir(t, filepath.Join(ws, "skills"), "big-skill", + "---\nname: Big Skill\ndescription: too large\n---\n\n"+bigBody) + + l := NewLoader(ws, "", "") + summary := l.BuildPinnedSummary(context.Background(), []string{"big-skill"}) + + if strings.Contains(summary, bigBody) { + t.Fatalf("oversized skill body must not be inlined: summary too long (%d bytes)", len(summary)) + } + if !strings.Contains(summary, "") || !strings.Contains(summary, "content too large to inline") { + t.Fatalf("expected pointer-only fallback with note, got: %s", summary) + } + if !strings.Contains(summary, "Big Skill") { + t.Fatalf("expected name in pointer fallback, got: %s", summary) + } +} + +// TestLoader_BuildPinnedSummary_TenantIsolation mirrors +// TestLoader_ManagedSkills_TenantIsolation: two tenants publish a managed +// pinned skill with the SAME slug but DIFFERENT content. The inlined content +// for tenant A must never leak tenant B's content, and vice versa. +func TestLoader_BuildPinnedSummary_TenantIsolation(t *testing.T) { + dataDir := t.TempDir() + + tenantA := uuid.MustParse("00000000-0000-0000-0000-0000000000aa") + tenantB := uuid.MustParse("00000000-0000-0000-0000-0000000000bb") + + dirA := filepath.Join(dataDir, "tenants", tenantA.String(), "skills-store", "shared-slug", "1") + os.MkdirAll(dirA, 0755) + os.WriteFile(filepath.Join(dirA, "SKILL.md"), + []byte("---\nname: Tenant A Content\n---\nTENANT A SECRET"), 0644) + + dirB := filepath.Join(dataDir, "tenants", tenantB.String(), "skills-store", "shared-slug", "1") + os.MkdirAll(dirB, 0755) + os.WriteFile(filepath.Join(dirB, "SKILL.md"), + []byte("---\nname: Tenant B Content\n---\nTENANT B SECRET"), 0644) + + l := NewLoader("", "", "") + l.SetManagedDir(dataDir) + + ctxA := store.WithTenantID(context.Background(), tenantA) + ctxB := store.WithTenantID(context.Background(), tenantB) + + summaryA := l.BuildPinnedSummary(ctxA, []string{"shared-slug"}) + if !strings.Contains(summaryA, "TENANT A SECRET") || strings.Contains(summaryA, "TENANT B SECRET") { + t.Fatalf("tenant A pinned summary leaked cross-tenant content: %q", summaryA) + } + + summaryB := l.BuildPinnedSummary(ctxB, []string{"shared-slug"}) + if !strings.Contains(summaryB, "TENANT B SECRET") || strings.Contains(summaryB, "TENANT A SECRET") { + t.Fatalf("tenant B pinned summary leaked cross-tenant content: %q", summaryB) + } +} diff --git a/internal/store/mcp_store.go b/internal/store/mcp_store.go index 0fd6fefa..a49ee46d 100644 --- a/internal/store/mcp_store.go +++ b/internal/store/mcp_store.go @@ -158,4 +158,22 @@ type MCPServerStore interface { GetUserCredentials(ctx context.Context, serverID uuid.UUID, userID string) (*MCPUserCredentials, error) SetUserCredentials(ctx context.Context, serverID uuid.UUID, userID string, creds MCPUserCredentials) error DeleteUserCredentials(ctx context.Context, serverID uuid.UUID, userID string) error + + // CacheToolDescriptions stores a map of tool name → cached tool info + // (description + real parameter schema, when available) into the + // server's settings JSONB under the "tool_cache" key. + CacheToolDescriptions(ctx context.Context, serverID uuid.UUID, toolInfo map[string]CachedToolInfo) error +} + +// CachedToolInfo is the cached, per-tool information stored under an MCP +// server's settings "tool_cache" key. It is used to render prompt previews +// without a live connection to the MCP server. +type CachedToolInfo struct { + // Description is the tool's human-readable description. + Description string `json:"description"` + // Parameters is the tool's full JSON Schema for its input parameters, + // captured from the live MCP connection at cache-write time. It may be + // nil for cache entries written before this field existed, or for + // servers whose tools genuinely expose no schema. + Parameters json.RawMessage `json:"parameters,omitempty"` } diff --git a/internal/store/pg/mcp_servers.go b/internal/store/pg/mcp_servers.go index bbbe3bd6..461dfd26 100644 --- a/internal/store/pg/mcp_servers.go +++ b/internal/store/pg/mcp_servers.go @@ -197,6 +197,28 @@ func (s *PGMCPServerStore) DeleteServer(ctx context.Context, id uuid.UUID) error return err } +// CacheToolDescriptions stores a map of tool name → cached tool info +// (description + parameter schema) into the server's settings JSONB under +// the "tool_cache" key using jsonb_set(). +func (s *PGMCPServerStore) CacheToolDescriptions(ctx context.Context, serverID uuid.UUID, toolInfo map[string]store.CachedToolInfo) error { + descJSON, err := json.Marshal(toolInfo) + if err != nil { + return fmt.Errorf("marshal tool descriptions: %w", err) + } + + query := ` + UPDATE mcp_servers + SET settings = jsonb_set(settings, '{tool_cache}', $1::jsonb, true), + updated_at = NOW() + WHERE id = $2 + ` + _, err = s.db.ExecContext(ctx, query, string(descJSON), serverID) + if err != nil { + return fmt.Errorf("mcp_servers.cache_tool_descriptions: %w", err) + } + return nil +} + // encryptJSONB encrypts a JSONB blob (env, headers) by converting it to a JSON string literal. // Unencrypted: {"key":"val"} (JSONB object). Encrypted: "aes-gcm:..." (JSONB string). func (s *PGMCPServerStore) encryptJSONB(data []byte) []byte { diff --git a/internal/store/sqlitestore/mcp_servers.go b/internal/store/sqlitestore/mcp_servers.go index fae026f9..b2185d68 100644 --- a/internal/store/sqlitestore/mcp_servers.go +++ b/internal/store/sqlitestore/mcp_servers.go @@ -196,6 +196,41 @@ func (s *SQLiteMCPServerStore) DeleteServer(ctx context.Context, id uuid.UUID) e return err } +// CacheToolDescriptions stores a map of tool name → cached tool info +// (description + parameter schema) into the server's settings JSON under +// the "tool_cache" key. +// SQLite has no jsonb_set(); we read-modify-write the settings column instead. +func (s *SQLiteMCPServerStore) CacheToolDescriptions(ctx context.Context, serverID uuid.UUID, toolInfo map[string]store.CachedToolInfo) error { + row := s.db.QueryRowContext(ctx, `SELECT COALESCE(settings, '{}') FROM mcp_servers WHERE id = ?`, serverID) + var rawSettings []byte + if err := row.Scan(&rawSettings); err != nil { + return fmt.Errorf("mcp_servers.cache_tool_descriptions read: %w", err) + } + + var settings map[string]json.RawMessage + if err := json.Unmarshal(rawSettings, &settings); err != nil { + settings = make(map[string]json.RawMessage) + } + + cacheJSON, err := json.Marshal(toolInfo) + if err != nil { + return fmt.Errorf("marshal tool descriptions: %w", err) + } + settings["tool_cache"] = json.RawMessage(cacheJSON) + + merged, err := json.Marshal(settings) + if err != nil { + return fmt.Errorf("marshal settings: %w", err) + } + + _, err = s.db.ExecContext(ctx, `UPDATE mcp_servers SET settings = ?, updated_at = ? WHERE id = ?`, + string(merged), time.Now().UTC(), serverID) + if err != nil { + return fmt.Errorf("mcp_servers.cache_tool_descriptions write: %w", err) + } + return nil +} + // encryptJSON encrypts a JSON blob by wrapping ciphertext as a JSON string. // Unencrypted: {"key":"val"} (JSON object). Encrypted: "aes-gcm:..." (JSON string). func (s *SQLiteMCPServerStore) encryptJSON(data []byte) []byte { diff --git a/internal/tools/context_keys.go b/internal/tools/context_keys.go index 9af2b8e7..5c79681a 100644 --- a/internal/tools/context_keys.go +++ b/internal/tools/context_keys.go @@ -30,8 +30,9 @@ const ( ctxAsyncCB toolContextKey = "tool_async_cb" ctxWorkspace toolContextKey = "tool_workspace" ctxAgentKey toolContextKey = "tool_agent_key" - ctxSessionKey toolContextKey = "tool_session_key" // origin session key for announce routing - ctxRunKind toolContextKey = "tool_run_kind" // "notification", "announce", "delegation" + ctxAgentPolicy toolContextKey = "tool_agent_policy" // per-agent tool policy for MCP bridge enforcement + ctxSessionKey toolContextKey = "tool_session_key" // origin session key for announce routing + ctxRunKind toolContextKey = "tool_run_kind" // "notification", "announce", "delegation" ) // ctxRateLimitOverride carries a per-agent tool rate limit (calls/hour) that @@ -152,6 +153,22 @@ func ToolAgentKeyFromCtx(ctx context.Context) string { return "" } +// WithToolAgentPolicy injects the calling agent's per-agent tool policy into +// context, so the MCP bridge server (internal/mcp/bridge_server.go) can +// enforce the same policy-filtered tool allowlist the normal agent loop uses, +// instead of exposing its full BridgeToolNames set unconditionally to every +// caller regardless of agent identity. +func WithToolAgentPolicy(ctx context.Context, policy *config.ToolPolicySpec) context.Context { + return context.WithValue(ctx, ctxAgentPolicy, policy) +} + +// ToolAgentPolicyFromCtx returns the calling agent's per-agent tool policy, or +// nil if none was injected (e.g. no verified agent context on the request). +func ToolAgentPolicyFromCtx(ctx context.Context) *config.ToolPolicySpec { + policy, _ := ctx.Value(ctxAgentPolicy).(*config.ToolPolicySpec) + return policy +} + // WithToolSessionKey injects the parent's session key so subagent announce // can route results back to the exact same session (required for WS where // session keys don't follow BuildScopedSessionKey format). diff --git a/internal/tools/policy.go b/internal/tools/policy.go index 3a054214..4c35ae8c 100644 --- a/internal/tools/policy.go +++ b/internal/tools/policy.go @@ -78,12 +78,34 @@ var leafSubagentDenyList = []string{ "sessions_list", "sessions_history", "spawn", } +// registryUnwrapper is implemented by ToolExecutor wrappers (e.g. userToolOverlay) +// that expose the concrete *Registry they wrap, so group-expansion machinery +// (which needs concrete *Registry, not the ToolExecutor interface) still works +// when the caller passes a request-scoped overlay instead of the raw registry. +type registryUnwrapper interface { + Unwrap() *Registry +} + +// ResolveConcreteRegistry extracts the concrete *Registry backing a ToolExecutor, +// for use by group-expansion/capability/deny helpers that need direct access to +// registry-internal tool groups and metadata. Returns nil when the executor is +// neither a *Registry nor a registryUnwrapper (e.g. a test mock) — callers must +// tolerate nil (group expansion falls back to a no-op pass-through). +func ResolveConcreteRegistry(executor ToolExecutor) *Registry { + if reg, ok := executor.(*Registry); ok { + return reg + } + if unwrapper, ok := executor.(registryUnwrapper); ok { + return unwrapper.Unwrap() + } + return nil +} + // PolicyEngine evaluates tool access based on layered config policies. type PolicyEngine struct { globalPolicy *config.ToolsConfig - mu sync.RWMutex // protects denyCapabilities + registry + mu sync.RWMutex // protects denyCapabilities denyCapabilities []ToolCapability // capability-based deny rules (v3) - registry *Registry // for metadata lookups (nil = skip capability checks) } // NewPolicyEngine creates a policy engine from global config. @@ -91,13 +113,6 @@ func NewPolicyEngine(cfg *config.ToolsConfig) *PolicyEngine { return &PolicyEngine{globalPolicy: cfg} } -// SetRegistry enables capability-based filtering by providing metadata lookups. -func (pe *PolicyEngine) SetRegistry(r *Registry) { - pe.mu.Lock() - defer pe.mu.Unlock() - pe.registry = r -} - // DenyCapability adds a capability to the deny list. // Tools with this capability are excluded from FilterTools results. func (pe *PolicyEngine) DenyCapability(cap ToolCapability) { @@ -117,16 +132,16 @@ func (pe *PolicyEngine) FilterTools( isSubagent bool, isLeafAgent bool, ) []providers.ToolDefinition { + reg := ResolveConcreteRegistry(registry) allTools := registry.List() - allowed := pe.evaluate(allTools, providerName, agentToolPolicy, groupToolAllow) + allowed := pe.evaluate(reg, allTools, providerName, agentToolPolicy, groupToolAllow) // Step 8: Capability-based deny (v3 RBAC) pe.mu.RLock() denyCaps := pe.denyCapabilities - capReg := pe.registry pe.mu.RUnlock() - if len(denyCaps) > 0 && capReg != nil { - allowed = filterByCapability(allowed, denyCaps, capReg) + if len(denyCaps) > 0 && reg != nil { + allowed = filterByCapability(allowed, denyCaps, reg) } // Apply subagent restrictions @@ -185,7 +200,13 @@ func (pe *PolicyEngine) FilterTools( } // evaluate runs the 7-step policy pipeline. +// reg is the concrete *Registry backing the caller's ToolExecutor (may be nil +// when the caller passed a mock/test executor with no group data), used for +// group-expansion ("group:xxx") and deny-spec matching. It is threaded through +// as an explicit parameter — never stored on pe — because pe is a shared, +// concurrently-used singleton; mutating a field per-call would be a data race. func (pe *PolicyEngine) evaluate( + reg *Registry, allTools []string, providerName string, agentToolPolicy *config.ToolPolicySpec, @@ -193,18 +214,13 @@ func (pe *PolicyEngine) evaluate( ) []string { g := pe.globalPolicy - // Get registry for group expansion (may be nil in early boot) - pe.mu.RLock() - reg := pe.registry - pe.mu.RUnlock() - // Step 1: Global profile - allowed := pe.applyProfile(allTools, g.Profile) + allowed := pe.applyProfile(reg, allTools, g.Profile) // Step 2: Provider-level profile override if g.ByProvider != nil { if pp, ok := g.ByProvider[providerName]; ok && pp.Profile != "" { - allowed = pe.applyProfile(allTools, pp.Profile) + allowed = pe.applyProfile(reg, allTools, pp.Profile) } } @@ -255,12 +271,23 @@ func (pe *PolicyEngine) evaluate( allowed = unionWithSpec(reg, allowed, allTools, agentToolPolicy.AlsoAllow) } + // Deny always wins: re-apply the same deny specs as a final step so that + // AlsoAllow can never reintroduce a tool (or a group containing it) that + // was explicitly denied above. Without this, unionWithSpec adds tools back + // from allTools without re-checking the deny list. + if len(g.Deny) > 0 { + allowed = subtractSpec(reg, allowed, g.Deny) + } + if agentToolPolicy != nil && len(agentToolPolicy.Deny) > 0 { + allowed = subtractSpec(reg, allowed, agentToolPolicy.Deny) + } + return allowed } // applyProfile returns tools allowed by a named profile. // "full" or empty profile = all tools allowed. -func (pe *PolicyEngine) applyProfile(allTools []string, profile string) []string { +func (pe *PolicyEngine) applyProfile(reg *Registry, allTools []string, profile string) []string { if profile == "" || profile == "full" { return copySlice(allTools) } @@ -271,11 +298,6 @@ func (pe *PolicyEngine) applyProfile(allTools []string, profile string) []string return copySlice(allTools) } - // Get registry for group expansion - pe.mu.RLock() - reg := pe.registry - pe.mu.RUnlock() - return expandSpec(reg, allTools, spec) } @@ -438,12 +460,14 @@ func unionWithSpec(reg *Registry, current []string, allTools []string, spec []st // bypass registration in the shared registry to prevent credential leaks, but // still need to respect the agent's tool policy. // -// It works by running evaluate() with just [name] as the available set. -// With a "full" profile and no allow restrictions, the name survives. -// With a restrictive allow list that excludes MCP tools, it's removed. -// With a deny containing group:mcp, MatchDenySpec removes it from the set. -func (pe *PolicyEngine) WouldAllow(name, providerName string, agentPolicy *config.ToolPolicySpec, groupAllow []string) bool { - allowed := pe.evaluate([]string{name}, providerName, agentPolicy, groupAllow) +// reg is the concrete *Registry to use for group-expansion/deny-spec matching +// (pass nil to fall back to a no-group-expansion match, e.g. from a caller that +// only has a ToolExecutor). It works by running evaluate() with just [name] as +// the available set. With a "full" profile and no allow restrictions, the name +// survives. With a restrictive allow list that excludes MCP tools, it's +// removed. With a deny containing group:mcp, MatchDenySpec removes it from the set. +func (pe *PolicyEngine) WouldAllow(reg *Registry, name, providerName string, agentPolicy *config.ToolPolicySpec, groupAllow []string) bool { + allowed := pe.evaluate(reg, []string{name}, providerName, agentPolicy, groupAllow) for _, a := range allowed { if a == name { return true @@ -455,15 +479,13 @@ func (pe *PolicyEngine) WouldAllow(name, providerName string, agentPolicy *confi // IsDenied checks if a tool name is explicitly denied by global or agent policy. // Used to prevent lazy-activated deferred tools from bypassing the deny list. // Checks under all candidate names: the raw name, the legacy alias (e.g. bash→exec), -// and the registry alias when available. -func (pe *PolicyEngine) IsDenied(name string, agentPolicy *config.ToolPolicySpec) bool { +// and the registry alias when available. reg is the concrete *Registry to use for +// group-expansion/deny-spec matching (nil is tolerated — falls back to a plain match). +func (pe *PolicyEngine) IsDenied(reg *Registry, name string, agentPolicy *config.ToolPolicySpec) bool { candidates := map[string]struct{}{name: {}} // Keep legacy alias compatibility (e.g. bash -> exec). candidates[resolveAlias(name)] = struct{}{} // Include registry alias mapping when available. - pe.mu.RLock() - reg := pe.registry - pe.mu.RUnlock() if reg != nil { if canonical, ok := reg.Aliases()[name]; ok && canonical != "" { candidates[canonical] = struct{}{} diff --git a/internal/tools/policy_deny_wins_test.go b/internal/tools/policy_deny_wins_test.go new file mode 100644 index 00000000..eac13837 --- /dev/null +++ b/internal/tools/policy_deny_wins_test.go @@ -0,0 +1,73 @@ +package tools + +// Tests proving deny always wins over AlsoAllow: previously AlsoAllow's +// unionWithSpec call ran AFTER Deny with no re-check, so a tool explicitly +// denied but also reachable via a "group:x" spec in AlsoAllow was silently +// reintroduced into the allowed set. See internal/tools/policy.go evaluate(). + +import ( + "context" + "testing" + + "github.com/nextlevelbuilder/goclaw/internal/config" +) + +// denyWinsMockTool is a minimal Tool for populating a Registry in tests. +type denyWinsMockTool struct{ name string } + +func (m *denyWinsMockTool) Name() string { return m.name } +func (m *denyWinsMockTool) Description() string { return "mock " + m.name } +func (m *denyWinsMockTool) Parameters() map[string]any { return map[string]any{"type": "object"} } +func (m *denyWinsMockTool) Execute(context.Context, map[string]any) *Result { + return &Result{ForLLM: "ok"} +} + +// TestFilterTools_DenyWinsOverAlsoAllowGroup proves that a tool explicitly +// listed in Deny is NOT reintroduced by an AlsoAllow spec that references a +// group containing that same tool. +func TestFilterTools_DenyWinsOverAlsoAllowGroup(t *testing.T) { + reg := NewRegistry() + reg.Register(&denyWinsMockTool{name: "mcp_svc__get_data"}) + reg.Register(&denyWinsMockTool{name: "mcp_svc__put_data"}) + reg.RegisterToolGroup("mcp", []string{"mcp_svc__get_data", "mcp_svc__put_data"}) + + pe := NewPolicyEngine(&config.ToolsConfig{ + Deny: []string{"mcp_svc__get_data"}, + AlsoAllow: []string{"group:mcp"}, + }) + + defs := pe.FilterTools(reg, "agent1", "openai", nil, nil, false, false) + names := defNameSetGE(defs) + + if names["mcp_svc__get_data"] { + t.Errorf("explicitly denied tool must NOT be reintroduced via AlsoAllow group expansion; got %v", names) + } + if !names["mcp_svc__put_data"] { + t.Errorf("non-denied tool in the same AlsoAllow group must still be allowed; got %v", names) + } +} + +// TestFilterTools_DenyWinsOverAlsoAllowGroup_AgentPolicy proves the same +// deny-wins guarantee for the per-agent Deny/AlsoAllow lists. +func TestFilterTools_DenyWinsOverAlsoAllowGroup_AgentPolicy(t *testing.T) { + reg := NewRegistry() + reg.Register(&denyWinsMockTool{name: "mcp_svc__get_data"}) + reg.Register(&denyWinsMockTool{name: "mcp_svc__put_data"}) + reg.RegisterToolGroup("mcp", []string{"mcp_svc__get_data", "mcp_svc__put_data"}) + + pe := NewPolicyEngine(&config.ToolsConfig{}) + agentPolicy := &config.ToolPolicySpec{ + Deny: []string{"mcp_svc__get_data"}, + AlsoAllow: []string{"group:mcp"}, + } + + defs := pe.FilterTools(reg, "agent1", "openai", agentPolicy, nil, false, false) + names := defNameSetGE(defs) + + if names["mcp_svc__get_data"] { + t.Errorf("agent-level denied tool must NOT be reintroduced via agent-level AlsoAllow group expansion; got %v", names) + } + if !names["mcp_svc__put_data"] { + t.Errorf("non-denied tool in the same AlsoAllow group must still be allowed; got %v", names) + } +} diff --git a/internal/tools/policy_group_expansion_test.go b/internal/tools/policy_group_expansion_test.go new file mode 100644 index 00000000..7cfbf3b2 --- /dev/null +++ b/internal/tools/policy_group_expansion_test.go @@ -0,0 +1,133 @@ +package tools + +// Tests proving the group-expansion fix: PolicyEngine.registry was previously a +// field only set via SetRegistry() (never called in production, since PolicyEngine +// is a shared singleton and mutating a field per-call would be a data race across +// concurrent requests). That meant every "group:xxx" spec entry silently expanded +// to nothing in production. The fix threads the concrete *Registry through as an +// explicit parameter (derived from FilterTools' registry argument), with no +// SetRegistry() call required — these tests exercise exactly that path. + +import ( + "context" + "testing" + + "github.com/nextlevelbuilder/goclaw/internal/config" + "github.com/nextlevelbuilder/goclaw/internal/providers" +) + +// groupExpansionMockTool is a minimal Tool for populating a Registry in tests. +type groupExpansionMockTool struct{ name string } + +func (m *groupExpansionMockTool) Name() string { return m.name } +func (m *groupExpansionMockTool) Description() string { return "mock " + m.name } +func (m *groupExpansionMockTool) Parameters() map[string]any { return map[string]any{"type": "object"} } +func (m *groupExpansionMockTool) Execute(context.Context, map[string]any) *Result { + return &Result{ForLLM: "ok"} +} + +// TestFilterTools_GroupExpansion_PlainRegistry proves that a "group:mcp" allow +// spec expands to the actual tool names registered in a plain *Registry passed +// directly to FilterTools, with NO SetRegistry() call ever made — matching the +// production code path (toolPE.SetRegistry was never called anywhere). +func TestFilterTools_GroupExpansion_PlainRegistry(t *testing.T) { + reg := NewRegistry() + reg.Register(&groupExpansionMockTool{name: "mcp_svc__get_data"}) + reg.Register(&groupExpansionMockTool{name: "mcp_svc__put_data"}) + reg.Register(&groupExpansionMockTool{name: "unrelated_tool"}) + reg.RegisterToolGroup("mcp", []string{"mcp_svc__get_data", "mcp_svc__put_data"}) + + pe := NewPolicyEngine(&config.ToolsConfig{ + Allow: []string{"group:mcp"}, + }) + + defs := pe.FilterTools(reg, "agent1", "openai", nil, nil, false, false) + names := defNameSetGE(defs) + + if !names["mcp_svc__get_data"] || !names["mcp_svc__put_data"] { + t.Errorf("group:mcp allow spec must expand to registered group members; got %v", names) + } + if names["unrelated_tool"] { + t.Errorf("tool outside group:mcp must not be allowed via group expansion; got %v", names) + } +} + +// TestFilterTools_GroupExpansion_UserToolOverlay proves the same group-expansion +// behavior works when FilterTools is called with a userToolOverlay wrapping a +// *Registry (the loop_tool_filter.go production path when an actor has per-user +// MCP tools) — proving the Unwrap() fallback resolves the concrete registry. +func TestFilterTools_GroupExpansion_UserToolOverlay(t *testing.T) { + reg := NewRegistry() + reg.Register(&groupExpansionMockTool{name: "mcp_svc__get_data"}) + reg.RegisterToolGroup("mcp", []string{"mcp_svc__get_data", "mcp_bx24__execute"}) + + ov := NewUserToolOverlay(reg, []Tool{ + &groupExpansionMockTool{name: "mcp_bx24__execute"}, + }) + + pe := NewPolicyEngine(&config.ToolsConfig{ + Allow: []string{"group:mcp"}, + }) + + defs := pe.FilterTools(ov, "agent1", "openai", nil, nil, false, false) + names := defNameSetGE(defs) + + if !names["mcp_svc__get_data"] { + t.Errorf("group:mcp must still expand base registry tools through the overlay; got %v", names) + } + if !names["mcp_bx24__execute"] { + t.Errorf("group:mcp must expand to include the per-user overlay tool that is a group member; got %v", names) + } +} + +// TestFilterTools_GroupExpansion_DenyGroup proves "group:xxx" also expands +// correctly on the deny side (subtractSpec) without any SetRegistry() call. +func TestFilterTools_GroupExpansion_DenyGroup(t *testing.T) { + reg := NewRegistry() + reg.Register(&groupExpansionMockTool{name: "mcp_svc__get_data"}) + reg.Register(&groupExpansionMockTool{name: "safe_tool"}) + reg.RegisterToolGroup("mcp", []string{"mcp_svc__get_data"}) + + pe := NewPolicyEngine(&config.ToolsConfig{ + Deny: []string{"group:mcp"}, + }) + + defs := pe.FilterTools(reg, "agent1", "openai", nil, nil, false, false) + names := defNameSetGE(defs) + + if names["mcp_svc__get_data"] { + t.Errorf("group:mcp deny spec must expand and exclude group members; got %v", names) + } + if !names["safe_tool"] { + t.Errorf("tool outside denied group must remain allowed; got %v", names) + } +} + +// TestPolicyEngine_WouldAllow_GroupExpansion proves WouldAllow (used by the MCP +// bridge server) also correctly expands group specs when passed a concrete +// *Registry, without any SetRegistry() call. +func TestPolicyEngine_WouldAllow_GroupExpansion(t *testing.T) { + reg := NewRegistry() + reg.RegisterToolGroup("mcp", []string{"mcp_svc__get_data"}) + + pe := NewPolicyEngine(&config.ToolsConfig{ + Deny: []string{"group:mcp"}, + }) + + if pe.WouldAllow(reg, "mcp_svc__get_data", "claude-cli", nil, nil) { + t.Error("WouldAllow must respect group:mcp deny expansion") + } + if !pe.WouldAllow(reg, "other_tool", "claude-cli", nil, nil) { + t.Error("WouldAllow must allow a tool outside the denied group") + } +} + +func defNameSetGE(defs []providers.ToolDefinition) map[string]bool { + m := make(map[string]bool, len(defs)) + for _, d := range defs { + if d.Function != nil { + m[d.Function.Name] = true + } + } + return m +} diff --git a/internal/tools/skill_search.go b/internal/tools/skill_search.go index ec2e55c7..e4797f16 100644 --- a/internal/tools/skill_search.go +++ b/internal/tools/skill_search.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "log/slog" + "sync" "github.com/google/uuid" @@ -12,12 +13,21 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/store" ) +// tenantIndexState holds a per-tenant BM25 index and the loader version +// it was built against, so tenants never share (or clobber) each other's +// cached search index. +type tenantIndexState struct { + index *skills.Index + lastVersion int64 // tracks loader version for lazy rebuild +} + // SkillSearchTool implements the skill_search tool with BM25 search // and optional hybrid search (BM25 + embedding). type SkillSearchTool struct { - index *skills.Index - loader *skills.Loader - lastVersion int64 // tracks loader version for lazy rebuild + mu sync.Mutex + indexes map[uuid.UUID]*tenantIndexState // keyed by tenant ID (uuid.Nil = master/no-tenant) + + loader *skills.Loader // Optional: embedding-based search embSearcher store.EmbeddingSkillSearcher @@ -29,11 +39,10 @@ type SkillSearchTool struct { skillAccess store.SkillAccessStore } -// NewSkillSearchTool creates a skill_search tool backed by a BM25 index. +// NewSkillSearchTool creates a skill_search tool backed by per-tenant BM25 indexes. func NewSkillSearchTool(loader *skills.Loader) *SkillSearchTool { - idx := skills.NewIndex() - t := &SkillSearchTool{index: idx, loader: loader} - t.rebuildIndex(context.Background()) + t := &SkillSearchTool{indexes: make(map[uuid.UUID]*tenantIndexState), loader: loader} + t.ensureIndex(context.Background()) return t } @@ -48,20 +57,33 @@ func (t *SkillSearchTool) SetSkillAccessStore(sas store.SkillAccessStore) { t.skillAccess = sas } -// rebuildIndex refreshes the BM25 index from the current skill set. -func (t *SkillSearchTool) rebuildIndex(ctx context.Context) { - allSkills := t.loader.ListSkills(ctx) - t.index.Build(allSkills) - t.lastVersion = t.loader.Version() - slog.Info("skill_search index rebuilt", "docs", len(allSkills), "version", t.lastVersion) -} - -// ensureIndex rebuilds the BM25 index if skills have changed since last build. -func (t *SkillSearchTool) ensureIndex(ctx context.Context) { +// ensureIndex returns the BM25 index for the tenant resolved from ctx, +// building or rebuilding it if it doesn't exist yet or skills have changed +// since it was last built. Each tenant's index is isolated in its own +// map entry so search results never leak across tenants. +func (t *SkillSearchTool) ensureIndex(ctx context.Context) *skills.Index { + tenantID := store.TenantIDFromContext(ctx) current := t.loader.Version() - if current > t.lastVersion { - t.rebuildIndex(ctx) + + t.mu.Lock() + defer t.mu.Unlock() + + state, ok := t.indexes[tenantID] + if !ok { + state = &tenantIndexState{index: skills.NewIndex(), lastVersion: -1} + t.indexes[tenantID] = state } + + if state.lastVersion >= current { + return state.index + } + + allSkills := t.loader.ListSkills(ctx) + state.index.Build(allSkills) + state.lastVersion = current + slog.Info("skill_search index rebuilt", "docs", len(allSkills), "version", current, "tenant", tenantID) + + return state.index } func (t *SkillSearchTool) Name() string { return "skill_search" } @@ -98,11 +120,11 @@ func (t *SkillSearchTool) Execute(ctx context.Context, args map[string]any) *Res maxResults = int(mr) } - // Lazy rebuild: check if skills changed since last index build - t.ensureIndex(ctx) + // Lazy rebuild: resolve the tenant-scoped index, rebuilding if skills changed + index := t.ensureIndex(ctx) // BM25 search (always available) - bm25Results := t.index.Search(query, maxResults*2) + bm25Results := index.Search(query, maxResults*2) // If embedding searcher is available, run hybrid search var results []skills.SkillSearchResult diff --git a/internal/tools/skill_search_test.go b/internal/tools/skill_search_test.go new file mode 100644 index 00000000..64d5b513 --- /dev/null +++ b/internal/tools/skill_search_test.go @@ -0,0 +1,81 @@ +package tools + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/skills" + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +// TestSkillSearchTool_TenantIsolation mirrors +// TestLoader_ManagedSkills_TenantIsolation (internal/skills/loader_test.go): +// two tenants each have a managed skill with the same slug but different, +// tenant-secret content. Execute() called under tenant A's context must +// never return tenant B's content, and vice versa, including after tenant +// B has already been searched (proving the per-tenant index cache doesn't +// get clobbered by a later, different-tenant call). +func TestSkillSearchTool_TenantIsolation(t *testing.T) { + dataDir := t.TempDir() + + tenantA := uuid.MustParse("00000000-0000-0000-0000-0000000000aa") + tenantB := uuid.MustParse("00000000-0000-0000-0000-0000000000bb") + + dirA := filepath.Join(dataDir, "tenants", tenantA.String(), "skills-store", "shared-slug", "1") + if err := os.MkdirAll(dirA, 0755); err != nil { + t.Fatalf("mkdir tenant A: %v", err) + } + if err := os.WriteFile(filepath.Join(dirA, "SKILL.md"), + []byte("---\nname: Tenant A Skill\ndescription: alpha rendering skill\n---\nTENANT A SECRET"), 0644); err != nil { + t.Fatalf("write tenant A skill: %v", err) + } + + dirB := filepath.Join(dataDir, "tenants", tenantB.String(), "skills-store", "shared-slug", "1") + if err := os.MkdirAll(dirB, 0755); err != nil { + t.Fatalf("mkdir tenant B: %v", err) + } + if err := os.WriteFile(filepath.Join(dirB, "SKILL.md"), + []byte("---\nname: Tenant B Skill\ndescription: alpha rendering skill\n---\nTENANT B SECRET"), 0644); err != nil { + t.Fatalf("write tenant B skill: %v", err) + } + + loader := skills.NewLoader("", "", "") + loader.SetManagedDir(dataDir) + + tool := NewSkillSearchTool(loader) + + ctxA := store.WithTenantID(context.Background(), tenantA) + ctxB := store.WithTenantID(context.Background(), tenantB) + + execAndDump := func(ctx context.Context) string { + res := tool.Execute(ctx, map[string]any{"query": "alpha rendering"}) + if res == nil { + t.Fatal("Execute returned nil result") + } + return res.ForLLM + } + + // First search under tenant A. + outA := execAndDump(ctxA) + if !strings.Contains(outA, "Tenant A Skill") || strings.Contains(outA, "Tenant B Skill") { + t.Fatalf("tenant A search leaked or missing tenant A content: %q", outA) + } + + // Now search under tenant B — must never see tenant A's content. + outB := execAndDump(ctxB) + if !strings.Contains(outB, "Tenant B Skill") || strings.Contains(outB, "Tenant A Skill") { + t.Fatalf("tenant B search leaked or missing tenant B content: %q", outB) + } + + // Re-check tenant A AFTER tenant B was searched, proving the per-tenant + // cache entry was not clobbered by the intervening tenant B lookup. + outA2 := execAndDump(ctxA) + if !strings.Contains(outA2, "Tenant A Skill") || strings.Contains(outA2, "Tenant B Skill") { + t.Fatalf("tenant A re-check leaked or missing tenant A content: %q", outA2) + } +} diff --git a/internal/tools/user_tool_overlay.go b/internal/tools/user_tool_overlay.go index 8a5cec29..279aff13 100644 --- a/internal/tools/user_tool_overlay.go +++ b/internal/tools/user_tool_overlay.go @@ -71,5 +71,17 @@ func (o *userToolOverlay) List() []string { return out } +// Unwrap returns the concrete *Registry backing this overlay's base executor, +// so PolicyEngine group-expansion (which needs the concrete type to read +// per-Registry tool groups) still works when a caller passes an overlay +// instead of the raw registry. Returns nil if the base isn't a *Registry +// (e.g. in tests using a mock ToolExecutor) — callers must tolerate nil. +func (o *userToolOverlay) Unwrap() *Registry { + if reg, ok := o.ToolExecutor.(*Registry); ok { + return reg + } + return nil +} + // Compile-time check: the overlay satisfies ToolExecutor. var _ ToolExecutor = (*userToolOverlay)(nil) diff --git a/ui/web/src/i18n/locales/en/agents.json b/ui/web/src/i18n/locales/en/agents.json index 2b2fd9f5..3b92f0d1 100644 --- a/ui/web/src/i18n/locales/en/agents.json +++ b/ui/web/src/i18n/locales/en/agents.json @@ -623,7 +623,12 @@ "insertContact": "Search contact to insert...", "contentPlaceholder": "File content...", "systemPromptPreview": "System Prompt", - "tokens": "tokens" + "tokens": "tokens", + "toolsAvailable": "{{count}} tools available", + "toolsSection": "Tools", + "toolParameters": "Parameters", + "noParameters": "No parameters", + "noTools": "No tools available" }, "fileEditor": { "editWithAi": "Edit with AI", diff --git a/ui/web/src/i18n/locales/vi/agents.json b/ui/web/src/i18n/locales/vi/agents.json index c03c3b86..18bd075e 100644 --- a/ui/web/src/i18n/locales/vi/agents.json +++ b/ui/web/src/i18n/locales/vi/agents.json @@ -608,7 +608,12 @@ "insertContact": "Tìm liên hệ để chèn...", "contentPlaceholder": "Nội dung tệp...", "systemPromptPreview": "System Prompt", - "tokens": "tokens" + "tokens": "tokens", + "toolsAvailable": "{{count}} công cụ khả dụng", + "toolsSection": "Công cụ", + "toolParameters": "Tham số", + "noParameters": "Không có tham số", + "noTools": "Không có công cụ nào" }, "fileEditor": { "editWithAi": "Chỉnh sửa với AI", diff --git a/ui/web/src/i18n/locales/zh/agents.json b/ui/web/src/i18n/locales/zh/agents.json index 0e94f420..91087fd6 100644 --- a/ui/web/src/i18n/locales/zh/agents.json +++ b/ui/web/src/i18n/locales/zh/agents.json @@ -608,7 +608,12 @@ "insertContact": "搜索联系人以插入...", "contentPlaceholder": "文件内容...", "systemPromptPreview": "系统提示词", - "tokens": "tokens" + "tokens": "tokens", + "toolsAvailable": "{{count}} 个可用工具", + "toolsSection": "工具", + "toolParameters": "参数", + "noParameters": "无参数", + "noTools": "没有可用工具" }, "fileEditor": { "editWithAi": "AI 编辑", diff --git a/ui/web/src/pages/agents/agent-detail/file-sections/system-prompt-preview.tsx b/ui/web/src/pages/agents/agent-detail/file-sections/system-prompt-preview.tsx index 1919c262..7bc5aa5b 100644 --- a/ui/web/src/pages/agents/agent-detail/file-sections/system-prompt-preview.tsx +++ b/ui/web/src/pages/agents/agent-detail/file-sections/system-prompt-preview.tsx @@ -5,6 +5,7 @@ import { Eye, Loader2 } from "lucide-react"; import { useHttp } from "@/hooks/use-ws"; import { queryKeys } from "@/lib/query-keys"; import { cn } from "@/lib/utils"; +import { ToolsPreviewSection, type ToolDefinition } from "./tools-preview-section"; const MODES = ["full", "task", "minimal", "none"] as const; type PromptMode = (typeof MODES)[number]; @@ -14,6 +15,7 @@ interface PreviewResponse { prompt: string; token_count: number; sections: { name: string; start: number; end: number }[]; + tools?: ToolDefinition[]; } interface SystemPromptPreviewProps { @@ -84,9 +86,12 @@ export function SystemPromptPreview({ agentKey }: SystemPromptPreviewProps) { ) : error ? (

{error}

) : data ? ( -
-            {renderPromptWithBoundary(data.prompt)}
-          
+ <> +
+              {renderPromptWithBoundary(data.prompt)}
+            
+ + ) : null} diff --git a/ui/web/src/pages/agents/agent-detail/file-sections/tools-preview-section.test.tsx b/ui/web/src/pages/agents/agent-detail/file-sections/tools-preview-section.test.tsx new file mode 100644 index 00000000..00c505f3 --- /dev/null +++ b/ui/web/src/pages/agents/agent-detail/file-sections/tools-preview-section.test.tsx @@ -0,0 +1,43 @@ +/** + * Unit tests for tools-preview-section. + * + * NOTE: @testing-library/react is not installed in this project — tests + * cover module contracts rather than DOM rendering (mirrors + * voice-picker.test.tsx / stt-provider-form.test.tsx pattern). + */ +import { describe, it, expect } from "vitest"; +import { ToolsPreviewSection, type ToolDefinition } from "./tools-preview-section"; + +describe("ToolsPreviewSection", () => { + it("is defined", () => { + expect(ToolsPreviewSection).toBeDefined(); + }); + + it("accepts a well-formed ToolDefinition list", () => { + const tools: ToolDefinition[] = [ + { + type: "function", + function: { + name: "read_file", + description: "Reads a file from disk", + parameters: { type: "object", properties: { path: { type: "string" } } }, + }, + }, + { + type: "function", + function: { + name: "no_params_tool", + description: "A tool without parameters", + parameters: {}, + }, + }, + ]; + expect(tools).toHaveLength(2); + expect(tools[0]?.function?.name).toBe("read_file"); + }); + + it("supports an empty tools array", () => { + const tools: ToolDefinition[] = []; + expect(tools).toHaveLength(0); + }); +}); diff --git a/ui/web/src/pages/agents/agent-detail/file-sections/tools-preview-section.tsx b/ui/web/src/pages/agents/agent-detail/file-sections/tools-preview-section.tsx new file mode 100644 index 00000000..833d61fc --- /dev/null +++ b/ui/web/src/pages/agents/agent-detail/file-sections/tools-preview-section.tsx @@ -0,0 +1,93 @@ +import { useState } from "react"; +import { useTranslation } from "react-i18next"; +import { ChevronRight, Wrench } from "lucide-react"; + +/** Mirrors providers.ToolFunctionSchema on the Go side. */ +interface ToolFunctionSchema { + name: string; + description: string; + parameters?: Record; +} + +/** Mirrors providers.ToolDefinition on the Go side. */ +export interface ToolDefinition { + type: string; + function?: ToolFunctionSchema; +} + +interface ToolsPreviewSectionProps { + tools: ToolDefinition[] | undefined; +} + +/** + * Lists the tool schemas (name, description, parameters) sent to the LLM + * provider as the `tools` API parameter, alongside the system prompt text. + */ +export function ToolsPreviewSection({ tools }: ToolsPreviewSectionProps) { + const { t } = useTranslation("agents"); + const list = tools ?? []; + + return ( +
+
+ + {t("files.toolsSection")} + + {t("files.toolsAvailable", { count: list.length })} + +
+ {list.length === 0 ? ( +

{t("files.noTools")}

+ ) : ( +
+ {list.map((tool, idx) => ( + + ))} +
+ )} +
+ ); +} + +function ToolEntry({ tool }: { tool: ToolDefinition }) { + const { t } = useTranslation("agents"); + const [expanded, setExpanded] = useState(false); + const fn = tool.function; + const hasParams = !!fn?.parameters && Object.keys(fn.parameters).length > 0; + + return ( +
+ + {expanded && ( +
+
+
+ {t("files.toolParameters")} +
+ {hasParams ? ( +
+
+                  {JSON.stringify(fn?.parameters, null, 2)}
+                
+
+ ) : ( +

{t("files.noParameters")}

+ )} +
+
+ )} +
+ ); +} diff --git a/ui/web/src/pages/agents/agent-detail/system-prompt-dialog.tsx b/ui/web/src/pages/agents/agent-detail/system-prompt-dialog.tsx index faa095bf..250137b9 100644 --- a/ui/web/src/pages/agents/agent-detail/system-prompt-dialog.tsx +++ b/ui/web/src/pages/agents/agent-detail/system-prompt-dialog.tsx @@ -7,6 +7,7 @@ import { import { MarkdownRenderer } from "@/components/shared/markdown-renderer"; import { useHttp } from "@/hooks/use-ws"; import { cn } from "@/lib/utils"; +import { ToolsPreviewSection, type ToolDefinition } from "./file-sections/tools-preview-section"; const MODES = ["full", "task", "minimal", "none"] as const; type PromptMode = (typeof MODES)[number]; @@ -16,6 +17,7 @@ interface PreviewResponse { prompt: string; token_count: number; sections: { name: string; start: number; end: number }[]; + tools?: ToolDefinition[]; } interface SystemPromptDialogProps { @@ -103,8 +105,11 @@ export function SystemPromptDialog({ agentKey, open, onOpenChange }: SystemPromp {error ? (

{error}

) : data ? ( -
- +
+
+ +
+
) : null}