Skip to content

fix(cli): avoid headless browser open crashes #4716

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.

Sign up for GitHub

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

Open
he-yufeng wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix(cli): avoid headless browser open crashes #4716

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
890bfbf
fix(cli): avoid headless browser open crashes
he-yufeng Jun 2, 2026
84fdcca
fix(cli): reuse browser launch guard
he-yufeng Jun 4, 2026
357000c
test(core): isolate browser launcher env
he-yufeng Jun 4, 2026
d148e8a
fix(core): make browser env launches non-blocking
he-yufeng Jun 4, 2026
72b35ef
fix(core): handle LSP stdio write failures
he-yufeng Jun 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions packages/cli/src/ui/commands/bugCommand.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,23 @@
*/

import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
import open from 'open';
import { bugCommand } from './bugCommand.js';
import { createMockCommandContext } from '../../test-utils/mockCommandContext.js';
import { GIT_COMMIT_INFO } from '../../generated/git-commit.js';
import { AuthType } from '@qwen-code/qwen-code-core';
import * as systemInfoUtils from '../../utils/systemInfo.js';

const mockOpenBrowserSecurely = vi.hoisted(() => vi.fn());

// Mock dependencies
vi.mock('open');
vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => {
const actual =
await importOriginal<typeof import('@qwen-code/qwen-code-core')>();
return {
...actual,
openBrowserSecurely: mockOpenBrowserSecurely,
};
});
vi.mock('../../utils/systemInfo.js');

describe('bugCommand', () => {
Expand All @@ -36,6 +44,8 @@ describe('bugCommand', () => {
? GIT_COMMIT_INFO
: undefined,
});
mockOpenBrowserSecurely.mockClear();
mockOpenBrowserSecurely.mockResolvedValue(undefined);
Copy link
Copy Markdown
Collaborator

@wenshao wenshao Jun 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

vi.stubEnv('SANDBOX', 'qwen-test');
});

Expand Down Expand Up @@ -84,7 +94,7 @@ Memory Usage: 100 MB`;
},
expect.any(Number),
);
expect(open).toHaveBeenCalledWith(expectedUrl);
expect(mockOpenBrowserSecurely).toHaveBeenCalledWith(expectedUrl);
});

it('should use a custom URL template from config if provided', async () => {
Expand Down Expand Up @@ -128,7 +138,7 @@ Memory Usage: 100 MB`;
},
expect.any(Number),
);
expect(open).toHaveBeenCalledWith(expectedUrl);
expect(mockOpenBrowserSecurely).toHaveBeenCalledWith(expectedUrl);
});

