Skip to content

sessions with sdk#295817

Closed
joshspicer wants to merge 31 commits intomainfrom
josh/sessions-with-sdk
Closed

sessions with sdk#295817
joshspicer wants to merge 31 commits intomainfrom
josh/sessions-with-sdk

Conversation

@joshspicer
Copy link
Member

@joshspicer joshspicer commented Feb 17, 2026

launch with code --sessions-utility-process

related https://github.com/microsoft/vscode-internalbacklog/issues/6848

Copilot AI review requested due to automatic review settings February 17, 2026 18:23
@joshspicer joshspicer changed the title Josh/sessions with sdk sessions with sdk Feb 17, 2026

This comment was marked as outdated.

Add @github/copilot-sdk 0.1.23 as a production dependency. The SDK ships
per-platform native CLI binaries (@github/copilot-darwin-arm64, etc.) that
require build system support:

- .moduleignore: strip copilot prebuilds/ripgrep/clipboard for other platforms
- gulpfile.vscode.ts: filter wrong-arch platform packages, ASAR-unpack binaries
- darwin/create-universal-app.ts: copy missing arch package for universal merger
- darwin/sign.ts: custom entitlements for the copilot CLI binary
- darwin/verify-macho.ts: skip copilot binaries in arch verification
- next/index.ts: add copilotSdkHost as a utility process entry point
Add 'sessions-sdk' to NativeParsedArgs and the OPTIONS descriptor so it
can be passed on the command line (e.g. ./scripts/code.sh --sessions-sdk).

Add isSessionsSdkWindow to IWorkbenchEnvironmentService so renderer code
can check whether the SDK backend is active. Browser returns false; native
reads from the window configuration.
Three-layer architecture for the Copilot SDK integration:

1. common/copilotSdkService.ts — ICopilotSdkService interface with session
   CRUD, messaging, events, model listing, and auth. All IPC types defined
   here so both main and renderer can reference them.

2. electron-main/copilotSdkStarter.ts — CopilotSdkMainService that lazily
   spawns a UtilityProcess on first use, connects via MessagePort, and
   exposes an IServerChannel. Follows the pty host pattern.

3. node/copilotSdkHost.ts — Utility process entry point that wraps the
   @github/copilot-sdk CopilotClient. Implements ICopilotSdkService so
   ProxyChannel.fromService() auto-generates the IPC channel. Resolves
   the bundled CLI binary, builds a clean env, and forwards all events.
Register CopilotSdkMainService and its IPC channel in the main process,
gated behind args[sessions-sdk] so no utility process is spawned or DI
service created during normal VS Code usage.

- app.ts: conditional DI registration + channel setup + window open logic
- chat.contribution.ts: register SdkChatViewPane when isSessionsSdkWindow
- sessions.desktop.main.ts: import copilotSdkService renderer proxy
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 34 out of 35 changed files in this pull request and generated 14 comments.

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 34 out of 35 changed files in this pull request and generated 6 comments.

@joshspicer
Copy link
Member Author

joshspicer commented Feb 17, 2026

Code Review — All Threads Addressed

All actionable feedback has been fixed. Resolving threads now.

✅ Fixed (16 threads)

  • _sdkListDisposables redundant _register → removed
  • CLI resolve fail continues silently → added throw
  • window.confirm()IDialogService.confirm()
  • execSync unsafe → fs.cpSync({ recursive: true })
  • Markdown renderer results leak → WeakMap<HTMLElement, DisposableStore>
  • Bracket notation configuration['sessions-sdk'] → proper .isSessionsUtilityProcess
  • process.stderr.write never restored → _originalStderrWrite field + restore in stop()
  • Non-null assertion on _worktreePath → local variable capture
  • split('/') not cross-platform → _labelForPath() with paths.basename()
  • Singleton _instance not cleared → override dispose() clears it
  • Session event listeners leak → per-session DisposableStore
  • Delete without confirmation → dialogService.confirm()
  • Silent catch on session destroy → now logs via _onProcessOutput
  • Delete button accessibility → aria-label, type="button", keydown handler
  • ASAR unpack glob too broad → narrowed to copilot-*/copilot*
  • Session list role="list" + aria-selected → added
  • _relativeTime future dates → diffMs <= 0 guard
  • _ensureClient race condition → promise-based lock
  • Tool labels not localized → all use localize(...)
  • Debug panel strings not localized → all use localize(...)

