fix(frontend): retry transient clarification uploads and surface the real failure reason

[codeacme17]

Fixes #1468

Problem

A clarification-form submission that failed anywhere in the send pipeline showed one fixed toast (chatPage.clarification.sendError) and gave up.

1. A transient upload 503 rejected the whole submission with no retry, so the task stayed parked in WAITING_FOR_USER and the pending tool/MCP call never ran.
2. The backend's rejection text ("A previous guidance message is still being applied…") reached the form intact and was then discarded by the catch.

In an embedded widget this is a customer-facing dead end: the visitor submits, sees a generic failure, and the conversation is stuck.

What changed

Retry only what was provably refused. withUploadRetry (src/lib/upload-retry.ts) re-sends on 503 alone, with three attempts and full jitter. 502 and 504 both mean a proxy reached the upstream and could not get a usable answer back, so the upload may have landed; a rejected fetch is the same. Widening past "provably refused" needs a server-side idempotency key, not a longer status list — that constraint is recorded on the constant.

The server makes that contract true. store_uploaded_files used to log and swallow a failed rollback and answer 503 anyway. It now answers 500 unless the registration rollback actually succeeded, so the one status the client replays means what the retry assumes. (An unlinkable staged temp file leaves no row to duplicate and does not cost the client its replay.) This is the only server change in this PR.

The upload step reports its own outcome. It returns {ok} | {refused, error} | {unknown, error}, decided where the evidence is, rather than leaving flags for a distant catch to reinterpret. refused means nothing was stored and the draft is safe to resend; unknown means the file may exist under an id this client never learned — a gateway 5xx, an unreadable body, a dropped request, or ownership lost while a request was on the wire. A response that does not account for every file sent is rejected rather than parsed into a short list, on both the default and injected paths.

Resubmission cannot duplicate. Resolved ids are stamped back onto the caller's File objects as each lands, so a resubmitted draft travels as references instead of bytes — including when a sibling in the same widget batch fails. An unresolved submission keeps one client message id so a retry meets the server's existing claim; an attempt whose fate was undecided carries that history, and a server request for a new id there escalates to reconciliation instead of opening a second turn. apiRequest's blind transport replay is disabled for uploads, so only the status policy owns replay.

The visitor sees the real reason. Delivery failures carry disposition, userFacing and requiresReconciliation. The server's rejection text and upload details are shown in the toast and a new inline alert; connection-plumbing diagnostics stay behind the localized string. Only an attachment that may need reconciling blocks resubmission and asks for a reload — an unconfirmed delivery keeps its id and says so.

Testing

Frontend 138 files / 2245 tests; the required test:widget:coverage lane 33 files / 871 tests, with upload-retry.ts, public-chat-file-upload.ts and clarification-form.tsx added to its coverage floors; backend tests/web/api green. tsc, eslint, ruff and mypy clean. Regression tests for the classification edges were each checked against the unfixed code first.

Out of scope — every known gap in the touched area

• security(websocket): redact incidental exception text from chat clients #1514 — the websocket redaction taxonomy, split out of this PR on review feedback that it belonged in its own change.
• security(websocket): raw exception text still reaches chat clients through unrecognized producer shapes #1497 — raw exception text still reaching chat clients through dict-spread, helper-built and wrapper-forwarded payloads; plus getUploadErrorMessage's raw-body fallback on the frontend.
• security(websocket): agent RuntimeError text is surfaced verbatim to public chat visitors #1479 — the agent RuntimeError passthrough (an existing tested contract), and the safe_for_display / error-code field that would also let the client localize backend rejection text.
• enh(frontend): give transport.uploadFiles a cancellation hook so abandoned chat uploads stop #1471 — transport.uploadFiles has no cancellation hook, so an abandoned submission's retries keep running.
• enh(frontend): give ClarificationForm's onSend the same delivery contract as the internal path #1485 — onSend has no declared delivery contract, config?: any, and the untyped uploader result behind the file_id side channel.
• fix(frontend): taskless Session send drops message attachments silently instead of rejecting them #1470 — the taskless-Session send path drops attachments silently (latent; the capability is typed files: "disabled").
• clarification-form.tsx carries 11 pre-existing @typescript-eslint/no-explicit-any errors. Untouched; the count is unchanged by this diff.

Review history

Five rounds. The carried blockers are closed; the design critique that landed hardest — that the upload outcome was reconstructed from mutable flags rather than returned — is what the current shape is a response to.

Related

#1467, #1469, #1081

[gemini-code-assist]

Code Review

This pull request introduces a robust, bounded retry mechanism with exponential backoff and cancellation support for file uploads, integrating it into both the WebSocket and public chat upload paths. It also enhances the ClarificationForm to gracefully handle and display granular delivery failures (such as backend rejections, unsent messages, and unknown outcomes) to the user. The review feedback highlights two key improvements: adding a userFacing flag to UploadRequestError so that specific upload errors are correctly surfaced in the UI, and racing the active upload execution with the cancellation promise to ensure immediate responsiveness when a connection changes.

[codeacme17]

Thanks for the review. Both suggestions declined with evidence in the inline threads; one commit pushed in response.

• UploadRequestError.userFacing (high) — the premise does not hold: the upload block's enclosing catch in use-websocket.ts:1343-1350 always rethrows an UploadRequestError as a MessageDeliveryError with userFacing set, on both upload paths, so the specific message already reaches the toast. Adding the field would mark a reason as user facing without the disposition that decides how the state is worded. The real gap was that nothing pinned the wrapping — e08eb88 adds a use-websocket regression test for it.
• Race perform() with cancellation (medium) — accurate observation, no observable effect: use-websocket.ts:1259-1262 already races the whole call against the claim, and the widget uploader passes no cancellation. It would also let a plumbing error mask a real upload reason when both settle together, which is the failure this PR removes. Genuine cancellation needs an AbortSignal across the transport boundary, tracked as enh(frontend): give transport.uploadFiles a cancellation hook so abandoned chat uploads stop #1471.

Verification after the added test: full frontend suite 138 files / 2219 tests green, required test:widget:coverage lane 33 files / 845 tests green, tsc --noEmit and eslint clean on the changed files.

[rogercloud]

This PR adds a shared bounded upload-retry helper, applies it to both the injected public single-file uploader and the authenticated WebSocket batch uploader, and threads rejection reasons and delivery dispositions into ClarificationForm. It is intended to recover transient upload failures while preserving drafts and distinguishing rejected, not-sent, and outcome-unknown turns. The focused tests and widget coverage configuration are extended for the new helper and upload paths.
Blocking: yes — recommended event: REQUEST_CHANGES

Update summary

The reviewed update is commit e08eb88b at the current PR head; no later commit is present in the reviewed history. It adds UploadRequestError, withUploadRetry with up to three 502/503 attempts, retry wiring for both upload paths, public error/rejection plumbing, resubmit and outcome-unknown hints, and focused tests/coverage entries. Those local improvements do not establish an upload idempotency boundary or a server-approved public-error contract, so the risks below remain.

Design verdict — wrong-direction

wrong-direction. The shared helper, coverage of both production upload paths, guest-token raw fetch boundary, draft preservation, explicit delivery dispositions, and symmetric i18n are useful local choices. The central design is still wrong: it treats generic 502/503 responses as proof that no upload side effect occurred even though uploads use fresh server UUIDs and no idempotency/reconciliation key; it infers public visibility from any non-empty backend string without a server-approved safe-message field; and it lets unknown upload outcomes flow into manual resubmission without a lifecycle/idempotency boundary. The outer cancellation race also abandons the caller's wait rather than cancelling the production public uploader.

Prior findings

• P1 — DROPPED / VERIFIED-SAFE. The root was inline 3803484343, with author reply 3803559835 and conversation 5327443302. The explanation is technically correct: the current useWebSocket catch wraps UploadRequestError with userFacing=true, and use-websocket.test.ts:168-196 covers that path. I therefore do not raise a duplicate finding; this is not being reclassified as an unresolved bug.
• P2 — NOT FIXED (major; follow-up below). The root was inline 3803484360, with author reply 3803561494 and conversation 5327443302. The claim that the outer Promise.race is sufficient is only correct for returning control to the caller promptly. The production uploadFiles contract has no AbortSignal, and public-chat-file-upload.ts:88 can continue its raw fetch, backoff, and new retry attempts after claim loss; those attempts can still persist files. N7 is the same lifecycle root and is merged into this P2 follow-up, with tracking context in #1471 rather than a separate finding.

Confirmed findings

Major

• N1 — frontend/src/lib/upload-retry.ts:19 — major. Status-only retry is not idempotent. A public upload and the default batch upload classify every 502/503 as retriable, and withUploadRetry repeats the POST. The backend creates a fresh file_id for each request, has no upload idempotency key, and compensation is not a universal proof of removal; a gateway or storage response can arrive after bytes or metadata were committed. A post-commit 502/503 can therefore create duplicate rows/objects, and the batch path resends every file. Generate and reuse a stable per-file/per-batch idempotency key with a server-side atomic dedupe/replay receipt; until that contract exists, retry only a structured response that proves zero side effects rather than trusting the numeric status.

