fix(web): surface connector runtime failures to the chat client by AlexLiu190625 · Pull Request #1919 · xorbitsai/xagent

AlexLiu190625 · 2026-08-28T18:13:06Z

Summary

classify ConnectorRuntimeError at terminal settlement instead of folding it
into the opaque task-failure fallback
project the failure onto a wire-safe code through connector_runtime_client_code,
checked at the frame builder against a closed set of eight connector-runtime
error codes
write code onto the task_error frame only when it passes that check,
leaving the four existing call sites byte-identical, and drop rather than
raise on an unrecognized value so a terminal frame is never lost
log the code, the reason and the connector identity from the raw exception,
on the server only -- neither reaches the frame
render the terminal error bubble in the chat client instead of filtering it
into an "Unknown error" placeholder, and flag it as the turn's result only
for the terminal task_error type
route the frame's code through the existing client error-code table
(translation key plus English fallback) instead of a second vocabulary, so
a code has one wording on every transport
dedup terminal failures on the (run_id, state_version) pair the frame
already carries, so two failed turns each keep their own bubble while one
settlement delivered twice still collapses

Problem

When a connector needs a runtime value the task does not have, the turn fails
with a ConnectorRuntimeError. That exception already carries a curated
public-safe sentence and a stable code naming which failure occurred. The
sentence reached nobody, and the code reached nowhere at all. This PR
delivers both to the client; the reason and the connector's identity go to
the operator log, not to the client.

The terminal settlement in task_orchestrator classified failures two ways:
RequiredMCPUnavailableError passed its own message through, everything else
became the fixed "Task execution failed.". ConnectorRuntimeError subclasses
RuntimeError, so it took the second path. The frame then reached the chat
client without an isResult flag, and the conversation panel renders only user,
isResult and system-notice messages -- so the bubble was filtered out
completely and the turn degraded to a virtual "Unknown error" placeholder that
persisted until the page was reloaded.

Two separate things had to be true for the user to see nothing useful, and both
are fixed here.

Operators had the same gap from the other side: the existing failure log prints
the exception's __str__, which is "missing_runtime_context: Required connector runtime context is missing." -- the reason naming the missing key
was never written down anywhere.

Approach

Two single-purpose projections. connector_runtime_client_message adapts
the exception's safe_message; connector_runtime_client_code projects it
onto its code attribute. Both have an isinstance gate and fall back to the
existing opaque behaviour for anything else, so neither becomes a generic
exception-to-text escape.

A third branch at the terminal settlement. Inserted between the two existing
ones; neither of them changes. It settles with the exception's string, marks the
history row client-safe, broadcasts the curated sentence, and passes the
projected code to the frame builder. The reason and the connector identity
are read straight off the raw exception, separately, and go only into a
structured operator log line -- neither reaches the frame.

One optional field on the task_error frame, written only when it
survives validation, so the four call sites that pass no code produce a
byte-identical frame. code must be a member of
CONNECTOR_RUNTIME_CLIENT_ERROR_CODES -- the eight connector-runtime codes
this repository actually raises as a ConnectorRuntimeError, not the full
~30-member /v1 error surface. A value failing that check is dropped and
logged rather than raised -- the reasoning is in Disclosure 1.

One structured log record naming the code, the reason and the connector.

The frontend flags the terminal task_error frame -- and only that type --
as the turn's result. Every frame of this type is broadcast after the row
has already committed FAILED: it comes from the settlement branch, and from
the legacy only_if_running=True helper, which does not broadcast at all when
the update it is trying to apply matches no row. The root error type is a
mixed channel -- live rejections such as a busy task or a refused resume also
arrive on it, and flagging those as a result would end the turn's live
indicator on a task that is still running. The frame's code, not the relayed
sentence, decides the bubble's wording: codes in the closed set that have a
live producer today are listed in the client error-code table this repository
already uses for the root error channel's error_code field, each with a
translation key and an English fallback. A code the table does not list --
whether it is outside the closed set entirely, or inside it but without a
producer yet -- keeps the generic prefixed wording. None of the wording names
a connector or a field.

Deduplication had to move with it. The 30-second dedup keys on the server
sentence, and one sentence covers a whole code -- so two turns failing under
the same code shared a key while being two distinct failures, and that
bubble is now the turn's result. Keying on the failure's class instead of on
the occurrence cannot tell them apart, whichever class you pick: the
sentence or the code. What identifies the occurrence is already on the frame.
broadcast_to_task stamps every frame of this type with the row's run_id
and state_version (task_error is in _VERSIONED_TASK_EVENT_TYPES), and
state_version is bumped by each control transition that changes (status, control_state) -- a retry takes the lease FAILED -> RUNNING and settles
RUNNING -> FAILED, so the second failure is at least two versions on, while
one settlement broadcast twice carries one version. The identity is
therefore run_id:state_version, read from the envelope the handler already
parses before the switch. No wire field is added, no backend line changes.
A frame that arrives with no version gets no identity and keys on the text
alone, which is the behaviour that predates this change: the version gate at
the top of the handler drops such a frame once any versioned event has been
seen for the task, and when the task has no versioned event on record either,
two such frames key on the same text and the second still collapses. The
identity is withheld there rather than guessed -- attaching the state tuple
needs the row, and a settled FAILED task has one.

Disclosures

1. The error frame carries no connector identity and no reason -- only a code.

code is the frame's only structured field. This projection is broadcast
through broadcast_to_task, whose audience -- as the client-safe guard's own
source comment states -- is every connection under the task id, anonymous
widget and share-link visitors included.

There is no connector_ref field, and no reason field at all. A client that
needs the connector's identity calls the per-task requirements endpoint,
which admits the task's owner -- the user who created the task. The frame's
code is the whole story everyone gets from this frame, owner included; key
names arrive with the requirements endpoints in the next PR of this series,
which discloses their full audience.

The closed set excludes mcp_oauth_authorization_failed and
delegated_authorization_failed: nothing raises either as a
ConnectorRuntimeError today, and each one states the outcome of an
authorization check, which this frame must never carry.

Withholding the reason and the connector identity costs nothing
operationally: the operator log reads the raw exception's details
directly, so both are still recorded server-side in full; what is lost is
one degree of wording precision in the failure bubble.

2. The closed set is enforced at the frame builder; a static test pins where
code is allowed to come from.

create_terminal_task_error_event's own runtime gate only checks the value
of its code argument -- membership in CONNECTOR_RUNTIME_CLIENT_ERROR_CODES
-- and has no way to tell a curated projection from an incidental string that
happens to collide with a real code today. tests/web/api/test_terminal_task_error_frame_origins.py
closes that gap statically: every call site under web/ that passes code=
must bind it, in the same function, from a direct call to
connector_runtime_client_code -- the one projector this repository trusts
for this purpose. Today that is exactly one call site,
task_orchestrator.py's _runner.

3. This is the second client-visible projection of ConnectorRuntimeError, and
the two deliberately differ.

_raise_v1_connector_runtime_error in web/api/v1/tasks.py already projects
this exception for the SDK surface, and it makes different choices on both
counts: it maps code through V1ErrorCode, falling back to
INVALID_RUNTIME_CONTEXT for anything unrecognized rather than dropping it,
and it ships to_public_error()["details"] whole, connector_ref included.
This frame checks its code against the connector-runtime closed set and drops
anything outside it, and carries no details at all.

The split is the audience, and it is the only thing that justifies it. /v1 is
reached with an API key by an authenticated SDK developer who is already
authorized for the task; shipping the connector identity to them is that
surface's existing contract. This frame goes through broadcast_to_task, which
reaches every connection under the task id -- anonymous widget visitors and
share-link visitors included. The same fact drives every other choice in this PR.

/v1 is left exactly as it is. Nothing here changes its payload, and the two
are not merged behind one projector taking an audience parameter: a single
function whose output width depends on a caller-supplied flag is the shape
that fails open the first time someone passes the wrong flag. Two
projectors, each with one audience and no branch, is the safer factoring.
connector_runtime_client_code's docstring names the sibling so the next
reader finds it.

4. The live bubble and the replayed transcript line are worded differently,
and that gap is fully accounted for.

The live bubble is localized into the visitor's language and carries the
client's own error prefix. The replayed line, read back after a page reload,
is the fixed English sentence this server writes into history. Neither one
names a field, a connector, or states anything the other side lacks -- the
frame carries no field name to leak in the first place.

Literal equality between the two is not achievable in this repository for any
error: the prefix and the localization both happen on the client, so making
the two sentences byte-identical would mean moving wording and translation to
the server -- and that same safe_message string is also the message field
/v1 returns to SDK callers, so changing it has a second consumer outside
this PR. What is pinned instead is that the two sides carry the same
information: the frame's code maps to exactly the un-interpolated
translation key under the variable-aware test mock, the same content a reader
gets from either side.

A reload is the one case where the wording is not recovered. The terminal
failure reaches the client on the live broadcast only: the historical replay
builder emits task_info, trace events and historical_data_complete, and
adds a status event only for a task in PAUSED or WAITING_FOR_USER --
FAILED is not in that set, so no task_error is replayed. After a reload
the visitor learns the task failed from task_info.status, and the panel's
virtual placeholder bubble stands in for the wording. That asymmetry
predates this PR and is not changed here.

Notes for reviewers

The operator log record in task_orchestrator reads details off the raw
exception and guards it with an isinstance check before reading it.
ConnectorRuntimeError.__init__ runs dict(details or {}), so the attribute
is a dict at construction, but it is a plain public attribute that anything
can reassign afterwards, so the log line verifies the shape rather than
assuming it.

The projected code has exactly one reader on the client: the bubble's
wording. The dedup identity does not read it -- it reads the frame's
state tuple -- and nothing else stores or forwards it.

The two ERROR logs for one failure answer different questions. The new
structured record carries the code, reason and connector as parseable fields; the
pre-existing one carries the traceback. The pre-existing line is left untouched.