🔇 Won't Fix / False Positive (5 threads)

  • SDK Node.js version: false positive, works fine
  • Timeout message mismatch: 30s matches 30000ms
  • allow-any-unicode-next-line: valid VS Code hygiene directive
  • autoRestart backoff: handled by SDK internally
  • Hardcoded model preferences: intentional experiment default
  • Folder history validation: low priority, paths from picker/SDK

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 34 out of 35 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

src/vs/platform/copilotSdk/node/copilotSdkHost.ts:398

  • _attachSessionEvents() unconditionally creates a new DisposableStore and overwrites _sessionDisposables for sessionId without disposing any existing store. If resumeSession() is called multiple times for the same session, this will leak listeners. Dispose the existing store (if any) before replacing it.
	private _attachSessionEvents(session: CopilotSession): void {
		const sessionId = session.sessionId;
		const store = new DisposableStore();
		const listener = session.on((event: SessionEvent) => {
			this._onSessionEvent.fire({
				sessionId,
				type: event.type as ICopilotSessionEvent['type'],
				data: (event as { data?: Record<string, unknown> }).data ?? {},
			});
		});
		store.add(typeof listener === 'function' ? toDisposable(listener) : listener);
		this._sessionDisposables.set(sessionId, store);
	}

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

I left some initial high level feedback for the areas I felt I can provide it and ignored changes to vs/sessions for now and also did not yet drill into the utility process specifics.

I want @deepak1556 to comment on the build changes, the entitlements and the platform specific modules. Is the SDK really built natively for different platforms? I was hoping for a JS library, but maybe that is not the case.

On a high level I feel the direction of mixing both the architectural change (to move to utility process) and introducing entire new UI components for chat (which are not compatible to the old UI) the wrong approach. I would like us to:

  • have 1 PR with the architectural change of a utility process and using SDK compatible with the old UI
  • have 1 PR with exploring to recreate the chat rendering UI on top of the architectural changes

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 33 out of 36 changed files in this pull request and generated 1 comment.

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 33 out of 36 changed files in this pull request and generated 5 comments.

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

Many unresolved Copilot code feedback comments and 1 comment from me.

server.registerChannel(CopilotSdkChannel, channel);
process.stderr.write(`[CopilotSdkHost] Channel '${CopilotSdkChannel}' registered on server\n`);

process.once('exit', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Are we certain this works? At this moment the process is stopping and you are calling a long running operation?

Copy link
Collaborator

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

Sorry for the late review, the comments can be addressed as followup

<true/>
<key>com.apple.security.cs.allow-unsigned-executable-memory</key>
<true/>
<key>com.apple.security.automation.apple-events</key>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the apple events needed ?

% codesign -d --entitlements :- ~/Downloads/copilot
Executable=/Users/demohan/Downloads/copilot
warning: Specifying ':' in the path is deprecated and will not work in a future release
<?xml version="1.0" encoding="UTF-8"?><!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "https://www.apple.com/DTDs/PropertyList-1.0.dtd"><plist version="1.0"><dict><key>com.apple.security.cs.allow-jit</key><true/><key>com.apple.security.cs.allow-unsigned-executable-memory</key><true/><key>com.apple.security.cs.disable-library-validation</key><true/></dict></plist>

For the other 3 the utility process is already signed with the same set https://github.com/microsoft/vscode/blob/main/build/azure-pipelines/darwin/helper-plugin-entitlements.plist , so copliot cli launched from the process should inherit it by default.

Tl:dr, if the apple events is not needed can we remove this entitlement file in favor inheritance.

} else if (fs.existsSync(inArm64U) && !fs.existsSync(inX64U)) {
fs.mkdirSync(path.dirname(inX64U), { recursive: true });
fs.cpSync(inArm64U, inX64U, { recursive: true });
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm can you remove the platform specific suffix and keep it as @github/copilot then we would just merge the binaries without having to do the above workaround.

type: 'copilotSdkHost',
name: 'copilot-sdk-host',
entryPoint: 'vs/sessions/services/copilotSdk/node/copilotSdkHost',
args: ['--logsPath', this._environmentMainService.logsHome.with({ scheme: Schemas.file }).fsPath, '--disable-gpu'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

why --disable-gpu ?

Comment on lines +123 to +124
delete cliEnv['__CFBundleIdentifier'];
delete cliEnv['APP_SANDBOX_CONTAINER_ID'];
Copy link
Collaborator

Choose a reason for hiding this comment

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

APP_SANDBOX_CONTAINER_ID is for apps from mac app store relying on the mac container sandbox, which our app doesn't, can be removed

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.

6 participants