Skip to content

Commit 1da1332

Browse files
DonJayamanneDonJayamanne
committed
More Copilot CLI tests and refactor permissions
1 parent e982499 commit 1da1332

File tree

9 files changed

+772
-301
lines changed

9 files changed

+772
-301
lines changed

‎src/extension/agents/copilotcli/node/copilotCli.ts‎

Lines changed: 34 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,19 @@ import { ILogService } from '../../../../platform/log/common/logService';
1212
import { IWorkspaceService } from '../../../../platform/workspace/common/workspaceService';
1313
import { createServiceIdentifier } from '../../../../util/common/services';
1414
import { Lazy } from '../../../../util/vs/base/common/lazy';
15-
import { Disposable, IDisposable, toDisposable } from '../../../../util/vs/base/common/lifecycle';
15+
import { IDisposable, toDisposable } from '../../../../util/vs/base/common/lifecycle';
1616
import { getCopilotLogger } from './logger';
1717
import { ensureNodePtyShim } from './nodePtyShim';
18+
import { PermissionRequest } from './permissionHelpers';
1819

1920
const COPILOT_CLI_MODEL_MEMENTO_KEY = 'github.copilot.cli.sessionModel';
2021
const DEFAULT_CLI_MODEL = 'claude-sonnet-4';
2122

23+
export interface CopilotCLISessionOptions {
24+
addPermissionHandler(handler: SessionOptions['requestPermission']): IDisposable;
25+
toSessionOptions(): SessionOptions;
26+
}
27+
2228
export interface ICopilotCLIModels {
2329
_serviceBrand: undefined;
2430
toModelProvider(modelId: string): string;
@@ -104,9 +110,8 @@ export class CopilotCLISDK implements ICopilotCLISDK {
104110
export interface ICopilotCLISessionOptionsService {
105111
readonly _serviceBrand: undefined;
106112
createOptions(
107-
options: SessionOptions,
108-
permissionHandler: CopilotCLIPermissionsHandler
109-
): Promise<SessionOptions>;
113+
options: SessionOptions
114+
): Promise<CopilotCLISessionOptions>;
110115
}
111116
export const ICopilotCLISessionOptionsService = createServiceIdentifier<ICopilotCLISessionOptionsService>('ICopilotCLISessionOptionsService');
112117

@@ -118,30 +123,50 @@ export class CopilotCLISessionOptionsService implements ICopilotCLISessionOption
118123
@ILogService private readonly logService: ILogService,
119124
) { }
120125

