From b39f0decb95666ab4c8f82b25b4122f32e519d2c Mon Sep 17 00:00:00 2001 From: HaiDuong <115532203+mor-duongmh@users.noreply.github.com> Date: Tue, 1 Sep 2026 02:42:47 +0700 Subject: [PATCH] fix(skills): five defects in slash activation, grants, subagent tool policy, preview and import (#1534) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(agent): filter skill slash commands by the agent's grants The inline block was already filtered by visibility and agent grants, but slash activation read the loader's full skill list. A / command could therefore activate a skill the agent was never granted, and the not-found suggestions disclosed that such skills existed. Resolve slash commands against the agent's allow list via FilterSkills. The list is filtered before matching, so /list-skills, /help and the near-match suggestions are all covered by the same change. The allow-list convention is unchanged: nil means every skill (the fallback when the access store errors), an empty slice means none, and a populated slice is an explicit set of slugs. * fix(agent): split slash commands on any whitespace The tokenizer separated the skill name from the rest of the message with strings.Cut(after, " ") — a literal space. A user who typed the command and pressed Enter before the rest of the message sent "/ck:git\nreview the diff", which parsed to the target "ck:git\nreview" and matched no skill. The reply was "skill not found" followed by near-matches that included the skill they had just named, because the similarity fallback searched the mangled string and still landed beside it. Multi-line messages are ordinary in Slack and Telegram, so this was reachable in normal use. Introduce cutFirstField, which splits on the first run of whitespace, and use it everywhere the literal-space split appeared. The match loop had the same assumption in strings.HasPrefix(raw, value+" "); hasFieldPrefix replaces it and decodes the following rune properly, so a multi-byte skill name is not truncated. That also stops a plain string prefix from matching: a command for "frontend-design-extra" no longer resolves to "frontend-design". * fix(tools): inherit the parent agent's tool policy in spawned subagents buildSubagentToolsRegistry clones the parent registry, then overwrites exec, read_file, write_file and list_files with freshly constructed tools. A fresh tool carries none of the hardening the gateway applies to the parent's instances at startup: exec path denials and their exemptions, the shell deny-group toggles, the command keyword allowlist, and the read/write/list deny prefixes covering config.json, the internal databases and delegate/. Spawning a subagent therefore widened what an agent could reach — the parent was blocked from the data dir, the subagent was not. Verified by disabling the new call: the subagent's exec read config.json out of the denied data dir, read_file did the same, and write_file overwrote it. Copy the policy from the live parent instances rather than re-deriving it, so there is one source of truth and a deny path reloaded later through config pub/sub reaches subagents without a second wiring site that can drift. Allow-prefixes are inherited alongside the denials: they are what make the denied roots usable at all, since the skills store sits under the denied data dir. Copying denials without them would leave a subagent unable to read the skills it is told to use. * fix(agent): use one inline-vs-search decision for prompt and preview The system-prompt preview exists to show the prompt an agent actually gets, but it decided between inline skills and search mode on its own terms: it counted tokens with the fallback counter over the fully rendered XML — tags and paths included, at roughly runes/2 — while the prompt builder estimated name+description at chars/4. On the same eighteen skills that read 3087 against 944, so the preview reported search mode for an agent that was running inline. Extract shouldInlineSkills and call it from both paths. The estimate keeps the prompt builder's rule, mirroring BuildSummary's 200-rune description truncation, so the decision still costs no rendering. The preview's loader interface gains FilterSkills to feed it. Its behaviour on an access-store error is unchanged: an empty allow list yields no skills rather than falling back to showing every skill. * fix(http): keep reference files when importing skills Export archives the whole skill directory, but import recognised only metadata.json, SKILL.md and grants.jsonl. The switch had no default, so every other file was discarded without a log line. A skill whose SKILL.md cites references/*.md arrived without them, the import reported success, and the skill failed at the first read. Collect the remaining entries and write them under the skill directory with their structure intact. Archive entry names are attacker-controlled, so each path goes through sanitizeRelPath: a traversal attempt collapses to a relative path and the write stays inside the skill directory. * test(agent): make the inline-decision table exercise both gates The "100 skills, 200-char descriptions" case claimed to cover the token ceiling, but 100 exceeds the count ceiling so the count check short-circuited and the token branch never ran — deleting that branch would not have failed the test. Split the table so each case isolates one gate: tiny descriptions for the count rows, a count safely under the ceiling for the token rows. Removing the token comparison now fails the over-the-ceiling case and nothing else. * fix(agent): gate slash commands on the managed tier only The allow list comes from a query over the `skills` table, so filesystem-tier skills — the workspace, .agents and ~/.agents directories of the five-tier loader — have no row in it. Filtering every skill against the list made those four tiers unreachable by slash while skill_search still found them unfiltered, which turned a security fix into a functional regression. Apply the list to managed skills only. Builtins are seeded with is_system and returned unconditionally, so they stay reachable either way. This closes slash activation of ungranted managed skills. It does not close skill_search and use_skill, which still read the loader's full list; that is a wider change and wants its own review. * fix(http): stop imported skill files from replacing guarded ones Two ways the auxiliary write could go wrong. GuardSkillContent scans SKILL.md before anything reaches disk, but the switch that routes archive entries compares the raw path. An entry named "./SKILL.md" does not match it, so it fell through to the auxiliary set — where sanitizeRelPath collapses it back to "SKILL.md" and the write replaced the file that had just been scanned. Reject any auxiliary path that resolves to a name the import handles by itself, and log the attempt. Tar directory entries carry a trailing separator and no content. Written as files they took the name a real directory needed, so everything beneath was dropped — and whether that happened depended on map iteration order, making it intermittent. Skip them; MkdirAll creates what is needed. Also cap the number of auxiliary files per skill and log when the cap trims an archive, so a truncated import is visible rather than silent. --------- Co-authored-by: mor-phongdt --- cmd/gateway_agents.go | 67 +++++- cmd/gateway_subagent_policy_inherit_test.go | 127 +++++++++++ internal/agent/loop_history_skills.go | 41 ++-- internal/agent/preview_prompt.go | 16 +- internal/agent/preview_prompt_test.go | 58 +++++- .../agent/preview_prompt_test_helpers_test.go | 52 ++++- .../agent/skill_slash_command_access_test.go | 98 +++++++++ .../agent/skill_slash_command_matching.go | 44 +++- .../agent/skill_slash_command_parsing_test.go | 95 +++++++++ internal/agent/skill_slash_commands.go | 49 ++++- internal/agent/skill_slash_commands_test.go | 39 +++- internal/http/agents.go | 2 + internal/http/skills_import.go | 74 +++++++ internal/http/skills_import_aux_files_test.go | 197 ++++++++++++++++++ internal/tools/security_policy_inherit.go | 75 +++++++ 15 files changed, 971 insertions(+), 63 deletions(-) create mode 100644 cmd/gateway_subagent_policy_inherit_test.go create mode 100644 internal/agent/skill_slash_command_access_test.go create mode 100644 internal/agent/skill_slash_command_parsing_test.go create mode 100644 internal/http/skills_import_aux_files_test.go create mode 100644 internal/tools/security_policy_inherit.go diff --git a/cmd/gateway_agents.go b/cmd/gateway_agents.go index 4a8efc30..269b7490 100644 --- a/cmd/gateway_agents.go +++ b/cmd/gateway_agents.go @@ -231,19 +231,34 @@ func buildSubagentToolsRegistry( ) (*tools.Registry, *tools.ExecTool) { reg := parentReg.Clone() var execTool *tools.ExecTool + var readTool *tools.ReadFileTool + var writeTool *tools.WriteFileTool + var listTool *tools.ListFilesTool if sandboxMgr != nil { - reg.Register(tools.NewSandboxedReadFileTool(workspace, restrict, sandboxMgr)) - reg.Register(tools.NewSandboxedWriteFileTool(workspace, restrict, sandboxMgr)) - reg.Register(tools.NewSandboxedListFilesTool(workspace, restrict, sandboxMgr)) + readTool = tools.NewSandboxedReadFileTool(workspace, restrict, sandboxMgr) + writeTool = tools.NewSandboxedWriteFileTool(workspace, restrict, sandboxMgr) + listTool = tools.NewSandboxedListFilesTool(workspace, restrict, sandboxMgr) execTool = tools.NewSandboxedExecTool(workspace, restrict, sandboxMgr) - reg.Register(execTool) } else { - reg.Register(tools.NewReadFileTool(workspace, restrict)) - reg.Register(tools.NewWriteFileTool(workspace, restrict)) - reg.Register(tools.NewListFilesTool(workspace, restrict)) + readTool = tools.NewReadFileTool(workspace, restrict) + writeTool = tools.NewWriteFileTool(workspace, restrict) + listTool = tools.NewListFilesTool(workspace, restrict) execTool = tools.NewExecTool(workspace, restrict) - reg.Register(execTool) } + + // These four tools are built fresh, so they start with none of the hardening the + // gateway applied to the parent's instances at startup: exec path denials and their + // exemptions, shell deny-group toggles, the command keyword allowlist, and the + // read/write/list deny prefixes covering config.json, the databases, and delegate/. + // Without this, spawning a subagent widened reach — the parent could not touch the + // data dir, the subagent could. Inherit from the live parent instances so there is + // one source of truth and a later config reload cannot leave subagents behind. + inheritParentPathPolicy(parentReg, readTool, writeTool, listTool, execTool) + + reg.Register(readTool) + reg.Register(writeTool) + reg.Register(listTool) + reg.Register(execTool) // Red Team F3: subagent ExecTool must enforce the secure-CLI gate // (and env scrub on fall-through) — without this, a parent agent // can spawn a subagent to bypass the gate via host-inherited env. @@ -253,6 +268,42 @@ func buildSubagentToolsRegistry( return reg, execTool } +// inheritParentPathPolicy copies the parent registry's tool hardening onto the freshly +// built subagent tools. A tool missing from the parent registry, or registered there +// under an unexpected concrete type, is skipped: the subagent then has no policy to +// inherit for it, which matches the parent having none to give. +func inheritParentPathPolicy( + parentReg *tools.Registry, + readTool *tools.ReadFileTool, + writeTool *tools.WriteFileTool, + listTool *tools.ListFilesTool, + execTool *tools.ExecTool, +) { + if parentReg == nil { + return + } + if pt, ok := parentReg.Get("read_file"); ok { + if parent, ok := pt.(*tools.ReadFileTool); ok { + readTool.InheritPathPolicy(parent) + } + } + if pt, ok := parentReg.Get("write_file"); ok { + if parent, ok := pt.(*tools.WriteFileTool); ok { + writeTool.InheritPathPolicy(parent) + } + } + if pt, ok := parentReg.Get("list_files"); ok { + if parent, ok := pt.(*tools.ListFilesTool); ok { + listTool.InheritPathPolicy(parent) + } + } + if pt, ok := parentReg.Get("exec"); ok { + if parent, ok := pt.(*tools.ExecTool); ok { + execTool.InheritSecurityPolicy(parent) + } + } +} + // setupTTS creates the TTS manager from config and registers providers. // Edge TTS is always registered (free, no API key required). // Always returns a non-nil manager with at least one provider. diff --git a/cmd/gateway_subagent_policy_inherit_test.go b/cmd/gateway_subagent_policy_inherit_test.go new file mode 100644 index 00000000..69185694 --- /dev/null +++ b/cmd/gateway_subagent_policy_inherit_test.go @@ -0,0 +1,127 @@ +package cmd + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/nextlevelbuilder/goclaw/internal/tools" +) + +// A subagent's file and exec tools are constructed fresh, so they start with none of the +// hardening the gateway applies to the parent's instances at startup. Before this was +// wired, spawning a subagent widened reach: the parent could not read config.json or +// exec against the data dir, the subagent could. These tests pin the inheritance. +func TestSubagentToolsInheritParentPolicy(t *testing.T) { + workspace := t.TempDir() + dataDir := t.TempDir() + + // The denied files must exist. Without them a read or a `cat` fails because the file + // is missing, the assertion sees IsError and passes — for the wrong reason. Verified + // by disabling the fix: only the write_file assertion went red until these existed. + if err := os.WriteFile(filepath.Join(workspace, "config.json"), []byte("{}"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dataDir, "config.json"), []byte("{}"), 0o644); err != nil { + t.Fatal(err) + } + + parent := tools.NewRegistry() + parentRead := tools.NewReadFileTool(workspace, true) + parentRead.DenyPaths("config.json", "memory.db") + parentWrite := tools.NewWriteFileTool(workspace, true) + parentWrite.DenyPaths("config.json", "delegate/") + parentList := tools.NewListFilesTool(workspace, true) + parentList.DenyPaths("config.json") + parentExec := tools.NewExecTool(workspace, true) + parentExec.DenyPaths(dataDir) + parent.Register(parentRead) + parent.Register(parentWrite) + parent.Register(parentList) + parent.Register(parentExec) + + reg, execTool := buildSubagentToolsRegistry(parent, workspace, true, nil, nil) + if reg == nil || execTool == nil { + t.Fatal("buildSubagentToolsRegistry returned nil") + } + + ctx := context.Background() + + // exec: a command referencing the parent's denied data dir must be refused. + res := execTool.Execute(ctx, map[string]any{"command": "cat " + dataDir + "/config.json"}) + if res == nil { + t.Fatal("exec returned nil result") + } + if !res.IsError { + t.Errorf("subagent exec reached the parent's denied data dir: %+v", res) + } + + // read_file: the parent's denied prefixes must apply. + rf, ok := reg.Get("read_file") + if !ok { + t.Fatal("read_file missing from subagent registry") + } + res = rf.Execute(ctx, map[string]any{"path": "config.json"}) + if res == nil || !res.IsError { + t.Errorf("subagent read_file reached config.json: %+v", res) + } + + // write_file: same. + wf, ok := reg.Get("write_file") + if !ok { + t.Fatal("write_file missing from subagent registry") + } + res = wf.Execute(ctx, map[string]any{"path": "config.json", "content": "x"}) + if res == nil || !res.IsError { + t.Errorf("subagent write_file reached config.json: %+v", res) + } +} + +// Inheriting denials without the parent's exemptions would leave the subagent unable to +// read the skills it is told to use: the skills store sits under the denied data dir. +func TestSubagentExecInheritsParentPathExemptions(t *testing.T) { + workspace := t.TempDir() + dataDir := t.TempDir() + skillsStore := dataDir + "/skills-store/" + + if err := os.MkdirAll(filepath.Join(skillsStore, "demo"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(skillsStore, "demo", "SKILL.md"), []byte("# demo\n"), 0o644); err != nil { + t.Fatal(err) + } + + parent := tools.NewRegistry() + parentExec := tools.NewExecTool(workspace, true) + parentExec.DenyPaths(dataDir) + parentExec.AllowPathExemptions(skillsStore) + parent.Register(parentExec) + + _, execTool := buildSubagentToolsRegistry(parent, workspace, true, nil, nil) + + res := execTool.Execute(context.Background(), map[string]any{"command": "cat " + skillsStore + "demo/SKILL.md"}) + if res == nil { + t.Fatal("exec returned nil result") + } + if res.IsError { + t.Errorf("exemption did not carry over; reading the skills store was refused: %+v", res) + } + if !strings.Contains(res.ForLLM, "# demo") { + t.Errorf("expected the skill file contents, got %q", res.ForLLM) + } +} + +// A parent registry without the tools, or with nothing configured, must not panic. +func TestSubagentToolsInheritTolerantOfMissingParentTools(t *testing.T) { + workspace := t.TempDir() + + reg, execTool := buildSubagentToolsRegistry(tools.NewRegistry(), workspace, true, nil, nil) + if reg == nil || execTool == nil { + t.Fatal("expected a usable registry from an empty parent") + } + if _, ok := reg.Get("read_file"); !ok { + t.Error("read_file should still be registered") + } +} diff --git a/internal/agent/loop_history_skills.go b/internal/agent/loop_history_skills.go index 012f0688..43547e66 100644 --- a/internal/agent/loop_history_skills.go +++ b/internal/agent/loop_history_skills.go @@ -2,6 +2,8 @@ package agent import ( "context" + + "github.com/nextlevelbuilder/goclaw/internal/skills" ) // Hybrid skill thresholds: when skill count and total token estimate are below @@ -29,26 +31,39 @@ func (l *Loop) resolveSkillsSummary(ctx context.Context, skillFilter []string) s } filtered := l.skillsLoader.FilterSkills(ctx, allowList) - if len(filtered) == 0 { + if !shouldInlineSkills(filtered) { + // Search mode: no XML in prompt, agent uses skill_search tool return "" } + return l.skillsLoader.BuildSummary(ctx, allowList) +} - // Estimate tokens: ~1 token per 4 chars for name+description. - // Cap description length to match BuildSummary() truncation (skillDescMaxLen=200 runes). +// shouldInlineSkills decides between inline mode and search mode for a set of skills. +// +// Both the live prompt builder and the system-prompt preview endpoint must call this. +// They used to decide separately and disagree: the preview counted tokens with +// tokencount.NewFallbackCounter() over the fully rendered XML — tags, paths +// and all, at roughly runes/2 — while the runtime estimated name+description at chars/4. +// On the same 18 skills that was 3087 against 944, so the preview reported search mode +// for an agent that was actually running inline. A preview whose whole purpose is to show +// the real prompt cannot use a different rule than the real prompt. +// +// The estimate deliberately mirrors BuildSummary()'s truncation (skillDescMaxLen=200 +// runes) rather than measuring the rendered string, so the decision costs no rendering. +func shouldInlineSkills(filtered []skills.Info) bool { + if len(filtered) == 0 { + return false + } + if len(filtered) > skillInlineMaxCount { + return false + } + // ~1 token per 4 chars for name+description, +10 for the XML tag overhead per entry. totalChars := 0 for _, s := range filtered { descLen := min(len(s.Description), 200) - totalChars += len(s.Name) + descLen + 10 // +10 for XML tags overhead + totalChars += len(s.Name) + descLen + 10 } - estimatedTokens := totalChars / 4 - - if len(filtered) <= skillInlineMaxCount && estimatedTokens <= skillInlineMaxTokens { - // Inline mode: build full XML summary - return l.skillsLoader.BuildSummary(ctx, allowList) - } - - // Search mode: no XML in prompt, agent uses skill_search tool - return "" + return totalChars/4 <= skillInlineMaxTokens } // resolvePinnedSkillsSummary builds XML for pinned skills only (always inline). diff --git a/internal/agent/preview_prompt.go b/internal/agent/preview_prompt.go index 26e4e45a..6e13e965 100644 --- a/internal/agent/preview_prompt.go +++ b/internal/agent/preview_prompt.go @@ -12,8 +12,8 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/bootstrap" "github.com/nextlevelbuilder/goclaw/internal/providers" + "github.com/nextlevelbuilder/goclaw/internal/skills" "github.com/nextlevelbuilder/goclaw/internal/store" - "github.com/nextlevelbuilder/goclaw/internal/tokencount" "github.com/nextlevelbuilder/goclaw/internal/tools" ) @@ -57,6 +57,9 @@ type PreviewDeps struct { SkillsLoader interface { BuildPinnedSummary(ctx context.Context, names []string) string BuildSummary(ctx context.Context, allowList []string) string + // FilterSkills feeds shouldInlineSkills, so the preview reaches the same + // inline-vs-search verdict as the live prompt builder. + FilterSkills(ctx context.Context, allowList []string) []skills.Info } // MCPLister provides store-based MCP tool info for preview (nil = skip). // When set, MCP tool descriptions are populated from configured servers @@ -304,13 +307,10 @@ func BuildPreviewPrompt(ctx context.Context, ag *store.AgentData, mode PromptMod } } - summary := deps.SkillsLoader.BuildSummary(ctx, skillAllowList) - if summary != "" { - tokens := tokencount.NewFallbackCounter().Count("claude-3", summary) - if tokens <= skillInlineMaxTokens { - skillsSummary = summary - } - // Over threshold → search-only mode (skillsSummary stays empty) + // Same decision function as the live prompt builder — see shouldInlineSkills. + // Over threshold → search-only mode (skillsSummary stays empty). + if shouldInlineSkills(deps.SkillsLoader.FilterSkills(ctx, skillAllowList)) { + skillsSummary = deps.SkillsLoader.BuildSummary(ctx, skillAllowList) } } diff --git a/internal/agent/preview_prompt_test.go b/internal/agent/preview_prompt_test.go index e860e4e3..403e113b 100644 --- a/internal/agent/preview_prompt_test.go +++ b/internal/agent/preview_prompt_test.go @@ -11,6 +11,7 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/config" "github.com/nextlevelbuilder/goclaw/internal/providers" + "github.com/nextlevelbuilder/goclaw/internal/skills" "github.com/nextlevelbuilder/goclaw/internal/store" "github.com/nextlevelbuilder/goclaw/internal/tools" ) @@ -42,6 +43,7 @@ func TestBuildPreviewPrompt_SkillsInline(t *testing.T) { r := BuildPreviewPrompt(context.Background(), baseAgent(), PromptFull, "", PreviewDeps{ SkillsLoader: &mockSkillsLoader{ summary: "\nGit operations\n", + infos: skillInfoN(1, 20), }, }) if !strings.Contains(r.Prompt, "") { @@ -51,14 +53,56 @@ func TestBuildPreviewPrompt_SkillsInline(t *testing.T) { func TestBuildPreviewPrompt_SkillsSearchMode(t *testing.T) { bigSummary := strings.Repeat("x", 10000) + // 100 skills at the 200-rune description cap: ~5300 estimated tokens, past the 3000 + // inline ceiling. The estimate is taken from the skill list, not from the rendered + // summary, so the count and description length are what push it over. r := BuildPreviewPrompt(context.Background(), baseAgent(), PromptFull, "", PreviewDeps{ - SkillsLoader: &mockSkillsLoader{summary: bigSummary}, + SkillsLoader: &mockSkillsLoader{summary: bigSummary, infos: skillInfoN(100, 200)}, }) if strings.Contains(r.Prompt, bigSummary) { t.Error("expected large summary to be excluded (search-only mode)") } } +// The preview endpoint exists to show the prompt the agent actually gets. It used to +// decide inline-vs-search with tokencount.NewFallbackCounter() over the rendered XML +// while the runtime estimated name+description at chars/4 — on 18 real skills that read +// 3087 against 944, so the preview claimed search mode for an agent running inline. +// Both paths now call shouldInlineSkills; this pins them to the same verdict. +func TestPreviewInlineDecisionMatchesRuntime(t *testing.T) { + for _, tc := range []struct { + name string + count int + descLen int + wantInline bool + }{ + {"empty", 0, 0, false}, + {"one small skill", 1, 20, true}, + // Count gate, isolated: descriptions stay tiny so the token estimate is far + // under the ceiling and only the count can decide. + {"at the count ceiling", skillInlineMaxCount, 10, true}, + {"one past the count ceiling", skillInlineMaxCount + 1, 10, false}, + // Token gate, isolated: the count stays under the ceiling, so a false verdict + // can only come from the token estimate. 58 x (8 + 200 + 10) / 4 = 3161 > 3000. + {"under the count ceiling, under the token ceiling", 40, 200, true}, + {"under the count ceiling, over the token ceiling", 58, 200, false}, + } { + t.Run(tc.name, func(t *testing.T) { + infos := skillInfoN(tc.count, tc.descLen) + if got := shouldInlineSkills(infos); got != tc.wantInline { + t.Fatalf("shouldInlineSkills = %v, want %v", got, tc.wantInline) + } + + loader := &mockSkillsLoader{summary: "marker", infos: infos} + r := BuildPreviewPrompt(context.Background(), baseAgent(), PromptFull, "", PreviewDeps{SkillsLoader: loader}) + gotInPreview := strings.Contains(r.Prompt, "marker") + if gotInPreview != tc.wantInline { + t.Fatalf("preview inlined = %v, want %v (must match shouldInlineSkills)", gotInPreview, tc.wantInline) + } + }) + } +} + func TestBuildPreviewPrompt_PinnedSkillsHybrid(t *testing.T) { ag := baseAgent() ag.OtherConfig = []byte(`{"pinned_skills":["deploy"]}`) @@ -77,6 +121,7 @@ func TestBuildPreviewPrompt_SkillAllowList(t *testing.T) { ag := baseAgent() loader := &mockSkillsLoader{ summary: "ok", + infos: []skills.Info{{Slug: "allowed-skill", Name: "allowed", Description: "ok"}}, } r := BuildPreviewPrompt(context.Background(), ag, PromptFull, "user1", PreviewDeps{ SkillsLoader: loader, @@ -96,6 +141,7 @@ func TestBuildPreviewPrompt_SkillAccessStoreError(t *testing.T) { ag := baseAgent() loader := &mockSkillsLoader{ summary: "desc", + infos: skillInfoN(3, 20), } r := BuildPreviewPrompt(context.Background(), ag, PromptFull, "user1", PreviewDeps{ SkillsLoader: loader, @@ -104,8 +150,14 @@ func TestBuildPreviewPrompt_SkillAccessStoreError(t *testing.T) { if r.Prompt == "" { t.Fatal("expected non-empty prompt on SkillAccessStore error") } - if loader.capturedAllow == nil || len(loader.capturedAllow) != 0 { - t.Errorf("expected empty (non-nil) allow list on error, got %v", loader.capturedAllow) + // Fail closed: an access-store error must not fall back to showing every skill. + // The tag itself appears in the skill-loading protocol text regardless, so match on + // the summary body. + if strings.Contains(r.Prompt, "") { + t.Error("expected no skills in the prompt when the access store errors") + } + if loader.capturedAllow != nil { + t.Errorf("BuildSummary should not run for an empty allow list, got %v", loader.capturedAllow) } } diff --git a/internal/agent/preview_prompt_test_helpers_test.go b/internal/agent/preview_prompt_test_helpers_test.go index c0e68958..c5456134 100644 --- a/internal/agent/preview_prompt_test_helpers_test.go +++ b/internal/agent/preview_prompt_test_helpers_test.go @@ -2,7 +2,10 @@ package agent import ( "context" + "fmt" + "github.com/nextlevelbuilder/goclaw/internal/skills" "sort" + "strings" "github.com/google/uuid" @@ -46,16 +49,17 @@ type mockTool struct { desc string } -func (t *mockTool) Name() string { return t.name } -func (t *mockTool) Description() string { return t.desc } -func (t *mockTool) Parameters() map[string]any { return nil } +func (t *mockTool) Name() string { return t.name } +func (t *mockTool) Description() string { return t.desc } +func (t *mockTool) Parameters() map[string]any { return nil } func (t *mockTool) Execute(_ context.Context, _ map[string]any) *tools.Result { return nil } // mockSkillsLoader implements the widened SkillsLoader interface. type mockSkillsLoader struct { - pinned string // pre-built pinned XML - summary string // pre-built full summary - capturedAllow []string // set by BuildSummary for test assertions + pinned string // pre-built pinned XML + summary string // pre-built full summary + infos []skills.Info // what FilterSkills returns; drives inline-vs-search + capturedAllow []string // set by BuildSummary for test assertions } func (m *mockSkillsLoader) BuildPinnedSummary(_ context.Context, _ []string) string { @@ -67,6 +71,42 @@ func (m *mockSkillsLoader) BuildSummary(_ context.Context, allowList []string) s return m.summary } +// FilterSkills mirrors the real loader's allowList convention: nil means everything, +// an empty slice means nothing, a populated slice selects by slug. +func (m *mockSkillsLoader) FilterSkills(_ context.Context, allowList []string) []skills.Info { + if allowList == nil { + return m.infos + } + if len(allowList) == 0 { + return nil + } + allowed := make(map[string]bool, len(allowList)) + for _, s := range allowList { + allowed[s] = true + } + var out []skills.Info + for _, s := range m.infos { + if allowed[s.Slug] { + out = append(out, s) + } + } + return out +} + +// skillInfoN builds n skills whose combined name+description length puts the estimate +// either side of the inline threshold, depending on descLen. +func skillInfoN(n, descLen int) []skills.Info { + out := make([]skills.Info, 0, n) + for i := range n { + out = append(out, skills.Info{ + Slug: fmt.Sprintf("skill-%d", i), + Name: fmt.Sprintf("skill-%d", i), + Description: strings.Repeat("x", descLen), + }) + } + return out +} + // mockMCPLister implements MCPPreviewLister for testing. type mockMCPLister struct { tools []MCPToolPreviewInfo diff --git a/internal/agent/skill_slash_command_access_test.go b/internal/agent/skill_slash_command_access_test.go new file mode 100644 index 00000000..c5c14ebf --- /dev/null +++ b/internal/agent/skill_slash_command_access_test.go @@ -0,0 +1,98 @@ +package agent + +import ( + "context" + "strings" + "testing" + + "github.com/nextlevelbuilder/goclaw/internal/config" + "github.com/nextlevelbuilder/goclaw/internal/skills" +) + +func managedSkill(slug string) skills.Info { + return skills.Info{Slug: slug, Name: slug, Source: "managed", Description: slug + " description"} +} + +func fsSkill(slug, source string) skills.Info { + return skills.Info{Slug: slug, Name: slug, Source: source, Description: slug + " description"} +} + +// The inline block has always been filtered by visibility and agent +// grants, but slash activation read the loader's full list, so / could activate a +// managed skill the agent was never granted. +func TestSkillsReachableBySlash_GatesManagedSkills(t *testing.T) { + all := []skills.Info{managedSkill("granted"), managedSkill("ungranted")} + + if got := len(skillsReachableBySlash(all, nil)); got != 2 { + t.Errorf("nil allow list should not restrict, got %d skills", got) + } + if got := len(skillsReachableBySlash(all, []string{})); got != 0 { + t.Errorf("empty allow list should block every managed skill, got %d", got) + } + + only := skillsReachableBySlash(all, []string{"granted"}) + if len(only) != 1 || only[0].Slug != "granted" { + t.Errorf("expected only the granted skill, got %+v", only) + } +} + +// The allow list comes from a query over the `skills` table, so filesystem-tier skills +// have no row and would be filtered out by slug. Gating them would strand four of the +// loader's five tiers for slash while skill_search still reaches them unfiltered. +func TestSkillsReachableBySlash_LeavesFilesystemTiersAlone(t *testing.T) { + all := []skills.Info{ + managedSkill("managed-ungranted"), + fsSkill("workspace-skill", "workspace"), + fsSkill("project-skill", "agents-project"), + fsSkill("personal-skill", "agents-personal"), + fsSkill("global-skill", "global"), + fsSkill("builtin-skill", "builtin"), + } + + got := skillsReachableBySlash(all, []string{}) + var slugs []string + for _, s := range got { + slugs = append(slugs, s.Slug) + } + if len(got) != 5 { + t.Fatalf("expected the five non-managed skills to survive, got %v", slugs) + } + for _, s := range got { + if s.Source == "managed" { + t.Errorf("ungranted managed skill leaked through: %s", s.Slug) + } + } +} + +// End-to-end through the resolver: a managed skill outside the allow list must not +// activate, and must not be disclosed by the not-found suggestions, /list-skills or +// /help either — blocking activation alone would still leak the name and description. +func TestResolveSkillSlashCommand_ManagedSkillOutsideAllowList(t *testing.T) { + loader := newManagedSlashTestLoader(t) + cfg := config.SkillSlashCommandConfig{Enabled: new(true), Prefix: "/", SuggestNotFound: new(true)} + + granted := resolveSkillSlashCommand(context.Background(), loader, nil, cfg, "/managed-only build a page") + if granted.Kind != skillSlashCommandActivate { + t.Fatalf("nil allow list should activate, got kind=%v", granted.Kind) + } + + denied := resolveSkillSlashCommand(context.Background(), loader, []string{"something-else"}, cfg, "/managed-only build a page") + if denied.Kind == skillSlashCommandActivate { + t.Fatalf("managed skill outside the allow list must not activate") + } + + suggest := resolveSkillSlashCommand(context.Background(), loader, []string{}, cfg, "/managed-onl build") + if strings.Contains(suggest.Guidance, "managed-only") { + t.Errorf("suggestions disclosed an ungranted managed skill: %q", suggest.Guidance) + } + + list := resolveSkillSlashCommand(context.Background(), loader, []string{}, cfg, "/list-skills") + if strings.Contains(list.Guidance, "managed-only") { + t.Errorf("/list-skills disclosed an ungranted managed skill: %q", list.Guidance) + } + + help := resolveSkillSlashCommand(context.Background(), loader, []string{}, cfg, "/help managed-only") + if help.Kind == skillSlashCommandHelp { + t.Error("/help described an ungranted managed skill") + } +} diff --git a/internal/agent/skill_slash_command_matching.go b/internal/agent/skill_slash_command_matching.go index 68a4bb1c..c1505310 100644 --- a/internal/agent/skill_slash_command_matching.go +++ b/internal/agent/skill_slash_command_matching.go @@ -3,6 +3,8 @@ package agent import ( "sort" "strings" + "unicode" + "unicode/utf8" "github.com/nextlevelbuilder/goclaw/internal/skills" ) @@ -22,9 +24,7 @@ func parseSkillSlashCommand(message, prefix string) (parsedSkillSlashCommand, bo if after == "" || looksLikePath(after) { return parsedSkillSlashCommand{}, false } - first, rest, _ := strings.Cut(after, " ") - first = strings.TrimSpace(first) - rest = strings.TrimSpace(rest) + first, rest := cutFirstField(after) switch strings.ToLower(first) { case "list-skills": return parsedSkillSlashCommand{verb: "list-skills"}, true @@ -44,16 +44,38 @@ func parseSkillSlashCommand(message, prefix string) (parsedSkillSlashCommand, bo } func looksLikePath(value string) bool { - first, _, _ := strings.Cut(value, " ") + first, _ := cutFirstField(value) return strings.Contains(first, "/") || strings.Contains(first, "\\") || strings.Contains(first, ".") } -func firstWord(value string) (string, string) { - first, rest, ok := strings.Cut(strings.TrimSpace(value), " ") - if !ok { - return strings.TrimSpace(value), "" +// cutFirstField splits value at the first run of whitespace and returns the leading +// field plus the trimmed remainder. +// +// This replaces strings.Cut(value, " "), which split on a literal space only. A user who +// types the slash command and presses Enter before the rest of the message sends +// "/ck:git\nreview the diff"; the old split yielded the target "ck:git\nreview", which +// matches no skill. The reply was "skill not found" followed by near-matches that +// included the skill they had just named — the similarity fallback searched the mangled +// string and still landed beside it. Multi-line messages are normal in Slack and +// Telegram, so this was reachable in ordinary use. +func cutFirstField(value string) (string, string) { + value = strings.TrimSpace(value) + i := strings.IndexFunc(value, unicode.IsSpace) + if i < 0 { + return value, "" } - return strings.TrimSpace(first), strings.TrimSpace(rest) + return value[:i], strings.TrimSpace(value[i:]) +} + +// hasFieldPrefix reports whether raw begins with value followed by whitespace, i.e. +// value occupies a whole leading field rather than merely being a string prefix. +// Both arguments are expected to be lowercased by the caller. +func hasFieldPrefix(raw, value string) bool { + if !strings.HasPrefix(raw, value) || len(raw) == len(value) { + return false + } + r, _ := utf8.DecodeRuneInString(raw[len(value):]) + return unicode.IsSpace(r) } func matchSkillCommandTarget(all []skills.Info, raw string, partial bool) (skills.Info, bool, string) { @@ -69,7 +91,7 @@ func matchSkillCommandTarget(all []skills.Info, raw string, partial bool) (skill } var matches []candidate lowerRaw := strings.ToLower(raw) - partialTarget, partialRemainder := firstWord(raw) + partialTarget, partialRemainder := cutFirstField(raw) lowerPartialTarget := strings.ToLower(partialTarget) for _, skill := range all { for _, value := range []string{skill.Slug, skill.Name} { @@ -82,7 +104,7 @@ func matchSkillCommandTarget(all []skills.Info, raw string, partial bool) (skill matches = append(matches, candidate{info: skill, matchText: value, score: len([]rune(value))}) continue } - if strings.HasPrefix(lowerRaw, lowerValue+" ") { + if hasFieldPrefix(lowerRaw, lowerValue) { remainder := trimMatchedSkillCommandPrefix(raw, value) matches = append(matches, candidate{info: skill, matchText: value, remainder: remainder, score: len([]rune(value))}) continue diff --git a/internal/agent/skill_slash_command_parsing_test.go b/internal/agent/skill_slash_command_parsing_test.go new file mode 100644 index 00000000..98b97076 --- /dev/null +++ b/internal/agent/skill_slash_command_parsing_test.go @@ -0,0 +1,95 @@ +package agent + +import ( + "context" + "testing" + + "github.com/nextlevelbuilder/goclaw/internal/config" +) + +// The tokenizer used to split the slash command off with strings.Cut(after, " ") — a +// literal space. Typing the command and pressing Enter before the rest of the message +// produced the target "ck:git\nreview", which matches no skill, and the user was told the +// skill did not exist while it sat in the suggestion list right below. Multi-line +// messages are ordinary in Slack and Telegram, so this was reachable in normal use. +func TestParseSkillSlashCommand_SeparatorIsAnyWhitespace(t *testing.T) { + cfg := config.SkillSlashCommandConfig{Enabled: new(true), Prefix: "/"} + + for _, tc := range []struct { + name string + message string + wantTarget string + wantRest string + }{ + {"space", "/ck:git review the diff", "ck:git", "review the diff"}, + {"newline", "/ck:git\nreview the diff", "ck:git", "review the diff"}, + {"crlf", "/ck:git\r\nreview the diff", "ck:git", "review the diff"}, + {"blank line between", "/ck:git\n\nreview the diff", "ck:git", "review the diff"}, + {"tab", "/ck:git\treview the diff", "ck:git", "review the diff"}, + {"no arguments", "/ck:git", "ck:git", ""}, + {"trailing newline only", "/ck:git\n", "ck:git", ""}, + } { + t.Run(tc.name, func(t *testing.T) { + parsed, ok := parseSkillSlashCommand(tc.message, cfg.EffectivePrefix()) + if !ok { + t.Fatalf("parse failed for %q", tc.message) + } + if parsed.target != tc.wantTarget { + t.Errorf("target = %q, want %q", parsed.target, tc.wantTarget) + } + if parsed.rest != tc.wantRest { + t.Errorf("rest = %q, want %q", parsed.rest, tc.wantRest) + } + }) + } +} + +// The verb forms split on the same separator. +func TestParseSkillSlashCommand_VerbsAcceptNewline(t *testing.T) { + cfg := config.SkillSlashCommandConfig{Enabled: new(true), Prefix: "/"} + + parsed, ok := parseSkillSlashCommand("/help\nck:git", cfg.EffectivePrefix()) + if !ok || parsed.verb != "help" || parsed.target != "ck:git" { + t.Fatalf("help: got verb=%q target=%q ok=%v", parsed.verb, parsed.target, ok) + } + + parsed, ok = parseSkillSlashCommand("/use\nck:git and then stop", cfg.EffectivePrefix()) + if !ok || parsed.verb != "use" || parsed.rest != "ck:git and then stop" { + t.Fatalf("use: got verb=%q rest=%q ok=%v", parsed.verb, parsed.rest, ok) + } +} + +// A newline-separated command must reach the same skill as the space-separated one. +func TestResolveSkillSlashCommand_NewlineActivatesSameSkill(t *testing.T) { + loader := newSlashTestLoader(t) + cfg := config.SkillSlashCommandConfig{Enabled: new(true), Prefix: "/"} + + spaced := resolveSkillSlashCommand(context.Background(), loader, nil, cfg, "/frontend-design build a landing page") + newlined := resolveSkillSlashCommand(context.Background(), loader, nil, cfg, "/frontend-design\nbuild a landing page") + + if spaced.Kind != skillSlashCommandActivate { + t.Fatalf("space form did not activate: kind=%v", spaced.Kind) + } + if newlined.Kind != spaced.Kind { + t.Fatalf("newline form kind = %v, want %v", newlined.Kind, spaced.Kind) + } + if newlined.Skill.Slug != spaced.Skill.Slug { + t.Fatalf("newline form matched %q, want %q", newlined.Skill.Slug, spaced.Skill.Slug) + } + if newlined.RemainingPrompt != spaced.RemainingPrompt { + t.Fatalf("newline remainder = %q, want %q", newlined.RemainingPrompt, spaced.RemainingPrompt) + } +} + +// hasFieldPrefix must not treat a longer skill name as a match for a shorter one. +func TestMatchSkillCommandTarget_RequiresWholeField(t *testing.T) { + loader := newSlashTestLoader(t) + cfg := config.SkillSlashCommandConfig{Enabled: new(true), Prefix: "/"} + + // "frontend-design-extra" is not a skill; without partial matching this must not + // resolve to "frontend-design" merely because that is a string prefix of it. + res := resolveSkillSlashCommand(context.Background(), loader, nil, cfg, "/frontend-design-extra do a thing") + if res.Kind == skillSlashCommandActivate { + t.Fatalf("string-prefix match should not activate, got %q", res.Skill.Slug) + } +} diff --git a/internal/agent/skill_slash_commands.go b/internal/agent/skill_slash_commands.go index b9da5d97..6fb07291 100644 --- a/internal/agent/skill_slash_commands.go +++ b/internal/agent/skill_slash_commands.go @@ -29,7 +29,13 @@ type skillSlashCommandResult struct { } func (l *Loop) applySkillSlashCommand(ctx context.Context, req *RunRequest, message, extraPrompt string, skillFilter []string) (string, string, []string) { - result := resolveSkillSlashCommand(ctx, l.skillsLoader, l.resolveSkillSlashCommandConfig(ctx), message) + // l.skillAllowList is the visibility + grant filter computed when the agent was + // resolved (internal/agent/resolver.go). The inline block already + // honours it; the slash path did not, so / activated any skill the loader could + // see — including internal skills never granted to this agent, and skills belonging to + // another tenant's store if the loader had them. Filtering here also stops the + // not-found suggestions from disclosing that such a skill exists. + result := resolveSkillSlashCommand(ctx, l.skillsLoader, l.skillAllowList, l.resolveSkillSlashCommandConfig(ctx), message) if result.Kind == skillSlashCommandNone { return message, extraPrompt, skillFilter } @@ -76,7 +82,10 @@ func (l *Loop) resolveSkillSlashCommandConfig(ctx context.Context) config.SkillS return cfg } -func resolveSkillSlashCommand(ctx context.Context, loader *skills.Loader, cfg config.SkillSlashCommandConfig, message string) skillSlashCommandResult { +// resolveSkillSlashCommand matches a slash command against the skills this agent may +// use. allowList follows the loader's convention: nil means every skill, an empty slice +// means none, and a populated slice is an explicit set of slugs. +func resolveSkillSlashCommand(ctx context.Context, loader *skills.Loader, allowList []string, cfg config.SkillSlashCommandConfig, message string) skillSlashCommandResult { if loader == nil || !cfg.EffectiveEnabled() { return skillSlashCommandResult{Kind: skillSlashCommandNone} } @@ -84,7 +93,7 @@ func resolveSkillSlashCommand(ctx context.Context, loader *skills.Loader, cfg co if !ok { return skillSlashCommandResult{Kind: skillSlashCommandNone} } - all := loader.ListSkills(ctx) + all := skillsReachableBySlash(loader.ListSkills(ctx), allowList) switch parsed.verb { case "list-skills": return skillSlashCommandResult{Kind: skillSlashCommandList, Guidance: buildSkillSlashListGuidance(all)} @@ -101,6 +110,40 @@ func resolveSkillSlashCommand(ctx context.Context, loader *skills.Loader, cfg co } } +// skillsReachableBySlash applies the agent's allow list to the skills the DB governs, +// and lets the rest through. +// +// The allow list comes from SkillAccessStore.ListAccessible, which queries the `skills` +// table only (internal/store/pg/skills_grants.go:410). Filesystem-tier skills — the +// workspace, .agents and ~/.agents/~/.goclaw directories of the five-tier loader — have +// no row there, so filtering every skill against the list would make those four tiers +// unreachable by slash while `skill_search` still finds them +// (internal/tools/skill_search.go:81 calls ListSkills unfiltered). Gating only the +// managed tier closes the grant bypass without stranding skills an operator placed on +// disk deliberately. +// +// Builtin skills are seeded into the table with is_system = true and ListAccessible +// returns those unconditionally, so they stay reachable either way. +// +// A nil allow list means no restriction; an empty one means no managed skill is allowed. +func skillsReachableBySlash(all []skills.Info, allowList []string) []skills.Info { + if allowList == nil { + return all + } + allowed := make(map[string]bool, len(allowList)) + for _, slug := range allowList { + allowed[slug] = true + } + out := make([]skills.Info, 0, len(all)) + for _, s := range all { + if s.Source == "managed" && !allowed[s.Slug] { + continue + } + out = append(out, s) + } + return out +} + func resolveSkillActivation(ctx context.Context, loader *skills.Loader, all []skills.Info, raw string, cfg config.SkillSlashCommandConfig) skillSlashCommandResult { skill, matched, remainder := matchSkillCommandTarget(all, raw, cfg.EffectivePartialMatching()) if !matched { diff --git a/internal/agent/skill_slash_commands_test.go b/internal/agent/skill_slash_commands_test.go index d67c0d7b..be911cfc 100644 --- a/internal/agent/skill_slash_commands_test.go +++ b/internal/agent/skill_slash_commands_test.go @@ -9,11 +9,12 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/config" "github.com/nextlevelbuilder/goclaw/internal/skills" + "github.com/nextlevelbuilder/goclaw/internal/store" ) func TestResolveSkillSlashCommandExactSlug(t *testing.T) { loader := newSlashTestLoader(t) - result := resolveSkillSlashCommand(context.Background(), loader, config.SkillSlashCommandConfig{Enabled: boolPtr(true), Prefix: "/"}, "/frontend-design build a landing page") + result := resolveSkillSlashCommand(context.Background(), loader, nil, config.SkillSlashCommandConfig{Enabled: new(true), Prefix: "/"}, "/frontend-design build a landing page") if result.Kind != skillSlashCommandActivate { t.Fatalf("kind = %v, want activate", result.Kind) @@ -31,7 +32,7 @@ func TestResolveSkillSlashCommandExactSlug(t *testing.T) { func TestResolveSkillSlashCommandExactNameUseSyntax(t *testing.T) { loader := newSlashTestLoader(t) - result := resolveSkillSlashCommand(context.Background(), loader, config.SkillSlashCommandConfig{Enabled: boolPtr(true), Prefix: "/"}, "/use Frontend Design build a landing page") + result := resolveSkillSlashCommand(context.Background(), loader, nil, config.SkillSlashCommandConfig{Enabled: new(true), Prefix: "/"}, "/use Frontend Design build a landing page") if result.Kind != skillSlashCommandActivate { t.Fatalf("kind = %v, want activate", result.Kind) @@ -47,12 +48,12 @@ func TestResolveSkillSlashCommandExactNameUseSyntax(t *testing.T) { func TestResolveSkillSlashCommandPartialMatchRequiresUniqueEnabled(t *testing.T) { loader := newSlashTestLoader(t) - disabled := resolveSkillSlashCommand(context.Background(), loader, config.SkillSlashCommandConfig{Enabled: boolPtr(true), Prefix: "/"}, "/front build") + disabled := resolveSkillSlashCommand(context.Background(), loader, nil, config.SkillSlashCommandConfig{Enabled: new(true), Prefix: "/"}, "/front build") if disabled.Kind != skillSlashCommandUnknown { t.Fatalf("disabled partial kind = %v, want unknown", disabled.Kind) } - enabled := resolveSkillSlashCommand(context.Background(), loader, config.SkillSlashCommandConfig{Enabled: boolPtr(true), Prefix: "/", PartialMatching: true}, "/front build") + enabled := resolveSkillSlashCommand(context.Background(), loader, nil, config.SkillSlashCommandConfig{Enabled: new(true), Prefix: "/", PartialMatching: true}, "/front build") if enabled.Kind != skillSlashCommandActivate { t.Fatalf("enabled partial kind = %v, want activate", enabled.Kind) } @@ -64,7 +65,7 @@ func TestResolveSkillSlashCommandPartialMatchRequiresUniqueEnabled(t *testing.T) func TestResolveSkillSlashCommandFalsePositives(t *testing.T) { loader := newSlashTestLoader(t) for _, msg := range []string{"/home/user/project", "/etc/config.yaml", "https://example.com/path", "regular prompt"} { - result := resolveSkillSlashCommand(context.Background(), loader, config.SkillSlashCommandConfig{Enabled: boolPtr(true), Prefix: "/"}, msg) + result := resolveSkillSlashCommand(context.Background(), loader, nil, config.SkillSlashCommandConfig{Enabled: new(true), Prefix: "/"}, msg) if result.Kind != skillSlashCommandNone { t.Fatalf("%q kind = %v, want none", msg, result.Kind) } @@ -73,9 +74,9 @@ func TestResolveSkillSlashCommandFalsePositives(t *testing.T) { func TestResolveSkillSlashCommandListAndHelp(t *testing.T) { loader := newSlashTestLoader(t) - cfg := config.SkillSlashCommandConfig{Enabled: boolPtr(true), Prefix: "/"} + cfg := config.SkillSlashCommandConfig{Enabled: new(true), Prefix: "/"} - list := resolveSkillSlashCommand(context.Background(), loader, cfg, "/list-skills") + list := resolveSkillSlashCommand(context.Background(), loader, nil, cfg, "/list-skills") if list.Kind != skillSlashCommandList { t.Fatalf("list kind = %v, want list", list.Kind) } @@ -83,7 +84,7 @@ func TestResolveSkillSlashCommandListAndHelp(t *testing.T) { t.Fatalf("list guidance missing skills: %s", list.Guidance) } - help := resolveSkillSlashCommand(context.Background(), loader, cfg, "/help frontend-design") + help := resolveSkillSlashCommand(context.Background(), loader, nil, cfg, "/help frontend-design") if help.Kind != skillSlashCommandHelp { t.Fatalf("help kind = %v, want help", help.Kind) } @@ -91,7 +92,7 @@ func TestResolveSkillSlashCommandListAndHelp(t *testing.T) { t.Fatalf("unexpected help result: %#v", help) } - helpByName := resolveSkillSlashCommand(context.Background(), loader, cfg, "/help Frontend Design") + helpByName := resolveSkillSlashCommand(context.Background(), loader, nil, cfg, "/help Frontend Design") if helpByName.Kind != skillSlashCommandHelp { t.Fatalf("help by name kind = %v, want help", helpByName.Kind) } @@ -102,7 +103,7 @@ func TestResolveSkillSlashCommandListAndHelp(t *testing.T) { func TestResolveSkillSlashCommandSuggestsUnknown(t *testing.T) { loader := newSlashTestLoader(t) - result := resolveSkillSlashCommand(context.Background(), loader, config.SkillSlashCommandConfig{Enabled: boolPtr(true), Prefix: "/", SuggestNotFound: boolPtr(true)}, "/fronted build") + result := resolveSkillSlashCommand(context.Background(), loader, nil, config.SkillSlashCommandConfig{Enabled: new(true), Prefix: "/", SuggestNotFound: new(true)}, "/fronted build") if result.Kind != skillSlashCommandUnknown { t.Fatalf("kind = %v, want unknown", result.Kind) @@ -112,8 +113,9 @@ func TestResolveSkillSlashCommandSuggestsUnknown(t *testing.T) { } } +//go:fix inline func boolPtr(v bool) *bool { - return &v + return new(v) } func newSlashTestLoader(t *testing.T) *skills.Loader { @@ -125,6 +127,21 @@ func newSlashTestLoader(t *testing.T) *skills.Loader { return skills.NewLoader("", root, "") } +// newManagedSlashTestLoader builds a loader whose skills come from the managed +// (DB-backed) tier, so the allow-list gate applies to them. The filesystem tiers are +// deliberately left empty: skillsReachableBySlash only gates Source == "managed". +func newManagedSlashTestLoader(t *testing.T) *skills.Loader { + t.Helper() + t.Setenv("GOCLAW_DISABLE_PERSONAL_SKILLS", "1") + dataDir := t.TempDir() + storeDir := config.TenantSkillsStoreDir(dataDir, store.MasterTenantID, "") + // Managed layout is ///SKILL.md. + writeSkill(t, filepath.Join(storeDir, "managed-only"), "1", "managed-only", "A managed skill.", "Body.") + loader := skills.NewLoader("", "", "") + loader.SetManagedDir(dataDir) + return loader +} + func writeSkill(t *testing.T, root, slug, name, description, body string) { t.Helper() dir := filepath.Join(root, slug) diff --git a/internal/http/agents.go b/internal/http/agents.go index 8356ea34..5debc69b 100644 --- a/internal/http/agents.go +++ b/internal/http/agents.go @@ -20,6 +20,7 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/i18n" "github.com/nextlevelbuilder/goclaw/internal/permissions" "github.com/nextlevelbuilder/goclaw/internal/providers" + "github.com/nextlevelbuilder/goclaw/internal/skills" "github.com/nextlevelbuilder/goclaw/internal/store" "github.com/nextlevelbuilder/goclaw/internal/tools" "github.com/nextlevelbuilder/goclaw/pkg/protocol" @@ -106,6 +107,7 @@ type ToolPreviewLister interface { type SkillPreviewBuilder interface { BuildPinnedSummary(ctx context.Context, names []string) string BuildSummary(ctx context.Context, allowList []string) string + FilterSkills(ctx context.Context, allowList []string) []skills.Info } // SetPreviewDeps attaches optional dependencies for system prompt preview. diff --git a/internal/http/skills_import.go b/internal/http/skills_import.go index 57744701..166195df 100644 --- a/internal/http/skills_import.go +++ b/internal/http/skills_import.go @@ -83,6 +83,66 @@ type SkillsImportSummary struct { } // doSkillsImport parses the skills tar.gz and creates skills + writes files + applies grants. +// writeImportedSkillAuxFiles writes a skill's non-SKILL.md files under skillDir, +// preserving their directory structure. Paths are sanitised per segment, so an archive +// entry such as "../../etc/passwd" collapses to "etc/passwd" and stays inside skillDir. +// A file that cannot be written is logged and skipped: a missing reference degrades the +// skill, but it should not abort an import that is otherwise sound. +// reservedSkillFiles are handled by name earlier in the import and must never be +// written from the auxiliary set. sanitizeRelPath collapses "./SKILL.md" to "SKILL.md", +// and that name does not match the switch above (which compares the raw archive path), +// so without this guard a crafted entry would land in aux and then overwrite the +// SKILL.md that GuardSkillContent had already scanned — the scan would pass on one file +// while a different one reached disk. +var reservedSkillFiles = map[string]bool{ + "SKILL.md": true, + "metadata.json": true, + "grants.jsonl": true, +} + +// maxImportedAuxFiles bounds how many extra files one skill may carry. Real skills hold +// a handful of references; a four-figure count is an archive doing something else. +const maxImportedAuxFiles = 512 + +func writeImportedSkillAuxFiles(skillDir, slug string, aux map[string][]byte) { + written := 0 + skipped := 0 + for rel, data := range aux { + // Directory entries carry no content and a trailing separator. Writing them as + // empty files would, depending on map iteration order, occupy the name a real + // directory needs and silently drop everything beneath it. + if strings.HasSuffix(rel, "/") { + continue + } + clean := sanitizeRelPath(rel) + if clean == "" { + continue + } + if reservedSkillFiles[clean] { + slog.Warn("security.skills.import_reserved_path_rejected", "slug", slug, "entry", rel, "resolved", clean) + skipped++ + continue + } + if written >= maxImportedAuxFiles { + skipped++ + continue + } + dest := filepath.Join(skillDir, filepath.FromSlash(clean)) + if err := os.MkdirAll(filepath.Dir(dest), 0755); err != nil { + slog.Warn("skills.import: mkdir for skill file", "slug", slug, "path", clean, "error", err) + continue + } + if err := os.WriteFile(dest, data, 0644); err != nil { + slog.Warn("skills.import: write skill file", "slug", slug, "path", clean, "error", err) + continue + } + written++ + } + if skipped > 0 { + slog.Warn("skills.import: skill files skipped", "slug", slug, "written", written, "skipped", skipped, "limit", maxImportedAuxFiles) + } +} + func (h *SkillsHandler) doSkillsImport(ctx context.Context, r io.Reader, userID string, progressFn func(ProgressEvent)) (*SkillsImportSummary, error) { entries, err := readTarGzEntries(r) if err != nil { @@ -94,6 +154,9 @@ func (h *SkillsHandler) doSkillsImport(ctx context.Context, r io.Reader, userID metadata []byte skillMD []byte grants []byte + // aux holds every other file under skills//, keyed by its path relative + // to the skill root — references/errors.md, scripts/run.sh, assets/, and so on. + aux map[string][]byte } bySlug := make(map[string]*skillEntry) @@ -121,6 +184,16 @@ func (h *SkillsHandler) doSkillsImport(ctx context.Context, r io.Reader, userID bySlug[slug].skillMD = data case "grants.jsonl": bySlug[slug].grants = data + default: + // Everything else in the skill directory. The export side archives the whole + // tree (addSkillDirectoryToArchive), but import used to recognise only the + // three names above and silently discard the rest — so a skill whose SKILL.md + // cites references/*.md arrived without them and failed at the first read. + // The switch had no default, so nothing logged and the import reported success. + if bySlug[slug].aux == nil { + bySlug[slug].aux = make(map[string][]byte) + } + bySlug[slug].aux[file] = data } } @@ -190,6 +263,7 @@ func (h *SkillsHandler) doSkillsImport(ctx context.Context, r io.Reader, userID slog.Warn("skills.import: write SKILL.md", "slug", slug, "error", err) } } + writeImportedSkillAuxFiles(skillDir, slug, entry.aux) skillID = uuid.Must(uuid.NewV7()) visibility := meta.Visibility diff --git a/internal/http/skills_import_aux_files_test.go b/internal/http/skills_import_aux_files_test.go new file mode 100644 index 00000000..319499ee --- /dev/null +++ b/internal/http/skills_import_aux_files_test.go @@ -0,0 +1,197 @@ +package http + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "testing" +) + +// Export archives the whole skill directory, but import used to recognise only +// metadata.json, SKILL.md and grants.jsonl and silently discard everything else — the +// switch had no default. A skill whose SKILL.md cites references/*.md arrived without +// them, the import reported success, and the skill failed at the first read. +func TestWriteImportedSkillAuxFiles_PreservesTree(t *testing.T) { + dir := t.TempDir() + + aux := map[string][]byte{ + "references/errors.md": []byte("# Errors"), + "references/nested/deep.md": []byte("# Deep"), + "scripts/run.sh": []byte("#!/bin/sh\n"), + "assets/logo.svg": []byte(""), + } + writeImportedSkillAuxFiles(dir, "demo", aux) + + for rel, want := range aux { + got, err := os.ReadFile(filepath.Join(dir, filepath.FromSlash(rel))) + if err != nil { + t.Fatalf("%s: %v", rel, err) + } + if string(got) != string(want) { + t.Errorf("%s: content = %q, want %q", rel, got, want) + } + } +} + +// Archive entry names are attacker-controlled. Each segment is sanitised, so a traversal +// attempt collapses to a relative path and the write stays inside the skill directory. +func TestWriteImportedSkillAuxFiles_ContainsTraversal(t *testing.T) { + root := t.TempDir() + skillDir := filepath.Join(root, "skill") + if err := os.MkdirAll(skillDir, 0o755); err != nil { + t.Fatal(err) + } + outside := filepath.Join(root, "outside.txt") + + writeImportedSkillAuxFiles(skillDir, "demo", map[string][]byte{ + "../outside.txt": []byte("escaped"), + "../../outside.txt": []byte("escaped"), + "references/../../x.md": []byte("escaped"), + "./references/ok.md": []byte("fine"), + }) + + if _, err := os.Stat(outside); !os.IsNotExist(err) { + t.Fatalf("traversal escaped the skill directory: %v", err) + } + + // The sanitised forms land inside skillDir instead. + if _, err := os.Stat(filepath.Join(skillDir, "outside.txt")); err != nil { + t.Errorf("expected the sanitised path inside the skill dir: %v", err) + } + if got, err := os.ReadFile(filepath.Join(skillDir, "references", "ok.md")); err != nil || string(got) != "fine" { + t.Errorf("ordinary path broke: got %q err %v", got, err) + } + + // Nothing may sit above skillDir. + entries, err := os.ReadDir(root) + if err != nil { + t.Fatal(err) + } + for _, e := range entries { + if e.Name() != "skill" { + t.Errorf("unexpected entry written above the skill dir: %s", e.Name()) + } + } +} + +// A path that sanitises to nothing is skipped rather than written as the directory itself. +func TestWriteImportedSkillAuxFiles_SkipsEmptyPaths(t *testing.T) { + dir := t.TempDir() + writeImportedSkillAuxFiles(dir, "demo", map[string][]byte{ + "..": []byte("x"), + ".": []byte("x"), + "/": []byte("x"), + "///": []byte("x"), + }) + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + if len(entries) != 0 { + names := make([]string, 0, len(entries)) + for _, e := range entries { + names = append(names, e.Name()) + } + t.Errorf("expected nothing written, got %s", strings.Join(names, ", ")) + } +} + +// A nil or empty map is a no-op, not a panic — most skills have no extra files. +func TestWriteImportedSkillAuxFiles_NoFiles(t *testing.T) { + dir := t.TempDir() + writeImportedSkillAuxFiles(dir, "demo", nil) + writeImportedSkillAuxFiles(dir, "demo", map[string][]byte{}) + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + if len(entries) != 0 { + t.Errorf("expected an empty directory, got %d entries", len(entries)) + } +} + +// SKILL.md is scanned by GuardSkillContent before anything is written. The switch that +// routes archive entries compares the raw path, so "./SKILL.md" does not match it and +// lands in the auxiliary set — where sanitizeRelPath collapses it back to "SKILL.md". +// Writing that would replace the scanned file with an unscanned one. +func TestWriteImportedSkillAuxFiles_RefusesReservedNames(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte("scanned"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "metadata.json"), []byte(`{"ok":true}`), 0o644); err != nil { + t.Fatal(err) + } + + writeImportedSkillAuxFiles(dir, "demo", map[string][]byte{ + "./SKILL.md": []byte("unscanned"), + "SKILL.md/": []byte("unscanned"), + ".//SKILL.md": []byte("unscanned"), + "./metadata.json": []byte(`{"ok":false}`), + "./grants.jsonl": []byte("forged"), + "references/ok.md": []byte("fine"), + }) + + got, err := os.ReadFile(filepath.Join(dir, "SKILL.md")) + if err != nil || string(got) != "scanned" { + t.Errorf("SKILL.md was overwritten from the auxiliary set: got %q err %v", got, err) + } + got, err = os.ReadFile(filepath.Join(dir, "metadata.json")) + if err != nil || string(got) != `{"ok":true}` { + t.Errorf("metadata.json was overwritten: got %q err %v", got, err) + } + if _, err := os.Stat(filepath.Join(dir, "grants.jsonl")); !os.IsNotExist(err) { + t.Errorf("grants.jsonl was written from the auxiliary set: %v", err) + } + // An ordinary path in the same batch must still land. + if got, err := os.ReadFile(filepath.Join(dir, "references", "ok.md")); err != nil || string(got) != "fine" { + t.Errorf("ordinary path broke: got %q err %v", got, err) + } +} + +// A tar directory entry has a trailing separator and no content. Written as a file it +// would take the name a real directory needs, and whether that happens depends on map +// iteration order — so the failure would be intermittent. +func TestWriteImportedSkillAuxFiles_IgnoresDirectoryEntries(t *testing.T) { + for range 20 { + dir := t.TempDir() + writeImportedSkillAuxFiles(dir, "demo", map[string][]byte{ + "references/": {}, + "references/errors.md": []byte("# Errors"), + "assets/": {}, + "assets/logo.svg": []byte(""), + }) + + got, err := os.ReadFile(filepath.Join(dir, "references", "errors.md")) + if err != nil || string(got) != "# Errors" { + t.Fatalf("directory entry blocked a real file: got %q err %v", got, err) + } + info, err := os.Stat(filepath.Join(dir, "references")) + if err != nil || !info.IsDir() { + t.Fatalf("references should be a directory: %v", err) + } + } +} + +// An archive with an implausible number of extra files is capped, and the cap is logged +// rather than applied silently. +func TestWriteImportedSkillAuxFiles_CapsFileCount(t *testing.T) { + dir := t.TempDir() + aux := make(map[string][]byte, maxImportedAuxFiles+50) + for i := range maxImportedAuxFiles + 50 { + aux[fmt.Sprintf("references/f%d.md", i)] = []byte("x") + } + writeImportedSkillAuxFiles(dir, "demo", aux) + + entries, err := os.ReadDir(filepath.Join(dir, "references")) + if err != nil { + t.Fatal(err) + } + if len(entries) > maxImportedAuxFiles { + t.Errorf("wrote %d files, cap is %d", len(entries), maxImportedAuxFiles) + } + if len(entries) != maxImportedAuxFiles { + t.Errorf("expected exactly the cap to be written, got %d", len(entries)) + } +} diff --git a/internal/tools/security_policy_inherit.go b/internal/tools/security_policy_inherit.go new file mode 100644 index 00000000..099693e5 --- /dev/null +++ b/internal/tools/security_policy_inherit.go @@ -0,0 +1,75 @@ +package tools + +// Security policy inheritance for spawned subagents. +// +// A subagent gets a cloned registry, but the file and exec tools inside it are +// constructed fresh (cmd/gateway_agents.go). A fresh tool carries none of the hardening +// the gateway applied to the parent's instances at startup — no path denials, no +// exemptions, no shell deny-group toggles, no command keyword allowlist. The result is +// that spawning a subagent widens what the agent can reach: the parent is blocked from +// the data dir and the internal databases, the subagent is not. +// +// These methods copy that policy across so the subagent starts no wider than its parent. +// Inheriting from the live parent instance keeps one source of truth — a deny path added +// at startup, or reloaded later via config pub/sub, reaches subagents without a second +// wiring site that can drift. +// +// Allow-prefixes are inherited too: they are what make the parent's deny roots usable at +// all (the skills store sits under the denied data dir), so copying denials without them +// would leave the subagent unable to read the skills it is told to use. + +import "maps" + +// InheritSecurityPolicy copies path denials, deny exemptions, global shell deny-group +// toggles and the command keyword allowlist from parent onto t. +func (t *ExecTool) InheritSecurityPolicy(parent *ExecTool) { + if t == nil || parent == nil || t == parent { + return + } + // DenyPaths rebuilds the compiled patterns from the raw roots, so pass the roots + // rather than copying pathDenyPatterns — that keeps the slash-variant expansion and + // the pathDenyRoots bookkeeping in one place. + t.DenyPaths(parent.pathDenyRoots...) + t.AllowPathExemptions(parent.denyExemptions...) + + parent.policyMu.RLock() + groups := parent.globalDenyGroups + rules := slicesClone(parent.commandKeywordAllowlist) + parent.policyMu.RUnlock() + + if len(groups) > 0 { + copied := make(map[string]bool, len(groups)) + maps.Copy(copied, groups) + t.SetGlobalShellDenyGroups(copied) + } + if len(rules) > 0 { + t.SetCommandKeywordAllowlist(rules) + } +} + +// InheritPathPolicy copies allowed and denied path prefixes from parent onto t. +func (t *ReadFileTool) InheritPathPolicy(parent *ReadFileTool) { + if t == nil || parent == nil || t == parent { + return + } + t.AllowPaths(parent.allowedPrefixes...) + t.DenyPaths(parent.deniedPrefixes...) +} + +// InheritPathPolicy copies allowed and denied path prefixes from parent onto t. +func (t *WriteFileTool) InheritPathPolicy(parent *WriteFileTool) { + if t == nil || parent == nil || t == parent { + return + } + t.AllowPaths(parent.allowedPrefixes...) + t.DenyPaths(parent.deniedPrefixes...) +} + +// InheritPathPolicy copies allowed and denied path prefixes from parent onto t. +func (t *ListFilesTool) InheritPathPolicy(parent *ListFilesTool) { + if t == nil || parent == nil || t == parent { + return + } + t.AllowPaths(parent.allowedPrefixes...) + t.DenyPaths(parent.deniedPrefixes...) +}