fix: Better error message if cognee is run without cognee.add and cognee.cognify #1940
Conversation
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on January 6. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
Please make sure all the checkboxes are checked:
|
WalkthroughTwo search-related modules add error handling to catch database and user-related exceptions ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| content={ | ||
| "error": "Search prerequisites not met", | ||
| "detail": str(e), | ||
| "hint": "Run `await cognee.add(...)` then `await cognee.cognify()` before searching.", | ||
| }, |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
In general, to fix this class of problem, the API should never send raw exception messages or stack traces back to the client. Instead, it should (a) log the full exception (with traceback) on the server for debugging/monitoring, and (b) return a generic, user-safe error message or a carefully curated, non-sensitive message. Custom exceptions that are intended for client consumption should provide a sanitized, structured payload rather than relying on str(e).
For this specific code, we can keep the behavior and HTTP status codes but stop exposing raw exception messages. The best minimal fix is:
- Add a logger for this module (using Python’s standard
logginglibrary). - In the
except (DatabaseNotCreatedError, UserNotFoundError, CogneeValidationError) as e:block:- Log the exception with traceback.
- Return a JSON response whose
detailis a generic, non-sensitive message (for example, based on the exception class name) rather thanstr(e).
- In the
except Exception as error:block:- Log the exception with traceback.
- Return a generic 409 response like
{"error": "Search operation failed"}and omitstr(error).
This preserves:
- Distinct handling and status codes for known precondition/validation errors vs generic server-side errors.
- The hint text that is already generic and safe.
- The
PermissionDeniedErrorbehavior (empty list) unchanged.
Concretely, in cognee/api/v1/search/routers/get_search_router.py:
- Add
import loggingat the top and definelogger = logging.getLogger(__name__). - Modify the first exception handler to:
- Call
logger.exception("Search prerequisites not met"). - Replace
"detail": str(e)with a generic message, for instance usinge.__class__.__name__or a static text.
- Call
- Modify the generic exception handler to:
- Call
logger.exception("Unexpected error during search"). - Replace
{"error": str(error)}with a generic error message.
- Call
No new external dependencies are required; we use Python’s built-in logging only.
| @@ -5,6 +5,7 @@ | ||
| from fastapi import Depends, APIRouter | ||
| from fastapi.responses import JSONResponse | ||
| from fastapi.encoders import jsonable_encoder | ||
| import logging | ||
|
|
||
| from cognee.modules.search.types import SearchType, SearchResult, CombinedSearchResult | ||
| from cognee.api.DTO import InDTO, OutDTO | ||
| @@ -17,7 +18,9 @@ | ||
| from cognee.infrastructure.databases.exceptions import DatabaseNotCreatedError | ||
| from cognee.exceptions import CogneeValidationError | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| # Note: Datasets sent by name will only map to datasets owned by the request sender | ||
| # To search for datasets not owned by the request sender dataset UUID is needed | ||
| class SearchPayloadDTO(InDTO): | ||
| @@ -142,18 +144,23 @@ | ||
| return jsonable_encoder(results) | ||
| except (DatabaseNotCreatedError, UserNotFoundError, CogneeValidationError) as e: | ||
| # Return a clear 422 with actionable guidance instead of leaking a stacktrace | ||
| logger.exception("Search prerequisites not met") | ||
| status_code = getattr(e, "status_code", 422) | ||
| return JSONResponse( | ||
| status_code=status_code, | ||
| content={ | ||
| "error": "Search prerequisites not met", | ||
| "detail": str(e), | ||
| "detail": "One or more search prerequisites or validations failed.", | ||
| "hint": "Run `await cognee.add(...)` then `await cognee.cognify()` before searching.", | ||
| }, | ||
| ) | ||
| except PermissionDeniedError: | ||
| return [] | ||
| except Exception as error: | ||
| return JSONResponse(status_code=409, content={"error": str(error)}) | ||
| logger.exception("Unexpected error during search") | ||
| return JSONResponse( | ||
| status_code=409, | ||
| content={"error": "Search operation failed due to an internal error."}, | ||
| ) | ||
|
|
||
| return router |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cognee/api/v1/search/routers/get_search_router.py (2)
143-143: Consider narrowing the exception catch to avoid unrelated validation errors.Catching
CogneeValidationErrorbroadly at the router level could inadvertently handle unrelated validation errors from deeper in the search pipeline (e.g., query validation, parameter validation) that might need different treatment or HTTP status codes.Since
search.pynow raises a specifically namedCogneeValidationErrorwithname="SearchPreconditionError", you could check the exception'snameattribute to distinguish precondition failures from other validation errors.🔎 Optional: filter by exception name
- except CogneeValidationError as e: + except CogneeValidationError as e: + # Only handle precondition errors here; let others propagate + if getattr(e, "name", None) != "SearchPreconditionError": + raise # Return a clear 422 with actionable guidance instead of leaking a stacktrace status_code = getattr(e, "status_code", 422)This ensures only the specific precondition check is handled here, while other validation errors fall through to the general
Exceptionhandler or propagate appropriately.
148-152: Add a comment explaining thatstr(e)is safe for external users.The caught exceptions (
DatabaseNotCreatedError,UserNotFoundError,CogneeValidationError) all define safe, user-friendly default messages and inherit a__str__method that returns only the exception name, message, and status code—no stack traces. However, add a brief comment at line 150 documenting this assumption (e.g., "Safe to expose: CogneeApiError subclasses have user-facing messages") to clarify for future maintainers why usingstr(e)in the response is acceptable here.cognee/api/v1/search/search.py (1)
186-191: Consider simpler formatting for JSON-serialized error messages.The message uses newline characters (
\n) and bullet points (•) which may not render well when serialized to JSON and displayed in API clients, terminals, or log aggregators. A single-line message or one using simple punctuation would be more portable.🔎 Suggested simplification
raise CogneeValidationError( message=( - "Search prerequisites not met: no database/default user found. " - "Initialize Cognee before searching by:\n" - "• running `await cognee.add(...)` followed by `await cognee.cognify()`." + "Search prerequisites not met: no database or default user found. " + "Initialize Cognee before searching by running " + "`await cognee.add(...)` followed by `await cognee.cognify()`." ), name="SearchPreconditionError", ) from errorThis preserves clarity while ensuring the message displays consistently across different interfaces.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
cognee/api/v1/search/routers/get_search_router.pycognee/api/v1/search/search.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use 4-space indentation in Python code
Use snake_case for Python module and function names
Use PascalCase for Python class names
Use ruff format before committing Python code
Use ruff check for import hygiene and style enforcement with line-length 100 configured in pyproject.toml
Prefer explicit, structured error handling in Python code
Files:
cognee/api/v1/search/search.pycognee/api/v1/search/routers/get_search_router.py
⚙️ CodeRabbit configuration file
**/*.py: When reviewing Python code for this project:
- Prioritize portability over clarity, especially when dealing with cross-Python compatibility. However, with the priority in mind, do still consider improvements to clarity when relevant.
- As a general guideline, consider the code style advocated in the PEP 8 standard (excluding the use of spaces for indentation) and evaluate suggested changes for code style compliance.
- As a style convention, consider the code style advocated in CEP-8 and evaluate suggested changes for code style compliance.
- As a general guideline, try to provide any relevant, official, and supporting documentation links to any tool's suggestions in review comments. This guideline is important for posterity.
- As a general rule, undocumented function definitions and class definitions in the project's Python code are assumed incomplete. Please consider suggesting a short summary of the code for any of these incomplete definitions as docstrings when reviewing.
Files:
cognee/api/v1/search/search.pycognee/api/v1/search/routers/get_search_router.py
cognee/api/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Public APIs should be type-annotated in Python where practical
Files:
cognee/api/v1/search/search.pycognee/api/v1/search/routers/get_search_router.py
cognee/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use shared logging utilities from cognee.shared.logging_utils in Python code
Files:
cognee/api/v1/search/search.pycognee/api/v1/search/routers/get_search_router.py
🧬 Code graph analysis (2)
cognee/api/v1/search/search.py (4)
cognee/infrastructure/databases/exceptions/exceptions.py (1)
DatabaseNotCreatedError(5-20)cognee/exceptions/exceptions.py (1)
CogneeValidationError(52-63)cognee/modules/users/exceptions/exceptions.py (1)
UserNotFoundError(29-38)cognee/modules/users/methods/get_default_user.py (1)
get_default_user(13-38)
cognee/api/v1/search/routers/get_search_router.py (3)
cognee/modules/users/exceptions/exceptions.py (2)
PermissionDeniedError(41-48)UserNotFoundError(29-38)cognee/infrastructure/databases/exceptions/exceptions.py (1)
DatabaseNotCreatedError(5-20)cognee/exceptions/exceptions.py (1)
CogneeValidationError(52-63)
🪛 GitHub Check: CodeQL
cognee/api/v1/search/routers/get_search_router.py
[warning] 148-152: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: Basic Tests / Run Integration Tests
- GitHub Check: End-to-End Tests / Test Cognify - Edge Centered Payload
- GitHub Check: End-to-End Tests / Test dataset database handlers in Cognee
- GitHub Check: End-to-End Tests / Test Entity Extraction
- GitHub Check: End-to-End Tests / Test adding of label for data in Cognee
- GitHub Check: End-to-End Tests / Test Feedback Enrichment
- GitHub Check: End-to-End Tests / S3 Bucket Test
- GitHub Check: End-to-End Tests / Test permissions with different situations in Cognee
- GitHub Check: End-to-End Tests / Concurrent Subprocess access test
- GitHub Check: End-to-End Tests / Test graph edge ingestion
- GitHub Check: End-to-End Tests / Test multi tenancy with different situations in Cognee
- GitHub Check: End-to-End Tests / Conversation sessions test (Redis)
- GitHub Check: End-to-End Tests / Conversation sessions test (FS)
- GitHub Check: End-to-End Tests / Test using different async databases in parallel in Cognee
- GitHub Check: Basic Tests / Run Unit Tests
- GitHub Check: End-to-End Tests / Test dataset database deletion in Cognee
- GitHub Check: End-to-End Tests / Deduplication Test
- GitHub Check: End-to-End Tests / Run Telemetry Pipeline Test
- GitHub Check: End-to-End Tests / Server Start Test
- GitHub Check: Basic Tests / Run Simple Examples
- GitHub Check: Basic Tests / Run Simple Examples BAML
- GitHub Check: CLI Tests / CLI Functionality Tests
- GitHub Check: CLI Tests / CLI Integration Tests
🔇 Additional comments (1)
cognee/api/v1/search/search.py (1)
182-193: Excellent precondition check with clear error messaging.The try/except block correctly catches low-level exceptions (
DatabaseNotCreatedError,UserNotFoundError) fromget_default_user()and converts them into a user-friendlyCogneeValidationErrorwith actionable guidance. Usingfrom errorpreserves the exception chain for debugging, and the customname="SearchPreconditionError"allows downstream code to distinguish this failure mode.
| except (DatabaseNotCreatedError, UserNotFoundError, CogneeValidationError) as e: | ||
| # Return a clear 422 with actionable guidance instead of leaking a stacktrace | ||
| status_code = getattr(e, "status_code", 422) | ||
| return JSONResponse( | ||
| status_code=status_code, | ||
| content={ | ||
| "error": "Search prerequisites not met", | ||
| "detail": str(e), | ||
| "hint": "Run `await cognee.add(...)` then `await cognee.cognify()` before searching.", | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Status code semantic mismatch for UserNotFoundError.
UserNotFoundError has status_code=404 (from the exception definition), but the error response says "Search prerequisites not met" which semantically aligns with 422 (unprocessable entity), not 404 (not found). This could confuse API consumers who rely on HTTP status semantics.
Since search.py already converts both DatabaseNotCreatedError and UserNotFoundError into CogneeValidationError (which defaults to 422), consider catching only CogneeValidationError here and removing the individual exception types from the tuple to avoid this inconsistency.
🔎 Proposed fix: catch only CogneeValidationError
- except (DatabaseNotCreatedError, UserNotFoundError, CogneeValidationError) as e:
+ except CogneeValidationError as e:
# Return a clear 422 with actionable guidance instead of leaking a stacktrace
status_code = getattr(e, "status_code", 422)This simplifies the handler and ensures consistent 422 responses since search.py already normalizes the exceptions.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 148-152: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
🤖 Prompt for AI Agents
In cognee/api/v1/search/routers/get_search_router.py around lines 143 to 153,
the except block currently catches (DatabaseNotCreatedError, UserNotFoundError,
CogneeValidationError) causing a semantic mismatch when UserNotFoundError (404)
is returned while the body implies a 422; change the handler to catch only
CogneeValidationError (i.e., except CogneeValidationError as e) so responses are
consistently treated as validation/prerequisite errors and keep the existing
JSONResponse construction (using getattr(e, "status_code", 422) if needed).
Description
Acceptance Criteria
Type of Change
Screenshots/Videos (if applicable)
Pre-submission Checklist
DCO Affirmation
I affirm that all code in every commit of this pull request conforms to the terms of the Topoteretes Developer Certificate of Origin.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.