diff --git a/CHANGELOG.md b/CHANGELOG.md index 6632c7ca19b..180ec43f0ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,7 @@ Docs: https://docs.openclaw.ai - Providers/MiniMax: derive Coding Plan usage polling from the configured MiniMax base URL, so global setups no longer query the CN usage host. Fixes #65054. Thanks @sixone74 and @Yanhu007. - Sessions: reject `sessions_send` targets that resolve to thread-scoped chat sessions, so inter-agent coordination cannot be injected into active human-facing Slack or Discord threads. Fixes #52496. Thanks @barry-p5cc. - Subagents: honor `sessions_spawn` with `expectsCompletionMessage: false` by skipping parent completion handoff delivery while still running child cleanup. Fixes #75848. Thanks @alfredjbclaw. +- Media/completions: treat media-only message-tool sends as delivered async completion output, avoiding duplicate raw `MEDIA:` fallback posts after video or music generation finishes. - Gateway/logging: keep deferred channel startup logs on the subsystem logger, so Slack, Discord, Telegram, and voice-call startup messages keep timestamped prefixes. Thanks @vincentkoc. - Replies/typing: keep typing alive for queued follow-up messages that are genuinely waiting behind an active run, instead of making chat surfaces look idle while work is queued. Fixes #65685. Thanks @papag00se. - ACP/Discord: suppress completion announce delivery for inline thread-bound ACP session runs, so Discord thread-bound ACP replies are not delivered twice. Fixes #60780. Thanks @solavrc. diff --git a/src/agents/pi-embedded-runner.run-embedded-pi-agent.auth-profile-rotation.e2e.test.ts b/src/agents/pi-embedded-runner.run-embedded-pi-agent.auth-profile-rotation.e2e.test.ts index f53a0da15d6..7cd811b0ff6 100644 --- a/src/agents/pi-embedded-runner.run-embedded-pi-agent.auth-profile-rotation.e2e.test.ts +++ b/src/agents/pi-embedded-runner.run-embedded-pi-agent.auth-profile-rotation.e2e.test.ts @@ -160,6 +160,7 @@ const makeAttempt = (overrides: Partial): EmbeddedRunA const didSendViaMessagingTool = overrides.didSendViaMessagingTool ?? false; const messagingToolSentTexts = overrides.messagingToolSentTexts ?? []; const messagingToolSentMediaUrls = overrides.messagingToolSentMediaUrls ?? []; + const messagingToolSentTargets = overrides.messagingToolSentTargets ?? []; const successfulCronAdds = overrides.successfulCronAdds; return { aborted: false, @@ -182,12 +183,13 @@ const makeAttempt = (overrides: Partial): EmbeddedRunA didSendViaMessagingTool, messagingToolSentTexts, messagingToolSentMediaUrls, + messagingToolSentTargets, successfulCronAdds, }), didSendViaMessagingTool, messagingToolSentTexts, messagingToolSentMediaUrls, - messagingToolSentTargets: [], + messagingToolSentTargets, cloudCodeAssistFormatError: false, itemLifecycle: { startedCount: 0, completedCount: 0, activeCount: 0 }, ...overrides, diff --git a/src/agents/pi-embedded-runner/delivery-evidence.ts b/src/agents/pi-embedded-runner/delivery-evidence.ts new file mode 100644 index 00000000000..b4a502dc946 --- /dev/null +++ b/src/agents/pi-embedded-runner/delivery-evidence.ts @@ -0,0 +1,108 @@ +type AgentPayloadLike = { + text?: unknown; + mediaUrl?: unknown; + mediaUrls?: unknown; + presentation?: unknown; + interactive?: unknown; + channelData?: unknown; + isError?: unknown; + isReasoning?: unknown; +}; + +export type AgentDeliveryEvidence = { + payloads?: unknown; + didSendViaMessagingTool?: unknown; + messagingToolSentTexts?: unknown; + messagingToolSentMediaUrls?: unknown; + messagingToolSentTargets?: unknown; + successfulCronAdds?: unknown; + meta?: { + toolSummary?: { + calls?: unknown; + }; + }; +}; + +function hasNonEmptyString(value: unknown): value is string { + return typeof value === "string" && value.trim().length > 0; +} + +function hasNonEmptyArray(value: unknown): boolean { + return Array.isArray(value) && value.length > 0; +} + +function hasNonEmptyStringArray(value: unknown): boolean { + return Array.isArray(value) && value.some(hasNonEmptyString); +} + +function hasPositiveNumber(value: unknown): boolean { + return typeof value === "number" && Number.isFinite(value) && value > 0; +} + +export function getGatewayAgentResult(response: unknown): AgentDeliveryEvidence | null { + if (!response || typeof response !== "object" || !("result" in response)) { + return null; + } + const result = (response as { result?: unknown }).result; + if (!result || typeof result !== "object") { + return null; + } + return result as AgentDeliveryEvidence; +} + +export function hasVisibleAgentPayload( + result: Pick, + options: { includeErrorPayloads?: boolean; includeReasoningPayloads?: boolean } = {}, +): boolean { + const payloads = result.payloads; + if (!Array.isArray(payloads)) { + return false; + } + return payloads.some((payload) => { + if (!payload || typeof payload !== "object") { + return false; + } + const record = payload as AgentPayloadLike; + if (options.includeErrorPayloads === false && record.isError === true) { + return false; + } + if (options.includeReasoningPayloads === false && record.isReasoning === true) { + return false; + } + return Boolean( + hasNonEmptyString(record.text) || + hasNonEmptyString(record.mediaUrl) || + hasNonEmptyStringArray(record.mediaUrls) || + record.presentation || + record.interactive || + record.channelData, + ); + }); +} + +export function hasMessagingToolDeliveryEvidence(result: AgentDeliveryEvidence): boolean { + return ( + result.didSendViaMessagingTool === true || hasCommittedMessagingToolDeliveryEvidence(result) + ); +} + +export function hasCommittedMessagingToolDeliveryEvidence( + result: Pick< + AgentDeliveryEvidence, + "messagingToolSentTexts" | "messagingToolSentMediaUrls" | "messagingToolSentTargets" + >, +): boolean { + return ( + hasNonEmptyStringArray(result.messagingToolSentTexts) || + hasNonEmptyStringArray(result.messagingToolSentMediaUrls) || + hasNonEmptyArray(result.messagingToolSentTargets) + ); +} + +export function hasOutboundDeliveryEvidence(result: AgentDeliveryEvidence): boolean { + return ( + hasMessagingToolDeliveryEvidence(result) || + hasPositiveNumber(result.successfulCronAdds) || + hasPositiveNumber(result.meta?.toolSummary?.calls) + ); +} diff --git a/src/agents/pi-embedded-runner/result-fallback-classifier.ts b/src/agents/pi-embedded-runner/result-fallback-classifier.ts index a202d7066f9..27742d3362b 100644 --- a/src/agents/pi-embedded-runner/result-fallback-classifier.ts +++ b/src/agents/pi-embedded-runner/result-fallback-classifier.ts @@ -1,6 +1,7 @@ import { isSilentReplyPayloadText } from "../../auto-reply/tokens.js"; import { isGpt5ModelId } from "../gpt5-prompt-overlay.js"; import type { ModelFallbackResultClassification } from "../model-fallback.js"; +import { hasOutboundDeliveryEvidence, hasVisibleAgentPayload } from "./delivery-evidence.js"; import type { EmbeddedPiRunResult } from "./types.js"; const EMPTY_TERMINAL_REPLY_RE = /Agent couldn't generate a response/i; @@ -16,31 +17,6 @@ function isEmbeddedPiRunResult(value: unknown): value is EmbeddedPiRunResult { ); } -function hasVisibleNonErrorPayload(result: EmbeddedPiRunResult): boolean { - return (result.payloads ?? []).some((payload) => { - if (!payload || payload.isError === true || payload.isReasoning === true) { - return false; - } - const text = typeof payload.text === "string" ? payload.text.trim() : ""; - return ( - text.length > 0 || - Boolean(payload.mediaUrl) || - (Array.isArray(payload.mediaUrls) && payload.mediaUrls.length > 0) - ); - }); -} - -function hasOutboundSideEffects(result: EmbeddedPiRunResult): boolean { - return ( - result.didSendViaMessagingTool === true || - (result.messagingToolSentTexts?.length ?? 0) > 0 || - (result.messagingToolSentMediaUrls?.length ?? 0) > 0 || - (result.messagingToolSentTargets?.length ?? 0) > 0 || - (result.successfulCronAdds ?? 0) > 0 || - (result.meta.toolSummary?.calls ?? 0) > 0 - ); -} - function hasDeliberateSilentTerminalReply(result: EmbeddedPiRunResult): boolean { return [result.meta.finalAssistantRawText, result.meta.finalAssistantVisibleText].some( (text) => typeof text === "string" && isSilentReplyPayloadText(text), @@ -90,11 +66,14 @@ export function classifyEmbeddedPiRunResultForModelFallback(params: { params.result.meta.aborted || params.hasDirectlySentBlockReply === true || params.hasBlockReplyPipelineOutput === true || - hasVisibleNonErrorPayload(params.result) + hasVisibleAgentPayload(params.result, { + includeErrorPayloads: false, + includeReasoningPayloads: false, + }) ) { return null; } - if (hasOutboundSideEffects(params.result)) { + if (hasOutboundDeliveryEvidence(params.result)) { return null; } diff --git a/src/agents/pi-embedded-runner/run.incomplete-turn.test.ts b/src/agents/pi-embedded-runner/run.incomplete-turn.test.ts index 57c1289f145..c101c1c3549 100644 --- a/src/agents/pi-embedded-runner/run.incomplete-turn.test.ts +++ b/src/agents/pi-embedded-runner/run.incomplete-turn.test.ts @@ -1,5 +1,6 @@ import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; import type { OpenClawConfig } from "../../config/config.js"; +import { hasCommittedMessagingToolDeliveryEvidence } from "./delivery-evidence.js"; import { makeAttemptResult } from "./run.overflow-compaction.fixture.js"; import { loadRunOverflowCompactionHarness, @@ -18,7 +19,6 @@ import { DEFAULT_REASONING_ONLY_RETRY_LIMIT, EMPTY_RESPONSE_RETRY_INSTRUCTION, extractPlanningOnlyPlanDetails, - hasCommittedUserVisibleToolDelivery, isLikelyExecutionAckPrompt, PLANNING_ONLY_RETRY_INSTRUCTION, REASONING_ONLY_RETRY_INSTRUCTION, @@ -1345,24 +1345,34 @@ describe("runEmbeddedPiAgent incomplete-turn safety", () => { expect(incompleteTurnText).toContain("verify before retrying"); }); - it("does not treat empty committed messaging arrays as user-visible delivery", () => { + it("does not treat empty committed messaging arrays as delivery", () => { expect( - hasCommittedUserVisibleToolDelivery({ + hasCommittedMessagingToolDeliveryEvidence({ messagingToolSentTexts: [" "], messagingToolSentMediaUrls: [], }), ).toBe(false); }); - it("treats committed messaging media as user-visible delivery", () => { + it("treats committed messaging media as delivery", () => { expect( - hasCommittedUserVisibleToolDelivery({ + hasCommittedMessagingToolDeliveryEvidence({ messagingToolSentTexts: [], messagingToolSentMediaUrls: ["file:///tmp/render.png"], }), ).toBe(true); }); + it("treats committed messaging targets as delivery", () => { + expect( + hasCommittedMessagingToolDeliveryEvidence({ + messagingToolSentTexts: [], + messagingToolSentMediaUrls: [], + messagingToolSentTargets: [{ tool: "message", provider: "slack", to: "channel-1" }], + }), + ).toBe(true); + }); + it("treats committed messaging text as replay-invalid side effect metadata", () => { expect( buildAttemptReplayMetadata({ @@ -1385,6 +1395,18 @@ describe("runEmbeddedPiAgent incomplete-turn safety", () => { ).toEqual({ hadPotentialSideEffects: true, replaySafe: false }); }); + it("treats committed messaging targets as replay-invalid side effect metadata", () => { + expect( + buildAttemptReplayMetadata({ + toolMetas: [], + didSendViaMessagingTool: false, + messagingToolSentTexts: [], + messagingToolSentMediaUrls: [], + messagingToolSentTargets: [{ tool: "message", provider: "slack", to: "channel-1" }], + }), + ).toEqual({ hadPotentialSideEffects: true, replaySafe: false }); + }); + it("leaves committed delivery plus tool errors to the tool-error payload path", () => { const incompleteTurnText = resolveIncompleteTurnPayloadText({ payloadCount: 0, diff --git a/src/agents/pi-embedded-runner/run.overflow-compaction.fixture.ts b/src/agents/pi-embedded-runner/run.overflow-compaction.fixture.ts index d9464c7289b..3d4c04b2494 100644 --- a/src/agents/pi-embedded-runner/run.overflow-compaction.fixture.ts +++ b/src/agents/pi-embedded-runner/run.overflow-compaction.fixture.ts @@ -37,6 +37,7 @@ export function makeAttemptResult( const didSendViaMessagingTool = overrides.didSendViaMessagingTool ?? false; const messagingToolSentTexts = overrides.messagingToolSentTexts ?? []; const messagingToolSentMediaUrls = overrides.messagingToolSentMediaUrls ?? []; + const messagingToolSentTargets = overrides.messagingToolSentTargets ?? []; const successfulCronAdds = overrides.successfulCronAdds; return { aborted: false, @@ -58,6 +59,7 @@ export function makeAttemptResult( didSendViaMessagingTool, messagingToolSentTexts, messagingToolSentMediaUrls, + messagingToolSentTargets, successfulCronAdds, }), itemLifecycle: { @@ -68,7 +70,7 @@ export function makeAttemptResult( didSendViaMessagingTool, messagingToolSentTexts, messagingToolSentMediaUrls, - messagingToolSentTargets: [], + messagingToolSentTargets, cloudCodeAssistFormatError: false, ...overrides, }; diff --git a/src/agents/pi-embedded-runner/run.ts b/src/agents/pi-embedded-runner/run.ts index 3cd5a7a800b..cc9dff465f2 100644 --- a/src/agents/pi-embedded-runner/run.ts +++ b/src/agents/pi-embedded-runner/run.ts @@ -88,6 +88,7 @@ import { redactRunIdentifier, resolveRunWorkspaceDir } from "../workspace-run.js import { runPostCompactionSideEffects } from "./compaction-hooks.js"; import { buildEmbeddedCompactionRuntimeContext } from "./compaction-runtime-context.js"; import { runContextEngineMaintenance } from "./context-engine-maintenance.js"; +import { hasMessagingToolDeliveryEvidence } from "./delivery-evidence.js"; import { resolveEmbeddedRunFailureSignal } from "./failure-signal.js"; import { resolveGlobalLane, resolveSessionLane } from "./lanes.js"; import { log } from "./logger.js"; @@ -1204,7 +1205,7 @@ export async function runEmbeddedPiAgent( ? sessionLastAssistant.errorMessage?.trim() || formattedAssistantErrorText : undefined; const canRestartForLiveSwitch = - !attempt.didSendViaMessagingTool && + !hasMessagingToolDeliveryEvidence(attempt) && !attempt.didSendDeterministicApprovalPrompt && !attempt.lastToolError && (attempt.toolMetas?.length ?? 0) === 0 && diff --git a/src/agents/pi-embedded-runner/run/incomplete-turn.ts b/src/agents/pi-embedded-runner/run/incomplete-turn.ts index 5bcf46f6123..96a99502a03 100644 --- a/src/agents/pi-embedded-runner/run/incomplete-turn.ts +++ b/src/agents/pi-embedded-runner/run/incomplete-turn.ts @@ -12,6 +12,10 @@ import { stripProviderPrefix, } from "../../execution-contract.js"; import { isLikelyMutatingToolName } from "../../tool-mutation.js"; +import { + hasCommittedMessagingToolDeliveryEvidence, + hasMessagingToolDeliveryEvidence, +} from "../delivery-evidence.js"; import { isZeroUsageEmptyStopAssistantTurn } from "../empty-assistant-turn.js"; import { assessLastAssistantMessage } from "../thinking.js"; import type { EmbeddedRunLivenessState } from "../types.js"; @@ -24,7 +28,8 @@ type ReplayMetadataAttempt = Pick< | "messagingToolSentTexts" | "messagingToolSentMediaUrls" | "successfulCronAdds" ->; +> & + Partial>; type IncompleteTurnAttempt = Pick< EmbeddedRunAttemptResult, @@ -36,6 +41,7 @@ type IncompleteTurnAttempt = Pick< | "didSendViaMessagingTool" | "messagingToolSentTexts" | "messagingToolSentMediaUrls" + | "messagingToolSentTargets" | "lastToolError" | "lastAssistant" | "replayMetadata" @@ -54,6 +60,9 @@ type PlanningOnlyAttempt = Pick< | "lastAssistant" | "itemLifecycle" | "replayMetadata" + | "messagingToolSentTexts" + | "messagingToolSentMediaUrls" + | "messagingToolSentTargets" | "toolMetas" >; @@ -185,30 +194,13 @@ export type PlanningOnlyPlanDetails = { steps: string[]; }; -function hasStringEntry(values: readonly unknown[] | undefined): boolean { - return ( - Array.isArray(values) && - values.some((value) => typeof value === "string" && value.trim().length > 0) - ); -} - -export function hasCommittedUserVisibleToolDelivery( - attempt: Pick, -): boolean { - return ( - hasStringEntry(attempt.messagingToolSentTexts) || - hasStringEntry(attempt.messagingToolSentMediaUrls) - ); -} - export function buildAttemptReplayMetadata( params: ReplayMetadataAttempt, ): EmbeddedRunAttemptResult["replayMetadata"] { const hadMutatingTools = params.toolMetas.some((t) => isLikelyMutatingToolName(t.toolName)); const hadPotentialSideEffects = hadMutatingTools || - params.didSendViaMessagingTool || - hasCommittedUserVisibleToolDelivery(params) || + hasMessagingToolDeliveryEvidence(params) || (params.successfulCronAdds ?? 0) > 0; return { hadPotentialSideEffects, @@ -244,7 +236,7 @@ export function resolveIncompleteTurnPayloadText(params: { return null; } - if (hasCommittedUserVisibleToolDelivery(params.attempt)) { + if (hasCommittedMessagingToolDeliveryEvidence(params.attempt)) { return null; } @@ -494,7 +486,7 @@ export function shouldTreatEmptyAssistantReplyAsSilent(params: { if (!params.allowEmptyAssistantReplyAsSilent || shouldSkipPlanningOnlyRetry(params)) { return false; } - if (hasCommittedUserVisibleToolDelivery(params.attempt)) { + if (hasCommittedMessagingToolDeliveryEvidence(params.attempt)) { return false; } return isNonVisibleAssistantTurnEligibleForSilentReply({ @@ -830,7 +822,7 @@ export function resolvePlanningOnlyRetryInstruction(params: { params.attempt.clientToolCall || params.attempt.yieldDetected || params.attempt.didSendDeterministicApprovalPrompt || - params.attempt.didSendViaMessagingTool || + hasMessagingToolDeliveryEvidence(params.attempt) || params.attempt.lastToolError || (hasNonPlanToolActivity(params.attempt.toolMetas) && !allowSingleActionRetryBypass) || ((params.attempt.itemLifecycle?.startedCount ?? 0) > planOnlyToolMetaCount && diff --git a/src/agents/pi-embedded-subscribe.handlers.lifecycle.ts b/src/agents/pi-embedded-subscribe.handlers.lifecycle.ts index f8e3e07d466..8840754c057 100644 --- a/src/agents/pi-embedded-subscribe.handlers.lifecycle.ts +++ b/src/agents/pi-embedded-subscribe.handlers.lifecycle.ts @@ -6,6 +6,7 @@ import { sanitizeForConsole, } from "./pi-embedded-error-observation.js"; import { classifyFailoverReason, formatAssistantErrorText } from "./pi-embedded-helpers.js"; +import { hasCommittedMessagingToolDeliveryEvidence } from "./pi-embedded-runner/delivery-evidence.js"; import { isIncompleteTerminalAssistantTurn } from "./pi-embedded-runner/run/incomplete-turn.js"; import { consumePendingToolMediaReply, @@ -45,8 +46,7 @@ export function handleAgentEnd(ctx: EmbeddedPiSubscribeContext): void | Promise< ctx.state.assistantTexts.some((text) => hasAssistantVisibleReply({ text })); const hadDeterministicSideEffect = ctx.state.hadDeterministicSideEffect === true || - (ctx.state.messagingToolSentTexts?.length ?? 0) > 0 || - (ctx.state.messagingToolSentMediaUrls?.length ?? 0) > 0 || + hasCommittedMessagingToolDeliveryEvidence(ctx.state) || (ctx.state.successfulCronAdds ?? 0) > 0; const incompleteTerminalAssistant = isIncompleteTerminalAssistantTurn({ hasAssistantVisibleText, diff --git a/src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.suppresses-message-end-block-replies-message-tool.test.ts b/src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.suppresses-message-end-block-replies-message-tool.test.ts index 13972d951c1..f050a37032b 100644 --- a/src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.suppresses-message-end-block-replies-message-tool.test.ts +++ b/src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.suppresses-message-end-block-replies-message-tool.test.ts @@ -10,26 +10,27 @@ import { subscribeEmbeddedPiSession } from "./pi-embedded-subscribe.js"; function createBlockReplyHarness(blockReplyBreak: "message_end" | "text_end") { const { session, emit } = createStubSessionHarness(); const onBlockReply = vi.fn(); - subscribeEmbeddedPiSession({ + const subscription = subscribeEmbeddedPiSession({ session, runId: "run", onBlockReply, blockReplyBreak, }); - return { emit, onBlockReply }; + return { emit, onBlockReply, subscription }; } async function emitMessageToolLifecycle(params: { emit: (evt: unknown) => void; toolCallId: string; message: string; + media?: string; result: unknown; }) { params.emit({ type: "tool_execution_start", toolName: "message", toolCallId: params.toolCallId, - args: { action: "send", to: "+1555", message: params.message }, + args: { action: "send", to: "+1555", message: params.message, media: params.media }, }); // Wait for async handler to complete. await Promise.resolve(); @@ -77,6 +78,23 @@ describe("subscribeEmbeddedPiSession", () => { expect(onBlockReply).not.toHaveBeenCalled(); }); + + it("tracks media-only message tool sends as messaging delivery", async () => { + const { emit, subscription } = createBlockReplyHarness("message_end"); + + await emitMessageToolLifecycle({ + emit, + toolCallId: "tool-message-media", + message: "", + media: "file:///tmp/render.mp4", + result: "ok", + }); + await Promise.resolve(); + + expect(subscription.didSendViaMessagingTool()).toBe(true); + expect(subscription.getMessagingToolSentMediaUrls()).toEqual(["file:///tmp/render.mp4"]); + }); + it("does not suppress message_end replies when message tool reports error", async () => { const { emit, onBlockReply } = createBlockReplyHarness("message_end"); diff --git a/src/agents/pi-embedded-subscribe.ts b/src/agents/pi-embedded-subscribe.ts index 96dc2f2bb30..72d7384ee95 100644 --- a/src/agents/pi-embedded-subscribe.ts +++ b/src/agents/pi-embedded-subscribe.ts @@ -16,6 +16,7 @@ import { normalizeTextForComparison, } from "./pi-embedded-helpers.js"; import type { BlockReplyPayload } from "./pi-embedded-payloads.js"; +import { hasCommittedMessagingToolDeliveryEvidence } from "./pi-embedded-runner/delivery-evidence.js"; import { createEmbeddedRunReplayState, mergeEmbeddedRunReplayState, @@ -855,8 +856,11 @@ export function subscribeEmbeddedPiSession(params: SubscribeEmbeddedPiSessionPar const resetForCompactionRetry = () => { state.hadDeterministicSideEffect = state.hadDeterministicSideEffect === true || - messagingToolSentTexts.length > 0 || - messagingToolSentMediaUrls.length > 0 || + hasCommittedMessagingToolDeliveryEvidence({ + messagingToolSentTexts, + messagingToolSentMediaUrls, + messagingToolSentTargets, + }) || state.successfulCronAdds > 0; assistantTexts.length = 0; toolMetas.length = 0; @@ -998,7 +1002,12 @@ export function subscribeEmbeddedPiSession(params: SubscribeEmbeddedPiSessionPar // Returns true if any messaging tool successfully sent a message. // Used to suppress agent's confirmation text (e.g., "Respondi no Telegram!") // which is generated AFTER the tool sends the actual answer. - didSendViaMessagingTool: () => messagingToolSentTexts.length > 0, + didSendViaMessagingTool: () => + hasCommittedMessagingToolDeliveryEvidence({ + messagingToolSentTexts, + messagingToolSentMediaUrls, + messagingToolSentTargets, + }), didSendDeterministicApprovalPrompt: () => state.deterministicApprovalPromptSent, getLastToolError: () => (state.lastToolError ? { ...state.lastToolError } : undefined), getUsageTotals, diff --git a/src/agents/subagent-announce-delivery.test.ts b/src/agents/subagent-announce-delivery.test.ts index 6c60f6d73bc..c65cb29db6a 100644 --- a/src/agents/subagent-announce-delivery.test.ts +++ b/src/agents/subagent-announce-delivery.test.ts @@ -829,6 +829,45 @@ describe("deliverSubagentAnnouncement completion delivery", () => { ); }); + it("does not fallback when announce-agent delivered media through the message tool", async () => { + const callGateway = createGatewayMock({ + result: { + payloads: [], + didSendViaMessagingTool: false, + messagingToolSentMediaUrls: ["/tmp/generated-night-drive.mp3"], + }, + }); + const sendMessage = createSendMessageMock(); + const result = await deliverDiscordDirectMessageCompletion({ + callGateway, + sendMessage, + internalEvents: [ + { + type: "task_completion", + source: "music_generation", + childSessionKey: "music_generate:task-123", + childSessionId: "task-123", + announceType: "music generation task", + taskLabel: "night-drive synthwave", + status: "ok", + statusLabel: "completed successfully", + result: "Generated 1 track.\nMEDIA:/tmp/generated-night-drive.mp3", + mediaUrls: ["/tmp/generated-night-drive.mp3"], + replyInstruction: "Deliver the generated music through the message tool.", + }, + ], + }); + + expect(result).toEqual( + expect.objectContaining({ + delivered: true, + path: "direct", + }), + ); + expect(callGateway).toHaveBeenCalled(); + expect(sendMessage).not.toHaveBeenCalled(); + }); + it("uses a direct channel fallback when announce-agent returns no visible output", async () => { const callGateway = createGatewayMock({ result: { diff --git a/src/agents/subagent-announce-delivery.ts b/src/agents/subagent-announce-delivery.ts index f6ce7090b80..9b4d9156838 100644 --- a/src/agents/subagent-announce-delivery.ts +++ b/src/agents/subagent-announce-delivery.ts @@ -19,6 +19,11 @@ import { } from "../utils/message-channel.js"; import { buildAnnounceIdempotencyKey, resolveQueueAnnounceId } from "./announce-idempotency.js"; import type { AgentInternalEvent } from "./internal-events.js"; +import { + getGatewayAgentResult, + hasMessagingToolDeliveryEvidence, + hasVisibleAgentPayload, +} from "./pi-embedded-runner/delivery-evidence.js"; import { callGateway, createBoundDeliveryRouter, @@ -554,43 +559,10 @@ export function extractThreadCompletionFallbackText(internalEvents?: AgentIntern } function hasVisibleGatewayAgentPayload(response: unknown): boolean { - const result = - response && typeof response === "object" && "result" in response - ? (response as { result?: unknown }).result - : undefined; - const payloads = - result && typeof result === "object" && "payloads" in result - ? (result as { payloads?: unknown }).payloads - : undefined; - if (!Array.isArray(payloads)) { - return false; - } - return payloads.some((payload) => { - if (!payload || typeof payload !== "object") { - return false; - } - const record = payload as { - text?: unknown; - mediaUrl?: unknown; - mediaUrls?: unknown; - presentation?: unknown; - interactive?: unknown; - channelData?: unknown; - }; - const text = typeof record.text === "string" ? record.text.trim() : ""; - const mediaUrl = typeof record.mediaUrl === "string" ? record.mediaUrl.trim() : ""; - const mediaUrls = Array.isArray(record.mediaUrls) - ? record.mediaUrls.some((item) => typeof item === "string" && item.trim()) - : false; - return Boolean( - text || - mediaUrl || - mediaUrls || - record.presentation || - record.interactive || - record.channelData, - ); - }); + const result = getGatewayAgentResult(response); + return Boolean( + result && (hasVisibleAgentPayload(result) || hasMessagingToolDeliveryEvidence(result)), + ); } async function sendCompletionFallback(params: { diff --git a/src/agents/subagent-registry-lifecycle.test.ts b/src/agents/subagent-registry-lifecycle.test.ts index a12ed3e91da..c3c7a3af971 100644 --- a/src/agents/subagent-registry-lifecycle.test.ts +++ b/src/agents/subagent-registry-lifecycle.test.ts @@ -4,6 +4,8 @@ import { SUBAGENT_ENDED_REASON_COMPLETE } from "./subagent-lifecycle-events.js"; import { createSubagentRegistryLifecycleController } from "./subagent-registry-lifecycle.js"; import type { SubagentRunRecord } from "./subagent-registry.types.js"; +type LifecycleControllerParams = Parameters[0]; + const taskExecutorMocks = vi.hoisted(() => ({ completeTaskRunByRunId: vi.fn(), failTaskRunByRunId: vi.fn(), @@ -110,7 +112,7 @@ function createLifecycleController({ entry: SubagentRunRecord; runs?: Map; } & Partial[0]>) { - return createSubagentRegistryLifecycleController({ + const params: LifecycleControllerParams = { runs, resumedRuns: new Set(), subagentAnnounceTimeoutMs: 1_000, @@ -128,8 +130,9 @@ function createLifecycleController({ captureSubagentCompletionReply: vi.fn(async () => "final completion reply"), runSubagentAnnounceFlow: vi.fn(async () => true), warn: vi.fn(), - ...overrides, - }); + }; + Object.assign(params, overrides); + return createSubagentRegistryLifecycleController(params); } describe("subagent registry lifecycle hardening", () => { diff --git a/src/agents/test-helpers/pi-embedded-runner-e2e-fixtures.ts b/src/agents/test-helpers/pi-embedded-runner-e2e-fixtures.ts index 4fed384fff1..185c6af2a24 100644 --- a/src/agents/test-helpers/pi-embedded-runner-e2e-fixtures.ts +++ b/src/agents/test-helpers/pi-embedded-runner-e2e-fixtures.ts @@ -101,6 +101,7 @@ export function makeEmbeddedRunnerAttempt( const didSendViaMessagingTool = overrides.didSendViaMessagingTool ?? false; const messagingToolSentTexts = overrides.messagingToolSentTexts ?? []; const messagingToolSentMediaUrls = overrides.messagingToolSentMediaUrls ?? []; + const messagingToolSentTargets = overrides.messagingToolSentTargets ?? []; const successfulCronAdds = overrides.successfulCronAdds; return { aborted: false, @@ -123,12 +124,13 @@ export function makeEmbeddedRunnerAttempt( didSendViaMessagingTool, messagingToolSentTexts, messagingToolSentMediaUrls, + messagingToolSentTargets, successfulCronAdds, }), didSendViaMessagingTool, messagingToolSentTexts, messagingToolSentMediaUrls, - messagingToolSentTargets: [], + messagingToolSentTargets, cloudCodeAssistFormatError: false, itemLifecycle: { startedCount: 0, completedCount: 0, activeCount: 0 }, ...overrides,