From ebd256aa1e8d746eed3f9f6896dfbc55cc99e4da Mon Sep 17 00:00:00 2001 From: Justin Chen <54879025+justschen@users.noreply.github.com> Date: Thu, 25 Jun 2026 19:56:53 -0700 Subject: [PATCH] Fix single tool calls getting stuck inside thinking parts (#323059) * Fix single tool calls getting stuck inside thinking parts A thinking/"Working" section with exactly one tool call should promote that tool to the top level instead of leaving it nested inside the collapsed part. It stayed stuck in three cases: - Multi-child tools: promotion bailed when the tool's DOM had more than one child element, so terminal/search/edit tools never got promoted. - Late completion: finalizeTitleIfDefault() is one-shot, so if the tool was not yet Completed/Cancelled at that instant, promotion was skipped forever. - Lost original position: singleItemInfo gets wiped whenever the item count isn't 1, so a transient sibling or eager rendering left undefined behind. Fix: - Remove the childElementCount > 1 guard so a lone tool is promoted regardless of tool type. - Observe the tool's state and promote once it completes instead of giving up. - Persist each tool's original position so a lone tool can always be reconstructed and promoted. Adds regression tests covering all three cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../chatThinkingContentPart.ts | 50 ++++- .../chatThinkingContentPart.test.ts | 197 ++++++++++++++++++ 2 files changed, 237 insertions(+), 10 deletions(-) diff --git a/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatThinkingContentPart.ts b/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatThinkingContentPart.ts index 9efd9963777..500f7ce4577 100644 --- a/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatThinkingContentPart.ts +++ b/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatThinkingContentPart.ts @@ -310,6 +310,7 @@ export class ChatThinkingContentPart extends ChatCollapsibleContentPart implemen private readonly toolWrappersByCallId = new Map(); private readonly toolIconsByCallId = new Map(); private readonly toolLabelsByCallId = new Map(); + private readonly toolOriginalPositionByCallId = new Map(); private readonly toolDisposables = this._register(new DisposableMap()); private readonly ownedToolParts = new Map(); private pendingRemovals: { toolCallId: string; toolLabel: string }[] = []; @@ -327,6 +328,7 @@ export class ChatThinkingContentPart extends ChatCollapsibleContentPart implemen private readonly _pendingExternalResources = new Map(); private readonly _titleDetailRendered = this._register(new MutableDisposable()); private readonly _pendingAppendRefresh = this._register(new MutableDisposable()); + private readonly _singleItemPromotionRetry = this._register(new MutableDisposable()); private readonly diffStatsByPartId = new Map(); private _aggregatedDiff: IEditSessionDiffStats = { added: 0, removed: 0 }; @@ -1149,10 +1151,39 @@ export class ChatThinkingContentPart extends ChatCollapsibleContentPart implemen } } } - // Only restore if the tool is complete so the progress spinner is resolved - const toolIsComplete = !this.singleItemInfo?.toolInvocation || IChatToolInvocation.isComplete(this.singleItemInfo.toolInvocation); - if (toolIsComplete && this.singleItemInfo && this.restoreSingleItemToOriginalPosition()) { - return; + // If singleItemInfo was cleared (eager render or transient sibling), reconstruct it from the sole tool's recorded original position. + if (!this.singleItemInfo && this.toolInvocations.length === 1) { + const soleTool = this.toolInvocations[0]; + const recorded = this.toolOriginalPositionByCallId.get(soleTool.toolCallId); + if (recorded) { + this.singleItemInfo = { + element: recorded.element, + originalParent: recorded.originalParent, + originalNextSibling: this.domNode, + toolInvocation: soleTool + }; + } + } + // Only restore once the tool is complete; if it completes after finalize, retry promotion. + if (this.singleItemInfo) { + const toolInvocation = this.singleItemInfo.toolInvocation; + if (!toolInvocation || IChatToolInvocation.isComplete(toolInvocation)) { + if (this.restoreSingleItemToOriginalPosition()) { + return; + } + } else { + this._singleItemPromotionRetry.value = autorun(reader => { + const pending = this.singleItemInfo; + if (this._store.isDisposed || !pending?.toolInvocation) { + return; + } + if (IChatToolInvocation.isComplete(pending.toolInvocation, reader)) { + this.restoreSingleItemToOriginalPosition(); + this._singleItemPromotionRetry.clear(); + } + }); + return; + } } } @@ -1431,12 +1462,6 @@ ${this.hookCount > 0 ? `EXAMPLES WITH BLOCKED CONTENT (from hooks): const { element, originalParent, originalNextSibling, toolInvocation } = this.singleItemInfo; - // don't restore it to original position - it contains multiple rendered elements - if (element.childElementCount > 1) { - this.singleItemInfo = undefined; - return false; - } - if (originalNextSibling && originalNextSibling.parentNode === originalParent) { originalParent.insertBefore(element, originalNextSibling); } else { @@ -1581,6 +1606,7 @@ ${this.hookCount > 0 ? `EXAMPLES WITH BLOCKED CONTENT (from hooks): this.toolWrappersByCallId.delete(toolCallId); this.toolIconsByCallId.delete(toolCallId); } + this.toolOriginalPositionByCallId.delete(toolCallId); this.appendedItemCount = Math.max(0, this.appendedItemCount - 1); this.toolInvocationCount = Math.max(0, this.toolInvocationCount - 1); @@ -1728,6 +1754,7 @@ ${this.hookCount > 0 ? `EXAMPLES WITH BLOCKED CONTENT (from hooks): this.toolWrappersByCallId.delete(toolCallId); this.toolIconsByCallId.delete(toolCallId); } + this.toolOriginalPositionByCallId.delete(toolCallId); // make sure to remove any lazy item as well const lazyIndex = this.lazyItems.findIndex(item => @@ -2074,6 +2101,9 @@ ${this.hookCount > 0 ? `EXAMPLES WITH BLOCKED CONTENT (from hooks): if (isToolInvocation && toolInvocationOrMarkdown.toolCallId) { this.toolWrappersByCallId.set(toolInvocationOrMarkdown.toolCallId, itemWrapper); this.toolIconsByCallId.set(toolInvocationOrMarkdown.toolCallId, iconElement); + if (originalParent) { + this.toolOriginalPositionByCallId.set(toolInvocationOrMarkdown.toolCallId, { element: content, originalParent }); + } } this.appendToWrapper(itemWrapper); diff --git a/src/vs/workbench/contrib/chat/test/browser/widget/chatContentParts/chatThinkingContentPart.test.ts b/src/vs/workbench/contrib/chat/test/browser/widget/chatContentParts/chatThinkingContentPart.test.ts index de5cdb0968d..1aea4f70b30 100644 --- a/src/vs/workbench/contrib/chat/test/browser/widget/chatContentParts/chatThinkingContentPart.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/widget/chatContentParts/chatThinkingContentPart.test.ts @@ -1184,6 +1184,203 @@ suite('ChatThinkingContentPart', () => { ariaLabel: 'Analyzed authentication flow', }); }); + + test('finalizeTitleIfDefault should promote a lone tool call to its original position even when its DOM has multiple children', () => { + // Regression: a single tool call that renders multiple child elements (e.g. a + // terminal/search tool with command + output) must still be promoted back to the + // top level instead of staying stuck inside the thinking part. + const content = createThinkingPart(''); + const context = createMockRenderContext(true); + + const part = store.add(instantiationService.createInstance( + ChatThinkingContentPart, + content, + context, + mockMarkdownRenderer, + true + )); + + mainWindow.document.body.appendChild(part.domNode); + disposables.add(toDisposable(() => part.domNode.remove())); + + const originalParent = $('div.original-parent'); + mainWindow.document.body.appendChild(originalParent); + disposables.add(toDisposable(() => originalParent.remove())); + + const tool: IChatToolInvocationSerialized = { + kind: 'toolInvocationSerialized', + toolId: 'run_in_terminal', + toolCallId: 'terminal-call-1', + invocationMessage: 'Running npm test', + originMessage: undefined, + pastTenseMessage: undefined, + presentation: undefined, + isConfirmed: { type: 0 }, + isComplete: true, + source: ToolDataSource.Internal, + isAttachedToThinking: false, + toolSpecificData: { + kind: 'terminal', + commandLine: { original: 'npm test' }, + language: 'shellscript', + } + }; + + // Render a tool DOM node with multiple child elements. + const toolDomNode = $('div.test-multi-child-tool'); + toolDomNode.appendChild($('div.command')); + toolDomNode.appendChild($('div.output')); + + part.appendItem(() => ({ domNode: toolDomNode }), tool.toolId, tool, originalParent); + part.finalizeTitleIfDefault(); + + assert.strictEqual(toolDomNode.parentElement, originalParent, + 'Lone tool call should be restored to its original parent regardless of child element count'); + assert.strictEqual(tool.isAttachedToThinking, false, + 'Promoted tool call should no longer be attached to the thinking part'); + }); + + test('finalizeTitleIfDefault should promote a lone tool call once it completes, even if it is still executing at finalize time', () => { + // Regression: finalizeTitleIfDefault is one-shot, so when a lone tool call has not + // yet reported completion at finalize time it must not be abandoned. Instead it + // should be promoted to its original position once its state observable reports + // completion, regardless of tool type. + const content = createThinkingPart(''); + const context = createMockRenderContext(true); + + const part = store.add(instantiationService.createInstance( + ChatThinkingContentPart, + content, + context, + mockMarkdownRenderer, + true + )); + + mainWindow.document.body.appendChild(part.domNode); + disposables.add(toDisposable(() => part.domNode.remove())); + + const originalParent = $('div.original-parent'); + mainWindow.document.body.appendChild(originalParent); + disposables.add(toDisposable(() => originalParent.remove())); + + const state = observableValue('state', { + type: IChatToolInvocation.StateKind.Executing, + confirmed: { type: 0 }, + progress: observableValue('progress', { progress: 0 }), + parameters: {}, + confirmationMessages: undefined, + } as IChatToolInvocation.State); + + const tool = { + kind: 'toolInvocation', + toolId: 'run_in_terminal', + toolCallId: 'terminal-call-1', + invocationMessage: 'Running npm test', + originMessage: undefined, + pastTenseMessage: undefined, + presentation: undefined, + source: ToolDataSource.Internal, + isAttachedToThinking: false, + generatedTitle: undefined, + state, + toolSpecificDataKind: observableValue('test', undefined), + toJSON: () => ({} as IChatToolInvocationSerialized), + } as IChatToolInvocation; + + const toolDomNode = $('div.test-tool'); + part.appendItem(() => ({ domNode: toolDomNode }), tool.toolId, tool, originalParent); + part.finalizeTitleIfDefault(); + + // Still executing at finalize time: the tool must not be promoted yet, but the + // promotion must not be abandoned either. + assert.notStrictEqual(toolDomNode.parentElement, originalParent, + 'Tool should not be promoted while still executing'); + + // Tool completes after finalize: it should now be promoted to its original position. + state.set({ + type: IChatToolInvocation.StateKind.Completed, + postConfirmed: undefined, + confirmed: { type: 0 }, + contentForModel: [], + resultDetails: undefined, + parameters: {}, + } as IChatToolInvocation.State, undefined); + + assert.strictEqual(toolDomNode.parentElement, originalParent, + 'Tool should be promoted to its original parent once it completes'); + assert.strictEqual(tool.isAttachedToThinking, false, + 'Promoted tool call should no longer be attached to the thinking part'); + }); + + test('finalizeTitleIfDefault should promote a lone tool even when singleItemInfo was wiped by a transient sibling item', () => { + // Regression: a second tool briefly arrives (clearing singleItemInfo) and is then + // removed, leaving a single eagerly-rendered tool with no singleItemInfo and no + // lazy item to recover from. finalize must reconstruct the lone tool's original + // position and still promote it instead of getting `undefined` back and leaving it + // stuck inside the thinking part. + mockConfigurationService.setUserConfiguration('chat.agent.thinkingStyle', ThinkingDisplayMode.FixedScrolling); + const content = createThinkingPart(''); + const context = createMockRenderContext(true); + + // streamingCompleted=false so fixed-scrolling renders items eagerly (no lazy items). + const part = store.add(instantiationService.createInstance( + ChatThinkingContentPart, + content, + context, + mockMarkdownRenderer, + false + )); + + mainWindow.document.body.appendChild(part.domNode); + disposables.add(toDisposable(() => part.domNode.remove())); + + const originalParent = $('div.original-parent'); + mainWindow.document.body.appendChild(originalParent); + disposables.add(toDisposable(() => originalParent.remove())); + + const makeSerializedTool = (toolCallId: string): IChatToolInvocationSerialized => ({ + kind: 'toolInvocationSerialized', + toolId: 'run_in_terminal', + toolCallId, + invocationMessage: `Running ${toolCallId}`, + originMessage: undefined, + pastTenseMessage: undefined, + presentation: undefined, + isConfirmed: { type: 0 }, + isComplete: true, + source: ToolDataSource.Internal, + isAttachedToThinking: false, + toolSpecificData: { + kind: 'terminal', + commandLine: { original: toolCallId }, + language: 'shellscript', + } + }); + + const toolA = makeSerializedTool('tool-a'); + const toolB = makeSerializedTool('tool-b'); + + const toolADomNode = $('div.test-tool-a'); + toolADomNode.textContent = 'Tool A body'; + const toolBDomNode = $('div.test-tool-b'); + toolBDomNode.textContent = 'Tool B body'; + + // Both tools render eagerly with toolInvocationCount === 2 — leaving singleItemInfo + // undefined and no lazy items. + part.appendItem(() => ({ domNode: toolADomNode }), toolA.toolId, toolA, originalParent); + part.appendItem(() => ({ domNode: toolBDomNode }), toolB.toolId, toolB, originalParent); + + // The second tool is removed, returning to a single tool but without re-establishing + // singleItemInfo. + part.removeMaterializedItem(toolB.toolCallId); + + part.finalizeTitleIfDefault(); + + assert.strictEqual(toolADomNode.parentElement, originalParent, + 'Lone remaining tool should be reconstructed and promoted to its original parent'); + assert.strictEqual(toolA.isAttachedToThinking, false, + 'Promoted tool call should no longer be attached to the thinking part'); + }); }); suite('hasSameContent', () => {