From dfb70ffb385fdcb52992e6dce6d99a1e95866544 Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=B4me=20Benoit?= Date: Wed, 15 Apr 2026 21:09:36 +0200 Subject: [PATCH] =?utf8?q?refactor(cli):=20audit=20fixes=20=E2=80=94=20val?= =?utf8?q?idate=20timeoutMs,=20extract=20mock=20factory,=20add=20comments?= MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit --- ui/cli/src/client/lifecycle.ts | 1 + ui/cli/src/client/ws-adapter.ts | 3 +- ui/cli/src/output/table.ts | 6 +- ui/cli/tests/lifecycle.test.ts | 5 + ui/cli/tests/output.test.ts | 5 + ui/cli/tests/ws-adapter.test.ts | 187 ++++++++---------------- ui/common/src/client/WebSocketClient.ts | 4 + ui/common/tests/WebSocketClient.test.ts | 61 ++++++++ 8 files changed, 137 insertions(+), 135 deletions(-) diff --git a/ui/cli/src/client/lifecycle.ts b/ui/cli/src/client/lifecycle.ts index 9f364232..b500f960 100644 --- a/ui/cli/src/client/lifecycle.ts +++ b/ui/cli/src/client/lifecycle.ts @@ -56,6 +56,7 @@ export const executeCommand = async (options: ExecuteOptions): Promise => let connectTimeoutId: ReturnType | undefined try { const connectPromise = client.connect() + // Prevent unhandled rejection when timeout wins the race and connect rejects later connectPromise.catch(() => undefined) await Promise.race([ connectPromise, diff --git a/ui/cli/src/client/ws-adapter.ts b/ui/cli/src/client/ws-adapter.ts index c7c70565..bdb7fe1c 100644 --- a/ui/cli/src/client/ws-adapter.ts +++ b/ui/cli/src/client/ws-adapter.ts @@ -1,5 +1,4 @@ -import type { WebSocketLike } from 'ui-common' -import type { WebSocketReadyState } from 'ui-common' +import type { WebSocketLike, WebSocketReadyState } from 'ui-common' import type { WebSocket as WsWebSocket } from 'ws' import { Buffer } from 'node:buffer' diff --git a/ui/cli/src/output/table.ts b/ui/cli/src/output/table.ts index b6b13e73..b6392a99 100644 --- a/ui/cli/src/output/table.ts +++ b/ui/cli/src/output/table.ts @@ -5,9 +5,7 @@ import { type ResponsePayload, ResponseStatus } from 'ui-common' export const outputTable = (payload: ResponsePayload): void => { if (payload.hashIdsSucceeded != null && payload.hashIdsSucceeded.length > 0) { - process.stdout.write( - chalk.green(`✓ Succeeded (${payload.hashIdsSucceeded.length.toString()}):\n`) - ) + process.stdout.write(chalk.green(`✓ Succeeded (${String(payload.hashIdsSucceeded.length)}):\n`)) const table = new Table({ head: [chalk.white('Hash ID')] }) for (const id of payload.hashIdsSucceeded) { table.push([id]) @@ -16,7 +14,7 @@ export const outputTable = (payload: ResponsePayload): void => { } if (payload.hashIdsFailed != null && payload.hashIdsFailed.length > 0) { - process.stderr.write(chalk.red(`✗ Failed (${payload.hashIdsFailed.length.toString()}):\n`)) + process.stderr.write(chalk.red(`✗ Failed (${String(payload.hashIdsFailed.length)}):\n`)) if (payload.responsesFailed != null && payload.responsesFailed.length > 0) { const table = new Table({ head: [chalk.white('Hash ID'), chalk.white('Error')] }) for (const entry of payload.responsesFailed) { diff --git a/ui/cli/tests/lifecycle.test.ts b/ui/cli/tests/lifecycle.test.ts index 0cdf5a54..60c41f92 100644 --- a/ui/cli/tests/lifecycle.test.ts +++ b/ui/cli/tests/lifecycle.test.ts @@ -1,3 +1,8 @@ +/** + * @file Unit tests for CLI lifecycle and error types + * @description Tests for connection lifecycle management and error handling + */ + import assert from 'node:assert' import { describe, it } from 'node:test' diff --git a/ui/cli/tests/output.test.ts b/ui/cli/tests/output.test.ts index fa2d7879..2120004a 100644 --- a/ui/cli/tests/output.test.ts +++ b/ui/cli/tests/output.test.ts @@ -1,3 +1,8 @@ +/** + * @file Unit tests for CLI output formatters (JSON and table) + * @description Tests for JSON and table output formatting functions + */ + import assert from 'node:assert' import { describe, it } from 'node:test' import { ResponseStatus } from 'ui-common' diff --git a/ui/cli/tests/ws-adapter.test.ts b/ui/cli/tests/ws-adapter.test.ts index eb309ecb..c33ace98 100644 --- a/ui/cli/tests/ws-adapter.test.ts +++ b/ui/cli/tests/ws-adapter.test.ts @@ -1,3 +1,8 @@ +/** + * @file Unit tests for the WebSocket adapter (ws → WebSocketLike) + * @description Tests for converting ws library WebSocket to WebSocketLike interface + */ + import type { WebSocket } from 'ws' import assert from 'node:assert' @@ -17,17 +22,19 @@ interface MockWs { send: (data: string) => void } +const createMockWs = (): MockWs => ({ + close: () => undefined, + onclose: null, + onerror: null, + onmessage: null, + onopen: null, + readyState: WebSocketReadyState.OPEN, + send: () => undefined, +}) + await describe('WS Adapter', async () => { await it('should convert Buffer data to string in onmessage', () => { - const mockWs: MockWs = { - close: () => undefined, - onclose: null, - onerror: null, - onmessage: null, - onopen: null, - readyState: WebSocketReadyState.OPEN, - send: () => undefined, - } + const mockWs = createMockWs() const adapter = createWsAdapter(mockWs as unknown as WebSocket) @@ -43,15 +50,7 @@ await describe('WS Adapter', async () => { }) await it('should convert ArrayBuffer data to string in onmessage', () => { - const mockWs: MockWs = { - close: () => undefined, - onclose: null, - onerror: null, - onmessage: null, - onopen: null, - readyState: WebSocketReadyState.OPEN, - send: () => undefined, - } + const mockWs = createMockWs() const adapter = createWsAdapter(mockWs as unknown as WebSocket) @@ -67,15 +66,7 @@ await describe('WS Adapter', async () => { }) await it('should convert Buffer[] data to string in onmessage', () => { - const mockWs: MockWs = { - close: () => undefined, - onclose: null, - onerror: null, - onmessage: null, - onopen: null, - readyState: WebSocketReadyState.OPEN, - send: () => undefined, - } + const mockWs = createMockWs() const adapter = createWsAdapter(mockWs as unknown as WebSocket) @@ -91,15 +82,7 @@ await describe('WS Adapter', async () => { }) await it('should pass through string data in onmessage', () => { - const mockWs: MockWs = { - close: () => undefined, - onclose: null, - onerror: null, - onmessage: null, - onopen: null, - readyState: WebSocketReadyState.OPEN, - send: () => undefined, - } + const mockWs = createMockWs() const adapter = createWsAdapter(mockWs as unknown as WebSocket) @@ -114,15 +97,8 @@ await describe('WS Adapter', async () => { }) await it('should delegate readyState getter to ws', () => { - const mockWs: MockWs = { - close: () => undefined, - onclose: null, - onerror: null, - onmessage: null, - onopen: null, - readyState: WebSocketReadyState.CONNECTING, - send: () => undefined, - } + const mockWs = createMockWs() + mockWs.readyState = WebSocketReadyState.CONNECTING const adapter = createWsAdapter(mockWs as unknown as WebSocket) @@ -131,16 +107,9 @@ await describe('WS Adapter', async () => { await it('should delegate send() to ws', () => { let sentData: string | undefined - const mockWs: MockWs = { - close: () => undefined, - onclose: null, - onerror: null, - onmessage: null, - onopen: null, - readyState: WebSocketReadyState.OPEN, - send: (data: string) => { - sentData = data - }, + const mockWs = createMockWs() + mockWs.send = (data: string) => { + sentData = data } const adapter = createWsAdapter(mockWs as unknown as WebSocket) @@ -152,17 +121,10 @@ await describe('WS Adapter', async () => { await it('should delegate close() to ws', () => { let closeCode: number | undefined let closeReason: string | undefined - const mockWs: MockWs = { - close: (code?: number, reason?: string) => { - closeCode = code - closeReason = reason - }, - onclose: null, - onerror: null, - onmessage: null, - onopen: null, - readyState: WebSocketReadyState.OPEN, - send: () => undefined, + const mockWs = createMockWs() + mockWs.close = (code?: number, reason?: string) => { + closeCode = code + closeReason = reason } const adapter = createWsAdapter(mockWs as unknown as WebSocket) @@ -173,15 +135,7 @@ await describe('WS Adapter', async () => { }) await it('should forward onerror event with error shape', () => { - const mockWs: MockWs = { - close: () => undefined, - onclose: null, - onerror: null, - onmessage: null, - onopen: null, - readyState: WebSocketReadyState.OPEN, - send: () => undefined, - } + const mockWs = createMockWs() const adapter = createWsAdapter(mockWs as unknown as WebSocket) @@ -202,16 +156,31 @@ await describe('WS Adapter', async () => { assert.strictEqual(receivedMessage, 'connection failed') }) - await it('should forward onclose event with code and reason', () => { - const mockWs: MockWs = { - close: () => undefined, - onclose: null, - onerror: null, - onmessage: null, - onopen: null, - readyState: WebSocketReadyState.CLOSED, - send: () => undefined, + await it('should forward onerror when event is a string', () => { + const mockWs = createMockWs() + const adapter = createWsAdapter(mockWs as unknown as WebSocket) + let receivedMessage = '' + adapter.onerror = event => { + receivedMessage = event.message } + mockWs.onerror?.('connection refused') + assert.strictEqual(receivedMessage, 'connection refused') + }) + + await it('should forward onerror with fallback for unknown event type', () => { + const mockWs = createMockWs() + const adapter = createWsAdapter(mockWs as unknown as WebSocket) + let receivedMessage = '' + adapter.onerror = event => { + receivedMessage = event.message + } + mockWs.onerror?.(42 as unknown as Error) + assert.strictEqual(receivedMessage, 'Unknown error') + }) + + await it('should forward onclose event with code and reason', () => { + const mockWs = createMockWs() + mockWs.readyState = WebSocketReadyState.CLOSED const adapter = createWsAdapter(mockWs as unknown as WebSocket) @@ -229,15 +198,7 @@ await describe('WS Adapter', async () => { }) await it('should forward onopen event', () => { - const mockWs: MockWs = { - close: () => undefined, - onclose: null, - onerror: null, - onmessage: null, - onopen: null, - readyState: WebSocketReadyState.OPEN, - send: () => undefined, - } + const mockWs = createMockWs() const adapter = createWsAdapter(mockWs as unknown as WebSocket) @@ -252,15 +213,7 @@ await describe('WS Adapter', async () => { }) await it('should have getter and setter for onmessage', () => { - const mockWs: MockWs = { - close: () => undefined, - onclose: null, - onerror: null, - onmessage: null, - onopen: null, - readyState: WebSocketReadyState.OPEN, - send: () => undefined, - } + const mockWs = createMockWs() const adapter = createWsAdapter(mockWs as unknown as WebSocket) @@ -277,15 +230,7 @@ await describe('WS Adapter', async () => { }) await it('should have getter and setter for onerror', () => { - const mockWs: MockWs = { - close: () => undefined, - onclose: null, - onerror: null, - onmessage: null, - onopen: null, - readyState: WebSocketReadyState.OPEN, - send: () => undefined, - } + const mockWs = createMockWs() const adapter = createWsAdapter(mockWs as unknown as WebSocket) @@ -302,15 +247,7 @@ await describe('WS Adapter', async () => { }) await it('should have getter and setter for onclose', () => { - const mockWs: MockWs = { - close: () => undefined, - onclose: null, - onerror: null, - onmessage: null, - onopen: null, - readyState: WebSocketReadyState.OPEN, - send: () => undefined, - } + const mockWs = createMockWs() const adapter = createWsAdapter(mockWs as unknown as WebSocket) @@ -327,15 +264,7 @@ await describe('WS Adapter', async () => { }) await it('should have getter and setter for onopen', () => { - const mockWs: MockWs = { - close: () => undefined, - onclose: null, - onerror: null, - onmessage: null, - onopen: null, - readyState: WebSocketReadyState.OPEN, - send: () => undefined, - } + const mockWs = createMockWs() const adapter = createWsAdapter(mockWs as unknown as WebSocket) diff --git a/ui/common/src/client/WebSocketClient.ts b/ui/common/src/client/WebSocketClient.ts index 4e5352b6..188ef798 100644 --- a/ui/common/src/client/WebSocketClient.ts +++ b/ui/common/src/client/WebSocketClient.ts @@ -109,6 +109,10 @@ export class WebSocketClient { const uuid = randomUUID() const message = JSON.stringify([uuid, procedureName, payload]) const effectiveTimeoutMs = timeoutMs ?? this.timeoutMs + if (!Number.isFinite(effectiveTimeoutMs) || effectiveTimeoutMs <= 0) { + reject(new Error(`Invalid timeout: ${String(effectiveTimeoutMs)}ms (must be > 0)`)) + return + } const timeoutId = setTimeout(() => { this.responseHandlers.delete(uuid) reject( diff --git a/ui/common/tests/WebSocketClient.test.ts b/ui/common/tests/WebSocketClient.test.ts index 1a2848d3..971a8984 100644 --- a/ui/common/tests/WebSocketClient.test.ts +++ b/ui/common/tests/WebSocketClient.test.ts @@ -1,3 +1,8 @@ +/** + * @file Unit tests for the SRPC WebSocket client + * @description Tests for WebSocketClient connection, request/response handling, and timeout validation + */ + import assert from 'node:assert' import { describe, it } from 'node:test' @@ -387,4 +392,60 @@ await describe('WebSocketClient', async () => { } ) }) + + await it('should reject sendRequest with timeoutMs = 0', async () => { + const mockWs = createMockWS() + const client = new WebSocketClient( + () => mockWs, + { + host: 'localhost', + port: 8080, + protocol: 'ui', + version: '0.0.1', + }, + 5000 + ) + const connectPromise = client.connect() + mockWs.triggerOpen() + await connectPromise + + await assert.rejects( + async () => { + await client.sendRequest(ProcedureName.SIMULATOR_STATE, {}, 0) + }, + (error: unknown) => { + assert.ok(error instanceof Error) + assert.ok(error.message.includes('Invalid timeout')) + return true + } + ) + }) + + await it('should reject sendRequest with timeoutMs = -1', async () => { + const mockWs = createMockWS() + const client = new WebSocketClient( + () => mockWs, + { + host: 'localhost', + port: 8080, + protocol: 'ui', + version: '0.0.1', + }, + 5000 + ) + const connectPromise = client.connect() + mockWs.triggerOpen() + await connectPromise + + await assert.rejects( + async () => { + await client.sendRequest(ProcedureName.SIMULATOR_STATE, {}, -1) + }, + (error: unknown) => { + assert.ok(error instanceof Error) + assert.ok(error.message.includes('Invalid timeout')) + return true + } + ) + }) }) -- 2.53.0