Skip to content

Commit dc25112

Browse files
bwachmanTheGreatAxios
authored andcommitted
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 a13cf88 commit dc25112

5 files changed

Lines changed: 108 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
@@ -186,6 +186,32 @@ describe("terminal failure record", () => {
186186
expect(after?.error).toBe("first");
187187
});
188188

189+
test("a retryable failure after a write or shell call is not replay-safe", () => {
190+
for (const tool of ["write_file", "run_shell", "mcp__x__y"]) {
191+
const { sessions } = store();
192+
const id = startRunning(sessions);
193+
sessions.appendEvent(id, toolStart("read_file", "c1"));
194+
sessions.appendEvent(id, toolStart(tool, "c2"));
195+
sessions.fail(id, "timeout", { failure_class: "provider_retryable" });
196+
expect(sessions.get(id)?.failure?.recovery).toEqual({
197+
available: false,
198+
reason: "side_effects_completed",
199+
});
200+
}
201+
});
202+
203+
test("a retryable failure after only read calls stays eligible", () => {
204+
const { sessions } = store();
205+
const id = startRunning(sessions);
206+
sessions.appendEvent(id, toolStart("read_file", "c1"));
207+
sessions.appendEvent(id, toolStart("grep", "c2"));
208+
sessions.fail(id, "timeout", { failure_class: "provider_retryable" });
209+
expect(sessions.get(id)?.failure?.recovery).toEqual({
210+
available: true,
211+
reason: "eligible",
212+
});
213+
});
214+
189215
test("a teardown failure reports partial cleanup", () => {
190216
const { sessions } = store();
191217
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
isSubAgentLoopGuardError,
3738
recoveryFor,
3839
type SubAgentFailureClass,
@@ -843,6 +844,10 @@ export function createSubAgentSessionStore(
843844
attempt,
844845
followupTurn: followupTurns.has(id),
845846
cause: pendingFailures.get(id)?.cause,
847+
// Every tool the run requested, started, or finished: a call
848+
// that started may have landed its side effect before the run
849+
// died, so this is a superset of completed calls.
850+
replaySafe: session.toolNames.every(isReplaySafeToolName),
846851
}),
847852
),
848853
attempt,

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

Lines changed: 51 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

@@ -131,6 +138,7 @@ describe("recoveryFor", () => {
131138
attempt: 1,
132139
followupTurn: false,
133140
cause: providerError("retryable"),
141+
replaySafe: true,
134142
}),
135143
).toEqual({ available: true, reason: "eligible" });
136144
expect(
@@ -139,6 +147,7 @@ describe("recoveryFor", () => {
139147
attempt: 1,
140148
followupTurn: false,
141149
cause: createSubAgentLoopGuardError(providerError("timeout")),
150+
replaySafe: true,
142151
}),
143152
).toEqual({ available: true, reason: "eligible" });
144153
});
@@ -150,6 +159,7 @@ describe("recoveryFor", () => {
150159
attempt: 1,
151160
followupTurn: false,
152161
cause: providerError("credential_failure"),
162+
replaySafe: true,
153163
}),
154164
).toEqual({ available: false, reason: "not_retryable" });
155165
expect(
@@ -158,6 +168,7 @@ describe("recoveryFor", () => {
158168
attempt: 1,
159169
followupTurn: false,
160170
cause: new Error("doom"),
171+
replaySafe: true,
161172
}),
162173
).toEqual({ available: false, reason: "not_retryable" });
163174
expect(
@@ -166,6 +177,7 @@ describe("recoveryFor", () => {
166177
attempt: 1,
167178
followupTurn: true,
168179
cause: providerError("retryable"),
180+
replaySafe: true,
169181
}),
170182
).toEqual({ available: false, reason: "not_retryable" });
171183
expect(
@@ -174,6 +186,7 @@ describe("recoveryFor", () => {
174186
attempt: 2,
175187
followupTurn: false,
176188
cause: providerError("retryable"),
189+
replaySafe: true,
177190
}),
178191
).toEqual({ available: false, reason: "recovery_exhausted" });
179192
});
@@ -184,14 +197,50 @@ describe("recoveryFor", () => {
184197
failureClass: "provider_retryable",
185198
attempt: 1,
186199
followupTurn: true,
200+
replaySafe: true,
187201
}),
188202
).toEqual({ available: false, reason: "not_retryable" });
189203
});
190204

205+
test("a retryable failure after a non-replay-safe call has side effects", () => {
206+
expect(
207+
recoveryFor({
208+
failureClass: "provider_retryable",
209+
attempt: 1,
210+
followupTurn: false,
211+
replaySafe: false,
212+
}),
213+
).toEqual({ available: false, reason: "side_effects_completed" });
214+
});
215+
216+
test("only read, search, and web read tools are replay-safe", () => {
217+
for (const name of ["read_file", "read", "grep", "search_files"]) {
218+
expect(isReplaySafeToolName(name)).toBe(true);
219+
}
220+
for (const name of ["web_fetch", "web_search"]) {
221+
expect(isReplaySafeToolName(name)).toBe(true);
222+
}
223+
for (const name of [
224+
"write_file",
225+
"edit_file",
226+
"run_shell",
227+
"bash",
228+
"mcp__server__tool",
229+
"some_unknown_tool",
230+
]) {
231+
expect(isReplaySafeToolName(name)).toBe(false);
232+
}
233+
});
234+
191235
test("a failed recovery attempt is exhausted whatever its class", () => {
192236
for (const failureClass of ["provider_retryable", "error"] as const) {
193237
expect(
194-
recoveryFor({ failureClass, attempt: 2, followupTurn: false }),
238+
recoveryFor({
239+
failureClass,
240+
attempt: 2,
241+
followupTurn: false,
242+
replaySafe: true,
243+
}),
195244
).toEqual({ available: false, reason: "recovery_exhausted" });
196245
}
197246
});

‎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
/**
@@ -125,22 +127,33 @@ export function classifySubAgentFailure(
125127
return "error";
126128
}
127129

130+
/** Whether a call to `name` can be replayed by a recovery attempt. */
131+
export function isReplaySafeToolName(name: string): boolean {
132+
return REPLAY_SAFE_TOOLS.has(canonicalToolName(name));
133+
}
134+
128135
/**
129136
* Recovery availability for a freshly written record. The chain cap wins
130137
* over class: a recovery attempt that fails is exhausted whatever its cause.
138+
* Replay safety fails closed: any tool outside the read-only allowlist means
139+
* a replacement could repeat a side effect.
131140
*/
132141
export function recoveryFor(input: {
133142
failureClass: SubAgentFailureClass;
134143
attempt: number;
135144
followupTurn: boolean;
136145
cause?: unknown;
146+
replaySafe: boolean;
137147
}): SubAgentRecovery {
138148
if (input.attempt > 1) {
139149
return { available: false, reason: "recovery_exhausted" };
140150
}
141151
if (input.followupTurn || !isRetryableFailure(input)) {
142152
return { available: false, reason: "not_retryable" };
143153
}
154+
if (!input.replaySafe) {
155+
return { available: false, reason: "side_effects_completed" };
156+
}
144157
return { available: true, reason: "eligible" };
145158
}
146159

0 commit comments

Comments
 (0)