fix: move ajv to dependencies and auto-install deps in install scripts#858
Conversation
`ajv` is required at runtime by the installer (`scripts/lib/install/config.js`) but was listed under `devDependencies`. This caused `Error: Cannot find module 'ajv'` when running `./install.sh` from a fresh git clone or via `npx`. - Move `ajv` from devDependencies to dependencies in package.json - Add auto `npm install` in install.sh when node_modules is missing - Add matching auto-install in install.ps1 for Windows parity
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe install wrappers (PowerShell and shell) now auto-run Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Wrapper as "install.{sh,ps1}"
participant FS as "filesystem (node_modules)"
participant NPM as "npm"
participant Node as "node (install-apply.js)"
User->>Wrapper: run installer
Wrapper->>FS: check for `node_modules` in script dir
alt node_modules missing
Wrapper->>NPM: npm install (--no-audit --no-fund --loglevel=error)
NPM-->>Wrapper: exit code
Wrapper->>FS: restore original cwd (try/finally)
end
Wrapper->>Node: node scripts/install-apply.js `@args`
Node-->>User: exit status
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Greptile SummaryThis PR fixes a "Cannot find module 'ajv'" error on fresh git clones by moving
Confidence Score: 4/5Safe to merge after fixing the dead exit $LASTEXITCODE in install.ps1 for correct exit-code propagation. The core bug (missing ajv in runtime deps) is correctly fixed, and the auto-install logic works as intended. The one remaining issue — exit $LASTEXITCODE being unreachable in the PowerShell script due to Write-Error throwing first — is a P2 concern about exit-code accuracy rather than correctness of the install flow itself. install.ps1 — exit $LASTEXITCODE on line 47 is dead code; replace Write-Error with Write-Host to let it run. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([User runs install.sh / install.ps1]) --> B{node_modules exists?}
B -- No --> C[npm install --no-audit --no-fund]
C -- success --> D[exec node install-apply.js]
C -- failure --> E([Exit with error])
B -- Yes --> D
D --> F([Installer runs])
style E fill:#f88,stroke:#c00
style F fill:#8f8,stroke:#080
Reviews (2): Last reviewed commit: "fix(install): stop after npm bootstrap f..." | Re-trigger Greptile |
| if [ ! -d "$SCRIPT_DIR/node_modules" ]; then | ||
| echo "[ECC] Installing dependencies..." | ||
| (cd "$SCRIPT_DIR" && npm install --no-audit --no-fund --loglevel=error) | ||
| fi |
There was a problem hiding this comment.
node_modules directory check may miss partial installs
The guard [ ! -d "$SCRIPT_DIR/node_modules" ] only triggers auto-install when node_modules does not exist at all. A stale or partial node_modules directory (e.g., created by a previously interrupted npm install) won't satisfy the condition, and the script will proceed directly to the Node entry-point, still hitting Cannot find module 'ajv'.
A more reliable sentinel is the package-lock or the specific package directory:
| if [ ! -d "$SCRIPT_DIR/node_modules" ]; then | |
| echo "[ECC] Installing dependencies..." | |
| (cd "$SCRIPT_DIR" && npm install --no-audit --no-fund --loglevel=error) | |
| fi | |
| if [ ! -d "$SCRIPT_DIR/node_modules" ] || [ ! -d "$SCRIPT_DIR/node_modules/ajv" ]; then | |
| echo "[ECC] Installing dependencies..." | |
| (cd "$SCRIPT_DIR" && npm install --no-audit --no-fund --loglevel=error) | |
| fi |
The same consideration applies to install.ps1 (line 39).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
install.ps1 (1)
37-44: Consider checkingnpm installexit code before proceeding.If
npm installfails (e.g., network error, missing npm), the script continues to run the installer anyway, which will likely fail with a confusing error. With$ErrorActionPreference = 'Stop', native command failures don't automatically throw.♻️ Proposed fix to handle npm install failure
if (-not (Test-Path -LiteralPath $nodeModules)) { Write-Host '[ECC] Installing dependencies...' Push-Location $scriptDir - try { & npm install --no-audit --no-fund --loglevel=error } + try { + & npm install --no-audit --no-fund --loglevel=error + if ($LASTEXITCODE -ne 0) { + Write-Error "npm install failed with exit code $LASTEXITCODE" + exit $LASTEXITCODE + } + } finally { Pop-Location } }Note: The static analysis warning about
Write-Hostis a false positive here—it's appropriate for user-facing progress messages in an installer script.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.ps1` around lines 37 - 44, The install block that runs npm install (checking $nodeModules and using Push-Location/$scriptDir) currently ignores failures from the native npm command; update the try/finally to capture and check the npm exit status (use $LASTEXITCODE or $? after the & npm install call) and if it indicates failure, log an error with Write-Host or Write-Error including the exit code and stop the script (e.g., throw or exit 1) so the installer does not continue on failed dependency installation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@install.ps1`:
- Around line 37-44: The install block that runs npm install (checking
$nodeModules and using Push-Location/$scriptDir) currently ignores failures from
the native npm command; update the try/finally to capture and check the npm exit
status (use $LASTEXITCODE or $? after the & npm install call) and if it
indicates failure, log an error with Write-Host or Write-Error including the
exit code and stop the script (e.g., throw or exit 1) so the installer does not
continue on failed dependency installation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b9aec56-83a4-406e-a5a4-f4ec81d7cb2b
📒 Files selected for processing (3)
install.ps1install.shpackage.json
There was a problem hiding this comment.
1 issue found across 3 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="install.ps1">
<violation number="1" location="install.ps1:42">
P2: `npm install` failures are not checked, so a non-zero exit can be ignored and the script continues into `node install-apply.js` with missing dependencies.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
I really need help |
There was a problem hiding this comment.
1 issue found across 1 file (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="install.ps1">
<violation number="1" location="install.ps1:45">
P2: `Write-Error` is terminating under `$ErrorActionPreference = 'Stop'`, so the script can abort before `exit $LASTEXITCODE`, losing npm’s exit code.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| try { | ||
| & npm install --no-audit --no-fund --loglevel=error | ||
| if ($LASTEXITCODE -ne 0) { | ||
| Write-Error "npm install failed with exit code $LASTEXITCODE" |
There was a problem hiding this comment.
P2: Write-Error is terminating under $ErrorActionPreference = 'Stop', so the script can abort before exit $LASTEXITCODE, losing npm’s exit code.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At install.ps1, line 45:
<comment>`Write-Error` is terminating under `$ErrorActionPreference = 'Stop'`, so the script can abort before `exit $LASTEXITCODE`, losing npm’s exit code.</comment>
<file context>
@@ -39,7 +39,13 @@ $nodeModules = Join-Path -Path $scriptDir -ChildPath 'node_modules'
+ try {
+ & npm install --no-audit --no-fund --loglevel=error
+ if ($LASTEXITCODE -ne 0) {
+ Write-Error "npm install failed with exit code $LASTEXITCODE"
+ exit $LASTEXITCODE
+ }
</file context>
| Write-Error "npm install failed with exit code $LASTEXITCODE" | |
| Write-Error -ErrorAction Continue "npm install failed with exit code $LASTEXITCODE" |
…v-dependency fix: move ajv to dependencies and auto-install deps in install scripts
…v-dependency fix: move ajv to dependencies and auto-install deps in install scripts
Summary
ajvfromdevDependenciestodependencies— it is required at runtime by the installer (scripts/lib/install/config.js) but was missing for end usersnpm installininstall.shwhennode_modulesis missing (fresh git clone)install.ps1for Windows parityFixes
Error: Cannot find module 'ajv'when running./install.shfrom a fresh clone.Type
Testing
./install.sh typescript pythonworks on a fresh clone without manualnpm installChecklist
Summary by cubic
Move
ajvto runtime deps and auto-install Node deps in the install scripts so fresh clones can run the installer without errors. Also stop the Windows installer ifnpm installfails.ajvfromdevDependenciestodependencies(used at runtime byscripts/lib/install/config.js).install.shandinstall.ps1, auto-runnpm installwhennode_modulesis missing; on Windows, exit early if the install fails.Written for commit 4eaee83. Summary will update on new commits.
Summary by CodeRabbit