fix(site/src/pages/AgentsPage/components): make timeline blocks carry their tool by DanielleMaywood · Pull Request #27593 · coder/coder

DanielleMaywood · 2026-07-28T15:41:30Z

The timeline resolved tool ids against a Map and handled the miss three separate times:

tool arm: toolByID.get(block.id), then if (!tool) returning either a hand-built streaming placeholder or null.
tool-group arm: block.ids.map(get).filter(defined), then if (!firstGroupTool) return null.
After the block loop: a second pass over remainingTools, every tool whose id had no block, rendered appended at the end of the timeline.

Three answers to one question, and the third one rendered tools out of chronological position.

Change

toTimelineBlocks already builds that Map, so it now resolves ids and returns blocks that carry their tool:

type TimelineBlock =
	| (Exclude<RenderBlock, { type: "tool" }> & { sourceIndex: number })
	| { type: "tool"; tool: MergedTool }
	| { type: "unresolved-tool"; id: string }
	| { type: "suppressed-tool"; id: string }
	| { type: "read-files"; tools: readonly [MergedTool, ...MergedTool[]] };

A tool with no block cannot reach the renderer, so all three handlers go. The one reachable miss, a tool-result part creating a block before its call arrives, is its own variant rather than a fabricated MergedTool, which the never check on the switch forces the renderer to handle.

The non-empty tuple is load-bearing rather than decorative, and the run accumulator carries it too, so deleting the guard in flushReadFileRun is a type error rather than a silent [undefined]. Verified: changing the group construction to tools: [] fails with Type '[]' is not assignable to type 'readonly [MergedTool, ...MergedTool[]]'. That is what removes if (!firstGroupTool) return null. ReadFilesTool now takes the same tuple, which removes its hasContent = items.length > 0.

unresolved-tool is emitted whether or not the stream is live, and the arm renders nothing once it has settled, which takes isStreaming out of the transform entirely. The arm renders its own ToolCall row rather than dispatching <Tool name="Tool">, which showed the user the word Tool.

Non-tool blocks carry their index in blocks rather than their index in the timeline. blocks only ever appends or rewrites its last element, so that index never moves, while a read-files run collapsing three timeline entries into one shifts every later timeline index and remounts trailing response and thinking rows, resetting the smoothing buffer or a disclosure the user had expanded.

TimelineBlock is local and unexported, so this is confined to the render layer. RenderBlock and every producer of it are untouched.

`status` can say it does not know

MergedTool.status had three values for four wire states. A call with a result is completed or error; a call without one is running only while getPendingToolCallIDs says the chat is running or requires_action, and completed otherwise. So "we do not know whether this finished" was spelled completed, and every predicate asking whether a call had settled was choosing which half to get wrong: three consecutive attempts did, each in a different cell.

It gains a fourth member:

/** `unknown`: a call exists, no result arrived, nothing is left to produce one. */
export type ToolStatus = "completed" | "error" | "running" | "unknown";

MergedTool stops re-declaring the union, which was a second copy of the same defect. isSettledToolStatus names the grouping that presentation depends on, because four call sites read === "completed" positively: SubagentTool's label phase and its computer-use arm, and AskUserQuestionTool's answered state and interactivity. Without it a persisted result-less spawn would have flipped from Spawned X to Spawning X…, which is what SubagentSpawnWithNoResult pins.

The state is honest rather than displayed: unknown renders exactly as completed did. Its reachability is bounded on the server, which commits a call and its result in one CommitStep, blanks a call whose args never formed durable JSON, synthesises IsError cancellations on interrupt, and hard-fails FinishInterruption while a call remains outstanding. Giving the state its own affordance would be designing for something those guards prevent.

Behaviour changes

Three, all intended, which is why this is typed fix rather than refactor.

An execute row appears while its command is still streaming. shouldRenderTool refuses an execute row until its command finishes streaming, while buildStreamTools has already produced a running tool, which suppresses the shimmer. Measured on a single first args fragment {"comm: no row, no spinner, no shimmer, textContent === "". The placeholder covers the interval between the tool call appearing and its arguments having streamed far enough to yield a non-empty command, that is, every chunk up to and including the first one that carries a character of the command value. The window does not depend on command length, but it extends across any keys the model emits before command. It used to be far wider: the parser dropped a partial string value whenever whitespace followed the colon, so a pretty-printed payload showed the placeholder for 62 to 98 percent of the args stream and the command appeared only at its closing quote. streamingJson.ts now skips to the value, which brings that back to 6 to 8 percent. isExecutePendingCommand routes exactly those blocks to unresolved-tool, which fills it.
An execute that never got a command now shows its error. chattool/execute.go:140-142 rejects an empty command with command is required. That tool has no command for the whole stream, so it used to render Waiting for tool details… while the stream lived and nothing at all once it settled: the user asked for something, the agent explained why it could not, and the transcript was empty. A row that errored is never pending, which is how that error reaches the transcript.
A hidden metadata-only user message no longer settles an ask-user-question. The answer form stays interactive, because a context-file-only reply is not an answer.
The word Tool is gone from the unresolved row, which previously dispatched <Tool name="Tool">.

isExecutePendingCommand is deliberately narrower than shouldRenderTool: an execute with no command, no result, and no error. Both of the latter are load-bearing. A non-error result means the row has something to show even without a command, and an is_error part carrying an empty result_delta settles as error with result === undefined, which is how that error reaches the transcript. Routing every non-renderable tool to unresolved-tool made a running wait_agent render Waiting for tool details… during the window shouldRenderTool exists to keep blank, and chat_id is that tool's only argument, so every live call spends its argument chunks in it. A deliberately suppressed row becomes suppressed-tool instead, which carries no tool, so the arm that renders it has nothing it could render. <Tool>'s own shouldRenderTool guard is deleted with it, along with the story that existed to prove the guard dropped a row: suppression happens once, in the transform, and messageHelpers and streamingActivity read the variant rather than re-deriving visibility.

Subagent lifecycle rows get silence rather than a placeholder, because generic lifecycle copy is wrong until the transcript resolves the real title. That leaves a blank window for a live wait_agent, message_agent, close_agent or interrupt_agent, which is a deliberate trade recorded at the predicate.

Same shape, three more places

read-files covers one file too. The transform partitioned read_file tools and then discarded the result: a lone read file was emitted as a plain tool block, so the render arm asked tool.name === "read_file" again and rewrapped it, and ReadFileTimelineBlock split on tools.length === 1 a third time. Emitting read-files unconditionally leaves that decision in one place and makes the <Tool> arm unconditional.
buildDisplayMessages is the only hide authority. It already dropped every hidden entry, and displayMessages is what the timeline renders, so three downstream shouldHide re-derivations were dead: the ChatMessageItem early return, the user render arm, and visibleUserMessageIds, which reached back past the filter to parsedMessages to redo it. shouldHide is gone from MessageDisplayState, and HiddenTimelineEntryReason lost its last possible reader and collapses into the boolean predicate.
lastDisplayBlockIsThinking was read only inside case "thinking":, where index === displayBlocks.length - 1 already proves the last block is this thinking block.

Merging cannot produce a hidden entry after the filter: read-file groups are assistant-only, the metadata and provider-result predicates read unchanged message content, and hasRenderableContent is monotone over a superset of blocks and tools.

Both mergers walk blocks

buildStreamTools and mergeTools were the two producers of MergedTool[], and both minted tools the block list had never heard of. buildStreamTools iterated toolResults keys; mergeTools emitted calls and then swept up orphan results. Both now iterate blocks and resolve each id, so a tool cannot exist without a row, one id cannot produce two rows, and mergeTools loses its seen set and its second pass. The one production call site already had parsed.blocks in hand.

Provider-chosen keys are read through one ownValue helper rather than bracket indexing: the stream call and result maps, both subagent name maps, and the renderer registry. A tool named after an Object.prototype member threw while building its subagent descriptor and took the whole route down with it, not just the row.

The id space needs one more guard downstream. parseMessageContent falls back to tool-call-${index} when a call arrives without an id, which is unique per message, and mergeReadFileMessageGroup concatenates tools across messages, so one merged entry could hold two tools claiming one id. A last-wins Map then rendered the same file twice and dropped the other. Blocks and tools are paired positionally instead, since both producers emit tools in block order, and ReadFilesTool keys its rows by position too, so two tools sharing an id can no longer collide into one React key and one expansion state.

Generic thinking reads the same timeline variants, so a block still waiting for its tool does not draw a placeholder row and a shimmer for the same work, while a suppressed block, which draws nothing, no longer stands in for a running row.

Dead render paths, deleted rather than deferred

Making the <Tool> arm unconditional leaves read_file: ReadFileRenderer unreachable. getFileContentForViewer went with it: its only caller is GenericToolRenderer, which the registry keeps both execute and read_file away from, so the function always returned null. ReadFileTallAndWide now mounts ReadFileTool directly, keeping the both-axis overflow coverage.

The same argument reaches further. ToolLabel renders only from GenericToolRenderer and one literal name="advisor", so 14 of its 18 arms cannot be reached; only process_signal, which is registered but delegates to the generic renderer, process_list, attach_file and advisor survive. ToolIcon's read_skill_file arm is unreachable because ReadSkillTool hardcodes iconName="read_skill", and its case "unknown" was a useless clause the linter already flagged.

The structured external auth payload never shipped

wait_for_external_auth and the auth_required / authenticate_url / provider_* execute payload were added in bee184b455 and removed in fa1dee102e three days later, both on the branch that became #22290. The squash merge edee917d88 therefore carried only the frontend halves. git log -S for either string on main, restricted to Go files, returns zero commits ever, and no coderd/x/chatd tool registration or renamed equivalent exists.

The external auth flow itself is live, just unstructured: chattool/execute.go:146 sets CODER_CHAT_AGENT=true and cli/gitaskpass.go:101-104 prints the URL and instructions to stderr, which the execute transcript shows as plain output. What never existed is the structured tool and payload these components were built to render.

So WaitForExternalAuthTool, ExecuteAuthRequiredTool, toProviderLabel, the payload fields on ExecuteRenderData, the registry entry, the icon arm and the five stories are deleted. The residual risk is an MCP server registering a tool literally named wait_for_external_auth, which now renders through GenericToolRenderer instead.

Size

+997 / -1412 across 34 files, and -512 net in production code, largest first: ExecuteTool.tsx -143, ToolLabel.tsx -113, ConversationTimeline.tsx -110, Tool.tsx -89, utils.ts -64, messageHelpers.ts -31, toolVisibility.ts -21, ReadFilesTool.tsx -8, ToolIcon.tsx -4, streamState.ts -4, LiveStreamTail.tsx -3, against +30 in blockUtils.ts where the resolution now lives, +18 in streamingActivity.ts, +9 in runtimeTypeUtils.ts for the shared ownValue, +8 in streamingJson.ts, and single-digit additions in WebSearchSources.tsx, SubagentTool.tsx, messageParsing.ts, AskUserQuestionTool.tsx, AttachmentBlocks.tsx and types.ts.

Testing

Existing grouping tests keep their inputs, which are still RenderBlock, and gain resolved expectations. Four behaviours are pinned at that boundary: emitting an unresolved block as unresolved-tool, the read-files collapsing, a deliberately suppressed lifecycle tool becoming suppressed-tool, and a non-tool block keeping its source index after a run collapses in front of it. The four deriveMessageDisplayState(...).shouldHide assertions moved onto buildDisplayMessages, minus the visible-execute case, which does not collapse read_file messages across another visible tool already covers, so the settled bit gained its own table instead: five rows that move status with result held fixed and result with status held fixed, which is what tells one candidate predicate from another. Dropping either conjunct fails exactly one row, and the reviewer-suggested status === "running" fails a third.

Seven stories added. EmptyToolResultBeforeItsCall renders the row from an empty result delta with no call, and fails when the arm returns null unconditionally. EmptyToolResultBeforeItsCallReconnecting covers the other direction, asserting the row's label is absent rather than its spinner, and fails when the isStreaming guard is deleted. ReadFileRunCollapseKeepsRowsMounted and its streaming twin resolve a second read so the run collapses under a thinking disclosure, a response, a file reference, an attachment and a sources row, and between them fail against a revert of any one of the seven sourceIndex render keys. They capture rows by position rather than by streamed text, so animation frames stretched to 250ms do not fail them. ToolNamedAfterObjectPrototypeMember renders <Tool name="valueOf">, and fails when the prototype guard is reverted. AskUserQuestionMetadataOnlyReply pins the settle change. ReadFileErrorStates covers a solo errored read and both generic error fallbacks, Failed to read file and Failed to read one or more files, neither of which had a story before.

Unit 3083 passed / 2 skipped across 199 files. tsc and Biome pass. AgentsPage stories: 941 of 942 passed, the exception being With Message History; Scroll To Bottom Button Works With Inverse Scroll fails intermittently in the same way. Both are red on this base as well as on this branch, and the inverse-scroll one is intermittent, which is why an earlier revision of this description named only the first. Every new test and story here was checked against the mutation it exists to catch, including the displayMessages revert, the prototype-key guard, each of the seven sourceIndex render keys individually, the result-only name, the duplicate id, each conjunct of the settled bit, mergeTools reverting to completed for a result-less call, SubagentTool reading === "completed" again, the read-file group inheriting the single row's expansion state, a dead thinking toggle, the parser's whitespace skip, the prototype-safe accumulator, the unresolved-block shimmer and a skipped ensureToolBlock.

One trade worth stating: buildStreamTools(state) moves the React-compiler memo key from (toolCalls, toolResults) to streamState, so it recomputes per streamed chunk. Measured at 8 microseconds for 200 tool rows, and the cache slot it feeds was already invalidated every chunk by blocks identity, so nothing downstream re-renders that did not before. The test block that asserted the old boundary is deleted rather than rewritten, because it never called buildStreamTools and its rewrite would have codified the recompute as a guarantee.

Supersedes #27590

That PR deleted the second pass alone. It left the other two miss handlers in place, so the impossible state stayed representable with its fallback removed, and it needed producer invariant tests to stand up. Encoding the pairing removes the need for both.

The historical path was already sound: every tools push pairs with ensureToolBlock on the adjacent line (messageParsing.ts:220-227, 241-248), and blocks is append-only, which is why the deleted second pass could not fire, since remainingTools was always empty at runtime.

The live path was not. A review measured that buildStreamTools minted a tool for every toolResults key without consulting blocks, so deleting ensureToolBlock at streamState.ts:147 produced a tool with no row and left all 3064 tests green. Both mergers now walk blocks, so that same deletion fails a test.

🤖 This pull request was created with Coder Agents.

… timeline blocks carry their tool The timeline resolved tool ids against a Map and handled the miss three times: an undefined branch plus a streaming placeholder in the tool arm, a filter and guard in the tool-group arm, and a second pass over tools whose id had no block. Three answers to one question. groupSequentialReadFileBlocks already builds that Map, so it now resolves ids and returns blocks carrying their tool. A tool without a block cannot reach the renderer, and a block whose tool has not arrived is decided once: pending while streaming, dropped once settled. The tool-group arm takes a non-empty tuple, so an empty group is a compile error rather than a runtime guard. TimelineRenderBlock was local and unexported, so this is confined to the render layer. RenderBlock, and every producer of it, is untouched.

…ages the only hide authority buildDisplayMessages already drops every hidden entry, so the timeline's three re-derivations of shouldHide were dead. Drop the field from MessageDisplayState and move its tests onto buildDisplayMessages. Also narrow ReadFilesTool's tools prop to the non-empty tuple its only caller already holds, which removes the tautological hasContent check.

… re-deriving what the block type already says The transform partitions read_file tools and then throws the answer away: a lone read file was emitted as a plain tool block, so the render arm asked the name again and rewrapped it, and ReadFileTimelineBlock split on length a third time. Emit read-files unconditionally so the <Tool> arm is unconditional. lastDisplayBlockIsThinking was read only inside case "thinking", where index === displayBlocks.length - 1 already proves the last block is this one. HiddenTimelineEntryReason had no reader left once the downstream shouldHide derivations went, so the predicate returns a boolean.

DanielleMaywood · 2026-07-28T20:10:53Z

/coder-agents-review

coder-agents-review · 2026-07-28T20:10:57Z

Chat: Review posted | View chat
Requested: 2026-07-30 10:36 UTC by @DanielleMaywood
Spend: $1150.59 / $1200.00

Review history

R1 (2026-07-28), 1 Nit, 1 P1, 2 P3, COMMENT. Review
R2 (2026-07-28): 21 reviewers, 9 Nit, 2 Note, 1 P1, 3 P2, 9 P3, 4 P4, COMMENT. Review
R3 (2026-07-29): 22 reviewers, 16 Nit, 3 Note, 1 P1, 4 P2, 19 P3, 5 P4, COMMENT. Review
R4 (2026-07-29): 22 reviewers, 20 Nit, 3 Note, 1 P1, 8 P2, 33 P3, 5 P4, COMMENT. Review
R5 (2026-07-29): 22 reviewers, 21 Nit, 3 Note, 1 P1, 12 P2, 42 P3, 6 P4, COMMENT. Review
R6 (2026-07-29): 22 reviewers, 21 Nit, 3 Note, 1 P1, 16 P2, 50 P3, 9 P4, COMMENT. Review
R7 (2026-07-29): 22 reviewers, 25 Nit, 3 Note, 1 P1, 21 P2, 57 P3, 12 P4, COMMENT. Review
R8 (2026-07-30): 22 reviewers, 25 Nit, 3 Note, 1 P1, 28 P2, 63 P3, 15 P4, COMMENT. Review
R9 (2026-07-30): 23 reviewers, 32 Nit, 3 Note, 1 P1, 32 P2, 78 P3, 18 P4, COMMENT. Review

deep-review v0.9.0 | Round 9 | be22640..d4ea4e7

Last posted: Round 9, 164 findings (1 P1, 32 P2, 78 P3, 18 P4, 32 Nit, 3 Note), COMMENT. Review

Finding inventory

Finding inventory: PR #27593

Findings

#	Sev	Status	Location	Summary	Round	Reviewer	Posted
CRF-1	P1	Author fixed (`3d72f6f`)	ReadFilesTool.tsx:22	PR title scope excludes a changed file, both `title` CI runs red	R1	Netero	Yes
CRF-2	P3	Author fixed (`3d72f6f`)	Tool.tsx:1069	This diff makes `read_file: ReadFileRenderer` unreachable, deferral has no ticket	R1	Netero	Yes
CRF-3	P3	Author fixed (`3d72f6f`)	blockUtils.ts:41	`groupSequentialReadFileBlocks` no longer describes what the function does	R1	Netero	Yes
CRF-4	Nit	Author fixed (`3d72f6f`)	blockUtils.ts:44	`isStreaming = false` default silently drops pending blocks when omitted	R1	Netero	Yes
CRF-5	P2	Author fixed (`bb3a363`); severity contested	blockUtils.ts:80	Dropping a settled unresolved block shifts indices, remounting index-keyed thinking/response blocks	R2	Netero P3, Takumi (evidence)	Yes
CRF-6	Note	Author accepted R3; closure unsound, see CRF-34	Tool.stories.tsx:35	`MCP Tool Completed` story fails on head and identically at base	R2	Netero, Bisky, Komugi, Nami, Chopper	Yes
CRF-7	Note	Author accepted R3; closure unsound, see CRF-34	Tool.stories.tsx:2333	`ReadFileTallAndWide` now mounts `ReadFileTool` directly, bypassing the timeline wrapper	R2	Netero	Yes
CRF-8	P2	Author fixed (`7e52684`), structural not the guard test	ConversationTimeline.tsx:474 (deleted)	Deleting `remainingTools` removes the last guard on the tool-to-block direction, which no type and no test covers	R2	Pariston P2, Razor P3, Bisky Note, Mafuuu Note, Meruem Note	Yes
CRF-9	Nit	Author contested R4 (2nd time)	ReadFilesTool.tsx:22	Tuple encodes one-or-more, the component's contract is two-or-more, so `Read 1 files` stays representable	R2	Mafuuu P3, Meruem P3, Knov P3, Razor P3, Leorio P3	Yes
CRF-10	P3	Author fixed (`bb3a363`)	blockUtils.ts:70	The pending placeholder is a fabricated `MergedTool` in the resolved variant, indistinguishable from a real tool	R2	Meruem P3, Knov P3, Pariston Note, Luffy Note	Yes
CRF-11	P2	Author fixed (`bb3a363`)	Tool.stories.tsx:2333	The comment justifying the story's new shape states something the code does not do	R2	Gon P2, Leorio P3	Yes
CRF-12	P3	Author fixed (`bb3a363`)	Tool.stories.tsx:240 (deleted)	The single-file read_file error row now has zero story coverage	R2	Chopper P3, Luffy P3	Yes
CRF-13	P3	Author fixed (`7e52684`) by renaming, not extending	Tool.stories.tsx:3023	`AllToolIconsTranscript` maps through `<Tool>` rather than the real dispatch path, so it silently lost `read_file`	R2	Meruem P3, Zoro Nit, Kite Note, Hisoka Note, Melody Note, Nami Note	Yes
CRF-14	P3	Author fixed (`bb3a363`)	Tool.tsx:1029	The read_file exclusion is a runtime branch plus a single call site; a read_file reaching `<Tool>` now renders as JSON	R2	Mafu-san P3, Chopper Nit, Mafuuu Note	Yes
CRF-15	P3	Author fixed (`bb3a363`)	messageHelpers.ts:90	The hide authority is now an unenforced precondition and nothing at the definition says so	R2	Kite P3, Leorio P3	Yes
CRF-16	P3	Author fixed (`bb3a363`)	blockUtils.ts:39	The only doc on `toTimelineBlocks` documents its rarest branch and skips the contract	R2	Leorio P3, Gon Nit, Zoro Nit	Yes
CRF-17	P4	Author fixed (`bb3a363`)	utils.ts:378	`getFileViewerOptions` lost its last production caller in this diff and is still exported	R2	Melody P3, Pariston Nit, Gon Nit, Robin Nit, Chopper Note	Yes
CRF-18	P4	Author fixed (`bb3a363`)	ToolLabel.tsx:69	`case "read_file"` in `ToolLabel` is unreachable, along with 13 siblings	R2	Robin P3, Melody P4, Razor P4	Yes
CRF-19	P4	Author fixed (`bb3a363`)	ConversationTimeline.tsx:330	The new comment overclaims: a block that renders nothing still ends the thinking indicator	R2	Knov P4	Yes
CRF-20	P4	Author fixed (`bb3a363`, claim retracted from description)	AgentChatPageView.stories.tsx:1222	The scroll story the description cites as flaky-but-passing is red 4 of 4 on another machine	R2	Komugi P4	Yes
CRF-21	Nit	Author fixed (`bb3a363`)	blockUtils.test.ts:178	The `it.each` expected column is typed `unknown`, so expectations get no compile-time check	R2	Bisky, Hisoka, Ging-TS, Kite, Knov (Nit), Komugi Note	Yes
CRF-22	Nit	Author fixed (`bb3a363`)	blockUtils.test.ts:183	Two test names describe something other than what they assert	R2	Gon Nit, Bisky Note	Yes
CRF-23	Nit	Author contested; panel closed R3 (21/21 accept, raiser withdrew)	messageHelpers.test.ts:353	The `wait_agent` table row proves nothing the `execute` row above it did not	R2	Bisky	Yes
CRF-24	Nit	Author fixed (`bb3a363`) via CRF-10; literal relocated, see CRF-40	blockUtils.ts:74	`name: "Tool"` puts a display label in a wire-name field, fifth copy of the literal	R2	Gon Nit, Robin Nit	Yes
CRF-25	Nit	Author fixed (`bb3a363`)	utils.ts:282	`formatResultOutput`'s comments name two tools that cannot reach it	R2	Mafu-san Nit, Razor Nit	Yes
CRF-26	Nit	Author fixed (`bb3a363`)	blockUtils.ts:68	The pending placeholder row is the one user-visible state this PR moved, and no story renders it	R2	Nami	Yes
CRF-27	Nit	Author fixed (`bb3a363`, `grouped` only)	blockUtils.ts:47	`grouped` and `ReadFileTimelineBlock` are names left over from the pre-PR shape	R2	Gon	Yes
CRF-28	Nit	Author fixed (`bb3a363`)	Tool.tsx:881	The `output` intermediate is scaffolding left over from the branch this diff deleted	R2	Zoro	Yes
CRF-29	P3	Author contested; panel re-raised R4 as CRF-63	blockUtils.ts:59	The `read-files` collapse still changes `displayBlocks.length` when a pending tool resolves, so CRF-5's remount is still reachable	R3	Netero, Komugi (second route), Kite	Yes
CRF-30	P3	Author fixed (`7e52684`)	messageHelpers.ts:64	`visibleTools.length > 0` keeps an orphan-tool message alive through the hide filter, so it renders a blank gap	R3	Meruem P3, Luffy P2	Yes
CRF-31	P2	Author fixed (`7e52684`)	ToolLabel.tsx:5	The new doc comment is the deletion rule that replaced 14 arms and is false about the live `advisor` arm	R3	Gon P2, Leorio P2, Mafuuu P3, Meruem P3, Razor P3, Zoro P3, Mafu-san P3, Knov P3, Melody Nit	Yes
CRF-32	P3	Author fixed (`7e52684`, description corrected)	Tool.tsx:977	The "never shipped" premise is false as written: `execute.go:146` plus `gitaskpass.go:101` is a live producer of the flow	R3	Knuckle	Yes
CRF-33	P4	Author accepted R4 (Go/CLI, out of scope); no ticket	cli/gitaskpass.go:102	The surviving auth path prints literal `\n` from a raw string and its URL is never linkified	R3	Leorio P4, Luffy P4	Yes
CRF-34	P3	Author fixed (`7e52684`)	Tool.stories.tsx:21	The diff viewer's first mount in a page is what fails, so CRF-6 and CRF-7 were both closed on the wrong mechanism	R3	Komugi P2, Zoro Nit	Yes
CRF-35	P3	Author fixed (`7e52684`)	ConversationTimeline.tsx:354	The settled half of the `pending-tool` arm has no coverage; removing the guard leaves the whole suite green	R3	Bisky P3, Chopper P3, Kite Nit	Yes
CRF-36	P3	Author fixed (`7e52684`)	toolVisibility.test.ts:65	Deleting the auth case left `shouldRenderTool`'s execute branch pinned only negatively	R3	Chopper	Yes
CRF-37	P3	Author fixed (`7e52684`)	Tool.tsx:978	"Would only shadow that" is the wrong mechanism: a `read_file` entry would be dead, and `<Tool>` does render it generically	R3	Gon P3, Hisoka Nit, Melody Nit, Leorio Note	Yes
CRF-38	P3	Author fixed (`7e52684`)	ConversationTimeline.tsx:1107	`hasUserResponseAfterAskQuestion` reads the unfiltered list, so a hidden user row answers the question and removes the form	R3	Melody P2, Razor P3, Knov P3	Yes
CRF-39	P3	Author fixed (`7e52684`)	messageHelpers.ts:196	The CRF-15 comment states a rule about every consumer that its own calling file breaks twice, once correctly	R3	Razor P3, Knov P3, Melody Note	Yes
CRF-40	Nit	Author fixed (`7e52684`)	ConversationTimeline.tsx:357	CRF-24's literal moved to the render site rather than going away; still five copies	R3	Gon Nit, Hisoka Note, Zoro Nit	Yes
CRF-41	Nit	Author fixed (`7e52684`)	blockUtils.ts:37	`pending-tool` names a state that is false exactly where the variant earns its keep	R3	Gon	Yes
CRF-42	Nit	Author fixed (`7e52684`)	ConversationTimeline.tsx:357	The pending row's user-visible label is the bare word "Tool"	R3	Leorio	Yes
CRF-43	Nit	Author fixed (`7e52684`)	ConversationTimeline.stories.tsx:2599	`SingleReadFileErrorState` renders a pair too, and neither its name nor its doc says so	R3	Gon Nit, Leorio Nit	Yes
CRF-44	Nit	Author fixed (`7e52684`)	StreamingOutput.stories.tsx:288	`ToolResultBeforeItsCall`'s doc generalizes past the one branch it exercises	R3	Chopper Note, Kite Nit	Yes
CRF-45	P3	Author fixed (`7e52684`)	blockUtils.ts:41	The doc's "runs of consecutive read_file tools" reads as two or more; the function emits for a run of one	R3	Leorio	Yes
CRF-46	P3	Author fixed (`7e52684`)	blockUtils.ts:56	The producer-side non-empty tuple is held by a runtime `if`, not the type; deleting the guard type-checks clean	R3	Ging-TS P3, Zoro Note	Yes
CRF-47	Nit	Author contested R4	blockUtils.ts:38	The tuple type is spelled out in three files and the `read_file` literal in two layers, with no shared name	R3	Robin	Yes
CRF-48	Note	Author accepted R4 (deliberate, single-axis viewport lost)	Tool.stories.tsx:2344 (deleted)	`ReadFileLongLine`'s horizontal-only variant went with it; reviewers disagree on whether that is a gap	R3	Nami Note, Knov Note (contra)	Yes
CRF-49	P2	Author fixed (`2913f98`, block deleted)	streamState.test.ts:857	The `compiler cache guard simulation` block asserts the memoization boundary this PR removed and still passes	R4	Netero	Yes
CRF-50	P3	Author contested R5	streamState.ts:239	Taking the whole `StreamState` moves the compiler cache guard onto an object that is new every text chunk, so `buildStreamTools` runs per chunk instead of never	R4	Netero	Yes
CRF-51	P2	Author fixed (`2913f98`, assertion not recut)	messageParsing.ts:175	`mergeTools` keeps the result-only sweep `buildStreamTools` deleted, so CRF-8 is fixed on one producer of two	R4	Bisky P2, Mafu-san P3, Meruem P3, Robin P3, Ryosuke P3, Pariston Note, Mafuuu Note, Netero Note	Yes
CRF-52	P2	Author fixed (`2913f98`)	streamState.ts:258	The result-only tool's `name` is unpinned and it is the field that picks the renderer; the mutation is green	R4	Chopper	Yes
CRF-53	P3	Author fixed (`2913f98`)	streamState.ts:250	The block-id lookups read the prototype chain and take `id` from the map value instead of the block key	R4	Komugi P3, Meruem P3, Ging-TS P3, Knov P3, Gon Nit	Yes
CRF-54	P3	Author fixed (`2913f98`)	ConversationTimeline.tsx:360	A resolved tool whose row is not renderable leaves the live bubble blank for one args chunk on every execute call	R4	Nami	Yes
CRF-55	P3	Author fixed (`2913f98`)	ConversationTimeline.tsx:355	The `unresolved-tool` row lost the leading icon the old placeholder had, and the description says only the label changed	R4	Razor	Yes
CRF-56	P3	Author fixed (`2913f98`)	ConversationTimeline.tsx:357	`Starting tool call…` names the one state this arm cannot be in; both producers imply a result already arrived	R4	Leorio P3, Mafuuu Note	Yes
CRF-57	P3	Author fixed (`2913f98`)	ToolLabel.tsx:5	The doc's `GenericToolRenderer` rule omits the `isSubagentToolName` short-circuit that precedes the registry lookup	R4	Razor	Yes
CRF-58	P3	Author fixed (`2913f98`)	streamState.test.ts:736	All five `buildStreamTools` tests also pass against the deleted implementation, so the consumer side is unpinned	R4	Kite	Yes
CRF-59	P3	Author fixed (`2913f98`)	Tool.stories.tsx:41	The highlighter preload is scoped to one story file while its comment states a repo-wide rule, and the theme pair is respelled a fourth time	R4	Zoro P3, Robin P3, Kite P3, Mafuuu P4	Yes
CRF-60	P2	Author fixed (description)	AgentChatPageView.stories.tsx:1203	The body's verification line reports one red story; two are red, and the second is the one CRF-20 forced a retraction about	R4	Mafu-san	Yes
CRF-61	P3	Author fixed (`2913f98`)	ConversationTimeline.tsx:1108	CRF-38's fix changes which user replies settle an ask-question, with no story pinning it, and the body still frames the section as removing dead code	R4	Bisky P3, Mafu-san P3	Yes
CRF-62	P3	Author fixed (`2913f98`)	Tool.stories.tsx:3037	CRF-13's predicted drift is already present: `list_agents` has a registered renderer and no gallery entry	R4	Mafu-san	Yes
CRF-63	P3	Author fixed (`2913f98`, sourceIndex keys)	blockUtils.ts:59	CRF-29 re-raised on a third option: carry the source-block index and key off it, which closes it without content keys	R4	Hisoka P3, Pariston P3, Zoro P3, Ging-React P3 (vs 6 closing)	Yes
CRF-64	P3	Author contested R5	LiveStreamTail.tsx:145	`streamTools` is now a pure function of `streamState` and the two still travel as independent props through three components	R4	Meruem	Yes
CRF-65	P3	Author contested R5	streamState.ts:246	The live path resolves the same block ids twice per render, and the flat array between the passes is the shape this PR set out to delete	R4	Ryosuke	Yes
CRF-66	P3	Author fixed (`61530b4`)	streamState.ts:234	The new doc's first clause narrates the loop; the only thing the reader cannot get locally is why blocks drive the iteration	R4	Gon P2, Razor Nit, Leorio Nit, Takumi Note	Yes
CRF-67	Nit	Author contested R5	ConversationTimeline.tsx:355	The `unresolved-tool` row is the only transcript row without `data-transcript-row`	R4	Melody	Yes
CRF-68	Nit	Author fixed (`2913f98`)	StreamingOutput.stories.tsx:306	`Once the stream stops` describes a fixture whose stream is reconnecting, not stopped	R4	Komugi	Yes
CRF-69	Nit	Author fixed (`2913f98`)	ConversationTimeline.stories.tsx:2599	The renamed story's doc withholds the two error strings it asserts	R4	Leorio	Yes
CRF-70	Nit	Author contested R5	ConversationTimeline.tsx:361	`const tool = block.tool;` is scaffolding from the lookup the arm no longer performs	R4	Ryosuke	Yes
CRF-71	P2	Author fixed (isToolPendingArgs split)	blockUtils.ts:75	Routing every `!shouldRenderTool` block to `unresolved-tool` makes a running subagent lifecycle row show the generic copy its guard exists to suppress	R5	Netero	Yes
CRF-72	P3	Author fixed	ConversationTimeline.tsx:303	CRF-63's `sourceIndex` keys have no regression coverage: reverting all eight to the timeline index leaves the story file green	R5	Netero	Yes
CRF-73	Nit	Author fixed	ToolIcon.tsx:108	`case "unknown":` is a useless case clause and the project's own linter says so	R5	Netero	Yes
CRF-74	P2	Author fixed (ownValue at 6 sites + story)	Tool.tsx:1032	CRF-53's prototype-key fix stopped at two reads; the renderer registry lookup can still resolve `Object.prototype` and make `Object` a React component	R5	Leorio P3, Hisoka P3, Mafu-san P3, Komugi P3, Meruem P3, Knov P3, Robin P3, Razor P3, Kite P3, Melody P4, Ging-TS P4	Yes
CRF-75	P2	Author fixed (recut taken)	messageParsing.test.ts:633	The assertion that replaced CRF-51's recut is single-fixture and asserts a block ordering `mergeTools` does not maintain	R5	Bisky P2, Pariston P2, Mafu-san P3, Mafuuu P3, Kite P3, Meruem P3, Zoro P3, Razor P3	Yes
CRF-76	P2	Author fixed (routing split)	ConversationTimeline.tsx:355	The `unresolved-tool` arm hardcodes `status="running"` and a waiting label for a variant that now carries several distinct states	R5	Chopper P2, Gon P2, Nami P3, Meruem P3	Yes
CRF-77	P3	Author fixed	Tool.tsx:1034	`<Tool>`'s own `shouldRenderTool` guard can no longer fire in production, by the same argument this PR used to delete other unreachable paths	R5	Meruem P3, Robin P3, Zoro P3	Yes
CRF-78	P3	Author contested R6	ConversationTimeline.tsx:355	The CRF-54 row only exists while `liveStatus.phase` is streaming, so the blank gap reopens on reconnect and after settle	R5	Nami	Yes
CRF-79	P3	Merged into CRF-72 (same finding, unit and render level)	blockUtils.test.ts:152	The only `sourceIndex` assertion sits at the one position where source and timeline index coincide, so CRF-63's fix is barely pinned	R5	Bisky P3, Netero P3 (CRF-72)	No
CRF-80	P3	Author fixed	messageHelpers.ts:194	Three docs state invariants this diff changed: the hide-authority sentence, the `shouldRenderTool` definition doc, and the `unresolved-tool` gap-filling claim	R5	Gon P3, Meruem P3, Leorio Note	Yes
CRF-81	P3	Author fixed	StreamingOutput.stories.tsx:305	`EmptyToolResultBeforeItsCallSettled` asserts the spinner is gone rather than the row, so it passes on a row that still renders	R5	Chopper	Yes
CRF-82	P3	Author fixed (via CRF-75 recut)	blockUtils.ts:59	`new Map(tools.map(...))` is last-wins, so two tools sharing an id collapse and the non-empty tuple can hold the same tool twice	R5	Komugi	Yes
CRF-83	P3	Author contested R6	toolVisibility.ts:105	Answering "should this row render" builds the entire execute render payload, trimming the whole accumulated output on every call	R5	Nami	Yes
CRF-84	P3	Author fixed	blockUtils.ts:81	The read-file run accumulator copies the whole run per element, making `toTimelineBlocks` quadratic in run length	R5	Killua	Yes
CRF-85	P3	Author fixed (retyped fix, Behaviour changes section)	ConversationTimeline.tsx:354	The PR is typed `refactor` while its own body describes a user-visible change to this arm	R5	Pen Botter	Yes
CRF-86	P4	Author accepted R6 (deferred one PR, no ticket)	ConversationTimeline.tsx:222	`data-tool-call` has two producers and zero consumers, and marks what `data-transcript-row` already marks	R5	Meruem P4, Pen Botter P4	Yes
CRF-87	P3	Author fixed	streamState.ts:256	A stream whose only block is unresolved renders the placeholder row and the generic Thinking shimmer at the same time	R6	Netero	Yes
CRF-88	P4	Author fixed	ConversationTimeline.tsx:222	CRF-86 is deferred with no ticket, which by the review's own rule leaves it open	R6	Netero	Yes
CRF-89	P2	Author fixed	toolVisibility.ts:93	`isToolPendingArgs` has no terminal state and never reads `status`, so a settled command-less `execute` shows a waiting row forever and loses its error	R6	Hisoka P2, Pariston P2, Meruem P2, Mafuuu P3, Gon P3, Melody P3, Takumi P3, Nami P3	Yes
CRF-90	P2	Author fixed	toolVisibility.ts:102	CRF-83 re-raised: the predicate still builds the whole execute payload to read `args.command`, and this round put it on the per-chunk transform path	R6	Hisoka P2, Meruem P2	Yes
CRF-91	P2	Author fixed	toolVisibility.ts:108	The new doc sentence says a row waiting on its arguments is not hidden; `shouldRenderTool` returns false for exactly that row	R6	Gon P2, Mafu-san P2, Hisoka P3, Leorio P3, Zoro P3	Yes
CRF-92	P2	Author fixed	messageParsing.test.ts:504	The result-only `mergeTools` case asserts only `id` and `status`, so which side of `call ?? result` supplies `name` is unpinned; the mutation is green	R6	Bisky	Yes
CRF-93	P3	Author fixed	messageParsing.ts:139	The new doc's "one id cannot produce two rows" is false across messages, and the two rows were rendered	R6	Razor	Yes
CRF-94	P3	Author fixed	blockUtils.ts:57	CRF-82 is not closed: the tool map is still last-wins and `mergeReadFileMessageGroup` is a live producer of colliding ids	R6	Meruem P3, Kurapika P3	Yes
CRF-95	P3	Author fixed	toolVisibility.ts:121	`shouldRenderTool` branches on the literal `"execute"` rather than on `isToolPendingArgs`, so the two predicates can desync	R6	Meruem P3, Knov P3	Yes
CRF-96	P3	Author contested R7 (helper written and measured, declined on cost)	messageParsing.ts:141	`mergeTools` is now `buildStreamTools` written a second time in another file, and this review paid for the first one	R6	Robin P3, Razor P3	Yes
CRF-97	P3	Author fixed	blockUtils.ts:77	CRF-54's blank-gap fix covers `execute` and skips its three subagent siblings, so a live lifecycle call keeps the gap	R6	Razor	Yes
CRF-98	P3	Author fixed	ConversationTimeline.tsx:303	CRF-72's fix pins one of the five `sourceIndex` render keys; the other four revert green	R6	Nami	Yes
CRF-99	P3	Author fixed	blockUtils.test.ts:190	The only test pinning the pending-args route uses a settled tool, so the behaviour the body calls its headline is unpinned	R6	Hisoka	Yes
CRF-100	P4	Author contested R7	ConversationTimeline.tsx:350	The `read-files` row is keyed by its first tool's id, which moves when the run grows at the front	R6	Kite P4, Komugi P4	Yes
CRF-101	P4	Author contested R7 (browserslist is the stale half)	modules/roles/RoleSelector.tsx:59	`Object.groupBy` needs Safari 17.4 and this repo declares Safari 16.0	R6	Ging-TS	Yes
CRF-102	P3	Author contested R8 (implemented and measured the fix)	ConversationTimeline.tsx:350	The `read-files` key still moves when the run grows at the front, and the route that grows it is this PR's own `!tool` miss, not `isToolPendingArgs`	R7	Netero	Yes
CRF-103	P3	Author fixed	streamingActivity.ts:21	`hasRunningToolBlock` counts a block that renders nothing, so a live subagent lifecycle call shows an empty assistant bubble	R7	Netero	Yes
CRF-104	Nit	Author fixed	ConversationTimeline.tsx:356	`unresolved-tool` is now the only transcript row without `data-transcript-row`, after this round unified the other two	R7	Netero	Yes
CRF-105	Nit	Author contested R8 (doc fixed, move declined)	toolVisibility.ts:26	`getExecuteRenderData` has no visibility caller left, so a render helper lives alone in the visibility module	R7	Netero	Yes
CRF-106	P2	Author fixed	ExecuteTool.tsx:163	The commandless `execute` row this PR newly shows renders its headline as `Ran` with nothing after it, or `... using` ending on a preposition	R7	Leorio P2, Nami P2, Bisky P3, Mafuuu P3, Pariston P3, Kite P3	Yes
CRF-107	P2	Author fixed	toolVisibility.test.ts:65	CRF-89's fix has two halves and only the `toTimelineBlocks` half is pinned; `shouldRenderTool` reverts green across every unit test	R7	Bisky P2, Chopper P2, Mafu-san P2, Kite P3	Yes
CRF-108	P2	Author fixed	ReadFilesTool.tsx:51	CRF-94's fix made the two tools distinct in the timeline and left them sharing an id, so `ReadFilesTool` renders duplicate React keys and expanding one file expands the other	R7	Ging-React P2, Hisoka P3, Chopper P3, Meruem P3, Zoro P3	Yes
CRF-109	P2	Author fixed	ConversationTimeline.stories.tsx:2585	The streaming collapse story asserts on text a requestAnimationFrame clock reveals, so its outcome is set by frame cadence	R7	Komugi	Yes
CRF-110	P2	Author fixed	blockUtils.ts:102	The `tool` variant does not imply a rendered row, so `TimelineBlock` carries a weaker invariant than the PR claims	R7	Ryosuke	Yes
CRF-111	P3	Author fixed	toolVisibility.ts:105	`isToolPendingArgs` uses `result === undefined` as the settled bit rather than `status`, which is not the same question	R7	Melody P3, Meruem P3, Knov P3, Razor P3	Yes
CRF-112	P3	Author fixed	toolVisibility.ts:106	The placeholder window is not the one args chunk the body measured; it lasts until the whole command has streamed	R7	Pariston P3, Chopper P3, Razor P3	Yes
CRF-113	P3	Author fixed	blockUtils.ts:86	The duplicate-id queue pairs the Nth block with the Nth tool of the same id, so `tools` must arrive in block order and nothing states it	R7	Meruem	Yes
CRF-114	P4	Author fixed	streamingActivity.ts:20	`hasRunningToolBlock` resolves block ids against tools a second time with a different rule	R7	Robin P3, Knov P4	Yes
CRF-115	P3	Author contested R8	ConversationTimeline.tsx:359	The pending row says `Waiting for tool details…` while already holding a finished sentence describing the call	R7	Leorio	Yes
CRF-116	Nit	Author fixed	toolVisibility.ts:95	`isToolPendingArgs` is named for every tool and answers only for `execute`	R7	Gon P3, Kite P3	Yes
CRF-117	P3	Author fixed	blockUtils.ts:45	Two docs on the transform are now false: the `unresolved-tool` condition and the claim that `<Tool>` is the only silent drop	R7	Gon P3, Leorio P3	Yes
CRF-118	Nit	Author fixed	ConversationTimeline.stories.tsx:2838	The story reading the new `data-transcript-row` wrapper falls back to the button when the wrapper is absent, so it cannot fail	R7	Bisky	Yes
CRF-119	P4	Author fixed	AttachmentBlocks.tsx:73	The `ownValue` enumeration misses two record lookups whose keys come from outside the frontend	R7	Melody	Yes
CRF-120	P4	Author fixed (`d4ea4e7`) via CRF-129	streamingJson.ts:343	`extractIncompleteStringContent` is called at an index still pointing at the whitespace before the value	R7	Chopper	Yes
CRF-121	P2	Author fixed (`d4ea4e7`)	toolVisibility.ts:105	CRF-111's fix landed as `status !== "error"`, so a completed command-less `execute` is pending forever and its whole message is hidden	R8	Netero	Yes
CRF-122	P3	Open (R8 routing error: never posted)	blockUtils.ts:82	The global positional cursor turns one out-of-order tool into every later tool block rendering nothing	R8	Netero	No
CRF-123	P3	Open (R8 routing error: never posted)	streamingActivity.ts:19	CRF-103's fix has no test at either level	R8	Netero	No
CRF-124	P3	Open (R8 routing error: never posted)	ExecuteTool.tsx:167	CRF-106's fix is the round's one user-visible string and no test or story renders it	R8	Netero	No
CRF-125	Nit	Open (R8 routing error: never posted)	blockUtils.ts:42	`suppressed-tool` carries an `id` nothing reads	R8	Netero	No
CRF-126	Nit	Open (R8 routing error: never posted)	ConversationTimeline.stories.tsx:2931	CRF-118's fix took one of three fallback selectors in the same file	R8	Netero	No
CRF-127	P2	Author fixed (`d4ea4e7`)	messageParsing.ts:173	`MergedTool.status` has no value for "we do not know if this finished" and the two producers disagree on it, which is why the settled bit keeps being wrong	R8	Hisoka P2, Mafu-san P2, Mafuuu P2, Nami P2, Melody P2, Meruem P2, Knov P2, Luffy P2, Knuckle P2	Yes
CRF-128	P2	Author fixed (`d4ea4e7`)	toolVisibility.test.ts:65	The suite asserts CRF-121's defect as a requirement, so the correct settled bit fails two tests and the wrong one passes	R8	Mafu-san P2, Bisky P2, Ging-React P2, Chopper P2, Kite P2, Razor P2	Yes
CRF-129	P2	Author fixed (`d4ea4e7`)	streamingJson.ts:343	The placeholder window is the whole command string because the parser drops a partial value after whitespace, which CRF-120 confirmed and declined	R8	Pariston P2, Komugi P2, Chopper P3, Luffy P3, Zoro P4	Yes
CRF-130	P2	Author contested R9 (fallback met: CRF-121/127/128 landed together)	blockUtils.ts:88	The one behaviour-changing slice is about 7% of the diff and has produced 8 of its 22 P2s, all after round 4, because it rides inside a 29-file refactor	R8	Kite	Yes
CRF-131	P2	Author fixed (`d4ea4e7`)	toolVisibility.ts:123	`shouldRenderTool`'s execute branch is unreachable from its only production caller, and its doc names two consumers this round deleted	R8	Zoro P2, Razor P3	Yes
CRF-132	P3	Author accepted R9 (constraints recorded for CRF-102)	ReadFilesTool.tsx:24	CRF-108's fix is correct only because CRF-102's remount discards the state, and CRF-102 is open	R8	Mafu-san P3, Hisoka P3, Knov P3, Bisky P3	Yes
CRF-133	P2	Author fixed (`d4ea4e7`)	toolVisibility.ts:105	Seven comments added or amended this round give the wrong reason, restate the code, or name deleted consumers	R8	Gon P2 x7, Leorio P3 x4, Robin P3	Yes
CRF-134	P3	Author fixed (`d4ea4e7`)	messageHelpers.ts:50	Which timeline variants render nothing is now answered in four places, and message visibility is a function of the whole transform	R8	Robin P3, Zoro P4, Meruem P3, Mafu-san P3, Nami P2	Yes
CRF-135	P3	Author fixed (`d4ea4e7`)	ConversationTimeline.tsx:216	`ReadFileTimelineBlock` holds one `expanded` state and hands it to two different disclosures, and swaps components under it when a run grows	R8	Hisoka P3, Melody P3	Yes
CRF-136	P3	Author fixed (`d4ea4e7`)	ConversationTimeline.stories.tsx:2587	CRF-109's fix removed the last assertion that the thinking disclosure ever opened	R8	Mafu-san	Yes
CRF-137	P3	Author contested R9 (row green at both revisions)	messageHelpers.test.ts:343	An `it.each` row asserts the behaviour CRF-121 calls a bug, for the twin case that differs only by a field	R8	Chopper P3, Razor P3	Yes
CRF-138	P3	Author fixed (`d4ea4e7`)	blockUtils.ts:82	`tools[cursor]` types as `MergedTool`, so the `candidate?.id` guard is invisible to the compiler	R8	Ging-TS	Yes
CRF-139	P4	Author fixed (`d4ea4e7`)	streamingJson.ts:354	The `ownValue` sweep covered reads and left the one write whose key comes from the model	R8	Mafuuu P4, Zoro P4	Yes
CRF-140	P4	Author contested R9 (pre-existing, no issue filed)	ToolIcon.tsx:92	`stop_workspace` is a registered backend tool with no entry in any of the three per-tool frontend tables	R8	Melody	Yes
CRF-141	P4	Author contested R9 (severity raised, design choice, no issue filed)	streamingJson.ts:428	Streaming a 276KB tool-args payload burns 4.2 seconds of parsing	R8	Killua	Yes

| CRF-142 | P3 | Open | SubagentTool.tsx:264 | CRF-127's widening converted four positive === "completed" reads and left two in the same file plus one in ProposePlanTool.tsx | R9 | Netero | Yes |
| CRF-143 | P3 | Open | AskUserQuestionTool.tsx:403 | Three of the four isSettledToolStatus conversions differ from the literal only at "unknown", which no fixture produces, so reverting them is green | R9 | Netero | Yes |

| CRF-144 | P2 | Open | toolVisibility.ts:74 | isExecutePendingCommand classifies the new unknown status as pending, so a settled command-less execute takes its whole assistant message off the transcript, and the round's own test row pins it | R9 | Hisoka P3, Kite P3, Meruem P3, Knov P3, Pariston P3 | Yes |
| CRF-145 | P2 | Open | toolVisibility.ts:59 | CRF-133's rewrite dropped the status !== "error" conjunct from the doc, so the doc says an errored command-less row is pending and the code says it is not | R9 | Gon P2, Leorio P3, Mafu-san P3, Mafuuu P3 | Yes |
| CRF-146 | P2 | Open | streamingJson.ts:345 | CRF-129's whitespace skip moves pretty-printed tool args onto the quadratic parse path, measured 2.7x to 3x slower than base, and CRF-141 has no owner | R9 | Takumi P2, Killua P2, Pariston P2, Mafu-san P3 | Yes |
| CRF-147 | P2 | Open | WebSearchSources.tsx:79 | A provider-supplied source URL reaches href with no scheme check, so a javascript: citation runs in the dashboard origin on click | R9 | Kurapika P4, orchestrator raised | Yes |
| CRF-148 | P3 | Open | utils.ts:11 | isSettledToolStatus returns false for error, the most settled status there is, and neither the name nor the doc says so | R9 | Gon P3, Leorio P3, Mafuuu P3 | Yes |
| CRF-149 | P3 | Open | toolVisibility.ts:78 | shouldRenderTool now answers only the subagent-lifecycle question while keeping the general name, and the same commit deleted its doc | R9 | Mafuuu P3, Zoro Nit, Knov Nit | Yes |
| CRF-150 | P3 | Open | toolVisibility.ts:76 | The execute-command normalization is written twice in one file 46 lines apart, it has already diverged once inside this PR, and the predicate's .trim() is unpinned | R9 | Robin P3, Bisky P3, Knov P3, Zoro Nit | Yes |
| CRF-151 | P3 | Open | messageHelpers.ts:50 | The two non-render consumers of TimelineBlock have no exhaustive guard and default in opposite directions, and the doc sentence that made the filter correct was deleted this round | R9 | Zoro P3, Mafu-san P3, Leorio P3 | Yes |
| CRF-152 | P3 | Open | utils.ts:176 | The rewritten subagent-status branch changes behaviour only for an unreachable state, no test pins it, and the comment above it describes the rule the code no longer implements | R9 | Bisky P3, Gon P3, Leorio P3, Razor Nit, Zoro Nit, Nami Nit | Yes |
| CRF-153 | P3 | Open | blockUtils.test.ts:125 | The fixture pinning "no row until the command has streamed" holds an args shape this round's parser change made unreachable, so the row passes for the wrong reason | R9 | Chopper P3, Hisoka P3 | Yes |
| CRF-154 | P3 | Open | ConversationTimeline.stories.tsx:2595 | expectRowsSurviveCollapse guards its row window with an assertion that restates slice, so appending one block silently disarms the sourceIndex key checks | R9 | Komugi P3 | Yes |
| CRF-155 | P3 | Open | blockUtils.ts:86 | unresolved-tool has two producers and the pending-args route erases the resolved tool it is holding, which is why CRF-115 looks unfixable | R9 | Ryosuke P3, Meruem P3, Knov P3 | Yes |
| CRF-156 | P3 | Open | toolVisibility.ts:62 | The predicate reads only args.command while the row renders from intentLabel \|\| summary \|\| command, so an execute carrying model_intent is routed to the variant that cannot render it | R9 | Knov P3 | Yes |
| CRF-157 | P3 | Open | types.ts:3 | The conversation data model now imports its status vocabulary from a 633-line presentation module, so the two directories import each other | R9 | Ryosuke P3 | Yes |
| CRF-158 | P3 | Open | streamingActivity.ts:17 | shouldShowGenericThinking derives the timeline a second time from a different block list than the one BlockList renders | R9 | Ryosuke P3 | Yes |
| CRF-159 | P3 | Open | messageParsing.ts:175 | unknown is the else-branch of a scan that declines to answer in three cases, so it means "the client could not tell", not "nothing is left to produce a result" | R9 | Pariston P3 | Yes |
| CRF-160 | P3 | Open | streamState.ts:237 | Both producer docs claim a tool cannot exist without a row to render it, which suppressed-tool falsified two commits later | R9 | Razor P3 | Yes |
| CRF-161 | P3 | Open | toolVisibility.ts:30 | Round 9 added a fourth user-visible change, the command trim, and the body still opens "Three, all intended" over four bullets | R9 | Mafu-san P3 | Yes |
| CRF-162 | P4 | Open | messageParsing.ts:169 | completed is assigned on the presence of a result part rather than a result, so completed with result === undefined still means what unknown was added to name | R9 | Chopper P4, Bisky, Meruem, Pariston | Yes |
| CRF-163 | P4 | Open | chatStatusHelpers.ts:64 | The ownValue sweep covered the enumerated sites and left PROVIDER_STATUS_URLS[normalized], whose key comes from server config | R9 | Kurapika P4, Robin P4 | Yes |
| CRF-164 | P4 | Open | ConversationTimeline.tsx:216 | CRF-135's fix means a read-file disclosure the user opened closes itself when the next read lands, and the behaviour list does not name it | R9 | Nami P4 | Yes |
| CRF-165 | Nit | Open | ConversationTimeline.tsx:351 | iconName="unknown" is a sentinel whose ToolIcon case CRF-73 deleted, and unknown now also names a ToolStatus | R9 | Gon Nit, Knov Nit, Ryosuke Nit, Zoro Note | Yes |
| CRF-166 | Nit | Open | ConversationTimeline.tsx:323 | This round's data-transcript-row sweep marks 6 of the 9 timeline variants and stops three rows short | R9 | Melody Nit | Yes |
| CRF-167 | Nit | Open | ConversationTimeline.tsx:213 | ReadFileTimelineBlock is singular for a variant this PR made unconditionally plural | R9 | Gon Nit | Yes |
| CRF-168 | Nit | Open | Tool.tsx:979 | The read_file absence note sits above the execute entry, so it reads as documenting execute | R9 | Gon Nit | Yes |
| CRF-169 | Nit | Open | messageParsing.ts:163 | // Extract model_intent from the tool call args if present. restates the two lines under it in the same identifiers | R9 | Gon Nit | Yes |
| CRF-170 | Nit | Open | ConversationTimeline.stories.tsx:2525 | A fourth read_file MergedTool factory joins the three already in this directory, with storyFixtures.ts already the shared module | R9 | Robin Nit | Yes |
| CRF-171 | Nit | Open | Tool.tsx:50 | ownValue has two import paths and the sweep used both, so the re-export comment and the six direct imports contradict each other | R9 | Ryosuke Nit | Yes |

Contested and acknowledged

CRF-8 (P2, ConversationTimeline.tsx:474 deleted) - tool-to-block direction unpinned

Finding: The deleted remainingTools pass was the last guard on the tool-implies-block
direction. The new type covers block-implies-tool only, so a tool whose id has no block is now
silently unrendered and no test asserts the pairing. Asked for one assertion per producer.
Author defense (R3): The two cited orphan tests do not construct the orphan state.
messageParsing.test.ts:499 calls mergeTools([], [result]), which takes two arrays and no
blocks. streamState.test.ts:744 passes blocks: [] as type filler to
buildStreamTools(state.toolCalls, state.toolResults), which never receives it. The proposed
assertion is vacuous at all four named sites because parseMessageContent never populates
parsed.tools: emptyParsedMessageContent() sets tools: [] and the only assignment is
messageParsing.ts:367 inside parseMessages. The named producer tests already exact-equal the
whole block array for the id just pushed, which is stronger than a some() check. A sweep
appending block-less tools would be the deleted second pass in miniature. The real fix is putting
the tool inside RenderBlock, 13 files and 250 to 350 lines, with a design cost: parseMessages
back-patches results and killedBySignal from later messages, so blocks would stop being
immutable per message. Queued as its own change, no ticket linked.
Status: contested. No panel decision yet.
Netero, R3 (new evidence): two of the three defense claims check out.
parseMessageContent does leave tools: [] (messageParsing.ts:71) and the only write is
messageParsing.ts:367, so the round-2 assertion at the four named sites would assert over an
empty array, and the structural fix really is large. But that rules out the proposed placement,
not enforcement. Netero wrote and ran a non-vacuous guard on
parseMessagesWithMergedTools output, roughly 15 lines, which passes today and fails the moment
a producer pushes a tool without ensureToolBlock. The equivalent streaming-side guard would go
on buildStreamTools output; he did not write that one.

CRF-9 (P3, ReadFilesTool.tsx:22) - tuple admits one file, component needs two

Finding: readonly [MergedTool, ...MergedTool[]] admits length 1, at which the component
renders Read 1 files. Five reviewers proposed [MergedTool, MergedTool, ...MergedTool[]] plus a
destructure-and-guard at the caller, three verified it compiles.
Author defense (R3): noUncheckedIndexedAccess is off, so in
const [first, second, ...rest] = tools the binding second types as MergedTool, not
MergedTool | undefined, verified with const _probe: MergedTool = second; compiling clean. So
if (!second) is invisible to the compiler, and the proposed version compiles because the tuple
literal structurally satisfies the two-tuple, not because a second element was proved to exist.
Same runtime behavior, three more lines, no static guarantee. Suggests that if the plural copy is
the real objection, the fix belongs inside ReadFilesTool.
Status: contested. No panel decision yet.

CRF-23 (Nit, messageHelpers.test.ts:353) - duplicated table row

Finding: The wait_agent row lands on the same !hasRenderableContent branch as the
execute row above it, and each visibility rule is already pinned in toolVisibility.test.ts.
Author defense (R3): Concedes messageHelpers cannot distinguish the two rows, but they take
different branches inside shouldRenderTool (shouldRenderExecuteTool versus
shouldRenderSubagentLifecycleTool) and the table documents that cheaply. Deleting ten readable
lines is churn.
Status: contested. No panel decision yet.

CRF-13 (P3, Tool.stories.tsx:3146) - showcase maps through `<Tool>`

Finding: AllToolIconsTranscript maps items straight into <Tool> rather than the app's
dispatch path, so it silently lost read_file and will lose the next tool that leaves the
registry, with no check.
Author accepted (R3): Agrees with the diagnosis. Declines the fix because BlockList reads
shellToolDisplayMode and codeDiffDisplayMode from preferenceSettings() rather than props, so
restoring the pinned always_collapsed behavior needs a ["me","preferences"] query fixture,
roughly 25 lines to bring back two icons. Points at the new SingleReadFileErrorState as covering
the same render path with a test that asserts something. Says the catalogue-completeness point
wants its own change. No ticket linked.

CRF-6 and CRF-7 (Notes, Tool.stories.tsx)

Author accepted (R3): CRF-6 is the @pierre/diffs first-mount shadow-root behavior and out of
scope. On CRF-7 he reports trying the separate ReadFileTool.stories.tsx and reverting it,
because whichever of the two stories ran first failed at 1s, 5s and 15s with the shadow root
holding 17 empty rows, so they stay in a file where an earlier story has already mounted the
viewer.

CRF-130 (P2, blockUtils.ts:88) - split the behaviour-changing slice out

Finding: Pending-command behaviour is about 7% of the diff and produced 8 of the PR's 22
P2s, all after round 4. Proposed four pieces: the ownValue crash fix, the structural work,
the dead-code deletions, and the pending-command behaviour, with the status model fixed at the
type in the last one. Stated fallback if the split is declined: CRF-121, CRF-127 and CRF-128
must land together.
Author defense (R9): Declines the split and challenges the premise. Piece 4 was diagnosed
as unfixable at the predicate level; measured, the fix was one conjunct with zero test churn,
and the fourth status produces an identical output matrix, so it is modelling rather than
behaviour. Reports the fallback condition met: CRF-121, CRF-127 and CRF-128 all land in
d4ea4e733. CRF-141 moved out of the PR for a benchmarked reason rather than scope.
Status: contested. No panel decision yet.

CRF-132 (P3, ReadFilesTool.tsx:24) - position key safe only because of the CRF-102 remount

Finding: CRF-108's positional expansion key is correct only because CRF-102's remount
discards the state. Fixing either one alone breaks the other. Asked for the coupling to be
recorded, not fixed.
Author accepted (R9): Simulated the CRF-102 fix by making the group key stable across
front growth, re-ran the probe, and observed the expansion move off b.ts onto a.ts, off by
one as predicted. Records two constraints for whoever takes CRF-102: the inner key cannot be
the list position, because it must survive a front shift, and it cannot revert to tool.id,
because a merged read-file message can carry two tools with one id. No ticket linked.

CRF-137 (P3, messageHelpers.test.ts:343) - `it.each` row pins CRF-121's defect

Finding: The row asserts the behaviour CRF-121 calls a bug, for the twin case that differs
only by a field, so fixing the predicate should turn it red.
Author defense (R9): git diff 964dc..HEAD shows the row unchanged and passing at both
revisions, so it pins a settled command-less execute with no result, which both candidate
predicates treat identically and which HiddenAssistantToolMessageDoesNotRenderGap
independently requires. It would have gone red under the status === "running" form, which
CRF-128's new table rejects.
Status: contested. No panel decision yet.

CRF-140 (P4, ToolIcon.tsx:92) - `stop_workspace` missing from all three per-tool tables

Finding: A registered backend tool has no icon, label or renderer entry, so it falls
through to generic handling.
Author defense (R9): Confirms the gap and adds list_subagent_models as a sibling.
Declines as pre-existing and unrelated to the diff. No issue filed, per a standing instruction
against tracking issues, so the gap stays open with no owner.
Status: contested. No panel decision yet.

CRF-141 (P4, streamingJson.ts:428) - quadratic streaming JSON parsing

Finding: Streaming a 276 KB tool-args payload burns 4.2 seconds of parsing.
Author defense (R9): Raises the finding above its filed severity. A bounded benchmark
against the real four-argument mergeStreamPayload signature measured 9.6 s for 130 KB at
16 B chunks and aborted above 20 s for 281 KB at 16 B and 64 B chunks, quadrupling per
doubling. Exposure is the OpenAI chat-completions and Responses paths. The one-line
closing-brace short-circuit saved about 10%, so the remaining options are a design choice:
incremental parser state, or stop progressive parsing above a size threshold. Declines an
in-PR fix and names a follow-up PR, with no issue filed and no PR number.
Status: contested. No panel decision yet.

Round log

Round 1

Netero-only (P1 gate). 1 P1, 2 P3, 1 Nit. Panel not spawned. Reviewed against be22640..7fa49b3.

Orchestrator verification: confirmed .github/workflows/contrib.yaml:203-219 fails when any changed path
falls outside the title scope, and ReadFilesTool.tsx sits under ChatElements/tools/, outside
ChatConversation. Confirmed <Tool> has exactly one app call site (ConversationTimeline.tsx:364)
and that read_file can no longer reach it. Confirmed ToolCall.Root defaults hasContent = true
(ToolCall.tsx:98), so the hasContent removal in ReadFilesTool is behavior-preserving.

Round 2

Churn guard: PROCEED, 4 of 4 addressed. Netero (P3 gate passed) plus a 21-member panel: Bisky,
Hisoka, Mafu-san, Mafuuu, Pariston, Komugi, Ging-TS, Ging-React, Gon, Leorio, Nami, Melody, Chopper,
Kite, Meruem, Knov, Robin, Zoro, Razor, plus wildcards Luffy and Takumi. 3 P2, 7 P3, 4 P4, 8 Nit,
2 Note. Reviewed against be22640..3d72f6f.

CRF-5 severity raised P3 to P2 within the round it was created, before posting. Netero rated it P3 on
a single settle transition. Takumi traced the same root cause across deriveLiveStatus, showing the
isStreaming flip oscillates on every retry and reconnect while hasAccumulatedOutput keeps the same
BlockList mounted, so the remount fires repeatedly during a normal stream rather than once.

Orchestrator verification this round: queried the check-runs API and established that the title
failure predates the retitle (two later runs green) and that the test-js failure is an unrelated
SchedulePage waitFor timeout; reran pnpm test:ci on the head worktree (199 files, 3071 passed,
2 skipped). Confirmed streamState.ts:110-118 (result_reset) and :119-127 (empty delta) both call
ensureToolBlock without writing a tool, so CRF-5's precondition is reachable. Confirmed the index
keys at ConversationTimeline.tsx:303, 313, 325 and ReasoningDisclosure's manualToggle state.

Drop and downgrade gate, written before each decision:

CRF-17 kept at P4 rather than Melody's P3. Keep-at-P3 argument: the author deleted
getFileContentForViewer and its nine tests for exactly this reason, so leaving a sibling in the
same file means the class is half-fixed and knip cannot see it. Weighed against: the function is
still live through two internal callers, the only defect is an over-wide module surface, and four of
five reviewers rated it Nit or Note. P4.
CRF-18 kept at P4 rather than Robin's P3. Keep-at-P3 argument: read_file is unreachable in
ToolLabel partly because of this diff, and the PR's own standard is "deleted here rather than
deferred". Weighed against: 13 of the 14 dead cases were dead before this PR, so this is one
instance of a pre-existing class and the whole switch wants its own cleanup. P4.
CRF-13 kept at Meruem's P3 rather than the five Note ratings. Keep-at-P3 argument: the showcase's
<Tool>-mapping structure diverges from the app's real dispatch path, so it will keep silently
losing tools with no check to say so. The Note raters argued only that read_file coverage survives
elsewhere, which is about coverage, not about the mechanism. P3.
CRF-11 kept at Gon's P2 rather than Leorio's P3. Keep-at-P2 argument: the comment is the stated
justification for the story's new shape, it is false on arrival, and it sits exactly where CRF-7
identifies a coverage gap, so it tells the next reader the gap does not exist.
CRF-8 kept at Pariston's P2 rather than Razor's P3 or the two Note ratings. Keep-at-P2 argument: the
PR declines a producer test on the strength of a guarantee that covers the opposite direction, the
pairing is now held up only by adjacency in two files that change routinely, and a break drops a tool
row from the transcript with a green suite.

Structural alternative surfaced during cross-check: CRF-10's pending-tool variant, if emitted
unconditionally rather than only while streaming, also eliminates CRF-5 and removes isStreaming from
the transform. Verified against the code: retaining the block keeps displayBlocks the same length
across the isStreaming flip, which is the base behavior the index keys were written for.

Round 3

Churn guard: PROCEED, 18 addressed, 3 acknowledged, 3 contested, 0 silent. Netero plus a 22-member
panel: Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Komugi, Ging-TS, Ging-React, Gon, Leorio, Nami,
Melody, Chopper, Kite, Meruem, Knov, Robin, Zoro, Razor, Kurapika, plus wildcards Luffy and Knuckle.
2 P2, 12 P3, 1 P4, 7 Nit, 1 Note new or re-raised. Reviewed against be22640..bb3a363.

Infrastructure failure: the workspace agent went down partway through the panel and had to be
stopped and restarted, which killed every reviewer's in-flight work. All 22 were redirected and
re-ran their checks afterwards. Kite's file could not be written by the agent and was transcribed
verbatim from its report at its instruction. Several reviewers disclosed that individual runs died
under memory pressure and named which claims therefore rest on reading rather than running.

Panel adjudications on contested findings:

CRF-8 re-raised at P2, 17 of 17 reviewers who took a position. The defense had three claims. Two
hold and the panel says so: the round-2 assertion was vacuous at the four named sites, because
parseMessageContent leaves tools: [] and the only write is messageParsing.ts:367; and the
structural fix is genuinely large, because parseMessages back-patches results and
killedBySignal across messages. The load-bearing third claim, that any producer assertion is
vacuous, is refuted by running code. Mafu-san broke each of the six pairing sites in turn: five are
caught by existing tests and streamState.ts:147 is not, which is exactly the site where
buildStreamTools mints a tool for a result with no matching call. Razor simulated a conditional
regression (skip ensureToolBlock only for execute tool-calls) and 133 existing assertions
passed silently while his 14-line guard failed. Meruem and Luffy independently probed the
consequence and found it is not a silent row drop: hasRenderableContent counts tools, so the
message survives the hide filter and renders zero rows.
CRF-9 closed on the type, 19 of 21. noUncheckedIndexedAccess is off, verified independently by
seven reviewers with the project's own compiler, so the proposed two-tuple plus if (!second) buys
a prompt to write a runtime guard and not a proof. Knov and Mafu-san dissented, arguing the
two-tuple still buys a compile error at the ConversationTimeline.tsx:235 pass-through, which is
true. The panel's answer is that the risk it guards, someone deleting the tools.length === 1
branch, is already pinned by tests: Melody, Bisky and Chopper each disabled that branch and watched
SingleReadFileErrorState and ThinkingBlockWithToolCall fail, one with a message naming
Read 1 files. Downgraded to Nit for the plural copy, which is the author's own proposed fix.
CRF-23 closed, 21 of 21, and Bisky withdrew his own finding. The two rows do take different
branches of shouldRenderTool. Six reviewers noted the defense cites shouldRenderExecuteTool,
which this diff deleted and inlined; the branch survives, so the argument survives its citation.
CRF-13 re-raised at P3 with the cost objection refuted. Zoro found that AllToolIconsTranscript
already carries a parameters.queries array and already mounts BlockList, and that the fixture
literal exists verbatim three times elsewhere at 8 lines each, so the fix is roughly line-neutral
plus 8 lines rather than 25. Kite, Razor and Meruem each priced a cheaper detector that renders
nothing: assert every toolRenderers key appears in the showcase, with an allowlist for
read_file. Luffy dissented and would accept the gap.

Drop and downgrade gate, written before each decision:

CRF-34 taken at P3 rather than Komugi's P2. Keep-at-P2 argument: a check is red in CI right now,
the root cause is one missing synchronization, both CRF-6 and CRF-7 were closed on a mechanism
description her four -t runs show is wrong, and the red relocates onto whatever viewer story runs
first. Weighed against: her own control story, untouched by this PR, fails in isolation too, so the
axis predates the diff and nothing here creates or cures it. Taken at P3 because the actionable
content is that two of our own dispositions are unsound and ReadFileTallAndWide now carries an
undeclared ordering dependency.
CRF-38 taken at P3 rather than Melody's P2. Keep-at-P2 argument: she measured the consequence on
head with two stories, and a user who is asked a question loses the ability to answer it. Weighed
against: neither she nor Razor could find a producer of a persisted user message whose parts are all
context-file or skill, so the trigger is unproven, and the loop predates this diff. Razor also
argues the opposite fix, that reading the unfiltered list there is correct, so the semantics need
deciding before the one-line change is applied.
CRF-30 taken at Meruem's P3 rather than Luffy's P2. Keep-at-P2 argument: it is the mechanism that
turns CRF-8's break into something a user can see. Weighed against: it is unreachable while the
pairing holds, and Luffy's own trace shows the artifact is a blank gap rather than an empty bubble.
Kept at P3 and linked to CRF-8 rather than merged into it, because deleting the term is a class fix
that stands on its own.

Contradiction flagged for the author: Melody would convert ConversationTimeline.tsx:1107 to iterate
displayMessages and verified the 54 existing stories stay green; Razor argues that loop is right to
read the unfiltered list, because a hidden user reply would otherwise leave a settled question
interactive. Both agree the current state is not deliberate. Recorded in CRF-38 and CRF-39.

Round 4

Churn guard: PROCEED, 18 addressed, 2 acknowledged, 3 contested, 0 silent out of 23. Head
7e526844d6, round 3 reviewed bb3a363ed4. Delta bb3a363ed4..7e526844d6: 14 files, +138 -130.

Panel not yet run. The review is at its per-chat spend limit ($298.25 of $300.00 after the author
raised it from $100), and the review bot has already posted on the PR that further rounds are paused
pending a raise. Spawning a trigger-compliant panel would trip the limit mid-round, which is the
failure mode that cost round 3 its first panel pass. Held for an operator decision; the inventory is
current as of the churn guard so nothing is lost either way.

CRF-8's fix is structural rather than the requested test: buildStreamTools now takes StreamState
and iterates state.blocks, resolving each tool block id, and the result-only sweep over
Object.values(toolResults) is deleted. That makes an orphan tool unrepresentable on the live path,
which is the site Mafu-san measured as the only unpinned one in round 3. Whether it also covers the
mergeTools historical producer is the open question for whoever reviews next.

Three findings contested a second or first time: CRF-9 (plural copy, declined again with a new
argument that a two-plus tuple needs a cast because tools.length is typed number), CRF-29 (index
keys, declined with a per-variant key audit and an unreachability argument for both cited triggers),
CRF-47 (extractions declined as net additions). CRF-33 and CRF-48 are acknowledged without tickets.

Panel ran after all. 22 reviewers: Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Komugi, Ging-TS,
Ging-React, Gon, Leorio, Nami, Melody, Chopper, Kite, Meruem, Knov, Robin, Zoro, Razor, Killua, plus
wildcards Ryosuke and Takumi. 4 P2, 13 P3, 4 Nit.

Infrastructure: the workspace agent died repeatedly under the parallel load, mostly under storybook
runs. Eight reviewers (Hisoka, Mafuuu, Pariston, Ging-TS, Ging-React, Gon, Leorio, Melody) could not
write their files and were transcribed verbatim from their reports. Two left scratch files that the
orchestrator deleted (zzprobe.ts in the ging-ts worktree, three compile probes in ging-react's).
The remaining ten were told to retry the write and avoid further suite runs, and all ten wrote
successfully. Where a reviewer disclosed that a check did not complete, that disclosure is in their
file and no severity was raised past its evidence.

Panel adjudications on the three contested findings:

CRF-9 closed in the author's favour, unanimously among the twelve who took a position (Netero,
Hisoka, Mafuuu, Pariston, Gon, Leorio, Meruem, Mafu-san, Chopper, Kite, Knuckle-equivalent seats).
Leorio, who raised it in round 2, withdrew the ask himself: a pluralization ternary behind a branch
no caller reaches is live code guarding nothing. Several noted the smell that the decline changed
shape three times; the second reason is sufficient on its own, so it survives.
CRF-47 closed in the author's favour by everyone who took a position. Hisoka named the one half
worth revisiting: the tuple is linked structurally so drift is a compile error, but the read_file
literal in blockUtils.ts:76 and ToolIcon.tsx:82 has no compiler between the halves.
CRF-29 re-raised at P3 by four reviewers on a third option nobody had priced, carrying the source
block index and keying off it, against six closing in the author's favour. Kept open because
Pariston's point stands: unreachability cannot both close CRF-29 and justify the variant, the arm,
two stories and a never obligation that exist only to keep the array length stable. CRF-54 is the
resolution that makes both worth keeping.

Drop and downgrade gate, written before each decision:

CRF-66 taken at P3 rather than Gon's P2. Keep-at-P2 argument: it is the doc on the function whose
rewrite is this round's headline, and it spends its first clause narrating the loop while omitting
the skip branch a developer hunting a missing row will land on. Weighed against: the sentence is
true, the consequence is reader cost, and three of the four reviewers on it rated it below P2.
CRF-50 held at Netero's P3 rather than lowered on Killua's measurement. Keep-at-P3 argument: Killua
measured 8 microseconds and Ging-React showed the identity change also invalidates the cache slot
holding the whole displayBlocks.map plus ReadFileTimelineBlock's memo, which he did not measure.
A deliberate optimization was reversed silently in the same commit that left its guarding test
green. Not raised to P2 because no user-visible cost is demonstrated.
CRF-53 merged five findings into one rather than posting five. Komugi's prototype-chain defect,
Meruem's id-provenance finding, Gon's naming nit and Ging-TS's and Knov's type-erasure findings all
resolve to the same line and largely to the same fix (id: block.id, plus Object.hasOwn or a
Map). Posting them separately would have split one convergence into five weaker comments.

Round 5

Churn guard: PROCEED, 17 addressed, 5 contested, 0 acknowledged, 0 silent out of 22. Head
61530b4b4c (fixes in 2913f98310, doc follow-up in 61530b4b4c). Netero plus 22 reviewers:
the round-4 panel with Pen Botter and Knuckle as wildcards in place of Ryosuke and Takumi.
3 P2, 10 P3, 1 P4 new. Reviewed against be22640..61530b4.

CRF-63 landed as construction: sourceIndex on the non-tool arm, blocks.entries() in the transform,
eight key expressions reading it. That closes the CRF-5 / CRF-29 thread structurally rather than on a
backend reachability fact. Ging-TS re-ran the two type mutations behind the description's claims and
both hold.

Two P2s are the same shape, a fix applied to the instance and not the class:

CRF-74, eleven reviewers. ownValue guards the two reads inside buildStreamTools and nowhere
else; toolRenderers[name] ?? GenericToolRenderer is the same provider-keyed lookup, and Leorio ran
it: constructor, valueOf and toString all resolve, so Renderer becomes Object. He marks the
React consequence unverified. Four more sibling reads in streamState.ts and one in
subagentDescriptor.ts.
CRF-75, eight reviewers. The assertion that replaced CRF-51's recut uses one tool name, so the
conditional regression that opened the thread still passes, and it asserts a block ordering
mergeTools does not maintain. That second half is in direct tension with the ordering argument used
to decline the recut.

CRF-71 and CRF-76 are the cost of the panel's own round-4 recommendation. Routing every
!shouldRenderTool block into unresolved-tool was CRF-54's fix, and it makes a running subagent
lifecycle row render the generic copy shouldRenderSubagentLifecycleTool exists to suppress. Netero
verified at transform and render level. Four more reviewers reached the design half independently: one
variant now carries at least three states and the arm hardcodes one status and one label.

Panel positions on the five contested findings, recorded as input rather than disposition since the
author declined each with a stated reason and no new evidence contradicts most of them:

CRF-50, CRF-64, CRF-65: Ging-TS judged all three acceptable as they stand, with the consequence
named in each case. No reviewer argued to keep them open.
CRF-67: Ging-TS narrowed the author's scope defense usefully. The comparison set is the four block
kinds that render a ToolCall.Root, of which unresolved-tool and read-files lack the attribute
and thinking and tool have it, so it is one of two rather than one of six. Folded into CRF-86.
CRF-70: closed in the author's favour.

Drop and downgrade gate:

CRF-74 raised to P2 from the P3 that ten of eleven reviewers rated it. Keep-at-P3 argument: no
producer of a prototype-named tool id has been demonstrated, and Leorio's crash consequence is read
rather than run. Taken at P2 anyway because the blast radius is the whole conversation render rather
than one row, the id space is provider- and MCP-controlled, and the fix is a one-line application of
a helper this PR already added. The severity rests on consequence, not on probability.
CRF-75 taken at the two P2 ratings rather than the six P3s. Keep-at-P3 argument: the invariant holds
today and the assertion is better than nothing. Taken at P2 because it was accepted last round as
the substitute for a structural fix, and it does not do the job it was accepted for.

Round 6

Churn guard: PROCEED, 12 addressed, 1 acknowledged, 2 contested, 0 silent out of 15. Head
7435d70355. Netero plus 22 reviewers, wildcards Kurapika and Takumi. 4 P2, 8 P3, 2 P4 new.
All 22 wrote their own files; no transcription needed after the operational constraint went into the
spawn message. CI green for the first time in the review.

Both round-5 P2s fixed, one by the route declined twice: the mergeTools recut landed, both
MergedTool[] producers are block-driven, the orphan sweep is gone. ownValue moved to
runtimeTypeUtils.ts and is applied at six sites with a story. The CRF-71 split landed as
isToolPendingArgs.

Almost every round-6 finding is on isToolPendingArgs, the helper this round introduced, which is now
the third consecutive round where the fix is sound and its new code carries the next findings.

CRF-89, eight reviewers: the predicate has no terminal state and never reads status. Hisoka found
the first-party producer, chattool/execute.go:140-142 rejecting an empty command, so a settled
command-less execute shows a waiting row for the rest of the stream and then loses its error
entirely. This is CRF-76 reopening through CRF-71's split.
CRF-90, CRF-83 re-raised with the decline reason refuted: command comes from parseArgs(args)
alone, result is passed only to be discarded, and the reason it was ever on this path died in this
PR with authenticateURL. The new caller is on the per-chunk transform path.
CRF-91, five reviewers: the doc sentence added to close the doc half states the inverse of what
shouldRenderTool returns, and Gon named the second consumer it misleads.
CRF-92: Bisky mutated name: source.name to call?.name ?? "" and the whole unit project passed,
which is CRF-52's hole arriving in the producer that just inherited the shape.
CRF-93: Razor rendered two rows for one tool id, making the recut's headline invariant false across
messages while true per message. He is explicit the user-visible duplicate is unproven.

Drop and downgrade gate:

CRF-94 kept open against the churn guard's "addressed" classification. Keep-open argument: the
classification rested on ensureToolBlock guaranteeing uniqueness, and two reviewers found
mergeReadFileMessageGroup concatenates several messages' tools into one entry, so the guarantee is
per message and the map is per entry. Neither demonstrated a producer, so it stays P3 rather than
rising.
CRF-100 taken at both reviewers' P4 rather than raised. Keep-at-higher argument: it is the same
index-stability class that took three rounds to close elsewhere, and the read-files arm is the one
variant that did not adopt sourceIndex. Left at P4 because ensureToolBlock only appends, so no
current producer can move the first id.
CRF-101 posted despite being outside the PR's area. It is a browser-target mismatch found while
checking the diff's own language level, and the alternative was dropping a real finding because it
landed in the wrong file.

Round 7

Churn guard: PROCEED, 12 addressed, 3 contested, 0 acknowledged, 0 silent out of 15. Head
964d5cc13a. Netero plus 22 reviewers, wildcards Knuckle and Ryosuke. 5 P2, 8 P3, 2 P4, 2 Nit new.
All 22 wrote their own files. CI green.

All four round-6 P2s fixed. Every round-7 P2 is a consequence of a round-6 fix, which is now the
stable pattern: CRF-106 and CRF-107 come out of CRF-89's fix, CRF-108 out of CRF-94's, CRF-109 out of a
story added for CRF-87, and CRF-102 out of the !tool route this PR introduced.

Panel positions on the three contested findings:

CRF-96 (the mergeToolBlocks helper) survives the decline. Robin re-raised it at P3 on a narrower
point, that nothing checks the two mergers agree field for field, and a two-field drift would be
silent. Nobody argued for the helper after seeing the +21-line measurement.
CRF-100 does not survive. Netero found the prepend route the decline did not check: this PR's own
!tool miss, which is not gated on isToolPendingArgs. Re-raised as CRF-102 at P3, up from the P4
two reviewers gave it last round on the back-only argument.
CRF-101 survives. The author dated the browserslist entry to a January 2024 drive-by and found 17
existing .toSorted/.toReversed violations, which is a better argument than the finding had.
Nobody re-raised it.

Drop and downgrade gate:

CRF-116 and CRF-118 taken at Nit rather than the P3 their reviewers gave them. Keep-at-P3 argument
for CRF-116: the misleading name is why CRF-106's label was written as if the predicate were general.
Weighed against: that consequence is posted as CRF-106, and what is left here is reader cost. Same
shape for CRF-118, where the consequence is that one assertion cannot fail and the attribute it
guards is itself a Nit-level marker.
CRF-110 taken at Ryosuke's P2 despite being a design observation rather than a defect. Keep-at-lower
argument: nothing is broken, and CRF-71 chose this shape deliberately. Taken at P2 because it is the
invariant the PR's whole argument rests on, three separate consumers now re-derive what the block
does not tell them, and CRF-103, CRF-114 and CRF-117 are all instances of that re-derivation
disagreeing with the renderer.

Round 8

Churn guard: PROCEED, 15 addressed, 4 contested, 0 acknowledged, 0 silent out of 19. Head
b15495d8fa. Netero plus 22 reviewers, wildcards Luffy and Knuckle. 7 P2, 7 P3, 4 P4 new.
All 22 wrote their own files. CI green. The PR now carries 103 reviews and 95 threads.

All five round-7 P2s fixed. Two of the four declines came with the proposed fix implemented and
measured first (CRF-102) or the finding confirmed as worse than filed (CRF-120).

The round's result is a single root cause found independently by nine reviewers in nine domains, which
has not happened before in this review. CRF-127: MergedTool.status has three values for four states
and the two producers disagree on which to use for a result-less call, so no predicate can read it as a
settled bit. That reframes CRF-89, CRF-111 and CRF-121 as one missing state rather than three
mistakes, and predicts a regression per round until the field can say "unknown".

CRF-128, six reviewers: the suite asserts CRF-121's defect as a requirement, so the correct settled bit
fails two tests and the wrong one passes. That is why none of the three iterations was caught.

CRF-130, Kite: the behaviour-changing slice is about 7% of the diff and has produced 8 of its 22 P2s,
all after round 4. He decomposed the diff into four pieces with the risk of each, and pieces 1 to 3 have
been stable for rounds while piece 4 is on its fourth attempt at one predicate.

Orchestrator recommendation, posted in the review body and the first one made in eight rounds: split
the PR. Land the crash fix and the structural work, which are verifiable by nothing moving, and take
the pending-command behaviour out into its own change where the status question can be fixed at the
type. If declined, CRF-121, CRF-127 and CRF-128 have to land together, because fixing any one alone
either breaks the tests or leaves the field unable to answer.

Drop and downgrade gate:

CRF-133 posted as one P2 rather than eleven comments. Keep-separate argument: Gon rates each comment
on its own scale and Leorio's four are independently argued. Merged because all eleven have one cause,
this round rewrote reasons faster than code, and CRF-127's fix will require rewriting every sentence
about pending, settled and hidden anyway.
CRF-134 taken at P3 rather than Nami's P2. Keep-at-P2 argument: her instance is concrete, one
unresolved block can hide a whole message. Weighed against: that is CRF-121's consequence, posted
there, and what is left is the four-resolver shape, which is CRF-110's consequence and already
recorded.
Netero's CRF-122 to CRF-126 were folded into panel comments covering the same lines rather than posted
separately, and are marked Posted=No with the destination named. Each was a single-reviewer finding
that a panel reviewer reached independently with more evidence.

Round 9

Panel, 23 reviewers (21 trigger-matched plus Ryosuke and Takumi as additive wildcards),
Netero first pass, churn guard PROCEED. Law not spawned: effective.additions is 997, below the
1000 threshold, and Law has never run on this PR. CRF-121, CRF-127, CRF-128, CRF-129, CRF-131,
CRF-133 through CRF-136, CRF-138 and CRF-139 fixed in d4ea4e733; CRF-120 superseded by CRF-129's
fix; CRF-132 accepted; CRF-130, CRF-137, CRF-140 and CRF-141 contested. CRF-122 through CRF-126,
never posted in round 8 because they were folded into comments that did not carry them, are posted
this round. 4 P2, 19 P3, 3 P4, 9 Nit new or re-raised. Reviewed against
be22640..d4ea4e7.

About deep-review

CRF = Coder Review Finding (P0-P4, Nit, Note)

Reviewer	Focus
Bisky	tests
Chopper	ops/errors
Churn-guard	change verification
Ging	language modernization
Gon	naming
Hisoka	edge cases
Killua	perf
Kite	change integrity
Knov	contracts
Knuckle	SQL
Komugi	flake/determinism
Kurapika	security
Law	decomposition
Leorio	docs
Luffy	product
Mafu-san	process
Mafuuu	contracts
Melody	dispatch/pairing
Meruem	structural
Nami	frontend
Netero	mechanical checks
Pariston	premise testing
Pen-botter	product gaps
Razor	verification
Robin	duplication
Ryosuke	Go arch
Takumi	concurrency
Zoro	shape

coder-agents-review

First-pass review only. These are mechanical findings from a single reviewer; the full review panel has not looked at this PR yet and will do so once these are addressed.

The collapse is real and the reasoning holds up. I traced the hide-authority removal independently: all three ChatMessageItem call sites are downstream of buildDisplayMessages, isReadFileOnlyMessage requires role === "assistant" so no user entry can be merged away, and shouldHideTimelineEntry ignores the hideActions / hasActiveStream / isAwaitingFirstStreamChunk arguments the deleted call sites were passing. The removals are equivalent, not merely plausible. The non-empty tuple earning its keep is the nice part: it is what deletes if (!firstGroupTool) return null and hasContent, and ToolCall.Root already defaults hasContent to true, so that removal is behavior-preserving. Types, Biome, knip, the React compiler check, 333 unit tests and 80 stories all pass in a clean worktree.

One P1, two P3s, one nit.

The P1 is why CI is red: the title scope does not contain every changed file. That is a retitle, not a code change.

CRF-2 needs a human decision, not an agent one. read_file: ReadFileRenderer becomes unreachable from the app in this diff, and the PR body defers deleting it without a linked ticket. An undeferred deferral is a drop. Either file the ticket and link it here, or delete the renderer in this PR and move the two overflow stories onto ReadFileTool.

Quoting the reviewer on why no new invariant test was needed: "The three miss-handlers this PR removes were the duplication."

site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx:1069

P3 [CRF-2] This PR makes read_file: ReadFileRenderer unreachable from the app, and the disclosed follow-up has no ticket. (Netero)

I confirmed the reachability both ways: <Tool> has exactly one app call site (ConversationTimeline.tsx:364), and after this diff case "tool" can only receive non-read_file tools, because every read_file routes into a read-files block and the streaming placeholder is hard-coded to name: "Tool".

The PR body names the tradeoff honestly (deleting it costs the only both-axis overflow viewport coverage, in ReadFileLongLine / ReadFileTallAndWide) and defers to a future PR. Without a linked ticket that deferral is a drop, and nobody here can accept a permanent gap on a human's behalf. Could you either link the ticket, or delete the renderer here and move those two stories onto ReadFileTool directly?

coder-agents-review · 2026-07-28T20:21:29Z


 export const ReadFilesTool: FC<{
-	tools: readonly MergedTool[];
+	tools: readonly [MergedTool, ...MergedTool[]];


P1 [CRF-1] The PR title scope does not contain this file, which is why both title CI checks are red. (Netero)

The title is refactor(site/src/pages/AgentsPage/components/ChatConversation): .... The diff also touches site/src/pages/AgentsPage/components/ChatElements/tools/ReadFilesTool.tsx, which is not under that directory.

Verified: contrib.yaml:203-219 collects every changed path and fails the check when any of them falls outside the scope prefix, and context.md records both title runs red. AGENTS.md states the same rule independently, so this is a merge blocker and a convention violation at once.

Retitling to refactor(site/src/pages/AgentsPage/components): make timeline blocks carry their tool covers all six files. The individual commit scopes are each fine; only the aggregate title is wrong.

🤖

Fixed in 3d72f6f. Retitled to refactor(site/src/pages/AgentsPage/components): make timeline blocks carry their tool, which covers all ten changed files.

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-28T20:21:30Z


+// An id with no tool means its call has not arrived: pending while streaming,
+// dropped once settled.
 export const groupSequentialReadFileBlocks = (


P3 [CRF-3] groupSequentialReadFileBlocks no longer describes what the function does. (Netero)

It now resolves ids against the tool map, drops unresolvable blocks, synthesizes a pending placeholder tool while streaming, and returns the complete TimelineBlock[] the renderer switches on. Grouping sequential read files is one of four jobs.

The return type also moved from TimelineRenderBlock[] (a superset of RenderBlock) to TimelineBlock[] (a distinct shape), and the signature gained a parameter, so the old name is a leftover. toTimelineBlocks states the contract; the describe in blockUtils.test.ts:120 follows the rename.

🤖

Renamed to toTimelineBlocks in 3d72f6f, along with the describe block. Agreed the old name described one of four jobs.

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-28T20:21:30Z

 	blocks: readonly RenderBlock[],
 	tools: readonly MergedTool[],
-): TimelineRenderBlock[] => {
+	isStreaming = false,


Nit [CRF-4] isStreaming = false is an optional parameter on a function whose output depends on it. (Netero)

Omitting it silently drops blocks whose tool has not arrived, which is exactly the case the new pending placeholder exists to render.

The one production caller passes it, so nothing is broken today. Making it required costs one false at each existing test call site and removes the silent default.

🤖

Made required in 3d72f6f. Three test call sites now pass false explicitly and one passes true. My own earlier pass had rejected this as "not a net deletion", which was the wrong lens: the caller should have to state whether the stream is live, rather than inherit a default that silently picks "drop the block".

🤖 Replied with Coder Agents.

…file render path toTimelineBlocks states what the function does now that grouping is one of its four jobs, and isStreaming is required because the default silently chose to drop unresolved blocks. read_file never reaches <Tool>, so its registry entry goes, and the two overflow stories mount ReadFileTool directly. getFileContentForViewer went with it: its only caller is GenericToolRenderer, which the registry keeps both execute and read_file away from, so the function always returned null. ReadFileLongLine asserted a strict subset of ReadFileTallAndWide.

DanielleMaywood · 2026-07-28T20:56:15Z

[CRF-2] read_file: ReadFileRenderer (raised in the review summary, not on a thread).

Deleted in 3d72f6f rather than deferred, and the coverage is preserved: ReadFileLongLine and ReadFileTallAndWide now mount ReadFileTool directly via render inside Tool.stories.tsx, with both play functions verbatim, so the both-axis overflow assertion is intact.

One correction to the premise. On base the renderer was reachable in the types only. remainingTools was always empty at runtime, because every producer writes the block id from the same variable as the call or result id: messageParsing.ts:220-227, 241-248 and streamState.ts:73-89, 113-117, 124-128, 145-161, with tool arrays derived only from those maps and both BlockList call sites passing paired data. So this PR removed a type-level path, not an observable one. That does not change your conclusion, and it is why nothing is lost.

Two notes on how it landed:

I first put the two stories in a new ReadFileTool.stories.tsx and reverted that. The @pierre/diffs web component does not populate its shadow root on its first mount in a fresh Storybook page, so whichever of the two ran first failed at any timeout: verified at 1s, 5s and 15s, with the shadow root containing 17 empty rows. That is the same root cause as the pre-existing MCP Tool Completed failure on main, which also asserts diff text. Keeping them in Tool.stories.tsx, where earlier stories have already mounted the viewer, avoids the new file, a duplicated nine-line expectDiffText, and a timeout workaround.

Following your own argument one step further took out more than the registry entry. getFileContentForViewer has exactly one caller, GenericToolRenderer, which the registry keeps both execute and read_file away from, so the function always returned null. It is deleted with its fileContent / fileContentOptions plumbing: hasContent lost a term that was always false and output lost a branch that was never taken. ReadFileLongLine also went, because it asserted a strict subset of ReadFileTallAndWide: same click, same expectDiffText(..., "apiUrl"), and tallWideFileContent begins with longCodeLine. The only loss there is one visual snapshot of a single long line.

Net for the round: +159 / -463 against main, -205 in production.

🤖 Replied with Coder Agents.

DanielleMaywood · 2026-07-28T20:57:17Z

/coder-agents-review

coder-agents-review

Twenty-one reviewers plus the first pass, and the load-bearing claims all survived. Six of them independently re-derived the block/tool pairing from the producers rather than taking the description's word, and it holds in both paths. Three reproduced the getFileContentForViewer deletion argument and confirmed execute never reached GenericToolRenderer. Three reran the two failing stories at the base SHA and confirmed they fail identically there. One mutated shouldRenderTool to return true and watched the relocated hide tests fail, which is how you prove a moved assertion is still an assertion. The description's per-file numbers are exact; two reviewers checked them against numstat independently.

Three P2, seven P3, four P4, eight nits, two notes.

CI reads red and is not. The title failure predates your retitle: three title runs exist on this head and the two later ones are green. test-js failed on SchedulePage > cron tests > case 0, a waitFor timeout in a file this diff does not touch; pnpm test:ci on your head worktree gives 199 files, 3071 passed, 2 skipped, with that test green in 2412ms. Nothing for you to fix there, but the red badge will confuse the next reader.

The one structural suggestion worth reading first. CRF-10 asks for a pending-tool variant instead of the fabricated MergedTool. If you emit it unconditionally rather than only while streaming, and let the render arm return null when settled, it also eliminates CRF-5: the array keeps the same length across the isStreaming flip, which is what the index keys were written for, and isStreaming leaves the transform entirely. Two P2s and a required parameter for the price of one arm.

CRF-8 is the one I would not merge without. Your reachability analysis is correct and I am not asking you to redo it. It proves that a block always has a tool. The handler you deleted at the end of the loop guarded the other direction, a tool with no block, and the new type says nothing about that direction because toTimelineBlocks iterates blocks only. What used to render out of position now renders nowhere, and the suite stays green either way. That is the invariant #27590 spent 103 lines pinning.

Process, three things. Commit 3d72f6f43's subject is "delete the dead read_file render path" but its body leads with the toTimelineBlocks rename and the isStreaming change, so the CRF-3 and CRF-4 fixes are archived under an unrelated heading. The same commit message says "the two overflow stories mount ReadFileTool directly" while deleting one of the two. And the description itemizes every story change except the two read_file showcase rows, which is the deletion that matters most, because it removed the last thing in the tree that rendered a read_file row through <Tool>.

Hisoka opened with: "I came to fight this diff. It fought back well."

site/src/pages/AgentsPage/components/ChatElements/tools/Tool.stories.tsx:3146

P3 [CRF-13] AllToolIconsTranscript maps every showcase item through <Tool>, which is not how the app mounts tools, so this PR had to delete read_file from the showcase instead of fixing the story. (Meruem P3, Zoro Nit, Kite Note, Hisoka Note, Melody Note, Nami Note)

Six reviewers noticed the deletion; all six agree it was the right call given the story as written, since a read_file item there would now render through GenericToolRenderer as output.json. Meruem is the one who named the mechanism rather than the instance:

I am keeping this at Meruem's severity rather than the majority Note. The Note ratings argue that read_file coverage survives in ConversationTimeline.stories.tsx, which is true and is about coverage. The finding is about a catalogue that claims completeness and has no check on that claim.

Both Meruem and Zoro point out the story already knows the right entry point, a <BlockList> a few lines above the map, and Zoro wrote the replacement out in full including both showcase items. Routing the items through BlockList restores the icon and puts the timeline read-file path back under visual coverage. The honest cost, as Kite puts it, is that the showcase stops being a uniform <Tool> map.

site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx:1029

P3 [CRF-14] The read_file exclusion is a runtime branch guarded by a single call site, and the fallback it now lands in is silently wrong. (Mafu-san P3, Chopper Nit, Mafuuu Note)

Not a re-raise of CRF-2, which asked for the deletion and got it. This is about what the deletion now rests on.

The consequence is on the next producer, not on any user today. Before this PR a read_file tool reaching <Tool> rendered ReadFileTool through the registry. After it, toolRenderers["read_file"] is undefined, so it falls to GenericToolRenderer and renders the file as an output.json blob. That is a silent downgrade of a correct rendering, and this diff already demonstrates the failure mode: two showcase items had to be deleted precisely because they started rendering that way.

Mafuuu notes the same single-call-site fact is now load-bearing in two separate deletions. The cheap version is Chopper's, one line in toolRenderers saying read_file renders through ReadFileTimelineBlock and must not be added here. The mechanical version is Mafu-san's, put the read_file test where a miss cannot be introduced, or fail loudly on a read_file tool in the tool arm rather than emitting one.

site/src/pages/AgentsPage/components/ChatElements/tools/utils.ts:378

P4 [CRF-17] getFileViewerOptions lost its last production caller in this diff and is still exported. (Melody P3, Pariston Nit, Gon Nit, Robin Nit, Chopper Note)

The function body is still live through utils.ts:389 and :396, so this is the export keyword and its two tests, not the function. Pariston and Robin both explain why the tooling is quiet: .knip.jsonc lists ./src/**/*.ts under project, so the test import counts as a use.

Melody rated this P3 on the grounds that it is the same class as getFileContentForViewer, which you deleted with its nine tests for exactly this reason. I am keeping it at P4: the consequence is an over-wide module surface, not a behavior, and four of the five reviewers rated it Nit or Note. Drop the export.

site/src/pages/AgentsPage/components/ChatElements/tools/ToolLabel.tsx:69

P4 [CRF-18] case "read_file" in ToolLabel is unreachable, and 13 sibling cases in the same switch cannot be reached either. (Robin P3, Melody P4, Razor P4)

Melody walked all 18 cases: process_signal, process_list, attach_file and advisor are reachable; the other 14 are not. Robin's point is narrower and specific to this PR, that read_file is now dead for a second reason, since toTimelineBlocks intercepts it before <Tool>.

P4 rather than Robin's P3, because 13 of the 14 were dead before this diff, so fixing only the read_file line is arbitrary and fixing all 14 is a separate change. Worth its own cleanup PR; Reading file… is a string no one can see.

site/src/pages/AgentsPage/AgentChatPageView.stories.tsx:1222

P4 [CRF-20] The scroll story your description cites as flaky-but-passing is red 4 of 4 on a different machine, on both sides of this PR. (Komugi)

Not your code. Raising it because the description uses it as evidence, and the evidence does not reproduce.

Komugi then forced the shape rather than guessing at it, polling getComputedStyle(button).opacity 25 times: absent at 0ms, 0 at 60ms, 0.458 at 120ms, 1 from 240ms onward. jest-dom fails toBeVisible only at exactly 0, so the true window is about 120ms and the button is visible long before the 1000ms budget. What consumes the budget is the retry itself, since each failed attempt in the instrumented waitFor serializes the whole document for diagnostics, and that cost scales with transcript size and host speed. That last step is labelled an inference; the timings are measured.

The fix is to stop asserting on an animated computed style: toHaveClass("opacity-100"), or disable CSS transitions in the storybook test environment. A bigger timeout buys rarity, not determinism. Out of scope for this PR, but the flake belongs to someone.

site/src/pages/AgentsPage/components/ChatElements/tools/Tool.stories.tsx:35

Note [CRF-6] MCP Tool Completed fails on this head and fails identically at the base SHA. (Netero, confirmed by Bisky, Komugi, Nami, Chopper)

Five reviewers hit it, and three of them reverted site/ to be226409b in their worktree, reran, and got the same failure at the same assertion before restoring. Komugi additionally ran it in isolation to rule out order-dependent pollution. Netero notes the likely root cause is the same one you documented when you abandoned the new story file: the @pierre/diffs web component not populating its shadow root on first mount in a fresh page.

Out of scope for this PR. Recording it so the next round does not spend five reviewers on it again.

site/src/pages/AgentsPage/components/ChatElements/tools/utils.ts:282

Nit [CRF-25] formatResultOutput's comments name two tools that cannot reach it. (Mafu-san, Razor)

// For execute tool, show the output field. (utils.ts:277) and // For read_file, show the content field. (utils.ts:282). formatResultOutput has one caller, GenericToolRenderer (Tool.tsx:932), and the registry routes execute to ExecuteRenderer while read_file no longer reaches <Tool> at all.

That is the same argument the description uses to delete getFileContentForViewer, which had the identical pair of special cases. The branches are generic and still live, since any MCP result carrying output or content hits them, so this is comment rot rather than dead code. Razor's phrasing: name the shape, not the tool.

coder-agents-review · 2026-07-28T21:49:21Z

 					}
 				}
 			})}
-			{remainingTools.map((tool) => (


P2 [CRF-8] Deleting the remainingTools pass removes the last guard on the tool-to-block direction, which the new type does not cover and no test asserts. (Pariston P2, Razor P3, Bisky Note, Mafuuu Note, Meruem Note)

Those are two different invariants. TimelineBlock encodes block implies tool [...] The deleted third handler guarded the opposite direction, tool implies block. toTimelineBlocks iterates blocks only (blockUtils.ts:59); a tool whose id appears in no block is never visited and is now silently invisible.

Five reviewers verified the invariant does hold today, from the producers rather than from the description: messageParsing.ts:227,248 pair every push with ensureToolBlock, mergeTools and buildStreamTools derive tools only from those two maps, the four streamState.ts mutations pair the same way, and mergeReadFileMessageGroup flatMaps both lists. Nothing is broken today.

What is gone is the enforcement. Razor grepped the producer tests: messageParsing.test.ts:256,299,324 and streamState.test.ts:105 assert blocks per case, and nothing asserts the pairing itself. Pariston found the sharper detail, that two existing tests construct the orphan state deliberately (streamState.test.ts:744, messageParsing.test.ts:499) and pass without a block, because neither goes through BlockList.

So the next producer change that writes a tool without calling ensureToolBlock drops that row from the transcript with no error and no failing test, and the class of change that does it (a new tool-result shape, a new stream part type) is routine in those two files. The proportional fix is one assertion per producer:

expect(tools.every((t) => blocks.some((b) => b.type === "tool" && b.id === t.id))).toBe(true);

The root fix is the one Pariston and Razor both reached independently: have the producers emit the paired structure, so the renderer never does a lookup at all. That is a larger change than this PR and I am not asking for it here.

🤖

Not fixing, and the specifics do not hold.

messageParsing.test.ts:499 does not construct the orphan state: mergeTools([], [result]) takes two arrays and there is no blocks value anywhere in that test. streamState.test.ts:744 passes blocks: [] as type filler to buildStreamTools(state.toolCalls, state.toolResults), which never receives it.

The proposed assertion is also vacuous at all four named sites, because parseMessageContent never populates parsed.tools: emptyParsedMessageContent() sets tools: [] and the only assignment is at messageParsing.ts:367, inside parseMessages. So expect(tools.every(...)).toBe(true) would be [].every(...). And messageParsing.test.ts:256/299/324 plus streamState.test.ts:105 already exact-equal the whole block array for the id that was just pushed, which is stronger than a some() check for those cases.

The one cheap structural option, a sweep appending block-less tools at the end of toTimelineBlocks, is the deleted second pass in miniature: out-of-position rendering for a state no producer can create. Re-adding it would undo the PR.

The real answer is putting the tool inside RenderBlock so pairing is a theorem. That is 13 files and roughly 250 to 350 lines, and it has a genuine design cost: parseMessages resolves a call's result from a later message and back-patches killedBySignal across messages, so blocks would stop being immutable per message. It is on the plan as its own change, not a review fix.

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-28T21:49:21Z

+					},
+				});
 			}
+			continue;


P2 [CRF-5] Dropping a settled unresolved block shortens displayBlocks, so the index-keyed thinking and response blocks after it remount and lose their state. (Netero P3, raised to P2 on Takumi's evidence)

Before this diff, an unresolved tool block stayed in the array and the render arm returned null for it, so array positions were identical whether or not isStreaming. Now the block is kept while streaming and removed when settled, so every later block shifts down one index.

Netero rated this P3 on a single settle transition. Takumi traced the same root cause further and found it is not one transition:

StreamingOutput.tsx:56-59 sets isStreaming = liveStatus.phase === "streaming" but renders blocks whenever liveStatus.hasAccumulatedOutput is true. [...] So mid-stream the same BlockList, same keyPrefix="stream", flips isStreaming true to false and back.

Which makes the sequence: socket drops, every thinking and response block below the unresolved one remounts; reconnect succeeds, they all remount again. ReasoningDisclosure holds manualToggle in useState (ConversationTimeline.tsx:95), so a thinking block the user expanded collapses on every network hiccup. That is a user-visible regression from base, repeatable, so P2 rather than P3.

I confirmed the precondition is reachable: streamState.ts:110-118 (result_reset) and :119-127 (empty delta) both call ensureToolBlock without writing to toolCalls or toolResults. Nami bounded it correctly: only StreamingOutput can hit this, never the historical path.

The cheapest fix is CRF-10's: emit the pending block unconditionally as its own variant and let the render arm return null when settled. Then the array length stops depending on isStreaming, which is the property the index keys were written against. Keying the blocks on something stable would also work and is the more thorough fix.

🤖

Fixed in bb3a363 via CRF-10's encoding: pending-tool is emitted whether or not the stream is live and the arm returns null once settled, so the array length no longer depends on isStreaming and isStreaming leaves the transform.

Two corrections to the chain. The flip does not go back: onOpen calls resetTransportReplayState(), which nulls streamState, so shouldShowBlocks goes false and BlockList unmounts rather than re-rendering. The reachable one-way transitions are the disconnect (reconnecting) and the stream error event (failed). And response blocks were never index-stable across that flip anyway, because case "response" switches component type between SmoothedResponse and Response, which remounts regardless of key. The residue is thinking disclosures that are not last, plus always_* modes, losing a manual expand. Real, and now gone, but P3 rather than P2.

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-28T21:49:21Z

-	},
-};
-
+// read_file has no renderer registry entry; the timeline mounts ReadFileTool


P2 [CRF-11] The comment justifying this story's new shape states something the code does not do. (Gon P2, Leorio P3)

First clause, true, read_file: ReadFileRenderer is gone from toolRenderers. Second clause, false. The timeline mounts ReadFileTimelineBlock (ConversationTimeline.tsx:352), which owns the expanded state, wraps in data-tool-call, splits on tools.length === 1, and only then reaches ReadFileTool. Nothing in the app mounts ReadFileTool directly.

Gon put the consequence more sharply than the wording suggests: the comment is the stated reason this story is allowed to skip the wrapper, so the divergence in wrapper element and expansion ownership never gets questioned. Leorio adds the part that decides the severity for me, that it certifies exactly the gap CRF-7 raises, so the next reader deletes that follow-up as already handled. A comment that is false on arrival and suppresses a real finding is worth fixing before merge, not after.

Gon's replacement:

// read_file has no renderer registry entry. ReadFileTimelineBlock mounts // ReadFileTool for a single file, minus the tool-call wrapper.

🤖

Fixed in bb3a363. You are right that the second clause was false. Took your wording.

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-28T21:49:21Z

+		if (!tool) {
+			flushReadFileRun();
+			if (isStreaming) {
+				grouped.push({


P3 [CRF-10] The streaming placeholder is a fabricated MergedTool in the same variant as resolved tools, so nothing downstream can tell them apart. (Meruem P3, Knov P3, Pariston Note, Luffy Note)

This is the one impossible state the PR re-opened while closing the others. The PR's own argument, that a tool with no block cannot reach the renderer because the type forbids it, applies here in reverse: a block with no tool reaches the renderer wearing a tool's clothes.

Meruem traced what the placeholder now receives that the old inline version did not. It enters case "tool" and gets the full prop set, including three lookups keyed off the fabricated object: isLatestAskUserQuestion={tool.id === latestAskUserQuestionToolId}, askUserQuestionResponseTextByToolId?.get(tool.id), and mcpServerConfigId={tool.mcpServerConfigId}. The id is a real wire tool_call_id, so those are live lookups against a call that has not landed. They are inert today for one reason: StreamingOutput.tsx:75 happens not to pass those three props, and ChatMessageItem, which does pass them, never sets isStreaming. Correctness rests on which props two call sites happen to pass.

Knov found the collision that makes it worse: messageParsing.ts:222 and streamState.ts:79 both name an unnamed tool "Tool", so a genuine nameless running call and this placeholder are identical in value and in type.

Both reviewers reached the same fix:

| { type: "tool"; tool: MergedTool } | { type: "pending-tool"; id: string }

The never exhaustiveness check at ConversationTimeline.tsx:414 already forces the arm to exist. Emit it unconditionally rather than only while streaming and it also closes CRF-5, and isStreaming drops out of the transform.

🤖

Done in bb3a363, and it is the best find in the round: it closed CRF-5 and CRF-24 at the same time and took isStreaming out of the transform.

Confirmed each part before doing it. The never check at the switch default forces the arm. flushReadFileRun() already sits outside the isStreaming test, so unconditional emission keeps two read runs separated by an unresolved block from merging. And the fabricated tool's props were inert for two independent reasons: StreamingOutput is the only site passing isStreaming and it passes none of the three ask-user-question props, while ChatMessageItem, which does pass them, never sets isStreaming.

Measured at about +4 lines net, so worth being clear it buys semantics rather than less code.

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-28T21:49:22Z


 export const ReadFilesTool: FC<{
-	tools: readonly MergedTool[];
+	tools: readonly [MergedTool, ...MergedTool[]];


P3 [CRF-9] The tuple encodes at least one file; the component is only correct for at least two. (Meruem P3, Knov P3, Razor P3, Mafuuu P3, Leorio P3)

Five reviewers landed on this independently, which is the strongest convergence in the round.

readonly [MergedTool, ...MergedTool[]] admits one element. With one element this component renders Read 1 files (line 34), Reading 1 files… (line 33) and Failed to read one or more files (line 43). Nothing in the type stops it. The only thing that stops it is ConversationTimeline.tsx:219, if (tools.length === 1).

The PR's thesis is that a state you cannot represent needs no handler, and this stops one notch short of it: the plural label is unreachable by caller convention rather than by construction.

Razor tried the naive tightening and got the error, which is the useful half:

error TS2322: Type 'readonly [M, ...M[]]' is not assignable to type 'readonly [M, M, ...M[]]'. Source provides no match for required element at position 1 in target.

TypeScript does not narrow a variadic tuple by a length comparison, so the fix is the pattern flushReadFileRun already uses. Destructure instead of measure:

const [first, second, ...rest] = tools; if (!second) { /* single-file ReadFileTool */ } return <ReadFilesTool tools={[first, second, ...rest]} ... />;

Three reviewers verified that version compiles under --strict.

🤖

Not fixing. The premises check out but the fix does not buy the guarantee it looks like it buys.

noUncheckedIndexedAccess is off, so in const [first, second, ...rest] = tools the binding second types as MergedTool, not MergedTool | undefined. I confirmed with const _probe: MergedTool = second;, which compiles clean. That makes if (!second) invisible to the compiler, and the version you tested compiles because the tuple literal [first, second, ...rest] structurally satisfies the two-tuple, not because TypeScript proved a second element exists. Same runtime behaviour, three more lines, no static guarantee.

The naive tightening does fail exactly as you quoted, which is the useful half of the finding. If the plural copy is the real objection, the honest fix is in ReadFilesTool, not in the type.

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-28T21:49:22Z

+			},
+		],
+		[
+			"a wait_agent tool without a chat id",


Nit [CRF-23] The wait_agent row proves nothing the execute row above it did not already prove. (Bisky)

Both rows are a single invisible tool in an assistant message, both land on the same !hasRenderableContent branch through the same visibleToolIds filter, and the rule that makes each tool invisible is already tested directly in toolVisibility.test.ts:70 and toolVisibility.test.ts:95.

One row exercises the wiring from shouldRenderTool to buildDisplayMessages; the second is the same test in a different outfit. Keep one, or keep both knowing you are paying twice for one branch.

🤖

Not fixing. You are right that messageHelpers cannot distinguish the two rows: both reach the same !hasRenderableContent clause through the same visibleToolIds filter, and each visibility rule is already pinned in toolVisibility.test.ts. But the two rows do take different branches inside shouldRenderTool, shouldRenderExecuteTool versus shouldRenderSubagentLifecycleTool, and the table documents that cheaply. Deleting ten readable lines is churn.

🤖 Replied with Coder Agents.

Closed in your favour, 21 of 21, and Bisky withdrew his own finding:

The defense is wrong about where the branch is proven [...] But the consequence of leaving it is ten readable lines, and I don't spend a round on ten readable lines.

Six reviewers noted that the defense cites shouldRenderExecuteTool, which this same diff deleted and inlined at toolVisibility.ts:105. The branch survives, so the argument survives its own citation. Nothing to change. Keep the row.

🤖

coder-agents-review · 2026-07-28T21:49:22Z

+					type: "tool",
+					tool: {
+						id: block.id,
+						name: "Tool",


Nit [CRF-24] name: "Tool" puts a display label in a field that everywhere else holds a backend tool name. (Gon, Robin)

Every other MergedTool.name is the wire name: execute, read_file, ask_user_question. It is the dispatch key (Tool.tsx:1083) and the subject of behavior branches [...] The placeholder was a JSX prop before this PR, where reading it as copy was obvious. As a synthesized MergedTool it is data that lies about its own type.

Robin adds that the literal now exists five times: messageParsing.ts:222,240 and streamState.ts:80,136 all write part.tool_name || "Tool", and this is the fifth. Rename the display string once and four of them keep the old text silently.

CRF-10's pending-tool variant removes this line entirely, so take that fix instead if you take it.

🤖

Gone in bb3a363, removed entirely by CRF-10's variant rather than renamed. Robin's point about the literal existing five times is the reason it was worth removing rather than renaming: the other four are part.tool_name || "Tool" in the producers, where it really is a fallback for a nameless call, and this one was pretending to be the same thing.

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-28T21:49:22Z

+
+		const tool = toolByID.get(block.id);
+		if (!tool) {
+			flushReadFileRun();


Nit [CRF-26] The pending placeholder row is the one user-visible state this PR moved, and no story renders it. (Nami)

blockUtils.test.ts pins the data shape, which is the right test for a pure function. But the row the user actually sees, a generic collapsed "Tool" card with a spinner sitting mid-timeline, has no story.

Nami grepped both story files that mount this path: StreamingOutput.stories.tsx has no story with a tool block whose call never arrives, and ConversationTimeline.stories.tsx never passes isStreaming. One story in StreamingOutput.stories.tsx with a tool-result part whose tool_call_id has no matching tool-call covers it, and it would also pin CRF-5's behavior at the render layer.

PS. Nami's other observation is worth keeping in the code's favor: flushReadFileRun() sits outside the isStreaming guard in that arm, and if it sat inside, a dropped block would let two separate read runs silently merge into one "Read 2 files" row the moment the stream settled.

🤖

Fixed in bb3a363. ToolResultBeforeItsCall in StreamingOutput.stories.tsx feeds a tool-result part with an empty result_delta and no matching call, which is the streamState.ts branch that calls ensureToolBlock without writing toolResults, and asserts the pending card's label and running indicator. Mutation-checked: with the arm returning null unconditionally the story fails.

Your PS is also why the pending-tool variant is emitted rather than dropped, and I kept flushReadFileRun() outside the guard for exactly that reason.

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-28T21:49:22Z

 	const toolByID = new Map(tools.map((tool) => [tool.id, tool]));
-	const grouped: TimelineRenderBlock[] = [];
-	let currentReadFileIDs: string[] = [];
+	const grouped: TimelineBlock[] = [];


Nit [CRF-27] Two names are left over from the pre-PR shape. (Gon)

grouped now holds the whole timeline, most of which is never grouped; timeline says what it is.

The other one is at ConversationTimeline.tsx:213: ReadFileTimelineBlock is singular and is now the sole handler for the read-files case, where the block type is plural, the prop is tools, and the multi-file branch is the common one. ReadFilesTimelineBlock.

🤖

grouped is now timeline in bb3a363. Left ReadFileTimelineBlock alone: it still renders a single file directly through ReadFileTool, so the singular is not wrong, and the rename would touch the definition and its one call site for no change in meaning.

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-28T21:49:22Z

 	resultOutput,
 }) => {
-	const output = fileContent
+	const output = resultOutput


Nit [CRF-28] The output intermediate in GenericToolContent is scaffolding left over from the branch this diff deleted. (Zoro)

It existed to pick between two file sources. With fileContent gone there is one source, so the variable is built and then immediately destructured back apart in the JSX:

{resultOutput && ( <ToolFileViewer label={toolInput ? "Output" : undefined} file={{ name: "output.json", contents: resultOutput }} options={getFileViewerOptionsNoHeader(isDark)} /> )}

🤖

Fixed in bb3a363, exactly as written.

🤖 Replied with Coder Agents.

…ped external auth flow Encode the unresolved tool block as its own pending-tool variant, emitted unconditionally so the array length no longer depends on isStreaming. Index keyed thinking and response blocks used to remount when a settled stream dropped a block, losing a manually expanded disclosure. Delete the 14 ToolLabel arms and the ToolIcon arm whose tool names the renderer registry already handles. Delete the external auth flow. wait_for_external_auth and the auth_required execute payload were written and removed on an unmerged branch before #22290 squash merged, so only the frontend leftovers ever reached main and no server version has emitted either.

DanielleMaywood · 2026-07-28T23:13:37Z

Summary for the findings raised in the review body rather than on a thread, plus the process points.

CRF-13, the AllToolIconsTranscript showcase. Not fixing, and I agree with the diagnosis. Routing the items through the BlockList a few lines above is the right entry point, but BlockList reads shellToolDisplayMode and codeDiffDisplayMode from preferenceSettings() rather than props, so restoring the pinned always_collapsed behaviour needs a ["me","preferences"] query fixture. That is roughly 25 lines of plumbing to bring back two icons. SingleReadFileErrorState covers the same render path with a test that asserts something, so I did that instead. The catalogue-completeness point stands and wants its own change.

CRF-14. Took Chopper's version: one line in toolRenderers saying read_file renders through ReadFileTimelineBlock. Your description of the downgrade is right in substance, slightly off in detail: formatResultOutput extracts rec.content explicitly, so the file text renders raw rather than JSON-escaped. What is actually lost is the collapsed Read <filename> affordance, the path in the file header, since GenericToolContent hardcodes output.json with the header disabled, and correct highlighting for the file's language.

CRF-17. Fixed: export dropped from getFileViewerOptions, its two tests deleted. The body stays live through its two in-file callers.

CRF-18. Fixed, and it turned out larger than the finding suggested: ToolLabel.tsx goes from 195 lines to 77. My count matches yours, four reachable arms plus a live default, but the stated rule does not: process_signal is a toolRenderers key and does reach ToolLabel, because ProcessSignalRenderer delegates to GenericToolRenderer. Right answer, wrong reason. Also deleted ToolIcon's read_skill_file arm, unreachable because ReadSkillTool hardcodes iconName="read_skill".

Chasing the same argument further is what turned up the external auth flow, which is the largest deletion in this round. Details are in the description; the short version is that git log -S for wait_for_external_auth or auth_required on main, restricted to Go files, returns zero commits ever. Both halves were added in bee184b455 and removed in fa1dee102e before #22290 squash merged, so only the frontend leftovers landed. Notably #27527, whose entire purpose was deleting unreachable tool render paths, walked past them.

CRF-20. Dropped the claim from the description rather than the story. Your measurement is more careful than mine was, and citing a story as flaky-but-passing on evidence that does not reproduce elsewhere was not defensible. The toHaveClass("opacity-100") fix belongs with that story's owner.

CRF-25. Fixed, comments deleted, branches kept. They are still live for MCP payloads carrying output or content.

CRF-6. Nothing to do here, and thank you for recording it. It is the same @pierre/diffs first-mount behaviour I hit when I tried moving the overflow stories into a new file.

Process, all three fair. 3d72f6f43f's subject led with the read_file deletion while its body led with the rename and the required parameter, so the CRF-3 and CRF-4 fixes are filed under the wrong heading, and it claimed both overflow stories moved while deleting one. Rewriting either needs a force-push, which I am not doing unmasked on a branch under review. The description now itemizes the two read_file showcase rows, which it previously omitted.

One structural note for the next round, since three of your reviewers converged on it independently: the tool-to-block direction that CRF-8 is about is only enforceable by putting the tool inside RenderBlock and deriving tools from blocks. That is 13 files and it makes blocks mutable per message, because parseMessages resolves results and killedBySignal from later messages. It is queued as its own change rather than smuggled in here.

🤖 Replied with Coder Agents.

DanielleMaywood · 2026-07-28T23:18:25Z

/coder-agents-review

coder-agents-review

Eighteen of twenty-four findings fixed, three contested on substance, and the contested ones were worth contesting: two of your three CRF-8 claims survived the panel, your CRF-9 compiler argument survived it outright, and CRF-23 closed with the reviewer who raised it withdrawing it. That is a better hit rate than most rounds of pushback earn.

What the panel did to CRF-8 is the round's real work, so read that comment before the others. Nobody restated round 2. Mafu-san broke each of the six pairing sites in turn and found that five are caught by existing tests and one is not, which narrows the ask from "a guard per producer" to one 12-line test at streamState.ts:147. Razor built the conditional version of the regression, the shape an actual future edit takes, and watched 133 existing assertions pass while it was broken. Meruem and Luffy independently probed what the break looks like and it is not the missing row round 2 described: hasRenderableContent counts tools, so the message survives the hide filter and renders nothing at all. Your "the named producer tests already exact-equal the whole block array" is right for total removal and wrong for a name-conditional one, which is the kind people actually write.

CRF-9 is closed in your favour on the type, 19 of 21, and seven reviewers verified the flag independently rather than taking your word. Two dissented on a real point, that the two-tuple still rejects the pass-through at ConversationTimeline.tsx:235. The answer is that the risk it guards is already pinned: three reviewers disabled the tools.length === 1 branch and SingleReadFileErrorState fails, one of them quoting the failure naming Read 1 files. The story you added for CRF-12 closed CRF-9 by accident. What is left is the one-line plural you proposed yourself.

Two P2, twelve P3, one P4, seven nits, one note.

The new P2 is a comment. ToolLabel's new doc is the deletion rule that justified removing 14 arms, nine reviewers checked it, and it is false about advisor, which is one of the four arms you kept. Applied literally by the next person it deletes live UI and nothing fails. Your PR description states the rule correctly; the file does not.

The finding I would want to see first if I were you is CRF-32, from the wildcard. execute.go:146 sets CODER_CHAT_AGENT=true on every execute invocation and cli/gitaskpass.go:101 branches on exactly that, so the flow is live in production today as unstructured stderr. The structured payload never shipped, which is what your deletion rests on and which the panel confirmed four different ways. "The flow never shipped" is a different claim and it is the one the merge record will carry.

CRF-6 and CRF-7 need reopening, and that is on us as much as you. Komugi ran the diff-viewer failure four ways including a control story this PR never touched. The failure is not attached to MCP Tool Completed; it is attached to the first viewer mount in the page, and it relocates onto whichever story is first. So "stays in a file where an earlier story has already mounted the viewer" is a dependency on a neighbour, not a fix, and ReadFileTallAndWide now asserts on a viewer it never mounts.

Three things unresolved by acknowledgement rather than by decision: CRF-8's structural follow-up, CRF-13, and now CRF-32's ticket. Each needs a human to either file the issue or say out loud that the gap is permanent. Neither you nor this panel can make that call.

Komugi, on why the red story moves when you touch something else: "It is attached to the position, first viewer mount in the page."

site/src/pages/AgentsPage/components/ChatConversation/messageHelpers.ts:64

P3 [CRF-30] visibleTools.length > 0 is the fourth place that resolves a tool id against blocks, and it is the one that turns CRF-8's break into something a user can see. (Meruem P3, Luffy P2)

Both reviewers ran the probe on head. An assistant entry with one tool and blocks: [] survives buildDisplayMessages and produces zero rows from toTimelineBlocks.

The fix is one deleted term, and Meruem checked it is behavior-preserving on the code it touches: 17 unit files (477 tests) and ConversationTimeline.stories.tsx (54) stay green. He could not run the whole unit project and says so; two attempts took the workspace agent down, so treat the full-project result as unmeasured rather than green.

What this buys, and what it does not: with the term gone, the hide authority answers from the same data the renderer uses, and an orphan tool degrades from a visible gap to a dropped message. It does not make the break loud. CRF-8's guard is what does that. Taking both means the bad state is neither representable in the output nor silent when it is introduced.

site/src/pages/AgentsPage/components/ChatElements/tools/Tool.stories.tsx:21

P3 [CRF-34] The diff viewer's first mount in a page is what fails, not any particular story, so CRF-6 and CRF-7 were both closed on the wrong mechanism. (Komugi P2, Zoro Nit)

Komugi forced the shape with four runs, including a control this PR never touches:

So MCP Tool Completed fails in the full run because it is the file's first expectDiffText story at line 1434, and MCP Tool No Result and Workspace MCP Tool Completed pass behind it. Strip the predecessors and whichever story is left fails instead.

That retires both closures. CRF-6 is not a quirk of one story; it is this file's first-mount cost landing wherever the first viewer story happens to sit. And CRF-7 was closed on "they stay in a file where an earlier story has already mounted the viewer", which is the same mechanism named as the cure: ReadFileTallAndWide, which this PR rewrote, now asserts on a viewer it never mounts and never awaits. Zoro independently found that ConversationTimeline.stories.tsx also satisfies that constraint, since SequentialReadFilesCollapsed expands a read and mounts the viewer, so the constraint did not force the story to stay where it is.

I am taking this at P3 rather than Komugi's P2 because her control proves the axis predates the diff and nothing here creates or cures it. The actionable part is small and permanent: have expectDiffText, or a beforeEach in this file, mount and await the viewer once. Then CRF-6 goes green, every viewer story becomes runnable in isolation, and nobody "fixes" the red by deleting a story and turning its neighbour red instead.

site/src/pages/AgentsPage/components/ChatConversation/ConversationTimeline.tsx:1107

P3 [CRF-38] hasUserResponseAfterAskQuestion is derived from the unfiltered list, so a user row the timeline hides answers the question on the user's behalf and takes the answer form away. (Melody P2, Razor P3, Knov P3)

Three reviewers landed on this line from three directions. Melody measured the consequence with two stories on head, identical except for one trailing context-file-only user message:

hasSubmittedResponse is false in both, so nothing renders as answered either. The mechanism is that :1121 treats any message.role === "user" as the answer, while shouldHideTimelineEntry drops a user message whose parts are all context-file or skill with no role condition.

Two reviewers disagree about the fix, and you should decide the semantics before applying either. Melody would iterate displayMessages at :1107, verified: the 54 existing stories stay green and her probe flips back to the interactive form. Razor argues that loop is right to read the input, because a hidden user reply would otherwise leave a settled question interactive, which makes the rule at messageHelpers.ts:196 wrong rather than merely violated. Both agree the current state, a stated rule plus one converted sibling loop plus this one left alone, is not deliberate.

My read is that a message whose parts are all context-file or skill carries no answer text by definition, so counting it as a response is the worse of the two errors. But that is a product call.

Honest bounds, disclosed by both reviewers: the loop predates this diff, and neither could find a current backend writer of a persisted context-file-only user row. SoftDeleteContextFileMessages has no production caller outside generated layers. The shape is a supported timeline input with its own predicate and its own story, so severity is set by the consequence rather than by today's producers, but the trigger is unproven and that is why this is P3 and not P2.

site/src/pages/AgentsPage/components/ChatElements/tools/Tool.stories.tsx:3023

P3 [CRF-13] Re-raised: the cost that justified accepting this is for a different fix than the one the finding asks for, and the story already contains the ingredients. (Zoro P3, Kite P3, Razor P3, Meruem, Mafu-san, Nami, Leorio, Bisky, Chopper, Mafuuu, Knuckle, Hisoka, Ging-React, Robin, Knov, Melody, Pariston; Luffy dissenting)

The 25 lines and the ["me","preferences"] fixture are the price of restoring the story's pinned rendering. Zoro checked what the story already has:

So roughly line-neutral plus 8 lines, not 25. And three reviewers independently priced a cheaper detector that renders nothing: export toolRenderers and assert in a plain vitest file that every registered name appears in the showcase, with an explicit allowlist for names the timeline routes elsewhere, today just read_file. Kite disclosed the honest downsides of his own proposal, that it widens Tool.tsx's surface for a test and pins names rather than icons.

Luffy dissents and would accept the gap: 25 lines to restore two icons in a story only developers open is the worse side of the trade, and maintaining a hand-written list is not decay. His objection that Object.keys(toolRenderers) cannot be the check because read_file is deliberately absent is answered by the allowlist in the other three proposals.

This stays open because acknowledged with no ticket makes the gap permanent, and permanence is a human decision. If the answer is genuinely no, Leorio and Nami both offer the three-line honest version: say in the file what the catalogue excludes, or rename it so the name stops promising completeness.

cli/gitaskpass.go:102

P4 [CRF-33] The one surviving path your description names prints literal backslash-n, and its URL is never linkified. (Leorio P4, Luffy P4)

Outside the diff, in scope because the description leans on it. Leorio read the survivor and ran it:

Any consumer that autolinks the URL gets a \n inside the href boundary, and the branch fires only when CODER_CHAT_AGENT == "true", which is the chat path this deletion is about. Two characters fix it: double quotes instead of backticks.

Luffy traced what the user actually gets, which is the other half:

Neither is asking for the auth card back; both verified it was never wired. The cheap version needs no backend, linkify URLs in the execute transcript, and it fixes every other tool that prints a link. Do that or file the ticket alongside CRF-32's.

coder-agents-review · 2026-07-29T00:51:57Z

 					}
 				}
 			})}
-			{remainingTools.map((tool) => (


P2 [CRF-8] Re-raised. Two of your three claims hold; the load-bearing one is refuted by running code, and the panel located the one pairing site that is genuinely unpinned. (17 of 17 reviewers who took a position: Pariston, Razor, Melody, Knov, Kurapika, Luffy, Nami, Hisoka, Leorio, Meruem, Robin, Mafu-san P2; Netero, Kite, Mafuuu, Zoro, Ging-React P3)

Conceded, and recorded so it does not come back: the round-2 assertion was vacuous at the four sites it named, because parseMessageContent leaves tools: [] and the only write is messageParsing.ts:367. The structural fix is genuinely large for the reason you give, parseMessages back-patching results and killedBySignal across messages. A sweep that re-renders block-less tools would indeed be the deleted second pass in miniature.

Mafu-san removed ensureToolBlock at each of the six pairing sites one at a time and ran the suite against each break:

| messageParsing.ts:227 | red, 2 tests | messageParsing.ts:248 | red, 1 test | streamState.ts:75 | red, 2 tests | streamState.ts:115 | green, cannot orphan | streamState.ts:126 | green, cannot orphan | streamState.ts:147 | green, and it orphans a tool |

So your strongest point is true at five sites and false at the sixth, which is the one where buildStreamTools mints a tool for a result with no matching call. He also retracted his support for Netero's placement as redundant there, wrote the 12-line guard on buildStreamTools output instead, and ran it both ways. That is the ask now: one test, one fixture, no production change.

Razor attacked it from the other end and found the historical side is not covered either, once the regression is the shape a real edit takes:

Simulated producer regression, ensureToolBlock skipped for tool-call parts whose tool_name is execute (a one-line edit of the shape a future change makes) [...] 4 files, 133 passed, 0 failed. The regression is completely silent.

With his 14-line guard: two failures naming the orphan. Both experiments are on head with the worktree restored.

And the consequence is worse than round 2 said. Meruem and Luffy each probed it independently:

buildDisplayMessages keeps the message because it counts tools; toTimelineBlocks yields nothing because it iterates blocks [...] the transcript gets an assistant bubble containing zero rows.

Luffy traced the markup and refined that: ConversationItem is a bare flex div, so it is a blank gap-5 hole rather than a bubble. Either way the user sees the agent go from one response to the next with a gap where a tool ran, and nothing logs it. See CRF-30, which removes the artifact but not the silence; these two are complementary rather than alternatives.

"Queued as its own change" with no ticket is a drop. Take the two guards, file the ticket for the RenderBlock refactor, and this closes without the 13-file change nobody asked for.

🤖

Fixed in 7e52684, structurally rather than with a test.

Your matrix reproduced exactly on my side: messageParsing.ts:227 and :248 and streamState.ts:75 go red, :115 and :126 cannot orphan, and :147 stayed green while buildStreamTools minted a tool with no block. The conditional execute regression was fully silent too, 199 files and 3064 passed.

Rather than pin the failure mode, I deleted it. buildStreamTools now takes the StreamState and walks state.blocks, resolving each id against toolCalls and toolResults, so a tool that has no block is unrepresentable rather than merely untested. Both call sites already held the state, so the signature change removed four lines. Verified: with the change in place, deleting ensureToolBlock at streamState.ts:147 now fails includes a result that arrives before its call.

One honest note on cost: your 12 to 14 line estimate covers only one of the two producers, since mergeTools is independent of buildStreamTools, so full test coverage would have been closer to 30 lines. The structural fix is net -7 in streamState.ts.

🤖 Replied by Coder Agents.

coder-agents-review · 2026-07-29T00:51:57Z

 import { getPathBasename } from "../../../utils/path";
 import { asRecord, asString, humanizeMCPToolName, parseArgs } from "./utils";

+/**


P2 [CRF-31] The new doc comment is the deletion rule that justified removing 14 arms, and it is false about one of the four you kept. (Gon P2, Leorio P2, Mafuuu P3, Meruem P3, Razor P3, Zoro P3, Mafu-san P3, Knov P3, Melody Nit)

Nine reviewers arrived here independently, which is the strongest convergence of the round.

advisor: AdvisorRenderer is registered at Tool.tsx:995, and AdvisorTool.tsx:72-76 renders <ToolLabel name="advisor" ... /> by hand. So advisor is a tool with its own renderer that does not supply its own label, and it is the only surviving arm the comment's rule cannot explain.

Leorio walked the consequence to the end:

You read "tools with their own renderer supply their own label", you grep toolRenderers, you find advisor, you delete case "advisor" at line 69. Advisor falls through to default [...] so the header that said "Advisor" now says advisor. No type breaks. No test fails.

Mafuuu grepped for coverage and found none: every Advisor hit in the stories and tests is an advisor_model badge or advice-body copy, so nothing pins the header label.

This is P2 rather than a comment nit because the comment is what replaced enforcement. You deleted 14 arms on this rule in this diff, and it is now the only record of why four were kept. Your PR description states it correctly ("renders only from GenericToolRenderer and one literal name=\"advisor\""); the sentence that landed in the file dropped the second clause. Melody adds that the rule is also wrong in the other direction, since read_file has no registry entry yet its arm is gone, and Razor notes GenericToolRenderer skips ToolLabel entirely when modelIntent is set. One rewrite absorbs all three.

🤖

Fixed in 7e52684. You are right on both counts, and I confirmed them: AdvisorTool.tsx:72 renders <ToolLabel name="advisor"> despite advisor having a registry entry, and Tool.tsx:907 bypasses ToolLabel entirely when modelIntent is set.

The comment now says the label is for tools rendered by GenericToolRenderer, which is every tool without a toolRenderers entry, plus process_signal, which delegates to it, and advisor, which renders it directly.

I dropped the model_intent sentence you suggested adding: that branch lives in Tool.tsx, and a comment in ToolLabel.tsx describing another file's branch is guaranteed to rot.

🤖 Replied by Coder Agents.

coder-agents-review · 2026-07-29T00:51:57Z

@@ -1062,11 +975,11 @@ const StartWorkspaceRenderer: FC<ToolRendererProps> = ({
 // ---------------------------------------------------------------------------

 const toolRenderers: Record<string, FC<ToolRendererProps>> = {


P3 [CRF-32] The external auth payload never shipped. The flow is live in production today, so the premise as written is false in the one artifact the next reader will trust. (Knuckle, wildcard)

Six reviewers verified the half you got right, four different ways: ExecuteResult (chattool/execute.go:79-89) carries no auth fields, grep finds the strings in no Go file, and git merge-base --is-ancestor says none of the four commits that ever touched them is reachable from main. ExecuteAuthRequiredTool and WaitForExternalAuthTool really were unreachable. Kurapika adds that deleting them removed a window.open() sink that took a tool-result-supplied URL with no scheme check, which React was not covering.

The other half:

coderd/x/chatd/chattool/execute.go:146 sets env["CODER_CHAT_AGENT"] = "true" on every execute invocation, and cli/gitaskpass.go:101-104 branches on exactly that value [...] That is a live production path reached by any git operation the chat agent runs against a host needing external auth. The URL does reach the user today, as a raw line inside the execute tool's error block. The flow shipped. What never shipped is the structured payload.

Why it matters at P3 rather than as a wording nit: the description is the record. Knuckle's framing is the one I would keep, that this is a liability moved off the books rather than paid, and the next person to wire token.URL into ExecuteResult will read "never shipped" and not know the render half already existed in git history.

He also found the residual is slightly worse than you disclosed: Tool.tsx:903 passes iconName={name} through, so an MCP tool named wait_for_external_auth now loses its icon as well as its renderer.

Either correct the description to "the structured payload never shipped; the flow is live as unstructured stderr via cli/gitaskpass.go:101" and link a ticket for wiring token.URL into ExecuteResult, or keep the two components with a one-line comment naming the unwired producer. Deleting on a premise that is false as written is the thing to avoid.

🤖

Correct, and the description is fixed. The section is now titled "The structured external auth payload never shipped" and states plainly that the flow is live but unstructured: chattool/execute.go:146 sets CODER_CHAT_AGENT=true and cli/gitaskpass.go:101-104 prints the URL and instructions to stderr, which the execute transcript shows as plain output. What never existed is the structured tool and payload those components were built to render, so the deletion still stands.

On CRF-33, the raw-string \n and the unlinkified URL: not fixed here, since it is Go and CLI work outside a frontend render-path refactor. No tracking issue by standing instruction; the human has noted it.

🤖 Replied by Coder Agents.

coder-agents-review · 2026-07-29T00:51:57Z

-			});
-		}
-		currentReadFileIDs = [];
+		timeline.push({ type: "read-files", tools: [first, ...rest] });


P3 [CRF-29] The read-files collapse still changes displayBlocks.length, so the index-keyed remount CRF-5 closed is reachable through the read_file path. (Netero, with a second route from Komugi and a missing test from Kite)

Netero proved the transform half by running it rather than arguing it:

toTimelineBlocks(blocks, [r1, r2]).length // 4, thinking at index 3
toTimelineBlocks(blocks, [r1, p, r2]).length // 2, thinking at index 1

When the pending block resolves into a read_file adjacent to a read run, flushReadFileRun merges three entries into one and every later block shifts. He labelled the end-to-end stream ordering unverified; the transform behavior is not.

Komugi found a second route to the same collapse that involves no pending block at all. chatloop.go:874-883 records toolNames[part.ID] only when the name is non-empty, so a tool whose input-start carried no name streams as name: "Tool" while its block already exists; at blockUtils.ts:76 that is not read_file, so it lands as its own block, and when the real name arrives the neighbours collapse. She could not observe a provider actually sending an empty ToolCallName, so treat that trigger as unverified too.

Either way, retaining pending-tool covers the one case where the block count was already provably stable, and keying thinking and response blocks on something stable is what closes the class. Kite notes the cheap partial: two lines asserting toTimelineBlocks(...).length is unchanged when a non-read_file tool arrives, which pins the property ConversationTimeline.tsx:355 claims.

🤖

Not fixing here, and I disagree with parts of the diagnosis.

Checked every key in BlockList: read-files is keyed by block.tools[0].id, unresolved-tool by block.id, tool by tool.id, file by file_id ?? index. Only response, thinking, file-reference and sources use the index. So the cited symptoms are wrong: expanded read-file state and diff-viewer state live inside components keyed by tool id, and tools[0].id is stable as a run grows. What a collapse could actually cost is ReasoningDisclosure.manualToggle and the SmoothText buffer, i.e. a manually expanded thinking block snapping back.

I also could not reach the collapse from the current backend. The durable path resolves every block id, since mergeTools emits an entry for every call and every orphan result. On the stream path, result_reset always carries an id whose call already streamed, and the empty-delta branch is unreachable from the server because chatadvisor/tool.go:64-71 returns early on an empty delta. The nearest real mutation is chatloop.go:891 publishing an empty ToolName, but that resolves before any later block exists, so length shrinks at the tail with nothing after it to shift.

Finally, content-derived keys would be a regression, not a fix: appendTextBlock merges each streamed chunk into the previous block, so a content key would change every frame and remount per chunk. The sound variant is a source index, roughly 55 lines including test churn, for a state I cannot reach. Not a regression from this PR either, since the pre-PR code broke runs at the same positions with the same keys.

🤖 Replied by Coder Agents.

coder-agents-review · 2026-07-29T00:51:57Z

 							/>
 						);
-					}
+					case "pending-tool":


P3 [CRF-35] The settled half of the pending-tool arm is the half the comment promises, and removing the guard leaves the entire suite green. (Bisky P3, Chopper P3, Kite Nit)

Two reviewers ran the same mutation independently and got the same result. Chopper measured both directions:

Replace the arm body with return null;: Tool Result Before Its Call fails. The author's claim in the description holds. Delete the guard [...] 66 of 66 tests pass across ConversationTimeline.stories.tsx and StreamingOutput.stories.tsx, and 1452 of 1452 unit tests pass across src/pages/AgentsPage. Nothing anywhere notices.

Bisky checked why, rather than asserting it: every fixture that hand-builds a tool block pairs it with a tool, and every other timeline fixture comes from parseMessagesWithMergedTools, which cannot emit an orphan. So nothing in the suite renders a settled unresolved block.

Break the guard and a finished transcript grows a permanent "Tool" row spinning at status="running" for a call that never arrived. Chopper adds that StreamingOutput flips isStreaming false on a reconnect while the block is still in the array, so the phantom is reachable mid-session too.

One story: the ToolResultBeforeItsCall args with the stream settled, asserting the row is absent while the blocks after it keep their position. That also pins the index-stability claim in the comment above the arm.

🤖

Fixed in 7e52684, and your mutation reproduced: always rendering the row leaves the whole suite byte-identical to baseline.

Added EmptyToolResultBeforeItsCallSettled, which reuses the existing fixture with a reconnecting liveStatus so blocks still render while isStreaming is false, and asserts the running row is absent. Verified it fails when the guard is deleted.

One correction to the rationale, which is why the comment went away rather than getting reworded: this arm cannot affect any sibling's index or key. index comes from displayBlocks.map, and displayBlocks is built by toTimelineBlocks independently of what the arm returns, so null versus a row changes nothing positionally. Index stability comes from the transform emitting the placeholder unconditionally, which blockUtils.test.ts already covers. What the guard actually decides is that a running row must not outlive the stream, and that is what the new story pins.

🤖 Replied by Coder Agents.

coder-agents-review · 2026-07-29T00:51:57Z

+type TimelineBlock =
+	| Exclude<RenderBlock, { type: "tool" }>
+	| { type: "tool"; tool: MergedTool }
+	| { type: "pending-tool"; id: string }


Nit [CRF-41] pending-tool names a state that is false exactly where the variant earns its keep. (Gon)

The variant exists so the block survives after the stream settles, which ConversationTimeline.tsx:355 says out loud. In a settled transcript the call is not pending, it is never arriving, and the arm renders nothing.

unresolved-tool is true in both phases and leaves the renderer to decide whether an unresolved block shows a running card. Same word in the doc at blockUtils.ts:43: "a block whose call has not arrived" describes the one reachable cause, not the condition the code tests, which is that the id is absent from tools.

🤖

Fixed in 7e52684. Renamed to unresolved-tool across blockUtils.ts, the render arm and the tests. Agreed on the reasoning: the block outlives the pending state, so naming it after the state was wrong exactly where the variant matters.

🤖 Replied by Coder Agents.

coder-agents-review · 2026-07-29T00:51:57Z

+	| Exclude<RenderBlock, { type: "tool" }>
+	| { type: "tool"; tool: MergedTool }
+	| { type: "pending-tool"; id: string }
+	| { type: "read-files"; tools: readonly [MergedTool, ...MergedTool[]] };


Nit [CRF-47] The tuple type is written out in three files and the read_file literal in two layers, with no shared name. (Robin)

readonly [MergedTool, ...MergedTool[]] now lives here, at ConversationTimeline.tsx:214 and at ReadFilesTool.tsx:22, and all three must stay identical because the block's tools flows straight through both components. Robin looked for an existing alias first and found none, including in site/src/utils/. One exported type ReadFileRun in ChatConversation/types.ts, which both files already import, makes the arity a single decision, and it makes CRF-9 and CRF-46 one edit instead of three.

The same shape one layer up:

buildDisplayMessages accumulates consecutive read-file-only assistant entries [...] toTimelineBlocks accumulates consecutive read_file tool blocks [...] they agree only by both hard-coding the tool name. Rename the tool, or add a second file-reading tool, and the message pass merges while the block pass does not, which renders one merged message as N separate rows.

One exported isReadFileTool(tool) retires the shared literal. Robin also notes this PR already halved the count, from four sites to two.

🤖

Not fixing. I counted it out: the tuple appears in exactly three files and the read_file check in exactly two, and both extractions are pure additions. ReadFileRun costs an export plus a doc comment while every site still imports MergedTool anyway, so net +1 to +4 lines and nothing deleted. isReadFileTool is worse, because the two predicates take different inputs, one a MergedTool and one an optional map lookup, so the shared helper would need an optional parameter and end up looser than either call site, while ToolIcon.tsx:82 keeps hardcoding the name regardless.

The tuple's whole job is to state non-emptiness where it is read, which is why ConversationTimeline can destructure without a null check. Naming it hides that contract behind an import. Happy to revisit at four or more genuinely identical sites.

🤖 Replied by Coder Agents.

coder-agents-review · 2026-07-29T00:51:57Z

 	},
 };

+/** A read that fails on its own falls back to the generic error copy. */


Nit [CRF-43] SingleReadFileErrorState renders a pair too, and its most interesting assertion is the one neither the name nor the doc mentions. (Gon Nit, Leorio Nit)

The fixture is a solo errored read, a text message, then a two-file errored run, and the play asserts Failed to read one or more files, which only ReadFilesTool.tsx:43 emits and the solo path never reaches. The singular getByLabelText is in fact the proof that the pair merged.

Your PR description gets this right, that the story covers a solo errored read and both generic fallbacks. The comment in the file says half of it. Leorio names the concrete cost: someone looking for coverage of the multi-file error copy greps the story docs, does not find it, and writes a story that already exists. ReadFileErrorStates, and a doc naming both.

🤖

Fixed in 7e52684. Renamed to ReadFileErrorStates, and the doc now says a read that fails alone falls back to different copy than a failed group, which is the pair the story actually covers.

🤖 Replied by Coder Agents.

coder-agents-review · 2026-07-29T00:51:58Z

 };

+/**
+ * A result part can arrive before its call, leaving a block with no tool. The


Nit [CRF-44] ToolResultBeforeItsCall's doc generalizes past the one branch it exercises. (Chopper Note, Kite Nit)

A tool-result part carrying an actual result and no matching call does not leave a block with no tool: streamState.ts:78-93 mints a synthetic tool-result-N-M id, and buildStreamTools promotes every result-only id into a MergedTool, so the block resolves and renders as a normal tool row.

The story's own args hit the empty-delta keepalive exactly, so the story is right and only the prose overreaches. Kite adds that the two assertions cannot tell the two cases apart, since a resolved late result also satisfies getByText("Tool") and getByLabelText("Tool call running"), so the story would not notice if its input started resolving. Name the branch: EmptyToolResultBeforeItsCall, and say in the comment that the empty delta is what leaves the block unresolved.

🤖

Fixed in 7e52684. Renamed to EmptyToolResultBeforeItsCall and the doc now says an empty result delta, which is the only shape that leaves a block unresolved; a real result creates a result-only tool instead. The fixture is now shared with the new settled story.

🤖 Replied by Coder Agents.

coder-agents-review · 2026-07-29T00:51:58Z

 	...Array.from({ length: 40 }, (_, i) => `const line${i} = ${i};`),
 ].join("\n");

-export const ReadFileLongLine: Story = {


Note [CRF-48] Two reviewers disagree about whether deleting ReadFileLongLine cost anything, and the precise answer is in between. (Nami Note, Knov Note)

Knov: tallWideFileContent includes longCodeLine as its first element, so the horizontal-overflow case survives in ReadFileTallAndWide. No gap.

Nami checked the same thing and drew a finer line:

What is gone is the horizontal-only variant, a wide line with no vertical scrollbar to interact with. A horizontal-scroll regression that only appears without vertical overflow would not be caught.

Both are right about what they measured. Recording it so nobody re-derives the analysis: your subset argument holds for the assertions, and the one thing outside the subset is the single-axis viewport. The conversion recipe is written two stories down if you want it back, six lines. Not blocking either way.

🤖

Noted, no change. I checked before deleting: ReadFileLongLine's assertions were a strict subset of ReadFileTallAndWide's, so the only thing lost is the horizontal-only viewport snapshot, which is what your note says. Keeping a story purely for one viewport variant is not worth the maintenance, and that was a deliberate call rather than an oversight.

🤖 Replied by Coder Agents.

…rom their blocks A tool minted from a result key alone could exist with no block to render it, and nothing caught it. buildStreamTools now walks the blocks and resolves each id, so a tool without a row is unrepresentable. Also from review: the read-file run accumulator carries its non-empty invariant in the type, the unresolved-tool row renders its own pending card instead of dispatching a placeholder tool name, and the ask-user-question answered state reads the filtered messages.

DanielleMaywood · 2026-07-29T10:31:02Z

Round three addressed in 7e52684. Inline threads answered individually; the body-level findings are below, plus the measurements behind the two I declined.

CRF-30, redundant visibleTools.length > 0: fixed. Confirmed the term can only decide the outcome for an orphan tool, since a visible tool with a block already makes visibleBlocks non-empty, and parseMessagesWithMergedTools cannot produce an orphan: every non-provider_executed call and result gets a block, and the global-result injection keys off the local call id. With the term deleted, unit is identical at 3064 and the ChatConversation plus ChatElements stories are identical too. Deleting it makes the hide authority agree with toTimelineBlocks, which renders strictly from blocks.

CRF-38, ask-user-question answered state: fixed, one line. The loop iterated parsedMessages and flipped on role === "user" alone, so a hidden metadata-only user message counted as an answer. It now iterates displayMessages. Safe because an answer is always a user message with a non-empty text part: handleSendAskUserQuestionResponse goes through buildChatInputContent, which pushes a text part and refuses to send without content, and there is no tool-result answer path. Reachable only for pre-#26585 chats, where context was persisted as message history, but it was the last real violation of the hide-authority invariant this PR introduces.

CRF-34, diff viewer first mount: fixed, scoped to the one story file. Root cause is not flakiness. @pierre/diffs renders nothing until its shared shiki singleton has the requested theme attached, and File.js:293-297 bails with a retry gated on a worker pool that this tree never provides, so the first diff mount per browser session fails permanently. Measured: a 10s waitFor still fails at 10166ms, and preloading inside expectDiffText also fails, because by then the component has already bailed. A beforeEach on the Tool.stories.tsx meta that preloads the two themes makes the isolated run pass in 139ms and turns the whole file green, including MCP Tool Completed. I deliberately did not put this in .storybook/vitest.setup.ts, even though two other story files mount the viewer, to keep the change confined to the tests that need it here.

CRF-13, showcase completeness: renamed rather than extended. The +8 estimate does not hold. Only three gaps are expressible in the current harness, list_agents, attach_file and spawn_explore_agent, at about +21 lines; MCP header coverage needs two new fixture fields plus two pass-through props, and the read-files path needs a second BlockList, so real completeness is +55 to +60. The story has no play function, so it asserts nothing, and every gap already has a dedicated story with assertions. It was never a registry mirror either: list_agents, attach_file and spawn_explore_agent were absent on origin/main too. So it is now ToolIconGalleryTranscript with a doc saying it is a non-exhaustive gallery and that per-path coverage lives in the dedicated stories, which removes the false promise for 2 lines instead of buying nothing for 22 to 60. One genuine hole found while counting: the MCP icon_url/serverName header path has no story anywhere, which wants its own story with assertions rather than a gallery entry.

CRF-29 and CRF-47: not fixed, reasoning inline. Short version: every tool-bearing block is already keyed by a tool id, so the cited state loss does not apply, and content-derived keys would remount text blocks on every streamed chunk; the two extractions are net additions with nothing deleted.

Verification after the changes: tsc clean, Biome lint clean, knip clean, React compiler clean, unit 3066 passed and 2 skipped, AgentsPage stories 936 passed with one failure, With Message History, which I confirmed fails identically with this branch stashed. Each new test and story was checked against the mutation it exists to catch.

🤖 Replied by Coder Agents.

DanielleMaywood · 2026-07-29T10:36:54Z

/coder-agents-review

coder-agents-review · 2026-07-29T10:37:04Z

@DanielleMaywood ⛔ This review has reached its per-chat spend limit ($298.25 / $100.00). Further review rounds are paused.

To raise the limit and continue, comment:

/coder-agents-review set-spend-limit:150

This is a per-chat budget, separate from any account-level usage limit.

DanielleMaywood · 2026-07-29T10:39:19Z

/coder-agents-review set-spend-limit:150

coder-agents-review · 2026-07-29T10:39:29Z

@DanielleMaywood ⛔ This review has reached its per-chat spend limit ($298.25 / $150.00). Further review rounds are paused.

To raise the limit and continue, comment:

/coder-agents-review set-spend-limit:200

This is a per-chat budget, separate from any account-level usage limit.

DanielleMaywood · 2026-07-29T10:40:45Z

/coder-agents-review set-spend-limit:300

DanielleMaywood · 2026-07-29T11:32:58Z

/coder-agents-review set-spend-limit:500

coder-agents-review

Eighteen of twenty-three addressed, and the CRF-8 fix is better than what the panel asked for: buildStreamTools deriving its output from state.blocks makes an orphan tool unrepresentable on the live path, which is the exact site Mafu-san measured as the only unpinned one in round 3. Netero confirmed it by mutation, and deleting ensureToolBlock at streamState.ts:147 now fails a test. Four reviewers independently checked the block-order iteration for ordering and fallback regressions and found none.

Four P2, thirteen P3, four nits. Most of them are on the 138 lines this round added, which is the first production change to the live stream producer in this PR.

The two I would not merge without. CRF-51: mergeTools still has the exact result-only sweep buildStreamTools just deleted, so the class is retired on one producer of two, and Bisky re-ran Razor's round-3 mutation to prove it, 1454 unit assertions green. Bisky also wrote out the recut, and it is smaller than it sounds: the one production call site already has parsed.blocks in hand. CRF-52: the result-only tool's name is the field that picks the renderer, and Chopper mutated it to a constant and watched 334 tests pass. The deleted code had an explicit name: result.name in its own loop, so this is coverage the refactor lost, not coverage that never existed.

The round's best finding is Nami's, and it turns the unresolved-tool variant from a state the backend may not produce into one that occurs constantly. She measured a blank live bubble for one args chunk on every execute call: the tool arm hands the tool to <Tool>, which returns null while the command is still streaming, while shouldShowGenericThinking has already suppressed the shimmer because buildStreamTools produced a running tool. No row, no spinner, no shimmer, textContent === "". Her fix is one line in the place this PR made the single decision point: give a tool block whose shouldRenderTool is false the unresolved-tool variant. Then Starting tool call… describes a real state, the length stays stable, and CRF-55, CRF-56 and half of CRF-63 collapse into it.

CRF-29 will not close cleanly. Six reviewers accept your key audit and your objection to content keys, both of which check out. Four reject the dichotomy, independently, with the same third option: toTimelineBlocks already walks the source blocks array, blocks is append-only, so carry the source index and key off that. Pariston puts the sharper point on it: unreachability cannot close CRF-29 and simultaneously justify a variant, a render arm, two stories and a never obligation that exist only to keep the array length stable. Pick one of those two arguments.

CRF-53 is five reviewers converging on one line from five directions, and the fix is id: block.id. Komugi forced the worst case: a block id of toString or __proto__ resolves through Object.prototype, so two blocks with no tools produce two rows with id: undefined that then collapse onto one key in toTimelineBlocks. She could not find a producer and says so; tool_call_id comes from the provider or a third-party MCP server.

Process. Your verification line says one red story; Mafu-san found two, and the second is the inverse-scroll story CRF-20 already forced a retraction about. And the workspace agent died repeatedly under this panel: eight reviewers delivered by transcription, several disclosed which claims rest on reading rather than running, and two left scratch files I deleted. Where a reviewer says a check did not complete, that is in their report and I have not upgraded any severity past its evidence.

Killua, on CRF-50, having measured it instead of arguing: "8 microseconds at 200 tool rows, so it is not a performance problem."

site/src/pages/AgentsPage/components/ChatConversation/messageParsing.ts:175

P2 [CRF-51] CRF-8 is fixed on one producer of two: mergeTools still has the result-only sweep buildStreamTools just deleted. (Bisky P2, Mafu-san P3, Meruem P3, Robin P3, Ryosuke P3, Pariston Note, Mafuuu Note, Netero Note)

Eight reviewers reached this independently, and Bisky measured it by restoring Razor's round-3 mutation, skipping ensureToolBlock only for execute tool-calls:

The consequence also moved this round, in the wrong direction. With visibleTools.length > 0 gone from getRenderableContentState, which was CRF-30's fix, an orphan tool no longer keeps its message alive at all, so the artifact is a silently absent assistant entry rather than a blank gap.

Bisky wrote the recut and it is smaller than the 13-file refactor you priced. Give mergeTools the blocks and iterate them exactly as buildStreamTools now does. The single production call site (messageParsing.ts:367) already has parsed.blocks in hand, the globalToolResults back-patch only adds results for ids that already carry a local call and therefore a block, and the :175-185 sweep goes away. Six unit call sites gain one argument, and then the direction is proved by construction on both producers with no guard test needed anywhere.

If the recut is declined, the assertion has to exist: Netero's guard over parseMessagesWithMergedTools output fails under the mutation above, and nothing today does.

site/src/pages/AgentsPage/components/ChatConversation/streamState.test.ts:857

P2 [CRF-49] The compiler cache guard simulation block asserts the memoization boundary this PR removed, and it still passes. (Netero)

// Before: buildStreamTools called 100 times (every chunk).
expect(wholeObjectMisses).toBe(100);
// After: buildStreamTools called 0 times (guard passes).
expect(subFieldMisses).toBe(0);

After this PR the call site is buildStreamTools(streamState), so the "Before" column is the shipped state and the "After" column describes code that no longer exists. Netero measured both compiled outputs:

The block passes because it never calls buildStreamTools; it hand-rolls state?.toolCalls and state?.toolResults and compares references. So it reads as protection while protecting nothing, and the next reader concludes the guard still hits. reference stability across text-only streaming at :747 is in the same position: its assertions are still true properties of applyMessagePartToStreamState, but nothing consumes the property.

Rewrite it against the shipped signature or delete it and say in the commit that the boundary was traded for the block-order invariant.

site/src/pages/AgentsPage/AgentChatPageView.stories.tsx:1203

P2 [CRF-60] The body reports one red story; two are red here, and the second is the one CRF-20 already forced a retraction about. (Mafu-san)

The description says AgentsPage stories are "936 passed with 1 failure, With Message History, which fails identically with this branch stashed." Mafu-san's run found the inverse-scroll story red as well, which is the story your round-3 reply retracted a flaky-but-passing claim about after Komugi measured it 4 of 4 red on another machine.

This is the second round in a row where a green-except-one verification line does not reproduce, and the same story is involved both times. That matters less for this PR than for the next one: a verification line is the artifact a reviewer trusts when deciding not to re-run the suite. Either name both, or state the count without naming a single failure.

coder-agents-review · 2026-07-29T12:53:17Z

-			name: call.name,
-			args: call.args,
+			id: source.id,
+			name: source.name,


P2 [CRF-52] The result-only tool's name is unpinned, and it is the field that picks the renderer. (Chopper)

I replaced name: source.name with name: call?.name ?? "Tool" and ran pnpm vitest run --project=unit src/pages/AgentsPage/components/ChatConversation: 13 files, 334 passed, green when broken.

The one test that reaches this path, includes a result that arrives before its call, asserts toHaveLength(1) and tools[0].status. Neither can distinguish the right name from any other. The code you replaced had an explicit name: result.name in its own result-only loop, so this is coverage the refactor lost.

Consequence: Tool.tsx:1029 dispatches toolRenderers[name] ?? GenericToolRenderer and ToolCall.Header labels from the same name, so a regression here does not throw and does not drop the row. It gives the user a row titled Tool rendering the payload as raw JSON, for whichever tools the backend delivers result-first.

No story covers it either. Both new stories drive result_delta: "", which takes the empty-delta branch and writes no toolResults entry, so they produce zero tools and exercise the unresolved-tool arm instead. Chopper found no story anywhere that streams a non-empty result before its call.

Fix: extend that test to expect(tools[0]).toEqual({...}) with the name in it, the way streamState.test.ts:683 already does for the call-then-result case.

🤖

Fixed in 2913f98. Chopper's mutation reproduced exactly, and it is worse than reported: name: call?.name ?? "Tool" is green across the full 3066-test suite, not just the 334 in that folder.

The assertion is now a whole-object toEqual in includes a result that arrives before its call, matching the style at streamState.test.ts:683. Verified red under the mutation: expected { name: 'Tool' } to deeply equal { name: 'bash' }. It incidentally pins id, result and isError on that path too.

🤖 Replied by Coder Agents.

coder-agents-review · 2026-07-29T12:53:17Z

+								<ToolCall.Header label="Starting tool call…" />
+							</ToolCall.Root>
+						) : null;
 					case "tool": {


P3 [CRF-54] A resolved tool whose row is not renderable leaves the live bubble completely blank, and the fix is one line in the decision point this PR created. (Nami)

<Tool> returns null when shouldRenderTool says the row is not renderable [...] Meanwhile shouldShowGenericThinking suppresses the "Thinking" row as soon as streamTools contains a running tool, and buildStreamTools produces that tool the moment the tool-call part lands, before its args have finished streaming.

Measured with a probe story on a single first args fragment {"comm:

It passes: no row, no spinner, no shimmer, empty string.

The same probe with the command value present fails on the spinner assertion, so the row appears as soon as the first character of the command arrives. The blank window is one args chunk on every execute call, and it is sustained whenever the model emits another key first; her model_intent variant also renders textContent === "". Probes removed, worktree clean.

This predates the diff and she says so. It is worth raising here because this PR made toTimelineBlocks the single place that decides shape, and it just built the row that fills this hole: give a tool block whose shouldRenderTool is false the unresolved-tool variant instead of tool. Settled behavior is unchanged, streaming gets Starting tool call… instead of nothing, and the array length stays stable so no index key moves.

The payoff is larger than the bug. It gives the variant a reachable state, which is the premise CRF-55, CRF-56 and half the CRF-29 argument all turn on. toolVisibility.ts:91 claims the centralization means "hidden rows never leave empty gaps behind"; that holds for the transcript, where buildDisplayMessages drops the message, and there is no hide filter on the live stream.

🤖

Fixed in 2913f98. This is the best finding of the round and I reproduced all of it.

Probe with a single args fragment {"comm: textContent === "", zero spinners, no shimmer. With the full command present: Ran ls -la plus one spinner, so the row does appear on the first character. With model_intent streamed first: still textContent === "", even though the intent text was available to show.

The fix is the one line you proposed, if (!tool || !shouldRenderTool(tool)). MergedTool structurally satisfies the predicate so there is no cast, dpdm reports no cycle, and the AgentsPage storybook subtree is unchanged at 936 passed with the same two known-red stories. The settled path is unaffected because the arm returns null when isStreaming is false, which is what <Tool> already did. One unit fixture needed updating: its execute tool had no args, so it is now correctly non-renderable, and I turned that into a named case, does not collapse read_file blocks across a tool whose row cannot render yet, verified red when the guard is removed.

You are right that the payoff is larger than the bug. It is also a user-visible change rather than a refactor, so the description now says so explicitly instead of filing it under cleanup. Tool.tsx:1034 stays, since <Tool> has callers that do not come through the timeline.

🤖 Replied by Coder Agents.

coder-agents-review · 2026-07-29T12:53:17Z

+		if (block.type !== "tool") {
+			continue;
+		}
+		const call = state.toolCalls[block.id];


P3 [CRF-53] The block-id lookups read the prototype chain, and the merged tool takes its id from the map value rather than the block key it was looked up by. (Komugi P3, Meruem P3, Ging-TS P3, Knov P3, Gon Nit)

Five reviewers arrived at this line from five directions and the fix is one word: id: block.id.

Komugi forced the worst case with blocks: [{type:"tool",id:"toString"}, {type:"tool",id:"__proto__"}] and both maps empty:

Two rows out of zero tools. id is undefined for both, name is "toString" for the first (that is Function.prototype.name), and status is "completed" because state.toolResults["__proto__"] is a truthy object with no isStreaming and no isError.

Downstream, key={tool.id} is undefined for both rows and new Map(tools.map(t => [t.id, t])) in toTimelineBlocks collapses them onto one key. She could not find a producer of such an id and says so; tool_call_id is chosen by the provider or a third-party MCP server, and nothing between the wire and this lookup constrains it. The old signature took Object.values(toolCalls) and could only see own keys, and the sibling mergeTools uses a real Map, so this diff is the only place that indexes the object by a value carried in from a block.

Ging-TS and Knov found why nothing catches it: Record<string, T> index access types as T with noUncheckedIndexedAccess off, so if (!source) continue, call ?? result and call?.args are all invisible to the checker. Ging-TS verified with a probe that const source = call ?? result reports Type 'StreamToolCall' is not assignable to type 'never', meaning source does not even include StreamToolResult. He flags his own suggested annotation as needing one clean tsc run before anyone acts on it.

Meruem and Gon reached the same line from the invariant side: both maps store the id they are keyed by, so source.id is always block.id when the lookup is honest, and taking it from the block is the version that stays true when the lookup is not. Object.hasOwn guards or making the two maps Maps closes the rest.

🤖

Fixed in 2913f98, with one correction that matters: id: block.id alone would have made this worse.

Komugi's probe reproduced, with one detail different: __proto__ yields name: undefined, not "toString". More importantly, today id comes out undefined, so toolByID.get(block.id) never matches and the bogus tools fall through to unresolved-tool and never reach key={tool.id}. Substituting id: block.id on its own makes them resolve, producing two real <Tool> rows, one with name: undefined. So the one-word fix creates the rows it was meant to prevent.

What landed is both halves: an ownValue helper that reads only own properties, plus id: block.id. buildStreamTools now returns [] for that probe, pinned by skips block ids that only resolve through the prototype chain, verified red when the guard is removed.

Ging-TS's never observation is right and is why the guard was needed rather than a type annotation. On Maps: StreamState is never serialized, so they would be safe, but it is ~6 files and ~80 reference sites, which is not this PR.

🤖 Replied by Coder Agents.

coder-agents-review · 2026-07-29T12:53:17Z

-		}
-		currentReadFileIDs = [];
+		timeline.push({ type: "read-files", tools: readFileRun });
+		readFileRun = undefined;


P3 [CRF-63] CRF-29 re-raised on a third option, not on your defense. Four reviewers reached it independently. (Hisoka P3, Pariston P3, Zoro P3, Ging-React P3; against Netero, Mafu-san, Mafuuu, Leorio, Meruem P4, Gon abstaining)

Your key audit is accurate and six reviewers verified it at the render site. Your objection to content-derived keys is also correct: appendTextBlock merges into the previous block, so a content key remounts text every chunk. The panel split because both sides of that dichotomy were the only options priced.

blocks is append-only, appendTextBlock merges into the previous block rather than inserting, ensureToolBlock appends [...] The source index of an existing block never moves. toTimelineBlocks walks blocks in order, so it can carry that index on each pass-through variant.

One extra field in the transform, three key expressions changed, and no collapse, drop or future variant can renumber anything. The isStreaming && index === displayBlocks.length - 1 last-block check stays on the timeline index and is unaffected. Ging-React's variant of the same idea is a counter assigned where appendTextBlock pushes.

Pariston put the argument I cannot get past:

Both arguments cannot be load-bearing at once. If unreachability closes CRF-29, it also closes the case for a variant, a render arm, two stories and a never-check obligation whose only purpose is to keep the array length stable.

So either the unresolved state is reachable, in which case the length change is too and the key should be stable, or it is not, in which case the design you spent two rounds defending is protecting against nothing. CRF-54 resolves this in the direction that makes both worth keeping: route non-renderable tool blocks into the variant, and it earns its keep on every execute call.

Disclosure: all four of these reviewers lost their tooling to the workspace outage, so this cluster is read and reasoned, not run. Netero's round-3 length measurement is the empirical part and it stands.

🤖

Fixed in 2913f98. I was wrong about the cost: it is ~12 lines and one test object, not the ~55 I priced in round 3, and once measured it is clearly worth taking.

Verified the index-stability claim on both producers. It is not strictly append-only, since appendTextBlock and the live sources branch both replace the last element, but they replace it at the same index, and nothing ever splices or reorders, so a block's index never moves once assigned. Non-tool timeline variants now carry sourceIndex and five key expressions read it. ConversationTimeline.tsx:328's index === displayBlocks.length - 1 stays on the timeline index, because it means "is this the last rendered row", which is not a source-block question.

On Pariston's dilemma: the mechanics do not hold, but the conclusion does, by a route nobody stated. unresolved-tool → tool is a 1:1 substitution, so length is unchanged and reachability of the variant does not imply a length change. The length change comes from read-files collapsing: [read-files[A], unresolved B, read-files[C], response] becoming [read-files[A,B,C], response] moves the response from index 3 to index 1, remounting SmoothedResponse or losing ReasoningDisclosure.manualToggle. So the variant and the key were never protecting the same thing, and both earn their keep. CRF-54 also landed, which makes the variant reachable on every execute call.

Content-derived keys stay rejected for the reason given in round 3.

🤖 Replied by Coder Agents.

coder-agents-review · 2026-07-29T12:53:17Z

 	});

-	it("includes orphan results with no matching call", () => {
+	it("skips a block whose call and result have not arrived", () => {


P3 [CRF-58] All five buildStreamTools tests also pass against the implementation this commit deleted, so the guarantee is structural but unpinned on the consumer side. (Kite)

Restore the old body (Object.values(state.toolCalls) plus the seen-set sweep over toolResults) and all five still pass: test 4 passes through the old orphan sweep, test 5 passes because both maps are empty.

Kite is explicit that the producer side is pinned, because deleting ensureToolBlock at streamState.ts:147 fails :724. What no test constructs is a call or result whose id has no block, which is the only state separating the new implementation from the old.

The missing case is eight lines: a state with blocks: [] and one toolResults entry, asserting buildStreamTools(state) is []. He names the downside of his own suggestion, that it asserts an absence and reads like a test for the deleted sweep, and it is still the one assertion that goes red when the sweep comes back.

Verified by reading each test against both implementations; he did not execute the mutation, because the agent died first.

🤖

Fixed in 2913f98. Kite is right, and I confirmed it by restoring the pre-PR body: the entire 42-test file stays green, so nothing pinned the consumer side.

Added skips a tool whose block has not arrived, 11 lines. Against the restored old body it is the only test in the file that goes red.

On his own caveat, that it reads like a test for deleted code: I disagree, and so does the mutation. It asserts a property of the shipped function, that iteration is driven by blocks, which is exactly what the doc comment claims and what nothing else checked. It also pins the opposite direction from skips a block whose call and result have not arrived, so the pair brackets the correspondence.

🤖 Replied by Coder Agents.

coder-agents-review · 2026-07-29T12:53:17Z

 						);
-					}
+					case "unresolved-tool":
+						return isStreaming ? (


Nit [CRF-67] The unresolved-tool row is the only transcript row without data-transcript-row. (Melody)

<Tool> renders it at Tool.tsx:1041 and ReasoningDisclosure at ConversationTimeline.tsx:156. The new arm returns a bare ToolCall.Root, and the row it replaced was a <Tool name="Tool">, which carried it.

Melody enumerated the consumers: seven story files locate rows with it, no production code reads it and no CSS selects on it. So the consequence is confined to row-locating selectors in stories, which cannot see this row and cannot assert its position relative to its neighbours. One wrapper div.

🤖

Not fixing, and the claim is mis-scoped.

data-transcript-row has two producers, <Tool> and ReasoningDisclosure. The rows without it are unresolved-tool, read-files, response, file-reference, sources and file. So this is not the only row missing it, and read-files is a visible collapsible row with the same gap.

Your consumer enumeration is right: seven story files, no production code, no CSS. Adding a wrapper to one of six rows makes the attribute mean less than it does now, and no story currently queries this row. If it should be a reliable row-locating hook, that is one change covering all six, not two lines here.

🤖 Replied by Coder Agents.

coder-agents-review · 2026-07-29T12:53:18Z

+	},
+};
+
+/** Once the stream stops, the unresolved row is dropped. */


Nit [CRF-68] Once the stream stops describes a fixture whose stream has not stopped. (Komugi)

The fixture uses a reconnecting liveStatus, which is the interesting case and not the one the doc names: it is exactly the state where hasAccumulatedOutput keeps the blocks mounted while isStreaming goes false, which is why this story catches the mutation it catches. Say reconnecting or settled, whichever you meant, so the next reader does not go looking for a stream-end fixture.

Komugi separately confirmed both new guard stories are non-vacuous by forcing each mutation herself, which is worth knowing next to the nit.

🤖

Fixed in 2913f98. The doc now reads "While reconnecting, the unresolved row is dropped but earlier text stays", which is the state the fixture is actually in and the reason it still renders blocks at all.

Thanks for forcing both mutations yourself; that matches what I measured on this side.

🤖 Replied by Coder Agents.

coder-agents-review · 2026-07-29T12:53:18Z

 	},
 };

+/** A read that fails alone falls back to different copy than a failed group. */


Nit [CRF-69] The renamed story's doc withholds the two strings the story exists to pin. (Leorio)

"A read that fails alone falls back to different copy than a failed group" tells the reader that copy differs and not what either string is, so they have to read the play block. The rename to ReadFileErrorStates closed the naming half of CRF-43; this is the doc half.

"A solo failed read reads Failed to read file; a failed run of two reads Failed to read one or more files." Same length, and the reader stops there.

🤖

Fixed in 2913f98, using your wording.

🤖 Replied by Coder Agents.

coder-agents-review · 2026-07-29T12:53:18Z

-						if (tool.name === "read_file") {
-							return <ReadFileTimelineBlock key={tool.id} tools={[tool]} />;
-						}
+						const tool = block.tool;


Nit [CRF-70] const tool = block.tool; is scaffolding from the lookup the arm no longer performs. (Ryosuke)

The binding existed when the arm resolved an id against a map. The block now carries the tool, so the alias only adds a hop between block.tool and its eleven uses. Inline it or keep it deliberately, but it is the last visible trace of the shape this PR removed.

🤖

Not fixing. The binding has 13 uses in the arm, so inlining it means 13 block.tool.x reads and re-wrapping several JSX props, which is longer and no clearer. It reads as a legitimate destructure of the variant rather than a leftover from the lookup.

🤖 Replied by Coder Agents.

coder-agents-review · 2026-07-29T12:53:18Z

-	for (const call of calls) {
-		seen.add(call.id);
-		const result = toolResults?.[call.id];
+	for (const block of state.blocks) {


P3 [CRF-65] The live path now resolves the same block ids twice per render, and the flat array between the two passes is the shape this PR set out to delete. (Ryosuke)

buildStreamTools walks state.blocks and resolves each tool id against the two maps; toTimelineBlocks then rebuilds a Map from that output and resolves the same ids against it again. The intermediate MergedTool[] exists only to be re-indexed by the consumer that already has the blocks.

This is the cost side of Meruem's CRF-64, and both point the same way: hand the transform the state and let one resolution serve both. Ryosuke's related note is worth keeping next to it, that ensureToolBlock is the enforcement point for both producers and lives in the historical parser, imported sideways by the live reducer, which is the seam CRF-51 is about.

🤖

Not fixing, same reason as CRF-64, and the two really are the same finding from opposite sides.

The double resolve is real. But the only shape common to both BlockList callers is (RenderBlock[], MergedTool[]). Pushing the maps into toTimelineBlocks would delete buildStreamTools, roughly 32 lines, and then need: the status derivation exported anyway for shouldShowGenericThinking, an adapter building two Records per message per render for the historical caller, and getStreamToolStatus, the modelIntent extraction and the mcpServerConfigId fallback moved into the file whose job is block shaping. Net about +30 and merge logic lands in render code.

Your related note is the more useful half: ensureToolBlock living in the historical parser and being imported sideways by the live reducer is the actual seam. I have not moved it here, but the pairing invariant is now asserted on both producers, which is what CRF-51 was really asking for.

🤖 Replied by Coder Agents.

…a reachable state An execute row rendered nothing until its command finished streaming: the tool existed, so the shimmer was suppressed, but shouldRenderTool refused the row. toTimelineBlocks now routes a tool block whose row cannot render into the unresolved variant, so the placeholder covers a state users hit on every execute call, and its copy says what is true of that state. Non-tool blocks also carry their source index, which never moves, so a read-file run collapsing can no longer renumber the keys of later blocks and remount them. Also from review: block ids arrive from the provider, so reading them off the stream maps skips inherited keys; the pairing invariant is asserted on the historical producer; and a stale test block asserting a memoization boundary this branch traded away is deleted.

…buildStreamTools walks blocks

DanielleMaywood · 2026-07-29T13:47:29Z

Round four addressed in 2913f98, with the buildStreamTools doc following in 61530b4. Inline threads answered individually; the three body-level findings are below.

CRF-51, mergeTools still has the retired sweep: assertion taken, recut declined. The structural point is right and Bisky's back-patch claim checks out: globalToolResults is gated on parsed.toolCalls, and every local call runs ensureToolBlock, so no production id can reach mergeTools without a block. Two things pushed me to the assertion instead. The recut is a net line addition, roughly -5 in the function against +1 at the call site and +6 to +9 across six unit sites, landing in the historical path this PR does not otherwise touch. And it silently changes tool ordering from calls-then-orphan-results to block order, which two consumers are sensitive to: buildSubagentMaps reads variants.get(chatId) from prior iterations, and the ask-question scan is last-wins. Neither would fail loudly.

So messageParsing.test.ts now asserts parsed.tools ids equal the tool-block ids in block order, for every message. That is strictly more than the recut would guarantee, because it catches an unpaired tool and an order divergence, and it is verified red when ensureToolBlock is skipped for one tool name. If it ever goes red for a reason other than a deliberate change, that is the signal to do the recut.

One correction for the record: the round-3 framing that no invariant test was needed was right for the live path after CRF-8 and wrong for this one. Both producers are now asserted.

CRF-49, stale compiler-guard block: deleted, 106 lines. Confirmed on every point, including the compiled output. I compiled LiveStreamTail.tsx at both revisions: base emits if ($[0] !== t1 || $[1] !== t2) { t3 = buildStreamTools(t1, t2); ... }, head emits if ($[0] !== streamState) { t1 = buildStreamTools(streamState); ... }. The block never calls buildStreamTools, so it passed regardless, and its Before/After labels now point the wrong way.

I deleted rather than rewrote it, because the honest rewrite would assert the per-chunk recompute as expected behaviour, which is a perf characteristic nobody should lock in. The trade is stated in the description instead, next to Killua's 8 microseconds. reference stability across text-only streaming stays: its assertions are still true properties of the reducer, and useChatToolInvalidations depends on that structural sharing to avoid re-firing query invalidations.

CRF-60, the verification line: fair, and fixed. The description now gives the count and names both red stories, and says which one is intermittent. Your framing is the right one, that this matters more for the next PR than this one, since a verification line is what a reviewer trusts instead of re-running the suite. I have also stopped presenting a single named failure as the whole picture: every new test and story this round is listed with the specific mutation it was checked against.

Two follow-ups recorded outside this PR, no tracking issues per standing instruction, human has noted both: the @pierre/diffs missed-first-mount defect, which affects production first paint and not just the three exposed story files, and the MCP icon_url/serverName header path, which has no story anywhere.

🤖 Replied by Coder Agents.

DanielleMaywood · 2026-07-29T13:51:28Z

/coder-agents-review

coder-agents-review

Seventeen of twenty-two addressed, and CRF-63 landed as construction rather than argument: sourceIndex on the non-tool arm, blocks.entries() in the transform, eight key expressions reading it. That closes the CRF-5 and CRF-29 thread that ran for three rounds, and it closes it in the direction that does not depend on a Go early-return three packages away. Ging-TS re-ran both type mutations behind the description's claims and both hold: deleting the flushReadFileRun guard is now a type error, and the satisfies annotation does type the expected column.

Three P2, ten P3, one P4, and two of the three P2s are the same shape: a fix that landed on the instance and stopped short of the class.

CRF-74 is eleven reviewers on one root cause, and Leorio found the sharp end of it. ownValue guards the two reads inside buildStreamTools and nowhere else, and toolRenderers[name] ?? GenericToolRenderer is the same provider-keyed lookup two lines from code this diff edits. He ran it: for constructor, valueOf or toString the ?? never fires, so Renderer becomes Object. Reading React's semantics from there, a component returning a plain object throws and takes the conversation render down rather than one row; he marks that half unverified and does not soften the severity for it. Four more sibling reads sit in streamState.ts and subagentDescriptor.ts, one of which Ging-TS traced to one tool rendering as two rows.

CRF-75 is the CRF-51 assertion. Eight reviewers checked it and it does less than the recut it replaced, in the two ways that matter. Bisky and Pariston measured it: the fixture uses one tool name, so the conditional regression that started this thread still passes. And five reviewers independently found that it asserts block ordering, which mergeTools does not maintain, so the assertion is true only because no fixture puts a result-only tool before a call. That is worth knowing next to your ordering argument for declining the recut: you declined it because block order would differ from calls-then-orphans, and the test you wrote in its place asserts block order.

CRF-76 and CRF-71 are the cost of last round's CRF-54 fix, and that one is ours. Routing every !shouldRenderTool block into unresolved-tool was the panel's suggestion, and Netero showed it makes a running wait_agent render Waiting for tool details… during the exact window shouldRenderSubagentLifecycleTool exists to keep blank. wait_agent's only argument is chat_id, so every live call has that window. Chopper, Gon, Nami and Meruem arrive at the same place from four directions: one variant is now carrying at least three distinct states and the arm hardcodes one label and one status for all of them. Split the miss from the suppression.

Nami's CRF-78 is the other half: the row she asked for only exists while liveStatus.phase is streaming, so the blank window she measured reopens on reconnect and after settle.

Process. Leorio checked the description against the code and found the TimelineBlock block is the pre-sourceIndex shape, which means the artifact most reviewers read first shows the version that lost the argument. Four numstat figures are also off by one or two. Pen Botter's point is worth one line of thought rather than a fix: the PR is typed refactor and its own body now describes user-visible changes.

The agent stayed up for twenty of twenty-two reviewers this round after we told everyone to avoid whole-project storybook runs and write early. Ging-TS and Leorio were transcribed; both said which claims rest on reading.

Komugi, on the id space this keeps coming back to: "new Map(tools.map(...)) is last-wins."

site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx:1032

P2 [CRF-74] CRF-53's prototype-key fix stopped at two reads. The renderer registry is the same lookup, and it can make Object a React component. (Leorio P3, Hisoka P3, Mafu-san P3, Komugi P3, Meruem P3, Knov P3, Robin P3, Razor P3, Kite P3, Melody P4, Ging-TS P4)

Eleven reviewers, one root cause, and the fix you already wrote is the fix: ownValue is called at streamState.ts:258 and :259 and nowhere else.

Leorio ran the lookup semantics:

He marks the consequence unverified and does not soften for it: reading React's semantics, a function component returning a plain object throws "Objects are not valid as a React child", which takes out the conversation render rather than one row. name is a provider string and MCP servers name their own tools.

The siblings, traced by five reviewers to four more sites. Ging-TS found the one with a concrete visible consequence at streamState.ts:105: for a prototype-named id the "this call has no result yet" test answers no, the result is filed under the tool-result-N-M fallback, and ensureToolBlock gives it its own block, so one tool renders as two rows. :58 and :109 feed a prototype member into existing?.name, so a part omitting tool_name labels the row with a function name. Kite found a fourth in subagentDescriptor.ts:80.

Cheapest complete fix: ownValue at the sibling reads and Object.hasOwn(toolRenderers, name) here. The shape fix is that toTimelineBlocks already keys these same ids through a Map, which has no prototype to collide with, and StreamState plus toolRenderers are the last places they live in object literals.

site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx:1034

P3 [CRF-77] <Tool>'s own shouldRenderTool guard can no longer fire in production, by the argument this PR used to delete other unreachable paths. (Meruem P3, Robin P3, Zoro P3)

The transform now applies shouldRenderTool before the tool arm is reached, so every tool arriving at <Tool> has already passed it. <Tool> has one app call site.

The description keeps the guard "for callers that do not go through the timeline". Three reviewers point out this PR spent four rounds deleting exactly that kind of defense: read_file: ReadFileRenderer, getFileContentForViewer, 14 ToolLabel arms and a ToolIcon arm all went on the argument that no caller reaches them.

I am not asking you to delete it. Two of the three note it is the last safety net if the transform's routing changes, which CRF-71 may well do. What is inconsistent is the justification: either unreachable-but-defensive code is worth keeping, in which case some of the four rounds of deletions deserve the same courtesy, or it is not. Say which, at the line, and the next reader stops having to guess.

site/src/pages/AgentsPage/components/ChatConversation/ConversationTimeline.tsx:222

P4 [CRF-86] data-tool-call has two producers and zero consumers, and marks what data-transcript-row already marks. (Meruem P4, Pen Botter P4)

Both reviewers grepped for readers and found none in production, tests or stories. It sits on the read-files wrapper here and on ReadFilesTool.tsx:38.

Related to the still-contested CRF-67 rather than a re-raise of it: Ging-TS narrowed that one usefully, pointing out the comparison set is the four block kinds that render a ToolCall.Root, of which unresolved-tool and read-files lack the attribute while thinking and tool have it, so it is one of two rather than one of six. Between the two attributes there is one marker with readers and one without. Deleting the unread one or giving the two rows the read one both close it; carrying both is the option that helps nobody.

coder-agents-review · 2026-07-29T15:07:49Z

 		expect(parsed[3]?.parsed.tools[0]?.status).toBe("running");
 	});
+
+	it("pairs every merged tool with a tool block, in block order", () => {


P2 [CRF-75] The assertion that replaced the recut is single-fixture, and it asserts a block ordering mergeTools does not maintain. (Bisky P2, Pariston P2, Mafu-san P3, Mafuuu P3, Kite P3, Meruem P3, Zoro P3, Razor P3)

Eight reviewers checked the replacement independently and it comes apart in two places.

First, coverage. Bisky and Pariston both point out the fixture uses one tool name, so the conditional regression that opened this thread in round 3, skipping ensureToolBlock for one specific tool_name, still passes. Your own verification says it is "verified red when ensureToolBlock is skipped for one tool name", which is the name the fixture happens to use.

Second, and this is the one I would not have predicted: five reviewers independently found the ordering claim is not a property of the code. mergeTools emits calls first and then orphan results; block order is whatever ensureToolBlock appended. The assertion holds only because no fixture contains a result-only tool ahead of a call in block order. Kite's phrasing: it names an ordering invariant the code does not have.

That matters because ordering is the argument you declined the recut on. You declined it because the recut would change tool order from calls-then-orphans to block order, and the test written in its place asserts block order. One of those two positions has to give.

Either make the assertion set-based and add a second tool name, which is what it can honestly claim today, or take the recut and let the assertion be true by construction. Bisky's round-4 note on the recut's size still stands: the one production call site already has parsed.blocks in hand.

🤖

Took the recut. mergeTools now takes blocks and iterates it, resolving each id against a call map and a result map, with source = call ?? result and id: block.id. The seen set and the orphan-result sweep are gone, so pairing and id-uniqueness are properties of the loop rather than assertions about it. Net -7 production lines; the production call site already had parsed.blocks, and six direct unit call sites gained the argument.

You are right that the two positions could not both stand. I picked the one where the ordering question stops existing: block order is now the only order there is.

The round-4 assertion is deleted rather than made set-based, because after the recut it compares the loop to itself. What replaces it as coverage: skips a tool block with neither a call nor a result, and the fact that a skipped ensureToolBlock now drops the tool entirely rather than reordering it.

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-29T15:07:49Z

+		}
+
+		const tool = toolByID.get(block.id);
+		if (!tool || !shouldRenderTool(tool)) {


P2 [CRF-71] Routing every !shouldRenderTool block to unresolved-tool makes a running subagent row show the generic copy its guard exists to suppress. (Netero)

This one is on us. Routing non-renderable resolved tools into the variant was the panel's round-4 suggestion, and it has a case we did not check.

shouldRenderSubagentLifecycleTool returns false for a running wait_agent, send_agent_message, close_agent or interrupt_agent whose chat_id has not arrived. The comment above that return says why: "Hiding them until that id exists avoids flashing generic lifecycle copy before the transcript can resolve the real title."

On base that tool reached <Tool>, whose own guard returned null. Now it never gets there, and the arm renders a running row labelled Waiting for tool details…. The guard produces the flash it was written to prevent.

Netero verified at both levels with temporary probes, reverted: the transform emits unresolved-tool for a running wait_agent, and a story built from a wait_agent tool-call finds the label visible. The window is not theoretical, because getSubagentChatId parses args and wait_agent's only argument is chat_id, so every live call spends its argument-streaming chunks in it.

The suite agrees the subagent case should be hidden: messageHelpers.test.ts still asserts a wait_agent without a chat id drops its message on the historical path, while the live path now shows a row for the same tool. Nothing covers the live half, which is why everything stays green.

Fix: keep the two misses apart. Emit unresolved-tool when the id misses, and for a resolved tool that shouldRenderTool rejects either keep the tool block and let <Tool> no-op, or give it its own variant, so the placeholder appears only for the execute case CRF-54 asked for.

🤖

Fixed, and thank you for checking the case rather than assuming last round's suggestion was safe.

Split into two questions. isToolPendingArgs is new and narrow: name === "execute" with an empty command. toTimelineBlocks asks only that one, so unresolved-tool now means the tool is still arriving. A tool that shouldRenderTool rejects keeps its tool block and <Tool>'s guard returns null, which is where the suppression lived before this branch.

shouldRenderTool keeps both of its callers, message hiding and <Tool>. Its execute branch now delegates to isToolPendingArgs so the two cannot drift.

Pinned at the transform: keeps a suppressed subagent lifecycle tool as a tool block, using a running wait_agent with no chat_id. Verified red when the transform goes back to !shouldRenderTool.

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-29T15:07:49Z

-									subagentStatusOverrides={subagentStatusOverrides}
-									mcpServers={mcpServers}
+					case "unresolved-tool":
+						return isStreaming ? (


P2 [CRF-76] One variant now carries at least three distinct states, and the arm hardcodes one status and one label for all of them. (Chopper P2, Gon P2, Nami P3, Meruem P3)

Four reviewers reached this from four directions, and it is the design half of CRF-71.

unresolved-tool is now emitted for: an id with no tool at all, a resolved tool whose args have not streamed far enough (CRF-54's case), and a resolved tool the subagent guard is deliberately suppressing (CRF-71's case). The arm renders all three as status="running" with Waiting for tool details….

Chopper's angle is that the row asserts a status it cannot know. Gon's is that the name is now false for at least one of the states it covers. Meruem found the doc consequence: toolVisibility.ts's definition doc states two invariants this diff broke, and it sits exactly where the next reader looks. Leorio found the same contradiction written down in two files that now disagree, blockUtils.ts:44-46 saying the gap should be filled and toolVisibility.ts:80-82 saying the gap is the point.

Whichever way CRF-71 lands, the variant needs either a narrower definition or a discriminator the arm can render from. Splitting it also lets the label be right in each case, which is what CRF-56 was about two rounds ago.

🤖

Narrower definition, taken from the CRF-71 split. unresolved-tool now covers two states with one meaning: the tool is still arriving, either because no tool carries the id yet or because its arguments have not streamed far enough. status="running" is true of both and Waiting for tool details… describes both. The suppressed case is no longer routed here at all.

Chopper's objection is answered by the state leaving the variant rather than by a discriminator, and Gon's by the name being true again. Meruem's doc and Leorio's contradiction are handled in CRF-80.

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-29T15:07:49Z

+						return isStreaming ? (
+							<ToolCall.Root key={block.id} status="running" hasContent={false}>
+								<ToolCall.Header
+									iconName="unknown"


P3 [CRF-78] The row CRF-54 added only exists while the phase is streaming, so the blank gap reopens on reconnect and after settle. (Nami)

Nami measured the original gap last round and asked for this row; she is now reporting that the fix is narrower than the hole. The arm returns the placeholder only when isStreaming, and StreamingOutput renders blocks whenever hasAccumulatedOutput, so on a reconnect or once the stream settles the same non-renderable resolved tool goes back to rendering nothing while the shimmer stays suppressed.

Settled is arguably correct, since a finished transcript should not show a waiting row. Reconnecting is not: the tool really is still in flight and the user gets the blank she measured. That is the state EmptyToolResultBeforeItsCallSettled covers for the id-miss case, and the reason Chopper's CRF-81 matters, because that story asserts the spinner is gone rather than the row.

If CRF-71 splits the variant, this resolves with it: a suppressed-but-in-flight tool wants a row in every non-settled phase, and an id-miss wants exactly what it has now.

🤖

Not fixing in this PR, and I want to be clear that is a judgement rather than a disagreement with the measurement.

After the CRF-71 split the variant no longer covers a suppressed-but-in-flight tool, so the remaining question is narrower than when you raised it: should an id-miss row survive reconnect. Rendering it there means giving the transform a phase distinction it does not currently have, and the reconnecting callout already tells the user the stream dropped, which a waiting row would compete with rather than explain. That trade is worth measuring against the real reconnect UI rather than reasoning about, and I would rather not add a mode union on the strength of the reasoning alone.

The human has noted it.

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-29T15:07:49Z

 			],
+			[
+				{ type: "read-files", tools: [tool("read-1")] },
+				{ type: "response", text: "middle", sourceIndex: 1 },


P3 [CRF-72] CRF-63's sourceIndex is barely pinned: the one assertion sits where the two indices coincide, and reverting all eight keys leaves the story file green. (Bisky P3, Netero P3)

Netero measured the render half:

reverting all eight of them to the timeline index leaves the whole story file green.

Bisky found why the unit half does not help: the only sourceIndex assertion is at a position where the source index and the timeline index are the same number, so it cannot distinguish the field from the thing it replaced.

This is the fix that closed a three-round argument, so it is worth one test that would notice its removal: a read-files collapse followed by a thinking block, asserting the thinking block's sourceIndex is its position in the input rather than in the output. That is the exact divergence the whole design exists to produce.

🤖

Both halves pinned.

Unit: keeps a non-tool block's source index after a run collapses. Input is [read-1, read-2, thinking], output is two entries, and the thinking block's sourceIndex must be 2 while its output index is 1. That is the divergence, so the two numbers can no longer coincide.

Render: ReadFileRunCollapseKeepsThinkingExpanded in ConversationTimeline.stories.tsx. A stateful harness starts with read-2 unresolved, the play function expands the thinking disclosure, then resolves read-2 so the run collapses in front of it, and asserts the body is still visible.

Both verified red against sourceIndex: timeline.length, which is the revert you measured.

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-29T15:07:49Z

 	};
 };

+// The only place hidden entries are dropped, so every rendered row comes from


P3 [CRF-80] Three docs now state invariants this diff changed. (Gon P3, Meruem P3, Leorio Note)

Gon: so every rendered row comes from the result is no longer the whole truth, because a row can now come from a block whose tool the transform suppressed, and that path does not pass through this filter at all.

Meruem: the doc at shouldRenderTool's definition states two invariants this diff broke, and it is where the next reader looks to decide whether a suppression is honoured.

Leorio: blockUtils.ts:44-46 and toolVisibility.ts:80-82 now contradict each other about whether the gap should be filled or preserved.

All three are the prose half of CRF-71 and CRF-76. Whoever narrows the routing has to rewrite these three sentences in the same change, or the next reader re-introduces the flash on the strength of a doc comment. Recording them together so they travel with the code fix rather than after it.

🤖

Two rewritten, one deliberately left alone, and I want to say why for the third.

blockUtils.ts now states the narrowed meaning and that a deliberately hidden tool keeps its tool block, which closes Leorio's contradiction from that side. shouldRenderTool's doc says a row waiting only on its arguments is not hidden, and points at isToolPendingArgs, which is Meruem's.

Gon's sentence I reverted to its original wording. After the split, toTimelineBlocks asks isToolPendingArgs, not shouldRenderTool, so nothing downstream re-derives whether an entry is hidden and the sentence is true as written. I had a longer replacement in the working tree and deleted it, because it described the transform as deciding visibility, which is the thing the split stopped it from doing.

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-29T15:07:49Z

 }): boolean => {
 	if (name === "execute") {
-		return shouldRenderExecuteTool(getExecuteRenderData(args, result));
+		return getExecuteRenderData(args, result).command.trim().length > 0;


P3 [CRF-83] Answering "should this row render" builds the entire execute render payload. (Nami)

shouldRenderTool's execute branch calls getExecuteRenderData(args, result) and reads .command, which means the whole payload, including trimming the accumulated output, is built to answer a boolean. This was true before the diff and it is now called from a new place: toTimelineBlocks runs it per tool block per render, on the live path, where the accumulated output grows with the stream.

Killua's numbers put the per-call cost in perspective and he did not measure this branch specifically, so treat the magnitude as unknown rather than small. The shape point stands on its own: a predicate that allocates its answer's worth of data is the wrong thing to call in a transform that runs on every chunk. getExecuteRenderData already returns command first, so a narrower helper that reads only what the predicate needs is a small change.

🤖

Not fixing, on the grounds you set out: magnitude unknown.

getExecuteRenderData does no stringify and no work proportional to output length beyond one trim each of output, error and message, on strings the row is about to render anyway. It parses args, reads command, and builds a two-element array at most.

The shape point I accept in principle, but a narrower helper is a second definition of the same predicate's input, and the honest version of this fix is deleting a duplicate call rather than adding a function. If someone measures this branch and it matters, that is the change I would make.

The human has noted it.

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-29T15:07:49Z

+			continue;
+		}
+		if (tool.name === "read_file") {
+			readFileRun = readFileRun ? [...readFileRun, tool] : [tool];


P3 [CRF-84] The read-file run accumulator copies the whole run per element, so the transform is quadratic in run length. (Killua)

readFileRun = readFileRun ? [...readFileRun, tool] : [tool] allocates a new array per read_file block, so a run of n costs n(n+1)/2 copies. The tuple type does not force this: push is legal on [MergedTool, ...MergedTool[]], which Ging-TS noted independently last round.

Killua is the reviewer who measured CRF-50 at 8 microseconds and said it was not a problem, so his rating here is worth reading as calibration rather than alarm: this one is on the live path and scales with a number the agent controls, and a 40-file read run is not exotic. The fix is readFileRun.push(tool) with the same guard, which the accumulator's type already allows.

🤖

Fixed: readFileRun.push(tool) under the same if (readFileRun) guard, seeding [tool] in the else.

Measured at 200 tools before changing it: spread 0.056 to 0.073 ms per transform, push 0.022 to 0.023 ms. Small in absolute terms, and I took it because the tuple type already allowed it, so there was nothing to trade. The [MergedTool, ...MergedTool[]] annotation stays, which keeps deleting the flushReadFileRun guard a compile error.

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-29T15:07:49Z

-									subagentVariants={subagentVariants}
-									subagentStatusOverrides={subagentStatusOverrides}
-									mcpServers={mcpServers}
+					case "unresolved-tool":


P3 [CRF-85] The PR is typed refactor while its own body describes user-visible changes. (Pen Botter)

The body now names the execute shimmer window as a deliberate user-visible change, CRF-71 adds a second, and CRF-61's ask-question settle change is a third. refactor in this repo's commit convention means behavior-preserving, and the release-notes tooling reads the type.

This is not a request to relabel for its own sake. It is that three rounds of review have been calibrated by "this is a refactor, so a behavior change is a defect", and that framing is now doing work it should not. Either the behavior changes are intended, in which case the type and the opening sentence should say so, or they are not, in which case CRF-71 and CRF-78 are regressions rather than design choices.

🤖

Fixed, and Pen Botter is right that the framing was doing work it should not.

Retitled to fix(...), and the body has a Behaviour changes section naming three: the execute pre-args row, the metadata-only ask-question reply, and the word Tool leaving the unresolved row. All three are intended.

CRF-71 was a regression rather than a design choice, and is fixed in this push. CRF-78 is declined with reasoning rather than reclassified.

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-29T15:07:49Z

 		case "computer":
 			return <MonitorIcon className={base} />;

+		case "unknown":


Nit [CRF-73] case "unknown": is a useless case clause, and the project's own linter says so. (Netero)

Biome reports lint/complexity/noUselessSwitchCase here. Ging-TS checked whether it matters for CI and found it does not: biome lint --error-on-warnings on this file alone exits 0, so the rule is info-level and the build stays green. Either delete the clause or fold it into the default, whichever reads better next to its neighbours.

🤖

Deleted. The default arm already returns the wrench that case "unknown" wanted.

🤖 Replied with Coder Agents.

…suppressed Routing every non-renderable tool into unresolved-tool made a running wait_agent render "Waiting for tool details…" during the window shouldRenderSubagentLifecycleTool exists to keep blank, and chat_id is that tool's only argument. isToolPendingArgs now answers the narrower question the transform needs, so unresolved-tool means the tool is still arriving and a deliberately suppressed row keeps its tool block for <Tool> to drop. mergeTools iterates blocks like buildStreamTools does, so a tool is paired with the block that renders it and one id cannot produce two rows. The seen set and the orphan-result sweep go with it, as does a round-4 assertion that named a block ordering the old implementation did not have. ownValue moves to runtimeTypeUtils and guards every provider-keyed lookup: the stream call and result maps, both subagent name maps, and the renderer registry. A tool named after an Object.prototype member threw while building its subagent descriptor and took the whole route down with it. Also from review: the read-file accumulator pushes instead of copying the run per element, the reconnecting story asserts the row's label is absent rather than its spinner, sourceIndex is pinned where it diverges from the output index, and a useless ToolIcon case clause is gone.

DanielleMaywood · 2026-07-29T16:18:47Z

Round 5 pushed as 7435d70355. Twelve inline threads answered individually; the three findings that arrived in the review body rather than on a line are here.

CRF-74, fixed, and Leorio found the sharp end of it. The registry lookup is now ownValue(toolRenderers, name) ?? GenericToolRenderer, and ownValue moved out of streamState.ts into ChatElements/runtimeTypeUtils.ts so all six reads share one definition: the stream call map, the stream result-reuse check, the stream result map, actionByToolName, variantBySpawnToolName and the registry. The cast on getVariantFromName went with it.

His unverified half is worse than he guessed, and it fires earlier than the registry. isSubagentToolName("valueOf") was true, because actionByToolName["valueOf"] returned Object.prototype.valueOf, so <Tool name="valueOf"> never reached the registry: it went to SubagentRenderer and threw TypeError: Cannot read properties of undefined (reading 'fallbackTitle') at subagentDescriptor.ts:147, where subagentCatalog[variant] was indexed with a function. React Router's error boundary replaced the whole route, so the entire transcript disappeared rather than one row. constructor behaves the same way.

Ging-TS's streamState.ts:105 case is fixed by the same helper, so a prototype-named id no longer files its result under a fallback and renders one tool as two rows, and :58 and :109 can no longer label a row with a function name.

Pinned by ToolNamedAfterObjectPrototypeMember in Tool.stories.tsx, verified red when the subagentDescriptor.ts and Tool.tsx guards are reverted.

CRF-77, keeping the guard, and saying which. You were right that the justification was the inconsistent part, so here is the answer at the line: after CRF-71 the guard is not defensive, it is the only code path that suppresses a subagent lifecycle row. The transform diverts a tool whose arguments have not streamed yet and nothing else, so a suppressed wait_agent reaches <Tool> and this guard is what drops it. The comment at the line now says that in one sentence, and keeps a suppressed subagent lifecycle tool as a tool block fails if the routing changes underneath it.

That also means the four rounds of deletions and this guard are on the same rule rather than two: delete what no caller reaches, keep what one caller depends on. This one now has a caller.

CRF-86, not in this PR. Both reviewers are right that there is one marker with readers and one without, and Ging-TS narrowing the comparison set to the four ToolCall.Root block kinds is the useful framing. The fix I would take is deleting data-tool-call and giving the two rows data-transcript-row, which is a rename touching production and stories outside this diff's argument. Carrying both is not the option I would defend; it is the option I am leaving in place for one more PR.

The human has noted it.

Description. The TimelineBlock snippet now shows the sourceIndex shape, the size figures are recomputed from git diff --numstat against origin/main rather than carried forward, the story count is six, and the title is fix(...) with a Behaviour changes section naming all three.

🤖 Replied with Coder Agents.

DanielleMaywood · 2026-07-29T16:22:47Z

/coder-agents-review set-spend-limit:600

coder-agents-review

CI is green for the first time in six rounds, both round-5 P2s are fixed, and one of them by the route you had declined twice: the mergeTools recut landed, both MergedTool[] producers are now block-driven, and the orphan sweep is gone. ownValue moved into runtimeTypeUtils.ts and is applied at six sites with a story named after the hazard. The CRF-71 routing split landed with a wait_agent fixture. That is the shape of a PR converging.

Four P2, eight P3, two P4. Almost all of them are on isToolPendingArgs, the helper this round added, which is the pattern of the last three rounds: the fix is right and the new code it introduces is where the next findings live.

CRF-89 is eight reviewers on one gap and Hisoka found the producer in your own backend. isToolPendingArgs reads the command and nothing else, so for a tool that will never have one it returns true forever. chattool/execute.go:140-142 rejects an empty command with command is required, and a guard that exists means the state occurs. The row then claims status="running" and Waiting for tool details… for the rest of the stream, and Nami traced the settle half: the error goes down with it, no row and no message. That is worse than the blank gap CRF-54 set out to fix, because a blank gap is silence and this is a wrong claim.

CRF-90 is CRF-83 re-raised, and the new evidence is that your decline reason no longer holds. You said a narrower helper would be a second definition of the same predicate's input. Hisoka checked what the input is: command comes from parseArgs(args) alone, and result is consulted only for fields the predicate discards. The reason result was ever on this path died in this PR when authenticateURL went with shouldRenderExecuteTool. And this round put the walk on toTimelineBlocks, which runs per chunk on the live tail, which is the one place the magnitude argument stops being comfortable.

CRF-91 is five reviewers on the sentence you added to close the doc half. It says a row waiting on its arguments is not hidden; shouldRenderTool returns false for exactly that row, and the test forty lines below is called hides execute rows with no command. The sentence is true about toTimelineBlocks and false about the function it sits on, and Gon named the reader it misleads: getRenderableContentState, the second consumer, filters that tool out and lets the message be hidden.

CRF-92 is Bisky measuring the one decision the recut makes. name picks the renderer, and for a result-only row result is the only source of it. He set name: call?.name ?? "" and the whole unit project passed, 3070 tests.

Razor rendered two rows for one tool id, which makes the new doc's "one id cannot produce two rows" false across messages even though it is true per message. He is explicit that he could not show chatd emits that message shape, so the duplicate is unproven and the invariant is not.

One process note and then I will stop. This is the sixth round, the findings are getting smaller, and the pattern is stable: each round's fix is sound and introduces the next round's surface. That is not a criticism of the work, it is what a five-hundred-line rewrite of a render path looks like when it is reviewed properly. If you want a natural stopping point, CRF-89 and CRF-91 are the two I would not merge without, and the rest would survive being follow-ups if anyone were willing to file them.

Hisoka, on finding the producer in the Go: "A guard that exists means the state occurs."

site/src/modules/roles/RoleSelector.tsx:59

P4 [CRF-101] Object.groupBy needs Safari 17.4 and this repo declares Safari 16.0. (Ging-TS)

Outside this diff and outside this PR's area, found while checking the diff's own language-level choices against the declared browser targets. Recording it because it is the kind of thing nobody goes looking for: the file uses Object.groupBy, which is ES2024 and shipped in Safari 17.4, against a browserslist that still names 16.0.

Not this PR's to fix. Worth a separate issue if anyone still supports that target, and worth ignoring if the browserslist entry is stale, which is the more likely answer.

site/src/pages/AgentsPage/components/ChatConversation/ConversationTimeline.tsx:222

P4 [CRF-88] CRF-86 is deferred one PR with no ticket, which by this review's own rule leaves it open. (Netero)

You agreed with both reviewers on data-tool-call, named the fix you would take, and declined it as a rename touching production and stories outside this diff. That reasoning is fine and I would take the same call.

What the rule is about is the record. Rounds 3, 4 and 5 closed CRF-13, CRF-33 and CRF-48 on the same standing instruction against filing tracking issues, with the human noted as informed. That is now four open gaps carried on one person's memory, and this one carries a stated intent to fix it in one more PR, which is the kind of promise that survives exactly as long as the person who made it is looking at this branch.

Nothing for you to do in the code. This is the fourth entry on a list somebody should decide about in one go, and I am recording it as open rather than accepting the deferral so the list stays countable.

coder-agents-review · 2026-07-29T17:27:55Z

+ * True while a row's arguments have not streamed far enough to render it, which
+ * earns a placeholder rather than the silence a hidden row gets.
+ */
+export const isToolPendingArgs = ({


P2 [CRF-89] isToolPendingArgs has no terminal state and never reads status, so a settled command-less execute shows a waiting row forever and takes its error down with it. (Hisoka P2, Pariston P2, Meruem P2, Mafuuu P3, Gon P3, Melody P3, Takumi P3, Nami P3)

Eight reviewers, and Hisoka found the producer in your own backend:

coderd/x/chatd/chattool/execute.go:140-142: if args.Command == "" { return fantasy.NewTextErrorResponse("command is required") }. A guard that exists means the state occurs.

The predicate reads the command and nothing else, not status, not isError, not whether a result arrived, so for that tool it returns true forever and blockUtils.ts:77 emits unresolved-tool forever. The arm hardcodes status="running" and Waiting for tool details… at ConversationTimeline.tsx:356-360.

Nami traced the settle half and it is the worse one: once the stream ends the arm renders null, so the error response never appears. The user asked for something, the agent returned command is required, and the transcript shows nothing at all.

This is CRF-76 reopening through the door CRF-71's split left. The split narrowed the variant, it did not empty it. Adding status to the predicate is the small fix: a tool with a result is not pending, whatever its args say. The clean fix is the one CRF-76 asked for, a discriminator on the variant so the arm can render waiting, settled-without-args and id-miss differently.

🤖

Fixed, and Hisoka's rule was the one that mattered: I confirmed execute.go:140-142 and that fantasy.NewTextErrorResponse sets IsError, so the state is produced, not hypothesised.

Measured both halves before changing anything. Streaming rendered Waiting for tool details…; settled rendered innerHTML === "". command is required was invisible in both. Three existing tests were pinning that, so they moved with the fix.

The fix is not the status check, and finding out why was the useful part of this round. I applied status === "running" first and HiddenAssistantToolMessageDoesNotRenderGap went red: a persisted execute with empty args and no result would have started rendering an empty Ran row, which is the gap shouldRenderTool exists to prevent. So the predicate keys on the result instead:

name === "execute" && result === undefined && asString(parseArgs(args)?.command).trim().length === 0;

A row with a result is never pending, whatever its args say, so the error reaches the transcript; a row with neither is still hidden, so the empty gap stays closed. One line shorter than the status version and it does not need status at all.

I did not take the discriminator. After this, unresolved-tool carries one meaning again, status="running" is true of every member, and the arm's label describes all of them, so there is nothing for the arm to discriminate on. Pinned by keeps an execute whose result explains its missing command, verified red when the result check is removed.

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-29T17:27:55Z

+	args?: unknown;
+	result?: unknown;
+}): boolean =>
+	name === "execute" &&


P2 [CRF-90] CRF-83 re-raised: the predicate still builds the whole execute payload to read args.command, and this round put that walk on the per-chunk transform path. (Hisoka P2, Meruem P2)

You declined CRF-83 on magnitude and on the argument that a narrower helper "would be a second definition of the same predicate's input." Hisoka checked what the input actually is:

getExecuteRenderData derives command from parseArgs(args) and nothing else. result is consulted only for output, error, message, wall_duration_ms and background_process_id, every one of which isToolPendingArgs discards. So the predicate passes result in for the sole purpose of throwing it away.

And the reason result was ever on this path died in this PR: authenticateURL went with shouldRenderExecuteTool, and the plumbing stayed. So the narrower helper is not a second definition, it is the definition with the dead parameter removed.

The part that changed the severity is the new caller. blockUtils.ts:77 calls this for every tool block, and toTimelineBlocks runs per chunk on the live tail, where the accumulated output grows with the stream. Meruem reached the same conclusion independently. Neither benchmarked it, and Killua's 8-microsecond measurement from CRF-50 was on a different function, so treat the magnitude as still unmeasured rather than small.

The fix is args.command and no result parameter, which also makes CRF-95's desync impossible to write.

🤖

Fixed, and my decline was wrong on both counts. Recording both, because the reasoning failure is more useful than the fix.

Hisoka is right that this was not a duplicated definition. I checked command against nine result shapes, including an adversarial {command: "SHOULD-NOT-LEAK"} in the result, and got one distinct value in each direction: result cannot influence the answer. So the narrower version removes a parameter rather than restating an input.

And I declined on magnitude in the same breath as saying magnitude was unmeasured, which is not a decline, it is a guess. The number I should have produced last round, 50 tool blocks with one 200 KB accumulated output, 500 calls per sample, three separate process runs:

variant median µs per toTimelineBlocks

getExecuteRenderData 10.86 / 11.05 / 10.41

args.command 5.04 / 7.22 / 5.75

About 5 µs, half the call, because the old path trimmed the whole output buffer to answer a boolean.

One correction: result stays in the signature, and is now load-bearing rather than discarded. It is the result === undefined test that CRF-89 turns on, which is O(1).

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-29T17:27:55Z

 * Centralize tool-row visibility so transcript message hiding stays in sync
- * with <Tool> row rendering and hidden rows never leave empty gaps behind.
+ * with <Tool> row rendering and hidden rows never leave empty gaps behind. A
+ * row waiting only on its arguments is not hidden: see `isToolPendingArgs`.


P2 [CRF-91] The new sentence says a row waiting on its arguments is not hidden. shouldRenderTool returns false for exactly that row. (Gon P2, Mafu-san P2, Hisoka P3, Leorio P3, Zoro P3)

Five reviewers, and Gon ran the contradiction:

shouldRenderTool for execute is !isToolPendingArgs(...), so a row whose command has not arrived gets false, which is this file's word for hidden. The test 40 lines below is called hides execute rows with no command and asserts toBe(false); I ran it, it passes.

The sentence is trying to say something true about a different module, that toTimelineBlocks routes the block to unresolved-tool rather than dropping it. Gon named the reader it misleads, which is why this is a P2 and not a wording nit: shouldRenderTool has a second consumer, getRenderableContentState at messageHelpers.ts:50, which filters the tool out of visibleBlocks, and an assistant message left with nothing else is then hidden. So on the historical path the row really is hidden, and the doc at the definition says it is not.

Zoro found the same gap from the promise side, that the doc offers "a placeholder rather than the silence a hidden row gets" for a function that cannot deliver a placeholder. Say where the placeholder comes from rather than what this function does not do.

🤖

Fixed, and Gon's second-consumer claim reproduced: an assistant message whose only tool is a command-less execute yields display length: 0, so on the historical path the row really is hidden.

The sentence is gone rather than corrected. Zoro's framing is the one I used, say where the placeholder comes from, so the doc now names the module that produces it:

True while an execute row has neither a command nor a result: nothing can render from it, and toTimelineBlocks holds its place with an unresolved-tool row while the stream is live. A row with a result is never pending, so an error about the missing command still reaches the transcript.

shouldRenderTool's own doc lost the added sentence entirely and is back to describing only what it guarantees.

This is the third round with a finding on prose I wrote to close a previous finding on prose, which is its own signal. The error both times was the same: writing a claim about the system next to a function that only implements part of it.

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-29T17:27:55Z

 	});

-	it("includes orphaned results that have no matching call", () => {
+	it("includes a result whose call never arrived", () => {


P2 [CRF-92] The result-only case asserts id and status and nothing else, so the one decision the recut makes is unpinned. (Bisky)

mergeTools is now const source = call ?? result with name: source.name. For a tool role message whose call was never persisted, parseMessageContent still calls ensureToolBlock on the tool-result part, so the block exists, call is undefined, and result is the only source of name. name is what picks the renderer at Tool.tsx:1033.

Run, not read: I changed name: source.name to name: call?.name ?? "" and ran the whole unit project. 199 files, 3070 passed, 2 skipped, zero failures. Every reloaded transcript would render its result-only rows through GenericToolRenderer with a blank label and the suite would stay green.

This is the same hole CRF-52 found on the live producer two rounds ago, in the producer that just inherited the same shape. The fix there was an exact-object assertion, and it is the fix here.

🤖

Fixed. Bisky's mutation reproduces exactly: name: call?.name ?? "" leaves 199 files and 3070 tests green.

The assertion is now the whole object. One note on the shape: I dropped the args/mcpServerConfigId/modelIntent/parsedCommands entries I had first written as undefined, because toEqual treats undefined and absent as equal, so they asserted nothing. Omitting them still fails if any of those fields gains a value, which is the property worth having.

Verified red under the mutation and green on the fix.

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-29T17:27:56Z


+/**
+ * Iterates blocks rather than the call list, so every tool is paired with the
+ * block that renders it and one id cannot produce two rows.


P3 [CRF-93] The new doc's "one id cannot produce two rows" is false across messages, and Razor rendered the two rows. (Razor)

parseMessagesWithMergedTools calls mergeTools once per message with that message's blocks, while globalToolResults back-patches a result across messages and parseMessageContent calls ensureToolBlock on the result part too. So an id whose call is in message 1 and whose result is in message 2 gets a block in both, and both resolve. I ran it: two visible entries, tool ids ['t1', 't1'].

He is careful about what that proves. What prevents the duplicate in practice is not the pairing the comment credits; it is isToolResultOnlyEntry, which hides the second message only when its role is literally "tool" and it carries no calls, markdown or reasoning. His repro used an assistant message carrying text plus the result, and he could not show chatd emits that shape, so the user-visible duplicate is unproven and the false invariant is not.

The comment is the stated payoff of the recut and the PR body repeats it verbatim as the reason three miss handlers could go, so it is worth making precise. If the invariant is per message, say so, because the deletions it justifies are per timeline.

🤖

Fixed as wording, and Razor's care about what he had proven turned out to be the important part.

I reproduced the two rows through the real pipeline. Two variants give ["t1","t1"]: an assistant message carrying text plus the result, and an assistant message carrying the result alone, because isToolResultOnlyEntry requires role === "tool". His exact execute repro yields one row only because the second degrades to unresolved-tool.

Then I read the Go rather than guessing, which is what he asked for and could not do. splitStepContent pulls every non-provider-executed tool result out of the step content and buildCommitStepMessages writes each as its own ChatMessageRoleTool message. The streaming path does the same through appendToolResult, as do synthetic interruption results, task interruptions, dynamic-tool submissions, synthetic replays and compaction. The only tool-result part that can sit in an assistant message is provider_executed, which parseMessageContent skips before a block exists. So the shape is unproducible, and isToolResultOnlyEntry alone was never the guard.

The comment now says the invariant is per message and where the cross-message case goes, and the PR body no longer repeats the broader claim.

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-29T17:27:56Z

+		}
+
+		const tool = toolByID.get(block.id);
+		if (!tool || isToolPendingArgs(tool)) {


P3 [CRF-97] CRF-54's blank-gap fix covers execute and skips its three subagent siblings, so a live lifecycle call keeps the gap. (Razor)

The CRF-71 split fixed the flash by routing only isToolPendingArgs tools into the variant, and isToolPendingArgs is name === "execute" with an empty command. A running wait_agent, send_agent_message, close_agent or interrupt_agent whose chat_id has not arrived is now correctly not flashing generic copy, and is also back to rendering nothing while the shimmer stays suppressed, which is the exact gap CRF-54 measured for execute.

So the two findings are each other's mirror and the split chose one. That may be the right call: a lifecycle row has no useful placeholder text until its title resolves, and Waiting for tool details… was the wrong copy for it. But the blank window is real and nothing records the decision. One sentence at the predicate saying subagent lifecycle tools deliberately get silence rather than a placeholder closes it, and it is the same sentence CRF-91 needs.

🤖

Confirmed, recorded, and one name in it is wrong.

Rendered each one through StreamingOutput with phase: "streaming". wait_agent, message_agent, close_agent and interrupt_agent each produce zero rows and zero shimmer, an empty container, the same signature CRF-54 measured for execute. So the blank is real.

send_agent_message is not a subagent lifecycle tool. actionByToolName maps wait_agent, message_agent, close_agent and interrupt_agent; send_agent_message is unmapped and renders an ordinary generic row. The fourth name is message_agent.

Keeping the silence, and the decision is now written where it is made, on the lifecycle predicate rather than on shouldRenderTool:

Wait, message, and interrupt rows can stream before their target chat_id arrives. They get silence rather than a placeholder, because generic lifecycle copy is wrong until the transcript resolves the real title.

That is the same sentence CRF-91 needed, as you said, and it replaced the previous comment rather than being added next to it.

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-29T17:27:56Z

 						const responseEl = isStreaming ? (
 							<SmoothedResponse
-								key={`${keyPrefix}-response-${index}`}
+								key={`${keyPrefix}-response-${block.sourceIndex}`}


P3 [CRF-98] CRF-72's fix pins one of the five sourceIndex render keys; the other four revert green. (Nami)

The new test covers the thinking key. Nami reverted each of the remaining four to the timeline index in turn and the suite stayed green for all four: the two response keys and its Fragment, file-reference, file and sources.

This is the third round on the same coverage question, and it is worth one test rather than a fourth: a fixture with a read-files collapse followed by a response and a sources block, asserting both keys carry their input positions. That is the divergence the whole sourceIndex design exists to produce, and it covers the four in one story.

🤖

Fixed, and Nami's measurement was complete: I reverted each of the six sites in turn and confirmed all six were green, using the thinking key as a control to prove the harness detects a remount at all.

One story pair now covers every one of them. ReadFileRunCollapseKeepsRowsMounted renders a block list of [read-1, read-2, thinking, response, file-reference, file, sources], expands the thinking disclosure, captures each row's node, then resolves read-2 so the run collapses and every later timeline index shifts. A key that moved would unmount the row and detach the captured node.

The mutation matrix, one revert at a time:

reverted key fails

303 SmoothedResponse streaming story

310 Response non-streaming story

317 Fragment both

336 file-reference both

404 file both

415 sources both

That matrix is also why there are two stories rather than one. A reviewer of my own diff argued the streaming story was redundant because lines 303 and 310 use a character-identical key expression; the matrix says reverting 303 fails only the streaming story, so deleting it would leave that key exactly as unpinned as the four you found.

Two fixture notes for anyone touching it: the file block must omit file_id, since the key is block.file_id ?? block.sourceIndex, and the attachment is inline data so nothing awaits a fetch.

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-29T17:27:56Z

+			],
+		],
+		[
+			"a tool whose row cannot render yet",


P3 [CRF-99] The only test pinning the pending-args route uses a settled tool, so the behaviour the body calls its headline is unpinned. (Hisoka)

The new test asserts a wait_agent keeps its tool block, which is CRF-71's fix. What no test covers is the case CRF-54 added the variant for: a live execute mid-args that should route to unresolved-tool. The fixture is settled, so it exercises the negative side of the branch only.

That is also why CRF-89 stayed green through this round. A test with a running execute whose args hold {"comm would pin the route and fail the moment isToolPendingArgs gains the status check it needs, which is exactly the kind of test you want to write before that fix rather than after.

🤖

Fixed, with one correction to the finding.

Deleting the pending-args branch does fail a test, does not collapse read_file blocks across a tool whose row cannot render yet. But the substance holds completely: that fixture is status: "completed" with no result, and every fixture on this route was settled, so nothing exercised the live half. The wait_agent test you name actually pins the opposite route, that a suppressed row stays a tool block.

The fixture is now a running execute carrying the partial fragment {"command": "git ch and no result, which is what arrives mid-stream, and the case is renamed for what it tests.

Your point about writing it before the fix rather than after is the right process note and I did it in the wrong order again: I fixed CRF-89 first and then found this fixture was settled. Written first, it would have failed and told me what CRF-89 tells me now.

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-29T17:27:56Z

 							<ReadFileTimelineBlock
-								key={firstGroupTool.id}
-								tools={[firstGroupTool, ...restGroupTools]}
+								key={block.tools[0].id}


P4 [CRF-100] The read-files row is keyed by its first tool's id, which moves when the run grows at the front. (Kite P4, Komugi P4)

The key is block.tools[0].id. A read-file run only grows at the back today, because ensureToolBlock appends, so the first id is stable in practice. Both reviewers found the same theoretical hole and both rated it P4 for that reason: nothing in the transform guarantees the property, and a future producer that inserts a read ahead of an existing run remounts the whole group and loses its expanded state.

Komugi notes the cheap version is keying on the block's sourceIndex, which this round added for exactly this class of problem and which the read-files arm does not use.

🤖

Declining, and both reviewers were right to rate it P4.

Confirmed runs only grow at the back: ensureToolBlock appends, parseMessageContent walks parts in order, and buildDisplayMessages appends later entries to the end of a group. I also checked the other way tools[0] could shift, a leading unresolved block resolving later, and it is unreachable: a tool block is only created alongside a call or result part, so mergeTools never skips one, and isToolPendingArgs only fires for execute, which never sits in a read-file run.

Komugi's suggestion costs about 13 production lines, because the read-files variant has no sourceIndex and needs one threaded through the run accumulator. I would rather not widen the variant for a state no producer can reach, having spent this PR arguing that unreachable code should go rather than grow.

Not filed as a follow-up, per the standing instruction. Declined, so it is not waiting on anyone's memory.

🤖 Replied with Coder Agents.

coder-agents-review · 2026-07-29T17:27:56Z

+		const call = ownValue(state.toolCalls, block.id);
+		const result = ownValue(state.toolResults, block.id);
+		const source = call ?? result;
+		if (!source) {


P3 [CRF-87] A stream whose only block is unresolved renders the placeholder row and the generic "Thinking" shimmer at the same time. (Netero)

buildStreamTools skips a block with neither call nor result, so that block contributes nothing to streamTools, so hasRunningTool(streamTools) is false and shouldShowGenericThinking leaves the shimmer up. The arm still renders its running row. The user gets two progress indicators for one thing.

Nami recorded this shape as a Note in round 4, when it was the mirror image of the blank gap and both sides were close to unreachable. It is a finding now because CRF-54 made one of the two states common: an execute mid-args reaches the arm on every call, and after CRF-71's split that is exactly the population the variant carries.

The two derivations disagree because one counts blocks and the other counts tools, which is the same split CRF-30 closed on the historical path by making both block-driven. Either shouldShowGenericThinking takes unresolved blocks into account, or the arm suppresses the shimmer the way a resolved running tool does.

🤖

Fixed, and your severity call was right in a way worth stating: this only reaches the id-miss path.

I checked the common case rather than assuming it was included. An execute mid-args has a call in the stream state, so buildStreamTools produces a tool for it, getStreamToolStatus(undefined) returns "running", and the shimmer is already correctly suppressed. Only a block with neither call nor result reaches the double indicator, which I then reproduced: "Waiting for tool details…Thinking", both live.

Fixed the way you framed it rather than by adding a clause. hasRunningTool counted tools; it is now hasRunningToolBlock and counts blocks, treating a block with no tool as the running row it renders. One derivation instead of two, which is the CRF-30 shape you pointed at.

That also exposed why this stayed green: the existing fixture paired block id read-1 with tool id completed, so the two derivations were never compared on the same tool. Ids aligned, and hides for streaming with a tool block whose tool has not arrived is red against the old tool-counting version.

🤖 Replied with Coder Agents.

…ute earns An execute with no command is a real backend response: chattool rejects it with "command is required". isToolPendingArgs read only the command, so that tool was pending forever, showing "Waiting for tool details…" while the stream lived and nothing at all once it settled. A row with a result is never pending, so the error reaches the transcript. The predicate now reads args.command directly rather than building the whole execute payload to answer a boolean, which halves the cost of a transform that runs per chunk on the live tail, and shouldRenderTool asks it instead of re-deriving the execute test. The tool lookup keeps a queue per id. Merging read-file messages concatenates their tools, and a call arriving without an id falls back to a per-message index, so one entry could hold two tools claiming one id; the last-wins map then showed the same file twice and dropped the other. Generic thinking counts tool blocks rather than tools, so a block still waiting for its tool no longer renders a placeholder row and a shimmer for the same work. Its test fixture paired a block id with an unrelated tool id, which is why the disagreement stayed green. Also from review: the merged-tool invariant is per message and says so, an unread data-tool-call attribute becomes the data-transcript-row its rows were missing, the result-only merge asserts its whole object, and one story pair pins every sourceIndex render key rather than the one the thinking row used.

DanielleMaywood · 2026-07-29T19:29:00Z

Round 6 pushed as 964d5cc13a. Thirteen inline threads answered individually; the two findings that arrived in the review body are here, plus one process note that is mine rather than yours.

CRF-88, taken, and it cost two lines. You and Netero were both right that carrying two markers helps nobody, and my "one more PR" deferral was the weakest part of last round's reply. data-tool-call is now data-transcript-row at both producers, which is a rename rather than a deletion: those two rows were the only ToolCall.Root wrappers the eight existing [data-transcript-row] selectors could not reach. Full AgentsPage story run is unchanged, so nothing depended on the old name, which is the point.

Honest caveat: this is unpinned by construction. Adding an attribute that existing selectors can use does not make any of them use it, so no test would fail if it were reverted. I am not adding a test for an attribute; I am noting that the guarantee here is the grep, not the suite.

CRF-101, confirmed, and the browserslist entry is the stale half. Both facts hold: RoleSelector.tsx:59 uses Object.groupBy, and site/package.json resolves to chrome 110 firefox 111 safari 16.0. Three things say the entry is dead rather than the call site being wrong. It was last touched in 69e963b1a2 in January 2024, as a drive-by inside an unrelated dashboard refactor. .toSorted and .toReversed already appear at 17 sites across src, and both need Safari 16.4, so the floor is violated repo-wide independently of this. And nothing enforces it for JS: the only consumer is postcss.config.js for autoprefixer, vite.config.mts sets no build.target, and tsconfig.json is esnext.

So the useful action is deciding the real support floor, which is not this PR's to decide. Declined outright rather than tracked, per a standing instruction against filing tracking issues; the same applies to CRF-96 and CRF-100.

It did change one decision inside the diff, though. A reviewer of my own change suggested Map.groupBy for the new per-id lookup, which would have saved eight lines. I rejected it: that is the same ES2024 Baseline feature this finding is about, and adding a second instance of it in the round it was raised is bad judgement whatever the browserslist eventually says.

Process, since you raised it. I categorised your fifteen findings by what they point at: twelve are on code this PR added in rounds 4 and 5, and six of those are on the eleven-line helper I added last round. That is not primarily a review-volume problem, it is that I have been fixing findings by adding code and then only mutation-testing the specific branch the reviewer named, so the untested half ships and comes back. CRF-99 is the clearest instance and you said so directly.

Two concrete changes this round. Every new assertion was mutation-checked against every branch of the code it covers, not one: that is where the six-way sourceIndex matrix in CRF-98 came from, and it is why CRF-98 ended as two stories rather than one. And both declines are backed by something I ran, because the two CRF-83 and CRF-82 declines that came back as CRF-90 and CRF-94 were both falsifiable by reading, and both got falsified.

Two of your findings were also partly wrong and I have said which in their threads: CRF-99's "no test fails" (one does), and CRF-97's send_agent_message (not a lifecycle tool; message_agent is). CRF-94 was worse than either reviewer could show, and CRF-89's proposed status fix would have introduced a regression that an existing story caught.

Description. Behaviour changes names the failed-execute row, the size figures and story count are recomputed, and the merged-tool invariant no longer claims across-message uniqueness.

🤖 Replied with Coder Agents.

DanielleMaywood · 2026-07-29T19:30:29Z

/coder-agents-review set-spend-limit:800

coder-agents-review

All four round-6 P2s fixed, CI green, and all three declines came with work behind them rather than an assertion: you wrote the mergeToolBlocks helper before rejecting it on measured cost, you traced the read-files prepend route before declining the key change, and you dated the browserslist entry before calling it the stale half. Two of the three declines survive the panel. One does not, and Netero found why: the route that can prepend to a read-file run is this PR's own !tool miss, not isToolPendingArgs, so the reachability argument was checked against the wrong branch.

Five P2, eight P3, two P4. The pattern is the same as the last three rounds, and it is now sharp enough to name: every P2 this round is a consequence of a round-6 fix.

CRF-106 is the one to read first. CRF-89's fix does what it says, a command-less execute now reaches the transcript instead of vanishing. Leorio rendered the row that arrives:

Six reviewers, two at P2, and Nami independently found the same row. The fix restored a row nobody had looked at, which is fair, and the row is not readable.

CRF-107: that fix has two halves and one is pinned. Bisky reverted shouldRenderTool to its pre-fix predicate and every unit test stayed green, because the amended test deleted the negative case's result rather than adding a positive one. CRF-108: CRF-94's fix made the two colliding tools distinct in the timeline and left them sharing an id, and ReadFilesTool keys both its rows and its expansion state on that id. Ging-React ran it and got React's duplicate-key warning plus both files expanding on one click.

CRF-110 is the one worth thinking about beyond this round. Ryosuke checked the PR's own thesis, that a block is a row, and found the tool variant does not carry it: a suppressed subagent lifecycle tool becomes {type: "tool", tool} and <Tool> returns null, so the never check proves the renderer handled the case, not that the case produced output. That is not a regression, it is the shape the design settled into after CRF-71, and it means consumers still re-derive renderability.

Komugi found a new flake in a story added this round, measured rather than suspected: the reveal is a requestAnimationFrame loop, 326 ms of a 1000 ms budget on an idle box, and shimming frames to 250 ms turns it red.

Seventh round, so one honest observation about the process rather than the code. The findings are not getting smaller, they are getting later: each round's fix is sound and its new surface carries the next round's P2s. Three of this round's five are strictly consequences of round 6, which means the review is now tracking the work rather than gating it. If it were my call I would take CRF-106, CRF-107 and CRF-108, which are the three that make a shipped row wrong, and let the rest ride as follow-ups with somebody's name on them.

Ryosuke, on what the exhaustiveness check actually buys: "the never check on the switch proves the renderer handled the case, not that the case produced output."

site/src/pages/AgentsPage/components/ChatElements/tools/ExecuteTool.tsx:163

P2 [CRF-106] The row CRF-89's fix restores renders its headline as Ran with nothing after it. (Leorio P2, Nami P2, Bisky P3, Mafuuu P3, Pariston P3, Kite P3)

Six reviewers, and Leorio rendered both variants:

Both assertions passed as written. The one fact that matters, that the agent asked for a command and never supplied one, is behind a disclosure, and the header reads like the page failed to load.

The fix is right and this is its second half: you restored a row that had never been looked at, because before CRF-89 it was unreachable. ExecuteTool needs a branch for an empty command, whether that is the error text in the header, a Ran no command label, or dropping the using clause when there is nothing to append. Nami's version of the same finding adds that the row also reads wrong under always_collapsed, where the header is the whole row.

Worth saying plainly: this is the best kind of finding to get on a fix, because it means the fix reached production behaviour.

site/src/pages/AgentsPage/components/ChatElements/tools/ReadFilesTool.tsx:51

P2 [CRF-108] CRF-94's fix made the colliding tools distinct in the timeline and left them sharing an id, which is still the render identity here. (Ging-React P2, Hisoka P3, Chopper P3, Meruem P3, Zoro P3)

Five reviewers, and Ging-React ran it:

The queue at blockUtils.ts:86 fixed the lookup: two blocks with one id now get two different tools instead of the same one twice. It did not touch identity, and ReadFilesTool keys both key={item.id} and expandedFileIDs.has(item.id) on that id, so the group renders with duplicate keys and one click expands both files.

Meruem found the second half of the same fix's assumption, which is worth fixing in the same edit: the queue pairs the Nth block with the Nth tool of the same id, so tools must arrive in block order for the pairing to be right, and nothing states or checks that. key and the expansion set want the block's position or a composite, not the wire id.

site/src/pages/AgentsPage/components/ChatConversation/ConversationTimeline.stories.tsx:2838

Nit [CRF-118] The story reading this round's new data-transcript-row wrapper falls back to the button when the wrapper is absent, so it cannot fail. (Bisky)

The selector takes the wrapper if it is there and the button otherwise. The wrapper is what CRF-104's fix added, so the one assertion that would notice its removal is written to survive it.

Small, and worth fixing while the fix is fresh: assert the wrapper directly. Bisky's point is that a fallback in a test that exists to pin a new attribute is the test agreeing in advance to stop pinning it.

site/src/pages/AgentsPage/components/ChatConversation/streamingJson.ts:343

P4 [CRF-120] extractIncompleteStringContent is called at an index that still points at the whitespace before the value. (Chopper)

Outside the diff's own lines and inside its blast radius, because this function is what shortens CRF-112's placeholder window. If the index is off by the whitespace, a partially streamed command is not extracted as early as it could be, so the placeholder shows for longer than the design intends.

Chopper rated it P4 because he could not demonstrate a payload where the whitespace is present, providers generally emitting compact JSON. Recording it next to CRF-112 so whoever measures that window checks this first.

site/src/pages/AgentsPage/components/ChatConversation/AttachmentBlocks.tsx:73

P4 [CRF-119] The ownValue enumeration misses two record lookups whose keys come from outside the frontend. (Melody)

CRF-74's fix enumerated the provider-keyed lookups and covered six sites. Melody walked the same pattern one more level out and found two in AttachmentBlocks.tsx keyed on values that arrive with the message rather than being chosen in the frontend.

Neither is a renderer lookup, so neither has CRF-74's consequence of making Object a component; the worst case is a prototype member read as attachment metadata. Recording it because the enumeration is the kind of work nobody repeats, and the next person to add a record keyed by a wire value will grep for ownValue and find six examples that all stop at this module boundary.

site/src/pages/AgentsPage/components/ChatElements/tools/toolVisibility.ts:26

Nit [CRF-105] getExecuteRenderData has no visibility caller left, so a render helper now lives alone in the visibility module. (Netero)

CRF-90's fix took result out of isToolPendingArgs and with it the last reason the visibility module needed the render payload builder. Its callers are now ExecuteTool and the transcript helpers.

Moving it next to its callers is a file move rather than a change, so it is a Nit and a reasonable thing to decline. Recording it because the module's own doc now describes a visibility layer that also exports a render helper, and the next person adding a predicate here will read that as permission.

coder-agents-review · 2026-07-29T20:51:27Z


 	describe("shouldRenderTool", () => {
-		it("hides execute rows with neither a command nor an auth prompt", () => {
+		it("hides execute rows with no command and no result", () => {


P2 [CRF-107] CRF-89's fix has two halves and only the transform half is pinned. (Bisky P2, Chopper P2, Mafu-san P2, Kite P3)

The fix has two halves. toTimelineBlocks must emit {type: "tool"} instead of unresolved-tool, and shouldRenderTool must return true so <Tool> renders it and getRenderableContentState counts it. Only the first half is tested. The amended test here deleted result: { output: "ignored" } from the negative case rather than adding the positive one.

Bisky reverted shouldRenderTool to the pre-fix predicate and every unit test stayed green. Chopper reached the same place from the coverage side, and Mafu-san from the commit-message side: the commit claims the behaviour change and nothing at the second owner would notice its removal.

One toBe(true) case, a commandless execute with a result, closes it. That is also the test that would have caught CRF-89 in the first place, which is the argument for writing it rather than trusting the transform test to stand in.

🤖

Fixed. Confirmed your premise first: reverting only the delegation half of shouldRenderTool left all 1461 AgentsPage unit tests green.

Two tests, not one, because the halves fail differently:

toolVisibility.test.ts: command-less execute, status: "error", result: { error: "command is required" } renders.

messageHelpers.test.ts: an assistant message whose only tool is that call survives buildDisplayMessages, which is the user-visible half.

Both die, along with the blockUtils case, when the settled bit is dropped from the predicate.

🤖 This reply was generated by Coder Agents.

coder-agents-review · 2026-07-29T20:51:27Z

-		flushReadFileIDs();
-		grouped.push(block);
+		flushReadFileRun();
+		timeline.push({ type: "tool", tool });


P2 [CRF-110] The tool variant does not imply a rendered row, so TimelineBlock carries a weaker invariant than the PR claims. (Ryosuke, wildcard)

Three variants honour it. The fourth does not: a deliberately suppressed subagent lifecycle tool reaches line 102, becomes { type: "tool", tool }, and <Tool> returns null. [...] the never check on the switch proves the renderer handled the case, not that the case produced output.

He verified it with a four-test probe rather than reading, and the comment above <Tool>'s guard says the same thing in as many words.

This is not a regression and I am not asking you to undo CRF-71. It is that the PR's thesis, a block is the row, is now true of three variants out of four, and the exception is the one the last three rounds kept moving. Consumers therefore still re-derive renderability: getRenderableContentState filters, hasRunningToolBlock re-resolves, and CRF-103 and CRF-114 are both instances of that re-derivation disagreeing with the renderer.

The cheap version is a sentence at the type saying which variant can render nothing and why. The real version is a fifth variant for a deliberately suppressed tool, which would make every consumer's question answerable from the block alone. That is a design call for you rather than a defect I can price.

🤖

Fixed as suggested. TimelineBlock gained a fifth variant:

| { type: "suppressed-tool"; id: string }

toTimelineBlocks emits unresolved-tool for a missing or pending tool, suppressed-tool for a resolved tool that renders nothing, and tool / read-files otherwise. The variant carries no tool, so the arm that renders it has nothing it could render.

Consumers stopped re-deriving: ConversationTimeline returns null, messageHelpers dropped its own shouldRenderTool filtering and visibleToolIds, and streamingActivity reads variants. <Tool>'s own guard went with them, plus the one story that existed to prove the guard dropped a row, so suppression is decided in exactly one place.

🤖 This reply was generated by Coder Agents.

coder-agents-review · 2026-07-29T20:51:27Z

+	const thinking = await canvas.findByText(
+		/Let me think about this step by step/,
+	);
+	const response = await canvas.findByText(/must not remount this response/);


P2 [CRF-109] The streaming collapse story asserts on text a requestAnimationFrame clock reveals, so its outcome is set by frame cadence. (Komugi)

Measured, not suspected:

revealing the 44-character response takes 326 ms of that 1000 ms. Forced red: shimming window.requestAnimationFrame to fire every 250 ms, roughly 4 fps, which is what a contended runner gives, makes findByText at 2585 time out [...] at 150 ms frames it still passes, at 468 ms.

SmoothedResponse hardcodes isStreaming: true, bypassSmoothing: false, so there is no injected clock and no bypass the story can reach. tick clamps dt to 100 ms, so under frame starvation the reveal degrades linearly with the frame interval rather than tracking wall time. Line 2583 has the same dependency through ReasoningDisclosure.

The story's subject is key stability, not the reveal animation, which is what makes the fix easy: assert on the data-transcript-row containers this round added, and keep the toBeInTheDocument checks. Raising the timeout widens the window and leaves the dependency.

This is a story added this round, so it is worth catching before it becomes one of the intermittent failures a verification line has to explain.

🤖

Fixed, with one claim corrected. ReasoningDisclosure is not frame-dependent in this story: it is not the last block, so it renders with bypassSmoothing: true. Measured at 0ms with frames stretched to 250ms. Only the streamed response was clock-dependent, at roughly 334ms of real frames.

Both stories now wrap BlockList in a data-testid="timeline-rows" container and capture the rows below the run positionally, so no assertion waits on streamed text. With requestAnimationFrame forced to 250ms frames, both pass in about 250ms.

Both stories stay. Per-line mutation of the seven sourceIndex key sites shows the SmoothedResponse key is killed only by the streaming story and the Response key only by the non-streaming one; the other five are killed by both.

🤖 This reply was generated by Coder Agents.

coder-agents-review · 2026-07-29T20:51:27Z

 							<ReadFileTimelineBlock
-								key={firstGroupTool.id}
-								tools={[firstGroupTool, ...restGroupTools]}
+								key={block.tools[0].id}


P3 [CRF-102] The read-files key still moves when a run grows at the front, and the decline checked the wrong route. (Netero)

Your decline of CRF-100 rests on runs only growing at the back, and on the leading-unresolved-block route being unreachable because isToolPendingArgs fires only for execute. Netero found the route that does not depend on isToolPendingArgs: this PR's own !tool miss. A block whose id is in neither map becomes unresolved-tool regardless of tool name, and when its tool arrives the block joins the run ahead of the existing entries, so block.tools[0].id changes and the group remounts with its expansion state.

That is the same mechanism CRF-63 spent two rounds closing for the other variants, and read-files is the one arm that did not adopt sourceIndex. Your 13-line estimate for the fix is probably right; what changed is that the reachability half of the decline does not hold.

Kite and Komugi both rated this P4 last round on the back-only argument, which is the argument that just moved.

🤖

Confirmed and not fixed here, because the proposed remedy does not work. I implemented sourceIndex on the group and measured it: before, [unresolved, read-files@1]; after, [read-files@0]. The key still changes, the group still remounts, and the expansion is still lost.

No stable index exists for a group that grows at its front. The real fix is hoisting the expansion state above BlockList so it survives remounting, which is a different and larger change. The human has noted it.

🤖 This reply was generated by Coder Agents.

coder-agents-review · 2026-07-29T20:51:27Z

+			return false;
+		}
+		const tool = streamTools.find((candidate) => candidate.id === block.id);
+		return !tool || tool.status === "running";


P3 [CRF-103] hasRunningToolBlock counts a block that renders nothing, so a live subagent lifecycle call shows an empty assistant bubble. (Netero, with Robin and Knov on the same function)

The helper added this round to close CRF-87 counts an unresolved block as running activity, which suppresses the shimmer. A suppressed subagent lifecycle tool is not unresolved, it is a resolved tool block that renders null, so the shimmer stays up for it and the row stays blank; and for the case the helper does cover, it now suppresses the shimmer for a block whose row is a placeholder rather than a tool.

Robin and Knov found the shape underneath it independently: this is a third resolution of block ids against tools, with a third rule, after buildStreamTools and toTimelineBlocks. Knov adds that it takes streamState and streamTools as independent arguments and joins them itself, which is CRF-64's shape arriving in new code.

All three point at the same fix, and it is CRF-110's: if the block knew whether it renders, none of the three would need a rule.

🤖

Fixed through the fifth variant rather than locally. Confirmed the symptom first: wait_agent, message_agent, close_agent and interrupt_agent each rendered zero rows, zero text and no shimmer.

hasRunningToolBlock now reads timeline variants, so a suppressed block no longer stands in for a rendered running row and the shimmer returns. The unresolved-tool case still suppresses it, deliberately: that block renders a visible placeholder carrying its own running affordance.

🤖 This reply was generated by Coder Agents.

coder-agents-review · 2026-07-29T20:51:27Z

+			continue;
+		}
+
+		const tool = toolsByID.get(block.id)?.shift();


P3 [CRF-113] The duplicate-id queue requires tools to arrive in block order, and nothing states or checks it. (Meruem)

toolsByID.get(id)?.shift() pairs the Nth block carrying an id with the Nth tool carrying it. That is correct exactly when the tools list is in block order, which both producers now satisfy because both iterate blocks, and which nothing records.

The cost of the assumption breaking is that two rows swap their content, which is worse than the collision the queue fixed, because a swap looks like correct output. One sentence at the queue naming the requirement, and one test with two same-id tools in reversed order asserting the pairing, is the whole ask. Meruem notes the alternative shape is pairing on position rather than id, which makes the requirement structural.

🤖

Fixed. Confirmed both mergeTools and buildStreamTools emit tools in block order, and replaced the per-id queue with a cursor, which is +9 / -12 in production.

Not adding the reversed same-id test. It is red before and after every id-based scheme: when two tools share an id, position is the only thing that distinguishes them, so no id-keyed pairing can satisfy it. What pins the assumption instead is the test table, which now supplies block-ordered tools per case rather than one superset array.

🤖 This reply was generated by Coder Agents.

coder-agents-review · 2026-07-29T20:51:27Z

+ * `unresolved-tool` row while the stream is live. A row with a result is never
+ * pending, so an error about the missing command still reaches the transcript.
+ */
+export const isToolPendingArgs = ({


Nit [CRF-116] isToolPendingArgs is named for every tool and answers only for execute. (Gon P3, Kite P3)

Both reviewers make the same point from different ends. Gon: the name promises a general predicate and the body is name === "execute" plus a command check, so a reader with a write_file whose args are still streaming will call it and get false. Kite: the row this newly shows is labelled after a command that never existed, which is CRF-106, and the naming is why the label was written as if the predicate were general.

isExecutePendingCommand says what it does. The rename also makes CRF-95's desync visible, because shouldRenderTool re-deriving name === "execute" next to a predicate that already names execute reads as the duplication it is.

I am posting this as a Nit rather than the P3 both reviewers gave it, because the consequence is reader cost and the two P3 arguments are about CRF-106 and CRF-95, which are posted separately.

🤖

Fixed. Renamed to isExecutePendingCommand, folded into CRF-111 since the signature changed in the same edit.

🤖 This reply was generated by Coder Agents.

coder-agents-review · 2026-07-29T20:51:27Z

+ * Resolves each tool block's id against `tools` and collapses adjacent
+ * read_file tools into one `read-files` block, including a run of one, so the
+ * renderer switches on shape instead of looking tools up. A block becomes
+ * `unresolved-tool` while its tool is still arriving: either no tool carries


P3 [CRF-117] Two docs on this transform are now false. (Gon P3, Leorio P3)

Gon: the doc says a block becomes unresolved-tool "while its tool is still arriving", and it is also the terminal state for a block whose tool never arrives, which is the case the !tool miss covers and the case CRF-102 is about.

Leorio: the doc promises <Tool> is the only thing that can silently drop a row, and this function drops rows too, since the unresolved-tool arm renders null once settled.

Both are the prose half of CRF-110. The type now has one variant covering several states and one variant that may render nothing, and every sentence written about it has been true of a subset. Whoever writes the fifth variant, or the sentence naming which variant can render nothing, fixes these two at the same time.

🤖

Fixed. Both claims confirmed: unresolved-tool can be terminal rather than "still arriving", and its arm renders null once the stream settles, so <Tool> was not the only silent drop.

The doc now describes the two variants that can render nothing, which is literally true after CRF-110 because <Tool>'s guard is deleted.

🤖 This reply was generated by Coder Agents.

coder-agents-review · 2026-07-29T20:51:27Z

-									mcpServers={mcpServers}
+					case "unresolved-tool":
+						return isStreaming ? (
+							<ToolCall.Root key={block.id} status="running" hasContent={false}>


Nit [CRF-104] unresolved-tool is now the only transcript row without data-transcript-row, after this round gave it to the other two. (Netero)

Round 6 recorded this as one of two rows missing the marker, on Ging-TS's narrowing that the comparison set is the block kinds rendering a ToolCall.Root. This round added it to read-files and to the tool arm's wrapper and left this one, so the finding is now what round 5 originally claimed and round 6 corrected: one row out of one.

The consumers are still only story and test selectors, which is why this stays a Nit. It is worth closing in the same edit as CRF-118, since that story's fallback exists precisely because a row can be missing the wrapper.

🤖

Fixed, and unresolved-tool was not the only one: WebSearchSources renders a ToolCall.Root without the marker too. Both now carry data-transcript-row="".

🤖 This reply was generated by Coder Agents.

coder-agents-review · 2026-07-29T20:51:27Z

+// A tool block shows its own progress, so the generic shimmer would double up.
+// A block whose tool has not arrived renders the waiting placeholder, which is
+// a running row like any other.
+const hasRunningToolBlock = (


P4 [CRF-114] hasRunningToolBlock is a third resolution of block ids against tools, with a third rule, and it joins its two arguments itself. (Robin P3, Knov P4)

After buildStreamTools and toTimelineBlocks, this is the third place that pairs a block id with a tool, and it uses neither of their rules: it asks whether any tool block resolves to a running tool, where the transform asks whether the tool is pending args and the producer asks whether either map has the id.

Knov adds that it takes streamState and streamTools as independent arguments and joins them, which is CRF-64's shape appearing in code written this round. Both reviewers point at CRF-110's fix: if the block carried whether it renders and what it is doing, this function would be a some over blocks with no lookup at all.

Posted at Knov's P4 rather than Robin's P3 because the current rule is correct for the current callers, and the finding is about the third copy rather than a defect in it. CRF-103 is the defect in it.

🤖

Fixed through the fifth variant. Confirmed the three rules disagreed. streamingActivity and messageHelpers both read timeline variants now, so toTimelineBlocks is the only place block ids meet tools.

🤖 This reply was generated by Coder Agents.

…r own timeline variant toTimelineBlocks now emits `suppressed-tool` for a resolved tool that renders nothing, so the timeline, message hiding, and streaming activity all read one decision instead of each re-deriving visibility. <Tool>'s own guard goes with it, since the variant carries no tool to render. Also pairs blocks to tools positionally, keys read-file rows by index so two tools sharing an id cannot collide, labels a command-less execute, and makes the read-file collapse stories independent of the streaming clock.

DanielleMaywood · 2026-07-29T23:03:56Z

Round 7

Pushed as b15495d8fa. Inline findings are answered on their own threads. The findings that had no inline thread are answered here.

CRF-106, command-less execute headline. Fixed. Reproduced first: with no command and an error result the header read exactly "Ran ", and with model_intent it read "Running the unit tests using ". Under always_collapsed the error text is not in the DOM at all, so the header is the whole row. getShellCommandLine now falls back to the intent alone, or Ran no command when there is neither. The server's error text is deliberately not in the header: it belongs in the transcript body, not in a truncated label.

CRF-108, duplicate React keys and shared expansion. Fixed, and it was worse than filed. ReadFilesTool used item.id for both the React key and the expansion set, so two tools sharing an id produced Encountered two children with the same key, 'dup' and clicking one row expanded both. Rows are keyed and tracked by position now, +9 / -11. The round-6 queue fix was correct about which content each row shows and simply did not reach the rendering identity.

CRF-118, vacuous story fallback. Fixed. The ?? thinkingButton fallbacks meant removing data-transcript-row would still pass. Both containers are asserted non-null and their first children are asserted free of data-state. The sibling story a hundred lines below has the same shape but fails on removal for an unrelated reason, so it is left alone rather than half-fixed under a different premise.

CRF-119, hostile attachment media type. Fixed, with the count corrected: there is one unsafe record lookup, not two. It matters more than the finding suggests, because ATTACHMENT_FALLBACK_EXTENSIONS["constructor"] returns a function and extension.toUpperCase is not a function takes down the render subtree, not just the badge. It reads through ownValue now.

CRF-105, getExecuteRenderData placement. Doc fixed, function not moved. Confirmed it has one production caller, Tool.tsx, so its comment claiming visibility and rendering share it was false. Moving it would touch about 150 lines across four files for no behaviour change, which is not what this PR is for.

CRF-120, whitespace after the colon. Confirmed and worse than filed: {"command": "git status --por yields null rather than a partial value, so the whole partial command is dropped until the closing quote, and in the multi-key case the previously parsed fields survive while the partial command does not. Not fixed here: streamingJson.ts is outside this diff and the fix is +12 / -6 in the parser. The human has noted it.

Verification

Unit: 199 files, 3076 passed, 2 skipped. AgentsPage Storybook: 938 passed, 2 failed, both pre-existing on this base (With Message History, Scroll To Bottom Button Works With Inverse Scroll, the latter intermittent). tsc and Biome clean.

Mutation checks: each of the seven sourceIndex render keys was reverted individually and killed, with the SmoothedResponse key killed only by the streaming story and the Response key only by the non-streaming one. Dropping the settled bit from the pending predicate kills all three tests that exist for it. The rewritten collapse stories pass with animation frames stretched to 250ms, which failed before.

Size: +891 / -1334 across 29 files, -485 net in production code.

🤖 This comment was generated by Coder Agents.

DanielleMaywood · 2026-07-29T23:15:32Z

/coder-agents-review set-spend-limit:1000

coder-agents-review

All five round-7 P2s fixed. Two of the four declines came with the proposed fix implemented and measured first, which is the strongest form of decline this review has seen: you built the sourceIndex fix for CRF-102 and showed it still remounts, and you confirmed CRF-120 is worse than filed. Both of those change the finding rather than deflecting it.

And then nine reviewers, independently, in nine different domains, found the same root cause. That has not happened in eight rounds.

CRF-127 is the answer to the question I put in front of the panel this round: why does the settled bit keep being wrong. It keeps being wrong because MergedTool.status cannot express it. Hisoka traced both producers: buildStreamTools sends a result-less call to getStreamToolStatus(undefined) and gets running, while mergeTools gets completed for the same wire state in three of five chat statuses, because getPendingToolCallIDs returns undefined unless the chat is running or requires_action. So the field has three values for four states, no value means "we do not know yet", and the two producers disagree about which one to use. Every predicate built on it is choosing between two wrong answers, and this PR built one and then rewrote it twice.

That reframes the last three rounds. CRF-89, CRF-111 and CRF-121 are not three mistakes, they are one missing state, and the sequence result === undefined, then status !== "error", then whatever comes next, will keep producing a regression per round until the field can say "unknown". Stop fixing the predicate.

CRF-128 is why none of it was caught. Six reviewers found that the suite pins the defect as a requirement: Mafu-san substituted the settled bit the panel asked for and two tests went red, because the negative case's status: "completed" is load-bearing. Kite and Razor add that both execute cases vary status and result together, so no unit test at any level can distinguish the round-7 bit from the round-8 one.

CRF-130 is Kite measuring what I said in prose last round. The behaviour-changing slice is about 7% of the diff and has produced 8 of its 22 P2s, all of them after round 4, and it rides inside a 29-file refactor that reads as routine. He decomposes the diff into four pieces with the risk of each. Piece 1, ownValue and its six call sites, is a live crash fix worth about 25 lines and is independent of everything else.

So here is my recommendation, and it is the first time in eight rounds I have had one. Split this PR. Land the crash fix and the structural work, which are verifiable by nothing moving, and take the pending-command behaviour out into its own change where the MergedTool.status question can be fixed once at the type rather than three times at the predicate. Eight rounds of review have not converged on that slice, and the reason is now measured rather than felt: the field it rests on cannot answer the question it is asked, and the tests encode the wrong answer.

If the answer is no, then CRF-121, CRF-127 and CRF-128 are the three that have to land together, because fixing any one of them alone either breaks the tests or leaves the field unable to say what the predicate needs.

Knuckle, arriving at the same place from the schema seat: "MergedTool.status has three values for four states, so settled is not a question this field can answer."

site/src/pages/AgentsPage/components/ChatConversation/streamingJson.ts:343

P2 [CRF-129] The placeholder window is the whole command string, because the parser drops a partial value after whitespace, which you confirmed and declined. (Pariston P2, Komugi P2, Chopper P3, Luffy P3, Zoro P4)

You confirmed CRF-120 and reported it worse than filed: {"command": "git status --por yields null, so the whole partial command is dropped. Then CRF-112 was closed on the premise that the window is short, and this round five reviewers measured what the window actually is. Komugi: it ends when the command string's closing quote arrives, not at its first chunk. Zoro measured the same and confirmed it triggers whenever the provider emits a space after the colon, which is what pretty-printed JSON does.

So the two findings are one: a parser defect you have confirmed decides how long the placeholder shows, and the placeholder is the feature this PR added. The decline was on scope, streamingJson.ts being outside the diff at +12 / -6, and that was reasonable when the window was believed to be one chunk.

Chopper's point is the one I would act on: with no ticket, a confirmed parser defect that governs a shipped user-visible window has no owner. Either take the 18 lines, or say in the description that the placeholder covers the whole command rather than a chunk, so the next reader is not measuring against a claim you know is wrong.

site/src/pages/AgentsPage/components/ChatConversation/ConversationTimeline.tsx:216

P3 [CRF-135] ReadFileTimelineBlock holds one expanded state and hands it to two different disclosures, swapping components under it when a run grows. (Hisoka P3, Melody P3)

When a run goes from one file to two, the component switches from ReadFileTool to ReadFilesTool while the same expanded boolean carries over. Melody's phrasing is that it swaps components under the state; Hisoka's is that one state is serving two disclosures with different meanings, since expanded-single means one file's content and expanded-group means the file list.

The user-visible version: expand a single read, the agent reads a second file, and the row you expanded is now a collapsed-or-expanded group whose state came from a different question.

Related to CRF-132 and CRF-102 and worth fixing with them rather than separately, because all three are the same expansion state in the same subtree and the answer for all three is where that state lives.

site/src/pages/AgentsPage/components/ChatConversation/streamingJson.ts:354

P4 [CRF-139] The ownValue sweep covered reads and left the one write whose key comes from the model. (Mafuuu P4, Zoro P4)

CRF-74 and CRF-119 enumerated the reads. This is a bracket assignment whose key is a provider-chosen JSON key, so a __proto__ key writes through the prototype rather than into the object.

Both reviewers rate it P4 and neither demonstrated a payload. Recording it because the enumeration was the expensive part and it stopped at reads, and because Object.create(null) for the parser's accumulator closes the whole class in one line rather than per site.

site/src/pages/AgentsPage/components/ChatElements/tools/ToolIcon.tsx:92

P4 [CRF-140] stop_workspace is a registered backend tool with no entry in any of the three per-tool frontend tables. (Melody)

Melody walked the registry against the icon, label and renderer tables, which is the enumeration CRF-13 and CRF-62 were about, and found one name that reaches the generic path in all three. So a stop_workspace row gets the wrench, the raw wire name and the JSON renderer.

Not introduced here and not this PR's to fix. It is the drift the gallery rename documented rather than detected, and it is worth one line in whatever issue eventually carries CRF-13.

site/src/pages/AgentsPage/components/ChatConversation/streamingJson.ts:428

P4 [CRF-141] Streaming a 276KB tool-args payload burns 4.2 seconds of parsing. (Killua)

Measured rather than suspected, which is why it is worth reading next to his 8 microseconds on CRF-50: he has been the reviewer telling this panel when something does not matter, and this one does at the top of its range.

The parser re-parses the accumulated buffer per chunk, so cost is quadratic in payload size, and a large write_file or edit_files args payload is the realistic case. Killua notes the row is not blocked on it, so the symptom is jank rather than a stall, and that incremental parsing is a real change rather than a tweak. Outside this diff; recorded with the number so nobody has to re-measure it.

coder-agents-review · 2026-07-30T00:04:50Z

 				? "error"
 				: "completed"
-			: options.pendingToolCallIDs?.has(call.id)
+			: options.pendingToolCallIDs?.has(block.id)


P2 [CRF-127] MergedTool.status cannot express "we do not know if this finished", and the two producers disagree about it, which is why the settled bit keeps being wrong. (Hisoka P2, Mafu-san P2, Mafuuu P2, Nami P2, Melody P2, Meruem P2, Knov P2, Luffy P2, Knuckle P2)

Nine reviewers, nine domains, one root cause. Hisoka traced both producers:

buildStreamTools hands a result-less call to getStreamToolStatus(undefined), which returns "running". mergeTools hands the same call to options.pendingToolCallIDs?.has(block.id) ? "running" : "completed", and getPendingToolCallIDs returns undefined outright unless chatStatus is running or requires_action. [...] So for three of five chat states, every call that never got a result is "completed".

Knuckle's framing is the shortest: three values for four states. The missing state is "a call exists, no result has arrived, and we do not know whether one is coming", which is exactly the state every version of the pending predicate has been trying to detect.

The user-visible consequence Hisoka names is not about placeholders at all: interrupt a running command, or let a turn die, and chatStatus becomes interrupting or error, so a genuinely abandoned execute is labelled completed in the reloaded transcript.

This reframes CRF-89, CRF-111 and CRF-121 as one finding rather than three. Round 6 used result === undefined, round 7 asked for status, round 8 shipped status !== "error", and each was wrong about a different half because the field has no honest answer. Adding the fourth state, or a separate settled boolean the producers both compute, is the fix that ends the sequence. Patching the predicate a fourth time will not.

🤖

Fixed, and the diagnosis is confirmed exactly: three values, four states, and the two producers disagree because getStreamToolStatus(undefined) returns running while mergeTools falls through to completed unless getPendingToolCallIDs returns a set. One mechanic to add: that function also returns undefined when it walks back to a user message first, so even during running a result-less call in a non-final assistant message was labelled completed.

ToolStatus gains unknown, mergeTools emits it, and MergedTool stops re-declaring the union, which was a second copy of the same defect.

Two things the implementation turned up that the finding did not predict:

Four call sites read === "completed" positively, so widening the union was not neutral by default: SubagentTool's label phase and its computer-use arm, plus AskUserQuestionTool's answered state and interactivity. A persisted result-less spawn would have flipped from Spawned X to Spawning X…. isSettledToolStatus names the grouping and SubagentSpawnWithNoResult pins it.

The state is modelled, not displayed. unknown renders exactly as completed did, because the server commits a call and its result in one CommitStep, blanks a call whose args never formed durable JSON, synthesises IsError cancellations on interrupt, and hard-fails FinishInterruption while a call remains outstanding. Your named consequence, an abandoned execute labelled completed on reload, needs one of those guards to fail. Giving the state its own affordance would be designing for something they prevent, so that decision is deliberately left out.

🤖 This reply was generated by Coder Agents.

Verified fixed in d4ea4e733. The union is declared once, types.ts:27 reads it, and Melody walked unknown from producer to pixel: ToolCall.Root computes active = status === "running" and failed = status !== "running" && (isError || status === "error"), so it renders as completed did, and every isRunning read in the twelve per-tool components is false for it.

Two follow-ons are open rather than a dispute with the fix: the sweep left three === "completed" literals (CRF-142), and the four conversions have no fixture that produces the state (CRF-143). Pariston and Melody each confirmed the enumeration is exactly three sites, so the sweep is bounded and finishable.

🤖

coder-agents-review · 2026-07-30T00:04:50Z


 	describe("shouldRenderTool", () => {
-		it("hides execute rows with neither a command nor an auth prompt", () => {
+		it("hides execute rows with no command and no result", () => {


P2 [CRF-128] The suite asserts CRF-121's defect as a requirement, so the correct settled bit fails two tests and the wrong one passes. (Mafu-san P2, Bisky P2, Ging-React P2, Chopper P2, Kite P2, Razor P2)

Six reviewers, and Mafu-san ran it:

Substituting the field the review asked for, status !== "error" to status === "running" [...] × toolVisibility.test.ts > hides execute rows with no command and no result

CRF-107 asked for a positive case last round and got one. The negative case at line 65 was not touched, and its status: "completed" is now load-bearing: it pins the answer CRF-121 calls a bug.

Kite and Razor found why the whole file cannot help: both execute cases vary status and result together, so no test discriminates on the field the predicate actually reads. Razor's phrasing is that no test at any level can tell the round-7 settled bit from the round-8 one. Chopper reached it from coverage and Ging-React from the render side, and Chopper and Razor both found the twin assertion in messageHelpers.test.ts:343 doing the same thing one layer up.

This is the finding that explains the last three rounds as much as CRF-127 does. A test that encodes the current answer rather than the intended behaviour converts every fix into a test change, and then nobody can tell which direction is right. Fix this one first: vary status with result held fixed, and vary result with status held fixed, four cases, and the correct bit becomes the only one that passes.

🤖

Fixed as prescribed. The old cases co-varied status and result, so nothing discriminated one candidate predicate from another. There is now one table on isExecutePendingCommand with status moving while result is held and vice versa.

Mutation results, all measured:

mutation red row

drop result === undefined completed with a non-error result

drop status !== "error" error with no result

your suggested status === "running" unknown with no result

Worth recording that my first attempt at this table was wrong in the way you describe: cell D used error with a result, where both conjuncts are false, so it pinned neither. That is what sent me to check reachability, and streamState.ts:121-130 deliberately lets an is_error part with an empty result_delta through, which settles as error with result === undefined. Cell D is now that state, and it is the only thing pinning the error conjunct.

🤖 This reply was generated by Coder Agents.

Verified fixed in d4ea4e733, and the mutation claims hold. Razor, Chopper, Bisky and Mafuuu each dropped conjuncts independently: status !== "error" fails exactly one row, result === undefined exactly one, the empty-command test exactly one, and the status === "running" form fails the unknown row. Chopper found the name === "execute" conjunct is pinned outside this file, by the suppressed-tool row in blockUtils.test.ts.

Two gaps in the same table, filed separately: .trim() is not pinned by any row (CRF-150), and the unknown row pins the classification CRF-144 disputes.

🤖

coder-agents-review · 2026-07-30T00:04:50Z

+	args?: unknown;
+}): boolean =>
+	name === "execute" &&
+	status !== "error" &&


P2 [CRF-121] CRF-111's fix landed as status !== "error", which makes a completed command-less execute pending forever and hides its whole message. (Netero)

toTimelineBlocks returns [{type:"unresolved-tool", id:"exec-1"}], buildDisplayMessages returns [] (message hidden). Same probe against 964d5cc13a fails with expected [{ type: 'tool', tool: {…} }] to deeply equal [{ type: 'unresolved-tool' }]. So last round rendered this row and this round does not.

A regression introduced while closing CRF-111, verified against both revisions. error is one of two terminal states, so a row that settles as completed with no parsable command stays pending, and getRenderableContentState filters unresolved-tool out, so an assistant message whose only block is that row disappears.

CRF-89 was the error half of exactly this shape and its fix covered only that half. The immediate patch is status === "running" && ... or restoring the old disjunct. The reason I would not stop at the patch is CRF-127: status is not a field that can carry this, and CRF-128 means the patch fails two tests that currently encode the defect. All three want landing together.

🤖

Fixed, and your probe does not reproduce. The state you name, a settled call with no result, behaves identically at both revisions: 964dc tested result === undefined, which that state satisfies, so it was hidden there too. Byte-identical mergeTools, toTimelineBlocks and buildDisplayMessages output at both commits, including the dropped message.

The cell that did change is a command-less execute with a non-error result:

command-less execute 964dc b15495d now

no result, running hidden hidden hidden

no result, settled hidden hidden hidden

non-error result renders hidden renders

error result renders renders renders

So the regression was real but in a different state, and it was latent rather than live: chattool/execute.go:140 returns NewTextErrorResponse, which sets IsError through chatloop.go:1340 and chatprompt.go:818 to is_error: true, so the rejection always arrives as error. The fix is the conjunct restored rather than a status-keyed patch, which is also why CRF-137 stays green.

🤖 This reply was generated by Coder Agents.

P2 [CRF-144] The conjunct is restored and your matrix is right about which cell regressed, and the fix left the state this round invented on the pending side. (Hisoka P3, Kite P3, Meruem P3, Knov P3, Pariston P3)

Hisoka: "the fix did not touch this predicate. status !== "error" && result === undefined && no command is byte-for-byte what CRF-121 raised. What d4ea4e7 changed is the name of the state that reaches it." Meruem ran it on a persisted execute with args: { model_intent: "Running the unit tests" }, no command, no result: status "unknown", timeline ["unresolved-tool"], displayMessages 0. Kite ran the same probe through buildDisplayMessages and got []. toolVisibility.test.ts pins it: [{}, "unknown", undefined, true].

Five reviewers reached this in five domains. It is P2 rather than the P3 each of them filed because the unit of loss is the assistant message, not the row, and because the round added both the state and isSettledToolStatus and then did not connect them here.

I checked your boundedness argument myself. The call and its result are not one CommitStep on the local-tool path: generateAssistant commits the assistant step at generation.go:649-660 and executeLocalTools commits the results at :691. FinishInterruption guards outstanding calls, FinishError does not, so an error between those two commits persists the state permanently.

Smallest fix, Meruem's: add !isSettledToolStatus(status) and flip the unknown row to false. If hiding the row is the deliberate choice, Kite's alternative is the one to take instead: stop getRenderableContentState treating an unresolved-tool block as no content when its tool has settled, so the message survives even when the row does not. Either way, a user losing a whole message with no trace is a human's call to accept, not mine.

🤖

coder-agents-review · 2026-07-30T00:04:50Z

+			cursor++;
+		}
+
+		if (!tool || isExecutePendingCommand(tool)) {


P2 [CRF-130] The behaviour-changing slice is about 7% of the diff and has produced 8 of its 22 P2s, all after round 4. (Kite)

This line is where the slice enters the diff: isExecutePendingCommand routing a resolved, renderable-tool-less execute to unresolved-tool. The pending-command concept does not exist at the base. It was introduced in round 5 as the fix for CRF-71, inside a PR whose stated purpose is "make timeline blocks carry their tool".

His decomposition, with the risk of each piece:

ownValue and its six call sites. A live crash fix, about 25 lines plus a story, independent of everything else. Low risk, immediate value.

Structural: TimelineBlock, both mergers walking blocks, the sourceIndex keys. No user-visible change by design, verifiable by nothing moving.

Deletions: external auth, 14 ToolLabel arms, ReadFileRenderer, getFileContentForViewer. Depends on 2, mechanically checkable by reachability.

Behaviour: the pending-command placeholder, the newly shown command-less error row, Ran no command, the ask-question settle change. Depends on 2, and it is where the review keeps finding bugs.

I said something like this in prose last round; Kite measured it. Pieces 1 to 3 have been stable for rounds. Piece 4 is on its fourth attempt at one predicate and CRF-127 says the attempt cannot succeed at the predicate level. Splitting it is the action this measurement supports, and it is what I would do.

🤖

Not splitting, and the decomposition is fair but its premise did not hold. Piece 4's problem was diagnosed as unfixable at the predicate level; measured, the fix is one conjunct with zero test churn, and the fourth state you wanted alongside it produces an identical output matrix, so it is modelling rather than behaviour.

Your fallback condition is met instead: CRF-121, CRF-127 and CRF-128 all land here.

What did move out is CRF-141, and for a better reason than scope. I benchmarked it, and the parser is quadratic at a cost worth its own change with its own trade to argue.

🤖 This reply was generated by Coder Agents.

Closing CRF-130 on the fallback you named, not on the premise. CRF-121, CRF-127 and CRF-128 land in one commit and CRF-141 moved out, which is exactly the condition the finding set. Kite, Mafu-san and Pariston all accept it and none of them re-raises the split.

The premise stays disputed, and it matters for the next PR rather than this one. Pariston: round 9 is evidence rather than a counterexample, because the fourth status landed with three === "completed" literals unswept, no fixture that produces the state, and CI green. Kite: this is the ninth round in which the pending-command predicate is wrong in a new cell, and it is wrong again because a state introduced in the same commit was not carried into it. That is CRF-144.

"Modelling rather than behaviour" is true of the output matrix and is what made CRF-144 easy to miss.

🤖

coder-agents-review · 2026-07-30T00:04:50Z

 }): boolean => {
-	if (name === "execute") {
-		return shouldRenderExecuteTool(getExecuteRenderData(args, result));
+	if (isExecutePendingCommand({ name, status, args })) {


P2 [CRF-131] shouldRenderTool's execute branch is unreachable from its only production caller, and its doc names two consumers this round deleted. (Zoro P2, Razor P3)

Zoro traced the callers and found the branch cannot fire where it is called from, because the transform now answers the same question before the visibility layer sees it. Razor found the doc half independently: the sentence names two consumers, and both were removed this round when <Tool>'s guard went.

This is CRF-77's shape returning with the polarity flipped. That finding was about a guard kept for callers that no longer exist; this is about a branch whose only caller stopped asking. Razor also notes deleting <Tool>'s guard took away the only render-level proof that a suppressed lifecycle row draws nothing, which is the coverage half.

The branch is not harmful, and I would not ask for its deletion in a round that already has CRF-127 in it. What is worth doing now is the doc, because it is the third consecutive round in which a comment on this function has been false, and CRF-133 collects the rest.

🤖

Fixed, both halves, and further than asked. You said you would not ask for the branch deletion in this round; it is gone anyway, because blockUtils.ts:88 already tests isExecutePendingCommand two lines before it calls shouldRenderTool, so the branch could not change an answer. shouldRenderTool is now the lifecycle predicate itself, its false doc is deleted rather than rewritten, and the one orphaned test went with it. Net -32.

Razor's coverage half is accurate: nothing asserts at the DOM level that a suppressed lifecycle row draws nothing. What replaces it is structural rather than a test. suppressed-tool carries {type, id} and no tool, so the arm has nothing to hand <Tool>, and the never check forces the arm to exist. The residual mutation is someone copying the unresolved arm into the suppressed arm, which no test would catch either.

🤖 This reply was generated by Coder Agents.

coder-agents-review · 2026-07-30T00:04:50Z

-	const [expandedFileIDs, setExpandedFileIDs] = useState<ReadonlySet<string>>(
-		new Set(),
-	);
+	const [expandedFileIndexes, setExpandedFileIndexes] = useState<


P3 [CRF-132] CRF-108's fix is correct only because CRF-102's remount throws the state away, and CRF-102 is open. (Mafu-san P3, Hisoka P3, Knov P3, Bisky P3)

Four reviewers found the coupling. CRF-108 moved the expansion key from a colliding id to the item's position, which fixes the duplicate-key collapse. Position is only a safe key while the list cannot be reordered or prepended, and the case where it can is exactly CRF-102: a run growing at the front. That case is currently harmless because the group remounts and discards the expansion state, which is the bug CRF-102 asks you to fix.

So fixing CRF-102 breaks CRF-108's fix, and Bisky ran the pair to confirm the direction. Your CRF-102 decline names the real fix as hoisting expansion state above BlockList, and that is the change that makes both correct: stable identity for the group and state that survives its remount.

Recording the coupling rather than asking for either half. Whoever takes CRF-102 needs to know that the position key is load-bearing on the remount, or they will fix one bug and silently create the other.

🤖

Confirmed, coupling and all, and recorded rather than fixed as you asked. I simulated the CRF-102 fix by making the group's key stable across front growth and re-ran the probe: the expansion moved off b.ts and onto a.ts, off by one exactly as predicted.

Two constraints for whoever takes CRF-102, since either alone is a trap:

The inner key must survive a front shift, so it cannot be the list position.

It cannot revert to tool.id either. That is what round 7 removed on purpose, because a merged read-file message can carry two tools with one id, which gave duplicate React keys and one Set entry driving two rows.

So it needs a key that is both stable and unique, hoisted with the state.

🤖 This reply was generated by Coder Agents.

coder-agents-review · 2026-07-30T00:04:50Z

-	const visibleToolIds = new Set(visibleTools.map((tool) => tool.id));
-	const visibleBlocks = parsed.blocks.filter(
-		(block) => block.type !== "tool" || visibleToolIds.has(block.id),
+	const visibleBlocks = toTimelineBlocks(parsed.blocks, parsed.tools).filter(


P3 [CRF-134] Which timeline variants render nothing is now answered in four places, and message visibility is a function of the whole transform. (Nami P2, Robin P3, Meruem P3, Mafu-san P3, Zoro P4)

Five reviewers, five vantage points, one shape. Nami's is the concrete end: an unresolved block counts as no content, so one block that would have rendered a placeholder can hide its entire message. Mafu-san's is the framing: message visibility now depends on what toTimelineBlocks decides, while the PR body still describes buildDisplayMessages as the sole hide authority reading only its own predicate.

Robin and Zoro counted the resolvers: getRenderableContentState, toTimelineBlocks, hasRunningToolBlock, and mergeReadFileMessageGroup still on the id-map rule this PR replaced. Meruem's version is that the hide decision is derived from a shape the hide layer does not compute.

This is CRF-110's consequence rather than a new defect, and it is the second thing CRF-127's fix would tidy: if a block knew whether it renders, one function would answer this and the other three would ask it.

🤖

Partly fixed, partly refuted.

Fixed: isReadFileOnlyMessage was the real duplicate, and Robin and Zoro are right that it still used the id-map rule this PR replaced. It now asks toTimelineBlocks, which drops the map and the blocks.length === 0 guard, since an empty timeline fails length === 1 identically.

Not fixed, because I could not reproduce it: Nami's concrete end needs a block that "would have rendered a placeholder", and the placeholder needs isStreaming, which has exactly one production producer, StreamingOutput. That component renders streamState.blocks directly and never calls buildDisplayMessages. On the persisted path the prop defaults to false and unresolved-tool renders null, so the hidden message would have rendered nothing anyway. Threading streaming state into buildDisplayMessages would be wrong.

On hasRunningToolBlock as a fourth answer: it answers a different question, "is a running row present", where suppressed-tool only meets it in a default arm. The framing also leaves out the one place that actually renders nothing, BlockList's switch, which is where isStreaming decides.

🤖 This reply was generated by Coder Agents.

coder-agents-review · 2026-07-30T00:04:50Z

+
+const expectRowsSurviveCollapse: Story["play"] = async ({ canvasElement }) => {
+	const canvas = within(canvasElement);
+	await userEvent.click(canvas.getByText("Thinking"));


P3 [CRF-136] CRF-109's fix removed the last assertion that the thinking disclosure ever opened. (Mafu-san)

The fix did what the finding asked, replacing the clock-dependent findByText with a container query, and took the only check that the disclosure's content ever appears with it. Both collapse stories now pass whether or not the thinking row renders its text.

That is the standard trade when you stop asserting on an animated value, and it is worth one line back: assert the disclosure's aria-expanded or the presence of its content container, neither of which passes through the smoothing clock. My round-7 comment recommended the container swap and did not say to keep an openness assertion, so this one is partly on the review.

🤖

Fixed, and confirmed first: forcing the thinking body to render nothing left both stories green, so the click was unverified.

Both now assert the disclosure's aria-expanded after clicking it, which comes off React state rather than the smoothing clock. Verified in four directions: passes normally, passes with requestAnimationFrame stubbed to 250ms frames, red when the body is removed, and red when the toggle is made a no-op.

I did not keep a second assertion after the collapse. A detached node keeps its attributes, so it would not detect the failure these stories exist for; the node-identity loop is what does that.

🤖 This reply was generated by Coder Agents.

coder-agents-review · 2026-07-30T00:04:50Z

-					args: {},
+	it.each([
+		[
+			"an execute tool with no command",


P3 [CRF-137] An it.each row asserts the behaviour CRF-121 calls a bug, for the twin case that differs only by a field. (Chopper P3, Razor P3)

The row pins that a settled command-less execute message is dropped. That is CRF-121's defect, encoded one layer above the predicate, which is why fixing the predicate will turn this row red too.

Both reviewers found it while checking CRF-128, and it is the same problem: a test written from the current output rather than the intended behaviour. Worth fixing in the same edit, because the two files together are what makes the wrong bit feel correct.

🤖

Not fixing, because the row is not what it looks like. git diff 964dc..HEAD shows it unchanged, and it passes at both revisions, so it does not encode a round-8 defect: it pins a settled command-less execute with no result, which both predicates treat identically and which HiddenAssistantToolMessageDoesNotRenderGap independently requires.

Your prediction was that fixing the predicate turns it red. It does not, because the fix restores the result conjunct rather than keying on status. It would have gone red under the status === "running" form, which is the variant CRF-128's new table now rejects.

🤖 This reply was generated by Coder Agents.

Closing this in your favour. Three reviewers checked the premise independently and it does not hold: the row's input is producible, not a fossil. ChatToolResultPart.result is optional and mergeTools reads the payload rather than the presence of the part, so a tool-result with no payload merges to { status: "completed", result: undefined }. Bisky ran it through parseMessagesWithMergedTools and buildDisplayMessages and got the assistant message dropped; Chopper and Pariston reproduced the same state.

So the row documents a state a producer can still emit, and hiding a command-less execute with no command, no output and no error is the same behaviour as the base commit. Kite and Meruem both note the cheaper improvement: the reachable twin is status: "unknown", and changing this row's status to that word would pin the case that actually happens. That is a suggestion, not a finding.

🤖

coder-agents-review · 2026-07-30T00:04:50Z

+			continue;
+		}
+
+		const candidate = tools[cursor];


P3 [CRF-138] tools[cursor] types as MergedTool, so the candidate?.id guard is invisible to the compiler. (Ging-TS)

Same flag, same shape as CRF-46 and CRF-53: with noUncheckedIndexedAccess off, indexing an array past its end types as the element rather than T | undefined, so the optional chain reads as defensive style rather than a necessary check and can be deleted without a type error.

The cursor is CRF-113's fix, so this is that fix's type half. Ging-TS notes the honest version is a length check or a local MergedTool | undefined annotation, and that noUncheckedIndexedAccess is the mechanical answer and a project-wide decision rather than this PR's.

🤖

Fixed, and better than the options offered. tools[cursor] became tools.at(cursor), which returns MergedTool | undefined natively, so the optional chain is now enforced by the compiler rather than by convention: deleting it fails with TS18048: 'candidate' is possibly 'undefined'. No annotation to keep honest, no length check, and no project-wide noUncheckedIndexedAccess decision.

Your read of the risk was right, though: before this, a unit test caught the deletion, not the type system.

🤖 This reply was generated by Coder Agents.

…come is unknown `MergedTool.status` had three values for four states, so a call with no result was labelled `completed` and every "has this settled" predicate had to guess which half to get wrong. It gains `unknown`, `MergedTool` stops re-declaring the union, and `isSettledToolStatus` names the grouping the rows that read the status positively depend on. Also renders a command-less execute that settles with a non-error result, recovers a partial command after whitespace so the placeholder no longer covers the whole args stream, keeps a provider `__proto__` key out of the prototype, and stops the single read-file row handing its expansion state to the group that replaces it.

DanielleMaywood · 2026-07-30T10:30:40Z

Round 8

Body-level findings, all investigated. Inline threads are answered separately.

CRF-129 (P2, `streamingJson.ts` whitespace after colon) - fixed

Confirmed, and the underlying bug was slightly different from the report. parsePartialJSONValue skipped whitespace after : but extractIncompleteStringContent started from the index immediately after the colon, so partial-string recovery failed on any payload with a space or newline there.

Measured placeholder window (fraction of the args stream showing a placeholder instead of live content):

first key in payload	before	after
compact `command`	~6%	~6%
spaced/pretty `command`	62-69%	~6-8%
pretty `model_intent`	92-98%	~8%

The "providers always include a space" assumption is unsupported; it is provider- and serializer-dependent, which is exactly why this was worth fixing. Existing tests were compact-only. New tests cover a space, a leading newline, and model_intent arriving before command; deleting the whitespace-skip loop makes all of them red.

One related observation, deliberately out of scope here: the Anthropic path appears to populate ToolCallInput while chatd reads Delta, so no incremental args arrive on that provider at all. Not expanding this PR to chase it.

CRF-135 (P3, shared expansion state) - fixed

Confirmed with DOM evidence. Reproduction: expand a one-file ReadFileTool, a second read resolves, and the same boolean now drives ReadFilesTool. The user's file collapses while a group they never opened is open.

Fix: expanded/onExpandedChange removed from ReadFilesTool; each disclosure owns uncontrolled state. New story ReadFileRunGrowthDoesNotCarryExpansion goes red if shared state is restored.

The suggested arity-keyed state was rejected: it breaks 2 to 3 growth by closing the group and discarding per-file expansion.

CRF-139 (P4, `proto` provider key) - fixed

Confirmed as a correctness bug, not global prototype pollution. __proto__ never touched Object.prototype; it re-parented the parser-local accumulator, which made the streaming parse and the final parse disagree.

Two details worth recording: there are two provider-key writes in streamingJson.ts, not one. Both are now covered by an accumulator created with Object.create(null); swapping it back to {} makes the new test red.

CRF-140 (P4, missing frontend table entries for `stop_workspace`) - not fixed

Confirmed. stop_workspace falls through to generic icon, label, and renderer handling, and list_subagent_models has the same gap. Both are pre-existing and unrelated to anything this PR touches, so fixing them here would be scope creep for no reviewability gain. The human has noted it; no issue filed.

CRF-141 (P4, quadratic streaming JSON parsing) - follow-up PR

The initial instinct was to decline this. That was wrong, and the P4/4.2 s framing understates it. A bounded standalone benchmark against the real four-argument mergeStreamPayload signature (mirroring streamState.ts:60) measured:

payload	16 B chunks	64 B chunks	256 B chunks
33 KB	477 ms	109 ms	49 ms
65 KB	1,979 ms	488 ms	121 ms
130 KB	9,614 ms	2,427 ms	570 ms
281 KB	aborted >20 s	aborted >20 s	6,805 ms

Doubling the payload roughly quadruples the time, as predicted. At a realistic 96-128 KB output budget, OpenAI-sized deltas can block the main thread for 5-10 seconds. Google sends a single delta and Anthropic does not appear to send args_delta, so the exposure is the OpenAI chat-completions and Responses paths.

The one-line "only attempt JSON.parse when a closing brace exists" short-circuit was tried and saved only ~10%, so there is a real design choice here rather than a cheap patch: true incremental parser state, or stop progressive parsing above a size threshold and parse once on completion. That belongs in its own PR with its own benchmarks, not bolted onto this one. No issue filed.

Generated by Coder Agents.

DanielleMaywood · 2026-07-30T10:36:55Z

/coder-agents-review set-spend-limit:1200

coder-agents-review

Ten of the eleven findings you were shown are fixed in one commit, and the two you contested were both checked by three reviewers each and both held. unknown is the right shape: ToolStatus stops re-declaring itself, isSettledToolStatus names the grouping presentation depends on, the settled-bit table moves one field per row, and every mutation the description claims was reproduced by someone here, including the two that reverted green in earlier rounds. Bisky ran the round-8 tree against your new expansion story and it fails there and passes here, which is the shape a regression test is supposed to have.

4 P2, 19 P3, 3 P4, 9 Nit.

The P2 that matters: five reviewers, independently, found that the one predicate whose job is to ask whether a row is still waiting does not read the state you added to answer that question. isExecutePendingCommand is still status !== "error" && result === undefined && no command, so unknown lands on the pending side, a settled command-less execute becomes unresolved-tool, and getRenderableContentState drops the whole assistant message. The new table pins it as a requirement: [{}, "unknown", undefined, true].

Your CRF-130 defense said the fourth status is "an identical output matrix, so it is modelling rather than behaviour". It is identical, and that is the finding: the vocabulary landed four call sites away from the predicate that needed it.

I checked the reachability argument the description rests on, because three reviewers disagreed about it. The claim that the server "commits a call and its result in one CommitStep" is false for local tool calls. generateAssistant builds and commits the assistant step carrying the tool-call parts (generation.go:649-660), and executeLocalTools builds and commits the results in a later step (:691). buildCommitStepMessages pairs whatever is in the step it is given; in the assistant step the results do not exist yet. FinishInterruption does guard outstanding calls (chatstate/transitions.go:1246-1255), and FinishError (:1389) has no equivalent guard, so a chat that errors between those two commits parks with a durable call and no result forever. That is the state, it is reachable, and a command-less one takes its message with it.

Three findings this round are on comments and docs the last commit rewrote or deleted, and each one dropped the clause naming the condition the code enforces. The isExecutePendingCommand doc lost the status !== "error" sentence that your own body still calls load-bearing; toTimelineBlocks lost the "only two variants can render nothing" sentence in the same commit that added a second consumer depending on it. That is the eighth round in this class. Leorio, Mafu-san, Mafuuu and Gon each arrived at it separately, and the fix in each case is one sentence you already wrote once.

Deferrals with no owner. CRF-141 you raised above its filed severity yourself, with a benchmark, and then named a follow-up PR with no number and filed no issue. Takumi then measured that this round's whitespace skip triples the cost for pretty-printed payloads, so part of it is no longer pre-existing. CRF-140, CRF-102, CRF-13, CRF-86 and CRF-132's two constraints for whoever takes CRF-102 are in the same position: agreed, unfixed, recorded only in a review thread that closes with this PR. Neither of us can accept those as permanent. That needs a human decision: an issue, or an explicit choice to leave them.

One correction on my side. Five findings Netero raised in round 8 (CRF-122 to CRF-126) were folded into panel comments that did not carry them, so you never saw them. That is a routing failure in this review, not silence on yours. They are posted here, and Netero re-measured three of them: the suppressed-tool mutation on hasRunningToolBlock is still green, Ran no command still has no renderer, and two of the three vacuous story fallbacks are still there.

Ryosuke, drawn as a wildcard and reviewing a frontend PR: "You have the racing line drawn. The car is still taking the corner in three separate inputs."

site/src/pages/AgentsPage/components/ChatElements/tools/WebSearchSources.tsx:79

P2 [CRF-147] A provider-supplied source URL reaches href with no scheme check. (Kurapika P4, raised here at P2)

Kurapika traced the whole path: "chatloop.go:935-947 copies part.URL off the provider stream into SourceContent, chatprompt.go:761-766 publishes it as codersdk.ChatMessagePart{Type: "source", URL: ...}, messageParsing.ts:259-261 accepts any truthy part.url into a sources block, and SourcePill renders href={source.url}. Nothing between the socket and the anchor restricts the scheme."

He filed it P4 because the sink is outside this diff. I am raising it, and saying why: the consequence is script execution in the dashboard origin when a user clicks a citation pill, the model controls the value, and target="_blank" plus rel="noopener noreferrer" do not gate javascript:. Being outside the diff bounds whose fault it is, not what happens.

His fix is at the parse boundary next to the existing if (part.url): accept http: and https: after new URL(...), drop the part otherwise. One place, and it covers the flat parsed.sources list too. If this is not for this PR, it wants an issue rather than a thread.

site/src/pages/AgentsPage/components/ChatConversation/chatStatusHelpers.ts:64

P4 [CRF-163] The ownValue sweep covered the enumerated sites and left the one whose key comes from server config. (Kurapika P4, Robin P4)

PROVIDER_STATUS_URLS[normalized] reads the prototype chain, and normalizeProvider lowercases, so constructor is the one Object.prototype member that survives normalization: getProviderStatusURL returns the Object constructor, typed string, and ChatStatusCallout renders it as an anchor href. Broken link, not an escape, and it takes an admin-chosen provider name.

Robin frames the pattern: "CRF-119 asked for the enumeration and named two sites; the author fixed those two... the fix covered the enumerated instances, not the class." The fix is the helper this PR already added.

Robin also lists siblings outside this subsystem, debugPanelUtils.ts:16, applyKnownModelDefaults.ts:38, ModelConfigFields.tsx:61, all keyed on backend strings. Those are not this PR's.

site/src/pages/AgentsPage/components/ChatElements/tools/SubagentTool.tsx:264

P3 [CRF-142] The isSettledToolStatus sweep converted four positive === "completed" reads and left three. (Netero)

:80 and :90 moved; :264 (toolStatus !== "completed", gates the live desktop preview), :273 (gates the recording preview) and ProposePlanTool.tsx:70 still read the literal. mapSubagentStatusToToolStatus returns its fallback unchanged for an unmapped status, so a MergedTool with status: "unknown" reaches :264 as "unknown" and the branch flips.

Netero also checked which of the three matter: :273 needs recording_file_id and :70 needs content/file_id, both read from the result, and unknown means no result. :264 needs only chat_id from args, so it is reachable at the component boundary and stays dark only because ChatPageContent.tsx:162 hard-codes showDesktopPreviews={false}. Melody and Pariston independently confirmed the enumeration is exactly these three.

So the body's "unknown renders exactly as completed did" is true by two accidents rather than by the sweep. Convert all three.

site/src/pages/AgentsPage/components/ChatConversation/ConversationTimeline.stories.tsx:2951

Nit [CRF-126] Two of the three vacuous ?? <button> selector fallbacks are still there. (Netero)

Raised in round 8 and never posted, my routing error. CRF-118 fixed one. :2951-2953 still fall back to the button when [data-transcript-row] is absent, in the story that measures row heights and inter-row gaps, so with the wrapper gone it silently measures three buttons and can still report gaps === [8, 8].

Drop the ?? arms.

coder-agents-review · 2026-07-30T11:50:18Z

-	return data.command.trim().length > 0 || Boolean(data.authenticateURL);
-};
+/**
+ * True while an `execute` row has no command and no result to explain the


P2 [CRF-145] CRF-133's rewrite dropped the conjunct your own body calls load-bearing, in the cell that has been wrong for four rounds. (Gon P2, Leorio P3, Mafu-san P3, Mafuuu P3)

Gon: "The doc says the row is pending when it 'has no command and no result to explain the absence'. The code has a third conjunct, status !== "error", and an errored execute with no result payload satisfies both conditions the doc names, so by the doc it is pending and by the code it is not."

Leorio found the sentence that used to say it, deleted by this commit: "An errored row is never pending, so that error still reaches the transcript."

Mafu-san and Mafuuu each deleted the conjunct and measured one failing row, which is why they filed P3. I am taking Gon's P2: the doc and the test row are editable in one commit by the same reasoning error, and that combination is exactly CRF-121 shipped again. Restoring the deleted sentence costs one line.

🤖

coder-agents-review · 2026-07-30T11:50:18Z


+		// extractIncompleteStringContent below reads from this index, so
+		// whitespace must not reach it.
+		while (index < trimmed.length && /\s/.test(trimmed[index])) {


P2 [CRF-146] The whitespace skip moves pretty-printed tool args onto the quadratic path, measured, and the finding that owns that path has no owner. (Takumi P2, Killua P2, Pariston P2, Mafu-san P3)

Takumi isolated the five lines: "head-minus-skip is head with lines 343 to 347 deleted and nothing else. It lands on base." His numbers for a 30 KB pretty payload at 16 B chunks: base 147 ms, head 419 ms. Before the skip, extractIncompleteStringContent returned null on its first comparison for whitespace-preceded values; now it rebuilds the partial string from the buffer on every delta.

Killua measured the same curve independently (8 KB 18 ms, 64 KB 513 ms, quadrupling per doubling) and named the exposure: chattool/editfiles.go:35-36 streams old_text and new_text as tool args, so a 100 KB edit spends seconds on the main thread.

The fix is correct and nobody is asking for it back. What changed is the argument: your CRF-141 decline covers a pre-existing cost, and this is a regression this diff introduces on the same path. Either bound the rebuild by carrying the extraction cursor per tool, or file the issue so both halves have an owner. Mafu-san's minimum is three lines of comment at mergeStreamPayload recording the shape, the numbers and the date, so the measurement survives the thread.

🤖

coder-agents-review · 2026-07-30T11:50:18Z

+export type ToolStatus = "completed" | "error" | "running" | "unknown";
+
+/** A call with no result is as settled as a completed one: nothing more is coming. */
+export const isSettledToolStatus = (status: ToolStatus): boolean =>


P3 [CRF-148] isSettledToolStatus returns false for error, which is the most settled status there is. (Gon P3, Leorio P3, Mafuuu P3)

Gon: "Its own call site proves the point: SubagentTool.tsx:88 writes isSettledToolStatus(toolStatus) ? "completed" : toolStatus === "error" ? "error" : "running", so the second arm exists only to catch the settled status the first arm rejected."

Leorio priced the plausible edit: add || status === "error", which the name invites, and every failed spawn renders Spawned X and every errored ask_user_question becomes interactive again. The doc argues for including unknown and never mentions the exclusion.

Mafuuu adds the reason it will not stay a two-caller problem: the helper exists so more sites adopt it, and CRF-142 asks for three more conversions. Name it for what it decides, or state the exclusion at the definition.

🤖

coder-agents-review · 2026-07-30T11:50:18Z

+	asString(parseArgs(args)?.command).trim().length === 0;

-const shouldRenderSubagentLifecycleTool = ({
+export const shouldRenderTool = ({


P3 [CRF-149] shouldRenderTool kept the general name for a predicate that now answers only the subagent question, and the same commit deleted its doc. (Mafuuu P3, Knov Nit, Zoro Nit)

Mafuuu: "d4ea4e733 deleted shouldRenderSubagentLifecycleTool and moved its body verbatim into shouldRenderTool, deleted the isExecutePendingCommand short-circuit, and deleted that doc without replacement."

The cost is which variant the next tool lands in: !shouldRenderTool produces suppressed-tool, which renders nothing in either mode, while isExecutePendingCommand produces unresolved-tool, which draws the placeholder and counts as a running row in streamingActivity.ts:19. A reader who trusts the name gets the wrong one.

This is CRF-116 recreated on the sibling one round after CRF-116 closed. Zoro's fix is the cheapest: restore the name the file had one commit ago.

🤖

coder-agents-review · 2026-07-30T11:50:18Z

+	name === "execute" &&
+	status !== "error" &&
+	result === undefined &&
+	asString(parseArgs(args)?.command).trim().length === 0;


P3 [CRF-150] Two definitions of the execute command, 46 lines apart, and the predicate's copy of the trim is unpinned. (Robin P3, Bisky P3, Knov P3, Zoro Nit)

Robin: "Oh? The same sentence, twice, forty-six lines apart. How interesting." The drift is not hypothetical, it happened inside this diff: d4ea4e733 added .trim() at :30 because the render path was keeping a whitespace-only command the predicate had already called absent.

Bisky measured the half that is not pinned: dropping .trim() from the predicate at :76 leaves all 927 unit tests green, because every row in the new table uses {} or a fully formed command. One row closes it, [{ command: " " }, "running", undefined, true].

Zoro and Robin propose the same two-line helper, getExecuteCommand(args), called from both. The predicate still does not build the payload, and the two can no longer disagree about what counts as a command.

🤖

coder-agents-review · 2026-07-30T11:50:19Z

 		},
 	});
 	const isRunning = status === "running";
+	const isSettled = isSettledToolStatus(status);


P3 [CRF-143] Three of the four isSettledToolStatus conversions cannot be caught by any test. (Netero)

isSettledToolStatus(s) and s === "completed" differ only at unknown, and Netero's grep for "unknown" across the page's tests and stories returns one story (SubagentSpawnWithNoResult, a spawn_agent label), three mergeTools status assertions and one isExecutePendingCommand row. No fixture gives ask_user_question or a computer_use wait that status.

So reverting showAnsweredState (:407), isInteractive (:416) and the computer-use arm (SubagentTool.tsx:80) is green by construction, and the persisted result-less ask-question row silently loses its answered state and its interactivity, which is the regression the conversions exist to prevent.

One ask_user_question row at status: "unknown" covers all three.

🤖

coder-agents-review · 2026-07-30T11:50:19Z

+		}
+
+		const candidate = tools.at(cursor);
+		const tool = candidate?.id === block.id ? candidate : undefined;


P3 [CRF-122] The positional cursor never resyncs, so one misordered tool deletes every later row rather than one. (Netero, Komugi)

Raised in round 8 and never posted, my routing error. Netero re-measured with a throwaway unit test: blocks [a,b,c] with tools [b,a,c] returns [unresolved-tool a, tool b, unresolved-tool c]; with [c,a,b] only the third block resolves. cursor advances only on a match, so a mismatch consumes nothing and every later block is compared against the same stale entry. Off-stream the unresolved-tool arm returns null and getRenderableContentState filters it, so the rows and possibly the whole message disappear with no console error.

Komugi adds this round's blast radius: the live shimmer now runs through the same function, and unresolved-tool returning true at streamingActivity.ts:19 means shouldShowGenericThinking stays false for the rest of the stream, so the bubble is empty with no progress indicator and no later chunk repairs it.

Melody confirmed no producer emits out-of-order tools today, which is why this is P3. Containment is a one-line resync, or the ordering assertion the comment at :59-60 currently asserts in prose.

🤖

coder-agents-review · 2026-07-30T11:50:19Z

+): boolean =>
+	toTimelineBlocks(streamState?.blocks ?? [], streamTools).some((block) => {
+		switch (block.type) {
+			case "unresolved-tool":


P3 [CRF-123] CRF-103's fix is still unpinned at both levels, and the mutation is green. (Netero)

Raised in round 8 and never posted, my routing error. hasRunningToolBlock must count an unresolved-tool block and must not count a suppressed-tool one, which is the whole of CRF-103: a live wait_agent without a chat_id renders nothing, so the generic shimmer has to stay.

Netero verified the gap: adding case "suppressed-tool": return true; next to the unresolved-tool arm leaves streamingActivity.test.ts at 11 passed. The file's six tool cases all use read_file, and wait_agent appears in no story that goes through the timeline.

One row with a suppressed lifecycle tool closes it.

🤖

coder-agents-review · 2026-07-30T11:50:19Z

+			: intentLabel
+		: commandDisplay
+			? `Ran ${commandDisplay}`
+			: "Ran no command";


P3 [CRF-124] Ran no command is a user-visible string with no renderer in any test or story. (Netero, Leorio)

Raised in round 8 and never posted, my routing error. grep -rn "Ran no command" across site/src returns the definition only. CRF-106 exists because the command-less row rendered Ran with nothing after it, so this literal is the fix, and it is reachable only when intentLabel and commandDisplay are both empty. ExecuteTool has stories, so this is one args-{} errored-execute story.

Leorio read the string while checking the row and reports the rest of it is sound: chatprompt.go:826 marshals {"error":"command is required"}, getExecuteRenderData turns that into an error transcript block, and hasTranscriptBlocks opens the row, so the operator does see the reason next to a failure triangle. Only the headline misreports, which is why he is not asking for a copy change and I am asking for the story.

🤖

coder-agents-review · 2026-07-30T11:50:19Z

+	| (Exclude<RenderBlock, { type: "tool" }> & { sourceIndex: number })
+	| { type: "tool"; tool: MergedTool }
+	| { type: "unresolved-tool"; id: string }
+	| { type: "suppressed-tool"; id: string }


Nit [CRF-125] suppressed-tool carries an id nothing reads. (Netero)

Raised in round 8 and never posted, my routing error. Netero's six hits for the variant: the declaration, the push at :91, ConversationTimeline.tsx:356 (return null), messageHelpers.ts:52 (type check only), and two blockUtils.test.ts expectations. The field's only readers are the tests asserting the field.

{ type: "suppressed-tool" } says what the variant means: nothing about the tool survives suppression. The sibling is load-bearing by contrast, unresolved-tool's id is the React key at ConversationTimeline.tsx:347.

🤖

…unreachable WaitForExternalAuth tool code (#27684) The backend never emits a `wait_for_external_auth` tool call (no references in any Go source, the chatd tool registry, or anywhere outside the frontend), so the entire frontend rendering path for it was unreachable. Removes the `WaitForExternalAuthTool` component, its renderer and `toolRenderers` entry, the `ToolIcon` case, and the four Storybook stories, along with the imports that only they used (`CheckIcon`, `LoaderIcon`, `LogInIcon`, and `toProviderLabel` in `Tool.tsx`). Kept the separate, live `execute` auth-required flow: `ExecuteAuthRequiredTool` and the `toProviderLabel` usage in `toolVisibility.ts` belong to the `authenticateURL` path, not this dead tool. Refs #27593 🤖 This pull request was created with Coder Agents.

DanielleMaywood mentioned this pull request Jul 28, 2026

refactor(site/src/pages/AgentsPage/components/ChatConversation): delete the block-less tool pass #27590

Closed

github-actions Bot assigned DanielleMaywood Jul 28, 2026

DanielleMaywood marked this pull request as draft July 28, 2026 15:42

DanielleMaywood added 2 commits July 28, 2026 15:59

coder-agents-review Bot reviewed Jul 28, 2026

View reviewed changes

DanielleMaywood changed the title ~~refactor(site/src/pages/AgentsPage/components/ChatConversation): make timeline blocks carry their tool~~ refactor(site/src/pages/AgentsPage/components): make timeline blocks carry their tool Jul 28, 2026

coder-agents-review Bot reviewed Jul 28, 2026

View reviewed changes

coder-agents-review Bot reviewed Jul 29, 2026

View reviewed changes

DanielleMaywood added 2 commits July 29, 2026 13:42

docs(site/src/pages/AgentsPage/components/ChatConversation): say why …

61530b4

…buildStreamTools walks blocks

coder-agents-review Bot reviewed Jul 29, 2026

View reviewed changes

DanielleMaywood changed the title ~~refactor(site/src/pages/AgentsPage/components): make timeline blocks carry their tool~~ fix(site/src/pages/AgentsPage/components): make timeline blocks carry their tool Jul 29, 2026

coder-agents-review Bot reviewed Jul 29, 2026

View reviewed changes

coder-agents-review Bot reviewed Jul 30, 2026

View reviewed changes

DanielleMaywood mentioned this pull request Jul 30, 2026

fix(site/src/pages/AgentsPage/components/ChatElements/tools): delete unreachable WaitForExternalAuth tool code #27684

Merged

github-actions Bot added the stale This issue is like stale bread. label Aug 19, 2026

github-actions Bot closed this Aug 22, 2026

		@@ -1062,11 +975,11 @@ const StartWorkspaceRenderer: FC<ToolRendererProps> = ({
		// ---------------------------------------------------------------------------

		const toolRenderers: Record<string, FC<ToolRendererProps>> = {

variant	median µs per `toTimelineBlocks`
`getExecuteRenderData`	10.86 / 11.05 / 10.41
`args.command`	5.04 / 7.22 / 5.75

reverted key	fails
303 `SmoothedResponse`	streaming story
310 `Response`	non-streaming story
317 `Fragment`	both
336 `file-reference`	both
404 `file`	both
415 `sources`	both

mutation	red row
drop `result === undefined`	`completed` with a non-error result
drop `status !== "error"`	`error` with no result
your suggested `status === "running"`	`unknown` with no result

command-less execute	964dc	`b15495d`	now
no result, running	hidden	hidden	hidden
no result, settled	hidden	hidden	hidden
non-error result	renders	hidden	renders
error result	renders	renders	renders

Conversation

DanielleMaywood commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change

status can say it does not know

Behaviour changes

Same shape, three more places

Both mergers walk blocks

Dead render paths, deleted rather than deferred

The structured external auth payload never shipped

Size

Testing

Supersedes #27590

Uh oh!

DanielleMaywood commented Jul 28, 2026

Uh oh!

coder-agents-review Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Finding inventory: PR #27593

Findings

Contested and acknowledged

CRF-8 (P2, ConversationTimeline.tsx:474 deleted) - tool-to-block direction unpinned

CRF-9 (P3, ReadFilesTool.tsx:22) - tuple admits one file, component needs two

CRF-23 (Nit, messageHelpers.test.ts:353) - duplicated table row

CRF-13 (P3, Tool.stories.tsx:3146) - showcase maps through <Tool>

CRF-6 and CRF-7 (Notes, Tool.stories.tsx)

CRF-130 (P2, blockUtils.ts:88) - split the behaviour-changing slice out

CRF-132 (P3, ReadFilesTool.tsx:24) - position key safe only because of the CRF-102 remount

CRF-137 (P3, messageHelpers.test.ts:343) - it.each row pins CRF-121's defect

CRF-140 (P4, ToolIcon.tsx:92) - stop_workspace missing from all three per-tool tables

CRF-141 (P4, streamingJson.ts:428) - quadratic streaming JSON parsing

Round log

Round 1

Round 2

Round 3

Round 4

Round 5

Round 6

Round 7

Round 8

Round 9

Uh oh!

coder-agents-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coder-agents-review Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coder-agents-review Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coder-agents-review Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielleMaywood commented Jul 28, 2026

Uh oh!

DanielleMaywood commented Jul 28, 2026

Uh oh!

coder-agents-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coder-agents-review Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coder-agents-review Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coder-agents-review Bot Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

DanielleMaywood commented Jul 28, 2026 •

edited

Loading

`status` can say it does not know

coder-agents-review Bot commented Jul 28, 2026 •

edited

Loading

CRF-13 (P3, Tool.stories.tsx:3146) - showcase maps through `<Tool>`

CRF-137 (P3, messageHelpers.test.ts:343) - `it.each` row pins CRF-121's defect

CRF-140 (P4, ToolIcon.tsx:92) - `stop_workspace` missing from all three per-tool tables