Skip to content

test(desktop): story-cover the interrupted tool row after a turn abort - #3951

Merged
Astro-Han merged 3 commits into
apache:mainfrom
liuxiaocs7:liuxiaocs7/tool-activity-failure-states
Aug 27, 2026
Merged

test(desktop): story-cover the interrupted tool row after a turn abort#3951
Astro-Han merged 3 commits into
apache:mainfrom
liuxiaocs7:liuxiaocs7/tool-activity-failure-states

Conversation

@liuxiaocs7

@liuxiaocs7 liuxiaocs7 commented Aug 26, 2026

Copy link
Copy Markdown
Member

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 (in Product/Shell Official AppShell, beside RunningStatusDuringToolRun) feeds the real ledger — user → running turn_state → assistant step → Bash tool_call → cancelled terminal result → aborted turn_state — through ChatView → materializeTurns → ToolTrow. The interrupted row is derived by toolResultActivityStatus from the cancelled result (not asserted), and renders inside the production .maka-turn frame. The turn is aborted with partial output retained, and the session status/timestamp are aligned to the transcript so the sidebar row agrees with it. running / completed / errored already have coverage, so this adds only the genuinely missing row.

Refs #3944, #3893

Verification

  • apps/desktop typecheck:stories + packages/ui typecheck pass; Biome and git diff --check clean. Re-verified after merging latest main into the branch (deps rebuilt, smoke re-run).
  • Storybook build + render smoke: 196 stories, no runtime/console/page errors, per-story AX-tree audit clean. Story id product-shell-official-appshell--interrupted-tool-after-turn-abort.
  • Reachable, not just parseable: npm test runs for minutes (build:test then the runner), and the fixture cancels at ~16s while still in the build phase — so it settles cancelled/130 (not timed_out/124, which needs the 120s foreground default, and not a finished run).
  • Faithful bytes: the retained stdout is a verbatim prefix of a real npm test run killed mid-build — leading/trailing newlines included, cut before the test runner starts so there is no stdio: 'inherit' interleaving, and small enough that nothing is truncated (stdoutTruncated: false).
  • The cancelled terminal result passes the canonical decoder: decodeCanonicalShellToolResultContent(...) → { state: 'valid' } (which requires exitCode: 130).
  • materializeTurns on 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 fabricated displayName/intent.

Screenshots (collapsed row + expanded terminal panel, in the AppShell frame):

interrupted-collapsed interrupted-expanded

AI use

Select exactly one:

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

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 Code trailer.

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

@liuxiaocs7
liuxiaocs7 force-pushed the liuxiaocs7/tool-activity-failure-states branch from 00f6086 to 31a88ad Compare August 26, 2026 18:02
@liuxiaocs7 liuxiaocs7 changed the title test(desktop): story-cover the tool-activity in-flight and interrupted rows test(desktop): story-cover the interrupted tool row after a turn abort Aug 26, 2026
@Astro-Han

Copy link
Copy Markdown
Contributor

I reviewed this PR at exact head 31a88ad481cbff2a203ce451e3dd2baaba8b1eeb (base 2d10b52, merge-base a6866d05, 1 file +54−0).

Spec: NO-GO — 1×P2 + 1×P3

  • P2 — unreachable Session/transcript timestamp combination. The story adds a visible user message at NOW - 3m (apps/desktop/stories/app-shell.stories.tsx:119-121,592-595) but overrides only Session status, inheriting the active-row lastMessageAt = NOW - 14m. In production the summary projection takes the max of the header and visible-message timestamps (packages/storage/src/session-store.ts:1381-1395; packages/ui/src/session-history-list.tsx:466-471,839), so that transcript must show and sort as 3m; the story visibly shows and sorts a stale 14m row. That makes test(desktop): enumerate failure and edge states for high-traffic surfaces #3944’s new stable audit target a state a user cannot reach, contrary to FIDELITY.md:24-39,74-76. Derive or override coherent Session metadata from the same fixture.
  • P3 — current AI-SDK call is stored in legacy shape. The resultless tool_call at app-shell.stories.tsx:605-613 omits stepId, but the current stop-button path stamps every tool start with currentStepMessageId and preserves it (packages/runtime/src/ai-sdk-backend.ts:2438-2450; packages/runtime/src/runtime-event-read-model.ts:799-803). Legacy fallback makes the row render, but the fixture is not canonical; add the generated step identity even though no assistant row settles.

Positives: the final ordered turn_state correctly derives aborted; the absent result correctly derives tool interrupted; the real path reaches ChatView → materializeTurns → TurnView → ToolTrow inside .maka-turn; no other story supplies the interrupted state, so the one-surface scope is not a duplicate.

Standards: 3×P3

  • PR body replaces the required template and omits its explicit selections/checklist (CONTRIBUTING.md:81, template 25-46).
  • Branch liuxiaocs7/tool-activity-failure-states does not follow <type>/<description> (CONTRIBUTING.md:83); the title itself conforms.
  • The new Product UI surface has no before/after screenshot or recording (CONTRIBUTING.md:85, template 13-15); render-smoke prose is not visual evidence.

