deploy: resolve npm via agent's embedded runtime in update-release#184
deploy: resolve npm via agent's embedded runtime in update-release#184benvinegar wants to merge 5 commits intomainfrom
Conversation
update-release.sh runs as root (sudo baudbot update) but the embedded Node/npm runtime is installed under baudbot_agent's home directory and is not on root's PATH. The previous code fell back to bare `npm` which violates the project rule against bare node/npm in root scripts and fails on systems where only the agent user has Node installed. Replace the inline fallback with a resolve_npm_bin() helper that tries: 1. Agent user's embedded runtime (/home/baudbot_agent/opt/node/bin) 2. SUDO_USER's home (mise, nvm, etc.) 3. Standard PATH (last resort) Dies with a clear error if npm cannot be found at all. Adds tests for the resolution logic (success + failure paths).
Greptile SummaryFixed npm resolution in Key improvements:
Minor gap:
The fix directly addresses the security-critical requirement from Confidence Score: 4/5
Important Files Changed
Last reviewed commit: 462c645 |
bin/update-release.test.sh
Outdated
| resolve_npm_bin() { | ||
| local candidate="" | ||
| local agent_home="/home/${BAUDBOT_AGENT_USER:-baudbot_agent}" | ||
| candidate="$(bb_resolve_runtime_node_bin_dir "$agent_home" 2>/dev/null || true)" | ||
| if [ -n "$candidate" ] && [ -x "$candidate/npm" ]; then | ||
| echo "$candidate/npm" | ||
| return 0 | ||
| fi | ||
| if command -v npm >/dev/null 2>&1; then | ||
| command -v npm | ||
| return 0 | ||
| fi | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
Test function is missing step 2 (SUDO_USER home resolution) from actual resolve_npm_bin() implementation. Consider either:
- Adding test coverage for SUDO_USER paths (
mise/nvm/.local/bin) - Clarifying comment to say "simplified version" instead of "extracted from"
Prompt To Fix With AI
This is a comment left during a code review.
Path: bin/update-release.test.sh
Line: 246-259
Comment:
Test function is missing step 2 (SUDO_USER home resolution) from actual `resolve_npm_bin()` implementation. Consider either:
- Adding test coverage for SUDO_USER paths (`mise`/`nvm`/`.local/bin`)
- Clarifying comment to say "simplified version" instead of "extracted from"
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Good catch — I updated the tests so the shared test copy of resolve_npm_bin() now includes the SUDO_USER resolution branch, and added a dedicated test (resolves npm from sudo user home) that exercises the mise shim fallback path.
Responded by pi-coding-agent using openai/gpt-5.
6d4cfdc to
cfc79cc
Compare
Problem
sudo baudbot updatefails to findnpmwhen installing bridge dependencies because Node is only installed forbaudbot_agent, not the admin user running sudo. The script fell back to barenpmwhich isn't on root's PATH.This violates the project rule in
bin/AGENTS.md:Fix
Replace the inline npm resolution in
install_release_bridge_dependencieswith a dedicatedresolve_npm_bin()helper that follows the same multi-source pattern used by thebaudbotCLI dispatcher:/home/baudbot_agent/opt/node/bin/npm.local/binDies with a clear error message if npm can't be found anywhere, instead of failing cryptically mid-install.
Changes
bin/update-release.sh— addedresolve_npm_bin(), replaced barenpmfallback withdieon failurebin/update-release.test.sh— added tests for npm resolution (success from fake agent home, failure when missing)Testing
6/6update-release tests pass59 files)15/15)