Add Render deployment support with Lite Mode and static serving#477
Add Render deployment support with Lite Mode and static serving#477
Conversation
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
|
Reviewer's GuideAdds Render deployment support by introducing a setup script, serving a built React frontend as static files when available, and implementing a Render Lite Mode that skips heavy model loading, along with deployment documentation. Sequence diagram for root and SPA routing with static React frontendsequenceDiagram
actor User
participant Browser
participant FastAPI_app
participant StaticFiles
participant Gradio_UI
User->>Browser: Open /
Browser->>FastAPI_app: GET /
alt static_dist_exists
FastAPI_app->>StaticFiles: Read index.html
StaticFiles-->>FastAPI_app: index.html
FastAPI_app-->>Browser: 200 index.html
loop SPA navigation
User->>Browser: Navigate to /some/path
Browser->>FastAPI_app: GET /some/path
alt path_starts_with_api_or_ui
FastAPI_app-->>Browser: 404 Not found
else file_exists_in_static
FastAPI_app->>StaticFiles: Read requested file
StaticFiles-->>FastAPI_app: file contents
FastAPI_app-->>Browser: 200 file
else fallback_to_index
FastAPI_app->>StaticFiles: Read index.html
StaticFiles-->>FastAPI_app: index.html
FastAPI_app-->>Browser: 200 index.html
end
end
else static_dist_missing
FastAPI_app-->>Browser: 307 Redirect to /ui
Browser->>FastAPI_app: GET /ui
FastAPI_app->>Gradio_UI: Render Gradio app
Gradio_UI-->>FastAPI_app: HTML response
FastAPI_app-->>Browser: 200 Gradio UI
end
Flow diagram for setup_env.sh build and frontend handlingflowchart TD
A["Start setup_env.sh"] --> B["Install Python dependencies with pip install -e ."]
B --> C{"RENDER == true?"}
C -- "No" --> D["Skip frontend build"]
D --> Z["Echo build completed"]
C -- "Yes" --> E{"client directory exists?"}
E -- "No" --> F["Echo warning: client not found"]
F --> Z
E -- "Yes" --> G["cd client"]
G --> H["npm install"]
H --> I["npm run build (allow failure)"]
I --> J{"npm run build succeeded?"}
J -- "No" --> K["Echo warning: frontend build failed"]
K --> L["cd .. if still in client"]
L --> Z
J -- "Yes" --> M{"dist directory exists?"}
M -- "No" --> N["Echo warning: dist not found"]
N --> L
M -- "Yes" --> O["cd .."]
O --> P["mkdir -p static/dist"]
P --> Q["cp -r client/dist/* static/dist/"]
Q --> R["ls -F static/dist/"]
R --> Z
Z["Build process completed successfully"]
Flow diagram for Render Lite Mode model loading decisionsflowchart TD
A["Request requires model load"] --> B["Determine model type (sklearn, transformers, tensorflow)"]
B --> C["_load_sklearn_model"]
B --> D["_load_transformers_model"]
B --> E["_load_tensorflow_model"]
C --> F{"RENDER environment variable set?"}
D --> G{"RENDER environment variable set?"}
E --> H{"RENDER environment variable set?"}
F -- "Yes" --> I["Log skipping heavy model load for model_id in Render Lite Mode"]
G -- "Yes" --> I
H -- "Yes" --> I
I --> J["Return None (no heavy model loaded)"]
F -- "No" --> K["Import joblib and load sklearn model"]
G -- "No" --> L["Import transformers and load transformers model"]
H -- "No" --> M["Import tensorflow and load tensorflow model"]
K --> N["Return loaded sklearn model"]
L --> O["Return loaded transformers model"]
M --> P["Return loaded tensorflow model"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
🤖 Hi @MasumRab, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughAdds a Render (free tier) deployment guide and build script, introduces a Render "lite" mode that disables heavy model loading, adds static React frontend serving and SPA routing (with path-traversal protections), and patches CI workflow steps to guard Gemini/Google CLI execution; includes tooling to apply those workflow patches and a small performance-monitoring fix. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant CDN as StaticFiles (static/dist)
participant FastAPI
participant ModelRegistry
participant Storage as Local JSON
Browser->>CDN: Request / or /app-route
alt Static asset exists
CDN-->>Browser: index.html / asset file
else SPA route (fallback)
Browser->>FastAPI: GET /{full_path}
FastAPI->>FastAPI: sanitize path, check bounds
alt file inside static/dist
FastAPI-->>Browser: FileResponse
else
FastAPI-->>Browser: FileResponse(index.html)
end
end
Browser->>FastAPI: API request (/api/...)
FastAPI->>ModelRegistry: load model? (checks RENDER)
alt RENDER=true
ModelRegistry-->>FastAPI: return no-op / None (lite mode)
else
ModelRegistry-->>FastAPI: load heavy model
end
FastAPI->>Storage: read/write ephemeral JSON (if used)
FastAPI-->>Browser: API response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🤖 I'm sorry @MasumRab, but I was unable to process your request. Please see the logs for more details. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In the SPA catch-all route, the
return FileResponse(file_path)line appears over-indented relative to itsifblock, which will cause a syntax error; align it with theifblock body. - The checks for Render Lite Mode in the model loaders use
if os.environ.get("RENDER"), which will treat any non-empty value (including 'false') as enabled; consider explicitly checking for"true"(or a small set of truthy values) to avoid accidental activation. - DEPLOY_RENDER.md suggests starting Uvicorn with
src.backend.python_backend.main:app, but the main FastAPI app now lives insrc/main.py; please verify the start command matches the actual module path so deployments don’t fail at runtime.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the SPA catch-all route, the `return FileResponse(file_path)` line appears over-indented relative to its `if` block, which will cause a syntax error; align it with the `if` block body.
- The checks for Render Lite Mode in the model loaders use `if os.environ.get("RENDER")`, which will treat any non-empty value (including 'false') as enabled; consider explicitly checking for `"true"` (or a small set of truthy values) to avoid accidental activation.
- DEPLOY_RENDER.md suggests starting Uvicorn with `src.backend.python_backend.main:app`, but the main FastAPI app now lives in `src/main.py`; please verify the start command matches the actual module path so deployments don’t fail at runtime.
## Individual Comments
### Comment 1
<location> `src/main.py:722-723` </location>
<code_context>
+
+ # Check if file exists in static dir
+ file_path = os.path.join(static_dir, full_path)
+ if os.path.exists(file_path) and os.path.isfile(file_path):
+ return FileResponse(file_path)
+
+ # Otherwise return index.html
</code_context>
<issue_to_address>
**issue (bug_risk):** Fix indentation on the FileResponse line to avoid a syntax error.
There’s an extra leading space before `return FileResponse(file_path)`, which will raise an `IndentationError`. Align this `return` with the `if` body so it’s exactly one level deeper than the `if` statement.
</issue_to_address>
### Comment 2
<location> `src/core/model_registry.py:467-468` </location>
<code_context>
async def _load_sklearn_model(self, metadata: ModelMetadata) -> Optional[Any]:
"""Load a scikit-learn model."""
+
+ if os.environ.get("RENDER"):
+ logger.info(f"Skipping heavy model load for {metadata.model_id} in Render Lite Mode")
+ return None
try:
</code_context>
<issue_to_address>
**suggestion:** Clarify the environment flag semantics for skipping model loads.
`if os.environ.get("RENDER")` treats any non-empty value as enabling lite mode, so `RENDER=0` or `RENDER=foo` would also skip model loads, not just `RENDER=true` as in the setup script. If you expect this flag to grow in meaning, consider normalizing to explicit values (e.g., only treating `"true"`/`"1"` as enabled) or wrapping this in a helper like `is_render_lite_mode()` so the behavior is defined in one place.
Suggested implementation:
```python
import json
import logging
import os
import time
from dataclasses import dataclass, field
from enum import Enum
def is_render_lite_mode() -> bool:
"""Return True when running in Render 'lite' mode (skip heavy model loads).
Only explicit truthy values enable lite mode, so that values like '0' or
'false' behave as disabled. This keeps the semantics stable if the RENDER
variable later grows additional meanings.
"""
value = os.environ.get("RENDER", "").strip().lower()
return value in {"1", "true", "yes", "on"}
```
```python
async def _load_sklearn_model(self, metadata: ModelMetadata) -> Optional[Any]:
"""Load a scikit-learn model."""
if is_render_lite_mode():
logger.info(f"Skipping heavy model load for {metadata.model_id} in Render Lite Mode")
return None
try:
import joblib
```
```python
async def _load_transformers_model(self, metadata: ModelMetadata) -> Optional[Any]:
"""Load a transformers model."""
if is_render_lite_mode():
logger.info(f"Skipping heavy model load for {metadata.model_id} in Render Lite Mode")
return None
try:
```
</issue_to_address>
### Comment 3
<location> `DEPLOY_RENDER.md:41-45` </location>
<code_context>
+Once deployed:
+* Navigate to your Render URL (e.g., `https://email-intelligence.onrender.com`).
+* The React frontend should load.
+* The `/api/health` (if exists) or other API endpoints should work.
+* **Note**: Local AI features will be disabled.
+
</code_context>
<issue_to_address>
**suggestion (typo):** Consider rephrasing "(if exists)" to "if it exists" or "if present" for correct grammar.
A possible revision: "The `/api/health` endpoint, if it exists, or other API endpoints should work."
```suggestion
Once deployed:
* Navigate to your Render URL (e.g., `https://email-intelligence.onrender.com`).
* The React frontend should load.
* The `/api/health` endpoint, if it exists, or other API endpoints should work.
* **Note**: Local AI features will be disabled.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if os.environ.get("RENDER"): | ||
| logger.info(f"Skipping heavy model load for {metadata.model_id} in Render Lite Mode") |
There was a problem hiding this comment.
suggestion: Clarify the environment flag semantics for skipping model loads.
if os.environ.get("RENDER") treats any non-empty value as enabling lite mode, so RENDER=0 or RENDER=foo would also skip model loads, not just RENDER=true as in the setup script. If you expect this flag to grow in meaning, consider normalizing to explicit values (e.g., only treating "true"/"1" as enabled) or wrapping this in a helper like is_render_lite_mode() so the behavior is defined in one place.
Suggested implementation:
import json
import logging
import os
import time
from dataclasses import dataclass, field
from enum import Enum
def is_render_lite_mode() -> bool:
"""Return True when running in Render 'lite' mode (skip heavy model loads).
Only explicit truthy values enable lite mode, so that values like '0' or
'false' behave as disabled. This keeps the semantics stable if the RENDER
variable later grows additional meanings.
"""
value = os.environ.get("RENDER", "").strip().lower()
return value in {"1", "true", "yes", "on"} async def _load_sklearn_model(self, metadata: ModelMetadata) -> Optional[Any]:
"""Load a scikit-learn model."""
if is_render_lite_mode():
logger.info(f"Skipping heavy model load for {metadata.model_id} in Render Lite Mode")
return None
try:
import joblib async def _load_transformers_model(self, metadata: ModelMetadata) -> Optional[Any]:
"""Load a transformers model."""
if is_render_lite_mode():
logger.info(f"Skipping heavy model load for {metadata.model_id} in Render Lite Mode")
return None
try:| Once deployed: | ||
| * Navigate to your Render URL (e.g., `https://email-intelligence.onrender.com`). | ||
| * The React frontend should load. | ||
| * The `/api/health` (if exists) or other API endpoints should work. | ||
| * **Note**: Local AI features will be disabled. |
There was a problem hiding this comment.
suggestion (typo): Consider rephrasing "(if exists)" to "if it exists" or "if present" for correct grammar.
A possible revision: "The /api/health endpoint, if it exists, or other API endpoints should work."
| Once deployed: | |
| * Navigate to your Render URL (e.g., `https://email-intelligence.onrender.com`). | |
| * The React frontend should load. | |
| * The `/api/health` (if exists) or other API endpoints should work. | |
| * **Note**: Local AI features will be disabled. | |
| Once deployed: | |
| * Navigate to your Render URL (e.g., `https://email-intelligence.onrender.com`). | |
| * The React frontend should load. | |
| * The `/api/health` endpoint, if it exists, or other API endpoints should work. | |
| * **Note**: Local AI features will be disabled. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
setup_env.sh (2)
18-18: Prefernpm ciovernpm installfor reproducible CI/CD builds
npm ciinstalls exactly frompackage-lock.json, enforces a cleannode_modules, and fails fast on lock-file mismatches — all desirable in a build script.♻️ Proposed fix
- npm install + npm ci🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup_env.sh` at line 18, Replace the plain "npm install" command in setup_env.sh with "npm ci" to ensure deterministic installs in CI; locate the line containing the "npm install" invocation and update it to use "npm ci" (ensuring package-lock.json is present) so the script enforces a clean, lockfile-driven install.
9-9: Preferpip install .overpip install -e .for production buildsEditable installs are intended for development workflows. For a Render production build, use the non-editable install to fully copy the package into
site-packagesand avoid any dependency on source-tree path resolution.♻️ Proposed fix
-pip install -e . +pip install .🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup_env.sh` at line 9, Replace the editable-install command "pip install -e ." with a non-editable install "pip install ." in the setup script so production builds copy the package into site-packages; update any build or CI scripts that call "pip install -e ." to use the non-editable form and move development-only deps to a separate dev requirements file if necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@DEPLOY_RENDER.md`:
- Line 32: The table entry claiming users must set `RENDER=true` is
misleading—Render injects the `RENDER` env var automatically; update the
documentation (the table row for `RENDER`) to state that `RENDER` is provided by
the Render platform and does not need to be added manually, and add a short note
referencing the `setup_env.sh` check (`[ "$RENDER" = "true" ]`) to explain it
will run automatically on Render without user action.
- Line 34: Update the documented Node.js patch version for `NODE_VERSION` in the
DEPLOY_RENDER.md table row (the line containing the `NODE_VERSION` | `20.0.0`
entry) to a stable LTS patch such as `20.11.0` (or later); replace the literal
`20.0.0` with the chosen LTS value and ensure any accompanying text still states
it enforces the correct Node.js version for the frontend build.
- Line 25: Update the deployment start command to use the current entry point
function create_app from src.main instead of the deprecated
src.backend.python_backend.main:app; modify the command string used for starting
Uvicorn to reference src.main:create_app and include the --factory flag along
with the existing host and port options so the app factory pattern is invoked at
runtime.
In `@src/core/model_registry.py`:
- Around line 467-469: Replace the loose truthy check os.environ.get("RENDER")
with a strict comparison os.environ.get("RENDER") == "true" in all places that
short-circuit model loading (the blocks that log "Skipping heavy model load for
{metadata.model_id} in Render Lite Mode" and return None); apply the same change
to the two other identical guards in this module so only RENDER explicitly set
to "true" triggers the Render Lite behavior.
In `@src/main.py`:
- Around line 717-718: The current check using full_path.startswith("api") /
.startswith("ui") is too broad and rejects routes like "apiary/..." or
"uikit/..."; update the condition in the handler that raises HTTPException to
only match the exact segment or a segment followed by a slash — e.g. check
(full_path == "api" or full_path.startswith("api/")) and similarly for "ui" — so
only true API/UI prefixes are excluded and SPA routes like "apiary/..." and
"uikit/..." will continue to serve index.html; adjust the logic around the
existing full_path and HTTPException usage accordingly.
- Around line 721-723: The code currently joins static_dir and user-controlled
full_path then serves file_path which allows path traversal; update the logic in
the handler that computes file_path (using static_dir, full_path, and
FileResponse) to normalize and confine the resolved path before serving: compute
the real/absolute path of static_dir and of the joined file_path
(os.path.realpath or os.path.abspath), ensure the resolved file_path starts with
the resolved static_dir prefix (and is a file) and only then call
FileResponse(file_path); if the check fails, return a safe error/404 instead of
serving the file.
---
Nitpick comments:
In `@setup_env.sh`:
- Line 18: Replace the plain "npm install" command in setup_env.sh with "npm ci"
to ensure deterministic installs in CI; locate the line containing the "npm
install" invocation and update it to use "npm ci" (ensuring package-lock.json is
present) so the script enforces a clean, lockfile-driven install.
- Line 9: Replace the editable-install command "pip install -e ." with a
non-editable install "pip install ." in the setup script so production builds
copy the package into site-packages; update any build or CI scripts that call
"pip install -e ." to use the non-editable form and move development-only deps
to a separate dev requirements file if necessary.
| * **Root Directory**: `.` (leave empty) | ||
| * **Runtime**: **Python 3** | ||
| * **Build Command**: `./setup_env.sh` | ||
| * **Start Command**: `uvicorn src.backend.python_backend.main:app --host 0.0.0.0 --port $PORT` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for src/main.py
echo "=== Checking src/main.py ==="
if [ -f "src/main.py" ]; then
echo "File exists"
echo "Content:"
cat -n src/main.py
else
echo "File does not exist"
fi
echo -e "\n=== Checking src/backend/python_backend/main.py ==="
if [ -f "src/backend/python_backend/main.py" ]; then
echo "File exists"
echo "Content:"
cat -n src/backend/python_backend/main.py
else
echo "File does not exist"
fi
echo -e "\n=== Searching for main.py files under src/ ==="
fd "main.py" src/ --type f 2>/dev/null || echo "No main.py files found under src/"Repository: MasumRab/EmailIntelligence
Length of output: 50382
🏁 Script executed:
cat -n DEPLOY_RENDER.mdRepository: MasumRab/EmailIntelligence
Length of output: 2736
Update start command to reference the current entry point instead of a deprecated module
The start command on line 25 references src.backend.python_backend.main:app, which is explicitly marked deprecated and slated for removal. The current, active entry point is defined in src/main.py and uses a factory pattern. Update the start command to:
uvicorn src.main:create_app --factory --host 0.0.0.0 --port $PORT
Using the deprecated module means the deployment will break when that module is removed in a future release.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@DEPLOY_RENDER.md` at line 25, Update the deployment start command to use the
current entry point function create_app from src.main instead of the deprecated
src.backend.python_backend.main:app; modify the command string used for starting
Uvicorn to reference src.main:create_app and include the --factory flag along
with the existing host and port options so the app factory pattern is invoked at
runtime.
|
|
||
| | Key | Value | Description | | ||
| | :--- | :--- | :--- | | ||
| | `RENDER` | `true` | Activates "Lite Mode" and triggers frontend build. | |
There was a problem hiding this comment.
RENDER=true is automatically injected by the Render platform — manual entry is redundant
Render sets this value to "true" for all runtimes — code can check this value to detect whether it's running on Render. The table entry in Step 5 is therefore unnecessary and may give users the impression that Lite Mode requires manual activation. Consider updating the note to clarify that RENDER is already set automatically, so users don't need to add it. This also makes setup_env.sh's [ "$RENDER" = "true" ] check activate automatically on Render without any user action.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@DEPLOY_RENDER.md` at line 32, The table entry claiming users must set
`RENDER=true` is misleading—Render injects the `RENDER` env var automatically;
update the documentation (the table row for `RENDER`) to state that `RENDER` is
provided by the Render platform and does not need to be added manually, and add
a short note referencing the `setup_env.sh` check (`[ "$RENDER" = "true" ]`) to
explain it will run automatically on Render without user action.
| if os.environ.get("RENDER"): | ||
| logger.info(f"Skipping heavy model load for {metadata.model_id} in Render Lite Mode") | ||
| return None |
There was a problem hiding this comment.
RENDER guard matches any non-empty value — use strict == "true" comparison
os.environ.get("RENDER") is truthy for any non-empty string ("false", "0", "no", etc.). This diverges from setup_env.sh's strict [ "$RENDER" = "true" ] check, meaning a developer who sets RENDER=false locally (to disable Render mode) would still skip all model loading silently.
🐛 Proposed fix (apply to all three methods)
- if os.environ.get("RENDER"):
+ if os.environ.get("RENDER") == "true":Also applies to: 488-490, 509-511
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/model_registry.py` around lines 467 - 469, Replace the loose truthy
check os.environ.get("RENDER") with a strict comparison os.environ.get("RENDER")
== "true" in all places that short-circuit model loading (the blocks that log
"Skipping heavy model load for {metadata.model_id} in Render Lite Mode" and
return None); apply the same change to the two other identical guards in this
module so only RENDER explicitly set to "true" triggers the Render Lite
behavior.
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
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)
src/core/performance_monitor.py (1)
316-325:⚠️ Potential issue | 🟡 MinorDuplicate
log_performancemethod — second definition silently shadows the first
log_performanceis defined at line 268 and again at line 316 on the same class. Python will silently use only the second definition. If the first implementation (lines 268–277) was intentional, it's dead code; if not, this duplicate should be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/performance_monitor.py` around lines 316 - 325, There are two definitions of the method log_performance in the same class (the earlier implementation around the first occurrence is being silently shadowed by the second); remove or consolidate the duplicate so only the intended implementation remains — locate both log_performance definitions, decide which behavior to keep (the earlier decorator-compatibility logic or the later record_metric-based version), delete the unwanted definition, and ensure any callers of log_performance still work with the retained implementation (refer to the method name log_performance and the helper record_metric to find and reconcile the implementations).
🧹 Nitpick comments (4)
src/main.py (2)
716-716: Duplicate commentLine 715 and 716 contain the same comment:
# Catch-all for SPA routing (excluding API and UI).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.py` at line 716, Remove the duplicate comment "# Catch-all for SPA routing (excluding API and UI)" so it appears only once immediately above the SPA route handler; locate the two identical comment lines and delete the redundant one, keeping the comment once to maintain clarity and consistent spacing around the catch-all route block.
699-703:static_diris resolved relative to CWD — fragile in production
os.path.join(os.getcwd(), "static", "dist")depends on the working directory at startup, which varies across deployment setups (e.g., container entrypoints, process managers). Consider anchoring to the project or module root:# Relative to this file's location static_dir = os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "static", "dist")This is less error-prone when the application is started from a non-root directory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.py` around lines 699 - 703, The static_dir is currently built from os.getcwd(), which is fragile in production; change the resolution in the block that defines static_dir (the os.path.join(...) call) to anchor it to this module's location by using os.path.dirname(os.path.abspath(__file__)) (or equivalent) as the base and then join the "..", "static", "dist" segments and abspath/normpath the result so the server reliably finds the static assets regardless of CWD.fix_gemini_workflows.py (1)
1-50: One-off patching script committed to the repo — consider removing after useThis script modifies workflow files in-place using string manipulation rather than a proper YAML parser. It serves its purpose as a one-time migration tool, but leaving it in the repository may cause confusion. Consider either removing it after use or adding a clear docstring explaining that it's a one-time utility not intended for repeated execution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fix_gemini_workflows.py` around lines 1 - 50, This file is a one-off in-repo patcher (workflows list, patch_content function) and should not remain as-is; either remove it after running or make its one-time intent explicit and safe: add a module-level docstring stating it's a one-time migration tool, move it into a non-imported scripts/tools folder (or rename to scripts/one_off_patch.py), wrap execution in a guarded main block (if __name__ == "__main__":) so it won't run on import, and add a short README note describing usage and that it should be deleted after use; reference the workflows variable and patch_content function so reviewers can locate the logic to delete or relocate.fix_path_traversal.py (1)
1-50: One-off patching script — same concerns asfix_gemini_workflows.pyLike the other patching script, this modifies source files via string manipulation. Consider removing it from the repository after use. The path traversal fix it injects is reviewed in detail on
src/main.py.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fix_path_traversal.py` around lines 1 - 50, This one-off patching script (fix_path_traversal.py) performs unsafe string-based modifications to src/main.py by injecting the new_logic catch_all block; remove this script from the repository after applying the changes and instead commit the intended changes directly to src/main.py (or apply them via a proper VCS patch/PR) to avoid accidental re-runs, brittle edits, and maintenance hazards; locate the injected function by the `@app.get`("/{full_path:path}") decorator and the catch_all function name and ensure the sanitized logic (os.path.normpath, os.path.commonpath check against static_dir, FileResponse fallback to index.html) is present in src/main.py before deleting fix_path_traversal.py.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/gemini-invoke.yml:
- Around line 42-43: Step-level if uses secrets directly (not allowed) so create
a preceding step (e.g., "set-credentials-env") that writes the secrets into
environment variables using the GitHub Actions mechanism (run: echo
"GEMINI_API_KEY=${{ secrets.GEMINI_API_KEY }}" >> $GITHUB_ENV and similarly for
GOOGLE_API_KEY), then update the problematic if expression (the line checking
${{ secrets.GEMINI_API_KEY != '' || secrets.GOOGLE_API_KEY != '' ||
vars.GCP_WIF_PROVIDER != '' }}) to test env instead: ${{ env.GEMINI_API_KEY !=
'' || env.GOOGLE_API_KEY != '' || vars.GCP_WIF_PROVIDER != '' }} so the
step-level condition can evaluate correctly.
In @.github/workflows/gemini-review.yml:
- Around line 45-46: The step-level if referencing secrets.GEMINI_API_KEY and
secrets.GOOGLE_API_KEY is invalid for workflow_call-triggered workflows; add a
preliminary step (e.g., "check-credentials" or similar) that reads those secrets
into env and sets a boolean output (like credentials_present) based on checking
env.GEMINI_API_KEY, env.GOOGLE_API_KEY, or vars.GCP_WIF_PROVIDER, then replace
the current step-level if (${{ secrets.GEMINI_API_KEY != '' ||
secrets.GOOGLE_API_KEY != '' || vars.GCP_WIF_PROVIDER != '' }}) with a check
against that step output (e.g., if:
steps.check-credentials.outputs.credentials_present == 'true') so the step uses
the computed output rather than inaccessible secrets in the if expression.
In `@fix_gemini_workflows.py`:
- Around line 29-32: The injected step-level if condition uses the secrets
context (secrets.GEMINI_API_KEY, secrets.GOOGLE_API_KEY) which isn’t available
in workflow_call step ifs; update the string built in fix_gemini_workflows.py
(the condition variable created where has_if is false, using indent and appended
to new_lines) to reference a supported context such as vars or inputs (e.g.
vars.GEMINI_API_KEY and vars.GOOGLE_API_KEY or the appropriate input names)
instead of secrets, so the generated workflow uses only contexts valid in step
if expressions.
---
Outside diff comments:
In `@src/core/performance_monitor.py`:
- Around line 316-325: There are two definitions of the method log_performance
in the same class (the earlier implementation around the first occurrence is
being silently shadowed by the second); remove or consolidate the duplicate so
only the intended implementation remains — locate both log_performance
definitions, decide which behavior to keep (the earlier decorator-compatibility
logic or the later record_metric-based version), delete the unwanted definition,
and ensure any callers of log_performance still work with the retained
implementation (refer to the method name log_performance and the helper
record_metric to find and reconcile the implementations).
---
Duplicate comments:
In `@src/main.py`:
- Around line 719-720: The prefix checks for routes use
full_path.startswith("api") / startswith("ui") which incorrectly matches strings
like "apiary/..." or "uikit/..."; change the condition to anchor the prefix by
checking exact segment boundaries, e.g. replace full_path.startswith("api") with
(full_path == "api" or full_path.startswith("api/")) and likewise for "ui"
(full_path == "ui" or full_path.startswith("ui/")), or use a split-based check
like full_path.split("/", 1)[0] in ("api", "ui") to ensure only the intended
route prefixes are excluded; update the if that currently references
full_path.startswith to use one of these anchored checks.
- Around line 722-732: The current check using os.path.normpath and
os.path.commonpath can be bypassed via symlinks; update the logic that builds
clean_path and file_path to resolve real filesystem paths before comparing:
compute real_static = os.path.realpath(static_dir) and real_path =
os.path.realpath(os.path.join(real_static, clean_path)) (or call realpath on
file_path) and then ensure real_path starts with real_static (or use
os.path.commonpath([real_static, real_path]) == real_static) before returning
FileResponse; reference symbols: clean_path, file_path, static_dir, and the
authorization check that raises HTTPException.
---
Nitpick comments:
In `@fix_gemini_workflows.py`:
- Around line 1-50: This file is a one-off in-repo patcher (workflows list,
patch_content function) and should not remain as-is; either remove it after
running or make its one-time intent explicit and safe: add a module-level
docstring stating it's a one-time migration tool, move it into a non-imported
scripts/tools folder (or rename to scripts/one_off_patch.py), wrap execution in
a guarded main block (if __name__ == "__main__":) so it won't run on import, and
add a short README note describing usage and that it should be deleted after
use; reference the workflows variable and patch_content function so reviewers
can locate the logic to delete or relocate.
In `@fix_path_traversal.py`:
- Around line 1-50: This one-off patching script (fix_path_traversal.py)
performs unsafe string-based modifications to src/main.py by injecting the
new_logic catch_all block; remove this script from the repository after applying
the changes and instead commit the intended changes directly to src/main.py (or
apply them via a proper VCS patch/PR) to avoid accidental re-runs, brittle
edits, and maintenance hazards; locate the injected function by the
`@app.get`("/{full_path:path}") decorator and the catch_all function name and
ensure the sanitized logic (os.path.normpath, os.path.commonpath check against
static_dir, FileResponse fallback to index.html) is present in src/main.py
before deleting fix_path_traversal.py.
In `@src/main.py`:
- Line 716: Remove the duplicate comment "# Catch-all for SPA routing (excluding
API and UI)" so it appears only once immediately above the SPA route handler;
locate the two identical comment lines and delete the redundant one, keeping the
comment once to maintain clarity and consistent spacing around the catch-all
route block.
- Around line 699-703: The static_dir is currently built from os.getcwd(), which
is fragile in production; change the resolution in the block that defines
static_dir (the os.path.join(...) call) to anchor it to this module's location
by using os.path.dirname(os.path.abspath(__file__)) (or equivalent) as the base
and then join the "..", "static", "dist" segments and abspath/normpath the
result so the server reliably finds the static assets regardless of CWD.
| if: |- | ||
| ${{ secrets.GEMINI_API_KEY != '' || secrets.GOOGLE_API_KEY != '' || vars.GCP_WIF_PROVIDER != '' }} |
There was a problem hiding this comment.
Same secrets context issue as in gemini-invoke.yml
This if condition references secrets.GEMINI_API_KEY and secrets.GOOGLE_API_KEY, which are not available in step-level if expressions for workflow_call-triggered workflows. Apply the same workaround (e.g., a prior step that checks credentials via env and sets an output).
🧰 Tools
🪛 actionlint (1.7.11)
[error] 45-45: context "secrets" is not allowed here. available contexts are "env", "github", "inputs", "job", "matrix", "needs", "runner", "steps", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
[error] 45-45: context "secrets" is not allowed here. available contexts are "env", "github", "inputs", "job", "matrix", "needs", "runner", "steps", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/gemini-review.yml around lines 45 - 46, The step-level if
referencing secrets.GEMINI_API_KEY and secrets.GOOGLE_API_KEY is invalid for
workflow_call-triggered workflows; add a preliminary step (e.g.,
"check-credentials" or similar) that reads those secrets into env and sets a
boolean output (like credentials_present) based on checking env.GEMINI_API_KEY,
env.GOOGLE_API_KEY, or vars.GCP_WIF_PROVIDER, then replace the current
step-level if (${{ secrets.GEMINI_API_KEY != '' || secrets.GOOGLE_API_KEY != ''
|| vars.GCP_WIF_PROVIDER != '' }}) with a check against that step output (e.g.,
if: steps.check-credentials.outputs.credentials_present == 'true') so the step
uses the computed output rather than inaccessible secrets in the if expression.
fix_gemini_workflows.py
Outdated
| if not has_if: | ||
| # Add the condition. | ||
| condition = f"{' ' * indent}if: |-\n{' ' * (indent + 2)}${{{{ secrets.GEMINI_API_KEY != '' || secrets.GOOGLE_API_KEY != '' || vars.GCP_WIF_PROVIDER != '' }}}}" | ||
| new_lines.append(condition) |
There was a problem hiding this comment.
Injected if condition uses secrets context — same issue as flagged in the workflow files
The condition injected on line 31 references secrets.GEMINI_API_KEY and secrets.GOOGLE_API_KEY in a step-level if. As noted in the workflow file reviews, the secrets context is reportedly not available in step if expressions for workflow_call-triggered workflows. This script is the root cause — fix the template here and the patched workflows will be correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fix_gemini_workflows.py` around lines 29 - 32, The injected step-level if
condition uses the secrets context (secrets.GEMINI_API_KEY,
secrets.GOOGLE_API_KEY) which isn’t available in workflow_call step ifs; update
the string built in fix_gemini_workflows.py (the condition variable created
where has_if is false, using indent and appended to new_lines) to reference a
supported context such as vars or inputs (e.g. vars.GEMINI_API_KEY and
vars.GOOGLE_API_KEY or the appropriate input names) instead of secrets, so the
generated workflow uses only contexts valid in step if expressions.
|
❌ The last analysis has failed. |
🧪 CI InsightsHere's what we observed from your CI run for 2a3f195. 🟢 All jobs passed!But CI Insights is watching 👀 |
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
| >>>>>>> a7da61cf1f697de3c8c81f536bf579d36d88e613 | ||
| # For poetry, we need to install it first if not available | ||
| try: | ||
| subprocess.run([python_exe, "-c", "import poetry"], check=True, capture_output=True) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
| >>>>>>> a7da61cf1f697de3c8c81f536bf579d36d88e613 | ||
| # For uv, install if not available | ||
| try: | ||
| subprocess.run([python_exe, "-c", "import uv"], check=True, capture_output=True) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
| try: | ||
| # Resolve the requested path relative to static_dir | ||
| # lstrip('/') ensures it's treated as relative | ||
| requested_path = (static_path / full_path.lstrip('/')).resolve() |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Copilot Autofix
AI about 1 month ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| raise HTTPException(status_code=403, detail="Access denied") | ||
|
|
||
| # Check if file exists and is a file | ||
| if requested_path.exists() and requested_path.is_file(): |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Copilot Autofix
AI about 1 month ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| raise HTTPException(status_code=403, detail="Access denied") | ||
|
|
||
| # Check if file exists and is a file | ||
| if requested_path.exists() and requested_path.is_file(): |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
General approach: normalize the user-influenced path relative to a fixed, safe root directory, then enforce that the normalized path remains within that root before using it. Avoid using APIs that may not exist in all supported Python versions or that static analysis doesn’t understand; prefer the canonical os.path.realpath/os.path.normpath + prefix check.
Best concrete fix here:
- Keep
static_pathas the resolved root for static files. - Replace the use of
requested_path.is_relative_to(static_path)with an explicit check thatrequested_pathstarts withstatic_path(as a directory boundary), usingos.path.commonpathor a normalized string-prefix as in the background example. - Continue to use
requested_path.exists()and.is_file()before serving withFileResponse.
Within src/main.py, around lines 725–739:
-
After computing
requested_path = (static_path / full_path.lstrip('/')).resolve(), convert bothrequested_pathandstatic_pathto strings and perform a normalized prefix check that ensuresrequested_pathdoes not escapestatic_path. For example:requested_path_str = os.path.realpath(str(requested_path)) static_path_str = os.path.realpath(str(static_path)) if not requested_path_str.startswith(static_path_str + os.sep): raise HTTPException(status_code=403, detail="Access denied")
-
Then use
requested_path_str(or convert back toPath(requested_path_str)) for the existence and file checks, and forFileResponse.
This keeps the functional behavior (serving files under static_dir and falling back to index.html) while strengthening the containment logic in a way that’s compatible with static analysis and all Python 3 versions. No new imports are necessary; os is already imported at the top of the file.
| @@ -731,12 +731,16 @@ | ||
| requested_path = (static_path / full_path.lstrip('/')).resolve() | ||
|
|
||
| # Verify the resolved path is still within static_dir | ||
| if not requested_path.is_relative_to(static_path): | ||
| # Use real paths and a prefix check to avoid directory traversal | ||
| static_path_str = os.path.realpath(str(static_path)) | ||
| requested_path_str = os.path.realpath(str(requested_path)) | ||
| if not requested_path_str.startswith(static_path_str + os.sep): | ||
| raise HTTPException(status_code=403, detail="Access denied") | ||
|
|
||
| # Check if file exists and is a file | ||
| if requested_path.exists() and requested_path.is_file(): | ||
| return FileResponse(str(requested_path)) | ||
| requested_path_safe = Path(requested_path_str) | ||
| if requested_path_safe.exists() and requested_path_safe.is_file(): | ||
| return FileResponse(str(requested_path_safe)) | ||
|
|
||
| # Otherwise return index.html for SPA | ||
| index_path = static_path / "index.html" |
|
|
||
| # Check if file exists and is a file | ||
| if requested_path.exists() and requested_path.is_file(): | ||
| return FileResponse(str(requested_path)) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
In general, to fix uncontrolled path usage you must ensure that any file path derived from user input is (1) normalized, and (2) verified to be within a predetermined safe root directory before being used in file operations. The usual strategy is to resolve both the base directory and the user-derived path to absolute, canonical forms and then check that the canonical user path is not able to escape the base directory, for example by comparing prefixes or using os.path.commonpath.
For this code, the best fix is to replace the use of Path.is_relative_to with a more explicit and broadly-supported check based on os.path.commonpath (or equivalent), while keeping the existing logic intact. We will:
- Keep
static_path = Path(static_dir).resolve()as the canonical base path. - Keep
requested_path = (static_path / full_path.lstrip('/')).resolve()to resolve the requested file path relative tostatic_path. - Replace the
requested_path.is_relative_to(static_path)call with a check usingos.path.commonpathon the string representations of both paths:- Compute
static_root = os.path.commonpath([str(static_path)]) - Compute
requested_root = os.path.commonpath([str(static_path), str(requested_path)]) - If
requested_root != static_root, thenrequested_pathis outsidestatic_pathand access should be denied.
- Compute
- Keep the subsequent existence and
is_filechecks and the SPA fallback toindex.htmlunchanged.
This change is localized to the catch_all route in src/main.py (around lines 733–735). No new third-party dependencies are required; we already import os at line 3, and we only add uses of os.path.commonpath. Functionality remains the same—static assets are still served from static_dir, SPA routes still fall back to index.html, and API/UI paths are excluded—but the path validation becomes explicit, robust across Python versions, and clearly satisfies CodeQL’s expectations.
| @@ -730,8 +730,10 @@ | ||
| # lstrip('/') ensures it's treated as relative | ||
| requested_path = (static_path / full_path.lstrip('/')).resolve() | ||
|
|
||
| # Verify the resolved path is still within static_dir | ||
| if not requested_path.is_relative_to(static_path): | ||
| # Verify the resolved path is still within static_dir using commonpath | ||
| static_root = os.path.commonpath([str(static_path)]) | ||
| requested_root = os.path.commonpath([str(static_path), str(requested_path)]) | ||
| if requested_root != static_root: | ||
| raise HTTPException(status_code=403, detail="Access denied") | ||
|
|
||
| # Check if file exists and is a file |
|
2 participants
Add Render deployment support with Lite Mode and static serving
setup_env.shto install dependencies (skipping heavy optional groups) and build the React frontend conditionally.src/backend/python_backend/main.py(actuallysrc/main.py) to serve static files fromstatic/distif present, falling back to Gradio redirect.src/core/model_registry.pyto skip heavy model loading (Transformers, TensorFlow, Sklearn) whenRENDER=trueis set.DEPLOY_RENDER.mdwith detailed deployment instructions.PR created automatically by Jules for task 8283781824745080007 started by @MasumRab
Summary by Sourcery
Add Render-friendly deployment support by serving an optional built React frontend from the backend and introducing a lite mode that skips heavy model loading.
New Features:
Enhancements:
Summary by CodeRabbit
New Features
Documentation
Bug Fixes / Security
Chores
Maintenance