• N3 — frontend/src/hooks/use-websocket.ts:902 — major. Arbitrary rejection text becomes public-facing. The new flag is true for any non-empty data.message, while the rejection wire contract has no safe-for-display field or allowlist and backend exception paths can send str(e). The same handler serves public widget/share connections, so visitors can receive validation, runtime, provider, storage, path, or other internal details. Add a typed safe_for_display/public_message or stable error-code contract produced only by allowlisted server branches, localize/map those codes on the client, and fall back to a generic message for all other backend text.

• N4 — frontend/src/hooks/use-websocket.ts:1349 — major. Unknown upload results are reported as safe-to-resubmit. The catch wraps an upload failure as message disposition not_sent, even though the helper deliberately treats 504, transport rejection, and unreadable/malformed success bodies as potentially persisted unknown outcomes. The WebSocket turn was not sent, but the attachment may be stored; ClarificationForm then shows the not_sent “submit again” hint and a manual retry can upload it again. Carry a separate upload outcome (refused versus unknown) through the error, do not label unknown uploads safe to resubmit, and provide reconciliation or a server-backed idempotent upload handle before enabling retry.

• N5 — frontend/src/components/chat/clarification-form.tsx:313 — major. Outcome-unknown remains submit-enabled and can create a second turn. This branch only records the reload hint; finally clears isSubmitting, and the button is disabled only by active, isSubmitting, or isSubmitted. A later submit generates a fresh clientMessageId (with force: true bypassing the local duplicate guard), so a first turn that was accepted or is still pending can execute twice. Keep one client ID for the unresolved logical submission and disable the form until an accepted/replayed receipt or explicit reconciliation/reload clears the state.

• N6 — frontend/src/components/chat/clarification-form.tsx:315 — major. Deterministic rejection resubmits raw files. This new branch explicitly tells the user to submit again for rejected/not_sent, but each submit reconstructs the original File objects and the upload IDs returned by useWebSocket are only transient. If upload succeeds and the message is then rejected, the next click uploads the bytes again, leaving task-bound duplicate rows/blobs; taskless orphan cleanup does not cover those task files. The upload/cache omission pre-existed this PR, but the new explicit resubmit hint makes it directly in scope. Persist successful IDs with the draft or a task-scoped cache and resubmit those references, or add a request-owned server token with safe cleanup on deterministic rejection.

• P2 follow-up (N7 merged) — frontend/src/lib/public-chat-file-upload.ts:88 — major. Claim loss does not cancel the production retry. The outer race can stop awaiting the upload, but the injected public uploader has no cancellation parameter, fetch receives no signal, and this call supplies no cancellation to withUploadRetry; after ownership loss it can continue waiting, retrying 502/503s, and persisting orphan files. Add an AbortSignal to the transport contract, drive an AbortController from the preparation claim, pass it to fetch, and check it before each attempt and backoff; pair that with upload idempotency because abort itself still leaves an unknown outcome. This is the same prior P2 root, not a separate N7 issue.

Minor

• N2 — frontend/src/hooks/use-websocket.ts:1259 — minor (severity adjusted). Mixed inner and outer replay can multiply an authenticated batch. The default authenticated path already used the pre-existing apiRequest/fetchWithRetry, so a pure sequence of transport rejections is not a new universal regression. However, the new outer status retry composes with that inner replay when an inner attempt can reject ambiguously and a later attempt returns 502/503; the outer wrapper then resends the whole FormData batch, despite the helper's stated transport-rejection boundary. Add an explicit no-replay policy or dedicated upload request primitive for this call, and rely on a server idempotency key before permitting any replay after an ambiguous response.

• N8 — frontend/src/lib/upload-retry.ts:74 — minor. Fixed backoff synchronizes retry bursts. Production defaults are exactly 400 ms and 800 ms, with no jitter, while public uploads fan out one request per file under Promise.all; an outage can therefore produce up to 3N synchronized requests across clients. The attempt count is bounded, so this is load amplification rather than a blocker. Use capped full/equal jitter and a small per-submission/shared upload concurrency cap, while preserving cancellation.

• N9 — frontend/src/hooks/use-websocket.test.ts:168 — minor. The new seam test bypasses the default batch path. The test injects a truthy uploadFiles mock, so useWebSocket takes the injected branch and never executes the changed FormData/apiRequest/response-parser/retry fallback. Helper tests mock perform, and public-uploader tests call a separate function, so a regression in default batch construction, retry composition, or fallback cancellation could leave CI green. Add an integration-level hook test with no injected uploader that asserts the batch request, a 503-to-success retry, malformed/transport non-retry behavior, and claim cancellation request counts.

• N10 — frontend/vitest.widget.config.ts:31 — minor. ClarificationForm is outside the explicit widget coverage contract. The coverage include list adds the new public uploader and retry helper, with per-file floors, but omits src/components/chat/clarification-form.tsx even though this PR adds its delivery-failure branches; clarification-form.test.tsx being in test.include does not instrument the source. The widget lane can therefore remain green without enforcing coverage for the new form behavior. Add the form source to coverage.include and give it a matching thresholds per-file floor.

Review criteria and limitations

I evaluated Code Quality (duplication, verbosity, and readability), Code Correctness (logic, edge cases, and error handling), Test Quality & Coverage, Design & Architecture, and Documentation/comments, including the public error boundary, retry/resource lifecycle, concurrency, security, and performance implications. All 14 reported CI checks completed successfully. No local tests were run, per PR-review policy; the review used source and test inspection only. The Simplification Lens was unavailable because review-spark hit usage_limit_reached, so no simplification findings are inferred.

Blocking status & recommended decision

Blocking: yes

• frontend/src/lib/upload-retry.ts:19 — major — Generic 502/503 retries can duplicate committed uploads. [new]
• frontend/src/hooks/use-websocket.ts:902 — major — Arbitrary backend rejection strings can be exposed to public visitors. [new]
• frontend/src/hooks/use-websocket.ts:1349 — major — Unknown upload outcomes are collapsed into a resubmit-able not_sent result. [new]
• frontend/src/components/chat/clarification-form.tsx:313 — major — Outcome-unknown turns remain enabled and can be resent with a fresh ID. [new]
• frontend/src/components/chat/clarification-form.tsx:315 — major — Resubmission after message rejection reuploads already-stored files. [new]
• frontend/src/lib/public-chat-file-upload.ts:88 — major — Public retries continue after preparation-claim loss because no cancellation reaches fetch. [prior]

Recommended event: REQUEST_CHANGES. N2, N8, N9, and N10 are minor and do not block.

[codeacme17]

Thanks — this was a substantive review and most of it landed. One commit, 293ed44; every inline thread has a reply.

Fixed (5 of 6 majors, all 4 minors)

• N1 — 502 removed; 503 is the only retriable status. You were right that "5xx" is not a safety argument: 502 means a proxy reached the upstream and could not get a usable answer back, the same shape as the 504 already excluded. 503 stays because the app raises it only after store_uploaded_files has awaited its compensation, and a gateway 503 never reached the app. The comment on the constant now records that widening past "provably refused" needs a server idempotency key.
• N5 — the form keeps one client message id for an unresolved submission (fresh only on success or retry_with_new_id), and blocks resubmission outright once the outcome is unknown. The id half was the part I had missed.
• N6 — resolved upload ids are stamped back onto the caller's File objects, so a resubmitted draft sends references instead of bytes.
• N4 — addressed at the cause rather than in the wording: with ids stamped and 502 no longer retriable, a resubmit cannot duplicate a file regardless of the upload's outcome. not_sent remains literally correct for the turn (the throw precedes socket.send).
• N2 — the two retry layers fire on disjoint conditions (fetchWithRetry on thrown fetch/status 0 only), so there is no single-response multiplication; narrowing to 503 removes the ambiguous half of the sequential case.
• N8/N9/N10 — full jitter with an injectable source; two tests that actually drive the default batch path (no injected uploader); clarification-form.tsx added to coverage.include with a 75/65/60/75 floor.

N3 — half fixed, half tracked, and one correction

Incidental ValueError/KeyError/TypeError text no longer reaches clients; curated messages raise a new ClientVisibleValidationError and keep their wording, pinned by a new backend test.

The RuntimeError path is deliberately unchanged. Redacting it broke test_websocket_owner_actor.py:2260, which asserts an agent post_user_message failure must reach the sender — so that passthrough is a pinned contract, and deciding whether an anonymous visitor should see it is a product call. Tracked as #1479, with your safe_for_display/error-code shape as the proposed fix.

The correction: this PR does not introduce that exposure. The same handler already emits the identical string as an error event, which app-context-chat.tsx:882 reads unchanged and renders as an assistant error bubble at :5578 — same text, same visitors, on main today.

P2/N7 — confirmed and deferred to #1471; it needs an AbortSignal on the transport contract plus both implementations, and per that issue should land with the idempotent upload handle rather than alone.

Verification — frontend 138 files / 2226 tests; required test:widget:coverage lane 33 files / 852 tests with the new floors; test:app-pages 10 files / 227 tests; backend websocket suites 140 tests; tsc --noEmit, eslint, ruff check, ruff format clean.

One unrelated observation from running the full frontend suite repeatedly: five tests flake under machine load with Test timed out in 5000ms — in src/app/build/page.test.tsx, src/app/workforces/workforces-routes.test.tsx, src/components/build/agent-triggers-dialog.test.tsx, and src/components/workforce/workforce-builder.test.tsx. None touch this diff, and two are in the required test:app-pages lane. Flagging rather than filing, since it is outside this PR's area.

