security(websocket): redact incidental exception text from chat clients

[codeacme17]

Part of #1497. Split out of #1472 on review feedback that it "belonged in its own PR" — it roughly doubled that PR's review surface while being unrelated to its retry work.

Problem

Chat clients, including anonymous widget and share visitors, receive raw exception text. Several handlers in websocket.py put str(e) straight into the message_rejected ack and the {"type": "error"} bubble, both of which the frontend renders verbatim (app-context-chat.tsx reads the string unchanged into an assistant error bubble).

DurableStorageOperationError is a RuntimeError subclass and its text carries scope segments, storage prefixes and tenant identifiers, so a storage fault mid-turn can put tenant-identifying internals in front of a visitor.

This PR does not close that specific path, and an earlier version of this description implied it did. Because DurableStorageOperationError is a RuntimeError, it falls under the passthrough allowlisted below. On the two main chat paths the text still reaches clients through broadcast_to_task, which delivers to every connection registered under the task_id — anonymous widget and share visitors included. What this PR closes is the ValueError/KeyError/TypeError surface and the PermissionError surface. The RuntimeError half is #1479. Round 3 review is what surfaced the overstatement; see the retraction below.

What changed

• client_safe_error_message() is the single place an exception may become client-visible text. Every direct call site that builds a rejection or error payload from an exception goes through it.
  • An adversarial self-review falsified the first version of this claim: handle_builder_chat reached a client through websocket.send_text(json.dumps({"type": "error", "message": str(e)})), a dict literal at a direct call site that the guard could not see because it matched only send_personal_message/broadcast_to_task. Routed, and the guard now recognizes send_text and unwraps the json.dumps around its payload.
• Messages genuinely written for the sender — "authentication required", "access denied", "Task not found", "Files are no longer bindable" — raise a ClientVisibleError subclass (ClientVisibleValidationError, ClientVisiblePermissionError) and keep their wording. Anything without the marker is redacted, so a new except branch that forgets it fails closed.
• An AST guard walks the module and fails if a recognized producer can reach a client with raw text. Verified non-vacuous by reintroducing a leak at a delivery producer, at an error bubble, and at the send_text payload above; each is caught. The RuntimeError carve-out is anchored to (function, expression) rather than to unparsed source text, so the blessed string is rejected in any function outside the allowlist — verified by reintroduction. Correction: an earlier version of this line also claimed the anchor caught the string moving into a validation branch of an allowlisted function. It does not — _local_assignments unions every assignment in the enclosing function regardless of branch. Reproduced and tracked in test(websocket): the client-safe AST guard is fail-open in four ways #1547. The sweep floors are pinned per category; the previous single floor of 20 against an actual 45 would have let half the surface disappear silently. Correction: the in-code comment said "23 / 23" — the payload figure was measured before send_text joined the sinks and is actually 28, so the floors are looser than that comment claimed. Tightening them is test(websocket): the client-safe AST guard is fail-open in four ways #1547.

What this does not cover

Stated plainly in the guard's own docstring, because an earlier version of it overclaimed and that is what let leaks survive three review rounds:

• dict-spread payloads ({**terminal_payload, "error": message})
• payloads built by a helper and passed as a call (create_stream_event(...))
• wrapper functions forwarding a raw argument into a producer (notify_deferred_delivery)
• the durable-command error channel — withdrawn in round 4, this was wrong. TaskCommandRejected is re-raised without broadcasting (websocket.py:8362); the text terminates in the TaskExecutionCommand.error column and nothing under src/xagent/web/ reads it back to a client. The mechanism that did broadcast — _broadcast_terminal_command_error, reached from the deferral- and failure-exhausted branches — was never named here, and is closed by this PR (see below). security(websocket): raw exception text still reaches chat clients through unrecognized producer shapes #1497 has been corrected.

Those three still leak, and each now has a strict xfail test that constructs the bypass and asserts the guard stays blind to it — so when one is fixed its test flips to a failure instead of the gap outliving the issue.

Closed in round 4, having been mis-scoped as out of scope: _broadcast_terminal_command_error built a {"type": "agent_error", ...} dict literal directly from an exception at a direct call site. It matched none of the three caveats above and escaped purely on its event type, which the caveat list never mentioned. Its message now goes through client_safe_error_message, the command kind moved to a structured command_kind field, and agent_error joined the guard's recognized payload types. #1497 tracks either closing them or inverting the design so payload construction itself is the only typed path to a client — which would make the guard unnecessary rather than smarter.

The agent RuntimeError passthrough is deliberately allowlisted, and narrowing it is a product decision tracked in #1479.

Accuracy note. Four rounds of review have found five claims in this description that did not hold — the guard's branch-sensitivity, the sweep counts, the durable-command mechanism, the completeness of a retraction, and "the outermost handler owns the traceback". Each had the same cause: one instance verified, the general property reported. Every factual claim now in this description has a command behind it whose output was read.

Retraction. This paragraph previously said the passthrough was safe because "surfacing it to the sender is an existing tested contract (test_websocket_owner_actor.py)". That citation was wrong. Correction (round 4): this retraction was itself inaccurate. It said the citation had been removed from the code comment; it had only been removed from the test file, and websocket.py:330-332 still carried it verbatim — so the module shipped two contradictory descriptions of the same carve-out. The source comment is now fixed too. test_websocket_owner_actor.py never pins that string; its one raw-RuntimeError assertion covers the sender-only fallback reached on a pool timeout, never the broadcast that actually fires once a task resolves. The justification was narrower than the code path it excused, on exactly the mechanism this PR is about. No behaviour changed in this round — the claim did.

Testing

test_websocket_client_safe_errors.py: a secret embedded in a ValueError/KeyError/TypeError never reaches any client payload; a curated message keeps its wording; a missing task keeps its own wording through the pause/resume enqueue path; a redacted enqueue refusal still reaches the log with a traceback in all three handlers; the AST guard over the producers. Verified against test_websocket_owner_actor.py and test_websocket_error_payload.py, which pin the contracts this must not break. ruff and mypy clean.

Out of scope

