Skip to content

Commit a019735

Browse files
committed
feat(subagent): require replay safety for recovery availability
A worker that wrote files or ran a shell command before its provider gave out was still marked continuable, so a successor with the same brief could repeat those side effects. Recovery is now available only when every tool the run requested or started is a read, search, or web read; anything else, including MCP and unknown tools, fails closed to side_effects_completed and normal director-led continuation.
1 parent 7d1bdcf commit a019735

5 files changed

Lines changed: 102 additions & 2 deletions

File tree

‎src/agent/tool-classification.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,19 @@ export const SEARCH_QUERY_TOOLS: ReadonlySet<string> = new Set([
2929
"search_files",
3030
]);
3131

32+
/**
33+
* Tools whose call a recovery may replay without repeating a side effect:
34+
* path-keyed reads, search queries, and web reads. Deliberately narrow and
35+
* closed: run_shell, writes, MCP, and any tool not named here make a failed
36+
* run ineligible for enforced recovery.
37+
*/
38+
export const REPLAY_SAFE_TOOLS: ReadonlySet<string> = new Set([
39+
...PATH_KEYED_READ_TOOLS,
40+
...SEARCH_QUERY_TOOLS,
41+
"web_fetch",
42+
"web_search",
43+
]);
44+
3245
/**
3346
* Tools that never need an approval prompt (cannot change the workspace):
3447
* the director's read surface minus run_shell/web_fetch/web_search, which

‎src/subagent/session-store-failure-record.test.ts‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,32 @@ describe("terminal failure record", () => {
156156
expect(after?.error).toBe("first");
157157
});
158158

159+
test("a retryable failure after a write or shell call is not replay-safe", () => {
160+
for (const tool of ["write_file", "run_shell", "mcp__x__y"]) {
161+
const { sessions } = store();
162+
const id = startRunning(sessions);
163+
sessions.appendEvent(id, toolStart("read_file", "c1"));
164+
sessions.appendEvent(id, toolStart(tool, "c2"));
165+
sessions.fail(id, "timeout", { failure_class: "provider_retryable" });
166+
expect(sessions.get(id)?.failure?.recovery).toEqual({
167+
available: false,
168+
reason: "side_effects_completed",
169+
});
170+
}
171+
});
172+
173+
test("a retryable failure after only read calls stays eligible", () => {
174+
const { sessions } = store();
175+
const id = startRunning(sessions);
176+
sessions.appendEvent(id, toolStart("read_file", "c1"));
177+
sessions.appendEvent(id, toolStart("grep", "c2"));
178+
sessions.fail(id, "timeout", { failure_class: "provider_retryable" });
179+
expect(sessions.get(id)?.failure?.recovery).toEqual({
180+
available: true,
181+
reason: "eligible",
182+
});
183+
});
184+
159185
test("a teardown failure reports partial cleanup", () => {
160186
const { sessions } = store();
161187
const id = startRunning(sessions);

‎src/subagent/session-store.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ import { canonicalToolName } from "../agent/canonical-tool-name.js";
3333
import {
3434
classifySubAgentFailure,
3535
failureClassForStopReason,
36+
isReplaySafeToolName,
3637
recoveryFor,
3738
type SubAgentFailureClass,
3839
type SubAgentFailureInput,
@@ -846,6 +847,10 @@ export function createSubAgentSessionStore(
846847
failureClass,
847848
attempt,
848849
followupTurn: followupTurns.has(id),
850+
// Every tool the run requested, started, or finished: a call
851+
// that started may have landed its side effect before the run
852+
// died, so this is a superset of completed calls.
853+
replaySafe: session.toolNames.every(isReplaySafeToolName),
849854
}),
850855
),
851856
attempt,

‎src/subagent/terminal-failure.test.ts‎

Lines changed: 45 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {
66
createSubAgentLoopGuardError,
77
failureClassForStopReason,
88
isProviderFailureClass,
9+
isReplaySafeToolName,
910
isSubAgentLoopGuardError,
1011
recoveryFor,
1112
} from "./terminal-failure.js";
@@ -108,6 +109,7 @@ describe("recoveryFor", () => {
108109
failureClass: "provider_retryable",
109110
attempt: 1,
110111
followupTurn: false,
112+
replaySafe: true,
111113
}),
112114
).toEqual({ available: true, reason: "eligible" });
113115
});
@@ -120,7 +122,12 @@ describe("recoveryFor", () => {
120122
"error",
121123
] as const)("%s is not_retryable", (failureClass) => {
122124
expect(
123-
recoveryFor({ failureClass, attempt: 1, followupTurn: false }),
125+
recoveryFor({
126+
failureClass,
127+
attempt: 1,
128+
followupTurn: false,
129+
replaySafe: true,
130+
}),
124131
).toEqual({ available: false, reason: "not_retryable" });
125132
});
126133

@@ -130,14 +137,50 @@ describe("recoveryFor", () => {
130137
failureClass: "provider_retryable",
131138
attempt: 1,
132139
followupTurn: true,
140+
replaySafe: true,
133141
}),
134142
).toEqual({ available: false, reason: "not_retryable" });
135143
});
136144

145+
test("a retryable failure after a non-replay-safe call has side effects", () => {
146+
expect(
147+
recoveryFor({
148+
failureClass: "provider_retryable",
149+
attempt: 1,
150+
followupTurn: false,
151+
replaySafe: false,
152+
}),
153+
).toEqual({ available: false, reason: "side_effects_completed" });
154+
});
155+
156+
test("only read, search, and web read tools are replay-safe", () => {
157+
for (const name of ["read_file", "read", "grep", "search_files"]) {
158+
expect(isReplaySafeToolName(name)).toBe(true);
159+
}
160+
for (const name of ["web_fetch", "web_search"]) {
161+
expect(isReplaySafeToolName(name)).toBe(true);
162+
}
163+
for (const name of [
164+
"write_file",
165+
"edit_file",
166+
"run_shell",
167+
"bash",
168+
"mcp__server__tool",
169+
"some_unknown_tool",
170+
]) {
171+
expect(isReplaySafeToolName(name)).toBe(false);
172+
}
173+
});
174+
137175
test("a failed recovery attempt is exhausted whatever its class", () => {
138176
for (const failureClass of ["provider_retryable", "error"] as const) {
139177
expect(
140-
recoveryFor({ failureClass, attempt: 2, followupTurn: false }),
178+
recoveryFor({
179+
failureClass,
180+
attempt: 2,
181+
followupTurn: false,
182+
replaySafe: true,
183+
}),
141184
).toEqual({ available: false, reason: "recovery_exhausted" });
142185
}
143186
});

‎src/subagent/terminal-failure.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@ import {
33
isResolvedProviderFailureError,
44
} from "../inference-error-message.js";
55
import { errorMessage } from "../agent/error-message.js";
6+
import { canonicalToolName } from "../agent/canonical-tool-name.js";
7+
import { REPLAY_SAFE_TOOLS } from "../agent/tool-classification.js";
68
import type { ForcedStopReason } from "./stop-policy.js";
79

810
/**
@@ -123,20 +125,31 @@ export function classifySubAgentFailure(
123125
return "error";
124126
}
125127

128+
/** Whether a call to `name` can be replayed by a recovery attempt. */
129+
export function isReplaySafeToolName(name: string): boolean {
130+
return REPLAY_SAFE_TOOLS.has(canonicalToolName(name));
131+
}
132+
126133
/**
127134
* Recovery availability for a freshly written record. The chain cap wins
128135
* over class: a recovery attempt that fails is exhausted whatever its cause.
136+
* Replay safety fails closed: any tool outside the read-only allowlist means
137+
* a replacement could repeat a side effect.
129138
*/
130139
export function recoveryFor(input: {
131140
failureClass: SubAgentFailureClass;
132141
attempt: number;
133142
followupTurn: boolean;
143+
replaySafe: boolean;
134144
}): SubAgentRecovery {
135145
if (input.attempt > 1) {
136146
return { available: false, reason: "recovery_exhausted" };
137147
}
138148
if (input.followupTurn || input.failureClass !== "provider_retryable") {
139149
return { available: false, reason: "not_retryable" };
140150
}
151+
if (!input.replaySafe) {
152+
return { available: false, reason: "side_effects_completed" };
153+
}
141154
return { available: true, reason: "eligible" };
142155
}

0 commit comments

Comments
 (0)