fix: preserve shell tool output after scrollback rollover by atharvasingh7007 · Pull Request #310104 · microsoft/vscode

atharvasingh7007 · 2026-04-15T09:30:40Z

Summary

preserve captured shell output when the terminal scrollback trims away the original offset
apply the same trim-safe behavior to sentinel detection, timeout handling, and unexpected shell exit handling
add regression coverage for sentinel completion and timeout results after scrollback rollover

Testing

node --loader ts-node/esm ./node_modules/mocha/bin/mocha src/vs/platform/agentHost/test/node/copilotShellTools.test.ts --ui tdd --timeout 10000
node build/eslint.ts src/vs/platform/agentHost/node/copilot/copilotShellTools.ts src/vs/platform/agentHost/test/node/copilotShellTools.test.ts

atharvasingh7007 · 2026-04-15T09:35:11Z

@microsoft-github-policy-service agree

afurm

The extracted getContentSinceOffset helper is cleaner than the inline Math.min clamp.

Edge case: when offsetBefore > fullContent.length, returning the entire fullContent is correct for preserving output. But if offsetBefore is invalid (negative or NaN), the condition offsetBefore > fullContent.length would also be true and would silently return the full buffer. Should getContentSinceOffset validate that offsetBefore is a non-negative number first?

atharvasingh7007 · 2026-04-15T11:17:54Z

Addressed the offset validation edge case in �914b69: getContentSinceOffset() now clamps invalid offsets before slicing, and I added a focused unit assertion covering NaN, negative, oversized, and normal offsets.

Copilot

Pull request overview

This PR aims to make Copilot’s PTY-backed shell tool more resilient when terminal scrollback trims older output, so tool results (including sentinel detection / timeout / unexpected exit) still return meaningful content.

Changes:

Introduces trim-safe output slicing via getContentSinceOffset and applies it to sentinel/timeout/exit paths.
Extends ShellManager shell creation behavior (working directory defaulting, history suppression prefixing, additional terminal options, terminal association event).
Adds a new unit test file covering working directory behavior, history suppression prefixing, and getContentSinceOffset.

Reviewed changes

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

File	Description
`src/vs/platform/agentHost/node/copilot/copilotShellTools.ts`	Adds trim-safe content handling, shell-integration execution path, history suppression helpers, and updates tool descriptions/options.
`src/vs/platform/agentHost/test/node/copilotShellTools.test.ts`	Adds unit tests for ShellManager terminal creation behavior and `getContentSinceOffset`.

Copilot · 2026-04-25T21:34:30Z

+	test('getContentSinceOffset keeps current content when the saved offset is no longer valid', () => {
+		assert.strictEqual(getContentSinceOffset('hello', Number.NaN), 'hello');
+		assert.strictEqual(getContentSinceOffset('hello', -1), 'hello');
+		assert.strictEqual(getContentSinceOffset('hello', 99), 'hello');
+		assert.strictEqual(getContentSinceOffset('hello', 2), 'llo');
+	});


The PR description mentions regression coverage for sentinel completion + timeout behavior after scrollback rollover, but this test file currently only unit-tests getContentSinceOffset. Consider adding a higher-level test that simulates trimmed scrollback and asserts the tool result still includes the remaining output for both the sentinel completion path and the timeout path.

Copilot · 2026-04-25T21:34:30Z

@@ -103,8 +108,8 @@
 			terminal: terminalUri,
 			claim,
 			name: shellDisplayName,
-			cwd,
-		}, { shell: getShellExecutable(shellType) });
+			cwd: cwd ?? this._workingDirectory?.fsPath,
+		}, { shell: getShellExecutable(shellType), preventShellHistory: true, nonInteractive: true });


IAgentHostTerminalManager.createTerminal currently types options as { shell?: string }. Passing an object literal with preventShellHistory / nonInteractive here will fail excess-property checking unless the interface/type is updated. Please extend the terminal manager API (and downstream implementation) to accept these flags, rather than relying on an untyped extra property.

Copilot · 2026-04-25T21:34:30Z

+
 	constructor(
 		private readonly _sessionUri: URI,
+		private readonly _workingDirectory: URI | undefined,


ShellManager now has a new required ctor argument (_workingDirectory), but there are existing call sites that still instantiate it with only sessionUri (e.g. copilotAgent.ts). Either make this parameter optional (workingDirectory?: URI) / give it a default, or update all call sites accordingly; otherwise this will break TypeScript compilation.

Suggested change

private readonly _workingDirectory: URI | undefined,

private readonly _workingDirectory?: URI,

Copilot · 2026-04-25T21:34:30Z


 	// PTY input uses \r for line endings — the PTY translates to \r\n
-	const input = `${command}\r${sentinelCmd}\r`;
+	const input = `${prefixForHistorySuppression(shell.shellType)}${command}\r${sentinelCmd}\r`;


Only the user command is prefixed for history suppression, but the injected sentinel command is not. For shells using HISTCONTROL=ignorespace, the sentinel will still be recorded in history; consider prefixing the sentinel line as well so preventShellHistory is consistently honored.

Suggested change

const input = `${prefixForHistorySuppression(shell.shellType)}${command}\r${sentinelCmd}\r`;

const historySuppressionPrefix = prefixForHistorySuppression(shell.shellType);

const input = `${historySuppressionPrefix}${command}\r${historySuppressionPrefix}${sentinelCmd}\r`;

Copilot · 2026-04-25T21:34:31Z

 		name: shellType,
-		description: `Execute a command in a persistent ${shellType} shell. The shell is reused across calls.`,
+		description: shellType === 'bash' ? createBashModelDescription(false) : createPowerShellModelDescription(shellType, 'pwsh.exe', false),
 		parameters: {


The PowerShell tool description is generated with shellPath='pwsh.exe', but getShellExecutable('powershell') actually launches powershell.exe. This makes isWindowsPowerShell() mis-detect and can lead the model to use && even though Windows PowerShell 5.1 doesn't support it. Please pass the actual shell executable/path being used (and consider making isWindowsPowerShell robust to bare filenames/case).

Copilot · 2026-04-25T21:34:31Z

+	const result = terminalManager.supportsCommandDetection(shell.terminalUri)
+		? await executeCommandWithShellIntegration(shell, command, timeoutMs, terminalManager, logService)
+		: await executeCommandWithSentinel(shell, command, timeoutMs, terminalManager, logService);
+	return {


executeCommandInShell calls terminalManager.supportsCommandDetection(...) / onCommandFinished(...), but IAgentHostTerminalManager (as currently defined in node/agentHostTerminalManager.ts) doesn't declare these members. This will fail type-checking unless the terminal manager interface + implementation are extended accordingly (or these calls are guarded behind a type that includes them).

Copilot · 2026-04-25T21:34:31Z

+import { Emitter, Event } from '../../../../base/common/event.js';
 import { ILogService } from '../../../log/common/log.js';
-import { TerminalClaimKind, type ITerminalSessionClaim } from '../../common/state/protocol/state.js';
+import { TerminalClaimKind, type TerminalSessionClaim } from '../../common/state/protocol/state.js';


TerminalSessionClaim doesn't appear to be exported from .../state/protocol/state.ts (it exports ITerminalSessionClaim). As-is this import and the claim annotation below will fail to compile; please switch back to ITerminalSessionClaim or add an exported alias in the protocol typings.

Suggested change

import { TerminalClaimKind, type TerminalSessionClaim } from '../../common/state/protocol/state.js';

import { TerminalClaimKind, type ITerminalSessionClaim as TerminalSessionClaim } from '../../common/state/protocol/state.js';

Copilot · 2026-04-25T21:34:31Z

+	private readonly _onDidAssociateTerminal = new Emitter<{ toolCallId: string; terminalUri: string; displayName: string }>();
+	readonly onDidAssociateTerminal: Event<{ toolCallId: string; terminalUri: string; displayName: string }> = this._onDidAssociateTerminal.event;
+


The new _onDidAssociateTerminal emitter should be disposed when ShellManager is disposed (otherwise listeners can leak). Consider having ShellManager extend Disposable and _register the emitter, or call this._onDidAssociateTerminal.dispose() from dispose().

Copilot · 2026-04-25T21:34:31Z

+import type { CreateTerminalParams } from '../../common/state/protocol/commands.js';
+import type { TerminalClaim, TerminalInfo } from '../../common/state/protocol/state.js';
+import { IAgentHostTerminalManager } from '../../node/agentHostTerminalManager.js';


state/protocol/state.ts exports ITerminalClaim and ITerminalInfo (with the I prefix). Importing TerminalClaim / TerminalInfo here will fail type-checking unless aliases exist; please update the type imports to match the protocol exports.

fix: preserve shell tool output after scrollback rollover

8e6df59

This comment was marked as outdated.

Sign in to view

afurm reviewed Apr 15, 2026

View reviewed changes

vs-code-engineering Bot added the triage-needed label Apr 15, 2026

vs-code-engineering Bot assigned Yoyokrazy Apr 15, 2026

fix: clamp invalid shell output offsets

b914b69

atharvasingh7007 and others added 2 commits April 15, 2026 20:25

Merge branch 'main' into fix/shell-tool-trimmed-output

d8956b3

fix: resolve shell tool output conflicts

3812477

Copilot AI review requested due to automatic review settings April 25, 2026 21:26

Copilot started reviewing on behalf of atharvasingh7007 April 25, 2026 21:28 View session

atharvasingh7007 added 2 commits April 26, 2026 03:02

test: avoid shell tool test merge conflict

c12f4eb

fix: keep shell output patch minimal

7713e0f

Copilot AI reviewed Apr 25, 2026

View reviewed changes

test: guard shell offset regression test

5ed3756

	private readonly _workingDirectory: URI \| undefined,
	private readonly _workingDirectory?: URI,

	const input = `${prefixForHistorySuppression(shell.shellType)}${command}\r${sentinelCmd}\r`;
	const historySuppressionPrefix = prefixForHistorySuppression(shell.shellType);
	const input = `${historySuppressionPrefix}${command}\r${historySuppressionPrefix}${sentinelCmd}\r`;

	import { TerminalClaimKind, type TerminalSessionClaim } from '../../common/state/protocol/state.js';
	import { TerminalClaimKind, type ITerminalSessionClaim as TerminalSessionClaim } from '../../common/state/protocol/state.js';

		private readonly _onDidAssociateTerminal = new Emitter<{ toolCallId: string; terminalUri: string; displayName: string }>();
		readonly onDidAssociateTerminal: Event<{ toolCallId: string; terminalUri: string; displayName: string }> = this._onDidAssociateTerminal.event;

Conversation

atharvasingh7007 commented Apr 15, 2026

Summary

Testing

Uh oh!

atharvasingh7007 commented Apr 15, 2026

Uh oh!

This comment was marked as outdated.

Uh oh!

afurm left a comment

Choose a reason for hiding this comment

Uh oh!

atharvasingh7007 commented Apr 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants