diff --git a/apps/web/app/needs-you-inbox/needs-you-inbox-page-client.test.tsx b/apps/web/app/needs-you-inbox/needs-you-inbox-page-client.test.tsx index 0261755b64..75318915cb 100644 --- a/apps/web/app/needs-you-inbox/needs-you-inbox-page-client.test.tsx +++ b/apps/web/app/needs-you-inbox/needs-you-inbox-page-client.test.tsx @@ -112,6 +112,10 @@ vi.mock("@/hooks/domains/failed-inbox/use-failed-inbox-controller", () => ({ useFailedInboxController: (...args: unknown[]) => mocks.useFailedInboxController(...args), })); +vi.mock("@/hooks/use-late-clarification-message", () => ({ + useLateClarificationMessage: () => ({ send: vi.fn() }), +})); + import { NeedsYouInboxPageClient } from "./needs-you-inbox-page-client"; function bundle(id: string): ClarificationInboxBundle { diff --git a/apps/web/app/needs-you-inbox/needs-you-inbox-page-client.tsx b/apps/web/app/needs-you-inbox/needs-you-inbox-page-client.tsx index 79acf66b27..0f05e126b7 100644 --- a/apps/web/app/needs-you-inbox/needs-you-inbox-page-client.tsx +++ b/apps/web/app/needs-you-inbox/needs-you-inbox-page-client.tsx @@ -43,6 +43,7 @@ import { import { InboxHistoryList } from "@/components/inbox-history/inbox-history-list"; import { InboxHistoryEmptyState } from "@/components/inbox-history/inbox-history-empty-state"; import { InboxHistoryErrorState } from "@/components/inbox-history/inbox-history-error-state"; +import { useLateClarificationMessage } from "@/hooks/use-late-clarification-message"; type ViewMode = "error" | "loading" | "empty" | "list"; @@ -87,7 +88,7 @@ function NeedsYouInboxList({ <>
{bundles.map((bundle) => ( - + ))}
{hasMore && ( @@ -102,6 +103,17 @@ function NeedsYouInboxList({ ); } +function NeedsYouInboxRowWithLateAnswer({ bundle }: { bundle: ClarificationInboxBundle }) { + const lateAnswer = useLateClarificationMessage(bundle.messages[0]); + return ( + + ); +} + // "Needs you" is the tab strip's default-selected tab, and this content, // count and behavior are unchanged by the strip's presence. function NeedsYouInboxTabContent({ retry }: { retry: () => void }) { diff --git a/apps/web/components/needs-you-inbox/needs-you-inbox-row.tsx b/apps/web/components/needs-you-inbox/needs-you-inbox-row.tsx index 4435385292..2d76c93dbf 100644 --- a/apps/web/components/needs-you-inbox/needs-you-inbox-row.tsx +++ b/apps/web/components/needs-you-inbox/needs-you-inbox-row.tsx @@ -42,6 +42,11 @@ import { } from "@/lib/needs-you-inbox/row-presentation"; import { resolveThreadSessionStatus } from "@/lib/threads/thread-session-status"; import type { TaskSessionState } from "@/lib/types/http"; +import type { + LateClarificationSnapshot, + LateClarificationState, +} from "@/hooks/use-late-clarification-message"; +import type { MessageAdmissionOutcome } from "@/hooks/use-message-handler"; const SNOOZE_DURATIONS: ClarificationInboxSnoozeDuration[] = ["1h", "4h", "24h"]; @@ -95,6 +100,9 @@ function useRowOutcomeNotice(primaryText: string, bumpRefreshTick: () => void) { toast(t("needsYouInbox:bundleNoLongerActive", { question: primaryText })); bumpRefreshTick(); } + if (outcome.kind === "late_message_admitted") { + bumpRefreshTick(); + } // submission_failed: the shared overlay's own inline banner already // offers a retry; the row and count stay untouched here. }, @@ -163,7 +171,16 @@ function taskHrefForBundle(bundle: ClarificationInboxBundle): string { return `/t/${bundle.task_id}?sessionId=${encodeURIComponent(bundle.session_id)}`; } -export function NeedsYouInboxRow({ bundle }: { bundle: ClarificationInboxBundle }) { +// eslint-disable-next-line max-lines-per-function -- the row keeps its responsive actions and inline panel together. +export function NeedsYouInboxRow({ + bundle, + onLateAnswer, + lateAnswerState, +}: { + bundle: ClarificationInboxBundle; + onLateAnswer?: (snapshot: LateClarificationSnapshot) => Promise; + lateAnswerState?: LateClarificationState; +}) { const { t } = useTranslation(); const [expanded, setExpanded] = useState(false); const rowRef = useRef(null); @@ -260,6 +277,8 @@ export function NeedsYouInboxRow({ bundle }: { bundle: ClarificationInboxBundle messages={bundle.messages} onResolved={() => {}} onOutcome={handleOutcome} + onLateAnswer={onLateAnswer} + lateAnswerState={lateAnswerState} shortcutScopeRef={rowRef} maxHeightVh={50} /> diff --git a/apps/web/components/quick-chat/quick-chat-content.tsx b/apps/web/components/quick-chat/quick-chat-content.tsx index 4a058ec348..351346c241 100644 --- a/apps/web/components/quick-chat/quick-chat-content.tsx +++ b/apps/web/components/quick-chat/quick-chat-content.tsx @@ -15,6 +15,7 @@ import { getSessionWorkspacePath } from "@/lib/session-workspace-path"; import { routePanelMouseDown } from "@/components/task/chat/route-panel-mouse-down"; import { useQuickChatInitialPrompt } from "./use-quick-chat-initial-prompt"; import { QuickChatCancelCommands } from "./quick-chat-cancel-commands"; +import { useLateClarificationMessage } from "@/hooks/use-late-clarification-message"; type QuickChatContentProps = { sessionId: string; @@ -57,6 +58,7 @@ export const QuickChatContent = memo(function QuickChatContent({ const state = useQuickChatState(sessionId); const { chatInputRef, panelState, isSending, handleSubmit, handleCancelTurn } = state; const { taskId, pendingClarification, pendingClarificationGroup } = panelState; + const lateAnswer = useLateClarificationMessage(pendingClarificationGroup?.[0]); useEffect(() => { const timer = setTimeout(() => chatInputRef.current?.focusInput(), 50); @@ -123,6 +125,8 @@ export const QuickChatContent = memo(function QuickChatContent({ messages={pendingClarificationGroup} agentDisconnected={panelState.session?.pending_action === null} onResolved={handleClarificationResolved} + onLateAnswer={lateAnswer.send} + lateAnswerState={lateAnswer.state} shortcutScopeRef={shortcutScopeRef} maxHeightVh={35} /> diff --git a/apps/web/components/task/chat/chat-input-container.tsx b/apps/web/components/task/chat/chat-input-container.tsx index 0a7ac20fac..35f58238fd 100644 --- a/apps/web/components/task/chat/chat-input-container.tsx +++ b/apps/web/components/task/chat/chat-input-container.tsx @@ -56,6 +56,8 @@ export type ChatSubmitResult = void | boolean | Promise; export type ChatSubmitPayload = { message: string; + /** Reused by recovery-aware adapters when an admission survives remounting. */ + clientMessageId?: string; reviewComments?: ReviewComment[]; attachments?: MessageAttachment[]; inlineMentions?: ContextFile[]; diff --git a/apps/web/components/task/chat/clarification-input-overlay.test.tsx b/apps/web/components/task/chat/clarification-input-overlay.test.tsx index 2f51b9b71a..9b0a9c1bec 100644 --- a/apps/web/components/task/chat/clarification-input-overlay.test.tsx +++ b/apps/web/components/task/chat/clarification-input-overlay.test.tsx @@ -15,6 +15,8 @@ import { import { ClarificationInputOverlay } from "./clarification-input-overlay"; import type { ClarificationOutcome } from "@/hooks/domains/session/use-clarification-group"; +/* eslint-disable max-lines -- overlay lifecycle regressions share one deterministic harness. */ + vi.mock("@/lib/config", () => ({ getBackendConfig: () => ({ apiBaseUrl: "https://api.test" }), })); @@ -36,7 +38,10 @@ vi.mock("@kandev/ui/tooltip", () => ({ const fetchMock = vi.fn(); const TESTID_OPTION = "clarification-option"; const TESTID_STEP = "clarification-step"; +const TESTID_SKIP = "clarification-skip"; const TESTID_SUBMIT_ERROR = "clarification-submit-error"; +const TESTID_RETRY = "clarification-retry"; +const DATA_SELECTED = "data-selected"; function clarMessage(opts: { id: string; @@ -75,6 +80,20 @@ function renderOverlay( onResolved: () => void; onDismiss: () => void; onOutcome: (outcome: ClarificationOutcome) => void; + mode: "active" | "late"; + initialAnswers: readonly { + question_id: string; + selected_options?: string[]; + custom_text?: string; + }[]; + onLateAnswer: (snapshot: { + messages: readonly Message[]; + answers: readonly { + question_id: string; + selected_options?: string[]; + custom_text?: string; + }[]; + }) => Promise<"sent" | "queued">; }> = {}, ) { const scopeRef = createRef(); @@ -87,6 +106,9 @@ function renderOverlay( onResolved={onResolved} onDismiss={onDismiss} onOutcome={overrides.onOutcome} + mode={overrides.mode} + onLateAnswer={overrides.onLateAnswer} + initialAnswers={overrides.initialAnswers} shortcutScopeRef={scopeRef} /> , @@ -226,6 +248,21 @@ describe("ClarificationInputOverlay — Escape guard predicate (F1 regression)", expect(getGuard()?.test(fakeEscape(composerRef.current!))).toBe(false); }); + it("does not claim Escape after an inactive response leaves retained props mounted", async () => { + fetchMock.mockResolvedValueOnce( + new Response(JSON.stringify({ code: "not_active" }), { status: 409 }), + ); + const messages = [clarMessage({ id: "m1", questionId: "q1", index: 0, total: 1 })]; + const { composerRef, onDismiss, getGuard } = renderOverlayWithGuard(messages); + + fireEvent.click(screen.getByTestId(TESTID_OPTION)); + await vi.waitFor(() => screen.getByTestId("clarification-expired")); + + expect(getGuard()?.test(fakeEscape(composerRef.current!))).not.toBe(true); + fireEvent.keyDown(composerRef.current!, { key: "Escape" }); + expect(onDismiss).not.toHaveBeenCalled(); + }); + it("does not claim Escape held with a modifier", () => { const messages = [clarMessage({ id: "m1", questionId: "q1", index: 0, total: 1 })]; const { composerRef, getGuard } = renderOverlayWithGuard(messages); @@ -348,7 +385,7 @@ describe("ClarificationInputOverlay — labelled Skip button", () => { const messages = [clarMessage({ id: "m1", questionId: "q1", index: 0, total: 1 })]; renderOverlay(messages); - fireEvent.click(screen.getByTestId("clarification-skip")); + fireEvent.click(screen.getByTestId(TESTID_SKIP)); await vi.waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(1)); const [, init] = fetchMock.mock.calls[0]; @@ -359,6 +396,7 @@ describe("ClarificationInputOverlay — labelled Skip button", () => { }); }); +// eslint-disable-next-line max-lines-per-function -- submit failure cases share one overlay harness. describe("ClarificationInputOverlay — submit failure feedback", () => { it("shows a retry banner on a failed submit, preserves the answer, and never resolves", async () => { fetchMock.mockResolvedValueOnce(new Response("nope", { status: 500 })); @@ -370,12 +408,12 @@ describe("ClarificationInputOverlay — submit failure feedback", () => { await vi.waitFor(() => screen.getByTestId(TESTID_SUBMIT_ERROR)); expect(onResolved).not.toHaveBeenCalled(); - expect(screen.getByTestId(TESTID_OPTION).getAttribute("data-selected")).toBe("true"); + expect(screen.getByTestId(TESTID_OPTION).getAttribute(DATA_SELECTED)).toBe("true"); fetchMock.mockResolvedValueOnce( new Response(JSON.stringify({ success: true }), { status: 200 }), ); - fireEvent.click(screen.getByTestId("clarification-retry")); + fireEvent.click(screen.getByTestId(TESTID_RETRY)); await vi.waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(2)); const [retryUrl, retryInit] = fetchMock.mock.calls[1]; @@ -405,8 +443,8 @@ describe("ClarificationInputOverlay — submit failure feedback", () => { fireEvent.click(screen.getByTestId(TESTID_OPTION)); await vi.waitFor(() => expect(screen.getByTestId(TESTID_SUBMIT_ERROR)).toBeTruthy()); - expect(screen.getByTestId("clarification-retry")).toBeTruthy(); - expect((screen.getByTestId("clarification-skip") as HTMLButtonElement).disabled).toBe(false); + expect(screen.getByTestId(TESTID_RETRY)).toBeTruthy(); + expect((screen.getByTestId(TESTID_SKIP) as HTMLButtonElement).disabled).toBe(false); fireEvent.keyDown(scopeRef.current!, { key: "Escape" }); expect(onDismiss).toHaveBeenCalledTimes(1); }); @@ -415,7 +453,7 @@ describe("ClarificationInputOverlay — submit failure feedback", () => { fetchMock.mockResolvedValueOnce(new Response("nope", { status: 500 })); renderOverlay([clarMessage({ id: "m1", questionId: "q1", index: 0, total: 1 })]); - fireEvent.click(screen.getByTestId("clarification-skip")); + fireEvent.click(screen.getByTestId(TESTID_SKIP)); await vi.waitFor(() => expect(screen.queryByTestId(TESTID_SUBMIT_ERROR)).not.toBeNull()); expect(screen.getByTestId(TESTID_SUBMIT_ERROR).textContent).toContain( @@ -423,7 +461,7 @@ describe("ClarificationInputOverlay — submit failure feedback", () => { ); }); - it("shows a non-retryable expired banner when the bundle is no longer active", async () => { + it("retires an inactive bundle without leaving response controls mounted", async () => { fetchMock.mockResolvedValueOnce( new Response( JSON.stringify({ code: "not_active", error: "clarification request is no longer active" }), @@ -438,7 +476,71 @@ describe("ClarificationInputOverlay — submit failure feedback", () => { await vi.waitFor(() => screen.getByTestId("clarification-expired")); expect(onResolved).not.toHaveBeenCalled(); - expect(screen.queryByTestId("clarification-retry")).toBeNull(); + expect(screen.queryByTestId(TESTID_RETRY)).toBeNull(); + expect(screen.queryByTestId(TESTID_SKIP)).toBeNull(); + expect(screen.queryByTestId(TESTID_OPTION)).toBeNull(); + expect(screen.queryByTestId("clarification-submit")).toBeNull(); + }); + + it("preserves affirmative answers and admits them as a new message after inactive", async () => { + fetchMock.mockResolvedValueOnce( + new Response(JSON.stringify({ code: "not_active" }), { status: 409 }), + ); + const onLateAnswer = vi.fn().mockResolvedValue("sent" as const); + const { onResolved } = renderOverlay( + [clarMessage({ id: "m1", questionId: "q1", index: 0, total: 1 })], + { onLateAnswer }, + ); + + fireEvent.click(screen.getByTestId(TESTID_OPTION)); + + await vi.waitFor(() => expect(onLateAnswer).toHaveBeenCalledTimes(1)); + expect(onLateAnswer.mock.calls[0][0].answers).toEqual([ + { question_id: "q1", selected_options: ["o1"] }, + ]); + expect(onResolved).not.toHaveBeenCalled(); + expect(screen.getByTestId("clarification-late-success").textContent).toContain( + "Answer sent as a new message.", + ); + }); + + it("restores a retained late answer when the inline form is reopened", () => { + const message = clarMessage({ id: "m-reopen", questionId: "q-reopen", index: 0, total: 1 }); + renderOverlay([message], { + mode: "late", + initialAnswers: [{ question_id: "q-reopen", selected_options: ["o1"] }], + onLateAnswer: vi.fn().mockResolvedValue("sent" as const), + }); + + expect(screen.getByTestId(TESTID_OPTION).getAttribute(DATA_SELECTED)).toBe("true"); + expect((screen.getByTestId("clarification-late-submit") as HTMLButtonElement).disabled).toBe( + false, + ); + }); + + it("keeps the active form and draft when late admission fails, then retries", async () => { + fetchMock.mockResolvedValueOnce( + new Response(JSON.stringify({ code: "not_active" }), { status: 409 }), + ); + const onLateAnswer = vi + .fn() + .mockRejectedValueOnce(new Error("unavailable")) + .mockResolvedValueOnce("queued" as const); + renderOverlay([clarMessage({ id: "m1", questionId: "q1", index: 0, total: 1 })], { + onLateAnswer, + }); + + fireEvent.click(screen.getByTestId(TESTID_OPTION)); + await vi.waitFor(() => expect(screen.getByTestId(TESTID_SUBMIT_ERROR)).toBeTruthy()); + expect(screen.getByTestId(TESTID_OPTION).getAttribute(DATA_SELECTED)).toBe("true"); + + fireEvent.click(screen.getByTestId(TESTID_RETRY)); + await vi.waitFor(() => expect(onLateAnswer).toHaveBeenCalledTimes(2)); + await vi.waitFor(() => { + expect(screen.getByTestId("clarification-late-success").textContent).toContain( + "Answer queued as a new message.", + ); + }); }); // Regression: a new bundle (different pending_id) arriving after a failed @@ -470,7 +572,7 @@ describe("ClarificationInputOverlay — submit failure feedback", () => { ); expect(screen.queryByTestId(TESTID_SUBMIT_ERROR)).toBeNull(); - expect(screen.getByTestId(TESTID_OPTION).getAttribute("data-selected")).not.toBe("true"); + expect(screen.getByTestId(TESTID_OPTION).getAttribute(DATA_SELECTED)).not.toBe("true"); fetchMock.mockClear(); fireEvent.click(screen.getByTestId(TESTID_OPTION)); diff --git a/apps/web/components/task/chat/clarification-input-overlay.tsx b/apps/web/components/task/chat/clarification-input-overlay.tsx index 4a6b91de3c..57a7c5f352 100644 --- a/apps/web/components/task/chat/clarification-input-overlay.tsx +++ b/apps/web/components/task/chat/clarification-input-overlay.tsx @@ -1,5 +1,7 @@ "use client"; +/* eslint-disable max-lines -- the overlay keeps active and late-answer lifecycle states together. */ + import { useCallback, useEffect, useMemo, useRef, useState, type RefObject } from "react"; import { IconInfoCircle } from "@tabler/icons-react"; import type { @@ -12,6 +14,11 @@ import { useClarificationGroup, type ClarificationOutcome, } from "@/hooks/domains/session/use-clarification-group"; +import type { + LateClarificationSnapshot, + LateClarificationState, +} from "@/hooks/use-late-clarification-message"; +import type { MessageAdmissionOutcome } from "@/hooks/use-message-handler"; import { useClarificationEscapeGuard } from "@/hooks/use-clarification-escape-guard"; import { CLARIFICATION_CUSTOM_TEXT_MAX_RUNES, @@ -47,6 +54,12 @@ type ClarificationInputOverlayProps = { // "no longer active" / "submission failed" to decide whether to remove its // row (design-02#Failure-and-recovery). onOutcome?: (outcome: ClarificationOutcome) => void; + mode?: "active" | "late"; + onLateAnswer?: (snapshot: LateClarificationSnapshot) => Promise; + /** Restores a late-message draft after its inline form was removed. */ + initialAnswers?: readonly ClarificationAnswer[]; + /** Shares late-message admission state across transcript and active hosts. */ + lateAnswerState?: LateClarificationState; }; type SingleQuestionMeta = { @@ -400,6 +413,8 @@ type CarouselBodyProps = { armedEventRef: RefObject; keyboardShortcutsEnabled: boolean; onSubmit: () => void; + submitAnswers: (override?: Record) => void | Promise; + autoSubmitSingleQuestion: boolean; onDismiss: () => void; }; @@ -411,8 +426,24 @@ type QuestionHandlerCtx = { total: number; setActiveIndex: (idx: number) => void; setCustomDrafts: React.Dispatch>>; + submitAnswers: (override?: Record) => void | Promise; + autoSubmitSingleQuestion: boolean; }; +type AnswerSubmitter = (override?: Record) => void | Promise; + +function chooseSubmitter( + lateMode: boolean, + lateInteraction: boolean, + lateSubmitter: AnswerSubmitter, + lateRetry: AnswerSubmitter, + activeSubmitter: AnswerSubmitter, +): AnswerSubmitter { + if (lateMode) return lateSubmitter; + if (lateInteraction) return lateRetry; + return activeSubmitter; +} + type QuestionHandlers = { onSelectOption: (optionId: string) => void; onCustomDraftChange: (value: string) => void; @@ -442,15 +473,24 @@ function deriveSelectionState( } function buildQuestionHandlers(ctx: QuestionHandlerCtx): QuestionHandlers { - const { meta, group, isSingleQuestion, activeIndex, total, setActiveIndex, setCustomDrafts } = - ctx; + const { + meta, + group, + isSingleQuestion, + activeIndex, + total, + setActiveIndex, + setCustomDrafts, + submitAnswers, + autoSubmitSingleQuestion, + } = ctx; // Records the answer, then auto-submits (single-question — uses the override // path because setState is async) or auto-advances to the next step. const commitAnswer = (answer: ClarificationAnswer) => { group.recordAnswer(meta.questionId, answer); - if (isSingleQuestion) { - void group.submitCollected({ [meta.questionId]: answer }); + if (isSingleQuestion && autoSubmitSingleQuestion) { + void submitAnswers({ [meta.questionId]: answer }); return; } if (activeIndex < total - 1) setActiveIndex(activeIndex + 1); @@ -513,6 +553,8 @@ function ClarificationCarouselBody({ armedEventRef, keyboardShortcutsEnabled, onSubmit, + submitAnswers, + autoSubmitSingleQuestion, onDismiss, }: CarouselBodyProps) { const total = sortedMessages.length; @@ -539,6 +581,8 @@ function ClarificationCarouselBody({ total, setActiveIndex, setCustomDrafts, + submitAnswers, + autoSubmitSingleQuestion, }); return ( @@ -585,27 +629,63 @@ function ClarificationCarouselBody({ ); } -// eslint-disable-next-line max-lines-per-function -- coordinates the complete clarification overlay lifecycle. +// eslint-disable-next-line max-lines-per-function, complexity, sonarjs/cognitive-complexity -- coordinates the complete clarification overlay lifecycle. export function ClarificationInputOverlay({ messages, onResolved, onOutcome, + mode = "active", + onLateAnswer, + initialAnswers, shortcutScopeRef, keyboardShortcutsEnabled = true, agentDisconnected = false, onDismiss, onCollapse, collapseContentId, + lateAnswerState, }: ClarificationInputOverlayProps) { + const { t } = useTranslation(); + const lateMode = mode === "late"; const sortedMessages = useMemo( () => sortMessagesByQuestionIndex(resolveQuestionMessages(messages)), [messages], ); - const group = useClarificationGroup(sortedMessages, onOutcome); - const isSubmitting = group.submitState === "submitting"; + const group = useClarificationGroup(sortedMessages, onOutcome, onLateAnswer); + const [lateStatus, setLateStatus] = useState<"idle" | "sending" | "sent" | "queued" | "error">( + "idle", + ); + const [lateSnapshot, setLateSnapshot] = useState(null); + const sharedLateStatus = lateMode ? (lateAnswerState?.status ?? "idle") : "idle"; + const effectiveLateStatus = sharedLateStatus === "idle" ? lateStatus : sharedLateStatus; + const effectiveLateSnapshot = lateMode + ? (lateAnswerState?.snapshot ?? lateSnapshot) + : lateSnapshot; + const isSubmitting = + group.submitState === "submitting" || + group.lateAnswerState === "sending" || + effectiveLateStatus === "sending"; + const lateInteraction = lateMode || group.lateAnswerState !== "idle"; const [customDrafts, setCustomDrafts] = useState>({}); const [rawActiveIndex, setActiveIndex] = useState(0); useResetOverlayStateOnBundleChange(group.pendingId, setCustomDrafts, setActiveIndex); + const restoredAnswers = lateMode + ? (lateAnswerState?.snapshot?.answers ?? initialAnswers) + : initialAnswers; + const initialAnswersKey = JSON.stringify(restoredAnswers ?? []); + useEffect(() => { + if (!restoredAnswers || restoredAnswers.length === 0) return; + for (const answer of restoredAnswers) { + group.recordAnswer(answer.question_id, answer); + } + setCustomDrafts((current) => { + const next = { ...current }; + for (const answer of restoredAnswers) { + if (answer.custom_text !== undefined) next[answer.question_id] = answer.custom_text; + } + return next; + }); + }, [group.recordAnswer, initialAnswersKey, restoredAnswers]); // Clamp the active index to the current bundle size so late-arriving // messages or shrunk bundles never put us out of range. const total = sortedMessages.length; @@ -614,16 +694,45 @@ export function ClarificationInputOverlay({ const meta = activeMessage ? readSingleQuestionMeta(activeMessage) : null; const sharedContext = readSharedContext(sortedMessages[0]); - useResolveCallback(group.submitState, onResolved); + useResolveCallback(lateMode ? "idle" : group.submitState, onResolved); // group is a fresh object every render, but its submitCollected callback is // memoised by the hook — depend on the function only so this useCallback // doesn't churn on every keystroke (via the live-record path). - const submitCollected = group.submitCollected; const allAnswered = computeAllAnswered(sortedMessages, group.answers); + const submitLateAnswer = useCallback( + async (override?: Record) => { + const answers = { ...group.answers, ...(override ?? {}) }; + if (!computeAllAnswered(sortedMessages, answers) || !onLateAnswer) return; + const snapshot: LateClarificationSnapshot = { + messages: sortedMessages.slice(), + answers: Object.values(answers), + }; + setLateSnapshot(snapshot); + setLateStatus("sending"); + try { + const outcome = await onLateAnswer(snapshot); + setLateStatus(outcome); + } catch { + setLateStatus("error"); + } + }, + [group.answers, onLateAnswer, sortedMessages], + ); + const submitAnswers = chooseSubmitter( + lateMode, + lateInteraction, + submitLateAnswer, + group.retryLateAnswer, + group.submitCollected, + ); const handleSubmit = useCallback(() => { - if (allAnswered) void submitCollected(); - }, [allAnswered, submitCollected]); + if (allAnswered) void submitAnswers(); + }, [allAnswered, submitAnswers]); + const retryLateAnswerForm = useCallback(() => { + if (effectiveLateSnapshot) void submitLateAnswer(); + }, [effectiveLateSnapshot, submitLateAnswer]); + const retryLateAnswer = lateMode ? retryLateAnswerForm : group.retryLateAnswer; // Gated on the same resolved `meta` that decides whether // ClarificationCarouselBody (and CarouselKeyboardShortcuts within it) mount @@ -631,12 +740,54 @@ export function ClarificationInputOverlay({ // guard could tell the dialog "handledHere" for a state where the widget // that would actually handle Escape never mounted in the first place. const armedEventRef = useEscapeGuardRegistration( - keyboardShortcutsEnabled && !isSubmitting && meta !== null, + keyboardShortcutsEnabled && + !isSubmitting && + group.submitState !== "expired" && + group.lateAnswerState !== "sent" && + group.lateAnswerState !== "queued" && + (!lateInteraction || (effectiveLateStatus !== "sent" && effectiveLateStatus !== "queued")) && + meta !== null, shortcutScopeRef, ); if (sortedMessages.length === 0) return null; + if (!lateMode && group.submitState === "expired") { + return ( +
+ void group.retry()} /> +
+ ); + } + + if ( + (lateMode && (effectiveLateStatus === "sent" || effectiveLateStatus === "queued")) || + (!lateMode && (group.lateAnswerState === "sent" || group.lateAnswerState === "queued")) + ) { + return ( +
+
+ + {(lateMode ? effectiveLateStatus : group.lateAnswerState) === "sent" + ? t("task:lateAnswerSent") + : t("task:lateAnswerQueued")} + + +
+
+ ); + } + return (
void group.skipAll("User skipped")} + lateMode={lateInteraction} + onLateClose={onDismiss} onCollapse={onCollapse} collapseContentId={collapseContentId} /> - {(group.submitState === "error" || group.submitState === "expired") && ( + {group.submitState === "error" && ( void group.retry()} /> )} + {lateInteraction && + ((lateMode && effectiveLateStatus === "error") || + (!lateMode && group.lateAnswerState === "error")) && ( + + )} {sharedContext && (
diff --git a/apps/web/components/task/chat/clarification-overlay-header.tsx b/apps/web/components/task/chat/clarification-overlay-header.tsx index 548a99b1ec..09407c7fb6 100644 --- a/apps/web/components/task/chat/clarification-overlay-header.tsx +++ b/apps/web/components/task/chat/clarification-overlay-header.tsx @@ -30,6 +30,8 @@ type ClarificationOverlayTopBarProps = { allAnswered: boolean; onSubmit: () => void; onSkip: () => void; + lateMode?: boolean; + onLateClose?: () => void; onCollapse?: () => void; collapseContentId?: string; }; @@ -48,6 +50,8 @@ export function ClarificationOverlayTopBar({ allAnswered, onSubmit, onSkip, + lateMode = false, + onLateClose, onCollapse, collapseContentId, }: ClarificationOverlayTopBarProps) { @@ -80,6 +84,8 @@ export function ClarificationOverlayTopBar({ isSubmitting={isSubmitting} onSubmit={onSubmit} onSkip={onSkip} + lateMode={lateMode} + onLateClose={onLateClose} onCollapse={onCollapse} collapseContentId={collapseContentId} /> @@ -93,6 +99,8 @@ type ClarificationHeaderActionsProps = { isSubmitting: boolean; onSubmit: () => void; onSkip: () => void; + lateMode?: boolean; + onLateClose?: () => void; onCollapse?: () => void; collapseContentId?: string; }; @@ -131,16 +139,47 @@ function ClarificationSkipButton({ ); } +// eslint-disable-next-line max-lines-per-function, complexity -- header actions keep active and late-answer controls aligned. export function ClarificationHeaderActions({ total, allAnswered, isSubmitting, onSubmit, onSkip, + lateMode = false, + onLateClose, onCollapse, collapseContentId, }: ClarificationHeaderActionsProps) { const { t } = useTranslation(); + if (lateMode) { + return ( +
+ + +
+ ); + } return (
void; + onLateAnswer?: (snapshot: LateClarificationSnapshot) => Promise; + lateAnswerState?: LateClarificationState; }; function pendingIdFromMessages(messages: readonly Message[] | null | undefined): string | null { const first = messages?.[0]; @@ -68,6 +75,8 @@ export function ClarificationPanelSection({ agentDisconnected = false, maxHeightVh, onOutcome, + onLateAnswer, + lateAnswerState, }: ClarificationPanelSectionProps) { const { t } = useTranslation(); const disclosure = useComposerDisclosureContext(); @@ -108,6 +117,8 @@ export function ClarificationPanelSection({ messages={messages} onCollapse={() => setCollapsed(true)} onOutcome={onOutcome} + onLateAnswer={onLateAnswer} + lateAnswerState={lateAnswerState} onResolved={onResolved} onToggleCollapse={() => setCollapsed((current) => !current)} questionCount={questionCount} @@ -129,6 +140,8 @@ type ClarificationPanelContentProps = { messages: readonly Message[] | null | undefined; onCollapse: () => void; onOutcome?: (outcome: ClarificationOutcome) => void; + onLateAnswer?: (snapshot: LateClarificationSnapshot) => Promise; + lateAnswerState?: LateClarificationState; onResolved: () => void; onToggleCollapse: () => void; questionCount: number; @@ -148,6 +161,8 @@ function ClarificationPanelContent({ messages, onCollapse, onOutcome, + onLateAnswer, + lateAnswerState, onResolved, onToggleCollapse, questionCount, @@ -220,6 +235,8 @@ function ClarificationPanelContent({ messages={messages} onResolved={onResolved} onOutcome={onOutcome} + onLateAnswer={onLateAnswer} + lateAnswerState={lateAnswerState} shortcutScopeRef={shortcutScopeRef} keyboardShortcutsEnabled={!collapsed} agentDisconnected={agentDisconnected} diff --git a/apps/web/components/task/chat/message-renderer.tsx b/apps/web/components/task/chat/message-renderer.tsx index 39cad8b054..eb11c0179c 100644 --- a/apps/web/components/task/chat/message-renderer.tsx +++ b/apps/web/components/task/chat/message-renderer.tsx @@ -1,6 +1,6 @@ "use client"; -import { memo, useState, useCallback, type ReactElement } from "react"; +import { memo, useState, useCallback, useMemo, type ReactElement } from "react"; import { IconPlayerPlay } from "@tabler/icons-react"; import { Button } from "@kandev/ui/button"; import { sessionId as toSessionId, taskId as toTaskId } from "@/lib/types/http"; @@ -26,6 +26,7 @@ import { ThinkingMessage } from "@/components/task/chat/messages/thinking-messag import { TodoMessage } from "@/components/task/chat/messages/todo-message"; import { ScriptExecutionMessage } from "@/components/task/chat/messages/script-execution-message"; import { ClarificationRequestMessage } from "@/components/task/chat/messages/clarification-request-message"; +import { useLateClarificationMessage } from "@/hooks/use-late-clarification-message"; import { ToolSubagentMessage } from "@/components/task/chat/messages/tool-subagent-message"; import { MonitorMessage } from "@/components/task/chat/messages/monitor-message"; import { AgentPlanMessage } from "@/components/task/chat/messages/agent-plan-message"; @@ -216,6 +217,43 @@ type MessageAdapter = { render: (comment: Message, ctx: AdapterContext) => ReactElement; }; +function ClarificationRequestMessageAdapter({ + comment, + isCurrentTurn, +}: { + comment: Message; + isCurrentTurn: boolean; +}) { + const lateAnswer = useLateClarificationMessage(comment); + const sessionMessages = useAppStore( + useCallback( + (state) => state.messages.bySession[comment.session_id] ?? [], + [comment.session_id], + ), + ); + const bundle = useMemo(() => { + const pendingId = (comment.metadata as { pending_id?: string } | undefined)?.pending_id; + if (!pendingId) return [comment]; + const matching = sessionMessages.filter( + (message) => + message.type === "clarification_request" && + (message.metadata as { pending_id?: string } | undefined)?.pending_id === pendingId, + ); + return matching.length > 0 ? matching : [comment]; + }, [comment, sessionMessages]); + return ( + + ); +} + const adapters: MessageAdapter[] = [ { matches: (comment) => @@ -384,7 +422,12 @@ const adapters: MessageAdapter[] = [ }, { matches: (comment) => comment.type === "clarification_request", - render: (comment) => , + render: (comment, ctx) => ( + + ), }, { matches: (comment) => comment.type === "agent_plan", diff --git a/apps/web/components/task/chat/messages/clarification-request-message.test.tsx b/apps/web/components/task/chat/messages/clarification-request-message.test.tsx index 15be4c054f..9eece4000a 100644 --- a/apps/web/components/task/chat/messages/clarification-request-message.test.tsx +++ b/apps/web/components/task/chat/messages/clarification-request-message.test.tsx @@ -1,5 +1,5 @@ -import { render } from "@testing-library/react"; -import { describe, expect, it } from "vitest"; +import { cleanup, fireEvent, render, screen, waitFor } from "@testing-library/react"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { sessionId as toSessionId, taskId as toTaskId, @@ -8,6 +8,16 @@ import { } from "@/lib/types/http"; import { ClarificationRequestMessage } from "./clarification-request-message"; +const mockUpdateMessage = vi.hoisted(() => vi.fn()); +vi.mock("@/components/state-provider", () => ({ + useAppStoreApi: () => ({ + getState: () => ({ + updateMessage: mockUpdateMessage, + messages: { bySession: {} }, + }), + }), +})); + function answeredClarification(): Message { const metadata: ClarificationRequestMetadata = { pending_id: "pending-1", @@ -45,6 +55,8 @@ function answeredClarification(): Message { } describe("ClarificationRequestMessage", () => { + afterEach(cleanup); + it("renders agent question Markdown but keeps custom answer text literal", () => { const { container } = render(); @@ -61,4 +73,79 @@ describe("ClarificationRequestMessage", () => { expect(customText).toBeDefined(); expect(customText?.querySelector("code, strong")).toBeNull(); }); + + it("offers an answer-as-new-message action for an unanswered historical question", () => { + const message = answeredClarification(); + message.metadata = { + ...(message.metadata as ClarificationRequestMetadata), + status: "pending", + response: undefined, + }; + + render(); + + expect(screen.getByTestId("clarification-answer-as-new-message")).toBeTruthy(); + }); + + it("does not offer a second action for the current pending turn", () => { + const message = answeredClarification(); + message.metadata = { + ...(message.metadata as ClarificationRequestMetadata), + status: "pending", + response: undefined, + }; + + render(); + + expect(screen.queryByTestId("clarification-answer-as-new-message")).toBeNull(); + }); + + it("sends a selected historical answer through the late-message callback", async () => { + const onLateAnswer = vi.fn().mockResolvedValue("sent" as const); + render( + , + ); + + fireEvent.click(screen.getByRole("button", { name: /answer as new message/i })); + fireEvent.click(screen.getByTestId("clarification-option")); + fireEvent.click(screen.getByTestId("clarification-late-submit")); + + await waitFor(() => expect(onLateAnswer).toHaveBeenCalledTimes(1)); + expect(onLateAnswer.mock.calls[0][0].answers).toEqual([ + { question_id: "question-1", selected_options: ["fast"] }, + ]); + }); + + it("closes the historical form without admitting a message", () => { + const onLateAnswer = vi.fn().mockResolvedValue("sent" as const); + render( + , + ); + + fireEvent.click(screen.getByRole("button", { name: /answer as new message/i })); + fireEvent.click(screen.getByTestId("clarification-late-close")); + + expect(onLateAnswer).not.toHaveBeenCalled(); + expect(screen.getByRole("button", { name: /answer as new message/i })).toBeTruthy(); + }); }); diff --git a/apps/web/components/task/chat/messages/clarification-request-message.tsx b/apps/web/components/task/chat/messages/clarification-request-message.tsx index d7ee23d29a..ada126dcd9 100644 --- a/apps/web/components/task/chat/messages/clarification-request-message.tsx +++ b/apps/web/components/task/chat/messages/clarification-request-message.tsx @@ -1,6 +1,8 @@ "use client"; -import { IconMessageQuestion, IconCheck, IconX } from "@tabler/icons-react"; +import { useCallback, useRef, useState } from "react"; +import { IconMessageQuestion, IconCheck, IconX, IconMessagePlus } from "@tabler/icons-react"; +import { Button } from "@kandev/ui/button"; import type { ClarificationAnswer, ClarificationQuestion, @@ -9,9 +11,21 @@ import type { } from "@/lib/types/http"; import { useTranslation } from "react-i18next"; import { ClarificationMarkdown } from "../clarification-markdown"; +import { ClarificationInputOverlay } from "../clarification-input-overlay"; +import type { + LateClarificationSnapshot, + LateClarificationState, +} from "@/hooks/use-late-clarification-message"; +import type { MessageAdmissionOutcome } from "@/hooks/use-message-handler"; type ClarificationRequestMessageProps = { comment: Message; + messages?: readonly Message[]; + onLateAnswer?: (snapshot: LateClarificationSnapshot) => Promise; + lateAnswerSnapshot?: LateClarificationSnapshot | null; + lateAnswerState?: LateClarificationState; + onResetLateAnswer?: () => void; + isCurrentTurn?: boolean; }; function AnswerSummary({ @@ -55,8 +69,24 @@ function AnswerSummary({ * Displays a resolved or superseded clarification request in the chat history. * The active pending clarification is shown in the input area instead. */ -export function ClarificationRequestMessage({ comment }: ClarificationRequestMessageProps) { +// eslint-disable-next-line max-lines-per-function, complexity -- the transcript row owns question status and late-answer controls. +export function ClarificationRequestMessage({ + comment, + messages = [comment], + onLateAnswer, + lateAnswerSnapshot, + lateAnswerState, + onResetLateAnswer, + isCurrentTurn = false, +}: ClarificationRequestMessageProps) { const { t } = useTranslation(); + const [isAnswering, setIsAnswering] = useState(false); + const answerButtonRef = useRef(null); + const answerScopeRef = useRef(null); + const closeAnswerForm = useCallback(() => { + setIsAnswering(false); + requestAnimationFrame(() => answerButtonRef.current?.focus()); + }, []); const metadata = comment.metadata as ClarificationRequestMetadata | undefined; if (!metadata?.question) { @@ -69,6 +99,8 @@ export function ClarificationRequestMessage({ comment }: ClarificationRequestMes const isSkipped = status === "rejected"; const isExpired = status === "expired"; const isCancelled = status === "cancelled"; + const canAnswerAsNewMessage = + !metadata.response && (status === "expired" || (status === "pending" && !isCurrentTurn)); const getStatusIndicator = () => { if (isAnswered) { @@ -140,6 +172,39 @@ export function ClarificationRequestMessage({ comment }: ClarificationRequestMes {t("task:timedOutAgentMovedOn")}
)} + {canAnswerAsNewMessage && !isAnswering && ( + + )} + {canAnswerAsNewMessage && isAnswering && ( +
+ {}} + shortcutScopeRef={answerScopeRef} + onDismiss={closeAnswerForm} + /> +
+ )}
diff --git a/apps/web/components/task/chat/use-chat-input-state.ts b/apps/web/components/task/chat/use-chat-input-state.ts index 193637423c..853c3b85e7 100644 --- a/apps/web/components/task/chat/use-chat-input-state.ts +++ b/apps/web/components/task/chat/use-chat-input-state.ts @@ -161,7 +161,9 @@ type SubmitDraftArgs = { clearArgs: Omit; }; -type DraftChatSubmitPayload = Required>; +type DraftChatSubmitPayload = Required< + Omit +>; function buildChatSubmitPayload(payload: DraftChatSubmitPayload): ChatSubmitPayload { return { diff --git a/apps/web/components/task/task-chat-panel.tsx b/apps/web/components/task/task-chat-panel.tsx index 5e4cb063c6..37bb06ae79 100644 --- a/apps/web/components/task/task-chat-panel.tsx +++ b/apps/web/components/task/task-chat-panel.tsx @@ -64,6 +64,7 @@ import { ParkedSessionNote, } from "./launch-queue-status"; import { WipQueueStatus } from "./wip-queue-status"; +import { useLateClarificationMessage } from "@/hooks/use-late-clarification-message"; /** Returns a `clarificationKey` that increments each time a pending * clarification is resolved, letting the composer reset its input state for @@ -1048,6 +1049,7 @@ export const TaskChatPanel = memo(function TaskChatPanel({ pendingClarificationGroup, } = panelState; const taskLaunchError = statusSummaryTaskError(launchStatusSummary); + const lateAnswer = useLateClarificationMessage(pendingClarificationGroup?.[0]); const launchErrorOwned = Boolean(taskLaunchError); const showAgentStartHint = useComposerAgentStartHint( resolvedSessionId, @@ -1263,6 +1265,8 @@ export const TaskChatPanel = memo(function TaskChatPanel({ messages={pendingClarificationGroup} agentDisconnected={session?.pending_action === null} onResolved={handleClarificationResolved} + onLateAnswer={lateAnswer.send} + lateAnswerState={lateAnswer.state} shortcutScopeRef={panelRef} maxHeightVh={50} /> diff --git a/apps/web/e2e/tests/chat/clarification-submit-failure.spec.ts b/apps/web/e2e/tests/chat/clarification-submit-failure.spec.ts index 4eda5ea024..da7b4e82e9 100644 --- a/apps/web/e2e/tests/chat/clarification-submit-failure.spec.ts +++ b/apps/web/e2e/tests/chat/clarification-submit-failure.spec.ts @@ -68,7 +68,7 @@ test.describe("Clarification submit failure feedback", () => { expect(attempt).toBe(2); }); - test("treats an inactive-bundle 409 as expired, never as a silent success", async ({ + test("late answer fallback sends affirmative answers after an inactive-bundle 409", async ({ testPage, apiClient, seedData, @@ -84,7 +84,9 @@ test.describe("Clarification submit failure feedback", () => { await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); + let attempts = 0; await testPage.route("**/api/v1/clarification/*/respond", async (route) => { + attempts += 1; await route.fulfill({ status: 409, contentType: "application/json", @@ -95,14 +97,24 @@ test.describe("Clarification submit failure feedback", () => { }); }); + const sessionId = await activeSessionId(testPage); + if (!sessionId) throw new Error("expected an active session for late clarification answer"); await session.clarificationOption("PostgreSQL").click(); - const expiredBanner = testPage.getByTestId("clarification-expired"); - await expect(expiredBanner).toBeVisible({ timeout: 15_000 }); - await expect(testPage.getByTestId("clarification-retry")).toHaveCount(0); - // A dropped answer must never flip the chat back to idle -- that would be - // reporting success for an answer nobody received. - await expect(session.idleInput()).toHaveCount(0); - await expect(session.clarificationOverlay()).toBeVisible(); + await expect + .poll( + async () => { + const { messages } = await apiClient.listSessionMessages(sessionId); + return messages.some( + (message) => + message.author_type === "user" && + message.content.includes("Question 1") && + message.content.includes("PostgreSQL"), + ); + }, + { timeout: 30_000, message: "inactive answer should be admitted as a new message" }, + ) + .toBe(true); + expect(attempts).toBe(1); }); }); diff --git a/apps/web/e2e/tests/chat/clarification.spec.ts b/apps/web/e2e/tests/chat/clarification.spec.ts index c7528406d2..6f2a55e811 100644 --- a/apps/web/e2e/tests/chat/clarification.spec.ts +++ b/apps/web/e2e/tests/chat/clarification.spec.ts @@ -3,9 +3,9 @@ import { test, expect } from "../../fixtures/test-base"; import { useRegularMode } from "../../helpers/regular-mode"; import type { SeedData } from "../../fixtures/test-base"; import type { ApiClient } from "../../helpers/api-client"; -import { seedClarificationSession } from "../../helpers/clarification"; +import { activeSessionId, seedClarificationSession } from "../../helpers/clarification"; import { waitForSessionState } from "../../helpers/session"; -import { dwell } from "../../helpers/causal-waits"; +import { dwell, waitForHttp } from "../../helpers/causal-waits"; import { SessionPage } from "../../pages/session-page"; import { KanbanPage } from "../../pages/kanban-page"; import { SidebarFilterPopoverPage } from "../../pages/sidebar-filter-popover"; @@ -363,6 +363,110 @@ test.describe("Clarification flow", () => { await expect(session.idleInput()).toBeVisible({ timeout: 30_000 }); }); + test("inactive dismissal removes the stale panel and does not retry", async ({ + testPage, + apiClient, + seedData, + }) => { + const session = await seedClarificationTask( + testPage, + apiClient, + seedData, + "Clarification Inactive Dismissal", + "clarification", + ); + await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); + + let attempts = 0; + await testPage.route("**/api/v1/clarification/*/respond", async (route) => { + attempts += 1; + await route.fulfill({ + status: 409, + contentType: "application/json", + body: JSON.stringify({ code: "not_active" }), + }); + }); + + const inactiveResponse = waitForHttp( + testPage, + "POST", + /\/api\/v1\/clarification\/[^/]+\/respond$/, + ); + await session.clarificationSkip().click(); + await expect((await inactiveResponse).status()).toBe(409); + + await expect(session.clarificationOverlay()).not.toBeVisible(); + await expect(session.anyIdleInput()).toBeVisible(); + await dwell( + testPage, + 250, + "negative-assertion", + "observe that an inactive clarification is not submitted a second time", + ); + expect(attempts).toBe(1); + }); + + test("late answer from an inactive historical question sends a new message", async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(60_000); + const session = await seedClarificationTask( + testPage, + apiClient, + seedData, + "Clarification Late Answer", + "clarification", + ); + const sessionId = await activeSessionId(testPage); + if (!sessionId) throw new Error("expected an active session for late clarification answer"); + + let responseAttempts = 0; + await testPage.route("**/api/v1/clarification/*/respond", async (route) => { + responseAttempts += 1; + await route.fulfill({ + status: 409, + contentType: "application/json", + body: JSON.stringify({ code: "not_active" }), + }); + }); + + const inactiveResponse = waitForHttp( + testPage, + "POST", + /\/api\/v1\/clarification\/[^/]+\/respond$/, + ); + await session.clarificationSkip().click(); + await expect((await inactiveResponse).status()).toBe(409); + await expect(session.clarificationOverlay()).not.toBeVisible(); + + const answerAction = testPage.getByTestId("clarification-answer-as-new-message"); + await expect(answerAction).toBeVisible({ timeout: 15_000 }); + await answerAction.click(); + await expect(testPage.getByTestId("clarification-late-submit")).toBeDisabled(); + await testPage.getByTestId("clarification-option").filter({ hasText: "PostgreSQL" }).click(); + await expect(testPage.getByTestId("clarification-late-submit")).toBeEnabled(); + + await testPage.getByTestId("clarification-late-submit").click(); + + await expect + .poll( + async () => { + const { messages } = await apiClient.listSessionMessages(sessionId); + return messages.some( + (message) => + message.author_type === "user" && + message.content.includes("Question 1") && + message.content.includes("PostgreSQL"), + ); + }, + { timeout: 30_000, message: "late clarification answer should be admitted as a message" }, + ) + .toBe(true); + expect(responseAttempts).toBe(1); + }); + test("timeout detaches clarification and accepts a custom deferred answer", async ({ testPage, apiClient, diff --git a/apps/web/e2e/tests/chat/mobile-clarification.spec.ts b/apps/web/e2e/tests/chat/mobile-clarification.spec.ts index 4ce73a5cf3..40c2409375 100644 --- a/apps/web/e2e/tests/chat/mobile-clarification.spec.ts +++ b/apps/web/e2e/tests/chat/mobile-clarification.spec.ts @@ -1,6 +1,6 @@ import { test, expect } from "../../fixtures/test-base"; import { activeSessionId, seedClarificationSession } from "../../helpers/clarification"; -import { watchWs } from "../../helpers/causal-waits"; +import { dwell, waitForHttp, watchWs } from "../../helpers/causal-waits"; import { waitForSessionSettled } from "./quick-chat-helpers"; /** @@ -219,6 +219,113 @@ test.describe("Mobile clarification multiline answer", () => { expect(attempt).toBe(2); }); + test("inactive dismissal removes the stale panel and does not retry on mobile", async ({ + testPage, + apiClient, + seedData, + }) => { + const session = await seedClarificationSession( + testPage, + apiClient, + seedData, + "Mobile Clarify Inactive Dismissal", + { scenario: "clarification" }, + ); + await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); + + let attempts = 0; + await testPage.route("**/api/v1/clarification/*/respond", async (route) => { + attempts += 1; + await route.fulfill({ + status: 409, + contentType: "application/json", + body: JSON.stringify({ code: "not_active" }), + }); + }); + + const inactiveResponse = waitForHttp( + testPage, + "POST", + /\/api\/v1\/clarification\/[^/]+\/respond$/, + ); + await session.clarificationSkip().tap(); + await expect((await inactiveResponse).status()).toBe(409); + + await expect(session.clarificationOverlay()).not.toBeVisible(); + await expect(session.anyIdleInput()).toBeVisible(); + await dwell( + testPage, + 250, + "negative-assertion", + "observe that an inactive clarification is not submitted a second time on mobile", + ); + expect(attempts).toBe(1); + }); + + test("late answer from an inactive historical question sends a new message on mobile", async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(60_000); + const session = await seedClarificationSession( + testPage, + apiClient, + seedData, + "Mobile Clarify Late Answer", + { scenario: "clarification" }, + ); + const sessionId = await activeSessionId(testPage); + if (!sessionId) throw new Error("expected an active session for mobile late answer"); + + let responseAttempts = 0; + await testPage.route("**/api/v1/clarification/*/respond", async (route) => { + responseAttempts += 1; + await route.fulfill({ + status: 409, + contentType: "application/json", + body: JSON.stringify({ code: "not_active" }), + }); + }); + + const inactiveResponse = waitForHttp( + testPage, + "POST", + /\/api\/v1\/clarification\/[^/]+\/respond$/, + ); + await session.clarificationSkip().tap(); + await expect((await inactiveResponse).status()).toBe(409); + await expect(session.clarificationOverlay()).not.toBeVisible(); + + const answerAction = testPage.getByTestId("clarification-answer-as-new-message"); + await expect(answerAction).toBeVisible({ timeout: 15_000 }); + await answerAction.tap(); + await expect(testPage.getByTestId("clarification-late-submit")).toBeDisabled(); + await testPage.getByTestId("clarification-option").filter({ hasText: "PostgreSQL" }).tap(); + await expect(testPage.getByTestId("clarification-late-submit")).toBeEnabled(); + + await testPage.getByTestId("clarification-late-submit").tap(); + + await expect + .poll( + async () => { + const { messages } = await apiClient.listSessionMessages(sessionId); + return messages.some( + (message) => + message.author_type === "user" && + message.content.includes("Question 1") && + message.content.includes("PostgreSQL"), + ); + }, + { + timeout: 30_000, + message: "mobile late clarification answer should be admitted as a message", + }, + ) + .toBe(true); + expect(responseAttempts).toBe(1); + }); + test("keeps the over-limit counter inside the phone viewport", async ({ testPage, apiClient, diff --git a/apps/web/hooks/domains/session/use-clarification-group.regressions.test.ts b/apps/web/hooks/domains/session/use-clarification-group.regressions.test.ts index 6983f0849b..612fff88f4 100644 --- a/apps/web/hooks/domains/session/use-clarification-group.regressions.test.ts +++ b/apps/web/hooks/domains/session/use-clarification-group.regressions.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; -import { act, renderHook } from "@testing-library/react"; +import { act, renderHook, waitFor } from "@testing-library/react"; import { sessionId as toSessionId, taskId as toTaskId, type Message } from "@/lib/types/http"; vi.mock("@/lib/config", () => ({ @@ -7,9 +7,13 @@ vi.mock("@/lib/config", () => ({ })); const mockUpdateMessage = vi.fn(); +let mockMessagesBySession: Record = {}; vi.mock("@/components/state-provider", () => ({ useAppStoreApi: () => ({ - getState: () => ({ updateMessage: mockUpdateMessage }), + getState: () => ({ + updateMessage: mockUpdateMessage, + messages: { bySession: mockMessagesBySession }, + }), }), })); @@ -21,6 +25,7 @@ function clarMessage(opts: { questionId: string; index: number; total: number; + updatedAt?: string; }): Message { return { id: opts.id, @@ -30,6 +35,7 @@ function clarMessage(opts: { content: "Q", type: "clarification_request", created_at: "2026-05-04T00:00:00Z", + ...(opts.updatedAt ? { updated_at: opts.updatedAt } : {}), metadata: { pending_id: opts.pendingId, question_id: opts.questionId, @@ -46,10 +52,21 @@ const fetchMock = vi.fn(); function setupFetchMock() { fetchMock.mockReset(); mockUpdateMessage.mockReset(); + mockMessagesBySession = {}; fetchMock.mockResolvedValue(new Response(JSON.stringify({ success: true }), { status: 200 })); globalThis.fetch = fetchMock as unknown as typeof globalThis.fetch; } +function deferred() { + let resolve!: (value: T) => void; + let reject!: (reason?: unknown) => void; + const promise = new Promise((resolvePromise, rejectPromise) => { + resolve = resolvePromise; + reject = rejectPromise; + }); + return { promise, resolve, reject }; +} + describe("useClarificationGroup — malformed conflict responses", () => { beforeEach(setupFetchMock); @@ -184,3 +201,406 @@ describe("useClarificationGroup — request generation guard", () => { expect(result.current.submitState).toBe("ok"); }); }); + +// eslint-disable-next-line max-lines-per-function -- inactive response races share one deterministic hook harness. +describe("useClarificationGroup — inactive response reconciliation", () => { + beforeEach(setupFetchMock); + + it("keeps a newer authoritative pending restoration when the inactive response settles late", async () => { + let resolveA: ((res: Response) => void) | null = null; + fetchMock.mockImplementationOnce( + () => new Promise((resolve) => (resolveA = resolve)), + ); + + const submitted = clarMessage({ + id: "m-a", + pendingId: "pA", + questionId: "qA", + index: 0, + total: 1, + updatedAt: "2026-05-04T00:00:00.000000001Z", + }); + const restored = { + ...submitted, + updated_at: "2026-05-04T00:00:00.000000002Z", + metadata: { ...submitted.metadata, status: "pending" as const }, + }; + mockMessagesBySession = { [submitted.session_id]: [submitted] }; + const onOutcome = vi.fn(); + const { result, rerender } = renderHook(({ msgs }) => useClarificationGroup(msgs, onOutcome), { + initialProps: { msgs: [submitted] }, + }); + + let pendingRequest!: Promise; + await act(async () => { + pendingRequest = result.current.skipAll(); + }); + + mockMessagesBySession = { [restored.session_id]: [restored] }; + rerender({ msgs: [restored] }); + + await act(async () => { + resolveA?.(new Response(null, { status: 409 })); + await pendingRequest; + }); + + expect(result.current.submitState).toBe("idle"); + expect(onOutcome).not.toHaveBeenCalled(); + expect(mockUpdateMessage).not.toHaveBeenCalled(); + + await act(async () => { + await result.current.skipAll(); + }); + expect(fetchMock).toHaveBeenCalledTimes(2); + expect(result.current.submitState).toBe("ok"); + }); + + it("hands affirmative inactive answers to ordinary message admission", async () => { + fetchMock.mockResolvedValueOnce( + new Response(JSON.stringify({ code: "not_active" }), { status: 409 }), + ); + const message = clarMessage({ + id: "m-late", + pendingId: "p-late", + questionId: "q-late", + index: 0, + total: 1, + }); + mockMessagesBySession = { [message.session_id]: [message] }; + const onLateAnswer = vi.fn().mockResolvedValue("sent" as const); + const onOutcome = vi.fn(); + const { result } = renderHook(() => useClarificationGroup([message], onOutcome, onLateAnswer)); + + await act(async () => { + await result.current.submitCollected({ + "q-late": { question_id: "q-late", selected_options: ["o1"] }, + }); + }); + + expect(onLateAnswer).toHaveBeenCalledWith({ + messages: [message], + answers: [{ question_id: "q-late", selected_options: ["o1"] }], + }); + expect(result.current.lateAnswerState).toBe("sent"); + expect(mockUpdateMessage).toHaveBeenCalledWith( + expect.objectContaining({ + id: "m-late", + metadata: expect.objectContaining({ status: "expired" }), + }), + ); + expect(onOutcome).toHaveBeenCalledWith({ + kind: "late_message_admitted", + delivery: "sent", + }); + }); + + it("retains the answer and reuses the late admission after a failed fallback", async () => { + fetchMock.mockResolvedValueOnce( + new Response(JSON.stringify({ code: "not_active" }), { status: 409 }), + ); + const message = clarMessage({ + id: "m-retry", + pendingId: "p-retry", + questionId: "q-retry", + index: 0, + total: 1, + }); + mockMessagesBySession = { [message.session_id]: [message] }; + const onLateAnswer = vi + .fn() + .mockRejectedValueOnce(new Error("message unavailable")) + .mockResolvedValueOnce("queued" as const); + const { result } = renderHook(() => useClarificationGroup([message], undefined, onLateAnswer)); + + await act(async () => { + await result.current.submitCollected({ + "q-retry": { question_id: "q-retry", custom_text: "Keep the draft" }, + }); + }); + expect(result.current.lateAnswerState).toBe("error"); + expect(result.current.answers["q-retry"]).toEqual({ + question_id: "q-retry", + custom_text: "Keep the draft", + }); + expect(mockUpdateMessage).not.toHaveBeenCalled(); + + await act(async () => { + await result.current.retryLateAnswer(); + }); + expect(onLateAnswer).toHaveBeenCalledTimes(2); + expect(onLateAnswer.mock.calls[1][0]).toEqual(onLateAnswer.mock.calls[0][0]); + expect(result.current.lateAnswerState).toBe("queued"); + }); + + it("ignores a late retry rejection after the bundle changes", async () => { + fetchMock.mockResolvedValueOnce( + new Response(JSON.stringify({ code: "not_active" }), { status: 409 }), + ); + const bundleA = [ + clarMessage({ id: "m-retry-a", pendingId: "pA", questionId: "qA", index: 0, total: 1 }), + ]; + const bundleB = [ + clarMessage({ id: "m-retry-b", pendingId: "pB", questionId: "qB", index: 0, total: 1 }), + ]; + mockMessagesBySession = { [bundleA[0].session_id]: bundleA }; + const retry = deferred<"sent">(); + const onLateAnswer = vi + .fn() + .mockRejectedValueOnce(new Error("message unavailable")) + .mockReturnValueOnce(retry.promise); + const { result, rerender } = renderHook( + ({ msgs }) => useClarificationGroup(msgs, undefined, onLateAnswer), + { + initialProps: { msgs: bundleA }, + }, + ); + + await act(async () => { + await result.current.submitCollected({ + qA: { question_id: "qA", selected_options: ["a"] }, + }); + }); + expect(result.current.lateAnswerState).toBe("error"); + + let retryRequest!: Promise; + await act(async () => { + retryRequest = result.current.retryLateAnswer(); + await waitFor(() => expect(onLateAnswer).toHaveBeenCalledTimes(2)); + }); + rerender({ msgs: bundleB }); + + await act(async () => { + retry.reject(new Error("late failure")); + await retryRequest; + }); + + expect(result.current.lateAnswerState).toBe("idle"); + }); + + it("ignores a late retry success after A leaves and returns as a new generation", async () => { + fetchMock.mockResolvedValueOnce( + new Response(JSON.stringify({ code: "not_active" }), { status: 409 }), + ); + const bundleA = [ + clarMessage({ + id: "m-retry-generation-a", + pendingId: "pA", + questionId: "qA", + index: 0, + total: 1, + }), + ]; + const bundleB = [ + clarMessage({ + id: "m-retry-generation-b", + pendingId: "pB", + questionId: "qB", + index: 0, + total: 1, + }), + ]; + mockMessagesBySession = { [bundleA[0].session_id]: bundleA }; + const retry = deferred<"sent">(); + const onLateAnswer = vi + .fn() + .mockRejectedValueOnce(new Error("message unavailable")) + .mockReturnValueOnce(retry.promise); + const onOutcome = vi.fn(); + const { result, rerender } = renderHook( + ({ msgs }) => useClarificationGroup(msgs, onOutcome, onLateAnswer), + { initialProps: { msgs: bundleA } }, + ); + + await act(async () => { + await result.current.submitCollected({ + qA: { question_id: "qA", selected_options: ["a"] }, + }); + }); + let retryRequest!: Promise; + await act(async () => { + retryRequest = result.current.retryLateAnswer(); + await waitFor(() => expect(onLateAnswer).toHaveBeenCalledTimes(2)); + }); + rerender({ msgs: bundleB }); + mockMessagesBySession = { [bundleA[0].session_id]: bundleA }; + rerender({ msgs: bundleA }); + + await act(async () => { + retry.resolve("sent"); + await retryRequest; + }); + + expect(result.current.lateAnswerState).toBe("idle"); + expect(mockUpdateMessage).not.toHaveBeenCalled(); + expect(onOutcome).not.toHaveBeenCalled(); + }); + + it("expires unchanged siblings while preserving a newer restored row", async () => { + let resolveA: ((res: Response) => void) | null = null; + fetchMock.mockImplementationOnce( + () => new Promise((resolve) => (resolveA = resolve)), + ); + + const submitted = [ + clarMessage({ + id: "m-restored", + pendingId: "pA", + questionId: "q-restored", + index: 0, + total: 2, + updatedAt: "2026-05-04T00:00:00.000000001Z", + }), + clarMessage({ + id: "m-sibling", + pendingId: "pA", + questionId: "q-sibling", + index: 1, + total: 2, + updatedAt: "2026-05-04T00:00:00.000000001Z", + }), + ]; + const restored = { + ...submitted[0], + updated_at: "2026-05-04T00:00:00.000000002Z", + metadata: { ...submitted[0].metadata, status: "pending" as const }, + }; + mockMessagesBySession = { [submitted[0].session_id]: [restored, submitted[1]] }; + const { result, rerender } = renderHook(({ msgs }) => useClarificationGroup(msgs), { + initialProps: { msgs: submitted }, + }); + + let pendingRequest!: Promise; + await act(async () => { + pendingRequest = result.current.skipAll(); + }); + rerender({ msgs: [restored, submitted[1]] }); + + await act(async () => { + resolveA?.(new Response(null, { status: 409 })); + await pendingRequest; + }); + + expect(mockUpdateMessage).toHaveBeenCalledTimes(1); + expect(mockUpdateMessage).toHaveBeenCalledWith({ + ...submitted[1], + metadata: { ...submitted[1].metadata, status: "expired" }, + }); + }); + + it("does not normalize an invalid current timestamp as newer authority", async () => { + fetchMock.mockResolvedValueOnce(new Response(null, { status: 409 })); + const submitted = clarMessage({ + id: "m-invalid", + pendingId: "p-invalid", + questionId: "q-invalid", + index: 0, + total: 1, + updatedAt: "2026-02-28T00:00:00.000000001Z", + }); + const invalidCurrent = { + ...submitted, + updated_at: "2026-02-30T00:00:00.000000002Z", + metadata: { ...submitted.metadata, status: "pending" as const }, + }; + mockMessagesBySession = { [submitted.session_id]: [invalidCurrent] }; + const { result, rerender } = renderHook(({ msgs }) => useClarificationGroup(msgs), { + initialProps: { msgs: [submitted] }, + }); + rerender({ msgs: [invalidCurrent] }); + + await act(async () => { + await result.current.skipAll(); + }); + + expect(result.current.submitState).toBe("expired"); + expect(mockUpdateMessage).toHaveBeenCalledWith({ + ...invalidCurrent, + metadata: { ...invalidCurrent.metadata, status: "expired" }, + }); + }); + + it("does not expire bundle A after it leaves and returns to the active generation", async () => { + let resolveA: ((res: Response) => void) | null = null; + fetchMock.mockImplementationOnce( + () => new Promise((resolve) => (resolveA = resolve)), + ); + + const bundle = (pendingId: string) => [ + clarMessage({ id: "m-shared", pendingId, questionId: "q1", index: 0, total: 1 }), + ]; + const bundleA = bundle("pA"); + const bundleB = bundle("pB"); + mockMessagesBySession = { [bundleA[0].session_id]: bundleA }; + const { result, rerender } = renderHook(({ msgs }) => useClarificationGroup(msgs), { + initialProps: { msgs: bundleA }, + }); + + let pendingRequest!: Promise; + await act(async () => { + pendingRequest = result.current.skipAll(); + }); + rerender({ msgs: bundleB }); + mockMessagesBySession = { [bundleA[0].session_id]: bundleA }; + rerender({ msgs: bundleA }); + + await act(async () => { + resolveA?.(new Response(null, { status: 409 })); + await pendingRequest; + }); + + expect(result.current.submitState).toBe("idle"); + expect(mockUpdateMessage).not.toHaveBeenCalled(); + }); + + it("keeps bundle B active when bundle A settles late with an inactive response", async () => { + let resolveA: ((res: Response) => void) | null = null; + fetchMock.mockImplementationOnce( + () => new Promise((resolve) => (resolveA = resolve)), + ); + fetchMock.mockResolvedValueOnce( + new Response(JSON.stringify({ success: true }), { status: 200 }), + ); + + const bundleA = [ + clarMessage({ id: "m-a", pendingId: "pA", questionId: "qA", index: 0, total: 1 }), + ]; + const bundleB = [ + clarMessage({ id: "m-b", pendingId: "pB", questionId: "qB", index: 0, total: 1 }), + ]; + mockMessagesBySession = { [bundleA[0].session_id]: [...bundleA, ...bundleB] }; + const onOutcome = vi.fn(); + const { result, rerender } = renderHook(({ msgs }) => useClarificationGroup(msgs, onOutcome), { + initialProps: { msgs: bundleA }, + }); + + let firstSkip!: Promise; + await act(async () => { + firstSkip = result.current.skipAll(); + }); + rerender({ msgs: bundleB }); + + let secondSubmit!: Promise; + await act(async () => { + secondSubmit = result.current.submitCollected({ + qB: { question_id: "qB", selected_options: ["second"] }, + }); + await secondSubmit; + }); + expect(result.current.submitState).toBe("ok"); + expect(onOutcome).toHaveBeenCalledTimes(1); + + await act(async () => { + resolveA?.(new Response(null, { status: 409 })); + await firstSkip; + }); + + expect(result.current.submitState).toBe("ok"); + expect(onOutcome).toHaveBeenCalledTimes(1); + expect(mockUpdateMessage).toHaveBeenCalledWith( + expect.objectContaining({ + id: "m-a", + metadata: expect.objectContaining({ status: "expired" }), + }), + ); + }); +}); diff --git a/apps/web/hooks/domains/session/use-clarification-group.test.ts b/apps/web/hooks/domains/session/use-clarification-group.test.ts index 6748d2ddf0..06264ba154 100644 --- a/apps/web/hooks/domains/session/use-clarification-group.test.ts +++ b/apps/web/hooks/domains/session/use-clarification-group.test.ts @@ -1,3 +1,5 @@ +/* eslint-disable max-lines -- clarification lifecycle cases share one deterministic hook harness. */ + import { describe, it, expect, vi, beforeEach } from "vitest"; import { act, renderHook } from "@testing-library/react"; import { sessionId as toSessionId, taskId as toTaskId, type Message } from "@/lib/types/http"; @@ -7,9 +9,13 @@ vi.mock("@/lib/config", () => ({ })); const mockUpdateMessage = vi.fn(); +let mockMessagesBySession: Record = {}; vi.mock("@/components/state-provider", () => ({ useAppStoreApi: () => ({ - getState: () => ({ updateMessage: mockUpdateMessage }), + getState: () => ({ + updateMessage: mockUpdateMessage, + messages: { bySession: mockMessagesBySession }, + }), }), })); @@ -50,6 +56,7 @@ function successResponse(): Response { function setupFetchMock() { fetchMock.mockReset(); mockUpdateMessage.mockReset(); + mockMessagesBySession = {}; fetchMock.mockResolvedValue(successResponse()); globalThis.fetch = fetchMock as unknown as typeof globalThis.fetch; } @@ -399,6 +406,7 @@ describe("useClarificationGroup — retry", () => { // pending bundle until the user refreshes. To stay robust against that we mark // each bundle message as answered/rejected in the store the moment the HTTP POST // resolves, mirroring the backend update the WS event would have delivered. +// eslint-disable-next-line max-lines-per-function -- optimistic store update cases share one store harness. describe("useClarificationGroup — optimistic store update on resolve", () => { beforeEach(setupFetchMock); @@ -467,6 +475,101 @@ describe("useClarificationGroup — optimistic store update on resolve", () => { expect(result.current.submitState).toBe("error"); expect(mockUpdateMessage).not.toHaveBeenCalled(); }); + + it("skipAll expires only matching pending rows from the latest message cache", async () => { + fetchMock.mockResolvedValueOnce( + new Response(JSON.stringify({ code: "not_active" }), { status: 409 }), + ); + const submitted = clarMessage({ + id: "m-pending", + pendingId: "p1", + questionId: "q1", + index: 0, + total: 1, + }); + const latestSubmitted = { + ...submitted, + content: "newer content from the message cache", + metadata: { ...submitted.metadata, context: "newer context" }, + }; + const terminalSiblingPending = clarMessage({ + id: "m-terminal", + pendingId: "p1", + questionId: "q2", + index: 1, + total: 2, + }); + const terminalSibling = { + ...terminalSiblingPending, + metadata: { ...terminalSiblingPending.metadata, status: "rejected" as const }, + }; + const deletedSubmitted = clarMessage({ + id: "m-deleted", + pendingId: "p1", + questionId: "q-deleted", + index: 1, + total: 2, + }); + const unrelatedBundle = clarMessage({ + id: "m-other", + pendingId: "p2", + questionId: "q-other", + index: 0, + total: 1, + }); + mockMessagesBySession = { + [submitted.session_id]: [latestSubmitted, terminalSibling, unrelatedBundle], + }; + + const { result } = renderHook(() => useClarificationGroup([submitted, deletedSubmitted])); + + await act(async () => { + await result.current.skipAll(); + }); + + expect(result.current.submitState).toBe("expired"); + expect(mockUpdateMessage).toHaveBeenCalledTimes(1); + expect(mockUpdateMessage).toHaveBeenCalledWith({ + ...latestSubmitted, + metadata: { ...latestSubmitted.metadata, status: "expired" }, + }); + }); + + it("blocks direct retries after expiry until the same bundle is restored as pending", async () => { + fetchMock.mockResolvedValueOnce(new Response(null, { status: 409 })); + fetchMock.mockResolvedValueOnce(successResponse()); + const message = clarMessage({ + id: "m1", + pendingId: "p1", + questionId: "q1", + index: 0, + total: 1, + }); + mockMessagesBySession = { [message.session_id]: [message] }; + const { result, rerender } = renderHook(({ msgs }) => useClarificationGroup(msgs), { + initialProps: { msgs: [message] }, + }); + + await act(async () => { + await result.current.skipAll(); + await result.current.retry(); + await result.current.skipAll(); + }); + expect(fetchMock).toHaveBeenCalledTimes(1); + + const expiredMessage = { + ...message, + metadata: { ...message.metadata, status: "expired" as const }, + }; + rerender({ msgs: [expiredMessage] }); + rerender({ msgs: [message] }); + + await act(async () => { + await result.current.skipAll(); + }); + expect(fetchMock).toHaveBeenCalledTimes(2); + expect(result.current.submitState).toBe("ok"); + }); }); describe("useClarificationGroup — optimistic store update edge cases", () => { diff --git a/apps/web/hooks/domains/session/use-clarification-group.ts b/apps/web/hooks/domains/session/use-clarification-group.ts index 291fa71eaf..f1599c4e49 100644 --- a/apps/web/hooks/domains/session/use-clarification-group.ts +++ b/apps/web/hooks/domains/session/use-clarification-group.ts @@ -1,12 +1,19 @@ "use client"; +/* eslint-disable max-lines, max-depth -- clarification submission and recovery share one wire contract. */ + import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { useTranslation } from "react-i18next"; import type { ClarificationAnswer, ClarificationRequestMetadata, Message } from "@/lib/types/http"; import { getBackendConfig } from "@/lib/config"; import { useAppStoreApi } from "@/components/state-provider"; +import { isPendingClarificationMessage } from "@/lib/utils/pending-clarification"; +import { parseTurnTimestamp } from "@/lib/state/slices/session/turn-actions"; +import type { LateClarificationSnapshot } from "@/lib/clarification/late-clarification-message"; type SubmitState = "idle" | "submitting" | "ok" | "error" | "expired"; +export type LateAnswerDelivery = "sent" | "queued"; +export type LateAnswerState = "idle" | "sending" | "sent" | "queued" | "error"; // The only stable machine-readable 409 cause the backend sends today // (internal/clarification/handlers.go's writeResolutionResult, guarded by @@ -117,6 +124,7 @@ function parseClarificationResponseBody(value: unknown): ClarificationRespondRes export type ClarificationOutcome = | { kind: "resolved"; claimedByThisCaller: boolean; status?: ResolvedStatus } | { kind: "no_longer_active" } + | { kind: "late_message_admitted"; delivery: LateAnswerDelivery } | { kind: "submission_failed" }; function clarificationOutcome(result: ClarificationRespondResult): ClarificationOutcome { @@ -135,6 +143,7 @@ export type ClarificationGroupApi = { answeredCount: number; answers: Record; submitState: SubmitState; + lateAnswerState: LateAnswerState; recordAnswer: (questionId: string, answer: ClarificationAnswer) => void; clearAnswer: (questionId: string) => void; // Submits every recorded answer in a single batch. An optional `override` @@ -148,6 +157,7 @@ export type ClarificationGroupApi = { // retries use the current live answers; skip retries keep the original // reason. A no-op before either has been called. retry: () => Promise; + retryLateAnswer: () => Promise; // The most recent settled ClarificationRespondResult this submission still // owned (see runClarificationRequest's ownsRequest fence), or null before // any submission has settled. Callers that need to distinguish "this @@ -172,6 +182,30 @@ function questionIdsFromMessages(messages: readonly Message[]): string[] { .filter(Boolean); } +function clarificationBundleStateKey(messages: readonly Message[] | null | undefined): string { + return JSON.stringify( + (messages ?? []).map((message) => { + const metadata = message.metadata as ClarificationRequestMetadata | undefined; + return [ + message.session_id, + message.id, + metadata?.pending_id ?? null, + metadata?.status ?? null, + ]; + }), + ); +} + +function hasNewerMessageVersion(current: Message, submitted: Message): boolean { + if (!current.updated_at) return false; + if (!submitted.updated_at) return true; + + const currentTime = parseTurnTimestamp(current.updated_at); + const submittedTime = parseTurnTimestamp(submitted.updated_at); + if (currentTime === null || submittedTime === null) return false; + return currentTime > submittedTime; +} + // classifyConflictResult reads a 409 response's body for a machine-readable // `code` (added alongside the existing human `error` string). A bodyless 409 // (legacy backend) or an explicit "not_active" code both mean the bundle is @@ -293,14 +327,20 @@ type RunClarificationRequestArgs = { requestGenerationRef: { current: number }; inflightRef: { current: boolean }; setSubmitState: (state: SubmitState) => void; - setLastResult: (result: ClarificationRespondResult) => void; + setLastResult: (result: ClarificationRespondResult | null) => void; + setLateAnswerState: (state: LateAnswerState) => void; + lateAnswerSnapshotRef: { current: LateClarificationSnapshot | null }; + onLateAnswer?: (snapshot: LateClarificationSnapshot) => Promise; onOutcome?: (outcome: ClarificationOutcome) => void; + inactivePendingIdRef: { current: string | null }; + getLatestMessage: (sessionId: string, messageId: string) => Message | undefined; updateMessage: (message: Message) => void; }; // Shared submit/skip plumbing: guard re-entry, POST, then apply the // optimistic update on success. Extracted so submitCollected and skipAll // (below) each stay a short wrapper around their own POST + own-answer shape. +// eslint-disable-next-line max-lines-per-function, complexity, sonarjs/cognitive-complexity -- one request owns the response and late-message fallback fences. async function runClarificationRequest(args: RunClarificationRequestArgs) { const { post, @@ -313,7 +353,12 @@ async function runClarificationRequest(args: RunClarificationRequestArgs) { inflightRef, setSubmitState, setLastResult, + setLateAnswerState, + lateAnswerSnapshotRef, + onLateAnswer, onOutcome, + inactivePendingIdRef, + getLatestMessage, updateMessage, } = args; const requestGeneration = ++requestGenerationRef.current; @@ -324,10 +369,52 @@ async function runClarificationRequest(args: RunClarificationRequestArgs) { setSubmitState("submitting"); try { const result = await post(); - if (ownsRequest()) { - setSubmitState(result.state); + const hasNewerAuthority = + result.state === "expired" && + hasNewerAuthoritativeMessage(bundle, requestPendingId, getLatestMessage); + const canFallbackToLateMessage = + ownsRequest() && + !hasNewerAuthority && + result.state === "expired" && + ownStatus === "answered" && + onLateAnswer !== undefined; + if (ownsRequest() && !hasNewerAuthority) { setLastResult(result); - onOutcome?.(clarificationOutcome(result)); + if (canFallbackToLateMessage) { + const snapshot: LateClarificationSnapshot = { + messages: bundle.slice(), + answers: Object.values(ownAnswers), + }; + lateAnswerSnapshotRef.current = snapshot; + inactivePendingIdRef.current = requestPendingId; + setSubmitState("idle"); + setLateAnswerState("sending"); + try { + const delivery = await onLateAnswer(snapshot); + if (ownsRequest()) { + setLateAnswerState(delivery); + safeApplyExpiredStatus({ + bundle, + pendingId: requestPendingId, + requestGeneration, + activePendingIdRef, + requestGenerationRef, + getLatestMessage, + update: updateMessage, + }); + onOutcome?.({ kind: "late_message_admitted", delivery }); + } + } catch { + if (ownsRequest()) setLateAnswerState("error"); + } + } else { + setSubmitState(result.state); + onOutcome?.(clarificationOutcome(result)); + if (result.state === "expired") inactivePendingIdRef.current = requestPendingId; + } + } else if (ownsRequest() && hasNewerAuthority) { + setSubmitState("idle"); + setLastResult(null); } if (result.state === "ok") { // Applies against the submit-time bundle snapshot regardless of which @@ -339,6 +426,16 @@ async function runClarificationRequest(args: RunClarificationRequestArgs) { ownAnswers, ); safeApplyResolvedStatus(bundle, status, answersByQuestionId, updateMessage); + } else if (result.state === "expired" && !canFallbackToLateMessage) { + safeApplyExpiredStatus({ + bundle, + pendingId: requestPendingId, + requestGeneration, + activePendingIdRef, + requestGenerationRef, + getLatestMessage, + update: updateMessage, + }); } } catch (err) { console.error("Clarification request threw:", err); @@ -393,6 +490,73 @@ function safeApplyResolvedStatus( } } +type ExpiredStatusReconciliationArgs = { + bundle: readonly Message[]; + pendingId: string; + requestGeneration: number; + activePendingIdRef: { current: string | null }; + requestGenerationRef: { current: number }; + getLatestMessage: (sessionId: string, messageId: string) => Message | undefined; + update: (message: Message) => void; +}; + +function applyExpiredStatusToBundle({ + bundle, + pendingId, + requestGeneration, + activePendingIdRef, + requestGenerationRef, + getLatestMessage, + update, +}: ExpiredStatusReconciliationArgs) { + if ( + activePendingIdRef.current === pendingId && + requestGenerationRef.current !== requestGeneration + ) + return; + + for (const submitted of bundle) { + const submittedMeta = submitted.metadata as ClarificationRequestMetadata | undefined; + if (submittedMeta?.pending_id !== pendingId) continue; + + const current = getLatestMessage(submitted.session_id, submitted.id); + if (!current || current.session_id !== submitted.session_id) continue; + const currentMeta = current.metadata as ClarificationRequestMetadata | undefined; + if (currentMeta?.pending_id !== pendingId || !isPendingClarificationMessage(current)) continue; + if (hasNewerMessageVersion(current, submitted)) continue; + + update({ + ...current, + metadata: { ...currentMeta, status: "expired" }, + }); + } +} + +function safeApplyExpiredStatus(args: ExpiredStatusReconciliationArgs) { + if (args.bundle.length === 0) return; + try { + applyExpiredStatusToBundle(args); + } catch (err) { + console.error("Clarification expired update threw:", err); + } +} + +function hasNewerAuthoritativeMessage( + bundle: readonly Message[], + pendingId: string, + getLatestMessage: (sessionId: string, messageId: string) => Message | undefined, +): boolean { + return bundle.some((submitted) => { + const submittedMeta = submitted.metadata as ClarificationRequestMetadata | undefined; + if (submittedMeta?.pending_id !== pendingId) return false; + + const current = getLatestMessage(submitted.session_id, submitted.id); + if (!current || current.session_id !== submitted.session_id) return false; + const currentMeta = current.metadata as ClarificationRequestMetadata | undefined; + return currentMeta?.pending_id === pendingId && hasNewerMessageVersion(current, submitted); + }); +} + type UseClarificationSubmissionArgs = { pendingId: string | null; questionIds: string[]; @@ -403,8 +567,13 @@ type UseClarificationSubmissionArgs = { inflightRef: { current: boolean }; setAnswers: (answers: Record) => void; setSubmitState: (state: SubmitState) => void; - setLastResult: (result: ClarificationRespondResult) => void; + setLastResult: (result: ClarificationRespondResult | null) => void; + setLateAnswerState: (state: LateAnswerState) => void; + lateAnswerSnapshotRef: { current: LateClarificationSnapshot | null }; + onLateAnswer?: (snapshot: LateClarificationSnapshot) => Promise; onOutcome?: (outcome: ClarificationOutcome) => void; + inactivePendingIdRef: { current: string | null }; + getLatestMessage: (sessionId: string, messageId: string) => Message | undefined; updateMessage: (message: Message) => void; defaultSkipReason: string; }; @@ -424,7 +593,12 @@ function baseClarificationRequestArgs( inflightRef: common.inflightRef, setSubmitState: common.setSubmitState, setLastResult: common.setLastResult, + setLateAnswerState: common.setLateAnswerState, + lateAnswerSnapshotRef: common.lateAnswerSnapshotRef, + onLateAnswer: common.onLateAnswer, onOutcome: common.onOutcome, + inactivePendingIdRef: common.inactivePendingIdRef, + getLatestMessage: common.getLatestMessage, updateMessage: common.updateMessage, }; } @@ -433,6 +607,7 @@ function baseClarificationRequestArgs( // each POST through runClarificationRequest, and retry() replays whichever of // the two was last attempted. Submit retries read the live answer map so edits // made after a failure are included; skip retries keep their original reason. +// eslint-disable-next-line max-lines-per-function -- submit, skip, and retry share one generation fence. function useClarificationSubmission(args: UseClarificationSubmissionArgs) { const { pendingId, @@ -445,7 +620,12 @@ function useClarificationSubmission(args: UseClarificationSubmissionArgs) { setAnswers, setSubmitState, setLastResult, + setLateAnswerState, + lateAnswerSnapshotRef, + onLateAnswer, onOutcome, + inactivePendingIdRef, + getLatestMessage, updateMessage, defaultSkipReason, } = args; @@ -454,6 +634,7 @@ function useClarificationSubmission(args: UseClarificationSubmissionArgs) { const submitCollected = useCallback( async (override?: Record) => { if (!pendingId) return; + if (inactivePendingIdRef.current === pendingId) return; if (inflightRef.current) return; const current = { ...answersRef.current, ...(override ?? {}) }; const haveAll = questionIds.every((id) => Boolean(current[id])); @@ -485,7 +666,12 @@ function useClarificationSubmission(args: UseClarificationSubmissionArgs) { setAnswers, setSubmitState, setLastResult, + setLateAnswerState, + lateAnswerSnapshotRef, + onLateAnswer, onOutcome, + inactivePendingIdRef, + getLatestMessage, updateMessage, ], ); @@ -493,6 +679,7 @@ function useClarificationSubmission(args: UseClarificationSubmissionArgs) { const skipAll = useCallback( async (reason?: string) => { if (!pendingId) return; + if (inactivePendingIdRef.current === pendingId) return; if (inflightRef.current) return; const effectiveReason = reason ?? defaultSkipReason; lastActionRef.current = { kind: "skip", reason: effectiveReason }; @@ -511,7 +698,12 @@ function useClarificationSubmission(args: UseClarificationSubmissionArgs) { inflightRef, setSubmitState, setLastResult, + setLateAnswerState, + lateAnswerSnapshotRef, + onLateAnswer, onOutcome, + inactivePendingIdRef, + getLatestMessage, updateMessage, defaultSkipReason, ], @@ -544,9 +736,11 @@ function useClarificationSubmission(args: UseClarificationSubmissionArgs) { // Decision A is preserved (per-question commit, batched on the wire) but the // final submit is no longer implicit — the user clicks "Submit answers" or // presses ArrowRight on the last step. +// eslint-disable-next-line max-lines-per-function -- owns the complete clarification lifecycle and restoration fence. export function useClarificationGroup( messages: readonly Message[] | null | undefined, onOutcome?: (outcome: ClarificationOutcome) => void, + onLateAnswer?: (snapshot: LateClarificationSnapshot) => Promise, ): ClarificationGroupApi { const { t } = useTranslation(); const storeApi = useAppStoreApi(); @@ -556,13 +750,16 @@ export function useClarificationGroup( answersRef.current = answers; }, [answers]); const [submitState, setSubmitState] = useState("idle"); + const [lateAnswerState, setLateAnswerState] = useState("idle"); const [lastResult, setLastResult] = useState(null); + const lateAnswerSnapshotRef = useRef(null); // Re-entry guard: multiple submit paths can race (Cmd+Enter inside the // custom input fires both the input's onSubmit and onRequestFinalSubmit; // a double-click on the Submit button can also race). The hook owns the // guarantee that only one POST is in flight at a time. const inflightRef = useRef(false); const requestGenerationRef = useRef(0); + const inactivePendingIdRef = useRef(null); const pendingId = useMemo(() => { if (!messages || messages.length === 0) return null; @@ -602,6 +799,17 @@ export function useClarificationGroup( // current (see runClarificationRequest). const activePendingIdRef = useRef(pendingId); activePendingIdRef.current = pendingId; + const bundleStateKey = useMemo(() => clarificationBundleStateKey(messages), [messages]); + const activeBundleStateKeyRef = useRef(bundleStateKey); + activeBundleStateKeyRef.current = bundleStateKey; + + const getLatestMessage = useCallback( + (sessionId: string, messageId: string) => { + const messages = storeApi.getState().messages?.bySession?.[sessionId]; + return messages?.find((message) => message.id === messageId); + }, + [storeApi], + ); // i18n-exempt: the default reason is POSTed as the clarification answer and // reaches the agent verbatim; it is not rendered in the UI. @@ -616,11 +824,65 @@ export function useClarificationGroup( setAnswers, setSubmitState, setLastResult, + setLateAnswerState, + lateAnswerSnapshotRef, + onLateAnswer, onOutcome, + inactivePendingIdRef, + getLatestMessage, updateMessage: storeApi.getState().updateMessage, defaultSkipReason: t("task:userSkippedClarification"), }); + const retryLateAnswer = useCallback(async () => { + const snapshot = lateAnswerSnapshotRef.current; + if (!snapshot || !onLateAnswer || !pendingId) return; + const currentAnswers = Object.values(answersRef.current); + const nextSnapshot = { ...snapshot, answers: currentAnswers }; + const requestPendingId = pendingId; + const requestGeneration = requestGenerationRef.current; + const requestBundle = submitBundleRef.current.slice(); + const requestBundleStateKey = bundleStateKey; + lateAnswerSnapshotRef.current = nextSnapshot; + setLateAnswerState("sending"); + const ownsRetry = () => + activePendingIdRef.current === requestPendingId && + requestGenerationRef.current === requestGeneration && + activeBundleStateKeyRef.current === requestBundleStateKey && + lateAnswerSnapshotRef.current === nextSnapshot; + try { + const delivery = await onLateAnswer(nextSnapshot); + if (!ownsRetry()) return; + setLateAnswerState(delivery); + if (!ownsRetry()) return; + safeApplyExpiredStatus({ + bundle: requestBundle, + pendingId: requestPendingId, + requestGeneration, + activePendingIdRef, + requestGenerationRef, + getLatestMessage, + update: storeApi.getState().updateMessage, + }); + if (!ownsRetry()) return; + onOutcome?.({ kind: "late_message_admitted", delivery }); + } catch { + if (ownsRetry()) setLateAnswerState("error"); + } + }, [ + activePendingIdRef, + answersRef, + bundleStateKey, + getLatestMessage, + onLateAnswer, + onOutcome, + pendingId, + requestGenerationRef, + setLateAnswerState, + storeApi, + submitBundleRef, + ]); + // A new bundle (different pendingId) replacing a still-pending one must not // inherit the previous bundle's answers, submit/retry banner, or replayable // action -- bundle-swap-without-unmount is a designed-for path (see @@ -639,23 +901,64 @@ export function useClarificationGroup( answersRef.current = {}; setAnswers({}); setSubmitState("idle"); + setLateAnswerState("idle"); setLastResult(null); + lateAnswerSnapshotRef.current = null; resetLastAction(); inflightRef.current = false; + inactivePendingIdRef.current = null; } }, [pendingId, resetLastAction, inflightRef]); + const lastBundleStateKeyRef = useRef(bundleStateKey); + useEffect(() => { + const changed = lastBundleStateKeyRef.current !== bundleStateKey; + lastBundleStateKeyRef.current = bundleStateKey; + if (!changed || inactivePendingIdRef.current !== pendingId || !pendingId) return; + + const restored = messages?.some((message) => { + const metadata = message.metadata as ClarificationRequestMetadata | undefined; + if (metadata?.pending_id !== pendingId || !isPendingClarificationMessage(message)) + return false; + const current = getLatestMessage(message.session_id, message.id); + return current ? isPendingClarificationMessage(current) : false; + }); + if (!restored) return; + + requestGenerationRef.current += 1; + inactivePendingIdRef.current = null; + answersRef.current = {}; + setAnswers({}); + setSubmitState("idle"); + setLateAnswerState("idle"); + setLastResult(null); + lateAnswerSnapshotRef.current = null; + resetLastAction(); + inflightRef.current = false; + }, [ + answersRef, + bundleStateKey, + getLatestMessage, + inflightRef, + messages, + pendingId, + requestGenerationRef, + resetLastAction, + ]); + return { pendingId, total, answeredCount, answers, submitState, + lateAnswerState, recordAnswer, clearAnswer, submitCollected, skipAll, retry, + retryLateAnswer, lastResult, }; } diff --git a/apps/web/hooks/use-late-clarification-message.test.tsx b/apps/web/hooks/use-late-clarification-message.test.tsx new file mode 100644 index 0000000000..21a713eda6 --- /dev/null +++ b/apps/web/hooks/use-late-clarification-message.test.tsx @@ -0,0 +1,218 @@ +import { act, cleanup, renderHook, waitFor } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { + sessionId as toSessionId, + taskId as toTaskId, + type Message, + type TaskSession, +} from "@/lib/types/http"; +import { + useLateClarificationMessage, + type LateClarificationSnapshot, +} from "./use-late-clarification-message"; + +const admissionMock = vi.hoisted(() => vi.fn()); +const fetchTaskSessionMock = vi.hoisted(() => vi.fn()); +const listTaskSessionMessagesMock = vi.hoisted(() => vi.fn()); +const storeRef = vi.hoisted(() => ({ current: null as TestStore | null })); +const TASK_ID = "task-late-answer"; +const SESSION_ID = "session-late-answer"; +const QUESTION_ID = "question-1"; + +vi.mock("react-i18next", () => ({ + useTranslation: () => ({ t: (key: string) => key }), +})); + +vi.mock("@/components/state-provider", () => ({ + useAppStoreApi: () => ({ getState: () => storeRef.current }), +})); + +vi.mock("@/hooks/use-message-handler", () => ({ + useMessageHandler: () => ({ handleSendMessageWithOutcome: admissionMock }), +})); + +vi.mock("@/lib/api/domains/session-api", () => ({ + fetchTaskSession: fetchTaskSessionMock, + listTaskSessionMessages: listTaskSessionMessagesMock, +})); + +type TestStore = { + taskSessions: { items: Record }; + messages: { bySession: Record }; + setTaskSession: (session: TaskSession) => void; + mergeMessages: (sessionId: string, messages: Message[]) => void; +}; + +function deferred() { + let resolve!: (value: T) => void; + let reject!: (reason?: unknown) => void; + const promise = new Promise((resolvePromise, rejectPromise) => { + resolve = resolvePromise; + reject = rejectPromise; + }); + return { promise, resolve, reject }; +} + +function sourceMessage(pendingId: string): Message { + return { + id: `message-${pendingId}`, + task_id: toTaskId(TASK_ID), + session_id: toSessionId(SESSION_ID), + author_type: "agent", + type: "clarification_request", + content: "Choose one", + created_at: "2026-09-19T00:00:00Z", + metadata: { + pending_id: pendingId, + question_id: QUESTION_ID, + question_index: 0, + question_total: 1, + status: "expired", + question: { + id: QUESTION_ID, + prompt: "Choose one", + options: [{ option_id: "option-1", label: "Option one" }], + }, + }, + } as unknown as Message; +} + +function sourceSession(): TaskSession { + return { + id: toSessionId(SESSION_ID), + task_id: toTaskId(TASK_ID), + state: "IDLE", + started_at: "2026-09-19T00:00:00Z", + updated_at: "2026-09-19T00:00:00Z", + } as TaskSession; +} + +function resetStore() { + const next: TestStore = { + taskSessions: { items: {} }, + messages: { bySession: {} }, + setTaskSession: (session) => { + next.taskSessions.items[session.id] = session; + }, + mergeMessages: (sessionId, messages) => { + next.messages.bySession[sessionId] = messages; + }, + }; + storeRef.current = next; +} + +beforeEach(() => { + resetStore(); + admissionMock.mockReset(); + fetchTaskSessionMock.mockReset(); + listTaskSessionMessagesMock.mockReset(); +}); + +afterEach(() => { + cleanup(); + vi.clearAllMocks(); +}); + +describe("useLateClarificationMessage", () => { + it("retains a failed draft and client identity across unmount and remount", async () => { + const source = sourceMessage("pending-remount"); + const state = storeRef.current!; + state.taskSessions.items[source.session_id] = sourceSession(); + state.messages.bySession[source.session_id] = [source]; + const firstDelivery = deferred<"sent">(); + admissionMock.mockReturnValueOnce(firstDelivery.promise).mockResolvedValueOnce("queued"); + const snapshot: LateClarificationSnapshot = { + messages: [source], + answers: [{ question_id: QUESTION_ID, selected_options: ["option-1"] }], + }; + + const first = renderHook(() => + // eslint-disable-next-line react-hooks/rules-of-hooks -- renderHook callback is the hook under test. + useLateClarificationMessage(source), + ); + let firstSend!: Promise<"sent" | "queued">; + await act(async () => { + firstSend = first.result.current.send(snapshot); + await waitFor(() => expect(admissionMock).toHaveBeenCalledTimes(1)); + }); + const firstClientMessageId = admissionMock.mock.calls[0][0].clientMessageId; + first.unmount(); + firstDelivery.reject(new Error("transport unavailable")); + await expect(firstSend).rejects.toThrow("transport unavailable"); + + const remounted = renderHook(() => + // eslint-disable-next-line react-hooks/rules-of-hooks -- renderHook callback is the hook under test. + useLateClarificationMessage(source), + ); + expect(remounted.result.current.state).toMatchObject({ + status: "error", + snapshot, + }); + + await act(async () => { + await remounted.result.current.send(snapshot); + }); + expect(admissionMock.mock.calls[1][0].clientMessageId).toBe(firstClientMessageId); + expect(remounted.result.current.state.status).toBe("queued"); + }); + + it("shares a delayed failure from the active form with the transcript form", async () => { + const source = sourceMessage("pending-shared"); + const state = storeRef.current!; + state.taskSessions.items[source.session_id] = sourceSession(); + state.messages.bySession[source.session_id] = [source]; + const delivery = deferred<"sent">(); + admissionMock.mockReturnValueOnce(delivery.promise); + const snapshot: LateClarificationSnapshot = { + messages: [source], + answers: [{ question_id: QUESTION_ID, custom_text: "Keep this answer" }], + }; + + const active = renderHook(() => useLateClarificationMessage(source)); + const transcript = renderHook(() => useLateClarificationMessage(source)); + let send!: Promise<"sent" | "queued">; + await act(async () => { + send = active.result.current.send(snapshot); + await waitFor(() => expect(transcript.result.current.state.status).toBe("sending")); + }); + delivery.reject(new Error("late delivery failed")); + await expect(send).rejects.toThrow("late delivery failed"); + await waitFor(() => expect(transcript.result.current.state.status).toBe("error")); + expect(transcript.result.current.state.snapshot?.answers).toEqual(snapshot.answers); + }); + + it("hydrates a source session and transcript before Inbox admission", async () => { + const source = sourceMessage("pending-inbox"); + const state = storeRef.current!; + const session = sourceSession(); + fetchTaskSessionMock.mockResolvedValue({ session }); + listTaskSessionMessagesMock.mockResolvedValue({ + messages: [source], + has_more: false, + cursor: "", + }); + admissionMock.mockResolvedValue("sent"); + const snapshot: LateClarificationSnapshot = { + messages: [source], + answers: [{ question_id: QUESTION_ID, selected_options: ["option-1"] }], + }; + + const hook = renderHook(() => useLateClarificationMessage(source)); + await act(async () => { + await hook.result.current.send(snapshot); + }); + + expect(fetchTaskSessionMock).toHaveBeenCalledWith(source.session_id, { cache: "no-store" }); + expect(listTaskSessionMessagesMock).toHaveBeenCalledWith( + source.session_id, + { limit: 100, sort: "asc" }, + { cache: "no-store" }, + ); + expect(state.taskSessions.items[source.session_id]).toEqual(session); + expect(state.messages.bySession[source.session_id]).toEqual([source]); + expect(admissionMock).toHaveBeenCalledWith( + expect.objectContaining({ clientMessageId: expect.any(String) }), + ); + expect(hook.result.current.state.status).toBe("sent"); + }); +}); diff --git a/apps/web/hooks/use-late-clarification-message.ts b/apps/web/hooks/use-late-clarification-message.ts new file mode 100644 index 0000000000..b935bd1529 --- /dev/null +++ b/apps/web/hooks/use-late-clarification-message.ts @@ -0,0 +1,261 @@ +"use client"; + +import { useCallback, useRef, useSyncExternalStore } from "react"; +import { useTranslation } from "react-i18next"; +import { useAppStoreApi } from "@/components/state-provider"; +import { MessageSendError } from "@/lib/chat/message-send-error"; +import { findPendingClarification } from "@/lib/utils/pending-clarification"; +import type { Message } from "@/lib/types/http"; +import { fetchTaskSession, listTaskSessionMessages } from "@/lib/api/domains/session-api"; +import { + formatLateClarificationMessage, + type LateClarificationSnapshot, +} from "@/lib/clarification/late-clarification-message"; +import { generateUUID } from "@/lib/utils"; +import { useMessageHandler, type MessageAdmissionOutcome } from "./use-message-handler"; + +export type { LateClarificationSnapshot } from "@/lib/clarification/late-clarification-message"; + +export type LateClarificationState = + | { status: "idle"; snapshot: null } + | { status: "sending"; snapshot: LateClarificationSnapshot } + | { status: "sent"; snapshot: LateClarificationSnapshot } + | { status: "queued"; snapshot: LateClarificationSnapshot } + | { status: "error"; error: unknown; snapshot: LateClarificationSnapshot }; + +type LateAdmissionRecord = { + clientMessageId: string; + state: LateClarificationState; + inFlight: Promise | null; +}; + +const IDLE_LATE_CLARIFICATION_STATE: LateClarificationState = { + status: "idle", + snapshot: null, +}; +const lateAdmissionRecords = new Map(); +const lateAdmissionListeners = new Map void>>(); + +function pendingIdFromMessage(message: Message | undefined): string | null { + const metadata = message?.metadata as { pending_id?: string } | undefined; + return metadata?.pending_id ?? null; +} + +function messageIdentity(messages: readonly Message[]): string { + return JSON.stringify(messages.map((message) => message.id).sort()); +} + +function lateAdmissionKey( + taskId: string | null, + sessionId: string | null, + messages: readonly Message[], +): string { + const pendingId = pendingIdFromMessage(messages[0]); + return JSON.stringify([taskId, sessionId, pendingId ?? messageIdentity(messages)]); +} + +function sourceIds(message: Message | undefined): { + taskId: string | null; + sessionId: string | null; +} { + if (!message) return { taskId: null, sessionId: null }; + return { taskId: message.task_id ?? null, sessionId: message.session_id ?? null }; +} + +function sourceAdmissionKey(message: Message | undefined): string | null { + const { taskId, sessionId } = sourceIds(message); + if (!message || !taskId || !sessionId) return null; + return lateAdmissionKey(taskId, sessionId, [message]); +} + +function recordForKey(key: string): LateAdmissionRecord { + const existing = lateAdmissionRecords.get(key); + if (existing) return existing; + const created: LateAdmissionRecord = { + clientMessageId: generateUUID(), + state: IDLE_LATE_CLARIFICATION_STATE, + inFlight: null, + }; + lateAdmissionRecords.set(key, created); + return created; +} + +function stateForKey(key: string | null): LateClarificationState { + return key + ? (lateAdmissionRecords.get(key)?.state ?? IDLE_LATE_CLARIFICATION_STATE) + : IDLE_LATE_CLARIFICATION_STATE; +} + +function notifyRecord(key: string) { + for (const listener of lateAdmissionListeners.get(key) ?? []) listener(); +} + +function updateRecordState(key: string, state: LateClarificationState) { + const record = recordForKey(key); + record.state = state; + notifyRecord(key); +} + +function subscribeToRecord(key: string | null, listener: () => void) { + if (!key) return () => {}; + const listeners = lateAdmissionListeners.get(key) ?? new Set<() => void>(); + listeners.add(listener); + lateAdmissionListeners.set(key, listeners); + return () => { + listeners.delete(listener); + if (listeners.size === 0) lateAdmissionListeners.delete(key); + }; +} + +function resetRecord(key: string | null) { + if (!key) return; + const record = lateAdmissionRecords.get(key); + if (!record) return; + record.state = IDLE_LATE_CLARIFICATION_STATE; + notifyRecord(key); +} + +type StoreApi = ReturnType; + +async function hydrateSourceSession({ + storeApi, + taskId, + sessionId, + unavailableMessage, +}: { + storeApi: StoreApi; + taskId: string; + sessionId: string; + unavailableMessage: string; +}) { + const state = storeApi.getState(); + const cachedSession = state.taskSessions.items[sessionId]; + if (cachedSession && cachedSession.task_id !== taskId) { + throw new MessageSendError("session-unavailable", unavailableMessage); + } + + const needsSession = cachedSession === undefined; + const needsMessages = !Object.prototype.hasOwnProperty.call(state.messages.bySession, sessionId); + if (!needsSession && !needsMessages) return; + + const [sessionResponse, messagesResponse] = await Promise.all([ + needsSession ? fetchTaskSession(sessionId, { cache: "no-store" }) : null, + needsMessages + ? listTaskSessionMessages(sessionId, { limit: 100, sort: "asc" }, { cache: "no-store" }) + : null, + ]); + + if (sessionResponse?.session) { + if (sessionResponse.session.task_id !== taskId) { + throw new MessageSendError("session-unavailable", unavailableMessage); + } + storeApi.getState().setTaskSession(sessionResponse.session); + } + if (messagesResponse) { + const messages = messagesResponse.messages ?? []; + storeApi.getState().mergeMessages(sessionId, messages, { + historyInitialized: true, + hasMore: messagesResponse.has_more ?? false, + oldestCursor: messages[0]?.id ?? null, + }); + } +} + +export function useLateClarificationMessage(sourceMessage: Message | undefined) { + const { t } = useTranslation(); + const storeApi = useAppStoreApi(); + const { taskId, sessionId } = sourceIds(sourceMessage); + const sourceKey = sourceAdmissionKey(sourceMessage); + const excludedPendingIdRef = useRef(null); + + const subscribe = useCallback( + (listener: () => void) => subscribeToRecord(sourceKey, listener), + [sourceKey], + ); + const getSnapshot = useCallback(() => stateForKey(sourceKey), [sourceKey]); + const state = useSyncExternalStore(subscribe, getSnapshot, getSnapshot); + + const getHasPendingClarification = useCallback(() => { + if (!sessionId) return false; + const messages = storeApi.getState().messages?.bySession?.[sessionId] ?? []; + const pending = findPendingClarification(messages); + if (!pending) return false; + const pendingId = pendingIdFromMessage(pending); + return pendingId !== excludedPendingIdRef.current; + }, [sessionId, storeApi]); + + const { handleSendMessageWithOutcome } = useMessageHandler({ + resolvedSessionId: sessionId, + taskId, + sessionModel: null, + activeModel: null, + getHasPendingClarification, + }); + + const send = useCallback( + async (snapshot: LateClarificationSnapshot): Promise => { + if (!taskId || !sessionId) { + throw new MessageSendError("no-active-session", t("task:lateAnswerFailed")); + } + const pendingId = pendingIdFromMessage(snapshot.messages[0]); + excludedPendingIdRef.current = pendingId; + const message = formatLateClarificationMessage(snapshot.messages, snapshot.answers, { + questionLabel: t("task:lateAnswerQuestionLabel"), + answerLabel: t("task:lateAnswerAnswerLabel"), + contextLabel: t("task:lateAnswerContextLabel"), + }); + const admissionKey = lateAdmissionKey(taskId, sessionId, snapshot.messages); + const record = recordForKey(admissionKey); + if (record.inFlight) return record.inFlight; + if (record.state.status === "sent" || record.state.status === "queued") { + record.clientMessageId = generateUUID(); + } + const admissionSnapshot: LateClarificationSnapshot = { + messages: snapshot.messages.slice(), + answers: snapshot.answers.slice(), + }; + updateRecordState(admissionKey, { status: "sending", snapshot: admissionSnapshot }); + const delivery = (async () => { + try { + await hydrateSourceSession({ + storeApi, + taskId, + sessionId, + unavailableMessage: t("task:sessionNotAvailableForInput"), + }); + const outcome = await handleSendMessageWithOutcome({ + message, + clientMessageId: record.clientMessageId, + }); + if (outcome === false || outcome === undefined) { + throw new MessageSendError("late-answer-admission-failed", t("task:lateAnswerFailed")); + } + updateRecordState(admissionKey, { status: outcome, snapshot: admissionSnapshot }); + return outcome; + } catch (error) { + updateRecordState(admissionKey, { + status: "error", + error, + snapshot: admissionSnapshot, + }); + throw error; + } + })(); + record.inFlight = delivery; + void delivery.then( + () => { + if (record.inFlight === delivery) record.inFlight = null; + }, + () => { + if (record.inFlight === delivery) record.inFlight = null; + }, + ); + return delivery; + }, + [handleSendMessageWithOutcome, sessionId, storeApi, t, taskId], + ); + + const reset = useCallback(() => resetRecord(sourceKey), [sourceKey]); + + return { state, send, reset, taskId, sessionId }; +} diff --git a/apps/web/hooks/use-message-handler.test.ts b/apps/web/hooks/use-message-handler.test.ts index c0562a04c5..0fdaab5f62 100644 --- a/apps/web/hooks/use-message-handler.test.ts +++ b/apps/web/hooks/use-message-handler.test.ts @@ -11,6 +11,8 @@ import type { AppState } from "@/lib/state/store"; import type { TaskMentionData } from "./use-inline-mention"; import type { EntityReference } from "@/lib/types/entity-reference"; +/* eslint-disable max-lines -- message admission wire and routing cases share one fixture. */ + const getWebSocketClientMock = vi.hoisted(() => vi.fn()); const listTaskSessionsMock = vi.hoisted(() => vi.fn()); const queueMock = vi.hoisted(() => vi.fn()); @@ -455,6 +457,37 @@ describe("useMessageHandler", () => { ); expect(request).not.toHaveBeenCalled(); }); + + it("returns sent or queued admission outcomes for late-message adapters", async () => { + const request = vi.fn().mockResolvedValue(undefined); + getWebSocketClientMock.mockReturnValue({ request }); + selectedSession("IDLE"); + const { result, rerender } = renderHook( + ({ hasPending }) => + useMessageHandler({ + resolvedSessionId: SESSION_ID, + taskId: TASK_ID, + sessionModel: null, + activeModel: null, + getHasPendingClarification: () => hasPending, + }), + { initialProps: { hasPending: false } }, + ); + + await expect( + result.current.handleSendMessageWithOutcome({ message: "late answer" }), + ).resolves.toBe("sent"); + expect(request).toHaveBeenCalled(); + + selectedSession("RUNNING", "generating"); + rerender({ hasPending: true }); + await expect( + result.current.handleSendMessageWithOutcome({ message: "another question is current" }), + ).resolves.toBe("queued"); + expect(queueMock).toHaveBeenCalledWith( + expect.objectContaining({ content: "another question is current" }), + ); + }); }); function selectedSession(state: string, foregroundActivity?: string) { diff --git a/apps/web/hooks/use-message-handler.ts b/apps/web/hooks/use-message-handler.ts index a5f7e68cde..5b3a2fd2af 100644 --- a/apps/web/hooks/use-message-handler.ts +++ b/apps/web/hooks/use-message-handler.ts @@ -142,12 +142,16 @@ export interface UseMessageHandlerParams { activeModel: string | null; planModeEnabled?: boolean; hasPendingClarification?: boolean; + /** Resolves the source session's current clarification barrier at send time. */ + getHasPendingClarification?: () => boolean; activeDocument?: ActiveDocument | null; planComments?: PlanComment[]; contextFiles?: ContextFile[]; prompts?: CustomPrompt[]; } +export type MessageAdmissionOutcome = "sent" | "queued"; + type SendMessagePayload = { taskId: string; resolvedSessionId: string; @@ -393,7 +397,7 @@ async function deliverComposedMessage({ queue: ReturnType["queue"]; storeApi: ReturnType; clientAdmissionId: string; -}) { +}): Promise { try { if (hasPendingClarification || inputMode === "queue") { const accepted = await queue({ @@ -411,7 +415,7 @@ async function deliverComposedMessage({ return false; } await refreshAcceptedPlanComments(taskId, planCommentRefs, storeApi); - return; + return "queued"; } const created = await sendMessageRequest({ @@ -429,6 +433,7 @@ async function deliverComposedMessage({ }); if (created?.id && created.session_id) storeApi.getState().addMessage(created); await refreshAcceptedPlanComments(taskId, planCommentRefs, storeApi); + return "sent"; } catch (error) { throw normalizePlanCommentSendError(error, taskId, storeApi); } @@ -512,6 +517,7 @@ function buildFinalMessageForSubmit({ }; } +// eslint-disable-next-line max-lines-per-function -- message admission keeps identity, recovery, and routing in one callback. export function useMessageHandler({ resolvedSessionId, taskId, @@ -519,6 +525,7 @@ export function useMessageHandler({ activeModel, planModeEnabled = false, hasPendingClarification = false, + getHasPendingClarification, activeDocument = null, planComments = [], contextFiles = [], @@ -528,7 +535,8 @@ export function useMessageHandler({ const storeApi = useAppStoreApi(); const pendingAdmissionRef = useRef(null); - const handleSendMessage = useCallback( + const sendMessage = useCallback( + // eslint-disable-next-line complexity -- admission evaluates one ordered input-mode and recovery path. async (payload: ChatSubmitPayload) => { if (!taskId || !resolvedSessionId) { const error = new MessageSendError( @@ -565,7 +573,9 @@ export function useMessageHandler({ }); const previousAdmission = pendingAdmissionRef.current?.key === admissionKey ? pendingAdmissionRef.current : null; - const admission = previousAdmission ?? { key: admissionKey, id: generateUUID() }; + const admission = + previousAdmission ?? + ({ key: admissionKey, id: payload.clientMessageId ?? generateUUID() } as const); pendingAdmissionRef.current = admission; if ( await recoverPendingMessageAdmission( @@ -577,7 +587,7 @@ export function useMessageHandler({ ) ) { if (pendingAdmissionRef.current === admission) pendingAdmissionRef.current = null; - return; + return "sent" as const; } const delivered = await deliverComposedMessage({ payload, @@ -586,7 +596,7 @@ export function useMessageHandler({ finalMessage, modelToSend, planModeEnabled, - hasPendingClarification, + hasPendingClarification: getHasPendingClarification?.() ?? hasPendingClarification, planCommentRefs, contextFilesMeta, inputMode, @@ -596,6 +606,7 @@ export function useMessageHandler({ }); if (delivered === false) return false; if (pendingAdmissionRef.current === admission) pendingAdmissionRef.current = null; + return delivered; }, [ resolvedSessionId, @@ -604,6 +615,7 @@ export function useMessageHandler({ sessionModel, planModeEnabled, hasPendingClarification, + getHasPendingClarification, queue, storeApi, planComments, @@ -613,5 +625,13 @@ export function useMessageHandler({ ], ); - return { handleSendMessage }; + const handleSendMessage = useCallback( + async (payload: ChatSubmitPayload) => { + const outcome = await sendMessage(payload); + if (outcome === false) return false; + }, + [sendMessage], + ); + + return { handleSendMessage, handleSendMessageWithOutcome: sendMessage }; } diff --git a/apps/web/lib/chat/message-send-error.ts b/apps/web/lib/chat/message-send-error.ts index d6976cc5ab..56f5201407 100644 --- a/apps/web/lib/chat/message-send-error.ts +++ b/apps/web/lib/chat/message-send-error.ts @@ -4,7 +4,8 @@ export type MessageSendErrorCode = | "session-unavailable" | "plan-comment-migration-pending" | "plan-comments-changed" - | "primary-session-changed"; + | "primary-session-changed" + | "late-answer-admission-failed"; export class MessageSendError extends Error { readonly code: MessageSendErrorCode; @@ -25,6 +26,7 @@ export function isMessageSendError(error: unknown): error is MessageSendError { code === "session-unavailable" || code === "plan-comment-migration-pending" || code === "plan-comments-changed" || - code === "primary-session-changed" + code === "primary-session-changed" || + code === "late-answer-admission-failed" ); } diff --git a/apps/web/lib/clarification/late-clarification-message.test.ts b/apps/web/lib/clarification/late-clarification-message.test.ts new file mode 100644 index 0000000000..0ecbc014a4 --- /dev/null +++ b/apps/web/lib/clarification/late-clarification-message.test.ts @@ -0,0 +1,93 @@ +import { describe, expect, it } from "vitest"; +import { sessionId as toSessionId, taskId as toTaskId, type Message } from "@/lib/types/http"; +import { formatLateClarificationMessage } from "./late-clarification-message"; + +// eslint-disable-next-line max-params -- the fixture maps the complete clarification message shape. +function questionMessage( + id: string, + questionId: string, + index: number, + prompt: string, + options: Array<{ option_id: string; label: string }>, + context?: string, +): Message { + return { + id, + session_id: toSessionId("session-1"), + task_id: toTaskId("task-1"), + author_type: "agent", + content: prompt, + type: "clarification_request", + created_at: "2026-09-18T00:00:00Z", + metadata: { + pending_id: "pending-1", + session_id: "session-1", + question_id: questionId, + question_index: index, + question_total: 2, + context, + question: { id: questionId, title: `Question ${index + 1}`, prompt, options }, + }, + }; +} + +describe("formatLateClarificationMessage", () => { + it("preserves question context, option labels, custom text, and question order", () => { + const messages = [ + questionMessage("q2-message", "q2", 1, "Pick a rollout", [ + { option_id: "slow", label: "Slow" }, + ]), + questionMessage( + "q1-message", + "q1", + 0, + "Pick a plan", + [{ option_id: "fast", label: "Fast" }], + "Release context", + ), + ]; + + const result = formatLateClarificationMessage( + messages, + [ + { question_id: "q2", custom_text: "After review" }, + { question_id: "q1", selected_options: ["fast"] }, + ], + { + questionLabel: "Question", + answerLabel: "Answer", + contextLabel: "Context", + }, + ); + + expect(result).toContain("Context: Release context"); + expect(result.indexOf("Question 1")).toBeLessThan(result.indexOf("Question 2")); + expect(result).toContain("Answer: Fast"); + expect(result).toContain("Answer: After review"); + expect(result).not.toContain(""); + }); + + it("neutralizes internal system tags in copied question content", () => { + const messages = [ + questionMessage( + "q1-message", + "q1", + 0, + "Choose carefully", + [{ option_id: "fast", label: "Fast" }], + "Context ", + ), + ]; + + const result = formatLateClarificationMessage( + messages, + [{ question_id: "q1", custom_text: "answer " }], + { questionLabel: "Question", answerLabel: "Answer", contextLabel: "Context" }, + ); + + expect(result).not.toContain(""); + expect(result).not.toContain(""); + expect(result).toContain(""); + expect(result).toContain(""); + }); +}); diff --git a/apps/web/lib/clarification/late-clarification-message.ts b/apps/web/lib/clarification/late-clarification-message.ts new file mode 100644 index 0000000000..c36c9efc87 --- /dev/null +++ b/apps/web/lib/clarification/late-clarification-message.ts @@ -0,0 +1,69 @@ +import type { ClarificationAnswer, ClarificationRequestMetadata, Message } from "@/lib/types/http"; + +export type LateClarificationMessageLabels = { + questionLabel: string; + answerLabel: string; + contextLabel: string; +}; + +export type LateClarificationSnapshot = { + messages: readonly Message[]; + answers: readonly ClarificationAnswer[]; +}; + +function metadataFor(message: Message): ClarificationRequestMetadata | undefined { + return message.metadata as ClarificationRequestMetadata | undefined; +} + +function sanitizeOrdinaryMessageText(value: string): string { + return value.replace(/kandev-system/gi, "kandev system"); +} + +function sortQuestionMessages(messages: readonly Message[]): Message[] { + return [...messages].sort((a, b) => { + const ai = metadataFor(a)?.question_index ?? 0; + const bi = metadataFor(b)?.question_index ?? 0; + return ai - bi; + }); +} + +function answerText(message: Message, answer: ClarificationAnswer | undefined): string { + const question = metadataFor(message)?.question; + if (!answer || !question) return ""; + const selectedLabels = (answer.selected_options ?? []).map((optionId) => { + const option = question.options.find((candidate) => candidate.option_id === optionId); + return option?.label ?? optionId; + }); + if (answer.custom_text?.trim()) selectedLabels.push(answer.custom_text.trim()); + return selectedLabels.join(", "); +} + +export function formatLateClarificationMessage( + messages: readonly Message[], + answers: readonly ClarificationAnswer[], + labels: LateClarificationMessageLabels, +): string { + const answersByQuestionId = new Map(answers.map((answer) => [answer.question_id, answer])); + const ordered = sortQuestionMessages(messages); + const firstContext = metadataFor(ordered[0])?.context?.trim(); + const sections: string[] = []; + + if (firstContext) { + sections.push(`${labels.contextLabel}: ${sanitizeOrdinaryMessageText(firstContext)}`); + } + + for (const [index, message] of ordered.entries()) { + const metadata = metadataFor(message); + const question = metadata?.question; + if (!question) continue; + const title = question.title?.trim(); + const prompt = sanitizeOrdinaryMessageText(question.prompt.trim()); + const answer = sanitizeOrdinaryMessageText( + answerText(message, answersByQuestionId.get(metadata.question_id ?? question.id)), + ); + const questionHeading = `${labels.questionLabel} ${index + 1}${title ? `: ${title}` : ""}`; + sections.push([questionHeading, prompt, `${labels.answerLabel}: ${answer || ""}`].join("\n")); + } + + return sections.join("\n\n"); +} diff --git a/apps/web/src/locales/en/task.json b/apps/web/src/locales/en/task.json index 9f04b7f7b4..1f8e8a6c92 100644 --- a/apps/web/src/locales/en/task.json +++ b/apps/web/src/locales/en/task.json @@ -252,6 +252,16 @@ "clarificationAnswerOverLimit_other": "{{count}} characters over the {{max}}-character limit.", "clarificationAnswerRuneCount": "{{current}} / {{max}}", "clarificationExpired": "This question is no longer active, so your answer was not recorded.", + "answerAsNewMessage": "Answer as new message", + "closeClarification": "Close question", + "lateAnswerQuestionLabel": "Question", + "lateAnswerAnswerLabel": "Answer", + "lateAnswerContextLabel": "Context", + "lateAnswerSend": "Send as new message", + "lateAnswerSending": "Sending answer...", + "lateAnswerSent": "Answer sent as a new message.", + "lateAnswerQueued": "Answer queued as a new message.", + "lateAnswerFailed": "Could not send the new message. Your answer is still here.", "clarificationResponseFailed": "Could not send your response.", "cleanup": "Cleanup", "clearAllAnnotations": "Clear all annotations", diff --git a/apps/web/src/locales/pseudo/task.json b/apps/web/src/locales/pseudo/task.json index d8619f3669..454f92b7f2 100644 --- a/apps/web/src/locales/pseudo/task.json +++ b/apps/web/src/locales/pseudo/task.json @@ -252,6 +252,16 @@ "clarificationAnswerOverLimit_other": "{{count}} ćĥàŕàćţēŕś ōvēŕ ţĥē {{max}}-ćĥàŕàćţēŕ ĺĩḿĩţ.", "clarificationAnswerRuneCount": "{{current}} / {{max}}", "clarificationExpired": "Ţĥĩś qũēśţĩōń ĩś ńō ĺōńĝēŕ àćţĩvē, śō ŷōũŕ àńśŵēŕ ŵàś ńōţ ŕēćōŕďēď.", + "answerAsNewMessage": "Àńśŵēŕ àś ńēŵ ḿēśśàĝē", + "closeClarification": "Ćĺōśē qũēśţĩōń", + "lateAnswerQuestionLabel": "Qũēśţĩōń", + "lateAnswerAnswerLabel": "Àńśŵēŕ", + "lateAnswerContextLabel": "Ćōńţēxţ", + "lateAnswerSend": "Śēńď àś ńēŵ ḿēśśàĝē", + "lateAnswerSending": "Śēńďĩńĝ àńśŵēŕ...", + "lateAnswerSent": "Àńśŵēŕ śēńţ àś à ńēŵ ḿēśśàĝē.", + "lateAnswerQueued": "Àńśŵēŕ qũēũēď àś à ńēŵ ḿēśśàĝē.", + "lateAnswerFailed": "Ćōũĺď ńōţ śēńď ţĥē ńēŵ ḿēśśàĝē. Ŷōũŕ àńśŵēŕ ĩś śţĩĺĺ ĥēŕē.", "clarificationResponseFailed": "Ćōũĺď ńōţ śēńď ŷōũŕ ŕēśƥōńśē.", "cleanup": "Ćĺēàńũƥ", "clearAllAnnotations": "Ćĺēàŕ àĺĺ àńńōţàţĩōńś", diff --git a/apps/web/src/locales/pt-pt/task.json b/apps/web/src/locales/pt-pt/task.json index 1c641c310e..017e9d6447 100644 --- a/apps/web/src/locales/pt-pt/task.json +++ b/apps/web/src/locales/pt-pt/task.json @@ -242,6 +242,16 @@ "clarificationAnswerOverLimit_other": "{{count}} carateres acima do limite de {{max}} carateres.", "clarificationAnswerRuneCount": "{{current}} / {{max}}", "clarificationExpired": "Esta pergunta já não está ativa, pelo que a sua resposta não foi registada.", + "answerAsNewMessage": "Responder como nova mensagem", + "closeClarification": "Fechar pergunta", + "lateAnswerQuestionLabel": "Pergunta", + "lateAnswerAnswerLabel": "Resposta", + "lateAnswerContextLabel": "Contexto", + "lateAnswerSend": "Enviar como nova mensagem", + "lateAnswerSending": "A enviar resposta...", + "lateAnswerSent": "Resposta enviada como nova mensagem.", + "lateAnswerQueued": "Resposta adicionada à fila como nova mensagem.", + "lateAnswerFailed": "Não foi possível enviar a nova mensagem. A sua resposta continua aqui.", "clarificationResponseFailed": "Não foi possível enviar a sua resposta.", "cleanup": "Limpeza", "clearAllAnnotations": "Limpar todas as anotações", diff --git a/apps/web/src/locales/zh-cn/task.json b/apps/web/src/locales/zh-cn/task.json index 4cc82bed3c..29482fde3b 100644 --- a/apps/web/src/locales/zh-cn/task.json +++ b/apps/web/src/locales/zh-cn/task.json @@ -242,6 +242,16 @@ "clarificationAnswerOverLimit_other": "超出 {{max}} 字符限制 {{count}} 个字符。", "clarificationAnswerRuneCount": "{{current}} / {{max}}", "clarificationExpired": "此问题已不再有效,您的回答未被记录。", + "answerAsNewMessage": "作为新消息回答", + "closeClarification": "关闭问题", + "lateAnswerQuestionLabel": "问题", + "lateAnswerAnswerLabel": "回答", + "lateAnswerContextLabel": "上下文", + "lateAnswerSend": "作为新消息发送", + "lateAnswerSending": "正在发送回答...", + "lateAnswerSent": "回答已作为新消息发送。", + "lateAnswerQueued": "回答已作为新消息排队。", + "lateAnswerFailed": "无法发送新消息。您的回答仍保留在这里。", "clarificationResponseFailed": "无法发送您的回复。", "cleanup": "清理", "clearAllAnnotations": "清除全部标注", diff --git a/apps/web/src/locales/zh-hk/task.json b/apps/web/src/locales/zh-hk/task.json index b5df50cc60..59adc195c0 100644 --- a/apps/web/src/locales/zh-hk/task.json +++ b/apps/web/src/locales/zh-hk/task.json @@ -242,6 +242,16 @@ "clarificationAnswerOverLimit_other": "超出 {{max}} 字符限制 {{count}} 個字符。", "clarificationAnswerRuneCount": "{{current}} / {{max}}", "clarificationExpired": "此問題已不再有效,您的回答未被記錄。", + "answerAsNewMessage": "作為新訊息回答", + "closeClarification": "關閉問題", + "lateAnswerQuestionLabel": "問題", + "lateAnswerAnswerLabel": "回答", + "lateAnswerContextLabel": "上下文", + "lateAnswerSend": "作為新訊息傳送", + "lateAnswerSending": "正在傳送回答...", + "lateAnswerSent": "回答已作為新訊息傳送。", + "lateAnswerQueued": "回答已作為新訊息排隊。", + "lateAnswerFailed": "無法傳送新訊息。您的回答仍保留在這裏。", "clarificationResponseFailed": "無法傳送您的回覆。", "cleanup": "清理", "clearAllAnnotations": "清除全部標註", diff --git a/apps/web/src/locales/zh-tw/task.json b/apps/web/src/locales/zh-tw/task.json index 5c386647ba..2199047a91 100644 --- a/apps/web/src/locales/zh-tw/task.json +++ b/apps/web/src/locales/zh-tw/task.json @@ -242,6 +242,16 @@ "clarificationAnswerOverLimit_other": "超出 {{max}} 字元限制 {{count}} 個字元。", "clarificationAnswerRuneCount": "{{current}} / {{max}}", "clarificationExpired": "此問題已不再有效,您的回答未被記錄。", + "answerAsNewMessage": "作為新訊息回答", + "closeClarification": "關閉問題", + "lateAnswerQuestionLabel": "問題", + "lateAnswerAnswerLabel": "回答", + "lateAnswerContextLabel": "上下文", + "lateAnswerSend": "作為新訊息傳送", + "lateAnswerSending": "正在傳送回答...", + "lateAnswerSent": "回答已作為新訊息傳送。", + "lateAnswerQueued": "回答已作為新訊息排隊。", + "lateAnswerFailed": "無法傳送新訊息。您的回答仍保留在這裡。", "clarificationResponseFailed": "無法傳送您的回覆。", "cleanup": "清理", "clearAllAnnotations": "清除全部標註", diff --git a/docs/decisions/2026-09-18-late-clarification-messages.md b/docs/decisions/2026-09-18-late-clarification-messages.md new file mode 100644 index 0000000000..c40c44e235 --- /dev/null +++ b/docs/decisions/2026-09-18-late-clarification-messages.md @@ -0,0 +1,40 @@ +# ADR-2026-09-18-late-clarification-messages: Answer earlier questions as new messages + +**Status:** accepted +**Date:** 2026-09-18 +**Area:** frontend, workflow + +## Context + +Current-turn ownership prevents stale tool responses from resuming obsolete work. +Users still need to answer questions after the original waiter or turn ends. +Removing the stale panel alone loses that conversation affordance. + +## Decision + +Preserve current-turn authority for the original tool response. Offer late +answers as ordinary user messages in the question's original conversation. +Use existing message admission, steering, queue ordering, and retry identity. +An affirmative submission receiving a recognized inactive response preserves +its answers and continues through that message path. Closing sends nothing. + +The decision extends the presentation of the +[current-turn ownership rule](2026-08-14-current-turn-clarification-ownership.md). +It does not grant historical questions operational authority. Ordinary messages +remain subject to existing session availability and other live input barriers. + +## Consequences + +Users can answer earlier questions without resurrecting stale tool waits. +Message admission must retain drafts and stable identities across failures. +The UI must distinguish tool delivery, new-message delivery, and queue admission. +The Inbox History tab can retain its read-only boundary and link to conversation. + +## Alternatives Considered + +- Reopen historical tool requests: rejected because this restores obsolete + workflow barriers and can resume the wrong turn. +- Remove inactive controls without a late-answer path: rejected because users + lose the ability to respond to questions the agent asked. +- Add a separate late-answer transport: rejected because ordinary message + admission already owns sending, queueing, and duplicate prevention. diff --git a/docs/plans/clarification-inactive-dismiss/plan.md b/docs/plans/clarification-inactive-dismiss/plan.md new file mode 100644 index 0000000000..b5ac4643e6 --- /dev/null +++ b/docs/plans/clarification-inactive-dismiss/plan.md @@ -0,0 +1,307 @@ +--- +created: 2026-09-18 +status: complete +requirements: + - REQ-TASKS-CLARIFICATION-LIFECYCLE-001 + - REQ-TASKS-CLARIFICATION-RESPONSE-RELIABILITY-001 +system_design: + - ../../specs/tasks/system-design/clarification-active-lifecycle.md + - ../../specs/tasks/system-design/clarification-response-reliability.md +legacy_specs: [] +--- + +# Implementation Plan: Close or answer inactive clarification questions + +## Overview + +Repair the confirmed inactive-response path associated with +[issue #3798](https://github.com/kdlbs/kandev/issues/3798) and preserve a useful +conversation path after the original clarification waiter ends. Task 01 +reconciles inactive responses, race ordering, and Escape behavior. Task 02 +supersedes its removal-only presentation with late answers delivered as +ordinary messages through the source task and session. + +## Evidence and root cause + +Investigation base: `26254fe51f`. The report names `57bbae10ad` but provides no +network trace or confirmed reproduction. Both revisions wire the X-shaped +`ClarificationSkipButton` to rejection, separately from local collapse. + +`useClarificationGroup` classifies `409 not_active` as `expired`. It updates +message status only for `ok`. `useResolveCallback` also handles only `ok`. +The overlay renders its expired banner beside enabled question actions. +Without an authoritative message update, the question remains pending locally. +Further X clicks repeat the request instead of removing the obsolete panel. + +A temporary test copied the existing overlay harness and clicked X twice. +Both responses returned `409 {"code":"not_active"}`. After both settled, the +test confirmed two requests, `rejected=true`, and no message-store update. +Its final removal assertion failed because X remained mounted and enabled. +The temporary test was removed; permanent regressions are recorded in Task 01. + +This proves one repairable path, not the cause of every reported occurrence. +Timeouts, failed requests, and malformed responses remain distinct outcomes. + +## Revised contract and delivery scope + +The user's latest direction takes precedence over the historical Task 01 +approach below. Reuse `REQ-TASKS-CLARIFICATION-LIFECYCLE-001`, amended criteria +`.1`/`.2` and new criteria `.4` through `.9`. Requirements, design, and the +[late-answer ADR](../../decisions/2026-09-18-late-clarification-messages.md) +now separate operational tool responses from ordinary messages. + +- Preserve earlier questions in transcript history with an answer-as-new-message action. +- Preserve submitted answers across `409 not_active` and send through normal admission. +- Keep X/Close as dismissal without a new message for inactive questions. +- Reuse existing sending/steering/queue rules; no new busy-agent policy is introduced. +- Keep Inbox History read-only and use its existing source-conversation navigation. +- Keep Task 01's race and Escape fixes wherever applicable. +- Task 02 owns UI, admission adapter, localization, and focused regression coverage. + +UI-02: Earlier question, shared desktop and phone inline composition. + +```text +[Earlier question ] +[Answer as new message ] + opens: +[Question and answer choices ] +[Custom answer ] +[Close] [Send as new message ] +[Failure: draft retained, Retry available ] +``` + +A recognized inactive result during answer submission uses the same delivery +path with the captured draft. Sent or queued admission replaces the form with +its actual outcome. Closing returns focus to its opener. Keep the existing +phone scroll owner and safe areas; controls have 44px coarse-pointer targets. +This view covers lifecycle criteria `.4` through `.9`. Exact spacing is illustrative. + +The technical approach and results below retain Task 01's historical repair +details and record Task 02's revised implementation separately. + +## Requirement conformance + +- `AC-TASKS-CLARIFICATION-LIFECYCLE-001.2` requires obsolete questions to stop + being answerable. Reuse this active requirement without creating a repair spec. +- `AC-TASKS-CLARIFICATION-RESPONSE-RELIABILITY-001.1` distinguishes expiry from + acceptance and retryable failure. +- `AC-TASKS-CLARIFICATION-RESPONSE-RELIABILITY-001.4` preserves retry controls + after actual failures. Do not make every unsuccessful response disappear. +- The response-reliability design now specifies cache reconciliation. Existing + authority and persistence decisions remain unchanged. + +## Scope + +### In scope + +- Reconcile submitted inactive bundles without overwriting newer authority, + terminal siblings, or restored pending rows. +- Keep expired static overlays free of response controls and preserve the + existing Escape, collapse, and generation fences. +- Offer historical unanswered questions an answer-as-new-message flow on + desktop and phone. +- Preserve affirmative answers after recognized inactive responses and deliver + them through ordinary message admission with stable retry identity. +- Localize the new actions and feedback, update public task guidance, and + validate the shared hook plus desktop/mobile browser flows. + +### Out of scope + +- Backend mutations, new endpoints, database migrations, and agent cancellation. +- Changing Escape/collapse semantics or treating expiry as successful rejection. +- Redesigning the Inbox, task summaries, or the clarification layout. +- Claiming that this repairs an unobserved timeout or backend failure. + +## Technical approach + +In `apps/web/hooks/domains/session/use-clarification-group.ts`, extend the +existing response reconciliation for `expired`. Read the current store rows +before changing only pending rows from the submitted bundle to local `expired`. +Match message ID, session ID, and pending ID. Preserve current metadata and +terminal siblings; never insert a missing row from the submitted snapshot. +Use `isPendingClarificationMessage` for the existing legacy pending semantics. +Compare `updated_at` independently for each row before applying expiry so a newer +authoritative restoration wins while unchanged pending siblings remain eligible. +If the request generation is stale while the same pending ID is current again, +skip the cache write; an old bundle remains eligible for retirement while a +different pending ID is active. + +Keep response identity separate from current UI identity. An old request may +retire its own stale rows but cannot change a replacement bundle's controls, +outcome callback, or in-flight guard. Preserve the existing generation fence. +Do not store expiry in the backend or suppress later authoritative restoration. + +In `clarification-input-overlay.tsx`, render only the existing expired notice +when static props retain an inactive bundle. Do not register active response +shortcuts in that state. Keep `no_longer_active` distinct from `resolved` and +leave `onResolved` success-only. Guard direct submission/retry calls after +expiry until bundle replacement or an authoritative restoration resets state. + +Task chat, Quick Chat, and run transcripts derive pending questions from the +shared message cache. Their existing selectors remove the obsolete panel. +The Inbox retains its existing `no_longer_active` handling. No host-specific +successful-response callback substitutes for cache reconciliation. + +For Task 02, the shared late-answer adapter captures the source task/session, +question bundle, and answers before invoking `useMessageHandler`. It reuses +ordinary input-mode and queue admission, keeps the captured client message ID +across remounts, and exposes sent, queued, and retryable failure outcomes. +Historical transcript rows open the same inline question form in explicit +new-message mode. The source conversation remains the routing authority. + +## ASCII UI preview + +UI-01: Task chat after X receives `409 not_active`, desktop and phone. + +```text +Before After +[Question X v] [Conversation / question history] +[Question no longer active] [Message composer ] +[Answer choices ] +[Message composer ] +``` + +The stale panel disappears; history remains. A static host can show the expired +notice without response controls. The diagram uses illustrative copy. +Phone retains the existing focused chat and inline composer, with no new sheet. +`ClarificationPanelSection` and `mobile-clarification.spec.ts` are the nearest +shipped exemplars. Keep current scroll ownership, safe-area handling, and +44px coarse-pointer targets for remaining controls. Shared state owns expiry. +Map this view to lifecycle AC `.2` and response-reliability AC `.1`. + +## Tests + +- `use-clarification-group.test.ts`: inactive Skip updates only matching + pending rows; preserve terminal siblings and unrelated bundles in one fixture. +- `use-clarification-group.regressions.test.ts`: delayed A response after B + mounts, deleted rows, latest metadata preservation, authoritative restoration, + and direct resubmission guards. +- `clarification-input-overlay.test.tsx`: `X retires an inactive bundle without +another rejection request`; replace the existing expired-banner expectation + with a non-actionable notice and no success callback. Cover answer retention + and late-message success, queue, and retry feedback. +- `clarification-request-message.test.tsx` and the formatter tests cover the + historical action, current-turn exclusion, question context, selected + labels, custom text, and system-marker neutralization. +- `use-message-handler.test.ts` covers sent/queued outcomes, a current + clarification barrier, unavailable transport, and caller-owned retry IDs. +- Existing timeout, retry, malformed-response, success, and collapse tests + remain required. These cover response-reliability AC `.1` and `.4`. + +## E2E tests + +Add a scenario tagged `inactive dismissal` in `e2e/tests/chat/clarification.spec.ts` +and `e2e/tests/chat/mobile-clarification.spec.ts`. Use `chromium` and +`mobile-chrome`, respectively. Intercept the exact response request with +`409 not_active`, while retaining the stale pending message in the client. +Arm the HTTP causal wait before clicking/tapping X. Assert panel removal, +composer availability, and no second rejection request. A newer bundle must +remain answerable. Also retain existing real-backend Skip success coverage. + +Add `late answer` scenarios to the same desktop and phone files. Assert that a +historical question can open the form, that the admitted message contains the +question and answer, and that coarse-pointer controls remain reachable. Keep +the active submission fallback covered by the submit-failure scenario. + +## Work orders + +- [x] [Task 01: Reconcile inactive clarification responses](task-01-reconcile-inactive-responses.md) +- [x] [Task 02: Send late answers as new messages](task-02-late-answer-messages.md) + +Execute sequentially. Task 02 depends on Task 01 and revises its user-facing outcome. + +## Verification results + +The temporary reproduction failed at the expected control-removal assertion. +The existing hook, overlay, and panel suites passed: 3 files, 73 tests. + +```bash +(cd apps/web && pnpm exec vitest run hooks/domains/session/use-clarification-group.test.ts components/task/chat/clarification-input-overlay.test.tsx components/task/chat/clarification-panel-section.test.tsx) +``` + +Documentation catalog validation passed: 291 decisions and 1015 specifications. +The full specification lint initially exposed a pre-existing duplicate +`AC-TASKS-QUEUED-SESSION-OWNERSHIP-003.8` in +`docs/specs/tasks/requirements/queued-session-ownership.md`; the unrelated +conversation-surface criterion was renumbered to `.003.10` so the catalog has +unique acceptance IDs. The full lint then passed. `git diff --check` passed. + +The normal commit hooks passed without a bypass. The issue assignment was +verified as `carlosflorencio`. + +## Implementation results + +Task 01 is complete. The hook now reconciles only current pending rows from the +submitted inactive bundle, preserves newer metadata and terminal siblings, and +blocks repeated actions until replacement or authoritative restoration. A newer +authoritative `updated_at` wins over a delayed inactive response, and stale +A→B→A generations cannot expire the bundle that is current again. Static expired +overlays retain only the non-actionable notice and do not arm their Escape +guard. Desktop and phone E2E cover the same intercepted inactive response. + +The browser-first regression failed before the correction at the expected +store-update and stale-control assertions. After the correction, validation +passed: + +The review regressions also failed before the correction: the delayed restored +bundle became `expired`, the A→B→A path wrote an expired row, and the expired +overlay still claimed Escape. Each now passes. + +The later PR review found three follow-up defects in retry ownership, mounted +late-message state, and Inbox source-session hydration. The remediation adds +post-await pending ID, generation, and bundle guards; a shared source-keyed +admission owner; and conditional session/message hydration before ordinary +admission. New deterministic coverage records delayed retry rejection and +success across bundle replacement, unmount/remount draft recovery, active to +transcript sharing, and an Inbox send with empty source caches. + +The review was explicitly code-only. No local build, typecheck, unit, browser, +or E2E command was run for that remediation. The normal commit hooks and +`git diff --check` are recorded separately from the prior implementation +validation below. + +- Task 02 focused Vitest: 8 files, 142 tests passed. +- Targeted ESLint: no errors or warnings on changed frontend and E2E files. +- Targeted Prettier, TypeScript typecheck, `make build-web`, `make build-backend`, and + `git diff --check` passed. +- Desktop historical late-answer E2E: 1 test passed. +- Phone historical late-answer E2E: 1 test passed. +- Active 409 late-answer fallback E2E: 1 test passed. +- E2E fixture plugin packaging passed. +- Documentation catalog validation passed: 291 decisions and 1015 + specifications. +- `i18n:check` passed for all supported catalogs. The all-locale + `i18n:zh-hant` helper still stops on the two pre-existing simplified + `workflows.openAgentSettings` entries; the changed `task` namespace converted + successfully for both Traditional Chinese catalogs. +- Full specification lint passed after the pre-existing duplicate acceptance + ID was corrected to `AC-TASKS-QUEUED-SESSION-OWNERSHIP-003.10`. +- PR documentation coverage passed after nesting the late-answer criteria under + the lifecycle requirement heading. +- The review follow-up now compares message versions with `parseTurnTimestamp`, + preserving nanosecond ordering and rejecting malformed timestamps. Expiry is + selective per row, so an unchanged pending sibling still retires beside a + newer restored row. Companion parking work orders reference the corrected + `.003.10` criterion. + +## Related delivery records + +[Response reliability](../clarification-response-reliability/plan.md) and +[active lifecycle](../clarification-active-lifecycle/plan.md) remain historical +delivery records. This follow-up does not change their completed scopes or results. + +## Risks + +- A 409 is not proof that this caller rejected the question. Preserve outcome identity. +- Another caller's failed delivery can restore pending state. Authoritative + restoration must remain possible; do not add permanent local suppression. +- A delayed response must not overwrite newer metadata or terminal siblings. +- The report lacks runtime evidence. Investigate separately if the failure + persists with a successful response after this focused repair. + +## Documentation impact + +Task 01 did not need public documentation changes. Task 02 must document the +late-answer action and its normal send/queue behavior in the existing task +conversation guide. This is recorded in `docs/public/tasks-and-workflows.md`; +queued admission is described as queueing, not agent receipt. diff --git a/docs/plans/clarification-inactive-dismiss/task-01-reconcile-inactive-responses.md b/docs/plans/clarification-inactive-dismiss/task-01-reconcile-inactive-responses.md new file mode 100644 index 0000000000..4df950b183 --- /dev/null +++ b/docs/plans/clarification-inactive-dismiss/task-01-reconcile-inactive-responses.md @@ -0,0 +1,194 @@ +--- +id: "01-reconcile-inactive-responses" +title: "Reconcile inactive clarification responses" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-TASKS-CLARIFICATION-LIFECYCLE-001 + - REQ-TASKS-CLARIFICATION-RESPONSE-RELIABILITY-001 +acceptance_criteria: + - AC-TASKS-CLARIFICATION-LIFECYCLE-001.2 + - AC-TASKS-CLARIFICATION-RESPONSE-RELIABILITY-001.1 + - AC-TASKS-CLARIFICATION-RESPONSE-RELIABILITY-001.4 +system_design: + - ../../specs/tasks/system-design/clarification-active-lifecycle.md + - ../../specs/tasks/system-design/clarification-response-reliability.md +--- + +# Task 01: Reconcile inactive clarification responses + +## Follow-up scope + +This work order records the completed removal-only implementation at PR #3799 +head `6edc7320dd`. The user's later direction supersedes that presentation. +[Task 02](task-02-late-answer-messages.md) adds answers as ordinary messages. +The results below are historical evidence, not validation of the new scope. + +## Summary + +Close stale task-chat questions when X receives an authoritative inactive response. +Keep expiry distinct from successful rejection and retryable failure. + +## In scope + +- Shared hook reconciliation against the latest store snapshot. +- Non-actionable expired rendering and response-shortcut cleanup. +- Focused hook/component regressions and desktop/phone browser evidence. +- Accurate plan and work-order results after implementation. + +## Out of scope + +Backend changes, permanent dismissal storage, new UI copy, layout redesign, +and changing Escape/collapse into rejection. + +## Acceptance + +1. A recognized inactive response retires only matching pending rows in the + submitted bundle. Task chat removes the panel without a WebSocket event. + Preserve terminal siblings, unrelated bundles, deleted rows, and newer metadata. +2. A static expired overlay exposes no answer, Skip, Submit, or Retry action. + Expiry reports `no_longer_active`, never successful rejection. Unknown or + malformed responses and transport failures preserve retryable behavior. +3. A delayed A response cannot alter B's submission state or callbacks. A later + authoritative pending restoration remains answerable, including the same ID. + Desktop and phone pass the same inactive-dismissal flow. + +## Implementation sequence + +1. Read the linked requirements and design's inactive-response section. +2. Add the regression before changing production code. Mock `409 not_active`, + click X, and assert that no actionable stale question remains. +3. Reconcile submitted rows using their latest message-cache values. Reuse + `isPendingClarificationMessage`, including its missing-status compatibility. + Compare each latest `updated_at` with its submitted snapshot and preserve a + newer authoritative row while retiring unchanged pending siblings. Fence + cache expiry when the request generation is stale and the same pending ID is + current again; still retire an old bundle while another pending ID is active. +4. Keep current request-generation checks for UI state and callbacks. Test a + mixed bundle containing pending and terminal rows, plus an unrelated live bundle. +5. Render the expired notice without response actions for static hosts. Prevent + repeated hook submissions until replacement or authoritative restoration. + An ordinary rerender of stale props must not reset expiry. Do not arm the + Escape guard when the expired notice has no keyboard handler. +6. Add desktop and phone coverage, run the commands below, and record results. + +## ASCII UI preview + +UI-01: Task chat after inactive X response, shared desktop/phone structure. + +```text +Before: [Question / expired notice / X / answers] +After: [Conversation / question history] + [Message composer ] +``` + +See the [full preview](plan.md#ascii-ui-preview) for composition and constraints. +The stale panel disappears; static hosts retain only the non-actionable notice. +Keep current phone scroll ownership, safe areas, and remaining touch targets. +This view covers lifecycle AC `.2` and response-reliability AC `.1`. + +## Verification + +Run from the repository root. Install dependencies first in a fresh worktree. +Run the new regression alone before the correction and record its expected failure. +Then run the complete checks below; E2E commands run sequentially. + +```bash +(cd apps && pnpm install --frozen-lockfile) +(cd apps/web && pnpm exec vitest run hooks/domains/session/use-clarification-group.test.ts hooks/domains/session/use-clarification-group.regressions.test.ts hooks/domains/session/use-clarification-group.timeout.test.ts components/task/chat/clarification-input-overlay.test.tsx components/task/chat/clarification-panel-section.test.tsx) +(cd apps/web && pnpm exec eslint hooks/domains/session/use-clarification-group.ts components/task/chat/clarification-input-overlay.tsx) +(cd apps/web && pnpm run typecheck) +make build-web +make build-backend +(cd apps/web && pnpm e2e:run --no-build --project chromium -- tests/chat/clarification.spec.ts --grep 'inactive dismissal') +(cd apps/web && pnpm e2e:run --no-build --project mobile-chrome -- tests/chat/mobile-clarification.spec.ts --grep 'inactive dismissal') +python3 scripts/list-docs.py validate +python3 scripts/lint-spec-files.py --all +git diff --check +``` + +Use the current E2E fixture and causal-wait helpers. Intercept only the target +response and retain stale client data so a message event cannot hide the bug. +Inspect the phone rendering against UI-01. Do not claim screenshots or E2E +results from the design turn; those checks have not run. + +## Files likely touched + +- `apps/web/hooks/domains/session/use-clarification-group.ts` +- `apps/web/hooks/domains/session/use-clarification-group.test.ts` +- `apps/web/hooks/domains/session/use-clarification-group.regressions.test.ts` +- `apps/web/hooks/domains/session/use-clarification-group.timeout.test.ts` +- `apps/web/components/task/chat/clarification-input-overlay.tsx` +- `apps/web/components/task/chat/clarification-input-overlay.test.tsx` +- `apps/web/components/task/chat/clarification-panel-section.test.tsx` +- `apps/web/e2e/tests/chat/clarification.spec.ts` +- `apps/web/e2e/tests/chat/mobile-clarification.spec.ts` + +Read-only integration points include `lib/utils/pending-clarification.ts`, +`hooks/use-processed-messages.ts`, `components/task/task-chat-panel.tsx`, +`components/quick-chat/quick-chat-content.tsx`, and `components/runs/run-transcript.tsx` +under `apps/web/`. Inspect the Inbox's existing outcome consumer too. + +## Dependencies + +None. Execute only after the user's explicit implementation request. + +## Risks + +Local expiry must not become a backend write or permanent suppression. Read +current rows before cache updates; a submitted snapshot can contain stale metadata. +The report's exact runtime cause remains unconfirmed beyond this reproduced path. + +## Parallelism + +`sequential` + +## Inputs + +- [Evidence and root cause](plan.md#evidence-and-root-cause). +- [Lifecycle requirements](../../specs/tasks/requirements/clarification-active-lifecycle.md). +- [Response requirements](../../specs/tasks/requirements/clarification-response-reliability.md). +- [Client recovery design](../../specs/tasks/system-design/clarification-response-reliability.md#inactive-response-reconciliation). +- Existing expired-banner test and successful Skip/store-update tests. + +## Results + +Implemented the inactive-response repair. A recognized `409 not_active` now +reconciles only matching pending rows from the latest message cache to local +`expired` status, preserving newer metadata and terminal siblings while +skipping deleted rows and unrelated bundles. A newer authoritative `updated_at` +wins over a delayed inactive response, and stale A→B→A generations cannot +expire the bundle that is current again. The overlay exposes only the existing +expired notice, does not arm its Escape guard, and direct submit, Skip, and +Retry calls are guarded until a replacement or authoritative pending +restoration arrives. + +Added shared hook and overlay regressions, delayed-response coverage, and +desktop/mobile E2E scenarios. The browser-first regression failed before the +production change at the expected cache-update and stale-control assertions. +The review regressions then failed before the ordering and Escape-guard +correction, and pass after it. + +Validation passed: + +- Focused Vitest: 5 files, 96 tests. +- Strict timestamp comparison preserves RFC3339Nano ordering and rejects + malformed or Date-normalized values. +- Selective expiry preserves a newer restored row while retiring an unchanged + pending sibling in the same bundle. +- Targeted ESLint: no errors or warnings. +- Prettier and TypeScript typecheck. +- `make build-web` and `make build-backend`. +- Desktop inactive-dismissal E2E: 1 passed. +- Phone inactive-dismissal E2E: 1 passed. +- E2E fixture plugin packaging. +- Documentation catalog validation, full specification lint, and + `git diff --check`. + +The full specification lint first exposed a pre-existing duplicate acceptance +ID in the queued-session ownership requirements. The unrelated +conversation-surface criterion was renumbered to +`AC-TASKS-QUEUED-SESSION-OWNERSHIP-003.10`; the final catalog lint passed. +Companion parking work orders now reference that corrected criterion. diff --git a/docs/plans/clarification-inactive-dismiss/task-02-late-answer-messages.md b/docs/plans/clarification-inactive-dismiss/task-02-late-answer-messages.md new file mode 100644 index 0000000000..2823e6d922 --- /dev/null +++ b/docs/plans/clarification-inactive-dismiss/task-02-late-answer-messages.md @@ -0,0 +1,221 @@ +--- +id: "02-late-answer-messages" +title: "Send late answers as new messages" +status: done +wave: 2 +depends_on: + - 01-reconcile-inactive-responses +plan: "plan.md" +requirements: + - REQ-TASKS-CLARIFICATION-LIFECYCLE-001 +acceptance_criteria: + - AC-TASKS-CLARIFICATION-LIFECYCLE-001.4 + - AC-TASKS-CLARIFICATION-LIFECYCLE-001.5 + - AC-TASKS-CLARIFICATION-LIFECYCLE-001.6 + - AC-TASKS-CLARIFICATION-LIFECYCLE-001.7 + - AC-TASKS-CLARIFICATION-LIFECYCLE-001.8 + - AC-TASKS-CLARIFICATION-LIFECYCLE-001.9 +system_design: + - ../../specs/tasks/system-design/clarification-active-lifecycle.md + - ../../specs/tasks/system-design/clarification-response-reliability.md +--- + +# Task 02: Send late answers as new messages + +## Summary + +Let users answer earlier human clarification questions through ordinary messages. +Preserve answers when tool submission receives a recognized inactive result. +This revises PR #3799's removal-only behavior without reopening tool authority. + +## In scope + +- Transcript entry point and inline late-answer form on desktop and phone. +- A shared question/answer formatter and adapter to ordinary message admission. +- Recognized inactive-answer fallback, draft retention, stable retry identity, + correct session targeting, and explicit sent/queued feedback. +- Existing live-waiter and detached-current-turn paths remain operational. +- Localization in all supported catalogs and focused public documentation. + +## Out of scope + +Backend tool-response authority changes, replacement sessions, forced interruption, +Inbox History mutations, permission approvals, and parent-agent question routing. +Do not make historical questions count as pending workflow input. + +## Acceptance + +1. A user can answer an earlier unanswered question from its conversation. + Question prompts, selected labels, and custom text reach ordinary admission + for the source task/session. Idle, busy, and unavailable cases match chat. +2. Recognized inactive results preserve affirmative answers for the same path. + Close sends nothing. Unknown conflicts and uncertain tool transport results + retain the existing response retry path instead of sending a duplicate message. +3. Failures retain drafts; retries reuse admission identity. Navigation and + newer questions cannot redirect delivery or lose a pending admission. + Desktop and phone distinguish queued admission from sent messages. + +## Technical approach + +Extend `useClarificationGroup` with a typed late-message callback/outcome. +Capture the submitted questions and answers before awaiting the response. +Do not emit a destructive host outcome before fallback admission settles. +Keep an admission owner mounted independently of the expiring overlay, so +its draft and stable request ID survive cache updates and close/reopen cycles. + +Reuse `useMessageHandler`, `deriveSessionInputMode`, and `useQueue` through +one shared adapter. Do not issue raw prompt or queue requests from components. +Read the source session's latest state; an obsolete question must not force +`hasPendingClarification=true`. A separate current question keeps its barrier. +Retain existing request-generation and per-row version fences. + +Add a shared formatter with focused tests. Render source questions and answers +as ordinary user text, not a privileged system marker. Never derive the target +from global active selection after asynchronous work. + +`ClarificationRequestMessage` exposes the history action for human questions +that are no longer operational. Reuse the existing question form with explicit +new-message mode and Close. The message renderer supplies needed conversation +context. Read-only surfaces navigate to that conversation; the Inbox History +read path remains free of mutations. Existing authenticated message admission +owns access and availability checks. + +## ASCII UI preview + +UI-02: Shared desktop and phone inline flow, from transcript history. + +```text +[Earlier question ] +[Answer as new message ] + opens: +[Question and answer choices ] +[Custom answer ] +[Close] [Send as new message ] +[Failure: draft retained, Retry available ] +``` + +See [the combined preview](plan.md#revised-contract-and-delivery-scope). +Reuse inline clarification controls and existing scroll ownership. Keep phone +safe areas and 44px coarse-pointer targets. Return focus to opener on Close. +Localize action and sent/queued feedback. Map to criteria `.4` through `.9`. + +## Tests and verification + +Deterministic coverage covers both entry points: inactive response after an +answer submission, and direct historical-question answering. The shared +message-handler tests cover source-session barriers, busy queue admission, +unavailable transport, and stable caller-owned retry IDs. Overlay and group +tests cover answer retention, retry feedback, current-question exclusion, +timeout/error separation, and zero-message Close behavior. Existing +live-answer and Skip tests stay green. + +Add `late answer` cases to desktop/mobile clarification browser files. Assert +actual admitted message or queue content with the original question and answers, +not only disappearance of the overlay. A failed admission preserves the form. + +Run these commands from the repository root during implementation, not design: + +```bash +(cd apps && pnpm install --frozen-lockfile) +(cd apps/web && pnpm exec vitest run hooks/domains/session/use-clarification-group.test.ts hooks/domains/session/use-clarification-group.regressions.test.ts hooks/domains/session/use-clarification-group.timeout.test.ts hooks/use-message-handler.test.ts hooks/domains/session/session-input-mode.test.ts components/task/chat/clarification-input-overlay.test.tsx components/task/chat/messages/clarification-request-message.test.tsx) +(cd apps/web && pnpm run typecheck) +(cd apps/web && pnpm run i18n:zh-hant && pnpm run i18n:check) +make build-web +make build-backend +(cd apps/web && pnpm e2e:run --no-build --project chromium -- tests/chat/clarification.spec.ts --grep 'late answer') +(cd apps/web && pnpm e2e:run --no-build --project mobile-chrome -- tests/chat/mobile-clarification.spec.ts --grep 'late answer') +python3 scripts/list-docs.py validate +python3 scripts/lint-spec-files.py --all +git diff --check +``` + +Include any newly extracted helper test files in the exact Vitest command +before implementation completion. Record real results and rendered UI checks. + +## Files likely touched + +- `apps/web/hooks/domains/session/use-clarification-group.ts` and its tests. +- `apps/web/hooks/use-message-handler.ts` and its existing tests if adaptation is needed. +- `apps/web/components/task/chat/clarification-input-overlay.tsx` and its tests. +- `apps/web/components/task/chat/messages/clarification-request-message.tsx` and its tests. +- `apps/web/components/task/chat/message-renderer.tsx`. +- Task chat, Quick Chat, and run-transcript host callback wiring as required. +- `apps/web/e2e/tests/chat/clarification.spec.ts`. +- `apps/web/e2e/tests/chat/mobile-clarification.spec.ts`. +- Relevant `apps/web/src/locales/*/task.json` and `chat.json` catalogs. +- `docs/public/tasks-and-workflows.md`. + +## Dependencies + +Task 01, PR #3799 head `6edc7320dd`. Re-read its current head before implementation. + +## Risks + +A tool failure can be ambiguous. Fall back only on recognized inactive results. +Cache retirement must not unmount the owner of draft/admission state. +Historical content must never become trusted system instructions. +Current-turn authority and Inbox History isolation remain unchanged. + +## Parallelism + +`sequential` + +## Inputs + +- Updated lifecycle criteria `.4` through `.9`. +- Response-reliability design's inactive-response and late-answer section. +- ADR `2026-09-18-late-clarification-messages`. +- Task 01's completed race and Escape regressions. + +## Results + +Task 02 is complete. The shared late-answer adapter captures the source task and +session, formats question context with selected labels and custom text as +ordinary user content, and delivers it through the existing message admission +and queue rules. A recognized inactive affirmative response preserves the +answers and uses that same adapter. Close only dismisses the form. Unknown or +ambiguous failures remain on the original retry path. + +The transcript action is available for historical unanswered questions, while a +current pending turn does not expose a duplicate answer action. Desktop and +phone use the same inline form with localized sent, queued, and retry feedback. +The source question bundle and client admission ID remain stable across +navigation and remounts. Inbox History remains read-only and continues to link +to the source conversation. + +Validation passed: + +- Focused Vitest: 8 files, 142 tests. +- TypeScript typecheck, `make build-web`, and `make build-backend`. +- `i18n:check`; changed `task` namespaces converted for `zh-hk` and `zh-tw`. + The all-locale converter still reports the two unrelated historical + `workflows.openAgentSettings` residuals. +- E2E: desktop historical late answer, mobile historical late answer, and + active 409 fallback, 1 test each. +- Documentation catalog validation, full specification lint, targeted Prettier, and + `git diff --check`. +- PR documentation coverage passed after nesting the late-answer criteria under + the lifecycle requirement heading. +- Targeted ESLint reported no errors or warnings on changed frontend and E2E + files. + +## Review remediation + +The code-review follow-up hardens late admission across the same source task, +session, and clarification bundle. Retry completion now captures and checks the +pending ID, request generation, and submitted bundle before every post-await +state update, cache reconciliation, and outcome callback. A delayed retry from +an older bundle cannot replace the state of a newer bundle or a returned +generation. + +Late-message status, draft answers, and client admission identity now live in a +module-owned external record shared by active, transcript, and Inbox hosts. +When the source session or message cache is absent, the adapter hydrates them +through the existing session APIs before ordinary send, steer, or queue +admission derives its input mode. The regression coverage includes delayed +failure across unmount/remount, active-to-transcript state sharing, missing +Inbox caches, and A→B and A→B→A delayed retry races. + +This review was code-only by instruction. No local builds, typechecks, unit +tests, browser tests, or E2E tests were run for the remediation. The normal +commit hooks and `git diff --check` remain the applicable local checks. diff --git a/docs/plans/session-open-recovery-eligibility/plan.md b/docs/plans/session-open-recovery-eligibility/plan.md index 9f77f31c9b..bfb0f1b3f7 100644 --- a/docs/plans/session-open-recovery-eligibility/plan.md +++ b/docs/plans/session-open-recovery-eligibility/plan.md @@ -41,7 +41,7 @@ resume on open even while a session is parked. The screenshot is presentation evidence; this plan does not claim a new live-state investigation of that task. The old inspection requirement 001.1 and parked-note requirement 003.3 are -explicitly superseded by 001.9 and 003.8. Criteria 001.7/8 retain restart and +explicitly superseded by 001.9 and 003.10. Criteria 001.7/8 retain restart and settled-queue coverage. Queue identity, automatic admission, callback correlation, and the deadlock fixes remain compatibility constraints. @@ -123,7 +123,7 @@ The task drawer remains the entry point. The session picker selects the conversation and normal recovery follows. The dedicated mobile layout, safe-area navigation, and single chat scroll owner remain. No hover interaction is required. -UI-01 and UI-02 cover AC 001.9 and 003.8. Placement and absence of parking chrome +UI-01 and UI-02 cover AC 001.9 and 003.10. Placement and absence of parking chrome are requirements; text spacing is illustrative. Existing recovery errors, queue waiting, loading, and explicit controls keep their established presentation. The nearest mobile exemplar is `mobile-queued-session-ownership.spec.ts`. @@ -144,7 +144,7 @@ AC suffixes use `AC-TASKS-QUEUED-SESSION-OWNERSHIP-`. | Archive, explicit cancellation, completion, authorization failure | Existing lifecycle restrictions remain | 001.9 | | Allowed status followed by conflicting queue or route change | Guarded admission rejects stale execution | 001.2, 001.6; 002.6 | | Restart after switch or queue settlement | Conversation context survives; opening requests normal recovery | 001.7, 001.8, 001.9 | -| Desktop/phone with stored parking marker | No parked note; ordinary controls and real queue status remain | 003.8 | +| Desktop/phone with stored parking marker | No parked note; ordinary controls and real queue status remain | 003.10 | ## E2E tests diff --git a/docs/plans/session-open-recovery-eligibility/task-02-remove-parking-presentation.md b/docs/plans/session-open-recovery-eligibility/task-02-remove-parking-presentation.md index acbbe0ed39..abaf776813 100644 --- a/docs/plans/session-open-recovery-eligibility/task-02-remove-parking-presentation.md +++ b/docs/plans/session-open-recovery-eligibility/task-02-remove-parking-presentation.md @@ -63,7 +63,7 @@ Both: No parked-session row; genuine task queue status remains when needed. Keep the existing desktop tabs and phone task drawer/session picker. The chat remains the single conversation scroll owner. No new touch targets or -hover-only behavior are introduced. Cover AC 001.9 and 003.8. +hover-only behavior are introduced. Cover AC 001.9 and 003.10. ## Verification diff --git a/docs/public/tasks-and-workflows.md b/docs/public/tasks-and-workflows.md index 716139f544..90c23e9d6a 100644 --- a/docs/public/tasks-and-workflows.md +++ b/docs/public/tasks-and-workflows.md @@ -461,6 +461,20 @@ The preference only gates opening a task. Choosing **Start agent** (or a workflow step transition) always starts the agent as usual, and a failed or interrupted session still shows its recovery actions. +## Answer clarification questions + +When an agent asks a clarification question, answer it from the question panel +while the session is waiting. If the question becomes inactive before you +answer, its entry remains in the conversation with **Answer as new message**. +That action includes the question and your answer in a normal conversation +message. It does not reopen the old tool request. + +Normal message rules still apply. An idle session receives the message; a busy +session sends or queues it according to its current input mode. If delivery +fails, the form keeps your answer so you can retry. **Close question** removes +the answer surface without sending anything, while the question stays in the +conversation history. + ## Task dependencies A task can declare that it **depends on** one or more other tasks. This is a diff --git a/docs/specs/tasks/requirements/clarification-active-lifecycle.md b/docs/specs/tasks/requirements/clarification-active-lifecycle.md index 859c3b1e9d..7a8cd6a6c8 100644 --- a/docs/specs/tasks/requirements/clarification-active-lifecycle.md +++ b/docs/specs/tasks/requirements/clarification-active-lifecycle.md @@ -15,7 +15,7 @@ owners: -Clarification bundles remain answerable only for the session turn that owns them, while detached and recovery paths preserve the response contract. +Tool responses belong to the owning session turn. Users can send answers to earlier questions as new messages without reopening the original tool request. @@ -32,6 +32,9 @@ hiding the action the icon represents. ## What +The active/answerable rules in this section describe the original tool request. +They do not prohibit the separate late-answer message action specified below. + - A clarification bundle is active only when at least one row in the bundle is pending and the bundle belongs to the session's current turn. - A detached bundle remains active and answerable while its turn remains current. Detachment sets @@ -130,8 +133,7 @@ hiding the action the icon represents. one turn identity from the same bundle rule: legacy empty turn IDs do not mask a consistent non-empty identity, while conflicting non-empty IDs invalidate the identity. - Any response to a superseded or terminal bundle returns conflict, performs no message mutation, and - initiates no agent resume. Current clients close their obsolete local overlay through the existing - conflict handling. + initiates no agent resume. Clients distinguish rejection from an affirmative answer. An affirmative answer can continue as a new user message under the late-answer criteria below. - Persisted task status summaries reconcile `pending_action` against current-turn repository state on source events and task-list/boot reads. Existing summaries are repaired, not only missing rows. - When a task row advertises a pending action, desktop and phone task activation load the task's @@ -158,7 +160,7 @@ hiding the action the icon represents. -**Intent:** Clarification bundles remain answerable only for the session turn that owns them, while detached and recovery paths preserve the response contract. +**Intent:** Tool responses belong to the owning session turn. Users can send answers to earlier questions as new messages without reopening the original tool request. @@ -166,12 +168,39 @@ hiding the action the icon represents. -- **AC-TASKS-CLARIFICATION-LIFECYCLE-001.1:** When a clarification is pending, the system shall expose and process only the bundle owned by the session's current turn. -- **AC-TASKS-CLARIFICATION-LIFECYCLE-001.2:** When a newer turn or terminal session state supersedes a bundle, the system shall keep its transcript history without leaving it answerable or able to block workflow progress. +- **AC-TASKS-CLARIFICATION-LIFECYCLE-001.1:** When a clarification is pending, the system shall process tool responses only for the bundle owned by the session's current turn. Earlier questions can accept new-message answers under the late-answer criteria below. +- **AC-TASKS-CLARIFICATION-LIFECYCLE-001.2:** When a newer turn or terminal session state supersedes a bundle, the system shall keep its transcript history without leaving the original tool request answerable or able to block workflow progress. Users can answer through a separate new-message action. - **AC-TASKS-CLARIFICATION-LIFECYCLE-001.3:** When detached clarification recovery succeeds or fails, the system shall return the bounded response and durable recovery outcome defined by the lifecycle contract. +#### Late answers as new messages + +These criteria extend the existing lifecycle requirement. They separate an +operational tool response from a user choosing to continue the conversation. + +- **AC-TASKS-CLARIFICATION-LIFECYCLE-001.4:** When an unanswered question is + superseded or expired, its transcript entry shall offer an answer-as-new-message + action on desktop and phone. It shall not claim that the agent is still waiting. +- **AC-TASKS-CLARIFICATION-LIFECYCLE-001.5:** When an affirmative answer receives + a recognized inactive result, the interface shall preserve the submitted answers + and send them with their question context through ordinary message delivery. + It shall not silently discard those answers or report successful tool delivery. +- **AC-TASKS-CLARIFICATION-LIFECYCLE-001.6:** A late answer shall target the + question's original task and session. It shall follow ordinary sending, + steering, queue ordering, and unavailable-session behavior. An idle input-capable + session shall receive it without requiring the original tool waiter. +- **AC-TASKS-CLARIFICATION-LIFECYCLE-001.7:** Closing an inactive question shall + dismiss its answer surface without sending a message or resuming the agent. + The question shall remain in history. Active rejection keeps its existing semantics. +- **AC-TASKS-CLARIFICATION-LIFECYCLE-001.8:** A failed late-message admission shall + preserve the answer and offer retry. An ambiguous transport result shall not + automatically start another delivery path. Retrying one admission shall reuse + its identity rather than create duplicate messages. +- **AC-TASKS-CLARIFICATION-LIFECYCLE-001.9:** Sending a late answer shall not reopen + the original tool request, restore its workflow barrier, or resolve another + pending question. The interface shall distinguish sent from queued admission. + ## Out of scope diff --git a/docs/specs/tasks/system-design/clarification-active-lifecycle.md b/docs/specs/tasks/system-design/clarification-active-lifecycle.md index f3ff054aba..1e6bf1e654 100644 --- a/docs/specs/tasks/system-design/clarification-active-lifecycle.md +++ b/docs/specs/tasks/system-design/clarification-active-lifecycle.md @@ -97,6 +97,13 @@ No new route. Task-session responses add `pending_action_revision` beside the ex still owned by the in-memory clarification store. The chat Skip control uses `/respond` with `rejected=true`, including for detached requests. +## Late conversation replies + +The tool-response contract above does not prohibit an ordinary user message +about an earlier question. See [late-answer delivery](clarification-response-reliability.md#inactive-response-reconciliation-and-late-answers) +for transcript actions and inactive-response fallback. That path does not claim +the historical bundle or revive its operational authority. + ## State machine One clarification bundle has five operational states: diff --git a/docs/specs/tasks/system-design/clarification-response-reliability.md b/docs/specs/tasks/system-design/clarification-response-reliability.md index dd0609487e..e8f37a0fce 100644 --- a/docs/specs/tasks/system-design/clarification-response-reliability.md +++ b/docs/specs/tasks/system-design/clarification-response-reliability.md @@ -3,6 +3,7 @@ status: current system: tasks requirements: - REQ-TASKS-CLARIFICATION-RESPONSE-RELIABILITY-001 + - REQ-TASKS-CLARIFICATION-LIFECYCLE-001 --- # Clarification response reliability System Design @@ -133,6 +134,68 @@ winner when the first request committed, or claims the still-pending bundle when the first request did not. A 409 `not_active` remains an expired outcome, not a successful submission. +### Inactive-response reconciliation and late answers + +This section also maps `REQ-TASKS-CLARIFICATION-LIFECYCLE-001`, criteria +`.4` through `.9`. The [late-answer decision](../../../decisions/2026-09-18-late-clarification-messages.md) +separates operational tool authority from ordinary conversation. + +`useClarificationGroup` captures question prompts, option labels, custom text, +answers, task ID, and session ID before submitting. An affirmative response +classified as inactive transfers that snapshot to a shared late-message action. +It does not clear the draft before message admission succeeds. Rejection instead +retires the obsolete surface without sending an ordinary message. + +Use the existing `useMessageHandler` admission path for late messages. Format +question prompts and selected labels with custom answers as ordinary user text. +Do not interpolate them into a privileged system block. A directly opened +historical question uses this path without retrying the obsolete response endpoint. +A recognized inactive result after an affirmative submit uses the same path +without requiring the user to enter the answers again. + +A typed host callback connects the shared question UI to message admission. +Resolve its task/session from the source question, not global active selection. +Retain the captured identity across navigation. Task chat and Quick Chat wire +this callback through their existing chat handler. A transcript question opens +an inline late-answer form using the same formatter and admission adapter. +Read-only run surfaces link to the source conversation rather than acquire a +second message transport. The Inbox History tab remains read-only; its existing +conversation navigation provides access to the transcript action. + +Refresh message/session authority after an inactive result before deriving +promptability. The stale question itself must not force `hasPendingClarification` +to true and queue an otherwise idle session forever. A different active question +retains its barrier. Reuse `deriveSessionInputMode` and queue admission for busy +sessions; do not introduce forced interruption or a separate queue policy. +Unavailable sessions preserve the answer and use ordinary recovery feedback. +No automatic replacement task or session is created. + +Keep the existing stable admission identity across retries, including when the +message surface closes and reopens while admission is unresolved. A rejected +admission preserves the draft; an uncertain admission reconciles by that same +identity. Do not send ordinary messages after timeouts, malformed responses, +unknown conflicts, or generic server failures in the tool-response path. +Those outcomes retain its existing retry flow. + +Separate tool outcomes from message outcomes. `no_longer_active` is not a +successful tool response; sent/queued late answers need their own outcome. +Consumers must not unmount an answer draft merely on `no_longer_active` while +new-message admission is pending or has failed. Success-only `onResolved` +remains specific to the original response path. + +Cache retirement preserves current row metadata, newer authoritative versions, +terminal siblings, deleted rows, and request-generation ownership. Keep the +per-row RFC3339Nano version checks from the inactive-dismissal repair. Retiring +operational state must not remove the transcript's late-answer entry point. +An expired surface has working Close and new-message actions. Its Escape guard +must match an actually mounted handler. New-message admission never marks the +historical tool request answered or reactivates its pending-action projection. + +Desktop and phone use the existing inline question form. Keep the current scroll +owner and safe areas, provide 44px coarse-pointer actions, and return focus to +the transcript opener or composer after closing. Translate new labels in all +supported catalogs. The action explicitly says it sends a new message. + Local collapse, dismiss, Escape, and task navigation do not mutate the bundle. Skip remains the explicit rejection path. These controls may be disabled only while the bounded request is in flight and become available again on a