test(desktop): story-cover the interrupted tool row after a turn abort - #3951
Conversation
00f6086 to
31a88ad
Compare
|
I reviewed this PR at exact head Spec: NO-GO — 1×P2 + 1×P3
Positives: the final ordered Standards: 3×P3
Other checks: What I did not check: full local suite beyond the checks noted. Gate: exact head has P2/P3 findings; it cannot be approved or merged until the Session timestamp coherence, step identity, template/branch, and visual-evidence issues are fixed.
|
Astro-Han
left a comment
There was a problem hiding this comment.
Adds one story, InterruptedToolAfterTurnAbort, to apps/desktop/stories/app-shell.stories.tsx, rendering a tool row left interrupted by a turn abort. No production code touched.
The gap is real and the story is worth having. It is the only surface that renders an interrupted row together with the abortedByStop marker inside the real .maka-turn frame, and nothing else covers those pixels — the one other interrupted fixture, statusOverviewItems in packages/ui/stories/tool-activity.fixtures.ts:170, is a dead export that no story imports. The construction is also right in the way that matters most: the status is derived, not asserted. unfinishedToolActivityStatus (packages/core/src/tool-result-status.ts:55) yields interrupted for a non-running turn, materialize.ts:227 applies it only when no tool_result matches, and deriveTurnRecords takes the last turn_state, so running → aborted settles correctly. It is deterministic — everything hangs off the module-level NOW, and the one Date.now() in the render path is inside TurnRunningStatus, which this story does not enable.
One thing to fix before I approve.
P2 — the tool_call fixture sets two fields a Bash call can never carry, and drops the one it always carries.
I verified each against source rather than inferring:
displayNameis persisted only from the static tool definition (tool-runtime.ts:1182,...(tool.displayName ? … )), andbuildExecutorBashTool(builtin-tools.ts:624-640) declares none. A Bash row's name in production is literallyBash—display-name.ts:41-49falls throughitem.displayNametoitem.toolName.intentcomes only fromdescribeToolIntent(tool-runtime.ts:3347-3356), which returnsundefinedunlesscategoryHint === 'subagent'and the name isExploreAgent, and even then always prefixes只读探索:. A Bash call cannot have one.- Bash declares
activityKind: 'command'(builtin-tools.ts:626) andtool-runtime.ts:1181persists it unconditionally. The fixture omits it.
So the story renders a row titled 运行完整回归 with a target line 无缓存重跑整套回归用例 (tool-activity.tsx:494,496). Production renders Bash, with no target line, and the command only inside the expanded invocation line. That is a different row shape — two lines versus one. Anyone doing pixel or geometry work against this story measures a layout Bash cannot produce, a future regression in the no-target single-line row goes uncaught, and a reviewer "confirms" a title the app never shows. Reachability class 1: this is what every stopped Bash row looks like.
The fix is small either way — drop displayName and intent, add activityKind: 'command'; or keep the phrasing and make the fixture an ExploreAgent call, which does have a real displayName and a genuinely derived intent.
P3 — the // Real path: annotation names the stop-button path, but that path may not be the one that produces this transcript. On stop, the executor's abort handling appears to commit a durable outcome and append a tool_result — endTurn awaits this.activeToolSettlements (tool-runtime.ts:623-630, 768-772) specifically so those outcomes land before the terminal fact. If that holds, a normal stop persists tool_call + tool_result(cancelled), still interrupted but via toolResultActivityStatus, and the row then also carries durationMs and a retained output panel — neither of which this story shows. The resultless shape pinned here would then be the fault/recovery case: a process kill or a missed commit, which is what materialize.test.ts:466 covers.
I am flagging this as P3 rather than P2 because I did not exhaustively enumerate the abort branches, and a narrow resultless sub-case on stop may well exist. Please check which path actually yields a resultless tool_call and name that one in the annotation. The state itself is legitimate either way; it is the sentence that may be wrong.
Ungraded, outside this PR's scope but worth someone's time: tool-activity.fixtures.ts carries three dead exports — statusOverviewItems (:133), terminalAndLiveOutputItems (:180), fileDiffAndWebSearchItems (:333) — imported by nothing. The first hand-asserts status: 'interrupted' with a plain-text result, i.e. exactly the shape an earlier revision of this PR was asked to remove. Worth deleting so it cannot be cited later as existing precedent.
AI use: Claude Code (Opus) produced the initial findings, including tracing the derivation chain and confirming the story is not duplicated by any existing story or E2E. I independently re-verified the P2 against source — the Bash tool definition, describeToolIntent's guard, and the displayName/intent/activityKind persistence at tool-runtime.ts:1178-1192. The story was not rendered. The reviewer of record reviewed and accepted this.
简体中文
在 apps/desktop/stories/app-shell.stories.tsx 新增一个 story InterruptedToolAfterTurnAbort,渲染被 turn abort 留在 interrupted 状态的工具行。未触及生产代码。
这个缺口是真实的,story 值得要。它是唯一在真实 .maka-turn 框架内同时渲染 interrupted 行与 abortedByStop 标记的界面,这些像素别处没有覆盖——另一个 interrupted fixture,packages/ui/stories/tool-activity.fixtures.ts:170 的 statusOverviewItems,是没有任何 story 引用的死导出。构造方式在最关键的一点上也是对的:状态是推导出来的,不是断言出来的。unfinishedToolActivityStatus(packages/core/src/tool-result-status.ts:55)对非 running 的 turn 给出 interrupted,materialize.ts:227 仅在没有匹配的 tool_result 时套用,deriveTurnRecords 取最后一条 turn_state,因此 running → aborted 正确落定。它也是确定性的——一切都挂在模块级 NOW 上,渲染路径里唯一的 Date.now() 在 TurnRunningStatus 内,而本 story 未启用它。
Approve 前有一处要改。
P2 —— tool_call fixture 设置了 Bash 调用永远不可能携带的两个字段,却丢掉了它必然携带的那个。
每一条我都按源码核实而非推断:
displayName只从静态工具定义持久化(tool-runtime.ts:1182,...(tool.displayName ? … )),而buildExecutorBashTool(builtin-tools.ts:624-640)没有声明。生产中 Bash 行的名字就是Bash——display-name.ts:41-49从item.displayName落到item.toolName。intent只来自describeToolIntent(tool-runtime.ts:3347-3356),除非categoryHint === 'subagent'且名称为ExploreAgent,否则返回undefined,且即便如此也总带只读探索:前缀。Bash 调用不可能有。- Bash 声明了
activityKind: 'command'(builtin-tools.ts:626),tool-runtime.ts:1181无条件持久化它。fixture 把它漏了。
于是 story 渲染出一行标题为"运行完整回归"、目标行为"无缓存重跑整套回归用例"的记录(tool-activity.tsx:494,496)。而生产渲染的是 Bash,没有目标行,命令只出现在展开的调用行里。这是不同的行形态——两行对一行。任何据此 story 做像素或几何工作的人,量的是 Bash 不可能产生的布局;将来无目标单行形态的回归不会被发现;评审者会去"确认"一个应用从不显示的标题。可达类别 1:每一个被停止的 Bash 行都长这样。
两种修法都很小——去掉 displayName 与 intent、补上 activityKind: 'command';或者保留这套文案,把 fixture 改成 ExploreAgent 调用,它确实有真实的 displayName 和真正推导出来的 intent。
P3 —— // Real path: 注释指名停止按钮路径,但该路径可能并不产生这份记录。 停止时,执行器的中止处理看起来会提交持久结果并追加 tool_result——endTurn 会 await this.activeToolSettlements(tool-runtime.ts:623-630, 768-772),正是为了让这些结果先于终态事实落地。若如此,正常停止会持久化 tool_call 加 tool_result(cancelled),仍是 interrupted,但走 toolResultActivityStatus,且该行还会带 durationMs 和保留的输出面板——这两样本 story 都没有。那么此处钉住的"无结果"形态就属于故障/恢复情形:进程被杀或提交丢失,也就是 materialize.test.ts:466 覆盖的那个。
我定 P3 而非 P2,是因为我没有穷举所有中止分支,停止路径上确实可能存在一个狭窄的无结果子情形。请确认究竟哪条路径会产生无结果的 tool_call,并在注释里指名那一条。无论如何这个状态本身是合法的;可能错的是那句话。
未定级、超出本 PR 范围但值得有人处理:tool-activity.fixtures.ts 有三个死导出——statusOverviewItems(:133)、terminalAndLiveOutputItems(:180)、fileDiffAndWebSearchItems(:333),无人引用。其中第一个手工断言 status: 'interrupted' 并配纯文本结果,正是本 PR 早先修订版被要求移除的那种形态。值得删掉,以免日后被当作既有先例引用。
a0c7006 to
3805030
Compare
The interrupted tool state — a tool the user stops mid-run — rendered in no story. Add InterruptedToolAfterTurnAbort beside RunningStatusDuringToolRun: a Bash tool_call whose abort settles as a cancelled `terminal` result (exitCode 130, "Command cancelled"), so `toolResultActivityStatus` derives the row as `interrupted` — there is no interrupted turn status; the state comes from the settled result, and the aborted turn retains that partial output. It projects through the real ChatView -> materializeTurns -> ToolTrow path, rendering inside the production `.maka-turn` frame; the session is aborted with its timestamp aligned to the transcript. The result passes the canonical shell-result decoder, and the ledger carries the fields the AI-SDK backend persists (assistant step + stepId, origin, modelVisibility, durationMs). running/completed/errored already have coverage; this adds only the genuinely missing interrupted row. First surface under apache#3944. Refs apache#3944, apache#3893 Generated-by: Claude Code
3805030 to
47f7730
Compare
Address review of InterruptedToolAfterTurnAbort. A `tsc` typecheck settles in under a second, so a multi-second cancel can't happen; and hand-written command output isn't the bytes the command produces. Use `npm test`, which runs for minutes (build:test then the runner), cancelled at ~16s while still building — genuinely `cancelled`/130, inside the 120s foreground default so it is not `timed_out`/124. The retained stdout is a verbatim prefix of a real `npm test` run killed mid-build, so the expanded panel shows the exact bytes — leading/trailing newlines included, no test-runner interleaving, nothing truncated. It derives through the real ChatView -> materializeTurns -> ToolTrow path and renders in the production `.maka-turn` frame; the session is aborted with its timestamp aligned to the transcript. Verified: shell-result decoder -> valid; materializeTurns -> tool.status interrupted, turn.status aborted; storybook smoke green (196 stories). Refs apache#3944, apache#3893 Generated-by: Claude Code
bcd43b8 to
804c4b5
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed at head 804c4b55. Both prior findings are genuinely fixed — but I am holding the approval on missing visual evidence.
Fixed:
- P2 (timestamp coherence). The transcript is now strictly monotonic (user
NOW-120s→ running-118s→ assistant-116s→ tool_call-114s→ result/abort-98s),durationMs: 16_000matches 114−98 exactly, and the explicitlastMessageAt: NOW - 98_000agrees with whattoSummary'smaxTimestamp(header, latestVisibleMessageAt)derives (session-store.ts:1387-1389). The sidebar row no longer sorts as a stale 14m. - P3 (step identity).
stepId: 'msg-assistant-i'is now stamped on thetool_call, matching the currentai-sdk-backendpath.
I also verified the "verbatim bytes" claim instead of taking it on trust: NPM_TEST_STDOUT_AT_CANCEL matches this repo character-for-character — maka@0.2.0, the test and build:test script bodies including all ten workspace build steps in order, clean-build.mjs, and @maka/core|storage|mcp at 0.1.0 with tsc -p tsconfig.json. It is a real prefix, not an imitation.
Blocking on evidence: the PR body still carries the screenshot placeholder comment with no image attached. This PR's entire deliverable is a visual surface that reviewers are meant to look at — whether the collapsed interrupted row and the expanded terminal panel actually read correctly cannot be judged from the fixture source or from render-smoke prose. Please attach the two captures the body already reserves space for (collapsed row + expanded terminal panel, in the AppShell frame). Same bar applied in #3970 and #3981.
Nothing else is outstanding; once the captures are up I will approve.
AI-assisted review disclosure: Claude Code checked the fixture against the runtime read model and the package scripts; I verified the timestamp derivation, the step identity, and the stdout provenance against current main myself.
Done! |
Astro-Han
left a comment
There was a problem hiding this comment.
Approve at head 804c4b55.
Correcting my previous review: I asked for before/after screenshots, and that was the wrong bar for this PR. That requirement exists for changes to rendering code - #3970 and #3981 altered CSS, where a diff cannot tell a reviewer whether the result looks right. This PR touches no rendering code at all; it adds a data fixture that flows through the existing path. Whether it renders correctly is already established by the Storybook smoke gate (196 stories, per-story AX audit, green), and whether the fixture is faithful I verified by reading source. Screenshots add nothing here that I could not already obtain, and the story itself is the live artifact. Sorry for the detour.
Both findings from my review of 31a88ad4 are fixed, re-verified at this head:
- Timestamp coherence. The transcript is strictly monotonic (user
NOW-120sto running-118sto assistant-116sto tool_call-114sto result/abort-98s),durationMs: 16_000matches 114-98 exactly, andlastMessageAt: NOW - 98_000agrees with whattoSummary'smaxTimestamp(header, latestVisibleMessageAt)derives (session-store.ts:1387-1389). - Step identity.
stepId: 'msg-assistant-i'is stamped on thetool_call, matching the currentai-sdk-backendpath.
I also checked the "verbatim bytes" claim against the repo rather than taking it on trust: NPM_TEST_STDOUT_AT_CANCEL matches character-for-character - maka@0.2.0, the test and build:test script bodies including all ten workspace build steps in order, clean-build.mjs, and @maka/core|storage|mcp at 0.1.0 with tsc -p tsconfig.json.
One non-blocking cleanup: the two images now in the body are from an earlier local version (they show npm test -- --no-cache and a hand-written running/passing count, which is the output the first review asked you to replace). Since screenshots are not required here, either refresh or drop them - a stale capture in the body will mislead whoever reads this PR later.
AI-assisted review disclosure: Claude Code checked the fixture against the runtime read model and the package scripts; I verified the timestamp derivation, the step identity, and the stdout provenance against current main myself.
Summary
First surface under #3944 (one surface per PR): add the interrupted tool row, a failure state that appeared in no story, so the Storybook gate and the #3893 a11y audit have a real target.
InterruptedToolAfterTurnAbort(inProduct/Shell Official AppShell, besideRunningStatusDuringToolRun) feeds the real ledger —user → running turn_state → assistant step → Bash tool_call → cancelledterminalresult → aborted turn_state— throughChatView → materializeTurns → ToolTrow. Theinterruptedrow is derived bytoolResultActivityStatusfrom the cancelled result (not asserted), and renders inside the production.maka-turnframe. The turn isabortedwith partial output retained, and the session status/timestamp are aligned to the transcript so the sidebar row agrees with it.running/completed/erroredalready have coverage, so this adds only the genuinely missing row.Refs #3944, #3893
Verification
apps/desktoptypecheck:stories+packages/uitypecheck pass; Biome andgit diff --checkclean. Re-verified after merging latestmaininto the branch (deps rebuilt, smoke re-run).product-shell-official-appshell--interrupted-tool-after-turn-abort.npm testruns for minutes (build:test then the runner), and the fixture cancels at ~16s while still in the build phase — so it settlescancelled/130 (nottimed_out/124, which needs the 120s foreground default, and not a finished run).npm testrun killed mid-build — leading/trailing newlines included, cut before the test runner starts so there is nostdio: 'inherit'interleaving, and small enough that nothing is truncated (stdoutTruncated: false).terminalresult passes the canonical decoder:decodeCanonicalShellToolResultContent(...) → { state: 'valid' }(which requiresexitCode: 130).materializeTurnson the sequence yields the state the runtime produces on stop:turn.status = aborted,tool.status = interrupted,tool.result = terminal/cancelled(exitCode 130, "Command cancelled"),activityKind = command,origin = provider, no fabricateddisplayName/intent.Screenshots (collapsed row + expanded terminal panel, in the AppShell frame):
AI use
Select exactly one:
Tool(s) and scope: Claude Code — source/issue investigation and authoring the story. Reviewed and submitted by the human contributor of record; the tip commit carries a git-parseable
Generated-by: Claude Codetrailer.Checklist
Does this PR entail a change in behavior?