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>
2026-07-07 07:16:41 +01:00 · 2026-06-25 19:56:53 -07:00
parent cd234557ac
commit ebd256aa1e
2 changed files with 237 additions and 10 deletions
@@ -310,6 +310,7 @@ export class ChatThinkingContentPart extends ChatCollapsibleContentPart implemen
 	private readonly toolWrappersByCallId = new Map<string, HTMLElement>();
 	private readonly toolIconsByCallId = new Map<string, HTMLElement>();
 	private readonly toolLabelsByCallId = new Map<string, string>();
+	private readonly toolOriginalPositionByCallId = new Map<string, { element: HTMLElement; originalParent: HTMLElement }>();
 	private readonly toolDisposables = this._register(new DisposableMap<string, DisposableStore>());
 	private readonly ownedToolParts = new Map<string, IDisposable>();
 	private pendingRemovals: { toolCallId: string; toolLabel: string }[] = [];
@@ -327,6 +328,7 @@ export class ChatThinkingContentPart extends ChatCollapsibleContentPart implemen
 	private readonly _pendingExternalResources = new Map<string, IChatToolInvocation | IChatToolInvocationSerialized>();
 	private readonly _titleDetailRendered = this._register(new MutableDisposable<IRenderedMarkdown>());
 	private readonly _pendingAppendRefresh = this._register(new MutableDisposable<IDisposable>());
+	private readonly _singleItemPromotionRetry = this._register(new MutableDisposable<IDisposable>());
 	private readonly diffStatsByPartId = new Map<string, IEditSessionDiffStats>();
 	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);
@@ -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<IChatToolInvocation.State>('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', () => {