From 3572283d729f61e1d849c480948d595187227fd3 Mon Sep 17 00:00:00 2001 From: admin-mf Date: Fri, 6 Mar 2026 16:33:50 -0600 Subject: [PATCH] Enhance workspace path security and metrics tracking - Introduced `workspace_root` configuration in global and MagicForm settings to enforce path security. - Updated CLI commands to require relative paths for `--workspace` and `--config-dir` when `workspace_root` is set. - Added detailed validation rules for workspace paths to prevent directory traversal and absolute paths. - Enhanced agent processing to track execution metrics, including token usage and tool calls, and included these metrics in outbound messages. - Updated callback payloads for MagicForm to support progress and escalation messages, including detailed metrics for final responses. - Refactored agent loop to accumulate metrics across LLM iterations and publish progress updates during tool execution. - Added tests to ensure proper handling of new metrics and workspace path validations. --- .claude/settings.local.json | 86 +++++++++++++ docs/cli.md | 47 ++++++- docs/magicform-integration.md | 182 ++++++++++++++++++++++++---- pkg/agent/loop.go | 105 +++++++++++++--- pkg/agent/loop_test.go | 2 +- pkg/bus/types.go | 56 +++++++++ pkg/channels/magicform/magicform.go | 82 +++++++++++-- 7 files changed, 503 insertions(+), 57 deletions(-) create mode 100644 .claude/settings.local.json diff --git a/.claude/settings.local.json b/.claude/settings.local.json new file mode 100644 index 000000000..246091acf --- /dev/null +++ b/.claude/settings.local.json @@ -0,0 +1,86 @@ +{ + "permissions": { + "allow": [ + "Bash(git add pkg/tools/web.go)", + "Bash(git add pkg/tools/filesystem.go)", + "Bash(go build ./pkg/skills/...)", + "Bash(git add pkg/skills/installer.go)", + "Bash(go build ./cmd/picoclaw-launcher/...)", + "Bash(git add cmd/picoclaw-launcher/internal/server/auth_handlers.go)", + "Bash(git add cmd/picoclaw-launcher/internal/server/server.go)", + "Bash(git add cmd/picoclaw-launcher/internal/server/server.go cmd/picoclaw-launcher/main.go)", + "Bash(go test ./pkg/skills/...)", + "Bash(git add pkg/skills/clawhub_registry.go)", + "Bash(git add pkg/skills/loader.go pkg/skills/loader_test.go)", + "Bash(go build ./pkg/skills/... ./pkg/tools/...)", + "Bash(git add pkg/skills/registry.go pkg/skills/clawhub_registry.go pkg/tools/skills_install.go)", + "Bash(go build ./...)", + "Bash(go test ./pkg/tools/ -run Shell -v -count=1)", + "Bash(go test ./pkg/tools/ -run \"DevNullAllowed|SafePathsInWorkspace\" -v -count=1)", + "Bash(git add pkg/config/config.go pkg/tools/shell.go)", + "Bash(grep -n \"func CreateProvider\" /c/src/picoclaw/pkg/providers/*.go)", + "Bash(grep -n \"func.*GetModelConfig\\\\|func.*WorkspacePath\" /c/src/picoclaw/pkg/config/*.go)", + "Bash(grep -n \"ConvertProvidersToModelList\" /c/src/picoclaw/pkg/config/*.go)", + "Bash(grep -n \"CreateProvider\" /c/src/picoclaw/cmd/*.go)", + "Bash(go build ./pkg/config/...)", + "Bash(go build ./cmd/picoclaw/internal/agent/...)", + "Bash(go build ./pkg/channels/magicform/...)", + "Bash(go run tmp_test_overlay.go)", + "Bash(go run tmp_test_raw.go)", + "Bash(go test ./pkg/config/... ./pkg/agent/... ./pkg/channels/magicform/...)", + "Bash(git stash)", + "Bash(go test ./pkg/config/... ./pkg/agent/...)", + "Bash(git stash pop)", + "Bash(git add cmd/picoclaw/internal/agent/command.go cmd/picoclaw/internal/agent/helpers.go pkg/agent/loop.go pkg/agent/bootstrap.go pkg/channels/magicform/magicform.go pkg/config/config.go pkg/tools/cron.go config/workspace.config.example.json)", + "Bash(git commit:*)", + "Bash(git rebase main)", + "Bash(python3 -c:*)", + "Bash(git add pkg/auth/oauth.go)", + "Bash(git rebase --continue)", + "Bash(git add pkg/agent/loop.go)", + "Bash(go build ./pkg/agent/)", + "Bash(git push -u origin cmdline)", + "Bash(gh pr:*)", + "Bash(go run github.com/daixiang0/gci@latest diff --section standard --section default --section \"prefix\\(github.com/sipeed/picoclaw\\)\" pkg/channels/magicform/magicform.go)", + "Bash(go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@latest)", + "Bash(golangci-lint fmt ./cmd/picoclaw/internal/gateway/ ./pkg/channels/magicform/ ./pkg/tools/shell.go ./pkg/tools/web.go ./cmd/picoclaw/internal/agent/ ./cmd/picoclaw-launcher/internal/server/ ./pkg/agent/loop.go ./pkg/auth/oauth.go)", + "Bash(golangci-lint fmt ./pkg/channels/magicform/ ./pkg/tools/ ./cmd/picoclaw/internal/agent/ ./cmd/picoclaw-launcher/internal/server/ ./pkg/agent/ ./pkg/auth/)", + "Bash(golangci-lint version)", + "Bash(golangci-lint fmt pkg/channels/magicform/magicform.go)", + "Bash(golangci-lint fmt \"c:\\\\src\\\\picoclaw\\\\pkg\\\\channels\\\\magicform\")", + "Bash(go install github.com/segmentio/golines@latest)", + "Bash(go install mvdan.cc/gofumpt@latest)", + "Bash(gofumpt -d cmd/picoclaw/internal/agent/helpers.go)", + "Bash(gofumpt -w cmd/picoclaw/internal/agent/helpers.go)", + "Bash(golines -m 120 -w cmd/picoclaw-launcher/internal/server/auth_handlers.go cmd/picoclaw-launcher/internal/server/server.go cmd/picoclaw/internal/agent/command.go pkg/agent/loop.go pkg/auth/oauth.go)", + "Bash(gofumpt -w pkg/channels/magicform/magicform.go pkg/channels/magicform/init.go pkg/tools/shell.go pkg/tools/web.go cmd/picoclaw/internal/gateway/helpers.go pkg/auth/oauth.go)", + "Bash(golines -m 120 -w pkg/channels/magicform/magicform.go pkg/tools/shell.go pkg/tools/web.go cmd/picoclaw/internal/gateway/helpers.go pkg/auth/oauth.go)", + "Bash(git add cmd/picoclaw-launcher/internal/server/auth_handlers.go cmd/picoclaw-launcher/internal/server/server.go cmd/picoclaw/internal/agent/command.go cmd/picoclaw/internal/agent/helpers.go cmd/picoclaw/internal/gateway/helpers.go pkg/agent/loop.go pkg/auth/oauth.go pkg/channels/magicform/init.go pkg/channels/magicform/magicform.go pkg/tools/shell.go pkg/tools/web.go)", + "Bash(git push)", + "Bash(git config --local -l)", + "Bash(xargs grep -l \"nuestra\\\\|fork\\\\|github.com/nuestra\\\\|github.com/sipeed\")", + "Bash(git check-ignore -v docs/magicform-integration.md)", + "Bash(xargs grep -l \"interface Channel\")", + "Bash(xargs grep -l \"filepath\\\\|validation\\\\|security\")", + "Bash(go test ./pkg/pathutil/...)", + "Bash(go test ./...)", + "Bash(go test ./pkg/pathutil/... ./pkg/config/... ./pkg/agent/... ./cmd/picoclaw/internal/agent/...)", + "Bash(go test ./pkg/config/...)", + "Bash(go test ./pkg/pathutil/... -v)", + "Bash(go test ./pkg/pathutil/... ./cmd/picoclaw/internal/agent/... ./pkg/config/... -v)", + "Bash(go test ./cmd/picoclaw/internal/agent/... -v -run TestValidateWorkspacePaths)", + "Bash(go test ./pkg/pathutil/... ./cmd/picoclaw/internal/agent/...)", + "Bash(go test ./pkg/pathutil/... ./cmd/picoclaw/internal/agent/... ./pkg/config/... ./pkg/agent/...)", + "Bash(git add cmd/picoclaw/internal/agent/helpers.go cmd/picoclaw/internal/agent/helpers_test.go config/config.example.json pkg/agent/loop.go pkg/channels/magicform/init.go pkg/channels/magicform/magicform.go pkg/config/config.go pkg/pathutil/resolve.go pkg/pathutil/resolve_test.go)", + "Bash(grep -rn \"^type Channel\\\\|^type.*Channel\" /c/src/picoclaw/pkg/channels/*.go)", + "Bash(go build ./pkg/bus/... ./pkg/agent/...)", + "Bash(go build ./pkg/bus/...)", + "Bash(go test ./pkg/bus/... ./pkg/agent/... ./pkg/channels/magicform/... ./cmd/picoclaw/internal/agent/... ./pkg/pathutil/...)", + "Bash(go test ./pkg/agent/...)", + "Bash(go test ./pkg/agent/ -run TestGlobalSkillFileContentChange -v -timeout 30s)", + "Bash(go test ./pkg/agent/)", + "Bash(go test ./pkg/bus/... ./pkg/channels/...)", + "Bash(go test ./pkg/agent/ -run 'Test[^G]' -v)" + ] + } +} diff --git a/docs/cli.md b/docs/cli.md index 95948ca19..9dbe11e58 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -22,6 +22,7 @@ Create `~/.picoclaw/config.json` (or set `PICOCLAW_CONFIG` env var): ], "agents": { "defaults": { + "workspace_root": "/data/workspaces", "model_name": "main", "max_tokens": 4096, "max_tool_iterations": 20 @@ -32,6 +33,8 @@ Create `~/.picoclaw/config.json` (or set `PICOCLAW_CONFIG` env var): The global config is the base. Per-workspace configs overlay it (see [Workspace Config](#workspace-config)). +`workspace_root` defines the security boundary for all workspace and config-dir paths. When set, `--workspace` and `--config-dir` must be relative subdirectories within it. See [Path Security](#path-security) for details. + --- ## Subcommands @@ -61,8 +64,8 @@ Interact with the agent directly. Without `-m`, starts an interactive REPL. With | `--message` | `-m` | | Single message (non-interactive). Omit for interactive REPL. | | `--session` | `-s` | `""` (→ `agent:main:cli:default`) | Session key for conversation isolation (e.g. `stackId:conversationId`). | | `--model` | | | Override model name from config. | -| `--workspace` | | | Override agent workspace directory. | -| `--config-dir` | | | Directory containing `config.json` (model/agent/tool overrides) and bootstrap files (`AGENTS.md`, `IDENTITY.md`, `SOUL.md`, `USER.md`). | +| `--workspace` | | | Agent workspace directory, relative to `workspace_root`. Resolved to an absolute path before use. | +| `--config-dir` | | | Config directory (relative to `workspace_root`) containing `config.json` and bootstrap files (`AGENTS.md`, `IDENTITY.md`, `SOUL.md`, `USER.md`). | | `--tools` | | | Comma-separated tool allowlist (e.g. `read_file,web_fetch`). Only these tools are enabled. | | `--skills` | | | Comma-separated skill filter (e.g. `summarize,translate`). Only these skills are loaded. | | `--debug` | `-d` | `false` | Enable debug logging. | @@ -98,11 +101,11 @@ picoclaw agent # One-shot message picoclaw agent -m "Hello, world" -# With workspace isolation +# With workspace isolation (paths relative to workspace_root) picoclaw agent -m "Summarize the report" \ -s tenant1:conv42 \ - --workspace /data/workspaces/tenant1/conv42 \ - --config-dir /data/workspaces/tenant1/config + --workspace tenant1/conv42 \ + --config-dir tenant1/config # Restricted tools, custom model picoclaw agent -m "Search the web for recent news" \ @@ -113,6 +116,8 @@ picoclaw agent -m "Search the web for recent news" \ picoclaw agent -d -m "Hello" -s test ``` +> **Note**: When `workspace_root` is set in the global config, `--workspace` and `--config-dir` must be relative paths (e.g. `tenant1/conv42`, not `/data/workspaces/tenant1/conv42`). When `workspace_root` is not set, paths fall back to `filepath.Abs` resolution for backward compatibility. + --- ## picoclaw gateway @@ -275,6 +280,8 @@ A workspace-level `config.json` (placed in the config directory) overlays the gl **Not honored** (infrastructure-level): `gateway`, `heartbeat`, `devices`, `providers`, `channels`. +**Protected fields**: `workspace_root` cannot be overridden by a workspace config overlay — the boundary is always set by the base (global) config. If a workspace overlay sets a `workspace` value, it is validated against the base config's `workspace_root`; traversal attempts (e.g. `../../escape`) cause a hard error. + Example `config.json`: ```json @@ -352,8 +359,38 @@ For `--tools` (CLI) or `allowedTools` (webhook API): --- +## Path Security + +When `workspace_root` is set in `agents.defaults`, all workspace and config-dir paths are validated as relative subdirectories of that root. This applies uniformly across the CLI (`--workspace`, `--config-dir`), the gateway (webhook `workspace`/`configDir` fields), and workspace config overlays. + +**Validation rules** (when `workspace_root` is set): + +- **Absolute paths are rejected** — use `tenant1/conv42`, not `/data/workspaces/tenant1/conv42`. +- **Directory traversal is rejected** — `../escape`, `a/../../etc`, `foo/..` all fail. +- **Empty string and bare `.` are rejected** — the path must pick a subdirectory, not root itself. +- **Relative paths are joined to `workspace_root`** — `tenant1/conv42` becomes `/data/workspaces/tenant1/conv42`. +- **A post-join boundary check** confirms the resolved path stays within `workspace_root`. + +**Without `workspace_root`**: paths fall back to `filepath.Abs` resolution for backward compatibility, but traversal (`..`) is still rejected. + +**Enforcement points**: + +1. **CLI** — `--workspace` and `--config-dir` are validated before any workspace config overlay is loaded. +2. **Gateway** — webhook `workspace` and `configDir` fields are validated by each channel handler (e.g. MagicForm). +3. **Agent loop** — defense-in-depth re-validation of metadata-driven workspace overrides. +4. **Config overlay** — `workspace_root` cannot be overridden; `workspace` values from overlays are validated against the base config boundary. + +--- + ## Troubleshooting +**`--workspace` or `--config-dir` rejected** +- When `workspace_root` is set, paths must be relative subdirectories (e.g. `s1/c1`). Absolute paths, `..` traversal, and empty/`.` paths are rejected. +- If `workspace_root` is not set and you get a traversal error, the path contains `..` which is always blocked. + +**Workspace config overlay rejected** +- A workspace `config.json` that sets `agents.defaults.workspace` to a path escaping `workspace_root` will cause a hard error during config merge. Fix the overlay's `workspace` value. + **Workspace config ignored** - Verify `config.json` exists in the `--config-dir` path. - Check that the JSON is valid (`picoclaw agent -d` shows parse errors). diff --git a/docs/magicform-integration.md b/docs/magicform-integration.md index a34b264c3..e65c61927 100644 --- a/docs/magicform-integration.md +++ b/docs/magicform-integration.md @@ -44,6 +44,11 @@ Add to `~/.picoclaw/config.json`: ```jsonc { + "agents": { + "defaults": { + "workspace_root": "/data/workspaces" + } + }, "gateway": { "host": "0.0.0.0", "port": 18790 @@ -54,7 +59,6 @@ Add to `~/.picoclaw/config.json`: "token": "your-shared-secret", "backend_url": "https://api.magicform.example.com", "webhook_path": "/hooks/magicform", - "workspace_root": "/data/workspaces", "allow_from": [] } } @@ -66,9 +70,16 @@ Add to `~/.picoclaw/config.json`: | `token` | Bearer token for webhook auth. Empty = allow all (dev only). | `""` | | `backend_url` | Fallback callback URL base (used when payload omits `callbackUrl`). | `""` | | `webhook_path` | HTTP path for the webhook endpoint. | `/hooks/magicform` | -| `workspace_root` | Root directory for workspace/configDir path validation. Required for path security. | `""` | +| `workspace_root` | Channel-level override for workspace path validation root. If not set, falls back to `agents.defaults.workspace_root`. At least one must be configured. | `""` | | `allow_from` | Sender ID allowlist. Empty = allow all. Accepts strings and numbers (e.g. `["user1", 12345]`). | `[]` | +**`workspace_root` resolution**: The MagicForm channel determines its effective workspace root using: +1. `channels.magicform.workspace_root` (channel-level override), if set. +2. `agents.defaults.workspace_root` (global), if set. +3. If neither is configured, the gateway **fails to start** with an error. A workspace root is required for MagicForm because all webhook paths must be validated against a boundary. + +The recommended approach is to set `workspace_root` once in `agents.defaults` so that it applies to both the CLI and all gateway channels. Use the channel-level `workspace_root` only if MagicForm needs a different root than other entry points. + All fields can be set via environment variables: ```bash @@ -78,6 +89,7 @@ PICOCLAW_CHANNELS_MAGICFORM_BACKEND_URL=https://api.magicform.example.com PICOCLAW_CHANNELS_MAGICFORM_WEBHOOK_PATH=/hooks/magicform PICOCLAW_CHANNELS_MAGICFORM_WORKSPACE_ROOT=/data/workspaces PICOCLAW_CHANNELS_MAGICFORM_ALLOW_FROM=sender1,sender2 +PICOCLAW_AGENTS_DEFAULTS_WORKSPACE_ROOT=/data/workspaces ``` ### Start the gateway @@ -138,7 +150,12 @@ Content-Type: application/json | `allowedTools` | string[] | No | Tool allowlist. Empty = all tools enabled. See [cli.md § Tool Names](cli.md#tool-names-reference). | | `allowedSkills` | string[] | No | Skill filter. Empty = all skills loaded. | -**Path security**: `workspace` and `configDir` must be relative paths that resolve under `workspace_root`. Traversal attempts (e.g. `../../etc`) are rejected with `400 Bad Request`. +**Path security**: `workspace` and `configDir` must be relative paths that resolve to subdirectories under `workspace_root`. The following are rejected with `400 Bad Request`: +- Absolute paths (e.g. `/data/workspaces/s1/c1`) +- Directory traversal (`../escape`, `a/../../etc`, `foo/..`) +- Empty string or bare `.` (would resolve to root itself) + +See [cli.md § Path Security](cli.md#path-security) for the full validation rules shared across CLI and gateway. **Request size limit**: Webhook payloads are limited to 1 MB. Larger requests receive `413 Request Entity Too Large`. @@ -150,7 +167,7 @@ Returns `200 OK` immediately. Processing happens asynchronously. ### Callback: receive the result -PicoClaw POSTs the result to the callback URL: +PicoClaw POSTs results back to the callback URL. There are three callback types: **final**, **progress**, and **escalation**. ``` POST {callbackUrl} @@ -158,21 +175,116 @@ Authorization: Bearer your-shared-secret Content-Type: application/json ``` -```json -{ - "stackId": "s1", - "conversationId": "c1", - "response": "Here is the summary of the latest sales report...", - "type": "final" -} -``` +#### Common fields + +Every callback includes these fields: | Field | Type | Description | |-------|------|-------------| | `stackId` | string | Echoed from request. | | `conversationId` | string | Echoed from request. | -| `response` | string | The agent's response text. | -| `type` | string | Always `"final"`. | +| `taskId` | string | Unique task ID: `claw_task_{conversationId}_{createdAtMs}`. | +| `type` | string | `"final"`, `"progress"`, or `"escalation"`. | +| `status` | string | `"success"` or `"error"`. | +| `response` | string | The agent's response text (may be empty for progress/escalation). | +| `runtime` | string | Always `"picoclaw"`. | + +#### Final callback + +Sent once when the agent finishes processing. Includes execution metrics. + +```json +{ + "stackId": "s1", + "conversationId": "c1", + "taskId": "claw_task_c1_1709712000000", + "type": "final", + "status": "success", + "response": "Here is the summary of the latest sales report...", + "runtime": "picoclaw", + "durationMs": 4523, + "tokenUsage": { + "promptTokens": 1200, + "completionTokens": 350, + "totalTokens": 1550, + "model": "anthropic/claude-sonnet-4.6" + }, + "toolCalls": 3, + "progress": null, + "escalation": null +} +``` + +| Field | Type | Description | +|-------|------|-------------| +| `durationMs` | number | Total processing time in milliseconds. | +| `tokenUsage` | object \| null | Token consumption breakdown. | +| `tokenUsage.promptTokens` | number | Prompt tokens used across all iterations. | +| `tokenUsage.completionTokens` | number | Completion tokens used across all iterations. | +| `tokenUsage.totalTokens` | number | Total tokens (prompt + completion). | +| `tokenUsage.model` | string | Model ID used for generation. | +| `toolCalls` | number | Total tool calls made during processing. | +| `error` | string | Error message (present only when `status` is `"error"`). | +| `progress` | null | Always null for final callbacks. | +| `escalation` | null | Always null for final callbacks. | + +#### Progress callback + +Sent during processing as the agent executes tools. Multiple progress callbacks may be sent before the final callback. + +```json +{ + "stackId": "s1", + "conversationId": "c1", + "taskId": "claw_task_c1_1709712000000", + "type": "progress", + "status": "success", + "response": "Running tools: web_fetch", + "runtime": "picoclaw", + "progress": { + "status": "thinking", + "toolName": "web_fetch", + "stepNumber": 2, + "message": "Running tools: web_fetch" + }, + "escalation": null +} +``` + +| Field | Type | Description | +|-------|------|-------------| +| `progress` | object | Progress details. | +| `progress.status` | string | Current status (e.g. `"thinking"`). | +| `progress.toolName` | string | Name of the tool being executed. | +| `progress.stepNumber` | number | Current iteration number. | +| `progress.message` | string | Human-readable progress description. | + +#### Escalation callback + +Sent when the agent determines it needs human intervention. + +```json +{ + "stackId": "s1", + "conversationId": "c1", + "taskId": "claw_task_c1_1709712000000", + "type": "escalation", + "status": "success", + "response": "", + "runtime": "picoclaw", + "progress": null, + "escalation": { + "reason": "User requested human support", + "notes": "Customer issue requires billing system access" + } +} +``` + +| Field | Type | Description | +|-------|------|-------------| +| `escalation` | object | Escalation details. | +| `escalation.reason` | string | Why escalation is needed. | +| `escalation.notes` | string | Additional context (optional). | ### Session isolation @@ -186,22 +298,32 @@ Sessions are stored at `{workspace}/sessions/`. Different conversations within t MagicForm PicoClaw Gateway | | |-- POST /hooks/magicform ----------->| + | validate token + | parse payload + | ResolveWorkspacePath(root, workspace) ← 400 on failure + | ResolveWorkspacePath(root, configDir) ← 400 on failure |<------------ 200 OK ---------------| | | - | resolveWorkspace(workspace) - | resolveWorkspace(configDir) | publish InboundMessage to bus | | | Agent loop: + | re-validate workspace/configDir (defense-in-depth) | create temp sessions + context | copyBootstrapFiles(configDir -> workspace) | loadWorkspaceConfig(configDir) | mergeWorkspaceConfig into cloned global cfg | createProvider (per-request) | apply tool/skill filters - | run LLM iterations + | run LLM iterations: + | for each iteration: + | LLM call → accumulate token usage + | tool calls → accumulate tool count | | - |<-- POST callbackUrl (result) -------| + |<-- POST callbackUrl (progress) ----| ← during tool execution + | | + | ... more iterations ... + | | + |<-- POST callbackUrl (final) -------| ← with metrics (duration, tokens, tool calls) ``` --- @@ -211,11 +333,11 @@ MagicForm PicoClaw Gateway You can test the same workspace/config setup without the gateway using `picoclaw agent`. See [cli.md § picoclaw agent](cli.md#picoclaw-agent) for full flag reference. ```bash -# One-shot with tenant isolation (same paths MagicForm would use) +# One-shot with tenant isolation (same relative paths MagicForm would use) picoclaw agent -m "Summarize the report" \ -s s1:c1 \ - --workspace /data/workspaces/s1/c1 \ - --config-dir /data/workspaces/s1/config + --workspace s1/c1 \ + --config-dir s1/config # Restricted tools, matching a webhook allowedTools filter picoclaw agent -m "Search the web for recent news" \ @@ -225,6 +347,8 @@ picoclaw agent -m "Search the web for recent news" \ picoclaw agent -d -m "Hello" -s test ``` +> **Note**: The CLI resolves `--workspace` and `--config-dir` against `agents.defaults.workspace_root` using the same validation as the MagicForm webhook. Both entry points use the shared `pathutil.ResolveWorkspacePath` function. + --- ## Troubleshooting @@ -235,19 +359,27 @@ picoclaw agent -d -m "Hello" -s test **Webhook returns 401 Unauthorized** - Check that the `Authorization: Bearer {token}` header matches the `token` in the MagicForm channel config. Token comparison uses constant-time comparison. -**Webhook returns 400 "workspace path escapes workspace_root"** -- The `workspace` or `configDir` path in the payload resolves outside `workspace_root`. Ensure paths are relative (e.g. `s1/c1`, not `/data/workspaces/s1/c1`). +**Webhook returns 400 "Invalid workspace" or "Invalid configDir"** +- The `workspace` or `configDir` path failed validation. Common causes: + - **Absolute path** — use `s1/c1`, not `/data/workspaces/s1/c1`. + - **Traversal** — paths like `../escape` or `a/../../etc` are rejected. + - **Empty or `.`** — the path must be a subdirectory, not root itself. + - **Escapes root** — the resolved path lands outside `workspace_root`. **Webhook returns 413 Request Entity Too Large** - The request payload exceeds the 1 MB limit. Reduce the message size. -**Webhook returns 400 "workspace_root not configured"** -- Set `workspace_root` in the MagicForm channel config. +**Gateway fails to start: "magicform channel requires workspace_root"** +- Neither `channels.magicform.workspace_root` nor `agents.defaults.workspace_root` is configured. Set at least one. The recommended location is `agents.defaults.workspace_root`. **Callback not received** - Check that `callbackUrl` in the payload or `backend_url` in config is reachable from PicoClaw. - Check PicoClaw logs for callback errors. - Request contexts expire after 10 minutes. +- Progress and escalation callbacks keep the request context alive. The final callback deletes it. + +**Unexpected `null` in progress/escalation fields** +- `progress` and `escalation` are always present in every callback but set to `null` when not applicable (e.g. `progress` is `null` in a final callback). Check the `type` field to determine which sub-object to read. **Session not persisting across requests** - Ensure the same `workspace` path is sent for the same conversation. diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index 88783c69e..8a57c6169 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -349,9 +349,10 @@ func (al *AgentLoop) Run(ctx context.Context) error { // } // }() - response, err := al.processMessage(ctx, msg) + response, metrics, err := al.processMessage(ctx, msg) if err != nil { response = fmt.Sprintf("Error processing message: %v", err) + metrics = nil // don't attach metrics to error responses } if response != "" { @@ -373,6 +374,7 @@ func (al *AgentLoop) Run(ctx context.Context) error { Channel: msg.Channel, ChatID: msg.ChatID, Content: response, + Metrics: metrics, }) logger.InfoCF("agent", "Published outbound response", map[string]any{ @@ -548,7 +550,8 @@ func (al *AgentLoop) ProcessDirectWithChannel( SessionKey: sessionKey, } - return al.processMessage(ctx, msg) + result, _, err := al.processMessage(ctx, msg) + return result, err } // ProcessHeartbeat processes a heartbeat request without session history. @@ -561,7 +564,7 @@ func (al *AgentLoop) ProcessHeartbeat( if agent == nil { return "", fmt.Errorf("no default agent for heartbeat") } - return al.runAgentLoop(ctx, agent, processOptions{ + result, _, err := al.runAgentLoop(ctx, agent, processOptions{ SessionKey: "heartbeat", Channel: channel, ChatID: chatID, @@ -571,9 +574,10 @@ func (al *AgentLoop) ProcessHeartbeat( SendResponse: false, NoHistory: true, // Don't load session history for heartbeat }) + return result, err } -func (al *AgentLoop) processMessage(ctx context.Context, msg bus.InboundMessage) (string, error) { +func (al *AgentLoop) processMessage(ctx context.Context, msg bus.InboundMessage) (string, *bus.ResponseMetrics, error) { // Add message preview to log (show full content for error messages) var logContent string if strings.Contains(msg.Content, "Error:") || strings.Contains(msg.Content, "error") { @@ -596,7 +600,8 @@ func (al *AgentLoop) processMessage(ctx context.Context, msg bus.InboundMessage) // Route system messages to processSystemMessage if msg.Channel == "system" { - return al.processSystemMessage(ctx, msg) + result, err := al.processSystemMessage(ctx, msg) + return result, nil, err } route, agent, routeErr := al.resolveMessageRoute(msg) @@ -606,11 +611,11 @@ func (al *AgentLoop) processMessage(ctx context.Context, msg bus.InboundMessage) // context-dependent commands check their own Runtime fields and report // "unavailable" when the required capability is nil. if response, handled := al.handleCommand(ctx, msg, agent); handled { - return response, nil + return response, nil, nil } if routeErr != nil { - return "", routeErr + return "", nil, routeErr } // Reset message-tool state for this round so we don't skip publishing due to a previous round. @@ -646,7 +651,7 @@ func (al *AgentLoop) processMessage(ctx context.Context, msg bus.InboundMessage) if err != nil { logger.WarnCF("agent", "Rejecting workspace_override from metadata", map[string]any{"workspace_override": workspaceOverride, "error": err.Error()}) - return "", fmt.Errorf("invalid workspace_override in metadata: %w", err) + return "", nil, fmt.Errorf("invalid workspace_override in metadata: %w", err) } workspaceOverride = resolved } @@ -655,7 +660,7 @@ func (al *AgentLoop) processMessage(ctx context.Context, msg bus.InboundMessage) if err != nil { logger.WarnCF("agent", "Rejecting config_dir from metadata", map[string]any{"config_dir": configDir, "error": err.Error()}) - return "", fmt.Errorf("invalid config_dir in metadata: %w", err) + return "", nil, fmt.Errorf("invalid config_dir in metadata: %w", err) } configDir = resolved } @@ -774,7 +779,7 @@ func (al *AgentLoop) processSystemMessage( // Use the origin session for context sessionKey := routing.BuildAgentMainSessionKey(agent.ID) - return al.runAgentLoop(ctx, agent, processOptions{ + result, _, err := al.runAgentLoop(ctx, agent, processOptions{ SessionKey: sessionKey, Channel: originChannel, ChatID: originChatID, @@ -783,6 +788,7 @@ func (al *AgentLoop) processSystemMessage( EnableSummary: false, SendResponse: true, }) + return result, err } // runAgentLoop is the core message processing logic. @@ -790,7 +796,7 @@ func (al *AgentLoop) runAgentLoop( ctx context.Context, agent *AgentInstance, opts processOptions, -) (string, error) { +) (string, *bus.ResponseMetrics, error) { // 0. Record last channel for heartbeat notifications (skip internal channels) if opts.Channel != "" && opts.ChatID != "" { // Don't record internal channels (cli, system, subagent) @@ -836,7 +842,7 @@ func (al *AgentLoop) runAgentLoop( } else if wc != nil { tmpCfg := al.cfg.Clone() if err := tmpCfg.MergeWorkspaceConfig(wc); err != nil { - return "", fmt.Errorf("workspace config overlay rejected: %w", err) + return "", nil, fmt.Errorf("workspace config overlay rejected: %w", err) } if tmpCfg.Agents.Defaults.GetModelName() == "" { tmpCfg.Agents.Defaults.ModelName = agent.Model @@ -887,9 +893,30 @@ func (al *AgentLoop) runAgentLoop( opts.effContextBuilder = effContextBuilder // 3. Run LLM iteration loop - finalContent, iteration, err := al.runLLMIteration(ctx, agent, messages, opts) + startTime := time.Now() + finalContent, iteration, iterMetrics, err := al.runLLMIteration(ctx, agent, messages, opts) + durationMs := time.Since(startTime).Milliseconds() if err != nil { - return "", err + return "", nil, err + } + + // Build response metrics from accumulated iteration metrics. + var respMetrics *bus.ResponseMetrics + if iterMetrics != nil { + respMetrics = &bus.ResponseMetrics{ + DurationMs: durationMs, + ToolCalls: iterMetrics.ToolCalls, + Iterations: iteration, + Model: iterMetrics.Model, + } + if iterMetrics.PromptTokens > 0 || iterMetrics.CompletionTokens > 0 { + respMetrics.TokenUsage = &bus.TokenUsage{ + PromptTokens: iterMetrics.PromptTokens, + CompletionTokens: iterMetrics.CompletionTokens, + TotalTokens: iterMetrics.PromptTokens + iterMetrics.CompletionTokens, + Model: iterMetrics.Model, + } + } } // If last tool had ForUser content and we already sent it, we might not need to send final response @@ -917,12 +944,13 @@ func (al *AgentLoop) runAgentLoop( ) } - // 7. Optional: send response via bus + // 7. Optional: send response via bus (with metrics on the final message) if opts.SendResponse { al.bus.PublishOutbound(ctx, bus.OutboundMessage{ Channel: opts.Channel, ChatID: opts.ChatID, Content: finalContent, + Metrics: respMetrics, }) } @@ -934,9 +962,10 @@ func (al *AgentLoop) runAgentLoop( "session_key": opts.SessionKey, "iterations": iteration, "final_length": len(finalContent), + "duration_ms": durationMs, }) - return finalContent, nil + return finalContent, respMetrics, nil } func (al *AgentLoop) targetReasoningChannelID(channelName string) (chatID string) { @@ -995,15 +1024,26 @@ func (al *AgentLoop) handleReasoning( } } +// turnMetrics accumulates cumulative totals across all LLM iterations within +// a single agent processing turn. Values are summed after each iteration in +// runLLMIteration, not per-iteration snapshots. +type turnMetrics struct { + PromptTokens int + CompletionTokens int + ToolCalls int + Model string // last model used +} + // runLLMIteration executes the LLM call loop with tool handling. func (al *AgentLoop) runLLMIteration( ctx context.Context, agent *AgentInstance, messages []providers.Message, opts processOptions, -) (string, int, error) { +) (string, int, *turnMetrics, error) { iteration := 0 var finalContent string + metrics := &turnMetrics{} // Resolve effective provider/model — workspace config overrides win effProvider := agent.Provider @@ -1200,7 +1240,7 @@ func (al *AgentLoop) runLLMIteration( "iteration": iteration, "error": err.Error(), }) - return "", iteration, fmt.Errorf("LLM call failed after retries: %w", err) + return "", iteration, metrics, fmt.Errorf("LLM call failed after retries: %w", err) } go al.handleReasoning( @@ -1220,6 +1260,15 @@ func (al *AgentLoop) runLLMIteration( "target_channel": al.targetReasoningChannelID(opts.Channel), "channel": opts.Channel, }) + + // Accumulate token usage and tool call counts across iterations. + if response.Usage != nil { + metrics.PromptTokens += response.Usage.PromptTokens + metrics.CompletionTokens += response.Usage.CompletionTokens + } + metrics.ToolCalls += len(response.ToolCalls) + metrics.Model = activeModel + // Check if no tool calls - we're done if len(response.ToolCalls) == 0 { finalContent = response.Content @@ -1283,6 +1332,24 @@ func (al *AgentLoop) runLLMIteration( // Save assistant message with tool calls to session opts.effSessions.AddFullMessage(opts.SessionKey, assistantMsg) + // Publish progress callback for channels that support it (e.g. MagicForm). + // Non-internal channels receive a progress update before tool execution. + if !constants.IsInternalChannel(opts.Channel) && opts.Channel != "" { + toolNamesList := strings.Join(toolNames, ", ") + al.bus.PublishOutbound(ctx, bus.OutboundMessage{ + Channel: opts.Channel, + ChatID: opts.ChatID, + Type: bus.MessageTypeProgress, + Content: fmt.Sprintf("Running tools: %s", toolNamesList), + Progress: &bus.OutboundProgress{ + Status: "thinking", + ToolName: toolNames[0], + StepNumber: iteration, + Message: fmt.Sprintf("Running tools: %s", toolNamesList), + }, + }) + } + // Execute tool calls in parallel type indexedAgentResult struct { result *tools.ToolResult @@ -1405,7 +1472,7 @@ func (al *AgentLoop) runLLMIteration( } } - return finalContent, iteration, nil + return finalContent, iteration, metrics, nil } // selectCandidates returns the model candidates and resolved model name to use diff --git a/pkg/agent/loop_test.go b/pkg/agent/loop_test.go index 2e456fa60..ac618ad34 100644 --- a/pkg/agent/loop_test.go +++ b/pkg/agent/loop_test.go @@ -374,7 +374,7 @@ func (h testHelper) executeAndGetResponse(tb testing.TB, ctx context.Context, ms timeoutCtx, cancel := context.WithTimeout(ctx, responseTimeout) defer cancel() - response, err := h.al.processMessage(timeoutCtx, msg) + response, _, err := h.al.processMessage(timeoutCtx, msg) if err != nil { tb.Fatalf("processMessage failed: %v", err) } diff --git a/pkg/bus/types.go b/pkg/bus/types.go index 7ad8f0417..b12d2f27e 100644 --- a/pkg/bus/types.go +++ b/pkg/bus/types.go @@ -29,10 +29,66 @@ type InboundMessage struct { Metadata map[string]string `json:"metadata,omitempty"` } +// Outbound message type constants identify the kind of outbound message. +// Channels that don't support progress/escalation treat all messages as content. +const ( + MessageTypeFinal = "" // default: final response + MessageTypeProgress = "progress" // intermediate progress update + MessageTypeEscalation = "escalation" // needs human intervention +) + type OutboundMessage struct { Channel string `json:"channel"` ChatID string `json:"chat_id"` Content string `json:"content"` + + // Type distinguishes final responses from progress/escalation updates. + // Empty string (default) means final response. Channels that don't support + // progress callbacks ignore this field and treat all messages as content. + Type string `json:"type,omitempty"` + + // Metrics is populated by the agent loop for the final response message. + // Channels that support rich callbacks (e.g. MagicForm) use this to include + // execution metrics in their response payloads. Nil for intermediate messages. + Metrics *ResponseMetrics `json:"metrics,omitempty"` + + // Progress is populated for Type="progress" messages. + Progress *OutboundProgress `json:"progress,omitempty"` + + // Escalation is populated for Type="escalation" messages. + Escalation *OutboundEscalation `json:"escalation,omitempty"` +} + +// OutboundProgress carries progress update details. +type OutboundProgress struct { + Status string `json:"status"` // e.g. "thinking" + ToolName string `json:"toolName,omitempty"` + StepNumber int `json:"stepNumber,omitempty"` + Message string `json:"message,omitempty"` +} + +// OutboundEscalation carries escalation details. +type OutboundEscalation struct { + Reason string `json:"reason"` + Notes string `json:"notes,omitempty"` +} + +// TokenUsage tracks LLM token consumption across all iterations in a turn. +type TokenUsage struct { + PromptTokens int `json:"promptTokens"` + CompletionTokens int `json:"completionTokens"` + TotalTokens int `json:"totalTokens"` + Model string `json:"model"` + Provider string `json:"provider,omitempty"` +} + +// ResponseMetrics captures execution metrics for a single agent processing turn. +type ResponseMetrics struct { + DurationMs int64 `json:"durationMs"` + TokenUsage *TokenUsage `json:"tokenUsage,omitempty"` + ToolCalls int `json:"toolCalls"` + Iterations int `json:"iterations"` + Model string `json:"model,omitempty"` } // MediaPart describes a single media attachment to send. diff --git a/pkg/channels/magicform/magicform.go b/pkg/channels/magicform/magicform.go index 1409620ca..e73026e72 100644 --- a/pkg/channels/magicform/magicform.go +++ b/pkg/channels/magicform/magicform.go @@ -38,8 +38,31 @@ type WebhookPayload struct { type CallbackPayload struct { StackID string `json:"stackId"` ConversationID string `json:"conversationId"` + TaskID string `json:"taskId,omitempty"` + Type string `json:"type"` // "final", "progress", "escalation" + Status string `json:"status"` // "success", "error" Response string `json:"response"` - Type string `json:"type"` // "final" + Error string `json:"error,omitempty"` + Runtime string `json:"runtime"` + DurationMs int64 `json:"durationMs,omitempty"` + TokenUsage *bus.TokenUsage `json:"tokenUsage,omitempty"` + ToolCalls int `json:"toolCalls,omitempty"` + Progress *ProgressPayload `json:"progress"` // null unless type=progress + Escalation *EscalationPayload `json:"escalation"` // null unless type=escalation +} + +// ProgressPayload is populated for type="progress" callbacks. +type ProgressPayload struct { + Status string `json:"status"` // e.g. "thinking" + ToolName string `json:"toolName,omitempty"` + StepNumber int `json:"stepNumber,omitempty"` + Message string `json:"message,omitempty"` +} + +// EscalationPayload is populated for type="escalation" callbacks. +type EscalationPayload struct { + Reason string `json:"reason"` + Notes string `json:"notes,omitempty"` } // requestContext stores per-request state so Send() can resolve callback info. @@ -332,12 +355,23 @@ func (c *MagicFormChannel) Send(ctx context.Context, msg bus.OutboundMessage) er return channels.ErrNotRunning } - // Look up request context - val, ok := c.requests.LoadAndDelete(msg.ChatID) - if !ok { - return fmt.Errorf("%w: no request context for chatID %s", channels.ErrSendFailed, msg.ChatID) + // For progress/escalation messages, Load (keep) the context since the final + // message is still coming. For final messages, LoadAndDelete to clean up. + isFinal := msg.Type == bus.MessageTypeFinal + var reqCtx *requestContext + if isFinal { + val, ok := c.requests.LoadAndDelete(msg.ChatID) + if !ok { + return fmt.Errorf("%w: no request context for chatID %s", channels.ErrSendFailed, msg.ChatID) + } + reqCtx = val.(*requestContext) + } else { + val, ok := c.requests.Load(msg.ChatID) + if !ok { + return fmt.Errorf("%w: no request context for chatID %s", channels.ErrSendFailed, msg.ChatID) + } + reqCtx = val.(*requestContext) } - reqCtx := val.(*requestContext) // Resolve callback URL callbackURL := reqCtx.callbackURL @@ -350,11 +384,44 @@ func (c *MagicFormChannel) Send(ctx context.Context, msg bus.OutboundMessage) er } // Build callback payload + taskID := fmt.Sprintf("claw_task_%s_%d", reqCtx.conversationID, reqCtx.createdAt.UnixMilli()) payload := CallbackPayload{ StackID: reqCtx.stackID, ConversationID: reqCtx.conversationID, + TaskID: taskID, + Runtime: "picoclaw", Response: msg.Content, - Type: "final", + } + + switch msg.Type { + case bus.MessageTypeProgress: + payload.Type = "progress" + payload.Status = "success" + if msg.Progress != nil { + payload.Progress = &ProgressPayload{ + Status: msg.Progress.Status, + ToolName: msg.Progress.ToolName, + StepNumber: msg.Progress.StepNumber, + Message: msg.Progress.Message, + } + } + case bus.MessageTypeEscalation: + payload.Type = "escalation" + payload.Status = "success" + if msg.Escalation != nil { + payload.Escalation = &EscalationPayload{ + Reason: msg.Escalation.Reason, + Notes: msg.Escalation.Notes, + } + } + default: // final + payload.Type = "final" + payload.Status = "success" + if m := msg.Metrics; m != nil { + payload.DurationMs = m.DurationMs + payload.ToolCalls = m.ToolCalls + payload.TokenUsage = m.TokenUsage + } } body, err := json.Marshal(payload) @@ -385,6 +452,7 @@ func (c *MagicFormChannel) Send(ctx context.Context, msg bus.OutboundMessage) er logger.InfoCF("magicform", "Callback sent", map[string]any{ "conversation_id": reqCtx.conversationID, + "type": payload.Type, "status": resp.StatusCode, })