• security(websocket): raw exception text still reaches chat clients through unrecognized producer shapes #1497 — the producer shapes listed above, and the frontend's getUploadErrorMessage raw-body fallback.
• test(websocket): the client-safe AST guard is fail-open in four ways #1547 — four fail-open shapes in the AST guard's own resolution logic (branch-insensitive allowlist, uncollected AnnAssign/AugAssign, unchecked .get() receiver, slack sweep floors and producer aliasing). None is a live leak; all are future-code holes in the test.
• security(websocket): agent RuntimeError text is surfaced verbatim to public chat visitors #1479 — the RuntimeError passthrough and the safe_for_display / error-code contract that would also fix localization.

Related

#1472, #1468

[gemini-code-assist]

Code Review

This pull request introduces a mechanism to redact sensitive exception details before they reach chat clients, defining client-safe exception classes and a helper function to filter outgoing error messages. It also adds a static analysis test to ensure no raw exception messages bypass this filter. The review feedback highlights several opportunities to improve error logging (specifically adding exc_info=True and lazy formatting) so that redacted errors are not swallowed without a trace in the server logs. Additionally, it suggests raising a client-visible validation error when a task is not found, and improving the robustness of the test file reader to handle .pyc files and explicit UTF-8 encoding.

[codeacme17]

Review round 1 addressed in 664c20f — all eight inline threads have individual replies. Summary:

Fixed (7)

Finding	Change
Task N not found redacted in the pause/resume enqueue path	raised as ClientVisibleValidationError, matching execute_task_background and the Access denied branch beside it
handle_chat_message / handle_pause_task / handle_resume_task logged nothing	each logs the exception with exc_info=True before answering the client
handle_intervention and the two unserialized pause/resume handlers lacked a traceback	lazy %s formatting plus exc_info=True, in line with the rest of the module
AST guard read the module source with the platform default encoding	explicit encoding="utf-8"

The three zero-logging handlers were the important ones: before this PR the leak to the client was the only record of those failures, so redacting the text deleted the evidence and made the module comment’s "the detail stays in the log" untrue at exactly those sites. That is a regression this PR introduced, and it is now closed.

Declined (1)

The .pyc half of the read_text suggestion. A source-imported module’s __file__ is always the .py path; it is a .pyc only under SourcelessFileLoader, where no .py exists to fall back to — so the branch is unreachable where it would help and turns a clear error into FileNotFoundError where it would fire. Reasoning in the thread.

Raised by me, not by the review

In the two unserialized pause/resume handlers, message_data["_durable_command_error"] = str(e) sits two lines above the send_personal_message this PR redacts. That dict is dict(command.payload), read back by the durable executor, re-raised as TaskCommandRejected(durable_error), and broadcast by _broadcast_terminal_command_error as f"Task command {kind} failed: {error}" to every client on the task. The redaction at those two sites is therefore cosmetic until that path is closed. It is one of the wrapper/helper shapes this PR already declares out of scope; I have added this specific path to the "What this does not cover" list in the description so it is not rediscovered next round. Tracked in #1497.

Verification

tests/web/api/test_websocket_client_safe_errors.py gains two regression tests — test_missing_task_keeps_its_wording_for_the_sender and a parametrized test_redacted_enqueue_failure_still_reaches_the_log covering all three handlers (secret absent from every client payload, present in a log record carrying exc_info). Full tests/web suite passes apart from four pre-existing SVG-rasterization failures in test_file_upload.py / test_svg_preview_cache.py, which I confirmed fail identically on the parent commit. ruff format, ruff check and mypy clean.

[codeacme17]

Round 2 — adversarial self-review, pushed as 5d0e65d.

Per the repo's pre-PR discipline I ran a two-axis review (standards + spec) over the full diff with independent high-reasoning agents, and verified every finding against the code before acting. It falsified one of this PR's own claims, so the description has been corrected too.

The claim that was wrong

client_safe_error_message() is the single place an exception may become client-visible text. Every direct call site … goes through it.

handle_builder_chat did not:

await websocket.send_text(json.dumps({"type": "error", "message": str(e)}))

A {"type": "error"} dict literal at a direct call site — precisely the shape claimed to be covered — missed because it exits through websocket.send_text rather than manager.send_personal_message, and the guard matched only the latter two. Authenticated-owner endpoint, so outside the anonymous-visitor threat model, but the claim was still false and the guard was blind to it. Now routed; the guard recognizes send_text and unwraps the json.dumps around its payload.

Widening it immediately caught a second site echoing the client's own message_type back. Not exception text, so out of this PR's threat model — but its sibling in the main loop already used the non-echoing wording, so the two now match rather than the reflection being allowlisted.

Guard hardening

The RuntimeError carve-out keyed on unparsed source text, so it blessed that string in all five functions containing it and would have blessed a sixth pasted into a validation branch. Anchored to (function, expression). The sweep floor asserted >= 20 against an actual 45 — half the surface could vanish without tripping it; now counted per category and pinned near actual. Both changes verified by reintroduction: a raw send_text payload and the carve-out string in a non-allowlisted function are each caught.

Logging, corrected twice over

Round 1 gave exc_info=True to the ValueError branches while their RuntimeError and bare-Exception siblings in the same try blocks stayed bare — the less severe branch got the traceback and the more severe one did not. That was my regression. Resolved with one rule: a traceback where the error is absorbed, a lazy breadcrumb where it is re-raised and an outer handler owns the stack. That also ends the three stack dumps a single unexpected error produced through the re-raise chain.

Level now follows the marker rather than the call site — a ClientVisibleError logs at WARNING without a traceback, anything else keeps ERROR with one. Without that split, an anonymous visitor could make the server dump a stack on demand by looping unauthenticated frames, which is a poor trade in a PR about what visitors can extract. This is slightly beyond what round 1 asked for; flagging it as a deliberate call rather than silent scope growth.

Flagged, deliberately not fixed — renames (CLIENT_SAFE_VALIDATION_ERROR holds a message, not an error, and is returned for bare except Exception too), extracting the five near-identical reject-and-log hunks into one helper, moving the exception hierarchy to src/xagent/web/jobs/exceptions.py, and ClientVisiblePermissionError inheriting OSError via PermissionError (two-arg construction would render [Errno a] b; no live hazard, the only two except OSError are symlink handling in a different function). All are churn a reviewer did not ask for. Say the word if you want any of them in.

