You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
grep failure exits script under set -eo pipefail: When .npmrc has no prefix= line, the grep in clean_npmrc_conflict returns exit code 1, causing the script to terminate before reaching install_qwen_code. This meant users with a clean .npmrc saw the script stop after Node.js detection and never install qwen.
User-configured .npmrc prefix gets overwritten: The script unconditionally removed any prefix= line from .npmrc regardless of whether it pointed to a valid user directory (e.g. ~/.local). The prefix was then reset to ~/.npm-global, breaking users who intentionally configured a custom prefix.
PATH set before prefix change causes old binary to run: The PATH was prepended with the npm global bin beforefix_npm_permissions potentially changed the prefix. If the prefix switched from an nvm path to ~/.npm-global, the qwen command still resolved to the old nvm binary because its directory was already in PATH ahead of the new one. Additionally, old qwen installations from other prefixes were never cleaned up.
Key reviewer notes:
The PATH reordering is the most impactful change — make sure the new order (fix permissions → set PATH → clean old → install) makes sense for your environment
The .npmrc change is backwards compatible — users without a custom prefix get the same behavior as before
Screenshots / Video Demo
No UI changes
Dive Deeper
Root cause analysis
The original issue reported that after running the installation script, qwen --version returned an old version (e.g. 0.14.0-preview.0) instead of the newly installed one (0.14.0-preview.4). Investigation revealed three compounding issues:
Bug 1: grep exit code kills the script
With set -eo pipefail, grep -E '^prefix *= *' returning no match (exit code 1) causes the entire pipeline to fail, triggering the set -e exit. This happens on any machine where .npmrc exists but doesn't have a prefix= line.
Bug 2: Aggressive .npmrc cleanup clean_npmrc_conflict removed anyprefix= line, even valid user-configured ones like prefix=$HOME/.local. The fix now checks if the prefix exists and, if so, respects it rather than overwriting it.
Bug 3: PATH timing mismatch
The old flow was:
set PATH (using old prefix) → fix_npm_permissions (may change prefix) → install
If fix_npm_permissions changed the prefix from ~/.nvm/... to ~/.npm-global, the PATH still had the old nvm bin directory prepended, so qwen resolved to the stale binary.
The new flow:
fix_npm_permissions (establish final prefix) → set PATH (using new prefix) → clean_old_installations → install
Test 1: Script runs to completion with clean .npmrc
# Backup your real .npmrc first
cp ~/.npmrc ~/.npmrc.backup
echo"registry=https://registry.npmjs.org">~/.npmrc
bash scripts/installation/install-qwen-with-source.sh
# Verify: script should not exit early, qwen should be installed# Restore
mv ~/.npmrc.backup ~/.npmrc
This PR addresses three critical bugs in the installation script that prevented proper qwen installation and caused version resolution issues. The changes are well-structured and address real root causes identified through thorough investigation. Overall, this is a solid fix that improves reliability for users with various .npmrc configurations.
🔍 General Feedback
The PR description provides excellent root cause analysis with clear before/after flow diagrams
The three bugs addressed are distinct but interrelated, and the fixes are appropriately separated
Good use of logging to inform users about what's happening (especially for the preserved prefix case)
The execution flow reordering (fix permissions → set PATH → clean old → install) is logical and correct
Shell scripting best practices are generally followed (proper quoting, error handling with || true)
🎯 Specific Feedback
🔴 Critical
File: scripts/installation/install-qwen-with-source.sh:165-185 - The grep command in clean_npmrc_conflict still has a potential issue. While the PR description mentions the grep exit code problem, the fix only addresses the prefix case. The grep -Eq '^globalconfig *= *' on line 177 can still return exit code 1 if no match is found, which with set -eo pipefail could cause issues. The || true is missing here.
Recommendation: Add || true to the globalconfig grep check:
if grep -Eq '^globalconfig *= *'"${npmrc}"2>/dev/null;then
File: scripts/installation/install-qwen-with-source.sh:443-480 - In clean_old_qwen_installations, the function iterates through known prefixes including /usr/local, but attempts to run npm uninstall on these paths. If the user doesn't have write permissions to /usr/local, this will fail silently (due to || true), but may leave users confused about why old installations persist. Additionally, running npm commands with different prefixes could be slow.
Recommendation: Add a permission check before attempting uninstall, or at least log when skipping due to permissions:
elif [[ -f"${prefix}/bin/qwen" ]];thenif [[ -w"${prefix}" ]];then
log_warning "Removing old qwen from ${prefix}/bin/qwen"
npm uninstall -g @qwen-code/qwen-code --prefix "${prefix}"2>/dev/null ||trueelse
log_warning "Skipping old qwen at ${prefix}/bin/qwen (no write permission)"fi
qwen_found=true
fi
🟢 Medium
File: scripts/installation/install-qwen-with-source.sh:170-173 - The sed pipeline for extracting the prefix value could be simplified and made more robust using bash parameter expansion:
existing_prefix=$(grep -E '^prefix *= *'"${npmrc}"2>/dev/null | head -1 | sed 's/^prefix *= *//'| sed 's/[[:space:]]*$//'|| true)
Could be:
local prefix_line
prefix_line=$(grep -E '^prefix *= *'"${npmrc}"2>/dev/null | head -1 || true)if [[ -n"${prefix_line}" ]];then
existing_prefix="${prefix_line#*=}"
existing_prefix="${existing_prefix#"${existing_prefix%%[![:space:]]*}"}"
existing_prefix="${existing_prefix%"${existing_prefix##*[![:space:]]}"}"fi
File: scripts/installation/install-qwen-with-source.sh:609-617 - The PATH cleaning logic in main() removes nvm paths that contain qwen, but this runs before fix_npm_permissions potentially changes the prefix. This means the PATH might be cleaned correctly here, but then the new prefix PATH is added on line 618, which is correct. However, the comment "use current prefix" is misleading since the prefix might change.
Recommendation: Update the comment to clarify the timing:
# Ensure NVM and npm global bin are in PATH (prefix may be updated later by fix_npm_permissions)
🔵 Low
File: scripts/installation/install-qwen-with-source.sh:178 - The backup message says "Backed up original .npmrc" but this happens even when only cleaning globalconfig (not prefix). Consider making the message more specific about what's being changed.
File: scripts/installation/install-qwen-with-source.sh:452 - The array known_prefixes is hardcoded. Consider making this configurable or documenting why these specific paths are chosen.
File: scripts/installation/install-qwen-with-source.sh:513 - The verification log now includes the path: log_info "Qwen Code version: ${qwen_version} (${qwen_path})". This is great! Consider also logging this in the "already installed" case on line 447 for consistency.
✅ Highlights
Excellent PR description with clear root cause analysis and testing instructions
The fix for respecting user-configured .npmrc prefix is a significant improvement in user experience
The new clean_old_qwen_installations function addresses a real pain point with stale binaries
Proper sequencing of operations (permissions → PATH → cleanup → install) shows good understanding of the dependency chain
The backup of .npmrc before modification is a good safety measure
Good use of logging levels (log_info, log_warning, log_success) to communicate what's happening to users
For detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TLDR
grepfailure exits script underset -eo pipefail: When.npmrchas noprefix=line, thegrepinclean_npmrc_conflictreturns exit code 1, causing the script to terminate before reachinginstall_qwen_code. This meant users with a clean.npmrcsaw the script stop after Node.js detection and never install qwen.User-configured
.npmrcprefix gets overwritten: The script unconditionally removed anyprefix=line from.npmrcregardless of whether it pointed to a valid user directory (e.g.~/.local). The prefix was then reset to~/.npm-global, breaking users who intentionally configured a custom prefix.PATH set before prefix change causes old binary to run: The PATH was prepended with the npm global bin before
fix_npm_permissionspotentially changed the prefix. If the prefix switched from an nvm path to~/.npm-global, theqwencommand still resolved to the old nvm binary because its directory was already in PATH ahead of the new one. Additionally, old qwen installations from other prefixes were never cleaned up.Key reviewer notes:
.npmrcchange is backwards compatible — users without a custom prefix get the same behavior as beforeScreenshots / Video Demo
No UI changes
Dive Deeper
Root cause analysis
The original issue reported that after running the installation script,
qwen --versionreturned an old version (e.g.0.14.0-preview.0) instead of the newly installed one (0.14.0-preview.4). Investigation revealed three compounding issues:Bug 1:
grepexit code kills the scriptWith
set -eo pipefail,grep -E '^prefix *= *'returning no match (exit code 1) causes the entire pipeline to fail, triggering theset -eexit. This happens on any machine where.npmrcexists but doesn't have aprefix=line.Bug 2: Aggressive
.npmrccleanupclean_npmrc_conflictremoved anyprefix=line, even valid user-configured ones likeprefix=$HOME/.local. The fix now checks if the prefix exists and, if so, respects it rather than overwriting it.Bug 3: PATH timing mismatch
The old flow was:
If
fix_npm_permissionschanged the prefix from~/.nvm/...to~/.npm-global, the PATH still had the old nvm bin directory prepended, soqwenresolved to the stale binary.The new flow:
Reviewer Test Plan
Quick syntax check
Test 1: Script runs to completion with clean
.npmrcTest 2: Custom prefix is preserved
Testing Matrix
Linked issues / bugs
#2810