Skip to content

Commit 4f7405f

Browse files
brandonkachenbrandonkachen
committed
refactor: improve error handling and test quality
- Add validation retry logic with 5s interval for network errors - Refactor test mocks to use explicit type annotations - Extract validation state management in CLI - Add helper functions to reduce test code duplication - Fix type casting to use 'as unknown as typeof fetch' - Add fingerprintId option to SDK client configuration
1 parent 45733c3 commit 4f7405f

6 files changed

Lines changed: 104 additions & 48 deletions

File tree

‎cli/src/hooks/__tests__/use-auth-query.test.ts‎

Lines changed: 30 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -29,12 +29,11 @@ describe('validateApiKey', () => {
2929

3030
describe('successful validation', () => {
3131
test('returns user info for valid API key', async () => {
32-
mockGetUserInfoFromApiKey = mock(() =>
33-
Promise.resolve({
34-
id: 'user-123',
35-
email: 'test@example.com',
36-
}),
37-
)
32+
mockGetUserInfoFromApiKey = mock<GetUserInfoFromApiKeyFn>(async () => ({
33+
id: 'user-123',
34+
email: 'test@example.com',
35+
discord_id: null,
36+
}))
3837

3938
const result = await validateApiKey({
4039
apiKey: 'valid-key',
@@ -50,12 +49,11 @@ describe('validateApiKey', () => {
5049
})
5150

5251
test('passes correct parameters to getUserInfoFromApiKey', async () => {
53-
mockGetUserInfoFromApiKey = mock(() =>
54-
Promise.resolve({
55-
id: 'user-456',
56-
email: 'user@test.com',
57-
}),
58-
)
52+
mockGetUserInfoFromApiKey = mock<GetUserInfoFromApiKeyFn>(async () => ({
53+
id: 'user-456',
54+
email: 'user@test.com',
55+
discord_id: null,
56+
}))
5957

6058
await validateApiKey({
6159
apiKey: 'test-api-key',
@@ -77,7 +75,9 @@ describe('validateApiKey', () => {
7775
) as any
7876
networkError.code = 'NETWORK_ERROR'
7977

80-
mockGetUserInfoFromApiKey = mock(() => Promise.reject(networkError))
78+
mockGetUserInfoFromApiKey = mock<GetUserInfoFromApiKeyFn>(async () => {
79+
throw networkError
80+
})
8181

8282
try {
8383
await validateApiKey({
@@ -96,7 +96,9 @@ describe('validateApiKey', () => {
9696
const networkError = new Error('Network timeout') as any
9797
networkError.code = 'NETWORK_ERROR'
9898

99-
mockGetUserInfoFromApiKey = mock(() => Promise.reject(networkError))
99+
mockGetUserInfoFromApiKey = mock<GetUserInfoFromApiKeyFn>(async () => {
100+
throw networkError
101+
})
100102

101103
try {
102104
await validateApiKey({
@@ -116,7 +118,7 @@ describe('validateApiKey', () => {
116118

117119
describe('authentication errors', () => {
118120
test('throws AUTH_FAILED error for invalid credentials', async () => {
119-
mockGetUserInfoFromApiKey = mock(() => Promise.resolve(null))
121+
mockGetUserInfoFromApiKey = mock<GetUserInfoFromApiKeyFn>(async () => null)
120122

121123
try {
122124
await validateApiKey({
@@ -134,7 +136,9 @@ describe('validateApiKey', () => {
134136
const authError = new Error('Authentication failed') as any
135137
authError.code = 'AUTH_FAILED'
136138

137-
mockGetUserInfoFromApiKey = mock(() => Promise.reject(authError))
139+
mockGetUserInfoFromApiKey = mock<GetUserInfoFromApiKeyFn>(async () => {
140+
throw authError
141+
})
138142

139143
try {
140144
await validateApiKey({
@@ -153,7 +157,9 @@ describe('validateApiKey', () => {
153157
const authError = new Error('Auth failed') as any
154158
authError.code = 'AUTH_FAILED'
155159

156-
mockGetUserInfoFromApiKey = mock(() => Promise.reject(authError))
160+
mockGetUserInfoFromApiKey = mock<GetUserInfoFromApiKeyFn>(async () => {
161+
throw authError
162+
})
157163

158164
try {
159165
await validateApiKey({
@@ -175,7 +181,9 @@ describe('validateApiKey', () => {
175181
test('logs and re-throws unknown errors', async () => {
176182
const unknownError = new Error('Something went wrong')
177183

178-
mockGetUserInfoFromApiKey = mock(() => Promise.reject(unknownError))
184+
mockGetUserInfoFromApiKey = mock<GetUserInfoFromApiKeyFn>(async () => {
185+
throw unknownError
186+
})
179187

180188
try {
181189
await validateApiKey({
@@ -195,7 +203,7 @@ describe('validateApiKey', () => {
195203
})
196204

197205
test('handles null response as authentication error', async () => {
198-
mockGetUserInfoFromApiKey = mock(() => Promise.resolve(null))
206+
mockGetUserInfoFromApiKey = mock<GetUserInfoFromApiKeyFn>(async () => null)
199207

200208
try {
201209
await validateApiKey({
@@ -241,7 +249,9 @@ describe('validateApiKey', () => {
241249
const error = new Error('Test error') as any
242250
error.code = testCase.errorCode
243251

244-
mockGetUserInfoFromApiKey = mock(() => Promise.reject(error))
252+
mockGetUserInfoFromApiKey = mock<GetUserInfoFromApiKeyFn>(async () => {
253+
throw error
254+
})
245255

246256
try {
247257
await validateApiKey({

‎cli/src/index.tsx‎

Lines changed: 50 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ const require = createRequire(import.meta.url)
2525

2626
const INTERNAL_OSC_FLAG = '--internal-osc-detect'
2727
const OSC_DEBUG_ENABLED = process.env.CODEBUFF_OSC_DEBUG === '1'
28+
const VALIDATION_RETRY_INTERVAL_MS = 5000
2829

2930
function logOscDebug(message: string, data?: Record<string, unknown>) {
3031
if (!OSC_DEBUG_ENABLED) return
@@ -157,17 +158,17 @@ async function bootstrapCli(): Promise<void> {
157158

158159
const loadedAgentsData = getLoadedAgentsData()
159160

160-
let validationErrors: Array<{ id: string; message: string }> = []
161-
let validationNetworkError: string | null = null
161+
let initialValidationErrors: Array<{ id: string; message: string }> = []
162+
let initialValidationNetworkError: string | null = null
162163

163164
if (loadedAgentsData) {
164165
const agentDefinitions = loadAgentDefinitions()
165166
const validationResult = await validateAgentsWithNetworkHandling(agentDefinitions, {
166167
remote: true,
167168
})
168169

169-
validationErrors = validationResult.validationErrors
170-
validationNetworkError = validationResult.networkError
170+
initialValidationErrors = validationResult.validationErrors
171+
initialValidationNetworkError = validationResult.networkError
171172
}
172173

173174
const queryClient = createQueryClient()
@@ -177,6 +178,39 @@ async function bootstrapCli(): Promise<void> {
177178
const [hasInvalidCredentials, setHasInvalidCredentials] =
178179
React.useState(false)
179180
const [fileTree, setFileTree] = React.useState<FileTreeNode[]>([])
181+
const [validationErrors, setValidationErrors] = React.useState(
182+
initialValidationErrors,
183+
)
184+
const [validationNetworkError, setValidationNetworkError] =
185+
React.useState(initialValidationNetworkError)
186+
const isValidationInFlight = React.useRef(false)
187+
188+
const refreshValidationState = React.useCallback(async () => {
189+
if (!loadedAgentsData || isValidationInFlight.current) {
190+
return
191+
}
192+
193+
isValidationInFlight.current = true
194+
try {
195+
const agentDefinitions = loadAgentDefinitions()
196+
const validationResult = await validateAgentsWithNetworkHandling(
197+
agentDefinitions,
198+
{ remote: true },
199+
)
200+
201+
setValidationErrors(validationResult.validationErrors)
202+
setValidationNetworkError(validationResult.networkError)
203+
} catch (error) {
204+
logger.warn(
205+
{
206+
error: error instanceof Error ? error.message : String(error),
207+
},
208+
'Agent validation retry failed',
209+
)
210+
} finally {
211+
isValidationInFlight.current = false
212+
}
213+
}, [loadedAgentsData])
180214

181215
React.useEffect(() => {
182216
const userCredentials = getUserCredentials()
@@ -213,6 +247,18 @@ async function bootstrapCli(): Promise<void> {
213247
loadFileTree()
214248
}, [])
215249

250+
React.useEffect(() => {
251+
if (!loadedAgentsData || !validationNetworkError) {
252+
return
253+
}
254+
255+
const interval = setInterval(() => {
256+
void refreshValidationState()
257+
}, VALIDATION_RETRY_INTERVAL_MS)
258+
259+
return () => clearInterval(interval)
260+
}, [loadedAgentsData, validationNetworkError, refreshValidationState])
261+
216262
return (
217263
// Hi!
218264
<App

‎sdk/src/__tests__/database-error-handling.test.ts‎

Lines changed: 14 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,10 @@ import { getUserInfoFromApiKey } from '../impl/database'
33
import type { Logger } from '@codebuff/common/types/contracts/logger'
44
import { isAuthenticationError, isNetworkError } from '../errors'
55

6+
const setMockFetch = (impl: () => Promise<any>) => {
7+
globalThis.fetch = mock(impl) as unknown as typeof fetch
8+
}
9+
610
describe('getUserInfoFromApiKey error handling', () => {
711
const originalFetch = globalThis.fetch
812
let mockLogger: Logger
@@ -22,7 +26,7 @@ describe('getUserInfoFromApiKey error handling', () => {
2226

2327
describe('authentication errors', () => {
2428
test('throws AUTH_FAILED error for 401 status', async () => {
25-
globalThis.fetch = mock(() =>
29+
setMockFetch(() =>
2630
Promise.resolve({
2731
ok: false,
2832
status: 401,
@@ -47,7 +51,7 @@ describe('getUserInfoFromApiKey error handling', () => {
4751
})
4852

4953
test('throws AUTH_FAILED error for 403 status', async () => {
50-
globalThis.fetch = mock(() =>
54+
setMockFetch(() =>
5155
Promise.resolve({
5256
ok: false,
5357
status: 403,
@@ -72,7 +76,7 @@ describe('getUserInfoFromApiKey error handling', () => {
7276
})
7377

7478
test('throws NETWORK_ERROR for 500 server errors', async () => {
75-
globalThis.fetch = mock(() =>
79+
setMockFetch(() =>
7680
Promise.resolve({
7781
ok: false,
7882
status: 500,
@@ -100,9 +104,7 @@ describe('getUserInfoFromApiKey error handling', () => {
100104

101105
describe('network errors', () => {
102106
test('throws NETWORK_ERROR for fetch failures', async () => {
103-
globalThis.fetch = mock(() =>
104-
Promise.reject(new Error('Network request failed')),
105-
)
107+
setMockFetch(() => Promise.reject(new Error('Network request failed')))
106108

107109
try {
108110
await getUserInfoFromApiKey({
@@ -119,9 +121,7 @@ describe('getUserInfoFromApiKey error handling', () => {
119121
})
120122

121123
test('throws NETWORK_ERROR for connection timeout', async () => {
122-
globalThis.fetch = mock(() =>
123-
Promise.reject(new Error('Connection timeout')),
124-
)
124+
setMockFetch(() => Promise.reject(new Error('Connection timeout')))
125125

126126
try {
127127
await getUserInfoFromApiKey({
@@ -137,9 +137,7 @@ describe('getUserInfoFromApiKey error handling', () => {
137137
})
138138

139139
test('logs network errors with masked API key', async () => {
140-
globalThis.fetch = mock(() =>
141-
Promise.reject(new Error('Network failure')),
142-
)
140+
setMockFetch(() => Promise.reject(new Error('Network failure')))
143141

144142
try {
145143
await getUserInfoFromApiKey({
@@ -160,7 +158,7 @@ describe('getUserInfoFromApiKey error handling', () => {
160158

161159
describe('successful requests', () => {
162160
test('returns user info for valid API key', async () => {
163-
globalThis.fetch = mock(() =>
161+
setMockFetch(() =>
164162
Promise.resolve({
165163
ok: true,
166164
json: () =>
@@ -195,7 +193,7 @@ describe('getUserInfoFromApiKey error handling', () => {
195193
}),
196194
} as Response),
197195
)
198-
globalThis.fetch = mockFetch as any
196+
globalThis.fetch = mockFetch as unknown as typeof fetch
199197

200198
const apiKey = 'cache-test-key'
201199

@@ -220,7 +218,7 @@ describe('getUserInfoFromApiKey error handling', () => {
220218

221219
describe('error propagation', () => {
222220
test('re-throws AUTH_FAILED errors without wrapping', async () => {
223-
globalThis.fetch = mock(() =>
221+
setMockFetch(() =>
224222
Promise.resolve({
225223
ok: false,
226224
status: 401,
@@ -241,9 +239,7 @@ describe('getUserInfoFromApiKey error handling', () => {
241239
})
242240

243241
test('wraps non-auth errors in NETWORK_ERROR', async () => {
244-
globalThis.fetch = mock(() =>
245-
Promise.reject(new Error('Random error')),
246-
)
242+
setMockFetch(() => Promise.reject(new Error('Random error')))
247243

248244
try {
249245
await getUserInfoFromApiKey({

‎sdk/src/__tests__/validate-agents-network-errors.test.ts‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ describe('validateAgents network error handling', () => {
88

99
beforeEach(() => {
1010
mockFetch = mock()
11-
globalThis.fetch = mockFetch as any
11+
globalThis.fetch = mockFetch as unknown as typeof fetch
1212
})
1313

1414
afterEach(() => {
@@ -18,8 +18,7 @@ describe('validateAgents network error handling', () => {
1818
const testAgent: AgentDefinition = {
1919
id: 'test-agent',
2020
displayName: 'Test Agent',
21-
description: 'Test agent for validation',
22-
tools: [],
21+
model: 'openai/gpt-5-mini',
2322
}
2423

2524
describe('network errors (thrown)', () => {
@@ -297,4 +296,4 @@ describe('validateAgents network error handling', () => {
297296
expect(result).toHaveProperty('success')
298297
})
299298
})
300-
})
299+
})

‎sdk/src/client.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,11 +32,15 @@ export class CodebuffClient {
3232
}
3333
: undefined
3434

35+
const fingerprintId =
36+
options.fingerprintId ??
37+
`codebuff-sdk-${Math.random().toString(36).substring(2, 15)}`
38+
3539
this.options = {
3640
apiKey: foundApiKey,
37-
handleEvent: options.handleEvent || defaultHandleEvent,
38-
fingerprintId: `codebuff-sdk-${Math.random().toString(36).substring(2, 15)}`,
3941
...options,
42+
fingerprintId,
43+
handleEvent: options.handleEvent ?? defaultHandleEvent,
4044
}
4145
}
4246

‎sdk/src/run.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ import type { CodebuffSpawn } from '@codebuff/common/types/spawn'
4949

5050
export type CodebuffClientOptions = {
5151
apiKey?: string
52+
fingerprintId?: string
5253

5354
cwd?: string
5455
projectFiles?: Record<string, string>

0 commit comments

Comments
 (0)