fix(frontend): gate clarification retries on structured terminal command outcomes - #2167
fix(frontend): gate clarification retries on structured terminal command outcomes#2167codeacme17 wants to merge 9 commits into
Conversation
Part 1 of the xorbitsai#1500 re-slice (surfaced by review round 3 on xorbitsai#2126, finding F1: no backend emits request_id, so every consumer of the frontend's request_id plumbing was inert in production). - Adopt the ask frame's event_id - the stable per-ask identity the runtime mints (core/agent/clarification.py: 'event_id is the clarification's stable identity') and forwards on ask frames and replayed trace rows - as the clarification round id everywhere the frontend previously read only the never-emitted request_id: the task_waiting_for_user handler, the agent_message trace reader, and a new findWaitingRequestId trace fallback in the conversation panel, sourced from the same ask events the prompt and interactions already fall back to. request_id stays the preferred field so a backend that later adopts the explicit name wins. - Stop the task-state version guard from swallowing error notices (round-3 finding F3): a stale-versioned error/agent_error frame now falls through with its control tuple neutralized - the notice (and, since xorbitsai#2124, the structured terminal command outcome) is sent exactly once and is not versioned state, so it keeps flowing while every status side effect is suppressed. Extends the guard's own documented rule for UNversioned error frames to versioned ones. Differentially verified: the regression test fails on the unfixed code. - Key the panel's active waiting instance by round id when one exists (round-3 finding F6): prompt-text equality could leave the persisted timeline message and the virtual waiting message simultaneously active for one round; the virtual copy now stays inert whenever a timeline message owns the round, so at most one form accepts a submission.
There was a problem hiding this comment.
Code Review
This pull request introduces robust tracking and handling of terminal command outcomes for clarification forms (Issue #1500). It ensures that when a user submits a clarification reply, the system tracks the command's terminal disposition (whether it was successfully applied, failed but safe to retry, or failed with an unknown outcome) using unique client-minted message IDs. The state is managed globally in the chat context to survive component remounts, and the UI displays appropriate feedback notices (pending, not applied, or outcome unknown) while locking or unlocking the form accordingly. Additionally, it updates the task conversation panel to uniquely identify waiting rounds using the ask frame's event_id as a fallback when request_id is absent. There are no review comments provided, so I have no feedback to provide.
Review round 1 of xorbitsai#2166: nullish coalescing let an empty-string (or non-string) request_id block the event_id fallback, leaving the round id-less. Both extraction sites now take the first non-empty string candidate, matching the panel helper's semantics; regression test covers the empty-string fallback.
b86316d to
2faaee3
Compare
rogercloud
left a comment
There was a problem hiding this comment.
Major
frontend/src/contexts/app-context-chat.tsx:3018,5963-5972— a stale-versionedagent_errorframe now falls through the version guard but itsADD_MESSAGEdispatch (~5963-5972) is unconditional and not routed throughisDuplicateMessageForViewedTask, unlike the siblingerror/task_errorcase (~5998-6016). The backend documents (websocket.py:9842-9843) that an operator retry can legitimately resend the samecommand_idvia a terminal broadcast twice, so every such resend now produces a duplicate, unremoved error bubble in the transcript. Route this dispatch through the same dedup check as the sibling case, or justify the exemption and add a regression test.
Trigger: operator-retry redelivery of a terminal outcome for the samecommand_id. Impact: duplicate user-visible error message, no test coverage.frontend/src/components/task/task-conversation-panel.tsx:907-915(findWaitingRequestId~181-219) — when both the authoritativewaitingRequestIdand the trace-scan/prompt-text fallbacks fail to resolve a round id (possible transient race during initial history load/reconnect), the virtual clarification message renders active withinteractionRequestId=undefined, bypassing the retry-gate entirely (an undefined key means "not gated" perclarification-form.tsx:231-236) — fail-open in a feature meant to be fail-closed. Treat unresolved round identity as locked/inert, or block virtual-message rendering until history load settles.
Minor
frontend/src/contexts/app-context-chat.tsx:1388-1395,1411-1418— the 500-entry FIFO eviction forcommandOutcomes/clarificationSubmissionscan silently un-strand or re-strand round state (an evicted unsafe outcome could later be "upgraded"; evicting an outstanding submission silently reopens the form with no record it was locked). Needs ~500 accumulated entries so low likelihood, but untested. Evict entries not referenced by any outstanding round, or add a boundary regression test.frontend/src/components/chat/clarification-form.tsx:891-919— the lock notice renders inside a user-collapsibleCollapsibleContent, contradicting the PR's claim it "anchors near the message box, not the Submit button"; collapsing hides the only explanation, and the copy points at an unrelated component. Move the notice outside the collapsible region and correct the copy.frontend/src/components/chat/clarification-form.tsx:525— mints a newclientMessageIdon every resubmit (including an advisory retry after an ack-timeout), unlikeChatInput.tsx:762-775which reuses the id for unchanged-content retries so the backend can dedupe; the file's own comment (~584-588) admits a resubmit "could answer the question twice." Reuse the id on unchanged content, mint a new one only when the answer changes.frontend/src/contexts/app-context-chat.tsx:1351,1871-1875,1495-1500—RESET_STATE/RESET_SESSION_CONVERSATION, dispatched by ordinary in-app navigation (task/agent/build pages), wipescommandOutcomes/clarificationSubmissionsentirely viacreateInitialState(). The PR documents non-persistence across a page reload as a deliberate tradeoff, but this covers ordinary SPA navigation too — an ambiguous/locked round silently returns ungated. Scope these maps to survive the reset, or extend the documented tradeoff explicitly.- Test coverage:
app-context-chat.test.tsx:7171-7265's real-reducer integration test covers only the ack-timeout→unsafe→lock branch; the accepted-submit/proven-safe-reactivation path — the PR's headline acceptance criterion — is deferred to mock-based suites that fully mockuseApp, so it's never exercised against the real reducer. Also missing: eviction-boundary, duplicate stale-frame bubble, and RESET_STATE tests. Add a real-reducer test for proven-safe→reactivate plus the listed gaps.
Simplification
- L1388: shrink: near-identical bounded-eviction blocks duplicated for
commandOutcomes(app-context-chat.tsx:1388-1395) andclarificationSubmissions(:1411-1418). Extract a sharedevictOldest(map, keyToKeep, maxSize)helper used by both. - L268: design note: the retry-gate in
clarification-form.tsx:268-347is a pure function of (active, outstandingSubmissions, commandOutcomes) computed via a 9-dependency imperativeuseEffectwith a module-level sentinel — could be a single render-derived value, eliminating the effect and sentinel.
net: -8 lines possible
Blocking: yes — recommended event: REQUEST_CHANGES
| ) | ||
| ) return | ||
| ) { | ||
| if (message.type !== "error" && message.type !== "agent_error") return |
There was a problem hiding this comment.
This is the fall-through that lets a stale-versioned agent_error frame proceed (instead of returning early) so RECORD_COMMAND_OUTCOME can still fire. But the ADD_MESSAGE dispatch further down this same case (~5963-5972) is unconditional and not routed through isDuplicateMessageForViewedTask, unlike the sibling error/task_error case (~5998-6016). The backend documents (websocket.py:9842-9843) that an operator retry can legitimately resend the same command_id via a terminal broadcast twice, so every such resend now produces a duplicate, unremoved error bubble in the transcript.
Route the agent_error ADD_MESSAGE dispatch through the same dedup check as the sibling case, or justify the exemption and add a regression test.
Blocking: trigger is a documented, legitimate backend operator-retry scenario; impact is a duplicate user-visible error message with no test coverage.
| interactionsActive={ | ||
| state.currentTask?.status === "waiting_for_user" | ||
| && activeWaitingMessageId === null | ||
| } |
There was a problem hiding this comment.
When both the authoritative waitingRequestId and the trace-scan/prompt-text fallbacks (findWaitingRequestId) fail to resolve a round id, the virtual clarification message renders active with interactionRequestId=undefined — which bypasses the retry-gate entirely (an undefined key means "not gated" per clarification-form.tsx:231-236). This is a fail-open path in a feature whose whole purpose is fail-closed duplicate prevention.
Treat unresolved round identity as locked/inert rather than active, or block virtual-message rendering until history load settles.
| ) | ||
| if (evictable !== undefined) delete commandOutcomes[evictable] | ||
| } | ||
| return { ...state, commandOutcomes } |
There was a problem hiding this comment.
This 500-entry FIFO eviction (and the mirrored block for clarificationSubmissions at ~1411-1418) can silently un-strand or re-strand round state: an evicted unsafe outcome could later be "upgraded" by a stale re-delivery, and evicting a still-outstanding submission silently reopens the form with no record it was ever locked. Requires ~500 accumulated entries so low likelihood, but untested.
Prefer evicting entries not referenced by any outstanding round, or add a boundary regression test.
Simplification: this block and the clarificationSubmissions eviction block (~1411-1418) are near-identical — extract a shared evictOldest(map, keyToKeep, maxSize) helper.
| ))} | ||
| </div> | ||
|
|
||
| {outcomeNotice === "unconfirmed" && ( |
There was a problem hiding this comment.
These lock notices render inside a user-collapsible CollapsibleContent (see onOpenChange further up), contradicting the PR's claim it "anchors near the message box, not the Submit button"; collapsing hides the only explanation, and the copy ("use the message box below") points at an unrelated component.
Move the notice outside the collapsible region and correct the copy.
| // 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() |
There was a problem hiding this comment.
Mints a new clientMessageId on every resubmit (including an advisory retry after an ack-timeout), unlike ChatInput.tsx:762-775 which reuses the id for unchanged-content retries so the backend can dedupe. The file's own comment nearby (~584-588) admits a resubmit "could answer the question twice."
Reuse the id on unchanged content, minting a new one only when the answer actually changes.
| contextUsage: null, | ||
| sessionConversation: { ...initialSessionConversationState }, | ||
| commandOutcomes: {}, | ||
| clarificationSubmissions: {}, |
There was a problem hiding this comment.
commandOutcomes/clarificationSubmissions are initialized here and get wiped whenever RESET_STATE or RESET_SESSION_CONVERSATION spreads createInitialState() (~1871-1875, ~1495-1500). Those actions are dispatched by ordinary in-app navigation (task/agent/build pages), not just reload. The PR documents non-persistence across a page reload as a deliberate tradeoff, but this covers ordinary SPA navigation too — an ambiguous/locked round silently returns ungated after switching tasks/agents in-app.
Scope these maps to survive this reset, or explicitly extend the documented tradeoff to cover this path.
…ss reasserts Review round 2 of xorbitsai#2166: - A failed round-id match in activeWaitingMessageId falls through to the prompt-text fallback instead of short-circuiting: replayed history rows carry no interactionRequestId, and with the ask as the last assistant message the virtual bubble is suppressed too, so the short-circuit could leave zero active reply instances for a waiting task (Critical). - The reducer keeps waitingRequestId across an id-less reassertion that re-sends the SAME question (reload/reconnect frames), while an id-less frame carrying a different question - a legacy-backend new round - still clears it (Major). - ChatInput's currentInteractionRequestId now receives waitingRoundId, the same identity (with the ask-trace fallback) the form path uses, so a free-text reply binds to the round after a reload too (Major). - findWaitingRequestId's fallback reads the replayed waiting result's real identity at result.clarification_draft.event_id and drops the agent_message arm, which never matches production trace rows (live asks land in the transcript instead); tests now use production shapes. - The stale-error exemption comment states the actual asymmetry with the guard's unversioned rule instead of claiming parity; added tests for the plain-error-type exemption, task_error's deliberate exclusion, and the stop-at-newest scan invariant. - The first-non-empty-string extraction is one shared lib/utils helper instead of three copies.
2faaee3 to
e0f69f0
Compare
…ting it Review round 3 of xorbitsai#2166. The round established that client-side reconstruction of the waiting round identity cannot work: the waiting task_info frames carried no id, chat-history rows carry no id, and public_trace_events strips clarification_draft from waiting react_task_end results for every audience. The companion backend PR now emits request_id on the waiting task_info and replay reassertion frames, so this PR sheds the reconstruction machinery instead of patching it: - findWaitingRequestId, its trace-event type, and the waitingRoundId scan are deleted; the round id is read directly off the task state, and the task_info shaping maps the frame's new request_id into waitingRequestId (the new consumption site). - The active timeline instance's form receives the resolved round id when its own row is id-less (replayed chat-history rows), so an elected form never submits an id-less reply (round-3 finding on the text-fallback election). - The prompt-text fallback skips lookalike rows that carry a DIFFERENT round id - only id-less rows may be text-elected. - The one adoption site production always exercises - the live ask's agent_message data.event_id becoming the transcript message's interactionRequestId - now has its own test, alongside the new task_info consumption test. Kept from round 2: the reducer's same-question id preservation, ChatInput's round id, the single-active-instance rule with its fall-through, and the shared firstNonEmptyString helper.
e0f69f0 to
2d8bf7e
Compare
Review round 4 of xorbitsai#2166: - The task_waiting_for_user handler reads request_id only: its emitters (backend xorbitsai#2232) carry the round identity under the explicit name and never emit event_id, so the event_id candidates there were dead code testable only with hand-crafted frames. The ask frame's event_id stays adopted where it genuinely lives - the agent_message trace reader. The prior review reply claiming those candidates were exercised is corrected in the thread. - A duplicated delivery of the SAME terminal agent_error broadcast no longer adds a second bubble: dedup is keyed on the frame's durable identity (command_id + outcome_version) and never on text, so two distinct commands failing with identical redacted text both stay visible. - The panel's round-id read is gated on the task actually being on screen (currentTask.id === taskId), the same guard the sibling ChatInput wiring uses - new code must not re-enter the task-switch window tracked in xorbitsai#2221. - The active-item id resolution uses firstNonEmptyString for the same empty-string semantics as every other id read; the helper gains a direct unit test.
…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.
… 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.
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).
Folds the xorbitsai#2126 review round 3 findings that belong to the gating layer (the round-identity and version-guard findings landed in the preceding commit's PR): - F4: RECORD_COMMAND_OUTCOME is monotone in the unsafe direction - a duplicate or racing frame may downgrade a proven-safe reading but never upgrade an unsafe one, mirroring UPDATE_TASK_STATUS's first-write-wins for terminal values. - F7: CLEAR_CLARIFICATION_SUBMISSION names exactly the commands the consuming render verified and the reducer filters rather than deleting the round, so a submission recorded between that render's snapshot and the dispatch survives with its outcome still matchable. - F8: both gating maps get the file's standard insertion-ordered eviction (MAX_TRACKED_COMMAND_OUTCOMES / MAX_TRACKED_CLARIFICATION_ROUNDS = 500); task-switch pruning stays tracked in xorbitsai#2143. - F10: the ambiguity notice now points at the message box for a deliberate fresh reply instead of implying the locked Submit can be used. - F9: an integration test runs the real reducer against the real ClarificationForm across a full round (ask identified by event_id, ack-timeout submission recorded through the reducer, unsafe terminal outcome locks the form with the notice); plus reducer-level tests for the monotone outcome, the filtered CLEAR, and a stale-versioned agent_error frame whose outcome is still recorded while its control tuple is discarded.
2d8bf7e to
9c4a6bc
Compare
Summary
Part 2 of the #1500 re-slice: the clarification retry gate, re-cut on top of the round-identity PR (#2166). Closes #1500 when merged. Replaces #2126, whose three review rounds are all folded in here; see that PR's closing comment for the finding-by-finding map.
waiting_for_user. Each round's outstanding replies are tracked inAppContext(an ordered list keyed by the round id, surviving component-instance replacement), and reactivation requires the structured terminal outcome from fix(web): expose structured terminal command outcomes on agent_error frames #2124'sagent_errorframes to prove every tracked reply was not applied.RECORD_COMMAND_OUTCOMEis monotone in the unsafe direction (a racing frame can downgrade a proven-safe reading, never upgrade an unsafe one);CLEAR_CLARIFICATION_SUBMISSIONnames the verified commands and filters instead of deleting the round, so a concurrently recorded submission survives; both gating maps get the file's standard 500-entry insertion-ordered eviction; the ambiguity notice points at the message box instead of the locked Submit; an integration test drives the real reducer against the realClarificationFormacross a full round.Scope
sendMessage's post-delivery bookkeeping: ref(frontend): guard sendMessage post-delivery bookkeeping so a throw cannot masquerade as a failed delivery #2144 (P2).command_idby disclosure policy (fix(web): expose structured terminal command outcomes on agent_error frames #2124); first-party clarification submissions never take external scope (client-supplied scope is refused server-side), so the gate never meets an identity-free frame for a command it tracks — an unmatched frame fails locked, never falsely unlocked.event_idforwarding) stay ungated, preserving today's behavior, pinned by test.command_idthrough a second terminal broadcast (outcome_versionexists for this); the frontend deliberately reads neitheroutcome_versionnortask_run_id, so a second outcome arriving after the round's record was consumed downgrades the stored reading (monotone) but cannot re-lock the already-unlocked round. Revocable proofs belong to the durable-authority work in fix(web): live terminal command broadcast can assert resend_safe before the disposition is durably persisted #2135/fix: replay terminal command outcomes #1904.Acceptance criteria mapping (#1500)
command_id; submissions are tracked per round id (from fix(frontend): give clarification rounds a real wire identity #2166).waiting_for_user→ the form reactivates without reload, draft preserved, record consumed.Verification
agent_error's outcome is recorded while its control tuple is discarded; the F4/F7 reducer behaviors are pinned through the real reducer.