-
Notifications
You must be signed in to change notification settings - Fork 3.1k
fix(cli): keep sudo-required npm installs on npm instead of migrating to standalone #5207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -597,4 +597,31 @@ describe('getInstallationInfo', () => { | |
| const infoDisabled = getInstallationInfo(projectRoot, false); | ||
| expect(infoDisabled.updateMessage).toContain('Please run npm install'); | ||
| }); | ||
|
|
||
| it('should ask for sudo and NOT migrate to standalone when the npm global prefix is not writable', () => { | ||
| const globalPath = `/usr/lib/node_modules/@qwen-code/qwen-code/cli-entry.js`; | ||
| process.argv[1] = globalPath; | ||
| mockedRealPathSync.mockReturnValue(globalPath); | ||
| mockedExecSync.mockImplementation(() => { | ||
| throw new Error('Command failed'); | ||
| }); | ||
| // npm global package dir is not writable -> `npm install -g` would need sudo. | ||
| vi.mocked(fs.accessSync).mockImplementation(() => { | ||
| throw Object.assign(new Error('EACCES: permission denied'), { | ||
| code: 'EACCES', | ||
| }); | ||
| }); | ||
|
|
||
| const info = getInstallationInfo(projectRoot, true); | ||
|
|
||
| expect(info.packageManager).toBe(PackageManager.NPM); | ||
| expect(info.isGlobal).toBe(true); | ||
| // Must NOT silently migrate to the standalone installer (bundled Node can be | ||
| // incompatible with the host, e.g. an older glibc). | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] This test only exercises Consider adding a companion assertion under the same EACCES mock: // Same scenario but with auto-update disabled — should still
// recommend sudo and NOT silently migrate to standalone.
const infoDisabled = getInstallationInfo(projectRoot, false);
expect(infoDisabled.packageManager).toBe(PackageManager.NPM);
expect(infoDisabled.isGlobal).toBe(true);
expect(infoDisabled.isStandalone).toBeUndefined();
expect(infoDisabled.standaloneDir).toBeUndefined();
expect(infoDisabled.updateCommand).toBeUndefined();
expect(infoDisabled.updateMessage).toContain('sudo');— qwen3.7-max via Qwen Code /review |
||
| expect(info.isStandalone).toBeUndefined(); | ||
| expect(info.standaloneDir).toBeUndefined(); | ||
| // No updateCommand -> the auto-updater won't attempt an unattended sudo. | ||
| expect(info.updateCommand).toBeUndefined(); | ||
| expect(info.updateMessage).toContain('sudo'); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,6 @@ | |
|
|
||
| import { createDebugLogger, isGitRepository } from '@qwen-code/qwen-code-core'; | ||
| import * as fs from 'node:fs'; | ||
| import * as os from 'node:os'; | ||
| import * as path from 'node:path'; | ||
| import * as childProcess from 'node:child_process'; | ||
|
|
||
|
|
@@ -177,7 +176,8 @@ export function getInstallationInfo( | |
| }; | ||
| } | ||
|
|
||
| // Check if the package directory is writable to determine whether npm update requires sudo | ||
| // Check if the npm global package directory is writable to determine | ||
| // whether `npm install -g` would require sudo. | ||
| const npmPackageDir = path.dirname(path.dirname(realPath)); | ||
| let npmPrefixWritable = false; | ||
| try { | ||
|
|
@@ -187,32 +187,18 @@ export function getInstallationInfo( | |
| // Not writable (e.g., /usr/local/lib/node_modules owned by root) | ||
| } | ||
|
|
||
| if (!npmPrefixWritable && isAutoUpdateEnabled) { | ||
| // npm prefix requires sudo — fall back to standalone update path | ||
| // which installs to ~/.local/lib/qwen-code/ (user-writable) | ||
| const installRoot = process.env['HOME'] || os.homedir(); | ||
| if (!installRoot || installRoot === '/') { | ||
| // Cannot determine a safe user-writable location; skip migration | ||
| return { | ||
| packageManager: PackageManager.NPM, | ||
| isGlobal: true, | ||
| updateMessage: | ||
| 'Update requires sudo. Run: sudo npm install -g @qwen-code/qwen-code@latest', | ||
| }; | ||
| } | ||
| const fallbackStandaloneDir = path.join( | ||
| installRoot, | ||
| '.local', | ||
| 'lib', | ||
| 'qwen-code', | ||
| ); | ||
| if (!npmPrefixWritable) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The block comment explains why standalone migration was removed, but does not explain why Consider extending the comment to document the guard broadening: // The npm global prefix requires sudo. Do NOT silently migrate to the
// standalone installer here: that swaps in a bundled Node runtime which
// can be incompatible with the host (e.g. an older glibc), breaking users
// who were updating fine via npm. Keep npm installs on npm and ask the
// user to update with sudo instead. No updateCommand is returned so the
// auto-updater does not attempt an unattended sudo.
// NOTE: this guard intentionally omits `&& isAutoUpdateEnabled`. Even when
// auto-update is off, a non-writable npm prefix means `npm install -g`
// (without sudo) is not a valid update path for any consumer.— qwen3.7-max via Qwen Code /review |
||
| // The npm global prefix requires sudo. Do NOT silently migrate to the | ||
| // standalone installer here: that swaps in a bundled Node runtime which | ||
| // can be incompatible with the host (e.g. an older glibc), breaking users | ||
| // who were updating fine via npm. Keep npm installs on npm and ask the | ||
| // user to update with sudo instead. No updateCommand is returned so the | ||
| // auto-updater does not attempt an unattended sudo. | ||
| return { | ||
| packageManager: PackageManager.NPM, | ||
| isGlobal: true, | ||
| isStandalone: true, | ||
| standaloneDir: fallbackStandaloneDir, | ||
| updateMessage: | ||
| 'npm install requires sudo. Migrating to standalone installer for automatic updates.', | ||
| 'Update requires sudo. Please run: sudo npm install -g @qwen-code/qwen-code@latest', | ||
| }; | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.