From 31bf719cbb474550f2fa6e00b855cff9380d2670 Mon Sep 17 00:00:00 2001 From: Alexliu Date: Fri, 28 Aug 2026 23:19:21 +0800 Subject: [PATCH 01/28] feat(web): project connector runtime failures onto a wire-safe error 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 "." 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. --- .../web/services/client_error_messages.py | 133 +++++ .../services/test_client_error_messages.py | 460 ++++++++++++++++++ 2 files changed, 593 insertions(+) create mode 100644 tests/web/services/test_client_error_messages.py diff --git a/src/xagent/web/services/client_error_messages.py b/src/xagent/web/services/client_error_messages.py index 197f293343..0826566b70 100644 --- a/src/xagent/web/services/client_error_messages.py +++ b/src/xagent/web/services/client_error_messages.py @@ -1,8 +1,11 @@ """Fixed client-visible fallbacks for incidental server failures.""" +import re +from dataclasses import dataclass from enum import StrEnum from ...core.tools.adapters.vibe.config import RequiredMCPUnavailableError +from ...core.tools.adapters.vibe.connector_runtime import ConnectorRuntimeError CLIENT_SAFE_VALIDATION_ERROR = "The message could not be processed. Please try again." @@ -111,3 +114,133 @@ def required_mcp_unavailable_client_message( if message.strip(): return message return fallback + + +def connector_runtime_client_message( + error: BaseException, + *, + fallback: str = CLIENT_SAFE_TASK_FAILURE, +) -> str: + """Adapt the curated connector-runtime failure without a generic escape. + + The runtime check keeps this boundary fail-closed even if a future caller + passes an incidental exception despite the function's specific name. + """ + + if not isinstance(error, ConnectorRuntimeError): + return fallback + message = error.safe_message + if isinstance(message, str) and message.strip(): + return message + return fallback + + +CONNECTOR_RUNTIME_PUBLIC_REASONS = frozenset( + { + # Missing values and binding. + "not_provided", + "store_lost", + "connector_not_selected", + "auth_selector_not_supported", + "duplicate_ref", + "undeclared_context_key", + "undeclared_secrets_key", + "undeclared_auth_selector_key", + # Raised by the runtime value-fill boundary. + "payload_too_large", + "encryption_unavailable", + # Fixed 503 strings built by direct ConnectorRuntimeError construction + # in three other modules. Each one states that a server-side component + # is unavailable; none of them states who owns the task, or how an + # authorization check resolved. Two further strings of exactly this + # shape (runtime_task_identity_mismatch, runtime_owner_mismatch) are + # deliberately absent for that reason -- see the class docstring below. + "team_scope_resolution_failed", + "team_env_resolution_failed", + "runtime_view_resolution_failed", + "custom_api_config_load_failed", + } +) +CONNECTOR_RUNTIME_PUBLIC_REASON_PREFIXES = frozenset( + { + "missing_context", + "type_mismatch.context", + "type_mismatch.secrets", + "type_mismatch.auth_selector", + "conflict.context", + "conflict.secrets", + "conflict.auth_selector", + } +) +# The declared runtime key grammar, reused verbatim from +# core/tools/adapters/vibe/connector_runtime.py:141-144. +_RUNTIME_KEY_RE = re.compile(r"^[A-Za-z0-9_-]+$") + + +def _is_public_reason(reason: object) -> bool: + """True when this reason may reach a client. Used by PublicErrorDetails.""" + + if not isinstance(reason, str): + return False + if reason in CONNECTOR_RUNTIME_PUBLIC_REASONS: + return True + prefix, separator, key = reason.rpartition(".") + if not separator: + return False + if prefix not in CONNECTOR_RUNTIME_PUBLIC_REASON_PREFIXES: + return False + return _RUNTIME_KEY_RE.fullmatch(key) is not None + + +@dataclass(frozen=True) +class PublicErrorDetails: + """The only shape allowed into a task_error frame's ``details``. + + ``reason`` is normalized on construction: a value that is not a listed + enum member, and not ``.``, becomes + ``None``. Constructing this type and passing the reason whitelist are + therefore the same act -- there is no path that produces an instance + carrying free text, including a direct call from another module. + + Nulling rather than raising is deliberate: every construction site is on + the reporting path of an already-failed task, and raising there would + turn a diagnosable failure into an undiagnosable crash. + + The sink is ``broadcast_to_task``, whose audience includes anonymous + widget and share-link visitors, so every listed reason and every new + field must answer one question first: can a visitor who is not the task + owner read the task's ownership, or the outcome of an authorization + check, out of it? There is no ``connector_ref`` field because the answer + for it is yes; two runtime reasons are omitted for the same answer. + """ + + reason: str | None + + def __post_init__(self) -> None: + if self.reason is not None and not _is_public_reason(self.reason): + object.__setattr__(self, "reason", None) + + def to_wire(self) -> dict[str, str]: + return {"reason": self.reason} if self.reason is not None else {} + + +def connector_runtime_public_error( + error: BaseException, +) -> tuple[str, PublicErrorDetails] | None: + """Project a connector-runtime failure onto the wire-safe (code, details). + + Returns ``None`` for anything else, so a caller cannot widen the surface + by passing an incidental exception. The reason filter itself lives in + ``PublicErrorDetails``; this function only decides whether the exception + is one we project at all. + """ + + if not isinstance(error, ConnectorRuntimeError): + return None + details = error.details + if not isinstance(details, dict): + # A details payload of the wrong shape means the exception instance + # itself is not trustworthy. Fall all the way back to the opaque + # failure rather than guessing which half of it is still readable. + return None + return error.code, PublicErrorDetails(reason=details.get("reason")) diff --git a/tests/web/services/test_client_error_messages.py b/tests/web/services/test_client_error_messages.py new file mode 100644 index 0000000000..8ef63f877a --- /dev/null +++ b/tests/web/services/test_client_error_messages.py @@ -0,0 +1,460 @@ +"""Contracts for the wire-safe projection of connector-runtime failures. + +The projection has two halves and both are pinned here: the message adapter +(fail-closed on anything that is not a ``ConnectorRuntimeError``) and the +``(code, details)`` projector whose reason whitelist lives inside +``PublicErrorDetails.__post_init__``. +""" + +from __future__ import annotations + +import ast +import re +from pathlib import Path + +import pytest + +from xagent.core.tools.adapters.vibe.config import RequiredMCPUnavailableError +from xagent.core.tools.adapters.vibe.connector_runtime import ConnectorRuntimeError +from xagent.web.services import client_error_messages +from xagent.web.services.client_error_messages import ( + CLIENT_SAFE_TASK_FAILURE, + CONNECTOR_RUNTIME_PUBLIC_REASON_PREFIXES, + CONNECTOR_RUNTIME_PUBLIC_REASONS, + PublicErrorDetails, + connector_runtime_client_message, + connector_runtime_public_error, +) + +# Anchored on a real module file rather than on the package: xagent is a +# namespace package, so it has no __file__ of its own and may span trees. +SRC_ROOT = Path(client_error_messages.__file__).resolve().parents[2] + + +# -------------------------------------------------------------------------- +# connector_runtime_client_message +# -------------------------------------------------------------------------- + + +def test_client_message_returns_the_curated_safe_message() -> None: + error = ConnectorRuntimeError( + "missing_runtime_context", + "Required connector runtime context is missing.", + ) + + assert ( + connector_runtime_client_message(error) + == "Required connector runtime context is missing." + ) + + +@pytest.mark.parametrize("safe_message", ["", " ", "\n\t"]) +def test_client_message_falls_back_on_a_blank_safe_message(safe_message: str) -> None: + error = ConnectorRuntimeError("missing_runtime_context", safe_message) + + assert connector_runtime_client_message(error) == CLIENT_SAFE_TASK_FAILURE + + +@pytest.mark.parametrize( + "error", + [ + ValueError("secret-token-xyz"), + KeyError("secret-token-xyz"), + RuntimeError("secret-token-xyz"), + RequiredMCPUnavailableError("secret-token-xyz"), + ], +) +def test_client_message_is_fail_closed_for_an_incidental_exception( + error: BaseException, +) -> None: + """The specific name is not the gate; the isinstance check is.""" + + assert connector_runtime_client_message(error) == CLIENT_SAFE_TASK_FAILURE + + +# -------------------------------------------------------------------------- +# I-3b: the reason whitelist lives in the constructor +# -------------------------------------------------------------------------- + + +ILLEGAL_REASONS: list[object] = [ + # The real str(exc) product of validate_runtime_source_key. + "runtime input key must match [A-Za-z0-9_-]+", + "The connector could not resolve tenant acme-corp", + "missing_context.auth_token\nSELECT * FROM connectors", + "missing_context.'auth_token'", + "/etc/xagent/connectors/acme.yaml", + "SELECT value FROM task_connector_runtime_contexts WHERE task_id = 1", + "x" * 5120, + object(), + # Shape-legal, deliberately withheld: both state something about who owns + # the task or how an authorization check resolved, and this frame reaches + # anonymous widget and share-link visitors. + "runtime_owner_mismatch", + "runtime_task_identity_mismatch", +] + + +@pytest.mark.parametrize("reason", ILLEGAL_REASONS) +def test_public_error_details_normalizes_reason(reason: object) -> None: + details = PublicErrorDetails(reason=reason) # type: ignore[arg-type] + + assert details.reason is None + assert details.to_wire() == {} + + +LEGAL_REASONS = [ + "not_provided", + "store_lost", + "payload_too_large", + "encryption_unavailable", + "team_env_resolution_failed", + "team_scope_resolution_failed", + "custom_api_config_load_failed", + "missing_context.auth_token", + "type_mismatch.context.tenant_id", + "conflict.secrets.authorization", +] + + +@pytest.mark.parametrize("reason", LEGAL_REASONS) +def test_public_error_details_keeps_a_listed_reason(reason: str) -> None: + details = PublicErrorDetails(reason=reason) + + assert details.reason == reason + assert details.to_wire() == {"reason": reason} + + +def test_public_error_details_accepts_an_absent_reason() -> None: + assert PublicErrorDetails(reason=None).to_wire() == {} + + +# -------------------------------------------------------------------------- +# connector_runtime_public_error: the three read tiers of exc.details +# -------------------------------------------------------------------------- + + +def test_public_error_projects_code_and_whitelisted_reason() -> None: + error = ConnectorRuntimeError( + "missing_runtime_context", + "Required connector runtime context is missing.", + details={"reason": "missing_context.auth_token"}, + ) + + assert connector_runtime_public_error(error) == ( + "missing_runtime_context", + PublicErrorDetails(reason="missing_context.auth_token"), + ) + + +@pytest.mark.parametrize( + "details", + [ + {}, + {"reason": "the connector could not be reached"}, + {"connector_ref": {"id": 7}}, + ], +) +def test_public_error_reads_an_empty_reason_as_a_present_code( + details: dict[str, object], +) -> None: + """Read-empty is not read-failed: the code still reaches the client.""" + + error = ConnectorRuntimeError("missing_runtime_context", "x", details=details) + + assert connector_runtime_public_error(error) == ( + "missing_runtime_context", + PublicErrorDetails(reason=None), + ) + + +def test_public_error_refuses_a_tampered_details_payload() -> None: + """A details of the wrong shape means the whole instance is untrusted.""" + + error = ConnectorRuntimeError("missing_runtime_context", "x") + error.details = "not a mapping" # type: ignore[assignment] + + assert connector_runtime_public_error(error) is None + + +@pytest.mark.parametrize( + "error", + [ + ValueError("boom"), + RuntimeError("boom"), + RequiredMCPUnavailableError("boom"), + ], +) +def test_public_error_does_not_project_an_incidental_exception( + error: BaseException, +) -> None: + assert connector_runtime_public_error(error) is None + + +# -------------------------------------------------------------------------- +# I-3: nothing but reason can reach the wire +# -------------------------------------------------------------------------- + + +def test_public_error_drops_every_field_but_reason() -> None: + error = ConnectorRuntimeError( + "missing_runtime_context", + "x", + details={ + "reason": "missing_context.auth_token", + "internal_sql": "SELECT 1", + "raw_value": "tenant-secret", + "connector_ref": {"id": 7, "name": "acme"}, + }, + ) + + projected = connector_runtime_public_error(error) + + assert projected is not None + assert set(projected[1].to_wire()) == {"reason"} + + +# -------------------------------------------------------------------------- +# I-34: ownership of the type +# -------------------------------------------------------------------------- + + +def _python_sources() -> list[Path]: + return sorted(SRC_ROOT.rglob("*.py")) + + +def test_public_error_details_is_constructed_in_one_module_only() -> None: + construction_sites: set[str] = set() + subclass_sites: set[str] = set() + + for path in _python_sources(): + tree = ast.parse(path.read_text(encoding="utf-8")) + relative = path.relative_to(SRC_ROOT).as_posix() + for node in ast.walk(tree): + if ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Name) + and node.func.id == "PublicErrorDetails" + ): + construction_sites.add(relative) + if isinstance(node, ast.ClassDef) and any( + isinstance(base, ast.Name) and base.id == "PublicErrorDetails" + for base in node.bases + ): + subclass_sites.add(relative) + + # Not a security boundary -- __post_init__ and the frame builder's + # type(...) is check are. This is an ownership boundary: the semantics of + # this type belong to the projector, so a second construction site or any + # subclass is a design drift that should be seen in review. + assert construction_sites == {"web/services/client_error_messages.py"} + assert subclass_sites == set() + + +# -------------------------------------------------------------------------- +# I-31: the whitelist and the real raise sites stay in step +# -------------------------------------------------------------------------- + + +# Reasons that are listed but have no literal raise site in this repository +# today. Both belong to the runtime value-fill endpoint, which is added by a +# later PR in this series; they are listed now because the whitelist is the +# contract that endpoint is written against. +REASONS_WITHOUT_A_LITERAL_RAISE_SITE = frozenset( + { + "payload_too_large", + "encryption_unavailable", + } +) + +# Literal reasons that the derivation below finds and that are deliberately +# kept off the wire. The first two state the task's ownership and the outcome +# of an authorization check; the audience of this frame includes anonymous +# widget and share-link visitors. The last two are safe fixed strings but are +# English sentences rather than enum values, and rewriting them would mean +# touching an old path this change has no bearing on. The rest are built from +# an exception message, so their content is not controlled. +DELIBERATELY_NOT_PUBLIC_REASONS = frozenset( + { + "runtime_owner_mismatch", + "runtime_task_identity_mismatch", + "runtime section must be an object", + "stored selected refs must be a list", + } +) + + +def _module_string_constants(tree: ast.Module) -> dict[str, str]: + constants: dict[str, str] = {} + for node in tree.body: + if isinstance(node, ast.Assign) and isinstance(node.value, ast.Constant): + if isinstance(node.value.value, str): + for target in node.targets: + if isinstance(target, ast.Name): + constants[target.id] = node.value.value + return constants + + +def _string_bindings( + tree: ast.Module, module_constants: dict[str, str] +) -> dict[str, set[str]]: + """Every ``name = `` binding in the module, flattened across scopes. + + ``module_constants`` is repo-wide so that a reason passed as an imported + constant (``reason=RUNTIME_SECRET_REASON_NOT_PROVIDED``) still resolves + without this scan having to follow imports. Flattening scopes is + deliberate for the same reason: this answers "which literal strings can + end up in a reason", and over-approximating there is the safe direction. + """ + + bindings: dict[str, set[str]] = { + name: {value} for name, value in module_constants.items() + } + + def resolve(node: ast.expr) -> set[str]: + if isinstance(node, ast.Constant) and isinstance(node.value, str): + return {node.value} + if isinstance(node, ast.Name): + value = module_constants.get(node.id) + return {value} if value is not None else set() + if isinstance(node, ast.IfExp): + return resolve(node.body) | resolve(node.orelse) + return set() + + for node in ast.walk(tree): + if isinstance(node, ast.Assign): + values = resolve(node.value) + if not values: + continue + for target in node.targets: + if isinstance(target, ast.Name): + bindings.setdefault(target.id, set()).update(values) + return bindings + + +def _fstring_pattern(node: ast.JoinedStr) -> str | None: + """Turn an f-string reason into a regex covering everything it can build.""" + + parts: list[str] = [] + for value in node.values: + if isinstance(value, ast.Constant) and isinstance(value.value, str): + parts.append(re.escape(value.value)) + elif isinstance(value, ast.FormattedValue): + parts.append(".+") + else: + return None + return "^" + "".join(parts) + "$" + + +def _reason_expressions(tree: ast.Module) -> list[ast.expr]: + """Every expression that becomes a reason on a ConnectorRuntimeError. + + Derived from the construction target, not from a list of modules: a module + list would silently stop covering a raise site added somewhere new. + """ + + found: list[ast.expr] = [] + for node in ast.walk(tree): + if not isinstance(node, ast.Call) or not isinstance(node.func, ast.Name): + continue + if node.func.id == "_raise_runtime_error": + for keyword in node.keywords: + if keyword.arg == "reason": + found.append(keyword.value) + elif node.func.id == "ConnectorRuntimeError": + for keyword in node.keywords: + if keyword.arg != "details" or not isinstance(keyword.value, ast.Dict): + continue + for key, value in zip(keyword.value.keys, keyword.value.values): + if isinstance(key, ast.Constant) and key.value == "reason": + found.append(value) + return found + + +def _derive_reasons() -> tuple[set[str], set[str]]: + trees = { + path: ast.parse(path.read_text(encoding="utf-8")) for path in _python_sources() + } + module_constants: dict[str, str] = {} + for tree in trees.values(): + module_constants.update(_module_string_constants(tree)) + + literals: set[str] = set() + patterns: set[str] = set() + for tree in trees.values(): + bindings = _string_bindings(tree, module_constants) + for expression in _reason_expressions(tree): + if isinstance(expression, ast.Constant) and isinstance( + expression.value, str + ): + literals.add(expression.value) + elif isinstance(expression, ast.Name): + literals.update(bindings.get(expression.id, set())) + elif isinstance(expression, ast.JoinedStr): + pattern = _fstring_pattern(expression) + if pattern is not None: + patterns.add(pattern) + return literals, patterns + + +def _is_listed(reason: str) -> bool: + if reason in CONNECTOR_RUNTIME_PUBLIC_REASONS: + return True + prefix, separator, key = reason.rpartition(".") + return bool(separator) and prefix in CONNECTOR_RUNTIME_PUBLIC_REASON_PREFIXES + + +def test_public_reason_whitelist_covers_every_raise_site() -> None: + literals, _ = _derive_reasons() + + assert literals, "the reason derivation found nothing; the scan is broken" + + unclassified = { + reason + for reason in literals + if not _is_listed(reason) and reason not in DELIBERATELY_NOT_PUBLIC_REASONS + } + assert not unclassified, ( + "these reasons are raised but neither whitelisted nor listed as " + f"deliberately withheld: {sorted(unclassified)}" + ) + + +def test_public_reason_whitelist_has_no_member_without_a_raise_site() -> None: + """The whitelist must not grow entries nothing can actually produce.""" + + literals, patterns = _derive_reasons() + compiled = [re.compile(pattern) for pattern in patterns] + + ungrounded = { + reason + for reason in CONNECTOR_RUNTIME_PUBLIC_REASONS + if reason not in literals + and reason not in REASONS_WITHOUT_A_LITERAL_RAISE_SITE + and not any(expression.match(reason) for expression in compiled) + } + assert not ungrounded, ( + f"these whitelisted reasons are produced nowhere in src/: {sorted(ungrounded)}" + ) + + +def test_the_withheld_reasons_are_really_raised_somewhere() -> None: + """A withheld entry that nothing raises is a stale exemption.""" + + literals, _ = _derive_reasons() + + assert DELIBERATELY_NOT_PUBLIC_REASONS <= literals + + +def test_knowledge_base_scope_reason_is_not_in_the_derived_surface() -> None: + """A same-named literal on a different exception class must stay out. + + ``knowledge_base_team_scope`` raises ``KnowledgeBaseScopeError`` with the + same ``team_scope_resolution_failed`` string. Deriving by construction + target rather than by module list is what keeps it out on its own. + """ + + path = SRC_ROOT / "web" / "services" / "knowledge_base_team_scope.py" + tree = ast.parse(path.read_text(encoding="utf-8")) + + assert _reason_expressions(tree) == [] From abb3e0655a4a1ead0a27c34e85e03e35b87f44ae Mon Sep 17 00:00:00 2001 From: Alexliu Date: Fri, 28 Aug 2026 23:19:47 +0800 Subject: [PATCH 02/28] feat(web): carry an error code on the terminal task_error frame 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. --- src/xagent/web/api/websocket.py | 32 +++++- .../web/api/test_terminal_task_error_event.py | 100 ++++++++++++++++++ 2 files changed, 130 insertions(+), 2 deletions(-) create mode 100644 tests/web/api/test_terminal_task_error_event.py diff --git a/src/xagent/web/api/websocket.py b/src/xagent/web/api/websocket.py index 3c218ccc22..59f43a170b 100644 --- a/src/xagent/web/api/websocket.py +++ b/src/xagent/web/api/websocket.py @@ -104,6 +104,7 @@ CLIENT_SAFE_TASK_FAILURE, CLIENT_SAFE_VALIDATION_ERROR, ClientErrorCode, + PublicErrorDetails, client_error_message, ) from ..services.db_runtime import ( @@ -323,10 +324,33 @@ def _task_error_payload( def create_terminal_task_error_event( task_id: int, message: str, + *, + code: str | None = None, + details: PublicErrorDetails | None = None, ) -> dict[str, Any]: - """Shape an error event after the exact lease owner commits FAILED.""" + """Shape an error event after the exact lease owner commits FAILED. - return { + ``code`` and ``details`` are written only when both are supplied, so a + caller that passes neither still gets the same six-key frame. ``details`` + is accepted as ``PublicErrorDetails`` itself and nothing else -- not a + subclass -- because that class's ``__post_init__`` is where the reason + whitelist lives. + """ + + # Python annotations are not enforced at run time, so the mypy gate on the + # signature above is not the whole door: a caller that routes through Any + # (a dict from JSON, a **kwargs splat) type-checks clean and would reach + # to_wire() as an AttributeError deep in this function. Name the contract + # here instead. + # + # `type(...) is`, not isinstance: a frozen dataclass can be subclassed, and + # a subclass that overrides to_wire() without reading self.reason passes + # both isinstance and mypy while bypassing the whitelist in __post_init__. + # Only the class itself carries that guarantee. + if details is not None and type(details) is not PublicErrorDetails: + raise TypeError("details must be a PublicErrorDetails") + + event: dict[str, Any] = { "type": "task_error", "message": message, "task_id": task_id, @@ -337,6 +361,10 @@ def create_terminal_task_error_event( "error": message, "timestamp": datetime.now(timezone.utc).timestamp(), } + if code is not None and details is not None: + event["code"] = code + event["details"] = details.to_wire() + return event def _client_message_id(value: Any) -> str | None: diff --git a/tests/web/api/test_terminal_task_error_event.py b/tests/web/api/test_terminal_task_error_event.py new file mode 100644 index 0000000000..811957f688 --- /dev/null +++ b/tests/web/api/test_terminal_task_error_event.py @@ -0,0 +1,100 @@ +"""Frame-shape contracts for ``create_terminal_task_error_event``. + +Two things are pinned here: the four call sites that pass neither ``code`` nor +``details`` still get the same six-key frame, and ``details`` is accepted as +``PublicErrorDetails`` itself and nothing else -- not a dict, not a duck type, +not a subclass. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from typing import Any + +import pytest + +from xagent.web.api.websocket import create_terminal_task_error_event +from xagent.web.services.client_error_messages import PublicErrorDetails + +BASE_FIELDS = {"type", "message", "task_id", "task", "error", "timestamp"} + + +@pytest.mark.parametrize( + "kwargs", + [ + {}, + {"code": "missing_runtime_context"}, + {"details": PublicErrorDetails(reason="not_provided")}, + ], + ids=["neither", "code-only", "details-only"], +) +def test_terminal_error_event_shape_unchanged(kwargs: dict[str, Any]) -> None: + """Both new fields are written together or not at all.""" + + event = create_terminal_task_error_event(1, "x", **kwargs) + + assert set(event.keys()) == BASE_FIELDS + + +def test_terminal_error_event_carries_both_new_fields_together() -> None: + event = create_terminal_task_error_event( + 1, + "x", + code="missing_runtime_context", + details=PublicErrorDetails(reason="missing_context.auth_token"), + ) + + assert set(event.keys()) == BASE_FIELDS | {"code", "details"} + assert event["code"] == "missing_runtime_context" + assert event["details"] == {"reason": "missing_context.auth_token"} + + +def test_terminal_error_event_keeps_an_emptied_details_object() -> None: + """A dropped reason still leaves the code, which the client reads.""" + + event = create_terminal_task_error_event( + 1, + "x", + code="missing_runtime_context", + details=PublicErrorDetails(reason="not a listed value"), + ) + + assert event["code"] == "missing_runtime_context" + assert event["details"] == {} + + +class _DuckDetails: + def to_wire(self) -> dict[str, str]: + return {"reason": "not a listed value"} + + +@dataclass(frozen=True) +class _SubclassDetails(PublicErrorDetails): + raw: str = "" + + def to_wire(self) -> dict[str, str]: + # Never reads self.reason, so __post_init__'s whitelist is bypassed. + return {"reason": self.raw} + + +@pytest.mark.parametrize( + "details", + [ + {"reason": "not a listed value"}, + "not a listed value", + _DuckDetails(), + _SubclassDetails(reason=None, raw="not a listed value"), + ], + ids=["dict", "str", "duck-type", "subclass"], +) +def test_public_error_details_is_the_only_accepted_shape(details: Any) -> None: + """The annotation is not the door; the explicit check in the body is. + + The subclass case is why the check reads ``type(...) is`` rather than + ``isinstance``: a frozen dataclass can be subclassed, and a subclass that + overrides ``to_wire`` without reading ``self.reason`` satisfies both mypy + and ``isinstance`` while writing an unlisted string into the frame. + """ + + with pytest.raises(TypeError): + create_terminal_task_error_event(1, "x", code="c", details=details) From 566fcb4dced3c6ad6ae8c323a7699e2de19b29e9 Mon Sep 17 00:00:00 2001 From: Alexliu Date: Fri, 28 Aug 2026 23:20:11 +0800 Subject: [PATCH 03/28] feat(web): classify connector runtime failures at terminal settlement 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. --- src/xagent/web/services/task_orchestrator.py | 35 +++ tests/web/services/test_task_orchestrator.py | 259 ++++++++++++++++++- 2 files changed, 293 insertions(+), 1 deletion(-) diff --git a/src/xagent/web/services/task_orchestrator.py b/src/xagent/web/services/task_orchestrator.py index 85efe0e329..93812c40e9 100644 --- a/src/xagent/web/services/task_orchestrator.py +++ b/src/xagent/web/services/task_orchestrator.py @@ -59,6 +59,7 @@ from ...core.agent.context.execution import CLOCK_TIMEZONE_METADATA_KEY from ...core.execution_scope import resolve_execution_scope from ...core.tools.adapters.vibe.config import RequiredMCPUnavailableError +from ...core.tools.adapters.vibe.connector_runtime import ConnectorRuntimeError from ..models.task import Task, TaskStatus from .assistant_history_safety import ( CLIENT_SAFE_FAILURE_MESSAGE_TYPE, @@ -73,6 +74,9 @@ ) from .client_error_messages import ( CLIENT_SAFE_TASK_FAILURE, + PublicErrorDetails, + connector_runtime_client_message, + connector_runtime_public_error, required_mcp_unavailable_client_message, ) from .db_runtime import ( @@ -1793,6 +1797,8 @@ async def _runner() -> None: client_history_error_message: str | None = None client_history_message_type = TASK_FAILURE_MESSAGE_TYPE broadcast_error_message: str | None = None + broadcast_error_code: str | None = None + broadcast_error_details: PublicErrorDetails | None = None defer_settlement_to_ttl_recovery = False skip_delivery_reconciliation = False # Positive evidence for finalize's delivery target: once @@ -1961,6 +1967,33 @@ async def execute_owned_run() -> None: fallback=CLIENT_SAFE_TASK_FAILURE, ) ) + elif isinstance(setup_or_run_err, ConnectorRuntimeError): + # This exception's message is a curated public-safe + # sentence naming what the connector still needs, so + # the client gets it instead of the opaque fallback. + # ``code`` and the whitelisted ``reason`` ride along on + # the frame; the projector decides what is wire-safe. + settlement_error = str(setup_or_run_err) + client_history_message_type = CLIENT_SAFE_FAILURE_MESSAGE_TYPE + broadcast_error_message = connector_runtime_client_message( + setup_or_run_err + ) + broadcast_error_code, broadcast_error_details = ( + connector_runtime_public_error(setup_or_run_err) + or (None, None) + ) + # Operators read the raw details, not the projection: + # the connector identity is useful here and does not + # leave the server, while the broadcast frame carries + # neither it nor any reason that was filtered out. + logger.error( + "task_id=%s component=connector-runtime code=%s " + "reason=%s connector=%s", + task_id, + setup_or_run_err.code, + setup_or_run_err.details.get("reason"), + setup_or_run_err.details.get("connector_ref"), + ) else: settlement_error = ( "setup/run error: " @@ -2033,6 +2066,8 @@ async def execute_owned_run() -> None: create_terminal_task_error_event( task_id, broadcast_error_message, + code=broadcast_error_code, + details=broadcast_error_details, ), task_id, ) diff --git a/tests/web/services/test_task_orchestrator.py b/tests/web/services/test_task_orchestrator.py index de1e57438c..1d9e527354 100644 --- a/tests/web/services/test_task_orchestrator.py +++ b/tests/web/services/test_task_orchestrator.py @@ -15,6 +15,7 @@ from __future__ import annotations import asyncio +import json import logging from concurrent.futures import ThreadPoolExecutor from contextlib import ExitStack, contextmanager @@ -39,7 +40,10 @@ ) from xagent.core.agent.checkpoint import CHECKPOINT_TYPE from xagent.core.tools.adapters.vibe.config import RequiredMCPUnavailableError -from xagent.core.tools.adapters.vibe.connector_runtime import ConnectorRef +from xagent.core.tools.adapters.vibe.connector_runtime import ( + ConnectorRef, + ConnectorRuntimeError, +) from xagent.web.models import database as database_module from xagent.web.models.agent import Agent from xagent.web.models.chat_message import TaskChatMessage @@ -66,6 +70,10 @@ inspect_user_message_delivery, mark_user_message_delivery, ) +from xagent.web.services.client_error_messages import ( + CLIENT_SAFE_TASK_FAILURE, + PublicErrorDetails, +) from xagent.web.services.connector_runtime import ( get_ephemeral_runtime_values, pop_ephemeral_runtime_values, @@ -3842,3 +3850,252 @@ def test_reconcile_finalized_delivery_noop_on_already_terminal_row( ) assert _delivery_status(db_session, "turn-term") == seeded_status + + +# --------------------------------------------------------------------------- +# Connector-runtime failures reach the client as a structured, wire-safe frame +# --------------------------------------------------------------------------- + + +CONNECTOR_RUNTIME_CODES = [ + "missing_runtime_context", + "runtime_secret_unavailable", + "scheduled_secret_unavailable", +] + + +@contextmanager +def _captured_terminal_broadcast(setup_or_run_error: BaseException, db_session): + """Drive one owned run to failure and hand back the broadcast frames.""" + + from xagent.web.api.websocket import background_task_manager + + user = _create_user(db_session) + task = _create_task(db_session, user.id, status=TaskStatus.RUNNING) + task_id = int(task.id) + lease = TaskLease(task_id=task_id, runner_id="runner-a", run_id="run-a") + frames: list[dict] = [] + + async def broadcast(event, *_args, **_kwargs) -> None: + frames.append(event) + + with ( + patch( + "xagent.web.services.task_orchestrator.acquire_task_lease_isolated", + return_value=lease, + ), + patch( + "xagent.web.services.task_orchestrator.run_task_lease_heartbeat", + new=AsyncMock(), + ), + patch( + "xagent.web.services.task_orchestrator.load_task_setup_snapshot_sync", + return_value=MagicMock(), + ), + patch.object( + task_orchestrator_module, + "resolve_execution_scope", + return_value=None, + create=True, + ), + patch( + "xagent.web.api.websocket.execute_task_background", + new=AsyncMock(side_effect=setup_or_run_error), + ), + patch( + "xagent.web.services.task_orchestrator.settle_task_lease_isolated", + return_value=True, + ), + patch( + "xagent.web.api.websocket.manager", + MagicMock(broadcast_to_task=AsyncMock(side_effect=broadcast)), + ), + patch.object(background_task_manager, "register_task"), + patch( + "xagent.web.services.task_orchestrator._get_agent_manager", + return_value=MagicMock(), + ), + ): + yield task_id, frames + + +async def _run_failing_turn(task_id: int, user_id: int, source) -> None: + await _schedule_bg( + task_id=task_id, + task_owner_user_id=user_id, + task_source=source, + payload=TaskTurnPayload("hello"), + force_fresh=False, + context=None, + ) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("code", CONNECTOR_RUNTIME_CODES) +async def test_connector_runtime_failure_broadcasts_its_safe_message( + db_session, + code: str, +) -> None: + """The curated sentence replaces the opaque task-failure fallback.""" + + safe_message = f"Required connector runtime input is missing ({code})." + error = ConnectorRuntimeError( + code, + safe_message, + details={"reason": "missing_context.auth_token"}, + ) + + with _captured_terminal_broadcast(error, db_session) as (task_id, frames): + task = db_session.query(Task).filter(Task.id == task_id).one() + await _run_failing_turn(task_id, int(task.user_id), task.source) + + assert [frame["message"] for frame in frames] == [safe_message] + assert frames[0]["error"] == safe_message + assert frames[0]["code"] == code + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "error", + [ + ValueError("secret-token-xyz"), + KeyError("secret-token-xyz"), + RuntimeError("secret-token-xyz"), + ], + ids=["value-error", "key-error", "runtime-error"], +) +async def test_incidental_failure_still_redacts( + db_session, + error: BaseException, +) -> None: + """Only the connector-runtime class earns the new branch.""" + + with _captured_terminal_broadcast(error, db_session) as (task_id, frames): + task = db_session.query(Task).filter(Task.id == task_id).one() + await _run_failing_turn(task_id, int(task.user_id), task.source) + + assert len(frames) == 1 + assert frames[0]["message"] == CLIENT_SAFE_TASK_FAILURE + assert frames[0]["error"] == CLIENT_SAFE_TASK_FAILURE + assert "secret-token-xyz" not in json.dumps(frames[0]) + assert "code" not in frames[0] + assert "details" not in frames[0] + + +@pytest.mark.asyncio +async def test_connector_runtime_frame_details_shape(db_session) -> None: + """Whatever the raise site attached, only ``reason`` can reach the wire.""" + + error = ConnectorRuntimeError( + "missing_runtime_context", + "Required connector runtime context is missing.", + details={ + "reason": "missing_context.auth_token", + "internal_sql": "SELECT value FROM task_connector_runtime_contexts", + "raw_value": "tenant-secret", + "connector_ref": {"connector_type": "mcp", "connector_id": 7}, + }, + ) + + with _captured_terminal_broadcast(error, db_session) as (task_id, frames): + task = db_session.query(Task).filter(Task.id == task_id).one() + await _run_failing_turn(task_id, int(task.user_id), task.source) + + assert set(frames[0]["details"]) <= {"reason"} + assert frames[0]["details"] == {"reason": "missing_context.auth_token"} + + +@pytest.mark.asyncio +async def test_connector_runtime_frame_never_carries_connector_ref( + db_session, +) -> None: + """The frame's audience includes anonymous widget and share visitors. + + The three assertions are structural on purpose. An earlier form of this + test also asserted the connector's numeric id was absent from the + serialized frame, which goes red on any fixture where that id collides + with the task id or a timestamp digit. + """ + + error = ConnectorRuntimeError( + "missing_runtime_context", + "Required connector runtime context is missing.", + connector_ref=ConnectorRef(connector_type="mcp", connector_id=7), + details={"reason": "missing_context.auth_token"}, + ) + + with _captured_terminal_broadcast(error, db_session) as (task_id, frames): + task = db_session.query(Task).filter(Task.id == task_id).one() + await _run_failing_turn(task_id, int(task.user_id), task.source) + + frame = frames[0] + assert set(frame) == { + "type", + "message", + "task_id", + "task", + "error", + "timestamp", + "code", + "details", + } + assert set(frame["details"]) <= {"reason"} + serialized = json.dumps(frame) + assert "connector_ref" not in serialized + assert "connector_id" not in serialized + + +@pytest.mark.asyncio +async def test_connector_runtime_frame_reason_matches_direct_construction( + db_session, +) -> None: + """End to end, the frame carries exactly what the type would produce.""" + + error = ConnectorRuntimeError( + "missing_runtime_context", + "Required connector runtime context is missing.", + details={"reason": "missing_context.auth_token"}, + ) + + with _captured_terminal_broadcast(error, db_session) as (task_id, frames): + task = db_session.query(Task).filter(Task.id == task_id).one() + await _run_failing_turn(task_id, int(task.user_id), task.source) + + assert ( + frames[0]["details"] + == PublicErrorDetails(reason="missing_context.auth_token").to_wire() + ) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("code", CONNECTOR_RUNTIME_CODES) +async def test_connector_runtime_failure_logs_missing_key( + db_session, + caplog, + code: str, +) -> None: + """Operators read the raw details, connector identity included.""" + + error = ConnectorRuntimeError( + code, + "Required connector runtime context is missing.", + connector_ref=ConnectorRef(connector_type="mcp", connector_id=7), + details={"reason": "missing_context.auth_token"}, + ) + + with caplog.at_level(logging.ERROR): + with _captured_terminal_broadcast(error, db_session) as (task_id, frames): + task = db_session.query(Task).filter(Task.id == task_id).one() + await _run_failing_turn(task_id, int(task.user_id), task.source) + + structured = [ + record.getMessage() + for record in caplog.records + if record.levelno == logging.ERROR + and "component=connector-runtime" in record.getMessage() + ] + assert len(structured) == 1 + assert f"code={code}" in structured[0] + assert "reason=missing_context.auth_token" in structured[0] + assert "connector=" in structured[0] + assert "'connector_id': 7" in structured[0] From 58593b71637aaf7f28fe84b5cdd6136e71457444 Mon Sep 17 00:00:00 2001 From: Alexliu Date: Fri, 28 Aug 2026 23:20:30 +0800 Subject: [PATCH 04/28] fix(frontend): render the terminal error bubble and name the missing key 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. --- .../src/contexts/app-context-chat.test.tsx | 251 ++++++++++++++++++ frontend/src/contexts/app-context-chat.tsx | 96 ++++++- frontend/src/i18n/locales/en.ts | 2 + frontend/src/i18n/locales/zh.ts | 2 + 4 files changed, 350 insertions(+), 1 deletion(-) diff --git a/frontend/src/contexts/app-context-chat.test.tsx b/frontend/src/contexts/app-context-chat.test.tsx index cd7c815999..b699d2e4f8 100644 --- a/frontend/src/contexts/app-context-chat.test.tsx +++ b/frontend/src/contexts/app-context-chat.test.tsx @@ -5624,3 +5624,254 @@ describe("AppProvider websocket message routing", () => { expect(defaultLink).not.toHaveAttribute("rel") }) }) + +function ConnectorRuntimeErrorProbe() { + const { state } = useApp() + return ( +
+ {JSON.stringify(state.lastConnectorRuntimeError)} +
+ ) +} + +describe("terminal error frames", () => { + // Same reset as the routing suite above: the websocket harness ref and the + // duplicate-message cache both outlive a single render, so without this the + // second test in this block would keep talking to the first one's provider. + beforeEach(() => { + webSocketOptions.current = null + webSocketOptions.all = [] + sessionControls = null + wsHarness.isConnected = true + apiRequestMock.mockReset() + routerPushMock.mockReset() + sendRawMessageMock.mockReset() + sendRawMessageMock.mockReturnValue("sent") + sendChatMessageMock.mockReset() + sendChatMessageMock.mockResolvedValue({ + client_message_id: "turn-optimistic", + turn_id: "turn-optimistic", + }) + localStorage.clear() + ;(window as typeof window & { clearDuplicateMessageCache?: () => void }) + .clearDuplicateMessageCache?.() + }) + + afterEach(() => { + cleanup() + localStorage.clear() + }) + + // The conversation panel renders only user / isResult / system-notice + // messages. Without the flag the bubble is filtered out and the UI falls + // back to a generic "unknown error" placeholder until the page reloads. + it.each(["error", "task_error"])( + "flags the %s bubble as the turn's result", + async (frameType) => { + render( + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + act(() => { + onMessage?.({ + type: frameType, + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + task: { id: 1, status: "failed" }, + message: "Task execution failed.", + error: "Task execution failed.", + } as TestWebSocketMessage) + }) + + await waitFor(() => { + expect(screen.getByTestId("messages").textContent).toContain( + "Task execution failed." + ) + }) + + const messages = JSON.parse( + screen.getByTestId("messages").textContent || "[]" + ) + const bubble = messages.find((m: { content: string }) => + m.content.includes("Task execution failed.") + ) + expect(bubble?.isResult).toBe(true) + } + ) + + it("names the missing connector key instead of relaying the server sentence", async () => { + render( + + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + act(() => { + onMessage?.({ + type: "task_error", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + task: { id: 1, status: "failed" }, + message: "Required connector runtime context is missing.", + error: "Required connector runtime context is missing.", + code: "missing_runtime_context", + details: { reason: "missing_context.auth_token" }, + } as TestWebSocketMessage) + }) + + // The i18n mock in this file returns the key and drops the variables, so + // the assertion is on which wording was chosen, not on the rendered key + // name. The reason the key name is parsed from is asserted below. + await waitFor(() => { + expect(screen.getByTestId("messages").textContent).toContain( + "common.errors.connectorRuntimeMissingKey" + ) + }) + + const messages = JSON.parse(screen.getByTestId("messages").textContent || "[]") + const bubble = messages.find((m: { content: string }) => + m.content.includes("common.errors.connectorRuntimeMissingKey") + ) + expect(bubble?.isResult).toBe(true) + // No prefix: this wording replaces the server sentence rather than + // decorating it. + expect(bubble?.content).toBe("common.errors.connectorRuntimeMissingKey") + expect(JSON.parse(screen.getByTestId("connector-runtime-error").textContent || "null")).toEqual({ + code: "missing_runtime_context", + details: { reason: "missing_context.auth_token" }, + }) + }) + + // A listed reason that is a bare enum value names no key, so the keyless + // wording is chosen even though the code is a missing-value one. + it("falls back to the generic wording when the reason names no key", async () => { + render( + + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + act(() => { + onMessage?.({ + type: "task_error", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + task: { id: 1, status: "failed" }, + message: "Connector secrets are unavailable.", + error: "Connector secrets are unavailable.", + code: "runtime_secret_unavailable", + details: { reason: "not_provided" }, + } as TestWebSocketMessage) + }) + + await waitFor(() => { + expect(screen.getByTestId("messages").textContent).toContain( + "common.errors.connectorRuntimeMissing" + ) + }) + + expect(screen.getByTestId("messages").textContent).not.toContain( + "common.errors.connectorRuntimeMissingKey" + ) + }) + + // The server drops a reason it cannot place on its whitelist, so the + // keyless wording has to exist and the frame still carries the code. + it("falls back to the generic wording when the reason was dropped", async () => { + render( + + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + act(() => { + onMessage?.({ + type: "task_error", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + task: { id: 1, status: "failed" }, + message: "Required connector runtime context is missing.", + error: "Required connector runtime context is missing.", + code: "missing_runtime_context", + details: {}, + } as TestWebSocketMessage) + }) + + await waitFor(() => { + expect(screen.getByTestId("messages").textContent).toContain( + "common.errors.connectorRuntimeMissing" + ) + }) + + expect(screen.getByTestId("messages").textContent).not.toContain( + "common.errors.connectorRuntimeMissingKey" + ) + expect(JSON.parse(screen.getByTestId("connector-runtime-error").textContent || "null")).toEqual({ + code: "missing_runtime_context", + details: {}, + }) + }) + + // connector_runtime_unavailable reports a server-side component being + // down, which the user cannot act on, so it keeps the generic prefix. + it("keeps the plain failure wording for a non-missing-value code", async () => { + render( + + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + act(() => { + onMessage?.({ + type: "task_error", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + task: { id: 1, status: "failed" }, + message: "Connector runtime is unavailable.", + error: "Connector runtime is unavailable.", + code: "connector_runtime_unavailable", + details: { reason: "team_env_resolution_failed" }, + } as TestWebSocketMessage) + }) + + await waitFor(() => { + expect(screen.getByTestId("messages").textContent).toContain( + "Connector runtime is unavailable." + ) + }) + + expect(screen.getByTestId("messages").textContent).not.toContain( + "common.errors.connectorRuntimeMissing" + ) + expect(JSON.parse(screen.getByTestId("connector-runtime-error").textContent || "null")).toEqual({ + code: "connector_runtime_unavailable", + details: { reason: "team_env_resolution_failed" }, + }) + }) +}) diff --git a/frontend/src/contexts/app-context-chat.tsx b/frontend/src/contexts/app-context-chat.tsx index ed1acae4fe..840530ad6f 100644 --- a/frontend/src/contexts/app-context-chat.tsx +++ b/frontend/src/contexts/app-context-chat.tsx @@ -44,6 +44,15 @@ type TaskControlState = | "completed" | "failed" +// The structured half of a terminal task_error frame. ``details`` holds at +// most ``reason``: the server projects the exception through a whitelist +// before broadcasting, because this frame reaches every connection on the +// task, anonymous widget and share-link visitors included. +type ConnectorRuntimeErrorState = { + code: string + details: { reason?: string } +} + type TaskControlEnvelope = { isStateEvent: boolean taskId?: number @@ -94,6 +103,7 @@ const TASK_SCOPED_ACTION_TYPES = new Set([ "UPSERT_STREAMING_FINAL_ANSWER", "ADD_TRACE_EVENT", "SET_CONTEXT_USAGE", + "SET_CONNECTOR_RUNTIME_ERROR", "SET_PLAN_MEMORY_INFO", "OPEN_FILE_PREVIEW", ]) @@ -918,6 +928,47 @@ const getWebSocketErrorCode = (message: WebSocketMessage) => { return errorCode.present ? readClientErrorCode(errorCode.value) : null } +// The task_error codes that mean a connector is still missing a runtime value +// the user can supply. The other connector-runtime code +// (connector_runtime_unavailable) reports a server-side component being down, +// which the user cannot act on, so it keeps the generic failure wording. +const CONNECTOR_RUNTIME_MISSING_VALUE_CODES = new Set([ + "missing_runtime_context", + "runtime_secret_unavailable", + "scheduled_secret_unavailable", +]) + +// The frame deliberately carries no connector identity: its audience includes +// anonymous widget and share-link visitors. The key name below is the only +// connector-specific thing available here; anything more (which connector, the +// declared type of each key) comes from the per-task requirements endpoint, +// which is owner-only. +const getConnectorRuntimeError = ( + message: WebSocketMessage, +): ConnectorRuntimeErrorState | null => { + const root = message as unknown as Record + const data = isJsonRecord(message.data) ? message.data : null + const code = getString(data?.code) || getString(root.code) + if (!code) return null + const details = isJsonRecord(data?.details) + ? data.details + : isJsonRecord(root.details) + ? root.details + : null + const reason = getString(details?.reason) + return { code, details: reason ? { reason } : {} } +} + +// A reason is either a bare enum value or ".". Only +// the second form names a key the user has to fill in. +const missingRuntimeKeyFromReason = (reason: string | undefined): string | null => { + if (!reason) return null + const separator = reason.lastIndexOf(".") + if (separator < 0) return null + const key = reason.slice(separator + 1) + return key || null +} + const getWebSocketTaskStatus = (message: WebSocketMessage): Task["status"] | null => { const root = message as unknown as Record const data = isJsonRecord(message.data) ? message.data : null @@ -1073,6 +1124,11 @@ export interface AppState { isHistoryLoading: boolean // Current context-window usage from the latest LLM call, for the usage gauge. contextUsage: { tokens: number; threshold: number } | null + // The structured half of the last terminal connector-runtime failure on the + // viewed task. It holds only what the frame is allowed to carry; the + // connector identity and the declared key types come from the per-task + // requirements endpoint instead. + lastConnectorRuntimeError: ConnectorRuntimeErrorState | null sessionConversation: SessionConversationState } @@ -1090,6 +1146,7 @@ type AppAction = | { type: "SET_DAG_EXECUTION"; payload: DAGExecution | null } | { type: "RESET_DAG_STATE" } | { type: "SET_CONTEXT_USAGE"; payload: { tokens: number; threshold: number } | null } + | { type: "SET_CONNECTOR_RUNTIME_ERROR"; payload: ConnectorRuntimeErrorState | null } | { type: "ADD_STEP"; payload: StepExecution } | { type: "UPDATE_STEP"; payload: { stepId: string; updates: Partial } } | { type: "SET_STEPS"; payload: StepExecution[] } @@ -1157,6 +1214,7 @@ const createInitialState = (): AppState => ({ lastTaskUpdate: Date.now(), isHistoryLoading: false, contextUsage: null, + lastConnectorRuntimeError: null, sessionConversation: { ...initialSessionConversationState }, }) @@ -1503,6 +1561,9 @@ function projectAppState(state: AppState, action: AppAction): AppState { case "SET_CONTEXT_USAGE": return { ...state, contextUsage: action.payload } + case "SET_CONNECTOR_RUNTIME_ERROR": + return { ...state, lastConnectorRuntimeError: action.payload } + case "ADD_STEP": const newStep = action.payload const existingStepIndex = state.steps.findIndex(s => s.id === newStep.id) @@ -5662,6 +5723,7 @@ export function AppProvider({ ? t(clientErrorTranslationKey(websocketErrorCode)) : getWebSocketErrorMessage(message, trustLegacyErrorProse) const websocketTaskStatus = getWebSocketTaskStatus(message) + const connectorRuntimeError = getConnectorRuntimeError(message) if (websocketTaskStatus) { dispatch({ type: "UPDATE_TASK_STATUS", payload: { status: websocketTaskStatus } }) @@ -5670,16 +5732,48 @@ export function AppProvider({ if (shouldStopProcessingForTaskStatus(websocketTaskStatus)) { dispatch({ type: "SET_PROCESSING", payload: false }) } + if (connectorRuntimeError) { + dispatch({ + type: "SET_CONNECTOR_RUNTIME_ERROR", + payload: connectorRuntimeError, + }) + } + + // A missing runtime value is the one failure here the user can fix, + // so name the key instead of relaying the server's sentence. The + // reason is dropped by the server whitelist whenever it is not a + // listed value, which is why the keyless wording has to exist. + let connectorRuntimeBubble: string | null = null + if ( + connectorRuntimeError + && CONNECTOR_RUNTIME_MISSING_VALUE_CODES.has(connectorRuntimeError.code) + ) { + const missingKey = missingRuntimeKeyFromReason( + connectorRuntimeError.details.reason + ) + connectorRuntimeBubble = missingKey + ? t('common.errors.connectorRuntimeMissingKey', { key: missingKey }) + : t('common.errors.connectorRuntimeMissing') + } + const errorBubbleContent = connectorRuntimeBubble + ?? `${t('agent.logs.event.messages.errorPrefix')} ${websocketErrorMessage}` + // The dedup key stays the server's own message: it identifies the + // failure, and the rendered wording above is derived from it. if (!isDuplicateMessageForViewedTask(websocketErrorMessage, "agent-error")) { dispatch({ type: "ADD_MESSAGE", payload: { id: generateMessageId("msg-error"), role: "assistant", - content: `${t('agent.logs.event.messages.errorPrefix')} ${websocketErrorMessage}`, + content: errorBubbleContent, timestamp: message.timestamp, status: "failed", + // Terminal failure IS this turn's result. Without the flag the + // conversation panel (which only shows user / isResult / + // system-notice messages) filters the bubble out and falls back + // to a virtual "unknown error" placeholder until reload. + isResult: true, }, }) } diff --git a/frontend/src/i18n/locales/en.ts b/frontend/src/i18n/locales/en.ts index 1c2fd75c69..7a47ff3b75 100644 --- a/frontend/src/i18n/locales/en.ts +++ b/frontend/src/i18n/locales/en.ts @@ -66,6 +66,8 @@ const en = { errors: { unknown: "Unknown error", taskFailed: "Something went wrong. Please try again.", + connectorRuntimeMissingKey: "This connector still needs a value for \"{key}\".", + connectorRuntimeMissing: "This connector needs additional runtime input before it can run.", }, }, voiceInput: { diff --git a/frontend/src/i18n/locales/zh.ts b/frontend/src/i18n/locales/zh.ts index 469a58a5d4..c977a9b5a8 100644 --- a/frontend/src/i18n/locales/zh.ts +++ b/frontend/src/i18n/locales/zh.ts @@ -66,6 +66,8 @@ const zh = { errors: { unknown: "未知错误", taskFailed: "出了点问题，请重试。", + connectorRuntimeMissingKey: "这个连接器还需要你提供 “{key}”。", + connectorRuntimeMissing: "这个连接器需要额外的运行时输入，请补充后重试。", }, }, voiceInput: { From e65fabba9abbd27a9dba3355025608fd58f7dcf9 Mon Sep 17 00:00:00 2001 From: Alexliu Date: Fri, 28 Aug 2026 23:42:25 +0800 Subject: [PATCH 05/28] refactor(web): tie each public reason to its own raise site 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. --- .../web/services/client_error_messages.py | 25 +++++++++++------- .../services/test_client_error_messages.py | 26 ++++++++----------- 2 files changed, 26 insertions(+), 25 deletions(-) diff --git a/src/xagent/web/services/client_error_messages.py b/src/xagent/web/services/client_error_messages.py index 0826566b70..c3b3e67b86 100644 --- a/src/xagent/web/services/client_error_messages.py +++ b/src/xagent/web/services/client_error_messages.py @@ -1,11 +1,13 @@ """Fixed client-visible fallbacks for incidental server failures.""" -import re from dataclasses import dataclass from enum import StrEnum from ...core.tools.adapters.vibe.config import RequiredMCPUnavailableError -from ...core.tools.adapters.vibe.connector_runtime import ConnectorRuntimeError +from ...core.tools.adapters.vibe.connector_runtime import ( + RUNTIME_SOURCE_KEY_RE, + ConnectorRuntimeError, +) CLIENT_SAFE_VALIDATION_ERROR = "The message could not be processed. Please try again." @@ -146,9 +148,6 @@ def connector_runtime_client_message( "undeclared_context_key", "undeclared_secrets_key", "undeclared_auth_selector_key", - # Raised by the runtime value-fill boundary. - "payload_too_large", - "encryption_unavailable", # Fixed 503 strings built by direct ConnectorRuntimeError construction # in three other modules. Each one states that a server-side component # is unavailable; none of them states who owns the task, or how an @@ -161,6 +160,11 @@ def connector_runtime_client_message( "custom_api_config_load_failed", } ) +# Every member above is raised somewhere in this repository today, and a test +# asserts that in both directions. Add a reason here in the same change that +# adds the site raising it, never ahead of it: a listed reason nothing produces +# is an allowance with no expiry date, and by the time the raising code arrives +# nobody remembers which audience the reason was judged against. CONNECTOR_RUNTIME_PUBLIC_REASON_PREFIXES = frozenset( { "missing_context", @@ -172,13 +176,14 @@ def connector_runtime_client_message( "conflict.auth_selector", } ) -# The declared runtime key grammar, reused verbatim from -# core/tools/adapters/vibe/connector_runtime.py:141-144. -_RUNTIME_KEY_RE = re.compile(r"^[A-Za-z0-9_-]+$") def _is_public_reason(reason: object) -> bool: - """True when this reason may reach a client. Used by PublicErrorDetails.""" + """True when this reason may reach a client. Used by PublicErrorDetails. + + The key half of a prefixed reason is matched against the declared runtime + key grammar itself, not a copy of it, so the two cannot drift apart. + """ if not isinstance(reason, str): return False @@ -189,7 +194,7 @@ def _is_public_reason(reason: object) -> bool: return False if prefix not in CONNECTOR_RUNTIME_PUBLIC_REASON_PREFIXES: return False - return _RUNTIME_KEY_RE.fullmatch(key) is not None + return RUNTIME_SOURCE_KEY_RE.fullmatch(key) is not None @dataclass(frozen=True) diff --git a/tests/web/services/test_client_error_messages.py b/tests/web/services/test_client_error_messages.py index 8ef63f877a..a29149ba68 100644 --- a/tests/web/services/test_client_error_messages.py +++ b/tests/web/services/test_client_error_messages.py @@ -106,10 +106,11 @@ def test_public_error_details_normalizes_reason(reason: object) -> None: LEGAL_REASONS = [ "not_provided", "store_lost", - "payload_too_large", - "encryption_unavailable", + "connector_not_selected", + "undeclared_context_key", "team_env_resolution_failed", "team_scope_resolution_failed", + "runtime_view_resolution_failed", "custom_api_config_load_failed", "missing_context.auth_token", "type_mismatch.context.tenant_id", @@ -256,17 +257,6 @@ def test_public_error_details_is_constructed_in_one_module_only() -> None: # -------------------------------------------------------------------------- -# Reasons that are listed but have no literal raise site in this repository -# today. Both belong to the runtime value-fill endpoint, which is added by a -# later PR in this series; they are listed now because the whitelist is the -# contract that endpoint is written against. -REASONS_WITHOUT_A_LITERAL_RAISE_SITE = frozenset( - { - "payload_too_large", - "encryption_unavailable", - } -) - # Literal reasons that the derivation below finds and that are deliberately # kept off the wire. The first two state the task's ownership and the outcome # of an authorization check; the audience of this frame includes anonymous @@ -421,7 +411,14 @@ def test_public_reason_whitelist_covers_every_raise_site() -> None: def test_public_reason_whitelist_has_no_member_without_a_raise_site() -> None: - """The whitelist must not grow entries nothing can actually produce.""" + """Every listed reason is produced somewhere, with no exemptions. + + Zero exemptions is the point of this assertion. A listed reason nothing + raises is a standing allowance with no expiry, and by the time the code + raising it arrives nobody remembers which audience it was judged against. + A reason therefore enters the whitelist in the same change as the site + that raises it. + """ literals, patterns = _derive_reasons() compiled = [re.compile(pattern) for pattern in patterns] @@ -430,7 +427,6 @@ def test_public_reason_whitelist_has_no_member_without_a_raise_site() -> None: reason for reason in CONNECTOR_RUNTIME_PUBLIC_REASONS if reason not in literals - and reason not in REASONS_WITHOUT_A_LITERAL_RAISE_SITE and not any(expression.match(reason) for expression in compiled) } assert not ungrounded, ( From d908ebd5240ef1824bc84b1cfc1d52d349d2dcc7 Mon Sep 17 00:00:00 2001 From: Alexliu Date: Fri, 28 Aug 2026 23:47:58 +0800 Subject: [PATCH 06/28] docs(web): describe what client_error_messages now holds 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. --- src/xagent/web/services/client_error_messages.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/xagent/web/services/client_error_messages.py b/src/xagent/web/services/client_error_messages.py index c3b3e67b86..83a558448c 100644 --- a/src/xagent/web/services/client_error_messages.py +++ b/src/xagent/web/services/client_error_messages.py @@ -1,4 +1,10 @@ -"""Fixed client-visible fallbacks for incidental server failures.""" +"""Client-visible projections of server-side failures. + +Holds the fixed fallback strings used when a failure has nothing safe to +say, the per-exception adapters that pass a curated message through, and +``PublicErrorDetails`` -- the only structured payload allowed onto a +task_error frame, together with the reason allowlist that governs it. +""" from dataclasses import dataclass from enum import StrEnum From 230a03230b83a928919282147c411e82b860605d Mon Sep 17 00:00:00 2001 From: Alexliu Date: Sat, 29 Aug 2026 00:43:30 +0800 Subject: [PATCH 07/28] fix(web): degrade instead of raising on the terminal error path 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. --- src/xagent/web/api/websocket.py | 56 ++++++++- .../web/api/test_terminal_task_error_event.py | 96 +++++++++++++- tests/web/services/test_task_orchestrator.py | 119 ++++++++++++++++-- 3 files changed, 252 insertions(+), 19 deletions(-) diff --git a/src/xagent/web/api/websocket.py b/src/xagent/web/api/websocket.py index 59f43a170b..72dcc1c8ad 100644 --- a/src/xagent/web/api/websocket.py +++ b/src/xagent/web/api/websocket.py @@ -13,6 +13,7 @@ from copy import deepcopy from dataclasses import dataclass from datetime import datetime, timedelta, timezone +from functools import lru_cache from pathlib import Path from typing import ( TYPE_CHECKING, @@ -321,6 +322,20 @@ def _task_error_payload( return payload +@lru_cache(maxsize=1) +def _client_visible_error_codes() -> frozenset[str]: + """The closed set of client-visible error codes, reused not recopied. + + Imported inside the function on purpose: the ``v1`` package's ``__init__`` + pulls in routers that import this module, so a module-level import would + close a cycle. The set is built once and cached. + """ + + from .v1.errors import V1ErrorCode + + return frozenset(member.value for member in V1ErrorCode) + + def create_terminal_task_error_event( task_id: int, message: str, @@ -330,11 +345,20 @@ def create_terminal_task_error_event( ) -> dict[str, Any]: """Shape an error event after the exact lease owner commits FAILED. - ``code`` and ``details`` are written only when both are supplied, so a - caller that passes neither still gets the same six-key frame. ``details`` - is accepted as ``PublicErrorDetails`` itself and nothing else -- not a - subclass -- because that class's ``__post_init__`` is where the reason - whitelist lives. + ``code`` and ``details`` are written only when both survive validation, so + a caller that passes neither still gets the same six-key frame, and a + caller that passes something unusable gets that same frame rather than an + exception. This runs on the reporting path of an already-failed task, and + the one call site that passes these arguments evaluates them inside the + ``except Exception`` that only logs a failed broadcast -- so raising here + would cost the terminal frame outright and leave the user on the silent + failure this path exists to remove. A bad optional argument costs that + argument and nothing else. Both rejections are logged with their stack. + + ``details`` is accepted as ``PublicErrorDetails`` itself and nothing else + -- not a subclass -- because that class's ``__post_init__`` is where the + reason whitelist lives. ``code`` must be a member of ``V1ErrorCode``, the + repository's closed set of client-visible error codes. """ # Python annotations are not enforced at run time, so the mypy gate on the @@ -348,7 +372,27 @@ def create_terminal_task_error_event( # both isinstance and mypy while bypassing the whitelist in __post_init__. # Only the class itself carries that guarantee. if details is not None and type(details) is not PublicErrorDetails: - raise TypeError("details must be a PublicErrorDetails") + logger.error( + "task_id=%s component=terminal-error-frame dropped=details " + "type=%s; the frame is still sent without it", + task_id, + type(details).__name__, + stack_info=True, + ) + details = None + + # ConnectorRuntimeError types its code as a bare str and stores it + # unvalidated, so "only the ten module constants reach here" is a fact + # about today's raise sites, not a property the code holds. + if code is not None and code not in _client_visible_error_codes(): + logger.error( + "task_id=%s component=terminal-error-frame dropped=code " + "value=%r; the frame is still sent without it", + task_id, + code, + stack_info=True, + ) + code = None event: dict[str, Any] = { "type": "task_error", diff --git a/tests/web/api/test_terminal_task_error_event.py b/tests/web/api/test_terminal_task_error_event.py index 811957f688..4759298250 100644 --- a/tests/web/api/test_terminal_task_error_event.py +++ b/tests/web/api/test_terminal_task_error_event.py @@ -8,12 +8,17 @@ from __future__ import annotations +import json +import logging from dataclasses import dataclass from typing import Any import pytest -from xagent.web.api.websocket import create_terminal_task_error_event +from xagent.web.api.websocket import ( + _client_visible_error_codes, + create_terminal_task_error_event, +) from xagent.web.services.client_error_messages import PublicErrorDetails BASE_FIELDS = {"type", "message", "task_id", "task", "error", "timestamp"} @@ -87,14 +92,97 @@ def to_wire(self) -> dict[str, str]: ], ids=["dict", "str", "duck-type", "subclass"], ) -def test_public_error_details_is_the_only_accepted_shape(details: Any) -> None: +def test_public_error_details_is_the_only_accepted_shape( + details: Any, + caplog: pytest.LogCaptureFixture, +) -> None: """The annotation is not the door; the explicit check in the body is. The subclass case is why the check reads ``type(...) is`` rather than ``isinstance``: a frozen dataclass can be subclassed, and a subclass that overrides ``to_wire`` without reading ``self.reason`` satisfies both mypy and ``isinstance`` while writing an unlisted string into the frame. + + Rejection drops the argument, it does not raise. The frame is the last + thing between the user and a silent failure, and the one caller that + passes these arguments builds the frame inside an ``except Exception`` + that only logs -- so an exception here would cost the whole frame. """ - with pytest.raises(TypeError): - create_terminal_task_error_event(1, "x", code="c", details=details) + with caplog.at_level(logging.ERROR): + event = create_terminal_task_error_event( + 1, "x", code="missing_runtime_context", details=details + ) + + # The unlisted string the bad shape wanted to smuggle in never appears. + assert set(event.keys()) == BASE_FIELDS + assert "not a listed value" not in json.dumps(event) + + dropped = [ + record.getMessage() + for record in caplog.records + if record.levelno == logging.ERROR and "dropped=details" in record.getMessage() + ] + assert len(dropped) == 1 + assert type(details).__name__ in dropped[0] + + +def test_unknown_code_is_dropped_and_logged( + caplog: pytest.LogCaptureFixture, +) -> None: + """``code`` passes the same closed set the /v1 surface pins against. + + ``ConnectorRuntimeError`` types its code as a bare ``str`` and stores it + without validation, so an unlisted value reaching the wire is a question + of what raise sites happen to exist today, not of what the code enforces. + """ + + with caplog.at_level(logging.ERROR): + event = create_terminal_task_error_event( + 1, + "x", + code="not_a_listed_code", + details=PublicErrorDetails(reason="not_provided"), + ) + + assert set(event.keys()) == BASE_FIELDS + assert "not_a_listed_code" not in json.dumps(event) + + dropped = [ + record.getMessage() + for record in caplog.records + if record.levelno == logging.ERROR and "dropped=code" in record.getMessage() + ] + assert len(dropped) == 1 + assert "not_a_listed_code" in dropped[0] + + +@pytest.mark.parametrize( + "code", + [ + "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", + ], +) +def test_every_connector_runtime_code_survives_the_closed_set(code: str) -> None: + """All ten connector-runtime codes are members, so none is dropped.""" + + event = create_terminal_task_error_event( + 1, "x", code=code, details=PublicErrorDetails(reason="not_provided") + ) + + assert event["code"] == code + + +def test_the_closed_set_is_the_v1_one_not_a_copy() -> None: + from xagent.web.api.v1.errors import V1ErrorCode + + assert _client_visible_error_codes() == {member.value for member in V1ErrorCode} diff --git a/tests/web/services/test_task_orchestrator.py b/tests/web/services/test_task_orchestrator.py index 1d9e527354..e45fcf98bc 100644 --- a/tests/web/services/test_task_orchestrator.py +++ b/tests/web/services/test_task_orchestrator.py @@ -60,6 +60,7 @@ from xagent.web.services import task_orchestrator as task_orchestrator_module from xagent.web.services.assistant_history_safety import ( CLIENT_SAFE_FAILURE_MESSAGE_TYPE, + TASK_FAILURE_MESSAGE_TYPE, ) from xagent.web.services.chat_history_service import ( DELIVERY_COMPLETED, @@ -3866,7 +3867,13 @@ def test_reconcile_finalized_delivery_noop_on_already_terminal_row( @contextmanager def _captured_terminal_broadcast(setup_or_run_error: BaseException, db_session): - """Drive one owned run to failure and hand back the broadcast frames.""" + """Drive one owned run to failure and hand back both halves it produced. + + The branch under test writes two things: the broadcast frame the live + client renders, and the durable settlement the transcript replays after a + reload. Capturing only the frame would let the durable half be deleted + with every test still green, so the settlement kwargs come back too. + """ from xagent.web.api.websocket import background_task_manager @@ -3875,10 +3882,15 @@ def _captured_terminal_broadcast(setup_or_run_error: BaseException, db_session): task_id = int(task.id) lease = TaskLease(task_id=task_id, runner_id="runner-a", run_id="run-a") frames: list[dict] = [] + settlements: list[dict] = [] async def broadcast(event, *_args, **_kwargs) -> None: frames.append(event) + def settle(*_args, **kwargs) -> bool: + settlements.append(kwargs) + return True + with ( patch( "xagent.web.services.task_orchestrator.acquire_task_lease_isolated", @@ -3904,7 +3916,7 @@ async def broadcast(event, *_args, **_kwargs) -> None: ), patch( "xagent.web.services.task_orchestrator.settle_task_lease_isolated", - return_value=True, + side_effect=settle, ), patch( "xagent.web.api.websocket.manager", @@ -3916,7 +3928,7 @@ async def broadcast(event, *_args, **_kwargs) -> None: return_value=MagicMock(), ), ): - yield task_id, frames + yield task_id, frames, settlements async def _run_failing_turn(task_id: int, user_id: int, source) -> None: @@ -3945,7 +3957,11 @@ async def test_connector_runtime_failure_broadcasts_its_safe_message( details={"reason": "missing_context.auth_token"}, ) - with _captured_terminal_broadcast(error, db_session) as (task_id, frames): + with _captured_terminal_broadcast(error, db_session) as ( + task_id, + frames, + settlements, + ): task = db_session.query(Task).filter(Task.id == task_id).one() await _run_failing_turn(task_id, int(task.user_id), task.source) @@ -3970,7 +3986,11 @@ async def test_incidental_failure_still_redacts( ) -> None: """Only the connector-runtime class earns the new branch.""" - with _captured_terminal_broadcast(error, db_session) as (task_id, frames): + with _captured_terminal_broadcast(error, db_session) as ( + task_id, + frames, + settlements, + ): task = db_session.query(Task).filter(Task.id == task_id).one() await _run_failing_turn(task_id, int(task.user_id), task.source) @@ -3997,7 +4017,11 @@ async def test_connector_runtime_frame_details_shape(db_session) -> None: }, ) - with _captured_terminal_broadcast(error, db_session) as (task_id, frames): + with _captured_terminal_broadcast(error, db_session) as ( + task_id, + frames, + settlements, + ): task = db_session.query(Task).filter(Task.id == task_id).one() await _run_failing_turn(task_id, int(task.user_id), task.source) @@ -4024,7 +4048,11 @@ async def test_connector_runtime_frame_never_carries_connector_ref( details={"reason": "missing_context.auth_token"}, ) - with _captured_terminal_broadcast(error, db_session) as (task_id, frames): + with _captured_terminal_broadcast(error, db_session) as ( + task_id, + frames, + settlements, + ): task = db_session.query(Task).filter(Task.id == task_id).one() await _run_failing_turn(task_id, int(task.user_id), task.source) @@ -4057,7 +4085,11 @@ async def test_connector_runtime_frame_reason_matches_direct_construction( details={"reason": "missing_context.auth_token"}, ) - with _captured_terminal_broadcast(error, db_session) as (task_id, frames): + with _captured_terminal_broadcast(error, db_session) as ( + task_id, + frames, + settlements, + ): task = db_session.query(Task).filter(Task.id == task_id).one() await _run_failing_turn(task_id, int(task.user_id), task.source) @@ -4084,7 +4116,11 @@ async def test_connector_runtime_failure_logs_missing_key( ) with caplog.at_level(logging.ERROR): - with _captured_terminal_broadcast(error, db_session) as (task_id, frames): + with _captured_terminal_broadcast(error, db_session) as ( + task_id, + frames, + settlements, + ): task = db_session.query(Task).filter(Task.id == task_id).one() await _run_failing_turn(task_id, int(task.user_id), task.source) @@ -4099,3 +4135,68 @@ async def test_connector_runtime_failure_logs_missing_key( assert "reason=missing_context.auth_token" in structured[0] assert "connector=" in structured[0] assert "'connector_id': 7" in structured[0] + + +@pytest.mark.asyncio +@pytest.mark.parametrize("code", CONNECTOR_RUNTIME_CODES) +async def test_connector_runtime_failure_persists_client_safe_history( + db_session, + code: str, +) -> None: + """The durable half: what the transcript replays after a reload. + + The new branch writes three things -- the frame, the settlement error and + the history message type. Without this test the whole + ``client_history_message_type`` line could be deleted and every other test + in this file would stay green, while a reloading user dropped back to the + generic failure text the frame no longer shows. + """ + + safe_message = f"Required connector runtime input is missing ({code})." + error = ConnectorRuntimeError( + code, + safe_message, + details={"reason": "missing_context.auth_token"}, + ) + + with _captured_terminal_broadcast(error, db_session) as ( + task_id, + frames, + settlements, + ): + task = db_session.query(Task).filter(Task.id == task_id).one() + await _run_failing_turn(task_id, int(task.user_id), task.source) + + assert len(settlements) == 1 + settled = settlements[0] + assert settled["client_message_type"] == CLIENT_SAFE_FAILURE_MESSAGE_TYPE + # The reloaded transcript says the same thing the live bubble said. + assert settled["client_error_message"] == safe_message + assert settled["client_error_message"] == frames[0]["message"] + # The durable error keeps the code prefix operators grep for, and never + # the "setup/run error: " shape the else branch produces. + assert settled["error_message"] == f"{code}: {safe_message}" + assert "setup/run error" not in settled["error_message"] + + +@pytest.mark.asyncio +async def test_incidental_failure_persists_the_generic_history_type( + db_session, +) -> None: + """The counterpart: an incidental failure keeps the untrusted settlement.""" + + error = RuntimeError("secret-token-xyz") + + with _captured_terminal_broadcast(error, db_session) as ( + task_id, + frames, + settlements, + ): + task = db_session.query(Task).filter(Task.id == task_id).one() + await _run_failing_turn(task_id, int(task.user_id), task.source) + + assert len(settlements) == 1 + settled = settlements[0] + assert settled["client_message_type"] == TASK_FAILURE_MESSAGE_TYPE + assert settled["client_error_message"] == CLIENT_SAFE_TASK_FAILURE + assert "secret-token-xyz" not in settled["client_error_message"] From 377eea51b1ba13aa3126cbb62a321ae308c4cf19 Mon Sep 17 00:00:00 2001 From: Alexliu Date: Sat, 29 Aug 2026 00:43:48 +0800 Subject: [PATCH 08/28] docs(web): name the /v1 sibling projection and why it differs `_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. --- .../web/services/client_error_messages.py | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/src/xagent/web/services/client_error_messages.py b/src/xagent/web/services/client_error_messages.py index 83a558448c..f0b9e483fe 100644 --- a/src/xagent/web/services/client_error_messages.py +++ b/src/xagent/web/services/client_error_messages.py @@ -244,14 +244,28 @@ def connector_runtime_public_error( by passing an incidental exception. The reason filter itself lives in ``PublicErrorDetails``; this function only decides whether the exception is one we project at all. + + This is not the only client-visible projection of this exception. + ``_raise_v1_connector_runtime_error`` (``web/api/v1/tasks.py``) projects it + for the SDK surface and ships ``to_public_error()["details"]`` whole, + ``connector_ref`` included. The two differ because their audiences do: that + one answers an API key held by a caller already authorized for the task, + while this one feeds ``broadcast_to_task``, which reaches every connection + under the task id including anonymous widget and share-link visitors. + Keep them as two projectors with one audience each; folding them into one + that takes the audience as an argument puts the width of the output behind + a caller-supplied flag, which fails open the first time it is passed wrong. """ if not isinstance(error, ConnectorRuntimeError): return None details = error.details if not isinstance(details, dict): - # A details payload of the wrong shape means the exception instance - # itself is not trustworthy. Fall all the way back to the opaque - # failure rather than guessing which half of it is still readable. + # ``__init__`` normalizes details to a dict, but it is a plain public + # attribute anything can reassign afterwards. This is the last step + # before the wire, so verify rather than assume: a payload of the wrong + # shape means the instance is not trustworthy, and the safe answer is + # to fall all the way back to the opaque failure rather than guess + # which half of it is still readable. return None return error.code, PublicErrorDetails(reason=details.get("reason")) From abc51dddc8cb92ba34c798108d7b0db1fdc58234 Mon Sep 17 00:00:00 2001 From: Alexliu Date: Sat, 29 Aug 2026 00:44:05 +0800 Subject: [PATCH 09/28] fix(frontend): dedup terminal errors by code and reason, drop unread 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. --- .../src/contexts/app-context-chat.test.tsx | 124 ++++++++++++++---- frontend/src/contexts/app-context-chat.tsx | 60 ++++----- 2 files changed, 128 insertions(+), 56 deletions(-) diff --git a/frontend/src/contexts/app-context-chat.test.tsx b/frontend/src/contexts/app-context-chat.test.tsx index b699d2e4f8..11655fee37 100644 --- a/frontend/src/contexts/app-context-chat.test.tsx +++ b/frontend/src/contexts/app-context-chat.test.tsx @@ -5625,15 +5625,6 @@ describe("AppProvider websocket message routing", () => { }) }) -function ConnectorRuntimeErrorProbe() { - const { state } = useApp() - return ( -
- {JSON.stringify(state.lastConnectorRuntimeError)} -
- ) -} - describe("terminal error frames", () => { // Same reset as the routing suite above: the websocket harness ref and the // duplicate-message cache both outlive a single render, so without this the @@ -5710,7 +5701,6 @@ describe("terminal error frames", () => { - ) @@ -5747,10 +5737,6 @@ describe("terminal error frames", () => { // No prefix: this wording replaces the server sentence rather than // decorating it. expect(bubble?.content).toBe("common.errors.connectorRuntimeMissingKey") - expect(JSON.parse(screen.getByTestId("connector-runtime-error").textContent || "null")).toEqual({ - code: "missing_runtime_context", - details: { reason: "missing_context.auth_token" }, - }) }) // A listed reason that is a bare enum value names no key, so the keyless @@ -5760,7 +5746,6 @@ describe("terminal error frames", () => { - ) @@ -5798,7 +5783,6 @@ describe("terminal error frames", () => { - ) @@ -5827,10 +5811,6 @@ describe("terminal error frames", () => { expect(screen.getByTestId("messages").textContent).not.toContain( "common.errors.connectorRuntimeMissingKey" ) - expect(JSON.parse(screen.getByTestId("connector-runtime-error").textContent || "null")).toEqual({ - code: "missing_runtime_context", - details: {}, - }) }) // connector_runtime_unavailable reports a server-side component being @@ -5840,7 +5820,6 @@ describe("terminal error frames", () => { - ) @@ -5869,9 +5848,106 @@ describe("terminal error frames", () => { expect(screen.getByTestId("messages").textContent).not.toContain( "common.errors.connectorRuntimeMissing" ) - expect(JSON.parse(screen.getByTestId("connector-runtime-error").textContent || "null")).toEqual({ - code: "connector_runtime_unavailable", - details: { reason: "team_env_resolution_failed" }, + }) + + // The server sends one fixed sentence per error code, so the message alone + // cannot tell two missing keys apart. Inside the 30-second dedup window the + // second bubble would vanish and the surviving one would name whichever key + // failed first -- one value standing for two facts, which is the shape this + // whole change exists to remove. + it("keeps both bubbles when two turns miss two different keys", async () => { + render( + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + const frameForKey = (reason: string, timestamp: string) => ({ + type: "task_error", + timestamp, + task_id: 1, + task: { id: 1, status: "failed" }, + // Identical on both frames: _message_for_code returns one string per + // code and does not vary with the reason. + message: "Required connector runtime context is missing.", + error: "Required connector runtime context is missing.", + code: "missing_runtime_context", + details: { reason }, + }) as TestWebSocketMessage + + act(() => { + onMessage?.(frameForKey("missing_context.auth_token", "2026-05-27T05:00:02Z")) + }) + await waitFor(() => { + expect(screen.getByTestId("messages").textContent).toContain( + "common.errors.connectorRuntimeMissingKey" + ) + }) + + act(() => { + onMessage?.(frameForKey("missing_context.tenant_id", "2026-05-27T05:00:03Z")) + }) + + await waitFor(() => { + const messages = JSON.parse( + screen.getByTestId("messages").textContent || "[]" + ) + const bubbles = messages.filter((m: { content: string }) => + m.content.includes("common.errors.connectorRuntimeMissingKey") + ) + expect(bubbles).toHaveLength(2) + }) + }) + + // The other half of the same contract: a genuine repeat of one failure is + // still collapsed, so the identity did not simply disable deduplication. + it("still collapses a repeat of the same missing key", async () => { + render( + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + const frame = { + type: "task_error", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + task: { id: 1, status: "failed" }, + message: "Required connector runtime context is missing.", + error: "Required connector runtime context is missing.", + code: "missing_runtime_context", + details: { reason: "missing_context.auth_token" }, + } as TestWebSocketMessage + + act(() => { + onMessage?.(frame) + }) + await waitFor(() => { + expect(screen.getByTestId("messages").textContent).toContain( + "common.errors.connectorRuntimeMissingKey" + ) + }) + + act(() => { + onMessage?.({ ...frame, timestamp: "2026-05-27T05:00:03Z" }) + }) + + await waitFor(() => { + const messages = JSON.parse( + screen.getByTestId("messages").textContent || "[]" + ) + const bubbles = messages.filter((m: { content: string }) => + m.content.includes("common.errors.connectorRuntimeMissingKey") + ) + expect(bubbles).toHaveLength(1) }) }) }) diff --git a/frontend/src/contexts/app-context-chat.tsx b/frontend/src/contexts/app-context-chat.tsx index 840530ad6f..88c677a6ea 100644 --- a/frontend/src/contexts/app-context-chat.tsx +++ b/frontend/src/contexts/app-context-chat.tsx @@ -47,8 +47,9 @@ type TaskControlState = // The structured half of a terminal task_error frame. ``details`` holds at // most ``reason``: the server projects the exception through a whitelist // before broadcasting, because this frame reaches every connection on the -// task, anonymous widget and share-link visitors included. -type ConnectorRuntimeErrorState = { +// task, anonymous widget and share-link visitors included. Nothing here is +// connector-specific: the code is what decides whether a given frame is. +type TaskErrorProjection = { code: string details: { reason?: string } } @@ -103,7 +104,6 @@ const TASK_SCOPED_ACTION_TYPES = new Set([ "UPSERT_STREAMING_FINAL_ANSWER", "ADD_TRACE_EVENT", "SET_CONTEXT_USAGE", - "SET_CONNECTOR_RUNTIME_ERROR", "SET_PLAN_MEMORY_INFO", "OPEN_FILE_PREVIEW", ]) @@ -939,13 +939,13 @@ const CONNECTOR_RUNTIME_MISSING_VALUE_CODES = new Set([ ]) // The frame deliberately carries no connector identity: its audience includes -// anonymous widget and share-link visitors. The key name below is the only -// connector-specific thing available here; anything more (which connector, the -// declared type of each key) comes from the per-task requirements endpoint, -// which is owner-only. -const getConnectorRuntimeError = ( +// anonymous widget and share-link visitors. A missing key name parsed out of +// the reason is the only connector-specific thing available here; anything +// more (which connector, the declared type of each key) comes from the +// per-task requirements endpoint, which is owner-only. +const getTaskErrorProjection = ( message: WebSocketMessage, -): ConnectorRuntimeErrorState | null => { +): TaskErrorProjection | null => { const root = message as unknown as Record const data = isJsonRecord(message.data) ? message.data : null const code = getString(data?.code) || getString(root.code) @@ -1124,11 +1124,6 @@ export interface AppState { isHistoryLoading: boolean // Current context-window usage from the latest LLM call, for the usage gauge. contextUsage: { tokens: number; threshold: number } | null - // The structured half of the last terminal connector-runtime failure on the - // viewed task. It holds only what the frame is allowed to carry; the - // connector identity and the declared key types come from the per-task - // requirements endpoint instead. - lastConnectorRuntimeError: ConnectorRuntimeErrorState | null sessionConversation: SessionConversationState } @@ -1146,7 +1141,6 @@ type AppAction = | { type: "SET_DAG_EXECUTION"; payload: DAGExecution | null } | { type: "RESET_DAG_STATE" } | { type: "SET_CONTEXT_USAGE"; payload: { tokens: number; threshold: number } | null } - | { type: "SET_CONNECTOR_RUNTIME_ERROR"; payload: ConnectorRuntimeErrorState | null } | { type: "ADD_STEP"; payload: StepExecution } | { type: "UPDATE_STEP"; payload: { stepId: string; updates: Partial } } | { type: "SET_STEPS"; payload: StepExecution[] } @@ -1214,7 +1208,6 @@ const createInitialState = (): AppState => ({ lastTaskUpdate: Date.now(), isHistoryLoading: false, contextUsage: null, - lastConnectorRuntimeError: null, sessionConversation: { ...initialSessionConversationState }, }) @@ -1561,9 +1554,6 @@ function projectAppState(state: AppState, action: AppAction): AppState { case "SET_CONTEXT_USAGE": return { ...state, contextUsage: action.payload } - case "SET_CONNECTOR_RUNTIME_ERROR": - return { ...state, lastConnectorRuntimeError: action.payload } - case "ADD_STEP": const newStep = action.payload const existingStepIndex = state.steps.findIndex(s => s.id === newStep.id) @@ -5723,7 +5713,7 @@ export function AppProvider({ ? t(clientErrorTranslationKey(websocketErrorCode)) : getWebSocketErrorMessage(message, trustLegacyErrorProse) const websocketTaskStatus = getWebSocketTaskStatus(message) - const connectorRuntimeError = getConnectorRuntimeError(message) + const taskErrorProjection = getTaskErrorProjection(message) if (websocketTaskStatus) { dispatch({ type: "UPDATE_TASK_STATUS", payload: { status: websocketTaskStatus } }) @@ -5732,12 +5722,6 @@ export function AppProvider({ if (shouldStopProcessingForTaskStatus(websocketTaskStatus)) { dispatch({ type: "SET_PROCESSING", payload: false }) } - if (connectorRuntimeError) { - dispatch({ - type: "SET_CONNECTOR_RUNTIME_ERROR", - payload: connectorRuntimeError, - }) - } // A missing runtime value is the one failure here the user can fix, // so name the key instead of relaying the server's sentence. The @@ -5745,11 +5729,11 @@ export function AppProvider({ // listed value, which is why the keyless wording has to exist. let connectorRuntimeBubble: string | null = null if ( - connectorRuntimeError - && CONNECTOR_RUNTIME_MISSING_VALUE_CODES.has(connectorRuntimeError.code) + taskErrorProjection + && CONNECTOR_RUNTIME_MISSING_VALUE_CODES.has(taskErrorProjection.code) ) { const missingKey = missingRuntimeKeyFromReason( - connectorRuntimeError.details.reason + taskErrorProjection.details.reason ) connectorRuntimeBubble = missingKey ? t('common.errors.connectorRuntimeMissingKey', { key: missingKey }) @@ -5758,9 +5742,21 @@ export function AppProvider({ const errorBubbleContent = connectorRuntimeBubble ?? `${t('agent.logs.event.messages.errorPrefix')} ${websocketErrorMessage}` - // The dedup key stays the server's own message: it identifies the - // failure, and the rendered wording above is derived from it. - if (!isDuplicateMessageForViewedTask(websocketErrorMessage, "agent-error")) { + // The server sentence alone is too coarse to dedup on: one fixed + // string covers a whole error code, so two turns failing on two + // different missing keys share it while the bubbles above differ. + // Carry the code and reason as the occurrence identity so the second + // key still gets its own bubble. + const errorOccurrenceIdentity = taskErrorProjection + ? `${taskErrorProjection.code}:${taskErrorProjection.details.reason ?? ""}` + : undefined + if ( + !isDuplicateMessageForViewedTask( + websocketErrorMessage, + "agent-error", + errorOccurrenceIdentity, + ) + ) { dispatch({ type: "ADD_MESSAGE", payload: { From 54376442373fbf32a6f063fb0dffb23711635862 Mon Sep 17 00:00:00 2001 From: Alexliu Date: Wed, 2 Sep 2026 02:37:07 +0800 Subject: [PATCH 10/28] fix(web): drop owner key names from the connector-runtime reason whitelist A reason of shape missing_context. 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. 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. --- .../web/services/client_error_messages.py | 60 ++++++------- src/xagent/web/services/task_orchestrator.py | 12 ++- .../web/api/test_terminal_task_error_event.py | 4 +- .../services/test_client_error_messages.py | 84 ++++++++++++++++--- tests/web/services/test_task_orchestrator.py | 48 +++++++---- 5 files changed, 136 insertions(+), 72 deletions(-) diff --git a/src/xagent/web/services/client_error_messages.py b/src/xagent/web/services/client_error_messages.py index f0b9e483fe..21136e0087 100644 --- a/src/xagent/web/services/client_error_messages.py +++ b/src/xagent/web/services/client_error_messages.py @@ -10,10 +10,7 @@ from enum import StrEnum from ...core.tools.adapters.vibe.config import RequiredMCPUnavailableError -from ...core.tools.adapters.vibe.connector_runtime import ( - RUNTIME_SOURCE_KEY_RE, - ConnectorRuntimeError, -) +from ...core.tools.adapters.vibe.connector_runtime import ConnectorRuntimeError CLIENT_SAFE_VALIDATION_ERROR = "The message could not be processed. Please try again." @@ -171,47 +168,34 @@ def connector_runtime_client_message( # adds the site raising it, never ahead of it: a listed reason nothing produces # is an allowance with no expiry date, and by the time the raising code arrives # nobody remembers which audience the reason was judged against. -CONNECTOR_RUNTIME_PUBLIC_REASON_PREFIXES = frozenset( - { - "missing_context", - "type_mismatch.context", - "type_mismatch.secrets", - "type_mismatch.auth_selector", - "conflict.context", - "conflict.secrets", - "conflict.auth_selector", - } -) def _is_public_reason(reason: object) -> bool: """True when this reason may reach a client. Used by PublicErrorDetails. - The key half of a prefixed reason is matched against the declared runtime - key grammar itself, not a copy of it, so the two cannot drift apart. + Membership is the whole rule: the listed values are fixed strings this + repository writes, so reading the list tells you exactly what can reach a + visitor. A reason assembled from something the connector's owner wrote -- + ``missing_context.`` is the one such reason raised here + -- is not admitted, however legal its shape, because this frame reaches + anonymous widget and share-link visitors and a key name is the owner's + configuration. Owners read key names from the per-task requirements + endpoint, which selects on ``Task.id == task_id AND + Task.user_id == current_user.id``. """ - if not isinstance(reason, str): - return False - if reason in CONNECTOR_RUNTIME_PUBLIC_REASONS: - return True - prefix, separator, key = reason.rpartition(".") - if not separator: - return False - if prefix not in CONNECTOR_RUNTIME_PUBLIC_REASON_PREFIXES: - return False - return RUNTIME_SOURCE_KEY_RE.fullmatch(key) is not None + return isinstance(reason, str) and reason in CONNECTOR_RUNTIME_PUBLIC_REASONS @dataclass(frozen=True) class PublicErrorDetails: """The only shape allowed into a task_error frame's ``details``. - ``reason`` is normalized on construction: a value that is not a listed - enum member, and not ``.``, becomes - ``None``. Constructing this type and passing the reason whitelist are - therefore the same act -- there is no path that produces an instance - carrying free text, including a direct call from another module. + ``reason`` is normalized on construction: anything that is not a listed + enum member becomes ``None``. Constructing this type and passing the + reason whitelist are therefore the same act -- there is no path that + produces an instance carrying free text, including a direct call from + another module. Nulling rather than raising is deliberate: every construction site is on the reporting path of an already-failed task, and raising there would @@ -219,10 +203,14 @@ class PublicErrorDetails: The sink is ``broadcast_to_task``, whose audience includes anonymous widget and share-link visitors, so every listed reason and every new - field must answer one question first: can a visitor who is not the task - owner read the task's ownership, or the outcome of an authorization - check, out of it? There is no ``connector_ref`` field because the answer - for it is yes; two runtime reasons are omitted for the same answer. + field has to answer two questions, and a yes to either keeps it off this + frame. First: can a visitor who is not the task owner read the task's + ownership, or the outcome of an authorization check, out of it? Second: + does any part of it come from something the connector's owner wrote down + -- a key name, a label, a ref -- rather than from a fixed string this + repository controls? There is no ``connector_ref`` field and two runtime + reasons are omitted on the first question; the + ``.`` forms are omitted on the second. """ reason: str | None diff --git a/src/xagent/web/services/task_orchestrator.py b/src/xagent/web/services/task_orchestrator.py index 93812c40e9..fe47bc5a16 100644 --- a/src/xagent/web/services/task_orchestrator.py +++ b/src/xagent/web/services/task_orchestrator.py @@ -1969,10 +1969,14 @@ async def execute_owned_run() -> None: ) elif isinstance(setup_or_run_err, ConnectorRuntimeError): # This exception's message is a curated public-safe - # sentence naming what the connector still needs, so - # the client gets it instead of the opaque fallback. - # ``code`` and the whitelisted ``reason`` ride along on - # the frame; the projector decides what is wire-safe. + # sentence -- it says a runtime input is missing, not + # which one -- so the client gets it instead of the + # opaque fallback. ``code`` rides along on the frame, + # and so does ``reason`` when the whitelist admits it; + # a reason assembled from a key name the connector's + # owner declared is dropped at the projector, so this + # branch's own reason reaches the frame for two of the + # three codes and not for missing_runtime_context. settlement_error = str(setup_or_run_err) client_history_message_type = CLIENT_SAFE_FAILURE_MESSAGE_TYPE broadcast_error_message = connector_runtime_client_message( diff --git a/tests/web/api/test_terminal_task_error_event.py b/tests/web/api/test_terminal_task_error_event.py index 4759298250..47caea7fa6 100644 --- a/tests/web/api/test_terminal_task_error_event.py +++ b/tests/web/api/test_terminal_task_error_event.py @@ -46,12 +46,12 @@ def test_terminal_error_event_carries_both_new_fields_together() -> None: 1, "x", code="missing_runtime_context", - details=PublicErrorDetails(reason="missing_context.auth_token"), + details=PublicErrorDetails(reason="not_provided"), ) assert set(event.keys()) == BASE_FIELDS | {"code", "details"} assert event["code"] == "missing_runtime_context" - assert event["details"] == {"reason": "missing_context.auth_token"} + assert event["details"] == {"reason": "not_provided"} def test_terminal_error_event_keeps_an_emptied_details_object() -> None: diff --git a/tests/web/services/test_client_error_messages.py b/tests/web/services/test_client_error_messages.py index a29149ba68..feda6fef93 100644 --- a/tests/web/services/test_client_error_messages.py +++ b/tests/web/services/test_client_error_messages.py @@ -19,7 +19,6 @@ from xagent.web.services import client_error_messages from xagent.web.services.client_error_messages import ( CLIENT_SAFE_TASK_FAILURE, - CONNECTOR_RUNTIME_PUBLIC_REASON_PREFIXES, CONNECTOR_RUNTIME_PUBLIC_REASONS, PublicErrorDetails, connector_runtime_client_message, @@ -92,6 +91,15 @@ def test_client_message_is_fail_closed_for_an_incidental_exception( # anonymous widget and share-link visitors. "runtime_owner_mismatch", "runtime_task_identity_mismatch", + # Shape-legal, free of ownership and authorization content, and withheld + # anyway: the key half is a name the connector's owner declared, and this + # frame reaches anonymous widget and share-link visitors. Owners read key + # names from the per-task requirements endpoint, which selects on + # Task.id == task_id AND Task.user_id == current_user.id. + "missing_context.auth_token", + "missing_context.tenant_secret", + "type_mismatch.context.tenant_id", + "conflict.secrets.authorization", ] @@ -112,9 +120,6 @@ def test_public_error_details_normalizes_reason(reason: object) -> None: "team_scope_resolution_failed", "runtime_view_resolution_failed", "custom_api_config_load_failed", - "missing_context.auth_token", - "type_mismatch.context.tenant_id", - "conflict.secrets.authorization", ] @@ -136,16 +141,45 @@ def test_public_error_details_accepts_an_absent_reason() -> None: def test_public_error_projects_code_and_whitelisted_reason() -> None: + error = ConnectorRuntimeError( + "runtime_secret_unavailable", + "Required runtime secret is unavailable.", + details={"reason": "not_provided"}, + ) + + projected = connector_runtime_public_error(error) + + assert projected is not None + code, details = projected + # Asserted through to_wire(), not by comparing to a second + # PublicErrorDetails: the comparison value runs the same __post_init__, so + # a whitelist that stopped admitting this reason would null both sides and + # the assertion would pass while verifying nothing. + assert code == "runtime_secret_unavailable" + assert details.to_wire() == {"reason": "not_provided"} + + +def test_a_reason_built_from_a_declared_key_name_never_reaches_the_wire() -> None: + """The one reason in this repository assembled from owner-written text. + + ``_require_context_values`` raises ``missing_context.``, where the key + is a name the connector's owner chose. It is dropped whole rather than + trimmed to its prefix: a prefix that only ever pairs with a dropped key + tells a visitor nothing the code has not already told them. + """ + error = ConnectorRuntimeError( "missing_runtime_context", "Required connector runtime context is missing.", details={"reason": "missing_context.auth_token"}, ) - assert connector_runtime_public_error(error) == ( - "missing_runtime_context", - PublicErrorDetails(reason="missing_context.auth_token"), - ) + projected = connector_runtime_public_error(error) + + assert projected is not None + code, details = projected + assert code == "missing_runtime_context" + assert details.to_wire() == {} @pytest.mark.parametrize( @@ -202,7 +236,7 @@ def test_public_error_drops_every_field_but_reason() -> None: "missing_runtime_context", "x", details={ - "reason": "missing_context.auth_token", + "reason": "not_provided", "internal_sql": "SELECT 1", "raw_value": "tenant-secret", "connector_ref": {"id": 7, "name": "acme"}, @@ -388,10 +422,7 @@ def _derive_reasons() -> tuple[set[str], set[str]]: def _is_listed(reason: str) -> bool: - if reason in CONNECTOR_RUNTIME_PUBLIC_REASONS: - return True - prefix, separator, key = reason.rpartition(".") - return bool(separator) and prefix in CONNECTOR_RUNTIME_PUBLIC_REASON_PREFIXES + return reason in CONNECTOR_RUNTIME_PUBLIC_REASONS def test_public_reason_whitelist_covers_every_raise_site() -> None: @@ -454,3 +485,30 @@ def test_knowledge_base_scope_reason_is_not_in_the_derived_surface() -> None: tree = ast.parse(path.read_text(encoding="utf-8")) assert _reason_expressions(tree) == [] + + +def test_no_reason_assembled_by_interpolation_is_admitted_by_its_shape() -> None: + """A reason with an interpolated half is never admitted by its shape. + + The two assertions above read only the literal reasons, because a reason + built by interpolation has no single value to look up. This one reads the + derived shapes instead: for every interpolated reason the scan finds, an + arbitrary instantiation of it must be dropped. Admitting a whole shape is + how a name the connector's owner chose reaches a visitor without any one + line of code saying so, and this is the assertion that a re-added prefix + set breaks. + """ + + _, patterns = _derive_reasons() + + assert patterns, "the derivation found no interpolated reason; the scan is broken" + for pattern in patterns: + probe = ( + pattern.removeprefix("^") + .removesuffix("$") + .replace("\\.", ".") + .replace(".+", "zzz-probe-zzz") + ) + assert PublicErrorDetails(reason=probe).to_wire() == {}, ( + f"a reason of shape {pattern} is admitted by its shape: {probe}" + ) diff --git a/tests/web/services/test_task_orchestrator.py b/tests/web/services/test_task_orchestrator.py index e45fcf98bc..b6dd197f5f 100644 --- a/tests/web/services/test_task_orchestrator.py +++ b/tests/web/services/test_task_orchestrator.py @@ -3864,6 +3864,14 @@ def test_reconcile_finalized_delivery_noop_on_already_terminal_row( "scheduled_secret_unavailable", ] +# The reason a missing declared context key produces. It is deliberately not +# public: the key half is a name the connector's owner chose, and the frame's +# audience includes anonymous widget and share-link visitors. +WITHHELD_KEY_REASON = "missing_context.auth_token" +# A listed reason, so the assertions below can speak about a reason that does +# reach the wire. +PUBLIC_REASON = "not_provided" + @contextmanager def _captured_terminal_broadcast(setup_or_run_error: BaseException, db_session): @@ -3954,7 +3962,7 @@ async def test_connector_runtime_failure_broadcasts_its_safe_message( error = ConnectorRuntimeError( code, safe_message, - details={"reason": "missing_context.auth_token"}, + details={"reason": WITHHELD_KEY_REASON}, ) with _captured_terminal_broadcast(error, db_session) as ( @@ -4007,10 +4015,10 @@ async def test_connector_runtime_frame_details_shape(db_session) -> None: """Whatever the raise site attached, only ``reason`` can reach the wire.""" error = ConnectorRuntimeError( - "missing_runtime_context", - "Required connector runtime context is missing.", + "runtime_secret_unavailable", + "Required runtime secret is unavailable.", details={ - "reason": "missing_context.auth_token", + "reason": PUBLIC_REASON, "internal_sql": "SELECT value FROM task_connector_runtime_contexts", "raw_value": "tenant-secret", "connector_ref": {"connector_type": "mcp", "connector_id": 7}, @@ -4026,7 +4034,7 @@ async def test_connector_runtime_frame_details_shape(db_session) -> None: await _run_failing_turn(task_id, int(task.user_id), task.source) assert set(frames[0]["details"]) <= {"reason"} - assert frames[0]["details"] == {"reason": "missing_context.auth_token"} + assert frames[0]["details"] == {"reason": PUBLIC_REASON} @pytest.mark.asyncio @@ -4042,10 +4050,10 @@ async def test_connector_runtime_frame_never_carries_connector_ref( """ error = ConnectorRuntimeError( - "missing_runtime_context", - "Required connector runtime context is missing.", + "runtime_secret_unavailable", + "Required runtime secret is unavailable.", connector_ref=ConnectorRef(connector_type="mcp", connector_id=7), - details={"reason": "missing_context.auth_token"}, + details={"reason": PUBLIC_REASON}, ) with _captured_terminal_broadcast(error, db_session) as ( @@ -4082,7 +4090,7 @@ async def test_connector_runtime_frame_reason_matches_direct_construction( error = ConnectorRuntimeError( "missing_runtime_context", "Required connector runtime context is missing.", - details={"reason": "missing_context.auth_token"}, + details={"reason": PUBLIC_REASON}, ) with _captured_terminal_broadcast(error, db_session) as ( @@ -4093,10 +4101,8 @@ async def test_connector_runtime_frame_reason_matches_direct_construction( task = db_session.query(Task).filter(Task.id == task_id).one() await _run_failing_turn(task_id, int(task.user_id), task.source) - assert ( - frames[0]["details"] - == PublicErrorDetails(reason="missing_context.auth_token").to_wire() - ) + assert frames[0]["details"] == PublicErrorDetails(reason=PUBLIC_REASON).to_wire() + assert frames[0]["details"] == {"reason": PUBLIC_REASON} @pytest.mark.asyncio @@ -4112,7 +4118,7 @@ async def test_connector_runtime_failure_logs_missing_key( code, "Required connector runtime context is missing.", connector_ref=ConnectorRef(connector_type="mcp", connector_id=7), - details={"reason": "missing_context.auth_token"}, + details={"reason": WITHHELD_KEY_REASON}, ) with caplog.at_level(logging.ERROR): @@ -4132,7 +4138,7 @@ async def test_connector_runtime_failure_logs_missing_key( ] assert len(structured) == 1 assert f"code={code}" in structured[0] - assert "reason=missing_context.auth_token" in structured[0] + assert f"reason={WITHHELD_KEY_REASON}" in structured[0] assert "connector=" in structured[0] assert "'connector_id': 7" in structured[0] @@ -4156,7 +4162,7 @@ async def test_connector_runtime_failure_persists_client_safe_history( error = ConnectorRuntimeError( code, safe_message, - details={"reason": "missing_context.auth_token"}, + details={"reason": WITHHELD_KEY_REASON}, ) with _captured_terminal_broadcast(error, db_session) as ( @@ -4170,9 +4176,17 @@ async def test_connector_runtime_failure_persists_client_safe_history( assert len(settlements) == 1 settled = settlements[0] assert settled["client_message_type"] == CLIENT_SAFE_FAILURE_MESSAGE_TYPE - # The reloaded transcript says the same thing the live bubble said. + # The durable row and the frame's own message field are the same server + # sentence. The live bubble is not that sentence: for a missing-value code + # the client replaces it with its own localized wording (see the + # "terminal error frames" suite in app-context-chat.test.tsx). What the two + # views owe each other is the facts they carry, and the key name is in + # neither -- the whitelist drops the reason that names it, so the frame + # cannot carry it and the client cannot render it. assert settled["client_error_message"] == safe_message assert settled["client_error_message"] == frames[0]["message"] + assert frames[0]["details"] == {} + assert "auth_token" not in json.dumps(frames[0]) # The durable error keeps the code prefix operators grep for, and never # the "setup/run error: " shape the else branch produces. assert settled["error_message"] == f"{code}: {safe_message}" From ca26ab85e16a6e63d5b609a8e35681ec1846d300 Mon Sep 17 00:00:00 2001 From: Alexliu Date: Wed, 2 Sep 2026 02:52:00 +0800 Subject: [PATCH 11/28] fix(frontend): drop the missing-key wording, match the server's dropped 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. --- .../src/contexts/app-context-chat.test.tsx | 180 +++--------------- frontend/src/contexts/app-context-chat.tsx | 45 ++--- frontend/src/i18n/locales/en.ts | 1 - frontend/src/i18n/locales/zh.ts | 1 - 4 files changed, 44 insertions(+), 183 deletions(-) diff --git a/frontend/src/contexts/app-context-chat.test.tsx b/frontend/src/contexts/app-context-chat.test.tsx index 11655fee37..2aa227f2bf 100644 --- a/frontend/src/contexts/app-context-chat.test.tsx +++ b/frontend/src/contexts/app-context-chat.test.tsx @@ -80,7 +80,10 @@ vi.mock("@/contexts/auth-context", () => ({ })) vi.mock("@/contexts/i18n-context", () => ({ - useI18n: () => ({ t: (key: string) => key }), + useI18n: () => ({ + t: (key: string, vars?: Record) => + vars ? `${key}:${JSON.stringify(vars)}` : key, + }), })) vi.mock("@/hooks/use-websocket", () => ({ @@ -5696,52 +5699,9 @@ describe("terminal error frames", () => { } ) - it("names the missing connector key instead of relaying the server sentence", async () => { - render( - - - - - ) - - const onMessage = webSocketOptions.current?.onMessage - expect(onMessage).toBeDefined() - - act(() => { - onMessage?.({ - type: "task_error", - timestamp: "2026-05-27T05:00:02Z", - task_id: 1, - task: { id: 1, status: "failed" }, - message: "Required connector runtime context is missing.", - error: "Required connector runtime context is missing.", - code: "missing_runtime_context", - details: { reason: "missing_context.auth_token" }, - } as TestWebSocketMessage) - }) - - // The i18n mock in this file returns the key and drops the variables, so - // the assertion is on which wording was chosen, not on the rendered key - // name. The reason the key name is parsed from is asserted below. - await waitFor(() => { - expect(screen.getByTestId("messages").textContent).toContain( - "common.errors.connectorRuntimeMissingKey" - ) - }) - - const messages = JSON.parse(screen.getByTestId("messages").textContent || "[]") - const bubble = messages.find((m: { content: string }) => - m.content.includes("common.errors.connectorRuntimeMissingKey") - ) - expect(bubble?.isResult).toBe(true) - // No prefix: this wording replaces the server sentence rather than - // decorating it. - expect(bubble?.content).toBe("common.errors.connectorRuntimeMissingKey") - }) - // A listed reason that is a bare enum value names no key, so the keyless // wording is chosen even though the code is a missing-value one. - it("falls back to the generic wording when the reason names no key", async () => { + it("uses the same wording for a listed bare reason", async () => { render( @@ -5771,14 +5731,24 @@ describe("terminal error frames", () => { ) }) - expect(screen.getByTestId("messages").textContent).not.toContain( - "common.errors.connectorRuntimeMissingKey" + const messages = JSON.parse(screen.getByTestId("messages").textContent || "[]") + const bubble = messages.find((m: { content: string }) => + m.content.includes("common.errors.connectorRuntimeMissing") ) + // Same toBe as the test below, on the other input: this branch is reached + // with a listed bare reason rather than an empty details, and an + // interpolation regression has to be caught on both. + expect(bubble?.content).toBe("common.errors.connectorRuntimeMissing") }) - // The server drops a reason it cannot place on its whitelist, so the - // keyless wording has to exist and the frame still carries the code. - it("falls back to the generic wording when the reason was dropped", async () => { + // What the server now sends for a missing declared context key: the code + // survives, the reason naming the key does not. The bubble therefore says a + // value is missing without saying which -- the key name is owner + // configuration and this frame reaches anonymous widget and share-link + // visitors. Asserted with toBe, under a variable-aware i18n mock: had the + // wording interpolated anything, the content would read + // ":{...}" and this assertion would fail. + it("names no declared key when a runtime value is missing", async () => { render( @@ -5808,9 +5778,15 @@ describe("terminal error frames", () => { ) }) - expect(screen.getByTestId("messages").textContent).not.toContain( - "common.errors.connectorRuntimeMissingKey" + const messages = JSON.parse(screen.getByTestId("messages").textContent || "[]") + const bubble = messages.find((m: { content: string }) => + m.content.includes("common.errors.connectorRuntimeMissing") ) + // No prefix: this wording replaces the server sentence rather than + // decorating it. Exactly the key, with nothing appended: no variable was + // interpolated, so no key name can be in the rendered text. + expect(bubble?.content).toBe("common.errors.connectorRuntimeMissing") + expect(bubble?.isResult).toBe(true) }) // connector_runtime_unavailable reports a server-side component being @@ -5850,104 +5826,4 @@ describe("terminal error frames", () => { ) }) - // The server sends one fixed sentence per error code, so the message alone - // cannot tell two missing keys apart. Inside the 30-second dedup window the - // second bubble would vanish and the surviving one would name whichever key - // failed first -- one value standing for two facts, which is the shape this - // whole change exists to remove. - it("keeps both bubbles when two turns miss two different keys", async () => { - render( - - - - - ) - - const onMessage = webSocketOptions.current?.onMessage - expect(onMessage).toBeDefined() - - const frameForKey = (reason: string, timestamp: string) => ({ - type: "task_error", - timestamp, - task_id: 1, - task: { id: 1, status: "failed" }, - // Identical on both frames: _message_for_code returns one string per - // code and does not vary with the reason. - message: "Required connector runtime context is missing.", - error: "Required connector runtime context is missing.", - code: "missing_runtime_context", - details: { reason }, - }) as TestWebSocketMessage - - act(() => { - onMessage?.(frameForKey("missing_context.auth_token", "2026-05-27T05:00:02Z")) - }) - await waitFor(() => { - expect(screen.getByTestId("messages").textContent).toContain( - "common.errors.connectorRuntimeMissingKey" - ) - }) - - act(() => { - onMessage?.(frameForKey("missing_context.tenant_id", "2026-05-27T05:00:03Z")) - }) - - await waitFor(() => { - const messages = JSON.parse( - screen.getByTestId("messages").textContent || "[]" - ) - const bubbles = messages.filter((m: { content: string }) => - m.content.includes("common.errors.connectorRuntimeMissingKey") - ) - expect(bubbles).toHaveLength(2) - }) - }) - - // The other half of the same contract: a genuine repeat of one failure is - // still collapsed, so the identity did not simply disable deduplication. - it("still collapses a repeat of the same missing key", async () => { - render( - - - - - ) - - const onMessage = webSocketOptions.current?.onMessage - expect(onMessage).toBeDefined() - - const frame = { - type: "task_error", - timestamp: "2026-05-27T05:00:02Z", - task_id: 1, - task: { id: 1, status: "failed" }, - message: "Required connector runtime context is missing.", - error: "Required connector runtime context is missing.", - code: "missing_runtime_context", - details: { reason: "missing_context.auth_token" }, - } as TestWebSocketMessage - - act(() => { - onMessage?.(frame) - }) - await waitFor(() => { - expect(screen.getByTestId("messages").textContent).toContain( - "common.errors.connectorRuntimeMissingKey" - ) - }) - - act(() => { - onMessage?.({ ...frame, timestamp: "2026-05-27T05:00:03Z" }) - }) - - await waitFor(() => { - const messages = JSON.parse( - screen.getByTestId("messages").textContent || "[]" - ) - const bubbles = messages.filter((m: { content: string }) => - m.content.includes("common.errors.connectorRuntimeMissingKey") - ) - expect(bubbles).toHaveLength(1) - }) - }) }) diff --git a/frontend/src/contexts/app-context-chat.tsx b/frontend/src/contexts/app-context-chat.tsx index 88c677a6ea..23c76738a9 100644 --- a/frontend/src/contexts/app-context-chat.tsx +++ b/frontend/src/contexts/app-context-chat.tsx @@ -938,11 +938,13 @@ const CONNECTOR_RUNTIME_MISSING_VALUE_CODES = new Set([ "scheduled_secret_unavailable", ]) -// The frame deliberately carries no connector identity: its audience includes -// anonymous widget and share-link visitors. A missing key name parsed out of -// the reason is the only connector-specific thing available here; anything -// more (which connector, the declared type of each key) comes from the -// per-task requirements endpoint, which is owner-only. +// The frame deliberately carries nothing connector-specific beyond the code: +// its audience includes anonymous widget and share-link visitors, and the +// server's reason whitelist admits only fixed strings it controls -- never a +// reason assembled from a key name the connector's owner declared. Which +// connector, which key, and each key's declared type all come from the +// per-task requirements endpoint, which selects on +// `Task.id == task_id AND Task.user_id == current_user.id`. const getTaskErrorProjection = ( message: WebSocketMessage, ): TaskErrorProjection | null => { @@ -959,16 +961,6 @@ const getTaskErrorProjection = ( return { code, details: reason ? { reason } : {} } } -// A reason is either a bare enum value or ".". Only -// the second form names a key the user has to fill in. -const missingRuntimeKeyFromReason = (reason: string | undefined): string | null => { - if (!reason) return null - const separator = reason.lastIndexOf(".") - if (separator < 0) return null - const key = reason.slice(separator + 1) - return key || null -} - const getWebSocketTaskStatus = (message: WebSocketMessage): Task["status"] | null => { const root = message as unknown as Record const data = isJsonRecord(message.data) ? message.data : null @@ -5723,22 +5715,17 @@ export function AppProvider({ dispatch({ type: "SET_PROCESSING", payload: false }) } - // A missing runtime value is the one failure here the user can fix, - // so name the key instead of relaying the server's sentence. The - // reason is dropped by the server whitelist whenever it is not a - // listed value, which is why the keyless wording has to exist. - let connectorRuntimeBubble: string | null = null - if ( + // A missing runtime value is the one failure here the user can act on, + // so the bubble says so in the viewer's own language instead of relaying + // the server's fixed English sentence. It does not name the missing key: + // the key name is configuration the connector's owner wrote, and this + // frame reaches anonymous widget and share-link visitors. An owner reads + // the key names from the per-task requirements endpoint instead. + const connectorRuntimeBubble = taskErrorProjection && CONNECTOR_RUNTIME_MISSING_VALUE_CODES.has(taskErrorProjection.code) - ) { - const missingKey = missingRuntimeKeyFromReason( - taskErrorProjection.details.reason - ) - connectorRuntimeBubble = missingKey - ? t('common.errors.connectorRuntimeMissingKey', { key: missingKey }) - : t('common.errors.connectorRuntimeMissing') - } + ? t('common.errors.connectorRuntimeMissing') + : null const errorBubbleContent = connectorRuntimeBubble ?? `${t('agent.logs.event.messages.errorPrefix')} ${websocketErrorMessage}` diff --git a/frontend/src/i18n/locales/en.ts b/frontend/src/i18n/locales/en.ts index 7a47ff3b75..e393b6534c 100644 --- a/frontend/src/i18n/locales/en.ts +++ b/frontend/src/i18n/locales/en.ts @@ -66,7 +66,6 @@ const en = { errors: { unknown: "Unknown error", taskFailed: "Something went wrong. Please try again.", - connectorRuntimeMissingKey: "This connector still needs a value for \"{key}\".", connectorRuntimeMissing: "This connector needs additional runtime input before it can run.", }, }, diff --git a/frontend/src/i18n/locales/zh.ts b/frontend/src/i18n/locales/zh.ts index c977a9b5a8..97366e2cd0 100644 --- a/frontend/src/i18n/locales/zh.ts +++ b/frontend/src/i18n/locales/zh.ts @@ -66,7 +66,6 @@ const zh = { errors: { unknown: "未知错误", taskFailed: "出了点问题，请重试。", - connectorRuntimeMissingKey: "这个连接器还需要你提供 “{key}”。", connectorRuntimeMissing: "这个连接器需要额外的运行时输入，请补充后重试。", }, }, From 1075fbe9e1e623dd2696fb7293b509489f00aa0d Mon Sep 17 00:00:00 2001 From: Alexliu Date: Wed, 2 Sep 2026 02:53:59 +0800 Subject: [PATCH 12/28] fix(frontend): dedup terminal errors on (code, reason), not the fixed 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). --- .../src/contexts/app-context-chat.test.tsx | 104 ++++++++++++++++++ frontend/src/contexts/app-context-chat.tsx | 15 ++- 2 files changed, 114 insertions(+), 5 deletions(-) diff --git a/frontend/src/contexts/app-context-chat.test.tsx b/frontend/src/contexts/app-context-chat.test.tsx index 2aa227f2bf..2357659676 100644 --- a/frontend/src/contexts/app-context-chat.test.tsx +++ b/frontend/src/contexts/app-context-chat.test.tsx @@ -5826,4 +5826,108 @@ describe("terminal error frames", () => { ) }) + // The dedup key is the server sentence, and one sentence covers a whole + // code. These two turns fail under one code for two different admitted + // reasons -- the same runtime secret, first never provided, then lost from + // its store -- so they share that key while being two distinct failures. + // Without the (code, reason) identity the second bubble vanishes, and that + // bubble is the turn's result. + it("keeps both bubbles when one code fails for two different reasons", async () => { + render( + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + const frameForReason = (reason: string, timestamp: string) => ({ + type: "task_error", + timestamp, + task_id: 1, + task: { id: 1, status: "failed" }, + // Identical on both frames, and that is the production shape: + // _message_for_code returns one string per code and does not vary with + // the reason. + message: "Required runtime secret is unavailable.", + error: "Required runtime secret is unavailable.", + code: "runtime_secret_unavailable", + details: { reason }, + }) as TestWebSocketMessage + + act(() => { + onMessage?.(frameForReason("not_provided", "2026-05-27T05:00:02Z")) + }) + await waitFor(() => { + expect(screen.getByTestId("messages").textContent).toContain( + "common.errors.connectorRuntimeMissing" + ) + }) + + act(() => { + onMessage?.(frameForReason("store_lost", "2026-05-27T05:00:03Z")) + }) + + await waitFor(() => { + const messages = JSON.parse( + screen.getByTestId("messages").textContent || "[]" + ) + const bubbles = messages.filter( + (m: { content: string }) => + m.content === "common.errors.connectorRuntimeMissing" + ) + expect(bubbles).toHaveLength(2) + }) + }) + + // The other half of the same contract: a genuine repeat of one failure is + // still collapsed, so the identity did not simply disable deduplication. + it("still collapses a repeat of the same code and reason", async () => { + render( + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + const frame = { + type: "task_error", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + task: { id: 1, status: "failed" }, + message: "Required runtime secret is unavailable.", + error: "Required runtime secret is unavailable.", + code: "runtime_secret_unavailable", + details: { reason: "not_provided" }, + } as TestWebSocketMessage + + act(() => { + onMessage?.(frame) + }) + await waitFor(() => { + expect(screen.getByTestId("messages").textContent).toContain( + "common.errors.connectorRuntimeMissing" + ) + }) + + act(() => { + onMessage?.({ ...frame, timestamp: "2026-05-27T05:00:03Z" }) + }) + + await waitFor(() => { + const messages = JSON.parse( + screen.getByTestId("messages").textContent || "[]" + ) + const bubbles = messages.filter( + (m: { content: string }) => + m.content === "common.errors.connectorRuntimeMissing" + ) + expect(bubbles).toHaveLength(1) + }) + }) }) diff --git a/frontend/src/contexts/app-context-chat.tsx b/frontend/src/contexts/app-context-chat.tsx index 23c76738a9..c42c79d0b2 100644 --- a/frontend/src/contexts/app-context-chat.tsx +++ b/frontend/src/contexts/app-context-chat.tsx @@ -5729,11 +5729,16 @@ export function AppProvider({ const errorBubbleContent = connectorRuntimeBubble ?? `${t('agent.logs.event.messages.errorPrefix')} ${websocketErrorMessage}` - // The server sentence alone is too coarse to dedup on: one fixed - // string covers a whole error code, so two turns failing on two - // different missing keys share it while the bubbles above differ. - // Carry the code and reason as the occurrence identity so the second - // key still gets its own bubble. + // The string this dedup keys on is the server sentence -- or, on a + // transport that marks legacy prose untrusted, a single constant + // standing in for it -- and either way one value covers a whole error + // code: two turns failing under one code for two different admitted + // reasons -- a runtime secret not_provided, then the same secret + // store_lost -- share that key while being two distinct failures. + // Collapsing the second one now costs more than a bubble, because the + // bubble is the turn's result: that turn would end showing nothing. + // The structured (code, reason) pair is the occurrence axis instead; + // a genuine repeat of one failure still collapses. const errorOccurrenceIdentity = taskErrorProjection ? `${taskErrorProjection.code}:${taskErrorProjection.details.reason ?? ""}` : undefined From 662ae6efad97fb23d9daeb7581999059e83f39f7 Mon Sep 17 00:00:00 2001 From: Alexliu Date: Wed, 2 Sep 2026 02:56:19 +0800 Subject: [PATCH 13/28] fix(frontend): only flag the terminal task_error frame as a turn's result 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. --- .../src/contexts/app-context-chat.test.tsx | 169 ++++++++++++++---- frontend/src/contexts/app-context-chat.tsx | 28 ++- 2 files changed, 159 insertions(+), 38 deletions(-) diff --git a/frontend/src/contexts/app-context-chat.test.tsx b/frontend/src/contexts/app-context-chat.test.tsx index 2357659676..950b55f9aa 100644 --- a/frontend/src/contexts/app-context-chat.test.tsx +++ b/frontend/src/contexts/app-context-chat.test.tsx @@ -5659,45 +5659,150 @@ describe("terminal error frames", () => { // The conversation panel renders only user / isResult / system-notice // messages. Without the flag the bubble is filtered out and the UI falls // back to a generic "unknown error" placeholder until the page reloads. - it.each(["error", "task_error"])( - "flags the %s bubble as the turn's result", - async (frameType) => { - render( - - - - + it("flags the terminal task_error bubble as the turn's result", async () => { + render( + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + act(() => { + onMessage?.({ + type: "task_error", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + task: { id: 1, status: "failed" }, + message: "Task execution failed.", + error: "Task execution failed.", + } as TestWebSocketMessage) + }) + + await waitFor(() => { + expect(screen.getByTestId("messages").textContent).toContain( + "Task execution failed." ) + }) - const onMessage = webSocketOptions.current?.onMessage - expect(onMessage).toBeDefined() + const messages = JSON.parse( + screen.getByTestId("messages").textContent || "[]" + ) + const bubble = messages.find((m: { content: string }) => + m.content.includes("Task execution failed.") + ) + expect(bubble?.isResult).toBe(true) + }) - act(() => { - onMessage?.({ - type: frameType, - timestamp: "2026-05-27T05:00:02Z", - task_id: 1, - task: { id: 1, status: "failed" }, - message: "Task execution failed.", - error: "Task execution failed.", - } as TestWebSocketMessage) - }) + // Rejections arrive on the root "error" type while the task keeps running: + // websocket.py refuses a chat message (:5521-5554) and a pause command + // (:8491, :8502) that way. Flagging one as this turn's result makes the + // conversation panel treat the turn as answered -- it renders only user / + // isResult / system-notice messages, so a flagged rejection ends the live + // progress indicator of a turn that is still running. + it("does not treat a rejection on a running task as the turn's result", async () => { + render( + + + + + ) - await waitFor(() => { - expect(screen.getByTestId("messages").textContent).toContain( - "Task execution failed." - ) - }) + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() - const messages = JSON.parse( - screen.getByTestId("messages").textContent || "[]" + act(() => { + onMessage?.({ + type: "error", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + task: { id: 1, status: "running" }, + message: "Task is currently busy; please wait for the previous turn to finish.", + error_code: "task_busy", + } as TestWebSocketMessage) + }) + + await waitFor(() => { + expect(screen.getByTestId("messages").textContent).toContain( + "clientErrors.taskBusy" ) - const bubble = messages.find((m: { content: string }) => - m.content.includes("Task execution failed.") + }) + + const messages = JSON.parse(screen.getByTestId("messages").textContent || "[]") + const bubble = messages.find((m: { content: string }) => + m.content.includes("clientErrors.taskBusy") + ) + expect(bubble).toBeDefined() + expect(bubble?.isResult).not.toBe(true) + // The turn is untouched: still running, still processing. + expect(screen.getByTestId("task-status").textContent).toBe("running") + expect(screen.getByTestId("processing").textContent).toBe("true") + }) + + // The waiting half. A refused resume arrives on the root "error" type + // carrying the task's real current status (websocket.py:8935 builds it from + // TaskControlSnapshot). The question the user still has to answer lives on + // the panel's virtual bubble, which a flagged rejection would remove. + it("does not treat a rejection on a waiting task as the turn's result", async () => { + render( + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + // Put the task where a resume can be refused: waiting on a question that + // carries an interaction the user has to fill in. + act(() => { + onMessage?.({ + type: "task_waiting_for_user", + timestamp: "2026-05-27T05:00:01Z", + task_id: 1, + task: { id: 1, status: "waiting_for_user" }, + message: "Which region should I use?", + request_id: "req-1", + interactions: [ + { type: "text", request_id: "req-1", prompt: "Which region should I use?" }, + ], + } as TestWebSocketMessage) + }) + await waitFor(() => { + expect(screen.getByTestId("task-status").textContent).toBe("waiting_for_user") + }) + expect(screen.getByTestId("waiting-interactions").textContent).not.toBe("[]") + + act(() => { + onMessage?.({ + type: "error", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + task: { id: 1, status: "waiting_for_user" }, + message: "Task pause is still being applied; please retry shortly.", + error_code: "task_pause_in_progress", + } as TestWebSocketMessage) + }) + + await waitFor(() => { + expect(screen.getByTestId("messages").textContent).toContain( + "clientErrors.taskPauseInProgress" ) - expect(bubble?.isResult).toBe(true) - } - ) + }) + + const messages = JSON.parse(screen.getByTestId("messages").textContent || "[]") + const bubble = messages.find((m: { content: string }) => + m.content.includes("clientErrors.taskPauseInProgress") + ) + expect(bubble).toBeDefined() + expect(bubble?.isResult).not.toBe(true) + // The question the user still owes an answer to is untouched. + expect(screen.getByTestId("task-status").textContent).toBe("waiting_for_user") + expect(screen.getByTestId("waiting-interactions").textContent).not.toBe("[]") + }) // A listed reason that is a bare enum value names no key, so the keyless // wording is chosen even though the code is a missing-value one. diff --git a/frontend/src/contexts/app-context-chat.tsx b/frontend/src/contexts/app-context-chat.tsx index c42c79d0b2..80b3dad12d 100644 --- a/frontend/src/contexts/app-context-chat.tsx +++ b/frontend/src/contexts/app-context-chat.tsx @@ -5705,7 +5705,19 @@ export function AppProvider({ ? t(clientErrorTranslationKey(websocketErrorCode)) : getWebSocketErrorMessage(message, trustLegacyErrorProse) const websocketTaskStatus = getWebSocketTaskStatus(message) - const taskErrorProjection = getTaskErrorProjection(message) + // Only task_error is terminal. Every frame of that type is emitted after + // the row has been committed FAILED -- task_orchestrator.py's settled + // branch, and websocket.py's legacy helper, which settles under + // only_if_running=True and does not broadcast when that update matches + // no row -- and task_error is also the only frame that carries the + // structured code/details pair. The root "error" type is a mixed + // channel: rejected chat messages, rejected pause and rejected resume + // all arrive on it while the viewed task is still RUNNING or + // WAITING_FOR_USER, and a rejection is not this turn's answer. + const isTerminalErrorFrame = message.type === "task_error" + const taskErrorProjection = isTerminalErrorFrame + ? getTaskErrorProjection(message) + : null if (websocketTaskStatus) { dispatch({ type: "UPDATE_TASK_STATUS", payload: { status: websocketTaskStatus } }) @@ -5757,11 +5769,15 @@ export function AppProvider({ content: errorBubbleContent, timestamp: message.timestamp, status: "failed", - // Terminal failure IS this turn's result. Without the flag the - // conversation panel (which only shows user / isResult / - // system-notice messages) filters the bubble out and falls back - // to a virtual "unknown error" placeholder until reload. - isResult: true, + // A terminal failure IS this turn's result: without the flag the + // conversation panel (which renders only user / isResult / + // system-notice messages) filters the bubble out and falls back to + // a virtual "unknown error" placeholder until reload. A + // non-terminal rejection is not, and flagging it would close the + // live progress indicator and the waiting-answer form of a turn + // that is still running, and drain this turn's accumulated trace + // events into the rejection bubble (see ADD_MESSAGE above). + isResult: isTerminalErrorFrame, }, }) } From 8587668509342e39994a128b39990eb084957d63 Mon Sep 17 00:00:00 2001 From: Alexliu Date: Wed, 2 Sep 2026 03:38:43 +0800 Subject: [PATCH 14/28] docs(web): name the code set and the fixture's two producers in comments 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. --- frontend/src/contexts/app-context-chat.test.tsx | 7 +++++++ src/xagent/web/services/task_orchestrator.py | 3 ++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/frontend/src/contexts/app-context-chat.test.tsx b/frontend/src/contexts/app-context-chat.test.tsx index 950b55f9aa..33acc4a30c 100644 --- a/frontend/src/contexts/app-context-chat.test.tsx +++ b/frontend/src/contexts/app-context-chat.test.tsx @@ -5745,6 +5745,13 @@ describe("terminal error frames", () => { // carrying the task's real current status (websocket.py:8935 builds it from // TaskControlSnapshot). The question the user still has to answer lives on // the panel's virtual bubble, which a flagged rejection would remove. + // The fixture below combines fields from two producers: `task` comes from + // the resume-refusal path (websocket.py:8935), `error_code` comes from the + // pause-refusal path (websocket.py:8491). Neither path emits both fields + // together today; each field is genuinely emitted by its own path. + // Carrying `task` drives the reducer's preservation branch (it is what + // makes `UPDATE_TASK_STATUS` dispatch at all) -- without it the assertions + // below would pass vacuously instead of exercising that branch. it("does not treat a rejection on a waiting task as the turn's result", async () => { render( diff --git a/src/xagent/web/services/task_orchestrator.py b/src/xagent/web/services/task_orchestrator.py index fe47bc5a16..3b6b66002b 100644 --- a/src/xagent/web/services/task_orchestrator.py +++ b/src/xagent/web/services/task_orchestrator.py @@ -1976,7 +1976,8 @@ async def execute_owned_run() -> None: # a reason assembled from a key name the connector's # owner declared is dropped at the projector, so this # branch's own reason reaches the frame for two of the - # three codes and not for missing_runtime_context. + # three missing-value codes and not for + # missing_runtime_context. settlement_error = str(setup_or_run_err) client_history_message_type = CLIENT_SAFE_FAILURE_MESSAGE_TYPE broadcast_error_message = connector_runtime_client_message( From b6b83244c8aa1d3b9f1ea113e3bdde1ce03f90de Mon Sep 17 00:00:00 2001 From: Alexliu Date: Thu, 3 Sep 2026 03:13:17 +0800 Subject: [PATCH 15/28] refactor(frontend): derive the error frame's display values in one function 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. --- .../src/contexts/app-context-chat.test.tsx | 151 ++++++++++++++++ frontend/src/contexts/app-context-chat.tsx | 169 +++++++++++------- 2 files changed, 257 insertions(+), 63 deletions(-) diff --git a/frontend/src/contexts/app-context-chat.test.tsx b/frontend/src/contexts/app-context-chat.test.tsx index 33acc4a30c..34d436ee0c 100644 --- a/frontend/src/contexts/app-context-chat.test.tsx +++ b/frontend/src/contexts/app-context-chat.test.tsx @@ -145,12 +145,14 @@ vi.mock("sonner", () => ({ import { AppProvider, extractTaskControlEnvelope, + projectErrorFrameForDisplay, type AppProviderTransportConfig, useApp, } from "./app-context-chat" import { ChatStartScreen } from "@/components/chat/ChatStartScreen" import { MarkdownRenderer } from "@/components/ui/markdown-renderer" import { TASK_ERROR_EVENT, type TaskErrorEventDetail } from "@/lib/task-error-events" +import type { Translate } from "@/contexts/i18n-context" type TaskControlMessage = Parameters[0] @@ -6043,3 +6045,152 @@ describe("terminal error frames", () => { }) }) }) + +describe("error frame display projection", () => { + const translate = ((key: string) => key) as unknown as Translate + + it.each([ + { + name: "a terminal frame with a missing-value code on a trusted transport", + frame: { + type: "task_error", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + task: { id: 1, status: "failed" }, + message: "Required connector runtime context is missing.", + error: "Required connector runtime context is missing.", + code: "missing_runtime_context", + details: {}, + } as unknown as TaskControlMessage, + trustLegacyErrorProse: true, + expected: { + isTerminal: true, + taskStatus: "failed", + stopsProcessing: true, + dedupText: "Required connector runtime context is missing.", + occurrenceIdentity: "missing_runtime_context:", + bubbleContent: "common.errors.connectorRuntimeMissing", + isResult: true, + }, + }, + { + name: "a terminal frame with no code on a trusted transport", + frame: { + type: "task_error", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + task: { id: 1, status: "failed" }, + message: "Task execution failed.", + error: "Task execution failed.", + } as unknown as TaskControlMessage, + trustLegacyErrorProse: true, + expected: { + isTerminal: true, + taskStatus: "failed", + stopsProcessing: true, + dedupText: "Task execution failed.", + occurrenceIdentity: undefined, + bubbleContent: "agent.logs.event.messages.errorPrefix Task execution failed.", + isResult: true, + }, + }, + { + name: "a terminal frame with a missing-value code on an untrusted transport", + frame: { + type: "task_error", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + task: { id: 1, status: "failed" }, + message: "Required connector runtime context is missing.", + error: "Required connector runtime context is missing.", + code: "missing_runtime_context", + details: {}, + } as unknown as TaskControlMessage, + trustLegacyErrorProse: false, + expected: { + isTerminal: true, + taskStatus: "failed", + stopsProcessing: true, + dedupText: "Unknown error", + occurrenceIdentity: "missing_runtime_context:", + bubbleContent: "common.errors.connectorRuntimeMissing", + isResult: true, + }, + }, + { + name: "a terminal frame with a state version and a listed reason on a trusted transport", + frame: { + type: "task_error", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + task: { id: 1, status: "failed" }, + message: "Required runtime secret is unavailable.", + error: "Required runtime secret is unavailable.", + code: "runtime_secret_unavailable", + details: { reason: "not_provided" }, + run_id: "run-1", + state_version: 12, + } as unknown as TaskControlMessage, + trustLegacyErrorProse: true, + expected: { + isTerminal: true, + taskStatus: "failed", + stopsProcessing: true, + dedupText: "Required runtime secret is unavailable.", + occurrenceIdentity: "runtime_secret_unavailable:not_provided", + bubbleContent: "common.errors.connectorRuntimeMissing", + isResult: true, + }, + }, + { + name: "a terminal frame with a state version and no code on a trusted transport", + frame: { + type: "task_error", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + task: { id: 1, status: "failed" }, + message: "Task execution failed.", + error: "Task execution failed.", + run_id: "run-1", + state_version: 12, + } as unknown as TaskControlMessage, + trustLegacyErrorProse: true, + expected: { + isTerminal: true, + taskStatus: "failed", + stopsProcessing: true, + dedupText: "Task execution failed.", + occurrenceIdentity: undefined, + bubbleContent: "agent.logs.event.messages.errorPrefix Task execution failed.", + isResult: true, + }, + }, + { + name: "a non-terminal frame with a code and a state version on a trusted transport", + frame: { + type: "error", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + task: { id: 1, status: "running" }, + message: "Task is currently busy; please wait for the previous turn to finish.", + error: "Task is currently busy; please wait for the previous turn to finish.", + code: "connector_runtime_unavailable", + state_version: 12, + } as unknown as TaskControlMessage, + trustLegacyErrorProse: true, + expected: { + isTerminal: false, + taskStatus: "running", + stopsProcessing: false, + dedupText: "Task is currently busy; please wait for the previous turn to finish.", + occurrenceIdentity: undefined, + bubbleContent: "agent.logs.event.messages.errorPrefix Task is currently busy; please wait for the previous turn to finish.", + isResult: false, + }, + }, + ])("derives $name", ({ frame, trustLegacyErrorProse, expected }) => { + expect( + projectErrorFrameForDisplay(frame, { trustLegacyErrorProse, translate }), + ).toEqual(expected) + }) +}) diff --git a/frontend/src/contexts/app-context-chat.tsx b/frontend/src/contexts/app-context-chat.tsx index 80b3dad12d..34b7128a6e 100644 --- a/frontend/src/contexts/app-context-chat.tsx +++ b/frontend/src/contexts/app-context-chat.tsx @@ -360,7 +360,7 @@ import { generateClientMessageId, getApiUrl, getUploadApiUrl, shouldAutoOpenTask import { apiRequest, classifyUploadError, getApiErrorMessage, isJsonRecord, parseApiResponse } from "@/lib/api-wrapper" import { clientErrorTranslationKey, readClientErrorCode } from "@/lib/client-errors" import { normalizeUploadFileIds } from "@/lib/upload-file-ids" -import { useI18n } from "@/contexts/i18n-context" +import { useI18n, type Translate } from "@/contexts/i18n-context" import { normalizeTimestampMs } from "@/lib/time-utils" import { unwrapFinalAnswerContent } from "@/lib/final-answer" import { normalizeTaskCompletedMessage } from "@/lib/task-completion" @@ -972,6 +972,97 @@ const getWebSocketTaskStatus = (message: WebSocketMessage): Task["status"] | nul const shouldStopProcessingForTaskStatus = (status: unknown): boolean => isStoppedTaskStatus(status) +export type ErrorFrameDisplay = { + /** Terminal (`task_error`) or a rejection on the mixed root `error` channel. */ + isTerminal: boolean + /** Status carried by the frame, or null when it carries none. */ + taskStatus: Task["status"] | null + stopsProcessing: boolean + /** First argument to the dedup check: the server sentence, or the constant + * that stands in for it on a transport that marks legacy prose untrusted. */ + dedupText: string + /** Third argument to the dedup check. Undefined means "no identity, key on + * the text alone". */ + occurrenceIdentity: string | undefined + bubbleContent: string + isResult: boolean +} + +// One place where a frame on the error/task_error handler becomes the five +// values the handler needs: the bubble's wording, the dedup text, the dedup +// identity, the result flag, and the task status to dispatch. Each of those +// needs a different subset of "is this terminal / is legacy prose trusted / +// did a code survive / is there a state version", and deriving each subset at +// its own use site is what let five separate defects land in this handler +// across two review rounds. Pure on purpose: no dispatch, no refs, nothing +// outside its arguments, so every cell of that matrix is unit-testable +// without rendering the provider -- the same shape extractTaskControlEnvelope +// above already uses. +export const projectErrorFrameForDisplay = ( + message: WebSocketMessage, + options: { trustLegacyErrorProse: boolean; translate: Translate }, +): ErrorFrameDisplay => { + const { trustLegacyErrorProse, translate } = options + const websocketErrorCode = getWebSocketErrorCode(message) + const dedupText = websocketErrorCode + ? translate(clientErrorTranslationKey(websocketErrorCode)) + : getWebSocketErrorMessage(message, trustLegacyErrorProse) + const taskStatus = getWebSocketTaskStatus(message) + // Only task_error is terminal. Every frame of that type is emitted after + // the row has been committed FAILED -- task_orchestrator.py's settled + // branch, and websocket.py's legacy helper, which settles under + // only_if_running=True and does not broadcast when that update matches + // no row -- and task_error is also the only frame that carries the + // structured code/details pair. The root "error" type is a mixed + // channel: rejected chat messages, rejected pause and rejected resume + // all arrive on it while the viewed task is still RUNNING or + // WAITING_FOR_USER, and a rejection is not this turn's answer. + const isTerminal = message.type === "task_error" + const projection = isTerminal ? getTaskErrorProjection(message) : null + // A missing runtime value is the one failure here the user can act on, + // so the bubble says so in the viewer's own language instead of relaying + // the server's fixed English sentence. It does not name the missing key: + // the key name is configuration the connector's owner wrote, and this + // frame reaches anonymous widget and share-link visitors. An owner reads + // the key names from the per-task requirements endpoint instead. + const connectorRuntimeBubble = + projection && CONNECTOR_RUNTIME_MISSING_VALUE_CODES.has(projection.code) + ? translate('common.errors.connectorRuntimeMissing') + : null + // The string this dedup keys on is the server sentence -- or, on a + // transport that marks legacy prose untrusted, a single constant + // standing in for it -- and either way one value covers a whole error + // code: two turns failing under one code for two different admitted + // reasons -- a runtime secret not_provided, then the same secret + // store_lost -- share that key while being two distinct failures. + // Collapsing the second one now costs more than a bubble, because the + // bubble is the turn's result: that turn would end showing nothing. + // The structured (code, reason) pair is the occurrence axis instead; + // a genuine repeat of one failure still collapses. + const occurrenceIdentity = projection + ? `${projection.code}:${projection.details.reason ?? ""}` + : undefined + return { + isTerminal, + taskStatus, + stopsProcessing: shouldStopProcessingForTaskStatus(taskStatus), + dedupText, + occurrenceIdentity, + bubbleContent: + connectorRuntimeBubble + ?? `${translate('agent.logs.event.messages.errorPrefix')} ${dedupText}`, + // A terminal failure IS this turn's result: without the flag the + // conversation panel (which renders only user / isResult / system-notice + // messages) filters the bubble out and falls back to a virtual "unknown + // error" placeholder until reload. A non-terminal rejection is not, and + // flagging it would close the live progress indicator and the + // waiting-answer form of a turn that is still running, and drain this + // turn's accumulated trace events into the rejection bubble (see + // ADD_MESSAGE above). + isResult: isTerminal, + } +} + const stepsFromPlanData = (planData: unknown, existingSteps: StepExecution[]): StepExecution[] | null => { const planRecord = planData && typeof planData === "object" ? planData as Record : null const planSteps = Array.isArray(planRecord?.steps) ? planRecord.steps : null @@ -5698,67 +5789,26 @@ export function AppProvider({ break case "error": - case "task_error": + case "task_error": { console.trace('Original message:', JSON.stringify(message), 'Handler: handleMessage (error)') - const websocketErrorCode = getWebSocketErrorCode(message) - const websocketErrorMessage = websocketErrorCode - ? t(clientErrorTranslationKey(websocketErrorCode)) - : getWebSocketErrorMessage(message, trustLegacyErrorProse) - const websocketTaskStatus = getWebSocketTaskStatus(message) - // Only task_error is terminal. Every frame of that type is emitted after - // the row has been committed FAILED -- task_orchestrator.py's settled - // branch, and websocket.py's legacy helper, which settles under - // only_if_running=True and does not broadcast when that update matches - // no row -- and task_error is also the only frame that carries the - // structured code/details pair. The root "error" type is a mixed - // channel: rejected chat messages, rejected pause and rejected resume - // all arrive on it while the viewed task is still RUNNING or - // WAITING_FOR_USER, and a rejection is not this turn's answer. - const isTerminalErrorFrame = message.type === "task_error" - const taskErrorProjection = isTerminalErrorFrame - ? getTaskErrorProjection(message) - : null - - if (websocketTaskStatus) { - dispatch({ type: "UPDATE_TASK_STATUS", payload: { status: websocketTaskStatus } }) + const errorFrame = projectErrorFrameForDisplay(message, { + trustLegacyErrorProse, + translate: t, + }) + + if (errorFrame.taskStatus) { + dispatch({ type: "UPDATE_TASK_STATUS", payload: { status: errorFrame.taskStatus } }) dispatch({ type: "TRIGGER_TASK_UPDATE" }) } - if (shouldStopProcessingForTaskStatus(websocketTaskStatus)) { + if (errorFrame.stopsProcessing) { dispatch({ type: "SET_PROCESSING", payload: false }) } - // A missing runtime value is the one failure here the user can act on, - // so the bubble says so in the viewer's own language instead of relaying - // the server's fixed English sentence. It does not name the missing key: - // the key name is configuration the connector's owner wrote, and this - // frame reaches anonymous widget and share-link visitors. An owner reads - // the key names from the per-task requirements endpoint instead. - const connectorRuntimeBubble = - taskErrorProjection - && CONNECTOR_RUNTIME_MISSING_VALUE_CODES.has(taskErrorProjection.code) - ? t('common.errors.connectorRuntimeMissing') - : null - const errorBubbleContent = connectorRuntimeBubble - ?? `${t('agent.logs.event.messages.errorPrefix')} ${websocketErrorMessage}` - - // The string this dedup keys on is the server sentence -- or, on a - // transport that marks legacy prose untrusted, a single constant - // standing in for it -- and either way one value covers a whole error - // code: two turns failing under one code for two different admitted - // reasons -- a runtime secret not_provided, then the same secret - // store_lost -- share that key while being two distinct failures. - // Collapsing the second one now costs more than a bubble, because the - // bubble is the turn's result: that turn would end showing nothing. - // The structured (code, reason) pair is the occurrence axis instead; - // a genuine repeat of one failure still collapses. - const errorOccurrenceIdentity = taskErrorProjection - ? `${taskErrorProjection.code}:${taskErrorProjection.details.reason ?? ""}` - : undefined if ( !isDuplicateMessageForViewedTask( - websocketErrorMessage, + errorFrame.dedupText, "agent-error", - errorOccurrenceIdentity, + errorFrame.occurrenceIdentity, ) ) { dispatch({ @@ -5766,22 +5816,15 @@ export function AppProvider({ payload: { id: generateMessageId("msg-error"), role: "assistant", - content: errorBubbleContent, + content: errorFrame.bubbleContent, timestamp: message.timestamp, status: "failed", - // A terminal failure IS this turn's result: without the flag the - // conversation panel (which renders only user / isResult / - // system-notice messages) filters the bubble out and falls back to - // a virtual "unknown error" placeholder until reload. A - // non-terminal rejection is not, and flagging it would close the - // live progress indicator and the waiting-answer form of a turn - // that is still running, and drain this turn's accumulated trace - // events into the rejection bubble (see ADD_MESSAGE above). - isResult: isTerminalErrorFrame, + isResult: errorFrame.isResult, }, }) } break + } case "message_received": console.trace('Original message:', JSON.stringify(message), 'Handler: handleMessage (message_received)') From 4b8b1ebc4a55cc38bc51fb41715e20a94ae90cb0 Mon Sep 17 00:00:00 2001 From: Alexliu Date: Thu, 3 Sep 2026 03:23:23 +0800 Subject: [PATCH 16/28] fix(frontend): dedup terminal errors by the frame's own state version 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. --- .../src/contexts/app-context-chat.test.tsx | 283 ++++++++++++++++-- frontend/src/contexts/app-context-chat.tsx | 45 ++- 2 files changed, 295 insertions(+), 33 deletions(-) diff --git a/frontend/src/contexts/app-context-chat.test.tsx b/frontend/src/contexts/app-context-chat.test.tsx index 34d436ee0c..47e0d1e2a6 100644 --- a/frontend/src/contexts/app-context-chat.test.tsx +++ b/frontend/src/contexts/app-context-chat.test.tsx @@ -5940,13 +5940,15 @@ describe("terminal error frames", () => { ) }) - // The dedup key is the server sentence, and one sentence covers a whole - // code. These two turns fail under one code for two different admitted - // reasons -- the same runtime secret, first never provided, then lost from - // its store -- so they share that key while being two distinct failures. - // Without the (code, reason) identity the second bubble vanishes, and that - // bubble is the turn's result. - it("keeps both bubbles when one code fails for two different reasons", async () => { + // The dedup identity is the frame's own (run_id, state_version), not its + // code or reason. These two turns fail under one code for two different + // admitted reasons -- the same runtime secret, first never provided, then + // lost from its store -- and each settlement bumps state_version at least + // once (the retry takes the lease FAILED -> RUNNING, then settles RUNNING + // -> FAILED), so the second turn's version is strictly greater. Two + // distinct versions mean two distinct identities, and the bubble is the + // turn's result. + it("keeps both bubbles when one code fails twice at different state versions", async () => { render( @@ -5957,7 +5959,7 @@ describe("terminal error frames", () => { const onMessage = webSocketOptions.current?.onMessage expect(onMessage).toBeDefined() - const frameForReason = (reason: string, timestamp: string) => ({ + const frameForReason = (reason: string, timestamp: string, stateVersion: number) => ({ type: "task_error", timestamp, task_id: 1, @@ -5969,10 +5971,12 @@ describe("terminal error frames", () => { error: "Required runtime secret is unavailable.", code: "runtime_secret_unavailable", details: { reason }, + run_id: "run-1", + state_version: stateVersion, }) as TestWebSocketMessage act(() => { - onMessage?.(frameForReason("not_provided", "2026-05-27T05:00:02Z")) + onMessage?.(frameForReason("not_provided", "2026-05-27T05:00:02Z", 12)) }) await waitFor(() => { expect(screen.getByTestId("messages").textContent).toContain( @@ -5981,7 +5985,7 @@ describe("terminal error frames", () => { }) act(() => { - onMessage?.(frameForReason("store_lost", "2026-05-27T05:00:03Z")) + onMessage?.(frameForReason("store_lost", "2026-05-27T05:00:03Z", 14)) }) await waitFor(() => { @@ -5996,9 +6000,10 @@ describe("terminal error frames", () => { }) }) - // The other half of the same contract: a genuine repeat of one failure is - // still collapsed, so the identity did not simply disable deduplication. - it("still collapses a repeat of the same code and reason", async () => { + // The other half of the same contract: a genuine redelivery of one + // settlement (same run_id, same state_version) is still collapsed, so the + // identity did not simply disable deduplication. + it("still collapses a redelivery of one settlement", async () => { render( @@ -6018,6 +6023,8 @@ describe("terminal error frames", () => { error: "Required runtime secret is unavailable.", code: "runtime_secret_unavailable", details: { reason: "not_provided" }, + run_id: "run-1", + state_version: 12, } as TestWebSocketMessage act(() => { @@ -6044,6 +6051,233 @@ describe("terminal error frames", () => { expect(bubbles).toHaveLength(1) }) }) + + // Blocking issue 1's direct anchor: two failed turns under the same code + // and the same reason, distinguished only by their state_version. Keying + // on the failure's class -- the code, the reason, or the rendered + // sentence -- cannot tell these apart; keying on the frame's own state + // tuple can. + it("keeps both bubbles when one failure repeats on the next turn", async () => { + render( + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + const frameAtVersion = (stateVersion: number, timestamp: string) => ({ + type: "task_error", + timestamp, + task_id: 1, + task: { id: 1, status: "failed" }, + message: "Required runtime secret is unavailable.", + error: "Required runtime secret is unavailable.", + code: "runtime_secret_unavailable", + details: { reason: "not_provided" }, + run_id: "run-1", + state_version: stateVersion, + }) as TestWebSocketMessage + + act(() => { + onMessage?.(frameAtVersion(12, "2026-05-27T05:00:02Z")) + }) + await waitFor(() => { + expect(screen.getByTestId("messages").textContent).toContain( + "common.errors.connectorRuntimeMissing" + ) + }) + + act(() => { + onMessage?.(frameAtVersion(14, "2026-05-27T05:00:03Z")) + }) + + await waitFor(() => { + const messages = JSON.parse( + screen.getByTestId("messages").textContent || "[]" + ) + const bubbles = messages.filter( + (m: { content: string }) => + m.content === "common.errors.connectorRuntimeMissing" + ) + expect(bubbles).toHaveLength(2) + }) + }) + + // Blocking issue 2's direct anchor: two failed turns that carry no code at + // all -- the rendered sentence is identical on both -- distinguished only + // by their state_version. Before this change the dedup key was the + // rendered sentence alone, so the second of these vanished and that bubble + // is the turn's result. + it("keeps both bubbles for two generic failures on consecutive turns", async () => { + render( + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + const frameAtVersion = (stateVersion: number, timestamp: string) => ({ + type: "task_error", + timestamp, + task_id: 1, + task: { id: 1, status: "failed" }, + message: "Task execution failed.", + error: "Task execution failed.", + run_id: "run-1", + state_version: stateVersion, + }) as TestWebSocketMessage + + act(() => { + onMessage?.(frameAtVersion(12, "2026-05-27T05:00:02Z")) + }) + await waitFor(() => { + expect(screen.getByTestId("messages").textContent).toContain( + "Task execution failed." + ) + }) + + act(() => { + onMessage?.(frameAtVersion(14, "2026-05-27T05:00:03Z")) + }) + + await waitFor(() => { + const messages = JSON.parse( + screen.getByTestId("messages").textContent || "[]" + ) + const bubbles = messages.filter( + (m: { content: string }) => + typeof m.content === "string" && m.content.includes("Task execution failed.") + ) + expect(bubbles).toHaveLength(2) + }) + }) + + // The widest form of blocking issue 2: a generic failure on an untrusted + // transport reads the same fixed "Unknown error" constant regardless of + // what precedes it, so a version-blind identity would collapse it into + // whatever coded failure happened to precede it within the window. The + // frame's own state tuple tells these two turns apart even though their + // rendered sentence -- and, on this transport, their entire dedup text -- + // is identical. + it("keeps a generic failure that follows a coded one on an untrusted transport", async () => { + render( + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + act(() => { + onMessage?.({ + type: "task_error", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + task: { id: 1, status: "failed" }, + message: "Required connector runtime context is missing.", + error: "Required connector runtime context is missing.", + code: "missing_runtime_context", + details: {}, + run_id: "run-1", + state_version: 12, + } as TestWebSocketMessage) + }) + await waitFor(() => { + expect(screen.getByTestId("messages").textContent).toContain( + "common.errors.connectorRuntimeMissing" + ) + }) + + act(() => { + onMessage?.({ + type: "task_error", + timestamp: "2026-05-27T05:00:03Z", + task_id: 1, + task: { id: 1, status: "failed" }, + message: "Task execution failed.", + error: "Task execution failed.", + run_id: "run-1", + state_version: 14, + } as TestWebSocketMessage) + }) + + await waitFor(() => { + const messages = JSON.parse( + screen.getByTestId("messages").textContent || "[]" + ) + expect(messages).toHaveLength(2) + const codedBubbles = messages.filter( + (m: { content: string }) => + m.content === "common.errors.connectorRuntimeMissing" + ) + const genericBubbles = messages.filter( + (m: { content: string }) => + typeof m.content === "string" && m.content.includes("Unknown error") + ) + expect(codedBubbles).toHaveLength(1) + expect(genericBubbles).toHaveLength(1) + }) + }) + + // I-B's witness at the integration level: two non-terminal rejections each + // carry a version ("error" is in VERSIONED_TASK_EVENT_TYPES too), but the + // terminal-only identity must not leak into this channel -- if it did, two + // different versions would make these look like two distinct rejections + // instead of one repeated one. + it("still collapses two identical non-terminal rejections", async () => { + render( + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + const rejectionAtVersion = (stateVersion: number, timestamp: string) => ({ + type: "error", + timestamp, + task_id: 1, + task: { id: 1, status: "running" }, + message: "Task is currently busy; please wait for the previous turn to finish.", + error_code: "task_busy", + run_id: "run-1", + state_version: stateVersion, + }) as TestWebSocketMessage + + act(() => { + onMessage?.(rejectionAtVersion(12, "2026-05-27T05:00:02Z")) + }) + await waitFor(() => { + expect(screen.getByTestId("messages").textContent).toContain( + "clientErrors.taskBusy" + ) + }) + + act(() => { + onMessage?.(rejectionAtVersion(14, "2026-05-27T05:00:03Z")) + }) + + await waitFor(() => { + const messages = JSON.parse( + screen.getByTestId("messages").textContent || "[]" + ) + const bubbles = messages.filter( + (m: { content: string }) => + typeof m.content === "string" && m.content.includes("clientErrors.taskBusy") + ) + expect(bubbles).toHaveLength(1) + }) + }) }) describe("error frame display projection", () => { @@ -6068,7 +6302,10 @@ describe("error frame display projection", () => { taskStatus: "failed", stopsProcessing: true, dedupText: "Required connector runtime context is missing.", - occurrenceIdentity: "missing_runtime_context:", + // No run_id/state_version on this fixture, same as production for a + // frame the version gate would drop once any versioned event has + // been seen -- see I-A and cell 2 below. + occurrenceIdentity: undefined, bubbleContent: "common.errors.connectorRuntimeMissing", isResult: true, }, @@ -6089,6 +6326,8 @@ describe("error frame display projection", () => { taskStatus: "failed", stopsProcessing: true, dedupText: "Task execution failed.", + // This is the witness for withholding the identity when the frame + // has no state version -- see I-A. occurrenceIdentity: undefined, bubbleContent: "agent.logs.event.messages.errorPrefix Task execution failed.", isResult: true, @@ -6112,7 +6351,7 @@ describe("error frame display projection", () => { taskStatus: "failed", stopsProcessing: true, dedupText: "Unknown error", - occurrenceIdentity: "missing_runtime_context:", + occurrenceIdentity: undefined, bubbleContent: "common.errors.connectorRuntimeMissing", isResult: true, }, @@ -6137,7 +6376,7 @@ describe("error frame display projection", () => { taskStatus: "failed", stopsProcessing: true, dedupText: "Required runtime secret is unavailable.", - occurrenceIdentity: "runtime_secret_unavailable:not_provided", + occurrenceIdentity: "run-1:12", bubbleContent: "common.errors.connectorRuntimeMissing", isResult: true, }, @@ -6160,7 +6399,7 @@ describe("error frame display projection", () => { taskStatus: "failed", stopsProcessing: true, dedupText: "Task execution failed.", - occurrenceIdentity: undefined, + occurrenceIdentity: "run-1:12", bubbleContent: "agent.logs.event.messages.errorPrefix Task execution failed.", isResult: true, }, @@ -6183,14 +6422,22 @@ describe("error frame display projection", () => { taskStatus: "running", stopsProcessing: false, dedupText: "Task is currently busy; please wait for the previous turn to finish.", + // This is the witness for keeping the terminal-only identity out of + // the rejection channel -- see I-B. occurrenceIdentity: undefined, bubbleContent: "agent.logs.event.messages.errorPrefix Task is currently busy; please wait for the previous turn to finish.", isResult: false, }, }, ])("derives $name", ({ frame, trustLegacyErrorProse, expected }) => { + // The envelope is parsed here rather than hand-built, matching the one + // call site in production (app-context-chat.tsx, before the switch): a + // hand-built envelope would be non-production-shaped input, and this is + // also what makes the no-version cell below (see its comment) actually + // exercise stateVersion being undefined rather than a value we chose. + const controlEnvelope = extractTaskControlEnvelope(frame) expect( - projectErrorFrameForDisplay(frame, { trustLegacyErrorProse, translate }), + projectErrorFrameForDisplay(frame, { trustLegacyErrorProse, translate, controlEnvelope }), ).toEqual(expected) }) }) diff --git a/frontend/src/contexts/app-context-chat.tsx b/frontend/src/contexts/app-context-chat.tsx index 34b7128a6e..87ea856e5c 100644 --- a/frontend/src/contexts/app-context-chat.tsx +++ b/frontend/src/contexts/app-context-chat.tsx @@ -1000,9 +1000,13 @@ export type ErrorFrameDisplay = { // above already uses. export const projectErrorFrameForDisplay = ( message: WebSocketMessage, - options: { trustLegacyErrorProse: boolean; translate: Translate }, + options: { + trustLegacyErrorProse: boolean + translate: Translate + controlEnvelope: TaskControlEnvelope + }, ): ErrorFrameDisplay => { - const { trustLegacyErrorProse, translate } = options + const { trustLegacyErrorProse, translate, controlEnvelope } = options const websocketErrorCode = getWebSocketErrorCode(message) const dedupText = websocketErrorCode ? translate(clientErrorTranslationKey(websocketErrorCode)) @@ -1029,19 +1033,29 @@ export const projectErrorFrameForDisplay = ( projection && CONNECTOR_RUNTIME_MISSING_VALUE_CODES.has(projection.code) ? translate('common.errors.connectorRuntimeMissing') : null - // The string this dedup keys on is the server sentence -- or, on a - // transport that marks legacy prose untrusted, a single constant - // standing in for it -- and either way one value covers a whole error - // code: two turns failing under one code for two different admitted - // reasons -- a runtime secret not_provided, then the same secret - // store_lost -- share that key while being two distinct failures. - // Collapsing the second one now costs more than a bubble, because the - // bubble is the turn's result: that turn would end showing nothing. - // The structured (code, reason) pair is the occurrence axis instead; - // a genuine repeat of one failure still collapses. - const occurrenceIdentity = projection - ? `${projection.code}:${projection.details.reason ?? ""}` - : undefined + // The dedup identity has to name WHICH occurrence this frame reports, not + // which class of failure it belongs to. broadcast_to_task stamps every frame + // of this type with the row's (run_id, state_version) pair before it goes + // out -- task_error is in websocket.py's _VERSIONED_TASK_EVENT_TYPES -- and + // state_version is bumped by every control transition that actually changes + // (status, control_state). So one settlement broadcast twice carries one + // version and still collapses, while two failed turns are at least two + // versions apart (the retry takes the lease FAILED -> RUNNING, then settles + // RUNNING -> FAILED) and both are shown. Keying on the failure's class + // instead -- the code, the reason, or the rendered sentence -- cannot tell + // those two apart, and on this handler the collapsed frame is the turn's + // result. The identity is withheld when the frame carries no version (the + // row was already gone when it was broadcast, so no state tuple was + // attached), which falls back to keying on the text alone: the version gate + // above drops such a frame once any versioned event has been seen for the + // task, and when none has, two of them key on the same text and the second + // still collapses -- the behaviour that predates this change. Withholding + // the identity is the honest answer there; attaching a state tuple needs + // the row, and a settled FAILED task has one. + const occurrenceIdentity = + isTerminal && controlEnvelope.stateVersion !== undefined + ? `${controlEnvelope.runId ?? ""}:${controlEnvelope.stateVersion}` + : undefined return { isTerminal, taskStatus, @@ -5794,6 +5808,7 @@ export function AppProvider({ const errorFrame = projectErrorFrameForDisplay(message, { trustLegacyErrorProse, translate: t, + controlEnvelope, }) if (errorFrame.taskStatus) { From 08c37391f9ef4cf232e9753f9b66fa3e5514bfc9 Mon Sep 17 00:00:00 2001 From: Alexliu Date: Thu, 3 Sep 2026 03:30:15 +0800 Subject: [PATCH 17/28] fix(frontend): localize connector runtime codes through the client error 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 (#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. --- .../src/contexts/app-context-chat.test.tsx | 128 +++++++++++++++--- frontend/src/contexts/app-context-chat.tsx | 35 ++--- frontend/src/i18n/locales/en.ts | 6 +- frontend/src/i18n/locales/zh.ts | 6 +- frontend/src/lib/client-errors.test.ts | 5 + frontend/src/lib/client-errors.ts | 38 ++++++ 6 files changed, 174 insertions(+), 44 deletions(-) diff --git a/frontend/src/contexts/app-context-chat.test.tsx b/frontend/src/contexts/app-context-chat.test.tsx index 47e0d1e2a6..beb4cafd1a 100644 --- a/frontend/src/contexts/app-context-chat.test.tsx +++ b/frontend/src/contexts/app-context-chat.test.tsx @@ -5841,18 +5841,18 @@ describe("terminal error frames", () => { await waitFor(() => { expect(screen.getByTestId("messages").textContent).toContain( - "common.errors.connectorRuntimeMissing" + "clientErrors.runtimeSecretUnavailable" ) }) const messages = JSON.parse(screen.getByTestId("messages").textContent || "[]") const bubble = messages.find((m: { content: string }) => - m.content.includes("common.errors.connectorRuntimeMissing") + m.content.includes("clientErrors.runtimeSecretUnavailable") ) // Same toBe as the test below, on the other input: this branch is reached // with a listed bare reason rather than an empty details, and an // interpolation regression has to be caught on both. - expect(bubble?.content).toBe("common.errors.connectorRuntimeMissing") + expect(bubble?.content).toBe("clientErrors.runtimeSecretUnavailable") }) // What the server now sends for a missing declared context key: the code @@ -5888,24 +5888,25 @@ describe("terminal error frames", () => { await waitFor(() => { expect(screen.getByTestId("messages").textContent).toContain( - "common.errors.connectorRuntimeMissing" + "clientErrors.missingRuntimeContext" ) }) const messages = JSON.parse(screen.getByTestId("messages").textContent || "[]") const bubble = messages.find((m: { content: string }) => - m.content.includes("common.errors.connectorRuntimeMissing") + m.content.includes("clientErrors.missingRuntimeContext") ) // No prefix: this wording replaces the server sentence rather than // decorating it. Exactly the key, with nothing appended: no variable was // interpolated, so no key name can be in the rendered text. - expect(bubble?.content).toBe("common.errors.connectorRuntimeMissing") + expect(bubble?.content).toBe("clientErrors.missingRuntimeContext") expect(bubble?.isResult).toBe(true) }) - // connector_runtime_unavailable reports a server-side component being - // down, which the user cannot act on, so it keeps the generic prefix. - it("keeps the plain failure wording for a non-missing-value code", async () => { + // The frame's code decides the wording -- this code reports a server-side + // component being down, and it has its own table entry rather than the + // generic prefix. + it("uses the code's own wording when a server-side component is down", async () => { render( @@ -5931,12 +5932,15 @@ describe("terminal error frames", () => { await waitFor(() => { expect(screen.getByTestId("messages").textContent).toContain( - "Connector runtime is unavailable." + "clientErrors.connectorRuntimeUnavailable" ) }) + // The server sentence is no longer relayed: this code has its own table + // entry now, on every transport, so the logged-in audience no longer sees + // a different wording than the one an untrusted transport gets. expect(screen.getByTestId("messages").textContent).not.toContain( - "common.errors.connectorRuntimeMissing" + "Connector runtime is unavailable." ) }) @@ -5980,7 +5984,7 @@ describe("terminal error frames", () => { }) await waitFor(() => { expect(screen.getByTestId("messages").textContent).toContain( - "common.errors.connectorRuntimeMissing" + "clientErrors.runtimeSecretUnavailable" ) }) @@ -5994,7 +5998,7 @@ describe("terminal error frames", () => { ) const bubbles = messages.filter( (m: { content: string }) => - m.content === "common.errors.connectorRuntimeMissing" + m.content === "clientErrors.runtimeSecretUnavailable" ) expect(bubbles).toHaveLength(2) }) @@ -6032,7 +6036,7 @@ describe("terminal error frames", () => { }) await waitFor(() => { expect(screen.getByTestId("messages").textContent).toContain( - "common.errors.connectorRuntimeMissing" + "clientErrors.runtimeSecretUnavailable" ) }) @@ -6046,7 +6050,7 @@ describe("terminal error frames", () => { ) const bubbles = messages.filter( (m: { content: string }) => - m.content === "common.errors.connectorRuntimeMissing" + m.content === "clientErrors.runtimeSecretUnavailable" ) expect(bubbles).toHaveLength(1) }) @@ -6086,7 +6090,7 @@ describe("terminal error frames", () => { }) await waitFor(() => { expect(screen.getByTestId("messages").textContent).toContain( - "common.errors.connectorRuntimeMissing" + "clientErrors.runtimeSecretUnavailable" ) }) @@ -6100,7 +6104,7 @@ describe("terminal error frames", () => { ) const bubbles = messages.filter( (m: { content: string }) => - m.content === "common.errors.connectorRuntimeMissing" + m.content === "clientErrors.runtimeSecretUnavailable" ) expect(bubbles).toHaveLength(2) }) @@ -6192,7 +6196,7 @@ describe("terminal error frames", () => { }) await waitFor(() => { expect(screen.getByTestId("messages").textContent).toContain( - "common.errors.connectorRuntimeMissing" + "clientErrors.missingRuntimeContext" ) }) @@ -6216,7 +6220,7 @@ describe("terminal error frames", () => { expect(messages).toHaveLength(2) const codedBubbles = messages.filter( (m: { content: string }) => - m.content === "common.errors.connectorRuntimeMissing" + m.content === "clientErrors.missingRuntimeContext" ) const genericBubbles = messages.filter( (m: { content: string }) => @@ -6278,6 +6282,86 @@ describe("terminal error frames", () => { expect(bubbles).toHaveLength(1) }) }) + + // Blocking issue 3's direct anchor: connector_runtime_unavailable now has + // its own table entry, so it survives a transport that marks legacy prose + // untrusted the same way the three previously-curated codes always did. + // Before this, only those three had client-side wording; every other code, + // this one included, fell through to "Unknown error" for an anonymous + // widget or share-link visitor. + it("localizes a connector-runtime code for an untrusted transport", async () => { + render( + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + act(() => { + onMessage?.({ + type: "task_error", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + task: { id: 1, status: "failed" }, + message: "Connector runtime is unavailable.", + error: "Connector runtime is unavailable.", + code: "connector_runtime_unavailable", + details: { reason: "team_env_resolution_failed" }, + } as TestWebSocketMessage) + }) + + await waitFor(() => { + expect(screen.getByTestId("messages").textContent).toContain( + "clientErrors.connectorRuntimeUnavailable" + ) + }) + + expect(screen.getByTestId("messages").textContent).not.toContain( + "Unknown error" + ) + }) + + // The boundary the client wording table draws: a code the table does not + // list keeps the generic prefixed wording, even though it is a member of + // the same connector-runtime family. connector_not_found is real -- it is + // one of the ten V1ErrorCode connector-runtime members -- but its only + // raise site is in the payload-validation stage before a task row exists, + // so it never reaches a terminal frame in production; this pins what the + // table does when a code outside its five members shows up anyway, not a + // claim that this specific code will arrive on this path. + it("falls back to generic wording for a code the table does not list", async () => { + render( + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + act(() => { + onMessage?.({ + type: "task_error", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + task: { id: 1, status: "failed" }, + message: "Connector could not be found.", + error: "Connector could not be found.", + code: "connector_not_found", + details: {}, + } as TestWebSocketMessage) + }) + + await waitFor(() => { + expect(screen.getByTestId("messages").textContent).toContain( + "agent.logs.event.messages.errorPrefix Unknown error" + ) + }) + }) }) describe("error frame display projection", () => { @@ -6306,7 +6390,7 @@ describe("error frame display projection", () => { // frame the version gate would drop once any versioned event has // been seen -- see I-A and cell 2 below. occurrenceIdentity: undefined, - bubbleContent: "common.errors.connectorRuntimeMissing", + bubbleContent: "clientErrors.missingRuntimeContext", isResult: true, }, }, @@ -6352,7 +6436,7 @@ describe("error frame display projection", () => { stopsProcessing: true, dedupText: "Unknown error", occurrenceIdentity: undefined, - bubbleContent: "common.errors.connectorRuntimeMissing", + bubbleContent: "clientErrors.missingRuntimeContext", isResult: true, }, }, @@ -6377,7 +6461,7 @@ describe("error frame display projection", () => { stopsProcessing: true, dedupText: "Required runtime secret is unavailable.", occurrenceIdentity: "run-1:12", - bubbleContent: "common.errors.connectorRuntimeMissing", + bubbleContent: "clientErrors.runtimeSecretUnavailable", isResult: true, }, }, diff --git a/frontend/src/contexts/app-context-chat.tsx b/frontend/src/contexts/app-context-chat.tsx index 87ea856e5c..0b7448662c 100644 --- a/frontend/src/contexts/app-context-chat.tsx +++ b/frontend/src/contexts/app-context-chat.tsx @@ -928,16 +928,6 @@ const getWebSocketErrorCode = (message: WebSocketMessage) => { return errorCode.present ? readClientErrorCode(errorCode.value) : null } -// The task_error codes that mean a connector is still missing a runtime value -// the user can supply. The other connector-runtime code -// (connector_runtime_unavailable) reports a server-side component being down, -// which the user cannot act on, so it keeps the generic failure wording. -const CONNECTOR_RUNTIME_MISSING_VALUE_CODES = new Set([ - "missing_runtime_context", - "runtime_secret_unavailable", - "scheduled_secret_unavailable", -]) - // The frame deliberately carries nothing connector-specific beyond the code: // its audience includes anonymous widget and share-link visitors, and the // server's reason whitelist admits only fixed strings it controls -- never a @@ -1023,16 +1013,21 @@ export const projectErrorFrameForDisplay = ( // WAITING_FOR_USER, and a rejection is not this turn's answer. const isTerminal = message.type === "task_error" const projection = isTerminal ? getTaskErrorProjection(message) : null - // A missing runtime value is the one failure here the user can act on, - // so the bubble says so in the viewer's own language instead of relaying - // the server's fixed English sentence. It does not name the missing key: - // the key name is configuration the connector's owner wrote, and this - // frame reaches anonymous widget and share-link visitors. An owner reads - // the key names from the per-task requirements endpoint instead. - const connectorRuntimeBubble = - projection && CONNECTOR_RUNTIME_MISSING_VALUE_CODES.has(projection.code) - ? translate('common.errors.connectorRuntimeMissing') - : null + // The frame's own code decides the wording, and one code has one sentence + // for every audience. This is the same table the root error channel already + // uses for its error_code field, extended with the connector-runtime codes + // that reach this frame -- not a second vocabulary beside it. It also fixes + // what the relayed sentence could not: on a transport that marks legacy + // prose untrusted, getWebSocketErrorMessage returns a constant by design + // (#1938: never render server free text there), so before this the curated + // sentence existed for three codes and every other code read "Unknown + // error". Nothing here relays server prose; the wording is the client's own, + // selected by code. A code the table does not list keeps the generic + // prefixed wording. + const projectedCode = projection ? readClientErrorCode(projection.code) : null + const connectorRuntimeBubble = projectedCode + ? translate(clientErrorTranslationKey(projectedCode)) + : null // The dedup identity has to name WHICH occurrence this frame reports, not // which class of failure it belongs to. broadcast_to_task stamps every frame // of this type with the row's (run_id, state_version) pair before it goes diff --git a/frontend/src/i18n/locales/en.ts b/frontend/src/i18n/locales/en.ts index e393b6534c..e784d08bb4 100644 --- a/frontend/src/i18n/locales/en.ts +++ b/frontend/src/i18n/locales/en.ts @@ -22,6 +22,11 @@ const en = { uploadTooLarge: "File is too large. Please reduce the upload size and try again.", uploadProxyError: "Upload failed before reaching the application. Please check the server upload limit.", uploadFailed: "Upload failed. Please try again.", + missingRuntimeContext: "This connector needs additional runtime input before it can run.", + runtimeSecretUnavailable: "This connector needs a runtime credential that is not available.", + scheduledSecretUnavailable: "A scheduled run needs a runtime credential that is not available.", + invalidRuntimeContext: "This connector's runtime input is not valid, so the task could not run.", + connectorRuntimeUnavailable: "A service this connector needs is unavailable. Please try again later.", }, common: { optional: "(Optional)", @@ -66,7 +71,6 @@ const en = { errors: { unknown: "Unknown error", taskFailed: "Something went wrong. Please try again.", - connectorRuntimeMissing: "This connector needs additional runtime input before it can run.", }, }, voiceInput: { diff --git a/frontend/src/i18n/locales/zh.ts b/frontend/src/i18n/locales/zh.ts index 97366e2cd0..4b1d31e54b 100644 --- a/frontend/src/i18n/locales/zh.ts +++ b/frontend/src/i18n/locales/zh.ts @@ -22,6 +22,11 @@ const zh = { uploadTooLarge: "文件过大，请减小上传大小后重试。", uploadProxyError: "上传请求未到达应用，请检查服务器的上传大小限制。", uploadFailed: "上传失败，请重试。", + missingRuntimeContext: "这个连接器需要额外的运行时输入才能运行。", + runtimeSecretUnavailable: "这个连接器需要的运行时凭据当前不可用。", + scheduledSecretUnavailable: "定时运行需要的运行时凭据当前不可用。", + invalidRuntimeContext: "这个连接器的运行时输入无效，任务无法运行。", + connectorRuntimeUnavailable: "这个连接器依赖的服务当前不可用，请稍后重试。", }, common: { optional: "(可选)", @@ -66,7 +71,6 @@ const zh = { errors: { unknown: "未知错误", taskFailed: "出了点问题，请重试。", - connectorRuntimeMissing: "这个连接器需要额外的运行时输入，请补充后重试。", }, }, voiceInput: { diff --git a/frontend/src/lib/client-errors.test.ts b/frontend/src/lib/client-errors.test.ts index bee70a6b0f..80d58ee62f 100644 --- a/frontend/src/lib/client-errors.test.ts +++ b/frontend/src/lib/client-errors.test.ts @@ -30,6 +30,11 @@ describe("client error wire contract", () => { ["upload_too_large", "clientErrors.uploadTooLarge", "File is too large. Please reduce the upload size and try again."], ["upload_proxy_error", "clientErrors.uploadProxyError", "Upload failed before reaching the application. Please check the server upload limit."], ["upload_failed", "clientErrors.uploadFailed", "Upload failed. Please try again."], + ["missing_runtime_context", "clientErrors.missingRuntimeContext", "This connector needs additional runtime input before it can run."], + ["runtime_secret_unavailable", "clientErrors.runtimeSecretUnavailable", "This connector needs a runtime credential that is not available."], + ["scheduled_secret_unavailable", "clientErrors.scheduledSecretUnavailable", "A scheduled run needs a runtime credential that is not available."], + ["invalid_runtime_context", "clientErrors.invalidRuntimeContext", "This connector's runtime input is not valid, so the task could not run."], + ["connector_runtime_unavailable", "clientErrors.connectorRuntimeUnavailable", "A service this connector needs is unavailable. Please try again later."], ] as const)("maps %s to a typed translation key", (code, key, fallback) => { expect(readClientErrorCode(code)).toBe(code) expect(clientErrorTranslationKey(code)).toBe(key) diff --git a/frontend/src/lib/client-errors.ts b/frontend/src/lib/client-errors.ts index 6edcab0bcc..75285cfce9 100644 --- a/frontend/src/lib/client-errors.ts +++ b/frontend/src/lib/client-errors.ts @@ -23,6 +23,34 @@ const CLIENT_ERROR_CODES = [ "upload_too_large", "upload_proxy_error", "upload_failed", + // Connector-runtime codes that reach the client on a terminal task_error + // frame's `code` field. They come from V1ErrorCode rather than the backend's + // ClientErrorCode enum -- this list is already a superset of that enum (the + // three upload_* codes are client-side only) and stays one table so a code + // has one wording for every audience. Only codes with a producer that can + // reach that frame today are listed, the same rule the server's reason + // whitelist states about itself: a listed code nothing produces is an entry + // with no expiry date. Which codes those are is a fact about this + // repository's raise sites, not a property the wire holds -- the field is + // typed as a bare string and validated only against the full V1ErrorCode + // set (see websocket.py's own note above that check), and a resolver + // installed through set_connector_runtime_resolver lives outside this + // repository and can raise any member. The other five members of the + // connector-runtime family are absent because nothing here produces them on + // this path: two have no raise site in this repository at all, and 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 this table does not list keeps the generic + // prefixed wording. + "missing_runtime_context", + "runtime_secret_unavailable", + "scheduled_secret_unavailable", + "invalid_runtime_context", + "connector_runtime_unavailable", ] as const export type ClientErrorCode = (typeof CLIENT_ERROR_CODES)[number] @@ -50,6 +78,11 @@ const CLIENT_ERROR_TRANSLATION_KEYS: Record = { upload_too_large: "clientErrors.uploadTooLarge", upload_proxy_error: "clientErrors.uploadProxyError", upload_failed: "clientErrors.uploadFailed", + missing_runtime_context: "clientErrors.missingRuntimeContext", + runtime_secret_unavailable: "clientErrors.runtimeSecretUnavailable", + scheduled_secret_unavailable: "clientErrors.scheduledSecretUnavailable", + invalid_runtime_context: "clientErrors.invalidRuntimeContext", + connector_runtime_unavailable: "clientErrors.connectorRuntimeUnavailable", } const CLIENT_ERROR_FALLBACKS: Record = { @@ -75,6 +108,11 @@ const CLIENT_ERROR_FALLBACKS: Record = { upload_too_large: "File is too large. Please reduce the upload size and try again.", upload_proxy_error: "Upload failed before reaching the application. Please check the server upload limit.", upload_failed: "Upload failed. Please try again.", + missing_runtime_context: "This connector needs additional runtime input before it can run.", + runtime_secret_unavailable: "This connector needs a runtime credential that is not available.", + scheduled_secret_unavailable: "A scheduled run needs a runtime credential that is not available.", + invalid_runtime_context: "This connector's runtime input is not valid, so the task could not run.", + connector_runtime_unavailable: "A service this connector needs is unavailable. Please try again later.", } const CLIENT_ERROR_CODE_SET = new Set(CLIENT_ERROR_CODES) From a59c5ca6f760b8e72504b5da3d2419894e07231f Mon Sep 17 00:00:00 2001 From: Alexliu Date: Thu, 3 Sep 2026 03:49:17 +0800 Subject: [PATCH 18/28] fix(web): type-gate the code before the closed-set membership test 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.`, 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. --- .../src/contexts/app-context-chat.test.tsx | 33 ++++ src/xagent/web/api/websocket.py | 14 +- src/xagent/web/services/task_orchestrator.py | 10 +- .../web/api/test_terminal_task_error_event.py | 27 +++ .../services/test_client_error_messages.py | 163 ++++++++++++++---- 5 files changed, 206 insertions(+), 41 deletions(-) diff --git a/frontend/src/contexts/app-context-chat.test.tsx b/frontend/src/contexts/app-context-chat.test.tsx index beb4cafd1a..65592722f4 100644 --- a/frontend/src/contexts/app-context-chat.test.tsx +++ b/frontend/src/contexts/app-context-chat.test.tsx @@ -6362,6 +6362,39 @@ describe("terminal error frames", () => { ) }) }) + + // getTaskErrorProjection reads `data?.code` before `root.code` -- this + // pins the nested half of that fallback, which no existing fixture + // exercises (they all carry code/details on the root). + it("reads code and details nested under data", async () => { + render( + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + act(() => { + onMessage?.({ + type: "task_error", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + data: { + code: "missing_runtime_context", + details: {}, + }, + } as TestWebSocketMessage) + }) + + await waitFor(() => { + expect(screen.getByTestId("messages").textContent).toContain( + "clientErrors.missingRuntimeContext" + ) + }) + }) }) describe("error frame display projection", () => { diff --git a/src/xagent/web/api/websocket.py b/src/xagent/web/api/websocket.py index 72dcc1c8ad..4254ea8123 100644 --- a/src/xagent/web/api/websocket.py +++ b/src/xagent/web/api/websocket.py @@ -13,7 +13,6 @@ from copy import deepcopy from dataclasses import dataclass from datetime import datetime, timedelta, timezone -from functools import lru_cache from pathlib import Path from typing import ( TYPE_CHECKING, @@ -322,13 +321,13 @@ def _task_error_payload( return payload -@lru_cache(maxsize=1) def _client_visible_error_codes() -> frozenset[str]: """The closed set of client-visible error codes, reused not recopied. Imported inside the function on purpose: the ``v1`` package's ``__init__`` pulls in routers that import this module, so a module-level import would - close a cycle. The set is built once and cached. + close a cycle. Rebuilt per call: a ~30-member frozenset is cheaper than a + cache to reason about. """ from .v1.errors import V1ErrorCode @@ -383,8 +382,13 @@ def create_terminal_task_error_event( # ConnectorRuntimeError types its code as a bare str and stores it # unvalidated, so "only the ten module constants reach here" is a fact - # about today's raise sites, not a property the code holds. - if code is not None and code not in _client_visible_error_codes(): + # about today's raise sites, not a property the code holds. The type + # check comes first for the same reason `details` has one: annotations + # are not enforced, and an unhashable value would raise inside the + # membership test on a path whose whole point is that it never raises. + if code is not None and ( + not isinstance(code, str) or code not in _client_visible_error_codes() + ): logger.error( "task_id=%s component=terminal-error-frame dropped=code " "value=%r; the frame is still sent without it", diff --git a/src/xagent/web/services/task_orchestrator.py b/src/xagent/web/services/task_orchestrator.py index 3b6b66002b..e1bafc4bff 100644 --- a/src/xagent/web/services/task_orchestrator.py +++ b/src/xagent/web/services/task_orchestrator.py @@ -1991,13 +1991,19 @@ async def execute_owned_run() -> None: # the connector identity is useful here and does not # leave the server, while the broadcast frame carries # neither it nor any reason that was filtered out. + # The projector two modules over checks the same + # attribute for a different reason; checking here too + # costs one line and removes the question. + raw_details = setup_or_run_err.details + if not isinstance(raw_details, dict): + raw_details = {} logger.error( "task_id=%s component=connector-runtime code=%s " "reason=%s connector=%s", task_id, setup_or_run_err.code, - setup_or_run_err.details.get("reason"), - setup_or_run_err.details.get("connector_ref"), + raw_details.get("reason"), + raw_details.get("connector_ref"), ) else: settlement_error = ( diff --git a/tests/web/api/test_terminal_task_error_event.py b/tests/web/api/test_terminal_task_error_event.py index 47caea7fa6..dd9e17efb4 100644 --- a/tests/web/api/test_terminal_task_error_event.py +++ b/tests/web/api/test_terminal_task_error_event.py @@ -157,6 +157,33 @@ def test_unknown_code_is_dropped_and_logged( assert "not_a_listed_code" in dropped[0] +@pytest.mark.parametrize("code", [["not", "hashable"], 7, object()]) +def test_a_non_string_code_is_dropped_without_raising( + code: Any, caplog: pytest.LogCaptureFixture +) -> None: + """The type gate runs before the membership test. + + ``ConnectorRuntimeError`` types its code as a bare ``str`` and stores it + unvalidated, and this builder runs inside an ``except`` that only logs a + failed broadcast -- so a non-string value must cost the argument, not the + frame. Only the unhashable case actually needs the gate: without it a list + raises inside the frozenset membership test, while a hashable non-string + (an int, a bare object) is simply not a member and already takes the drop + path. All three are pinned so the outcome is the same shape either way. + """ + with caplog.at_level(logging.ERROR): + event = create_terminal_task_error_event( + 1, "x", code=code, details=PublicErrorDetails(reason="not_provided") + ) + assert set(event.keys()) == BASE_FIELDS + dropped = [ + record.getMessage() + for record in caplog.records + if record.levelno == logging.ERROR and "dropped=code" in record.getMessage() + ] + assert len(dropped) == 1 + + @pytest.mark.parametrize( "code", [ diff --git a/tests/web/services/test_client_error_messages.py b/tests/web/services/test_client_error_messages.py index feda6fef93..df9ba194c0 100644 --- a/tests/web/services/test_client_error_messages.py +++ b/tests/web/services/test_client_error_messages.py @@ -15,7 +15,12 @@ import pytest from xagent.core.tools.adapters.vibe.config import RequiredMCPUnavailableError -from xagent.core.tools.adapters.vibe.connector_runtime import ConnectorRuntimeError +from xagent.core.tools.adapters.vibe.connector_runtime import ( + RUNTIME_INPUT_AUTH_SELECTOR, + RUNTIME_INPUT_CONTEXT, + RUNTIME_INPUT_SECRETS, + ConnectorRuntimeError, +) from xagent.web.services import client_error_messages from xagent.web.services.client_error_messages import ( CLIENT_SAFE_TASK_FAILURE, @@ -249,43 +254,10 @@ def test_public_error_drops_every_field_but_reason() -> None: assert set(projected[1].to_wire()) == {"reason"} -# -------------------------------------------------------------------------- -# I-34: ownership of the type -# -------------------------------------------------------------------------- - - def _python_sources() -> list[Path]: return sorted(SRC_ROOT.rglob("*.py")) -def test_public_error_details_is_constructed_in_one_module_only() -> None: - construction_sites: set[str] = set() - subclass_sites: set[str] = set() - - for path in _python_sources(): - tree = ast.parse(path.read_text(encoding="utf-8")) - relative = path.relative_to(SRC_ROOT).as_posix() - for node in ast.walk(tree): - if ( - isinstance(node, ast.Call) - and isinstance(node.func, ast.Name) - and node.func.id == "PublicErrorDetails" - ): - construction_sites.add(relative) - if isinstance(node, ast.ClassDef) and any( - isinstance(base, ast.Name) and base.id == "PublicErrorDetails" - for base in node.bases - ): - subclass_sites.add(relative) - - # Not a security boundary -- __post_init__ and the frame builder's - # type(...) is check are. This is an ownership boundary: the semantics of - # this type belong to the projector, so a second construction site or any - # subclass is a design drift that should be seen in review. - assert construction_sites == {"web/services/client_error_messages.py"} - assert subclass_sites == set() - - # -------------------------------------------------------------------------- # I-31: the whitelist and the real raise sites stay in step # -------------------------------------------------------------------------- @@ -512,3 +484,126 @@ def test_no_reason_assembled_by_interpolation_is_admitted_by_its_shape() -> None assert PublicErrorDetails(reason=probe).to_wire() == {}, ( f"a reason of shape {pattern} is admitted by its shape: {probe}" ) + + +def test_the_interpolated_reasons_are_grounded_in_their_section_names() -> None: + """The three undeclared_* members pass only via the f-string pattern. + + A pattern is an over-approximation: ``^undeclared_.+_key$`` would also + admit a listed reason nothing raises. Pin both halves -- the pattern set + the derivation actually produces, and the exact members it is allowed to + ground -- against the section names the raise site loops over. + + The derivation finds a second pattern, ``^missing_context\\..+$`` + (connector_runtime.py:857's ``f"missing_context.{key}"``), and it is + deliberately ungrounded: that reason is built from a key name the + connector's owner declared, and ``_is_public_reason``'s own docstring + names this exact shape as the one an owner-controlled interpolation must + not admit. ``test_no_reason_assembled_by_interpolation_is_admitted_by_its_shape`` + above already asserts every pattern this derivation finds is rejected by + shape; this test only pins which patterns exist and grounds the one that + is supposed to resolve to real whitelist members. + """ + + _, patterns = _derive_reasons() + assert patterns == {"^undeclared_.+_key$", "^missing_context\\..+$"} + expected = { + f"undeclared_{section}_key" + for section in ( + RUNTIME_INPUT_CONTEXT, + RUNTIME_INPUT_SECRETS, + RUNTIME_INPUT_AUTH_SELECTOR, + ) + } + assert { + reason + for reason in CONNECTOR_RUNTIME_PUBLIC_REASONS + if reason.startswith("undeclared_") + } == expected + + +def test_no_construction_site_hides_its_reason_from_the_scanner() -> None: + """The scanner has two blind spots; neither is occupied today. + + It reads a ``details=`` argument only when it is a literal dict, and it + matches a construction only when the callee is a bare name. Both are safe + only while nothing sits in them, so pin both: every non-literal + ``details=`` belongs to the one indirection the scanner handles on its own + (``_raise_runtime_error``, whose ``reason`` keyword it reads at the outer + call sites instead), and no construction reaches the class through an + attribute (``module.ConnectorRuntimeError(...)``). + + Sites are keyed by (file, line), not by file alone: a second non-literal + ``details=`` call added to the same file the one known site already lives + in must still change this set, or the assertion below would not notice a + new blind-spot occupant landing next to the one it already admits. + """ + + non_literal_details_sites: set[tuple[str, int]] = set() + attribute_construction_sites: set[str] = set() + + for path in _python_sources(): + tree = ast.parse(path.read_text(encoding="utf-8")) + relative = path.relative_to(SRC_ROOT).as_posix() + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + if ( + isinstance(node.func, ast.Name) + and node.func.id == "ConnectorRuntimeError" + ): + for keyword in node.keywords: + if keyword.arg == "details" and not isinstance( + keyword.value, ast.Dict + ): + non_literal_details_sites.add((relative, node.lineno)) + if ( + isinstance(node.func, ast.Attribute) + and node.func.attr == "ConnectorRuntimeError" + ): + attribute_construction_sites.add(relative) + + assert non_literal_details_sites == {("web/services/connector_runtime.py", 1007)} + assert attribute_construction_sites == set() + + +def test_every_opaque_reason_expression_is_a_str_call() -> None: + """Five reason expressions in this repository are opaque calls. + + Three pass ``reason=str(exc)`` into ``_raise_runtime_error``; two spell + ``details={"reason": str(exc)}`` on a direct construction. Both shapes are + collected by ``_reason_expressions``, and neither resolves to a literal -- + the runtime whitelist is what keeps them off the wire. Pin the shape so a + new opaque reason expression (a %-format, a .format(), a join) shows up as + a failure here instead of silently leaving the derivation. + """ + + trees = { + path: ast.parse(path.read_text(encoding="utf-8")) for path in _python_sources() + } + module_constants: dict[str, str] = {} + for tree in trees.values(): + module_constants.update(_module_string_constants(tree)) + + opaque: list[ast.expr] = [] + for tree in trees.values(): + bindings = _string_bindings(tree, module_constants) + for expression in _reason_expressions(tree): + if isinstance(expression, ast.Constant) and isinstance( + expression.value, str + ): + continue + if isinstance(expression, ast.Name) and bindings.get(expression.id): + continue + if isinstance(expression, ast.JoinedStr): + continue + opaque.append(expression) + + assert len(opaque) == 5, ( + "expected 5 opaque reason expressions (reason=str(exc) at " + "connector_runtime.py:824/854/880, details={'reason': str(exc)} at " + f":540/773), found {len(opaque)}" + ) + for expression in opaque: + assert isinstance(expression, ast.Call) + assert isinstance(expression.func, ast.Name) and expression.func.id == "str" From 2ef06c7d50185f63d86fdc3310e324a7f32b0ed2 Mon Sep 17 00:00:00 2001 From: Alexliu Date: Thu, 3 Sep 2026 04:03:26 +0800 Subject: [PATCH 19/28] docs(frontend): fix three positional comment references broken by the 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. --- frontend/src/contexts/app-context-chat.test.tsx | 2 +- frontend/src/contexts/app-context-chat.tsx | 17 +++++++++-------- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/frontend/src/contexts/app-context-chat.test.tsx b/frontend/src/contexts/app-context-chat.test.tsx index 65592722f4..9953681e2d 100644 --- a/frontend/src/contexts/app-context-chat.test.tsx +++ b/frontend/src/contexts/app-context-chat.test.tsx @@ -6550,7 +6550,7 @@ describe("error frame display projection", () => { // The envelope is parsed here rather than hand-built, matching the one // call site in production (app-context-chat.tsx, before the switch): a // hand-built envelope would be non-production-shaped input, and this is - // also what makes the no-version cell below (see its comment) actually + // also what makes the no-version cell above (see its comment) actually // exercise stateVersion being undefined rather than a value we chose. const controlEnvelope = extractTaskControlEnvelope(frame) expect( diff --git a/frontend/src/contexts/app-context-chat.tsx b/frontend/src/contexts/app-context-chat.tsx index 0b7448662c..065a9f8e75 100644 --- a/frontend/src/contexts/app-context-chat.tsx +++ b/frontend/src/contexts/app-context-chat.tsx @@ -1041,12 +1041,12 @@ export const projectErrorFrameForDisplay = ( // those two apart, and on this handler the collapsed frame is the turn's // result. The identity is withheld when the frame carries no version (the // row was already gone when it was broadcast, so no state tuple was - // attached), which falls back to keying on the text alone: the version gate - // above drops such a frame once any versioned event has been seen for the - // task, and when none has, two of them key on the same text and the second - // still collapses -- the behaviour that predates this change. Withholding - // the identity is the honest answer there; attaching a state tuple needs - // the row, and a settled FAILED task has one. + // attached), which falls back to keying on the text alone: + // canAcceptTaskControlVersion drops such a frame once any versioned event + // has been seen for the task, and when none has, two of them key on the + // same text and the second still collapses -- the behaviour that predates + // this change. Withholding the identity is the honest answer there; + // attaching a state tuple needs the row, and a settled FAILED task has one. const occurrenceIdentity = isTerminal && controlEnvelope.stateVersion !== undefined ? `${controlEnvelope.runId ?? ""}:${controlEnvelope.stateVersion}` @@ -1066,8 +1066,9 @@ export const projectErrorFrameForDisplay = ( // error" placeholder until reload. A non-terminal rejection is not, and // flagging it would close the live progress indicator and the // waiting-answer form of a turn that is still running, and drain this - // turn's accumulated trace events into the rejection bubble (see - // ADD_MESSAGE above). + // turn's accumulated trace events into the rejection bubble (see the + // ADD_MESSAGE reducer case's isResult branch, which merges + // state.traceEvents into the message and clears it). isResult: isTerminal, } } From bb3940a5299fbba9640bf77bcdfcf5d8156dcc67 Mon Sep 17 00:00:00 2001 From: Alexliu Date: Thu, 3 Sep 2026 19:06:41 +0800 Subject: [PATCH 20/28] fix(web): whitelist the access-resolution reason main now raises main's #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. --- src/xagent/web/services/client_error_messages.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/xagent/web/services/client_error_messages.py b/src/xagent/web/services/client_error_messages.py index 21136e0087..8f3599237a 100644 --- a/src/xagent/web/services/client_error_messages.py +++ b/src/xagent/web/services/client_error_messages.py @@ -158,6 +158,11 @@ def connector_runtime_client_message( # shape (runtime_task_identity_mismatch, runtime_owner_mismatch) are # deliberately absent for that reason -- see the class docstring below. "team_scope_resolution_failed", + # Same module, same shape: the generic-exception fallback arm of + # resolve_connector_access_or_raise() in connector_team_scope.py. + # It states that resolving connector access failed, not who owns the + # task or what the access check concluded. + "connector_access_resolution_failed", "team_env_resolution_failed", "runtime_view_resolution_failed", "custom_api_config_load_failed", From 0a3c3f947bf5e302a407aaa294b3c8e4f020cfca Mon Sep 17 00:00:00 2001 From: Alexliu Date: Thu, 3 Sep 2026 19:28:03 +0800 Subject: [PATCH 21/28] docs(frontend): anchor two websocket producer references by symbol, not line Merging main shifted websocket.py's line numbers; replaced the two stale websocket.py: 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. --- .../src/contexts/app-context-chat.test.tsx | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/frontend/src/contexts/app-context-chat.test.tsx b/frontend/src/contexts/app-context-chat.test.tsx index 9953681e2d..1b703f0443 100644 --- a/frontend/src/contexts/app-context-chat.test.tsx +++ b/frontend/src/contexts/app-context-chat.test.tsx @@ -5744,13 +5744,19 @@ describe("terminal error frames", () => { }) // The waiting half. A refused resume arrives on the root "error" type - // carrying the task's real current status (websocket.py:8935 builds it from - // TaskControlSnapshot). The question the user still has to answer lives on - // the panel's virtual bubble, which a flagged rejection would remove. + // carrying the task's real current status (built from TaskControlSnapshot + // in websocket.py's _handle_resume_task_unserialized, near where + // resume_control_state is assembled -- that function is long, so look for + // the assignment rather than a fixed offset). The question the user still + // has to answer lives on the panel's virtual bubble, which a flagged + // rejection would remove. // The fixture below combines fields from two producers: `task` comes from - // the resume-refusal path (websocket.py:8935), `error_code` comes from the - // pause-refusal path (websocket.py:8491). Neither path emits both fields - // together today; each field is genuinely emitted by its own path. + // the resume-refusal branch, websocket.py's + // _handle_resume_task_unserialized; `error_code` comes from the + // pause-refusal branch, websocket.py's handle_pause_task, near its + // ClientErrorCode.MESSAGE_PROCESSING_FAILED fallback. Neither path emits + // both fields together today; each field is genuinely emitted by its own + // path. // Carrying `task` drives the reducer's preservation branch (it is what // makes `UPDATE_TASK_STATUS` dispatch at all) -- without it the assertions // below would pass vacuously instead of exercising that branch. From 62bcf1380cead722e5959722100fa9e204b10fe5 Mon Sep 17 00:00:00 2001 From: Alexliu Date: Fri, 4 Sep 2026 00:20:00 +0800 Subject: [PATCH 22/28] refactor(web): send only the error code on terminal task_error frames 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. --- .../src/contexts/app-context-chat.test.tsx | 104 ++-- frontend/src/contexts/app-context-chat.tsx | 29 +- frontend/src/lib/client-errors.ts | 5 +- src/xagent/web/api/websocket.py | 51 +- .../web/services/client_error_messages.py | 134 +---- src/xagent/web/services/task_orchestrator.py | 34 +- .../web/api/test_terminal_task_error_event.py | 110 +--- .../services/test_client_error_messages.py | 546 +----------------- tests/web/services/test_task_orchestrator.py | 34 +- 9 files changed, 143 insertions(+), 904 deletions(-) diff --git a/frontend/src/contexts/app-context-chat.test.tsx b/frontend/src/contexts/app-context-chat.test.tsx index 1b703f0443..ab6d9d8016 100644 --- a/frontend/src/contexts/app-context-chat.test.tsx +++ b/frontend/src/contexts/app-context-chat.test.tsx @@ -5819,9 +5819,10 @@ describe("terminal error frames", () => { expect(screen.getByTestId("waiting-interactions").textContent).not.toBe("[]") }) - // A listed reason that is a bare enum value names no key, so the keyless - // wording is chosen even though the code is a missing-value one. - it("uses the same wording for a listed bare reason", async () => { + // The frame's code alone decides the wording -- nothing the raise site + // attached beyond the code reaches the client, so a missing-value code + // gets its own table entry regardless of what it would have named. + it("resolves wording from the code alone", async () => { render( @@ -5841,7 +5842,6 @@ describe("terminal error frames", () => { message: "Connector secrets are unavailable.", error: "Connector secrets are unavailable.", code: "runtime_secret_unavailable", - details: { reason: "not_provided" }, } as TestWebSocketMessage) }) @@ -5855,19 +5855,19 @@ describe("terminal error frames", () => { const bubble = messages.find((m: { content: string }) => m.content.includes("clientErrors.runtimeSecretUnavailable") ) - // Same toBe as the test below, on the other input: this branch is reached - // with a listed bare reason rather than an empty details, and an - // interpolation regression has to be caught on both. + // Same toBe as the test below, on a different code: an interpolation + // regression has to be caught on both. expect(bubble?.content).toBe("clientErrors.runtimeSecretUnavailable") }) // What the server now sends for a missing declared context key: the code - // survives, the reason naming the key does not. The bubble therefore says a - // value is missing without saying which -- the key name is owner - // configuration and this frame reaches anonymous widget and share-link - // visitors. Asserted with toBe, under a variable-aware i18n mock: had the - // wording interpolated anything, the content would read - // ":{...}" and this assertion would fail. + // survives, and nothing else about the failure -- the key name included -- + // ever reaches this frame at all. The bubble therefore says a value is + // missing without saying which -- the key name is owner configuration and + // this frame reaches anonymous widget and share-link visitors. Asserted + // with toBe, under a variable-aware i18n mock: had the wording + // interpolated anything, the content would read ":{...}" and this + // assertion would fail. it("names no declared key when a runtime value is missing", async () => { render( @@ -5888,7 +5888,6 @@ describe("terminal error frames", () => { message: "Required connector runtime context is missing.", error: "Required connector runtime context is missing.", code: "missing_runtime_context", - details: {}, } as TestWebSocketMessage) }) @@ -5932,7 +5931,6 @@ describe("terminal error frames", () => { message: "Connector runtime is unavailable.", error: "Connector runtime is unavailable.", code: "connector_runtime_unavailable", - details: { reason: "team_env_resolution_failed" }, } as TestWebSocketMessage) }) @@ -5951,13 +5949,12 @@ describe("terminal error frames", () => { }) // The dedup identity is the frame's own (run_id, state_version), not its - // code or reason. These two turns fail under one code for two different - // admitted reasons -- the same runtime secret, first never provided, then - // lost from its store -- and each settlement bumps state_version at least - // once (the retry takes the lease FAILED -> RUNNING, then settles RUNNING - // -> FAILED), so the second turn's version is strictly greater. Two - // distinct versions mean two distinct identities, and the bubble is the - // turn's result. + // code. These two turns fail under the same code -- the same runtime + // secret, both times unavailable -- and each settlement bumps + // state_version at least once (the retry takes the lease FAILED -> + // RUNNING, then settles RUNNING -> FAILED), so the second turn's version + // is strictly greater. Two distinct versions mean two distinct + // identities, and the bubble is the turn's result. it("keeps both bubbles when one code fails twice at different state versions", async () => { render( @@ -5969,24 +5966,22 @@ describe("terminal error frames", () => { const onMessage = webSocketOptions.current?.onMessage expect(onMessage).toBeDefined() - const frameForReason = (reason: string, timestamp: string, stateVersion: number) => ({ + const frameAtVersion = (timestamp: string, stateVersion: number) => ({ type: "task_error", timestamp, task_id: 1, task: { id: 1, status: "failed" }, // Identical on both frames, and that is the production shape: - // _message_for_code returns one string per code and does not vary with - // the reason. + // _message_for_code returns one string per code. message: "Required runtime secret is unavailable.", error: "Required runtime secret is unavailable.", code: "runtime_secret_unavailable", - details: { reason }, run_id: "run-1", state_version: stateVersion, }) as TestWebSocketMessage act(() => { - onMessage?.(frameForReason("not_provided", "2026-05-27T05:00:02Z", 12)) + onMessage?.(frameAtVersion("2026-05-27T05:00:02Z", 12)) }) await waitFor(() => { expect(screen.getByTestId("messages").textContent).toContain( @@ -5995,7 +5990,7 @@ describe("terminal error frames", () => { }) act(() => { - onMessage?.(frameForReason("store_lost", "2026-05-27T05:00:03Z", 14)) + onMessage?.(frameAtVersion("2026-05-27T05:00:03Z", 14)) }) await waitFor(() => { @@ -6032,7 +6027,6 @@ describe("terminal error frames", () => { message: "Required runtime secret is unavailable.", error: "Required runtime secret is unavailable.", code: "runtime_secret_unavailable", - details: { reason: "not_provided" }, run_id: "run-1", state_version: 12, } as TestWebSocketMessage @@ -6062,11 +6056,10 @@ describe("terminal error frames", () => { }) }) - // Blocking issue 1's direct anchor: two failed turns under the same code - // and the same reason, distinguished only by their state_version. Keying - // on the failure's class -- the code, the reason, or the rendered - // sentence -- cannot tell these apart; keying on the frame's own state - // tuple can. + // Two failed turns under the same code, distinguished only by their + // state_version. Keying on the failure's class -- the code or the + // rendered sentence -- cannot tell these apart; keying on the frame's own + // state tuple can. it("keeps both bubbles when one failure repeats on the next turn", async () => { render( @@ -6086,7 +6079,6 @@ describe("terminal error frames", () => { message: "Required runtime secret is unavailable.", error: "Required runtime secret is unavailable.", code: "runtime_secret_unavailable", - details: { reason: "not_provided" }, run_id: "run-1", state_version: stateVersion, }) as TestWebSocketMessage @@ -6116,10 +6108,10 @@ describe("terminal error frames", () => { }) }) - // Blocking issue 2's direct anchor: two failed turns that carry no code at - // all -- the rendered sentence is identical on both -- distinguished only - // by their state_version. Before this change the dedup key was the - // rendered sentence alone, so the second of these vanished and that bubble + // Two failed turns that carry no code at all -- the rendered sentence is + // identical on both -- distinguished only by their state_version. Before + // this change the dedup key was the rendered sentence alone, so the + // second of these vanished and that bubble // is the turn's result. it("keeps both bubbles for two generic failures on consecutive turns", async () => { render( @@ -6168,7 +6160,7 @@ describe("terminal error frames", () => { }) }) - // The widest form of blocking issue 2: a generic failure on an untrusted + // The widest form of the same case: a generic failure on an untrusted // transport reads the same fixed "Unknown error" constant regardless of // what precedes it, so a version-blind identity would collapse it into // whatever coded failure happened to precede it within the window. The @@ -6195,7 +6187,6 @@ describe("terminal error frames", () => { message: "Required connector runtime context is missing.", error: "Required connector runtime context is missing.", code: "missing_runtime_context", - details: {}, run_id: "run-1", state_version: 12, } as TestWebSocketMessage) @@ -6237,7 +6228,7 @@ describe("terminal error frames", () => { }) }) - // I-B's witness at the integration level: two non-terminal rejections each + // The witness at the integration level: two non-terminal rejections each // carry a version ("error" is in VERSIONED_TASK_EVENT_TYPES too), but the // terminal-only identity must not leak into this channel -- if it did, two // different versions would make these look like two distinct rejections @@ -6289,12 +6280,11 @@ describe("terminal error frames", () => { }) }) - // Blocking issue 3's direct anchor: connector_runtime_unavailable now has - // its own table entry, so it survives a transport that marks legacy prose - // untrusted the same way the three previously-curated codes always did. - // Before this, only those three had client-side wording; every other code, - // this one included, fell through to "Unknown error" for an anonymous - // widget or share-link visitor. + // connector_runtime_unavailable has its own table entry, so it survives a + // transport that marks legacy prose untrusted the same way the three + // previously-curated codes always did. Before this, only those three had + // client-side wording; every other code, this one included, fell through + // to "Unknown error" for an anonymous widget or share-link visitor. it("localizes a connector-runtime code for an untrusted transport", async () => { render( @@ -6315,7 +6305,6 @@ describe("terminal error frames", () => { message: "Connector runtime is unavailable.", error: "Connector runtime is unavailable.", code: "connector_runtime_unavailable", - details: { reason: "team_env_resolution_failed" }, } as TestWebSocketMessage) }) @@ -6358,7 +6347,6 @@ describe("terminal error frames", () => { message: "Connector could not be found.", error: "Connector could not be found.", code: "connector_not_found", - details: {}, } as TestWebSocketMessage) }) @@ -6371,8 +6359,8 @@ describe("terminal error frames", () => { // getTaskErrorProjection reads `data?.code` before `root.code` -- this // pins the nested half of that fallback, which no existing fixture - // exercises (they all carry code/details on the root). - it("reads code and details nested under data", async () => { + // exercises (they all carry code on the root). + it("reads code nested under data", async () => { render( @@ -6390,7 +6378,6 @@ describe("terminal error frames", () => { task_id: 1, data: { code: "missing_runtime_context", - details: {}, }, } as TestWebSocketMessage) }) @@ -6417,7 +6404,6 @@ describe("error frame display projection", () => { message: "Required connector runtime context is missing.", error: "Required connector runtime context is missing.", code: "missing_runtime_context", - details: {}, } as unknown as TaskControlMessage, trustLegacyErrorProse: true, expected: { @@ -6427,7 +6413,7 @@ describe("error frame display projection", () => { dedupText: "Required connector runtime context is missing.", // No run_id/state_version on this fixture, same as production for a // frame the version gate would drop once any versioned event has - // been seen -- see I-A and cell 2 below. + // been seen; the second case below is the witness for that fallback. occurrenceIdentity: undefined, bubbleContent: "clientErrors.missingRuntimeContext", isResult: true, @@ -6450,7 +6436,7 @@ describe("error frame display projection", () => { stopsProcessing: true, dedupText: "Task execution failed.", // This is the witness for withholding the identity when the frame - // has no state version -- see I-A. + // has no state version. occurrenceIdentity: undefined, bubbleContent: "agent.logs.event.messages.errorPrefix Task execution failed.", isResult: true, @@ -6466,7 +6452,6 @@ describe("error frame display projection", () => { message: "Required connector runtime context is missing.", error: "Required connector runtime context is missing.", code: "missing_runtime_context", - details: {}, } as unknown as TaskControlMessage, trustLegacyErrorProse: false, expected: { @@ -6480,7 +6465,7 @@ describe("error frame display projection", () => { }, }, { - name: "a terminal frame with a state version and a listed reason on a trusted transport", + name: "a terminal frame with a state version on a trusted transport", frame: { type: "task_error", timestamp: "2026-05-27T05:00:02Z", @@ -6489,7 +6474,6 @@ describe("error frame display projection", () => { message: "Required runtime secret is unavailable.", error: "Required runtime secret is unavailable.", code: "runtime_secret_unavailable", - details: { reason: "not_provided" }, run_id: "run-1", state_version: 12, } as unknown as TaskControlMessage, @@ -6546,7 +6530,7 @@ describe("error frame display projection", () => { stopsProcessing: false, dedupText: "Task is currently busy; please wait for the previous turn to finish.", // This is the witness for keeping the terminal-only identity out of - // the rejection channel -- see I-B. + // the rejection channel. occurrenceIdentity: undefined, bubbleContent: "agent.logs.event.messages.errorPrefix Task is currently busy; please wait for the previous turn to finish.", isResult: false, diff --git a/frontend/src/contexts/app-context-chat.tsx b/frontend/src/contexts/app-context-chat.tsx index 065a9f8e75..77ba9e7637 100644 --- a/frontend/src/contexts/app-context-chat.tsx +++ b/frontend/src/contexts/app-context-chat.tsx @@ -44,14 +44,12 @@ type TaskControlState = | "completed" | "failed" -// The structured half of a terminal task_error frame. ``details`` holds at -// most ``reason``: the server projects the exception through a whitelist -// before broadcasting, because this frame reaches every connection on the -// task, anonymous widget and share-link visitors included. Nothing here is -// connector-specific: the code is what decides whether a given frame is. +// The structured half of a terminal task_error frame: the stable error code +// the client renders and localizes. The frame carries nothing else +// connector-specific -- its audience includes anonymous widget and +// share-link visitors. type TaskErrorProjection = { code: string - details: { reason?: string } } type TaskControlEnvelope = { @@ -929,9 +927,7 @@ const getWebSocketErrorCode = (message: WebSocketMessage) => { } // The frame deliberately carries nothing connector-specific beyond the code: -// its audience includes anonymous widget and share-link visitors, and the -// server's reason whitelist admits only fixed strings it controls -- never a -// reason assembled from a key name the connector's owner declared. Which +// its audience includes anonymous widget and share-link visitors. Which // connector, which key, and each key's declared type all come from the // per-task requirements endpoint, which selects on // `Task.id == task_id AND Task.user_id == current_user.id`. @@ -941,14 +937,7 @@ const getTaskErrorProjection = ( const root = message as unknown as Record const data = isJsonRecord(message.data) ? message.data : null const code = getString(data?.code) || getString(root.code) - if (!code) return null - const details = isJsonRecord(data?.details) - ? data.details - : isJsonRecord(root.details) - ? root.details - : null - const reason = getString(details?.reason) - return { code, details: reason ? { reason } : {} } + return code ? { code } : null } const getWebSocketTaskStatus = (message: WebSocketMessage): Task["status"] | null => { @@ -983,8 +972,8 @@ export type ErrorFrameDisplay = { // identity, the result flag, and the task status to dispatch. Each of those // needs a different subset of "is this terminal / is legacy prose trusted / // did a code survive / is there a state version", and deriving each subset at -// its own use site is what let five separate defects land in this handler -// across two review rounds. Pure on purpose: no dispatch, no refs, nothing +// its own use site is what let five separate defects land in this handler. +// Pure on purpose: no dispatch, no refs, nothing // outside its arguments, so every cell of that matrix is unit-testable // without rendering the provider -- the same shape extractTaskControlEnvelope // above already uses. @@ -1007,7 +996,7 @@ export const projectErrorFrameForDisplay = ( // branch, and websocket.py's legacy helper, which settles under // only_if_running=True and does not broadcast when that update matches // no row -- and task_error is also the only frame that carries the - // structured code/details pair. The root "error" type is a mixed + // structured code. The root "error" type is a mixed // channel: rejected chat messages, rejected pause and rejected resume // all arrive on it while the viewed task is still RUNNING or // WAITING_FOR_USER, and a rejection is not this turn's answer. diff --git a/frontend/src/lib/client-errors.ts b/frontend/src/lib/client-errors.ts index 75285cfce9..98f9fa9426 100644 --- a/frontend/src/lib/client-errors.ts +++ b/frontend/src/lib/client-errors.ts @@ -28,9 +28,8 @@ const CLIENT_ERROR_CODES = [ // ClientErrorCode enum -- this list is already a superset of that enum (the // three upload_* codes are client-side only) and stays one table so a code // has one wording for every audience. Only codes with a producer that can - // reach that frame today are listed, the same rule the server's reason - // whitelist states about itself: a listed code nothing produces is an entry - // with no expiry date. Which codes those are is a fact about this + // reach that frame today are listed: a listed code nothing produces is an + // entry with no expiry date. Which codes those are is a fact about this // repository's raise sites, not a property the wire holds -- the field is // typed as a bare string and validated only against the full V1ErrorCode // set (see websocket.py's own note above that check), and a resolver diff --git a/src/xagent/web/api/websocket.py b/src/xagent/web/api/websocket.py index be544f21d1..e57b5672eb 100644 --- a/src/xagent/web/api/websocket.py +++ b/src/xagent/web/api/websocket.py @@ -105,7 +105,6 @@ CLIENT_SAFE_TASK_FAILURE, CLIENT_SAFE_VALIDATION_ERROR, ClientErrorCode, - PublicErrorDetails, client_error_message, ) from ..services.db_runtime import ( @@ -352,52 +351,35 @@ def create_terminal_task_error_event( message: str, *, code: str | None = None, - details: PublicErrorDetails | None = None, ) -> dict[str, Any]: """Shape an error event after the exact lease owner commits FAILED. - ``code`` and ``details`` are written only when both survive validation, so - a caller that passes neither still gets the same six-key frame, and a - caller that passes something unusable gets that same frame rather than an - exception. This runs on the reporting path of an already-failed task, and - the one call site that passes these arguments evaluates them inside the - ``except Exception`` that only logs a failed broadcast -- so raising here - would cost the terminal frame outright and leave the user on the silent + ``code`` is written only when it survives validation, so a caller that + passes none still gets the same six-key frame, and a caller that passes + something unusable gets that same frame rather than an exception. This + runs on the reporting path of an already-failed task, and the one call + site that passes this argument evaluates it inside the ``except + Exception`` that only logs a failed broadcast -- so raising here would + cost the terminal frame outright and leave the user on the silent failure this path exists to remove. A bad optional argument costs that - argument and nothing else. Both rejections are logged with their stack. + argument and nothing else. The rejection is logged with its stack. - ``details`` is accepted as ``PublicErrorDetails`` itself and nothing else - -- not a subclass -- because that class's ``__post_init__`` is where the - reason whitelist lives. ``code`` must be a member of ``V1ErrorCode``, the - repository's closed set of client-visible error codes. + ``code`` must be a member of the connector-runtime family the client + renders, the repository's closed set of client-visible error codes. """ # Python annotations are not enforced at run time, so the mypy gate on the # signature above is not the whole door: a caller that routes through Any # (a dict from JSON, a **kwargs splat) type-checks clean and would reach - # to_wire() as an AttributeError deep in this function. Name the contract - # here instead. + # this function with a value of the wrong shape. Name the contract here + # instead. # - # `type(...) is`, not isinstance: a frozen dataclass can be subclassed, and - # a subclass that overrides to_wire() without reading self.reason passes - # both isinstance and mypy while bypassing the whitelist in __post_init__. - # Only the class itself carries that guarantee. - if details is not None and type(details) is not PublicErrorDetails: - logger.error( - "task_id=%s component=terminal-error-frame dropped=details " - "type=%s; the frame is still sent without it", - task_id, - type(details).__name__, - stack_info=True, - ) - details = None - # ConnectorRuntimeError types its code as a bare str and stores it # unvalidated, so "only the ten module constants reach here" is a fact # about today's raise sites, not a property the code holds. The type - # check comes first for the same reason `details` has one: annotations - # are not enforced, and an unhashable value would raise inside the - # membership test on a path whose whole point is that it never raises. + # check comes first for the same reason: annotations are not enforced, + # and an unhashable value would raise inside the membership test on a + # path whose whole point is that it never raises. if code is not None and ( not isinstance(code, str) or code not in _client_visible_error_codes() ): @@ -421,9 +403,8 @@ def create_terminal_task_error_event( "error": message, "timestamp": datetime.now(timezone.utc).timestamp(), } - if code is not None and details is not None: + if code is not None: event["code"] = code - event["details"] = details.to_wire() return event diff --git a/src/xagent/web/services/client_error_messages.py b/src/xagent/web/services/client_error_messages.py index 8f3599237a..564ba16333 100644 --- a/src/xagent/web/services/client_error_messages.py +++ b/src/xagent/web/services/client_error_messages.py @@ -2,11 +2,10 @@ Holds the fixed fallback strings used when a failure has nothing safe to say, the per-exception adapters that pass a curated message through, and -``PublicErrorDetails`` -- the only structured payload allowed onto a -task_error frame, together with the reason allowlist that governs it. +the projector that lifts a connector-runtime failure's code onto a +task_error frame. """ -from dataclasses import dataclass from enum import StrEnum from ...core.tools.adapters.vibe.config import RequiredMCPUnavailableError @@ -140,125 +139,26 @@ def connector_runtime_client_message( return fallback -CONNECTOR_RUNTIME_PUBLIC_REASONS = frozenset( - { - # Missing values and binding. - "not_provided", - "store_lost", - "connector_not_selected", - "auth_selector_not_supported", - "duplicate_ref", - "undeclared_context_key", - "undeclared_secrets_key", - "undeclared_auth_selector_key", - # Fixed 503 strings built by direct ConnectorRuntimeError construction - # in three other modules. Each one states that a server-side component - # is unavailable; none of them states who owns the task, or how an - # authorization check resolved. Two further strings of exactly this - # shape (runtime_task_identity_mismatch, runtime_owner_mismatch) are - # deliberately absent for that reason -- see the class docstring below. - "team_scope_resolution_failed", - # Same module, same shape: the generic-exception fallback arm of - # resolve_connector_access_or_raise() in connector_team_scope.py. - # It states that resolving connector access failed, not who owns the - # task or what the access check concluded. - "connector_access_resolution_failed", - "team_env_resolution_failed", - "runtime_view_resolution_failed", - "custom_api_config_load_failed", - } -) -# Every member above is raised somewhere in this repository today, and a test -# asserts that in both directions. Add a reason here in the same change that -# adds the site raising it, never ahead of it: a listed reason nothing produces -# is an allowance with no expiry date, and by the time the raising code arrives -# nobody remembers which audience the reason was judged against. - - -def _is_public_reason(reason: object) -> bool: - """True when this reason may reach a client. Used by PublicErrorDetails. - - Membership is the whole rule: the listed values are fixed strings this - repository writes, so reading the list tells you exactly what can reach a - visitor. A reason assembled from something the connector's owner wrote -- - ``missing_context.`` is the one such reason raised here - -- is not admitted, however legal its shape, because this frame reaches - anonymous widget and share-link visitors and a key name is the owner's - configuration. Owners read key names from the per-task requirements - endpoint, which selects on ``Task.id == task_id AND - Task.user_id == current_user.id``. - """ - - return isinstance(reason, str) and reason in CONNECTOR_RUNTIME_PUBLIC_REASONS - - -@dataclass(frozen=True) -class PublicErrorDetails: - """The only shape allowed into a task_error frame's ``details``. - - ``reason`` is normalized on construction: anything that is not a listed - enum member becomes ``None``. Constructing this type and passing the - reason whitelist are therefore the same act -- there is no path that - produces an instance carrying free text, including a direct call from - another module. - - Nulling rather than raising is deliberate: every construction site is on - the reporting path of an already-failed task, and raising there would - turn a diagnosable failure into an undiagnosable crash. - - The sink is ``broadcast_to_task``, whose audience includes anonymous - widget and share-link visitors, so every listed reason and every new - field has to answer two questions, and a yes to either keeps it off this - frame. First: can a visitor who is not the task owner read the task's - ownership, or the outcome of an authorization check, out of it? Second: - does any part of it come from something the connector's owner wrote down - -- a key name, a label, a ref -- rather than from a fixed string this - repository controls? There is no ``connector_ref`` field and two runtime - reasons are omitted on the first question; the - ``.`` forms are omitted on the second. - """ - - reason: str | None - - def __post_init__(self) -> None: - if self.reason is not None and not _is_public_reason(self.reason): - object.__setattr__(self, "reason", None) - - def to_wire(self) -> dict[str, str]: - return {"reason": self.reason} if self.reason is not None else {} - - -def connector_runtime_public_error( - error: BaseException, -) -> tuple[str, PublicErrorDetails] | None: - """Project a connector-runtime failure onto the wire-safe (code, details). +def connector_runtime_client_code(error: BaseException) -> str | None: + """Project a connector-runtime failure onto its wire-safe error code. Returns ``None`` for anything else, so a caller cannot widen the surface - by passing an incidental exception. The reason filter itself lives in - ``PublicErrorDetails``; this function only decides whether the exception - is one we project at all. + by passing an incidental exception. Membership in the client-visible + closed set is checked by the frame builder, not here: this function + only decides whether the exception is one we project at all. This is not the only client-visible projection of this exception. - ``_raise_v1_connector_runtime_error`` (``web/api/v1/tasks.py``) projects it - for the SDK surface and ships ``to_public_error()["details"]`` whole, - ``connector_ref`` included. The two differ because their audiences do: that - one answers an API key held by a caller already authorized for the task, - while this one feeds ``broadcast_to_task``, which reaches every connection - under the task id including anonymous widget and share-link visitors. - Keep them as two projectors with one audience each; folding them into one - that takes the audience as an argument puts the width of the output behind - a caller-supplied flag, which fails open the first time it is passed wrong. + ``_raise_v1_connector_runtime_error`` (``web/api/v1/tasks.py``) projects + it for the SDK surface and ships ``to_public_error()["details"]`` + whole, ``connector_ref`` included. The two differ because their + audiences do: that one answers an API key held by a caller already + authorized for the task, while this one feeds ``broadcast_to_task``, + which reaches every connection under the task id including anonymous + widget and share-link visitors. Keep them as two projectors with one + audience each. """ if not isinstance(error, ConnectorRuntimeError): return None - details = error.details - if not isinstance(details, dict): - # ``__init__`` normalizes details to a dict, but it is a plain public - # attribute anything can reassign afterwards. This is the last step - # before the wire, so verify rather than assume: a payload of the wrong - # shape means the instance is not trustworthy, and the safe answer is - # to fall all the way back to the opaque failure rather than guess - # which half of it is still readable. - return None - return error.code, PublicErrorDetails(reason=details.get("reason")) + code = error.code + return code if isinstance(code, str) else None diff --git a/src/xagent/web/services/task_orchestrator.py b/src/xagent/web/services/task_orchestrator.py index 843214dbcd..e133ead477 100644 --- a/src/xagent/web/services/task_orchestrator.py +++ b/src/xagent/web/services/task_orchestrator.py @@ -74,9 +74,8 @@ ) from .client_error_messages import ( CLIENT_SAFE_TASK_FAILURE, - PublicErrorDetails, + connector_runtime_client_code, connector_runtime_client_message, - connector_runtime_public_error, required_mcp_unavailable_client_message, ) from .db_runtime import ( @@ -1798,7 +1797,6 @@ async def _runner() -> None: client_history_message_type = TASK_FAILURE_MESSAGE_TYPE broadcast_error_message: str | None = None broadcast_error_code: str | None = None - broadcast_error_details: PublicErrorDetails | None = None defer_settlement_to_ttl_recovery = False skip_delivery_reconciliation = False # Positive evidence for finalize's delivery target: once @@ -1971,29 +1969,24 @@ async def execute_owned_run() -> None: # This exception's message is a curated public-safe # sentence -- it says a runtime input is missing, not # which one -- so the client gets it instead of the - # opaque fallback. ``code`` rides along on the frame, - # and so does ``reason`` when the whitelist admits it; - # a reason assembled from a key name the connector's - # owner declared is dropped at the projector, so this - # branch's own reason reaches the frame for two of the - # three missing-value codes and not for - # missing_runtime_context. + # opaque fallback, and ``code`` rides along on the + # frame so the client can pick its own wording. + # Nothing else from the exception reaches the frame: + # the reason and the connector identity go to the + # operator log below. settlement_error = str(setup_or_run_err) client_history_message_type = CLIENT_SAFE_FAILURE_MESSAGE_TYPE broadcast_error_message = connector_runtime_client_message( setup_or_run_err ) - broadcast_error_code, broadcast_error_details = ( - connector_runtime_public_error(setup_or_run_err) - or (None, None) + broadcast_error_code = connector_runtime_client_code( + setup_or_run_err ) - # Operators read the raw details, not the projection: - # the connector identity is useful here and does not - # leave the server, while the broadcast frame carries - # neither it nor any reason that was filtered out. - # The projector two modules over checks the same - # attribute for a different reason; checking here too - # costs one line and removes the question. + # Operators read the raw details: the connector + # identity is useful here and does not leave the + # server. ``details`` is a plain public attribute + # anything can reassign, so verify the shape before + # reading it. raw_details = setup_or_run_err.details if not isinstance(raw_details, dict): raw_details = {} @@ -2078,7 +2071,6 @@ async def execute_owned_run() -> None: task_id, broadcast_error_message, code=broadcast_error_code, - details=broadcast_error_details, ), task_id, ) diff --git a/tests/web/api/test_terminal_task_error_event.py b/tests/web/api/test_terminal_task_error_event.py index dd9e17efb4..8ab54f8544 100644 --- a/tests/web/api/test_terminal_task_error_event.py +++ b/tests/web/api/test_terminal_task_error_event.py @@ -1,16 +1,14 @@ """Frame-shape contracts for ``create_terminal_task_error_event``. -Two things are pinned here: the four call sites that pass neither ``code`` nor -``details`` still get the same six-key frame, and ``details`` is accepted as -``PublicErrorDetails`` itself and nothing else -- not a dict, not a duck type, -not a subclass. +Pinned here: the four call sites that pass no ``code`` still get the same +six-key frame, and a ``code`` that survives validation is written onto the +frame under its own key with nothing else alongside it. """ from __future__ import annotations import json import logging -from dataclasses import dataclass from typing import Any import pytest @@ -19,7 +17,6 @@ _client_visible_error_codes, create_terminal_task_error_event, ) -from xagent.web.services.client_error_messages import PublicErrorDetails BASE_FIELDS = {"type", "message", "task_id", "task", "error", "timestamp"} @@ -28,103 +25,23 @@ "kwargs", [ {}, - {"code": "missing_runtime_context"}, - {"details": PublicErrorDetails(reason="not_provided")}, ], - ids=["neither", "code-only", "details-only"], + ids=["neither"], ) def test_terminal_error_event_shape_unchanged(kwargs: dict[str, Any]) -> None: - """Both new fields are written together or not at all.""" + """A caller that passes no code gets the same six-key frame.""" event = create_terminal_task_error_event(1, "x", **kwargs) assert set(event.keys()) == BASE_FIELDS -def test_terminal_error_event_carries_both_new_fields_together() -> None: - event = create_terminal_task_error_event( - 1, - "x", - code="missing_runtime_context", - details=PublicErrorDetails(reason="not_provided"), - ) +def test_terminal_error_event_carries_a_valid_code() -> None: + event = create_terminal_task_error_event(1, "x", code="missing_runtime_context") - assert set(event.keys()) == BASE_FIELDS | {"code", "details"} + assert set(event.keys()) == BASE_FIELDS | {"code"} assert event["code"] == "missing_runtime_context" - assert event["details"] == {"reason": "not_provided"} - - -def test_terminal_error_event_keeps_an_emptied_details_object() -> None: - """A dropped reason still leaves the code, which the client reads.""" - - event = create_terminal_task_error_event( - 1, - "x", - code="missing_runtime_context", - details=PublicErrorDetails(reason="not a listed value"), - ) - - assert event["code"] == "missing_runtime_context" - assert event["details"] == {} - - -class _DuckDetails: - def to_wire(self) -> dict[str, str]: - return {"reason": "not a listed value"} - - -@dataclass(frozen=True) -class _SubclassDetails(PublicErrorDetails): - raw: str = "" - - def to_wire(self) -> dict[str, str]: - # Never reads self.reason, so __post_init__'s whitelist is bypassed. - return {"reason": self.raw} - - -@pytest.mark.parametrize( - "details", - [ - {"reason": "not a listed value"}, - "not a listed value", - _DuckDetails(), - _SubclassDetails(reason=None, raw="not a listed value"), - ], - ids=["dict", "str", "duck-type", "subclass"], -) -def test_public_error_details_is_the_only_accepted_shape( - details: Any, - caplog: pytest.LogCaptureFixture, -) -> None: - """The annotation is not the door; the explicit check in the body is. - - The subclass case is why the check reads ``type(...) is`` rather than - ``isinstance``: a frozen dataclass can be subclassed, and a subclass that - overrides ``to_wire`` without reading ``self.reason`` satisfies both mypy - and ``isinstance`` while writing an unlisted string into the frame. - - Rejection drops the argument, it does not raise. The frame is the last - thing between the user and a silent failure, and the one caller that - passes these arguments builds the frame inside an ``except Exception`` - that only logs -- so an exception here would cost the whole frame. - """ - - with caplog.at_level(logging.ERROR): - event = create_terminal_task_error_event( - 1, "x", code="missing_runtime_context", details=details - ) - - # The unlisted string the bad shape wanted to smuggle in never appears. - assert set(event.keys()) == BASE_FIELDS - assert "not a listed value" not in json.dumps(event) - - dropped = [ - record.getMessage() - for record in caplog.records - if record.levelno == logging.ERROR and "dropped=details" in record.getMessage() - ] - assert len(dropped) == 1 - assert type(details).__name__ in dropped[0] + assert "details" not in event def test_unknown_code_is_dropped_and_logged( @@ -142,7 +59,6 @@ def test_unknown_code_is_dropped_and_logged( 1, "x", code="not_a_listed_code", - details=PublicErrorDetails(reason="not_provided"), ) assert set(event.keys()) == BASE_FIELDS @@ -172,9 +88,7 @@ def test_a_non_string_code_is_dropped_without_raising( path. All three are pinned so the outcome is the same shape either way. """ with caplog.at_level(logging.ERROR): - event = create_terminal_task_error_event( - 1, "x", code=code, details=PublicErrorDetails(reason="not_provided") - ) + event = create_terminal_task_error_event(1, "x", code=code) assert set(event.keys()) == BASE_FIELDS dropped = [ record.getMessage() @@ -202,9 +116,7 @@ def test_a_non_string_code_is_dropped_without_raising( def test_every_connector_runtime_code_survives_the_closed_set(code: str) -> None: """All ten connector-runtime codes are members, so none is dropped.""" - event = create_terminal_task_error_event( - 1, "x", code=code, details=PublicErrorDetails(reason="not_provided") - ) + event = create_terminal_task_error_event(1, "x", code=code) assert event["code"] == code diff --git a/tests/web/services/test_client_error_messages.py b/tests/web/services/test_client_error_messages.py index df9ba194c0..5ba049a759 100644 --- a/tests/web/services/test_client_error_messages.py +++ b/tests/web/services/test_client_error_messages.py @@ -2,39 +2,21 @@ The projection has two halves and both are pinned here: the message adapter (fail-closed on anything that is not a ``ConnectorRuntimeError``) and the -``(code, details)`` projector whose reason whitelist lives inside -``PublicErrorDetails.__post_init__``. +code projector, which is fail-closed the same way. """ from __future__ import annotations -import ast -import re -from pathlib import Path - import pytest from xagent.core.tools.adapters.vibe.config import RequiredMCPUnavailableError -from xagent.core.tools.adapters.vibe.connector_runtime import ( - RUNTIME_INPUT_AUTH_SELECTOR, - RUNTIME_INPUT_CONTEXT, - RUNTIME_INPUT_SECRETS, - ConnectorRuntimeError, -) -from xagent.web.services import client_error_messages +from xagent.core.tools.adapters.vibe.connector_runtime import ConnectorRuntimeError from xagent.web.services.client_error_messages import ( CLIENT_SAFE_TASK_FAILURE, - CONNECTOR_RUNTIME_PUBLIC_REASONS, - PublicErrorDetails, + connector_runtime_client_code, connector_runtime_client_message, - connector_runtime_public_error, ) -# Anchored on a real module file rather than on the package: xagent is a -# namespace package, so it has no __file__ of its own and may span trees. -SRC_ROOT = Path(client_error_messages.__file__).resolve().parents[2] - - # -------------------------------------------------------------------------- # connector_runtime_client_message # -------------------------------------------------------------------------- @@ -77,533 +59,31 @@ def test_client_message_is_fail_closed_for_an_incidental_exception( # -------------------------------------------------------------------------- -# I-3b: the reason whitelist lives in the constructor +# connector_runtime_client_code # -------------------------------------------------------------------------- -ILLEGAL_REASONS: list[object] = [ - # The real str(exc) product of validate_runtime_source_key. - "runtime input key must match [A-Za-z0-9_-]+", - "The connector could not resolve tenant acme-corp", - "missing_context.auth_token\nSELECT * FROM connectors", - "missing_context.'auth_token'", - "/etc/xagent/connectors/acme.yaml", - "SELECT value FROM task_connector_runtime_contexts WHERE task_id = 1", - "x" * 5120, - object(), - # Shape-legal, deliberately withheld: both state something about who owns - # the task or how an authorization check resolved, and this frame reaches - # anonymous widget and share-link visitors. - "runtime_owner_mismatch", - "runtime_task_identity_mismatch", - # Shape-legal, free of ownership and authorization content, and withheld - # anyway: the key half is a name the connector's owner declared, and this - # frame reaches anonymous widget and share-link visitors. Owners read key - # names from the per-task requirements endpoint, which selects on - # Task.id == task_id AND Task.user_id == current_user.id. - "missing_context.auth_token", - "missing_context.tenant_secret", - "type_mismatch.context.tenant_id", - "conflict.secrets.authorization", -] - - -@pytest.mark.parametrize("reason", ILLEGAL_REASONS) -def test_public_error_details_normalizes_reason(reason: object) -> None: - details = PublicErrorDetails(reason=reason) # type: ignore[arg-type] - - assert details.reason is None - assert details.to_wire() == {} - - -LEGAL_REASONS = [ - "not_provided", - "store_lost", - "connector_not_selected", - "undeclared_context_key", - "team_env_resolution_failed", - "team_scope_resolution_failed", - "runtime_view_resolution_failed", - "custom_api_config_load_failed", -] - - -@pytest.mark.parametrize("reason", LEGAL_REASONS) -def test_public_error_details_keeps_a_listed_reason(reason: str) -> None: - details = PublicErrorDetails(reason=reason) - - assert details.reason == reason - assert details.to_wire() == {"reason": reason} - - -def test_public_error_details_accepts_an_absent_reason() -> None: - assert PublicErrorDetails(reason=None).to_wire() == {} - - -# -------------------------------------------------------------------------- -# connector_runtime_public_error: the three read tiers of exc.details -# -------------------------------------------------------------------------- - - -def test_public_error_projects_code_and_whitelisted_reason() -> None: - error = ConnectorRuntimeError( - "runtime_secret_unavailable", - "Required runtime secret is unavailable.", - details={"reason": "not_provided"}, - ) - - projected = connector_runtime_public_error(error) - - assert projected is not None - code, details = projected - # Asserted through to_wire(), not by comparing to a second - # PublicErrorDetails: the comparison value runs the same __post_init__, so - # a whitelist that stopped admitting this reason would null both sides and - # the assertion would pass while verifying nothing. - assert code == "runtime_secret_unavailable" - assert details.to_wire() == {"reason": "not_provided"} - - -def test_a_reason_built_from_a_declared_key_name_never_reaches_the_wire() -> None: - """The one reason in this repository assembled from owner-written text. - - ``_require_context_values`` raises ``missing_context.``, where the key - is a name the connector's owner chose. It is dropped whole rather than - trimmed to its prefix: a prefix that only ever pairs with a dropped key - tells a visitor nothing the code has not already told them. - """ - +def test_client_code_projects_a_connector_runtime_error() -> None: error = ConnectorRuntimeError( "missing_runtime_context", "Required connector runtime context is missing.", - details={"reason": "missing_context.auth_token"}, - ) - - projected = connector_runtime_public_error(error) - - assert projected is not None - code, details = projected - assert code == "missing_runtime_context" - assert details.to_wire() == {} - - -@pytest.mark.parametrize( - "details", - [ - {}, - {"reason": "the connector could not be reached"}, - {"connector_ref": {"id": 7}}, - ], -) -def test_public_error_reads_an_empty_reason_as_a_present_code( - details: dict[str, object], -) -> None: - """Read-empty is not read-failed: the code still reaches the client.""" - - error = ConnectorRuntimeError("missing_runtime_context", "x", details=details) - - assert connector_runtime_public_error(error) == ( - "missing_runtime_context", - PublicErrorDetails(reason=None), ) - -def test_public_error_refuses_a_tampered_details_payload() -> None: - """A details of the wrong shape means the whole instance is untrusted.""" - - error = ConnectorRuntimeError("missing_runtime_context", "x") - error.details = "not a mapping" # type: ignore[assignment] - - assert connector_runtime_public_error(error) is None + assert connector_runtime_client_code(error) == "missing_runtime_context" @pytest.mark.parametrize( "error", [ - ValueError("boom"), - RuntimeError("boom"), - RequiredMCPUnavailableError("boom"), + ValueError("secret-token-xyz"), + KeyError("secret-token-xyz"), + RuntimeError("secret-token-xyz"), + RequiredMCPUnavailableError("secret-token-xyz"), ], ) -def test_public_error_does_not_project_an_incidental_exception( +def test_client_code_is_fail_closed_for_an_incidental_exception( error: BaseException, ) -> None: - assert connector_runtime_public_error(error) is None - - -# -------------------------------------------------------------------------- -# I-3: nothing but reason can reach the wire -# -------------------------------------------------------------------------- - - -def test_public_error_drops_every_field_but_reason() -> None: - error = ConnectorRuntimeError( - "missing_runtime_context", - "x", - details={ - "reason": "not_provided", - "internal_sql": "SELECT 1", - "raw_value": "tenant-secret", - "connector_ref": {"id": 7, "name": "acme"}, - }, - ) - - projected = connector_runtime_public_error(error) - - assert projected is not None - assert set(projected[1].to_wire()) == {"reason"} - - -def _python_sources() -> list[Path]: - return sorted(SRC_ROOT.rglob("*.py")) - - -# -------------------------------------------------------------------------- -# I-31: the whitelist and the real raise sites stay in step -# -------------------------------------------------------------------------- - - -# Literal reasons that the derivation below finds and that are deliberately -# kept off the wire. The first two state the task's ownership and the outcome -# of an authorization check; the audience of this frame includes anonymous -# widget and share-link visitors. The last two are safe fixed strings but are -# English sentences rather than enum values, and rewriting them would mean -# touching an old path this change has no bearing on. The rest are built from -# an exception message, so their content is not controlled. -DELIBERATELY_NOT_PUBLIC_REASONS = frozenset( - { - "runtime_owner_mismatch", - "runtime_task_identity_mismatch", - "runtime section must be an object", - "stored selected refs must be a list", - } -) - - -def _module_string_constants(tree: ast.Module) -> dict[str, str]: - constants: dict[str, str] = {} - for node in tree.body: - if isinstance(node, ast.Assign) and isinstance(node.value, ast.Constant): - if isinstance(node.value.value, str): - for target in node.targets: - if isinstance(target, ast.Name): - constants[target.id] = node.value.value - return constants - - -def _string_bindings( - tree: ast.Module, module_constants: dict[str, str] -) -> dict[str, set[str]]: - """Every ``name = `` binding in the module, flattened across scopes. - - ``module_constants`` is repo-wide so that a reason passed as an imported - constant (``reason=RUNTIME_SECRET_REASON_NOT_PROVIDED``) still resolves - without this scan having to follow imports. Flattening scopes is - deliberate for the same reason: this answers "which literal strings can - end up in a reason", and over-approximating there is the safe direction. - """ - - bindings: dict[str, set[str]] = { - name: {value} for name, value in module_constants.items() - } - - def resolve(node: ast.expr) -> set[str]: - if isinstance(node, ast.Constant) and isinstance(node.value, str): - return {node.value} - if isinstance(node, ast.Name): - value = module_constants.get(node.id) - return {value} if value is not None else set() - if isinstance(node, ast.IfExp): - return resolve(node.body) | resolve(node.orelse) - return set() - - for node in ast.walk(tree): - if isinstance(node, ast.Assign): - values = resolve(node.value) - if not values: - continue - for target in node.targets: - if isinstance(target, ast.Name): - bindings.setdefault(target.id, set()).update(values) - return bindings - - -def _fstring_pattern(node: ast.JoinedStr) -> str | None: - """Turn an f-string reason into a regex covering everything it can build.""" - - parts: list[str] = [] - for value in node.values: - if isinstance(value, ast.Constant) and isinstance(value.value, str): - parts.append(re.escape(value.value)) - elif isinstance(value, ast.FormattedValue): - parts.append(".+") - else: - return None - return "^" + "".join(parts) + "$" - - -def _reason_expressions(tree: ast.Module) -> list[ast.expr]: - """Every expression that becomes a reason on a ConnectorRuntimeError. - - Derived from the construction target, not from a list of modules: a module - list would silently stop covering a raise site added somewhere new. - """ - - found: list[ast.expr] = [] - for node in ast.walk(tree): - if not isinstance(node, ast.Call) or not isinstance(node.func, ast.Name): - continue - if node.func.id == "_raise_runtime_error": - for keyword in node.keywords: - if keyword.arg == "reason": - found.append(keyword.value) - elif node.func.id == "ConnectorRuntimeError": - for keyword in node.keywords: - if keyword.arg != "details" or not isinstance(keyword.value, ast.Dict): - continue - for key, value in zip(keyword.value.keys, keyword.value.values): - if isinstance(key, ast.Constant) and key.value == "reason": - found.append(value) - return found - - -def _derive_reasons() -> tuple[set[str], set[str]]: - trees = { - path: ast.parse(path.read_text(encoding="utf-8")) for path in _python_sources() - } - module_constants: dict[str, str] = {} - for tree in trees.values(): - module_constants.update(_module_string_constants(tree)) - - literals: set[str] = set() - patterns: set[str] = set() - for tree in trees.values(): - bindings = _string_bindings(tree, module_constants) - for expression in _reason_expressions(tree): - if isinstance(expression, ast.Constant) and isinstance( - expression.value, str - ): - literals.add(expression.value) - elif isinstance(expression, ast.Name): - literals.update(bindings.get(expression.id, set())) - elif isinstance(expression, ast.JoinedStr): - pattern = _fstring_pattern(expression) - if pattern is not None: - patterns.add(pattern) - return literals, patterns - - -def _is_listed(reason: str) -> bool: - return reason in CONNECTOR_RUNTIME_PUBLIC_REASONS - - -def test_public_reason_whitelist_covers_every_raise_site() -> None: - literals, _ = _derive_reasons() - - assert literals, "the reason derivation found nothing; the scan is broken" - - unclassified = { - reason - for reason in literals - if not _is_listed(reason) and reason not in DELIBERATELY_NOT_PUBLIC_REASONS - } - assert not unclassified, ( - "these reasons are raised but neither whitelisted nor listed as " - f"deliberately withheld: {sorted(unclassified)}" - ) - - -def test_public_reason_whitelist_has_no_member_without_a_raise_site() -> None: - """Every listed reason is produced somewhere, with no exemptions. - - Zero exemptions is the point of this assertion. A listed reason nothing - raises is a standing allowance with no expiry, and by the time the code - raising it arrives nobody remembers which audience it was judged against. - A reason therefore enters the whitelist in the same change as the site - that raises it. - """ - - literals, patterns = _derive_reasons() - compiled = [re.compile(pattern) for pattern in patterns] - - ungrounded = { - reason - for reason in CONNECTOR_RUNTIME_PUBLIC_REASONS - if reason not in literals - and not any(expression.match(reason) for expression in compiled) - } - assert not ungrounded, ( - f"these whitelisted reasons are produced nowhere in src/: {sorted(ungrounded)}" - ) - - -def test_the_withheld_reasons_are_really_raised_somewhere() -> None: - """A withheld entry that nothing raises is a stale exemption.""" - - literals, _ = _derive_reasons() - - assert DELIBERATELY_NOT_PUBLIC_REASONS <= literals - - -def test_knowledge_base_scope_reason_is_not_in_the_derived_surface() -> None: - """A same-named literal on a different exception class must stay out. - - ``knowledge_base_team_scope`` raises ``KnowledgeBaseScopeError`` with the - same ``team_scope_resolution_failed`` string. Deriving by construction - target rather than by module list is what keeps it out on its own. - """ - - path = SRC_ROOT / "web" / "services" / "knowledge_base_team_scope.py" - tree = ast.parse(path.read_text(encoding="utf-8")) - - assert _reason_expressions(tree) == [] - - -def test_no_reason_assembled_by_interpolation_is_admitted_by_its_shape() -> None: - """A reason with an interpolated half is never admitted by its shape. - - The two assertions above read only the literal reasons, because a reason - built by interpolation has no single value to look up. This one reads the - derived shapes instead: for every interpolated reason the scan finds, an - arbitrary instantiation of it must be dropped. Admitting a whole shape is - how a name the connector's owner chose reaches a visitor without any one - line of code saying so, and this is the assertion that a re-added prefix - set breaks. - """ - - _, patterns = _derive_reasons() - - assert patterns, "the derivation found no interpolated reason; the scan is broken" - for pattern in patterns: - probe = ( - pattern.removeprefix("^") - .removesuffix("$") - .replace("\\.", ".") - .replace(".+", "zzz-probe-zzz") - ) - assert PublicErrorDetails(reason=probe).to_wire() == {}, ( - f"a reason of shape {pattern} is admitted by its shape: {probe}" - ) - - -def test_the_interpolated_reasons_are_grounded_in_their_section_names() -> None: - """The three undeclared_* members pass only via the f-string pattern. - - A pattern is an over-approximation: ``^undeclared_.+_key$`` would also - admit a listed reason nothing raises. Pin both halves -- the pattern set - the derivation actually produces, and the exact members it is allowed to - ground -- against the section names the raise site loops over. - - The derivation finds a second pattern, ``^missing_context\\..+$`` - (connector_runtime.py:857's ``f"missing_context.{key}"``), and it is - deliberately ungrounded: that reason is built from a key name the - connector's owner declared, and ``_is_public_reason``'s own docstring - names this exact shape as the one an owner-controlled interpolation must - not admit. ``test_no_reason_assembled_by_interpolation_is_admitted_by_its_shape`` - above already asserts every pattern this derivation finds is rejected by - shape; this test only pins which patterns exist and grounds the one that - is supposed to resolve to real whitelist members. - """ - - _, patterns = _derive_reasons() - assert patterns == {"^undeclared_.+_key$", "^missing_context\\..+$"} - expected = { - f"undeclared_{section}_key" - for section in ( - RUNTIME_INPUT_CONTEXT, - RUNTIME_INPUT_SECRETS, - RUNTIME_INPUT_AUTH_SELECTOR, - ) - } - assert { - reason - for reason in CONNECTOR_RUNTIME_PUBLIC_REASONS - if reason.startswith("undeclared_") - } == expected - - -def test_no_construction_site_hides_its_reason_from_the_scanner() -> None: - """The scanner has two blind spots; neither is occupied today. - - It reads a ``details=`` argument only when it is a literal dict, and it - matches a construction only when the callee is a bare name. Both are safe - only while nothing sits in them, so pin both: every non-literal - ``details=`` belongs to the one indirection the scanner handles on its own - (``_raise_runtime_error``, whose ``reason`` keyword it reads at the outer - call sites instead), and no construction reaches the class through an - attribute (``module.ConnectorRuntimeError(...)``). - - Sites are keyed by (file, line), not by file alone: a second non-literal - ``details=`` call added to the same file the one known site already lives - in must still change this set, or the assertion below would not notice a - new blind-spot occupant landing next to the one it already admits. - """ - - non_literal_details_sites: set[tuple[str, int]] = set() - attribute_construction_sites: set[str] = set() - - for path in _python_sources(): - tree = ast.parse(path.read_text(encoding="utf-8")) - relative = path.relative_to(SRC_ROOT).as_posix() - for node in ast.walk(tree): - if not isinstance(node, ast.Call): - continue - if ( - isinstance(node.func, ast.Name) - and node.func.id == "ConnectorRuntimeError" - ): - for keyword in node.keywords: - if keyword.arg == "details" and not isinstance( - keyword.value, ast.Dict - ): - non_literal_details_sites.add((relative, node.lineno)) - if ( - isinstance(node.func, ast.Attribute) - and node.func.attr == "ConnectorRuntimeError" - ): - attribute_construction_sites.add(relative) - - assert non_literal_details_sites == {("web/services/connector_runtime.py", 1007)} - assert attribute_construction_sites == set() - - -def test_every_opaque_reason_expression_is_a_str_call() -> None: - """Five reason expressions in this repository are opaque calls. - - Three pass ``reason=str(exc)`` into ``_raise_runtime_error``; two spell - ``details={"reason": str(exc)}`` on a direct construction. Both shapes are - collected by ``_reason_expressions``, and neither resolves to a literal -- - the runtime whitelist is what keeps them off the wire. Pin the shape so a - new opaque reason expression (a %-format, a .format(), a join) shows up as - a failure here instead of silently leaving the derivation. - """ - - trees = { - path: ast.parse(path.read_text(encoding="utf-8")) for path in _python_sources() - } - module_constants: dict[str, str] = {} - for tree in trees.values(): - module_constants.update(_module_string_constants(tree)) - - opaque: list[ast.expr] = [] - for tree in trees.values(): - bindings = _string_bindings(tree, module_constants) - for expression in _reason_expressions(tree): - if isinstance(expression, ast.Constant) and isinstance( - expression.value, str - ): - continue - if isinstance(expression, ast.Name) and bindings.get(expression.id): - continue - if isinstance(expression, ast.JoinedStr): - continue - opaque.append(expression) + """The specific name is not the gate; the isinstance check is.""" - assert len(opaque) == 5, ( - "expected 5 opaque reason expressions (reason=str(exc) at " - "connector_runtime.py:824/854/880, details={'reason': str(exc)} at " - f":540/773), found {len(opaque)}" - ) - for expression in opaque: - assert isinstance(expression, ast.Call) - assert isinstance(expression.func, ast.Name) and expression.func.id == "str" + assert connector_runtime_client_code(error) is None diff --git a/tests/web/services/test_task_orchestrator.py b/tests/web/services/test_task_orchestrator.py index f3c564d228..851ab40ace 100644 --- a/tests/web/services/test_task_orchestrator.py +++ b/tests/web/services/test_task_orchestrator.py @@ -71,10 +71,7 @@ inspect_user_message_delivery, mark_user_message_delivery, ) -from xagent.web.services.client_error_messages import ( - CLIENT_SAFE_TASK_FAILURE, - PublicErrorDetails, -) +from xagent.web.services.client_error_messages import CLIENT_SAFE_TASK_FAILURE from xagent.web.services.connector_runtime import ( get_ephemeral_runtime_values, pop_ephemeral_runtime_values, @@ -4015,8 +4012,8 @@ async def test_incidental_failure_still_redacts( @pytest.mark.asyncio -async def test_connector_runtime_frame_details_shape(db_session) -> None: - """Whatever the raise site attached, only ``reason`` can reach the wire.""" +async def test_connector_runtime_frame_carries_code_only(db_session) -> None: + """Whatever the raise site attached to ``details``, none of it reaches the wire.""" error = ConnectorRuntimeError( "runtime_secret_unavailable", @@ -4037,8 +4034,16 @@ async def test_connector_runtime_frame_details_shape(db_session) -> None: task = db_session.query(Task).filter(Task.id == task_id).one() await _run_failing_turn(task_id, int(task.user_id), task.source) - assert set(frames[0]["details"]) <= {"reason"} - assert frames[0]["details"] == {"reason": PUBLIC_REASON} + assert set(frames[0]) == { + "type", + "message", + "task_id", + "task", + "error", + "timestamp", + "code", + } + assert "details" not in frames[0] @pytest.mark.asyncio @@ -4077,9 +4082,7 @@ async def test_connector_runtime_frame_never_carries_connector_ref( "error", "timestamp", "code", - "details", } - assert set(frame["details"]) <= {"reason"} serialized = json.dumps(frame) assert "connector_ref" not in serialized assert "connector_id" not in serialized @@ -4089,7 +4092,7 @@ async def test_connector_runtime_frame_never_carries_connector_ref( async def test_connector_runtime_frame_reason_matches_direct_construction( db_session, ) -> None: - """End to end, the frame carries exactly what the type would produce.""" + """End to end, a listed reason on the exception still never reaches the wire.""" error = ConnectorRuntimeError( "missing_runtime_context", @@ -4105,8 +4108,7 @@ async def test_connector_runtime_frame_reason_matches_direct_construction( task = db_session.query(Task).filter(Task.id == task_id).one() await _run_failing_turn(task_id, int(task.user_id), task.source) - assert frames[0]["details"] == PublicErrorDetails(reason=PUBLIC_REASON).to_wire() - assert frames[0]["details"] == {"reason": PUBLIC_REASON} + assert "details" not in frames[0] @pytest.mark.asyncio @@ -4185,11 +4187,11 @@ async def test_connector_runtime_failure_persists_client_safe_history( # the client replaces it with its own localized wording (see the # "terminal error frames" suite in app-context-chat.test.tsx). What the two # views owe each other is the facts they carry, and the key name is in - # neither -- the whitelist drops the reason that names it, so the frame - # cannot carry it and the client cannot render it. + # neither -- the frame never carries a details object at all, so there is + # no key name for the client to render. assert settled["client_error_message"] == safe_message assert settled["client_error_message"] == frames[0]["message"] - assert frames[0]["details"] == {} + assert "details" not in frames[0] assert "auth_token" not in json.dumps(frames[0]) # The durable error keeps the code prefix operators grep for, and never # the "setup/run error: " shape the else branch produces. From 1710e8b79fb40e25afccb081acd5caec1e9e8dd7 Mon Sep 17 00:00:00 2001 From: Alexliu Date: Fri, 4 Sep 2026 00:34:21 +0800 Subject: [PATCH 23/28] fix(web): scope the terminal-frame code closed set to connector-runtime 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. --- frontend/src/lib/client-errors.ts | 27 ++-- src/xagent/web/api/websocket.py | 22 +-- .../web/services/client_error_messages.py | 35 ++++- .../web/api/test_terminal_task_error_event.py | 146 ++++++++++++++++-- 4 files changed, 188 insertions(+), 42 deletions(-) diff --git a/frontend/src/lib/client-errors.ts b/frontend/src/lib/client-errors.ts index 98f9fa9426..8604264552 100644 --- a/frontend/src/lib/client-errors.ts +++ b/frontend/src/lib/client-errors.ts @@ -31,20 +31,19 @@ const CLIENT_ERROR_CODES = [ // reach that frame today are listed: a listed code nothing produces is an // entry with no expiry date. Which codes those are is a fact about this // repository's raise sites, not a property the wire holds -- the field is - // typed as a bare string and validated only against the full V1ErrorCode - // set (see websocket.py's own note above that check), and a resolver - // installed through set_connector_runtime_resolver lives outside this - // repository and can raise any member. The other five members of the - // connector-runtime family are absent because nothing here produces them on - // this path: two have no raise site in this repository at all, and 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 this table does not list keeps the generic - // prefixed wording. + // typed as a bare string and validated against the connector-runtime closed + // set the server keeps next to its fallback table, and a resolver installed + // through set_connector_runtime_resolver lives outside this repository and + // can raise any member of that set. The closed set has eight members, and + // only five have their own entry below; the other three -- + // connector_not_found, runtime_context_immutable, and + // runtime_secret_not_allowed -- have no producer that can reach a terminal + // frame in this repository today, so they keep the generic prefixed + // wording. Two further connector-runtime codes are outside the closed set + // entirely (mcp_oauth_authorization_failed, delegated_authorization_failed): + // each names the outcome of an authorization check, so the server drops + // them before this frame is built. A code this table does not list keeps + // the generic prefixed wording. "missing_runtime_context", "runtime_secret_unavailable", "scheduled_secret_unavailable", diff --git a/src/xagent/web/api/websocket.py b/src/xagent/web/api/websocket.py index e57b5672eb..727fcb35d3 100644 --- a/src/xagent/web/api/websocket.py +++ b/src/xagent/web/api/websocket.py @@ -104,6 +104,7 @@ CLIENT_SAFE_GUIDANCE_IN_PROGRESS, CLIENT_SAFE_TASK_FAILURE, CLIENT_SAFE_VALIDATION_ERROR, + CONNECTOR_RUNTIME_CLIENT_ERROR_CODES, ClientErrorCode, client_error_message, ) @@ -332,20 +333,6 @@ def _task_error_payload( return payload -def _client_visible_error_codes() -> frozenset[str]: - """The closed set of client-visible error codes, reused not recopied. - - Imported inside the function on purpose: the ``v1`` package's ``__init__`` - pulls in routers that import this module, so a module-level import would - close a cycle. Rebuilt per call: a ~30-member frozenset is cheaper than a - cache to reason about. - """ - - from .v1.errors import V1ErrorCode - - return frozenset(member.value for member in V1ErrorCode) - - def create_terminal_task_error_event( task_id: int, message: str, @@ -364,8 +351,7 @@ def create_terminal_task_error_event( failure this path exists to remove. A bad optional argument costs that argument and nothing else. The rejection is logged with its stack. - ``code`` must be a member of the connector-runtime family the client - renders, the repository's closed set of client-visible error codes. + ``code`` must be a member of ``CONNECTOR_RUNTIME_CLIENT_ERROR_CODES``. """ # Python annotations are not enforced at run time, so the mypy gate on the @@ -375,13 +361,13 @@ def create_terminal_task_error_event( # instead. # # ConnectorRuntimeError types its code as a bare str and stores it - # unvalidated, so "only the ten module constants reach here" is a fact + # unvalidated, so "only the eight module constants reach here" is a fact # about today's raise sites, not a property the code holds. The type # check comes first for the same reason: annotations are not enforced, # and an unhashable value would raise inside the membership test on a # path whose whole point is that it never raises. if code is not None and ( - not isinstance(code, str) or code not in _client_visible_error_codes() + not isinstance(code, str) or code not in CONNECTOR_RUNTIME_CLIENT_ERROR_CODES ): logger.error( "task_id=%s component=terminal-error-frame dropped=code " diff --git a/src/xagent/web/services/client_error_messages.py b/src/xagent/web/services/client_error_messages.py index 564ba16333..969d5ee5e4 100644 --- a/src/xagent/web/services/client_error_messages.py +++ b/src/xagent/web/services/client_error_messages.py @@ -9,7 +9,17 @@ from enum import StrEnum from ...core.tools.adapters.vibe.config import RequiredMCPUnavailableError -from ...core.tools.adapters.vibe.connector_runtime import ConnectorRuntimeError +from ...core.tools.adapters.vibe.connector_runtime import ( + ERROR_CONNECTOR_NOT_FOUND, + ERROR_CONNECTOR_RUNTIME_UNAVAILABLE, + ERROR_INVALID_RUNTIME_CONTEXT, + ERROR_MISSING_RUNTIME_CONTEXT, + ERROR_RUNTIME_CONTEXT_IMMUTABLE, + ERROR_RUNTIME_SECRET_NOT_ALLOWED, + ERROR_RUNTIME_SECRET_UNAVAILABLE, + ERROR_SCHEDULED_SECRET_UNAVAILABLE, + ConnectorRuntimeError, +) CLIENT_SAFE_VALIDATION_ERROR = "The message could not be processed. Please try again." @@ -162,3 +172,26 @@ def connector_runtime_client_code(error: BaseException) -> str | None: return None code = error.code return code if isinstance(code, str) else None + + +# The connector-runtime codes a terminal task_error frame may carry. Every +# member is raised as a ``ConnectorRuntimeError`` somewhere in this +# repository today, and none of them states who owns the task or how an +# authorization check resolved -- the two questions a value has to answer +# "no" to before it may reach anonymous widget and share-link visitors. +# ``mcp_oauth_authorization_failed`` and ``delegated_authorization_failed`` +# are deliberately absent: nothing here raises them as this exception, and +# each one is the outcome of an authorization check. Add a code here in the +# same change that adds the raise site, never ahead of it. +CONNECTOR_RUNTIME_CLIENT_ERROR_CODES = frozenset( + { + ERROR_CONNECTOR_NOT_FOUND, + ERROR_INVALID_RUNTIME_CONTEXT, + ERROR_MISSING_RUNTIME_CONTEXT, + ERROR_RUNTIME_CONTEXT_IMMUTABLE, + ERROR_RUNTIME_SECRET_NOT_ALLOWED, + ERROR_RUNTIME_SECRET_UNAVAILABLE, + ERROR_SCHEDULED_SECRET_UNAVAILABLE, + ERROR_CONNECTOR_RUNTIME_UNAVAILABLE, + } +) diff --git a/tests/web/api/test_terminal_task_error_event.py b/tests/web/api/test_terminal_task_error_event.py index 8ab54f8544..f7d06e634f 100644 --- a/tests/web/api/test_terminal_task_error_event.py +++ b/tests/web/api/test_terminal_task_error_event.py @@ -7,19 +7,32 @@ from __future__ import annotations +import ast import json import logging +from pathlib import Path from typing import Any import pytest -from xagent.web.api.websocket import ( - _client_visible_error_codes, - create_terminal_task_error_event, +import xagent +from xagent.core.tools.adapters.vibe import ( + connector_runtime as connector_runtime_module, +) +from xagent.web.api.v1.errors import V1ErrorCode +from xagent.web.api.websocket import create_terminal_task_error_event +from xagent.web.services.client_error_messages import ( + CONNECTOR_RUNTIME_CLIENT_ERROR_CODES, ) BASE_FIELDS = {"type", "message", "task_id", "task", "error", "timestamp"} +# Anchored on a real package file rather than assumed relative to this test +# file: xagent is a namespace package, so it has no single __file__ of its +# own, but the first path entry is the actual source tree to scan. +SRC_ROOT = Path(xagent.__path__[0]) +RAISE_CALL_NAMES = {"_raise_runtime_error", "ConnectorRuntimeError"} + @pytest.mark.parametrize( "kwargs", @@ -109,19 +122,134 @@ def test_a_non_string_code_is_dropped_without_raising( "runtime_secret_unavailable", "scheduled_secret_unavailable", "connector_runtime_unavailable", - "mcp_oauth_authorization_failed", - "delegated_authorization_failed", ], ) def test_every_connector_runtime_code_survives_the_closed_set(code: str) -> None: - """All ten connector-runtime codes are members, so none is dropped.""" + """All eight connector-runtime codes are members, so none is dropped.""" event = create_terminal_task_error_event(1, "x", code=code) assert event["code"] == code -def test_the_closed_set_is_the_v1_one_not_a_copy() -> None: - from xagent.web.api.v1.errors import V1ErrorCode +def test_the_closed_set_is_the_connector_runtime_subset_of_v1() -> None: + """The closed set is a curated subset of V1ErrorCode, not the whole enum. + + Membership excludes the two authorization-outcome codes: nothing raises + them as a ``ConnectorRuntimeError`` today, and each one is the outcome of + an authorization check -- the kind of fact this frame must not carry. + """ + + assert CONNECTOR_RUNTIME_CLIENT_ERROR_CODES == { + "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", + } + assert CONNECTOR_RUNTIME_CLIENT_ERROR_CODES <= { + member.value for member in V1ErrorCode + } + + +def test_a_non_connector_v1_code_is_dropped_and_logged( + caplog: pytest.LogCaptureFixture, +) -> None: + """A V1ErrorCode member outside the connector-runtime family is dropped. + + ``invalid_api_key`` is a real member of ``V1ErrorCode`` -- the /v1 error + surface -- but it is not a connector-runtime code, so it must not reach + this frame. + """ + + with caplog.at_level(logging.ERROR): + event = create_terminal_task_error_event(1, "x", code="invalid_api_key") + + assert "code" not in event + dropped = [ + record.getMessage() + for record in caplog.records + if record.levelno == logging.ERROR and "dropped=code" in record.getMessage() + ] + assert len(dropped) == 1 + + +@pytest.mark.parametrize( + "code", + ["mcp_oauth_authorization_failed", "delegated_authorization_failed"], +) +def test_authorization_outcome_codes_are_dropped(code: str) -> None: + """These two codes are the outcome of an authorization check. + + Neither is raised as a ``ConnectorRuntimeError`` in this repository + today, and both are deliberately absent from the closed set: this frame + reaches anonymous widget and share-link visitors, and the outcome of an + authorization check is exactly the kind of fact it must not carry. + """ + + event = create_terminal_task_error_event(1, "x", code=code) + + assert "code" not in event + + +def _raise_site_error_names() -> set[str]: + """The ``ERROR_*`` names this source tree actually produces as a code. + + Walks every ``.py`` file for two shapes: a call to + ``_raise_runtime_error(, ...)`` or ``ConnectorRuntimeError(, + ...)`` whose first positional argument is a bare name, and a bare + ``return `` -- some raise sites choose the code dynamically + through a small dispatch function (for example, one runtime-secret code + or its scheduled-trigger variant, picked by the failing task's source) + and pass the result through a local variable rather than the constant + itself, so the constant only appears literally at the ``return``. Either + shape is read as "this name is a code value somewhere in the raise + path," not as a full trace of which raise call it eventually reaches. + """ + + names: set[str] = set() + for path in SRC_ROOT.rglob("*.py"): + tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path)) + for node in ast.walk(tree): + if isinstance(node, ast.Return): + if isinstance(node.value, ast.Name) and node.value.id.startswith( + "ERROR_" + ): + names.add(node.value.id) + continue + if not isinstance(node, ast.Call): + continue + func = node.func + func_name = ( + func.id + if isinstance(func, ast.Name) + else (func.attr if isinstance(func, ast.Attribute) else None) + ) + if func_name not in RAISE_CALL_NAMES or not node.args: + continue + first_arg = node.args[0] + if isinstance(first_arg, ast.Name) and first_arg.id.startswith("ERROR_"): + names.add(first_arg.id) + return names + + +def test_every_client_code_has_a_raise_site() -> None: + """Every code in the closed set is actually produced somewhere. + + A code that reaches the closed set ahead of its raise site is an + allowance with no expiry date: nothing enforces that a promise like this + stays true, so this test derives the raise sites fresh from the AST + instead of trusting a hand-maintained list of them. + """ - assert _client_visible_error_codes() == {member.value for member in V1ErrorCode} + error_names = _raise_site_error_names() + assert error_names, "the AST scan found no ERROR_* raise sites at all" + resolved_codes = { + getattr(connector_runtime_module, name) + for name in error_names + if hasattr(connector_runtime_module, name) + } + assert CONNECTOR_RUNTIME_CLIENT_ERROR_CODES <= resolved_codes From 0ad1b7d99657ea5fc4741e5bcef050c5b32e6c9c Mon Sep 17 00:00:00 2001 From: Alexliu Date: Fri, 4 Sep 2026 00:39:20 +0800 Subject: [PATCH 24/28] test(web): pin where terminal-frame code arguments come from 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. --- .../test_terminal_task_error_frame_origins.py | 136 ++++++++++++++++++ 1 file changed, 136 insertions(+) create mode 100644 tests/web/api/test_terminal_task_error_frame_origins.py diff --git a/tests/web/api/test_terminal_task_error_frame_origins.py b/tests/web/api/test_terminal_task_error_frame_origins.py new file mode 100644 index 0000000000..af74075c95 --- /dev/null +++ b/tests/web/api/test_terminal_task_error_frame_origins.py @@ -0,0 +1,136 @@ +"""Pins where a terminal-frame ``code`` argument is allowed to come from. + +``create_terminal_task_error_event``'s own runtime gate only checks the +*value* of ``code`` (a member of the closed set, and nothing else) -- it has +no way to know whether that value came from a curated projector or from an +incidental string a future caller happened to have on hand. This test closes +that gap statically: every call site that passes ``code=`` must bind it, in +the same function, from a call to ``connector_runtime_client_code`` -- the +one function in this repository that projects an exception onto a +client-visible code. Anything else (a literal, ``str(exc)``, a field read off +the exception directly) is an unauthorized source, even if the value it +produces happens to be a real closed-set member today. +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +import xagent + +# Anchored on a real package file rather than assumed relative to this test +# file: xagent is a namespace package, so it has no single __file__ of its +# own, but the first path entry is the actual source tree to scan. +WEB_ROOT = Path(xagent.__path__[0]) / "web" + +PROJECTOR_NAME = "connector_runtime_client_code" + + +def _call_func_name(call: ast.Call) -> str | None: + func = call.func + if isinstance(func, ast.Name): + return func.id + if isinstance(func, ast.Attribute): + return func.attr + return None + + +def _enclosing_function( + node: ast.AST, parents: dict[ast.AST, ast.AST] +) -> ast.FunctionDef | ast.AsyncFunctionDef | None: + current = parents.get(node) + while current is not None: + if isinstance(current, (ast.FunctionDef, ast.AsyncFunctionDef)): + return current + current = parents.get(current) + return None + + +def _has_projector_binding( + func: ast.FunctionDef | ast.AsyncFunctionDef, bound_name: str +) -> bool: + """True when ``bound_name`` is assigned from a bare projector call. + + Matches only `` = connector_runtime_client_code(...)`` -- + a single-target ``Assign`` whose value is a direct call to the + projector. A tuple-unpacking or boolean-fallback shape does not count: + the projector already returns ``None`` for anything it does not + recognize, so a caller does not need (and should not add) a second + layer of fallback between the call and the frame. + """ + + for stmt in ast.walk(func): + if not isinstance(stmt, ast.Assign): + continue + if len(stmt.targets) != 1: + continue + target = stmt.targets[0] + if not (isinstance(target, ast.Name) and target.id == bound_name): + continue + value = stmt.value + if isinstance(value, ast.Call) and _call_func_name(value) == PROJECTOR_NAME: + return True + return False + + +def _scan() -> tuple[set[tuple[str, str]], int]: + """Returns (call sites that pass code=, count of recognized bindings). + + A call site is included in the first element only when its ``code=`` + argument is a bare name AND that name is bound, in the same enclosing + function, from a direct call to the projector -- anything else is a + hard failure, not a silently-excluded call site, so an unauthorized + source cannot pass this test by looking like an unrecognized shape. + """ + + call_sites: set[tuple[str, str]] = set() + recognized_bindings = 0 + + for path in sorted(WEB_ROOT.rglob("*.py")): + source = path.read_text(encoding="utf-8") + tree = ast.parse(source, filename=str(path)) + parents: dict[ast.AST, ast.AST] = {} + for node in ast.walk(tree): + for child in ast.iter_child_nodes(node): + parents[child] = node + + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + if _call_func_name(node) != "create_terminal_task_error_event": + continue + code_kw = next((kw for kw in node.keywords if kw.arg == "code"), None) + if code_kw is None: + continue + + func = _enclosing_function(node, parents) + func_name = func.name if func is not None else "" + rel_path = path.relative_to(WEB_ROOT.parent).as_posix() + + if not isinstance(code_kw.value, ast.Name): + raise AssertionError( + f"unauthorized code= source at {rel_path}:{node.lineno} " + f"in {func_name}: {ast.dump(code_kw.value)}" + ) + if func is None or not _has_projector_binding(func, code_kw.value.id): + raise AssertionError( + f"code= argument {code_kw.value.id!r} at " + f"{rel_path}:{node.lineno} in {func_name} is not bound " + f"from {PROJECTOR_NAME}(...) in the same function" + ) + + call_sites.add((rel_path, func_name)) + recognized_bindings += 1 + + return call_sites, recognized_bindings + + +def test_every_code_argument_traces_to_the_projector() -> None: + call_sites, recognized_bindings = _scan() + + assert call_sites == {("web/services/task_orchestrator.py", "_runner")} + # Guards against the scanner silently matching nothing: a rewritten + # projector call, a renamed binding, or a moved raise site should fail + # loudly here rather than let the assertion above pass on an empty set. + assert recognized_bindings == 1 From 946bfa7f043d2d4bfbe57790350b534fd6b6abd4 Mon Sep 17 00:00:00 2001 From: Alexliu Date: Fri, 4 Sep 2026 00:48:29 +0800 Subject: [PATCH 25/28] test(frontend): cover the resume-settlement task_error frame and the 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. --- .../src/contexts/app-context-chat.test.tsx | 98 +++++++++++++++++++ frontend/src/lib/client-errors.test.ts | 13 +++ frontend/src/lib/client-errors.ts | 2 +- 3 files changed, 112 insertions(+), 1 deletion(-) diff --git a/frontend/src/contexts/app-context-chat.test.tsx b/frontend/src/contexts/app-context-chat.test.tsx index ab6d9d8016..69cdbbe940 100644 --- a/frontend/src/contexts/app-context-chat.test.tsx +++ b/frontend/src/contexts/app-context-chat.test.tsx @@ -6388,6 +6388,74 @@ describe("terminal error frames", () => { ) }) }) + + // A cancellation carries no code (external_task_cancel.py:404 passes only + // a message), so isTerminal alone -- not a code -- has to make this frame + // the turn's result and route it through ADD_MESSAGE's isResult branch, + // the one place trace events accumulated on state.traceEvents move onto + // the settling message and state.traceEvents is cleared. + it("drains accumulated trace events onto the cancellation bubble", async () => { + render( + + + + + + ) + + const onMessage = webSocketOptions.current?.onMessage + expect(onMessage).toBeDefined() + + act(() => { + getSessionControls().dispatch({ + type: "ADD_TRACE_EVENT", + payload: { + event_id: "trace-1", + event_type: "agent_progress", + timestamp: "2026-05-27T05:00:01Z", + data: { message: "Reading the connector config" }, + }, + }) + getSessionControls().dispatch({ + type: "ADD_TRACE_EVENT", + payload: { + event_id: "trace-2", + event_type: "agent_progress", + timestamp: "2026-05-27T05:00:01.500Z", + data: { message: "Calling the tool" }, + }, + }) + }) + expect(getSessionControls().state.traceEvents).toHaveLength(2) + + act(() => { + onMessage?.({ + type: "task_error", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + task: { id: 1, status: "failed" }, + message: "This response was interrupted.", + error: "This response was interrupted.", + } as TestWebSocketMessage) + }) + + await waitFor(() => { + expect(screen.getByTestId("messages").textContent).toContain( + "This response was interrupted." + ) + }) + + expect(getSessionControls().state.traceEvents).toEqual([]) + const bubble = getSessionControls().state.messages.find( + (message) => + typeof message.content === "string" && + message.content.includes("This response was interrupted.") + ) + expect(bubble?.traceEvents?.map((event) => event.event_id)).toEqual([ + "trace-1", + "trace-2", + ]) + }) }) describe("error frame display projection", () => { @@ -6442,6 +6510,36 @@ describe("error frame display projection", () => { isResult: true, }, }, + { + // A resume that settles the task before the caller can hand it a code: + // websocket.py's resume-settlement broadcast (:3038) carries error_code + // on the root, the same field the non-terminal rejection channel + // uses -- not the code field create_terminal_task_error_event writes. + // getWebSocketErrorCode reads it regardless of which channel it came + // from, so dedupText picks up the coded wording; the bubble still gets + // the generic prefix, because that only drops for a frame carrying a + // `code` field, which this one does not. + name: "a resume-settlement frame with a root error_code on a trusted transport", + frame: { + type: "task_error", + timestamp: "2026-05-27T05:00:02Z", + task_id: 1, + task: { id: 1, status: "failed" }, + message: "Task execution failed.", + error: "Task execution failed.", + error_code: "task_execution_failed", + } as unknown as TaskControlMessage, + trustLegacyErrorProse: true, + expected: { + isTerminal: true, + taskStatus: "failed", + stopsProcessing: true, + dedupText: "clientErrors.taskExecutionFailed", + occurrenceIdentity: undefined, + bubbleContent: "agent.logs.event.messages.errorPrefix clientErrors.taskExecutionFailed", + isResult: true, + }, + }, { name: "a terminal frame with a missing-value code on an untrusted transport", frame: { diff --git a/frontend/src/lib/client-errors.test.ts b/frontend/src/lib/client-errors.test.ts index 80d58ee62f..8067534173 100644 --- a/frontend/src/lib/client-errors.test.ts +++ b/frontend/src/lib/client-errors.test.ts @@ -1,6 +1,8 @@ import { describe, expect, it } from "vitest" +import en from "@/i18n/locales/en" import { + CLIENT_ERROR_CODES, clientErrorFallback, clientErrorTranslationKey, readClientErrorCode, @@ -45,4 +47,15 @@ describe("client error wire contract", () => { expect(readClientErrorCode("provider_secret")).toBeNull() expect(readClientErrorCode({ error_code: "upload_failed" })).toBeNull() }) + + it("keeps the fallback strings identical to the English locale", () => { + for (const code of CLIENT_ERROR_CODES) { + const translationKey = clientErrorTranslationKey(code) + const localeKey = translationKey.replace( + /^clientErrors\./, + "", + ) as keyof typeof en.clientErrors + expect(clientErrorFallback(code)).toBe(en.clientErrors[localeKey]) + } + }) }) diff --git a/frontend/src/lib/client-errors.ts b/frontend/src/lib/client-errors.ts index 8604264552..f3d3c36c86 100644 --- a/frontend/src/lib/client-errors.ts +++ b/frontend/src/lib/client-errors.ts @@ -1,6 +1,6 @@ import type { TranslationKey } from "@/i18n/translations" -const CLIENT_ERROR_CODES = [ +export const CLIENT_ERROR_CODES = [ "message_processing_failed", "task_execution_failed", "guidance_in_progress", From ecf95545c42ed36d8f34341d50b26c371d8178b8 Mon Sep 17 00:00:00 2001 From: Alexliu Date: Fri, 4 Sep 2026 00:49:41 +0800 Subject: [PATCH 26/28] refactor(web): drop the unused fallback parameter 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. --- src/xagent/web/services/client_error_messages.py | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/xagent/web/services/client_error_messages.py b/src/xagent/web/services/client_error_messages.py index 969d5ee5e4..af2dca80a3 100644 --- a/src/xagent/web/services/client_error_messages.py +++ b/src/xagent/web/services/client_error_messages.py @@ -130,11 +130,7 @@ def required_mcp_unavailable_client_message( return fallback -def connector_runtime_client_message( - error: BaseException, - *, - fallback: str = CLIENT_SAFE_TASK_FAILURE, -) -> str: +def connector_runtime_client_message(error: BaseException) -> str: """Adapt the curated connector-runtime failure without a generic escape. The runtime check keeps this boundary fail-closed even if a future caller @@ -142,11 +138,11 @@ def connector_runtime_client_message( """ if not isinstance(error, ConnectorRuntimeError): - return fallback + return CLIENT_SAFE_TASK_FAILURE message = error.safe_message if isinstance(message, str) and message.strip(): return message - return fallback + return CLIENT_SAFE_TASK_FAILURE def connector_runtime_client_code(error: BaseException) -> str | None: From 9d01552391155fff99303d0b339c14d904ae3660 Mon Sep 17 00:00:00 2001 From: Alexliu Date: Fri, 4 Sep 2026 01:01:49 +0800 Subject: [PATCH 27/28] docs: describe protected invariants without review-round references 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. --- tests/web/api/test_terminal_task_error_event.py | 2 +- tests/web/services/test_task_orchestrator.py | 12 +++++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/tests/web/api/test_terminal_task_error_event.py b/tests/web/api/test_terminal_task_error_event.py index f7d06e634f..f525802b55 100644 --- a/tests/web/api/test_terminal_task_error_event.py +++ b/tests/web/api/test_terminal_task_error_event.py @@ -60,7 +60,7 @@ def test_terminal_error_event_carries_a_valid_code() -> None: def test_unknown_code_is_dropped_and_logged( caplog: pytest.LogCaptureFixture, ) -> None: - """``code`` passes the same closed set the /v1 surface pins against. + """``code`` is checked against ``CONNECTOR_RUNTIME_CLIENT_ERROR_CODES``. ``ConnectorRuntimeError`` types its code as a bare ``str`` and stores it without validation, so an unlisted value reaching the wire is a question diff --git a/tests/web/services/test_task_orchestrator.py b/tests/web/services/test_task_orchestrator.py index 851ab40ace..d05dee895d 100644 --- a/tests/web/services/test_task_orchestrator.py +++ b/tests/web/services/test_task_orchestrator.py @@ -3865,12 +3865,14 @@ def test_reconcile_finalized_delivery_noop_on_already_terminal_row( "scheduled_secret_unavailable", ] -# The reason a missing declared context key produces. It is deliberately not -# public: the key half is a name the connector's owner chose, and the frame's -# audience includes anonymous widget and share-link visitors. +# The reason a missing declared context key produces, used to populate the +# exception's own ``details`` below. The key half is a name the connector's +# owner chose; nothing under ``details`` reaches the terminal frame at all, +# so these fixtures exist to exercise the operator log, which still reads it. WITHHELD_KEY_REASON = "missing_context.auth_token" -# A listed reason, so the assertions below can speak about a reason that does -# reach the wire. +# An arbitrary reason value with no significance of its own -- it lives only +# in the exception's ``details``, which the frame never carries, so which +# string this is does not affect any assertion below. PUBLIC_REASON = "not_provided" From cdadb67c66d38cedd3e1eca589a0a7858320b376 Mon Sep 17 00:00:00 2001 From: Alexliu Date: Fri, 4 Sep 2026 01:42:54 +0800 Subject: [PATCH 28/28] test(web): name the closed-set producer guard for what it pins and drop a duplicate frame pin --- .../web/api/test_terminal_task_error_event.py | 19 +++++++++------ tests/web/services/test_task_orchestrator.py | 23 ------------------- 2 files changed, 12 insertions(+), 30 deletions(-) diff --git a/tests/web/api/test_terminal_task_error_event.py b/tests/web/api/test_terminal_task_error_event.py index f525802b55..bc483281ff 100644 --- a/tests/web/api/test_terminal_task_error_event.py +++ b/tests/web/api/test_terminal_task_error_event.py @@ -236,13 +236,18 @@ def _raise_site_error_names() -> set[str]: return names -def test_every_client_code_has_a_raise_site() -> None: - """Every code in the closed set is actually produced somewhere. - - A code that reaches the closed set ahead of its raise site is an - allowance with no expiry date: nothing enforces that a promise like this - stays true, so this test derives the raise sites fresh from the AST - instead of trusting a hand-maintained list of them. +def test_every_client_code_is_named_at_a_producer_site() -> None: + """Every code in the closed set is named where connector-runtime errors are made. + + A code that reaches the closed set ahead of its producer is an allowance + with no expiry date, so this test derives the producer names fresh from + the AST instead of trusting a hand-maintained list. What it pins is + narrower than "raised somewhere": a code counts as produced when its + constant is the first argument of a ``ConnectorRuntimeError`` or + ``_raise_runtime_error`` call, or is returned by one of the helpers that + pick the code before such a call. A helper that returns a code and is + never called would satisfy this test; reachability of the helper is not + checked here. """ error_names = _raise_site_error_names() diff --git a/tests/web/services/test_task_orchestrator.py b/tests/web/services/test_task_orchestrator.py index d05dee895d..cd62784ae0 100644 --- a/tests/web/services/test_task_orchestrator.py +++ b/tests/web/services/test_task_orchestrator.py @@ -4090,29 +4090,6 @@ async def test_connector_runtime_frame_never_carries_connector_ref( assert "connector_id" not in serialized -@pytest.mark.asyncio -async def test_connector_runtime_frame_reason_matches_direct_construction( - db_session, -) -> None: - """End to end, a listed reason on the exception still never reaches the wire.""" - - error = ConnectorRuntimeError( - "missing_runtime_context", - "Required connector runtime context is missing.", - details={"reason": PUBLIC_REASON}, - ) - - with _captured_terminal_broadcast(error, db_session) as ( - task_id, - frames, - settlements, - ): - task = db_session.query(Task).filter(Task.id == task_id).one() - await _run_failing_turn(task_id, int(task.user_id), task.source) - - assert "details" not in frames[0] - - @pytest.mark.asyncio @pytest.mark.parametrize("code", CONNECTOR_RUNTIME_CODES) async def test_connector_runtime_failure_logs_missing_key(