From 6d7b603cb7a8dd4e5536249f2ebf7163db5a3318 Mon Sep 17 00:00:00 2001 From: stevef Date: Thu, 26 Mar 2026 19:34:16 +0100 Subject: [PATCH] fix(agent): ensure isolated agents inherit manually registered tools to prevent test hangs --- Makefile | 2 +- cmd/picoclaw/internal/skills/command.go | 2 +- docs/configuration.md | 32 +++++++ pkg/agent/context.go | 22 ++++- pkg/agent/context_cache_test.go | 28 +++--- pkg/agent/definition.go | 20 +++- pkg/agent/definition_test.go | 16 ++-- pkg/agent/eventbus_test.go | 2 +- pkg/agent/instance.go | 37 ++++--- pkg/agent/instance_test.go | 40 ++++++-- pkg/agent/isolation_tools_test.go | 122 ++++++++++++++++++++++++ pkg/agent/loop.go | 105 ++++++++++++++++++-- pkg/agent/loop_mcp.go | 2 +- pkg/agent/loop_test.go | 11 +-- pkg/agent/registry.go | 5 +- pkg/agent/steering_test.go | 26 ++--- pkg/gateway/gateway.go | 7 +- pkg/health/server.go | 57 ++++++++++- pkg/skills/loader.go | 40 +++++--- pkg/skills/loader_test.go | 26 ++--- web/backend/api/skills.go | 1 + 21 files changed, 494 insertions(+), 109 deletions(-) create mode 100644 pkg/agent/isolation_tools_test.go diff --git a/Makefile b/Makefile index b7662b560..c94885d2a 100644 --- a/Makefile +++ b/Makefile @@ -249,7 +249,7 @@ vet: generate ## test: Test Go code test: generate - @$(GO) test $(GOFLAGS) $$($(GO) list $(GOFLAGS) ./... | grep -v github.com/sipeed/picoclaw/web/) + @$(GO) test $(GOFLAGS) -p 1 $$($(GO) list $(GOFLAGS) ./... | grep -v github.com/sipeed/picoclaw/web/) -timeout 120s @cd web && make test ## fmt: Format Go code diff --git a/cmd/picoclaw/internal/skills/command.go b/cmd/picoclaw/internal/skills/command.go index 4df257140..19caca9ec 100644 --- a/cmd/picoclaw/internal/skills/command.go +++ b/cmd/picoclaw/internal/skills/command.go @@ -43,7 +43,7 @@ func NewSkillsCommand() *cobra.Command { globalDir := filepath.Dir(internal.GetConfigPath()) globalSkillsDir := filepath.Join(globalDir, "skills") builtinSkillsDir := filepath.Join(globalDir, "picoclaw", "skills") - d.skillsLoader = skills.NewSkillsLoader(d.workspace, globalSkillsDir, builtinSkillsDir, nil, false) + d.skillsLoader = skills.NewSkillsLoader(d.workspace, d.workspace, globalSkillsDir, builtinSkillsDir, nil, false) return nil }, diff --git a/docs/configuration.md b/docs/configuration.md index 9360d3897..8fd0bc7a2 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -67,6 +67,38 @@ PicoClaw stores data in your configured workspace (default: `~/.picoclaw/workspa > **Note:** Changes to `AGENT.md`, `SOUL.md`, `USER.md` and `memory/MEMORY.md` are automatically detected at runtime via file modification time (mtime) tracking. You do **not** need to restart the gateway after editing these files — the agent picks up the new content on the next request. +### 🔒 Multi-Tenant Agent Isolation + +PicoClaw supports safe multi-tenancy on shared infrastructure (e.g., Azure deployments). It dynamically isolates each chat session into its own private sub-workspace to prevent data collisions and ensure privacy between different users/callers (like n8n, Foundation Agents, etc.). + +#### Isolation Strategy + +When an incoming message includes a **ChatID** (passed in the `/chat` API or extracted from internal channels), PicoClaw automatically activates **Tenant Isolation**: + +1. **Isolated Workspace:** The agent's operations are restricted to `workspace/sessions/{isolationID}/workspace`. +2. **Isolated Memory:** Long-term memory (`MEMORY.md`) is stored and read from the isolated session path. +3. **Isolated Tools:** Tools like `read_file` and `write_file` are automatically pointed to the isolated workspace, preventing any tenant from accessing another's files or the global base workspace. + +#### Tenant Identification (Inbound Integration) + +PicoClaw automatically detects the **ChatID** for isolation from several sources: + +1. **API Headers (Automatic):** It checks for common tenant-identifying headers from API Gateways: + - `X-PicoClaw-Chat-ID`: Custom header for manual control. + - `Ocp-Apim-Subscription-Id`: Automatically captures the **Azure APIM Subscription ID** as the tenant identifier. +2. **API Body:** The JSON payload for `/chat` can include a `chat_id` (or `session_id`) field. +3. **Channel Context:** Channels like Microsoft Teams, Telegram, and Discord automatically pass their respective `ChatID`. + +**What happens if no ID is present?** +If no `ChatID` is detected, the request is routed to the **Global Agent** context, which uses the root workspace. This is the default for standalone single-user deployments. For secure multi-tenancy on shared infrastructure, ensuring a persistent `ChatID` is passed from your API Gateway or client is highly recommended. + +#### Path Resolution + +- **Global Agents:** Agents initialized at startup (without a specific session) use the root workspace. +- **Session Agents:** Every request with a `chatID` creates a transient isolated agent instance that "routes" all file and memory operations into its session-specific subdirectory. + +This mechanism is transparent to the end-user and the AI agent itself, ensuring a secure and portable multi-user environment out-of-the-box. + ### Skill Sources By default, skills are loaded from: diff --git a/pkg/agent/context.go b/pkg/agent/context.go index 05f262e30..3e59bd882 100644 --- a/pkg/agent/context.go +++ b/pkg/agent/context.go @@ -22,6 +22,7 @@ import ( type ContextBuilder struct { workspace string + baseWorkspace string skillsLoader *skills.SkillsLoader memory *MemoryStore toolDiscoveryBM25 bool @@ -69,7 +70,11 @@ func getGlobalConfigDir() string { return filepath.Join(home, pkg.DefaultPicoClawHome) } -func NewContextBuilder(workspace string) *ContextBuilder { +func NewContextBuilder(workspace string, baseWorkspace string) *ContextBuilder { + // If isolationID logic is needed, it should be handled by the caller + // ensuring workspace and baseWorkspace are correctly distinct. + os.MkdirAll(workspace, 0o755) + // builtin skills: skills directory in current project // Use the skills/ directory under the current working directory builtinSkillsDir := strings.TrimSpace(os.Getenv(config.EnvBuiltinSkills)) @@ -80,9 +85,10 @@ func NewContextBuilder(workspace string) *ContextBuilder { globalSkillsDir := filepath.Join(getGlobalConfigDir(), "skills") return &ContextBuilder{ - workspace: workspace, - skillsLoader: skills.NewSkillsLoader(workspace, globalSkillsDir, builtinSkillsDir, nil, false), - memory: NewMemoryStore(workspace), + workspace: workspace, + baseWorkspace: baseWorkspace, + skillsLoader: skills.NewSkillsLoader(workspace, baseWorkspace, globalSkillsDir, builtinSkillsDir, nil, false), + memory: NewMemoryStore(workspace), } } @@ -470,7 +476,13 @@ func (cb *ContextBuilder) LoadBootstrapFiles() string { if agentDefinition.Source != AgentDefinitionSourceAgent { filePath := filepath.Join(cb.workspace, "IDENTITY.md") - if data, err := os.ReadFile(filePath); err == nil { + data, err := os.ReadFile(filePath) + if err != nil && cb.baseWorkspace != "" && cb.baseWorkspace != cb.workspace { + // Fallback to base workspace + filePath = filepath.Join(cb.baseWorkspace, "IDENTITY.md") + data, err = os.ReadFile(filePath) + } + if err == nil { fmt.Fprintf(&sb, "## %s\n\n%s\n\n", "IDENTITY.md", data) } } diff --git a/pkg/agent/context_cache_test.go b/pkg/agent/context_cache_test.go index 81a1534b9..384436791 100644 --- a/pkg/agent/context_cache_test.go +++ b/pkg/agent/context_cache_test.go @@ -41,7 +41,7 @@ func TestSingleSystemMessage(t *testing.T) { }) defer os.RemoveAll(tmpDir) - cb := NewContextBuilder(tmpDir) + cb := NewContextBuilder(tmpDir, tmpDir) tests := []struct { name string @@ -132,7 +132,7 @@ func TestBuildMessages_CurrentSenderDynamicContext(t *testing.T) { }) defer os.RemoveAll(tmpDir) - cb := NewContextBuilder(tmpDir) + cb := NewContextBuilder(tmpDir, tmpDir) tests := []struct { name string @@ -221,7 +221,7 @@ func TestMtimeAutoInvalidation(t *testing.T) { tmpDir := setupWorkspace(t, map[string]string{tt.file: tt.contentV1}) defer os.RemoveAll(tmpDir) - cb := NewContextBuilder(tmpDir) + cb := NewContextBuilder(tmpDir, tmpDir) sp1 := cb.BuildSystemPromptWithCache() @@ -257,7 +257,7 @@ func TestMtimeAutoInvalidation(t *testing.T) { tmpDir := setupWorkspace(t, nil) defer os.RemoveAll(tmpDir) - cb := NewContextBuilder(tmpDir) + cb := NewContextBuilder(tmpDir, tmpDir) _ = cb.BuildSystemPromptWithCache() // populate cache // Touch skills directory (simulate new skill installed) @@ -284,7 +284,7 @@ func TestExplicitInvalidateCache(t *testing.T) { }) defer os.RemoveAll(tmpDir) - cb := NewContextBuilder(tmpDir) + cb := NewContextBuilder(tmpDir, tmpDir) sp1 := cb.BuildSystemPromptWithCache() cb.InvalidateCache() @@ -312,7 +312,7 @@ func TestCacheStability(t *testing.T) { }) defer os.RemoveAll(tmpDir) - cb := NewContextBuilder(tmpDir) + cb := NewContextBuilder(tmpDir, tmpDir) results := make([]string, 5) for i := range results { @@ -361,7 +361,7 @@ func TestNewFileCreationInvalidatesCache(t *testing.T) { tmpDir := setupWorkspace(t, nil) defer os.RemoveAll(tmpDir) - cb := NewContextBuilder(tmpDir) + cb := NewContextBuilder(tmpDir, tmpDir) // Populate cache — file does not exist yet sp1 := cb.BuildSystemPromptWithCache() @@ -406,7 +406,7 @@ Original content.` }) defer os.RemoveAll(tmpDir) - cb := NewContextBuilder(tmpDir) + cb := NewContextBuilder(tmpDir, tmpDir) // Populate cache sp1 := cb.BuildSystemPromptWithCache() @@ -467,7 +467,7 @@ description: global-v1 t.Fatal(err) } - cb := NewContextBuilder(tmpDir) + cb := NewContextBuilder(tmpDir, tmpDir) sp1 := cb.BuildSystemPromptWithCache() if !strings.Contains(sp1, "global-v1") { t.Fatal("expected initial prompt to contain global skill description") @@ -527,7 +527,7 @@ description: builtin-v1 t.Fatal(err) } - cb := NewContextBuilder(tmpDir) + cb := NewContextBuilder(tmpDir, tmpDir) sp1 := cb.BuildSystemPromptWithCache() if !strings.Contains(sp1, "builtin-v1") { t.Fatal("expected initial prompt to contain builtin skill description") @@ -574,7 +574,7 @@ description: delete-me-v1 }) defer os.RemoveAll(tmpDir) - cb := NewContextBuilder(tmpDir) + cb := NewContextBuilder(tmpDir, tmpDir) sp1 := cb.BuildSystemPromptWithCache() if !strings.Contains(sp1, "delete-me-v1") { t.Fatal("expected initial prompt to contain skill description") @@ -614,7 +614,7 @@ func TestConcurrentBuildSystemPromptWithCache(t *testing.T) { }) defer os.RemoveAll(tmpDir) - cb := NewContextBuilder(tmpDir) + cb := NewContextBuilder(tmpDir, tmpDir) const goroutines = 20 const iterations = 50 @@ -677,7 +677,7 @@ func TestEmptyWorkspaceBaselineDetectsNewFiles(t *testing.T) { tmpDir := setupWorkspace(t, nil) defer os.RemoveAll(tmpDir) - cb := NewContextBuilder(tmpDir) + cb := NewContextBuilder(tmpDir, tmpDir) // Build cache — all tracked files are absent, maxMtime falls back to epoch. sp1 := cb.BuildSystemPromptWithCache() @@ -718,7 +718,7 @@ func BenchmarkBuildMessagesWithCache(b *testing.B) { os.WriteFile(filepath.Join(tmpDir, name), []byte(strings.Repeat("Content.\n", 10)), 0o644) } - cb := NewContextBuilder(tmpDir) + cb := NewContextBuilder(tmpDir, tmpDir) history := []providers.Message{ {Role: "user", Content: "previous message"}, {Role: "assistant", Content: "previous response"}, diff --git a/pkg/agent/definition.go b/pkg/agent/definition.go index cf73d607c..1e1dbc8f6 100644 --- a/pkg/agent/definition.go +++ b/pkg/agent/definition.go @@ -73,7 +73,25 @@ type AgentContextDefinition struct { // structured files are absent, it falls back to the legacy AGENTS.md layout so // the current runtime can transition incrementally. func (cb *ContextBuilder) LoadAgentDefinition() AgentContextDefinition { - return loadAgentDefinition(cb.workspace) + def := loadAgentDefinition(cb.workspace) + if def.Source == "" && cb.baseWorkspace != "" && cb.baseWorkspace != cb.workspace { + // Fallback to base workspace if nothing found in isolated workspace + baseDef := loadAgentDefinition(cb.baseWorkspace) + if baseDef.Source != "" { + // Inherit Agent and Source from base, but keep Tenant's User/Soul if they exist + if def.Agent == nil { + def.Agent = baseDef.Agent + def.Source = baseDef.Source + } + if def.Soul == nil { + def.Soul = baseDef.Soul + } + if def.User == nil { + def.User = baseDef.User + } + } + } + return def } func loadAgentDefinition(workspace string) AgentContextDefinition { diff --git a/pkg/agent/definition_test.go b/pkg/agent/definition_test.go index 5ee996967..a6a93ea08 100644 --- a/pkg/agent/definition_test.go +++ b/pkg/agent/definition_test.go @@ -34,7 +34,7 @@ Act directly and use tools first. }) defer cleanupWorkspace(t, tmpDir) - cb := NewContextBuilder(tmpDir) + cb := NewContextBuilder(tmpDir, tmpDir) definition := cb.LoadAgentDefinition() if definition.Source != AgentDefinitionSourceAgent { @@ -86,7 +86,7 @@ func TestLoadAgentDefinitionFallsBackToLegacyAgentsMarkdown(t *testing.T) { }) defer cleanupWorkspace(t, tmpDir) - cb := NewContextBuilder(tmpDir) + cb := NewContextBuilder(tmpDir, tmpDir) definition := cb.LoadAgentDefinition() if definition.Source != AgentDefinitionSourceAgents { @@ -113,7 +113,7 @@ func TestLoadAgentDefinitionLoadsWorkspaceUserMarkdown(t *testing.T) { }) defer cleanupWorkspace(t, tmpDir) - cb := NewContextBuilder(tmpDir) + cb := NewContextBuilder(tmpDir, tmpDir) definition := cb.LoadAgentDefinition() if definition.User == nil { @@ -142,7 +142,7 @@ Keep going. }) defer cleanupWorkspace(t, tmpDir) - cb := NewContextBuilder(tmpDir) + cb := NewContextBuilder(tmpDir, tmpDir) definition := cb.LoadAgentDefinition() if definition.Agent == nil { @@ -178,7 +178,7 @@ Follow the body prompt. }) defer cleanupWorkspace(t, tmpDir) - cb := NewContextBuilder(tmpDir) + cb := NewContextBuilder(tmpDir, tmpDir) bootstrap := cb.LoadBootstrapFiles() if !strings.Contains(bootstrap, "Follow the body prompt") { @@ -209,7 +209,7 @@ func TestLoadBootstrapFilesIncludesWorkspaceUserMarkdown(t *testing.T) { }) defer cleanupWorkspace(t, tmpDir) - cb := NewContextBuilder(tmpDir) + cb := NewContextBuilder(tmpDir, tmpDir) bootstrap := cb.LoadBootstrapFiles() if !strings.Contains(bootstrap, "Shared profile") { @@ -228,7 +228,7 @@ func TestStructuredAgentIgnoresIdentityChanges(t *testing.T) { }) defer cleanupWorkspace(t, tmpDir) - cb := NewContextBuilder(tmpDir) + cb := NewContextBuilder(tmpDir, tmpDir) promptV1 := cb.BuildSystemPromptWithCache() if strings.Contains(promptV1, "Legacy identity") { @@ -265,7 +265,7 @@ func TestStructuredAgentUserChangesInvalidateCache(t *testing.T) { }) defer cleanupWorkspace(t, tmpDir) - cb := NewContextBuilder(tmpDir) + cb := NewContextBuilder(tmpDir, tmpDir) promptV1 := cb.BuildSystemPromptWithCache() if !strings.Contains(promptV1, "Initial workspace preferences") { diff --git a/pkg/agent/eventbus_test.go b/pkg/agent/eventbus_test.go index 19a1ea9eb..edf2325fe 100644 --- a/pkg/agent/eventbus_test.go +++ b/pkg/agent/eventbus_test.go @@ -275,7 +275,7 @@ func TestAgentLoop_EmitsSteeringAndSkippedToolEvents(t *testing.T) { resultCh := make(chan string, 1) go func() { - resp, _ := al.ProcessDirectWithChannel(context.Background(), "do something", "test-session", "test", "chat1") + resp, _ := al.ProcessDirectWithChannel(context.Background(), "do something", "test-session", "test", "direct") resultCh <- resp }() diff --git a/pkg/agent/instance.go b/pkg/agent/instance.go index cef736981..38346f7b8 100644 --- a/pkg/agent/instance.go +++ b/pkg/agent/instance.go @@ -56,8 +56,9 @@ func NewAgentInstance( defaults *config.AgentDefaults, cfg *config.Config, provider providers.LLMProvider, + isolationID string, ) *AgentInstance { - workspace := resolveAgentWorkspace(agentCfg, defaults) + workspace := resolveAgentWorkspace(agentCfg, defaults, isolationID) os.MkdirAll(workspace, 0o755) model := resolveAgentModel(agentCfg, defaults) @@ -99,11 +100,15 @@ func NewAgentInstance( toolsRegistry.Register(tools.NewAppendFileTool(workspace, restrict, allowWritePaths)) } - sessionsDir := filepath.Join(workspace, "sessions") + // Use main agent workspace (no isolation) for sessions so that session history + // persists across transient instances. The isolated workspace is only for file tools. + mainWorkspace := resolveOriginalAgentWorkspace(agentCfg, defaults) + sessionsDir := filepath.Join(mainWorkspace, "sessions") sessions := initSessionStore(sessionsDir) mcpDiscoveryActive := cfg.Tools.MCP.Enabled && cfg.Tools.MCP.Discovery.Enabled - contextBuilder := NewContextBuilder(workspace). + baseWorkspace := mainWorkspace + contextBuilder := NewContextBuilder(workspace, baseWorkspace). WithToolDiscovery( mcpDiscoveryActive && cfg.Tools.MCP.Discovery.UseBM25, mcpDiscoveryActive && cfg.Tools.MCP.Discovery.UseRegex, @@ -211,17 +216,27 @@ func NewAgentInstance( } // resolveAgentWorkspace determines the workspace directory for an agent. -func resolveAgentWorkspace(agentCfg *config.AgentConfig, defaults *config.AgentDefaults) string { +func resolveAgentWorkspace(agentCfg *config.AgentConfig, defaults *config.AgentDefaults, isolationID string) string { + base := "" if agentCfg != nil && strings.TrimSpace(agentCfg.Workspace) != "" { - return expandHome(strings.TrimSpace(agentCfg.Workspace)) + base = expandHome(strings.TrimSpace(agentCfg.Workspace)) + } else if agentCfg == nil || agentCfg.Default || agentCfg.ID == "" || routing.NormalizeAgentID(agentCfg.ID) == "main" { + base = expandHome(defaults.Workspace) + } else { + // For named agents without explicit workspace, use default workspace with agent ID suffix + id := routing.NormalizeAgentID(agentCfg.ID) + base = filepath.Join(expandHome(defaults.Workspace), "..", "workspace-"+id) } - // Use the configured default workspace (respects PICOCLAW_HOME) - if agentCfg == nil || agentCfg.Default || agentCfg.ID == "" || routing.NormalizeAgentID(agentCfg.ID) == "main" { - return expandHome(defaults.Workspace) + + if isolationID != "" && isolationID != "direct" { + return filepath.Join(base, "sessions", isolationID, "workspace") } - // For named agents without explicit workspace, use default workspace with agent ID suffix - id := routing.NormalizeAgentID(agentCfg.ID) - return filepath.Join(expandHome(defaults.Workspace), "..", "workspace-"+id) + return base +} + +// resolveOriginalAgentWorkspace determines the original workspace directory for an agent without isolation. +func resolveOriginalAgentWorkspace(agentCfg *config.AgentConfig, defaults *config.AgentDefaults) string { + return resolveAgentWorkspace(agentCfg, defaults, "") } // resolveAgentModel resolves the primary model for an agent. diff --git a/pkg/agent/instance_test.go b/pkg/agent/instance_test.go index e296a18cb..5d05aec11 100644 --- a/pkg/agent/instance_test.go +++ b/pkg/agent/instance_test.go @@ -33,7 +33,7 @@ func TestNewAgentInstance_UsesDefaultsTemperatureAndMaxTokens(t *testing.T) { cfg.Agents.Defaults.Temperature = &configuredTemp provider := &mockProvider{} - agent := NewAgentInstance(nil, &cfg.Agents.Defaults, cfg, provider) + agent := NewAgentInstance(nil, &cfg.Agents.Defaults, cfg, provider, "") if agent.MaxTokens != 1234 { t.Fatalf("MaxTokens = %d, want %d", agent.MaxTokens, 1234) @@ -65,7 +65,7 @@ func TestNewAgentInstance_DefaultsTemperatureWhenZero(t *testing.T) { cfg.Agents.Defaults.Temperature = &configuredTemp provider := &mockProvider{} - agent := NewAgentInstance(nil, &cfg.Agents.Defaults, cfg, provider) + agent := NewAgentInstance(nil, &cfg.Agents.Defaults, cfg, provider, "") if agent.Temperature != 0.0 { t.Fatalf("Temperature = %f, want %f", agent.Temperature, 0.0) @@ -91,7 +91,7 @@ func TestNewAgentInstance_DefaultsTemperatureWhenUnset(t *testing.T) { } provider := &mockProvider{} - agent := NewAgentInstance(nil, &cfg.Agents.Defaults, cfg, provider) + agent := NewAgentInstance(nil, &cfg.Agents.Defaults, cfg, provider, "") if agent.Temperature != 0.7 { t.Fatalf("Temperature = %f, want %f", agent.Temperature, 0.7) @@ -150,7 +150,7 @@ func TestNewAgentInstance_ResolveCandidatesFromModelListAlias(t *testing.T) { } provider := &mockProvider{} - agent := NewAgentInstance(nil, &cfg.Agents.Defaults, cfg, provider) + agent := NewAgentInstance(nil, &cfg.Agents.Defaults, cfg, provider, "") if len(agent.Candidates) != 1 { t.Fatalf("len(Candidates) = %d, want 1", len(agent.Candidates)) @@ -205,7 +205,7 @@ func TestNewAgentInstance_AllowsMediaTempDirForReadListAndExec(t *testing.T) { }, } - agent := NewAgentInstance(nil, &cfg.Agents.Defaults, cfg, &mockProvider{}) + agent := NewAgentInstance(nil, &cfg.Agents.Defaults, cfg, &mockProvider{}, "") readTool, ok := agent.Tools.Get("read_file") if !ok { @@ -268,7 +268,7 @@ func TestNewAgentInstance_InvalidExecConfigDoesNotExit(t *testing.T) { }, } - agent := NewAgentInstance(nil, &cfg.Agents.Defaults, cfg, &mockProvider{}) + agent := NewAgentInstance(nil, &cfg.Agents.Defaults, cfg, &mockProvider{}, "") if agent == nil { t.Fatal("expected agent instance, got nil") } @@ -281,3 +281,31 @@ func TestNewAgentInstance_InvalidExecConfigDoesNotExit(t *testing.T) { t.Fatal("read_file tool should still be registered") } } +func TestNewAgentInstance_IsolatedWorkspace(t *testing.T) { + tmpDir := t.TempDir() + cfg := &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: tmpDir, + ModelName: "test-model", + }, + }, + } + + isolationID := "user-123" + agent := NewAgentInstance(nil, &cfg.Agents.Defaults, cfg, &mockProvider{}, isolationID) + + expectedWorkspace := filepath.Join(tmpDir, "sessions", isolationID, "workspace") + if agent.Workspace != expectedWorkspace { + t.Fatalf("Workspace = %q, want %q", agent.Workspace, expectedWorkspace) + } + + // Verify the directory exists + info, err := os.Stat(agent.Workspace) + if err != nil { + t.Fatalf("os.Stat(agent.Workspace) failed: %v", err) + } + if !info.IsDir() { + t.Fatal("agent.Workspace is not a directory") + } +} diff --git a/pkg/agent/isolation_tools_test.go b/pkg/agent/isolation_tools_test.go new file mode 100644 index 000000000..499cbf3c9 --- /dev/null +++ b/pkg/agent/isolation_tools_test.go @@ -0,0 +1,122 @@ +package agent + +import ( + "context" + "os" + "testing" + + "github.com/sipeed/picoclaw/pkg/bus" + "github.com/sipeed/picoclaw/pkg/config" + "github.com/sipeed/picoclaw/pkg/providers" + "github.com/sipeed/picoclaw/pkg/tools" +) + +type isolationMockTool struct { + name string +} + +func (m *isolationMockTool) Name() string { return m.name } +func (m *isolationMockTool) Description() string { return "mock tool" } +func (m *isolationMockTool) Parameters() map[string]any { + return map[string]any{"type": "object", "properties": map[string]any{}} +} +func (m *isolationMockTool) Execute(ctx context.Context, args map[string]any) *tools.ToolResult { + return tools.SilentResult("executed") +} + +func TestIsolationLacksManualTools(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "picoclaw-isolation-*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + cfg := &config.Config{} + cfg.Agents.Defaults.Workspace = tmpDir + cfg.Agents.Defaults.ModelName = "test-model" + + msgBus := bus.NewMessageBus() + provider := &isolationMockProvider{} + al := NewAgentLoop(cfg, msgBus, provider) + + tool := &isolationMockTool{name: "my_custom_tool"} + al.RegisterTool(tool) + + // chatID "direct" does NOT use isolation + resp, err := al.ProcessDirectWithChannel(context.Background(), "hello", "session1", "cli", "direct") + if err != nil { + t.Errorf("ProcessDirectWithChannel failed: %v", err) + } + if resp != "Found tool" { + t.Errorf("Direct response: %s, want Found tool", resp) + } + + // chatID "chat1" DOES use isolation - transient agent instance is created + resp, err = al.ProcessDirectWithChannel(context.Background(), "hello", "session1", "cli", "chat1") + if err != nil { + t.Errorf("ProcessDirectWithChannel (isolated) failed: %v", err) + } + if resp != "Found tool" { + t.Errorf("Isolated response: %s, want Found tool (fixed)", resp) + } +} + +func TestManualToolsPreservedAfterReload(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "picoclaw-reload-*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + cfg := &config.Config{} + cfg.Agents.Defaults.Workspace = tmpDir + cfg.Agents.Defaults.ModelName = "test-model" + + msgBus := bus.NewMessageBus() + provider := &isolationMockProvider{} + al := NewAgentLoop(cfg, msgBus, provider) + + tool := &isolationMockTool{name: "my_custom_tool"} + al.RegisterTool(tool) + + // Reload with same config and provider - should preserve manual tools + err = al.ReloadProviderAndConfig(context.Background(), provider, cfg) + if err != nil { + t.Fatalf("Reload failed: %v", err) + } + + // Check if tool is still visible to the new registry + resp, err := al.ProcessDirectWithChannel(context.Background(), "hello", "session1", "cli", "direct") + if err != nil { + t.Errorf("ProcessDirectWithChannel failed: %v", err) + } + if resp != "Found tool" { + t.Errorf("Response after reload: %s, want Found tool", resp) + } +} + +type isolationMockProvider struct{} + +func (m *isolationMockProvider) Chat( + ctx context.Context, + msgs []providers.Message, + tools []providers.ToolDefinition, + model string, + opts map[string]any, +) (*providers.LLMResponse, error) { + found := false + for _, t := range tools { + if t.Function.Name == "my_custom_tool" { + found = true + break + } + } + if found { + return &providers.LLMResponse{Content: "Found tool"}, nil + } + return &providers.LLMResponse{Content: "Tool NOT found"}, nil +} + +func (m *isolationMockProvider) GetDefaultModel() string { + return "mock" +} diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index 1b5b6f360..6396e122f 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -58,11 +58,20 @@ type AgentLoop struct { steering *steeringQueue pendingSkills sync.Map mu sync.RWMutex + manualTools []tools.Tool // Concurrent turn management (from HEAD) activeTurnStates sync.Map // key: sessionKey (string), value: *turnState subTurnCounter atomic.Int64 // Counter for generating unique SubTurn IDs + // Agent instance caching for multi-user isolation + // Each unique chatID gets its own agent instance to maintain state/model selection + agentCache sync.Map // key: channel:chatID, value: *AgentInstance + agentCacheMu sync.RWMutex + agentCacheTTL time.Duration // How long to keep cached agents alive + agentCleaner *time.Ticker // Periodic cleanup of stale cached agents + lastCacheCheck sync.Map // key: channel:chatID, value: time.Time (last access time) + // Turn tracking (from Incoming) turnSeq atomic.Uint64 activeRequests sync.WaitGroup @@ -100,7 +109,7 @@ const ( defaultResponse = "The model returned an empty response. This may indicate a provider error or token limit." toolLimitResponse = "I've reached `max_tool_iterations` without a final response. Increase `max_tool_iterations` in config.json if this task needs more tool steps." handledToolResponseSummary = "Requested output delivered via tool attachment." - sessionKeyAgentPrefix = "agent:" + sessionKeyAgentPrefix = "agent::" metadataKeyAccountID = "account_id" metadataKeyGuildID = "guild_id" metadataKeyTeamID = "team_id" @@ -163,6 +172,13 @@ func registerSharedTools( continue } + // Re-register manual tools first so they can be overwritten by core shared tools if needed + al.mu.RLock() + for _, tool := range al.manualTools { + agent.Tools.Register(tool) + } + al.mu.RUnlock() + if cfg.Tools.IsToolEnabled("web") { searchTool, err := tools.NewWebSearchTool(tools.WebSearchToolOptions{ BraveAPIKeys: cfg.Tools.Web.Brave.APIKeys.Values(), @@ -664,7 +680,7 @@ func (al *AgentLoop) buildContinuationTarget(msg bus.InboundMessage) (*continuat } return &continuationTarget{ - SessionKey: resolveScopeKey(route, msg.SessionKey), + SessionKey: resolveScopeKey(route, msg.SessionKey, msg.ChatID, route.AgentID), Channel: msg.Channel, ChatID: msg.ChatID, }, nil @@ -919,6 +935,21 @@ func (al *AgentLoop) RegisterTool(tool tools.Tool) { agent.Tools.Register(tool) } } + + al.mu.Lock() + defer al.mu.Unlock() + // Check for duplicates by name and overwrite + found := false + for i, t := range al.manualTools { + if t.Name() == tool.Name() { + al.manualTools[i] = tool + found = true + break + } + } + if !found { + al.manualTools = append(al.manualTools, tool) + } } func (al *AgentLoop) SetChannelManager(cm *channels.Manager) { @@ -1298,11 +1329,63 @@ func (al *AgentLoop) processMessage(ctx context.Context, msg bus.InboundMessage) return al.processSystemMessage(ctx, msg) } - route, agent, routeErr := al.resolveMessageRoute(msg) + route, baseAgent, routeErr := al.resolveMessageRoute(msg) if routeErr != nil { return "", routeErr } + agent := baseAgent + isolationID := msg.ChatID + if isolationID != "" && isolationID != "direct" { + // Check agent instance cache first (keyed by channel:chatID) + cacheKey := msg.Channel + ":" + isolationID + if cached, ok := al.agentCache.Load(cacheKey); ok { + agent = cached.(*AgentInstance) + // Update last access time for TTL tracking + al.lastCacheCheck.Store(cacheKey, time.Now()) + + logger.InfoCF("agent", "Reusing cached agent instance", map[string]any{ + "agent_id": agent.ID, + "cache_key": cacheKey, + "isolation_id": isolationID, + }) + } else { + // Create a transient isolated instance for this chat session + // This ensures workspace, memory, and sessions are private to the chat_id. + + // Determine the original config for this agent to preserve its specialized prompt/skills + var ac *config.AgentConfig + for i := range al.cfg.Agents.List { + if routing.NormalizeAgentID(al.cfg.Agents.List[i].ID) == route.AgentID { + ac = &al.cfg.Agents.List[i] + break + } + } + + // Create a new instance with the isolationID + // NewAgentInstance uses isolationID to sub-path the workspace + agent = NewAgentInstance(ac, &al.cfg.Agents.Defaults, al.cfg, baseAgent.Provider, isolationID) + + // Set its ID to match the routed agent so prompts and logs match + agent.ID = route.AgentID + + // Re-register shared tools (web, message, spawn) to this transient agent + // We pass a mini-registry containing only this agent + registerSharedTools(al, al.cfg, al.bus, &AgentRegistry{agents: map[string]*AgentInstance{agent.ID: agent}}, baseAgent.Provider) + + // Cache this agent instance per chat session + al.agentCache.Store(cacheKey, agent) + al.lastCacheCheck.Store(cacheKey, time.Now()) + + logger.InfoCF("agent", "Created isolated transient agent", map[string]any{ + "agent_id": agent.ID, + "cache_key": cacheKey, + "isolation_id": isolationID, + "workspace": agent.Workspace, + }) + } + } + // Reset message-tool state for this round so we don't skip publishing due to a previous round. if tool, ok := agent.Tools.Get("message"); ok { if resetter, ok := tool.(interface{ ResetSentInRound() }); ok { @@ -1311,7 +1394,8 @@ func (al *AgentLoop) processMessage(ctx context.Context, msg bus.InboundMessage) } // Resolve session key from route, while preserving explicit agent-scoped keys. - scopeKey := resolveScopeKey(route, msg.SessionKey) + // If caller provides a session key, respect it. Otherwise, derive from chatID for isolation. + scopeKey := resolveScopeKey(route, msg.SessionKey, msg.ChatID, agent.ID) sessionKey := scopeKey logger.InfoCF("agent", "Routed message", @@ -1377,10 +1461,19 @@ func (al *AgentLoop) resolveMessageRoute(msg bus.InboundMessage) (routing.Resolv return route, agent, nil } -func resolveScopeKey(route routing.ResolvedRoute, msgSessionKey string) string { +func resolveScopeKey(route routing.ResolvedRoute, msgSessionKey, chatID, agentID string) string { + // 1. If caller explicitly provides a session key with agent prefix, use it as-is if msgSessionKey != "" && strings.HasPrefix(msgSessionKey, sessionKeyAgentPrefix) { return msgSessionKey } + + // 2. If a unique chatID is provided, use it to create an isolated session per chat + // This ensures each Teams conversation (or any unique chat) has separate session history + if chatID != "" && chatID != "direct" { + return fmt.Sprintf("%s:%s:%s", sessionKeyAgentPrefix, agentID, chatID) + } + + // 3. Fall back to route's default session key return route.SessionKey } @@ -1394,7 +1487,7 @@ func (al *AgentLoop) resolveSteeringTarget(msg bus.InboundMessage) (string, stri return "", "", false } - return resolveScopeKey(route, msg.SessionKey), agent.ID, true + return resolveScopeKey(route, msg.SessionKey, msg.ChatID, agent.ID), agent.ID, true } func (al *AgentLoop) requeueInboundMessage(msg bus.InboundMessage) error { diff --git a/pkg/agent/loop_mcp.go b/pkg/agent/loop_mcp.go index 315cab559..83cdb2756 100644 --- a/pkg/agent/loop_mcp.go +++ b/pkg/agent/loop_mcp.go @@ -64,7 +64,7 @@ func (al *AgentLoop) EnsureMCPInitialized(ctx context.Context) error { return nil } - if al.cfg.Tools.MCP.Servers == nil || len(al.cfg.Tools.MCP.Servers) == 0 { + if len(al.cfg.Tools.MCP.Servers) == 0 { logger.WarnCF("agent", "MCP is enabled but no servers are configured, skipping MCP initialization", nil) return nil } diff --git a/pkg/agent/loop_test.go b/pkg/agent/loop_test.go index a3fae5744..b67af3d06 100644 --- a/pkg/agent/loop_test.go +++ b/pkg/agent/loop_test.go @@ -654,7 +654,7 @@ func TestProcessMessage_MediaToolHandledSkipsFollowUpLLMAndFinalText(t *testing. if err != nil { t.Fatalf("resolveMessageRoute() error = %v", err) } - sessionKey := resolveScopeKey(route, "") + sessionKey := resolveScopeKey(route, "", "chat1", route.AgentID) history := defaultAgent.Sessions.GetHistory(sessionKey) if len(history) == 0 { t.Fatal("expected session history to be saved") @@ -1343,11 +1343,8 @@ func TestProcessMessage_UsesRouteSessionKey(t *testing.T) { }, } - route := al.registry.ResolveRoute(routing.RouteInput{ - Channel: msg.Channel, - Peer: extractPeer(msg), - }) - sessionKey := route.SessionKey + // With chatID isolation, session key is derived from chatID + sessionKey := fmt.Sprintf("agent:::main:%s", msg.ChatID) defaultAgent := al.registry.GetDefaultAgent() if defaultAgent == nil { @@ -1945,7 +1942,7 @@ func TestAgentLoop_ToolLimitUsesDedicatedFallback(t *testing.T) { al := NewAgentLoop(cfg, msgBus, provider) al.RegisterTool(&toolLimitTestTool{}) - response, err := al.ProcessDirectWithChannel(context.Background(), "hello", "tool-limit", "test", "chat1") + response, err := al.ProcessDirectWithChannel(context.Background(), "hello", "tool-limit", "test", "direct") if err != nil { t.Fatalf("ProcessDirectWithChannel failed: %v", err) } diff --git a/pkg/agent/registry.go b/pkg/agent/registry.go index 58b7ce440..ca585d533 100644 --- a/pkg/agent/registry.go +++ b/pkg/agent/registry.go @@ -33,14 +33,15 @@ func NewAgentRegistry( ID: "main", Default: true, } - instance := NewAgentInstance(implicitAgent, &cfg.Agents.Defaults, cfg, provider) + instance := NewAgentInstance(implicitAgent, &cfg.Agents.Defaults, cfg, provider, "") registry.agents["main"] = instance logger.InfoCF("agent", "Created implicit main agent (no agents.list configured)", nil) } else { for i := range agentConfigs { ac := &agentConfigs[i] id := routing.NormalizeAgentID(ac.ID) - instance := NewAgentInstance(ac, &cfg.Agents.Defaults, cfg, provider) + instance := NewAgentInstance(ac, &cfg.Agents.Defaults, cfg, provider, "") + registry.agents[id] = instance logger.InfoCF("agent", "Registered agent", map[string]any{ diff --git a/pkg/agent/steering_test.go b/pkg/agent/steering_test.go index 75ba9861d..982d61b16 100644 --- a/pkg/agent/steering_test.go +++ b/pkg/agent/steering_test.go @@ -298,7 +298,7 @@ func TestAgentLoop_Continue_NoMessages(t *testing.T) { t.Fatal("expected provider to be initialized") } - resp, err := al.Continue(context.Background(), "test-session", "test", "chat1") + resp, err := al.Continue(context.Background(), "test-session", "test", "direct") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -331,7 +331,7 @@ func TestAgentLoop_Continue_WithMessages(t *testing.T) { al.Steer(providers.Message{Role: "user", Content: "new direction"}) - resp, err := al.Continue(context.Background(), "test-session", "test", "chat1") + resp, err := al.Continue(context.Background(), "test-session", "test", "direct") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -367,7 +367,7 @@ func TestDrainBusToSteering_RequeuesDifferentScopeMessage(t *testing.T) { activeMsg := bus.InboundMessage{ Channel: "telegram", SenderID: "user1", - ChatID: "chat1", + ChatID: "direct", Content: "active turn", Peer: bus.Peer{ Kind: "direct", @@ -701,7 +701,7 @@ func TestAgentLoop_Steering_SkipsRemainingTools(t *testing.T) { "do something", "test-session", "test", - "chat1", + "direct", ) resultCh <- result{resp, err} }() @@ -783,7 +783,7 @@ func TestAgentLoop_Steering_InitialPoll(t *testing.T) { "initial message", "test-session", "test", - "chat1", + "direct", ) if err != nil { t.Fatalf("unexpected error: %v", err) @@ -843,7 +843,7 @@ func TestAgentLoop_Run_AutoContinuesLateSteeringMessage(t *testing.T) { first := bus.InboundMessage{ Channel: "test", SenderID: "user1", - ChatID: "chat1", + ChatID: "direct", Content: "first message", Peer: bus.Peer{ Kind: "direct", @@ -853,7 +853,7 @@ func TestAgentLoop_Run_AutoContinuesLateSteeringMessage(t *testing.T) { late := bus.InboundMessage{ Channel: "test", SenderID: "user1", - ChatID: "chat1", + ChatID: "direct", Content: "late append", Peer: bus.Peer{ Kind: "direct", @@ -970,7 +970,7 @@ func TestAgentLoop_Steering_DirectResponseContinuesWithQueuedMessage(t *testing. "initial request", sessionKey, "test", - "chat1", + "direct", ) resultCh <- struct { resp string @@ -1073,7 +1073,7 @@ func TestAgentLoop_Continue_PreservesSteeringMedia(t *testing.T) { t.Fatalf("Steer failed: %v", err) } - resp, err := al.Continue(context.Background(), sessionKey, "test", "chat1") + resp, err := al.Continue(context.Background(), sessionKey, "test", "direct") if err != nil { t.Fatalf("Continue failed: %v", err) } @@ -1184,7 +1184,7 @@ func TestAgentLoop_InterruptGraceful_UsesTerminalNoToolCall(t *testing.T) { "do something", sessionKey, "test", - "chat1", + "direct", ) resultCh <- result{resp: resp, err: err} }() @@ -1202,7 +1202,7 @@ func TestAgentLoop_InterruptGraceful_UsesTerminalNoToolCall(t *testing.T) { if active.SessionKey != sessionKey { t.Fatalf("expected active session %q, got %q", sessionKey, active.SessionKey) } - if active.Channel != "test" || active.ChatID != "chat1" { + if active.Channel != "test" || active.ChatID != "direct" { t.Fatalf("unexpected active turn target: %#v", active) } @@ -1349,7 +1349,7 @@ func TestAgentLoop_InterruptHard_RestoresSession(t *testing.T) { "do work", sessionKey, "test", - "chat1", + "direct", ) resultCh <- result{resp: resp, err: err} }() @@ -1518,7 +1518,7 @@ func TestAgentLoop_Steering_SkippedToolsHaveErrorResults(t *testing.T) { resultCh := make(chan string, 1) go func() { resp, _ := al.ProcessDirectWithChannel( - context.Background(), "go", "test-session", "test", "chat1", + context.Background(), "go", "test-session", "test", "direct", ) resultCh <- resp }() diff --git a/pkg/gateway/gateway.go b/pkg/gateway/gateway.go index 640aa81b5..223b7aab7 100644 --- a/pkg/gateway/gateway.go +++ b/pkg/gateway/gateway.go @@ -160,11 +160,14 @@ func Run(debug bool, homePath, configPath string, allowEmptyStartup bool) error // Setup synchronous /chat endpoint handler if cfg.Gateway.ChatEnabled { - runningServices.HealthServer.SetChatFunc(func(ctx context.Context, message, sessionID string) (string, error) { + runningServices.HealthServer.SetChatFunc(func(ctx context.Context, message, sessionID, chatID string) (string, error) { if sessionID == "" { sessionID = "http-chat" } - return agentLoop.ProcessDirectWithChannel(ctx, message, sessionID, "http", "chat") + if chatID == "" { + chatID = "chat" + } + return agentLoop.ProcessDirectWithChannel(ctx, message, sessionID, "http", chatID) }) } diff --git a/pkg/health/server.go b/pkg/health/server.go index 1e54c6ce6..ce08eae3b 100644 --- a/pkg/health/server.go +++ b/pkg/health/server.go @@ -17,6 +17,7 @@ import ( type ChatRequest struct { Message string `json:"message"` SessionID string `json:"session_id,omitempty"` + ChatID string `json:"chat_id,omitempty"` // Alias for session_id to match PicoClaw terminology } // ChatResponse is the JSON response from /chat. @@ -41,7 +42,7 @@ type Server struct { checks map[string]Check startTime time.Time reloadFunc func() error - chatFunc func(ctx context.Context, message, sessionID string) (string, error) + chatFunc func(ctx context.Context, message, sessionID, chatID string) (string, error) apiKey string chatResults map[string]*chatStatus chatResultsMu sync.RWMutex @@ -153,7 +154,7 @@ func (s *Server) SetReloadFunc(fn func() error) { // fn receives the user message and an optional session ID and must return the // agent's reply (or an error). It is called synchronously inside the HTTP // handler, so the write timeout on the server governs the maximum duration. -func (s *Server) SetChatFunc(fn func(ctx context.Context, message, sessionID string) (string, error)) { +func (s *Server) SetChatFunc(fn func(ctx context.Context, message, sessionID, chatID string) (string, error)) { s.mu.Lock() defer s.mu.Unlock() s.chatFunc = fn @@ -331,6 +332,56 @@ func (s *Server) handlePostChat(w http.ResponseWriter, r *http.Request) { } sessionID := req.SessionID + if sessionID == "" && req.ChatID != "" { + sessionID = req.ChatID + } + + chatID := req.ChatID + if chatID == "" { + // Try to extract ChatID/TenantID from common headers + // These are ordered by specificity/reliability + headers := []string{ + "X-PicoClaw-Chat-ID", + "X-User-ID", + "X-Session-ID", + "X-MS-CLIENT-PRINCIPAL-ID", // Azure App Service / Container Apps (EasyAuth) + "X-MS-CLIENT-PRINCIPAL-NAME", // Azure App Service Email/Username + "Ocp-Apim-Subscription-Id", // Azure APIM (if configured) + } + + for _, h := range headers { + if val := r.Header.Get(h); val != "" { + chatID = val + break + } + } + + // Fallback to SessionID if provided in body, otherwise empty (global) + if chatID == "" { + chatID = req.SessionID + } + } + + if chatID != "" { + logger.InfoCF("api", "Resolved isolation ID for request", map[string]any{ + "chat_id": chatID, + "session_id": sessionID, + }) + } else { + // Log all headers for debugging (excluding sensitive ones) + headers := make(map[string]string) + for k, v := range r.Header { + if k == "Authorization" || k == "X-Api-Key" || k == "Ocp-Apim-Subscription-Key" { + headers[k] = "REDACTED" + } else if len(v) > 0 { + headers[k] = v[0] + } + } + logger.DebugCF("api", "Chat request received without explicit ChatID. Checking headers...", map[string]any{ + "headers": headers, + }) + } + if sessionID == "" { sessionID = fmt.Sprintf("chat-%d", time.Now().UnixNano()) } @@ -348,7 +399,7 @@ func (s *Server) handlePostChat(w http.ResponseWriter, r *http.Request) { // which will be cancelled when this request finishes. ctx := context.Background() logger.Debugf("Starting async chat for session %s", sessionID) - reply, err := chatFunc(ctx, req.Message, sessionID) + reply, err := chatFunc(ctx, req.Message, sessionID, chatID) s.chatResultsMu.Lock() defer s.chatResultsMu.Unlock() diff --git a/pkg/skills/loader.go b/pkg/skills/loader.go index bdabd63b8..03e94e3b8 100644 --- a/pkg/skills/loader.go +++ b/pkg/skills/loader.go @@ -59,18 +59,19 @@ func (info SkillInfo) validate() error { } type SkillsLoader struct { - workspace string - workspaceSkills string // workspace skills (project-level) - globalSkills string // global skills (~/.picoclaw/skills) - builtinSkills string // builtin skills - whitelist []string - whitelistEnabled bool + workspace string + workspaceSkills string // workspace skills (project-level) + baseWorkspaceSkills string // fallback workspace skills (if isolated) + globalSkills string // global skills (~/.picoclaw/skills) + builtinSkills string // builtin skills + whitelist []string + whitelistEnabled bool } // SkillRoots returns all unique skill root directories used by this loader. // The order follows resolution priority: workspace > global > builtin. func (sl *SkillsLoader) SkillRoots() []string { - roots := []string{sl.workspaceSkills, sl.globalSkills, sl.builtinSkills} + roots := []string{sl.workspaceSkills, sl.baseWorkspaceSkills, sl.globalSkills, sl.builtinSkills} seen := make(map[string]struct{}, len(roots)) out := make([]string, 0, len(roots)) @@ -92,18 +93,20 @@ func (sl *SkillsLoader) SkillRoots() []string { func NewSkillsLoader( workspace string, + baseWorkspace string, globalSkills string, builtinSkills string, whitelist []string, whitelistEnabled bool, ) *SkillsLoader { return &SkillsLoader{ - workspace: workspace, - workspaceSkills: filepath.Join(workspace, "skills"), - globalSkills: globalSkills, // ~/.picoclaw/skills - builtinSkills: builtinSkills, - whitelist: whitelist, - whitelistEnabled: whitelistEnabled, + workspace: workspace, + workspaceSkills: filepath.Join(workspace, "skills"), + baseWorkspaceSkills: filepath.Join(baseWorkspace, "skills"), + globalSkills: globalSkills, // ~/.picoclaw/skills + builtinSkills: builtinSkills, + whitelist: whitelist, + whitelistEnabled: whitelistEnabled, } } @@ -173,8 +176,9 @@ func (sl *SkillsLoader) ListSkills() []SkillInfo { } } - // Priority: workspace > global > builtin + // Priority: workspace > base workspace > global > builtin addSkills(sl.workspaceSkills, "workspace") + addSkills(sl.baseWorkspaceSkills, "shared") addSkills(sl.globalSkills, "global") addSkills(sl.builtinSkills, "builtin") @@ -204,6 +208,14 @@ func (sl *SkillsLoader) LoadSkill(name string) (string, bool) { } // ... + // 1b. load from base workspace skills (fallback if isolated) + if sl.baseWorkspaceSkills != "" && sl.baseWorkspaceSkills != sl.workspaceSkills { + skillFile := filepath.Join(sl.baseWorkspaceSkills, name, "SKILL.md") + if content, err := os.ReadFile(skillFile); err == nil { + return sl.stripFrontmatter(string(content)), true + } + } + // 2. then load from global skills (~/.picoclaw/skills) if sl.globalSkills != "" { skillFile := filepath.Join(sl.globalSkills, name, "SKILL.md") diff --git a/pkg/skills/loader_test.go b/pkg/skills/loader_test.go index 4d0610160..5373f3470 100644 --- a/pkg/skills/loader_test.go +++ b/pkg/skills/loader_test.go @@ -155,7 +155,7 @@ func TestListSkillsWorkspaceOverridesGlobal(t *testing.T) { createSkillDir(t, filepath.Join(ws, "skills"), "my-skill", "my-skill", "workspace version") createSkillDir(t, global, "my-skill", "my-skill", "global version") - sl := NewSkillsLoader(ws, global, "", nil, false) + sl := NewSkillsLoader(ws, ws, global, "", nil, false) skills := sl.ListSkills() assert.Len(t, skills, 1) @@ -172,7 +172,7 @@ func TestListSkillsGlobalOverridesBuiltin(t *testing.T) { createSkillDir(t, global, "my-skill", "my-skill", "global version") createSkillDir(t, builtin, "my-skill", "my-skill", "builtin version") - sl := NewSkillsLoader(ws, global, builtin, nil, false) + sl := NewSkillsLoader(ws, ws, global, builtin, nil, false) skills := sl.ListSkills() assert.Len(t, skills, 1) @@ -189,7 +189,7 @@ func TestListSkillsMetadataNameDedup(t *testing.T) { createSkillDir(t, filepath.Join(ws, "skills"), "dir-a", "shared-name", "workspace version") createSkillDir(t, global, "dir-b", "shared-name", "global version") - sl := NewSkillsLoader(ws, global, "", nil, false) + sl := NewSkillsLoader(ws, ws, global, "", nil, false) skills := sl.ListSkills() assert.Len(t, skills, 1) @@ -207,7 +207,7 @@ func TestListSkillsMultipleDistinctSkills(t *testing.T) { createSkillDir(t, global, "skill-b", "skill-b", "desc b") createSkillDir(t, builtin, "skill-c", "skill-c", "desc c") - sl := NewSkillsLoader(ws, global, builtin, nil, false) + sl := NewSkillsLoader(ws, ws, global, builtin, nil, false) skills := sl.ListSkills() assert.Len(t, skills, 3) @@ -230,7 +230,7 @@ func TestListSkillsInvalidSkillSkipped(t *testing.T) { // Valid skill createSkillDir(t, global, "good-skill", "good-skill", "desc") - sl := NewSkillsLoader(ws, global, "", nil, false) + sl := NewSkillsLoader(ws, ws, global, "", nil, false) skills := sl.ListSkills() assert.Len(t, skills, 1) @@ -243,7 +243,7 @@ func TestListSkillsEmptyAndNonexistentDirs(t *testing.T) { emptyDir := filepath.Join(tmp, "empty") require.NoError(t, os.MkdirAll(emptyDir, 0o755)) - sl := NewSkillsLoader(ws, emptyDir, filepath.Join(tmp, "nonexistent"), nil, false) + sl := NewSkillsLoader(ws, ws, emptyDir, filepath.Join(tmp, "nonexistent"), nil, false) skills := sl.ListSkills() assert.Empty(t, skills) @@ -259,7 +259,7 @@ func TestListSkillsDirWithoutSkillMD(t *testing.T) { // Valid skill alongside createSkillDir(t, global, "real-skill", "real-skill", "desc") - sl := NewSkillsLoader(ws, global, "", nil, false) + sl := NewSkillsLoader(ws, ws, global, "", nil, false) skills := sl.ListSkills() assert.Len(t, skills, 1) @@ -333,7 +333,7 @@ func TestSkillRootsTrimsWhitespaceAndDedups(t *testing.T) { global := filepath.Join(tmp, "global") builtin := filepath.Join(tmp, "builtin") - sl := NewSkillsLoader(workspace, " "+global+" ", "\t"+builtin+"\n", nil, false) + sl := NewSkillsLoader(workspace, workspace, " "+global+" ", "\t"+builtin+"\n", nil, false) roots := sl.SkillRoots() assert.Equal(t, []string{ @@ -429,14 +429,14 @@ func TestListSkillsWithWhitelist(t *testing.T) { createSkillDir(t, builtin, "skill-c", "skill-c", "desc c") t.Run("allow-one", func(t *testing.T) { - sl := NewSkillsLoader(ws, global, builtin, []string{"skill-a"}, true) + sl := NewSkillsLoader(ws, ws, global, builtin, []string{"skill-a"}, true) skills := sl.ListSkills() assert.Len(t, skills, 1) assert.Equal(t, "skill-a", skills[0].Name) }) t.Run("allow-two", func(t *testing.T) { - sl := NewSkillsLoader(ws, global, builtin, []string{"skill-a", "skill-c"}, true) + sl := NewSkillsLoader(ws, ws, global, builtin, []string{"skill-a", "skill-c"}, true) skills := sl.ListSkills() assert.Len(t, skills, 2) names := []string{skills[0].Name, skills[1].Name} @@ -445,19 +445,19 @@ func TestListSkillsWithWhitelist(t *testing.T) { }) t.Run("allow-none", func(t *testing.T) { - sl := NewSkillsLoader(ws, global, builtin, []string{"non-existent"}, true) + sl := NewSkillsLoader(ws, ws, global, builtin, []string{"non-existent"}, true) skills := sl.ListSkills() assert.Empty(t, skills) }) t.Run("empty-whitelist-allows-all", func(t *testing.T) { - sl := NewSkillsLoader(ws, global, builtin, []string{}, false) + sl := NewSkillsLoader(ws, ws, global, builtin, []string{}, false) skills := sl.ListSkills() assert.Len(t, skills, 3) }) t.Run("nil-whitelist-allows-all", func(t *testing.T) { - sl := NewSkillsLoader(ws, global, builtin, nil, false) + sl := NewSkillsLoader(ws, ws, global, builtin, nil, false) skills := sl.ListSkills() assert.Len(t, skills, 3) }) diff --git a/web/backend/api/skills.go b/web/backend/api/skills.go index a1d7f13b8..05caa1d91 100644 --- a/web/backend/api/skills.go +++ b/web/backend/api/skills.go @@ -191,6 +191,7 @@ func (h *Handler) handleDeleteSkill(w http.ResponseWriter, r *http.Request) { func newSkillsLoader(workspace string) *skills.SkillsLoader { return skills.NewSkillsLoader( + workspace, workspace, filepath.Join(globalConfigDir(), "skills"), builtinSkillsDir(),