diff --git a/docs/design/acp-channel-initialize-profiling.md b/docs/design/acp-channel-initialize-profiling.md index 4c7bb7652fc..af70ec68836 100644 --- a/docs/design/acp-channel-initialize-profiling.md +++ b/docs/design/acp-channel-initialize-profiling.md @@ -102,3 +102,66 @@ telemetry failure isolation, Config event ordering, and the serve fast-path bundle boundary. The release-built candidate is compared with the exact #6907 merge baseline on the representative 2C4G host with paired, alternating cold runs before any optimization is selected. + +## P0-B optimization decision + +The 2C4G P0-A profile attributed 67.3% of child startup P50 to Gemini and ACP +module loading. CPU profiles then showed that source-module compilation was the +largest CPU cost and that the ACP static import graph loaded Ink, React, React +Reconciler, and Yoga even though the ACP child does not render a TUI. + +The optional edges were existing UI-only dependencies rather than a new ACP +entry point. The ACP Session imported an API-error classifier through a React +hook; extension completion imported its data shape and result limit through a +render component; the command registry statically loaded UI support needed +only when `/init` asks for confirmation, approval mode enters auto mode, or +collapsed history expands. The optimization moves the two pure data helpers +out of render modules, makes the React type import type-only, and loads the +three interactive action dependencies only when those actions execute. + +The ACP initialize response, startup ordering, Config initialization, command +registry contents, failure handling, and Session behavior remain unchanged. A +bundle-metafile check follows the ACP agent's static output closure and rejects +Ink, React, React Reconciler, or Yoga inputs while continuing to allow them +behind dynamic imports. + +The causal comparison used release artifacts built from the same main commit, +`af6a9b640c5d9097c5151b8705dd73aee8e180d0`, with only this optimization +applied to the candidate. Two alternating cold runs produced 60 pairs after an +excluded warmup; a separate alternating preheated run produced 30 pairs. The +second cold run was started after the first run exposed two candidate-side +parent-listener stalls before the ACP path. No samples from either run were +discarded. The pooled cold P50 results were: + +| Metric | Matched control | P0-B candidate | Change | +| ------------------------- | --------------: | -------------: | -----------------: | +| ACP import | 115.06 ms | 52.00 ms | -63.06 ms (-54.8%) | +| Child process to response | 1102.88 ms | 1041.09 ms | -61.80 ms | +| `channel.initialize` | 1098.25 ms | 1035.61 ms | -62.64 ms | +| Process to first Session | 2046.88 ms | 1980.03 ms | -66.85 ms | +| Cold Session request | 1358.95 ms | 1290.23 ms | -68.72 ms | + +All 60 cold profiles in each variant and all 30 preheated profiles in each +variant were complete. Every run exited cleanly, and concurrent first Sessions, +telemetry-disabled startup, and legacy default `single` behavior succeeded in +both functional rounds. In the pooled cold data, warm-Session P95 changed from +137.53 ms to 104.98 ms, first-health P95 from 962.99 ms to 824.14 ms, and +process-tree RSS P95 from 442.27 MiB to 435.70 MiB. In the preheated data, +Session P50 changed from 73.90 ms to 73.75 ms and P95 from 88.38 ms to 76.17 ms. + +Transient host-wide stalls affected both variants and were retained. In the +first 30-pair run, two candidate parent-listener stalls raised first-health P95 +from 803.82 ms to 1175.67 ms even though the health requests themselves took +6-11 ms and the changed ACP path had not started. The diagnostic retry reversed +the direction, with control/candidate first-health P95 of 1522.44/727.64 ms; +pooling all 60 retained pairs produced the values above. The exact P0-A merge +was also compared with the candidate as a secondary 30-pair check and +independently showed the same ACP-import reduction and no P95 regression. + +The module-loading candidate therefore clears the P0-B gate: the selected +phase improves by more than 30% and 10 ms, while both `channel.initialize` and +process-to-first-Session P50 improve by more than 10 ms. Lazy top-level yargs +command builders were rejected because their selected-phase improvement did +not clear the 30% gate. Tool registry and warmup remain a separate descriptor +decoupling design; extension refresh, hierarchical memory, and transport were +too small to justify a P0 behavior change. diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 54642052af4..1d8b2ae0887 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -208,7 +208,7 @@ import { parseAcpModelOption, resolveAcpModelOption, } from '../../utils/acpModelUtils.js'; -import { classifyApiError } from '../../ui/hooks/useGeminiStream.js'; +import { classifyApiError } from '../../utils/classify-api-error.js'; import { getPersistScopeForModelSelection } from '../../config/modelProvidersScope.js'; import { writeStderrLine } from '../../utils/stdioHelpers.js'; import { diff --git a/packages/cli/src/ui/commands/approvalModeCommand.test.ts b/packages/cli/src/ui/commands/approvalModeCommand.test.ts index ebe78d6f053..73c56de4650 100644 --- a/packages/cli/src/ui/commands/approvalModeCommand.test.ts +++ b/packages/cli/src/ui/commands/approvalModeCommand.test.ts @@ -83,6 +83,20 @@ describe('approvalModeCommand', () => { expect(mockSetApprovalMode).toHaveBeenCalledWith('yolo'); }); + it('should emit the entry notice when switching to auto mode', async () => { + const result = (await approvalModeCommand.action?.( + mockContext, + 'auto', + )) as MessageActionReturn; + + expect(result.type).toBe('message'); + expect(mockSetApprovalMode).toHaveBeenCalledWith('auto'); + expect(mockContext.ui.addItem).toHaveBeenCalledWith( + expect.objectContaining({ type: 'info' }), + expect.any(Number), + ); + }); + it('should set approval mode to "auto-edit" when argument is "auto-edit"', async () => { const result = (await approvalModeCommand.action?.( mockContext, diff --git a/packages/cli/src/ui/commands/approvalModeCommand.ts b/packages/cli/src/ui/commands/approvalModeCommand.ts index 6ea1b77328a..e5aa44d1043 100644 --- a/packages/cli/src/ui/commands/approvalModeCommand.ts +++ b/packages/cli/src/ui/commands/approvalModeCommand.ts @@ -17,7 +17,6 @@ import { APPROVAL_MODES, ApprovalMode as ApprovalModeEnum, } from '@qwen-code/qwen-code-core'; -import { emitAutoModeEntryNotices } from '../hooks/useAutoAcceptIndicator.js'; import { formatApprovalModeName } from '../utils/approvalModeDisplay.js'; /** @@ -74,6 +73,24 @@ export const approvalModeCommand: SlashCommand = { if (config) { try { priorMode = config.getApprovalMode(); + } catch (e) { + return { + type: 'message', + messageType: 'error', + content: (e as Error).message, + }; + } + } + + const autoModeNotices = + mode === ApprovalModeEnum.AUTO && + priorMode !== ApprovalModeEnum.AUTO && + config + ? await import('../hooks/useAutoAcceptIndicator.js') + : undefined; + + if (config) { + try { config.setApprovalMode(mode); } catch (e) { return { @@ -87,12 +104,8 @@ export const approvalModeCommand: SlashCommand = { // When the user switches INTO AUTO via this command (not just via // Shift+Tab), emit the same first-time-acknowledgement + stripped-rules // notices as the keyboard handler. - if ( - mode === ApprovalModeEnum.AUTO && - priorMode !== ApprovalModeEnum.AUTO && - config - ) { - emitAutoModeEntryNotices({ + if (autoModeNotices && config) { + autoModeNotices.emitAutoModeEntryNotices({ config, settings, addItem: context.ui.addItem, diff --git a/packages/cli/src/ui/commands/historyCommand.ts b/packages/cli/src/ui/commands/historyCommand.ts index a519958601c..b6d13a4f1ad 100644 --- a/packages/cli/src/ui/commands/historyCommand.ts +++ b/packages/cli/src/ui/commands/historyCommand.ts @@ -8,7 +8,6 @@ import type { SlashCommand, MessageActionReturn } from './types.js'; import { CommandKind } from './types.js'; import { t } from '../../i18n/index.js'; import { SettingScope } from '../../config/settings.js'; -import { expandCollapsedHistory } from '../utils/resumeHistoryUtils.js'; const collapseOnResumeCommand: SlashCommand = { name: 'collapse-on-resume', @@ -61,7 +60,7 @@ const expandNowCommand: SlashCommand = { }, kind: CommandKind.BUILT_IN, supportedModes: ['interactive'] as const, - action: (context): MessageActionReturn | void => { + action: async (context): Promise => { const { history, loadHistory, refreshStatic } = context.ui; const hasSuppressed = history.some( @@ -77,6 +76,9 @@ const expandNowCommand: SlashCommand = { } // Remove suppressOnRestore from all items and drop collapse summary items. + const { expandCollapsedHistory } = await import( + '../utils/resumeHistoryUtils.js' + ); const updated = expandCollapsedHistory(history); loadHistory(updated); refreshStatic(); diff --git a/packages/cli/src/ui/commands/initCommand.test.ts b/packages/cli/src/ui/commands/initCommand.test.ts index 6dea22c4884..187c78ec790 100644 --- a/packages/cli/src/ui/commands/initCommand.test.ts +++ b/packages/cli/src/ui/commands/initCommand.test.ts @@ -7,6 +7,7 @@ import { vi, describe, it, expect, beforeEach, afterEach } from 'vitest'; import * as fs from 'node:fs'; import * as path from 'node:path'; +import React from 'react'; import { initCommand } from './initCommand.js'; import { createMockCommandContext } from '../../test-utils/mockCommandContext.js'; import { type CommandContext } from './types.js'; @@ -73,6 +74,23 @@ describe('initCommand', () => { expect(fs.writeFileSync).not.toHaveBeenCalled(); }); + it(`should preserve ${DEFAULT_CONTEXT_FILENAME} if the confirmation prompt cannot be built`, async () => { + vi.mocked(fs.existsSync).mockReturnValue(true); + vi.spyOn(fs, 'readFileSync').mockReturnValue('# Existing content'); + vi.spyOn(React, 'createElement').mockImplementationOnce(() => { + throw new Error('prompt unavailable'); + }); + + const result = await initCommand.action!(mockContext, ''); + + expect(result).toEqual({ + type: 'message', + messageType: 'error', + content: `Unexpected error preparing ${DEFAULT_CONTEXT_FILENAME}: prompt unavailable`, + }); + expect(fs.writeFileSync).not.toHaveBeenCalled(); + }); + it(`should create ${DEFAULT_CONTEXT_FILENAME} and submit a prompt if it does not exist`, async () => { // Arrange: Simulate that the file does not exist vi.mocked(fs.existsSync).mockReturnValue(false); diff --git a/packages/cli/src/ui/commands/initCommand.ts b/packages/cli/src/ui/commands/initCommand.ts index 15f89de82e2..de879c34582 100644 --- a/packages/cli/src/ui/commands/initCommand.ts +++ b/packages/cli/src/ui/commands/initCommand.ts @@ -13,8 +13,6 @@ import type { } from './types.js'; import { getCurrentGeminiMdFilename } from '@qwen-code/qwen-code-core'; import { CommandKind } from './types.js'; -import { Text } from 'ink'; -import React from 'react'; import { t } from '../../i18n/index.js'; export const initCommand: SlashCommand = { @@ -42,30 +40,33 @@ export const initCommand: SlashCommand = { try { if (fs.existsSync(contextFilePath)) { // If file exists but is empty (or whitespace), continue to initialize + let existing = ''; try { - const existing = fs.readFileSync(contextFilePath, 'utf8'); - if (existing && existing.trim().length > 0) { - // File exists and has content - ask for confirmation to overwrite - if (!context.overwriteConfirmed) { - return { - type: 'confirm_action', - // TODO: Move to .tsx file to use JSX syntax instead of React.createElement - // For now, using React.createElement to maintain .ts compatibility for PR review - prompt: React.createElement( - Text, - null, - `A ${contextFileName} file already exists in this directory. Do you want to regenerate it?`, - ), - originalInvocation: { - raw: context.invocation?.raw || '/init', - }, - }; - } - // User confirmed overwrite, continue with regeneration - } + existing = fs.readFileSync(contextFilePath, 'utf8'); } catch { // If we fail to read, conservatively proceed to (re)create the file } + if (existing && existing.trim().length > 0) { + // File exists and has content - ask for confirmation to overwrite + if (!context.overwriteConfirmed) { + const [{ Text }, { default: React }] = await Promise.all([ + import('ink'), + import('react'), + ]); + return { + type: 'confirm_action', + prompt: React.createElement( + Text, + null, + `A ${contextFileName} file already exists in this directory. Do you want to regenerate it?`, + ), + originalInvocation: { + raw: context.invocation?.raw || '/init', + }, + }; + } + // User confirmed overwrite, continue with regeneration + } } // Ensure an empty context file exists before prompting the model to populate it diff --git a/packages/cli/src/ui/components/SuggestionsDisplay.tsx b/packages/cli/src/ui/components/SuggestionsDisplay.tsx index 6d84e6c8a44..2cf9acac56b 100644 --- a/packages/cli/src/ui/components/SuggestionsDisplay.tsx +++ b/packages/cli/src/ui/components/SuggestionsDisplay.tsx @@ -9,40 +9,16 @@ import { Box, Text, type DOMElement } from 'ink'; import { theme } from '../semantic-colors.js'; import { RowMouseController } from './shared/RowMouseController.js'; import { PrepareLabel, MAX_WIDTH } from './PrepareLabel.js'; -import type { - CommandKind, - CommandSource, - ExecutionMode, -} from '../commands/types.js'; import { Colors } from '../colors.js'; import { t } from '../../i18n/index.js'; -export interface Suggestion { - label: string; - value: string; - description?: string; - matchedIndex?: number; - /** @deprecated Use source/sourceBadge instead. */ - commandKind?: CommandKind; - source?: CommandSource; - sourceLabel?: string; - sourceBadge?: string; - argumentHint?: string; - matchedAlias?: string; - supportedModes?: ExecutionMode[]; - modelInvocable?: boolean; - /** Whether the suggestion represents a directory path. When true, handleAutocomplete should NOT append a trailing space so the user can continue tab-completing deeper into the directory tree. */ - isDirectory?: boolean; - /** - * When true, the input layer should submit `/` immediately on - * Enter-accept rather than just inserting the suggestion text and - * waiting for a second Enter. Mirrors the `submitOnAccept` flag on the - * underlying SlashCommand (see `commands/types.ts`). Used for parent - * commands like `/skills` whose bare action just opens a dialog and - * takes no further argument — typing `/skil` should land in the - * dialog in one keystroke. - */ - submitOnAccept?: boolean; -} +import { + MAX_SUGGESTIONS_TO_SHOW, + type Suggestion, +} from '../utils/suggestions.js'; + +export { MAX_SUGGESTIONS_TO_SHOW } from '../utils/suggestions.js'; +export type { Suggestion } from '../utils/suggestions.js'; + interface SuggestionsDisplayProps { suggestions: Suggestion[]; activeIndex: number; @@ -60,7 +36,6 @@ interface SuggestionsDisplayProps { mouseEnabled?: boolean; } -export const MAX_SUGGESTIONS_TO_SHOW = 8; export { MAX_WIDTH }; /** diff --git a/packages/cli/src/ui/hooks/extension-mention-ref.ts b/packages/cli/src/ui/hooks/extension-mention-ref.ts index dd6bd6c08e2..1e69fb3ff2c 100644 --- a/packages/cli/src/ui/hooks/extension-mention-ref.ts +++ b/packages/cli/src/ui/hooks/extension-mention-ref.ts @@ -5,8 +5,10 @@ */ import type { Config } from '@qwen-code/qwen-code-core'; -import type { Suggestion } from '../components/SuggestionsDisplay.js'; -import { MAX_SUGGESTIONS_TO_SHOW } from '../components/SuggestionsDisplay.js'; +import { + MAX_SUGGESTIONS_TO_SHOW, + type Suggestion, +} from '../utils/suggestions.js'; import { t } from '../../i18n/index.js'; export { EXTENSION_REF_PREFIX, diff --git a/packages/cli/src/ui/hooks/useGeminiStream.test.tsx b/packages/cli/src/ui/hooks/useGeminiStream.test.tsx index 46ffb2ad590..61beb142c10 100644 --- a/packages/cli/src/ui/hooks/useGeminiStream.test.tsx +++ b/packages/cli/src/ui/hooks/useGeminiStream.test.tsx @@ -8,7 +8,7 @@ import type { Mock, MockInstance } from 'vitest'; import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { renderHook, act, waitFor } from '@testing-library/react'; -import { useGeminiStream, classifyApiError } from './useGeminiStream.js'; +import { useGeminiStream } from './useGeminiStream.js'; import * as atCommandProcessor from './atCommandProcessor.js'; import type { TrackedToolCall, @@ -9960,115 +9960,3 @@ describe('useGeminiStream', () => { }); }); }); - -describe('classifyApiError', () => { - it('should classify rate limit errors by status code 429', () => { - expect(classifyApiError({ message: 'error', status: 429 })).toBe( - 'rate_limit', - ); - }); - - it('should classify rate limit errors by message', () => { - expect(classifyApiError({ message: 'Rate limit exceeded' })).toBe( - 'rate_limit', - ); - }); - - it('should classify authentication errors by status code 401', () => { - expect(classifyApiError({ message: 'error', status: 401 })).toBe( - 'authentication_failed', - ); - }); - - it('should classify authentication errors by message', () => { - expect(classifyApiError({ message: 'Unauthorized access' })).toBe( - 'authentication_failed', - ); - }); - - it('should classify billing errors by status code 402', () => { - expect(classifyApiError({ message: 'error', status: 402 })).toBe( - 'billing_error', - ); - }); - - it('should classify billing errors by status code 403', () => { - expect(classifyApiError({ message: 'error', status: 403 })).toBe( - 'billing_error', - ); - }); - - it('should classify billing errors by message containing billing', () => { - expect(classifyApiError({ message: 'Billing issue detected' })).toBe( - 'billing_error', - ); - }); - - it('should classify billing errors by message containing quota', () => { - expect(classifyApiError({ message: 'Quota exceeded' })).toBe( - 'billing_error', - ); - }); - - it('should classify invalid request errors by status code 400', () => { - expect(classifyApiError({ message: 'error', status: 400 })).toBe( - 'invalid_request', - ); - }); - - it('should classify invalid request errors by message', () => { - expect(classifyApiError({ message: 'Invalid request format' })).toBe( - 'invalid_request', - ); - }); - - it('should classify server errors by status code 500', () => { - expect(classifyApiError({ message: 'error', status: 500 })).toBe( - 'server_error', - ); - }); - - it('should classify server errors by status code 502', () => { - expect(classifyApiError({ message: 'error', status: 502 })).toBe( - 'server_error', - ); - }); - - it('should classify server errors by status code 503', () => { - expect(classifyApiError({ message: 'error', status: 503 })).toBe( - 'server_error', - ); - }); - - it('should classify max output tokens errors by message', () => { - expect(classifyApiError({ message: 'max_tokens limit reached' })).toBe( - 'max_output_tokens', - ); - }); - - it('should classify token limit errors by message', () => { - expect(classifyApiError({ message: 'Token limit exceeded' })).toBe( - 'max_output_tokens', - ); - }); - - it('should return unknown for unrecognized errors', () => { - expect(classifyApiError({ message: 'Some random error' })).toBe('unknown'); - }); - - it('should return unknown for empty message', () => { - expect(classifyApiError({ message: '' })).toBe('unknown'); - }); - - it('should handle case insensitive matching', () => { - expect(classifyApiError({ message: 'RATE LIMIT exceeded' })).toBe( - 'rate_limit', - ); - expect(classifyApiError({ message: 'UNAUTHORIZED' })).toBe( - 'authentication_failed', - ); - expect(classifyApiError({ message: 'BILLING error' })).toBe( - 'billing_error', - ); - }); -}); diff --git a/packages/cli/src/ui/hooks/useGeminiStream.ts b/packages/cli/src/ui/hooks/useGeminiStream.ts index e19fc9e7387..2be5bb40dbf 100644 --- a/packages/cli/src/ui/hooks/useGeminiStream.ts +++ b/packages/cli/src/ui/hooks/useGeminiStream.ts @@ -25,7 +25,6 @@ import { type ThoughtSummary, type ToolCallRequestInfo, type GeminiErrorEventValue, - type StopFailureErrorType, type ActiveGoal, type SteerInput, GeminiEventType as ServerGeminiEventType, @@ -118,6 +117,7 @@ import { useDualOutput } from '../../dualOutput/DualOutputContext.js'; import { recordGoalStatusItem } from '../utils/restoreGoal.js'; import { sanitizeDisplayText } from '../../utils/extension-mention.js'; import process from 'node:process'; +import { classifyApiError } from '../../utils/classify-api-error.js'; const debugLogger = createDebugLogger('GEMINI_STREAM'); @@ -239,43 +239,6 @@ function extractToolResultText(parts: Part[] | Part | undefined): unknown { return chunks; } -/** - * Classify API error to StopFailureErrorType - * @internal Exported for testing purposes - */ -export function classifyApiError(error: { - message: string; - status?: number; -}): StopFailureErrorType { - const status = error.status; - const message = error.message?.toLowerCase() ?? ''; - - if (status === 429 || message.includes('rate limit')) { - return 'rate_limit'; - } - if (status === 401 || message.includes('unauthorized')) { - return 'authentication_failed'; - } - if ( - status === 402 || - status === 403 || - message.includes('billing') || - message.includes('quota') - ) { - return 'billing_error'; - } - if (status === 400 || message.includes('invalid')) { - return 'invalid_request'; - } - if (status !== undefined && status >= 500) { - return 'server_error'; - } - if (message.includes('max_tokens') || message.includes('token limit')) { - return 'max_output_tokens'; - } - return 'unknown'; -} - /** * Checks if image parts have supported formats and returns unsupported ones */ diff --git a/packages/cli/src/ui/types.ts b/packages/cli/src/ui/types.ts index 74afc5cc06c..ea5811e37f2 100644 --- a/packages/cli/src/ui/types.ts +++ b/packages/cli/src/ui/types.ts @@ -16,7 +16,7 @@ import type { ArenaDiffSummary, } from '@qwen-code/qwen-code-core'; import type { PartListUnion } from '@google/genai'; -import { type ReactNode } from 'react'; +import type { ReactNode } from 'react'; export type { ThoughtSummary }; diff --git a/packages/cli/src/ui/utils/suggestions.ts b/packages/cli/src/ui/utils/suggestions.ts new file mode 100644 index 00000000000..c38cc6c4ebd --- /dev/null +++ b/packages/cli/src/ui/utils/suggestions.ts @@ -0,0 +1,41 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { + CommandKind, + CommandSource, + ExecutionMode, +} from '../commands/types.js'; + +export interface Suggestion { + label: string; + value: string; + description?: string; + matchedIndex?: number; + /** @deprecated Use source/sourceBadge instead. */ + commandKind?: CommandKind; + source?: CommandSource; + sourceLabel?: string; + sourceBadge?: string; + argumentHint?: string; + matchedAlias?: string; + supportedModes?: ExecutionMode[]; + modelInvocable?: boolean; + /** Whether the suggestion represents a directory path. When true, handleAutocomplete should NOT append a trailing space so the user can continue tab-completing deeper into the directory tree. */ + isDirectory?: boolean; + /** + * When true, the input layer should submit `/` immediately on + * Enter-accept rather than just inserting the suggestion text and + * waiting for a second Enter. Mirrors the `submitOnAccept` flag on the + * underlying SlashCommand (see `commands/types.ts`). Used for parent + * commands like `/skills` whose bare action just opens a dialog and + * takes no further argument — typing `/skil` should land in the + * dialog in one keystroke. + */ + submitOnAccept?: boolean; +} + +export const MAX_SUGGESTIONS_TO_SHOW = 8; diff --git a/packages/cli/src/utils/classify-api-error.test.ts b/packages/cli/src/utils/classify-api-error.test.ts new file mode 100644 index 00000000000..f376b459166 --- /dev/null +++ b/packages/cli/src/utils/classify-api-error.test.ts @@ -0,0 +1,120 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, expect, it } from 'vitest'; +import { classifyApiError } from './classify-api-error.js'; + +describe('classifyApiError', () => { + it('should classify rate limit errors by status code 429', () => { + expect(classifyApiError({ message: 'error', status: 429 })).toBe( + 'rate_limit', + ); + }); + + it('should classify rate limit errors by message', () => { + expect(classifyApiError({ message: 'Rate limit exceeded' })).toBe( + 'rate_limit', + ); + }); + + it('should classify authentication errors by status code 401', () => { + expect(classifyApiError({ message: 'error', status: 401 })).toBe( + 'authentication_failed', + ); + }); + + it('should classify authentication errors by message', () => { + expect(classifyApiError({ message: 'Unauthorized access' })).toBe( + 'authentication_failed', + ); + }); + + it('should classify billing errors by status code 402', () => { + expect(classifyApiError({ message: 'error', status: 402 })).toBe( + 'billing_error', + ); + }); + + it('should classify billing errors by status code 403', () => { + expect(classifyApiError({ message: 'error', status: 403 })).toBe( + 'billing_error', + ); + }); + + it('should classify billing errors by message containing billing', () => { + expect(classifyApiError({ message: 'Billing issue detected' })).toBe( + 'billing_error', + ); + }); + + it('should classify billing errors by message containing quota', () => { + expect(classifyApiError({ message: 'Quota exceeded' })).toBe( + 'billing_error', + ); + }); + + it('should classify invalid request errors by status code 400', () => { + expect(classifyApiError({ message: 'error', status: 400 })).toBe( + 'invalid_request', + ); + }); + + it('should classify invalid request errors by message', () => { + expect(classifyApiError({ message: 'Invalid request format' })).toBe( + 'invalid_request', + ); + }); + + it('should classify server errors by status code 500', () => { + expect(classifyApiError({ message: 'error', status: 500 })).toBe( + 'server_error', + ); + }); + + it('should classify server errors by status code 502', () => { + expect(classifyApiError({ message: 'error', status: 502 })).toBe( + 'server_error', + ); + }); + + it('should classify server errors by status code 503', () => { + expect(classifyApiError({ message: 'error', status: 503 })).toBe( + 'server_error', + ); + }); + + it('should classify max output tokens errors by message', () => { + expect(classifyApiError({ message: 'max_tokens limit reached' })).toBe( + 'max_output_tokens', + ); + }); + + it('should classify token limit errors by message', () => { + expect(classifyApiError({ message: 'Token limit exceeded' })).toBe( + 'max_output_tokens', + ); + }); + + it('should return unknown for unrecognized errors', () => { + expect(classifyApiError({ message: 'Some random error' })).toBe('unknown'); + }); + + it('should return unknown for empty message', () => { + expect(classifyApiError({ message: '' })).toBe('unknown'); + }); + + it('should handle case insensitive matching', () => { + expect(classifyApiError({ message: 'RATE LIMIT exceeded' })).toBe( + 'rate_limit', + ); + expect(classifyApiError({ message: 'UNAUTHORIZED' })).toBe( + 'authentication_failed', + ); + expect(classifyApiError({ message: 'BILLING error' })).toBe( + 'billing_error', + ); + }); +}); diff --git a/packages/cli/src/utils/classify-api-error.ts b/packages/cli/src/utils/classify-api-error.ts new file mode 100644 index 00000000000..1c349c2eab3 --- /dev/null +++ b/packages/cli/src/utils/classify-api-error.ts @@ -0,0 +1,40 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { StopFailureErrorType } from '@qwen-code/qwen-code-core'; + +export function classifyApiError(error: { + message: string; + status?: number; +}): StopFailureErrorType { + const status = error.status; + const message = error.message?.toLowerCase() ?? ''; + + if (status === 429 || message.includes('rate limit')) { + return 'rate_limit'; + } + if (status === 401 || message.includes('unauthorized')) { + return 'authentication_failed'; + } + if ( + status === 402 || + status === 403 || + message.includes('billing') || + message.includes('quota') + ) { + return 'billing_error'; + } + if (status === 400 || message.includes('invalid')) { + return 'invalid_request'; + } + if (status !== undefined && status >= 500) { + return 'server_error'; + } + if (message.includes('max_tokens') || message.includes('token limit')) { + return 'max_output_tokens'; + } + return 'unknown'; +} diff --git a/scripts/check-serve-fast-path-bundle.js b/scripts/check-serve-fast-path-bundle.js index 949a488ea4a..30f4f188f02 100644 --- a/scripts/check-serve-fast-path-bundle.js +++ b/scripts/check-serve-fast-path-bundle.js @@ -36,6 +36,14 @@ const SERVE_PRE_LISTEN_ROOTS = [ }, ]; +const ACP_RUNTIME_ROOT = { + label: 'ACP agent runtime', + suffixes: [ + 'packages/cli/src/acp-integration/acpAgent.ts', + 'packages/cli/dist/src/acp-integration/acpAgent.js', + ], +}; + const FORBIDDEN_SOURCE_INPUTS = [ { label: 'Gemini runtime', @@ -116,6 +124,13 @@ const FORBIDDEN_VENDOR_PACKAGES = [ { label: 'fzf vendor package', packageName: 'fzf' }, ]; +const FORBIDDEN_ACP_UI_PACKAGES = [ + { label: 'Ink TUI runtime', packageName: 'ink' }, + { label: 'React runtime', packageName: 'react' }, + { label: 'React reconciler runtime', packageName: 'react-reconciler' }, + { label: 'Yoga layout runtime', packageName: 'yoga-layout' }, +]; + export function normalizeMetafilePath(filePath) { return filePath.replace(/\\/g, '/').replace(/^\.\//, ''); } @@ -219,6 +234,59 @@ function buildImportPath(entryOutputs, outputPath, parent) { return reversed.reverse(); } +export function findAcpImportBoundaryOffenders(metafile) { + const outputs = normalizeOutputs(metafile); + let entryOutput; + + for (const [outputPath, output] of outputs) { + const inputs = Object.keys(output.inputs ?? {}); + if ( + inputs.some((input) => + inputMatchesAnySuffix(input, ACP_RUNTIME_ROOT.suffixes), + ) + ) { + entryOutput = outputPath; + break; + } + } + + if (!entryOutput) { + throw new Error( + `Could not find bundled output for ${ACP_RUNTIME_ROOT.label} ` + + `(${ACP_RUNTIME_ROOT.suffixes.join(' or ')}).\n` + + `Run \`${METAFILE_BUILD_COMMAND}\` to produce the metafile.`, + ); + } + + const entryOutputs = [entryOutput]; + const { closure, parent } = collectStaticClosure(outputs, entryOutputs); + const offenders = []; + const seen = new Set(); + + for (const outputPath of closure) { + const output = outputs.get(outputPath); + for (const input of Object.keys(output?.inputs ?? {})) { + const match = FORBIDDEN_ACP_UI_PACKAGES.find(({ packageName }) => + inputMatchesPackage(input, packageName), + ); + if (!match) continue; + const key = `${match.label}\0${outputPath}`; + if (seen.has(key)) continue; + seen.add(key); + + offenders.push({ + label: match.label, + matchedInput: normalizeMetafilePath(input), + outputPath, + bytes: output?.bytes ?? 0, + importPath: buildImportPath(entryOutputs, outputPath, parent), + }); + } + } + + return offenders; +} + export function findServeFastPathBundleOffenders(metafile) { const outputs = normalizeOutputs(metafile); const entryOutputs = findServePreListenRootOutputs(outputs); @@ -309,19 +377,54 @@ export function checkServeFastPathBundle({ return { ok: offenders.length === 0, offenders }; } +export function checkAcpImportBoundary({ + metafilePath = DEFAULT_METAFILE_PATH, +} = {}) { + if (!existsSync(metafilePath)) { + throw new Error( + `Missing esbuild metafile at ${metafilePath}. ` + + `Run \`${METAFILE_BUILD_COMMAND}\` to produce it.`, + ); + } + + let metafile; + try { + metafile = JSON.parse(readFileSync(metafilePath, 'utf8')); + } catch (error) { + const reason = error instanceof Error ? error.message : String(error); + throw new Error( + `Invalid esbuild metafile at ${metafilePath}: ${reason}. ` + + `Run \`${METAFILE_BUILD_COMMAND}\` to regenerate it.`, + ); + } + + const offenders = findAcpImportBoundaryOffenders(metafile); + return { ok: offenders.length === 0, offenders }; +} + function main() { try { - const result = checkServeFastPathBundle(); - if (result.ok) { - console.log('Serve fast-path bundle closure check passed.'); - return; + const serveResult = checkServeFastPathBundle(); + if (!serveResult.ok) { + console.error( + 'Serve fast-path bundle closure includes pre-listen runtime modules:\n' + + formatServeFastPathBundleOffenders(serveResult.offenders), + ); + process.exitCode = 1; } - console.error( - 'Serve fast-path bundle closure includes pre-listen runtime modules:\n' + - formatServeFastPathBundleOffenders(result.offenders), - ); - process.exitCode = 1; + const acpResult = checkAcpImportBoundary(); + if (!acpResult.ok) { + console.error( + 'ACP static import closure includes TUI runtime modules:\n' + + formatServeFastPathBundleOffenders(acpResult.offenders), + ); + process.exitCode = 1; + } + + if (serveResult.ok && acpResult.ok) { + console.log('Startup bundle closure checks passed.'); + } } catch (error) { console.error(error instanceof Error ? error.message : String(error)); process.exitCode = 1; diff --git a/scripts/tests/serve-fast-path-bundle-check.test.js b/scripts/tests/serve-fast-path-bundle-check.test.js index 2d18b423e27..ff05de558e3 100644 --- a/scripts/tests/serve-fast-path-bundle-check.test.js +++ b/scripts/tests/serve-fast-path-bundle-check.test.js @@ -11,7 +11,9 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { fileURLToPath } from 'node:url'; import { + checkAcpImportBoundary, checkServeFastPathBundle, + findAcpImportBoundaryOffenders, findServeFastPathBundleOffenders, formatServeFastPathBundleOffenders, normalizeMetafilePath, @@ -33,6 +35,9 @@ function makeMetafile(outputs) { 'dist/chunks/run-qwen-serve.js': output({ inputs: ['packages/cli/src/serve/run-qwen-serve.ts'], }), + 'dist/chunks/acp-agent.js': output({ + inputs: ['packages/cli/src/acp-integration/acpAgent.ts'], + }), ...outputs, }, }; @@ -416,3 +421,75 @@ describe('serve fast-path bundle check', () => { } }); }); + +describe('ACP import boundary check', () => { + it('reports TUI packages reached through static imports', () => { + const metafile = makeMetafile({ + 'dist/chunks/acp-agent.js': output({ + inputs: ['packages/cli/src/acp-integration/acpAgent.ts'], + imports: [staticImport('dist/chunks/tui.js')], + }), + 'dist/chunks/tui.js': output({ + bytes: 250_000, + inputs: [ + 'node_modules/ink/build/index.js', + 'node_modules/react/index.js', + 'node_modules/react-reconciler/index.js', + 'node_modules/yoga-layout/dist/src/index.js', + ], + }), + }); + + expect(findAcpImportBoundaryOffenders(metafile)).toEqual( + expect.arrayContaining([ + expect.objectContaining({ label: 'Ink TUI runtime' }), + expect.objectContaining({ label: 'React runtime' }), + expect.objectContaining({ label: 'React reconciler runtime' }), + expect.objectContaining({ label: 'Yoga layout runtime' }), + ]), + ); + }); + + it('allows TUI packages behind dynamic imports', () => { + const metafile = makeMetafile({ + 'dist/chunks/acp-agent.js': output({ + inputs: ['packages/cli/src/acp-integration/acpAgent.ts'], + imports: [dynamicImport('dist/chunks/tui.js')], + }), + 'dist/chunks/tui.js': output({ + inputs: ['node_modules/ink/build/index.js'], + }), + }); + + expect(findAcpImportBoundaryOffenders(metafile)).toEqual([]); + }); + + it('reads a metafile path and returns ACP boundary offenders', () => { + const tempDir = mkdtempSync(join(tmpdir(), 'acp-import-boundary-')); + try { + const metafilePath = writeMetafile( + tempDir, + makeMetafile({ + 'dist/chunks/acp-agent.js': output({ + inputs: [ + 'packages/cli/src/acp-integration/acpAgent.ts', + 'node_modules/ink/build/index.js', + ], + }), + }), + ); + + expect(checkAcpImportBoundary({ metafilePath })).toEqual({ + ok: false, + offenders: [ + expect.objectContaining({ + label: 'Ink TUI runtime', + matchedInput: 'node_modules/ink/build/index.js', + }), + ], + }); + } finally { + rmSync(tempDir, { recursive: true, force: true }); + } + }); +});