fix: define agent message display contract - #2021
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
xagent/frontend/src/components/build/agent-builder-chat.tsx
Lines 364 to 367 in 09e7969
Keep non-waiting builder messages as separate bubbles
For an ordinary chat-surface message with expect_response: false, execution continues, but this overwrites the single assistant placeholder. A later ai_message, another ordinary message, or the task_completed handler overwrites that same last message again, so the newly supported message disappears from Agent Builder chat after the run advances; append/preserve non-waiting transcript messages rather than storing them only in the final-answer placeholder.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if expect_response or message_type == "question": | ||
| return "chat" | ||
| if not visible: |
There was a problem hiding this comment.
When the allowed send_message input combines message_type: "question", expect_response: false, and visible: false, this branch resolves the message to chat before checking visibility. Since the websocket handler no longer performs its former independent visible is False guard, the supposedly hidden message is persisted, broadcast, and rendered by the frontend; check visible before applying the legacy question fallback.
Useful? React with 👍 / 👎.
|
Addressed the earlier Codex P2 finding in @codex review |
ad0f759 to
036ca93
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad0f759fd3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| } else if (data.event_type === "agent_message") { | ||
| const displayReply = data.data?.message || "" | ||
| } else if (isMessageDisplayEventType(data.event_type) && messageSurface === "chat") { |
There was a problem hiding this comment.
When the Builder receives a chat-visible message with expect_response: false, this branch writes it into the sole assistant placeholder but never appends a fresh placeholder. The later ai_message and task_completed handlers also update the last assistant message, so a normal final answer replaces this supposedly durable transcript bubble. Fresh evidence in this head is that the new branch merely calls setMessages to replace lastMsg; it does not perform the stated finalization-and-append step.
Useful? React with 👍 / 👎.
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
Summary
This PR introduces an explicit message-display contract so that where an agent message renders (chat transcript vs. execution timeline vs. dropped) is decided by a single named surface value instead of being inferred ad hoc at each call site. It adds src/xagent/core/agent/message_display.py (resolve_message_display) and frontend/src/lib/message-surface.ts (getMessageSurface / expectsUserResponse), then rewires PatternRuntime.send_message, the Auto/DAG/ReAct adapters, websocket.py's outbound event factory, persistence and replay, and four frontend consumers onto those helpers. The stated goal (issue #554) is to decouple visibility from expect_response and to stop expect_response from doubling as a visibility switch. Scope is 19 files, +578/-92, spanning backend Python and frontend TypeScript.
Update since last review
Since the previous automated review round, the author pushed 036ca931, which addresses Prior Finding B in the forward direction: agent-builder-chat.tsx now pushes a fresh assistant placeholder after a non-waiting agent_message, and a regression test was added (agent-builder-chat.test.tsx:258-298). Per the author's reply, that finding is considered fixed. Verification shows the forward-order case is indeed fixed, but the symmetric reverse-order case is not — details in the checklist below. Prior Finding A was re-examined and is being waived by design; no code change is expected there.
Approach verdict: acceptable-with-reservations
The direction is right and materially reduces scattered routing logic. Two design-level reservations stand:
- Default surface flip without model-facing guidance (F5). Every ordinary
message_type="info"message now resolves tochatwhere it previously resolved toagent_progress(timeline-only). The newdisplaytool-schema field has nodescription, and there is no system-prompt guidance anywhere insrc/xagenton chat vs. timeline semantics. Every existing agent's routine narration silently migrates into the main transcript with zero migration path. - A "centralized" contract that exists twice (F6). The routing algorithm is hand-reimplemented in Python and TypeScript, and the two copies have already drifted in three confirmed ways before the PR even lands. A contract meant to be single-sourced should not be two independent literal tables.
Additionally, expectsUserResponse now returns true purely on message_type === "question", independent of expect_response — which re-couples waiting-state to message_type in the way #554 explicitly set out to avoid, and is the root cause of F1 and F10.
Prior findings checklist
Prior Finding A — message_display.py:35, "Honor visibility before question routing" — WAIVED (by design).
The precedence is real: visible=false combined with expect_response=True or message_type="question" does resolve to "chat" (message_display.py:33-36), and the websocket handler's old unconditional visible is False early-return is gone. But this is deliberate and documented in the function's own docstring ("Visibility and response waiting are deliberately separate concerns. A response-bearing message is always chat-visible..."), and locked in by an explicit test — tests/core/agent/test_runtime.py:568-583, test_runtime_send_message_resolves_display_independently_from_waiting, asserting hidden_question["display"] == "chat". The old behavior suppressed even response-required questions, leaving the UI unable to solicit a reply that the backend was blocking on. Waived. One narrower sub-case is undocumented and untested: message_type="question", expect_response=False, visible=False also becomes chat-visible, with no stated rationale — see the minor note inline.
Prior Finding B — agent-builder-chat.tsx:352, 364-367, "Preserve non-waiting Builder messages before final output" — PARTIAL / NOT FULLY FIXED.
The forward-order case is fixed (agent-builder-chat.tsx:362-382 now pushes a fresh placeholder after a non-waiting agent_message), with a regression test. The reverse order is still broken: the ai_message branch (agent-builder-chat.tsx:300-351, overwrite at ~:345) mutates updated[updated.length-1].content in place with no push, so if a final ai_message lands first and a non-waiting agent_message arrives after it, the agent_message branch (:362-372) overwrites the finalized final answer and its content is lost. Only the forward order is tested. Tracked below as F2.
Findings
Blocking
F1. Historical replay of a stray message_type="question" row can flip a completed task's status back to "waiting for user" — frontend/src/lib/message-surface.ts:36-47, frontend/src/contexts/app-context-chat.tsx:3077-3087 — severity: high — Blocking: yes.
expectsUserResponse now returns true whenever message_type === "question", independent of expect_response, and app-context-chat.tsx:3077-3087 dispatches UPDATE_TASK_STATUS -> waiting_for_user off that. Historical trace-event replay runs through the same handleMessage path as live messages (live socket handler app-context-chat.tsx:2378, replay scheduler :6600, both passing data verbatim), and raw TraceEvent rows are replayed with original field values intact (websocket.py:7908-7947) — unlike the chat-history replay path (websocket.py:8025-8051), which deliberately hardcodes expect_response: False on reconstructed rows precisely so old questions cannot flip status back. The triggering row (event_type=agent_message, message_type=question, expect_response=false) is reachable: the ReAct send_message tool schema (react.py:2131-2154) exposes message_type and expect_response as independent LLM-controlled arguments with no validation tying them together, and only expect_response gates real suspension (react.py:2356). The single post-replay status correction (websocket.py:8158-8165) fires only for tasks already PAUSED/WAITING_FOR_USER — nothing re-asserts status for a COMPLETED task. Pre-PR, expectsUserResponse was gated on expect_response === true alone, so this is newly introduced. No test covers it.
Suggestion: gate expectsUserResponse on expect_response === true (optionally OR'd with message_type === "question" && expect_response !== false), or normalize expect_response: false question rows during trace replay the way websocket.py:8025-8051 already does for chat history.
F2. Builder chat: reverse-order arrival silently destroys the final answer — frontend/src/components/build/agent-builder-chat.tsx:345, 362-372 — severity: high — Blocking: yes. (Carried over from Prior Finding B; forward case fixed, reverse case open.)
See the Prior Finding B entry above. Secondary artifact of the same fix: a turn ending via task_completed with genuinely empty final content leaves the pushed empty placeholder to be filled with a canned filler string, showing as a spurious extra bubble.
Suggestion: have the ai_message/task_completed branch mark its bubble finalized (or push instead of overwrite), and make the agent_message branch refuse to overwrite a finalized bubble — push a new one instead. Add a test for the reverse ordering, and skip the filler placeholder when final content is empty.
F3. Content-based replay dedup can silently drop a real durable chat message — src/xagent/web/api/websocket.py:7899-7906, 8025-8031 — severity: high — Blocking: yes.
trace_message_keys is built from TraceEvent rows whose event_type is in {"agent_message","ai_message"}, and a durable TaskChatMessage row is then skipped on replay when its (role, content, attachment_fingerprint) key is already in that set — with no turn-id disambiguation for assistant rows. The dedup logic is unchanged by this PR, but the PR changes which event_type an ordinary send_message(..., message_type="info") persists as: pre-PR agent_progress (never fed the set), post-PR agent_message (now feeds it) — confirmed by the PR's own new test test_agent_outbound_event_type_separates_progress_from_questions (tests/web/test_agent_checkpoint_stream.py:210-220). So an agent that calls send_message("Done.") mid-task and whose final answer is also literally "Done." will, on reload, have the durable final-answer row silently skipped. Narrow but real, and short generic strings ("Done.", "OK", "Task completed.") make it plausible. No test covers it.
Suggestion: include turn/message id (or the source trace event id) in the dedup key for assistant rows, or restrict trace_message_keys to the event types that actually shadow durable chat rows.
F4. Agent Builder's outbound bridge now silently drops final-answer stream payloads entirely — src/xagent/web/api/websocket.py:10166-10171 (vs. :1126-1148) — severity: high — Blocking: yes.
The main live-delivery path (make_agent_outbound_handler, websocket.py:1126-1148) intercepts final_answer_start/delta/end/error before calling the shared factory, including a _reconcile_streamed_final_answer repair step. send_builder_outbound_message (websocket.py:10166-10171) calls create_agent_outbound_stream_event directly with no such interception — and that factory returns None (event dropped, nothing sent) for display == "stream", which is exactly what resolve_message_display returns for any final_answer_* event type (message_display.py:39-40). The path is reachable: Builder wires this handler on AgentService running the normal ReAct pattern (default execution_mode="balanced" → ReAct), and ReAct unconditionally instantiates ReActFinalAnswerStreamer; no builder-specific disable flag exists. Pre-PR, send_builder_outbound_message had no final-answer branch and sent the event unconditionally — mislabeled as agent_message, but delivered. This is a regression from "delivered but mislabeled" to "silently lost", with no error and no fallback.
Suggestion: factor the final-answer interception out of make_agent_outbound_handler and reuse it in send_builder_outbound_message (or route Builder through the same handler), and add a Builder-path test asserting a streamed final answer is delivered.
Should fix (non-blocking)
F5. Default display surface for ordinary info messages flipped from timeline to chat, with no LLM-facing guidance — src/xagent/core/agent/message_display.py:39-45, src/xagent/core/agent/pattern/react/react.py:2150-2153 — severity: medium — Blocking: no (UX/volume regression; no data loss or state-machine violation).
Pre-PR, any non-question agent_message — including the default message_type="info" — resolved to agent_progress (timeline-only). Post-PR, none of the earlier branches (question/expect_response, explicit display, agent_status, "progress" message_type) match a plain info message, so the fallback returns "chat". Meanwhile the new display tool-schema field (react.py:2150-2153, enum ["chat","timeline"]) has no description, and no system-prompt text anywhere in src/xagent explains chat vs. timeline. Every existing agent's routine narration now floods the main transcript, with nothing telling the model how to opt out.
Suggestion: at minimum add a description to the schema field spelling out the two surfaces; preferably keep ordinary info messages on timeline unless the model explicitly asks for chat, and document the change for prompt authors.
F6. Backend and frontend implementations of the "shared" contract have measurable divergences — src/xagent/core/agent/message_display.py:33-45, frontend/src/lib/message-surface.ts:20-47 — severity: medium — Blocking: no (latent, no active producer triggers them today).
Three confirmed divergences with concrete inputs: (a) Python's expect_response/question short-circuit has no event_type gate (message_display.py:33-34) while TS requires eventType === "agent_message" (message-surface.ts:42-46), so event_type="ai_message", display="timeline", expect_response=True → "chat" in Python, "timeline" in TS; (b) Python uses display or metadata_display (falls through on "") while TS uses display ?? metadata.display (nullish only), so display="" with a metadata fallback resolves differently; (c) user_message is in both chat-routing lists but missing from TS's isMessageDisplayEventType gate, and chat_message is listed in both routing tables with no producer anywhere.
Suggestion: single-source the table — generate one side from the other, or add a shared fixture list of (event_type, data) → surface cases asserted by both the Python and TS suites so drift fails CI.
F7. An off-enum display value silently blackholes the agent's own message while reporting success to the model — src/xagent/core/agent/pattern/react/react.py:2339-2345 (schema at :2150-2153; drop at websocket.py:1073-1080) — severity: medium — Blocking: no (requires an off-spec model response).
The schema advertises display: enum ["chat","timeline"], but the argument is passed through as str(display) if display is not None else None with no allow-list check, and resolve_message_display accepts all five internal surfaces including "stream"/"ignore". A model emitting display: "ignore" gets its message dropped by create_agent_outbound_stream_event (no broadcast, no persistence), while the tool-call recorder still reports status="completed"/"sent" back into the LLM context (react.py:2347-2355, 2388-2392) with no delivery check. The model believes the message was sent; it wasn't, and nothing in the loop signals otherwise.
Suggestion: validate the argument against the advertised enum (coerce or reject), and/or have the tool result reflect actual delivery.
F13. Test coverage gap at exactly the places these defects live — frontend/src/contexts/app-context-chat.tsx, tests/web/test_agent_checkpoint_stream.py — severity: medium — Blocking: no (per policy), but flagging prominently.
app-context-chat.tsx is the most consequential frontend consumer of the new contract and was substantially rewritten here (~30 lines of hand-rolled routing collapsed into the shared helpers), yet app-context-chat.test.tsx is untouched by this PR. There is no dedicated unit test for resolve_message_display's branches in isolation (no tests/core/agent/test_message_display.py), and the new backend tests exercise producer-side functions only — nothing invokes the actual historical-replay function to assert that a legacy row with no display field replays to the correct surface, despite replay being an explicit coverage point in #554. F1, F3 and F4 all sit precisely in these two gaps, which is plausibly why a green suite (2821 passed) missed them.
Suggestion: add (1) a branch-level unit test for resolve_message_display, (2) replay tests asserting legacy agent_message rows without display land in chat and that a completed task's status survives replay, and (3) app-context-chat.test.tsx cases for the rewritten routing.
Minor
F8. No explicit ignore/stream branch in the main chat router — frontend/src/contexts/app-context-chat.tsx:3019, 3054 — severity: low — Blocking: no. Such events would fall through to the terminal else (:5098-5102), which console.traces the full payload and stores it via ADD_TRACE_EVENT. Currently unreachable — create_agent_outbound_stream_event (websocket.py:1079) filters ignore/stream before broadcast, persistence and replay on both the main and Builder paths. But agent-builder-chat.tsx:286-289 already has its own explicit ignore guard, so the inconsistency is worth closing for defense-in-depth.
F9. Two of the five surface values are dead vocabulary — src/xagent/core/agent/message_display.py:7 — severity: low — Blocking: no. No producer sets display="status" or emits event_type="chat_message"; "status" is reachable only via an off-schema LLM value (F7) and gets no rendering distinct from "timeline" in TraceEventRenderer.tsx. Shipping chat|timeline|ignore and adding the rest when a producer exists would be simpler. YAGNI observation, not a defect.
F10. Waiting-prompt back-scan widened in the task panel — frontend/src/components/task/task-conversation-panel.tsx:138, 171 — severity: low — Blocking: no. Reusing expectsUserResponse widened the back-scan to match message_type === "question" regardless of expect_response (same root cause as F1). Bounded by the outer currentTask.status === "waiting_for_user" gate, so the worst case is picking the wrong prompt text among candidates while the task is legitimately waiting — it cannot spuriously trigger the waiting UI. (TraceEventRenderer.tsx already had this broader match pre-PR and is unaffected.) No test covers the "later non-waiting question after the real waiting question" ordering.
F11. Silent fallback on a malformed display value — src/xagent/core/agent/message_display.py:37, frontend/src/lib/message-surface.ts — severity: low — Blocking: no. A dict/list/typo/wrong-case display silently falls back to "chat" with no log on either side (confirmed by test_runtime.py:572-584, test_agent_checkpoint_stream.py:297-302), and there's no case normalization — a "Timeline" typo would misroute permanently and undetectably. Add a logger.warning / console.warn on the fallback path.
F12. Display resolution computed twice per event — src/xagent/web/api/websocket.py (create_agent_outbound_stream_event, then _agent_outbound_event_type recomputes), frontend/src/contexts/app-context-chat.tsx:3019, 3054 — severity: low — Blocking: no. Inputs are identical today so there's no divergence risk, but resolving once and threading the value through is cheaper and removes the invitation to drift.
Investigated and cleared
Two items raised across the earlier review passes did not hold up and are not findings:
- "Delegated child timeline messages leak into the parent's execution timeline" — the guard-reordering observation is accurate, but parent/child separation is enforced downstream by
data.source === "xagent-agent-tool-child"filtering intask-conversation-panel.tsx, untouched by this PR. No leak occurs. (A one-line test-coverage nit stands: no test coversdisplay: "timeline"on a delegated child event.) - "30-second content dedup now applies to more messages than before" — the premise fails: ordinary
agent_messageevents were already subject to that dedup pre-PR regardless ofmessage_type, and this PR exposes no new message class to it.
Simplification opportunities
L7 (src/xagent/core/agent/message_display.py): stdlib: MESSAGE_DISPLAYS re-lists the five MessageDisplay literals by hand. Replace with frozenset(typing.get_args(MessageDisplay)).
L1126 (src/xagent/web/api/websocket.py): delete: hand-copies the final-answer event-type set already defined as FINAL_ANSWER_EVENT_TYPES in message_display.py:8-15. Import and reuse FINAL_ANSWER_EVENT_TYPES instead of re-listing the 4 strings.
L19 (frontend/src/lib/streaming-final-answer.ts): shrink: FinalAnswerStreamEventType is a second independent literal union duplicating message-surface.ts:20-25's FINAL_ANSWER_EVENT_TYPES. Derive it from the same shared array instead of re-listing the 4 strings.
net: ~10-15 lines possible
Blocking status & recommended decision
Blocking: yes
Recommended event: REQUEST_CHANGES
| file:line | severity | impact | source |
|---|---|---|---|
frontend/src/lib/message-surface.ts:36-47 (with app-context-chat.tsx:3077-3087) |
high | replaying a stray message_type="question" row flips a completed task's UI status back to waiting_for_user |
[new] |
frontend/src/components/build/agent-builder-chat.tsx:345, 362-372 |
high | reverse-order arrival overwrites and silently loses the finalized final answer in Builder chat | [prior] |
src/xagent/web/api/websocket.py:7899-7906, 8025-8031 |
high | content-key collision silently drops a durable final-answer chat row on reload | [new] |
src/xagent/web/api/websocket.py:10166-10171 |
high | Builder outbound path silently drops all final_answer_* stream payloads — content vanishes with no error |
[new] |
F5, F6, F7 and F13 are strong pre-merge should-fixes but do not independently block.
|
|
||
| if expect_response or message_type == "question": | ||
| return "chat" | ||
| if not visible: |
There was a problem hiding this comment.
Re: the earlier "Honor visibility before question routing" finding — waived by design. The precedence is real (visible=false + expect_response/message_type="question" still resolves to "chat"), but it is explicitly documented in this function's docstring ("Visibility and response waiting are deliberately separate concerns. A response-bearing message is always chat-visible...") and locked in by tests/core/agent/test_runtime.py:568-583 (test_runtime_send_message_resolves_display_independently_from_waiting, asserting hidden_question["display"] == "chat"). The old unconditional visible is False early-return suppressed even response-required questions, leaving the UI unable to solicit a reply the backend was blocking on, so the reversal is intentional. No change requested.
One narrower sub-case is undocumented and untested though: message_type="question", expect_response=False, visible=False also becomes chat-visible, and the docstring rationale ("response-bearing") doesn't cover it since nothing is waiting on a response. Worth either documenting why, or gating this branch on expect_response.
| ): boolean => { | ||
| const messageData = | ||
| data && typeof data === "object" ? (data as MessageSurfaceData) : undefined; | ||
| return ( |
There was a problem hiding this comment.
Blocking (high). expectsUserResponse now returns true whenever message_type === "question", independent of expect_response. app-context-chat.tsx:3077-3087 dispatches UPDATE_TASK_STATUS -> waiting_for_user off this, and historical trace-event replay goes through the same handleMessage path as live messages (live handler at app-context-chat.tsx:2378, replay scheduler at :6600, both passing data verbatim), with raw TraceEvent rows replayed with original field values intact (src/xagent/web/api/websocket.py:7908-7947). So replaying an old event_type=agent_message, message_type=question, expect_response=false row flips an already-COMPLETED task's UI back to "waiting for user". Note the chat-history replay path (websocket.py:8025-8051) hardcodes expect_response: False on reconstructed rows precisely to prevent this; trace replay has no such override, and the only post-replay status correction (websocket.py:8158-8165) fires solely for tasks already PAUSED/WAITING_FOR_USER.
The row is reachable: the ReAct send_message schema (src/xagent/core/agent/pattern/react/react.py:2131-2154) exposes message_type and expect_response as independent LLM-controlled arguments with no validation tying them together, and only expect_response gates actual suspension (react.py:2356). Pre-PR this helper was gated on expect_response === true alone, so this is newly introduced, and no test covers it.
Suggestion: require expect_response === true (or at least message_type === "question" && expect_response !== false), and add a replay test asserting a completed task's status survives replay of an old question row.
| eventType === "agent_message" && | ||
| (messageData?.expect_response === true || | ||
| messageData?.message_type === "question") | ||
| ); |
There was a problem hiding this comment.
Should fix (medium, non-blocking). This helper and resolve_message_display in src/xagent/core/agent/message_display.py are two independent hand-written implementations of a contract that's meant to be shared/centralized, and they have already drifted in three confirmed ways:
- Python's
expect_response/question short-circuit has noevent_typegate (message_display.py:33-34) while this one requireseventType === "agent_message"— soevent_type="ai_message", display="timeline", expect_response=Trueresolves to"chat"in Python but"timeline"here. - Python uses
display or metadata_display(falls through on"") while this usesdisplay ?? metadata.display(nullish only) —display: ""with a metadata fallback resolves differently. user_messageis in both languages' chat-routing lists but missing fromisMessageDisplayEventType's gate;chat_messageis listed in both routing tables with no producer anywhere.
None of the three is triggered by a currently active producer, so these are latent rather than live bugs — but they're exactly the drift a "centralized contract" is supposed to eliminate. Suggestion: single-source the table (generate one side, or add a shared fixture of (event_type, data) -> surface cases asserted by both the Python and TS suites) so future drift fails CI.
| } | ||
| } else if (data.event_type === "agent_message") { | ||
| const displayReply = data.data?.message || "" | ||
| } else if (isMessageDisplayEventType(data.event_type) && messageSurface === "chat") { |
There was a problem hiding this comment.
Blocking (high) — carried over from the earlier "Preserve non-waiting Builder messages" finding: PARTIAL, not fully fixed.
Commit 036ca93 fixed the forward-order case — lines 362-382 now push a fresh placeholder bubble after a non-waiting agent_message, covered by agent-builder-chat.test.tsx:258-298. But the symmetric reverse order is still broken: the ai_message branch above (overwrite at line 345, updated[updated.length - 1].content = displayReply) mutates the last message in place with no push, so if a final ai_message lands first and a non-waiting agent_message arrives after it, this branch unconditionally overwrites updated[updated.length - 1] again and silently destroys the finalized final answer's content. Only the forward ordering is tested.
Secondary, lower-severity artifact of the same fix: when a turn ends via task_completed with genuinely empty final content, the pushed empty placeholder gets filled with the canned filler string and shows as a spurious extra bubble.
Suggestion: mark the bubble finalized when ai_message/task_completed writes it, and have this branch push a new bubble rather than overwrite a finalized one. Please add a reverse-ordering regression test, and skip the filler placeholder when final content is empty.
| ) | ||
| ) | ||
| ) | ||
| event = create_agent_outbound_stream_event(builder_task_id, payload) |
There was a problem hiding this comment.
Blocking (high) — regression from "delivered but mislabeled" to "silently lost". send_builder_outbound_message calls create_agent_outbound_stream_event directly, with none of the final-answer interception that the main live-delivery path in make_agent_outbound_handler (:1126-1148) performs for final_answer_start/delta/end/error (including the _reconcile_streamed_final_answer repair step). That factory returns None here — event dropped, nothing sent — for display == "stream", which is exactly what resolve_message_display returns for any final_answer_* event type (src/xagent/core/agent/message_display.py:39-40).
The path is reachable: Builder wires this handler on AgentService running the normal ReAct pattern (default execution_mode="balanced" maps to ReAct), and ReAct unconditionally instantiates ReActFinalAnswerStreamer, streaming final answers through the same runtime path as regular tasks — there is no builder-specific disable flag. At the base commit this function had no final-answer branch and sent the event unconditionally: wrong type (agent_message), but delivered. Now the content vanishes from the Builder chat UI with no error and no fallback.
Suggestion: extract the final-answer interception from make_agent_outbound_handler into a shared helper and use it here (or route Builder through the same handler), plus a Builder-path test asserting a streamed final answer is delivered.
(Note: the content-key replay dedup issue, F3 in the review body, and this PR's default-display-routing change (F5) also feed into message-loss risk elsewhere in websocket.py, e.g. around trace_message_keys at lines 7899-7906/8025-8031 — that code is unchanged by this PR so there's no diff line to anchor a comment there, see the review body.)
| "visible": {"type": "boolean"}, | ||
| "display": { | ||
| "type": "string", | ||
| "enum": ["chat", "timeline"], |
There was a problem hiding this comment.
Should fix (medium, non-blocking). Two gaps around this new display field.
- No
description. The enum["chat","timeline"]is advertised with no explanation of what either surface means, and there is no system-prompt text anywhere insrc/xagentguiding the model on chat vs. timeline. That matters because this PR also flips the default: pre-PR any non-questionagent_message— including the defaultmessage_type="info"— resolved toagent_progress(timeline-only); post-PR none ofresolve_message_display's earlier branches match a plain info message, so the fallback returns"chat". Every existing agent's routine narration silently migrates into the main chat transcript, with nothing telling the model how to opt back out. Suggestion: add adescriptionat minimum, and consider keeping ordinaryinfomessages ontimelineunless the model explicitly requestschat. - The enum isn't enforced. Line 2340ish passes the argument through as
str(display) if display is not None else Nonewith no allow-list check, whileresolve_message_displayaccepts all five internal surfaces including"stream"/"ignore". A model emitting off-schemadisplay: "ignore"has its message dropped bycreate_agent_outbound_stream_event(src/xagent/web/api/websocket.py:1073-1080) — no broadcast, no persistence — yet the tool-call recorder still reportsstatus="completed"/"sent"back into the LLM context (:2347-2355, 2388-2392) with no delivery check. The model believes it sent a message that never left. Suggestion: validate against the advertised enum, and/or surface actual delivery status in the tool result.
|
|
||
| // Agent-to-user messages, including ask_user_question prompts. | ||
| else if (eventType === "agent_message" || eventType === "ai_message") { | ||
| else if (isMessageDisplayEventType(eventType) && getMessageSurface(eventType, eventData) === "chat") { |
There was a problem hiding this comment.
Minor / defense-in-depth (non-blocking). This else-if chain only branches on getMessageSurface for timeline/status (:3019) and chat (here) — there's no explicit ignore/stream branch, so such an event would fall through to the terminal else at :5098-5102, which console.traces the full payload and stores it via ADD_TRACE_EVENT. Not a live leak today: the backend choke point create_agent_outbound_stream_event (src/xagent/web/api/websocket.py:1079) filters display in {"ignore","stream"} and returns None before broadcast, persistence, or replay, on both the main and Builder paths. But agent-builder-chat.tsx:286-289 already carries its own explicit ignore guard, so the two consumers are inconsistent, and any future outbound path (or a bug in resolve_message_display) would start dumping full message JSON to the browser console with no frontend safety net. Worth adding the same guard here.
Also minor: getMessageSurface is resolved twice per event in this chain (:3019 and here) — same inputs so no divergence risk today, but resolving once and reusing the value is cheaper and removes the invitation to drift.
|
|
||
| MessageDisplay = Literal["chat", "timeline", "status", "stream", "ignore"] | ||
|
|
||
| MESSAGE_DISPLAYS = frozenset({"chat", "timeline", "status", "stream", "ignore"}) |
There was a problem hiding this comment.
Two minor notes on this module (neither blocking).
Dead vocabulary. Of the five surfaces, "status" and the chat_message event type have no producer anywhere: nothing sets display="status" or emits event_type="chat_message" as a live outbound event, the only route to "status" is an off-schema LLM value, and TraceEventRenderer.tsx folds it into the same generic progress block as "timeline" anyway. Shipping chat|timeline|ignore now and adding the rest when a real producer exists would be simpler. YAGNI observation, not a defect.
Simplification. MESSAGE_DISPLAYS re-lists the five MessageDisplay literals by hand — frozenset(typing.get_args(MessageDisplay)) keeps them in sync automatically. Relatedly, src/xagent/web/api/websocket.py:1126 hand-copies the final-answer event-type set already defined here as FINAL_ANSWER_EVENT_TYPES (:8-15) and should import it, and frontend/src/lib/streaming-final-answer.ts:19 declares a third independent literal union duplicating message-surface.ts:20-25.
| return "chat" | ||
| if not visible: | ||
| return "ignore" | ||
| if isinstance(display, str) and display in MESSAGE_DISPLAYS: |
There was a problem hiding this comment.
Minor (non-blocking). A malformed display value silently falls back to "chat" with no diagnostic on either the Python or TypeScript side, and there's no case normalization — confirmed by the existing tests, where a dict/list display falls back with zero log trace (tests/core/agent/test_runtime.py:572-584, tests/web/test_agent_checkpoint_stream.py:297-302). A producer typo like "Timeline" would misroute permanently with no way to notice. Suggestion: logger.warning here (and console.warn in the TS equivalent) on the fallback path, and consider lowercasing before the membership check.
| const event = traceEvents[i] | ||
| if (event.event_type === "agent_message") { | ||
| const expectsResponse = event.data?.expect_response === true | ||
| const expectsResponse = expectsUserResponse(event.event_type || "", event.data) |
There was a problem hiding this comment.
Minor (non-blocking). Switching this backward "find the waiting prompt" scan to the shared expectsUserResponse helper widened it to match message_type === "question" regardless of expect_response — the same widening that drives the blocking finding on frontend/src/lib/message-surface.ts. Here the consequence is display-only and bounded by the outer currentTask.status === "waiting_for_user" gate, so the worst case is showing the wrong prompt text among several candidates while the task is legitimately waiting; it cannot spuriously trigger the waiting UI. (TraceEventRenderer.tsx already had this broader match pre-PR, so only this file newly picked it up.) No test covers the "a later non-waiting question follows the real waiting question" ordering — worth one. If the message-surface.ts helper is tightened to require expect_response, this resolves along with it.
036ca93 to
15b0755
Compare
|
Addressed the full review on top of the latest
Local verification:
@codex review |
|
@rogercloud The two refreshed workflow runs are currently |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15b0755207
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let index = findLastAssistantIndex( | ||
| updated, | ||
| message => message.isPending === true, | ||
| ) |
There was a problem hiding this comment.
When a Builder ReAct completion emits final_answer_start/delta/end, AgentTracingLifecycle.on_run_end subsequently emits an ai_message carrying the same stream_message_id. The stream end has already marked its bubble non-pending, so this pending-only lookup creates a second assistant bubble for the ai_message; the following task_completed updates that duplicate and leaves both copies visible. Match ai_message.data.stream_message_id to the streamed bubble, as the main chat path does, instead of appending another final answer.
Useful? React with 👍 / 👎.
| if (data.type === "final_answer_error") { | ||
| setIsLoading(false) |
There was a problem hiding this comment.
A final_answer_error is not necessarily terminal: ReAct emits it while automatically retrying invalid or unavailable final-answer tool protocols. Clearing isLoading here lets the user submit another Builder turn during that retry, and the Builder websocket endpoint cancels the existing active_chat_task whenever a new payload arrives, aborting the recovery. Keep the input locked until the terminal task_completed/task error event rather than treating every stream-session error as run completion.
Useful? React with 👍 / 👎.
3 participants
Summary
chat | timeline | status | stream | ignoredisplay contract at the runtime boundaryexpect_response, while preserving legacymessage_type=questionwaiting behaviorValidation
518 passed: backend runtime, Auto, DAG, ReAct, WebSocket delivery/persistence/replay tests2821 passed, 1 failed: full frontend suite; the single pre-existing failure is the unrelated missing Salesforce entry inPROVIDER_DISPLAY_NAMES, and neither affected file is changed by this PRnpm run type-checknpm run build(production build and static export completed; existing ESLint CLI option warning remains non-fatal)git diff --check/loginredirects to/setup,/setup-admincreates the first administrator, and/loginreturns access and refresh tokensCloses #554