Skip to content

feat(desktop): improve tool output rendering and interaction - #5030

Closed
colaforniaw wants to merge 4 commits into
apache:mainfrom
colaforniaw:feat/tool-output-rendering
Closed

colaforniaw wants to merge 4 commits into
apache:mainfrom
colaforniaw:feat/tool-output-rendering

Conversation

@colaforniaw

@colaforniaw colaforniaw commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

Summary

A long turn renders as a wall of tool rows, and opening one rarely helps: every tool shares the same generic detail — raw invocation lines, unstructured dumps, and for results the runtime has archived, a bare [archived_tool_result] tag with nothing behind it.

This PR rebuilds the desktop tool transcript around reading: collapsed rows state the action, target, and result count; large results render as bounded, structured previews; and whatever the preview omits stays reachable through an explicit, sha256-verified "Open full output" in the Files pane.

Current state

  • Every tool renders through the same generic detail, so expanding a row yields invocation noise regardless of what the tool did.
  • Results too large to keep inline are archived by the runtime; the desktop renders the placeholder as a bare [archived_tool_result] tag, so a growing share of tool output has no reader-facing path at all.
  • Large text, JSON, and diff results dominate the conversation, and WebFetch lands as page-sized text instead of a source reference.
  • During streaming, terminal output follows the tail and drags the viewport back when the reader scrolls away.

Implementation approach

  • Add semantic tool summaries for file operations, search results, and success receipts so collapsed rows communicate the action and outcome directly.
  • Use bounded, redacted previews for text, JSON, diffs, and shell output. Preserve complete retained results behind an explicit “View saved output” action in the Files panel.
  • Render successful WebFetch results as compact citation cards with the title, source link, byte count, line count, and truncation state; keep failed fetches readable as text errors.
  • Introduce a reusable output scroller that pauses tail-following when the user scrolls away and provides a “Jump to bottom” action to resume it.
  • Transfer archived output through validated, chunked Host reads with bounded caching, while preserving existing storage and permission behavior.

Before / After

1. Collapsed tool rows: invocation lines to semantic targets

Before After
image image

Collapsed rows lead with the file, target, or result count instead of the raw invocation line, and surface result counts next to the row.

2. WebFetch: page-sized text to a citation card

Before After
image image

A compact citation replaces the page body. Users visit the source through its external link instead of expanding the body in the conversation.

3. Retained results: unreachable to inspectable

Before After
image image

Results omitted from the preview — including archived ones, which previously rendered as an invocation echo with no path to the body — can now be inspected and copied from the Files panel.

4. A full tool turn, expanded: dumps to bounded previews

Before After
image image

Expanding several completed tools stacks page-sized text walls and raw dumps. Each detail becomes a bounded, structured preview, with retained output reachable on demand.

Close #4863

Verification

  • Targeted Core, UI interaction, Desktop IPC/client, and Runtime Host checks passed.
  • Desktop builds and workspace typechecking passed.
  • Staged Biome, ASF header, protocol epoch, and whitespace checks passed.

Full repository lint, format, build, test, and Desktop/UI Knip checks are not confirmed by the recorded validation. Real-window visual verification remains pending.

AI use

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

Tool(s) and scope: OpenAI Codex contributed implementation, regression tests, review-feedback analysis, local verification, and this PR description. The commit includes Generated-by: OpenAI Codex; retain it in the squash commit.

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/XL Under 2500 readable lines label Sep 8, 2026
@colaforniaw
colaforniaw force-pushed the feat/tool-output-rendering branch from f8d4cca to 59b1c16 Compare September 8, 2026 14:43
@colaforniaw colaforniaw changed the title feat(desktop): readable tool rows, WebFetch citations, and on-demand full retained output feat(desktop): improve tool output rendering and interaction Sep 8, 2026
@colaforniaw
colaforniaw marked this pull request as ready for review September 8, 2026 16:48
@colaforniaw
colaforniaw force-pushed the feat/tool-output-rendering branch 2 times, most recently from 86d29d8 to 4338172 Compare September 8, 2026 17:09

@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.

Thanks for improving the tool-output reading experience! We reviewed 4338172 and left one small functional issue inline, plus a non-blocking request to measure the long-text rendering path. The Write issue was checked against the base version; the text-rendering note is a measurement suggestion, not a confirmed performance regression.

Separately, the current CI test job reports three unused exports in the Desktop story barrel; the Windows packaging check has passed.

This review was assisted by OpenAI Codex, including independent reviewer passes. The maintainer has confirmed the visual direction; the code findings and checks above are AI-assisted verification.

import { formatBytes, readResultText, webFetchReference } from './preview-utils.js';

