diff --git a/.gitignore b/.gitignore index 2484d9e8..2c3156b9 100644 --- a/.gitignore +++ b/.gitignore @@ -57,4 +57,3 @@ gha-creds-*.json # Log files patch_output.log -QWEN.md diff --git a/integration-tests/json-output.test.ts b/integration-tests/json-output.test.ts index 9c6bce18..8221aa5b 100644 --- a/integration-tests/json-output.test.ts +++ b/integration-tests/json-output.test.ts @@ -74,35 +74,236 @@ describe('JSON output', () => { expect(thrown).toBeDefined(); const message = (thrown as Error).message; - // The error JSON is written to stderr, so it should be in the error message - // Use a regex to find the first complete JSON object in the string - const jsonMatch = message.match(/{[\s\S]*}/); - - // Fail if no JSON-like text was found + // The error JSON is written to stdout as a CLIResultMessageError + // Extract stdout from the error message + const stdoutMatch = message.match(/Stdout:\n([\s\S]*?)(?:\n\nStderr:|$)/); expect( - jsonMatch, - 'Expected to find a JSON object in the error output', + stdoutMatch, + 'Expected to find stdout in the error message', ).toBeTruthy(); - let payload; + const stdout = stdoutMatch![1]; + let parsed: unknown[]; try { - // Parse the matched JSON string - payload = JSON.parse(jsonMatch![0]); + // Parse the JSON array from stdout + parsed = JSON.parse(stdout); } catch (parseError) { - console.error('Failed to parse the following JSON:', jsonMatch![0]); + console.error('Failed to parse the following JSON:', stdout); throw new Error( - `Test failed: Could not parse JSON from error message. Details: ${parseError}`, + `Test failed: Could not parse JSON from stdout. Details: ${parseError}`, ); } - // The JsonFormatter.formatError() outputs: { error: { type, message, code } } - expect(payload).toHaveProperty('error'); - expect(payload.error).toBeDefined(); - expect(payload.error.type).toBe('Error'); - expect(payload.error.code).toBe(1); - expect(payload.error.message).toContain( + // The output should be an array of messages + expect(Array.isArray(parsed)).toBe(true); + expect(parsed.length).toBeGreaterThan(0); + + // Find the result message with error + const resultMessage = parsed.find( + (msg: unknown) => + typeof msg === 'object' && + msg !== null && + 'type' in msg && + msg.type === 'result' && + 'is_error' in msg && + msg.is_error === true, + ) as { + type: string; + is_error: boolean; + subtype: string; + error?: { message: string; type?: string }; + }; + + expect(resultMessage).toBeDefined(); + expect(resultMessage.is_error).toBe(true); + expect(resultMessage).toHaveProperty('subtype'); + expect(resultMessage.subtype).toBe('error_during_execution'); + expect(resultMessage).toHaveProperty('error'); + expect(resultMessage.error).toBeDefined(); + expect(resultMessage.error?.message).toContain( 'configured auth type is qwen-oauth', ); - expect(payload.error.message).toContain('current auth type is openai'); + expect(resultMessage.error?.message).toContain( + 'current auth type is openai', + ); + }); + + it('should return line-delimited JSON messages for stream-json output format', async () => { + const result = await rig.run( + 'What is the capital of France?', + '--output-format', + 'stream-json', + ); + + // Stream-json output is line-delimited JSON (one JSON object per line) + const lines = result + .trim() + .split('\n') + .filter((line) => line.trim()); + expect(lines.length).toBeGreaterThan(0); + + // Parse each line as a JSON object + const messages: unknown[] = []; + for (const line of lines) { + try { + const parsed = JSON.parse(line); + messages.push(parsed); + } catch (parseError) { + throw new Error( + `Failed to parse JSON line: ${line}. Error: ${parseError}`, + ); + } + } + + // Should have at least system, assistant, and result messages + expect(messages.length).toBeGreaterThanOrEqual(3); + + // Find system message + const systemMessage = messages.find( + (msg: unknown) => + typeof msg === 'object' && + msg !== null && + 'type' in msg && + msg.type === 'system', + ); + expect(systemMessage).toBeDefined(); + expect(systemMessage).toHaveProperty('subtype'); + expect(systemMessage).toHaveProperty('session_id'); + + // Find assistant message + const assistantMessage = messages.find( + (msg: unknown) => + typeof msg === 'object' && + msg !== null && + 'type' in msg && + msg.type === 'assistant', + ); + expect(assistantMessage).toBeDefined(); + expect(assistantMessage).toHaveProperty('message'); + expect(assistantMessage).toHaveProperty('session_id'); + + // Find result message (should be the last message) + const resultMessage = messages[messages.length - 1] as { + type: string; + is_error: boolean; + result: string; + }; + expect(resultMessage).toBeDefined(); + expect( + typeof resultMessage === 'object' && + resultMessage !== null && + 'type' in resultMessage && + resultMessage.type === 'result', + ).toBe(true); + expect(resultMessage).toHaveProperty('is_error'); + expect(resultMessage.is_error).toBe(false); + expect(resultMessage).toHaveProperty('result'); + expect(typeof resultMessage.result).toBe('string'); + expect(resultMessage.result.toLowerCase()).toContain('paris'); + }); + + it('should include stream events when using stream-json with include-partial-messages', async () => { + const result = await rig.run( + 'What is the capital of France?', + '--output-format', + 'stream-json', + '--include-partial-messages', + ); + + // Stream-json output is line-delimited JSON (one JSON object per line) + const lines = result + .trim() + .split('\n') + .filter((line) => line.trim()); + expect(lines.length).toBeGreaterThan(0); + + // Parse each line as a JSON object + const messages: unknown[] = []; + for (const line of lines) { + try { + const parsed = JSON.parse(line); + messages.push(parsed); + } catch (parseError) { + throw new Error( + `Failed to parse JSON line: ${line}. Error: ${parseError}`, + ); + } + } + + // Should have more messages than without include-partial-messages + // because we're including stream events + expect(messages.length).toBeGreaterThan(3); + + // Find stream_event messages + const streamEvents = messages.filter( + (msg: unknown) => + typeof msg === 'object' && + msg !== null && + 'type' in msg && + msg.type === 'stream_event', + ); + expect(streamEvents.length).toBeGreaterThan(0); + + // Verify stream event structure + const firstStreamEvent = streamEvents[0]; + expect(firstStreamEvent).toHaveProperty('event'); + expect(firstStreamEvent).toHaveProperty('session_id'); + expect(firstStreamEvent).toHaveProperty('uuid'); + + // Check for expected stream event types + const eventTypes = streamEvents.map((event: unknown) => + typeof event === 'object' && + event !== null && + 'event' in event && + typeof event.event === 'object' && + event.event !== null && + 'type' in event.event + ? event.event.type + : null, + ); + + // Should have message_start event + expect(eventTypes).toContain('message_start'); + + // Should have content_block_start event + expect(eventTypes).toContain('content_block_start'); + + // Should have content_block_delta events + expect(eventTypes).toContain('content_block_delta'); + + // Should have content_block_stop event + expect(eventTypes).toContain('content_block_stop'); + + // Should have message_stop event + expect(eventTypes).toContain('message_stop'); + + // Verify that we still have the complete assistant message + const assistantMessage = messages.find( + (msg: unknown) => + typeof msg === 'object' && + msg !== null && + 'type' in msg && + msg.type === 'assistant', + ); + expect(assistantMessage).toBeDefined(); + expect(assistantMessage).toHaveProperty('message'); + + // Verify that we still have the result message + const resultMessage = messages[messages.length - 1] as { + type: string; + is_error: boolean; + result: string; + }; + expect(resultMessage).toBeDefined(); + expect( + typeof resultMessage === 'object' && + resultMessage !== null && + 'type' in resultMessage && + resultMessage.type === 'result', + ).toBe(true); + expect(resultMessage).toHaveProperty('is_error'); + expect(resultMessage.is_error).toBe(false); + expect(resultMessage).toHaveProperty('result'); + expect(resultMessage.result.toLowerCase()).toContain('paris'); }); }); diff --git a/integration-tests/test-helper.ts b/integration-tests/test-helper.ts index a1eb15c8..0fe658c5 100644 --- a/integration-tests/test-helper.ts +++ b/integration-tests/test-helper.ts @@ -340,7 +340,8 @@ export class TestRig { // as it would corrupt the JSON const isJsonOutput = commandArgs.includes('--output-format') && - commandArgs.includes('json'); + (commandArgs.includes('json') || + commandArgs.includes('stream-json')); // If we have stderr output and it's not a JSON test, include that also if (stderr && !isJsonOutput) { @@ -349,7 +350,23 @@ export class TestRig { resolve(result); } else { - reject(new Error(`Process exited with code ${code}:\n${stderr}`)); + // Check if this is a JSON output test - for JSON errors, the error is in stdout + const isJsonOutputOnError = + commandArgs.includes('--output-format') && + (commandArgs.includes('json') || + commandArgs.includes('stream-json')); + + // For JSON output tests, include stdout in the error message + // as the error JSON is written to stdout + if (isJsonOutputOnError && stdout) { + reject( + new Error( + `Process exited with code ${code}:\nStdout:\n${stdout}\n\nStderr:\n${stderr}`, + ), + ); + } else { + reject(new Error(`Process exited with code ${code}:\n${stderr}`)); + } } }); }); diff --git a/packages/cli/package.json b/packages/cli/package.json index a38f1d61..9c8d02b4 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -16,10 +16,6 @@ ".": { "types": "./dist/index.d.ts", "import": "./dist/index.js" - }, - "./protocol": { - "types": "./dist/src/types/protocol.d.ts", - "import": "./dist/src/types/protocol.js" } }, "scripts": { diff --git a/packages/cli/src/validateNonInterActiveAuth.test.ts b/packages/cli/src/validateNonInterActiveAuth.test.ts index dba93e62..867777b3 100644 --- a/packages/cli/src/validateNonInterActiveAuth.test.ts +++ b/packages/cli/src/validateNonInterActiveAuth.test.ts @@ -10,6 +10,9 @@ import { AuthType, OutputFormat } from '@qwen-code/qwen-code-core'; import type { Config } from '@qwen-code/qwen-code-core'; import * as auth from './config/auth.js'; import { type LoadedSettings } from './config/settings.js'; +import * as JsonOutputAdapterModule from './nonInteractive/io/JsonOutputAdapter.js'; +import * as StreamJsonOutputAdapterModule from './nonInteractive/io/StreamJsonOutputAdapter.js'; +import * as cleanupModule from './utils/cleanup.js'; describe('validateNonInterActiveAuth', () => { let originalEnvGeminiApiKey: string | undefined; @@ -17,8 +20,8 @@ describe('validateNonInterActiveAuth', () => { let originalEnvGcp: string | undefined; let originalEnvOpenAiApiKey: string | undefined; let consoleErrorSpy: ReturnType; - let processExitSpy: ReturnType; - let refreshAuthMock: vi.Mock; + let processExitSpy: ReturnType>; + let refreshAuthMock: ReturnType; let mockSettings: LoadedSettings; beforeEach(() => { @@ -33,7 +36,7 @@ describe('validateNonInterActiveAuth', () => { consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); processExitSpy = vi.spyOn(process, 'exit').mockImplementation((code) => { throw new Error(`process.exit(${code}) called`); - }); + }) as ReturnType>; refreshAuthMock = vi.fn().mockResolvedValue('refreshed'); mockSettings = { system: { path: '', settings: {} }, @@ -235,7 +238,24 @@ describe('validateNonInterActiveAuth', () => { }); describe('JSON output mode', () => { - it('prints JSON error when no auth is configured and exits with code 1', async () => { + let emitResultMock: ReturnType; + let runExitCleanupMock: ReturnType; + + beforeEach(() => { + emitResultMock = vi.fn(); + runExitCleanupMock = vi.fn().mockResolvedValue(undefined); + vi.spyOn(JsonOutputAdapterModule, 'JsonOutputAdapter').mockImplementation( + () => + ({ + emitResult: emitResultMock, + }) as unknown as JsonOutputAdapterModule.JsonOutputAdapter, + ); + vi.spyOn(cleanupModule, 'runExitCleanup').mockImplementation( + runExitCleanupMock, + ); + }); + + it('emits error result and exits when no auth is configured', async () => { const nonInteractiveConfig = { refreshAuth: refreshAuthMock, getOutputFormat: vi.fn().mockReturnValue(OutputFormat.JSON), @@ -244,7 +264,6 @@ describe('validateNonInterActiveAuth', () => { .mockReturnValue({ authType: undefined }), } as unknown as Config; - let thrown: Error | undefined; try { await validateNonInteractiveAuth( undefined, @@ -252,21 +271,27 @@ describe('validateNonInterActiveAuth', () => { nonInteractiveConfig, mockSettings, ); + expect.fail('Should have exited'); } catch (e) { - thrown = e as Error; + expect((e as Error).message).toContain('process.exit(1) called'); } - expect(thrown?.message).toBe('process.exit(1) called'); - const errorArg = consoleErrorSpy.mock.calls[0]?.[0] as string; - const payload = JSON.parse(errorArg); - expect(payload.error.type).toBe('Error'); - expect(payload.error.code).toBe(1); - expect(payload.error.message).toContain( - 'Please set an Auth method in your', - ); + expect(emitResultMock).toHaveBeenCalledWith({ + isError: true, + errorMessage: expect.stringContaining( + 'Please set an Auth method in your', + ), + durationMs: 0, + apiDurationMs: 0, + numTurns: 0, + usage: undefined, + }); + expect(runExitCleanupMock).toHaveBeenCalled(); + expect(processExitSpy).toHaveBeenCalledWith(1); + expect(consoleErrorSpy).not.toHaveBeenCalled(); }); - it('prints JSON error when enforced auth mismatches current auth and exits with code 1', async () => { + it('emits error result and exits when enforced auth mismatches current auth', async () => { mockSettings.merged.security!.auth!.enforcedType = AuthType.QWEN_OAUTH; process.env['OPENAI_API_KEY'] = 'fake-key'; @@ -278,7 +303,6 @@ describe('validateNonInterActiveAuth', () => { .mockReturnValue({ authType: undefined }), } as unknown as Config; - let thrown: Error | undefined; try { await validateNonInteractiveAuth( undefined, @@ -286,23 +310,27 @@ describe('validateNonInterActiveAuth', () => { nonInteractiveConfig, mockSettings, ); + expect.fail('Should have exited'); } catch (e) { - thrown = e as Error; + expect((e as Error).message).toContain('process.exit(1) called'); } - expect(thrown?.message).toBe('process.exit(1) called'); - { - const errorArg = consoleErrorSpy.mock.calls[0]?.[0] as string; - const payload = JSON.parse(errorArg); - expect(payload.error.type).toBe('Error'); - expect(payload.error.code).toBe(1); - expect(payload.error.message).toContain( + expect(emitResultMock).toHaveBeenCalledWith({ + isError: true, + errorMessage: expect.stringContaining( 'The configured auth type is qwen-oauth, but the current auth type is openai.', - ); - } + ), + durationMs: 0, + apiDurationMs: 0, + numTurns: 0, + usage: undefined, + }); + expect(runExitCleanupMock).toHaveBeenCalled(); + expect(processExitSpy).toHaveBeenCalledWith(1); + expect(consoleErrorSpy).not.toHaveBeenCalled(); }); - it('prints JSON error when validateAuthMethod fails and exits with code 1', async () => { + it('emits error result and exits when validateAuthMethod fails', async () => { vi.spyOn(auth, 'validateAuthMethod').mockReturnValue('Auth error!'); process.env['OPENAI_API_KEY'] = 'fake-key'; @@ -314,7 +342,6 @@ describe('validateNonInterActiveAuth', () => { .mockReturnValue({ authType: undefined }), } as unknown as Config; - let thrown: Error | undefined; try { await validateNonInteractiveAuth( AuthType.USE_OPENAI, @@ -322,18 +349,159 @@ describe('validateNonInterActiveAuth', () => { nonInteractiveConfig, mockSettings, ); + expect.fail('Should have exited'); } catch (e) { - thrown = e as Error; + expect((e as Error).message).toContain('process.exit(1) called'); } - expect(thrown?.message).toBe('process.exit(1) called'); - { - const errorArg = consoleErrorSpy.mock.calls[0]?.[0] as string; - const payload = JSON.parse(errorArg); - expect(payload.error.type).toBe('Error'); - expect(payload.error.code).toBe(1); - expect(payload.error.message).toBe('Auth error!'); + expect(emitResultMock).toHaveBeenCalledWith({ + isError: true, + errorMessage: 'Auth error!', + durationMs: 0, + apiDurationMs: 0, + numTurns: 0, + usage: undefined, + }); + expect(runExitCleanupMock).toHaveBeenCalled(); + expect(processExitSpy).toHaveBeenCalledWith(1); + expect(consoleErrorSpy).not.toHaveBeenCalled(); + }); + }); + + describe('STREAM_JSON output mode', () => { + let emitResultMock: ReturnType; + let runExitCleanupMock: ReturnType; + + beforeEach(() => { + emitResultMock = vi.fn(); + runExitCleanupMock = vi.fn().mockResolvedValue(undefined); + vi.spyOn( + StreamJsonOutputAdapterModule, + 'StreamJsonOutputAdapter', + ).mockImplementation( + () => + ({ + emitResult: emitResultMock, + }) as unknown as StreamJsonOutputAdapterModule.StreamJsonOutputAdapter, + ); + vi.spyOn(cleanupModule, 'runExitCleanup').mockImplementation( + runExitCleanupMock, + ); + }); + + it('emits error result and exits when no auth is configured', async () => { + const nonInteractiveConfig = { + refreshAuth: refreshAuthMock, + getOutputFormat: vi.fn().mockReturnValue(OutputFormat.STREAM_JSON), + getIncludePartialMessages: vi.fn().mockReturnValue(false), + getContentGeneratorConfig: vi + .fn() + .mockReturnValue({ authType: undefined }), + } as unknown as Config; + + try { + await validateNonInteractiveAuth( + undefined, + undefined, + nonInteractiveConfig, + mockSettings, + ); + expect.fail('Should have exited'); + } catch (e) { + expect((e as Error).message).toContain('process.exit(1) called'); } + + expect(emitResultMock).toHaveBeenCalledWith({ + isError: true, + errorMessage: expect.stringContaining( + 'Please set an Auth method in your', + ), + durationMs: 0, + apiDurationMs: 0, + numTurns: 0, + usage: undefined, + }); + expect(runExitCleanupMock).toHaveBeenCalled(); + expect(processExitSpy).toHaveBeenCalledWith(1); + expect(consoleErrorSpy).not.toHaveBeenCalled(); + }); + + it('emits error result and exits when enforced auth mismatches current auth', async () => { + mockSettings.merged.security!.auth!.enforcedType = AuthType.QWEN_OAUTH; + process.env['OPENAI_API_KEY'] = 'fake-key'; + + const nonInteractiveConfig = { + refreshAuth: refreshAuthMock, + getOutputFormat: vi.fn().mockReturnValue(OutputFormat.STREAM_JSON), + getIncludePartialMessages: vi.fn().mockReturnValue(false), + getContentGeneratorConfig: vi + .fn() + .mockReturnValue({ authType: undefined }), + } as unknown as Config; + + try { + await validateNonInteractiveAuth( + undefined, + undefined, + nonInteractiveConfig, + mockSettings, + ); + expect.fail('Should have exited'); + } catch (e) { + expect((e as Error).message).toContain('process.exit(1) called'); + } + + expect(emitResultMock).toHaveBeenCalledWith({ + isError: true, + errorMessage: expect.stringContaining( + 'The configured auth type is qwen-oauth, but the current auth type is openai.', + ), + durationMs: 0, + apiDurationMs: 0, + numTurns: 0, + usage: undefined, + }); + expect(runExitCleanupMock).toHaveBeenCalled(); + expect(processExitSpy).toHaveBeenCalledWith(1); + expect(consoleErrorSpy).not.toHaveBeenCalled(); + }); + + it('emits error result and exits when validateAuthMethod fails', async () => { + vi.spyOn(auth, 'validateAuthMethod').mockReturnValue('Auth error!'); + process.env['OPENAI_API_KEY'] = 'fake-key'; + + const nonInteractiveConfig = { + refreshAuth: refreshAuthMock, + getOutputFormat: vi.fn().mockReturnValue(OutputFormat.STREAM_JSON), + getIncludePartialMessages: vi.fn().mockReturnValue(false), + getContentGeneratorConfig: vi + .fn() + .mockReturnValue({ authType: undefined }), + } as unknown as Config; + + try { + await validateNonInteractiveAuth( + AuthType.USE_OPENAI, + undefined, + nonInteractiveConfig, + mockSettings, + ); + expect.fail('Should have exited'); + } catch (e) { + expect((e as Error).message).toContain('process.exit(1) called'); + } + + expect(emitResultMock).toHaveBeenCalledWith({ + isError: true, + errorMessage: 'Auth error!', + durationMs: 0, + apiDurationMs: 0, + numTurns: 0, + usage: undefined, + }); + expect(runExitCleanupMock).toHaveBeenCalled(); + expect(processExitSpy).toHaveBeenCalledWith(1); + expect(consoleErrorSpy).not.toHaveBeenCalled(); }); }); }); diff --git a/packages/cli/src/validateNonInterActiveAuth.ts b/packages/cli/src/validateNonInterActiveAuth.ts index e44cd0a4..78ccc993 100644 --- a/packages/cli/src/validateNonInterActiveAuth.ts +++ b/packages/cli/src/validateNonInterActiveAuth.ts @@ -9,7 +9,9 @@ import { AuthType, OutputFormat } from '@qwen-code/qwen-code-core'; import { USER_SETTINGS_PATH } from './config/settings.js'; import { validateAuthMethod } from './config/auth.js'; import { type LoadedSettings } from './config/settings.js'; -import { handleError } from './utils/errors.js'; +import { JsonOutputAdapter } from './nonInteractive/io/JsonOutputAdapter.js'; +import { StreamJsonOutputAdapter } from './nonInteractive/io/StreamJsonOutputAdapter.js'; +import { runExitCleanup } from './utils/cleanup.js'; function getAuthTypeFromEnv(): AuthType | undefined { if (process.env['OPENAI_API_KEY']) { @@ -27,7 +29,7 @@ export async function validateNonInteractiveAuth( useExternalAuth: boolean | undefined, nonInteractiveConfig: Config, settings: LoadedSettings, -) { +): Promise { try { const enforcedType = settings.merged.security?.auth?.enforcedType; if (enforcedType) { @@ -58,15 +60,38 @@ export async function validateNonInteractiveAuth( await nonInteractiveConfig.refreshAuth(authType); return nonInteractiveConfig; } catch (error) { - if (nonInteractiveConfig.getOutputFormat() === OutputFormat.JSON) { - handleError( - error instanceof Error ? error : new Error(String(error)), - nonInteractiveConfig, - 1, - ); - } else { - console.error(error instanceof Error ? error.message : String(error)); + const outputFormat = nonInteractiveConfig.getOutputFormat(); + + // In JSON and STREAM_JSON modes, emit error result and exit + if ( + outputFormat === OutputFormat.JSON || + outputFormat === OutputFormat.STREAM_JSON + ) { + let adapter; + if (outputFormat === OutputFormat.JSON) { + adapter = new JsonOutputAdapter(nonInteractiveConfig); + } else { + adapter = new StreamJsonOutputAdapter( + nonInteractiveConfig, + nonInteractiveConfig.getIncludePartialMessages(), + ); + } + const errorMessage = + error instanceof Error ? error.message : String(error); + adapter.emitResult({ + isError: true, + errorMessage, + durationMs: 0, + apiDurationMs: 0, + numTurns: 0, + usage: undefined, + }); + await runExitCleanup(); process.exit(1); } + + // For other modes (text), use existing error handling + console.error(error instanceof Error ? error.message : String(error)); + process.exit(1); } }