Conversation
fix docker with fastapi
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughSwitches runtime from Flask to FastAPI/uvicorn, updates env variable names (FLASK_* → FASTAPI_*), adjusts OAuth handlers, adds validation in database routes, restructures Dockerfile to multi-stage with Node.js for frontend builds, updates ESLint config, refreshes templates/CSS, bumps select dependencies, and aligns CI/docs/tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Container
participant S as start.sh
participant U as Uvicorn
participant A as FastAPI app (api.index:app)
participant DB as FalkorDB
C->>S: Entry (env: FALKORDB_HOST/PORT, FASTAPI_DEBUG)
S->>DB: Wait for readiness
DB-->>S: Ready
S->>U: Launch uvicorn api.index:app (--reload if FASTAPI_DEBUG=true)
U->>A: Serve HTTP on 0.0.0.0:5000
sequenceDiagram
autonumber
participant User as Browser
participant A as FastAPI app
participant OA as OAuth Provider
participant Auth as oauth_handlers
participant UM as User Mgmt
User->>A: OAuth callback (Google/GitHub)
A->>Auth: Invoke provider-specific handler
Auth->>Auth: Validate user_id and email
alt Missing fields
Auth-->>A: Return False / error log
A-->>User: 400/redirect as defined
else Valid
Auth->>UM: ensure_user_in_organizations(...)
UM-->>Auth: OK
Auth-->>A: Success
A-->>User: Redirect to app
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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 Scorecard
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
start.sh (1)
9-11: Start FalkorDB only when targeting localhost to avoid accidental double-database launchesRight now we always start a local FalkorDB even if FALKORDB_HOST points to a remote service. This can cause port conflicts or unnecessary in-container DBs in Compose/K8s. Guard the launch so we only start the server when HOST is localhost/127.0.0.1.
Apply:
-# Start FalkorDB Redis server in background -redis-server --loadmodule /var/lib/falkordb/bin/falkordb.so & +# Start FalkorDB locally only when targeting localhost +if [ "$FALKORDB_HOST" = "localhost" ] || [ "$FALKORDB_HOST" = "127.0.0.1" ]; then + # Start FalkorDB Redis server in background + redis-server --loadmodule /var/lib/falkordb/bin/falkordb.so & +else + echo "Skipping local FalkorDB start (FALKORDB_HOST=$FALKORDB_HOST)." +fi
🧹 Nitpick comments (5)
start.sh (2)
15-17: Add a timeout and clearer diagnostics to the readiness loopAn infinite wait can hang deployments. Add a bounded timeout and emit useful logs before exiting non-zero.
Apply:
-while ! nc -z "$FALKORDB_HOST" "$FALKORDB_PORT"; do - sleep 0.5 -done +TIMEOUT_SECONDS="${FALKORDB_WAIT_TIMEOUT:-60}" +DEADLINE=$(( $(date +%s) + TIMEOUT_SECONDS )) +until nc -z "$FALKORDB_HOST" "$FALKORDB_PORT"; do + if [ "$(date +%s)" -ge "$DEADLINE" ]; then + echo "Timed out after ${TIMEOUT_SECONDS}s waiting for $FALKORDB_HOST:$FALKORDB_PORT" >&2 + exit 1 + fi + sleep 0.5 +done
20-27: Harden reload detection and remove duplicated banner
- Normalize FLASK_DEBUG handling to accept true/1/yes (case-insensitive).
- Drop the duplicate "FalkorDB is up" banner; keep a single consistent message.
Apply:
-echo "FalkorDB is up - launching FastAPI..." -# Determine whether to run in reload (debug) mode. The project uses FLASK_DEBUG -# environment variable historically; keep compatibility by honoring it here. -if [ "${FLASK_DEBUG:-False}" = "True" ] || [ "${FLASK_DEBUG:-true}" = "true" ]; then - RELOAD_FLAG="--reload" -else - RELOAD_FLAG="" -fi +# Determine whether to run in reload (debug) mode. Preserve FLASK_DEBUG for compat. +case "${FLASK_DEBUG:-false}" in + [Tt][Rr][Uu][Ee]|1|[Yy][Ee][Ss]) RELOAD_FLAG="--reload" ;; + *) RELOAD_FLAG="" ;; +esacDockerfile (3)
1-2: Pin base images to immutable tags/digests for reproducibilityFROM python:3.12-bookworm and FROM falkordb/falkordb:latest are floating. Pin to explicit patch versions or digests to avoid surprise rebuilds.
Example:
-FROM python:3.12-bookworm as python-base +FROM python:3.12.5-bookworm@sha256:<digest> as python-base ... -FROM falkordb/falkordb:latest +FROM falkordb/falkordb:3.6.0@sha256:<digest>
13-15: Copying entire /usr/local increases image size and risk of lib conflictsCopying all of /usr/local brings along build tools and unused artifacts. Consider copying just Python runtime bits or installing python3 via apt in the final image if compatible.
Slimmer alternative:
-COPY --from=python-base /usr/local /usr/local +# Minimal Python runtime (adjust paths if needed) +COPY --from=python-base /usr/local/bin/python3.12 /usr/local/bin/python3.12 +COPY --from=python-base /usr/local/lib/python3.12 /usr/local/lib/python3.12
40-52: Frontend install/cache flow looks good; consider pruning dev deps post-buildYou’re correctly using npm ci when a lockfile exists. If dev deps aren’t needed at runtime, prune after building assets to reduce size.
Example follow-up (after assets are built):
RUN npm --prefix ./app run build && npm --prefix ./app prune --omit=dev
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
Dockerfile(3 hunks)start.sh(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
Dockerfile
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Maintain container configuration in Dockerfile supporting app runtime
Files:
Dockerfile
🧠 Learnings (1)
📚 Learning: 2025-08-19T08:18:37.470Z
Learnt from: CR
PR: FalkorDB/QueryWeaver#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-19T08:18:37.470Z
Learning: Applies to Dockerfile : Maintain container configuration in Dockerfile supporting app runtime
Applied to files:
Dockerfile
⏰ 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). (1)
- GitHub Check: e2e-tests
🔇 Additional comments (1)
Dockerfile (1)
60-64: ENTRYPOINT switch overrides FalkorDB base behavior; confirm intent and exposed ports
- ENTRYPOINT /start.sh replaces any upstream entrypoint from falkordb/falkordb. Confirm no base initialization is required.
- EXPOSE 3000 is only needed if a dev server runs in-container; otherwise omit to reduce port surface.
If the base image had an entrypoint you need to preserve, consider:
ENTRYPOINT ["/usr/local/bin/docker-entrypoint.sh"] CMD ["/start.sh"]Or wrap base entrypoint within start.sh if needed.
| RUN apt-get update && apt-get install -y \ | ||
| python3 \ | ||
| python3-pip \ | ||
| netcat-openbsd \ | ||
| && rm -rf /var/lib/apt/lists/* | ||
| && rm -rf /var/lib/apt/lists/* \ | ||
| && ln -sf /usr/local/bin/python3.12 /usr/bin/python3 \ | ||
| && ln -sf /usr/local/bin/python3.12 /usr/bin/python | ||
|
|
There was a problem hiding this comment.
Install required tooling in one layer; add missing essentials and tighten apt flags
We use curl later but it’s not installed. Also add ca-certificates and gnupg for NodeSource, and avoid recommends to keep the image smaller. Create python symlinks after installing.
Apply:
-RUN apt-get update && apt-get install -y \
- netcat-openbsd \
- && rm -rf /var/lib/apt/lists/* \
+RUN apt-get update && apt-get install -y --no-install-recommends \
+ netcat-openbsd curl ca-certificates gnupg \
+ && rm -rf /var/lib/apt/lists/* \
&& ln -sf /usr/local/bin/python3.12 /usr/bin/python3 \
&& ln -sf /usr/local/bin/python3.12 /usr/bin/python📝 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.
| RUN apt-get update && apt-get install -y \ | |
| python3 \ | |
| python3-pip \ | |
| netcat-openbsd \ | |
| && rm -rf /var/lib/apt/lists/* | |
| && rm -rf /var/lib/apt/lists/* \ | |
| && ln -sf /usr/local/bin/python3.12 /usr/bin/python3 \ | |
| && ln -sf /usr/local/bin/python3.12 /usr/bin/python | |
| RUN apt-get update && apt-get install -y --no-install-recommends \ | |
| netcat-openbsd curl ca-certificates gnupg \ | |
| && rm -rf /var/lib/apt/lists/* \ | |
| && ln -sf /usr/local/bin/python3.12 /usr/bin/python3 \ | |
| && ln -sf /usr/local/bin/python3.12 /usr/bin/python |
🤖 Prompt for AI Agents
In Dockerfile around lines 17 to 22, the RUN instruction installs only
netcat-openbsd and then creates python symlinks, but it is missing curl,
ca-certificates and gnupg (needed for NodeSource), and it should avoid
installing recommends and create symlinks after packages are installed; update
the single RUN layer to apt-get update && apt-get install -y
--no-install-recommends curl ca-certificates gnupg netcat-openbsd (and any other
required essentials), remove apt lists after install, and then create the python
symlinks (ln -sf ...) in the same layer so the image stays small and has the
necessary tooling.
| # Use NodeSource setup script to get a recent Node version on Debian-based images. | ||
| RUN curl -fsSL https://deb.nodesource.com/setup_22.x | bash - \ | ||
| && apt-get update && apt-get install -y nodejs \ | ||
| && rm -rf /var/lib/apt/lists/* | ||
|
|
||
| # Copy only frontend package files so Docker can cache npm installs when |
There was a problem hiding this comment.
🛠️ Refactor suggestion
NodeSource setup lacks curl/gnupg/CA packages and repeats apt metadata
The step will fail on images without curl/gnupg. Also collapse updates into one and use --no-install-recommends.
Apply:
-RUN curl -fsSL https://deb.nodesource.com/setup_22.x | bash - \
- && apt-get update && apt-get install -y nodejs \
- && rm -rf /var/lib/apt/lists/*
+RUN curl -fsSL https://deb.nodesource.com/setup_22.x | bash - \
+ && apt-get update \
+ && apt-get install -y --no-install-recommends nodejs \
+ && rm -rf /var/lib/apt/lists/*Note: This assumes curl/gnupg/ca-certificates were installed earlier (see previous comment). If you prefer fully self-contained step, include them here instead.
📝 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.
| # Install Node.js (Node 22) so we can build the frontend inside the image. | |
| # Use NodeSource setup script to get a recent Node version on Debian-based images. | |
| RUN curl -fsSL https://deb.nodesource.com/setup_22.x | bash - \ | |
| && apt-get update && apt-get install -y nodejs \ | |
| && rm -rf /var/lib/apt/lists/* | |
| # Install Node.js (Node 22) so we can build the frontend inside the image. | |
| # Use NodeSource setup script to get a recent Node version on Debian-based images. | |
| RUN curl -fsSL https://deb.nodesource.com/setup_22.x | bash - \ | |
| && apt-get update \ | |
| && apt-get install -y --no-install-recommends nodejs \ | |
| && rm -rf /var/lib/apt/lists/* |
🤖 Prompt for AI Agents
In Dockerfile around lines 34 to 39, the NodeSource setup step assumes
curl/gnupg/ca-certificates exist and runs apt-get update twice; modify the step
to ensure required packages (curl, gnupg, ca-certificates) are installed if not
already, collapse apt-get update/install into a single apt-get update && apt-get
install -y --no-install-recommends <packages> sequence, then run the NodeSource
setup script and install nodejs, and finally rm -rf /var/lib/apt/lists/* to
clean up; alternatively if those packages are guaranteed earlier, remove the
redundant apt-get update and add --no-install-recommends to the nodejs install
line and keep apt lists cleanup.
| fi | ||
|
|
||
| echo "FalkorDB is up - launching FastAPI (uvicorn)..." | ||
| exec uvicorn api.index:app --host 0.0.0.0 --port 5000 $RELOAD_FLAG No newline at end of file |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure uvicorn reloader dependencies and module path exist
- uvicorn --reload requires watchfiles (or standard extras). Verify Pipfile includes uvicorn[standard] or watchfiles to avoid runtime errors.
- Verify api/index.py exports app.
Run from repo root:
🏁 Script executed:
#!/bin/bash
# Check FastAPI app and uvicorn deps
rg -nP '^\s*app\s*=\s*FastAPI\(' -C2 --glob 'api/**.py'
rg -nP '^\s*from\s+fastapi\s+import\s+FastAPI' --glob 'api/index.py'
rg -nP '^\s*uvicorn(\[standard\])?' Pipfile Pipfile.lock || rg -n 'watchfiles' Pipfile Pipfile.lockLength of output: 538
🏁 Script executed:
#!/bin/bash
# Verify that api/index.py exports an app instance
rg -nP '^\s*app\s*=' --glob api/index.py
rg -n 'create_app' --glob api/index.pyLength of output: 203
Add Uvicorn reload dependencies to Pipfile
- Verified that
api/index.pycorrectly exportsappviacreate_app(); no changes needed there. - However, the
Pipfilecurrently has:This does not include the extras required for[packages] uvicorn = "~=0.32.0"
--reload. Withoutuvicorn[standard]or thewatchfilespackage, using--reloadwill fail at runtime.
Action Items:
- In
Pipfile, update theuvicornentry to include thestandardextras (or separately addwatchfiles):[packages] - uvicorn = "~=0.32.0" + uvicorn = {version = "~=0.32.0", extras = ["standard"]}
- Run
pipenv lockto updatePipfile.lock.
🤖 Prompt for AI Agents
In start.sh around line 30 the script runs uvicorn with potential live reload
via $RELOAD_FLAG but the project's Pipfile only lists uvicorn without extras;
update the Pipfile to specify uvicorn with the standard extras
(uvicorn[standard] = "~=0.32.0") or add watchfiles to [packages] so --reload
works, then run pipenv lock to regenerate Pipfile.lock.
clean flask
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/app_factory.py (1)
55-62: Make session cookie secure in non-debug; derive from FASTAPI_DEBUG.Right now
https_only=Falsesets an insecure session cookie even in production. Tie this to FASTAPI_DEBUG so production defaults to secure cookies and local dev stays convenient.Apply this minimal, self-contained change within the existing block:
app.add_middleware( SessionMiddleware, secret_key=secret_key, session_cookie="qw_session", same_site="lax", # allow top-level OAuth GET redirects to send cookies - https_only=False, # allow http on localhost in development + https_only=os.getenv("FASTAPI_DEBUG", "False").lower() != "true", # secure in prod, relaxed in dev max_age=60 * 60 * 24 * 14, # 14 days - measured by seconds )Follow-up: consider also setting
domain/pathexplicitly if you’ll front this with a different host in production.
🧹 Nitpick comments (12)
app/README.md (1)
12-16: Clarify static bundle path and FastAPI static mount to avoid 404s.“FASTAPI templates should load via /static/dist/bundle.js” assumes StaticFiles is mounted from api/static at /static. Please confirm api/index.py mounts StaticFiles accordingly, otherwise the path will 404. Also, this file states the bundle outputs to api/static/dist/bundle.js, while .github/copilot-instructions.md currently mentions app/public/js/app.js—please unify to one canonical path.
Example FastAPI static mount (for docs):
from fastapi import FastAPI from fastapi.staticfiles import StaticFiles app = FastAPI() app.mount("/static", StaticFiles(directory="api/static"), name="static").github/copilot-instructions.md (1)
129-151: Env var rename to FASTAPI_ looks good; harmonize frontend bundle path elsewhere in this doc.*The FASTAPI_SECRET_KEY/FASTAPI_DEBUG updates are consistent with the migration. One lingering inconsistency: earlier in this doc the build output is documented as app/public/js/app.js, but app/README.md documents api/static/dist/bundle.js and templates loading via /static/dist/bundle.js. Recommend standardizing to the latter to match “api/static/” as the static root.
Proposed wording for the earlier “build output” sentence:
After make install, run make build-prod (or cd app && npm run build) to compile the TypeScript into the static bundle (build output: api/static/dist/bundle.js), which templates load via /static/dist/bundle.js.Pipfile (1)
25-25: pytest-asyncio 1.1.0: opt into modern asyncio mode to prevent fixture errors.If your tests rely on the legacy event loop policy, you may see “asyncio mode is strict” errors. Consider adding asyncio_mode=auto in pytest.ini to smooth the transition.
Suggested pytest.ini snippet (outside this file):
[pytest] asyncio_mode = autotests/e2e/README.md (1)
145-151: Update CI reference to include the dedicated e2e workflow.Docs point to .github/workflows/tests.yml only. Given the dedicated E2E workflow now exists at .github/workflows/e2e-tests.yml, reference both or the specific one used for E2E.
Suggested wording:
The E2E tests run automatically in CI via .github/workflows/e2e-tests.yml (dedicated E2E) and may also be invoked from .github/workflows/tests.yml depending on the pipeline configuration.api/app_factory.py (4)
49-53: Long log line trips pylint; wrap message.Pylint flagged line length (102/100). Wrap the warning string to keep CI clean.
- logging.warning("FASTAPI_SECRET_KEY not set, using generated key. Set this in production!") + logging.warning( + "FASTAPI_SECRET_KEY not set, using generated key. " + "Set this in production!" + )
23-41: Pylint: too-few-public-methods on SecurityMiddleware — silence or switch to function-based middleware.This is a single-responsibility middleware with one public method; either disable the warning on this class or convert it to the
@app.middleware("http")style to avoid the class-level check.Option A (disable on class):
-class SecurityMiddleware(BaseHTTPMiddleware): +class SecurityMiddleware(BaseHTTPMiddleware): # pylint: disable=too-few-public-methodsOption B (function-based): Move the static path checks into a function-based
@app.middleware("http")and drop the class.
28-38: Harden static-file traversal check against encoded payloads or odd paths.
'../' in filenamewon’t catch URL-encoded traversal like%2e%2e/. Starlette’sStaticFilesalready normalizes paths, but since you’ve added a pre-check, consider decoding and normalizing before validating.- filename = request.url.path[len(self.STATIC_PREFIX):] + from urllib.parse import unquote + filename = unquote(request.url.path[len(self.STATIC_PREFIX):]) + # Normalize separators and remove '..' segments + import posixpath + normalized = posixpath.normpath('/' + filename).lstrip('/') - if not filename or '../' in filename or filename.endswith('/'): + if not normalized or normalized.endswith('/') or normalized.startswith('../'): return JSONResponse( status_code=403, content={"detail": "Forbidden"} )Alternatively, rely solely on
StaticFilesand drop this pre-check.
68-71: Use an absolute static path for clarity and robustness.The current
../app/publicjoin works but is easier to reason about when normalized to an absolute path.- static_path = os.path.join(os.path.dirname(__file__), "../app/public") + static_path = os.path.abspath( + os.path.join(os.path.dirname(__file__), "..", "app", "public") + )api/index.py (2)
10-10: Trim trailing whitespace to satisfy pylint.The blank line at Line 10 contains trailing spaces; remove them to clear the lint warning.
13-19: Optional: make host/port configurable via env for parity with Docker/start.sh.Not required, but it can reduce divergence between local and container runs.
- uvicorn.run( - "api.index:app", - host="127.0.0.1", - port=5000, + uvicorn.run( + "api.index:app", + host=os.getenv("HOST", "127.0.0.1"), + port=int(os.getenv("PORT", "5000")), reload=debug_mode, log_level="info" if debug_mode else "warning" ).github/workflows/tests.yml (1)
45-49: Conditionals usingexists('app/package.json')are non-standard; prefer hashFiles.GitHub Actions expressions don’t provide an
exists()function. UsehashFiles()to gate frontend steps on the presence ofapp/package.json.- - name: Install frontend dependencies - if: "exists('app/package.json')" + - name: Install frontend dependencies + if: ${{ hashFiles('app/package.json') != '' }} @@ - - name: Run frontend lint - if: "exists('app/package.json')" + - name: Run frontend lint + if: ${{ hashFiles('app/package.json') != '' }}Also applies to: 66-69
README.md (1)
98-106: Environment variables snippet: include FASTAPI_DEBUG for discoverability.Add
FASTAPI_DEBUGto the Docker example to mirror local dev and CI.docker run -p 5000:5000 -it \ -e FASTAPI_SECRET_KEY=your_super_secret_key_here \ + -e FASTAPI_DEBUG=False \ -e GOOGLE_CLIENT_ID=your_google_client_id \ -e GOOGLE_CLIENT_SECRET=your_google_client_secret \ -e GITHUB_CLIENT_ID=your_github_client_id \ -e GITHUB_CLIENT_SECRET=your_github_client_secret \ -e AZURE_API_KEY=your_azure_api_key \ falkordb/queryweaver
📜 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 (12)
.env.example(1 hunks).github/copilot-instructions.md(2 hunks).github/workflows/e2e-tests.yml(1 hunks).github/workflows/tests.yml(2 hunks)Pipfile(2 hunks)README.md(2 hunks)api/app_factory.py(1 hunks)api/auth/user_management.py(1 hunks)api/index.py(2 hunks)app/README.md(1 hunks)start.sh(1 hunks)tests/e2e/README.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- api/auth/user_management.py
🚧 Files skipped from review as they are similar to previous changes (1)
- start.sh
🧰 Additional context used
📓 Path-based instructions (7)
api/app_factory.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Implement the application factory (including OAuth setup) in api/app_factory.py
Files:
api/app_factory.py
Pipfile
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Manage Python dependencies with Pipenv using Pipfile
Files:
Pipfile
.github/workflows/e2e-tests.yml
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use a dedicated E2E CI workflow in .github/workflows/e2e-tests.yml
Files:
.github/workflows/e2e-tests.yml
api/index.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep the Flask application entry point at api/index.py
Files:
api/index.py
.github/workflows/tests.yml
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep the main test CI workflow in .github/workflows/tests.yml
Files:
.github/workflows/tests.yml
.env.example
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Maintain all required environment variables in .env.example as a template
Files:
.env.example
tests/e2e/README.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Document E2E testing procedures in tests/e2e/README.md
Files:
tests/e2e/README.md
🧠 Learnings (2)
📚 Learning: 2025-08-19T08:18:37.470Z
Learnt from: CR
PR: FalkorDB/QueryWeaver#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-19T08:18:37.470Z
Learning: Applies to .github/workflows/e2e-tests.yml : Use a dedicated E2E CI workflow in .github/workflows/e2e-tests.yml
Applied to files:
.github/workflows/e2e-tests.yml
📚 Learning: 2025-08-19T08:18:37.470Z
Learnt from: CR
PR: FalkorDB/QueryWeaver#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-19T08:18:37.470Z
Learning: Applies to api/static/** : Store frontend static assets under api/static/
Applied to files:
app/README.md
🪛 GitHub Actions: Pylint
api/app_factory.py
[warning] 46-46: Line too long (102/100).
[warning] 23-23: Too few public methods (1/2) (too-few-public-methods).
api/index.py
[warning] 10-10: Trailing whitespace (trailing-whitespace).
🪛 actionlint (1.7.7)
.github/workflows/e2e-tests.yml
58-58: could not parse as YAML: yaml: line 58: could not find expected ':'
(syntax-check)
.github/workflows/tests.yml
54-54: could not parse as YAML: yaml: line 54: could not find expected ':'
(syntax-check)
🪛 YAMLlint (1.37.1)
.github/workflows/e2e-tests.yml
[error] 59-59: syntax error: could not find expected ':'
(syntax)
.github/workflows/tests.yml
[error] 55-55: syntax error: could not find expected ':'
(syntax)
🔇 Additional comments (7)
Pipfile (2)
9-9: litellm bump to ~=1.75.9: check for breaking config changes.Looks fine at a glance, but litellm can change provider/model aliases. Please verify prompts/config in api/config.py still resolve without warnings.
23-23: Playwright 1.54.0 upgrade: ensure test helpers remain compatible.This is a notable jump. If using pytest-playwright fixtures or expect() APIs, confirm no deprecations affect tests. Re-run E2E locally with headed mode to catch UI timing issues introduced by browser updates.
tests/e2e/README.md (1)
120-123: Env var rename to FASTAPI_ is consistent.*The updated keys align with the FastAPI migration and workflow changes.
api/app_factory.py (1)
49-49: Ensure FASTAPI_SECRET_KEY Is Set in All Non-Local EnvironmentsI’ve verified that no stale FLASK_SECRET_KEY references remain and that our CI/E2E pipelines already inject a fixed secret for testing:
- No occurrences of
FLASK_SECRET_KEYorFLASK_DEBUGwere found in the codebase.- The GitHub Actions workflows for CI (
tests.yml) and E2E (e2e-tests.yml) both appendFASTAPI_SECRET_KEYinto the environment before running tests.- The
README.mdandtests/e2e/README.mdinclude instructions to setFASTAPI_SECRET_KEYwhen spinning up the app locally or in CI.Because the default behavior falls back to a randomly generated key—which will invalidate all sessions on every restart—please confirm that your production (and any other deployment) pipelines (Docker runs in
publish-docker.yml, Kubernetes manifests, etc.) also set a persistentFASTAPI_SECRET_KEY. This will prevent unexpected session resets in non-local environments.api/index.py (1)
11-12: Switch to FASTAPI_DEBUG — looks good and aligns with the migration.The env flag change is correct and keeps the entry point at
api/index.pyper guidelines.README.md (2)
60-61: FastAPI/uvicorn run command — good update.The runtime instructions now correctly use
uvicorn api.index:app. Matches the codebase changes.
37-56: Verify OAuth Callback PathsI wasn’t able to locate any
/login/google/authorizedor/login/github/authorizedhandlers in the listed FastAPI routes—only the router instantiation inapi/routes/auth.py. Please double-check that the OAuth callback endpoints declared there actually match the URLs in the README:• Open
api/routes/auth.pyand look for the decorator paths, for example:@auth_router.get("/login/google/authorized") @auth_router.get("/login/github/authorized")or whichever routes you intend as the callback.
• If the actual callback paths differ (e.g.
/auth/google/callbackor/login/google/callback), update the README to use the exact paths exposed by FastAPI.Once you confirm the true endpoint paths, please adjust the “Authorization callback URL” entries in the Google and GitHub setup sections of README.md to match.
| # FASTAPI configuration | ||
| FASTAPI_SECRET_KEY=your_super_secret_key_here |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add FASTAPI_DEBUG to the template for consistency with api/index.py and CI.
api/index.py reads FASTAPI_DEBUG, and CI appends it. Include it here so local setups don’t miss it.
# FASTAPI configuration
FASTAPI_SECRET_KEY=your_super_secret_key_here
+FASTAPI_DEBUG=False # set to True to enable auto-reload in dev📝 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.
| # FASTAPI configuration | |
| FASTAPI_SECRET_KEY=your_super_secret_key_here | |
| # FASTAPI configuration | |
| FASTAPI_SECRET_KEY=your_super_secret_key_here | |
| FASTAPI_DEBUG=False # set to True to enable auto-reload in dev |
🤖 Prompt for AI Agents
.env.example around lines 20 to 21: the template includes FASTAPI_SECRET_KEY but
is missing FASTAPI_DEBUG which api/index.py reads and CI appends; add a
FASTAPI_DEBUG entry (e.g. FASTAPI_DEBUG=false) to the .env.example immediately
after FASTAPI_SECRET_KEY so local setups and developers have the same variable
present and can toggle debug mode consistently with CI and the application.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
tests/e2e/test_api_endpoints.py (1)
27-29: Fix assertion to match comment; allow 404 for missing favicon.Comment says 404 is acceptable, but the assertion only allows 200.
- assert response.status_code in [200] # 404 is acceptable if no favicon + assert response.status_code in [200, 404]api/auth/oauth_handlers.py (2)
27-33: Google “id” may be absent; fall back to OIDC ‘sub’ to avoid auth failures.Google’s userinfo typically uses ‘sub’ as the stable subject identifier. Requiring only “id” risks rejecting valid callbacks.
- user_id = user_info.get("id") + # Prefer OIDC 'sub', fall back to 'id' for older payloads + user_id = user_info.get("id") or user_info.get("sub")
55-63: Don’t require email directly from /user; fetch verified primary email when absent.GitHub frequently omits email unless it’s public. Current logic rejects such users. Use the “user:email” scope or fall back to the emails endpoint.
- # Validate required fields - if not user_id or not email: - logging.error("Missing required fields from GitHub OAuth response") - return False + # Ensure email is available — GitHub often omits it in /user + if not email: + try: + resp = await _request.app.state.oauth.github.get( + "user/emails", token=_token + ) + if getattr(resp, "status_code", 200) == 200: + emails = resp.json() + email = next( + (e.get("email") for e in emails + if e.get("primary") and e.get("verified")), + None, + ) + except Exception: # pylint: disable=broad-exception-caught + logging.exception("Failed fetching GitHub emails") + + # Validate required fields + if not user_id or not email: + logging.error("Missing required fields from GitHub OAuth response") + return FalseIf you prefer, I can follow up with a scoped try/except and stricter response typing.
app/ts/modules/chat.ts (2)
79-87: Ensure final buffered message goes through handleStreamMessage.Currently, when the stream ends, a final JSON object in buffer is parsed and passed straight to addMessage, bypassing handleStreamMessage. That can skip UI state reset and special handling (final_result, followups, etc.).
Apply this diff to route parsed JSON to the normal handler when it has a type:
- if (buffer.trim()) { - try { - const step = JSON.parse(buffer); - addMessage(step.message || JSON.stringify(step), false); - } catch { - addMessage(buffer, false); - } - } + if (buffer.trim()) { + try { + const step = JSON.parse(buffer); + if (step && typeof step === 'object' && 'type' in step) { + handleStreamMessage(step); + } else { + addMessage(step.message || JSON.stringify(step), false); + } + } catch { + addMessage(buffer, false); + } + }
216-238: Remove inline event handlers & sanitize user content to prevent XSSThe confirmation dialog in app/ts/modules/chat.ts (around lines 216–238) is currently built via innerHTML and inline onclick attributes, directly interpolating user-supplied values (
step.message,step.sql_query) without proper escaping. This allows arbitrary markup or script injection. You must:
- Escape any HTML in
step.messagebefore inserting it, replacing only newlines with<br>.- Encode the SQL query into a data attribute (e.g. via encodeURIComponent) rather than interpolating it into an attribute string.
- Remove inline
onclickhandlers and attach listeners with addEventListener.Additionally, our scan uncovered other uses of innerHTML (or inline handlers) in:
- app/ts/modules/schema.ts
- app/ts/modules/ui.ts
- app/ts/modules/modals.ts
- app/ts/modules/messages.ts
- app/ts/modules/graphs.ts
- app/ts/app.ts
Please audit these locations to ensure they either use safe text insertion (e.g. textContent) or proper escaping.
Apply this diff in app/ts/modules/chat.ts:
- const confirmationHTML = ` - <div class="destructive-confirmation" data-confirmation-id="${confirmationId}"> - <div class="confirmation-text">${(step.message || '').replace(/\n/g, '<br>')}</div> - <div class="confirmation-buttons"> - <button class="confirm-btn danger" onclick="handleDestructiveConfirmation('CONFIRM', '${escapeForSingleQuotedJsString(step.sql_query || '')}', '${confirmationId}')"> - CONFIRM - Execute Query - </button> - <button class="cancel-btn" onclick="handleDestructiveConfirmation('CANCEL', '${escapeForSingleQuotedJsString(step.sql_query || '')}', '${confirmationId}')"> - CANCEL - Abort Operation - </button> - </div> - </div> - `; + // Safely escape HTML and encode SQL for data attributes + const safeMessageHtml = escapeHtml(step.message || '').replace(/\n/g, '<br>'); + const encodedSql = encodeURIComponent(step.sql_query || ''); + const confirmationHTML = ` + <div class="destructive-confirmation" data-confirmation-id="${confirmationId}"> + <div class="confirmation-text">${safeMessageHtml}</div> + <div class="confirmation-buttons"> + <button class="confirm-btn danger" data-action="CONFIRM" data-sql="${encodedSql}"> + CONFIRM - Execute Query + </button> + <button class="cancel-btn" data-action="CANCEL" data-sql="${encodedSql}"> + CANCEL - Abort Operation + </button> + </div> + </div> + `; @@ - messageDiv.innerHTML = confirmationHTML; + messageDiv.innerHTML = confirmationHTML; @@ - if (DOM.chatMessages) DOM.chatMessages.appendChild(messageDivContainer); + if (DOM.chatMessages) DOM.chatMessages.appendChild(messageDivContainer); + // Attach click handlers instead of using inline onclick + const scope = messageDivContainer.querySelector(`[data-confirmation-id="${confirmationId}"]`); + const onClick = (ev: Event) => { + const btn = ev.currentTarget as HTMLButtonElement; + const action = btn.dataset.action || 'CANCEL'; + const sql = decodeURIComponent(btn.dataset.sql || ''); + handleDestructiveConfirmation(action, sql, confirmationId); + }; + scope?.querySelectorAll<HTMLButtonElement>('.confirm-btn, .cancel-btn') + .forEach(b => b.addEventListener('click', onClick, { once: true }));And add this helper in a common utilities module (or above):
function escapeHtml(str: string) { return str .replace(/&/g, '&') .replace(/</g, '<') .replace(/>/g, '>') .replace(/"/g, '"') .replace(/'/g, '''); }api/routes/graphs.py (1)
50-58: ConfirmRequest: add token binding and avoid mutable default; fix docstring.Two issues:
- There’s no binding between the client-supplied SQL and the confirmation step. A client could POST a different SQL to /confirm and get it executed if they know to send "CONFIRM". Add a short‑lived token to bind confirmation to the exact SQL generated earlier. See endpoint changes below.
- chat uses a mutable default []. Prefer None (or Field(default_factory=list)).
Also replace the placeholder docstring with actionable docs.
- """Confirmation request model. - - Args: - BaseModel (_type_): _description_ - """ - sql_query: str - confirmation: str = "" - chat: list = [] + """Confirmation request for executing destructive SQL. + + Attributes: + sql_query: The exact SQL string returned by the analysis step. + confirmation: Must be the literal string "CONFIRM". + confirmation_token: Token binding the SQL to the prior response. + chat: Optional prior chat messages for context. + """ + sql_query: str + confirmation: str = "" + confirmation_token: str = "" + chat: list = NoneOutside this hunk (imports), if you prefer to keep a list default while avoiding mutability:
from pydantic import BaseModel, Field # add Field from typing import List, Any # if you choose to strengthen types # then use: chat: List[Any] = Field(default_factory=list)Token helpers (outside this hunk, near the top of the file):
import os, hmac, hashlib def _sign_confirmation(sql_query: str, user_id: str) -> str: secret = os.environ.get("FASTAPI_SECRET_KEY", "") if not secret: # fall back, but strongly recommend configuring FASTAPI_SECRET_KEY secret = "dev" msg = f"{user_id}:{sql_query}".encode("utf-8") return hmac.new(secret.encode("utf-8"), msg, hashlib.sha256).hexdigest() def _verify_confirmation_token(sql_query: str, token: str, user_id: str) -> bool: expected = _sign_confirmation(sql_query, user_id) return hmac.compare_digest(expected, token or "")
🧹 Nitpick comments (47)
tests/conftest.py (3)
1-1: Docstring can better reflect file scope (pytest fixtures + FastAPI harness).This file hosts pytest fixtures plus the FastAPI/uvicorn bootstrap, not just Playwright. Minor wording tweak improves clarity.
Apply this diff:
-"""Playwright configuration for E2E tests.""" +"""Pytest fixtures for E2E tests (FastAPI + Playwright)."""
19-43: Use context manager for Popen, robust shutdown, avoid IPv6 mismatch, and throttle readiness polling.
- Addresses the Pylint warning (consider-using-with) and prevents orphaned uvicorn processes.
- Binds explicitly to 127.0.0.1 to avoid localhost resolving to ::1 on some runners.
- Avoids a hot loop when the server responds but isn’t “ready” yet by adding a sleep on non-200 responses too.
- Adds terminate/kill fallback with a wait timeout for reliable teardown in CI.
Apply this diff:
# Start the FastAPI app using pipenv - process = subprocess.Popen([ - "pipenv", "run", "uvicorn", "api.index:app", - "--host", "localhost", "--port", "5000" - ], cwd=project_root) + base_url = "http://127.0.0.1:5000" + with subprocess.Popen([ + "pipenv", "run", "uvicorn", "api.index:app", + "--host", "127.0.0.1", "--port", "5000" + ], cwd=project_root) as process: - # Wait for the app to start - max_retries = 30 - for _ in range(max_retries): - try: - response = requests.get("http://localhost:5000/", timeout=1) - if response.status_code == 200: - break - except requests.exceptions.RequestException: - time.sleep(1) - else: - process.terminate() - raise RuntimeError("FastAPI app failed to start") + # Wait for the app to start + max_retries = 60 + for _ in range(max_retries): + try: + response = requests.get(f"{base_url}/", timeout=1) + if response.status_code == 200: + break + except requests.exceptions.RequestException: + pass + # Avoid a hot loop on non-200 responses + time.sleep(1) + else: + process.terminate() + try: + process.wait(timeout=5) + except subprocess.TimeoutExpired: + process.kill() + raise RuntimeError("FastAPI app failed to start") - yield "http://localhost:5000" + # Hand off to tests + try: + yield base_url + finally: + # Cleanup + process.terminate() + try: + process.wait(timeout=5) + except subprocess.TimeoutExpired: + process.kill() - # Cleanup - process.terminate() - process.wait()Optional follow-up (no diff needed): if you have a health endpoint (e.g., GET /health), polling that instead of / will further reduce flakiness.
52-57: Improve page stability by waiting for DOM readiness on navigation.This reduces flakiness in CI by ensuring the initial navigation completes before tests proceed.
Apply this diff:
- page.goto(app_url) + page.goto(app_url, wait_until="domcontentloaded")tests/e2e/test_api_endpoints.py (6)
10-12: Optional: reuse a single HTTP session for all requests to speed up E2E.Using a shared requests.Session cuts TLS handshakes and TCP setup across tests; tends to reduce flakiness.
Add a module-scoped fixture and use it in tests:
import pytest import requests @pytest.fixture(scope="module") def http(): with requests.Session() as s: yield sThen replace requests.get/post(...) with http.get/post(...).
31-33: Avoid probing a directory; broaden acceptable statuses or target a concrete asset.Hitting /static/css/ is environment-dependent (directory listing, redirects, or 404). This can cause flaky tests.
Broaden the assertion for the directory probe:
- response = requests.get(f"{app_url}/static/css/", timeout=10) - assert response.status_code in [403] # Various acceptable responses + response = requests.get(f"{app_url}/static/css/", timeout=10) + assert response.status_code in [200, 301, 302, 403, 404]Better: test a known asset (e.g., /static/app.css) if your build guarantees it.
34-43: Parametrize login provider tests to remove duplication.Keeps the test concise and consistent across providers.
- def test_login_endpoints(self, app_url): - """Test login endpoints.""" - # Test Google login endpoint - response = requests.get(f"{app_url}/login/google", timeout=10, allow_redirects=False) - assert response.status_code in [302, 401, 403] # Should redirect or deny - - # Test GitHub login endpoint - response = requests.get(f"{app_url}/login/github", timeout=10, allow_redirects=False) - assert response.status_code in [302, 401, 403] # Should redirect or deny + @pytest.mark.parametrize("provider", ["google", "github"]) + def test_login_endpoints(self, app_url, provider): + """Test login endpoints.""" + response = requests.get(f"{app_url}/login/{provider}", timeout=10, allow_redirects=False) + assert response.status_code in [302, 401, 403] # Should redirect or deny
46-49: Align expected status with “requires authentication” semantics.If the endpoint enforces auth, 401/403 are valid outcomes; 405 may occur if GET isn’t allowed.
- # Should require authentication - assert response.status_code in [405] + # Should require authentication or reject unsupported method + assert response.status_code in [401, 403, 405]
55-60: Tighten method-not-allowed expectations.Allowing 200 can mask a handler that incorrectly accepts POST on the home route.
- assert response.status_code in [405, 200] # Some frameworks handle this differently + assert response.status_code in [405, 404] # Reject POST; 404 if no POST route exists
76-87: Use a monotonic clock for timing assertions.time.perf_counter() is monotonic and higher resolution; more accurate for elapsed time.
- start_time = time.time() + start_time = time.perf_counter() response = requests.get(app_url, timeout=10) - end_time = time.time() + end_time = time.perf_counter()api/auth/oauth_handlers.py (8)
1-5: Update docstring to reflect FastAPI callbacks (not “signals”).Minor wording drift from the Flask era; makes the module intent clearer post-migration.
-"""OAuth signal handlers for Google and GitHub authentication. - -Lightweight handlers are stored on the FastAPI app state so route -callbacks can invoke them when processing OAuth responses. -""" +"""OAuth callback handlers for Google and GitHub authentication. + +Lightweight handlers are stored on FastAPI's app.state so route +callbacks can invoke them when processing OAuth/OIDC responses. +"""
22-24: Add explicit return type to the Google handler.Both handlers consistently return booleans; annotate to aid static analysis.
- async def handle_google_callback(_request: Request, - _token: Dict[str, Any], - user_info: Dict[str, Any]): + async def handle_google_callback(_request: Request, + _token: Dict[str, Any], + user_info: Dict[str, Any]) -> bool:
37-43: Normalize identifier type when persisting.GitHub IDs are ints; Google ‘sub’ is str. Casting ensures consistent storage/lookup keys across providers.
- _, _ = ensure_user_in_organizations( - user_id, + _, _ = ensure_user_in_organizations( + str(user_id), email, name, "google", user_info.get("picture"), )
46-48: Log stack traces and quiet pylint warning locally.Use logging.exception for full trace; add a local disable if the broad catch is required by handler semantics. Pipeline flagged this exact warning.
- except Exception as exc: # capture exception for logging - logging.error("Error handling Google OAuth callback: %s", exc) + except Exception as exc: # pylint: disable=broad-exception-caught + logging.exception("Error handling Google OAuth callback")If you prefer to narrow exceptions instead, we can scope the try/except around only the persistence call.
50-52: Add explicit return type to the GitHub handler.- async def handle_github_callback(_request: Request, - _token: Dict[str, Any], - user_info: Dict[str, Any]): + async def handle_github_callback(_request: Request, + _token: Dict[str, Any], + user_info: Dict[str, Any]) -> bool:
66-71: Use GitHub’s avatar_url (not picture) and normalize user_id.The GitHub user payload uses “avatar_url”; passing “picture” yields None.
- _, _ = ensure_user_in_organizations( - user_id, + _, _ = ensure_user_in_organizations( + str(user_id), email, name, "github", - user_info.get("picture"), + user_info.get("avatar_url"), )
74-76: Mirror improved exception logging in GitHub handler.- except Exception as exc: # capture exception for logging - logging.error("Error handling GitHub OAuth callback: %s", exc) + except Exception as exc: # pylint: disable=broad-exception-caught + logging.exception("Error handling GitHub OAuth callback")This also addresses the pylint warning noted in CI.
22-76: Reduce duplication between provider handlers.Both handlers share the same structure (extract, validate, persist, log). Consider a small internal helper that receives provider-specific field names and transforms, which will simplify future provider additions (e.g., Azure AD, GitLab).
Example sketch (outside of this diff to avoid churn):
async def _process_oauth_callback( provider: str, request: Request, token: Dict[str, Any], user_info: Dict[str, Any], id_getters: list[str], name_getters: list[str], avatar_getters: list[str], ensure_email: callable | None = None, ) -> bool: try: user_id = next((user_info.get(k) for k in id_getters if user_info.get(k)), None) name = next((user_info.get(k) for k in name_getters if user_info.get(k)), None) email = user_info.get("email") if ensure_email and not email: email = await ensure_email(request, token) if not user_id or not email: logging.error("Missing required fields from %s OAuth response", provider) return False avatar = next((user_info.get(k) for k in avatar_getters if user_info.get(k)), None) ensure_user_in_organizations(str(user_id), email, name, provider, avatar) return True except Exception: # pylint: disable=broad-exception-caught logging.exception("Error handling %s OAuth callback", provider) return Falsetests/e2e/test_chat_functionality.py (1)
89-92: Optional: prefer role-based locator for robustnessUsing ARIA roles is typically more resilient.
textboxcovers both<input type="text">and<textarea>. You can then filter for enabled/visible if needed.Example:
enabled_inputs = page.get_by_role("textbox").filter(has_not=page.locator("[disabled]")).all() # Optionally also check visibility when interacting: # first_input = enabled_inputs[0] # assert first_input.is_visible()tests/test_mysql_loader.py (7)
78-79: Prefer approximate comparison for floatsComparing float 123.45 directly can be brittle due to binary representation. Use pytest.approx to avoid precision pitfalls.
Apply this diff:
- assert MySQLLoader._serialize_value(dec) == 123.45 + assert MySQLLoader._serialize_value(dec) == pytest.approx(123.45)
66-66: Broaden serialization coverage: tz-aware and microsecondsCurrent cases cover naive types with zero microseconds. Consider adding tz-aware datetime/time and microsecond cases to assert exact ISO-8601 formatting the loader emits.
Example additions:
# tz-aware datetime tz_dt = datetime.datetime(2023, 1, 1, 12, 0, 0, tzinfo=datetime.timezone.utc) assert MySQLLoader._serialize_value(tz_dt) == "2023-01-01T12:00:00+00:00" # microseconds dt_us = datetime.datetime(2023, 1, 1, 12, 0, 0, 123456) assert MySQLLoader._serialize_value(dt_us) == "2023-01-01T12:00:00.123456" # tz-aware time tz_t = datetime.time(12, 0, 0, tzinfo=datetime.timezone(datetime.timedelta(hours=-5))) assert MySQLLoader._serialize_value(tz_t) == "12:00:00-05:00"Also applies to: 70-70, 74-74
92-95: Assert operation type too (not just boolean); consider parametrize for readabilityThe function returns (is_schema_modifying, operation_type). Verifying the operation type tightens the contract.
Apply this diff:
- assert MySQLLoader.is_schema_modifying_query( - "ALTER TABLE test ADD COLUMN name VARCHAR(50)")[0] is True - assert MySQLLoader.is_schema_modifying_query( - " CREATE INDEX idx_name ON test(name)")[0] is True + assert MySQLLoader.is_schema_modifying_query( + "ALTER TABLE test ADD COLUMN name VARCHAR(50)") == (True, "ALTER") + assert MySQLLoader.is_schema_modifying_query( + " CREATE INDEX idx_name ON test(name)") == (True, "CREATE")Optional follow-up (outside this hunk): collapse these into a parametrized test for multiple DDL verbs to reduce duplication.
105-105: Duplicate empty-query assertionThis is identical to the previous line and can be removed.
Apply this diff:
- assert MySQLLoader.is_schema_modifying_query("")[0] is False
132-137: Combine nested context managers (Ruff SIM117)Use a single with statement with multiple contexts for readability and to satisfy the linter.
Apply this diff:
- with patch.object(MySQLLoader, 'extract_tables_info', - return_value={'users': {'description': 'User table'}}): - with patch.object(MySQLLoader, 'extract_relationships', return_value={}): - success, message = MySQLLoader.load( - "test_prefix", "mysql://user:pass@localhost:3306/testdb" - ) + with patch.object( + MySQLLoader, 'extract_tables_info', + return_value={'users': {'description': 'User table'}} + ), patch.object( + MySQLLoader, 'extract_relationships', return_value={} + ): + success, message = MySQLLoader.load( + "test_prefix", "mysql://user:pass@localhost:3306/testdb" + )
141-141: Strengthen verification: assert load_to_graph argumentsVerifies the exact graph name, payload, and keyword args passed by load(), not just that it was called.
Apply this diff:
- mock_load_to_graph.assert_called_once() + mock_load_to_graph.assert_called_once_with( + "test_prefix_testdb", + {'users': {'description': 'User table'}}, + {}, + db_name="testdb", + db_url="mysql://user:pass@localhost:3306/testdb", + )
18-19: Pylint warning: protected member access (_parse_mysql_url) in testsAccessing a protected member is fine in tests but triggers Pylint (protected-access). Two options:
- Expose a public parse_mysql_url API and test that instead.
- Silence the rule for this test module.
If you prefer silencing in tests, add this near the top of the file (after the module docstring):
# pylint: disable=protected-accessAlternatively, add inline disables on each call site:
result = MySQLLoader._parse_mysql_url(url) # pylint: disable=protected-accessapp/ts/modules/schema.ts (4)
89-91: Bare catch is fine here; keep it intentionally empty.Optional: add a short comment like “// intentionally ignore style parsing errors” so future readers don’t second‑guess this. If you enforce no-empty in ESLint, allow empty catches.
130-131: Empty catch acknowledged; consider a tiny note for clarity.Same as above—this is defensible since the fallback is returned below. A brief comment helps maintainers.
140-144: Minor DRY: avoid duplicating arrow settings in try/catch.You set linkDirectionalArrowLength/linkDirectionalArrowRelPos in both branches. Not a blocker, but you can set arrows once outside the try/catch and keep only the color logic inside.
Example refactor sketch (outside the selected range):
// after computing edgeColor (inside try) OR in catch fallback: Graph.linkDirectionalArrowLength(6).linkDirectionalArrowRelPos(1);
74-98: Perf: memoize edge color to avoid repeated getComputedStyle calls during drawing.getComputedStyle + regex/measureText runs every link render; cache the resolved color once per frame or once after load unless theme changes.
Example:
let cachedEdgeColor: string | null = null; const getEdgeColor = () => cachedEdgeColor ?? (cachedEdgeColor = /* current logic */);api/routes/database.py (6)
16-20: Fix placeholder docstring on the request modelThe current docstring has placeholder content and doesn't describe the payload. Replace with a concise, accurate docstring.
Apply this diff:
-class DatabaseConnectionRequest(BaseModel): - """Database connection request model. - - Args: - BaseModel (_type_): _description_ - """ +class DatabaseConnectionRequest(BaseModel): + """Schema for a database connection request. + + Attributes: + url: Connection string to the source DB (e.g., postgresql://..., mysql://...). + """ url: str
32-39: Simplify and harden URL validation (strip; drop redundant isinstance check)Pydantic guarantees
urlis astr(or raises). Strip once and fold the two checks into one.Apply this diff:
- url = db_request.url - if not url: - raise HTTPException(status_code=400, detail="No URL provided") - - # Validate URL format - if not isinstance(url, str) or len(url.strip()) == 0: - raise HTTPException(status_code=400, detail="Invalid URL format") + url = (db_request.url or "").strip() + if not url: + raise HTTPException(status_code=400, detail="No URL provided")Optionally, make scheme checks case-insensitive and accept driver variants:
+ url_lower = url.lower() - if url.startswith("postgres://") or url.startswith("postgresql://"): + if url_lower.startswith(("postgres://", "postgresql://", "postgresql+psycopg2://")): ... - elif url.startswith("mysql://"): + elif url_lower.startswith(("mysql://", "mysql+pymysql://")):
68-74: Error detail can list all accepted schemes (align with checks if you adopt them)If you accept driver variants and case-insensitivity as suggested, update the message to match.
- raise HTTPException( - status_code=400, - detail=( - "Invalid database URL. Supported formats: postgresql:// " - "or mysql://" - ), - ) + raise HTTPException( + status_code=400, + detail=( + "Invalid database URL. Supported schemes include: " + "postgresql://, postgres://, postgresql+psycopg2://, " + "mysql://, mysql+pymysql://." + ), + )
40-43: Guard against missingrequest.state.user_idbefore calling loadersIf
token_requiredfails to setrequest.state.user_id, this will crash later or mislabel errors. Fail fast with 401.success = False result = "" + user_id = getattr(request.state, "user_id", None) + if not user_id: + logging.error("Missing request.state.user_id; auth middleware may not have run") + raise HTTPException(status_code=401, detail="Unauthorized")Follow-up: Please confirm that
token_requiredreliably setsrequest.state.user_idin the FastAPI context (see note below on using dependencies).
24-26: Prefer FastAPI dependencies over decorators for authUsing a decorator can obscure the path operation signature and break schema/docs or async flow. FastAPI-native dependency injection is safer and clearer.
Proposed signature:
from fastapi import Depends @database_router.post("/database") async def connect_database( request: Request, db_request: DatabaseConnectionRequest, _user=Depends(token_required), # ensures auth ran; use the returned user if available ): ...Please confirm
token_requiredis implemented as a dependency (or a decorator compatible with async andfunctools.wraps).
85-85: Log hygiene: avoid leaking connection details
resultmay include driver error messages. Good that you don't return it, but consider downgrading towarningand ensuring logs are not exposed in lower environments. Ifresultcan contain URLs, sanitize credentials before logging.api/routes/graphs.py (3)
30-34: Replace placeholder docstring with meaningful model docs.The docstring reads like a template stub and doesn’t add value. Tighten it to document the payload.
- """Graph data model. - - Args: - BaseModel (_type_): _description_ - """ + """Payload for graph creation/loading. + + Attributes: + database: Logical database name (without user prefix). + """
39-43: Clarify ChatRequest docstring; optionally strengthen types.Docstring is a template stub. Also consider marking optional fields explicitly for readability.
- """Chat request model. - - Args: - BaseModel (_type_): _description_ - """ + """Request body for text2sql. + + Attributes: + chat: Ordered list of user/system messages. + result: Optional prior SQL execution results used for context. + instructions: Optional system hints to guide analysis. + """Optionally, strengthen types (outside this hunk):
# at the top of the file from typing import List, Optional, Any # in ChatRequest chat: List[Any] result: Optional[list] = None instructions: Optional[str] = None
345-348: LGTM: Formatting the timing log improves readability.No functional change; good call. Minor nit: nearby, line 359 logs an error at info level — consider logging.error for failures.
app/public/css/landing.css (5)
114-116: Provide a safe fallback for var(--bg-tertiary)If --bg-tertiary isn’t defined in all themes, the background computes to initial. Add a fallback to the existing secondary surface to ensure consistent rendering.
-.demo-card { - background: var(--bg-tertiary); +.demo-card { + background: var(--bg-tertiary, var(--falkor-secondary));
268-274: Also add background fallback for feature cardsMirror the same defensive fallback on feature card backgrounds.
-.feature-card { - background: var(--bg-tertiary); +.feature-card { + background: var(--bg-tertiary, var(--falkor-secondary));
109-112: Prefer :focus-visible over :focus for custom focus ringsSwitching to :focus-visible preserves keyboard accessibility while avoiding focus rings on mouse click.
-.btn-pill:focus { +.btn-pill:focus-visible { outline: none; box-shadow: 0 0 0 4px rgba(59,130,246,0.12); }-.demo-cta .btn-full:focus { +.demo-cta .btn-full:focus-visible { outline: none; box-shadow: 0 0 0 4px rgba(59,130,246,0.08); }Also applies to: 243-246
222-231: Add transitions to .btn-full to match hover/active effectsHover/active use transform/box-shadow but no base transition, so the motion is abrupt. Add a transition for smoother micro-interactions.
.demo-cta .btn-full { display: inline-block; width: 100%; padding: 0.75rem; border-radius: 8px; background: #e7f1ff; color: var(--falkor-primary); text-decoration: none; border: none; + transition: transform 140ms ease, box-shadow 140ms ease, background 120ms ease; }Also applies to: 233-241
100-107: Respect reduced motion preferencesConsider disabling transform/animation when users prefer reduced motion (also covers the blinking caret).
Add this at the end of the file:
@media (prefers-reduced-motion: reduce) { .btn-pill, .demo-cta .btn-full { transition: none !important; transform: none !important; } .demo-sql.typing::after { animation: none !important; } }Also applies to: 233-241, 196-205
app/templates/landing.j2 (4)
12-15: Make the logo a home link and expose a readable site titleTurning the logo into a link improves nav; a visually-present/hidden site title improves a11y. Update href to the correct home route name.
- <div class="site-header-inner"> - <img src="{{ url_for('static', path='icons/queryweaver.webp') }}" alt="QueryWeaver" /> - </div> + <a class="site-header-inner" href="{{ url_for('index') }}" aria-label="QueryWeaver home"> + <img src="{{ url_for('static', path='icons/queryweaver.webp') }}" alt="QueryWeaver" /> + <span class="site-title">QueryWeaver</span> + </a>If your root route isn’t named "index", please adjust accordingly (e.g., "home", "landing").
49-49: Announce SQL updates to assistive techThe generated SQL changes dynamically. Marking this region as a polite live region keeps screen readers informed without being disruptive.
- <pre class="demo-sql"></pre> + <pre class="demo-sql" role="status" aria-live="polite" aria-atomic="true"></pre>
81-100: Lightweight highlighter is safe, but avoid highlighting inside string literalsCurrent order can still wrap keywords/numbers that appear inside quoted strings. Minimal improvement: split on SQL string literals first, then only highlight keywords/functions/numbers in non-string parts. Also handle doubled single quotes.
- // very small client-side SQL highlighter (runs after typing completes) - function highlightSQL(sql){ - if(!sql) return ''; - // escape first - let out = escapeHtml(sql); - // strings (single quotes) - out = out.replace(/('[^']*')/g, '<span class="sql-string">$1</span>'); - // keywords - out = out.replace(/\b(SELECT|FROM|JOIN|ON|WHERE|AND|OR|GROUP|BY|ORDER|LIMIT|AS|IN|IS|NULL|INNER|LEFT|RIGHT|OUTER)\b/gi, - function(m){ return '<span class="sql-keyword">'+m+'</span>'; }); - // aggregate/functions - out = out.replace(/\b(COUNT|SUM|AVG|MIN|MAX)\b/gi, function(m){ return '<span class="sql-func">'+m+'</span>'; }); - // numbers - out = out.replace(/\b(\d+\.?\d*)\b/g, '<span class="sql-number">$1</span>'); - return out; - } + // very small client-side SQL highlighter: + // - escape first + // - split by single-quoted strings (supports doubled quotes) + // - highlight only non-string parts + function highlightSQL(sql) { + if (!sql) return ''; + const parts = sql.split(/('(?:''|[^'])*')/g); // keep delimiters + const out = parts.map((part, i) => { + const escaped = escapeHtml(part); + // odd indices are quoted strings due to split keeping delimiters + const isString = i % 2 === 1; + if (isString) { + return '<span class="sql-string">' + escaped + '</span>'; + } + // keywords + let tmp = escaped.replace(/\b(SELECT|FROM|JOIN|ON|WHERE|AND|OR|GROUP|BY|ORDER|LIMIT|AS|IN|IS|NULL|INNER|LEFT|RIGHT|OUTER)\b/gi, + m => '<span class="sql-keyword">' + m + '</span>'); + // functions + tmp = tmp.replace(/\b(COUNT|SUM|AVG|MIN|MAX)\b/gi, + m => '<span class="sql-func">' + m + '</span>'); + // numbers + tmp = tmp.replace(/\b(\d+\.?\d*)\b/g, '<span class="sql-number">$1</span>'); + return tmp; + }).join(''); + return out; + }
113-139: Support reduced motion and avoid tight setIntervalHonor user motion prefs and avoid a 15ms interval loop. Early-return for reduced motion; otherwise, throttle with requestAnimationFrame while preserving your logic.
- function typeSql(text) { + function typeSql(text) { if (!sEl) return; // hide success while typing if (successEl) successEl.style.display = 'none'; // stop any previous typing if (typingTimer) { clearInterval(typingTimer); typingTimer = null; } sEl.classList.add('typing'); sEl.textContent = ''; - let pos = 0; - typingTimer = setInterval(() => { - pos += 1; - // progressively render highlighted HTML for the substring so colors appear while typing - if(sEl) sEl.innerHTML = highlightSQL(text.slice(0, pos)); - if (pos >= text.length) { - clearInterval(typingTimer); - typingTimer = null; - sEl.classList.remove('typing'); - if (sEl) sEl.innerHTML = highlightSQL(text); - // show success when typing completes - if (successEl) successEl.style.display = 'flex'; - } - }, typingSpeed); + // respect reduced motion + const reduceMotion = window.matchMedia && window.matchMedia('(prefers-reduced-motion: reduce)').matches; + if (reduceMotion) { + sEl.classList.remove('typing'); + sEl.innerHTML = highlightSQL(text); + if (successEl) successEl.style.display = 'flex'; + return; + } + let pos = 0, last = 0; + const step = (ts) => { + if (!sEl) return; + // throttle roughly by typingSpeed + if (!last || ts - last >= typingSpeed) { + last = ts; + pos += 1; + sEl.innerHTML = highlightSQL(text.slice(0, pos)); + if (pos >= text.length) { + sEl.classList.remove('typing'); + sEl.innerHTML = highlightSQL(text); + if (successEl) successEl.style.display = 'flex'; + return; // stop + } + } + requestAnimationFrame(step); + }; + requestAnimationFrame(step); }
📜 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 (21)
.github/workflows/tests.yml(2 hunks)api/app_factory.py(1 hunks)api/auth/__init__.py(2 hunks)api/auth/oauth_handlers.py(4 hunks)api/index.py(1 hunks)api/loaders/mysql_loader.py(1 hunks)api/routes/auth.py(3 hunks)api/routes/database.py(4 hunks)api/routes/graphs.py(4 hunks)app/.eslintrc.cjs(0 hunks)app/eslint.config.cjs(1 hunks)app/public/css/landing.css(7 hunks)app/templates/base.j2(1 hunks)app/templates/landing.j2(5 hunks)app/ts/modules/chat.ts(2 hunks)app/ts/modules/schema.ts(3 hunks)tests/conftest.py(1 hunks)tests/e2e/test_api_endpoints.py(1 hunks)tests/e2e/test_basic_functionality.py(3 hunks)tests/e2e/test_chat_functionality.py(1 hunks)tests/test_mysql_loader.py(5 hunks)
💤 Files with no reviewable changes (1)
- app/.eslintrc.cjs
✅ Files skipped from review due to trivial changes (5)
- api/loaders/mysql_loader.py
- app/templates/base.j2
- api/routes/auth.py
- api/auth/init.py
- tests/e2e/test_basic_functionality.py
🚧 Files skipped from review as they are similar to previous changes (2)
- api/app_factory.py
- api/index.py
🧰 Additional context used
📓 Path-based instructions (8)
tests/conftest.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep pytest configuration and fixtures in tests/conftest.py
Files:
tests/conftest.py
tests/e2e/test_*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place Playwright E2E tests as tests/e2e/test_*.py
Files:
tests/e2e/test_api_endpoints.pytests/e2e/test_chat_functionality.py
tests/test_*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Name Python unit tests as tests/test_*.py
Files:
tests/test_mysql_loader.py
api/routes/database.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep database management route handlers in api/routes/database.py
Files:
api/routes/database.py
api/routes/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place all additional Flask route handlers under api/routes/
Files:
api/routes/database.pyapi/routes/graphs.py
.github/workflows/tests.yml
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep the main test CI workflow in .github/workflows/tests.yml
Files:
.github/workflows/tests.yml
api/routes/graphs.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep graph/database route handlers in api/routes/graphs.py
Files:
api/routes/graphs.py
api/auth/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place authentication modules under api/auth/
Files:
api/auth/oauth_handlers.py
🧠 Learnings (4)
📚 Learning: 2025-08-19T08:18:37.470Z
Learnt from: CR
PR: FalkorDB/QueryWeaver#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-19T08:18:37.470Z
Learning: Applies to tests/conftest.py : Keep pytest configuration and fixtures in tests/conftest.py
Applied to files:
tests/conftest.py
📚 Learning: 2025-08-19T08:18:37.470Z
Learnt from: CR
PR: FalkorDB/QueryWeaver#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-19T08:18:37.470Z
Learning: Applies to tests/e2e/test_*.py : Place Playwright E2E tests as tests/e2e/test_*.py
Applied to files:
tests/conftest.py
📚 Learning: 2025-08-19T08:18:37.470Z
Learnt from: CR
PR: FalkorDB/QueryWeaver#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-19T08:18:37.470Z
Learning: Applies to .github/workflows/e2e-tests.yml : Use a dedicated E2E CI workflow in .github/workflows/e2e-tests.yml
Applied to files:
.github/workflows/tests.yml
📚 Learning: 2025-08-19T08:18:37.470Z
Learnt from: CR
PR: FalkorDB/QueryWeaver#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-19T08:18:37.470Z
Learning: Applies to .github/workflows/tests.yml : Keep the main test CI workflow in .github/workflows/tests.yml
Applied to files:
.github/workflows/tests.yml
🧬 Code graph analysis (2)
tests/test_mysql_loader.py (1)
api/loaders/mysql_loader.py (4)
MySQLLoader(20-538)_serialize_value(49-68)is_schema_modifying_query(376-404)load(128-170)
api/routes/database.py (2)
api/loaders/postgres_loader.py (2)
PostgresLoader(18-491)load(67-108)api/loaders/mysql_loader.py (2)
load(128-170)MySQLLoader(20-538)
🪛 GitHub Actions: Pylint
tests/conftest.py
[warning] 20-20: pylint: Consider using 'with' for resource-allocating operations (consider-using-with). Command: 'pipenv run pylint $(git ls-files '*.py')'.
tests/test_mysql_loader.py
[warning] 18-18: pylint: Access to a protected member _parse_mysql_url of a client class (protected-access). Command: 'pipenv run pylint $(git ls-files '*.py')'.
api/auth/oauth_handlers.py
[warning] 46-46: pylint: Catching too general exception Exception (broad-exception-caught). Command: 'pipenv run pylint $(git ls-files '*.py')'.
[warning] 74-74: pylint: Catching too general exception Exception (broad-exception-caught). Command: 'pipenv run pylint $(git ls-files '*.py')'.
🪛 Ruff (0.12.2)
tests/test_mysql_loader.py
132-134: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
api/routes/database.py
51-54: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
63-65: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🪛 Biome (2.1.2)
app/public/css/landing.css
[error] 127-127: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
border is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
[error] 277-277: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
border is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
⏰ 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 (17)
tests/conftest.py (1)
3-7: Good move: lift imports to module scope and keep fixtures in tests/conftest.py.Module-level import of os (and keeping pytest/request imports here) reduces per-fixture overhead and aligns with our guideline to keep pytest configuration/fixtures in tests/conftest.py. Nice.
tests/e2e/test_api_endpoints.py (1)
4-4: Good move: import time at module scope for reuse.Avoids function-level imports and keeps tests clean.
api/auth/oauth_handlers.py (2)
19-21: LGTM: exposing OAuth registry via app.state.This keeps route code lean and aligns with FastAPI patterns.
78-81: LGTM: exposing callbacks via app.state.Clear separation: routes resolve state, handlers encapsulate provider-specific logic.
tests/e2e/test_chat_functionality.py (1)
89-92: Fix: passing two positional args to locator() raises TypeErrorPlaywright’s
page.locatoraccepts a single positionalselectorargument. The trailing comma after the second string turns this into two positional strings and will error at runtime. Join the selector into a single string (implicit concatenation) or put it on one line.Apply this diff:
- enabled_inputs = page.locator( - "input[type='text']:not([disabled]):visible, " - "textarea:not([disabled]):visible", - ).all() + enabled_inputs = page.locator( + "input[type='text']:not([disabled]):visible, " + "textarea:not([disabled]):visible" + ).all()Likely an incorrect or invalid review comment.
.github/workflows/tests.yml (2)
53-54: LGTM: switched to FASTAPI_ env vars for unit tests*The env keys now match the FastAPI migration and the indentation is correct within the multiline run block.
106-107: FASTAPI_DEBUG boolean parsing validatedA quick grep confirms that:
- In api/index.py (lines 11–12), the code reads the env var with a default of
'False', lowercases it, and compares to'true', so onlyTrue/trueyields debug mode.- In start.sh (lines 21–24), the script checks for both
"True"and"true"before enabling--reload.With
FASTAPI_DEBUG=Falseappended in the CI workflow, the app correctly runs in production mode. No further changes required.tests/test_mysql_loader.py (1)
3-8: Imports rework to module-qualified datetime/decimal looks goodUsing datetime and decimal modules directly in tests improves clarity and avoids name shadowing. Import grouping (stdlib → third-party → local) also looks clean.
app/ts/modules/chat.ts (2)
83-85: Modernized to optional catch binding—LGTM.Behavior unchanged; JSON parse failure falls back to raw message display.
103-105: Same here—LGTM.Per-message JSON parse errors degrade gracefully to a visible “Failed: …” message.
api/routes/database.py (2)
4-4: Good cleanup on importsDropping the unused
statusimport keeps things lean. No further issues here.
48-49: No changes needed for loader.loadcalls
Verified that bothPostgresLoader.loadandMySQLLoader.loadare defined as@staticmethodon their respective classes, so callingPostgresLoader.load(...)andMySQLLoader.load(...)will work correctly at runtime.api/routes/graphs.py (1)
9-9: LGTM: FastAPI import set is appropriate.Imports match usage in this module (APIRouter, Request, HTTPException, UploadFile, File). No action needed.
app/public/css/landing.css (2)
11-16: Header styles look goodThe banner block is straightforward and consistent with the new template. No issues.
184-191: SQL token styles—nice touchThe lightweight token coloring and monospace/pre-wrap improve readability a lot. Looks good.
app/templates/landing.j2 (2)
56-57: JS structure reads well and is safely escapedI like the encapsulation and the escapeHtml + innerHTML flow; this keeps XSS at bay while enabling highlighting.
Also applies to: 150-154
26-28: Verify intent behind “Sign Up” and “Sign In” CTAsI see both buttons currently link to the
auth.chatroute (defined in api/routes/auth.py at line 46) and there aren’t any dedicated login or signup endpoints present. If this unified entry point is intentional, consider renaming the route to something more generic (e.g.auth.entry). Otherwise, please:
- Wire the “Sign In” button to its proper login endpoint (e.g.
auth.login).- Implement or wire the “Sign Up for Free” button to a distinct signup route (e.g.
auth.signup).Affected locations:
- app/templates/landing.j2 (lines 26–28)
- api/routes/auth.py (chat route definition)
| - name: Run lint | ||
| run: | | ||
| pipenv run pylint "$(git ls-files '*.py')" || true | ||
|
|
||
| - name: Run frontend lint | ||
| if: "exists('app/package.json')" | ||
| run: | | ||
| (cd app && npm run lint) | ||
| make lint | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm make lint target exists and fails on issues
Replacing direct pylint invocation with make lint is fine, but please verify the Makefile has a lint target, aggregates both backend and frontend linters as intended, and returns non-zero on violations to fail the job.
Run this to verify:
🏁 Script executed:
#!/bin/bash
# Validate that a 'lint' target exists and is non-phony no-op.
rg -nP '^\s*lint\s*:' Makefile
# Show the command(s) it runs for quick inspection
rg -nA5 -P '^\s*lint\s*:' MakefileLength of output: 330
Ensure the lint target fails on all lint errors
The Makefile’s lint target is defined (line 45) and correctly invokes both backend (pylint) and frontend (eslint) linters, but the backend step suppresses failures with || true, so Python lint errors won’t fail the CI job.
- Makefile (around lines 46–47): remove the trailing
|| truefrom thepipenv run pylint …command to allow non-zero exit on violations. - Makefile (around line 49): verify that the
lint-frontendtarget invoked bymake lint-frontendsimilarly propagates its exit code (i.e., does not use any|| trueor equivalent).
🤖 Prompt for AI Agents
.github/workflows/tests.yml around lines 60 to 63: the CI step "Run lint"
currently runs `make lint` but the Makefile's backend lint command suppresses
failures; edit the Makefile (around lines 46–47) to remove the trailing `||
true` from the `pipenv run pylint …` invocation so pylint non-zero exits
propagate, and ensure the frontend lint target invoked (around line 49) does not
suppress its exit code (remove any `|| true` or equivalent); then keep the
workflow step as-is so `make lint` will fail the job on any lint errors.
| if url.startswith("postgres://") or url.startswith("postgresql://"): | ||
| try: | ||
| # Attempt to connect/load using the PostgreSQL loader | ||
| success, result = PostgresLoader.load(request.state.user_id, url) | ||
| except (ValueError, ConnectionError) as e: | ||
| logging.error("PostgreSQL connection error: %s", str(e)) | ||
| raise HTTPException(status_code=500, detail="Failed to connect to PostgreSQL database") | ||
|
|
||
| raise HTTPException( | ||
| status_code=500, | ||
| detail="Failed to connect to PostgreSQL database", | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve exception context per Ruff B904; consider 4xx for user-caused connection errors
Attach the original exception when raising HTTPException to satisfy B904 and aid debugging. Optionally, map bad creds/host to 4xx since they are client inputs.
Apply this diff (required for B904):
except (ValueError, ConnectionError) as e:
logging.error("PostgreSQL connection error: %s", str(e))
- raise HTTPException(
- status_code=500,
- detail="Failed to connect to PostgreSQL database",
- )
+ raise HTTPException(
+ status_code=500,
+ detail="Failed to connect to PostgreSQL database",
+ ) from eOptional API semantics tweak:
- status_code=500,
+ status_code=400,📝 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 url.startswith("postgres://") or url.startswith("postgresql://"): | |
| try: | |
| # Attempt to connect/load using the PostgreSQL loader | |
| success, result = PostgresLoader.load(request.state.user_id, url) | |
| except (ValueError, ConnectionError) as e: | |
| logging.error("PostgreSQL connection error: %s", str(e)) | |
| raise HTTPException(status_code=500, detail="Failed to connect to PostgreSQL database") | |
| raise HTTPException( | |
| status_code=500, | |
| detail="Failed to connect to PostgreSQL database", | |
| ) | |
| if url.startswith("postgres://") or url.startswith("postgresql://"): | |
| try: | |
| # Attempt to connect/load using the PostgreSQL loader | |
| success, result = PostgresLoader.load(request.state.user_id, url) | |
| except (ValueError, ConnectionError) as e: | |
| logging.error("PostgreSQL connection error: %s", str(e)) | |
| raise HTTPException( | |
| status_code=500, | |
| detail="Failed to connect to PostgreSQL database", | |
| ) from e |
🧰 Tools
🪛 Ruff (0.12.2)
51-54: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In api/routes/database.py around lines 45 to 54, the HTTPException raised on
PostgreSQL connection failure discards the original exception (Ruff B904) and
always returns 500; modify the exception handling to re-raise the HTTPException
while preserving the original exception as the cause (attach the caught
exception to the new exception) so exception chaining is preserved for
debugging, and optionally inspect the caught exception message/type (e.g., auth
failure, host unreachable) to return a 4xx (client error) status when the error
clearly indicates bad client input, otherwise return 500.
| elif url.startswith("mysql://"): | ||
| try: | ||
| # Attempt to connect/load using the MySQL loader | ||
| success, result = MySQLLoader.load(request.state.user_id, url) | ||
| 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" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same as above for MySQL path: add from e and optionally use 4xx
Mirror the PostgreSQL fix to keep behavior consistent and satisfy B904.
Apply this diff:
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 eOptional:
- status_code=500,
+ status_code=400,📝 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.
| elif url.startswith("mysql://"): | |
| try: | |
| # Attempt to connect/load using the MySQL loader | |
| success, result = MySQLLoader.load(request.state.user_id, url) | |
| 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" | |
| ) | |
| elif url.startswith("mysql://"): | |
| try: | |
| # Attempt to connect/load using the MySQL loader | |
| success, result = MySQLLoader.load(request.state.user_id, url) | |
| 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" | |
| ) from e |
🧰 Tools
🪛 Ruff (0.12.2)
63-65: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In api/routes/database.py around lines 57 to 65, the MySQL except block logs the
error and raises an HTTPException without exception chaining; update the raise
to include the original exception (raise HTTPException(status_code=500,
detail="Failed to connect to MySQL database") from e) to preserve the traceback
and mirror the PostgreSQL fix, and optionally change the status_code to an
appropriate 4xx value if this is a client-side input/URL error.
| async def confirm_destructive_operation( | ||
| request: Request, | ||
| graph_id: str, | ||
| confirm_data: ConfirmRequest, | ||
| ): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Critical: /confirm trusts client-supplied SQL; bind confirmation to server-issued SQL and normalize graph_id.
- Anyone who can hit this endpoint with a valid token can alter sql_query and still get execution if they send "CONFIRM". Bind confirmations to the exact SQL you previously streamed by including a server-generated token in the initial destructive_confirmation payload, and verify it here.
- Normalize graph_id consistently (you already cap to 100 chars in query_graph).
Apply these focused changes:
-async def confirm_destructive_operation(
+async def confirm_destructive_operation(
request: Request,
graph_id: str,
confirm_data: ConfirmRequest,
):
@@
- graph_id = request.state.user_id + "_" + graph_id.strip()
+ # Align with query_graph: trim and cap length
+ raw = (graph_id or "").strip()[:100]
+ graph_id = f"{request.state.user_id}_{raw}"
@@
- if hasattr(confirm_data, 'confirmation'):
- confirmation = confirm_data.confirmation.strip().upper()
- else:
- confirmation = ""
+ confirmation = (getattr(confirm_data, "confirmation", "") or "").strip().upper()Then, still within this function (just after extracting sql_query and before executing anything), verify the token (outside-hunk helper shown in the previous comment):
token = getattr(confirm_data, "confirmation_token", "")
if not _verify_confirmation_token(sql_query, token, request.state.user_id):
raise HTTPException(status_code=403, detail="Invalid or expired confirmation token")And when you first request confirmation in query_graph, include the token (outside this hunk; illustrative snippet):
confirmation_token = _sign_confirmation(sql_query, request.state.user_id)
yield json.dumps({
"type": "destructive_confirmation",
"message": confirmation_message,
"sql_query": sql_query,
"operation_type": sql_type,
"confirmation_token": confirmation_token,
}) + MESSAGE_DELIMITEROptional hardening:
- Use your loader’s is_schema_modifying_query to decide whether to ask for confirmation (instead of relying on the first keyword). This avoids bypasses via leading CTEs/comments.
- Consider adding a TTL’d server-side cache keyed by (user_id, graph_id, token) to enforce one-time use.
Also applies to: 534-540
| // ESLint v9 flat config equivalent for the project's TypeScript rules | ||
| module.exports = [ | ||
| { | ||
| ignores: ['**/node_modules/**', 'dist/**'], | ||
| }, | ||
| { | ||
| languageOptions: { | ||
| parser: require('@typescript-eslint/parser'), | ||
| parserOptions: { | ||
| ecmaVersion: 2020, | ||
| sourceType: 'module', | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| plugins: { | ||
| '@typescript-eslint': require('@typescript-eslint/eslint-plugin'), | ||
| }, | ||
| }, | ||
| { | ||
| rules: { | ||
| // Base JS recommended rules | ||
| 'no-unused-vars': 'warn', | ||
| // TypeScript rules | ||
| '@typescript-eslint/no-explicit-any': 'off', | ||
| }, | ||
| linterOptions: { | ||
| reportUnusedDisableDirectives: true, | ||
| }, | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align flat config with TS best practices and project patterns.
Current config uses base no-unused-vars (can conflict with TS) and lacks browser globals. Recommend scoping TS settings by files, enabling @typescript-eslint/no-unused-vars, allowing empty catches, and relaxing no-constant-condition for streaming loops.
Apply this diff to replace the file contents:
-// ESLint v9 flat config equivalent for the project's TypeScript rules
-module.exports = [
- {
- ignores: ['**/node_modules/**', 'dist/**'],
- },
- {
- languageOptions: {
- parser: require('@typescript-eslint/parser'),
- parserOptions: {
- ecmaVersion: 2020,
- sourceType: 'module',
- },
- },
- },
- {
- plugins: {
- '@typescript-eslint': require('@typescript-eslint/eslint-plugin'),
- },
- },
- {
- rules: {
- // Base JS recommended rules
- 'no-unused-vars': 'warn',
- // TypeScript rules
- '@typescript-eslint/no-explicit-any': 'off',
- },
- linterOptions: {
- reportUnusedDisableDirectives: true,
- },
- },
-];
+// ESLint v9 flat config
+const globals = require('globals');
+module.exports = [
+ { ignores: ['**/node_modules/**', 'dist/**'] },
+ // Base JS settings
+ {
+ files: ['**/*.{js,cjs,mjs}'],
+ languageOptions: {
+ ecmaVersion: 'latest',
+ sourceType: 'module',
+ globals: { ...globals.browser, ...globals.es2021 },
+ },
+ rules: {
+ // Allow while (true) streaming loops without per-line disables
+ 'no-constant-condition': ['warn', { checkLoops: false }],
+ // Allow intentionally empty catch blocks (used across app)
+ 'no-empty': ['warn', { allowEmptyCatch: true }],
+ },
+ },
+ // TypeScript settings
+ {
+ files: ['**/*.{ts,tsx}'],
+ languageOptions: {
+ parser: require('@typescript-eslint/parser'),
+ parserOptions: { ecmaVersion: 'latest', sourceType: 'module' },
+ globals: { ...globals.browser, ...globals.es2021 },
+ },
+ plugins: {
+ '@typescript-eslint': require('@typescript-eslint/eslint-plugin'),
+ },
+ rules: {
+ // Prefer TS-aware unused-vars and disable the base rule for TS files
+ 'no-unused-vars': 'off',
+ '@typescript-eslint/no-unused-vars': ['warn', { argsIgnorePattern: '^_', varsIgnorePattern: '^_' }],
+ '@typescript-eslint/no-explicit-any': 'off',
+ // Mirror JS loop rule
+ 'no-constant-condition': ['warn', { checkLoops: false }],
+ // Empty catch blocks permitted (we use bare catch {})
+ 'no-empty': ['warn', { allowEmptyCatch: true }],
+ },
+ linterOptions: { reportUnusedDisableDirectives: true },
+ },
+];📝 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.
| // ESLint v9 flat config equivalent for the project's TypeScript rules | |
| module.exports = [ | |
| { | |
| ignores: ['**/node_modules/**', 'dist/**'], | |
| }, | |
| { | |
| languageOptions: { | |
| parser: require('@typescript-eslint/parser'), | |
| parserOptions: { | |
| ecmaVersion: 2020, | |
| sourceType: 'module', | |
| }, | |
| }, | |
| }, | |
| { | |
| plugins: { | |
| '@typescript-eslint': require('@typescript-eslint/eslint-plugin'), | |
| }, | |
| }, | |
| { | |
| rules: { | |
| // Base JS recommended rules | |
| 'no-unused-vars': 'warn', | |
| // TypeScript rules | |
| '@typescript-eslint/no-explicit-any': 'off', | |
| }, | |
| linterOptions: { | |
| reportUnusedDisableDirectives: true, | |
| }, | |
| }, | |
| ]; | |
| // ESLint v9 flat config | |
| const globals = require('globals'); | |
| module.exports = [ | |
| { ignores: ['**/node_modules/**', 'dist/**'] }, | |
| // Base JS settings | |
| { | |
| files: ['**/*.{js,cjs,mjs}'], | |
| languageOptions: { | |
| ecmaVersion: 'latest', | |
| sourceType: 'module', | |
| globals: { ...globals.browser, ...globals.es2021 }, | |
| }, | |
| rules: { | |
| // Allow while (true) streaming loops without per-line disables | |
| 'no-constant-condition': ['warn', { checkLoops: false }], | |
| // Allow intentionally empty catch blocks (used across app) | |
| 'no-empty': ['warn', { allowEmptyCatch: true }], | |
| }, | |
| }, | |
| // TypeScript settings | |
| { | |
| files: ['**/*.{ts,tsx}'], | |
| languageOptions: { | |
| parser: require('@typescript-eslint/parser'), | |
| parserOptions: { ecmaVersion: 'latest', sourceType: 'module' }, | |
| globals: { ...globals.browser, ...globals.es2021 }, | |
| }, | |
| plugins: { | |
| '@typescript-eslint': require('@typescript-eslint/eslint-plugin'), | |
| }, | |
| rules: { | |
| // Prefer TS-aware unused-vars and disable the base rule for TS files | |
| 'no-unused-vars': 'off', | |
| '@typescript-eslint/no-unused-vars': ['warn', { argsIgnorePattern: '^_', varsIgnorePattern: '^_' }], | |
| '@typescript-eslint/no-explicit-any': 'off', | |
| // Mirror JS loop rule | |
| 'no-constant-condition': ['warn', { checkLoops: false }], | |
| // Empty catch blocks permitted (we use bare catch {}) | |
| 'no-empty': ['warn', { allowEmptyCatch: true }], | |
| }, | |
| linterOptions: { reportUnusedDisableDirectives: true }, | |
| }, | |
| ]; |
🤖 Prompt for AI Agents
In app/eslint.config.cjs around lines 1 to 31, the current flat config uses the
base no-unused-vars rule (conflicts with TypeScript), lacks browser globals, and
doesn't scope TypeScript-specific rules to TS files; replace it with a
file-scoped flat config that: disable the base 'no-unused-vars', add an
overrides entry for ['**/*.ts','**/*.tsx'] that sets the @typescript-eslint
parser, enables '@typescript-eslint/no-unused-vars' (warn/error as project
prefers), allows empty catch blocks (e.g.,
'@typescript-eslint/no-empty-function' or 'no-empty' exception for catches),
relaxes 'no-constant-condition' (or set 'no-constant-condition': ['warn', {
"checkLoops": false }] to allow streaming loops), and include 'env: { browser:
true }' or equivalent browser globals; keep
linterOptions.reportUnusedDisableDirectives true and ensure the plugin
'@typescript-eslint' remains registered.
| .demo-inner { | ||
| background: var(--bg-tertiary); | ||
| border-radius: 8px; | ||
| padding: 1rem; | ||
| border: 1px solid var(--falkor-border-tertiary); | ||
| border: none; | ||
| } |
There was a problem hiding this comment.
Fix duplicate border declaration on .demo-inner
You set border twice; the latter overrides the former and triggers the Biome error. Keep a single declaration to make intent explicit and silence the linter.
Apply this diff:
.demo-inner {
border-radius: 8px;
padding: 1rem;
- border: 1px solid var(--falkor-border-tertiary);
- border: none;
+ border: none;
}🧰 Tools
🪛 Biome (2.1.2)
[error] 127-127: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
border is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
🤖 Prompt for AI Agents
In app/public/css/landing.css around lines 123 to 128, .demo-inner contains
duplicate border declarations (first sets 1px solid
var(--falkor-border-tertiary), then border: none) which causes the latter to
override the former and triggers the linter; remove the redundant declaration so
there is a single explicit border rule — either keep the 1px solid
var(--falkor-border-tertiary) and delete "border: none" or if no border is
desired, remove the first declaration and keep "border: none" to make intent
explicit.
| .feature-card { | ||
| width: 280px; | ||
| background: var(--bg-tertiary); | ||
| border-radius: 6px; | ||
| padding: 0.9rem 1rem; | ||
| border: 1px solid var(--falkor-border-tertiary); | ||
| box-shadow: 0 8px 20px rgba(11, 18, 32, 0.04); | ||
| text-align: center; | ||
| transition: transform 180ms ease, box-shadow 180ms ease, border-color 180ms ease; | ||
| border: none; | ||
| } | ||
|
|
There was a problem hiding this comment.
Resolve duplicate border and avoid layout shift on hover for .feature-card
- Default rule sets a border and later resets it to none; Biome flags this as a duplicate.
- Hover adds a 1px border which can shift layout when the default has no border.
Prefer a transparent 1px border by default and only change border-color on hover. This removes duplication, prevents layout shift, and matches your existing transition for border-color.
Apply these diffs:
.feature-card {
- width: 280px;
- background: var(--bg-tertiary);
+ width: 280px;
+ background: var(--bg-tertiary);
border-radius: 6px;
padding: 0.9rem 1rem;
- border: 1px solid var(--falkor-border-tertiary);
+ border: 1px solid transparent;
box-shadow: 0 8px 20px rgba(11, 18, 32, 0.04);
text-align: center;
transition: transform 180ms ease, box-shadow 180ms ease, border-color 180ms ease;
- border: none;
} .feature-card:hover {
transform: translateY(-6px);
box-shadow: 0 20px 40px rgba(11, 18, 32, 0.18);
- border: solid 1px var(--falkor-border-secondary);
+ border-color: var(--falkor-border-secondary);
}Also applies to: 280-284
🧰 Tools
🪛 Biome (2.1.2)
[error] 277-277: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
border is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
🤖 Prompt for AI Agents
In app/public/css/landing.css around lines 268-279 (and also apply same change
to lines 280-284), the .feature-card currently declares a 1px solid border then
later resets border:none which creates a duplicate rule and causes layout shift
when hover adds a 1px border. Replace those contradictory declarations by
setting a default border: 1px solid transparent (remove the later border:none
and the earlier duplicate) and update any :hover rules to only change
border-color (not border width) so the existing transition on border-color
prevents layout shift.
| </div> | ||
| </aside> | ||
|
|
There was a problem hiding this comment.
Fix invalid nesting: stray closing before
There’s a closing immediately after the inline script (Line 156) that closes the parent container before the child (Line 157) is closed. This creates mis-nested HTML and can lead to unpredictable layout. Remove the stray and close .landing-container after the features section.
Apply these diffs:
-</div>
</aside>And add the container closing just before the endblock:
-{% endblock %}
+</div>
+{% endblock %}Also applies to: 209-211
🤖 Prompt for AI Agents
In app/templates/landing.j2 around lines 156-158 (and similarly 209-211), there
is a stray closing </div> placed before the closing </aside>, causing invalid
HTML nesting; remove this stray </div> so the aside closes correctly, then add a
single closing tag to terminate the .landing-container (e.g., </div>)
immediately before the template endblock to ensure the parent container is
closed only after the features/aside sections; verify both locations (156-158
and 209-211) are corrected and the final container close appears just before the
endblock.
- Bump litellm ~=1.82.0 → ~=1.83.0 (fixes CVE: auth bypass via OIDC cache key collision, privilege escalation via unrestricted proxy config) - Bump fastmcp >=2.13.1 → >=3.2.0 (fixes CVE: SSRF & path traversal in OpenAPI provider) - Add aiohttp>=3.13.4 constraint (fixes multiple HTTP-level CVEs: duplicate Host headers, header injection, multipart bypass, memory DoS, CRLF injection, cookie leaks, SSRF on Windows) - Add npm override for lodash-es to 4.18.1 (fixes prototype pollution via array path bypass, code injection via template imports) - Update fastmcp import path from deprecated fastmcp.server.openapi to fastmcp.server.providers.openapi Closes #125, #124, #123, #122, #121, #120, #119, #118, #117, #116, #115, #114, #113, #112, #111, #110, #109, #108, #107, #106, #105, #104, #103, #102, #101, #100, #99, #98, #97, #96 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 participant
Summary by CodeRabbit