Fix #362: Surface OpenAI authentication errors as HTTP 401#371
Fix #362: Surface OpenAI authentication errors as HTTP 401#371
Conversation
- Enhanced RAGService.search_documents() to re-raise specific embedding errors instead of returning empty lists - Improved API error handling in perform_rag_query() to catch and return detailed OpenAI quota/rate limit errors - Added comprehensive test suite for OpenAI error handling scenarios - API now returns HTTP 429 with detailed error messages when OpenAI quota is exhausted Resolves #362: Users now get clear error messages instead of silent failures when OpenAI API key lacks credits 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Created knowledgeBaseErrorHandler with specialized OpenAI error parsing - Enhanced knowledgeBaseService to use error handler for better UX - Added ErrorAlert React component with actionable error messages - Included usage documentation and examples Frontend now displays clear messages like "OpenAI API quota exhausted" with specific guidance like "Check your OpenAI billing dashboard" Completes frontend portion of issue #362 fix 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Claude finished @Wirasm's task —— View job Code Review Complete ✅Reviewed PR #371 - Bugfix issue 362 (OpenAI error handling improvements) Todo List:
SummaryThis PR implements comprehensive OpenAI error handling to address issue #362 where users experienced silent failures during quota exhaustion. The implementation spans both backend (Python/FastAPI) and frontend (React/TypeScript) with proper error propagation and user-friendly messaging. Previous Review CommentsFirst review - no previous comments Issues FoundTotal: 0 critical, 3 important, 4 minor 🟡 Important (Should Fix)
🟢 Minor (Consider)
Security AssessmentNo security issues found. The implementation properly:
Performance Considerations
Good Practices Observed
Questionable Practices
Test CoverageCurrent Coverage: 85% (estimated based on comprehensive backend tests) Missing Tests:
RecommendationsMerge Decision:
Priority Actions:
Rationale: The architecture and implementation quality is high - this just needs the final integration step to be complete. Review based on Archon V2 Alpha guidelines and CLAUDE.md principles |
…ry guidance - Replace silent failures with detailed OpenAI API error messages - Add fail-fast error propagation from RAG service to API endpoints - Implement comprehensive error message sanitization to prevent information disclosure - Add retry-after guidance for rate limit errors (30 second default) - Enhance frontend error display with actionable user guidance - Add robust input validation for malformed error objects and circular references - Extend sanitization patterns for organization IDs, project IDs, request IDs, and bearer tokens - Add 21 comprehensive test cases covering all error scenarios and sanitization patterns Resolves issue where users experienced 90+ minutes of debugging due to silent OpenAI quota exhaustion errors returning empty results instead of clear error messages. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
leex279
changed the title
Critical Security Fixes: - Fix ReDoS vulnerability in Bearer token regex with bounded quantifier - Add comprehensive input validation to _sanitize_openai_error function - Replace realistic test credentials with obviously fake ones to prevent security scanner false positives Performance Optimizations: - Optimize JSON.stringify performance in isSafeObject with size limits and depth checking - Add proper error state cleanup in React useEffect to prevent memory leaks Additional Improvements: - Add new test case for input validation edge cases - Update all test credentials to use proper regex-matching formats - Maintain 22/22 test pass rate with enhanced security validation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughAdds centralized OpenAI/RAG error normalization and sanitization, new UI Alert/ErrorAlert components and hook, embedding-specific exceptions and fail-fast propagation (401/429/502), API key preflight validation, provider cache invalidation on credential changes, RAG embedding call fixes, logging tweaks, and expanded tests for propagation and sanitization. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as KnowledgeBasePage (UI)
participant FES as Frontend Service
participant EH as Error Normalizer
participant API as Server (knowledge_api)
participant RAG as RAG Service
participant EMB as Embedding Service
UI->>FES: request (fetch/refresh/upload/crawl)
FES->>API: HTTP request
API->>RAG: perform_rag_query(...)
RAG->>EMB: create_embedding(text=query)
alt embedding auth/quota/rate-limit/api error
EMB-->>RAG: raise Embedding*Error
RAG-->>API: propagate specific Embedding*Error
API->>API: _sanitize_openai_error(...)
API-->>FES: HTTP 401/429/502 + sanitized payload
FES->>EH: parseKnowledgeBaseError(response)
EH-->>FES: EnhancedError (severity, action, message)
FES-->>UI: normalized error
UI->>UI: render ErrorAlert (variant, description, suggested action)
else success
EMB-->>RAG: embedding vector
RAG-->>API: results
API-->>FES: 200 OK
FES-->>UI: data
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The issue was that changing the OpenAI API key in settings wasn't reflected immediately due to caching in llm_provider_service.py. The service caches provider configurations for 5 minutes to optimize performance, but when users updated their API key via settings, the cache wasn't being invalidated. **Changes:** - Added cache invalidation functions to llm_provider_service.py - Modified credential_service.set_credential() to invalidate LLM provider cache when API keys are updated - Uses late import to avoid circular dependency issues - Cache is cleared for OPENAI_API_KEY, GOOGLE_API_KEY, and rag_strategy category changes **Testing:** - Verified API key updates now work immediately - Added debug logging for cache invalidation tracking - Used API endpoints to test real-world usage scenario Fixes the regression where API key changes required waiting up to 5 minutes or server restart to take effect. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…improvements - Merged main branch changes including new /rag/query endpoint - Preserved comprehensive OpenAI error handling from bugfix branch: - _sanitize_openai_error function for security - Specific handling for quota exhaustion (429) - Specific handling for rate limits (429) - Sanitized API error messages (502) - Kept enhanced frontend error display improvements - Maintained all test coverage for error scenarios
- Add EmbeddingAuthenticationError exception class for invalid/expired API keys - Update embedding service to catch openai.AuthenticationError exceptions - Handle authentication errors in knowledge_api with 401 status code - Add test cases for authentication error propagation and API handling - Prevent sensitive API key patterns from being exposed in error logs This ensures that authentication failures (401 errors) are properly caught and return clear, actionable error messages to users instead of failing silently or exposing sensitive information in logs.
- Fix logging error in threading_service.py (remove invalid kwargs) - Track embedding failures in document storage service - Return statistics from add_documents_to_supabase including failures - Propagate embedding failures up through document storage operations - Show error status in UI when embeddings fail instead of false success - Send error_crawl_progress event when there are embedding failures - Display clear error message about checking OpenAI API key This ensures users get proper error notifications when embeddings fail due to invalid API keys or other OpenAI errors, instead of showing a misleading success message.
|
leex279
force-pushed
the
bugfix-issue-362
branch
from
1cb4c9c to
f770675
Compare
… prefix; accurate reranking_applied; remove unused import
leex279
force-pushed
the
bugfix-issue-362
branch
from
d1137b0 to
46a98da
Compare
leex279
changed the title
leex279
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
python/src/server/services/llm_provider_service.py (1)
143-147: Preserve stack traces in logsLog with
exc_info=Trueto keep the traceback per guidelines.- logger.error( - f"Error creating LLM client for provider {provider_name if 'provider_name' in locals() else 'unknown'}: {e}" - ) + logger.error( + f"Error creating LLM client for provider " + f"{provider_name if 'provider_name' in locals() else 'unknown'}: {e}", + exc_info=True, + )python/src/server/services/credential_service.py (2)
474-486: Bug: wrong key casing prevents provider updates from taking effect
set_active_providerwritesllm_provider(lowercase) but readers useLLM_PROVIDER(uppercase) viaget_active_provider/env setup. Provider changes will be ignored.- return await self.set_credential( - "llm_provider", - provider, - category="rag_strategy", - description=f"Active {service_type} provider", - ) + return await self.set_credential( + "LLM_PROVIDER", + provider, + category="rag_strategy", + description=f"Active {service_type} provider", + )
418-451: Use uppercase 'LLM_PROVIDER' when storing RAG settings
In credential_service.py at line 479, you’re calling:- return await self.set_credential( - "llm_provider", + return await self.set_credential( + "LLM_PROVIDER", provider, category="rag_strategy", )This lowercase key will never match the uppercase keys fetched by get_credentials_by_category and causes a silent fallback to defaults.
python/src/server/services/threading_service.py (1)
99-105: Prefer structured logging and avoid recursive re-entry in acquire()
- Inline f-string drops structured fields and makes metrics harder to search/aggregate. Use logger extras.
- Recursing back into acquire() after sleep can cause deep call stacks under prolonged throttle. Loop instead.
- logfire_logger.info( - f"Rate limiting: waiting {wait_time:.1f}s (tokens: {estimated_tokens}, current usage: {self._get_current_usage()})" - ) - await asyncio.sleep(wait_time) - return await self.acquire(estimated_tokens) + logfire_logger.info( + "Rate limiting: waiting", + extra={ + "wait_time_s": round(wait_time, 1), + "estimated_tokens": estimated_tokens, + **self._get_current_usage(), + }, + ) + await asyncio.sleep(wait_time) + # Re-check limits without recursion + while True: + now = time.time() + self._clean_old_entries(now) + if self._can_make_request(estimated_tokens): + break + await asyncio.sleep(0.1) + # fall through to record the requestpython/src/server/services/storage/document_storage_service.py (1)
96-105: Fallback delete uses hardcoded slice size (10) instead of fallback_batch_sizeThis defeats the fallback tuning and may create oversized/undersized batches.
- batch_urls = unique_urls[i : i + 10] + batch_urls = unique_urls[i : i + fallback_batch_size]archon-ui-main/src/components/mcp/ToolTestingPanel.tsx (1)
271-293: Use sanitized enhanced error; avoid logging raw errors; fix terminal message source.Leverage the parsed/sanitized error for logging and terminal output, and guard against
clientbecoming null between async ticks.- } catch (error: any) { - console.error('MCP tool execution failed:', error); - - // Parse error through enhanced error handler for better user experience - const enhancedError = parseKnowledgeBaseError(error); - setError(enhancedError); + } catch (error: any) { + // Parse error through enhanced error handler for better user experience + const enhancedError = parseKnowledgeBaseError(error); + // Log only sanitized message/details + console.error('MCP tool execution failed:', enhancedError.message, enhancedError.errorDetails ?? null); + setError(enhancedError); + const clientNameSnapshot = client?.name ?? 'client'; @@ - addTypingLine(`> ERROR: Failed to execute tool on ${client.name}`, false, true); - addTypingLine(`> ${error.message || 'Unknown error occurred'}`, false, true); + addTypingLine(`> ERROR: Failed to execute tool on ${clientNameSnapshot}`, false, true); + addTypingLine(`> ${getDisplayErrorMessage(enhancedError)}`, false, true);
🧹 Nitpick comments (31)
python/src/server/services/llm_provider_service.py (3)
41-46: Drop unnecessary global; consider guarding cache ops
global _settings_cacheisn’t needed when mutating the dict in place. Also, multiple async tasks/threads can clear/read the cache concurrently; consider a lock around cache mutations/reads to avoid race conditions.Apply minimal cleanup:
def invalidate_provider_cache() -> None: """Invalidate all cached provider configurations.""" - global _settings_cache _settings_cache.clear() logger.debug("Provider configuration cache cleared")If you want to add concurrency safety, introduce a module-level lock and wrap all cache reads/writes:
# at top-level from threading import RLock _cache_lock = RLock()def _get_cached_settings(key: str) -> Any | None: with _cache_lock: entry = _settings_cache.get(key) if not entry: return None value, ts = entry if time.time() - ts < _CACHE_TTL_SECONDS: return value del _settings_cache[key] return None def _set_cached_settings(key: str, value: Any) -> None: with _cache_lock: _settings_cache[key] = (value, time.time()) def invalidate_provider_cache() -> None: with _cache_lock: _settings_cache.clear() logger.debug("Provider configuration cache cleared")
48-56: Pattern invalidation works; add lock and avoid globalSame nits: remove
global, and protect deletion with the same lock to avoid mutation during iteration elsewhere.def invalidate_specific_cache(key_pattern: str) -> None: """Invalidate cache entries matching a specific pattern.""" - global _settings_cache - keys_to_remove = [key for key in _settings_cache.keys() if key_pattern in key] - for key in keys_to_remove: - del _settings_cache[key] + with _cache_lock: + keys_to_remove = [key for key in _settings_cache if key_pattern in key] + for key in keys_to_remove: + del _settings_cache[key] if keys_to_remove: logger.debug(f"Invalidated cache entries: {keys_to_remove}")
202-206: Don’t swallow errors for embedding model; at least log tracebackReturning a default can mask misconfig. If keeping the fallback, log with traceback; consider re-raising a typed exception to fail fast.
- except Exception as e: - logger.error(f"Error getting embedding model: {e}") - # Fallback to OpenAI default - return "text-embedding-3-small" + except Exception as e: + logger.error(f"Error getting embedding model: {e}", exc_info=True) + # Fallback to OpenAI default (consider raising a specific error instead) + return "text-embedding-3-small"python/src/server/services/credential_service.py (7)
242-252: Good: cache invalidation on credential updates; prefer targeted invalidation to reduce cache stampedesClearing the entire provider cache works but is heavy. Use the new pattern invalidator to evict only affected entries.
- if key in ["OPENAI_API_KEY", "GOOGLE_API_KEY"] or category == "rag_strategy": + if key in ["OPENAI_API_KEY", "GOOGLE_API_KEY"] or category == "rag_strategy": try: - # Use late import to avoid circular dependency - from .llm_provider_service import invalidate_provider_cache - invalidate_provider_cache() - logger.debug(f"Invalidated LLM provider cache due to update of {key}") + # Late import to avoid circular dependency + from .llm_provider_service import ( + invalidate_specific_cache, + invalidate_provider_cache, + ) + # Narrow invalidation to reduce churn + invalidate_specific_cache("provider_config_") + if category == "rag_strategy": + invalidate_specific_cache("rag_strategy_settings") + logger.debug(f"Invalidated LLM provider caches due to update of {key}") except ImportError as import_err: logger.warning(f"Could not invalidate LLM provider cache: {import_err}")
258-261: Consider raising instead of returning False on write failuresReturning
Falsehides the cause from callers; raise a typed exception and log withexc_info=Trueto meet fail-fast and traceability guidelines.- except Exception as e: - logger.error(f"Error setting credential {key}: {e}") - return False + except Exception as e: + logger.error(f"Error setting credential {key}: {e}", exc_info=True) + raise
76-79: Add traceback to initialization error logsPreserve stack traces when Supabase client init fails.
- except Exception as e: - logger.error(f"Error initializing Supabase client: {e}") - raise + except Exception as e: + logger.error(f"Error initializing Supabase client: {e}", exc_info=True) + raise
332-335: Propagate category fetch failures or include tracebackReturning
{}may mask DB errors; at minimum addexc_info=True.- except Exception as e: - logger.error(f"Error getting credentials for category {category}: {e}") - return {} + except Exception as e: + logger.error(f"Error getting credentials for category {category}: {e}", exc_info=True) + return {}
381-383: Same for list_all_credentials errorsKeep traceback for triage.
- except Exception as e: - logger.error(f"Error listing credentials: {e}") - return [] + except Exception as e: + logger.error(f"Error listing credentials: {e}", exc_info=True) + return []
553-559: Environment export: include traceback on failuresMinor, but consistent error logging helps diagnostics.
- except Exception as e: - logger.warning(f"Failed to set environment variable {key}: {e}") + except Exception as e: + logger.warning(f"Failed to set environment variable {key}: {e}", exc_info=True)
561-571: Optional: reduce noise in logs for missing optional credsConsider
logger.debug(..., exc_info=True)only on unexpected exceptions; current code is fine if intentional.python/src/server/services/storage/document_storage_service.py (4)
46-49: Docstring return spec incompleteYou also return "success"; document it for callers.
- Dict with statistics: chunks_stored, embedding_failures, total_chunks + Dict with statistics: chunks_stored, embedding_failures, total_chunks, success
374-387: Final progress message can misreport stored countMessage claims “X chunks stored” but uses len(contents), not the actual stored count; prefer total_chunks_stored.
- await progress_callback( - f"Document storage completed: {len(contents)} chunks stored in {total_batches} batches", + await progress_callback( + f"Document storage completed: {total_chunks_stored} chunks stored in {total_batches} batches", 100, {
64-71: Unused settings and parameter can be removed
- enable_parallel is computed but unused.
- enable_parallel_batches parameter isn’t referenced.
Remove dead code to reduce confusion.- enable_parallel_batches: bool = True, + # enable_parallel_batches: bool = True, # removed (unused) @@ - enable_parallel = rag_settings.get("ENABLE_PARALLEL_BATCHES", "true").lower() == "true" + _ = rag_settings.get("ENABLE_PARALLEL_BATCHES", "true") # reserved for future use @@ - enable_parallel = True + _ = TrueAlso applies to: 18-30
268-276: O(n^2) mapping from texts back to original indices; breaks on duplicatesBuild an index from contextual_contents to lists of positions to handle duplicates and reduce complexity.
- orig_idx = None - for idx, orig_text in enumerate(contextual_contents): - if orig_text == text: - orig_idx = idx - break + # Precompute once outside the loop (not shown here): + # index_map = defaultdict(list); for idx, t in enumerate(contextual_contents): index_map[t].append(idx) + positions = index_map.get(text) + orig_idx = positions.pop(0) if positions else Nonepython/src/server/services/crawling/document_storage_operations.py (2)
142-155: Adopts new return value — good; consider removing unused enable_parallel_batchesYou’re capturing storage_stats correctly. The enable_parallel_batches arg isn’t used downstream; remove to avoid confusion.
157-163: Good: log embedding failure summary; add minimal contextIncluding the source_id or crawl url helps triage multi-tenant runs without leaking PII.
- safe_logfire_error( - f"Document storage had embedding failures | failures={storage_stats['embedding_failures']} | " - f"stored={storage_stats['chunks_stored']} | total={storage_stats['total_chunks']}" - ) + safe_logfire_error( + f"Document storage had embedding failures | failures={storage_stats['embedding_failures']} | " + f"stored={storage_stats['chunks_stored']} | total={storage_stats['total_chunks']} | source_id={original_source_id}" + )archon-ui-main/src/services/knowledgeBaseService.ts (2)
237-245: Align uploadDocument error path with centralized parserUse parseKnowledgeBaseError for consistency with the rest of the service.
- if (!response.ok) { - const error = await response.json() - throw new Error(error.error || `HTTP ${response.status}`) - } + if (!response.ok) { + let errBody: any; + try { errBody = await response.json(); } catch { errBody = { error: await response.text() }; } + throw parseKnowledgeBaseError({ status: response.status, ...errBody }); + }
84-109: Consider gating verbose console logs behind DEV checksReduce production noise; keep sensitive data out of logs.
- console.log(`🔍 [KnowledgeBase] Starting API request to: ${url}`); + if (import.meta.env?.DEV) console.log(`🔍 [KnowledgeBase] Starting API request to: ${url}`);archon-ui-main/src/components/ui/Alert.tsx (2)
15-30: Forward props and add accessibility rolesAllow passing standard div props and add ARIA to announce destructive alerts. Keeps API flexible and improves a11y.
Apply:
-export const Alert: React.FC<AlertProps> = ({ - children, - variant = 'default', - className = '' -}) => { +export const Alert: React.FC<AlertProps> = ({ + children, + variant = 'default', + className = '', + ...rest +}) => { const baseClasses = 'p-4 border rounded-lg'; const variantClasses = variant === 'destructive' ? 'border-red-300 bg-red-50 text-red-800' : 'border-blue-300 bg-blue-50 text-blue-800'; return ( - <div className={`${baseClasses} ${variantClasses} ${className}`}> + <div + role={variant === 'destructive' ? 'alert' : undefined} + aria-live={variant === 'destructive' ? 'assertive' : 'polite'} + className={`${baseClasses} ${variantClasses} ${className}`} + {...rest} + > {children} </div> ); };
9-13: Type props for reuseExtend HTMLAttributes so consumers can set id, data-* attrs, etc., without TS casts.
Apply:
-interface AlertProps { +interface AlertProps extends React.HTMLAttributes<HTMLDivElement> { children: React.ReactNode; variant?: 'default' | 'destructive'; className?: string; } -interface AlertDescriptionProps { +interface AlertDescriptionProps extends React.HTMLAttributes<HTMLDivElement> { children: React.ReactNode; className?: string; } export const AlertDescription: React.FC<AlertDescriptionProps> = ({ - children, - className = '' + children, + className = '', + ...rest }) => { return ( - <div className={className}> + <div className={className} {...rest}> {children} </div> ); };Also applies to: 32-46
python/tests/test_openai_quota_error_handling.py (1)
11-15: Remove unused importTestClient is imported but not used.
-from fastapi.testclient import TestClientpython/src/server/api_routes/knowledge_api.py (3)
28-33: Duplicate imports — consolidate to single setThese lines duplicate imports already defined above; they’ll trigger linter warnings and confuse readers.
-# Import unified logging -from ..config.logfire_config import get_logger, safe_logfire_error, safe_logfire_info -from ..services.crawler_manager import get_crawler -from ..services.search.rag_service import RAGService -from ..services.storage import DocumentStorageService -from ..utils import get_supabase_client +from ..config.logfire_config import get_logger, safe_logfire_error, safe_logfire_info
12-18: Add missing import for regex sanitizationSanitizer uses regex; add import.
import asyncio +import re import json import time import uuid from datetime import datetime
771-785: Remove unused variable and use sanitized text in logssanitized_message is computed but unused in 401 branch; also ensure logs never print sensitive details.
- sanitized_message = _sanitize_openai_error(str(e)) raise HTTPException( status_code=401, detail={ "error": "OpenAI API authentication failed", "message": "Invalid or expired OpenAI API key. Please check your API key in settings.", "error_type": "authentication_failed", "api_key_prefix": getattr(e, "api_key_prefix", None), } )python/src/server/services/embeddings/embedding_exceptions.py (1)
75-88: Optional: decide masking at API boundary, not in exception typePrefer storing raw prefix in the exception for debugging correlation, and mask/redact only in HTTP responses/logs as needed. Aligns with sanitizer responsibilities.
python/src/server/services/embeddings/embedding_service.py (1)
221-231: Optional: surface retry-after when availableIf the provider exposes a retry-after header on 429s, capture and propagate via EmbeddingRateLimitError(metadata) so the API can return a dynamic retry_after instead of a fixed 30s.
- except openai.RateLimitError as e: + except openai.RateLimitError as e: error_message = str(e) + retry_after = getattr(getattr(e, "response", None), "headers", {}).get("retry-after") ... - else: - raise # Will be caught by outer try + else: + raise EmbeddingRateLimitError( + f"Rate limit hit: {error_message}", + retry_count=retry_count, + retry_after=retry_after if retry_after else None, + )And in API mapping, prefer using e.metadata.get("retry_after") when present. I can patch that if you want.
Also applies to: 268-279
archon-ui-main/src/components/mcp/ToolTestingPanel.tsx (2)
6-8: Import the display helper used in the catch block.Required for the change above.
-import { parseKnowledgeBaseError, EnhancedError } from '../../services/knowledgeBaseErrorHandler'; +import { parseKnowledgeBaseError, EnhancedError, getDisplayErrorMessage } from '../../services/knowledgeBaseErrorHandler';
98-104: Also clear typing intervals on unmount to prevent leaks.
addTypingLinesetssetIntervaltimers that continue after unmount. Track and clear them in cleanup.Additional changes outside the selected range:
// 1) Add near other refs const typingIntervalsRef = useRef<number[]>([]); // 2) In addTypingLine, replace setInterval(...) with: const intervalId = window.setInterval(() => { /* existing body */ }, 15); typingIntervalsRef.current.push(intervalId); // When clearing: clearInterval(intervalId); typingIntervalsRef.current = typingIntervalsRef.current.filter(id => id !== intervalId); // 3) In the unmount cleanup (this effect), also clear timers: return () => { clearError(); typingIntervalsRef.current.forEach(id => clearInterval(id)); typingIntervalsRef.current = []; };python/src/server/services/search/rag_service.py (1)
155-161: Add span attributes before re-raising to improve observability.Tagging the error class/type on the span helps correlate failures without altering behavior.
- except EmbeddingAuthenticationError: - # Let auth failures bubble to API layer -> 401 - raise - except (EmbeddingQuotaExhaustedError, EmbeddingRateLimitError, EmbeddingAPIError): - # Re-raise embedding errors so they propagate to the API layer with specific error info - raise + except EmbeddingAuthenticationError as e: + span.set_attribute("error_class", type(e).__name__) + span.set_attribute("error_auth", True) + raise + except (EmbeddingQuotaExhaustedError, EmbeddingRateLimitError, EmbeddingAPIError) as e: + span.set_attribute("error_class", type(e).__name__) + raiseOPENAI_ERROR_HANDLING.md (1)
3-3: Use a proper heading instead of emphasis (MD036).Switch the emphasized line to a heading to satisfy markdownlint.
-**Related to GitHub Issue #362** +## Related to GitHub Issue #362archon-ui-main/src/services/knowledgeBaseErrorHandler.ts (1)
54-58: Safer own-property check.Avoids issues if
hasOwnPropertyis shadowed.- for (const key in value) { - if (value.hasOwnProperty(key)) { - if (!checkDepth(value[key], currentDepth + 1)) return false; - } - } + for (const key in value) { + if (Object.prototype.hasOwnProperty.call(value, key)) { + if (!checkDepth(value[key], currentDepth + 1)) return false; + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (17)
OPENAI_ERROR_HANDLING.md(1 hunks)archon-ui-main/src/components/mcp/ToolTestingPanel.tsx(6 hunks)archon-ui-main/src/components/ui/Alert.tsx(1 hunks)archon-ui-main/src/components/ui/ErrorAlert.tsx(1 hunks)archon-ui-main/src/services/knowledgeBaseErrorHandler.ts(1 hunks)archon-ui-main/src/services/knowledgeBaseService.ts(3 hunks)python/src/server/api_routes/knowledge_api.py(1 hunks)python/src/server/services/crawling/crawling_service.py(1 hunks)python/src/server/services/crawling/document_storage_operations.py(2 hunks)python/src/server/services/credential_service.py(1 hunks)python/src/server/services/embeddings/embedding_exceptions.py(2 hunks)python/src/server/services/embeddings/embedding_service.py(5 hunks)python/src/server/services/llm_provider_service.py(1 hunks)python/src/server/services/search/rag_service.py(8 hunks)python/src/server/services/storage/document_storage_service.py(7 hunks)python/src/server/services/threading_service.py(1 hunks)python/tests/test_openai_quota_error_handling.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
python/src/{server,mcp,agents}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
python/src/{server,mcp,agents}/**/*.py: Fail fast on service startup failures, missing configuration, database connection issues, auth failures, critical dependency outages, and invalid data that would corrupt state
External API calls should use retry with exponential backoff and ultimately fail with a clear, contextual error message
Error messages must include context (operation being attempted) and relevant IDs/URLs/data for debugging
Preserve full stack traces in logs (e.g., Python logging with exc_info=True)
Use specific exception types; avoid catching broad Exception unless re-raising with context
Never signal failure by returning None/null; raise a descriptive exception instead
Files:
python/src/server/services/llm_provider_service.pypython/src/server/services/credential_service.pypython/src/server/services/threading_service.pypython/src/server/services/embeddings/embedding_service.pypython/src/server/services/crawling/crawling_service.pypython/src/server/services/crawling/document_storage_operations.pypython/src/server/services/storage/document_storage_service.pypython/src/server/api_routes/knowledge_api.pypython/src/server/services/search/rag_service.pypython/src/server/services/embeddings/embedding_exceptions.py
python/src/{server/services,agents}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Never accept or store corrupted data (e.g., zero embeddings, null foreign keys, malformed JSON); skip failed items entirely instead of persisting bad data
Files:
python/src/server/services/llm_provider_service.pypython/src/server/services/credential_service.pypython/src/server/services/threading_service.pypython/src/server/services/embeddings/embedding_service.pypython/src/server/services/crawling/crawling_service.pypython/src/server/services/crawling/document_storage_operations.pypython/src/server/services/storage/document_storage_service.pypython/src/server/services/search/rag_service.pypython/src/server/services/embeddings/embedding_exceptions.py
python/src/server/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
python/src/server/**/*.py: For batch processing and background tasks, continue processing but log detailed per-item failures and return both successes and failures
Do not crash the server on a single WebSocket event failure; log the error and continue serving other clients
Files:
python/src/server/services/llm_provider_service.pypython/src/server/services/credential_service.pypython/src/server/services/threading_service.pypython/src/server/services/embeddings/embedding_service.pypython/src/server/services/crawling/crawling_service.pypython/src/server/services/crawling/document_storage_operations.pypython/src/server/services/storage/document_storage_service.pypython/src/server/api_routes/knowledge_api.pypython/src/server/services/search/rag_service.pypython/src/server/services/embeddings/embedding_exceptions.py
python/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
python/**/*.py: Target Python 3.12 with a 120-character line length
Use Ruff for linting and Mypy for type checking before commit
Files:
python/src/server/services/llm_provider_service.pypython/src/server/services/credential_service.pypython/src/server/services/threading_service.pypython/src/server/services/embeddings/embedding_service.pypython/tests/test_openai_quota_error_handling.pypython/src/server/services/crawling/crawling_service.pypython/src/server/services/crawling/document_storage_operations.pypython/src/server/services/storage/document_storage_service.pypython/src/server/api_routes/knowledge_api.pypython/src/server/services/search/rag_service.pypython/src/server/services/embeddings/embedding_exceptions.py
{python/**/*.py,archon-ui-main/src/**/*.{ts,tsx,js,jsx}}
📄 CodeRabbit inference engine (CLAUDE.md)
{python/**/*.py,archon-ui-main/src/**/*.{ts,tsx,js,jsx}}: Remove dead code immediately; do not keep legacy/unused functions
Avoid comments that reference change history (e.g., LEGACY, CHANGED, REMOVED); keep comments focused on current functionality
Files:
python/src/server/services/llm_provider_service.pypython/src/server/services/credential_service.pypython/src/server/services/threading_service.pyarchon-ui-main/src/components/mcp/ToolTestingPanel.tsxarchon-ui-main/src/services/knowledgeBaseService.tsarchon-ui-main/src/components/ui/Alert.tsxpython/src/server/services/embeddings/embedding_service.pypython/tests/test_openai_quota_error_handling.pyarchon-ui-main/src/services/knowledgeBaseErrorHandler.tsarchon-ui-main/src/components/ui/ErrorAlert.tsxpython/src/server/services/crawling/crawling_service.pypython/src/server/services/crawling/document_storage_operations.pypython/src/server/services/storage/document_storage_service.pypython/src/server/api_routes/knowledge_api.pypython/src/server/services/search/rag_service.pypython/src/server/services/embeddings/embedding_exceptions.py
python/src/server/**
📄 CodeRabbit inference engine (CLAUDE.md)
Keep FastAPI application code under python/src/server/ (routes in api_routes/, services in services/, main in main.py)
Files:
python/src/server/services/llm_provider_service.pypython/src/server/services/credential_service.pypython/src/server/services/threading_service.pypython/src/server/services/embeddings/embedding_service.pypython/src/server/services/crawling/crawling_service.pypython/src/server/services/crawling/document_storage_operations.pypython/src/server/services/storage/document_storage_service.pypython/src/server/api_routes/knowledge_api.pypython/src/server/services/search/rag_service.pypython/src/server/services/embeddings/embedding_exceptions.py
archon-ui-main/src/components/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place reusable UI components in archon-ui-main/src/components/
Files:
archon-ui-main/src/components/mcp/ToolTestingPanel.tsxarchon-ui-main/src/components/ui/Alert.tsxarchon-ui-main/src/components/ui/ErrorAlert.tsx
archon-ui-main/src/services/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place API communication and business logic in archon-ui-main/src/services/
Files:
archon-ui-main/src/services/knowledgeBaseService.tsarchon-ui-main/src/services/knowledgeBaseErrorHandler.ts
python/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Place backend tests under python/tests/
Files:
python/tests/test_openai_quota_error_handling.py
🧬 Code graph analysis (10)
python/src/server/services/credential_service.py (1)
python/src/server/services/llm_provider_service.py (1)
invalidate_provider_cache(41-45)
archon-ui-main/src/components/mcp/ToolTestingPanel.tsx (2)
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts (1)
parseKnowledgeBaseError(75-138)archon-ui-main/src/components/ui/ErrorAlert.tsx (1)
ErrorAlert(21-88)
archon-ui-main/src/services/knowledgeBaseService.ts (1)
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts (1)
parseKnowledgeBaseError(75-138)
python/src/server/services/embeddings/embedding_service.py (1)
python/src/server/services/embeddings/embedding_exceptions.py (2)
EmbeddingAuthenticationError(75-87)EmbeddingAuthenticationError(101-114)
python/tests/test_openai_quota_error_handling.py (3)
python/src/server/services/embeddings/embedding_exceptions.py (5)
EmbeddingAuthenticationError(75-87)EmbeddingAuthenticationError(101-114)EmbeddingQuotaExhaustedError(46-58)EmbeddingRateLimitError(61-72)EmbeddingAPIError(117-130)python/src/server/services/search/rag_service.py (3)
RAGService(37-404)perform_rag_query(194-297)search_documents(93-165)python/src/server/api_routes/knowledge_api.py (3)
perform_rag_query(736-830)RagQueryRequest(100-103)_sanitize_openai_error(64-67)
archon-ui-main/src/components/ui/ErrorAlert.tsx (2)
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts (4)
EnhancedError(19-23)getErrorSeverity(180-199)getDisplayErrorMessage(143-175)getErrorAction(204-220)archon-ui-main/src/components/ui/Alert.tsx (2)
Alert(15-30)AlertDescription(37-46)
python/src/server/services/crawling/document_storage_operations.py (2)
python/src/server/services/storage/document_storage_service.py (1)
add_documents_to_supabase(18-399)python/src/server/config/logfire_config.py (1)
safe_logfire_error(238-250)
python/src/server/services/storage/document_storage_service.py (2)
python/src/server/services/embeddings/embedding_service.py (1)
has_failures(60-61)python/src/server/config/logfire_config.py (1)
set_attribute(177-179)
python/src/server/api_routes/knowledge_api.py (3)
python/src/server/services/storage/storage_services.py (2)
DocumentStorageService(19-284)upload_document(22-193)python/src/server/services/search/rag_service.py (1)
perform_rag_query(194-297)python/src/server/services/embeddings/embedding_exceptions.py (5)
EmbeddingAuthenticationError(75-87)EmbeddingAuthenticationError(101-114)EmbeddingAPIError(117-130)EmbeddingQuotaExhaustedError(46-58)EmbeddingRateLimitError(61-72)
python/src/server/services/search/rag_service.py (4)
python/src/server/services/embeddings/embedding_exceptions.py (5)
EmbeddingAuthenticationError(75-87)EmbeddingAuthenticationError(101-114)EmbeddingAPIError(117-130)EmbeddingQuotaExhaustedError(46-58)EmbeddingRateLimitError(61-72)python/src/server/config/logfire_config.py (3)
get_logger(136-146)safe_span(150-171)set_attribute(177-179)python/src/server/services/embeddings/embedding_service.py (1)
create_embedding(72-132)python/src/server/services/search/reranking_strategy.py (1)
rerank_results(139-194)
🪛 LanguageTool
OPENAI_ERROR_HANDLING.md
[grammar] ~5-~5: There might be a mistake here.
Context: ...ted to GitHub Issue #362**
Problem
Users experienced silent failures when O...
(QB_NEW_EN)
[grammar] ~6-~6: There might be a mistake here.
Context: ...blem
Users experienced silent failures when OpenAI API quota was exhausted, receivi...
(QB_NEW_EN)
[grammar] ~8-~8: There might be a mistake here.
Context: ...out any error indication.
Solution
Enhanced error handling throughout the s...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: .... Fixed Error Propagation in RAG Service
File: `python/src/server/services/sear...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ...`
2. Enhanced API Error Handling
File: `python/src/server/api_routes/kn...
(QB_NEW_EN)
[grammar] ~47-~47: There might be a mistake here.
Context: ...`
3. Comprehensive Test Coverage
File: `python/tests/test_openai_quota_...
(QB_NEW_EN)
[grammar] ~50-~50: There might be a mistake here.
Context: ...vice through RAG service to API endpoint
- Verifies correct HTTP status codes and e...
(QB_NEW_EN)
[grammar] ~51-~51: There might be a mistake here.
Context: ...rect HTTP status codes and error details
- Ensures successful queries still work af...
(QB_NEW_EN)
[grammar] ~56-~56: There might be a mistake here.
Context: ...
1. Error Parsing and Enhancement
File: `archon-ui-main/src/services/kno...
(QB_NEW_EN)
[grammar] ~59-~59: There might be a mistake here.
Context: ...rrors to identify OpenAI-specific issues
- Provides user-friendly error messages an...
(QB_NEW_EN)
[grammar] ~60-~60: There might be a mistake here.
Context: ...dly error messages and suggested actions
- Categorizes errors by severity level
...
(QB_NEW_EN)
[grammar] ~63-~63: There might be a mistake here.
Context: ... #### 2. Enhanced Knowledge Base Service
File: `archon-ui-main/src/services/kno...
(QB_NEW_EN)
[grammar] ~69-~69: There might be a mistake here.
Context: ...## 3. React Components for Error Display
Files:
- `archon-ui-main/src/compone...
(QB_NEW_EN)
[grammar] ~136-~136: There might be a mistake here.
Context: ...ry again"
- API Error (HTTP 502)
- Message: "OpenAI API error: [specific er...
(QB_NEW_EN)
[grammar] ~137-~137: There might be a mistake here.
Context: ...ge: "OpenAI API error: [specific error]"
- Action: "Verify your OpenAI API key in S...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
OPENAI_ERROR_HANDLING.md
3-3: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 Biome (2.1.2)
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts
[error] 210-210: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
archon-ui-main/src/components/ui/ErrorAlert.tsx
[error] 109-111: Expected a statement but instead found '}
- </>
- )'.
Expected a statement here.
(parse)
[error] 112-113: Expected a statement but instead found '* ```
*/'.
Expected a statement here.
(parse)
🔇 Additional comments (19)
python/src/server/services/storage/document_storage_service.py (3)
335-335: LGTM: accurate accounting of stored chunks on batch insert
367-367: LGTM: tracks successful individual inserts after batch failure
388-399: Span attributes and return payload look consistent and usefulGood: exposes success flag and key counters for downstream decisions.
python/src/server/services/crawling/document_storage_operations.py (1)
165-171: Return shape aligns with upstream usagechunk_count derived from chunks_stored is consistent with consumers.
archon-ui-main/src/services/knowledgeBaseService.ts (2)
136-137: LGTM: explicit timeout normalization to EnhancedError
139-146: LGTM: preserve enhanced OpenAI errors; normalize the restpython/src/server/services/crawling/crawling_service.py (2)
421-433: Nice: propagate embedding_failures into progress and finalizationThis unlocks partial-success handling on the frontend.
459-483: Success path completion payload matches prior expectationsCarries chunks_stored, code_examples_found, and sourceId; consistent with existing consumers.
python/src/server/services/embeddings/embedding_service.py (2)
233-237: Good: explicit mapping of OpenAI auth errors to EmbeddingAuthenticationErrorThis ensures 401 propagation and clean separation of concerns.
280-282: Good: auth errors bubble up immediatelyPrevents partial writes and makes failure modes clear to callers.
Also applies to: 333-335
archon-ui-main/src/components/mcp/ToolTestingPanel.tsx (3)
313-315: Proactive error reset before execution looks good.Prevents stale alerts from previous runs.
445-447: Good UX: centralized error surfacing via ErrorAlert.Placement and onDismiss wiring are correct.
42-42: Hook usage LGTM.Local error state via
useErrorHandlerkeeps concerns isolated to this panel.python/src/server/services/search/rag_service.py (3)
126-133: Correct single-text embedding usage and fail-fast behavior.Switch to
create_embedding(text=query)and raising on failure aligns with objectives #362/#371.
251-263: Reranking guard is correct.Conditional application with a boolean
reranking_appliedflag is clean and telemetry-friendly.
356-364: Code examples: correctreranking_appliedhandling.Accurately reflects reranking success/failure in response and spans.
archon-ui-main/src/components/ui/ErrorAlert.tsx (2)
21-87: Component implementation looks solid.Severity mapping, suggested actions, token details, and dismiss handling are appropriately wired.
113-125: Hook API is minimal and clear.
clearErrormemoization avoids re-renders; interface is easy to consume.archon-ui-main/src/services/knowledgeBaseErrorHandler.ts (1)
75-138: Normalization logic LGTM.Covers empty/invalid inputs, status mapping, and OpenAI-shaped payloads without overfitting.
| // Use enhanced error handling for better user experience | ||
| const enhancedError = parseKnowledgeBaseError({ | ||
| status: response.status, | ||
| error: error.error, | ||
| detail: error.detail | ||
| }); | ||
| throw enhancedError; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Safer error parsing when response body isn’t JSON
await response.json() can throw on HTML/plain errors; add a text fallback before normalizing via parseKnowledgeBaseError.
- const error = await response.json();
+ let error: any;
+ try {
+ error = await response.json();
+ } catch {
+ const text = await response.text();
+ error = { status: response.status, error: text };
+ }
@@
- const enhancedError = parseKnowledgeBaseError({
+ const enhancedError = parseKnowledgeBaseError({
status: response.status,
error: error.error,
detail: error.detail
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Use enhanced error handling for better user experience | |
| const enhancedError = parseKnowledgeBaseError({ | |
| status: response.status, | |
| error: error.error, | |
| detail: error.detail | |
| }); | |
| throw enhancedError; | |
| // Use enhanced error handling for better user experience | |
| let error: any; | |
| try { | |
| error = await response.json(); | |
| } catch { | |
| const text = await response.text(); | |
| error = { status: response.status, error: text }; | |
| } | |
| const enhancedError = parseKnowledgeBaseError({ | |
| status: response.status, | |
| error: error.error, | |
| detail: error.detail | |
| }); | |
| throw enhancedError; |
🤖 Prompt for AI Agents
In archon-ui-main/src/services/knowledgeBaseService.ts around lines 115 to 121,
the code assumes response.json() always succeeds; update the error-path to
attempt JSON parsing in a try/catch and fall back to response.text() if JSON
parse fails, then normalize the parsed value into the shape expected by
parseKnowledgeBaseError (extract error/detail if present, otherwise pass the raw
text as error) and throw the returned enhanced error.
| safe_logfire_error( | ||
| f"OpenAI API error during RAG query | error={str(e)} | query={request.query[:50]} | source={request.source}" | ||
| ) | ||
| sanitized_message = _sanitize_openai_error(str(e)) | ||
| raise HTTPException( | ||
| status_code=502, | ||
| detail={ | ||
| "error": "OpenAI API error", | ||
| "message": f"OpenAI API error: {sanitized_message}", | ||
| "error_type": "api_error", | ||
| } | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sanitize error text before logging to avoid leaking secrets
Log the sanitized message instead of raw exception text.
- safe_logfire_error(
- f"OpenAI API error during RAG query | error={str(e)} | query={request.query[:50]} | source={request.source}"
- )
- sanitized_message = _sanitize_openai_error(str(e))
+ sanitized_message = _sanitize_openai_error(str(e))
+ safe_logfire_error(
+ f"OpenAI API error during RAG query | error={sanitized_message} | query={request.query[:50]} | source={request.source}"
+ )
raise HTTPException(
status_code=502,
detail={
"error": "OpenAI API error",
"message": f"OpenAI API error: {sanitized_message}",
"error_type": "api_error",
}
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| safe_logfire_error( | |
| f"OpenAI API error during RAG query | error={str(e)} | query={request.query[:50]} | source={request.source}" | |
| ) | |
| sanitized_message = _sanitize_openai_error(str(e)) | |
| raise HTTPException( | |
| status_code=502, | |
| detail={ | |
| "error": "OpenAI API error", | |
| "message": f"OpenAI API error: {sanitized_message}", | |
| "error_type": "api_error", | |
| } | |
| ) | |
| sanitized_message = _sanitize_openai_error(str(e)) | |
| safe_logfire_error( | |
| f"OpenAI API error during RAG query | error={sanitized_message} | query={request.query[:50]} | source={request.source}" | |
| ) | |
| raise HTTPException( | |
| status_code=502, | |
| detail={ | |
| "error": "OpenAI API error", | |
| "message": f"OpenAI API error: {sanitized_message}", | |
| "error_type": "api_error", | |
| } | |
| ) |
🤖 Prompt for AI Agents
In python/src/server/api_routes/knowledge_api.py around lines 813 to 824, the
code logs the raw exception text then sanitizes it for the HTTP response; change
the order so you sanitize the exception first and then log the sanitized
message. Specifically, call sanitized_message = _sanitize_openai_error(str(e))
before safe_logfire_error and pass sanitized_message into safe_logfire_error
(and keep using sanitized_message in the HTTPException detail) to avoid leaking
secrets in logs.
| class EmbeddingAuthenticationError(EmbeddingError): | ||
| """ | ||
| Raised when API authentication fails (invalid API key, expired key, etc). | ||
|
|
||
| This is a CRITICAL error that should stop the entire process | ||
| as continuing would be pointless without valid authentication. | ||
| """ | ||
|
|
||
| def __init__(self, message: str, api_key_prefix: str | None = None, **kwargs): | ||
| super().__init__(message, **kwargs) | ||
| self.api_key_prefix = api_key_prefix | ||
| if api_key_prefix: | ||
| self.metadata["api_key_prefix"] = api_key_prefix | ||
|
|
There was a problem hiding this comment.
Duplicate EmbeddingAuthenticationError class — remove one to avoid shadowing and test failures
Two class definitions exist; the later masks the former. This will cause confusion and inconsistencies (e.g., masked prefix vs. raw). Keep a single definition. Given tests assert api_key_prefix == "sk-1234", retain the first (raw) variant and delete the second.
-class EmbeddingAuthenticationError(EmbeddingError):
- """
- Raised when API authentication fails (invalid API key, expired key, etc).
-
- This is a CRITICAL error that should stop the entire process
- as continuing would be pointless without valid authentication.
- """
-
- def __init__(self, message: str, api_key_prefix: str | None = None, **kwargs):
- super().__init__(message, **kwargs)
- masked = f"{api_key_prefix[:4]}…" if api_key_prefix else None
- self.api_key_prefix = masked
- if masked:
- self.metadata["api_key_prefix"] = maskedAlso applies to: 101-115
🤖 Prompt for AI Agents
python/src/server/services/embeddings/embedding_exceptions.py lines 75-88 (and
also remove the duplicate at 101-115): there are two definitions of
EmbeddingAuthenticationError causing the later one to shadow the earlier; keep
the first/raw variant and delete the second duplicate definition (remove lines
101-115), ensuring any references/imports still point to the retained class and
that the api_key_prefix metadata handling remains as in the first definition.
| # Log any failures and track them | ||
| if result.has_failures: | ||
| total_embedding_failures += result.failure_count | ||
| search_logger.error( |
There was a problem hiding this comment.
Bug: has_failures is a method, but used as a property — condition is always True
This will miscount failures and spam error logs. Call the method.
- if result.has_failures:
+ if result.has_failures():
total_embedding_failures += result.failure_count📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Log any failures and track them | |
| if result.has_failures: | |
| total_embedding_failures += result.failure_count | |
| search_logger.error( | |
| # Log any failures and track them | |
| if result.has_failures(): | |
| total_embedding_failures += result.failure_count | |
| search_logger.error( | |
| # … | |
| ) |
🤖 Prompt for AI Agents
In python/src/server/services/storage/document_storage_service.py around lines
246 to 249, the review points out that result.has_failures is a method but is
being used as a property, causing the condition to always evaluate truthy and
leading to miscounted failures and excessive error logs; update the condition to
call the method (result.has_failures()) and ensure you use result.failure_count
only when the method returns True so total_embedding_failures is incremented
correctly and search_logger.error is only invoked when actual failures exist.
| def test_sanitize_openai_error_removes_urls(self): | ||
| """Test that sanitization function removes URLs from error messages.""" | ||
| error_message = "Connection failed to https://api.openai.com/v1/embeddings with status 400" | ||
| sanitized = _sanitize_openai_error(error_message) | ||
|
|
||
| assert "https://api.openai.com" not in sanitized | ||
| assert "[REDACTED_URL]" in sanitized | ||
| assert "Connection failed" in sanitized |
There was a problem hiding this comment.
Sanitizer tests will fail with current stub — implement sanitizer accordingly
Multiple tests expect URL/key/org/proj/req redaction, Bearer-token hard generic fallback, and safe-message passthrough. Current implementation is a stub returning a generic message. Implement the sanitizer in knowledge_api.py to match these expectations (see suggested diff in that file).
Also applies to: 238-246, 247-254, 255-262, 263-270, 271-297, 399-415, 442-458
🤖 Prompt for AI Agents
In python/tests/test_openai_quota_error_handling.py around lines 229 to 236, the
tests expect the OpenAI error sanitizer to remove/replace URLs, API
keys/org/proj/req identifiers, redact Bearer tokens with a generic placeholder,
and preserve safe messages, but the current sanitizer stub returns a generic
message; update the sanitizer implementation in knowledge_api.py to parse input
error strings (and exceptions), replace any URL patterns with "[REDACTED_URL]",
replace bearer/authorization tokens with "[REDACTED_BEARER_TOKEN]" (and other
API keys/ids with "[REDACTED_KEY]" or appropriate placeholders), preserve safe
non-sensitive text, and ensure tests across the specified line ranges (238-246,
247-254, 255-262, 263-270, 271-297, 399-415, 442-458) are satisfied by handling
edge cases like multiple redactions and returning the sanitized message string
rather than a hardcoded generic message.
- Validate API key before starting crawls, refreshes, and uploads - Users now get immediate HTTP 401 feedback instead of waiting through entire crawl - Prevents time waste from crawling with invalid/expired API keys - Addresses user feedback about discovering auth failures too late in process 🎯 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…ePage - Import and use parseKnowledgeBaseError, getDisplayErrorMessage, getErrorAction - Replace generic error messages with enhanced OpenAI-specific feedback - Users now see actionable errors instead of generic 'Failed to...' messages - Fix for merge overwriting UI error enhancements from original implementation 🎯 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
python/src/server/api_routes/knowledge_api.py (2)
64-68: Implement robust, bounded sanitizer to prevent info disclosureCurrent stub does not meet the PR’s security goals and past review. Replace with bounded redaction and safe fallback.
-def _sanitize_openai_error(error_message: str) -> str: - """Sanitize OpenAI API error messages to prevent information disclosure.""" - # Simple fallback implementation - return "OpenAI API encountered an error. Please verify your API key and quota." +def _sanitize_openai_error(error_message: str) -> str: + """Sanitize OpenAI/OpenAI-compatible error messages without leaking secrets.""" + GENERIC = "OpenAI API encountered an error. Please verify your API key and quota." + try: + if not isinstance(error_message, str) or not error_message.strip(): + return GENERIC + msg = error_message[:2000] + lower = msg.lower() + # Hard generic for highly sensitive markers + if "bearer " in lower or "auth_bearer" in lower: + return GENERIC + # Bounded redactions + patterns = ( + (r"https?://[^\s\"'<>]+", "[REDACTED_URL]"), + (r"\bsk-[A-Za-z0-9]{10,}\b", "[REDACTED_KEY]"), + (r"\borg-[A-Za-z0-9\-]{8,}\b", "[REDACTED_ORG]"), + (r"\bproj_[A-Za-z0-9]{6,}\b", "[REDACTED_PROJ]"), + (r"\breq_[A-Za-z0-9]{6,}\b", "[REDACTED_REQ]"), + (r"\bBearer\s+[A-Za-z0-9\-\._~\+/]+=*", "[REDACTED_TOKEN]"), + ) + sanitized = msg + for pat, repl in patterns: + sanitized = re.sub(pat, repl, sanitized) + return sanitized.strip() or GENERIC + except Exception: + return GENERIC
864-868: Log sanitized message, not raw exception textSanitize before logging to avoid leaking secrets.
- safe_logfire_error( - f"OpenAI API error during RAG query | error={str(e)} | query={request.query[:50]} | source={request.source}" - ) - sanitized_message = _sanitize_openai_error(str(e)) + sanitized_message = _sanitize_openai_error(str(e)) + safe_logfire_error( + f"OpenAI API error during RAG query | error={sanitized_message} | query={request.query[:50]} | source={request.source}" + )
🧹 Nitpick comments (7)
python/src/server/api_routes/knowledge_api.py (7)
21-33: Deduplicate imports and add missingreimportThere are repeated imports for
get_crawler,RAGService,DocumentStorageService, andget_supabase_client. Also,_sanitize_openai_errorneedsre. Clean up to satisfy Ruff and avoid confusion.@@ -import asyncio -import json -import time -import uuid +import asyncio +import json +import time +import uuid +import re @@ -from ..services.crawler_manager import get_crawler -# Import unified logging +from ..services.crawler_manager import get_crawler +# Import unified logging from ..config.logfire_config import get_logger, safe_logfire_error, safe_logfire_info -from ..services.crawler_manager import get_crawler -from ..services.search.rag_service import RAGService -from ..services.storage import DocumentStorageService -from ..utils import get_supabase_client +from ..services.search.rag_service import RAGService +from ..services.storage import DocumentStorageService +from ..utils import get_supabase_client
19-20: UseField(default_factory=list)for mutable defaultsPrevents shared list instances in Pydantic models.
-from pydantic import BaseModel +from pydantic import BaseModel, Field @@ - tags: list[str] = [] + tags: list[str] = Field(default_factory=list) @@ - tags: list[str] = [] + tags: list[str] = Field(default_factory=list)Also applies to: 116-117, 137-138
827-827: Remove unused variable
sanitized_messageis computed but never used.- sanitized_message = _sanitize_openai_error(str(e))
855-861: Include retry metadata in rate-limit responsesExpose retry info for smarter UI.
detail={ "error": "OpenAI API rate limit exceeded", "message": "Too many requests to OpenAI API. Please wait a moment and try again.", "error_type": "rate_limit", - "retry_after": 30, # Suggest 30 second wait + "retry_after": 30, # Suggest 30 second wait + "retry_count": getattr(e, "retry_count", 0), }
1060-1061: Catchasyncio.TimeoutErroronly (built-in alias redundancy)Use the explicit asyncio type; the built-in alias is redundant.
- except (TimeoutError, asyncio.CancelledError): + except (asyncio.TimeoutError, asyncio.CancelledError): pass
684-691: Offload document text extraction to a thread pool
extract_text_from_documentis likely CPU-bound; offload to avoid blocking the event loop.I can provide a small refactor using
anyio.to_thread.run_syncorloop.run_in_executorif desired.
591-597: Consider enforcing upload size limits before reading entire fileProtects memory and DoS; e.g., reject >10–20MB with 413.
I can wire a streaming read + size check tailored to Starlette’s
UploadFile.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
python/src/server/api_routes/knowledge_api.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
python/src/{server,mcp,agents}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
python/src/{server,mcp,agents}/**/*.py: Fail fast on service startup failures, missing configuration, database connection issues, auth failures, critical dependency outages, and invalid data that would corrupt state
External API calls should use retry with exponential backoff and ultimately fail with a clear, contextual error message
Error messages must include context (operation being attempted) and relevant IDs/URLs/data for debugging
Preserve full stack traces in logs (e.g., Python logging with exc_info=True)
Use specific exception types; avoid catching broad Exception unless re-raising with context
Never signal failure by returning None/null; raise a descriptive exception instead
Files:
python/src/server/api_routes/knowledge_api.py
python/src/server/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
python/src/server/**/*.py: For batch processing and background tasks, continue processing but log detailed per-item failures and return both successes and failures
Do not crash the server on a single WebSocket event failure; log the error and continue serving other clients
Files:
python/src/server/api_routes/knowledge_api.py
python/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
python/**/*.py: Target Python 3.12 with a 120-character line length
Use Ruff for linting and Mypy for type checking before commit
Files:
python/src/server/api_routes/knowledge_api.py
{python/**/*.py,archon-ui-main/src/**/*.{ts,tsx,js,jsx}}
📄 CodeRabbit inference engine (CLAUDE.md)
{python/**/*.py,archon-ui-main/src/**/*.{ts,tsx,js,jsx}}: Remove dead code immediately; do not keep legacy/unused functions
Avoid comments that reference change history (e.g., LEGACY, CHANGED, REMOVED); keep comments focused on current functionality
Files:
python/src/server/api_routes/knowledge_api.py
python/src/server/**
📄 CodeRabbit inference engine (CLAUDE.md)
Keep FastAPI application code under python/src/server/ (routes in api_routes/, services in services/, main in main.py)
Files:
python/src/server/api_routes/knowledge_api.py
🧬 Code graph analysis (1)
python/src/server/api_routes/knowledge_api.py (7)
python/src/server/services/storage/storage_services.py (2)
DocumentStorageService(19-284)upload_document(22-193)python/src/server/services/search/rag_service.py (1)
perform_rag_query(194-297)python/src/server/services/knowledge/knowledge_item_service.py (2)
list_items(26-193)get_item(195-228)python/src/server/services/embeddings/embedding_exceptions.py (5)
EmbeddingAuthenticationError(75-87)EmbeddingAuthenticationError(101-114)EmbeddingQuotaExhaustedError(46-58)EmbeddingAPIError(117-130)EmbeddingRateLimitError(61-72)python/src/server/services/embeddings/embedding_service.py (1)
create_embedding(72-132)python/src/server/services/source_management_service.py (2)
SourceManagementService(337-660)delete_source(372-440)python/src/server/services/crawling/crawling_service.py (5)
set_progress_id(112-116)orchestrate_crawl(248-277)get_active_orchestration(56-58)unregister_orchestration(66-69)cancel(118-121)
| # Track active async crawl tasks for cancellation support | ||
| active_crawl_tasks: dict[str, asyncio.Task] = {} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add TTL cache to API key preflight and sanitize unexpected error logs
Avoid paying per-request embedding cost and spiking rate limits; log only sanitized text.
@@
-active_crawl_tasks: dict[str, asyncio.Task] = {}
+active_crawl_tasks: dict[str, asyncio.Task] = {}
+_last_key_validation_ts: float = 0.0
+_KEY_VALIDATION_TTL_SEC = 600 # 10 minutes
@@
async def _validate_openai_api_key() -> None:
@@
- try:
+ try:
+ # TTL cache to avoid repeated paid calls
+ global _last_key_validation_ts
+ now = time.monotonic()
+ if now - _last_key_validation_ts < _KEY_VALIDATION_TTL_SEC:
+ return
@@
- await create_embedding(text="test")
+ await create_embedding(text="test")
+ _last_key_validation_ts = now
@@
- except EmbeddingAuthenticationError as e:
+ except EmbeddingAuthenticationError as e:
raise HTTPException(
status_code=401,
detail={
"error": "Invalid OpenAI API key",
- "message": "Please verify your OpenAI API key in Settings before starting a crawl.",
- "error_type": "authentication_required"
+ "message": "Please verify your OpenAI API key in Settings before starting a crawl.",
+ "error_type": "authentication_failed",
+ "api_key_prefix": getattr(e, "api_key_prefix", None),
}
)
@@
- except Exception as e:
+ except Exception as e:
# For any other errors, allow the crawl to continue
# The error will be caught later during actual processing
- logger.warning(f"API key validation failed with unexpected error: {e}")
+ sanitized = _sanitize_openai_error(str(e))
+ logger.warning(f"API key validation failed with unexpected error: {sanitized}")
passAlso applies to: 69-110
🤖 Prompt for AI Agents
In python/src/server/api_routes/knowledge_api.py around lines 60-62 and 69-110,
the API key preflight check is being executed on every request causing
unnecessary embedding costs and rate spikes, and unexpected errors are logged
verbatim; add a TTL-backed cache (e.g., in-memory dict with expiry or use
cachetools.TTLCache) keyed by API key to memoize successful preflight results
for a short duration (configurable, e.g., 5-10 minutes) and consult the cache
before performing the preflight call, and change the error logging to sanitize
and redact sensitive data by logging only a short, non-sensitive summary (error
type, sanitized message, and correlation id) rather than full error contents or
raw inputs; ensure cache is thread-safe for asyncio and that failures invalidate
cache entries and fall back to fresh preflight.
| # Parse tags | ||
| tag_list = json.loads(tags) if tags else [] | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate tags JSON and return 422 instead of 500 on bad input
Avoids generic 500s for client errors.
- # Parse tags
- tag_list = json.loads(tags) if tags else []
+ # Parse tags
+ try:
+ tag_list = json.loads(tags) if tags else []
+ if not isinstance(tag_list, list):
+ raise ValueError("Tags must be a JSON array")
+ except (json.JSONDecodeError, ValueError):
+ await error_crawl_progress(progress_id, "Invalid tags format; expected JSON array")
+ raise HTTPException(status_code=422, detail={"error": "Invalid tags format; expected JSON array"})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Parse tags | |
| tag_list = json.loads(tags) if tags else [] | |
| # Parse tags | |
| try: | |
| tag_list = json.loads(tags) if tags else [] | |
| if not isinstance(tag_list, list): | |
| raise ValueError("Tags must be a JSON array") | |
| except (json.JSONDecodeError, ValueError): | |
| await error_crawl_progress(progress_id, "Invalid tags format; expected JSON array") | |
| raise HTTPException(status_code=422, detail={"error": "Invalid tags format; expected JSON array"}) |
🤖 Prompt for AI Agents
In python/src/server/api_routes/knowledge_api.py around lines 588 to 590, the
code directly calls json.loads(tags) which will raise a JSONDecodeError and
produce a 500 for bad client input; wrap the json.loads call in a try/except
catching json.JSONDecodeError (and TypeError if tags can be non-string), and on
failure return an HTTP 422 response with a clear error message (e.g., "Invalid
tags JSON") instead of letting the exception bubble to a 500; if successful,
continue using the parsed tag_list.
| if success: | ||
| # Add success flag to match expected API response format | ||
| result["success"] = True | ||
| return result | ||
| else: | ||
| raise HTTPException( | ||
| status_code=500, detail={"error": result.get("error", "RAG query failed")} | ||
| ) | ||
| except HTTPException: |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Defensively map embedding errors when service returns (False, result) instead of raising
If RAGService.perform_rag_query returns (False, result) for embedding failures, ensure correct HTTP status mapping rather than 500.
- else:
- raise HTTPException(
- status_code=500, detail={"error": result.get("error", "RAG query failed")}
- )
+ else:
+ err = str(result.get("error", ""))
+ err_type = str(result.get("error_type", ""))
+ if "EmbeddingAuthenticationError" in err_type:
+ raise HTTPException(status_code=401, detail={
+ "error": "OpenAI API authentication failed",
+ "message": "Invalid or expired OpenAI API key. Please check your API key in settings.",
+ "error_type": "authentication_failed",
+ })
+ if "EmbeddingQuotaExhaustedError" in err_type or "quota" in err.lower():
+ raise HTTPException(status_code=429, detail={
+ "error": "OpenAI API quota exhausted",
+ "message": "Your OpenAI API key has no remaining credits. Please add credits.",
+ "error_type": "quota_exhausted",
+ })
+ if "EmbeddingRateLimitError" in err_type or "rate limit" in err.lower():
+ raise HTTPException(status_code=429, detail={
+ "error": "OpenAI API rate limit exceeded",
+ "message": "Too many requests. Please wait and try again.",
+ "error_type": "rate_limit",
+ })
+ if "EmbeddingAPIError" in err_type:
+ sanitized = _sanitize_openai_error(err)
+ raise HTTPException(status_code=502, detail={
+ "error": "OpenAI API error",
+ "message": f"OpenAI API error: {sanitized}",
+ "error_type": "api_error",
+ })
+ raise HTTPException(status_code=500, detail={"error": result.get("error", "RAG query failed")})Run to verify the service re-raises as intended:
🏁 Script executed:
#!/bin/bash
# Verify rag_service raises embedding exceptions (and not swallow them)
rg -nP -C2 'perform_rag_query\(|except\s+Embedding(Authentication|Quota|RateLimit|API)Error|raise\s+Embedding(Authentication|Quota|RateLimit|API)Error' python/src/server/services/search/rag_service.pyLength of output: 936
🏁 Script executed:
#!/bin/bash
sed -n '750,860p' python/src/server/api_routes/knowledge_api.pyLength of output: 4853
🏁 Script executed:
#!/bin/bash
# Search for any 'return False' occurrences in perform_rag_query to see if embedding errors are returned instead of raised
rg -nP 'return\s+False' -C3 python/src/server/services/search/rag_service.pyLength of output: 984
🏁 Script executed:
#!/bin/bash
# Show the beginning of RAGService implementation to inspect exception handling and return paths
sed -n '1,200p' python/src/server/services/search/rag_service.pyLength of output: 8104
🏁 Script executed:
#!/bin/bash
sed -n '250,350p' python/src/server/services/search/rag_service.pyLength of output: 4348
Map embedding failures to correct HTTP statuses in perform_rag_query
The RAGService returns (False, result) for quota, rate-limit and other embedding errors (except authentication), but the else branch always raises a 500. Update the else block to inspect result["error_type"] (and message) and re-raise as 401/429/502 for EmbeddingAuthenticationError, EmbeddingQuotaExhaustedError/rate limits, and EmbeddingAPIError respectively. For example:
- else:
- raise HTTPException(
- status_code=500, detail={"error": result.get("error", "RAG query failed")}
- )
+ else:
+ err = str(result.get("error", ""))
+ err_type = result.get("error_type", "")
+ if "EmbeddingAuthenticationError" in err_type:
+ raise HTTPException(status_code=401, detail={
+ "error": "OpenAI API authentication failed",
+ "message": "Invalid or expired OpenAI API key. Please check your API key in settings.",
+ "error_type": "authentication_failed",
+ })
+ if "EmbeddingQuotaExhaustedError" in err_type or "quota" in err.lower():
+ raise HTTPException(status_code=429, detail={
+ "error": "OpenAI API quota exhausted",
+ "message": "Your OpenAI API key has no remaining credits. Please add credits.",
+ "error_type": "quota_exhausted",
+ })
+ if "EmbeddingRateLimitError" in err_type or "rate limit" in err.lower():
+ raise HTTPException(status_code=429, detail={
+ "error": "OpenAI API rate limit exceeded",
+ "message": "Too many requests. Please wait and try again.",
+ "error_type": "rate_limit",
+ })
+ if "EmbeddingAPIError" in err_type:
+ sanitized = _sanitize_openai_error(err)
+ raise HTTPException(status_code=502, detail={
+ "error": "OpenAI API error",
+ "message": f"OpenAI API error: {sanitized}",
+ "error_type": "api_error",
+ })
+ raise HTTPException(status_code=500, detail={"error": result.get("error", "RAG query failed")})This ensures embedding errors returned by the service are mapped to the proper status codes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if success: | |
| # Add success flag to match expected API response format | |
| result["success"] = True | |
| return result | |
| else: | |
| raise HTTPException( | |
| status_code=500, detail={"error": result.get("error", "RAG query failed")} | |
| ) | |
| except HTTPException: | |
| if success: | |
| # Add success flag to match expected API response format | |
| result["success"] = True | |
| return result | |
| else: | |
| err = str(result.get("error", "")) | |
| err_type = result.get("error_type", "") | |
| # Authentication failures → 401 | |
| if "EmbeddingAuthenticationError" in err_type: | |
| raise HTTPException( | |
| status_code=401, | |
| detail={ | |
| "error": "OpenAI API authentication failed", | |
| "message": "Invalid or expired OpenAI API key. Please check your API key in settings.", | |
| "error_type": "authentication_failed", | |
| }, | |
| ) | |
| # Quota exhausted → 429 | |
| if "EmbeddingQuotaExhaustedError" in err_type or "quota" in err.lower(): | |
| raise HTTPException( | |
| status_code=429, | |
| detail={ | |
| "error": "OpenAI API quota exhausted", | |
| "message": "Your OpenAI API key has no remaining credits. Please add credits.", | |
| "error_type": "quota_exhausted", | |
| }, | |
| ) | |
| # Rate limiting → 429 | |
| if "EmbeddingRateLimitError" in err_type or "rate limit" in err.lower(): | |
| raise HTTPException( | |
| status_code=429, | |
| detail={ | |
| "error": "OpenAI API rate limit exceeded", | |
| "message": "Too many requests. Please wait and try again.", | |
| "error_type": "rate_limit", | |
| }, | |
| ) | |
| # Other API errors → 502 | |
| if "EmbeddingAPIError" in err_type: | |
| sanitized = _sanitize_openai_error(err) | |
| raise HTTPException( | |
| status_code=502, | |
| detail={ | |
| "error": "OpenAI API error", | |
| "message": f"OpenAI API error: {sanitized}", | |
| "error_type": "api_error", | |
| }, | |
| ) | |
| # Fallback to generic server error → 500 | |
| raise HTTPException( | |
| status_code=500, | |
| detail={"error": result.get("error", "RAG query failed")}, | |
| ) | |
| except HTTPException: |
🤖 Prompt for AI Agents
In python/src/server/api_routes/knowledge_api.py around lines 803 to 811, the
else branch always raises a 500 for embedding failures; inspect
result["error_type"] and map known embedding errors to proper HTTP statuses: if
error_type == "EmbeddingAuthenticationError" raise
HTTPException(status_code=401, detail=...), if error_type in
("EmbeddingQuotaExhaustedError","EmbeddingRateLimitError") raise
HTTPException(status_code=429, detail=...), if error_type == "EmbeddingAPIError"
raise HTTPException(status_code=502, detail=...), otherwise fall back to 500;
include the original result.get("error") message in the detail payload for all
cases.
| safe_logfire_error( | ||
| f"RAG query failed | error={str(e)} | query={request.query[:50]} | source={request.source}" | ||
| ) | ||
| raise HTTPException(status_code=500, detail={"error": f"RAG query failed: {str(e)}"}) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sanitize generic error logs and responses
Prevents propagating sensitive details in logs and payloads.
- safe_logfire_error(
- f"RAG query failed | error={str(e)} | query={request.query[:50]} | source={request.source}"
- )
- raise HTTPException(status_code=500, detail={"error": f"RAG query failed: {str(e)}"})
+ sanitized_message = _sanitize_openai_error(str(e))
+ safe_logfire_error(
+ f"RAG query failed | error={sanitized_message} | query={request.query[:50]} | source={request.source}"
+ )
+ raise HTTPException(status_code=500, detail={"error": f"RAG query failed: {sanitized_message}"})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| safe_logfire_error( | |
| f"RAG query failed | error={str(e)} | query={request.query[:50]} | source={request.source}" | |
| ) | |
| raise HTTPException(status_code=500, detail={"error": f"RAG query failed: {str(e)}"}) | |
| sanitized_message = _sanitize_openai_error(str(e)) | |
| safe_logfire_error( | |
| f"RAG query failed | error={sanitized_message} | query={request.query[:50]} | source={request.source}" | |
| ) | |
| raise HTTPException(status_code=500, detail={"error": f"RAG query failed: {sanitized_message}"}) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
python/tests/test_rag_strategies.py (1)
361-367: Strengthen test_error_handling_in_rag_pipeline: verify fail-fast behavior and use resilient error assertions
- Patch
rag_service.base_strategy.vector_searchand assert it’s never called.- Avoid brittle exact-string checks; assert on the structured
error_type == "RuntimeError"or lowercaseerrorcontaining"embedding"and one of"failed"|"configuration"|"api".@@ python/tests/test_rag_strategies.py:363 - # Should now fail fast and return error result instead of empty results - success, result = await rag_service.perform_rag_query(query="test query", match_count=5) - - # Should return failure result due to "fail fast" principle - assert success is False - assert "error" in result - assert "Failed to create embedding" in result["error"] or "configuration or API issue" in result["error"] + from unittest.mock import patch as _patch + # Verify fail-fast: embeddings fail → no vector_search call + with _patch.object(rag_service.base_strategy, "vector_search") as mock_vector_search: + success, result = await rag_service.perform_rag_query(query="test query", match_count=5) + assert success is False + mock_vector_search.assert_not_called() + # Resilient error contract assertions + assert result.get("error_type") == "RuntimeError" + err = str(result.get("error", "")).lower() + assert "embedding" in err and any(tok in err for tok in ("failed", "configuration", "api"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
python/tests/test_rag_strategies.py(1 hunks)python/tests/test_source_url_shadowing.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- python/tests/test_source_url_shadowing.py
🧰 Additional context used
📓 Path-based instructions (3)
python/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
python/**/*.py: Target Python 3.12 with a 120-character line length
Use Ruff for linting and Mypy for type checking before commit
Files:
python/tests/test_rag_strategies.py
{python/**/*.py,archon-ui-main/src/**/*.{ts,tsx,js,jsx}}
📄 CodeRabbit inference engine (CLAUDE.md)
{python/**/*.py,archon-ui-main/src/**/*.{ts,tsx,js,jsx}}: Remove dead code immediately; do not keep legacy/unused functions
Avoid comments that reference change history (e.g., LEGACY, CHANGED, REMOVED); keep comments focused on current functionality
Files:
python/tests/test_rag_strategies.py
python/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Place backend tests under python/tests/
Files:
python/tests/test_rag_strategies.py
…on failure - Replace EmbeddingResult import with mock object creation - Fix ImportError that was preventing test collection - Use dynamic type creation instead of importing non-existent class 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
python/tests/test_rag_strategies.py (1)
393-400: Reduce brittleness of error-message assertion.Avoid coupling tests to specific wording; assert presence/type of error instead.
- assert "Failed to create embedding" in result["error"] or "configuration or API issue" in result["error"] + assert isinstance(result["error"], str) and result["error"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
python/tests/test_document_storage_metrics.py(4 hunks)python/tests/test_rag_strategies.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- python/tests/test_document_storage_metrics.py
🧰 Additional context used
📓 Path-based instructions (3)
python/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
python/**/*.py: Target Python 3.12 with a 120-character line length
Use Ruff for linting and Mypy for type checking before commit
Files:
python/tests/test_rag_strategies.py
{python/**/*.py,archon-ui-main/src/**/*.{ts,tsx,js,jsx}}
📄 CodeRabbit inference engine (CLAUDE.md)
{python/**/*.py,archon-ui-main/src/**/*.{ts,tsx,js,jsx}}: Remove dead code immediately; do not keep legacy/unused functions
Avoid comments that reference change history (e.g., LEGACY, CHANGED, REMOVED); keep comments focused on current functionality
Files:
python/tests/test_rag_strategies.py
python/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Place backend tests under python/tests/
Files:
python/tests/test_rag_strategies.py
🧬 Code graph analysis (1)
python/tests/test_rag_strategies.py (1)
python/src/server/services/embeddings/embedding_service.py (1)
has_failures(60-61)
🪛 GitHub Actions: Continuous Integration
python/tests/test_rag_strategies.py
[error] 62-62: ImportError: cannot import name 'EmbeddingResult' from 'src.server.services.embeddings.embedding_service' (/home/runner/work/Archion/Archon/python/src/server/services/embeddings/embedding_service.py)
| # Global mock to prevent any real API calls during testing | ||
| @pytest.fixture(autouse=True) | ||
| def mock_embedding_service(): | ||
| """Auto-use fixture to mock embedding service globally""" | ||
| with patch("src.server.services.embeddings.embedding_service.create_embedding") as mock_embed: | ||
| mock_embed.return_value = [0.1] * 1536 | ||
| with patch("src.server.services.embeddings.embedding_service.create_embeddings_batch") as mock_batch: | ||
| from src.server.services.embeddings.embedding_service import EmbeddingResult | ||
| mock_batch.return_value = EmbeddingResult( | ||
| embeddings=[[0.1] * 1536], | ||
| texts_processed=["test"], | ||
| success_count=1, | ||
| failure_count=0, | ||
| has_failures=False, | ||
| failed_items=[] | ||
| ) | ||
| yield | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix failing CI: stop importing EmbeddingResult; return a mock with has_failures().
The pipeline error shows ImportError for EmbeddingResult and, per implementation, has_failures is likely a method, not a field. Replace the import and constructor with a MagicMock that provides attributes and a callable has_failures().
@@
- with patch("src.server.services.embeddings.embedding_service.create_embeddings_batch") as mock_batch:
- from src.server.services.embeddings.embedding_service import EmbeddingResult
- mock_batch.return_value = EmbeddingResult(
- embeddings=[[0.1] * 1536],
- texts_processed=["test"],
- success_count=1,
- failure_count=0,
- has_failures=False,
- failed_items=[]
- )
+ with patch("src.server.services.embeddings.embedding_service.create_embeddings_batch") as mock_batch:
+ embedding_batch_result = MagicMock()
+ embedding_batch_result.embeddings = [[0.1] * 1536]
+ embedding_batch_result.texts_processed = ["test"]
+ embedding_batch_result.success_count = 1
+ embedding_batch_result.failure_count = 0
+ embedding_batch_result.failed_items = []
+ embedding_batch_result.has_failures.return_value = False
+ mock_batch.return_value = embedding_batch_resultRun to ensure no remaining test-time imports of EmbeddingResult:
🏁 Script executed:
#!/usr/bin/env bash
rg -n "EmbeddingResult" python/tests/ -C1Length of output: 909
Stop importing EmbeddingResult in tests; use MagicMock for create_embeddings_batch return.
- In python/tests/test_rag_strategies.py at both occurrences (around lines 21–23 and 62–64), remove
and instead build a
from src.server.services.embeddings.embedding_service import EmbeddingResult mock_batch.return_value = EmbeddingResult(...)
MagicMock:Repeat for both patch blocks to fully eliminate EmbeddingResult imports.embedding_batch_result = MagicMock() embedding_batch_result.embeddings = [[0.1] * 1536] embedding_batch_result.texts_processed = ["test"] embedding_batch_result.success_count = 1 embedding_batch_result.failure_count = 0 embedding_batch_result.failed_items = [] embedding_batch_result.has_failures.return_value = False mock_batch.return_value = embedding_batch_result
🤖 Prompt for AI Agents
In python/tests/test_rag_strategies.py around lines 14 to 31 (and likewise for
the second occurrence around lines ~62 to 64), stop importing EmbeddingResult in
the tests and instead return a MagicMock from create_embeddings_batch: remove
the from src.server.services.embeddings.embedding_service import EmbeddingResult
and the EmbeddingResult(...) instantiation, create a MagicMock instance, set its
attributes embeddings, texts_processed, success_count, failure_count and
failed_items to the same values, set has_failures to a callable that returns
False, and assign that MagicMock to mock_batch.return_value for both patch
blocks.
- Change expected api_key_prefix from "sk-1234" to "sk-1…" - Align test expectations with EmbeddingAuthenticationError masking behavior - API keys are automatically masked to first 4 chars + ellipsis for security 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
… mock - Remove create_embedding from autouse fixture to prevent interference - Allow individual tests to properly mock embedding failures - Keep create_embeddings_batch mock for safety - Fix test_error_handling_in_rag_pipeline to properly test fail-fast behavior 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
python/tests/test_openai_quota_error_handling.py (1)
229-236: Verify sanitizer implementation matches tests (import and patterns).Earlier feedback flagged a stub; current tests assume fully implemented sanitizer. Ensure
knowledge_api.pyimportsreand that patterns/behavior match expectations.Run:
#!/bin/bash # Confirm `re` is imported where `_sanitize_openai_error` lives rg -nP '^\s*import\s+re\b' python/src/server/api_routes/knowledge_api.py -n # Inspect sanitizer definition for pattern coverage rg -nP '(?s)^def\s+_sanitize_openai_error\(' python/src/server/api_routes/knowledge_api.py -n -C5 # Ensure only one EmbeddingAuthenticationError class exists (avoid duplicate defs) rg -nP '^class\s+EmbeddingAuthenticationError\b' python/src/server/services/embeddings/embedding_exceptions.py -n -C2
🧹 Nitpick comments (7)
python/tests/test_openai_quota_error_handling.py (7)
11-15: Remove unused import (will trip Ruff F401).
TestClientisn’t used in this module.Apply this diff:
-from fastapi.testclient import TestClient
50-70: Avoid brittle equality on masked key (Unicode ellipsis).Asserting exact
"sk-1…"can be flaky across locales. Prefer prefix + ellipsis presence.Apply this diff:
- assert exc_info.value.api_key_prefix == "sk-1…" + assert exc_info.value.api_key_prefix.startswith("sk-1") + assert "…" in exc_info.value.api_key_prefix
120-145: Consider asserting Retry-After header in addition to JSON field.The API sets
retry_afterin the body; adding an HTTPRetry-Afterheader would help clients. No test change required if you don’t add it, but worth considering upstream.
229-246: Sanitizer test coverage is thorough; consider parametrization to reduce duplication.You can parametrize inputs/expectations for URL/keys/org/proj/req/bearer to shrink boilerplate and ease future additions.
Also applies to: 247-254, 255-262, 263-270, 271-297, 363-415, 442-458
281-284: Wrap long literal to satisfy 120-char line limit.Improves readability and avoids Ruff E501.
Apply this diff:
- mock_service.perform_rag_query = AsyncMock( - side_effect=EmbeddingAPIError("Request failed to https://api.openai.com/v1/embeddings with key sk-FAKE00000000000000000000000000000000000000000000") - ) + api_err_msg = ( + "Request failed to https://api.openai.com/v1/embeddings " + "with key sk-FAKE00000000000000000000000000000000000000000000" + ) + mock_service.perform_rag_query = AsyncMock( + side_effect=EmbeddingAPIError(api_err_msg) + )
392-397: Wrap long bearer-token literal to satisfy 120-char limit.Apply this diff:
- error_message = "Authorization failed: Bearer sk-FAKE00000000000000000000000000000000000000000000 invalid" + error_message = ( + "Authorization failed: " + "Bearer sk-FAKE00000000000000000000000000000000000000000000 invalid" + )
401-403: Wrap long multi-pattern literal to satisfy 120-char limit.Apply this diff:
- error_message = "Request req_abc123 to https://api.openai.com failed for org-FAKE00000000000000000000 with key sk-FAKE00000000000000000000000000000000000000000000" + error_message = ( + "Request req_abc123 to https://api.openai.com failed for " + "org-FAKE00000000000000000000 with key " + "sk-FAKE00000000000000000000000000000000000000000000" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
python/tests/test_openai_quota_error_handling.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
python/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
python/**/*.py: Target Python 3.12 with a 120-character line length
Use Ruff for linting and Mypy for type checking before commit
Files:
python/tests/test_openai_quota_error_handling.py
{python/**/*.py,archon-ui-main/src/**/*.{ts,tsx,js,jsx}}
📄 CodeRabbit inference engine (CLAUDE.md)
{python/**/*.py,archon-ui-main/src/**/*.{ts,tsx,js,jsx}}: Remove dead code immediately; do not keep legacy/unused functions
Avoid comments that reference change history (e.g., LEGACY, CHANGED, REMOVED); keep comments focused on current functionality
Files:
python/tests/test_openai_quota_error_handling.py
python/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Place backend tests under python/tests/
Files:
python/tests/test_openai_quota_error_handling.py
🧬 Code graph analysis (1)
python/tests/test_openai_quota_error_handling.py (3)
python/src/server/services/embeddings/embedding_exceptions.py (5)
EmbeddingAuthenticationError(75-87)EmbeddingAuthenticationError(101-114)EmbeddingQuotaExhaustedError(46-58)EmbeddingRateLimitError(61-72)EmbeddingAPIError(117-130)python/src/server/services/search/rag_service.py (3)
RAGService(37-404)perform_rag_query(194-297)search_documents(93-165)python/src/server/api_routes/knowledge_api.py (3)
perform_rag_query(816-910)RagQueryRequest(171-174)_sanitize_openai_error(61-96)
🔇 Additional comments (3)
python/tests/test_openai_quota_error_handling.py (3)
29-49: Quota-exhausted propagation path looks correct.Good use of
side_effectand explicit assertions on message andtokens_used.
71-90: Rate-limit propagation path looks correct.
91-200: API endpoint error mapping: solid coverage.401/429/502/500 branches and payload fields are validated end-to-end. Nicely scoped mocks.
- Replace problematic create_embedding mock with direct search_documents mock - Ensure test can properly simulate embedding failures - Remove module-level mocking that was interfering with test-specific mocks - Fix test_error_handling_in_rag_pipeline to properly test fail-fast behavior 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Critical fixes: - Remove duplicate EmbeddingAuthenticationError class definition - Merge masking logic into single class implementation Warning fixes: - Simplify error parsing with WeakSet for cycle detection - Add proper error type validation with VALID_ERROR_TYPES - Standardize authentication error types to 'authentication_failed' - Add comprehensive input validation to ErrorAlert component - Extract magic numbers to named constants (MAX_OBJECT_KEYS, MAX_RECURSION_DEPTH) - Fix ESLint hasOwnProperty usage issue 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (6)
python/src/server/api_routes/knowledge_api.py (6)
617-619: Parse tags safely and return 422 on bad JSONPrevents 500s on client mistakes. This was requested previously.
- # Parse tags - tag_list = json.loads(tags) if tags else [] + # Parse tags + try: + tag_list = json.loads(tags) if tags else [] + if tag_list is not None and not isinstance(tag_list, list): + raise ValueError("Tags must be a JSON array") + except (json.JSONDecodeError, ValueError): + raise HTTPException(status_code=422, detail={"error": "Invalid tags format; expected JSON array"})
832-840: Map RAG failure payloads to 401/429/502 instead of blanket 500Aligns with PR goal to surface auth/quota/rate-limit clearly. Previously requested.
- else: - raise HTTPException( - status_code=500, detail={"error": result.get("error", "RAG query failed")} - ) + else: + err = str(result.get("error", "")) + err_type = str(result.get("error_type", "")) + if "EmbeddingAuthenticationError" in err_type: + raise HTTPException(status_code=401, detail={ + "error": "OpenAI API authentication failed", + "message": "Invalid or expired OpenAI API key. Please check your API key in settings.", + "error_type": "authentication_failed", + }) + if "EmbeddingQuotaExhaustedError" in err_type or "quota" in err.lower(): + raise HTTPException(status_code=429, detail={ + "error": "OpenAI API quota exhausted", + "message": "Your OpenAI API key has no remaining credits. Please add credits.", + "error_type": "quota_exhausted", + }) + if "EmbeddingRateLimitError" in err_type or "rate limit" in err.lower(): + raise HTTPException(status_code=429, detail={ + "error": "OpenAI API rate limit exceeded", + "message": "Too many requests. Please wait and try again.", + "error_type": "rate_limit", + }) + if "EmbeddingAPIError" in err_type: + sanitized = _sanitize_openai_error(err) + raise HTTPException(status_code=502, detail={ + "error": "OpenAI API error", + "message": f"OpenAI API error: {sanitized}", + "error_type": "api_error", + }) + raise HTTPException(status_code=500, detail={"error": result.get("error", "RAG query failed")})
892-904: Log sanitized error for EmbeddingAPIError (avoid leaking secrets)Sanitize before logging and re-use sanitized text in response. Previously requested.
- elif isinstance(e, EmbeddingAPIError): - safe_logfire_error( - f"OpenAI API error during RAG query | error={str(e)} | query={request.query[:50]} | source={request.source}" - ) - sanitized_message = _sanitize_openai_error(str(e)) + elif isinstance(e, EmbeddingAPIError): + sanitized_message = _sanitize_openai_error(str(e)) + safe_logfire_error( + f"OpenAI API error during RAG query | error={sanitized_message} | query={request.query[:50]} | source={request.source}" + ) raise HTTPException( status_code=502, detail={ "error": "OpenAI API error", "message": f"OpenAI API error: {sanitized_message}", "error_type": "api_error", } )
905-911: Sanitize generic RAG error logs and payloadsPrevents accidental leakage.
- else: - # Generic error handling for other exceptions - safe_logfire_error( - f"RAG query failed | error={str(e)} | query={request.query[:50]} | source={request.source}" - ) - raise HTTPException(status_code=500, detail={"error": f"RAG query failed: {str(e)}"}) + else: + # Generic error handling for other exceptions + sanitized_message = _sanitize_openai_error(str(e)) + safe_logfire_error( + f"RAG query failed | error={sanitized_message} | query={request.query[:50]} | source={request.source}" + ) + raise HTTPException(status_code=500, detail={"error": f"RAG query failed: {sanitized_message}"})
57-61: Add TTL cache for API key preflight to cut costs and rate spikesMemoize successful validation for a short TTL.
@@ -# Track active async crawl tasks for cancellation support -active_crawl_tasks: dict[str, asyncio.Task] = {} +# Track active async crawl tasks for cancellation support +active_crawl_tasks: dict[str, asyncio.Task] = {} +# TTL cache for API key validation to reduce paid calls/rate spikes +_last_key_validation_ts: float = 0.0 +_KEY_VALIDATION_TTL_SEC = 600 # 10 minutes
98-139: Preflight: use TTL, include masked prefix in 401, and sanitize unexpected error logsReduces spend, improves UX consistency, avoids leaking secrets.
@@ - try: - # Test the API key with a minimal embedding request + try: + # Test the API key with a minimal embedding request from ..services.embeddings.embedding_service import create_embedding from ..services.embeddings.embedding_exceptions import ( EmbeddingAuthenticationError, EmbeddingQuotaExhaustedError, ) - - # Try to create a test embedding with minimal content - await create_embedding(text="test") - + + # TTL cache to avoid repeated paid calls + global _last_key_validation_ts + now = time.monotonic() + if now - _last_key_validation_ts < _KEY_VALIDATION_TTL_SEC: + return + + # Try to create a test embedding with minimal content + await create_embedding(text="test") + _last_key_validation_ts = now @@ - "error_type": "authentication_failed" + "error_type": "authentication_failed", + "api_key_prefix": getattr(e, "api_key_prefix", None), } ) @@ - except Exception as e: + except Exception as e: # For any other errors, allow the crawl to continue # The error will be caught later during actual processing - logger.warning(f"API key validation failed with unexpected error: {e}") + sanitized = _sanitize_openai_error(str(e)) + logger.warning(f"API key validation failed with unexpected error: {sanitized}") pass
🧹 Nitpick comments (6)
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts (4)
7-9: Avoid change-history references in commentsPrefer functionality-focused comments. Remove the GitHub issue reference to align with repo guidelines.
Apply this diff:
- * Related to GitHub issue #362 - improves user experience - * by displaying clear error messages when OpenAI API fails. + * Displays clear error messages when OpenAI API fails.
57-60: Harden object-safety guard for large arraysGuard against huge arrays to avoid heavy traversal and potential UI stalls.
Apply this diff:
const MAX_OBJECT_KEYS = 100; const MAX_RECURSION_DEPTH = 10; +const MAX_ARRAY_LENGTH = 1000; @@ function isSafeObject(obj: any): boolean { if (typeof obj !== 'object' || obj === null) return true; + if (Array.isArray(obj) && obj.length > MAX_ARRAY_LENGTH) return false; @@ function hasCycles(value: any, depth = 0): boolean { if (depth > MAX_RECURSION_DEPTH) return true; // Prevent deep recursion if (typeof value !== 'object' || value === null) return false; + if (Array.isArray(value) && value.length > MAX_ARRAY_LENGTH) return true;Also applies to: 64-75, 73-91
214-229: Map HTTP 402 Payment Required to billing guidance and severityImproves UX when servers use 402 for exhausted credits.
Apply this diff:
@@ if (error.statusCode) { switch (error.statusCode) { + case 402: + return '402 Payment Required - Your OpenAI account has no active credits. Add credits in your OpenAI billing settings.'; case 401: return '401 Unauthorized - Invalid or missing OpenAI credentials. Verify your API key in Settings.'; @@ export function getErrorSeverity(error: EnhancedError): 'error' | 'warning' | 'info' { @@ - if (error.statusCode === 401 || error.statusCode === 403) return 'error'; + if (error.statusCode === 401 || error.statusCode === 402 || error.statusCode === 403) return 'error'; if (error.statusCode === 429) return 'warning'; @@ if (error.statusCode) { switch (error.statusCode) { + case 402: + return 'Check your OpenAI billing dashboard and add credits'; case 401: return 'Go to Settings and verify your OpenAI API key';Also applies to: 251-257, 283-295
237-257: Return type includes 'info' but never returnedEither use 'info' for benign cases or narrow the type to 'error' | 'warning' to reflect reality.
Possible minimal change:
-export function getErrorSeverity(error: EnhancedError): 'error' | 'warning' | 'info' { +export function getErrorSeverity(error: EnhancedError): 'error' | 'warning' {python/src/server/api_routes/knowledge_api.py (2)
145-149: Avoid mutable default for Pydantic fieldUse None and coerce later.
- tags: list[str] = [] + tags: list[str] | None = None
620-627: Protect upload path with file size limitAvoids excessive memory/CPU on huge files.
- # Read file content immediately to avoid closed file issues - file_content = await file.read() + # Read file content immediately to avoid closed file issues + file_content = await file.read() + if len(file_content) > MAX_UPLOAD_BYTES: + raise HTTPException(status_code=413, detail={"error": f"File too large; max {MAX_UPLOAD_BYTES} bytes"}) file_metadata = {Add near constants:
MAX_UPLOAD_BYTES = 10 * 1024 * 1024 # 10MB
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
archon-ui-main/src/components/ui/ErrorAlert.tsx(1 hunks)archon-ui-main/src/services/knowledgeBaseErrorHandler.ts(1 hunks)python/src/server/api_routes/knowledge_api.py(1 hunks)python/src/server/services/embeddings/embedding_exceptions.py(1 hunks)python/tests/test_rag_strategies.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- python/tests/test_rag_strategies.py
- archon-ui-main/src/components/ui/ErrorAlert.tsx
- python/src/server/services/embeddings/embedding_exceptions.py
🧰 Additional context used
📓 Path-based instructions (6)
python/src/{server,mcp,agents}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
python/src/{server,mcp,agents}/**/*.py: Fail fast on service startup failures, missing configuration, database connection issues, auth failures, critical dependency outages, and invalid data that would corrupt state
External API calls should use retry with exponential backoff and ultimately fail with a clear, contextual error message
Error messages must include context (operation being attempted) and relevant IDs/URLs/data for debugging
Preserve full stack traces in logs (e.g., Python logging with exc_info=True)
Use specific exception types; avoid catching broad Exception unless re-raising with context
Never signal failure by returning None/null; raise a descriptive exception instead
Files:
python/src/server/api_routes/knowledge_api.py
python/src/server/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
python/src/server/**/*.py: For batch processing and background tasks, continue processing but log detailed per-item failures and return both successes and failures
Do not crash the server on a single WebSocket event failure; log the error and continue serving other clients
Files:
python/src/server/api_routes/knowledge_api.py
python/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
python/**/*.py: Target Python 3.12 with a 120-character line length
Use Ruff for linting and Mypy for type checking before commit
Files:
python/src/server/api_routes/knowledge_api.py
{python/**/*.py,archon-ui-main/src/**/*.{ts,tsx,js,jsx}}
📄 CodeRabbit inference engine (CLAUDE.md)
{python/**/*.py,archon-ui-main/src/**/*.{ts,tsx,js,jsx}}: Remove dead code immediately; do not keep legacy/unused functions
Avoid comments that reference change history (e.g., LEGACY, CHANGED, REMOVED); keep comments focused on current functionality
Files:
python/src/server/api_routes/knowledge_api.pyarchon-ui-main/src/services/knowledgeBaseErrorHandler.ts
python/src/server/**
📄 CodeRabbit inference engine (CLAUDE.md)
Keep FastAPI application code under python/src/server/ (routes in api_routes/, services in services/, main in main.py)
Files:
python/src/server/api_routes/knowledge_api.py
archon-ui-main/src/services/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place API communication and business logic in archon-ui-main/src/services/
Files:
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts
🧬 Code graph analysis (1)
python/src/server/api_routes/knowledge_api.py (6)
python/src/server/services/storage/storage_services.py (2)
DocumentStorageService(19-284)upload_document(22-193)python/src/server/services/search/rag_service.py (4)
RAGService(37-404)perform_rag_query(194-297)search_code_examples(167-192)search_code_examples_service(299-404)python/src/server/services/embeddings/embedding_exceptions.py (4)
EmbeddingAuthenticationError(75-88)EmbeddingQuotaExhaustedError(46-58)EmbeddingAPIError(102-115)EmbeddingRateLimitError(61-72)python/src/server/services/embeddings/embedding_service.py (1)
create_embedding(72-132)python/src/server/services/source_management_service.py (3)
SourceManagementService(395-718)delete_source(430-498)get_available_sources(402-428)python/src/server/services/crawling/crawling_service.py (5)
set_progress_id(112-116)orchestrate_crawl(248-277)get_active_orchestration(56-58)unregister_orchestration(66-69)cancel(118-121)
🔇 Additional comments (2)
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts (1)
270-273: Nice fix: block scope + nullish coalescing for retryAfterResolves noSwitchDeclarations and preserves 0s. LGTM.
python/src/server/api_routes/knowledge_api.py (1)
202-209: Temporary stub returning [] — confirm this is intended pre-mergeReturning an empty list hides real sources and may surprise clients.
Do you want this stub in main, or should we wire it to the DB service now?
| def _sanitize_openai_error(error_message: str) -> str: | ||
| """Sanitize OpenAI API error messages to prevent information disclosure.""" | ||
| # Input validation | ||
| if not isinstance(error_message, str): | ||
| return "OpenAI API encountered an error. Please verify your API key and quota." | ||
| if not error_message.strip(): | ||
| return "OpenAI API encountered an error. Please verify your API key and quota." | ||
|
|
||
| # Common patterns to sanitize | ||
| sanitized_patterns = { | ||
| r'https?://[^\s]+': '[REDACTED_URL]', # Remove URLs | ||
| r'sk-[a-zA-Z0-9]{48}': '[REDACTED_KEY]', # Remove API keys (OpenAI format) | ||
| r'"[^"]*auth[^"]*"': '[REDACTED_AUTH]', # Remove auth details | ||
| r'org-[a-zA-Z0-9]{24}': '[REDACTED_ORG]', # Remove OpenAI organization IDs | ||
| r'proj_[a-zA-Z0-9]{10,}': '[REDACTED_PROJ]', # Remove OpenAI project IDs (adjusted length) | ||
| r'req_[a-zA-Z0-9]{6,}': '[REDACTED_REQ]', # Remove OpenAI request IDs (adjusted length) | ||
| r'user-[a-zA-Z0-9]{10,}': '[REDACTED_USER]', # Remove OpenAI user IDs | ||
| r'sess_[a-zA-Z0-9]{10,}': '[REDACTED_SESS]', # Remove session IDs | ||
| r'Bearer\s+[^\s]{1,200}': 'Bearer [REDACTED_AUTH_TOKEN]', # Remove bearer tokens (bounded to prevent ReDoS) | ||
| } | ||
|
|
||
| sanitized = error_message | ||
| for pattern, replacement in sanitized_patterns.items(): | ||
| sanitized = re.sub(pattern, replacement, sanitized, flags=re.IGNORECASE) | ||
|
|
||
| # Check for sensitive words after pattern replacement, but exclude our redacted patterns | ||
| sensitive_words = ['internal', 'server', 'token'] | ||
| # Only check for 'endpoint' if it's not part of our redacted URL pattern | ||
| if 'endpoint' in sanitized.lower() and '[REDACTED_URL]' not in sanitized: | ||
| sensitive_words.append('endpoint') | ||
|
|
||
| # Return generic message if still contains sensitive info | ||
| if any(word in sanitized.lower() for word in sensitive_words): | ||
| return "OpenAI API encountered an error. Please verify your API key and quota." | ||
|
|
||
| return sanitized |
There was a problem hiding this comment.
Sanitizer is too narrow; may leak secrets and over-genericize benign errors
Broaden key/org/project patterns, add hard fails for bearer tokens, and always log/return sanitized text only.
Apply:
@@
-def _sanitize_openai_error(error_message: str) -> str:
- """Sanitize OpenAI API error messages to prevent information disclosure."""
- # Input validation
- if not isinstance(error_message, str):
- return "OpenAI API encountered an error. Please verify your API key and quota."
- if not error_message.strip():
- return "OpenAI API encountered an error. Please verify your API key and quota."
-
- # Common patterns to sanitize
- sanitized_patterns = {
- r'https?://[^\s]+': '[REDACTED_URL]', # Remove URLs
- r'sk-[a-zA-Z0-9]{48}': '[REDACTED_KEY]', # Remove API keys (OpenAI format)
- r'"[^"]*auth[^"]*"': '[REDACTED_AUTH]', # Remove auth details
- r'org-[a-zA-Z0-9]{24}': '[REDACTED_ORG]', # Remove OpenAI organization IDs
- r'proj_[a-zA-Z0-9]{10,}': '[REDACTED_PROJ]', # Remove OpenAI project IDs (adjusted length)
- r'req_[a-zA-Z0-9]{6,}': '[REDACTED_REQ]', # Remove OpenAI request IDs (adjusted length)
- r'user-[a-zA-Z0-9]{10,}': '[REDACTED_USER]', # Remove OpenAI user IDs
- r'sess_[a-zA-Z0-9]{10,}': '[REDACTED_SESS]', # Remove session IDs
- r'Bearer\s+[^\s]{1,200}': 'Bearer [REDACTED_AUTH_TOKEN]', # Remove bearer tokens (bounded to prevent ReDoS)
- }
-
- sanitized = error_message
- for pattern, replacement in sanitized_patterns.items():
- sanitized = re.sub(pattern, replacement, sanitized, flags=re.IGNORECASE)
-
- # Check for sensitive words after pattern replacement, but exclude our redacted patterns
- sensitive_words = ['internal', 'server', 'token']
- # Only check for 'endpoint' if it's not part of our redacted URL pattern
- if 'endpoint' in sanitized.lower() and '[REDACTED_URL]' not in sanitized:
- sensitive_words.append('endpoint')
-
- # Return generic message if still contains sensitive info
- if any(word in sanitized.lower() for word in sensitive_words):
- return "OpenAI API encountered an error. Please verify your API key and quota."
-
- return sanitized
+def _sanitize_openai_error(error_message: str) -> str:
+ """Sanitize OpenAI/OpenAI-compatible error messages without leaking secrets."""
+ GENERIC = "OpenAI API encountered an error. Please verify your API key and quota."
+ try:
+ if not isinstance(error_message, str) or not error_message.strip():
+ return GENERIC
+ msg = error_message
+ lower = msg.lower()
+
+ # Hard generic for highly sensitive markers
+ if "bearer " in lower or "auth_bearer" in lower:
+ return GENERIC
+
+ # Redactions (bounded, low-risk patterns)
+ patterns = (
+ (r"https?://[^\s\"']+", "[REDACTED_URL]"),
+ (r"\bsk-[A-Za-z0-9]{10,}\b", "[REDACTED_KEY]"),
+ (r"\borg-[A-Za-z0-9\-]{10,}\b", "[REDACTED_ORG]"),
+ (r"\bproj_[A-Za-z0-9]{8,}\b", "[REDACTED_PROJ]"),
+ (r"\breq_[A-Za-z0-9]{8,}\b", "[REDACTED_REQ]"),
+ (r"auth_bearer_[^\s\"')]+", "[REDACTED_AUTH]"),
+ (r"Bearer\s+[^\s]{1,200}", "Bearer [REDACTED_AUTH_TOKEN]"),
+ )
+ sanitized = msg
+ for pat, repl in patterns:
+ sanitized = re.sub(pat, repl, sanitized, flags=re.IGNORECASE)
+ return sanitized
+ except Exception:
+ return GENERIC📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _sanitize_openai_error(error_message: str) -> str: | |
| """Sanitize OpenAI API error messages to prevent information disclosure.""" | |
| # Input validation | |
| if not isinstance(error_message, str): | |
| return "OpenAI API encountered an error. Please verify your API key and quota." | |
| if not error_message.strip(): | |
| return "OpenAI API encountered an error. Please verify your API key and quota." | |
| # Common patterns to sanitize | |
| sanitized_patterns = { | |
| r'https?://[^\s]+': '[REDACTED_URL]', # Remove URLs | |
| r'sk-[a-zA-Z0-9]{48}': '[REDACTED_KEY]', # Remove API keys (OpenAI format) | |
| r'"[^"]*auth[^"]*"': '[REDACTED_AUTH]', # Remove auth details | |
| r'org-[a-zA-Z0-9]{24}': '[REDACTED_ORG]', # Remove OpenAI organization IDs | |
| r'proj_[a-zA-Z0-9]{10,}': '[REDACTED_PROJ]', # Remove OpenAI project IDs (adjusted length) | |
| r'req_[a-zA-Z0-9]{6,}': '[REDACTED_REQ]', # Remove OpenAI request IDs (adjusted length) | |
| r'user-[a-zA-Z0-9]{10,}': '[REDACTED_USER]', # Remove OpenAI user IDs | |
| r'sess_[a-zA-Z0-9]{10,}': '[REDACTED_SESS]', # Remove session IDs | |
| r'Bearer\s+[^\s]{1,200}': 'Bearer [REDACTED_AUTH_TOKEN]', # Remove bearer tokens (bounded to prevent ReDoS) | |
| } | |
| sanitized = error_message | |
| for pattern, replacement in sanitized_patterns.items(): | |
| sanitized = re.sub(pattern, replacement, sanitized, flags=re.IGNORECASE) | |
| # Check for sensitive words after pattern replacement, but exclude our redacted patterns | |
| sensitive_words = ['internal', 'server', 'token'] | |
| # Only check for 'endpoint' if it's not part of our redacted URL pattern | |
| if 'endpoint' in sanitized.lower() and '[REDACTED_URL]' not in sanitized: | |
| sensitive_words.append('endpoint') | |
| # Return generic message if still contains sensitive info | |
| if any(word in sanitized.lower() for word in sensitive_words): | |
| return "OpenAI API encountered an error. Please verify your API key and quota." | |
| return sanitized | |
| def _sanitize_openai_error(error_message: str) -> str: | |
| """Sanitize OpenAI/OpenAI-compatible error messages without leaking secrets.""" | |
| GENERIC = "OpenAI API encountered an error. Please verify your API key and quota." | |
| try: | |
| if not isinstance(error_message, str) or not error_message.strip(): | |
| return GENERIC | |
| msg = error_message | |
| lower = msg.lower() | |
| # Hard generic for highly sensitive markers | |
| if "bearer " in lower or "auth_bearer" in lower: | |
| return GENERIC | |
| # Redactions (bounded, low-risk patterns) | |
| patterns = ( | |
| (r"https?://[^\s\"']+", "[REDACTED_URL]"), | |
| (r"\bsk-[A-Za-z0-9]{10,}\b", "[REDACTED_KEY]"), | |
| (r"\borg-[A-Za-z0-9\-]{10,}\b", "[REDACTED_ORG]"), | |
| (r"\bproj_[A-Za-z0-9]{8,}\b", "[REDACTED_PROJ]"), | |
| (r"\breq_[A-Za-z0-9]{8,}\b", "[REDACTED_REQ]"), | |
| (r"auth_bearer_[^\s\"')]+", "[REDACTED_AUTH]"), | |
| (r"Bearer\s+[^\s]{1,200}", "Bearer [REDACTED_AUTH_TOKEN]"), | |
| ) | |
| sanitized = msg | |
| for pat, repl in patterns: | |
| sanitized = re.sub(pat, repl, sanitized, flags=re.IGNORECASE) | |
| return sanitized | |
| except Exception: | |
| return GENERIC |
🤖 Prompt for AI Agents
In python/src/server/api_routes/knowledge_api.py around lines 61 to 96, the
current sanitizer uses too-narrow regexes (may miss keys/org/project IDs),
sometimes leaves bearer tokens partially exposed, and can return the original
message when patterns aren't matched; update the sanitizer to (1) broaden and
generalize API key/org/project/request/user/session regexes to cover common
formats and variable lengths while keeping quantifier bounds to avoid ReDoS, (2)
add a strict/priority rule that replaces any Bearer token occurrence with a
fixed redaction and returns the redacted message (never allow bearer tokens
through), (3) include additional generic secret patterns (e.g. hex/UUID-like
tokens, long base64-like strings) and ensure replacements are case-insensitive,
and (4) always return only the sanitized string (never the original) and fall
back to a fixed generic error message if sensitive terms remain after
replacement; ensure unit tests cover edge cases and performance by exercising
long inputs.
| # Validate query | ||
| if not request.query: | ||
| raise HTTPException(status_code=422, detail="Query is required") | ||
|
|
||
| if not request.query.strip(): | ||
| raise HTTPException(status_code=422, detail="Query cannot be empty") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enforce query length limit to mitigate ReDoS/over-embedding
Bound the payload to 5,000 chars.
if not request.query.strip():
raise HTTPException(status_code=422, detail="Query cannot be empty")
+ if len(request.query) > MAX_QUERY_LENGTH:
+ raise HTTPException(status_code=422, detail=f"Query too long; max {MAX_QUERY_LENGTH} characters")Add near constants:
MAX_QUERY_LENGTH = 5000
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Validate query | |
| if not request.query: | |
| raise HTTPException(status_code=422, detail="Query is required") | |
| if not request.query.strip(): | |
| raise HTTPException(status_code=422, detail="Query cannot be empty") | |
| # near the top of the file, alongside other constants | |
| MAX_QUERY_LENGTH = 5000 | |
| # Validate query | |
| if not request.query: | |
| raise HTTPException(status_code=422, detail="Query is required") | |
| if not request.query.strip(): | |
| raise HTTPException(status_code=422, detail="Query cannot be empty") | |
| if len(request.query) > MAX_QUERY_LENGTH: | |
| raise HTTPException( | |
| status_code=422, | |
| detail=f"Query too long; max {MAX_QUERY_LENGTH} characters" | |
| ) |
🤖 Prompt for AI Agents
In python/src/server/api_routes/knowledge_api.py around lines 804 to 810, add a
MAX_QUERY_LENGTH = 5000 constant near the other constants and enforce it after
existing empty checks by validating len(request.query) <= MAX_QUERY_LENGTH; if
it exceeds the limit raise an HTTPException (e.g., status_code=413 or 422) with
a clear message like "Query exceeds maximum length of 5000 characters" so
oversized queries are rejected before further processing.
| if not request.query: | ||
| raise HTTPException(status_code=422, detail="Query is required") | ||
|
|
||
| if not request.query.strip(): | ||
| raise HTTPException(status_code=422, detail="Query cannot be empty") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply same query length guard in /rag/query
Parity across endpoints.
if not request.query.strip():
raise HTTPException(status_code=422, detail="Query cannot be empty")
+ if len(request.query) > MAX_QUERY_LENGTH:
+ raise HTTPException(status_code=422, detail=f"Query too long; max {MAX_QUERY_LENGTH} characters")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not request.query: | |
| raise HTTPException(status_code=422, detail="Query is required") | |
| if not request.query.strip(): | |
| raise HTTPException(status_code=422, detail="Query cannot be empty") | |
| if not request.query: | |
| raise HTTPException(status_code=422, detail="Query is required") | |
| if not request.query.strip(): | |
| raise HTTPException(status_code=422, detail="Query cannot be empty") | |
| if len(request.query) > MAX_QUERY_LENGTH: | |
| raise HTTPException( | |
| status_code=422, | |
| detail=f"Query too long; max {MAX_QUERY_LENGTH} characters", | |
| ) |
🤖 Prompt for AI Agents
In python/src/server/api_routes/knowledge_api.py around lines 819 to 824, the
validation that enforces a non-null, non-empty (after trimming) query is present
for one endpoint but missing in the /rag/query handler; add the same two checks
there: first raise HTTPException(status_code=422, detail="Query is required") if
request.query is falsy, and then raise HTTPException(status_code=422,
detail="Query cannot be empty") if request.query.strip() is empty, ensuring
parity across endpoints.
|
looking good, but we need to hold this until we decide between WS and Polling @coleam00 |
Yeah that makes sense! Now that we're going with polling, you're saying this PR will have to change? |
This merge brings in the latest changes from main including: - HTTP polling architecture (replacing Socket.IO) - TanStack Query migration - Document browser functionality - Latest bug fixes and improvements Our OpenAI error handling features have been preserved and integrated: - Authentication error handling with HTTP 401 responses - API key validation before operations - Error message sanitization for security - Comprehensive error handling for quota/rate limits 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
The parseKnowledgeBaseError import was missing after the merge, causing runtime errors. This restores the enhanced OpenAI error handling functionality. The 401 error shown in the console confirms that our OpenAI API key validation is working correctly - it's properly detecting invalid/missing API keys and returning appropriate error responses. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
This will help diagnose why 401 OpenAI errors are showing as 'Unknown error occurred' instead of the proper error message. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Make auth pattern non-greedy to prevent backtracking attacks. This addresses a code review finding for our new error sanitization. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Replace expensive JSON.stringify() with efficient recursive traversal using WeakSet for circular reference detection and depth limiting. This addresses the performance concern identified in code review. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
python/src/server/services/embeddings/embedding_service.py (1)
159-176: Do not coerce invalid inputs to empty strings; track and fail them explicitlyCoercing to
""leads to meaningless embeddings and violates “skip, don’t corrupt.” Collect invalid items, record failures, and skip them.- # Validate that all items in texts are strings - validated_texts = [] - for i, text in enumerate(texts): + # Validate that all items in texts are strings and within size limits + MAX_EMBED_TEXT_LEN = 5000 + validated_texts: list[str] = [] + invalid_items: list[tuple[int, Any, Exception | None]] = [] + for i, text in enumerate(texts): if not isinstance(text, str): search_logger.error( f"Invalid text type at index {i}: {type(text)}, value: {text}", exc_info=True ) # Try to convert to string try: validated_texts.append(str(text)) except Exception as e: search_logger.error( f"Failed to convert text at index {i} to string: {e}", exc_info=True ) - validated_texts.append("") # Use empty string as fallback + invalid_items.append((i, text, e)) else: - validated_texts.append(text) + if len(text) > MAX_EMBED_TEXT_LEN: + invalid_items.append((i, text, ValueError("text too long"))) + else: + validated_texts.append(text) texts = validated_texts result = EmbeddingBatchResult() + # Record invalid items as failures + for idx, bad, err in invalid_items: + result.add_failure( + str(bad)[:200] if bad is not None else "", + EmbeddingAPIError("Invalid input text", original_error=err), + batch_index=None, + )Also applies to: 179-179
python/src/server/api_routes/knowledge_api.py (2)
888-895: Map service-returned embedding failures to 401/429/502 instead of generic 500When
RAGService.perform_rag_queryreturns(False, result), inspecterror_typeto return precise statuses as per PR goals.- else: - raise HTTPException( - status_code=500, detail={"error": result.get("error", "RAG query failed")} - ) + else: + err = str(result.get("error", "")) + err_type = str(result.get("error_type", "")) + if "EmbeddingAuthenticationError" in err_type: + raise HTTPException(status_code=401, detail={ + "error": "OpenAI API authentication failed", + "message": "Invalid or expired OpenAI API key. Please check your API key in settings.", + "error_type": "authentication_failed", + }) + if "EmbeddingQuotaExhaustedError" in err_type or "quota" in err.lower(): + raise HTTPException(status_code=429, detail={ + "error": "OpenAI API quota exhausted", + "message": "Your OpenAI API key has no remaining credits. Please add credits.", + "error_type": "quota_exhausted", + }) + if "EmbeddingRateLimitError" in err_type or "rate limit" in err.lower(): + raise HTTPException(status_code=429, detail={ + "error": "OpenAI API rate limit exceeded", + "message": "Too many requests. Please wait and try again.", + "error_type": "rate_limit", + }) + if "EmbeddingAPIError" in err_type: + sanitized = _sanitize_openai_error(err) + raise HTTPException(status_code=502, detail={ + "error": "OpenAI API error", + "message": f"OpenAI API error: {sanitized}", + "error_type": "api_error", + }) + raise HTTPException(status_code=500, detail={"error": result.get("error", "RAG query failed")})
861-866: Enforce query length limit (e.g., 5,000 chars) to mitigate ReDoS/over-embeddingAdd a shared constant and enforce in both endpoints.
if not request.query.strip(): raise HTTPException(status_code=422, detail="Query cannot be empty") + MAX_QUERY_LENGTH = 5000 + if len(request.query) > MAX_QUERY_LENGTH: + raise HTTPException(status_code=422, detail=f"Query too long; max {MAX_QUERY_LENGTH} characters")Repeat the same check in
/rag/queryafter empty check.Also applies to: 878-880
♻️ Duplicate comments (2)
python/src/server/api_routes/knowledge_api.py (2)
899-959: Sanitize before logging; sanitize generic error pathLog only sanitized messages to avoid leaking secrets; also sanitize the generic branch.
- elif isinstance(e, EmbeddingAPIError): - safe_logfire_error( - f"OpenAI API error during RAG query | error={str(e)} | query={request.query[:50]} | source={request.source}" - ) - sanitized_message = _sanitize_openai_error(str(e)) + elif isinstance(e, EmbeddingAPIError): + sanitized_message = _sanitize_openai_error(str(e)) + safe_logfire_error( + f"OpenAI API error during RAG query | error={sanitized_message} | query={request.query[:50]} | source={request.source}" + ) raise HTTPException( status_code=502, detail={ "error": "OpenAI API error", "message": f"OpenAI API error: {sanitized_message}", "error_type": "api_error", } ) else: # Generic error handling for other exceptions - safe_logfire_error( - f"RAG query failed | error={str(e)} | query={request.query[:50]} | source={request.source}" - ) - raise HTTPException(status_code=500, detail={"error": f"RAG query failed: {str(e)}"}) + sanitized = _sanitize_openai_error(str(e)) + safe_logfire_error( + f"RAG query failed | error={sanitized} | query={request.query[:50]} | source={request.source}" + ) + raise HTTPException(status_code=500, detail={"error": f"RAG query failed: {sanitized}"})
61-99: Broaden and harden sanitizer; prefer generic message on bearer tokensCurrent patterns miss some formats and logics. Strengthen redactions and default to a generic message for bearer tokens or highly sensitive markers.
-def _sanitize_openai_error(error_message: str) -> str: - """Sanitize OpenAI API error messages to prevent information disclosure.""" - import re - - # Input validation - if not isinstance(error_message, str): - return "OpenAI API encountered an error. Please verify your API key and quota." - if not error_message.strip(): - return "OpenAI API encountered an error. Please verify your API key and quota." - - # Common patterns to sanitize - sanitized_patterns = { - r'https?://[^\s]+': '[REDACTED_URL]', # Remove URLs - r'sk-[a-zA-Z0-9]{48}': '[REDACTED_KEY]', # Remove API keys (OpenAI format) - r'"[^"]*auth[^"]*?"': '[REDACTED_AUTH]', # Remove auth details (non-greedy) - r'org-[a-zA-Z0-9]{24}': '[REDACTED_ORG]', # Remove OpenAI organization IDs - r'proj_[a-zA-Z0-9]{10,}': '[REDACTED_PROJ]', # Remove OpenAI project IDs (adjusted length) - r'req_[a-zA-Z0-9]{6,}': '[REDACTED_REQ]', # Remove OpenAI request IDs (adjusted length) - r'user-[a-zA-Z0-9]{10,}': '[REDACTED_USER]', # Remove OpenAI user IDs - r'sess_[a-zA-Z0-9]{10,}': '[REDACTED_SESS]', # Remove session IDs - r'Bearer\s+[^\s]{1,200}': 'Bearer [REDACTED_AUTH_TOKEN]', # Remove bearer tokens (bounded to prevent ReDoS) - } - - sanitized = error_message - for pattern, replacement in sanitized_patterns.items(): - sanitized = re.sub(pattern, replacement, sanitized, flags=re.IGNORECASE) - - # Check for sensitive words after pattern replacement, but exclude our redacted patterns - sensitive_words = ['internal', 'server', 'token'] - # Only check for 'endpoint' if it's not part of our redacted URL pattern - if 'endpoint' in sanitized.lower() and '[REDACTED_URL]' not in sanitized: - sensitive_words.append('endpoint') - - # Return generic message if still contains sensitive info - if any(word in sanitized.lower() for word in sensitive_words): - return "OpenAI API encountered an error. Please verify your API key and quota." - - return sanitized +def _sanitize_openai_error(error_message: str) -> str: + """Sanitize OpenAI/OpenAI-compatible error messages without leaking secrets.""" + import re + GENERIC = "OpenAI API encountered an error. Please verify your API key and quota." + try: + if not isinstance(error_message, str) or not error_message.strip(): + return GENERIC + msg = error_message + lower = msg.lower() + # Hard generic for bearer tokens or explicit auth markers + if "bearer " in lower or "auth_bearer" in lower: + return GENERIC + patterns = ( + (r"https?://[^\s\"']+", "[REDACTED_URL]"), + (r"\bsk-[A-Za-z0-9]{10,}\b", "[REDACTED_KEY]"), + (r"\borg-[A-Za-z0-9\-]{10,}\b", "[REDACTED_ORG]"), + (r"\bproj_[A-Za-z0-9]{8,}\b", "[REDACTED_PROJ]"), + (r"\breq_[A-Za-z0-9]{8,}\b", "[REDACTED_REQ]"), + (r"\buser-[A-Za-z0-9]{8,}\b", "[REDACTED_USER]"), + (r"\bsess_[A-Za-z0-9]{8,}\b", "[REDACTED_SESS]"), + (r"Bearer\s+[^\s]{1,200}", "Bearer [REDACTED_AUTH_TOKEN]"), + ) + sanitized = msg + for pat, repl in patterns: + sanitized = re.sub(pat, repl, sanitized, flags=re.IGNORECASE) + return sanitized + except Exception: + return GENERIC
🧹 Nitpick comments (9)
polling-analysis.md (2)
96-98: Optional: add reconnect handling and set gcTime to reduce churn.Small resiliency improvements for flaky networks and to avoid cache evictions during brief tab switches.
return useQuery<Task[]>({ queryKey: projectId ? taskKeys.all(projectId) : ["tasks-undefined"], queryFn: async () => { @@ refetchInterval, - refetchOnWindowFocus: true, + refetchOnWindowFocus: true, + refetchOnReconnect: true, staleTime: 5000, // 5s instead of 10s for consistency + gcTime: 5 * 60 * 1000, // keep data cached for quick tab switches });
1-116: Minor copyedits for clarity and consistency.Tighten wording, fix minor grammar/punctuation flagged by tools.
I can push an edited markdown pass if desired.
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts (1)
64-89: Optional: bound array traversal in isSafeObject.Large arrays can be expensive; cap inspected entries.
- // Quick size check to prevent expensive operations on large objects - if (Object.keys(obj).length > MAX_OBJECT_KEYS) return false; + // Quick size check + if (Array.isArray(obj)) { + if (obj.length > MAX_OBJECT_KEYS) return false; + } else if (Object.keys(obj).length > MAX_OBJECT_KEYS) { + return false; + }python/src/server/services/embeddings/embedding_service.py (2)
240-244: Convert OpenAI auth errors and include masked key prefixMapping
openai.AuthenticationError→EmbeddingAuthenticationErroris correct. Consider passing a masked API key prefix to aid troubleshooting without leaking secrets.Apply:
- except openai.AuthenticationError as e: + except openai.AuthenticationError as e: # Invalid API key - critical error, stop everything - search_logger.error("Authentication failed: Invalid API key", exc_info=True) - raise EmbeddingAuthenticationError("Invalid API key") from e + search_logger.error("Authentication failed: Invalid API key", exc_info=True) + raise EmbeddingAuthenticationError( + "Invalid API key", + api_key_prefix=os.getenv("OPENAI_API_KEY") + ) from e
211-219: Minor: avoid shadowingrate_limit_callbackidentifierRename the inner callback to reduce confusion and improve readability.
- rate_limit_callback = None + rate_limit_callback = None if progress_callback: - async def rate_limit_callback(data: dict): + async def on_rate_limit(data: dict): # Send heartbeat during rate limit wait processed = result.success_count + result.failure_count message = f"Rate limited: {data.get('message', 'Waiting...')}" await progress_callback(message, (processed / len(texts)) * 100) ... - async with threading_service.rate_limited_operation(batch_tokens, rate_limit_callback): + async with threading_service.rate_limited_operation(batch_tokens, on_rate_limit):python/src/server/api_routes/knowledge_api.py (4)
58-60: Add TTL cache for API key preflight to reduce cost and rate spikesPreflight embedding on every call is costly. Cache a successful check for ~10 minutes.
active_crawl_tasks: dict[str, asyncio.Task] = {} +_last_key_validation_ts: float = 0.0 +_KEY_VALIDATION_TTL_SEC = 600 # 10 minutes
101-138: Apply TTL cache and sanitize unexpected validation errorsAvoid repeated paid calls and sanitize logs.
async def _validate_openai_api_key() -> None: @@ - try: + try: + # TTL cache to avoid repeated paid calls + global _last_key_validation_ts + now = asyncio.get_running_loop().time() + if now - _last_key_validation_ts < _KEY_VALIDATION_TTL_SEC: + return # Test the API key with a minimal embedding request from ..services.embeddings.embedding_service import create_embedding @@ - await create_embedding(text="test") + await create_embedding(text="test") + _last_key_validation_ts = now @@ - except Exception as e: + except Exception as e: # For any other errors, allow the crawl to continue # The error will be caught later during actual processing - logger.warning(f"API key validation failed with unexpected error: {e}") + sanitized = _sanitize_openai_error(str(e)) + logger.warning(f"API key validation failed with unexpected error: {sanitized}") pass
410-412: Preflight checks are good; TTL cache will prevent extra costKeep these calls but rely on the TTL cache to avoid unnecessary spend and rate-limit pressure.
Also applies to: 524-526, 680-682
931-939: Optional: include Retry-After header for rate limit responsesHelps clients back off correctly.
- raise HTTPException( - status_code=429, - detail={ + raise HTTPException( + status_code=429, + headers={"Retry-After": "30"}, + detail={ "error": "OpenAI API rate limit exceeded", "message": "Too many requests to OpenAI API. Please wait a moment and try again.", "error_type": "rate_limit", "retry_after": 30, # Suggest 30 second wait } )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
archon-ui-main/src/pages/KnowledgeBasePage.tsx(4 hunks)archon-ui-main/src/services/knowledgeBaseErrorHandler.ts(1 hunks)archon-ui-main/src/services/knowledgeBaseService.ts(3 hunks)polling-analysis.md(1 hunks)python/src/server/api_routes/knowledge_api.py(6 hunks)python/src/server/services/credential_service.py(1 hunks)python/src/server/services/embeddings/embedding_exceptions.py(1 hunks)python/src/server/services/embeddings/embedding_service.py(5 hunks)python/src/server/services/threading_service.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- python/src/server/services/credential_service.py
- python/src/server/services/embeddings/embedding_exceptions.py
- python/src/server/services/threading_service.py
- archon-ui-main/src/services/knowledgeBaseService.ts
- archon-ui-main/src/pages/KnowledgeBasePage.tsx
🧰 Additional context used
📓 Path-based instructions (7)
python/src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
python/src/**/*.py: Fail fast on service startup failures, missing configuration, database connection failures, authentication/authorization failures, critical dependencies unavailable, and invalid/corrupting data
Never accept or store corrupted data; on operation failure skip the item entirely rather than writing placeholders (e.g., zero embeddings)
For batch/background operations, continue processing but log each failure with details; track both successes and failures
Use specific exception types (avoid bare Exception), include context/IDs/URLs in messages, preserve full stack traces with logging (exc_info=True), and never return None/null to indicate failure—raise with details
Use database task status values directly: todo, doing, review, done
Target Python 3.12 style with 120-character line length; use Ruff for linting and Mypy for type checking
python/src/**/*.py: Target Python 3.12 and keep lines within 120 characters
Use Ruff for linting (errors, warnings, unused imports, code style) and keep code Ruff-clean
Use Mypy for type checking; maintain type-safe code across backend
Files:
python/src/server/api_routes/knowledge_api.pypython/src/server/services/embeddings/embedding_service.py
python/src/server/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI exception handlers to return rich error responses with appropriate HTTP status codes and typed error payloads
Files:
python/src/server/api_routes/knowledge_api.pypython/src/server/services/embeddings/embedding_service.py
python/src/{server,agents,mcp}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
python/src/{server,agents,mcp}/**/*.py: Fail fast on service startup failures (credentials, DB, or service init) with clear errors
Treat missing configuration (env vars/invalid settings) as fatal and stop execution
Do not suppress database connection failures; bubble up immediately
Authentication/authorization failures must halt the operation and be clearly surfaced
Data corruption or validation errors should raise (use Pydantic to enforce), never silently accept
If a critical dependency is unavailable, fail immediately
Never persist invalid data that would corrupt state (e.g., zero embeddings, null FKs, malformed JSON)
Batch processing should continue, logging detailed errors per item; always return both successes and failures
Background tasks (e.g., embedding generation) should finish queues and log failures without crashing the whole process
Include context and IDs/URLs in error messages; preserve full stack traces with logging (exc_info=True)
Use specific exception types; avoid catching bare Exception
Never return None to signal failure; raise exceptions with details
For crawling/document processing: continue batches, skip failed items entirely, and log detailed errors; never store placeholders (e.g., zeroed embeddings)
Files:
python/src/server/api_routes/knowledge_api.pypython/src/server/services/embeddings/embedding_service.py
archon-ui-main/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use state naming conventions: is[Action]ing for loading, [resource]Error for errors, selected[Resource] for selections
archon-ui-main/src/**/*.{ts,tsx}: WebSocket event failures should be logged and not crash serving other clients
External API calls should retry with exponential backoff and ultimately fail with a clear, specific message
Include actionable context in frontend error logs/messages (what was attempted, relevant IDs/URLs)
Never return null to signal failure in async/data flows; throw errors with details
Use polling (HTTP) with provided hooks (usePolling, useDatabaseMutation, useProjectMutation); Socket.IO is removed
State naming: is[Action]ing for loading, [resource]Error for errors, selected[Resource] for selections
Persist theme choice in localStorage and respect Tailwind dark mode classes across components
Files:
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts
archon-ui-main/src/services/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
archon-ui-main/src/services/**/*.ts: Frontend service method naming: get[Resource]sByProject(projectId), getResource, create/update/delete[Resource]
Use GET /api/projects/{id}/tasks (not getTasks) for project tasks
Files:
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts
archon-ui-main/src/**/*.{ts,tsx,py}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid comment keywords like LEGACY, CHANGED, REMOVED; write comments that document current functionality only
Files:
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts
python/src/server/services/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Backend service method naming mirrors CRUD patterns: get/create/update/delete with clear resource scoping
Files:
python/src/server/services/embeddings/embedding_service.py
🧠 Learnings (10)
📚 Learning: 2025-09-06T20:05:27.061Z
Learnt from: CR
PR: coleam00/Archon#0
File: AGENTS.md:0-0
Timestamp: 2025-09-06T20:05:27.061Z
Learning: Applies to archon-ui-main/src/**/*.{ts,tsx} : Use polling (HTTP) with provided hooks (usePolling, useDatabaseMutation, useProjectMutation); Socket.IO is removed
Applied to files:
polling-analysis.md
📚 Learning: 2025-09-06T20:04:08.111Z
Learnt from: CR
PR: coleam00/Archon#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-06T20:04:08.111Z
Learning: Applies to archon-ui-main/src/features/**/*.{ts,tsx} : Implement HTTP polling patterns: 1–2s for active ops, 5–10s for background, smart pausing on tab inactivity, use ETag caching where applicable
Applied to files:
polling-analysis.md
📚 Learning: 2025-08-28T13:51:59.203Z
Learnt from: Wirasm
PR: coleam00/Archon#514
File: archon-ui-main/src/services/pollingService.ts:60-102
Timestamp: 2025-08-28T13:51:59.203Z
Learning: The pollingService.ts file was removed from the codebase after initially being added in PR #514, confirming the team's decision to consolidate on the usePolling hook pattern rather than maintaining separate polling services.
Applied to files:
polling-analysis.md
📚 Learning: 2025-08-28T12:56:47.840Z
Learnt from: Wirasm
PR: coleam00/Archon#514
File: archon-ui-main/src/pages/ProjectPage.tsx:329-331
Timestamp: 2025-08-28T12:56:47.840Z
Learning: In the ProjectPage.tsx polling refactor, temporary project creation logic with progress cards was removed in favor of simpler modal loading states, as the complexity of managing in-flight temporary projects wasn't justified when HTTP polling would show new projects within seconds anyway.
Applied to files:
polling-analysis.md
📚 Learning: 2025-08-28T13:50:10.499Z
Learnt from: Wirasm
PR: coleam00/Archon#514
File: archon-ui-main/src/services/pollingService.ts:38-45
Timestamp: 2025-08-28T13:50:10.499Z
Learning: The pollingService.ts file is being deprecated in favor of the usePolling hook from usePolling.ts. The team has decided to consolidate on the usePolling hook pattern rather than maintaining separate polling services, as it provides better React integration and reduces code duplication.
Applied to files:
polling-analysis.md
📚 Learning: 2025-08-29T08:57:47.558Z
Learnt from: Wirasm
PR: coleam00/Archon#514
File: archon-ui-main/src/services/progressService.ts:0-0
Timestamp: 2025-08-29T08:57:47.558Z
Learning: The progressService.ts file was removed from the codebase in PR #514 as part of the consolidation effort to use the usePolling hook pattern from usePolling.ts instead of maintaining separate polling services. This aligns with the architectural decision to deprecate individual polling services in favor of the centralized hook-based approach.
Applied to files:
polling-analysis.md
📚 Learning: 2025-09-06T20:04:08.111Z
Learnt from: CR
PR: coleam00/Archon#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-06T20:04:08.111Z
Learning: Applies to archon-ui-main/src/features/**/{hooks,services}/**/*.{ts,tsx} : Use TanStack Query for all data fetching (no prop drilling), with query key factory pattern and smart polling via useSmartPolling
Applied to files:
polling-analysis.md
📚 Learning: 2025-08-28T13:51:59.203Z
Learnt from: Wirasm
PR: coleam00/Archon#514
File: archon-ui-main/src/services/pollingService.ts:60-102
Timestamp: 2025-08-28T13:51:59.203Z
Learning: The pollingService.ts file exists physically but is unused dead code in PR #514. It's only referenced in documentation but not imported anywhere in the TypeScript codebase. The actual polling implementation uses the usePolling hook from usePolling.ts instead.
Applied to files:
polling-analysis.md
📚 Learning: 2025-09-06T20:05:27.061Z
Learnt from: CR
PR: coleam00/Archon#0
File: AGENTS.md:0-0
Timestamp: 2025-09-06T20:05:27.061Z
Learning: Applies to archon-ui-main/src/**/*.{ts,tsx} : Include actionable context in frontend error logs/messages (what was attempted, relevant IDs/URLs)
Applied to files:
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts
📚 Learning: 2025-09-06T20:05:27.061Z
Learnt from: CR
PR: coleam00/Archon#0
File: AGENTS.md:0-0
Timestamp: 2025-09-06T20:05:27.061Z
Learning: Applies to archon-ui-main/src/**/*.{ts,tsx} : External API calls should retry with exponential backoff and ultimately fail with a clear, specific message
Applied to files:
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts
🧬 Code graph analysis (3)
python/src/server/api_routes/knowledge_api.py (3)
python/src/server/services/embeddings/embedding_exceptions.py (4)
EmbeddingAuthenticationError(75-88)EmbeddingQuotaExhaustedError(46-58)EmbeddingRateLimitError(61-72)EmbeddingAPIError(102-115)python/src/server/services/embeddings/embedding_service.py (1)
create_embedding(72-132)python/src/server/config/logfire_config.py (1)
safe_logfire_error(239-251)
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts (1)
python/src/server/utils/progress/progress_tracker.py (1)
error(149-169)
python/src/server/services/embeddings/embedding_service.py (1)
python/src/server/services/embeddings/embedding_exceptions.py (1)
EmbeddingAuthenticationError(75-88)
🪛 LanguageTool
polling-analysis.md
[grammar] ~5-~5: There might be a mistake here.
Context: ... UI Refresh Times ### Problem Statement The UI sometimes takes over 10 seconds t...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...INGS:** #### Multiple Polling Intervals - Tasks: 5-second base interval - **Proj...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...vals - Tasks: 5-second base interval - Projects: 20-second base interval - ...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ... - Projects: 20-second base interval - MCP Status: 5-second interval - **Back...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...al - MCP Status: 5-second interval - Backend Health: 30-second interval ##...
(QB_NEW_EN)
[grammar] ~32-~32: There might be a mistake here.
Context: ...l }; ``` #### Stale Time Configurations - Tasks: 10-second stale time - **Projec...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ...ations - Tasks: 10-second stale time - Projects: 15-second stale time - **MCP...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...ime - Projects: 15-second stale time - MCP Status: 3-second stale time #### ...
(QB_NEW_EN)
[grammar] ~53-~53: There might be a mistake here.
Context: ...ses focus → polling slows from 5s to 60s 2. Stale time conflicts: 5s polling + 10s...
(QB_NEW_EN)
[grammar] ~54-~54: There might be a mistake here.
Context: ... + 10s stale time = potential 15s delays 3. Variable polling rates: Different comp...
(QB_NEW_EN)
[grammar] ~55-~55: There might be a mistake here.
Context: ...nt components update at different speeds 4. ETag cache misses: Rare scenarios requ...
(QB_NEW_EN)
[grammar] ~64-~64: There might be a mistake here.
Context: ...is Makes Sense #### ✅ Technical Support - ETag caching provides 70-90% bandwidth...
(QB_NEW_EN)
[grammar] ~70-~70: There might be a mistake here.
Context: ...ciently #### ✅ User Experience Benefits - Task management is highly interactive (s...
(QB_NEW_EN)
[grammar] ~76-~76: There might be a mistake here.
Context: ...k updates #### ✅ Minimal Technical Cost - Most requests return HTTP 304 (cached) w...
(QB_NEW_EN)
[grammar] ~77-~77: There might be a mistake here.
Context: ... Most requests return HTTP 304 (cached) with existing ETag system - Smart polling pr...
(QB_NEW_EN)
[grammar] ~102-~102: There might be a mistake here.
Context: ...istency }); } ``` ### Expected Impact - Perceived responsiveness: Updates appe...
(QB_NEW_EN)
[grammar] ~113-~113: There might be a mistake here.
Context: ...e 15: Change useSmartPolling(5000) to useSmartPolling(2000) - Line 26: Change staleTime: 10000 to `s...
(QB_NEW_EN)
[grammar] ~116-~116: There might be a mistake here.
Context: ... bandwidth-efficient architecture while providing much better user experience for the mos...
(QB_NEW_EN)
🔇 Additional comments (7)
polling-analysis.md (1)
83-100: LGTM on recommended polling changes; align with hook pattern and naming.useSmartPolling(2000) and staleTime: 5000 fit the guidelines; taskService.getTasksByProject naming is correct.
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts (1)
214-229: Good: explicit 401/403/429/502/503 display mappings.Clear, actionable messages aligned with PR objectives.
python/src/server/services/embeddings/embedding_service.py (4)
111-113: Auth errors correctly bubble to HTTP layerLetting
EmbeddingAuthenticationErrorpropagate ensures 401s are returned. Good.
235-238: Verify response/data length parity
zip(..., strict=False)hides length mismatches betweenbatchandresponse.data. Add a parity check to log and fail missing items.Would you like me to patch in a parity check that adds failures for missing items and logs extras?
Also applies to: 234-236
287-289: Auth errors re-raised immediately during batchCorrect behavior to stop processing and enable 401 mapping.
327-329: Top-level auth bubble-up is correctKeeps semantics consistent across nesting.
python/src/server/api_routes/knowledge_api.py (1)
29-34: Good: import explicit embedding error typesAligns HTTP mapping to error categories.
| 1. **Focus-based throttling**: Window loses focus → polling slows from 5s to 60s | ||
| 2. **Stale time conflicts**: 5s polling + 10s stale time = potential 15s delays | ||
| 3. **Variable polling rates**: Different components update at different speeds | ||
| 4. **ETag cache misses**: Rare scenarios require full refetch cycles | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix TanStack Query staleTime explanation (it doesn't add to refetchInterval).
With refetchInterval set, TanStack Query polls at that interval regardless of staleTime; staleTime mainly gates refetchOnWindowFocus/Reconnect. The “5s polling + 10s stale time = 15s delays” statement is misleading.
Apply this edit to clarify:
-2. **Stale time conflicts**: 5s polling + 10s stale time = potential 15s delays
+2. **Focus/visibility throttling**: When unfocused, polling slows to 60s per useSmartPolling.
+3. **Stale time nuance**: With refetchInterval enabled, polling continues on schedule; staleTime mostly affects focus/reconnect refetch behavior.
-3. **Variable polling rates**: Different components update at different speeds
-4. **ETag cache misses**: Rare scenarios require full refetch cycles
+4. **Variable polling rates**: Different components update at different speeds
+5. **ETag cache misses**: Rare scenarios require full refetch cyclesCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 LanguageTool
[grammar] ~53-~53: There might be a mistake here.
Context: ...ses focus → polling slows from 5s to 60s 2. Stale time conflicts: 5s polling + 10s...
(QB_NEW_EN)
[grammar] ~54-~54: There might be a mistake here.
Context: ... + 10s stale time = potential 15s delays 3. Variable polling rates: Different comp...
(QB_NEW_EN)
[grammar] ~55-~55: There might be a mistake here.
Context: ...nt components update at different speeds 4. ETag cache misses: Rare scenarios requ...
(QB_NEW_EN)
🤖 Prompt for AI Agents
In polling-analysis.md around lines 53 to 57, the explanation that "5s polling +
10s stale time = potential 15s delays" is incorrect; update the text to clarify
that with refetchInterval set, TanStack Query will poll at that interval
regardless of staleTime, and that staleTime only affects whether data is
considered fresh for behaviors like refetchOnWindowFocus or refetchOnReconnect
(and cache staleness/garbage collection), so replace the misleading line with a
sentence stating staleTime does not add to refetchInterval and instead gates
focus/reconnect refetching.
Address CodeRabbit security feedback by: - Adding sanitization patterns for API keys, URLs, org IDs - Implementing defense-in-depth security in frontend error handling - Removing debug logs that could leak PII in production - Adding type-safe status code detection from various error shapes This prevents sensitive information leakage through error messages displayed in the UI, complementing the backend sanitization. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts (1)
179-185: Redact and bound OpenAI errorDetails.messageerrorDetails.message is assigned verbatim; sanitize it to enforce redaction/length limits consistently.
- enhancedError.errorDetails = { + enhancedError.errorDetails = { error: errorData.error || 'unknown', - message: errorData.message || 'Unknown error occurred', + message: sanitizeMessage(typeof errorData.message === 'string' ? errorData.message : 'Unknown error occurred'), error_type: validatedErrorType, tokens_used: typeof errorData.tokens_used === 'number' ? errorData.tokens_used : undefined, retry_after: typeof errorData.retry_after === 'number' ? errorData.retry_after : undefined };
🧹 Nitpick comments (6)
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts (6)
161-168: Capture HTTP status from nested/transport-specific shapes (Axios/fetch/body)Improve robustness so 401/403/429 mapping works regardless of wrapper.
if (typeof error.response?.status === 'number') enhancedError.statusCode = error.response.status; if (typeof error.response?.statusCode === 'number') enhancedError.statusCode = error.response.statusCode; + // Nested payloads (Axios/fetch body wrappers) + if (enhancedError.statusCode == null && typeof error.response?.data?.status === 'number') { + enhancedError.statusCode = error.response.data.status; + } + if (enhancedError.statusCode == null && typeof error.response?.data?.statusCode === 'number') { + enhancedError.statusCode = error.response.data.statusCode; + } + if (enhancedError.statusCode == null && typeof error.error?.status === 'number') { + enhancedError.statusCode = error.error.status; + } + if (enhancedError.statusCode == null && typeof error.error?.statusCode === 'number') { + enhancedError.statusCode = error.error.statusCode; + } + if (enhancedError.statusCode == null && typeof error.detail?.status === 'number') { + enhancedError.statusCode = error.detail.status; + } + if (enhancedError.statusCode == null && typeof error.detail?.statusCode === 'number') { + enhancedError.statusCode = error.detail.statusCode; + }
189-198: Set default HTTP status for known OpenAI error types when missingEnsures consistent severity/UX even if the backend omitted status codes.
switch (validatedErrorType) { case 'authentication_required': case 'authentication_failed': + if (enhancedError.statusCode == null) enhancedError.statusCode = 401; enhancedError.message = '401 Unauthorized - Invalid OpenAI API key. Please verify your OpenAI API key in Settings before starting a crawl.'; break; case 'quota_exhausted': + if (enhancedError.statusCode == null) enhancedError.statusCode = 429; enhancedError.message = 'OpenAI API quota exhausted. Please add credits to your OpenAI account or check your billing settings.'; break; case 'rate_limit': + if (enhancedError.statusCode == null) enhancedError.statusCode = 429; enhancedError.message = 'OpenAI API rate limit exceeded. Please wait a moment and try again.'; break;Also applies to: 193-198
86-90: Harden size check against Proxy traps and gettersObject.keys can throw on hostile Proxy objects. Wrap in try/catch to avoid crashing the parser.
- // Quick size check to prevent expensive operations on large objects - if (Object.keys(obj).length > MAX_OBJECT_KEYS) return false; + // Quick size check to prevent expensive operations on large objects + try { + if (Object.keys(obj).length > MAX_OBJECT_KEYS) return false; + } catch { + // Proxies or throwing traps + return false; + }
62-68: Broaden last‑mile redaction (session tokens, emails)Covers common sensitive shapes that still appear in logs/messages.
const REDACTION_PATTERNS: ReadonlyArray<[RegExp, string]> = [ [/\bsk-[A-Za-z0-9]{10,}\b/g, 'sk-REDACTED'], [/\borg-[A-Za-z0-9]{6,}\b/g, 'org-REDACTED'], [/\bproj_[A-Za-z0-9]{10,}\b/g, 'proj_REDACTED'], + [/\bsess-[A-Za-z0-9]{10,}\b/g, 'sess-REDACTED'], [/\bBearer\s+[A-Za-z0-9._-]{10,}\b/gi, 'Bearer REDACTED'], [/\bhttps?:\/\/[^\s]{1,200}/gi, '[REDACTED_URL]'], + [/[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,}/gi, '[REDACTED_EMAIL]'], ];
189-192: Make the auth message operation‑agnostic“before starting a crawl” is specific; this handler is used across uploads/refresh/retry. Prefer a generic directive.
- enhancedError.message = '401 Unauthorized - Invalid OpenAI API key. Please verify your OpenAI API key in Settings before starting a crawl.'; + enhancedError.message = '401 Unauthorized - Invalid OpenAI API key. Please verify your OpenAI API key in Settings before continuing.';- return `401 Unauthorized - Invalid OpenAI API key. Please verify your OpenAI API key in Settings before starting a crawl.`; + return `401 Unauthorized - Invalid OpenAI API key. Please verify your OpenAI API key in Settings before continuing.`;Also applies to: 224-227
221-261: Add optional unit tests for error‐handling helpers
- Cover
sanitizeMessage(redactions, length cap)- Validate
parseKnowledgeBaseErrorwith Axios/fetch/native Error, string, and HTTP‐body shapes- Assert
getErrorSeverity/getErrorActionmappings for OpenAI error types and HTTP statuses (401, 403, 429, 502, 503, default)I can scaffold these test cases if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts(1 hunks)archon-ui-main/src/services/knowledgeBaseService.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- archon-ui-main/src/services/knowledgeBaseService.ts
🧰 Additional context used
📓 Path-based instructions (3)
archon-ui-main/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use state naming conventions: is[Action]ing for loading, [resource]Error for errors, selected[Resource] for selections
archon-ui-main/src/**/*.{ts,tsx}: WebSocket event failures should be logged and not crash serving other clients
External API calls should retry with exponential backoff and ultimately fail with a clear, specific message
Include actionable context in frontend error logs/messages (what was attempted, relevant IDs/URLs)
Never return null to signal failure in async/data flows; throw errors with details
Use polling (HTTP) with provided hooks (usePolling, useDatabaseMutation, useProjectMutation); Socket.IO is removed
State naming: is[Action]ing for loading, [resource]Error for errors, selected[Resource] for selections
Persist theme choice in localStorage and respect Tailwind dark mode classes across components
Files:
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts
archon-ui-main/src/services/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
archon-ui-main/src/services/**/*.ts: Frontend service method naming: get[Resource]sByProject(projectId), getResource, create/update/delete[Resource]
Use GET /api/projects/{id}/tasks (not getTasks) for project tasks
Files:
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts
archon-ui-main/src/**/*.{ts,tsx,py}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid comment keywords like LEGACY, CHANGED, REMOVED; write comments that document current functionality only
Files:
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts
🧠 Learnings (2)
📚 Learning: 2025-09-06T20:05:27.061Z
Learnt from: CR
PR: coleam00/Archon#0
File: AGENTS.md:0-0
Timestamp: 2025-09-06T20:05:27.061Z
Learning: Applies to archon-ui-main/src/**/*.{ts,tsx} : Include actionable context in frontend error logs/messages (what was attempted, relevant IDs/URLs)
Applied to files:
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts
📚 Learning: 2025-09-06T20:05:27.061Z
Learnt from: CR
PR: coleam00/Archon#0
File: AGENTS.md:0-0
Timestamp: 2025-09-06T20:05:27.061Z
Learning: Applies to archon-ui-main/src/**/*.{ts,tsx} : External API calls should retry with exponential backoff and ultimately fail with a clear, specific message
Applied to files:
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts
🧬 Code graph analysis (1)
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts (1)
python/src/server/utils/progress/progress_tracker.py (1)
error(149-169)
🔇 Additional comments (1)
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts (1)
112-216: Overall: Strong normalization, clear UX, good production‑safe loggingGood alignment with PR goals: consistent 401/429 handling, actionable guidance, and belt‑and‑suspenders redaction. The small tweaks above close remaining leakage/robustness gaps.
Also applies to: 221-326
The string error handling path was creating Error objects directly from unsanitized input, which could leak API keys, tokens, or URLs. Now sanitizes the string before creating both Error.message and errorDetails.message fields, preserving the existing type structure while ensuring no sensitive data can leak through string errors. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts (1)
181-186: Sanitize errorDetails.message at source to prevent secret leakage downstreamThis field may be logged/propagated by other consumers; sanitize before assignment.
- enhancedError.errorDetails = { - error: errorData.error || 'unknown', - message: errorData.message || 'Unknown error occurred', + enhancedError.errorDetails = { + error: errorData.error || 'unknown', + message: sanitizeMessage( + typeof errorData.message === 'string' ? errorData.message : 'Unknown error occurred' + ), error_type: validatedErrorType, tokens_used: typeof errorData.tokens_used === 'number' ? errorData.tokens_used : undefined, retry_after: typeof errorData.retry_after === 'number' ? errorData.retry_after : undefined };
🧹 Nitpick comments (5)
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts (5)
70-77: Cap sanitized message length to 5k to align with PR objective and reduce worst‑case workCurrent cap is 10k. The PR’s hardening goal mentions 5,000 chars; lowering the ceiling also trims replace passes.
- let out = msg.slice(0, 10_000); // cap length + let out = msg.slice(0, 5_000); // cap length per PR hardening goal
245-257: Add explicit mappings for 500/504 to improve user guidanceKeeps messages consistent across common server failures.
switch (error.statusCode) { + case 500: + return 'Internal server error. Please try again shortly.'; case 401: return '401 Unauthorized - Invalid or missing OpenAI credentials. Verify your API key in Settings.'; case 403: return '403 Forbidden - Your OpenAI key/org/project lacks access to this operation.'; case 429: return 'API rate limit exceeded. Please wait a moment and try again.'; case 502: return 'API service unavailable. Please try again in a few minutes.'; case 503: return 'Service temporarily unavailable. Please try again later.'; + case 504: + return 'Gateway timeout. Please retry in a moment.'; default: return sanitizeMessage(error.message); }
112-116: Accept optional context to include actionable identifiers in debug logsHelps meet “include actionable context” guideline without leaking PII (still sanitized).
-export function parseKnowledgeBaseError(error: any): EnhancedError { - if (process.env.NODE_ENV !== 'production') { - const safeMsg = sanitizeMessage((error && (error.message || error?.response?.data?.message)) ?? ''); - console.debug('parseKnowledgeBaseError', { status: error?.status ?? error?.response?.status, message: safeMsg }); - } +export function parseKnowledgeBaseError( + error: any, + context?: { operation?: string; resourceId?: string; url?: string } +): EnhancedError { + if (process.env.NODE_ENV !== 'production') { + const safeMsg = sanitizeMessage((error && (error.message || error?.response?.data?.message)) ?? ''); + console.debug('parseKnowledgeBaseError', { + status: error?.status ?? error?.response?.status, + message: safeMsg, + operation: context?.operation, + resourceId: context?.resourceId, + url: context?.url && sanitizeMessage(context.url), + }); + }
292-327: Provide a non-null fallback action to avoid “no CTA” statesWhen we can’t infer specifics, offer a safe default. Keeps UI consistent with “clear, specific message” guidance.
- default: - return null; + default: + return 'Try again later or contact support if the problem persists'; } } - return null; + return 'Try again later or contact support if the problem persists';
20-36: Make error_type validation case-insensitivePrevents surprises if backend sends capitalized variants.
-function validateErrorType(errorType: any): OpenAIErrorDetails['error_type'] { - if (typeof errorType === 'string' && VALID_ERROR_TYPES.includes(errorType as any)) { - return errorType as OpenAIErrorDetails['error_type']; +function validateErrorType(errorType: unknown): OpenAIErrorDetails['error_type'] { + if (typeof errorType === 'string') { + const lowered = errorType.toLowerCase() as OpenAIErrorDetails['error_type']; + if (VALID_ERROR_TYPES.includes(lowered)) return lowered; } return 'api_error'; // Default fallback }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
archon-ui-main/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use state naming conventions: is[Action]ing for loading, [resource]Error for errors, selected[Resource] for selections
archon-ui-main/src/**/*.{ts,tsx}: WebSocket event failures should be logged and not crash serving other clients
External API calls should retry with exponential backoff and ultimately fail with a clear, specific message
Include actionable context in frontend error logs/messages (what was attempted, relevant IDs/URLs)
Never return null to signal failure in async/data flows; throw errors with details
Use polling (HTTP) with provided hooks (usePolling, useDatabaseMutation, useProjectMutation); Socket.IO is removed
State naming: is[Action]ing for loading, [resource]Error for errors, selected[Resource] for selections
Persist theme choice in localStorage and respect Tailwind dark mode classes across components
Files:
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts
archon-ui-main/src/services/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
archon-ui-main/src/services/**/*.ts: Frontend service method naming: get[Resource]sByProject(projectId), getResource, create/update/delete[Resource]
Use GET /api/projects/{id}/tasks (not getTasks) for project tasks
Files:
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts
archon-ui-main/src/**/*.{ts,tsx,py}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid comment keywords like LEGACY, CHANGED, REMOVED; write comments that document current functionality only
Files:
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts
🧠 Learnings (2)
📚 Learning: 2025-09-06T20:05:27.061Z
Learnt from: CR
PR: coleam00/Archon#0
File: AGENTS.md:0-0
Timestamp: 2025-09-06T20:05:27.061Z
Learning: Applies to archon-ui-main/src/**/*.{ts,tsx} : Include actionable context in frontend error logs/messages (what was attempted, relevant IDs/URLs)
Applied to files:
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts
📚 Learning: 2025-09-06T20:05:27.061Z
Learnt from: CR
PR: coleam00/Archon#0
File: AGENTS.md:0-0
Timestamp: 2025-09-06T20:05:27.061Z
Learning: Applies to archon-ui-main/src/**/*.{ts,tsx} : External API calls should retry with exponential backoff and ultimately fail with a clear, specific message
Applied to files:
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts
🧬 Code graph analysis (1)
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts (1)
python/src/server/utils/progress/progress_tracker.py (1)
error(149-169)
🔇 Additional comments (2)
archon-ui-main/src/services/knowledgeBaseErrorHandler.ts (2)
61-68: Redaction set looks solidBounded patterns, no catastrophic backtracking, and covers keys/tokens/URLs. Nice.
267-287: Severity mapping is correct and aligned with objectivesAuth/quota errors surface as error; rate limits as warning; 5xx as error. Good.
| if (error && typeof error === 'object') { | ||
| // Handle fetch Response errors | ||
| if (typeof error.status === 'number') enhancedError.statusCode = error.status; | ||
| if (typeof error.statusCode === 'number') enhancedError.statusCode = error.statusCode; | ||
| if (typeof error.response?.status === 'number') enhancedError.statusCode = error.response.status; | ||
| if (typeof error.response?.statusCode === 'number') enhancedError.statusCode = error.response.statusCode; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize status codes from multiple shapes, including numeric strings
Prevents missing mappings when status arrives as a string or nested in response.data.
- if (typeof error.status === 'number') enhancedError.statusCode = error.status;
- if (typeof error.statusCode === 'number') enhancedError.statusCode = error.statusCode;
- if (typeof error.response?.status === 'number') enhancedError.statusCode = error.response.status;
- if (typeof error.response?.statusCode === 'number') enhancedError.statusCode = error.response.statusCode;
+ const candidates = [
+ (error as any).status,
+ (error as any).statusCode,
+ (error as any).response?.status,
+ (error as any).response?.statusCode,
+ (error as any).response?.data?.status,
+ (error as any).response?.data?.statusCode,
+ ];
+ for (const s of candidates) {
+ const n = typeof s === 'string' ? parseInt(s, 10) : s;
+ if (Number.isFinite(n)) { enhancedError.statusCode = n as number; break; }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (error && typeof error === 'object') { | |
| // Handle fetch Response errors | |
| if (typeof error.status === 'number') enhancedError.statusCode = error.status; | |
| if (typeof error.statusCode === 'number') enhancedError.statusCode = error.statusCode; | |
| if (typeof error.response?.status === 'number') enhancedError.statusCode = error.response.status; | |
| if (typeof error.response?.statusCode === 'number') enhancedError.statusCode = error.response.statusCode; | |
| if (error && typeof error === 'object') { | |
| // Handle fetch/HTTP/Axios error shapes (including numeric strings and nested data) | |
| const candidates = [ | |
| (error as any).status, | |
| (error as any).statusCode, | |
| (error as any).response?.status, | |
| (error as any).response?.statusCode, | |
| (error as any).response?.data?.status, | |
| (error as any).response?.data?.statusCode, | |
| ]; | |
| for (const s of candidates) { | |
| const n = typeof s === 'string' ? parseInt(s, 10) : s; | |
| if (Number.isFinite(n)) { | |
| enhancedError.statusCode = n as number; | |
| break; | |
| } | |
| } | |
| // …rest of the error-parsing logic continues here |
🤖 Prompt for AI Agents
In archon-ui-main/src/services/knowledgeBaseErrorHandler.ts around lines 163 to
169, the status extraction only checks numeric properties and misses status
codes provided as numeric strings or nested under response.data; update the
checks to also look for status/statusCode on error, error.response, and
error.response.data, and when a value is present as a string attempt to parse it
to an integer and only assign enhancedError.statusCode when the parsed value is
a finite number; ensure you preserve existing precedence (explicit status >
statusCode > response.status > response.statusCode > response.data.status >
response.data.statusCode).
|
@leex279 Would love to see these resolved and merge this pr now that the main part of the migration is done |
|
The knowledge base services would need to be migrated to the new folder structure |
|
Rebased in #650 |
…oleam00#371) * fix(ui): post-merge test fixes - accessibility and async handling Accessibility (WCAG 2.1): - Add proper label associations (htmlFor + id) to all form inputs in ApprovalRulesConfig for proper screen reader support Test Fixes: - Fix ResearchResults clipboard mock to return resolved promise - Wrap async copy operations in act() for proper React testing - All tests now passing: 379/380 (99.7%) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * refactor(ui): consolidate duplicate UI patterns Extracts shared utilities and components to reduce code duplication: - formatTimeAgo: Consolidates 2 implementations (ingestion-queue, SyncStatus) - AlertBanner: Consolidates 3+ error banner patterns - ConfirmDialog: Replaces native confirm() with accessible React component Files created: - pmoves/ui/lib/timeUtils.ts - pmoves/ui/components/common/AlertBanner.tsx - pmoves/ui/components/common/ConfirmDialog.tsx - pmoves/ui/components/common/index.ts Tests updated: - SyncStatus.test.tsx: Updated for new time format behavior - ApprovalRulesConfig.test.tsx: Updated for ConfirmDialog interaction Test results: 380 passed, 1 skipped 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * feat(ui): add Notebook Runtime dashboard page Adds missing /dashboard/notebook/runtime page with: - Service health monitoring for notebook-sync (port 8095) - Prometheus metrics display - Manual sync trigger button - Auto-refresh option (10 seconds) - API routes for runtime status and sync trigger Also fixes gateway-agent environment configuration to use env_file instead of hardcoded environment variables. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(ui): avoid template literals for Turbopack compatibility The ingestion-queue page had template literals that were causing Turbopack to fail parsing. Replaced all template literals with string concatenation or a cn() helper function. Changes: - Added cn() helper function for className concatenation - Replaced all template literals in error messages - Replaced all template literals in JSX className attributes - Replaced CSV escaping backticks with String.fromCharCode() 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(ui): address CodeRabbit PR review comments Fixes accessibility and error handling issues identified in PR coleam00#371: - AlertBanner: Make aria-live conditional (assertive for errors, polite for other variants) - ConfirmDialog: Add WCAG 2.1 compliant focus management - ConfirmDialog: Add Escape key handler for keyboard accessibility - Notebook runtime: Check res.ok before parsing JSON in handleSync - Notebook runtime: Add htmlFor/id to checkbox for label association All changes maintain backward compatibility and improve WCAG 2.1 compliance. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(ui): add defensive Array.isArray checks to prevent map/reduce errors Fixes "e.map is not a function" error by ensuring all state operations properly guard against undefined/null values: - fetchIngestionQueue: Wrap data in Array.isArray before setItems - Realtime callbacks: Check item validity + ensure prev is array - handleExport: Defensive check on items before filter/map Root cause: Supabase realtime may send unexpected payload formats during connection edge cases. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Codex Agent <codex-agent@example.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
4 participants
Fix #362: Complete OpenAI API error handling + Merge PR #521
Summary
Resolves issue #362 where users experienced silent failures and empty results when OpenAI API quota was exhausted or other API errors occurred. This PR now includes all changes from PR #521 which provides the core authentication error handling fixes.
Key Improvements
🛡️ Critical Authentication Error Handling (from PR #521)
create_embedding(query)→create_embedding(text=query)callis not Noneinstead of truthy checks🔧 Enhanced Error Handling
🔒 Security Hardening
⚡ Performance Optimizations
Before vs After
Before (Issue #362)
After (This PR + PR #521)
HTTP 401: "Invalid API key - Please check your API key in settings"Technical Implementation
Changes from PR #521 (Now Merged)
python/src/server/services/search/rag_service.py- Added EmbeddingAuthenticationError handling + single-vector API fixpython/src/server/services/embeddings/embedding_service.py- Enhanced authentication error mappingpython/src/server/api_routes/knowledge_api.py- HTTP 401 error responses with clear messagespython/src/server/services/embeddings/embedding_exceptions.py- EmbeddingAuthenticationError class (already existed)Additional Changes
archon-ui-main/src/services/knowledgeBaseService.ts- Enhanced error parsing and validation cachingarchon-ui-main/src/pages/KnowledgeBasePage.tsx- UI improvements for error handlingTesting
Type of Change
Related Issues & PRs
Breaking Changes
None. This is a non-breaking enhancement that improves error handling without changing existing APIs.
Security Review
The implementation transforms the frustrating silent failure experience into clear, actionable feedback while maintaining robust security and performance standards.
Summary by CodeRabbit
New Features
Improvements
Tests