fix(cli): avoid headless browser open crashes#4716
fix(cli): avoid headless browser open crashes#4716wenshao merged 5 commits intoQwenLM:mainQwenLM/qwen-code:mainfrom he-yufeng:fix/headless-slash-command-openhe-yufeng/qwen-code:fix/headless-slash-command-openCopy head branch name to clipboard
Conversation
| command = browserCommand.command; | ||
| args = [...browserCommand.args, url]; | ||
| } else { | ||
| throw new Error('Invalid BROWSER environment variable'); |
There was a problem hiding this comment.
[Suggestion] throw new Error('Invalid BROWSER environment variable') violates the function's documented contract. The JSDoc at line 53 states: "this function does NOT throw an error… and resolves successfully to prevent application crashes." However, when parseBrowserCommand returns undefined (e.g., BROWSER='""'), this throw propagates to callers like bugCommand and insightCommand which rely on the non-throwing guarantee.
| throw new Error('Invalid BROWSER environment variable'); | |
| console.warn('Invalid BROWSER environment variable, falling back to platform default.'); | |
| // fall through to the platform switch below |
Alternatively, restructure the if/else so an invalid BROWSER simply falls through to the platform-specific switch rather than throwing.
— qwen3.7-max via Qwen Code /review
| const browserEnv = process.env['BROWSER']?.trim(); | ||
| const browserBlocklist = ['www-browser']; | ||
| if (browserEnv && !browserBlocklist.includes(browserEnv)) { | ||
| const browserCommand = parseBrowserCommand(browserEnv); |
There was a problem hiding this comment.
[Suggestion] browserBlocklist.includes(browserEnv) compares the full trimmed BROWSER value against 'www-browser'. A value like BROWSER="www-browser --headless" bypasses the blocklist because the full string doesn't match. The check should compare against the parsed command name instead:
const browserCommand = parseBrowserCommand(browserEnv);
if (browserCommand && !browserBlocklist.includes(browserCommand.command)) {
command = browserCommand.command;
args = [...browserCommand.args, url];
} else if (browserCommand && browserBlocklist.includes(browserCommand.command)) {
// blocked — fall through to platform switch
} else {
throw new Error('Invalid BROWSER environment variable');
}Also, the same ['www-browser'] array is duplicated in shouldLaunchBrowser() (line ~199). Consider extracting it to a module-level constant:
const BROWSER_BLOCKLIST = ['www-browser'];Additionally, when BROWSER is set on Linux and the specified command fails, the fallback chain (gnome-open, kde-open, firefox, etc.) is never attempted because the guard at line ~140 checks command === 'xdg-open'. Consider also triggering fallbacks when BROWSER was the primary source.
— qwen3.7-max via Qwen Code /review
| context.ui.addItem( | ||
| { | ||
| type: MessageType.INFO, | ||
| text: t('Opening insights in your browser: {{path}}', { |
There was a problem hiding this comment.
[Suggestion] Moving the "Opening insights" message before the try block creates a UX issue: when the browser fails to launch, the user sees the optimistic "Opening insights in your browser" message followed by a raw file:// URL on stderr from openBrowserSecurely's internal catch. The catch block below (which shows a friendlier filesystem-path fallback) is effectively dead code for browser-launch failures, because openBrowserSecurely swallows errors internally and resolves normally.
Consider either:
- Moving the "Opening" message back inside
tryafter theawait, or - Having
openBrowserSecurelysignal failure to the caller (e.g., return a boolean) soinsightCommandcan decide which message to show.
— qwen3.7-max via Qwen Code /review
| Date.now(), | ||
| ); | ||
| await open(docsUrl); | ||
| await openBrowserSecurely(docsUrl); |
There was a problem hiding this comment.
[Suggestion] Unlike bugCommand (line 57-66) and insightCommand (line 230-244), this call to openBrowserSecurely is not wrapped in a try/catch. While openBrowserSecurely catches browser-launch failures internally, validateUrl() inside it does throw for malformed URLs. Adding a defensive try/catch here would be consistent with the other two call sites and protect against unexpected URL construction issues.
| await openBrowserSecurely(docsUrl); | |
| try { | |
| await openBrowserSecurely(docsUrl); | |
| } catch (error) { | |
| const errorMessage = error instanceof Error ? error.message : String(error); | |
| context.ui.addItem( | |
| { | |
| type: MessageType.ERROR, | |
| text: t('Failed to open browser: {{error}}', { error: errorMessage }), | |
| }, | |
| Date.now(), | |
| ); | |
| } |
— qwen3.7-max via Qwen Code /review
| 'xdg-open', | ||
| ['file:///tmp/report.html'], | ||
| expect.any(Object), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] Several test scenarios for the new BROWSER env var feature are missing:
- Blocklisted value:
BROWSER=www-browsershould fall through to platform default (the blocklist branch is untested). - Quoted command paths:
parseBrowserCommand's quote-handling regex is the main value-add over a simple.split(' '), but no test covers paths likeBROWSER='"/path/to/my browser" --new-tab'. - Fallback suppression: No test verifies that Linux fallbacks are skipped when
BROWSERis set and fails.
Adding these would strengthen coverage of the new code paths.
— qwen3.7-max via Qwen Code /review
| const browserCommand = parseBrowserCommand(browserEnv); | ||
| if (browserCommand) { | ||
| command = browserCommand.command; | ||
| args = [...browserCommand.args, url]; |
There was a problem hiding this comment.
BROWSER values commonly use %s as the URL placeholder (for example BROWSER="firefox --new-tab %s"). With the current construction we keep %s as a literal argument and append the real URL after it, so the configured browser may try to open %s or receive two targets. Please substitute the URL into any %s argument, and only append url when no placeholder is present; add a test for that form.
|
Thanks for the contribution — the direction of replacing A few architectural points beyond the inline feedback from wenshao: 1. Missing The codebase already has a browser-availability detection flow: Without this, a headless container still runs through the full xdg-open → gnome-open → kde-open → firefox → chromium → google-chrome → microsoft-edge fallback chain before giving up. 2. Other callsites not addressed
3. Duplicated browser-check logic
4. Manual testing evidence Once the PR is updated, please add screenshots or a short recording showing the behavior in a headless Linux container (the core scenario from the issue) — both the success path (URL displayed, no crash) and the fallback path. This helps confirm the fix end-to-end before merge. For context — I had already analyzed this issue in detail and another contributor was looking into it as well. That said, I appreciate you picking this up. The points above are the main gaps I'd like to see addressed before we move forward. |
|
Updated the branch to address the browser-launch review points:
Validation run locally:
|
|
Pushed a small test-only follow-up for the CI failure on this branch. Root cause: the full CI job sets Change:
Validation run locally:
|
wenshao
left a comment
There was a problem hiding this comment.
[Suggestion] The open package ("open": "^10.1.2") remains in both packages/cli/package.json and packages/core/package.json despite zero remaining imports across the codebase. All callers now use openBrowserSecurely. Remove the dead dependency from both files and re-run npm install to update the lock file. (Posted as a body-level comment because package.json files are not in this PR's diff.)
— qwen3.7-max via Qwen Code /review
|
|
||
| try { | ||
| await execFileAsync(command, args, options); | ||
| await execFileAsync(command, args, execOptions); |
There was a problem hiding this comment.
[Critical] execFileAsync (promisified execFile) waits for the child process to exit even with detached: true. The detached option only calls setsid() on the child — it does NOT make the promise resolve early.
For the default platform commands (open on macOS, xdg-open on Linux, powershell Start-Process on Windows), this is fine because they exit immediately after launching the browser. But when BROWSER points to a browser binary (e.g., firefox, chromium), the process stays alive and await execFileAsync(...) blocks indefinitely.
This causes two critical failure modes:
- OAuth device flow deadlock (
qwenOAuth2.ts):launchBrowser()blocks, the token polling loop never starts, the device code expires. - TUI command freeze (
bugCommand,docsCommand,insightCommand): the command handler never returns.
| await execFileAsync(command, args, execOptions); | |
| try { | |
| // For the BROWSER path, use spawn + unref for true fire-and-forget | |
| // (execFileAsync would block until the long-running browser process exits) | |
| const { spawn } = await import('node:child_process'); | |
| const child = spawn(command, args, { | |
| detached: true, | |
| stdio: 'ignore', | |
| env: execOptions.env as NodeJS.ProcessEnv, | |
| }); | |
| child.unref(); | |
| } |
| ): Promise<void> { | ||
| // Validate the URL first | ||
| validateUrl(url); | ||
| validateUrl(url, browserOptions); |
There was a problem hiding this comment.
[Suggestion] shouldAttemptBrowserLaunch() is checked here BEFORE the BROWSER env var is evaluated (line 82). When a user explicitly sets BROWSER in a CI or headless environment (e.g., BROWSER=/usr/bin/chromium), shouldAttemptBrowserLaunch() returns false (because CI is set or no display is available), and the function exits early without ever reaching the BROWSER command logic.
An explicit BROWSER setting is a strong signal that the user intends browser launching to work. Consider evaluating BROWSER first; if it is set and not blocklisted, skip the shouldAttemptBrowserLaunch() gate so users can override headless detection.
|
|
||
| default: | ||
| throw new Error(`Unsupported platform: ${platformName}`); | ||
| const browserEnv = process.env['BROWSER']?.trim(); |
There was a problem hiding this comment.
[Suggestion] On Windows (win32), the BROWSER env var bypasses the PowerShell Start-Process path entirely. The BROWSER branch is evaluated before the platform switch, so the URL goes to execFile(parsedCommand, [...args, url]) directly — undoing the deliberate defense-in-depth decision to use Start-Process over cmd.exe to avoid shell injection (as noted in the comment on the win32 case below).
Consider either ignoring BROWSER on Windows (always use the PowerShell path) or restricting it to an allowlist of known browser binaries.
| } | ||
| } | ||
|
|
||
| function parseBrowserCommand( |
There was a problem hiding this comment.
[Suggestion] The regex /"[^"]*"|'[^']*'|[^\s]+/g only recognizes quotes at token boundaries. It cannot handle embedded quotes within compound tokens. For example, BROWSER='chromium --user-data-dir="/tmp/my profile"' splits into ["chromium", "--user-data-dir=\"/tmp/my", "profile\""] — two garbled arguments instead of one clean --user-data-dir=/tmp/my profile.
This is a realistic BROWSER configuration (Chrome/Chromium with a spaced user-data directory). Consider using a proper shell-like tokenizer that recognizes quote boundaries mid-token.
|
|
||
| // Only allow HTTP and HTTPS protocols | ||
| if (parsedUrl.protocol !== 'http:' && parsedUrl.protocol !== 'https:') { | ||
| const allowedProtocols = allowFile |
There was a problem hiding this comment.
[Suggestion] When allowFile is true, any file:// URL passes validation without path restriction — including file:///etc/shadow, file:///proc/self/environ, or file:///~/.ssh/id_rsa. The sole current caller (insightCommand) uses internally generated paths, so this is not exploitable today. However, this function is publicly exported from @qwen-code/qwen-code-core, and any future caller passing user-influenced paths with { allowFile: true } would create a local-file disclosure vector.
Consider adding an optional allowedBasePath parameter to constrain file:// URLs to a known output directory.
| // For non-Linux OSes, we generally assume a GUI is available | ||
| // unless other signals (like SSH) suggest otherwise. | ||
| return true; | ||
| return shouldAttemptBrowserLaunch(); |
There was a problem hiding this comment.
[Suggestion] shouldLaunchBrowser() is now a one-line wrapper delegating to shouldAttemptBrowserLaunch() and has zero callers anywhere in the codebase. It is still re-exported from index.ts via export *. Consider removing it entirely to reduce unnecessary public API surface.
| * @throws Error if the URL is invalid or uses an unsafe protocol | ||
| */ | ||
| function validateUrl(url: string): void { | ||
| function validateUrl(url: string, { allowFile = false } = {}): void { |
There was a problem hiding this comment.
[Suggestion] The JSDoc at line 16-17 states "Only allows HTTP and HTTPS URLs to prevent command injection" but the function now also permits file: when allowFile is true. The doc no longer matches the implementation — consider updating to reflect the allowFile parameter.
| default: | ||
| throw new Error(`Unsupported platform: ${platformName}`); | ||
| const browserEnv = process.env['BROWSER']?.trim(); | ||
| const browserBlocklist = ['www-browser']; |
There was a problem hiding this comment.
[Suggestion] browserBlocklist is duplicated between browser.ts (line 18, reads process.env['BROWSER'] raw) and this file (reads process.env['BROWSER']?.trim()). The inconsistent trimming means a whitespace-padded value like " www-browser " passes the blocklist in shouldAttemptBrowserLaunch() but matches it here, causing divergent behavior.
Consider removing the duplicate blocklist from openBrowserSecurely entirely (since shouldAttemptBrowserLaunch() already gates the launch) or extracting it into a shared constant with consistent .trim() in both files.
|
Updated in What changed:
I intentionally left Validation run locally: Notes:
|
91bed04 to
4c6734d
Compare
|
Pushed Additional changes:
I still intentionally left Validation on Windows:
|
4c6734d to
f7eb570
Compare
|
Addressed the dead dependency review note. What changed:
Validation on head
The build still reports the existing VS Code companion curly warnings and stale browserslist notices, but they are warnings only and unrelated to this patch. |
| for (const fallbackCommand of fallbackCommands) { | ||
| try { | ||
| await execFileAsync(fallbackCommand, [url], options); | ||
| await execFileAsync(fallbackCommand, [url], execOptions); |
There was a problem hiding this comment.
[Suggestion] The Linux platform-fallback path (lines 159–175) still uses execFileAsync for browser binaries (firefox, chromium, google-chrome, microsoft-edge). execFileAsync with detached: true blocks until the child process exits — the detached option only calls setsid(), it does NOT make the promise resolve early. This is the same deadlock class as the R2 Critical fix that was correctly applied to the BROWSER env var path via launchDetached.
When xdg-open is missing (common in containers, minimal servers, WSL) and a fallback binary is a real browser, openBrowserSecurely won't return until the user closes the browser. This blocks the calling slash command (/bug, /docs, /insight, /extensions) and the OAuth device-flow polling loop in qwenOAuth2.ts.
| await execFileAsync(fallbackCommand, [url], execOptions); | |
| await launchDetached(fallbackCommand, [url]); |
— qwen3.7-max via Qwen Code /review
| await launchDetached(browserCommand.command, browserCommand.args); | ||
| } catch (_error) { | ||
| /* eslint-disable no-console */ | ||
| console.warn( |
There was a problem hiding this comment.
[Suggestion] When launchDetached rejects (binary not found, permissions error, misparsed path), this catch block prints a warning and the return on the next line exits immediately — never falling through to the platform default (open on macOS, xdg-open on Linux).
A stale or slightly-off BROWSER env var completely disables browser launching. The most common real-world case: a macOS user with BROWSER="/Applications/Google Chrome.app/Contents/MacOS/Google Chrome" — the shell evaluates the outer quotes, leaving a bare string with spaces that parseBrowserCommand splits incorrectly.
Consider falling through to the platform-default path instead:
if (browserCommand) {
try {
await launchDetached(browserCommand.command, browserCommand.args);
return;
} catch (_error) {
// Fall through to platform default below
}
}— qwen3.7-max via Qwen Code /review
| if (browserCommand) { | ||
| try { | ||
| await launchDetached(browserCommand.command, browserCommand.args); | ||
| } catch (_error) { |
There was a problem hiding this comment.
[Suggestion] The caught error is discarded (_error). The warning doesn't mention the BROWSER env var, the binary name that was attempted, or the underlying OS error. An operator with a misconfigured BROWSER has no way to diagnose the failure from this message alone.
| } catch (_error) { | |
| console.warn( | |
| `Failed to open browser (BROWSER=${browserCommand.command}): ${_error instanceof Error ? _error.message : String(_error)}. Please open this URL manually: ${url}`, | |
| ); |
— qwen3.7-max via Qwen Code /review
| : undefined, | ||
| }); | ||
| mockOpenBrowserSecurely.mockClear(); | ||
| mockOpenBrowserSecurely.mockResolvedValue(undefined); |
There was a problem hiding this comment.
[Suggestion] docsCommand and extensionsCommand both have tests for when openBrowserSecurely throws (e.g., mockOpenBrowserSecurely.mockRejectedValue(...)), but bugCommand only tests the success path. The catch block in bugCommand.ts (lines 60–68) that adds an ERROR-type history item is dead code from a testing perspective.
Consider adding:
it('should show an error message when browser opening fails', async () => {
mockOpenBrowserSecurely.mockRejectedValue(new Error('Browser not found'));
// ... invoke bugCommand and assert ERROR item
});— qwen3.7-max via Qwen Code /review
| const browserEnv = process.env['BROWSER']; | ||
| if (browserEnv && browserBlocklist.includes(browserEnv)) { | ||
| const browserEnv = process.env['BROWSER']?.trim(); | ||
| const browserCommand = browserEnv?.match(/^\S+/)?.[0]; |
There was a problem hiding this comment.
[Suggestion] On Windows, BROWSER is intentionally excluded from the spawn path in secure-browser-launcher.ts (platformName === 'win32' ? undefined). But this function is still called as a fallback gate, and it reads BROWSER and checks the blocklist. If BROWSER=www-browser (common on Debian via update-alternatives), this returns false, preventing the PowerShell Start-Process opener from running.
The existing test only covers BROWSER='firefox --new-tab' (non-blocklisted), masking this gap. On Windows machines sharing dotfiles with a Linux environment that sets BROWSER=www-browser, the PowerShell-based browser launch silently does not run.
— qwen3.7-max via Qwen Code /review
| import { shouldAttemptBrowserLaunch } from './browser.js'; | ||
|
|
||
| const execFileAsync = promisify(execFile); | ||
| const browserBlocklist = ['www-browser']; |
There was a problem hiding this comment.
[Suggestion] browserBlocklist.includes(browserCommand.command) uses exact string matching. BROWSER=/usr/bin/www-browser (standard path on Debian via update-alternatives) bypasses this check because /usr/bin/www-browser !== 'www-browser'. Both browser.ts and this file have the same issue — the blocklist is ineffective for absolute-path BROWSER values.
Consider normalizing via path.basename():
import { basename } from 'node:path';
if (browserBlocklist.includes(basename(browserCommand.command))) {
return undefined;
}— qwen3.7-max via Qwen Code /review
f7eb570 to
9b73b83
Compare
|
Pushed What changed:
Validation run locally: I also tried the broader CLI workspace typecheck locally after the cache cleanup. It is currently blocked by unrelated workspace build/export issues outside this browser-launch patch ( |
| ); | ||
| } | ||
|
|
||
| if (parsedUrl.protocol === 'file:' && allowFile && allowedFilePaths?.length) { |
There was a problem hiding this comment.
[Suggestion] When allowFile is true but allowedFilePaths is undefined or empty, the condition allowedFilePaths?.length is falsy, so the entire path-validation block is skipped — any file:// URL passes. While the current sole caller (insightCommand) always provides allowedFilePaths, the API is a footgun for future callers.
Consider throwing when allowFile is true but allowedFilePaths is missing or empty:
| if (parsedUrl.protocol === 'file:' && allowFile && allowedFilePaths?.length) { | |
| if (parsedUrl.protocol === 'file:' && allowFile) { | |
| if (!allowedFilePaths?.length) { | |
| throw new Error('allowedFilePaths is required when allowFile is true'); | |
| } | |
| const requestedPath = resolve(fileURLToPath(parsedUrl)); | |
| const allowed = allowedFilePaths.map((filePath) => resolve(filePath)); | |
| if (!allowed.includes(requestedPath)) { | |
| throw new Error('File URL is not in the allowed file set'); | |
| } | |
| } |
— qwen3.7-max via Qwen Code /review
| ); | ||
| }); | ||
|
|
||
| it('should restrict file URLs to the caller allow-list when provided', async () => { |
There was a problem hiding this comment.
[Suggestion] The allowedFilePaths feature only has a rejection test here (path NOT in allow-list). The success path — where a file:// URL matches an allowedFilePaths entry and proceeds to launch — is not exercised in this test file. A regression that accidentally rejects valid allowed-file URLs would go undetected.
Consider adding a positive test:
it('should allow file URLs that match the allowedFilePaths list', async () => {
setPlatform('linux');
vi.stubEnv('DISPLAY', ':1');
const filePath = resolve('report.html');
await openBrowserSecurely(pathToFileURL(filePath).href, {
allowFile: true,
allowedFilePaths: [filePath],
});
expect(mockExecFile).toHaveBeenCalledWith(
'xdg-open',
[pathToFileURL(filePath).href],
expect.any(Object),
);
});— qwen3.7-max via Qwen Code /review
| expect(mockSpawn).toHaveBeenCalledWith( | ||
| 'firefox', | ||
| ['--new-tab', 'https://example.com'], | ||
| expect.any(Object), |
There was a problem hiding this comment.
[Suggestion] Every mockSpawn assertion uses expect.any(Object) for the third argument (spawn options). The security-critical properties of launchDetached — detached: true, stdio: 'ignore', and env.SHELL: undefined — are never explicitly verified. If a refactor accidentally removes detached: true or changes stdio, no test would catch it.
At least one test should assert the spawn options explicitly:
expect(mockSpawn).toHaveBeenCalledWith(
'firefox',
['--new-tab', 'https://example.com'],
expect.objectContaining({
detached: true,
stdio: 'ignore',
}),
);— qwen3.7-max via Qwen Code /review
Local Verification ReportBranch: TypeScript Compilation
Test Results
CI StatusAll CI checks passed:
Code Review SummaryWhat this PR does:
Key observations:
Verdict: Ready to merge — All PR-related tests pass, TSC regressions are zero (local build artifact only), CI all green. Clean security-conscious refactor removing an external dependency. |
| ); | ||
| } | ||
|
|
||
| if (parsedUrl.protocol === 'file:' && allowFile && allowedFilePaths?.length) { |
There was a problem hiding this comment.
[Suggestion] allowedFilePaths?.length evaluates to 0 (falsy) when allowedFilePaths is [], which silently skips the entire check — equivalent to passing no restriction. Semantically, an empty array should mean "no files allowed."
| if (parsedUrl.protocol === 'file:' && allowFile && allowedFilePaths?.length) { | |
| if (parsedUrl.protocol === 'file:' && allowFile && allowedFilePaths !== undefined) { |
— qwen3.7-max via Qwen Code /review
|
|
||
| for (const fallbackCommand of fallbackCommands) { | ||
| for (const { command: fallbackCommand, detached } of fallbackCommands) { | ||
| try { |
There was a problem hiding this comment.
[Suggestion] When BROWSER=firefox fails (ENOENT), the code falls through to xdg-open → gnome-open → kde-open → firefox again (line 182). The retry of the same binary is guaranteed to fail with the same error. Consider tracking which command was already tried:
| try { | |
| const alreadyTried = browserCommand?.command | |
| .replace(/\\/g, '/').split('/').pop(); | |
| for (const { command: fallbackCommand, detached } of fallbackCommands) { | |
| if (fallbackCommand === alreadyTried) continue; |
— qwen3.7-max via Qwen Code /review
| @@ -116,17 +178,21 @@ export async function openBrowserSecurely(url: string): Promise<void> { | ||
| command === 'xdg-open' | ||
| ) { | ||
| const fallbackCommands = [ |
There was a problem hiding this comment.
[Suggestion] The detached: false vs detached: true split is intentional (desktop openers exit quickly; real browsers are long-running) but undocumented. A future maintainer might "fix" the inconsistency by setting all entries to detached: true, re-introducing the deadlock this PR fixed. A one-liner comment prevents this:
| const fallbackCommands = [ | |
| // gnome-open/kde-open are short-lived dispatchers (exit after | |
| // handing off to the desktop handler) — execFileAsync is safe. | |
| // Real browsers are long-running and must use launchDetached. | |
| const fallbackCommands = [ |
— qwen3.7-max via Qwen Code /review
| const browserEnv = process.env['BROWSER']; | ||
| if (browserEnv && browserBlocklist.includes(browserEnv)) { | ||
| const browserEnv = process.env['BROWSER']?.trim(); | ||
| const browserCommand = browserEnv?.match(/^\S+/)?.[0]; |
There was a problem hiding this comment.
[Suggestion] shouldAttemptBrowserLaunch() parses BROWSER with /^\S+/ (preserves quotes), while buildBrowserCommand() uses the quote-aware parseBrowserCommand(). For BROWSER='"www-browser"', isBrowserCommandBlocked('"www-browser"') returns false — the blocklist check disagrees with buildBrowserCommand on command extraction. No security impact (platform opener opens a validated URL), but the inconsistency could bite if the blocklist grows. Consider reusing parseBrowserCommand or at minimum stripping surrounding quotes here.
— qwen3.7-max via Qwen Code /review
|
Thanks for the verification report. The earlier requested dead The local CLI typecheck delta in the report is from stale pre-built core declarations; the source export is present and CI typecheck passes after the workspace build path. This is ready for maintainer re-review from my side. |
| return arg; | ||
| } | ||
| usedPlaceholder = true; | ||
| return arg.replace(/%s/g, url); |
There was a problem hiding this comment.
[Suggestion] String.prototype.replace interprets special $-prefixed patterns in the replacement string: $& (matched substring), $' (text after match), $$ (literal $). When url is passed directly as the second argument, any URL containing these sequences is silently mangled.
Concrete example: OAuth callback URL https://auth.example.com/callback?code=$&state=abc becomes https://auth.example.com/callback?code=%s&state=abc (the $& is replaced with the matched text %s, re-introducing the placeholder literally).
Note: the codebase already has a safeLiteralReplace utility in packages/core/src/utils/textUtils.ts that handles exactly this pitfall.
| return arg.replace(/%s/g, url); | |
| return arg.replace(/%s/g, () => url); |
— qwen3.7-max via Qwen Code /review
|
|
||
| default: | ||
| throw new Error(`Unsupported platform: ${platformName}`); | ||
| if (!shouldAttemptBrowserLaunch()) { |
There was a problem hiding this comment.
[Suggestion] When BROWSER=www-browser (a blocklisted value), buildBrowserCommand() already returns undefined (line 96), so the BROWSER branch is correctly skipped. However, shouldAttemptBrowserLaunch() here ALSO checks the blocklist internally (in browser.ts:24-30) and returns false — which prevents the platform-opener fallback below (open on macOS, xdg-open on Linux with a display).
On macOS with BROWSER=www-browser, the system open command works perfectly (it ignores BROWSER entirely), but the user sees "Browser launch is not available in this environment" instead.
Consider splitting the blocklist concern: shouldAttemptBrowserLaunch() should only gate on environment availability (display server, CI, SSH), while buildBrowserCommand() handles BROWSER-specific filtering. The blocklist check in shouldAttemptBrowserLaunch() is needed for the OAuth flow (qwenOAuth2.ts) but should not short-circuit the platform-opener fallback here.
— qwen3.7-max via Qwen Code /review
9b73b83 to
d148e8a
Compare
|
Updated this branch to address the browser-launch review points. Changes made:
Validation:
Note: |
|
Pushed The failing job was not a browser assertion failure; all core tests passed, then Vitest failed the macOS run because
Validation on this branch:
Note: an earlier local attempt ran two core Vitest commands in parallel and hit a Windows coverage directory race ( |
| () => processInstance.stdin?.end(), | ||
| ); | ||
|
|
||
| processInstance.stdin.on('error', (err) => { |
There was a problem hiding this comment.
[Suggestion] The new stdin.on('error', ...) handler has no test coverage. The existing LspConnectionFactory.test.ts only covers stderr/exit during initialize and UTF-8 chunking — neither exercises a stdin stream error. This is new production error-handling code; if the connection.end() call is ever removed or altered, LSP connections could hang with pending requests that never resolve or reject.
Consider adding a test that spawns a process whose stdin emits an error, then verifies the connection ends and pending requests reject:
it('ends the connection when stdin emits an error', async () => {
const { connection } = await factory.createStdioConnection(
process.execPath,
['-e', 'process.stdin.destroy(new Error("stdin broken"));'],
);
await expect(connection.initialize({})).rejects.toThrow();
});— qwen3.7-max via Qwen Code /review
DragonnZhang
left a comment
There was a problem hiding this comment.
Review Summary
This PR has already received extensive review (30 inline comments across multiple rounds, 11 issue-level comments, and a detailed verification report from @wenshao). I performed a full diff review covering all 18 changed files and a deterministic analysis pass.
Deterministic analysis: 0 findings (tsc and eslint clean; 4 tools skipped for non-applicable languages).
Manual review: I examined every changed file in detail, including the core secure-browser-launcher.ts refactor (BROWSER env support, launchDetached with spawn/unref, quote-aware argument parser, allowedFilePaths allow-list for file:// URLs), the browser.ts blocklist extraction by basename, the LspConnectionFactory.ts resilience improvements, and all four command-file migrations from open to openBrowserSecurely.
Duplicate check: All issues I could identify are already covered by the existing inline comments. No new findings to add.
Assessment of current state at HEAD (72b35efea):
- The direction is correct: replacing
openwithopenBrowserSecurelyacross all slash commands and the OAuth flow eliminates the headless crash (issue #4712) and centralizes browser-launch security. - The
launchDetachedhelper usingspawn+unref()correctly avoids blocking the CLI for long-running browser processes. - The
parseBrowserCommandquote-aware parser and%splaceholder substitution (with function replacement to avoid$patterns) are solid improvements. - The
allowedFilePathsallow-list for file:// URLs ininsightCommandis the right approach for restricting file access. - The LspConnectionFactory changes (try/catch around disposer, writer, and stdin error handler) add necessary resilience against stream failures.
- The open dependency removal from both package.json files is clean.
- Test coverage is improved across all command files and the secure launcher.
The open suggestions from prior reviewers identify real issues that deserve attention (particularly the execFileAsync blocking in the platform opener path, the discrepancy between shouldAttemptBrowserLaunch and buildBrowserCommand BROWSER parsing, and the missing test scenarios). These are already well-documented in the existing comments.
— qwen-code via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — qwen3.7-max via Qwen Code /review
DragonnZhang
left a comment
There was a problem hiding this comment.
Reviewed all 18 changed files (+896/-202). ESLint clean on all changed source files. CI passes (12/12 checks). The migration from open to openBrowserSecurely is thorough and well-structured: BROWSER env var parsing with quote-aware parser, file:// URL gating behind explicit allow-list, non-blocking browser launches via spawn+unref, and LSP resilience improvements (try/catch on writer, disposer, stdin error handler). Test coverage is comprehensive across bugCommand, docsCommand, insightCommand, extensionsCommand, qwenOAuth2, and secure-browser-launcher. The 30 existing inline comments from prior reviewers already cover the remaining suggestions. No new issues found. — claude-sonnet-4-20250514 via Qwen Code /review
Real-Run Verification Report (tmux-driven interactive CLI)Complementing the earlier compile/unit-test report, this round exercises the actual built CLI end-to-end. Every interactive scenario below was executed against a real Head: 1. Build & unit tests
2. Control experiment — old vs. new behavior with missing platform binaryReproduces the exact #4712 mechanism on this machine (PATH stripped so the platform opener binary is missing — macOS equivalent of headless Linux without Old path ( Identical stack to the issue report: the promise resolves, ENOENT arrives asynchronously on an unguarded ChildProcess — New path ( 3. Interactive scenarios (real TUI sessions)
4. Non-blocking observations
5. Not covered by this real run
VerdictThe fix does what it claims: the async-ENOENT crash class is eliminated, the URL is always surfaced in the terminal, |
72b35ef to
fb9dd77
Compare
|
Rebased this branch onto current upstream/main and resolved the core export conflict by keeping both the newer sessionIdContext export and the secure browser launcher export. Focused validation after the rebase: core browser/OAuth tests passed (114 tests), core LSP connection test passed (2 tests), CLI bug/docs/insight/extensions command tests passed (25 tests), prettier check passed on touched files, eslint passed on touched files, core typecheck passed, and git diff --check passed. Local CLI workspace typecheck is still blocked by unrelated generated workspace artifacts around acp-bridge/serve/channel declarations; the focused CLI tests above pass on the touched command paths. |
fb9dd77 to
de51360
Compare
|
Rebased this branch onto current upstream/main and resolved the OAuth import conflict by keeping both the newer Focused validation after rebase: Current head: |
DragonnZhang
left a comment
There was a problem hiding this comment.
Re-reviewed at de51360. No high-confidence issues found.
Summary of changes reviewed:
- Replaces all direct
openimports withopenBrowserSecurely()across/bug,/docs,/insight, and/extensionscommands, and removes theopennpm dependency from bothpackages/cliandpackages/core. - Extends
openBrowserSecurelyto honor theBROWSERenv var (without a shell) via a well-implemented tokenizer that handles quoted arguments and%splaceholder substitution. - Adds opt-in
file://URL support with an explicit allow-list for the insight HTML report — secure by default. - Consolidates headless-Linux detection into
shouldAttemptBrowserLaunch()and reuses it inshouldLaunchBrowser(). - Improves LSP connection resilience: wraps
disposerandwriterin try/catch, adds a stdinerrorlistener. - Adds comprehensive tests for BROWSER parsing (quoted args, placeholder substitution, blocklist, headless fallback, Windows/macOS behavior) and error-path coverage for each command.
All changes are consistent, well-tested, and security-conscious (protocol allow-list, blocklist on basename, PowerShell single-quote escaping on Windows). CI has 1 passing check with 5 still pending.
* fix(cli): avoid headless browser open crashes * fix(cli): reuse browser launch guard * test(core): isolate browser launcher env * fix(core): make browser env launches non-blocking * fix(core): handle LSP stdio write failures
Summary
opencalls in/bug,/docs, and/insightwith the existingopenBrowserSecurely()pathBROWSERwithout going through a shellfile://support opt-in for the local insight HTML report, while HTTP/HTTPS remain the defaultFixes #4712.
To verify
npm run test --workspace=packages/core -- src/utils/secure-browser-launcher.test.tsnpm run test --workspace=packages/cli -- src/ui/commands/bugCommand.test.ts src/ui/commands/docsCommand.test.ts src/ui/commands/insightCommand.test.tsnpx eslint packages/core/src/utils/secure-browser-launcher.ts packages/core/src/utils/secure-browser-launcher.test.ts packages/core/src/index.ts packages/cli/src/ui/commands/bugCommand.ts packages/cli/src/ui/commands/docsCommand.ts packages/cli/src/ui/commands/insightCommand.ts packages/cli/src/ui/commands/bugCommand.test.ts packages/cli/src/ui/commands/docsCommand.test.ts packages/cli/src/ui/commands/insightCommand.test.tsnpm run typecheck --workspace=packages/corenpm run build --workspace=packages/corenpm run build --workspace=@qwen-code/acp-bridgenpm run build --workspace=@qwen-code/channel-basenpm run build --workspace=@qwen-code/channel-telegramnpm run build --workspace=@qwen-code/channel-weixinnpm run build --workspace=@qwen-code/channel-dingtalknpm run build --workspace=@qwen-code/channel-feishunpm run build --workspace=@qwen-code/web-templatesnpm run typecheck --workspace=packages/cligit diff --checkNote:
npm ci --ignore-scripts --prefer-offlinereported existing dependency audit findings; I did not runnpm audit fixbecause this PR does not change dependencies.