Skip to content

Commit 44cb3f7

Browse files
fsdwenfsdwen
authored
fix(agents): prevent duplicate subagent announce sends
Fixes the subagent announce prompt-lock race by treating lock-change failures with visible-send evidence as terminal, while keeping pre-send failures retryable. Scoped proof: - node scripts/run-vitest.mjs src/agents/subagent-announce-delivery.test.ts src/agents/subagent-announce.test.ts src/agents/subagent-registry-lifecycle.test.ts - node scripts/run-tsgo.mjs -p tsconfig.core.json - node scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.core.test.agents.json - oxfmt --check on touched agent files - git diff --check - .agents/skills/autoreview/scripts/autoreview --mode local Thanks @fsdwen!
1 parent ab2f6f5 commit 44cb3f7

5 files changed

Lines changed: 336 additions & 17 deletions

‎src/agents/subagent-announce-delivery.test.ts‎

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4968,4 +4968,158 @@ describe("deliverSubagentAnnouncement completion delivery", () => {
49684968
bestEffortDeliver: true,
49694969
});
49704970
});
4971+
4972+
it("does not retry session-file-changed failures with send evidence", async () => {
4973+
const sendErr = new OutboundDeliveryError("outbound delivery failed", {
4974+
cause: new Error("outbound delivery failed"),
4975+
results: [{ channel: "telegram", messageId: "msg-1" }],
4976+
});
4977+
const callGateway: typeof runtimeCallGateway = vi.fn(async () => {
4978+
throw new Error("session file changed while embedded prompt lock was released", {
4979+
cause: sendErr,
4980+
});
4981+
});
4982+
const queueEmbeddedAgentMessageWithOutcome = createQueueOutcomeSequenceMock(["no_active_run"]);
4983+
const result = await deliverSlackChannelAnnouncement({
4984+
callGateway,
4985+
queueEmbeddedAgentMessageWithOutcome,
4986+
sessionId: "requester-session-lock-race-evidence",
4987+
isActive: true,
4988+
expectsCompletionMessage: true,
4989+
directIdempotencyKey: "announce-permanent-lock-error-evidence",
4990+
});
4991+
4992+
expect(result.delivered).toBe(false);
4993+
expect(result.path).toBe("direct");
4994+
expect(result.terminal).toBe(true);
4995+
expect(result.phases?.map((phase) => phase.phase)).toEqual(["direct-primary"]);
4996+
expect(callGateway).toHaveBeenCalledTimes(1);
4997+
expect(queueEmbeddedAgentMessageWithOutcome).toHaveBeenCalledTimes(1);
4998+
});
4999+
5000+
it("does not fallback-steer after wrapped prompt-lock takeover with send evidence", async () => {
5001+
const takeoverErr = Object.assign(
5002+
new Error("session file changed while embedded prompt lock was released: /tmp/session.jsonl"),
5003+
{ name: "EmbeddedAttemptSessionTakeoverError" },
5004+
);
5005+
5006+
const promptErr = Object.assign(new Error("some model error"), { visibleReplySent: true });
5007+
const wrapperErr = Object.assign(new Error("some model error", { cause: takeoverErr }), {
5008+
name: "EmbeddedAttemptSessionTakeoverError",
5009+
cleanupError: takeoverErr,
5010+
promptError: promptErr,
5011+
});
5012+
5013+
const callGateway: typeof runtimeCallGateway = vi.fn(async () => {
5014+
throw wrapperErr;
5015+
});
5016+
const queueEmbeddedAgentMessageWithOutcome = createQueueOutcomeSequenceMock(["no_active_run"]);
5017+
const result = await deliverSlackChannelAnnouncement({
5018+
callGateway,
5019+
queueEmbeddedAgentMessageWithOutcome,
5020+
sessionId: "requester-session-lock-race-wrapped-evidence",
5021+
isActive: true,
5022+
expectsCompletionMessage: true,
5023+
directIdempotencyKey: "announce-permanent-wrapped-lock-error-evidence",
5024+
});
5025+
5026+
expect(result.delivered).toBe(false);
5027+
expect(result.path).toBe("direct");
5028+
expect(result.error).toBe("some model error");
5029+
expect(result.terminal).toBe(true);
5030+
expect(result.phases?.map((phase) => phase.phase)).toEqual(["direct-primary"]);
5031+
expect(callGateway).toHaveBeenCalledTimes(1);
5032+
expect(queueEmbeddedAgentMessageWithOutcome).toHaveBeenCalledTimes(1);
5033+
});
5034+
5035+
it("retries session-file-changed failures without send evidence", async () => {
5036+
let attempts = 0;
5037+
const callGatewaySpy = vi.fn();
5038+
const callGateway: typeof runtimeCallGateway = async <
5039+
T = Record<string, unknown>,
5040+
>(): Promise<T> => {
5041+
callGatewaySpy();
5042+
attempts++;
5043+
if (attempts <= 1) {
5044+
throw new Error("session file changed while embedded prompt lock was released");
5045+
}
5046+
return {
5047+
result: {
5048+
payloads: [{ text: "recovered after retry" }],
5049+
},
5050+
} as T;
5051+
};
5052+
const queueEmbeddedAgentMessageWithOutcome = createQueueOutcomeSequenceMock(["no_active_run"]);
5053+
const result = await deliverSlackChannelAnnouncement({
5054+
callGateway,
5055+
queueEmbeddedAgentMessageWithOutcome,
5056+
sessionId: "requester-session-lock-race-no-evidence",
5057+
isActive: true,
5058+
expectsCompletionMessage: true,
5059+
directIdempotencyKey: "announce-retry-lock-error-no-evidence",
5060+
});
5061+
5062+
expect(result.delivered).toBe(true);
5063+
expect(result.path).toBe("direct");
5064+
expect(callGatewaySpy).toHaveBeenCalledTimes(2);
5065+
});
5066+
5067+
it("detects send evidence from OutboundDeliveryError in the error chain", () => {
5068+
const err = new Error(
5069+
"session file changed while embedded prompt lock was released: /tmp/session.jsonl",
5070+
{
5071+
cause: new OutboundDeliveryError("outbound delivery failed", {
5072+
cause: new Error("outbound delivery failed"),
5073+
results: [{ channel: "telegram", messageId: "msg-1" }],
5074+
}),
5075+
},
5076+
);
5077+
5078+
expect(testing.isSessionFileChangedAnnounceError(err.message)).toBe(true);
5079+
expect(testing.hasAnnounceSendEvidence(err)).toBe(true);
5080+
});
5081+
5082+
it("classifies session-file-changed error as no-send-evidence when the error chain has no send markers", () => {
5083+
const err = new Error(
5084+
"session file changed while embedded prompt lock was released: /tmp/session.jsonl",
5085+
);
5086+
5087+
expect(testing.isSessionFileChangedAnnounceError(err.message)).toBe(true);
5088+
expect(testing.hasAnnounceSendEvidence(err)).toBe(false);
5089+
});
5090+
5091+
it("detects send evidence from visibleReplySent flag on session-file-changed error", () => {
5092+
const err = Object.assign(
5093+
new Error("session file changed while embedded prompt lock was released: /tmp/session.jsonl"),
5094+
{ visibleReplySent: true },
5095+
);
5096+
5097+
expect(testing.hasAnnounceSendEvidence(err)).toBe(true);
5098+
});
5099+
5100+
it("detects send evidence from sentBeforeError flag on session-file-changed error", () => {
5101+
const err = Object.assign(
5102+
new Error("session file changed while embedded prompt lock was released: /tmp/session.jsonl"),
5103+
{ sentBeforeError: true },
5104+
);
5105+
5106+
expect(testing.hasAnnounceSendEvidence(err)).toBe(true);
5107+
});
5108+
5109+
it("detects send evidence recursively through promptError", () => {
5110+
const takeoverErr = Object.assign(
5111+
new Error("session file changed while embedded prompt lock was released: /tmp/session.jsonl"),
5112+
{ name: "EmbeddedAttemptSessionTakeoverError" },
5113+
);
5114+
5115+
const promptErr = Object.assign(new Error("some model error"), { visibleReplySent: true });
5116+
5117+
const wrapperErr = Object.assign(new Error("some model error", { cause: takeoverErr }), {
5118+
name: "EmbeddedAttemptSessionTakeoverError",
5119+
promptError: promptErr,
5120+
});
5121+
5122+
expect(testing.hasAnnounceSendEvidence(wrapperErr)).toBe(true);
5123+
expect(testing.hasSessionFileChangedAnnounceError(wrapperErr)).toBe(true);
5124+
});
49715125
});

