Skip to content

fix(desktop): show expandable failed-turn diagnostics - #4986

Merged
Astro-Han merged 7 commits into
mainfrom
fix/4502-turn-failure-details
Sep 7, 2026
Merged

fix(desktop): show expandable failed-turn diagnostics#4986
Astro-Han merged 7 commits into
mainfrom
fix/4502-turn-failure-details

Conversation

@Astro-Han

@Astro-Han Astro-Han commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Summary

Failed turns show the failure category but hide the bounded diagnostic already stored on their terminal RuntimeEvent. This makes a rate limit's provider code, message, status and request ID impossible to inspect. The same banner can also recommend resending while the recorded retry decision warns against repeating tool activity.

Fixes #4502. Builds on #4951.

  • Carry the existing terminal error text through the owner Turn projection, bounded to the existing 2 KiB limit without redacting diagnostic text. No new storage or error classifier. Shared transcript projections continue to omit the diagnostic.
  • Reuse Astryx Banner's default collapsed disclosure to show the available text, with wrapping for long diagnostics. Missing details are stated without an empty disclosure. Do not infer additional provider fields from the text.
  • Keep the recorded retry reason and existing Host-controlled continuation action visible. Remove unconditional resend advice from provider failure summaries. A sandbox tool failure cannot hide a terminal diagnostic, and a completed turn's tool error does not become a Turn failure.

Diagnostic text policy

Preserve failure diagnostic text without secret-pattern replacement throughout provider normalization, Codex OAuth error wrapping, caught-failure recording, and Turn projection. The owner explicitly chose unredacted failure diagnostics; byte budgets, error classification, retry decisions and shared-view field exclusion remain in force. Generic trace/log redaction and tool-output handling are outside this change.

Disclosure motion

Keep Astryx Banner's built-in chevron rotation and immediate expand/collapse. Newly mounted diagnostic text fades in using the existing fast duration (125ms) and standard easing tokens; reduced motion disables the fade. There is no height transition or delayed unmount.

Height animation repeatedly lays out the transcript and triggers ResizeObserver-driven scroll following. In a local Chromium comparison using the actual chat shell and scroll authority (10 toggles per case), a 220ms height transition increased cumulative layout time per toggle from 4.6ms to 54ms with 1000 turns. A settled repeat under 4x CPU throttling increased the maximum frame interval from 41ms to 67ms. These are local synthetic measurements, not a device-wide performance guarantee. This follows Astryx's motion guidance to favor opacity/transform over layout animation.

Compatibility and overlap

Host compatibility epoch advances from 129 to 130 for the optional failureMessage field in the Turn contribution codec. Historical records without details remain readable. Retry budgets, recovery conditions, permissions and execution actions are unchanged.

The first commit only forwards the diagnostic through the existing runtime-event-read-model.ts, session.ts, session-turns.ts and UI materialization seams. These files may overlap #4879; its inspected head a1db59a0 does not add this field. This PR does not migrate transcript storage or change its read path. Rebase the later merge across this optional field.

Verification

