diff --git a/packages/junior/src/chat/app/factory.ts b/packages/junior/src/chat/app/factory.ts index d4283c3f..10b8d204 100644 --- a/packages/junior/src/chat/app/factory.ts +++ b/packages/junior/src/chat/app/factory.ts @@ -7,12 +7,7 @@ import { import { createJuniorRuntimeServices } from "@/chat/app/services"; import type { JuniorRuntimeServiceOverrides } from "@/chat/app/services"; import { coerceThreadConversationState } from "@/chat/state/conversation"; -import { - logException, - logWarn, - resolveErrorReference, - withSpan, -} from "@/chat/logging"; +import { logException, logWarn, withSpan } from "@/chat/logging"; import { createReplyToThread } from "@/chat/runtime/reply-executor"; import { initializeAssistantThread as initializeAssistantThreadImpl, @@ -66,7 +61,6 @@ export function createSlackRuntime( assistantUserName: botConfig.userName, modelId: botConfig.modelId, now: options.now ?? (() => Date.now()), - getErrorReference: resolveErrorReference, getThreadId, getChannelId, getRunId, diff --git a/packages/junior/src/chat/logging.ts b/packages/junior/src/chat/logging.ts index 31acf5c1..5e1e479a 100644 --- a/packages/junior/src/chat/logging.ts +++ b/packages/junior/src/chat/logging.ts @@ -1413,11 +1413,6 @@ export function setSentryScopeContext( // High-level observability API (spans, error capture, convenience loggers) // --------------------------------------------------------------------------- -export interface ErrorReference { - traceId: string; - eventId?: string; -} - type SpanAttributePrimitive = string | number | boolean; type SpanAttributeValue = SpanAttributePrimitive | string[]; @@ -1657,21 +1652,12 @@ export function getActiveTraceId(): string | undefined { } } -/** Build a trace + event ID reference for error correlation. */ -export function resolveErrorReference(eventId?: string): ErrorReference | null { - const traceId = getActiveTraceId(); - if (!eventId && !traceId) { - return null; - } +const TURN_FAILURE_RESPONSE_TEMPLATE = + "I ran into an internal error while processing that. Reference: `event_id={eventId}`."; - if (!traceId) { - return null; - } - - return { - traceId, - ...(eventId ? { eventId } : {}), - }; +/** Build the static user-facing response for a failed turn. */ +export function buildTurnFailureResponse(eventId: string): string { + return TURN_FAILURE_RESPONSE_TEMPLATE.replace("{eventId}", eventId); } // --------------------------------------------------------------------------- diff --git a/packages/junior/src/chat/respond-helpers.ts b/packages/junior/src/chat/respond-helpers.ts index f343a552..eec87f42 100644 --- a/packages/junior/src/chat/respond-helpers.ts +++ b/packages/junior/src/chat/respond-helpers.ts @@ -227,15 +227,6 @@ export function encodeNonImageAttachmentForPrompt(attachment: { ].join("\n"); } -/** Build a user-facing message for execution failures. */ -export function buildExecutionFailureMessage(toolErrorCount: number): string { - if (toolErrorCount > 0) { - return "I couldn't complete this because one or more required tools failed in this turn. I've logged the failure details."; - } - - return "I couldn't complete this request in this turn due to an execution failure. I've logged the details for debugging."; -} - /** Type guard for Pi SDK tool result messages. */ export function isToolResultMessage( value: unknown, diff --git a/packages/junior/src/chat/runtime/reply-executor.ts b/packages/junior/src/chat/runtime/reply-executor.ts index 2b7ff488..ee6cac82 100644 --- a/packages/junior/src/chat/runtime/reply-executor.ts +++ b/packages/junior/src/chat/runtime/reply-executor.ts @@ -16,7 +16,6 @@ import { type PlannedSlackReplyStage, } from "@/chat/slack/reply"; import { buildSlackOutputMessage } from "@/chat/slack/output"; -import { GEN_AI_PROVIDER_NAME } from "@/chat/pi/client"; import { generateAssistantReply as generateAssistantReplyImpl } from "@/chat/respond"; import { shouldEmitDevAgentTrace } from "@/chat/runtime/dev-agent-trace"; import { @@ -62,6 +61,10 @@ import { markTurnCompleted, markTurnFailed } from "@/chat/runtime/turn"; import { startActiveTurn } from "@/chat/runtime/turn"; import { isRedundantReactionAckText } from "@/chat/services/reply-delivery-plan"; import { deleteSlackMessage } from "@/chat/slack/outbound"; +import { + finalizeFailedTurnReply, + getAgentTurnDiagnosticsAttributes, +} from "@/chat/services/turn-failure-response"; export interface ReplyExecutorServices { generateAssistantReply: typeof generateAssistantReplyImpl; @@ -72,26 +75,6 @@ export interface ReplyExecutorServices { ) => Promise; } -function getExecutionFailureReason(reply: { - diagnostics: { - assistantMessageCount: number; - errorMessage?: string; - toolErrorCount: number; - }; -}): string { - const errorMessage = reply.diagnostics.errorMessage?.trim(); - if (errorMessage) { - return errorMessage; - } - if (reply.diagnostics.toolErrorCount > 0) { - return `${reply.diagnostics.toolErrorCount} tool result error(s)`; - } - if (reply.diagnostics.assistantMessageCount > 0) { - return "assistant returned no text"; - } - return "empty assistant turn"; -} - interface ReplyExecutorDeps { getSlackAdapter: () => SlackAdapter; resolveUserAttachments: ( @@ -302,7 +285,7 @@ export function createReplyToThread(deps: ReplyExecutorDeps) { try { const toolChannelId = preparedState.artifacts.assistantContextChannelId ?? channelId; - const reply = await deps.services.generateAssistantReply(userText, { + let reply = await deps.services.generateAssistantReply(userText, { requester: { userId: message.author.userId, userName: message.author.userName ?? fallbackIdentity?.userName, @@ -364,59 +347,15 @@ export function createReplyToThread(deps: ReplyExecutorDeps) { assistantUserName: botConfig.userName, modelId: reply.diagnostics.modelId, }; - const diagnosticsAttributes = { - "gen_ai.provider.name": GEN_AI_PROVIDER_NAME, - "gen_ai.operation.name": "invoke_agent", - "app.ai.outcome": reply.diagnostics.outcome, - "app.ai.assistant_messages": - reply.diagnostics.assistantMessageCount, - "app.ai.tool_results": reply.diagnostics.toolResultCount, - "app.ai.tool_error_results": reply.diagnostics.toolErrorCount, - "app.ai.tool_call_count": reply.diagnostics.toolCalls.length, - "app.ai.used_primary_text": reply.diagnostics.usedPrimaryText, - ...(reply.diagnostics.thinkingLevel - ? { - "app.ai.reasoning_effort": reply.diagnostics.thinkingLevel, - } - : {}), - ...(reply.diagnostics.stopReason - ? { - "gen_ai.response.finish_reasons": [ - reply.diagnostics.stopReason, - ], - } - : {}), - ...(reply.diagnostics.errorMessage - ? { "error.message": reply.diagnostics.errorMessage } - : {}), - }; + const diagnosticsAttributes = + getAgentTurnDiagnosticsAttributes(reply); setSpanAttributes(diagnosticsAttributes); - if (reply.diagnostics.outcome === "provider_error") { - const providerError = - reply.diagnostics.providerError ?? - new Error( - reply.diagnostics.errorMessage ?? - "Provider error without explicit message", - ); - logException( - providerError, - "agent_turn_provider_error", - diagnosticsContext, - diagnosticsAttributes, - "Agent turn failed with provider error", - ); - } else if (reply.diagnostics.outcome !== "success") { - const failureReason = getExecutionFailureReason(reply); - logException( - new Error(`Agent turn execution failure: ${failureReason}`), - "agent_turn_execution_failure", - diagnosticsContext, - { - ...diagnosticsAttributes, - "app.ai.execution_failure_reason": failureReason, - }, - "Agent turn completed with execution failure", - ); + if (reply.diagnostics.outcome !== "success") { + reply = finalizeFailedTurnReply({ + reply, + logException, + context: diagnosticsContext, + }); } markConversationMessage( diff --git a/packages/junior/src/chat/runtime/slack-runtime.ts b/packages/junior/src/chat/runtime/slack-runtime.ts index 030725da..f37f7020 100644 --- a/packages/junior/src/chat/runtime/slack-runtime.ts +++ b/packages/junior/src/chat/runtime/slack-runtime.ts @@ -1,7 +1,7 @@ import type { Message, Thread } from "chat"; import { getSubscribedReplyPreflightDecision } from "@/chat/services/subscribed-decision"; import { isRetryableTurnError } from "@/chat/runtime/turn"; -import type { ErrorReference } from "@/chat/logging"; +import { buildTurnFailureResponse } from "@/chat/logging"; import { getSlackErrorObservabilityAttributes } from "@/chat/runtime/thread-context"; import type { SubscribedReplyDecision } from "@/chat/services/subscribed-reply-policy"; @@ -89,7 +89,6 @@ export interface SlackTurnRuntimeDependencies { ) => void; modelId: string; now: () => number; - getErrorReference: (eventId?: string) => ErrorReference | null; recordSkippedSubscribedMessage: (args: { completedAtMs: number; decision: SubscribedReplyDecision; @@ -146,16 +145,6 @@ export interface SlackTurnRuntimeDependencies { ) => Promise; } -function buildFailureMessage(reference: ErrorReference | null): string { - if (!reference) { - return "I ran into an internal error while processing that. Please try again."; - } - if (reference.eventId) { - return `I ran into an internal error while processing that. Reference: \`event_id=${reference.eventId} trace_id=${reference.traceId}\`.`; - } - return `I ran into an internal error while processing that. Reference: \`trace_id=${reference.traceId}\`.`; -} - export interface SlackTurnRuntime< _TPreparedState, TAssistantEvent extends AssistantLifecycleEvent = AssistantLifecycleEvent, @@ -215,14 +204,13 @@ export function createSlackTurnRuntime< const postFallbackErrorReplyWithLogging = async (args: { thread: Thread; - reference: ErrorReference | null; errorContext: RuntimeLogContext; - eventId?: string; + eventId: string; postFailureEventName: string; postFailureBody: string; }): Promise => { try { - await args.thread.post(buildFailureMessage(args.reference)); + await args.thread.post(buildTurnFailureResponse(args.eventId)); } catch (postError) { deps.logException( postError, @@ -230,9 +218,7 @@ export function createSlackTurnRuntime< args.errorContext, { "app.slack.reply_stage": "error_fallback_post", - ...(args.eventId - ? { "app.error.original_event_id": args.eventId } - : {}), + "app.error.original_event_id": args.eventId, ...getSlackErrorObservabilityAttributes(postError), }, args.postFailureBody, @@ -335,11 +321,14 @@ export function createSlackTurnRuntime< {}, "onNewMention failed", ); + if (!eventId) { + throw new Error( + "Sentry did not return an event ID for mention_handler_failed", + ); + } await hooks?.beforeFirstResponsePost?.(); - const reference = deps.getErrorReference(eventId); await postFallbackErrorReplyWithLogging({ thread, - reference, errorContext, eventId, postFailureEventName: "mention_handler_failure_reply_post_failed", @@ -491,11 +480,14 @@ export function createSlackTurnRuntime< {}, "onSubscribedMessage failed", ); + if (!eventId) { + throw new Error( + "Sentry did not return an event ID for subscribed_message_handler_failed", + ); + } await hooks?.beforeFirstResponsePost?.(); - const reference = deps.getErrorReference(eventId); await postFallbackErrorReplyWithLogging({ thread, - reference, errorContext, eventId, postFailureEventName: diff --git a/packages/junior/src/chat/services/turn-failure-response.ts b/packages/junior/src/chat/services/turn-failure-response.ts new file mode 100644 index 00000000..5a614145 --- /dev/null +++ b/packages/junior/src/chat/services/turn-failure-response.ts @@ -0,0 +1,143 @@ +import type { LogContext } from "@/chat/logging"; +import { buildTurnFailureResponse } from "@/chat/logging"; +import { GEN_AI_PROVIDER_NAME } from "@/chat/pi/client"; +import type { AssistantReply } from "@/chat/services/turn-result"; + +type LogException = ( + error: unknown, + eventName: string, + context?: LogContext, + attributes?: Record, + body?: string, +) => string | undefined; + +/** Require captured turn failures to carry a real Sentry event reference. */ +export function requireTurnFailureEventId( + eventId: string | undefined, + eventName: string, +): string { + if (!eventId) { + throw new Error(`Sentry did not return an event ID for ${eventName}`); + } + return eventId; +} + +function getExecutionFailureReason(reply: { + diagnostics: { + assistantMessageCount: number; + errorMessage?: string; + toolErrorCount: number; + }; +}): string { + const errorMessage = reply.diagnostics.errorMessage?.trim(); + if (errorMessage) { + return errorMessage; + } + if (reply.diagnostics.toolErrorCount > 0) { + return `${reply.diagnostics.toolErrorCount} tool result error(s)`; + } + if (reply.diagnostics.assistantMessageCount > 0) { + return "assistant returned no text"; + } + return "empty assistant turn"; +} + +function getFailureCapture(reply: AssistantReply): { + attributes: Record; + body: string; + error: unknown; + eventName: string; +} { + if (reply.diagnostics.outcome === "provider_error") { + return { + eventName: "agent_turn_provider_error", + error: + reply.diagnostics.providerError ?? + new Error( + reply.diagnostics.errorMessage ?? + "Provider error without explicit message", + ), + attributes: {}, + body: "Agent turn failed with provider error", + }; + } + + const failureReason = getExecutionFailureReason(reply); + return { + eventName: "agent_turn_execution_failure", + error: new Error(`Agent turn execution failure: ${failureReason}`), + attributes: { + "app.ai.execution_failure_reason": failureReason, + }, + body: "Agent turn completed with execution failure", + }; +} + +/** Keep failed-turn Sentry captures and completion spans on the same keys. */ +export function getAgentTurnDiagnosticsAttributes( + reply: AssistantReply, +): Record { + return { + "gen_ai.provider.name": GEN_AI_PROVIDER_NAME, + "gen_ai.operation.name": "invoke_agent", + "app.ai.outcome": reply.diagnostics.outcome, + "app.ai.assistant_messages": reply.diagnostics.assistantMessageCount, + "app.ai.tool_results": reply.diagnostics.toolResultCount, + "app.ai.tool_error_results": reply.diagnostics.toolErrorCount, + "app.ai.tool_call_count": reply.diagnostics.toolCalls.length, + "app.ai.used_primary_text": reply.diagnostics.usedPrimaryText, + ...(reply.diagnostics.thinkingLevel + ? { + "app.ai.reasoning_effort": reply.diagnostics.thinkingLevel, + } + : {}), + ...(reply.diagnostics.stopReason + ? { + "gen_ai.response.finish_reasons": [reply.diagnostics.stopReason], + } + : {}), + ...(reply.diagnostics.errorMessage + ? { "error.message": reply.diagnostics.errorMessage } + : {}), + }; +} + +/** Enforce one captured, event-ID-bearing failure response before delivery. */ +export function finalizeFailedTurnReply(args: { + reply: AssistantReply; + logException: LogException; + context: LogContext; + attributes?: Record; +}): AssistantReply { + if (args.reply.diagnostics.outcome === "success") { + return args.reply; + } + + const capture = getFailureCapture(args.reply); + const eventId = requireTurnFailureEventId( + args.logException( + capture.error, + capture.eventName, + args.context, + { + ...getAgentTurnDiagnosticsAttributes(args.reply), + ...args.attributes, + ...capture.attributes, + }, + capture.body, + ), + capture.eventName, + ); + + return { + ...args.reply, + text: buildTurnFailureResponse(eventId), + deliveryMode: "thread", + deliveryPlan: { + mode: "thread", + postThreadText: true, + attachFiles: + args.reply.files && args.reply.files.length > 0 ? "inline" : "none", + }, + }; +} diff --git a/packages/junior/src/chat/services/turn-result.ts b/packages/junior/src/chat/services/turn-result.ts index 33d3bd75..afcbebd7 100644 --- a/packages/junior/src/chat/services/turn-result.ts +++ b/packages/junior/src/chat/services/turn-result.ts @@ -14,7 +14,6 @@ import { isExplicitChannelPostIntent } from "@/chat/services/channel-intent"; import { enforceAttachmentClaimTruth } from "@/chat/services/attachment-claims"; import type { ThreadArtifactsState } from "@/chat/state/artifacts"; import { - buildExecutionFailureMessage, extractAssistantText, getTerminalAssistantMessages, isAssistantMessage, @@ -198,16 +197,13 @@ export function buildTurnResult(input: TurnResultInput): AssistantReply { : sideEffectOnlySuccess ? "success" : "execution_failure"; - const fallbackText = buildExecutionFailureMessage(toolErrorCount); const suppressReactionOnlyText = reactionPerformed && !channelPostPerformed && replyFiles.length === 0 && Boolean(primaryText) && isReactionOnlyIntent(userInput); - const rawResponseText = suppressReactionOnlyText - ? "" - : primaryText || (sideEffectOnlySuccess ? "" : fallbackText); + const rawResponseText = suppressReactionOnlyText ? "" : primaryText; const responseText = canvasCreated && isVerbosePostCanvasReply(rawResponseText) ? buildBriefPostCanvasReply(artifactStatePatch) @@ -217,22 +213,22 @@ export function buildTurnResult(input: TurnResultInput): AssistantReply { (isExecutionEscapeResponse(primaryText) || isRawToolPayloadResponse(primaryText)); const resolvedText = escapedOrRawPayload - ? fallbackText + ? "" : enforceAttachmentClaimTruth(responseText, replyFiles.length > 0); + const resolvedOutcome: AgentTurnDiagnostics["outcome"] = escapedOrRawPayload + ? "execution_failure" + : outcome; const deliveryPlan = - reactionPerformed && + resolvedOutcome === "success" && !resolvedText && replyFiles.length === 0 && - !channelPostPerformed + (reactionPerformed || channelPostPerformed) ? { ...baseDeliveryPlan, postThreadText: false, } : baseDeliveryPlan; const deliveryMode: "thread" | "channel_only" = deliveryPlan.mode; - const resolvedOutcome: AgentTurnDiagnostics["outcome"] = escapedOrRawPayload - ? "execution_failure" - : outcome; if (shouldTrace) { logInfo( diff --git a/packages/junior/src/chat/slack/output.ts b/packages/junior/src/chat/slack/output.ts index f7806f8b..8b0396f8 100644 --- a/packages/junior/src/chat/slack/output.ts +++ b/packages/junior/src/chat/slack/output.ts @@ -1,5 +1,4 @@ import type { FileUpload, PostableMessage } from "chat"; -import { logWarn } from "@/chat/logging"; import { renderSlackMrkdwn } from "@/chat/slack/mrkdwn"; const MAX_INLINE_CHARS = 2200; @@ -338,20 +337,9 @@ export function buildSlackOutputMessage( }; } - logWarn( - "slack_output_normalized_empty", - {}, - { - "app.output.original_length": text.length, - "app.output.parsed_length": normalized.length, - "app.output.file_count": fileCount, - }, - "Slack output normalized to empty content", + throw new Error( + `Slack output normalized to empty content: original_length=${text.length} parsed_length=${normalized.length}`, ); - return { - markdown: "I couldn't produce a response.", - files, - }; } return { diff --git a/packages/junior/src/chat/slack/reply.ts b/packages/junior/src/chat/slack/reply.ts index 3710c16c..0099c679 100644 --- a/packages/junior/src/chat/slack/reply.ts +++ b/packages/junior/src/chat/slack/reply.ts @@ -23,10 +23,6 @@ export interface PlannedSlackReplyPost { text: string; } -function isInterruptedVisibleReply(reply: AssistantReply): boolean { - return reply.diagnostics.outcome === "provider_error"; -} - function resolveReplyDelivery(reply: AssistantReply): { shouldPostThreadReply: boolean; attachFiles: ReplyFileDelivery; @@ -68,13 +64,10 @@ function buildReplyText(text: string): string { function buildTextPosts(args: { text: string; - interrupted: boolean; firstFiles?: FileUpload[]; firstStage?: PlannedSlackReplyStage; }): PlannedSlackReplyPost[] { - const chunks = splitSlackReplyText(args.text, { - interrupted: args.interrupted, - }); + const chunks = splitSlackReplyText(args.text); return chunks.map((chunk, index) => ({ text: chunk, ...(index === 0 && args.firstFiles ? { files: args.firstFiles } : {}), @@ -151,13 +144,11 @@ export function planSlackReplyPosts(args: { const { shouldPostThreadReply, attachFiles } = resolveReplyDelivery( args.reply, ); - const interrupted = isInterruptedVisibleReply(args.reply); const posts: PlannedSlackReplyPost[] = []; const textPosts = shouldPostThreadReply ? buildTextPosts({ text: args.reply.text, - interrupted, firstFiles: attachFiles === "inline" ? replyFiles : undefined, }) : []; diff --git a/packages/junior/src/chat/slack/resume.ts b/packages/junior/src/chat/slack/resume.ts index cfd845c2..274e364b 100644 --- a/packages/junior/src/chat/slack/resume.ts +++ b/packages/junior/src/chat/slack/resume.ts @@ -5,7 +5,16 @@ import { type AssistantReply, type ReplyRequestContext, } from "@/chat/respond"; +import { + buildTurnFailureResponse, + logException, + type LogContext, +} from "@/chat/logging"; import { isRetryableTurnError } from "@/chat/runtime/turn"; +import { + finalizeFailedTurnReply, + requireTurnFailureEventId, +} from "@/chat/services/turn-failure-response"; import { persistThreadStateById } from "@/chat/runtime/thread-state"; import { createSlackWebApiAssistantStatusSession, @@ -96,7 +105,6 @@ export interface ResumeSlackTurnArgs { replyContext?: ReplyRequestContext; lockKey?: string; initialText?: string; - failureText?: string; generateReply?: typeof generateAssistantReply; onSuccess?: (reply: AssistantReply) => Promise; onFailure?: (error: unknown) => Promise; @@ -109,6 +117,83 @@ function getDefaultLockKey(channelId: string, threadTs: string): string { return `slack:${channelId}:${threadTs}`; } +function getResumeLogContext( + args: ResumeSlackTurnArgs, + lockKey: string, +): LogContext { + return { + conversationId: args.replyContext?.correlation?.conversationId ?? lockKey, + slackThreadId: args.replyContext?.correlation?.threadId ?? lockKey, + slackUserId: + args.replyContext?.requester?.userId ?? + args.replyContext?.correlation?.requesterId, + slackUserName: args.replyContext?.requester?.userName, + slackChannelId: args.channelId, + runId: args.replyContext?.correlation?.runId, + assistantUserName: botConfig.userName, + modelId: botConfig.modelId, + }; +} + +async function postResumeFailureReply(args: { + channelId: string; + threadTs: string; + eventId: string; + logContext: LogContext; +}): Promise { + try { + await postSlackApiMessage({ + channelId: args.channelId, + threadTs: args.threadTs, + text: buildTurnFailureResponse(args.eventId), + }); + } catch (error) { + logException( + error, + "slack_resume_failure_reply_post_failed", + args.logContext, + { + "app.error.original_event_id": args.eventId, + }, + "Failed to post resumed turn failure reply", + ); + throw error; + } +} + +async function handleResumeFailure(args: { + body: string; + error: unknown; + eventName: string; + lockKey: string; + resumeArgs: ResumeSlackTurnArgs; +}): Promise { + const logContext = getResumeLogContext(args.resumeArgs, args.lockKey); + const capturedEventId = logException( + args.error, + args.eventName, + logContext, + {}, + args.body, + ); + await args.resumeArgs.onFailure?.(args.error); + const eventId = requireTurnFailureEventId(capturedEventId, args.eventName); + let postError: unknown; + try { + await postResumeFailureReply({ + channelId: args.resumeArgs.channelId, + threadTs: args.resumeArgs.threadTs, + eventId, + logContext, + }); + } catch (error) { + postError = error; + } + if (postError) { + throw postError; + } +} + function createResumeReplyContext( args: ResumeSlackTurnArgs, statusSession: AssistantStatusSession, @@ -199,7 +284,7 @@ export async function resumeSlackTurn(args: ResumeSlackTurnArgs) { ...replyContext, }); const replyTimeoutMs = resolveReplyTimeoutMs(args.replyTimeoutMs); - const reply = + let reply = typeof replyTimeoutMs === "number" ? await Promise.race([ replyPromise, @@ -216,6 +301,11 @@ export async function resumeSlackTurn(args: ResumeSlackTurnArgs) { ), ]) : await replyPromise; + reply = finalizeFailedTurnReply({ + reply, + logException, + context: getResumeLogContext(args, lockKey), + }); await status.stop(); const footer = buildSlackReplyFooter({ @@ -252,15 +342,13 @@ export async function resumeSlackTurn(args: ResumeSlackTurnArgs) { }; } else { deferredFailureHandler = async () => { - await args.onFailure?.(error); - - if (args.failureText) { - await postSlackMessageBestEffort( - args.channelId, - args.threadTs, - args.failureText, - ); - } + await handleResumeFailure({ + body: "Failed to resume Slack turn", + error, + eventName: "slack_resume_turn_failed", + lockKey, + resumeArgs: args, + }); }; } } finally { @@ -272,15 +360,13 @@ export async function resumeSlackTurn(args: ResumeSlackTurnArgs) { await deferredPauseHandler(); return; } catch (pauseError) { - await args.onFailure?.(pauseError); - - if (args.failureText) { - await postSlackMessageBestEffort( - args.channelId, - args.threadTs, - args.failureText, - ); - } + await handleResumeFailure({ + body: "Failed to handle resumed turn pause", + error: pauseError, + eventName: "slack_resume_pause_handler_failed", + lockKey, + resumeArgs: args, + }); return; } } @@ -296,7 +382,6 @@ export async function resumeAuthorizedRequest(args: { channelId: string; threadTs: string; connectedText: string; - failureText: string; replyContext?: ReplyRequestContext; lockKey?: string; generateReply?: typeof generateAssistantReply; @@ -313,7 +398,6 @@ export async function resumeAuthorizedRequest(args: { replyContext: args.replyContext, lockKey: args.lockKey, initialText: args.connectedText, - failureText: args.failureText, generateReply: args.generateReply, onSuccess: args.onSuccess, onFailure: args.onFailure, diff --git a/packages/junior/src/handlers/mcp-oauth-callback.ts b/packages/junior/src/handlers/mcp-oauth-callback.ts index 36d3c6f6..633609cb 100644 --- a/packages/junior/src/handlers/mcp-oauth-callback.ts +++ b/packages/junior/src/handlers/mcp-oauth-callback.ts @@ -231,8 +231,6 @@ async function resumeAuthorizedMcpTurn(args: { threadTs: authSession.threadTs, lockKey: authSession.conversationId, connectedText: "", - failureText: - "MCP authorization completed, but resuming the request failed. Please retry the original command.", replyContext: { requester: { userId: authSession.userId, @@ -285,14 +283,7 @@ async function resumeAuthorizedMcpTurn(args: { ); } }, - onFailure: async (error) => { - logException( - error, - "mcp_oauth_callback_resume_failed", - {}, - { "app.credential.provider": provider }, - "Failed to resume MCP-authorized turn", - ); + onFailure: async () => { try { await persistFailedReplyState( authSession.channelId!, diff --git a/packages/junior/src/handlers/oauth-callback.ts b/packages/junior/src/handlers/oauth-callback.ts index 1c08225d..aafb6dd7 100644 --- a/packages/junior/src/handlers/oauth-callback.ts +++ b/packages/junior/src/handlers/oauth-callback.ts @@ -22,7 +22,7 @@ import { resumeSlackTurn, } from "@/chat/slack/resume"; import { persistAuthPauseTurnState } from "@/chat/runtime/auth-pause-state"; -import { logException, logInfo } from "@/chat/logging"; +import { logInfo } from "@/chat/logging"; import { htmlCallbackResponse } from "@/handlers/oauth-html"; import { getChannelConfigurationServiceById, @@ -255,8 +255,6 @@ async function resumeCheckpointedOAuthTurn( threadTs: stored.threadTs, lockKey: stored.resumeConversationId, initialText: "", - failureText: - "I connected your account but hit an error processing your request. Please try the command again.", replyContext: { requester: { userId: userMessage.author.userId, @@ -304,14 +302,7 @@ async function resumeCheckpointedOAuthTurn( reply, }); }, - onFailure: async (error) => { - logException( - error, - "oauth_callback_resume_failed", - {}, - { "app.credential.provider": stored.provider }, - "Failed to auto-resume checkpointed turn after OAuth callback", - ); + onFailure: async () => { await persistFailedOAuthReplyState({ conversationId: stored.resumeConversationId!, sessionId: resolvedSessionId, @@ -370,7 +361,6 @@ async function resumePendingOAuthMessage( channelId: stored.channelId, threadTs: stored.threadTs, connectedText: "", - failureText: `I connected your account but hit an error processing your request. Please try \`${stored.pendingMessage}\` again.`, replyContext: { requester: { userId: stored.userId }, conversationContext, @@ -389,15 +379,6 @@ async function resumePendingOAuthMessage( "OAuth callback auto-resumed pending message finished replying", ); }, - onFailure: async (error) => { - logException( - error, - "oauth_callback_resume_failed", - {}, - { "app.credential.provider": stored.provider }, - "Failed to auto-resume pending message after OAuth callback", - ); - }, }); } diff --git a/packages/junior/src/handlers/turn-resume.ts b/packages/junior/src/handlers/turn-resume.ts index 506b70c9..7efd9c53 100644 --- a/packages/junior/src/handlers/turn-resume.ts +++ b/packages/junior/src/handlers/turn-resume.ts @@ -171,8 +171,6 @@ async function resumeTimedOutTurn( channelId: thread.channelId, threadTs: thread.threadTs, lockKey: payload.conversationId, - failureText: - "I hit an error while resuming that request. Please try the command again.", replyContext: { requester: { userId: userMessage.author.userId, @@ -221,20 +219,7 @@ async function resumeTimedOutTurn( ); } }, - onFailure: async (error) => { - logException( - error, - "timeout_resume_failed", - {}, - { - "app.ai.conversation_id": payload.conversationId, - "app.ai.session_id": payload.sessionId, - ...(isRetryableTurnError(error) - ? { "app.turn.retryable_reason": error.reason } - : {}), - }, - "Failed to resume timed-out turn", - ); + onFailure: async () => { await persistFailedReplyState(checkpoint); }, onAuthPause: async () => { diff --git a/packages/junior/tests/integration/mcp-oauth-callback-slack.test.ts b/packages/junior/tests/integration/mcp-oauth-callback-slack.test.ts index a96100f2..87a44bea 100644 --- a/packages/junior/tests/integration/mcp-oauth-callback-slack.test.ts +++ b/packages/junior/tests/integration/mcp-oauth-callback-slack.test.ts @@ -347,17 +347,6 @@ describe("mcp oauth callback slack integration", () => { }), ]), ); - expect(getCapturedSlackApiCalls("chat.postMessage")).not.toEqual( - expect.arrayContaining([ - expect.objectContaining({ - params: expect.objectContaining({ - channel: "C123", - thread_ts: "1700000000.001", - text: "I couldn't complete this request in this turn due to an execution failure. I've logged the details for debugging.", - }), - }), - ]), - ); }); it("does not resume a stale MCP-blocked request after a newer thread message", async () => { diff --git a/packages/junior/tests/integration/oauth-resume-slack.test.ts b/packages/junior/tests/integration/oauth-resume-slack.test.ts index 9840cbd8..600e0002 100644 --- a/packages/junior/tests/integration/oauth-resume-slack.test.ts +++ b/packages/junior/tests/integration/oauth-resume-slack.test.ts @@ -12,7 +12,7 @@ import { } from "../msw/handlers/slack-api"; function makeDiagnostics( - outcome: "success" | "provider_error" = "success", + outcome: "success" | "execution_failure" | "provider_error" = "success", extras: Record = {}, ) { return { @@ -47,8 +47,6 @@ describe("oauth resume slack integration", () => { threadTs: "1700000000.001", connectedText: "Your eval-auth MCP access is now connected. Continuing the original request...", - failureText: - "MCP authorization completed, but resuming the request failed. Please retry the original command.", replyContext: { requester: { userId: "U123" }, }, @@ -137,7 +135,6 @@ describe("oauth resume slack integration", () => { channelId: "C123", threadTs: "1700000000.002", connectedText: "Connected. Continuing...", - failureText: "Resume failed.", replyContext: { requester: { userId: "U123" }, }, @@ -164,7 +161,7 @@ describe("oauth resume slack integration", () => { expect(postCalls[4]?.params.text).toContain("line 80"); }); - it("marks interrupted resumed replies explicitly", async () => { + it("replaces resumed provider-error replies with the canonical event-id response", async () => { const { resumeAuthorizedRequest } = await import("@/chat/slack/resume"); await resumeAuthorizedRequest({ @@ -172,7 +169,6 @@ describe("oauth resume slack integration", () => { channelId: "C123", threadTs: "1700000000.003", connectedText: "Connected. Continuing...", - failureText: "Resume failed.", replyContext: { requester: { userId: "U123" }, }, @@ -188,8 +184,46 @@ describe("oauth resume slack integration", () => { expect(postCalls[1]?.params).toMatchObject({ channel: "C123", thread_ts: "1700000000.003", - text: `Partial output${getSlackInterruptionMarker()}`, }); + expect(postCalls[1]?.params.text).toContain( + "I ran into an internal error while processing that. Reference: `event_id=", + ); + expect(postCalls[1]?.params.text).not.toContain("Partial output"); + expect(postCalls[1]?.params.text).not.toContain( + getSlackInterruptionMarker().trim(), + ); + }); + + it("replaces resumed execution-failure replies before Slack planning", async () => { + const { resumeAuthorizedRequest } = await import("@/chat/slack/resume"); + + await resumeAuthorizedRequest({ + messageText: "Continue the original request", + channelId: "C123", + threadTs: "1700000000.006", + connectedText: "Connected. Continuing...", + replyContext: { + requester: { userId: "U123" }, + }, + generateReply: async () => + ({ + text: "", + diagnostics: makeDiagnostics("execution_failure", { + assistantMessageCount: 0, + usedPrimaryText: false, + }), + }) as any, + }); + + const postCalls = getCapturedSlackApiCalls("chat.postMessage"); + expect(postCalls).toHaveLength(2); + expect(postCalls[1]?.params).toMatchObject({ + channel: "C123", + thread_ts: "1700000000.006", + }); + expect(postCalls[1]?.params.text).toContain( + "I ran into an internal error while processing that. Reference: `event_id=", + ); }); it("delivers resumed reply files through the shared reply planner", async () => { @@ -200,7 +234,6 @@ describe("oauth resume slack integration", () => { channelId: "C123", threadTs: "1700000000.004", connectedText: "Connected. Continuing...", - failureText: "Resume failed.", replyContext: { requester: { userId: "U123" }, }, @@ -254,7 +287,6 @@ describe("oauth resume slack integration", () => { channelId: "C123", threadTs: "1700000000.005", connectedText: "Connected. Continuing...", - failureText: "Resume failed.", replyContext: { requester: { userId: "U123" }, }, diff --git a/packages/junior/tests/integration/slack/bot-handlers.test.ts b/packages/junior/tests/integration/slack/bot-handlers.test.ts index a0c9635f..2a0c2e8f 100644 --- a/packages/junior/tests/integration/slack/bot-handlers.test.ts +++ b/packages/junior/tests/integration/slack/bot-handlers.test.ts @@ -1,6 +1,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import type { JuniorRuntimeServiceOverrides } from "@/chat/app/services"; import { makeAssistantStatus } from "@/chat/slack/assistant-thread/status"; +import { getSlackInterruptionMarker } from "@/chat/slack/output"; import { RetryableTurnError } from "@/chat/runtime/turn"; import { FakeSlackAdapter, @@ -484,12 +485,13 @@ describe("bot handlers (integration)", () => { ); expect(thread.posts).toHaveLength(1); - expect(thread.posts[0]).toEqual( - expect.objectContaining({ - markdown: - "Partial output...\n\n[Response interrupted before completion]", - }), - ); + const postText = + typeof thread.posts[0] === "string" + ? thread.posts[0] + : ((thread.posts[0] as { markdown?: string }).markdown ?? ""); + expect(postText).toContain("I ran into an internal error"); + expect(postText).toContain("event_id="); + expect(postText).not.toContain(getSlackInterruptionMarker().trim()); }); it("emits assistant status updates in shared channel threads", async () => { diff --git a/packages/junior/tests/integration/slack/finalized-reply-behavior.test.ts b/packages/junior/tests/integration/slack/finalized-reply-behavior.test.ts index ffffc473..b45b618c 100644 --- a/packages/junior/tests/integration/slack/finalized-reply-behavior.test.ts +++ b/packages/junior/tests/integration/slack/finalized-reply-behavior.test.ts @@ -301,7 +301,7 @@ describe("Slack behavior: finalized thread replies", () => { expect(secondPost.startsWith("```ts\n")).toBe(true); }); - it("adds an interruption marker to provider-error replies", async () => { + it("replaces provider-error replies with the canonical event-id response", async () => { const longReply = `${"A".repeat(slackOutputPolicy.maxInlineChars)}\n\n` + "This should continue into a second post."; @@ -328,8 +328,12 @@ describe("Slack behavior: finalized thread replies", () => { ); expect(thread.postKinds.every((kind) => kind === "value")).toBe(true); - expect(toPostedText(thread.posts.at(-1))).toContain( - getSlackInterruptionMarker().trim(), + expect(thread.posts).toHaveLength(1); + const postedText = toPostedText(thread.posts[0]); + expect(postedText).toContain( + "I ran into an internal error while processing that. Reference: `event_id=", ); + expect(postedText).not.toContain(longReply); + expect(postedText).not.toContain(getSlackInterruptionMarker().trim()); }); }); diff --git a/packages/junior/tests/integration/turn-resume-slack.test.ts b/packages/junior/tests/integration/turn-resume-slack.test.ts index b8a30940..c1e7ba4f 100644 --- a/packages/junior/tests/integration/turn-resume-slack.test.ts +++ b/packages/junior/tests/integration/turn-resume-slack.test.ts @@ -336,7 +336,9 @@ describe("turn resume slack integration", () => { params: expect.objectContaining({ channel: "C123", thread_ts: "1712345.0002", - text: "I hit an error while resuming that request. Please try the command again.", + text: expect.stringContaining( + "I ran into an internal error while processing that. Reference: `event_id=", + ), }), }), ]); diff --git a/packages/junior/tests/unit/handlers/oauth-resume.test.ts b/packages/junior/tests/unit/handlers/oauth-resume.test.ts index 727ce225..cefd153f 100644 --- a/packages/junior/tests/unit/handlers/oauth-resume.test.ts +++ b/packages/junior/tests/unit/handlers/oauth-resume.test.ts @@ -2,7 +2,8 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { RetryableTurnError } from "@/chat/runtime/turn"; import { disconnectStateAdapter, getStateAdapter } from "@/chat/state/adapter"; -const { postMessageMock, setStatusMock } = vi.hoisted(() => ({ +const { logExceptionMock, postMessageMock, setStatusMock } = vi.hoisted(() => ({ + logExceptionMock: vi.fn(), postMessageMock: vi.fn(), setStatusMock: vi.fn(), })); @@ -44,11 +45,21 @@ vi.mock("@/chat/slack/client", () => ({ }), })); +vi.mock("@/chat/logging", async (importOriginal) => { + const original = await importOriginal(); + return { + ...original, + logException: logExceptionMock, + }; +}); + import { resumeAuthorizedRequest, resumeSlackTurn } from "@/chat/slack/resume"; describe("resumeAuthorizedRequest", () => { beforeEach(async () => { vi.useFakeTimers(); + logExceptionMock.mockReset(); + logExceptionMock.mockReturnValue("evt_test"); postMessageMock.mockReset(); setStatusMock.mockReset(); postMessageMock.mockResolvedValue({ ts: "1700000000.100" }); @@ -69,7 +80,6 @@ describe("resumeAuthorizedRequest", () => { channelId: "C-test", threadTs: "1700000000.0001", connectedText: "connected", - failureText: "resume failed", replyContext: { requester: { userId: "U-test" }, }, @@ -82,6 +92,55 @@ describe("resumeAuthorizedRequest", () => { await resumePromise; expect(onFailure).toHaveBeenCalledTimes(1); + expect(postMessageMock).toHaveBeenLastCalledWith( + expect.objectContaining({ + channel: "C-test", + thread_ts: "1700000000.0001", + text: expect.stringContaining( + "I ran into an internal error while processing that. Reference: `event_id=", + ), + }), + ); + }); + + it("persists failure state before requiring a Sentry event ID", async () => { + const onFailure = vi.fn(async () => undefined); + logExceptionMock.mockReturnValueOnce(undefined); + + await expect( + resumeAuthorizedRequest({ + messageText: "tell me the saved deadline", + channelId: "C-test", + threadTs: "1700000000.0004", + connectedText: "connected", + replyContext: { + requester: { userId: "U-test" }, + }, + generateReply: async () => { + throw new Error("resume failed"); + }, + onFailure, + }), + ).rejects.toThrow( + "Sentry did not return an event ID for slack_resume_turn_failed", + ); + + expect(onFailure).toHaveBeenCalledTimes(1); + expect(postMessageMock).toHaveBeenCalledTimes(1); + expect(postMessageMock).toHaveBeenCalledWith( + expect.objectContaining({ + channel: "C-test", + thread_ts: "1700000000.0004", + text: "connected", + }), + ); + expect(postMessageMock).not.toHaveBeenCalledWith( + expect.objectContaining({ + channel: "C-test", + thread_ts: "1700000000.0004", + text: expect.stringContaining("event_id=unknown"), + }), + ); }); it("releases the thread lock before scheduling another timeout slice", async () => { @@ -119,14 +178,13 @@ describe("resumeAuthorizedRequest", () => { expect(onTimeoutPause).toHaveBeenCalledTimes(1); }); - it("falls back to normal failure handling when timeout pause handling throws", async () => { + it("posts the canonical failure response when timeout pause handling throws", async () => { const onFailure = vi.fn(async () => undefined); await resumeSlackTurn({ messageText: "continue this turn", channelId: "C-test", threadTs: "1700000000.0003", - failureText: "resume failed", replyContext: { requester: { userId: "U-test" }, }, @@ -145,5 +203,14 @@ describe("resumeAuthorizedRequest", () => { }); expect(onFailure).toHaveBeenCalledTimes(1); + expect(postMessageMock).toHaveBeenCalledWith( + expect.objectContaining({ + channel: "C-test", + thread_ts: "1700000000.0003", + text: expect.stringContaining( + "I ran into an internal error while processing that. Reference: `event_id=", + ), + }), + ); }); }); diff --git a/packages/junior/tests/unit/handlers/turn-resume.test.ts b/packages/junior/tests/unit/handlers/turn-resume.test.ts index 1fa09220..1d1ddc00 100644 --- a/packages/junior/tests/unit/handlers/turn-resume.test.ts +++ b/packages/junior/tests/unit/handlers/turn-resume.test.ts @@ -272,13 +272,6 @@ describe("turn resume handler", () => { await args.onSuccess?.(reply); } catch (error) { await args.onFailure?.(error); - if (args.failureText) { - await postSlackMessageMock( - args.channelId, - args.threadTs, - args.failureText, - ); - } } }); @@ -377,13 +370,6 @@ describe("turn resume handler", () => { ); } catch (error) { await args.onFailure?.(error); - if (args.failureText) { - await postSlackMessageMock( - args.channelId, - args.threadTs, - args.failureText, - ); - } } }); diff --git a/packages/junior/tests/unit/runtime/respond-mcp-progressive-loading.test.ts b/packages/junior/tests/unit/runtime/respond-mcp-progressive-loading.test.ts index 4c062294..0ffe9c25 100644 --- a/packages/junior/tests/unit/runtime/respond-mcp-progressive-loading.test.ts +++ b/packages/junior/tests/unit/runtime/respond-mcp-progressive-loading.test.ts @@ -861,9 +861,7 @@ describe("generateAssistantReply progressive MCP loading", () => { }), ); - expect(reply.text).toBe( - "I couldn't complete this request in this turn due to an execution failure. I've logged the details for debugging.", - ); + expect(reply.text).toBe(""); expect(reply.diagnostics.outcome).toBe("execution_failure"); expect(reply.diagnostics.usedPrimaryText).toBe(false); }); diff --git a/packages/junior/tests/unit/runtime/slack-runtime.test.ts b/packages/junior/tests/unit/runtime/slack-runtime.test.ts index a9d9d057..27e1eecf 100644 --- a/packages/junior/tests/unit/runtime/slack-runtime.test.ts +++ b/packages/junior/tests/unit/runtime/slack-runtime.test.ts @@ -37,7 +37,6 @@ describe("createSlackTurnRuntime", () => { assistantUserName: "junior", decideSubscribedReply, getChannelId: () => "C123", - getErrorReference: () => null, getPreparedConversationContext: () => "prior thread context", getRunId: () => "run_123", getThreadId: () => "thread_123", diff --git a/packages/junior/tests/unit/slack/slack-runtime.test.ts b/packages/junior/tests/unit/slack/slack-runtime.test.ts index 2e648071..7e1efefe 100644 --- a/packages/junior/tests/unit/slack/slack-runtime.test.ts +++ b/packages/junior/tests/unit/slack/slack-runtime.test.ts @@ -22,13 +22,12 @@ function createMockDeps( assistantUserName: "test-bot", modelId: "test-model", now: () => 1700000000000, - getErrorReference: () => null, getChannelId: (_thread, message) => message.threadId?.split(":")[1], getThreadId: (_thread, message) => message.threadId, getRunId: () => undefined, initializeAssistantThread: vi.fn().mockResolvedValue(undefined), refreshAssistantThreadContext: vi.fn().mockResolvedValue(undefined), - logException: vi.fn(), + logException: vi.fn(() => "evt_test"), logWarn: vi.fn(), onSubscribedMessageSkipped: vi.fn().mockResolvedValue(undefined), recordSkippedSubscribedMessage: vi.fn().mockResolvedValue(undefined), @@ -77,7 +76,7 @@ describe("createSlackTurnRuntime", () => { await runtime.handleNewMention(thread, message); expect(thread.posts).toContain( - "I ran into an internal error while processing that. Please try again.", + "I ran into an internal error while processing that. Reference: `event_id=evt_test`.", ); }); @@ -97,7 +96,7 @@ describe("createSlackTurnRuntime", () => { await runtime.handleNewMention(thread, message); expect(thread.posts).toContain( - "I ran into an internal error while processing that. Please try again.", + "I ran into an internal error while processing that. Reference: `event_id=evt_test`.", ); }); @@ -107,10 +106,6 @@ describe("createSlackTurnRuntime", () => { replyToThread: vi.fn().mockRejectedValue(replyError), withSpan: vi.fn(async (_n, _o, _c, cb) => cb()), logException: vi.fn(() => "evt_123"), - getErrorReference: () => ({ - eventId: "evt_123", - traceId: "trace_ignored", - }), }); const runtime = createSlackTurnRuntime(deps); const thread = createTestThread({}); @@ -119,27 +114,25 @@ describe("createSlackTurnRuntime", () => { await runtime.handleNewMention(thread, message); expect(thread.posts).toContain( - "I ran into an internal error while processing that. Reference: `event_id=evt_123 trace_id=trace_ignored`.", + "I ran into an internal error while processing that. Reference: `event_id=evt_123`.", ); }); - it("falls back to trace id when sentry event id is unavailable", async () => { + it("fails closed when sentry capture returns no event id", async () => { const replyError = new Error("reply failed"); const deps = createMockDeps({ replyToThread: vi.fn().mockRejectedValue(replyError), withSpan: vi.fn(async (_n, _o, _c, cb) => cb()), logException: vi.fn(() => undefined), - getErrorReference: () => ({ traceId: "trace_123" }), }); const runtime = createSlackTurnRuntime(deps); const thread = createTestThread({}); const message = createTestMessage({}); - await runtime.handleNewMention(thread, message); - - expect(thread.posts).toContain( - "I ran into an internal error while processing that. Reference: `trace_id=trace_123`.", + await expect(runtime.handleNewMention(thread, message)).rejects.toThrow( + "Sentry did not return an event ID for mention_handler_failed", ); + expect(thread.posts).toHaveLength(0); }); }); @@ -384,7 +377,7 @@ describe("createSlackTurnRuntime", () => { await runtime.handleSubscribedMessage(thread, message); expect(thread.posts).toContain( - "I ran into an internal error while processing that. Please try again.", + "I ran into an internal error while processing that. Reference: `event_id=evt_test`.", ); }); }); diff --git a/packages/junior/tests/unit/turn-result.test.ts b/packages/junior/tests/unit/turn-result.test.ts index a523bc60..681ce6c6 100644 --- a/packages/junior/tests/unit/turn-result.test.ts +++ b/packages/junior/tests/unit/turn-result.test.ts @@ -38,9 +38,7 @@ describe("buildTurnResult", () => { thinkingSelection, }); - expect(reply.text).toBe( - "I couldn't complete this request in this turn due to an execution failure. I've logged the details for debugging.", - ); + expect(reply.text).toBe(""); expect(reply.diagnostics.outcome).toBe("execution_failure"); }); @@ -73,9 +71,7 @@ describe("buildTurnResult", () => { thinkingSelection, }); - expect(reply.text).toBe( - "I couldn't complete this request in this turn due to an execution failure. I've logged the details for debugging.", - ); + expect(reply.text).toBe(""); expect(reply.diagnostics.outcome).toBe("execution_failure"); expect(reply.diagnostics.usedPrimaryText).toBe(false); }); @@ -142,6 +138,35 @@ describe("buildTurnResult", () => { expect(reply.diagnostics.usedPrimaryText).toBe(false); }); + it("suppresses empty thread text when a channel post is the successful side effect", () => { + const reply = buildTurnResult({ + newMessages: [ + { + role: "toolResult", + toolName: "slackChannelPostMessage", + isError: false, + content: [{ type: "text", text: "message posted" }], + }, + ], + userInput: "share the update", + replyFiles: [], + artifactStatePatch: {}, + toolCalls: ["slackChannelPostMessage"], + generatedFileCount: 0, + shouldTrace: false, + spanContext: {}, + thinkingSelection, + }); + + expect(reply.text).toBe(""); + expect(reply.deliveryPlan).toMatchObject({ + mode: "thread", + postThreadText: false, + }); + expect(reply.diagnostics.outcome).toBe("success"); + expect(reply.diagnostics.usedPrimaryText).toBe(false); + }); + it("keeps thread text when a turn adds a reaction and returns real text", () => { const reply = buildTurnResult({ newMessages: [ @@ -208,6 +233,48 @@ describe("buildTurnResult", () => { expect(reply.diagnostics.usedPrimaryText).toBe(true); }); + it("keeps thread delivery enabled for reaction turns that fail validation", () => { + const reply = buildTurnResult({ + newMessages: [ + { + role: "toolResult", + toolName: "slackMessageAddReaction", + isError: false, + content: [{ type: "text", text: "reaction added" }], + }, + { + role: "assistant", + content: [ + { + type: "text", + text: JSON.stringify({ + type: "tool_call", + name: "slackMessageAddReaction", + input: { reaction: "thumbsup" }, + }), + }, + ], + stopReason: "stop", + }, + ], + userInput: "react and tell me what happened", + replyFiles: [], + artifactStatePatch: {}, + toolCalls: ["slackMessageAddReaction"], + generatedFileCount: 0, + shouldTrace: false, + spanContext: {}, + thinkingSelection, + }); + + expect(reply.text).toBe(""); + expect(reply.deliveryPlan).toMatchObject({ + postThreadText: true, + }); + expect(reply.diagnostics.outcome).toBe("execution_failure"); + expect(reply.diagnostics.usedPrimaryText).toBe(true); + }); + it("keeps post-canvas thread replies brief", () => { const verboseReply = [ "I put together a reusable reference here:",