Skip to content

Commit 4a4413d

Browse files
claudeclaude
committed
fix: address PR #109 review comments for error handling and tests
- Reorder error detection in parseAPIError: HTTP status codes are now parsed before Ollama-specific patterns. This ensures 500 errors containing "invalid character" are correctly handled as server errors, not Ollama errors. - Export parseAPIError function for direct testing instead of duplicating logic - Add getMaxRetries() getter to AISDKClient for testability - Update maxRetries tests to verify actual client behavior: - Test client instantiation with various maxRetries values - Verify 0 is correctly handled (not treated as falsy) - Verify undefined falls back to default of 2 - Update error-handling tests to use exported parseAPIError function - Remove duplicated test function - Use exact assertions instead of OR conditions - Fix 500 error test to expect "Server error" (status code precedence) - Add comprehensive test coverage for all error types Addresses Copilot review comments on PR #109.
1 parent b243f4d commit 4a4413d

File tree

3 files changed

+174
-139
lines changed

3 files changed

+174
-139
lines changed

‎source/ai-sdk-client-error-handling.spec.ts‎

Lines changed: 88 additions & 106 deletions
Original file line numberDiff line numberDiff line change
@@ -1,101 +1,15 @@
11
import test from 'ava';
2+
import {parseAPIError} from './ai-sdk-client.js';
23

3-
// Note: parseAPIError is an internal function in ai-sdk-client.ts that is not exported.
4-
// For testing purposes, we replicate the logic here to verify it works correctly.
5-
6-
/**
7-
* Parses API errors into user-friendly messages
8-
* This is a copy of the internal parseAPIError function for testing
9-
*/
10-
function parseAPIErrorForTest(error: unknown): string {
11-
if (!(error instanceof Error)) {
12-
return 'An unknown error occurred while communicating with the model';
13-
}
14-
15-
const errorMessage = error.message;
16-
17-
// Handle Ollama-specific unmarshal/JSON parsing errors
18-
if (
19-
errorMessage.includes('unmarshal') ||
20-
(errorMessage.includes('invalid character') &&
21-
errorMessage.includes('after top-level value'))
22-
) {
23-
return (
24-
'Ollama server error: The model returned malformed JSON. ' +
25-
'This usually indicates an issue with the Ollama server or model. ' +
26-
'Try:\n' +
27-
' 1. Restart Ollama: systemctl restart ollama (Linux) or restart the Ollama app\n' +
28-
' 2. Re-pull the model: ollama pull <model-name>\n' +
29-
' 3. Check Ollama logs for more details\n' +
30-
' 4. Try a different model to see if the issue is model-specific\n' +
31-
`Original error: ${errorMessage}`
32-
);
33-
}
34-
35-
// Extract status code and clean message from common error patterns
36-
const statusMatch = errorMessage.match(
37-
/(?:Error: )?(\d{3})\s+(?:\d{3}\s+)?(?:Bad Request|[^:]+):\s*(.+)/i,
38-
);
39-
if (statusMatch) {
40-
const [, statusCode, message] = statusMatch;
41-
const cleanMessage = message.trim();
42-
43-
switch (statusCode) {
44-
case '400':
45-
return `Bad request: ${cleanMessage}`;
46-
case '401':
47-
return 'Authentication failed: Invalid API key or credentials';
48-
case '403':
49-
return 'Access forbidden: Check your API permissions';
50-
case '404':
51-
return 'Model not found: The requested model may not exist or is unavailable';
52-
case '429':
53-
return 'Rate limit exceeded: Too many requests. Please wait and try again';
54-
case '500':
55-
case '502':
56-
case '503':
57-
return `Server error: ${cleanMessage}`;
58-
default:
59-
return `Request failed (${statusCode}): ${cleanMessage}`;
60-
}
61-
}
62-
63-
// Handle timeout errors
64-
if (errorMessage.includes('timeout') || errorMessage.includes('ETIMEDOUT')) {
65-
return 'Request timed out: The model took too long to respond';
66-
}
67-
68-
// Handle network errors
69-
if (
70-
errorMessage.includes('ECONNREFUSED') ||
71-
errorMessage.includes('connect')
72-
) {
73-
return 'Connection failed: Unable to reach the model server';
74-
}
75-
76-
// Handle context length errors
77-
if (
78-
errorMessage.includes('context length') ||
79-
errorMessage.includes('too many tokens')
80-
) {
81-
return 'Context too large: Please reduce the conversation length or message size';
82-
}
83-
84-
// Handle token limit errors
85-
if (errorMessage.includes('reduce the number of tokens')) {
86-
return 'Too many tokens: Please shorten your message or clear conversation history';
87-
}
88-
89-
// If we can't parse it, return a cleaned up version
90-
return errorMessage.replace(/^Error:\s*/i, '').split('\n')[0];
91-
}
4+
// Tests for parseAPIError function
5+
// Now using the actual exported function instead of a duplicated copy
926

