Skip to content

Commit a4e95ea

Browse files
haasonsaasclaude
haasonsaas
and
claude
committed
refactor: Implement dedicated error classes for precise error handling
- Create custom error classes (SessionError, ApiError, FileSystemError, RateLimitError, etc.) - Replace broad message.includes() checks with instanceof checks in ErrorClassifier - Update all services to throw specific error types instead of generic Errors - Add comprehensive tests for new error classes - Maintain backward compatibility for native filesystem errors This addresses Copilot's feedback about using dedicated error classes instead of relying on error message content for classification. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
1 parent 758ff9e commit a4e95ea

10 files changed

+300
-35
lines changed

‎src/__tests__/ConversationManager.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ describe('ConversationManager', () => {
108108
it('should throw error when adding turn to non-existent session', () => {
109109
expect(() => {
110110
manager.addTurn('non-existent', 'claude', 'Test');
111-
}).toThrow('Session non-existent is not active');
111+
}).toThrow('Session non-existent not found or expired');
112112
});
113113
});
114114

‎src/__tests__/ConversationalGeminiService.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -149,15 +149,15 @@ describe('ConversationalGeminiService', () => {
149149

150150
await expect(
151151
service.continueConversation('non-existent', 'test message')
152-
).rejects.toThrow('No active conversation found');
152+
).rejects.toThrow('Session non-existent not found or expired');
153153
});
154154

155155
it('should throw error for non-existent session in finalizeConversation', async () => {
156156
const service = new ConversationalGeminiService('test-api-key');
157157

158158
await expect(
159159
service.finalizeConversation('non-existent')
160-
).rejects.toThrow('No active conversation found');
160+
).rejects.toThrow('Session non-existent not found or expired');
161161
});
162162
});
163163
});

‎src/__tests__/errors.test.ts

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,152 @@
1+
import { describe, it, expect, jest } from '@jest/globals';
2+
import {
3+
SessionError,
4+
ApiError,
5+
FileSystemError,
6+
RateLimitError,
7+
ConversationLockedError,
8+
SessionNotFoundError,
9+
} from '../errors/index.js';
10+
import { ErrorClassifier } from '../utils/ErrorClassifier.js';
11+
12+
describe('Custom Error Classes', () => {
13+
describe('SessionError', () => {
14+
it('should create a session error with proper properties', () => {
15+
const error = new SessionError('Test message', 'TEST_CODE', 'session-123');
16+
17+
expect(error).toBeInstanceOf(Error);
18+
expect(error).toBeInstanceOf(SessionError);
19+
expect(error.name).toBe('SessionError');
20+
expect(error.message).toBe('Test message');
21+
expect(error.code).toBe('TEST_CODE');
22+
expect(error.sessionId).toBe('session-123');
23+
});
24+
});
25+
26+
describe('ApiError', () => {
27+
it('should create an API error with proper properties', () => {
28+
const error = new ApiError('API failed', 'API_ERROR', 'gemini', 500);
29+
30+
expect(error).toBeInstanceOf(Error);
31+
expect(error).toBeInstanceOf(ApiError);
32+
expect(error.name).toBe('ApiError');
33+
expect(error.message).toBe('API failed');
34+
expect(error.code).toBe('API_ERROR');
35+
expect(error.service).toBe('gemini');
36+
expect(error.statusCode).toBe(500);
37+
});
38+
});
39+
40+
describe('FileSystemError', () => {
41+
it('should create a filesystem error with proper properties', () => {
42+
const error = new FileSystemError('File not found', 'ENOENT', '/path/to/file', 'read');
43+
44+
expect(error).toBeInstanceOf(Error);
45+
expect(error).toBeInstanceOf(FileSystemError);
46+
expect(error.name).toBe('FileSystemError');
47+
expect(error.message).toBe('File not found');
48+
expect(error.code).toBe('ENOENT');
49+
expect(error.path).toBe('/path/to/file');
50+
expect(error.operation).toBe('read');
51+
});
52+
});
53+
54+
describe('RateLimitError', () => {
55+
it('should create a rate limit error with proper properties', () => {
56+
const error = new RateLimitError('Rate limit exceeded', 'gemini', 60);
57+
58+
expect(error).toBeInstanceOf(Error);
59+
expect(error).toBeInstanceOf(ApiError);
60+
expect(error).toBeInstanceOf(RateLimitError);
61+
expect(error.name).toBe('RateLimitError');
62+
expect(error.message).toBe('Rate limit exceeded');
63+
expect(error.code).toBe('RATE_LIMIT_ERROR');
64+
expect(error.service).toBe('gemini');
65+
expect(error.statusCode).toBe(429);
66+
expect(error.retryAfter).toBe(60);
67+
});
68+
});
69+
70+
describe('ConversationLockedError', () => {
71+
it('should create a conversation locked error', () => {
72+
const error = new ConversationLockedError('session-123');
73+
74+
expect(error).toBeInstanceOf(SessionError);
75+
expect(error.name).toBe('ConversationLockedError');
76+
expect(error.message).toBe('Session session-123 is currently processing another request');
77+
expect(error.code).toBe('SESSION_LOCKED');
78+
expect(error.sessionId).toBe('session-123');
79+
});
80+
});
81+
82+
describe('SessionNotFoundError', () => {
83+
it('should create a session not found error', () => {
84+
const error = new SessionNotFoundError('session-456');
85+
86+
expect(error).toBeInstanceOf(SessionError);
87+
expect(error.name).toBe('SessionNotFoundError');
88+
expect(error.message).toBe('Session session-456 not found or expired');
89+
expect(error.code).toBe('SESSION_NOT_FOUND');
90+
expect(error.sessionId).toBe('session-456');
91+
});
92+
});
93+
});
94+
95+
describe('ErrorClassifier with Custom Errors', () => {
96+
it('should classify SessionError correctly', () => {
97+
const error = new SessionError('Test', 'SESSION_ERROR', 'session-123');
98+
const classification = ErrorClassifier.classify(error);
99+
100+
expect(classification.category).toBe('session');
101+
expect(classification.code).toBe('SESSION_ERROR');
102+
expect(classification.isRetryable).toBe(false);
103+
});
104+
105+
it('should classify ConversationLockedError as retryable', () => {
106+
const error = new ConversationLockedError('session-123');
107+
const classification = ErrorClassifier.classify(error);
108+
109+
expect(classification.category).toBe('session');
110+
expect(classification.code).toBe('SESSION_LOCKED');
111+
expect(classification.isRetryable).toBe(true);
112+
});
113+
114+
it('should classify RateLimitError correctly', () => {
115+
const error = new RateLimitError('Rate limited', 'gemini');
116+
const classification = ErrorClassifier.classify(error);
117+
118+
expect(classification.category).toBe('api');
119+
expect(classification.code).toBe('RATE_LIMIT_ERROR');
120+
expect(classification.isRetryable).toBe(true);
121+
});
122+
123+
it('should classify FileSystemError correctly', () => {
124+
const error = new FileSystemError('File error', 'ENOENT', '/path');
125+
const classification = ErrorClassifier.classify(error);
126+
127+
expect(classification.category).toBe('filesystem');
128+
expect(classification.code).toBe('ENOENT');
129+
expect(classification.isRetryable).toBe(false);
130+
});
131+
132+
it('should still handle native filesystem errors', () => {
133+
const error = new Error('ENOENT: no such file or directory');
134+
(error as any).code = 'ENOENT';
135+
136+
const classification = ErrorClassifier.classify(error);
137+
138+
expect(classification.category).toBe('filesystem');
139+
expect(classification.code).toBe('ENOENT');
140+
expect(classification.isRetryable).toBe(false);
141+
});
142+
143+
it('should handle GoogleGenerativeAIError for backward compatibility', () => {
144+
const error = new Error('GoogleGenerativeAIError: Invalid API key');
145+
146+
const classification = ErrorClassifier.classify(error);
147+
148+
expect(classification.category).toBe('api');
149+
expect(classification.code).toBe('API_AUTH_ERROR');
150+
expect(classification.isRetryable).toBe(false);
151+
});
152+
});

‎src/analyzers/DeepCodeReasonerV2.ts

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import { ConversationalGeminiService } from '../services/ConversationalGeminiSer
88
import { ConversationManager } from '../services/ConversationManager.js';
99
import { CodeReader } from '../utils/CodeReader.js';
1010
import { ErrorClassifier } from '../utils/ErrorClassifier.js';
11+
import { ConversationLockedError, SessionNotFoundError } from '../errors/index.js';
1112

1213
export class DeepCodeReasonerV2 {
1314
private geminiService: GeminiService;
@@ -396,14 +397,14 @@ export class DeepCodeReasonerV2 {
396397
// Acquire lock before processing
397398
const lockAcquired = this.conversationManager.acquireLock(sessionId);
398399
if (!lockAcquired) {
399-
throw new Error(`Session ${sessionId} is currently processing another request or not available`);
400+
throw new ConversationLockedError(sessionId);
400401
}
401402

402403
try {
403404
// Validate session
404405
const session = this.conversationManager.getSession(sessionId);
405406
if (!session) {
406-
throw new Error(`Session ${sessionId} not found or expired`);
407+
throw new SessionNotFoundError(sessionId);
407408
}
408409

409410
// Add Claude's message to conversation history
@@ -446,14 +447,14 @@ export class DeepCodeReasonerV2 {
446447
// Acquire lock before processing
447448
const lockAcquired = this.conversationManager.acquireLock(sessionId);
448449
if (!lockAcquired) {
449-
throw new Error(`Session ${sessionId} is currently processing another request or not available`);
450+
throw new ConversationLockedError(sessionId);
450451
}
451452

452453
try {
453454
// Validate session
454455
const session = this.conversationManager.getSession(sessionId);
455456
if (!session) {
456-
throw new Error(`Session ${sessionId} not found or expired`);
457+
throw new SessionNotFoundError(sessionId);
457458
}
458459

459460
// Get final analysis from Gemini

‎src/errors/index.ts

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
/**
2+
* Custom error classes for precise error handling
3+
*/
4+
5+
export class SessionError extends Error {
6+
readonly code: string;
7+
readonly sessionId?: string;
8+
9+
constructor(message: string, code: string = 'SESSION_ERROR', sessionId?: string) {
10+
super(message);
11+
this.name = 'SessionError';
12+
this.code = code;
13+
this.sessionId = sessionId;
14+
}
15+
}
16+
17+
export class ApiError extends Error {
18+
readonly code: string;
19+
readonly statusCode?: number;
20+
readonly service: string;
21+
22+
constructor(message: string, code: string = 'API_ERROR', service: string = 'unknown', statusCode?: number) {
23+
super(message);
24+
this.name = 'ApiError';
25+
this.code = code;
26+
this.service = service;
27+
this.statusCode = statusCode;
28+
}
29+
}
30+
31+
export class FileSystemError extends Error {
32+
readonly code: string;
33+
readonly path?: string;
34+
readonly operation?: string;
35+
36+
constructor(message: string, code: string = 'FS_ERROR', path?: string, operation?: string) {
37+
super(message);
38+
this.name = 'FileSystemError';
39+
this.code = code;
40+
this.path = path;
41+
this.operation = operation;
42+
}
43+
}
44+
45+
export class RateLimitError extends ApiError {
46+
readonly retryAfter?: number;
47+
48+
constructor(message: string, service: string = 'gemini', retryAfter?: number) {
49+
super(message, 'RATE_LIMIT_ERROR', service, 429);
50+
this.name = 'RateLimitError';
51+
this.retryAfter = retryAfter;
52+
}
53+
}
54+
55+
export class ConversationLockedError extends SessionError {
56+
constructor(sessionId: string) {
57+
super(`Session ${sessionId} is currently processing another request`, 'SESSION_LOCKED', sessionId);
58+
this.name = 'ConversationLockedError';
59+
}
60+
}
61+
62+
export class SessionNotFoundError extends SessionError {
63+
constructor(sessionId: string) {
64+
super(`Session ${sessionId} not found or expired`, 'SESSION_NOT_FOUND', sessionId);
65+
this.name = 'SessionNotFoundError';
66+
}
67+
}

‎src/services/ConversationManager.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { v4 as uuidv4 } from 'uuid';
22
import { ChatSession } from '@google/generative-ai';
33
import { ClaudeCodeContext, DeepAnalysisResult } from '../models/types.js';
4+
import { SessionError, SessionNotFoundError } from '../errors/index.js';
45

56
export interface ConversationTurn {
67
id: string;
@@ -121,8 +122,11 @@ export class ConversationManager {
121122

122123
addTurn(sessionId: string, role: ConversationTurn['role'], content: string, metadata?: ConversationTurn['metadata']): void {
123124
const session = this.getSession(sessionId);
124-
if (!session || (session.status !== 'active' && session.status !== 'processing')) {
125-
throw new Error(`Session ${sessionId} is not active or processing`);
125+
if (!session) {
126+
throw new SessionNotFoundError(sessionId);
127+
}
128+
if (session.status !== 'active' && session.status !== 'processing') {
129+
throw new SessionError(`Session ${sessionId} is not active or processing`, 'SESSION_INVALID_STATE', sessionId);
126130
}
127131

128132
const turn: ConversationTurn = {
@@ -172,7 +176,7 @@ export class ConversationManager {
172176
extractResults(sessionId: string): DeepAnalysisResult {
173177
const session = this.getSession(sessionId);
174178
if (!session) {
175-
throw new Error(`Session ${sessionId} not found`);
179+
throw new SessionNotFoundError(sessionId);
176180
}
177181

178182
// Synthesize all findings from the conversation

‎src/services/ConversationalGeminiService.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import type {
44
DeepAnalysisResult,
55
CodeLocation,
66
} from '../models/types.js';
7+
import { SessionError, SessionNotFoundError } from '../errors/index.js';
78

89
export interface ConversationContext {
910
sessionId: string;
@@ -101,7 +102,7 @@ export class ConversationalGeminiService {
101102
const context = this.sessionContexts.get(sessionId);
102103

103104
if (!chat || !context) {
104-
throw new Error(`No active conversation found for session ${sessionId}`);
105+
throw new SessionNotFoundError(sessionId);
105106
}
106107

107108
// Process Claude's message
@@ -133,7 +134,7 @@ export class ConversationalGeminiService {
133134
const context = this.sessionContexts.get(sessionId);
134135

135136
if (!chat || !context) {
136-
throw new Error(`No active conversation found for session ${sessionId}`);
137+
throw new SessionNotFoundError(sessionId);
137138
}
138139

139140
// Request final synthesis

‎src/services/GeminiService.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import type {
88
SystemImpact,
99
RootCause,
1010
} from '../models/types.js';
11+
import { ApiError, RateLimitError } from '../errors/index.js';
1112

1213
export class GeminiService {
1314
private genAI: GoogleGenerativeAI;
@@ -43,7 +44,12 @@ export class GeminiService {
4344
return this.parseGeminiResponse(text, context);
4445
} catch (error) {
4546
console.error('Gemini API error:', error);
46-
throw new Error(`Gemini analysis failed: ${error}`);
47+
// Check for rate limit errors
48+
const errorMessage = error instanceof Error ? error.message : String(error);
49+
if (errorMessage.includes('rate limit') || errorMessage.includes('quota')) {
50+
throw new RateLimitError(`Gemini API rate limit exceeded: ${errorMessage}`, 'gemini');
51+
}
52+
throw new ApiError(`Gemini analysis failed: ${errorMessage}`, 'GEMINI_API_ERROR', 'gemini');
4753
}
4854
}
4955

‎src/utils/CodeReader.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import * as fs from 'fs/promises';
22
import * as path from 'path';
33
import type { CodeScope, CodeLocation } from '../models/types.js';
4+
import { FileSystemError } from '../errors/index.js';
45

56
export class CodeReader {
67
private cache: Map<string, string> = new Map();
@@ -46,7 +47,16 @@ export class CodeReader {
4647
this.cache.set(filePath, content);
4748
return content;
4849
} catch (error) {
49-
throw new Error(`Cannot read file ${filePath}: ${error}`);
50+
if (error instanceof Error) {
51+
const code = (error as any).code || 'FS_ERROR';
52+
throw new FileSystemError(
53+
`Cannot read file ${filePath}: ${error.message}`,
54+
code,
55+
filePath,
56+
'read'
57+
);
58+
}
59+
throw error;
5060
}
5161
}
5262

0 commit comments

Comments
 (0)