fix(frontend): gate clarification retries on structured terminal command outcomes

frontend/src/components/chat/clarification-form.tsx

@@ -1,5 +1,10 @@
 import React, { useEffect, useLayoutEffect, useMemo, useRef, useState } from "react"
-import { Interaction } from "@/contexts/app-context-chat"
+import {
+  ClarificationSubmission,
+  Interaction,
+  TerminalCommandOutcome,
+} from "@/contexts/app-context-chat"
+import { generateClientMessageId } from "@/lib/utils"
 import { Input } from "@/components/ui/input"
 import { Button } from "@/components/ui/button"
 import { Textarea } from "@/components/ui/textarea"
@@ -114,6 +119,13 @@ const sendHintKey = (
     ? "chatPage.clarification.sendNotSent"
     : null
 
+// Stable empty fallback for rounds with no tracked submissions: minting a
+// fresh [] per render would change the gating effect's dependency identity
+// on ANY round's record/clear, and a rerun on an active untracked round
+// reaches the tail that clears a visible send-failure alert - the exact
+// regression class the boolean reduction below exists to prevent.
+const NO_SUBMISSIONS: ClarificationSubmission[] = []
+
 // Interaction types that are "live widgets" reflecting external state (e.g.
 // useMcpApps()'s connection state), not a question with an answer to submit
 // - see the comment on isConnectAppsOnly below for why that distinction
@@ -133,11 +145,17 @@ export function ClarificationForm({
 }: ClarificationFormProps) {
   // If onSend is provided, use it (e.g., from builder chat), otherwise use useApp
   let sendMessage: any, dispatch: any, contextFilesDisabled: boolean | undefined;
+  let commandOutcomes: Record<string, TerminalCommandOutcome> | undefined;
+  let clarificationSubmissions:
+    | Record<string, ClarificationSubmission[]>
+    | undefined;
   try {
     const appCtx = useApp();
     sendMessage = appCtx.sendMessage;
     dispatch = appCtx.dispatch;
     contextFilesDisabled = appCtx.filesDisabled;
+    commandOutcomes = appCtx.state?.commandOutcomes;
+    clarificationSubmissions = appCtx.state?.clarificationSubmissions;
   } catch {
     // We might not be in the app context (e.g., agent builder chat)
   }
@@ -184,6 +202,11 @@ export function ClarificationForm({
       errorCode: ClientErrorCode | null
     } | null
   >(null)
+  // Which outcome notice the form owes the user. Raw category only; the
+  // sentence is resolved at render so a locale switch reaches it.
+  const [outcomeNotice, setOutcomeNotice] = useState<
+    "notApplied" | "unconfirmed" | "pending" | null
+  >(null)
 
   useLayoutEffect(() => {
     latestRequestIdRef.current = requestId
@@ -194,18 +217,134 @@ export function ClarificationForm({
     setIsSubmitted(!active && !isConnectAppsOnly)
     setIsOpen(active || isConnectAppsOnly)
     setSendFailure(null)
+    // A new round is a new question: the previous round's outcome notice no
+    // longer belongs on top of it.
+    setOutcomeNotice(null)
   }, [active, isConnectAppsOnly, requestId])
 
+  // The reply commands this round is still accountable for, read from
+  // context so the gate survives this component instance being replaced
+  // (virtual waiting message vs. persisted timeline message render the same
+  // round). One round can hold several: an ack-timeout entry stays
+  // resubmittable, so a resubmit appends rather than replaces, and every
+  // tracked command's outcome still gets matched.
+  const outstandingSubmissions = useMemo<ClarificationSubmission[]>(
+    () =>
+      (requestId ? clarificationSubmissions?.[requestId] : undefined)
+      ?? NO_SUBMISSIONS,
+    [requestId, clarificationSubmissions],
+  )
+  // Reduced to booleans before entering the effect's dependency list: an
+  // unrelated command's outcome landing must not re-run the effect - that
+  // rerun used to wipe a visible failure alert.
+  const hasOutstanding = outstandingSubmissions.length > 0
+  // Any tracked reply whose terminal outcome cannot prove non-application
+  // may have been committed: the round must surface that and stay locked.
+  const anyUnsafeOutcome = outstandingSubmissions.some((submission) => {
+    const outcome = commandOutcomes?.[submission.commandId]
+    return outcome !== undefined && outcome.resendSafe !== true
+  })
+  // Every tracked reply is proven not applied: resending is safe.
+  const allProvenNotApplied =
+    hasOutstanding
+    && outstandingSubmissions.every(
+      (submission) => commandOutcomes?.[submission.commandId]?.resendSafe === true,
+    )
+  // A durably acknowledged reply is still awaiting its terminal outcome.
+  // (An unconfirmed ack-timeout entry without an outcome deliberately does
+  // not count: it may never have been accepted, and the composer keeps its
+  // advisory retry for it - which is also why it blocks the proven-safe
+  // unlock above without forcing a lock here.)
+  const confirmedPending = outstandingSubmissions.some(
+    (submission) =>
+      submission.accepted && commandOutcomes?.[submission.commandId] === undefined,
+  )
+  // Distinguishes the round that has heard nothing yet from the one whose
+  // resolved replies are all proven safe while unconfirmed ones remain.
+  const anyOutcome = outstandingSubmissions.some(
+    (submission) => commandOutcomes?.[submission.commandId] !== undefined,
+  )
+
   useEffect(() => {
-    if (active) {
-      // A new clarification round reuses this component instance on the live
-      // turn render path, so a stale round-1 failure alert would sit on top
-      // of round 2's question.
-      setIsSubmitted(false)
-      setIsOpen(true)
-      setSendFailure(null)
+    if (!active) return
+    // Task state alone answers whether the task accepts input; it does not
+    // prove that repeating this round's accepted reply is safe (#1500).
+    // While a submission is outstanding, reactivation requires terminal
+    // outcomes that prove non-application for every tracked reply.
+    if (hasOutstanding) {
+      if (anyUnsafeOutcome) {
+        // Committed or unknown: surface the ambiguity without inviting a
+        // duplicate. The chat input remains available for a deliberate
+        // fresh message. The send-failure alert would compete with this
+        // one (two role="alert" regions), and this notice supersedes it.
+        setOutcomeNotice("unconfirmed")
+        setIsSubmitted(true)
+        setIsOpen(true)
+        setSendFailure(null)
+        return
+      }
+      if (confirmedPending) {
+        // Accepted and still being applied: lock even a freshly mounted
+        // instance (whose initial isSubmitted is false), and say why the
+        // form is locked instead of leaving a bare greyed-out button.
+        setOutcomeNotice("pending")
+        setIsSubmitted(true)
+        setIsOpen(true)
+        return
+      }
+      if (!allProvenNotApplied) {
+        if (anyOutcome) {
+          // Every resolved reply is proven not applied; only unconfirmed
+          // ack-timeout replies remain, and their duplicate risk is the
+          // same risk the advisory retry already accepted for them. Match
+          // the fresh-instance stance: return the round to advisory
+          // instead of leaving the submitting instance locked behind a
+          // now-stale pending notice. No proven-safe notice - that promise
+          // cannot be made while an unconfirmed reply is unresolved.
+          setOutcomeNotice((notice) => (notice === "pending" ? null : notice))
+          setIsSubmitted(false)
+          setIsOpen(true)
+        }
+        // Nothing resolved yet: keep the composer's advisory retry - and
+        // its ack-timeout warning alert - exactly as they stand.
+        return
+      }
+      // Every tracked reply is proven not applied: consume the record so
+      // the resend is armed once, then reactivate below with the draft
+      // intact. The CLEAR names exactly the commands this render verified,
+      // so a submission recorded concurrently (another instance's resubmit
+      // landing between this render's snapshot and the dispatch) survives
+      // with its outcome still matchable.
+      dispatch?.({
+        type: "CLEAR_CLARIFICATION_SUBMISSION",
+        payload: {
+          requestId,
+          commandIds: outstandingSubmissions.map(
+            (submission) => submission.commandId,
+          ),
+        },
+      })
+      setOutcomeNotice("notApplied")
     }
-  }, [active])
+    // A new clarification round reuses this component instance on the live
+    // turn render path, so a stale round-1 failure alert would sit on top
+    // of round 2's question. The outcome notice is deliberately not cleared
+    // here: it explains why the form re-opened, and it falls away on the
+    // next submit or the next round.
+    setIsSubmitted(false)
+    setIsOpen(true)
+    setSendFailure(null)
+  }, [
+    active,
+    hasOutstanding,
+    anyUnsafeOutcome,
+    allProvenNotApplied,
+    confirmedPending,
+    anyOutcome,
+    outstandingSubmissions,
+    requestId,
+    dispatch,
+  ])
 
   const normalizedInteractions = useMemo(() => {
     const seenFields = new Set<string>()
@@ -377,17 +516,46 @@ export function ClarificationForm({
       }
     })
 
+    // Minted here, not in sendMessage, so this round knows the id the
+    // durable command will carry and can correlate its terminal outcome
+    // (#1500). The builder onSend path has no durable command behind it and
+    // takes no part in outcome gating; neither does a round without a
+    // request id - the gate would have no round identity to bind to, and a
+    // recorded reply could end up gating a different question.
+    const clientMessageId = generateClientMessageId()
+    // ``accepted`` separates a durably acknowledged reply from one whose
+    // ack timed out: only a confirmed acceptance may lock a freshly mounted
+    // form while its terminal outcome is still pending.
+    const recordSubmission = (accepted: boolean) => {
+      if (onSend || !dispatch) return
+      if (typeof submittedRequestId !== "string" || !submittedRequestId) return
+      dispatch({
+        type: "RECORD_CLARIFICATION_SUBMISSION",
+        payload: {
+          requestId: submittedRequestId,
+          commandId: clientMessageId,
+          accepted,
+        },
+      })
+    }
+
     try {
       setIsSubmitting(true)
       setSendFailure(null)
+      setOutcomeNotice(null)
       // If textMessage is empty but we have files, send a generic message?
       const outboundFiles = filesDisabled ? [] : files
       const finalMessage = textMessage || (outboundFiles.length > 0 ? t("chatPage.clarification.uploadedFiles") : t("chatPage.clarification.confirmed"))
 
       if (onSend) {
         await onSend(finalMessage, outboundFiles, metadata);
       } else if (sendMessage) {
-        await sendMessage(finalMessage, { force: true, metadata }, outboundFiles)
+        await sendMessage(
+          finalMessage,
+          { force: true, metadata, clientMessageId },
+          outboundFiles,
+        )
+        recordSubmission(true)
       }
 
       if (latestRequestIdRef.current !== submittedRequestId) return
@@ -397,6 +565,13 @@ export function ClarificationForm({
         dispatch({ type: "UPDATE_TASK_STATUS", payload: { status: "running" } })
       }
     } catch (error) {
+      if (readSendDisposition(error) === "outcome_unknown") {
+        // An ack timeout: the reply may still have been durably accepted,
+        // so its eventual terminal outcome must gate this round exactly as
+        // an acknowledged submission's would - but as an unconfirmed
+        // delivery it must not lock the form while no outcome exists.
+        recordSubmission(false)
+      }
       if (latestRequestIdRef.current !== submittedRequestId) return
       console.error("Failed to send clarification response", error)
       // The rejection reason ("a previous guidance message is still being
@@ -713,6 +888,21 @@ export function ClarificationForm({
               ))}
             </div>
 
+            {outcomeNotice === "unconfirmed" && (
+              <div role="alert" className="rounded-md border border-destructive/40 bg-destructive/5 p-3 text-sm text-destructive">
+                {t("chatPage.clarification.replyOutcomeUnknown")}
+              </div>
+            )}
+            {outcomeNotice === "pending" && (
+              <div role="status" className="rounded-md border bg-muted/50 p-3 text-sm text-muted-foreground">
+                {t("chatPage.clarification.replyPending")}
+              </div>
+            )}
+            {outcomeNotice === "notApplied" && (
+              <div role="status" className="rounded-md border bg-muted/50 p-3 text-sm text-muted-foreground">
+                {t("chatPage.clarification.replyNotApplied")}
+              </div>
+            )}
             {sendFailure && (
               <div role="alert" className="rounded-md border border-destructive/40 bg-destructive/5 p-3 text-sm text-destructive">
                 <div>{sendFailureMessage}</div>