diff --git a/client/src/hooks/Input/useAskAnswerMode.ts b/client/src/hooks/Input/useAskAnswerMode.ts index aaed8494f1..681c0ad2bd 100644 --- a/client/src/hooks/Input/useAskAnswerMode.ts +++ b/client/src/hooks/Input/useAskAnswerMode.ts @@ -94,8 +94,14 @@ export default function useAskAnswerMode(conversationId?: string | null) { * suppressing it behind an open popover would strand the user at a locked * card with no explanation. (`error`, unlike `expired`, stays active: it is * retryable — see the composer-preserving submit path.) + * + * A `submitted` question is likewise done — the run has resumed and there is + * nothing left to answer. The chat card already self-hides on it; without the + * same test here a card that outlives its strip (a resurrected copy, or a + * submit whose store write couldn't run) holds the popover open over an + * answered question with every option greyed out. */ - const active = liveAsk != null && !dismissed && status !== 'expired'; + const active = liveAsk != null && !dismissed && status !== 'expired' && status !== 'submitted'; const collapsed = active && collapsedIds.includes(liveAsk.actionId); /** The popover renders only while expanded; collapse keeps `active` (and the * composer's answer role) but hands the question display to the chat card. */ diff --git a/client/src/hooks/SSE/__tests__/useStepHandler.spec.ts b/client/src/hooks/SSE/__tests__/useStepHandler.spec.ts index f9c438c03c..fe773109b1 100644 --- a/client/src/hooks/SSE/__tests__/useStepHandler.spec.ts +++ b/client/src/hooks/SSE/__tests__/useStepHandler.spec.ts @@ -18,6 +18,7 @@ import type { Agents, } from 'librechat-data-provider'; import { subagentProgressByToolCallId } from '~/store/subagents'; +import { resolveAskUserQuestionPart } from '~/utils/approval'; import useStepHandler from '~/hooks/SSE/useStepHandler'; /** `Constants` is a heterogeneous enum (`string | number`); annotate as @@ -1899,6 +1900,117 @@ describe('useStepHandler', () => { consoleSpy.mockRestore(); }); + it('drops an answered ask card when the resumed segment streams AROUND its slot', () => { + /** + * The first event after a resume re-renders the ask tool_call at ITS OWN + * index, never the card's, so the slot displacement above cannot fire. + * This handler's cached copy still holds the card the answer-submit + * stripped from the store; writing it back reopened the popover over an + * answered question with its options locked. + */ + const askPart = { + type: 'ask_user_question', + ask_user_question: { actionId: 'a2', question: { question: 'Which env?' } }, + } as unknown as TMessageContentParts; + const askToolCall = { + type: ContentTypes.TOOL_CALL, + tool_call: { + id: 'ask-call-1', + name: 'ask_user_question', + args: '{"question":"Which env?"}', + type: ToolCallTypes.TOOL_CALL, + }, + } as unknown as TMessageContentParts; + /** Card appended at the END (index 2), ask tool_call at index 1. */ + const paused = createResponseMessage({ + content: [{ type: ContentTypes.TEXT, text: 'Let me ask.' }, askToolCall, askPart], + }); + + const { result } = renderHook(() => useStepHandler(createHookParams())); + act(() => { + result.current.syncStepMessage(paused); + }); + + /** The user answers: the store copy is stripped + stamped, and the action + * is recorded as answered. The handler's cached copy is untouched. */ + const answered = resolveAskUserQuestionPart(paused, 'a2', 'prod'); + mockGetMessages.mockReturnValue([answered]); + + /** Resume replays the ask tool_call's completion at index 1, not 2. */ + const askCompletion = createToolCallRunStep({ + id: 'step-ask', + index: 1, + stepDetails: { + type: StepTypes.TOOL_CALLS, + tool_calls: [ + { + id: 'ask-call-1', + name: 'ask_user_question', + args: '{"question":"Which env?"}', + output: 'prod', + type: ToolCallTypes.TOOL_CALL, + }, + ], + }, + } as Partial); + + act(() => { + result.current.stepHandler( + { event: StepEvents.ON_RUN_STEP, data: askCompletion }, + createSubmission(), + ); + }); + + const lastCall = mockSetMessages.mock.calls[mockSetMessages.mock.calls.length - 1]; + const updated = (lastCall[0] as TMessage[]).find((m) => m.messageId === 'response-msg-1'); + const content = (updated?.content ?? []) as Array<{ type?: string }>; + expect(content.some((part) => part?.type === 'ask_user_question')).toBe(false); + /** The real content is untouched: only the answered card is dropped. */ + expect(content[1]).toMatchObject({ type: ContentTypes.TOOL_CALL }); + }); + + it('keeps a still-live ask card when an event streams around its slot', () => { + /** Only ANSWERED cards are droppable — a late event racing a live pause + * must not take the question away before the user can respond. */ + const askPart = { + type: 'ask_user_question', + ask_user_question: { actionId: 'a3', question: { question: 'Live?' } }, + } as unknown as TMessageContentParts; + const responseMessage = createResponseMessage({ + content: [{ type: ContentTypes.TEXT, text: 'Pre-pause' }, askPart], + }); + mockGetMessages.mockReturnValue([responseMessage]); + + const { result } = renderHook(() => useStepHandler(createHookParams())); + act(() => { + result.current.syncStepMessage(responseMessage); + }); + + act(() => { + result.current.stepHandler( + { event: StepEvents.ON_RUN_STEP, data: createRunStep({ index: 0 }) }, + createSubmission(), + ); + }); + act(() => { + result.current.stepHandler( + { + event: StepEvents.ON_MESSAGE_DELTA, + data: { + id: 'step-1', + delta: { content: [{ type: ContentTypes.TEXT, text: ' more' }] }, + } as Agents.MessageDeltaEvent, + }, + createSubmission(), + ); + }); + + const lastCall = mockSetMessages.mock.calls[mockSetMessages.mock.calls.length - 1]; + const updated = (lastCall[0] as TMessage[]).find((m) => m.messageId === 'response-msg-1'); + const content = (updated?.content ?? []) as Array<{ type?: string }>; + expect(content.some((part) => part?.type === 'ask_user_question')).toBe(true); + }); + it('should warn on content type mismatch and not overwrite', () => { const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(); diff --git a/client/src/hooks/SSE/useStepHandler.ts b/client/src/hooks/SSE/useStepHandler.ts index 68106652db..b6e97c6743 100644 --- a/client/src/hooks/SSE/useStepHandler.ts +++ b/client/src/hooks/SSE/useStepHandler.ts @@ -27,8 +27,8 @@ import { initSubagentAggregatorState, initSubagentTickerState, } from '~/utils/subagentContent'; +import { isAskUserQuestionPart, isAnsweredAskUserQuestionPart } from '~/utils/approval'; import { subagentProgressByToolCallId, sandboxStartingByToolCallId } from '~/store'; -import { isAskUserQuestionPart } from '~/utils/approval'; import { MESSAGE_UPDATE_INTERVAL } from '~/common'; type TUseStepHandler = { @@ -388,6 +388,17 @@ export default function useStepHandler({ */ if (isAskUserQuestionPart(updatedContent[index])) { updatedContent = updatedContent.filter((part) => !isAskUserQuestionPart(part)); + } else if (updatedContent.some(isAnsweredAskUserQuestionPart)) { + /** + * An ALREADY-ANSWERED card the resumed segment streams around rather than + * into: the first event after the resume re-renders the ask tool_call at + * ITS OWN index, so the slot test above never fires and this handler's + * cached copy — which still holds the card the answer-submit stripped from + * the store — gets written back, reopening the popover with its options + * locked. Only cards the user actually answered are dropped, so an event + * racing a still-live pause can't take its card down. + */ + updatedContent = updatedContent.filter((part) => !isAnsweredAskUserQuestionPart(part)); } if (!updatedContent[index] && contentType !== ContentTypes.TOOL_CALL) { diff --git a/client/src/utils/approval.spec.ts b/client/src/utils/approval.spec.ts index 5b9a3dcc6c..0f2de67d69 100644 --- a/client/src/utils/approval.spec.ts +++ b/client/src/utils/approval.spec.ts @@ -11,6 +11,7 @@ import { resolveAskUserQuestionPart, getSubmittedAskAnswer, findLiveAskUserQuestion, + isAnsweredAskUserQuestionPart, splitOtherOption, } from './approval'; @@ -391,6 +392,52 @@ describe('findLiveAskUserQuestion', () => { expect(findLiveAskUserQuestion(null)).toBeNull(); expect(findLiveAskUserQuestion(undefined)).toBeNull(); }); + + /** + * The strip on answer-submit is a store write, so any holder of an older copy + * of the message (the SSE step handler's in-flight cache, a replayed event) + * can put the card back. Honouring a resurrected card reopened the popover + * over a question the user had already answered, options greyed out. + */ + it('ignores a resurrected card for an answered question', () => { + const paused = applyPendingAction(msg({ content: [] }), askAction({ actionId: 'a-answered' })); + expect(findLiveAskUserQuestion([paused])?.actionId).toBe('a-answered'); + + resolveAskUserQuestionPart(paused, 'a-answered', 'Ada'); + + // `paused` is the pre-answer copy — exactly what a stale cache writes back. + expect(findLiveAskUserQuestion([paused])).toBeNull(); + }); + + it('falls back to an older live question when the newest is answered', () => { + const older = applyPendingAction(msg({ content: [] }), askAction({ actionId: 'a-live' })); + const newer = applyPendingAction( + { ...msg({ content: [] }), messageId: 'm2' }, + askAction({ actionId: 'a-done' }), + ); + resolveAskUserQuestionPart(newer, 'a-done', 'Ada'); + + expect(findLiveAskUserQuestion([older, newer])?.actionId).toBe('a-live'); + }); +}); + +describe('isAnsweredAskUserQuestionPart', () => { + it('marks only cards whose question was actually answered', () => { + const live = applyPendingAction(msg({ content: [] }), askAction({ actionId: 'a-open' })); + const answered = applyPendingAction(msg({ content: [] }), askAction({ actionId: 'a-closed' })); + resolveAskUserQuestionPart(answered, 'a-closed', 'Ada'); + + expect(isAnsweredAskUserQuestionPart(answered.content?.[0])).toBe(true); + expect(isAnsweredAskUserQuestionPart(live.content?.[0])).toBe(false); + expect(isAnsweredAskUserQuestionPart(textPart('hi'))).toBe(false); + expect(isAnsweredAskUserQuestionPart(undefined)).toBe(false); + }); + + it('stays false for a question whose resolve found no matching card', () => { + const paused = applyPendingAction(msg({ content: [] }), askAction({ actionId: 'a-other' })); + resolveAskUserQuestionPart(paused, 'a-missing', 'Ada'); + expect(isAnsweredAskUserQuestionPart(paused.content?.[0])).toBe(false); + }); }); describe('applyPendingAction — unsupported type', () => { diff --git a/client/src/utils/approval.ts b/client/src/utils/approval.ts index 603470a5c0..eb99abda06 100644 --- a/client/src/utils/approval.ts +++ b/client/src/utils/approval.ts @@ -272,11 +272,28 @@ export function removeAskUserQuestionPart(message: TMessage, actionId: string): */ const submittedAskAnswers = new Map(); +/** + * Ask actions the user has answered this session. Same rationale as + * {@link submittedAskAnswers}: the SSE step handler evolves its own cached copy + * of the streaming message, so the store-level strip below can't reach it — + * writing that copy back would resurrect an answered card. Keyed by `actionId` + * and only ever added to by {@link resolveAskUserQuestionPart}, so a step event + * racing a still-LIVE pause can never mistake its card for an answered one. + */ +const answeredAskActionIds = new Set(); + /** The locally-submitted answer for an ask tool_call, if any. */ export function getSubmittedAskAnswer(toolCallId: string | undefined): string | undefined { return toolCallId ? submittedAskAnswers.get(toolCallId) : undefined; } +/** Whether `part` is an ask card whose question the user already answered. */ +export const isAnsweredAskUserQuestionPart = ( + part: Partial | undefined, +): boolean => + isAskUserQuestionPart(part) && + answeredAskActionIds.has((part as unknown as AskUserQuestionPart)[ASK_USER_QUESTION].actionId); + /** * Resolve an answered ask-user-question pause on the client, mirroring the * server's resume-time stamp so the durable Q&A card shows the answer the @@ -303,6 +320,7 @@ export function resolveAskUserQuestionPart( if (!syntheticPart) { return message; } + answeredAskActionIds.add(actionId); let patched = false; const nextContent: TMessageContentParts[] = []; @@ -381,6 +399,11 @@ export function splitOtherOption(options: Agents.AskUserQuestionOption[] | undef * messages — the newest synthetic part wins. Drives the composer popover: the * part exists exactly while a pause is live (applied on `on_pending_action`, * stripped when the answer submits), so its presence IS the popover signal. + * + * Answered cards are skipped rather than assumed absent: the strip is a store + * write, and any holder of an older message copy (the SSE step handler's + * in-flight cache, a replayed event) can put one back. Honouring it would + * reopen the popover on a question the user already answered. */ export function findLiveAskUserQuestion( messages: TMessage[] | null | undefined, @@ -396,7 +419,7 @@ export function findLiveAskUserQuestion( } for (let j = content.length - 1; j >= 0; j--) { const part = content[j]; - if (isAskUserQuestionPart(part)) { + if (isAskUserQuestionPart(part) && !isAnsweredAskUserQuestionPart(part)) { const ask = (part as unknown as AskUserQuestionPart)[ASK_USER_QUESTION]; return { actionId: ask.actionId, question: ask.question, messageId: message.messageId }; }