Use unified timeout setting under shell integration in runCommand #272369

anthonykim1 · 2025-10-20T23:21:05Z

Resolves: #270813

Main points from: #272369 (comment)

Move the chat shell integration timeout setting under move it under terminal.integrated.shellIntegration.
Use that setting in runCommand
Record the time the terminal process was ready and only apply that timeout however long after that time occurs. If the timeout is 5 seconds and the terminal has been ready for 6 seconds, there's no need to wait at all.

src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/toolTerminalCreator.ts

...rkbench/contrib/terminalContrib/chatAgentTools/common/terminalChatAgentToolsConfiguration.ts

src/vs/workbench/contrib/terminal/browser/terminalCommands.ts

Copilot

Pull Request Overview

This PR unifies shell integration timeout configuration by introducing a new general terminal setting terminal.integrated.shellIntegration.timeout that replaces the chat-specific setting. The change allows this timeout to be used more broadly across terminal features, including the runCommand API.

Key Changes:

Adds new unified terminal.integrated.shellIntegration.timeout setting
Updates chat agent tools to prefer the new setting with fallback to deprecated setting
Modifies runCommand to use the unified timeout with adjusted wait logic based on process ready time

Reviewed Changes

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

Show a summary per file

File	Description
`src/vs/platform/terminal/common/terminal.ts`	Adds `ShellIntegrationTimeout` enum value for the new unified setting
`src/vs/workbench/contrib/terminal/common/terminalConfiguration.ts`	Defines the new `terminal.integrated.shellIntegration.timeout` configuration property
`src/vs/workbench/contrib/terminalContrib/chatAgentTools/common/terminalChatAgentToolsConfiguration.ts`	Marks the chat-specific timeout setting as deprecated and references the new unified setting
`src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/toolTerminalCreator.ts`	Updates to prefer the new unified timeout setting with fallback to deprecated chat setting
`src/vs/workbench/contrib/terminal/common/terminal.ts`	Adds `processReadyTime` property to track when terminal process becomes ready
`src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts`	Implements tracking of process ready time
`src/vs/workbench/contrib/terminal/browser/terminalInstance.ts`	Refactors `runCommand` to use unified timeout with elapsed time calculation
`src/vs/workbench/contrib/terminal/browser/terminalCommands.ts`	Adds temporary test command for validating the runCommand timeout behavior

src/vs/workbench/contrib/terminal/browser/terminalInstance.ts

...rkbench/contrib/terminalContrib/chatAgentTools/common/terminalChatAgentToolsConfiguration.ts

src/vs/workbench/contrib/terminal/browser/terminalInstance.ts

src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/toolTerminalCreator.ts

src/vs/workbench/contrib/terminal/browser/terminalInstance.ts

Tyriar · 2025-10-21T12:17:54Z

src/vs/workbench/contrib/terminal/browser/terminalInstance.ts

