Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the skill generator template to invoke tools via npx @toolbox-sdk/server instead of a local binary and adds a custom execution path for Windows. Feedback includes removing the now-unused execSync import, addressing the regression in local development support, and replacing the fragile Windows bypass logic with the standard shell: true option in spawn for better reliability.
| @@ -124,37 +124,11 @@ const nodeScriptTemplate = `#!/usr/bin/env node | |||
| const { spawn, execSync } = require('child_process'); | |||
There was a problem hiding this comment.
execSync is no longer used in the script template after the removal of the getToolboxPath function. It should be removed from the require statement to keep the script clean.
| const { spawn, execSync } = require('child_process'); | |
| const { spawn } = require('child_process'); |
| } | ||
|
|
||
| const args = process.argv.slice(2); | ||
| const npxArgs = ["--yes", "@toolbox-sdk/server", "--log-level", "error", ...configArgs, "invoke", toolName, "--user-agent-metadata", userAgent, ...args]; |
There was a problem hiding this comment.
Switching to npx @toolbox-sdk/server removes the ability to use a local toolbox binary for tool invocation, which was previously supported (especially via the GEMINI_CLI=1 environment variable). This is a regression for developers who want to test local changes to the toolbox without publishing to npm. Consider allowing the package source or version to be configurable, or restoring the logic to check for a local binary first.
| let command = 'npx'; | ||
| let spawnArgs = npxArgs; | ||
|
|
||
| // The Windows Dependency-Free Bypass | ||
| if (os.platform() === 'win32') { | ||
| const nodeDir = path.dirname(process.execPath); | ||
| const npxCliJs = path.join(nodeDir, 'node_modules', 'npm', 'bin', 'npx-cli.js'); | ||
|
|
||
| if (fs.existsSync(npxCliJs)) { | ||
| command = process.execPath; | ||
| spawnArgs = [npxCliJs, ...npxArgs]; | ||
| } else { | ||
| console.error("Error: Could not find the npx executable to launch."); | ||
| process.exit(1); | ||
| } | ||
| } | ||
|
|
||
| const toolboxArgs = ["--log-level", "error", ...configArgs, "invoke", toolName, "--user-agent-metadata", userAgent, ...args]; | ||
|
|
||
| const child = spawn(toolboxBinary, toolboxArgs, { stdio: 'inherit', env }); | ||
| const child = spawn(command, spawnArgs, { stdio: 'inherit', env }); |
There was a problem hiding this comment.
The "Windows Dependency-Free Bypass" logic is fragile as it assumes a specific directory structure for the Node.js installation to find npx-cli.js. This is likely to fail on systems using version managers (like NVM) or non-standard installation paths. A more robust and standard approach for Windows is to invoke npx using the shell: true option, which correctly handles the .cmd extension and path resolution while avoiding fragile path discovery.
| let command = 'npx'; | |
| let spawnArgs = npxArgs; | |
| // The Windows Dependency-Free Bypass | |
| if (os.platform() === 'win32') { | |
| const nodeDir = path.dirname(process.execPath); | |
| const npxCliJs = path.join(nodeDir, 'node_modules', 'npm', 'bin', 'npx-cli.js'); | |
| if (fs.existsSync(npxCliJs)) { | |
| command = process.execPath; | |
| spawnArgs = [npxCliJs, ...npxArgs]; | |
| } else { | |
| console.error("Error: Could not find the npx executable to launch."); | |
| process.exit(1); | |
| } | |
| } | |
| const toolboxArgs = ["--log-level", "error", ...configArgs, "invoke", toolName, "--user-agent-metadata", userAgent, ...args]; | |
| const child = spawn(toolboxBinary, toolboxArgs, { stdio: 'inherit', env }); | |
| const child = spawn(command, spawnArgs, { stdio: 'inherit', env }); | |
| const child = spawn('npx', npxArgs, { | |
| stdio: 'inherit', | |
| env, | |
| shell: os.platform() === 'win32' | |
| }); |
|
Testing it for macOS |
2 participants
No description provided.