fix: native host setup scripts#198
Conversation
🦋 Changeset detectedLatest commit: efd51ba The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces managed installation support for OpenCode to improve native host setup reliability. It adds OS-specific installer entrypoints, refactors the launcher with a spawn-plan abstraction that tracks launch sources, and integrates a preflight managed install attempt in setupOpencode. ChangesManaged OpenCode installs and launcher refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/scripts/installer/install-windows.bat`:
- Around line 1-9: The file install-windows.bat currently uses LF-only endings
which can break Windows batch parsing; convert install-windows.bat to use CRLF
line endings (ensure the file uses CRLF for every line) and verify the script
still contains the existing lines (setlocal, set "SCRIPT_DIR=%~dp0", call
"%SCRIPT_DIR%install.bat" %*, exit /b %ERRORLEVEL%) so the batch entrypoint
behavior is unchanged; update git attributes or the commit so .bat files are
saved with CRLF to prevent regression.
In `@packages/cli/scripts/README.md`:
- Around line 111-113: The example sets CALYCODE_VERSION and
CALYCODE_SKIP_NATIVE_HOST on the curl process so the installer won't see them;
change the command so the env vars are applied to the bash installer process
instead (e.g. export them before running, use bash -c "$(curl -fsSL ...)", or
pipe curl into bash with the env vars in front of bash like `curl -fsSL
https://links.calycode.com/install-cli-unix | CALYCODE_VERSION=1.2.3
CALYCODE_SKIP_NATIVE_HOST=1 bash -s --`), update the README example accordingly
so the installer reads CALYCODE_VERSION and CALYCODE_SKIP_NATIVE_HOST.
In `@packages/cli/src/commands/opencode/implementation.ts`:
- Around line 199-207: The current block in buildOpencodeSpawnPlan that calls
ensureManagedOpencodeInstalled(version) can throw and prevent falling back to
global or npx; wrap the managed-install attempt in a try/catch so that when
ensureManagedOpencodeInstalled throws you log or capture the error and then
continue instead of returning, allowing the function to evaluate the global and
npx branches; specifically modify the code around managedEnabled /
options?.ensureManagedInstall and the ensureManagedOpencodeInstalled call to
catch exceptions (preserve the error for telemetry/logging) and only return the
managed plan if installation succeeded, otherwise proceed to the existing
global/npx resolution logic (ensure displayCommand/opencodeArgs behavior remains
unchanged).
- Around line 145-148: The code currently calls execSync with a constructed
shell string (execSync(`npm install --no-save --prefix "${safeInstallDir}"
"${packageSpecifier}"`, ...)), which risks shell injection; update the import to
include execFileSync and replace both execSync invocations that run npm install
with execFileSync using an argument array (e.g., execFileSync('npm',
['install','--no-save','--prefix', safeInstallDir, packageSpecifier'], {...}))
so no shell quoting is required—locate uses by the symbol execSync and the
variables safeInstallDir and packageSpecifier and update the call sites and the
import (add execFileSync) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 057720b0-042d-43b5-bdd2-668c5ae89f10
📒 Files selected for processing (6)
.changeset/proud-boats-type.mdpackages/cli/scripts/README.mdpackages/cli/scripts/installer/install-unix.shpackages/cli/scripts/installer/install-windows.batpackages/cli/scripts/installer/install-windows.ps1packages/cli/src/commands/opencode/implementation.ts
| @echo off | ||
| REM Windows installer entrypoint. | ||
| REM Delegates to existing shared batch installer implementation. | ||
|
|
||
| setlocal | ||
| set "SCRIPT_DIR=%~dp0" | ||
|
|
||
| call "%SCRIPT_DIR%install.bat" %* | ||
| exit /b %ERRORLEVEL% |
There was a problem hiding this comment.
Use CRLF line endings for this batch entrypoint.
This file is committed with LF-only endings; .bat parsing can become unreliable in some Windows environments. Please convert it to CRLF before merge.
🧰 Tools
🪛 Blinter (1.0.112)
[error] 1-1: Unix line endings detected. Explanation: Batch file uses Unix line endings (LF-only) which can cause GOTO/CALL label parsing failures and script malfunction due to Windows batch parser 512-byte boundary bugs. Recommendation: Convert file to Windows line endings (CRLF). Use tools like dos2unix, notepad++, or configure git with 'git config core.autocrlf true'. Context: File uses Unix line endings (LF-only) - 9 LF sequences found
(E018)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/scripts/installer/install-windows.bat` around lines 1 - 9, The
file install-windows.bat currently uses LF-only endings which can break Windows
batch parsing; convert install-windows.bat to use CRLF line endings (ensure the
file uses CRLF for every line) and verify the script still contains the existing
lines (setlocal, set "SCRIPT_DIR=%~dp0", call "%SCRIPT_DIR%install.bat" %*, exit
/b %ERRORLEVEL%) so the batch entrypoint behavior is unchanged; update git
attributes or the commit so .bat files are saved with CRLF to prevent
regression.
| ```bash | ||
| CALYCODE_VERSION=1.2.3 CALYCODE_SKIP_NATIVE_HOST=1 curl -fsSL https://links.calycode.com/install-cli-unix | bash | ||
| ``` |
There was a problem hiding this comment.
Fix env-var example so variables reach the installer process.
The current example sets CALYCODE_VERSION/CALYCODE_SKIP_NATIVE_HOST on curl, not on bash, so the installer won’t read them.
Suggested doc fix
-CALYCODE_VERSION=1.2.3 CALYCODE_SKIP_NATIVE_HOST=1 curl -fsSL https://links.calycode.com/install-cli-unix | bash
+curl -fsSL https://links.calycode.com/install-cli-unix | CALYCODE_VERSION=1.2.3 CALYCODE_SKIP_NATIVE_HOST=1 bash📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```bash | |
| CALYCODE_VERSION=1.2.3 CALYCODE_SKIP_NATIVE_HOST=1 curl -fsSL https://links.calycode.com/install-cli-unix | bash | |
| ``` |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/scripts/README.md` around lines 111 - 113, The example sets
CALYCODE_VERSION and CALYCODE_SKIP_NATIVE_HOST on the curl process so the
installer won't see them; change the command so the env vars are applied to the
bash installer process instead (e.g. export them before running, use bash -c
"$(curl -fsSL ...)", or pipe curl into bash with the env vars in front of bash
like `curl -fsSL https://links.calycode.com/install-cli-unix |
CALYCODE_VERSION=1.2.3 CALYCODE_SKIP_NATIVE_HOST=1 bash -s --`), update the
README example accordingly so the installer reads CALYCODE_VERSION and
CALYCODE_SKIP_NATIVE_HOST.
| execSync(`npm install --no-save --prefix "${safeInstallDir}" "${packageSpecifier}"`, { | ||
| stdio: 'ignore', | ||
| env: process.env, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify dynamic string-based execSync usage in TypeScript sources
rg -nP --type=ts 'execSync\(\s*`'Repository: calycode/xano-tools
Length of output: 1190
🏁 Script executed:
# Read the file to see the exact context of lines 145-161
sed -n '140,165p' packages/cli/src/commands/opencode/implementation.tsRepository: calycode/xano-tools
Length of output: 849
🏁 Script executed:
# Also check the imports at the top of the file
head -20 packages/cli/src/commands/opencode/implementation.tsRepository: calycode/xano-tools
Length of output: 813
Switch to execFileSync with argument arrays to avoid shell injection risks.
The code uses execSync with dynamic shell strings and manual quote escaping (line 145 with safeInstallDir), which is vulnerable to injection and error-prone. Use execFileSync with argument arrays instead:
-
Update the import to include
execFileSync:import { spawn, execSync, execFileSync } from 'node:child_process'; -
Replace both invocations (lines 145 and 158) with
execFileSyncusing argument arrays—this eliminates the need for manual escaping and shell interpretation.
Suggested fix
-import { spawn, execSync } from 'node:child_process';
+import { spawn, execSync, execFileSync } from 'node:child_process';
@@
- execSync(`npm install --no-save --prefix "${safeInstallDir}" "${packageSpecifier}"`, {
+ execFileSync('npm', ['install', '--no-save', '--prefix', installDir, packageSpecifier], {
stdio: 'ignore',
env: process.env,
});
@@
- execSync(`xattr -dr com.apple.quarantine "${installDir}"`, {
+ execFileSync('xattr', ['-dr', 'com.apple.quarantine', installDir], {
stdio: 'ignore',
- shell: true,
});🧰 Tools
🪛 OpenGrep (1.20.0)
[ERROR] 145-148: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/commands/opencode/implementation.ts` around lines 145 - 148,
The code currently calls execSync with a constructed shell string (execSync(`npm
install --no-save --prefix "${safeInstallDir}" "${packageSpecifier}"`, ...)),
which risks shell injection; update the import to include execFileSync and
replace both execSync invocations that run npm install with execFileSync using
an argument array (e.g., execFileSync('npm', ['install','--no-save','--prefix',
safeInstallDir, packageSpecifier'], {...})) so no shell quoting is
required—locate uses by the symbol execSync and the variables safeInstallDir and
packageSpecifier and update the call sites and the import (add execFileSync)
accordingly.
| if (managedEnabled && options?.ensureManagedInstall !== false) { | ||
| const installedBin = ensureManagedOpencodeInstalled(version); | ||
| return { | ||
| command: installedBin, | ||
| args: opencodeArgs, | ||
| source: 'managed', | ||
| displayCommand: `${installedBin} ${opencodeArgs.join(' ')}`.trim(), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Managed-install failure currently prevents global/npx fallback.
If ensureManagedOpencodeInstalled(version) throws here, buildOpencodeSpawnPlan exits early and never reaches the global or npx branches. That conflicts with the intended fallback behavior and can break launching when npm install fails.
Suggested fix
- if (managedEnabled && options?.ensureManagedInstall !== false) {
- const installedBin = ensureManagedOpencodeInstalled(version);
- return {
- command: installedBin,
- args: opencodeArgs,
- source: 'managed',
- displayCommand: `${installedBin} ${opencodeArgs.join(' ')}`.trim(),
- };
- }
+ if (managedEnabled && options?.ensureManagedInstall !== false) {
+ try {
+ const installedBin = ensureManagedOpencodeInstalled(version);
+ return {
+ command: installedBin,
+ args: opencodeArgs,
+ source: 'managed',
+ displayCommand: `${installedBin} ${opencodeArgs.join(' ')}`.trim(),
+ };
+ } catch {
+ // Continue to global/npx fallback.
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (managedEnabled && options?.ensureManagedInstall !== false) { | |
| const installedBin = ensureManagedOpencodeInstalled(version); | |
| return { | |
| command: installedBin, | |
| args: opencodeArgs, | |
| source: 'managed', | |
| displayCommand: `${installedBin} ${opencodeArgs.join(' ')}`.trim(), | |
| }; | |
| } | |
| if (managedEnabled && options?.ensureManagedInstall !== false) { | |
| try { | |
| const installedBin = ensureManagedOpencodeInstalled(version); | |
| return { | |
| command: installedBin, | |
| args: opencodeArgs, | |
| source: 'managed', | |
| displayCommand: `${installedBin} ${opencodeArgs.join(' ')}`.trim(), | |
| }; | |
| } catch { | |
| // Continue to global/npx fallback. | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/commands/opencode/implementation.ts` around lines 199 - 207,
The current block in buildOpencodeSpawnPlan that calls
ensureManagedOpencodeInstalled(version) can throw and prevent falling back to
global or npx; wrap the managed-install attempt in a try/catch so that when
ensureManagedOpencodeInstalled throws you log or capture the error and then
continue instead of returning, allowing the function to evaluate the global and
npx branches; specifically modify the code around managedEnabled /
options?.ensureManagedInstall and the ensureManagedOpencodeInstalled call to
catch exceptions (preserve the error for telemetry/logging) and only return the
managed plan if installation succeeded, otherwise proceed to the existing
global/npx resolution logic (ensure displayCommand/opencodeArgs behavior remains
unchanged).
1 participant
Summary by CodeRabbit
Bug Fixes
New Features
Documentation