it('should include Base URL when auth type is OpenAI', async () => {
Expand Down Expand Up @@ -193,6 +203,6 @@ Memory Usage: 100 MB`;
},
expect.any(Number),
);
expect(open).toHaveBeenCalledWith(expectedUrl);
expect(mockOpenBrowserSecurely).toHaveBeenCalledWith(expectedUrl);
});
});
4 changes: 2 additions & 2 deletions packages/cli/src/ui/commands/bugCommand.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@
* SPDX-License-Identifier: Apache-2.0
*/

import open from 'open';
import {
type CommandContext,
type SlashCommand,
CommandKind,
} from './types.js';
import { openBrowserSecurely } from '@qwen-code/qwen-code-core';
import { MessageType, type HistoryItem } from '../types.js';
import { getExtendedSystemInfo } from '../../utils/systemInfo.js';
import { getSystemInfoFields } from '../../utils/systemInfoFields.js';
Expand Down Expand Up @@ -55,7 +55,7 @@ export const bugCommand: SlashCommand = {
context.ui.addItem(bugReportItem, Date.now());

try {
await open(bugReportUrl);
await openBrowserSecurely(bugReportUrl);
} catch (error) {
const errorMessage =
error instanceof Error ? error.message : String(error);
Expand Down
29 changes: 17 additions & 12 deletions packages/cli/src/ui/commands/docsCommand.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,24 +5,29 @@
*/

import { vi, describe, it, expect, beforeEach, afterEach } from 'vitest';
import open from 'open';
import { docsCommand } from './docsCommand.js';
import { type CommandContext } from './types.js';
import { createMockCommandContext } from '../../test-utils/mockCommandContext.js';
import { MessageType } from '../types.js';

// Mock the 'open' library
vi.mock('open', () => ({
default: vi.fn(),
}));
const mockOpenBrowserSecurely = vi.hoisted(() => vi.fn());

vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => {
const actual =
await importOriginal<typeof import('@qwen-code/qwen-code-core')>();
return {
...actual,
openBrowserSecurely: mockOpenBrowserSecurely,
};
});

describe('docsCommand', () => {
let mockContext: CommandContext;
beforeEach(() => {
// Create a fresh mock context before each test
mockContext = createMockCommandContext();
// Reset the `open` mock
vi.mocked(open).mockClear();
mockOpenBrowserSecurely.mockClear();
mockOpenBrowserSecurely.mockResolvedValue(undefined);
});

afterEach(() => {
Expand All @@ -47,7 +52,7 @@ describe('docsCommand', () => {
expect.any(Number),
);

expect(open).toHaveBeenCalledWith(docsUrl);
expect(mockOpenBrowserSecurely).toHaveBeenCalledWith(docsUrl);
});

it('should only add an info message in a sandbox environment', async () => {
Expand All @@ -70,7 +75,7 @@ describe('docsCommand', () => {
);

// Ensure 'open' was not called in the sandbox
expect(open).not.toHaveBeenCalled();
expect(mockOpenBrowserSecurely).not.toHaveBeenCalled();
});

it("should not open browser for 'sandbox-exec'", async () => {
Expand All @@ -93,8 +98,8 @@ describe('docsCommand', () => {
expect.any(Number),
);

// 'open' should be called in this specific sandbox case
expect(open).toHaveBeenCalledWith(docsUrl);
// Browser launch should be called in this specific sandbox case.
expect(mockOpenBrowserSecurely).toHaveBeenCalledWith(docsUrl);
});

describe('non-interactive mode', () => {
Expand All @@ -112,7 +117,7 @@ describe('docsCommand', () => {
messageType: 'info',
content: expect.stringContaining('qwenlm.github.io'),
});
expect(open).not.toHaveBeenCalled();
expect(mockOpenBrowserSecurely).not.toHaveBeenCalled();
expect(nonInteractiveContext.ui.addItem).not.toHaveBeenCalled();
});
});
Expand Down
4 changes: 2 additions & 2 deletions packages/cli/src/ui/commands/docsCommand.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@
* SPDX-License-Identifier: Apache-2.0
*/

import open from 'open';
import process from 'node:process';
import {
type CommandContext,
type SlashCommand,
CommandKind,
} from './types.js';
import { openBrowserSecurely } from '@qwen-code/qwen-code-core';
import { MessageType } from '../types.js';
import { t, getCurrentLanguage } from '../../i18n/index.js';

Expand Down Expand Up @@ -57,7 +57,7 @@ export const docsCommand: SlashCommand = {
},
Date.now(),
);
await open(docsUrl);
await openBrowserSecurely(docsUrl);
Copy link
Copy Markdown
Collaborator

@wenshao wenshao Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

}
return;
},
Expand Down
29 changes: 21 additions & 8 deletions packages/cli/src/ui/commands/insightCommand.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,30 @@

import { beforeEach, afterEach, describe, expect, it, vi } from 'vitest';
import path from 'path';
import open from 'open';
import { pathToFileURL } from 'node:url';
import { parseInsightMessage, Storage } from '@qwen-code/qwen-code-core';
import { insightCommand } from './insightCommand.js';
import type { CommandContext } from './types.js';
import { createMockCommandContext } from '../../test-utils/mockCommandContext.js';

const mockGenerateStaticInsight = vi.fn();
const mockOpenBrowserSecurely = vi.hoisted(() => vi.fn());

vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => {
const actual =
await importOriginal<typeof import('@qwen-code/qwen-code-core')>();
return {
...actual,
openBrowserSecurely: mockOpenBrowserSecurely,
};
});

vi.mock('../../services/insight/generators/StaticInsightGenerator.js', () => ({
StaticInsightGenerator: vi.fn(() => ({
generateStaticInsight: mockGenerateStaticInsight,
})),
}));

vi.mock('open', () => ({
default: vi.fn(),
}));

describe('insightCommand', () => {
let mockContext: CommandContext;

Expand All @@ -32,7 +38,8 @@ describe('insightCommand', () => {
mockGenerateStaticInsight.mockResolvedValue(
path.resolve('runtime-output', 'insights', 'insight-2026-03-05.html'),
);
vi.mocked(open).mockResolvedValue(undefined as never);
mockOpenBrowserSecurely.mockClear();
mockOpenBrowserSecurely.mockResolvedValue(undefined);

mockContext = createMockCommandContext({
services: {
Expand Down Expand Up @@ -62,6 +69,12 @@ describe('insightCommand', () => {
path.join(Storage.getRuntimeBaseDir(), 'projects'),
expect.any(Function),
);
expect(mockOpenBrowserSecurely).toHaveBeenCalledWith(
pathToFileURL(
path.resolve('runtime-output', 'insights', 'insight-2026-03-05.html'),
).href,
{ allowFile: true },
);
});

it('streams ACP progress messages without waiting for generation to finish', async () => {
Expand Down Expand Up @@ -198,7 +211,7 @@ describe('insightCommand', () => {
expect((result as { content: string }).content).toContain(
path.resolve('runtime-output', 'insights', 'insight-2026-03-05.html'),
);
expect(open).not.toHaveBeenCalled();
expect(mockOpenBrowserSecurely).not.toHaveBeenCalled();
});

it('non_interactive: returns error message when generation fails', async () => {
Expand Down Expand Up @@ -227,6 +240,6 @@ describe('insightCommand', () => {
messageType: 'error',
});
expect((result as { content: string }).content).toContain('disk full');
expect(open).not.toHaveBeenCalled();
expect(mockOpenBrowserSecurely).not.toHaveBeenCalled();
});
});
27 changes: 15 additions & 12 deletions packages/cli/src/ui/commands/insightCommand.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,15 @@ import { MessageType } from '../types.js';
import type { HistoryItemInsightProgress } from '../types.js';
import { t } from '../../i18n/index.js';
import { join } from 'path';
import { pathToFileURL } from 'node:url';
import { StaticInsightGenerator } from '../../services/insight/generators/StaticInsightGenerator.js';
import {
createDebugLogger,
encodeInsightProgressMessage,
encodeInsightReadyMessage,
openBrowserSecurely,
Storage,
} from '@qwen-code/qwen-code-core';
import open from 'open';

const logger = createDebugLogger('DataProcessor');

Expand Down Expand Up @@ -215,18 +216,20 @@ export const insightCommand: SlashCommand = {
Date.now(),
);

try {
await open(outputPath);
context.ui.addItem(
{
type: MessageType.INFO,
text: t('Opening insights in your browser: {{path}}', {
Copy link
Copy Markdown
Collaborator

@wenshao wenshao Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

path: outputPath,
}),
},
Date.now(),
);

context.ui.addItem(
{
type: MessageType.INFO,
text: t('Opening insights in your browser: {{path}}', {
path: outputPath,
}),
},
Date.now(),
);
try {
await openBrowserSecurely(pathToFileURL(outputPath).href, {
allowFile: true,
});
} catch (browserError) {
logger.error('Failed to open browser automatically:', browserError);

Expand Down
1 change: 1 addition & 0 deletions packages/core/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,7 @@ export {
} from './utils/runtimeFetchOptions.js';
export * from './utils/runtimeStatus.js';
export * from './utils/schemaValidator.js';
export * from './utils/secure-browser-launcher.js';
export * from './utils/shell-utils.js';
export * from './utils/subagentGenerator.js';
export * from './utils/symlink.js';
Expand Down
33 changes: 33 additions & 0 deletions packages/core/src/utils/secure-browser-launcher.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,14 @@ describe('secure-browser-launcher', () => {
vi.clearAllMocks();
mockExecFile.mockResolvedValue({ stdout: '', stderr: '' });
originalPlatform = Object.getOwnPropertyDescriptor(process, 'platform');
vi.stubEnv('BROWSER', '');
});

afterEach(() => {
if (originalPlatform) {
Object.defineProperty(process, 'platform', originalPlatform);
}
vi.unstubAllEnvs();
});

function setPlatform(platform: string) {
Expand Down Expand Up @@ -71,6 +73,24 @@ describe('secure-browser-launcher', () => {
);
});

it('should allow file URLs only when explicitly requested', async () => {
setPlatform('linux');

await expect(openBrowserSecurely('file:///tmp/report.html')).rejects.toThrow(
'Unsafe protocol',
);

await openBrowserSecurely('file:///tmp/report.html', {
allowFile: true,
});

expect(mockExecFile).toHaveBeenCalledWith(
'xdg-open',
['file:///tmp/report.html'],
expect.any(Object),
);
Copy link
Copy Markdown
Collaborator

@wenshao wenshao Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

});

it('should reject invalid URLs', async () => {
await expect(openBrowserSecurely('not-a-url')).rejects.toThrow(
'Invalid URL',
Expand Down Expand Up @@ -198,6 +218,19 @@ describe('secure-browser-launcher', () => {
'Unsupported platform',
);
});

it('should prefer BROWSER when it is configured', async () => {
setPlatform('linux');
vi.stubEnv('BROWSER', 'firefox --new-tab');

await openBrowserSecurely('https://example.com');

expect(mockExecFile).toHaveBeenCalledWith(
'firefox',
['--new-tab', 'https://example.com'],
expect.any(Object),
);
});
});

describe('Error handling', () => {
Expand Down
Loading
Loading