Conversation
Bumps [litellm](https://github.com/BerriAI/litellm) from 1.76.2 to 1.76.3. - [Release notes](https://github.com/BerriAI/litellm/releases) - [Commits](https://github.com/BerriAI/litellm/commits) --- updated-dependencies: - dependency-name: litellm dependency-version: 1.76.3 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [pytest](https://github.com/pytest-dev/pytest) from 8.4.1 to 8.4.2. - [Release notes](https://github.com/pytest-dev/pytest/releases) - [Changelog](https://github.com/pytest-dev/pytest/blob/main/CHANGELOG.rst) - [Commits](pytest-dev/pytest@8.4.1...8.4.2) --- updated-dependencies: - dependency-name: pytest dependency-version: 8.4.2 dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [pytest-playwright](https://github.com/microsoft/playwright-pytest) from 0.7.0 to 0.7.1. - [Release notes](https://github.com/microsoft/playwright-pytest/releases) - [Commits](microsoft/playwright-pytest@v0.7.0...v0.7.1) --- updated-dependencies: - dependency-name: pytest-playwright dependency-version: 0.7.1 dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/parser) from 8.40.0 to 8.42.0. - [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases) - [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/parser/CHANGELOG.md) - [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v8.42.0/packages/parser) --- updated-dependencies: - dependency-name: "@typescript-eslint/parser" dependency-version: 8.42.0 dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [eslint](https://github.com/eslint/eslint) from 9.34.0 to 9.35.0. - [Release notes](https://github.com/eslint/eslint/releases) - [Changelog](https://github.com/eslint/eslint/blob/main/CHANGELOG.md) - [Commits](eslint/eslint@v9.34.0...v9.35.0) --- updated-dependencies: - dependency-name: eslint dependency-version: 9.35.0 dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Break the Core as a library
WalkthroughAdds core modules for error types, streaming schema loading, and text-to-SQL orchestration; refactors route handlers to delegate to core; upgrades runtime/dependencies (Python 3.12, fastmcp), adds MCP integration and server manifest, tweaks Docker/frontend, CI, and project policy files. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Route as "POST /database"
participant SchemaCore as "api.core.schema_loader"
participant DBLoader as "DB Loader"
Client->>Route: POST /database { url }
Route->>SchemaCore: load_database(url, user_id) (async generator)
activate SchemaCore
SchemaCore-->>Client: stream { step: start }|||FALKORDB_MESSAGE_BOUNDARY|||
SchemaCore->>SchemaCore: detect DB type
SchemaCore-->>Client: stream { step: detect_db_type }|||FALKORDB_MESSAGE_BOUNDARY|||
SchemaCore->>DBLoader: loader.load(user_id, url) (async progress)
DBLoader-->>SchemaCore: progress / final / error
SchemaCore-->>Client: stream { progress | final_result | error }|||FALKORDB_MESSAGE_BOUNDARY|||
deactivate SchemaCore
sequenceDiagram
autonumber
participant Client
participant Routes as "api/routes/graphs"
participant Core as "api.core.text2sql"
participant Store as "Graph Store"
participant Loader as "DB Loader"
Client->>Routes: list/get/query/confirm/refresh/delete
Routes->>Core: delegate request
alt fetch schema
Core->>Store: fetch graph schema
Store-->>Core: schema | not found
Core-->>Routes: schema | GraphNotFoundError/InternalError
else query (stream)
Core-->>Client: stream steps/sql/results/ai_response|||FALKORDB_MESSAGE_BOUNDARY|||
Core->>Loader: execute SQL (may be destructive)
Loader-->>Core: rows/status
Core->>Store: optional schema refresh
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Dependency ReviewThe following issues were found:
|
…cript-eslint/parser-8.42.0
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| return StreamingResponse(generate(), media_type="application/json") | ||
| generator = await load_database(db_request.url, request.state.user_id) | ||
| return StreamingResponse(generator, media_type="application/json") |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
To resolve the information exposure risk, the system should stream only high-level, generic error messages to the user and avoid passing exception details (such as from str(e) in exception handlers) through the API response. In api/loaders/mysql_loader.py, inside the loader's load method, change the yielded error messages to generic phrases (e.g., "MySQL connection error" and "Error loading MySQL schema") without including str(e). Continue logging the detailed errors for server-side debugging. Only the server logs should retain full stack trace and error information, not the client-facing responses.
Only edit the code inside the shown snippets: update the yield statements in the exception handlers (except pymysql.MySQLError as e and the broad except Exception as e) to remove variable content from str(e), yielding generic messages instead. No further import changes or method definitions are necessary since generic error handling and logging are already present.
| @@ -205,10 +205,10 @@ | ||
|
|
||
| except pymysql.MySQLError as e: | ||
| logging.error("MySQL connection error: %s", e) | ||
| yield False, f"MySQL connection error: {str(e)}" | ||
| yield False, "MySQL connection error" | ||
| except Exception as e: # pylint: disable=broad-exception-caught | ||
| logging.error("Error loading MySQL schema: %s", e) | ||
| yield False, f"Error loading MySQL schema: {str(e)}" | ||
| yield False, "Error loading MySQL schema" | ||
|
|
||
| @staticmethod | ||
| def extract_tables_info(cursor, db_name: str) -> Dict[str, Any]: |
| schema = await get_schema(request.state.user_id, graph_id) | ||
| return JSONResponse(content=schema) | ||
| except GraphNotFoundError as gnfe: | ||
| return JSONResponse(content={"error": str(gnfe)}, status_code=404) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To resolve the issue, we should replace the usage of str(gnfe) with a fixed, generic error message intended for end-user consumption, such as "Graph not found". The details of the original exception should be logged server-side (using Python's logging module), but not exposed in the response. This change should only be made in the except GraphNotFoundError as gnfe block (line 76) of the file api/routes/graphs.py. No other existing logic should be altered. For server-side logging, we need to ensure the logging module is imported and used.
Steps to implement:
- Add an import for the
loggingmodule. - Log the exception details (with stack trace) on the server in the
except GraphNotFoundError as gnfeblock. - Replace the user-facing error message with a generic "Graph not found" string.
| @@ -3,6 +3,7 @@ | ||
| from fastapi import APIRouter, Request, HTTPException, UploadFile, File | ||
| from fastapi.responses import JSONResponse, StreamingResponse | ||
| from pydantic import BaseModel | ||
| import logging | ||
|
|
||
| from api.core.schema_loader import list_databases | ||
| from api.core.text2sql import ( | ||
| @@ -73,7 +74,8 @@ | ||
| schema = await get_schema(request.state.user_id, graph_id) | ||
| return JSONResponse(content=schema) | ||
| except GraphNotFoundError as gnfe: | ||
| return JSONResponse(content={"error": str(gnfe)}, status_code=404) | ||
| logging.error("GraphNotFoundError: %s", gnfe, exc_info=True) | ||
| return JSONResponse(content={"error": "Graph not found"}, status_code=404) | ||
| except InternalError as ie: | ||
| return JSONResponse(content={"error": str(ie)}, status_code=500) | ||
|
|
|
|
||
| try: | ||
| generator = await query_database(request.state.user_id, graph_id, chat_data) | ||
| return StreamingResponse(generator, media_type="application/json") |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
The correct fix is to sanitize all exception-derived error messages before they are sent to end users in API responses. In particular, any exception caught internally and returning its str(e) content to the user should instead be replaced by a generic error message. The original error string should only be logged on the server for operator/debugging use.
Targeted changes:
- api/loaders/mysql_loader.py: In the
MySQLLoader.load()method (lines 206–208 and 209–211), replace the yields which exposestr(e)(either directly or within formatted strings) with generic, non-specific messages. Continue to log the exception details internally. - Make sure this applies to any yield that can be surfaced to the user through the dataflow described (i.e., via streaming/response mechanisms).
- No changes need to be made to the API layer or FastAPI route code, as the vulnerability is best handled at the source, in the loader method "load," ensuring any error surfaced is sanitized.
| @@ -205,10 +205,10 @@ | ||
|
|
||
| except pymysql.MySQLError as e: | ||
| logging.error("MySQL connection error: %s", e) | ||
| yield False, f"MySQL connection error: {str(e)}" | ||
| yield False, "MySQL connection error. Please check your configuration and try again." | ||
| except Exception as e: # pylint: disable=broad-exception-caught | ||
| logging.error("Error loading MySQL schema: %s", e) | ||
| yield False, f"Error loading MySQL schema: {str(e)}" | ||
| yield False, "An unexpected error occurred while loading MySQL schema." | ||
|
|
||
| @staticmethod | ||
| def extract_tables_info(cursor, db_name: str) -> Dict[str, Any]: |
| except Exception as e: | ||
| logging.error("Error in refresh_graph_schema: %s", str(e)) | ||
| raise HTTPException(status_code=500, detail="Internal server error while refreshing schema") # pylint: disable=raise-missing-from | ||
| return await refresh_database_schema(request.state.user_id, graph_id) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fix this, make sure that any error data yielded by the loader (specifically in PostgresLoader.load) does not expose the full exception message or stack trace to the external user.
- In
PostgresLoader.load, alter theyield False, ...lines so that the error message is a sanitized, generic error description such as"Database connection error"or"Failed to load schema from database"rather than including rawstr(e)from exceptions. - Continue to log the detailed exception and stack on the server side for debugging, but do NOT yield their content to the client.
- This ensures external users see only generic, non-sensitive errors; detailed diagnostics stay in server logs.
- Only the file
api/loaders/postgres_loader.pyneeds a change: lines inPostgresLoader.loadwhereFalse, <raw error string>is yielded should be replaced with a generic message. - No new dependencies are needed; just use Python's built-in logging for server-side error trace.
| @@ -148,10 +148,10 @@ | ||
|
|
||
| except psycopg2.Error as e: | ||
| logging.error("PostgreSQL connection error: %s", e) | ||
| yield False, f"PostgreSQL connection error: {str(e)}" | ||
| yield False, "Database connection error" | ||
| except Exception as e: # pylint: disable=broad-exception-caught | ||
| logging.error("Error loading PostgreSQL schema: %s", e) | ||
| yield False, f"Error loading PostgreSQL schema: {str(e)}" | ||
| yield False, "Failed to load schema from database" | ||
|
|
||
| @staticmethod | ||
| def extract_tables_info(cursor) -> Dict[str, Any]: |
| return JSONResponse(content=result) | ||
|
|
||
| except InvalidArgumentError as iae: | ||
| return JSONResponse(content={"error": str(iae)}, status_code=400) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fix this issue, change the error message returned in the response from including the exception string (str(iae)) to a generic, non-sensitive message such as "Invalid arguments provided". Instead of exposing potentially sensitive data to users, you should log the actual error details on the server side for diagnostics. The change is localized to the handling of InvalidArgumentError in the delete_graph function (lines 197-198 of api/routes/graphs.py). To log the exception details, you can use the standard Python logging module—add an import for logging if not already present, configure logging if needed, and replace the exception handler to log the real issue before responding with the generic message.
Required changes:
- Add
import loggingif it's not yet present. - In the
except InvalidArgumentError as iae:block, log the error with its details. - Change the response to a generic error message, e.g.,
{"error": "Invalid arguments provided"}.
| @@ -1,6 +1,7 @@ | ||
| """Graph-related routes for the text2sql API.""" | ||
|
|
||
| from fastapi import APIRouter, Request, HTTPException, UploadFile, File | ||
| import logging | ||
| from fastapi.responses import JSONResponse, StreamingResponse | ||
| from pydantic import BaseModel | ||
|
|
||
| @@ -195,7 +196,8 @@ | ||
| return JSONResponse(content=result) | ||
|
|
||
| except InvalidArgumentError as iae: | ||
| return JSONResponse(content={"error": str(iae)}, status_code=400) | ||
| logging.error("InvalidArgumentError in delete_graph: %s", iae) | ||
| return JSONResponse(content={"error": "Invalid arguments provided"}, status_code=400) | ||
| except GraphNotFoundError as gnfe: | ||
| return JSONResponse(content={"error": str(gnfe)}, status_code=404) | ||
| except InternalError as ie: |
| except InvalidArgumentError as iae: | ||
| return JSONResponse(content={"error": str(iae)}, status_code=400) | ||
| except GraphNotFoundError as gnfe: | ||
| return JSONResponse(content={"error": str(gnfe)}, status_code=404) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
The fix is to avoid exposing internal exception messages to the client. Specifically, we should replace {"error": str(gnfe)} with a static and generic error message like {"error": "Graph not found"}. Internally, we may wish to log the full exception details for debugging, but only return safe content externally.
- Edit the
except GraphNotFoundError as gnfeblock in thedelete_graphendpoint (lines 199–200). - Replace use of
str(gnfe)with a static string. - If logging is appropriate and available in the codebase context, log the original exception (
gnfe) server-side, but don't expose details in the response. - No external dependencies are needed; optionally, add
import loggingand log the error, if permitted.
| @@ -197,6 +197,7 @@ | ||
| except InvalidArgumentError as iae: | ||
| return JSONResponse(content={"error": str(iae)}, status_code=400) | ||
| except GraphNotFoundError as gnfe: | ||
| return JSONResponse(content={"error": str(gnfe)}, status_code=404) | ||
| # Optionally log the error internally; do NOT expose details. | ||
| return JSONResponse(content={"error": "Graph not found."}, status_code=404) | ||
| except InternalError as ie: | ||
| return JSONResponse(content={"error": str(ie)}, status_code=500) |
| except GraphNotFoundError as gnfe: | ||
| return JSONResponse(content={"error": str(gnfe)}, status_code=404) | ||
| except InternalError as ie: | ||
| return JSONResponse(content={"error": str(ie)}, status_code=500) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fix this issue, update the exception handler for InternalError in the delete_graph endpoint (lines 201-202 of api/routes/graphs.py) so that a generic error message is returned to the user instead of the string representation of the exception. For example, you can return "An internal error has occurred." or similar. To preserve debugging usefulness, you may want to log the exception server-side, but do not send its details to the client.
No new imports or methods are strictly required unless you want enhanced logging; however, for completeness, you may use Python's built-in logging if not already present, but only if it is shown in the context.
The concrete change is:
- On line 202, replace
{"error": str(ie)}with a generic message like{"error": "An internal error has occurred."}.
| @@ -199,4 +199,4 @@ | ||
| except GraphNotFoundError as gnfe: | ||
| return JSONResponse(content={"error": str(gnfe)}, status_code=404) | ||
| except InternalError as ie: | ||
| return JSONResponse(content={"error": str(ie)}, status_code=500) | ||
| return JSONResponse(content={"error": "An internal error has occurred."}, status_code=500) |
…ging/typescript-eslint/parser-8.42.0 Bump @typescript-eslint/parser from 8.40.0 to 8.42.0 in /app
…ging/eslint-9.35.0 Bump eslint from 9.34.0 to 9.35.0 in /app
…laywright-0.7.1 Bump pytest-playwright from 0.7.0 to 0.7.1
….4.2 Bump pytest from 8.4.1 to 8.4.2
…1.76.3 Bump litellm from 1.76.2 to 1.76.3
Bumps [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/eslint-plugin) from 8.40.0 to 8.42.0. - [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases) - [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/CHANGELOG.md) - [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v8.42.0/packages/eslint-plugin) --- updated-dependencies: - dependency-name: "@typescript-eslint/eslint-plugin" dependency-version: 8.42.0 dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [playwright](https://github.com/microsoft/playwright-python) from 1.54.0 to 1.55.0. - [Release notes](https://github.com/microsoft/playwright-python/releases) - [Commits](microsoft/playwright-python@v1.54.0...v1.55.0) --- updated-dependencies: - dependency-name: playwright dependency-version: 1.55.0 dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
…ht-1.55.0 Bump playwright from 1.54.0 to 1.55.0
…cript-eslint/eslint-plugin-8.42.0
…ging/typescript-eslint/eslint-plugin-8.42.0 Bump @typescript-eslint/eslint-plugin from 8.40.0 to 8.42.0 in /app
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
api/routes/database.py (1)
34-35: Handle InvalidArgumentError to avoid 500s and potential info exposure.
load_databasevalidates URL before creating the generator and can raiseInvalidArgumentError; currently this bubbles up as a 500.Apply:
- generator = await load_database(db_request.url, request.state.user_id) - return StreamingResponse(generator, media_type="application/json") + try: + generator = await load_database(db_request.url, request.state.user_id) + except InvalidArgumentError as iae: + return JSONResponse(content={"error": str(iae)}, status_code=400) + return StreamingResponse(generator, media_type="application/json")And add required imports:
-from fastapi.responses import StreamingResponse +from fastapi.responses import StreamingResponse, JSONResponse +from api.core.errors import InvalidArgumentErrorapi/routes/graphs.py (1)
60-67: Error surface looks safe; keep avoiding raw stack traces.You’re mapping to sanitized messages via typed errors. No action needed; note addresses prior CodeQL concerns.
🧹 Nitpick comments (17)
Pipfile (1)
20-20: Pin git dependency to a commit for reproducible builds.Using a moving branch ref ("staging") can break installs unpredictably. Prefer a commit SHA.
Example:
-graphiti-core = {ref = "staging", git = "git+https://github.com/FalkorDB/graphiti.git"} +graphiti-core = {ref = "<commit-sha>", git = "git+https://github.com/FalkorDB/graphiti.git"}api/core/errors.py (2)
1-1: Docstring scope nit.This module is used across the core API, not only “text2sql”. Consider broadening the wording.
-"""Custom exceptions for the text2sql API.""" +"""Custom exceptions for the core API layer."""
3-13: Fix typo and use more specific base class; export symbols.
- “Interal” → “Internal”.
- Derive InvalidArgumentError from ValueError for semantic accuracy.
- Add all and tighten docstrings.
-# Interal Error Exception -class InternalError(Exception): - """Custom exception for internal errors.""" +# Internal error. +class InternalError(Exception): + """Internal, unexpected error.""" -# Graph not found Exception -class GraphNotFoundError(Exception): - """Custom exception for graph not found errors.""" +# Graph not found. +class GraphNotFoundError(Exception): + """Graph not found.""" -# Wrong argument Exception -class InvalidArgumentError(Exception): - """Custom exception for invalid argument errors.""" +# Invalid argument. +class InvalidArgumentError(ValueError): + """Invalid argument.""" + +__all__ = ("InternalError", "GraphNotFoundError", "InvalidArgumentError")api/core/schema_loader.py (7)
1-2: Module docstring accuracy.This isn’t a route; it’s core schema loading utilities.
-"""Database connection routes for the text2sql API.""" +"""Core schema loading and database listing utilities used by the API."""
19-27: Clean up Pydantic model and validate URL type.Docstring has placeholders; prefer AnyUrl for basic validation or keep str if you must support nonstandard schemes.
-from pydantic import BaseModel +from pydantic import BaseModel, AnyUrl @@ -class DatabaseConnectionRequest(BaseModel): - """Database connection request model. - - Args: - BaseModel (_type_): _description_ - """ - url: str +class DatabaseConnectionRequest(BaseModel): + """Request payload for connecting to a database.""" + url: AnyUrl # accepts arbitrary schemes; validate presence/format
59-66: Minor readability fix for tuple assignment.- success, result = [False, ""] + success, result = False, ""
84-89: logging.exception: don’t pass the exception string.logging.exception already includes the traceback; passing str(e) is redundant and flagged by linters.
- except Exception as e: # pylint: disable=broad-exception-caught - logging.exception("Error while loading database schema: %s", str(e)) + except Exception as e: # pylint: disable=broad-exception-caught + logging.exception("Error while loading database schema") yield {"type": "error", "message": "Error connecting to database"}
127-131: logging.exception: same redundancy in outer generator.- except Exception as e: # pylint: disable=broad-exception-caught - logging.exception("Unexpected error in connect_database stream: %s", str(e)) + except Exception as e: # pylint: disable=broad-exception-caught + logging.exception("Unexpected error in connect_database stream") yield _step_result({"type": "error", "message": "Internal server error"})
15-16: Centralize MESSAGE_DELIMITER into a single backend constant and import it.
- Duplicates found at: api/core/schema_loader.py:16, api/core/text2sql.py:23, api/routes/database.py:13, and frontend app/ts/modules/config.ts:5.
- Action: create api/core/constants.py with MESSAGE_DELIMITER and update the three backend files to import it. Keep the TS export in app/ts/modules/config.ts (or introduce a build-time/shared config if you want a single cross-language source).
35-47: DB scheme detection — allow SQLAlchemy-style '+driver' schemes or document allowed prefixesRepo uses only "postgresql://"/"postgres://" and "mysql://". Either normalize the scheme in api/core/schema_loader.py (strip any "+driver" suffix — e.g. scheme.split("+")[0] or use urllib.parse) to accept "postgresql+psycopg2://" and "mysql+pymysql://", or explicitly document/validate only the existing prefixes (docs/postgres_loader.md, app/ts/modules/modals.ts, api/loaders/, tests/). Also standardize the invalid-URL wording: api/core/schema_loader.py currently raises "Invalid database URL format" — align this text with other user-facing validations (e.g. "Invalid database URL").
api/core/__init__.py (1)
13-20: Sort all for consistency and lint happiness.Minor nit; keeps exports stable and satisfies RUF022.
Apply:
-__all__ = [ - "InternalError", - "GraphNotFoundError", - "InvalidArgumentError", - "load_database", - "list_databases", - "MESSAGE_DELIMITER", -] +__all__ = [ + "GraphNotFoundError", + "InternalError", + "InvalidArgumentError", + "MESSAGE_DELIMITER", + "list_databases", + "load_database", +]api/routes/database.py (2)
7-7: Prefer importing from the public surface (api.core) to decouple routes from internals.Not mandatory, but keeps the route stable if internals move.
-from api.core.schema_loader import load_database +from api.core import load_database
35-35: Consider a streaming-appropriate media type.Since the body is a stream of JSON chunks delimited by a boundary string,
text/plain; charset=utf-8orapplication/x-ndjsonis typically clearer thanapplication/json.- return StreamingResponse(generator, media_type="application/json") + return StreamingResponse(generator, media_type="text/plain; charset=utf-8")api/core/text2sql.py (4)
71-80: Broaden DB URL detection (common DSNs).Support
mysql+pymysql://and typical Postgres driver-qualified URLs.- if db_url_lower.startswith('postgresql://') or db_url_lower.startswith('postgres://'): + if (db_url_lower.startswith('postgresql://') + or db_url_lower.startswith('postgres://') + or db_url_lower.startswith('postgresql+')): return 'postgresql', PostgresLoader - if db_url_lower.startswith('mysql://'): + if db_url_lower.startswith('mysql://') or db_url_lower.startswith('mysql+pymysql://'): return 'mysql', MySQLLoader
85-94: Docstring mismatch with implementation.Docstring says “wrap in repr()” but code doesn’t. Either adjust text or implement
repr.Apply either:
- Update docstring (simplest):
- Sanitize input for safe logging—remove newlines, - carriage returns, tabs, and wrap in repr(). + Sanitize input for safe logging—remove newlines, carriage returns, and tabs.
- Or wrap:
- return value.replace('\n', ' ').replace('\r', ' ').replace('\t', ' ') + cleaned = value.replace('\n', ' ').replace('\r', ' ').replace('\t', ' ') + return repr(cleaned)
497-505: Uselogging.exceptionfor caught exceptions to retain trace without leaking to clients.Keeps server logs rich while responses remain sanitized.
- logging.error("Error executing SQL query: %s", str(e)) # nosemgrep + logging.exception("Error executing SQL query") # nosemgrep ... - logging.error("Error executing confirmed SQL query: %s", str(e)) # nosemgrep + logging.exception("Error executing confirmed SQL query") # nosemgrep ... - lambda t: logging.error( # nosemgrep - "Failed confirmed query memory save failed: %s", t.exception() - ) if t.exception() else logging.info( + lambda t: logging.exception( # nosemgrep + "Failed confirmed query memory save failed" + ) if t.exception() else logging.info( "Failed confirmed query memory saved successfully" ) ... - logging.error("Error in refresh_graph_schema: %s", str(e)) + logging.exception("Error in refresh_graph_schema") ... - logging.exception("Failed to delete graph %s: %s", sanitize_log_input(namespaced), e) + logging.exception("Failed to delete graph %s", sanitize_log_input(namespaced))Also applies to: 718-719, 731-735, 777-778, 800-801
319-321: Avoid logging full generated SQL in info-level logs.Potential PII/data sensitivity; consider redaction or debug level.
- logging.info("Generated SQL query: %s", answer_an['sql_query']) # nosemgrep + logging.debug("Generated SQL query (redacted)")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Pipfile.lockis excluded by!**/*.lockapp/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
Pipfile(2 hunks)api/core/__init__.py(1 hunks)api/core/errors.py(1 hunks)api/core/schema_loader.py(1 hunks)api/core/text2sql.py(1 hunks)api/routes/database.py(2 hunks)api/routes/graphs.py(7 hunks)app/package.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All Python code should pass pylint (use make lint)
Files:
api/core/__init__.pyapi/core/errors.pyapi/core/schema_loader.pyapi/routes/database.pyapi/core/text2sql.pyapi/routes/graphs.py
Pipfile
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Manage Python dependencies with pipenv; declare packages in Pipfile
Files:
Pipfile
🧠 Learnings (1)
📚 Learning: 2025-09-02T15:24:21.682Z
Learnt from: CR
PR: FalkorDB/QueryWeaver#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-02T15:24:21.682Z
Learning: Applies to Pipfile : Manage Python dependencies with pipenv; declare packages in Pipfile
Applied to files:
Pipfile
🧬 Code graph analysis (5)
api/core/__init__.py (3)
api/core/errors.py (3)
InternalError(4-5)GraphNotFoundError(8-9)InvalidArgumentError(12-13)api/core/schema_loader.py (2)
load_database(96-139)list_databases(142-161)app/ts/modules/config.ts (1)
MESSAGE_DELIMITER(5-5)
api/core/schema_loader.py (6)
api/core/errors.py (1)
InvalidArgumentError(12-13)api/loaders/base_loader.py (1)
BaseLoader(8-92)api/loaders/postgres_loader.py (1)
PostgresLoader(27-544)api/loaders/mysql_loader.py (1)
MySQLLoader(28-581)app/ts/modules/config.ts (1)
MESSAGE_DELIMITER(5-5)api/routes/graphs.py (1)
list_graphs(38-43)
api/routes/database.py (2)
api/auth/user_management.py (1)
token_required(251-291)api/core/schema_loader.py (1)
load_database(96-139)
api/core/text2sql.py (7)
api/core/errors.py (3)
GraphNotFoundError(8-9)InternalError(4-5)InvalidArgumentError(12-13)api/core/schema_loader.py (2)
load_database(96-139)generate(107-137)api/config.py (1)
Config(62-148)api/graph.py (1)
get_db_description(39-54)api/loaders/postgres_loader.py (1)
PostgresLoader(27-544)api/loaders/mysql_loader.py (1)
MySQLLoader(28-581)api/memory/graphiti_tool.py (2)
MemoryTool(47-718)create(64-75)
api/routes/graphs.py (3)
api/core/schema_loader.py (1)
list_databases(142-161)api/core/text2sql.py (6)
ChatRequest(36-44)ConfirmRequest(47-55)execute_destructive_operation(593-750)get_schema(106-198)query_database(200-590)refresh_database_schema(752-778)api/core/errors.py (3)
GraphNotFoundError(8-9)InternalError(4-5)InvalidArgumentError(12-13)
🪛 Ruff (0.12.2)
api/core/__init__.py
13-20: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
api/core/schema_loader.py
46-46: Avoid specifying long messages outside the exception class
(TRY003)
87-87: Redundant exception object included in logging.exception call
(TRY401)
105-105: Avoid specifying long messages outside the exception class
(TRY003)
130-130: Redundant exception object included in logging.exception call
(TRY401)
api/core/text2sql.py
99-99: Avoid specifying long messages outside the exception class
(TRY003)
120-120: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
121-121: Avoid specifying long messages outside the exception class
(TRY003)
141-141: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
142-142: Avoid specifying long messages outside the exception class
(TRY003)
148-149: try-except-continue detected, consider logging the exception
(S112)
148-148: Do not catch blind exception: Exception
(BLE001)
176-177: try-except-continue detected, consider logging the exception
(S112)
176-176: Do not catch blind exception: Exception
(BLE001)
190-191: try-except-continue detected, consider logging the exception
(S112)
190-190: Do not catch blind exception: Exception
(BLE001)
215-215: Avoid specifying long messages outside the exception class
(TRY003)
218-218: Avoid specifying long messages outside the exception class
(TRY003)
497-497: Do not catch blind exception: Exception
(BLE001)
500-500: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
613-613: Avoid specifying long messages outside the exception class
(TRY003)
717-717: Do not catch blind exception: Exception
(BLE001)
718-718: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
731-733: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
762-762: Avoid specifying long messages outside the exception class
(TRY003)
769-769: Abstract raise to an inner function
(TRY301)
769-769: Avoid specifying long messages outside the exception class
(TRY003)
777-777: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
778-778: Avoid specifying long messages outside the exception class
(TRY003)
790-790: Avoid specifying long messages outside the exception class
(TRY003)
796-796: Consider moving this statement to an else block
(TRY300)
798-798: Avoid specifying long messages outside the exception class
(TRY003)
800-800: Redundant exception object included in logging.exception call
(TRY401)
801-801: Avoid specifying long messages outside the exception class
(TRY003)
🪛 GitHub Check: CodeQL
api/routes/database.py
[warning] 35-35: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
api/routes/graphs.py
[warning] 64-64: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
[warning] 66-66: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
[warning] 118-118: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
[warning] 120-120: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
[warning] 138-138: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
[warning] 140-140: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
[warning] 153-153: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
[warning] 155-155: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
[warning] 175-175: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
[warning] 177-177: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
[warning] 179-179: 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). (3)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (3)
app/package.json (1)
14-16: ESLint/TS-ESLint bumps — cannot verify (missing deps & no lint script)Verification failed: Node reported MODULE_NOT_FOUND for @typescript-eslint/parser and @typescript-eslint/eslint-plugin and the repo has no "lint" script, so I couldn't confirm peer-dep warnings or that lint passes in CI. Install dependencies (npm ci / pnpm install), then run npx eslint -v, check the two @typescript-eslint package versions, and run the project's lint task locally/CI; report any peer warnings or lint failures.
Pipfile (2)
9-9: litellm bump: confirm no breaking config changes.File: Pipfile (line 9) — litellm = "~=1.76.3"
Ensure prompts, env vars, and client options still work with 1.76.3. Automated verification failed in the sandbox (pipenv not found). Run locally and paste output:
pipenv run python - <<'PY' import litellm, sys print("litellm:", litellm.__version__) PY pipenv check || trueIf not using pipenv, run in your venv:
python -c "import litellm, sys; print('litellm:', litellm.__version__)" pip check || true
23-26: Playwright upgrades: ensure CI installs browsers.CI must run
pipenv run playwright install(and on Linuxpipenv run playwright install-deps) after bumping Playwright; also confirm pytest-playwright is present. Sandbox verification failed:pipenvnot found here — run locally to verify:pipenv run playwright --version
pipenv run playwright install(Linux) pipenv run playwright install-deps
pipenv run python - <<'PY'
import importlib.util
print("pytest-playwright installed:", importlib.util.find_spec("pytest_playwright") is not None)
PY
| async def list_databases(user_id: str, general_prefix: str, db) -> list[str]: | ||
| """ | ||
| This route is used to list all the graphs (databases names) that are available in the database. | ||
| """ | ||
| user_graphs = await db.list_graphs() | ||
|
|
||
| # Only include graphs that start with user_id + '_', and strip the prefix | ||
| filtered_graphs = [ | ||
| graph[len(f"{user_id}_") :] | ||
| for graph in user_graphs | ||
| if graph.startswith(f"{user_id}_") | ||
| ] | ||
|
|
||
| if general_prefix: | ||
| demo_graphs = [ | ||
| graph for graph in user_graphs if graph.startswith(general_prefix) | ||
| ] | ||
| filtered_graphs = filtered_graphs + demo_graphs | ||
|
|
||
| return filtered_graphs |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Signature/argument-order bug: list_databases call sites pass (user_id, db, GENERAL_PREFIX).
Current signature is (user_id, general_prefix, db) and will misassign args, causing AttributeError at runtime. Also dedupe overlapping prefixes.
Apply:
-async def list_databases(user_id: str, general_prefix: str, db) -> list[str]:
+from typing import Optional
+
+async def list_databases(user_id: str, db, general_prefix: Optional[str] = None) -> list[str]:
@@
- user_graphs = await db.list_graphs()
+ user_graphs = await db.list_graphs()
@@
- if general_prefix:
- demo_graphs = [
- graph for graph in user_graphs if graph.startswith(general_prefix)
- ]
- filtered_graphs = filtered_graphs + demo_graphs
+ if general_prefix:
+ demo_graphs = [g for g in user_graphs if g.startswith(general_prefix)]
+ # Preserve order while removing duplicates
+ filtered_graphs = list(dict.fromkeys(filtered_graphs + demo_graphs))
- return filtered_graphs
+ return filtered_graphs🏁 Script executed:
#!/bin/bash
# Confirm all call-sites use the (user_id, db, general_prefix) order.
rg -nP '\blist_databases\s*\(' -C2Length of output: 686
Signature/argument-order bug — change list_databases to (user_id, db, general_prefix) and dedupe overlaps
Call-site api/routes/graphs.py:42 calls list_databases(request.state.user_id, db, GENERAL_PREFIX); current signature (user_id, general_prefix, db) will assign a string to db and raise AttributeError at runtime. Function location: api/core/schema_loader.py (lines ~142–161).
-async def list_databases(user_id: str, general_prefix: str, db) -> list[str]:
+from typing import Optional
+
+async def list_databases(user_id: str, db, general_prefix: Optional[str] = None) -> list[str]:
@@
- user_graphs = await db.list_graphs()
+ user_graphs = await db.list_graphs()
@@
- if general_prefix:
- demo_graphs = [
- graph for graph in user_graphs if graph.startswith(general_prefix)
- ]
- filtered_graphs = filtered_graphs + demo_graphs
+ if general_prefix:
+ demo_graphs = [g for g in user_graphs if g.startswith(general_prefix)]
+ # Preserve order while removing duplicates
+ filtered_graphs = list(dict.fromkeys(filtered_graphs + demo_graphs))
- return filtered_graphs
+ return filtered_graphs📝 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.
| async def list_databases(user_id: str, general_prefix: str, db) -> list[str]: | |
| """ | |
| This route is used to list all the graphs (databases names) that are available in the database. | |
| """ | |
| user_graphs = await db.list_graphs() | |
| # Only include graphs that start with user_id + '_', and strip the prefix | |
| filtered_graphs = [ | |
| graph[len(f"{user_id}_") :] | |
| for graph in user_graphs | |
| if graph.startswith(f"{user_id}_") | |
| ] | |
| if general_prefix: | |
| demo_graphs = [ | |
| graph for graph in user_graphs if graph.startswith(general_prefix) | |
| ] | |
| filtered_graphs = filtered_graphs + demo_graphs | |
| return filtered_graphs | |
| from typing import Optional | |
| async def list_databases(user_id: str, db, general_prefix: Optional[str] = None) -> list[str]: | |
| """ | |
| This route is used to list all the graphs (databases names) that are available in the database. | |
| """ | |
| user_graphs = await db.list_graphs() | |
| # Only include graphs that start with user_id + '_', and strip the prefix | |
| filtered_graphs = [ | |
| graph[len(f"{user_id}_") :] | |
| for graph in user_graphs | |
| if graph.startswith(f"{user_id}_") | |
| ] | |
| if general_prefix: | |
| demo_graphs = [g for g in user_graphs if g.startswith(general_prefix)] | |
| # Preserve order while removing duplicates | |
| filtered_graphs = list(dict.fromkeys(filtered_graphs + demo_graphs)) | |
| return filtered_graphs |
🤖 Prompt for AI Agents
In api/core/schema_loader.py around lines 142–161, the function signature and
argument order are wrong: change list_databases to accept (user_id: str, db,
general_prefix: str) so the db parameter gets the DB object passed by callers;
update the type hint for db appropriately (e.g., the DB client type or Any).
Inside the function preserve the existing filtering logic but when combining
user-specific graphs and demo_graphs dedupe overlaps (e.g., use an ordered set
or filter demo_graphs to exclude names that already matched the user prefix)
before returning, and ensure the return type annotation remains list[str].
| def _graph_name(user_id: str, graph_id:str) -> str: | ||
|
|
||
| graph_id = graph_id.strip()[:200] | ||
| if not graph_id: | ||
| raise GraphNotFoundError("Invalid graph_id, must be less than 200 characters.") | ||
|
|
||
| if GENERAL_PREFIX and graph_id.startswith(GENERAL_PREFIX): | ||
| return graph_id | ||
|
|
||
| return f"{user_id}_{graph_id}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix validation semantics and error type in _graph_name.
Message claims a 200-char limit but code only trims; also raising GraphNotFoundError for invalid input is misleading.
Apply:
-def _graph_name(user_id: str, graph_id:str) -> str:
-
- graph_id = graph_id.strip()[:200]
- if not graph_id:
- raise GraphNotFoundError("Invalid graph_id, must be less than 200 characters.")
+def _graph_name(user_id: str, graph_id: str) -> str:
+ graph_id = graph_id.strip()
+ if not graph_id:
+ raise InvalidArgumentError("graph_id must be non-empty")
+ if len(graph_id) > 200:
+ raise InvalidArgumentError("graph_id must be <= 200 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.
| def _graph_name(user_id: str, graph_id:str) -> str: | |
| graph_id = graph_id.strip()[:200] | |
| if not graph_id: | |
| raise GraphNotFoundError("Invalid graph_id, must be less than 200 characters.") | |
| if GENERAL_PREFIX and graph_id.startswith(GENERAL_PREFIX): | |
| return graph_id | |
| return f"{user_id}_{graph_id}" | |
| def _graph_name(user_id: str, graph_id: str) -> str: | |
| graph_id = graph_id.strip() | |
| if not graph_id: | |
| raise InvalidArgumentError("graph_id must be non-empty") | |
| if len(graph_id) > 200: | |
| raise InvalidArgumentError("graph_id must be <= 200 characters") | |
| if GENERAL_PREFIX and graph_id.startswith(GENERAL_PREFIX): | |
| return graph_id | |
| return f"{user_id}_{graph_id}" |
🧰 Tools
🪛 Ruff (0.12.2)
99-99: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In api/core/text2sql.py around lines 95 to 104, the function currently trims
graph_id to 200 characters and raises GraphNotFoundError for invalid input;
change it to validate instead of silently truncating: strip whitespace, if the
resulting graph_id is empty or longer than 200 characters raise a ValueError (or
a more specific invalid-id exception if available) with a clear message about
the 200-char limit, keep the existing GENERAL_PREFIX check, and only return
f"{user_id}_{graph_id}" when validation passes.
| graphs = await list_databases(request.state.user_id, db, GENERAL_PREFIX) | ||
| return JSONResponse(content=graphs) |
There was a problem hiding this comment.
Argument order bug: list_databases parameters are swapped.
Signature is (user_id, general_prefix, db) but call passes (user_id, db, GENERAL_PREFIX), leading to 'str' object has no attribute 'list_graphs' at runtime.
Apply:
- graphs = await list_databases(request.state.user_id, db, GENERAL_PREFIX)
+ graphs = await list_databases(request.state.user_id, GENERAL_PREFIX, db)Optional: import from api.core to use the public API.
-from api.core.schema_loader import list_databases
+from api.core import list_databases📝 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.
| graphs = await list_databases(request.state.user_id, db, GENERAL_PREFIX) | |
| return JSONResponse(content=graphs) | |
| graphs = await list_databases(request.state.user_id, GENERAL_PREFIX, db) | |
| return JSONResponse(content=graphs) |
🤖 Prompt for AI Agents
In api/routes/graphs.py around lines 42 to 43, the call to list_databases passes
arguments in the wrong order (user_id, db, GENERAL_PREFIX) while the function
signature is (user_id, general_prefix, db); swap the second and third arguments
to call list_databases(request.state.user_id, GENERAL_PREFIX, db) so the db
object is passed last and avoid the "'str' object has no attribute
'list_graphs'" runtime error; optionally import list_databases from api.core to
use the public API if not already imported.
| return await refresh_database_schema(request.state.user_id, graph_id) | ||
| except InternalError as ie: | ||
| return JSONResponse(content={"error": str(ie)}, status_code=500) |
There was a problem hiding this comment.
Streaming bug: returning a generator directly instead of a StreamingResponse.
refresh_database_schema returns a generator (delegates to load_database). FastAPI expects a Response; returning the generator will 500.
Apply:
- try:
- return await refresh_database_schema(request.state.user_id, graph_id)
+ try:
+ generator = await refresh_database_schema(request.state.user_id, graph_id)
+ return StreamingResponse(generator, media_type="text/plain; charset=utf-8")Ensure import (top of file):
-from fastapi.responses import JSONResponse, StreamingResponse
+from fastapi.responses import JSONResponse, StreamingResponse(already present)
📝 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.
| return await refresh_database_schema(request.state.user_id, graph_id) | |
| except InternalError as ie: | |
| return JSONResponse(content={"error": str(ie)}, status_code=500) | |
| generator = await refresh_database_schema(request.state.user_id, graph_id) | |
| return StreamingResponse(generator, media_type="text/plain; charset=utf-8") | |
| except InternalError as ie: | |
| return JSONResponse(content={"error": str(ie)}, status_code=500) |
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 153-153: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
[warning] 155-155: 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 api/routes/graphs.py around lines 153-155, the handler returns the generator
from refresh_database_schema directly which causes a 500; wrap the generator in
a FastAPI/Starlette StreamingResponse before returning (e.g. return
StreamingResponse(refresh_database_schema(request.state.user_id, graph_id),
media_type="application/json") or the appropriate media_type for the stream) so
the generator is sent as a proper streaming HTTP response; ensure
StreamingResponse is imported at the top and preserve any headers/status if
required.
Update deps
Bumps [uvicorn](https://github.com/Kludex/uvicorn) from 0.35.0 to 0.36.0. - [Release notes](https://github.com/Kludex/uvicorn/releases) - [Changelog](https://github.com/Kludex/uvicorn/blob/main/docs/release-notes.md) - [Commits](Kludex/uvicorn@0.35.0...0.36.0) --- updated-dependencies: - dependency-name: uvicorn dependency-version: 0.36.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
fix docker build
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Dockerfile (1)
18-22: Consider purging build-essential after builds finish.Leaving
build-essentialin the final image roughly doubles the layer size (g++, make, headers). Since we only need it to compile wheels duringpipenv sync, please add anapt-get purge build-essential(andautoremove) once dependencies are installed so the runtime image stays lean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dockerfile(1 hunks)
⏰ 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). (5)
- GitHub Check: e2e-tests
- GitHub Check: unit-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
…0.36.0 Bump uvicorn from 0.36.0 to 0.37.0
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Pipfile.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
Pipfile(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
Pipfile
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Manage Python dependencies with pipenv; declare packages in Pipfile
Files:
Pipfile
⏰ 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). (6)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: unit-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: unit-tests
🔇 Additional comments (2)
Pipfile (2)
7-13: Solid bugfix upgrades on FastAPI/Authlib stackLine [7] pulls in FastAPI 0.117.1’s fix for the
File-after-Formvalidation bug, and Line [13] captures Authlib 1.6.4’s Starletteform_postand insecure transport fixes—both cut down on support noise without breaking compatibility.
(fastapi.tiangolo.com)
25-26: Verify Playwright-based tests for manifest v2 extensionsLine [25] moves Playwright to 1.55.0, which drops Chromium extension manifest v2 support—please confirm our browser tests (or dev tooling) aren’t still packaging v2 extensions before merging.
(playwright.dev)
| litellm = "~=1.76.0" | ||
| fastapi = "~=0.117.1" | ||
| uvicorn = "~=0.37.0" | ||
| litellm = "~=1.77.3" |
There was a problem hiding this comment.
Avoid litellm 1.77.3 due to known startup OOM
Line [9] pins to LiteLLM 1.77.3, but that release’s own notes flag a Kubernetes startup OOM regression; we should hold at 1.76.x or skip forward once a patched build (e.g., 1.77.4) is validated.
(docs.litellm.ai)
🤖 Prompt for AI Agents
In Pipfile around line 9, the dependency is pinned to litellm ~=1.77.3 which has
a known Kubernetes startup OOM regression; update the Pipfile to use a safe
version (e.g., pin to the last known-good 1.76.x series or wait and pin to
>=1.77.4 once a patched build is validated), then regenerate the lockfile
(pipenv lock or pipenv install) and run CI/tests to verify startup memory
behavior before merging.
Add Contributor Covenant Code of Conduct
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CODE_OF_CONDUCT.md (1)
117-128: Wrap bare URLs to satisfy markdownlint
markdownlint(MD034) is flagging the bare links. Wrapping them in angle brackets clears the warning without changing the text.-https://www.contributor-covenant.org/version/2/0/code_of_conduct.html. +<https://www.contributor-covenant.org/version/2/0/code_of_conduct.html>. … -https://www.contributor-covenant.org/faq. +<https://www.contributor-covenant.org/faq>. -https://www.contributor-covenant.org/translations. +<https://www.contributor-covenant.org/translations>.Based on static analysis hints
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/wordlist.txt(1 hunks)CODE_OF_CONDUCT.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
CODE_OF_CONDUCT.md
63-63: Bare URL used
(MD034, no-bare-urls)
119-119: Bare URL used
(MD034, no-bare-urls)
127-127: Bare URL used
(MD034, no-bare-urls)
128-128: Bare URL used
(MD034, no-bare-urls)
⏰ 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). (4)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (1)
.github/wordlist.txt (1)
88-91: Additions look goodThese extra tokens seem appropriate for the whitelist—no concerns on my end.
update all to Python 3.12
Bumps [fastapi](https://github.com/fastapi/fastapi) from 0.117.1 to 0.118.0. - [Release notes](https://github.com/fastapi/fastapi/releases) - [Commits](fastapi/fastapi@0.117.1...0.118.0) --- updated-dependencies: - dependency-name: fastapi dependency-version: 0.118.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
…0.118.0 Bump fastapi from 0.117.1 to 0.118.0
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Move from FastAPI_MCP to FastMCP
| except GraphNotFoundError as gnfe: | ||
| return JSONResponse(content={"error": str(gnfe)}, status_code=404) | ||
| except InternalError as ie: | ||
| return JSONResponse(content={"error": str(ie)}, status_code=500) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fix this problem, we should avoid returning the exception details (str(ie)) to the client. Instead, internally log the error (including its traceback for debugging) and return a generic message to clients. Only the GraphNotFoundError case should continue to return its string (all other responses should be generic).
Specifically, in get_graph_data, in the error handler for InternalError:
- Replace
return JSONResponse(content={"error": str(ie)}, status_code=500)with:- Log the exception on the server (using the
loggingmodule). - Return a generic error message such as "An internal error has occurred" with status code
500.
Also, ensure theloggingmodule is imported at the top of the file if it isn't already.
- Log the exception on the server (using the
Edits:
- Add
import loggingat the top (if not present). - Update the except block for
InternalErrorto log details and return a generic error message.
| @@ -2,6 +2,7 @@ | ||
|
|
||
| from fastapi import APIRouter, Request, HTTPException, UploadFile, File | ||
| from fastapi.responses import JSONResponse, StreamingResponse | ||
| import logging | ||
| from pydantic import BaseModel | ||
|
|
||
| from api.core.schema_loader import list_databases | ||
| @@ -75,7 +76,8 @@ | ||
| except GraphNotFoundError as gnfe: | ||
| return JSONResponse(content={"error": str(gnfe)}, status_code=404) | ||
| except InternalError as ie: | ||
| return JSONResponse(content={"error": str(ie)}, status_code=500) | ||
| logging.error("Internal error in get_graph_data: %s", ie, exc_info=True) | ||
| return JSONResponse(content={"error": "An internal error has occurred"}, status_code=500) | ||
|
|
||
|
|
||
| @graphs_router.post("", responses={401: UNAUTHORIZED_RESPONSE}) |
| generator = await query_database(request.state.user_id, graph_id, chat_data) | ||
| return StreamingResponse(generator, media_type="application/json") | ||
| except InvalidArgumentError as iae: | ||
| return JSONResponse(content={"error": str(iae)}, status_code=400) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fix the problem, avoid sending the raw exception message from the InvalidArgumentError instance (str(iae)) to the user in the HTTP response. Instead, return a fixed, generic error message to the user, and log the exception—including stack trace and details—for server-side diagnostics. Specifically, change line 145 to return a fixed error string such as "Invalid arguments provided." in the response, while ensuring the full exception info is logged.
- In
api/routes/graphs.py, update theexcept InvalidArgumentError as iaeblock in thequery_graphfunction:- Log the exception and stack trace on the server (using Python’s
loggingmodule). - In the response, send only a generic error message.
- Log the exception and stack trace on the server (using Python’s
- If the Python logging module is not already imported, add the import at the top of the file.
| @@ -3,7 +3,7 @@ | ||
| from fastapi import APIRouter, Request, HTTPException, UploadFile, File | ||
| from fastapi.responses import JSONResponse, StreamingResponse | ||
| from pydantic import BaseModel | ||
|
|
||
| import logging | ||
| from api.core.schema_loader import list_databases | ||
| from api.core.text2sql import ( | ||
| GENERAL_PREFIX, | ||
| @@ -142,7 +142,8 @@ | ||
| generator = await query_database(request.state.user_id, graph_id, chat_data) | ||
| return StreamingResponse(generator, media_type="application/json") | ||
| except InvalidArgumentError as iae: | ||
| return JSONResponse(content={"error": str(iae)}, status_code=400) | ||
| logging.exception("InvalidArgumentError in query_graph:") | ||
| return JSONResponse(content={"error": "Invalid arguments provided."}, status_code=400) | ||
|
|
||
|
|
||
| @graphs_router.post("/{graph_id}/confirm", responses={401: UNAUTHORIZED_RESPONSE}) |
| ) | ||
| return StreamingResponse(generator, media_type="application/json") | ||
| except InvalidArgumentError as iae: | ||
| return JSONResponse(content={"error": str(iae)}, status_code=400) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fix this problem, change the error handling for InvalidArgumentError in the confirm_destructive_operation route so that the actual exception message is not returned directly in the HTTP response. Instead, return a generic, user-friendly error message such as "An invalid argument was provided.", and log the full exception (including its stack trace) on the server for diagnosis.
This requires the following changes in api/routes/graphs.py:
- Change line 165 in the except block to use a generic error message.
- Add logging for the exception (e.g., using the standard Python
loggingmodule). - If not already imported in the file, import the
loggingmodule and initialize a logger at the top of the file.
| @@ -3,6 +3,8 @@ | ||
| from fastapi import APIRouter, Request, HTTPException, UploadFile, File | ||
| from fastapi.responses import JSONResponse, StreamingResponse | ||
| from pydantic import BaseModel | ||
| import logging | ||
| logger = logging.getLogger(__name__) | ||
|
|
||
| from api.core.schema_loader import list_databases | ||
| from api.core.text2sql import ( | ||
| @@ -162,7 +164,8 @@ | ||
| ) | ||
| return StreamingResponse(generator, media_type="application/json") | ||
| except InvalidArgumentError as iae: | ||
| return JSONResponse(content={"error": str(iae)}, status_code=400) | ||
| logger.exception("InvalidArgumentError in confirm_destructive_operation: %s", iae) | ||
| return JSONResponse(content={"error": "An invalid argument was provided."}, status_code=400) | ||
|
|
||
|
|
||
| @graphs_router.post("/{graph_id}/refresh", responses={401: UNAUTHORIZED_RESPONSE}) |
| return await connect_database(request, db_request) | ||
| return await refresh_database_schema(request.state.user_id, graph_id) | ||
| except InternalError as ie: | ||
| return JSONResponse(content={"error": str(ie)}, status_code=500) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fix the problem, replace the returned error message with a generic, non-sensitive response. Internally, the actual exception (including stack trace/content) should be logged on the server side for debugging purposes, but the error response sent to the client should NEVER include sensitive details.
Implementation Plan:
- In the endpoint handler (
refresh_graph_schema), change the line that returns the error to instead return a generic message, such as"An internal error has occurred while refreshing the schema."for code 500. - Log the actual exception server-side so that developers can investigate the root cause using logs.
- Because we have not been shown a logging import or setup, and only shown lines in this file, we can use Python's standard
loggingmodule in this file (add appropriate import at the top). - The change applies only to the error handling in the
refresh_graph_schemafunction, in fileapi/routes/graphs.py, specifically in the block starting at line 178-179.
| @@ -4,6 +4,7 @@ | ||
| from fastapi.responses import JSONResponse, StreamingResponse | ||
| from pydantic import BaseModel | ||
|
|
||
| import logging | ||
| from api.core.schema_loader import list_databases | ||
| from api.core.text2sql import ( | ||
| GENERAL_PREFIX, | ||
| @@ -176,7 +177,8 @@ | ||
| try: | ||
| return await refresh_database_schema(request.state.user_id, graph_id) | ||
| except InternalError as ie: | ||
| return JSONResponse(content={"error": str(ie)}, status_code=500) | ||
| logging.exception("Internal error occurred during schema refresh") | ||
| return JSONResponse(content={"error": "An internal error occurred while refreshing the schema."}, status_code=500) | ||
|
|
||
|
|
||
| @graphs_router.delete("/{graph_id}", responses={401: UNAUTHORIZED_RESPONSE}) |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/routes/database.py (1)
12-13: Drop the unused delimiter constant
MESSAGE_DELIMITERis no longer referenced in this module, so pylint will flag it as an unused variable. Remove it (or import the shared constant fromapi.core) to keepmake lintgreen.-# Use the same delimiter as in the JavaScript frontend for streaming chunks -MESSAGE_DELIMITER = "|||FALKORDB_MESSAGE_BOUNDARY|||"
♻️ Duplicate comments (1)
api/core/schema_loader.py (1)
98-141: Fix the remaining call sites that still pass(user_id, db_url)With the signature now
(url, user_id),refresh_database_schemainapi/core/text2sql.pystill callsload_database(user_id, db_url), so_step_detect_db_typeruns on a base64 user id, always raisingInvalidArgumentError. Refreshing schemas is therefore broken. Please swap the arguments at that call site (and any others) toload_database(db_url, user_id)so the URL detection works.- return await load_database(user_id, db_url) + return await load_database(db_url, user_id)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Pipfile.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Pipfile(1 hunks)api/app_factory.py(5 hunks)api/core/schema_loader.py(1 hunks)api/routes/database.py(3 hunks)api/routes/graphs.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Pipfile
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All Python code should pass pylint (use make lint)
Files:
api/routes/database.pyapi/routes/graphs.pyapi/app_factory.pyapi/core/schema_loader.py
🧬 Code graph analysis (3)
api/routes/database.py (2)
api/auth/user_management.py (1)
token_required(251-291)api/core/schema_loader.py (1)
load_database(98-141)
api/routes/graphs.py (4)
api/core/schema_loader.py (1)
list_databases(144-163)api/core/text2sql.py (8)
ChatRequest(36-44)ConfirmRequest(47-55)delete_database(780-801)execute_destructive_operation(593-750)get_schema(106-198)query_database(200-590)refresh_database_schema(752-778)GraphData(27-33)api/core/errors.py (3)
GraphNotFoundError(8-9)InternalError(4-5)InvalidArgumentError(12-13)api/auth/user_management.py (1)
token_required(251-291)
api/core/schema_loader.py (6)
api/core/errors.py (1)
InvalidArgumentError(12-13)api/loaders/base_loader.py (1)
BaseLoader(8-92)api/loaders/postgres_loader.py (1)
PostgresLoader(27-544)api/loaders/mysql_loader.py (1)
MySQLLoader(28-581)app/ts/modules/config.ts (1)
MESSAGE_DELIMITER(5-5)api/core/text2sql.py (1)
generate(236-588)
🪛 GitHub Check: CodeQL
api/routes/database.py
[warning] 35-35: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
api/routes/graphs.py
[warning] 76-76: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
[warning] 78-78: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
[warning] 143-143: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
[warning] 145-145: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
[warning] 163-163: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
[warning] 165-165: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
[warning] 177-177: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
[warning] 179-179: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
[warning] 198-198: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
[warning] 200-200: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
[warning] 202-202: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
🪛 Ruff (0.13.3)
api/routes/graphs.py
84-84: Unused function argument: request
(ARG001)
84-84: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
api/app_factory.py
199-199: Unused function argument: request
(ARG001)
api/core/schema_loader.py
48-48: Avoid specifying long messages outside the exception class
(TRY003)
89-89: Redundant exception object included in logging.exception call
(TRY401)
107-107: Avoid specifying long messages outside the exception class
(TRY003)
132-132: Redundant exception object included in logging.exception call
(TRY401)
⏰ 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). (4)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
| generator = await load_database(db_request.url, request.state.user_id) | ||
| return StreamingResponse(generator, media_type="application/json") |
There was a problem hiding this comment.
Handle invalid connection URLs explicitly
load_database raises InvalidArgumentError when the input URL is empty/invalid (see api/core/schema_loader.py:105). Because the route doesn’t catch it, FastAPI turns it into a 500, so the client can’t tell it’s a bad request. Wrap the call and return a 400 instead.
-from fastapi import APIRouter, Request
+from fastapi import APIRouter, Request, HTTPException
+from api.core.errors import InvalidArgumentError
@@
- generator = await load_database(db_request.url, request.state.user_id)
- return StreamingResponse(generator, media_type="application/json")
+ try:
+ generator = await load_database(db_request.url, request.state.user_id)
+ except InvalidArgumentError as exc:
+ raise HTTPException(status_code=400, detail=str(exc)) from exc
+ return StreamingResponse(generator, media_type="application/json")📝 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.
| generator = await load_database(db_request.url, request.state.user_id) | |
| return StreamingResponse(generator, media_type="application/json") | |
| # at the top of api/routes/database.py | |
| from fastapi import APIRouter, Request, HTTPException | |
| from api.core.errors import InvalidArgumentError | |
| # … other imports … | |
| @router.post("/database") | |
| async def database_route(request: Request): | |
| db_request = DatabaseRequest(**await request.json()) | |
| try: | |
| generator = await load_database(db_request.url, request.state.user_id) | |
| except InvalidArgumentError as exc: | |
| raise HTTPException(status_code=400, detail=str(exc)) from exc | |
| return StreamingResponse(generator, media_type="application/json") |
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 35-35: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
🤖 Prompt for AI Agents
In api/routes/database.py around lines 34 to 35, the call to load_database can
raise InvalidArgumentError for empty/invalid URLs which currently bubbles up as
a 500; catch InvalidArgumentError around the await load_database(...) call and
raise or return an HTTP 400 (use FastAPI's HTTPException(status_code=400,
detail="...") with a clear message or the original error message). Also add the
needed import for HTTPException (and for InvalidArgumentError if not already
imported) at the top of the file so the handler compiles.
| return await refresh_database_schema(request.state.user_id, graph_id) | ||
| except InternalError as ie: | ||
| return JSONResponse(content={"error": str(ie)}, status_code=500) | ||
|
|
There was a problem hiding this comment.
Wrap refresh stream and surface user errors
refresh_database_schema returns an async generator. Returning it directly makes FastAPI choke (500). It can also raise InvalidArgumentError (e.g., demo graphs), which currently bubbles up as another 500. Mirror the other streaming endpoints: await the generator, wrap it in StreamingResponse, and translate InvalidArgumentError into a 400.
- try:
- return await refresh_database_schema(request.state.user_id, graph_id)
- except InternalError as ie:
- return JSONResponse(content={"error": str(ie)}, status_code=500)
+ try:
+ generator = await refresh_database_schema(request.state.user_id, graph_id)
+ return StreamingResponse(generator, media_type="application/json")
+ except InvalidArgumentError as iae:
+ return JSONResponse(content={"error": str(iae)}, status_code=400)
+ except InternalError as ie:
+ return JSONResponse(content={"error": str(ie)}, status_code=500)📝 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.
| return await refresh_database_schema(request.state.user_id, graph_id) | |
| except InternalError as ie: | |
| return JSONResponse(content={"error": str(ie)}, status_code=500) | |
| try: | |
| generator = await refresh_database_schema(request.state.user_id, graph_id) | |
| return StreamingResponse(generator, media_type="application/json") | |
| except InvalidArgumentError as iae: | |
| return JSONResponse(content={"error": str(iae)}, status_code=400) | |
| except InternalError as ie: | |
| return JSONResponse(content={"error": str(ie)}, status_code=500) |
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 177-177: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
[warning] 179-179: 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 api/routes/graphs.py around lines 177-180, refresh_database_schema returns an
async generator so returning it directly causes a 500; change the handler to
await the generator, wrap the resulting async iterator in a StreamingResponse
(set media_type to match other streaming endpoints, e.g. "text/event-stream"),
and return that; also catch InvalidArgumentError and return a JSONResponse with
a 400 status and the error message, while preserving the existing InternalError
-> 500 handling; ensure StreamingResponse and InvalidArgumentError are imported
if not already.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
api/core/text2sql.py (5)
47-56: UseField(default_factory=list)for mutable defaults.The
chatfield uses a mutable list default, which creates a shared list across all model instances.Apply this diff:
-from pydantic import BaseModel +from pydantic import BaseModel, Fieldclass ConfirmRequest(BaseModel): """Confirmation request model. Args: BaseModel (_type_): _description_ """ sql_query: str confirmation: str = "" - chat: list = [] + chat: list = Field(default_factory=list)
772-772: Cannot await an async generator; return it directly.
load_databasereturns an async generator function that the caller must iterate. Awaiting it will raise a runtime error.Apply this diff:
- # Call load_database to refresh the schema by reconnecting - return await load_database(db_url, user_id) + # Call load_database to refresh the schema by reconnecting (returns stream) + return load_database(db_url, user_id)
416-419: Avoid blocking the event loop with synchronous DB calls.
execute_sql_queryuses synchronous database drivers (psycopg2/pymysql) which will block the event loop and degrade streaming responsiveness.Apply this diff to run in a thread:
- query_results = loader_class.execute_sql_query( - answer_an["sql_query"], - db_url - ) + query_results = await asyncio.to_thread( + loader_class.execute_sql_query, + answer_an["sql_query"], + db_url, + )
95-104: Validate length instead of silently truncating.The function truncates
graph_idto 200 characters without validation, which can cause namespace collisions. Additionally,GraphNotFoundErroris semantically incorrect for invalid input.Apply this diff to validate and use the correct error type:
def _graph_name(user_id: str, graph_id: str) -> str: - - graph_id = graph_id.strip()[:200] + graph_id = graph_id.strip() if not graph_id: - raise GraphNotFoundError("Invalid graph_id, must be less than 200 characters.") + raise InvalidArgumentError("graph_id must be non-empty") + if len(graph_id) > 200: + raise InvalidArgumentError("graph_id must be <= 200 characters") if GENERAL_PREFIX and graph_id.startswith(GENERAL_PREFIX): return graph_id return f"{user_id}_{graph_id}"
642-642: Avoid blocking the event loop with synchronous DB calls.Same issue as in
query_database:execute_sql_querywill block the event loop.Apply this diff:
- query_results = loader_class.execute_sql_query(sql_query, db_url) + query_results = await asyncio.to_thread( + loader_class.execute_sql_query, + sql_query, + db_url, + )
🧹 Nitpick comments (5)
api/core/text2sql.py (5)
119-121: Uselogging.exceptionfor automatic traceback capture.When logging errors within an exception handler,
logging.exceptionautomatically includes the stack trace.Apply this diff:
- logging.error("Failed to select graph %s: %s", sanitize_log_input(namespaced), e) + logging.exception("Failed to select graph %s", sanitize_log_input(namespaced))
148-149: Log exceptions in try-except-continue blocks.Silent exception swallowing makes debugging difficult. Consider logging at least at DEBUG level.
Example for line 148-149:
try: table_name, columns = row except Exception: # pylint: disable=broad-exception-caught + logging.debug("Failed to unpack table row: %s", row, exc_info=True) continueApply similar changes to lines 176-177 and 190-191.
Also applies to: 176-177, 190-191
500-500: Uselogging.exceptionfor automatic traceback capture.Apply this diff:
- logging.error("Error executing SQL query: %s", str(e)) # nosemgrep + logging.exception("Error executing SQL query") # nosemgrep
718-718: Uselogging.exceptionfor automatic traceback capture.Apply this diff:
- logging.error("Error executing confirmed SQL query: %s", str(e)) # nosemgrep + logging.exception("Error executing confirmed SQL query") # nosemgrep
799-799: Remove redundant exception object fromlogging.exception.
logging.exceptionautomatically includes exception info; passingeis redundant.Apply this diff:
- logging.exception("Failed to delete graph %s: %s", sanitize_log_input(namespaced), e) + logging.exception("Failed to delete graph %s", sanitize_log_input(namespaced))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/core/text2sql.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All Python code should pass pylint (use make lint)
Files:
api/core/text2sql.py
🧬 Code graph analysis (1)
api/core/text2sql.py (11)
api/core/errors.py (3)
GraphNotFoundError(8-9)InternalError(4-5)InvalidArgumentError(12-13)api/core/schema_loader.py (2)
load_database(98-141)generate(109-139)api/agents/analysis_agent.py (2)
AnalysisAgent(9-303)get_analysis(14-50)api/agents/relevancy_agent.py (2)
RelevancyAgent(69-93)get_answer(74-93)api/agents/response_formatter_agent.py (2)
ResponseFormatterAgent(42-135)format_response(49-75)api/agents/follow_up_agent.py (2)
FollowUpAgent(33-84)generate_follow_up_question(36-84)api/config.py (1)
Config(62-148)api/graph.py (2)
find(251-338)get_db_description(39-54)api/loaders/postgres_loader.py (1)
PostgresLoader(27-544)api/loaders/mysql_loader.py (1)
MySQLLoader(28-581)api/memory/graphiti_tool.py (6)
MemoryTool(47-718)create(64-75)search_memories(547-620)save_query_memory(281-366)add_new_memory(251-279)clean_memory(622-645)
🪛 Ruff (0.13.3)
api/core/text2sql.py
99-99: Avoid specifying long messages outside the exception class
(TRY003)
120-120: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
121-121: Avoid specifying long messages outside the exception class
(TRY003)
141-141: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
142-142: Avoid specifying long messages outside the exception class
(TRY003)
148-149: try-except-continue detected, consider logging the exception
(S112)
148-148: Do not catch blind exception: Exception
(BLE001)
176-177: try-except-continue detected, consider logging the exception
(S112)
176-176: Do not catch blind exception: Exception
(BLE001)
190-191: try-except-continue detected, consider logging the exception
(S112)
190-190: Do not catch blind exception: Exception
(BLE001)
215-215: Avoid specifying long messages outside the exception class
(TRY003)
218-218: Avoid specifying long messages outside the exception class
(TRY003)
497-497: Do not catch blind exception: Exception
(BLE001)
500-500: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
613-613: Avoid specifying long messages outside the exception class
(TRY003)
717-717: Do not catch blind exception: Exception
(BLE001)
718-718: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
731-733: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
762-762: Avoid specifying long messages outside the exception class
(TRY003)
769-769: Abstract raise to an inner function
(TRY301)
769-769: Avoid specifying long messages outside the exception class
(TRY003)
776-776: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
777-777: Avoid specifying long messages outside the exception class
(TRY003)
789-789: Avoid specifying long messages outside the exception class
(TRY003)
795-795: Consider moving this statement to an else block
(TRY300)
797-797: Avoid specifying long messages outside the exception class
(TRY003)
799-799: Redundant exception object included in logging.exception call
(TRY401)
800-800: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (6)
- GitHub Check: e2e-tests
- GitHub Check: unit-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: unit-tests
4 participants
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores
Documentation