🛡️ Sentinel: [HIGH] Fix IP spoofing vulnerability in RateLimitMiddleware - #310
🛡️ Sentinel: [HIGH] Fix IP spoofing vulnerability in RateLimitMiddleware#310MasumRab wants to merge 3 commits into
Conversation
…oxy headers The RateLimitMiddleware previously trusted the `X-Forwarded-For` header by default, allowing attackers to bypass rate limits by spoofing their IP address. This change: 1. Adds `trust_proxy_headers` configuration to `AppConfig` (default: False). 2. Updates `RateLimitMiddleware` to ignore `X-Forwarded-For` unless `trust_proxy_headers` is True. 3. Updates `app.py` to pass the configuration to the middleware. 4. Adds regression tests to verify secure behavior by default and correct proxy handling when enabled. Co-authored-by: MasumRab <8943353+MasumRab@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. |
|
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. WalkthroughThis pull request modernizes the codebase by standardizing type annotations to Python 3.10+ union syntax, strengthening security with proxy header trust configuration, expanding the graph architecture with new nodes and routing modes, and adding enhanced state management with new fields for validation and context tracking. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RateLimitMiddleware
participant Proxy as Trusted Proxy<br/>(or Attacker)
participant Backend
Note over Client,Backend: trust_proxy_headers = False (Secure Default)
Client->>Proxy: Request with X-Forwarded-For: 1.1.1.1
Proxy->>RateLimitMiddleware: Forward request
RateLimitMiddleware->>RateLimitMiddleware: Extract client IP from<br/>request.client.host (ignore header)
RateLimitMiddleware->>Backend: Allow (real client tracked)
Proxy->>RateLimitMiddleware: Spoofed X-Forwarded-For: 2.2.2.2
RateLimitMiddleware->>RateLimitMiddleware: Extract client IP from<br/>request.client.host (ignore header)
RateLimitMiddleware->>Backend: Rate limit enforced on same IP
Note over Client,Backend: trust_proxy_headers = True (Explicit Trust)
Proxy->>RateLimitMiddleware: Forward with X-Forwarded-For: 1.1.1.1
RateLimitMiddleware->>RateLimitMiddleware: Extract from X-Forwarded-For<br/>(trust enabled)
RateLimitMiddleware->>Backend: Track IP 1.1.1.1
Proxy->>RateLimitMiddleware: Forward with X-Forwarded-For: 2.2.2.2
RateLimitMiddleware->>RateLimitMiddleware: Extract from X-Forwarded-For<br/>(different IP seen)
RateLimitMiddleware->>Backend: New client, separate quota
sequenceDiagram
participant Client
participant SearchRouter
participant PrimaryProvider
participant FallbackProvider
Client->>SearchRouter: search(query, tuned=True)
SearchRouter->>PrimaryProvider: Attempt tuned search
alt Tuned search succeeds
PrimaryProvider-->>SearchRouter: SearchResult[]
SearchRouter-->>Client: Return results
else Tuned search fails
SearchRouter->>PrimaryProvider: Retry untuned search
alt Untuned search succeeds
PrimaryProvider-->>SearchRouter: SearchResult[]
SearchRouter-->>Client: Return results
else Both fail & fallback configured
SearchRouter->>FallbackProvider: Attempt fallback search
FallbackProvider-->>SearchRouter: SearchResult[]
SearchRouter-->>Client: Return fallback results
else No results available
SearchRouter-->>Client: Empty list (log errors)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
backend/src/agent/llm_client.py (1)
61-63:⚠️ Potential issue | 🟡 MinorLog message is misleading on final retry attempt.
The message "attempting retry" is logged on every failure, including the final attempt where no retry will occur. This could be confusing during log analysis.
🔧 Suggested fix
except Exception as e: - logger.warning(f"LLM call failed (attempting retry): {e}") - raise e + logger.warning(f"LLM call failed: {e}") + raiseUsing bare
raiseis also preferred style in except blocks. The tenacity decorator handles retry decisions externally.backend/tests/test_kaggle_integration.py (1)
80-105:⚠️ Potential issue | 🟡 MinorMocked tokenizer
.to()should return the same inputs.Right now
inputs = tokenizer(...).to(...)becomes a new MagicMock withoutinput_ids.shape, so the slice logic isn’t actually exercised. Return the sameinputsfrom.to().✅ Suggested fix
inputs = MagicMock() inputs.input_ids.shape = [1, 5] # 5 input tokens + inputs.to.return_value = inputs mock_tokenizer.return_value = inputsbackend/tests/test_mcp_tools.py (1)
29-57:⚠️ Potential issue | 🟠 MajorMock
SSEConnectionas an async context manager.
get_tools_from_mcpusesasync with SSEConnection(...), but the tests currently provide a plainMagicMock. That object won’t satisfy the async context manager protocol, so these tests can raise before reachingload_mcp_tools. ConfigureSSEConnectionwithAsyncMock(and__aenter__) in both tests.🛠 Suggested fix
-from unittest.mock import MagicMock, patch +from unittest.mock import AsyncMock, MagicMock, patch @@ - mock_sessions_module = MagicMock() + mock_sessions_module = MagicMock() + mock_session = AsyncMock() + mock_session.__aenter__.return_value = MagicMock() + mock_sessions_module.SSEConnection = AsyncMock(return_value=mock_session) @@ - mock_conn_cls = mock_sessions_module.SSEConnection + mock_conn_cls = mock_sessions_module.SSEConnection @@ - mock_sessions_module = MagicMock() + mock_sessions_module = MagicMock() + mock_session = AsyncMock() + mock_session.__aenter__.return_value = MagicMock() + mock_sessions_module.SSEConnection = AsyncMock(return_value=mock_session)Also applies to: 82-89
backend/src/agent/mcp_server.py (1)
190-224:⚠️ Potential issue | 🟡 MinorAdd directory type validation before iteration.
The
list_directorymethod checks if the path exists but doesn't verify it's actually a directory. If a file path is passed,iterdir()will raiseNotADirectoryError.🛡️ Proposed fix
try: dir_path = Path(path) if not dir_path.exists(): return ToolResult(success=False, error=f"Directory not found: {path}") + if not dir_path.is_dir(): + return ToolResult(success=False, error=f"Path is not a directory: {path}") items = []backend/src/agent/graph.py (1)
176-190:⚠️ Potential issue | 🟡 MinorStale graph documentation doesn't match actual edges.
The
graph_registry.document_edgecalls are inconsistent with the actual graph wiring:
- Line 176-179: Documents
kg_enrich -> reflection, but actual edge iskg_enrich -> checklist_verifier(line 123)- Line 181-185: Documents
reflection -> web_research, but no such edge exists- Line 186-190: Documents
reflection -> denoising_refiner, but actual edge isreflection -> update_plan(line 127)📝 Proposed fix to align documentation with actual edges
graph_registry.document_edge( "kg_enrich", - "reflection", - description="Enriched/Compressed results reach the reasoning loop.", + "checklist_verifier", + description="Enriched/Compressed results are verified against the outline.", +) +graph_registry.document_edge( + "checklist_verifier", + "reflection", + description="Verified results reach the reasoning loop.", ) graph_registry.document_edge( "reflection", - "web_research", - description="Follow-up queries trigger additional web searches until sufficient.", -) -graph_registry.document_edge( - "reflection", - "denoising_refiner", - description="Once sufficient or max loops reached, finalize the response via high-fidelity refiner.", + "update_plan", + description="Reflection updates the plan for next iteration.", )backend/src/agent/rag.py (1)
101-131:⚠️ Potential issue | 🟠 MajorMisleading warning log inside Chroma initialization block.
Lines 129-131 log "Dual write enabled but ChromaDB is missing. Writing to FAISS only." However, this code is inside the
if self.use_chroma and CHROMA_AVAILABLE:block (line 102), meaning it only executes when ChromaDB is available. This warning message appears to be a copy-paste error from elsewhere or leftover code.🐛 Proposed fix - remove the misplaced warning
self.chroma = ChromaStore( collection_name="deep_search_evidence", persist_path=persist_path, embedding_function=embedding_fn, ) - logger.warning( - "Dual write enabled but ChromaDB is missing. Writing to FAISS only." - ) + logger.info( + f"ChromaStore initialized at '{persist_path}'" + )Alternatively, if this warning was intended for a different condition, it should be moved outside this block to where
self.use_chromamight be False despiteself.config.dual_writebeing True.backend/tests/agent/test_api_security.py (1)
91-109:⚠️ Potential issue | 🟠 MajorTest will fail:
trust_proxy_headersnot enabled in fixture.This test expects
X-Forwarded-Forto be respected for rate limiting, but the middleware fixture (lines 17-19) doesn't settrust_proxy_headers=True. Per the PR's security fix, the middleware now ignoresX-Forwarded-Forby default.The test will fail because both
headers_aandheaders_brequests will be rate-limited under the same client IP (request.client.host), not differentiated by theX-Forwarded-Forheader.🐛 Proposed fix: Enable proxy trust in fixture or create separate fixture
Option 1: Update the existing fixture to enable proxy headers for this specific test scenario:
app.add_middleware( - RateLimitMiddleware, limit=5, window=1, protected_paths=["/agent"] + RateLimitMiddleware, limit=5, window=1, protected_paths=["/agent"], trust_proxy_headers=True )Option 2: Create a separate fixture with proxy trust enabled specifically for
test_rate_limit_respects_x_forwarded_for, keeping the default fixture secure for other tests.backend/tests/test_state.py (1)
157-168:⚠️ Potential issue | 🟡 MinorUse
isfor type identity comparison.Per Python best practices (and flagged by Ruff E721), type comparisons should use
isinstead of==because types are singletons.🔧 Proposed fix
else: - assert anno == bool + assert anno is boolbackend/src/agent/nodes.py (1)
739-885:⚠️ Potential issue | 🟠 MajorNormalize plan item keys to avoid missing queries in sequential flows.
generate_planbuilds items with"task", whileupdate_planemits"title"/"query".select_next_taskonly checks"query"/"title", so plans produced bygenerate_plancan yieldNonequeries. Standardize the schema or add backward-compat fallbacks to prevent empty searches.🔧 Suggested compatibility fix
@@ - todo = { - "title": item.get("title", ""), - "description": item.get("description", ""), - "status": item.get("status", "pending"), - "query": item.get("title", ""), - } + title = item.get("title", "") + todo = { + "task": title, # backward-compat with existing plan schema + "title": title, + "description": item.get("description", ""), + "status": item.get("status", "pending"), + "query": item.get("query") or title, + } @@ - todo = { - "title": item.title, - "description": item.description, - "status": item.status, - "query": item.title, - } + title = item.title + todo = { + "task": title, # backward-compat + "title": title, + "description": item.description, + "status": item.status, + "query": getattr(item, "query", None) or title, + } @@ - return { - "current_task_idx": idx, - "search_query": [task.get("query") or task.get("title")], - } + query = task.get("query") or task.get("title") or task.get("task") + return { + "current_task_idx": idx, + "search_query": [query] if query else [], + }backend/src/agent/graphs/parallel.py (1)
19-38:⚠️ Potential issue | 🔴 CriticalThe parallel.py graph will fail at runtime when
planning_routerreturns"select_next_task".
planning_routerreturns"select_next_task"when/end_planor/confirm_plancommands are executed, or whenrequire_planning_confirmationisFalse(the default path). However, the conditional edges at lines 33–38 only allow["planning_wait", "web_research"], causing an unmapped route error.The correct fix is to update the allowed routes to match what
planning_routeractually returns:Recommended fix
builder.add_conditional_edges( - "planning_mode", planning_router, ["planning_wait", "web_research"] + "planning_mode", planning_router, ["planning_wait", "select_next_task"] ) builder.add_conditional_edges( - "planning_wait", planning_router, ["planning_wait", "web_research"] + "planning_wait", planning_router, ["planning_wait", "select_next_task"] )Note: This same issue exists in
planning.py,supervisor.py, andlinear.py. Referencegraph.pywhich has the correct routing.backend/src/evaluation/bench.py (1)
39-43:⚠️ Potential issue | 🟡 MinorMissing explicit file encoding.
Opening files without specifying
encodingmay causeUnicodeDecodeErroron systems where the default encoding isn't UTF-8, or if the JSONL files contain non-ASCII characters.🛡️ Proposed fix
- with open(file_path) as f: + with open(file_path, encoding="utf-8") as f: for line in f:backend/src/agent/orchestration.py (1)
457-460:⚠️ Potential issue | 🟠 Major
tools.get_tools()called whentoolsmay beNone.The parameter
tools: ToolRegistry | None = NoneallowsNone, but line 458 callstools.get_tools()without checking forNonefirst. This will raiseAttributeErroriftoolsisNone.🐛 Proposed fix
# Add tool node if tools are available - lc_tools = tools.get_tools() + lc_tools = tools.get_tools() if tools else [] if lc_tools: builder.add_node("tools", ToolNode(lc_tools))
🤖 Fix all issues with AI agents
In `@backend/examples/kaggle_integration.py`:
- Around line 197-215: The unary operator handling in the calculator is wrong:
the operators dict currently contains ast.UnaryOp as a key but the _eval unary
branch looks up type(node.op) (e.g., ast.USub/ast.UAdd), causing a KeyError for
-1/+1; update the operators mapping to include ast.USub: operator.neg and
ast.UAdd: operator.pos (or a no-op for UAdd) instead of ast.UnaryOp, and ensure
the unary branch in _eval continues to call
operators[type(node.op)](_eval(node.operand)); adjust the same mapping/lookup at
the other occurrence mentioned (around the second location) so both unary + and
- are handled correctly.
In `@backend/scripts/verify_imports.py`:
- Around line 7-16: The try blocks in verify_imports.py currently only print
success messages so imports are never actually tested; restore real import
checks by importing the modules (e.g., the Configuration module and the nodes
module) or use importlib.import_module with the correct module paths inside the
try blocks, catch exceptions as before and print the failure with the exception
(and exit non-zero on error). Update the that imports are performed where the
success prints currently are (the blocks referencing "Configuration" and
"nodes") so a failed import raises and is reported.
In `@backend/src/agent/_graph.py`:
- Around line 330-337: The loop that retries when finish_reason == "length" can
spin forever; modify the retry logic inside the while loop that calls
llm.invoke(...) (using symbols finish_reason, llm.invoke, final_answer,
response_metadata) to enforce a maximum number of continuation attempts (e.g.,
max_retries) and increment a counter each iteration, and when the counter
exceeds the limit either break and return the current final_answer or raise a
controlled exception; also ensure you still extract continuation content via
getattr(continuation, "content", str(continuation)) and update response_metadata
as before, and optionally log a warning when the retry limit is hit.
In `@backend/src/agent/configuration.py`:
- Around line 79-92: The type-comparison logic in the boolean and integer
conversion branches uses equality (==) instead of identity; update the checks so
that comparisons use "is" (e.g., change "field_type == bool" to "field_type is
bool" and "field_type.__origin__ == bool" to "field_type.__origin__ is bool",
likewise for int) while keeping the existing hasattr(field_type, "__origin__")
guards and the rest of the conversion logic (references: field_type, value).
In `@backend/src/agent/graph.py`:
- Around line 92-100: The scoping_router function returns "generate_plan" when
scoping_status != "active", but builder.add_conditional_edges for "scoping_node"
only lists destinations ["planning_wait", "outline_gen"], causing a runtime
ValueError; fix by making scoping_router return one of the declared destinations
(e.g., return "outline_gen" instead of "generate_plan") or update the
builder.add_conditional_edges call to include "generate_plan" as a valid
destination so the return values of scoping_router and the conditional edge list
are consistent.
In `@backend/src/agent/mcp_client.py`:
- Around line 74-84: The docstring for plan_tool_sequence lacks the required
blank line between the one-line summary and the longer description; update the
plan_tool_sequence function's docstring so there is an empty line after the
summary line (i.e., after """Use LLM to plan sequence of tool calls for a task.)
keeping the rest of the description intact and ensure formatting still describes
the planned tool use and any subsequent comments referencing self.tool_registry
remain unchanged.
In `@backend/src/agent/orchestration.py`:
- Around line 436-440: create_coordinator_node is being passed tools from
builder.add_node without a null check, and create_coordinator_node calls
tools.get_tool_names() (so if tools is None it will raise); update the call site
or create_coordinator_node to ensure tools is non-null by initializing tools to
an empty Tools-like object or list and/or adding a guard: when builder adds the
"coordinator" node pass a safe default (e.g., an empty tools collection) or
modify create_coordinator_node to check for None and treat it as an empty
toolset before calling tools.get_tool_names(); reference
create_coordinator_node, builder.add_node("coordinator", ...) and the
tools.get_tool_names usage when making the change.
In `@backend/src/agent/persistence.py`:
- Around line 8-14: The sanitized safe_id in _get_plan_path can be empty
(yielding "plans/.json"); modify _get_plan_path to detect empty safe_id and
handle it: either raise a ValueError for invalid thread_id or (recommended)
produce a stable fallback filename (e.g., compute a deterministic digest of the
original thread_id such as a sha256 hex or hex[:8] and prepend a fixed prefix)
and use that as safe_id, ensuring the result remains filesystem-safe and unique;
keep the existing PLAN_DIR creation and join logic intact and reference
_get_plan_path and safe_id when making the change.
In `@backend/src/evaluation/bench.py`:
- Around line 132-140: The file write uses open(output_path, "w") without an
explicit encoding which can cause cross-platform inconsistencies; update the
open call in backend/src/evaluation/bench.py (the open(...) that writes the JSON
with json.dump and variables final_scores/all_results) to include
encoding="utf-8" so the JSON is written using UTF-8 consistently.
In `@backend/src/search/providers/brave_adapter.py`:
- Around line 26-30: The region parameter in the search method is accepted but
never used; update the request params in brave_adapter.py (the method that takes
region: str | None) to map region to the Brave API country query parameter
(params["country"]) similar to how time_range maps to params["freshness"]; only
set params["country"] when region is provided and normalize it to an ISO 3166-1
alpha-2 format (e.g., uppercased) before adding.
In `@backend/tests/test_graph_mock.py`:
- Around line 1-15: The test currently redefines the imported symbol TEST_MODEL
by declaring a local constant TEST_MODEL = "gemma-3-27b-it", which shadows the
imported value; remove the local override or remove the import so only one
definition remains (e.g., delete the local TEST_MODEL assignment or stop
importing TEST_MODEL from agent.models) and update any references in the test to
use the single remaining symbol to avoid redefinition/shadowing.
In `@backend/tests/test_search_router.py`:
- Around line 15-21: The import order patching sys.modules["google.genai"] must
stay before importing SearchResult and SearchRouter, so silence Ruff E402 by
adding a local noqa on the affected import lines: append " # noqa: E402" to the
lines that import SearchResult (from search.provider import SearchResult) and
SearchRouter (from search.router import SearchRouter) so the sys.modules mock
can precede those imports without a linter failure.
In `@backend/tests/test_utils.py`:
- Around line 8-33: Move the module-level import of get_citations,
get_research_topic, insert_citation_markers, and resolve_urls so it appears with
the other top-level imports (before any function or helper definitions like
make_human_message and make_ai_message) and remove any duplicate imports later
in the file; ensure all imports are consolidated at the top of the module to
satisfy Ruff E402.
🧹 Nitpick comments (31)
backend/src/observability/config.py (1)
7-12: DRY up truthy parsing for env flags.Both functions duplicate the same truthy parsing tuple. Consider extracting a small helper/constant to keep parsing consistent and easier to update.
♻️ Suggested refactor
import os + +TRUTHY = {"true", "1", "yes", "on"} + +def _is_truthy(value: str | None) -> bool: + return (value or "").lower() in TRUTHY def is_enabled() -> bool: """Check if Langfuse observability is enabled via environment variables.""" # Check if explicitly enabled - enabled = os.getenv("LANGFUSE_ENABLED", "false").lower() in ( - "true", - "1", - "yes", - "on", - ) + enabled = _is_truthy(os.getenv("LANGFUSE_ENABLED", "false")) if not enabled: return False @@ def is_audit_mode() -> bool: """Check if audit mode is enabled for richer metadata.""" - return os.getenv("AUDIT_MODE", "false").lower() in ("true", "1", "yes", "on") + return _is_truthy(os.getenv("AUDIT_MODE", "false"))Also applies to: 25-26
backend/scripts/visualize_dependencies.py (1)
129-131: Remove unused variable assignment.The
dendrogramvariable is assigned but never used. Thesch.dendrogram()call renders the plot as a side effect, so the return value can be discarded.♻️ Proposed fix
# Create dendrogram - dendrogram = sch.dendrogram( + sch.dendrogram( Z, labels=modules, orientation="right", leaf_font_size=10 )backend/src/agent/llm_client.py (2)
18-22: Consider narrowing the retry scope to transient errors only.Retrying on all
Exceptiontypes will cause unnecessary delays for non-transient errors (e.g.,ValueError,TypeError,AuthenticationError). As the comment notes, this should ideally target specific transient failures like network timeouts, connection errors, or rate limits.♻️ Suggested improvement
+from requests.exceptions import RequestException, Timeout +# or for httpx: from httpx import HTTPStatusError, TimeoutException + `@retry`( stop=stop_after_attempt(3), wait=wait_exponential(multiplier=1, min=1, max=10), - retry=retry_if_exception_type(Exception), + retry=retry_if_exception_type((RequestException, Timeout, ConnectionError, TimeoutError)), reraise=True, )Adjust the exception tuple based on the actual HTTP client and LLM SDK in use.
50-52: Minor inconsistency in return value handling.Line 51 returns
response.textdirectly, while other branches wrap withstr()(lines 41, 52, 58). Ifresponse.textcould ever beNoneor a non-string type, this would cause inconsistent behavior. Consider wrapping for consistency.♻️ Optional fix for consistency
if hasattr(response, "text"): - return response.text + return str(response.text) if response.text else "" return str(response)backend/src/evaluation/deep_research_bench.py (1)
35-37: Docstring style nit (from static analysis).Ruff D200/D401 flags this docstring: it spans two lines and isn't in imperative mood. Consider:
-def evaluate_deep_research(): - """Evaluates the agent on DeepResearch-Bench (muset-ai). - """ +def evaluate_deep_research(): + """Evaluate the agent on DeepResearch-Bench (muset-ai)."""This is a minor style issue in placeholder code.
backend/src/evaluation/mle_bench.py (2)
31-32: Docstring formatting can be improved.Static analysis flagged that the docstring should fit on one line and use imperative mood. Since this is a stub file, this is a minor nitpick.
♻️ Optional: Single-line imperative docstring
def evaluate_mle_bench(): - """Evaluates the agent on MLE-bench tasks. - """ + """Evaluate the agent on MLE-bench tasks."""
37-37: Unused variables flagged by static analysis.
resultsandscoresare assigned but never used (F841). Since this is intentional placeholder code for future implementation, consider suppressing the warnings with# noqa: F841comments to make the intent explicit.♻️ Optional: Add noqa comments for intentional placeholders
# TODO(priority=High, complexity=Medium): [mle_bench:2] Run agent - results = [] + results = [] # noqa: F841 for task in dataset: # output = run_agent(task.prompt) # results.append({"task_id": task.id, "output": output}) pass # TODO(priority=Medium, complexity=Medium): [mle_bench:3] Evaluate - scores = [] + scores = [] # noqa: F841Also applies to: 44-44
backend/src/evaluation/metrics.py (1)
261-263: Consider adding punctuation to docstrings.Static analysis flags that helper method docstrings should end with proper punctuation (period, question mark, or exclamation point) per D415.
📝 Optional docstring fix
`@staticmethod` def _extract_facts(text: str) -> List[str]: - """Extract fact-like sentences from text""" + """Extract fact-like sentences from text."""`@staticmethod` def _extract_domain(url: str) -> str: - """Extract domain from URL""" + """Extract domain from URL.""".Jules/sentinel.md (1)
1-4: Good security documentation with a minor date discrepancy.The documentation clearly explains the vulnerability, learning, and prevention guidance. This is valuable for future reference.
Note: The date "2025-02-18" appears to be inconsistent with the PR creation date (2026-02-01). Please verify if this should be updated.
📝 Suggested date correction
-## 2025-02-18 - Rate Limit IP Spoofing via Unverified Proxy Headers +## 2026-02-01 - Rate Limit IP Spoofing via Unverified Proxy Headersbackend/src/config/app_config.py (1)
20-63: Normalize comma-separated env lists to avoid whitespace bugs.
Values like"https://a.com, https://b.com"will currently retain leading spaces and may fail matching. Consider stripping entries when splitting.♻️ Suggested refactor
- kg_allowlist: Tuple[str, ...] = tuple( - filter(None, os.getenv("KG_ALLOWLIST", "").split(",")) - ) + kg_allowlist: Tuple[str, ...] = tuple( + filter( + None, + (v.strip() for v in os.getenv("KG_ALLOWLIST", "").split(",")), + ) + ) @@ - cors_origins: Tuple[str, ...] = tuple( - filter(None, os.getenv("CORS_ORIGINS", "http://localhost:5173").split(",")) - ) + cors_origins: Tuple[str, ...] = tuple( + filter( + None, + (v.strip() for v in os.getenv("CORS_ORIGINS", "http://localhost:5173").split(",")), + ) + ) @@ - allowed_hosts: Tuple[str, ...] = tuple( - filter(None, os.getenv("ALLOWED_HOSTS", "localhost,127.0.0.1").split(",")) - ) + allowed_hosts: Tuple[str, ...] = tuple( + filter( + None, + (v.strip() for v in os.getenv("ALLOWED_HOSTS", "localhost,127.0.0.1").split(",")), + ) + ) @@ - CORS_ORIGINS: List[str] = field( - default_factory=lambda: os.getenv( - "CORS_ORIGINS", "http://localhost:5173" - ).split(",") - ) + CORS_ORIGINS: List[str] = field( + default_factory=lambda: [ + v.strip() + for v in os.getenv("CORS_ORIGINS", "http://localhost:5173").split(",") + if v.strip() + ] + )Also applies to: 71-75
backend/src/agent/tool_adapter.py (1)
164-170: Consider moving theuuidimport to module level.The
import uuidstatement inside the function body works but is unconventional. Module-level imports are preferred for clarity and to avoid repeated import overhead on each call (though Python caches imports).♻️ Suggested refactor
import json import logging import re +import uuid from typing import Any, Dict, ListThen at line 166:
- import uuid - call_id = f"call_{uuid.uuid4().hex[:8]}"backend/src/agent/mcp_server.py (1)
203-209: Handlestat()errors for special files.Calling
f.stat().st_sizeon broken symlinks or special files can raise exceptions. Consider wrapping this in a try-except to ensure directory listing doesn't fail on individual problematic entries.🛡️ Proposed fix
for f in dir_path.iterdir(): - items.append( - { - "name": f.name, - "type": "file" if f.is_file() else "directory", - "size": f.stat().st_size if f.is_file() else None, - } - ) + try: + is_file = f.is_file() + items.append( + { + "name": f.name, + "type": "file" if is_file else "directory", + "size": f.stat().st_size if is_file else None, + } + ) + except OSError: + # Handle broken symlinks or inaccessible files + items.append({"name": f.name, "type": "unknown", "size": None}) count += 1backend/src/search/providers/duckduckgo_adapter.py (2)
62-64: Simplify exception re-raising.Using
raise eis functionally correct butraisealone preserves the original traceback more cleanly.♻️ Suggested fix
except Exception as e: logger.error(f"DuckDuckGo search failed: {e}") - raise e + raise
18-26: Review defaultregionvalue alignment with base class.The
region="wt-wt"default inDuckDuckGoAdapter.search()is inconsistent with the base classSearchProvider.search()which defaults toNone, and is the only adapter that overrides this default. While the implementation includes fallback logic (ddg_region = region if region else "wt-wt"), this inconsistency may cause confusion for developers. Either align the default with other adapters or add a comment explaining why DuckDuckGo requires a distinct default.backend/src/agent/deep_search_agent.py (2)
112-123: Fix type annotation formcp_serversparameter.The parameter
mcp_servers: List = Noneshould useList | None = Nonefor consistency with the modern type annotation style used throughout this PR.♻️ Proposed fix
class DeepSearchAgent: - def __init__(self, llm_client, mcp_servers: List = None): + def __init__(self, llm_client, mcp_servers: List | None = None): self.llm = llm_client
218-226: In-place plan modification may cause side effects.The code modifies
step["params"]directly, which mutates the originalsave_planlist. If the caller retains a reference, this could cause unexpected behavior. Consider working on a copy if the plan should remain immutable.♻️ Proposed fix using deep copy
+ import copy + save_plan = copy.deepcopy(save_plan) + for step in save_plan: if step.get("tool", "").endswith("write_file"):backend/src/search/providers/tavily_adapter.py (2)
32-48: Document unused parameters in method signature.The
region,time_range, andsafe_searchparameters are defined in the signature (inherited from base class) but not used in the Tavily API call. Consider adding a brief comment noting these are present for interface compliance.📝 Suggested documentation
def search( self, query: str, max_results: int = 5, - region: str | None = None, - time_range: str | None = None, - safe_search: bool = True, + region: str | None = None, # Not supported by Tavily + time_range: str | None = None, # Not supported by Tavily + safe_search: bool = True, # Not supported by Tavily tuned: bool = True, ) -> List[SearchResult]:
81-84: Simplify exception re-raising.Same pattern as the DuckDuckGo adapter -
raisealone is cleaner thanraise e.♻️ Suggested fix
except Exception as e: logger.error(f"Tavily Search failed: {e}") - raise e + raisebackend/src/observability/langfuse.py (1)
121-129: Consider using bareraiseinstead ofraise e.While functionally equivalent in this context, using bare
raiseis the idiomatic Python pattern as it preserves the complete traceback without modification.♻️ Suggested change
except Exception as e: # If the exception came from observe() itself (unlikely) or the yield, it bubbles here. # Langfuse's observe() re-raises exceptions from the body. # We just need to make sure we don't suppress it. # The previous code had `yield` inside `try` and suppressed `Exception`. # By removing the suppression, we are good. # But wait, we still want to log if it was an observability error vs app error? # No, simpler is better. Just let it bubble. - raise e + raisebackend/src/rag/chroma_store.py (1)
86-91: Redundant conditional expression.The expression
embeddings if embeddings is not None else Nonesimplifies to justembeddings, since whenembeddings is None, the result isNoneanyway.♻️ Suggested simplification
self.collection.upsert( ids=ids, documents=documents, metadatas=metadatas, - embeddings=embeddings if embeddings is not None else None, + embeddings=embeddings, )backend/src/agent/research_tools.py (1)
513-537: Unused parametermodel_nameinis_token_limit_exceeded.The
model_nameparameter is documented as being used "to optimize provider detection" but is never referenced in the function body. Either remove it or implement the intended provider-specific logic.♻️ Option 1: Remove unused parameter
-def is_token_limit_exceeded(exception: Exception, model_name: str = None) -> bool: +def is_token_limit_exceeded(exception: Exception) -> bool: """Determine if an exception indicates a token/context limit was exceeded. Args: exception: The exception to analyze - model_name: Optional model name to optimize provider detection Returns: True if the exception indicates a token limit was exceeded, False otherwise """♻️ Option 2: Implement provider-specific detection
def is_token_limit_exceeded(exception: Exception, model_name: str | None = None) -> bool: error_str = str(exception).lower() # Provider-specific patterns if model_name: if "gemini" in model_name.lower() or "google" in model_name.lower(): if "resource exhausted" in error_str: return True elif "openai" in model_name.lower() or "gpt" in model_name.lower(): if "maximum context length" in error_str: return True # Generic patterns token_keywords = [...] return any(keyword in error_str for keyword in token_keywords)backend/tests/test_notebook_logic.py (1)
44-49: Address static analysis findings in test.Per Ruff analysis:
- Line 44: The
llmvariable is assigned but never used. The mock assertion on Line 47 validates the call, so the variable itself is unnecessary.- Line 49: Using
♻️ Proposed fix
# Instantiate LLM - llm = mock_llm_class(model=model_name, temperature=0) + mock_llm_class(model=model_name, temperature=0) # Assertions mock_llm_class.assert_called_with(model="gemma-3-27b-it", temperature=0) self.assertEqual(model_name, "gemma-3-27b-it") - print("✅ Notebook logic for model selection is correct.")backend/src/agent/scoping_schema.py (1)
6-8: Consider collapsing docstring to single line.Per Ruff D200, the class docstring can fit on one line.
♻️ Proposed fix
class ScopingAssessment(BaseModel): - """Assessment of whether the user's query requires clarification. - """ + """Assessment of whether the user's query requires clarification."""backend/src/agent/state.py (1)
96-98: Unusual multi-line parenthesized type hint.The parenthesized formatting for
todo_listtype hint is unconventional and may confuse readers or formatters:todo_list: ( List[dict] | None )Standard style would be
todo_list: List[dict] | Noneon a single line, or useOptional[List[dict]]if line length is a concern.backend/src/agent/graphs/supervisor.py (1)
61-64: Truncation may corrupt URLs or markdown links.Slicing
combined_text[:max_chars]at a fixed character boundary could break mid-URL or mid-markdown-link (e.g.,[Title](http://...), which may confuse the LLM or corrupt citation formatting.Consider truncating at a paragraph or sentence boundary instead.
♻️ Proposed fix for safer truncation
# Safety: avoid blowing context if it's massive max_chars = 50000 if len(combined_text) > max_chars: - combined_text = combined_text[:max_chars] + "\n[... truncated]" + # Truncate at last complete paragraph/newline before max_chars + truncate_at = combined_text.rfind("\n\n", 0, max_chars) + if truncate_at == -1: + truncate_at = combined_text.rfind("\n", 0, max_chars) + if truncate_at == -1: + truncate_at = max_chars + combined_text = combined_text[:truncate_at] + "\n\n[... truncated]"backend/src/search/providers/brave_adapter.py (1)
69-71: Use bareraiseinstead ofraise e.Using
raise einstead ofraiseresets the traceback to this line, losing the original stack trace. Use bareraiseto preserve the full traceback.♻️ Proposed fix
except Exception as e: logger.error(f"Brave Search failed: {e}") - raise e + raisebackend/src/evaluation/bench.py (2)
190-198: Redundantpass_at_1_accuracycomputation.
pass_at_1_accuracyis computed twice: once at line 166-170 for thepass_at_1result, and again at lines 193-197 for thecontext_efficiencyscore. This duplicates potentially expensive LLM calls or computations.♻️ Proposed fix to reuse the computed score
# Metric 5: Context Efficiency "context_efficiency": self.metrics.context_efficiency( final_answer_length=len(generated_answer), total_context_length=len(context_used), - answer_quality_score=self.metrics.pass_at_1_accuracy( - generated_answer, - reference["reference_answer"], - reference["key_facts"], - )["score"], + answer_quality_score=results["pass_at_1"]["score"], # Reuse computed result ),Note: This requires moving the
context_efficiencycomputation afterpass_at_1is added toresults, or restructuring the dict construction.
98-103: Consider using logger for traceback instead of print.
traceback.print_exc()writes to stderr. For consistency with the rest of the logging, consider usinglogger.exception()which automatically includes the traceback.♻️ Proposed fix
except Exception as e: - logger.error(f"Error evaluating {query_id}: {e}") - import traceback - - traceback.print_exc() + logger.exception(f"Error evaluating {query_id}: {e}") continuebackend/src/agent/rate_limiter.py (1)
163-165: Consider using a custom exception for quota exceeded.Raising a generic
Exceptionmakes it difficult for callers to specifically catch and handle quota exhaustion scenarios (e.g., to implement backoff or notify users). A customQuotaExceededExceptionwould be cleaner.♻️ Proposed fix
Add at the top of the file:
class QuotaExceededException(Exception): """Raised when daily API quota is exhausted.""" passThen update the raise:
- raise Exception( + raise QuotaExceededException( f"Daily quota exceeded for {self.model}. Resets at midnight Pacific time." )backend/src/agent/orchestration.py (2)
254-267: Mutable default argumentcapabilities: List[str] = None.Using
Noneas a default then assigningcapabilities or []works, but the type hintList[str] = Noneis misleading. For consistency withAgentSpec(which usesfield(default_factory=list)), consider:♻️ Proposed fix
def register( self, name: str, graph: Any, description: str = "", - capabilities: List[str] = None, + capabilities: List[str] | None = None, ):
361-369: Regex JSON extraction may fail on complex JSON.The regex
r"\{.*\}"withre.DOTALLis greedy and will match from the first{to the last}in the response, which could include multiple JSON objects or malformed content. Consider using a more robust JSON extraction approach.♻️ Proposed fix for more robust JSON parsing
- json_match = re.search(r"\{.*\}", content, re.DOTALL) + # Use non-greedy match to get first complete JSON object + json_match = re.search(r"\{.*?\}", content, re.DOTALL) if json_match: - decision = json.loads(json_match.group()) + try: + decision = json.loads(json_match.group()) + except json.JSONDecodeError: + # Fallback to greedy match for nested objects + json_match = re.search(r"\{.*\}", content, re.DOTALL) + if json_match: + decision = json.loads(json_match.group())Alternatively, consider using a library like
json-repairor prompting the LLM to output only JSON.
| operators = { | ||
| ast.Add: op.add, | ||
| ast.Sub: op.sub, | ||
| ast.Mult: op.mul, | ||
| ast.Div: op.truediv, | ||
| ast.Pow: op.pow, | ||
| ast.UnaryOp: op.neg, | ||
| } | ||
|
|
||
| def _eval(node): | ||
| if isinstance(node, ast.Constant): | ||
| return node.value | ||
| elif isinstance(node, ast.BinOp): | ||
| return operators[type(node.op)](_eval(node.left), _eval(node.right)) | ||
| return operators[type(node.op)]( | ||
| _eval(node.left), _eval(node.right) | ||
| ) | ||
| elif isinstance(node, ast.UnaryOp): | ||
| return operators[type(node.op)](_eval(node.operand)) | ||
| else: |
There was a problem hiding this comment.
Unary minus will raise a KeyError in the calculator.
operators maps ast.UnaryOp, but lookup uses type(node.op) (e.g., ast.USub). This breaks -1/+1. Add explicit unary op keys.
🛠️ Suggested fix
operators = {
ast.Add: op.add,
ast.Sub: op.sub,
ast.Mult: op.mul,
ast.Div: op.truediv,
ast.Pow: op.pow,
- ast.UnaryOp: op.neg,
+ ast.USub: op.neg,
+ ast.UAdd: op.pos,
}Also applies to: 221-221
🤖 Prompt for AI Agents
In `@backend/examples/kaggle_integration.py` around lines 197 - 215, The unary
operator handling in the calculator is wrong: the operators dict currently
contains ast.UnaryOp as a key but the _eval unary branch looks up type(node.op)
(e.g., ast.USub/ast.UAdd), causing a KeyError for -1/+1; update the operators
mapping to include ast.USub: operator.neg and ast.UAdd: operator.pos (or a no-op
for UAdd) instead of ast.UnaryOp, and ensure the unary branch in _eval continues
to call operators[type(node.op)](_eval(node.operand)); adjust the same
mapping/lookup at the other occurrence mentioned (around the second location) so
both unary + and - are handled correctly.
| while finish_reason == "length": | ||
| continuation = llm.invoke("Please continue from where you left off:\n" + final_answer[-500:]) | ||
| continuation = llm.invoke( | ||
| "Please continue from where you left off:\n" + final_answer[-500:] | ||
| ) | ||
| final_answer += "\n" + getattr(continuation, "content", str(continuation)) | ||
| response_metadata = getattr(continuation, "response_metadata", {}) | ||
| finish_reason = response_metadata.get("finish_reason") | ||
| time.sleep(2) |
There was a problem hiding this comment.
Potential infinite loop if LLM consistently returns truncated responses.
The while finish_reason == "length" loop lacks an iteration limit. If the LLM keeps returning truncated responses (e.g., due to a bug or edge case), this could loop indefinitely.
🛡️ Proposed fix: Add iteration limit
+ max_continuations = 5
+ continuation_count = 0
while finish_reason == "length":
+ if continuation_count >= max_continuations:
+ logger.warning("Max continuations reached, returning partial answer")
+ break
+ continuation_count += 1
continuation = llm.invoke(
"Please continue from where you left off:\n" + final_answer[-500:]
)
final_answer += "\n" + getattr(continuation, "content", str(continuation))
response_metadata = getattr(continuation, "response_metadata", {})
finish_reason = response_metadata.get("finish_reason")
time.sleep(2)🤖 Prompt for AI Agents
In `@backend/src/agent/_graph.py` around lines 330 - 337, The loop that retries
when finish_reason == "length" can spin forever; modify the retry logic inside
the while loop that calls llm.invoke(...) (using symbols finish_reason,
llm.invoke, final_answer, response_metadata) to enforce a maximum number of
continuation attempts (e.g., max_retries) and increment a counter each
iteration, and when the counter exceeds the limit either break and return the
current final_answer or raise a controlled exception; also ensure you still
extract continuation content via getattr(continuation, "content",
str(continuation)) and update response_metadata as before, and optionally log a
warning when the retry limit is hit.
| if field_type == bool or ( | ||
| hasattr(field_type, "__origin__") and field_type.__origin__ == bool | ||
| ): | ||
| if isinstance(value, str): | ||
| value = value.lower() in ('true', '1', 'yes', 'on') | ||
| value = value.lower() in ("true", "1", "yes", "on") | ||
| else: | ||
| value = bool(value) | ||
|
|
||
| # Handle integer fields | ||
| elif field_type == int or (hasattr(field_type, '__origin__') and field_type.__origin__ == int): | ||
| elif field_type == int or ( | ||
| hasattr(field_type, "__origin__") and field_type.__origin__ == int | ||
| ): | ||
| if isinstance(value, str): | ||
| value = int(value) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use is instead of == for type comparisons.
Static analysis correctly flags that type comparisons should use is (identity) rather than == (equality). While == often works, is is the idiomatic and more reliable approach for type checks.
🔧 Proposed fix
# Handle boolean fields
- if field_type == bool or (
- hasattr(field_type, "__origin__") and field_type.__origin__ == bool
+ if field_type is bool or (
+ hasattr(field_type, "__origin__") and field_type.__origin__ is bool
):
if isinstance(value, str):
value = value.lower() in ("true", "1", "yes", "on")
else:
value = bool(value)
# Handle integer fields
- elif field_type == int or (
- hasattr(field_type, "__origin__") and field_type.__origin__ == int
+ elif field_type is int or (
+ hasattr(field_type, "__origin__") and field_type.__origin__ is int
):
if isinstance(value, str):
value = int(value)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if field_type == bool or ( | |
| hasattr(field_type, "__origin__") and field_type.__origin__ == bool | |
| ): | |
| if isinstance(value, str): | |
| value = value.lower() in ('true', '1', 'yes', 'on') | |
| value = value.lower() in ("true", "1", "yes", "on") | |
| else: | |
| value = bool(value) | |
| # Handle integer fields | |
| elif field_type == int or (hasattr(field_type, '__origin__') and field_type.__origin__ == int): | |
| elif field_type == int or ( | |
| hasattr(field_type, "__origin__") and field_type.__origin__ == int | |
| ): | |
| if isinstance(value, str): | |
| value = int(value) | |
| if field_type is bool or ( | |
| hasattr(field_type, "__origin__") and field_type.__origin__ is bool | |
| ): | |
| if isinstance(value, str): | |
| value = value.lower() in ("true", "1", "yes", "on") | |
| else: | |
| value = bool(value) | |
| # Handle integer fields | |
| elif field_type is int or ( | |
| hasattr(field_type, "__origin__") and field_type.__origin__ is int | |
| ): | |
| if isinstance(value, str): | |
| value = int(value) |
🧰 Tools
🪛 Ruff (0.14.14)
[error] 79-79: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
[error] 80-80: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
[error] 88-88: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
[error] 89-89: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
🤖 Prompt for AI Agents
In `@backend/src/agent/configuration.py` around lines 79 - 92, The type-comparison
logic in the boolean and integer conversion branches uses equality (==) instead
of identity; update the checks so that comparisons use "is" (e.g., change
"field_type == bool" to "field_type is bool" and "field_type.__origin__ == bool"
to "field_type.__origin__ is bool", likewise for int) while keeping the existing
hasattr(field_type, "__origin__") guards and the rest of the conversion logic
(references: field_type, value).
| def scoping_router(state: OverallState) -> str: | ||
| """Route based on scoping status.""" | ||
| if state.get("scoping_status") == "active": | ||
| return "planning_wait" # Reusing planning_wait to pause for user input | ||
| return "planning_wait" # Reusing planning_wait to pause for user input | ||
| return "generate_plan" | ||
|
|
||
|
|
||
| builder.add_conditional_edges( | ||
| "scoping_node", scoping_router, ["planning_wait", "outline_gen"] |
There was a problem hiding this comment.
Critical: scoping_router return value does not match conditional edge destinations.
The router returns "generate_plan" (line 96), but the conditional edges specify only ["planning_wait", "outline_gen"] as valid destinations (line 100). This will cause a ValueError at runtime when the scoping status is not "active".
🐛 Proposed fix
def scoping_router(state: OverallState) -> str:
"""Route based on scoping status."""
if state.get("scoping_status") == "active":
return "planning_wait" # Reusing planning_wait to pause for user input
- return "generate_plan"
+ return "outline_gen"
builder.add_conditional_edges(
"scoping_node", scoping_router, ["planning_wait", "outline_gen"]
)🤖 Prompt for AI Agents
In `@backend/src/agent/graph.py` around lines 92 - 100, The scoping_router
function returns "generate_plan" when scoping_status != "active", but
builder.add_conditional_edges for "scoping_node" only lists destinations
["planning_wait", "outline_gen"], causing a runtime ValueError; fix by making
scoping_router return one of the declared destinations (e.g., return
"outline_gen" instead of "generate_plan") or update the
builder.add_conditional_edges call to include "generate_plan" as a valid
destination so the return values of scoping_router and the conditional edge list
are consistent.
| with open(output_path, "w") as f: | ||
| json.dump( | ||
| { | ||
| "final_scores": final_scores, | ||
| "detailed_results": all_results if save_detailed else [], | ||
| }, | ||
| f, | ||
| indent=2, | ||
| ) |
There was a problem hiding this comment.
Missing explicit file encoding for output.
Same issue as the input file - specify encoding="utf-8" for consistent cross-platform behavior.
🛡️ Proposed fix
- with open(output_path, "w") as f:
+ with open(output_path, "w", encoding="utf-8") as f:
json.dump(📝 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.
| with open(output_path, "w") as f: | |
| json.dump( | |
| { | |
| "final_scores": final_scores, | |
| "detailed_results": all_results if save_detailed else [], | |
| }, | |
| f, | |
| indent=2, | |
| ) | |
| with open(output_path, "w", encoding="utf-8") as f: | |
| json.dump( | |
| { | |
| "final_scores": final_scores, | |
| "detailed_results": all_results if save_detailed else [], | |
| }, | |
| f, | |
| indent=2, | |
| ) |
🤖 Prompt for AI Agents
In `@backend/src/evaluation/bench.py` around lines 132 - 140, The file write uses
open(output_path, "w") without an explicit encoding which can cause
cross-platform inconsistencies; update the open call in
backend/src/evaluation/bench.py (the open(...) that writes the JSON with
json.dump and variables final_scores/all_results) to include encoding="utf-8" so
the JSON is written using UTF-8 consistently.
| region: str | None = None, | ||
| time_range: str | None = None, | ||
| safe_search: bool = True, | ||
| tuned: bool = True, | ||
| ) -> List[SearchResult]: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n backend/src/search/providers/brave_adapter.pyRepository: MasumRab/gemini-fullstack-langgraph-quickstart
Length of output: 2947
🌐 Web query:
Brave Search API region country filtering parameters documentation
💡 Result:
Brave Search API supports country/region targeting in two main ways:
- API query parameters (recommended)
country: targets results for a specific country using a 2‑character country code (ISO 3166-1 alpha-2 style, e.g.US,DE). Brave’s docs show this under “Country and Language Targeting” and example usage like...&country=DE&search_lang=de. [1]search_lang: language preference for results (e.g.en,de). [1]ui_lang: preferred language for response metadata. [1]
- In-query “search operators” (inside the
qstring)
loc:/location:: filters to pages from a specific country/region, using ISO 3166-1 alpha‑2 codes (e.g.loc:ca). [2]lang:/language:: filters to content written in a specific language (ISO 639-1 two-letter, e.g.lang:es). [2]
Example:
curl "https://api.search.brave.com/res/v1/web/search?q=niagara%20falls%20loc:ca%20lang:en&country=CA&search_lang=en" \
-H "X-Subscription-Token: <YOUR_API_KEY>"Sources: Brave Web Search docs (“Country and Language Targeting”) [1], Brave “Search operators” docs (loc: / lang:) [2].
region parameter is accepted but never used in the request.
The region parameter is defined in the method signature (line 26) but is not added to the request params. The Brave Search API supports region filtering via the country query parameter (ISO 3166-1 alpha-2 format, e.g., US, DE). Map region to params["country"] similar to how time_range is mapped to freshness (lines 46-49).
🤖 Prompt for AI Agents
In `@backend/src/search/providers/brave_adapter.py` around lines 26 - 30, The
region parameter in the search method is accepted but never used; update the
request params in brave_adapter.py (the method that takes region: str | None) to
map region to the Brave API country query parameter (params["country"]) similar
to how time_range maps to params["freshness"]; only set params["country"] when
region is provided and normalize it to an ISO 3166-1 alpha-2 format (e.g.,
uppercased) before adding.
| from unittest.mock import Mock, patch | ||
|
|
||
| import pytest | ||
| from unittest.mock import Mock, patch, MagicMock | ||
| from agent.nodes import generate_plan, web_research, reflection, denoising_refiner, load_context | ||
| from langchain_core.messages import HumanMessage, AIMessage | ||
| from langchain_core.messages import AIMessage, HumanMessage | ||
|
|
||
| from agent.models import TEST_MODEL | ||
| from agent.nodes import ( | ||
| denoising_refiner, | ||
| generate_plan, | ||
| load_context, | ||
| reflection, | ||
| web_research, | ||
| ) | ||
|
|
||
| TEST_MODEL = "gemma-3-27b-it" |
There was a problem hiding this comment.
Avoid shadowing imported TEST_MODEL.
The local constant redefines the imported name and will trip linting (shadowing/redefinition). Drop the import or remove the local override.
🛠️ Proposed fix
-from agent.models import TEST_MODEL📝 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.
| from unittest.mock import Mock, patch | |
| import pytest | |
| from unittest.mock import Mock, patch, MagicMock | |
| from agent.nodes import generate_plan, web_research, reflection, denoising_refiner, load_context | |
| from langchain_core.messages import HumanMessage, AIMessage | |
| from langchain_core.messages import AIMessage, HumanMessage | |
| from agent.models import TEST_MODEL | |
| from agent.nodes import ( | |
| denoising_refiner, | |
| generate_plan, | |
| load_context, | |
| reflection, | |
| web_research, | |
| ) | |
| TEST_MODEL = "gemma-3-27b-it" | |
| from unittest.mock import Mock, patch | |
| import pytest | |
| from langchain_core.messages import AIMessage, HumanMessage | |
| from agent.nodes import ( | |
| denoising_refiner, | |
| generate_plan, | |
| load_context, | |
| reflection, | |
| web_research, | |
| ) | |
| TEST_MODEL = "gemma-3-27b-it" |
🧰 Tools
🪛 Ruff (0.14.14)
[error] 15-15: Redefinition of unused TEST_MODEL from line 6: TEST_MODEL redefined here
(F811)
🤖 Prompt for AI Agents
In `@backend/tests/test_graph_mock.py` around lines 1 - 15, The test currently
redefines the imported symbol TEST_MODEL by declaring a local constant
TEST_MODEL = "gemma-3-27b-it", which shadows the imported value; remove the
local override or remove the import so only one definition remains (e.g., delete
the local TEST_MODEL assignment or stop importing TEST_MODEL from agent.models)
and update any references in the test to use the single remaining symbol to
avoid redefinition/shadowing.
| # MOCK google.genai BEFORE importing search.router to avoid broken environment dependencies | ||
| # (e.g. pycares/aiohttp issues in current env) | ||
| sys.modules["google.genai"] = MagicMock() | ||
|
|
||
| from search.router import SearchRouter, SearchProviderType | ||
| from search.provider import SearchResult | ||
| from search.router import SearchRouter | ||
|
|
There was a problem hiding this comment.
Ruff E402 will fail unless these imports are exempted.
Since the sys.modules patch must precede the imports, add a local noqa to silence E402.
🩹 Suggested fix
-from search.provider import SearchResult
-from search.router import SearchRouter
+from search.provider import SearchResult # noqa: E402
+from search.router import SearchRouter # noqa: E402📝 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.
| # MOCK google.genai BEFORE importing search.router to avoid broken environment dependencies | |
| # (e.g. pycares/aiohttp issues in current env) | |
| sys.modules["google.genai"] = MagicMock() | |
| from search.router import SearchRouter, SearchProviderType | |
| from search.provider import SearchResult | |
| from search.router import SearchRouter | |
| # MOCK google.genai BEFORE importing search.router to avoid broken environment dependencies | |
| # (e.g. pycares/aiohttp issues in current env) | |
| sys.modules["google.genai"] = MagicMock() | |
| from search.provider import SearchResult # noqa: E402 | |
| from search.router import SearchRouter # noqa: E402 |
🧰 Tools
🪛 Ruff (0.14.14)
[error] 19-19: Module level import not at top of file
(E402)
[error] 20-20: Module level import not at top of file
(E402)
🤖 Prompt for AI Agents
In `@backend/tests/test_search_router.py` around lines 15 - 21, The import order
patching sys.modules["google.genai"] must stay before importing SearchResult and
SearchRouter, so silence Ruff E402 by adding a local noqa on the affected import
lines: append " # noqa: E402" to the lines that import SearchResult (from
search.provider import SearchResult) and SearchRouter (from search.router import
SearchRouter) so the sys.modules mock can precede those imports without a linter
failure.
|
Hi @jules, could you please take care of all the conflicts that are currently present? Also, can you please make sure your code is updated to the very latest version that's currently in the main branch? Finally, could you please fix those actions that are currently failing on PR # |
I've reverted the broad formatting changes that were likely causing conflicts and noise. The PR now only contains the necessary security fixes for the RateLimitMiddleware and the corresponding tests. |
…oxy headers The RateLimitMiddleware previously trusted the `X-Forwarded-For` header by default, allowing attackers to bypass rate limits by spoofing their IP address. This change: 1. Adds `trust_proxy_headers` configuration to `AppConfig` (default: False). 2. Updates `RateLimitMiddleware` to ignore `X-Forwarded-For` unless `trust_proxy_headers` is True. 3. Updates `app.py` to pass the configuration to the middleware. 4. Adds regression tests to verify secure behavior by default and correct proxy handling when enabled. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
Closing as redundant; IP spoofing vulnerability already fixed in PR #331. |
Understood. Acknowledging that this work is now redundant and stopping work on this task. |
…oxy headers The RateLimitMiddleware previously trusted the `X-Forwarded-For` header by default, allowing attackers to bypass rate limits by spoofing their IP address. This change: 1. Adds `trust_proxy_headers` configuration to `AppConfig` (default: False). 2. Updates `RateLimitMiddleware` to ignore `X-Forwarded-For` unless `trust_proxy_headers` is True. 3. Updates `app.py` to pass the configuration to the middleware. 4. Adds regression tests to verify secure behavior by default and correct proxy handling when enabled. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
@jules Resolve conflicts: Avoid full repo diff - focus only on your changed paths. |
|
@google-labs-jules STOP - PR #310 is NOT redundant! The IP spoofing vulnerability fix is unique work. If this overlaps with PR #331, we should merge BOTH - security fixes are additive, not redundant. Please:
Branch: sentinel/fix-ip-spoofing-ratelimit-2166097941151715551 |
1 participant
Security Fix: Prevent IP Spoofing in Rate Limiter
Vulnerability:
The
RateLimitMiddlewareblindly trusted theX-Forwarded-Forheader to determine the client IP. This allowed malicious actors to bypass rate limits by sending a spoofedX-Forwarded-Forheader (e.g.,X-Forwarded-For: <random_ip>).Fix:
trust_proxy_headers(env:TRUST_PROXY_HEADERS), defaulting toFalse.RateLimitMiddlewarenow only usesX-Forwarded-Forif this configuration is explicitly enabled.request.client.host).Verification:
backend/tests/test_proxy_security.pywhich verifies:backend/tests/agent/test_rate_limiter_proxy.pyto explicitly enable proxy trust, preserving test coverage for the parsing logic.PR created automatically by Jules for task 2166097941151715551 started by @MasumRab
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.