Rebased onto c58c48182 (including #4879). Preserve the RuntimeEvent-backed transcript and the removal of derived Turn fields; compatibility epoch advances from 129 to 130. Core, Storage, Runtime, Host, UI and desktop main builds pass; 633 focused runtime, protocol, paging, materialization and presentation tests pass. Storybook typecheck and the renderer architecture check against the new base (101 checker tests) also pass.

  • 142 focused Runtime, Host, Desktop and UI tests pass. Coverage includes terminal projection and serialization round-trip decoding, UTF-8 bounds, shared bootstrap/paging exclusion, incremental view updates, tool/Turn separation and existing continuation actions.
  • The terminal projection test fails on main because the diagnostic is absent. Removing UI materialization also makes its refresh regression fail; restoring it passes. Reused Banner disclosure and existing test seams; no new interaction abstraction or retry path.
  • Core, Storage, Runtime, Host and UI builds; Desktop main/renderer builds and all Desktop typechecks; format, lint, ASF headers and Astryx inventory pass. The renderer architecture check passes against 2f5f04ea, including 101 checker tests; the affected module's token count decreases.
  • 28 Chromium renderer checks via CDP cover light/dark at 1280 and 720 widths: category failures, tool failures, long text, missing details, partial output, live arrival and restart continuation. Checked DOM, computed styles, geometry, focus and console. Reload returns the same diagnostic behind a collapsed disclosure. No real provider request or native process-restart exercise was run.

Same main/branch stories at 1280 × 900; top row rate limit, bottom row truncated stream. Details are expanded in AFTER.

Light comparison

Dark comparison

Additional motion validation: Storybook production build passes. Eight Chromium expand/collapse cycles across light/dark and normal/reduced motion verify the 125ms opacity-only animation, immediate final detail height, immediate removal, repeat opening, and no page errors. Reduced motion has no active detail animation. This CSS-only follow-up adds no state, interaction abstraction, or test-only production path.

Latest follow-up validation: Runtime and Desktop main builds pass; 377 focused Runtime tests and 8 presentation tests pass. Existing provider, OAuth and terminal projection tests now check preservation of diagnostic text. Two small regressions cover caught failures and UTF-8 projection bounds: restoring terminal redaction or removing the projection byte cap makes the respective test fail. Replaced the copy keyword blacklist with explicit cause/refusal presentation assertions; no keyword expansion or duplicate interaction framework. Changed-file lint and diff checks pass. Serialization round-trip coverage is not a native restart test.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Codex traced the failure path, implemented the projection and UI changes, updated regression coverage and performed local/CDP verification.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 7, 2026
@Astro-Han
Astro-Han force-pushed the fix/4502-turn-failure-details branch from 1958fb8 to a88ce6d Compare September 7, 2026 15:34
@Astro-Han
Astro-Han marked this pull request as ready for review September 7, 2026 16:07
@Astro-Han
Astro-Han force-pushed the fix/4502-turn-failure-details branch from 168b1cb to 33eaaa9 Compare September 7, 2026 16:35

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at exact head 33eaaa9d2, by two independent reviewers across the diagnostic chain and the trust boundary. The [P1] raised against the earlier heads is resolved here — verified on this code rather than assumed from the commit titles. test was still running at the time of writing and is not treated as evidence in either direction, so this is a comment rather than an approval.

What changed matters more than that it changed. The diagnostic now reaches the transcript because the second projection that was dropping it no longer exists, not merely because a field was added in one more place. That is filed inline.

On redaction: taken as settled and out of scope. Removing it from the failure-diagnostic chain while keeping the byte limit is recorded here as a product decision, and no argument from the earlier heads about redaction gaps is carried forward. Worth separating clearly, since the two were adjacent: the [P1] was never a redaction finding — it was about a field not being written at all, which is independent of what the written value would have contained.

Verified and sound on this head: the diagnostic source is the terminal RuntimeEvent's error text, bounded to the same byte budget, with the decoder refusing an oversized turn_state outright rather than silently truncating; the shared projection's allowlist still never copies the private string while keeping errorClass and retry; a failed turn_state written before the field existed still decodes, since it is optional in the exact shape; and the retry description reads the decision from the ledger rather than recomputing one.

One pre-existing exposure, deliberately not charged to this PR. A guest's subscription snapshot still carries rootTurn.failureMessage, which the projector turns into an error event that reaches a toast — the shared transcript omits the field, but this path does not. The field predates this branch, so it is not a regression here. It is still worth resolving, and this PR is the reason: by removing the message from one guest-facing path you have made a judgement about what a guest should see, and the other path does not yet follow it.

Still open: the capacity-copy question from the earlier head — the guidance was shortened and a leftover assertion expected the longer text. Whether that is settled on this head depends on the run in progress.

简体中文

在 exact head 33eaaa9d2 上,由两位独立评审分别覆盖诊断链路与信任边界。针对先前几个 head 提出的那条 [P1],在这里已经解决 —— 这是在本代码上验证的,不是从提交标题推断的。 撰写时 test 仍在运行,两个方向上都未作为证据,所以本次是评论而非批准。

「改了什么」比「改了」更重要。 诊断之所以能到达 transcript,是因为那份会丢掉它的第二投影已经不存在了,而不仅仅是因为又在一个地方补了字段。这条已发在行内。

关于脱敏:视为已定、不在范围内。 把它从失败诊断链路中移除、同时保留字节上限,在此记录为产品决定,先前几个 head 上任何关于脱敏缺口的论证都不带入有必要清楚地分开,因为两者相邻:那条 [P1] 从来不是脱敏 finding —— 它说的是字段根本没被写入,这与写入的值原本会包含什么无关。

本 head 上核过且成立的: 诊断来源是终态 RuntimeEvent 的错误文本,受同一字节预算约束,而解码器对超限的 turn_state 整条拒绝,而不是静默截断;共享投影的白名单始终不拷贝那个私有字符串,同时保留 errorClassretry;在该字段存在之前写下的失败 turn_state 仍可解码,因为它在精确形状中是可选的;而重试描述读的是账本里的那个决定,不是重新算一个。

一处既有暴露,刻意不算在本 PR 账上。 guest 的订阅快照仍然携带 rootTurn.failureMessage,projector 会把它变成一个到达 toast 的 error 事件 —— 共享 transcript 省略了该字段,而这条路径没有。 该字段早于本分支,所以在这里不是回归。 但它仍然值得解决,而理由正是本 PR:你在一条面向 guest 的路径上移除了该消息,就等于对「guest 应当看到什么」作出了判断,而另一条路径尚未遵循它。

仍然开着: 先前 head 上的容量文案问题 —— 指引被缩短,而一条遗留断言期待的是较长的文本。它在本 head 上是否已了结,取决于正在跑的那次运行。


Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

Comment thread packages/runtime/src/runtime-event-read-model.ts

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at exact head 33eaaa9d2. test is terminal green here, the branch is out of draft, and the [P1] raised against the earlier heads is resolved on this code — verified here rather than inferred from the commits, and explained in my inline comment.

On the shortened capacity guidance, which I had flagged: reading the whole block rather than the one key changes the answer. The terse form is uniform and principled — transient provider conditions state the condition (timeout, rateLimit, providerCapacity), while the states that need something from the user keep their instruction (stepCap says to send a message, tool says to read the result first, permission says it will ask again). Capacity did not lose guidance against a pattern; it joined one. The leftover assertion that expected the older wording was the real defect, and it is gone.

Two things remain, neither blocking, both already inline or noted:

The failureMessage name still means two different things inside agent-run.ts — a private run-level field and the transcript field. That collision cost a round in this review, because the private one was changed while the transcript one was still unpopulated. A rename or a comment would end it.

A guest's subscription snapshot still carries rootTurn.failureMessage where the shared transcript omits it. That field predates this branch, so it is not this PR's regression — but this PR is the reason to settle it: removing the message from one guest-facing path is a decision about what a guest should see, and the other path has not yet followed.

What makes the fix durable is worth restating, since it will matter to whoever reads this later: the diagnostic reaches the transcript because the second projection that was dropping it no longer exists, not only because a field was added. One authority is what keeps the two from disagreeing again.

This approval binds to this exact head. It is not the independent human review CONTRIBUTING.md requires, and it is not a merge decision.

简体中文

在 exact head 33eaaa9d2 上批准。test 在此已终态转绿,分支已非 draft,而针对先前几个 head 提出的那条 [P1] 在本代码上已解决 —— 是在这里验证的,不是从提交推断的,缘由写在我的行内评论里。

关于我曾提出的「容量指引被缩短」:读整块而不是读那一个键,答案就变了。 这种简短形式是统一且有原则的 —— 瞬时性的 provider 状况只陈述状况(timeoutrateLimitproviderCapacity),而需要用户做点什么的状态都保留了指令(stepCap 说发消息让它继续,tool 说先看工具结果,permission 说会再问一次)。容量并不是逆着某个模式丢掉了指引;它是加入了一个模式。 真正的缺陷是那条期待旧措辞的遗留断言,而它已经不在了。

还剩两件,都不阻塞,且都已在行内或已记录:

failureMessage 这个名字在 agent-run.ts 内部仍然指两件不同的东西 —— 一个 run 级私有字段,和那个 transcript 字段。这次冲突在本轮评审中实实在在花掉了一轮,因为被改动的是私有那个,而 transcript 那个当时仍是空的。改名或加一行注释就能了结。

guest 的订阅快照仍携带 rootTurn.failureMessage,而共享 transcript 省略了它。该字段早于本分支,所以不是本 PR 的回归 —— 但值得了结它的理由正是本 PR:在一条面向 guest 的路径上移除该消息,本身就是对「guest 应当看到什么」的判断,而另一条路径尚未跟上。

这个修复为什么是持久的,值得再说一遍,因为它对日后读到的人有用:诊断能到达 transcript,是因为那份会丢掉它的第二投影不再存在,而不只是因为补了一个字段。让两者不再分歧的,是「只有一个权威」这件事。

本批准绑定该 exact head。它不是 CONTRIBUTING.md 所要求的独立人类审查,也不是合并决定。


Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

@Astro-Han
Astro-Han merged commit 00f2c9e into main Sep 7, 2026
1 check passed
@Astro-Han
Astro-Han deleted the fix/4502-turn-failure-details branch September 7, 2026 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Failed turns hide the provider's own response; show it collapsed, expandable, for every failure class

2 participants