From 169e0bafafda983b53ebdb9f884d7bf5e0204249 Mon Sep 17 00:00:00 2001 From: isaacgao4396 Date: Thu, 3 Sep 2026 19:13:56 +0800 Subject: [PATCH] fix(tools): use_skill inlines content when read_file isn't granted (#1547) * feat(store): add context helper for per-iteration tool allowlist * feat(pipeline): surface resolved tool allowlist to tool dispatch via context * fix(tools): use_skill inlines content when read_file isn't granted Fixes #1477 --------- Co-authored-by: isaacgao4396 --- cmd/gateway_setup.go | 2 +- internal/agent/usage_events_test.go | 2 +- internal/pipeline/tool_stage.go | 9 ++ internal/store/context.go | 22 +++++ internal/tools/use_skill.go | 29 +++++-- internal/tools/use_skill_test.go | 122 ++++++++++++++++++++++++++++ 6 files changed, 179 insertions(+), 7 deletions(-) create mode 100644 internal/tools/use_skill_test.go diff --git a/cmd/gateway_setup.go b/cmd/gateway_setup.go index f168fd0a..4de7e726 100644 --- a/cmd/gateway_setup.go +++ b/cmd/gateway_setup.go @@ -579,7 +579,7 @@ func setupSkillsSystem( skillsLoader := skills.NewLoader(workspace, globalSkillsDir, builtinSkillsDir) skillSearchTool := tools.NewSkillSearchTool(skillsLoader) toolsReg.Register(skillSearchTool) - toolsReg.Register(tools.NewUseSkillTool()) + toolsReg.Register(tools.NewUseSkillTool(skillsLoader)) slog.Info("skill_search tool registered", "skills", len(skillsLoader.ListSkills(context.Background()))) // Wire skills-store directory into filesystem loader so agents diff --git a/internal/agent/usage_events_test.go b/internal/agent/usage_events_test.go index 165878c2..0c80e159 100644 --- a/internal/agent/usage_events_test.go +++ b/internal/agent/usage_events_test.go @@ -100,7 +100,7 @@ func TestRecordToolUsageEvent_RuntimeAliasUsesCanonicalName(t *testing.T) { func TestRecordToolUsageEvent_UseSkillCountsSkillName(t *testing.T) { storeSpy := newFakeUsageEventStore() registry := tools.NewRegistry() - registry.Register(tools.NewUseSkillTool()) + registry.Register(tools.NewUseSkillTool(nil)) // Execute() not exercised by this test — nil loader is safe loop := &Loop{registry: registry, usageEvents: storeSpy, agentUUID: uuid.New(), tenantID: uuid.New()} ctx := tracing.WithTraceID(store.WithTenantID(t.Context(), loop.tenantID), uuid.New()) diff --git a/internal/pipeline/tool_stage.go b/internal/pipeline/tool_stage.go index b9b0c5ef..0dff59a3 100644 --- a/internal/pipeline/tool_stage.go +++ b/internal/pipeline/tool_stage.go @@ -51,6 +51,15 @@ func (s *ToolStage) Execute(ctx context.Context, state *RunState) error { return fmt.Errorf("ExecuteToolCall callback not configured") } + // Surface this iteration's resolved tool allowlist to the tools themselves, + // so a tool can introspect whether a sibling tool is available to the + // calling agent (e.g. use_skill inlining skill content when read_file + // isn't granted). Covers both the sequential and parallel dispatch paths + // below since both derive their ctx from this one. + if state.Tool.AllowedTools != nil { + ctx = store.WithAvailableToolNames(ctx, state.Tool.AllowedTools) + } + // Parallel path: separate I/O (parallel) from state mutation (sequential). // Requires both ExecuteToolRaw and ProcessToolResult callbacks. if len(toolCalls) > 1 && s.canExecuteParallel(toolCalls) && !s.batchExceedsBudget(state, toolCalls) { diff --git a/internal/store/context.go b/internal/store/context.go index 026b1c57..3eae62bc 100644 --- a/internal/store/context.go +++ b/internal/store/context.go @@ -49,6 +49,9 @@ const ( TenantSlugKey contextKey = "goclaw_tenant_slug" // RoleKey is the context key for the caller's permission role (e.g. "admin", "operator", "viewer"). RoleKey contextKey = "goclaw_role" + // AvailableToolNamesKey carries this iteration's policy-resolved tool allowlist + // (canonical registry names) so a tool can introspect its own sibling tools. + AvailableToolNamesKey contextKey = "goclaw_available_tool_names" // CredentialUserIDKey holds the resolved tenant user identity for credential lookups. // Falls back to UserIDFromContext if not set. CredentialUserIDKey contextKey = "goclaw_credential_user_id" @@ -496,3 +499,22 @@ func RoleFromContext(ctx context.Context) string { } return "" } + +// WithAvailableToolNames returns a new context carrying this iteration's +// policy-resolved tool allowlist, so a tool can check whether a sibling tool +// is available to the calling agent. +func WithAvailableToolNames(ctx context.Context, names map[string]bool) context.Context { + return context.WithValue(ctx, AvailableToolNamesKey, names) +} + +// AvailableToolNamesFromContext extracts the tool allowlist from context. +// Returns nil when not set — callers MUST treat nil as "no restriction known" +// (every tool available), matching the same nil convention already used by +// RunState.Tool.AllowedTools (see ThinkStage.Execute): nil means either the +// agent has no tool policy configured, or the caller never populated this key +// at all (e.g. a code path outside the pipeline's per-iteration tool dispatch). +// Never treat nil as "no tools available". +func AvailableToolNamesFromContext(ctx context.Context) map[string]bool { + v, _ := ctx.Value(AvailableToolNamesKey).(map[string]bool) + return v +} diff --git a/internal/tools/use_skill.go b/internal/tools/use_skill.go index cc92ee5a..99ebc051 100644 --- a/internal/tools/use_skill.go +++ b/internal/tools/use_skill.go @@ -4,15 +4,23 @@ import ( "context" "fmt" "log/slog" + + "github.com/nextlevelbuilder/goclaw/internal/skills" + "github.com/nextlevelbuilder/goclaw/internal/store" ) // UseSkillTool is a marker tool for observability. // It generates tool.call / tool.result events in spans and realtime -// so skill activation is visible in tracing. The actual skill content -// is still loaded via read_file — this tool is a deliberate no-op. -type UseSkillTool struct{} +// so skill activation is visible in tracing. For agents that have read_file, +// the actual skill content is still loaded via read_file — this tool stays a +// no-op for them. Agents without read_file in their resolved tool set (see +// store.AvailableToolNamesFromContext) have no way to follow up, so this tool +// inlines the skill content directly instead. +type UseSkillTool struct { + loader *skills.Loader +} -func NewUseSkillTool() *UseSkillTool { return &UseSkillTool{} } +func NewUseSkillTool(loader *skills.Loader) *UseSkillTool { return &UseSkillTool{loader: loader} } func (t *UseSkillTool) Name() string { return "use_skill" } @@ -37,7 +45,7 @@ func (t *UseSkillTool) Parameters() map[string]any { } } -func (t *UseSkillTool) Execute(_ context.Context, args map[string]any) *Result { +func (t *UseSkillTool) Execute(ctx context.Context, args map[string]any) *Result { name, _ := args["name"].(string) if name == "" { return ErrorResult("name parameter is required") @@ -45,5 +53,16 @@ func (t *UseSkillTool) Execute(_ context.Context, args map[string]any) *Result { slog.Info("skill.activated", "skill", name) + // nil AvailableToolNames means "no restriction known" (see + // store.AvailableToolNamesFromContext) — only inline when we can positively + // confirm read_file is missing from this agent's resolved tool set. + if available := store.AvailableToolNamesFromContext(ctx); available != nil && !available["read_file"] { + content, ok := t.loader.LoadSkill(ctx, name) + if !ok { + return ErrorResult(fmt.Sprintf("skill %q not found", name)) + } + return NewResult(content) + } + return NewResult(fmt.Sprintf("Skill %q activated. Proceed to read the skill's SKILL.md with read_file.", name)) } diff --git a/internal/tools/use_skill_test.go b/internal/tools/use_skill_test.go new file mode 100644 index 00000000..0438b567 --- /dev/null +++ b/internal/tools/use_skill_test.go @@ -0,0 +1,122 @@ +package tools + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/nextlevelbuilder/goclaw/internal/skills" + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +// newTestSkillLoader writes a single skill (name/SKILL.md) under a temp +// workspace and returns a Loader that resolves it via the flat +// /skills//SKILL.md path (see skills.Loader.LoadSkill). +func newTestSkillLoader(t *testing.T, name, body string) *skills.Loader { + t.Helper() + workspace := t.TempDir() + dir := filepath.Join(workspace, "skills", name) + if err := os.MkdirAll(dir, 0755); err != nil { + t.Fatalf("mkdir skill dir: %v", err) + } + content := "---\nname: " + name + "\ndescription: test skill\n---\n" + body + if err := os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte(content), 0644); err != nil { + t.Fatalf("write SKILL.md: %v", err) + } + return skills.NewLoader(workspace, "", "") +} + +// TestUseSkillTool_NoReadFile_InlinesContent covers issue #1477: an agent +// whose resolved tool set does not include read_file has no way to follow up +// on the "activated" message, so Execute must inline the skill body directly. +func TestUseSkillTool_NoReadFile_InlinesContent(t *testing.T) { + loader := newTestSkillLoader(t, "ck-plan", "SKILL BODY CONTENT") + tool := NewUseSkillTool(loader) + + restricted := map[string]bool{"web_search": true} // no read_file + ctx := store.WithAvailableToolNames(context.Background(), restricted) + + res := tool.Execute(ctx, map[string]any{"name": "ck-plan"}) + if res == nil { + t.Fatal("Execute returned nil result") + } + if res.IsError { + t.Fatalf("unexpected error result: %s", res.ForLLM) + } + if res.ForLLM != "SKILL BODY CONTENT" { + t.Fatalf("ForLLM = %q, want inlined skill body", res.ForLLM) + } +} + +// TestUseSkillTool_HasReadFile_BehaviorUnchanged is a regression check: an +// agent whose resolved tool set DOES include read_file must see the exact +// same "activated" message as before this change — byte for byte. +func TestUseSkillTool_HasReadFile_BehaviorUnchanged(t *testing.T) { + loader := newTestSkillLoader(t, "ck-plan", "SKILL BODY CONTENT") + tool := NewUseSkillTool(loader) + + allowed := map[string]bool{"read_file": true, "write_file": true} + ctx := store.WithAvailableToolNames(context.Background(), allowed) + + res := tool.Execute(ctx, map[string]any{"name": "ck-plan"}) + if res == nil { + t.Fatal("Execute returned nil result") + } + want := `Skill "ck-plan" activated. Proceed to read the skill's SKILL.md with read_file.` + if res.ForLLM != want { + t.Fatalf("ForLLM = %q, want %q", res.ForLLM, want) + } +} + +// TestUseSkillTool_NoAllowlistInContext_BehaviorUnchanged is the direction-2 +// nil case: when the context carries no tool allowlist at all (e.g. a call +// path outside the pipeline's per-iteration tool dispatch, or an agent with +// no tool policy configured — see store.AvailableToolNamesFromContext), the +// tool must NOT assume read_file is missing. It must keep the original +// two-step behavior, since nil here means "no restriction known", not +// "nothing is available". +func TestUseSkillTool_NoAllowlistInContext_BehaviorUnchanged(t *testing.T) { + loader := newTestSkillLoader(t, "ck-plan", "SKILL BODY CONTENT") + tool := NewUseSkillTool(loader) + + res := tool.Execute(context.Background(), map[string]any{"name": "ck-plan"}) + if res == nil { + t.Fatal("Execute returned nil result") + } + want := `Skill "ck-plan" activated. Proceed to read the skill's SKILL.md with read_file.` + if res.ForLLM != want { + t.Fatalf("ForLLM = %q, want %q (nil allowlist must not be treated as \"no tools available\")", res.ForLLM, want) + } +} + +// TestUseSkillTool_NoReadFile_SkillNotFound_ReturnsError covers the inlining +// path's failure mode: a restricted agent asking for a nonexistent skill must +// get a normal error result, not a panic or an empty inline. +func TestUseSkillTool_NoReadFile_SkillNotFound_ReturnsError(t *testing.T) { + loader := newTestSkillLoader(t, "ck-plan", "SKILL BODY CONTENT") + tool := NewUseSkillTool(loader) + + restricted := map[string]bool{"web_search": true} + ctx := store.WithAvailableToolNames(context.Background(), restricted) + + res := tool.Execute(ctx, map[string]any{"name": "does-not-exist"}) + if res == nil { + t.Fatal("Execute returned nil result") + } + if !res.IsError { + t.Fatalf("expected IsError, got ForLLM = %q", res.ForLLM) + } +} + +// TestUseSkillTool_EmptyName_ReturnsError is unchanged pre-existing behavior. +func TestUseSkillTool_EmptyName_ReturnsError(t *testing.T) { + tool := NewUseSkillTool(nil) + res := tool.Execute(context.Background(), map[string]any{}) + if res == nil { + t.Fatal("Execute returned nil result") + } + if !res.IsError { + t.Fatalf("expected IsError for missing name, got ForLLM = %q", res.ForLLM) + } +}