[rogercloud]

PR 1472 changes the clarification form and chat transport so drafts survive upload or delivery failures, safe server rejection details reach the sender, and delivery IDs prevent accidental double turns. It adds bounded full-jitter retries for selected upload responses, propagates not_sent/rejected/outcome_unknown, covers authenticated and public/share uploaders, and adds backend validation redaction. The follow-up narrows status retries and strengthens tests around retry and unknown-outcome paths.
Blocking: yes — recommended event: REQUEST_CHANGES

Approach / design verdict

Verdict: acceptable-with-reservations. The direction is appropriate: it reuses the existing durable turn identity and acknowledgement dispositions, centralizes bounded upload retry, preserves the draft, and fails closed for ambiguous WebSocket delivery. It is materially safer than blindly retrying every 5xx or adding a generic UI retry over an unknown outcome.

Design note. Replay safety is still inferred from HTTP 503 even though the backend's compensation can fail; the injected public uploader fans out per-file requests without preserving partial results into the draft; and the client treats every non-empty rejection string as safe while preparation/outer backend paths can still serialize raw exceptions. The stronger root design is an upload idempotency key (or an explicit server replay-safety contract), a typed per-file/cancellation-aware upload result, and a server-authored safe error code/message rather than a client-inferred trust bit.

Update summary

293ed445e5eab6e0d3e9a6d7906ad3363bb9be1d is the one commit since e08eb88b589de7868e1ae348fd71d8956108ccde; it changes 9 files with 410 additions and 33 deletions. It narrows upload retries to explicit 503 UploadRequestErrors, adds full jitter and post-upload ID stamping, strengthens clarification-form delivery-ID and unknown-outcome handling, and introduces ClientVisibleValidationError redaction with focused tests. It does not make the public injected uploader atomic/idempotent, nor does it make every WebSocket rejection path safe to render; those residuals are covered below.

Prior-finding status

Root	Status	Current verification
P1	DROPPED / VERIFIED-SAFE	UploadRequestError is marked userFacing=true on both task-bound upload paths; no follow-up and no resolution.
P2	DROPPED under the explicit tracking rule	The cancellation concern remains deferred to issue #1471; do not re-report or resolve it here.
N1	PARTIAL — major, still blocking	502 was removed, but status-only 503 still retries without proof that compensation succeeded or an idempotency key.
N2	PARTIAL — minor, still open	Authenticated apiRequest/fetchWithRetry transport replay can compose with a later outer 503 retry.
N3	PARTIAL — major, still blocking	Inner and legacy validation catches are sanitized, but preparation and outer/pre-dispatch paths can still serialize raw exception text. The RuntimeError half remains tracked in #1479 and is not duplicated.
N4	PARTIAL — major, still blocking	Successful IDs are stamped for the complete-upload/deterministic-rejection case, but ambiguous upload errors still reject before stamping and permit raw-file resubmission.
N5	FIXED	Stable deliveryAttemptRef plus the permanent outcome_unknown block prevents a second turn.
N6	FIXED	A complete upload followed by deterministic message rejection stamps and reuses successful file IDs.
N8	FIXED	Full jitter is used for bounded upload backoff.
N9	FIXED	Default-path tests no longer inject an uploader when testing the authenticated batch path.
N10	FIXED	Clarification-form include/floor coverage is present.

Confirmed findings

Major

[prior N1] 503 is not proof of upload replay safety — frontend/src/lib/upload-retry.ts:24

Severity: major. I saw reply 3805197552 asserting that the upload endpoint compensates before returning 503. The current backend can fail during compensation after durable writes, log and swallow that failure, and still return 503; this helper has no idempotency key to deduplicate the batch. A subsequent whole-batch retry through frontend/src/hooks/use-websocket.ts:1212-1261 can therefore duplicate or orphan a file. Make 503 retryable only after compensation is confirmed, classify compensation failure as outcome_unknown, and/or enforce a server-side upload idempotency key.

[prior N3] client-visible redaction is still incomplete — frontend/src/hooks/use-websocket.ts:902

Severity: major. I saw reply 3805206336; I agree that the inner chat validation catch and legacy execute catch now use _client_safe_validation_message. However, _prepare_websocket_turn_sync runs before the inner catch, and the outer format/generic paths still pass str(e) to finish_delivery_failure; this new userFacing flag at frontend/src/hooks/use-websocket.ts:900-903 marks every non-empty rejection string as user-facing. A preparation or outer failure can therefore put paths, provider details, or other internals into the acknowledgement/toast. This is the same redaction root, not a new duplicate; the separate RuntimeError behavior tracked in #1479 is intentionally excluded. Route every message_rejected producer through one fail-closed safe message/code and add coverage for preparation and outer/generic paths.

[prior N4] ambiguous upload failures remain replayable — frontend/src/hooks/use-websocket.ts:1360

Severity: major. I saw reply 3805202656 that successful IDs are now stamped and reused after a complete upload followed by deterministic message rejection. That fix does not cover failures before uploadedFiles resolves: a 504, malformed response, exhausted 503, or transport rejection can leave an upload persisted while this catch reports not_sent and the form retains raw File objects. Resubmission can then duplicate or orphan attachments. Classify ambiguous upload outcomes as outcome_unknown and require reconciliation, or enforce upload idempotency/per-file durable result semantics before allowing a raw-byte retry.

[new] public multi-file upload drops partial successes — frontend/src/hooks/use-websocket.ts:1268

Severity: major. The widget/share transport at frontend/src/components/widget/public-agent-chat-page.tsx:603-613 starts one upload per file under Promise.all. If file A succeeds and file B rejects (for example, 413), the aggregate rejects before this all-or-nothing stamping block runs; the retained draft then resubmits A's raw bytes and duplicates the already-persisted sibling. Preserve per-file IDs/results in the retained draft as each upload completes, or use an atomic/idempotent multi-file endpoint, and add a two-file success-plus-failure retry test.

Minor and informational

[prior N2] nested transport and status retries can compose — frontend/src/hooks/use-websocket.ts:1259

Severity: minor. I saw reply 3805206638 arguing that the predicates are disjoint. They are disjoint for one response, but apiRequest's inner fetchWithRetry can replay a post-commit transport rejection; if a later response is 503, the outer withUploadRetry here starts another whole-batch request. Use one commit-safe retry/idempotency policy for this upload path, or make the inner request non-retrying when the outer status policy owns replay.

[new] onSend bypasses the form's delivery-ID context — frontend/src/components/chat/clarification-form.tsx:305

Severity: minor. The internal sendMessage branch allocates or reuses deliveryAttemptRef, but the injected onSend branch at lines 302-304 receives only (message, files, metadata) and no delivery ID, cancellation, or disposition contract, while the catch below still applies retryWithNewId and outcome_unknown handling to both branches. A builder callback therefore owns a separate protocol and can retry or commit without the form's duplicate-turn guarantees. Type this callback with the shared delivery result/error contract and pass the attempt context, or make the form own the same delivery ID for both paths.

[new] outcome_unknown editing remains intentionally sticky — frontend/src/components/chat/clarification-form.tsx:203

Severity: minor (product-policy confirmation, non-blocking). After an unknown outcome, this guard keeps resubmitBlocked set even when a field is edited, and the Submit button remains disabled at line 590; the locales tell the visitor to reload. That is the safe duplicate-turn policy today, not a blocking correctness defect, but please confirm the product decision or provide an explicit reconciliation/reload action that can clear the block rather than silently clearing it on edit.

[informational] positional ID stamping is safe under today's order contract — frontend/src/hooks/use-websocket.ts:1268

The current backend preserves multipart order and the public Promise.all preserves input order, so this index-based assignment is safe today when the response contains exactly one result per input. Keep that invariant explicit and typed (or key results by a stable file token); if an uploader later reorders or filters results, this block could stamp the wrong File.

[informational] preserve the explicit ClientVisibleValidationError maintainer contract — src/xagent/web/api/websocket.py:339

_client_safe_validation_message intentionally preserves text only for ClientVisibleValidationError and redacts every other ValueError/KeyError/TypeError. Keep future sender-actionable validation messages on this explicit subtype, and add the corresponding path test, rather than widening the broad catch to expose generic exception text.

[new] typed delivery/retry semantics are scattered (body-only design note)

Severity: minor. ClarificationForm duck-types disposition/retry fields, AppContext exposes config?: any, onSend has no declared delivery contract, and uploadFiles returns a bare array without a shared lifecycle/error shape. Introduce one small discriminated delivery-failure/send-options contract and a typed uploader result rather than relying on undocumented properties. This is an API-maintainability concern, not a separate blocker or a duplicate of the concrete findings above.

CI status and review limitations

CI preflight passed all reported checks. No local tests, builds, linters, or formatters were run under PR-review policy; the changed test code was inspected statically. The initial and update Simplification Lens runs were unavailable because both returned usage_limit_reached; no simplification findings are asserted and no empty Simplification opportunities section is included. Remaining conclusions are from static code/history verification, including the complete exported review/conversation/inline history and linked issue discussions.

Blocking status & recommended decision

Blocking: yes — recommended event: REQUEST_CHANGES

Blocking issues:

• frontend/src/lib/upload-retry.ts:24 — major — [prior] N1: status-only 503 can replay a whole batch after compensation failure, with no upload idempotency key.
• frontend/src/hooks/use-websocket.ts:902 — major — [prior] N3: preparation and outer rejection paths can still expose raw exception text.
• frontend/src/hooks/use-websocket.ts:1360 — major — [prior] N4: ambiguous upload failures are reported as not_sent, so the form can resubmit raw bytes.
• frontend/src/hooks/use-websocket.ts:1268 — major — [new] public multi-file partial success is discarded before draft retry.

Carried but non-blocking:

• frontend/src/hooks/use-websocket.ts:1259 — minor — [prior] N2: inner transport replay and a later outer 503 retry can compose.

Minor and informational findings do not change the requested decision.

[codeacme17]

All four blocking findings fixed in d38ee36; the two carried minors are fixed or tracked. Every inline thread has a reply.

Two corrections to my previous round's replies, which were wrong

• N1 — I claimed 503 could only escape after compensation completed. It awaited compensation, but a failed rollback was caught, logged and swallowed, and the 503 propagated anyway. I had verified that compensation was awaited and asserted that it succeeded. store_uploaded_files now answers 500 (_durable_storage_outcome_unknown) when the rollback fails, so the status the client replays means what the retry assumes. Parametrized test: compensation succeeds → 503, compensation fails → 500.
• N3 — I fixed one producer out of eight and reported the path covered. My search matched the message = f"..." assignment shape; three producers passed str(e) as an argument and four error bubbles interpolated it inline. All eight now route through client_safe_error_message, and sender-authored messages raise ClientVisibleError subclasses so an unmarked exception fails closed.

The durable fix for N3 is not the eight edits — it is a new AST guard that walks the module, resolves each producer's message expression in its own scope, and fails unless it is a literal, a client_safe_error_message(...) call, or a forwarded parameter. I confirmed it is not vacuous by reintroducing a leak at a delivery producer and again at an error bubble; it caught both. "Did I find them all" was my failure mode two rounds running, and grepping does not settle it — this replaces it with "can anything bypass".

Also fixed

• N4 — UploadRequestError now carries a refused/unknown outcome. A gateway 5xx, an unreadable success body or a dropped request surfaces as outcome_unknown, which routes into the N5 block, so the form asks for a reload instead of offering a resubmit. A refusal still reports not_sent; a pre-upload failure still reports not_sent.
• Public multi-file partial success — uploadPublicChatFile stamps each id as its own request lands, before the aggregate settles, so a sibling's 413 no longer discards what already uploaded. This also removes the order assumption in your informational note: per-file stamping needs no positional pairing.
• N2 — apiRequest takes an explicit replay policy and the batch upload opts out, so the shared transport retry cannot re-send a request that may already have committed underneath the status policy that owns replay here.

Tracked, not fixed — #1485 for the onSend delivery contract and the untyped config/uploader shapes; #1479 for the RuntimeError passthrough (allowlisted explicitly in the guard, so narrowing it stays a visible decision); #1471 for upload cancellation.

The sticky outcome_unknown edit is deliberate: clearing the block on an edit hands back the second-turn path N5 was about, so the escape hatch is a reload. Happy to add an explicit reconciliation control instead, as a product change.

Verification — backend tests/web/api full suite green; frontend 138 files / 2235 tests; required test:widget:coverage lane 33 files / 861 tests; ruff check, ruff format --check, mypy, tsc --noEmit and eslint clean.

I have not added an upload idempotency key, and I am not claiming the retriable set can widen without one — that constraint is recorded on the constant itself.

[rogercloud]

PR Summary

Adds a bounded-retry helper (withUploadRetry, 3 attempts) for clarification-form file uploads, applied to both the widget/share single-file uploader and the authenticated batch uploader. Distinguishes retriable (503, meaning "provably rolled back") from non-retriable upload failures, and adds an outcome axis (refused vs unknown) so ambiguous failures (504, dropped connection, unreadable body) surface as outcome_unknown (blocks resubmission, asks for reload) rather than not_sent (safe to resubmit). Also hardens backend client-visible error redaction (client_safe_error_message, ClientVisibleError subclasses, a new AST guard) so raw exception text doesn't leak to public widget visitors, and stamps upload file_ids back onto File objects to prevent duplicate-byte resubmission.

Update Summary

Since the last review, commit d38ee36 claims to fix all four remaining findings (N1, N3, N4, the public multi-file partial-success issue) plus minor N2. Independent re-verification confirms N1, N2, N4, and the partial-success fix are genuinely fixed and well-tested. N3 (raw exception text reaching public clients) is not fixed — the sweep closed two previously-cited producers but missed two new leak paths and one previously-flagged path that was never touched. This round also surfaced two new blocking findings around cancellation/reconnect handling that undermine the very outcome_unknown protection this PR introduces.

Design Verdict

Acceptable-with-reservations, carried forward from the previous round. The direction is sound: reusing durable turn identity, bounded/jittered retry keyed on a provably-safe status, and failing closed on ambiguous outcomes for most cases. However, the redaction chokepoint (N3) and the ambiguous-outcome classification (cancellation/reconnect handling) both still have real gaps, detailed below.

Prior-Finding Status

Finding	Previous status	This round (post d38ee36)
N1 — 503 not proof of replay safety	Open	FIXED — _durable_storage_outcome_unknown() returns 500 unless compensation is confirmed to have succeeded; frontend only retries 503. Pinned by a parametrized test in tests/web/api/test_upload_connection_boundary.py that drives the real branching logic.
N2 — nested transport+status retry composition	Open (minor)	FIXED — apiRequest takes a replayTransportFailures policy (default true, preserving behavior for other callers); the upload batch call site passes replayTransportFailures: false, so only the outer status retry owns replay. Confirmed by call-count assertions in api-wrapper.test.ts.
N3 — arbitrary rejection text becomes public-facing	Open	STILL NOT FIXED — see Critical finding below.
N4 — unknown upload results reported as safe-to-resubmit	Open	FIXED — outcome axis (refused/unknown) added; the catch defaults to unknown unless explicitly proven refused, so timeouts/aborts/malformed bodies all correctly surface as outcome_unknown. Well covered by tests.
Public multi-file partial success	Open	FIXED — uploadPublicChatFile stamps each file's id per-request, independent of Promise.all siblings. Confirmed by a two-file (one success, one failure) test asserting the successful file keeps its id.

Confirmed Findings

Critical

[CRITICAL, prior finding, still open] N3 — Raw exception text still reaches public clients via three producers

The client_safe_error_message/ClientVisibleError chokepoint and its new AST guard (tests/web/api/test_websocket_client_safe_errors.py) are real improvements and do close the two previously-cited producers. Independent verification found the sweep is incomplete again (third time), and three producers still send raw exception text to real client connections:

1. src/xagent/web/api/websocket.py:2749, in execute_task_background: message = str(e) is broadcast verbatim via manager.broadcast_to_task({**terminal_payload, "error": message, ...}, task_id). Untouched by this PR's diff (only an unrelated ValueError→ClientVisibleValidationError swap earlier in the function was made).
2. src/xagent/web/api/websocket.py:3620, in execute_resume_background (the V2 resume path): error_message = str(e) is forwarded raw into notify_deferred_delivery(False, error_message, ...) → send_message_delivery(...), building the exact message_rejected ack N3 was originally about, and also into manager.broadcast_to_task(create_terminal_task_error_event(task_id, error_message), task_id) — a raw task_error broadcast to every task listener.
3. src/xagent/web/api/websocket.py:7331, in send_historical_data_as_stream: "message": f"Data format error: {str(e)}" — never rewritten to use client_safe_error_message; this is the same "outer format path" leak flagged in the previous review round, still untouched.

Why the AST guard misses these: it only recognizes a producer when the call is literally finish_delivery_failure/finish_delivery/send_message_delivery, or when send_personal_message/broadcast_to_task receives an ast.Dict literal with a literal "type": "error" key (and even then it only inspects "message", never "error"). It misses dict-spread payloads, payloads built by a helper function and passed as a bare Call/Name, and wrapper functions (notify_deferred_delivery) that forward a raw parameter to a real producer without appearing in the checked PRODUCERS list.

Severity context: DurableStorageOperationError (raised by the storage layer this same PR touches) is itself a subclass of RuntimeError, and internal docs note durable-storage exception text carries scope segments, storage prefixes, and tenant identifiers — so a storage fault mid-turn can leak tenant/scope-identifying internals to an anonymous widget/share visitor through the two execute_*_background paths above. This is the exact category of information N3 was raised to stop.

