Merge pull request #72308 from NousResearch/bb/tool-group-bounding

fix(desktop): stop reads and edits vetoing tool-call grouping

apps/desktop/src/components/assistant-ui/tool/fallback.test.ts

@@ -1,7 +1,6 @@
 import { describe, expect, it } from 'vitest'
 
 import { isUnboundableTool, shouldBoundToolGroup, technicalTrace } from './fallback'
-import { isFileEditTool } from './fallback-model'
 
 describe('shouldBoundToolGroup', () => {
   it('bounds long runs of ordinary tool calls', () => {
@@ -23,25 +22,18 @@ describe('isUnboundableTool', () => {
     expect(isUnboundableTool('image_generate')).toBe(true)
   })
 
-  it('exempts tools whose body is a code block the user reads', () => {
-    expect(isUnboundableTool('execute_code')).toBe(true)
-    expect(isUnboundableTool('read_file')).toBe(true)
-  })
-
-  // The window clips a diff to a ~2-row viewport, so every tool that renders
-  // one has to be exempt. Derived from isFileEditTool rather than re-listed,
-  // so a newly supported edit tool can't be exempt in one place and clipped
-  // in the other.
-  it('exempts every file-edit tool, so diffs are never clipped', () => {
-    for (const toolName of ['edit_file', 'patch', 'write_file']) {
-      expect(isFileEditTool(toolName)).toBe(true)
-      expect(isUnboundableTool(toolName)).toBe(true)
+  // Everything ToolEntry renders carries `data-tool-row`, so the
+  // `:has([data-tool-row][data-tool-open])` rule in styles.css lifts the cap
+  // on its own. A diff row mounts open and frees the group immediately; a
+  // collapsed row has no body in the DOM to clip. Exempting these in JS
+  // instead vetoed grouping for the whole run — and since reads and edits are
+  // most of a coding session, runs of 19 calls never collapsed at all.
+  it('bounds the rows the CSS break-out already covers', () => {
+    for (const toolName of ['read_file', 'execute_code', 'edit_file', 'patch', 'write_file']) {
+      expect(isUnboundableTool(toolName)).toBe(false)
     }
   })
 
-  // Console output is a log tail: the last lines are the ones that matter,
-  // which is exactly what the bounded window pins. Keeping it boundable is
-  // what stops the exemption from swallowing the feature whole.
   it('still bounds console output and other ordinary rows', () => {
     expect(isUnboundableTool('terminal')).toBe(false)
     expect(isUnboundableTool('web_search')).toBe(false)

apps/desktop/src/components/assistant-ui/tool/fallback.tsx

@@ -700,29 +700,24 @@ function TerminalTranscript({ command, exitCode }: TerminalTranscriptProps) {
 // auto-scrolling window; fewer than this stays a plain inline stack.
 const TOOL_GROUP_SCROLL_THRESHOLD = 3
 
-// Tools whose body (an interactive form, a full-size image, a syntax-
-// highlighted code/diff block) must never be trapped behind the window's
-// max-height + fade mask. A run holding any of them stays a plain, fully-
-// visible stack no matter how long it is.
+// Tools whose body must never be trapped behind the window's max-height +
+// fade mask. A run holding any of them stays a plain, fully-visible stack no
+// matter how long it is.
 //
-// A row rendered by ToolEntry carries `data-tool-row`, so once the user
-// expands it the `:has([data-tool-row][data-tool-open])` rule in styles.css
-// lifts the cap on its own. That escape hatch is why most tools are safe to
-// bound. These are the ones it cannot reach:
+// This list is deliberately tiny. A row rendered by ToolEntry carries
+// `data-tool-row`, and the `:has([data-tool-row][data-tool-open])` rule in
+// styles.css lifts the cap whenever one is open — so anything ToolEntry
+// renders takes care of itself. A code/diff row mounts open (see
+// `defaultOpen`), lifting the cap the moment it appears; a collapsed row is a
+// one-line status with no body in the DOM at all, so there is nothing to clip.
 //
-//   - `clarify` / `image_generate` render their own components and never emit
-//     `data-tool-row`, so no amount of expanding frees them.
-//   - the code tools *do* emit it, but their body is a code block the user
-//     reads rather than a one-line status — peering at a diff through a
-//     ~2-row viewport until you think to expand it is the bug. Console output
-//     (`terminal`) stays boundable: it's a log tail, and the last lines are
-//     the ones that matter, which is exactly what the window pins.
-const CODE_BODY_TOOLS = ['execute_code', 'read_file']
-
-const UNBOUNDABLE_TOOLS = new Set(['clarify', 'image_generate', ...CODE_BODY_TOOLS])
+// Only components that bypass ToolEntry need this opt-out: `clarify` and
+// `image_generate` render their own markup, never emit `data-tool-row`, and so
+// the CSS escape hatch can never reach them.
+const UNBOUNDABLE_TOOLS = new Set(['clarify', 'image_generate'])
 
 export function isUnboundableTool(toolName: string): boolean {
-  return UNBOUNDABLE_TOOLS.has(toolName) || isFileEditTool(toolName)
+  return UNBOUNDABLE_TOOLS.has(toolName)
 }
 
 export function shouldBoundToolGroup(childCount: number, hasUnboundable: boolean) {