+
+		if (timeOutValue === undefined || typeof timeOutValue !== 'number' || timeOutValue < 0) {
+			const siEnabled = this._configurationService.getValue(TerminalSettingId.ShellIntegrationEnabled) === true;
+			// Discuss: This default from copilot chat setting feels pretty slow..


It's slow, but only if things don't work and only for the first command. We need to check the time against when processReady happens

Is what I have now below good?
I do math to subtract the processReadyTime from now.

vscode/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts

Lines 935 to 955 in e46d70b

const processReadyTime = this._processManager.processReadyTime;

if (processReadyTime) {

const elapsed = Date.now() - processReadyTime;

timeoutMs = Math.max(0, timeoutMs - elapsed);

}

// Discuss: Talk about, get clarification one more time.

await Promise.race([

new Promise<void>(r => {

store.add(this.capabilities.onDidAddCommandDetectionCapability(e => {

commandDetection = e;

if (commandDetection.promptInputModel.state === PromptInputState.Input) {

r();

} else {

store.add(commandDetection.promptInputModel.onDidStartInput(() => {

r();

}));

}

}));

}),

timeout(timeoutMs)

src/vs/workbench/contrib/terminal/browser/terminalInstance.ts

...rkbench/contrib/terminalContrib/chatAgentTools/common/terminalChatAgentToolsConfiguration.ts

src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/toolTerminalCreator.ts

...rkbench/contrib/terminalContrib/chatAgentTools/common/terminalChatAgentToolsConfiguration.ts

src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/toolTerminalCreator.ts

…ering processReadytime

src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/toolTerminalCreator.ts

...bench/contrib/terminalContrib/chatAgentTools/browser/terminal.chatAgentTools.contribution.ts

Tyriar · 2025-10-22T12:15:56Z

...bench/contrib/terminalContrib/chatAgentTools/browser/terminal.chatAgentTools.contribution.ts

+		const newSettingValue = this._configurationService.getValue<unknown>(TerminalSettingId.ShellIntegrationTimeout);
+		const deprecatedSettingValue = this._configurationService.getValue<unknown>(TerminalChatAgentToolsSettingId.ShellIntegrationTimeout);
+
+		if (!isNumber(newSettingValue) && isNumber(deprecatedSettingValue)) {
+			this._configurationService.updateValue(TerminalSettingId.ShellIntegrationTimeout, deprecatedSettingValue);
+		}


Do the minimum work to gate this:

const deprecatedSettingValue = this._configurationService.getValue<unknown>(TerminalChatAgentToolsSettingId.ShellIntegrationTimeout); if (isNumber(deprecatedSettingValue)) { //... }

That makes it more clear and in the common case saves a couple of function calls.

Nice, yeah.
Something like:

if (isNumber(deprecatedSettingValue)) { const newSettingValue = configurationService.getValue<unknown>(TerminalSettingId.ShellIntegrationTimeout); if (!isNumber(newSettingValue)) { configurationService.updateValue(TerminalSettingId.ShellIntegrationTimeout, deprecatedSettingValue); } }

Tried to go one more step, and inverted the first if and return ear, what do you think? 262be84

src/vs/workbench/contrib/terminal/common/terminalConfiguration.ts

src/vs/workbench/contrib/terminal/common/terminal.ts

Co-authored-by: Daniel Imms <2193314+Tyriar@users.noreply.github.com>

Tyriar

Red CI

src/vs/workbench/contrib/terminal/browser/terminalInstance.ts

Use value from setting

9559fab

anthonykim1 self-assigned this Oct 20, 2025

anthonykim1 added 4 commits October 20, 2025 16:33

more changes

32a00cd

comment

a937b42

clean up comments for code block review

3089880

delete more comments

1513957

anthonykim1 commented Oct 21, 2025

View reviewed changes

src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/toolTerminalCreator.ts Outdated Show resolved Hide resolved

anthonykim1 commented Oct 21, 2025

View reviewed changes

...rkbench/contrib/terminalContrib/chatAgentTools/common/terminalChatAgentToolsConfiguration.ts Outdated Show resolved Hide resolved

anthonykim1 commented Oct 21, 2025

View reviewed changes

src/vs/workbench/contrib/terminal/browser/terminalCommands.ts Outdated Show resolved Hide resolved

anthonykim1 added 4 commits October 20, 2025 23:18

more cleanup

33209c8

await in front of timeout is not needed?

810112d

Command detection isnt enough to prevent dup UI

dcded23

Use processReadyTime from terminalprocessManager

cf42953

anthonykim1 requested a review from Tyriar October 21, 2025 06:51

anthonykim1 marked this pull request as ready for review October 21, 2025 06:51

Copilot AI review requested due to automatic review settings October 21, 2025 06:51

Copilot AI reviewed Oct 21, 2025

View reviewed changes

vs-code-engineering bot added this to the October 2025 milestone Oct 21, 2025

Tyriar requested changes Oct 21, 2025

View reviewed changes

anthonykim1 marked this pull request as draft October 21, 2025 16:06

anthonykim1 added 8 commits October 21, 2025 09:08

timeoutValue instead of timeOutValue

d4cbe19

Use getValue<unknown> since we are doing full type checking

cd33508

Use isNumber instead of typeof

1f12e3b

Be smarter in else if, else

7c4b29d

Promise race with promptInputState.Input and remainder timeout consid…

21d509f

…ering processReadytime

Use markdownDeprecationMessage

80711dc

registerWorkbenchContribution for timeout migration

e46d70b

Try to match ToolTerminalCreator to runCommand in terminalInstance

3002ea0

anthonykim1 commented Oct 22, 2025

View reviewed changes

src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/toolTerminalCreator.ts Show resolved Hide resolved

comments

06b0bb8

anthonykim1 commented Oct 22, 2025

View reviewed changes

src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/toolTerminalCreator.ts Show resolved Hide resolved

anthonykim1 marked this pull request as ready for review October 22, 2025 07:09

anthonykim1 requested a review from Tyriar October 22, 2025 07:09

Tyriar requested changes Oct 22, 2025

View reviewed changes

anthonykim1 marked this pull request as draft October 22, 2025 17:07

anthonykim1 and others added 6 commits October 22, 2025 10:08

Update src/vs/workbench/contrib/terminal/common/terminalConfiguration.ts

ec365d3

Co-authored-by: Daniel Imms <2193314+Tyriar@users.noreply.github.com>

Take feedback on timeoutMigrationContribution

262be84

Reduce number of changes in toolTerminalCreator

55d3e16

Rename processReadyTime to processReadyTimestamp

7c1ac8f

Merge branch 'main' into anthonykim1/shellIntegrationTimeout

adb0e88

Remove registerTestRunCommandAction that was used for testing

2925d07

anthonykim1 marked this pull request as ready for review October 22, 2025 19:32

anthonykim1 requested a review from Tyriar October 22, 2025 19:32

Merge branch 'main' into anthonykim1/shellIntegrationTimeout

76b6983

Tyriar requested changes Oct 23, 2025

View reviewed changes

src/vs/workbench/contrib/terminal/browser/terminalInstance.ts Outdated Show resolved Hide resolved

anthonykim1 marked this pull request as draft October 23, 2025 16:44

anthonykim1 added 2 commits October 23, 2025 10:22

Fix missing bracket and comma

980c4c9

Move getShellIntegrationTimeout to terminalEnvironment

f87c039

anthonykim1 marked this pull request as ready for review October 23, 2025 18:11

anthonykim1 requested a review from Tyriar October 23, 2025 18:11

	const processReadyTime = this._processManager.processReadyTime;
	if (processReadyTime) {
	const elapsed = Date.now() - processReadyTime;
	timeoutMs = Math.max(0, timeoutMs - elapsed);
	}

	// Discuss: Talk about, get clarification one more time.
	await Promise.race([
	new Promise<void>(r => {
	store.add(this.capabilities.onDidAddCommandDetectionCapability(e => {
	commandDetection = e;
	if (commandDetection.promptInputModel.state === PromptInputState.Input) {
	r();
	} else {
	store.add(commandDetection.promptInputModel.onDidStartInput(() => {
	r();
	}));
	}
	}));
	}),
	timeout(timeoutMs)

Use unified timeout setting under shell integration in runCommand #272369

Are you sure you want to change the base?

Use unified timeout setting under shell integration in runCommand #272369

Conversation

anthonykim1 commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Tyriar Oct 21, 2025

Choose a reason for hiding this comment

Uh oh!

anthonykim1 Oct 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Tyriar Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

anthonykim1 Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Tyriar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

anthonykim1 commented Oct 20, 2025 •

edited

Loading