Skip to content

fix(web): carry the clarification round id on waiting task-state frames - #2232

Draft
codeacme17 wants to merge 1 commit into
xorbitsai:mainfrom
codeacme17:fix/emit-clarification-round-id-1500
Draft

fix(web): carry the clarification round id on waiting task-state frames#2232
codeacme17 wants to merge 1 commit into
xorbitsai:mainfrom
codeacme17:fix/emit-clarification-round-id-1500

Conversation

@codeacme17

Copy link
Copy Markdown
Contributor

Summary

Part of the #1500 re-slice, and the structural answer to #2166's review round 3: the frontend cannot reconstruct the clarification round identity client-side — the waiting task-state frames carried no id, chat-history rows carry none, and public_trace_events strips clarification_draft from every waiting react_task_end result for all audiences. The runtime already mints one event_id per ask (the clarification's stable identity) and the ask agent_message trace event carries it on every surface; this PR puts the same identity, as an additive request_id field, on the task-state frames a waiting round reaches the client through:

  • the live waiting_for_control task_info and the resume-path waiting task_info, read off the normalized result's clarification_draft;
  • the history-replay task_info and the task_waiting_for_user reassertion, read off the newest persisted ask trace row.

Two helpers carry the policy: _clarification_request_id (tolerates the draft dataclass and its dict form; anything else reads as no identity, never a throw, since these sit on broadcast paths) and _latest_ask_event_id (the scan never reaches past an id-less newest ask, so an older round's id cannot label a newer question).

Scope

  • No DB migration: the id is sourced from live results and persisted trace rows only.
  • Deliberately not emitted: the lease-restore corrective broadcast (question-less; the frontend preserves a known id across an id-less same-question reassertion, and a DB read inside that best-effort exception path is not worth the surface) and the builder-chat waiting response (the builder path takes no part in the retry gate by design).
  • public_trace_events stays untouched: the ask agent_message's data.event_id already survives every projection, and the react_task_end strip remains the disclosure policy it was.
  • Consumers: the frontend round-identity PR fix(frontend): give clarification rounds a real wire identity #2166 reads the new field (request_id ?? event_id, so this field wins wherever emitted); the retry gate lands in fix(frontend): gate clarification retries on structured terminal command outcomes #2167. Both degrade to today's behavior against a backend without this PR.

Verification

  • New suite tests/web/api/test_clarification_round_id_emission.py: helper semantics (draft forms, degradation, newest-ask stop rule) and history-replay integration through send_historical_data_as_stream (both waiting frames carry the newest ask's id; an ask-less task stays id-less).
  • Neighbor frame-shape suites (owner-actor, execution-scope turn wiring, read-surface shape contract, legacy failure replay, public trace events): 189 passed; full tests/web/api batch passes.
  • Ruff format/check and mypy clean.

Part of the xorbitsai#1500 re-slice (review round 3 on xorbitsai#2166 established that the
frontend cannot reconstruct the round identity client-side: the waiting
task_info frames carry no id, chat-history rows carry no id, and
public_trace_events strips clarification_draft from every waiting
react_task_end result for all audiences).

The runtime already mints one event_id per ask (the clarification's stable
identity) and the ask agent_message trace event carries it on every
surface. This adds the same identity as an additive request_id field to
the task-state frames a waiting round reaches the client through:

- the live waiting_for_control task_info and the resume-path waiting
  task_info, read off the normalized result's clarification_draft;
- the history-replay task_info and the task_waiting_for_user reassertion,
  read off the newest persisted ask trace row - the scan never reaches
  past an id-less newest ask, so an older round's id cannot label a newer
  question.

Deliberately not emitted: the lease-restore corrective broadcast (it is
question-less; the client preserves a known id across an id-less
same-question reassertion, and adding a DB read inside that best-effort
exception path is not worth the surface) and the builder-chat waiting
response (the builder path takes no part in the retry gate by design).
No DB migration: the id is sourced from live results and persisted trace
rows only.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a stable identity (request_id) for clarification rounds (Issue #1500) to allow clients to bind replies to exact questions across live delivery, resume, replay, and restore. It adds helper functions to extract the event ID and updates task-state frames to include this ID. Additionally, comprehensive unit tests have been added to verify this behavior. The reviewer suggested an optimization in _latest_ask_event_id to iterate through the trace events in reverse order, which would improve efficiency by returning immediately upon finding the most recent ask.

Comment thread src/xagent/web/api/websocket.py

@rogercloud rogercloud left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major

  • src/xagent/web/api/websocket.py:1229-1239 — The live ask agent_message frame never sets request_id; only the reload/replay path reaches the frontend's waitingRequestId state (frontend/src/contexts/app-context-chat.tsx:867-897,1610-1617,3200-3202). The PR description's claim of a request_id ?? event_id frontend fallback does not exist in code, and 3 of the PR's 4 new stamp sites are task_info-typed frames the frontend never reads request_id from at all — only the replay reassertion (:8320) is currently wired up. Add "request_id": payload.get("event_id") at this ask-frame emission site (the live chat handler already reads eventData.request_id here); this also lets _clarification_request_id and the 3 inert task_info stamps be dropped.
  • src/xagent/web/api/websocket.py:1222 (vs src/xagent/core/agent/pattern/react/react.py:2379-2385) — send_message with expect_response=True, visible=False skips trace persistence, so _latest_ask_event_id (websocket.py:1017-1040) returns the prior round's id on reconnect, misbinding a reply to the wrong round. Persist the trace before the visibility gate, or force visible=True when expect_response=True, matching ask_user_question.
  • src/xagent/web/api/websocket.py:2601-2612 vs :2938-2969 — The live-site request_id stamp lacks the status guard the resume-site has (:3945-3978, gated on status == "waiting_for_user"); an externally canceled a2a task with a stale clarification_draft can emit a non-waiting frame that still carries request_id. Currently inert since no frontend reads it here, but gate on final_task_status == TaskStatus.WAITING_FOR_USER.value for consistency.
  • tests/web/api/test_clarification_round_id_emission.py — The live waiting_for_control task_info (websocket.py:2938-2971) and resume-path task_info (websocket.py:3945-3978) emission sites have zero direct test coverage. Add tests exercising both, including the resume path's status gate.

Minor

  • src/xagent/web/api/websocket.py:1017-1030 — Docstring says the scan "stops at the newest ask" but the loop never breaks (full forward scan, last-write-wins); trace_events at the call site (:7940-7953) is already a materialized list, so reversed() with early return is possible with no copy cost. Fix the docstring, or apply reversed() for a real (if small) perf win.
  • src/xagent/web/api/websocket.py:4252-4266 — The lease-restore broadcast omits request_id correctly only because the frontend does an exact string match on "Task waiting for user response" (frontend/src/contexts/app-context-chat.tsx:5696,5707), uncommented on both sides. Add cross-reference comments, or have the restore frame carry the id explicitly.
  • src/xagent/web/api/websocket.py:1002-1014_clarification_request_id returns any non-empty string with no format check, though COMMAND_ID_PATTERN (:178, used at :405-411) exists for this. Gate with COMMAND_ID_PATTERN.fullmatch for consistency.
  • src/xagent/web/api/websocket.py:992-1014 — The result: Any parameter's dict-form branch is only exercised by a synthetic unit test; every real caller passes a ClarificationDraft. Type as Optional[ClarificationDraft] and drop or demote the dict branch.
  • src/xagent/web/api/websocket.py:7930-7966 — The replay request_id backfill mutates task_event["data"] after append, relying on dict aliasing; the isinstance(..., dict) check near line 7965 is dead code. Compute the id before building/appending the frame.
  • src/xagent/web/api/websocket.py:992-1014 / :1017-1040 — Wire field request_id collides in name with the existing request_id concept for provider/HTTP correlation (src/xagent/core/tools/artifacts.py:71, src/xagent/core/model/image/openai.py:150) while the codebase already calls this identity event_id (src/xagent/core/agent/clarification.py:17). Rename to clarification_request_id/round_id.
  • src/xagent/web/api/websocket.py:2952-2963, :3959-3972 — Walrus-inside-conditional-dict-spread (**({...} if (x := ...) else {})) leaks the temp variable into function scope for no benefit. Use a plain local variable before the dict literal, as done at :7955-7966.
  • tests/web/api/test_clarification_round_id_emission.py — Gaps: _trace_row's expect_response param (~line 64-68) never exercised non-default; no dict-form draft test with non-string event_id; stubbed cache (~lines 143-144) never exercised end-to-end; no tied-timestamp case for _latest_ask_event_id; __all__ = ["_test_db"] (line 19) is an unconventional F401 workaround. Add the missing cases; use # noqa: F401 instead.
  • src/xagent/web/api/websocket.py:998-1002 — Docstring claims the id is carried "across live delivery, resume, replay, and restore," but restore deliberately never sets it. Narrow the docstring, or comment the omission at the restore call site.

Blocking: yes — recommended event: REQUEST_CHANGES

  • src/xagent/web/api/websocket.py:1229 — Major: live clarification asks never carry a round id, defeating the PR's stated purpose for the primary (non-reload) scenario. [new]
  • src/xagent/web/api/websocket.py:1222 — Major: an invisible ask (visible=False + expect_response=True) causes replay to mislabel a new question with a stale round id. [new]

codeacme17 added a commit to codeacme17/xagent that referenced this pull request Sep 9, 2026
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.
@codeacme17
codeacme17 marked this pull request as draft September 11, 2026 03:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants