Skip to content

Commit 67df08b

Browse files
DonJayamanneDonJayamanne
authored
Remove Copilot CLI Session Options (#1894)
* Remove Copilot CLI Session Options * Fix tests * Fix tests * Updates * Fix tests * Dispose unwanted readonly sessions
1 parent 04d9df6 commit 67df08b

File tree

9 files changed

+161
-209
lines changed

9 files changed

+161
-209
lines changed

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

Lines changed: 58 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import { IAuthenticationService } from '../../../../platform/authentication/comm
99
import { IEnvService } from '../../../../platform/env/common/envService';
1010
import { IVSCodeExtensionContext } from '../../../../platform/extContext/common/extensionContext';
1111
import { ILogService } from '../../../../platform/log/common/logService';
12-
import { IWorkspaceService } from '../../../../platform/workspace/common/workspaceService';
1312
import { createServiceIdentifier } from '../../../../util/common/services';
1413
import { Lazy } from '../../../../util/vs/base/common/lazy';
1514
import { IDisposable, toDisposable } from '../../../../util/vs/base/common/lifecycle';
@@ -20,10 +19,56 @@ import { PermissionRequest } from './permissionHelpers';
2019
const COPILOT_CLI_MODEL_MEMENTO_KEY = 'github.copilot.cli.sessionModel';
2120
const DEFAULT_CLI_MODEL = 'claude-sonnet-4';
2221

23-
export interface CopilotCLISessionOptions {
24-
addPermissionHandler(handler: SessionOptions['requestPermission']): IDisposable;
25-
toSessionOptions(): SessionOptions;
26-
isolationEnabled: boolean;
22+
export class CopilotCLISessionOptions {
23+
public readonly isolationEnabled: boolean;
24+
public readonly workingDirectory?: string;
25+
private readonly model?: string;
26+
private readonly logger: ReturnType<typeof getCopilotLogger>;
27+
private readonly requestPermissionRejected: NonNullable<SessionOptions['requestPermission']>;
28+
private requestPermissionHandler: NonNullable<SessionOptions['requestPermission']>;
29+
constructor(options: { model?: string; isolationEnabled?: boolean; workingDirectory?: string }, logger: ILogService) {
30+
this.isolationEnabled = !!options.isolationEnabled;
31+
this.workingDirectory = options.workingDirectory;
32+
this.model = options.model;
33+
this.logger = getCopilotLogger(logger);
34+
this.requestPermissionRejected = async (permission: PermissionRequest): ReturnType<NonNullable<SessionOptions['requestPermission']>> => {
35+
logger.info(`[CopilotCLISession] Permission request denied for permission as no handler was set: ${permission.kind}`);
36+
return {
37+
kind: "denied-interactively-by-user"
38+
};
39+
};
40+
this.requestPermissionHandler = this.requestPermissionRejected;
41+
}
42+
43+
public addPermissionHandler(handler: NonNullable<SessionOptions['requestPermission']>): IDisposable {
44+
this.requestPermissionHandler = handler;
45+
return toDisposable(() => {
46+
if (this.requestPermissionHandler === handler) {
47+
this.requestPermissionHandler = this.requestPermissionRejected;
48+
}
49+
});
50+
}
51+
52+
public toSessionOptions(): Readonly<SessionOptions & { requestPermission: NonNullable<SessionOptions['requestPermission']> }> {
53+
const allOptions: SessionOptions = {
54+
env: {
55+
...process.env,
56+
COPILOTCLI_DISABLE_NONESSENTIAL_TRAFFIC: '1'
57+
},
58+
logger: this.logger,
59+
requestPermission: async (request: PermissionRequest) => {
60+
return await this.requestPermissionHandler(request);
61+
}
62+
};
63+
64+
if (this.workingDirectory) {
65+
allOptions.workingDirectory = this.workingDirectory;
66+
}
67+
if (this.model) {
68+
allOptions.model = this.model as unknown as SessionOptions['model'];
69+
}
70+
return allOptions as Readonly<SessionOptions & { requestPermission: NonNullable<SessionOptions['requestPermission']> }>;
71+
}
2772
}
2873

2974
export interface ICopilotCLIModels {
@@ -102,7 +147,7 @@ export class CopilotCLISDK implements ICopilotCLISDK {
102147
await this.ensureNodePtyShim();
103148
return await import('@github/copilot/sdk');
104149
} catch (error) {
105-
this.logService.error(`[CopilotCLISDK] Failed to load @github/copilot/sdk: ${error}`);
150+
this.logService.error(`[CopilotCLISession] Failed to load @github/copilot/sdk: ${error}`);
106151
throw error;
107152
}
108153
}
@@ -112,78 +157,11 @@ export class CopilotCLISDK implements ICopilotCLISDK {
112157
}
113158
}
114159

115-
export interface ICopilotCLISessionOptionsService {
116-
readonly _serviceBrand: undefined;
117-
createOptions(
118-
options: SessionOptions
119-
): Promise<CopilotCLISessionOptions>;
120-
}
121-
export const ICopilotCLISessionOptionsService = createServiceIdentifier<ICopilotCLISessionOptionsService>('ICopilotCLISessionOptionsService');
122-
123-
export class CopilotCLISessionOptionsService implements ICopilotCLISessionOptionsService {
124-
declare _serviceBrand: undefined;
125-
constructor(
126-
@IWorkspaceService private readonly workspaceService: IWorkspaceService,
127-
@IAuthenticationService private readonly _authenticationService: IAuthenticationService,
128-
@ILogService private readonly logService: ILogService,
129-
) { }
130-
131-
public async createOptions(options: SessionOptions) {
132-
const copilotToken = await this._authenticationService.getAnyGitHubSession();
133-
const workingDirectory = options.workingDirectory ?? await this.getWorkspaceFolderPath();
134-
const logger = this.logService;
135-
const requestPermissionRejected = async (permission: PermissionRequest): ReturnType<NonNullable<SessionOptions['requestPermission']>> => {
136-
logger.info(`[CopilotCLISessionOptionsService] Permission request denied for permission as no handler was set: ${permission.kind}`);
137-
return {
138-
kind: "denied-interactively-by-user"
139-
};
140-
};
141-
const permissionHandler: Required<Pick<SessionOptions, 'requestPermission'>> = {
142-
requestPermission: requestPermissionRejected
143-
};
144-
145-
const allOptions: SessionOptions = {
146-
env: {
147-
...process.env,
148-
COPILOTCLI_DISABLE_NONESSENTIAL_TRAFFIC: '1'
149-
},
150-
logger: getCopilotLogger(this.logService),
151-
requestPermission: async (request: PermissionRequest) => {
152-
return await permissionHandler.requestPermission(request);
153-
},
154-
authInfo: {
155-
type: 'token',
156-
token: copilotToken?.accessToken ?? '',
157-
host: 'https://github.com'
158-
},
159-
...options,
160-
};
161-
162-
if (workingDirectory) {
163-
allOptions.workingDirectory = workingDirectory;
164-
}
165-
166-
return {
167-
addPermissionHandler: (handler: NonNullable<SessionOptions['requestPermission']>) => {
168-
permissionHandler.requestPermission = handler;
169-
return toDisposable(() => {
170-
if (permissionHandler.requestPermission === handler) {
171-
permissionHandler.requestPermission = requestPermissionRejected;
172-
}
173-
});
174-
},
175-
toSessionOptions: () => allOptions,
176-
isolationEnabled: false
177-
} satisfies CopilotCLISessionOptions;
178-
}
179-
private async getWorkspaceFolderPath() {
180-
if (this.workspaceService.getWorkspaceFolders().length === 0) {
181-
return undefined;
182-
}
183-
if (this.workspaceService.getWorkspaceFolders().length === 1) {
184-
return this.workspaceService.getWorkspaceFolders()[0].fsPath;
185-
}
186-
const folder = await this.workspaceService.showWorkspaceFolderPicker();
187-
return folder?.uri?.fsPath;
188-
}
160+
export async function getAuthInfo(authentService: IAuthenticationService): Promise<SessionOptions['authInfo']> {
161+
const copilotToken = await authentService.getAnyGitHubSession();
162+
return {
163+
type: 'token',
164+
token: copilotToken?.accessToken ?? '',
165+
host: 'https://github.com'
166+
};
189167
}

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
import type { Attachment, Session } from '@github/copilot/sdk';
77
import type * as vscode from 'vscode';
8+
import { IAuthenticationService } from '../../../../platform/authentication/common/authentication';
89
import { IGitService } from '../../../../platform/git/common/gitService';
910
import { ILogService } from '../../../../platform/log/common/logService';
1011
import { IWorkspaceService } from '../../../../platform/workspace/common/workspaceService';
@@ -15,7 +16,7 @@ import { ResourceMap } from '../../../../util/vs/base/common/map';
1516
import { extUriBiasedIgnorePathCase } from '../../../../util/vs/base/common/resources';
1617
import { ChatRequestTurn2, ChatResponseThinkingProgressPart, ChatResponseTurn2, ChatSessionStatus, EventEmitter, Uri } from '../../../../vscodeTypes';
1718
import { ExternalEditTracker } from '../../common/externalEditTracker';
18-
import { CopilotCLISessionOptions, ICopilotCLISessionOptionsService } from './copilotCli';
19+
import { CopilotCLISessionOptions, getAuthInfo } from './copilotCli';
1920
import { buildChatHistoryFromEvents, getAffectedUrisForEditTool, isCopilotCliEditToolCall, processToolExecutionComplete, processToolExecutionStart } from './copilotcliToolInvocationFormatter';
2021
import { PermissionRequest } from './permissionHelpers';
2122

@@ -71,7 +72,7 @@ export class CopilotCLISession extends DisposableStore implements ICopilotCLISes
7172
@IGitService private readonly gitService: IGitService,
7273
@ILogService private readonly logService: ILogService,
7374
@IWorkspaceService private readonly workspaceService: IWorkspaceService,
74-
@ICopilotCLISessionOptionsService private readonly cliSessionOptions: ICopilotCLISessionOptionsService,
75+
@IAuthenticationService private readonly authenticationService: IAuthenticationService
7576
) {
7677
super();
7778
this.sessionId = _sdkSession.sessionId;
@@ -135,7 +136,7 @@ export class CopilotCLISession extends DisposableStore implements ICopilotCLISes
135136
try {
136137
const [currentModel, authInfo] = await Promise.all([
137138
modelId ? this._sdkSession.getSelectedModel() : undefined,
138-
this.cliSessionOptions.createOptions({}).then(opts => opts.toSessionOptions().authInfo)
139+
getAuthInfo(this.authenticationService)
139140
]);
140141
if (authInfo) {
141142
this._sdkSession.setAuthInfo(authInfo);

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

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

6-
import type { ModelMetadata, Session, internal } from '@github/copilot/sdk';
6+
import type { Session, internal } from '@github/copilot/sdk';
77
import { ChatCompletionMessageParam } from 'openai/resources/chat/completions';
88
import type { CancellationToken, ChatRequest } from 'vscode';
99
import { INativeEnvService } from '../../../../platform/env/common/envService';
@@ -19,7 +19,7 @@ import { Disposable, DisposableMap, DisposableStore, IDisposable, toDisposable }
1919
import { joinPath } from '../../../../util/vs/base/common/resources';
2020
import { IInstantiationService } from '../../../../util/vs/platform/instantiation/common/instantiation';
2121
import { ChatSessionStatus } from '../../../../vscodeTypes';
22-
import { CopilotCLISessionOptions, ICopilotCLISDK, ICopilotCLISessionOptionsService } from './copilotCli';
22+
import { CopilotCLISessionOptions, ICopilotCLISDK } from './copilotCli';
2323
import { CopilotCLISession, ICopilotCLISession } from './copilotcliSession';
2424
import { stripReminders } from './copilotcliToolInvocationFormatter';
2525
import { getCopilotLogger } from './logger';
@@ -45,13 +45,13 @@ export interface ICopilotCLISessionService {
4545
deleteSession(sessionId: string): Promise<void>;
4646

4747
// Session wrapper tracking
48-
getSession(sessionId: string, model: string | undefined, workingDirectory: string | undefined, readonly: boolean, token: CancellationToken): Promise<ICopilotCLISession | undefined>;
49-
createSession(prompt: string, model: string | undefined, workingDirectory: string | undefined, isolationEnabled: boolean | undefined, token: CancellationToken): Promise<ICopilotCLISession>;
48+
getSession(sessionId: string, options: { model?: string; workingDirectory?: string; isolationEnabled?: boolean; readonly: boolean }, token: CancellationToken): Promise<ICopilotCLISession | undefined>;
49+
createSession(prompt: string, options: { model?: string; workingDirectory?: string; isolationEnabled?: boolean }, token: CancellationToken): Promise<ICopilotCLISession>;
5050
}
5151

5252
export const ICopilotCLISessionService = createServiceIdentifier<ICopilotCLISessionService>('ICopilotCLISessionService');
5353

54-
const SESSION_SHUTDOWN_TIMEOUT_MS = 30 * 1000;
54+
const SESSION_SHUTDOWN_TIMEOUT_MS = 300 * 1000;
5555

5656
export class CopilotCLISessionService extends Disposable implements ICopilotCLISessionService {
5757
declare _serviceBrand: undefined;
@@ -70,7 +70,6 @@ export class CopilotCLISessionService extends Disposable implements ICopilotCLIS
7070
@ILogService private readonly logService: ILogService,
7171
@ICopilotCLISDK private readonly copilotCLISDK: ICopilotCLISDK,
7272
@IInstantiationService private readonly instantiationService: IInstantiationService,
73-
@ICopilotCLISessionOptionsService private readonly optionsService: ICopilotCLISessionOptionsService,
7473
@INativeEnvService private readonly nativeEnv: INativeEnvService,
7574
@IFileSystemService private readonly fileSystem: IFileSystemService,
7675
) {
@@ -119,16 +118,7 @@ export class CopilotCLISessionService extends Disposable implements ICopilotCLIS
119118
// This is a new session not yet persisted to disk by SDK
120119
return undefined;
121120
}
122-
const timestamp = metadata.startTime;
123121
const id = metadata.sessionId;
124-
const label = metadata.summary ? labelFromPrompt(metadata.summary) : undefined;
125-
if (label) {
126-
return {
127-
id,
128-
label,
129-
timestamp,
130-
} satisfies ICopilotCLISessionItem;
131-
}
132122
let dispose: (() => Promise<void>) | undefined = undefined;
133123
let session: Session | undefined = undefined;
134124
try {
@@ -144,6 +134,18 @@ export class CopilotCLISessionService extends Disposable implements ICopilotCLIS
144134
return undefined;
145135
}
146136
const label = this._generateSessionLabel(session.sessionId, chatMessages, undefined);
137+
138+
// Get timestamp from last SDK event, or fallback to metadata.startTime
139+
const sdkEvents = session.getEvents();
140+
const lastEventWithTimestamp = [...sdkEvents].reverse().find(event =>
141+
event.type !== 'session.import_legacy'
142+
&& event.type !== 'session.start'
143+
&& 'timestamp' in event
144+
);
145+
const timestamp = lastEventWithTimestamp && 'timestamp' in lastEventWithTimestamp
146+
? new Date(lastEventWithTimestamp.timestamp)
147+
: metadata.startTime;
148+
147149
return {
148150
id,
149151
label,
@@ -183,14 +185,10 @@ export class CopilotCLISessionService extends Disposable implements ICopilotCLIS
183185
return { session, dispose: () => Promise.resolve() };
184186
}
185187

186-
public async createSession(prompt: string, model: string | undefined, workingDirectory: string | undefined, isolationEnabled: boolean | undefined, token: CancellationToken): Promise<CopilotCLISession> {
188+
public async createSession(prompt: string, { model, workingDirectory, isolationEnabled }: { model?: string; workingDirectory?: string; isolationEnabled?: boolean }, token: CancellationToken): Promise<CopilotCLISession> {
187189
const sessionDisposables = this._register(new DisposableStore());
188190
try {
189-
const options = await raceCancellationError(this.optionsService.createOptions({
190-
model: model as unknown as ModelMetadata['model'],
191-
workingDirectory
192-
}), token);
193-
options.isolationEnabled = isolationEnabled === true;
191+
const options = new CopilotCLISessionOptions({ model, workingDirectory, isolationEnabled }, this.logService);
194192
const sessionManager = await raceCancellationError(this.getSessionManager(), token);
195193
const sdkSession = await sessionManager.createSession(options.toSessionOptions());
196194
const chatMessages = await sdkSession.getChatContextMessages();
@@ -220,7 +218,7 @@ export class CopilotCLISessionService extends Disposable implements ICopilotCLIS
220218
}
221219
}
222220

223-
public async getSession(sessionId: string, model: string | undefined, workingDirectory: string | undefined, readonly: boolean, token: CancellationToken): Promise<CopilotCLISession | undefined> {
221+
public async getSession(sessionId: string, { model, workingDirectory, isolationEnabled, readonly }: { model?: string; workingDirectory?: string; isolationEnabled?: boolean; readonly: boolean }, token: CancellationToken): Promise<CopilotCLISession | undefined> {
224222
const session = this._sessionWrappers.get(sessionId);
225223

226224
if (session) {
@@ -231,10 +229,7 @@ export class CopilotCLISessionService extends Disposable implements ICopilotCLIS
231229
const sessionDisposables = this._register(new DisposableStore());
232230
try {
233231
const sessionManager = await raceCancellationError(this.getSessionManager(), token);
234-
const options = await raceCancellationError(this.optionsService.createOptions({
235-
model: model as unknown as ModelMetadata['model'],
236-
workingDirectory
237-
}), token);
232+
const options = new CopilotCLISessionOptions({ model, workingDirectory, isolationEnabled }, this.logService);
238233

239234
const sdkSession = await sessionManager.getSession({ ...options.toSessionOptions(), sessionId }, !readonly);
240235
if (!sdkSession) {

0 commit comments

Comments
 (0)