937
test('parseAPIError - handles Ollama unmarshal error from issue #87', t => {
948
const error = new Error(
959
"RetryError [AI_RetryError]: Failed after 3 attempts. Last error: unmarshal: invalid character '{' after top-level value",
9610
);
9711

98-
const result = parseAPIErrorForTest(error);
12+
const result = parseAPIError(error);
9913

10014
t.true(result.includes('Ollama server error'));
10115
t.true(result.includes('malformed JSON'));
@@ -109,35 +23,38 @@ test('parseAPIError - handles Ollama unmarshal error from issue #87', t => {
10923
test('parseAPIError - handles unmarshal error without retry wrapper', t => {
11024
const error = new Error("unmarshal: invalid character '{' after top-level value");
11125

112-
const result = parseAPIErrorForTest(error);
26+
const result = parseAPIError(error);
11327

11428
t.true(result.includes('Ollama server error'));
11529
t.true(result.includes('malformed JSON'));
11630
});
11731

118-
test('parseAPIError - handles invalid character error', t => {
32+
test('parseAPIError - handles 500 error with invalid character (status code takes precedence)', t => {
33+
// This test verifies that HTTP status codes are parsed FIRST,
34+
// so a 500 error with "invalid character" in the message is treated
35+
// as a server error, not an Ollama-specific error
11936
const error = new Error(
12037
"500 Internal Server Error: invalid character 'x' after top-level value",
12138
);
12239

123-
const result = parseAPIErrorForTest(error);
40+
const result = parseAPIError(error);
12441

125-
t.true(result.includes('Ollama server error'));
126-
t.true(result.includes('malformed JSON'));
42+
// Status code parsing takes precedence over Ollama-specific pattern matching
43+
t.is(result, "Server error: invalid character 'x' after top-level value");
12744
});
12845

12946
test('parseAPIError - handles 500 error without JSON parsing issue', t => {
13047
const error = new Error('500 Internal Server Error: database connection failed');
13148

132-
const result = parseAPIErrorForTest(error);
49+
const result = parseAPIError(error);
13350

13451
t.is(result, 'Server error: database connection failed');
13552
});
13653

13754
test('parseAPIError - handles 404 error', t => {
13855
const error = new Error('404 Not Found: model not available');
13956

140-
const result = parseAPIErrorForTest(error);
57+
const result = parseAPIError(error);
14158

14259
t.is(
14360
result,
@@ -148,45 +65,110 @@ test('parseAPIError - handles 404 error', t => {
14865
test('parseAPIError - handles connection refused', t => {
14966
const error = new Error('ECONNREFUSED: Connection refused');
15067

151-
const result = parseAPIErrorForTest(error);
68+
const result = parseAPIError(error);
15269

15370
t.is(result, 'Connection failed: Unable to reach the model server');
15471
});
15572

15673
test('parseAPIError - handles timeout error', t => {
15774
const error = new Error('Request timeout: ETIMEDOUT');
15875

159-
const result = parseAPIErrorForTest(error);
76+
const result = parseAPIError(error);
16077

16178
t.is(result, 'Request timed out: The model took too long to respond');
16279
});
16380

16481
test('parseAPIError - handles non-Error objects', t => {
165-
const result = parseAPIErrorForTest('string error');
82+
const result = parseAPIError('string error');
16683

16784
t.is(result, 'An unknown error occurred while communicating with the model');
16885
});
16986

17087
test('parseAPIError - handles context length errors', t => {
17188
const error = new Error(
172-
'context length exceeded, please reduce the number of tokens',
89+
'context length exceeded',
17390
);
17491

175-
const result = parseAPIErrorForTest(error);
92+
const result = parseAPIError(error);
93+
94+
// Use exact assertion instead of OR condition
95+
t.is(result, 'Context too large: Please reduce the conversation length or message size');
96+
});
17697

177-
t.true(
178-
result.includes('Context too large') ||
179-
result.includes('Too many tokens'),
98+
test('parseAPIError - handles too many tokens errors', t => {
99+
const error = new Error(
100+
'too many tokens in the request',
180101
);
102+
103+
const result = parseAPIError(error);
104+
105+
t.is(result, 'Context too large: Please reduce the conversation length or message size');
181106
});
182107

183108
test('parseAPIError - handles 400 with context length in message', t => {
184109
const error = new Error(
185110
'400 Bad Request: context length exceeded',
186111
);
187112

188-
const result = parseAPIErrorForTest(error);
113+
const result = parseAPIError(error);
189114

190115
// The 400 status code pattern matches first, so we get the full message
191116
t.is(result, 'Bad request: context length exceeded');
192117
});
118+
119+
test('parseAPIError - handles 401 authentication error', t => {
120+
const error = new Error('401 Unauthorized: Invalid API key');
121+
122+
const result = parseAPIError(error);
123+
124+
t.is(result, 'Authentication failed: Invalid API key or credentials');
125+
});
126+
127+
test('parseAPIError - handles 403 forbidden error', t => {
128+
const error = new Error('403 Forbidden: Access denied');
129+
130+
const result = parseAPIError(error);
131+
132+
t.is(result, 'Access forbidden: Check your API permissions');
133+
});
134+
135+
test('parseAPIError - handles 429 rate limit error', t => {
136+
const error = new Error('429 Too Many Requests: Rate limit exceeded');
137+
138+
const result = parseAPIError(error);
139+
140+
t.is(result, 'Rate limit exceeded: Too many requests. Please wait and try again');
141+
});
142+
143+
test('parseAPIError - handles 502 bad gateway error', t => {
144+
const error = new Error('502 Bad Gateway: upstream error');
145+
146+
const result = parseAPIError(error);
147+
148+
t.is(result, 'Server error: upstream error');
149+
});
150+
151+
test('parseAPIError - handles 503 service unavailable error', t => {
152+
const error = new Error('503 Service Unavailable: server overloaded');
153+
154+
const result = parseAPIError(error);
155+
156+
t.is(result, 'Server error: server overloaded');
157+
});
158+
159+
test('parseAPIError - handles reduce tokens message', t => {
160+
const error = new Error('Please reduce the number of tokens in your request');
161+
162+
const result = parseAPIError(error);
163+
164+
t.is(result, 'Too many tokens: Please shorten your message or clear conversation history');
165+
});
166+
167+
test('parseAPIError - cleans up unknown errors', t => {
168+
const error = new Error('Error: Something unexpected happened\nWith more details');
169+
170+
const result = parseAPIError(error);
171+
172+
// Should strip "Error: " prefix and only return first line
173+
t.is(result, 'Something unexpected happened');
174+
});

‎source/ai-sdk-client-maxretries.spec.ts‎

Lines changed: 56 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,11 @@
11
import test from 'ava';
2+
import {AISDKClient} from './ai-sdk-client.js';
23
import type {AIProviderConfig} from './types/config.js';
34

4-
test('AISDKClient - maxRetries configuration default value', t => {
5-
// Test that maxRetries defaults to 2 when not specified
5+
// Tests for maxRetries configuration
6+
// Now tests actual AISDKClient instantiation and behavior
7+
8+
test('AISDKClient - maxRetries defaults to 2 when not specified', t => {
69
const config: AIProviderConfig = {
710
name: 'TestProvider',
811
type: 'openai-compatible',
@@ -13,15 +16,13 @@ test('AISDKClient - maxRetries configuration default value', t => {
1316
},
1417
};
1518

16-
// Verify default is 2 (AI SDK default)
17-
const expectedDefault = 2;
18-
const actualDefault = config.maxRetries ?? 2;
19+
const client = new AISDKClient(config);
1920

20-
t.is(actualDefault, expectedDefault);
21+
// Verify the client's internal maxRetries is set to default of 2
22+
t.is(client.getMaxRetries(), 2);
2123
});
2224

23-
test('AISDKClient - maxRetries configuration custom value', t => {
24-
// Test that maxRetries can be set to a custom value
25+
test('AISDKClient - maxRetries respects custom value', t => {
2526
const config: AIProviderConfig = {
2627
name: 'TestProvider',
2728
type: 'openai-compatible',
@@ -33,11 +34,15 @@ test('AISDKClient - maxRetries configuration custom value', t => {
3334
},
3435
};
3536

36-
t.is(config.maxRetries, 5);
37+
const client = new AISDKClient(config);
38+
39+
// Verify the client uses the custom maxRetries value
40+
t.is(client.getMaxRetries(), 5);
3741
});
3842

39-
test('AISDKClient - maxRetries configuration zero retries', t => {
40-
// Test that maxRetries can be set to 0 to disable retries
43+
test('AISDKClient - maxRetries can be set to 0 to disable retries', t => {
44+
// Important: This test verifies that 0 is treated as a valid value,
45+
// not as falsy (which would incorrectly default to 2)
4146
const config: AIProviderConfig = {
4247
name: 'TestProvider',
4348
type: 'openai-compatible',
@@ -49,7 +54,27 @@ test('AISDKClient - maxRetries configuration zero retries', t => {
4954
},
5055
};
5156

52-
t.is(config.maxRetries, 0);
57+
const client = new AISDKClient(config);
58+
59+
// Verify that 0 is respected (nullish coalescing handles this correctly)
60+
t.is(client.getMaxRetries(), 0);
61+
});
62+
63+
test('AISDKClient - maxRetries handles value of 1', t => {
64+
const config: AIProviderConfig = {
65+
name: 'TestProvider',
66+
type: 'openai-compatible',
67+
models: ['test-model'],
68+
maxRetries: 1,
69+
config: {
70+
baseURL: 'http://localhost:11434/v1',
71+
apiKey: 'test-key',
72+
},
73+
};
74+
75+
const client = new AISDKClient(config);
76+
77+
t.is(client.getMaxRetries(), 1);
5378
});
5479

5580
test('AIProviderConfig type - includes maxRetries in interface', t => {
@@ -68,3 +93,22 @@ test('AIProviderConfig type - includes maxRetries in interface', t => {
6893
t.is(typeof config.maxRetries, 'number');
6994
t.true('maxRetries' in config);
7095
});
96+
97+
test('AISDKClient - undefined maxRetries uses default', t => {
98+
// Explicitly set to undefined to test fallback behavior
99+
const config: AIProviderConfig = {
100+
name: 'TestProvider',
101+
type: 'openai-compatible',
102+
models: ['test-model'],
103+
maxRetries: undefined,
104+
config: {
105+
baseURL: 'http://localhost:11434/v1',
106+
apiKey: 'test-key',
107+
},
108+
};
109+
110+
const client = new AISDKClient(config);
111+
112+
// Verify undefined falls back to default of 2
113+
t.is(client.getMaxRetries(), 2);
114+
});

0 commit comments

Comments
 (0)