Repository navigation
feat(native-chat): a failed turn's bar says "Failed after N" - #23687
brennanb2025 wants to merge 3 commits into
Conversation
The journal already records the provider's verdict on each turn, but the turn-bar pipeline dropped it: the settled timing carried only a duration, so a turn that ended on an API error read "Worked for N" like any other. Carry the verdict from the turn record through the settled timing and the turn status to both bars. Only `failure` changes the copy; an unknown verdict keeps "Worked for", and an untimed record still renders nothing. The verdict rides the same key as the duration, so a turn anchored to its own record shows it too.
…ar labels are translated
…nb2025/failed-turn-bar
Review statusState: draft, head Review loops: 1 loop, clean, no code changes. The reviewer traced the provider result through every settled-bar path (a turn opened by your message, a turn the provider opened, the older record format, the turn that has just ended), checked reload, SSH and older hosts, and confirmed there are no new wire fields, subscriptions or polling. Removing the result at the timing reader or the transcript row turns 4 tests red. Base update: Tests: desktop vitest 4 files, 76 tests; mobile Desktop QA (macOS): passed. The failure came from a deterministic fake Claude that ends the turn with an API-error result.
Phone QA: blocked. On the test machine the phone could not open a structured Claude chat. Chats created on the desktop are not listed on the phone, and a Claude chat started from the phone opened as a terminal chat, which has no turn bar. That behaviour is outside this PR. The mobile label is covered by the mobile tests above. Design note: other chat interfaces keep "Worked for" on a failed turn and show the failure only in the error rows. Orca changes the label because its sidebar already says the chat failed, and the bar should not contradict it. This is recorded in the PR body. Behaviours a user may find surprising. None of these come from this PR:
Needs a human: native review of the fr, ja, ko and zh strings. Spanish is left out on purpose. Readiness check (end): passes, with no P0, P1 or P2 findings. CI is red on three checks: |
|
Closing in favour of #23467, which now covers everything this PR did and more:
Keeping both would have added the same key and a second route for the same fact. The desktop QA screenshots above still show the before and after of the failed-turn label. |
1 participant
Stacked on #23671 (#23573)
ELI5
When the agent's turn ends in an error, the bar under your message now says "Failed after 12s" instead of "Worked for 12s", so a failed turn no longer looks like a successful one.
What Changed
Before: every finished turn read "Worked for N", including a turn the provider ended with an API error. The sidebar already said the chat had failed, so the same screen said "Failed" in one place and "Worked for 3s" in another, and you had to scroll through the turn to find out which was true.
After: a turn the provider reported as failed reads "Failed after N" on desktop and mobile. Successful turns, turns you stopped, and turns with no recorded result still read "Worked for N". A running turn still reads "Working for N", and a turn with no recorded start or end still shows no bar at all (never "Failed after 0s").
Mechanism: the host already writes the provider's result on each turn record in the journal (
outcome: 'failure'), but the turn-bar pipeline dropped it. The timing reader (structured-agent-session-turn-timing.ts) now reads that result with the existingreadAgentJournalTurnOutcomeand keeps it on the turn's timing, next to the start and end times it already reads from the same record. From there it is passed into the settled turn (NativeChatSettledTurn), then the turn status (NativeChatTurnStatus), anddescribeNativeChatTurnStatuspicks the newfailedAfterlabel when the result isfailure. Desktop and mobile build the bar with the same shared function; desktop translates the key and mobile uses the shared English copy.The result travels under the same key as the turn's duration, so every way a turn is anchored gets it: a turn opened by your message, a turn the provider opened on its own, and a turn whose opening message is outside the loaded history.
Why
failurechanges the copy. A missing result means unknown (an older host, or a turn that never settled), which is neither a success nor a fault, so it keeps the neutral "Worked for". A stop you asked for is not a failure, so it keeps "Worked for" too; there is no separate stopped label today.Differences from the common pattern
Known limits (existing rules, not changed here)
terminal_reason.claude-result-outcome.tsreads a user stop from that field; without it, a stop reported as an error is recorded as a failure, so its bar reads "Failed after N". The error row already shows in that case, so the two agree.New translation keys awaiting native review,
components.native-chat.status.failedAfter: fr "A échoué après {{value0}}", ja "{{value0}} で失敗", ko "{{value0}} 작업 후 실패", zh "工作 {{value0}} 后失败" (en "Failed after {{value0}}"). fr, ja, ko and zh need a native speaker's review. Spanish is left out on purpose: its sibling bar labels ("Worked for", "Working for") are not translated yet, and one Spanish label beside English ones would read worse than a consistently English bar.Linked Issue
N/A (follow-up to the turn grouping work in #23671).
Visual Proof
Desktop, macOS, a structured Claude chat. The failure comes from a deterministic fake Claude that ends the turn with an API-error result (
is_error), which the host records as the provider's failure verdict. The right sidebar is collapsed in every shot.Before (base
bb29232d458): the failed turn reads "Worked for 3s" while the chat is marked failed.After (PR
e28c4316b0c; the later base merge does not touch these files): the same failed turn reads "Failed after 3s".A successful turn in the same chat still reads "Worked for".
A running turn still reads "Working for 12s".
A turn you stop still reads "Worked for 21s".
The failed turn still reads "Failed after" after a renderer reload and after quitting and relaunching the app.
Phone: not captured. On the test machine the phone could not open a structured Claude chat. Chats created on the desktop are not listed on the phone, and a Claude chat started from the phone opened as a terminal chat, which has no turn bar. The mobile bar is covered by
MobileNativeChatTurnStatus.test.tsand the mobile message tests, which render "Failed after N" through the same shared function the desktop uses.Testing
New tests, written first and red on #23671's head: a failed turn reads "Failed after N"; success, cancellation and an unknown result read "Worked for N"; a failed turn reached through a provider key and a failed turn anchored to its own record both read "Failed after N"; an untimed failed record shows no bar. Desktop list and mobile bar render tests cover the new label.
Removing the result at each step (timing reader, settled turn, turn status, transcript row) turns the matching tests red.
Targeted vitest (desktop: 4 files, 76 tests; mobile:
MobileNativeChatTurnStatus.test.tsandMobileNativeChatMessage.test.ts, 31 tests, plus every mobile native-chat test file, 781 tests), oxlint with--deny-warnings, the changed-code quality gate, allverify:localization-*scripts, andtc:node,tc:weband mobile typechecks pass on the current head. Manual desktop QA on macOS covered the before and after, success, running, stopped, reload and relaunch cases shown above. Platforms: the change is platform-neutral shared logic and copy. SSH: the result is read from the host's journal, so remote hosts behave the same.AI Disclosure
Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Backward compatibility: older hosts that record no result keep "Worked for". Mobile ships English and gets the new label through the shared formatter. No new wire fields: the result is already on the turn record both sides receive.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)