Move strategy creation into active execution

This commit is contained in:
Daniel Imms
2026-01-26 01:20:16 -08:00
parent 67f58d0103
commit 1d8ae27e4c
5 changed files with 57 additions and 48 deletions

View File

@@ -6,7 +6,7 @@
import type { CancellationToken } from '../../../../../../base/common/cancellation.js';
import { CancellationError } from '../../../../../../base/common/errors.js';
import { Emitter, Event } from '../../../../../../base/common/event.js';
import { DisposableStore, MutableDisposable } from '../../../../../../base/common/lifecycle.js';
import { Disposable, DisposableStore, MutableDisposable } from '../../../../../../base/common/lifecycle.js';
import { isNumber } from '../../../../../../base/common/types.js';
import type { ICommandDetectionCapability } from '../../../../../../platform/terminal/common/capabilities/capabilities.js';
import { ITerminalLogService } from '../../../../../../platform/terminal/common/terminal.js';
@@ -37,11 +37,11 @@ import { createAltBufferPromise, setupRecreatingStartMarker } from './strategyHe
* output. We lean on the LLM to be able to differentiate the actual output from prompts and bad
* output when it's not ideal.
*/
export class BasicExecuteStrategy implements ITerminalExecuteStrategy {
export class BasicExecuteStrategy extends Disposable implements ITerminalExecuteStrategy {
readonly type = 'basic';
private readonly _startMarker = new MutableDisposable<IXtermMarker>();
private readonly _startMarker = this._register(new MutableDisposable<IXtermMarker>());
private readonly _onDidCreateStartMarker = new Emitter<IXtermMarker | undefined>;
private readonly _onDidCreateStartMarker = this._register(new Emitter<IXtermMarker | undefined>);
public onDidCreateStartMarker: Event<IXtermMarker | undefined> = this._onDidCreateStartMarker.event;
@@ -51,6 +51,7 @@ export class BasicExecuteStrategy implements ITerminalExecuteStrategy {
private readonly _commandDetection: ICommandDetectionCapability,
@ITerminalLogService private readonly _logService: ITerminalLogService,
) {
super();
}
async execute(commandLine: string, token: CancellationToken, commandId?: string): Promise<ITerminalExecuteStrategyResult> {

View File

@@ -6,11 +6,11 @@
import { DeferredPromise, RunOnceScheduler } from '../../../../../../base/common/async.js';
import type { CancellationToken } from '../../../../../../base/common/cancellation.js';
import type { Event } from '../../../../../../base/common/event.js';
import { DisposableStore } from '../../../../../../base/common/lifecycle.js';
import { DisposableStore, type IDisposable } from '../../../../../../base/common/lifecycle.js';
import type { ITerminalInstance } from '../../../../terminal/browser/terminal.js';
import type { IMarker as IXtermMarker } from '@xterm/xterm';
export interface ITerminalExecuteStrategy {
export interface ITerminalExecuteStrategy extends IDisposable {
readonly type: 'rich' | 'basic' | 'none';
/**
* Executes a command line and gets a result designed to be passed directly to an LLM. The

View File

@@ -6,7 +6,7 @@
import type { CancellationToken } from '../../../../../../base/common/cancellation.js';
import { CancellationError } from '../../../../../../base/common/errors.js';
import { Emitter, Event } from '../../../../../../base/common/event.js';
import { DisposableStore, MutableDisposable } from '../../../../../../base/common/lifecycle.js';
import { Disposable, DisposableStore, MutableDisposable } from '../../../../../../base/common/lifecycle.js';
import { ITerminalLogService } from '../../../../../../platform/terminal/common/terminal.js';
import { waitForIdle, waitForIdleWithPromptHeuristics, type ITerminalExecuteStrategy, type ITerminalExecuteStrategyResult } from './executeStrategy.js';
import type { IMarker as IXtermMarker } from '@xterm/xterm';
@@ -19,12 +19,12 @@ import { createAltBufferPromise, setupRecreatingStartMarker } from './strategyHe
* with `sendText` instead of `shellIntegration.executeCommand` and relying on idle events instead
* of execution events.
*/
export class NoneExecuteStrategy implements ITerminalExecuteStrategy {
export class NoneExecuteStrategy extends Disposable implements ITerminalExecuteStrategy {
readonly type = 'none';
private readonly _startMarker = new MutableDisposable<IXtermMarker>();
private readonly _startMarker = this._register(new MutableDisposable<IXtermMarker>());
private readonly _onDidCreateStartMarker = new Emitter<IXtermMarker | undefined>;
private readonly _onDidCreateStartMarker = this._register(new Emitter<IXtermMarker | undefined>);
public onDidCreateStartMarker: Event<IXtermMarker | undefined> = this._onDidCreateStartMarker.event;
constructor(
@@ -32,6 +32,7 @@ export class NoneExecuteStrategy implements ITerminalExecuteStrategy {
private readonly _hasReceivedUserInput: () => boolean,
@ITerminalLogService private readonly _logService: ITerminalLogService,
) {
super();
}
async execute(commandLine: string, token: CancellationToken, commandId?: string): Promise<ITerminalExecuteStrategyResult> {

View File

@@ -6,7 +6,7 @@
import type { CancellationToken } from '../../../../../../base/common/cancellation.js';
import { CancellationError } from '../../../../../../base/common/errors.js';
import { Emitter, Event } from '../../../../../../base/common/event.js';
import { DisposableStore, MutableDisposable } from '../../../../../../base/common/lifecycle.js';
import { Disposable, DisposableStore, MutableDisposable } from '../../../../../../base/common/lifecycle.js';
import { isNumber } from '../../../../../../base/common/types.js';
import type { ICommandDetectionCapability } from '../../../../../../platform/terminal/common/capabilities/capabilities.js';
import { ITerminalLogService } from '../../../../../../platform/terminal/common/terminal.js';
@@ -22,11 +22,11 @@ import { createAltBufferPromise, setupRecreatingStartMarker } from './strategyHe
* wrong in this state, minimal verification is done in this mode since rich command detection is a
* strong signal that it's behaving correctly.
*/
export class RichExecuteStrategy implements ITerminalExecuteStrategy {
export class RichExecuteStrategy extends Disposable implements ITerminalExecuteStrategy {
readonly type = 'rich';
private readonly _startMarker = new MutableDisposable<IXtermMarker>();
private readonly _startMarker = this._register(new MutableDisposable<IXtermMarker>());
private readonly _onDidCreateStartMarker = new Emitter<IXtermMarker | undefined>;
private readonly _onDidCreateStartMarker = this._register(new Emitter<IXtermMarker | undefined>);
public onDidCreateStartMarker: Event<IXtermMarker | undefined> = this._onDidCreateStartMarker.event;
constructor(
@@ -34,6 +34,7 @@ export class RichExecuteStrategy implements ITerminalExecuteStrategy {
private readonly _commandDetection: ICommandDetectionCapability,
@ITerminalLogService private readonly _logService: ITerminalLogService,
) {
super();
}
async execute(commandLine: string, token: CancellationToken, commandId?: string): Promise<ITerminalExecuteStrategyResult> {

View File

@@ -724,25 +724,24 @@ export class RunInTerminalTool extends Disposable implements IToolImpl {
let executionPromise: Promise<ITerminalExecuteStrategyResult> | undefined;
try {
const strategy = this._getExecuteStrategy(toolTerminal.shellIntegrationQuality, toolTerminal, commandDetection!);
// Create unified ActiveTerminalExecution (creates and owns the strategy)
const execution = new ActiveTerminalExecution(
this._instantiationService,
chatSessionId,
termId,
toolTerminal,
commandDetection!,
args.isBackground
);
if (toolTerminal.shellIntegrationQuality === ShellIntegrationQuality.None) {
toolResultMessage = '$(info) Enable [shell integration](https://code.visualstudio.com/docs/terminal/shell-integration) to improve command detection';
}
this._logService.debug(`RunInTerminalTool: Using \`${strategy.type}\` execute strategy for command \`${command}\``);
// Create unified ActiveTerminalExecution
const execution = new ActiveTerminalExecution(
toolTerminal.instance,
chatSessionId,
termId,
strategy,
args.isBackground
);
// Don't add execution to store - its lifecycle is managed separately via _activeExecutions
this._logService.debug(`RunInTerminalTool: Using \`${execution.strategy.type}\` execute strategy for command \`${command}\``);
store.add(execution);
RunInTerminalTool._activeExecutions.set(termId, execution);
// Set up OutputMonitor when start marker is created
store.add(strategy.onDidCreateStartMarker(startMarker => {
store.add(execution.strategy.onDidCreateStartMarker(startMarker => {
if (!outputMonitor) {
outputMonitor = store.add(this._instantiationService.createInstance(
OutputMonitor,
@@ -853,7 +852,7 @@ export class RunInTerminalTool extends Disposable implements IToolImpl {
toolSpecificData.terminalCommandState = state;
}
this._logService.debug(`RunInTerminalTool: Finished \`${strategy.type}\` execute strategy with exitCode \`${executeResult.exitCode}\`, result.length \`${executeResult.output?.length}\`, error \`${executeResult.error}\``);
this._logService.debug(`RunInTerminalTool: Finished \`${execution.strategy.type}\` execute strategy with exitCode \`${executeResult.exitCode}\`, result.length \`${executeResult.output?.length}\`, error \`${executeResult.error}\``);
outputLineCount = executeResult.output === undefined ? 0 : count(executeResult.output.trim(), '\n') + 1;
exitCode = executeResult.exitCode;
error = executeResult.error;
@@ -1131,22 +1130,6 @@ export class RunInTerminalTool extends Disposable implements IToolImpl {
}
}
}
private _getExecuteStrategy(shellIntegrationQuality: ShellIntegrationQuality, toolTerminal: IToolTerminal, commandDetection: ICommandDetectionCapability): ITerminalExecuteStrategy {
let strategy: ITerminalExecuteStrategy;
switch (shellIntegrationQuality) {
case ShellIntegrationQuality.None:
strategy = this._instantiationService.createInstance(NoneExecuteStrategy, toolTerminal.instance, () => toolTerminal.receivedUserInput ?? false);
break;
case ShellIntegrationQuality.Basic:
strategy = this._instantiationService.createInstance(BasicExecuteStrategy, toolTerminal.instance, () => toolTerminal.receivedUserInput ?? false, commandDetection!);
break;
case ShellIntegrationQuality.Rich:
strategy = this._instantiationService.createInstance(RichExecuteStrategy, toolTerminal.instance, commandDetection!);
break;
}
return strategy;
}
// #endregion
}
@@ -1176,18 +1159,30 @@ class ActiveTerminalExecution extends Disposable {
return this._startMarker;
}
readonly strategy: ITerminalExecuteStrategy;
private readonly _toolTerminal: IToolTerminal;
get instance(): ITerminalInstance {
return this._toolTerminal.instance;
}
constructor(
readonly instance: ITerminalInstance,
instantiationService: IInstantiationService,
readonly sessionId: string,
readonly termId: string,
private readonly _strategy: ITerminalExecuteStrategy,
toolTerminal: IToolTerminal,
commandDetection: ICommandDetectionCapability,
isBackground: boolean,
) {
super();
this._toolTerminal = toolTerminal;
this._isBackground = isBackground;
this._completionDeferred = new DeferredPromise<ITerminalExecuteStrategyResult>();
this._register(this._strategy.onDidCreateStartMarker(marker => {
// Create and register the strategy for disposal to clean up its internal resources
this.strategy = this._register(this._createStrategy(instantiationService, commandDetection));
this._register(this.strategy.onDidCreateStartMarker(marker => {
if (marker) {
// Don't register marker - strategy already manages its lifecycle
this._startMarker = marker;
@@ -1195,6 +1190,17 @@ class ActiveTerminalExecution extends Disposable {
}));
}
private _createStrategy(instantiationService: IInstantiationService, commandDetection: ICommandDetectionCapability): ITerminalExecuteStrategy {
switch (this._toolTerminal.shellIntegrationQuality) {
case ShellIntegrationQuality.None:
return instantiationService.createInstance(NoneExecuteStrategy, this._toolTerminal.instance, () => this._toolTerminal.receivedUserInput ?? false);
case ShellIntegrationQuality.Basic:
return instantiationService.createInstance(BasicExecuteStrategy, this._toolTerminal.instance, () => this._toolTerminal.receivedUserInput ?? false, commandDetection);
case ShellIntegrationQuality.Rich:
return instantiationService.createInstance(RichExecuteStrategy, this._toolTerminal.instance, commandDetection);
}
}
/**
* Starts the command execution using the execute strategy.
* @param commandLine The command to execute
@@ -1204,7 +1210,7 @@ class ActiveTerminalExecution extends Disposable {
*/
async start(commandLine: string, token: CancellationToken, commandId?: string): Promise<ITerminalExecuteStrategyResult> {
try {
const result = await this._strategy.execute(commandLine, token, commandId);
const result = await this.strategy.execute(commandLine, token, commandId);
this._completionDeferred.complete(result);
return result;
} catch (e) {