Add MCP server #153
Add MCP server #153
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds the fastapi-mcp dependency and integrates FastApiMCP (mounts MCP HTTP endpoints, configurable via DISABLE_MCP); enables Bearer token parsing from the Authorization header for auth; adds operation_id metadata to several routes; tightens list typing to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant FastAPI_App as FastAPI App
participant MCP as FastApiMCP
participant Handlers as Operation Handlers
Note over FastAPI_App,MCP: App startup — MCP registered and mounted (unless DISABLE_MCP=true)
FastAPI_App->>MCP: FastApiMCP(name, description, include_operations)
MCP-->>FastAPI_App: mount_http() registers MCP routes
Note over Client,MCP: MCP request flow
Client->>MCP: HTTP call to MCP endpoint (e.g., list_databases)
MCP->>Handlers: Invoke operation handler
Handlers-->>MCP: Return result or error
MCP-->>Client: HTTP response (JSON)
sequenceDiagram
autonumber
actor Client
participant API as FastAPI Endpoint
participant Auth as validate_user
participant UserSvc as _get_user_info
Client->>API: Request (cookies/query and/or Authorization: Bearer <token>)
API->>Auth: validate_user(request)
rect rgba(220,235,255,0.25)
note right of Auth: Token sourcing (cookie, query param, Authorization header)
Auth->>Auth: Read cookie / query param
Auth->>Auth: Parse Authorization header for Bearer token
end
alt token found
Auth->>UserSvc: _get_user_info(token)
UserSvc-->>Auth: User info / failure
Auth-->>API: Authenticated / user context
else no token
Auth-->>API: Unauthenticated
end
API-->>Client: Proceed / 401
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Dependency ReviewThe following issues were found:
License IssuesPipfile
Pipfile.lock
OpenSSF ScorecardScorecard details
Scanned Files
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (14)
api/routes/database.py (1)
49-55: Fix Pylint raise-missing-from and preserve exception contextChain caught exceptions when converting to HTTPException.
Apply:
- except (ValueError, ConnectionError) as e: + except (ValueError, ConnectionError) as e: logging.error("PostgreSQL connection error: %s", str(e)) - raise HTTPException( + raise HTTPException( status_code=500, detail="Failed to connect to PostgreSQL database", - ) + ) from e ... - except (ValueError, ConnectionError) as e: + except (ValueError, ConnectionError) as e: logging.error("MySQL connection error: %s", str(e)) - raise HTTPException( - status_code=500, detail="Failed to connect to MySQL database" - ) + raise HTTPException( + status_code=500, detail="Failed to connect to MySQL database" + ) from e ... - except (ValueError, TypeError) as e: + except (ValueError, TypeError) as e: logging.error("Unexpected error in database connection: %s", str(e)) - raise HTTPException(status_code=500, detail="Internal server error") + raise HTTPException(status_code=500, detail="Internal server error") from eAlso applies to: 61-66, 86-89
api/auth/user_management.py (1)
76-76: Fix Pylint R0913/R0917 and line lengthEither refactor params into a dataclass or disable for this hotspot; also wrap the signature to <=100 chars.
-async def ensure_user_in_organizations(provider_user_id: str, email: str, name: str, provider: str, api_token: str, picture: str = None): +async def ensure_user_in_organizations( # pylint: disable=too-many-arguments, too-many-positional-arguments + provider_user_id: str, + email: str, + name: str, + provider: str, + api_token: str, + picture: str | None = None, +):If you prefer a cleaner API, I can provide a dataclass-based refactor.
api/agents/relevancy_agent.py (1)
61-69: Call base init, fix typing, and simplify initPrevents Pylint W0231 and aligns types with None handling.
- def __init__(self, queries_history: list[str], result_history: list[str]): + def __init__(self, queries_history: list[str], result_history: list[str] | None): """Initialize the relevancy agent with query and result history.""" - if result_history is None: - self.messages = [] - else: - self.messages = [] - for query, result in zip(queries_history[:-1], result_history): - self.messages.append({"role": "user", "content": query}) - self.messages.append({"role": "assistant", "content": result}) + super().__init__() + self.messages = [] + if result_history: + for query, result in zip(queries_history[:-1], result_history): + self.messages.append({"role": "user", "content": query}) + self.messages.append({"role": "assistant", "content": result})api/routes/graphs.py (11)
59-81: Fix pylint no-else-return in get_database_type_and_loaderSimplify the conditional chain to satisfy no-else-return and improve clarity.
- if db_url_lower.startswith('postgresql://') or db_url_lower.startswith('postgres://'): - return 'postgresql', PostgresLoader - elif db_url_lower.startswith('mysql://'): - return 'mysql', MySQLLoader - else: - # Default to PostgresLoader for backward compatibility - return 'postgresql', PostgresLoader + if db_url_lower.startswith('postgresql://') or db_url_lower.startswith('postgres://'): + return 'postgresql', PostgresLoader + if db_url_lower.startswith('mysql://'): + return 'mysql', MySQLLoader + # Default to PostgresLoader for backward compatibility + return 'postgresql', PostgresLoader
123-128: Narrow or explicitly suppress broad exceptCatching bare Exception trips pylint and hides actionable errors. If specific exceptions are unknown, add a scoped suppression.
- except Exception as e: + except Exception as e: # pylint: disable=broad-exception-caught logging.error("Failed to select graph %s: %s", sanitize_log_input(namespaced), e) return JSONResponse(content={"error": "Graph not found or database error"}, status_code=404)
144-148: Same: overly broad except on graph queries- except Exception as e: + except Exception as e: # pylint: disable=broad-exception-caught logging.error("Error querying graph data for %s: %s", sanitize_log_input(namespaced), e) return JSONResponse(content={"error": "Failed to read graph data"}, status_code=500)
152-156: Narrow the row-unpack except- except Exception: + except (ValueError, TypeError): continue
181-183: Narrow normalization except- except Exception: + except (ValueError, TypeError, KeyError): continue
194-197: Narrow link row except- except Exception: + except (ValueError, TypeError): continue
308-321: Remove unused variable db_type- db_type, loader_class = get_database_type_and_loader(db_url) + _, loader_class = get_database_type_and_loader(db_url)
526-536: Overly broad except during executionEither narrow expected errors or explicitly suppress.
- except Exception as e: + except Exception as e: # pylint: disable=broad-exception-caught overall_elapsed = time.perf_counter() - overall_start logging.error("Error executing SQL query: %s", str(e))
581-586: Remove unused variable db_type (confirm path)- db_type, loader_class = get_database_type_and_loader(db_url) + _, loader_class = get_database_type_and_loader(db_url)
658-663: Broad except inside confirmation flowSame rationale; narrow or suppress.
- except Exception as e: + except Exception as e: # pylint: disable=broad-exception-caught logging.error("Error executing confirmed SQL query: %s", str(e))
719-725: Broad except in refresh endpointNarrow or explicitly suppress to satisfy pylint.
- except Exception as e: + except Exception as e: # pylint: disable=broad-exception-caught logging.error("Error in manual schema refresh: %s", e)
🧹 Nitpick comments (15)
api/routes/database.py (2)
45-58: Broaden supported DSN schemes and simplify checksAccept common driver-qualified schemes.
- # Check for PostgreSQL URL - if url.startswith("postgres://") or url.startswith("postgresql://"): + # Check for PostgreSQL URL + if url.startswith(("postgres://", "postgresql://", "postgresql+psycopg://")): ... - # Check for MySQL URL - elif url.startswith("mysql://"): + # Check for MySQL URL + elif url.startswith(("mysql://", "mysql+pymysql://")):
82-85: Avoid logging potentially sensitive loader error detailsLoader errors may contain connection info. Log minimally or sanitize.
- logging.error("Database loader failed: %s", result) + logging.error("Database loader failed")api/auth/user_management.py (2)
21-24: Docstring mismatchThis helper fetches by token, not by email. Update to avoid confusion.
- Get user information from the database by email. + Get user information from the database by API token.
42-42: Resolve Pylint fixmeReplace TODO with a tracked issue reference or downgrade to NOTE to appease lint.
- # TODO delete invalid token from DB + # NOTE: delete invalid token from DB (track in issue #<id>)api/app_factory.py (3)
26-31: Silence Pylint R0903 on lightweight middlewareThis class intentionally has a single public method.
-class SecurityMiddleware(BaseHTTPMiddleware): +class SecurityMiddleware(BaseHTTPMiddleware): # pylint: disable=too-few-public-methods
99-107: Fix unused argument warning and remove TODO fixmeRename request to _request and avoid Pylint W0613; downgrade the TODO.
- async def handle_oauth_error(request: Request, exc: Exception): + async def handle_oauth_error(_request: Request, exc: Exception): @@ - # TODO check this scenario + # NOTE: handle common OAuth/token error scenarios
33-41: Static path hardening (optional)Consider normalizing with os.path.normpath and rejecting backslashes to harden traversal checks.
api/routes/graphs.py (8)
96-96: Docstring grammarFix “databases names” → “database names”.
- This route is used to list all the graphs (databases names) that are available in the database. + List all graphs (database names) available to the user.
108-116: Docstring style (Args casing and content)Use “Args:” and tighten wording.
- """Return all nodes and edges for the specified database schema (namespaced to the user). + """Return all nodes and edges for the specified database schema (namespaced to the user). @@ - - args: - graph_id (str): The ID of the graph to query (the database name). + + Args: + graph_id (str): Graph identifier (database name, without user prefix). """
268-276: Good: operation_id + clearer docstringMinor docstring nits below for consistency.
- Query the Database with the given graph_id and chat_data. + Query the database with the given graph_id and chat_data. @@ - Args: + Args: graph_id (str): The ID of the graph to query. chat_data (ChatRequest): The chat data containing user queries and context.
8-11: Avoid blocking the event loop on sync DB callsexecute_sql_query appears sync. Run it in a threadpool.
from fastapi import APIRouter, Request, HTTPException, UploadFile, File from fastapi.responses import JSONResponse, StreamingResponse +from fastapi.concurrency import run_in_threadpool from pydantic import BaseModel
454-455: Run blocking DB call off the event loop- query_results = loader_class.execute_sql_query(answer_an["sql_query"], db_url) + query_results = await run_in_threadpool( + loader_class.execute_sql_query, answer_an["sql_query"], db_url + )
519-550: Streaming media type mismatchThe payload is a delimiter-separated JSON stream, not a single JSON document. Use NDJSON (or SSE) content type.
- return StreamingResponse(generate(), media_type="application/json") + return StreamingResponse(generate(), media_type="application/x-ndjson")Confirm client compatibility before merging.
595-606: Run blocking DB call off the event loop (confirm path)- query_results = loader_class.execute_sql_query(sql_query, db_url) + query_results = await run_in_threadpool( + loader_class.execute_sql_query, sql_query, db_url + )
672-672: NDJSON content type for confirmation stream- return StreamingResponse(generate_confirmation(), media_type="application/json") + return StreamingResponse(generate_confirmation(), media_type="application/x-ndjson")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Pipfile.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Pipfile(1 hunks)api/agents/relevancy_agent.py(1 hunks)api/app_factory.py(3 hunks)api/auth/user_management.py(1 hunks)api/routes/database.py(1 hunks)api/routes/graphs.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{Pipfile,Pipfile.lock}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Pipenv for dependency management; add and lock dependencies via
PipfileandPipfile.lock
Files:
Pipfile
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to pylint standards across all Python files (repository uses
make lint)
Files:
api/auth/user_management.pyapi/routes/database.pyapi/agents/relevancy_agent.pyapi/routes/graphs.pyapi/app_factory.py
api/app_factory.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Maintain the application factory and OAuth setup in
api/app_factory.py(core configuration in the top of the file)
Files:
api/app_factory.py
🧬 Code graph analysis (2)
api/routes/graphs.py (1)
api/auth/user_management.py (1)
token_required(266-306)
api/app_factory.py (1)
api/auth/oauth_handlers.py (1)
setup_oauth_handlers(16-50)
🪛 GitHub Actions: Pylint
api/auth/user_management.py
[error] 76-76: C0301: Line too long (137/100) (line-too-long)
[error] 244-244: C0301: Line too long (102/100) (line-too-long)
[warning] 42-42: W0511: TODO delete invalid token from DB (fixme)
[warning] 51-51: W0718: Catching too general exception Exception (broad-exception-caught)
[warning] 72-72: W0718: Catching too general exception Exception (broad-exception-caught)
[error] 76-76: R0913: Too many arguments (6/5) (too-many-arguments)
[error] 76-76: R0917: Too many positional arguments (6/5) (too-many-positional-arguments)
api/routes/database.py
[error] 51-51: PyLint: Raise missing-from (raise-missing-from)
[error] 63-63: PyLint: Raise missing-from (raise-missing-from)
[error] 88-88: PyLint: Raise missing-from (raise-missing-from)
api/agents/relevancy_agent.py
[warning] 61-61: W0231: init method from base class 'BaseAgent' is not called (super-init-not-called)
api/routes/graphs.py
[error] 74-74: PyLint: Unnecessary "elif" after "return", remove the leading "el" from "elif" (no-else-return)
[error] 107-107: PyLint: Too many local variables (22/15) (too-many-locals)
[error] 125-125: PyLint: Catching too general exception Exception (broad-exception-caught)
[error] 146-146: PyLint: Catching too general exception Exception (broad-exception-caught)
[error] 154-154: PyLint: Catching too general exception Exception (broad-exception-caught)
[error] 182-182: PyLint: Catching too general exception Exception (broad-exception-caught)
[error] 196-196: PyLint: Catching too general exception Exception (broad-exception-caught)
[error] 107-107: PyLint: Too many branches (16/12) (too-many-branches)
[error] 107-107: PyLint: Too many statements (52/50) (too-many-statements)
[error] 318-318: PyLint: Unused variable 'db_type' (unused-variable)
[error] 270-270: PyLint: Too many statements (108/50) (too-many-statements)
[error] 302-302: PyLint: Too many local variables (30/15) (too-many-locals)
[error] 526-526: PyLint: Catching too general exception Exception (broad-exception-caught)
[error] 302-302: PyLint: Too many branches (18/12) (too-many-branches)
[error] 302-302: PyLint: Too many statements (94/50) (too-many-statements)
[error] 318-318: PyLint: Unused variable 'db_type' (unused-variable)
[error] 658-658: PyLint: Catching too general exception Exception (broad-exception-caught)
[error] 582-582: PyLint: Unused variable 'db_type' (unused-variable)
[error] 719-719: PyLint: Catching too general exception Exception (broad-exception-caught)
api/app_factory.py
[warning] 103-103: W0511: TODO check this scenario (fixme)
[error] 26-26: R0903: Too few public methods (1/2) (too-few-public-methods)
[warning] 100-100: W0613: Unused argument 'request' (unused-argument)
⏰ 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). (2)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (8)
Pipfile (2)
19-19: Dependency addition looks goodfastapi-mcp ~=0.4.0 aligns with the new MCP integration.
6-20: Dependency lock confirmed
fastapi-mcpis present inPipfile.lock; no further action required.api/routes/database.py (1)
24-24: Add operation_id: LGTMThis enables MCP discovery for the route.
api/auth/user_management.py (1)
51-53: Narrow broad Exception catches where feasibleCatching Exception hides real issues and triggers lint warnings. Prefer specific exceptions or add targeted disables with rationale.
I can scan callers to enumerate likely exception types and tailor the except clauses. Want a quick pass?
Also applies to: 72-74, 223-227, 299-304
api/app_factory.py (2)
88-95: MCP integration: LGTMFastApiMCP initialization + mount_http aligns with declared operation_ids.
Also applies to: 97-98
57-57: ProxyHeadersMiddleware: pass a sequence for trusted_hostsBe explicit to match expected type.
- app.add_middleware(ProxyHeadersMiddleware, trusted_hosts="*") + app.add_middleware(ProxyHeadersMiddleware, trusted_hosts=["*"])If you're targeting uvicorn 0.35.0, confirm the signature; I can adjust based on docs.
api/routes/graphs.py (2)
105-105: Good: operation_id addeddatabase_schema is a sensible, stable identifier.
92-92: Approve: No duplicate operation_id values found.
Ran a search for duplicateoperation_idvalues across the codebase; none detected.
| # If still not found, also accept Authorization: Bearer <token> | ||
| if not api_token: | ||
| auth_header = request.headers.get("authorization") or request.headers.get("Authorization") | ||
| if auth_header: | ||
| try: | ||
| parts = auth_header.split(None, 1) | ||
| if len(parts) == 2 and parts[0].lower() == "bearer": | ||
| api_token = parts[1].strip() | ||
| except Exception: | ||
| # If parsing fails, ignore and continue (will return unauthenticated) | ||
| api_token = None | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden Authorization parsing and fix line length
Guard empty tokens and wrap long lines.
- # If still not found, also accept Authorization: Bearer <token>
- if not api_token:
- auth_header = request.headers.get("authorization") or request.headers.get("Authorization")
- if auth_header:
- try:
- parts = auth_header.split(None, 1)
- if len(parts) == 2 and parts[0].lower() == "bearer":
- api_token = parts[1].strip()
- except Exception:
- # If parsing fails, ignore and continue (will return unauthenticated)
- api_token = None
+ # If still not found, also accept Authorization: Bearer <token>
+ if not api_token:
+ auth_header = (
+ request.headers.get("authorization")
+ or request.headers.get("Authorization")
+ )
+ if auth_header:
+ try:
+ parts = auth_header.split(None, 1)
+ if len(parts) == 2 and parts[0].lower() == "bearer":
+ token_candidate = parts[1].strip()
+ api_token = token_candidate if token_candidate else None
+ except Exception: # pylint: disable=broad-exception-caught
+ # Ignore parse errors; unauthenticated flow continues
+ api_token = None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # If still not found, also accept Authorization: Bearer <token> | |
| if not api_token: | |
| auth_header = request.headers.get("authorization") or request.headers.get("Authorization") | |
| if auth_header: | |
| try: | |
| parts = auth_header.split(None, 1) | |
| if len(parts) == 2 and parts[0].lower() == "bearer": | |
| api_token = parts[1].strip() | |
| except Exception: | |
| # If parsing fails, ignore and continue (will return unauthenticated) | |
| api_token = None | |
| # If still not found, also accept Authorization: Bearer <token> | |
| if not api_token: | |
| auth_header = ( | |
| request.headers.get("authorization") | |
| or request.headers.get("Authorization") | |
| ) | |
| if auth_header: | |
| try: | |
| parts = auth_header.split(None, 1) | |
| if len(parts) == 2 and parts[0].lower() == "bearer": | |
| token_candidate = parts[1].strip() | |
| api_token = token_candidate if token_candidate else None | |
| except Exception: # pylint: disable=broad-exception-caught | |
| # Ignore parse errors; unauthenticated flow continues | |
| api_token = None |
🧰 Tools
🪛 GitHub Actions: Pylint
[error] 244-244: C0301: Line too long (102/100) (line-too-long)
🤖 Prompt for AI Agents
In api/auth/user_management.py around lines 242 to 253, the Authorization header
parsing should be hardened and long lines wrapped: trim auth_header first, split
using auth_header.strip().split(None, 1) (or auth_header.strip().partition(" "))
and then verify there are exactly two parts and that the token part is non-empty
after .strip() before assigning api_token; remove the broad try/except (or
narrow it to AttributeError) so parsing failures surface, and break long
expressions into multiple short statements to satisfy line-length limits.
| chat: list[str] | ||
| result: list[str] = None | ||
| instructions: str = None | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Type Optional fields explicitly
result and instructions accept None; annotate them as optional to match the runtime.
class ChatRequest(BaseModel):
- chat: list[str]
- result: list[str] = None
- instructions: str = None
+ chat: list[str]
+ result: list[str] | None = None
+ instructions: str | None = NoneConfirm the project uses Python 3.10+ (for PEP 604 unions). If not, switch to typing.Optional[List[str]].
🏁 Script executed:
#!/bin/bash
rg -nC1 'python_version|python\s*=' Pipfile pyproject.toml || trueLength of output: 179
Type optional fields explicitly using PEP 604 union syntax
Since the project uses Python 3.12, replace the = None annotations with | None to accurately reflect Optional types:
class ChatRequest(BaseModel):
chat: list[str]
- result: list[str] = None
- instructions: str = None
+ result: list[str] | None = None
+ instructions: str | None = None📝 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.
| chat: list[str] | |
| result: list[str] = None | |
| instructions: str = None | |
| class ChatRequest(BaseModel): | |
| chat: list[str] | |
| - result: list[str] = None | |
| result: list[str] | None = None | |
| instructions: str | None = None |
🤖 Prompt for AI Agents
In api/routes/graphs.py around lines 43 to 46, the optional field annotations
use assignment-to-None style (e.g. `result: list[str] = None` and `instructions:
str = None`); update these to use PEP 604 union syntax so the types read
`result: list[str] | None = None` and `instructions: str | None = None` (leave
`chat: list[str]` unchanged) to correctly reflect Optional types on Python 3.12.
| @graphs_router.get("/{graph_id}/data") | ||
| @graphs_router.get("/{graph_id}/data", operation_id="database_schema") | ||
| @token_required | ||
| async def get_graph_data(request: Request, graph_id: str): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Temporary suppression for complexity; plan follow-up refactor
Function exceeds statements/branches/locals limits. Suppress now; split into helpers later (e.g., _fetch_graph_rows, _normalize_columns, _build_links).
-async def get_graph_data(request: Request, graph_id: str):
+async def get_graph_data(request: Request, graph_id: str): # pylint: disable=too-many-locals, too-many-branches, too-many-statementsI can propose a decomposition patch if you want a follow-up PR.
📝 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 get_graph_data(request: Request, graph_id: str): | |
| async def get_graph_data(request: Request, graph_id: str): # pylint: disable=too-many-locals, too-many-branches, too-many-statements |
🧰 Tools
🪛 GitHub Actions: Pylint
[error] 107-107: PyLint: Too many local variables (22/15) (too-many-locals)
[error] 107-107: PyLint: Too many branches (16/12) (too-many-branches)
[error] 107-107: PyLint: Too many statements (52/50) (too-many-statements)
🤖 Prompt for AI Agents
In api/routes/graphs.py around line 107, the get_graph_data function currently
exceeds complexity limits; temporarily add the appropriate linter suppression
(e.g., # noqa: C901 or # pylint:
disable=too-many-branches,too-many-locals,too-many-statements) immediately above
the function definition, and include a concise TODO comment referencing a
follow-up refactor to split logic into helpers (e.g., _fetch_graph_rows,
_normalize_columns, _build_links); keep behavior and tests unchanged.
| @graphs_router.post("/{graph_id}") | ||
| @graphs_router.post("/{graph_id}", operation_id="query_database") | ||
| @token_required | ||
| async def query_graph(request: Request, graph_id: str, chat_data: ChatRequest): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Temporary suppression for complexity; plan follow-up refactor
Streaming pipeline is large; suppress now and split into helpers (_stream_offtopic, _execute_and_format, _maybe_refresh_schema, etc.).
-async def query_graph(request: Request, graph_id: str, chat_data: ChatRequest):
+async def query_graph(request: Request, graph_id: str, chat_data: ChatRequest): # pylint: disable=too-many-statements, too-many-branches, too-many-localsI can submit a structured refactor patch next.
📝 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 query_graph(request: Request, graph_id: str, chat_data: ChatRequest): | |
| async def query_graph(request: Request, graph_id: str, chat_data: ChatRequest): # pylint: disable=too-many-statements, too-many-branches, too-many-locals |
🧰 Tools
🪛 GitHub Actions: Pylint
[error] 270-270: PyLint: Too many statements (108/50) (too-many-statements)
🤖 Prompt for AI Agents
In api/routes/graphs.py around line 270, the query_graph function's streaming
pipeline is currently a large, complex block; refactor by extracting logical
sections into small helper functions such as _stream_offtopic(request,
chat_data), _execute_and_format(request, graph_id, chat_data), and
_maybe_refresh_schema(request, graph_id) and invoke them from query_graph. Move
parsing/validation, DB/query execution, response formatting, and off-topic
streaming logic into separate helpers with clear inputs/outputs, keep
query_graph as the orchestrator only, and update imports/locals so each helper
receives required context (request, graph_id, chat_data) and returns simple
results/async generators to preserve streaming behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
api/app_factory.py (4)
26-26: Silence benign Pylint complaint on middleware class.Pylint flags too-few-public-methods for BaseHTTPMiddleware subclasses. Disable at declaration.
-class SecurityMiddleware(BaseHTTPMiddleware): +class SecurityMiddleware(BaseHTTPMiddleware): # pylint: disable=too-few-public-methods
31-41: Harden static path traversal check or rely on StaticFiles.Current check only catches '../' and trailing '/'. Prefer normalized checks to catch encoded traversal.
- filename = request.url.path[len(self.STATIC_PREFIX):] - # Basic security check for directory traversal - if not filename or '../' in filename or filename.endswith('/'): + filename = request.url.path[len(self.STATIC_PREFIX):] + # Basic traversal guard (works even with encoded sequences) + from pathlib import PurePosixPath + parts = PurePosixPath("/" + filename).parts + if not filename or any(p in ("..", "") for p in parts) or filename.endswith("/"): return JSONResponse(Note: Starlette’s StaticFiles already protects against traversal; consider deleting this block if redundant.
73-76: Use pathlib for static path resolution.Minor readability and cross-platform robustness.
- static_path = os.path.join(os.path.dirname(__file__), "../app/public") + from pathlib import Path + static_path = str((Path(__file__).parent / "../app/public").resolve())
105-119: Address Pylint warnings and narrow the OAuth error heuristic.
- Remove FIXMEs.
- Use the request arg (or explicitly disable the warning) to fix unused-argument.
- Keep HTTPException pass-through first.
@app.exception_handler(Exception) - async def handle_oauth_error(request: Request, exc: Exception): + async def handle_oauth_error(request: Request, exc: Exception): # pylint: disable=unused-argument """Handle OAuth-related errors gracefully""" - # Check if it's an OAuth-related error - # TODO check this scenario - if "token" in str(exc).lower() or "oauth" in str(exc).lower(): - logging.warning("OAuth error occurred: %s", exc) - return RedirectResponse(url="/", status_code=302) - - # If it's an HTTPException, re-raise so FastAPI handles it properly if isinstance(exc, HTTPException): raise exc + # Heuristic: redirect to home on likely OAuth/token errors + if "token" in str(exc).lower() or "oauth" in str(exc).lower(): + try: + logging.warning("OAuth-like error at %s: %s", request.url.path, exc) + except Exception: + logging.warning("OAuth-like error: %s", exc) + return RedirectResponse(url="/", status_code=302) # For other errors, let them bubble up raise excIf you can import a specific OAuth exception type, prefer isinstance checks over substring matching.
README.md (5)
133-136: Fix unordered list indentation (markdownlint MD007).Indent list items by 2 spaces.
- - `list_databases` - - `connect_database` - - `database_schema` - - `query_database` + - `list_databases` + - `connect_database` + - `database_schema` + - `query_database`
138-143: Tighten grammar and structure; add a proper subsection header.Replace the dangling "- Configuration" line, clarify default wording.
-- Configuration - -- `DISABLE_MCP` — disable QueryWeaver's built-in MCP HTTP surface. Set to `true` to disable. Default: `false` (MCP enabled). +### Configuration + +- `DISABLE_MCP` — disable QueryWeaver's built-in MCP HTTP surface. Set to `true` to disable. Default: `false` (enabled).
150-153: Provide a concrete HTTP example or remove the placeholder sentence.Add a curl example that matches the mounted base path.
-Calling the built-in MCP endpoints (example) -- The MCP surface is exposed as HTTP endpoints. - +Calling the built-in MCP endpoints (example) + +The MCP surface is exposed under `/mcp`. For example, to list operations: + +```bash +curl -s -H "Authorization: Bearer $API_TOKEN" \ + http://127.0.0.1:5000/mcp/operations +``` +Please verify the exact path created by mcp.mount_http(...) and adjust if your fastapi-mcp version differs.
154-170: Minor copyedits and consistency.
- Use consistent phrasing (“Server configuration”).
- Keep JSON minimal but valid.
-### Server Configuration +### Server configuration Below is a minimal example `mcp.json` client configuration that targets a local QueryWeaver instance exposing the MCP HTTP surface at `/mcp`.
127-170: Resolve spellcheck failures by whitelisting project-specific terms.pyspelling likely flags QueryWeaver/Text2SQL/FalkorDB/OAuth/etc. Add them to your dictionary instead of “fixing” proper nouns.
Proposed additions (e.g., in docs/spellings.txt or .pyspelling config wordlist):
- QueryWeaver
- Text2SQL
- FalkorDB
- OAuth
- GitHub
- Playwright
- OpenAPI
- MCP
- mcp.json
I can open a follow-up PR to adjust the pyspelling config if you share its path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.env.example(1 hunks)README.md(1 hunks)api/app_factory.py(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- .env.example
🧰 Additional context used
📓 Path-based instructions (2)
api/app_factory.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Maintain the application factory and OAuth setup in
api/app_factory.py(core configuration in the top of the file)
Files:
api/app_factory.py
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to pylint standards across all Python files (repository uses
make lint)
Files:
api/app_factory.py
🧬 Code graph analysis (1)
api/app_factory.py (1)
api/auth/oauth_handlers.py (1)
setup_oauth_handlers(16-50)
🪛 LanguageTool
README.md
[grammar] ~138-~138: There might be a mistake here.
Context: ...or environment (default: MCP enabled). - Configuration -DISABLE_MCP` — disable...
(QB_NEW_EN)
[grammar] ~150-~150: There might be a mistake here.
Context: ...ing the built-in MCP endpoints (example) - The MCP surface is exposed as HTTP endpo...
(QB_NEW_EN)
[grammar] ~154-~154: There might be a mistake here.
Context: ...P endpoints. ### Server Configuration Below is a minimal example mcp.json cl...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
README.md
133-133: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
134-134: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
135-135: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
136-136: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
🪛 GitHub Actions: Spellcheck
README.md
[error] 1-1: Spellcheck failed: Spelling errors detected in README.md during Markdown spellcheck (pyspelling).
🪛 GitHub Actions: Pylint
api/app_factory.py
[warning] 109-109: Pylint: TODO check this scenario (fixme)
[warning] 26-26: Pylint: Too few public methods (1/2) (too-few-public-methods)
[warning] 106-106: Pylint: Unused argument 'request' (unused-argument)
⏰ 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). (2)
- GitHub Check: unit-tests
- GitHub Check: e2e-tests
| app.add_middleware(ProxyHeadersMiddleware, trusted_hosts="*") | ||
| ) | ||
|
|
||
| app.add_middleware(ProxyHeadersMiddleware, trusted_hosts="*") |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Don't trust all proxies; fix ProxyHeadersMiddleware config.
Passing trusted_hosts="*" lets any upstream spoof X-Forwarded headers. Restrict to known proxies or make it configurable via env. Also pass a list, not a raw string.
- app.add_middleware(ProxyHeadersMiddleware, trusted_hosts="*")
+ trusted_proxies = os.getenv("PROXY_TRUSTED_IPS", "127.0.0.1,::1")
+ app.add_middleware(
+ ProxyHeadersMiddleware,
+ trusted_hosts=[ip.strip() for ip in trusted_proxies.split(",") if ip.strip()]
+ )If you actually need to trust all proxies in a controlled environment, make it explicit in .env (PROXY_TRUSTED_IPS=*), not hardcoded.
🏁 Script executed:
#!/bin/bash
python3 - << 'EOF'
from uvicorn.middleware.proxy_headers import ProxyHeadersMiddleware
import inspect
print(inspect.getsource(ProxyHeadersMiddleware.__init__))
EOFLength of output: 131
🏁 Script executed:
#!/bin/bash
set -e
pip install starlette
python3 - << 'EOF'
import inspect
from starlette.middleware.proxy_headers import ProxyHeadersMiddleware
print(inspect.signature(ProxyHeadersMiddleware.__init__))
EOFLength of output: 1041
🏁 Script executed:
#!/bin/bash
set -e
pip install uvicorn
python3 - << 'EOF'
import inspect
from uvicorn.middleware.proxy_headers import ProxyHeadersMiddleware
print("Constructor signature:", inspect.signature(ProxyHeadersMiddleware.__init__))
print("Docstring:", ProxyHeadersMiddleware.__doc__)
EOF
rg -nP "ProxyHeadersMiddleware" -C3 .Length of output: 2321
🏁 Script executed:
#!/bin/bash
set -e
pip install uvicorn
python3 - << 'EOF'
import inspect
from uvicorn.middleware.proxy_headers import ProxyHeadersMiddleware
print(inspect.getsource(ProxyHeadersMiddleware))
EOFLength of output: 2827
🏁 Script executed:
#!/bin/bash
set -e
pip install uvicorn
python3 - << 'EOF'
import inspect
from uvicorn.middleware.proxy_headers import _TrustedHosts
print(inspect.getsource(_TrustedHosts))
EOFLength of output: 3609
Restrict ProxyHeadersMiddleware’s trusted_hosts via env—remove hardcoded "*"
Replace the middleware call at api/app_factory.py:57:
- app.add_middleware(ProxyHeadersMiddleware, trusted_hosts="*")
+ trusted_ips = os.getenv("PROXY_TRUSTED_IPS", "127.0.0.1,::1")
+ app.add_middleware(
+ ProxyHeadersMiddleware,
+ trusted_hosts=[ip.strip() for ip in trusted_ips.split(",") if ip.strip()],
+ )If you truly need to trust all proxies, set PROXY_TRUSTED_IPS="*" in your environment—don’t hardcode it.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In api/app_factory.py around line 57, the ProxyHeadersMiddleware call currently
hardcodes trusted_hosts="*"; change it to read PROXY_TRUSTED_IPS from the
environment (os.getenv("PROXY_TRUSTED_IPS")) and use that value instead: if the
env var is "*" pass "*" to trusted_hosts, otherwise split the comma-separated
string into a list of stripped host/IP entries and pass that list (or None if
the env var is empty) to ProxyHeadersMiddleware; ensure you import os and handle
empty/missing env gracefully so the default behavior is not to trust all
proxies.
| # Control MCP endpoints via environment variable DISABLE_MCP | ||
| # Default: MCP is enabled unless DISABLE_MCP is set to true | ||
| disable_mcp = os.getenv("DISABLE_MCP", "false").lower() in ("1", "true", "yes") | ||
| if disable_mcp: | ||
| logging.info("MCP endpoints disabled via DISABLE_MCP environment variable") | ||
| else: | ||
| mcp = FastApiMCP(app, | ||
| name="queryweaver", | ||
| description="QueryWeaver MCP API, provides Text2SQL capabilities", | ||
| include_operations=["list_databases", | ||
| "connect_database", | ||
| "database_schema", | ||
| "query_database"] | ||
| ) | ||
|
|
||
| mcp.mount_http() | ||
|
|
||
| @app.exception_handler(Exception) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Mount MCP under an explicit base path to match docs and avoid surprises.
README points clients to /mcp, but mount_http() is called with no base path. Mount explicitly and keep docs consistent.
- mcp.mount_http()
+ # Mount under explicit base path to match README and client examples
+ mcp.mount_http("/mcp")Also verify the operation_id strings exist on the registered routes.
🏁 Script executed:
#!/bin/bash
# Verify operation_ids exist (expect 4 matches).
rg -nP 'operation_id\s*=\s*"(list_databases|connect_database|database_schema|query_database)"' -g 'api/**/*.py' -C1Length of output: 756
Mount MCP under explicit /mcp base path
Change the call to include “/mcp”, matching the README and client examples:
- mcp.mount_http()
+ mcp.mount_http("/mcp")Operation IDs for list_databases, connect_database, database_schema, and query_database are already present in api/routes (no changes needed).
📝 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.
| # Control MCP endpoints via environment variable DISABLE_MCP | |
| # Default: MCP is enabled unless DISABLE_MCP is set to true | |
| disable_mcp = os.getenv("DISABLE_MCP", "false").lower() in ("1", "true", "yes") | |
| if disable_mcp: | |
| logging.info("MCP endpoints disabled via DISABLE_MCP environment variable") | |
| else: | |
| mcp = FastApiMCP(app, | |
| name="queryweaver", | |
| description="QueryWeaver MCP API, provides Text2SQL capabilities", | |
| include_operations=["list_databases", | |
| "connect_database", | |
| "database_schema", | |
| "query_database"] | |
| ) | |
| mcp.mount_http() | |
| @app.exception_handler(Exception) | |
| # Control MCP endpoints via environment variable DISABLE_MCP | |
| # Default: MCP is enabled unless DISABLE_MCP is set to true | |
| disable_mcp = os.getenv("DISABLE_MCP", "false").lower() in ("1", "true", "yes") | |
| if disable_mcp: | |
| logging.info("MCP endpoints disabled via DISABLE_MCP environment variable") | |
| else: | |
| mcp = FastApiMCP(app, | |
| name="queryweaver", | |
| description="QueryWeaver MCP API, provides Text2SQL capabilities", | |
| include_operations=["list_databases", | |
| "connect_database", | |
| "database_schema", | |
| "query_database"] | |
| ) | |
| mcp.mount_http("/mcp") | |
| @app.exception_handler(Exception) |
🤖 Prompt for AI Agents
In api/app_factory.py around lines 88 to 105, the MCP is mounted at the app root
but should be mounted under the explicit "/mcp" base path per README and client
examples; update the FastApiMCP creation/mount to include the base path (for
example pass base_path="/mcp" or set the mount path argument) so the MCP
endpoints are served under /mcp (e.g., /mcp/list_databases,
/mcp/connect_database, etc.), then call mount_http as before.
1 participant
Summary by CodeRabbit