From f2f46e938303d2ba4213167a5a82add29c7d90ad Mon Sep 17 00:00:00 2001 From: weishu Date: Sat, 3 Jan 2026 22:15:10 +0800 Subject: [PATCH] refactor(api): extract versioned update handling to shared utility Extract duplicate version acknowledgment logic from apiMachine and apiSession into a reusable applyVersionedAck utility. This handles success, version-mismatch, and error responses consistently with customizable parsing and error handling via options object. --- cli/src/api/apiMachine.ts | 127 +++++++++------------------- cli/src/api/apiSession.ts | 127 +++++++++------------------- cli/src/api/versionedUpdate.test.ts | 119 ++++++++++++++++++++++++++ cli/src/api/versionedUpdate.ts | 65 ++++++++++++++ 4 files changed, 266 insertions(+), 172 deletions(-) create mode 100644 cli/src/api/versionedUpdate.test.ts create mode 100644 cli/src/api/versionedUpdate.ts diff --git a/cli/src/api/apiMachine.ts b/cli/src/api/apiMachine.ts index 660f9911..20cda20a 100644 --- a/cli/src/api/apiMachine.ts +++ b/cli/src/api/apiMachine.ts @@ -12,6 +12,7 @@ import { backoff } from '@/utils/time' import { RpcHandlerManager } from './rpc/RpcHandlerManager' import { registerCommonHandlers } from '../modules/common/registerCommonHandlers' import type { SpawnSessionOptions, SpawnSessionResult } from '../modules/common/rpcTypes' +import { applyVersionedAck } from './versionedUpdate' interface ServerToDaemonEvents { update: (data: Update) => void @@ -157,49 +158,26 @@ export class ApiMachineClient { expectedVersion: this.machine.metadataVersion }) as unknown - if (!answer || typeof answer !== 'object') { - throw new Error('Invalid machine-update-metadata response') - } - - const obj = answer as { result?: unknown; version?: unknown; metadata?: unknown } - if (obj.result === 'success' && typeof obj.version === 'number') { - const next = obj.metadata - if (next == null) { - this.machine.metadata = null - } else { - const parsed = MachineMetadataSchema.safeParse(next) - if (parsed.success) { - this.machine.metadata = parsed.data - } else { - logger.debug('[API MACHINE] Ignoring invalid metadata value from ack', { version: obj.version }) - } - } - this.machine.metadataVersion = obj.version - return - } - - if (obj.result === 'version-mismatch' && typeof obj.version === 'number') { - const next = obj.metadata - if (next == null) { - this.machine.metadata = null - } else { - const parsed = MachineMetadataSchema.safeParse(next) - if (parsed.success) { - this.machine.metadata = parsed.data - } else { - logger.debug('[API MACHINE] Ignoring invalid metadata value from version-mismatch ack', { version: obj.version }) - } - } - this.machine.metadataVersion = obj.version - throw new Error('Metadata version mismatch') - } - - if (obj.result === 'error') { - const reason = typeof (obj as { reason?: unknown }).reason === 'string' - ? (obj as { reason?: string }).reason - : 'unknown' - throw new Error(`Machine metadata update failed (${reason})`) - } + applyVersionedAck(answer, { + valueKey: 'metadata', + parseValue: (value) => { + const parsed = MachineMetadataSchema.safeParse(value) + return parsed.success ? parsed.data : null + }, + applyValue: (value) => { + this.machine.metadata = value + }, + applyVersion: (version) => { + this.machine.metadataVersion = version + }, + logInvalidValue: (context, version) => { + const suffix = context === 'success' ? 'ack' : 'version-mismatch ack' + logger.debug(`[API MACHINE] Ignoring invalid metadata value from ${suffix}`, { version }) + }, + invalidResponseMessage: 'Invalid machine-update-metadata response', + errorMessage: 'Machine metadata update failed', + versionMismatchMessage: 'Metadata version mismatch' + }) }) } @@ -213,49 +191,26 @@ export class ApiMachineClient { expectedVersion: this.machine.daemonStateVersion }) as unknown - if (!answer || typeof answer !== 'object') { - throw new Error('Invalid machine-update-state response') - } - - const obj = answer as { result?: unknown; version?: unknown; daemonState?: unknown } - if (obj.result === 'success' && typeof obj.version === 'number') { - const next = obj.daemonState - if (next == null) { - this.machine.daemonState = null - } else { - const parsed = DaemonStateSchema.safeParse(next) - if (parsed.success) { - this.machine.daemonState = parsed.data - } else { - logger.debug('[API MACHINE] Ignoring invalid daemonState value from ack', { version: obj.version }) - } - } - this.machine.daemonStateVersion = obj.version - return - } - - if (obj.result === 'version-mismatch' && typeof obj.version === 'number') { - const next = obj.daemonState - if (next == null) { - this.machine.daemonState = null - } else { - const parsed = DaemonStateSchema.safeParse(next) - if (parsed.success) { - this.machine.daemonState = parsed.data - } else { - logger.debug('[API MACHINE] Ignoring invalid daemonState value from version-mismatch ack', { version: obj.version }) - } - } - this.machine.daemonStateVersion = obj.version - throw new Error('Daemon state version mismatch') - } - - if (obj.result === 'error') { - const reason = typeof (obj as { reason?: unknown }).reason === 'string' - ? (obj as { reason?: string }).reason - : 'unknown' - throw new Error(`Machine state update failed (${reason})`) - } + applyVersionedAck(answer, { + valueKey: 'daemonState', + parseValue: (value) => { + const parsed = DaemonStateSchema.safeParse(value) + return parsed.success ? parsed.data : null + }, + applyValue: (value) => { + this.machine.daemonState = value + }, + applyVersion: (version) => { + this.machine.daemonStateVersion = version + }, + logInvalidValue: (context, version) => { + const suffix = context === 'success' ? 'ack' : 'version-mismatch ack' + logger.debug(`[API MACHINE] Ignoring invalid daemonState value from ${suffix}`, { version }) + }, + invalidResponseMessage: 'Invalid machine-update-state response', + errorMessage: 'Machine state update failed', + versionMismatchMessage: 'Daemon state version mismatch' + }) }) } diff --git a/cli/src/api/apiSession.ts b/cli/src/api/apiSession.ts index 743603fd..80edb759 100644 --- a/cli/src/api/apiSession.ts +++ b/cli/src/api/apiSession.ts @@ -31,6 +31,7 @@ import { TerminalResizePayloadSchema, TerminalWritePayloadSchema } from '@/terminal/types' +import { applyVersionedAck } from './versionedUpdate' export class ApiSessionClient extends EventEmitter { private readonly token: string @@ -459,49 +460,26 @@ export class ApiSessionClient extends EventEmitter { metadata: updated }) as unknown - if (!answer || typeof answer !== 'object') { - throw new Error('Invalid update-metadata response') - } - - const obj = answer as { result?: unknown; version?: unknown; metadata?: unknown } - if (obj.result === 'success' && typeof obj.version === 'number') { - const next = obj.metadata - if (next == null) { - this.metadata = null - } else { - const parsed = MetadataSchema.safeParse(next) - if (parsed.success) { - this.metadata = parsed.data - } else { - logger.debug('[API] Ignoring invalid metadata value from ack', { version: obj.version }) - } - } - this.metadataVersion = obj.version - return - } - - if (obj.result === 'version-mismatch' && typeof obj.version === 'number') { - const next = obj.metadata - if (next == null) { - this.metadata = null - } else { - const parsed = MetadataSchema.safeParse(next) - if (parsed.success) { - this.metadata = parsed.data - } else { - logger.debug('[API] Ignoring invalid metadata value from version-mismatch ack', { version: obj.version }) - } - } - this.metadataVersion = obj.version - throw new Error('Metadata version mismatch') - } - - if (obj.result === 'error') { - const reason = typeof (obj as { reason?: unknown }).reason === 'string' - ? (obj as { reason?: string }).reason - : 'unknown' - throw new Error(`Metadata update failed (${reason})`) - } + applyVersionedAck(answer, { + valueKey: 'metadata', + parseValue: (value) => { + const parsed = MetadataSchema.safeParse(value) + return parsed.success ? parsed.data : null + }, + applyValue: (value) => { + this.metadata = value + }, + applyVersion: (version) => { + this.metadataVersion = version + }, + logInvalidValue: (context, version) => { + const suffix = context === 'success' ? 'ack' : 'version-mismatch ack' + logger.debug(`[API] Ignoring invalid metadata value from ${suffix}`, { version }) + }, + invalidResponseMessage: 'Invalid update-metadata response', + errorMessage: 'Metadata update failed', + versionMismatchMessage: 'Metadata version mismatch' + }) }) }) } @@ -518,49 +496,26 @@ export class ApiSessionClient extends EventEmitter { agentState: updated }) as unknown - if (!answer || typeof answer !== 'object') { - throw new Error('Invalid update-state response') - } - - const obj = answer as { result?: unknown; version?: unknown; agentState?: unknown } - if (obj.result === 'success' && typeof obj.version === 'number') { - const next = obj.agentState - if (next == null) { - this.agentState = null - } else { - const parsed = AgentStateSchema.safeParse(next) - if (parsed.success) { - this.agentState = parsed.data - } else { - logger.debug('[API] Ignoring invalid agentState value from ack', { version: obj.version }) - } - } - this.agentStateVersion = obj.version - return - } - - if (obj.result === 'version-mismatch' && typeof obj.version === 'number') { - const next = obj.agentState - if (next == null) { - this.agentState = null - } else { - const parsed = AgentStateSchema.safeParse(next) - if (parsed.success) { - this.agentState = parsed.data - } else { - logger.debug('[API] Ignoring invalid agentState value from version-mismatch ack', { version: obj.version }) - } - } - this.agentStateVersion = obj.version - throw new Error('Agent state version mismatch') - } - - if (obj.result === 'error') { - const reason = typeof (obj as { reason?: unknown }).reason === 'string' - ? (obj as { reason?: string }).reason - : 'unknown' - throw new Error(`Agent state update failed (${reason})`) - } + applyVersionedAck(answer, { + valueKey: 'agentState', + parseValue: (value) => { + const parsed = AgentStateSchema.safeParse(value) + return parsed.success ? parsed.data : null + }, + applyValue: (value) => { + this.agentState = value + }, + applyVersion: (version) => { + this.agentStateVersion = version + }, + logInvalidValue: (context, version) => { + const suffix = context === 'success' ? 'ack' : 'version-mismatch ack' + logger.debug(`[API] Ignoring invalid agentState value from ${suffix}`, { version }) + }, + invalidResponseMessage: 'Invalid update-state response', + errorMessage: 'Agent state update failed', + versionMismatchMessage: 'Agent state version mismatch' + }) }) }) } diff --git a/cli/src/api/versionedUpdate.test.ts b/cli/src/api/versionedUpdate.test.ts new file mode 100644 index 00000000..f41aab91 --- /dev/null +++ b/cli/src/api/versionedUpdate.test.ts @@ -0,0 +1,119 @@ +import { describe, expect, it } from 'vitest'; +import { applyVersionedAck, type AckResult, type VersionedAckOptions } from './versionedUpdate'; + +type TestState = { + value: string | null; + version: number; +}; + +const baseOptions = ( + state: TestState, + logInvalids: Array<{ context: AckResult; version: number }>, + overrides?: Partial> +): VersionedAckOptions => ({ + valueKey: 'metadata', + parseValue: (value) => (typeof value === 'string' ? value : null), + applyValue: (value) => { + state.value = value; + }, + applyVersion: (version) => { + state.version = version; + }, + logInvalidValue: (context, version) => { + logInvalids.push({ context, version }); + }, + invalidResponseMessage: 'Invalid update-metadata response', + errorMessage: 'Metadata update failed', + versionMismatchMessage: 'Metadata version mismatch', + ...(overrides ?? {}) +}); + +describe('applyVersionedAck', () => { + it('applies value and version on success', () => { + const state: TestState = { value: null, version: 0 }; + const logInvalids: Array<{ context: AckResult; version: number }> = []; + const options = baseOptions(state, logInvalids); + + expect(() => applyVersionedAck({ + result: 'success', + version: 2, + metadata: 'next' + }, options)).not.toThrow(); + + expect(state.value).toBe('next'); + expect(state.version).toBe(2); + expect(logInvalids).toHaveLength(0); + }); + + it('applies value/version then throws on version mismatch', () => { + const state: TestState = { value: 'old', version: 1 }; + const logInvalids: Array<{ context: AckResult; version: number }> = []; + const options = baseOptions(state, logInvalids); + + let caught: unknown; + try { + applyVersionedAck({ + result: 'version-mismatch', + version: 5, + metadata: 'server' + }, options); + } catch (error) { + caught = error; + } + + if (!(caught instanceof Error)) { + throw new Error('Expected version mismatch error'); + } + + expect(caught.message).toBe('Metadata version mismatch'); + expect(state.value).toBe('server'); + expect(state.version).toBe(5); + }); + + it('throws on error results without mutating state', () => { + const state: TestState = { value: 'existing', version: 3 }; + const logInvalids: Array<{ context: AckResult; version: number }> = []; + const options = baseOptions(state, logInvalids); + + expect(() => applyVersionedAck({ + result: 'error', + reason: 'access-denied' + }, options)).toThrow('Metadata update failed (access-denied)'); + + expect(state.value).toBe('existing'); + expect(state.version).toBe(3); + }); + + it('throws on malformed responses', () => { + const state: TestState = { value: 'existing', version: 3 }; + const logInvalids: Array<{ context: AckResult; version: number }> = []; + const options = baseOptions(state, logInvalids); + + expect(() => applyVersionedAck({ + result: 'success', + version: 'nope', + metadata: 'value' + }, options)).toThrow('Invalid update-metadata response'); + + expect(state.value).toBe('existing'); + expect(state.version).toBe(3); + }); + + it('logs invalid values but still updates the version', () => { + const state: TestState = { value: 'existing', version: 1 }; + const logInvalids: Array<{ context: AckResult; version: number }> = []; + const options = baseOptions(state, logInvalids, { + parseValue: () => null + }); + + applyVersionedAck({ + result: 'success', + version: 4, + metadata: 123 + }, options); + + expect(state.value).toBe('existing'); + expect(state.version).toBe(4); + expect(logInvalids).toEqual([{ context: 'success', version: 4 }]); + }); +}); diff --git a/cli/src/api/versionedUpdate.ts b/cli/src/api/versionedUpdate.ts new file mode 100644 index 00000000..7569624f --- /dev/null +++ b/cli/src/api/versionedUpdate.ts @@ -0,0 +1,65 @@ +export type AckResult = 'success' | 'version-mismatch' + +export type VersionedAckResult = + | ({ result: 'success'; version: number } & Record) + | ({ result: 'version-mismatch'; version: number } & Record) + | { result: 'error'; reason?: string } + +export type VersionedAckOptions = { + valueKey: ValueKey + parseValue: (value: unknown) => TValue | null + applyValue: (value: TValue | null) => void + applyVersion: (version: number) => void + logInvalidValue: (context: AckResult, version: number) => void + invalidResponseMessage: string + errorMessage: string + versionMismatchMessage: string +} + +const isRecord = (value: unknown): value is Record => { + return typeof value === 'object' && value !== null +} + +export const applyVersionedAck = ( + ack: unknown, + options: VersionedAckOptions +): void => { + if (!isRecord(ack)) { + throw new Error(options.invalidResponseMessage) + } + + const result = ack.result + if (result === 'success' || result === 'version-mismatch') { + const version = ack.version + if (typeof version !== 'number') { + throw new Error(options.invalidResponseMessage) + } + + const rawValue = ack[options.valueKey] + if (rawValue == null) { + options.applyValue(null) + } else { + const parsed = options.parseValue(rawValue) + if (parsed === null) { + options.logInvalidValue(result, version) + } else { + options.applyValue(parsed) + } + } + + options.applyVersion(version) + + if (result === 'version-mismatch') { + throw new Error(options.versionMismatchMessage) + } + + return + } + + if (result === 'error') { + const reason = typeof ack.reason === 'string' ? ack.reason : 'unknown' + throw new Error(`${options.errorMessage} (${reason})`) + } + + throw new Error(options.invalidResponseMessage) +}