fix(frontend): gate clarification retries on structured terminal command outcomes - #2126
fix(frontend): gate clarification retries on structured terminal command outcomes#2126codeacme17 wants to merge 3 commits into
Conversation
…and outcomes A clarification form re-enabled whenever the task returned to waiting_for_user, even when the durably accepted reply's terminal outcome was unknown or possibly still in flight - inviting an accidental duplicate submission (xorbitsai#1500). The form now mints its own client message id (the durable command id) and records the round's accountable submission in AppContext keyed by request id, so the gate survives the submitting component instance being replaced (virtual waiting message vs. persisted timeline message). An ack-timeout (outcome_unknown) delivery records the submission too, since the reply may still have been durably accepted. The form reactivates only when the structured terminal outcome broadcast for that exact command proves the reply was not applied (resend_safe); the record is then consumed so the resend is armed once. An outcome that cannot prove non-application locks the form with a visible notice and the draft preserved; the chat input remains available for a deliberate fresh message. Rounds without a request id are not gated - with no round identity a recorded reply could gate a different question. AppContext records terminal agent_error dispositions keyed by command id; exact-id correlation is what makes stale or cross-run outcomes inert, and the reactivation effect depends on the derived per-round values so an unrelated command's outcome cannot wipe a visible failure alert. Legacy frames without the structured fields stay in the unsafe reading. Requires the wire contract from xorbitsai#2124 for live frames; without it every outcome reads as unknown, which is the conservative direction.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses Issue #1500 by tracking terminal command outcomes and clarification submissions in the chat context state to determine if a clarification form can be safely resent. It introduces state management for commandOutcomes and clarificationSubmissions, handles structured terminal outcomes from agent_error WebSocket messages, and updates the ClarificationForm component to gate resubmissions based on whether the failed command is marked as safe to resend. Additionally, localization strings and comprehensive unit tests are added. Feedback points out a potential issue in ClarificationForm where the form might unexpectedly reactivate and show editable fields while a reply is still in flight (i.e., outstandingSubmission is present but outstandingOutcome is undefined). To prevent this, setIsSubmitted(true) should be set during the in-flight state.
… in flight Review round 1 of xorbitsai#2126: a freshly mounted ClarificationForm starts with isSubmitted=false, and the gating effect's in-flight branch returned without locking - so replacing the submitting instance (virtual waiting message vs. persisted timeline message) while the accepted reply had no terminal outcome yet re-offered Submit for that round. clarificationSubmissions entries now carry accepted: true only after sendMessage resolves; an entry recorded from the outcome_unknown (ack-timeout) catch carries accepted: false, because the reply may never have been accepted and locking would remove the advisory retry the composer has always offered for unconfirmed deliveries. The in-flight branch locks only confirmed-accepted submissions.
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
Summary
This PR gates clarification-reply resubmission on a structured terminal command outcome instead of just task status, addressing issue #1500 (P0): a client previously could not tell whether an unacknowledged clarification reply was safe to retry, already applied, or still in flight. It introduces a client-minted command id as the durable correlation key, records each round's submission in AppContext (keyed by request_id, so it survives the submitting component being remounted), and consumes the eventual outcome/resend_safe fields from companion PR #2124's agent_error frames to unlock or explain the form. It also fixes a previously-flagged reactivation bug (see Prior review resolution below) by locking the form while a submission's outcome is still pending, instead of allowing edits.
Blocking: yes — recommended event: REQUEST_CHANGES
Round 0 — Design verdict: acceptable-with-reservations
The core command-correlation model is sound: minting a client message id as the durable command id, recording the round's submission in AppContext rather than component-local state (so it survives the submitting instance being replaced), and consuming it only on a resend-safe terminal outcome, correctly handles cross-instance persistence, legacy backends, and backends that don't yet emit PR #2124's fields (a conservative fallback that keeps the pre-existing gate). The most serious reservation is the silent-permanent-lock behavior in Finding 3, compounded by the overwrite bug in Finding 1 and the non-persistence gap in Finding 2.
Design notes (non-blocking, informational):
- The recording logic is split between the component and
AppContextrather than centralized insendMessage. Investigated as an alternative and it is not a clear win —sendMessagehas an intentionally-unrecorded call site (connect-apps skip) and the recording rules are clarification-specific, so centralizing would just relocate the coupling rather than remove it. Not recommended as an action item. - The outcome/submission maps lack a success/superseded lifecycle and bypass the file's own
SET_TASK_IDreset discipline used elsewhere (e.g.MAX_TRACKED_TASK_STATE_VERSIONS). See Findings 1 and 7.
Findings
1. [Major] [Blocking: yes] Overwriting a pending submission record orphans an earlier command's outcome
frontend/src/contexts/app-context-chat.tsx (reducer, ~1352-1363) / frontend/src/components/chat/clarification-form.tsx (~453-464, ~492-498)
recordSubmission / RECORD_CLARIFICATION_SUBMISSION unconditionally overwrites any existing clarificationSubmissions[requestId] entry. Trigger: an ack-timeout (outcome_unknown) records command1 as an unconfirmed, still-resubmittable entry — this is the advisory-retry path the PR itself adds. If the user resubmits, command2's record overwrites command1's. If command1's terminal outcome later arrives with resend_safe: false (i.e. it may have been committed — a genuine duplicate-send risk), it can no longer be matched to anything, so the round shows no ambiguity notice even though a real duplicate-send risk existed. This defeats the PR's own stated core purpose (issue #1500: surface/prevent an undetected duplicate) via a path the PR itself introduces, and has zero test coverage.
Suggestion: on RECORD_CLARIFICATION_SUBMISSION, either merge/preserve the prior pending (unconfirmed) entry so its outcome can still be matched, or track a small ordered list of outstanding command ids per request_id instead of a single slot, so an older command's outcome isn't orphaned when a newer one is recorded for the same round.
2. [Major] [Blocking: yes] Gating state is pure in-memory React state — a reload during an ambiguous outcome silently drops the gate
frontend/src/contexts/app-context-chat.tsx (createInitialState, ~1325-1329; no persistence anywhere in the file) / frontend/src/components/chat/clarification-form.tsx (~221-226, outstandingSubmission derivation)
commandOutcomes/clarificationSubmissions are never persisted or rehydrated. A page reload while a clarification reply's outcome is still ambiguous (recorded but unresolved — the exact "locked" state this PR introduces) silently drops the gating record. If the server still authoritatively reports waiting_for_user for the same round after reload — a plausible, ordinary scenario this feature is explicitly built around — the form re-derives active=true with no outstandingSubmission, so it becomes freely submittable again. Server-side, stage_task_command's idempotency only dedupes an identical (task_id, command_id) pair, and a post-reload resubmit mints a brand-new command_id (generateClientMessageId() runs fresh on every submit), so the server has no way to recognize the resubmit as a duplicate of the earlier reply. A user reloading while confused by the (currently unexplained — see Finding 3) locked state is an entirely ordinary action, not a contrived edge case.
Suggestion: persist at minimum the clarificationSubmissions/commandOutcomes needed to reconstruct the gate (e.g. sessionStorage keyed by task/request id), or explicitly accept reload-during-ambiguity as an out-of-scope, documented non-goal — but that should be a deliberate call, not a silent gap.
3. [Major] [Non-blocking, should be raised prominently] Locked-but-unconfirmed submissions render with zero explanatory text
frontend/src/components/chat/clarification-form.tsx (gating effect ~228-271, notice JSX ~815-832)
Issue #1500's invariant is: "if the command may already be committed or in flight, or its outcome is unknown, the client surfaces that ambiguity." When a submission is recorded as accepted: true and no terminal outcome has arrived yet, the effect only sets isSubmitted(true) and returns — none of the three notice blocks (unconfirmed/notApplied/sendFailure) render, so the form shows as locked/disabled with no explanation, and there is no timeout or fallback (no setTimeout/setInterval in the file). The only ways out are a request_id change (new round), an eventual resend-safe outcome, or a full state reset.
To be clear, this is not a conflict with acceptance criterion #8 — that criterion concerns preserving the pre-existing task-state-reassertion behavior for rounds with no tracked submission, not this newly-introduced tracked-submission case — and locking (rather than reactivating with editable fields) is the correct call already validated by the prior gemini-code-assist review (see Prior review resolution below). The remaining gap is narrower but real: nothing is surfaced to the user, who just sees a permanently greyed-out Submit button with no idea why or what to do next.
Suggestion: render the existing unconfirmed-style notice (or a new "outcome pending" notice) for the accepted-but-no-outcome-yet state too, so the lock is at least explained, even without solving the "how long do we wait" question (which the PR reasonably defers).
4. [Minor] [Non-blocking] Regression test for the reactivation fix doesn't actually exercise the fixed path
frontend/src/components/chat/clarification-form.test.tsx (~1033-1046), test "is not reopened by a late terminal event after a turn is established"
This test renders with active=false throughout and never flips back to active=true after the outcome arrives. Both the pre-PR (base commit) and post-PR gating effect short-circuit entirely when active=false, so the test passes identically whether the new gating logic exists or is fully reverted — it provides no regression protection for the criterion it's meant to cover.
Suggestion: strengthen the test so active becomes true again after the late outcome arrives, so it actually exercises the resendSafe/outcome branch.
5. [Minor] [Non-blocking] Combined test-coverage gaps
- No test with
outcome: "failed"present butresend_safeabsent (the existing "legacy frames" test omits both together — a different, less specific case). - No test with a non-
"failed"outcome(e.g."completed") alongsideresend_safe: true. - No test exercises resubmission-after-ack-timeout overwriting a still-pending record — this is exactly the untested path underlying Finding 1.
- No test for a stray
agent_errorcarrying a differenttask_id(cross-task write intocommandOutcomes) — currently harmless per Finding 6, but untested. - No true fresh-mount/reload simulation test — ties to Finding 2.
6. [Minor] [Non-blocking, informational — no action needed] RECORD_COMMAND_OUTCOME is not task-scoped, but this is harmless by design
frontend/src/contexts/app-context-chat.tsx (TASK_SCOPED_ACTION_TYPES ~86-107, dispatch site ~5824)
RECORD_COMMAND_OUTCOME is not in the task-scoping set, so a background task's agent_error could in principle record into commandOutcomes while a different task is being viewed. Verified harmless: commandId is a crypto.randomUUID(), globally unique and never reused, and the only consumer looks it up by that exact id from a component that isn't even mounted for a non-viewed task. Adding this action to the scoped set would instead cause a real regression — the one-time terminal event would be silently dropped for background tasks. No code change recommended.
7. [Minor] [Non-blocking] commandOutcomes/clarificationSubmissions are never pruned on task switch
frontend/src/contexts/app-context-chat.tsx
Unlike the file's own precedent (MAX_TRACKED_TASK_STATE_VERSIONS, MAX_RETIRED_SESSION_TASK_IDS), these maps are only cleared on a full RESET_STATE/RESET_SESSION_CONVERSATION, not on SET_TASK_ID/ADOPT_SESSION_TASK. Entry size is tiny and growth is bounded by realistic session length, so this is a memory-hygiene nit rather than a real risk today. Suggest a follow-up to prune on task switch for consistency with the existing pattern.
8. [Minor] [Non-blocking] Resend-safe outcome arriving for an inactive round is never cleared
frontend/src/components/chat/clarification-form.tsx (~229, !active early return)
A resend-safe outcome arriving while a round is inactive is never consumed/cleared — there's no cleanup path besides the resend-safe branch itself or a full reset — so the record for that old request_id is orphaned in state indefinitely. Verified harmless in practice since there's no evidence of request_id reuse in this codebase; purely a stale-state nit.
9. [Minor] [Non-blocking, robustness] Post-delivery bookkeeping in sendMessage isn't wrapped in try/catch, creating a fail-open gap at the highest-risk moment
frontend/src/contexts/app-context-chat.tsx (~6604-6710, "existing task" branch of sendMessage, post-delivery bookkeeping after sendChatMessage resolves)
This block isn't wrapped in try/catch. No currently-reachable throw site was found in its four reducer dispatches, so this is a defensive gap rather than a demonstrated bug — but if it were to throw, the exception would escape as an unstructured throw with no disposition, so clarification-form.tsx's catch would never call recordSubmission at all, right after the reply was durably confirmed sent.
Suggestion: move recordSubmission(true) to fire immediately after sendChatMessage resolves, before the post-delivery bookkeeping runs.
10. [Minor] [Non-blocking, a11y/test-fragility] Two competing role="alert" regions can render simultaneously
frontend/src/components/chat/clarification-form.tsx (~816, ~826)
The unconfirmed notice and the sendFailure alert both use role="alert", and their guard conditions are not mutually exclusive. Reachable case: an outcome_unknown send failure (sets sendFailure) followed later by a resend_safe: false terminal outcome for that command (sets outcomeNotice="unconfirmed") leaves both rendered simultaneously, since nothing clears sendFailure on that path. Two competing role="alert" regions confuse screen readers and would break any future findByRole("alert") (singular) test.
Suggestion: give at most one of the two role="alert", or clear sendFailure when outcomeNotice is set.
Simplification opportunities
frontend/src/contexts/app-context-chat.tsx:1173:delete:outcome: "failed"is a single-valued literal field onTerminalCommandOutcome, never read by value anywhere (only object-truthiness is checked), and the construction gate (~5823) already restricts this path tooutcome === "failed". Drop the field from the interface and construction site; keepresendSafe/messageCode.frontend/src/contexts/app-context-chat.tsx:1175:delete:messageCodeis recorded (~5831-5833) but has zero production readers outside the two new test files. Drop the field and its extraction until a real consumer exists.frontend/src/contexts/app-context-chat.tsx:1240/frontend/src/components/chat/clarification-form.tsx:138-140:shrink:theclarificationSubmissionsentry shape{ commandId: string; accepted: boolean }is duplicated as an anonymous inline type in both files instead of one exported interface (contrast withTerminalCommandOutcome, which is exported). Extract a sharedClarificationSubmissioninterface.frontend/src/components/chat/clarification-form.test.tsx:925:shrink:theafterEachblock redundantly re-applies the same mock-state reset already done inbeforeEach(~920); onlycleanup()is load-bearing there. Drop the redundant reset fromafterEach.frontend/src/components/chat/clarification-form.test.tsx:942:delete:thesubmitAcceptedtest helper'srequestIdparameter is never passed a non-default value at any of its 7 call sites. Inline the default ("inputreq_r1") as a constant inside the helper and drop the parameter.frontend/src/components/chat/clarification-form.test.tsx:1189:delete:the "keys resolve in both locale trees" test cannot fail —TranslationKeyis derived fromen, so a missingenkey is a compile-time error, not a runtime one, andtranslations.test.ts:66already asserts full en/zh structural parity. Drop the test as redundant and tautological.
net: -8 to -12 lines possible
Verified non-issue
One thing worth preempting: a claim that the frontend reads agent_error's structured fields from the wrong nesting (message.data vs. the backend's root-level shape), implying the whole feature is inert. This was checked and refuted — use-websocket.ts's onmessage handler has a per-type normalization branch for agent_error that wraps the entire raw parsed frame as message.data, so message.data.command_id/.outcome/.resend_safe correctly resolve to the backend's root-level fields in the real production flow (confirmed against origin/main's current websocket.py, which already has companion PR #2124 merged). No action needed.
Prior review resolution
- gemini-code-assist's inline comment on
clarification-form.tsx:245(high priority: form incorrectly reactivates with editable fields while a reply is in-flight) — FIXED in commit63bb3220(current HEAD). The mechanism differs from the suggested patch (it locks onlyaccepted: truein-flight submissions, notoutcome_unknownones, to preserve the advisory retry path), but the reactivation bug itself is gone — a freshly-mounted instance now correctly locks (isSubmitted: true) rather than showing editable fields, per the tests around lines 1064-1090 and the effect logic at lines 228-271. Resolving this thread.
Blocking status & recommended decision
Blocking: yes — recommended event: REQUEST_CHANGES
Blocking issues:
frontend/src/contexts/app-context-chat.tsx:1352-1363/frontend/src/components/chat/clarification-form.tsx:453-464— Major — overwriting a pending submission record orphans an earlier command's outcome, defeating the PR's own duplicate-detection purpose via a path the PR itself introduces; zero test coverage.[new]frontend/src/contexts/app-context-chat.tsx:1325-1329/frontend/src/components/chat/clarification-form.tsx:221-226— Major — gating state is pure in-memory React state; a page reload during an ambiguous outcome silently drops the gate and allows an undetected duplicate resubmission with a freshcommand_id.[new]
Resolved from prior review (non-blocking, for context):
frontend/src/components/chat/clarification-form.tsx:245— gemini-code-assist's reactivation-with-editable-fields concern — FIXED in 63bb322.[prior]
Review round 2 of xorbitsai#2126: - clarificationSubmissions entries become ordered per-round lists: an ack-timeout entry stays resubmittable, so a resubmit appends instead of overwriting, and an earlier command's late outcome can still be matched and surfaced (finding 1 - the single slot orphaned exactly the undetected-duplicate risk the state exists to catch). - Gate semantics over the list: any tracked reply with a non-resend-safe outcome locks the round and surfaces the ambiguity; a durably accepted reply still awaiting its outcome locks with a new 'reply pending' notice instead of a bare greyed-out button (finding 3); the proven-safe unlock requires every tracked reply to be proven not applied, because an unresolved unconfirmed reply may still have been committed. - The ambiguity notice supersedes a lingering send-failure alert so two role=alert regions never compete (finding 10). - The late-terminal-event test now flips active back on and asserts the resend-safe branch actually runs, so it fails if the gating is removed (finding 4); added coverage for resubmit-after-ack-timeout, a frame with outcome but no resend_safe, a non-failed outcome, and a cross-task frame (finding 5). - Simplifications: drop the never-read outcome/messageCode fields from TerminalCommandOutcome, export a shared ClarificationSubmission interface, inline the test helper's unused parameter, drop a redundant afterEach reset and the tautological locale-tree test. Reload-during-ambiguity stays a documented non-goal (state comment): rebuilding the gate from browser storage without durable terminal-event replay would turn a missed outcome frame into a permanent lock; the server-authoritative reconstruction is tracked in xorbitsai#2142 (with xorbitsai#2135/xorbitsai#1904).
|
Round 2 is addressed in one commit, f2d8c79; per-thread replies carry the details. Summary: Fixed in f2d8c79
Disclosed from our own preflight of this round: the first cut of the multi-entry gate left the submitting instance locked behind a stale pending notice in the mixed state (unconfirmed cmd1 unresolved + cmd2 later proven not applied). Shipped version returns that state to the advisory stance, matching a fresh instance, without the proven-safe notice (cmd1's fate is unproven); regression test included. Deferred with tracking issues
No action, per your own analysis: Finding 6 (cross-task recording is harmless by design and scoping it would drop terminal events for background tasks; now pinned by a test). Verification for 63bb322..f2d8c79: 217 focused tests across both touched files pass; full frontend vitest suite passes (exit 0); tsc clean; zero eslint issues on the diff's added lines (line-level intersection against origin/main). |
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
PR Summary: This PR gates clarification-form resubmission on structured terminal command outcomes (resendSafe) rather than raw task status, keyed by a client-minted request_id/commandId pair, so that a reply whose underlying command failed unsafely keeps the form locked instead of silently re-enabling it. It's Part 2 of #1500 (P0), building on the wire-contract additions merged in #2124. This is a re-review: all 10 prior findings from my earlier review + the endorsed gemini-code-assist finding + 5 simplification suggestions were re-verified against the current code (commit f2d8c79), and a fresh, independent design/discovery pass was run from scratch and surfaced significant new findings.
Blocking: yes — recommended event: REQUEST_CHANGES
Round 0 — Design Verdict: acceptable-with-reservations
The core approach is sound and well-reasoned: a client-minted durable command id for correlation, context-level round state that survives ClarificationForm instance replacement, an ordered list rather than a single-slot record (fixing the original overwrite bug from my prior review), and conservative fail-closed defaults when the outcome is unknown. This is the right shape for the problem.
However, F1 + F2 + F3 together mean the mechanism cannot currently close #1500 as implemented. The gate is either functionally inert in production (F1 — no backend path ever emits request_id), or, once wired up, has multiple silent-permanent-lock failure modes with no escape hatch (F2: no positive release on success; F3: a pre-existing task-state version guard can drop the outcome frame entirely). That is exactly the failure mode this PR's own description calls unacceptable — a form the user can never unlock. F1, F2, and F3 are new findings from this pass, not re-litigated ground from my prior review — the prior review's 10 findings were about the record/notify/render logic assuming a working request_id handshake existed; this pass checked whether that handshake exists at all, and traced the outcome-delivery path for gaps.
Findings by Severity
CRITICAL
F1 — request_id is never emitted by the backend, making the entire gating mechanism inert in production.
frontend/src/components/chat/clarification-form.tsx:225,513; frontend/src/contexts/app-context-chat.tsx:3285-3288,5772-5776
Blocking: yes — Trigger: any clarification round on any currently-deployed backend. No code path in websocket.py's frame builders (agent_message, task_waiting_for_user) sets request_id — the real per-round identity is event_id (minted in runtime.py, threaded through react.py/clarification.py), never mapped to requestId in the frontend. Both frontend read sites have no fallback, so requestId is always undefined. This forces outstandingSubmissions to always be [] and recordSubmission to always early-return — only the "legacy backend" fallback path (pre-PR behavior) ever executes. Companion PR #2124 (already merged) adds outcome/resend_safe/message_code to agent_error frames but does not add request_id anywhere.
Suggestion: this PR cannot close #1500 until the backend emits a stable per-round id (reusing event_id or adding request_id explicitly) and the frontend maps it into requestId. Treat this as the actual blocking gap, not the notice/reducer logic downstream of it.
F2 — No positive release signal: a successful command never clears the gate, permanently locking the form for an accepted-then-succeeded reply.
frontend/src/contexts/app-context-chat.tsx:5841-5854; frontend/src/components/chat/clarification-form.tsx:249-252
Blocking: yes — Trigger: once F1 is fixed, any reply whose command later completes successfully. RECORD_COMMAND_OUTCOME only dispatches for outcome === "failed"; the backend's finish_task_command unconditionally stages a terminal event with outcome: "completed" for successes too, and the frontend deliberately discards it (pinned intentional by test app-context-chat.test.tsx:6917-6949). confirmedPending therefore stays true forever for that round — no reducer action can ever populate commandOutcomes[commandId] for the success case, and the only exits are a new requestId or a full state reset. Currently latent (gated behind F1) but becomes live the moment F1 is wired up.
Suggestion: treat outcome: "completed" as a consume/clear signal for the matching submission, closing the lifecycle hole at its source rather than relying on #2143 (which only covers task-switch pruning).
F3 — A pre-existing task-state version guard can silently swallow the versioned agent_error frame carrying the outcome, via a genuine broadcast-ordering race (not a rare edge case).
frontend/src/contexts/app-context-chat.tsx:65,225-261,284-311,2954-2959,5846,5883-5892
Blocking: yes — Trigger: two state-changing events for the same task broadcast close together such that an older-version or same-version-different-run agent_error frame arrives; canAcceptTaskControlVersion only exempts state_version checks when stateVersion === undefined, and once any state_version is stamped there is no exemption for error/outcome frames — handleMessage returns early before the switch/case "agent_error" body runs, skipping both RECORD_COMMAND_OUTCOME and the user-visible error bubble. The version stamped is a live DB snapshot read at broadcast time, not a value frozen at command-accept time, so this is a genuine ordering race, not a deterministic case. No test constructs an older-version agent_error frame. This directly contradicts the PR's stated design argument that exact-id correlation alone (without state-version protection) is sufficient here.
Suggestion: exempt outcome-bearing agent_error frames from the state-version guard (correlate by command_id/request_id instead), or add an explicit test proving the guard cannot drop a terminal-outcome frame.
MAJOR
F4 — RECORD_COMMAND_OUTCOME reducer is non-monotone: a later frame for the same commandId can overwrite a recorded unsafe outcome with a safe one, with no guard.
frontend/src/contexts/app-context-chat.tsx:1362-1369
Blocking: no — unconditional overwrite, unlike this same file's UPDATE_TASK_STATUS case, which explicitly implements first-write-wins for a terminal value with a comment calling out this exact risk class. A concrete reproduction via history-replay was checked and refuted (durable trace-event replay of agent_error doesn't reach this dispatch), so no confirmed live failure path exists today, but a live-broadcast duplicate/race is not ruled out.
Suggestion: don't allow a later frame to flip an already-false resendSafe to true.
F5 — External-scope terminal command errors (external cancel / external turn interrupt) omit command_id entirely, permanently locking the form.
src/xagent/web/api/websocket.py:9766-9782,9783-9805 (vs. :9806-9823) — note: this file is not touched by this PR's diff; flagging as a pre-existing backend gap this PR's design depends on.
Blocking: yes — Trigger: a command reaching a terminal state via the external-cancel or other external-scope path. Both paths send {type, message, task_id, timestamp} with no command_id; only the default/non-external branch includes it. The frontend's recording gate requires a non-empty command_id, so this reaches the same permanent-lock outcome as F2/F3 via a distinct, real trigger squarely within #1500's stated scope.
Suggestion: include command_id on the external-scope error frames too, or explicitly define a policy for how the gate should treat command-id-less terminal errors.
F6 — Two ClarificationForm instances (virtual waiting message and persisted timeline message) can be simultaneously active for the same round, not just sequentially replaced as assumed.
frontend/src/components/task/task-conversation-panel.tsx:484-507 — note: this file is not touched by this PR's diff; flagging as pre-existing code this PR's design depends on.
Blocking: yes — Trigger: a resubmit (or other new message) arriving while the task is still waiting_for_user; activeWaitingMessageId matches by prompt-text equality and hasFinalAssistantMessage only inspects the last message item, so both the virtual and persisted instances can render active=true at once. Each instance owns independent local state via useState (not context) and runs its own effect, so the two can visibly diverge — one showing a lock/notice, the other a live editable form — for the same round. No test renders two instances concurrently.
Suggestion: key activeness by round/request id rather than prompt-text equality, or explicitly design for at-most-one-active-instance and add a regression test asserting it.
F7 — CLEAR_CLARIFICATION_SUBMISSION deletes the entire round's submission array instead of filtering out only the proven-safe commandIds, creating a realistic race with a concurrent RECORD_CLARIFICATION_SUBMISSION.
frontend/src/components/chat/clarification-form.tsx:306-309
Blocking: yes — Trigger: the CLEAR effect fires off a render-time snapshot of allProvenNotApplied while a fresh recordSubmission(true) (from a resubmit, or from the second instance in F6) dispatches asynchronously after await sendMessage(...); React does not serialize dispatches from independent async sources, so RECORD-then-CLEAR interleaving is reachable, silently dropping tracking for a legitimately outstanding, unresolved reply.
Suggestion: have the CLEAR payload carry the specific commandIds being consumed, and have the reducer filter rather than delete-all for the requestId.
F8 — commandOutcomes and clarificationSubmissions grow unbounded for the whole SPA session, breaking this file's own established convention, and clarificationSubmissions keys only by requestId with no task-scoping.
frontend/src/contexts/app-context-chat.tsx (new maps, cf. MAX_TRACKED_TASK_STATE_VERSIONS/MAX_RETIRED_SESSION_TASK_IDS capped at 500 elsewhere in the same file)
Blocking: no — neither new map has a cap or eviction; only a full RESET_STATE/RESET_SESSION_CONVERSATION clears them, and clarificationSubmissions has no task_id component in its key. Overlaps substantially with the already-WAIVED #2143, but the "unbounded for the whole session" framing and cross-task key-collision risk look broader than what #2143's description covers.
Suggestion: cap both maps with the same eviction pattern used elsewhere in this file, and confirm/document request_id global uniqueness or scope the key by task_id.
F9 — Test suite never exercises the real reducer + real form composed together, never covers two concurrent instances, and mislabels the no-request_id path as "legacy backend" when F1 implies it's the only path any current backend exercises.
frontend/src/components/chat/clarification-form.test.tsx; frontend/src/contexts/app-context-chat.test.tsx
Blocking: no — clarification-form.test.tsx fully mocks @/contexts/app-context-chat and hand-reimplements append semantics via mirrorSubmissions(), which never simulates CLEAR_CLARIFICATION_SUBMISSION, so no test observes post-consume state through the real reducer. app-context-chat.test.tsx tests the reducer via a probe component but never renders ClarificationForm. No test mounts two instances concurrently (ties to F6).
Suggestion: add an integration-style test wiring the real reducer to a real ClarificationForm, add a concurrent-instances test, and relabel the no-request_id test to reflect it's the currently-universal path, not a legacy fallback.
MINOR
F10 — Notice copy tells the user to consider resubmitting, but Submit is permanently disabled for that round with no path to re-enable it.
frontend/src/components/chat/clarification-form.tsx (unsafe/unknown-outcome notice copy)
Blocking: no — the underlying behavior (locked, safe-by-default) is correct; only the wording is imprecise about mechanism. Note: the hypothesis that dropping messageCode (a prior-review simplification) caused this mismatch is refuted — #1500's acceptance criteria only require the notice to keep the form locked for "committed"/"unknown" alike, not to visually distinguish them.
Suggestion: reword to point at composing a new message rather than "submitting again."
Prior Review Resolution
FIXED:
- Finding 1 (overwrite bug) —
clarificationSubmissions[requestId]is now an ordered list, append not overwrite; regression test added (app-context-chat.tsx:1370-1386,clarification-form.test.tsx:1132-1168). - Finding 3 (no pending notice) — new
outcomeNotice: "pending"state withrole="status"+ i18n (clarification-form.tsx:277-284,878-881). - Finding 4 (weak regression test) — reopen-after-late-event test now asserts Submit re-enables + notice (
clarification-form.test.tsx:1060-1083). - Finding 10 (competing alert regions) —
setSendFailure(null)on ambiguity notice; pending notice usesrole="status"(clarification-form.tsx:274, test:1240-1268). - gemini-code-assist finding (reactivation with editable fields in-flight) — hardened further for the freshly-mounted instance case.
- 5 simplifications (unused
outcome/messageCodefields,ClarificationSubmissionextracted, unused test-helper param, redundantafterEach, tautological i18n test) — all confirmed against current code. One disclosed minor gap: the newreplyPendingi18n key isn't covered by a per-key resolves-in-both-locales test (deleted as tautological) but is covered by the general structural-parity test (translations.test.ts:66-68) — acceptable.
WAIVED (properly deferred, tracking issues confirmed to match):
- Finding 2 (reload drops gate) — documented tradeoff + explicit code comment (
app-context-chat.tsx:1251-1255), tracked in #2142. - Findings 7+8 (no pruning on task switch / orphaned resend-safe outcome) — tracked in #2143.
- Finding 9 (sendMessage bookkeeping fail-open, no try/catch) — tracked in #2144.
Finding 5 (combined test-coverage gaps) — partially addressed, superseded by new findings F1/F9. Finding 6 (not task-scoped, harmless) — informational, closed, no action needed.
Simplification opportunities
L1172: shrink TerminalCommandOutcome wraps a single resendSafe: boolean field with nothing else. Flatten Record<string, TerminalCommandOutcome> to Record<string, boolean> and the RECORD_COMMAND_OUTCOME payload's nested outcome field to a flat {commandId, resendSafe}.
net: -8 lines possible (low value — only ~6 mechanical access sites, no functional benefit; informational only, not required for merge).
Blocking status & recommended decision
Blocking: yes — recommended event: REQUEST_CHANGES
Blocking issues:
frontend/src/components/chat/clarification-form.tsx:225,513— critical — feature is inert in production, backend never emitsrequest_id.[new]frontend/src/contexts/app-context-chat.tsx:5841-5854— critical — no positive release on command success, permanent lock once F1 is fixed.[new]frontend/src/contexts/app-context-chat.tsx:2954-2959,5846— critical — state-version guard can silently drop the outcome frame via a real broadcast race.[new]src/xagent/web/api/websocket.py:9766-9805— major — external-cancel/interrupt paths omitcommand_id, permanent lock.[new]frontend/src/components/task/task-conversation-panel.tsx:484-507— major — twoClarificationForminstances can be simultaneously active with diverging state.[new]frontend/src/components/chat/clarification-form.tsx:306-309— major —CLEAR_CLARIFICATION_SUBMISSIONdeletes the whole round, racing a concurrent record dispatch.[new]
None of F1–F9 duplicate the prior review's 10 findings — those covered the record/notify/render logic assuming a working handshake; this pass found the handshake itself doesn't exist yet (F1) and traced additional silent-lock paths around it (F2, F3, F5, F6, F7).
| // 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) ?? [], |
There was a problem hiding this comment.
[F1 - Critical, Blocking] No fallback here when requestId is undefined. Since no backend WS frame builder currently sets request_id (verified across websocket.py's agent_message/task_waiting_for_user builders), outstandingSubmissions is always [] in production, so this gating mechanism never engages. This PR + #2124 together do not close #1500 as implemented — the round-identity half of the wire contract is missing.
| // form while its terminal outcome is still pending. | ||
| const recordSubmission = (accepted: boolean) => { | ||
| if (onSend || !dispatch) return | ||
| if (typeof submittedRequestId !== "string" || !submittedRequestId) return |
There was a problem hiding this comment.
[F1 - Critical, Blocking] recordSubmission early-returns whenever requestId is falsy, which is unconditionally the case today since no backend path emits request_id. This makes the append/record logic added by this PR unreachable in production.
| const terminalCommandId = agentErrorData.command_id | ||
| if (typeof terminalCommandId === "string" && terminalCommandId | ||
| && agentErrorData.outcome === "failed") { | ||
| dispatch({ |
There was a problem hiding this comment.
[F2 - Critical, Blocking] RECORD_COMMAND_OUTCOME only dispatches for outcome === "failed". The backend's finish_task_command also stages a terminal outcome: "completed" event for successful commands (task_command_terminal_events.py:100-107), which this deliberately discards (per test at app-context-chat.test.tsx:6917-6949). There is no reducer path that ever populates commandOutcomes[commandId] for a successful command, so confirmedPending in clarification-form.tsx:249-252 becomes permanently true for that round. Please add a consume/clear path for outcome: "completed".
| case "RECORD_COMMAND_OUTCOME": | ||
| return { | ||
| ...state, | ||
| commandOutcomes: { |
There was a problem hiding this comment.
[F4 - Major] RECORD_COMMAND_OUTCOME unconditionally overwrites commandOutcomes[commandId], unlike UPDATE_TASK_STATUS in this same file which implements first-write-wins for a terminal value specifically to avoid this class of risk. A later duplicate/stray frame could in principle flip a recorded unsafe (resendSafe:false) outcome back to safe. Recommend guarding against downgrading an already-false resendSafe. (A concrete reproduction via history-replay was checked and refuted, so this is currently a defense-in-depth gap, not a confirmed live bug.)
| // intact. | ||
| dispatch?.({ | ||
| type: "CLEAR_CLARIFICATION_SUBMISSION", | ||
| payload: { requestId }, |
There was a problem hiding this comment.
[F7 - Major, Blocking] CLEAR_CLARIFICATION_SUBMISSION deletes the entire round's submission array (keyed only by requestId), rather than filtering out just the proven-safe commandIds. This effect fires from a render-time snapshot of allProvenNotApplied, while a fresh recordSubmission(true) (from a resubmit, or from a second concurrently-active form instance for the same round) dispatches asynchronously after await sendMessage(...). React doesn't serialize these two dispatch sources, so a RECORD-then-CLEAR interleaving can silently drop tracking for a legitimately outstanding reply. Recommend the CLEAR payload carry the specific commandIds consumed, with the reducer filtering rather than deleting the whole array.
| // turn a missed outcome frame into a permanent, unexplainable lock | ||
| // (fail-closed with no exit). Reload-during-ambiguity is tracked in | ||
| // #2142 as server-authoritative reconstruction on the #2135/#1904 arc. | ||
| clarificationSubmissions: Record<string, ClarificationSubmission[]> |
There was a problem hiding this comment.
[F8 - Major] Unlike MAX_TRACKED_TASK_STATE_VERSIONS/MAX_RETIRED_SESSION_TASK_IDS elsewhere in this file (both capped at 500 with eviction), neither commandOutcomes nor clarificationSubmissions has a cap or eviction, and only a full RESET_STATE/RESET_SESSION_CONVERSATION clears them. clarificationSubmissions is also keyed by requestId alone with no task_id component. This overlaps with #2143 but "unbounded for the whole session" and the cross-task key-collision risk look broader than what that issue's description covers — worth confirming in scope there.
3 participants
Summary
Part 2 of #1500 (frontend consumption; the wire contract is PR #2124). Closes #1500 when merged together with #2124.
waiting_for_user. While a durably accepted reply is outstanding, reactivation requires the structured terminal outcome broadcast for that exact command to prove the reply was not applied (resend_safe).AppContextkeyed byrequest_id, so the gate survives the submitting component instance being replaced (virtual waiting message vs. persisted timeline message rendering the same round).outcome_unknown) delivery records the submission too: the reply may still have been durably accepted, and its eventual terminal outcome gates the round the same way.AppContextrecords terminalagent_errordispositions keyed bycommand_id. Exact-id correlation is what makes stale or cross-run outcomes inert; a late terminal event while the task is not waiting reopens nothing. The reactivation effect depends on per-round derived values, so an unrelated command's outcome cannot wipe a visible failure alert.Scope
Reload-during-ambiguity is a documented non-goal (added in review round 2): the gating maps are deliberately not persisted to browser storage — without the durable terminal-event replay (#1904), a reload that missed the outcome frame would rehydrate a locked entry whose terminal event is never re-broadcast, turning fail-open into fail-closed with no exit. Server-authoritative reconstruction of the gate across reloads is tracked in #2142, on the same arc as #2135 (durable events as the auto-gating authority) and #1904.
Review-round deferrals, each with a tracking issue: #2142 (reload gate reconstruction, P1), #2143 (prune the gating maps on task switch; clear orphaned entries, P2), #2144 (guard sendMessage post-delivery bookkeeping, P2).
request_id(legacy backends) are deliberately not gated: with no round identity, a recorded reply could gate a different question. Their behavior is unchanged and pinned by a test.onSendpath has no durable command behind it and takes no part in outcome gating; unchanged.Acceptance criteria mapping (#1500)
command_id== the form's minted client message id, stored perrequest_id.waiting_for_user→ form reactivates without reload, draft preserved.UPDATE_TASK_STATUSpath, unchanged.waiting_for_userreassertion behavior is retained (context-level regression test).Verification
clarification-form.test.tsx(40) andapp-context-chat.test.tsx(168) pass; full frontend vitest suite passes (exit 0).tsc --noEmitclean; the diff's added lines introduce 0 new eslint errors or warnings (line-level intersection against theorigin/mainbaseline — the two touched hub files carry 66 pre-existing errors).