‎src/agents/subagent-announce-delivery.ts‎

Lines changed: 82 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -380,6 +380,9 @@ const TRANSIENT_ANNOUNCE_DELIVERY_ERROR_PATTERNS: readonly RegExp[] = [
380380
/\b(econnreset|econnrefused|etimedout|enotfound|ehostunreach|network error)\b/i,
381381
];
382382

383+
const SESSION_FILE_CHANGED_ANNOUNCE_RE =
384+
/session file changed while embedded prompt lock was released/i;
385+
383386
const PERMANENT_ANNOUNCE_DELIVERY_ERROR_PATTERNS: readonly RegExp[] = [
384387
/unsupported channel/i,
385388
/unknown channel/i,
@@ -390,23 +393,83 @@ const PERMANENT_ANNOUNCE_DELIVERY_ERROR_PATTERNS: readonly RegExp[] = [
390393
/forbidden: bot was kicked/i,
391394
/recipient is not a valid/i,
392395
/outbound not configured for channel/i,
396+
SESSION_FILE_CHANGED_ANNOUNCE_RE,
393397
];
394398

399+
function isSessionFileChangedAnnounceError(message: string): boolean {
400+
return SESSION_FILE_CHANGED_ANNOUNCE_RE.test(message);
401+
}
402+
403+
const ANNOUNCE_ERROR_CHAIN_KEYS = [
404+
"cause",
405+
"cleanupError",
406+
"error",
407+
"promptError",
408+
"reason",
409+
] as const;
410+
type AnnounceErrorChainKey = (typeof ANNOUNCE_ERROR_CHAIN_KEYS)[number];
411+
type AnnounceErrorRecord = Partial<Record<AnnounceErrorChainKey, unknown>> & {
412+
sentBeforeError?: unknown;
413+
visibleReplySent?: unknown;
414+
};
415+
416+
function isAnnounceErrorRecord(error: unknown): error is AnnounceErrorRecord {
417+
return Boolean(error && typeof error === "object");
418+
}
419+
420+
function hasAnnounceErrorMatch(
421+
error: unknown,
422+
matches: (candidate: unknown) => boolean,
423+
seen: Set<object> = new Set(),
424+
): boolean {
425+
if (matches(error)) {
426+
return true;
427+
}
428+
if (!isAnnounceErrorRecord(error)) {
429+
return false;
430+
}
431+
if (seen.has(error)) {
432+
return false;
433+
}
434+
seen.add(error);
435+
436+
return ANNOUNCE_ERROR_CHAIN_KEYS.some((key) => hasAnnounceErrorMatch(error[key], matches, seen));
437+
}
438+
439+
function hasSessionFileChangedAnnounceError(error: unknown): boolean {
440+
return hasAnnounceErrorMatch(error, (candidate) =>
441+
isSessionFileChangedAnnounceError(summarizeDeliveryError(candidate)),
442+
);
443+
}
444+
395445
function isTransientAnnounceDeliveryError(error: unknown): boolean {
396446
const message = summarizeDeliveryError(error);
447+
const topLevelPermanent = Boolean(
448+
message && PERMANENT_ANNOUNCE_DELIVERY_ERROR_PATTERNS.some((re) => re.test(message)),
449+
);
450+
if (topLevelPermanent && !isSessionFileChangedAnnounceError(message)) {
451+
return false;
452+
}
453+
454+
const sessionFileChanged = hasSessionFileChangedAnnounceError(error);
455+
if (sessionFileChanged) {
456+
return !hasAnnounceSendEvidence(error);
457+
}
458+
397459
if (!message) {
398460
return false;
399461
}
400-
if (PERMANENT_ANNOUNCE_DELIVERY_ERROR_PATTERNS.some((re) => re.test(message))) {
462+
if (topLevelPermanent) {
401463
return false;
402464
}
403465
return TRANSIENT_ANNOUNCE_DELIVERY_ERROR_PATTERNS.some((re) => re.test(message));
404466
}
405467

406468
function isPermanentAnnounceDeliveryError(error: unknown): boolean {
407469
const message = summarizeDeliveryError(error);
408-
return Boolean(
409-
message && PERMANENT_ANNOUNCE_DELIVERY_ERROR_PATTERNS.some((re) => re.test(message)),
470+
return (
471+
(message && PERMANENT_ANNOUNCE_DELIVERY_ERROR_PATTERNS.some((re) => re.test(message))) ||
472+
hasSessionFileChangedAnnounceError(error)
410473
);
411474
}
412475

@@ -426,17 +489,18 @@ function isSessionWriteLockAnnounceAgentError(error: unknown): boolean {
426489
);
427490
}
428491

429-
function didVisibleSendFailAfterPartialDelivery(error: unknown): boolean {
492+
function hasDirectAnnounceSendEvidence(error: unknown): boolean {
430493
if (isOutboundDeliveryError(error) && error.sentBeforeError) {
431494
return true;
432495
}
433-
const maybeDeliveryError = error as {
434-
sentBeforeError?: unknown;
435-
visibleReplySent?: unknown;
436-
};
437-
return (
438-
maybeDeliveryError.sentBeforeError === true || maybeDeliveryError.visibleReplySent === true
439-
);
496+
if (!isAnnounceErrorRecord(error)) {
497+
return false;
498+
}
499+
return error.sentBeforeError === true || error.visibleReplySent === true;
500+
}
501+
502+
function hasAnnounceSendEvidence(error: unknown): boolean {
503+
return hasAnnounceErrorMatch(error, hasDirectAnnounceSendEvidence);
440504
}
441505

442506
async function waitForAnnounceRetryDelay(ms: number, signal?: AbortSignal): Promise<void> {
@@ -891,7 +955,7 @@ async function deliverGeneratedMediaCompletionDirect(params: {
891955
path: "direct",
892956
};
893957
} catch (err) {
894-
const terminal = didVisibleSendFailAfterPartialDelivery(err);
958+
const terminal = hasAnnounceSendEvidence(err);
895959
return {
896960
delivered: false,
897961
path: "direct",
@@ -1508,7 +1572,7 @@ async function sendSubagentAnnounceDirectly(params: {
15081572
}),
15091573
});
15101574
} catch (err) {
1511-
if (isPermanentAnnounceDeliveryError(err)) {
1575+
if (isPermanentAnnounceDeliveryError(err) && hasAnnounceSendEvidence(err)) {
15121576
throw err;
15131577
}
15141578
if (
@@ -1695,10 +1759,12 @@ async function sendSubagentAnnounceDirectly(params: {
16951759
path: "direct",
16961760
};
16971761
} catch (err) {
1762+
const terminal = isPermanentAnnounceDeliveryError(err) && hasAnnounceSendEvidence(err);
16981763
return {
16991764
delivered: false,
17001765
path: "direct",
17011766
error: summarizeDeliveryError(err),
1767+
...(terminal ? { terminal: true } : {}),
17021768
};
17031769
}
17041770
}
@@ -1785,5 +1851,8 @@ export const testing = {
17851851
}
17861852
: defaultSubagentAnnounceDeliveryDeps;
17871853
},
1854+
hasAnnounceSendEvidence,
1855+
hasSessionFileChangedAnnounceError,
1856+
isSessionFileChangedAnnounceError,
17881857
};
17891858
export { testing as __testing };

0 commit comments

Comments
 (0)