From 52b6bf04af9ec5fd777ee65f148210b0220d9cb5 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Fri, 23 Jan 2026 00:59:44 +0000 Subject: [PATCH] fix: improve tool summaries --- CHANGELOG.md | 1 + docs/gateway/logging.md | 2 +- .../pi-embedded-runner/run/payloads.test.ts | 2 +- ...vas-action-metadata-tool-summaries.test.ts | 2 +- ...ompaction-retries-before-resolving.test.ts | 4 +- src/agents/subagent-registry.ts | 3 +- src/agents/tool-display.json | 15 ++++- src/agents/tool-display.test.ts | 56 ++++++++++++++++++ src/agents/tool-display.ts | 58 +++++++++++++++++-- src/auto-reply/tool-meta.test.ts | 8 +-- 10 files changed, 134 insertions(+), 17 deletions(-) create mode 100644 src/agents/tool-display.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 6622569d5..871c19857 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,7 @@ Docs: https://docs.clawd.bot - Agents: surface concrete API error details instead of generic AI service errors. - Exec approvals: allow per-segment allowlists for chained shell commands on gateway + node hosts. (#1458) Thanks @czekaj. - Agents: make OpenAI sessions image-sanitize-only; gate tool-id/repair sanitization by provider. +- Agents: make tool summaries more readable and only show optional params when set. - Docs: fix gog auth services example to include docs scope. (#1454) Thanks @zerone0x. - macOS: prefer linked channels in gateway summary to avoid false “not linked” status. diff --git a/docs/gateway/logging.md b/docs/gateway/logging.md index 3c36e8bf5..10ad2cf9d 100644 --- a/docs/gateway/logging.md +++ b/docs/gateway/logging.md @@ -51,7 +51,7 @@ You can tune console verbosity independently via: ## Tool summary redaction -Verbose tool summaries (e.g. `🛠️ exec: ...`) can mask sensitive tokens before they hit the +Verbose tool summaries (e.g. `🛠️ Exec: ...`) can mask sensitive tokens before they hit the console stream. This is **tools-only** and does not alter file logs. - `logging.redactSensitive`: `off` | `tools` (default: `tools`) diff --git a/src/agents/pi-embedded-runner/run/payloads.test.ts b/src/agents/pi-embedded-runner/run/payloads.test.ts index a27fe3262..f3dfc71e0 100644 --- a/src/agents/pi-embedded-runner/run/payloads.test.ts +++ b/src/agents/pi-embedded-runner/run/payloads.test.ts @@ -127,7 +127,7 @@ describe("buildEmbeddedRunPayloads", () => { expect(payloads).toHaveLength(1); expect(payloads[0]?.isError).toBe(true); - expect(payloads[0]?.text).toContain("browser"); + expect(payloads[0]?.text).toContain("Browser"); expect(payloads[0]?.text).toContain("tab not found"); }); diff --git a/src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.includes-canvas-action-metadata-tool-summaries.test.ts b/src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.includes-canvas-action-metadata-tool-summaries.test.ts index 02fb7b940..37532c48a 100644 --- a/src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.includes-canvas-action-metadata-tool-summaries.test.ts +++ b/src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.includes-canvas-action-metadata-tool-summaries.test.ts @@ -44,7 +44,7 @@ describe("subscribeEmbeddedPiSession", () => { expect(onToolResult).toHaveBeenCalledTimes(1); const payload = onToolResult.mock.calls[0][0]; expect(payload.text).toContain("🖼️"); - expect(payload.text).toContain("canvas"); + expect(payload.text).toContain("Canvas"); expect(payload.text).toContain("A2UI push"); expect(payload.text).toContain("/tmp/a2ui.jsonl"); }); diff --git a/src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.waits-multiple-compaction-retries-before-resolving.test.ts b/src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.waits-multiple-compaction-retries-before-resolving.test.ts index 7483206b7..6d31dae66 100644 --- a/src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.waits-multiple-compaction-retries-before-resolving.test.ts +++ b/src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.waits-multiple-compaction-retries-before-resolving.test.ts @@ -166,7 +166,7 @@ describe("subscribeEmbeddedPiSession", () => { expect(onToolResult).toHaveBeenCalledTimes(1); const payload = onToolResult.mock.calls[0][0]; expect(payload.text).toContain("🌐"); - expect(payload.text).toContain("browser"); + expect(payload.text).toContain("Browser"); expect(payload.text).toContain("snapshot"); expect(payload.text).toContain("https://example.com"); }); @@ -200,7 +200,7 @@ describe("subscribeEmbeddedPiSession", () => { expect(onToolResult).toHaveBeenCalledTimes(1); const summary = onToolResult.mock.calls[0][0]; - expect(summary.text).toContain("exec"); + expect(summary.text).toContain("Exec"); expect(summary.text).toContain("pty"); handler?.({ diff --git a/src/agents/subagent-registry.ts b/src/agents/subagent-registry.ts index d39bb5fe4..d325e40e2 100644 --- a/src/agents/subagent-registry.ts +++ b/src/agents/subagent-registry.ts @@ -31,7 +31,8 @@ const subagentRuns = new Map(); let sweeper: NodeJS.Timeout | null = null; let listenerStarted = false; let listenerStop: (() => void) | null = null; -let restoreAttempted = false; +// Use var to avoid TDZ when init runs across circular imports during bootstrap. +var restoreAttempted = false; function persistSubagentRuns() { try { diff --git a/src/agents/tool-display.json b/src/agents/tool-display.json index 603c51e3b..f2206e205 100644 --- a/src/agents/tool-display.json +++ b/src/agents/tool-display.json @@ -256,17 +256,26 @@ "sessions_history": { "emoji": "🧾", "title": "Session History", - "detailKeys": ["sessionKey", "limit"] + "detailKeys": ["sessionKey", "limit", "includeTools"] }, "sessions_send": { "emoji": "📨", "title": "Session Send", - "detailKeys": ["sessionKey", "timeoutSeconds"] + "detailKeys": ["label", "sessionKey", "agentId", "timeoutSeconds"] }, "sessions_spawn": { "emoji": "🧑‍🔧", "title": "Sub-agent", - "detailKeys": ["label", "agentId", "thinking", "runTimeoutSeconds", "cleanup"] + "detailKeys": [ + "label", + "task", + "agentId", + "model", + "thinking", + "runTimeoutSeconds", + "cleanup", + "timeoutSeconds" + ] }, "session_status": { "emoji": "📊", diff --git a/src/agents/tool-display.test.ts b/src/agents/tool-display.test.ts new file mode 100644 index 000000000..7d97f57f3 --- /dev/null +++ b/src/agents/tool-display.test.ts @@ -0,0 +1,56 @@ +import { describe, expect, it } from "vitest"; + +import { formatToolDetail, resolveToolDisplay } from "./tool-display.js"; + +describe("tool display details", () => { + it("skips zero/false values for optional detail fields", () => { + const detail = formatToolDetail( + resolveToolDisplay({ + name: "sessions_spawn", + args: { + task: "double-message-bug-gpt", + label: 0, + runTimeoutSeconds: 0, + timeoutSeconds: 0, + }, + }), + ); + + expect(detail).toBe("double-message-bug-gpt"); + }); + + it("includes only truthy boolean details", () => { + const detail = formatToolDetail( + resolveToolDisplay({ + name: "message", + args: { + action: "react", + provider: "discord", + to: "chan-1", + remove: false, + }, + }), + ); + + expect(detail).toContain("provider discord"); + expect(detail).toContain("to chan-1"); + expect(detail).not.toContain("remove"); + }); + + it("keeps positive numbers and true booleans", () => { + const detail = formatToolDetail( + resolveToolDisplay({ + name: "sessions_history", + args: { + sessionKey: "agent:main:main", + limit: 20, + includeTools: true, + }, + }), + ); + + expect(detail).toContain("session agent:main:main"); + expect(detail).toContain("limit 20"); + expect(detail).toContain("tools true"); + }); +}); diff --git a/src/agents/tool-display.ts b/src/agents/tool-display.ts index 1b125d376..da6eab476 100644 --- a/src/agents/tool-display.ts +++ b/src/agents/tool-display.ts @@ -33,6 +33,25 @@ export type ToolDisplay = { const TOOL_DISPLAY_CONFIG = TOOL_DISPLAY_JSON as ToolDisplayConfig; const FALLBACK = TOOL_DISPLAY_CONFIG.fallback ?? { emoji: "🧩" }; const TOOL_MAP = TOOL_DISPLAY_CONFIG.tools ?? {}; +const DETAIL_LABEL_OVERRIDES: Record = { + agentId: "agent", + sessionKey: "session", + targetId: "target", + targetUrl: "url", + nodeId: "node", + requestId: "request", + messageId: "message", + threadId: "thread", + channelId: "channel", + guildId: "guild", + userId: "user", + runTimeoutSeconds: "timeout", + timeoutSeconds: "timeout", + includeTools: "tools", + pollQuestion: "poll", + maxChars: "max chars", +}; +const MAX_DETAIL_ENTRIES = 8; function normalizeToolName(name?: string): string { return (name ?? "tool").trim(); @@ -66,7 +85,11 @@ function coerceDisplayValue(value: unknown): string | undefined { if (!firstLine) return undefined; return firstLine.length > 160 ? `${firstLine.slice(0, 157)}…` : firstLine; } - if (typeof value === "number" || typeof value === "boolean") { + if (typeof value === "boolean") { + return value ? "true" : undefined; + } + if (typeof value === "number") { + if (!Number.isFinite(value) || value === 0) return undefined; return String(value); } if (Array.isArray(value)) { @@ -92,13 +115,40 @@ function lookupValueByPath(args: unknown, path: string): unknown { return current; } +function formatDetailKey(raw: string): string { + const segments = raw.split(".").filter(Boolean); + const last = segments.at(-1) ?? raw; + const override = DETAIL_LABEL_OVERRIDES[last]; + if (override) return override; + const cleaned = last.replace(/_/g, " ").replace(/-/g, " "); + const spaced = cleaned.replace(/([a-z0-9])([A-Z])/g, "$1 $2"); + return spaced.trim().toLowerCase() || last.toLowerCase(); +} + function resolveDetailFromKeys(args: unknown, keys: string[]): string | undefined { + const entries: Array<{ label: string; value: string }> = []; for (const key of keys) { const value = lookupValueByPath(args, key); const display = coerceDisplayValue(value); - if (display) return display; + if (!display) continue; + entries.push({ label: formatDetailKey(key), value: display }); } - return undefined; + if (entries.length === 0) return undefined; + if (entries.length === 1) return entries[0].value; + + const seen = new Set(); + const unique: Array<{ label: string; value: string }> = []; + for (const entry of entries) { + const token = `${entry.label}:${entry.value}`; + if (seen.has(token)) continue; + seen.add(token); + unique.push(entry); + } + if (unique.length === 0) return undefined; + return unique + .slice(0, MAX_DETAIL_ENTRIES) + .map((entry) => `${entry.label} ${entry.value}`) + .join(" · "); } function resolveReadDetail(args: unknown): string | undefined { @@ -139,7 +189,7 @@ export function resolveToolDisplay(params: { const spec = TOOL_MAP[key]; const emoji = spec?.emoji ?? FALLBACK.emoji ?? "🧩"; const title = spec?.title ?? defaultTitle(name); - const label = spec?.label ?? name; + const label = spec?.label ?? title; const actionRaw = params.args && typeof params.args === "object" ? ((params.args as Record).action as string | undefined) diff --git a/src/auto-reply/tool-meta.test.ts b/src/auto-reply/tool-meta.test.ts index cdcf8abe1..6447effd9 100644 --- a/src/auto-reply/tool-meta.test.ts +++ b/src/auto-reply/tool-meta.test.ts @@ -30,7 +30,7 @@ describe("tool meta formatting", () => { "note", "a→b", ]); - expect(out).toMatch(/^🧩 fs/); + expect(out).toMatch(/^🧩 Fs/); expect(out).toContain("~/dir/{a.txt, b.txt}"); expect(out).toContain("note"); expect(out).toContain("a→b"); @@ -47,12 +47,12 @@ describe("tool meta formatting", () => { const out = formatToolAggregate("exec", ["cd /Users/test/dir && gemini 2>&1 · elevated"], { markdown: true, }); - expect(out).toBe("🛠️ exec: elevated · `cd ~/dir && gemini 2>&1`"); + expect(out).toBe("🛠️ Exec: elevated · `cd ~/dir && gemini 2>&1`"); }); it("formats prefixes with default labels", () => { vi.stubEnv("HOME", "/Users/test"); - expect(formatToolPrefix(undefined, undefined)).toBe("🧩 tool"); - expect(formatToolPrefix("x", "/Users/test/a.txt")).toBe("🧩 x: ~/a.txt"); + expect(formatToolPrefix(undefined, undefined)).toBe("🧩 Tool"); + expect(formatToolPrefix("x", "/Users/test/a.txt")).toBe("🧩 X: ~/a.txt"); }); });