function isSuccessReceipt(result: ToolActivityItem['result']): boolean {
if (result?.kind === 'file_write') return true;

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.

[P2] Keep the full destination path inspectable for Write results

Could we retain a detail view for file_write results? This is a normal Write outcome when the runtime does not return a diff (for example, a 70,000-byte write). The collapsed row now shows only the basename, and treating this result as a status-only receipt disables its disclosure, so a write to /repo/packages/backend/config.json leaves no way to inspect the full destination from the tool row. This makes same-named files difficult to distinguish. I compared the same result against the base: the full path and disclosure were present there, and both are absent here. Keeping the compact basename while preserving the full path in the existing detail view would address it.

// No heading: the collapsed row above already carries the invocation,
// and it can be an archive ref — repeating it here wraps a raw URI
// across the block's title slot.
text={formatUserVisibleToolText(redactSecrets(content.text), locale)}

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.

Non-blocking: could we measure the long-text path?

This now passes the complete text through ToolTextPreview to CodeBlock, whereas the previous path capped it at 500 lines. The 208px height limit bounds the visible area, but the complete text still reaches the renderer. I have not reproduced a slowdown, so this is a request for validation rather than a confirmed regression. Could we compare base and head with a representative large retained text result (including many short lines), checking expansion time and responsiveness while scrolling? If the cost is material, we could keep a bounded inline preview and use the existing saved-output viewer for the full text; if the measurements are healthy, recording them would be sufficient.

@colaforniaw
colaforniaw force-pushed the feat/tool-output-rendering branch 5 times, most recently from 336873c to 1631dbc Compare September 10, 2026 15:15

@me2seeks me2seeks 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.

Automated review (Command Code) — not an approval

The dispatch is at the existing source of truth (standardToolCall → ToolResultPreview), the archive reader verifies size and sha256 before exposing bytes, and the bounded previews never drop content without reporting it. One state-lifetime bug, plus some dead additions.

P2 (Should-Fix) — the tool-output viewer's state outlives the pane that renders it, so closing and reopening the Files panel resurrects the previous output.

The preview lives in an app-level provider:

// apps/desktop/src/renderer/features/workbar/tools/artifacts/tool-output-preview-context.tsx:32-39
export function ToolOutputPreviewProvider(props: { children?: ReactNode }) {
  const [preview, setPreview] = useState<{ request: ToolOutputOpenRequest; id: number; visible: boolean }>();
  ...
  const hide = useCallback(() => {
    setPreview(current => current ? { ...current, visible: false } : current);
    ...
// apps/desktop/src/renderer/composition/desktop-feature-services.tsx:85-87
<ToolOutputPreviewProvider>

but the pane renders the viewer instead of the artifact list whenever visible is true:

// .../tools/artifacts/artifact-pane.tsx:453-455
{toolOutput?.preview?.visible && <ToolOutputPreview key={toolOutput.preview.id} request={toolOutput.preview.request}
    onClose={() => { setView({ kind: 'list' }); toolOutput.hide(); }} />}
{!toolOutput?.preview?.visible && (

The preview is only cleared by hide() from the viewer's own back/Escape (:454) or by close() from the controller's session-scoped cleanup (use-workbar-controller.ts:663-664, a useLayoutEffect keyed on [activeSessionId, closeToolOutput]). Nothing clears it when the Files tab is dismissed. The pane is rendered per tab from positionedTabs (workbar-surface.tsx:482, 527-537), so closing the tab unmounts it — and reopening Files (openTool('files')) remounts it with preview.visible still true.

Failure mode: open a retained tool output → dismiss the Files panel → open Files again: the viewer shows the previous output rather than the artifact list, and re-issues the archive read.

Smallest sound fix: clear it in the pane's existing unmount cleanup (artifact-pane.tsx:126-132) — toolOutput?.close() — or move the preview state down so the pane owns its own lifetime. This provider has one consumer, so the pane is the tighter owner.

P3 (Nice-to-have) — dead additions worth deleting before merge.

  • packages/ui/src/tool-activity/tool-code-block.tsx:33,45: hasCopyButton is added and forwarded to Astryx CodeBlock, but no caller passes it (all four ToolCodeBlock call sites omit it). Delete the prop.
  • packages/ui/src/tool-activity/copy.ts:39,213,338,463: detail.jsonHidden is defined for all three locales and never read.
  • packages/ui/src/tool-activity/preview-utils.ts:33,41-59: the new capLines options tail and paragraphs (and the surrogate/paragraph-boundary branches they drive) have no caller — the three call sites pass {lines,chars}, {chars}, and {}. They are unreachable, untested branches in the path everything depends on. Either wire them or drop them.

P3 (Nice-to-have) — the "source-truncated" notice cannot currently render, and would be missing if it could.

tool-text-preview.tsx:70 only surfaces the notice and the saved-output button under props.truncated || previewTruncated, while the text branch of ToolResultPreview passes sourceTruncated (which only changes the wording) and never truncated. content.truncated on a text result is only produced by the WebFetch tool, and non-failed WebFetch results are diverted to WebFetchPreview first — so copy.detail.sourceTruncated is unreachable today. If a future tool sets it, a runtime-truncated text result whose retained body fits the cap would get no notice at all. Pass truncated={content.truncated} from the text branch, or delete the path.

Verified clean (recorded because these are the areas most likely to hide a defect here): bounded previews always report hidden content (capLines only ever shortens, and the notice/button are gated on hiddenChars > 0); the diff trim only drops an incomplete trailing hunk, so a complete hunk is never lost; per-row disclosure and scroll authority are keyed by tool-call identity, so state cannot leak between calls; the new scroll behavior is gated on an explicit resume option, leaving the transcript's own pin/resume semantics unchanged; and the archive reader validates size and sha256 before yielding bytes.

Review-relevant risks. No security, licensing or dependency change identified. The protocol epoch moves to 142 with an explanatory comment and both documentation inventories updated. One item a maintainer should accept knowingly: styles/workbar/shell.css adds an Astryx-internal-nesting selector scoped across all workbar panels (> .astryx-section { height:100%; overflow:hidden }), while the linked issue asks to avoid nesting-dependent selectors; clipping in non-files panels is unverified without a real window.

Required conclusion.

  1. Optimal for the actual problem? Close. The dispatch is at the existing source of truth and no parallel preview path was added; the two things stopping it being optimal are the preview's ownership (P2) and the dead API/branches added alongside.
  2. Production code that can be deleted? ToolCodeBlock.hasCopyButton; copy.detail.jsonHidden; the unused capLines options; the sourceTruncated path unless the text branch forwards content.truncated.
  3. Low-quality tests to delete or replace? none identified. The gap is coverage: the viewer (ToolOutputPreview) is only exercised by a Storybook play test, so its envelope unwrapping, partial-read detection, and failure/retry messages have no unit test — which is why the unreachable notice above went unnoticed.
  4. Deeper refactor required? No. One ownership move as in P2.
  5. Ready to merge? Not as-is; P2 is a one-line fix and the P3 dead code should be dropped before merge.
  6. Residual risks / verification gaps: I did not run the suite or a real window. The settled shell preview is no longer line-capped, so the whole retained tail renders — I verified the upstream bounds (≤2000 lines / 50 KiB per stream, 500-row PTY scrollback), so this is roughly 4× the previous DOM rather than unbounded, but it is a deliberate change worth accepting explicitly.

Approval boundary. This is automated review; it is not an approval. Per CONTRIBUTING.md, the merge decision requires an independent human review. No approve was submitted.

@colaforniaw
colaforniaw force-pushed the feat/tool-output-rendering branch from 1631dbc to 6690847 Compare September 11, 2026 03:27
@colaforniaw

Copy link
Copy Markdown
Contributor Author

@me2seeks Thanks for the thorough review. I’ve addressed the P2 issue and removed the unused props, copy, branches, and styling noted in the review.

@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.

LGTM, all nice ui improvements!

@me2seeks me2seeks 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.

Follow-up (Command Code) — verified resolved

I re-checked the head after your push.

The P3 dead-code items are all gone — the unused hasCopyButton prop, the unread jsonHidden copy key, and the capLines options with no caller have been removed, so the preview path no longer carries unreachable branches.

If the P2 (the retained-output viewer's state outliving the Files pane, cleared in the pane's unmount) landed in the same pass, that closes everything I raised.

Thanks for the cleanup. (Automated review; not an approval.)

@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.

Reviewed across four boundaries — core/Host artifact transport, the Desktop IPC surface, the packages/ui rendering, and the runtime tool producers — building each package and running the suites, with targeted probes through the production entry points rather than reading alone.

The security surface holds, and it was worth checking carefully. A forged ref gets nothing: the legacy path requires getInSession plus source === 'tool_result_archive' plus a size match, the ledger path requires the session, event, toolCallId and toolName to match and a real placeholder in the reduced transition chain, and both verify sha256 over the complete body before the first chunk goes out. Traversal is structurally impossible, not filtered — the builder percent-encodes and the parser re-tests /^[A-Za-z0-9._-]{1,160}$/ after decodeURIComponent. 18 malformed inputs against the compiled IPC handler all returned not_allowed without reaching the client. The guest gate runs before the read_archive_chunk branch and #sharedArtifactGrantId returns undefined for it, so a read-only shared viewer can't read archived output at all — fail-closed, no new guest capability, and worth a sentence in the body. The protocol epoch moved (141 → 142). In the renderer, javascript: and data: URLs produce no href attribute at all, the scraped title renders as an escaped React text node, redaction runs before truncation on every branch, and capLines keeps one unit (UTF-16 code units) throughout with no byte budget mixed in.

Five things below.

P2 — The new tail-follow effect destroys the scroll authority's gesture state, so a scrollbar drag can't pause a streaming output

Reachability ②. The wheel path still pauses correctly — once unpinned the effect stops firing — so this breaks one input route into a new feature rather than regressing existing behaviour.

tool-text-preview.tsx:92-94 handles "content grew" with if (scroll.getSnapshot().pinned) scroll.pinToTail(). But pinToTail() begins with gesture = undefined (transcript-scroll-authority.tsx:287), and a scrollbar drag arms gesture in onPointerDown without setting pinned = false — unpinning happens later in onScroll, once a direction is known. So: hold the thumb (gesture armed, still pinned) → next chunk arrives → this effect sees pinned === true → pinToTail() clears the gesture → every subsequent scroll event has no gesture to compare against and never unpins. The drag cannot pause the follow, "Jump to bottom" never appears, and the next chunk snaps the viewport back.

Verified by driving ToolCallDetail through real DOM: with no chunk during the drag, dragging to 400 stays at 400 and the button appears; with one chunk after pointerdown, the button never appears and the next chunk returns scrollTop to 800.

The authority's own growth path is written if (pinned && !gesture) writeToTail() (transcript-scroll-authority.tsx:250). That !gesture guard is load-bearing, and it exists for exactly this. The new component re-implemented the same job in a React effect, dropped the guard, and reached for a public method that destroys the state the guard reads.

The same misplacement shows up a second way at the end of a tool run. Streaming renders one <pre> inside ToolOutputStream; completion renders a different <pre> in ShellOutputBody, a different subtree, so the ToolOutputScroller and its authority instance unmount and rebuild with pinned = true and an immediate writeToTail(). Verified: a reader paused at scrollTop = 100 during streaming loses both the position and the button the moment the command finishes. A reader scrolling back to an error while a command finishes is an ordinary thing to do. Some handover is unavoidable because the landed content differs from the 16 KB live buffer — but the reader's explicit pause shouldn't be what gets dropped.

Both point the same way: put "content grew" in the authority as its own operation (followTail() = if (pinned && !gesture) writeToTail()), leave pinToTail() for the explicit Jump-to-bottom intent, and hold the scroll state per tool row (toolUseId) rather than per <pre> instance. Gesture ownership then stays inside the authority and the component never needs to know it exists.

P2 — The archive transfer table is bounded by entry count, not the bytes its comment claims, and evicts FIFO

Reachability ②③.

artifact-coordinator.ts:61-64 says "At most 16 MiB of verified bodies". What's enforced is MAX_ARCHIVE_TRANSFERS = 4 entries, evicted FIFO, with no reinsert on hit. Past four concurrent transfers every request evicts another live one and the hit rate goes to zero — a cliff, not a curve.

Measured through handlers['artifact.query'] with a 512 KiB body and 32 KiB chunks:

4 concurrent panes, 16 chunks each ->  4 full archive reads (ideal 4);  2 MiB read to deliver 2 MiB
5 concurrent panes, 16 chunks each -> 77 full archive reads (ideal 5); 39 MiB read to deliver 3 MiB

13× amplification at five. At the 4 MiB archive cap, one 32 KiB chunk request costs a 4 MiB read plus a 4 MiB sha256 — 128× request-to-work. sessionAdmission.run only serializes within a session, not across them.

Three changes, and they belong together: reinsert on hit so paging doesn't evict itself (FIFO → LRU); evict on accumulated bytes rather than entries, using the 16 MiB the comment already names; and drop the input.offset === 0 forced re-read at :389 — the ref is content-addressed and the digest is verified on every read, so a re-read cannot produce different bytes, and TTL already handles freshness.

P2 — Collapsed-row projection widened the crash surface from one detail block to the whole transcript row

Reachability ②③.

toolResultStats now runs for every row while collapsed, and two branches trust the persisted shape:

  • result-projection.ts:59 — result.rows.length throws when rows is missing.
  • result-projection.ts:53 — a missing text reaches webFetchReference, where text.slice(0, 16_000) throws.

Verified: both shapes throw while rendering the collapsed row. On base, collapsed rows never read result content. The repo already treats this as a contract — there is a test named contains a malformed persisted terminal result instead of crashing the renderer — and the same malformed archive used to break only the expanded block. One wrong assumption in two places, so it's here rather than in line comments; the fixes themselves are one expression each (result.rows?.length ?? 0, and a typeof result.text === 'string' check).

P2 — Several PR-body claims don't match the code

Worth correcting because they shape what a reader trusts:

  • "citation cards with the title, source link, byte count, line count, and truncation state" — WebFetch shows neither count; toolResultStats returns only a title for it. And none of this is producer-side: web-fetch-tool.ts only changed its return from string to {kind:'text', text, truncated?}. The title is scraped renderer-side from the fetched markdown's first # heading (preview-utils.ts:96), and the source is args.url.
  • The source link is the requested URL, not the final one. local-web-fetch.ts follows up to 10 redirects and keeps response.url only for rewriting in-page relative links; it never returns. So https://short.link/x redirecting to evil.example still renders as short.link/x. The fix is producer-side — return {content, finalUrl} and put finalUrl in the result. Not graded higher because the model's view is unaffected and the final URL was never shown before, but a citation that names the wrong source is the one field a citation exists for.
  • "sha256-verified" reads as client verification. It's Host-side (correctly, and before the first chunk) — DesktopRuntimeHostClient.readToolResult concatenates chunks and returns without hashing or asserting totalBytes === identity.originalBytes. Fine under "the Host is the only authority", but say that rather than implying the client checks. (If you ever want the client to verify, note archive_chunk carries nothing binding a response to the requested ref, unlike chunk which carries artifactId.)
  • "bounded caching" is the Host-side #archiveTransfers map. There is no renderer cache at all — each open performs a fresh chunked read.
  • The model's view of WebFetch is unchanged — I checked the old and new projections byte-for-byte. So "lands as a source reference" describes only the renderer and is not a tool-contract change. Good, but say so, because the sentence currently reads like one.

P2 — Two more places where the state lives above what owns it

  • The preview selection is app-global with no session key. ToolOutputPreviewProvider mounts at app root (desktop-feature-services.tsx:85) and holds {request, id}; its lifetime is then patched from outside in two unrelated places — use-workbar-controller.ts:664's layout-effect cleanup on session switch, and artifact-pane.tsx:133-135's queueMicrotask + artifactPaneMountedRef dance whose only job is to tell a StrictMode effect replay from a real unmount. ArtifactPane renders toolOutput.preview without checking preview.request.source.sessionId === props.sessionId; the controller's cleanup closes that window, but one commit late. Key the preview by sessionId in the provider and have the pane render only its own session's, and all three patches delete. (The StrictMode guard itself is correct as written — useMountedRef registers before the unmount effect — the objection is to needing it.)
  • complete removes the 256 KiB display bound on the path with the largest payloads. artifact-preview.tsx:258,277 makes displayText the full string and forces isDisplayTruncated false; the Host caps a body at 4 MiB, 16× TEXT_DISPLAY_LIMIT_BYTES. Showing the whole thing is the point of the feature — the gap is that "show all" was implemented by deleting the bound rather than rendering incrementally, so opening a retained Bash output runs redactSecrets over 4 MiB synchronously and puts ~4 MiB into one <pre> node. Either chunk plainRemainder, or state the larger bound explicitly and say in the body why 4 MiB is acceptable here.

P3 — One PR, two opposite conventions for the truncation marker

WebFetch moves the marker out of the persisted text into a truncated: true flag and re-adds it in toModelOutput — the right direction, and the justification for the new truncated?: boolean in events.ts / tool-result-record-schema.ts. PTY keeps the marker embedded in screen/scrollback and strips it back out on read (pty-output-view.ts:32-38), even though PtyShellOutput.truncated already exists and is what triggers the strip.

stripTruncatedMarker only removes a leading marker, while projectPtyOutputForModel calls takeTailText separately for screen, lastAlternateScreen and scrollback — each can be marked. It's safe today only because the budget ordering zeroes remaining after the first truncation, so the marker can only land first. That invariant spans @maka/core and @maka/runtime, and is neither written down nor tested (the new test covers only scrollback).

And only one of three views strips it: ptyTuiTerminalRows / ptyTuiTerminalView / ptyCompactTerminalLine don't, so the CLI transcript renders the marker as a real output line and it eats 6 lines of budget. Measured:

tui  : ["[terminal snapshot truncated to fit the output limit]","line1","line2","done"]
human: "line1\nline2\ndone"

If the WebFetch convention is the right one, PTY should follow it: add the marker only in the model projection, keep it out of the persisted object, and stripTruncatedMarker deletes entirely. That's the ablation this PR's own new abstraction invites.

Test coverage

The tests do drive real DOM through production components, and the IPC tests drive the registered handler — the seams are right. The gaps are at the bounds:

  • artifact-coordinator.test.ts stubs readArchive entirely, so offset > originalBytes, ref-parse failure, and originalBytes > MAX have no test at all.
  • The IPC test covers the happy path plus one sha mismatch. I confirmed by probe that every other rejection already works — a table over null, 'x', ../../etc/passwd, a/b, an uppercase sha, negative and fractional originalBytes, file:///etc/passwd and a malformed ledger ref, asserting not_allowed and that the client was never called, is cheap regression protection.
  • explicitResume, the PR's only change to transcript-scroll-authority.tsx, has zero coverage: the harness stubs requestAnimationFrame as () => 1 and never dispatches scrollend, so the existing test passes even with explicitResume deleted.
  • capLines has no direct unit test despite being the shared upstream of four preview paths.
  • No assertion that an untruncated result still projects as plain text — the invariant the compatibility path below would break.
  • pty-output-view.test.ts covers only the human view, which is why the TUI marker leak wasn't caught.
中文

分四条边界评审——core/Host artifact 传输、Desktop IPC 面、packages/ui 渲染、runtime 工具生产者——每条都构建并跑了测试,并通过生产入口打探针,不是只读代码。

安全面是成立的,而且值得仔细核。伪造 ref 拿不到任何东西:legacy 路径要求 getInSession + source === 'tool_result_archive' + 尺寸匹配,ledger 路径要求 session、事件、toolCallId、toolName 全匹配且 reduce 后的 transition 链里真有对应 placeholder,两条路径都在发出第一个 chunk 之前对完整 body 做 sha256。路径穿越是结构性不可能而非过滤:构造时 percent-encode,解析时 decodeURIComponent 后再过 /^[A-Za-z0-9._-]{1,160}$/。18 组畸形输入打编译后的 IPC handler 全部返回 not_allowed 且没碰到 client。guest gate 在 read_archive_chunk 分支之前,#sharedArtifactGrantId 对它返回 undefined,所以只读共享者根本读不到归档输出——fail-closed,没有新增 guest 能力,值得在正文写一句。protocol epoch 已涨(141 → 142)。渲染侧,javascript: 和 data: URL 产出的结果里完全没有 href 属性,抓来的标题作为转义的 React 文本节点渲染,脱敏在每个分支上都先于截断,capLines 全程只用一种单位(UTF-16 code unit),没有字节预算混入。

以下五条。

P2 — 新的跟尾 effect 销毁了滚动权威的手势状态,导致流式输出无法用滚动条暂停(可达②)

滚轮路径仍然能正常暂停——一旦 unpin,这个 effect 就不再触发——所以坏掉的是新功能的一条输入路径,不是既有行为的回归。

tool-text-preview.tsx:92-94 用 if (scroll.getSnapshot().pinned) scroll.pinToTail() 处理"内容增长了"。但 pinToTail() 的第一句是 gesture = undefined(transcript-scroll-authority.tsx:287),而滚动条拖拽在 onPointerDown 里只 arm gesture、不置 pinned = false——unpin 要等 onScroll 判出方向之后。于是:按住滑块(gesture 已 arm,仍 pinned)→ 下一个 chunk 到达 → effect 看到 pinned === true → pinToTail() 清空 gesture → 之后每个 scroll 事件都没有 gesture 可比,永远不会 unpin。拖动无法暂停跟随,"Jump to bottom" 不出现,下一个 chunk 直接把视口弹回尾部。

用真实 DOM 驱动 ToolCallDetail 验证:拖拽期间不来 chunk 时,拖到 400 停在 400、按钮出现;pointerdown 之后来一个 chunk,按钮不出现,下一个 chunk 把 scrollTop 拉回 800。

而 authority 自己的增长路径写的是 if (pinned && !gesture) writeToTail()(transcript-scroll-authority.tsx:250)。那个 !gesture 守卫是承重的,存在的理由正是这件事。新组件在 React effect 里重新实现了同一件事,丢了守卫,还用了一个会销毁守卫所读状态的公开方法。

同一处错位在工具结束时以第二种形态出现。流式渲染的是 ToolOutputStream 里的一个 <pre>,结束后渲染的是 ShellOutputBody 里另一个 <pre>——不同子树,于是 ToolOutputScroller 连同它的 authority 实例卸载重建,新实例 pinned = true 并立即 writeToTail()。已验证:流式中停在 scrollTop = 100 的读者,在命令结束的那一刻同时失去位置和按钮。命令还在跑时往回翻看报错是很普通的行为。落地内容确实和 16KB 实时缓冲不同,所以一次交接不可避免——但不该由读者已明确表达的暂停来承担。

两者指向同一个方向:把"内容增长了"做成 authority 自己的操作(followTail() = if (pinned && !gesture) writeToTail()),pinToTail() 只留给显式的 Jump to bottom 意图,并把滚动状态按工具行(toolUseId)持有而不是按 <pre> 实例。这样手势的所有权仍然只在 authority 里,组件根本不需要知道它存在。

P2 — archive transfer 表的界是条目数而非注释声称的字节数,且 FIFO 淘汰(可达②③)

artifact-coordinator.ts:61-64 写着 "At most 16 MiB of verified bodies",实际强制的是 MAX_ARCHIVE_TRANSFERS = 4 条,FIFO 淘汰,命中不回插。超过四路并发后,每个请求都会驱逐另一条活跃 transfer,命中率归零——是断崖不是曲线。

通过 handlers['artifact.query'] 实测(512 KiB body,32 KiB chunk):4 路是 4 次全量读(理想值),送出 2 MiB 读 2 MiB;5 路变成 77 次全量读(理想 5),送出 3 MiB 读了 39 MiB。五路时 13 倍放大。按 4 MiB archive 上限算,一个 32 KiB 的 chunk 请求要花一次 4 MiB 读盘加一次 4 MiB sha256——128 倍的请求/工作量不对称。sessionAdmission.run 只在 session 内串行,跨 session 不设防。

三处一起改:命中时回插,别让分页驱逐自己(FIFO → LRU);按累计字节而非条数淘汰,用注释里已经写着的 16 MiB;删掉 :389 的 input.offset === 0 强制重读——ref 是内容寻址的且每次读都校验摘要,重读不可能得到不同字节,新鲜度由 TTL 负责。

P2 — 折叠行投影把崩溃面从一个详情块扩大到整条 transcript 行(可达②③)

toolResultStats 现在对每一行在折叠状态下就跑,其中两个分支信任了持久化的形状:result-projection.ts:59 的 result.rows.length 在 rows 缺失时抛错;:53 在 text 缺失时把它送进 webFetchReference,那里的 text.slice(0, 16_000) 抛错。

已验证:两种形状都在渲染折叠行时抛错。base 上折叠行从不读 result 内容。仓库已经把这当作契约——有一条测试叫 contains a malformed persisted terminal result instead of crashing the renderer——而同样的畸形归档以前只会炸展开的那一块。同一个错误假设出现在两处,所以放这里而不是行内;修法各自只有一个表达式(result.rows?.length ?? 0,以及一个 typeof result.text === 'string' 检查)。

P2 — PR 正文有几处与代码不符

值得更正,因为它们决定读者会相信什么:

  • "citation cards with the title, source link, byte count, line count, and truncation state"——WebFetch 两个计数都不显示,toolResultStats 对它只返回 title。而且这些都不在 producer 侧:web-fetch-tool.ts 这次只把返回值从 string 改成 {kind:'text', text, truncated?}。title 是渲染侧从抓取到的 markdown 第一个 # 标题正则抠出来的(preview-utils.ts:96),来源是 args.url。
  • 来源链接是请求 URL,不是最终 URL。 local-web-fetch.ts 最多跟 10 跳重定向,response.url 只用于改写页内相对链接,从不返回。所以 https://short.link/x 重定向到 evil.example 时仍然渲染成 short.link/x。修法在 producer 侧——返回 {content, finalUrl} 并把 finalUrl 放进结果。不定更高级别是因为模型看到的正文不受影响、且最终 URL 以前也从未展示过;但 citation 存在的意义就是那一个字段,指错了就没了意义。
  • "sha256-verified" 读起来像客户端校验。它在 Host 侧完成(做得对,而且在第一个 chunk 之前)——DesktopRuntimeHostClient.readToolResult 把 chunk 拼起来就返回,既没做 hash 也没断言 totalBytes === identity.originalBytes。在"Host 是唯一权威"下这没问题,但应该这么写,而不是暗示客户端验了。(若将来想让客户端验:archive_chunk 帧里没有任何东西能把响应绑回请求的 ref,不像 chunk 带 artifactId。)
  • "bounded caching" 指的是 Host 侧的 #archiveTransfers。渲染侧根本没有缓存,每次打开都是一次全新的分块读。
  • 模型看到的 WebFetch 没有变化——我逐字节核对了新旧投影。所以 "lands as a source reference" 只描述渲染层,不是工具契约变更。这是好事,但要说出来,因为现在那句话读起来像是变更。

P2 — 还有两处状态住在比它所有者更高的地方

  • preview 选择态是 app 全局的,没有 session key。 ToolOutputPreviewProvider 挂在 app 根(desktop-feature-services.tsx:85)持有 {request, id},它的生命周期于是被两个不相干的地方从外面打补丁——use-workbar-controller.ts:664 切会话时的 layout-effect 清理,以及 artifact-pane.tsx:133-135 的 queueMicrotask + artifactPaneMountedRef 绕法,后者唯一的作用是区分 StrictMode 的 effect 重放和真正的卸载。ArtifactPane 渲染 toolOutput.preview 时不检查 preview.request.source.sessionId === props.sessionId;controller 的清理会合上这个窗口,但晚一次 commit。把 preview 在 provider 里按 sessionId 存,让 pane 只渲染自己 session 的,三处补丁都能删。(那个 StrictMode 守卫本身写得是对的——useMountedRef 注册早于卸载 effect——异议在于不该需要它。)
  • complete 在载荷最大的那条路径上去掉了 256 KiB 显示上界。 artifact-preview.tsx:258,277 让 displayText 变成完整字符串并强制 isDisplayTruncated 为假;而 Host 对 body 的上限是 4 MiB,是 TEXT_DISPLAY_LIMIT_BYTES 的 16 倍。完整显示正是这个功能的意义所在——问题在于"全部显示"是用删掉上界实现的,而不是增量渲染,于是打开一个留存的 Bash 输出会同步跑 4 MiB 的 redactSecrets 再把约 4 MiB 塞进单个 <pre> 节点。要么把 plainRemainder 分块,要么显式声明更大的上界并在正文说明为什么 4 MiB 在这里可以接受。

P3 — 同一个 PR 里对截断标记用了两套相反的约定

WebFetch 把标记移出持久化文本、改成 truncated: true 标志,由 toModelOutput 再拼回——方向是对的,也正是 events.ts / tool-result-record-schema.ts 新增 truncated?: boolean 的正当理由。PTY 则继续把标记内嵌在 screen/scrollback 里,读的时候再剥掉(pty-output-view.ts:32-38),尽管 PtyShellOutput.truncated 本来就存在,而且正是触发剥离的条件。

stripTruncatedMarker 只剥开头的标记,而 projectPtyOutputForModel 对 screen、lastAlternateScreen、scrollback 各自调 takeTailText,每段都可能被标记。现在之所以安全,纯粹是因为预算顺序:第一次截断就会把 remaining 置 0,所以标记只可能出现在最前面。这个不变量跨 @maka/core 和 @maka/runtime 两个包,既没写下来也没测(新测试只覆盖 scrollback)。

而且三个视图只有一个剥:ptyTuiTerminalRows / ptyTuiTerminalView / ptyCompactTerminalLine 都不剥,所以 CLI transcript 会把标记当成一行真实输出渲染,还挤占 6 行预算。实测 tui 视图第一行就是那条标记,human 视图没有。

如果 WebFetch 那套是对的,PTY 也该照做:标记只在模型投影里加,持久化对象里不带,stripTruncatedMarker 随之整个删掉。这正是本 PR 自己新增的抽象所邀请的那次消融。

测试覆盖

测试确实通过真实 DOM 驱动生产组件,IPC 测试也驱动已注册的 handler——接缝是对的。缺口都在边界上:artifact-coordinator.test.ts 把 readArchive 整个 stub 掉,offset > originalBytes、ref 解析失败、originalBytes > MAX 三条分支毫无测试;IPC 测试只覆盖 happy path 加一次 sha 不匹配(我已用探针确认其余拒绝都已生效,补一张表断言 not_allowed 且 client 未被调用,是很便宜的回归保护);explicitResume 作为本 PR 对 transcript-scroll-authority.tsx 的唯一改动,覆盖率为零——harness 把 requestAnimationFrame 打桩成 () => 1 且从不派发 scrollend,所以把 explicitResume 整个删掉现有测试照样通过;capLines 作为四条预览路径的共同上游没有任何直接单测;没有任何断言保证未截断的结果仍投影为纯文本;pty-output-view.test.ts 只覆盖 human 视图,这正是 TUI 那处标记泄漏没被发现的原因。

const identity = parseToolResultArchiveResourceRef(input.ref);
if (!identity) return unavailable('not_allowed');
if (identity.originalBytes > TOOL_RESULT_ARCHIVE_MAX_BYTES) return unavailable('too_large');
if (input.offset > identity.originalBytes) return invalidQuery('Archive offset is invalid');

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.

P3 Off by one: offset === originalBytes passes and returns an ok empty chunk with nextOffset: null. originalBytes is always > 0 and the decoder guarantees nextOffset < totalBytes, so no client will ever ask for it — but >= is the rule you mean.

中文

差一:offset === originalBytes 会通过,返回一个 ok 的空 chunk 且 nextOffset: null。originalBytes 恒大于 0,解码器又保证 nextOffset < totalBytes,所以没有客户端会请求这个偏移——但你要表达的规则是 >=。

}
const key = JSON.stringify([context.connectionId, input.sessionId, input.ref]);
let transfer = this.#archiveTransfers.get(key);
if (input.offset === 0 || transfer?.ref !== input.ref) {

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.

P3 transfer?.ref !== input.ref is dead: key already contains input.ref (line 387), so a retrieved entry's ref is necessarily equal.

The input.offset === 0 half is worth dropping too. The ref is content-addressed and bodySha256 is verified on every read, so a re-read cannot return different bytes; freshness is the TTL's job. Forcing a fresh full read at the start of every pagination run is also the single biggest contributor to the amplification in the review body. if (!transfer) { … } removes the dead code and that cost together.

中文

transfer?.ref !== input.ref 是死条件:key 在 387 行就含了 input.ref,取到的条目 ref 必然相等。

input.offset === 0 那一半也该删。ref 是内容寻址的,每次读都校验 bodySha256,重读不可能返回不同字节;新鲜度由 TTL 负责。而在每轮分页开始时强制一次全量重读,正是正文里那个放大效应最大的单一来源。改成 if (!transfer) { … } 同时消掉死代码和这份开销。

this.#readArchive!({
...identity,
sessionId: input.sessionId,
maxBytes: TOOL_RESULT_ARCHIVE_MAX_BYTES,

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.

P3 The only other caller, readToolResultArchiveResource (tool-result-archive-resource.ts:233), passes maxBytes: identity.originalBytes. Passing the 4 MiB cap here turns the ledger reader's maxBytes < originalBytes → too_large guard into a no-op and makes the legacy path read against 4 MiB rather than the size the ref itself declares. It can't over-read in practice (sizeBytes !== originalBytes → size_mismatch fires first, and byte length is checked again), so this is about not running two conventions on one read path. Use identity.originalBytes.

中文

唯一的另一个调用方 readToolResultArchiveResource(tool-result-archive-resource.ts:233)传的是 maxBytes: identity.originalBytes。这里传 4 MiB 上限会让 ledger reader 的 maxBytes < originalBytes → too_large 守卫变成空转,也让 legacy 路径按 4 MiB 而不是 ref 自己声明的尺寸去读。实际读不超(sizeBytes !== originalBytes → size_mismatch 会先挡住,byte length 也会再校验一次),所以这条只关乎别在同一条读路径上跑两套约定。改成 identity.originalBytes。

export function toolHasDetail(item: ToolActivityItem): boolean {
if (item.outputChunks?.length || item.outputTruncated) return true;
if (item.status !== 'completed' && item.args !== undefined) return true;
if (item.status === 'completed' && isSuccessReceipt(item.result)) return false;

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.

P2 (reachability ①) This short-circuits before the args check on line 41, which only covers status !== 'completed'. So a tool whose result is just {ok: true} and whose content lives entirely in its args ends up with no disclosure arrow and nowhere showing the args.

Verified: mcp__notes__append with args {entries: ['buy milk','call bob'], notebook: 'personal'} and result {ok:true} renders as "mcp__notes__appendSucceeded", aria-expanded is null, and neither buy milk nor notebook nor personal appears anywhere in the markup. The collapsed target can't recover it either — the default branch only looks for the scalar keys query/pattern/url/name/title/path/file/id.

Suppressing a redundant success receipt is right; it just shouldn't swallow the args with it. return item.args !== undefined; is enough — ToolCallDetail's hasSharedPanelContent already handles the args branch.

中文

(可达①)这一句在 41 行的 args 检查之前短路,而那个检查只覆盖 status !== 'completed'。于是一个结果只有 {ok:true}、内容全在 args 里的工具,完成后既没有展开箭头,也没有任何地方显示 args。

已验证:mcp__notes__append,args 为 {entries: ['buy milk','call bob'], notebook: 'personal'},result {ok:true} → 行文本 "mcp__notes__appendSucceeded",aria-expanded 为 null,整段 markup 里搜不到 buy milk / notebook / personal。折叠标题也救不回来——默认分支只找 query/pattern/url/name/title/path/file/id 这几个标量键。

省掉冗余的成功回执是对的,只是不该把 args 一起吞掉。return item.args !== undefined; 就够了——ToolCallDetail 的 hasSharedPanelContent 已经能走 args 分支。

if (item.toolName === 'Read' && item.args && typeof item.args === 'object' && 'path' in item.args && typeof item.args.path === 'string') {
const name = redactSecrets(item.args.path.split(/[\\/]/).pop() ?? item.args.path);
const text = readResultText(result);
return text === undefined ? name : copy.detail.lines(text === '' ? 0 : text.split('\n').length);

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.

P3 text.split('\n').length counts one line too many for any file ending in a newline, which is most source files — a 10-line file reads "11 lines". The text === '' case is already special-cased, so the edge was considered; this one was missed. Same expression at tool-result-preview.tsx:158.

(text.endsWith('\n') ? text.slice(0, -1) : text).split('\n').length fixes both, with '' still taking the zero branch. Note the existing test pins the current behaviour with ['one\n', 2], so that changes too.

中文

text.split('\n').length 对以换行结尾的文件多算一行,而绝大多数源码文件都是这样——10 行的文件显示成 "11 lines"。text === '' 已经特判过,说明边界是考虑过的,只是漏了这个。tool-result-preview.tsx:158 是同一个表达式。

(text.endsWith('\n') ? text.slice(0, -1) : text).split('\n').length 两处都能修,'' 仍走 0 分支。注意现有测试用 ['one\n', 2] 把当前行为钉死了,要一起改。

let partial = false;
// ArchiveRead wraps a retained page in protocol metadata. Only unwrap
// successful body responses; ordinary JSON and diagnostics stay intact.
if (props.request.toolName === 'ArchiveRead' && parsed && typeof parsed === 'object' && 'kind' in parsed && parsed.kind === 'tool_result_archive'

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.

P2 (reachability ②) The ArchiveRead envelope is re-implemented here with string literals — kind === 'tool_result_archive', operation === 'read' | 'query', content, hasMore, offset, lineOffset. Its producer is packages/runtime/src/tool-result-archive-resource.ts (pagedContent at :480, pagedLines at :682). That is a second authority for the shape, living in the desktop app, with no test on this side: rename a field on the producer and the viewer silently degrades to showing the raw JSON envelope.

Export unwrapArchiveReadPage(value): { content: string; partial: boolean } | undefined from tool-result-archive-resource.ts next to the builders and call it here — the existing tool-result-archive-resource.test.ts then covers both sides.

中文

(可达②)ArchiveRead 信封在这里用字符串字面量重新实现了一遍——kind === 'tool_result_archive'、operation === 'read' | 'query'、content、hasMore、offset、lineOffset。它的生产者是 packages/runtime/src/tool-result-archive-resource.ts(pagedContent 在 :480,pagedLines 在 :682)。这就是这个形状的第二个权威,住在 desktop app 里,且这一侧没有任何测试:生产者改个字段名,查看器就静默退化成显示原始 JSON 信封。

从 tool-result-archive-resource.ts 的构造函数旁边导出 unwrapArchiveReadPage(value): { content: string; partial: boolean } | undefined,在这里调用它——现有的 tool-result-archive-resource.test.ts 就同时覆盖了两侧。

deps.ipcMain,
'artifacts:readToolResult',
async (_event, sessionId: string, identity: ToolResultArchiveIdentity): Promise<ArtifactTextReadResult> => {
if (!identity || (!identity.resourceRef && typeof identity.artifactId !== 'string')

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.

P3 (reachability ③) { artifactId, resourceRef } with both present is accepted and resourceRef silently wins, while ToolResultArchiveIdentity declares them mutually exclusive (artifactId?: never) — and the PR's own new test at runtime-host-artifacts-ipc-main.test.ts:57 constructs exactly that forbidden combination. Confirmed by running: {artifactId:'a', resourceRef:'maka://archive/b/<sha>/5'} reaches the Host as the b ref.

Low impact — the Host re-parses, the reader re-verifies the sha, and the renderer already reaches arbitrary artifact ids through artifacts:readText. But the guard should reject what the type says can't exist: add || (identity.resourceRef !== undefined && identity.artifactId !== undefined).

中文

(可达③){ artifactId, resourceRef } 两者都在时会被接受且 resourceRef 静默胜出,而 ToolResultArchiveIdentity 声明它们互斥(artifactId?: never)——PR 自己新增的测试 runtime-host-artifacts-ipc-main.test.ts:57 构造的正是这个被禁止的组合。运行确认:{artifactId:'a', resourceRef:'maka://archive/b/<sha>/5'} 到达 Host 时是 b 那个 ref。

影响很小——Host 会重新解析,reader 会重新校验 sha,而且渲染侧本来就能经 artifacts:readText 取到任意 artifact id。但 guard 应该拒绝类型声称不可能存在的东西:加上 || (identity.resourceRef !== undefined && identity.artifactId !== undefined)。

>;

const ATTACHMENT_PREVIEW_LIMIT_EXCEEDED = Symbol("attachment-preview-limit-exceeded");
const ARTIFACT_READ_LIMIT_EXCEEDED = Symbol("artifact-read-limit-exceeded");

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.

P3 ATTACHMENT_PREVIEW_LIMIT_EXCEEDED → ARTIFACT_READ_LIMIT_EXCEEDED is unrelated churn. The symbol is still thrown and caught only inside registerRuntimeHostAttachmentPreviewIpc (:186, :191), the new archive path never touches it, and the broader name is now less accurate than the old one. Revert all three lines.

中文

ATTACHMENT_PREVIEW_LIMIT_EXCEEDED → ARTIFACT_READ_LIMIT_EXCEEDED 是无关改动。这个符号仍然只在 registerRuntimeHostAttachmentPreviewIpc 内部抛出和捕获(:186、:191),新的归档路径从不碰它,而更宽泛的名字现在反而比旧名字更不准确。三行都回退。

if (Buffer.byteLength(text, 'utf8') <= budget) return { text, truncated: false };
if (budget <= 0) return { text: '', truncated: text.length > 0 };
const markerBytes = Buffer.byteLength(TRUNCATED_MARKER, 'utf8');
const markerBytes = Buffer.byteLength(PTY_TRUNCATED_MARKER, 'utf8');

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.

P2 (reachability ①) takeTailText prepends PTY_TRUNCATED_MARKER in-band, and the model already receives the structured answer alongside it. Measured on bashToolResultToModelOutput: the JSON carries "truncated": true in the same object as the marked-up scrollback. The marker is redundant for the model, and on the display side only one of three views removes it — ptyTuiTerminalRows / ptyTuiTerminalView / ptyCompactTerminalLine don't, so the CLI transcript (pi-transcript-tools.ts:292/360/645/714) renders it as a real output line and it eats 6 lines of budget:

tui  : ["[terminal snapshot truncated to fit the output limit]","line1","line2","done"]
human: "line1\nline2\ndone"

return { text: sliceUtf8Tail(text, budget), truncated: true } without the prefix makes all three views agree, and then the PTY_TRUNCATED_MARKER export and stripTruncatedMarker both delete — which is the WebFetch convention this same PR adopts. If the marker stays, move the strip into textRows() so the three views share one removal.

中文

(可达①)takeTailText 把 PTY_TRUNCATED_MARKER in-band 前置,而模型本来就同时拿到了结构化答案。实测 bashToolResultToModelOutput:JSON 里 "truncated": true 和带标记的 scrollback 在同一个对象里。对模型而言这个标记是冗余的;显示侧三个视图只有一个剔除它——ptyTuiTerminalRows / ptyTuiTerminalView / ptyCompactTerminalLine 都不剔,所以 CLI transcript(pi-transcript-tools.ts:292/360/645/714)会把它当成一行真实输出渲染,还挤占 6 行预算。

改成不带前缀的 return { text: sliceUtf8Tail(text, budget), truncated: true },三个视图自动一致,PTY_TRUNCATED_MARKER 的导出和 stripTruncatedMarker 随之都能删——这正是同一个 PR 在 WebFetch 上采用的约定。若坚持保留标记,就把剔除放进 textRows(),让三个视图共用一次。

);
}

if (content.kind === 'summary') {

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.

P3 The new summary branch was inserted directly above the comment // image / summary / unknown — show a compact descriptor…, which is now wrong — summary no longer takes that path. Drop summary from the comment.

While here, content.original on this branch has no fallback. It's required by the type, but since the path is new it's cheap to guard alongside the malformed-result fixes in the review body.

中文

新增的 summary 分支插在了注释 // image / summary / unknown — show a compact descriptor… 的正上方,这条注释现在是错的——summary 已经不走那条路径了。把 summary 从注释里删掉。

顺带:这个分支上的 content.original 没有兜底。类型上它是必填,但既然这条路径是新加的,跟正文里那两处畸形结果的修复一起加个兜底很省事。

@github-actions github-actions Bot added effort/XXL Over 2500 readable lines and removed effort/XL Under 2500 readable lines labels Sep 12, 2026
Refine WebFetch citation cards, bounded text previews, shell tail-following, archived output access, and tool invocation labels across the Desktop transcript surface.

Generated-by: OpenAI Codex
Tighten archived output validation and cache lifecycle, preserve Session-scoped preview and scroll intent, harden persisted result rendering, and carry redirected WebFetch sources. Stabilize the archived-startup E2E around derived title effects.

Generated-by: OpenAI Codex
@colaforniaw
colaforniaw force-pushed the feat/tool-output-rendering branch from 9169996 to 8828cbb Compare September 13, 2026 09:12
@colaforniaw

Copy link
Copy Markdown
Contributor Author

Closing this without merging.

In hindsight this PR bundles too many independent concerns — archived full-output access (new IPC + validation), WebFetch citations, shell tail-following, text preview bounding, tool labels, and typography — which makes it fragile to land and hard to review meaningfully, and the overall value doesn't justify that risk in one shot.

I'll follow up with a better sequence instead:

  1. A no-behavior-change refactor that restructures how tool results are rendered (per-tool renderers behind a registry, shared bounded-preview primitives), so future changes touch one tool at a time.
  2. The individual improvements as separate, single-concern PRs on top of that base — typography, WebFetch citations, shell tail-following, etc. — each small and independently revertable.
  3. The archived full-output viewer as its own dedicated PR, since it deserves focused review of the IPC surface and validation.

Thanks for the early looks — the feedback here will fold into the split-up work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XXL Over 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(desktop): tailor tool result rendering to the content

4 participants