fix: harden sub-agent model overrides
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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`)
|
||||
|
||||
|
||||
@@ -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<string, string>();
|
||||
|
||||
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);
|
||||
});
|
||||
});
|
||||
|
||||
28
src/agents/pi-tool-definition-adapter.test.ts
Normal file
28
src/agents/pi-tool-definition-adapter.test.ts
Normal file
@@ -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<unknown, unknown>;
|
||||
|
||||
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");
|
||||
});
|
||||
});
|
||||
@@ -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<any, unknown>;
|
||||
@@ -26,7 +28,24 @@ export function toToolDefinitions(tools: AnyAgentTool[]): ToolDefinition[] {
|
||||
): Promise<AgentToolResult<unknown>> => {
|
||||
// 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;
|
||||
});
|
||||
|
||||
@@ -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,
|
||||
});
|
||||
},
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user