fix: Chroma connection errors and remove dead last_user_message code (#525)
* fix: distinguish connection errors from collection-not-found in ChromaSync Previously, ensureCollection() caught ALL errors from chroma_get_collection_info and assumed they meant "collection doesn't exist". This caused connection errors like "Not connected" to trigger unnecessary collection creation attempts. Now connection-related errors are re-thrown immediately instead of being misinterpreted as missing collections. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: improve error handling for Chroma connection and collection creation * fix: remove dead last_user_message from summarize flow The last_user_message field was extracted from transcripts but never used. In Claude Code transcripts, "user" type messages are mostly tool_results, not actual user input. The user's original request is already stored in user_prompts table. This removes the false warning "Missing last_user_message when queueing summary" which was complaining about missing data that didn't exist and wasn't needed. Changes: - summary-hook: Only extract last_assistant_message - SessionRoutes: Remove last_user_message from request body handling - SessionManager.queueSummarize: Remove lastUserMessage parameter - PendingMessage interface: Remove last_user_message field - SDKSession interface: Remove last_user_message field - All agents: Remove last_user_message from buildSummaryPrompt calls 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * build artifacts for plugin * Enhance error handling across multiple services - Improved logging in `BranchManager.ts` to capture recovery checkout failures. - Updated `PaginationHelper.ts` to log when file paths are plain strings instead of valid JSON. - Enhanced error logging in `SDKAgent.ts` for Claude executable detection failures. - Added logging for plain string handling in `SearchManager.ts` for files read and edited. - Improved logging in `paths.ts` for git root detection failures. - Enhanced JSON parsing error handling in `timeline-formatting.ts` with previews of failed inputs. - Updated `transcript-parser.ts` to log summary of parse errors after processing transcript lines. - Established a baseline for error handling practices in `error-handling-baseline.txt`. - Documented error handling anti-pattern rules in `CLAUDE.md` to prevent silent failures and improve code quality. * Add error handling anti-pattern detection script and guidelines - Introduced `detect-error-handling-antipatterns.ts` to identify common error handling issues in TypeScript code. - Created comprehensive documentation in `CLAUDE.md` outlining forbidden patterns, allowed patterns, and critical path protection rules. - Implemented checks for empty catch blocks, logging practices, and try-catch block sizes to prevent silent failures and improve debugging. - Established a reporting mechanism to summarize detected anti-patterns with severity levels. * feat: add console filter bar and log line parsing with filtering capabilities - Introduced a console filter bar with options to filter logs by level and component. - Implemented parsing of log lines to extract structured data including timestamp, level, component, and correlation ID. - Added functionality to toggle individual and all levels/components for filtering. - Enhanced log line rendering with color coding based on log level and special message types. - Improved responsiveness of the filter bar for smaller screens. --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Alex Newman
@@ -0,0 +1,137 @@
|
||||
# Error Handling Anti-Pattern Rules
|
||||
|
||||
This folder contains `detect-error-handling-antipatterns.ts` - run it before committing any error handling changes.
|
||||
|
||||
## The Try-Catch Problem That Cost 10 Hours
|
||||
|
||||
A single overly-broad try-catch block wasted 10 hours of debugging time by silently swallowing errors.
|
||||
**This pattern is BANNED.**
|
||||
|
||||
## BEFORE You Write Any Try-Catch
|
||||
|
||||
**RUN THIS TEST FIRST:**
|
||||
```bash
|
||||
bun run scripts/anti-pattern-test/detect-error-handling-antipatterns.ts
|
||||
```
|
||||
|
||||
**You MUST answer these 5 questions to the user BEFORE writing try-catch:**
|
||||
|
||||
1. **What SPECIFIC error am I catching?** (Name the error type: `FileNotFoundError`, `NetworkTimeout`, `ValidationError`)
|
||||
2. **Show documentation proving this error can occur** (Link to docs or show me the source code)
|
||||
3. **Why can't this error be prevented?** (If it can be prevented, prevent it instead)
|
||||
4. **What will the catch block DO?** (Must include logging + either rethrow OR explicit fallback)
|
||||
5. **Why shouldn't this error propagate?** (Justify swallowing it rather than letting caller handle)
|
||||
|
||||
**If you cannot answer ALL 5 questions with specifics, DO NOT write the try-catch.**
|
||||
|
||||
## FORBIDDEN PATTERNS (Zero Tolerance)
|
||||
|
||||
### CRITICAL - Never Allowed
|
||||
|
||||
```typescript
|
||||
// FORBIDDEN: Empty catch
|
||||
try {
|
||||
doSomething();
|
||||
} catch {}
|
||||
|
||||
// FORBIDDEN: Catch without logging
|
||||
try {
|
||||
doSomething();
|
||||
} catch (error) {
|
||||
return null; // Silent failure!
|
||||
}
|
||||
|
||||
// FORBIDDEN: Large try blocks (>10 lines)
|
||||
try {
|
||||
// 50 lines of code
|
||||
// Multiple operations
|
||||
// Different failure modes
|
||||
} catch (error) {
|
||||
logger.error('Something failed'); // Which thing?!
|
||||
}
|
||||
|
||||
// FORBIDDEN: Promise empty catch
|
||||
promise.catch(() => {}); // Error disappears into void
|
||||
|
||||
// FORBIDDEN: Try-catch to fix TypeScript errors
|
||||
try {
|
||||
// @ts-ignore
|
||||
const value = response.propertyThatDoesntExist;
|
||||
} catch {}
|
||||
```
|
||||
|
||||
### ALLOWED Patterns
|
||||
|
||||
```typescript
|
||||
// GOOD: Specific, logged, explicit handling
|
||||
try {
|
||||
await fetch(url);
|
||||
} catch (error) {
|
||||
if (error instanceof NetworkError) {
|
||||
logger.warn('SYNC', 'Network request failed, will retry', { url }, error);
|
||||
return null; // Explicit: null means "fetch failed"
|
||||
}
|
||||
throw error; // Unexpected errors propagate
|
||||
}
|
||||
|
||||
// GOOD: Minimal scope, clear recovery
|
||||
try {
|
||||
JSON.parse(data);
|
||||
} catch (error) {
|
||||
logger.error('CONFIG', 'Corrupt settings file, using defaults', {}, error);
|
||||
return DEFAULT_SETTINGS;
|
||||
}
|
||||
|
||||
// GOOD: Fire-and-forget with logging
|
||||
backgroundTask()
|
||||
.catch(error => logger.warn('BACKGROUND', 'Task failed', {}, error));
|
||||
|
||||
// GOOD: Ignored anti-pattern for genuine hot paths only
|
||||
try {
|
||||
checkIfProcessAlive(pid);
|
||||
} catch (error) {
|
||||
// [ANTI-PATTERN IGNORED]: Tight loop checking 100s of PIDs during cleanup
|
||||
return false;
|
||||
}
|
||||
```
|
||||
|
||||
## Ignoring Anti-Patterns (Rare)
|
||||
|
||||
**Only for genuine hot paths** where logging would cause performance problems:
|
||||
|
||||
```typescript
|
||||
// [ANTI-PATTERN IGNORED]: Reason why logging is impossible
|
||||
```
|
||||
|
||||
**Rules:**
|
||||
- **Hot paths only** - code in tight loops called 1000s of times
|
||||
- If you can add logging, ADD LOGGING - don't ignore
|
||||
- Valid examples:
|
||||
- "Tight loop checking process exit status during cleanup"
|
||||
- "Health check polling every 100ms"
|
||||
- Invalid examples:
|
||||
- "Expected JSON parse failures" - Just add logger.debug
|
||||
- "Common fallback path" - Just add logger.debug
|
||||
|
||||
## The Meta-Rule
|
||||
|
||||
**UNCERTAINTY TRIGGERS RESEARCH, NOT TRY-CATCH**
|
||||
|
||||
When you're unsure if a property exists or a method signature is correct:
|
||||
1. **READ** the source code or documentation
|
||||
2. **VERIFY** with the Read tool
|
||||
3. **USE** TypeScript types to catch errors at compile time
|
||||
4. **WRITE** code you KNOW is correct
|
||||
|
||||
Never use try-catch to paper over uncertainty. That wastes hours of debugging time later.
|
||||
|
||||
## Critical Path Protection
|
||||
|
||||
These files are **NEVER** allowed to have catch-and-continue:
|
||||
- `SDKAgent.ts` - Errors must propagate, not hide
|
||||
- `GeminiAgent.ts` - Must fail loud, not silent
|
||||
- `OpenRouterAgent.ts` - Must fail loud, not silent
|
||||
- `SessionStore.ts` - Database errors must propagate
|
||||
- `worker-service.ts` - Core service errors must be visible
|
||||
|
||||
On critical paths, prefer **NO TRY-CATCH** and let errors propagate naturally.
|
||||
+120
-8
@@ -56,6 +56,117 @@ function detectAntiPatterns(filePath: string, projectRoot: string): AntiPattern[
|
||||
const relPath = relative(projectRoot, filePath);
|
||||
const isCriticalPath = CRITICAL_PATHS.some(cp => filePath.includes(cp));
|
||||
|
||||
// Detect error message string matching for type detection (line-by-line patterns)
|
||||
for (let i = 0; i < lines.length; i++) {
|
||||
const line = lines[i];
|
||||
const trimmed = line.trim();
|
||||
|
||||
// Check for [ANTI-PATTERN IGNORED] on the same or previous line
|
||||
const hasOverride = trimmed.includes('[ANTI-PATTERN IGNORED]') ||
|
||||
(i > 0 && lines[i - 1].includes('[ANTI-PATTERN IGNORED]'));
|
||||
const overrideMatch = (trimmed + (i > 0 ? lines[i - 1] : '')).match(/\[ANTI-PATTERN IGNORED\]:\s*(.+)/i);
|
||||
const overrideReason = overrideMatch?.[1]?.trim();
|
||||
|
||||
// CRITICAL: Error message string matching for type detection
|
||||
// Patterns like: errorMessage.includes('connection') or error.message.includes('timeout')
|
||||
const errorStringMatchPatterns = [
|
||||
/error(?:Message|\.message)\s*\.includes\s*\(\s*['"`](\w+)['"`]\s*\)/i,
|
||||
/(?:err|e)\.message\s*\.includes\s*\(\s*['"`](\w+)['"`]\s*\)/i,
|
||||
/String\s*\(\s*(?:error|err|e)\s*\)\s*\.includes\s*\(\s*['"`](\w+)['"`]\s*\)/i,
|
||||
];
|
||||
|
||||
for (const pattern of errorStringMatchPatterns) {
|
||||
const match = trimmed.match(pattern);
|
||||
if (match) {
|
||||
const matchedString = match[1];
|
||||
// Common generic patterns that are too broad
|
||||
const genericPatterns = ['error', 'fail', 'connection', 'timeout', 'not', 'invalid', 'unable'];
|
||||
const isGeneric = genericPatterns.some(gp => matchedString.toLowerCase().includes(gp));
|
||||
|
||||
if (hasOverride && overrideReason) {
|
||||
antiPatterns.push({
|
||||
file: relPath,
|
||||
line: i + 1,
|
||||
pattern: 'ERROR_STRING_MATCHING',
|
||||
severity: 'APPROVED_OVERRIDE',
|
||||
description: `Error type detection via string matching on "${matchedString}" - approved override.`,
|
||||
code: trimmed,
|
||||
overrideReason
|
||||
});
|
||||
} else {
|
||||
antiPatterns.push({
|
||||
file: relPath,
|
||||
line: i + 1,
|
||||
pattern: 'ERROR_STRING_MATCHING',
|
||||
severity: isGeneric ? 'CRITICAL' : 'HIGH',
|
||||
description: `Error type detection via string matching on "${matchedString}" - fragile and masks the real error. Log the FULL error object. We don't care about pretty error handling, we care about SEEING what went wrong.`,
|
||||
code: trimmed
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// HIGH: Logging only error.message instead of the full error object
|
||||
// Patterns like: logger.error('X', 'Y', {}, error.message) or console.error(error.message)
|
||||
const partialErrorLoggingPatterns = [
|
||||
/logger\.(error|warn|info|debug)\s*\([^)]*,\s*(?:error|err|e)\.message\s*\)/,
|
||||
/logger\.(error|warn|info|debug)\s*\([^)]*\{\s*(?:error|err|e):\s*(?:error|err|e)\.message\s*\}/,
|
||||
/console\.(error|warn|log)\s*\(\s*(?:error|err|e)\.message\s*\)/,
|
||||
/console\.(error|warn|log)\s*\(\s*['"`][^'"`]+['"`]\s*,\s*(?:error|err|e)\.message\s*\)/,
|
||||
];
|
||||
|
||||
for (const pattern of partialErrorLoggingPatterns) {
|
||||
if (pattern.test(trimmed)) {
|
||||
if (hasOverride && overrideReason) {
|
||||
antiPatterns.push({
|
||||
file: relPath,
|
||||
line: i + 1,
|
||||
pattern: 'PARTIAL_ERROR_LOGGING',
|
||||
severity: 'APPROVED_OVERRIDE',
|
||||
description: 'Logging only error.message instead of full error object - approved override.',
|
||||
code: trimmed,
|
||||
overrideReason
|
||||
});
|
||||
} else {
|
||||
antiPatterns.push({
|
||||
file: relPath,
|
||||
line: i + 1,
|
||||
pattern: 'PARTIAL_ERROR_LOGGING',
|
||||
severity: 'HIGH',
|
||||
description: 'Logging only error.message HIDES the stack trace, error type, and all properties. ALWAYS pass the full error object - you need the complete picture, not a summary.',
|
||||
code: trimmed
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// CRITICAL: Catch-all error type guessing based on message content
|
||||
// Pattern: if (errorMessage.includes('X') || errorMessage.includes('Y'))
|
||||
const multipleIncludes = trimmed.match(/(?:error(?:Message|\.message)|(?:err|e)\.message).*\.includes.*\|\|.*\.includes/i);
|
||||
if (multipleIncludes) {
|
||||
if (hasOverride && overrideReason) {
|
||||
antiPatterns.push({
|
||||
file: relPath,
|
||||
line: i + 1,
|
||||
pattern: 'ERROR_MESSAGE_GUESSING',
|
||||
severity: 'APPROVED_OVERRIDE',
|
||||
description: 'Multiple string checks on error message to guess error type - approved override.',
|
||||
code: trimmed,
|
||||
overrideReason
|
||||
});
|
||||
} else {
|
||||
antiPatterns.push({
|
||||
file: relPath,
|
||||
line: i + 1,
|
||||
pattern: 'ERROR_MESSAGE_GUESSING',
|
||||
severity: 'CRITICAL',
|
||||
description: 'Multiple string checks on error message to guess error type. STOP GUESSING. Log the FULL error object. We don\'t care what the library throws - we care about SEEING the error when it happens.',
|
||||
code: trimmed
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Track try-catch blocks
|
||||
let inTry = false;
|
||||
let tryStartLine = 0;
|
||||
@@ -98,7 +209,7 @@ function detectAntiPatterns(filePath: string, projectRoot: string): AntiPattern[
|
||||
braceCount += (nextLine.match(/{/g) || []).length - (nextLine.match(/}/g) || []).length;
|
||||
}
|
||||
|
||||
const hasLogging = catchBody.match(/logger\.(error|warn|debug|info)/) ||
|
||||
const hasLogging = catchBody.match(/logger\.(error|warn|debug|info|failure)/) ||
|
||||
catchBody.match(/console\.(error|warn)/);
|
||||
|
||||
if (!hasLogging && lookAhead > 0) { // Only flag if it's actually a multi-line handler
|
||||
@@ -216,12 +327,12 @@ function analyzeTryCatchBlock(
|
||||
});
|
||||
}
|
||||
|
||||
// Check for [APPROVED OVERRIDE] marker
|
||||
const overrideMatch = catchContent.match(/\/\/\s*\[APPROVED OVERRIDE\]:\s*(.+)/i);
|
||||
// Check for [ANTI-PATTERN IGNORED] marker
|
||||
const overrideMatch = catchContent.match(/\/\/\s*\[ANTI-PATTERN IGNORED\]:\s*(.+)/i);
|
||||
const overrideReason = overrideMatch?.[1]?.trim();
|
||||
|
||||
// CRITICAL: No logging in catch block (unless explicitly approved)
|
||||
const hasLogging = catchContent.match(/logger\.(error|warn|debug|info)/);
|
||||
const hasLogging = catchContent.match(/logger\.(error|warn|debug|info|failure)/);
|
||||
const hasConsoleError = catchContent.match(/console\.(error|warn)/);
|
||||
const hasStderr = catchContent.match(/process\.stderr\.write/);
|
||||
const hasThrow = catchContent.match(/throw/);
|
||||
@@ -286,16 +397,17 @@ function analyzeTryCatchBlock(
|
||||
// CRITICAL on critical paths: Catch-and-continue
|
||||
if (isCriticalPath && nonCommentContent && !hasThrow) {
|
||||
const hasReturn = catchContent.match(/return/);
|
||||
const continuesExecution = !hasReturn; // If no return/throw, execution continues
|
||||
const hasProcessExit = catchContent.match(/process\.exit/);
|
||||
const terminatesExecution = hasReturn || hasProcessExit;
|
||||
|
||||
if (continuesExecution && hasLogging) {
|
||||
if (!terminatesExecution && hasLogging) {
|
||||
if (overrideReason) {
|
||||
antiPatterns.push({
|
||||
file: relPath,
|
||||
line: catchStartLine,
|
||||
pattern: 'CATCH_AND_CONTINUE_CRITICAL_PATH',
|
||||
severity: 'APPROVED_OVERRIDE',
|
||||
description: 'Critical path continues after error - approved override.',
|
||||
description: 'Critical path continues after error - anti-pattern ignored.',
|
||||
code: catchBlock.trim(),
|
||||
overrideReason
|
||||
});
|
||||
@@ -400,7 +512,7 @@ function formatReport(antiPatterns: AntiPattern[]): string {
|
||||
report += '4. What will the catch block DO? (Log + rethrow? Fallback?)\n';
|
||||
report += '5. Why shouldn\'t this error propagate to the caller?\n';
|
||||
report += '\n';
|
||||
report += 'To approve an anti-pattern, add: // [APPROVED OVERRIDE]: reason\n';
|
||||
report += 'To ignore an anti-pattern, add: // [ANTI-PATTERN IGNORED]: reason\n';
|
||||
report += '═══════════════════════════════════════════════════════════════\n\n';
|
||||
|
||||
return report;
|
||||
Reference in New Issue
Block a user