Skip to content

Conversation

@DonJayamanne
Copy link
Collaborator

No description provided.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Introduces an opt-in path to run Copilot CLI chat sessions and model resolution via the new @github/copilot-sdk, while keeping the existing CLI SDK implementation as the default/fallback.

Changes:

  • Add a new advanced setting (chat.advanced.cli.sdk.enabled) and a selector service to choose between old vs new CLI SDK implementations.
  • Route Copilot CLI session + model services through delegating implementations to support both SDKs.
  • Add new @github/copilot-sdk dependency and update session history retrieval to be async.

Reviewed changes

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

Show a summary per file
File Description
src/platform/configuration/common/configurationService.ts Adds a config key to gate new Copilot CLI SDK usage.
src/extension/chatSessions/vscode-node/copilotCLIChatSessionsContribution.ts Updates session history retrieval to await the now-async API.
src/extension/chatSessions/vscode-node/chatSessions.ts Wires new selector/delegating services into DI for Copilot CLI chat sessions.
src/extension/agents/copilotcli/node/sdk/delegatingSessionService.ts Adds delegating session service that routes to old/new implementations.
src/extension/agents/copilotcli/node/sdk/delegatingModels.ts Adds delegating model service that routes to old/new implementations.
src/extension/agents/copilotcli/node/sdk/copilotcliSessionService.ts Implements session management using @github/copilot-sdk client sessions.
src/extension/agents/copilotcli/node/sdk/copilotcliSession.ts Wraps @github/copilot-sdk session events into the existing ICopilotCLISession contract.
src/extension/agents/copilotcli/node/sdk/copilotcliModels.ts Implements model listing/default model via CopilotClient.listModels().
src/extension/agents/copilotcli/node/sdk/copilotClientManager.ts Introduces a manager to create/start/stop a CopilotClient.
src/extension/agents/copilotcli/node/copilotcliSession.ts Makes getChatHistory() async on the legacy session implementation.
src/extension/agents/copilotcli/node/copilotCliSdkSelector.ts Adds selector service based on the new config key.
package.json Adds @github/copilot-sdk dependency.
package-lock.json Locks @github/copilot-sdk and its transitive dependencies.
Comments suppressed due to low confidence (2)

src/extension/agents/copilotcli/node/sdk/copilotClientManager.ts:71

  • Same logging issue here: pass the caught error as the first argument to logService.error(...) and the context string as the second, so stack/details are preserved and log formatting is consistent.
				this.logService.trace('[CopilotClientManager] CopilotClient stopped');
			} catch (error) {
				this.logService.error('[CopilotClientManager] Failed to stop CopilotClient', error);
			}

src/extension/agents/copilotcli/node/sdk/copilotcliModels.ts:32

  • ILogService.error expects the Error (or error string) as the first parameter, and an optional context message second. These calls currently pass the context string first and the error object second, which usually loses stack/details. Consider swapping the arguments to preserve exception details.
		// Eagerly fetch available models so that they're ready when needed.
		this._availableModels.value.catch(error => {
			this.logService.error('[NewSdkCopilotCLIModels] Failed to fetch available models', error);
		});

Comment on lines 37 to 43
this._oldService = this._register(instantiationService.createInstance(CopilotCLISessionService));
this._newService = this._register(instantiationService.createInstance(NewSdkCopilotCLISessionService));

// Forward change events from both services
this._register(this._oldService.onDidChangeSessions(() => this._onDidChangeSessions.fire()));
this._register(this._newService.onDidChangeSessions(() => this._onDidChangeSessions.fire()));
}
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The delegating session service eagerly creates both the old and new session services. Both implementations set up filesystem watchers on the same session-state directory and forward onDidChangeSessions, which can cause duplicated change events and unnecessary overhead even when only one SDK is in use. Consider lazily instantiating the inactive service (or centralizing file watching in the delegator) to avoid duplicate watchers/events.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Comment on lines 5809 to 5811
"@github/blackbird-external-ingest-utils": "^0.2.0",
"@github/copilot": "^0.0.400",
"@github/copilot-sdk": "^0.1.22",
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding @github/copilot-sdk introduces a nested dependency on @github/copilot@0.0.403 while the repo currently depends on @github/copilot@^0.0.400 at the top level. This will typically result in two Copilot CLI binaries being installed (larger install footprint and potentially confusing runtime behavior). Consider aligning the root @github/copilot version (or using an override/resolution) so npm can dedupe to a single version.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under discussion

@DonJayamanne DonJayamanne force-pushed the don/technological-amphibian branch from bcc899d to 9ed39d6 Compare February 12, 2026 18:29
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

