refactor(cli): audit fixes — validate timeoutMs, extract mock factory, add comments

diff --git a/ui/cli/src/client/lifecycle.ts b/ui/cli/src/client/lifecycle.ts
index 9f36423244f49c078a02a3c3f90f473cd93a1767..b500f9608128ee732ddcf653c347537600cfa669 100644 (file)
--- 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<void> =>
   let connectTimeoutId: ReturnType<typeof setTimeout> | 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 c7c705651ad6e784e0603f630d86d88e1ad88ef4..bdb7fe1ca6d8b2175950aebe6500402f5da326c0 100644 (file)
--- 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 b6b13e73f819a1cb0ef2ce725bd4534cbc13a2db..b6392a9910de09df7b7af09b17fb81610c141ac2 100644 (file)
--- 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 0cdf5a54b5cb0c6942ec68e60b6ad651d0188a5b..60c41f928c0803dbdb1194ae5ae5f9fe82be1e0b 100644 (file)
--- 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 fa2d787970089291c4a0cd9df2e70fe4e73b1087..2120004a21080b5a46604e2541b8219bf3a0033a 100644 (file)
--- 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 eb309ecb67663655b9af9d29bbae7a9c69e471b7..c33ace9827e4f746f34464a02f70b342fe8a10f4 100644 (file)
--- 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 4e5352b6c41dd4fb37388c8e7f7829e1cf06ba5c..188ef79871b039b7024bfadfa5e94d3d1b0bfa37 100644 (file)
--- 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 1a2848d3e07ea47c7bb79962556c6c8461780362..971a89841e489cb094b4fb3ef97d851c0e9cbc26 100644 (file)
--- 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
+      }
+    )
+  })
 })