-
Notifications
You must be signed in to change notification settings - Fork 39.7k
Add coverage for Copilot CLI node-pty shimming #314128
Copy link
Copy link
Closed
Labels
copilot-cli-agentBackground Agent related features/bugsBackground Agent related features/bugsdebtCode quality issuesCode quality issuesinsiders-releasedPatch has been released in VS Code InsidersPatch has been released in VS Code Insidersterminal-processProblems launching processes, managing ptys, exiting, process leaks, etc.Problems launching processes, managing ptys, exiting, process leaks, etc.
Milestone
Metadata
Metadata
Assignees
Labels
copilot-cli-agentBackground Agent related features/bugsBackground Agent related features/bugsdebtCode quality issuesCode quality issuesinsiders-releasedPatch has been released in VS Code InsidersPatch has been released in VS Code Insidersterminal-processProblems launching processes, managing ptys, exiting, process leaks, etc.Problems launching processes, managing ptys, exiting, process leaks, etc.
Follow-up to #313609 / #313550.
The regression was fixed by bringing back runtime
node-ptyshimming for the Copilot CLI SDK path. We should add a few targeted tests so this does not regress again during SDK/layout changes.Today we already have one useful guard:
copilotCLISDKUpgrade.spec.tschecks the Copilot SDK native-binary layout against a known allowlist. This is the existing guard for added/removed SDK native files.The gap is around the runtime shim behavior itself:
nodePtyShim.spec.tsonly testsresolveNodePtySourcePath.nodePtyShim.tshas the actualensureNodePtyShim/copyNodePtyFilespath, but we don't directly assert that files are copied intonode_modules/@github/copilot/sdk/prebuilds/<platform>-<arch>.CopilotCLISDK.getPackagewaits forensureShims()before importing@github/copilot/sdk, but there is no test around the ordering /shims.txtskip behavior.AGENTS.mdstill saysnode-ptyis no longer shimmed, which is stale after Bring back runtime nodePtyShim for Copilot CLI marketplace route #313550.What this issue should cover:
copyNodePtyFiles: create a tempnode-ptysource containingpty.node/spawn-helper, callcopyNodePtyFiles, and assert the files land undernode_modules/@github/copilot/sdk/prebuilds/<platform>-<arch>.CopilotCLISDK.ensureShims: noshims.txtshould invoke both ripgrep and node-pty shims before SDK import; existingshims.txtshould skip the work.copilotCLISDKUpgrade.spec.tscurrent as the SDK native-binary layout guard.node-pty is no longer shimmednote inAGENTS.md.These should run in the existing Copilot
npm run test:unitPR checks. The goal is to catch the practical ways this support can regress without adding something too heavy/flaky for the first pass.