Track external tool calls

[roblourens]

No description provided.

[Copilot]

Pull request overview

This PR adds support for tracking external tool invocations from extensions in the chat system. It enables extensions to create live, in-progress tool invocations that can be updated and completed asynchronously, providing better visibility into tool execution status.

Changes:

• Introduced createExternal factory method and update/complete methods for external tool invocations
• Added new DTO type IChatExternalToolInvocationUpdateDto for streaming external tool invocation updates
• Implemented handling logic in mainThreadChatAgents2 to track and update external tool invocations

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
src/vs/workbench/contrib/chat/common/model/chatProgressTypes/chatToolInvocation.ts	Adds factory method, interface, and update/complete methods for external tool invocations
src/vs/workbench/api/common/extHostTypeConverters.ts	Converts ChatToolInvocationPart to either legacy serialized format or new update DTO based on isComplete field
src/vs/workbench/api/common/extHost.protocol.ts	Defines IChatExternalToolInvocationUpdateDto for external tool invocation updates
src/vs/workbench/api/browser/mainThreadChatAgents2.ts	Implements tracking map and handler for external tool invocation lifecycle

Comments suppressed due to low confidence (3)

src/vs/workbench/api/browser/mainThreadChatAgents2.ts:409

• When creating an external tool invocation that is already completed (isComplete: true on first push), it's added to the pending map and then immediately removed. This is inefficient - the invocation should only be added to _pendingExternalToolInvocations if it's not complete (isComplete: false).

		if (progress.isComplete) {
			// Already completed on first push
			invocation.completeExternalInvocation({
				pastTenseMessage: progress.pastTenseMessage,
				isError: progress.isError,
			});
		} else {
			// Track for future updates
			this._pendingExternalToolInvocations.set(progress.toolCallId, invocation);
		}

src/vs/workbench/api/browser/mainThreadChatAgents2.ts:388

