fix(desktop): show expandable failed-turn diagnostics - #4986
Conversation
1958fb8 to
a88ce6d
Compare
Generated-by: Codex
Generated-by: Codex
168b1cb to
33eaaa9
Compare
jackwener
left a comment
There was a problem hiding this comment.
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 整条拒绝,而不是静默截断;共享投影的白名单始终不拷贝那个私有字符串,同时保留 errorClass 与 retry;在该字段存在之前写下的失败 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.
jackwener
left a comment
There was a problem hiding this comment.
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 状况只陈述状况(timeout、rateLimit、providerCapacity),而需要用户做点什么的状态都保留了指令(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.
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.
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
failureMessagefield 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.tsand UI materialization seams. These files may overlap #4879; its inspected heada1db59a0does 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.2f5f04ea, including 101 checker tests; the affected module's token count decreases.Same main/branch stories at 1280 × 900; top row rate limit, bottom row truncated stream. Details are expanded in AFTER.
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
Tool(s) and scope: Codex traced the failure path, implemented the projection and UI changes, updated regression coverage and performed local/CDP verification.
Checklist
Does this PR entail a change in behavior?