Note: the RuntimeError passthrough for post_user_message (allowlisted in the AST guard) is a legitimate, deliberately tracked exception (issue #1479) — not flagged here.

Suggested fix: route all three producers through client_safe_error_message/ClientVisibleError before they reach broadcast_to_task/send_personal_message/send_message_delivery. Then strengthen the AST guard (or replace it, see Simplification below) to trace dict-spread payloads and wrapper-function forwarding, not just literal dicts at the direct call site.

---

[CRITICAL, new] Cancellation during an in-flight upload is reported as not_sent, bypassing this PR's own outcome_unknown protection
frontend/src/hooks/use-websocket.ts:1270

All rejectPreparations(...) call sites construct a MessageDeliveryError with disposition: "not_sent" baked in at construction time, before the upload's actual state is known. When claim.cancellation (the "this submission attempt was superseded/replaced" signal, e.g. a socket reconnect) fires while Promise.race([withUploadRetry(...), claim.cancellation]) is still racing an in-flight upload, this pre-baked not_sent error wins the race. The outer catch's if (error instanceof MessageDeliveryError) throw error (line 1368) rethrows it verbatim, so the uploadOutcome remapping logic (lines 1375-1383 — the code that correctly turns every OTHER ambiguous mid-upload failure into outcome_unknown) is never reached for this race.

Concretely: a slow file upload (large attachment / weak connection — exactly when a reconnect is most likely) that gets superseded by a reconnect reports not_sent even if the file lands server-side moments later, inviting the user to resubmit and duplicate it. This directly defeats the outcome_unknown mechanism this PR introduces to prevent exactly this class of duplicate. A test at use-websocket.test.ts:1288-1354 exercises this exact race (explicitly asserts the cancellation settles before the upload does) but only asserts error.message contains "replaced" — never asserts disposition — so nothing in the suite catches a wrong disposition here.

Suggested fix: don't hardcode disposition at rejectPreparations construction time when an upload may still be in flight; defer the disposition decision to the same remapping logic used for other failure shapes, or have rejectPreparations check whether an upload was in-flight for this claim and use unknown in that case.

Major

[MAJOR, new] outcome_unknown also fires — and permanently locks the form — on an ordinary WebSocket disconnect/reconnect, not just on a genuinely ambiguous upload outcome
frontend/src/hooks/use-websocket.ts (onclose/onerror handlers, ~lines 736-743, 851-860 — outside this PR's diff, pre-existing), frontend/src/components/chat/clarification-form.tsx:590

The socket's onclose/onerror handlers unconditionally mark any pending delivery outcome_unknown before separately deciding whether to auto-reconnect (reconnect uses maxReconnectAttempts = 3 with backoff, and IS attempted for ordinary transient disconnects on a task-bound connection). So a routine drop-and-reconnect during ack-wait — a normal operating event for this socket layer, not a rare edge case — produces the same permanent "cannot resubmit, please reload" lockout as a genuinely ambiguous upload outcome. clarification-form.tsx sets resubmitBlocked = true on any outcome_unknown disposition with no in-component way to clear it (only a full page reload). This conflates "the connection blipped and reconnected" (usually safe to just resubmit) with "we genuinely don't know if an upload committed" (correctly should block).

Suggested fix: only escalate to outcome_unknown from onclose/onerror when reconnection ultimately fails (exhausts maxReconnectAttempts) or when there was an in-flight upload at the moment of disconnect; a successful reconnect that never touched an in-flight upload should not permanently lock resubmission.

---

[MAJOR, new] resubmitBlocked and deliveryAttemptRef are never reset except on unmount, and the live-turn render path never remounts ClarificationForm between clarification rounds on the same task
frontend/src/components/chat/clarification-form.tsx:118

The useEffect keyed on active resets only isSubmitted/isOpen when the form reactivates — not resubmitBlocked, sendFailure, or deliveryAttemptRef. This is reachable, not theoretical: the live "virtual message" render path in task-conversation-panel.tsx (unlike the historical message list, which is keyed by item.id) renders the waiting-for-user ClarificationForm with no key prop, staying mounted across isProcessing || paused || waiting_for_user || failed — so the same component instance persists across multiple clarification rounds within one task. If round 1 ends in outcome_unknown (including via the ordinary-reconnect trigger above) and the conversation proceeds to a round 2 clarification, resubmitBlocked stays stuck from round 1, and the stale deliveryAttemptRef (round 1's client_message_id) could attach to round 2's answer. No test exercises active toggling false→true across two rounds on one instance.

Suggested fix: reset resubmitBlocked, sendFailure, and deliveryAttemptRef in the same active-keyed effect that resets isSubmitted/isOpen; alternatively, key the live-turn ClarificationForm render by a per-round identifier so a fresh round always gets a fresh instance.

Minor / Informational (non-blocking)

[MEDIUM] frontend/src/lib/api-wrapper.ts's getUploadErrorMessage (outside this PR's diff, pre-existing, ~line 210-222) has a last-resort fallback that returns raw (200-char-truncated) response body text when the body is neither JSON-with-detail/message, HTML, nor a 413 — this reaches UploadRequestError.message marked userFacing: true and renders verbatim to a public widget visitor, with no allowlist/redaction chokepoint analogous to the backend's client_safe_error_message. Narrower than a full pass-through (JSON-detail/413/HTML already routed to safe fixed strings) but a real gap — e.g. a plain-text infra error ("upstream connect error...") would leak through. Worth fixing alongside N3 (same threat model), not blocking on its own.

[MINOR] frontend/src/hooks/use-websocket.ts:1282, frontend/src/lib/public-chat-file-upload.ts: attaching file_id directly onto caller-owned File objects is a side channel. If a File survives a failed send (no try/catch clears TaskConversationPanel's files state on sendMessage throw) and the user navigates to a different task (no remount on task-id change), a resubmit attaches a file_id bound to the wrong task. Confirmed this fails closed: bind_turn_files_no_commit only claims rows with task_id IS NULL, so a cross-task file_id is rejected with a user-visible "Files are no longer bindable" error — an availability/UX annoyance, not data leakage or corruption. Also confirmed messageData.files = [...preUploadedFiles, ...uploadedFiles] reorders attachments relative to original selection order on resubmit — cosmetic only.

[NON-BLOCKING, pre-existing] use-websocket.ts's batch-upload response parsing returns [] (not a thrown error) on a 200 with success: false or a malformed/short files array, and nothing gates message assembly on uploadedFiles.length matching the selected count beyond the id-stamping block. Confirmed via diff against the base commit that this logic is byte-identical before/after this PR (moved, not changed), and confirmed the backend has no code path that returns such a response (all-or-nothing: any per-file failure raises an HTTPException, handled by the already-safe !response.ok branch). Not introduced or widened by this PR, not currently reachable. Worth a defensive hardening note only.

[DROPPED — duplicate of tracked #1471] Neither widget upload call site passes retry.cancellation into uploadPublicChatFile, so an abandoned upload's retries keep running after the outer race is lost. This is exactly issue #1471's tracked scope, no new symptom beyond it. Not reported as a separate finding.

[DESIGN NOTE, non-blocking] src/xagent/web/api/files.py's compensated-gated 500-vs-503 split (the N1 fix) is correctly wired, but given the current write ordering (durable object written before db.commit(), so DurableStorageOperationError fires pre-commit, and the compensation path basically cannot fail in the same request), compensated=false is very hard to reach in production — the pinning test only reaches it by monkeypatching the compensation function. Not incorrect, just a documented invariant broader than what the code path currently guarantees.

[DESIGN NOTE, non-blocking, already acknowledged in-PR] RETRIABLE_UPLOAD_STATUSES = new Set([503]) treats any 503 as proof of safe rollback, but that endpoint has multiple 503 producers, and a gateway/proxy 503 can occur after a request was already forwarded to the app in some infra configurations. The code's own comment already acknowledges this ("widening past 'provably refused' needs a server idempotency key") — a known, documented residual risk, not a fresh gap.

Simplification Opportunities

L67: delete: `attempts`/`baseDelayMs` fields on `UploadRetryOptions` (frontend/src/lib/upload-retry.ts:67-70) are never overridden by any production caller — only test code sets them, and removing them loses no test coverage/speed. Hardcode 3 attempts / 400ms as internal constants.
L126: shrink: the 160+ line hand-rolled AST guard (tests/web/api/test_websocket_client_safe_errors.py:126-289) has real detection gaps (dict-spread payloads, wrapper-function forwarding, checks only the "message" key not "error") that undercut its soundness relative to its cost. Replace with a grep-based check plus a review checklist item, or close the gaps if the AST approach is kept.

net: -~180 lines possible

Blocking Status & Recommended Decision

Blocking: yes — recommended event: REQUEST_CHANGES

Blocking issues:

• src/xagent/web/api/websocket.py — N3: raw exception text still reaches public clients via three producers [prior, still open]
• frontend/src/hooks/use-websocket.ts:1270 — cancellation during in-flight upload reported as not_sent [new]
• frontend/src/hooks/use-websocket.ts (onclose/onerror) — outcome_unknown fires and permanently locks the form on ordinary reconnect, not just genuine ambiguity [new]
• frontend/src/components/chat/clarification-form.tsx:118 — resubmitBlocked/deliveryAttemptRef never reset across clarification rounds on the same mounted form [new]

Non-blocking: the medium (getUploadErrorMessage fallback) and minor findings, and the design notes above — worth addressing but not gating this round.

Note on verification: this review is static-only (code reading + prior test evidence review); no tests were run locally. CI is reported as passing all checks on the latest commit.

[codeacme17]

Three of the four blocking findings are fixed in 7dddf20; the simplification is applied in b9db134. N3 I am escalating for a scope ruling rather than sweeping a fourth time — reasoning below, and I would like a maintainer's call before this PR moves again.

Fixed — and two of these were defects this PR introduced

• Cancellation reported as not_sent (critical) — confirmed exactly as described. The outer catch now computes the upload outcome before rethrowing, so ownership lost while bytes were moving becomes outcome_unknown; ownership lost outside an upload stays not_sent. Tests assert disposition, which the existing race test never did.
• Reconnect locking the form (major) — this was a regression I introduced last round. The socket marks every pending delivery outcome_unknown on close, before deciding to reconnect, which it does for ordinary drops; my new block then turned a routine blip into "reload the page". The block now keys on whether a resubmit could duplicate something the server cannot deduplicate — an attachment that may have landed under an id this client never learned. An unknown delivery outcome no longer blocks, because the turn keeps its client message id and the server adjudicates the duplicate. That also retires the "sticky edit" policy question: the sticky case is now only the genuinely unreconcilable one.
• State surviving across clarification rounds (major) — also mine. Every per-submission guard I added was per-instance rather than per-round; the active effect now clears resubmitBlocked, sendFailure and deliveryAttemptRef, covered by a two-round test.
• file_id side channel (minor) — folded into enh(frontend): give ClarificationForm's onSend the same delivery contract as the internal path #1485, since the honest fix is the typed uploader result that issue already covers.
• Retry options — attempts/baseDelayMs are internal constants now.

N3 — escalating

You are right that it is still not fixed, and right about why the guard missed it. I verified all three producers. I have corrected the guard's docstring and the chokepoint's, which both claimed more than they enforce.

What I am not doing is a fourth sweep inside this PR, and the record is the argument: three rounds, three incomplete sweeps, each finding producers further from anything this PR touches — execute_task_background, execute_resume_background, send_historical_data_as_stream. "Client-visible text" is a systemic property of an ~8000-line module, and each of my claims about having covered it has been too strong. A fourth attempt inside a frontend retry PR would most likely produce a fourth.

So: #1497 carries the three producers, the three guard gaps, the DurableStorageOperationError severity note, and the frontend half you flagged (getUploadErrorMessage's raw-body fallback reaching userFacing). It proposes either closing the gaps or inverting the design so payload construction is the only typed path to a client — which would make the guard unnecessary rather than smarter.

This PR keeps what it already landed there — eight producers routed, the ClientVisibleError marker hierarchy, the guard for recognized shapes with its scope stated — and declares the rest out of scope.

@maintainers — the scope call I need: is the partial hardening plus #1497 acceptable for this PR to land, or should the backend redaction be reverted out entirely so this goes back to frontend-only? I do not think a fourth in-PR sweep is the right answer either way, and I would rather have that decided than keep iterating.

Verification — frontend 138 files / 2239 tests; required test:widget:coverage lane 33 files / 865 tests; backend tests/web/api green; ruff, mypy, tsc, eslint clean.

[rogercloud]

This PR hardens clarification-message delivery across the WebSocket client and attachment uploader. It adds typed upload outcomes, bounded retries for explicitly refused 503 uploads, lifecycle cancellation, stable client-message IDs, and form guards so retries do not duplicate turns or attachments. It also surfaces selected backend details and adds coverage for reconnect and upload outcomes.

Blocking: yes — recommended event: REQUEST_CHANGES

Update summary

Since the last review, commit 7dddf20 fixed the old in-flight cancellation classification, ordinary reconnect reconciliation lock, and per-round form-state reset. Commit b9db134 simplified the retry configuration to internal constants. Those updates close the previously reported 7dd paths, but cancellation during retry backoff can still erase a definitive 503 refusal, and malformed-response, localization, and state-copy edges remain.

Prior-finding status

Root	Status	Evidence
N1 status-only retry safety	FIXED	Verified in d38; tests and backend compensation were reviewed.
N2 nested retry composition	FIXED	Verified in d38; the batch call disables transport replay.
N3 raw client-error redaction	DROPPED / VERIFIED-TRACKED	OPEN #1497 tracks the same remaining raw producers and frontend fallback and explicitly places them out of #1472; no duplicate finding.
N4 unknown upload classification	FIXED	Verified in d38 through the refused/unknown outcome axis and file-id stamping.
Public multi-file partial-success handling	FIXED	Verified in d38 for the widget/share uploader.
Old in-flight cancellation reporting not_sent	FIXED	7dd now reports outcome_unknown plus reconciliation when bytes may have landed.
Ordinary reconnect outcome_unknown form lock	FIXED	7dd allows same-client-ID retry unless attachment reconciliation is required.
Form guards leaking across rounds	FIXED	7dd resets the per-submission guards and attempt reference on a new active round.
File-object file_id side channel	DROPPED / VERIFIED-TRACKED	Tracked in #1485; no duplicate finding.
Public uploader continuing after claim cancellation	DROPPED / VERIFIED-TRACKED	Tracked in #1471; no duplicate finding.
attempts/baseDelayMs simplification	FIXED	b9 moved the retry tunables to internal constants.
P3 uploadPhase before missing-task guard	DROPPED / VERIFIED-SAFE	Shipped callers are pre-guarded and the connection invariant makes this path unreachable; no report.

Design verdict

acceptable-with-reservations. The typed outcome axis, bounded retries restricted to 503, and stable client IDs are coherent with the requirement to avoid duplicate side effects. The reservation is at the cancellation boundary: the helper tracks a definitive upload outcome but lets generic cancellation win over it, so the UI reconciliation decision is no longer grounded in the strongest available evidence. The response-shape and localization seams below are smaller boundary issues, not a reason to replace the overall approach.

Confirmed findings

P2 — major/blocking — cancellation drops a definitive refused upload outcome (frontend/src/lib/upload-retry.ts:106)

After a retriable 503, withUploadRetry enters backoff and races sleep against cancellation. If connection/reconnect cancellation rejects first, the typed UploadRequestError with outcome=refused is lost; the outer race in frontend/src/hooks/use-websocket.ts:1290-1293 then reaches the catch as MessageDeliveryError(not_sent), which is remapped during uploadPhase to outcome_unknown and requiresReconciliation=true. The form consequently disables Submit even though this 503 is explicitly refused after server rollback. This is distinct from the 7dd fix for cancellation while bytes are still moving. Preserve the last definitive upload outcome when cancellation interrupts backoff (or propagate a typed cancellation carrying that outcome) so a safe refusal remains retryable; changing only the form guard would leave the classification bug in place.

P4 — minor (severity-adjusted) — malformed batch success can send chat without attachments (frontend/src/hooks/use-websocket.ts:1277)

A 200 response with success=false, a non-array files value, invalid entries, or fewer valid entries than files sent is converted to an empty or short list at lines 1277-1288; messageData.files is still sent at line 1306, while the stamping guard only protects the caller's File objects. The normal backend contract is success=true with an exact all-or-none list, so this is an uncommon malformed/proxy response, and the parser predates this PR. It remains an in-scope minor because this PR's retry/stamping flow still permits the silent omission. Reject the response with a typed upload error before message delivery unless success is true and the validated list has exactly filesToUpload.length entries with valid IDs.

P5 — minor — backend rejection detail bypasses localization (frontend/src/hooks/use-websocket.ts:922)

Any non-empty data.message sets userFacing=true, and ClarificationForm renders that detail as-is while only the hint is translated. A backend English rejection therefore appears untranslated in a Chinese form. Keep known backend reasons mapped/localized, or only mark already-localized messages as user-facing and use the localized fallback for unknown text. This is a presentation/localization finding only, distinct from the raw-error/security producer surface already tracked in OPEN #1497.

P9 — minor — outcome-unknown hint conflicts with safe retry state (frontend/src/components/chat/clarification-form.tsx:338)

The outcome_unknown branch always chooses copy saying the response may already be submitted and telling the user to reload before submitting again. Since 7dd, unknown delivery with requiresReconciliation=false intentionally keeps the button enabled and retries the same clientMessageId; only upload reconciliation should block/reload. Split the hint by requiresReconciliation, or use wording that distinguishes safe same-ID retry from an attachment that needs reload/reconciliation.

Review criteria / limitations

• Correctness, concurrency, and error-lifecycle review confirms the blocking P2 path and the three non-blocking boundary issues above; no additional critical or major root was confirmed.
• The overall API/design shape is acceptable with the reservations stated above; no separate documentation gap was identified for these internal client-state changes.
• Changed frontend tests and their assertions were inspected, but no local tests, build, formatter, or linter were run per assignment. The supplied review context reports CI checks as SUCCESS.
• The supplied context records that the OMP history extractor reached usage_limit_reached; the raw paginated history exports and linked issue evidence were manually checked for the status table. No author response was treated as proof of a fix.

Blocking status & recommended decision

Blocking: yes — recommended event: REQUEST_CHANGES.

Blocking issue:

• frontend/src/lib/upload-retry.ts:106 — major — cancellation can downgrade a known refused/rolled-back 503 upload to outcome_unknown and force an unnecessary reconciliation lock. [new]

Non-blocking minors:

• frontend/src/hooks/use-websocket.ts:1277 — minor — malformed or short batch responses can silently omit attachments before message delivery. [new]
• frontend/src/hooks/use-websocket.ts:922 — minor — backend rejection text can bypass the form's localization. [new]
• frontend/src/components/chat/clarification-form.tsx:338 — minor — the outcome-unknown hint tells users to reload even when same-ID retry is intentionally enabled. [new]

[codeacme17]

The blocking major and all three minors are fixed in 78e37d0; P5 is recorded on #1479. Thank you for confirming the #1497 split — that was the call I most needed.

P2 — and why it happened

Confirmed exactly as traced. The diagnosis matters more than the patch: my previous round generalized "cancelled during the upload phase" into "cancelled while bytes were moving", and those are different windows. Backoff only exists after a 503, and during it nothing is on the wire — so the one window where the outcome was provably safe was the one I marked unknown. It also fires only in this PR's headline scenario, since a 503 burst is the only thing that produces backoff at all.

The send path now tracks whether a request is actually on the wire, separately from the last outcome the server gave. Ownership lost mid-request still yields unknown; ownership lost during backoff keeps the refusal. Done in the caller rather than by changing what withUploadRetry throws, so the classification stays in one place.

P4 — taken with your stricter reading. A batch response that does not account for every file sent is now rejected instead of parsed into a short list, so a turn can no longer go out with its attachments quietly missing.

P9 — also mine from last round: I split the blocking by requiresReconciliation but left the copy keyed on disposition, so a safely repeatable delivery still said "reload first". Reload wording is now reserved for the case that needs it; a new string covers unconfirmed delivery and says what is true — submitting again will not create a second answer.

P5 — recorded on #1479 rather than patched. Both remedies you offer reduce to the same contract: the client cannot distinguish a localized backend message from an unlocalized one, so gating on "already localized" collapses into never showing the reason, which is the bug #1468 exists to fix. I argued there for designing the field as an enum the client maps to i18n keys rather than a boolean, since codes are what let a client both trust and translate a message.

Verification — frontend 138 files / 2242 tests; required test:widget:coverage lane 33 files / 868 tests; tsc and eslint clean (the 11 no-explicit-any in clarification-form.tsx are pre-existing and unchanged in count). No backend files changed this round.

[rogercloud]

This PR hardens the clarification/interaction form's submit path so a transient attachment-upload failure no longer dead-ends a widget visitor. It adds a shared bounded upload retry (frontend/src/lib/upload-retry.ts, 3 attempts, full jitter) that fires only on statuses provably meaning "refused before anything was stored" (503 alone), makes that contract true server-side by only letting store_uploaded_files emit 503 once its compensation actually succeeded (otherwise a 500 "outcome unknown"), turns off apiRequest's blind transport replay for uploads, stamps returned file_ids back onto the caller's File objects so a resubmit does not re-upload bytes, and propagates a disposition / userFacing / requiresReconciliation triple into the form so the visitor sees the real reason plus an accurate "safe to retry" vs "reload first" hint. It additionally introduces a ClientVisibleError marker taxonomy and client_safe_error_message() in src/xagent/web/api/websocket.py, redacting incidental exception text from ~10 client-facing payloads.

Blocking: yes — recommended event: REQUEST_CHANGES

Round 0 — approach verdict

Verdict: acceptable-with-reservations.

The central insight is right, and it is a root-cause fix rather than a symptom patch: instead of retrying blindly, the PR defines "provably refused" as the only replayable class, and then makes the server actually honour that definition. Verifying the 503 producers on /api/files/upload confirms the contract holds — every other 503 on that route (release_db_connection_if_clean in public_chat_access.py, the startup-sync middleware in web/app.py) fires strictly before any storage work. Establishing the invariant on both sides of the wire, rather than trusting a status code, is the strongest part of this change.

Three design-level reservations:

1. Scope. The websocket redaction taxonomy (ClientVisibleError + subclasses, ~10 converted call sites, a 297-line test) is a separate security-hardening concern that issue #1468 does not ask for, and it ships self-documented as incomplete (client_safe_error_message's own docstring points at #1497). It roughly doubles the review surface, and it actively interferes with the PR's other half (see the localization note under the C27 discussion below). This belonged in its own PR.

2. The outcome is reconstructed rather than returned. sendChatMessage now carries three mutable locals — uploadPhase, uploadRequestInFlight, lastUploadOutcome (frontend/src/hooks/use-websocket.ts:1201-1209) — and rebuilds the upload outcome from them inside a catch 230 lines later (:1435-1444). Both new major findings below live in exactly that reconstruction. Having the upload step return a typed result ({ ok } | { refused } | { unknown }) instead of leaving breadcrumbs for a distant catch to interpret would make the classification total by construction, and would have made N1 unrepresentable.

3. userFacing is inferred, not signalled. It is derived from "the server sent a non-empty string" (:922). That makes it structurally impossible to separate actionable rejection text from a mechanical retry diagnostic or from the new redacted generic. This is already tracked on #1479, and the tracking is accepted — but the backend half added in this PR makes the case for the enum stronger, not weaker (see below).

Line-level findings

N1 — major — frontend/src/hooks/use-websocket.ts:1444 — a stale lastUploadOutcome reports a genuinely unknown upload as safe to resubmit

The fix for the previous round's blocking finding introduced lastUploadOutcome so that a reconnect interrupting the backoff preserves the server's definitive 503 refusal. But that value is written once and never cleared, and sendUpload's finally clears uploadRequestInFlight before the error reaches the outer catch. So on any attempt after the first, a raw fetch rejection lands on the stale value:

• attempt 1 → 503 → lastUploadOutcome = "refused" (:1260), uploadRequestInFlight = false (:1264)
• backoff, attempt 2 → the connection drops mid-body → a plain TypeError (no UploadRequestError), uploadRequestInFlight already false again
• :1444 → lastUploadOutcome ?? "unknown" → "refused" → :1459 throws disposition: "not_sent", requiresReconciliation: false
• the form shows sendNotSent: "Your answers were kept — you can submit again."

The request was on the wire when it died, so the batch may have committed; resubmitting re-uploads the same bytes and duplicates the attachments. The code is internally inconsistent about this: the identical dropped fetch on attempt 1 is correctly classified outcome_unknown, and there is a passing test pinning that (frontend/src/hooks/use-websocket.test.ts:82-94), which is good evidence this is an oversight rather than a deliberate call.

Suggested fix — clear the breadcrumb when a new attempt starts, which keeps the prior cancellation-during-backoff behaviour intact (cancellation during backoff still reads the previous attempt's value, because the next attempt never begins):

const sendUpload = async () => {
  lastUploadOutcome = null   // this attempt has said nothing yet
  uploadRequestInFlight = true
  try { return await sendUploadRequest() }
  ...
}

Please add a test for "503, then a dropped fetch on the retry" asserting outcome_unknown + requiresReconciliation: true.

N2 — major — frontend/src/components/chat/clarification-form.tsx:355-357 — clearing the delivery id on retryWithNewId re-opens the duplicate-answer path after an unknown-outcome delivery

deliveryAttemptRef.current = null is unconditional on retryWithNewId, with no regard for whether the previous attempt's outcome was known. That makes this sequence reachable:

1. Submit → the server acks message_rejected with rejection_outcome: "outcome_unknown" (e.g. websocket.py:5786, "The message is still being applied"). The form keeps the id, leaves Submit enabled, and shows sendDeliveryUnconfirmed: "Submitting again is safe — it will not create a second answer."
2. The visitor edits an answer (permitted — handleInputChange only short-circuits when resubmitBlocked) and resubmits under the same id.
3. finish_existing_delivery checks payload_matches first, before failed/pending (websocket.py:5769-5775), so it answers "Message id was already used for different content or files." with retry_with_new_id=True and rejection_outcome: "not_accepted" → disposition "rejected", requiresReconciliation stays false, and :357 clears the id.
4. The next submit mints a fresh id and opens a second turn — on top of a first one that may still be landing.

Note that step 3's rejection only fires when a server-side claim already exists, i.e. precisely when the first attempt did reach the server. So the promise in the sendDeliveryUnconfirmed copy becomes false the moment the visitor takes the edit the form invites. This is a partial regression of the "keep one client ID for the unresolved logical submission" guarantee, through a path that guarantee's original fix does not cover.

Suggested fix: only honour retryWithNewId when the prior attempt was definitively not accepted (disposition === "not_sent" || disposition === "rejected" and the previous outcome was not unknown); when an unknown-outcome delivery is followed by a payload mismatch, treat it as needing reconciliation rather than silently minting a new turn. Secondarily, note that ChatInput.tsx:735-740 already solves the adjacent problem properly by keying the retained id to a deliveryKey derived from the draft contents — the clarification form keys it to nothing, which is what lets an edited draft travel under the old id in the first place.

N3 — minor — frontend/src/hooks/use-websocket.ts:1236-1246 — the injected-uploader branch has no "answered for every file" check

The default batch path now rejects a short or unreadable file list (:1313-1327). The injected uploadFiles branch does not: a shorter array simply skips the uploadedFiles.length === filesToUpload.length stamping guard at :1338 and the turn is delivered with attachments silently missing. Not currently reachable — both injected implementations use Promise.all, which rejects rather than returning a short array — but the asymmetry is a trap for the next transport, and the contract is stated nowhere. Suggest applying the same length assertion to both branches, or documenting the 1:1 requirement on the uploadFiles type in frontend/src/contexts/app-context-chat.tsx:1805.

N4 — minor — src/xagent/web/api/files.py:641-656 — compensated conflates registration rollback with staged-file deletion

compensated is flipped to False by any exception out of _cleanup(), and _cleanup's inner finally also runs _delete_local_paths() (:631-637). So a failure to unlink a staged temp file — which leaves no database row and therefore nothing a retry could duplicate — downgrades a genuinely rolled-back 503 into the _durable_storage_outcome_unknown 500. On the client that 500 now means requiresReconciliation, which disables Submit and tells the visitor to reload. The direction is fail-safe, so this is not blocking, but it is stricter than the invariant requires. Suggest tracking compensation of registrations separately from staged-path cleanup and gating the downgrade on the former only.

N5 — minor (security) — src/xagent/web/api/websocket.py:5090-5092 — the access-denied text is now explicitly blessed as client-visible

ClientVisiblePermissionError(f"Access denied: Task {task_id} does not belong to you") is deliberately marked safe to show, while the sibling raise ValueError(f"Task {task_id} not found") two lines above (:5087) is left unmarked and therefore redacted. The pair is an existence/ownership oracle: a non-owner learns that task N exists and belongs to someone else, while a nonexistent id yields the generic string. Since this PR is where that text becomes an explicit contract, it is the right place to fix it — suggest a fixed "Access denied." for the client with the task id kept in the log, matching the redaction posture of the rest of the change.

N6 — minor (test) — frontend/src/hooks/use-websocket.test.ts:287 — the new backoff-interrupt test races real jitter

sendChatMessage exposes no seam for sleep/random, so this test runs against the production delay Math.round(random() * 400). It only awaits fetch having been called once before act(() => triggerClose(4001)). When the jitter lands low, attempt 2 starts first, uploadRequestInFlight becomes true, and the assertion flips. Suggest threading the existing UploadRetryOptions.sleep/random seams through sendChatMessage (or exposing an injectable retry options bag) so the test can pin the schedule.

N7 — minor (test) — frontend/src/hooks/use-websocket.test.ts:355 — "keeps a pre-upload failure reported as never sent" never reaches the branch it names

The fixture passes autoConnect: false, so there is no socket owner and the throw happens at use-websocket.ts:1142 ("the connection is not ready"), before the try block and before uploadPhase = true. It therefore exercises none of the new classification code and duplicates the pre-existing test immediately below it. Suggest deleting it, or rewriting it to actually reach :1221.

N8 — minor (test) — tests/web/api/test_upload_connection_boundary.py:997 — the new condition is only half covered

The parametrized test genuinely exercises the failed-compensation → 500 downgrade, which is good, but files.py:655 is if durable_storage_refused and not compensated, and there is no case for durable_storage_refused == False and not compensated. Changing the and to or would leave the suite green. The test also asserts only status_code, never the detail strings.

N9 — minor (test) — tests/web/api/test_websocket_client_safe_errors.py — the redaction guarantee rests mostly on the AST scan

Two genuine behavioural tests exist through handle_execute_task, and they do fail on a revert. Beyond that, there is no unit test of client_safe_error_message itself, and ClientVisiblePermissionError is never exercised behaviourally. Eight of the ten converted call sites, including the handle_chat_message message_rejected ack that the frontend's userFacing flag is derived from (websocket.py:5019-5027), are covered by source shape only.

N10 — minor (test) — the three new i18n keys are not usage-checked

t is stubbed as identity in clarification-form.test.tsx, so the component tests assert the key strings. The per-feature usage scan in frontend/src/i18n/translations.test.ts:147-160 is still hard-coded to builds.publication.* and was not extended, so a typo in clarification-form.tsx:345/347/349 would pass every test in the repo.

Prior-findings verification (re-review)

Verified against current head 78e37d0b.

Last review's four items:

• Cancellation during backoff erasing a definitive refused 503 — FIXED via lastUploadOutcome + uploadRequestInFlight, but the fix introduced N1 above.
• Short/malformed batch response delivered without attachments — FIXED (:1313-1327); injected-path asymmetry filed as N3.
• outcome_unknown copy telling the visitor to reload when same-id retry was safe — FIXED (sendDeliveryUnconfirmed split out).
• Backend rejection text bypassing localization — DROPPED / tracked → #1479, with one new observation: the fixed English CLIENT_SAFE_VALIDATION_ERROR combined with the "non-empty string ⇒ userFacing" inference now always pre-empts the localized fallback, reaching zh visitors verbatim — strengthens the case for the enum tracked there.

Earlier rounds re-verified in current code:

• "One client id kept for an unresolved submission" — PARTIAL: the id-reuse half holds but N2 shows the unconditional clear re-opens the second-turn path.
• Ownership loss during in-flight upload → outcome_unknown — FIXED.
• Ordinary reconnect permanently locking the form — FIXED; blocking now keys on requiresReconciliation only.
• Per-submission guards leaking across clarification rounds — FIXED.
• Eleven other previously-raised items — FIXED, unchanged since verification (retriable-status set, transport replay disabled for uploads, per-file id stamping, full jitter).
• One item — DROPPED (author's rebuttal independently verified).
• Several items tracked on #1471 / #1485 / #1497 — scope split accepted, not re-raised.
• One item retired by the requiresReconciliation split.
• One design note (the compensated-gated 500 branch being near-unreachable in production since the durable write precedes db.commit()) — NOT FIXED, no reply on record, no tracking issue. Non-blocking, but the docstring should say so if the branch is only reachable under test monkeypatching. N4 above is the adjacent over-trigger on the same flag.

Simplification opportunities

L1201-1209 use-websocket.ts: shrink: three mutable locals (uploadPhase / uploadRequestInFlight / lastUploadOutcome) reconstruct the upload outcome in a catch 230 lines away. Return a typed {ok|refused|unknown} result from the upload step and delete all three.
L1221 use-websocket.ts: delete: `if (!currentTaskId)` is unreachable — chatTaskIdMode is only "required" | "omit" and both throw earlier for a file-carrying send. Drop the guard.
L95-105 use-websocket.ts: shrink: deliveryError()/MessageDeliveryError now take five positionals, three of them booleans defaulting to false, so every call site reads `..., false, false)`. Take an options object.
L1249-1266 use-websocket.ts: shrink: sendUpload wraps sendUploadRequest purely to set two flags; fold into one function.
net: -35 lines possible

Blocking status & recommended decision

Blocking: yes — recommended event: REQUEST_CHANGES

file:line	severity	issue	source
frontend/src/hooks/use-websocket.ts:1444	major	A stale lastUploadOutcome classifies a dropped request on retry attempt ≥2 as refused, telling the visitor a possibly-committed upload is safe to resubmit	[new]
frontend/src/components/chat/clarification-form.tsx:355-357	major	Unconditional retryWithNewId id-clear after an unknown-outcome delivery lets an edited resubmit open a second turn over a possibly-live answer	[new] (partial regression of a prior fix's guarantee)

Both are localized fixes inside code added this round; N3-N10 are non-blocking and can ride along or follow. No test results are claimed here — this was a static read of the diff and the surrounding modules; the test observations above are code-reading findings, not run results.

[codeacme17]

All three of your design reservations acted on, both majors fixed, and the minors either fixed or moved with the split. Commits b75f0d6, 4dcfac1; the security half is now #1514.

Reservation 2 was the diagnosis I needed. "The outcome is reconstructed rather than returned" explains four rounds of this PR, not just this one. Each round I added a flag to discriminate the case the previous round got wrong — uploadPhase, then requiresReconciliation, then uploadRequestInFlight + lastUploadOutcome — and each flag introduced a combination the distant catch did not handle, which became the next round's finding. That is why the new majors kept landing in exactly the code I had just touched.

So the upload step now returns {ok} | {refused, error} | {unknown, error}, decided where the evidence is. The three breadcrumbs are gone from the send scope; the two that survive live in a twenty-line closure beside the retry, and the outer catch is back to rethrowing what it is handed. N1 is unrepresentable rather than fixed: a refusal cannot outlive the attempt that earned it, because each attempt starts having said nothing.

N2 — your step 3 is the part I had missed: the server only asks for a new id once a claim exists under the old one, i.e. exactly when the undecided answer did reach it. An attempt whose fate was undecided now carries that history across retries, and a retryWithNewId there escalates to reconciliation instead of minting a second turn. I did not take the deliveryKey suggestion — for this form it makes the dangerous case worse, since an edit after an undecided delivery would mint a new id with no server round trip to catch it; the reasoning is in that thread.

Reservation 1 — the redaction taxonomy is out. #1514 carries it, with the guard's real scope in its docstring; N5's access-denied oracle went to #1497. What remains on the server here is only the backend half of the retry contract (files.py), which the frontend's "503 means provably refused" depends on.

Reservation 3 — userFacing being inferred rather than signalled stays on #1479, where I have also argued the field should be an enum the client maps to i18n keys rather than a boolean.

Minors — N3 (injected branch now asserts the same 1:1 contract), N4 (the downgrade gates on registration rollback alone, so an unlinkable temp file no longer costs the client its replay), N6 (the backoff test pins the schedule through the new uploadRetry seam instead of racing jitter), N7 (rewritten — it threw at "connection is not ready" and never reached what it named), N8 (detail text pinned, plus the two uncovered branches).

N7 is worth naming: it was one of three tests I added to demonstrate the previous round's fix, and it demonstrated nothing. Alongside N6 racing by construction, that is a large part of why my "verified" claims kept outrunning the evidence.

Verification — frontend 138 files / 2245 tests; required test:widget:coverage lane 33 files / 871 tests; backend tests/web/api green; tsc, eslint, ruff, mypy clean. The two new regression tests were each checked against the unfixed code first.