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 <gaoyuan4396@gmail.com>

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

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())
 

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) {

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
+}

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))
 }

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
+// <workspace>/skills/<name>/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)
+	}
+}