From 5913149664989b411a924c328783eb4ed1bce37f 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 | 91 +++++++++--------- 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, 507 insertions(+), 155 deletions(-) create mode 100644 pkg/agent/isolation_tools_test.go diff --git a/Makefile b/Makefile index 42e6c299b..5f8c26e1a 100644 --- a/Makefile +++ b/Makefile @@ -268,7 +268,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 7a5902f58..e94374160 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -6,8 +6,6 @@ Config file: `~/.picoclaw/config.json` -> **Security Configuration:** For storing API keys, tokens, and other sensitive data, see the [Security Configuration Guide](security_configuration.md). - ### Environment Variables You can override default paths using environment variables. This is useful for portable installations, containerized deployments, or running picoclaw as a system service. These variables are independent and control different paths. @@ -40,12 +38,12 @@ PICOCLAW_HOME=/srv/picoclaw PICOCLAW_CONFIG=/srv/picoclaw/main.json picoclaw gat ```json { "gateway": { - "log_level": "warn" + "log_level": "fatal" } } ``` -When omitted, the default is `warn`. Supported values: `debug`, `info`, `warn`, `error`, `fatal`. +When omitted, the default is `fatal`. Supported values: `debug`, `info`, `warn`, `error`, `fatal`. You can also override this with the environment variable `PICOCLAW_LOG_LEVEL`. @@ -69,18 +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. -### Web launcher dashboard - -**picoclaw-launcher** serves a browser UI that requires sign-in first. By default, the **dashboard token** and **session signing key** are **generated in memory on each start** (a new random token after every restart). Set **`PICOCLAW_LAUNCHER_TOKEN`** to pin a fixed token for that process (startup logs do not print the secret when this env var is used). - -**Where to read the token**: In **console mode** (`-console`), it is printed at startup. In **tray / GUI mode**, use the tray action **Copy dashboard token**, and check **`$PICOCLAW_HOME/logs/launcher.log`** (typically `~/.picoclaw/logs/launcher.log` if `PICOCLAW_HOME` is unset) for the random token logged on startup. The login page shows hints that match how the launcher is running (including the absolute log path); **responses do not include the token itself**. - -- **Config file**: Same directory as `config.json` (or the file pointed to by `PICOCLAW_CONFIG`). The launcher-specific file is `launcher-config.json`. -- **Sign-in and links**: Enter the token on the login page, or open with `?token=` when the browser is launched automatically. All responses include **`Referrer-Policy: no-referrer`** to reduce leakage of `token` via the `Referer` header. -- **Sign-out**: Use **`POST /api/auth/logout`** with **`Content-Type: application/json`** (body may be `{}`). Do not rely on a GET URL for logout (CSRF-safe pattern). -- **Brute-force**: **`POST /api/auth/login`** is **rate-limited per client IP per minute** (HTTP 429 when exceeded). -- **Session lifetime**: The HttpOnly session cookie lasts about **7 days** by default; sign in again with the token after it expires. - +### 🔒 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: @@ -541,9 +559,8 @@ This design also enables **multi-agent support** with flexible provider selectio - **Different agents, different providers**: Each agent can use its own LLM provider - **Model fallbacks**: Configure primary and fallback models for resilience -- **Load balancing**: Distribute requests across multiple endpoints or keys +- **Load balancing**: Distribute requests across multiple endpoints - **Centralized configuration**: Manage all providers in one place -- **Model enable/disable**: Use the `enabled` field to temporarily disable a model without removing its configuration #### 🔒 Security Configuration (Recommended) @@ -623,7 +640,6 @@ For complete documentation, see [`security_configuration.md`](security_configura | **通义千问 (Qwen)** | `qwen/` | `https://dashscope.aliyuncs.com/compatible-mode/v1` | OpenAI | [Get Key](https://dashscope.console.aliyun.com) | | **NVIDIA** | `nvidia/` | `https://integrate.api.nvidia.com/v1` | OpenAI | [Get Key](https://build.nvidia.com) | | **Ollama** | `ollama/` | `http://localhost:11434/v1` | OpenAI | Local (no key needed) | -| **LM Studio** | `lmstudio/` | `http://localhost:1234/v1` | OpenAI | Optional (local default: no key) | | **OpenRouter** | `openrouter/` | `https://openrouter.ai/api/v1` | OpenAI | [Get Key](https://openrouter.ai/keys) | | **LiteLLM Proxy** | `litellm/` | `http://localhost:4000/v1` | OpenAI | Your LiteLLM proxy key | | **VLLM** | `vllm/` | `http://localhost:8000/v1` | OpenAI | Local | @@ -645,22 +661,22 @@ For complete documentation, see [`security_configuration.md`](security_configura { "model_name": "ark-code-latest", "model": "volcengine/ark-code-latest", - "api_keys": ["sk-your-api-key"] + "api_key": "sk-your-api-key" }, { "model_name": "gpt-5.4", "model": "openai/gpt-5.4", - "api_keys": ["sk-your-openai-key"] + "api_key": "sk-your-openai-key" }, { "model_name": "claude-sonnet-4.6", "model": "anthropic/claude-sonnet-4.6", - "api_keys": ["sk-ant-your-key"] + "api_key": "sk-ant-your-key" }, { "model_name": "glm-4.7", "model": "zhipu/glm-4.7", - "api_keys": ["your-zhipu-key"] + "api_key": "your-zhipu-key" } ], "agents": { @@ -671,9 +687,7 @@ For complete documentation, see [`security_configuration.md`](security_configura } ``` -> **Security Note**: You can remove `api_keys` fields from your config and store them in `.security.yml` instead. See [Security Configuration](#-security-configuration-recommended) above for details. -> -> **Note**: The `enabled` field can be set to `false` to disable a model entry without removing it. When omitted, it defaults to `true` during migration for models that have API keys. +> **Security Note**: You can remove `api_key` fields from your config and store them in `.security.yml` instead. See [Security Configuration](#-security-configuration-recommended) above for details. #### Vendor-Specific Examples @@ -750,7 +764,7 @@ For direct Anthropic API access or custom endpoints that only support Anthropic' { "model_name": "claude-opus-4-6", "model": "anthropic-messages/claude-opus-4-6", - "api_keys": ["sk-ant-your-key"], + "api_key": "sk-ant-your-key", "api_base": "https://api.anthropic.com" } ``` @@ -771,21 +785,6 @@ For direct Anthropic API access or custom endpoints that only support Anthropic' -
-LM Studio (local) - -```json -{ - "model_name": "lmstudio-local", - "model": "lmstudio/openai/gpt-oss-20b" -} -``` - -`api_base` defaults to `http://localhost:1234/v1`. API key is optional unless your LM Studio server enables authentication.
-PicoClaw sends OpenAI-compatible requests to LM Studio, and strips the `lmstudio/` prefix before sending requests, so `lmstudio/openai/gpt-oss-20b` sends `openai/gpt-oss-20b` to the LM Studio server. - -
-
Custom Proxy / LiteLLM @@ -840,13 +839,13 @@ model_list: "model_name": "gpt-5.4", "model": "openai/gpt-5.4", "api_base": "https://api1.example.com/v1", - "api_keys": ["sk-key1"] + "api_key": "sk-key1" }, { "model_name": "gpt-5.4", "model": "openai/gpt-5.4", "api_base": "https://api2.example.com/v1", - "api_keys": ["sk-key2"] + "api_key": "sk-key2" } ] } @@ -854,7 +853,7 @@ model_list: #### Migration from Legacy `providers` Config -The old `providers` configuration is **deprecated** and has been removed in V2. Existing V0/V1 configs are auto-migrated. See [docs/migration/model-list-migration.md](../migration/model-list-migration.md) for the full guide. +The old `providers` configuration is **deprecated** but still supported for backward compatibility. See [docs/migration/model-list-migration.md](../migration/model-list-migration.md) for the full guide. ### Provider Architecture @@ -864,7 +863,7 @@ PicoClaw routes providers by protocol family: - **Anthropic**: Claude-native API behavior. - **Codex/OAuth**: OpenAI OAuth/token authentication route. -This keeps the runtime lightweight while making new OpenAI-compatible backends mostly a config operation (`api_base` + `api_keys`). +This keeps the runtime lightweight while making new OpenAI-compatible backends mostly a config operation (`api_base` + `api_key`).
Zhipu (legacy providers format) diff --git a/pkg/agent/context.go b/pkg/agent/context.go index 09b649761..3975a6da7 100644 --- a/pkg/agent/context.go +++ b/pkg/agent/context.go @@ -21,6 +21,7 @@ import ( type ContextBuilder struct { workspace string + baseWorkspace string skillsLoader *skills.SkillsLoader memory *MemoryStore toolDiscoveryBM25 bool @@ -61,7 +62,11 @@ func getGlobalConfigDir() string { return config.GetHome() } -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)) @@ -72,9 +77,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), } } @@ -462,7 +468,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 ef5e6c5de..a2cdf2b54 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() @@ -750,7 +750,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 2785d70a5..586bdc84a 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 bacfa49c5..73d90dac5 100644 --- a/pkg/agent/instance.go +++ b/pkg/agent/instance.go @@ -59,8 +59,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) @@ -107,11 +108,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, @@ -234,17 +239,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 ba907e88b..93649f8ec 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)) @@ -257,7 +257,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 { @@ -361,7 +361,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") } @@ -374,3 +374,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 6cfbe1a56..864e5ecc2 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -59,11 +59,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 @@ -103,7 +112,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" @@ -183,6 +192,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(), @@ -730,7 +746,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 @@ -985,6 +1001,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) { @@ -1387,11 +1418,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 { @@ -1400,7 +1483,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", @@ -1468,10 +1552,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 } @@ -1485,7 +1578,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 e35609340..39e3b4d60 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 9513d8aca..cc81f181c 100644 --- a/pkg/agent/loop_test.go +++ b/pkg/agent/loop_test.go @@ -670,7 +670,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") @@ -1399,11 +1399,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 { @@ -2087,7 +2084,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 bf1d90e70..631238cb6 100644 --- a/pkg/gateway/gateway.go +++ b/pkg/gateway/gateway.go @@ -208,11 +208,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 4eea0118f..2ff2dae6e 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. @@ -43,7 +44,7 @@ type Server struct { startTime time.Time reloadFunc func() error authToken string // optional bearer token for protected endpoints - 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 @@ -157,7 +158,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 @@ -335,6 +336,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()) } @@ -352,7 +403,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 56cb155b7..608672172 100644 --- a/web/backend/api/skills.go +++ b/web/backend/api/skills.go @@ -504,6 +504,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(),