toolCalls: Map<string, ToolCall>,
): NewSdkRefCountedSession {
const unsubscribeToolStart = sdkSession.on('tool.execution_start', (event) => {
toolCalls.set(event.id, event.data as unknown as ToolCall);
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toolCalls is later accessed via toolCalls.get(toolCallId) (from permReq.toolCallId), but this listener stores entries under event.id. If event.id is not the same as event.data.toolCallId, permission handling/edit tracking will fail to find the tool call (affecting auto-approval and edit tracking). Store tool calls keyed by the tool-call id (e.g. event.data.toolCallId) to match the lookup path.

Suggested change
toolCalls.set(event.id, event.data as unknown as ToolCall);
const toolCallId = (event as { data: { toolCallId?: string } }).data.toolCallId ?? event.id;
toolCalls.set(toolCallId, event.data as unknown as ToolCall);

Copilot uses AI. Check for mistakes.
"github.copilot.config.cli.customAgents.enabled": "Enable Custom Agents for Background Agents.",
"github.copilot.config.cli.mcp.enabled": "Enable Model Context Protocol (MCP) server for Background Agents.",
"github.copilot.config.cli.branchSupport.enabled": "Enable branch support for Background Agents.",
"github.copilot.config.cli.sdk.enabled": "Enable the new SDK for Background Agents.",
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DelegatingCopilotCLIModels / DelegatingCopilotCLISessionService evaluate the SDK selector once on first access and require a reload for changes to take effect, but this setting description doesn’t mention that. Consider adding “Requires VS Code reload” (similar to the sessionController setting) to avoid confusing users who toggle it at runtime.

Suggested change
"github.copilot.config.cli.sdk.enabled": "Enable the new SDK for Background Agents.",
"github.copilot.config.cli.sdk.enabled": "Enable the new SDK for Background Agents. Requires VS Code reload.",

Copilot uses AI. Check for mistakes.
Comment on lines +378 to +381
const [model, history] = await Promise.all([
(existingSession ? (_sessionModel.get(copilotcliSessionId) ?? await existingSession.object.getSelectedModelId()) : _sessionModel.get(copilotcliSessionId)) ?? await this.getCustomAgentModel(defaultAgent, token) ?? defaultModel,
existingSession?.object?.getChatHistory() || []
]);
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Promise.all([...]) here doesn’t actually run model resolution and history loading in parallel because the first array element contains await expressions and is evaluated sequentially before Promise.all executes. Consider turning the model computation into a Promise (e.g., an async IIFE) so history loading can overlap with model selection, or just compute them sequentially without Promise.all for clarity.

Suggested change
const [model, history] = await Promise.all([
(existingSession ? (_sessionModel.get(copilotcliSessionId) ?? await existingSession.object.getSelectedModelId()) : _sessionModel.get(copilotcliSessionId)) ?? await this.getCustomAgentModel(defaultAgent, token) ?? defaultModel,
existingSession?.object?.getChatHistory() || []
]);
const modelPromise = (async () => {
return (existingSession ? (_sessionModel.get(copilotcliSessionId) ?? await existingSession.object.getSelectedModelId()) : _sessionModel.get(copilotcliSessionId)) ?? await this.getCustomAgentModel(defaultAgent, token) ?? defaultModel;
})();
const historyPromise = existingSession?.object?.getChatHistory() ?? Promise.resolve([]);
const [model, history] = await Promise.all([modelPromise, historyPromise]);

Copilot uses AI. Check for mistakes.
Comment on lines +129 to +161
public async createSession({ model, workingDirectory, isolationEnabled, agent }: { model?: string; workingDirectory?: Uri; isolationEnabled?: boolean; agent?: SweCustomAgent }, token: CancellationToken): Promise<NewSdkRefCountedSession> {
const client = await raceCancellationError(this.copilotClientManager.getClient(), token);
const { config, toolCalls, sessionOptions } = await this._buildSessionConfig({ model, workingDirectory, isolationEnabled, token });

const sdkSession = await client.createSession(config);
this.logService.trace(`[NewSdkSessionService] Created new CopilotCLI session ${sdkSession.sessionId}.`);

return this._wrapSdkSession(sdkSession, sessionOptions, model, toolCalls);
}

public async getSession(sessionId: string, { model, workingDirectory, isolationEnabled, readonly: _readonly, agent }: { model?: string; workingDirectory?: Uri; isolationEnabled?: boolean; readonly: boolean; agent?: SweCustomAgent }, token: CancellationToken): Promise<NewSdkRefCountedSession | undefined> {
const lock = this.sessionMutexForGetSession.get(sessionId) ?? new Mutex();
this.sessionMutexForGetSession.set(sessionId, lock);
const lockDisposable = await lock.acquire(token);
if (!lockDisposable || this._store.isDisposed || token.isCancellationRequested) {
lockDisposable?.dispose();
return;
}

try {
const existing = this._sessionWrappers.get(sessionId);
if (existing) {
this.logService.trace(`[NewSdkSessionService] Reusing CopilotCLI session ${sessionId}.`);
existing.acquire();
return existing;
}

const client = await raceCancellationError(this.copilotClientManager.getClient(), token);
const { config, toolCalls, sessionOptions } = await this._buildSessionConfig({ model, workingDirectory, isolationEnabled, token });

const sdkSession = await client.resumeSession(sessionId, config);
return this._wrapSdkSession(sdkSession, sessionOptions, model, toolCalls);
} finally {
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agent is accepted in createSession/getSession but never used when building the new SDK SessionConfig. This means selecting a custom agent in the UI (passed via agent) won’t affect the session when the new SDK flag is enabled. Please wire agent into the new SDK configuration/selection mechanism (equivalent to old SDK’s selectedCustomAgent / selectCustomAgent) so behavior matches the existing implementation.

Copilot uses AI. Check for mistakes.
Comment on lines 400 to 414
/**
* Note: The new SDK does not support emitting synthetic events on the session.
* This is a no-op. If delegation context is needed, use SessionConfig.systemMessage instead.
*/
addUserMessage(_content: string): void {
this.logService.trace('[NewSdkCopilotCLISession] addUserMessage is not supported by the new SDK — using no-op');
}

/**
* Note: The new SDK does not support emitting synthetic events on the session.
* This is a no-op. If delegation context is needed, use SessionConfig.systemMessage instead.
*/
addUserAssistantMessage(_content: string): void {
this.logService.trace('[NewSdkCopilotCLISession] addUserAssistantMessage is not supported by the new SDK — using no-op');
}
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addUserMessage / addUserAssistantMessage are no-ops in the new SDK wrapper, but existing features (e.g. delegation to cloud via recordPushToSession) rely on these synthetic messages being added so they appear in session history. If the new SDK can’t emit events, consider maintaining a local synthetic history (merged in getChatHistory) or another mechanism so delegation metadata and other synthetic turns aren’t lost when the new SDK is enabled.

Copilot uses AI. Check for mistakes.
Comment on lines +30 to +36
export class NewSdkCopilotCLISessionService extends Disposable implements ICopilotCLISessionService {
declare _serviceBrand: undefined;

private _sessionWrappers = new DisposableMap<string, NewSdkRefCountedSession>();

private readonly _onDidChangeSessions = new Emitter<void>();
public readonly onDidChangeSessions = this._onDidChangeSessions.event;
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR introduces a full new SDK-backed session/model stack (new files under node/sdk/) but there are no unit tests covering the new path (e.g., tool-call tracking for permission requests, agent selection, listSessions mapping). There are already unit tests for the old CopilotCLISessionService; adding analogous tests for the new services would help prevent regressions behind the feature flag.

Copilot uses AI. Check for mistakes.
Comment on lines 5978 to 5980
"@github/copilot": "^0.0.406",
"@github/copilot-sdk": "^0.1.22",
"@google/genai": "^1.22.0",
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding @github/copilot-sdk introduces its own dependency on @github/copilot (package-lock shows @github/copilot-sdk/node_modules/@github/copilot@0.0.403) while this repo depends on @github/copilot@^0.0.406. This can lead to duplicated CLI binaries and potentially different runtime behavior between old vs new SDK paths. Consider aligning the @github/copilot version via an overrides entry (or upgrading the SDK) so both stacks use the same Copilot CLI version.

Copilot uses AI. Check for mistakes.
Comment on lines +68 to +69
// We'd need to resume the session or track working directories ourselves.
// For now return undefined — the delegating service can fall back to the old SDK path.
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment says the delegating service can “fall back to the old SDK path”, but DelegatingCopilotCLISessionService chooses old vs new SDK once and then forwards calls to that implementation only. Either update the comment to reflect the current behavior, or implement an explicit fallback strategy when the new SDK can’t provide the working directory.

Suggested change
// We'd need to resume the session or track working directories ourselves.
// For now return undefined — the delegating service can fall back to the old SDK path.
// We'd need to resume the session or track working directories ourselves to reconstruct it.
// For now, this implementation does not provide a working directory and always returns undefined.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant