fix(cli): avoid headless browser open crashes

[he-yufeng]

Summary

• replace direct open calls in /bug, /docs, and /insight with the existing openBrowserSecurely() path
• teach the secure launcher to honor BROWSER without going through a shell
• keep file:// support opt-in for the local insight HTML report, while HTTP/HTTPS remain the default

Fixes #4712.

To verify

• npm run test --workspace=packages/core -- src/utils/secure-browser-launcher.test.ts
• npm run test --workspace=packages/cli -- src/ui/commands/bugCommand.test.ts src/ui/commands/docsCommand.test.ts src/ui/commands/insightCommand.test.ts
• npx 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.ts
• npm run typecheck --workspace=packages/core
• npm run build --workspace=packages/core
• npm run build --workspace=@qwen-code/acp-bridge
• npm run build --workspace=@qwen-code/channel-base
• npm run build --workspace=@qwen-code/channel-telegram
• npm run build --workspace=@qwen-code/channel-weixin
• npm run build --workspace=@qwen-code/channel-dingtalk
• npm run build --workspace=@qwen-code/channel-feishu
• npm run build --workspace=@qwen-code/web-templates
• npm run typecheck --workspace=packages/cli
• git diff --check

Note: npm ci --ignore-scripts --prefer-offline reported existing dependency audit findings; I did not run npm audit fix because this PR does not change dependencies.

[wenshao]

[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.

Suggested change

	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

[wenshao]

[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

[wenshao]

[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:

1. Moving the "Opening" message back inside try after the await, or
2. Having openBrowserSecurely signal failure to the caller (e.g., return a boolean) so insightCommand can decide which message to show.

— qwen3.7-max via Qwen Code /review

[wenshao]

[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.

Suggested change

	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

[wenshao]

[Suggestion] Several test scenarios for the new BROWSER env var feature are missing:

1. Blocklisted value: BROWSER=www-browser should fall through to platform default (the blocklist branch is untested).
2. Quoted command paths: parseBrowserCommand's quote-handling regex is the main value-add over a simple .split(' '), but no test covers paths like BROWSER='"/path/to/my browser" --new-tab'.
3. Fallback suppression: No test verifies that Linux fallbacks are skipped when BROWSER is set and fails.

Adding these would strengthen coverage of the new code paths.

— qwen3.7-max via Qwen Code /review

[BZ-D]

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.

[yiliang114]

Thanks for the contribution — the direction of replacing open with openBrowserSecurely is right and matches what I had in mind for this issue.

A few architectural points beyond the inline feedback from wenshao:

1. Missing shouldLaunchBrowser() pre-check

The codebase already has a browser-availability detection flow: shouldAttemptBrowserLaunch() in browser.ts → isBrowserLaunchSuppressed() in config.ts. The OAuth path in qwenOAuth2.ts (L856) uses this correctly — it checks the environment before attempting to open a browser. These three commands should follow the same pattern: check first, skip the launch attempt entirely in headless/CI/SSH environments, and just display the URL.

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

extensionsCommand.ts and qwenOAuth2.ts also use import open from 'open' and have the same headless crash risk. If we're fixing this pattern, it's worth covering all the callsites in one pass.

3. Duplicated browser-check logic

shouldLaunchBrowser() in secure-browser-launcher.ts and shouldAttemptBrowserLaunch() in browser.ts are identical (the comment even says so). Exporting both without consolidating them will cause drift over time. Worth deduplicating as part of this PR.

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.