Skip to content

Commit 6cc1576

Browse files
EurFeluxclaudehappy-otter
authored
refactor: migrate Serializable type to Zod schema and fix circular reference handling (#12865)
### What this PR does Before this PR: - `Serializable` type and `isSerializable` function were maintained separately - `SerializableValue` used `any` workaround (with FIXME comment about TS2589) - No runtime validation schema available for consumers - `error.ts` directly assigned `error.response` without serialization, causing type errors with `Date` objects - `isSerializable` incorrectly returned `true` for circular references, despite `JSON.stringify` throwing on them After this PR: - Unified type definition using Zod schema backed by the existing `isSerializable` type guard via `z.custom()`, ensuring consistent validation behavior - Added `SerializableSchema` export for use by other modules - Removed unused helper functions (`assertSerializable`, `tryParseSerializable`) to keep exports minimal - Fixed `error.ts` to properly serialize `response` field using `safeSerialize()` - Fixed `isSerializable` to correctly reject circular references - Improved `isSerializable` return type to `value is Serializable` for better type narrowing - Added comprehensive unit tests for `isSerializable` and `SerializableSchema` - Removed misleading FIXME comment about TS2589 Fixes # ### Why we need it and why it was done in this way The following tradeoffs were made: - Used `z.custom(isSerializable)` instead of building a recursive Zod schema with `z.lazy()`, to guarantee the Zod schema and the hand-written type guard share identical validation logic (e.g. circular reference handling, prototype chain checks, built-in object rejection) - Kept existing `isSerializable()` implementation as the single source of truth for runtime checks The following alternatives were considered: - Building a standalone recursive Zod schema with `z.lazy()` + `z.union()` + `z.record()`, but this had subtle behavioral differences from `isSerializable()` (e.g. `z.record()` does not check prototype chains or reject `Date`/class instances) - Using `z.infer` to derive type from schema, but `z.lazy()` requires explicit type annotation to avoid `any` inference - **Using Zod 4's built-in `z.json()`** — this was evaluated as a potential replacement for the entire hand-written `isSerializable` + `z.custom()` approach. `z.json()` validates the same recursive union (`string | number | boolean | null | JsonValue[] | Record<string, JsonValue>`), which is structurally identical to our `Serializable` type. However, it was not adopted for the following reasons: 1. **Circular reference safety**: `z.json()` is built on `z.lazy()`, which recurses without cycle detection. Passing a circular reference causes a stack overflow instead of returning a validation failure. Our `isSerializable()` uses a `seen` Set to detect cycles and safely returns `false`. This is critical because `utils/serialize.ts` calls `isSerializable()` on arbitrary values of unknown provenance. 2. **Performance**: The hand-written check uses direct `typeof` comparisons and a single recursive walk. `z.json()` uses `z.union([...])` which must attempt each branch in order, adding overhead for deep/large objects. 3. **Explicit built-in object rejection**: `isSerializable()` explicitly rejects `Date`, `RegExp`, `Map`, `Set`, `Error`, `File`, and `Blob` with clear `instanceof` checks and a prototype chain guard, making the rejection reasons self-documenting. `z.json()` rejects them implicitly (they don't match any union branch), which is correct but less obvious to readers. ### Breaking changes None. All existing exports remain unchanged: - `Serializable` type (same signature) - `isSerializable` function (same behavior for all valid inputs; circular references now correctly return `false` instead of `true` — this is a bug fix, as circular references are not JSON-serializable and `tryLenientSerialize` already expects them to be rejected) ### Special notes for your reviewer - The `error.ts` change (line 196) fixes a type error that surfaced after making `Serializable` stricter — `error.response` is `LanguageModelResponseMetadata` which contains `Date` objects, so it needs `safeSerialize()` instead of direct assignment. - `SerializableSchema` uses `z.custom()` to delegate all validation to `isSerializable`, so the two are guaranteed to be consistent. - The circular reference fix is a bug fix: `isSerializable` previously returned `true` for circular references to "avoid infinite recursion", but this was incorrect — circular references cause `JSON.stringify` to throw, so they are by definition not serializable. The `tryLenientSerialize` design also assumes circular references are not serializable. ### Checklist - [x] PR: The PR description is expressive enough and will help future contributors - [x] Code: [Write code that humans can understand](https://en.wikiquote.org/wiki/Martin_Fowler#code-for-humans) and [Keep it simple](https://en.wikipedia.org/wiki/KISS_principle) - [x] Refactor: You have [left the code cleaner than you found it (Boy Scout Rule)](https://learning.oreilly.com/library/view/97-things-every/9780096809515/ch08.html) - [x] Upgrade: Impact of this change on upgrade flows was considered and addressed if required - [ ] Documentation: A [user-guide update](https://docs.cherry-ai.com) was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature. ### Release note ```release-note NONE ``` 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Happy <yesreply@happy.engineering>
1 parent 894664f commit 6cc1576

File tree

3 files changed

+165
-17
lines changed

3 files changed

+165
-17
lines changed
Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
import { describe, expect, it } from 'vitest'
2+
3+
import { isSerializable, SerializableSchema } from '../serialize'
4+
5+
describe('isSerializable', () => {
6+
describe('primitives', () => {
7+
it.each([
8+
['null', null],
9+
['string', 'hello'],
10+
['empty string', ''],
11+
['number', 42],
12+
['zero', 0],
13+
['NaN', NaN],
14+
['Infinity', Infinity],
15+
['negative number', -3.14],
16+
['true', true],
17+
['false', false]
18+
])('should accept %s', (_, value) => {
19+
expect(isSerializable(value)).toBe(true)
20+
})
21+
})
22+
23+
describe('non-serializable primitives', () => {
24+
it.each([
25+
['undefined', undefined],
26+
['symbol', Symbol('test')],
27+
['bigint', BigInt(123)],
28+
['function', () => {}]
29+
])('should reject %s', (_, value) => {
30+
expect(isSerializable(value)).toBe(false)
31+
})
32+
})
33+
34+
describe('arrays', () => {
35+
it('should accept empty array', () => {
36+
expect(isSerializable([])).toBe(true)
37+
})
38+
39+
it('should accept array of primitives', () => {
40+
expect(isSerializable([1, 'two', true, null])).toBe(true)
41+
})
42+
43+
it('should accept nested arrays', () => {
44+
expect(
45+
isSerializable([
46+
[1, 2],
47+
[3, [4, 5]]
48+
])
49+
).toBe(true)
50+
})
51+
52+
it('should reject array containing undefined', () => {
53+
expect(isSerializable([1, undefined, 3])).toBe(false)
54+
})
55+
56+
it('should reject array containing function', () => {
57+
expect(isSerializable([1, () => {}, 3])).toBe(false)
58+
})
59+
})
60+
61+
describe('plain objects', () => {
62+
it('should accept empty object', () => {
63+
expect(isSerializable({})).toBe(true)
64+
})
65+
66+
it('should accept object with primitive values', () => {
67+
expect(isSerializable({ a: 1, b: 'two', c: true, d: null })).toBe(true)
68+
})
69+
70+
it('should accept deeply nested objects', () => {
71+
expect(isSerializable({ a: { b: { c: { d: 1 } } } })).toBe(true)
72+
})
73+
74+
it('should accept object with array values', () => {
75+
expect(isSerializable({ items: [1, 2, 3] })).toBe(true)
76+
})
77+
78+
it('should reject object with undefined value', () => {
79+
expect(isSerializable({ a: undefined })).toBe(false)
80+
})
81+
82+
it('should reject object with function value', () => {
83+
expect(isSerializable({ fn: () => {} })).toBe(false)
84+
})
85+
})
86+
87+
describe('built-in objects', () => {
88+
it.each([
89+
['Date', new Date()],
90+
['RegExp', /test/],
91+
['Map', new Map()],
92+
['Set', new Set()],
93+
['Error', new Error('test')]
94+
])('should reject %s', (_, value) => {
95+
expect(isSerializable(value)).toBe(false)
96+
})
97+
})
98+
99+
describe('class instances', () => {
100+
it('should reject class instances (non-plain objects)', () => {
101+
class Foo {
102+
x = 1
103+
}
104+
expect(isSerializable(new Foo())).toBe(false)
105+
})
106+
})
107+
108+
describe('circular references', () => {
109+
it('should reject circular reference in object', () => {
110+
const obj: Record<string, unknown> = { a: 1 }
111+
obj.self = obj
112+
expect(isSerializable(obj)).toBe(false)
113+
})
114+
115+
it('should reject circular reference in array', () => {
116+
const arr: unknown[] = [1, 2]
117+
arr.push(arr)
118+
expect(isSerializable(arr)).toBe(false)
119+
})
120+
})
121+
})
122+
123+
describe('SerializableSchema', () => {
124+
it('should accept valid serializable values', () => {
125+
const value = { a: 1, b: [true, 'hello', null], c: { nested: 42 } }
126+
expect(SerializableSchema.safeParse(value).success).toBe(true)
127+
})
128+
129+
it('should reject non-serializable values', () => {
130+
expect(SerializableSchema.safeParse(undefined).success).toBe(false)
131+
expect(SerializableSchema.safeParse(() => {}).success).toBe(false)
132+
expect(SerializableSchema.safeParse(new Date()).success).toBe(false)
133+
})
134+
135+
it('should have consistent behavior with isSerializable', () => {
136+
const cases = [null, 42, 'str', true, [1, 2], { a: 1 }, undefined, new Date(), new Map(), () => {}]
137+
for (const value of cases) {
138+
expect(SerializableSchema.safeParse(value).success).toBe(isSerializable(value))
139+
}
140+
})
141+
})
Lines changed: 23 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,26 @@
1-
export type Serializable = null | boolean | number | string | { [key: string]: SerializableValue } | SerializableValue[]
1+
import * as z from 'zod'
22

3-
// FIXME: any 不是可安全序列化的类型,但是递归定义会报ts2589
4-
type SerializableValue = null | boolean | number | string | { [key: string]: any } | any[]
3+
/**
4+
* Serializable type
5+
*/
6+
export type Serializable = string | number | boolean | null | Serializable[] | { [key: string]: Serializable }
57

68
/**
7-
* 判断一个值是否可序列化(适合用于 Redux 状态)
8-
* 支持嵌套对象、数组的深度检测
9+
* Zod schema for serializable values
10+
* Uses z.custom() with isSerializable type guard to ensure consistent validation behavior
911
*/
12+
export const SerializableSchema: z.ZodType<Serializable> = z.custom<Serializable>(isSerializable)
1013

11-
export function isSerializable(value: unknown): boolean {
12-
const seen = new Set() // 用于防止循环引用
14+
/**
15+
* Check if a value is serializable (suitable for Redux state)
16+
* Supports deep detection of nested objects and arrays
17+
*/
18+
export function isSerializable(value: unknown): value is Serializable {
19+
const seen = new Set<unknown>()
1320

1421
function _isSerializable(val: unknown): boolean {
1522
if (val === null || val === undefined) {
16-
return val !== undefined // null ✅, undefined ❌
23+
return val !== undefined
1724
}
1825

1926
const type = typeof val
@@ -23,23 +30,23 @@ export function isSerializable(value: unknown): boolean {
2330
}
2431

2532
if (type === 'object') {
26-
// 检查循环引用
33+
// Circular references are not JSON-serializable
2734
if (seen.has(val)) {
28-
return true // 避免无限递归,假设循环引用对象本身结构合法(但实际 JSON.stringify 会报错)
35+
return false
2936
}
3037
seen.add(val)
3138

3239
if (Array.isArray(val)) {
3340
return val.every((item) => _isSerializable(item))
3441
}
3542

36-
// 检查是否为纯对象(plain object
43+
// Check if it's a plain object
3744
const proto = Object.getPrototypeOf(val)
3845
if (proto !== null && proto !== Object.prototype && proto !== Array.prototype) {
39-
return false // 不是 plain object,比如 class 实例
46+
return false
4047
}
4148

42-
// 检查内置对象(如 DateRegExpMapSet 等)
49+
// Check for built-in objects (Date, RegExp, Map, Set, etc.)
4350
if (
4451
val instanceof Date ||
4552
val instanceof RegExp ||
@@ -52,17 +59,17 @@ export function isSerializable(value: unknown): boolean {
5259
return false
5360
}
5461

55-
// 递归检查所有属性值
62+
// Recursively check all property values
5663
return Object.values(val).every((v) => _isSerializable(v))
5764
}
5865

59-
// functionsymbol 不可序列化
66+
// function, symbol are not serializable
6067
return false
6168
}
6269

6370
try {
6471
return _isSerializable(value)
6572
} catch {
66-
return false // 如出现循环引用错误等
73+
return false
6774
}
6875
}

src/renderer/src/utils/error.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ export const serializeError = (error: AiSdkErrorUnion): SerializedError => {
193193
if ('toolInput' in error) serializedError.toolInput = error.toolInput
194194
if ('text' in error) serializedError.text = error.text ?? null
195195
if ('originalMessage' in error) serializedError.originalMessage = safeSerialize(error.originalMessage)
196-
if ('response' in error) serializedError.response = error.response ?? null
196+
if ('response' in error) serializedError.response = safeSerialize(error.response)
197197
if ('usage' in error) serializedError.usage = safeSerialize(error.usage)
198198
if ('finishReason' in error) serializedError.finishReason = error.finishReason ?? null
199199
if ('modelId' in error) serializedError.modelId = error.modelId

0 commit comments

Comments
 (0)