• The check for existingInvocation and the early return on line 388 means that if an extension sends an update with isComplete: false after the invocation already exists, nothing is returned and no progress is added. However, if an invocation is created as complete on first push (line 400-405), it won't be in the pending map for any subsequent updates. This creates an inconsistent state where updating an already-completed invocation is silently ignored.

	private _handleExternalToolInvocationUpdate(progress: IChatExternalToolInvocationUpdateDto): ChatToolInvocation | undefined {
		const existingInvocation = this._pendingExternalToolInvocations.get(progress.toolCallId);

		if (existingInvocation) {
			if (progress.isComplete) {
				existingInvocation.completeExternalInvocation({
					pastTenseMessage: progress.pastTenseMessage,
					isError: progress.isError,
				});
				this._pendingExternalToolInvocations.delete(progress.toolCallId);
			} else {
				existingInvocation.updateExternalInvocation({
					invocationMessage: progress.invocationMessage,
					pastTenseMessage: progress.pastTenseMessage,
				});
			}
			return undefined;

src/vs/workbench/api/browser/mainThreadChatAgents2.ts:412

• The new external tool invocation functionality lacks test coverage. Consider adding tests for the following scenarios: (1) creating and completing external tool invocations, (2) updating in-progress invocations, (3) handling duplicate toolCallIds, (4) memory cleanup when requests complete, and (5) error state propagation. The existing test suite at src/vs/workbench/contrib/chat/test/browser/tools/languageModelToolsService.test.ts provides a good pattern to follow.

	/**
	 * Handle external tool invocation updates from extensions.
	 * Returns the invocation if it's new and should be added to progress, otherwise undefined.
	 */
	private _handleExternalToolInvocationUpdate(progress: IChatExternalToolInvocationUpdateDto): ChatToolInvocation | undefined {
		const existingInvocation = this._pendingExternalToolInvocations.get(progress.toolCallId);

		if (existingInvocation) {
			if (progress.isComplete) {
				existingInvocation.completeExternalInvocation({
					pastTenseMessage: progress.pastTenseMessage,
					isError: progress.isError,
				});
				this._pendingExternalToolInvocations.delete(progress.toolCallId);
			} else {
				existingInvocation.updateExternalInvocation({
					invocationMessage: progress.invocationMessage,
					pastTenseMessage: progress.pastTenseMessage,
				});
			}
			return undefined;
		}

		// Create a new external tool invocation
		const invocation = ChatToolInvocation.createExternal({
			toolCallId: progress.toolCallId,
			toolName: progress.toolName,
			invocationMessage: progress.invocationMessage,
			pastTenseMessage: progress.pastTenseMessage,
			toolSpecificData: progress.toolSpecificData as IPreparedToolInvocation['toolSpecificData'],
		});

		if (progress.isComplete) {
			// Already completed on first push
			invocation.completeExternalInvocation({
				pastTenseMessage: progress.pastTenseMessage,
				isError: progress.isError,
			});
		} else {
			// Track for future updates
			this._pendingExternalToolInvocations.set(progress.toolCallId, invocation);
		}

		return invocation;
	}

[Copilot]

The presentation field is supported in the legacy path (line 2921) but not in the external tool invocation update DTO (line 2895-2904). This inconsistency means extensions cannot control the presentation mode (hidden, hiddenAfterComplete) when using the new live update mechanism with isComplete set. Consider adding a presentation field to IChatExternalToolInvocationUpdateDto for consistency.

Suggested change

	pastTenseMessage: part.pastTenseMessage ? MarkdownString.from(part.pastTenseMessage) : undefined,
	pastTenseMessage: part.pastTenseMessage ? MarkdownString.from(part.pastTenseMessage) : undefined,
	presentation,

[Copilot]

The cast to IPreparedToolInvocation['toolSpecificData'] is unsafe. The toolSpecificData from the extension can be any unknown type, but IPreparedToolInvocation['toolSpecificData'] is a union of specific types (IChatTerminalToolInvocationData, IChatToolInputInvocationData, etc.). This type assertion bypasses type safety and could lead to runtime issues if extensions pass incompatible data. Consider validating the structure or using a more general type.

Suggested change

	// Create a new external tool invocation
	const invocation = ChatToolInvocation.createExternal({
	toolCallId: progress.toolCallId,
	toolName: progress.toolName,
	invocationMessage: progress.invocationMessage,
	pastTenseMessage: progress.pastTenseMessage,
	toolSpecificData: progress.toolSpecificData as IPreparedToolInvocation['toolSpecificData'],
	// Validate toolSpecificData from the extension before forwarding it
	let validatedToolSpecificData: IPreparedToolInvocation['toolSpecificData'] | undefined;
	const rawToolSpecificData = progress.toolSpecificData;
	if (rawToolSpecificData && typeof rawToolSpecificData === 'object') {
	const candidate = rawToolSpecificData as { kind?: unknown };
	if (typeof candidate.kind === 'string') {
	validatedToolSpecificData = rawToolSpecificData as IPreparedToolInvocation['toolSpecificData'];
	}
	}
	// Create a new external tool invocation
	const invocation = ChatToolInvocation.createExternal({
	toolCallId: progress.toolCallId,
	toolName: progress.toolName,
	invocationMessage: progress.invocationMessage,
	pastTenseMessage: progress.pastTenseMessage,
	toolSpecificData: validatedToolSpecificData,

[Copilot]

The _pendingExternalToolInvocations map may leak memory if tool invocations never complete. When a request completes (line 193 in the finally block), any pending external tool invocations for that request should be cleaned up. Consider tracking which request each external tool invocation belongs to and cleaning up the map when the request completes, similar to how _pendingProgress is managed.

This issue also appears in the following locations of the same file:

• line 400
• line 372
• line 368

[Copilot]

The isError parameter is accepted but not stored or used. The completeExternalInvocation method should either store this in the resultDetails field (which supports an isError property on IToolResultInputOutputDetails) or remove the parameter if it's not needed. Currently, error information is silently discarded.