From ccd8ede95ee3c0b9ba0db18ea9ecc67d1574a055 Mon Sep 17 00:00:00 2001 From: felirami Date: Tue, 25 Aug 2026 04:55:41 -0400 Subject: [PATCH] fix(subagents): preserve announce drop vs none delivery reasons (#122855) * fix(subagents): preserve announce drop vs none delivery reasons Keep live-queue refusals as steer_dropped through dispatch mapping and registry persistence so they stay distinguishable from no viable requester (sink_unavailable). Do not mark dropped as terminal, so completion fallback still returns the primary direct result. * fix(subagents): persist steer_dropped after completion fallback drop Carry the nonterminal dropped-steer reason on the retained failed direct completion result so registry cleanup can persist lastDropReason. * fix(subagents): keep direct announce reason when steer fallback drops Completion fallback no longer overwrites the direct failure classification with steer_dropped. Cleanup still persists steer_dropped from the fallback phase so queue refusal stays distinct from sink_unavailable. * test(subagents): keep visible_reply_missing through dropped steer fallback Use the no-output Discord completion path so deliverSubagentAnnouncement keeps the direct classification while the fallback phase records steer_dropped. * fix(agents): persist subagent drop diagnostics atomically Co-authored-by: felirami <6752178+felirami@users.noreply.github.com> --------- Co-authored-by: Cursor Agent Co-authored-by: Peter Steinberger Co-authored-by: felirami <6752178+felirami@users.noreply.github.com> --- .../subagent-announce-delivery.test.ts | 53 ++++++- .../subagent-announce-dispatch.test.ts | 103 +++++++++++--- .../announce/subagent-announce-dispatch.ts | 3 + .../subagent-announce.format.e2e.test.ts | 10 +- ...ent-registry-lifecycle-announce-cleanup.ts | 12 +- .../subagent-registry-lifecycle-delivery.ts | 10 ++ .../subagent-registry-lifecycle.test.ts | 132 +++++++++++++++++- .../registry/subagent-registry.types.ts | 1 + 8 files changed, 295 insertions(+), 29 deletions(-) diff --git a/src/agents/subagents/announce/subagent-announce-delivery.test.ts b/src/agents/subagents/announce/subagent-announce-delivery.test.ts index 089f7d4761d3..3df21e6a2c30 100644 --- a/src/agents/subagents/announce/subagent-announce-delivery.test.ts +++ b/src/agents/subagents/announce/subagent-announce-delivery.test.ts @@ -1340,8 +1340,15 @@ describe("deliverSubagentAnnouncement active requester steering", () => { expectRecordFields(result, { delivered: fallsBack, path: fallsBack ? "direct" : "none", + ...(fallsBack ? {} : { reason: "steer_dropped" }), phases: [ - { phase: "steer-primary", delivered: false, path: "none", error: undefined }, + { + phase: "steer-primary", + delivered: false, + path: "none", + error: undefined, + ...(fallsBack ? {} : { reason: "steer_dropped" }), + }, ...(fallsBack ? [{ phase: "direct-primary", delivered: true, path: "direct", error: undefined }] : []), @@ -2581,6 +2588,50 @@ describe("deliverSubagentAnnouncement completion delivery", () => { expect(sendMessage).not.toHaveBeenCalled(); }); + it("preserves visible_reply_missing when completion direct delivery fails and fallback steering drops", async () => { + const callGateway = createPayloadGatewayMock(); + const sendMessage = createSendMessageMock(); + const queueEmbeddedAgentMessageWithOutcome = createQueueOutcomeMock(false); + const result = await deliverDiscordDirectMessageCompletion({ + callGateway, + sendMessage, + isActive: true, + queueEmbeddedAgentMessageWithOutcome, + internalEvents: taskCompletionEvents({ + childSessionId: "child-session-id", + status: "error", + statusLabel: "failed: all models failed", + result: "(no output)", + }), + }); + + expectRecordFields(result, { + delivered: false, + path: "direct", + error: "completion agent did not produce a visible reply", + reason: "visible_reply_missing", + phases: [ + { + phase: "direct-primary", + delivered: false, + path: "direct", + reason: "visible_reply_missing", + error: "completion agent did not produce a visible reply", + }, + { + phase: "steer-fallback", + delivered: false, + path: "none", + reason: "steer_dropped", + error: undefined, + }, + ], + }); + expect(result.terminal).toBeUndefined(); + expect(queueEmbeddedAgentMessageWithOutcome).toHaveBeenCalled(); + expect(sendMessage).not.toHaveBeenCalled(); + }); + it("reports failure for Telegram DMs when announce-agent delivery fails", async () => { const callGateway = createGatewayMock({ result: { diff --git a/src/agents/subagents/announce/subagent-announce-dispatch.test.ts b/src/agents/subagents/announce/subagent-announce-dispatch.test.ts index 3e95bef01a18..8db23fae33e3 100644 --- a/src/agents/subagents/announce/subagent-announce-dispatch.test.ts +++ b/src/agents/subagents/announce/subagent-announce-dispatch.test.ts @@ -28,6 +28,7 @@ describe("runSubagentAnnounceDispatch", () => { expect(direct).toHaveBeenCalledTimes(1); expect(result.delivered).toBe(true); expect(result.path).toBe("direct"); + expect(result.reason).toBeUndefined(); expect(result.phases).toEqual([ { phase: "steer-primary", delivered: false, path: "none", error: undefined }, { phase: "direct-primary", delivered: true, path: "direct", error: undefined }, @@ -170,28 +171,79 @@ describe("runSubagentAnnounceDispatch", () => { ]); }); - it("returns direct failure when completion fallback steering cannot deliver", async () => { - const steer = vi.fn(async () => ({ status: "none" }) as const); - const direct = vi.fn(async () => ({ - delivered: false, - path: "direct" as const, - error: "failed", - })); + it.each([ + { + name: "cannot deliver", + steerStatus: "none" as const, + directReason: undefined, + expectedReason: undefined, + fallbackPhase: { + phase: "steer-fallback" as const, + delivered: false, + path: "none" as const, + error: undefined, + }, + }, + { + name: "drops the new item", + steerStatus: "dropped" as const, + directReason: undefined, + expectedReason: undefined, + fallbackPhase: { + phase: "steer-fallback" as const, + delivered: false, + path: "none" as const, + reason: "steer_dropped" as const, + error: undefined, + }, + }, + { + name: "drops the new item after a visible reply is missing", + steerStatus: "dropped" as const, + directReason: "visible_reply_missing" as const, + expectedReason: "visible_reply_missing" as const, + fallbackPhase: { + phase: "steer-fallback" as const, + delivered: false, + path: "none" as const, + reason: "steer_dropped" as const, + error: undefined, + }, + }, + ])( + "returns direct failure when completion fallback steering $name", + async ({ steerStatus, directReason, expectedReason, fallbackPhase }) => { + const steer = vi.fn(async () => ({ status: steerStatus })); + const direct = vi.fn(async () => ({ + delivered: false, + path: "direct" as const, + error: "failed", + ...(directReason ? { reason: directReason } : {}), + })); - const result = await runSubagentAnnounceDispatch({ - expectsCompletionMessage: true, - steer, - direct, - }); + const result = await runSubagentAnnounceDispatch({ + expectsCompletionMessage: true, + steer, + direct, + }); - expect(result.delivered).toBe(false); - expect(result.path).toBe("direct"); - expect(result.error).toBe("failed"); - expect(result.phases).toEqual([ - { phase: "direct-primary", delivered: false, path: "direct", error: "failed" }, - { phase: "steer-fallback", delivered: false, path: "none", error: undefined }, - ]); - }); + expect(result.delivered).toBe(false); + expect(result.path).toBe("direct"); + expect(result.error).toBe("failed"); + expect(result.reason).toBe(expectedReason); + expect(result.terminal).toBeUndefined(); + expect(result.phases).toEqual([ + { + phase: "direct-primary", + delivered: false, + path: "direct", + error: "failed", + ...(directReason ? { reason: directReason } : {}), + }, + fallbackPhase, + ]); + }, + ); it("returns terminal source ownership loss from completion fallback steering", async () => { const steer = vi.fn(async () => ({ status: "source_owner_changed" }) as const); @@ -230,7 +282,16 @@ describe("runSubagentAnnounceDispatch", () => { expect(result).toEqual({ delivered: false, path: "none", - phases: [{ phase: "steer-primary", delivered: false, path: "none", error: undefined }], + reason: "steer_dropped", + phases: [ + { + phase: "steer-primary", + delivered: false, + path: "none", + reason: "steer_dropped", + error: undefined, + }, + ], }); }); diff --git a/src/agents/subagents/announce/subagent-announce-dispatch.ts b/src/agents/subagents/announce/subagent-announce-dispatch.ts index 20137f4f867f..197f35cfca5d 100644 --- a/src/agents/subagents/announce/subagent-announce-dispatch.ts +++ b/src/agents/subagents/announce/subagent-announce-dispatch.ts @@ -20,6 +20,7 @@ type SubagentAnnounceDeliveryFailureReason = | "message_tool_delivery_missing" | "requester_abandoned" | "source_owner_changed" + | "steer_dropped" | "visible_reply_missing"; type SubagentAnnounceSteerOutcome = @@ -81,6 +82,7 @@ function mapSteerOutcomeToDeliveryResult( return { delivered: false, path: "none", + ...(outcome.status === "dropped" ? { reason: "steer_dropped" } : {}), }; } @@ -174,5 +176,6 @@ export async function runSubagentAnnounceDispatch(params: { return withPhases(fallbackSteer); } + // Keep the direct failure authoritative; dropped fallback remains in its phase. return withPhases(primaryDirect); } diff --git a/src/agents/subagents/announce/subagent-announce.format.e2e.test.ts b/src/agents/subagents/announce/subagent-announce.format.e2e.test.ts index 3f02b5533fe9..4f6c5e369787 100644 --- a/src/agents/subagents/announce/subagent-announce.format.e2e.test.ts +++ b/src/agents/subagents/announce/subagent-announce.format.e2e.test.ts @@ -2007,8 +2007,16 @@ describe("subagent announce formatting", () => { }); expect(delivery.delivered).toBe(false); + expect(delivery.reason).toBe("steer_dropped"); + expect(delivery.terminal).toBeUndefined(); expect(delivery.phases).toEqual([ - { phase: "steer-primary", delivered: false, path: "none", error: undefined }, + { + phase: "steer-primary", + delivered: false, + path: "none", + reason: "steer_dropped", + error: undefined, + }, ]); expect(direct).not.toHaveBeenCalled(); }); diff --git a/src/agents/subagents/registry/subagent-registry-lifecycle-announce-cleanup.ts b/src/agents/subagents/registry/subagent-registry-lifecycle-announce-cleanup.ts index d2212903e670..4c37c8368856 100644 --- a/src/agents/subagents/registry/subagent-registry-lifecycle-announce-cleanup.ts +++ b/src/agents/subagents/registry/subagent-registry-lifecycle-announce-cleanup.ts @@ -576,6 +576,7 @@ export const startSubagentAnnounceCleanupFlow = ( } : undefined, onDeliveryResult: (delivery) => { + const previousDropReason = entry.delivery?.lastDropReason; if (!context.isCleanupAttemptCurrent(runId, entry, cleanupGeneration)) { retireSupersededCleanupInBackground(context, runId, entry, cleanupGeneration); return; @@ -598,12 +599,13 @@ export const startSubagentAnnounceCleanupFlow = ( latestDeliveryError = undefined; return; } - if (delivery.path === "none" && delivery.disposition !== "intentional_non_delivery") { - ensureDeliveryState(entry).lastDropReason = "sink_unavailable"; - } + const deliveryState = ensureDeliveryState(entry); latestDeliveryError = formatAnnounceDeliveryError(delivery); - if (ensureDeliveryState(entry).lastError !== latestDeliveryError) { - ensureDeliveryState(entry).lastError = latestDeliveryError; + if ( + deliveryState.lastError !== latestDeliveryError || + deliveryState.lastDropReason !== previousDropReason + ) { + deliveryState.lastError = latestDeliveryError; params.persist(runId); } }, diff --git a/src/agents/subagents/registry/subagent-registry-lifecycle-delivery.ts b/src/agents/subagents/registry/subagent-registry-lifecycle-delivery.ts index 14d895911de0..08c5ee68d9b0 100644 --- a/src/agents/subagents/registry/subagent-registry-lifecycle-delivery.ts +++ b/src/agents/subagents/registry/subagent-registry-lifecycle-delivery.ts @@ -85,6 +85,16 @@ export const recordAnnounceDeliveryResult = ( if (typeof delivery.enqueuedAt === "number") { deliveryState.enqueuedAt ??= delivery.enqueuedAt; } + if (!delivery.delivered && delivery.disposition !== "intentional_non_delivery") { + if ( + delivery.reason === "steer_dropped" || + delivery.phases?.some((phase) => phase.reason === "steer_dropped") + ) { + deliveryState.lastDropReason = "steer_dropped"; + } else if (delivery.path === "none") { + deliveryState.lastDropReason = "sink_unavailable"; + } + } if (delivery.delivered) { const deliveredAt = typeof delivery.deliveredAt === "number" ? delivery.deliveredAt : Date.now(); diff --git a/src/agents/subagents/registry/subagent-registry-lifecycle.test.ts b/src/agents/subagents/registry/subagent-registry-lifecycle.test.ts index 00ca8a2845a3..1ed8f3f8aba3 100644 --- a/src/agents/subagents/registry/subagent-registry-lifecycle.test.ts +++ b/src/agents/subagents/registry/subagent-registry-lifecycle.test.ts @@ -21,7 +21,10 @@ import { buildAnnounceIdempotencyKey, } from "../../announce-idempotency.js"; import { createStructuredOutputTool } from "../../tools/structured-output-tool.js"; -import type { SubagentAnnounceDeliveryResult } from "../announce/subagent-announce-dispatch.js"; +import { + runSubagentAnnounceDispatch, + type SubagentAnnounceDeliveryResult, +} from "../announce/subagent-announce-dispatch.js"; import { SUBAGENT_ENDED_REASON_COMPLETE, SUBAGENT_ENDED_REASON_ERROR, @@ -2851,6 +2854,133 @@ describe("subagent registry lifecycle hardening", () => { }); }); + it.each([ + { + name: "persists steer_dropped when announce mapping preserves a live-queue refusal", + delivery: { + delivered: false as const, + path: "none" as const, + reason: "steer_dropped" as const, + }, + lastDropReason: "steer_dropped", + lastError: "steer_dropped", + }, + { + name: "persists sink_unavailable when announce mapping reports no viable requester", + delivery: { + delivered: false as const, + path: "none" as const, + }, + lastDropReason: "sink_unavailable", + lastError: "delivery path none did not complete", + }, + ])("$name", async ({ delivery, lastDropReason, lastError }) => { + const persist = vi.fn(); + const entry = createRunEntry({ + endedAt: 4_000, + expectsCompletionMessage: true, + retainAttachmentsOnKeep: true, + }); + const runSubagentAnnounceFlow: LifecycleControllerParams["runSubagentAnnounceFlow"] = vi.fn( + async (announceParams) => { + announceParams.onDeliveryResult?.(delivery); + return "retryable" as const; + }, + ); + + const controller = createLifecycleController({ entry, persist, runSubagentAnnounceFlow }); + + await expect( + completeRun(controller, entry, { + triggerCleanup: true, + terminalReply: { disposition: "visible", text: "final completion reply" }, + }), + ).resolves.toBeUndefined(); + + await waitForLifecycleState(() => expect(entry.delivery?.lastDropReason).toBe(lastDropReason)); + expect(entry.delivery?.lastError).toBe(lastError); + expect(entry.delivery?.status).toBe("suspended"); + expect(persist).toHaveBeenCalledWith(entry.runId); + }); + + it.each([ + { + name: "persists a newly failed completion", + previousDropReason: undefined, + reusePreviousError: false, + persistCalls: 1, + }, + { + name: "persists a changed drop reason when the direct error is unchanged", + previousDropReason: "sink_unavailable" as const, + reusePreviousError: true, + persistCalls: 1, + }, + { + name: "does not persist unchanged completion diagnostics", + previousDropReason: "steer_dropped" as const, + reusePreviousError: true, + persistCalls: 0, + }, + ])("$name before stalled announce bookkeeping settles", async (scenario) => { + const lastError = "failed; visible_reply_missing; direct-primary: failed"; + const persist = vi.fn(); + const entry = createRunEntry({ + endedAt: 4_000, + expectsCompletionMessage: true, + retainAttachmentsOnKeep: true, + delivery: { + status: "pending", + ...(scenario.reusePreviousError ? { lastError } : {}), + ...(scenario.previousDropReason ? { lastDropReason: scenario.previousDropReason } : {}), + }, + }); + let releaseAnnounce!: () => void; + const announcePending = new Promise((resolve) => { + releaseAnnounce = resolve; + }); + const runSubagentAnnounceFlow: LifecycleControllerParams["runSubagentAnnounceFlow"] = vi.fn( + async (announceParams) => { + const delivery = await runSubagentAnnounceDispatch({ + expectsCompletionMessage: true, + steer: async () => ({ status: "dropped" }), + direct: async () => ({ + delivered: false, + path: "direct", + error: "failed", + reason: "visible_reply_missing", + }), + }); + persist.mockClear(); + announceParams.onDeliveryResult?.(delivery); + await announcePending; + return "retryable" as const; + }, + ); + const controller = createLifecycleController({ entry, persist, runSubagentAnnounceFlow }); + + try { + await expect( + completeRun(controller, entry, { + triggerCleanup: true, + terminalReply: { disposition: "visible", text: "final completion reply" }, + }), + ).resolves.toBeUndefined(); + await waitForLifecycleState(() => expect(entry.delivery?.disposition).toBe("retryable")); + expect(entry.delivery?.lastDropReason).toBe("steer_dropped"); + expect(entry.delivery?.lastError).toBe(lastError); + expect(entry.cleanupCompletedAt).toBeUndefined(); + expect(persist).toHaveBeenCalledTimes(scenario.persistCalls); + if (scenario.persistCalls > 0) { + expect(persist).toHaveBeenCalledWith(entry.runId); + } + } finally { + releaseAnnounce(); + } + + await waitForLifecycleState(() => expect(entry.delivery?.status).toBe("suspended")); + }); + it("persists identified completion delivery before stalled announce bookkeeping settles", async () => { const persist = vi.fn(); const entry = createRunEntry({ diff --git a/src/agents/subagents/registry/subagent-registry.types.ts b/src/agents/subagents/registry/subagent-registry.types.ts index 90a1954557eb..7fe4429203d2 100644 --- a/src/agents/subagents/registry/subagent-registry.types.ts +++ b/src/agents/subagents/registry/subagent-registry.types.ts @@ -172,6 +172,7 @@ export type SubagentCompletionDeliveryState = { | "queue_cap" | "parent_run_ended" | "sink_unavailable" + | "steer_dropped" | "dedupe" | "waiting_for_requester_turn"; };