From 514fcfe77e4c576e582e6d402091be784b6bb771 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 7 Jan 2026 04:48:20 +0000 Subject: [PATCH] fix: harden sub-agent model overrides --- CHANGELOG.md | 1 + docs/tools/subagents.md | 2 +- src/agents/clawdbot-tools.subagents.test.ts | 45 +++++++++++++++++-- src/agents/pi-tool-definition-adapter.test.ts | 28 ++++++++++++ src/agents/pi-tool-definition-adapter.ts | 21 ++++++++- src/agents/tools/sessions-spawn-tool.ts | 27 ++++++++--- 6 files changed, 113 insertions(+), 11 deletions(-) create mode 100644 src/agents/pi-tool-definition-adapter.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 9cdb868b5..744737a4f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ - CLI: add `clawdbot docs` live docs search with pretty output. - Agent: treat compaction retry AbortError as a fallback trigger without swallowing non-abort errors. Thanks @erikpr1994 for PR #341. - Sub-agents: allow `sessions_spawn` model overrides and error on invalid models. Thanks @azade-c for PR #298. +- Sub-agents: skip invalid model overrides with a warning and keep the run alive; tool exceptions now return tool errors instead of crashing the agent. - Heartbeat: default interval 30m; clarified default prompt usage and HEARTBEAT.md template behavior. - Onboarding: write auth profiles to the multi-agent path (`~/.clawdbot/agents/main/agent/`) so the gateway finds credentials on first startup. Thanks @minghinmatthewlam for PR #327. - Docs: add missing `ui:install` setup step in the README. Thanks @hugobarauna for PR #300. diff --git a/docs/tools/subagents.md b/docs/tools/subagents.md index 939df523e..6286e3d6b 100644 --- a/docs/tools/subagents.md +++ b/docs/tools/subagents.md @@ -24,7 +24,7 @@ Use `sessions_spawn`: Tool params: - `task` (required) - `label?` (optional) -- `model?` (optional; overrides the sub-agent model; invalid values error) +- `model?` (optional; overrides the sub-agent model; invalid values are skipped and the sub-agent runs on the default model with a warning in the tool result) - `timeoutSeconds?` (default `0`; `0` = fire-and-forget) - `cleanup?` (`delete|keep`, default `delete`) diff --git a/src/agents/clawdbot-tools.subagents.test.ts b/src/agents/clawdbot-tools.subagents.test.ts index a4a9e308b..e27ff2d17 100644 --- a/src/agents/clawdbot-tools.subagents.test.ts +++ b/src/agents/clawdbot-tools.subagents.test.ts @@ -277,9 +277,12 @@ describe("subagents", () => { }); }); - it("sessions_spawn fails when model override is invalid", async () => { + it("sessions_spawn skips invalid model overrides and continues", async () => { callGatewayMock.mockReset(); const calls: Array<{ method?: string; params?: unknown }> = []; + let agentCallCount = 0; + let lastWaitedRunId: string | undefined; + const replyByRunId = new Map(); callGatewayMock.mockImplementation(async (opts: unknown) => { const request = opts as { method?: string; params?: unknown }; @@ -287,6 +290,37 @@ describe("subagents", () => { if (request.method === "sessions.patch") { throw new Error("invalid model: bad-model"); } + if (request.method === "agent") { + agentCallCount += 1; + const runId = `run-${agentCallCount}`; + const params = request.params as + | { message?: string; sessionKey?: string } + | undefined; + const message = params?.message ?? ""; + const reply = + message === "Sub-agent announce step." ? "ANNOUNCE_SKIP" : "done"; + replyByRunId.set(runId, reply); + return { + runId, + status: "accepted", + acceptedAt: 4000 + agentCallCount, + }; + } + if (request.method === "agent.wait") { + const params = request.params as { runId?: string } | undefined; + lastWaitedRunId = params?.runId; + return { runId: params?.runId ?? "run-1", status: "ok" }; + } + if (request.method === "chat.history") { + const text = + (lastWaitedRunId && replyByRunId.get(lastWaitedRunId)) ?? ""; + return { + messages: [{ role: "assistant", content: [{ type: "text", text }] }], + }; + } + if (request.method === "sessions.delete") { + return { ok: true }; + } return {}; }); @@ -301,10 +335,13 @@ describe("subagents", () => { timeoutSeconds: 1, model: "bad-model", }); - expect(result.details).toMatchObject({ status: "error" }); + expect(result.details).toMatchObject({ + status: "ok", + modelApplied: false, + }); expect( - String((result.details as { error?: string }).error ?? ""), + String((result.details as { warning?: string }).warning ?? ""), ).toContain("invalid model"); - expect(calls.some((call) => call.method === "agent")).toBe(false); + expect(calls.some((call) => call.method === "agent")).toBe(true); }); }); diff --git a/src/agents/pi-tool-definition-adapter.test.ts b/src/agents/pi-tool-definition-adapter.test.ts new file mode 100644 index 000000000..e05d579c0 --- /dev/null +++ b/src/agents/pi-tool-definition-adapter.test.ts @@ -0,0 +1,28 @@ +import { describe, expect, it } from "vitest"; + +import type { AgentTool } from "@mariozechner/pi-agent-core"; + +import { toToolDefinitions } from "./pi-tool-definition-adapter.js"; + +describe("pi tool definition adapter", () => { + it("wraps tool errors into a tool result", async () => { + const tool = { + name: "boom", + label: "Boom", + description: "throws", + parameters: {}, + execute: async () => { + throw new Error("nope"); + }, + } satisfies AgentTool; + + const defs = toToolDefinitions([tool]); + const result = await defs[0].execute("call1", {}, undefined, undefined); + + expect(result.details).toMatchObject({ + status: "error", + tool: "boom", + }); + expect(JSON.stringify(result.details)).toContain("nope"); + }); +}); diff --git a/src/agents/pi-tool-definition-adapter.ts b/src/agents/pi-tool-definition-adapter.ts index fdb0777f4..6df70bf6b 100644 --- a/src/agents/pi-tool-definition-adapter.ts +++ b/src/agents/pi-tool-definition-adapter.ts @@ -4,6 +4,8 @@ import type { AgentToolUpdateCallback, } from "@mariozechner/pi-agent-core"; import type { ToolDefinition } from "@mariozechner/pi-coding-agent"; +import { logError } from "../logger.js"; +import { jsonResult } from "./tools/common.js"; // biome-ignore lint/suspicious/noExplicitAny: TypeBox schema type from pi-agent-core uses a different module instance. type AnyAgentTool = AgentTool; @@ -26,7 +28,24 @@ export function toToolDefinitions(tools: AnyAgentTool[]): ToolDefinition[] { ): Promise> => { // KNOWN: pi-coding-agent `ToolDefinition.execute` has a different signature/order // than pi-agent-core `AgentTool.execute`. This adapter keeps our existing tools intact. - return tool.execute(toolCallId, params, signal, onUpdate); + try { + return await tool.execute(toolCallId, params, signal, onUpdate); + } catch (err) { + if (signal?.aborted) throw err; + const name = + err && typeof err === "object" && "name" in err + ? String((err as { name?: unknown }).name) + : ""; + if (name === "AbortError") throw err; + const message = + err instanceof Error ? err.stack ?? err.message : String(err); + logError(`[tools] ${tool.name} failed: ${message}`); + return jsonResult({ + status: "error", + tool: tool.name, + error: message, + }); + } }, } satisfies ToolDefinition; }); diff --git a/src/agents/tools/sessions-spawn-tool.ts b/src/agents/tools/sessions-spawn-tool.ts index ef8db652e..650638082 100644 --- a/src/agents/tools/sessions-spawn-tool.ts +++ b/src/agents/tools/sessions-spawn-tool.ts @@ -190,6 +190,8 @@ export function createSessionsSpawnTool(opts?: { ? Math.max(0, Math.floor(params.timeoutSeconds)) : 0; const timeoutMs = timeoutSeconds * 1000; + let modelWarning: string | undefined; + let modelApplied = false; const cfg = loadConfig(); const { mainKey, alias } = resolveMainSessionAlias(cfg); @@ -238,6 +240,7 @@ export function createSessionsSpawnTool(opts?: { params: { key: childSessionKey, model }, timeoutMs: 10_000, }); + modelApplied = true; } catch (err) { const messageText = err instanceof Error @@ -245,11 +248,17 @@ export function createSessionsSpawnTool(opts?: { : typeof err === "string" ? err : "error"; - return jsonResult({ - status: "error", - error: messageText, - childSessionKey, - }); + const recoverable = + messageText.includes("invalid model") || + messageText.includes("model not allowed"); + if (!recoverable) { + return jsonResult({ + status: "error", + error: messageText, + childSessionKey, + }); + } + modelWarning = messageText; } } const childSystemPrompt = buildSubagentSystemPrompt({ @@ -307,6 +316,8 @@ export function createSessionsSpawnTool(opts?: { status: "accepted", childSessionKey, runId: childRunId, + modelApplied: model ? modelApplied : undefined, + warning: modelWarning, }); } @@ -354,6 +365,8 @@ export function createSessionsSpawnTool(opts?: { error: waitError, childSessionKey, runId: childRunId, + modelApplied: model ? modelApplied : undefined, + warning: modelWarning, }); } if (waitStatus === "error") { @@ -372,6 +385,8 @@ export function createSessionsSpawnTool(opts?: { error: waitError ?? "agent error", childSessionKey, runId: childRunId, + modelApplied: model ? modelApplied : undefined, + warning: modelWarning, }); } @@ -395,6 +410,8 @@ export function createSessionsSpawnTool(opts?: { childSessionKey, runId: childRunId, reply: replyText, + modelApplied: model ? modelApplied : undefined, + warning: modelWarning, }); }, };