The dedup identity is one expression. errorOccurrenceIdentity and the
argument that threads it into isDuplicateMessageForViewedTask both predate
this PR; what this PR settles is which value goes in. It reads run_id and
state_version off the envelope the handler already parses, so there is no
new mechanism to revert -- only that expression, its comment, and the tests
that pin it.

Out of scope

The agent builder's chat WebSocket (components/build/agent-builder-chat.tsx)
is the other task_error consumer in the frontend. It reads data.message /
data.error and raises a toast; the new field is inert there, so it is
unchanged and needs no follow-up.

The Telegram, Feishu and Slack channels are unaffected. They do not go through
the terminal settlement's classification: they run the agent themselves and
project the outcome through project_execution_result_for_channel, which has its
own hardcoded failure branch. Their users still see "Task execution failed.",
unchanged. Improving that is owned by a different function and is not part of
this change.

Narrowing the terminal-result flag to the task_error type also withdraws a
side effect the frame carried before this PR. Two exception paths in
websocket.py's execute-task handler (the two send_personal_message arms in its exception handling) send the task's initiator a
personal type: "error" frame; before this narrowing those were flagged as
the turn's result along with everything else on the shared handler, and now
they are not. The task-level broadcast for that same failure goes out as
agent_error, which was never flagged either way, so the panel falls back to
its virtual placeholder bubble -- this is the existing behavior on main,
and this PR does not change it.

The resume-background path is unchanged. Two of its arms broadcast a
terminal frame without a code (client_safe_error_message only projects a
ClientVisibleError, and ConnectorRuntimeError is a plain RuntimeError
there), and the arm that broadcasts without settling can emit two failures
at one state version, in which case the second still collapses -- the same
outcome that path has on main. Extending the classification to it is a
separate change; nothing here regresses it.

getWebSocketErrorMessage's rule is untouched: on a transport that marks
legacy prose untrusted it still refuses to render any server sentence.
Nothing in this PR relays that prose to those visitors; the wording they get
is the client's own, selected by code.

Verification

Backend, on this branch:

$ uv run pytest tests/web/api tests/web/services/test_client_error_messages.py \
                 tests/web/services/test_task_orchestrator.py -q
2848 passed, 17 skipped

The 17 skips are postgres-only tests that do not run against this branch's
sqlite test database.

Frontend:

$ npx vitest run src/contexts/app-context-chat.test.tsx src/lib/client-errors.test.ts
193 passed

$ npx tsc --noEmit

Clean, no errors.

gemini-code-assist · 2026-08-28T18:13:10Z

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

AlexLiu190625 · 2026-08-31T02:55:44Z

/gemini review

gemini-code-assist · 2026-08-31T02:55:49Z

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

A ConnectorRuntimeError carries a curated, public-safe message and a details payload, but nothing today turns either into something a chat client can read. Add the two functions that do, plus the type that owns what is allowed onto the wire. connector_runtime_client_message adapts the exception's safe_message and falls back to the fixed task-failure text for anything else, so the boundary stays fail-closed even if a future caller passes an incidental exception despite the function's specific name. connector_runtime_public_error projects the exception onto (code, PublicErrorDetails). PublicErrorDetails holds a single field and normalizes it in __post_init__: a reason that is neither a listed enum value nor "<listed prefix>.<declared key name>" becomes None. Putting the whitelist in the constructor rather than in the projector makes "constructing this type" and "passing the whitelist" the same act, so a direct construction from another module cannot carry free text. It nulls rather than raises because every construction site is on the reporting path of an already-failed task. The type has no connector_ref field. The sink for this projection is broadcast_to_task, whose audience includes anonymous widget and share-link visitors, and the same judgement keeps two runtime reasons off the whitelist: runtime_task_identity_mismatch and runtime_owner_mismatch state the task's ownership and the outcome of an authorization check. The four 503 reasons that are listed only state that a server-side component is unavailable. The question that decides this is written into the class docstring so a later addition has to answer it too. Tests derive the reason surface by AST-scanning src/ for every site that constructs a ConnectorRuntimeError -- both construction forms -- rather than from a list of modules, and assert the whitelist neither misses a raise site nor grows an entry nothing produces.

create_terminal_task_error_event gains two keyword-only parameters. They are written into the frame only when both are supplied, so the four call sites that pass neither produce a byte-identical frame. The details parameter is annotated PublicErrorDetails, which makes mypy refuse a dict at every call site it can see. That is not the whole door: annotations are not enforced at run time, and a caller routing through Any -- a dict decoded from JSON, a **kwargs splat -- type-checks clean and would only fail deep inside the function on a missing to_wire. The first statement in the body names the contract instead. That check reads `type(details) is not PublicErrorDetails` rather than isinstance. A frozen dataclass can be subclassed, and a subclass that overrides to_wire without reading self.reason satisfies both isinstance and mypy while bypassing the whitelist that lives in __post_init__. Only the class itself carries that guarantee. The client-safe AST guard is untouched: the new parameters are keyword-only so the message argument does not move, the new fields are not in the guard's sensitive-field set, and no producer or error-payload sink is added. Both exact baselines still hold at 30 and 52.

A ConnectorRuntimeError subclasses RuntimeError and is not a RequiredMCPUnavailableError, so the terminal settlement's two-way classification sent it down the else branch and every such failure reached the user as "Task execution failed." -- which the chat client then rendered as an unknown error. The exception's own docstring already commits its message and details to being safe for API callers. Add a third branch between the two existing ones; neither of them changes. It settles with the exception's string, marks the history row client-safe, and broadcasts the curated sentence along with the projected code and details. It also logs one structured record naming the code, the reason and the connector. That log reads the raw exc.details rather than the projection: its audience is operators, the connector identity is what makes the record actionable, and it never leaves the server. The broadcast frame carries neither the connector identity nor any reason the whitelist dropped. The log is unconditional -- an observability record that can be switched off is one that is not there when it is needed -- and all three values are short bounded strings.

The error and task_error branches dispatched an assistant message without isResult. The conversation panel renders only user, isResult and system-notice messages, so the bubble was filtered out entirely and the turn fell back to a virtual "unknown error" placeholder until the page reloaded -- the server could say whatever it liked and none of it was shown. The task_completed branch already fixed this the same way; copy that precedent, comment included. Read the frame's code and details. When the code is one of the three that mean a connector is missing a value the user can supply, replace the relayed sentence with wording that names the key, parsed off the reason's last segment. A reason that names no key -- a bare enum value, or one the server whitelist dropped -- gets the keyless wording, and connector_runtime_unavailable keeps the plain failure wording because it reports a component being down, which the user cannot act on. Both are i18n keys with en and zh entries, not hardcoded English. The pair is also kept in a new lastConnectorRuntimeError state field. It holds only what the frame is allowed to carry: the frame has no connector identity, deliberately, because its audience includes anonymous widget and share-link visitors. Anything more specific has to come from the owner-only per-task requirements endpoint.

Two follow-ups on the reason whitelist, both narrowing it. The whitelist no longer lists payload_too_large or encryption_unavailable. Both belong to the runtime value-fill endpoint, which does not exist yet; they were listed ahead of it because the whitelist is the contract that endpoint will be written against. That required an exemption in the test asserting no listed reason is unproducible, and an exemption on that assertion is an allowance with no expiry date -- by the time the raising code lands, the audience judgement behind the entry has to be reconstructed from scratch. Each string now arrives with the site that raises it, and the assertion holds with no exemptions at all. The key half of a prefixed reason is now matched against RUNTIME_SOURCE_KEY_RE, the grammar the connector runtime already exports, rather than a second copy of the same pattern compiled here. The module already imports from that file, so there is no new coupling, and the two definitions can no longer drift apart.

The module docstring described it as fixed fallback strings, which was accurate when the file held two constants and one adapter. It now also owns PublicErrorDetails and the reason allowlist governing what may ride on a task_error frame, so say so.

Three changes to the terminal frame, all in the same direction: this frame is what stands between a failed task and a silent one, so nothing about it should be able to cost the frame itself. The details type check no longer raises. PublicErrorDetails already nulls an unlisted reason rather than raising, and its docstring gives the reason -- every construction site is on the reporting path of an already-failed task. The frame builder sits on that same path, and the one call site passing these arguments evaluates them inside the `except Exception` that only logs "its terminal broadcast failed". A TypeError there meant the task committed FAILED and the client saw nothing at all, which is the exact failure this branch exists to remove: the strict half degraded to a worse outcome than the bug. A rejected value is now dropped, logged at ERROR with its stack, and the frame goes out without it. `code` now passes the same closed set. ConnectorRuntimeError types it as a bare str and assigns it unvalidated, so "only the ten module constants reach here" describes today's raise sites rather than anything the code enforces -- one door locked and the one beside it open. V1ErrorCode is the repository's existing closed set of client-visible codes and carries all ten; it is imported rather than recopied, inside the function because the v1 package's __init__ pulls in routers that import this module. Unknown values follow the same drop-and-log path. The durable half of the settlement is now pinned too. The classification branch writes three things and the tests covered one: deleting `client_history_message_type = CLIENT_SAFE_FAILURE_MESSAGE_TYPE` left every test in the file green while a reloading user dropped back to the generic failure text the live bubble no longer shows. The capture helper hands back the settlement kwargs, and both the curated and the incidental branch assert what they persist.

`_raise_v1_connector_runtime_error` (web/api/v1/tasks.py) already projects this same exception for a client, and it makes the opposite call on both halves: it maps the code through V1ErrorCode with an unknown fallback, and ships `to_public_error()["details"]` whole with `connector_ref` in it. Reading either projector alone, the other looks like a contradiction. It is not: the audiences differ. /v1 answers an API key held by a caller already authorized for the task. This path feeds `broadcast_to_task`, which reaches every connection under the task id, anonymous widget and share-link visitors included -- the fact every other choice here follows from. Say so where someone comparing the two will be standing, and say why they stay two projectors rather than one taking the audience as an argument: output width behind a caller-supplied flag fails open the first time the flag is passed wrong. Also states why the details-shape check exists at all, since __init__ normalizes that attribute: it is a plain public attribute anything can reassign afterwards, and this is the last step before the wire.

