mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-16 10:15:12 +00:00
fix(mcp): tenant isolation, tool policy engine, and prompt preview fixes (#1333)
* fix(prompt): remove duplicate Team Members section from system prompt The TEAM.md context file already provides the Members section with better formatting. The system prompt section was redundant and inconsistently formatted. - Remove buildTeamMembersSection() call from system prompt building - Remove unused buildTeamMembersSection() function * fix(mcp): wire store to manager for prompt preview tool visibility MCP manager needs database access to query configured servers for tool visibility in prompt preview. - Add SetStore() method to MCP manager - Wire pgStores.MCP to manager after initialization - Add debug logging for MCP initialization flow * fix(systemprompt): hide Tooling section when agent has no tools The Tooling section header and boilerplate were displayed even when the agent had no tools available. Add early return in buildToolingSection() to skip the section entirely when toolNames is empty. This reduces prompt noise for agents with no tool access. * feat(mcp): cache tool descriptions for prompt preview visibility MCP tools now show descriptions in prompt preview without requiring a live server connection. - Add CacheToolDescriptions() method to MCPServerStore (PG + SQLite) - Cache tool descriptions in settings['tool_cache'] when server connects - Use cached descriptions in ListToolsForAgent (fallback: hints → cache → global) - Descriptions are auto-populated from live server manifest on connection - Admins can still override via tool_hints in server settings * test(mcp): add CacheToolDescriptions to MCPServerStore test fakes Commit 5ce410b6 added CacheToolDescriptions() to the MCPServerStore interface but missed updating test mock implementations, breaking go vet across internal/mcp, internal/agent, internal/http, and internal/channels/bitrix24. Add no-op implementations matching each fake's existing style. * fix(providers): enforce per-agent tool policy for Claude CLI provider The Claude CLI provider (stdio+MCP bridge) was not enforcing per-agent tool policy, unlike other providers where the policy-filtered tool list already drives the system prompt's Tooling section. Two gaps closed: 1. --disallowedTools was previously skipped entirely when no MCP config path was resolved, letting the CLI subprocess run with its full native toolset (Bash, Edit, Read, Write, Glob, Grep, WebFetch, WebSearch) regardless of agent policy. It's now unconditional and derived from the agent's actual allowed-tools list (state.Tool.AllowedTools), mapped to Claude CLI's native tool names. 2. The MCP bridge server executed any tool call without checking the calling agent's policy. It now resolves the agent's policy from context (via the existing HMAC-verified agent lookup) and denies calls to tools outside that agent's allowed set, logging security.mcp_bridge_denied on denial. Both gaps were closed using existing plumbing (PolicyEngine.WouldAllow, AgentData.ParseToolsConfig, the bridge context middleware) — no new cross-cutting mechanism was introduced. * feat(web): show MCP/tool schemas in system prompt preview dialog The prompt-preview API response includes a separate `tools` field (the actual JSON schemas sent to the LLM as the tools API parameter) alongside `prompt` (the system prompt text), but the web UI only rendered `prompt`, silently dropping the tools list. Add a collapsible Tools section to both the full-screen System Prompt dialog and the inline agent-detail preview, showing tool count, name, description, and expandable parameter schema per tool. i18n keys added to en/vi/zh locales. * fix(prompt): render pinned skills on bootstrap turns Pinned skills are documented (web UI copy) as "always inlined in the system prompt", but the entire Skills section was gated behind !cfg.IsBootstrap, so pinned skill XML never appeared on bootstrap turns (first message of a session) despite the promise. Separate pinned-skill rendering from bootstrap-suppressed guidance: - Bootstrap + pinned skills present: render pinned XML only, no search/manage guidance (which stays suppressed as before) - Non-bootstrap: unchanged behavior - Minimal/none modes: pinned skills always render regardless of bootstrap state Add regression tests covering all four prompt modes on bootstrap turns, plus a non-bootstrap guard confirming existing behavior is preserved. * fix(skills): resolve managed skills directory per-tenant, not master-only skills.Loader was wired at startup to scan a single fixed directory (the master tenant's managed-skills dir), making any skill belonging to a non-master tenant invisible to both pinned-skills prompt resolution and skill_search/use_skill, regardless of DB visibility settings. - Loader now resolves the calling tenant's managed-skills directory per-call via context (store.TenantIDFromContext), never enumerating other tenants' directories - Skill cache is now tenant-keyed to prevent slug collisions and cross-tenant cache leaks across tenants using the same skill slug - gateway_setup.go passes the root data dir instead of a pre-resolved master-tenant path Write-side tooling (skill_manage, publish_skill) was already correctly tenant-scoped per-operation — no changes needed there. Added TestLoader_ManagedSkills_TenantIsolation proving two tenants with same-slug/different-content skills never see each other's content, including after cache population from a different tenant's lookup. Known follow-up (not in this commit): skill_search's BM25 index is still a single process-global index shared across tenants, which is a related but separate cross-tenant search-result leak requiring its own scoped fix (per-tenant index maps + threading tenant context through ensureIndex/rebuildIndex). * fix(tools): scope skill_search BM25 index per-tenant SkillSearchTool held a single process-global BM25 index built once from whichever tenant's context first triggered ensureIndex, then reused for all subsequent Execute() calls regardless of caller — leaking one tenant's skill search results into another's, the search-path counterpart to the managed-directory bug fixed in 7b4668ad. - index/lastVersion are now keyed per-tenant (map[uuid.UUID]*tenantIndexState) - ensureIndex resolves the calling tenant from context and only builds/reads that tenant's index entry, never touching another tenant's cached state - Builtin/bundled skills remain visible in every tenant's index (Loader already merges those tiers correctly per 7b4668ad) Loader.Version() remains a single global counter — a version bump in one tenant causes unnecessary rebuilds in others but does not cause cross-tenant leakage, an acceptable tradeoff to avoid scope creep. Added TestSkillSearchTool_TenantIsolation proving two tenants with same-slug/different-content skills never see each other's search results, including after cache population from a different tenant. * fix(tools): fix group-spec expansion in tool policy engine PolicyEngine.registry was only ever set via SetRegistry(), which was never called in production (only in one test) — so pe.registry was permanently nil in production. Every group-expansion helper (applyProfile, intersectWithSpec, unionWithSpec, subtractSpec, expandSpec, matchDenySpec, filterByCapability) silently dropped any "group:*" spec entry instead of expanding it when registry was nil. Concretely: "group:mcp" (auto-injected into agentToolPolicy.AlsoAllow for any agent with MCP tools) never resolved to real tool names, so MCP tools connected successfully and appeared in prompt text (which reads the registry directly, bypassing PolicyEngine) but were never included in the actual ChatRequest.Tools payload sent to the LLM — confirmed live via mcp.agent.tools_loaded tools=6 immediately followed by mcp.filtered_tools mcp_defs_count=0 in the same request. This affects any agent relying on group-based grants, not just MCP. PolicyEngine is a shared/global singleton used concurrently across all agents (constructed once at gateway startup), so mutating a registry field per-call would be a data race. Fix instead threads the registry as an explicit parameter from FilterTools down through all internal group-expansion helpers, and adds IsDenied/WouldAllow registry parameters, removing the dead SetRegistry() mechanism entirely. Also fixes group expansion for the per-user-MCP-tools path: FilterTools is sometimes called with a userToolOverlay wrapping a *Registry rather than a *Registry directly; added Unwrap() to userToolOverlay so the new registry-resolution logic works for both cases. Added 4 tests proving group expansion works via the threaded parameter alone (no SetRegistry): plain registry allow, userToolOverlay allow, deny-side group expansion, and the WouldAllow bridge-server path. Blast radius note: this restores intended access for every agent configured with group:* specs (group:mcp, group:vault, group:goclaw, group:coding, etc.) that were silently inert before. Existing agent configs relying on group grants will gain the tool access they were nominally already configured for. * fix(mcp): cache tool descriptions from pool-connected servers too 5ce410b6 added tool-description caching (for prompt-preview visibility) only inside connectServer. connectViaPool — the separate connect path used when MCP connections go through the shared pool — never got the same caching hook, even though it shares the same underlying connectAndDiscover wire handshake. Confirmed live: cloudflare/docker connected via connectViaPool and received real descriptions over the wire, but prompt preview still showed blank descriptions because this path never wrote to the cache. Also removes the temporary mcp.connect.raw_tool debug log added earlier this session for diagnosing the same issue — no longer needed now that the root cause is fixed. * chore(skills): remove temporary pinned-skills diagnostic logging Confirmed live: pinned skills (caveman, infra-ansible-knowledge) now resolve correctly end-to-end for tenant-scoped agents. Debug logging added to trace the resolution chain is no longer needed. * fix(tools): deny always wins over AlsoAllow group grants AlsoAllow's unionWithSpec could reintroduce a tool explicitly listed in Deny, since it added tools back from allTools without re-checking deny specs. Previously masked because AlsoAllow's group-expansion was also broken (fixed in f7af95de this session) — group specs silently expanded to nothing, so this ordering bug never manifested. Now that group expansion works, an admin-denied tool that's also reachable via a group:* AlsoAllow entry (e.g. group:mcp) would silently reappear. Re-apply deny-spec subtraction as a final step after AlsoAllow union, for both global and per-agent policy, so deny always wins regardless of which allow mechanism tries to add a tool back. Added tests proving global and per-agent Deny correctly override an overlapping AlsoAllow group grant, while sibling non-denied tools in the same group remain allowed. * fix(mcp): enumerate cached tools instead of wildcard placeholder in prompt preview ListToolsForAgent (the prompt-preview path) collapsed any server with an empty ToolAllow (unrestricted grant — the common case) into a single "server__*" placeholder entry, even when tool_cache already had every real tool name and description from connect time (5ce410b6, 8ffd67b9). This meant agents with unrestricted MCP server access never saw individual tool names or descriptions in prompt preview, forcing trial-and-error tool usage. When ToolAllow is empty and tool_cache is populated, enumerate every cached tool (skipping any explicitly denied) and emit one MCPToolPreviewInfo per tool, matching the construction logic already used for the ToolAllow-non-empty case. Falls back to the single placeholder only when tool_cache is also empty (server never connected). The live (non-preview) conversation path, buildMCPToolDescs, does not have this bug — it resolves tool identity from the live connected registry, never from ToolAllow, so no placeholder shortcut exists there. Added tests covering both the cache-populated enumeration case and the no-cache placeholder fallback. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(prompt): filter alias re-injection and add MCP schemas to preview ToolDefs Prompt-preview's tools: schema array (PreviewResult.ToolDefs) had two bugs, both preview-only — confirmed live conversations already use correctly-filtered tool payloads via buildToolsPayload/PolicyEngine.FilterTools, unaffected by either bug: 1. Alias re-injection iterated ALL registry aliases globally with no check against the deny-filtered toolNames list, letting denied tools reappear via their alias name (e.g. a denied canonical tool still showing up as its Claude-Code-compat alias like Bash/Edit/Write/Read). Now skips any alias whose canonical tool isn't in the filtered set. 2. MCP tool descriptions (from the store-based, connection-free ListToolsForAgent path) only ever fed the prompt TEXT section, never got converted into ToolDefinition schema objects — so the tools: array never showed MCP tools at all, even when the MCP section of the prompt text correctly listed them. Now appends a ToolDefinition per MCP tool with name+description populated and a placeholder {"type":"object"} Parameters schema, documented as preview-only (real parameter schemas require a live MCP connection, only available during an actual conversation turn). Added tests proving denied-tool-alias exclusion and MCP tool inclusion in preview ToolDefs. * feat(skills): inline full content for pinned skills instead of pointer-only Web UI documented "Pinned skills are always inlined in the system prompt", but BuildPinnedSummary was just BuildSummary with an allowlist filter — same as the general searchable skill list: name/description(truncated)/location pointer only, requiring use_skill+read_file round trips to get actual content. No code path inlined real SKILL.md content for pinned skills specifically. BuildPinnedSummary now reads and inlines full SKILL.md content (frontmatter stripped) per pinned skill inside <skill_instructions> tags. Per-skill (10000 bytes) and total (30000 bytes) size caps fall back to the original pointer-only format with a note when a skill is too large to inline, so oversized skills degrade gracefully instead of blowing the prompt budget. Added tests: full-content inlining, size-cap fallback, and tenant isolation for the new inline path (mirroring the existing managed- skills tenant isolation test). * fix(mcp): real parameter schemas and full policy enforcement in prompt preview Three interconnected fixes to prompt preview, none affecting the live conversation path (which was already correct): 1. Real MCP parameter schemas instead of a useless empty placeholder. tool_cache previously stored only name+description; extended to also capture the real JSON Schema from the MCP server's tools/list response (CachedToolInfo{Description, Parameters}) at connect time, for both direct-connect and pool-connect paths. Preview now shows complete, real input schemas instead of {"type":"object"} with no properties — verified against actual wire data from a live cloudflare MCP server showing genuinely rich schemas (zone_id, type, name, content, ttl, proxied, all typed with descriptions and correct required arrays) that were previously being discarded. Backward-compatible: old-shape cache entries degrade gracefully to description-only rather than crashing, self-healing on next connect. 2. Global tool deny now enforced in preview. BuildPreviewPrompt previously hand-rolled a partial policy reimplementation (per-agent deny only, explicitly skipping the full PolicyEngine "because runtime state isn't available in preview") — but PolicyEngine.WouldAllow already handles this per-tool-name without needing channel context. A tool denied via the global config (not per-agent) would appear in preview despite being correctly denied in every real conversation. Preview now calls WouldAllow per candidate tool, with a graceful per-agent-deny-only fallback when no PolicyEngine is wired (e.g. in tests). 3. MCP tools now also subject to the same policy check — previously the MCP tool supplement (store-based, connection-free tool listing) added MCP tool names unconditionally, bypassing WouldAllow entirely, so a denied MCP tool could still appear in preview. Added tests for all three: real-schema presence, global-deny exclusion for both core and MCP tools, and backward-compat cache handling. * fix(prompt): remove redundant per-tool MCP enumeration from prompt text Now that MCP tool schemas in the tools: API parameter are real and complete (1290d4f1), the ## MCP Tools prompt-text section's per-tool "- mcp_x__y: description" enumeration is pure duplication with zero added value — the model already gets each tool's real schema (including description) via tools:. buildMCPToolsInlineSection now keeps only the behavioral instructions that aren't expressible via JSON schema and thus aren't duplicated: prefer-MCP-over-core-tools guidance, and the optional-parameter guidance (don't guess/fill optional fields). The per-tool name+ description enumeration loop is removed. Section still only appears when the agent has MCP tools (len(cfg.MCPToolDescs) > 0, unchanged gate). Updated tests to assert the enumeration is gone while the behavioral instructions remain; ToolDefs assertions are now the authoritative check for MCP tool allow/deny filtering behavior (prompt text no longer enumerates names at all). * test(agent): update TeamContextInjection test for removed Team Members section TestBuildSystemPrompt_TeamContextInjection asserted the presence of a 'Team Members' prompt-text section that was intentionally removed in 71d33180 (duplicate of the canonical TEAM.md-context-file Members section, which has better formatting). The test was never updated to match, causing it to fail on every run since. Moved the assertion from wantIn to wantNotIn for the 3 affected subtests -- BuildSystemPrompt correctly no longer renders team-member roster info directly; that info now comes exclusively from the TEAM.md context-file mechanism, outside this test's isolated scope. * fix(prompt): resolve real registry for WouldAllow calls in preview BuildPreviewPrompt's two WouldAllow calls hardcoded reg=nil, silently breaking group:* expansion (e.g. group:mcp) needed to resolve the AlsoAllow grant production actually uses to grant MCP tool access (resolver_helpers.go's agentToolPolicyWithMCP injects AlsoAllow: ["group:mcp"]). With reg=nil, WouldAllow could match literal tool names fine (the 18 core/static tools) but could never resolve group-based grants, so every MCP tool silently failed WouldAllow and was excluded from preview -- confirmed live via curl: 18 tools returned, zero mcp_* ones, for an agent with genuinely working MCP access in real conversations. Live conversations were never affected -- internal/mcp/bridge_server.go's WouldAllow call already correctly passes a real registry. Fix resolves a real *tools.Registry from deps.ToolLister via tools.ResolveConcreteRegistry (the same helper used at the live call site), passing it to both WouldAllow calls instead of nil. Falls back to nil gracefully for test mocks that don't implement the full ToolExecutor interface, preserving existing test behavior. Added a test proving an MCP tool granted via the exact production AlsoAllow: ["group:mcp"] pattern is now correctly included in preview ToolDefs, where the old reg=nil bug would have silently excluded it. * fix(prompt): use literal deny check for MCP tools in preview, not group expansion The MCP-tools policy gate added in 1290d4f1 called WouldAllow with a real registry (per 9b5fd2eb), which requires group:mcp expansion against that registry to grant access via the production AlsoAllow: ["group:mcp"] pattern. But MCP tools are only ever registered into ephemeral per-agent registry clones at live connection time (manager_connect.go) -- never into the shared/global registry preview uses. group:mcp always resolved empty in preview's connection-free context, so WouldAllow denied every MCP tool -- confirmed live via tool-name diff: live conversations correctly included all 6 MCP tools, preview included zero. MCP access-granting is already correctly handled by ListToolsForAgent's own per-server tool_allow/tool_deny grant logic (confirmed working correctly earlier this session). The preview gate only needs to catch the narrower case of a literally-denied tool name via global/per-agent policy config -- it never needed group expansion. Replaced WouldAllow with IsDenied(nil, name, agentPolicy), which forces a pure literal-name match with zero registry dependency, matching the existing usage pattern already established elsewhere in policy.go. This class of bug cannot recur: there's no registry-passing code path left in this check to silently reintroduce group-expansion dependence. Added a test proving MCP tool inclusion in preview is independent of group-expansion outcome (no AlsoAllow: group:mcp needed for a non-denied tool to appear). Also reverts the temporary loop.filtered_tool_names/ preview_prompt.filtered_tool_names diagnostic logging used to capture the live-vs-preview tool-name comparison that diagnosed this bug. * fix(http): preserve real MCP parameter schemas through HTTP preview adapter mcpPreviewAdapter.ListToolsForAgent (the HTTP-layer glue converting mcp.MCPToolPreviewInfo to agent.MCPToolPreviewInfo for BuildPreviewPrompt) only copied RegisteredName and Description, silently dropping Parameters -- a bug present since this adapter was introduced (2499d0be/7e250244), unrelated to today's other MCP preview fixes. This was masked until d5fc6344 fixed MCP tools being excluded from preview entirely (a separate bug) -- once MCP tools started appearing again, this pre-existing adapter gap became visible: tools showed up correctly, but always with the bare {"type":"object"} placeholder instead of their real cached schema (confirmed live: update_dns_record missing its 7 real properties). One-line fix: copy Parameters through in the adapter's struct literal. Added a regression test constructing a real *mcp.Manager with populated tool_cache, asserting the adapter's output preserves specific real schema properties (not just non-nil Parameters) -- verified this test fails without the fix and passes with it. --------- Co-authored-by: Bruno Clermont <bruno.clermont@gmail.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
48 files changed
+2405
-229
No files matched your search
+19
-1
@@ -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")
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
+17
-3
@@ -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
|
||||
}
|
||||
@@ -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).
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
@@ -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 {
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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}
|
||||
}
|
||||
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
|
||||
@@ -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",
|
||||
"",
|
||||
|
||||
@@ -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 = "<available_skills><skill><name>weather-lookup</name></skill></available_skills>"
|
||||
|
||||
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 = "<available_skills><skill><name>weather-lookup</name></skill></available_skills>"
|
||||
|
||||
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")
|
||||
}
|
||||
}
|
||||
@@ -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 `<skill_instructions name=\"...\">` already contain their full SKILL.md content inline — use it directly, no `use_skill`/`read_file` round trip needed. "+
|
||||
"Pinned skills shown as `<skill>` (pointer only, too large to inline) still need `use_skill` then `read_file` with the exact `<location>`.",
|
||||
"",
|
||||
)
|
||||
}
|
||||
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 {
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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 --------------------------------------------------------
|
||||
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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")
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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"])
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
+94
-8
@@ -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)
|
||||
|
||||
@@ -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
|
||||
}
|
||||
@@ -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.
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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{
|
||||
|
||||
+151
-31
@@ -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: <dir>/<slug>/<version>/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: <dataDir>/[tenants/<slug>/]skills-store/<slug>/<version>/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: <dir>/<slug>/<version>/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: <managedSkillsDir>/<slug>/<version>/SKILL.md
|
||||
// listManagedSkills scans the given managed skills directory (already resolved
|
||||
// to a single tenant via resolveManagedDir) for versioned skill directories.
|
||||
// Structure: <managedDir>/<slug>/<version>/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 <skill_instructions>. Skills whose content exceeds
|
||||
// skillInlineMaxBytes, or that would push the combined total over
|
||||
// skillInlineTotalMaxBytes, fall back to a pointer-only <skill> 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, "<available_skills>")
|
||||
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(" <skill_instructions name=%q>", s.Name))
|
||||
lines = append(lines, content)
|
||||
lines = append(lines, " </skill_instructions>")
|
||||
totalInlined += len(content)
|
||||
continue
|
||||
}
|
||||
|
||||
lines = append(lines, " <skill>")
|
||||
lines = append(lines, fmt.Sprintf(" <name>%s</name>", escapeXML(s.Name)))
|
||||
desc := s.Description
|
||||
if len([]rune(desc)) > skillDescMaxLen {
|
||||
desc = string([]rune(desc)[:skillDescMaxLen]) + "…"
|
||||
}
|
||||
lines = append(lines, fmt.Sprintf(" <description>%s</description>", escapeXML(desc)))
|
||||
lines = append(lines, fmt.Sprintf(" <location>%s</location>", escapeXML(s.Path)))
|
||||
lines = append(lines, " <note>content too large to inline, use use_skill+read_file</note>")
|
||||
lines = append(lines, " </skill>")
|
||||
}
|
||||
lines = append(lines, "</available_skills>")
|
||||
|
||||
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
|
||||
}
|
||||
|
||||
|
||||
@@ -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 <dataDir>/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: <dataDir>/skills-store/<slug>/<ver>/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: <dataDir>/tenants/<A>/skills-store/<slug>/<ver>/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: <dataDir>/tenants/<B>/skills-store/<slug>/<ver>/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, `<skill_instructions name="My Skill">`) {
|
||||
t.Fatalf("expected <skill_instructions> wrapper, got: %s", summary)
|
||||
}
|
||||
if strings.Contains(summary, "<note>") {
|
||||
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, "<skill>") || !strings.Contains(summary, "<note>content too large to inline") {
|
||||
t.Fatalf("expected pointer-only fallback with note, got: %s", summary)
|
||||
}
|
||||
if !strings.Contains(summary, "<name>Big Skill</name>") {
|
||||
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)
|
||||
}
|
||||
}
|
||||
@@ -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"`
|
||||
}
|
||||
@@ -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 {
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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).
|
||||
|
||||
+59
-37
@@ -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{}{}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
}
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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)
|
||||
@@ -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",
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -608,7 +608,12 @@
|
||||
"insertContact": "搜索联系人以插入...",
|
||||
"contentPlaceholder": "文件内容...",
|
||||
"systemPromptPreview": "系统提示词",
|
||||
"tokens": "tokens"
|
||||
"tokens": "tokens",
|
||||
"toolsAvailable": "{{count}} 个可用工具",
|
||||
"toolsSection": "工具",
|
||||
"toolParameters": "参数",
|
||||
"noParameters": "无参数",
|
||||
"noTools": "没有可用工具"
|
||||
},
|
||||
"fileEditor": {
|
||||
"editWithAi": "AI 编辑",
|
||||
|
||||
@@ -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 ? (
|
||||
<p className="text-sm text-destructive">{error}</p>
|
||||
) : data ? (
|
||||
<pre className="whitespace-pre-wrap break-words font-mono text-xs leading-relaxed text-foreground/90">
|
||||
{renderPromptWithBoundary(data.prompt)}
|
||||
</pre>
|
||||
<>
|
||||
<pre className="whitespace-pre-wrap break-words font-mono text-xs leading-relaxed text-foreground/90">
|
||||
{renderPromptWithBoundary(data.prompt)}
|
||||
</pre>
|
||||
<ToolsPreviewSection tools={data.tools} />
|
||||
</>
|
||||
) : null}
|
||||
</div>
|
||||
</div>
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
@@ -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<string, unknown>;
|
||||
}
|
||||
|
||||
/** 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 (
|
||||
<div className="mt-4 border-t pt-3">
|
||||
<div className="mb-2 flex items-center gap-2">
|
||||
<Wrench className="h-4 w-4 text-muted-foreground" />
|
||||
<span className="text-sm font-medium">{t("files.toolsSection")}</span>
|
||||
<span className="rounded bg-muted px-2 py-0.5 text-xs tabular-nums text-muted-foreground">
|
||||
{t("files.toolsAvailable", { count: list.length })}
|
||||
</span>
|
||||
</div>
|
||||
{list.length === 0 ? (
|
||||
<p className="text-sm text-muted-foreground">{t("files.noTools")}</p>
|
||||
) : (
|
||||
<div className="space-y-1.5">
|
||||
{list.map((tool, idx) => (
|
||||
<ToolEntry key={`${tool.function?.name ?? "tool"}-${idx}`} tool={tool} />
|
||||
))}
|
||||
</div>
|
||||
)}
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
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 (
|
||||
<div className="rounded-md border bg-muted">
|
||||
<button
|
||||
type="button"
|
||||
className="flex w-full items-center gap-2 px-3 py-1.5 text-left text-xs cursor-pointer"
|
||||
onClick={() => setExpanded((v) => !v)}
|
||||
>
|
||||
<ChevronRight
|
||||
className={`h-3 w-3 shrink-0 text-muted-foreground transition-transform ${expanded ? "rotate-90" : ""}`}
|
||||
/>
|
||||
<span className="font-medium shrink-0 font-mono">{fn?.name ?? tool.type}</span>
|
||||
{fn?.description && (
|
||||
<span className="truncate text-muted-foreground ml-1">{fn.description}</span>
|
||||
)}
|
||||
</button>
|
||||
{expanded && (
|
||||
<div className="border-t border-muted px-2 py-1.5 space-y-1.5">
|
||||
<div>
|
||||
<div className="text-2xs font-semibold uppercase text-muted-foreground mb-0.5">
|
||||
{t("files.toolParameters")}
|
||||
</div>
|
||||
{hasParams ? (
|
||||
<div className="overflow-x-auto">
|
||||
<pre className="whitespace-pre-wrap break-words text-xs-plus font-mono bg-background rounded p-1.5 max-h-60 overflow-y-auto">
|
||||
{JSON.stringify(fn?.parameters, null, 2)}
|
||||
</pre>
|
||||
</div>
|
||||
) : (
|
||||
<p className="text-xs-plus text-muted-foreground">{t("files.noParameters")}</p>
|
||||
)}
|
||||
</div>
|
||||
</div>
|
||||
)}
|
||||
</div>
|
||||
);
|
||||
}
|
||||
@@ -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 ? (
|
||||
<p className="text-sm text-destructive p-4">{error}</p>
|
||||
) : data ? (
|
||||
<div className="prose prose-sm dark:prose-invert max-w-none p-1">
|
||||
<MarkdownRenderer content={insertCacheBoundary(data.prompt)} />
|
||||
<div className="p-1">
|
||||
<div className="prose prose-sm dark:prose-invert max-w-none">
|
||||
<MarkdownRenderer content={insertCacheBoundary(data.prompt)} />
|
||||
</div>
|
||||
<ToolsPreviewSection tools={data.tools} />
|
||||
</div>
|
||||
) : null}
|
||||
</div>
|
||||
|
||||
Reference in new issue
Block a user