mirror of
https://github.com/rowboatlabs/rowboat.git
synced 2026-07-12 21:02:17 +02:00
fix(x): never record a repository failure as a tool's own error result
executeSyncTool's success-result append sat inside the try whose catch
appends an error tool result. A transient repository failure (disk
pressure, EMFILE) on that append was therefore durably recorded as the
TOOL having failed — after its side effect already happened — inviting
the model to retry the side effect on the next call (double-sent email,
duplicate issue). This violated §21.4: repository failures must reject
the execution, not masquerade as tool outcomes.
The try now covers execution and result parsing only (a tool returning
garbage is still a tool error). The success append happens outside: a
failure there propagates as an infrastructure error, the log keeps the
invocation without a result, and the existing §23 recovery closes it
honestly on re-advance ("outcome is unknown and it was not retried")
without re-executing.
Tests: a repo fake failing exactly the success append — asserts no
false error is persisted, the outcome rejects, recovery yields the
indeterminate result without re-execution, and a committed sibling
result survives untouched with correct model-facing ordering; plus a
parse-failure case pinning garbage returns as tool errors. Both repo
tests fail against the old code.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
235f5802b9
commit
46ad5aa3b6
2 changed files with 227 additions and 10 deletions
|
|
@ -45,6 +45,8 @@ import type {
|
||||||
// Fixtures
|
// Fixtures
|
||||||
// ---------------------------------------------------------------------------
|
// ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
type TEvent = z.infer<typeof TurnEvent>;
|
||||||
|
|
||||||
const TS = "2026-07-02T10:00:00Z";
|
const TS = "2026-07-02T10:00:00Z";
|
||||||
|
|
||||||
const echoDescriptor: z.infer<typeof ToolDescriptor> = {
|
const echoDescriptor: z.infer<typeof ToolDescriptor> = {
|
||||||
|
|
@ -2230,6 +2232,215 @@ describe("concurrent sync tool execution (10.5)", () => {
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// Fails appends whose batch matches the predicate; counts the failures.
|
||||||
|
class FailingAppendRepo extends InMemoryTurnRepo {
|
||||||
|
failWhen?: (events: TEvent[]) => boolean;
|
||||||
|
failures = 0;
|
||||||
|
|
||||||
|
override async append(turnId: string, events: TEvent[]): Promise<void> {
|
||||||
|
if (this.failWhen?.(events)) {
|
||||||
|
this.failures += 1;
|
||||||
|
throw new Error("disk full");
|
||||||
|
}
|
||||||
|
return super.append(turnId, events);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
describe("sync tool result append failures (21.4)", () => {
|
||||||
|
const isSyncSuccessResult = (events: TEvent[]) =>
|
||||||
|
events.some(
|
||||||
|
(e) =>
|
||||||
|
e.type === "tool_result" &&
|
||||||
|
e.source === "sync" &&
|
||||||
|
e.result.isError === false,
|
||||||
|
);
|
||||||
|
|
||||||
|
it("rejects as infrastructure instead of recording a false tool error, then recovers honestly", async () => {
|
||||||
|
const executed: unknown[] = [];
|
||||||
|
const repo = new FailingAppendRepo();
|
||||||
|
repo.failWhen = isSyncSuccessResult;
|
||||||
|
const { runtime } = makeRuntime({
|
||||||
|
repo,
|
||||||
|
tools: [
|
||||||
|
syncTool(echoDescriptor, async (input) => {
|
||||||
|
executed.push(input);
|
||||||
|
return { output: "sent", isError: false };
|
||||||
|
}),
|
||||||
|
...defaultTools.slice(1),
|
||||||
|
],
|
||||||
|
models: [
|
||||||
|
respond(
|
||||||
|
completedResp(assistantCalls(toolCallPart("A", "echo"))),
|
||||||
|
),
|
||||||
|
respond(completedResp(assistantText("done"))),
|
||||||
|
],
|
||||||
|
});
|
||||||
|
const turnId = await newTurn(runtime);
|
||||||
|
|
||||||
|
// The tool ran (side effect happened) but its result could not be
|
||||||
|
// persisted: the invocation rejects as infrastructure, and the log
|
||||||
|
// must NOT contain a fabricated error result claiming the tool
|
||||||
|
// failed.
|
||||||
|
const first = await advanceAndSettle(runtime, turnId);
|
||||||
|
expect(first.outcome).toBeUndefined();
|
||||||
|
expect(String(first.error)).toContain("disk full");
|
||||||
|
expect(repo.failures).toBe(1);
|
||||||
|
expect(executed).toHaveLength(1);
|
||||||
|
let log = await persisted(repo, turnId);
|
||||||
|
expect(typesOf(log)).toEqual([
|
||||||
|
"turn_created",
|
||||||
|
"model_call_requested",
|
||||||
|
"model_call_completed",
|
||||||
|
"tool_invocation_requested",
|
||||||
|
]);
|
||||||
|
|
||||||
|
// Recovery: the interrupted invocation is closed with the honest
|
||||||
|
// indeterminate result — never re-executed — and the turn continues
|
||||||
|
// to completion.
|
||||||
|
repo.failWhen = undefined;
|
||||||
|
const second = await advanceAndSettle(runtime, turnId);
|
||||||
|
expect(second.outcome?.status).toBe("completed");
|
||||||
|
expect(executed).toHaveLength(1);
|
||||||
|
log = await persisted(repo, turnId);
|
||||||
|
const result = log.find((e) => e.type === "tool_result");
|
||||||
|
expect(result).toMatchObject({
|
||||||
|
source: "runtime",
|
||||||
|
result: {
|
||||||
|
output: "Tool execution was interrupted; its outcome is unknown and it was not retried.",
|
||||||
|
isError: true,
|
||||||
|
},
|
||||||
|
});
|
||||||
|
expect(typesOf(log)).toContain("turn_completed");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("one tool's failed result append does not disturb a committed sibling result", async () => {
|
||||||
|
const slowDescriptor: z.infer<typeof ToolDescriptor> = {
|
||||||
|
toolId: "tool.slow",
|
||||||
|
name: "slow",
|
||||||
|
description: "Slow tool",
|
||||||
|
inputSchema: {},
|
||||||
|
execution: "sync",
|
||||||
|
requiresHuman: false,
|
||||||
|
};
|
||||||
|
const agent: z.infer<typeof ResolvedAgent> = {
|
||||||
|
...defaultAgent,
|
||||||
|
tools: [echoDescriptor, slowDescriptor],
|
||||||
|
};
|
||||||
|
const executions: string[] = [];
|
||||||
|
const repo = new FailingAppendRepo();
|
||||||
|
// Fail only echo's SUCCESS result; slow's must commit. (Old code
|
||||||
|
// would then durably record echo as failed via the catch path — the
|
||||||
|
// recovery assertions below reject that fabrication.)
|
||||||
|
repo.failWhen = (events) =>
|
||||||
|
events.some(
|
||||||
|
(e) =>
|
||||||
|
e.type === "tool_result" &&
|
||||||
|
e.source === "sync" &&
|
||||||
|
e.toolCallId === "E" &&
|
||||||
|
e.result.isError === false,
|
||||||
|
);
|
||||||
|
const { runtime } = makeRuntime({
|
||||||
|
repo,
|
||||||
|
agent,
|
||||||
|
tools: [
|
||||||
|
syncTool(echoDescriptor, async () => {
|
||||||
|
executions.push("echo");
|
||||||
|
return { output: "e-done", isError: false };
|
||||||
|
}),
|
||||||
|
syncTool(slowDescriptor, async () => {
|
||||||
|
executions.push("slow");
|
||||||
|
return { output: "s-done", isError: false };
|
||||||
|
}),
|
||||||
|
],
|
||||||
|
models: [
|
||||||
|
respond(
|
||||||
|
completedResp(
|
||||||
|
assistantCalls(
|
||||||
|
toolCallPart("E", "echo"),
|
||||||
|
toolCallPart("S", "slow"),
|
||||||
|
),
|
||||||
|
),
|
||||||
|
),
|
||||||
|
respond(completedResp(assistantText("done"))),
|
||||||
|
],
|
||||||
|
});
|
||||||
|
const turnId = await newTurn(runtime);
|
||||||
|
|
||||||
|
const first = await advanceAndSettle(runtime, turnId);
|
||||||
|
expect(first.outcome).toBeUndefined();
|
||||||
|
expect(String(first.error)).toContain("disk full");
|
||||||
|
let log = await persisted(repo, turnId);
|
||||||
|
// Both invocations durable; exactly the sibling's result committed
|
||||||
|
// (the append chain stays alive after a failed commit).
|
||||||
|
const results = log.filter((e) => e.type === "tool_result");
|
||||||
|
expect(results).toHaveLength(1);
|
||||||
|
expect(results[0]).toMatchObject({
|
||||||
|
toolCallId: "S",
|
||||||
|
result: { output: "s-done", isError: false },
|
||||||
|
});
|
||||||
|
|
||||||
|
// Recovery: echo closes as indeterminate, slow's real result is
|
||||||
|
// preserved, neither re-executes, and the batch reaches the model
|
||||||
|
// in source order.
|
||||||
|
repo.failWhen = undefined;
|
||||||
|
const second = await advanceAndSettle(runtime, turnId);
|
||||||
|
expect(second.outcome?.status).toBe("completed");
|
||||||
|
expect(executions.sort()).toEqual(["echo", "slow"]);
|
||||||
|
log = await persisted(repo, turnId);
|
||||||
|
const byId = new Map(
|
||||||
|
log
|
||||||
|
.filter((e) => e.type === "tool_result")
|
||||||
|
.map((e) => [e.toolCallId, e]),
|
||||||
|
);
|
||||||
|
expect(byId.get("E")).toMatchObject({
|
||||||
|
source: "runtime",
|
||||||
|
result: { isError: true },
|
||||||
|
});
|
||||||
|
expect(byId.get("S")).toMatchObject({
|
||||||
|
source: "sync",
|
||||||
|
result: { output: "s-done", isError: false },
|
||||||
|
});
|
||||||
|
const followUp = log.find(
|
||||||
|
(e) => e.type === "model_call_requested" && e.modelCallIndex === 1,
|
||||||
|
);
|
||||||
|
expect(followUp).toMatchObject({
|
||||||
|
request: {
|
||||||
|
messages: ["assistant:0", "toolResult:E", "toolResult:S"],
|
||||||
|
},
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
it("a tool returning an unparseable result is a tool error, not infrastructure", async () => {
|
||||||
|
const { runtime, repo } = makeRuntime({
|
||||||
|
tools: [
|
||||||
|
syncTool(
|
||||||
|
echoDescriptor,
|
||||||
|
async () =>
|
||||||
|
"garbage" as unknown as Awaited<
|
||||||
|
ReturnType<SyncRuntimeTool["execute"]>
|
||||||
|
>,
|
||||||
|
),
|
||||||
|
...defaultTools.slice(1),
|
||||||
|
],
|
||||||
|
models: [
|
||||||
|
respond(
|
||||||
|
completedResp(assistantCalls(toolCallPart("A", "echo"))),
|
||||||
|
),
|
||||||
|
respond(completedResp(assistantText("done"))),
|
||||||
|
],
|
||||||
|
});
|
||||||
|
const turnId = await newTurn(runtime);
|
||||||
|
const { outcome } = await advanceAndSettle(runtime, turnId);
|
||||||
|
expect(outcome?.status).toBe("completed");
|
||||||
|
const log = await persisted(repo, turnId);
|
||||||
|
const result = log.find((e) => e.type === "tool_result");
|
||||||
|
expect(result).toMatchObject({
|
||||||
|
source: "sync",
|
||||||
|
result: { isError: true },
|
||||||
|
});
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
describe("mid-turn tool extension", () => {
|
describe("mid-turn tool extension", () => {
|
||||||
const writeDescriptor: z.infer<typeof ToolDescriptor> = {
|
const writeDescriptor: z.infer<typeof ToolDescriptor> = {
|
||||||
toolId: "tool.write",
|
toolId: "tool.write",
|
||||||
|
|
|
||||||
|
|
@ -888,7 +888,13 @@ class TurnAdvance {
|
||||||
tc: ToolCallState,
|
tc: ToolCallState,
|
||||||
syncTool: SyncRuntimeTool,
|
syncTool: SyncRuntimeTool,
|
||||||
): Promise<void> {
|
): Promise<void> {
|
||||||
let settled: z.infer<typeof ToolResultData> | undefined;
|
// The try covers tool execution only (including parsing its return —
|
||||||
|
// a tool returning garbage is a tool error). The success-result
|
||||||
|
// append stays OUTSIDE: a repository failure there must reject the
|
||||||
|
// invocation as infrastructure, never masquerade as a failed tool —
|
||||||
|
// the side effect already happened, and a durable false error would
|
||||||
|
// invite the model to retry it (§21.4).
|
||||||
|
let settled: z.infer<typeof ToolResultData>;
|
||||||
try {
|
try {
|
||||||
const result = await syncTool.execute(tc.input, {
|
const result = await syncTool.execute(tc.input, {
|
||||||
turnId: this.turnId,
|
turnId: this.turnId,
|
||||||
|
|
@ -906,15 +912,6 @@ class TurnAdvance {
|
||||||
},
|
},
|
||||||
});
|
});
|
||||||
settled = ToolResultData.parse(result);
|
settled = ToolResultData.parse(result);
|
||||||
await this.append({
|
|
||||||
type: "tool_result",
|
|
||||||
turnId: this.turnId,
|
|
||||||
ts: this.now(),
|
|
||||||
toolCallId: tc.toolCallId,
|
|
||||||
toolName: tc.toolName,
|
|
||||||
source: "sync",
|
|
||||||
result: settled,
|
|
||||||
});
|
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
if (this.signal.aborted) {
|
if (this.signal.aborted) {
|
||||||
await this.append(
|
await this.append(
|
||||||
|
|
@ -936,6 +933,15 @@ class TurnAdvance {
|
||||||
});
|
});
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
await this.append({
|
||||||
|
type: "tool_result",
|
||||||
|
turnId: this.turnId,
|
||||||
|
ts: this.now(),
|
||||||
|
toolCallId: tc.toolCallId,
|
||||||
|
toolName: tc.toolName,
|
||||||
|
source: "sync",
|
||||||
|
result: settled,
|
||||||
|
});
|
||||||
await this.maybeExtendTools(tc, settled);
|
await this.maybeExtendTools(tc, settled);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue