Skip to content

Commit 6e2b55c

Browse files
TyriarTyriar
committed
Clean up OutputMonitor
1 parent 2f4cf13 commit 6e2b55c

File tree

2 files changed

+30
-52
lines changed

2 files changed

+30
-52
lines changed

‎src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/outputMonitor.ts

Lines changed: 6 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -22,43 +22,30 @@ export const enum OutputMonitorAction {
2222
}
2323

2424
export interface IOutputMonitor extends Disposable {
25-
// for constructing telemetry event
2625
readonly actions: OutputMonitorAction[];
27-
28-
// for constructing tool result
2926
readonly isIdle: boolean;
3027

31-
// enable async listening to when the command is considered finished
3228
readonly onDidFinishCommand: Event<void>;
33-
34-
// if we need this outside the class
3529
readonly onDidIdle: Event<void>;
36-
37-
// timeout for showing elicitation
3830
readonly onDidTimeout: Event<void>;
3931

40-
// Start monitoring output with automatic extended polling if needed
4132
startMonitoring(
4233
chatService: IChatService,
4334
command: string,
4435
invocationContext: any,
4536
token: CancellationToken
4637
): Promise<{ terminalExecutionIdleBeforeTimeout: boolean; output: string; pollDurationMs?: number; modelOutputEvalResponse?: string }>;
47-
48-
// Legacy method for testing
49-
startMonitoringLegacy(extendedPolling: boolean, token: CancellationToken): Promise<{ terminalExecutionIdleBeforeTimeout: boolean; output: string; pollDurationMs?: number; modelOutputEvalResponse?: string }>;
5038
}
5139

5240
export class OutputMonitor extends Disposable implements IOutputMonitor {
53-
private readonly _onDidFinishCommand = new Emitter<void>();
54-
private readonly _onDidIdle = new Emitter<void>();
55-
private readonly _onDidTimeout = new Emitter<void>();
56-
5741
private readonly _actions: OutputMonitorAction[] = [];
5842
private _isIdle = false;
5943

44+
private readonly _onDidFinishCommand = this._register(new Emitter<void>());
6045
readonly onDidFinishCommand = this._onDidFinishCommand.event;
46+
private readonly _onDidIdle = this._register(new Emitter<void>());
6147
readonly onDidIdle = this._onDidIdle.event;
48+
private readonly _onDidTimeout = this._register(new Emitter<void>());
6249
readonly onDidTimeout = this._onDidTimeout.event;
6350

6451
get actions(): OutputMonitorAction[] {
@@ -82,13 +69,11 @@ export class OutputMonitor extends Disposable implements IOutputMonitor {
8269
invocationContext: any,
8370
token: CancellationToken
8471
): Promise<{ terminalExecutionIdleBeforeTimeout: boolean; output: string; pollDurationMs?: number; modelOutputEvalResponse?: string }> {
85-
// First, try initial polling
86-
let result = await this._startMonitoringInternal(false, token);
72+
let result = await this._startMonitoring(false, token);
8773

88-
// If the initial polling didn't complete before timeout, try extended polling with user prompt
8974
if (!result.terminalExecutionIdleBeforeTimeout) {
9075
result = await racePollingOrPrompt(
91-
() => this._startMonitoringInternal(true, token),
76+
() => this._startMonitoring(true, token),
9277
() => promptForMorePolling(command, token, invocationContext, chatService),
9378
result,
9479
token,
@@ -100,12 +85,7 @@ export class OutputMonitor extends Disposable implements IOutputMonitor {
10085
return result;
10186
}
10287

103-
// Legacy method for testing
104-
startMonitoringLegacy(extendedPolling: boolean, token: CancellationToken): Promise<{ terminalExecutionIdleBeforeTimeout: boolean; output: string; pollDurationMs?: number; modelOutputEvalResponse?: string }> {
105-
return this._startMonitoringInternal(extendedPolling, token);
106-
}
107-
108-
private async _startMonitoringInternal(extendedPolling: boolean, token: CancellationToken): Promise<{ terminalExecutionIdleBeforeTimeout: boolean; output: string; pollDurationMs?: number; modelOutputEvalResponse?: string }> {
88+
protected async _startMonitoring(extendedPolling: boolean, token: CancellationToken): Promise<{ terminalExecutionIdleBeforeTimeout: boolean; output: string; pollDurationMs?: number; modelOutputEvalResponse?: string }> {
10989
this._addAction(OutputMonitorAction.PollingStarted);
11090
if (extendedPolling) {
11191
this._addAction(OutputMonitorAction.ExtendedPollingStarted);
@@ -189,11 +169,4 @@ export class OutputMonitor extends Disposable implements IOutputMonitor {
189169
private _addAction(action: OutputMonitorAction): void {
190170
this._actions.push(action);
191171
}
192-
193-
override dispose(): void {
194-
this._onDidFinishCommand.dispose();
195-
this._onDidIdle.dispose();
196-
this._onDidTimeout.dispose();
197-
super.dispose();
198-
}
199172
}

‎src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/outputMonitor.test.ts

Lines changed: 24 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,17 @@
55

66
import { strictEqual, deepStrictEqual } from 'assert';
77
import { timeout } from '../../../../../../base/common/async.js';
8-
import { CancellationTokenSource } from '../../../../../../base/common/cancellation.js';
8+
import { CancellationTokenSource, type CancellationToken } from '../../../../../../base/common/cancellation.js';
99
import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../../base/test/common/utils.js';
1010
import { ILanguageModelsService } from '../../../../chat/common/languageModels.js';
1111
import { OutputMonitor, OutputMonitorAction } from '../../browser/outputMonitor.js';
1212

13+
class TestOutputMonitor extends OutputMonitor {
14+
async internalStartMonitoring(extendedPolling: boolean, token: CancellationToken): Promise<{ terminalExecutionIdleBeforeTimeout: boolean; output: string; pollDurationMs?: number; modelOutputEvalResponse?: string }> {
15+
return super._startMonitoring(extendedPolling, token);
16+
}
17+
}
18+
1319
suite('OutputMonitor', () => {
1420
const store = ensureNoDisposablesAreLeakedInTestSuite();
1521

@@ -51,7 +57,7 @@ suite('OutputMonitor', () => {
5157

5258
test('should implement IOutputMonitor interface', () => {
5359
const execution = createMockExecution(['']);
54-
const monitor = new OutputMonitor(execution, mockLanguageModelsService);
60+
const monitor = new TestOutputMonitor(execution, mockLanguageModelsService);
5561
store.add(monitor);
5662

5763
// Verify core interface properties exist
@@ -61,17 +67,16 @@ suite('OutputMonitor', () => {
6167
strictEqual(monitor.onDidIdle !== undefined, true);
6268
strictEqual(monitor.onDidTimeout !== undefined, true);
6369
strictEqual(monitor.startMonitoring !== undefined, true);
64-
strictEqual(monitor.startMonitoringLegacy !== undefined, true);
6570
strictEqual(monitor.dispose !== undefined, true);
6671
});
6772

6873
test('should track actions correctly', async () => {
6974
const execution = createMockExecution(['output1', 'output1', 'output1']); // No new output to trigger idle
70-
const monitor = new OutputMonitor(execution, mockLanguageModelsService);
75+
const monitor = new TestOutputMonitor(execution, mockLanguageModelsService);
7176
store.add(monitor);
7277

7378
const tokenSource = new CancellationTokenSource();
74-
const result = monitor.startMonitoringLegacy(false, tokenSource.token);
79+
const result = monitor.internalStartMonitoring(false, tokenSource.token);
7580

7681
// Give it some time to start and detect idle
7782
await timeout(100);
@@ -86,7 +91,7 @@ suite('OutputMonitor', () => {
8691

8792
test('should detect idle state when no new output', async () => {
8893
const execution = createMockExecution(['initial output', 'initial output', 'initial output']); // Same output to trigger idle
89-
const monitor = new OutputMonitor(execution, mockLanguageModelsService);
94+
const monitor = new TestOutputMonitor(execution, mockLanguageModelsService);
9095
store.add(monitor);
9196

9297
let idleEventFired = false;
@@ -104,7 +109,7 @@ suite('OutputMonitor', () => {
104109

105110
const tokenSource = new CancellationTokenSource();
106111

107-
const result = await monitor.startMonitoringLegacy(false, tokenSource.token);
112+
const result = await monitor.internalStartMonitoring(false, tokenSource.token);
108113

109114
strictEqual(result.terminalExecutionIdleBeforeTimeout, true);
110115
strictEqual(typeof result.output, 'string');
@@ -128,15 +133,15 @@ suite('OutputMonitor', () => {
128133
isActive: undefined
129134
};
130135

131-
const monitor = new OutputMonitor(execution, mockLanguageModelsService);
136+
const monitor = new TestOutputMonitor(execution, mockLanguageModelsService);
132137
store.add(monitor);
133138

134139
const tokenSource = new CancellationTokenSource();
135140

136141
// Cancel after a short time to simulate timeout
137142
setTimeout(() => tokenSource.cancel(), 100);
138143

139-
await monitor.startMonitoringLegacy(false, tokenSource.token);
144+
await monitor.internalStartMonitoring(false, tokenSource.token);
140145

141146
const actions = monitor.actions;
142147
strictEqual(actions.includes(OutputMonitorAction.PollingStarted), true);
@@ -145,7 +150,7 @@ suite('OutputMonitor', () => {
145150

146151
test('should handle cancellation correctly', async () => {
147152
const execution = createMockExecution(['output']);
148-
const monitor = new OutputMonitor(execution, mockLanguageModelsService);
153+
const monitor = new TestOutputMonitor(execution, mockLanguageModelsService);
149154
store.add(monitor);
150155

151156
const tokenSource = new CancellationTokenSource();
@@ -154,7 +159,7 @@ suite('OutputMonitor', () => {
154159
tokenSource.cancel();
155160

156161
try {
157-
await monitor.startMonitoringLegacy(false, tokenSource.token);
162+
await monitor.internalStartMonitoring(false, tokenSource.token);
158163
strictEqual(true, false, 'Should have thrown cancellation error');
159164
} catch (error) {
160165
// Expected cancellation
@@ -167,12 +172,12 @@ suite('OutputMonitor', () => {
167172

168173
test('should track output received actions', async () => {
169174
const execution = createMockExecution(['output1', 'output2', 'output2', 'output2']); // Output changes once then stays same
170-
const monitor = new OutputMonitor(execution, mockLanguageModelsService);
175+
const monitor = new TestOutputMonitor(execution, mockLanguageModelsService);
171176
store.add(monitor);
172177

173178
const tokenSource = new CancellationTokenSource();
174179

175-
const result = await monitor.startMonitoringLegacy(false, tokenSource.token);
180+
const result = await monitor.internalStartMonitoring(false, tokenSource.token);
176181

177182
strictEqual(result.terminalExecutionIdleBeforeTimeout, true);
178183

@@ -184,12 +189,12 @@ suite('OutputMonitor', () => {
184189

185190
test('should handle extended polling correctly', async () => {
186191
const execution = createMockExecution(['output', 'output', 'output']); // Same output to trigger idle quickly
187-
const monitor = new OutputMonitor(execution, mockLanguageModelsService);
192+
const monitor = new TestOutputMonitor(execution, mockLanguageModelsService);
188193
store.add(monitor);
189194

190195
const tokenSource = new CancellationTokenSource();
191196

192-
const result = await monitor.startMonitoringLegacy(true, tokenSource.token); // Extended polling = true
197+
const result = await monitor.internalStartMonitoring(true, tokenSource.token); // Extended polling = true
193198

194199
strictEqual(result.terminalExecutionIdleBeforeTimeout, true);
195200
strictEqual(typeof result.modelOutputEvalResponse, 'string');
@@ -206,15 +211,15 @@ suite('OutputMonitor', () => {
206211
['output', 'output', 'output', 'output'], // Same output to trigger no new data
207212
[true, false] // Active once, then inactive (should trigger idle quickly)
208213
);
209-
const monitor = new OutputMonitor(execution, mockLanguageModelsService);
214+
const monitor = new TestOutputMonitor(execution, mockLanguageModelsService);
210215
store.add(monitor);
211216

212217
const tokenSource = new CancellationTokenSource();
213218

214219
// Set a timeout to cancel if it takes too long
215220
setTimeout(() => tokenSource.cancel(), 2000);
216221

217-
await monitor.startMonitoringLegacy(false, tokenSource.token);
222+
await monitor.internalStartMonitoring(false, tokenSource.token);
218223

219224
// Check that it didn't timeout (either completed successfully or was cancelled)
220225
const actions = monitor.actions;
@@ -227,7 +232,7 @@ suite('OutputMonitor', () => {
227232

228233
test('should return immutable copy of actions', () => {
229234
const execution = createMockExecution(['']);
230-
const monitor = new OutputMonitor(execution, mockLanguageModelsService);
235+
const monitor = new TestOutputMonitor(execution, mockLanguageModelsService);
231236
store.add(monitor);
232237

233238
const actions1 = monitor.actions;
@@ -247,7 +252,7 @@ suite('OutputMonitor', () => {
247252

248253
test('should dispose correctly', () => {
249254
const execution = createMockExecution(['']);
250-
const monitor = new OutputMonitor(execution, mockLanguageModelsService);
255+
const monitor = new TestOutputMonitor(execution, mockLanguageModelsService);
251256

252257
let eventFired = false;
253258
const disposable = monitor.onDidFinishCommand(() => {

0 commit comments

Comments
 (0)