Enable Gemma via Gemini API for Render Free Tier - #340
Open
MasumRab wants to merge 9 commits into
Open
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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR introduces support for Google GenAI (Gemma) as an alternative Gemma provider alongside Ollama and Vertex AI, updates deployment configuration for Render's free tier, refactors benchmark and visualization scripts, simplifies security middleware, and updates default model selections across the configuration stack. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
render.yaml (1)
7-27:⚠️ Potential issue | 🔴 CriticalMissing
ALLOWED_HOSTS—TrustedHostMiddlewarewill reject all Render traffic.The backend uses
TrustedHostMiddlewarewithallowed_hostsdefaulting tolocalhost,127.0.0.1(fromapp_config.pylines 48-50). On Render, the incomingHostheader will be<service-name>.onrender.com, which won't match the defaults. Every request will get a 400 response.Add an
ALLOWED_HOSTSenv var to include the Render domain:Proposed fix
- key: MODEL_COMPRESSION value: "gemma-2-27b-it" + - key: ALLOWED_HOSTS + value: "*" + - key: CORS_ORIGINS + value: "*"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@render.yaml` around lines 7 - 27, The app’s TrustedHostMiddleware (configured via allowed_hosts in app_config.py) will reject Render requests because ALLOWED_HOSTS is not set in envVars; add an env var entry named ALLOWED_HOSTS to the render.yaml envVars block that includes your Render service domain (e.g. <your-service>.onrender.com) and keep localhost and 127.0.0.1 so local dev still works; ensure the key is exactly ALLOWED_HOSTS and the value is a comma-separated list matching the format expected by the allowed_hosts parser used by TrustedHostMiddleware.backend/src/config/app_config.py (1)
53-59:⚠️ Potential issue | 🟠 MajorModel parameter ignored for Gemma clients—version mismatch between config defaults.
When
model_validationormodel_compression(defaulting togemma-3-27b-it) are passed to_get_rate_limited_llm(), the model name is discarded byget_cached_llm(). The function detects "gemma" in the model name and delegates toget_gemma_client(), which ignores the model parameter entirely. Instead,GoogleGenAIGemmaClientalways usesapp_config.gemma_model_name(gemma-2-27b-it).This creates a hidden mismatch: locally without env vars, the code defaults to planning/validation/compression using Gemma 3 but the actual Gemma client uses Gemma 2. While
render.yamloverrides all model vars togemma-2-27b-it(masking the issue in production), local development and any non-Render deployment will silently use the wrong model version.Fix: Either pass the model name to the Gemma client and respect it, or align the config defaults (
model_planning/validation/compressionshould default togemma-2-27b-itto matchgemma_model_name).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/config/app_config.py` around lines 53 - 59, The defaults for model_planning/model_validation/model_compression currently use "gemma-3-27b-it" while GoogleGenAIGemmaClient always uses app_config.gemma_model_name ("gemma-2-27b-it"), causing a silent version mismatch; either change the defaults for model_planning/model_validation/model_compression to "gemma-2-27b-it" to match gemma_model_name, or update get_cached_llm/_get_rate_limited_llm/get_gemma_client (and GoogleGenAIGemmaClient) to accept and respect a model_name parameter passed through so the model passed (e.g., model_validation/model_compression) is actually used by the Gemma client.
🧹 Nitpick comments (3)
backend/src/agent/gemma_client.py (1)
60-62: Use bareraiseinstead ofraise eto preserve the full traceback.
raise eresets the traceback origin to this line, making debugging harder. The same pattern appears inVertexAIGemmaClient.invoke(line 108) andOllamaGemmaClient.invoke(line 140).Proposed fix
except Exception as e: logger.error(f"Google GenAI (Gemma) call failed: {e}") - raise e + raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/gemma_client.py` around lines 60 - 62, The exception handlers in gemma_client.py currently re-raise exceptions with "raise e", which resets the traceback; update the handlers to use a bare "raise" instead. Find the blocks in Google GenAI call (the try/except around the Google Gemma client where logger.error is called), the VertexAIGemmaClient.invoke method, and the OllamaGemmaClient.invoke method, and replace "raise e" with "raise" while keeping the existing logger.error calls unchanged so the original traceback is preserved.backend/src/agent/app.py (1)
165-168: Consider using a 301 (permanent) redirect instead of the default 307 (temporary).
RedirectResponsedefaults tostatus_code=307. Since the root-to-frontend redirect is semantically permanent, a 301 would allow browsers to cache it, reducing latency on repeat visits—useful on Render's Free Tier where cold starts are already slow.Proposed fix
`@app.get`("/") async def root_redirect(): """Redirect root path to the frontend app.""" - return RedirectResponse(url="/app/") + return RedirectResponse(url="/app/", status_code=301)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/app.py` around lines 165 - 168, The root_redirect endpoint currently returns a RedirectResponse with the default 307 temporary status; change the RedirectResponse in async def root_redirect() to use a 301 status_code (RedirectResponse(url="/app/", status_code=301)) so the redirect is permanent and can be cached by browsers; update the return in root_redirect accordingly (no other logic changes needed).DEPLOY_RENDER.md (1)
56-60: Troubleshooting section should mentionALLOWED_HOSTS.As noted in the
render.yamlreview, theTrustedHostMiddlewaredefault will reject Render traffic. A "502 Bad Gateway" or "400 Bad Request" due to missingALLOWED_HOSTSconfig is a likely deployment issue that should be documented here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DEPLOY_RENDER.md` around lines 56 - 60, Add a Troubleshooting note instructing users to set ALLOWED_HOSTS to include Render hostnames because Django's TrustedHostMiddleware/ALLOWED_HOSTS default will reject Render traffic; mention that missing ALLOWED_HOSTS can cause 502/400 errors and reference TrustedHostMiddleware and render.yaml so they know where to update configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/agent/gemma_client.py`:
- Around line 34-38: The code creates genai.Client(api_key=None) when
GEMINI_API_KEY is missing which defers failure; update the initialization in
gemma_client.py so you don't instantiate genai.Client with a None key: check
os.getenv("GEMINI_API_KEY") and if it's falsy either log an error and raise
(e.g., ValueError or RuntimeError) or return/disable the client, and only call
genai.Client(api_key=api_key) when api_key is present; modify the code that sets
self.client (the genai.Client instantiation) accordingly and reference
GEMINI_API_KEY, logger, and the genai.Client/self.client symbols to implement
the fail-fast behavior.
In `@backend/src/config/app_config.py`:
- Line 58: The change of gemma_provider default to "google_genai" is breaking
for users who relied on Ollama; revert the default in app_config.py by setting
gemma_provider back to "ollama" and also harden get_gemma_client(): before
instantiating GoogleGenAIGemmaClient, check that the GEMINI_API_KEY (or
equivalent config) is present and only construct GoogleGenAIGemmaClient when the
key exists—otherwise log a clear warning and fall back to Ollama (or raise a
clear error); ensure GoogleGenAIGemmaClient.__init__ is never called with
api_key=None so the app won't attempt to create a client that will fail at call
time.
---
Outside diff comments:
In `@backend/src/config/app_config.py`:
- Around line 53-59: The defaults for
model_planning/model_validation/model_compression currently use "gemma-3-27b-it"
while GoogleGenAIGemmaClient always uses app_config.gemma_model_name
("gemma-2-27b-it"), causing a silent version mismatch; either change the
defaults for model_planning/model_validation/model_compression to
"gemma-2-27b-it" to match gemma_model_name, or update
get_cached_llm/_get_rate_limited_llm/get_gemma_client (and
GoogleGenAIGemmaClient) to accept and respect a model_name parameter passed
through so the model passed (e.g., model_validation/model_compression) is
actually used by the Gemma client.
In `@render.yaml`:
- Around line 7-27: The app’s TrustedHostMiddleware (configured via
allowed_hosts in app_config.py) will reject Render requests because
ALLOWED_HOSTS is not set in envVars; add an env var entry named ALLOWED_HOSTS to
the render.yaml envVars block that includes your Render service domain (e.g.
<your-service>.onrender.com) and keep localhost and 127.0.0.1 so local dev still
works; ensure the key is exactly ALLOWED_HOSTS and the value is a
comma-separated list matching the format expected by the allowed_hosts parser
used by TrustedHostMiddleware.
---
Nitpick comments:
In `@backend/src/agent/app.py`:
- Around line 165-168: The root_redirect endpoint currently returns a
RedirectResponse with the default 307 temporary status; change the
RedirectResponse in async def root_redirect() to use a 301 status_code
(RedirectResponse(url="/app/", status_code=301)) so the redirect is permanent
and can be cached by browsers; update the return in root_redirect accordingly
(no other logic changes needed).
In `@backend/src/agent/gemma_client.py`:
- Around line 60-62: The exception handlers in gemma_client.py currently
re-raise exceptions with "raise e", which resets the traceback; update the
handlers to use a bare "raise" instead. Find the blocks in Google GenAI call
(the try/except around the Google Gemma client where logger.error is called),
the VertexAIGemmaClient.invoke method, and the OllamaGemmaClient.invoke method,
and replace "raise e" with "raise" while keeping the existing logger.error calls
unchanged so the original traceback is preserved.
In `@DEPLOY_RENDER.md`:
- Around line 56-60: Add a Troubleshooting note instructing users to set
ALLOWED_HOSTS to include Render hostnames because Django's
TrustedHostMiddleware/ALLOWED_HOSTS default will reject Render traffic; mention
that missing ALLOWED_HOSTS can cause 502/400 errors and reference
TrustedHostMiddleware and render.yaml so they know where to update
configuration.
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/src/agent/app.py (1)
165-168:RedirectResponsedefaults to307 Temporary Redirect— consider302for this UX redirect.
307requires the redirect to preserve the HTTP method, which is fine for GET-only browsers, but302 Foundis the conventional choice for "the UI lives here" redirects and is better understood by crawlers, monitoring probes, and any client that might POST to/.♻️ Proposed change
`@app.get`("/") async def root_redirect(): """Redirect root path to the frontend app.""" - return RedirectResponse(url="/app/") + return RedirectResponse(url="/app/", status_code=302)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/app.py` around lines 165 - 168, The root_redirect function currently returns a RedirectResponse that defaults to 307; change it to return RedirectResponse(url="/app/", status_code=302) so the redirect uses HTTP 302 (conventional UX redirect) instead of 307 — update the return in root_redirect to pass status_code=302 to RedirectResponse.backend/src/agent/gemma_client.py (1)
60-62: Prefer bareraiseoverraise eto preserve the original traceback.
raise eresets the exception's traceback to this line, making it harder to pinpoint the actual failure site inside the SDK.♻️ Proposed fix
except Exception as e: logger.error(f"Google GenAI (Gemma) call failed: {e}") - raise e + raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/gemma_client.py` around lines 60 - 62, In gemma_client.py locate the except Exception as e block where the code logs the error with logger.error(f"Google GenAI (Gemma) call failed: {e}") and change the exception re-raise from "raise e" to a bare "raise" so the original traceback is preserved (leave the logger call intact); this applies to the exception handler catching Exception as e in the Gemma call wrapper function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/src/agent/gemma_client.py`:
- Around line 34-38: The code currently reads GEMINI_API_KEY into api_key and
proceeds to call genai.Client(api_key=api_key) even when api_key is None; change
this to fail fast: after reading api_key (the variable), if it's falsy raise a
clear exception (e.g., RuntimeError or ValueError) with a message like
"GEMINI_API_KEY not set" instead of only logging a warning, so genai.Client is
never constructed with api_key=None; update the client construction site (the
genai.Client call in gemma_client.py) to only run when api_key is present.
---
Nitpick comments:
In `@backend/src/agent/app.py`:
- Around line 165-168: The root_redirect function currently returns a
RedirectResponse that defaults to 307; change it to return
RedirectResponse(url="/app/", status_code=302) so the redirect uses HTTP 302
(conventional UX redirect) instead of 307 — update the return in root_redirect
to pass status_code=302 to RedirectResponse.
In `@backend/src/agent/gemma_client.py`:
- Around line 60-62: In gemma_client.py locate the except Exception as e block
where the code logs the error with logger.error(f"Google GenAI (Gemma) call
failed: {e}") and change the exception re-raise from "raise e" to a bare "raise"
so the original traceback is preserved (leave the logger call intact); this
applies to the exception handler catching Exception as e in the Gemma call
wrapper function.
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/pruning_plan.py (2)
49-56:⚠️ Potential issue | 🟡 Minor
"NO_DIFF"falls into"DELETE (Small Change)"— misleading and potentially incorrect.When
get_diff_statsreturns("NO_DIFF", 0),total == 0satisfieselif total < 50, so the branch is labelled"DELETE (Small Change)". A branch with zero diff relative tomainmay simply be a stale but already-merged branch, but this label misrepresents why it is being marked for deletion. Adding an explicit guard keeps the plan accurate and is consistent with how"MERGED"is handled.🐛 Proposed fix to handle NO_DIFF explicitly
if stats == "MERGED": plans.append({"branch": branch, "action": "DELETE (Merged)", "size": 0}) + elif stats == "NO_DIFF": + plans.append({"branch": branch, "action": "DELETE (No Diff)", "size": 0}) elif total < 50: plans.append({"branch": branch, "action": "DELETE (Small Change)", "size": total, "details": stats})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/pruning_plan.py` around lines 49 - 56, The loop that builds deletion plans treats get_diff_stats(...) returning ("NO_DIFF", 0) as a "DELETE (Small Change)"; update the branch evaluation in the for branch in branches loop to explicitly check for stats == "NO_DIFF" before the total < 50 case and append a descriptive plan like {"branch": branch, "action": "DELETE (No Diff)", "size": 0} (or similar) so NO_DIFF is distinguished from small changes; keep the existing handling for "MERGED" and the total >= 50 "KEEP (Large Change)" case.
24-29:⚠️ Potential issue | 🟠 MajorSilent git failure is silently mis-classified as
NO_DIFF→ risks incorrect DELETE recommendation.If
git difffails for any reason (branch ref doesn't resolve, git unavailable, network issue for remote refs),result.stdoutis empty,outputis"", and the function returns("NO_DIFF", 0). Inmain(),total == 0 < 50causes this branch to be labelled "DELETE (Small Change)". For a script that produces a deletion plan, masking subprocess errors as a low-diff result is dangerous.At minimum,
result.returncodeshould be checked before treating empty output as "no diff":🛡️ Proposed fix to guard against subprocess failure
cmd = ["git", "diff", "--shortstat", f"main...{branch}"] result = subprocess.run(cmd, capture_output=True, text=True) + if result.returncode != 0: + return "ERROR", 0 output = result.stdout.strip() if not output: return "NO_DIFF", 0Also update
main()to handle the new"ERROR"state:for branch in branches: stats, total = get_diff_stats(branch) if stats == "MERGED": plans.append({"branch": branch, "action": "DELETE (Merged)", "size": 0}) + elif stats == "ERROR": + plans.append({"branch": branch, "action": "SKIP (Git Error)", "size": 0}) elif total < 50:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/pruning_plan.py` around lines 24 - 29, The git diff call currently treats empty stdout as "NO_DIFF" which masks subprocess failures; update the block around cmd/subprocess.run/result/output to check result.returncode (and if non-zero, use result.stderr or the returncode) and return a distinct error state (e.g., "ERROR", error_message) instead of ("NO_DIFF", 0); then update main() to handle the new "ERROR" state (avoid classifying it as "DELETE") and surface/log the error. Reference the variables cmd, result, output and the main() function when making these changes.
🧹 Nitpick comments (8)
scripts/pruning_plan.py (1)
16-21: Hard-coded"main"removes configurability.Removing the
default_branchparameter and hard-coding"main"on both lines 18 and 24 means this script silently produces wrong results on repos whose default branch ismasteror any other name. The original parameterised design was more robust.♻️ Proposed refactor to restore configurable base branch
-def get_diff_stats(branch): +def get_diff_stats(branch, default_branch: str = "main"): # Check if merged first - cmd_merged = ["git", "rev-list", "--count", f"main..{branch}"] + cmd_merged = ["git", "rev-list", "--count", f"{default_branch}..{branch}"] res_merged = subprocess.run(cmd_merged, capture_output=True, text=True) if res_merged.returncode == 0 and res_merged.stdout.strip() == "0": return "MERGED", 0 # Get diff stats - cmd = ["git", "diff", "--shortstat", f"main...{branch}"] + cmd = ["git", "diff", "--shortstat", f"{default_branch}...{branch}"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/pruning_plan.py` around lines 16 - 21, The function get_diff_stats currently hard-codes "main" for the base branch; restore configurability by accepting a base/default branch parameter (e.g., default_branch) or using an existing parameter and replace both occurrences of the literal "main" in get_diff_stats with that parameter so the git commands (cmd_merged and the subsequent diff command) run against the configured base branch (refer to function get_diff_stats and the variables cmd_merged and the later git diff/count command) ensuring callers can pass "master" or any other default branch.backend/src/agent/app.py (1)
165-168: Consider decoupling the redirect target from the hardcoded mount path.The redirect target
/app/(line 168) is tightly coupled to the mount path on line 408 (app.mount("/app", ...)). If the mount point is ever renamed, this redirect silently breaks with no compile-time or startup error.♻️ Proposed refactor to keep the two in sync
+FRONTEND_MOUNT_PATH = "/app" + # Mount the frontend under /app to not conflict with the LangGraph API routes app.mount( - "/app", + FRONTEND_MOUNT_PATH, create_frontend_router(), name="frontend", )`@app.get`("/") async def root_redirect(): """Redirect root path to the frontend app.""" - return RedirectResponse(url="/app/", status_code=302) + return RedirectResponse(url=f"{FRONTEND_MOUNT_PATH}/", status_code=302)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/app.py` around lines 165 - 168, The root_redirect function returns a hardcoded "/app/" which is coupled to the app.mount("/app", ...) call; make the mount path a single shared value and use that in both places (e.g., define FRONTEND_MOUNT = "/app" or FRONTEND_PREFIX and use it when calling app.mount and when building the RedirectResponse in root_redirect), or alternatively register the mounted app with a known name and compute the redirect via app.url_path_for(...) so the redirect and app.mount("/app", ...) stay in sync; update references to the literal "/app" in root_redirect and the mount call to use that shared symbol.backend/src/search/router.py (1)
27-51: Unknown provider names are now silently ignored.With the previous locking code, an unrecognized provider name triggered a warning log. In the new implementation, if
namedoesn't match anyelifbranch, no entry is added toself.providers,self.providers.get(name)returnsNone, and the caller falls through to the fallback — with no log entry. This makes misconfiguredsearch_providervalues very hard to diagnose.Consider adding a single
elsebranch to log the unknown provider:🔧 Suggested addition
elif name == SearchProviderType.BING.value: from .providers.bing_adapter import BingAdapter self.providers[name] = BingAdapter() + else: + logger.warning(f"Unknown search provider requested: {name!r}") return self.providers.get(name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/search/router.py` around lines 27 - 51, The _get_provider method currently returns None for unrecognized provider names without logging, making misconfigurations silent; update _get_provider to add a final else branch (after the elif chain) that logs a warning or error including the unknown provider name (e.g., use logger.warning or logger.error with the name) before returning None, so callers can diagnose bad search_provider values; keep the existing try/except for import errors but ensure the new else executes when no known SearchProviderType matches.backend/src/agent/utils.py (1)
219-241:temperatureis part of thelru_cachekey but silently discarded for Gemma models.
@lru_cacheuses bothmodelandtemperatureas the cache key, soget_cached_llm("gemma-2-27b-it", 0.0)andget_cached_llm("gemma-2-27b-it", 1.0)create separate cache entries even though both return behaviourally identicalGemmaAdapterinstances (temperature is never forwarded). Withmaxsize=16this can exhaust the cache unnecessarily.A lightweight fix is to normalise the temperature to
Nonefor Gemma before the cached call, or add a clarifying comment:♻️ Suggested approach — split dispatch from caching
+def get_cached_llm(model: str, temperature: float) -> Any: + """Returns a configured LLM client (Gemini or Gemma).""" + if "gemma" in model.lower(): + return _get_cached_gemma(model) + return _get_cached_gemini(model, temperature) + + +@lru_cache(maxsize=8) +def _get_cached_gemma(model: str) -> Any: + from agent.gemma_client import get_gemma_client + from agent.llm_client import GemmaAdapter + return GemmaAdapter(client=get_gemma_client(model_name=model)) + + `@lru_cache`(maxsize=16) -def get_cached_llm(model: str, temperature: float) -> Any: - """ - Returns a configured LLM client. - Supports Gemini (native) and Gemma (via GemmaAdapter). - """ - is_gemma = "gemma" in model.lower() - - if is_gemma: - from agent.gemma_client import get_gemma_client - from agent.llm_client import GemmaAdapter - - # Instantiate the correct provider (Google GenAI, Vertex or Ollama) from app_config - # Pass the specific model name to allow overriding the default - client = get_gemma_client(model_name=model) - # Return an adapter that mimics LangChain's invoke interface - return GemmaAdapter(client=client) - - return ChatGoogleGenerativeAI( +def _get_cached_gemini(model: str, temperature: float) -> Any: + return ChatGoogleGenerativeAI( model=model, temperature=temperature, api_key=os.getenv("GEMINI_API_KEY"), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/utils.py` around lines 219 - 241, get_cached_llm currently uses `@lru_cache`(model, temperature) but ignores temperature for Gemma paths, causing redundant cache entries; fix by splitting dispatch from caching: create a small cached helper (e.g., _get_cached_llm_by_model with `@lru_cache`(maxsize=16)) that takes only model and returns either a GemmaAdapter (using get_gemma_client and GemmaAdapter) or the model string/provider; then have get_cached_llm handle temperature: if model is Gemma, return the cached GemmaAdapter from _get_cached_llm_by_model(model) (ignoring temperature), otherwise construct and return ChatGoogleGenerativeAI(model=model, temperature=temperature, api_key=...), ensuring only model is used as the cache key for Gemma.backend/src/agent/llm_client.py (1)
97-108: RedundantAIMessageimport — import once at the top ofinvoke.
AIMessageis imported on line 99 (inside theif self.tools:block) and again on line 107 (unconditionally before the fallback return). When the tools path is taken but no tool_calls are parsed, line 107's import is still hit, making the line 99 import redundant on that path. More importantly, when tools are present and tool_calls are found, the return on line 105 means the line 107 import is dead code for that branch. Moving the import to the top ofinvoke(or at least once before both branches) is cleaner.♻️ Suggested refactor
# If tools are present, parse for tool calls if self.tools: from agent.tool_adapter import parse_tool_calls - from langchain_core.messages import AIMessage tool_names = [t.name for t in self.tools] tool_calls = parse_tool_calls(response_text, allowed_tools=tool_names) if tool_calls: + from langchain_core.messages import AIMessage return AIMessage(content=response_text, tool_calls=tool_calls) from langchain_core.messages import AIMessage return AIMessage(content=response_text)Or, simpler — import once before the branching:
+ from langchain_core.messages import AIMessage + # If tools are present, parse for tool calls if self.tools: from agent.tool_adapter import parse_tool_calls - from langchain_core.messages import AIMessage tool_names = [t.name for t in self.tools] tool_calls = parse_tool_calls(response_text, allowed_tools=tool_names) if tool_calls: return AIMessage(content=response_text, tool_calls=tool_calls) - from langchain_core.messages import AIMessage return AIMessage(content=response_text)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/llm_client.py` around lines 97 - 108, The AIMessage import is duplicated inside the invoke flow; move the "from langchain_core.messages import AIMessage" import so it's executed once before the tools branching (e.g., at the top of the invoke function) to remove the redundant import inside the if self.tools: block; keep parse_tool_calls and the tool_calls logic unchanged, then use AIMessage in both the tool_calls return path and the fallback return path.backend/src/agent/gemma_client.py (2)
154-156:VertexAIGemmaClientdoesn't acceptmodel_name, unlike the other providers.The factory passes
model_nametoGoogleGenAIGemmaClientandOllamaGemmaClient, but not toVertexAIGemmaClient. This is an inconsistency — if a caller provides amodel_name, it's silently ignored for the Vertex path. This may be intentional (Vertex uses an endpoint ID instead), but worth a note or TODO.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/gemma_client.py` around lines 154 - 156, The factory currently constructs VertexAIGemmaClient without forwarding model_name, causing model_name to be ignored for provider == "vertex"; update the factory so it forwards model_name to VertexAIGemmaClient (consistent with how GoogleGenAIGemmaClient and OllamaGemmaClient are constructed) and update VertexAIGemmaClient's constructor/signature to accept a model_name parameter (or explicitly handle/translate model_name to Vertex endpoint_id inside VertexAIGemmaClient); include a short TODO/log in VertexAIGemmaClient if model_name->endpoint translation is nontrivial so callers aren’t silently ignored.
151-151: Useinfor multi-value comparison.Minor readability improvement —
provider in ("google_genai", "google")is more Pythonic than chaining==withor.♻️ Suggested change
- if provider == "google_genai" or provider == "google": + if provider in ("google_genai", "google"):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/gemma_client.py` at line 151, Replace the chained equality conditional that checks provider (the line currently written as if provider == "google_genai" or provider == "google") with a membership test using provider in (...) to improve readability; locate the conditional in gemma_client.py where the variable provider is checked and change it to use an in-tuple membership check, preserving the existing branch behavior and any surrounding logic.backend/src/agent/security.py (1)
149-154: Dead code: theexceptbranch is unreachable.Given
forwardedis already confirmed non-empty at this point,forwarded.split(",")always yields at least one element, the list comprehension never raises, andips[-1]/ips[0]indexing never raisesIndexError. Theexcept Exceptionblock and the fallback on line 154 are unreachable. Consider removing the try/except and using the expression directly.♻️ Proposed simplification
- try: - ips = [ip.strip() for ip in forwarded.split(",")] - client_ip = ips[0] # after applying the fix above - except Exception: - # Fallback to simple extraction if parsing fails - client_ip = forwarded.split(",")[0].strip() + ips = [ip.strip() for ip in forwarded.split(",")] + client_ip = ips[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/security.py` around lines 149 - 154, The try/except around parsing the forwarded header is dead code because forwarded is guaranteed non-empty; remove the redundant exception handling and simplify by directly computing ips = [ip.strip() for ip in forwarded.split(",")] and setting client_ip = ips[-1] (or client_ip = ips[0] if intended), eliminating the unreachable except branch and the fallback split expression; update references to forwarded, ips, and client_ip accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/scripts/benchmark.py`:
- Line 151: The file opens the output report with open("benchmark_report.md",
"w") without specifying encoding, which can break on non-ASCII text; update the
open call that creates the benchmark_report.md (the with open(...) as f:
statement) to explicitly pass encoding="utf-8" so the generated report is
written using UTF-8. Ensure any other writes to "benchmark_report.md" in the
same script use the same explicit encoding.
- Line 60: The access item["question"] is currently outside the try/except in
run_benchmark and will raise a KeyError for malformed items, aborting the entire
benchmark; move the lookup for item["question"] (and any other required keys)
inside the existing try block or replace it with item.get("question") plus a
guard that skips the item and continues, so malformed entries are logged/ignored
instead of raising; update the code paths around the try/except in run_benchmark
to handle a missing question (e.g., continue to next item and optionally log a
warning).
- Line 44: The open call using with open(path, "r") as f: should be changed to
omit the redundant "r" mode and explicitly specify UTF-8 encoding to avoid
platform-dependent decoding errors; update the open invocation that uses the
path variable and file handle f to use open(path, encoding="utf-8") so files are
read with a consistent encoding across platforms.
In `@backend/src/agent/app.py`:
- Around line 263-265: Fix the indentation inconsistency in the exception
handler for the max_research_loops validator: adjust the leading spaces so the
two raise lines under the except block match the style used in the
initial_search_query_count block (remove the extra leading space before "raise
e" and "raise ValueError...") within the same validator method (the
max_research_loops validator in backend/src/agent/app.py) so formatting is
consistent across both validators.
In `@backend/src/agent/gemma_client.py`:
- Around line 60-62: Remove the unnecessary f-string prefixes used with
logger.error where there are no interpolations in
backend/src/agent/gemma_client.py: replace f"Google GenAI (Gemma) call failed"
(and the other two similar occurrences flagged) with a plain string literal;
locate the three logger.error calls in the Gemma client exception handlers (the
except blocks that currently call logger.error(..., exc_info=True)) and remove
the leading "f" so the messages are regular strings.
- Around line 134-136: The POST to self.requests.post(self.generate_url,
json=payload) lacks a timeout and can hang; add a timeout argument (e.g.,
timeout=60 or timeout=120) to the call and propagate/handle timeout errors from
requests (catch requests.exceptions.Timeout and/or
requests.exceptions.RequestException around the self.requests.post /
response.raise_for_status block) so the method (in gemma_client.py where the
POST and response.raise_for_status occur) fails fast with a clear error/log
instead of blocking indefinitely.
- Line 148: app_config.gemma_provider can be None so calling .lower() will raise
AttributeError; update the code around the provider assignment (the provider
variable in gemma_client.py) to guard against None — e.g., derive provider from
(app_config.gemma_provider or ""); then .lower() it and if the result is
empty/unsupported raise a clear ValueError or fall back to a sensible default
provider so the factory never calls .lower() on None.
- Line 58: The direct access to response.text can raise ValueError when the
GenAI response has no valid candidates; wrap the access to response.text in a
try/except that catches ValueError (and optionally TypeError) and returns an
empty string instead of letting the exception propagate, updating the code
around the return statement that currently does "return response.text if
response.text else """, so locate the usage of response.text in gemma_client.py
and implement the defensive fallback.
- Around line 127-132: The current payload construction in gemma_client.py
spreads **kwargs directly into the top-level payload (payload = {"model":
self.model_name, "prompt": prompt, "stream": False, **kwargs}), which can inject
keys Ollama doesn't expect; update the invoke (used by call_llm_robust) to
instead build an "options" dict and move known LLM params (e.g., temperature,
max_tokens, top_p, stop, repetition_penalty, etc.) into payload["options"], only
include safe top-level keys (model, prompt, stream), and either validate/ignore
or explicitly map any remaining kwargs so no unknown keys are placed at the top
level.
In `@backend/src/agent/llm_client.py`:
- Line 101: The current change replaces the defensive extraction with tool_names
= [t.name for t in self.tools], which will raise AttributeError if any tool in
self.tools lacks a .name; restore defensive extraction by iterating self.tools
and using getattr(t, "name", None) (or similar) to skip or log missing names so
tool_names remains safe, updating the logic around tool_names and any related
logging to warn about missing .name attributes instead of crashing.
In `@backend/src/agent/nodes.py`:
- Around line 841-842: The fallback currently uses dict(t) which preserves
producer-specific keys and breaks downstream expectations; instead reapply the
normalization previously done by _normalize_task so plan items always have
title/description/status/query shape. Update both fallback sites that set
plan_todos = [dict(t) for t in current_plan] (and the similar line at the other
catch) to map each item through the _normalize_task logic (i.e., produce items
with keys title, description, status, query) so select_next_task and update_plan
(which read task.get("query")/task.get("title") and
current_plan[current_idx].get("title")) receive normalized entries; reference
_normalize_task, generate_plan, update_plan, select_next_task, current_plan,
plan_todos, and current_idx when making the change.
- Line 25: The code removed HumanMessage from the imports but research_subgraph
still constructs messages using HumanMessage, causing a NameError; fix by
re-adding HumanMessage to the imports from langchain_core.messages (alongside
AIMessage) or replace the usage inside research_subgraph with an imported
equivalent (e.g., AIMessage or another appropriate message class); reference the
import at the top (where AIMessage is imported) and the research_subgraph
function which creates "messages": [HumanMessage(content=subtopic_query)] to
locate and update the code.
In `@backend/tests/conftest.py`:
- Around line 192-220: The file contains duplicate definitions of
pytest_addoption and pytest_collection_modifyitems which shadow the earlier
definitions; remove the duplicate pair (the second pytest_addoption and
pytest_collection_modifyitems block) so only one definition of each function
(pytest_addoption and pytest_collection_modifyitems) remains, ensuring the
original behavior is preserved and eliminating the Ruff F811 redefinition
warnings.
In `@backend/tests/data/benchmark_questions.json`:
- Line 3: Fix the typo in the JSON test data by updating the "question" field
value in backend/tests/data/benchmark_questions.json: replace "rooms-temperature
superconductivity" with the correct term "room-temperature superconductivity"
(locate the entry by the "question" key or the current string "What are the
latest developments in rooms-temperature superconductivity as of 2025?"). Ensure
the JSON string is updated exactly and the file remains valid JSON.
---
Outside diff comments:
In `@scripts/pruning_plan.py`:
- Around line 49-56: The loop that builds deletion plans treats
get_diff_stats(...) returning ("NO_DIFF", 0) as a "DELETE (Small Change)";
update the branch evaluation in the for branch in branches loop to explicitly
check for stats == "NO_DIFF" before the total < 50 case and append a descriptive
plan like {"branch": branch, "action": "DELETE (No Diff)", "size": 0} (or
similar) so NO_DIFF is distinguished from small changes; keep the existing
handling for "MERGED" and the total >= 50 "KEEP (Large Change)" case.
- Around line 24-29: The git diff call currently treats empty stdout as
"NO_DIFF" which masks subprocess failures; update the block around
cmd/subprocess.run/result/output to check result.returncode (and if non-zero,
use result.stderr or the returncode) and return a distinct error state (e.g.,
"ERROR", error_message) instead of ("NO_DIFF", 0); then update main() to handle
the new "ERROR" state (avoid classifying it as "DELETE") and surface/log the
error. Reference the variables cmd, result, output and the main() function when
making these changes.
---
Nitpick comments:
In `@backend/src/agent/app.py`:
- Around line 165-168: The root_redirect function returns a hardcoded "/app/"
which is coupled to the app.mount("/app", ...) call; make the mount path a
single shared value and use that in both places (e.g., define FRONTEND_MOUNT =
"/app" or FRONTEND_PREFIX and use it when calling app.mount and when building
the RedirectResponse in root_redirect), or alternatively register the mounted
app with a known name and compute the redirect via app.url_path_for(...) so the
redirect and app.mount("/app", ...) stay in sync; update references to the
literal "/app" in root_redirect and the mount call to use that shared symbol.
In `@backend/src/agent/gemma_client.py`:
- Around line 154-156: The factory currently constructs VertexAIGemmaClient
without forwarding model_name, causing model_name to be ignored for provider ==
"vertex"; update the factory so it forwards model_name to VertexAIGemmaClient
(consistent with how GoogleGenAIGemmaClient and OllamaGemmaClient are
constructed) and update VertexAIGemmaClient's constructor/signature to accept a
model_name parameter (or explicitly handle/translate model_name to Vertex
endpoint_id inside VertexAIGemmaClient); include a short TODO/log in
VertexAIGemmaClient if model_name->endpoint translation is nontrivial so callers
aren’t silently ignored.
- Line 151: Replace the chained equality conditional that checks provider (the
line currently written as if provider == "google_genai" or provider == "google")
with a membership test using provider in (...) to improve readability; locate
the conditional in gemma_client.py where the variable provider is checked and
change it to use an in-tuple membership check, preserving the existing branch
behavior and any surrounding logic.
In `@backend/src/agent/llm_client.py`:
- Around line 97-108: The AIMessage import is duplicated inside the invoke flow;
move the "from langchain_core.messages import AIMessage" import so it's executed
once before the tools branching (e.g., at the top of the invoke function) to
remove the redundant import inside the if self.tools: block; keep
parse_tool_calls and the tool_calls logic unchanged, then use AIMessage in both
the tool_calls return path and the fallback return path.
In `@backend/src/agent/security.py`:
- Around line 149-154: The try/except around parsing the forwarded header is
dead code because forwarded is guaranteed non-empty; remove the redundant
exception handling and simplify by directly computing ips = [ip.strip() for ip
in forwarded.split(",")] and setting client_ip = ips[-1] (or client_ip = ips[0]
if intended), eliminating the unreachable except branch and the fallback split
expression; update references to forwarded, ips, and client_ip accordingly.
In `@backend/src/agent/utils.py`:
- Around line 219-241: get_cached_llm currently uses `@lru_cache`(model,
temperature) but ignores temperature for Gemma paths, causing redundant cache
entries; fix by splitting dispatch from caching: create a small cached helper
(e.g., _get_cached_llm_by_model with `@lru_cache`(maxsize=16)) that takes only
model and returns either a GemmaAdapter (using get_gemma_client and
GemmaAdapter) or the model string/provider; then have get_cached_llm handle
temperature: if model is Gemma, return the cached GemmaAdapter from
_get_cached_llm_by_model(model) (ignoring temperature), otherwise construct and
return ChatGoogleGenerativeAI(model=model, temperature=temperature,
api_key=...), ensuring only model is used as the cache key for Gemma.
In `@backend/src/search/router.py`:
- Around line 27-51: The _get_provider method currently returns None for
unrecognized provider names without logging, making misconfigurations silent;
update _get_provider to add a final else branch (after the elif chain) that logs
a warning or error including the unknown provider name (e.g., use logger.warning
or logger.error with the name) before returning None, so callers can diagnose
bad search_provider values; keep the existing try/except for import errors but
ensure the new else executes when no known SearchProviderType matches.
In `@scripts/pruning_plan.py`:
- Around line 16-21: The function get_diff_stats currently hard-codes "main" for
the base branch; restore configurability by accepting a base/default branch
parameter (e.g., default_branch) or using an existing parameter and replace both
occurrences of the literal "main" in get_diff_stats with that parameter so the
git commands (cmd_merged and the subsequent diff command) run against the
configured base branch (refer to function get_diff_stats and the variables
cmd_merged and the later git diff/count command) ensuring callers can pass
"master" or any other default branch.
Comment on lines
+263
to
+265
| if "cannot exceed" in str(e) or "must be at least" in str(e): | ||
| raise e | ||
| raise ValueError("max_research_loops must be an integer") | ||
| raise ValueError("max_research_loops must be an integer") |
There was a problem hiding this comment.
Indentation inconsistency in max_research_loops exception handler.
Lines 263 and 265 have one extra leading space compared to the identical pattern in the initial_search_query_count block (lines 250 and 252). The code is syntactically valid, but the style is inconsistent within the same validator method.
🛠️ Proposed fix
except ValueError as e:
- if "cannot exceed" in str(e) or "must be at least" in str(e):
+ if "cannot exceed" in str(e) or "must be at least" in str(e):
raise e
- raise ValueError("max_research_loops must be an integer")
+ raise ValueError("max_research_loops must be an integer")📝 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.
Suggested change
| if "cannot exceed" in str(e) or "must be at least" in str(e): | |
| raise e | |
| raise ValueError("max_research_loops must be an integer") | |
| raise ValueError("max_research_loops must be an integer") | |
| if "cannot exceed" in str(e) or "must be at least" in str(e): | |
| raise e | |
| raise ValueError("max_research_loops must be an integer") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/agent/app.py` around lines 263 - 265, Fix the indentation
inconsistency in the exception handler for the max_research_loops validator:
adjust the leading spaces so the two raise lines under the except block match
the style used in the initial_search_query_count block (remove the extra leading
space before "raise e" and "raise ValueError...") within the same validator
method (the max_research_loops validator in backend/src/agent/app.py) so
formatting is consistent across both validators.
| from search.router import search_router | ||
| from google.genai import Client | ||
| from langchain_core.messages import AIMessage, HumanMessage | ||
| from langchain_core.messages import AIMessage |
There was a problem hiding this comment.
HumanMessage still used at line 1170 but removed from imports — runtime NameError.
research_subgraph (line 1170) constructs:
"messages": [HumanMessage(content=subtopic_query)],but HumanMessage is no longer imported at line 25 (AIMessage is the only retained symbol). There is no local import of HumanMessage inside research_subgraph either. Every invocation of research_subgraph will immediately raise:
NameError: name 'HumanMessage' is not defined
🐛 Proposed fix
-from langchain_core.messages import AIMessage
+from langchain_core.messages import AIMessage, HumanMessage🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/agent/nodes.py` at line 25, The code removed HumanMessage from
the imports but research_subgraph still constructs messages using HumanMessage,
causing a NameError; fix by re-adding HumanMessage to the imports from
langchain_core.messages (alongside AIMessage) or replace the usage inside
research_subgraph with an imported equivalent (e.g., AIMessage or another
appropriate message class); reference the import at the top (where AIMessage is
imported) and the research_subgraph function which creates "messages":
[HumanMessage(content=subtopic_query)] to locate and update the code.
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/pruning_plan.py (2)
9-17:⚠️ Potential issue | 🟡 Minor
get_remote_branchessilently swallows git failures.
result.returncodeis never checked. Ifgit branch -rfails (e.g., not inside a git repo, no remote configured), the function returns an empty list andmain()prints "Analyzing 0 remote branches…" with no error signal.🛡️ Proposed fix
def get_remote_branches(): # Get all remote branches except HEAD and main cmd = ["git", "branch", "-r"] result = subprocess.run(cmd, capture_output=True, text=True) + if result.returncode != 0: + raise RuntimeError(f"git branch -r failed: {result.stderr.strip()}") branches = []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/pruning_plan.py` around lines 9 - 17, The get_remote_branches function currently ignores git failures because it doesn't check result.returncode; update it to detect and surface git errors by either calling subprocess.run with check=True or by checking result.returncode and raising an exception that includes result.stderr (e.g., raise RuntimeError(f"git branch -r failed: {result.stderr.strip()}")), so callers (like main) don't proceed as if there are zero branches; ensure the error message includes the failing command context and stderr to aid debugging.
14-14:⚠️ Potential issue | 🟡 MinorHardcoded
"origin/main"exclusion doesn't account for a configurable--basebranch.When
--baseis anything other than"main"(e.g.,--base develop),origin/developis not excluded from the branch list and will be compared against itself, yielding a spuriousNO_DIFF → DELETEentry in the report. Meanwhile,origin/mainis still unconditionally filtered out even if it shouldn't be.♻️ Proposed fix — pass base to the filter
-def get_remote_branches(): - # Get all remote branches except HEAD and main +def get_remote_branches(base_branch="main"): + # Get all remote branches except HEAD and the base branch cmd = ["git", "branch", "-r"] result = subprocess.run(cmd, capture_output=True, text=True) + if result.returncode != 0: + raise RuntimeError(f"git branch -r failed: {result.stderr.strip()}") branches = [] for line in result.stdout.splitlines(): branch = line.strip() - if "->" in branch or "origin/main" in branch or "upstream" in branch: + if "->" in branch or branch == f"origin/{base_branch}" or "upstream" in branch: continue branches.append(branch) return branchesThen update the call-site in
main():- branches = get_remote_branches() + branches = get_remote_branches(args.base)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/pruning_plan.py` at line 14, The filter currently hardcodes "origin/main" and must instead use the configured base; change the condition that checks branches (the line using "if \"->\" in branch or \"origin/main\" in branch or \"upstream\" in branch:") to use the runtime base (e.g., check f"origin/{base}" or equivalent) and make the function that contains this condition accept a base parameter (name it base) so it can be used in the branch check; then update the call-site in main() to pass the CLI/base variable into that function so the configured --base is respected.
🧹 Nitpick comments (2)
backend/src/agent/llm_client.py (1)
55-57:raise eresets the traceback — prefer bareraise.Using
raise einstead ofraisediscards the original traceback frames, making debugging harder when tenacity exhausts retries.Proposed fix
except Exception as e: logger.warning(f"LLM call failed (attempting retry): {e}") - raise e + raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/llm_client.py` around lines 55 - 57, In the except block that catches exceptions during LLM calls (the "except Exception as e:" block that logs via logger.warning in llm_client.py), replace the current "raise e" with a bare "raise" so the original traceback is preserved; keep the existing logger.warning call but re-raise using plain "raise" to avoid losing traceback information when tenacity exhausts retries.backend/src/agent/utils.py (1)
216-225:_get_cached_gemma_adaptercreates adapter without tools — tool-calling via Gemma won't work through this path.
GemmaAdapter(client=client)is constructed with notools, so the cached adapter always bypasses tool-call parsing. If tool support is eventually needed for Gemma models returned byget_cached_llm, tools would need to be injected separately or the caching strategy would need to change.If this is intentional (tools injected elsewhere or not needed for this path), a brief comment would help clarify.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/utils.py` around lines 216 - 225, _get_cached_gemma_adapter currently constructs a GemmaAdapter without passing any tools, which prevents tool-call parsing when this cached adapter is used; update the function to either accept/inject the required tools (pass them into GemmaAdapter) or change caching to include tools in the cache key so tool-enabled adapters are returned, specifically modify _get_cached_gemma_adapter to obtain the tool list (or accept a tools parameter) and construct GemmaAdapter(client=client, tools=tools), or if omission is intentional add a clarifying comment in _get_cached_gemma_adapter and document where tools are injected downstream (referencing GemmaAdapter and get_gemma_client to locate the code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/scripts/benchmark.py`:
- Line 110: The f-string on the write call is unnecessary because there are no
interpolation expressions; update the call that currently uses f.write(f"#
Benchmark Report\n\n") to use a plain string (e.g., f.write("# Benchmark
Report\n\n")) so Ruff F541 is resolved and no behavior changes occur; locate the
write invocation (the f.write(...) call in backend/scripts/benchmark.py) and
remove the leading f from the string literal.
In `@backend/src/agent/gemma_client.py`:
- Line 129: OllamaGemmaClient.model_name can end up None if both the constructor
arg and app_config.gemma_model_name are unset; update the assignment in
OllamaGemmaClient (self.model_name) to provide a safe fallback (e.g., use
model_name or app_config.gemma_model_name or a default string like "gemma") so
the Ollama payload never contains a null model; mirror the defensive behavior
used in GoogleGenAIGemmaClient by ensuring a non-None default.
In `@backend/src/agent/security.py`:
- Around line 64-94: The IP extraction currently takes the last hop (ips[-1])
which returns the proxy IP; change the logic in the block that reads
X-Forwarded-For (where trust_proxy_headers is checked and ips is built from
request.headers.get("X-Forwarded-For")) to use the first entry (ips[0].strip())
as the originating client IP, remove the long indecisive comment block and
replace it with a concise clarifying comment like "Use first X-Forwarded-For
entry as originating client when trust_proxy_headers is True", keep the fallback
to request.client.host and the sanitization regex for client_ip unchanged.
- Line 27: The Content-Security-Policy header currently set via
response.headers["Content-Security-Policy"] = "default-src 'self'" is too
restrictive and doesn't match tests; update the assignment to include explicit
directives for scripts and styles (e.g. include "script-src 'self'" and a
style-src that allows nonce-based inline styles such as "style-src 'self'
'nonce-<nonce>'" or the equivalent pattern your app uses) so the header contains
at least default-src 'self'; script-src 'self'; style-src 'self' 'nonce-*'; and
also add the missing Permissions-Policy header (e.g.
response.headers["Permissions-Policy"] = "<expected-policy-string>") in the same
middleware where response.headers is set so the test in
backend/tests/agent/test_api_security.py that looks for both CSP and
Permissions-Policy passes.
In `@scripts/pruning_plan.py`:
- Around line 67-68: The elif branch handling stats == "ERROR" has one extra
leading space in its body, causing inconsistent indentation; locate the elif
stats == "ERROR" block in scripts/pruning_plan.py and normalize the indentation
of the plans.append line to match the other if/elif bodies (same indent as other
plans.append calls) so the visual alignment is consistent around the branch/
action/size/details dict construction.
---
Outside diff comments:
In `@scripts/pruning_plan.py`:
- Around line 9-17: The get_remote_branches function currently ignores git
failures because it doesn't check result.returncode; update it to detect and
surface git errors by either calling subprocess.run with check=True or by
checking result.returncode and raising an exception that includes result.stderr
(e.g., raise RuntimeError(f"git branch -r failed: {result.stderr.strip()}")), so
callers (like main) don't proceed as if there are zero branches; ensure the
error message includes the failing command context and stderr to aid debugging.
- Line 14: The filter currently hardcodes "origin/main" and must instead use the
configured base; change the condition that checks branches (the line using "if
\"->\" in branch or \"origin/main\" in branch or \"upstream\" in branch:") to
use the runtime base (e.g., check f"origin/{base}" or equivalent) and make the
function that contains this condition accept a base parameter (name it base) so
it can be used in the branch check; then update the call-site in main() to pass
the CLI/base variable into that function so the configured --base is respected.
---
Duplicate comments:
In `@backend/scripts/benchmark.py`:
- Around line 103-126: The generate_report function now opens the output file
with encoding="utf-8" which resolves cross-platform non-ASCII issues—no changes
required to the report write logic; keep the encoding argument as present and
ensure the module defines/imports the logger used by logger.info at the end (so
the call in generate_report will not raise a NameError).
- Around line 25-36: The review content contains a duplicate/extra review tag
([duplicate_comment]) that should be removed; no code changes to
load_benchmark_data are needed—leave the function and its logger/error handling
as-is—simply edit the PR review comment/body to remove the redundant
"[duplicate_comment]" marker (or consolidate duplicate remarks) so the review is
not duplicated.
- Around line 38-73: The current extraction of the final answer in
run_single_benchmark uses messages[-1].content which can raise AttributeError or
KeyError for dicts or unexpected message shapes; update the extraction to handle
both dict and object message types safely: grab last = messages[-1] (if
messages), then if isinstance(last, dict) use last.get("content", str(last)),
else use getattr(last, "content", str(last)) and fall back to "No answer
produced" when messages is empty; keep the surrounding try/except and
logger.error as-is to preserve graceful failure reporting.
---
Nitpick comments:
In `@backend/src/agent/llm_client.py`:
- Around line 55-57: In the except block that catches exceptions during LLM
calls (the "except Exception as e:" block that logs via logger.warning in
llm_client.py), replace the current "raise e" with a bare "raise" so the
original traceback is preserved; keep the existing logger.warning call but
re-raise using plain "raise" to avoid losing traceback information when tenacity
exhausts retries.
In `@backend/src/agent/utils.py`:
- Around line 216-225: _get_cached_gemma_adapter currently constructs a
GemmaAdapter without passing any tools, which prevents tool-call parsing when
this cached adapter is used; update the function to either accept/inject the
required tools (pass them into GemmaAdapter) or change caching to include tools
in the cache key so tool-enabled adapters are returned, specifically modify
_get_cached_gemma_adapter to obtain the tool list (or accept a tools parameter)
and construct GemmaAdapter(client=client, tools=tools), or if omission is
intentional add a clarifying comment in _get_cached_gemma_adapter and document
where tools are injected downstream (referencing GemmaAdapter and
get_gemma_client to locate the code).
| avg_time = total_time / len(results) if results else 0 | ||
|
|
||
| with open(output_path, "w", encoding="utf-8") as f: | ||
| f.write(f"# Benchmark Report\n\n") |
There was a problem hiding this comment.
Remove extraneous f prefix on string without placeholders.
Static analysis (Ruff F541) flags this f-string as having no interpolation expressions. The f prefix is unnecessary here.
Proposed fix
- f.write(f"# Benchmark Report\n\n")
+ f.write("# Benchmark Report\n\n")📝 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.
Suggested change
| f.write(f"# Benchmark Report\n\n") | |
| f.write("# Benchmark Report\n\n") |
🧰 Tools
🪛 Ruff (0.15.1)
[error] 110-110: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/scripts/benchmark.py` at line 110, The f-string on the write call is
unnecessary because there are no interpolation expressions; update the call that
currently uses f.write(f"# Benchmark Report\n\n") to use a plain string (e.g.,
f.write("# Benchmark Report\n\n")) so Ruff F541 is resolved and no behavior
changes occur; locate the write invocation (the f.write(...) call in
backend/scripts/benchmark.py) and remove the leading f from the string literal.
| self.requests = requests | ||
| self.base_url = app_config.ollama_base_url | ||
| self.model_name = app_config.gemma_model_name | ||
| self.model_name = model_name or app_config.gemma_model_name |
There was a problem hiding this comment.
OllamaGemmaClient.model_name can be None if both model_name arg and app_config.gemma_model_name are unset.
Unlike GoogleGenAIGemmaClient (line 40), there's no hardcoded fallback here. A None model name will produce "model": None in the Ollama payload, likely causing a request failure.
Proposed fix — add fallback default
- self.model_name = model_name or app_config.gemma_model_name
+ self.model_name = model_name or app_config.gemma_model_name or "gemma-2-27b-it"📝 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.
Suggested change
| self.model_name = model_name or app_config.gemma_model_name | |
| self.model_name = model_name or app_config.gemma_model_name or "gemma-2-27b-it" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/agent/gemma_client.py` at line 129, OllamaGemmaClient.model_name
can end up None if both the constructor arg and app_config.gemma_model_name are
unset; update the assignment in OllamaGemmaClient (self.model_name) to provide a
safe fallback (e.g., use model_name or app_config.gemma_model_name or a default
string like "gemma") so the Ollama payload never contains a null model; mirror
the defensive behavior used in GoogleGenAIGemmaClient by ensuring a non-None
default.
| # XSS Protection (legacy but good defense in depth) | ||
| response.headers["X-XSS-Protection"] = "1; mode=block" | ||
|
|
||
| response.headers["Content-Security-Policy"] = "default-src 'self'" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the security.py file to see the full context
cat -n backend/src/agent/security.pyRepository: MasumRab/gemini-fullstack-langgraph-quickstart
Length of output: 5655
🏁 Script executed:
# Find the frontend build location and look for index.html
fd 'index.html' --type fRepository: MasumRab/gemini-fullstack-langgraph-quickstart
Length of output: 109
🏁 Script executed:
# Check for any CSP-related configuration or documentation
rg -i 'content-security-policy|csp' --type py --type mdRepository: MasumRab/gemini-fullstack-langgraph-quickstart
Length of output: 308
🏁 Script executed:
# Check the frontend index.html for inline styles and external resources
head -150 frontend/index.htmlRepository: MasumRab/gemini-fullstack-langgraph-quickstart
Length of output: 455
🏁 Script executed:
# Examine the test to see what CSP is expected vs actual
cat -n backend/tests/agent/test_api_security.pyRepository: MasumRab/gemini-fullstack-langgraph-quickstart
Length of output: 10441
CSP header is incomplete and doesn't match test expectations.
The current default-src 'self' is too restrictive and will likely break a Vite React SPA that injects inline styles. Additionally, the test at backend/tests/agent/test_api_security.py:47 expects script-src 'self' in the CSP header, but the code only sets default-src 'self'. The test also checks for a Permissions-Policy header that is not set in the middleware. Update the CSP to include explicit directives for scripts and styles (script-src 'self'; style-src 'self' 'nonce-*' or similar), and add the missing Permissions-Policy header to match test expectations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/agent/security.py` at line 27, The Content-Security-Policy header
currently set via response.headers["Content-Security-Policy"] = "default-src
'self'" is too restrictive and doesn't match tests; update the assignment to
include explicit directives for scripts and styles (e.g. include "script-src
'self'" and a style-src that allows nonce-based inline styles such as "style-src
'self' 'nonce-<nonce>'" or the equivalent pattern your app uses) so the header
contains at least default-src 'self'; script-src 'self'; style-src 'self'
'nonce-*'; and also add the missing Permissions-Policy header (e.g.
response.headers["Permissions-Policy"] = "<expected-policy-string>") in the same
middleware where response.headers is set so the test in
backend/tests/agent/test_api_security.py that looks for both CSP and
Permissions-Policy passes.
| self.limit = limit | ||
| self.window = window | ||
| self.protected_paths = protected_paths if protected_paths is not None else [] | ||
| self.limiter = RateLimiter(limit, window) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate and examine the RateLimiter class definition
echo "=== Searching for RateLimiter class definition ==="
rg -n "class RateLimiter" --type=py -A 15
echo ""
echo "=== Searching for RateLimitExceeded exception ==="
rg -n "class RateLimitExceeded" --type=py
echo ""
echo "=== Searching for wait_if_needed method ==="
rg -n "def wait_if_needed" --type=py -A 5
echo ""
echo "=== Checking actual import and usage in security.py ==="
rg -n "from.*RateLimiter\|import.*RateLimiter\|RateLimiter(" backend/src/agent/security.py -A 2 -B 2Repository: MasumRab/gemini-fullstack-langgraph-quickstart
Length of output: 1988
🏁 Script executed:
#!/bin/bash
echo "=== RateLimiter instantiation in security.py ==="
rg -n "RateLimiter(" backend/src/agent/security.py -B 2 -A 2
echo ""
echo "=== wait_if_needed calls in security.py ==="
rg -n "wait_if_needed" backend/src/agent/security.py -B 2 -A 2
echo ""
echo "=== Import statements in security.py ==="
head -20 backend/src/agent/security.py
echo ""
echo "=== Check if RateLimitExceeded is defined anywhere ==="
rg -n "RateLimitExceeded" --type=pyRepository: MasumRab/gemini-fullstack-langgraph-quickstart
Length of output: 1333
🏁 Script executed:
#!/bin/bash
echo "=== Lines 45-60 in security.py ==="
sed -n '45,60p' backend/src/agent/security.py | cat -n
echo ""
echo "=== Full search for RateLimitExceeded (any line) ==="
rg "RateLimitExceeded" --type=py -B 2 -A 2
echo ""
echo "=== Check rate_limiter.py for RateLimitExceeded or any exception class ==="
rg "class.*Exception\|class.*Error\|RateLimitExceeded" backend/src/agent/rate_limiter.py -B 1 -A 3
echo ""
echo "=== Full content of rate_limiter.py for context ==="
wc -l backend/src/agent/rate_limiter.pyRepository: MasumRab/gemini-fullstack-langgraph-quickstart
Length of output: 1876
🏁 Script executed:
#!/bin/bash
echo "=== Full rate_limiter.py file ==="
cat backend/src/agent/rate_limiter.pyRepository: MasumRab/gemini-fullstack-langgraph-quickstart
Length of output: 12483
RateLimiter API mismatch — constructor signature, method call, and missing exception class are incompatible.
The RateLimiter imported from agent.rate_limiter is designed for Gemini API rate limiting (by RPM/TPM/RPD with a model string). Here, line 52 instantiates it with two integers (limit, window), but the constructor signature is __init__(self, model: str = GEMINI_FLASH). Line 109 calls wait_if_needed(client_key) with a string, but the method signature expects estimated_tokens: int. Additionally, RateLimitExceeded is imported on line 10 but is not defined anywhere in rate_limiter.py—only a generic Exception is raised.
This will fail immediately at instantiation or at the first rate-limit check, and the exception handler will never work.
Comment on lines
+64
to
+94
| client_ip = "unknown" | ||
|
|
||
| if self.trust_proxy_headers: | ||
| forwarded = request.headers.get("X-Forwarded-For") | ||
| if forwarded and self.trust_proxy_headers: | ||
| # 🛡️ Sentinel: The leftmost IP (ips[0]) is the original client IP. | ||
| # Each proxy appends its IP to the right, so ips[-1] would be the nearest proxy. | ||
| # We use ips[0] to get the original client address for rate limiting. | ||
| try: | ||
| ips = [ip.strip() for ip in forwarded.split(",")] | ||
| client_ip = ips[0] # Original client IP (leftmost) | ||
| except Exception: | ||
| # Fallback to simple extraction if parsing fails | ||
| client_ip = forwarded.split(",")[0].strip() | ||
|
|
||
| # Truncate to 100 chars to prevent memory exhaustion attacks | ||
| client_ip = client_ip[:100] | ||
| else: | ||
| client_ip = request.client.host if request.client else "unknown" | ||
|
|
||
| # 🛡️ Sentinel: Group IPv6 addresses by /64 prefix to prevent subnet rotation attacks | ||
| client_key = self.get_client_key(client_ip) | ||
|
|
||
| now = time.time() | ||
|
|
||
| # Clean old requests (simple sliding window) | ||
| current_requests = self.requests[client_key] | ||
| # Prune old timestamps | ||
| active_requests = [t for t in current_requests if now - t < self.window] | ||
|
|
||
| if len(active_requests) >= self.limit: | ||
| # Update map with pruned list before returning | ||
| self.requests[client_key] = active_requests | ||
|
|
||
| # Calculate retry_after | ||
| oldest_request_time = active_requests[0] | ||
| reset_time = oldest_request_time + self.window | ||
| retry_after = max(1, int(math.ceil(reset_time - now))) | ||
|
|
||
| logger.warning(f"Rate limit exceeded for {client_key} on {path}") | ||
|
|
||
| return JSONResponse( | ||
| status_code=429, | ||
| content={"detail": "Too Many Requests", "retry_after": retry_after}, | ||
| headers={"Retry-After": str(retry_after)}, | ||
| ) | ||
|
|
||
| active_requests.append(now) | ||
|
|
||
| # Simple Memory Leak Prevention: | ||
| # If dictionary gets too large, perform cleanup to prevent OOM. | ||
| # 🛡️ Sentinel: Throttle cleanup to prevent CPU exhaustion (DoS) via O(N) loop | ||
| if now - self.last_cleanup > self.cleanup_interval: | ||
| self.last_cleanup = now | ||
| # Cleanup: Remove clients that haven't made a request within the window. | ||
| # Since active_requests for each client might not be updated until they make a request, | ||
| # we need to check the last timestamp in their list. | ||
| # Note: This is an O(N) operation where N is number of clients. | ||
| if len(self.requests) > 10000: | ||
| stale_ips = [] | ||
| for ip, timestamps in self.requests.items(): | ||
| # If list is empty (shouldn't happen with logic above but possible) | ||
| # or if the most recent request is older than window | ||
| if not timestamps or (now - timestamps[-1] > self.window): | ||
| stale_ips.append(ip) | ||
|
|
||
| for ip in stale_ips: | ||
| del self.requests[ip] | ||
|
|
||
| # Fallback: If still too large (active attack with >10k distinct IPs), | ||
| # 🛡️ Sentinel: Do NOT clear everything, as that allows attackers to reset everyone's limit. | ||
| # Instead, if we are full, REJECT new clients. | ||
| if len(self.requests) > 10000: | ||
| # If the client is already known, we updated them above. | ||
| # But wait, if we are > 10000, and this is a NEW client_ip (or one that was just added), | ||
| # we should remove it and block. | ||
| # However, we already added `now` to `active_requests` and set `self.requests[client_ip]`. | ||
|
|
||
| # We need to check if we just added a NEW key that pushed us over. | ||
| # If client_ip was already in requests, we are fine (we are just updating an existing slot). | ||
| # If client_ip is NEW, and size > 10000, we should reject. | ||
|
|
||
| # Optimization: Move the check BEFORE adding to `self.requests`. | ||
| # But we used `defaultdict`, so accessing `self.requests[client_ip]` already created the entry if missing. | ||
|
|
||
| # So, if we are over limit: | ||
| # Check if we should allow this IP. | ||
| # If we just created it (len=1), delete it and 503. | ||
| if ( | ||
| len(active_requests) == 1 | ||
| ): # This was a new entry (or re-entry after expiry) | ||
| # Safe delete using pop to avoid KeyErrors in race conditions | ||
| self.requests.pop(client_key, None) | ||
| return Response("Server Busy", status_code=503) | ||
|
|
||
| self.requests[client_key] = active_requests | ||
| if forwarded: | ||
| # Direct split and strip, no try/except needed as split never raises | ||
| ips = [ip.strip() for ip in forwarded.split(",")] | ||
| # Use the last IP in the list as it's the most trusted (closest to our server) | ||
| # or the first if we want the original client (but that's spoofable). | ||
| # Standard practice for identifying the *connecting* client in a trusted chain is often the last one added by the trusted proxy. | ||
| # However, for rate limiting user origin, usually the *first* is used if we trust the chain. | ||
| # If trust_proxy_headers is True, we assume we are behind a trusted proxy (like Render/Cloudflare) | ||
| # which appends the real client IP to the end or beginning depending on config. | ||
| # Let's use the last one as it is the one that connected to the proxy. | ||
| # Actually, X-Forwarded-For: <client>, <proxy1>, <proxy2> | ||
| # If we trust the proxy, the *last* IP is the one that connected to *us* (the proxy), | ||
| # but we want the *original* client. | ||
| # If we trust the proxy, we can trust the *first* non-private IP, or just the first one if strict. | ||
| # Let's stick to the previous logic but simplified: last IP is safer against spoofing if we only trust the immediate upstream. | ||
| if ips: | ||
| client_ip = ips[-1] | ||
|
|
||
| if client_ip == "unknown" and request.client and request.client.host: | ||
| client_ip = request.client.host | ||
|
|
||
| # Sanitize to prevent log injection | ||
| # Allow only alphanumeric, dots, colons (IPv6) | ||
| if not re.match(r"^[a-zA-Z0-9.:]+$", client_ip): | ||
| return "unknown" | ||
|
|
||
| return client_ip |
There was a problem hiding this comment.
IP extraction uses ips[-1] (last hop) — this is typically the proxy, not the original client.
The standard X-Forwarded-For format is <client>, <proxy1>, <proxy2>. Behind a trusted reverse proxy (e.g., Render), the first IP is the real client. Using ips[-1] returns the last proxy that connected to your server — likely the proxy itself — so all clients will share the same rate-limit bucket.
Also, the 12-line comment block (lines 71–82) is indecisive and reads like internal debate notes rather than documentation.
Proposed fix — use first IP and trim the comments
if forwarded:
- # Direct split and strip, no try/except needed as split never raises
ips = [ip.strip() for ip in forwarded.split(",")]
- # Use the last IP in the list as it's the most trusted (closest to our server)
- # or the first if we want the original client (but that's spoofable).
- # Standard practice for identifying the *connecting* client in a trusted chain is often the last one added by the trusted proxy.
- # However, for rate limiting user origin, usually the *first* is used if we trust the chain.
- # If trust_proxy_headers is True, we assume we are behind a trusted proxy (like Render/Cloudflare)
- # which appends the real client IP to the end or beginning depending on config.
- # Let's use the last one as it is the one that connected to the proxy.
- # Actually, X-Forwarded-For: <client>, <proxy1>, <proxy2>
- # If we trust the proxy, the *last* IP is the one that connected to *us* (the proxy),
- # but we want the *original* client.
- # If we trust the proxy, we can trust the *first* non-private IP, or just the first one if strict.
- # Let's stick to the previous logic but simplified: last IP is safer against spoofing if we only trust the immediate upstream.
+ # X-Forwarded-For: <client>, <proxy1>, <proxy2>
+ # Behind a trusted proxy, use the first IP (original client).
if ips:
- client_ip = ips[-1]
+ client_ip = ips[0]📝 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.
Suggested change
| client_ip = "unknown" | |
| if self.trust_proxy_headers: | |
| forwarded = request.headers.get("X-Forwarded-For") | |
| if forwarded and self.trust_proxy_headers: | |
| # 🛡️ Sentinel: The leftmost IP (ips[0]) is the original client IP. | |
| # Each proxy appends its IP to the right, so ips[-1] would be the nearest proxy. | |
| # We use ips[0] to get the original client address for rate limiting. | |
| try: | |
| ips = [ip.strip() for ip in forwarded.split(",")] | |
| client_ip = ips[0] # Original client IP (leftmost) | |
| except Exception: | |
| # Fallback to simple extraction if parsing fails | |
| client_ip = forwarded.split(",")[0].strip() | |
| # Truncate to 100 chars to prevent memory exhaustion attacks | |
| client_ip = client_ip[:100] | |
| else: | |
| client_ip = request.client.host if request.client else "unknown" | |
| # 🛡️ Sentinel: Group IPv6 addresses by /64 prefix to prevent subnet rotation attacks | |
| client_key = self.get_client_key(client_ip) | |
| now = time.time() | |
| # Clean old requests (simple sliding window) | |
| current_requests = self.requests[client_key] | |
| # Prune old timestamps | |
| active_requests = [t for t in current_requests if now - t < self.window] | |
| if len(active_requests) >= self.limit: | |
| # Update map with pruned list before returning | |
| self.requests[client_key] = active_requests | |
| # Calculate retry_after | |
| oldest_request_time = active_requests[0] | |
| reset_time = oldest_request_time + self.window | |
| retry_after = max(1, int(math.ceil(reset_time - now))) | |
| logger.warning(f"Rate limit exceeded for {client_key} on {path}") | |
| return JSONResponse( | |
| status_code=429, | |
| content={"detail": "Too Many Requests", "retry_after": retry_after}, | |
| headers={"Retry-After": str(retry_after)}, | |
| ) | |
| active_requests.append(now) | |
| # Simple Memory Leak Prevention: | |
| # If dictionary gets too large, perform cleanup to prevent OOM. | |
| # 🛡️ Sentinel: Throttle cleanup to prevent CPU exhaustion (DoS) via O(N) loop | |
| if now - self.last_cleanup > self.cleanup_interval: | |
| self.last_cleanup = now | |
| # Cleanup: Remove clients that haven't made a request within the window. | |
| # Since active_requests for each client might not be updated until they make a request, | |
| # we need to check the last timestamp in their list. | |
| # Note: This is an O(N) operation where N is number of clients. | |
| if len(self.requests) > 10000: | |
| stale_ips = [] | |
| for ip, timestamps in self.requests.items(): | |
| # If list is empty (shouldn't happen with logic above but possible) | |
| # or if the most recent request is older than window | |
| if not timestamps or (now - timestamps[-1] > self.window): | |
| stale_ips.append(ip) | |
| for ip in stale_ips: | |
| del self.requests[ip] | |
| # Fallback: If still too large (active attack with >10k distinct IPs), | |
| # 🛡️ Sentinel: Do NOT clear everything, as that allows attackers to reset everyone's limit. | |
| # Instead, if we are full, REJECT new clients. | |
| if len(self.requests) > 10000: | |
| # If the client is already known, we updated them above. | |
| # But wait, if we are > 10000, and this is a NEW client_ip (or one that was just added), | |
| # we should remove it and block. | |
| # However, we already added `now` to `active_requests` and set `self.requests[client_ip]`. | |
| # We need to check if we just added a NEW key that pushed us over. | |
| # If client_ip was already in requests, we are fine (we are just updating an existing slot). | |
| # If client_ip is NEW, and size > 10000, we should reject. | |
| # Optimization: Move the check BEFORE adding to `self.requests`. | |
| # But we used `defaultdict`, so accessing `self.requests[client_ip]` already created the entry if missing. | |
| # So, if we are over limit: | |
| # Check if we should allow this IP. | |
| # If we just created it (len=1), delete it and 503. | |
| if ( | |
| len(active_requests) == 1 | |
| ): # This was a new entry (or re-entry after expiry) | |
| # Safe delete using pop to avoid KeyErrors in race conditions | |
| self.requests.pop(client_key, None) | |
| return Response("Server Busy", status_code=503) | |
| self.requests[client_key] = active_requests | |
| if forwarded: | |
| # Direct split and strip, no try/except needed as split never raises | |
| ips = [ip.strip() for ip in forwarded.split(",")] | |
| # Use the last IP in the list as it's the most trusted (closest to our server) | |
| # or the first if we want the original client (but that's spoofable). | |
| # Standard practice for identifying the *connecting* client in a trusted chain is often the last one added by the trusted proxy. | |
| # However, for rate limiting user origin, usually the *first* is used if we trust the chain. | |
| # If trust_proxy_headers is True, we assume we are behind a trusted proxy (like Render/Cloudflare) | |
| # which appends the real client IP to the end or beginning depending on config. | |
| # Let's use the last one as it is the one that connected to the proxy. | |
| # Actually, X-Forwarded-For: <client>, <proxy1>, <proxy2> | |
| # If we trust the proxy, the *last* IP is the one that connected to *us* (the proxy), | |
| # but we want the *original* client. | |
| # If we trust the proxy, we can trust the *first* non-private IP, or just the first one if strict. | |
| # Let's stick to the previous logic but simplified: last IP is safer against spoofing if we only trust the immediate upstream. | |
| if ips: | |
| client_ip = ips[-1] | |
| if client_ip == "unknown" and request.client and request.client.host: | |
| client_ip = request.client.host | |
| # Sanitize to prevent log injection | |
| # Allow only alphanumeric, dots, colons (IPv6) | |
| if not re.match(r"^[a-zA-Z0-9.:]+$", client_ip): | |
| return "unknown" | |
| return client_ip | |
| client_ip = "unknown" | |
| if self.trust_proxy_headers: | |
| forwarded = request.headers.get("X-Forwarded-For") | |
| if forwarded: | |
| ips = [ip.strip() for ip in forwarded.split(",")] | |
| # X-Forwarded-For: <client>, <proxy1>, <proxy2> | |
| # Behind a trusted proxy, use the first IP (original client). | |
| if ips: | |
| client_ip = ips[0] | |
| if client_ip == "unknown" and request.client and request.client.host: | |
| client_ip = request.client.host | |
| # Sanitize to prevent log injection | |
| # Allow only alphanumeric, dots, colons (IPv6) | |
| if not re.match(r"^[a-zA-Z0-9.:]+$", client_ip): | |
| return "unknown" | |
| return client_ip |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/agent/security.py` around lines 64 - 94, The IP extraction
currently takes the last hop (ips[-1]) which returns the proxy IP; change the
logic in the block that reads X-Forwarded-For (where trust_proxy_headers is
checked and ips is built from request.headers.get("X-Forwarded-For")) to use the
first entry (ips[0].strip()) as the originating client IP, remove the long
indecisive comment block and replace it with a concise clarifying comment like
"Use first X-Forwarded-For entry as originating client when trust_proxy_headers
is True", keep the fallback to request.client.host and the sanitization regex
for client_ip unchanged.
Comment on lines
+67
to
+68
| elif stats == "ERROR": | ||
| plans.append({"branch": branch, "action": "SKIP (Error)", "size": 0, "details": "Git command failed"}) |
There was a problem hiding this comment.
Extra leading space causes inconsistent indentation on the ERROR branch.
Line 68 has 13 spaces of indentation vs. 12 for every other if/elif body. Python accepts it (single-statement block), but it's visually misleading.
♻️ Proposed fix
elif stats == "ERROR":
- plans.append({"branch": branch, "action": "SKIP (Error)", "size": 0, "details": "Git command failed"})
+ plans.append({"branch": branch, "action": "SKIP (Error)", "size": 0, "details": "Git command failed"})📝 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.
Suggested change
| elif stats == "ERROR": | |
| plans.append({"branch": branch, "action": "SKIP (Error)", "size": 0, "details": "Git command failed"}) | |
| elif stats == "ERROR": | |
| plans.append({"branch": branch, "action": "SKIP (Error)", "size": 0, "details": "Git command failed"}) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/pruning_plan.py` around lines 67 - 68, The elif branch handling stats
== "ERROR" has one extra leading space in its body, causing inconsistent
indentation; locate the elif stats == "ERROR" block in scripts/pruning_plan.py
and normalize the indentation of the plans.append line to match the other
if/elif bodies (same indent as other plans.append calls) so the visual alignment
is consistent around the branch/ action/size/details dict construction.
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Owner
Author
|
@jules Resolve conflicts: Avoid full repo diff - focus only on your changed paths. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GoogleGenAIGemmaClientto use Gemma models viaGEMINI_API_KEY(Google GenAI SDK), bypassing the need for heavy local inference.AppConfigto default togoogle_genaiprovider andgemma-2-27b-itmodel.render.yamlwith Free Tier optimized environment variables (Gemma via API, skip Playwright)./->/app) inbackend/src/agent/app.pyfor better UX.DEPLOY_RENDER.mdwith step-by-step instructions.PR created automatically by Jules for task 18323209961354138092 started by @MasumRab
Summary by CodeRabbit
New Features
Improvements