From 5bc309b3dc237e6a279037531a9889bd0ff84a53 Mon Sep 17 00:00:00 2001 From: tanzhenxin Date: Tue, 18 Nov 2025 13:43:17 +0800 Subject: [PATCH 01/14] feat: add os platform and version in log report (#1053) --- .../src/telemetry/qwen-logger/event-types.ts | 17 ++++++++++++++ .../telemetry/qwen-logger/qwen-logger.test.ts | 23 ++++++++++++++++++- .../src/telemetry/qwen-logger/qwen-logger.ts | 11 +++++++++ 3 files changed, 50 insertions(+), 1 deletion(-) diff --git a/packages/core/src/telemetry/qwen-logger/event-types.ts b/packages/core/src/telemetry/qwen-logger/event-types.ts index f81fb712..ed84ba78 100644 --- a/packages/core/src/telemetry/qwen-logger/event-types.ts +++ b/packages/core/src/telemetry/qwen-logger/event-types.ts @@ -19,6 +19,21 @@ export interface RumView { name: string; } +export interface RumOS { + type?: string; + version?: string; + container?: string; + container_version?: string; +} + +export interface RumDevice { + id?: string; + name?: string; + type?: string; + brand?: string; + model?: string; +} + export interface RumEvent { timestamp?: number; event_type?: 'view' | 'action' | 'exception' | 'resource'; @@ -78,6 +93,8 @@ export interface RumPayload { user: RumUser; session: RumSession; view: RumView; + os?: RumOS; + device?: RumDevice; events: RumEvent[]; properties?: Record; _v: string; diff --git a/packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts b/packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts index 8efb2e4e..9dbaa4f9 100644 --- a/packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts +++ b/packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts @@ -13,8 +13,10 @@ import { afterEach, afterAll, } from 'vitest'; +import * as os from 'node:os'; import { QwenLogger, TEST_ONLY } from './qwen-logger.js'; import type { Config } from '../../config/config.js'; +import { AuthType } from '../../core/contentGenerator.js'; import { StartSessionEvent, EndSessionEvent, @@ -22,7 +24,7 @@ import { KittySequenceOverflowEvent, IdeConnectionType, } from '../types.js'; -import type { RumEvent } from './event-types.js'; +import type { RumEvent, RumPayload } from './event-types.js'; // Mock dependencies vi.mock('../../utils/user_id.js', () => ({ @@ -46,6 +48,7 @@ const makeFakeConfig = (overrides: Partial = {}): Config => { getCliVersion: () => '1.0.0', getProxy: () => undefined, getContentGeneratorConfig: () => ({ authType: 'test-auth' }), + getAuthType: () => AuthType.QWEN_OAUTH, getMcpServers: () => ({}), getModel: () => 'test-model', getEmbeddingModel: () => 'test-embedding', @@ -102,6 +105,24 @@ describe('QwenLogger', () => { }); }); + describe('createRumPayload', () => { + it('includes os metadata in payload', async () => { + const logger = QwenLogger.getInstance(mockConfig)!; + const payload = await ( + logger as unknown as { + createRumPayload(): Promise; + } + ).createRumPayload(); + + expect(payload.os).toEqual( + expect.objectContaining({ + type: os.platform(), + version: os.release(), + }), + ); + }); + }); + describe('event queue management', () => { it('should handle event overflow gracefully', () => { const debugConfig = makeFakeConfig({ getDebugMode: () => true }); diff --git a/packages/core/src/telemetry/qwen-logger/qwen-logger.ts b/packages/core/src/telemetry/qwen-logger/qwen-logger.ts index 96f796e4..5f9a2a51 100644 --- a/packages/core/src/telemetry/qwen-logger/qwen-logger.ts +++ b/packages/core/src/telemetry/qwen-logger/qwen-logger.ts @@ -6,6 +6,7 @@ import { Buffer } from 'buffer'; import * as https from 'https'; +import * as os from 'node:os'; import { HttpsProxyAgent } from 'https-proxy-agent'; import type { @@ -45,6 +46,7 @@ import type { RumResourceEvent, RumExceptionEvent, RumPayload, + RumOS, } from './event-types.js'; import type { Config } from '../../config/config.js'; import { safeJsonStringify } from '../../utils/safeJsonStringify.js'; @@ -214,9 +216,17 @@ export class QwenLogger { return this.createRumEvent('exception', type, name, properties); } + private getOsMetadata(): RumOS { + return { + type: os.platform(), + version: os.release(), + }; + } + async createRumPayload(): Promise { const authType = this.config?.getAuthType(); const version = this.config?.getCliVersion() || 'unknown'; + const osMetadata = this.getOsMetadata(); return { app: { @@ -235,6 +245,7 @@ export class QwenLogger { id: this.sessionId, name: 'qwen-code-cli', }, + os: osMetadata, events: this.events.toArray() as RumEvent[], properties: { From 6bb829f87641deb6ef7717727bae207803938749 Mon Sep 17 00:00:00 2001 From: tanzhenxin Date: Tue, 18 Nov 2025 13:43:43 +0800 Subject: [PATCH 02/14] feat: Add Terminal Attention Notifications for User Alerts (#1052) --- packages/cli/src/ui/AppContainer.tsx | 7 + .../hooks/useAttentionNotifications.test.ts | 151 ++++++++++++++++++ .../src/ui/hooks/useAttentionNotifications.ts | 63 ++++++++ .../src/utils/attentionNotification.test.ts | 72 +++++++++ .../cli/src/utils/attentionNotification.ts | 43 +++++ 5 files changed, 336 insertions(+) create mode 100644 packages/cli/src/ui/hooks/useAttentionNotifications.test.ts create mode 100644 packages/cli/src/ui/hooks/useAttentionNotifications.ts create mode 100644 packages/cli/src/utils/attentionNotification.test.ts create mode 100644 packages/cli/src/utils/attentionNotification.ts diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index fbfc732b..c3443b43 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -97,6 +97,7 @@ import { type VisionSwitchOutcome } from './components/ModelSwitchDialog.js'; import { processVisionSwitchOutcome } from './hooks/useVisionAutoSwitch.js'; import { useSubagentCreateDialog } from './hooks/useSubagentCreateDialog.js'; import { useAgentsManagerDialog } from './hooks/useAgentsManagerDialog.js'; +import { useAttentionNotifications } from './hooks/useAttentionNotifications.js'; const CTRL_EXIT_PROMPT_DURATION_MS = 1000; @@ -944,6 +945,12 @@ export const AppContainer = (props: AppContainerProps) => { settings.merged.ui?.customWittyPhrases, ); + useAttentionNotifications({ + isFocused, + streamingState, + elapsedTime, + }); + // Dialog close functionality const { closeAnyOpenDialog } = useDialogClose({ isThemeDialogOpen, diff --git a/packages/cli/src/ui/hooks/useAttentionNotifications.test.ts b/packages/cli/src/ui/hooks/useAttentionNotifications.test.ts new file mode 100644 index 00000000..1475aa52 --- /dev/null +++ b/packages/cli/src/ui/hooks/useAttentionNotifications.test.ts @@ -0,0 +1,151 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { renderHook } from '@testing-library/react'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { StreamingState } from '../types.js'; +import { + AttentionNotificationReason, + notifyTerminalAttention, +} from '../../utils/attentionNotification.js'; +import { + LONG_TASK_NOTIFICATION_THRESHOLD_SECONDS, + useAttentionNotifications, +} from './useAttentionNotifications.js'; + +vi.mock('../../utils/attentionNotification.js', () => ({ + notifyTerminalAttention: vi.fn(), + AttentionNotificationReason: { + ToolApproval: 'tool_approval', + LongTaskComplete: 'long_task_complete', + }, +})); + +const mockedNotify = vi.mocked(notifyTerminalAttention); + +describe('useAttentionNotifications', () => { + beforeEach(() => { + mockedNotify.mockReset(); + }); + + const render = ( + props?: Partial[0]>, + ) => + renderHook(({ hookProps }) => useAttentionNotifications(hookProps), { + initialProps: { + hookProps: { + isFocused: true, + streamingState: StreamingState.Idle, + elapsedTime: 0, + ...props, + }, + }, + }); + + it('notifies when tool approval is required while unfocused', () => { + const { rerender } = render(); + + rerender({ + hookProps: { + isFocused: false, + streamingState: StreamingState.WaitingForConfirmation, + elapsedTime: 0, + }, + }); + + expect(mockedNotify).toHaveBeenCalledWith( + AttentionNotificationReason.ToolApproval, + ); + }); + + it('notifies when focus is lost after entering approval wait state', () => { + const { rerender } = render({ + isFocused: true, + streamingState: StreamingState.WaitingForConfirmation, + }); + + rerender({ + hookProps: { + isFocused: false, + streamingState: StreamingState.WaitingForConfirmation, + elapsedTime: 0, + }, + }); + + expect(mockedNotify).toHaveBeenCalledTimes(1); + }); + + it('sends a notification when a long task finishes while unfocused', () => { + const { rerender } = render(); + + rerender({ + hookProps: { + isFocused: false, + streamingState: StreamingState.Responding, + elapsedTime: LONG_TASK_NOTIFICATION_THRESHOLD_SECONDS + 5, + }, + }); + + rerender({ + hookProps: { + isFocused: false, + streamingState: StreamingState.Idle, + elapsedTime: 0, + }, + }); + + expect(mockedNotify).toHaveBeenCalledWith( + AttentionNotificationReason.LongTaskComplete, + ); + }); + + it('does not notify about long tasks when the CLI is focused', () => { + const { rerender } = render(); + + rerender({ + hookProps: { + isFocused: true, + streamingState: StreamingState.Responding, + elapsedTime: LONG_TASK_NOTIFICATION_THRESHOLD_SECONDS + 2, + }, + }); + + rerender({ + hookProps: { + isFocused: true, + streamingState: StreamingState.Idle, + elapsedTime: 0, + }, + }); + + expect(mockedNotify).not.toHaveBeenCalledWith( + AttentionNotificationReason.LongTaskComplete, + expect.anything(), + ); + }); + + it('does not treat short responses as long tasks', () => { + const { rerender } = render(); + + rerender({ + hookProps: { + isFocused: false, + streamingState: StreamingState.Responding, + elapsedTime: 5, + }, + }); + + rerender({ + hookProps: { + isFocused: false, + streamingState: StreamingState.Idle, + elapsedTime: 0, + }, + }); + + expect(mockedNotify).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/cli/src/ui/hooks/useAttentionNotifications.ts b/packages/cli/src/ui/hooks/useAttentionNotifications.ts new file mode 100644 index 00000000..e632c827 --- /dev/null +++ b/packages/cli/src/ui/hooks/useAttentionNotifications.ts @@ -0,0 +1,63 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { useEffect, useRef } from 'react'; +import { StreamingState } from '../types.js'; +import { + notifyTerminalAttention, + AttentionNotificationReason, +} from '../../utils/attentionNotification.js'; + +export const LONG_TASK_NOTIFICATION_THRESHOLD_SECONDS = 20; + +interface UseAttentionNotificationsOptions { + isFocused: boolean; + streamingState: StreamingState; + elapsedTime: number; +} + +export const useAttentionNotifications = ({ + isFocused, + streamingState, + elapsedTime, +}: UseAttentionNotificationsOptions) => { + const awaitingNotificationSentRef = useRef(false); + const respondingElapsedRef = useRef(0); + + useEffect(() => { + if ( + streamingState === StreamingState.WaitingForConfirmation && + !isFocused && + !awaitingNotificationSentRef.current + ) { + notifyTerminalAttention(AttentionNotificationReason.ToolApproval); + awaitingNotificationSentRef.current = true; + } + + if (streamingState !== StreamingState.WaitingForConfirmation || isFocused) { + awaitingNotificationSentRef.current = false; + } + }, [isFocused, streamingState]); + + useEffect(() => { + if (streamingState === StreamingState.Responding) { + respondingElapsedRef.current = elapsedTime; + return; + } + + if (streamingState === StreamingState.Idle) { + const wasLongTask = + respondingElapsedRef.current >= + LONG_TASK_NOTIFICATION_THRESHOLD_SECONDS; + if (wasLongTask && !isFocused) { + notifyTerminalAttention(AttentionNotificationReason.LongTaskComplete); + } + // Reset tracking for next task + respondingElapsedRef.current = 0; + return; + } + }, [streamingState, elapsedTime, isFocused]); +}; diff --git a/packages/cli/src/utils/attentionNotification.test.ts b/packages/cli/src/utils/attentionNotification.test.ts new file mode 100644 index 00000000..9ebb785c --- /dev/null +++ b/packages/cli/src/utils/attentionNotification.test.ts @@ -0,0 +1,72 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { + notifyTerminalAttention, + AttentionNotificationReason, +} from './attentionNotification.js'; + +describe('notifyTerminalAttention', () => { + let stream: { write: ReturnType; isTTY: boolean }; + + beforeEach(() => { + stream = { write: vi.fn().mockReturnValue(true), isTTY: true }; + }); + + it('emits terminal bell character', () => { + const result = notifyTerminalAttention( + AttentionNotificationReason.ToolApproval, + { + stream, + }, + ); + + expect(result).toBe(true); + expect(stream.write).toHaveBeenCalledWith('\u0007'); + }); + + it('returns false when not running inside a tty', () => { + stream.isTTY = false; + + const result = notifyTerminalAttention( + AttentionNotificationReason.ToolApproval, + { stream }, + ); + + expect(result).toBe(false); + expect(stream.write).not.toHaveBeenCalled(); + }); + + it('returns false when stream write fails', () => { + stream.write = vi.fn().mockImplementation(() => { + throw new Error('Write failed'); + }); + + const result = notifyTerminalAttention( + AttentionNotificationReason.ToolApproval, + { stream }, + ); + + expect(result).toBe(false); + }); + + it('works with different notification reasons', () => { + const reasons = [ + AttentionNotificationReason.ToolApproval, + AttentionNotificationReason.LongTaskComplete, + ]; + + reasons.forEach((reason) => { + stream.write.mockClear(); + + const result = notifyTerminalAttention(reason, { stream }); + + expect(result).toBe(true); + expect(stream.write).toHaveBeenCalledWith('\u0007'); + }); + }); +}); diff --git a/packages/cli/src/utils/attentionNotification.ts b/packages/cli/src/utils/attentionNotification.ts new file mode 100644 index 00000000..26dc2a25 --- /dev/null +++ b/packages/cli/src/utils/attentionNotification.ts @@ -0,0 +1,43 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import process from 'node:process'; + +export enum AttentionNotificationReason { + ToolApproval = 'tool_approval', + LongTaskComplete = 'long_task_complete', +} + +export interface TerminalNotificationOptions { + stream?: Pick; +} + +const TERMINAL_BELL = '\u0007'; + +/** + * Grabs the user's attention by emitting the terminal bell character. + * This causes the terminal to flash or play a sound, alerting the user + * to check the CLI for important events. + * + * @returns true when the bell was successfully written to the terminal. + */ +export function notifyTerminalAttention( + _reason: AttentionNotificationReason, + options: TerminalNotificationOptions = {}, +): boolean { + const stream = options.stream ?? process.stdout; + if (!stream?.write || stream.isTTY === false) { + return false; + } + + try { + stream.write(TERMINAL_BELL); + return true; + } catch (error) { + console.warn('Failed to send terminal bell:', error); + return false; + } +} From efca0bc7951eee3e051d5dd3688b1e357bd269cb Mon Sep 17 00:00:00 2001 From: Mingholy Date: Tue, 18 Nov 2025 13:46:42 +0800 Subject: [PATCH 03/14] fix: basic slash command support (#1020) --- packages/cli/src/nonInteractiveCli.test.ts | 5 + packages/cli/src/nonInteractiveCliCommands.ts | 104 ++++++++++++++- packages/cli/src/zed-integration/schema.ts | 24 ++++ .../cli/src/zed-integration/zedIntegration.ts | 123 +++++++++++++++++- 4 files changed, 247 insertions(+), 9 deletions(-) diff --git a/packages/cli/src/nonInteractiveCli.test.ts b/packages/cli/src/nonInteractiveCli.test.ts index 1aad835e..066b1848 100644 --- a/packages/cli/src/nonInteractiveCli.test.ts +++ b/packages/cli/src/nonInteractiveCli.test.ts @@ -23,6 +23,7 @@ import type { Part } from '@google/genai'; import { runNonInteractive } from './nonInteractiveCli.js'; import { vi } from 'vitest'; import type { LoadedSettings } from './config/settings.js'; +import { CommandKind } from './ui/commands/types.js'; // Mock core modules vi.mock('./ui/hooks/atCommandProcessor.js'); @@ -727,6 +728,7 @@ describe('runNonInteractive', () => { const mockCommand = { name: 'testcommand', description: 'a test command', + kind: CommandKind.FILE, action: vi.fn().mockResolvedValue({ type: 'submit_prompt', content: [{ text: 'Prompt from command' }], @@ -766,6 +768,7 @@ describe('runNonInteractive', () => { const mockCommand = { name: 'confirm', description: 'a command that needs confirmation', + kind: CommandKind.FILE, action: vi.fn().mockResolvedValue({ type: 'confirm_shell_commands', commands: ['rm -rf /'], @@ -821,6 +824,7 @@ describe('runNonInteractive', () => { const mockCommand = { name: 'noaction', description: 'unhandled type', + kind: CommandKind.FILE, action: vi.fn().mockResolvedValue({ type: 'unhandled', }), @@ -847,6 +851,7 @@ describe('runNonInteractive', () => { const mockCommand = { name: 'testargs', description: 'a test command', + kind: CommandKind.FILE, action: mockAction, }; mockGetCommands.mockReturnValue([mockCommand]); diff --git a/packages/cli/src/nonInteractiveCliCommands.ts b/packages/cli/src/nonInteractiveCliCommands.ts index 166a1706..77b9d099 100644 --- a/packages/cli/src/nonInteractiveCliCommands.ts +++ b/packages/cli/src/nonInteractiveCliCommands.ts @@ -13,15 +13,56 @@ import { type Config, } from '@qwen-code/qwen-code-core'; import { CommandService } from './services/CommandService.js'; +import { BuiltinCommandLoader } from './services/BuiltinCommandLoader.js'; import { FileCommandLoader } from './services/FileCommandLoader.js'; -import type { CommandContext } from './ui/commands/types.js'; +import { + CommandKind, + type CommandContext, + type SlashCommand, +} from './ui/commands/types.js'; import { createNonInteractiveUI } from './ui/noninteractive/nonInteractiveUi.js'; import type { LoadedSettings } from './config/settings.js'; import type { SessionStatsState } from './ui/contexts/SessionContext.js'; +/** + * Filters commands based on the allowed built-in command names. + * + * - Always includes FILE commands + * - Only includes BUILT_IN commands if their name is in the allowed set + * - Excludes other command types (e.g., MCP_PROMPT) in non-interactive mode + * + * @param commands All loaded commands + * @param allowedBuiltinCommandNames Set of allowed built-in command names (empty = none allowed) + * @returns Filtered commands + */ +function filterCommandsForNonInteractive( + commands: readonly SlashCommand[], + allowedBuiltinCommandNames: Set, +): SlashCommand[] { + return commands.filter((cmd) => { + if (cmd.kind === CommandKind.FILE) { + return true; + } + + // Built-in commands: only include if in the allowed list + if (cmd.kind === CommandKind.BUILT_IN) { + return allowedBuiltinCommandNames.has(cmd.name); + } + + // Exclude other types (e.g., MCP_PROMPT) in non-interactive mode + return false; + }); +} + /** * Processes a slash command in a non-interactive environment. * + * @param rawQuery The raw query string (should start with '/') + * @param abortController Controller to cancel the operation + * @param config The configuration object + * @param settings The loaded settings + * @param allowedBuiltinCommandNames Optional array of built-in command names that are + * allowed. If not provided or empty, only file commands are available. * @returns A Promise that resolves to `PartListUnion` if a valid command is * found and results in a prompt, or `undefined` otherwise. * @throws {FatalInputError} if the command result is not supported in @@ -32,21 +73,35 @@ export const handleSlashCommand = async ( abortController: AbortController, config: Config, settings: LoadedSettings, + allowedBuiltinCommandNames?: string[], ): Promise => { const trimmed = rawQuery.trim(); if (!trimmed.startsWith('/')) { return; } - // Only custom commands are supported for now. - const loaders = [new FileCommandLoader(config)]; + const allowedBuiltinSet = new Set(allowedBuiltinCommandNames ?? []); + + // Only load BuiltinCommandLoader if there are allowed built-in commands + const loaders = + allowedBuiltinSet.size > 0 + ? [new BuiltinCommandLoader(config), new FileCommandLoader(config)] + : [new FileCommandLoader(config)]; + const commandService = await CommandService.create( loaders, abortController.signal, ); const commands = commandService.getCommands(); + const filteredCommands = filterCommandsForNonInteractive( + commands, + allowedBuiltinSet, + ); - const { commandToExecute, args } = parseSlashCommand(rawQuery, commands); + const { commandToExecute, args } = parseSlashCommand( + rawQuery, + filteredCommands, + ); if (commandToExecute) { if (commandToExecute.action) { @@ -107,3 +162,44 @@ export const handleSlashCommand = async ( return; }; + +/** + * Retrieves all available slash commands for the current configuration. + * + * @param config The configuration object + * @param settings The loaded settings + * @param abortSignal Signal to cancel the loading process + * @param allowedBuiltinCommandNames Optional array of built-in command names that are + * allowed. If not provided or empty, only file commands are available. + * @returns A Promise that resolves to an array of SlashCommand objects + */ +export const getAvailableCommands = async ( + config: Config, + settings: LoadedSettings, + abortSignal: AbortSignal, + allowedBuiltinCommandNames?: string[], +): Promise => { + try { + const allowedBuiltinSet = new Set(allowedBuiltinCommandNames ?? []); + + // Only load BuiltinCommandLoader if there are allowed built-in commands + const loaders = + allowedBuiltinSet.size > 0 + ? [new BuiltinCommandLoader(config), new FileCommandLoader(config)] + : [new FileCommandLoader(config)]; + + const commandService = await CommandService.create(loaders, abortSignal); + const commands = commandService.getCommands(); + const filteredCommands = filterCommandsForNonInteractive( + commands, + allowedBuiltinSet, + ); + + // Filter out hidden commands + return filteredCommands.filter((cmd) => !cmd.hidden); + } catch (error) { + // Handle errors gracefully - log and return empty array + console.error('Error loading available commands:', error); + return []; + } +}; diff --git a/packages/cli/src/zed-integration/schema.ts b/packages/cli/src/zed-integration/schema.ts index b35cc47d..e5f72b50 100644 --- a/packages/cli/src/zed-integration/schema.ts +++ b/packages/cli/src/zed-integration/schema.ts @@ -128,6 +128,14 @@ export type AgentRequest = z.infer; export type AgentNotification = z.infer; +export type AvailableCommandInput = z.infer; + +export type AvailableCommand = z.infer; + +export type AvailableCommandsUpdate = z.infer< + typeof availableCommandsUpdateSchema +>; + export const writeTextFileRequestSchema = z.object({ content: z.string(), path: z.string(), @@ -386,6 +394,21 @@ export const promptRequestSchema = z.object({ sessionId: z.string(), }); +export const availableCommandInputSchema = z.object({ + hint: z.string(), +}); + +export const availableCommandSchema = z.object({ + description: z.string(), + input: availableCommandInputSchema.nullable().optional(), + name: z.string(), +}); + +export const availableCommandsUpdateSchema = z.object({ + availableCommands: z.array(availableCommandSchema), + sessionUpdate: z.literal('available_commands_update'), +}); + export const sessionUpdateSchema = z.union([ z.object({ content: contentBlockSchema, @@ -423,6 +446,7 @@ export const sessionUpdateSchema = z.union([ entries: z.array(planEntrySchema), sessionUpdate: z.literal('plan'), }), + availableCommandsUpdateSchema, ]); export const agentResponseSchema = z.union([ diff --git a/packages/cli/src/zed-integration/zedIntegration.ts b/packages/cli/src/zed-integration/zedIntegration.ts index 4a01ed7e..d83395f2 100644 --- a/packages/cli/src/zed-integration/zedIntegration.ts +++ b/packages/cli/src/zed-integration/zedIntegration.ts @@ -31,6 +31,7 @@ import { MCPServerConfig, ToolConfirmationOutcome, logToolCall, + logUserPrompt, getErrorStatus, isWithinRoot, isNodeError, @@ -38,6 +39,7 @@ import { TaskTool, Kind, TodoWriteTool, + UserPromptEvent, } from '@qwen-code/qwen-code-core'; import * as acp from './acp.js'; import { AcpFileSystemService } from './fileSystemService.js'; @@ -53,6 +55,26 @@ import { ExtensionStorage, type Extension } from '../config/extension.js'; import type { CliArgs } from '../config/config.js'; import { loadCliConfig } from '../config/config.js'; import { ExtensionEnablementManager } from '../config/extensions/extensionEnablement.js'; +import { + handleSlashCommand, + getAvailableCommands, +} from '../nonInteractiveCliCommands.js'; +import type { AvailableCommand, AvailableCommandsUpdate } from './schema.js'; +import { isSlashCommand } from '../ui/utils/commandUtils.js'; + +/** + * Built-in commands that are allowed in ACP integration mode. + * Only these commands will be available when using handleSlashCommand + * or getAvailableCommands in ACP integration. + * + * Currently, only "init" is supported because `handleSlashCommand` in + * nonInteractiveCliCommands.ts only supports handling results where + * result.type is "submit_prompt". Other result types are either coupled + * to the UI or cannot send notifications to the client via ACP. + * + * If you have a good idea to add support for more commands, PRs are welcome! + */ +const ALLOWED_BUILTIN_COMMANDS_FOR_ACP = ['init']; /** * Resolves the model to use based on the current configuration. @@ -151,7 +173,7 @@ class GeminiAgent { cwd, mcpServers, }: acp.NewSessionRequest): Promise { - const sessionId = randomUUID(); + const sessionId = this.config.getSessionId() || randomUUID(); const config = await this.newSessionConfig(sessionId, cwd, mcpServers); let isAuthenticated = false; @@ -182,9 +204,20 @@ class GeminiAgent { const geminiClient = config.getGeminiClient(); const chat = await geminiClient.startChat(); - const session = new Session(sessionId, chat, config, this.client); + const session = new Session( + sessionId, + chat, + config, + this.client, + this.settings, + ); this.sessions.set(sessionId, session); + // Send available commands update as the first session update + setTimeout(async () => { + await session.sendAvailableCommandsUpdate(); + }, 0); + return { sessionId, }; @@ -242,12 +275,14 @@ class GeminiAgent { class Session { private pendingPrompt: AbortController | null = null; + private turn: number = 0; constructor( private readonly id: string, private readonly chat: GeminiChat, private readonly config: Config, private readonly client: acp.Client, + private readonly settings: LoadedSettings, ) {} async cancelPendingPrompt(): Promise { @@ -264,10 +299,57 @@ class Session { const pendingSend = new AbortController(); this.pendingPrompt = pendingSend; - const promptId = Math.random().toString(16).slice(2); - const chat = this.chat; + // Increment turn counter for each user prompt + this.turn += 1; - const parts = await this.#resolvePrompt(params.prompt, pendingSend.signal); + const chat = this.chat; + const promptId = this.config.getSessionId() + '########' + this.turn; + + // Extract text from all text blocks to construct the full prompt text for logging + const promptText = params.prompt + .filter((block) => block.type === 'text') + .map((block) => (block.type === 'text' ? block.text : '')) + .join(' '); + + // Log user prompt + logUserPrompt( + this.config, + new UserPromptEvent( + promptText.length, + promptId, + this.config.getContentGeneratorConfig()?.authType, + promptText, + ), + ); + + // Check if the input contains a slash command + // Extract text from the first text block if present + const firstTextBlock = params.prompt.find((block) => block.type === 'text'); + const inputText = firstTextBlock?.text || ''; + + let parts: Part[]; + + if (isSlashCommand(inputText)) { + // Handle slash command - allow specific built-in commands for ACP integration + const slashCommandResult = await handleSlashCommand( + inputText, + pendingSend, + this.config, + this.settings, + ALLOWED_BUILTIN_COMMANDS_FOR_ACP, + ); + + if (slashCommandResult) { + // Use the result from the slash command + parts = slashCommandResult as Part[]; + } else { + // Slash command didn't return a prompt, continue with normal processing + parts = await this.#resolvePrompt(params.prompt, pendingSend.signal); + } + } else { + // Normal processing for non-slash commands + parts = await this.#resolvePrompt(params.prompt, pendingSend.signal); + } let nextMessage: Content | null = { role: 'user', parts }; @@ -361,6 +443,37 @@ class Session { await this.client.sessionUpdate(params); } + async sendAvailableCommandsUpdate(): Promise { + const abortController = new AbortController(); + try { + const slashCommands = await getAvailableCommands( + this.config, + this.settings, + abortController.signal, + ALLOWED_BUILTIN_COMMANDS_FOR_ACP, + ); + + // Convert SlashCommand[] to AvailableCommand[] format for ACP protocol + const availableCommands: AvailableCommand[] = slashCommands.map( + (cmd) => ({ + name: cmd.name, + description: cmd.description, + input: null, + }), + ); + + const update: AvailableCommandsUpdate = { + sessionUpdate: 'available_commands_update', + availableCommands, + }; + + await this.sendUpdate(update); + } catch (error) { + // Log error but don't fail session creation + console.error('Error sending available commands update:', error); + } + } + private async runTool( abortSignal: AbortSignal, promptId: string, From f0bbeac04a10bd5477c0d8944bbea921837c722f Mon Sep 17 00:00:00 2001 From: DS-Controller2 Date: Tue, 18 Nov 2025 00:47:20 -0500 Subject: [PATCH 04/14] fix(core): add modelscope provider to handle stream_options (#848) * fix(core): add modelscope provider to handle stream_options --------- Co-authored-by: Qwen Code Co-authored-by: mingholy.lmh --- .../src/core/openaiContentGenerator/index.ts | 9 ++ .../openaiContentGenerator/provider/index.ts | 1 + .../provider/modelscope.test.ts | 96 +++++++++++++++++++ .../provider/modelscope.ts | 32 +++++++ 4 files changed, 138 insertions(+) create mode 100644 packages/core/src/core/openaiContentGenerator/provider/modelscope.test.ts create mode 100644 packages/core/src/core/openaiContentGenerator/provider/modelscope.ts diff --git a/packages/core/src/core/openaiContentGenerator/index.ts b/packages/core/src/core/openaiContentGenerator/index.ts index 192bf096..8559258c 100644 --- a/packages/core/src/core/openaiContentGenerator/index.ts +++ b/packages/core/src/core/openaiContentGenerator/index.ts @@ -13,6 +13,7 @@ import { OpenAIContentGenerator } from './openaiContentGenerator.js'; import { DashScopeOpenAICompatibleProvider, DeepSeekOpenAICompatibleProvider, + ModelScopeOpenAICompatibleProvider, OpenRouterOpenAICompatibleProvider, type OpenAICompatibleProvider, DefaultOpenAICompatibleProvider, @@ -78,6 +79,14 @@ export function determineProvider( ); } + // Check for ModelScope provider + if (ModelScopeOpenAICompatibleProvider.isModelScopeProvider(config)) { + return new ModelScopeOpenAICompatibleProvider( + contentGeneratorConfig, + cliConfig, + ); + } + // Default provider for standard OpenAI-compatible APIs return new DefaultOpenAICompatibleProvider(contentGeneratorConfig, cliConfig); } diff --git a/packages/core/src/core/openaiContentGenerator/provider/index.ts b/packages/core/src/core/openaiContentGenerator/provider/index.ts index 9886b70f..cb33834d 100644 --- a/packages/core/src/core/openaiContentGenerator/provider/index.ts +++ b/packages/core/src/core/openaiContentGenerator/provider/index.ts @@ -1,3 +1,4 @@ +export { ModelScopeOpenAICompatibleProvider } from './modelscope.js'; export { DashScopeOpenAICompatibleProvider } from './dashscope.js'; export { DeepSeekOpenAICompatibleProvider } from './deepseek.js'; export { OpenRouterOpenAICompatibleProvider } from './openrouter.js'; diff --git a/packages/core/src/core/openaiContentGenerator/provider/modelscope.test.ts b/packages/core/src/core/openaiContentGenerator/provider/modelscope.test.ts new file mode 100644 index 00000000..da5a71a8 --- /dev/null +++ b/packages/core/src/core/openaiContentGenerator/provider/modelscope.test.ts @@ -0,0 +1,96 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import type OpenAI from 'openai'; +import { ModelScopeOpenAICompatibleProvider } from './modelscope.js'; +import type { Config } from '../../../config/config.js'; +import type { ContentGeneratorConfig } from '../../contentGenerator.js'; + +vi.mock('openai'); + +describe('ModelScopeOpenAICompatibleProvider', () => { + let provider: ModelScopeOpenAICompatibleProvider; + let mockContentGeneratorConfig: ContentGeneratorConfig; + let mockCliConfig: Config; + + beforeEach(() => { + mockContentGeneratorConfig = { + apiKey: 'test-api-key', + baseUrl: 'https://api.modelscope.cn/v1', + model: 'qwen-max', + } as ContentGeneratorConfig; + + mockCliConfig = { + getCliVersion: vi.fn().mockReturnValue('1.0.0'), + } as unknown as Config; + + provider = new ModelScopeOpenAICompatibleProvider( + mockContentGeneratorConfig, + mockCliConfig, + ); + }); + + describe('isModelScopeProvider', () => { + it('should return true if baseUrl includes "modelscope"', () => { + const config = { baseUrl: 'https://api.modelscope.cn/v1' }; + expect( + ModelScopeOpenAICompatibleProvider.isModelScopeProvider( + config as ContentGeneratorConfig, + ), + ).toBe(true); + }); + + it('should return false if baseUrl does not include "modelscope"', () => { + const config = { baseUrl: 'https://api.openai.com/v1' }; + expect( + ModelScopeOpenAICompatibleProvider.isModelScopeProvider( + config as ContentGeneratorConfig, + ), + ).toBe(false); + }); + }); + + describe('buildRequest', () => { + it('should remove stream_options when stream is false', () => { + const originalRequest: OpenAI.Chat.ChatCompletionCreateParams = { + model: 'qwen-max', + messages: [{ role: 'user', content: 'Hello!' }], + stream: false, + stream_options: { include_usage: true }, + }; + + const result = provider.buildRequest(originalRequest, 'prompt-id'); + + expect(result).not.toHaveProperty('stream_options'); + }); + + it('should keep stream_options when stream is true', () => { + const originalRequest: OpenAI.Chat.ChatCompletionCreateParams = { + model: 'qwen-max', + messages: [{ role: 'user', content: 'Hello!' }], + stream: true, + stream_options: { include_usage: true }, + }; + + const result = provider.buildRequest(originalRequest, 'prompt-id'); + + expect(result).toHaveProperty('stream_options'); + }); + + it('should handle requests without stream_options', () => { + const originalRequest: OpenAI.Chat.ChatCompletionCreateParams = { + model: 'qwen-max', + messages: [{ role: 'user', content: 'Hello!' }], + stream: false, + }; + + const result = provider.buildRequest(originalRequest, 'prompt-id'); + + expect(result).not.toHaveProperty('stream_options'); + }); + }); +}); diff --git a/packages/core/src/core/openaiContentGenerator/provider/modelscope.ts b/packages/core/src/core/openaiContentGenerator/provider/modelscope.ts new file mode 100644 index 00000000..1afb2d03 --- /dev/null +++ b/packages/core/src/core/openaiContentGenerator/provider/modelscope.ts @@ -0,0 +1,32 @@ +import type OpenAI from 'openai'; +import { DefaultOpenAICompatibleProvider } from './default.js'; +import type { ContentGeneratorConfig } from '../../contentGenerator.js'; + +/** + * Provider for ModelScope API + */ +export class ModelScopeOpenAICompatibleProvider extends DefaultOpenAICompatibleProvider { + /** + * Checks if the configuration is for ModelScope. + */ + static isModelScopeProvider(config: ContentGeneratorConfig): boolean { + return !!config.baseUrl?.includes('modelscope'); + } + + /** + * ModelScope does not support `stream_options` when `stream` is false. + * This method removes `stream_options` if `stream` is not true. + */ + override buildRequest( + request: OpenAI.Chat.ChatCompletionCreateParams, + userPromptId: string, + ): OpenAI.Chat.ChatCompletionCreateParams { + const newRequest = super.buildRequest(request, userPromptId); + if (!newRequest.stream) { + delete (newRequest as OpenAI.Chat.ChatCompletionCreateParamsNonStreaming) + .stream_options; + } + + return newRequest; + } +} From 71646490f1df3d5f26a9fa6122bbf72fd9e06172 Mon Sep 17 00:00:00 2001 From: tanzhenxin Date: Tue, 18 Nov 2025 19:38:30 +0800 Subject: [PATCH 05/14] Fix: Improve ripgrep binary detection and cross-platform compatibility (#1060) --- packages/core/scripts/postinstall.js | 119 ++++++++------- packages/core/src/tools/ripGrep.test.ts | 15 +- packages/core/src/tools/ripGrep.ts | 23 +-- packages/core/src/utils/ripgrepUtils.test.ts | 150 ++++++++----------- packages/core/src/utils/ripgrepUtils.ts | 100 +++++++------ 5 files changed, 203 insertions(+), 204 deletions(-) diff --git a/packages/core/scripts/postinstall.js b/packages/core/scripts/postinstall.js index 38f6d548..641b89a3 100644 --- a/packages/core/scripts/postinstall.js +++ b/packages/core/scripts/postinstall.js @@ -20,66 +20,81 @@ const vendorDir = path.join(packageRoot, 'vendor', 'ripgrep'); /** * Remove quarantine attribute and set executable permissions on macOS/Linux + * This script never throws errors to avoid blocking npm workflows. */ function setupRipgrepBinaries() { - if (!fs.existsSync(vendorDir)) { - console.log('Vendor directory not found, skipping ripgrep setup'); - return; - } - - const platform = process.platform; - const arch = process.arch; - - // Determine the binary directory based on platform and architecture - let binaryDir; - if (platform === 'darwin' || platform === 'linux') { - const archStr = arch === 'x64' || arch === 'arm64' ? arch : null; - if (archStr) { - binaryDir = path.join(vendorDir, `${archStr}-${platform}`); - } - } else if (platform === 'win32') { - // Windows doesn't need these fixes - return; - } - - if (!binaryDir || !fs.existsSync(binaryDir)) { - console.log( - `Binary directory not found for ${platform}-${arch}, skipping ripgrep setup`, - ); - return; - } - - const rgBinary = path.join(binaryDir, 'rg'); - - if (!fs.existsSync(rgBinary)) { - console.log(`Ripgrep binary not found at ${rgBinary}`); - return; - } - try { - // Set executable permissions - fs.chmodSync(rgBinary, 0o755); - console.log(`✓ Set executable permissions on ${rgBinary}`); + if (!fs.existsSync(vendorDir)) { + console.log('ℹ Vendor directory not found, skipping ripgrep setup'); + return; + } - // On macOS, remove quarantine attribute - if (platform === 'darwin') { - try { - execSync(`xattr -d com.apple.quarantine "${rgBinary}"`, { - stdio: 'pipe', - }); - console.log(`✓ Removed quarantine attribute from ${rgBinary}`); - } catch (error) { - // Quarantine attribute might not exist, which is fine - if (error.message && !error.message.includes('No such xattr')) { - console.warn( - `Warning: Could not remove quarantine attribute: ${error.message}`, - ); + const platform = process.platform; + const arch = process.arch; + + // Determine the binary directory based on platform and architecture + let binaryDir; + if (platform === 'darwin' || platform === 'linux') { + const archStr = arch === 'x64' || arch === 'arm64' ? arch : null; + if (archStr) { + binaryDir = path.join(vendorDir, `${archStr}-${platform}`); + } + } else if (platform === 'win32') { + // Windows doesn't need these fixes + console.log('ℹ Windows detected, skipping ripgrep setup'); + return; + } + + if (!binaryDir || !fs.existsSync(binaryDir)) { + console.log( + `ℹ Binary directory not found for ${platform}-${arch}, skipping ripgrep setup`, + ); + return; + } + + const rgBinary = path.join(binaryDir, 'rg'); + + if (!fs.existsSync(rgBinary)) { + console.log(`ℹ Ripgrep binary not found at ${rgBinary}, skipping setup`); + return; + } + + try { + // Set executable permissions + fs.chmodSync(rgBinary, 0o755); + console.log(`✓ Set executable permissions on ${rgBinary}`); + + // On macOS, remove quarantine attribute + if (platform === 'darwin') { + try { + execSync(`xattr -d com.apple.quarantine "${rgBinary}"`, { + stdio: 'pipe', + }); + console.log(`✓ Removed quarantine attribute from ${rgBinary}`); + } catch { + // Quarantine attribute might not exist, which is fine + console.log('ℹ Quarantine attribute not present or already removed'); } } + } catch (error) { + console.log( + `⚠ Could not complete ripgrep setup: ${error.message || 'Unknown error'}`, + ); + console.log(' This is not critical - ripgrep may still work correctly'); } } catch (error) { - console.error(`Error setting up ripgrep binary: ${error.message}`); + console.log( + `⚠ Ripgrep setup encountered an issue: ${error.message || 'Unknown error'}`, + ); + console.log(' Continuing anyway - this should not affect functionality'); } } -setupRipgrepBinaries(); +// Wrap the entire execution to ensure no errors escape to npm +try { + setupRipgrepBinaries(); +} catch { + // Last resort catch - never let errors block npm + console.log('⚠ Postinstall script encountered an unexpected error'); + console.log(' This will not affect the installation'); +} diff --git a/packages/core/src/tools/ripGrep.test.ts b/packages/core/src/tools/ripGrep.test.ts index a2f813f4..1b7dfe2d 100644 --- a/packages/core/src/tools/ripGrep.test.ts +++ b/packages/core/src/tools/ripGrep.test.ts @@ -22,12 +22,12 @@ import type { Config } from '../config/config.js'; import { createMockWorkspaceContext } from '../test-utils/mockWorkspaceContext.js'; import type { ChildProcess } from 'node:child_process'; import { spawn } from 'node:child_process'; -import { ensureRipgrepPath } from '../utils/ripgrepUtils.js'; +import { getRipgrepCommand } from '../utils/ripgrepUtils.js'; import { DEFAULT_FILE_FILTERING_OPTIONS } from '../config/constants.js'; // Mock ripgrepUtils vi.mock('../utils/ripgrepUtils.js', () => ({ - ensureRipgrepPath: vi.fn(), + getRipgrepCommand: vi.fn(), })); // Mock child_process for ripgrep calls @@ -109,7 +109,7 @@ describe('RipGrepTool', () => { beforeEach(async () => { vi.clearAllMocks(); - (ensureRipgrepPath as Mock).mockResolvedValue('/mock/path/to/rg'); + (getRipgrepCommand as Mock).mockResolvedValue('/mock/path/to/rg'); mockSpawn.mockReset(); tempRootDir = await fs.mkdtemp(path.join(os.tmpdir(), 'grep-tool-root-')); fileExclusionsMock = { @@ -588,18 +588,15 @@ describe('RipGrepTool', () => { }); it('should throw an error if ripgrep is not available', async () => { - // Make ensureRipgrepBinary throw - (ensureRipgrepPath as Mock).mockRejectedValue( - new Error('Ripgrep binary not found'), - ); + (getRipgrepCommand as Mock).mockResolvedValue(null); const params: RipGrepToolParams = { pattern: 'world' }; const invocation = grepTool.build(params); expect(await invocation.execute(abortSignal)).toStrictEqual({ llmContent: - 'Error during grep search operation: Ripgrep binary not found', - returnDisplay: 'Error: Ripgrep binary not found', + 'Error during grep search operation: ripgrep binary not found.', + returnDisplay: 'Error: ripgrep binary not found.', }); }); }); diff --git a/packages/core/src/tools/ripGrep.ts b/packages/core/src/tools/ripGrep.ts index 80273f31..402a5d3c 100644 --- a/packages/core/src/tools/ripGrep.ts +++ b/packages/core/src/tools/ripGrep.ts @@ -6,7 +6,6 @@ import fs from 'node:fs'; import path from 'node:path'; -import { EOL } from 'node:os'; import { spawn } from 'node:child_process'; import type { ToolInvocation, ToolResult } from './tools.js'; import { BaseDeclarativeTool, BaseToolInvocation, Kind } from './tools.js'; @@ -14,7 +13,7 @@ import { ToolNames } from './tool-names.js'; import { resolveAndValidatePath } from '../utils/paths.js'; import { getErrorMessage } from '../utils/errors.js'; import type { Config } from '../config/config.js'; -import { ensureRipgrepPath } from '../utils/ripgrepUtils.js'; +import { getRipgrepCommand } from '../utils/ripgrepUtils.js'; import { SchemaValidator } from '../utils/schemaValidator.js'; import type { FileFilteringOptions } from '../config/constants.js'; import { DEFAULT_FILE_FILTERING_OPTIONS } from '../config/constants.js'; @@ -88,7 +87,7 @@ class GrepToolInvocation extends BaseToolInvocation< } // Split into lines and count total matches - const allLines = rawOutput.split(EOL).filter((line) => line.trim()); + const allLines = rawOutput.split('\n').filter((line) => line.trim()); const totalMatches = allLines.length; const matchTerm = totalMatches === 1 ? 'match' : 'matches'; @@ -159,7 +158,7 @@ class GrepToolInvocation extends BaseToolInvocation< returnDisplay: displayMessage, }; } catch (error) { - console.error(`Error during GrepLogic execution: ${error}`); + console.error(`Error during ripgrep search operation: ${error}`); const errorMessage = getErrorMessage(error); return { llmContent: `Error during grep search operation: ${errorMessage}`, @@ -210,11 +209,15 @@ class GrepToolInvocation extends BaseToolInvocation< rgArgs.push(absolutePath); try { - const rgPath = this.config.getUseBuiltinRipgrep() - ? await ensureRipgrepPath() - : 'rg'; + const rgCommand = await getRipgrepCommand( + this.config.getUseBuiltinRipgrep(), + ); + if (!rgCommand) { + throw new Error('ripgrep binary not found.'); + } + const output = await new Promise((resolve, reject) => { - const child = spawn(rgPath, rgArgs, { + const child = spawn(rgCommand, rgArgs, { windowsHide: true, }); @@ -234,7 +237,7 @@ class GrepToolInvocation extends BaseToolInvocation< child.on('error', (err) => { options.signal.removeEventListener('abort', cleanup); - reject(new Error(`Failed to start ripgrep: ${err.message}.`)); + reject(new Error(`failed to start ripgrep: ${err.message}.`)); }); child.on('close', (code) => { @@ -256,7 +259,7 @@ class GrepToolInvocation extends BaseToolInvocation< return output; } catch (error: unknown) { - console.error(`GrepLogic: ripgrep failed: ${getErrorMessage(error)}`); + console.error(`Ripgrep failed: ${getErrorMessage(error)}`); throw error; } } diff --git a/packages/core/src/utils/ripgrepUtils.test.ts b/packages/core/src/utils/ripgrepUtils.test.ts index 59180aa8..47bba8a5 100644 --- a/packages/core/src/utils/ripgrepUtils.test.ts +++ b/packages/core/src/utils/ripgrepUtils.test.ts @@ -7,8 +7,8 @@ import { describe, it, expect, beforeEach, vi, type Mock } from 'vitest'; import { canUseRipgrep, - ensureRipgrepPath, - getRipgrepPath, + getRipgrepCommand, + getBuiltinRipgrep, } from './ripgrepUtils.js'; import { fileExists } from './fileUtils.js'; import path from 'node:path'; @@ -27,7 +27,7 @@ describe('ripgrepUtils', () => { vi.clearAllMocks(); }); - describe('getRipgrepPath', () => { + describe('getBulltinRipgrepPath', () => { it('should return path with .exe extension on Windows', () => { const originalPlatform = process.platform; const originalArch = process.arch; @@ -36,7 +36,7 @@ describe('ripgrepUtils', () => { Object.defineProperty(process, 'platform', { value: 'win32' }); Object.defineProperty(process, 'arch', { value: 'x64' }); - const rgPath = getRipgrepPath(); + const rgPath = getBuiltinRipgrep(); expect(rgPath).toContain('x64-win32'); expect(rgPath).toContain('rg.exe'); @@ -55,7 +55,7 @@ describe('ripgrepUtils', () => { Object.defineProperty(process, 'platform', { value: 'darwin' }); Object.defineProperty(process, 'arch', { value: 'arm64' }); - const rgPath = getRipgrepPath(); + const rgPath = getBuiltinRipgrep(); expect(rgPath).toContain('arm64-darwin'); expect(rgPath).toContain('rg'); @@ -75,7 +75,7 @@ describe('ripgrepUtils', () => { Object.defineProperty(process, 'platform', { value: 'linux' }); Object.defineProperty(process, 'arch', { value: 'x64' }); - const rgPath = getRipgrepPath(); + const rgPath = getBuiltinRipgrep(); expect(rgPath).toContain('x64-linux'); expect(rgPath).toContain('rg'); @@ -87,7 +87,7 @@ describe('ripgrepUtils', () => { Object.defineProperty(process, 'arch', { value: originalArch }); }); - it('should throw error for unsupported platform', () => { + it('should return null for unsupported platform', () => { const originalPlatform = process.platform; const originalArch = process.arch; @@ -95,14 +95,14 @@ describe('ripgrepUtils', () => { Object.defineProperty(process, 'platform', { value: 'freebsd' }); Object.defineProperty(process, 'arch', { value: 'x64' }); - expect(() => getRipgrepPath()).toThrow('Unsupported platform: freebsd'); + expect(getBuiltinRipgrep()).toBeNull(); // Restore original values Object.defineProperty(process, 'platform', { value: originalPlatform }); Object.defineProperty(process, 'arch', { value: originalArch }); }); - it('should throw error for unsupported architecture', () => { + it('should return null for unsupported architecture', () => { const originalPlatform = process.platform; const originalArch = process.arch; @@ -110,7 +110,7 @@ describe('ripgrepUtils', () => { Object.defineProperty(process, 'platform', { value: 'darwin' }); Object.defineProperty(process, 'arch', { value: 'ia32' }); - expect(() => getRipgrepPath()).toThrow('Unsupported architecture: ia32'); + expect(getBuiltinRipgrep()).toBeNull(); // Restore original values Object.defineProperty(process, 'platform', { value: originalPlatform }); @@ -136,7 +136,7 @@ describe('ripgrepUtils', () => { Object.defineProperty(process, 'platform', { value: platform }); Object.defineProperty(process, 'arch', { value: arch }); - const rgPath = getRipgrepPath(); + const rgPath = getBuiltinRipgrep(); const binaryName = platform === 'win32' ? 'rg.exe' : 'rg'; const expectedPathSegment = path.join( `${arch}-${platform}`, @@ -169,107 +169,77 @@ describe('ripgrepUtils', () => { expect(result).toBe(true); expect(fileExists).toHaveBeenCalledOnce(); }); - - it('should fall back to system rg if bundled ripgrep binary does not exist', async () => { - (fileExists as Mock).mockResolvedValue(false); - // When useBuiltin is true but bundled binary doesn't exist, - // it should fall back to checking system rg (which will spawn a process) - // In this test environment, system rg is likely available, so result should be true - // unless spawn fails - - const result = await canUseRipgrep(); - - // The test may pass or fail depending on system rg availability - // Just verify that fileExists was called to check bundled binary first - expect(fileExists).toHaveBeenCalledOnce(); - // Result depends on whether system rg is installed - expect(typeof result).toBe('boolean'); - }); - - // Note: Tests for system ripgrep detection (useBuiltin=false) would require mocking - // the child_process spawn function, which is complex in ESM. These cases are tested - // indirectly through integration tests. - - it('should return false if platform is unsupported', async () => { - const originalPlatform = process.platform; - - // Mock unsupported platform - Object.defineProperty(process, 'platform', { value: 'aix' }); - - const result = await canUseRipgrep(); - - expect(result).toBe(false); - expect(fileExists).not.toHaveBeenCalled(); - - // Restore original value - Object.defineProperty(process, 'platform', { value: originalPlatform }); - }); - - it('should return false if architecture is unsupported', async () => { - const originalArch = process.arch; - - // Mock unsupported architecture - Object.defineProperty(process, 'arch', { value: 's390x' }); - - const result = await canUseRipgrep(); - - expect(result).toBe(false); - expect(fileExists).not.toHaveBeenCalled(); - - // Restore original value - Object.defineProperty(process, 'arch', { value: originalArch }); - }); }); - describe('ensureRipgrepBinary', () => { - it('should return ripgrep path if binary exists', async () => { + describe('ensureRipgrepPath', () => { + it('should return bundled ripgrep path if binary exists (useBuiltin=true)', async () => { (fileExists as Mock).mockResolvedValue(true); - const rgPath = await ensureRipgrepPath(); + const rgPath = await getRipgrepCommand(true); expect(rgPath).toBeDefined(); expect(rgPath).toContain('rg'); + expect(rgPath).not.toBe('rg'); // Should be full path, not just 'rg' expect(fileExists).toHaveBeenCalledOnce(); expect(fileExists).toHaveBeenCalledWith(rgPath); }); - it('should throw error if binary does not exist', async () => { - (fileExists as Mock).mockResolvedValue(false); + it('should return bundled ripgrep path if binary exists (default)', async () => { + (fileExists as Mock).mockResolvedValue(true); - await expect(ensureRipgrepPath()).rejects.toThrow( - /Ripgrep binary not found/, - ); - await expect(ensureRipgrepPath()).rejects.toThrow(/Platform:/); - await expect(ensureRipgrepPath()).rejects.toThrow(/Architecture:/); + const rgPath = await getRipgrepCommand(); - expect(fileExists).toHaveBeenCalled(); + expect(rgPath).toBeDefined(); + expect(rgPath).toContain('rg'); + expect(fileExists).toHaveBeenCalledOnce(); }); - it('should throw error with correct path information', async () => { + it('should fall back to system rg if bundled binary does not exist', async () => { (fileExists as Mock).mockResolvedValue(false); + // When useBuiltin is true but bundled binary doesn't exist, + // it should fall back to checking system rg + // The test result depends on whether system rg is actually available + + const rgPath = await getRipgrepCommand(true); + + expect(fileExists).toHaveBeenCalledOnce(); + // If system rg is available, it should return 'rg' (or 'rg.exe' on Windows) + // This test will pass if system ripgrep is installed + expect(rgPath).toBeDefined(); + }); + + it('should use system rg when useBuiltin=false', async () => { + // When useBuiltin is false, should skip bundled check and go straight to system rg + const rgPath = await getRipgrepCommand(false); + + // Should not check for bundled binary + expect(fileExists).not.toHaveBeenCalled(); + // If system rg is available, it should return 'rg' (or 'rg.exe' on Windows) + expect(rgPath).toBeDefined(); + }); + + it('should throw error if neither bundled nor system ripgrep is available', async () => { + // This test only makes sense in an environment where system rg is not installed + // We'll skip this test in CI/local environments where rg might be available + // Instead, we test the error message format + const originalPlatform = process.platform; + + // Use an unsupported platform to trigger the error path + Object.defineProperty(process, 'platform', { value: 'freebsd' }); try { - await ensureRipgrepPath(); - // Should not reach here - expect(true).toBe(false); + await getRipgrepCommand(); + // If we get here without error, system rg was available, which is fine } catch (error) { expect(error).toBeInstanceOf(Error); const errorMessage = (error as Error).message; - expect(errorMessage).toContain('Ripgrep binary not found at'); - expect(errorMessage).toContain(process.platform); - expect(errorMessage).toContain(process.arch); + // Should contain helpful error information + expect( + errorMessage.includes('Ripgrep binary not found') || + errorMessage.includes('Failed to locate ripgrep') || + errorMessage.includes('Unsupported platform'), + ).toBe(true); } - }); - - it('should throw error if platform is unsupported', async () => { - const originalPlatform = process.platform; - - // Mock unsupported platform - Object.defineProperty(process, 'platform', { value: 'openbsd' }); - - await expect(ensureRipgrepPath()).rejects.toThrow( - 'Unsupported platform: openbsd', - ); // Restore original value Object.defineProperty(process, 'platform', { value: originalPlatform }); diff --git a/packages/core/src/utils/ripgrepUtils.ts b/packages/core/src/utils/ripgrepUtils.ts index 467bf2ca..c6d795a3 100644 --- a/packages/core/src/utils/ripgrepUtils.ts +++ b/packages/core/src/utils/ripgrepUtils.ts @@ -18,37 +18,42 @@ type Architecture = 'x64' | 'arm64'; /** * Maps process.platform values to vendor directory names */ -function getPlatformString(platform: string): Platform { +function getPlatformString(platform: string): Platform | undefined { switch (platform) { case 'darwin': case 'linux': case 'win32': return platform; default: - throw new Error(`Unsupported platform: ${platform}`); + return undefined; } } /** * Maps process.arch values to vendor directory names */ -function getArchitectureString(arch: string): Architecture { +function getArchitectureString(arch: string): Architecture | undefined { switch (arch) { case 'x64': case 'arm64': return arch; default: - throw new Error(`Unsupported architecture: ${arch}`); + return undefined; } } /** * Returns the path to the bundled ripgrep binary for the current platform + * @returns The path to the bundled ripgrep binary, or null if not available */ -export function getRipgrepPath(): string { +export function getBuiltinRipgrep(): string | null { const platform = getPlatformString(process.platform); const arch = getArchitectureString(process.arch); + if (!platform || !arch) { + return null; + } + // Binary name includes .exe on Windows const binaryName = platform === 'win32' ? 'rg.exe' : 'rg'; @@ -83,6 +88,51 @@ export function getRipgrepPath(): string { return vendorPath; } +/** + * Checks if system ripgrep is available and returns the command to use + * @returns The ripgrep command ('rg' or 'rg.exe') if available, or null if not found + */ +export async function getSystemRipgrep(): Promise { + try { + const { spawn } = await import('node:child_process'); + const rgCommand = process.platform === 'win32' ? 'rg.exe' : 'rg'; + const isAvailable = await new Promise((resolve) => { + const proc = spawn(rgCommand, ['--version']); + proc.on('error', () => resolve(false)); + proc.on('exit', (code) => resolve(code === 0)); + }); + return isAvailable ? rgCommand : null; + } catch (_error) { + return null; + } +} + +/** + * Checks if ripgrep binary exists and returns its path + * @param useBuiltin If true, tries bundled ripgrep first, then falls back to system ripgrep. + * If false, only checks for system ripgrep. + * @returns The path to ripgrep binary ('rg' or 'rg.exe' for system ripgrep, or full path for bundled), or null if not available + */ +export async function getRipgrepCommand( + useBuiltin: boolean = true, +): Promise { + try { + if (useBuiltin) { + // Try bundled ripgrep first + const rgPath = getBuiltinRipgrep(); + if (rgPath && (await fileExists(rgPath))) { + return rgPath; + } + // Fallback to system rg if bundled binary is not available + } + + // Check for system ripgrep + return await getSystemRipgrep(); + } catch (_error) { + return null; + } +} + /** * Checks if ripgrep binary is available * @param useBuiltin If true, tries bundled ripgrep first, then falls back to system ripgrep. @@ -91,42 +141,6 @@ export function getRipgrepPath(): string { export async function canUseRipgrep( useBuiltin: boolean = true, ): Promise { - try { - if (useBuiltin) { - // Try bundled ripgrep first - const rgPath = getRipgrepPath(); - if (await fileExists(rgPath)) { - return true; - } - // Fallback to system rg if bundled binary is not available - } - - // Check for system ripgrep by trying to spawn 'rg --version' - const { spawn } = await import('node:child_process'); - return await new Promise((resolve) => { - const proc = spawn('rg', ['--version']); - proc.on('error', () => resolve(false)); - proc.on('exit', (code) => resolve(code === 0)); - }); - } catch (_error) { - // Unsupported platform/arch or other error - return false; - } -} - -/** - * Ensures ripgrep binary exists and returns its path - * @throws Error if ripgrep binary is not available - */ -export async function ensureRipgrepPath(): Promise { - const rgPath = getRipgrepPath(); - - if (!(await fileExists(rgPath))) { - throw new Error( - `Ripgrep binary not found at ${rgPath}. ` + - `Platform: ${process.platform}, Architecture: ${process.arch}`, - ); - } - - return rgPath; + const rgPath = await getRipgrepCommand(useBuiltin); + return rgPath !== null; } From 3ed93d5b5d2f3bfdcfab2b6f96449503e2a41c41 Mon Sep 17 00:00:00 2001 From: tanzhenxin Date: Wed, 19 Nov 2025 10:23:16 +0800 Subject: [PATCH 06/14] fix: integration tests (#1062) --- docs/features/subagents.md | 10 ++--- .../context-compress-interactive.test.ts | 42 +++++++++---------- .../file-system-interactive.test.ts | 20 ++++----- .../messages/CompressionMessage.tsx | 2 +- 4 files changed, 34 insertions(+), 40 deletions(-) diff --git a/docs/features/subagents.md b/docs/features/subagents.md index 225384ca..506d856f 100644 --- a/docs/features/subagents.md +++ b/docs/features/subagents.md @@ -106,7 +106,7 @@ Subagents are configured using Markdown files with YAML frontmatter. This format --- name: agent-name description: Brief description of when and how to use this agent -tools: +tools: - tool1 - tool2 - tool3 # Optional @@ -170,7 +170,7 @@ Perfect for comprehensive test creation and test-driven development. --- name: testing-expert description: Writes comprehensive unit tests, integration tests, and handles test automation with best practices -tools: +tools: - read_file - write_file - read_many_files @@ -214,7 +214,7 @@ Specialized in creating clear, comprehensive documentation. --- name: documentation-writer description: Creates comprehensive documentation, README files, API docs, and user guides -tools: +tools: - read_file - write_file - read_many_files @@ -267,7 +267,7 @@ Focused on code quality, security, and best practices. --- name: code-reviewer description: Reviews code for best practices, security issues, performance, and maintainability -tools: +tools: - read_file - read_many_files --- @@ -311,7 +311,7 @@ Optimized for React development, hooks, and component patterns. --- name: react-specialist description: Expert in React development, hooks, component patterns, and modern React best practices -tools: +tools: - read_file - write_file - read_many_files diff --git a/integration-tests/context-compress-interactive.test.ts b/integration-tests/context-compress-interactive.test.ts index ddfa6839..68b095ef 100644 --- a/integration-tests/context-compress-interactive.test.ts +++ b/integration-tests/context-compress-interactive.test.ts @@ -21,23 +21,21 @@ describe('Interactive Mode', () => { it.skipIf(process.platform === 'win32')( 'should trigger chat compression with /compress command', async () => { - await rig.setup('interactive-compress-test'); + await rig.setup('interactive-compress-test', { + settings: { + security: { + auth: { + selectedType: 'openai', + }, + }, + }, + }); const { ptyProcess } = rig.runInteractive(); let fullOutput = ''; ptyProcess.onData((data) => (fullOutput += data)); - const authDialogAppeared = await rig.waitForText( - 'How would you like to authenticate', - 5000, - ); - - // select the second option if auth dialog come's up - if (authDialogAppeared) { - ptyProcess.write('2'); - } - // Wait for the app to be ready const isReady = await rig.waitForText('Type your message', 15000); expect( @@ -71,23 +69,21 @@ describe('Interactive Mode', () => { it.skipIf(process.platform === 'win32')( 'should handle compression failure on token inflation', async () => { - await rig.setup('interactive-compress-test'); + await rig.setup('interactive-compress-test', { + settings: { + security: { + auth: { + selectedType: 'openai', + }, + }, + }, + }); const { ptyProcess } = rig.runInteractive(); let fullOutput = ''; ptyProcess.onData((data) => (fullOutput += data)); - const authDialogAppeared = await rig.waitForText( - 'How would you like to authenticate', - 5000, - ); - - // select the second option if auth dialog come's up - if (authDialogAppeared) { - ptyProcess.write('2'); - } - // Wait for the app to be ready const isReady = await rig.waitForText('Type your message', 25000); expect( @@ -106,7 +102,7 @@ describe('Interactive Mode', () => { expect(foundEvent).toBe(true); const compressionFailed = await rig.waitForText( - 'compression was not beneficial', + 'Nothing to compress.', 25000, ); diff --git a/integration-tests/file-system-interactive.test.ts b/integration-tests/file-system-interactive.test.ts index 6927fde9..31a9feb1 100644 --- a/integration-tests/file-system-interactive.test.ts +++ b/integration-tests/file-system-interactive.test.ts @@ -22,21 +22,19 @@ describe('Interactive file system', () => { 'should perform a read-then-write sequence in interactive mode', async () => { const fileName = 'version.txt'; - await rig.setup('interactive-read-then-write'); + await rig.setup('interactive-read-then-write', { + settings: { + security: { + auth: { + selectedType: 'openai', + }, + }, + }, + }); rig.createFile(fileName, '1.0.0'); const { ptyProcess } = rig.runInteractive(); - const authDialogAppeared = await rig.waitForText( - 'How would you like to authenticate', - 5000, - ); - - // select the second option if auth dialog come's up - if (authDialogAppeared) { - ptyProcess.write('2'); - } - // Wait for the app to be ready const isReady = await rig.waitForText('Type your message', 15000); expect( diff --git a/packages/cli/src/ui/components/messages/CompressionMessage.tsx b/packages/cli/src/ui/components/messages/CompressionMessage.tsx index 22c642f7..cd6224e3 100644 --- a/packages/cli/src/ui/components/messages/CompressionMessage.tsx +++ b/packages/cli/src/ui/components/messages/CompressionMessage.tsx @@ -47,7 +47,7 @@ export function CompressionMessage({ case CompressionStatus.COMPRESSION_FAILED_TOKEN_COUNT_ERROR: return 'Could not compress chat history due to a token counting error.'; case CompressionStatus.NOOP: - return 'Chat history is already compressed.'; + return 'Nothing to compress.'; default: return ''; } From d0e76c76a8ad1f31bf24ee0d77dba155602964f1 Mon Sep 17 00:00:00 2001 From: Mingholy Date: Wed, 19 Nov 2025 11:21:46 +0800 Subject: [PATCH 07/14] refactor(auth): save authType after successfully authenticated (#1036) --- docs/support/troubleshooting.md | 7 + .../context-compress-interactive.test.ts | 2 +- packages/cli/src/config/settings.ts | 1 + packages/cli/src/core/auth.ts | 16 +- packages/cli/src/core/initializer.ts | 18 +- packages/cli/src/ui/AppContainer.tsx | 58 ++--- packages/cli/src/ui/auth/AuthDialog.test.tsx | 121 ++++++---- packages/cli/src/ui/auth/AuthDialog.tsx | 129 ++++------ packages/cli/src/ui/auth/useAuth.ts | 222 ++++++++++++------ .../cli/src/ui/components/DialogManager.tsx | 84 ++++--- .../cli/src/ui/components/OpenAIKeyPrompt.tsx | 46 +++- .../ui/components/QwenOAuthProgress.test.tsx | 17 +- .../src/ui/components/QwenOAuthProgress.tsx | 37 ++- .../src/ui/components/SettingsDialog.test.tsx | 7 +- .../cli/src/ui/contexts/UIActionsContext.tsx | 8 +- .../cli/src/ui/contexts/UIStateContext.tsx | 16 +- packages/cli/src/ui/hooks/useDialogClose.ts | 4 +- .../ui/hooks/useInitializationAuthError.ts | 47 ++++ packages/cli/src/ui/hooks/useQwenAuth.test.ts | 205 ++++++++-------- packages/cli/src/ui/hooks/useQwenAuth.ts | 29 +-- packages/core/index.ts | 2 + packages/core/src/config/config.ts | 3 +- packages/core/src/core/contentGenerator.ts | 9 +- packages/core/src/qwen/qwenOAuth2.test.ts | 16 +- packages/core/src/qwen/qwenOAuth2.ts | 159 ++++++++----- packages/core/src/telemetry/constants.ts | 1 + packages/core/src/telemetry/index.ts | 2 + packages/core/src/telemetry/loggers.ts | 28 +++ .../src/telemetry/qwen-logger/qwen-logger.ts | 20 ++ packages/core/src/telemetry/types.ts | 26 +- 30 files changed, 822 insertions(+), 518 deletions(-) create mode 100644 packages/cli/src/ui/hooks/useInitializationAuthError.ts diff --git a/docs/support/troubleshooting.md b/docs/support/troubleshooting.md index f53c25ec..f654c1f6 100644 --- a/docs/support/troubleshooting.md +++ b/docs/support/troubleshooting.md @@ -14,6 +14,13 @@ This guide provides solutions to common issues and debugging tips, including top - **Solution:** Set the `NODE_EXTRA_CA_CERTS` environment variable to the absolute path of your corporate root CA certificate file. - Example: `export NODE_EXTRA_CA_CERTS=/path/to/your/corporate-ca.crt` +- **Issue: Unable to display UI after authentication failure** + - **Cause:** If authentication fails after selecting an authentication type, the `security.auth.selectedType` setting may be persisted in `settings.json`. On restart, the CLI may get stuck trying to authenticate with the failed auth type and fail to display the UI. + - **Solution:** Clear the `security.auth.selectedType` configuration item in your `settings.json` file: + - Open `~/.qwen/settings.json` (or `./.qwen/settings.json` for project-specific settings) + - Remove the `security.auth.selectedType` field + - Restart the CLI to allow it to prompt for authentication again + ## Frequently asked questions (FAQs) - **Q: How do I update Qwen Code to the latest version?** diff --git a/integration-tests/context-compress-interactive.test.ts b/integration-tests/context-compress-interactive.test.ts index 68b095ef..845d9db0 100644 --- a/integration-tests/context-compress-interactive.test.ts +++ b/integration-tests/context-compress-interactive.test.ts @@ -92,7 +92,7 @@ describe('Interactive Mode', () => { ).toBe(true); await type(ptyProcess, '/compress'); - await new Promise((resolve) => setTimeout(resolve, 100)); + await new Promise((resolve) => setTimeout(resolve, 1000)); await type(ptyProcess, '\r'); const foundEvent = await rig.waitForTelemetryEvent( diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index edc7709f..aefcb103 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -839,5 +839,6 @@ export function saveSettings(settingsFile: SettingsFile): void { ); } catch (error) { console.error('Error saving user settings file:', error); + throw error; } } diff --git a/packages/cli/src/core/auth.ts b/packages/cli/src/core/auth.ts index 2284a112..15bb5fd3 100644 --- a/packages/cli/src/core/auth.ts +++ b/packages/cli/src/core/auth.ts @@ -8,6 +8,8 @@ import { type AuthType, type Config, getErrorMessage, + logAuth, + AuthEvent, } from '@qwen-code/qwen-code-core'; /** @@ -25,11 +27,21 @@ export async function performInitialAuth( } try { - await config.refreshAuth(authType); + await config.refreshAuth(authType, true); // The console.log is intentionally left out here. // We can add a dedicated startup message later if needed. + + // Log authentication success + const authEvent = new AuthEvent(authType, 'auto', 'success'); + logAuth(config, authEvent); } catch (e) { - return `Failed to login. Message: ${getErrorMessage(e)}`; + const errorMessage = `Failed to login. Message: ${getErrorMessage(e)}`; + + // Log authentication failure + const authEvent = new AuthEvent(authType, 'auto', 'error', errorMessage); + logAuth(config, authEvent); + + return errorMessage; } return null; diff --git a/packages/cli/src/core/initializer.ts b/packages/cli/src/core/initializer.ts index 039b9277..870632d7 100644 --- a/packages/cli/src/core/initializer.ts +++ b/packages/cli/src/core/initializer.ts @@ -11,7 +11,7 @@ import { logIdeConnection, type Config, } from '@qwen-code/qwen-code-core'; -import { type LoadedSettings } from '../config/settings.js'; +import { type LoadedSettings, SettingScope } from '../config/settings.js'; import { performInitialAuth } from './auth.js'; import { validateTheme } from './theme.js'; @@ -33,10 +33,18 @@ export async function initializeApp( config: Config, settings: LoadedSettings, ): Promise { - const authError = await performInitialAuth( - config, - settings.merged.security?.auth?.selectedType, - ); + const authType = settings.merged.security?.auth?.selectedType; + const authError = await performInitialAuth(config, authType); + + // Fallback to user select when initial authentication fails + if (authError) { + settings.setValue( + SettingScope.User, + 'security.auth.selectedType', + undefined, + ); + } + const themeError = validateTheme(settings); const shouldOpenAuthDialog = diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index c3443b43..ecacbda4 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -25,7 +25,6 @@ import { type HistoryItem, ToolCallStatus, type HistoryItemWithoutId, - AuthState, } from './types.js'; import { MessageType, StreamingState } from './types.js'; import { @@ -48,7 +47,6 @@ import { useHistory } from './hooks/useHistoryManager.js'; import { useMemoryMonitor } from './hooks/useMemoryMonitor.js'; import { useThemeCommand } from './hooks/useThemeCommand.js'; import { useAuthCommand } from './auth/useAuth.js'; -import { useQwenAuth } from './hooks/useQwenAuth.js'; import { useQuotaAndFallback } from './hooks/useQuotaAndFallback.js'; import { useEditorSettings } from './hooks/useEditorSettings.js'; import { useSettingsCommand } from './hooks/useSettingsCommand.js'; @@ -93,6 +91,7 @@ import { ShellFocusContext } from './contexts/ShellFocusContext.js'; import { useQuitConfirmation } from './hooks/useQuitConfirmation.js'; import { useWelcomeBack } from './hooks/useWelcomeBack.js'; import { useDialogClose } from './hooks/useDialogClose.js'; +import { useInitializationAuthError } from './hooks/useInitializationAuthError.js'; import { type VisionSwitchOutcome } from './components/ModelSwitchDialog.js'; import { processVisionSwitchOutcome } from './hooks/useVisionAutoSwitch.js'; import { useSubagentCreateDialog } from './hooks/useSubagentCreateDialog.js'; @@ -349,20 +348,13 @@ export const AppContainer = (props: AppContainerProps) => { onAuthError, isAuthDialogOpen, isAuthenticating, + pendingAuthType, + qwenAuthState, handleAuthSelect, openAuthDialog, + cancelAuthentication, } = useAuthCommand(settings, config); - // Qwen OAuth authentication state - const { - isQwenAuth, - isQwenAuthenticating, - deviceAuth, - authStatus, - authMessage, - cancelQwenAuth, - } = useQwenAuth(settings, isAuthenticating); - const { proQuotaRequest, handleProQuotaChoice } = useQuotaAndFallback({ config, historyManager, @@ -371,19 +363,7 @@ export const AppContainer = (props: AppContainerProps) => { setModelSwitchedFromQuotaError, }); - // Handle Qwen OAuth timeout - const handleQwenAuthTimeout = useCallback(() => { - onAuthError('Qwen OAuth authentication timed out. Please try again.'); - cancelQwenAuth(); - setAuthState(AuthState.Updating); - }, [onAuthError, cancelQwenAuth, setAuthState]); - - // Handle Qwen OAuth cancel - const handleQwenAuthCancel = useCallback(() => { - onAuthError('Qwen OAuth authentication cancelled.'); - cancelQwenAuth(); - setAuthState(AuthState.Updating); - }, [onAuthError, cancelQwenAuth, setAuthState]); + useInitializationAuthError(initializationResult.authError, onAuthError); // Sync user tier from config when authentication changes // TODO: Implement getUserTier() method on Config if needed @@ -395,6 +375,8 @@ export const AppContainer = (props: AppContainerProps) => { // Check for enforced auth type mismatch useEffect(() => { + // Check for initialization error first + if ( settings.merged.security?.auth?.enforcedType && settings.merged.security?.auth.selectedType && @@ -959,7 +941,7 @@ export const AppContainer = (props: AppContainerProps) => { handleApprovalModeSelect, isAuthDialogOpen, handleAuthSelect, - selectedAuthType: settings.merged.security?.auth?.selectedType, + pendingAuthType, isEditorDialogOpen, exitEditorDialog, isSettingsDialogOpen, @@ -1201,7 +1183,7 @@ export const AppContainer = (props: AppContainerProps) => { isVisionSwitchDialogOpen || isPermissionsDialogOpen || isAuthDialogOpen || - (isAuthenticating && isQwenAuthenticating) || + isAuthenticating || isEditorDialogOpen || showIdeRestartPrompt || !!proQuotaRequest || @@ -1224,12 +1206,9 @@ export const AppContainer = (props: AppContainerProps) => { isConfigInitialized, authError, isAuthDialogOpen, + pendingAuthType, // Qwen OAuth state - isQwenAuth, - isQwenAuthenticating, - deviceAuth, - authStatus, - authMessage, + qwenAuthState, editorError, isEditorDialogOpen, corgiMode, @@ -1319,12 +1298,9 @@ export const AppContainer = (props: AppContainerProps) => { isConfigInitialized, authError, isAuthDialogOpen, + pendingAuthType, // Qwen OAuth state - isQwenAuth, - isQwenAuthenticating, - deviceAuth, - authStatus, - authMessage, + qwenAuthState, editorError, isEditorDialogOpen, corgiMode, @@ -1418,9 +1394,7 @@ export const AppContainer = (props: AppContainerProps) => { handleAuthSelect, setAuthState, onAuthError, - // Qwen OAuth handlers - handleQwenAuthTimeout, - handleQwenAuthCancel, + cancelAuthentication, handleEditorSelect, exitEditorDialog, closeSettingsDialog, @@ -1454,9 +1428,7 @@ export const AppContainer = (props: AppContainerProps) => { handleAuthSelect, setAuthState, onAuthError, - // Qwen OAuth handlers - handleQwenAuthTimeout, - handleQwenAuthCancel, + cancelAuthentication, handleEditorSelect, exitEditorDialog, closeSettingsDialog, diff --git a/packages/cli/src/ui/auth/AuthDialog.test.tsx b/packages/cli/src/ui/auth/AuthDialog.test.tsx index 2011eac6..34398941 100644 --- a/packages/cli/src/ui/auth/AuthDialog.test.tsx +++ b/packages/cli/src/ui/auth/AuthDialog.test.tsx @@ -9,6 +9,53 @@ import { AuthDialog } from './AuthDialog.js'; import { LoadedSettings, SettingScope } from '../../config/settings.js'; import { AuthType } from '@qwen-code/qwen-code-core'; import { renderWithProviders } from '../../test-utils/render.js'; +import { UIStateContext } from '../contexts/UIStateContext.js'; +import { UIActionsContext } from '../contexts/UIActionsContext.js'; +import type { UIState } from '../contexts/UIStateContext.js'; +import type { UIActions } from '../contexts/UIActionsContext.js'; + +const createMockUIState = (overrides: Partial = {}): UIState => { + // AuthDialog only uses authError and pendingAuthType + const baseState = { + authError: null, + pendingAuthType: undefined, + } as Partial; + + return { + ...baseState, + ...overrides, + } as UIState; +}; + +const createMockUIActions = (overrides: Partial = {}): UIActions => { + // AuthDialog only uses handleAuthSelect + const baseActions = { + handleAuthSelect: vi.fn(), + } as Partial; + + return { + ...baseActions, + ...overrides, + } as UIActions; +}; + +const renderAuthDialog = ( + settings: LoadedSettings, + uiStateOverrides: Partial = {}, + uiActionsOverrides: Partial = {}, +) => { + const uiState = createMockUIState(uiStateOverrides); + const uiActions = createMockUIActions(uiActionsOverrides); + + return renderWithProviders( + + + + + , + { settings }, + ); +}; describe('AuthDialog', () => { const wait = (ms = 50) => new Promise((resolve) => setTimeout(resolve, ms)); @@ -66,13 +113,9 @@ describe('AuthDialog', () => { new Set(), ); - const { lastFrame } = renderWithProviders( - {}} - settings={settings} - initialErrorMessage="GEMINI_API_KEY environment variable not found" - />, - ); + const { lastFrame } = renderAuthDialog(settings, { + authError: 'GEMINI_API_KEY environment variable not found', + }); expect(lastFrame()).toContain( 'GEMINI_API_KEY environment variable not found', @@ -116,9 +159,7 @@ describe('AuthDialog', () => { new Set(), ); - const { lastFrame } = renderWithProviders( - {}} settings={settings} />, - ); + const { lastFrame } = renderAuthDialog(settings); // Since the auth dialog only shows OpenAI option now, // it won't show GEMINI_API_KEY messages @@ -162,9 +203,7 @@ describe('AuthDialog', () => { new Set(), ); - const { lastFrame } = renderWithProviders( - {}} settings={settings} />, - ); + const { lastFrame } = renderAuthDialog(settings); expect(lastFrame()).not.toContain( 'Existing API key detected (GEMINI_API_KEY)', @@ -208,9 +247,7 @@ describe('AuthDialog', () => { new Set(), ); - const { lastFrame } = renderWithProviders( - {}} settings={settings} />, - ); + const { lastFrame } = renderAuthDialog(settings); // Since the auth dialog only shows OpenAI option now, // it won't show GEMINI_API_KEY messages @@ -255,9 +292,7 @@ describe('AuthDialog', () => { new Set(), ); - const { lastFrame } = renderWithProviders( - {}} settings={settings} />, - ); + const { lastFrame } = renderAuthDialog(settings); // This is a bit brittle, but it's the best way to check which item is selected. expect(lastFrame()).toContain('● 2. OpenAI'); @@ -297,9 +332,7 @@ describe('AuthDialog', () => { new Set(), ); - const { lastFrame } = renderWithProviders( - {}} settings={settings} />, - ); + const { lastFrame } = renderAuthDialog(settings); // Default is Qwen OAuth (first option) expect(lastFrame()).toContain('● 1. Qwen OAuth'); @@ -341,9 +374,7 @@ describe('AuthDialog', () => { new Set(), ); - const { lastFrame } = renderWithProviders( - {}} settings={settings} />, - ); + const { lastFrame } = renderAuthDialog(settings); // Since the auth dialog doesn't show QWEN_DEFAULT_AUTH_TYPE errors anymore, // it will just show the default Qwen OAuth option @@ -352,7 +383,7 @@ describe('AuthDialog', () => { }); it('should prevent exiting when no auth method is selected and show error message', async () => { - const onSelect = vi.fn(); + const handleAuthSelect = vi.fn(); const settings: LoadedSettings = new LoadedSettings( { settings: { ui: { customThemes: {} }, mcpServers: {} }, @@ -386,8 +417,10 @@ describe('AuthDialog', () => { new Set(), ); - const { lastFrame, stdin, unmount } = renderWithProviders( - , + const { lastFrame, stdin, unmount } = renderAuthDialog( + settings, + {}, + { handleAuthSelect }, ); await wait(); @@ -395,16 +428,16 @@ describe('AuthDialog', () => { stdin.write('\u001b'); // ESC key await wait(); - // Should show error message instead of calling onSelect + // Should show error message instead of calling handleAuthSelect expect(lastFrame()).toContain( 'You must select an auth method to proceed. Press Ctrl+C again to exit.', ); - expect(onSelect).not.toHaveBeenCalled(); + expect(handleAuthSelect).not.toHaveBeenCalled(); unmount(); }); it('should not exit if there is already an error message', async () => { - const onSelect = vi.fn(); + const handleAuthSelect = vi.fn(); const settings: LoadedSettings = new LoadedSettings( { settings: { ui: { customThemes: {} }, mcpServers: {} }, @@ -438,12 +471,10 @@ describe('AuthDialog', () => { new Set(), ); - const { lastFrame, stdin, unmount } = renderWithProviders( - , + const { lastFrame, stdin, unmount } = renderAuthDialog( + settings, + { authError: 'Initial error' }, + { handleAuthSelect }, ); await wait(); @@ -453,13 +484,13 @@ describe('AuthDialog', () => { stdin.write('\u001b'); // ESC key await wait(); - // Should not call onSelect - expect(onSelect).not.toHaveBeenCalled(); + // Should not call handleAuthSelect + expect(handleAuthSelect).not.toHaveBeenCalled(); unmount(); }); it('should allow exiting when auth method is already selected', async () => { - const onSelect = vi.fn(); + const handleAuthSelect = vi.fn(); const settings: LoadedSettings = new LoadedSettings( { settings: { ui: { customThemes: {} }, mcpServers: {} }, @@ -493,8 +524,10 @@ describe('AuthDialog', () => { new Set(), ); - const { stdin, unmount } = renderWithProviders( - , + const { stdin, unmount } = renderAuthDialog( + settings, + {}, + { handleAuthSelect }, ); await wait(); @@ -502,8 +535,8 @@ describe('AuthDialog', () => { stdin.write('\u001b'); // ESC key await wait(); - // Should call onSelect with undefined to exit - expect(onSelect).toHaveBeenCalledWith(undefined, SettingScope.User); + // Should call handleAuthSelect with undefined to exit + expect(handleAuthSelect).toHaveBeenCalledWith(undefined, SettingScope.User); unmount(); }); }); diff --git a/packages/cli/src/ui/auth/AuthDialog.tsx b/packages/cli/src/ui/auth/AuthDialog.tsx index 9d9baa89..ec0b2577 100644 --- a/packages/cli/src/ui/auth/AuthDialog.tsx +++ b/packages/cli/src/ui/auth/AuthDialog.tsx @@ -8,26 +8,13 @@ import type React from 'react'; import { useState } from 'react'; import { AuthType } from '@qwen-code/qwen-code-core'; import { Box, Text } from 'ink'; -import { validateAuthMethod } from '../../config/auth.js'; -import { type LoadedSettings, SettingScope } from '../../config/settings.js'; +import { SettingScope } from '../../config/settings.js'; import { Colors } from '../colors.js'; import { useKeypress } from '../hooks/useKeypress.js'; -import { OpenAIKeyPrompt } from '../components/OpenAIKeyPrompt.js'; import { RadioButtonSelect } from '../components/shared/RadioButtonSelect.js'; - -interface AuthDialogProps { - onSelect: ( - authMethod: AuthType | undefined, - scope: SettingScope, - credentials?: { - apiKey?: string; - baseUrl?: string; - model?: string; - }, - ) => void; - settings: LoadedSettings; - initialErrorMessage?: string | null; -} +import { useUIState } from '../contexts/UIStateContext.js'; +import { useUIActions } from '../contexts/UIActionsContext.js'; +import { useSettings } from '../contexts/SettingsContext.js'; function parseDefaultAuthType( defaultAuthType: string | undefined, @@ -41,15 +28,14 @@ function parseDefaultAuthType( return null; } -export function AuthDialog({ - onSelect, - settings, - initialErrorMessage, -}: AuthDialogProps): React.JSX.Element { - const [errorMessage, setErrorMessage] = useState( - initialErrorMessage || null, - ); - const [showOpenAIKeyPrompt, setShowOpenAIKeyPrompt] = useState(false); +export function AuthDialog(): React.JSX.Element { + const { pendingAuthType, authError } = useUIState(); + const { handleAuthSelect: onAuthSelect } = useUIActions(); + const settings = useSettings(); + + const [errorMessage, setErrorMessage] = useState(null); + const [selectedIndex, setSelectedIndex] = useState(null); + const items = [ { key: AuthType.QWEN_OAUTH, @@ -62,10 +48,17 @@ export function AuthDialog({ const initialAuthIndex = Math.max( 0, items.findIndex((item) => { + // Priority 1: pendingAuthType + if (pendingAuthType) { + return item.value === pendingAuthType; + } + + // Priority 2: settings.merged.security?.auth?.selectedType if (settings.merged.security?.auth?.selectedType) { return item.value === settings.merged.security?.auth?.selectedType; } + // Priority 3: QWEN_DEFAULT_AUTH_TYPE env var const defaultAuthType = parseDefaultAuthType( process.env['QWEN_DEFAULT_AUTH_TYPE'], ); @@ -73,49 +66,29 @@ export function AuthDialog({ return item.value === defaultAuthType; } + // Priority 4: default to QWEN_OAUTH return item.value === AuthType.QWEN_OAUTH; }), ); - const handleAuthSelect = (authMethod: AuthType) => { - if (authMethod === AuthType.USE_OPENAI) { - setShowOpenAIKeyPrompt(true); - setErrorMessage(null); - } else { - const error = validateAuthMethod(authMethod); - if (error) { - setErrorMessage(error); - } else { - setErrorMessage(null); - onSelect(authMethod, SettingScope.User); - } - } + const hasApiKey = Boolean(settings.merged.security?.auth?.apiKey); + const currentSelectedAuthType = + selectedIndex !== null + ? items[selectedIndex]?.value + : items[initialAuthIndex]?.value; + + const handleAuthSelect = async (authMethod: AuthType) => { + setErrorMessage(null); + await onAuthSelect(authMethod, SettingScope.User); }; - const handleOpenAIKeySubmit = ( - apiKey: string, - baseUrl: string, - model: string, - ) => { - setShowOpenAIKeyPrompt(false); - onSelect(AuthType.USE_OPENAI, SettingScope.User, { - apiKey, - baseUrl, - model, - }); - }; - - const handleOpenAIKeyCancel = () => { - setShowOpenAIKeyPrompt(false); - setErrorMessage('OpenAI API key is required to use OpenAI authentication.'); + const handleHighlight = (authMethod: AuthType) => { + const index = items.findIndex((item) => item.value === authMethod); + setSelectedIndex(index); }; useKeypress( (key) => { - if (showOpenAIKeyPrompt) { - return; - } - if (key.name === 'escape') { // Prevent exit if there is an error message. // This means they user is not authenticated yet. @@ -129,33 +102,11 @@ export function AuthDialog({ ); return; } - onSelect(undefined, SettingScope.User); + onAuthSelect(undefined, SettingScope.User); } }, { isActive: true }, ); - const getDefaultOpenAIConfig = () => { - const fromSettings = settings.merged.security?.auth; - const modelSettings = settings.merged.model; - return { - apiKey: fromSettings?.apiKey || process.env['OPENAI_API_KEY'] || '', - baseUrl: fromSettings?.baseUrl || process.env['OPENAI_BASE_URL'] || '', - model: modelSettings?.name || process.env['OPENAI_MODEL'] || '', - }; - }; - - if (showOpenAIKeyPrompt) { - const defaults = getDefaultOpenAIConfig(); - return ( - - ); - } return ( - {errorMessage && ( + {(authError || errorMessage) && ( - {errorMessage} + {authError || errorMessage} )} (Use Enter to Set Auth) + {hasApiKey && currentSelectedAuthType === AuthType.QWEN_OAUTH && ( + + + Note: Your existing API key in settings.json will not be cleared + when using Qwen OAuth. You can switch back to OpenAI authentication + later if needed. + + + )} Terms of Services and Privacy Notice for Qwen Code diff --git a/packages/cli/src/ui/auth/useAuth.ts b/packages/cli/src/ui/auth/useAuth.ts index e761043d..9b1198bf 100644 --- a/packages/cli/src/ui/auth/useAuth.ts +++ b/packages/cli/src/ui/auth/useAuth.ts @@ -6,27 +6,19 @@ import { useState, useCallback, useEffect } from 'react'; import type { LoadedSettings, SettingScope } from '../../config/settings.js'; -import type { AuthType, Config } from '@qwen-code/qwen-code-core'; +import type { Config } from '@qwen-code/qwen-code-core'; import { + AuthType, clearCachedCredentialFile, getErrorMessage, + logAuth, + AuthEvent, } from '@qwen-code/qwen-code-core'; import { AuthState } from '../types.js'; -import { validateAuthMethod } from '../../config/auth.js'; +import { useQwenAuth } from '../hooks/useQwenAuth.js'; +import type { OpenAICredentials } from '../components/OpenAIKeyPrompt.js'; -export function validateAuthMethodWithSettings( - authType: AuthType, - settings: LoadedSettings, -): string | null { - const enforcedType = settings.merged.security?.auth?.enforcedType; - if (enforcedType && enforcedType !== authType) { - return `Authentication is enforced to be ${enforcedType}, but you are currently using ${authType}.`; - } - if (settings.merged.security?.auth?.useExternal) { - return null; - } - return validateAuthMethod(authType); -} +export type { QwenAuthState } from '../hooks/useQwenAuth.js'; export const useAuthCommand = (settings: LoadedSettings, config: Config) => { const unAuthenticated = @@ -40,6 +32,14 @@ export const useAuthCommand = (settings: LoadedSettings, config: Config) => { const [isAuthenticating, setIsAuthenticating] = useState(false); const [isAuthDialogOpen, setIsAuthDialogOpen] = useState(unAuthenticated); + const [pendingAuthType, setPendingAuthType] = useState( + undefined, + ); + + const { qwenAuthState, cancelQwenAuth } = useQwenAuth( + pendingAuthType, + isAuthenticating, + ); const onAuthError = useCallback( (error: string | null) => { @@ -52,90 +52,123 @@ export const useAuthCommand = (settings: LoadedSettings, config: Config) => { [setAuthError, setAuthState], ); - // Authentication flow - useEffect(() => { - const authFlow = async () => { - const authType = settings.merged.security?.auth?.selectedType; - if (isAuthDialogOpen || !authType) { - return; + const handleAuthFailure = useCallback( + (error: unknown) => { + setIsAuthenticating(false); + const errorMessage = `Failed to authenticate. Message: ${getErrorMessage(error)}`; + onAuthError(errorMessage); + + // Log authentication failure + if (pendingAuthType) { + const authEvent = new AuthEvent( + pendingAuthType, + 'manual', + 'error', + errorMessage, + ); + logAuth(config, authEvent); } + }, + [onAuthError, pendingAuthType, config], + ); - const validationError = validateAuthMethodWithSettings( - authType, - settings, - ); - if (validationError) { - onAuthError(validationError); - return; - } - - try { - setIsAuthenticating(true); - await config.refreshAuth(authType); - console.log(`Authenticated via "${authType}".`); - setAuthError(null); - setAuthState(AuthState.Authenticated); - } catch (e) { - onAuthError(`Failed to login. Message: ${getErrorMessage(e)}`); - } finally { - setIsAuthenticating(false); - } - }; - - void authFlow(); - }, [isAuthDialogOpen, settings, config, onAuthError]); - - // Handle auth selection from dialog - const handleAuthSelect = useCallback( + const handleAuthSuccess = useCallback( async ( - authType: AuthType | undefined, + authType: AuthType, scope: SettingScope, - credentials?: { - apiKey?: string; - baseUrl?: string; - model?: string; - }, + credentials?: OpenAICredentials, ) => { - if (authType) { - await clearCachedCredentialFile(); + try { + settings.setValue(scope, 'security.auth.selectedType', authType); - // Save OpenAI credentials if provided - if (credentials) { - // Update Config's internal generationConfig before calling refreshAuth - // This ensures refreshAuth has access to the new credentials - config.updateCredentials({ - apiKey: credentials.apiKey, - baseUrl: credentials.baseUrl, - model: credentials.model, - }); - - // Also set environment variables for compatibility with other parts of the code - if (credentials.apiKey) { + // Only update credentials if not switching to QWEN_OAUTH, + // so that OpenAI credentials are preserved when switching to QWEN_OAUTH. + if (authType !== AuthType.QWEN_OAUTH && credentials) { + if (credentials?.apiKey != null) { settings.setValue( scope, 'security.auth.apiKey', credentials.apiKey, ); } - if (credentials.baseUrl) { + if (credentials?.baseUrl != null) { settings.setValue( scope, 'security.auth.baseUrl', credentials.baseUrl, ); } - if (credentials.model) { + if (credentials?.model != null) { settings.setValue(scope, 'model.name', credentials.model); } + await clearCachedCredentialFile(); } - - settings.setValue(scope, 'security.auth.selectedType', authType); + } catch (error) { + handleAuthFailure(error); + return; } - setIsAuthDialogOpen(false); setAuthError(null); + setAuthState(AuthState.Authenticated); + setPendingAuthType(undefined); + setIsAuthDialogOpen(false); + setIsAuthenticating(false); + + // Log authentication success + const authEvent = new AuthEvent(authType, 'manual', 'success'); + logAuth(config, authEvent); }, - [settings, config], + [settings, handleAuthFailure, config], + ); + + const performAuth = useCallback( + async ( + authType: AuthType, + scope: SettingScope, + credentials?: OpenAICredentials, + ) => { + try { + await config.refreshAuth(authType); + handleAuthSuccess(authType, scope, credentials); + } catch (e) { + handleAuthFailure(e); + } + }, + [config, handleAuthSuccess, handleAuthFailure], + ); + + const handleAuthSelect = useCallback( + async ( + authType: AuthType | undefined, + scope: SettingScope, + credentials?: OpenAICredentials, + ) => { + if (!authType) { + setIsAuthDialogOpen(false); + setAuthError(null); + return; + } + + setPendingAuthType(authType); + setAuthError(null); + setIsAuthDialogOpen(false); + setIsAuthenticating(true); + + if (authType === AuthType.USE_OPENAI) { + if (credentials) { + config.updateCredentials({ + apiKey: credentials.apiKey, + baseUrl: credentials.baseUrl, + model: credentials.model, + }); + await performAuth(authType, scope, credentials); + } + return; + } + + await performAuth(authType, scope); + }, + [config, performAuth], ); const openAuthDialog = useCallback(() => { @@ -143,8 +176,45 @@ export const useAuthCommand = (settings: LoadedSettings, config: Config) => { }, []); const cancelAuthentication = useCallback(() => { + if (isAuthenticating && pendingAuthType === AuthType.QWEN_OAUTH) { + cancelQwenAuth(); + } + + // Log authentication cancellation + if (isAuthenticating && pendingAuthType) { + const authEvent = new AuthEvent(pendingAuthType, 'manual', 'cancelled'); + logAuth(config, authEvent); + } + + // Do not reset pendingAuthType here, persist the previously selected type. setIsAuthenticating(false); - }, []); + setIsAuthDialogOpen(true); + setAuthError(null); + }, [isAuthenticating, pendingAuthType, cancelQwenAuth, config]); + + /** + /** + * We previously used a useEffect to trigger authentication automatically when + * settings.security.auth.selectedType changed. This caused problems: if authentication failed, + * the UI could get stuck, since settings.json would update before success. Now, we + * update selectedType in settings only when authentication fully succeeds. + * Authentication is triggered explicitly—either during initial app startup or when the + * user switches methods—not reactively through settings changes. This avoids repeated + * or broken authentication cycles. + */ + useEffect(() => { + const defaultAuthType = process.env['QWEN_DEFAULT_AUTH_TYPE']; + if ( + defaultAuthType && + ![AuthType.QWEN_OAUTH, AuthType.USE_OPENAI].includes( + defaultAuthType as AuthType, + ) + ) { + onAuthError( + `Invalid QWEN_DEFAULT_AUTH_TYPE value: "${defaultAuthType}". Valid values are: ${[AuthType.QWEN_OAUTH, AuthType.USE_OPENAI].join(', ')}`, + ); + } + }, [onAuthError]); return { authState, @@ -153,6 +223,8 @@ export const useAuthCommand = (settings: LoadedSettings, config: Config) => { onAuthError, isAuthDialogOpen, isAuthenticating, + pendingAuthType, + qwenAuthState, handleAuthSelect, openAuthDialog, cancelAuthentication, diff --git a/packages/cli/src/ui/components/DialogManager.tsx b/packages/cli/src/ui/components/DialogManager.tsx index 01d95392..75795797 100644 --- a/packages/cli/src/ui/components/DialogManager.tsx +++ b/packages/cli/src/ui/components/DialogManager.tsx @@ -12,9 +12,9 @@ import { ShellConfirmationDialog } from './ShellConfirmationDialog.js'; import { ConsentPrompt } from './ConsentPrompt.js'; import { ThemeDialog } from './ThemeDialog.js'; import { SettingsDialog } from './SettingsDialog.js'; -import { AuthInProgress } from '../auth/AuthInProgress.js'; import { QwenOAuthProgress } from './QwenOAuthProgress.js'; import { AuthDialog } from '../auth/AuthDialog.js'; +import { OpenAIKeyPrompt } from './OpenAIKeyPrompt.js'; import { EditorSettingsDialog } from './EditorSettingsDialog.js'; import { WorkspaceMigrationDialog } from './WorkspaceMigrationDialog.js'; import { ProQuotaDialog } from './ProQuotaDialog.js'; @@ -26,6 +26,9 @@ import { useUIState } from '../contexts/UIStateContext.js'; import { useUIActions } from '../contexts/UIActionsContext.js'; import { useConfig } from '../contexts/ConfigContext.js'; import { useSettings } from '../contexts/SettingsContext.js'; +import { SettingScope } from '../../config/settings.js'; +import { AuthState } from '../types.js'; +import { AuthType } from '@qwen-code/qwen-code-core'; import process from 'node:process'; import { type UseHistoryManagerReturn } from '../hooks/useHistoryManager.js'; import { IdeTrustChangeDialog } from './IdeTrustChangeDialog.js'; @@ -56,6 +59,16 @@ export const DialogManager = ({ const { constrainHeight, terminalHeight, staticExtraHeight, mainAreaWidth } = uiState; + const getDefaultOpenAIConfig = () => { + const fromSettings = settings.merged.security?.auth; + const modelSettings = settings.merged.model; + return { + apiKey: fromSettings?.apiKey || process.env['OPENAI_API_KEY'] || '', + baseUrl: fromSettings?.baseUrl || process.env['OPENAI_BASE_URL'] || '', + model: modelSettings?.name || process.env['OPENAI_MODEL'] || '', + }; + }; + if (uiState.showWelcomeBackDialog && uiState.welcomeBackInfo?.hasHistory) { return ( ; } + + if (uiState.isAuthDialogOpen || uiState.authError) { + return ( + + + + ); + } + if (uiState.isAuthenticating) { - // Show Qwen OAuth progress if it's Qwen auth and OAuth is active - if (uiState.isQwenAuth && uiState.isQwenAuthenticating) { + if (uiState.pendingAuthType === AuthType.USE_OPENAI) { + const defaults = getDefaultOpenAIConfig(); return ( - { + uiActions.handleAuthSelect(AuthType.USE_OPENAI, SettingScope.User, { + apiKey, + baseUrl, + model, + }); + }} + onCancel={() => { + uiActions.cancelAuthentication(); + uiActions.setAuthState(AuthState.Updating); + }} + defaultApiKey={defaults.apiKey} + defaultBaseUrl={defaults.baseUrl} + defaultModel={defaults.model} /> ); } - // Default auth progress for other auth types - return ( - { - uiActions.onAuthError('Authentication cancelled.'); - }} - /> - ); - } - if (uiState.isAuthDialogOpen) { - return ( - - { + uiActions.onAuthError('Qwen OAuth authentication timed out.'); + uiActions.cancelAuthentication(); + uiActions.setAuthState(AuthState.Updating); + }} + onCancel={() => { + uiActions.cancelAuthentication(); + uiActions.setAuthState(AuthState.Updating); + }} /> - - ); + ); + } } if (uiState.isEditorDialogOpen) { return ( diff --git a/packages/cli/src/ui/components/OpenAIKeyPrompt.tsx b/packages/cli/src/ui/components/OpenAIKeyPrompt.tsx index bc78b8c5..0dc89bc7 100644 --- a/packages/cli/src/ui/components/OpenAIKeyPrompt.tsx +++ b/packages/cli/src/ui/components/OpenAIKeyPrompt.tsx @@ -6,6 +6,7 @@ import type React from 'react'; import { useState } from 'react'; +import { z } from 'zod'; import { Box, Text } from 'ink'; import { Colors } from '../colors.js'; import { useKeypress } from '../hooks/useKeypress.js'; @@ -18,6 +19,16 @@ interface OpenAIKeyPromptProps { defaultModel?: string; } +export const credentialSchema = z.object({ + apiKey: z.string().min(1, 'API key is required'), + baseUrl: z + .union([z.string().url('Base URL must be a valid URL'), z.literal('')]) + .optional(), + model: z.string().min(1, 'Model must be a non-empty string').optional(), +}); + +export type OpenAICredentials = z.infer; + export function OpenAIKeyPrompt({ onSubmit, onCancel, @@ -31,6 +42,34 @@ export function OpenAIKeyPrompt({ const [currentField, setCurrentField] = useState< 'apiKey' | 'baseUrl' | 'model' >('apiKey'); + const [validationError, setValidationError] = useState(null); + + const validateAndSubmit = () => { + setValidationError(null); + + try { + const validated = credentialSchema.parse({ + apiKey: apiKey.trim(), + baseUrl: baseUrl.trim() || undefined, + model: model.trim() || undefined, + }); + + onSubmit( + validated.apiKey, + validated.baseUrl === '' ? '' : validated.baseUrl || '', + validated.model || '', + ); + } catch (error) { + if (error instanceof z.ZodError) { + const errorMessage = error.errors + .map((e) => `${e.path.join('.')}: ${e.message}`) + .join(', '); + setValidationError(`Invalid credentials: ${errorMessage}`); + } else { + setValidationError('Failed to validate credentials'); + } + } + }; useKeypress( (key) => { @@ -52,7 +91,7 @@ export function OpenAIKeyPrompt({ } else if (currentField === 'model') { // 只有在提交时才检查 API key 是否为空 if (apiKey.trim()) { - onSubmit(apiKey.trim(), baseUrl.trim(), model.trim()); + validateAndSubmit(); } else { // 如果 API key 为空，回到 API key 字段 setCurrentField('apiKey'); @@ -168,6 +207,11 @@ export function OpenAIKeyPrompt({ OpenAI Configuration Required + {validationError && ( + + {validationError} + + )} Please enter your OpenAI configuration. You can get an API key from{' '} diff --git a/packages/cli/src/ui/components/QwenOAuthProgress.test.tsx b/packages/cli/src/ui/components/QwenOAuthProgress.test.tsx index 3cd31375..a93db2b7 100644 --- a/packages/cli/src/ui/components/QwenOAuthProgress.test.tsx +++ b/packages/cli/src/ui/components/QwenOAuthProgress.test.tsx @@ -8,7 +8,7 @@ import { render } from 'ink-testing-library'; import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { QwenOAuthProgress } from './QwenOAuthProgress.js'; -import type { DeviceAuthorizationInfo } from '../hooks/useQwenAuth.js'; +import type { DeviceAuthorizationData } from '@qwen-code/qwen-code-core'; import { useKeypress } from '../hooks/useKeypress.js'; import type { Key } from '../contexts/KeypressContext.js'; @@ -42,12 +42,13 @@ describe('QwenOAuthProgress', () => { let keypressHandler: ((key: Key) => void) | null = null; const createMockDeviceAuth = ( - overrides: Partial = {}, - ): DeviceAuthorizationInfo => ({ + overrides: Partial = {}, + ): DeviceAuthorizationData => ({ verification_uri: 'https://example.com/device', verification_uri_complete: 'https://example.com/device?user_code=ABC123', user_code: 'ABC123', expires_in: 300, + device_code: 'test-device-code', ...overrides, }); @@ -55,7 +56,7 @@ describe('QwenOAuthProgress', () => { const renderComponent = ( props: Partial<{ - deviceAuth: DeviceAuthorizationInfo; + deviceAuth: DeviceAuthorizationData; authStatus: | 'idle' | 'polling' @@ -158,7 +159,7 @@ describe('QwenOAuthProgress', () => { }); it('should format time correctly', () => { - const deviceAuthWithCustomTime: DeviceAuthorizationInfo = { + const deviceAuthWithCustomTime: DeviceAuthorizationData = { ...mockDeviceAuth, expires_in: 125, // 2 minutes and 5 seconds }; @@ -176,7 +177,7 @@ describe('QwenOAuthProgress', () => { }); it('should format single digit seconds with leading zero', () => { - const deviceAuthWithCustomTime: DeviceAuthorizationInfo = { + const deviceAuthWithCustomTime: DeviceAuthorizationData = { ...mockDeviceAuth, expires_in: 67, // 1 minute and 7 seconds }; @@ -196,7 +197,7 @@ describe('QwenOAuthProgress', () => { describe('Timer functionality', () => { it('should countdown and call onTimeout when timer expires', async () => { - const deviceAuthWithShortTime: DeviceAuthorizationInfo = { + const deviceAuthWithShortTime: DeviceAuthorizationData = { ...mockDeviceAuth, expires_in: 2, // 2 seconds }; @@ -520,7 +521,7 @@ describe('QwenOAuthProgress', () => { describe('Props changes', () => { it('should display initial timer value from deviceAuth', () => { - const deviceAuthWith10Min: DeviceAuthorizationInfo = { + const deviceAuthWith10Min: DeviceAuthorizationData = { ...mockDeviceAuth, expires_in: 600, // 10 minutes }; diff --git a/packages/cli/src/ui/components/QwenOAuthProgress.tsx b/packages/cli/src/ui/components/QwenOAuthProgress.tsx index 685cb107..3e630fb3 100644 --- a/packages/cli/src/ui/components/QwenOAuthProgress.tsx +++ b/packages/cli/src/ui/components/QwenOAuthProgress.tsx @@ -11,13 +11,13 @@ import Spinner from 'ink-spinner'; import Link from 'ink-link'; import qrcode from 'qrcode-terminal'; import { Colors } from '../colors.js'; -import type { DeviceAuthorizationInfo } from '../hooks/useQwenAuth.js'; +import type { DeviceAuthorizationData } from '@qwen-code/qwen-code-core'; import { useKeypress } from '../hooks/useKeypress.js'; interface QwenOAuthProgressProps { onTimeout: () => void; onCancel: () => void; - deviceAuth?: DeviceAuthorizationInfo; + deviceAuth?: DeviceAuthorizationData; authStatus?: | 'idle' | 'polling' @@ -131,8 +131,8 @@ export function QwenOAuthProgress({ useKeypress( (key) => { - if (authStatus === 'timeout') { - // Any key press in timeout state should trigger cancel to return to auth dialog + if (authStatus === 'timeout' || authStatus === 'error') { + // Any key press in timeout or error state should trigger cancel to return to auth dialog onCancel(); } else if (key.name === 'escape' || (key.ctrl && key.name === 'c')) { onCancel(); @@ -234,6 +234,35 @@ export function QwenOAuthProgress({ ); } + if (authStatus === 'error') { + return ( + + + Qwen OAuth Authentication Error + + + + + {authMessage || + 'An error occurred during authentication. Please try again.'} + + + + + + Press any key to return to authentication type selection. + + + + ); + } + // Show loading state when no device auth is available yet if (!deviceAuth) { return ( diff --git a/packages/cli/src/ui/components/SettingsDialog.test.tsx b/packages/cli/src/ui/components/SettingsDialog.test.tsx index 25ba9ec0..bbd18ecf 100644 --- a/packages/cli/src/ui/components/SettingsDialog.test.tsx +++ b/packages/cli/src/ui/components/SettingsDialog.test.tsx @@ -487,8 +487,11 @@ describe('SettingsDialog', () => { it('loops back when reaching the end of an enum', async () => { vi.mocked(saveModifiedSettings).mockClear(); vi.mocked(getSettingsSchema).mockReturnValue(FAKE_SCHEMA); - const settings = createMockSettings(); - settings.setValue(SettingScope.User, 'ui.theme', StringEnum.BAZ); + const settings = createMockSettings({ + ui: { + theme: StringEnum.BAZ, + }, + }); const onSelect = vi.fn(); const component = ( diff --git a/packages/cli/src/ui/contexts/UIActionsContext.tsx b/packages/cli/src/ui/contexts/UIActionsContext.tsx index 409b4c4c..4788f7fa 100644 --- a/packages/cli/src/ui/contexts/UIActionsContext.tsx +++ b/packages/cli/src/ui/contexts/UIActionsContext.tsx @@ -16,6 +16,7 @@ import { import { type SettingScope } from '../../config/settings.js'; import type { AuthState } from '../types.js'; import { type VisionSwitchOutcome } from '../components/ModelSwitchDialog.js'; +import { type OpenAICredentials } from '../components/OpenAIKeyPrompt.js'; export interface UIActions { handleThemeSelect: ( @@ -30,12 +31,11 @@ export interface UIActions { handleAuthSelect: ( authType: AuthType | undefined, scope: SettingScope, - ) => void; + credentials?: OpenAICredentials, + ) => Promise; setAuthState: (state: AuthState) => void; onAuthError: (error: string) => void; - // Qwen OAuth handlers - handleQwenAuthTimeout: () => void; - handleQwenAuthCancel: () => void; + cancelAuthentication: () => void; handleEditorSelect: ( editorType: EditorType | undefined, scope: SettingScope, diff --git a/packages/cli/src/ui/contexts/UIStateContext.tsx b/packages/cli/src/ui/contexts/UIStateContext.tsx index fae2db66..21ff5389 100644 --- a/packages/cli/src/ui/contexts/UIStateContext.tsx +++ b/packages/cli/src/ui/contexts/UIStateContext.tsx @@ -16,10 +16,11 @@ import type { HistoryItemWithoutId, StreamingState, } from '../types.js'; -import type { DeviceAuthorizationInfo } from '../hooks/useQwenAuth.js'; +import type { QwenAuthState } from '../hooks/useQwenAuth.js'; import type { CommandContext, SlashCommand } from '../commands/types.js'; import type { TextBuffer } from '../components/shared/text-buffer.js'; import type { + AuthType, IdeContext, ApprovalMode, UserTierId, @@ -49,18 +50,9 @@ export interface UIState { isConfigInitialized: boolean; authError: string | null; isAuthDialogOpen: boolean; + pendingAuthType: AuthType | undefined; // Qwen OAuth state - isQwenAuth: boolean; - isQwenAuthenticating: boolean; - deviceAuth: DeviceAuthorizationInfo | null; - authStatus: - | 'idle' - | 'polling' - | 'success' - | 'error' - | 'timeout' - | 'rate_limit'; - authMessage: string | null; + qwenAuthState: QwenAuthState; editorError: string | null; isEditorDialogOpen: boolean; corgiMode: boolean; diff --git a/packages/cli/src/ui/hooks/useDialogClose.ts b/packages/cli/src/ui/hooks/useDialogClose.ts index 06e221ac..70a06abc 100644 --- a/packages/cli/src/ui/hooks/useDialogClose.ts +++ b/packages/cli/src/ui/hooks/useDialogClose.ts @@ -7,6 +7,7 @@ import { useCallback } from 'react'; import { SettingScope } from '../../config/settings.js'; import type { AuthType, ApprovalMode } from '@qwen-code/qwen-code-core'; +import type { OpenAICredentials } from '../components/OpenAIKeyPrompt.js'; export interface DialogCloseOptions { // Theme dialog @@ -25,8 +26,9 @@ export interface DialogCloseOptions { handleAuthSelect: ( authType: AuthType | undefined, scope: SettingScope, + credentials?: OpenAICredentials, ) => Promise; - selectedAuthType: AuthType | undefined; + pendingAuthType: AuthType | undefined; // Editor dialog isEditorDialogOpen: boolean; diff --git a/packages/cli/src/ui/hooks/useInitializationAuthError.ts b/packages/cli/src/ui/hooks/useInitializationAuthError.ts new file mode 100644 index 00000000..bb25d323 --- /dev/null +++ b/packages/cli/src/ui/hooks/useInitializationAuthError.ts @@ -0,0 +1,47 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { useEffect, useRef } from 'react'; + +/** + * Hook that handles initialization authentication error only once. + * This ensures that if an auth error occurred during app initialization, + * it is reported to the user exactly once, even if the component re-renders. + * + * @param authError - The authentication error from initialization, or null if no error. + * @param onAuthError - Callback function to handle the authentication error. + * + * @example + * ```tsx + * useInitializationAuthError( + * initializationResult.authError, + * onAuthError + * ); + * ``` + */ +export const useInitializationAuthError = ( + authError: string | null, + onAuthError: (error: string) => void, +): void => { + const hasHandled = useRef(false); + const authErrorRef = useRef(authError); + const onAuthErrorRef = useRef(onAuthError); + + // Update refs to always use latest values + authErrorRef.current = authError; + onAuthErrorRef.current = onAuthError; + + useEffect(() => { + if (hasHandled.current) { + return; + } + + if (authErrorRef.current) { + hasHandled.current = true; + onAuthErrorRef.current(authErrorRef.current); + } + }, [authError, onAuthError]); +}; diff --git a/packages/cli/src/ui/hooks/useQwenAuth.test.ts b/packages/cli/src/ui/hooks/useQwenAuth.test.ts index 1e104136..06644a00 100644 --- a/packages/cli/src/ui/hooks/useQwenAuth.test.ts +++ b/packages/cli/src/ui/hooks/useQwenAuth.test.ts @@ -6,14 +6,13 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { renderHook, act } from '@testing-library/react'; -import type { DeviceAuthorizationInfo } from './useQwenAuth.js'; +import type { DeviceAuthorizationData } from '@qwen-code/qwen-code-core'; import { useQwenAuth } from './useQwenAuth.js'; import { AuthType, qwenOAuth2Events, QwenOAuth2Event, } from '@qwen-code/qwen-code-core'; -import type { LoadedSettings } from '../../config/settings.js'; // Mock the qwenOAuth2Events vi.mock('@qwen-code/qwen-code-core', async () => { @@ -36,24 +35,14 @@ vi.mock('@qwen-code/qwen-code-core', async () => { const mockQwenOAuth2Events = vi.mocked(qwenOAuth2Events); describe('useQwenAuth', () => { - const mockDeviceAuth: DeviceAuthorizationInfo = { + const mockDeviceAuth: DeviceAuthorizationData = { verification_uri: 'https://oauth.qwen.com/device', verification_uri_complete: 'https://oauth.qwen.com/device?user_code=ABC123', user_code: 'ABC123', expires_in: 1800, + device_code: 'device_code_123', }; - const createMockSettings = (authType: AuthType): LoadedSettings => - ({ - merged: { - security: { - auth: { - selectedType: authType, - }, - }, - }, - }) as LoadedSettings; - beforeEach(() => { vi.clearAllMocks(); }); @@ -63,36 +52,33 @@ describe('useQwenAuth', () => { }); it('should initialize with default state when not Qwen auth', () => { - const settings = createMockSettings(AuthType.USE_GEMINI); - const { result } = renderHook(() => useQwenAuth(settings, false)); + const { result } = renderHook(() => + useQwenAuth(AuthType.USE_GEMINI, false), + ); - expect(result.current).toEqual({ - isQwenAuthenticating: false, + expect(result.current.qwenAuthState).toEqual({ deviceAuth: null, authStatus: 'idle', authMessage: null, - isQwenAuth: false, - cancelQwenAuth: expect.any(Function), }); + expect(result.current.cancelQwenAuth).toBeInstanceOf(Function); }); it('should initialize with default state when Qwen auth but not authenticating', () => { - const settings = createMockSettings(AuthType.QWEN_OAUTH); - const { result } = renderHook(() => useQwenAuth(settings, false)); + const { result } = renderHook(() => + useQwenAuth(AuthType.QWEN_OAUTH, false), + ); - expect(result.current).toEqual({ - isQwenAuthenticating: false, + expect(result.current.qwenAuthState).toEqual({ deviceAuth: null, authStatus: 'idle', authMessage: null, - isQwenAuth: true, - cancelQwenAuth: expect.any(Function), }); + expect(result.current.cancelQwenAuth).toBeInstanceOf(Function); }); it('should set up event listeners when Qwen auth and authenticating', () => { - const settings = createMockSettings(AuthType.QWEN_OAUTH); - renderHook(() => useQwenAuth(settings, true)); + renderHook(() => useQwenAuth(AuthType.QWEN_OAUTH, true)); expect(mockQwenOAuth2Events.on).toHaveBeenCalledWith( QwenOAuth2Event.AuthUri, @@ -105,8 +91,7 @@ describe('useQwenAuth', () => { }); it('should handle device auth event', () => { - const settings = createMockSettings(AuthType.QWEN_OAUTH); - let handleDeviceAuth: (deviceAuth: DeviceAuthorizationInfo) => void; + let handleDeviceAuth: (deviceAuth: DeviceAuthorizationData) => void; mockQwenOAuth2Events.on.mockImplementation((event, handler) => { if (event === QwenOAuth2Event.AuthUri) { @@ -115,19 +100,17 @@ describe('useQwenAuth', () => { return mockQwenOAuth2Events; }); - const { result } = renderHook(() => useQwenAuth(settings, true)); + const { result } = renderHook(() => useQwenAuth(AuthType.QWEN_OAUTH, true)); act(() => { handleDeviceAuth!(mockDeviceAuth); }); - expect(result.current.deviceAuth).toEqual(mockDeviceAuth); - expect(result.current.authStatus).toBe('polling'); - expect(result.current.isQwenAuthenticating).toBe(true); + expect(result.current.qwenAuthState.deviceAuth).toEqual(mockDeviceAuth); + expect(result.current.qwenAuthState.authStatus).toBe('polling'); }); it('should handle auth progress event - success', () => { - const settings = createMockSettings(AuthType.QWEN_OAUTH); let handleAuthProgress: ( status: 'success' | 'error' | 'polling' | 'timeout' | 'rate_limit', message?: string, @@ -140,18 +123,19 @@ describe('useQwenAuth', () => { return mockQwenOAuth2Events; }); - const { result } = renderHook(() => useQwenAuth(settings, true)); + const { result } = renderHook(() => useQwenAuth(AuthType.QWEN_OAUTH, true)); act(() => { handleAuthProgress!('success', 'Authentication successful!'); }); - expect(result.current.authStatus).toBe('success'); - expect(result.current.authMessage).toBe('Authentication successful!'); + expect(result.current.qwenAuthState.authStatus).toBe('success'); + expect(result.current.qwenAuthState.authMessage).toBe( + 'Authentication successful!', + ); }); it('should handle auth progress event - error', () => { - const settings = createMockSettings(AuthType.QWEN_OAUTH); let handleAuthProgress: ( status: 'success' | 'error' | 'polling' | 'timeout' | 'rate_limit', message?: string, @@ -164,18 +148,19 @@ describe('useQwenAuth', () => { return mockQwenOAuth2Events; }); - const { result } = renderHook(() => useQwenAuth(settings, true)); + const { result } = renderHook(() => useQwenAuth(AuthType.QWEN_OAUTH, true)); act(() => { handleAuthProgress!('error', 'Authentication failed'); }); - expect(result.current.authStatus).toBe('error'); - expect(result.current.authMessage).toBe('Authentication failed'); + expect(result.current.qwenAuthState.authStatus).toBe('error'); + expect(result.current.qwenAuthState.authMessage).toBe( + 'Authentication failed', + ); }); it('should handle auth progress event - polling', () => { - const settings = createMockSettings(AuthType.QWEN_OAUTH); let handleAuthProgress: ( status: 'success' | 'error' | 'polling' | 'timeout' | 'rate_limit', message?: string, @@ -188,20 +173,19 @@ describe('useQwenAuth', () => { return mockQwenOAuth2Events; }); - const { result } = renderHook(() => useQwenAuth(settings, true)); + const { result } = renderHook(() => useQwenAuth(AuthType.QWEN_OAUTH, true)); act(() => { handleAuthProgress!('polling', 'Waiting for user authorization...'); }); - expect(result.current.authStatus).toBe('polling'); - expect(result.current.authMessage).toBe( + expect(result.current.qwenAuthState.authStatus).toBe('polling'); + expect(result.current.qwenAuthState.authMessage).toBe( 'Waiting for user authorization...', ); }); it('should handle auth progress event - rate_limit', () => { - const settings = createMockSettings(AuthType.QWEN_OAUTH); let handleAuthProgress: ( status: 'success' | 'error' | 'polling' | 'timeout' | 'rate_limit', message?: string, @@ -214,7 +198,7 @@ describe('useQwenAuth', () => { return mockQwenOAuth2Events; }); - const { result } = renderHook(() => useQwenAuth(settings, true)); + const { result } = renderHook(() => useQwenAuth(AuthType.QWEN_OAUTH, true)); act(() => { handleAuthProgress!( @@ -223,14 +207,13 @@ describe('useQwenAuth', () => { ); }); - expect(result.current.authStatus).toBe('rate_limit'); - expect(result.current.authMessage).toBe( + expect(result.current.qwenAuthState.authStatus).toBe('rate_limit'); + expect(result.current.qwenAuthState.authMessage).toBe( 'Too many requests. The server is rate limiting our requests. Please select a different authentication method or try again later.', ); }); it('should handle auth progress event without message', () => { - const settings = createMockSettings(AuthType.QWEN_OAUTH); let handleAuthProgress: ( status: 'success' | 'error' | 'polling' | 'timeout' | 'rate_limit', message?: string, @@ -243,27 +226,30 @@ describe('useQwenAuth', () => { return mockQwenOAuth2Events; }); - const { result } = renderHook(() => useQwenAuth(settings, true)); + const { result } = renderHook(() => useQwenAuth(AuthType.QWEN_OAUTH, true)); act(() => { handleAuthProgress!('success'); }); - expect(result.current.authStatus).toBe('success'); - expect(result.current.authMessage).toBe(null); + expect(result.current.qwenAuthState.authStatus).toBe('success'); + expect(result.current.qwenAuthState.authMessage).toBe(null); }); it('should clean up event listeners when auth type changes', () => { - const qwenSettings = createMockSettings(AuthType.QWEN_OAUTH); const { rerender } = renderHook( - ({ settings, isAuthenticating }) => - useQwenAuth(settings, isAuthenticating), - { initialProps: { settings: qwenSettings, isAuthenticating: true } }, + ({ pendingAuthType, isAuthenticating }) => + useQwenAuth(pendingAuthType, isAuthenticating), + { + initialProps: { + pendingAuthType: AuthType.QWEN_OAUTH, + isAuthenticating: true, + }, + }, ); // Change to non-Qwen auth - const geminiSettings = createMockSettings(AuthType.USE_GEMINI); - rerender({ settings: geminiSettings, isAuthenticating: true }); + rerender({ pendingAuthType: AuthType.USE_GEMINI, isAuthenticating: true }); expect(mockQwenOAuth2Events.off).toHaveBeenCalledWith( QwenOAuth2Event.AuthUri, @@ -276,9 +262,9 @@ describe('useQwenAuth', () => { }); it('should clean up event listeners when authentication stops', () => { - const settings = createMockSettings(AuthType.QWEN_OAUTH); const { rerender } = renderHook( - ({ isAuthenticating }) => useQwenAuth(settings, isAuthenticating), + ({ isAuthenticating }) => + useQwenAuth(AuthType.QWEN_OAUTH, isAuthenticating), { initialProps: { isAuthenticating: true } }, ); @@ -296,8 +282,9 @@ describe('useQwenAuth', () => { }); it('should clean up event listeners on unmount', () => { - const settings = createMockSettings(AuthType.QWEN_OAUTH); - const { unmount } = renderHook(() => useQwenAuth(settings, true)); + const { unmount } = renderHook(() => + useQwenAuth(AuthType.QWEN_OAUTH, true), + ); unmount(); @@ -312,8 +299,7 @@ describe('useQwenAuth', () => { }); it('should reset state when switching from Qwen auth to another auth type', () => { - const qwenSettings = createMockSettings(AuthType.QWEN_OAUTH); - let handleDeviceAuth: (deviceAuth: DeviceAuthorizationInfo) => void; + let handleDeviceAuth: (deviceAuth: DeviceAuthorizationData) => void; mockQwenOAuth2Events.on.mockImplementation((event, handler) => { if (event === QwenOAuth2Event.AuthUri) { @@ -323,9 +309,14 @@ describe('useQwenAuth', () => { }); const { result, rerender } = renderHook( - ({ settings, isAuthenticating }) => - useQwenAuth(settings, isAuthenticating), - { initialProps: { settings: qwenSettings, isAuthenticating: true } }, + ({ pendingAuthType, isAuthenticating }) => + useQwenAuth(pendingAuthType, isAuthenticating), + { + initialProps: { + pendingAuthType: AuthType.QWEN_OAUTH, + isAuthenticating: true, + }, + }, ); // Simulate device auth @@ -333,22 +324,19 @@ describe('useQwenAuth', () => { handleDeviceAuth!(mockDeviceAuth); }); - expect(result.current.deviceAuth).toEqual(mockDeviceAuth); - expect(result.current.authStatus).toBe('polling'); + expect(result.current.qwenAuthState.deviceAuth).toEqual(mockDeviceAuth); + expect(result.current.qwenAuthState.authStatus).toBe('polling'); // Switch to different auth type - const geminiSettings = createMockSettings(AuthType.USE_GEMINI); - rerender({ settings: geminiSettings, isAuthenticating: true }); + rerender({ pendingAuthType: AuthType.USE_GEMINI, isAuthenticating: true }); - expect(result.current.isQwenAuthenticating).toBe(false); - expect(result.current.deviceAuth).toBe(null); - expect(result.current.authStatus).toBe('idle'); - expect(result.current.authMessage).toBe(null); + expect(result.current.qwenAuthState.deviceAuth).toBe(null); + expect(result.current.qwenAuthState.authStatus).toBe('idle'); + expect(result.current.qwenAuthState.authMessage).toBe(null); }); it('should reset state when authentication stops', () => { - const settings = createMockSettings(AuthType.QWEN_OAUTH); - let handleDeviceAuth: (deviceAuth: DeviceAuthorizationInfo) => void; + let handleDeviceAuth: (deviceAuth: DeviceAuthorizationData) => void; mockQwenOAuth2Events.on.mockImplementation((event, handler) => { if (event === QwenOAuth2Event.AuthUri) { @@ -358,7 +346,8 @@ describe('useQwenAuth', () => { }); const { result, rerender } = renderHook( - ({ isAuthenticating }) => useQwenAuth(settings, isAuthenticating), + ({ isAuthenticating }) => + useQwenAuth(AuthType.QWEN_OAUTH, isAuthenticating), { initialProps: { isAuthenticating: true } }, ); @@ -367,21 +356,19 @@ describe('useQwenAuth', () => { handleDeviceAuth!(mockDeviceAuth); }); - expect(result.current.deviceAuth).toEqual(mockDeviceAuth); - expect(result.current.authStatus).toBe('polling'); + expect(result.current.qwenAuthState.deviceAuth).toEqual(mockDeviceAuth); + expect(result.current.qwenAuthState.authStatus).toBe('polling'); // Stop authentication rerender({ isAuthenticating: false }); - expect(result.current.isQwenAuthenticating).toBe(false); - expect(result.current.deviceAuth).toBe(null); - expect(result.current.authStatus).toBe('idle'); - expect(result.current.authMessage).toBe(null); + expect(result.current.qwenAuthState.deviceAuth).toBe(null); + expect(result.current.qwenAuthState.authStatus).toBe('idle'); + expect(result.current.qwenAuthState.authMessage).toBe(null); }); it('should handle cancelQwenAuth function', () => { - const settings = createMockSettings(AuthType.QWEN_OAUTH); - let handleDeviceAuth: (deviceAuth: DeviceAuthorizationInfo) => void; + let handleDeviceAuth: (deviceAuth: DeviceAuthorizationData) => void; mockQwenOAuth2Events.on.mockImplementation((event, handler) => { if (event === QwenOAuth2Event.AuthUri) { @@ -390,53 +377,49 @@ describe('useQwenAuth', () => { return mockQwenOAuth2Events; }); - const { result } = renderHook(() => useQwenAuth(settings, true)); + const { result } = renderHook(() => useQwenAuth(AuthType.QWEN_OAUTH, true)); // Set up some state act(() => { handleDeviceAuth!(mockDeviceAuth); }); - expect(result.current.deviceAuth).toEqual(mockDeviceAuth); + expect(result.current.qwenAuthState.deviceAuth).toEqual(mockDeviceAuth); // Cancel auth act(() => { result.current.cancelQwenAuth(); }); - expect(result.current.isQwenAuthenticating).toBe(false); - expect(result.current.deviceAuth).toBe(null); - expect(result.current.authStatus).toBe('idle'); - expect(result.current.authMessage).toBe(null); + expect(result.current.qwenAuthState.deviceAuth).toBe(null); + expect(result.current.qwenAuthState.authStatus).toBe('idle'); + expect(result.current.qwenAuthState.authMessage).toBe(null); }); - it('should maintain isQwenAuth flag correctly', () => { - // Test with Qwen OAuth - const qwenSettings = createMockSettings(AuthType.QWEN_OAUTH); + it('should handle different auth types correctly', () => { + // Test with Qwen OAuth - should set up event listeners when authenticating const { result: qwenResult } = renderHook(() => - useQwenAuth(qwenSettings, false), + useQwenAuth(AuthType.QWEN_OAUTH, true), ); - expect(qwenResult.current.isQwenAuth).toBe(true); + expect(qwenResult.current.qwenAuthState.authStatus).toBe('idle'); + expect(mockQwenOAuth2Events.on).toHaveBeenCalled(); - // Test with other auth types - const geminiSettings = createMockSettings(AuthType.USE_GEMINI); + // Test with other auth types - should not set up event listeners const { result: geminiResult } = renderHook(() => - useQwenAuth(geminiSettings, false), + useQwenAuth(AuthType.USE_GEMINI, true), ); - expect(geminiResult.current.isQwenAuth).toBe(false); + expect(geminiResult.current.qwenAuthState.authStatus).toBe('idle'); - const oauthSettings = createMockSettings(AuthType.LOGIN_WITH_GOOGLE); const { result: oauthResult } = renderHook(() => - useQwenAuth(oauthSettings, false), + useQwenAuth(AuthType.LOGIN_WITH_GOOGLE, true), ); - expect(oauthResult.current.isQwenAuth).toBe(false); + expect(oauthResult.current.qwenAuthState.authStatus).toBe('idle'); }); - it('should set isQwenAuthenticating to true when starting authentication with Qwen auth', () => { - const settings = createMockSettings(AuthType.QWEN_OAUTH); - const { result } = renderHook(() => useQwenAuth(settings, true)); + it('should initialize with idle status when starting authentication with Qwen auth', () => { + const { result } = renderHook(() => useQwenAuth(AuthType.QWEN_OAUTH, true)); - expect(result.current.isQwenAuthenticating).toBe(true); - expect(result.current.authStatus).toBe('idle'); + expect(result.current.qwenAuthState.authStatus).toBe('idle'); + expect(mockQwenOAuth2Events.on).toHaveBeenCalled(); }); }); diff --git a/packages/cli/src/ui/hooks/useQwenAuth.ts b/packages/cli/src/ui/hooks/useQwenAuth.ts index 44cd8fdf..2b1819c1 100644 --- a/packages/cli/src/ui/hooks/useQwenAuth.ts +++ b/packages/cli/src/ui/hooks/useQwenAuth.ts @@ -5,23 +5,15 @@ */ import { useState, useCallback, useEffect } from 'react'; -import type { LoadedSettings } from '../../config/settings.js'; import { AuthType, qwenOAuth2Events, QwenOAuth2Event, + type DeviceAuthorizationData, } from '@qwen-code/qwen-code-core'; -export interface DeviceAuthorizationInfo { - verification_uri: string; - verification_uri_complete: string; - user_code: string; - expires_in: number; -} - -interface QwenAuthState { - isQwenAuthenticating: boolean; - deviceAuth: DeviceAuthorizationInfo | null; +export interface QwenAuthState { + deviceAuth: DeviceAuthorizationData | null; authStatus: | 'idle' | 'polling' @@ -33,25 +25,22 @@ interface QwenAuthState { } export const useQwenAuth = ( - settings: LoadedSettings, + pendingAuthType: AuthType | undefined, isAuthenticating: boolean, ) => { const [qwenAuthState, setQwenAuthState] = useState({ - isQwenAuthenticating: false, deviceAuth: null, authStatus: 'idle', authMessage: null, }); - const isQwenAuth = - settings.merged.security?.auth?.selectedType === AuthType.QWEN_OAUTH; + const isQwenAuth = pendingAuthType === AuthType.QWEN_OAUTH; // Set up event listeners when authentication starts useEffect(() => { if (!isQwenAuth || !isAuthenticating) { // Reset state when not authenticating or not Qwen auth setQwenAuthState({ - isQwenAuthenticating: false, deviceAuth: null, authStatus: 'idle', authMessage: null, @@ -61,12 +50,11 @@ export const useQwenAuth = ( setQwenAuthState((prev) => ({ ...prev, - isQwenAuthenticating: true, authStatus: 'idle', })); // Set up event listeners - const handleDeviceAuth = (deviceAuth: DeviceAuthorizationInfo) => { + const handleDeviceAuth = (deviceAuth: DeviceAuthorizationData) => { setQwenAuthState((prev) => ({ ...prev, deviceAuth: { @@ -74,6 +62,7 @@ export const useQwenAuth = ( verification_uri_complete: deviceAuth.verification_uri_complete, user_code: deviceAuth.user_code, expires_in: deviceAuth.expires_in, + device_code: deviceAuth.device_code, }, authStatus: 'polling', })); @@ -106,7 +95,6 @@ export const useQwenAuth = ( qwenOAuth2Events.emit(QwenOAuth2Event.AuthCancel); setQwenAuthState({ - isQwenAuthenticating: false, deviceAuth: null, authStatus: 'idle', authMessage: null, @@ -114,8 +102,7 @@ export const useQwenAuth = ( }, []); return { - ...qwenAuthState, - isQwenAuth, + qwenAuthState, cancelQwenAuth, }; }; diff --git a/packages/core/index.ts b/packages/core/index.ts index c5f3ee41..3227199e 100644 --- a/packages/core/index.ts +++ b/packages/core/index.ts @@ -30,6 +30,7 @@ export { logExtensionEnable, logIdeConnection, logExtensionDisable, + logAuth, } from './src/telemetry/loggers.js'; export { @@ -40,6 +41,7 @@ export { ExtensionEnableEvent, ExtensionUninstallEvent, ModelSlashCommandEvent, + AuthEvent, } from './src/telemetry/types.js'; export { makeFakeConfig } from './src/test-utils/config.js'; export * from './src/utils/pathReader.js'; diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 93a65035..c8fa74ab 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -562,7 +562,7 @@ export class Config { } } - async refreshAuth(authMethod: AuthType) { + async refreshAuth(authMethod: AuthType, isInitialAuth?: boolean) { // Vertex and Genai have incompatible encryption and sending history with // throughtSignature from Genai to Vertex will fail, we need to strip them if ( @@ -582,6 +582,7 @@ export class Config { newContentGeneratorConfig, this, this.getSessionId(), + isInitialAuth, ); // Only assign to instance properties after successful initialization this.contentGeneratorConfig = newContentGeneratorConfig; diff --git a/packages/core/src/core/contentGenerator.ts b/packages/core/src/core/contentGenerator.ts index 4d0d33a9..2218832e 100644 --- a/packages/core/src/core/contentGenerator.ts +++ b/packages/core/src/core/contentGenerator.ts @@ -120,6 +120,7 @@ export async function createContentGenerator( config: ContentGeneratorConfig, gcConfig: Config, sessionId?: string, + isInitialAuth?: boolean, ): Promise { const version = process.env['CLI_VERSION'] || process.version; const userAgent = `QwenCode/${version} (${process.platform}; ${process.arch})`; @@ -191,13 +192,17 @@ export async function createContentGenerator( try { // Get the Qwen OAuth client (now includes integrated token management) - const qwenClient = await getQwenOauthClient(gcConfig); + // If this is initial auth, require cached credentials to detect missing credentials + const qwenClient = await getQwenOauthClient( + gcConfig, + isInitialAuth ? { requireCachedCredentials: true } : undefined, + ); // Create the content generator with dynamic token management return new QwenContentGenerator(qwenClient, config, gcConfig); } catch (error) { throw new Error( - `Failed to initialize Qwen: ${error instanceof Error ? error.message : String(error)}`, + `${error instanceof Error ? error.message : String(error)}`, ); } } diff --git a/packages/core/src/qwen/qwenOAuth2.test.ts b/packages/core/src/qwen/qwenOAuth2.test.ts index 3369f22c..2e8bf83e 100644 --- a/packages/core/src/qwen/qwenOAuth2.test.ts +++ b/packages/core/src/qwen/qwenOAuth2.test.ts @@ -825,7 +825,7 @@ describe('getQwenOAuthClient', () => { import('./qwenOAuth2.js').then((module) => module.getQwenOAuthClient(mockConfig), ), - ).rejects.toThrow('Qwen OAuth authentication failed'); + ).rejects.toThrow('Device authorization flow failed'); SharedTokenManager.getInstance = originalGetInstance; }); @@ -983,7 +983,7 @@ describe('getQwenOAuthClient - Enhanced Error Scenarios', () => { import('./qwenOAuth2.js').then((module) => module.getQwenOAuthClient(mockConfig), ), - ).rejects.toThrow('Qwen OAuth authentication failed'); + ).rejects.toThrow('Device authorization flow failed'); SharedTokenManager.getInstance = originalGetInstance; }); @@ -1032,7 +1032,7 @@ describe('getQwenOAuthClient - Enhanced Error Scenarios', () => { import('./qwenOAuth2.js').then((module) => module.getQwenOAuthClient(mockConfig), ), - ).rejects.toThrow('Qwen OAuth authentication timed out'); + ).rejects.toThrow('Authorization timeout, please restart the process.'); SharedTokenManager.getInstance = originalGetInstance; }); @@ -1082,7 +1082,7 @@ describe('getQwenOAuthClient - Enhanced Error Scenarios', () => { module.getQwenOAuthClient(mockConfig), ), ).rejects.toThrow( - 'Too many request for Qwen OAuth authentication, please try again later.', + 'Too many requests. The server is rate limiting our requests. Please select a different authentication method or try again later.', ); SharedTokenManager.getInstance = originalGetInstance; @@ -1119,7 +1119,7 @@ describe('getQwenOAuthClient - Enhanced Error Scenarios', () => { import('./qwenOAuth2.js').then((module) => module.getQwenOAuthClient(mockConfig), ), - ).rejects.toThrow('Qwen OAuth authentication failed'); + ).rejects.toThrow('Device authorization flow failed'); SharedTokenManager.getInstance = originalGetInstance; }); @@ -1177,7 +1177,7 @@ describe('authWithQwenDeviceFlow - Comprehensive Testing', () => { import('./qwenOAuth2.js').then((module) => module.getQwenOAuthClient(mockConfig), ), - ).rejects.toThrow('Qwen OAuth authentication failed'); + ).rejects.toThrow('Device authorization flow failed'); SharedTokenManager.getInstance = originalGetInstance; }); @@ -1264,7 +1264,9 @@ describe('authWithQwenDeviceFlow - Comprehensive Testing', () => { import('./qwenOAuth2.js').then((module) => module.getQwenOAuthClient(mockConfig), ), - ).rejects.toThrow('Qwen OAuth authentication failed'); + ).rejects.toThrow( + 'Device code expired or invalid, please restart the authorization process.', + ); SharedTokenManager.getInstance = originalGetInstance; }); diff --git a/packages/core/src/qwen/qwenOAuth2.ts b/packages/core/src/qwen/qwenOAuth2.ts index 7a4c7beb..b9a35bff 100644 --- a/packages/core/src/qwen/qwenOAuth2.ts +++ b/packages/core/src/qwen/qwenOAuth2.ts @@ -467,6 +467,7 @@ export type AuthResult = | { success: false; reason: 'timeout' | 'cancelled' | 'error' | 'rate_limit'; + message?: string; // Detailed error message for better error reporting }; /** @@ -476,6 +477,7 @@ export const qwenOAuth2Events = new EventEmitter(); export async function getQwenOAuthClient( config: Config, + options?: { requireCachedCredentials?: boolean }, ): Promise { const client = new QwenOAuth2Client(); @@ -488,11 +490,6 @@ export async function getQwenOAuthClient( client.setCredentials(credentials); return client; } catch (error: unknown) { - console.debug( - 'Shared token manager failed, attempting device flow:', - error, - ); - // Handle specific token manager errors if (error instanceof TokenManagerError) { switch (error.type) { @@ -520,12 +517,20 @@ export async function getQwenOAuthClient( // Try device flow instead of forcing refresh const result = await authWithQwenDeviceFlow(client, config); if (!result.success) { - throw new Error('Qwen OAuth authentication failed'); + // Use detailed error message if available, otherwise use default + const errorMessage = + result.message || 'Qwen OAuth authentication failed'; + throw new Error(errorMessage); } return client; } - // No cached credentials, use device authorization flow for authentication + if (options?.requireCachedCredentials) { + throw new Error( + 'No cached Qwen-OAuth credentials found. Please re-authenticate.', + ); + } + const result = await authWithQwenDeviceFlow(client, config); if (!result.success) { // Only emit timeout event if the failure reason is actually timeout @@ -538,20 +543,24 @@ export async function getQwenOAuthClient( ); } - // Throw error with appropriate message based on failure reason - switch (result.reason) { - case 'timeout': - throw new Error('Qwen OAuth authentication timed out'); - case 'cancelled': - throw new Error('Qwen OAuth authentication was cancelled by user'); - case 'rate_limit': - throw new Error( - 'Too many request for Qwen OAuth authentication, please try again later.', - ); - case 'error': - default: - throw new Error('Qwen OAuth authentication failed'); - } + // Use detailed error message if available, otherwise use default based on reason + const errorMessage = + result.message || + (() => { + switch (result.reason) { + case 'timeout': + return 'Qwen OAuth authentication timed out'; + case 'cancelled': + return 'Qwen OAuth authentication was cancelled by user'; + case 'rate_limit': + return 'Too many request for Qwen OAuth authentication, please try again later.'; + case 'error': + default: + return 'Qwen OAuth authentication failed'; + } + })(); + + throw new Error(errorMessage); } return client; @@ -644,13 +653,10 @@ async function authWithQwenDeviceFlow( for (let attempt = 0; attempt < maxAttempts; attempt++) { // Check if authentication was cancelled if (isCancelled) { - console.debug('\nAuthentication cancelled by user.'); - qwenOAuth2Events.emit( - QwenOAuth2Event.AuthProgress, - 'error', - 'Authentication cancelled by user.', - ); - return { success: false, reason: 'cancelled' }; + const message = 'Authentication cancelled by user.'; + console.debug('\n' + message); + qwenOAuth2Events.emit(QwenOAuth2Event.AuthProgress, 'error', message); + return { success: false, reason: 'cancelled', message }; } try { @@ -738,13 +744,14 @@ async function authWithQwenDeviceFlow( // Check for cancellation after waiting if (isCancelled) { - console.debug('\nAuthentication cancelled by user.'); + const message = 'Authentication cancelled by user.'; + console.debug('\n' + message); qwenOAuth2Events.emit( QwenOAuth2Event.AuthProgress, 'error', - 'Authentication cancelled by user.', + message, ); - return { success: false, reason: 'cancelled' }; + return { success: false, reason: 'cancelled', message }; } continue; @@ -758,7 +765,7 @@ async function authWithQwenDeviceFlow( ); } } catch (error: unknown) { - // Handle specific error cases + // Extract error information const errorMessage = error instanceof Error ? error.message : String(error); const statusCode = @@ -766,42 +773,49 @@ async function authWithQwenDeviceFlow( ? (error as Error & { status?: number }).status : null; - if (errorMessage.includes('401') || statusCode === 401) { - const message = - 'Device code expired or invalid, please restart the authorization process.'; - - // Emit error event - qwenOAuth2Events.emit(QwenOAuth2Event.AuthProgress, 'error', message); - - return { success: false, reason: 'error' }; - } - - // Handle 429 Too Many Requests error - if (errorMessage.includes('429') || statusCode === 429) { - const message = - 'Too many requests. The server is rate limiting our requests. Please select a different authentication method or try again later.'; - - // Emit rate limit event to notify user + // Helper function to handle error and stop polling + const handleError = ( + reason: 'error' | 'rate_limit', + message: string, + eventType: 'error' | 'rate_limit' = 'error', + ): AuthResult => { qwenOAuth2Events.emit( QwenOAuth2Event.AuthProgress, - 'rate_limit', + eventType, message, ); + console.error('\n' + message); + return { success: false, reason, message }; + }; - console.log('\n' + message); + // Handle credential caching failures - stop polling immediately + if (errorMessage.includes('Failed to cache credentials')) { + return handleError('error', errorMessage); + } - // Return false to stop polling and go back to auth selection - return { success: false, reason: 'rate_limit' }; + // Handle 401 Unauthorized - device code expired or invalid + if (errorMessage.includes('401') || statusCode === 401) { + return handleError( + 'error', + 'Device code expired or invalid, please restart the authorization process.', + ); + } + + // Handle 429 Too Many Requests - rate limiting + if (errorMessage.includes('429') || statusCode === 429) { + return handleError( + 'rate_limit', + 'Too many requests. The server is rate limiting our requests. Please select a different authentication method or try again later.', + 'rate_limit', + ); } const message = `Error polling for token: ${errorMessage}`; - - // Emit error event qwenOAuth2Events.emit(QwenOAuth2Event.AuthProgress, 'error', message); - // Check for cancellation before waiting if (isCancelled) { - return { success: false, reason: 'cancelled' }; + const message = 'Authentication cancelled by user.'; + return { success: false, reason: 'cancelled', message }; } await new Promise((resolve) => setTimeout(resolve, pollInterval)); @@ -818,11 +832,12 @@ async function authWithQwenDeviceFlow( ); console.error('\n' + timeoutMessage); - return { success: false, reason: 'timeout' }; + return { success: false, reason: 'timeout', message: timeoutMessage }; } catch (error: unknown) { const errorMessage = error instanceof Error ? error.message : String(error); - console.error('Device authorization flow failed:', errorMessage); - return { success: false, reason: 'error' }; + const message = `Device authorization flow failed: ${errorMessage}`; + console.error(message); + return { success: false, reason: 'error', message }; } finally { // Clean up event listener qwenOAuth2Events.off(QwenOAuth2Event.AuthCancel, cancelHandler); @@ -852,10 +867,30 @@ async function loadCachedQwenCredentials( async function cacheQwenCredentials(credentials: QwenCredentials) { const filePath = getQwenCachedCredentialPath(); - await fs.mkdir(path.dirname(filePath), { recursive: true }); + try { + await fs.mkdir(path.dirname(filePath), { recursive: true }); - const credString = JSON.stringify(credentials, null, 2); - await fs.writeFile(filePath, credString); + const credString = JSON.stringify(credentials, null, 2); + await fs.writeFile(filePath, credString); + } catch (error: unknown) { + // Handle file system errors (e.g., EACCES permission denied) + const errorMessage = error instanceof Error ? error.message : String(error); + const errorCode = + error instanceof Error && 'code' in error + ? (error as Error & { code?: string }).code + : undefined; + + if (errorCode === 'EACCES') { + throw new Error( + `Failed to cache credentials: Permission denied (EACCES). Current user has no permission to access \`${filePath}\`. Please check permissions.`, + ); + } + + // Throw error for other file system failures + throw new Error( + `Failed to cache credentials: error when creating folder \`${path.dirname(filePath)}\` and writing to \`${filePath}\`. ${errorMessage}. Please check permissions.`, + ); + } } /** diff --git a/packages/core/src/telemetry/constants.ts b/packages/core/src/telemetry/constants.ts index fc8affed..bc654637 100644 --- a/packages/core/src/telemetry/constants.ts +++ b/packages/core/src/telemetry/constants.ts @@ -33,6 +33,7 @@ export const EVENT_MALFORMED_JSON_RESPONSE = export const EVENT_FILE_OPERATION = 'qwen-code.file_operation'; export const EVENT_MODEL_SLASH_COMMAND = 'qwen-code.slash_command.model'; export const EVENT_SUBAGENT_EXECUTION = 'qwen-code.subagent_execution'; +export const EVENT_AUTH = 'qwen-code.auth'; // Performance Events export const EVENT_STARTUP_PERFORMANCE = 'qwen-code.startup.performance'; diff --git a/packages/core/src/telemetry/index.ts b/packages/core/src/telemetry/index.ts index 6d2b6d9e..16c230ba 100644 --- a/packages/core/src/telemetry/index.ts +++ b/packages/core/src/telemetry/index.ts @@ -43,6 +43,7 @@ export { logExtensionUninstall, logRipgrepFallback, logNextSpeakerCheck, + logAuth, } from './loggers.js'; export type { SlashCommandEvent, ChatCompressionEvent } from './types.js'; export { @@ -61,6 +62,7 @@ export { ToolOutputTruncatedEvent, RipgrepFallbackEvent, NextSpeakerCheckEvent, + AuthEvent, } from './types.js'; export { makeSlashCommandEvent, makeChatCompressionEvent } from './types.js'; export type { TelemetryEvent } from './types.js'; diff --git a/packages/core/src/telemetry/loggers.ts b/packages/core/src/telemetry/loggers.ts index fdb8810e..5b56719b 100644 --- a/packages/core/src/telemetry/loggers.ts +++ b/packages/core/src/telemetry/loggers.ts @@ -37,6 +37,7 @@ import { EVENT_SUBAGENT_EXECUTION, EVENT_MALFORMED_JSON_RESPONSE, EVENT_INVALID_CHUNK, + EVENT_AUTH, } from './constants.js'; import { recordApiErrorMetrics, @@ -83,6 +84,7 @@ import type { SubagentExecutionEvent, MalformedJsonResponseEvent, InvalidChunkEvent, + AuthEvent, } from './types.js'; import type { UiEvent } from './uiTelemetry.js'; import { uiTelemetryService } from './uiTelemetry.js'; @@ -838,3 +840,29 @@ export function logExtensionDisable( }; logger.emit(logRecord); } + +export function logAuth(config: Config, event: AuthEvent): void { + QwenLogger.getInstance(config)?.logAuthEvent(event); + if (!isTelemetrySdkInitialized()) return; + + const attributes: LogAttributes = { + ...getCommonAttributes(config), + ...event, + 'event.name': EVENT_AUTH, + 'event.timestamp': new Date().toISOString(), + auth_type: event.auth_type, + action_type: event.action_type, + status: event.status, + }; + + if (event.error_message) { + attributes['error.message'] = event.error_message; + } + + const logger = logs.getLogger(SERVICE_NAME); + const logRecord: LogRecord = { + body: `Auth event: ${event.action_type} ${event.status} for ${event.auth_type}`, + attributes, + }; + logger.emit(logRecord); +} diff --git a/packages/core/src/telemetry/qwen-logger/qwen-logger.ts b/packages/core/src/telemetry/qwen-logger/qwen-logger.ts index 5f9a2a51..c5dc70d7 100644 --- a/packages/core/src/telemetry/qwen-logger/qwen-logger.ts +++ b/packages/core/src/telemetry/qwen-logger/qwen-logger.ts @@ -37,6 +37,7 @@ import type { ExtensionEnableEvent, ModelSlashCommandEvent, ExtensionDisableEvent, + AuthEvent, } from '../types.js'; import { EndSessionEvent } from '../types.js'; import type { @@ -746,6 +747,25 @@ export class QwenLogger { this.flushIfNeeded(); } + logAuthEvent(event: AuthEvent): void { + const snapshots: Record = { + auth_type: event.auth_type, + action_type: event.action_type, + status: event.status, + }; + + if (event.error_message) { + snapshots['error_message'] = event.error_message; + } + + const rumEvent = this.createActionEvent('auth', 'auth', { + snapshots: JSON.stringify(snapshots), + }); + + this.enqueueLogEvent(rumEvent); + this.flushIfNeeded(); + } + // misc events logFlashFallbackEvent(event: FlashFallbackEvent): void { const rumEvent = this.createActionEvent('misc', 'flash_fallback', { diff --git a/packages/core/src/telemetry/types.ts b/packages/core/src/telemetry/types.ts index 1ba29116..cef83323 100644 --- a/packages/core/src/telemetry/types.ts +++ b/packages/core/src/telemetry/types.ts @@ -686,6 +686,29 @@ export class SubagentExecutionEvent implements BaseTelemetryEvent { } } +export class AuthEvent implements BaseTelemetryEvent { + 'event.name': 'auth'; + 'event.timestamp': string; + auth_type: AuthType; + action_type: 'auto' | 'manual'; + status: 'success' | 'error' | 'cancelled'; + error_message?: string; + + constructor( + auth_type: AuthType, + action_type: 'auto' | 'manual', + status: 'success' | 'error' | 'cancelled', + error_message?: string, + ) { + this['event.name'] = 'auth'; + this['event.timestamp'] = new Date().toISOString(); + this.auth_type = auth_type; + this.action_type = action_type; + this.status = status; + this.error_message = error_message; + } +} + export type TelemetryEvent = | StartSessionEvent | EndSessionEvent @@ -713,7 +736,8 @@ export type TelemetryEvent = | ExtensionInstallEvent | ExtensionUninstallEvent | ToolOutputTruncatedEvent - | ModelSlashCommandEvent; + | ModelSlashCommandEvent + | AuthEvent; export class ExtensionDisableEvent implements BaseTelemetryEvent { 'event.name': 'extension_disable'; From 97bf48b14c3c96f3eacc3f9cf48d41baf981cba8 Mon Sep 17 00:00:00 2001 From: tanzhenxin Date: Wed, 19 Nov 2025 11:55:19 +0800 Subject: [PATCH 08/14] fix: skip problematic integration test (#1065) --- .../context-compress-interactive.test.ts | 64 +++++++++---------- 1 file changed, 30 insertions(+), 34 deletions(-) diff --git a/integration-tests/context-compress-interactive.test.ts b/integration-tests/context-compress-interactive.test.ts index 845d9db0..378575f4 100644 --- a/integration-tests/context-compress-interactive.test.ts +++ b/integration-tests/context-compress-interactive.test.ts @@ -66,47 +66,43 @@ describe('Interactive Mode', () => { }, ); - it.skipIf(process.platform === 'win32')( - 'should handle compression failure on token inflation', - async () => { - await rig.setup('interactive-compress-test', { - settings: { - security: { - auth: { - selectedType: 'openai', - }, + it.skip('should handle compression failure on token inflation', async () => { + await rig.setup('interactive-compress-test', { + settings: { + security: { + auth: { + selectedType: 'openai', }, }, - }); + }, + }); - const { ptyProcess } = rig.runInteractive(); + const { ptyProcess } = rig.runInteractive(); - let fullOutput = ''; - ptyProcess.onData((data) => (fullOutput += data)); + let fullOutput = ''; + ptyProcess.onData((data) => (fullOutput += data)); - // Wait for the app to be ready - const isReady = await rig.waitForText('Type your message', 25000); - expect( - isReady, - 'CLI did not start up in interactive mode correctly', - ).toBe(true); + // Wait for the app to be ready + const isReady = await rig.waitForText('Type your message', 25000); + expect(isReady, 'CLI did not start up in interactive mode correctly').toBe( + true, + ); - await type(ptyProcess, '/compress'); - await new Promise((resolve) => setTimeout(resolve, 1000)); - await type(ptyProcess, '\r'); + await type(ptyProcess, '/compress'); + await new Promise((resolve) => setTimeout(resolve, 1000)); + await type(ptyProcess, '\r'); - const foundEvent = await rig.waitForTelemetryEvent( - 'chat_compression', - 90000, - ); - expect(foundEvent).toBe(true); + const foundEvent = await rig.waitForTelemetryEvent( + 'chat_compression', + 90000, + ); + expect(foundEvent).toBe(true); - const compressionFailed = await rig.waitForText( - 'Nothing to compress.', - 25000, - ); + const compressionFailed = await rig.waitForText( + 'Nothing to compress.', + 25000, + ); - expect(compressionFailed).toBe(true); - }, - ); + expect(compressionFailed).toBe(true); + }); }); From 3c64f7bff5c1e51c8369606cf2135f3beda96d8a Mon Sep 17 00:00:00 2001 From: tanzhenxin Date: Thu, 20 Nov 2025 10:09:12 +0800 Subject: [PATCH 09/14] chore: pump version to 0.2.3 (#1073) --- package-lock.json | 12 ++++++------ package.json | 4 ++-- packages/cli/package.json | 4 ++-- packages/core/package.json | 2 +- packages/test-utils/package.json | 2 +- packages/vscode-ide-companion/package.json | 2 +- 6 files changed, 13 insertions(+), 13 deletions(-) diff --git a/package-lock.json b/package-lock.json index 296fc29b..c7ac21f6 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@qwen-code/qwen-code", - "version": "0.2.2", + "version": "0.2.3", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@qwen-code/qwen-code", - "version": "0.2.2", + "version": "0.2.3", "workspaces": [ "packages/*" ], @@ -16024,7 +16024,7 @@ }, "packages/cli": { "name": "@qwen-code/qwen-code", - "version": "0.2.2", + "version": "0.2.3", "dependencies": { "@google/genai": "1.16.0", "@iarna/toml": "^2.2.5", @@ -16139,7 +16139,7 @@ }, "packages/core": { "name": "@qwen-code/qwen-code-core", - "version": "0.2.2", + "version": "0.2.3", "hasInstallScript": true, "dependencies": { "@google/genai": "1.16.0", @@ -16278,7 +16278,7 @@ }, "packages/test-utils": { "name": "@qwen-code/qwen-code-test-utils", - "version": "0.2.2", + "version": "0.2.3", "dev": true, "license": "Apache-2.0", "devDependencies": { @@ -16290,7 +16290,7 @@ }, "packages/vscode-ide-companion": { "name": "qwen-code-vscode-ide-companion", - "version": "0.2.2", + "version": "0.2.3", "license": "LICENSE", "dependencies": { "@modelcontextprotocol/sdk": "^1.15.1", diff --git a/package.json b/package.json index a8b69061..85c90f84 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@qwen-code/qwen-code", - "version": "0.2.2", + "version": "0.2.3", "engines": { "node": ">=20.0.0" }, @@ -13,7 +13,7 @@ "url": "git+https://github.com/QwenLM/qwen-code.git" }, "config": { - "sandboxImageUri": "ghcr.io/qwenlm/qwen-code:0.2.2" + "sandboxImageUri": "ghcr.io/qwenlm/qwen-code:0.2.3" }, "scripts": { "start": "cross-env node scripts/start.js", diff --git a/packages/cli/package.json b/packages/cli/package.json index 6dce0ccc..bece2f31 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -1,6 +1,6 @@ { "name": "@qwen-code/qwen-code", - "version": "0.2.2", + "version": "0.2.3", "description": "Qwen Code", "repository": { "type": "git", @@ -25,7 +25,7 @@ "dist" ], "config": { - "sandboxImageUri": "ghcr.io/qwenlm/qwen-code:0.2.2" + "sandboxImageUri": "ghcr.io/qwenlm/qwen-code:0.2.3" }, "dependencies": { "@google/genai": "1.16.0", diff --git a/packages/core/package.json b/packages/core/package.json index 3232a664..72e4612f 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -1,6 +1,6 @@ { "name": "@qwen-code/qwen-code-core", - "version": "0.2.2", + "version": "0.2.3", "description": "Qwen Code Core", "repository": { "type": "git", diff --git a/packages/test-utils/package.json b/packages/test-utils/package.json index 512ada66..0e23606c 100644 --- a/packages/test-utils/package.json +++ b/packages/test-utils/package.json @@ -1,6 +1,6 @@ { "name": "@qwen-code/qwen-code-test-utils", - "version": "0.2.2", + "version": "0.2.3", "private": true, "main": "src/index.ts", "license": "Apache-2.0", diff --git a/packages/vscode-ide-companion/package.json b/packages/vscode-ide-companion/package.json index dd86c816..afeed670 100644 --- a/packages/vscode-ide-companion/package.json +++ b/packages/vscode-ide-companion/package.json @@ -2,7 +2,7 @@ "name": "qwen-code-vscode-ide-companion", "displayName": "Qwen Code Companion", "description": "Enable Qwen Code with direct access to your VS Code workspace.", - "version": "0.2.2", + "version": "0.2.3", "publisher": "qwenlm", "icon": "assets/icon.png", "repository": { From e1f793b2e0f8263bcbb1a3f2dab492e0e2556787 Mon Sep 17 00:00:00 2001 From: citlalinda Date: Thu, 20 Nov 2025 10:23:17 +0800 Subject: [PATCH 10/14] fix: character encoding corruption when executing the /copy command on Windows. (#1069) Co-authored-by: linda --- .../cli/src/ui/utils/commandUtils.test.ts | 6 +++++- packages/cli/src/ui/utils/commandUtils.ts | 19 ++++++++++++++++++- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/ui/utils/commandUtils.test.ts b/packages/cli/src/ui/utils/commandUtils.test.ts index b48bb4c9..9d2fddd9 100644 --- a/packages/cli/src/ui/utils/commandUtils.test.ts +++ b/packages/cli/src/ui/utils/commandUtils.test.ts @@ -13,6 +13,7 @@ import { isSlashCommand, copyToClipboard, getUrlOpenCommand, + CodePage, } from './commandUtils.js'; // Mock child_process @@ -188,7 +189,10 @@ describe('commandUtils', () => { await copyToClipboard(testText); - expect(mockSpawn).toHaveBeenCalledWith('clip', []); + expect(mockSpawn).toHaveBeenCalledWith('cmd', [ + '/c', + `chcp ${CodePage.UTF8} >nul && clip`, + ]); expect(mockChild.stdin.write).toHaveBeenCalledWith(testText); expect(mockChild.stdin.end).toHaveBeenCalled(); }); diff --git a/packages/cli/src/ui/utils/commandUtils.ts b/packages/cli/src/ui/utils/commandUtils.ts index 32bebceb..89d1045a 100644 --- a/packages/cli/src/ui/utils/commandUtils.ts +++ b/packages/cli/src/ui/utils/commandUtils.ts @@ -7,6 +7,23 @@ import type { SpawnOptions } from 'node:child_process'; import { spawn } from 'node:child_process'; +/** + * Common Windows console code pages (CP) used for encoding conversions. + * + * @remarks + * - `UTF8` (65001): Unicode (UTF-8) — recommended for cross-language scripts. + * - `GBK` (936): Simplified Chinese — default on most Chinese Windows systems. + * - `BIG5` (950): Traditional Chinese. + * - `LATIN1` (1252): Western European — default on many Western systems. + */ +export const CodePage = { + UTF8: 65001, + GBK: 936, + BIG5: 950, + LATIN1: 1252, +} as const; + +export type CodePage = (typeof CodePage)[keyof typeof CodePage]; /** * Checks if a query string potentially represents an '@' command. * It triggers if the query starts with '@' or contains '@' preceded by whitespace @@ -80,7 +97,7 @@ export const copyToClipboard = async (text: string): Promise => { switch (process.platform) { case 'win32': - return run('clip', []); + return run('cmd', ['/c', `chcp ${CodePage.UTF8} >nul && clip`]); case 'darwin': return run('pbcopy', []); case 'linux': From fc638851e755d3155985594615e5dfc5fccfe83f Mon Sep 17 00:00:00 2001 From: cwtuan Date: Thu, 20 Nov 2025 12:50:06 +0800 Subject: [PATCH 11/14] fix: remove broken link --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 4c4396ec..c6230b96 100644 --- a/README.md +++ b/README.md @@ -25,7 +25,7 @@ -Qwen Code is a powerful command-line AI workflow tool adapted from [**Gemini CLI**](https://github.com/google-gemini/gemini-cli) ([details](./README.gemini.md)), specifically optimized for [Qwen3-Coder](https://github.com/QwenLM/Qwen3-Coder) models. It enhances your development workflow with advanced code understanding, automated tasks, and intelligent assistance. +Qwen Code is a powerful command-line AI workflow tool adapted from [**Gemini CLI**](https://github.com/google-gemini/gemini-cli), specifically optimized for [Qwen3-Coder](https://github.com/QwenLM/Qwen3-Coder) models. It enhances your development workflow with advanced code understanding, automated tasks, and intelligent assistance. ## 💡 Free Options Available From 07069f00d1360a40191507552dddd62fdd32d822 Mon Sep 17 00:00:00 2001 From: tanzhenxin Date: Thu, 20 Nov 2025 14:36:51 +0800 Subject: [PATCH 12/14] feat: remove prompt completion feature (#1076) --- packages/cli/src/config/config.ts | 1 - packages/cli/src/config/settings.ts | 1 - packages/cli/src/config/settingsSchema.ts | 10 - .../src/ui/components/InputPrompt.test.tsx | 5 - .../cli/src/ui/components/InputPrompt.tsx | 328 ++++-------------- .../src/ui/components/SettingsDialog.test.tsx | 2 - .../SettingsDialog.test.tsx.snap | 40 +-- .../src/ui/hooks/useCommandCompletion.test.ts | 81 +---- .../cli/src/ui/hooks/useCommandCompletion.tsx | 35 +- .../cli/src/ui/hooks/usePromptCompletion.ts | 254 -------------- packages/core/src/config/config.ts | 7 - 11 files changed, 99 insertions(+), 665 deletions(-) delete mode 100644 packages/cli/src/ui/hooks/usePromptCompletion.ts diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index 50a11991..7286ff12 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -789,7 +789,6 @@ export async function loadCliConfig( useBuiltinRipgrep: settings.tools?.useBuiltinRipgrep, shouldUseNodePtyShell: settings.tools?.shell?.enableInteractiveShell, skipNextSpeakerCheck: settings.model?.skipNextSpeakerCheck, - enablePromptCompletion: settings.general?.enablePromptCompletion ?? false, skipLoopDetection: settings.model?.skipLoopDetection ?? false, skipStartupContext: settings.model?.skipStartupContext ?? false, vlmSwitchMode, diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index aefcb103..8ff022c8 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -77,7 +77,6 @@ const MIGRATION_MAP: Record = { disableAutoUpdate: 'general.disableAutoUpdate', disableUpdateNag: 'general.disableUpdateNag', dnsResolutionOrder: 'advanced.dnsResolutionOrder', - enablePromptCompletion: 'general.enablePromptCompletion', enforcedAuthType: 'security.auth.enforcedType', excludeTools: 'tools.exclude', excludeMCPServers: 'mcp.excluded', diff --git a/packages/cli/src/config/settingsSchema.ts b/packages/cli/src/config/settingsSchema.ts index 70037dfd..e0ece3ac 100644 --- a/packages/cli/src/config/settingsSchema.ts +++ b/packages/cli/src/config/settingsSchema.ts @@ -167,16 +167,6 @@ const SETTINGS_SCHEMA = { }, }, }, - enablePromptCompletion: { - type: 'boolean', - label: 'Enable Prompt Completion', - category: 'General', - requiresRestart: true, - default: false, - description: - 'Enable AI-powered prompt completion suggestions while typing.', - showInDialog: true, - }, debugKeystrokeLogging: { type: 'boolean', label: 'Debug Keystroke Logging', diff --git a/packages/cli/src/ui/components/InputPrompt.test.tsx b/packages/cli/src/ui/components/InputPrompt.test.tsx index 38d5f7b1..25274a12 100644 --- a/packages/cli/src/ui/components/InputPrompt.test.tsx +++ b/packages/cli/src/ui/components/InputPrompt.test.tsx @@ -164,11 +164,6 @@ describe('InputPrompt', () => { setActiveSuggestionIndex: vi.fn(), setShowSuggestions: vi.fn(), handleAutocomplete: vi.fn(), - promptCompletion: { - text: '', - accept: vi.fn(), - clear: vi.fn(), - }, }; mockedUseCommandCompletion.mockReturnValue(mockCommandCompletion); diff --git a/packages/cli/src/ui/components/InputPrompt.tsx b/packages/cli/src/ui/components/InputPrompt.tsx index f33700d8..2bd9b275 100644 --- a/packages/cli/src/ui/components/InputPrompt.tsx +++ b/packages/cli/src/ui/components/InputPrompt.tsx @@ -12,9 +12,8 @@ import { theme } from '../semantic-colors.js'; import { useInputHistory } from '../hooks/useInputHistory.js'; import type { TextBuffer } from './shared/text-buffer.js'; import { logicalPosToOffset } from './shared/text-buffer.js'; -import { cpSlice, cpLen, toCodePoints } from '../utils/textUtils.js'; +import { cpSlice, cpLen } from '../utils/textUtils.js'; import chalk from 'chalk'; -import stringWidth from 'string-width'; import { useShellHistory } from '../hooks/useShellHistory.js'; import { useReverseSearchCompletion } from '../hooks/useReverseSearchCompletion.js'; import { useCommandCompletion } from '../hooks/useCommandCompletion.js'; @@ -91,7 +90,6 @@ export const InputPrompt: React.FC = ({ commandContext, placeholder = ' Type your message or @path/to/file', focus = true, - inputWidth, suggestionsWidth, shellModeActive, setShellModeActive, @@ -526,16 +524,6 @@ export const InputPrompt: React.FC = ({ } } - // Handle Tab key for ghost text acceptance - if ( - key.name === 'tab' && - !completion.showSuggestions && - completion.promptCompletion.text - ) { - completion.promptCompletion.accept(); - return; - } - if (!shellModeActive) { if (keyMatchers[Command.REVERSE_SEARCH](key)) { setCommandSearchActive(true); @@ -657,18 +645,6 @@ export const InputPrompt: React.FC = ({ // Fall back to the text buffer's default input handling for all other keys buffer.handleInput(key); - - // Clear ghost text when user types regular characters (not navigation/control keys) - if ( - completion.promptCompletion.text && - key.sequence && - key.sequence.length === 1 && - !key.ctrl && - !key.meta - ) { - completion.promptCompletion.clear(); - setExpandedSuggestionIndex(-1); - } }, [ focus, @@ -703,118 +679,6 @@ export const InputPrompt: React.FC = ({ buffer.visualCursor; const scrollVisualRow = buffer.visualScrollRow; - const getGhostTextLines = useCallback(() => { - if ( - !completion.promptCompletion.text || - !buffer.text || - !completion.promptCompletion.text.startsWith(buffer.text) - ) { - return { inlineGhost: '', additionalLines: [] }; - } - - const ghostSuffix = completion.promptCompletion.text.slice( - buffer.text.length, - ); - if (!ghostSuffix) { - return { inlineGhost: '', additionalLines: [] }; - } - - const currentLogicalLine = buffer.lines[buffer.cursor[0]] || ''; - const cursorCol = buffer.cursor[1]; - - const textBeforeCursor = cpSlice(currentLogicalLine, 0, cursorCol); - const usedWidth = stringWidth(textBeforeCursor); - const remainingWidth = Math.max(0, inputWidth - usedWidth); - - const ghostTextLinesRaw = ghostSuffix.split('\n'); - const firstLineRaw = ghostTextLinesRaw.shift() || ''; - - let inlineGhost = ''; - let remainingFirstLine = ''; - - if (stringWidth(firstLineRaw) <= remainingWidth) { - inlineGhost = firstLineRaw; - } else { - const words = firstLineRaw.split(' '); - let currentLine = ''; - let wordIdx = 0; - for (const word of words) { - const prospectiveLine = currentLine ? `${currentLine} ${word}` : word; - if (stringWidth(prospectiveLine) > remainingWidth) { - break; - } - currentLine = prospectiveLine; - wordIdx++; - } - inlineGhost = currentLine; - if (words.length > wordIdx) { - remainingFirstLine = words.slice(wordIdx).join(' '); - } - } - - const linesToWrap = []; - if (remainingFirstLine) { - linesToWrap.push(remainingFirstLine); - } - linesToWrap.push(...ghostTextLinesRaw); - const remainingGhostText = linesToWrap.join('\n'); - - const additionalLines: string[] = []; - if (remainingGhostText) { - const textLines = remainingGhostText.split('\n'); - for (const textLine of textLines) { - const words = textLine.split(' '); - let currentLine = ''; - - for (const word of words) { - const prospectiveLine = currentLine ? `${currentLine} ${word}` : word; - const prospectiveWidth = stringWidth(prospectiveLine); - - if (prospectiveWidth > inputWidth) { - if (currentLine) { - additionalLines.push(currentLine); - } - - let wordToProcess = word; - while (stringWidth(wordToProcess) > inputWidth) { - let part = ''; - const wordCP = toCodePoints(wordToProcess); - let partWidth = 0; - let splitIndex = 0; - for (let i = 0; i < wordCP.length; i++) { - const char = wordCP[i]; - const charWidth = stringWidth(char); - if (partWidth + charWidth > inputWidth) { - break; - } - part += char; - partWidth += charWidth; - splitIndex = i + 1; - } - additionalLines.push(part); - wordToProcess = cpSlice(wordToProcess, splitIndex); - } - currentLine = wordToProcess; - } else { - currentLine = prospectiveLine; - } - } - if (currentLine) { - additionalLines.push(currentLine); - } - } - } - - return { inlineGhost, additionalLines }; - }, [ - completion.promptCompletion.text, - buffer.text, - buffer.lines, - buffer.cursor, - inputWidth, - ]); - - const { inlineGhost, additionalLines } = getGhostTextLines(); const getActiveCompletion = () => { if (commandSearchActive) return commandSearchCompletion; if (reverseSearchActive) return reverseSearchCompletion; @@ -887,134 +751,96 @@ export const InputPrompt: React.FC = ({ {placeholder} ) ) : ( - linesToRender - .map((lineText, visualIdxInRenderedSet) => { - const absoluteVisualIdx = - scrollVisualRow + visualIdxInRenderedSet; - const mapEntry = buffer.visualToLogicalMap[absoluteVisualIdx]; - const cursorVisualRow = - cursorVisualRowAbsolute - scrollVisualRow; - const isOnCursorLine = - focus && visualIdxInRenderedSet === cursorVisualRow; + linesToRender.map((lineText, visualIdxInRenderedSet) => { + const absoluteVisualIdx = + scrollVisualRow + visualIdxInRenderedSet; + const mapEntry = buffer.visualToLogicalMap[absoluteVisualIdx]; + const cursorVisualRow = cursorVisualRowAbsolute - scrollVisualRow; + const isOnCursorLine = + focus && visualIdxInRenderedSet === cursorVisualRow; - const renderedLine: React.ReactNode[] = []; + const renderedLine: React.ReactNode[] = []; - const [logicalLineIdx, logicalStartCol] = mapEntry; - const logicalLine = buffer.lines[logicalLineIdx] || ''; - const tokens = parseInputForHighlighting( - logicalLine, - logicalLineIdx, - ); + const [logicalLineIdx, logicalStartCol] = mapEntry; + const logicalLine = buffer.lines[logicalLineIdx] || ''; + const tokens = parseInputForHighlighting( + logicalLine, + logicalLineIdx, + ); - const visualStart = logicalStartCol; - const visualEnd = logicalStartCol + cpLen(lineText); - const segments = buildSegmentsForVisualSlice( - tokens, - visualStart, - visualEnd, - ); + const visualStart = logicalStartCol; + const visualEnd = logicalStartCol + cpLen(lineText); + const segments = buildSegmentsForVisualSlice( + tokens, + visualStart, + visualEnd, + ); - let charCount = 0; - segments.forEach((seg, segIdx) => { - const segLen = cpLen(seg.text); - let display = seg.text; + let charCount = 0; + segments.forEach((seg, segIdx) => { + const segLen = cpLen(seg.text); + let display = seg.text; - if (isOnCursorLine) { - const relativeVisualColForHighlight = - cursorVisualColAbsolute; - const segStart = charCount; - const segEnd = segStart + segLen; - if ( - relativeVisualColForHighlight >= segStart && - relativeVisualColForHighlight < segEnd - ) { - const charToHighlight = cpSlice( + if (isOnCursorLine) { + const relativeVisualColForHighlight = cursorVisualColAbsolute; + const segStart = charCount; + const segEnd = segStart + segLen; + if ( + relativeVisualColForHighlight >= segStart && + relativeVisualColForHighlight < segEnd + ) { + const charToHighlight = cpSlice( + seg.text, + relativeVisualColForHighlight - segStart, + relativeVisualColForHighlight - segStart + 1, + ); + const highlighted = showCursor + ? chalk.inverse(charToHighlight) + : charToHighlight; + display = + cpSlice( seg.text, + 0, relativeVisualColForHighlight - segStart, + ) + + highlighted + + cpSlice( + seg.text, relativeVisualColForHighlight - segStart + 1, ); - const highlighted = showCursor - ? chalk.inverse(charToHighlight) - : charToHighlight; - display = - cpSlice( - seg.text, - 0, - relativeVisualColForHighlight - segStart, - ) + - highlighted + - cpSlice( - seg.text, - relativeVisualColForHighlight - segStart + 1, - ); - } - charCount = segEnd; - } - - const color = - seg.type === 'command' || seg.type === 'file' - ? theme.text.accent - : theme.text.primary; - - renderedLine.push( - - {display} - , - ); - }); - - const currentLineGhost = isOnCursorLine ? inlineGhost : ''; - if ( - isOnCursorLine && - cursorVisualColAbsolute === cpLen(lineText) - ) { - if (!currentLineGhost) { - renderedLine.push( - - {showCursor ? chalk.inverse(' ') : ' '} - , - ); } + charCount = segEnd; } - const showCursorBeforeGhost = - focus && - isOnCursorLine && - cursorVisualColAbsolute === cpLen(lineText) && - currentLineGhost; + const color = + seg.type === 'command' || seg.type === 'file' + ? theme.text.accent + : theme.text.primary; - return ( - - - {renderedLine} - {showCursorBeforeGhost && - (showCursor ? chalk.inverse(' ') : ' ')} - {currentLineGhost && ( - - {currentLineGhost} - - )} - - + renderedLine.push( + + {display} + , ); - }) - .concat( - additionalLines.map((ghostLine, index) => { - const padding = Math.max( - 0, - inputWidth - stringWidth(ghostLine), - ); - return ( - - {ghostLine} - {' '.repeat(padding)} - - ); - }), - ) + }); + + if ( + isOnCursorLine && + cursorVisualColAbsolute === cpLen(lineText) + ) { + renderedLine.push( + + {showCursor ? chalk.inverse(' ') : ' '} + , + ); + } + + return ( + + {renderedLine} + + ); + }) )} diff --git a/packages/cli/src/ui/components/SettingsDialog.test.tsx b/packages/cli/src/ui/components/SettingsDialog.test.tsx index bbd18ecf..f96ec33c 100644 --- a/packages/cli/src/ui/components/SettingsDialog.test.tsx +++ b/packages/cli/src/ui/components/SettingsDialog.test.tsx @@ -1271,7 +1271,6 @@ describe('SettingsDialog', () => { vimMode: true, disableAutoUpdate: true, debugKeystrokeLogging: true, - enablePromptCompletion: true, }, ui: { hideWindowTitle: true, @@ -1517,7 +1516,6 @@ describe('SettingsDialog', () => { vimMode: false, disableAutoUpdate: false, debugKeystrokeLogging: false, - enablePromptCompletion: false, }, ui: { hideWindowTitle: false, diff --git a/packages/cli/src/ui/components/__snapshots__/SettingsDialog.test.tsx.snap b/packages/cli/src/ui/components/__snapshots__/SettingsDialog.test.tsx.snap index b63948e1..cf8d4444 100644 --- a/packages/cli/src/ui/components/__snapshots__/SettingsDialog.test.tsx.snap +++ b/packages/cli/src/ui/components/__snapshots__/SettingsDialog.test.tsx.snap @@ -10,8 +10,6 @@ exports[`SettingsDialog > Snapshot Tests > should render default state correctly │ │ │ Disable Auto Update false │ │ │ -│ Enable Prompt Completion false │ -│ │ │ Debug Keystroke Logging false │ │ │ │ Output Format Text │ @@ -22,6 +20,8 @@ exports[`SettingsDialog > Snapshot Tests > should render default state correctly │ │ │ Hide Tips false │ │ │ +│ Hide Banner false │ +│ │ │ ▼ │ │ │ │ │ @@ -44,8 +44,6 @@ exports[`SettingsDialog > Snapshot Tests > should render focused on scope select │ │ │ Disable Auto Update false │ │ │ -│ Enable Prompt Completion false │ -│ │ │ Debug Keystroke Logging false │ │ │ │ Output Format Text │ @@ -56,6 +54,8 @@ exports[`SettingsDialog > Snapshot Tests > should render focused on scope select │ │ │ Hide Tips false │ │ │ +│ Hide Banner false │ +│ │ │ ▼ │ │ │ │ │ @@ -78,8 +78,6 @@ exports[`SettingsDialog > Snapshot Tests > should render with accessibility sett │ │ │ Disable Auto Update false │ │ │ -│ Enable Prompt Completion false │ -│ │ │ Debug Keystroke Logging false │ │ │ │ Output Format Text │ @@ -90,6 +88,8 @@ exports[`SettingsDialog > Snapshot Tests > should render with accessibility sett │ │ │ Hide Tips false │ │ │ +│ Hide Banner false │ +│ │ │ ▼ │ │ │ │ │ @@ -112,8 +112,6 @@ exports[`SettingsDialog > Snapshot Tests > should render with all boolean settin │ │ │ Disable Auto Update false* │ │ │ -│ Enable Prompt Completion false* │ -│ │ │ Debug Keystroke Logging false* │ │ │ │ Output Format Text │ @@ -124,6 +122,8 @@ exports[`SettingsDialog > Snapshot Tests > should render with all boolean settin │ │ │ Hide Tips false* │ │ │ +│ Hide Banner false │ +│ │ │ ▼ │ │ │ │ │ @@ -146,8 +146,6 @@ exports[`SettingsDialog > Snapshot Tests > should render with different scope se │ │ │ Disable Auto Update (Modified in System) false │ │ │ -│ Enable Prompt Completion false │ -│ │ │ Debug Keystroke Logging false │ │ │ │ Output Format Text │ @@ -158,6 +156,8 @@ exports[`SettingsDialog > Snapshot Tests > should render with different scope se │ │ │ Hide Tips false │ │ │ +│ Hide Banner false │ +│ │ │ ▼ │ │ │ │ │ @@ -180,8 +180,6 @@ exports[`SettingsDialog > Snapshot Tests > should render with different scope se │ │ │ Disable Auto Update false │ │ │ -│ Enable Prompt Completion false │ -│ │ │ Debug Keystroke Logging (Modified in Workspace) false │ │ │ │ Output Format Text │ @@ -192,6 +190,8 @@ exports[`SettingsDialog > Snapshot Tests > should render with different scope se │ │ │ Hide Tips false │ │ │ +│ Hide Banner false │ +│ │ │ ▼ │ │ │ │ │ @@ -214,8 +214,6 @@ exports[`SettingsDialog > Snapshot Tests > should render with file filtering set │ │ │ Disable Auto Update false │ │ │ -│ Enable Prompt Completion false │ -│ │ │ Debug Keystroke Logging false │ │ │ │ Output Format Text │ @@ -226,6 +224,8 @@ exports[`SettingsDialog > Snapshot Tests > should render with file filtering set │ │ │ Hide Tips false │ │ │ +│ Hide Banner false │ +│ │ │ ▼ │ │ │ │ │ @@ -248,8 +248,6 @@ exports[`SettingsDialog > Snapshot Tests > should render with mixed boolean and │ │ │ Disable Auto Update true* │ │ │ -│ Enable Prompt Completion false │ -│ │ │ Debug Keystroke Logging false │ │ │ │ Output Format Text │ @@ -260,6 +258,8 @@ exports[`SettingsDialog > Snapshot Tests > should render with mixed boolean and │ │ │ Hide Tips false │ │ │ +│ Hide Banner false │ +│ │ │ ▼ │ │ │ │ │ @@ -282,8 +282,6 @@ exports[`SettingsDialog > Snapshot Tests > should render with tools and security │ │ │ Disable Auto Update false │ │ │ -│ Enable Prompt Completion false │ -│ │ │ Debug Keystroke Logging false │ │ │ │ Output Format Text │ @@ -294,6 +292,8 @@ exports[`SettingsDialog > Snapshot Tests > should render with tools and security │ │ │ Hide Tips false │ │ │ +│ Hide Banner false │ +│ │ │ ▼ │ │ │ │ │ @@ -316,8 +316,6 @@ exports[`SettingsDialog > Snapshot Tests > should render with various boolean se │ │ │ Disable Auto Update true* │ │ │ -│ Enable Prompt Completion true* │ -│ │ │ Debug Keystroke Logging true* │ │ │ │ Output Format Text │ @@ -328,6 +326,8 @@ exports[`SettingsDialog > Snapshot Tests > should render with various boolean se │ │ │ Hide Tips true* │ │ │ +│ Hide Banner false │ +│ │ │ ▼ │ │ │ │ │ diff --git a/packages/cli/src/ui/hooks/useCommandCompletion.test.ts b/packages/cli/src/ui/hooks/useCommandCompletion.test.ts index bf978395..659b99db 100644 --- a/packages/cli/src/ui/hooks/useCommandCompletion.test.ts +++ b/packages/cli/src/ui/hooks/useCommandCompletion.test.ts @@ -83,9 +83,7 @@ const setupMocks = ({ describe('useCommandCompletion', () => { const mockCommandContext = {} as CommandContext; - const mockConfig = { - getEnablePromptCompletion: () => false, - } as Config; + const mockConfig = {} as Config; const testDirs: string[] = []; const testRootDir = '/'; @@ -516,81 +514,4 @@ describe('useCommandCompletion', () => { ); }); }); - - describe('prompt completion filtering', () => { - it('should not trigger prompt completion for line comments', async () => { - const mockConfig = { - getEnablePromptCompletion: () => true, - } as Config; - - const { result } = renderHook(() => { - const textBuffer = useTextBufferForTest('// This is a line comment'); - const completion = useCommandCompletion( - textBuffer, - testDirs, - testRootDir, - [], - mockCommandContext, - false, - mockConfig, - ); - return { ...completion, textBuffer }; - }); - - // Should not trigger prompt completion for comments - expect(result.current.suggestions.length).toBe(0); - }); - - it('should not trigger prompt completion for block comments', async () => { - const mockConfig = { - getEnablePromptCompletion: () => true, - } as Config; - - const { result } = renderHook(() => { - const textBuffer = useTextBufferForTest( - '/* This is a block comment */', - ); - const completion = useCommandCompletion( - textBuffer, - testDirs, - testRootDir, - [], - mockCommandContext, - false, - mockConfig, - ); - return { ...completion, textBuffer }; - }); - - // Should not trigger prompt completion for comments - expect(result.current.suggestions.length).toBe(0); - }); - - it('should trigger prompt completion for regular text when enabled', async () => { - const mockConfig = { - getEnablePromptCompletion: () => true, - } as Config; - - const { result } = renderHook(() => { - const textBuffer = useTextBufferForTest( - 'This is regular text that should trigger completion', - ); - const completion = useCommandCompletion( - textBuffer, - testDirs, - testRootDir, - [], - mockCommandContext, - false, - mockConfig, - ); - return { ...completion, textBuffer }; - }); - - // This test verifies that comments are filtered out while regular text is not - expect(result.current.textBuffer.text).toBe( - 'This is regular text that should trigger completion', - ); - }); - }); }); diff --git a/packages/cli/src/ui/hooks/useCommandCompletion.tsx b/packages/cli/src/ui/hooks/useCommandCompletion.tsx index e26bb73d..3deaa8a5 100644 --- a/packages/cli/src/ui/hooks/useCommandCompletion.tsx +++ b/packages/cli/src/ui/hooks/useCommandCompletion.tsx @@ -13,11 +13,6 @@ import { isSlashCommand } from '../utils/commandUtils.js'; import { toCodePoints } from '../utils/textUtils.js'; import { useAtCompletion } from './useAtCompletion.js'; import { useSlashCompletion } from './useSlashCompletion.js'; -import type { PromptCompletion } from './usePromptCompletion.js'; -import { - usePromptCompletion, - PROMPT_COMPLETION_MIN_LENGTH, -} from './usePromptCompletion.js'; import type { Config } from '@qwen-code/qwen-code-core'; import { useCompletion } from './useCompletion.js'; @@ -25,7 +20,6 @@ export enum CompletionMode { IDLE = 'IDLE', AT = 'AT', SLASH = 'SLASH', - PROMPT = 'PROMPT', } export interface UseCommandCompletionReturn { @@ -41,7 +35,6 @@ export interface UseCommandCompletionReturn { navigateUp: () => void; navigateDown: () => void; handleAutocomplete: (indexToUse: number) => void; - promptCompletion: PromptCompletion; } export function useCommandCompletion( @@ -126,32 +119,13 @@ export function useCommandCompletion( } } - // Check for prompt completion - only if enabled - const trimmedText = buffer.text.trim(); - const isPromptCompletionEnabled = - config?.getEnablePromptCompletion() ?? false; - - if ( - isPromptCompletionEnabled && - trimmedText.length >= PROMPT_COMPLETION_MIN_LENGTH && - !isSlashCommand(trimmedText) && - !trimmedText.includes('@') - ) { - return { - completionMode: CompletionMode.PROMPT, - query: trimmedText, - completionStart: 0, - completionEnd: trimmedText.length, - }; - } - return { completionMode: CompletionMode.IDLE, query: null, completionStart: -1, completionEnd: -1, }; - }, [cursorRow, cursorCol, buffer.lines, buffer.text, config]); + }, [cursorRow, cursorCol, buffer.lines]); useAtCompletion({ enabled: completionMode === CompletionMode.AT, @@ -172,12 +146,6 @@ export function useCommandCompletion( setIsPerfectMatch, }); - const promptCompletion = usePromptCompletion({ - buffer, - config, - enabled: completionMode === CompletionMode.PROMPT, - }); - useEffect(() => { setActiveSuggestionIndex(suggestions.length > 0 ? 0 : -1); setVisibleStartIndex(0); @@ -264,6 +232,5 @@ export function useCommandCompletion( navigateUp, navigateDown, handleAutocomplete, - promptCompletion, }; } diff --git a/packages/cli/src/ui/hooks/usePromptCompletion.ts b/packages/cli/src/ui/hooks/usePromptCompletion.ts deleted file mode 100644 index 504a22c9..00000000 --- a/packages/cli/src/ui/hooks/usePromptCompletion.ts +++ /dev/null @@ -1,254 +0,0 @@ -/** - * @license - * Copyright 2025 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ - -import { useState, useCallback, useRef, useEffect, useMemo } from 'react'; -import type { Config } from '@qwen-code/qwen-code-core'; -import { - DEFAULT_GEMINI_FLASH_LITE_MODEL, - getResponseText, -} from '@qwen-code/qwen-code-core'; -import type { Content, GenerateContentConfig } from '@google/genai'; -import type { TextBuffer } from '../components/shared/text-buffer.js'; -import { isSlashCommand } from '../utils/commandUtils.js'; - -export const PROMPT_COMPLETION_MIN_LENGTH = 5; -export const PROMPT_COMPLETION_DEBOUNCE_MS = 250; - -export interface PromptCompletion { - text: string; - isLoading: boolean; - isActive: boolean; - accept: () => void; - clear: () => void; - markSelected: (selectedText: string) => void; -} - -export interface UsePromptCompletionOptions { - buffer: TextBuffer; - config?: Config; - enabled: boolean; -} - -export function usePromptCompletion({ - buffer, - config, - enabled, -}: UsePromptCompletionOptions): PromptCompletion { - const [ghostText, setGhostText] = useState(''); - const [isLoadingGhostText, setIsLoadingGhostText] = useState(false); - const abortControllerRef = useRef(null); - const [justSelectedSuggestion, setJustSelectedSuggestion] = - useState(false); - const lastSelectedTextRef = useRef(''); - const lastRequestedTextRef = useRef(''); - - const isPromptCompletionEnabled = - enabled && (config?.getEnablePromptCompletion() ?? false); - - const clearGhostText = useCallback(() => { - setGhostText(''); - setIsLoadingGhostText(false); - }, []); - - const acceptGhostText = useCallback(() => { - if (ghostText && ghostText.length > buffer.text.length) { - buffer.setText(ghostText); - setGhostText(''); - setJustSelectedSuggestion(true); - lastSelectedTextRef.current = ghostText; - } - }, [ghostText, buffer]); - - const markSuggestionSelected = useCallback((selectedText: string) => { - setJustSelectedSuggestion(true); - lastSelectedTextRef.current = selectedText; - }, []); - - const generatePromptSuggestions = useCallback(async () => { - const trimmedText = buffer.text.trim(); - const geminiClient = config?.getGeminiClient(); - - if (trimmedText === lastRequestedTextRef.current) { - return; - } - - if (abortControllerRef.current) { - abortControllerRef.current.abort(); - } - - if ( - trimmedText.length < PROMPT_COMPLETION_MIN_LENGTH || - !geminiClient || - isSlashCommand(trimmedText) || - trimmedText.includes('@') || - !isPromptCompletionEnabled - ) { - clearGhostText(); - lastRequestedTextRef.current = ''; - return; - } - - lastRequestedTextRef.current = trimmedText; - setIsLoadingGhostText(true); - - abortControllerRef.current = new AbortController(); - const signal = abortControllerRef.current.signal; - - try { - const contents: Content[] = [ - { - role: 'user', - parts: [ - { - text: `You are a professional prompt engineering assistant. Complete the user's partial prompt with expert precision and clarity. User's input: "${trimmedText}" Continue this prompt by adding specific, actionable details that align with the user's intent. Focus on: clear, precise language; structured requirements; professional terminology; measurable outcomes. Length Guidelines: Keep suggestions concise (ideally 10-20 characters); prioritize brevity while maintaining clarity; use essential keywords only; avoid redundant phrases. Start your response with the exact user text ("${trimmedText}") followed by your completion. Provide practical, implementation-focused suggestions rather than creative interpretations. Format: Plain text only. Single completion. Match the user's language. Emphasize conciseness over elaboration.`, - }, - ], - }, - ]; - - const generationConfig: GenerateContentConfig = { - temperature: 0.3, - maxOutputTokens: 16000, - thinkingConfig: { - thinkingBudget: 0, - }, - }; - - const response = await geminiClient.generateContent( - contents, - generationConfig, - signal, - DEFAULT_GEMINI_FLASH_LITE_MODEL, - ); - - if (signal.aborted) { - return; - } - - if (response) { - const responseText = getResponseText(response); - - if (responseText) { - const suggestionText = responseText.trim(); - - if ( - suggestionText.length > 0 && - suggestionText.startsWith(trimmedText) - ) { - setGhostText(suggestionText); - } else { - clearGhostText(); - } - } - } - } catch (error) { - if ( - !( - signal.aborted || - (error instanceof Error && error.name === 'AbortError') - ) - ) { - console.error('prompt completion error:', error); - // Clear the last requested text to allow retry only on real errors - lastRequestedTextRef.current = ''; - } - clearGhostText(); - } finally { - if (!signal.aborted) { - setIsLoadingGhostText(false); - } - } - }, [buffer.text, config, clearGhostText, isPromptCompletionEnabled]); - - const isCursorAtEnd = useCallback(() => { - const [cursorRow, cursorCol] = buffer.cursor; - const totalLines = buffer.lines.length; - if (cursorRow !== totalLines - 1) { - return false; - } - - const lastLine = buffer.lines[cursorRow] || ''; - return cursorCol === lastLine.length; - }, [buffer.cursor, buffer.lines]); - - const handlePromptCompletion = useCallback(() => { - if (!isCursorAtEnd()) { - clearGhostText(); - return; - } - - const trimmedText = buffer.text.trim(); - - if (justSelectedSuggestion && trimmedText === lastSelectedTextRef.current) { - return; - } - - if (trimmedText !== lastSelectedTextRef.current) { - setJustSelectedSuggestion(false); - lastSelectedTextRef.current = ''; - } - - generatePromptSuggestions(); - }, [ - buffer.text, - generatePromptSuggestions, - justSelectedSuggestion, - isCursorAtEnd, - clearGhostText, - ]); - - // Debounce prompt completion - useEffect(() => { - const timeoutId = setTimeout( - handlePromptCompletion, - PROMPT_COMPLETION_DEBOUNCE_MS, - ); - return () => clearTimeout(timeoutId); - }, [buffer.text, buffer.cursor, handlePromptCompletion]); - - // Ghost text validation - clear if it doesn't match current text or cursor not at end - useEffect(() => { - const currentText = buffer.text.trim(); - - if (ghostText && !isCursorAtEnd()) { - clearGhostText(); - return; - } - - if ( - ghostText && - currentText.length > 0 && - !ghostText.startsWith(currentText) - ) { - clearGhostText(); - } - }, [buffer.text, buffer.cursor, ghostText, clearGhostText, isCursorAtEnd]); - - // Cleanup on unmount - useEffect(() => () => abortControllerRef.current?.abort(), []); - - const isActive = useMemo(() => { - if (!isPromptCompletionEnabled) return false; - - if (!isCursorAtEnd()) return false; - - const trimmedText = buffer.text.trim(); - return ( - trimmedText.length >= PROMPT_COMPLETION_MIN_LENGTH && - !isSlashCommand(trimmedText) && - !trimmedText.includes('@') - ); - }, [buffer.text, isPromptCompletionEnabled, isCursorAtEnd]); - - return { - text: ghostText, - isLoading: isLoadingGhostText, - isActive, - accept: acceptGhostText, - clear: clearGhostText, - markSelected: markSuggestionSelected, - }; -} diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index c8fa74ab..aa91f785 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -280,7 +280,6 @@ export interface ConfigParameters { skipNextSpeakerCheck?: boolean; shellExecutionConfig?: ShellExecutionConfig; extensionManagement?: boolean; - enablePromptCompletion?: boolean; skipLoopDetection?: boolean; vlmSwitchMode?: string; truncateToolOutputThreshold?: number; @@ -377,7 +376,6 @@ export class Config { private readonly skipNextSpeakerCheck: boolean; private shellExecutionConfig: ShellExecutionConfig; private readonly extensionManagement: boolean = true; - private readonly enablePromptCompletion: boolean = false; private readonly skipLoopDetection: boolean; private readonly skipStartupContext: boolean; private readonly vlmSwitchMode: string | undefined; @@ -495,7 +493,6 @@ export class Config { this.useSmartEdit = params.useSmartEdit ?? false; this.extensionManagement = params.extensionManagement ?? true; this.storage = new Storage(this.targetDir); - this.enablePromptCompletion = params.enablePromptCompletion ?? false; this.vlmSwitchMode = params.vlmSwitchMode; this.fileExclusions = new FileExclusions(this); this.eventEmitter = params.eventEmitter; @@ -1038,10 +1035,6 @@ export class Config { return this.accessibility.screenReader ?? false; } - getEnablePromptCompletion(): boolean { - return this.enablePromptCompletion; - } - getSkipLoopDetection(): boolean { return this.skipLoopDetection; } From a15b84e2a1f1c6ae750127a3bdaf5d08b3acaa43 Mon Sep 17 00:00:00 2001 From: Mingholy Date: Thu, 20 Nov 2025 14:37:39 +0800 Subject: [PATCH 13/14] refactor(auth): enhance useAuthCommand to include history management and improve error handling in QwenOAuth2Client (#1077) --- packages/cli/src/ui/AppContainer.tsx | 2 +- packages/cli/src/ui/auth/useAuth.ts | 28 +++++++--- packages/core/src/qwen/qwenOAuth2.test.ts | 30 ++++++----- packages/core/src/qwen/qwenOAuth2.ts | 65 ++++++++++++----------- 4 files changed, 74 insertions(+), 51 deletions(-) diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index ecacbda4..345bebd2 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -353,7 +353,7 @@ export const AppContainer = (props: AppContainerProps) => { handleAuthSelect, openAuthDialog, cancelAuthentication, - } = useAuthCommand(settings, config); + } = useAuthCommand(settings, config, historyManager.addItem); const { proQuotaRequest, handleProQuotaChoice } = useQuotaAndFallback({ config, diff --git a/packages/cli/src/ui/auth/useAuth.ts b/packages/cli/src/ui/auth/useAuth.ts index 9b1198bf..04da911c 100644 --- a/packages/cli/src/ui/auth/useAuth.ts +++ b/packages/cli/src/ui/auth/useAuth.ts @@ -4,23 +4,28 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { useState, useCallback, useEffect } from 'react'; -import type { LoadedSettings, SettingScope } from '../../config/settings.js'; import type { Config } from '@qwen-code/qwen-code-core'; import { + AuthEvent, AuthType, clearCachedCredentialFile, getErrorMessage, logAuth, - AuthEvent, } from '@qwen-code/qwen-code-core'; -import { AuthState } from '../types.js'; -import { useQwenAuth } from '../hooks/useQwenAuth.js'; +import { useCallback, useEffect, useState } from 'react'; +import type { LoadedSettings, SettingScope } from '../../config/settings.js'; import type { OpenAICredentials } from '../components/OpenAIKeyPrompt.js'; +import { useQwenAuth } from '../hooks/useQwenAuth.js'; +import { AuthState, MessageType } from '../types.js'; +import type { HistoryItem } from '../types.js'; export type { QwenAuthState } from '../hooks/useQwenAuth.js'; -export const useAuthCommand = (settings: LoadedSettings, config: Config) => { +export const useAuthCommand = ( + settings: LoadedSettings, + config: Config, + addItem: (item: Omit, timestamp: number) => void, +) => { const unAuthenticated = settings.merged.security?.auth?.selectedType === undefined; @@ -117,8 +122,17 @@ export const useAuthCommand = (settings: LoadedSettings, config: Config) => { // Log authentication success const authEvent = new AuthEvent(authType, 'manual', 'success'); logAuth(config, authEvent); + + // Show success message + addItem( + { + type: MessageType.INFO, + text: `Authenticated successfully with ${authType} credentials.`, + }, + Date.now(), + ); }, - [settings, handleAuthFailure, config], + [settings, handleAuthFailure, config, addItem], ); const performAuth = useCallback( diff --git a/packages/core/src/qwen/qwenOAuth2.test.ts b/packages/core/src/qwen/qwenOAuth2.test.ts index 2e8bf83e..23c26296 100644 --- a/packages/core/src/qwen/qwenOAuth2.test.ts +++ b/packages/core/src/qwen/qwenOAuth2.test.ts @@ -623,14 +623,16 @@ describe('QwenOAuth2Client', () => { }); it('should handle authorization_pending with HTTP 400 according to RFC 8628', async () => { + const errorData = { + error: 'authorization_pending', + error_description: 'The authorization request is still pending', + }; const mockResponse = { ok: false, status: 400, statusText: 'Bad Request', - json: async () => ({ - error: 'authorization_pending', - error_description: 'The authorization request is still pending', - }), + text: async () => JSON.stringify(errorData), + json: async () => errorData, }; vi.mocked(global.fetch).mockResolvedValue(mockResponse as Response); @@ -646,14 +648,16 @@ describe('QwenOAuth2Client', () => { }); it('should handle slow_down with HTTP 429 according to RFC 8628', async () => { + const errorData = { + error: 'slow_down', + error_description: 'The client is polling too frequently', + }; const mockResponse = { ok: false, status: 429, statusText: 'Too Many Requests', - json: async () => ({ - error: 'slow_down', - error_description: 'The client is polling too frequently', - }), + text: async () => JSON.stringify(errorData), + json: async () => errorData, }; vi.mocked(global.fetch).mockResolvedValue(mockResponse as Response); @@ -1993,14 +1997,16 @@ describe('Enhanced Error Handling and Edge Cases', () => { }); it('should handle authorization_pending with correct status', async () => { + const errorData = { + error: 'authorization_pending', + error_description: 'Authorization request is pending', + }; const mockResponse = { ok: false, status: 400, statusText: 'Bad Request', - json: vi.fn().mockResolvedValue({ - error: 'authorization_pending', - error_description: 'Authorization request is pending', - }), + text: vi.fn().mockResolvedValue(JSON.stringify(errorData)), + json: vi.fn().mockResolvedValue(errorData), }; vi.mocked(global.fetch).mockResolvedValue( diff --git a/packages/core/src/qwen/qwenOAuth2.ts b/packages/core/src/qwen/qwenOAuth2.ts index b9a35bff..c4cfa933 100644 --- a/packages/core/src/qwen/qwenOAuth2.ts +++ b/packages/core/src/qwen/qwenOAuth2.ts @@ -345,44 +345,47 @@ export class QwenOAuth2Client implements IQwenOAuth2Client { }); if (!response.ok) { - // Parse the response as JSON to check for OAuth RFC 8628 standard errors + // Read response body as text first (can only be read once) + const responseText = await response.text(); + + // Try to parse as JSON to check for OAuth RFC 8628 standard errors + let errorData: ErrorData | null = null; try { - const errorData = (await response.json()) as ErrorData; - - // According to OAuth RFC 8628, handle standard polling responses - if ( - response.status === 400 && - errorData.error === 'authorization_pending' - ) { - // User has not yet approved the authorization request. Continue polling. - return { status: 'pending' } as DeviceTokenPendingData; - } - - if (response.status === 429 && errorData.error === 'slow_down') { - // Client is polling too frequently. Return pending with slowDown flag. - return { - status: 'pending', - slowDown: true, - } as DeviceTokenPendingData; - } - - // Handle other 400 errors (access_denied, expired_token, etc.) as real errors - - // For other errors, throw with proper error information - const error = new Error( - `Device token poll failed: ${errorData.error || 'Unknown error'} - ${errorData.error_description || 'No details provided'}`, - ); - (error as Error & { status?: number }).status = response.status; - throw error; + errorData = JSON.parse(responseText) as ErrorData; } catch (_parseError) { - // If JSON parsing fails, fall back to text response - const errorData = await response.text(); + // If JSON parsing fails, use text response const error = new Error( - `Device token poll failed: ${response.status} ${response.statusText}. Response: ${errorData}`, + `Device token poll failed: ${response.status} ${response.statusText}. Response: ${responseText}`, ); (error as Error & { status?: number }).status = response.status; throw error; } + + // According to OAuth RFC 8628, handle standard polling responses + if ( + response.status === 400 && + errorData.error === 'authorization_pending' + ) { + // User has not yet approved the authorization request. Continue polling. + return { status: 'pending' } as DeviceTokenPendingData; + } + + if (response.status === 429 && errorData.error === 'slow_down') { + // Client is polling too frequently. Return pending with slowDown flag. + return { + status: 'pending', + slowDown: true, + } as DeviceTokenPendingData; + } + + // Handle other 400 errors (access_denied, expired_token, etc.) as real errors + + // For other errors, throw with proper error information + const error = new Error( + `Device token poll failed: ${errorData.error || 'Unknown error'} - ${errorData.error_description}`, + ); + (error as Error & { status?: number }).status = response.status; + throw error; } return (await response.json()) as DeviceTokenResponse; From 442a9aed58064a7a049e872eb5ea4b495db9897d Mon Sep 17 00:00:00 2001 From: tanzhenxin Date: Thu, 20 Nov 2025 15:04:00 +0800 Subject: [PATCH 14/14] Replace spawn with execFile for memory-safe command execution (#1068) --- .../shared/BaseSelectionList.test.tsx | 2 +- .../cli/src/ui/hooks/useGitBranchName.test.ts | 106 +-- packages/cli/src/ui/hooks/useGitBranchName.ts | 10 +- packages/cli/src/ui/utils/clipboardUtils.ts | 6 +- packages/cli/src/utils/userStartupWarnings.ts | 12 +- packages/core/src/config/config.test.ts | 6 +- packages/core/src/config/config.ts | 16 +- packages/core/src/services/gitService.test.ts | 25 +- packages/core/src/services/gitService.ts | 13 +- packages/core/src/telemetry/loggers.test.ts | 10 +- packages/core/src/telemetry/loggers.ts | 2 +- .../src/telemetry/qwen-logger/qwen-logger.ts | 13 +- packages/core/src/telemetry/types.ts | 12 +- packages/core/src/tools/grep.test.ts | 75 +- packages/core/src/tools/grep.ts | 28 +- packages/core/src/tools/ripGrep.test.ts | 683 +++--------------- packages/core/src/tools/ripGrep.ts | 61 +- packages/core/src/utils/ripgrepUtils.test.ts | 119 +-- packages/core/src/utils/ripgrepUtils.ts | 260 ++++++- packages/core/src/utils/shell-utils.ts | 130 +++- 20 files changed, 620 insertions(+), 969 deletions(-) diff --git a/packages/cli/src/ui/components/shared/BaseSelectionList.test.tsx b/packages/cli/src/ui/components/shared/BaseSelectionList.test.tsx index 6d432956..e17dea39 100644 --- a/packages/cli/src/ui/components/shared/BaseSelectionList.test.tsx +++ b/packages/cli/src/ui/components/shared/BaseSelectionList.test.tsx @@ -330,7 +330,7 @@ describe('BaseSelectionList', () => { expect(output).not.toContain('Item 5'); }); - it('should scroll up when activeIndex moves before the visible window', async () => { + it.skip('should scroll up when activeIndex moves before the visible window', async () => { const { updateActiveIndex, lastFrame } = renderScrollableList(0); await updateActiveIndex(4); diff --git a/packages/cli/src/ui/hooks/useGitBranchName.test.ts b/packages/cli/src/ui/hooks/useGitBranchName.test.ts index eb1d53d1..a752d073 100644 --- a/packages/cli/src/ui/hooks/useGitBranchName.test.ts +++ b/packages/cli/src/ui/hooks/useGitBranchName.test.ts @@ -4,13 +4,13 @@ * SPDX-License-Identifier: Apache-2.0 */ -import type { MockedFunction } from 'vitest'; +import type { Mock } from 'vitest'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { act } from 'react'; import { renderHook, waitFor } from '@testing-library/react'; import { useGitBranchName } from './useGitBranchName.js'; import { fs, vol } from 'memfs'; // For mocking fs -import { spawnAsync as mockSpawnAsync } from '@qwen-code/qwen-code-core'; +import { isCommandAvailable, execCommand } from '@qwen-code/qwen-code-core'; // Mock @qwen-code/qwen-code-core vi.mock('@qwen-code/qwen-code-core', async () => { @@ -19,7 +19,8 @@ vi.mock('@qwen-code/qwen-code-core', async () => { >('@qwen-code/qwen-code-core'); return { ...original, - spawnAsync: vi.fn(), + execCommand: vi.fn(), + isCommandAvailable: vi.fn(), }; }); @@ -47,6 +48,7 @@ describe('useGitBranchName', () => { [GIT_LOGS_HEAD_PATH]: 'ref: refs/heads/main', }); vi.useFakeTimers(); // Use fake timers for async operations + (isCommandAvailable as Mock).mockReturnValue({ available: true }); }); afterEach(() => { @@ -55,11 +57,11 @@ describe('useGitBranchName', () => { }); it('should return branch name', async () => { - (mockSpawnAsync as MockedFunction).mockResolvedValue( - { - stdout: 'main\n', - } as { stdout: string; stderr: string }, - ); + (execCommand as Mock).mockResolvedValueOnce({ + stdout: 'main\n', + stderr: '', + code: 0, + }); const { result, rerender } = renderHook(() => useGitBranchName(CWD)); await act(async () => { @@ -71,9 +73,7 @@ describe('useGitBranchName', () => { }); it('should return undefined if git command fails', async () => { - (mockSpawnAsync as MockedFunction).mockRejectedValue( - new Error('Git error'), - ); + (execCommand as Mock).mockRejectedValue(new Error('Git error')); const { result, rerender } = renderHook(() => useGitBranchName(CWD)); expect(result.current).toBeUndefined(); @@ -86,16 +86,16 @@ describe('useGitBranchName', () => { }); it('should return short commit hash if branch is HEAD (detached state)', async () => { - ( - mockSpawnAsync as MockedFunction - ).mockImplementation(async (command: string, args: string[]) => { - if (args.includes('--abbrev-ref')) { - return { stdout: 'HEAD\n' } as { stdout: string; stderr: string }; - } else if (args.includes('--short')) { - return { stdout: 'a1b2c3d\n' } as { stdout: string; stderr: string }; - } - return { stdout: '' } as { stdout: string; stderr: string }; - }); + (execCommand as Mock).mockImplementation( + async (_command: string, args?: readonly string[] | null) => { + if (args?.includes('--abbrev-ref')) { + return { stdout: 'HEAD\n', stderr: '', code: 0 }; + } else if (args?.includes('--short')) { + return { stdout: 'a1b2c3d\n', stderr: '', code: 0 }; + } + return { stdout: '', stderr: '', code: 0 }; + }, + ); const { result, rerender } = renderHook(() => useGitBranchName(CWD)); await act(async () => { @@ -106,16 +106,16 @@ describe('useGitBranchName', () => { }); it('should return undefined if branch is HEAD and getting commit hash fails', async () => { - ( - mockSpawnAsync as MockedFunction - ).mockImplementation(async (command: string, args: string[]) => { - if (args.includes('--abbrev-ref')) { - return { stdout: 'HEAD\n' } as { stdout: string; stderr: string }; - } else if (args.includes('--short')) { - throw new Error('Git error'); - } - return { stdout: '' } as { stdout: string; stderr: string }; - }); + (execCommand as Mock).mockImplementation( + async (_command: string, args?: readonly string[] | null) => { + if (args?.includes('--abbrev-ref')) { + return { stdout: 'HEAD\n', stderr: '', code: 0 }; + } else if (args?.includes('--short')) { + throw new Error('Git error'); + } + return { stdout: '', stderr: '', code: 0 }; + }, + ); const { result, rerender } = renderHook(() => useGitBranchName(CWD)); await act(async () => { @@ -127,14 +127,16 @@ describe('useGitBranchName', () => { it('should update branch name when .git/HEAD changes', async ({ skip }) => { skip(); // TODO: fix - (mockSpawnAsync as MockedFunction) - .mockResolvedValueOnce({ stdout: 'main\n' } as { - stdout: string; - stderr: string; + (execCommand as Mock) + .mockResolvedValueOnce({ + stdout: 'main\n', + stderr: '', + code: 0, }) - .mockResolvedValueOnce({ stdout: 'develop\n' } as { - stdout: string; - stderr: string; + .mockResolvedValueOnce({ + stdout: 'develop\n', + stderr: '', + code: 0, }); const { result, rerender } = renderHook(() => useGitBranchName(CWD)); @@ -162,11 +164,11 @@ describe('useGitBranchName', () => { // Remove .git/logs/HEAD to cause an error in fs.watch setup vol.unlinkSync(GIT_LOGS_HEAD_PATH); - (mockSpawnAsync as MockedFunction).mockResolvedValue( - { - stdout: 'main\n', - } as { stdout: string; stderr: string }, - ); + (execCommand as Mock).mockResolvedValue({ + stdout: 'main\n', + stderr: '', + code: 0, + }); const { result, rerender } = renderHook(() => useGitBranchName(CWD)); @@ -177,11 +179,11 @@ describe('useGitBranchName', () => { expect(result.current).toBe('main'); // Branch name should still be fetched initially - ( - mockSpawnAsync as MockedFunction - ).mockResolvedValueOnce({ + (execCommand as Mock).mockResolvedValueOnce({ stdout: 'develop\n', - } as { stdout: string; stderr: string }); + stderr: '', + code: 0, + }); // This write would trigger the watcher if it was set up // but since it failed, the branch name should not update @@ -207,11 +209,11 @@ describe('useGitBranchName', () => { close: closeMock, } as unknown as ReturnType); - (mockSpawnAsync as MockedFunction).mockResolvedValue( - { - stdout: 'main\n', - } as { stdout: string; stderr: string }, - ); + (execCommand as Mock).mockResolvedValue({ + stdout: 'main\n', + stderr: '', + code: 0, + }); const { unmount, rerender } = renderHook(() => useGitBranchName(CWD)); diff --git a/packages/cli/src/ui/hooks/useGitBranchName.ts b/packages/cli/src/ui/hooks/useGitBranchName.ts index af7bccb6..326051a0 100644 --- a/packages/cli/src/ui/hooks/useGitBranchName.ts +++ b/packages/cli/src/ui/hooks/useGitBranchName.ts @@ -5,7 +5,7 @@ */ import { useState, useEffect, useCallback } from 'react'; -import { spawnAsync } from '@qwen-code/qwen-code-core'; +import { isCommandAvailable, execCommand } from '@qwen-code/qwen-code-core'; import fs from 'node:fs'; import fsPromises from 'node:fs/promises'; import path from 'node:path'; @@ -15,7 +15,11 @@ export function useGitBranchName(cwd: string): string | undefined { const fetchBranchName = useCallback(async () => { try { - const { stdout } = await spawnAsync( + if (!isCommandAvailable('git').available) { + return; + } + + const { stdout } = await execCommand( 'git', ['rev-parse', '--abbrev-ref', 'HEAD'], { cwd }, @@ -24,7 +28,7 @@ export function useGitBranchName(cwd: string): string | undefined { if (branch && branch !== 'HEAD') { setBranchName(branch); } else { - const { stdout: hashStdout } = await spawnAsync( + const { stdout: hashStdout } = await execCommand( 'git', ['rev-parse', '--short', 'HEAD'], { cwd }, diff --git a/packages/cli/src/ui/utils/clipboardUtils.ts b/packages/cli/src/ui/utils/clipboardUtils.ts index 67636711..f6d2380b 100644 --- a/packages/cli/src/ui/utils/clipboardUtils.ts +++ b/packages/cli/src/ui/utils/clipboardUtils.ts @@ -6,7 +6,7 @@ import * as fs from 'node:fs/promises'; import * as path from 'node:path'; -import { spawnAsync } from '@qwen-code/qwen-code-core'; +import { execCommand } from '@qwen-code/qwen-code-core'; /** * Checks if the system clipboard contains an image (macOS only for now) @@ -19,7 +19,7 @@ export async function clipboardHasImage(): Promise { try { // Use osascript to check clipboard type - const { stdout } = await spawnAsync('osascript', ['-e', 'clipboard info']); + const { stdout } = await execCommand('osascript', ['-e', 'clipboard info']); const imageRegex = /«class PNGf»|TIFF picture|JPEG picture|GIF picture|«class JPEG»|«class TIFF»/; return imageRegex.test(stdout); @@ -80,7 +80,7 @@ export async function saveClipboardImage( end try `; - const { stdout } = await spawnAsync('osascript', ['-e', script]); + const { stdout } = await execCommand('osascript', ['-e', script]); if (stdout.trim() === 'success') { // Verify the file was created and has content diff --git a/packages/cli/src/utils/userStartupWarnings.ts b/packages/cli/src/utils/userStartupWarnings.ts index 21932684..e2e7c440 100644 --- a/packages/cli/src/utils/userStartupWarnings.ts +++ b/packages/cli/src/utils/userStartupWarnings.ts @@ -67,11 +67,15 @@ const ripgrepAvailabilityCheck: WarningCheck = { return null; } - const isAvailable = await canUseRipgrep(options.useBuiltinRipgrep); - if (!isAvailable) { - return 'Ripgrep not available: Please install ripgrep globally to enable faster file content search. Falling back to built-in grep.'; + try { + const isAvailable = await canUseRipgrep(options.useBuiltinRipgrep); + if (!isAvailable) { + return 'Ripgrep not available: Please install ripgrep globally to enable faster file content search. Falling back to built-in grep.'; + } + return null; + } catch (error) { + return `Ripgrep not available: ${error instanceof Error ? error.message : 'Unknown error'}. Falling back to built-in grep.`; } - return null; }, }; diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 15ef951b..ea897db2 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -1085,7 +1085,7 @@ describe('setApprovalMode with folder trust', () => { expect.any(RipgrepFallbackEvent), ); const event = (logRipgrepFallback as Mock).mock.calls[0][1]; - expect(event.error).toContain('Ripgrep is not available'); + expect(event.error).toContain('ripgrep is not available'); }); it('should fall back to GrepTool and log error when useRipgrep is true and builtin ripgrep is not available', async () => { @@ -1109,7 +1109,7 @@ describe('setApprovalMode with folder trust', () => { expect.any(RipgrepFallbackEvent), ); const event = (logRipgrepFallback as Mock).mock.calls[0][1]; - expect(event.error).toContain('Ripgrep is not available'); + expect(event.error).toContain('ripgrep is not available'); }); it('should fall back to GrepTool and log error when canUseRipgrep throws an error', async () => { @@ -1133,7 +1133,7 @@ describe('setApprovalMode with folder trust', () => { expect.any(RipgrepFallbackEvent), ); const event = (logRipgrepFallback as Mock).mock.calls[0][1]; - expect(event.error).toBe(String(error)); + expect(event.error).toBe(`ripGrep check failed`); }); it('should register GrepTool when useRipgrep is false', async () => { diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index aa91f785..76f923e7 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -82,6 +82,7 @@ import { shouldAttemptBrowserLaunch } from '../utils/browser.js'; import { FileExclusions } from '../utils/ignorePatterns.js'; import { WorkspaceContext } from '../utils/workspaceContext.js'; import { isToolEnabled, type ToolName } from '../utils/tool-utils.js'; +import { getErrorMessage } from '../utils/errors.js'; // Local config modules import type { FileFilteringOptions } from './constants.js'; @@ -1147,17 +1148,20 @@ export class Config { try { useRipgrep = await canUseRipgrep(this.getUseBuiltinRipgrep()); } catch (error: unknown) { - errorString = String(error); + errorString = getErrorMessage(error); } if (useRipgrep) { registerCoreTool(RipGrepTool, this); } else { - errorString = - errorString || - 'Ripgrep is not available. Please install ripgrep globally.'; - // Log for telemetry - logRipgrepFallback(this, new RipgrepFallbackEvent(errorString)); + logRipgrepFallback( + this, + new RipgrepFallbackEvent( + this.getUseRipgrep(), + this.getUseBuiltinRipgrep(), + errorString || 'ripgrep is not available', + ), + ); registerCoreTool(GrepTool, this); } } else { diff --git a/packages/core/src/services/gitService.test.ts b/packages/core/src/services/gitService.test.ts index f2c08831..2442bf56 100644 --- a/packages/core/src/services/gitService.test.ts +++ b/packages/core/src/services/gitService.test.ts @@ -19,10 +19,10 @@ import * as path from 'node:path'; import * as fs from 'node:fs/promises'; import * as os from 'node:os'; import { getProjectHash, QWEN_DIR } from '../utils/paths.js'; -import { spawnAsync } from '../utils/shell-utils.js'; +import { isCommandAvailable } from '../utils/shell-utils.js'; vi.mock('../utils/shell-utils.js', () => ({ - spawnAsync: vi.fn(), + isCommandAvailable: vi.fn(), })); const hoistedMockEnv = vi.hoisted(() => vi.fn()); @@ -76,10 +76,7 @@ describe('GitService', () => { vi.clearAllMocks(); hoistedIsGitRepositoryMock.mockReturnValue(true); - (spawnAsync as Mock).mockResolvedValue({ - stdout: 'git version 2.0.0', - stderr: '', - }); + (isCommandAvailable as Mock).mockReturnValue({ available: true }); hoistedMockHomedir.mockReturnValue(homedir); @@ -119,23 +116,9 @@ describe('GitService', () => { }); }); - describe('verifyGitAvailability', () => { - it('should resolve true if git --version command succeeds', async () => { - const service = new GitService(projectRoot, storage); - await expect(service.verifyGitAvailability()).resolves.toBe(true); - expect(spawnAsync).toHaveBeenCalledWith('git', ['--version']); - }); - - it('should resolve false if git --version command fails', async () => { - (spawnAsync as Mock).mockRejectedValue(new Error('git not found')); - const service = new GitService(projectRoot, storage); - await expect(service.verifyGitAvailability()).resolves.toBe(false); - }); - }); - describe('initialize', () => { it('should throw an error if Git is not available', async () => { - (spawnAsync as Mock).mockRejectedValue(new Error('git not found')); + (isCommandAvailable as Mock).mockReturnValue({ available: false }); const service = new GitService(projectRoot, storage); await expect(service.initialize()).rejects.toThrow( 'Checkpointing is enabled, but Git is not installed. Please install Git or disable checkpointing to continue.', diff --git a/packages/core/src/services/gitService.ts b/packages/core/src/services/gitService.ts index 8d087564..52700bda 100644 --- a/packages/core/src/services/gitService.ts +++ b/packages/core/src/services/gitService.ts @@ -6,7 +6,7 @@ import * as fs from 'node:fs/promises'; import * as path from 'node:path'; -import { spawnAsync } from '../utils/shell-utils.js'; +import { isCommandAvailable } from '../utils/shell-utils.js'; import type { SimpleGit } from 'simple-git'; import { simpleGit, CheckRepoActions } from 'simple-git'; import type { Storage } from '../config/storage.js'; @@ -26,7 +26,7 @@ export class GitService { } async initialize(): Promise { - const gitAvailable = await this.verifyGitAvailability(); + const { available: gitAvailable } = isCommandAvailable('git'); if (!gitAvailable) { throw new Error( 'Checkpointing is enabled, but Git is not installed. Please install Git or disable checkpointing to continue.', @@ -41,15 +41,6 @@ export class GitService { } } - async verifyGitAvailability(): Promise { - try { - await spawnAsync('git', ['--version']); - return true; - } catch (_error) { - return false; - } - } - /** * Creates a hidden git repository in the project root. * The Git repository is used to support checkpointing. diff --git a/packages/core/src/telemetry/loggers.test.ts b/packages/core/src/telemetry/loggers.test.ts index a11ea7f2..324a4f2d 100644 --- a/packages/core/src/telemetry/loggers.test.ts +++ b/packages/core/src/telemetry/loggers.test.ts @@ -447,7 +447,11 @@ describe('loggers', () => { }); it('should log ripgrep fallback event', () => { - const event = new RipgrepFallbackEvent(); + const event = new RipgrepFallbackEvent( + false, + false, + 'ripgrep is not available', + ); logRipgrepFallback(mockConfig, event); @@ -460,13 +464,13 @@ describe('loggers', () => { 'session.id': 'test-session-id', 'user.email': 'test-user@example.com', 'event.name': EVENT_RIPGREP_FALLBACK, - error: undefined, + error: 'ripgrep is not available', }), ); }); it('should log ripgrep fallback event with an error', () => { - const event = new RipgrepFallbackEvent('rg not found'); + const event = new RipgrepFallbackEvent(false, false, 'rg not found'); logRipgrepFallback(mockConfig, event); diff --git a/packages/core/src/telemetry/loggers.ts b/packages/core/src/telemetry/loggers.ts index 5b56719b..efd5af06 100644 --- a/packages/core/src/telemetry/loggers.ts +++ b/packages/core/src/telemetry/loggers.ts @@ -314,7 +314,7 @@ export function logRipgrepFallback( config: Config, event: RipgrepFallbackEvent, ): void { - QwenLogger.getInstance(config)?.logRipgrepFallbackEvent(); + QwenLogger.getInstance(config)?.logRipgrepFallbackEvent(event); if (!isTelemetrySdkInitialized()) return; const attributes: LogAttributes = { diff --git a/packages/core/src/telemetry/qwen-logger/qwen-logger.ts b/packages/core/src/telemetry/qwen-logger/qwen-logger.ts index c5dc70d7..a56723a7 100644 --- a/packages/core/src/telemetry/qwen-logger/qwen-logger.ts +++ b/packages/core/src/telemetry/qwen-logger/qwen-logger.ts @@ -38,6 +38,7 @@ import type { ModelSlashCommandEvent, ExtensionDisableEvent, AuthEvent, + RipgrepFallbackEvent, } from '../types.js'; import { EndSessionEvent } from '../types.js'; import type { @@ -778,8 +779,16 @@ export class QwenLogger { this.flushIfNeeded(); } - logRipgrepFallbackEvent(): void { - const rumEvent = this.createActionEvent('misc', 'ripgrep_fallback', {}); + logRipgrepFallbackEvent(event: RipgrepFallbackEvent): void { + const rumEvent = this.createActionEvent('misc', 'ripgrep_fallback', { + snapshots: JSON.stringify({ + platform: process.platform, + arch: process.arch, + use_ripgrep: event.use_ripgrep, + use_builtin_ripgrep: event.use_builtin_ripgrep, + error: event.error ?? undefined, + }), + }); this.enqueueLogEvent(rumEvent); this.flushIfNeeded(); diff --git a/packages/core/src/telemetry/types.ts b/packages/core/src/telemetry/types.ts index cef83323..8d21f634 100644 --- a/packages/core/src/telemetry/types.ts +++ b/packages/core/src/telemetry/types.ts @@ -318,10 +318,20 @@ export class FlashFallbackEvent implements BaseTelemetryEvent { export class RipgrepFallbackEvent implements BaseTelemetryEvent { 'event.name': 'ripgrep_fallback'; 'event.timestamp': string; + use_ripgrep: boolean; + use_builtin_ripgrep: boolean; + error?: string; - constructor(public error?: string) { + constructor( + use_ripgrep: boolean, + use_builtin_ripgrep: boolean, + error?: string, + ) { this['event.name'] = 'ripgrep_fallback'; this['event.timestamp'] = new Date().toISOString(); + this.use_ripgrep = use_ripgrep; + this.use_builtin_ripgrep = use_builtin_ripgrep; + this.error = error; } } diff --git a/packages/core/src/tools/grep.test.ts b/packages/core/src/tools/grep.test.ts index d613ff03..5a07dcad 100644 --- a/packages/core/src/tools/grep.test.ts +++ b/packages/core/src/tools/grep.test.ts @@ -18,19 +18,68 @@ import * as glob from 'glob'; vi.mock('glob', { spy: true }); // Mock the child_process module to control grep/git grep behavior -vi.mock('child_process', () => ({ - spawn: vi.fn(() => ({ - on: (event: string, cb: (...args: unknown[]) => void) => { - if (event === 'error' || event === 'close') { - // Simulate command not found or error for git grep and system grep - // to force it to fall back to JS implementation. - setTimeout(() => cb(1), 0); // cb(1) for error/close - } - }, - stdout: { on: vi.fn() }, - stderr: { on: vi.fn() }, - })), -})); +vi.mock('child_process', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + spawn: vi.fn(() => { + // Create a proper mock EventEmitter-like child process + const listeners: Map< + string, + Set<(...args: unknown[]) => void> + > = new Map(); + + const createStream = () => ({ + on: vi.fn((event: string, cb: (...args: unknown[]) => void) => { + const key = `stream:${event}`; + if (!listeners.has(key)) listeners.set(key, new Set()); + listeners.get(key)!.add(cb); + }), + removeListener: vi.fn( + (event: string, cb: (...args: unknown[]) => void) => { + const key = `stream:${event}`; + listeners.get(key)?.delete(cb); + }, + ), + }); + + return { + on: vi.fn((event: string, cb: (...args: unknown[]) => void) => { + const key = `child:${event}`; + if (!listeners.has(key)) listeners.set(key, new Set()); + listeners.get(key)!.add(cb); + + // Simulate command not found or error for git grep and system grep + // to force it to fall back to JS implementation. + if (event === 'error') { + setTimeout(() => cb(new Error('Command not found')), 0); + } else if (event === 'close') { + setTimeout(() => cb(1), 0); // Exit code 1 for error + } + }), + removeListener: vi.fn( + (event: string, cb: (...args: unknown[]) => void) => { + const key = `child:${event}`; + listeners.get(key)?.delete(cb); + }, + ), + stdout: createStream(), + stderr: createStream(), + connected: false, + disconnect: vi.fn(), + }; + }), + exec: vi.fn( + ( + cmd: string, + callback: (error: Error | null, stdout: string, stderr: string) => void, + ) => { + // Mock exec to fail for git grep commands + callback(new Error('Command not found'), '', ''); + }, + ), + }; +}); describe('GrepTool', () => { let tempRootDir: string; diff --git a/packages/core/src/tools/grep.ts b/packages/core/src/tools/grep.ts index df410f0c..934ab57b 100644 --- a/packages/core/src/tools/grep.ts +++ b/packages/core/src/tools/grep.ts @@ -18,6 +18,7 @@ import { isGitRepository } from '../utils/gitUtils.js'; import type { Config } from '../config/config.js'; import type { FileExclusions } from '../utils/ignorePatterns.js'; import { ToolErrorType } from './tool-error.js'; +import { isCommandAvailable } from '../utils/shell-utils.js'; // --- Interfaces --- @@ -195,29 +196,6 @@ class GrepToolInvocation extends BaseToolInvocation< } } - /** - * Checks if a command is available in the system's PATH. - * @param {string} command The command name (e.g., 'git', 'grep'). - * @returns {Promise} True if the command is available, false otherwise. - */ - private isCommandAvailable(command: string): Promise { - return new Promise((resolve) => { - const checkCommand = process.platform === 'win32' ? 'where' : 'command'; - const checkArgs = - process.platform === 'win32' ? [command] : ['-v', command]; - try { - const child = spawn(checkCommand, checkArgs, { - stdio: 'ignore', - shell: process.platform === 'win32', - }); - child.on('close', (code) => resolve(code === 0)); - child.on('error', () => resolve(false)); - } catch { - resolve(false); - } - }); - } - /** * Parses the standard output of grep-like commands (git grep, system grep). * Expects format: filePath:lineNumber:lineContent @@ -297,7 +275,7 @@ class GrepToolInvocation extends BaseToolInvocation< try { // --- Strategy 1: git grep --- const isGit = isGitRepository(absolutePath); - const gitAvailable = isGit && (await this.isCommandAvailable('git')); + const gitAvailable = isGit && isCommandAvailable('git').available; if (gitAvailable) { strategyUsed = 'git grep'; @@ -350,7 +328,7 @@ class GrepToolInvocation extends BaseToolInvocation< } // --- Strategy 2: System grep --- - const grepAvailable = await this.isCommandAvailable('grep'); + const { available: grepAvailable } = isCommandAvailable('grep'); if (grepAvailable) { strategyUsed = 'system grep'; const grepArgs = ['-r', '-n', '-H', '-E']; diff --git a/packages/core/src/tools/ripGrep.test.ts b/packages/core/src/tools/ripGrep.test.ts index 1b7dfe2d..05730a7e 100644 --- a/packages/core/src/tools/ripGrep.test.ts +++ b/packages/core/src/tools/ripGrep.test.ts @@ -20,14 +20,13 @@ import fs from 'node:fs/promises'; import os, { EOL } from 'node:os'; import type { Config } from '../config/config.js'; import { createMockWorkspaceContext } from '../test-utils/mockWorkspaceContext.js'; -import type { ChildProcess } from 'node:child_process'; import { spawn } from 'node:child_process'; -import { getRipgrepCommand } from '../utils/ripgrepUtils.js'; +import { runRipgrep } from '../utils/ripgrepUtils.js'; import { DEFAULT_FILE_FILTERING_OPTIONS } from '../config/constants.js'; // Mock ripgrepUtils vi.mock('../utils/ripgrepUtils.js', () => ({ - getRipgrepCommand: vi.fn(), + runRipgrep: vi.fn(), })); // Mock child_process for ripgrep calls @@ -37,60 +36,6 @@ vi.mock('child_process', () => ({ const mockSpawn = vi.mocked(spawn); -// Helper function to create mock spawn implementations -function createMockSpawn( - options: { - outputData?: string; - exitCode?: number; - signal?: string; - onCall?: ( - command: string, - args: readonly string[], - spawnOptions?: unknown, - ) => void; - } = {}, -) { - const { outputData, exitCode = 0, signal, onCall } = options; - - return (command: string, args: readonly string[], spawnOptions?: unknown) => { - onCall?.(command, args, spawnOptions); - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - // Set up event listeners immediately - setTimeout(() => { - const stdoutDataHandler = mockProcess.stdout.on.mock.calls.find( - (call) => call[0] === 'data', - )?.[1]; - - const closeHandler = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (stdoutDataHandler && outputData) { - stdoutDataHandler(Buffer.from(outputData)); - } - - if (closeHandler) { - closeHandler(exitCode, signal); - } - }, 0); - - return mockProcess as unknown as ChildProcess; - }; -} - describe('RipGrepTool', () => { let tempRootDir: string; let grepTool: RipGrepTool; @@ -109,7 +54,6 @@ describe('RipGrepTool', () => { beforeEach(async () => { vi.clearAllMocks(); - (getRipgrepCommand as Mock).mockResolvedValue('/mock/path/to/rg'); mockSpawn.mockReset(); tempRootDir = await fs.mkdtemp(path.join(os.tmpdir(), 'grep-tool-root-')); fileExclusionsMock = { @@ -200,12 +144,11 @@ describe('RipGrepTool', () => { describe('execute', () => { it('should find matches for a simple pattern in all files', async () => { - mockSpawn.mockImplementationOnce( - createMockSpawn({ - outputData: `fileA.txt:1:hello world${EOL}fileA.txt:2:second line with world${EOL}sub/fileC.txt:1:another world in sub dir${EOL}`, - exitCode: 0, - }), - ); + (runRipgrep as Mock).mockResolvedValue({ + stdout: `fileA.txt:1:hello world${EOL}fileA.txt:2:second line with world${EOL}sub/fileC.txt:1:another world in sub dir${EOL}`, + truncated: false, + error: undefined, + }); const params: RipGrepToolParams = { pattern: 'world' }; const invocation = grepTool.build(params); @@ -223,12 +166,11 @@ describe('RipGrepTool', () => { it('should find matches in a specific path', async () => { // Setup specific mock for this test - searching in 'sub' should only return matches from that directory - mockSpawn.mockImplementationOnce( - createMockSpawn({ - outputData: `fileC.txt:1:another world in sub dir${EOL}`, - exitCode: 0, - }), - ); + (runRipgrep as Mock).mockResolvedValue({ + stdout: `fileC.txt:1:another world in sub dir${EOL}`, + truncated: false, + error: undefined, + }); const params: RipGrepToolParams = { pattern: 'world', path: 'sub' }; const invocation = grepTool.build(params); @@ -243,16 +185,11 @@ describe('RipGrepTool', () => { }); it('should use target directory when path is not provided', async () => { - mockSpawn.mockImplementationOnce( - createMockSpawn({ - outputData: `fileA.txt:1:hello world${EOL}`, - exitCode: 0, - onCall: (_, args) => { - // Should search in the target directory (tempRootDir) - expect(args[args.length - 1]).toBe(tempRootDir); - }, - }), - ); + (runRipgrep as Mock).mockResolvedValue({ + stdout: `fileA.txt:1:hello world${EOL}`, + truncated: false, + error: undefined, + }); const params: RipGrepToolParams = { pattern: 'world' }; const invocation = grepTool.build(params); @@ -264,12 +201,11 @@ describe('RipGrepTool', () => { it('should find matches with a glob filter', async () => { // Setup specific mock for this test - mockSpawn.mockImplementationOnce( - createMockSpawn({ - outputData: `fileB.js:2:function baz() { return "hello"; }${EOL}`, - exitCode: 0, - }), - ); + (runRipgrep as Mock).mockResolvedValue({ + stdout: `fileB.js:2:function baz() { return "hello"; }${EOL}`, + truncated: false, + error: undefined, + }); const params: RipGrepToolParams = { pattern: 'hello', glob: '*.js' }; const invocation = grepTool.build(params); @@ -290,39 +226,10 @@ describe('RipGrepTool', () => { ); // Setup specific mock for this test - searching for 'hello' in 'sub' with '*.js' filter - mockSpawn.mockImplementationOnce(() => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - setTimeout(() => { - const onData = mockProcess.stdout.on.mock.calls.find( - (call) => call[0] === 'data', - )?.[1]; - const onClose = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (onData) { - // Only return match from the .js file in sub directory - onData(Buffer.from(`another.js:1:const greeting = "hello";${EOL}`)); - } - if (onClose) { - onClose(0); - } - }, 0); - - return mockProcess as unknown as ChildProcess; + (runRipgrep as Mock).mockResolvedValue({ + stdout: `another.js:1:const greeting = "hello";${EOL}`, + truncated: false, + error: undefined, }); const params: RipGrepToolParams = { @@ -346,15 +253,11 @@ describe('RipGrepTool', () => { path.join(tempRootDir, '.qwenignore'), 'ignored.txt\n', ); - mockSpawn.mockImplementationOnce( - createMockSpawn({ - exitCode: 1, - onCall: (_, args) => { - expect(args).toContain('--ignore-file'); - expect(args).toContain(path.join(tempRootDir, '.qwenignore')); - }, - }), - ); + (runRipgrep as Mock).mockResolvedValue({ + stdout: '', + truncated: false, + error: undefined, + }); const params: RipGrepToolParams = { pattern: 'secret' }; const invocation = grepTool.build(params); @@ -375,16 +278,11 @@ describe('RipGrepTool', () => { }), }); - mockSpawn.mockImplementationOnce( - createMockSpawn({ - outputData: `kept.txt:1:keep me${EOL}`, - exitCode: 0, - onCall: (_, args) => { - expect(args).not.toContain('--ignore-file'); - expect(args).not.toContain(path.join(tempRootDir, '.qwenignore')); - }, - }), - ); + (runRipgrep as Mock).mockResolvedValue({ + stdout: `kept.txt:1:keep me${EOL}`, + truncated: false, + error: undefined, + }); const params: RipGrepToolParams = { pattern: 'keep' }; const invocation = grepTool.build(params); @@ -404,14 +302,11 @@ describe('RipGrepTool', () => { }), }); - mockSpawn.mockImplementationOnce( - createMockSpawn({ - exitCode: 1, - onCall: (_, args) => { - expect(args).toContain('--no-ignore-vcs'); - }, - }), - ); + (runRipgrep as Mock).mockResolvedValue({ + stdout: '', + truncated: false, + error: undefined, + }); const params: RipGrepToolParams = { pattern: 'ignored' }; const invocation = grepTool.build(params); @@ -421,12 +316,11 @@ describe('RipGrepTool', () => { it('should truncate llm content when exceeding maximum length', async () => { const longMatch = 'fileA.txt:1:' + 'a'.repeat(30_000); - mockSpawn.mockImplementationOnce( - createMockSpawn({ - outputData: `${longMatch}${EOL}`, - exitCode: 0, - }), - ); + (runRipgrep as Mock).mockResolvedValue({ + stdout: `${longMatch}${EOL}`, + truncated: false, + error: undefined, + }); const params: RipGrepToolParams = { pattern: 'a+' }; const invocation = grepTool.build(params); @@ -439,11 +333,11 @@ describe('RipGrepTool', () => { it('should return "No matches found" when pattern does not exist', async () => { // Setup specific mock for no matches - mockSpawn.mockImplementationOnce( - createMockSpawn({ - exitCode: 1, // No matches found - }), - ); + (runRipgrep as Mock).mockResolvedValue({ + stdout: '', + truncated: false, + error: undefined, + }); const params: RipGrepToolParams = { pattern: 'nonexistentpattern' }; const invocation = grepTool.build(params); @@ -463,39 +357,10 @@ describe('RipGrepTool', () => { it('should handle regex special characters correctly', async () => { // Setup specific mock for this test - regex pattern 'foo.*bar' should match 'const foo = "bar";' - mockSpawn.mockImplementationOnce(() => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - setTimeout(() => { - const onData = mockProcess.stdout.on.mock.calls.find( - (call) => call[0] === 'data', - )?.[1]; - const onClose = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (onData) { - // Return match for the regex pattern - onData(Buffer.from(`fileB.js:1:const foo = "bar";${EOL}`)); - } - if (onClose) { - onClose(0); - } - }, 0); - - return mockProcess as unknown as ChildProcess; + (runRipgrep as Mock).mockResolvedValue({ + stdout: `fileB.js:1:const foo = "bar";${EOL}`, + truncated: false, + error: undefined, }); const params: RipGrepToolParams = { pattern: 'foo.*bar' }; // Matches 'const foo = "bar";' @@ -509,43 +374,10 @@ describe('RipGrepTool', () => { it('should be case-insensitive by default (JS fallback)', async () => { // Setup specific mock for this test - case insensitive search for 'HELLO' - mockSpawn.mockImplementationOnce(() => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - setTimeout(() => { - const onData = mockProcess.stdout.on.mock.calls.find( - (call) => call[0] === 'data', - )?.[1]; - const onClose = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (onData) { - // Return case-insensitive matches for 'HELLO' - onData( - Buffer.from( - `fileA.txt:1:hello world${EOL}fileB.js:2:function baz() { return "hello"; }${EOL}`, - ), - ); - } - if (onClose) { - onClose(0); - } - }, 0); - - return mockProcess as unknown as ChildProcess; + (runRipgrep as Mock).mockResolvedValue({ + stdout: `fileA.txt:1:hello world${EOL}fileB.js:2:function baz() { return "hello"; }${EOL}`, + truncated: false, + error: undefined, }); const params: RipGrepToolParams = { pattern: 'HELLO' }; @@ -568,12 +400,11 @@ describe('RipGrepTool', () => { }); it('should search within a single file when path is a file', async () => { - mockSpawn.mockImplementationOnce( - createMockSpawn({ - outputData: `fileA.txt:1:hello world${EOL}fileA.txt:2:second line with world${EOL}`, - exitCode: 0, - }), - ); + (runRipgrep as Mock).mockResolvedValue({ + stdout: `fileA.txt:1:hello world${EOL}fileA.txt:2:second line with world${EOL}`, + truncated: false, + error: undefined, + }); const params: RipGrepToolParams = { pattern: 'world', @@ -588,7 +419,11 @@ describe('RipGrepTool', () => { }); it('should throw an error if ripgrep is not available', async () => { - (getRipgrepCommand as Mock).mockResolvedValue(null); + (runRipgrep as Mock).mockResolvedValue({ + stdout: '', + truncated: false, + error: new Error('ripgrep binary not found.'), + }); const params: RipGrepToolParams = { pattern: 'world' }; const invocation = grepTool.build(params); @@ -612,54 +447,6 @@ describe('RipGrepTool', () => { const result = await invocation.execute(controller.signal); expect(result).toBeDefined(); }); - - it('should abort streaming search when signal is triggered', async () => { - // Setup specific mock for this test - simulate process being killed due to abort - mockSpawn.mockImplementationOnce(() => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - // Simulate process being aborted - use setTimeout to ensure handlers are registered first - setTimeout(() => { - const closeHandler = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (closeHandler) { - // Simulate process killed by signal (code is null, signal is SIGTERM) - closeHandler(null, 'SIGTERM'); - } - }, 0); - - return mockProcess as unknown as ChildProcess; - }); - - const controller = new AbortController(); - const params: RipGrepToolParams = { pattern: 'test' }; - const invocation = grepTool.build(params); - - // Abort immediately before starting the search - controller.abort(); - - const result = await invocation.execute(controller.signal); - expect(result.llmContent).toContain( - 'Error during grep search operation: ripgrep exited with code null', - ); - expect(result.returnDisplay).toContain( - 'Error: ripgrep exited with code null', - ); - }); }); describe('error handling and edge cases', () => { @@ -675,32 +462,10 @@ describe('RipGrepTool', () => { await fs.mkdir(emptyDir); // Setup specific mock for this test - searching in empty directory should return no matches - mockSpawn.mockImplementationOnce(() => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - setTimeout(() => { - const onClose = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (onClose) { - onClose(1); - } - }, 0); - - return mockProcess as unknown as ChildProcess; + (runRipgrep as Mock).mockResolvedValue({ + stdout: '', + truncated: false, + error: undefined, }); const params: RipGrepToolParams = { pattern: 'test', path: 'empty' }; @@ -715,32 +480,10 @@ describe('RipGrepTool', () => { await fs.writeFile(path.join(tempRootDir, 'empty.txt'), ''); // Setup specific mock for this test - searching for anything in empty files should return no matches - mockSpawn.mockImplementationOnce(() => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - setTimeout(() => { - const onClose = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (onClose) { - onClose(1); - } - }, 0); - - return mockProcess as unknown as ChildProcess; + (runRipgrep as Mock).mockResolvedValue({ + stdout: '', + truncated: false, + error: undefined, }); const params: RipGrepToolParams = { pattern: 'anything' }; @@ -758,42 +501,10 @@ describe('RipGrepTool', () => { ); // Setup specific mock for this test - searching for 'world' should find the file with special characters - mockSpawn.mockImplementationOnce(() => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - setTimeout(() => { - const onData = mockProcess.stdout.on.mock.calls.find( - (call) => call[0] === 'data', - )?.[1]; - const onClose = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (onData) { - onData( - Buffer.from( - `${specialFileName}:1:hello world with special chars${EOL}`, - ), - ); - } - if (onClose) { - onClose(0); - } - }, 0); - - return mockProcess as unknown as ChildProcess; + (runRipgrep as Mock).mockResolvedValue({ + stdout: `file with spaces & symbols!.txt:1:hello world with special chars${EOL}`, + truncated: false, + error: undefined, }); const params: RipGrepToolParams = { pattern: 'world' }; @@ -813,42 +524,10 @@ describe('RipGrepTool', () => { ); // Setup specific mock for this test - searching for 'deep' should find the deeply nested file - mockSpawn.mockImplementationOnce(() => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - setTimeout(() => { - const onData = mockProcess.stdout.on.mock.calls.find( - (call) => call[0] === 'data', - )?.[1]; - const onClose = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (onData) { - onData( - Buffer.from( - `a/b/c/d/e/deep.txt:1:content in deep directory${EOL}`, - ), - ); - } - if (onClose) { - onClose(0); - } - }, 0); - - return mockProcess as unknown as ChildProcess; + (runRipgrep as Mock).mockResolvedValue({ + stdout: `a/b/c/d/e/deep.txt:1:content in deep directory${EOL}`, + truncated: false, + error: undefined, }); const params: RipGrepToolParams = { pattern: 'deep' }; @@ -868,42 +547,10 @@ describe('RipGrepTool', () => { ); // Setup specific mock for this test - regex pattern should match function declarations - mockSpawn.mockImplementationOnce(() => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - setTimeout(() => { - const onData = mockProcess.stdout.on.mock.calls.find( - (call) => call[0] === 'data', - )?.[1]; - const onClose = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (onData) { - onData( - Buffer.from( - `code.js:1:function getName() { return "test"; }${EOL}`, - ), - ); - } - if (onClose) { - onClose(0); - } - }, 0); - - return mockProcess as unknown as ChildProcess; + (runRipgrep as Mock).mockResolvedValue({ + stdout: `code.js:1:function getName() { return "test"; }${EOL}`, + truncated: false, + error: undefined, }); const params: RipGrepToolParams = { pattern: 'function\\s+\\w+\\s*\\(' }; @@ -921,42 +568,10 @@ describe('RipGrepTool', () => { ); // Setup specific mock for this test - case insensitive search should match all variants - mockSpawn.mockImplementationOnce(() => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - setTimeout(() => { - const onData = mockProcess.stdout.on.mock.calls.find( - (call) => call[0] === 'data', - )?.[1]; - const onClose = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (onData) { - onData( - Buffer.from( - `case.txt:1:Hello World${EOL}case.txt:2:hello world${EOL}case.txt:3:HELLO WORLD${EOL}`, - ), - ); - } - if (onClose) { - onClose(0); - } - }, 0); - - return mockProcess as unknown as ChildProcess; + (runRipgrep as Mock).mockResolvedValue({ + stdout: `case.txt:1:Hello World${EOL}case.txt:2:hello world${EOL}case.txt:3:HELLO WORLD${EOL}`, + truncated: false, + error: undefined, }); const params: RipGrepToolParams = { pattern: 'hello' }; @@ -975,38 +590,10 @@ describe('RipGrepTool', () => { ); // Setup specific mock for this test - escaped regex pattern should match price format - mockSpawn.mockImplementationOnce(() => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - setTimeout(() => { - const onData = mockProcess.stdout.on.mock.calls.find( - (call) => call[0] === 'data', - )?.[1]; - const onClose = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (onData) { - onData(Buffer.from(`special.txt:1:Price: $19.99${EOL}`)); - } - if (onClose) { - onClose(0); - } - }, 0); - - return mockProcess as unknown as ChildProcess; + (runRipgrep as Mock).mockResolvedValue({ + stdout: `special.txt:1:Price: $19.99${EOL}`, + truncated: false, + error: undefined, }); const params: RipGrepToolParams = { pattern: '\\$\\d+\\.\\d+' }; @@ -1032,42 +619,10 @@ describe('RipGrepTool', () => { await fs.writeFile(path.join(tempRootDir, 'test.txt'), 'text content'); // Setup specific mock for this test - glob pattern should filter to only ts/tsx files - mockSpawn.mockImplementationOnce(() => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - setTimeout(() => { - const onData = mockProcess.stdout.on.mock.calls.find( - (call) => call[0] === 'data', - )?.[1]; - const onClose = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (onData) { - onData( - Buffer.from( - `test.ts:1:typescript content${EOL}test.tsx:1:tsx content${EOL}`, - ), - ); - } - if (onClose) { - onClose(0); - } - }, 0); - - return mockProcess as unknown as ChildProcess; + (runRipgrep as Mock).mockResolvedValue({ + stdout: `test.ts:1:typescript content${EOL}test.tsx:1:tsx content${EOL}`, + truncated: false, + error: undefined, }); const params: RipGrepToolParams = { @@ -1092,38 +647,10 @@ describe('RipGrepTool', () => { await fs.writeFile(path.join(tempRootDir, 'other.ts'), 'other code'); // Setup specific mock for this test - glob pattern should filter to only src/** files - mockSpawn.mockImplementationOnce(() => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - setTimeout(() => { - const onData = mockProcess.stdout.on.mock.calls.find( - (call) => call[0] === 'data', - )?.[1]; - const onClose = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (onData) { - onData(Buffer.from(`src/main.ts:1:source code${EOL}`)); - } - if (onClose) { - onClose(0); - } - }, 0); - - return mockProcess as unknown as ChildProcess; + (runRipgrep as Mock).mockResolvedValue({ + stdout: `src/main.ts:1:source code${EOL}`, + truncated: false, + error: undefined, }); const params: RipGrepToolParams = { diff --git a/packages/core/src/tools/ripGrep.ts b/packages/core/src/tools/ripGrep.ts index 402a5d3c..9fcd0e3d 100644 --- a/packages/core/src/tools/ripGrep.ts +++ b/packages/core/src/tools/ripGrep.ts @@ -6,14 +6,13 @@ import fs from 'node:fs'; import path from 'node:path'; -import { spawn } from 'node:child_process'; import type { ToolInvocation, ToolResult } from './tools.js'; import { BaseDeclarativeTool, BaseToolInvocation, Kind } from './tools.js'; import { ToolNames } from './tool-names.js'; import { resolveAndValidatePath } from '../utils/paths.js'; import { getErrorMessage } from '../utils/errors.js'; import type { Config } from '../config/config.js'; -import { getRipgrepCommand } from '../utils/ripgrepUtils.js'; +import { runRipgrep } from '../utils/ripgrepUtils.js'; import { SchemaValidator } from '../utils/schemaValidator.js'; import type { FileFilteringOptions } from '../config/constants.js'; import { DEFAULT_FILE_FILTERING_OPTIONS } from '../config/constants.js'; @@ -208,60 +207,12 @@ class GrepToolInvocation extends BaseToolInvocation< rgArgs.push('--threads', '4'); rgArgs.push(absolutePath); - try { - const rgCommand = await getRipgrepCommand( - this.config.getUseBuiltinRipgrep(), - ); - if (!rgCommand) { - throw new Error('ripgrep binary not found.'); - } - - const output = await new Promise((resolve, reject) => { - const child = spawn(rgCommand, rgArgs, { - windowsHide: true, - }); - - const stdoutChunks: Buffer[] = []; - const stderrChunks: Buffer[] = []; - - const cleanup = () => { - if (options.signal.aborted) { - child.kill(); - } - }; - - options.signal.addEventListener('abort', cleanup, { once: true }); - - child.stdout.on('data', (chunk) => stdoutChunks.push(chunk)); - child.stderr.on('data', (chunk) => stderrChunks.push(chunk)); - - child.on('error', (err) => { - options.signal.removeEventListener('abort', cleanup); - reject(new Error(`failed to start ripgrep: ${err.message}.`)); - }); - - child.on('close', (code) => { - options.signal.removeEventListener('abort', cleanup); - const stdoutData = Buffer.concat(stdoutChunks).toString('utf8'); - const stderrData = Buffer.concat(stderrChunks).toString('utf8'); - - if (code === 0) { - resolve(stdoutData); - } else if (code === 1) { - resolve(''); // No matches found - } else { - reject( - new Error(`ripgrep exited with code ${code}: ${stderrData}`), - ); - } - }); - }); - - return output; - } catch (error: unknown) { - console.error(`Ripgrep failed: ${getErrorMessage(error)}`); - throw error; + const result = await runRipgrep(rgArgs, options.signal); + if (result.error && !result.stdout) { + throw result.error; } + + return result.stdout; } private getFileFilteringOptions(): FileFilteringOptions { diff --git a/packages/core/src/utils/ripgrepUtils.test.ts b/packages/core/src/utils/ripgrepUtils.test.ts index 47bba8a5..43af1039 100644 --- a/packages/core/src/utils/ripgrepUtils.test.ts +++ b/packages/core/src/utils/ripgrepUtils.test.ts @@ -4,30 +4,12 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, beforeEach, vi, type Mock } from 'vitest'; -import { - canUseRipgrep, - getRipgrepCommand, - getBuiltinRipgrep, -} from './ripgrepUtils.js'; -import { fileExists } from './fileUtils.js'; +import { describe, it, expect } from 'vitest'; +import { getBuiltinRipgrep } from './ripgrepUtils.js'; import path from 'node:path'; -// Mock fileUtils -vi.mock('./fileUtils.js', async (importOriginal) => { - const actual = await importOriginal(); - return { - ...actual, - fileExists: vi.fn(), - }; -}); - describe('ripgrepUtils', () => { - beforeEach(() => { - vi.clearAllMocks(); - }); - - describe('getBulltinRipgrepPath', () => { + describe('getBuiltinRipgrep', () => { it('should return path with .exe extension on Windows', () => { const originalPlatform = process.platform; const originalArch = process.arch; @@ -150,99 +132,4 @@ describe('ripgrepUtils', () => { Object.defineProperty(process, 'arch', { value: originalArch }); }); }); - - describe('canUseRipgrep', () => { - it('should return true if ripgrep binary exists (builtin)', async () => { - (fileExists as Mock).mockResolvedValue(true); - - const result = await canUseRipgrep(true); - - expect(result).toBe(true); - expect(fileExists).toHaveBeenCalledOnce(); - }); - - it('should return true if ripgrep binary exists (default)', async () => { - (fileExists as Mock).mockResolvedValue(true); - - const result = await canUseRipgrep(); - - expect(result).toBe(true); - expect(fileExists).toHaveBeenCalledOnce(); - }); - }); - - describe('ensureRipgrepPath', () => { - it('should return bundled ripgrep path if binary exists (useBuiltin=true)', async () => { - (fileExists as Mock).mockResolvedValue(true); - - const rgPath = await getRipgrepCommand(true); - - expect(rgPath).toBeDefined(); - expect(rgPath).toContain('rg'); - expect(rgPath).not.toBe('rg'); // Should be full path, not just 'rg' - expect(fileExists).toHaveBeenCalledOnce(); - expect(fileExists).toHaveBeenCalledWith(rgPath); - }); - - it('should return bundled ripgrep path if binary exists (default)', async () => { - (fileExists as Mock).mockResolvedValue(true); - - const rgPath = await getRipgrepCommand(); - - expect(rgPath).toBeDefined(); - expect(rgPath).toContain('rg'); - expect(fileExists).toHaveBeenCalledOnce(); - }); - - it('should fall back to system rg if bundled binary does not exist', async () => { - (fileExists as Mock).mockResolvedValue(false); - // When useBuiltin is true but bundled binary doesn't exist, - // it should fall back to checking system rg - // The test result depends on whether system rg is actually available - - const rgPath = await getRipgrepCommand(true); - - expect(fileExists).toHaveBeenCalledOnce(); - // If system rg is available, it should return 'rg' (or 'rg.exe' on Windows) - // This test will pass if system ripgrep is installed - expect(rgPath).toBeDefined(); - }); - - it('should use system rg when useBuiltin=false', async () => { - // When useBuiltin is false, should skip bundled check and go straight to system rg - const rgPath = await getRipgrepCommand(false); - - // Should not check for bundled binary - expect(fileExists).not.toHaveBeenCalled(); - // If system rg is available, it should return 'rg' (or 'rg.exe' on Windows) - expect(rgPath).toBeDefined(); - }); - - it('should throw error if neither bundled nor system ripgrep is available', async () => { - // This test only makes sense in an environment where system rg is not installed - // We'll skip this test in CI/local environments where rg might be available - // Instead, we test the error message format - const originalPlatform = process.platform; - - // Use an unsupported platform to trigger the error path - Object.defineProperty(process, 'platform', { value: 'freebsd' }); - - try { - await getRipgrepCommand(); - // If we get here without error, system rg was available, which is fine - } catch (error) { - expect(error).toBeInstanceOf(Error); - const errorMessage = (error as Error).message; - // Should contain helpful error information - expect( - errorMessage.includes('Ripgrep binary not found') || - errorMessage.includes('Failed to locate ripgrep') || - errorMessage.includes('Unsupported platform'), - ).toBe(true); - } - - // Restore original value - Object.defineProperty(process, 'platform', { value: originalPlatform }); - }); - }); }); diff --git a/packages/core/src/utils/ripgrepUtils.ts b/packages/core/src/utils/ripgrepUtils.ts index c6d795a3..1f432541 100644 --- a/packages/core/src/utils/ripgrepUtils.ts +++ b/packages/core/src/utils/ripgrepUtils.ts @@ -6,7 +6,53 @@ import path from 'node:path'; import { fileURLToPath } from 'node:url'; +import { execFile } from 'node:child_process'; import { fileExists } from './fileUtils.js'; +import { execCommand, isCommandAvailable } from './shell-utils.js'; + +const RIPGREP_COMMAND = 'rg'; +const RIPGREP_BUFFER_LIMIT = 20_000_000; // Keep buffers aligned with the original bundle. +const RIPGREP_TEST_TIMEOUT_MS = 5_000; +const RIPGREP_RUN_TIMEOUT_MS = 10_000; +const RIPGREP_WSL_TIMEOUT_MS = 60_000; + +type RipgrepMode = 'builtin' | 'system'; + +interface RipgrepSelection { + mode: RipgrepMode; + command: string; +} + +interface RipgrepHealth { + working: boolean; + lastTested: number; + selection: RipgrepSelection; +} + +export interface RipgrepRunResult { + /** + * The stdout output from ripgrep + */ + stdout: string; + /** + * Whether the results were truncated due to buffer overflow or signal termination + */ + truncated: boolean; + /** + * Any error that occurred during execution (non-fatal errors like no matches won't populate this) + */ + error?: Error; +} + +let cachedSelection: RipgrepSelection | null = null; +let cachedHealth: RipgrepHealth | null = null; +let macSigningAttempted = false; + +function wslTimeout(): number { + return process.platform === 'linux' && process.env['WSL_INTEROP'] + ? RIPGREP_WSL_TIMEOUT_MS + : RIPGREP_RUN_TIMEOUT_MS; +} // Get the directory of the current module const __filename = fileURLToPath(import.meta.url); @@ -88,59 +134,201 @@ export function getBuiltinRipgrep(): string | null { return vendorPath; } -/** - * Checks if system ripgrep is available and returns the command to use - * @returns The ripgrep command ('rg' or 'rg.exe') if available, or null if not found - */ -export async function getSystemRipgrep(): Promise { - try { - const { spawn } = await import('node:child_process'); - const rgCommand = process.platform === 'win32' ? 'rg.exe' : 'rg'; - const isAvailable = await new Promise((resolve) => { - const proc = spawn(rgCommand, ['--version']); - proc.on('error', () => resolve(false)); - proc.on('exit', (code) => resolve(code === 0)); - }); - return isAvailable ? rgCommand : null; - } catch (_error) { - return null; - } -} - /** * Checks if ripgrep binary exists and returns its path * @param useBuiltin If true, tries bundled ripgrep first, then falls back to system ripgrep. * If false, only checks for system ripgrep. * @returns The path to ripgrep binary ('rg' or 'rg.exe' for system ripgrep, or full path for bundled), or null if not available + * @throws {Error} If an error occurs while resolving the ripgrep binary. */ -export async function getRipgrepCommand( +export async function resolveRipgrep( useBuiltin: boolean = true, -): Promise { - try { - if (useBuiltin) { - // Try bundled ripgrep first - const rgPath = getBuiltinRipgrep(); - if (rgPath && (await fileExists(rgPath))) { - return rgPath; - } - // Fallback to system rg if bundled binary is not available - } +): Promise { + if (cachedSelection) return cachedSelection; - // Check for system ripgrep - return await getSystemRipgrep(); - } catch (_error) { - return null; + if (useBuiltin) { + // Try bundled ripgrep first + const rgPath = getBuiltinRipgrep(); + if (rgPath && (await fileExists(rgPath))) { + cachedSelection = { mode: 'builtin', command: rgPath }; + return cachedSelection; + } + // Fallback to system rg if bundled binary is not available } + + const { available, error } = isCommandAvailable(RIPGREP_COMMAND); + if (available) { + cachedSelection = { mode: 'system', command: RIPGREP_COMMAND }; + return cachedSelection; + } + + if (error) { + throw error; + } + + return null; +} + +/** + * Ensures that ripgrep is healthy by checking its version. + * @param selection The ripgrep selection to check. + * @throws {Error} If ripgrep is not found or is not healthy. + */ +export async function ensureRipgrepHealthy( + selection: RipgrepSelection, +): Promise { + if ( + cachedHealth && + cachedHealth.selection.command === selection.command && + cachedHealth.working + ) + return; + + try { + const { stdout, code } = await execCommand( + selection.command, + ['--version'], + { + timeout: RIPGREP_TEST_TIMEOUT_MS, + }, + ); + const working = code === 0 && stdout.startsWith('ripgrep'); + cachedHealth = { working, lastTested: Date.now(), selection }; + } catch (error) { + cachedHealth = { working: false, lastTested: Date.now(), selection }; + throw error; + } +} + +export async function ensureMacBinarySigned( + selection: RipgrepSelection, +): Promise { + if (process.platform !== 'darwin') return; + if (macSigningAttempted) return; + macSigningAttempted = true; + + if (selection.mode !== 'builtin') return; + const binaryPath = selection.command; + + const inspect = await execCommand('codesign', ['-vv', '-d', binaryPath], { + preserveOutputOnError: false, + }); + const alreadySigned = + inspect.stdout + ?.split('\n') + .some((line) => line.includes('linker-signed')) ?? false; + if (!alreadySigned) return; + + await execCommand('codesign', [ + '--sign', + '-', + '--force', + '--preserve-metadata=entitlements,requirements,flags,runtime', + binaryPath, + ]); + await execCommand('xattr', ['-d', 'com.apple.quarantine', binaryPath]); } /** * Checks if ripgrep binary is available * @param useBuiltin If true, tries bundled ripgrep first, then falls back to system ripgrep. * If false, only checks for system ripgrep. + * @returns True if ripgrep is available, false otherwise. + * @throws {Error} If an error occurs while resolving the ripgrep binary. */ export async function canUseRipgrep( useBuiltin: boolean = true, ): Promise { - const rgPath = await getRipgrepCommand(useBuiltin); - return rgPath !== null; + const selection = await resolveRipgrep(useBuiltin); + if (!selection) { + return false; + } + await ensureRipgrepHealthy(selection); + return true; +} + +/** + * Runs ripgrep with the provided arguments + * @param args The arguments to pass to ripgrep + * @param signal The signal to abort the ripgrep process + * @returns The result of running ripgrep + * @throws {Error} If an error occurs while running ripgrep. + */ +export async function runRipgrep( + args: string[], + signal?: AbortSignal, +): Promise { + const selection = await resolveRipgrep(); + if (!selection) { + throw new Error('ripgrep not found.'); + } + await ensureRipgrepHealthy(selection); + + return new Promise((resolve) => { + const child = execFile( + selection.command, + args, + { + maxBuffer: RIPGREP_BUFFER_LIMIT, + timeout: wslTimeout(), + signal, + }, + (error, stdout = '', stderr = '') => { + if (!error) { + // Success case + resolve({ + stdout, + truncated: false, + }); + return; + } + + // Exit code 1 = no matches found (not an error) + // The error.code from execFile can be string | number | undefined | null + const errorCode = ( + error as Error & { code?: string | number | undefined | null } + ).code; + if (errorCode === 1) { + resolve({ stdout: '', truncated: false }); + return; + } + + // Detect various error conditions + const wasKilled = + error.signal === 'SIGTERM' || error.name === 'AbortError'; + const overflow = errorCode === 'ERR_CHILD_PROCESS_STDIO_MAXBUFFER'; + const syntaxError = errorCode === 2; + + const truncated = wasKilled || overflow; + let partialOutput = stdout; + + // If killed or overflow with partial output, remove the last potentially incomplete line + if (truncated && partialOutput.length > 0) { + const lines = partialOutput.split('\n'); + if (lines.length > 0) { + lines.pop(); + partialOutput = lines.join('\n'); + } + } + + // Log warnings for abnormal exits (except syntax errors) + if (!syntaxError && truncated) { + console.warn( + `ripgrep exited abnormally (signal=${error.signal} code=${error.code}) with stderr:\n${stderr.trim() || '(empty)'}`, + ); + } + + resolve({ + stdout: partialOutput, + truncated, + error: error instanceof Error ? error : undefined, + }); + }, + ); + + // Handle spawn errors + child.on('error', (err) => + resolve({ stdout: '', truncated: false, error: err }), + ); + }); } diff --git a/packages/core/src/utils/shell-utils.ts b/packages/core/src/utils/shell-utils.ts index 5a5128e3..6afab89d 100644 --- a/packages/core/src/utils/shell-utils.ts +++ b/packages/core/src/utils/shell-utils.ts @@ -10,7 +10,12 @@ import os from 'node:os'; import { quote } from 'shell-quote'; import { doesToolInvocationMatch } from './tool-utils.js'; import { isShellCommandReadOnly } from './shellReadOnlyChecker.js'; -import { spawn, type SpawnOptionsWithoutStdio } from 'node:child_process'; +import { + execFile, + execFileSync, + type ExecFileOptions, +} from 'node:child_process'; +import { accessSync, constants as fsConstants } from 'node:fs'; const SHELL_TOOL_NAMES = ['run_shell_command', 'ShellTool']; @@ -455,46 +460,101 @@ export function checkCommandPermissions( } /** - * Determines whether a given shell command is allowed to execute based on - * the tool's configuration including allowlists and blocklists. + * Executes a command with the given arguments without using a shell. * - * This function operates in "default allow" mode. It is a wrapper around - * `checkCommandPermissions`. + * This is a wrapper around Node.js's `execFile`, which spawns a process + * directly without invoking a shell, making it safer than `exec`. + * It's suitable for short-running commands with limited output. * - * @param command The shell command string to validate. - * @param config The application configuration. - * @returns An object with 'allowed' boolean and optional 'reason' string if not allowed. + * @param command The command to execute (e.g., 'git', 'osascript'). + * @param args Array of arguments to pass to the command. + * @param options Optional spawn options including: + * - preserveOutputOnError: If false (default), rejects on error. + * If true, resolves with output and error code. + * - Other standard spawn options (e.g., cwd, env). + * @returns A promise that resolves with stdout, stderr strings, and exit code. + * @throws Rejects with an error if the command fails (unless preserveOutputOnError is true). */ -export const spawnAsync = ( +export function execCommand( command: string, args: string[], - options?: SpawnOptionsWithoutStdio, -): Promise<{ stdout: string; stderr: string }> => - new Promise((resolve, reject) => { - const child = spawn(command, args, options); - let stdout = ''; - let stderr = ''; - - child.stdout.on('data', (data) => { - stdout += data.toString(); - }); - - child.stderr.on('data', (data) => { - stderr += data.toString(); - }); - - child.on('close', (code) => { - if (code === 0) { - resolve({ stdout, stderr }); - } else { - reject(new Error(`Command failed with exit code ${code}:\n${stderr}`)); - } - }); - - child.on('error', (err) => { - reject(err); - }); + options: { preserveOutputOnError?: boolean } & ExecFileOptions = {}, +): Promise<{ stdout: string; stderr: string; code: number }> { + return new Promise((resolve, reject) => { + const child = execFile( + command, + args, + { encoding: 'utf8', ...options }, + (error, stdout, stderr) => { + if (error) { + if (!options.preserveOutputOnError) { + reject(error); + } else { + resolve({ + stdout: stdout ?? '', + stderr: stderr ?? '', + code: typeof error.code === 'number' ? error.code : 1, + }); + } + return; + } + resolve({ stdout: stdout ?? '', stderr: stderr ?? '', code: 0 }); + }, + ); + child.on('error', reject); }); +} + +/** + * Resolves the path of a command in the system's PATH. + * @param {string} command The command name (e.g., 'git', 'grep'). + * @returns {path: string | null; error?: Error} The path of the command, or null if it is not found and any error that occurred. + */ +export function resolveCommandPath(command: string): { + path: string | null; + error?: Error; +} { + try { + const isWin = process.platform === 'win32'; + + const checkCommand = isWin ? 'where' : 'command'; + const checkArgs = isWin ? [command] : ['-v', command]; + + let result: string | null = null; + try { + result = execFileSync(checkCommand, checkArgs, { + encoding: 'utf8', + shell: isWin, + }).trim(); + } catch { + console.warn(`Command ${checkCommand} not found`); + } + + if (!result) return { path: null, error: undefined }; + if (!isWin) { + accessSync(result, fsConstants.X_OK); + } + return { path: result, error: undefined }; + } catch (error) { + return { + path: null, + error: error instanceof Error ? error : new Error(String(error)), + }; + } +} + +/** + * Checks if a command is available in the system's PATH. + * @param {string} command The command name (e.g., 'git', 'grep'). + * @returns {available: boolean; error?: Error} The availability of the command and any error that occurred. + */ +export function isCommandAvailable(command: string): { + available: boolean; + error?: Error; +} { + const { path, error } = resolveCommandPath(command); + return { available: path !== null, error }; +} export function isCommandAllowed( command: string,