…state The dedup key was the server's own sentence, and `_message_for_code` returns one fixed string per error code that does not vary with the missing key. So two turns failing on two different keys inside the 30-second window collapsed into one bubble, and the survivor named whichever key failed first -- one value standing for two facts, the same shape this change set exists to remove, reintroduced a layer up. The comment added alongside it claimed the server message "identifies the failure", which stopped being true the moment the rendered wording began deriving from `reason` instead. `isDuplicateMessage` already takes an `occurrenceIdentity` argument for exactly this. Pass the code and reason. Two tests cover both directions: different keys keep both bubbles, a genuine repeat still collapses. Also drops `lastConnectorRuntimeError`. It was written on every terminal error frame and read by nothing -- the dialog that consumes it belongs to a later PR in this series, so it was a surface with no consumer in the PR that introduced it. The state field, its action type, its reducer case, its TASK_SCOPED_ACTION_TYPES entry and the test probe all go with it. The helper that reads the pair off the frame is renamed `getTaskErrorProjection`: it returns for any frame carrying a code, and the old name promised a connector-specific answer it never checked for. Whether a frame is connector-related is decided by the code, at the one place that asks.

AlexLiu190625 · 2026-08-31T18:12:08Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces structured terminal error frames for connector runtime failures, allowing the frontend to display specific missing configuration keys to the user instead of generic error messages. It includes robust backend validation/whitelisting of error reasons, updated frontend error handling and deduplication, and comprehensive tests. The review feedback correctly identifies a potential AttributeError in the backend logging when details is not a dictionary, and points out that the translation files in English and Chinese should use double curly braces for proper react-i18next interpolation.

rogercloud

Summary

This PR adds a connector-runtime error projection that carries a curated code/reason from task settlement to a terminal websocket frame and renders missing-runtime guidance in the React chat. It also persists a safe failure message, adds public-reason filtering, and introduces backend/frontend tests for frame shape, routing, and deduplication. The overall terminal-settlement → wire-frame → frontend-projection path is coherent, but the current implementation has three blocking correctness/security gaps and one non-blocking false-green test.

Blocking: yes — recommended event: REQUEST_CHANGES

Design verdict

Acceptable with reservations. The chokepoint choice is appropriate, but F2 (public audience boundary) and F3 (durable/replay parity) are confirmed design reservations. F1 is a separate event-classification correctness defect; F4 is minor test quality.

Findings

F1 — Nonterminal root errors are marked as final results

frontend/src/contexts/app-context-chat.tsx:5772 · major · Blocking: yes

Trigger: While a task is RUNNING or WAITING_FOR_USER, a supported root error can be emitted for chat enqueue/policy/permission/validation rejection, pause/resume enqueue or conflict failure, or active-interaction resume rejection. The shared case "error": case "task_error" handler accepts these events even though only task_error is terminal.
Impact: TaskConversationPanel treats the assistant bubble as the final result and filters it into the result-only view, hiding the live process or waiting UI and presenting a nonterminal rejection as the task's final answer.
Contract/evidence: Root error is a mixed personal/control/rejection channel; these producers do not transition the task to a terminal state. The base handler did not set isResult; this PR unconditionally adds it, and the existing test exercises both event types only with a failed task.
Fix direction: Set isResult only for an explicitly terminal marker (or the terminal task_error contract), and add coverage for nonterminal chat/pause/resume and waiting-state rejections.

F2 — Arbitrary runtime key names are disclosed to public widget/share audiences

frontend/src/contexts/app-context-chat.tsx:5739 · major · Blocking: yes

Trigger: An owner publishes a widget/share task selecting a custom API/MCP connector with a required runtime key; a missing value produces missing_context.<key>, and this branch interpolates the parsed key directly into the user-facing bubble.
Impact: The terminal frame is broadcast to every task connection, including anonymous widget/share visitors, so owner-defined names such as password, api_key, or tenant_secret disclose connector configuration metadata. No secret value needs to leak for this confidentiality violation.
Contract/evidence: Connector schema validation only enforces the [A-Za-z0-9_-] key grammar, while owners can define arbitrary keys and the schema/UI has no public-safe label or confidentiality classification. The new dynamic-prefix allowlist accepts any syntactically valid suffix; existing sensitive-value redaction does not redact key names. The base path used only the generic fallback.
Fix direction: Keep dynamic key names owner-only on public broadcasts, or require and project an explicitly audited public-safe label instead of the schema key.

F3 — Structured connector error projection is lost on durable history replay

src/xagent/web/services/task_orchestrator.py:2069 · major · Blocking: yes

Trigger: A selected connector is missing required context such as auth_token. The live branch computes a wire-safe (code, details) projection, but this call attaches those fields only to create_terminal_task_error_event; the preceding settle_task_lease_isolated call persists only client_error_message and client_message_type.
Impact: Live UI can render key-specific missing-runtime guidance, while reload/history replay reconstructs an assistant message from the durable prose fields and loses the code/reason projection, reverting to the generic failure sentence. This breaks live/replay parity and drops actionable guidance after reload.
Contract/evidence: Supported task creation stores connector references, and missing required context reaches the connector-runtime failure path. TaskChatMessage has no error metadata fields, and the unchanged historical serializer emits only message/content (plus ordinary chat metadata); the base had no equivalent structured connector-runtime branch. No migration or replay projection compensates for the omission.
Fix direction: Persist a wire-safe (code, reason) projection or the exact public-safe final content with the durable failure, and include it in historical replay; add live-versus-reload parity coverage. Do not persist only the generic safe sentence.

F4 — Interpolation test mock makes key-rendering coverage false-green

frontend/src/contexts/app-context-chat.test.tsx:5723 · minor · Blocking: no

Trigger: This test file's i18n mock returns the translation key and discards interpolation variables, while the fixture supplies missing_context.auth_token and production calls t(..., { key: missingKey }).
Impact: The assertions prove only that the translation-key branch was selected. Removing or renaming the parsed suffix, or omitting { key }, still returns the same bare key and passes, so the new key-specific rendering contract can regress unnoticed.
Contract/evidence: The real locale strings interpolate {key} and the production path passes that variable; variable-aware mocks already exist elsewhere in the test suite. The comment and bare-key assertion explicitly leave the concrete auth_token output unverified.
Fix direction: Use a variable-aware mock or the real resolver, then assert the rendered content contains auth_token (or the exact localized output).

Prior finding dispositions

Details-shape root 3897121320 (reply 3900709422; review summary 5069704805) — DROPPED. ConnectorRuntimeError.__init__ normalizes details to a built-in dict; all production construction sites pass dict literals/local dicts, and no production post-construction reassignment was found. The projector's wrong-shape check is defensive, and test-only tampering is not a supported production trigger. This is not a current finding.
Interpolation-syntax roots 3897121329 and 3897121363 (replies 3900710021 and 3900710294; review summary 5069704805) — DROPPED. The in-repo translations use single-brace interpolation, the English/Chinese locale entries are correct, and there is no react-i18next usage to reinterpret them. These prior roots are not duplicated as current findings; F4 is a separate false-green test-mock issue.

Blocking status & recommended decision

Blocking: yes — recommended event: REQUEST_CHANGES

Blocking issues:

frontend/src/contexts/app-context-chat.tsx:5772 — major — nonterminal rejection errors become final results and hide live/waiting state [new]
frontend/src/contexts/app-context-chat.tsx:5739 — major — arbitrary connector key names reach anonymous widget/share visitors [new]
src/xagent/web/services/task_orchestrator.py:2069 — major — code/reason details are live-only and disappear after history replay [new]

F4 is minor and non-blocking.

…elist A reason of shape missing_context.<key> carried the connector owner's own declared field name onto task_error, whose broadcast reaches anonymous widget and share-link visitors. Collapse _is_public_reason to a plain membership check against the fixed-string allowlist and drop the prefix set entirely -- the other five prefixes had no raise site in src/ either. missing_context.<key> is now dropped whole rather than trimmed to its prefix, so details comes back {} and code alone survives. The other two missing-value codes are unaffected: their reasons are the fixed strings not_provided/store_lost, already on the allowlist. Two tests that used to compare a PublicErrorDetails instance against another instance built from the same withheld reason went quietly blind: both sides null out under __post_init__ and the equality still holds. Rewritten to assert through to_wire() instead. Also adds an AST-derived pattern check so a re-added prefix form is caught by shape, not just by its literal value.

…ed reason The server no longer sends a reason built from the connector owner's declared field name (previous commit), so the client-side key parser has nothing left to read. Drop missingRuntimeKeyFromReason and the keyed translation string, and always use the keyless connectorRuntimeMissing wording for the three missing-value codes. Also switches this test file's i18n mock to the variable-aware form already used across 23 other test files (t returns `key:JSON(vars)` when vars are passed). The old key-only mock could not tell an interpolated call from a bare one, so a future regression that starts passing the key back into the wording would go undetected here. The two dedup tests that asserted on the now-removed keyed wording are dropped; their (code, reason) replacements land separately.

… sentence The prior comment on errorOccurrenceIdentity argued from a scenario that never occurs on this path: two different missing-value codes sharing one dedup key. Each of the three connector-runtime codes maps to its own fixed server sentence, so two different codes never share a key in the first place. The scenario the (code, reason) identity actually guards is one code failing under two different admitted reasons -- a runtime secret that was never provided, then later found lost from its store. Both share the same server sentence and therefore the same base dedup key, and without the reason folded in, the second turn's result bubble would vanish inside the existing dedup window. Replaces the two dedup tests with that scenario and its inverse (a genuine repeat of one failure still collapses to one bubble).

