Fix/wsl remote tool calling#12422
Conversation
…ng and Node 22.22.3
dosubot
Bot
added
the
size:XL
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 104 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="extensions/vscode/src/diff/vertical/handler.ts">
<violation number="1" location="extensions/vscode/src/diff/vertical/handler.ts:377">
P1: Undo options are evaluated as a no-op comma expression inside the `edit` callback and are never passed to the VS Code API. `TextEditor.edit` expects options as its second argument, not inside the callback body. This breaks intended undo-grouping during Myers diff reapply.</violation>
</file>
<file name="binary/build.js">
<violation number="1" location="binary/build.js:221">
P1: Removed sqlite3 native module verification without completing dependency removal or updating the new SEA build path; builds may pass validation while failing at runtime when sqlite is exercised.</violation>
</file>
<file name="binary/utils/bundle-binary.js">
<violation number="1" location="binary/utils/bundle-binary.js:83">
P1: Cross-target binary packaging is broken: the SEA build always copies the host Node.js executable (`process.execPath`) regardless of the `target` parameter. The previous `pkg`-based build supported cross-compilation by using `pkgJson/${target}`, but Node SEA requires a platform/arch-matching base binary and the new code provides none. This causes all non-host targets to receive a host-architecture binary, producing broken artifacts.</violation>
<violation number="2" location="binary/utils/bundle-binary.js:149">
P1: Unsafe default LanceDB package fallback may bundle wrong native module for unmapped targets</violation>
</file>
Note: This PR contains a large number of files. cubic only reviews up to 100 files per PR, so some files may not have been reviewed. cubic prioritizes the most important files to review.
On a pro plan you can use ultrareview for larger PRs.
Fix all with cubic
Re-trigger cubic
| await this.editor.edit((editBuilder) => { | ||
| editBuilder.replace(this.range, replaceContent), | ||
| { undoStopAfter: false, undoStopBefore: false }; | ||
| (editBuilder.replace(this.range, replaceContent), |
There was a problem hiding this comment.
P1: Undo options are evaluated as a no-op comma expression inside the edit callback and are never passed to the VS Code API. TextEditor.edit expects options as its second argument, not inside the callback body. This breaks intended undo-grouping during Myers diff reapply.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At extensions/vscode/src/diff/vertical/handler.ts, line 377:
<comment>Undo options are evaluated as a no-op comma expression inside the `edit` callback and are never passed to the VS Code API. `TextEditor.edit` expects options as its second argument, not inside the callback body. This breaks intended undo-grouping during Myers diff reapply.</comment>
<file context>
@@ -374,8 +374,8 @@ export class VerticalDiffHandler implements vscode.Disposable {
await this.editor.edit((editBuilder) => {
- editBuilder.replace(this.range, replaceContent),
- { undoStopAfter: false, undoStopBefore: false };
+ (editBuilder.replace(this.range, replaceContent),
+ { undoStopAfter: false, undoStopBefore: false });
});
</file context>
| `${targetDir}/continue-binary${exe}`, | ||
| `${targetDir}/index.node`, // @lancedb | ||
| `${targetDir}/build/Release/node_sqlite3.node`, | ||
| `${targetDir}/rg${exe}`, // ripgrep binary |
There was a problem hiding this comment.
P1: Removed sqlite3 native module verification without completing dependency removal or updating the new SEA build path; builds may pass validation while failing at runtime when sqlite is exercised.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At binary/build.js, line 221:
<comment>Removed sqlite3 native module verification without completing dependency removal or updating the new SEA build path; builds may pass validation while failing at runtime when sqlite is exercised.</comment>
<file context>
@@ -218,7 +218,6 @@ async function buildWithEsbuild() {
`${targetDir}/continue-binary${exe}`,
`${targetDir}/index.node`, // @lancedb
- `${targetDir}/build/Release/node_sqlite3.node`,
`${targetDir}/rg${exe}`, // ripgrep binary
);
}
</file context>
| `node_modules/${TARGET_TO_LANCEDB[target]}/index.node`, | ||
| `${targetDir}/index.node`, | ||
| // 10. Source path | ||
| const lancedbPackage = TARGET_TO_LANCEDB[target] || "vectordb-linux-x64-gnu"; |
There was a problem hiding this comment.
P1: Unsafe default LanceDB package fallback may bundle wrong native module for unmapped targets
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At binary/utils/bundle-binary.js, line 149:
<comment>Unsafe default LanceDB package fallback may bundle wrong native module for unmapped targets</comment>
<file context>
@@ -46,26 +47,159 @@ async function downloadNodeSqlite(target, targetDir) {
- `node_modules/${TARGET_TO_LANCEDB[target]}/index.node`,
- `${targetDir}/index.node`,
+ // 10. Source path
+ const lancedbPackage = TARGET_TO_LANCEDB[target] || "vectordb-linux-x64-gnu";
+ const lancedbSource = path.join(
+ __dirname,
</file context>
| const lancedbPackage = TARGET_TO_LANCEDB[target] || "vectordb-linux-x64-gnu"; | |
| if (!TARGET_TO_LANCEDB[target]) { | |
| throw new Error(`No LanceDB package mapped for target: ${target}`); | |
| } | |
| const lancedbPackage = TARGET_TO_LANCEDB[target]; |
|
|
||
| // 4. copy the current Node.js executable as the shell/base of the binary. | ||
| console.log(`[info] [SEA] Copiando executável base do Node...`); | ||
| fs.copyFileSync(process.execPath, finalBinaryPath); |
There was a problem hiding this comment.
P1: Cross-target binary packaging is broken: the SEA build always copies the host Node.js executable (process.execPath) regardless of the target parameter. The previous pkg-based build supported cross-compilation by using pkgJson/${target}, but Node SEA requires a platform/arch-matching base binary and the new code provides none. This causes all non-host targets to receive a host-architecture binary, producing broken artifacts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At binary/utils/bundle-binary.js, line 83:
<comment>Cross-target binary packaging is broken: the SEA build always copies the host Node.js executable (`process.execPath`) regardless of the `target` parameter. The previous `pkg`-based build supported cross-compilation by using `pkgJson/${target}`, but Node SEA requires a platform/arch-matching base binary and the new code provides none. This causes all non-host targets to receive a host-architecture binary, producing broken artifacts.</comment>
<file context>
@@ -46,26 +47,159 @@ async function downloadNodeSqlite(target, targetDir) {
+
+ // 4. copy the current Node.js executable as the shell/base of the binary.
+ console.log(`[info] [SEA] Copiando executável base do Node...`);
+ fs.copyFileSync(process.execPath, finalBinaryPath);
+
+ // 5. Inject the code BLOB into the copied executable.
</file context>
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="extensions/vscode/scripts/prepackage.js">
<violation number="1" location="extensions/vscode/scripts/prepackage.js:458">
P1: Unconditionally overwrites ripgrep binaries with placeholder text, destroying real executables installed by dependency setup and causing runtime spawn failures.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Fix all with cubic | Re-trigger cubic
| fs.writeFileSync(path.join(dir, "rg"), "placeholder-content-for-dev-build"); | ||
| fs.writeFileSync( | ||
| path.join(dir, "rg.exe"), |
There was a problem hiding this comment.
P1: Unconditionally overwrites ripgrep binaries with placeholder text, destroying real executables installed by dependency setup and causing runtime spawn failures.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At extensions/vscode/scripts/prepackage.js, line 458:
<comment>Unconditionally overwrites ripgrep binaries with placeholder text, destroying real executables installed by dependency setup and causing runtime spawn failures.</comment>
<file context>
@@ -439,6 +439,29 @@ void (async () => {
+
+ ripgrepDirs.forEach((dir) => {
+ if (!fs.existsSync(dir)) fs.mkdirSync(dir, { recursive: true });
+ fs.writeFileSync(path.join(dir, "rg"), "placeholder-content-for-dev-build");
+ fs.writeFileSync(
+ path.join(dir, "rg.exe"),
</file context>
| fs.writeFileSync(path.join(dir, "rg"), "placeholder-content-for-dev-build"); | |
| fs.writeFileSync( | |
| path.join(dir, "rg.exe"), | |
| if (!fs.existsSync(path.join(dir, "rg"))) { | |
| fs.writeFileSync(path.join(dir, "rg"), "placeholder-content-for-dev-build"); | |
| } | |
| const rgExePath = path.join(dir, "rg.exe"); | |
| if (!fs.existsSync(rgExePath)) { | |
| fs.writeFileSync(rgExePath, "placeholder-content-for-dev-build"); | |
| } |
1 participant
Summary
This PR is based almost entirely on the earlier work from PR #10786 by @shanevcantwell, titled: feat: shell integration-based terminal output capture for remote environments.
The original PR addressed the issue where the extension host runs on Windows with a remote workspace (SSH, WSL, Dev Container) and child process execution was incorrectly performed locally instead of on the remote machine. The existing fallback used
ide.runCommandwith no output capture, returning a hardcoded "Command completed" status with no actual remote output.In addition to applying Shane's solution, this PR also updates the repository to work with Node
22.22.3, which required changes to the dependency installation and packaging workflow.What this PR includes
@shanevcantwellfor the remote terminal shell integration solution.runCommandWithOutputIDE method using VS Code Shell Integration API (1.93+) for remote terminal execution and output capture.sendTextwithout double-execution.22.22.3and Node SEA-based binary bundling.Key changes
Remote shell integration support
runCommandWithOutputto the IDE interface and protocol layers:core/index.d.tscore/protocol/ide.tscore/protocol/messenger/messageIde.tscore/protocol/messenger/reverseMessageIde.tsextensions/vscode/src/extension/VsCodeMessenger.tsextensions/vscode/src/VsCodeIde.tscore/tools/implementations/runTerminalCommand.tscore/util/filesystem.tscore/util/paths.tscore/config/types.tscore/tools/implementations/runTerminalCommand.vitest.tsNode and install workflow updates
package.jsonandbinary/package.jsonfor new devDependencies and runtime requirements.binary/utils/bundle-binary.js.binary/build.js.scripts/install-dependencies.sh@vscode/ripgrepand temporary install behavior:extensions/vscode/scripts/install-copy-nodemodule.jsTests and validation
22.22.3compatible install flow and packaging changes.Why this matters
The original issue affected remote development workflows where commands executed by the extension host were incorrectly routed to the local machine. This PR delivers the intended fix by capturing remote terminal output with VS Code shell integration, while preserving ANSI formatting and providing a safe fallback.
The Node
22.22.3update is also important because it aligns the repository with the newer runtime and packaging expectations, and it required corresponding install and bundle adjustments.Notes
This PR is intentionally built on top of Shane's original shell integration approach.
The Node upgrade is a complementary change needed to make the updated install and packaging flow work reliably.
The new output capture preserves ANSI colors and strips only VS Code internal OSC markers.
When shell integration is unavailable, the command still executes and the caller receives a sensible completion message without duplicate execution.
Some files were automatically updated by Prettier formatting as part of these changes. These formatting-only edits are non-functional and should be considered separate from the behavioral changes above.
Summary by cubic
Fixes remote tool execution in SSH/WSL/dev containers by running commands on the remote machine and capturing output via VS Code shell integration. Also upgrades to Node 22.22.3, switches the binary to Node SEA packaging, and adjusts
@vscode/ripgrephandling for reliable builds.Bug Fixes
runCommandWithOutput(uses VS Code Shell Integration; falls back tosendTextwhen unavailable).Migration
.nvmrc,.node-version,enginesupdated).postject,ncp).@vscode/ripgrepand updates packaging: dev packaging writes placeholderrg/rg.exeto satisfy VS Code validation on Windows; copy/cleanup scripts adjusted.Written for commit 88a39f0. Summary will update on new commits. Review in cubic