Stop copying node-pty into Copilot CLI SDK#310925
Merged
anthonykim1 merged 9 commits intomainfrom Apr 21, 2026
Merged
Conversation
Contributor
Contributor
There was a problem hiding this comment.
Pull request overview
Removes the now-obsolete node-pty shim/copy logic for the Copilot CLI SDK after the SDK started resolving node-pty from the host (VS Code) via hostRequire, keeping only the ripgrep shim behavior.
Changes:
- Delete the runtime
node-ptyshim (nodePtyShim.ts) and stop invoking it during CLI SDK initialization. - Remove the build-time packaging copy step for
node-ptyand update build tasks to only prepare Copilot shims (ripgrep + marker). - Update upgrade tests/docs to no longer treat
node-ptyas a shimmed artifact.
Show a summary per file
| File | Description |
|---|---|
| extensions/copilot/src/extension/chatSessions/vscode-node/test/copilotCLISDKUpgrade.spec.ts | Removes test-time node-pty shim copying; keeps ripgrep shim setup and binary inventory checks. |
| extensions/copilot/src/extension/chatSessions/copilotcli/node/nodePtyShim.ts | Removes the runtime node-pty shim implementation entirely. |
| extensions/copilot/src/extension/chatSessions/copilotcli/node/copilotCli.ts | Stops creating node-pty shims at runtime; keeps ripgrep shim before SDK import. |
| extensions/copilot/src/extension/chatSessions/copilotcli/AGENTS.md | Updates documentation to reflect that only ripgrep is shimmed; node-pty is resolved via hostRequire. |
| extensions/copilot/eslint.config.mjs | Removes ESLint ignore entry for the deleted shim file. |
| build/lib/copilot.ts | Removes build-time node-pty copy helper; keeps built-in shim preparation for ripgrep + marker. |
| build/gulpfile.vscode.ts | Updates packaging pipeline to only prepare Copilot shims (no native deps copy). |
| build/gulpfile.reh.ts | Same as above for REH/server packaging. |
Copilot's findings
Comments suppressed due to low confidence (3)
extensions/copilot/src/extension/chatSessions/vscode-node/test/copilotCLISDKUpgrade.spec.ts:125
- Test name says it verifies loading the
@github/copilotmodule, but the test currently imports@github/copilot/sdkagain (same as the earlier test). This won’t catch regressions in the@github/copilotentrypoint; either change the import to@github/copilotor update/remove the redundant test so the assertion matches the intent.
it('should be able to load the @github/copilot module without errors', async function () {
await import('@github/copilot/sdk');
});
extensions/copilot/src/extension/chatSessions/vscode-node/test/copilotCLISDKUpgrade.spec.ts:131
copyBinariesredefinescopilotSDKPath, shadowing the outercopilotSDKPathconstant from the suite. This makes the test a bit harder to follow; consider reusing the outer constant (or rename the inner variable) to avoid shadowing.
async function copyBinaries(extensionPath: string) {
const copilotSDKPath = path.join(extensionPath, 'node_modules', '@github', 'copilot');
const vscodeRipgrepPath = path.join(copilotSDKPath, 'ripgrep', 'bin', process.platform + '-' + process.arch);
await copyRipgrepShim(extensionPath, vscodeRipgrepPath, new TestLogService());
extensions/copilot/src/extension/chatSessions/copilotcli/node/copilotCli.ts:467
ILogService.erroris intended to take the thrownErroras the first argument and an optional stable context message as the second (and the message should not embed error details). Here the error is interpolated into the message and theErrorobject isn’t passed, which can lose stack/metadata and may include sensitive info. Passerroras the first arg and a short context string as the second.
try {
// Ensure the ripgrep shim exists before importing the SDK (required for CLI sessions)
await this._ensureShimsPromise;
return await import('@github/copilot/sdk');
} catch (error) {
this.logService.error(`[CopilotCLISession] Failed to load @github/copilot/sdk: ${error}`);
throw error;
- Files reviewed: 8/8 changed files
- Comments generated: 1
Co-authored-by: Copilot <copilot@github.com>
DonJayamanne
approved these changes
Apr 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #307746
The Copilot CLI SDK now resolves
node-ptyfrom the host (VS Code) viahostRequireand falls back to its bundled copy only if that fails, shipped in@github/copilot@1.0.26-0node-ptybinaries into the SDK's prebuilds folder.We had two copies of the same workaround:
build/lib/copilot.ts) ran during packaging, so shipped builds hadnode-ptyalready in place inside the SDK's prebuilds folder.nodePtyShim.ts, called fromensureShims()) ran the first time the CLI was used. This covered the dev loop (Code OSS /./scripts/code.sh) where the build-time step never runs, plus served as a safety net if the build-time copy was ever missing.Both are now obsolete — the SDK would finds VS Code's
node-ptyon its own.Kept as-is:
.moduleignorerules that strip@github/copilot/prebuilds/**and@github/copilot-{platform}packages — we still don't want to ship the SDK's bundled prebuilds, even as an unused fallback.ensureRipgrepShim— unchanged; ripgrep still needs to be shimmed.@DonJayamanne Now that we no longer shim node-pty, the computer.node and win32.node files under the previously skipped sdk/prebuilds/ are surfaced. We should acknowledge them in
knownBinariesso the test continues to track changes to them.