…sult The shared "error" / "task_error" handler flagged every bubble it produced as isResult, but the root "error" type is a mixed channel: a rejected chat message, a rejected pause, or a rejected resume all arrive on it while the viewed task is still RUNNING or WAITING_FOR_USER. Flagging one of those closes the conversation panel's live progress indicator and waiting-answer form for a turn that has not actually ended, and drains the turn's accumulated trace events into the rejection bubble. task_error has no such ambiguity: every frame of that type is emitted only after the row has been committed FAILED (task_orchestrator.py's settled branch, and websocket.py's legacy only_if_running helper, which does not broadcast when its update matches no row), and it is also the only frame carrying the structured code/details pair used by the connector-runtime wording and the dedup identity. Gate both on the frame type instead of treating every "error"/"task_error" frame alike. Replaces the parametrized isResult test (which asserted the same, now-wrong, behavior for both types) with one for task_error, and adds two for the root "error" type on a running and a waiting task.

task_orchestrator.py: the elif branch for ConnectorRuntimeError covers more codes than the three missing-value ones (invalid_runtime_context, connector_runtime_unavailable, and the *_resolution_failed codes also land here), so "the three codes" needs a referent to not read as the whole branch. app-context-chat.test.tsx: the waiting-rejection fixture combines `task` from the resume-refusal path (websocket.py:8935) with `error_code` from the pause-refusal path (websocket.py:8491). Neither path emits both today; note the synthesis so it doesn't read as one producer emitting both fields.

rogercloud

Summary

This PR fixes a real bug: terminal task_error frames for connector-runtime failures (missing secret, missing runtime context, connector unavailable) were previously invisible in the conversation panel because they weren't flagged isResult, so a failed turn appeared to end with no answer. The fix flags terminal task_error frames as isResult (mirroring the existing task_failed trace-event pattern), adds a code/details wire vocabulary with a server-side PublicErrorDetails whitelist so a curated, localized message can reach anonymous widget/share-link visitors without leaking internal exception prose, and changes message deduping to key off (code, reason) instead of the rendered sentence so two distinct failures under one error code aren't collapsed into one bubble.

The core fix is sound and narrowly scoped. However, the surrounding mechanism (a new dataclass, a 12-item reason whitelist, an AST-based self-enforcing bidirectional test, and a parallel code/details wire vocabulary — roughly 361 lines of production code and 1075 lines of tests) is disproportionate to the 3 localized bubble strings and one dedup discriminator it delivers. The codebase already has a same-file precedent for "this bubble is the turn's result, don't collapse it" — the per-request waitingRequestId identity at frontend/src/contexts/app-context-chat.tsx:5575 (base). A turn_id-based occurrence identity would likely have avoided most of the new wire-level details mechanism, since reason is only ever consumed client-side to build the dedup key and is never rendered.

More importantly, three confirmed correctness gaps mean the dedup/rendering logic this PR adds does not fully deliver on its own stated goal (never showing a turn with no result, and never showing "Unknown error" to the audience this mechanism exists for). Details below.

Round 0 — design-level findings (not tied to a single line)

Disproportionate mechanism for the value delivered. ~361 lines of production code (new PublicErrorDetails dataclass, 12-item reason whitelist, code/details wire fields, three near-identical "project this exception" helper functions) plus ~1075 lines of tests (including a full AST-based bidirectional self-enforcement test) to deliver 3 localized strings and a dedup discriminator. See the Simplification section below for the parts of the test suite that can shrink without losing the safety property.
Missed reuse of an existing pattern. waitingRequestId (frontend/src/contexts/app-context-chat.tsx:5575, base commit) already gives a per-request unique identity for "this bubble is this turn's result, do not collapse it with a previous turn's bubble." A turn_id-based occurrence identity for terminal task_error frames would generalize that existing pattern instead of introducing a new (code, reason) composite key — and would also close Blockers 1 and 2 below, since turn_id is already available at the settlement site (src/xagent/web/services/task_orchestrator.py:2015) but simply isn't threaded onto the wire frame today.
Parallel error-code vocabulary. The new code/details fields (backed by V1ErrorCode) sit alongside the pre-existing error_code/ClientErrorCode mechanism (src/xagent/web/services/client_error_messages.py:25-46, mirrored in a full translation-key + fallback table in the frontend), which was purpose-built for exactly this need. The new field instead uses a hand-rolled 3-element frontend Set mapped to one i18n string with no fallback table. Not a functional bug, but worth a follow-up to consider consolidating into ClientErrorCode.

Blocking findings (3, all severity: major)

1. Dedup silently drops the whole result bubble on a repeat connector failure within 30s

frontend/src/contexts/app-context-chat.tsx:5754-5762

The occurrence identity for connector-coded task_error frames is `${code}:${reason}`, consumed by isDuplicateMessageForViewedTask (frontend/src/contexts/app-context-chat.tsx:1987-2010) with a 30-second cache window. If the same connector failure (same code + same reason — e.g. a user retries a task while a secret is still missing) happens on two consecutive turns within 30 seconds, the second turn's ADD_MESSAGE dispatch is skipped entirely: no bubble at all, even though the task is FAILED and processing has stopped. This is exactly the "turn ends showing nothing" failure this PR sets out to fix, and it's asserted as intended by the PR's own test (frontend/src/contexts/app-context-chat.test.tsx:5999, "still collapses a repeat of the same code and reason", using two frames timestamped a second apart representing two separately-settled turns). This is a routine retry pattern, not a contrived edge case.

Recommend keying occurrence identity on turn_id (available at src/xagent/web/services/task_orchestrator.py:2015 but not currently threaded onto the wire frame) instead of (code, reason), mirroring the waitingRequestId precedent — or at minimum, never let a dedup match suppress the message dispatch entirely; always render some terminal marker per settled turn.

2. Same dedup gap also swallows a second identical generic (non-connector) failure

frontend/src/contexts/app-context-chat.tsx:5780

isResult: isTerminalErrorFrame is gated purely on message.type === "task_error", independent of whether a code was parsed, so it now applies to the untouched generic "Task execution failed." fallback path too. But errorOccurrenceIdentity is undefined when there's no connector code (line 5754-5756), so dedup falls back to pure message-text keying — unchanged from base. Two consecutive generic failures with identical text within 30s: the first is now shown (newly, thanks to this PR's isResult broadening) but the second is silently collapsed, reproducing the exact bug this PR fixes, for the generic-failure case. This is newly exposed by this PR: on base, no task_error bubble was ever shown at all regardless of dedup, so the collapse was never observable.

Same root cause as #1 — recommend fixing both together by giving every terminal task_error frame a turn_id-based occurrence identity, not just connector-coded ones.

3. "Unknown error" fallback defeats the curated safe-message mechanism for 7 of 10 target error codes on anonymous transports

frontend/src/contexts/app-context-chat.tsx:921-923 and frontend/src/contexts/app-context-chat.tsx:5736-5742

getWebSocketErrorMessage (line 921) keys its behavior on the old error_code field, not the new code field. New terminal task_error frames for connector failures never set error_code (only code — confirmed via src/xagent/web/api/websocket.py:397-411), so on an untrusted transport (trustLegacyErrorProse === false, the case for widget/share-link visitors — see frontend/src/components/widget/public-agent-chat-page.tsx:623 and frontend/src/components/widget/session-agent-chat-page.tsx:261) this function unconditionally returns "Unknown error" for every such frame.

connectorRuntimeBubble (line 5736-5740) only intercepts this for the 3 codes in CONNECTOR_RUNTIME_MISSING_VALUE_CODES (frontend/src/contexts/app-context-chat.tsx:935-939). The connector/runtime error family in V1ErrorCode (src/xagent/web/api/v1/errors.py:105-114) has 10 members (CONNECTOR_NOT_FOUND, INVALID_RUNTIME_CONTEXT, MISSING_RUNTIME_CONTEXT, RUNTIME_CONTEXT_IMMUTABLE, RUNTIME_SECRET_NOT_ALLOWED, RUNTIME_SECRET_UNAVAILABLE, SCHEDULED_SECRET_UNAVAILABLE, CONNECTOR_RUNTIME_UNAVAILABLE, MCP_OAUTH_AUTHORIZATION_FAILED, DELEGATED_AUTHORIZATION_FAILED); only 3 get the curated message, and the other 7 (including connector_runtime_unavailable, this PR's own headline example) fall through to `${errorPrefix} Unknown error` at line 5742, discarding the safe sentence the backend already computed and sent on the same frame, in favor of a useless generic string — for precisely the anonymous audience this mechanism exists to serve. The bubble is still visible (isResult is unconditional), so this is a UX/message-quality gap, not a silent-failure gap like #1/#2, but it defeats the PR's own stated purpose for most of its target code set.

Recommend extending CONNECTOR_RUNTIME_MISSING_VALUE_CODES-style handling (or a simpler "if code is present and recognized as connector-runtime, never fall back to Unknown error") to cover all 10 codes.

Non-blocking findings (grouped)

code validation is looser than details's. code not in _client_visible_error_codes() (src/xagent/web/api/websocket.py:387) is a bare frozenset membership test with no type/hashability gate, unlike details's strict type(details) is not PublicErrorDetails check (line 374). An unhashable code would raise an uncaught TypeError inside create_terminal_task_error_event — not reachable today (every call site passes None or a known string constant), and even if it fired the blast radius is the same already-tolerated "lost terminal frame + log warning," not a crash escaping further. Minor, latent.
_client_visible_error_codes() gates against the full V1ErrorCode enum (src/xagent/web/api/websocket.py:325-336, ~30 members including billing/tenant/auth codes for the authenticated SDK audience), not a connector-runtime-specific subset — asymmetric with the tightly-curated 12-item reason whitelist. No live leak today (only ~9 connector-specific constants ever reach this path), but nothing prevents a future connector-runtime raise site from passing a non-connector code straight through to anonymous broadcast recipients. Suggest a dedicated closed set for this gate.
Log line skips the dict-type guard its neighbor uses. src/xagent/web/services/task_orchestrator.py:1994-2001 calls .details.get("reason") / .get("connector_ref") directly, unlike the projector two lines above (connector_runtime_public_error, src/xagent/web/services/client_error_messages.py:251) which explicitly checks isinstance(details, dict) with a comment explaining .details is a reassignable public attribute. Not reachable today; even if triggered, only the diagnostic log line is lost. Already flagged in a prior review round — still open, not new.
The client_safe_ast_guard safety-net test wasn't extended to model the new code/details fields on the task_error builder; its hardcoded model still only produces {type, message, error}. Confirmed current baselines are producers == 29, error_payloads == 50 (tests/web/api/test_websocket_client_safe_errors.py:193,201) — the PR description's Disclosure 2 cites stale numbers (30/52), worth a one-line fix. Not a live gap: PublicErrorDetails's whitelist and the code closed-set check are separate, functioning controls, and the PR already discloses this scope decision.
Whitelist self-enforcement test has rigor gaps (tests/web/services/test_client_error_messages.py): 3 undeclared_*_key whitelist members are validated only by regex fallback, not exact raise-site matching; the AST scanner only recognizes ast.Name-style calls and literal dict details=, missing ast.Attribute calls and non-literal values; 5 reason=str(exc) call sites (e.g. src/xagent/web/services/connector_runtime.py:824,854,880) are invisible to the scanner (independently safe — the runtime _is_public_reason check still catches them). All confirmed test-rigor weaknesses with no live security impact — the actual runtime gate is separate and sound.
Persisted/replayed history uses raw English safe_message while the live bubble is localized, so a reload after the fix shows English where the live view showed the localized string. Already raised and acknowledged as a deferred follow-up in a prior round — not new, not blocking.
The resume-background path and legacy execute_task command path still get the old opaque fallback (no code/details) since ConnectorRuntimeError isn't a ClientVisibleError there. execute_task is explicitly disclosed as out of scope by the PR; the resume path is not mentioned in the disclosures, though this is pre-existing behavior, not a regression. Worth a follow-up: either extend the fix or add it to the disclosed scope.
settlement_error now stores f"{code}: {safe_message}" (src/xagent/web/services/task_orchestrator.py:1981) into task.error_message, a deliberate, test-pinned change (comment: "the durable error keeps the code prefix operators grep for"), but other readers of task.error_message beyond the two websocket paths checked (src/xagent/web/api/v1/tasks.py:836, conversation_logs.py:593, a2a_protocol.py:434-435) weren't verified against the new prefixed format. Not a regression, no secrets exposed — worth a follow-up confirmation from the author.
connector_runtime_client_message is a near-verbatim structural clone of the pre-existing required_mcp_unavailable_client_message (and a third function, connector_runtime_public_error, uses the same idiom). Confirmed deliberate per each docstring (a recurring fail-closed idiom in this file), not accidental duplication — mention only as a minor simplification-adjacent note.
isResult: true drains state.traceEvents into the message (existing reducer behavior), now also applying, untested, to the new terminal-error case. When two error bubbles both get isResult: true in one task (the same-code-different-reason scenario this PR's dedup fix is designed to preserve), the second bubble's trace ends up empty because the first already drained the accumulator. Not a crash or data loss, but a genuine trace-accounting gap worth a follow-up test.
i18n inconsistency: the English "missing value" bubble string has no call-to-action while the Chinese version adds "please supply it and retry" — misleading for anonymous widget/share-link visitors who can't act on it anyway. Minor wording nit.
Test-quality notes (all minor): test_connector_runtime_failure_persists_client_safe_history asserts on stubbed-writer kwargs rather than a persisted row; no test for non-str/unhashable code; no orchestrator test for a tampered non-dict .details reaching the log line; the manual LEGAL_REASONS-like test list covers 8 of 12 whitelist members (separately covered by the AST bidirectional test); no frontend test for code nested under message.data; no test for the traceEvents-drain interaction noted above.

Simplification opportunities

src/xagent/web/api/websocket.py:325: yagni: @lru_cache(maxsize=1) memoizing a trivial ~10-item frozenset rebuild on every call. Drop the decorator; keep the delayed V1ErrorCode import (legitimate circular-import workaround).
tests/web/services/test_client_error_messages.py:261: yagni: full-src/-tree AST walk to prove PublicErrorDetails is constructed in exactly one module — the test's own comment admits this is a review-hygiene/ownership-boundary check, not a security boundary (the real guard, type(details) is not PublicErrorDetails at src/xagent/web/api/websocket.py:374, is load-bearing and correctly blocks a demonstrated subclass-based whitelist bypass — do not remove that check). Replace the full-repo AST scan with a lighter lint/import-boundary convention, or drop it.
tests/web/services/test_client_error_messages.py:311-421: shrink: ~110 lines of AST-based constant-propagation/pattern-derivation machinery to auto-derive which reasons are raised where, cross-checked against the whitelist. The machinery has its own gaps (only resolves simple Name/Constant/IfExp bindings) that already limit its value. A direct, hardcoded raise-site list (mirroring DELIBERATELY_NOT_PUBLIC_REASONS right next to this code) would test the same two-directional invariant in far less code.

net: ~-20 lines possible

Blocking status & recommended decision

Blocking: yes — recommended event: REQUEST_CHANGES

frontend/src/contexts/app-context-chat.tsx:5754-5762 — major — a routine same-code-same-reason retry within 30s shows no result bubble at all for the second turn. [new]
frontend/src/contexts/app-context-chat.tsx:5780 — major — two consecutive generic (non-connector) failures within 30s: the second is silently collapsed, reproducing the bug this PR set out to fix. [new]
frontend/src/contexts/app-context-chat.tsx:921-923 (and :5736-5742) — major — on anonymous/widget/share-link transports, 7 of 10 connector-runtime error codes render "Unknown error" instead of the curated safe message, defeating the PR's own stated purpose for its primary audience. [new]

…nction These 85 lines derive five display values from one frame, and the four conditions each value needs are today recomputed in place with no shared decision point. Two review rounds have found defects in this handler: R1 found isResult mixed across channels and a keyed bubble; R2 found an untrusted-transport wording gap, a wrong dedup identity axis, and an unwitnessed no-version path. Turning the matrix into one function's explicit return value is the precondition for the next defect being visible instead of re-derived and missed again. Pure on purpose: no dispatch, no refs, nothing outside its arguments, so every cell of the matrix is unit-testable without rendering the provider -- the same shape extractTaskControlEnvelope above already uses. Verified equivalent, not just typed the same: a temporary shim in the test file, built by copying the case block's five expressions verbatim, pinned six cells' expected values before this extraction touched any production code (155 passed). After the extraction, the same six cells against the real function produce the same 155 passed with the same expected values. The shim is not part of this commit.

The 30-second dedup keys on the server sentence, and one sentence covers a whole code -- so two turns failing under the same code shared a key while being two distinct failures, and that bubble is now the turn's result. Keying on the failure's class instead of on the occurrence cannot tell them apart, whichever class you pick: the sentence, the code, or the code and reason together. What identifies the occurrence is already on the frame. broadcast_to_task stamps every frame of this type with the row's run_id and state_version (task_error is in _VERSIONED_TASK_EVENT_TYPES), and state_version is bumped by each control transition that changes (status, control_state) -- a retry takes the lease FAILED -> RUNNING and settles RUNNING -> FAILED, so the second failure is at least two versions on, while one settlement broadcast twice carries one version. The identity is therefore run_id:state_version, read from the envelope the handler already parses before the switch. No wire field is added, no backend line changes, and the earlier (code, reason) identity is removed rather than kept alongside it. A frame that arrives with no version gets no identity and keys on the text alone, which is the behaviour that predates this change: the version gate at the top of the handler drops such a frame once any versioned event has been seen for the task, and when the task has no versioned event on record either, two such frames key on the same text and the second still collapses. The identity is withheld there rather than guessed -- attaching the state tuple needs the row, and a settled FAILED task has one.

…ror table The frame's code, not the relayed sentence, decides the bubble's wording now: the five connector-runtime codes that can reach this frame are listed in the client error-code table this repository already uses for the root error channel's error_code field, each with a translation key and an English fallback. That table is the reason the wording now survives an untrusted transport, where relaying server prose is refused by design (xorbitsai#1938) -- before this, only three codes had curated wording and every other code, connector codes included, read "Unknown error" for an anonymous widget or share-link visitor. Only codes with a producer that can reach this frame today are listed, the same rule the server's reason whitelist already states about itself. Of the other five connector-runtime codes, two have no raise site in this repository at all. The remaining three are raised while a connector-runtime payload is being validated. Nothing that reaches those checks settles a task: a request handler answers the call with an error response (the /v1 task endpoints, and the trigger-config endpoints, which convert the failure into their own service error), and the trigger run-preparation path throws before the task row is created and records the failure on its TriggerRun row. No settled task means no terminal frame. A code the table does not list keeps the generic prefixed wording. This also fixes: the logged-in audience no longer sees a different wording than an untrusted transport gets for connector_runtime_unavailable, since that code now has its own table entry instead of relaying the server's four different English sentences for it. The old single-purpose CONNECTOR_RUNTIME_MISSING_VALUE_CODES set and its one i18n key are gone.

N1: the code passed into create_terminal_task_error_event is typed as a bare str but not enforced at runtime -- ConnectorRuntimeError itself types its code the same way and stores it unvalidated. An unhashable value (a list) would raise inside the frozenset membership test on a path whose whole point is that it never raises; a hashable non-string is simply not a member and already takes the drop path either way. The type check now comes first, the same way the neighboring `details` check already does. N3: the operator log line in task_orchestrator reads the same ConnectorRuntimeError.details attribute the wire projector does, for a different reason (an operator gets the raw connector identity; the wire projector filters it out) -- but it read it without the same shape guard. Guarded now with the same isinstance check before reading, at the cost of three lines. The non-dict branch this adds has no test witness in this repository today: triggering it needs a `details` attribute reassigned to a non-dict shape after construction, a shape no raise site produces, and the existing log-line test only covers the dict-shaped path continuing to log correctly. The follow-up (a test that exercises the non-dict branch) is not done in this PR, per approved scope. S1: dropped the @lru_cache on _client_visible_error_codes -- a ~30-member frozenset is cheaper to rebuild per call than to reason about as a cache. The deferred import and its circular-dependency comment are unchanged. S2: deleted the ownership-boundary test for PublicErrorDetails's single construction site; the wire-safety guarantee it was adjacent to (`type(details) is not PublicErrorDetails` in the frame builder) is untouched. `_python_sources` stays -- the reason-whitelist derivation still uses it. The reason-whitelist AST machinery is kept rather than flattened to a hardcoded list -- it exists to catch a future raise site shaped like today's interpolated ones, which a list can't -- and gets three more assertions: the three regex-admitted `undeclared_*_key` reasons are now derived from the same section-name constants the raise site loops over, rather than only regex-matched (this surfaced a second interpolated pattern the scanner finds, `missing_context.<key>`, which is deliberately ungrounded -- it is assembled from a connector owner's declared key name and is already asserted elsewhere to never reach the wire); the two blind spots in the scanner itself (a `details=` argument that isn't a literal dict, and a construction reached through an attribute rather than a bare name) are each pinned to their real occupancy today (one non-literal site, zero attribute constructions); and the five opaque reason expressions this repository raises are pinned to the one shape they take (`str(exc)`), so a future %-format or .format() shows up as a failure here instead of silently widening what the whitelist has to cover. Also added: a parametrized test pinning three non-string code shapes through the type gate, and a frontend test for the code/details-nested-under-data half of the existing root/data fallback.

… C1/C2 moves All three comments were carried over verbatim from where the code used to live and pointed at a fixed position ("above", "below") rather than at what they meant. Moving the code left the words in place while the thing they pointed at moved elsewhere in the file, or left a position reference that still landed on something true but not on what the sentence meant: - The isResult comment in projectErrorFrameForDisplay said "see ADD_MESSAGE above", written when this code lived inside the case block far below the ADD_MESSAGE reducer case. The extraction moved it above that reducer case, so "above" now points at nothing there; named the reducer case directly instead of relying on file position. - The six-cell test's controlEnvelope comment said "the no-version cell below", but that cell is the array literal above the it.each callback the comment sits in, not below it. - The dedup-identity comment said "the version gate above", meaning the version gate's call site in the handler (described elsewhere in this PR as "the version gate at the top of the handler", which sits well below this comment, not above it). "Above" happened to still land on something true -- canAcceptTaskControlVersion's definition is above -- but not on what the sentence meant; named the function directly instead of relying on position.

AlexLiu190625 · 2026-09-02T21:21:45Z

This round pushed 5 commits (b6b83244c..2ef06c7d5) on top of the previous head, covering the 3 blocking review comments plus the minor/latent findings. The 8 inline threads each have a per-thread reply; this comment covers the review-body items that don't have a GitHub thread to reply on.

Size (D0-1). Current numbers, full branch vs. main: production 483 insertions / 15 deletions across app-context-chat.tsx, en.ts, zh.ts, client-errors.ts, websocket.py, client_error_messages.py, task_orchestrator.py; tests 2137 insertions / 2 deletions across app-context-chat.test.tsx, client-errors.test.ts, test_terminal_task_error_event.py, test_client_error_messages.py, test_task_orchestrator.py (git diff --numstat against the merge-base). This has grown across two review rounds; the original PR description's estimate is stale.

"A self-built compound key" (D0-2). The mechanism itself -- occurrenceIdentity as a third argument to the dedup check, threaded through isDuplicateMessageForViewedTask -- predates this PR; this round only changes which value goes into that existing argument. What's now fixed is the value itself: it reads run_id:state_version off the frame's own envelope, the same shape as waitingRequestId (a per-turn identity this file already generates and keys on elsewhere) rather than a hash of the failure's class. That's the generalization of the existing pattern you pointed at, not a new one.

Two wordings, one table (D0-3). Fixed via option A -- see the reply on the "Unknown error" thread.

Narrowing the closed set (N2). Deferred, not done in this PR. The closed set is the gate deciding which codes can reach the wire at all; narrowing it to a connector-specific subset means defining that subset and touching the shared /v1 surface, which is a larger change than this PR's point. No leak today: everything that reaches this path is already a member of the connector-runtime family.

Guard baseline numbers (N4). producers == 29, error_payloads == 51 on current main. This PR does not touch test_websocket_client_safe_errors.py at all -- the branch's own copy of that file predates a recent bump on main (its baseline reads error_payloads == 50 there), and PR description Disclosure 2 has been corrected to state the current-main numbers and that this PR is not the reason they moved.

Four blind spots (N5). Addressed with three new assertions -- see the reply on the AST-machinery thread.

Replay is English-only (N6). Confirmed, unchanged, no action -- this was already an acknowledged deferred item and PR description Disclosure 6 now has an added paragraph stating it explicitly (the historical replay builder never emits task_error, only task_info/trace events/historical_data_complete).

resume / execute_task (N7). Partial correction: this PR's description has never listed execute_task as out of scope -- there is no occurrence of that string in the body, past or present. What is true, and is now written into Out of scope, is the resume-background path's existing behavior: two of its arms broadcast a terminal frame without a code at all, and the arm that broadcasts without settling can emit two failures at one state version, in which case the second still collapses -- unchanged from main.

settlement_error prefix (N8). The risk direction is the opposite of the concern. On main, a ConnectorRuntimeError fell through to the generic else branch at the terminal settlement, producing "setup/run error: ConnectorRuntimeError: <code>: <safe_message>" (ConnectorRuntimeError.__str__ already returns f"{self.code}: {self.safe_message}"). This PR's third branch just assigns str(exc) directly, so the string is shorter -- the code prefix was already part of __str__ and is still there, only the outer "setup/run error: ConnectorRuntimeError: " wrapper is gone. All four readers of this string (v1/tasks.py:836, services/a2a_protocol.py:434-435, api/conversation_logs.py:593, services/triggers.py:1769) were checked individually: none of them parse or pattern-match its content, they all forward it as an opaque string.

Structural twin (N9). No action -- already acknowledged as intentional in an earlier pass.

traceEvents drained (N10). Deferred, confirmed as a follow-up. The mechanism is real: the reducer folds the accumulated trace-event buffer into the first isResult message and clears it (app-context-chat.tsx:1071 region), so a second bubble's wording is unaffected but its trace is empty. Not adding a test for it this round to keep this batch to the dedup/wording fixes rather than widening scope.

CTA mismatch (N11). Resolved as a side effect of option A: common.errors.connectorRuntimeMissing and both its locale strings are deleted; the 5 new wording entries have the same call-to-action on both sides of every entry (only the connector_runtime_unavailable/scheduled_secret_unavailable-style ones include "try again later," on both en and zh).

N12(a)(c)(d)(f). Deferred, recorded as follow-ups, not done this round: (a) the existing stub-call test asserts on kwargs passed to the mock rather than a value read back off the row; (c) no orchestrator-level test tampers with .details after construction; (d) the hand-written reason list still covers 8 of 12 raise sites directly (the AST double-direction test covers the rest); (f) no test exercises the traceEvents-draining interaction directly (see N10 above).

N12(b) and N12(e). Both done this round -- see the reply on the type-gate thread (N12(b)); N12(e) is a new frontend test for the code/details-nested-under-data half of the existing fallback (getTaskErrorProjection already read data?.code before root.code; this pins that branch).

"Blocking: yes." All three are fixed, replies posted on each thread with the test and mutation evidence.

…ime-structured-errors

main's xorbitsai#1912 added resolve_connector_access_or_raise() to connector_team_scope.py, whose generic-exception fallback arm raises ConnectorRuntimeError with reason "connector_access_resolution_failed". This PR's whitelist-coverage test requires every reason a ConnectorRuntimeError construction can produce to be classified in the same change that adds the raise site, so merging main's new raise site into this branch turned that test red until the reason is classified here. Judged against the two questions this whitelist's comment asks: does the reason name who owns the task, or state what an authorization check concluded? Neither. It only reports that resolving connector access failed, the same as the already-whitelisted team_scope_resolution_failed a few lines above -- same module, same 503, same fallback-arm shape.

…ot line Merging main shifted websocket.py's line numbers; replaced the two stale websocket.py:<line> citations with the hosting function's name, matching this PR's existing convention of naming the referent instead of relying on a position that moves.

AlexLiu190625 · 2026-09-03T11:49:01Z

Heads-up on a push during the re-review window — this is not a response to any review comment, and no reviewed logic changed.

CI's Pytest Fast (web) went red on the merge commit it builds: main merged #1912, which adds a new ConnectorRuntimeError reason (connector_access_resolution_failed), and this PR's whitelist-coverage test requires every raise-site reason to be classified. Each side is green on its own; the combination was not.

New head is 0a3c3f9, three commits on top of 2ef06c7:

a merge of upstream/main, so the new reason's raise site and its whitelist entry live in the same tree (merged rather than rebased to keep the commit SHAs cited in the review replies valid; the squash merge folds the merge commit away),
bb3940a: one line adding connector_access_resolution_failed to CONNECTOR_RUNTIME_PUBLIC_REASONS — it names no task ownership and no authorization outcome, and it is the same fixed-503 shape as its sibling team_scope_resolution_failed from the same module's fallback path,
0a3c3f9: comment-only — two websocket.py:<line> citations in a test comment drifted with the merge and are now anchored by function name instead.

The PR description's whitelist enumeration was updated to match.

rogercloud

Summary

ConnectorRuntimeError previously fell into the generic terminal-failure path: its curated public-safe message was discarded for a fixed "Task execution failed." string, and the resulting frame lacked isResult, so the chat UI silently filtered it out and left the turn stuck on a placeholder until reload. This PR classifies the failure at settlement, adds a stable code/details pair the client can render and localize without relaying server prose to an untrusted (possibly anonymous) audience, marks the frame isResult so it renders and ends the turn, and switches terminal-frame dedup to the pre-existing (run_id, state_version) identity.

Blocking: no — recommended event: APPROVE

Design note (Round 0)

Verdict: acceptable with reservations. The diagnosis is correct, the client-side classification/dedup mechanism is sound, and it correctly reuses existing machinery rather than inventing new plumbing. The reason whitelist in PublicErrorDetails (src/xagent/web/services/client_error_messages.py) is a genuinely well-built security boundary: frozen dataclass, __post_init__-only construction, all 13 whitelisted members trace to real raise sites, and the one owner-controlled reason shape (missing_context.<key>) is correctly excluded from the whitelist.

The one substantive reservation: the entire details/PublicErrorDetails/whitelist apparatus (~150 lines of the most safety-critical new code in this PR, plus ~90 lines of comments justifying it) ships a reason field that no client-side consumer reads today. getTaskErrorProjection (frontend/src/contexts/app-context-chat.tsx:938-951) parses details.reason into a projection object, but projectErrorFrameForDisplay (frontend/src/contexts/app-context-chat.tsx:991-1074) never reads it — only .code drives the rendered bubble. History shows the field's one former consumer (the old (code, reason) dedup key) was removed later in this same PR when dedup moved to (run_id, state_version), so the apparatus went from "one consumer" to "zero consumers" during this PR's own review cycle without the escalation being revisited. This isn't a correctness problem — it's real defense-in-depth for the wire boundary, and worth keeping if the "owner-gated per-task requirements endpoint" mentioned in the PR description is a near-term follow-up. But as of this PR, code alone would deliver the same user-visible fix with far less new surface area to review and maintain. Non-blocking; recommend either wiring a real consumer soon or trimming the apparatus until one exists.

Inline findings (all non-blocking)

Posted as separate inline review comments on src/xagent/web/api/websocket.py:424, src/xagent/web/api/websocket.py:345, tests/web/api/client_safe_ast_guard.py / tests/web/api/test_websocket_client_safe_errors.py:179, frontend/src/contexts/app-context-chat.tsx:1072, and src/xagent/web/api/websocket.py:402. Summaries:

code is silently discarded whenever details fails validation (websocket.py:424) — code and details are validated independently but written under one combined if code is not None and details is not None: gate, so a rejected details also drops a perfectly valid code — the field the client actually renders. Not reachable via the current production call site (the sole caller always builds a valid PublicErrorDetails alongside a valid code), so latent rather than live, but worth decoupling the two writes and updating the "dropped=details" log line to mention code too.
Deferred V1ErrorCode import inside a function documented as "never raises" isn't guarded (websocket.py:345, inside _client_visible_error_codes()) — the sole caller's contract (and its own try/except) explicitly relies on this function never raising, but the deferred import isn't wrapped. .v1.errors is already imported at app-startup (web/app.py), so this is a guaranteed cache hit today, not a realistic failure path — but it's the one line that technically violates the function's own stated invariant. Consider a one-line try/except ImportError or a comment noting the reliance on app-boot import ordering.
Static client-safe AST guard has a blind spot for this PR's new fields — tests/web/api/client_safe_ast_guard.py's SENSITIVE_PAYLOAD_FIELDS/SAFE_MESSAGE_BUILDERS and terminal-task-error handling only inspect the message positional argument and don't list connector_runtime_client_message/connector_runtime_public_error; tests/web/api/test_websocket_client_safe_errors.py:179 scans only websocket.py, not task_orchestrator.py, which is where the actual code=/details= call site lives. A future unsafe caller passing bad code/details would be invisible to this static sweep (though still caught at runtime by the builder's own type/membership checks, which are correctly implemented and tested). Worth closing in a follow-up.
isResult: true now also applies to cancel and resume-settlement task_error frames, draining state.traceEvents into those bubbles, with no dedicated test — frontend/src/contexts/app-context-chat.tsx:1014 (isTerminal = message.type === "task_error") and :1072 (isResult: isTerminal), feeding the traceEvents-draining reducer branch around :1385-1391; producers are src/xagent/web/services/external_task_cancel.py:404 and src/xagent/web/api/websocket.py:3007 (resume-settlement failure), both task_error-typed without a code. Verified not a regression — these frames had no isResult handling pre-PR either, so they were previously invisible in the panel — but the cancel/resume shapes' isResult/traceEvents interaction is untested. A related two-terminal-frames-in-one-task trace-accounting concern was already raised and accepted as a non-blocking follow-up in an earlier round. Recommend a follow-up test for these shapes.
_client_visible_error_codes() validates against the full ~30-member V1ErrorCode enum, not just the connector-runtime subset (websocket.py:402) — carried forward from an earlier round's finding "N2" (review body 5085310824, conversation comment 5516598157), which the author acknowledged as real but explicitly deferred this round ("would require touching shared /v1 surface"). It remains a type-membership check rather than an audience/purpose check, inconsistent with the audience-scoping care applied elsewhere in this PR (e.g. the reason whitelist). Confirmed intentional via test_the_closed_set_is_the_v1_one_not_a_copy and not exploitable today (the sole call site only ever passes one of ~10 known connector codes). Flagging again since it was deferred rather than resolved — worth a decision on whether another deferral is fine or a tracking issue is warranted.

Additional minor/nit findings (grouped, non-blocking)

frontend/src/lib/client-errors.ts and frontend/src/i18n/locales/en.ts independently duplicate the same English fallback strings for the 5 connector-runtime error codes, with nothing tying them to each other or to the backend's V1ErrorCode enum — a future rename/copy-edit could silently drift them apart.
tests/web/services/test_client_error_messages.py has brittleness/perf nits: a few assertions hardcode exact counts/line numbers (e.g. assert len(opaque) == 5) that will break on unrelated edits, and ~7 tests each re-parse the full src/ tree via AST with no caching (functools.lru_cache would help).
A docstring in client_error_messages.py states a stricter reachability rule than what's actually enforced — one cited illustrative reason turned out to be reachable mid-execution too, not only from pre-settlement validation. The rule itself is fine; the example needs a small correction.
Several new comments reference this review's own round/issue numbering (e.g. "I-3b", "I-A") that will be meaningless once this review is forgotten — consider replacing with plain descriptions of the invariant being protected.
getTaskErrorProjection reads code and details via two independently-chosen fallback chains (data.code ?? root.code, data.details ?? root.details) that could in principle pair mismatched values from different nesting levels. No current producer emits such a shape — latent only.
PublicErrorDetails.reason uses a frozenset[str] + silent-null-in-__post_init__ pattern, while this file's own ClientErrorCode uses a StrEnum + exhaustive table — a design-consistency suggestion, not a defect (the frozenset also accommodates a couple of dynamically-derived/opaque reason shapes a strict enum would complicate).

Checked, no issue

The dual code/error_code wire-field structure looked like it could cause dedup/render divergence, but both practical failure modes were already found and fixed in earlier rounds (commits 4b8b1ebc4, 08c37391f) with dedicated regression tests.
handle_execute_task's except RuntimeError arm still using generic messaging for ConnectorRuntimeError is provably unreachable for this failure class — the exception is fully consumed and classified inside task_orchestrator's own handler first.

Simplification opportunities

delete: connector_runtime_client_message's fallback: str = CLIENT_SAFE_TASK_FAILURE parameter (src/xagent/web/services/client_error_messages.py) has exactly one caller (task_orchestrator.py:1983), which never overrides the default. Drop the parameter and reference CLIENT_SAFE_TASK_FAILURE directly in the function body.

net: -1 lines possible

Blocking status & recommended decision

Blocking: no. Every finding above — inline and grouped — is confirmed non-blocking: latent-but-unreachable robustness gaps, a testing/tooling coverage gap, an unread-but-safe field, and cosmetic/consistency nits. This is a well-engineered, thoroughly self-reviewed PR: the author already resolved 3 major blockers and multiple minor issues across 18 prior review rounds, and every new concern raised in this pass is non-blocking.

Recommended event: APPROVE.

The terminal task_error frame no longer carries a details object. The server used to project a connector-runtime failure onto a (code, reason) pair, filtering reason through a fixed allowlist before it reached broadcast_to_task -- whose audience includes anonymous widget and share-link visitors. That allowlist has no consumer today: the client only ever read the code, so the whole reason channel was dead weight carrying review risk with nothing on the other end. create_terminal_task_error_event now takes only a code argument. PublicErrorDetails, the reason allowlist, and the two-value projector are gone; connector_runtime_client_code replaces them with a single-purpose projection from exception to code. The frontend projection follows: TaskErrorProjection carries only code, and getTaskErrorProjection no longer reads a details field.

…me codes The terminal task_error frame validated its code argument against V1ErrorCode, the repository's full /v1 error surface -- roughly thirty codes covering everything from rate limiting to workforce archival, most of which have nothing to do with a connector runtime. The frame reaches anonymous widget and share-link visitors, so its own closed set should describe exactly what belongs there, not borrow a much wider one that happens to be a superset. CONNECTOR_RUNTIME_CLIENT_ERROR_CODES replaces the V1ErrorCode lookup with the eight connector-runtime codes this repository actually raises as a ConnectorRuntimeError. The two authorization-outcome codes (mcp_oauth_authorization_failed, delegated_authorization_failed) stay out: nothing raises them today, and each one states the outcome of an authorization check, which this frame must never carry.

create_terminal_task_error_event's own runtime gate only checks the value of a code argument -- whether it is a member of the closed set -- and has no way to tell a curated projection from an incidental string that happens to collide with a real code today. A future call site could pass str(exc) or read .code straight off an exception and this repository would not notice until the wrong fact reached an anonymous visitor. This AST-based test closes that gap statically: every call site under web/ that passes code= must bind it, in the same function, from a direct call to connector_runtime_client_code -- the one projector this repository trusts for this purpose. Today that is exactly one call site, task_orchestrator.py's _runner.

…trace drain Two terminal task_error producers carry no code today, and neither had coverage: external_task_cancel.py's cancellation broadcast (message only) and websocket.py's resume-settlement broadcast, which carries error_code on the root instead. Both still have to make the frame the turn's result on isTerminal alone, and the cancellation path also has to carry forward whatever trace events accumulated on state.traceEvents before the settlement -- the one place that happens, in ADD_MESSAGE's isResult branch. Adds a resume-settlement case to the projectErrorFrameForDisplay table and a transport-level test that seeds two trace events, delivers a cancellation frame, and checks they land on the settling message while state.traceEvents is cleared. Also pins client-errors.ts's fallback strings against the English locale so the two tables cannot drift.

connector_runtime_client_message's fallback parameter has had exactly one caller since it was added, and that caller never overrides the default. The two return sites use CLIENT_SAFE_TASK_FAILURE directly now; required_mcp_unavailable_client_message keeps its own fallback parameter, which does have an overriding caller.

Sweeps the batch for review-round references and stale comments the earlier commits in this sequence left behind. The regex sweep itself found nothing, but a manual read turned up three comments describing a mechanism the batch already removed: one docstring still claimed code was checked against "the same closed set the /v1 surface pins against" after the closed set became a curated subset, and two fixture comments in test_task_orchestrator.py still described a reason value as "public" or "withheld" from the wire after this batch's first commit removed the wire's reason channel entirely.

…op a duplicate frame pin

AlexLiu190625 · 2026-09-03T17:50:56Z

The seven commits after 0a3c3f947 act on each point in the last review:

details.reason has no consumer on the client. Agreed, and taken further than wiring one up: details is gone from the frame entirely. TaskErrorProjection is now { code: string } and getTaskErrorProjection only ever reads code (62bcf1380, frontend/src/contexts/app-context-chat.tsx:51-53 and :934-941). The frame now carries only what the client actually renders.
code was silently discarded whenever details failed validation. Moot along with the field it was coupled to: the frame builder now has one gate, if code is not None: (62bcf1380, src/xagent/web/api/websocket.py:392-393).
The deferred V1ErrorCode import inside _client_visible_error_codes() wasn't guarded. That function is gone. The closed set is now a module-level constant, CONNECTOR_RUNTIME_CLIENT_ERROR_CODES, imported at the top of websocket.py from client_error_messages.py (1710e8b79, src/xagent/web/api/websocket.py:107, defined at src/xagent/web/services/client_error_messages.py:182-192). No import happens inside the function any more, so there's nothing left to guard.
The static AST guard doesn't cover the actual code= call site in task_orchestrator.py. New file tests/web/api/test_terminal_task_error_frame_origins.py (0ad1b7d99) scans all of web/ and asserts every code= argument passed to create_terminal_task_error_event is bound, in the same function, from a direct call to connector_runtime_client_code -- test_every_code_argument_traces_to_the_projector (:127-134), which today resolves to exactly one call site, task_orchestrator.py's _runner.
Cancel and resume-settlement task_error frames were untested for isResult/trace draining. Two new tests (946bfa7f0): it("drains accumulated trace events onto the cancellation bubble") (frontend/src/contexts/app-context-chat.test.tsx:6397) delivers a cancellation frame after seeding two trace events and checks they land on the settling message while state.traceEvents clears; the it.each case "a resume-settlement frame with a root error_code on a trusted transport" (:6522) covers the resume-settlement shape, which carries error_code on the root rather than code.
The closed set validated against the full ~30-member V1ErrorCode, not the connector-runtime subset. Narrowed to CONNECTOR_RUNTIME_CLIENT_ERROR_CODES, the eight codes this repository actually raises as a ConnectorRuntimeError (1710e8b79, src/xagent/web/services/client_error_messages.py:182-192). mcp_oauth_authorization_failed and delegated_authorization_failed are the two excluded: nothing raises either as this exception today, and each one states the outcome of an authorization check, which this frame must never carry.
client-errors.ts and en.ts duplicate the same fallback strings with nothing tying them together. New test it("keeps the fallback strings identical to the English locale") (946bfa7f0, frontend/src/lib/client-errors.test.ts:51) asserts every code's fallback string equals the corresponding en.ts entry, so an edit to one without the other now fails.
Brittle exact counts/line numbers and repeated AST parsing in test_client_error_messages.py. That whole file is gone along with the whitelist it was testing -- 62bcf1380 cut it from 609 lines to 89. The AST test that remains, test_terminal_task_error_frame_origins.py, asserts against a (relative path, enclosing function name) tuple rather than a line number or a raw count.
A docstring states a stricter reachability rule than what's enforced. We could not find a matching docstring in client_error_messages.py -- that file makes no reachability claim. The comment that does make one is frontend/src/lib/client-errors.ts:34-41; if that's the one meant, its wording is softened rather than corrected: it no longer asserts which of the three producer-less codes get raised mid-execution versus pre-settlement, only that none of the three has a producer that can reach a terminal frame today (1710e8b79). If a different sentence was meant, please point us at it.
New comments reference this review's own round/issue numbering. Removed; comments now describe the invariant directly rather than citing a round or an id (9d0155239).
getTaskErrorProjection read code/details via two independently-chosen fallback chains that could pair mismatched values. Moot: there's only one field to read now, so there's no second chain to mismatch (62bcf1380).
PublicErrorDetails.reason used a frozenset while ClientErrorCode uses a StrEnum. Moot: PublicErrorDetails and the reason whitelist are gone (62bcf1380).
connector_runtime_client_message's unused fallback parameter. Dropped; the function now references CLIENT_SAFE_TASK_FAILURE directly in its body (ecf95545c, src/xagent/web/services/client_error_messages.py:133).

Verification on this branch: backend 2848 passed, 17 skipped (postgres-only), frontend 193 passed, tsc --noEmit clean.

XprobeBot added the bug Something isn't working label Aug 28, 2026

AlexLiu190625 added 9 commits August 31, 2026 11:36

AlexLiu190625 force-pushed the feat/connector-runtime-structured-errors branch from 9d2a624 to abc51dd Compare August 31, 2026 03:53

gemini-code-assist Bot reviewed Aug 31, 2026

View reviewed changes

Comment thread src/xagent/web/services/task_orchestrator.py

Comment thread frontend/src/i18n/locales/en.ts Outdated

Comment thread frontend/src/i18n/locales/zh.ts Outdated

AlexLiu190625 requested a review from rogercloud September 1, 2026 09:47

rogercloud requested changes Sep 1, 2026

View reviewed changes

Comment thread frontend/src/contexts/app-context-chat.tsx Outdated

Comment thread frontend/src/contexts/app-context-chat.tsx Outdated

Comment thread src/xagent/web/services/task_orchestrator.py

Comment thread frontend/src/contexts/app-context-chat.test.tsx Outdated

AlexLiu190625 added 5 commits September 2, 2026 02:37

AlexLiu190625 requested a review from rogercloud September 1, 2026 23:13

rogercloud requested changes Sep 2, 2026

View reviewed changes

AlexLiu190625 added 5 commits September 3, 2026 03:13

AlexLiu190625 added 3 commits September 3, 2026 19:06

Merge remote-tracking branch 'upstream/main' into feat/connector-runt…

0841ea9

…ime-structured-errors

AlexLiu190625 requested a review from rogercloud September 3, 2026 12:34

rogercloud approved these changes Sep 3, 2026

View reviewed changes

AlexLiu190625 added 7 commits September 4, 2026 00:20

test(web): name the closed-set producer guard for what it pins and dr…

cdadb67

…op a duplicate frame pin

AlexLiu190625 added this pull request to the merge queue Sep 3, 2026

Merged via the queue into xorbitsai:main with commit 8fa0efd Sep 3, 2026
16 checks passed

Uh oh!

Conversation

AlexLiu190625 commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Approach

Disclosures

Notes for reviewers

Out of scope

Verification

Uh oh!

gemini-code-assist Bot commented Aug 28, 2026

Uh oh!

AlexLiu190625 commented Aug 31, 2026

Uh oh!

gemini-code-assist Bot commented Aug 31, 2026

Uh oh!

AlexLiu190625 commented Aug 31, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rogercloud left a comment

Choose a reason for hiding this comment

Summary

Design verdict

Findings

F1 — Nonterminal root errors are marked as final results

F2 — Arbitrary runtime key names are disclosed to public widget/share audiences

F3 — Structured connector error projection is lost on durable history replay

F4 — Interpolation test mock makes key-rendering coverage false-green

Prior finding dispositions

Blocking status & recommended decision

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rogercloud left a comment

Choose a reason for hiding this comment

Summary

Round 0 — design-level findings (not tied to a single line)

Blocking findings (3, all severity: major)

1. Dedup silently drops the whole result bubble on a repeat connector failure within 30s

2. Same dedup gap also swallows a second identical generic (non-connector) failure

3. "Unknown error" fallback defeats the curated safe-message mechanism for 7 of 10 target error codes on anonymous transports

Non-blocking findings (grouped)

Simplification opportunities

Blocking status & recommended decision

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexLiu190625 commented Sep 2, 2026

Uh oh!

AlexLiu190625 commented Sep 3, 2026

Uh oh!

rogercloud left a comment

Choose a reason for hiding this comment

Summary

Design note (Round 0)

Inline findings (all non-blocking)

Additional minor/nit findings (grouped, non-blocking)

Checked, no issue

Simplification opportunities

Blocking status & recommended decision

Uh oh!

AlexLiu190625 commented Sep 3, 2026

Uh oh!

Uh oh!

AlexLiu190625 commented Aug 28, 2026 •

edited

Loading