persist command in pty service, refactor how commands are restored on window reload for inline chat terminal #274417
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: Copilot <[email protected]>
- Added terminalCommandUri?: UriComponents field to the interface - Fixes TypeScript compilation errors in chatTerminalToolProgressPart and runInTerminalTool Co-authored-by: meganrogge <[email protected]>
- Removed _updateTerminalCommandMetadata and all its calls - Use getTerminalInstanceByToolSessionId instead of terminalCommandUri for getting terminal instance - Remove _getTerminalResource method from chatTerminalToolProgressPart - Remove terminalCommandUri field from IChatTerminalToolInvocationData - Output HTML is now retrieved dynamically via _collectOutput when needed - Simplifies code by 66 lines while maintaining functionality Co-authored-by: meganrogge <[email protected]>
- Updated registerTerminalInstanceWithToolSession to accept optional terminalCommandId - Store pre-assigned command ID immediately instead of waiting for onCommandFinished - Pass terminalCommandId from RunInTerminalTool to terminalChatService - Centralizes command ID management in terminalChatService for better consistency Co-authored-by: meganrogge <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR implements a mechanism to pre-assign command IDs for terminal commands executed by chat agent tools, enabling better tracking and linking of commands between the renderer and ptyHost. The system generates a custom command ID early in the process flow and propagates it through the execution pipeline.
Key changes:
- Introduced
terminalCommandIdfield toIChatTerminalToolInvocationDatafor pre-assigned command IDs - Added
setNextCommandIdmethod throughout the terminal process chain (fromITerminalInstancetoITerminalChildProcess) - Updated execution strategies to pass the command ID when executing terminal commands
- Modified command detection to use pre-assigned IDs when available
- Refactored output collection to use the pre-assigned command ID for lookup
Reviewed Changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/common/chatService.ts | Added terminalCommandId field to terminal tool invocation data, replacing terminalCommandUri |
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/runInTerminalTool.ts | Generated and propagated custom command ID through tool execution flow; removed _updateTerminalCommandMetadata |
| src/vs/workbench/contrib/terminal/browser/terminal.ts | Added commandId parameter to runCommand and registerTerminalInstanceWithToolSession methods |
| src/vs/workbench/contrib/terminal/browser/terminalInstance.ts | Implemented command ID pre-assignment before command execution |
| src/vs/workbench/contrib/terminalContrib/chat/browser/terminalChatService.ts | Updated to handle pre-assigned command IDs and fixed persistent process ID access |
| src/vs/workbench/contrib/chat/browser/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts | Refactored to use command ID for output lookup; streamlined action creation |
| src/vs/workbench/contrib/terminal/browser/xterm/xtermTerminal.ts | Changed getCommandOutputAsHtml return type to include truncation status |
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/executeStrategy/*.ts | Added commandId parameter to execute strategy interface and implementations |
| src/vs/platform/terminal/common/capabilities/commandDetectionCapability.ts | Implemented setNextCommandId and logic to use pre-assigned IDs |
| src/vs/platform/terminal/common/capabilities/commandDetection/terminalCommand.ts | Changed command ID from required to optional |
| src/vs/platform/terminal/node/ptyService.ts | Added setNextCommandId to pty service chain |
| src/vs/workbench/services/terminal/common/embedderTerminalService.ts | Added no-op setNextCommandId implementation |
| Multiple test files | Added setNextCommandId stub implementations to test mocks |
Comments suppressed due to low confidence (1)
src/vs/workbench/contrib/chat/browser/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts:444
- The explicit
return;on line 444 is unnecessary and inconsistent. The function already returnsundefinedimplicitly when no other return path is taken. Remove line 444 for cleaner code.
const commandId = this._terminalChatService.getTerminalCommandIdByToolSessionId(this._terminalData.terminalToolSessionId);
if (commandId) {
return commands.find(c => c.id === commandId);
}
return;
src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/runInTerminalTool.ts
Outdated
Show resolved
Hide resolved
src/vs/platform/terminal/common/capabilities/commandDetectionCapability.ts
Outdated
Show resolved
Hide resolved
src/vs/platform/terminal/common/capabilities/commandDetection/terminalCommand.ts
Show resolved
Hide resolved
src/vs/workbench/contrib/terminalContrib/chat/browser/terminalChatService.ts
Outdated
Show resolved
Hide resolved
…r/tools/runInTerminalTool.ts Co-authored-by: Copilot <[email protected]>
…apability.ts Co-authored-by: Copilot <[email protected]>
This was referenced Oct 31, 2025
rebornix
approved these changes
Oct 31, 2025
Tyriar
reviewed
Nov 1, 2025
src/vs/platform/terminal/common/capabilities/commandDetectionCapability.ts
Show resolved
Hide resolved
Comment on lines
+143
to
+145
| async setNextCommandId(commandLine: string, commandId: string): Promise<void> { | ||
| // No-op | ||
| } |
There was a problem hiding this comment.
This isn't great that there are a bunch of no-op ones. That's due to being on ITerminalChildProcess?
Could we put it on IPtyService instead?
src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/runInTerminalTool.ts
Show resolved
Hide resolved
This was referenced Nov 3, 2025
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #274311
fixes #274179
This PR implements a mechanism to pre-assign command IDs for terminal commands executed by chat agent tools, enabling better tracking and linking of commands between the renderer and ptyHost. The system generates a custom command ID early in the process flow and propagates it through the execution pipeline.
Key changes:
terminalCommandIdfield toIChatTerminalToolInvocationDatafor pre-assigned command IDssetNextCommandIdmethod throughout the terminal process chain (fromITerminalInstancetoITerminalChildProcess)