121-
public async createOptions(options: SessionOptions, permissionHandler: CopilotCLIPermissionsHandler) {
126+
public async createOptions(options: SessionOptions) {
122127
const copilotToken = await this._authenticationService.getAnyGitHubSession();
123128
const workingDirectory = options.workingDirectory ?? await this.getWorkspaceFolderPath();
129+
const logger = this.logService;
130+
const requestPermissionRejected = async (permission: PermissionRequest): ReturnType<NonNullable<SessionOptions['requestPermission']>> => {
131+
logger.info(`[CopilotCLISessionOptionsService] Permission request denied for permission as no handler was set: ${permission.kind}`);
132+
return {
133+
kind: "denied-interactively-by-user"
134+
};
135+
};
136+
const permissionHandler: Required<Pick<SessionOptions, 'requestPermission'>> = {
137+
requestPermission: requestPermissionRejected
138+
};
139+
124140
const allOptions: SessionOptions = {
125141
env: {
126142
...process.env,
127143
COPILOTCLI_DISABLE_NONESSENTIAL_TRAFFIC: '1'
128144
},
129145
logger: getCopilotLogger(this.logService),
130-
requestPermission: async (permissionRequest) => {
131-
return await permissionHandler.getPermissions(permissionRequest);
132-
},
133146
authInfo: {
134147
type: 'token',
135148
token: copilotToken?.accessToken ?? '',
136149
host: 'https://github.com'
137150
},
138151
...options,
152+
requestPermission: async (request: PermissionRequest) => {
153+
return await permissionHandler.requestPermission(request);
154+
}
139155
};
140156

141157
if (workingDirectory) {
142158
allOptions.workingDirectory = workingDirectory;
143159
}
144-
return allOptions;
160+
161+
return {
162+
addPermissionHandler: (handler: NonNullable<SessionOptions['requestPermission']>) => {
163+
permissionHandler.requestPermission = handler;
164+
return toDisposable(() => {
165+
permissionHandler.requestPermission = requestPermissionRejected;
166+
});
167+
},
168+
toSessionOptions: () => allOptions
169+
} satisfies CopilotCLISessionOptions;
145170
}
146171
private async getWorkspaceFolderPath() {
147172
if (this.workspaceService.getWorkspaceFolders().length === 0) {
@@ -154,32 +179,3 @@ export class CopilotCLISessionOptionsService implements ICopilotCLISessionOption
154179
return folder?.uri?.fsPath;
155180
}
156181
}
157-
158-
159-
/**
160-
* Perhaps temporary interface to handle permission requests from the Copilot CLI SDK
161-
* Perhaps because the SDK needs a better way to handle this in long term per session.
162-
*/
163-
export interface ICopilotCLIPermissions {
164-
onDidRequestPermissions(handler: SessionOptions['requestPermission']): IDisposable;
165-
}
166-
167-
export class CopilotCLIPermissionsHandler extends Disposable implements ICopilotCLIPermissions {
168-
private _handler: SessionOptions['requestPermission'] | undefined;
169-
170-
public onDidRequestPermissions(handler: SessionOptions['requestPermission']): IDisposable {
171-
this._handler = handler;
172-
return this._register(toDisposable(() => {
173-
this._handler = undefined;
174-
}));
175-
}
176-
177-
public async getPermissions(permission: Parameters<NonNullable<SessionOptions['requestPermission']>>[0]): Promise<ReturnType<NonNullable<SessionOptions['requestPermission']>>> {
178-
if (!this._handler) {
179-
return {
180-
kind: "denied-interactively-by-user"
181-
};
182-
}
183-
return await this._handler(permission);
184-
}
185-
}

‎src/extension/agents/copilotcli/node/copilotcliSession.ts‎

Lines changed: 83 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -3,35 +3,41 @@
33
* Licensed under the MIT License. See License.txt in the project root for license information.
44
*--------------------------------------------------------------------------------------------*/
55

6-
import type { Attachment, Session, SessionOptions } from '@github/copilot/sdk';
6+
import type { Attachment, Session } from '@github/copilot/sdk';
77
import type * as vscode from 'vscode';
88
import { ILogService } from '../../../../platform/log/common/logService';
99
import { IWorkspaceService } from '../../../../platform/workspace/common/workspaceService';
1010
import { CancellationToken } from '../../../../util/vs/base/common/cancellation';
11+
import { Emitter, Event } from '../../../../util/vs/base/common/event';
1112
import { DisposableStore, IDisposable, toDisposable } from '../../../../util/vs/base/common/lifecycle';
1213
import { ResourceMap } from '../../../../util/vs/base/common/map';
1314
import { extUriBiasedIgnorePathCase } from '../../../../util/vs/base/common/resources';
14-
import { ChatRequestTurn2, ChatResponseThinkingProgressPart, ChatResponseTurn2, ChatSessionStatus, EventEmitter, LanguageModelTextPart, Uri } from '../../../../vscodeTypes';
15-
import { IToolsService } from '../../../tools/common/toolsService';
15+
import { ChatRequestTurn2, ChatResponseThinkingProgressPart, ChatResponseTurn2, ChatSessionStatus, EventEmitter, Uri } from '../../../../vscodeTypes';
1616
import { ExternalEditTracker } from '../../common/externalEditTracker';
17-
import { CopilotCLIPermissionsHandler, ICopilotCLISessionOptionsService } from './copilotCli';
17+
import { CopilotCLISessionOptions, ICopilotCLISessionOptionsService } from './copilotCli';
1818
import { buildChatHistoryFromEvents, getAffectedUrisForEditTool, isCopilotCliEditToolCall, processToolExecutionComplete, processToolExecutionStart } from './copilotcliToolInvocationFormatter';
19-
import { getConfirmationToolParams, PermissionRequest } from './permissionHelpers';
19+
import { PermissionRequest } from './permissionHelpers';
20+
21+
type PermissionHandler = (
22+
permissionRequest: PermissionRequest,
23+
token: CancellationToken,
24+
) => Promise<boolean>;
2025

2126
export interface ICopilotCLISession extends IDisposable {
2227
readonly sessionId: string;
2328
readonly status: vscode.ChatSessionStatus | undefined;
2429
readonly onDidChangeStatus: vscode.Event<vscode.ChatSessionStatus | undefined>;
30+
readonly permissionRequested?: PermissionRequest;
31+
readonly onPermissionRequested: vscode.Event<PermissionRequest>;
2532

33+
attachPermissionHandler(handler: PermissionHandler): IDisposable;
34+
attchStream(stream: vscode.ChatResponseStream): IDisposable;
2635
handleRequest(
2736
prompt: string,
2837
attachments: Attachment[],
2938
modelId: string | undefined,
30-
stream: vscode.ChatResponseStream,
31-
toolInvocationToken: vscode.ChatParticipantToolToken,
3239
token: vscode.CancellationToken
3340
): Promise<void>;
34-
3541
addUserMessage(content: string): void;
3642
addUserAssistantMessage(content: string): void;
3743
getSelectedModelId(): Promise<string | undefined>;
@@ -49,25 +55,49 @@ export class CopilotCLISession extends DisposableStore implements ICopilotCLISes
4955

5056
public readonly onDidChangeStatus = this._statusChange.event;
5157

58+
private _permissionRequested?: PermissionRequest;
59+
public get permissionRequested(): PermissionRequest | undefined {
60+
return this._permissionRequested;
61+
}
62+
private readonly _onPermissionRequested = this.add(new EventEmitter<PermissionRequest>());
63+
public readonly onPermissionRequested = this._onPermissionRequested.event;
64+
private _permissionHandler?: PermissionHandler;
65+
private readonly _permissionHandlerSet = this.add(new Emitter<void>());
66+
private _stream?: vscode.ChatResponseStream;
5267
constructor(
68+
private readonly _options: CopilotCLISessionOptions,
5369
private readonly _sdkSession: Session,
54-
private readonly _options: SessionOptions,
55-
private readonly _permissionHandler: CopilotCLIPermissionsHandler,
5670
@ILogService private readonly logService: ILogService,
5771
@IWorkspaceService private readonly workspaceService: IWorkspaceService,
58-
@IToolsService private readonly toolsService: IToolsService,
5972
@ICopilotCLISessionOptionsService private readonly cliSessionOptions: ICopilotCLISessionOptionsService,
6073
) {
6174
super();
6275
this.sessionId = _sdkSession.sessionId;
6376
}
6477

78+
attchStream(stream: vscode.ChatResponseStream): IDisposable {
79+
this._stream = stream;
80+
return toDisposable(() => {
81+
if (this._stream === stream) {
82+
this._stream = undefined;
83+
}
84+
});
85+
}
86+
87+
attachPermissionHandler(handler: PermissionHandler): IDisposable {
88+
this._permissionHandler = handler;
89+
this._permissionHandlerSet.fire();
90+
return toDisposable(() => {
91+
if (this._permissionHandler === handler) {
92+
this._permissionHandler = undefined;
93+
}
94+
});
95+
}
96+
6597
public async handleRequest(
6698
prompt: string,
6799
attachments: Attachment[],
68100
modelId: string | undefined,
69-
stream: vscode.ChatResponseStream,
70-
toolInvocationToken: vscode.ChatParticipantToolToken,
71101
token: vscode.CancellationToken
72102
): Promise<void> {
73103
if (this.isDisposed) {
@@ -88,26 +118,26 @@ export class CopilotCLISession extends DisposableStore implements ICopilotCLISes
88118
const editToolIds = new Set<string>();
89119
const editTracker = new ExternalEditTracker();
90120
const editFilesAndToolCallIds = new ResourceMap<string[]>();
91-
disposables.add(this._permissionHandler.onDidRequestPermissions(async (permissionRequest) => {
92-
return await this.requestPermission(permissionRequest, stream, editTracker,
121+
disposables.add(this._options.addPermissionHandler(async (permissionRequest) => {
122+
// Need better API from SDK to correlate file edits in permission requests to tool invocations.
123+
return await this.requestPermission(permissionRequest, editTracker,
93124
(file: Uri) => {
94125
const ids = editFilesAndToolCallIds.get(file);
95126
return ids?.shift();
96127
},
97-
toolInvocationToken,
98-
this._options.workingDirectory
128+
this._options.toSessionOptions().workingDirectory,
129+
token
99130
);
100131
}));
101132

102133
try {
103-
const [currentModel,
104-
sessionOptions
105-
] = await Promise.all([
134+
const [currentModel, sessionOptions] = await Promise.all([
106135
modelId ? this._sdkSession.getSelectedModel() : undefined,
107-
this.cliSessionOptions.createOptions(this._options, this._permissionHandler)
136+
this.cliSessionOptions.createOptions({})
108137
]);
109-
if (sessionOptions.authInfo) {
110-
this._sdkSession.setAuthInfo(sessionOptions.authInfo);
138+
const autoInfo = sessionOptions.toSessionOptions().authInfo;
139+
if (autoInfo) {
140+
this._sdkSession.setAuthInfo(autoInfo);
111141
}
112142
if (modelId && modelId !== currentModel) {
113143
await this._sdkSession.setSelectedModel(modelId);
@@ -116,7 +146,7 @@ export class CopilotCLISession extends DisposableStore implements ICopilotCLISes
116146
disposables.add(toDisposable(this._sdkSession.on('*', (event) => this.logService.trace(`[CopilotCLISession]CopilotCLI Event: ${JSON.stringify(event, null, 2)}`))));
117147
disposables.add(toDisposable(this._sdkSession.on('assistant.message', (event) => {
118148
if (typeof event.data.content === 'string' && event.data.content.length) {
119-
stream.markdown(event.data.content);
149+
this._stream?.markdown(event.data.content);
120150
}
121151
})));
122152
disposables.add(toDisposable(this._sdkSession.on('tool.execution_start', (event) => {
@@ -136,7 +166,7 @@ export class CopilotCLISession extends DisposableStore implements ICopilotCLISes
136166
} else {
137167
const responsePart = processToolExecutionStart(event, this._pendingToolInvocations);
138168
if (responsePart instanceof ChatResponseThinkingProgressPart) {
139-
stream.push(responsePart);
169+
this._stream?.push(responsePart);
140170
}
141171
}
142172
this.logService.trace(`[CopilotCLISession] Start Tool ${event.data.toolName || '<unknown>'}`);
@@ -151,7 +181,7 @@ export class CopilotCLISession extends DisposableStore implements ICopilotCLISes
151181

152182
const responsePart = processToolExecutionComplete(event, this._pendingToolInvocations);
153183
if (responsePart && !(responsePart instanceof ChatResponseThinkingProgressPart)) {
154-
stream.push(responsePart);
184+
this._stream?.push(responsePart);
155185
}
156186

157187
const toolName = toolNames.get(event.data.toolCallId) || '<unknown>';
@@ -163,7 +193,7 @@ export class CopilotCLISession extends DisposableStore implements ICopilotCLISes
163193
})));
164194
disposables.add(toDisposable(this._sdkSession.on('session.error', (event) => {
165195
this.logService.error(`[CopilotCLISession]CopilotCLI error: (${event.data.errorType}), ${event.data.message}`);
166-
stream.markdown(`\n\n❌ Error: (${event.data.errorType}) ${event.data.message}`);
196+
this._stream?.markdown(`\n\n❌ Error: (${event.data.errorType}) ${event.data.message}`);
167197
})));
168198

169199
await this._sdkSession.send({ prompt, attachments, abortController });
@@ -175,7 +205,7 @@ export class CopilotCLISession extends DisposableStore implements ICopilotCLISes
175205
this._status = ChatSessionStatus.Failed;
176206
this._statusChange.fire(this._status);
177207
this.logService.error(`[CopilotCLISession] Invoking session (error) ${this.sessionId}`, error);
178-
stream.markdown(`\n\n❌ Error: ${error instanceof Error ? error.message : String(error)}`);
208+
this._stream?.markdown(`\n\n❌ Error: ${error instanceof Error ? error.message : String(error)}`);
179209
} finally {
180210
disposables.dispose();
181211
}
@@ -197,17 +227,16 @@ export class CopilotCLISession extends DisposableStore implements ICopilotCLISes
197227
}
198228

199229
public async getChatHistory(): Promise<(ChatRequestTurn2 | ChatResponseTurn2)[]> {
200-
const events = await this._sdkSession.getEvents();
230+
const events = this._sdkSession.getEvents();
201231
return buildChatHistoryFromEvents(events);
202232
}
203233

204234
private async requestPermission(
205235
permissionRequest: PermissionRequest,
206-
stream: vscode.ChatResponseStream,
207236
editTracker: ExternalEditTracker,
208237
getEditKeyForFile: (file: Uri) => string | undefined,
209-
toolInvocationToken: vscode.ChatParticipantToolToken,
210-
workingDirectory?: string
238+
workingDirectory: string | undefined,
239+
token: vscode.CancellationToken
211240
): Promise<{ kind: 'approved' } | { kind: 'denied-interactively-by-user' }> {
212241
if (permissionRequest.kind === 'read') {
213242
// If user is reading a file in the working directory or workspace, auto-approve
@@ -239,26 +268,40 @@ export class CopilotCLISession extends DisposableStore implements ICopilotCLISes
239268
}
240269

241270
try {
242-
const { tool, input } = getConfirmationToolParams(permissionRequest);
243-
const result = await this.toolsService.invokeTool(tool,
244-
{ input, toolInvocationToken },
245-
CancellationToken.None);
271+
const permissionHandler = await this.waitForPermissionHandler(permissionRequest);
272+
if (!permissionHandler) {
273+
this.logService.warn(`[CopilotCLISession] No permission handler registered, denying request for ${permissionRequest.kind} permission.`);
274+
return { kind: 'denied-interactively-by-user' };
275+
}
246276

247-
const firstResultPart = result.content.at(0);
248-
if (firstResultPart instanceof LanguageModelTextPart && firstResultPart.value === 'yes') {
277+
if (await permissionHandler(permissionRequest, token)) {
249278
// If we're editing a file, start tracking the edit & wait for core to acknowledge it.
250279
const editFile = permissionRequest.kind === 'write' ? Uri.file(permissionRequest.fileName) : undefined;
251280
const editKey = editFile ? getEditKeyForFile(editFile) : undefined;
252-
if (editFile && editKey) {
281+
if (editFile && editKey && this._stream) {
253282
this.logService.trace(`[CopilotCLISession] Starting to track edit for toolCallId ${editKey} & file ${editFile.fsPath}`);
254-
await editTracker.trackEdit(editKey, [editFile], stream);
283+
await editTracker.trackEdit(editKey, [editFile], this._stream);
255284
}
256285
return { kind: 'approved' };
257286
}
258287
} catch (error) {
259288
this.logService.error(`[CopilotCLISession] Permission request error: ${error}`);
289+
} finally {
290+
this._permissionRequested = undefined;
260291
}
261292

262293
return { kind: 'denied-interactively-by-user' };
263294
}
295+
296+
private async waitForPermissionHandler(permissionRequest: PermissionRequest): Promise<PermissionHandler | undefined> {
297+
if (!this._permissionHandler) {
298+
this._permissionRequested = permissionRequest;
299+
this._onPermissionRequested.fire(permissionRequest);
300+
const disposables = this.add(new DisposableStore());
301+
await Event.toPromise(this._permissionHandlerSet.event, disposables);
302+
disposables.dispose();
303+
this._permissionRequested = undefined;
304+
}
305+
return this._permissionHandler;
306+
}
264307
}

0 commit comments

Comments
 (0)