Also corrected outside this PR — #1497 and #1479 both credited the chokepoint, marker hierarchy and guard to #1472, which is still open and does not contain them; that work is here. Both issues now carry a baseline correction so their residual scope reads against this PR.

Verification — full tests/web green apart from five pre-existing SVG-rasterization failures I confirmed fail identically on the parent commit. ruff, mypy clean. New test pins the WARNING/ERROR split.

[rogercloud]

PR Summary

Several websocket handlers (handle_chat_message, handle_pause_task, handle_resume_task, handle_execute_task, _handle_chat_message_unserialized, handle_builder_chat, and others) were forwarding raw str(e) exception text to chat clients — including anonymous widget/share visitors — in message_rejected acks and {"type": "error"} payloads. Because DurableStorageOperationError embeds scope/storage/tenant identifiers in its message, a mid-turn storage fault could leak tenant-identifying internals to an unauthenticated visitor. This PR (tracking issue #1497) introduces a ClientVisibleError marker-exception hierarchy, a single client_safe_error_message() gate that fails closed for unmarked exceptions, log_client_facing_failure() to preserve server-side WARNING/ERROR logging despite the redaction, and a new AST-based static test that walks websocket.py to catch producers that can bypass the gate. Several shapes (dict-spread payloads, helper-built payloads, wrapper-forwarded arguments, and the durable-command error channel in pause/resume) are explicitly declared out of scope and tracked in #1497.

Blocking: yes — recommended event: REQUEST_CHANGES

Round 0 — Approach Verdict: acceptable-with-reservations

The marker-class + single-gate design is a reasonable transitional approach: double-inheriting ClientVisibleValidationError/ClientVisiblePermissionError from both the marker and the original builtin exception type means zero control-flow changes to existing except clauses while adding an orthogonal "is this client-visible" dimension. The log_client_facing_failure WARNING/ERROR split is a good side-benefit that prevents the redaction from erasing the server-side record. Using an AST-based guard to statically enforce the invariant is consistent with existing convention in this repo (there are 25+ other AST-based architecture-guard tests), so this is not a one-off fragile mechanism.

Non-blocking design observations (all already surfaced and consciously deferred by the author in the self-review discussion, not new):

• The guard's PRODUCERS/type-discriminator set reads as tuned to pass current tests rather than derived from an invariant — see F1/F4/F5 below for the concrete consequences of this.
• The same class of leak (detail=str(e)) exists across ~152 REST call sites outside websocket.py (e.g. agents.py, chat.py). If the long-term direction under #1497 is a shared typed-payload mechanism, this hierarchy/gate/guard should eventually move out of the 9000-line websocket.py into a shared module. The author already proposed and declined this exact move ("churn a reviewer did not ask for. Say the word if you want any of them in") — treat as an open, non-blocking suggestion, not a new finding.
• CLIENT_SAFE_VALIDATION_ERROR is misnamed (used for more than validation errors), and ClientVisiblePermissionError inheriting OSError via PermissionError is a slightly awkward hierarchy (verified: no live hazard, the file's only two except OSError blocks are unrelated symlink-handling code). Both already explicitly flagged and declined by the author as minor.

Findings

F1 — MAJOR — AST guard's RuntimeError carve-out is not branch-sensitive, contradicting the PR's own stated fix

File: tests/web/api/test_websocket_client_safe_errors.py, _local_assignments helper (L236-245) and the (function, expression)-keyed allowlist, exercised against src/xagent/web/api/websocket.py's handle_execute_task (branches at L6698 and L6725)

_local_assignments walks the entire enclosing function body and unions every assignment to a given variable name — it is not scoped to a specific branch, except clause, or line range. The self-review claims the new (function, expression) anchoring "would have blessed a sixth [carve-out string] pasted into a validation branch" and closes that gap. This was reproduced directly: copying the guard's logic and mutating a copy of handle_execute_task so the validation branch's assignment is replaced with the exact blessed RuntimeError string, in the same already-allowlisted function, produces zero offenders both before and after the mutation. The (function, expression) anchor only stops the blessed string being reused in a different, non-allowlisted function — it does nothing to prevent it moving into a different branch of the same allowlisted function. This directly contradicts the specific claim made about this fix.

Suggested fix: scope _local_assignments (and the allowlist key) to the enclosing except/branch node, not the whole function.

F2 — MAJOR — _local_assignments misses AnnAssign/AugAssign/walrus, causing a fail-open in the mixed-assignment case

File: tests/web/api/test_websocket_client_safe_errors.py, _local_assignments (L236-245)

Only ast.Assign nodes are walked. When a function has both a normal Assign to a variable (allowlisted/safe) and a sibling ast.AnnAssign (message: str = f"secret: {e}") assigning raw exception text to the same variable name, _local_assignments returns only the safe candidate — the leaking AnnAssign value is never inspected, and the guard reports zero offenders. (An AnnAssign-only case with no sibling plain Assign is instead flagged as an unresolvable name, so this is scoped to the mixed-form case.)

Suggested fix: also walk ast.AnnAssign.value, ast.AugAssign.value, and ast.NamedExpr.value when collecting candidate values.

F3 — MAJOR — _is_client_safe's .get() whitelist doesn't check the receiver object

File: tests/web/api/test_websocket_client_safe_errors.py, _is_client_safe .get() branch (L253-258)

The branch only checks that the method name is "get" and that trailing args are constants/attributes — it never checks what object .get() is called on. The one current call site (_TURN_REJECTION_MESSAGES.get(reason, default), src/xagent/web/api/websocket.py:6422) is a safe literal table, so there's no live exploit today. But untrusted_dict.get("error") or payload.get("message") — including a bare single-argument .get() with no default, since all([]) on an empty trailing-arg list evaluates True — would be silently accepted as client-safe. This is exactly the future-code shape the guard exists to catch.

Suggested fix: additionally require the receiver to match a known-safe curated-table allowlist (e.g. _TURN_REJECTION_MESSAGES), not just the method name.

F4 — MAJOR — Sweep-floor assertions are looser than claimed, and producer aliasing bypasses matching entirely

File: tests/web/api/test_websocket_client_safe_errors.py, floor assertions (L328-333)

The comment above the assertions (checked_producers >= 21, checked_error_payloads >= 21) claims they're "pinned near the actual counts (23 / 23 at the time of writing)." Running the guard's own counting logic against current websocket.py gives 23 producers and 28 error payloads — the comment's own "23/23" is already stale for payloads (28, not 23), and the >=21 floor leaves 7 of 28 payload sites (25%) free to silently disappear via refactor/rename without tripping any assertion. Separately, _called_name matches only the literal call-expression name/attribute with no alias resolution: aliasing a producer (_p = send_message_delivery; await _p(...)) drops that producer's recognized-use count to zero with no failure, given the existing floor slack.

Suggested fix: tighten the floors to the exact current counts (or an exact-count assertion with a comment requiring deliberate bump), and either resolve simple local aliases before matching or add a companion check against alias reassignment of guarded names.

F5 — MAJOR — RuntimeError carve-out broadcasts to all task subscribers, not just "the sender" as claimed

File: src/xagent/web/api/websocket.py, handle_execute_task's RuntimeError branch (L6725-6754) and _handle_chat_message_unserialized's equivalent (L6482-6509)

Once authorized_task_id is not None (the normal case for any resolved task), the raw f"Runtime error: {str(e)}" is sent via broadcast_to_task, which delivers to every connection registered under that task_id via ConnectionManager.connections_for_task — confirmed to include anonymous widget/share connections, since public_chat_access.py's public_chat_websocket_endpoint registers into the same manager/dict keyed only by task_id with no per-connection audience filtering. The code comment justifies this passthrough as "surfacing it to the sender is an existing, tested contract (tests/web/api/test_websocket_owner_actor.py)," but that test file only exercises the sender-only fallback path (pre-task-lookup failures) — never the broadcast-to-all-subscribers case that actually fires here. The security justification is narrower than the code path it excuses, on the exact mechanism meant to limit exposure to anonymous visitors.

Suggested fix: either route this carve-out through send_personal_message to the raising connection only, or run it through client_safe_error_message/mark it ClientVisibleError if the text is genuinely meant to be broadcast-safe.

F6 — MAJOR — handle_builder_chat's redaction fix has zero runtime test coverage

File: tests/web/api/test_websocket_client_safe_errors.py, _client_payloads helper (L24-32); fix site src/xagent/web/api/websocket.py (L8818-8820)

_client_payloads only inspects send_personal_message/broadcast_to_task mock calls — never websocket.send_text, which is exactly the sink handle_builder_chat uses. No test in the file calls handle_builder_chat or asserts on a send_text payload; the only coverage of this leak-and-fix (the PR's own headline catch from its "adversarial self-review") is the static AST sweep, not a runtime behavioral test.

Suggested fix: add a runtime test that drives handle_builder_chat through a raised exception and asserts the websocket.send_text payload is redacted.

(Minor, non-blocking, same finding area: handle_builder_chat currently has no ClientVisibleError opt-in raise sites, so an authenticated builder-chat user always gets the fully generic message even where more specific wording would be safe and useful — consistent with the file's "fail closed unless marked" convention elsewhere, so this is an optional follow-up, not a defect introduced by this PR.)

F7 — MAJOR — No test asserts the permission-wording contract through pause/resume

File: src/xagent/web/api/websocket.py, handle_pause_task (L7484), handle_resume_task (L7747), raise site (L5109); tests tests/web/api/test_websocket_owner_actor.py (test_pause_non_owner_non_admin_is_refused, test_resume_non_owner_non_admin_is_refused)

Both handlers reach ClientVisiblePermissionError(f"Access denied: Task {task_id} does not belong to you") via the same except (PermissionError, ValueError) pattern as handle_chat_message. test_websocket_error_payload.py asserts this exact wording only for handle_chat_message. The two owner-actor tests do drive a non-owner through pause/resume and hit this raise site, but only assert on side-effect non-invocation — never on the resulting client message text. A regression that silently redacted this wording to the generic fallback (or leaked unrelated raw text through this path) would go undetected today for two of the three handlers sharing this contract.

Suggested fix: extend test_websocket_error_payload.py's wording assertion to cover handle_pause_task/handle_resume_task, or add the assertion directly to the existing owner-actor tests.

Minor / Advisory Notes (non-blocking)

• Shared dict key, different trust models: _durable_command_error in _handle_chat_message_unserialized's generic except Exception branches is now correctly redacted via client_safe_error_message(e) before being passed to finish_delivery_failure (verified: this value is also sent directly to the connected client, so this was a real fix). In _handle_pause_task_unserialized/_handle_resume_task_unserialized, message_data["_durable_command_error"] = str(e) stays raw — this is the already-disclosed, already-tracked (#1497) gap, not a new issue, but the two branches use the same dict key for fields with different trust semantics, which could confuse future maintainers. Worth a short comment at the raw-assignment sites noting the distinction.
• CLIENT_SAFE_VALIDATION_ERROR's name and ClientVisiblePermissionError's OSError ancestry (via PermissionError) are both already flagged and consciously deferred by the author — optional naming/hierarchy cleanup, not blocking.
• Moving the ClientVisibleError hierarchy / client_safe_error_message / AST guard into a shared module (given the parallel REST-layer leak surface) is an optional forward-looking suggestion the author has already considered and declined for this PR — not a defect.

Simplification Opportunities

L5041: shrink duplicate client_safe_error_message(exc) call in handle_chat_message's except (PermissionError, ValueError) block (also at L5045). Compute message = client_safe_error_message(exc) once and reuse at both the send_message_delivery and send_personal_message call sites.

net: -1 line possible

Prior Findings (re-review) Status

All prior Gemini-bot findings are FIXED, verified against current HEAD (5d0e65d0a):

• P1 (task-not-found wording over-redacted in pause/resume): FIXED — now raises ClientVisibleValidationError; regression test asserts the specific wording.
• P2 (three handlers logging nothing before redacting): FIXED — all three now log via log_client_facing_failure before responding. The later WARNING/ERROR-by-marker-type split is a deliberate, correct refinement, not a regression. Verified by running the test suite: 11/11 passed in tests/web/api/test_websocket_client_safe_errors.py.
• P3 (three handlers logging without exc_info/lazy formatting): FIXED — all three now use lazy %s formatting with exc_info=True.
• P4 (AST guard's test-file reader should specify utf-8 encoding): FIXED (confirmed present at test file line 277). The declined .pyc-fallback half was independently verified as a technically correct decision — __file__ for a normally source-imported module is always the .py path since Python 3.4+, and is only a .pyc path under SourcelessFileLoader, where no .py exists to fall back to.

Blocking Status & Recommended Decision

Blocking: yes
Recommended event: REQUEST_CHANGES

Blocking issues:

• tests/web/api/test_websocket_client_safe_errors.py:236-245 — MAJOR — RuntimeError carve-out allowlist is not branch-sensitive, contradicting the PR's own stated fix (F1) — [new]
• tests/web/api/test_websocket_client_safe_errors.py:236-245 — MAJOR — misses AnnAssign/AugAssign/walrus, fail-open in mixed-assignment case (F2) — [new]
• tests/web/api/test_websocket_client_safe_errors.py:253-258 — MAJOR — .get() whitelist doesn't check receiver object (F3) — [new]
• tests/web/api/test_websocket_client_safe_errors.py:326-333 — MAJOR — sweep floors looser than claimed; producer aliasing bypasses matching (F4) — [new]
• src/xagent/web/api/websocket.py:6725 / :6482 — MAJOR — carve-out broadcasts to all task subscribers, not just the sender as claimed (F5) — [new]
• tests/web/api/test_websocket_client_safe_errors.py:24-32 — MAJOR — handle_builder_chat's fix has no runtime test coverage (F6) — [new]
• src/xagent/web/api/websocket.py:5109, :7484, :7747 — MAJOR — no test asserts wording preservation through pause/resume (F7) — [new]

[codeacme17]

Round 3 addressed in ccc6434. All eight inline threads have replies. Every finding was reproduced before I acted on it; two of them falsified claims I had made, and those are retracted rather than quietly patched.

The one that matters most (F5)

You found that the carve-out's justification is narrower than the path it excuses. It is worse than that: the citation does not exist. grep -c "Runtime error" tests/web/api/test_websocket_owner_actor.py returns 0. Its only raw-RuntimeError assertion is "inject failed" at :2260, reached through the pool-timeout early return — sender-only, never the broadcast.

And DurableStorageOperationError subclasses RuntimeError. That is the example this PR opens with as its motivation. So the tenant-scope leak the PR is about is still open on the two main chat paths, and the description was presenting it as closed. Runtime probe:

{"type": "agent_error", "message": "Runtime error: durable object scope=tenant-42/prod prefix=s3://xagent-prod/tenants/42", ...}

The description now says this plainly, the false justification is out of the code comment, and #1479 carries a correction because it repeats the same wrong citation. The code path stays with #1479 — routing it to send_personal_message changes an existing client contract that is that issue's subject. If you want it here instead, say so and I will take your first option (sender-only).

Fixed here

• F6 — _sent_text_payloads decodes websocket.send_text JSON, and a new test drives handle_builder_chat through a raised exception carrying a secret. Verified to fail when the :8818 redaction is reverted.
• F7 — test_permission_wording_survives_redaction_in_every_handler, parametrized over pause and resume, asserts the exact Access denied wording. Verified to fail when the raise site is downgraded to a bare PermissionError.
• shrink — one client_safe_error_message(exc) call, two sinks.

Corrected here, hardening deferred to #1547

• F1 — reproduced independently: the blessed string in handle_execute_task's validation branch gives zero offenders. My claim that the (function, expression) anchor closed this was wrong and is gone from the comment, the commit message and the description.
• F4 — re-measured: 23 producers, 28 error payloads. My "23 / 23" comment was measured before send_text joined the sinks. Alias bypass also reproduced (23→22 producers, still zero offenders).
• F2 / F3 — both confirmed. One correction: NamedExpr does not need handling — walrus is not a fail-open in either form (inline is caught as unsafe, walrus-then-Name as unresolvable), so including it would be dead code. AugAssign does fail open, which was not in your repro. test(websocket): the client-safe AST guard is fail-open in four ways #1547 records both.

#1547 exists because none of F1–F4 is a live leak — all four are future-code fail-opens in the test — and F1+F2 share one _local_assignments refactor that is better done once than half-done under review pressure. Nothing in the tree now asserts a property the guard lacks. Happy to pull any of them forward if you would rather they land here.

Verification

Full tests/web green apart from five pre-existing SVG-rasterization failures, confirmed failing identically on the parent commit. ruff, mypy clean. Both new tests mutation-checked against their own fixes.

[rogercloud]

Summary

This PR closes an incidental exception-text leak to chat clients: ValueError / KeyError / TypeError / PermissionError text that previously reached anonymous widget and share visitors through error bubbles and message_rejected acks is now funnelled through a single chokepoint, client_safe_error_message, which returns a fixed string unless the exception carries the ClientVisibleError marker. The design is fail-closed (forget the marker and you get redaction, not a leak), and a new AST-walking guard in tests/web/api/test_websocket_client_safe_errors.py sweeps the module for producer/payload sites that bypass the chokepoint. Split out of #1472, it deliberately leaves several bypass shapes open — dict-spread and helper-built payloads, wrapper forwarding, the durable-command channel, the agent RuntimeError passthrough — tracked in #1497 / #1547 / #1479.

Design assessment

Acceptable with reservations, leaning toward the lower end of that band. The chokepoint + marker-subclass + AST-guard combination is architecturally sound and has the right polarity: redact by default, allowlist the exceptions that were actually written for a human. Splitting this out of #1472 was the right call.

Two reservations:

1. The enforcement mechanism is a hand-maintained, string-and-shape-matching AST walker over an ~8000-line module. That is inherently the kind of guard that keeps missing shapes, and it has already been demonstrated twice: it missed send_text and needed a mid-review fix, and this round found it also misses the agent_error event-type variant (see MAJOR 3). That is a known, accepted tradeoff tracked in #1547 and not this PR's job to fully solve — but "the AST guard proves coverage" should be read as a weaker claim than it currently reads in the docstrings.
2. The PR body records three self-retracted coverage claims. This round verified that one of those retractions was never actually applied to the code (MAJOR 2). The architecture verdict does not change, but the disclosure process has a reliability gap worth naming: the written description of what this diff covers cannot currently be trusted without independent checking, and two of the MAJOR findings below are exactly that class of defect.

MAJOR

1. src/xagent/web/api/websocket.py:6666 — bare Exception leaves the client with no message at all

if request is None:
    raise Exception(f"Task {task_id} not found or access denied")

This same diff converted the sibling raise in this very function (websocket.py:6641-6643, user-authentication check) to ClientVisibleValidationError, but this one was left as a bare Exception. Note: this specific line is outside the diff's changed hunks (it sits between two touched regions), so it is pre-existing code — but it is directly impacted by the principle this diff establishes a few lines above and below it. Traced effect: it propagates through two generic except Exception handlers that only log-and-reraise (websocket.py:6752, websocket.py:7434) and terminates the connection via finally: manager.disconnect(websocket). The client therefore receives nothing — not the raw text, and not the redacted generic string either. That is a worse outcome than redaction and it contradicts the PR's own stated principle that a sender-facing, actionable message keeps its wording by carrying the marker.

Suggested fix:

raise ClientVisibleValidationError(f"Task {task_id} not found or access denied")

2. src/xagent/web/api/websocket.py:330-332 — the stale RuntimeError comment was never removed, and now contradicts a comment this PR itself added

Still present verbatim at HEAD:

# Agent RuntimeError text is deliberately left alone: surfacing it to the sender
# is an existing, tested contract (tests/web/api/test_websocket_owner_actor.py),
# and narrowing it is a product decision rather than a redaction bug.

The PR body's "Retraction" section states this citation "was wrong and has been removed from the code comment too." It has not been removed. And tests/web/api/test_websocket_owner_actor.py does not pin the claimed contract: its single raw-RuntimeError assertion covers only the sender-only pool-timeout fallback, never the broadcast path.

The situation is worse than a stale comment, because tests/web/api/test_websocket_client_safe_errors.py:157-168 — added by this PR — carries a new comment that correctly says the opposite: "Do NOT read this as 'sender only'... this text goes out via broadcast_to_task to every connection on the task." The module now ships two directly contradictory descriptions of the same carve-out.

Please delete the test_websocket_owner_actor.py citation and the "surfacing it to the sender" framing from websocket.py:330-332, replace it with the accurate broadcast description (pointing at #1479), and correct the PR body's retraction claim.

3. src/xagent/web/api/websocket.py:355-362 — the chokepoint docstring's coverage claim is false as written

Every ``message_rejected`` and ``error`` payload that this module builds
from an exception *at a direct call site* goes through here.

with caveats listed only for helper-built, dict-spread, and wrapper-forwarded payloads. _broadcast_terminal_command_error (websocket.py:8310-8321, pre-existing, untouched by this diff) is a counterexample that matches none of the three caveats — it builds a dict literal directly from an exception at a direct call site and hands it to broadcast_to_task:

{
    "type": "agent_error",
    "message": (f"Task command {command.kind.value} failed: {error}"),
    ...
}

It escapes the guard and the docstring's claim purely because its event type is "agent_error" rather than the literal "error" that both the docstring and the AST guard anchor on — a dimension the docstring does not disclose at all. Either add the event-type limitation to the caveat list, or (cheaper and better) add "agent_error" to the guard's recognized payload types and route this payload through client_safe_error_message.

4. Disclosure accuracy: the PR body documents a durable-command mechanism that does not fire, and never names the one that does

Not a new code bug — a paper-trail correction, but a load-bearing one for #1497's future work.

The PR body's "What this does NOT cover" section describes the durable-command gap as: _handle_pause_task_unserialized / _handle_resume_task_unserialized set message_data["_durable_command_error"] = str(e), which is re-raised as TaskCommandRejected(durable_error) and then broadcast by _broadcast_terminal_command_error to every client on the task.

Verified: that path does not broadcast. TaskCommandRejected is caught and re-raised without calling _broadcast_terminal_command_error (websocket.py:8335-8338, comment: "Rejections come from handlers that already expose their durable domain-level outcome"). The text is only persisted to the TaskExecutionCommand.error column via fail_task_command in task_command_transport.py, with no client-facing read path anywhere under src/xagent/web/.

Meanwhile _broadcast_terminal_command_error genuinely is reachable — via two branches the PR body never mentions: TaskCommandDeferred exhausted and generic Exception exhausted (websocket.py:8331-8341), both broadcasting raw str(exc) through the agent_error dict literal from MAJOR 3.

Net: the documented mechanism is inert and the live mechanism is undisclosed. Because the existence of a gap here is pre-existing and already tracked under #1497, this need not block on scope grounds — but merging with an inaccurate description means #1497 will be worked against the wrong mechanism. Please either correct the PR body and #1497's description, or just close it now by routing the agent_error payload through the chokepoint (a one-line producer-set addition on the guard side).

5. src/xagent/web/api/websocket.py:6512, :6531, :6752 — "the outermost handler owns the traceback" is not true

Three new comments justify omitting exc_info=True:

# Re-raised: the outermost handler owns the traceback.
logger.error("Unexpected error in agent execution: %s", e)

The actual outermost handler in this chain is websocket.py:7434-7437:

except Exception as e:
    logger.error(f"Unexpected error in WebSocket: {e}")
    raise

No exc_info=True there either, and none anywhere else in the traced chain — so no handler ever logs a traceback for these paths. Since this PR's whole trade is "the client gets a generic string, the operator gets the detail in the log," losing the traceback on the redacted paths is exactly the wrong side to skimp on. Either add exc_info=True at the inner sites, or add it at websocket.py:7434 and keep the comment (then it becomes true). As written the comment asserts a property the code does not have.

6. src/xagent/web/api/websocket.py:7442-7461 — handle_intervention echoes raw client input to every connection on the task

await manager.broadcast_to_task(
    {
        "type": "intervention_processed",
        "message": f"Manual intervention processed: {intervention_data['action']}",
        ...

action is message_data.get("action") (websocket.py:7449) — unvalidated, unsanitized client-controlled text — broadcast to every connection on the task. This line is outside the diff's changed hunks (pre-existing), but this same diff edits the except blocks of this exact function a few lines below (adding exc_info=True, client_safe_error_message(e)), and removes an analogous echo pattern elsewhere in the same diff (websocket.py:~8900, with the new comment "Not echoed back: matches the main loop..."). This is precisely the cross-visitor injection threat model the PR closes elsewhere in this same function, missed here.

Suggest either dropping the interpolation ("message": "Manual intervention processed"), keeping the action in a structured field only, or validating action against the known enum before echoing.

7. Zero regression guard for any of the four confessed bypass shapes

tests/web/api/test_websocket_client_safe_errors.py has no test — not even an xfail — for any of the four shapes the PR itself admits are uncovered: dict-spread payloads, helper-built payloads, wrapper forwarding, and the durable-command channel. They exist only as prose in the AST guard's docstring (~lines 296-306).

For a change whose thesis is "fail closed, and prove it," a known-open hole with no pinned assertion means nothing will notice if the guard's blindness widens. Please add at least one pytest.mark.xfail(strict=True) test per shape (or a single parametrized one) that constructs the bypass and asserts raw text reaches the client — so that the day someone fixes the guard, the xfail flips and the tracking issue closes itself instead of drifting.

MINOR

• The AST guard requires a literal "error" string constant for the type key, so kind = "error"; ...({"type": kind, ...}) evades it. Advisory — foreseeable from the guard's own stated scope.
• The AST guard docstring (tests/web/api/test_websocket_client_safe_errors.py:~296-306) does not disclose that non-"error"-literal event types (e.g. agent_error) are out of scope. Ties into MAJOR 3.
• Inconsistent exc_info within the same hunks: the validation-error branch gets exc_info=True while the sibling RuntimeError branch a few lines below does not (websocket.py:7736, websocket.py:8131), with no stated reason.
• One leftover f-string log call at websocket.py:6527 (logger.error(f"Connection error handling chat message: {e}"), pre-existing, untouched by this diff) was not converted to lazy %s formatting like its siblings in this same function.
• client_safe_error_message returns "" for a ClientVisibleError subclass constructed with an empty or whitespace message, and send_message_delivery's if message: check then drops the key entirely rather than falling back to CLIENT_SAFE_VALIDATION_ERROR. Latent — no live call site passes an empty message today.
• The base ClientVisibleError is directly instantiable but inherits only Exception, not the stdlib types its subclasses mix in, so a bare raise ClientVisibleError(...) would slip past except (ValueError, KeyError, TypeError) handlers. Latent — nothing raises the bare class today. Consider making it non-instantiable or documenting "subclass only" in the marker docstring.
• tests/web/api/test_websocket_client_safe_errors.py:~103-108 monkeypatches TaskTurnOrchestrator.schedule_existing_task_execution with raising=False, giving up a free typo/rename guard for a method that exists today.
• test_builder_chat_redacts_through_its_own_socket_sink (~495-520) asserts the client never sees the secret but, unlike its sibling test_redacted_enqueue_failure_still_reaches_the_log, never asserts the operator-facing log still records it — half the redaction contract goes unpinned.
• handle_intervention's error-redaction branches have no behavioral test anywhere in the suite; they are covered only by the AST guard sweep, whose measured gaps are listed above.
• No test pins an empty/whitespace curated message, a non-ASCII exception message round-tripping through redaction, or a chained (__cause__) exception with an empty message. Test-pin gaps only.
• log_client_facing_failure's "template ends in %s, args fill the placeholders before it" contract is docstring-only and unenforced; all three current call sites are correctly formed.
• The AST guard's _called_name matches bare attribute names with no receiver-type check: an unrelated object's .send_text() / .broadcast_to_task() false-positive matches (harmless — fails toward redaction), and calling client_safe_error_message through a qualified attribute would be misflagged as an offender (also fails safe, just noisy). No live instance of either.

Checked and already tracked — not re-raised

Recorded for the trail, deliberately not re-litigated:

• ClientVisiblePermissionError inheriting OSError via PermissionError — raised in a prior round; only two unrelated except OSError blocks exist in the file and neither is reachable from this class's raise sites. Author's "no live hazard, non-blocking" call accepted.
• The AST guard's four confessed fail-open shapes (branch/scope-insensitive _local_assignments allowlist; AnnAssign / AugAssign not visited; .get() blessing any ast.Attribute default unconditionally; sweep floors sitting ~9-25% under measured producer/payload counts) — all four were raised in a prior round, reproduced and confirmed by the author, and deferred to #1547. A nested-closure variant was investigated this round and is the same underlying mechanism, not a distinct bypass: the offender check evaluates each candidate independently, so a "safe" candidate from a nested closure cannot suppress a genuinely unsafe one elsewhere.
• The existence of the _broadcast_terminal_command_error / agent_error gap is pre-existing, untouched by this diff, and already listed in the PR body under #1497. Only its described mechanism is wrong — see MAJOR 4.
• "Access denied" vs "Task not found" now being explicitly blessed as client-visible (an existence/enumeration oracle) — pre-existing wording, raised and discussed in a prior round, deferred with a considered rationale to the broader #1479 decision to narrow both messages together.

Simplification opportunities

Lean already.

Two candidates were proposed and tested ("delete the AST guard, rely on the behavioral tests") and both were dropped on evidence: mutation testing shows the guard catches 7/10 reverted-redaction-site mutations where the behavioral tests alone catch 3/10, and the three named behavioral tests cover roughly 4-5 handler/exception-type combinations against the guard's structural 23-producer / 28-payload sweep. Removing it would be a real detection-power regression, not an equivalent simplification.

Blocking status & recommended decision

Blocking: yes. Multiple MAJOR findings survived verification — one newly-inconsistent bare Exception that drops the connection instead of redacting, two documentation claims that are demonstrably false (one of them a retraction the PR body says was applied and was not), a chokepoint docstring that overstates its own coverage, a comment asserting traceback behavior the code does not have, a client-input echo broadcast that reproduces the exact threat model this PR closes, and no test-level guard for any of the four bypass shapes the PR admits it leaves open.

Recommended event: REQUEST_CHANGES

Blocking issues:

• src/xagent/web/api/websocket.py:6666 — MAJOR — bare Exception for "Task not found or access denied" in handle_execute_task drops the connection so the client gets no message at all; sibling raise in this same function uses ClientVisibleValidationError — [new]
• src/xagent/web/api/websocket.py:330-332 — MAJOR — stale RuntimeError carve-out comment cites a contract test_websocket_owner_actor.py does not pin, was never removed despite the PR body's retraction saying it was, and now contradicts a comment this PR added at test_websocket_client_safe_errors.py:157-168 — [new]
• src/xagent/web/api/websocket.py:355-362 — MAJOR — client_safe_error_message docstring claims coverage of every direct-call-site error payload; _broadcast_terminal_command_error (websocket.py:8310-8321) is a direct-call-site dict literal matching none of the listed caveats, escaping only via its agent_error type — [new]
• PR body / #1497 description — MAJOR — the documented durable-command broadcast mechanism (TaskCommandRejected) provably does not broadcast, while the two branches that do (TaskCommandDeferred-exhausted, generic-Exception-exhausted, websocket.py:8331-8341) are undisclosed — [new]
• src/xagent/web/api/websocket.py:6512, :6531, :6752 — MAJOR — "Re-raised: the outermost handler owns the traceback" is false; the outermost handler at websocket.py:7434-7437 also omits exc_info=True, so no traceback is ever logged on these redacted paths — [new]
• src/xagent/web/api/websocket.py:7442-7461 — MAJOR — handle_intervention broadcasts raw client-controlled action text to every connection on the task, the same injection shape this diff removes at websocket.py:~8900 — [new]
• tests/web/api/test_websocket_client_safe_errors.py — MAJOR — no test or xfail guards any of the four confessed bypass shapes (dict-spread, helper-built, wrapper-forwarded, durable-command); they are prose-only at ~296-306 — [new]

[codeacme17]

Round 4 addressed in 2e4c32f. All seven inline threads have replies. Every finding was reproduced before acting, and this time the self-review ran before the push rather than after — which is the only reason three of these landed correctly.

Your central point is the one I want to answer first

the written description of what this diff covers cannot currently be trusted without independent checking

Correct, and worse than the two instances you found. Across four rounds this description has carried seven claims that did not hold: the guard's branch-sensitivity, the sweep counts (twice), the durable-command mechanism, the completeness of a retraction, "the outermost handler owns the traceback", and "nothing reads that column back to a client". Every one had the same cause — one instance verified, the general property written down.

Two of those seven were caught by a pre-push review in this round rather than by you. The change in practice is that every factual assertion now has a command behind it whose output I read; where I could not establish a general claim, the narrow verified one is written instead, with its dependency stated.

Three findings were fixed differently from the suggestion, because tracing them turned up a second layer

:6666 (M1). Marking the raise lands it in the except (ValueError, KeyError, TypeError) branch, which hard-codes exc_info=True. Applied literally, your suggestion would let any anonymous visitor dump a full stack per guessed task id — the exact thing log_client_facing_failure was added to prevent. That branch and three siblings now route through it, so a curated refusal logs at WARNING without a stack. Pinned by a test that fails if the site goes back to hard-coded exc_info.

:6512 (M5). There is no single outermost handler. The durable path does record a stack — dispatch_one_task_command catches at task_command_transport.py:1096 and calls logger.exception at :1100, so the worker at :1216 never fires for it — while websocket_chat_endpoint and both public_chat_access.py endpoints do not. My first replacement comment said "three outer contexts and none of them logs one"; that was false and was caught pre-push. The branch that withholds the detail now logs it.

Docstring (M3). Your event-type axis is right, and there is a second: the guard only ever inspects the message key. execute_task_background (:2780) hits both at once — text under "error", type task_error, a literal outside the set rather than the variable-typed case the docstring disclosed. Both named now. Routing the terminal payload also required moving the command kind to a structured field, because an f-string wrapper is opaque to the guard and I would rather not teach it to peek inside interpolations — that is how #1547's .get() hole got in.

One correction to a finding of yours

M4 says the durable text has "no client-facing read path anywhere under src/xagent/web". It does: a2a.py:1338 returns str(stored.error …) verbatim as a 500 internal_error body. Your conclusion survives, but for a narrower reason — a2a only ever enqueues CANCEL (a2a.py:1388), so pause/resume text cannot reach it. That dependency is now written into the code, because widening a2a to another command kind would make it a real leak.

Everything else

:330-332 corrected, and the PR body now says the round-3 retraction was itself inaccurate. handle_intervention no longer interpolates client input into a broadcast. Three strict xfails pin the remaining bypass shapes, each mirroring its real site — a fourth was drafted and dropped because the durable-command channel is not client-facing and pinning it would assert a leak that does not exist. Minors: raising=False, the builder-chat log assertion, the leftover f-string, and a stated reason on the passthrough branches.

Two payload shape changes worth flagging: intervention_processed moves the action out of message into an action field, and agent_error moves the command kind into command_kind. No consumer parses either string today (checked frontend/ and the suite), but anyone who starts to should read the structured fields.

Also fixed by this round, having been mis-scoped as deferred: the agent_error terminal-command payload. #1497 has been corrected — it credited an inert mechanism and never named the live one.

Verification

Full tests/web: five pre-existing SVG-rasterization failures, identical to the parent commit, nothing new. Affected suites 101 passed, 3 xfailed. ruff and mypy clean. New and changed tests mutation-checked against their own fixes.