Other checks: git diff --check clean; the CI planner expects only test (code=true, storybook=true); exact-head run 32997486706 / job 98270422690 is terminal SUCCESS. The hosted smoke explicitly rendered product-shell-official-appshell--interrupted-tool-after-turn-abort and passed all 196 stories with per-story AX-tree audit. Post-seal the PR is OPEN / non-draft / MERGEABLE / BLOCKED / REVIEW_REQUIRED, exact head unchanged, 0 reviews/comments.

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.


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

@github-actions github-actions Bot added the effort/S Under 100 readable lines label Aug 27, 2026

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 runningaborted 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:

  • displayName is persisted only from the static tool definition (tool-runtime.ts:1182, ...(tool.displayName ? … )), and buildExecutorBashTool (builtin-tools.ts:624-640) declares none. A Bash row's name in production is literally Bashdisplay-name.ts:41-49 falls through item.displayName to item.toolName.
  • intent comes only from describeToolIntent (tool-runtime.ts:3347-3356), which returns undefined unless categoryHint === 'subagent' and the name is ExploreAgent, and even then always prefixes 只读探索:. A Bash call cannot have one.
  • Bash declares activityKind: 'command' (builtin-tools.ts:626) and tool-runtime.ts:1181 persists 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_resultendTurn 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:170statusOverviewItems,是没有任何 story 引用的死导出。构造方式在最关键的一点上也是对的:状态是推导出来的,不是断言出来的。unfinishedToolActivityStatuspackages/core/src/tool-result-status.ts:55)对非 running 的 turn 给出 interruptedmaterialize.ts:227 仅在没有匹配的 tool_result 时套用,deriveTurnRecords 取最后一条 turn_state,因此 runningaborted 正确落定。它也是确定性的——一切都挂在模块级 NOW 上,渲染路径里唯一的 Date.now()TurnRunningStatus 内,而本 story 未启用它。

Approve 前有一处要改。

P2 —— tool_call fixture 设置了 Bash 调用永远不可能携带的两个字段,却丢掉了它必然携带的那个。

每一条我都按源码核实而非推断:

  • displayName 只从静态工具定义持久化(tool-runtime.ts:1182...(tool.displayName ? … )),而 buildExecutorBashToolbuiltin-tools.ts:624-640)没有声明。生产中 Bash 行的名字就是 Bash——display-name.ts:41-49item.displayName 落到 item.toolName
  • intent 只来自 describeToolIntenttool-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 行都长这样。

两种修法都很小——去掉 displayNameintent、补上 activityKind: 'command';或者保留这套文案,把 fixture 改成 ExploreAgent 调用,它确实有真实的 displayName 和真正推导出来的 intent。

P3 —— // Real path: 注释指名停止按钮路径,但该路径可能并不产生这份记录。 停止时,执行器的中止处理看起来会提交持久结果并追加 tool_result——endTurn 会 await this.activeToolSettlementstool-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 早先修订版被要求移除的那种形态。值得删掉,以免日后被当作既有先例引用。

@liuxiaocs7
liuxiaocs7 force-pushed the liuxiaocs7/tool-activity-failure-states branch 2 times, most recently from a0c7006 to 3805030 Compare August 27, 2026 04:59
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
@liuxiaocs7
liuxiaocs7 force-pushed the liuxiaocs7/tool-activity-failure-states branch from 3805030 to 47f7730 Compare August 27, 2026 08:32
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
@liuxiaocs7
liuxiaocs7 force-pushed the liuxiaocs7/tool-activity-failure-states branch from bcd43b8 to 804c4b5 Compare August 27, 2026 10:49
@liuxiaocs7
liuxiaocs7 requested a review from Astro-Han August 27, 2026 13:30

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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_000 matches 114−98 exactly, and the explicit lastMessageAt: NOW - 98_000 agrees with what toSummary's maxTimestamp(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 the tool_call, matching the current ai-sdk-backend path.

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.

@liuxiaocs7
liuxiaocs7 requested a review from Astro-Han August 27, 2026 16:09
@liuxiaocs7

Copy link
Copy Markdown
Member Author

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_000 matches 114−98 exactly, and the explicit lastMessageAt: NOW - 98_000 agrees with what toSummary's maxTimestamp(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 the tool_call, matching the current ai-sdk-backend path.

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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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-120s to running -118s to assistant -116s to tool_call -114s to result/abort -98s), durationMs: 16_000 matches 114-98 exactly, and lastMessageAt: NOW - 98_000 agrees with what toSummary's maxTimestamp(header, latestVisibleMessageAt) derives (session-store.ts:1387-1389).
  • Step identity. stepId: 'msg-assistant-i' is stamped on the tool_call, matching the current ai-sdk-backend path.

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.

@Astro-Han
Astro-Han merged commit 4e16782 into apache:main Aug 27, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/S Under 100 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants